Repository navigation
fix(teams): back-fill PG teams row from ensureNativeTeam - #1165
Conversation
Symptom: after a reboot that wiped the pgserve data dir (or any scenario
where PG state is lost while the on-disk native team config at
~/.claude/teams/<name>/config.json survived), `genie team ls` returned
an empty list even though the team was fully usable via the native
protocol. The two registries — PG `teams` table and native team files —
drifted apart and no code path reconciled them.
Fix: add `ensureTeamRow(name, { repo })` to team-manager as a
lightweight, idempotent back-fill, and call it from `ensureNativeTeam`
after the native config is either loaded or saved. Mirrors newly
minted (or surviving) native teams into the PG registry so `team ls`
reflects reality.
Design choices to keep this minimal:
- `ensureTeamRow` does NOT create a git worktree — `worktreePath`
defaults to `repo` (current cwd). If a user later runs
`genie team create <same-name>`, its own `getTeam` check returns
the back-filled row and skips worktree creation. Callers who need
a real worktree must run `createTeam` explicitly.
- `INSERT ... ON CONFLICT (name) DO NOTHING` — never clobbers an
explicit `createTeam` row.
- `backfillTeamRow` in claude-native-teams.ts uses a dynamic import to
avoid circular dependency with team-manager.ts, and swallows errors
so the native-team path is never blocked by PG issues.
- Invalid branch names are rejected silently (returns null) since the
caller is `ensureNativeTeam`, which is on a hot path.
Tests: 4 new cases in team-manager.test.ts covering fresh insert,
idempotency, non-clobber of createTeam rows, and invalid name
rejection. Full suite: 75/75 pass across team-manager + native-teams.
Co-Authored-By: Claude Opus 4.7 (1M context) <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 implements a back-fill mechanism to synchronize on-disk native teams with the PostgreSQL database, ensuring consistency in team listings. The review feedback identifies opportunities to improve performance by making the back-fill operation non-blocking and optimizing the database insertion logic to eliminate redundant queries and refine audit logging.
| // Back-fill the PG teams row if it's missing (e.g. after a pgserve reset | ||
| // where the on-disk native team survived but the `teams` row did not). | ||
| // Best-effort — never block the native team code path on PG failures. | ||
| await backfillTeamRow(sanitizeTeamName(teamName)); |
There was a problem hiding this comment.
The backfillTeamRow function is intended to be a best-effort, non-blocking operation. However, it is currently being awaited, which means if the database is slow to respond or if pgserve needs to be auto-started (which has a 16-second timeout), the "hot" spawn path will be significantly delayed. Since the native team functionality does not depend on the PostgreSQL row being present immediately, this call should not be awaited.
| await backfillTeamRow(sanitizeTeamName(teamName)); | |
| backfillTeamRow(sanitizeTeamName(teamName)).catch(() => {}); |
| await saveConfig(teamName, config); | ||
| // Mirror the newly created native team into the PG `teams` registry so | ||
| // `genie team ls` reflects reality. Idempotent and best-effort. | ||
| await backfillTeamRow(sanitized); |
| await sql` | ||
| INSERT INTO teams ( | ||
| name, repo, base_branch, worktree_path, leader, | ||
| members, status, native_teams_enabled, created_at | ||
| ) VALUES ( | ||
| ${config.name}, ${config.repo}, ${config.baseBranch}, | ||
| ${config.worktreePath}, ${null}, | ||
| ${JSON.stringify(config.members)}, ${config.status}, | ||
| ${config.nativeTeamsEnabled ?? false}, ${config.createdAt} | ||
| ) ON CONFLICT (name) DO NOTHING | ||
| `; | ||
| recordAuditEvent('team', name, 'backfilled', getActor(), { repo: repoPath, source: 'native-team' }).catch(() => {}); | ||
| return (await getTeam(name)) ?? config; |
There was a problem hiding this comment.
This block can be optimized to avoid a redundant SELECT query when a new row is successfully inserted. Additionally, the audit event should only be recorded if a row was actually backfilled (i.e., when the INSERT didn't conflict with an existing row).
const result = await sql`\n INSERT INTO teams (\n name, repo, base_branch, worktree_path, leader,\n members, status, native_teams_enabled, created_at\n ) VALUES (\n ${config.name}, ${config.repo}, ${config.baseBranch},\n ${config.worktreePath}, ${null},\n ${JSON.stringify(config.members)}, ${config.status},\n ${config.nativeTeamsEnabled}, ${config.createdAt}\n ) ON CONFLICT (name) DO NOTHING\n `;\n\n if (result.count === 0) {\n return (await getTeam(name)) ?? config;\n }\n\n recordAuditEvent('team', name, 'backfilled', getActor(), { repo: repoPath, source: 'native-team' }).catch(() => {});\n return config;There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f9e7d3707
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Back-fill the PG teams row if it's missing (e.g. after a pgserve reset | ||
| // where the on-disk native team survived but the `teams` row did not). | ||
| // Best-effort — never block the native team code path on PG failures. | ||
| await backfillTeamRow(sanitizeTeamName(teamName)); |
There was a problem hiding this comment.
Backfill using canonical team name, not sanitized slug
ensureNativeTeam backfills PG with sanitizeTeamName(teamName), which changes branch-style team names like feat/auth into feat-auth. For teams originally created through createTeam, this misses the existing row (name='feat/auth') and can insert a second, mismatched row or collide with a real feat-auth team. That breaks team identity consistency across commands (getTeam, listTeams, disband/hire/fire) whenever names include / or other sanitized characters.
Useful? React with 👍 / 👎.
| name, | ||
| repo: repoPath, | ||
| baseBranch: 'dev', | ||
| worktreePath: repoPath, |
There was a problem hiding this comment.
Do not set backfilled worktreePath to repo directory
The backfilled row stores worktreePath equal to repoPath. Later, disbandTeam calls removeWorktree(config.worktreePath), and that helper falls back to recursive rm when git worktree remove fails; for a backfilled team this targets the repo directory itself. In practice, disbanding one of these teams can delete the main checkout (or a user’s current subdirectory if process.cwd() was used), which is destructive data loss.
Useful? React with 👍 / 👎.
Summary
Genie keeps two parallel team registries: PG
teams(queried bygenie team ls,listTeams, etc.) and on-disk~/.claude/teams/<name>/config.json(used by the Claude Code native-team IPC protocol). Onlygenie team createwrites to both. Every other spawn path that reachesensureNativeTeam(implicit team creation duringgenie agent spawn) writes ONLY to disk.After a reboot that resets pgserve data (or any PG-loss scenario with the native team file intact),
genie team lsreturns empty even though the team is fully operational on disk. No code path reconciles the two sources — this was observed on a real reboot as part of the same incident that motivated #1163 and #1164.Fix
Add
ensureTeamRow(name, { repo })inteam-manager.tsas a lightweight, idempotent back-fill. Call it fromensureNativeTeamafter the native config is either loaded or freshly saved.Design — minimal by intent
ensureTeamRowdoes not create a git worktree.worktreePathdefaults torepo(the current cwd). If a user later runsgenie team create <same-name>, the existinggetTeamcheck returns this back-filled row and skips worktree creation. Callers who need a real worktree must runcreateTeamexplicitly before spawning.INSERT ... ON CONFLICT (name) DO NOTHING— never clobbers an explicitcreateTeamrow.claude-native-teams.tsusesawait import('./team-manager.js')to avoid a circular dependency (team-manager already imports claude-native-teams), and swallows all errors so the native-team code path is never blocked by PG issues.nullsilently rather than throwing, because the caller is on a hot spawn path.Not in scope
Unifying the two registries into a single source of truth is a bigger design change and is deliberately not attempted here. This PR only eliminates the empty-
team lsdrift after reboot. A full unification needs a separate design discussion — e.g. should native-team files become a projection of the PG table, or vice versa, or shouldgenie team lsmerge both?Tests
Four new cases in
team-manager.test.ts:createTeamFull suite: 2487/2487 pass. Team + native-teams files alone: 75/75.
Test plan
🤖 Generated with Claude Code