Skip to content

fix(review): structured review gate reads a terminated run as a clean pass - #2813

Open
chongwon83 wants to merge 1 commit into
garrytan:mainfrom
chongwon83:fix/review-gate-fails-open-on-timeout
Open

fix(review): structured review gate reads a terminated run as a clean pass#2813
chongwon83 wants to merge 1 commit into
garrytan:mainfrom
chongwon83:fix/review-gate-fails-open-on-timeout

Conversation

@chongwon83

Copy link
Copy Markdown

The codex review --base pass in DEFAULT_REVIEW_STEP never captures the
wrapper's exit status, and its gate asks only whether [P1] appears:

_gstack_codex_timeout_wrapper 540 codex review --base <base> ... 2>"$TMPERR"

Check for `[P1]` markers: found → `GATE: FAIL`, not found → `GATE: PASS`.

So when the 540s wrapper terminates the review, output is empty, no [P1] is
present, and the run is recorded as GATE: PASS — then gstack-review-log
stores "status":"clean". A review that produced nothing becomes evidence that
the branch was reviewed and found clean. The same is true for expired auth, a
rejected flag, or a model-entitlement 400: every non-completion reads as a pass.

The adversarial pass a few lines above already gets this right — it treats
exit 124 as "MISSING COVERAGE, not a pass". Only the structured path is
fail-open, which is why it went unnoticed.

Changes:

  • Capture _CODEX_EXIT=$? after the wrapper (it was not captured at all, so the
    gate had no way to know whether codex completed).
  • Replace the single-condition gate with four rules, first match wins: non-zero
    exit → FAIL, empty/whitespace output → FAIL, [P1] → FAIL, otherwise PASS.
  • Ledger: STATUS = "clean" only when every pass COMPLETED. A run that hit
    rule 1 or 2 is missing coverage and must not be logged as clean.

Rules 1 and 2 are the point: absence of a bad marker is not evidence of a good
result unless the check actually ran to completion.

Goldens regenerated (factory-ship-SKILL.md, 9 lines — gate text only).
bun test test/host-config.test.ts test/skill-validation.test.ts test/gen-skill-docs.test.ts → 833 pass / 0 fail.

… pass

The `codex review --base` pass in `DEFAULT_REVIEW_STEP` never captures the
wrapper's exit status, and its gate asks only whether `[P1]` appears:

    _gstack_codex_timeout_wrapper 540 codex review --base <base> ... 2>"$TMPERR"

    Check for `[P1]` markers: found → `GATE: FAIL`, not found → `GATE: PASS`.

So when the 540s wrapper terminates the review, output is empty, no `[P1]` is
present, and the run is recorded as `GATE: PASS` — then `gstack-review-log`
stores `"status":"clean"`. A review that produced nothing becomes evidence that
the branch was reviewed and found clean. The same is true for expired auth, a
rejected flag, or a model-entitlement 400: every non-completion reads as a pass.

The adversarial pass a few lines above already gets this right — it treats
exit 124 as "MISSING COVERAGE, not a pass". Only the structured path is
fail-open, which is why it went unnoticed.

Changes:

- Capture `_CODEX_EXIT=$?` after the wrapper (it was not captured at all, so the
  gate had no way to know whether codex completed).
- Replace the single-condition gate with four rules, first match wins: non-zero
  exit → FAIL, empty/whitespace output → FAIL, `[P1]` → FAIL, otherwise PASS.
- Ledger: `STATUS = "clean"` only when every pass COMPLETED. A run that hit
  rule 1 or 2 is missing coverage and must not be logged as clean.

Rules 1 and 2 are the point: absence of a bad marker is not evidence of a good
result unless the check actually ran to completion.

Goldens regenerated (`factory-ship-SKILL.md`, 9 lines — gate text only).
`bun test test/host-config.test.ts test/skill-validation.test.ts
test/gen-skill-docs.test.ts` → 833 pass / 0 fail.
@trunk-io

trunk-io Bot commented Sep 6, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

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