fix(aicr-cross-review skill): consumer-less demotion; resumable Codex lane

[yuanchen8911]

Summary

Three fixes to the aicr-cross-review skill, all from failure modes observed on real runs:

• Demote consumer-less integration findings, not the run. An integration-lane finding without a consumerPath/consumerLine pair is now demoted to an ordinary finding (consumer fields stripped, flagged, no integration severity escalation) instead of dropped — so a legitimately consumer-less observation can no longer make the whole review return incomplete.
• Prompt-free lane operations. The CodeRabbit lane deletes its temp worktrees with git worktree remove + find -xdev -depth -delete instead of rm -rf, and sandbox bypass for the coderabbit review call is now probe-gated — machines whose sandbox allowlist covers ~/.coderabbit run it fully sandboxed with no bypass approval prompt.
• Resumable Codex lane on exhausted wait budget. The continuation-wait budget rises from three bounded waits (~27 min) to five (~45 min); a Codex lane that still outlasts it returns the live job id as a structured jobId field, the workflow surfaces it top-level as codexJobId on incomplete results, and a new codexResumeJobId arg resumes the run against the existing job with no re-dispatch.

Motivation / Context

Demotion: observed on PR #2097 — the integration lane returned exactly one finding, a real, evidenced observation that legitimately has no consumer. The consumer-coordinate filter dropped it, the zero-survivor guard saw a required lane contribute nothing, and the run stopped incomplete with all four lanes ok, ~550k subagent tokens spent, and no report produced. A consumer-less finding cannot be verified as an integration claim, but if it locates a defect (own path/line) with evidence it is a perfectly reviewable ordinary finding. Demotion keeps the lane's contribution and lets consensus run.

Prompt-free ops: observed on a live run of this branch — managed (admin-deployed) permission policies commonly put Bash(rm:*) in the ask list, which matches every sub-command of a compound call and overrides any allow rule, so each lane cleanup blocked on a manual confirmation. Separately, the unconditional sandbox bypass on the coderabbit review call costs a per-run approval prompt even on machines where ~/.coderabbit is sandbox-writable.

Resumable Codex lane: observed reviewing PR #2097 — a real Codex review job on a +1951/−153 diff ran ~53 minutes, past the three-wait budget (#2101) that was sized above the then-measured ~19-minute maximum. The lane correctly returned unavailable with the job id only in free text, the required-lane rule reported the whole run incomplete, and recovery had to be improvised: poll the job by hand, then resume with a hand-patched script telling the Codex lane to fetch the existing job. The recovery worked — cached lanes replayed, the finished job was fetched, zero work re-dispatched — so this change codifies it as a first-class path instead of an improvisation.

Fixes: N/A
Related: #2101 (previous skill fix: Codex wait budget)

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Refactoring (no functional changes)
• Build/CI/tooling

Component(s) Affected

