fix(tui): Fleet setup role/profile roster editor (#4093) - #4181
Conversation
|
Claude finished @Hmbown's task in 4m 46s —— View job Review: Fleet setup role/profile roster editor (#4093)Todo list
SummaryThe refactor of 1. Compile error — test suite won't build (inline comment on 2. The cross-provider route is never actually persisted or honored (inline comment on
Net effect: none of the PR's stated acceptance criteria ("create/edit a profile for a model from a different provider than the parent session," "saved profile clearly identifies its provider/model route," "profile usable by Fleet without depending on current provider at launch") are actually satisfied — the picker offers cross-provider choices the rest of the stack can't act on. This needs either a real 3. Minor (inline comment on Note: The PR description also claims "Fix draft preview ratify key binding conflict ( No test coverage was added for the new cross-provider selection/persistence path, which is exactly what would have caught finding #2. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// drawn from ALL configured providers — not only the active one (#4093). | ||
| /// Shown after `inherit` in the Model step so a Fleet worker can be pinned | ||
| /// to a route independent of the parent/current provider. | ||
| available_models: Vec<(String, String)>, |
There was a problem hiding this comment.
Compile error introduced by this type change. The tests::snapshot() helper (line 913, unchanged by this diff) still builds this field as a Vec<&str>:
available_models: vec!["deepseek-v4-pro", "deepseek-v4-flash"],That no longer type-checks against Vec<(String, String)>. cargo check -p codewhale-tui --tests / cargo test -p codewhale-tui fleet_setup (the PR's own test plan) will fail to build. Needs a matching update to (String, String) tuples in the test helper.
|
|
||
| /// The concrete model chosen for this worker, or `None` for `inherit` | ||
| /// (reuse the session route). Written to the profile `model` field. | ||
| /// The concrete model chosen for this worker, written to the profile | ||
| /// `model` field. `None` means `inherit` (reuse the session route). | ||
| fn selected_model(&self) -> Option<String> { | ||
| match self.model_choices.get(self.model_idx) { | ||
| Some(choice) if choice.label != "inherit" => Some(choice.label.to_string()), | ||
| _ => None, | ||
| self.selected_route().map(|(_, model)| model) | ||
| } | ||
|
|
||
| /// The concrete `(provider, model)` chosen for this worker — a pinned route | ||
| /// independent of the parent/current provider (#4093) — or `None` when | ||
| /// `inherit` is selected (reuse the session route). | ||
| fn selected_route(&self) -> Option<(String, String)> { | ||
| if self.model_idx == 0 { | ||
| return None; | ||
| } | ||
| self.model_routes.get(self.model_idx).cloned() | ||
| } |
There was a problem hiding this comment.
The provider half of the picked route is computed here but never persisted or displayed correctly downstream — the cross-provider feature doesn't actually reach the saved profile or the review UI.
starter_profile_draft()(fleet_setup.rs:440, unchanged by this diff) only writesmodel: self.selected_model()ontoFleetProfileDraft. That struct (crates/tui/src/fleet/profile.rs:309-317) has noproviderfield, and its untrusted-JSON schema explicitlydeny_unknown_fieldss aproviderkey by design (profile.rs:327-332, "the worker profile carries no provider authority"). So the provider half ofselected_route()is silently dropped — the saved TOML never records which provider a pinned model belongs to.- At actual launch, a Fleet worker always runs under the parent/session's active provider client (
tools/subagent/mod.rs,SubAgentRuntime::background_runtimeclonesself.client;resolve_subagent_assignment_routevalidates the pinned model againstruntime.client.api_provider(), not any provider recorded on the profile). So picking e.g. an Anthropic model while the session is on DeepSeek either gets silently misattributed to DeepSeek (if the id happens to validate there) or the worker launch fails withToolError::invalid_input. It never actually calls the picked provider. render_review()(fleet_setup.rs:689, unchanged) compounds this by displayingformat!("{model} · provider {}", self.snapshot.provider)—self.snapshot.provideris always the active provider, not the one the user picked viaselected_route(). So the review screen can straight-up lie about which provider a pinned model will run on.
This means the PR's core acceptance criteria ("create/edit a Fleet profile for a model from any configured provider while parent session uses a different provider," "saved profile clearly identifies intended provider/model route," "profile usable by Fleet without depending on current provider at launch time") aren't met — the UI now offers cross-provider routes, but nothing downstream understands or persists them. This needs either a provider field added to FleetProfileDraft/TOML schema (and the loader/resolver taught to honor it), or the feature scope needs to shrink to "browse all providers' catalogs for reference" without claiming to pin a real cross-provider route.
| @@ -260,8 +294,8 @@ impl FleetSetupView { | |||
| } | |||
There was a problem hiding this comment.
Minor: selected_role() now returns an owned String (was &'static str). The caller at line 504 (role: self.selected_role().to_string()) is now cloning an already-owned String for no reason — drop the extra .to_string().
3d40ad2 to
c001692
Compare
|
Claude finished @Hmbown's task in 1m 11s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after syncTodo list
SummaryNo code changed. 1. Compile error — test suite won't build. 2. Cross-provider route is computed but never persisted or honored.
None of the PR's stated acceptance criteria (cross-provider profile creation, saved profile identifying its real route, launch independent of the active provider) are met yet. 3. Minor: No new test coverage was added for the cross-provider selection/persistence path, which is exactly what would catch finding #2. Next step: push a commit that (a) fixes the test helper's literal type, and (b) threads a real |
c001692 to
f8ec5fc
Compare
|
Claude finished @Hmbown's task in 1m 15s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after syncTodo list
SummaryNo new commits landed. 1. Compile error — test suite still won't build. available_models: vec!["deepseek-v4-pro", "deepseek-v4-flash"],but the field ( 2. Cross-provider route is computed but still never persisted or honored.
None of the PR's stated acceptance criteria (cross-provider profile creation, saved profile identifying its real route, launch independent of the active provider) are met yet. 3. Minor, still present: No new test coverage was added for the cross-provider selection/persistence path, which is exactly what would catch finding #2. Next step (unchanged from last review): push a commit that (a) fixes the test helper's literal type at line 931, and (b) threads a real |
f8ec5fc to
e15e49c
Compare
|
Claude finished @Hmbown's task in 40s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after syncTodo list
SummaryNo new commits landed since the last two reviews. 1. Compile error — test suite still won't build. available_models: vec!["deepseek-v4-pro", "deepseek-v4-flash"],but the field ( 2. Cross-provider route is computed but still never persisted or honored.
None of the PR's stated acceptance criteria (cross-provider profile creation, saved profile identifying its real route, launch independent of the active provider) are met yet. 3. Minor, still present: No new test coverage was added for the cross-provider selection/persistence path, which is exactly what would catch finding #2. Next step (unchanged): push a commit that (a) fixes the test helper's literal type at line 931, and (b) threads a real |
e15e49c to
31006ed
Compare
|
Claude finished @Hmbown's task in 1m 6s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after syncTodo list
Summary
Finding #1 (compile error) — fixed. Finding #2 (cross-provider route dropped before it's persisted) — still not fixed. The plumbing that computes the route is solid (
None of this PR's core acceptance criteria (cross-provider profile creation, saved profile identifying its real route, launch independent of the active provider) are met by this diff. The new Finding #3 (minor, still present). Note (unchanged): the PR description's "Fix draft preview ratify key binding conflict ( Next step: thread a real |
… keybinding (#4093) Continues PR #4181 (Fleet setup role/profile roster editor). Fixes the compile mismatch flagged in review, then closes the two substantive gaps the earlier pass left open: - FleetProfileDraft / the saved profile TOML / the loader / FleetProfile (in codewhale-config) now all carry an explicit `provider` field alongside `model`, validated against the real ApiProvider vocabulary at load time and never inferred from the model id (EPIC #2608). The Fleet setup Model step now records the canonical provider id (not just a display label) so a cross-provider pick round-trips correctly into the saved profile. - resolve_fleet_route (worker_runtime.rs) resolves within the profile's own explicit provider scope when it has one, instead of a hardcoded DeepSeek default, so the receipt/route-resolution surface reflects a pinned cross-provider route rather than silently defaulting. - The draft-preview ratify keypress no longer competes with a separate pager's own g/G scroll bindings: the exact TOML preview now renders inline on the same Review step that ratifies it, so g/Enter always ratify directly with no Esc-then-g round trip. - Review screen's Model section now names the actually-picked route's provider instead of the parent/active session's. Note: the real Fleet subprocess dispatch (fleet/executor.rs building the `codewhale exec` argv via fleet/manager.rs's start_leased_workers) does not yet pass a --provider flag (no such CLI flag exists yet) and does not thread the per-task/profile resolved model into that command line at all today -- only the run-level session model. That is a materially larger, pre-existing gap beyond this pass's scope; it is not fixed here and remains open. Tests: added a cross-provider save/load/route-resolution test plus a direct resolve_fleet_route provider-precedence test in worker_runtime.rs, a provider round-trip test and an unrecognized-provider rejection test in profile.rs (replacing the now-obsolete "provider is always rejected" test), and inline-preview coverage in fleet_setup.rs.
|
Claude finished @Hmbown's task in 3m 18s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after commit
|
|
Pushed What this commit does (verified locally:
What is NOT fixed here — the real remaining gap: The actual Fleet worker dispatch path (
So while the profile's provider is now correctly persisted, validated, and honored by the resolver/receipt surface this PR touches, a Fleet worker's actual spawned process does not yet reliably launch on the profile's own provider — it inherits whatever provider the freshly-started subprocess's own on-disk config resolves to, with only an inventory-based model→provider match (in
Recommendation: do not merge until (a) Generated by Claude Code |
|
Follow-up: Generated by Claude Code |
|
Claude finished @Hmbown's task in 1m 14s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after sync (
|
|
Status update (commit CI fixes in this commitBoth CI failures on
Re-verified locally after the fix, before pushing
What's genuinely solid as of
|
Agent WIP: reframe setup modal around profile roster rather than provider-scoped configuration.
… keybinding (#4093) Continues PR #4181 (Fleet setup role/profile roster editor). Fixes the compile mismatch flagged in review, then closes the two substantive gaps the earlier pass left open: - FleetProfileDraft / the saved profile TOML / the loader / FleetProfile (in codewhale-config) now all carry an explicit `provider` field alongside `model`, validated against the real ApiProvider vocabulary at load time and never inferred from the model id (EPIC #2608). The Fleet setup Model step now records the canonical provider id (not just a display label) so a cross-provider pick round-trips correctly into the saved profile. - resolve_fleet_route (worker_runtime.rs) resolves within the profile's own explicit provider scope when it has one, instead of a hardcoded DeepSeek default, so the receipt/route-resolution surface reflects a pinned cross-provider route rather than silently defaulting. - The draft-preview ratify keypress no longer competes with a separate pager's own g/G scroll bindings: the exact TOML preview now renders inline on the same Review step that ratifies it, so g/Enter always ratify directly with no Esc-then-g round trip. - Review screen's Model section now names the actually-picked route's provider instead of the parent/active session's. Note: the real Fleet subprocess dispatch (fleet/executor.rs building the `codewhale exec` argv via fleet/manager.rs's start_leased_workers) does not yet pass a --provider flag (no such CLI flag exists yet) and does not thread the per-task/profile resolved model into that command line at all today -- only the run-level session model. That is a materially larger, pre-existing gap beyond this pass's scope; it is not fixed here and remains open. Tests: added a cross-provider save/load/route-resolution test plus a direct resolve_fleet_route provider-precedence test in worker_runtime.rs, a provider round-trip test and an unrecognized-provider rejection test in profile.rs (replacing the now-obsolete "provider is always rejected" test), and inline-preview coverage in fleet_setup.rs.
cargo fmt reformatted a broken() call in fleet/roster.rs (over the 100-col limit after the #4093 provider-validation comment landed), and crates/tui/CHANGELOG.md had drifted from the root CHANGELOG.md Fixed-section slice describing the #4093 fix. Both were failing CI (Lint, Version drift) on this PR. No behavior change.
) Deliver AC #4 end-to-end. The resolved profile route now reaches the actual worker subprocess, not just the receipt: a worker whose profile pins provider B launches on provider B's model even when the parent session is on provider A. - executor/worker_runtime: build_worker_exec_command_with_profiles resolves each worker's model+provider from its AgentProfile via the same explicit-provider-only path the receipt uses (EPIC #2608 — never inferred from a model id), and threads --model/--provider onto the codewhale exec argv. The provider is a non-secret identifier; credentials still resolve from env inside the worker. Profile-less workers are unchanged: no --provider, run-level model. - exec CLI: add --provider, overriding config.provider before model resolution so an auto/default model resolves to the overridden provider. - fleet setup: the `m` model-assisted redraft no longer drops the picked provider. The picked (provider, model) route is carried through the draft request and re-pinned in install_model_draft (a model draft is always provider: None), so ratifying keeps the cross-provider route instead of persisting an ambiguous, provider-scoped profile. - save-time validation (AC #5): ratifying a profile that pins an unconfigured/uncredentialed provider now fails with a clear message, reusing provider_is_configured_for_active. Tests: launch-argv assertion for a provider-B worker under a provider-A session; explicit-provider-only launch resolver; `m`-redraft provider preservation regression; exec --provider parse. Updated crates/tui/CHANGELOG.md and root CHANGELOG.md. Completed with assistance from Claude Code (Opus 4.8).
a63a5d6 to
7a401ec
Compare
|
Claude finished @Hmbown's task in 4m 27s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after commit
|
Adversarial tracing found the `--provider` launch fix is reachable only from the headless `codewhale fleet run` CLI; the interactive TUI spawns roster members in-process via the engine `agent` tool, which does not yet read the profile's pinned provider (it uses the session provider). Reword the CHANGELOG bullet in both files to state the CLI-only coverage and point to #4193 for the in-process TUI remainder, so we do not overclaim. Completed with assistance from Claude Code (Opus 4.8).
|
Claude finished @Hmbown's task in 3m 52s —— View job Review: Fleet setup role/profile roster editor (#4093) — re-check after commit
|
…bown#4193) PR Hmbown#4181 made the headless `codewhale fleet run` CLI launch workers on their profile-pinned provider+model route, but the interactive TUI spawns Fleet roster members in-process via the engine `agent` tool, which still ignored the profile's pinned provider. A profile pinning provider B + model B therefore sent model B's id to provider A's (the session's) endpoint — the Hmbown#4093 defect, intact for TUI users. The root cause was that the in-process child reused the parent session's LLM client (provider A's base_url + creds); the `provider` metadata tag alone is inert while the client is shared. This threads the session Config into the spawn runtime and builds a fresh client for the pinned provider. Seam 1 (worker_profile_for_spawn): now reflects the pinned provider transitively — it reads `runtime.client.api_provider()`, and the runtime it receives is the child whose client is rebound to provider B below, so the worker-record provider tag is correct without a hardcoded session provider. Seam 2 (model normalization/routing in spawn_subagent_from_input): model validation (`normalize_requested_subagent_model`, `configured_model_for_role_or_type`), strength/inherit/faster routing (`resolve_subagent_assignment_route`), and the final namespace guard (`ensure_subagent_model_for_provider`) now run against the child runtime (pinned provider) instead of the session runtime. Seam 3 (the substantive fix): SubAgentRuntime gains an `api_config` snapshot (threaded by the engine via `with_api_config`). `child_client_for_member` resolves the member's explicit pin via `worker_runtime::explicit_fleet_provider` (explicit-only, never inferred from a model id — EPIC Hmbown#2608) and, when it differs from the session provider, builds a new `DeepSeekClient` for it by cloning the session Config and overriding only `provider` — the same per-provider client factory pattern used by per-turn auto-routing and the engine's provider switch. A pinned-but-unbuildable provider fails the spawn rather than silently misrouting to the session endpoint (Hmbown#4093). Profile-less / `inherit` members keep the session client (no regression). Tests: an in-process spawn whose member pins provider B under a session on provider A produces a child client targeting B (asserts api_provider + base_url), a profile-less/same-provider member keeps the session client, and a pinned provider with no threaded Config fails closed. Guards: EPIC Hmbown#2608 (provider explicit or session fallback, never model-inferred); Hmbown#4172 (no DEEPSEEK* / active-module renames). Agent note: implemented by an Opus 4.8 coding agent under Hunter's direction.
Refs #4093 (headline cross-provider fix landed for the
codewhale fleet runCLI path; the interactive TUI in-process remainder is tracked in #4193, so this does not auto-close #4093)Summary
gscroll vs ratify)Acceptance criteria
Issue: #4093
Test plan
cargo test -p codewhale-tui fleet_setup(or focused fleet setup tests)gscroll conflictSigned-off-by: Hunter hunter@codewhale.dev
Made with Cursor