Land three-harness audit fixes: schema backfill lockstep, freshness injection gate, shortcuts exact-snippet uninstall - #2752
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 73ac11a141
ℹ️ 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".
|
|
||
| function ensurePromptRl(): readline.Interface { | ||
| if (promptRl) return promptRl; | ||
| promptRl = readline.createInterface({ input: process.stdin, output: process.stdout }); |
There was a problem hiding this comment.
When shortcuts install or shortcuts uninstall is run from an interactive terminal, stdin never reaches EOF and this persistent readline interface is never closed. After the final answer, the command prints its completion message but remains alive indefinitely until the user sends EOF or interrupts it; close or pause the interface once the command has finished prompting.
Useful? React with 👍 / 👎.
| while (kept.length > 0 && kept[kept.length - 1] === '') kept.pop(); | ||
| const newContent = kept.length > 0 ? `${kept.join('\n')}\n` : ''; |
There was a problem hiding this comment.
When a pre-existing rc file lacks a final newline or ends with multiple blank lines, an install/uninstall round trip changes those user-owned bytes: the loop removes every trailing blank line and the reconstruction always adds exactly one newline. Remove only the exact appended byte range or retain the untouched prefix and suffix so uninstall restores the original config exactly.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
completeTask violated the never-clobbered invariant claimTask/releaseTask
already implement: read-then-unconditional-UPDATE let any caller finish
another worker's claimed card. Move the claim predicate into the UPDATE:
WHERE id = ? AND status IN ('ready','in_progress')
AND (claimed_by IS NULL OR claimed_by = ?)
Identity reconciliation FIRST (this commit's resolver change, predating the
SQL): claim-side (handleCheckout worker = GENIE_AGENT_NAME ?? 'cli') and
complete-side (resolveEventAuthor author = GENIE_AGENT_NAME ??
GENIE_AGENT_ID ?? null) were two diverging fallback chains - a
GENIE_AGENT_ID-only runtime claimed as 'cli' but completed as the ID.
Both now resolve through resolveWorkerIdentity (NAME ?? ID ?? 'cli'), so a
plain no-env 'genie task checkout' -> 'genie task done' still completes.
Zero changed rows -> typed TaskCompleteError mirroring releaseFailure
(card status carried for the operator). Identity-less completion of a
claimed card is refused; unclaimed cards remain completable without
identity. No force/override knob - release-then-reclaim composes from
existing verbs. Migration for in-flight claims: release (stays
identity-free) then re-claim under the new identity.
Tests: racing identities (claimant wins, other gets typed refusal),
non-claimant refused, identity-less complete of claimed card refused,
default-CLI regression, done-guard.\n\nCo-Authored-By: Empryo <noreply@empryo.com>
The wish-group execution machine (state transitions, drift signatures, cycle detection, typed errors) has zero production callers — wish_groups has no writer, so every MCP wish-status query returned a guaranteed-empty set. Deleted by exact symbol list per the harness-audit-landing wish: assertWishSignature, startWishGroup, completeWishGroup, promoteReadyGroups, getWishGroups, getWishGroup, mapWishGroup, createWishGroups, computeGroupsSignature, wishSigKey, validateGroups, validateGroupRefs, detectGroupCycles, WishGroupStateError, WishGroupDriftError, WishGroupDef. Asymmetric by design: - CREATE TABLE IF NOT EXISTS wish_groups DDL stays inert, annotated vestigial-pending-drop; no schema version bump (older binaries' validateSnapshot expects the key). - exportState keeps emitting wish_groups: [] via the live query (table is empty in production); SNAPSHOT_TABLE_KEYS unchanged. - importState tolerates-and-drops a legacy wish_groups snapshot array instead of inserting rows. - genieWishStatus preserves groups: [] in WishStatusPayload (WishGroupRow survives to type it); resolveWishBranch loses the verified-launch- worktree step — safe, since no production writer exists. - listWishSlugs (UNION on wish_groups), hasOperationalState, wipeAllTables untouched; group_name on tasks untouched. - TAXONOMY.md loses the state-machine prose; column reference stays with the vestigial note. Test rework: wish-group state-machine blocks removed; export round-trip asserts the field is empty; mcp/ui-bridge tests assert groups is [].
dev's Codex Plugin Smoke has been red since b0e77cd (skills runtime-generic refactor, PR #2736) deleted plugins/genie/skills/wizard: the retirement only retires a fallback whose skill the authenticated payload ships (proven-ownership scoping in runtime-integrations.ts), so the seeded wizard fallback is preserved and the smoke's hardcoded '23 retired' / '46 skill cards' assertions fail (journal accepted 22, expected 23). The oracle now derives its expectations instead of hardcoding them: - pure-23/mixed: expected retired = seeded ∩ payload-shipped skills; dropped-skill fallbacks are asserted PRESERVED (strengthens the oracle). - post-upgrade doctor assertions accept N preserved clean fallbacks. - doc-contract: mirror parity (plugins/genie/skills vs skills/) plus non-empty, instead of a rot-prone hardcoded card count. Validated on both compositions: payload with wizard (23 retired) and without (22 retired, wizard preserved); full suite green on 1.3.14.
Group 2 (claimed_by CAS fence) required every claim-then-complete caller to thread identity; two callers were missed by the wish's four-file enumeration and broke CI: - tests/integration/codex-project-route-migration.test.ts statically imported createWishGroups (deleted in the Group 3 wish-group removal) — the integration file is env-gated on CI so the crash only surfaced there. Removed the dead import/call; the sentinel assertions only use wish slugs, the group row was incidental. - tests/e2e/v5-lifecycle.sh claimed T1 as $WORKER then completed with no identity — resolveEventAuthor() floors at 'cli', which the fence refuses. Completion now threads GENIE_AGENT_NAME=$WORKER. Both verified locally: integration file 2/2, e2e PASS.
Merge origin/dev into wish/harness-audit-landing: carries the merged roadmap-truth + kimi-plugin + auto-version changes, resolving the mcp read-only proof test graft (dev version adapted to the wish-group removal) and keeping the branch mergeable for #2752.
The auto-version bump (f5bbadc) ran before the kimi manifest joined this branch's tree line, so plugins/genie/.kimi-plugin/plugin.json was left at 5.260805.1 while package.json moved to 5.260807.2 — the same drift class #2757 fixed on dev. Align it so the release-payload gate passes on the merge tree.
All three groups shipped: ready-fixes PR #2752 open with gate-zero receipt, completeTask fence landed, wish-group machinery deleted.
Follow-up fixes from the #2752 post-merge review
1 participant
Gate-zero receipt — harness-audit-landing
bun install, NO stashing.bun run check:fastgreen;bun test2994 pass / 18 fail (3012 tests, 126 files). Scratch worktree extraction mangled file modes (666/777 vs committed 644/755) — modes restored to committed values and the role-agent parity gate re-run green.bun run check:fastgreen exceptwishes:lint, which fails ONLY on the unrelated untracked.genie/wishes/roadmap-truth/WISH.md(unsupported status REVIEWED; stale design SHA) — not this wish's files.bun test3014 pass / 13 fail (3027 tests, 127 files).Gate-zero receipt (Group 1)
bun --version= 1.3.14 (engines require >=1.3.10) ✓73ac11a14, freshbun install, no stash):bun run check:fastgreen;bun testgreen except 10 infra/timing failures — 6reconcile-release-assetstests timing out against live GitHub API calls, 1 doctor 120 ms latency-budget test, 3 codex integration harness-root spawn-cleanup tests. None touch files this branch modifies.bun run check:fastgreen;bun test3014 pass / 14 fail — the same failure classes (release-reconciliation API timeouts, doctor latency budget, integration spawn cleanup; the exact set shifts run-to-run as the tests hit live GitHub). Zero failures attributable to this branch's changes.bun testpassed on the merge ref).Dev CI repair carried by this PR
dev was red on two gates after the #2751 (roadmap-truth) and #2736 (kimi-plugin) merges:
REVIEWED+ a stale design-evidence digest. Fixed on dev via fix(ci): restore dev gate after roadmap-truth merge (wishes-lint + codex smoke) #2754 (status -> SHIPPED; evidence re-stamped with the re-reviewed content digest; blocks edge qualified cross-repo).b0e77cd15deletedplugins/genie/skills/wizard; the retirement only retires payload-shipped skills, so the hardcoded "23 retired" oracle broke (journal accepted 22). The smoke now derives expectations from the shipped payload (e1331c188): retired = seeded intersect shipped skills, dropped-skill fallbacks asserted preserved, doc-contract asserts mirror parity instead of a hardcoded 46-card count. Validated on both payload compositions.