From 48c05d465692bbcedbe58ede000f7477cbee5d88 Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Sun, 16 Aug 2026 13:27:44 +0000 Subject: [PATCH] fix(hub): resolve the engine from backend_json for runtime mode/model switching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changing a steward's model or permission mode never worked. Both halves of the path resolved the engine family from `agents.kind`, which for a steward is the persona template (`steward.claude-m4`), not the family (`claude-code`): - resolveRuntimeModeSwitch looked the family up to read its `runtime_mode_switch` table, matched nothing, and answered 422 "engine does not support runtime mode switching". - respawnWithSpecMutation keyed `flagForField` the same way and would have returned errUnknownFamilyField had a request ever reached it. So the feature was dead end to end for the agent class this product is built around, while working fine for a direct engine spawn — which is exactly why every existing test passed. The seed helper could not even express a steward: it wrote no `backend_json`, so no test in the file could exhibit the bug. Third instance of this class, after the `/compact` marker (R3) and the desktop's engine-gated affordances (R2). The canonical fix was already written down, with its rationale, at handlers_sessions.go:1088. Fixing it armed a trap the repo had already predicted. `spliceResume` is family-keyed too, and was unreachable for stewards only because the flag table rejected them first — resume_splice_table_test.go:55 says as much: "Latent (flagForField gates first), but it would have become a silent cold-start". Resolving the engine at one site and not the other would have replaced a loud 422 with a SILENT loss of the resume cursor, so a steward's model flip would cold-start mid-session. Both lookups moved together, and a test pins the cursor. Verified: 5 mutations all killed, including one per site and one per legacy-row fallback; full `go test ./...` green. Found while surveying R6's (composer pills) consumer side — building the pill first would have shipped a control that is disabled for exactly the agents it exists for, looking like an engine limitation rather than ours. Co-Authored-By: Claude Opus 5 --- docs/changelog.md | 19 ++ docs/plans/desktop-companion-vision-parity.md | 49 +++++ hub/internal/server/handlers_agent_input.go | 19 +- .../server/respawn_with_spec_mutation.go | 27 ++- .../server/respawn_with_spec_mutation_test.go | 170 +++++++++++++++++- 5 files changed, 272 insertions(+), 12 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index a26e3f72b..220765fc1 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -210,6 +210,25 @@ binding). Seed entries prior to that are in ### Fixed +- **Changing a steward's model or permission mode never worked.** Both the + routing gate and the executor resolved the engine from `agents.kind`, which + for a steward is the persona template (`steward.claude-m4`), not the family + (`claude-code`). The gate matched no family and answered 422 *"engine does + not support runtime mode switching"*; had a request reached the executor it + would have failed the same way on its flag table. So the feature was dead + end-to-end for the agent class the product is built around, while working + fine for a direct engine spawn — which is why every existing test passed. + Both now resolve through `backend_json.kind`, the column spawn writes for + exactly this purpose, falling back to `kind` for rows written before it was + populated. **Third instance of this class**, after the `/compact` marker and + the desktop's engine-gated affordances. + + Fixing it also armed a trap the codebase had already written down: the + resume-cursor splice is family-keyed too, and was unreachable for stewards + only because the flag table rejected them first. Left alone, a steward's + model switch would have started respawning with no `--resume` — trading a + loud 422 for a silent cold start mid-session. Both lookups moved together. + - **A host-runner-raised attention row was attributed to the host, never to the agent that asked.** `POST /attention` honoured a body-supplied `actor_handle` only when the authenticated caller had no handle of its diff --git a/docs/plans/desktop-companion-vision-parity.md b/docs/plans/desktop-companion-vision-parity.md index 73403fb63..74ef41f4d 100644 --- a/docs/plans/desktop-companion-vision-parity.md +++ b/docs/plans/desktop-companion-vision-parity.md @@ -892,6 +892,55 @@ Audit ground truth: claude M2 = `driver_stdio.go`, codex M2 = engine reports it (`status_line` / `fast_mode_state`). Slash picker already exists — fold it into the same pill row. + ★ *Producer bug found while surveying the consumer — fixed ahead of + the UI (2026-08-16).* **Runtime model/mode switching was dead for + stewards, end to end.** Both the routing gate + (`resolveRuntimeModeSwitch`) and the executor + (`respawnWithSpecMutation`) resolved the engine from `agents.kind`, + which for a steward is the persona template (`steward.claude-m4`), + never the family. The gate matched no family and returned 422 + *"engine does not support runtime mode switching"*. Building the pill + on top would have shipped a control that is disabled for exactly the + agents the Companion is built around — and looked like an engine + limitation rather than our bug. **Third instance of this class** + (`/compact` markers in R3; engine-gated affordances in R2), all the + same column, and the canonical fix was already written down at + `handlers_sessions.go:1088`. + + The fix carried a trap the repo had itself predicted: `spliceResume` + is family-keyed too, and was unreachable for stewards only because + the flag table rejected them first + (`resume_splice_table_test.go:55` — *"Latent (flagForField gates + first), but it would have become a silent cold-start"*). Resolving + the engine at one site and not the other would have replaced a loud + 422 with a **silent** loss of the resume cursor on every steward's + model flip. Both moved together, with a test pinning the cursor. + + *Three plan specifics to correct before building the UI:* + + 1. *"the family registry's `permission_modes`"* — that is the + **spawn-time** argv map (`skip` → `--dangerously-skip-permissions`, + `prompt` → `--permission-prompt-tool …`), not a runtime vocabulary. + The runtime ids are the agent's advertised `availableModes` / + `availableModels` (`ModeID` is *"the agent's availableModes id + (`default`, `yolo`, `plan`, …)"*, `handlers_agent_input.go:219`). + 2. *A pill implies a light toggle; for our two engines it is a + **respawn**.* `runtime_mode_switch` reads + `{claude-code: {M1: respawn, M2: respawn}, codex: {M1: respawn, + M2: respawn}}` — only gemini-cli routes `rpc`/`per_turn_argv`. The + hub terminates the agent and spawns a fresh one on the same + session row, answering `202 {"routed":"respawn"}`. Under IAA that + must be previewed, not silently actuated: switching model restarts + the agent, and the pill has to say so. + 3. *This is a parity port, not a new design.* Mobile already ships + the picker (`session_details_sheet.dart` — a compact pill picker + over `availableModes`/`availableModels`, fed by + `modeModelStateFromEvents` in `live_feed.dart`). Port **its** + source of truth rather than the paraphrase above, including its + two shape quirks: mode entries carry `id` while model entries + carry `modelId` (ACP spec), and the picker hides itself entirely + when no agent has advertised the lists — degrade honestly (D-4). + ### Lane D — design-system enforcement (desktop) - **D1 — desktop UI reference doc.** `ui-guidelines.md` is diff --git a/hub/internal/server/handlers_agent_input.go b/hub/internal/server/handlers_agent_input.go index 6e01d74c8..4e00c08f1 100644 --- a/hub/internal/server/handlers_agent_input.go +++ b/hub/internal/server/handlers_agent_input.go @@ -164,10 +164,10 @@ func validateAttachments( // into "unsupported" because the agent_belongs_to_team check has // already ruled it out at the call site. func (s *Server) resolveRuntimeModeSwitch(ctx context.Context, agentID string) (string, error) { - var kind, drivingMode sql.NullString + var kind, drivingMode, backendJSON sql.NullString err := s.db.QueryRowContext(ctx, - `SELECT kind, driving_mode FROM agents WHERE id = ?`, - agentID).Scan(&kind, &drivingMode) + `SELECT kind, driving_mode, COALESCE(backend_json, '{}') FROM agents WHERE id = ?`, + agentID).Scan(&kind, &drivingMode, &backendJSON) if err != nil { if errors.Is(err, sql.ErrNoRows) { return "unsupported", nil @@ -178,7 +178,18 @@ func (s *Server) resolveRuntimeModeSwitch(ctx context.Context, agentID string) ( if mode == "" { mode = "M4" } - fam, ok := s.agentFamilies.ByName(kind.String) + // The family registry is keyed by ENGINE, and `agents.kind` carries a + // persona template for stewards (`steward.claude-m4`) — so looking it up + // with `kind` matched no family and answered "unsupported", i.e. a 422 + // "engine does not support runtime mode switching" for exactly the agent + // class the product is built around. The engine lives in + // `backend_json.kind` (handlers_agents.go:1567); fall back to `kind` for + // rows written before that column was populated. + engine := backendKindOf(backendJSON.String) + if engine == "" { + engine = kind.String + } + fam, ok := s.agentFamilies.ByName(engine) if !ok { return "unsupported", nil } diff --git a/hub/internal/server/respawn_with_spec_mutation.go b/hub/internal/server/respawn_with_spec_mutation.go index e3c4fc8c1..ad2144b1c 100644 --- a/hub/internal/server/respawn_with_spec_mutation.go +++ b/hub/internal/server/respawn_with_spec_mutation.go @@ -63,16 +63,16 @@ func (s *Server) respawnWithSpecMutation( // 1. Resolve the agent's identity + the session it's attached to. var ( teamID, kind, handle, hostID, parentID sql.NullString - worktreePath sql.NullString + worktreePath, backendJSON sql.NullString ) if err := s.db.QueryRowContext(ctx, ` SELECT team_id, kind, handle, host_id, (SELECT parent_agent_id FROM agent_spawns WHERE child_agent_id = agents.id ORDER BY spawned_at DESC LIMIT 1), - worktree_path + worktree_path, COALESCE(backend_json, '{}') FROM agents WHERE id = ?`, agentID).Scan( - &teamID, &kind, &handle, &hostID, &parentID, &worktreePath, + &teamID, &kind, &handle, &hostID, &parentID, &worktreePath, &backendJSON, ); err != nil { return fmt.Errorf("respawn-with-spec-mutation: lookup agent: %w", err) } @@ -80,8 +80,19 @@ func (s *Server) respawnWithSpecMutation( return errors.New("respawn-with-spec-mutation: agent missing required fields (team/kind/handle)") } + // `agents.kind` is the ENGINE only for a direct spawn. For a steward it is + // the persona template (`steward.claude-m4`), and the engine family lives + // in `backend_json.kind` (handlers_agents.go:1567) — the same distinction + // handlers_sessions.go draws for context mutations. Everything below that + // is keyed by FAMILY must use `engine`; the respawn's own `Kind` must stay + // `kind`, because it re-spawns the same persona, not a bare engine. + engine := backendKindOf(backendJSON.String) + if engine == "" { + engine = kind.String + } + // 2. Resolve the flag for the agent's family + field. - flagMap, ok := flagForField[kind.String] + flagMap, ok := flagForField[engine] if !ok { return errUnknownFamilyField } @@ -132,8 +143,14 @@ func (s *Server) respawnWithSpecMutation( // and antigravity is absent — so an antigravity agent never reached here. // Adding it to flagForField would have silently turned that into a // cold-start on every mode/model flip. One table, one dispatch. + // + // That warning came due the moment step 2 started resolving stewards: + // `spliceResume` takes a FAMILY, and a persona template matches none, so + // it would have returned the spec untouched and every steward's + // mode/model flip would have cold-started — a silent transcript break + // replacing a loud 422. Hence `engine`, not `kind`. if engineSessionID.Valid && engineSessionID.String != "" { - mutated = spliceResume(mutated, kind.String, engineSessionID.String) + mutated = spliceResume(mutated, engine, engineSessionID.String) } // 6. Best-effort host-side terminate command for the running pane; diff --git a/hub/internal/server/respawn_with_spec_mutation_test.go b/hub/internal/server/respawn_with_spec_mutation_test.go index 2c3ec70d9..2bb905b8f 100644 --- a/hub/internal/server/respawn_with_spec_mutation_test.go +++ b/hub/internal/server/respawn_with_spec_mutation_test.go @@ -3,6 +3,7 @@ package server import ( "context" "errors" + "fmt" "strings" "testing" ) @@ -105,6 +106,159 @@ backend: } } +// TestRespawnWithSpecMutation_StewardResolvesEngine — a STEWARD, which is +// the agent class this product is built around and the one every earlier +// test in this file failed to represent. +// +// `agents.kind` is the engine only for a direct spawn. For a steward it is +// the persona template (`steward.claude-m4`), and the engine family lives in +// `backend_json.kind` — the column spawn writes for exactly this reason. The +// flag table is keyed by FAMILY, so looking it up with `kind` meant every +// steward's model/mode switch returned errUnknownFamilyField and surfaced as +// "this engine doesn't support runtime switching" — silently wrong, for the +// only agents that matter. +// +// The separating input the older tests could not provide: Kind is a template +// id, and only Backend names the engine. A test seeding Kind:"claude-code" +// passes whether the code reads `kind` or `backend_json`. +func TestRespawnWithSpecMutation_StewardResolvesEngine(t *testing.T) { + srv, _ := newTestServer(t) + agentID, sessionID := seedAgentWithSession(t, srv, agentSeed{ + Kind: "steward.claude-m4", + Backend: "claude-code", + Handle: "steward-persona", + Spec: `kind: steward +backend: + kind: claude-code + cmd: claude --model claude-3-5-sonnet --print --output-format stream-json +`, + }) + + if err := srv.respawnWithSpecMutation(context.Background(), + agentID, "model", "claude-3-7-opus"); err != nil { + t.Fatalf("steward respawn: %v (a persona template must still resolve to its engine)", err) + } + + var newSpec string + if err := srv.db.QueryRow( + `SELECT spawn_spec_yaml FROM sessions WHERE id = ?`, sessionID).Scan(&newSpec); err != nil { + t.Fatalf("read session: %v", err) + } + if !strings.Contains(newSpec, "--model claude-3-7-opus") { + t.Errorf("session spec missing new model:\n%s", newSpec) + } +} + +// TestRespawnWithSpecMutation_StewardKeepsResumeCursor — the trap inside the +// fix, and the reason both family-keyed lookups had to move together. +// +// `spliceResume` also takes a FAMILY. While step 2 rejected stewards outright, +// this line was unreachable for them; the moment step 2 learned to resolve a +// steward's engine, a persona template reaching `spliceResume` would match no +// family, return the spec untouched, and cold-start the agent — trading a loud +// 422 for a SILENT transcript break. The file's own antigravity regression test +// had predicted exactly this shape of failure. +func TestRespawnWithSpecMutation_StewardKeepsResumeCursor(t *testing.T) { + srv, _ := newTestServer(t) + agentID, sessionID := seedAgentWithSession(t, srv, agentSeed{ + Kind: "steward.claude-m4", + Backend: "claude-code", + Handle: "steward-resume", + Spec: `kind: steward +backend: + kind: claude-code + cmd: claude --model claude-3-5-sonnet --print --output-format stream-json +`, + }) + if _, err := srv.db.Exec( + `UPDATE sessions SET engine_session_id = ? WHERE id = ?`, + "engine-sess-42", sessionID); err != nil { + t.Fatalf("seed engine_session_id: %v", err) + } + + if err := srv.respawnWithSpecMutation(context.Background(), + agentID, "model", "claude-3-7-opus"); err != nil { + t.Fatalf("steward respawn: %v", err) + } + + var newSpec string + if err := srv.db.QueryRow( + `SELECT spawn_spec_yaml FROM sessions WHERE id = ?`, sessionID).Scan(&newSpec); err != nil { + t.Fatalf("read session: %v", err) + } + if !strings.Contains(newSpec, "--resume engine-sess-42") { + t.Errorf("steward respawn dropped the resume cursor — the new agent cold-starts:\n%s", newSpec) + } + if !strings.Contains(newSpec, "--model claude-3-7-opus") { + t.Errorf("model not mutated:\n%s", newSpec) + } +} + +// TestRespawnWithSpecMutation_LegacyRowFallsBackToKind — a row written before +// backend_json was populated carries `{}`. Those must keep working off `kind`, +// so the fix may not simply swap one source for the other. +func TestRespawnWithSpecMutation_LegacyRowFallsBackToKind(t *testing.T) { + srv, _ := newTestServer(t) + agentID, sessionID := seedAgentWithSession(t, srv, agentSeed{ + Kind: "claude-code", + Handle: "legacy-direct", + Spec: `kind: agent +backend: + kind: claude-code + cmd: claude --model claude-3-5-sonnet --print --output-format stream-json +`, + }) + if err := srv.respawnWithSpecMutation(context.Background(), + agentID, "model", "claude-3-7-opus"); err != nil { + t.Fatalf("legacy respawn: %v", err) + } + var newSpec string + _ = srv.db.QueryRow(`SELECT spawn_spec_yaml FROM sessions WHERE id = ?`, sessionID).Scan(&newSpec) + if !strings.Contains(newSpec, "--model claude-3-7-opus") { + t.Errorf("legacy row must still resolve via kind:\n%s", newSpec) + } +} + +// TestResolveRuntimeModeSwitch_StewardRoutes — the same defect one layer up, +// at the gate. `resolveRuntimeModeSwitch` looks the family up to read its +// `runtime_mode_switch` table; with `kind` a steward matched no family and the +// handler answered 422 "engine does not support runtime mode switching". So +// the feature was dead for stewards at BOTH the routing gate and the executor +// — fixing either alone leaves it broken. +func TestResolveRuntimeModeSwitch_StewardRoutes(t *testing.T) { + srv, _ := newTestServer(t) + for i, tc := range []struct { + name string + kind string + backend string + want string + }{ + {"steward over claude-code", "steward.claude-m4", "claude-code", "respawn"}, + {"steward over codex", "steward.codex", "codex", "respawn"}, + {"direct spawn still works", "claude-code", "", "respawn"}, + {"unknown engine stays unsupported", "steward.mystery", "no-such-engine", "unsupported"}, + } { + t.Run(tc.name, func(t *testing.T) { + // The handle must be unique per case: `agents` has a live-handle + // uniqueness index, and NewID()'s leading chars are a timestamp + // that repeats inside one millisecond. + agentID, _ := seedAgentWithSession(t, srv, agentSeed{ + Kind: tc.kind, + Backend: tc.backend, + Handle: fmt.Sprintf("route-case-%d", i), + Spec: "kind: steward\n", + }) + got, err := srv.resolveRuntimeModeSwitch(context.Background(), agentID) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got != tc.want { + t.Errorf("route = %q; want %q", got, tc.want) + } + }) + } +} + // agentSeed bundles the inputs seedAgentWithSession needs to set up a // (agent, session) pair. Spec is the rendered spawn_spec_yaml; the // helper stores it on both agent_spawns and sessions so the helper @@ -113,6 +267,12 @@ type agentSeed struct { Kind string Handle string Spec string + // Backend is the engine family spawn writes into `agents.backend_json` + // (handlers_agents.go:1567). It is what distinguishes a steward — whose + // `Kind` is a persona template like `steward.claude-m4` — from a direct + // engine spawn where `Kind` IS the family. Empty leaves the column at the + // `{}` a pre-column row carries, which is the legacy-row case. + Backend string } func seedAgentWithSession(t *testing.T, s *Server, seed agentSeed) (agentID, sessionID string) { @@ -129,11 +289,15 @@ func seedAgentWithSession(t *testing.T, s *Server, seed agentSeed) (agentID, ses hostID, defaultTeamID, "host-"+hostID[len(hostID)-6:], now); err != nil { t.Fatalf("seed host: %v", err) } + backendJSON := "{}" + if seed.Backend != "" { + backendJSON = `{"kind":"` + seed.Backend + `"}` + } if _, err := s.db.ExecContext(ctx, ` INSERT INTO agents (id, team_id, handle, kind, status, - host_id, driving_mode, created_at) - VALUES (?, ?, ?, ?, 'running', ?, 'M2', ?)`, - agentID, defaultTeamID, seed.Handle, seed.Kind, hostID, now); err != nil { + host_id, driving_mode, backend_json, created_at) + VALUES (?, ?, ?, ?, 'running', ?, 'M2', ?, ?)`, + agentID, defaultTeamID, seed.Handle, seed.Kind, hostID, backendJSON, now); err != nil { t.Fatalf("seed agent: %v", err) } // Spawn row anchors parent_agent_id lookups in the helper.