Runner & adapter resilience (review/generic/codex) - #370
Merged
Conversation
Audit fixes for runner & adapter resilience: - review.ts: confirm a claimed REVIEW_RESULT:MERGED decision against `gh pr view <n> --json state` before applying the terminal merged outcome; downgrade to "unknown" (task fails loudly) when the PR is not actually merged, so a hallucinated sentinel can't move an issue to Done with an open PR. - review.ts: fail loudly when PR-branch resolution returns a non-zero exit code or empty headRefName, instead of silently reviewing (and potentially approving/merging) the BASE branch. - cli/codex.ts: wrap the lastMessageFile read in extractFinalResponse in try/catch so an IO race falls through to the stream-json log instead of throwing out of result handling. - runner/generic.ts: treat an empty/whitespace-only .critter-report.md as missing and fall through to the stream-json fallback. Adds src/__tests__/audit-runners.test.ts covering the merged-confirmation gate (downgrade + confirmed-merge + branch-resolution failure), the generic empty-report fallback, and codex IO resilience. Deferred: removing parseReviewOutcome is left out because the non-owned test src/__tests__/review-spawner.test.ts imports it. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Production review parsing already flows through ctx.cliAdapter.extractReviewDecision; drop the thin parseReviewOutcome wrapper and its export, and repoint its tests to exercise the adapter directly. Also collapse the merged-confirmation downgrade and the no-sentinel fallback into a single `gh pr view --json state` query since both inspect the identical post-spawn PR state. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
resolvePhaseMcpConfig expands ~ via node:os homedir(); the test compared it to process.env.HOME. Bun caches homedir() on first call and ignores later process.env.HOME mutations, so when a sibling test changes HOME they diverge on Linux CI and this test fails (works on macOS by execution-order luck). Assert against the same homedir() the implementation uses. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden the phase runners against trusting agent output and IO races.
REVIEW_RESULT:MERGEDsentinelgh pr view <n> --json state == MERGEDbefore applying the merged outcome; else downgradeghfailure → silently reviewed the base branch (and could approve/merge)extractFinalResponseread the last-message file withouttry/catchunknown.critter-report.mdsuppressed the stream-json fallbackparseReviewOutcome(bypassed the adapter abstraction)cliAdapter.extractReviewDecision; redundant doublegh pr viewdedupedTests
audit-runners.test.ts— merged-confirmation gate and empty-report fallback (gh/file IO mocked).Verification
typecheck clean · lint exit 0 ·
bun test849 pass / 0 fail.🤖 Generated with Claude Code