airvzxf / airvzxf/moagan

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

Closed
#924 0 comments 0 reactions 0 assignees View on GitHub
area:llm enhancement priority:P1 size:L
Dominant language
Rust
Stars
0
Forks
1
Avg merge
29m
Merged PRs (30d)
246

Description

## 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`).

Contributor guide

Open the contributing guide

Research direction

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.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
ai, backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
66/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.