Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds v5 SQLite reconciliation with schema validation, locking, tombstone propagation, durable snapshots, recovery, rollback, and the ChangesV5 database synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as genie db sync
participant Snapshots as db-sync-snapshots
participant Reconciliation as db-reconciliation
participant SQLite as SQLite databases
CLI->>Snapshots: plan or apply synchronization
Snapshots->>Reconciliation: reconcile database images
Reconciliation->>SQLite: validate, lock, mutate, and verify
SQLite-->>Reconciliation: postimage and commit status
Reconciliation-->>Snapshots: reconciliation report
Snapshots-->>CLI: sync, recovery, rollback, or cleanup result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: ac3a1d0c32
ℹ️ 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".
| if (errno[0] !== 0) throw nativeError('readdir', errno[0]); | ||
| return entries; | ||
| } | ||
| const name = new CString(entry, nameOffset).toString(); |
There was a problem hiding this comment.
On the supported Linux/Darwin path, the second CString argument is a byte-length bound rather than a pointer offset, so this decodes the binary dirent header instead of d_name. Consequently list() returns malformed names, matchingGenerationNames() never finds published generations, and recovery, rollback, retention, and staging cleanup all fail to discover their files; after a partial commit, a later sync can proceed without restoring the retained preimages.
Useful? React with 👍 / 👎.
| if (conflicts.length === 0) { | ||
| applyTombstonesToState(left); | ||
| applyTombstonesToState(right); |
There was a problem hiding this comment.
After an unhire has synchronized, both replicas retain the tombstone; rehiring on one replica clears only its local marker. The next bidirectional sync unions the remote marker back, and directional sync preserves it as a destination-only meta row, so these calls delete the newly hired row from both sides. A tombstone therefore needs ordering or explicit resurrection semantics rather than being applied unconditionally, otherwise an agent can never be rehired after its deletion has propagated.
Useful? React with 👍 / 👎.
| if (left.hasSidecars || right.hasSidecars) { | ||
| throw error('invalid-data', 'Hardlink aliases with path-specific SQLite sidecars are ambiguous.'); |
There was a problem hiding this comment.
For hardlink aliases of an ordinary Genie database, Bun can leave an empty -wal and an inert -shm after all handles close, because openDb enables WAL mode. Treating mere pathname existence as ambiguity therefore rejects otherwise safe same-database no-ops; the newly added hardlink-alias cases fail before planning on Bun 1.2.14. The ambiguity check should inspect whether a path-specific sidecar contains relevant live/committed state rather than rejecting every leftover sidecar.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/release-docs.test.ts`:
- Around line 752-760: Update the test describing database reconciliation docs
to normalize README whitespace before asserting text, so expectations such as
the explicit path and combined lock wait claims are unaffected by hard-wrap
reflow. Apply the same collapsed-whitespace comparison to all related assertions
in the test while preserving the existing positive and negative claims.
In `@src/genie.ts`:
- Around line 203-207: Remove the process.argv-based requestedRootCommand check
and rely on installWorkspaceCheck’s existing per-command gating. Update
commandRequiresWorkspace so the db command is treated as standalone alongside
task and board, while preserving workspace validation for other commands.
In `@src/lib/v5/db-reconciliation.test.ts`:
- Around line 64-101: Update spawnFlockHolder to choose the flock library
candidates by platform, matching resolveAdvisoryFlock: use
linuxLibcCandidates(process.arch) on Linux and /usr/lib/libSystem.B.dylib on
darwin. Preserve the existing child setup and readiness polling, and ensure the
lock tests can complete successfully on macOS.
In `@src/lib/v5/db-sync-snapshots.test.ts`:
- Around line 2010-2041: Update the zero-retention test’s private-root discovery
in the test beginning “zero retention uses private 0700 state...” to use the
existing privateRoot.temporaryDirectory fixture seam instead of scanning the
shared OS tmpdir via readdirSync(tmpdir()). Also update the related
privateSnapshotDirectories and inline scans in the nearby tests to inspect only
that fixture directory, preserving the existing assertions and cleanup behavior.
In `@src/lib/v5/db-sync-snapshots.ts`:
- Around line 557-568: Update the renameAt wrapper to match openAt, mkdirAt,
linkAt, and unlinkAt: capture the native rename failure errno and throw
nativeError with the rename operation and errno value instead of a bare Error,
preserving the existing successful path.
In `@src/term-commands/v5-db-sync.test.ts`:
- Around line 75-89: Update the cli spawn helper to isolate each spawned CLI's
global state by setting GENIE_HOME in its env to a temporary directory, while
preserving the existing inherited environment and test-specific variables. Apply
the same isolation to the other spawn helper referenced by the comment so task
and board commands never use the real ~/.genie state.
- Around line 305-339: Replace the positional `index >= 8` assertion in the
`cases` matrix loop with per-case metadata containing the expected stderr
fragment. Update duplicate-option cases to specify “may be specified only once,”
leave other cases without an expectation, and assert the fragment from each test
case’s metadata rather than relying on insertion order.
In `@src/term-commands/v5-db-sync.ts`:
- Around line 118-128: Type DatabaseSyncCliReport.status using the upstream
report-status union rather than string, and update exitCode to exhaustively
handle that union while mapping unknown runtime values to operationalFailure
instead of success. Apply the same status typing and fallback behavior in the
related code around exitCode.
- Around line 462-465: Update cleanupFailureCount to include
report.apply.recovery.cleanupFailures alongside report.apply.cleanupFailures
when apply is present, so recovery-phase cleanup failures contribute to the exit
code and human summary. Preserve the existing rollback cleanup count behavior
when apply is null.
- Around line 147-159: The repeated-option validation must use Commander’s
parsed raw arguments rather than reading process.argv indirectly. Update the
handleSync/action flow and rejectRepeatedOptions call to pass the raw-args slice
from program.parseAsync(args), preserving the existing duplicate detection
behavior.
In `@tests/support/codex-dogfood-harness.ts`:
- Line 271: Update the temporary-directory setup around mkdtempSync and
mkdirSync: do not use mkdirSync to change the existing root permissions. Remove
the redundant mkdirSync call to preserve private temporary-directory
permissions, or replace it with chmodSync only if mode 755 is explicitly
required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06e03218-faed-46a6-a959-6bb0b19e9759
⛔ Files ignored due to path filters (2)
CLAUDE.mdis excluded by!*.mdREADME.mdis excluded by!*.md
📒 Files selected for processing (18)
scripts/release-docs.test.tssrc/genie-commands/__tests__/update-command-publication.test.tssrc/genie-commands/__tests__/update.test.tssrc/genie-commands/local-delivery-repair.test.tssrc/genie.tssrc/lib/agent-sync.test.tssrc/lib/v5/db-reconciliation.test.tssrc/lib/v5/db-reconciliation.tssrc/lib/v5/db-sync-snapshots.test.tssrc/lib/v5/db-sync-snapshots.tssrc/lib/v5/reconciliation-tombstone.tssrc/lib/v5/task-state.test.tssrc/lib/v5/task-state.tssrc/term-commands/ui-bridge.tssrc/term-commands/v5-db-sync.test.tssrc/term-commands/v5-db-sync.tstests/support/codex-dogfood-harness.tstests/support/update-current-boundary-runner.ts
| const ownsRoot = dependencies.root === undefined; | ||
| const root = dependencies.root ?? mkdtempSync(join(tmpdir(), 'genie-dogfood-entry-')); | ||
| mkdirSync(root, { recursive: true }); | ||
| mkdirSync(root, { recursive: true, mode: 0o755 }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const { mkdtempSync, mkdirSync, statSync, rmSync } = require('node:fs');
const { join } = require('node:path');
const { tmpdir } = require('node:os');
const root = mkdtempSync(join(tmpdir(), 'genie-mode-check-'));
const before = statSync(root).mode & 0o777;
mkdirSync(root, { recursive: true, mode: 0o755 });
const after = statSync(root).mode & 0o777;
console.log({ before, after });
rmSync(root, { recursive: true, force: true });
NODERepository: automagik-dev/genie
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- locate file ---\n'
git ls-files | rg '^tests/support/codex-dogfood-harness\.ts$' || true
printf '\n--- relevant context ---\n'
sed -n '240,290p' tests/support/codex-dogfood-harness.ts | cat -n -v
printf '\n--- mkdtemp/mkdir usages in harness ---\n'
rg -n "mkdtempSync|mkdirSync|chmodSync|tmpdir\(" tests/support/codex-dogfood-harness.tsRepository: automagik-dev/genie
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate file ---'
git ls-files | rg '^tests/support/codex-dogfood-harness\.ts$' || true
printf '%s\n' ''
printf '%s\n' '--- relevant context ---'
sed -n '240,290p' tests/support/codex-dogfood-harness.ts | cat -n -v
printf '%s\n' ''
printf '%s\n' '--- mkdtemp/mkdir usages in harness ---'
rg -n "mkdtempSync|mkdirSync|chmodSync|tmpdir\(" tests/support/codex-dogfood-harness.tsRepository: automagik-dev/genie
Length of output: 3231
Do not rely on mkdirSync to change the mkdtemp root mode.
mkdtempSync already creates root, and mkdirSync(root, { recursive: true, mode: 0o755 }) leaves existing directories unchanged. Use chmodSync if 755 is required; otherwise remove this redundant call and keep the temporary-directory permissions private.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/support/codex-dogfood-harness.ts` at line 271, Update the
temporary-directory setup around mkdtempSync and mkdirSync: do not use mkdirSync
to change the existing root permissions. Remove the redundant mkdirSync call to
preserve private temporary-directory permissions, or replace it with chmodSync
only if mode 755 is explicitly required.
Bidirectional synchronization treated every differing shared key as a conflict, even when tasks and wish groups carried a clear updated_at ordering. Stale sandbox mirrors therefore required manual repair instead of converging. Choose the entire row with the greater bigint updated_at for tasks and wish groups and publish it to both databases. Equal-version differences and unversioned records remain fail-closed. Directional sync and tombstones are unchanged. Verified with the full check suite: 3116 tests passed.
Make deletion markers versioned so a later rehire or deletion converges safely. Use immediate task-state transactions to preserve ordering under concurrent writers. Treat empty WAL and SHM hardlinks as inert and retain native errno details. Fail closed on unknown CLI outcomes and count recovery cleanup failures. Make CLI and filesystem tests portable and isolated. Verified with bun run check: 3113 tests passed.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/v5/task-state.ts (1)
1313-1331: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn the hire from inside the transaction.
getHireruns after the immediate transaction commits. A concurrentunhireAgentbetween the commit and this read makesgetHirereturnnull, and theas HireRosterRowcast turns that into a null value typed as non-null. The caller then dereferences null. Move the read inside the transaction so the returned row is the row this call wrote.🐛 Proposed fix
- db.transaction(() => { + return db + .transaction(() => { const priorMarker = db.query('SELECT value FROM meta WHERE key = ?').get(tombstoneKey) as { value: string } | null; @@ ).run(input.wish, input.agentAdapterId, input.profile ?? null, input.worktree, now, state); - }).immediate(); - return getHire(db, input.wish, input.agentAdapterId) as HireRosterRow; + return getHire(db, input.wish, input.agentAdapterId) as HireRosterRow; + }) + .immediate(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/v5/task-state.ts` around lines 1313 - 1331, Update the transaction block in the hire flow to call getHire inside the immediate transaction after the INSERT/UPSERT completes, capture and return that row from the transaction callback, and remove the post-commit getHire call and non-null cast. Ensure the transaction returns the row written by this call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/v5/task-state.ts`:
- Around line 1339-1360: Define and apply a retention rule for reconciliation
tombstones created by unhireAgent, pruning meta rows for wishes that are deleted
or archived while preserving tombstones needed for active reconciliation. Anchor
the cleanup to reconciliationTombstoneMeta and the unhireAgent transaction, and
ensure retained tombstones continue to prevent stale hire records from being
reapplied.
In `@src/term-commands/v5-db-sync.ts`:
- Around line 631-634: Update the raw argument handling in the command-root flow
before handleSync so missing rawArgs is rejected rather than replaced with an
empty array, and preserve the existing process.argv-based `.slice(2)` offset
instead of deriving it from parsed `{ from: 'user' }` arguments.
---
Outside diff comments:
In `@src/lib/v5/task-state.ts`:
- Around line 1313-1331: Update the transaction block in the hire flow to call
getHire inside the immediate transaction after the INSERT/UPSERT completes,
capture and return that row from the transaction callback, and remove the
post-commit getHire call and non-null cast. Ensure the transaction returns the
row written by this call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c865d71d-9243-4cfb-af34-8855bd937bc7
⛔ Files ignored due to path filters (1)
README.mdis excluded by!*.md
📒 Files selected for processing (13)
scripts/release-docs.test.tssrc/genie.tssrc/lib/interactivity.tssrc/lib/v5/db-reconciliation.test.tssrc/lib/v5/db-reconciliation.tssrc/lib/v5/db-sync-snapshots.test.tssrc/lib/v5/db-sync-snapshots.tssrc/lib/v5/reconciliation-tombstone.tssrc/lib/v5/task-state.test.tssrc/lib/v5/task-state.tssrc/term-commands/v5-db-sync.test.tssrc/term-commands/v5-db-sync.tstests/support/codex-dogfood-harness.ts
| export function unhireAgent(db: Database, wish: string, agentAdapterId: string): boolean { | ||
| const res = db.query('DELETE FROM hire_roster WHERE wish = ? AND agent_adapter_id = ?').run(wish, agentAdapterId); | ||
| return res.changes > 0; | ||
| const tombstoneKey = reconciliationTombstoneMeta({ table: 'hire_roster', wish, agentAdapterId, deletedAt: 0 }).key; | ||
| return db | ||
| .transaction(() => { | ||
| const live = db | ||
| .query('SELECT hired_at FROM hire_roster WHERE wish = ? AND agent_adapter_id = ?') | ||
| .get(wish, agentAdapterId) as { hired_at: number } | null; | ||
| const priorMarker = db.query('SELECT value FROM meta WHERE key = ?').get(tombstoneKey) as { | ||
| value: string; | ||
| } | null; | ||
| const priorDeletion = | ||
| priorMarker === null ? null : parseReconciliationTombstoneMeta(tombstoneKey, priorMarker.value); | ||
| let deletedAt = BigInt(Date.now()); | ||
| if (live !== null) deletedAt = maxBigInt(deletedAt, BigInt(live.hired_at) + 1n); | ||
| if (priorDeletion !== null) deletedAt = maxBigInt(deletedAt, priorDeletion.deletedAt + 1n); | ||
| const tombstone = reconciliationTombstoneMeta({ table: 'hire_roster', wish, agentAdapterId, deletedAt }); | ||
| const res = db.query('DELETE FROM hire_roster WHERE wish = ? AND agent_adapter_id = ?').run(wish, agentAdapterId); | ||
| db.query('INSERT OR REPLACE INTO meta (key, value) VALUES (?, ?)').run(tombstone.key, tombstone.value); | ||
| return res.changes > 0; | ||
| }) | ||
| .immediate(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial
Tombstones in meta grow without bound.
Each unhireAgent call writes a permanent meta row, and no path prunes them. Repeated hire/unhire cycles on the same wish reuse one key, so growth is bounded by distinct (wish, agentAdapterId) pairs rather than by call count. Plan a retention rule for wishes that are deleted or archived, so reconciliation input size stays bounded over the lifetime of a repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/v5/task-state.ts` around lines 1339 - 1360, Define and apply a
retention rule for reconciliation tombstones created by unhireAgent, pruning
meta rows for wishes that are deleted or archived while preserving tombstones
needed for active reconciliation. Anchor the cleanup to
reconciliationTombstoneMeta and the unhireAgent transaction, and ensure retained
tombstones continue to prevent stale hire records from being reapplied.
| let root = command; | ||
| while (root.parent !== null) root = root.parent; | ||
| const rawArguments = (root as Command & { readonly rawArgs?: readonly string[] }).rawArgs ?? []; | ||
| handleSync(databaseA, databaseB, options, rawArguments.slice(2), dependencies); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
commander 12.1.0 rawArgs parse from user option
💡 Result:
In Commander.js v12.1.0, you can access the original, unparsed arguments passed to the program via the program.rawArgs property [1]. This property is populated during the execution of .parse() [1]. If your goal is to access arguments that were not consumed as options or command-arguments, Commander provides the following properties on the Command object: 1. program.rawArgs: The full, original array of arguments, typically including the node executable and script path (e.g., [process.argv]) [1]. 2. program.args: The remaining array of arguments that were not consumed by options or command-arguments [2][3]. 3. program.processedArgs: The processed command-argument values (after any custom processing) [4]. If you are using an action handler, you can also access the command object itself (which holds these properties) via this within a standard function declaration (not an arrow function) [5]: program.action(function { console.log(this.rawArgs); console.log(this.args); }); If you are looking to specifically handle arguments after a user option, Commander typically handles option values automatically. If you have "unknown" arguments or options, ensure you are not using .allowUnknownOption() if you want Commander to stop or error, or use it if you intend to capture them [6]. For more granular control over argument parsing, you may refer to the program.parse(argv, { from: 'user' }) syntax, which allows you to define the source of the arguments [2][3].
Citations:
- 1: https://tj.github.io/commander.js/
- 2: https://github.com/tj/commander.js/blob/master/Readme.md
- 3: https://registry.npmjs.org/commander
- 4: Proper way to get value of argument tj/commander.js#1959
- 5: Alternative order of arguments supplied to action handler tj/commander.js#1639
- 6: parseOptions rework phase 1 tj/commander.js#1138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -a 'v5-db-sync\.(ts|test\.ts)|package\.json' . | sed 's#^\./##'
echo "== target slice =="
sed -n '540,645p' src/term-commands/v5-db-sync.ts
echo "== test slice around parse async =="
if [ -f src/term-commands/v5-db-sync.test.ts ]; then
sed -n '1,230p' src/term-commands/v5-db-sync.test.ts
fi
echo "== commander declarations/rawArgs references =="
rg -n "rawArgs|parseAsync|parse\\(" -S --glob '*.ts' --glob '!dist/**' --glob '!node_modules/**' src/term-commands/v5-db-sync.ts src/term-commands/v5-db-sync.test.ts package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -200Repository: automagik-dev/genie
Length of output: 13542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rejection logic =="
rg -n -A12 -B6 "rejectRepeatedOptions" src/term-commands/v5-db-sync.ts
echo "== command registration / parse paths =="
rg -n "parseAsync|parse\\(" src/term-commands src -g '*.ts' | sed -n '1,220p'
echo "== v5 import and main registration =="
rg -n "registerV5DatabaseSyncCommand|genie.ts|command\\('db'|parseAsync" src -g '*.ts' | sed -n '1,220p'Repository: automagik-dev/genie
Length of output: 29527
🌐 Web query:
Commander.js 12.1.0 rawArgs parseAsync from user undefined program.rawArgs documentation
💡 Result:
In Commander.js, program.rawArgs provides access to the complete, unparsed array of arguments passed to the application [1][2]. While program.args contains only the arguments not consumed by options, program.rawArgs retains the original input as received by the process [3][1]. Regarding the use of { from: 'user' } with.parse or.parseAsync: When you pass an array of strings and specify { from: 'user' }, you are instructing Commander to treat the provided array as containing only the user-supplied arguments, skipping the default behavior where it expects argv to be the application binary and argv[4] to be the script path [5][6]. For example: program.parse(['--port', '80'], { from: 'user' }); Using.parseAsync is the recommended method when your command or action handlers involve asynchronous operations (e.g., promises) [5][7][6]. If you use.parse instead of.parseAsync with async code, you may encounter issues where the application exits before your asynchronous logic completes [7]. If you find that your options or arguments are "undefined," it is often because: 1. You are using an async action handler without.parseAsync [8][7]. 2. The arguments are being consumed incorrectly or the input is not being passed as expected (e.g., via npm scripts that may swallow certain flags) [8]. If you need to test for the absence of user input, you can use program.rawArgs.length as a reliable check for the number of arguments provided to the process [9][2]. Top results: [5][6][2][7]
Citations:
- 1: Get remaining arguments tj/commander.js#802
- 2: Triage: detecting missing or unknown commands tj/commander.js#1088
- 3: https://registry.npmjs.org/commander
- 4: https://npmx.dev/package-docs/commander/v/12.1.0
- 5: https://github.com/tj/commander.js/blob/master/Readme.md
- 6: https://github.com/visionmedia/commander.js/
- 7: parse() works for async action handler in subcommand tj/commander.js#1681
- 8: options Undefined tj/commander.js#1820
- 9: https://raw.githubusercontent.com/tj/commander.js/0692be58103e1ea8052d5d45ab11cc02e197eea5/CHANGELOG.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package commander version =="
python3 - <<'PY'
import json
from pathlib import Path
paths = [Path('package.json'), Path('plugins/genie/package.json')]
for p in paths:
if p.exists():
data=json.loads(p.read_text())
print(p)
for dep in ('dependencies','devDependencies','peerDependencies'):
if 'commander' in data.get(dep, {}):
print(f' {dep}: commander={data[dep]["commander"]}')
PY
echo "== local commander source rawArgs population if available =="
for dir in node_modules plugins/genie/node_modules; do
for f in "$dir"/commander/lib/command.js "$dir"/commander/lib/command.mjs; do
[ -f "$f" ] && { echo "--- $f"; rg -n -C 4 "rawArgs|from:'user'|from: 'user'" "$f" | sed -n '1,120p'; }
done
done
echo "== relevant local tests for repeated options =="
rg -n -C 5 "repeated|duplicate|--json --json|once|rejectRepeatedOptions" src/term-commands/v5-db-sync.test.ts src/term-commands/v5-db-sync.tsRepository: automagik-dev/genie
Length of output: 9318
Make the rawArgs fallback fail-closed.
rawArguments becomes [] when rawArgs is absent, so rejectRepeatedOptions accepts every duplicate option. A validation guard should reject instead of failing open. Also keep .slice(2) tied to the actual process.argv shape, because { from: 'user' } parsing would shift the window and hide duplicates in the first two arguments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/term-commands/v5-db-sync.ts` around lines 631 - 634, Update the raw
argument handling in the command-root flow before handleSync so missing rawArgs
is rejected rather than replaced with an empty array, and preserve the existing
process.argv-based `.slice(2)` offset instead of deriving it from parsed `{
from: 'user' }` arguments.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/genie-commands/__tests__/update-command-publication.test.ts`:
- Around line 49-57: Update the fixture setup around the mkdirSync and
writeFileSync calls to explicitly normalize permissions after creation: apply
0755 to every created directory and 0644 to every written file before the
archive is generated. Preserve the existing fixture paths and contents while
ensuring restrictive umasks cannot change the archived entry permissions.
In `@src/lib/v5/db-reconciliation.test.ts`:
- Line 1844: Increase the elapsed-time assertion thresholds in both
reconciliation tests around the apply flows: change the 1,000 ms bound near the
first assertion to 10,000 ms and the 500 ms bound near the second assertion to
5,000 ms, while preserving the existing failure-code and phase assertions.
In `@src/lib/v5/db-reconciliation.ts`:
- Around line 2590-2593: In the fallback return object, simplify the `code`
assignment so it directly uses the constant `'unexpected-failure'` instead of
the redundant ternary. Preserve the existing spread of `fallback` and all other
return fields.
In `@src/lib/v5/db-sync-snapshots.test.ts`:
- Around line 324-334: Widen the upper elapsed-time assertions in the affected
snapshot-sync tests, including this test and the similar test around the later
elapsed check, while keeping the existing lower bounds unchanged. Update only
the strict upper limits so the assertions continue validating the shared
deadline without imposing a fragile wall-clock ceiling.
In `@src/lib/v5/reconciliation-tombstone.ts`:
- Around line 66-72: Update parseReconciliationTombstoneMeta so unknown tables
using the v1 reconciliation-tombstone prefix are ignored rather than passed to
invalidTombstone or converted into invalid-data by reconciliationTombstones;
preserve the existing hire_roster parsing and rejection of malformed tombstone
keys.
In `@src/lib/v5/task-state.ts`:
- Around line 1313-1330: Update the hireAgent transaction around the hire_roster
upsert so every re-hire advances the reconciliation version monotonically:
include hired_at in the ON CONFLICT update using the newly computed now value.
Revise the nearby first-hire documentation and affected task-state tests to
reflect the changed contract, while preserving the existing tombstone ordering
logic.
In `@src/term-commands/v5-db-sync.ts`:
- Around line 350-365: Update summarizeBootstrapApply so the bootstrap path
keeps report.cleanupFailures only in the top-level cleanupFailures field and
sets recovery.cleanupFailures to an empty array, preventing cleanupFailureCount
from counting the same failures twice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c0515e2-00d7-4640-8537-ff001e1b7599
⛔ Files ignored due to path filters (2)
CLAUDE.mdis excluded by!*.mdREADME.mdis excluded by!*.md
📒 Files selected for processing (19)
scripts/release-docs.test.tssrc/genie-commands/__tests__/update-command-publication.test.tssrc/genie-commands/__tests__/update.test.tssrc/genie-commands/local-delivery-repair.test.tssrc/genie.tssrc/lib/agent-sync.test.tssrc/lib/interactivity.tssrc/lib/v5/db-reconciliation.test.tssrc/lib/v5/db-reconciliation.tssrc/lib/v5/db-sync-snapshots.test.tssrc/lib/v5/db-sync-snapshots.tssrc/lib/v5/reconciliation-tombstone.tssrc/lib/v5/task-state.test.tssrc/lib/v5/task-state.tssrc/term-commands/ui-bridge.tssrc/term-commands/v5-db-sync.test.tssrc/term-commands/v5-db-sync.tstests/support/codex-dogfood-harness.tstests/support/update-current-boundary-runner.ts
| mkdirSync(join(payload, directory), { recursive: true, mode: 0o755 }); | ||
| } | ||
| writeFileSync(join(payload, '.agents', 'plugin.json'), '{}\n'); | ||
| writeFileSync(join(payload, '.claude-plugin', 'marketplace.json'), '{}\n'); | ||
| writeFileSync(join(payload, 'LICENSE'), 'test fixture\n'); | ||
| writeFileSync(join(payload, 'VERSION'), `${version}\n`); | ||
| writeFileSync(join(payload, 'plugins', 'genie', 'plugin.txt'), 'authenticated plugin payload\n'); | ||
| writeFileSync(join(payload, 'skills', 'review', 'SKILL.md'), '# Review\n'); | ||
| writeFileSync(join(payload, 'templates', 'template.txt'), 'template\n'); | ||
| writeFileSync(join(payload, '.agents', 'plugin.json'), '{}\n', { mode: 0o644 }); | ||
| writeFileSync(join(payload, '.claude-plugin', 'marketplace.json'), '{}\n', { mode: 0o644 }); | ||
| writeFileSync(join(payload, 'LICENSE'), 'test fixture\n', { mode: 0o644 }); | ||
| writeFileSync(join(payload, 'VERSION'), `${version}\n`, { mode: 0o644 }); | ||
| writeFileSync(join(payload, 'plugins', 'genie', 'plugin.txt'), 'authenticated plugin payload\n', { mode: 0o644 }); | ||
| writeFileSync(join(payload, 'skills', 'review', 'SKILL.md'), '# Review\n', { mode: 0o644 }); | ||
| writeFileSync(join(payload, 'templates', 'template.txt'), 'template\n', { mode: 0o644 }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
const { mkdtempSync, mkdirSync, writeFileSync, statSync, rmSync } = require('node:fs');
const { join } = require('node:path');
const { tmpdir } = require('node:os');
const root = mkdtempSync(join(tmpdir(), 'genie-mode-check-'));
const previous = process.umask(0o077);
try {
const directory = join(root, 'directory');
const file = join(directory, 'file');
mkdirSync(directory, { recursive: true, mode: 0o755 });
writeFileSync(file, '', { mode: 0o644 });
console.log({
directory: (statSync(directory).mode & 0o777).toString(8),
file: (statSync(file).mode & 0o777).toString(8),
});
} finally {
process.umask(previous);
rmSync(root, { recursive: true, force: true });
}
NODERepository: automagik-dev/genie
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching test name:"
fd -a 'update-command-publication\.test\.ts$' . || true
file="$(fd 'update-command-publication\.test\.ts$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "---- outline ----"
ast-grep outline "$file" || true
echo "---- relevant lines 1-180 ----"
sed -n '1,180p' "$file" | nl -ba
fi
echo
echo "Search for tarball/archive creation in this file:"
rg -n "tar|archive|createArchive|package|publish|chmod|chmodSync|mkdirSync|writeFileSync|umask" "$file" || trueRepository: automagik-dev/genie
Length of output: 808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="src/genie-commands/__tests__/update-command-publication.test.ts"
echo "---- lines 40-120 ----"
sed -n '40,120p' "$target"
echo
echo "---- lines 120-260 ----"
sed -n '120,260p' "$target"
echo
echo "---- lines 260-420 ----"
sed -n '260,420p' "$target"
echo
echo "---- search archive/tar/chmod/umask references ----"
rg -n "tar|archive|package|publish|chmod|chmodSync|mkdirSync|writeFileSync|umask|sha256|generate|create|Release" "$target" || trueRepository: automagik-dev/genie
Length of output: 7818
Normalize release fixture permissions before archiving.
mkdirSync() and writeFileSync() only apply mode as the per-path umask, so restrictive environments create 0700/ 0600 entries. Since this fixture is archived with tar, set directory permissions after creation and file permissions after writing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/genie-commands/__tests__/update-command-publication.test.ts` around lines
49 - 57, Update the fixture setup around the mkdirSync and writeFileSync calls
to explicitly normalize permissions after creation: apply 0755 to every created
directory and 0644 to every written file before the archive is generated.
Preserve the existing fixture paths and contents while ensuring restrictive
umasks cannot change the archived entry permissions.
| { code: 'close-failed', phase: 'cleanup' }, | ||
| ], | ||
| }); | ||
| expect(Date.now() - startedAt).toBeLessThan(1_000); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wall-clock assertions will flake on a loaded CI runner.
Line 1844 asserts the whole apply finishes within 1000 ms and line 1957 asserts within 500 ms. Both tests already assert the correct failure code and phase, which is the actual contract. The elapsed-time bound only guards against an unbounded wait, so a much larger budget still proves the point without failing when the runner stalls.
Raise both bounds, for example to 10_000 ms and 5_000 ms.
Also applies to: 1957-1957
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/v5/db-reconciliation.test.ts` at line 1844, Increase the elapsed-time
assertion thresholds in both reconciliation tests around the apply flows: change
the 1,000 ms bound near the first assertion to 10,000 ms and the 500 ms bound
near the second assertion to 5,000 ms, while preserving the existing
failure-code and phase assertions.
| return { | ||
| ...fallback, | ||
| code: fallback.code === 'unexpected-failure' ? fallback.code : 'unexpected-failure', | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Collapse the always-true ternary.
fallback.code === 'unexpected-failure' ? fallback.code : 'unexpected-failure' evaluates to 'unexpected-failure' in both branches. The expression suggests the fallback code is sometimes preserved, but it never is. Simplify it so the intent is explicit.
♻️ Proposed simplification
- return {
- ...fallback,
- code: fallback.code === 'unexpected-failure' ? fallback.code : 'unexpected-failure',
- };
+ // Untyped throws are never attributed to a specific reconciliation code.
+ return { ...fallback, code: 'unexpected-failure' };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| ...fallback, | |
| code: fallback.code === 'unexpected-failure' ? fallback.code : 'unexpected-failure', | |
| }; | |
| // Untyped throws are never attributed to a specific reconciliation code. | |
| return { ...fallback, code: 'unexpected-failure' }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/v5/db-reconciliation.ts` around lines 2590 - 2593, In the fallback
return object, simplify the `code` assignment so it directly uses the constant
`'unexpected-failure'` instead of the redundant ternary. Preserve the existing
spread of `fallback` and all other return fields.
| const elapsed = Date.now() - startedAt; | ||
|
|
||
| expect(report).toMatchObject({ | ||
| status: 'operational-failure', | ||
| failure: 'locked-operation-failed', | ||
| cleanupFailures: [], | ||
| }); | ||
| expect(existsSync(target)).toBe(false); | ||
| expect(openedPaths).toEqual([reconciliationAdvisoryLockPath(source), reconciliationAdvisoryLockPath(target)]); | ||
| expect(elapsed).toBeGreaterThanOrEqual(60); | ||
| expect(elapsed).toBeLessThan(300); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Tight wall-clock upper bounds can flake on loaded CI.
elapsed < 300 allows only 100 ms of slack over the 200 ms injected advisory wait. The test at Line 417 has a similar bound. A loaded runner or a GC pause can exceed it and fail a correct implementation. The lower bounds prove the deadline sharing; consider widening only the upper bounds.
♻️ Proposed change
expect(elapsed).toBeGreaterThanOrEqual(60);
- expect(elapsed).toBeLessThan(300);
+ expect(elapsed).toBeLessThan(1_500);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const elapsed = Date.now() - startedAt; | |
| expect(report).toMatchObject({ | |
| status: 'operational-failure', | |
| failure: 'locked-operation-failed', | |
| cleanupFailures: [], | |
| }); | |
| expect(existsSync(target)).toBe(false); | |
| expect(openedPaths).toEqual([reconciliationAdvisoryLockPath(source), reconciliationAdvisoryLockPath(target)]); | |
| expect(elapsed).toBeGreaterThanOrEqual(60); | |
| expect(elapsed).toBeLessThan(300); | |
| const elapsed = Date.now() - startedAt; | |
| expect(report).toMatchObject({ | |
| status: 'operational-failure', | |
| failure: 'locked-operation-failed', | |
| cleanupFailures: [], | |
| }); | |
| expect(existsSync(target)).toBe(false); | |
| expect(openedPaths).toEqual([reconciliationAdvisoryLockPath(source), reconciliationAdvisoryLockPath(target)]); | |
| expect(elapsed).toBeGreaterThanOrEqual(60); | |
| expect(elapsed).toBeLessThan(1_500); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/v5/db-sync-snapshots.test.ts` around lines 324 - 334, Widen the upper
elapsed-time assertions in the affected snapshot-sync tests, including this test
and the similar test around the later elapsed check, while keeping the existing
lower bounds unchanged. Update only the strict upper limits so the assertions
continue validating the shared deadline without imposing a fragile wall-clock
ceiling.
| const suffix = key.slice(RECONCILIATION_TOMBSTONE_PREFIX.length); | ||
| const separator = suffix.indexOf(':'); | ||
| if (separator < 1) invalidTombstone(); | ||
| const table = suffix.slice(0, separator); | ||
| const [first, second] = decodedParts(suffix.slice(separator + 1)); | ||
| if (table === 'hire_roster') return { table, wish: first, agentAdapterId: second, deletedAt }; | ||
| return invalidTombstone(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any other table is ever encoded under the v1 tombstone prefix,
# and whether docs promise forward compatibility for unknown tombstone tables.
rg -n -C3 'RECONCILIATION_TOMBSTONE_PREFIX|reconciliationTombstoneMeta|reconciliation-tombstone' --type=ts
rg -n -i -C3 'tombstone' --glob '*.md'Repository: automagik-dev/genie
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
echo "## locate files"
fd -a 'reconciliation-tombstone|db-reconciliation' . || true
echo "## search symbols excluding binary"
rg -n -C3 'RECONCILIATION_TOMBSTONE_PREFIX|reconciliationTombstoneMeta|reconciliationTombstones|decodedParts|invalidTombstone|invalid-data' . --glob '*.ts' --glob '!node_modules/**' || true
echo "## git status/stat"
git status --short --short 2>/dev/null || true
git diff --stat 2>/dev/null || trueRepository: automagik-dev/genie
Length of output: 24729
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
from urllib.parse import quote
# Minimal read-only replication of the relevant prefix/suffix behavior.
prefix = 'genie:reconciliation-tombstone:v1:'
def encoded_parts(parts):
import base64, json
return base64.urlsafe_b64encode(json.dumps(parts).encode()).rstrip(b'=').decode()
def make(key_suffix):
return f"{prefix}{encoded_parts(key_suffix.split(':', 1)[0])}:{encoded_parts(key_suffix.split(':', 1)[0])}"
samples = ['hire_roster:known:table', 'unknown_table:fresh:fresh']
for sample in samples:
print(f"#{sample}")
for value in [0, 'deleted', 'not-canonical-base64']:
key = make(sample)
print(f" value={value}; starts_with={key.startswith(prefix)}")
suffix = key[len(prefix):]
sep = suffix.index(':')
table = suffix[:sep]
encoded_after = suffix[sep + 1:]
print(f" table={table}; unknown_table_rejects={table!r != 'hire_roster'}")
PYRepository: automagik-dev/genie
Length of output: 380
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
from urllib.parse import quote
# Minimal read-only replication of the relevant prefix/suffix behavior.
prefix = 'genie:reconciliation-tombstone:v1:'
def encoded_parts(parts):
import base64, json
return base64.urlsafe_b64encode(json.dumps(parts).encode()).rstrip(b'=').decode()
def make(key_suffix):
return f"{prefix}{encoded_parts(key_suffix.split(':', 1)[0])}:{encoded_parts(key_suffix.split(':', 1)[0])}"
samples = ['hire_roster:known:table', 'unknown_table:fresh:fresh']
for sample in samples:
print(f"#{sample}")
for value in [0, 'deleted', 'not-canonical-base64']:
key = make(sample)
print(f" value={value}; starts_with={key.startswith(prefix)}")
suffix = key[len(prefix):]
sep = suffix.index(':')
table = suffix[:sep]
encoded_after = suffix[sep + 1:]
unknown_rejects = table != 'hire_roster'
print(f" table={table}; unknown_table_rejects={unknown_rejects}")
PYRepository: automagik-dev/genie
Length of output: 763
Do not reject unknown v1 tombstone tables with invalid-data.
parseReconciliationTombstoneMeta returns null only for keys outside RECONCILIATION_TOMBSTONE_PREFIX; a future genie:reconciliation-tombstone:v1: key for a new table throws Invalid reconciliation tombstone metadata, which reconciliationTombstones converts to invalid-data for the whole input. Bump the prefix to v2 for a new tombstone table, or treat unknown v1 tables as ignored instead of poisoning reconciliation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/v5/reconciliation-tombstone.ts` around lines 66 - 72, Update
parseReconciliationTombstoneMeta so unknown tables using the v1
reconciliation-tombstone prefix are ignored rather than passed to
invalidTombstone or converted into invalid-data by reconciliationTombstones;
preserve the existing hire_roster parsing and rejection of malformed tombstone
keys.
| db.transaction(() => { | ||
| const priorMarker = db.query('SELECT value FROM meta WHERE key = ?').get(tombstoneKey) as { value: string } | null; | ||
| const priorDeletion = | ||
| priorMarker === null ? null : parseReconciliationTombstoneMeta(tombstoneKey, priorMarker.value); | ||
| const now = Number( | ||
| priorDeletion === null ? BigInt(Date.now()) : maxBigInt(BigInt(Date.now()), priorDeletion.deletedAt + 1n), | ||
| ); | ||
| if (!Number.isSafeInteger(now)) throw new Error('Roster reconciliation version exceeds the safe timestamp range.'); | ||
| db.query('DELETE FROM meta WHERE key = ?').run(tombstoneKey); | ||
| db.query( | ||
| `INSERT INTO hire_roster (wish, agent_adapter_id, profile, worktree, hired_at, state) | ||
| VALUES (?, ?, ?, ?, ?, ?) | ||
| ON CONFLICT(wish, agent_adapter_id) DO UPDATE SET | ||
| profile = excluded.profile, | ||
| worktree = excluded.worktree, | ||
| state = excluded.state`, | ||
| ).run(input.wish, input.agentAdapterId, input.profile ?? null, input.worktree, now, state); | ||
| }).immediate(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
A re-hire that happens before a remote unhire is synced is silently discarded.
hired_at is the reconciliation version for hire_roster rows. db-reconciliation.ts compares live.hiredAt against tombstone.deletedAt and deletes the row when hiredAt <= deletedAt. The ON CONFLICT DO UPDATE SET list here deliberately omits hired_at, so a re-hire on a row that still exists keeps the original timestamp.
Failure sequence:
- Host and guest both hold the row with
hired_at = 1. - Guest runs
unhireAgentand writes a tombstone withdeletedAt = T, whereT > 1. - Before any sync, the host runs
hireAgentwith newworktreeorstate. The host has no local tombstone, so theON CONFLICTbranch runs andhired_atstays1. - Sync copies the guest tombstone to the host.
1 <= T, so the host row is deleted.
The user's re-hire is lost with no conflict reported. The existing test at src/lib/v5/db-reconciliation.test.ts lines 1054-1067 only covers a re-hire that happens after the tombstone was already applied locally, so it does not catch this ordering.
Advance hired_at monotonically on every re-hire, or introduce a separate roster version column so hired_at can keep its "first hire" meaning.
🐛 Proposed fix: make the reconciliation version advance on re-hire
db.transaction(() => {
const priorMarker = db.query('SELECT value FROM meta WHERE key = ?').get(tombstoneKey) as { value: string } | null;
const priorDeletion =
priorMarker === null ? null : parseReconciliationTombstoneMeta(tombstoneKey, priorMarker.value);
- const now = Number(
- priorDeletion === null ? BigInt(Date.now()) : maxBigInt(BigInt(Date.now()), priorDeletion.deletedAt + 1n),
- );
+ const live = db
+ .query('SELECT hired_at FROM hire_roster WHERE wish = ? AND agent_adapter_id = ?')
+ .get(input.wish, input.agentAdapterId) as { hired_at: number } | null;
+ let version = BigInt(Date.now());
+ if (priorDeletion !== null) version = maxBigInt(version, priorDeletion.deletedAt + 1n);
+ if (live !== null) version = maxBigInt(version, BigInt(live.hired_at) + 1n);
+ const now = Number(version);
if (!Number.isSafeInteger(now)) throw new Error('Roster reconciliation version exceeds the safe timestamp range.');
db.query('DELETE FROM meta WHERE key = ?').run(tombstoneKey);
db.query(
`INSERT INTO hire_roster (wish, agent_adapter_id, profile, worktree, hired_at, state)
VALUES (?, ?, ?, ?, ?, ?)
ON CONFLICT(wish, agent_adapter_id) DO UPDATE SET
profile = excluded.profile,
worktree = excluded.worktree,
+ hired_at = excluded.hired_at,
state = excluded.state`,
).run(input.wish, input.agentAdapterId, input.profile ?? null, input.worktree, now, state);
}).immediate();Note that this changes the documented "first hire timestamp survives every re-hire" contract in the doc comment at lines 1297-1304, and src/lib/v5/task-state.test.ts asserts that behaviour. If the original timestamp must be preserved, add a separate version column instead and compare that column against deletedAt in applyTombstonesToState.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/v5/task-state.ts` around lines 1313 - 1330, Update the hireAgent
transaction around the hire_roster upsert so every re-hire advances the
reconciliation version monotonically: include hired_at in the ON CONFLICT update
using the newly computed now value. Revise the nearby first-hire documentation
and affected task-state tests to reflect the changed contract, while preserving
the existing tombstone ordering logic.
| function summarizeBootstrapApply(report: MissingDatabaseBootstrapApplyReport): ApplyReport { | ||
| return { | ||
| status: report.status, | ||
| generationId: null, | ||
| recovery: { | ||
| status: report.status === 'changed' ? 'none' : 'operational-failure', | ||
| generationId: null, | ||
| restoredDatabaseIdentities: [], | ||
| failure: report.failure, | ||
| cleanupFailures: report.cleanupFailures, | ||
| }, | ||
| apply: null, | ||
| failure: report.failure, | ||
| cleanupFailures: report.cleanupFailures, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bootstrap cleanup failures are counted twice in the human summary.
summarizeBootstrapApply copies report.cleanupFailures into both cleanupFailures and recovery.cleanupFailures. cleanupFailureCount at Line 466 adds both arrays, so genie db sync prints Cleanup failures: 2 for a single bootstrap cleanup failure. The exit code is unaffected because the branch tests > 0, but the printed count misleads an operator sizing the leak.
Leave the recovery array empty for the bootstrap path, since the bootstrap report has no separate recovery phase.
🐛 Proposed fix
recovery: {
status: report.status === 'changed' ? 'none' : 'operational-failure',
generationId: null,
restoredDatabaseIdentities: [],
failure: report.failure,
- cleanupFailures: report.cleanupFailures,
+ cleanupFailures: [],
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function summarizeBootstrapApply(report: MissingDatabaseBootstrapApplyReport): ApplyReport { | |
| return { | |
| status: report.status, | |
| generationId: null, | |
| recovery: { | |
| status: report.status === 'changed' ? 'none' : 'operational-failure', | |
| generationId: null, | |
| restoredDatabaseIdentities: [], | |
| failure: report.failure, | |
| cleanupFailures: report.cleanupFailures, | |
| }, | |
| apply: null, | |
| failure: report.failure, | |
| cleanupFailures: report.cleanupFailures, | |
| }; | |
| } | |
| function summarizeBootstrapApply(report: MissingDatabaseBootstrapApplyReport): ApplyReport { | |
| return { | |
| status: report.status, | |
| generationId: null, | |
| recovery: { | |
| status: report.status === 'changed' ? 'none' : 'operational-failure', | |
| generationId: null, | |
| restoredDatabaseIdentities: [], | |
| failure: report.failure, | |
| cleanupFailures: [], | |
| }, | |
| apply: null, | |
| failure: report.failure, | |
| cleanupFailures: report.cleanupFailures, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/term-commands/v5-db-sync.ts` around lines 350 - 365, Update
summarizeBootstrapApply so the bootstrap path keeps report.cleanupFailures only
in the top-level cleanupFailures field and sets recovery.cleanupFailures to an
empty array, preventing cleanupFailureCount from counting the same failures
twice.
Capture the roster row in the upsert with RETURNING, preserving defaults and original hired_at while eliminating the post-write read race. Exercise two processes hiring and removing the same row against one real SQLite database. Reauthored from the hireAgent race identified in Liraz Siri's PR #2737 review. Validated with the full gate (2031 pass, 1 skip, 0 fail) and isolated API plus built CLI backup/import dogfood. Co-authored-by: Felipe Rosa <felipe@namastex.ai>
Port the surviving fixture creation modes from Liraz Siri's PR #2737, commit 9711b15. Keep retired agent-sync and obsolete plugin payload hunks absent. Process umask still applies. Independent review SHIP; surviving suites pass under default and 027 umasks with 12 tests and 73 assertions each. B2 selection determinism is already fixed on main by 01598ec. Co-authored-by: Felipe Rosa <felipe@namastex.ai>
2 participants
Summary
genie db syncplanning and apply flows for bidirectional and directional reconciliationWhy
Genie repositories can acquire independent host and guest
genie.dbstate. Previously there was no supported way to inspect, reconcile, recover, or bootstrap those databases without replacing files manually.This adds an explicit zero-daemon synchronization command. Bidirectional mode merges non-conflicting state and refuses mutable-row ambiguity. Directional mode makes the source authoritative for shared mutable rows while preserving destination-only rows.
Safety and behavior
Validation
bun run checkautomagik-dev/genie:main0775.geniedirectoryno-opNotes
The two existing
doctor.tscognitive-complexity warnings remain within the repository's explicit complexity budget and are unrelated to this change.Summary by CodeRabbit
genie db syncfor bidirectional or directional database reconciliation.