Repository navigation
fix(v5): harden roadmap sync per PR #2729 bot-review findings - #2734
Conversation
- task export/import: resolve explicit paths before the canonical comparison, so a relative spelling of .genie/roadmap.json preserves local hires and records the sync baseline - task export --write <file>: custom-file backups emit the complete state including hire_roster; only canonical roadmap.json gets the roadmap slice, keeping backup round-trips lossless - importState: --replace wipes all tables even when only meta rows exist, so stale wish_sig/backfill markers cannot merge into the snapshot; malformed rows that pass shape validation rethrow as SnapshotFormatError after rollback - syncRoadmap: compare-and-act runs under BEGIN IMMEDIATE, closing the window where a concurrent task write could be destroyed by a replace-import and serializing concurrent syncs - pre-commit: stage roadmap.json only when sync itself rewrote it, never absorbing pre-existing unstaged edits - post-checkout (new hook): run task sync after branch checkouts so the shared db stops exposing the previous branch's board - .hermes/SOUL.md: narrow the obsolete-serve prohibition so it cannot be read as banning the supported 'genie omni serve' - release-docs.test.ts: assert the security-sensitive bypass rule in router metadata too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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: 594a273e69
ℹ️ 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".
| // machine-local worktree paths). Custom-file writes and the plain stdout | ||
| // dump stay the complete database, so a backup file round-trips lossless. | ||
| const target = opts.write ? (typeof opts.write === 'string' ? resolve(opts.write) : resolveRoadmapPath()) : null; | ||
| const canonical = target === resolveRoadmapPath(); |
There was a problem hiding this comment.
When this command runs inside a linked worktree and the user explicitly passes .genie/roadmap.json, resolve(opts.write) points to that worktree's tracked file while resolveRoadmapPath() deliberately points to the main checkout via the common Git directory, so canonical is false. The export then writes the complete hire_roster, including machine-local worktree paths, into a tracked roadmap; the mirrored import comparison also treats that file as a full backup and can wipe local hires with --replace. I reproduced this from a linked worktree, which is a supported Genie execution mode, so recognize the linked checkout's roadmap as canonical or reject that target.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (err instanceof SnapshotFormatError || err instanceof NonEmptyImportError) throw err; | ||
| // A malformed row that passed shape validation (roadmap.json survives git | ||
| // merges) surfaces as an opaque SQLite bind error — rethrow it in the same | ||
| // actionable class as the structural checks. The transaction rolled back. | ||
| throw new SnapshotFormatError( | ||
| `Snapshot rows could not be imported: ${err instanceof Error ? err.message : String(err)}. The database was left unchanged; re-export the snapshot or repair the malformed rows.`, |
There was a problem hiding this comment.
When another process holds the SQLite write lock longer than busy_timeout, apply() throws a retryable SQLITE_BUSY, but this blanket conversion reports SnapshotFormatError and instructs the user to repair or re-export a snapshot that is not malformed. Preserve transient and operational SQLite errors here, and only translate bind or constraint failures that can actually be attributed to malformed snapshot rows.
Useful? React with 👍 / 👎.
Verifies and fixes the bot review findings (Codex + CodeRabbit) on rolling PR #2729. Each finding was checked against the actual code; invalid ones are dispositioned in the PR #2729 review discussion.
Fixed (verified real):
task export/import: path-normalize before the canonical-roadmap comparison — an explicit relative.genie/roadmap.jsonno longer wipes localhire_rosteror skips the sync baseline (Codex P1)task export --write <file>: custom-file backups now carry the complete state incl.hire_roster; only the canonical path gets the roadmap slice, sobackup.json → import --replaceround-trips lossless (Codex P2)importState:--replacewipes all tables even when onlymetarows exist — stalewish_sig/backfill markers can no longer merge into the snapshot and trigger false drift (Codex P2); malformed rows that pass shape validation now rethrow asSnapshotFormatErrorafter rollback instead of raw SQLite bind errors (CodeRabbit)syncRoadmap: whole compare-and-act sequence underBEGIN IMMEDIATE— closes the TOCTOU where a task write committed between hash comparison and replace-import would be silently destroyed, and serializes concurrent syncs (git hook vs interactive command) via busy_timeout (Codex P1 + CodeRabbit major).husky/pre-commit: stagesroadmap.jsononly when sync itself rewrote it (hash-compare before/after) — pre-existing unstaged edits are no longer absorbed into unrelated commits (Codex P1).husky/post-checkout(new): same three-waytask syncafter branch checkouts ($3=1 only), so the shared db stops exposing the previous branch's board (Codex P1).hermes/SOUL.md: prohibition narrowed to the obsolete top-levelgenie serve;genie omni serveexplicitly exempt (CodeRabbit)scripts/release-docs.test.ts: router metadata (agents/openai.yaml) now asserted to carry the security-sensitive bypass rule (CodeRabbit)Regression tests added: explicit-relative-canonical-path behavior + lossless custom backup (v5-task.test.ts); metadata-only replace wipe + SnapshotFormatError rollback (task-state.test.ts).
Validation: typecheck, biome, knip, skills/wishes lint, complexity budget, hook-bundle parity all green; full
bun testgreen in all touched areas (remaining local failures are pre-existing umask-002 / GitHub-release-asset environment issues, absent under umask 022 and green in CI).🤖 Generated with Claude Code