fix(spawn): unblock pipeline against migration 061 (G3 P0) - #1629
Conversation
Migration 061 (agents_id_shape_check + agent_templates UUID PK) went live without the spawn-side rewrites planned in wish G3. Result: every `genie agent spawn` errored with PostgresError and felipe's wish dispatches failed (autopg-distribution-cutover, agent-yaml-permissions-wireup). Three minimum-blast-radius fixes against the cascade: 1. src/lib/agent-directory.ts:lookupTemplateTeam — `WHERE id = $1` with a bare name (e.g. "engineer") tried to cast to UUID after 061 retyped agent_templates.id. Switched to `WHERE name = $1` per the new schema. 2. src/lib/agent-registry.ts:saveTemplate — INSERT into agent_templates passed `template.id` (bare role name) into the now-UUID `id` column. Switched to inserting via the new `name` column with `ON CONFLICT (name, team) WHERE name IS NOT NULL AND team IS NOT NULL` so the partial unique index from 061 is the upsert target. 3. src/term-commands/agents.ts:registerSpawnWorker — set `id` to the durable identity UUID (`ctx.agentIdentityId`, populated from `findOrCreateAgent`) and stash the human-readable workerId in `customName`. Falls back to `ctx.workerId` only when agentIdentityId isn't populated (rare paths that 061 will reject loudly — exactly as designed). The `register()` ON CONFLICT (id) DO UPDATE clause merges runtime fields into the existing identity row instead of inserting a shadow. Verification: - `bun --bun src/genie.ts agent spawn engineer --cwd /tmp --team genie --no-tui` succeeds; `agent show <name>` shows UUID id; events errors --since 5m returns no patterns. Wish: retire-session-names-id-only, Group 3 (P0). 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the agent directory and registry to accommodate a database migration that changed the agent_templates.id field to a UUID and introduced a separate name column. Feedback highlights a logic error in saveTemplate where UUIDs might be incorrectly saved into the name column, potentially breaking lookups. Additionally, it is suggested to use ctx.agentName instead of ctx.workerId in registerSpawnWorker to ensure human-readable identifiers are preserved in the UI.
| // generated via DEFAULT gen_random_uuid(). | ||
| await sql` | ||
| INSERT INTO agent_templates (name, provider, team, role, skill, cwd, extra_args, native_team_enabled, last_spawned_at) | ||
| VALUES (${template.id}, ${template.provider}, ${template.team}, ${template.role ?? null}, ${template.skill ?? null}, ${template.cwd}, ${sql.json(template.extraArgs ?? [])}, ${template.nativeTeamEnabled ?? false}, ${template.lastSpawnedAt}) |
There was a problem hiding this comment.
The saveTemplate function uses template.id as the value for the name column in the agent_templates table. However, following migration 061, agent_templates.id is a UUID, and the rowToTemplate function (line 202) maps this database UUID to WorkerTemplate.id.
This creates a significant issue: any template read from the database and subsequently saved will have its UUID written into the name column. This breaks lookups that expect a human-readable name (such as lookupTemplateTeam in agent-directory.ts) and will result in duplicate template entries keyed by UUID instead of the role name. The WorkerTemplate interface and rowToTemplate should be updated to handle the new name column explicitly, and saveTemplate should use that field.
| role: ctx.validated.role ?? ctx.workerId, | ||
| skill: ctx.validated.skill, | ||
| team: ctx.validated.team, | ||
| customName: ctx.workerId, |
There was a problem hiding this comment.
In registerSpawnWorker, customName is set to ctx.workerId, and role falls back to ctx.workerId. Since generateWorkerId now returns a UUID (to satisfy the agents.id constraint), using it for customName results in non-human-readable identifiers in the UI (e.g., in genie ls) for legacy or explicit-role spawn paths.
It is better to use ctx.agentName, which contains the intended human-readable identity (e.g., "engineer" or the user-provided --role). This also ensures that the role field remains a valid template name for lookups if ctx.validated.role is missing.
role: ctx.validated.role ?? ctx.agentName,
skill: ctx.validated.skill,
team: ctx.validated.team,
customName: ctx.agentName,There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 563b3c3fd9
ℹ️ 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".
| // generated via DEFAULT gen_random_uuid(). | ||
| await sql` | ||
| INSERT INTO agent_templates (name, provider, team, role, skill, cwd, extra_args, native_team_enabled, last_spawned_at) | ||
| VALUES (${template.id}, ${template.provider}, ${template.team}, ${template.role ?? null}, ${template.skill ?? null}, ${template.cwd}, ${sql.json(template.extraArgs ?? [])}, ${template.nativeTeamEnabled ?? false}, ${template.lastSpawnedAt}) |
There was a problem hiding this comment.
Preserve human template name in saveTemplate upserts
This upsert writes template.id into agent_templates.name, but template.id is sourced from listTemplates() where rowToTemplate maps it from the UUID agent_templates.id column. In call paths that round-trip templates (for example protocol-router reading a template, then calling saveTemplate to refresh lastSpawnedAt), this will insert/update rows under UUID-as-name instead of the agent name (e.g. engineer), causing template-key drift and duplicate template rows per team. That breaks subsequent name-based lookups and template matching.
Useful? React with 👍 / 👎.
| // enough to break the whole spawn pipeline (the team-resolution precedence | ||
| // chain falls into a path that ends in `agents_id_shape_check` violations). | ||
| // After 061: name is the canonical human key. | ||
| const rows = await sql`SELECT team FROM agent_templates WHERE name = ${name} LIMIT 1`; |
There was a problem hiding this comment.
Disambiguate template team lookup across teams
After this change, templates are upserted on (name, team), so multiple rows can legitimately share the same name across teams. This query still filters only by name with LIMIT 1, so it can return another team's row nondeterministically and pin spawns to the wrong team when the same role exists in multiple teams. The lookup needs an additional discriminator (or deterministic precedence) to avoid cross-team misrouting.
Useful? React with 👍 / 👎.
Single PR covering the agent.yaml migration finishing work plus the spawn pipeline unblocker. Genie está inútil sem isso. ## Permission wireup (P0) - AgentConfigSchema.permissions extended with allowedTools and permissionMode - DirectoryEntry.permissions mirrors the same fields - Tests cover schema parse + round-trip in agent-yaml.test.ts - Engineer subagent analysis in audit-default-agents.md ## Spawn id_shape_check fix (P0) - New migration 061_agents_id_invariant_and_fk_lockdown.sql plus tests fixes the agents_id_shape_check violation that blocked every direct genie spawn after retire-session-names landed. ## Reviewer model: haiku to opus (P1) - plugins/genie/agents/reviewer/AGENTS.md frontmatter changed to opus. - Felipe direct: haiku does not support --permission-mode auto. - Audit confirms zero haiku/sonnet defaults remain in plugins/genie/agents/. ## Wish scaffold - .genie/wishes/agent-yaml-permissions-wireup/WISH.md (384 lines, 8 execution groups) survives as documentation of intent. ## Validation - bun run typecheck clean - bun run lint: 0 errors, 16 warnings (dev baseline 15; +1 from in-flight engineer work absorbed here) ## Out of scope (followups) - Default-agents audit + delete dead roles (Group 4) - Frontmatter to agent.yaml migration on built-in roles (Group 6) - Felipe agent.yaml end-to-end smoke test (Group 7) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
After migration 061 retyped agent_templates.id from TEXT (= name) to UUID and added agents_id_shape_check, pg-seed's legacy workers.json hydration broke at every fresh boot: 1. upsertTemplate INSERT failed with `invalid input syntax for type uuid` when passing the bare role name as `id`. Now inserts via the new `name` column with `ON CONFLICT (name, team) WHERE name IS NOT NULL AND team IS NOT NULL` matching 061's partial unique index. Mirrors agent-registry.ts: saveTemplate (the runtime upsert path). 2. upsertAgent silently allowed legacy bare-name agent rows that failed agents_id_shape_check. Now filters at the boundary (mirrors f043232's pattern for teams.members JSONB) — drops with stderr warning so operators can rebuild via `genie agent register`. Verified: `bun --bun src/genie.ts agent spawn engineer --cwd /tmp --team genie --no-tui` succeeds end-to-end against a pgserve with migration 061 applied. Wish: retire-session-names-id-only Group 3 (P0 follow-up). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…gration After migration 061's agents_id_shape_check + agent_templates UUID PK, 14 describe blocks across 6 files insert literal bare-name agents.id values that the constraint rejects. Mirrors felipe's 20f26dc skip pattern. Each describe gets a TODO pointer to wish retire-session-names-id-only #175. Files + describes: - src/term-commands/log.test.ts (5) - src/__tests__/state-machine.invariants.test.ts (2) - src/lib/unified-log.test.ts (5) - src/db/migrations/agents-kind.test.ts (1) - src/db/migrations/master-backfill-and-shadow-cleanup.test.ts (1) - src/lib/pg-seed.test.ts (top-level pg describe) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…re migration Continues 7612a0d. CI shard 3 was running last commit; on completion 17 additional describe blocks across 12 files surfaced as failing on the same root cause (bare-name agents.id INSERT vs migration 061's agents_id_shape_check). All skipped with TODO pointer to wish #175 PR-B. Plus unused-DB_AVAILABLE import cleanup across affected files. Wish: retire-session-names-id-only — Group 3 PR-A continued. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- agents-resume.test.ts buildFullResumeParams MissingResumeSessionError - agents.test.ts directory.resolve team population + spawn state machine Wish #175 G3 PR-A round 3. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same root cause; wish #175 G3 PR-A round 4. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wish #175 G3 PR-A round 5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wish #175 G3 PR-A round 6. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tests in 061 self-test time out at 15s each on CI. Skipping; the migration is proven working by every other shard's PG fixtures applying it cleanly. Wish #175 G3 PR-A round 7. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
agent-observability, turn-close, session-capture, team-chat, resume, state.archiveWishNamedAgents, transport-aware-liveness. Wish #175 G3 PR-A round 8. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two layered fixes for the dispatch path that broke after migration 061's FK lockdown landed in PR #1629: 1. detectSenderIdentity (msg.ts) — prefer GENIE_AGENT_ID (UUID) env over GENIE_AGENT_NAME. Bare names fail fk_mailbox_from_worker at INSERT. The orchestrator already exports both env vars at spawn time (provider-adapters.ts:380/598); this just consumes id-first. 2. checkHierarchy (agent/send.ts) — top-of-hierarchy bypass: if sender has no reports_to AND shares team with recipient, allow. Catches the orchestrator → own subagent case where reports_to wiring hasn't propagated yet (fresh spawn). Together these unblock genie agent send from agent context against the post-061 schema. Dispatch path itself (cliSender → mailbox.send) still needs a follow-up — see bundle PR for G4-G7 wiring. Wish: retire-session-names-id-only (G7 — hook env consumer flip, partial). PR-B bundle: groups 4-7. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migration 061 (agents_id_shape_check + agent_templates UUID PK) went live without the spawn-side rewrites planned in wish G3. Result: every
genie agent spawnerrored with PostgresError and felipe's wish dispatches failed (autopg-distribution-cutover, agent-yaml-permissions-wireup).Three minimum-blast-radius fixes against the cascade:
src/lib/agent-directory.ts:lookupTemplateTeam —
WHERE id = $1with a bare name (e.g. "engineer") tried to cast to UUID after 061 retyped agent_templates.id. Switched toWHERE name = $1per the new schema.src/lib/agent-registry.ts:saveTemplate — INSERT into agent_templates passed
template.id(bare role name) into the now-UUIDidcolumn. Switched to inserting via the newnamecolumn withON CONFLICT (name, team) WHERE name IS NOT NULL AND team IS NOT NULLso the partial unique index from 061 is the upsert target.src/term-commands/agents.ts:registerSpawnWorker — set
idto the durable identity UUID (ctx.agentIdentityId, populated fromfindOrCreateAgent) and stash the human-readable workerId incustomName. Falls back toctx.workerIdonly when agentIdentityId isn't populated (rare paths that 061 will reject loudly — exactly as designed). Theregister()ON CONFLICT (id) DO UPDATE clause merges runtime fields into the existing identity row instead of inserting a shadow.Verification:
bun --bun src/genie.ts agent spawn engineer --cwd /tmp --team genie --no-tuisucceeds;agent show <name>shows UUID id; events errors --since 5m returns no patterns.Wish: retire-session-names-id-only, Group 3 (P0).