• CLI (cmd/aicr, pkg/cli)
• API server (cmd/aicrd, pkg/server)
• Recipe engine / data (pkg/recipe)
• Bundlers (pkg/bundler, pkg/component/*)
• Collectors / snapshotter (pkg/collector, pkg/snapshotter)
• Validator (pkg/validator)
• Core libraries (pkg/errors, pkg/k8s)
• Docs/examples (docs/, examples/)
• Other: reviewer tooling (.agents/skills/aicr-cross-review/)

Implementation Notes

Demotion (workflow.mjs intake):

• Consumer-less integration findings are demoted, never silently: the demotion is log()ed with locations, and each demoted candidate carries a demoted from integration claim (no consumer pair) flag that reaches the cross-review candidate list and refuter prompts.
• The integration severity escalation (confirmed minor → medium) now explicitly requires a verified consumer pair (hasCoords(c.consumerPath, c.consumerLine)), so demoted findings cannot inherit the escalation reserved for proven broken consumers.
• The fail-closed zero-survivor guard is unchanged in spirit: demoted findings pass the same coordinate/evidence intake gates as every candidate, and the run still stops when a non-empty integration result yields no accepted finding at all — the message now states that demotion already absorbed the consumer-less case, so a stop means the findings lacked even a locatable defect or evidence.

Prompt-free lane operations (CodeRabbit protocol):

• STEP 3 cleanup and STEP 1's stale-reaper delete with find … -xdev -depth -delete instead of rm -rf; the skill's Phase 1/Phase 5 diff-file cleanup follows suit. find -xdev -depth -delete removes exactly the named tree with the same bounds — -xdev keeps it on the filesystem it was pointed at — without matching an rm ask rule. The rationale is documented at both sites so it doesn't get "simplified" back.
• STEP 1 now probes whether ~/.coderabbit is sandbox-writable (an if-wrapped write, safe under set -e) and echoes CR_SANDBOXED. STEP 2 runs sandboxed when the probe passed — no bypass, no approval prompt — and with dangerouslyDisableSandbox from the start otherwise. The probe replaces guessing: a sandboxed run on a denied machine does not fail fast, it hangs for the full ten-minute timebox, so try-sandboxed-first without a probe would cost a whole timebox per unprepared machine. The lane stays portable: contributors without the allowlist entry get exactly the previous behavior.

Resumable Codex lane (workflow.mjs dispatch protocol + args):

• The continuation wait is granted four times (five bounded waits total, ~45 min); a fifth .waitTimedOut is exhausted budget. Wait mechanics are unchanged: 540000 ms inner wait under a 600000 ms Bash cap, longer budgets always mean more calls, never longer ones.
• The lane result schema gains an optional jobId string. The dispatch protocol requires the lane to set it whenever a dispatched job may still be live — a job that outlasts all five waits, and a wait call killed by the outer Bash timeout — since prose alone is not machine-recoverable; the job's last observed state goes in statusNote.
• incomplete results surface a carried jobId top-level as codexJobId (alongside reviewerStatus), so an orchestrator can resume mechanically without parsing free text.
• A new optional codexResumeJobId arg prepends a resume note to the Codex lane's prompt: skip dispatch, status the given job id, result it if terminal, otherwise wait on it under the normal continuation protocol; all other rules apply unchanged. Only the Codex prompt varies with the arg — the claude/coderabbit/integration prompts stay byte-identical so resume caching replays their round-1 results. SKILL.md documents the full recovery loop (poll codexJobId until terminal, then Workflow({..., resumeFromRunId, args: {...prevArgs, codexResumeJobId}})). The arg is validated at intake against a plain job-id pattern and fails closed on shell metacharacters, since the lane interpolates it into a status command.

SKILL.md's protocol rationale (finding-level exception paragraphs, escalation rule, sandbox-exception rules, cleanup commands, wait-budget arithmetic, recovery notes) is updated in lockstep, as the skill requires.

Protocol-compliance hardening (from a live review-loop run where lane agents deviated from the written protocol):

• The CodeRabbit lane's STEP blocks now carry an execute-verbatim instruction — same commands and paths, no substitutions — since an agent-invented rm -rf cleanup once blocked a run for hours on a managed-policy confirmation prompt; SKILL.md mirrors the rule.
• The Codex continuation-wait budget is marked non-discretionary: a live job gets all five waits before the lane may return unavailable (a lane once quit after two waits and the abandoned job completed soon after).
• Phase 1 gains a script-version sentinel: grep -c codexResumeJobId <scriptPath> must be non-zero before Phase 2, catching a stale/reverted script copy in a shared checkout (observed live); the check notes it must be updated if that identifier is renamed.

Testing

# Prose/logic change to a review-skill script; no Go/YAML/docs-site files touched.
node --check <(wrapped)   # syntax-checked with the workflow's async-body wrapper (top-level return is runtime-legal)
git diff --check          # clean

Full make qualify skipped: the change touches only .agents/skills/aicr-cross-review/; tests, lint, and e2e cannot regress from it. Prompt byte-identity under codexResumeJobId was verified statically — the claude/coderabbit/integration/cross/refute prompt builders reference neither the arg nor anything derived from it. Behavioral validation: the demotion path runs on the next cross-review whose integration lane reports a consumer-less finding (previously an incomplete stop); the prompt-free cleanup and probe run on every CodeRabbit lane invocation; the resume path reproduces a live recovery already exercised end-to-end on PR #2097 (three cached lanes replayed, the ~53-minute job's result fetched, nothing re-dispatched).

Risk Assessment

• Low — Isolated change, well-tested, easy to revert
• Medium — Touches multiple components or has broader impact
• High — Breaking change, affects critical paths, or complex rollout

Rollout notes: Takes effect on the next skill invocation from an updated checkout.

Checklist

• Tests pass locally (make test with -race) — N/A, no Go changes
• Linter passes (make lint) — N/A, no lintable sources touched
• I did not skip/disable tests to make CI green
• I added/updated tests for new functionality — N/A (workflow-script logic; validated by syntax check + next live run)
• I updated docs if user-facing behavior changed — SKILL.md updated in lockstep
• Changes follow existing patterns in the codebase
• Commits are cryptographically signed (git commit -S) — GPG signing info

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The workflow supports Codex job resumption with longer continuation waits and exposes codexJobId for incomplete runs. Integration findings without valid consumer coordinates are demoted when their own evidence remains valid. Severity escalation excludes demoted findings. CodeRabbit sandbox bypass is probe-gated and limited to the review command. Cleanup uses find -delete.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: mchmarny, njhensley

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the consumer-less finding demotion and resumable Codex lane changes.
Description check	✅ Passed	The description directly explains the three fixes, implementation details, validation, and affected reviewer tooling.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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 @.agents/skills/aicr-cross-review/scripts/workflow.mjs:
- Around line 927-931: Move the zero-survivor guard containing
incomplete(zeroSurvivors(...)) into the function that owns integBatch and
demotedBatch, or propagate its result through that function’s caller. Preserve
the return incomplete(...) behavior so processing stops immediately; do not
leave a return at module scope or replace it with a bare call.

🪄 Autofix

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: Enterprise

Run ID: 8df0cc7b-9b84-4ca3-bdec-66f90d92a3a6

📥 Commits

Reviewing files that changed from the base of the PR and between 6ed2486 and b612c97.

📒 Files selected for processing (2)

• .agents/skills/aicr-cross-review/SKILL.md
• .agents/skills/aicr-cross-review/scripts/workflow.mjs

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 3

🤖 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 @.agents/skills/aicr-cross-review/scripts/workflow.mjs:
- Around line 295-307: Update both stale-worktree cleanup find commands in the
workflow to include -xdev alongside -depth -delete, ensuring cleanup cannot
descend into mounted filesystems. Revise the nearby explanation to state that
find is restricted from crossing filesystem boundaries, without changing the
existing prefix and age limits or replacing find with rm.
- Around line 925-929: Update the report-routing logic around intakeBatch and
demotedBatch so Integration Findings requires valid consumerPath and
consumerLine values. Detect the demotion flag and route those candidates to
ordinary findings even when sources contains "integration"; preserve normal
integration routing only for findings with a valid consumer pair.

In @.agents/skills/aicr-cross-review/SKILL.md:
- Line 122: The cleanup commands use non-portable find options. In
.agents/skills/aicr-cross-review/SKILL.md at lines 122-122 and 670-671, replace
both direct-file cleanup commands with find "$DIFFPATH" -delete; update the STEP
1 depth-limited reaper to use a BSD/macOS-compatible enumerator or explicitly
require GNU find. In .agents/skills/aicr-cross-review/scripts/workflow.mjs at
lines 292-292, make the corresponding direct-file cleanup change. Ensure reaper
failures are handled so stale-worktree cleanup is not silently skipped.

🪄 Autofix

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: Enterprise

Run ID: e0d43f5c-6066-473f-8f79-0d09ec5b66e1

📥 Commits

Reviewing files that changed from the base of the PR and between 6ed2486 and 65fb234.

📒 Files selected for processing (2)

• .agents/skills/aicr-cross-review/SKILL.md
• .agents/skills/aicr-cross-review/scripts/workflow.mjs

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 3

🤖 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 @.agents/skills/aicr-cross-review/scripts/workflow.mjs:
- Line 46: Validate codexResumeJobId against the companion’s expected job-ID
format before it reaches the Codex resume/status command, reject invalid values,
and shell-quote the validated value during interpolation. Update the parsedArgs
handling and command construction in the workflow flow while leaving unrelated
arguments unchanged.
- Around line 213-217: Update the outer-timeout handling in
.agents/skills/aicr-cross-review/scripts/workflow.mjs lines 213-217 to preserve
the known Codex job ID in the unavailable result and require it for resumption;
update the corresponding protocol guidance in
.agents/skills/aicr-cross-review/SKILL.md lines 393-405, and make
FINDINGS_SCHEMA.jobId required for outer-timeout results so incomplete() emits
codexJobId.

In @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 409-416: The resume example in the Codex recovery instructions
uses invalid object-spread syntax. Update the `Workflow` invocation in the
`incomplete`/`codexJobId` guidance to spread a valid identifier such as
`previousArgs` while preserving the existing arguments and `codexResumeJobId`
value.

🪄 Autofix

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: Enterprise

Run ID: 47e7e7b7-5387-4cdc-b111-b790ec45ac02

📥 Commits

Reviewing files that changed from the base of the PR and between 6ed2486 and de5d32d.

📒 Files selected for processing (2)

• .agents/skills/aicr-cross-review/SKILL.md
• .agents/skills/aicr-cross-review/scripts/workflow.mjs

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 1

🤖 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 @.agents/skills/aicr-cross-review/SKILL.md:
- Around line 690-691: Make the final cleanup around the DIFFPATH removal
idempotent by tolerating an already-absent path, so `set -e` cannot prevent the
subsequent scoped `PRREF` and `BASEREF` deletion commands from running. Update
the “Clean up before finishing” instructions without changing the cleanup scope.

🪄 Autofix

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: Enterprise

Run ID: aed2a065-f71b-4ad3-a62a-817ae08df5da

📥 Commits

Reviewing files that changed from the base of the PR and between 6ed2486 and cd49cd4.

📒 Files selected for processing (2)

• .agents/skills/aicr-cross-review/SKILL.md
• .agents/skills/aicr-cross-review/scripts/workflow.mjs