airvzxf / airvzxf/moagan

refactor(llm): migrate phase.rs explicit-pair dispatch (dispatch_to_provider_for + cascade loops)

Đã đóng
#924 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
area:llm enhancement priority:P1 size:L
Ngôn ngữ chính
Rust
Star
0
Fork
1
Merge trung bình
29 phút
Pull request đã merge (30 ngày)
246

Mô tả

## Goal

Migrate `src/phases/phase.rs::dispatch_to_provider_for` (the **explicit-pair** dispatch — takes `section` and `model_id` as parameters, used by the discovery matrix / coordinator for multi-provider fan-out) from `Arc` to `Arc`. Includes the two cascade loops at `phase.rs:1968-1985` (preflight) and `:2011-2057` (retry). Lands alongside the existing `Provider`-using code path.

## Why now

The explicit-pair dispatch is the call-site used by the discovery subsystem (`src/discovery/coordinator.rs`, `src/discovery/matrix.rs`, etc.) when fanning out across multiple `(provider, model)` pairs. It mirrors `dispatch_to_provider` structurally — same cascade pattern, just with caller-supplied routing instead of `self.default_provider`. Migrating it after #5 (default-pair) is the natural next step because both functions share the same `BreakeredClient` adapter and the same `convert_request`/`convert_response` helpers from #5.

## Scope

**In scope:**
- `phase.rs::dispatch_to_provider_for` (lines 1898-2244): switch from `Arc` to `Arc`.
- The two cascade loops in this function (preflight at `:1968-1985`, retry at `:2011-2057`): same shape as #5's migration.
- The audit-hash branch at `phase.rs:1994` (`if section == "minimax"`) **stays** — #14 removes it.
- The `RunContext::provider_for(section, model_id)` accessor gets a sibling `RunContext::llm_client_for(section, model_id)` that returns `Arc`. The old accessor stays.
- New unit tests at `phase.rs::tests::v2_for::*` that pin the explicit-pair cascade contract.

**Out of scope:**
- Default-pair dispatch migration — issue #5.
- Migration of probe / CLI / other phases / discovery / tests — issues #7-#11.
- Cascade absorption into `LlmClient::send` (D9) — issue #14.
- Removal of the `if section == "minimax"` audit-hash branch (D8) — issue #14.
- Deletion of `dispatch_to_provider_for`'s legacy `Provider`-based code path — issue #15.

## Approach

### 1. `llm_client_for` accessor

```rust
impl RunContext {
pub(crate) async fn llm_client_for(&self, section: &str, model_id: &str) -> Arc {
// Resolves (section, model_id) via the same lookup path as provider_for(),
// wraps the result in a BreakeredClient adapter (introduced in #5).
...
}
}
```

Same dual-path strategy as #5: both `provider_for` and `llm_client_for` exist; `llm_client_for` wraps the legacy `BreakeredProvider` in `BreakeredClient`.

### 2. `dispatch_to_provider_for` migration

The function signature changes from:

```rust
async fn dispatch_to_provider_for(&self, section, model_id, req, cache_key, started_unix, retry_count) -> Result
```

to:

```rust
async fn dispatch_to_provider_for(&self, section, model_id, req, cache_key, started_unix, retry_count) -> Result
```

Internally:
- `self.provider_for(section, model_id)` → `self.llm_client_for(section, model_id)`.
- `provider.send(&hash_input)` → `client.send(&hash_input)`.
- The 4xx cascade loop now calls `client.send` (no section-name branching).
- The `if section == "minimax"` audit-hash branch **stays** (D8 = #14).
- All 4 `provider.effective_max_tokens(&req)` calls drop in favour of `client.body_sha256(&req)`.

### 3. Unit tests migration

Add 3 new tests in `phase.rs::tests::v2_for::*`:

- `dispatch_for_recovers_from_three_param_cascade` — mirror of the existing `dispatch_recovers_from_three_param_cascade` (which tests default-pair), but for explicit-pair.
- `dispatch_for_aborts_when_detector_returns_none` — mirror.
- `dispatch_for_caps_at_param_names_len` — mirror.

Each uses a `ScriptedLlmClient` stub that returns programmable 4xx bodies.

The legacy explicit-pair tests (using `ScriptedProvider`) stay green throughout — both code paths run until #15.

## Acceptance criteria

- [ ] `cargo build --release --all-features` succeeds with **zero warnings**.
- [ ] `make fmt-check guard-deps lint build test-ci` green.
- [ ] `phase.rs::dispatch_to_provider_for` is fully migrated to `Arc`.
- [ ] The 3 new `v2_for::*` unit tests pass.
- [ ] The legacy `ScriptedProvider`-based explicit-pair tests still pass.
- [ ] The integration tests in `tests/integration_phase_*.rs`, `tests/integration_discover_legacy_paths.rs`, `tests/integration_pr09_provider_pool.rs` continue to pass.
- [ ] Smoke: `moagan run --mode fast --provider mock:mock-model` produces `final/portfolio.md` and `rankings/ranking.json` byte-identical to v0.17.6.
- [ ] The `Provider`-using helpers (`provider_for`, `BreakeredProvider` direct construction) are untouched in the legacy code paths.

## Merge order

```
#1-#4 (foundation wave)

#5 (phase.rs default-pair migration)

#6 (this issue — phase.rs explicit-pair migration — parallel to #5 is fine, but #6 should land after #5's BreakeredClient adapter exists)

#7-#11 (other migrations)
```

Can be merged in parallel with #5 if both PRs agree on the `BreakeredClient` API surface. Practically: ship #5 first, then #6.

## Validation

- `make fmt-check guard-deps lint build test-ci` green after the PR lands.
- 3 new `v2_for::*` unit tests in `phase.rs::tests` pass.
- Legacy `ScriptedProvider`-based tests still pass.
- Smoke (`moagan run --mode fast --provider mock:mock-model`) produces the same artefacts.

## Version target

v0.18.0.

## References

- [EPIC #847](https://github.com/airvzxf/moagan/issues/847) — the umbrella.
- [#900 D8, D9](https://github.com/airvzxf/moagan/issues/900) — D8 + D9 deferred to #14.
- [`src/phases/phase.rs:1898-2244`](../blob/main/src/phases/phase.rs) — `dispatch_to_provider_for` and its cascade loops.
- [`src/discovery/coordinator.rs`](../blob/main/src/discovery/coordinator.rs) — primary caller of `dispatch_to_provider_for` (via `call_with_retry_at_temp_for`).

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Start in src/phases/phase.rs:1898-2244 and compare the explicit-pair path with the default-pair migration from #5. Trace RunContext::provider_for, the two cascade loops, and the existing phase.rs tests before running the listed make checks. Done means the LlmClient path and three v2_for tests pass, legacy tests remain green, and the smoke and integration checks succeed.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
rust
Lĩnh vực
ai, backend
Loại issue
Tái cấu trúc
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
66/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.