fix(cli-hygiene): kill-path dedup + pgserve stderr gate (G8 + G9, closes #1677) - #1685
Conversation
|
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 |
![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: bfe1516c29
ℹ️ 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".
| `) as { id: string }[]; | ||
| if (remaining.length > 0) return []; | ||
| const dirId = `dir:${displayName}`; | ||
| const removed = (await tx<{ id: string }[]>`DELETE FROM agents WHERE id = ${dirId} RETURNING id`) as { |
There was a problem hiding this comment.
When killing a UUID row, this code deletes dir:<name> by id only after checking for remaining UUIDs in the killed row's team. If another team owns the dir:<name> row (while this team only has a UUID peer with the same custom_name), killing the UUID here will remove that other team's directory identity unexpectedly. This is reproducible with the cross-team sibling shape already used in tests (same custom_name in different teams) and can orphan the surviving team's runtime row.
Useful? React with 👍 / 👎.
| SELECT id FROM agents | ||
| WHERE custom_name = ${displayName} AND team IS NOT DISTINCT FROM ${team} | ||
| `) as { id: string }[]; |
There was a problem hiding this comment.
The paired-row lookup for dir: kills selects by custom_name and team but does not exclude the matched row itself. Because directory rows normally have custom_name=<name>, paired will include w.id even when no UUID twin exists, causing false "paired row(s) also removed" output and spurious kill.dedup_paired audit events for single-row deletes.
Useful? React with 👍 / 👎.
![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 contains documentation and QA planning updates related to the 'CLI Noise and Hygiene Cleanup' wish, specifically documenting the audit results of recent fixes and the QA plan for verifying them. As there are no review comments provided for this pull request, I have no feedback to offer.
…coped per reviewer Lands the wish doc that scaffolds PR-A (#1634) and PR-B (#1636/#1637/#1638/ #1640/#1642), plus the 2026-05-07 PR-C draft + reviewer FIX-FIRST corrections. Why this is a separate docs commit: - The wish file was authored 2026-05-04 but only ever sat in a stash; never committed despite shipping work referencing it. This commit lands the reference document for completed + pending work in one place. - PR-C as originally drafted had three invalid premises against live 4.260507.1 (G3 amendment already implemented at scheduler-daemon.ts:1296; G9 line is on stderr not stdout; G10 design assumes binary-spawn that the HTTP probe doesn't do). Reviewer corrections folded in. - Only G8 (kill-path shadow+UUID dedup) survives intact — file path corrected to src/term-commands/agents.ts:2817 (handleWorkerKill). - G9 reframed as stderr-noise reduction (DEBUG=pgserve gating). - G10 deferred pending /trace into update.ts:362. QA dogfooding-72h artifacts (AUDIT.md, QA-PLAN.md) document the 72-h fix-audit sweep that surfaced the bugs and triggered the wish update. Refs: #1677 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
#1677 (G8) Killing a `dir:<name>` shadow OR its paired UUID twin now removes both halves of the logical agent in one atomic transaction. Today's behavior left the other half alive in `genie ls`, forcing operators into a second kill to clean up the visual zombie (proven on 2026-05-07: 7× `dir:codex-*` kills left 7× UUID twins in `error` state). - New helper `killAgentWithDedup` in `src/term-commands/agents.ts` issues a single transactional cascade: `dir:` kill → all UUID twins for the same (name, team); UUID kill → `dir:` shadow when no other UUIDs share the name. - New audit event `agent.kill.dedup_paired { matched, paired }` fires once per cascade for forensic traceability. - `--keep-paired` escape hatch preserves today's single-row behavior for the rare case an operator wants the surviving half to study. - Tests at `src/term-commands/agents.test.ts -t "kill dedup paired"` cover: kill-dir cascade, kill-UUID cascade, both `--keep-paired` variants, and cross-team isolation under migration 061's unique constraint. Closes #1677 (G8 of wish cli-noise-and-hygiene-cleanup PR-C). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…closes #1677 (G9) Every CLI invocation that touches the DB printed `[pgserve] connected to <db>` on stderr. The line is on stderr (so JSON-on-stdout pipelines still work) but clutters every operator terminal. Gate it behind `DEBUG=pgserve`, matching the G1 pg-seed pattern; default-mode operator terminals stay quiet, debug recovery still works. - `src/lib/db.ts:maybePrintBanner` now requires `process.env.DEBUG?.includes('pgserve')` before emitting. Other audit-worthy stderr writes in the same file (retention warnings, pgserve cwd-pin failures, GENIE_PROFILE_DB instrumentation) get explicit `// emit-discipline: ok — <reason>` markers. - New `_resetBannerForTest` export keeps the module-level `bannerPrinted` flag testable without touching production paths. - `tools/lint/emit-discipline-connection.ts` adds a CI gate that flags any new informational `process.stderr.write` / `console.error` in connection/bootstrap modules without an exemption marker. Wired into `bun run check:fast` via `scripts/lint-emit-discipline.ts`. - Tests at `src/lib/db.test.ts -t "no default stderr emit on connect"` pin the gating contract: default mode silent, `DEBUG=pgserve` (and comma-list variants) recover the line, plus a defense-in-depth source-string check that fails if the gate is ever removed. Live verified on dist/genie.js: ./dist/genie.js ls --json 2>&1 1>/dev/null # silent DEBUG=pgserve ./dist/genie.js ls --json 2>&1 1>/dev/null # one banner line Closes #1677 (G9 of wish cli-noise-and-hygiene-cleanup PR-C). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…paired lookup Two findings from Codex review on PR #1685: - **P1 (high):** killing a UUID owned by team B was deleting `dir:<name>` even when the dir shadow belonged to team A. The dir-shadow delete now requires `team IS NOT DISTINCT FROM ${team}` so a UUID kill in one team can never orphan another team's directory identity. New regression test: `UUID kill respects team scope when dir shadow lives in another team`. - **P2 (medium):** the dir-kill paired lookup matched the dir row itself when legacy shadows carry `custom_name = <name>` alongside the `dir:` prefix — emitting a false "paired row(s) also removed" message and a spurious `agent.kill.dedup_paired` audit event for what is really a single-row delete. The lookup now excludes the matched row and any other `dir:%` ids. New regression test: `dir kill with no UUID twins reports zero paired even when dir.custom_name is set`. Also fixes the pgserve v2 smoke step in CI: G9 silenced the `[pgserve] connected` banner by default, so the smoke needs to opt in via `DEBUG=pgserve` to keep asserting the connection round-trip without re-introducing operator stderr noise. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bfe1516 to
785ebd5
Compare
1 participant
Summary
Wish
.genie/wishes/cli-noise-and-hygiene-cleanup/WISH.md— PR-C wave, post-reviewer-correction.Two surviving groups from the QA dogfood findings; G3 was dropped (already implemented), G10 stays deferred pending a
/tracepass intoupdate.ts:362.Groups
G8 —
genie agent killdedups shadow + UUID rows in one passsrc/term-commands/agents.ts:handleWorkerKillnow atomically clears both halves of the logical agent (dir:shadow ↔ UUID twin, scoped by team). Operators no longer need a second kill to clean the visual zombie that today'sdir:kill leaves behind ingenie ls.genie agent kill dir:fooremoves BOTH thedir:fooshadow AND any UUID row inagentsnamedfooin the same team. — Live: dedup logic inkillAgentWithDedupissues a singleBEGIN; DELETE dir; DELETE UUIDs; COMMIT;. Testkill dir → both halves removedpasses.genie agent kill <uuid-of-foo>removes BOTH the UUID row AND thedir:fooshadow IF no other UUID instances share the name. — Migration 061'sidx_agents_custom_name_teamunique constraint makes the "no other UUIDs in same team" clause vacuously true; the testkill UUID → dir shadow also removedcovers it. Cross-team siblings stay isolated (testcross-team siblings — killing TEAM-A halves leaves TEAM-B UUID intact).agent.kill.dedup_pairedevent per dedup-active kill. —recordAuditEvent('agent', w.id, 'kill.dedup_paired', getActor(), { matched, paired })fires once when the cascade nuked rows.--keep-pairedpreserves today's single-row behavior. —handleWorkerKill(name, { keepPaired: true })short-circuits the dedup.genie agent kill <name> --keep-pairedexposes it via commander. Tests--keep-paired preserves the dir shadow when killing a UUIDand--keep-paired preserves UUID twins when killing the dir shadowboth pass.kill dir → both halves removedpluskill UUID → dir shadow also removedcover the 2+2 cascade.G9 — Reduce
[pgserve] connected to postgresstderr noisesrc/lib/db.ts:maybePrintBanneris gated behindDEBUG=pgserve(parity with G1 pg-seed pattern). Real warnings (retention failures, cwd-pin failures, profile instrumentation) keep emitting via// emit-discipline: ok — <reason>markers. New CI lint attools/lint/emit-discipline-connection.ts(wired intobun run lint:emit) blocks future informational stderr from leaking back into connection/bootstrap modules.genie ls --jsonproduces clean JSON on stdout AND no[pgserve] connected to postgresline on stderr. — Live:./dist/genie.js ls --json 2>&1 1>/dev/null | head -3is silent.DEBUG=pgserve genie ls --json 2>&1 1>/dev/null | head -5retains today's verbose connection log. — Live: prints[pgserve] connected to postgres.db.ts(retention warning, GENIE_PROFILE_DB profile, two pgserve cwd WARNs) carry// emit-discipline: okmarkers and remain default-on.process.stderr.writein connection/bootstrap modules without the exemption comment. — Verified by removing the gate locally →bun run lint:emitreported 5 violations ondb.ts. With the gate restored, lint is clean.bun run check:fastincludes the new lint rule. —scripts/lint-emit-discipline.tsimports and runscheckConnectionEmitDiscipline().Validation
Live verification of G9:
G10 — Deferred
Post-update verify probe re-spec is blocked on a
/tracepass intosrc/genie-commands/update.ts:362. The wish carries the deferred breadcrumb; do not attempt without trace results first.Closes
#1677
Test plan
bun run check:fast— all gates greendist/genie.jsconfirms default silent / DEBUG=pgserve verbose🤖 Generated with Claude Code