Skip to content

feat(codex, babysit): review runs take a base and a tier contract, and tools codex reviews lists and imports the review ledger - #592

Merged
genesiscz merged 10 commits into
masterfrom
fix/2026-10-25-codex-review-tiers
Oct 26, 2026
Merged

genesiscz merged 10 commits into
masterfrom
fix/2026-10-25-codex-review-tiers

Conversation

@genesiscz

Copy link
Copy Markdown
Owner

Codex review runs get a base and a tier contract: spawn --mode review --base injects the diff manifest and picks high or xhigh from the diff size; tools codex reviews lists every run from the review ledger (and import backfills it); the review gate keeps paths with underscores and spaces; a ledger append after a torn line starts on its own line. The babysit and handoff-to skills document it.

  • 5202050 docs(skills, babysit, handoff-to): handoff-to and babysit set a Codex review's effort from the diff size (high, xhigh above about 50 files), because medium reviews were measured shallow
  • b9d6d7f feat(worker): the shared worker contract carries a per-tier review block and the diff manifest, so a reviewer on any backend gets the same depth rules and output shape
  • 1fe09ca feat(codex): spawn --mode review takes a base, injects the diff manifest and the tier contract, picks the tier from the diff size, and gates the depth after each turn (one steer, one rerun a tier higher, then shallow), logging every turn and decision to the review ledger
  • ddd861a feat(codex): tools codex reviews lists every review run from the ledger with filters and totals, and review-depth shows one review's coverage, reasoning per 100 lines and gate history
  • 1940c58 docs(skills, babysit, handoff-to): reviews spawn with --mode review --base and leave depth to its contract and gate, and both skills name tools codex reviews; plugin 1.0.116
  • d12cc7b feat(codex): tools codex reviews import backfills the review ledger from sessions that ran before it, with the verdict their coverage line implies and a manifest up to the commit the review saw
  • 0d92732 fix(codex): reviews import logs each import to the day log only, since its table already prints the result
  • d2c7c96 fix(codex, babysit): Enhancements 2026-10-13 (unified) #507 followup - the review gate keeps underscores and spaces in paths and fails a named unread file, records each check for review-depth, which reads only the current run, an import fails loudly and redoes a partial one, and the review spawn line quotes every value
  • 35d67b9 fix(codex): Enhancements 2026-10-13 (unified) #507 followup - a review ledger append after a torn last line starts on its own line, so a redone import never folds the old batch's turns into its row

Gates on this branch (clean worktree, 2026-10-25): bun scripts/git/check-commits.ts, root tsc --noEmit, biome on the changed files, the related tests, and scripts/ci/placeholder-check.sh. Tests: 7 files, 108 pass.

