airvzxf / airvzxf/moagan

refactor(llm): migrate CLI (cli/probe.rs + cli/run.rs minimax short-circuit)

Đã đóng
#926 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
area:llm enhancement priority:P1 size:M
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 the **CLI boundary** — `src/cli/probe.rs` and `src/cli/run.rs` — from `Arc` to `Arc`. Both files construct `Provider` instances for CLI-side use (the `moagan probe ...` subcommand and the `moagan run ...` startup path). Migrate the construction sites and the per-section wrapper logic to use `LlmClient` and the new dispatcher (issue #4).

## Why now

After #7 migrates the inner probe subsystem, the only remaining `Provider::send` consumer outside `phase.rs` is the CLI. Migrating the CLI closes the `Provider`-using code surface and unblocks issue #15 (deletion of `Provider`).

## Scope

**In scope:**
- `src/cli/probe.rs::build_provider_for_spec` (lines 692-759): replace the section-name + `WireFormatId` `match` with a single call to `client::dispatcher::build_client` (introduced in #4).
- `src/cli/probe.rs::build_provider_for_probe` (lines 692-744): same.
- `src/cli/probe.rs::tests` (the test at `:737` that asserts the section-name branch): migrate to use `build_client` and assert via `sdk_type()`.
- `src/cli/run.rs::build_registry_for_with_active` (lines around `:936`): the `minimax` short-circuit that hand-rolls a `MinimaxProvider` when `--api-key` is supplied. Replace with a call to `client::dispatcher::build_client` that picks the SDK from the URL.
- `src/cli/telemetry_cmd.rs` (the test at `:2566-2569` that filters by provider name): migrate to filter by `sdk_type()` instead of `name`.

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

## Approach

### 1. `build_provider_for_spec` → `build_client_for_spec`

The 30-line `match` at `src/cli/probe.rs:728-744` collapses to:

```rust
fn build_client_for_spec(spec: &ProviderConfig, model_id: &str, api_key: SecretString) -> Result> {
let endpoint = spec.endpoint.as_deref()
.or_else(|| /* fallback to model_id's endpoint */)
.ok_or_else(|| Error::InvalidArgs(format!("provider section '{}' has no endpoint", spec.name)))?;
client::dispatcher::build_client(endpoint, &spec.name, model_id, api_key, spec)
}
```

This deletes the section-name special cases (`minimax`, `deepseek`) — D2 says no `sdk` knob, and the URL alone decides. The two `minimax` / `deepseek` kind-cap field initialisations that the legacy code inlined (`MINIMAX_MAX_TOKENS_CAP`, `DEEPSEEK_MAX_TOKENS_CAP`) move into the `AnthropicClient::new` / `OpenAIClient::new_chat` constructors (issues #2 + #3).

### 2. `build_registry_for_with_active` → drop the minimax short-circuit

The `--api-key` flag's minimax short-circuit at `src/cli/run.rs:936`:

```rust
if section == "minimax" && api_key.is_some() {
let resolved = cfg.resolved_model(§ion, &model_id)?;
let provider = crate::llm::minimax::MinimaxProvider::from_resolved(&resolved)?;
...
}
```

becomes:

```rust
if let Some(api_key) = api_key {
// Build via the dispatcher regardless of section name.
let resolved = cfg.resolved_model(§ion, &model_id)?;
let endpoint = resolved.endpoint.as_deref()
.ok_or_else(|| Error::InvalidArgs(format!("provider section '{}' has no endpoint", section)))?;
let client = client::dispatcher::build_client(endpoint, §ion, &model_id, api_key, &spec)?;
// Wrap + insert into registry.
...
}
```

The section-name special case goes away — the URL decides, per D2.

### 3. `telemetry_cmd.rs` test migration

The test at `src/cli/telemetry_cmd.rs:2566-2569`:

```rust
let minimax = rows.iter().find(|r| r.provider == "minimax").unwrap();
let oc = rows.iter().find(|r| r.provider == "opencode").unwrap();
```

becomes:

```rust
let minimax = rows.iter().find(|r| r.sdk_type() == "anthropic").unwrap(); // matches the section's wire format
let oc = rows.iter().find(|r| r.sdk_type() == "openai_responses").unwrap();
```

Wait — that's wrong. The `provider_usage` table is keyed by the **section name** (`minimax`, `opencode`), not the wire format. The migration is:

```rust
let minimax = rows.iter().find(|r| r.name() == "minimax").unwrap();
let oc = rows.iter().find(|r| r.name() == "opencode").unwrap();
```

`name()` is on `LlmClient` too — same accessor, same return value. No semantic change.

### 4. Test migration

- `src/cli/probe.rs::tests::build_provider_for_spec_*` (3 cases): migrate to assert `build_client_for_spec` returns the right SDK (`assert!(client.sdk_type() == "anthropic")`).
- `src/cli/run.rs::tests::*` (4 cases that touch the minimax short-circuit): migrate to assert the dispatcher path picks `AnthropicClient` when the URL ends in `/v1/messages`.
- `src/cli/telemetry_cmd.rs::tests::*` (1 case): migrate to `name()`.

## Acceptance criteria

- [ ] `cargo build --release --all-features` succeeds with **zero warnings**.
- [ ] `make fmt-check guard-deps lint build test-ci` green.
- [ ] `src/cli/probe.rs::build_provider_for_spec` collapses to a single call to `client::dispatcher::build_client`. No `match section_name { "deepseek" | "minimax" | _ => match wire_format { ... } }` anywhere in `src/cli/`.
- [ ] `src/cli/run.rs::build_registry_for_with_active` no longer has a `minimax` short-circuit. The `--api-key` flag goes through the dispatcher.
- [ ] The 8 CLI test cases (3 + 4 + 1) pass.
- [ ] Smoke: `moagan run --mode fast --provider mock:mock-model --api-key dummy` produces the same artefacts as v0.17.6.
- [ ] Smoke: `moagan probe temperature --provider mock:mock-model` and `moagan probe max_tokens --provider mock:mock-model` continue to work (their sidecars are identical to v0.17.6).

## Merge order

```
#1-#7 (foundation + phase.rs + probe subsystem)

#8 (this issue — CLI migration)

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

Can be merged in parallel with #7 if both PRs agree on the test-stub API surface. Practically: ship #7 first, then #8.

## Validation

- `make fmt-check guard-deps lint build test-ci` green after the PR lands.
- 8 CLI test cases pass.
- Three smoke probes run (`moagan run`, `moagan probe temperature`, `moagan probe max_tokens`) produce v0.17.6-equivalent artefacts.

## Version target

v0.18.0.

## References

- [EPIC #847](https://github.com/airvzxf/moagan/issues/847) — the umbrella.
- [#900 D2](https://github.com/airvzxf/moagan/issues/900) — dispatcher by URL path, no `sdk` knob.
- [`src/cli/probe.rs:692-759`](../blob/main/src/cli/probe.rs) — `build_provider_for_spec` / `build_provider_for_probe`.
- [`src/cli/run.rs:920-960`](../blob/main/src/cli/run.rs) — `build_registry_for_with_active` minimax short-circuit.
- [`src/cli/telemetry_cmd.rs:2566-2569`](../blob/main/src/cli/telemetry_cmd.rs) — provider-name filter test.
- Issue #4 — `client::dispatcher::build_client` factory.

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

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

Hướng nghiên cứu

Start with issue #4's client::dispatcher::build_client, then read src/cli/probe.rs::build_provider_for_spec and build_provider_for_probe, src/cli/run.rs::build_registry_for_with_active, and the cited telemetry test. Run the listed CLI tests and validation commands before changing anything. Done means the CLI paths use the dispatcher, the minimax short-circuit and section-name matches are gone, eight tests pass, and the three smoke commands retain their v0.17.6-equivalent artifacts.

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, cli
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
Khá rõ ràng
Mức phù hợp với người mới
45/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.