airvzxf / airvzxf/moagan

refactor(llm): absorb cascade loops into LlmClient::send (D9) + kill audit-hash hack (D8)

Open
#932 0 comments 0 reactions 0 assignees View on GitHub
area:llm enhancement priority:P1 size:M
Dominant language
Rust
Stars
0
Forks
1
Avg merge
35m
Merged PRs (30d)
238

Description

## Goal

Land the two **architectural cleanup** items from #900 D8 and D9:

- **D8:** Delete the `if minimax` audit-hash hack from `src/phases/phase.rs:1536` and `:1994`. `LlmClient::body_sha256` becomes the single source of truth for the audit hash; no provider-name branching anywhere.
- **D9:** Absorb the 4 cascade loops (`phase.rs:1496-1527`, `:1607-1672`, `:1968-1985`, `:2011-2057`) into `LlmClient::send`. The 6 `call_*` methods in `phase.rs` become thin constructors of `LlmRequest`; the SDK handles preflight + retry.

## Why now

After the migration wave (#5-#11) finishes, every call site already speaks `LlmClient`. The cascade loops and the `if minimax` branch are the last architectural debt that ties the audit hash to a provider name and duplicates retry logic across 4 sites. Removing them is the single biggest simplification in the EPIC.

## Scope

**In scope:**

**D8 — audit-hash kill:**
- `src/phases/phase.rs:1536` — `let request_body_sha256 = (self.default_provider == "minimax").then(|| ...).transpose()?;` becomes `let request_body_sha256 = client.body_sha256(&hash_input)?;`.
- `src/phases/phase.rs:1994` — `let request_body_sha256 = (section == "minimax").then(|| ...).transpose()?;` becomes `let request_body_sha256 = client.body_sha256(&hash_input)?;`.
- Remove the `crate::llm::http::request_body_sha256` call site from `phase.rs` (the function stays in `http.rs` until #15 deletes the legacy code that calls it).
- Update the regression tests at `phase.rs:5050-5200` to assert the audit hash comes from `LlmClient::body_sha256`, not from a provider-name branch.

**D9 — cascade absorption:**
- `LlmClient::send` (in `src/llm/client/mod.rs`, introduced in #1) gains the cascade preflight + retry logic that currently lives at `phase.rs:1496-1527, :1607-1672, :1968-1985, :2011-2057`.
- The 4 cascade loops in `phase.rs` collapse into a single call: `client.send(&req)`.
- The `PARAM_NAMES` constant stays in `src/llm/param_rejections.rs:68` (still used by `detect_all_rejections`), but `phase.rs` no longer iterates it — the SDK impl iterates it internally.
- The `ParamRejectionsTable::record(...)` call (currently in the cascade retry) moves into the SDK impl. `phase.rs` no longer touches the table directly.
- Update the 4 cascade-loop regression tests at `phase.rs:5050-5200` to assert the cascade runs inside `LlmClient::send` (via a `ScriptedLlmClient` that returns 4xx-then-200).

**Out of scope:**
- `LlmClient` trait definition / Mock / Anthropic / OpenAI impls — issues #1-#3.
- Migration of `phase.rs` to `LlmClient` — issues #5 + #6.
- Discovery / CLI / integration test migration — issues #7-#11.
- Legacy `Provider` deletion — issue #15.

## Approach

### 1. D8 — audit-hash kill (small change)

Two 3-line edits in `phase.rs`:

```rust
// Before (line 1536):
let request_body_sha256 = (self.default_provider == "minimax")
.then(|| crate::llm::http::request_body_sha256(&hash_input))
.transpose()?;

// After:
let request_body_sha256 = client.body_sha256(&hash_input)?;
```

Same edit at line 1994 with `section` instead of `self.default_provider`.

Verify the audit hash produced by `LlmClient::body_sha256` matches what the proxy captures for the wire body — this is the contract that the new SDK impls (#2, #3) preserve.

### 2. D9 — cascade absorption (bigger change)

The 4 cascade loops collapse into one place inside `LlmClient::send`. The implementation:

```rust
// In src/llm/client/mod.rs (added to the trait default):
async fn send(&self, req: &LlmRequest) -> Result {
let mut hash_input = req.clone();
// Preflight: omit known-rejected params from the table.
if let Some(table) = self.param_rejections_table() {
for param in PARAM_NAMES {
if table.should_omit(self.name(), self.model(), param) {
crate::llm::wire::omit_param(&mut hash_input, param);
}
}
}
// Initial send.
let mut result = self.send_once(&hash_input).await;
// Cascade retry: on 4xx, detect rejections, persist + omit, retry.
let max_rejection_retries = PARAM_NAMES.len();
let mut rejection_attempts = 0;
while rejection_attempts < max_rejection_retries {
let status = match result.as_ref().err().and_then(|e| e.http_status()) {
Some(s) if (400..500).contains(&s) => s,
_ => break,
};
let body = parse_provider_error_body(result.as_ref().expect_err("status set"), status);
let detected = detect_all_rejections(status, body.as_ref());
if detected.is_empty() { break; }
for detected_param in &detected {
if let Some(table) = self.param_rejections_table() {
let _ = table.record(self.name(), self.model(), detected_param);
}
crate::llm::wire::omit_param(&mut hash_input, detected_param);
}
rejection_attempts += 1;
result = self.send_once(&hash_input).await;
if result.is_ok() { break; }
}
result
}
```

The new `send_once` method is the **bare** HTTP call (no preflight, no retry) — every concrete impl (`AnthropicClient`, `OpenAIClient`, `MockClient`) implements `send_once` instead of `send`. The default `send` method provides the cascade.

This means:

- `MockClient::send_once` returns a programmed response.
- `AnthropicClient::send_once` makes the HTTP call + applies the max-tokens clamp + computes the wire body.
- `OpenAIClient::send_once` does the same.

The 4 cascade loops in `phase.rs` collapse:

```rust
// Before (cascade loops at 1602-1672):
let mut result = provider.send(&hash_input).await;
let max_rejection_retries = PARAM_NAMES.len();
let mut rejection_attempts = 0;
while rejection_attempts < max_rejection_retries {
// ... 70 lines of cascade logic ...
}

// After:
let result = client.send(&req).await;
```

`phase.rs::dispatch_to_provider` and `dispatch_to_provider_for` shrink by ~70 lines each.

### 3. Test migration

The 4 cascade-loop tests at `phase.rs:5050-5200` move into `src/llm/client/mod.rs::tests`:

- `cascade_recovers_from_three_param_cascade` — uses `ScriptedLlmClient` that returns 4xx-then-200.
- `cascade_aborts_when_detector_returns_none` — 4xx body has no rejection signature.
- `cascade_caps_at_param_names_len` — pathological upstream loops 5 times; cascade stops at 3.
- `cascade_persists_to_param_rejections_table` — verify `ParamRejectionsTable::record` is called.

The legacy `phase.rs` tests stay green throughout — they exercise the `phase.rs::dispatch_to_provider` path which still works (it just calls `client.send` instead of the inlined cascade).

## Acceptance criteria

- [ ] `cargo build --release --all-features` succeeds with **zero warnings**.
- [ ] `make fmt-check guard-deps lint build test-ci` green.
- [ ] Zero matches for `if.*default_provider == "minimax"` in `src/` (verified by `rg 'minimax' src/phases/phase.rs` returning no conditional checks).
- [ ] Zero matches for `if.*section == "minimax"` in `src/`.
- [ ] `LlmClient::send` (the default impl in `src/llm/client/mod.rs`) handles the full cascade in one place. The 4 cascade loops in `phase.rs` collapse to one line: `client.send(&req).await`.
- [ ] `phase.rs::dispatch_to_provider` and `dispatch_to_provider_for` shrink by ~70 lines each (cascade logic deleted).
- [ ] The 4 cascade-loop tests pass against `ScriptedLlmClient` (now in `src/llm/client/mod.rs::tests`).
- [ ] The integration tests in `tests/integration_param_rejection_cascade.rs` continue to pass end-to-end.
- [ ] The audit hash produced by `LlmClient::body_sha256` matches the proxy's wire-body SHA-256 for every test case — verified by a new parity test.
- [ ] Smoke: `moagan run --mode fast --provider mock:mock-model` produces the same artefacts as v0.17.6.

## Merge order

```
#1-#11 (foundation + migration wave)
#12-#13 (optional features before cleanup)

#14 (this issue — D8 + D9 cleanup)

#15 (legacy deletion — final cleanup)
```

## Validation

- `make fmt-check guard-deps lint build test-ci` green after the PR lands.
- 4 new cascade tests in `src/llm/client/mod.rs::tests` pass.
- Legacy phase.rs cascade tests still pass.
- Parity test confirms `LlmClient::body_sha256` ≡ proxy SHA-256.
- Smoke runs identically to v0.17.6.

## 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 (no compat layer), D9 (cascade absorbed in `send`).
- [`src/phases/phase.rs:1496-1527, :1607-1672, :1968-1985, :2011-2057`](../blob/main/src/phases/phase.rs) — the 4 cascade loops that collapse.
- [`src/phases/phase.rs:5050-5200`](../blob/main/src/phases/phase.rs) — the 4 cascade-loop regression tests.
- [`src/llm/param_rejections.rs:68`](../blob/main/src/llm/param_rejections.rs) — `PARAM_NAMES` (still used by the new `LlmClient::send` default).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.