airvzxf / airvzxf/moagan

refactor(llm): migrate probe subsystem (probe.rs + temperature_probe.rs ProviderProbeTransport)

Closed
#925 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
29m
Merged PRs (30d)
246

Description

## Goal

Migrate the **probe subsystem** — `src/llm/probe.rs` and `src/llm/temperature_probe.rs` — from `Arc` to `Arc`. Both files define a `ProviderProbeTransport` (or `ProviderTemperatureProbeTransport`) adapter that wraps a `Provider` for the auto-probe algorithm. Migrate the wrappers to take `Arc` instead, keeping the algorithm bodies unchanged.

## Why now

The probe subsystem runs **outside** the normal `dispatch_to_provider` path: it directly calls `provider.send_probe(...)` on the underlying client to bypass the safety wire-clamp. Until #5 + #6 land (which move `phase.rs` to `LlmClient`), `provider.rs` itself stays `Provider`-based, so the probe subsystem's adapter has nothing to wrap. But once the rest of the migration runs, the probe subsystem is the only `Provider::send_probe` consumer left outside `phase.rs` — making it the natural next migration target so the `ProbeTransport` trait can stay generic over a future `LlmClient::send_probe` instead of `Provider::send_probe`.

## Scope

**In scope:**
- `src/llm/probe.rs::ProviderProbeTransport` (struct at `:50-100`, impl at `:100-200`): wrap `Arc` instead of `Arc`.
- `src/llm/temperature_probe.rs::ProviderTemperatureProbeTransport` (struct at `:362-407`): same.
- The two `ProbeTransport` traits (one per file, used by the auto-probe algorithms) get a parallel set of methods that take `Arc` — or, more cleanly, the trait methods are generic and the algorithm body calls `client.send_probe(...)` directly.
- Tests at `src/llm/probe.rs::tests` and `src/llm/temperature_probe.rs::tests` migrate their `ScriptedProvider` test stubs to `ScriptedLlmClient`.

**Out of scope:**
- Migration of `phase.rs` — issues #5 + #6.
- Migration of CLI — issue #8.
- Migration of other phase files / discovery / integration tests — issues #9-#11.
- New `top_p_auto` / `top_k_auto` probes — issues #12 + #13.
- Cascade absorption (D9) / audit-hash kill (D8) — issue #14.
- Legacy `Provider` deletion — issue #15.

## Approach

### 1. `ProviderProbeTransport` → `LlmClientProbeTransport`

The struct fields change from:

```rust
pub struct ProviderProbeTransport<'a> {
pub provider: &'a Arc,
pub name: &'a str,
pub model: &'a str,
}
```

to:

```rust
pub struct LlmClientProbeTransport<'a> {
pub client: &'a Arc,
pub name: &'a str,
pub model: &'a str,
}
```

The `probe_send_with_body` method (the only one the algorithm calls) becomes:

```rust
async fn probe_send_with_body(&self, body: &serde_json::Value) -> Result<(u16, Response)> {
// Build a minimal LlmRequest from the probe body.
let req = LlmRequest::from_probe_body(body, self.name, self.model)?;
let resp = self.client.send_probe(&req).await?;
Ok((200, resp.into_legacy())) // 200 is a placeholder; the algorithm only inspects .text + .usage
}
```

Wait — the algorithm inspects `(status, response)` to detect probe outcomes. `LlmResponse` doesn't carry an HTTP status (the status is the outer `Result<(u16, Response)>` in the legacy API). Two options:

- **Option A (chosen):** keep the `(u16, Response)` return shape. `LlmResponse` gains an `http_status: u16` field for probe-internal use. Documented as probe-only; production `send` returns `LlmResponse` without it.

Actually that's adding complexity to `LlmResponse`. Simpler:

- **Option B (chosen):** `LlmClient::send_probe` returns `Result` (no status). The algorithm body is updated to inspect `LlmResponse.text` and `LlmResponse.finish_reason` for probe outcomes, not the HTTP status. The 4xx-vs-2xx distinction is irrelevant for the probe — only the response content matters (was the body accepted or rejected?).

This is a clean break with the legacy API and aligns with the new trait shape. The algorithm body's "status" check becomes a "did the call succeed?" check (`Result::is_ok()`).

### 2. `ProviderTemperatureProbeTransport` → `LlmClientTemperatureProbeTransport`

Same shape as #1. The temperature probe algorithm inspects `response.text` for rejection patterns like `"temperature"`, `"temperature_value"`, etc. — no HTTP status needed.

### 3. Tests migration

`src/llm/probe.rs::tests` has ~6 test cases using a `ScriptedProvider` stub. Each gets a parallel `ScriptedLlmClient` stub. Same for `temperature_probe.rs::tests` (~10 test cases).

The new test stubs live in `src/llm/client/test_stubs.rs` (new file, exported by `src/llm/client/mod.rs` under `#[cfg(test)]` so it doesn't bloat the release binary).

## Acceptance criteria

- [ ] `cargo build --release --all-features` succeeds with **zero warnings**.
- [ ] `make fmt-check guard-deps lint build test-ci` green.
- [ ] `src/llm/probe.rs` no longer imports `crate::llm::Provider`. All `provider.send_probe(...)` calls become `client.send_probe(...)`.
- [ ] `src/llm/temperature_probe.rs` no longer imports `crate::llm::Provider`.
- [ ] The probe algorithm tests (6 in `probe.rs`, 10 in `temperature_probe.rs`) pass against `ScriptedLlmClient`.
- [ ] Smoke: `moagan probe temperature --provider mock:mock-model` produces the same sidecar (`/temperatures_auto.toml`) as v0.17.6 for the same mock input.
- [ ] Smoke: `moagan probe max_tokens --provider mock:mock-model` produces the same sidecar (`/max_tokens_auto.toml`) as v0.17.6 for the same mock input.
- [ ] The CLI handler in `src/cli/probe.rs` is **untouched** in this issue — it still builds a `Provider` (via `build_provider_for_spec`); only the inner probe transport switches to `LlmClient`.

## Merge order

```
#1-#6 (foundation + phase.rs)

#7 (this issue — probe subsystem migration)

#8 (CLI migration)

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

Can be merged in parallel with #8 if both PRs agree on the test-stub API surface.

## Validation

- `make fmt-check guard-deps lint build test-ci` green after the PR lands.
- 16 test cases migrated (6 in probe.rs + 10 in temperature_probe.rs).
- Two smoke probes (`moagan probe temperature`, `moagan probe max_tokens`) produce v0.17.6-equivalent sidecars.

## Version target

v0.18.0.

## References

- [EPIC #847](https://github.com/airvzxf/moagan/issues/847) — the umbrella.
- [`src/llm/probe.rs:50-200`](../blob/main/src/llm/probe.rs) — `ProviderProbeTransport`.
- [`src/llm/temperature_probe.rs:362-407`](../blob/main/src/llm/temperature_probe.rs) — `ProviderTemperatureProbeTransport`.
- [`src/cli/probe.rs:692-759`](../blob/main/src/cli/probe.rs) — `build_provider_for_spec` (unchanged in this issue; migrated in #8).

Contributor guide

Open the contributing guide

Research direction

Start by reading src/llm/probe.rs and src/llm/temperature_probe.rs, then inspect the LlmClient and send_probe APIs from the prerequisite migration issues. Run the existing probe tests before changing the ScriptedProvider stubs. Done means both transports and their 16 tests use LlmClient, the listed validation commands pass, and both smoke probes preserve their v0.17.6-equivalent sidecars.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
ai, backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.