fix(registry): preserve native_team_enabled + provider across ON CONFLICT in register() - #1684
Conversation
…LICT in register() Closes #1682 `findOrCreateAgent` writes the durable identity row first with all native_* and provider fields defaulting to NULL/false. The subsequent `register()` call carries the real protocol settings, but its ON CONFLICT (id) DO UPDATE clause only refreshed pane/session/state — leaving nativeTeamEnabled=false and provider=null pinned to the row. Routing then read `nativeTeamEnabled === false` and dispatched claude workers down `injectToTmuxPane` (the documented body→200ms→Enter race) instead of `writeToNativeInbox`. Three live agents observed misrouted in this state: engineer, fix, fix-bug2. Causal chain: findOrCreateAgent INSERT (defaults) → register() ON CONFLICT no-op on protocol fields → routing reads stale defaults → injectToTmuxPane fallback for claude workers Fix: extend the SET clause. - native_team_enabled = EXCLUDED (spawn intent is authoritative; a respawn under different protocol settings must re-flag the row) - native_agent_id, native_color, parent_session_id, provider, transport use COALESCE — preserve immutable identity values when later callers omit them. Regressing commit: dfc875e. Coordination: courtesy ping for wish #175 G2 #177 (retire-session-names- id-only) — same file, different region (G2 touches reconcileStaleSpawns at line 604+, this fix is at register() line 293). Happy to fold into G2 if cleaner. Backfill SQL (operator runs post-merge — NOT in this PR): UPDATE agents a SET native_team_enabled = true, provider = COALESCE(a.provider, t.provider) FROM agent_templates t WHERE a.custom_name = t.name AND a.team = t.team AND t.native_team_enabled = true AND a.state IS NOT NULL; Idempotent — restores correctness for currently-misrouted live workers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #1682 by updating the register function to preserve protocol fields during database updates using ON CONFLICT logic and adding corresponding regression tests. Feedback indicates that the current implementation fails to preserve these fields correctly because hardcoded defaults in the VALUES clause (such as 'tmux' for transport and false for native_team_enabled) will overwrite existing values when those fields are omitted in subsequent calls.
| // ON CONFLICT preservation (#1682): | ||
| // - `native_team_enabled` uses EXCLUDED — a respawn under different protocol | ||
| // settings must be allowed to re-flag the row. Spawn intent is authoritative. | ||
| // - The other native_* / provider / transport fields use COALESCE — the | ||
| // identity row created by `findOrCreateAgent` defaults them to NULL/false, | ||
| // so the first real `register()` after spawn must populate them. Later | ||
| // callers that omit these fields must NOT clobber the established values. | ||
| // Without this, the ON CONFLICT no-op left routing reading stale defaults | ||
| // (nativeTeamEnabled=false, provider=null), forcing claude workers down the | ||
| // injectToTmuxPane path instead of writeToNativeInbox. | ||
| await sql`INSERT INTO agents (id, pane_id, session, worktree, task_id, task_title, wish_slug, group_number, started_at, state, last_state_change, repo_path, window_name, window_id, role, custom_name, sub_panes, provider, transport, skill, team, tmux_window, native_agent_id, native_color, native_team_enabled, parent_session_id, suspended_at, auto_resume, resume_attempts, last_resume_attempt, max_resume_attempts, pane_color) VALUES (${agent.id}, ${agent.paneId}, ${agent.session}, ${agent.worktree ?? null}, ${agent.taskId ?? null}, ${agent.taskTitle ?? null}, ${agent.wishSlug ?? null}, ${agent.groupNumber ?? null}, ${agent.startedAt ?? now}, ${agent.state ?? 'spawning'}, ${agent.lastStateChange ?? now}, ${agent.repoPath}, ${agent.windowName ?? null}, ${agent.windowId ?? null}, ${agent.role ?? null}, ${agent.customName ?? null}, ${sql.json(agent.subPanes ?? [])}, ${agent.provider ?? null}, ${agent.transport ?? 'tmux'}, ${agent.skill ?? null}, ${agent.team ?? null}, ${agent.window ?? null}, ${agent.nativeAgentId ?? null}, ${agent.nativeColor ?? null}, ${agent.nativeTeamEnabled ?? false}, ${agent.parentSessionId ?? null}, ${agent.suspendedAt ?? null}, ${agent.autoResume ?? false}, ${agent.resumeAttempts ?? 0}, ${agent.lastResumeAttempt ?? null}, ${agent.maxResumeAttempts ?? 3}, ${agent.paneColor ?? null}) ON CONFLICT (id) DO UPDATE SET pane_id = EXCLUDED.pane_id, session = EXCLUDED.session, state = EXCLUDED.state, last_state_change = EXCLUDED.last_state_change, team = COALESCE(agents.team, EXCLUDED.team), role = COALESCE(agents.role, EXCLUDED.role), custom_name = COALESCE(agents.custom_name, EXCLUDED.custom_name), native_team_enabled = EXCLUDED.native_team_enabled, native_agent_id = COALESCE(EXCLUDED.native_agent_id, agents.native_agent_id), native_color = COALESCE(EXCLUDED.native_color, agents.native_color), parent_session_id = COALESCE(EXCLUDED.parent_session_id, agents.parent_session_id), provider = COALESCE(EXCLUDED.provider, agents.provider), transport = COALESCE(EXCLUDED.transport, agents.transport), updated_at = now()`; |
There was a problem hiding this comment.
The implementation of transport and native_team_enabled preservation is inconsistent with the stated intent in the comments and the PR description.
transportlogic error: TheUPDATEclause usesCOALESCE(EXCLUDED.transport, agents.transport), but theVALUESclause provides a hardcoded default:${agent.transport ?? 'tmux'}. This ensuresEXCLUDED.transportis neverNULL, so theCOALESCEwill always return the new value (defaulting to'tmux'if omitted). If an agent was registered withtransport: 'inline', a subsequentregister()call (like a heartbeat) that omits the field will incorrectly reset it to'tmux'.native_team_enabledclobbering: TheUPDATEclause usesEXCLUDED.native_team_enableddirectly, and theVALUESclause defaults tofalse. If a heartbeat caller omits this field, it will be reset tofalse, even if it was previouslytrue. This contradicts the goal of not clobbering established values during refreshes.
To fix this for transport (which is nullable in the DB), you should pass null in the VALUES clause when omitted and handle the default in the UPDATE clause: transport = COALESCE(EXCLUDED.transport, agents.transport, 'tmux').
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e00b64c482
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Without this, the ON CONFLICT no-op left routing reading stale defaults | ||
| // (nativeTeamEnabled=false, provider=null), forcing claude workers down the | ||
| // injectToTmuxPane path instead of writeToNativeInbox. | ||
| await sql`INSERT INTO agents (id, pane_id, session, worktree, task_id, task_title, wish_slug, group_number, started_at, state, last_state_change, repo_path, window_name, window_id, role, custom_name, sub_panes, provider, transport, skill, team, tmux_window, native_agent_id, native_color, native_team_enabled, parent_session_id, suspended_at, auto_resume, resume_attempts, last_resume_attempt, max_resume_attempts, pane_color) VALUES (${agent.id}, ${agent.paneId}, ${agent.session}, ${agent.worktree ?? null}, ${agent.taskId ?? null}, ${agent.taskTitle ?? null}, ${agent.wishSlug ?? null}, ${agent.groupNumber ?? null}, ${agent.startedAt ?? now}, ${agent.state ?? 'spawning'}, ${agent.lastStateChange ?? now}, ${agent.repoPath}, ${agent.windowName ?? null}, ${agent.windowId ?? null}, ${agent.role ?? null}, ${agent.customName ?? null}, ${sql.json(agent.subPanes ?? [])}, ${agent.provider ?? null}, ${agent.transport ?? 'tmux'}, ${agent.skill ?? null}, ${agent.team ?? null}, ${agent.window ?? null}, ${agent.nativeAgentId ?? null}, ${agent.nativeColor ?? null}, ${agent.nativeTeamEnabled ?? false}, ${agent.parentSessionId ?? null}, ${agent.suspendedAt ?? null}, ${agent.autoResume ?? false}, ${agent.resumeAttempts ?? 0}, ${agent.lastResumeAttempt ?? null}, ${agent.maxResumeAttempts ?? 3}, ${agent.paneColor ?? null}) ON CONFLICT (id) DO UPDATE SET pane_id = EXCLUDED.pane_id, session = EXCLUDED.session, state = EXCLUDED.state, last_state_change = EXCLUDED.last_state_change, team = COALESCE(agents.team, EXCLUDED.team), role = COALESCE(agents.role, EXCLUDED.role), custom_name = COALESCE(agents.custom_name, EXCLUDED.custom_name), native_team_enabled = EXCLUDED.native_team_enabled, native_agent_id = COALESCE(EXCLUDED.native_agent_id, agents.native_agent_id), native_color = COALESCE(EXCLUDED.native_color, agents.native_color), parent_session_id = COALESCE(EXCLUDED.parent_session_id, agents.parent_session_id), provider = COALESCE(EXCLUDED.provider, agents.provider), transport = COALESCE(EXCLUDED.transport, agents.transport), updated_at = now()`; |
There was a problem hiding this comment.
transport is intended to be preserved via COALESCE(EXCLUDED.transport, agents.transport), but this UPSERT always inserts ${agent.transport ?? 'tmux'} into EXCLUDED.transport, so it is never NULL on conflict. That means any partial register() refresh that omits transport will force the row to 'tmux', including agents that were previously 'inline'; downstream logic (for example findDeadResumable filtering on w.transport === 'tmux') will then treat those rows as resumable tmux workers and can route into the wrong tmux-only path.
Useful? React with 👍 / 👎.
Addresses Gemini + Codex review on PR #1684. The prior patch wrote `${agent.transport ?? 'tmux'}` into the INSERT VALUES clause, so EXCLUDED.transport was never NULL — `transport = COALESCE(EXCLUDED.transport, agents.transport)` always returned the new value. A heartbeat-style refresh that omitted `transport` would silently rewrite a row registered with `'inline'` back to `'tmux'`, and downstream `findDeadResumable` filters on `w.transport === 'tmux'` would misclassify the row into the tmux-only resume path. Fix: - VALUES clause: `${agent.transport ?? null}` (was `?? 'tmux'`). - DO UPDATE clause: `transport = COALESCE(EXCLUDED.transport, agents.transport, 'tmux')` — fresh INSERT with no transport still defaults to `'tmux'`, but a refresh that omits the field preserves the established value. Tightened the ON CONFLICT comment with the caller contract for `native_team_enabled` (spawn intent is authoritative, callers MUST set explicitly — the `?? false` fallback in VALUES will clobber an existing `true` if omitted). Added a third regression test exercising the `'inline'` transport heartbeat path that the existing two tests didn't cover (both used `'tmux'` on both sides, so the COALESCE no-op was invisible). Validation: - `bun run typecheck` → 0 errors - `bunx biome check src/lib/agent-registry.ts src/lib/agent-registry.test.ts` → clean - `bun test src/lib/agent-registry.test.ts` hangs in this workspace (same environmental issue documented in the original PR body — collateral from in-flight dogfood work). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed @gemini-code-assist and @chatgpt-codex-connector review in
Validation: |
|
Thanks for the detailed follow-up and for incorporating the regression test for the transport field. The logic in |
|
No further code changes were needed — the branch is already at commit Checks run
Because there were no new modifications to commit, I did not create an additional commit or call |
1 participant
Summary
Closes #1682.
register()insrc/lib/agent-registry.tswas no-op'ing the protocol-related fields on itsON CONFLICT (id) DO UPDATEpath. AfterfindOrCreateAgentcreated the identity row with default values (native_team_enabled=false,provider=NULL, etc.), the subsequentregister()carrying the real spawn intent left those defaults pinned — and routing kept readingnativeTeamEnabled === false, dispatching claude workers downinjectToTmuxPane(the body→200ms→Enter race surface) instead ofwriteToNativeInbox.Three live workers observed in this misrouted state:
engineer,fix,fix-bug2.Causal chain
findOrCreateAgent('engineer', 'genie', 'engineer')— INSERT withnative_team_enabled DEFAULT false,provider IS NULL.register({ ...id, nativeTeamEnabled: true, provider: 'claude', transport: 'tmux', nativeAgentId: 'engineer@genie', ... }).(id);DO UPDATE SET …refreshes onlypane_id, session, state, last_state_change, team, role, custom_name, updated_at— none of the protocol fields.agents.native_team_enabledstaysfalse,agents.providerstaysNULL.protocol-routerreads stale row →injectToTmuxPanefor claude workers.Fix
Extend the
ON CONFLICT … DO UPDATE SETclause atsrc/lib/agent-registry.ts:293:native_team_enabled = EXCLUDED.native_team_enabled— spawn intent is authoritative; a respawn under different protocol settings must be allowed to re-flag the row.native_agent_id,native_color,parent_session_id,provider,transportuseCOALESCE(EXCLUDED.<col>, agents.<col>)— preserve immutable identity values when later callers (heartbeat-style refreshes) omit them.Regressing commit:
dfc875ef.Tests
Two new regressions added in
src/lib/agent-registry.test.tsunder a fresh top-level (non-skipped)describeblock:register preserves nativeTeamEnabled + provider across findOrCreateAgent → register— drives the exact misroute path. Assertsnative_team_enabledflipsfalse → true(EXCLUDED) andprovider/transport/native_agent_id/native_color/parent_session_idpopulate from NULL (COALESCE).register preserves established protocol fields when later caller omits them— confirms the COALESCE branches don't clobber values when a later refresh leaves the fields undefined.These use real UUIDs (via
randomUUID()fromfindOrCreateAgent) so migration 061'sagents_id_shape_checkaccepts the inserts. The existing legacydescribe.skipblock above is untouched (still pending wish #175 fixture rewrite).Backfill SQL — operator runs post-merge (NOT in this PR)
Idempotent — restores correctness for currently-misrouted live workers.
Coordination
Courtesy ping for wish #175 G2 #177 (
retire-session-names-id-only). Same file, different region — G2 touchesreconcileStaleSpawns(line 604+); this fix is atregister()line 293. No conflict observed againstorigin/wish/retire-session-names-id-onlyat fetch time. Happy to fold into G2 if cleaner.Validation
bun run typecheck→ 0 errors ✅bunx biome check src/lib/agent-registry.ts src/lib/agent-registry.test.ts→ clean ✅Test-gate caveat:
bun test src/lib/agent-registry.test.tshung in this workspace (unrelated environmental issue — collateral from in-flight dogfood work, see #1682 thread). Reviewer should run the test pre-merge to confirm the two new regression tests pass.Out of scope
injectToTmuxPaneretirement (tracked separately as a hardening ticket).findOrCreateAgent(works correctly; bug is purely on the register path).Test plan
bun test src/lib/agent-registry.test.ts -t "1682"— both new tests pass.bun test src/lib/agent-registry.test.tsstays green (no regression in the existingdescribe.skipblock which remains skipped pending 🚀 Release: Interactive Onboarding with Ink Wizard (rc.27) #175).🤖 Generated with Claude Code