Split out of the unified branch (#507); each commit is a cherry-pick of its unified original. Two text conflicts with master's 7e40b01 were merged by hand: babysit SKILL.md 7a keeps master's policy (a review when the user asks, no minimum round count) and adds the spawn mechanics; the review-brief.ts header keeps master's first sentence and adds the printed spawn line.

@eve-bot-lovinka

eve-bot-lovinka Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

🐉 eve review — ⏭️ superseded

→ run

  • Queued 01:08:47Z
  • Reading diff — 10 files
  • Building repo map
  • Analyzing (find → verify)
  • Posting review
  • Done 01:20:55Z (12m 8s)

superseded — head moved to 2b6bdbb during prep; the queued review covers it

run wrun_01M5TW1K9CZPRE0B33KN8JWWHT

Previous runs (1)
run head outcome findings took
1 35d67b9 ✅ APPROVE 4 16m 29s

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 3 included reviews currently available. Your 43 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 326b858 and 2b6bdbb.

Files selected for processing: 28
  • .claude-plugin/marketplace.json
  • .claude/skills/babysit/SKILL.md
  • .claude/skills/babysit/scripts/review-brief.ts
  • .claude/skills/babysit/scripts/scripts.test.ts
  • plugins/genesis-tools/.claude-plugin/plugin.json
  • plugins/genesis-tools/skills/handoff-to/SKILL.md
  • plugins/genesis-tools/skills/handoff-to/references/codex.md
  • src/codex/README.md
  • src/codex/commands/reviews.ts
  • src/codex/daemon.ts
  • src/codex/index.ts
  • src/codex/lib/driver.ts
  • src/codex/lib/review/controller.ts
  • src/codex/lib/review/escalate.ts
  • src/codex/lib/review/gate.ts
  • src/codex/lib/review/import.ts
  • src/codex/lib/review/ledger.ts
  • src/codex/lib/review/prepare.ts
  • src/codex/lib/review/report.ts
  • src/codex/lib/review/review.test.ts
  • src/codex/lib/review/types.ts
  • src/codex/lib/seed-instructions.ts
  • src/codex/lib/session.test.ts
  • src/codex/lib/session.ts
  • src/codex/lib/spawn.ts
  • src/codex/lib/store.ts
  • src/utils/worker/contract.test.ts
  • src/utils/worker/contract.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@eve-bot-lovinka eve-bot-lovinka Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐉 eve review — 🟡 Review comments

35d67b9 · 4 actionable findings · reviewed by find codex/gpt-6.1-sol + codex/gpt-6-luna, verify codex/gpt-6-luna · view run ↗

Severity Count
🟡 Medium 4

ℹ️ Checks (advisory — CI is authoritative) — typecheck timed out (tsconfig.json): out of memory, then its 6144 MiB retry ran out of time

Blast radius

  • codexDriver (~) → 7 dependent(s) outside this PR: src/codex/lib/message-delivery.ts, src/codex/commands/queue.ts, src/codex/lib/spec.ts
  • buildWorkerContract (~) → 4 dependent(s) outside this PR: src/grok/lib/worker.ts, src/claude/lib/worker/worker.ts
  • 78 compiler diagnostic(s) on the PR head touch this diff (graph truncated)

ℹ️ Claims vs diff — 5 claims could not be checked from the diff (c1, c5, c6, c7, c8).

Comment thread src/codex/lib/review/import.ts Outdated
Comment thread .claude/skills/babysit/scripts/review-brief.ts
Comment thread src/codex/lib/review/ledger.ts Outdated
Comment thread src/codex/lib/review/prepare.ts Outdated
genesiscz added a commit that referenced this pull request Oct 26, 2026
…an import recovers the historical base and head, and live ledger writes survive a torn line
@genesiscz

Copy link
Copy Markdown
Owner Author

Review fixes 2026-10-26 02:08 from GPT-6.1 Sol judged by Claude Opus 5.5 (fable-judge)

Round 1. 4 open threads (eve 4, CodeRabbit 0). All 4 findings are valid and have uncommitted fixes, along with 1 self-review finding. No findings were rejected or deferred. Fixed in 86c8f7f.

Recover the historical base and head before importing coverage (eve-bot-lovinka)

  • Context: The old importer bounded only the reviewed head by commit date and compared it with today's target branch. Once that target contained the reviewed head, the reconstructed manifest became empty. Commit dates also do not establish when a branch ref moved.
  • Commit(s): 86c8f7ffd
  • Verdict: accepted
  • Code before:
    const seen = await git.commitBefore(cwd, meta.startedAt);
    const head = seen ?? "HEAD";
    const manifest = buildManifest(chosen.ref, await git.numstat(cwd, chosen.ref, undefined, head));
  • Code after:
    const head = await git.commitBefore(cwd, meta.startedAt);
    const historicalBase = await git.commitBefore(cwd, meta.startedAt, chosen.ref);
    
    if (!head || !historicalBase) {
        return { ...none(`historical head or base before ${meta.startedAt} cannot be established; file and line counts are null`), chosen, branch };
    }
  • How was this fixed: New sessions restore their saved pinned manifest, including when the original cwd is gone. Legacy imports recover both refs at the session start from reflogs, resolve their merge-base, and diff to the historical head. Missing historical refs leave counts null and cannot establish a passing full review. The shared reader resolves short branch names to fully qualified refs before checking reflog existence. Existing review.test.ts now covers a target that already contains the reviewed head, unavailable history, and saved manifests after cwd removal. README.md documents the changed reconstruction and null-count behavior. Verification: not run (the lead verifies at integration). The real-reflog regression failed in an intermediate run before the fully qualified ref correction; the final correction has not been rerun.
  • Confidence: 85%

Record explicit PR identity for detached reviews (eve-bot-lovinka)

  • Context: Post-merge review worktrees are detached, so branch discovery cannot attach a PR number. A number in the session name or label does not satisfy the reviews command's PR filter.
  • Commit(s): 86c8f7ffd
  • Verdict: accepted
  • Code before:
    ...["tools", "codex", "spawn", "--mode", "review", ...target],
  • Code after:
    ...["tools", "codex", "spawn", "--mode", "review", "--pr", String(pr), ...target],
  • How was this fixed: The spawn CLI accepts --pr and forwards it into review preparation. Preparation validates a positive safe integer and records the explicit PR ahead of inferred identity. Babysit's spawn command passes its PR number for both open and post-merge reviews. Task mode rejects the review-only flag. Existing tests cover detached preparation through ledger summarization and PR filtering, invalid identities, task-mode refusal, and the generated shell argv. README.md documents the explicit flag. Verification: not run (the lead verifies at integration).
  • Confidence: 94%

Use torn-line recovery for live ledger writes (eve-bot-lovinka)

  • Context: Live start, turn and gate records used JsonlWriter directly, bypassing the newline boundary recovery used by imports. An unterminated fragment could swallow the next start record and mix runs that reused a session name.
  • Commit(s): 86c8f7ffd
  • Verdict: accepted
  • Code before:
    new JsonlWriter(path).append({ ...record });
    return record;
  • Code after:
    return appendReviewRecords([record], path).ok ? record : null;
  • How was this fixed: Live appends delegate to the batch append implementation, which inserts a newline after a torn fragment and returns a failure result without throwing. The single-record adapter preserves its record-or-null contract. Existing review.test.ts now exercises live starts, turns and gates after torn fragments, a reused session name, and an unwritable destination. Verification: not run (the lead verifies at integration).
  • Confidence: 95%

Pin the range used by manifests and review contracts (eve-bot-lovinka)

  • Context: The saved manifest counted the range at preparation time, but the worker instructions used a mutable base ref and HEAD. A later commit or fetch could change the reviewed range while the gate and escalation retained the old counts.
  • Commit(s): 86c8f7ffd
  • Verdict: accepted
  • Code before:
    const manifest = buildManifest(base.ref, await readers.numstat(input.cwd, base.ref, input.paths));
  • Code after:
    const head = await readers.resolveCommit(input.cwd, "HEAD");
    const baseCommit = await readers.resolveCommit(input.cwd, base.ref);
    const mergeBase = await readers.mergeBase(input.cwd, baseCommit, head);
    const manifest = {
        ...buildManifest(mergeBase, await readers.numstat(input.cwd, mergeBase, input.paths, head)),
        head,
    };
  • How was this fixed: Preparation resolves the head and base commits, computes their merge-base, and counts that exact range. The saved manifest carries the pinned merge-base and head. Shared worker instructions use those endpoints with converter off switches and tell the reviewer to read file versions from the saved head. Escalation already retains the manifest, so it retains the same pinned range. Ledger records also retain both endpoints. Existing review and worker-contract tests cover moved refs, escalation, ledger endpoints and exact injected commands. README.md explains the pinned behavior. The optional manifest head keeps older persisted records readable. Verification: not run (the lead verifies at integration).
  • Confidence: 94%

Apply the live gate's binary exemption before declaring imported coverage shallow (self-review, no thread)

  • Context: The live coverage gate accepts a report that reviews all text files and names unread binary files with a reason. Import checked the raw reviewed count first and declared that same report shallow before consulting the shared coverage gate.
  • Commit(s): 86c8f7ffd
  • Verdict: accepted
  • Code before:
    if (coverage.reviewed < coverage.total) {
        return { verdict: "shallow", reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}, ${coverage.total - coverage.reviewed} unread${mismatch}` };
    }
  • Code after:
    const check = manifest ? checkCoverage(parsed, manifest, report) : null;
    
    if (check?.ok) {
        return { verdict: "pass", reason: `imported: Files reviewed ${coverage.reviewed} of ${coverage.total}` };
    }
  • How was this fixed: The importer evaluates the shared coverage check against the confirmed historical manifest before classifying a lower raw count as shallow. A named binary with a reason is therefore excused, while an unread text file remains shallow and unavailable scope cannot pass. The existing import test file adds both the binary case and an unread-text control. README.md documents the matching exemption. Verification: not run (the lead verifies at integration).
  • Confidence: 92%

Threads: t1 accepted · t2 accepted · t3 accepted · t4 accepted

… review's effort from the diff size (high, xhigh above about 50 files), because medium reviews were measured shallow
…ock and the diff manifest, so a reviewer on any backend gets the same depth rules and output shape
…est and the tier contract, picks the tier from the diff size, and gates the depth after each turn (one steer, one rerun a tier higher, then shallow), logging every turn and decision to the review ledger
…er with filters and totals, and review-depth shows one review's coverage, reasoning per 100 lines and gate history
…-base and leave depth to its contract and gate, and both skills name tools codex reviews; plugin 1.0.116
…rom sessions that ran before it, with the verdict their coverage line implies and a manifest up to the commit the review saw
…s and spaces in paths and fails a named unread file, records each check for review-depth, which reads only the current run, an import fails loudly and redoes a partial one, and the review spawn line quotes every value
…line starts on its own line, so a redone import never folds the old batch's turns into its row
…an import recovers the historical base and head, and live ledger writes survive a torn line
@genesiscz
genesiscz force-pushed the fix/2026-10-25-codex-review-tiers branch from 86c8f7f to 2b6bdbb Compare October 26, 2026 01:15
@genesiscz
genesiscz merged commit 2b6bdbb into master Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant