From fc37ae0c4f57844f6c8661f879f17216abb336d2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 12 Jun 2026 15:55:17 -0400 Subject: [PATCH] fix(#1115): capability-probe codex hook-trust bypass flag in /gsd:review; fail loud on empty output (#1122) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On codex-cli < 0.137.0 the review.md `codex exec` invocation passed --dangerously-bypass-hook-trust (added in 0.137) unconditionally and discarded stderr, so codex exited "unexpected argument" before reading the prompt and the empty output file was treated as a completed review — a silent degraded review. - Capability-probe the flag (`codex exec --help | grep`) and apply it via $CODEX_BYPASS_FLAG only when supported (works fine without it on older CLIs). - Capture codex stderr to a .err file instead of /dev/null, and replace an empty output with a diagnostic so a broken reviewer is surfaced (mirrors Cursor/OpenCode). - Update enh-773 enforcement test to require the capability gate + fail-loud guard instead of the unconditional flag. Closes #1115 Co-authored-by: Claude Opus 4.8 --- ...1115-review-codex-flag-capability-probe.md | 5 +++ gsd-core/workflows/review.md | 21 ++++++++- ...h-773-codex-exec-automation-flags.test.cjs | 43 ++++++++++++++++--- tests/workflow-size-baseline.json | 2 +- 4 files changed, 63 insertions(+), 8 deletions(-) create mode 100644 .changeset/1115-review-codex-flag-capability-probe.md diff --git a/.changeset/1115-review-codex-flag-capability-probe.md b/.changeset/1115-review-codex-flag-capability-probe.md new file mode 100644 index 000000000..f289486c2 --- /dev/null +++ b/.changeset/1115-review-codex-flag-capability-probe.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1122 +--- +**`/gsd:review` no longer produces a silent empty Codex review on codex-cli < 0.137** — the `codex exec` invocation passed `--dangerously-bypass-hook-trust` (added in codex 0.137.0) unconditionally and discarded stderr, so on older CLIs codex exited with `unexpected argument` before reading the prompt and the empty output was treated as a completed review. The flag is now capability-probed (`codex exec --help | grep`) and applied via `$CODEX_BYPASS_FLAG` only when supported, codex stderr is captured to a `.err` file instead of `/dev/null`, and an empty Codex output is replaced with a diagnostic so a broken reviewer is surfaced rather than silently skipped. (#1115) diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index d2322f07d..488f52da2 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -223,6 +223,16 @@ CODEX_MODEL=$(gsd_run query config-get review.models.codex 2>/dev/null | jq -r ' OPENCODE_MODEL=$(gsd_run query config-get review.models.opencode 2>/dev/null | jq -r '.' 2>/dev/null || true) # review.models.agy is reserved for future model-pinning support; agy selects its model internally AGY_MODEL=$(gsd_run query config-get review.models.agy 2>/dev/null | jq -r '.' 2>/dev/null || true) + +# #1115: `--dangerously-bypass-hook-trust` only exists on codex-cli >= 0.137.0. +# Capability-probe it so older installs don't fail with "unexpected argument" +# (which, with stderr suppressed, produced a silent empty review). The codex +# invocation works fine without the flag on older versions. +if codex exec --help 2>/dev/null | grep -q -- '--dangerously-bypass-hook-trust'; then + CODEX_BYPASS_FLAG="--dangerously-bypass-hook-trust" +else + CODEX_BYPASS_FLAG="" +fi ``` For each selected CLI, invoke in sequence (not parallel — avoid rate limits): @@ -247,10 +257,17 @@ fi **Codex:** ```bash +# $CODEX_BYPASS_FLAG is capability-gated above (#1115). Capture stderr to a .err +# file (not /dev/null) so a non-zero exit — e.g. a flag the installed codex-cli +# does not support — is diagnosable instead of a silent empty review. if [ -n "$CODEX_MODEL" ] && [ "$CODEX_MODEL" != "null" ]; then - cat /tmp/gsd-review-prompt-{phase}.md | codex exec --ephemeral --dangerously-bypass-hook-trust --model "$CODEX_MODEL" --skip-git-repo-check - 2>/dev/null > /tmp/gsd-review-codex-{phase}.md + cat /tmp/gsd-review-prompt-{phase}.md | codex exec --ephemeral $CODEX_BYPASS_FLAG --model "$CODEX_MODEL" --skip-git-repo-check - 2>/tmp/gsd-review-codex-{phase}.err > /tmp/gsd-review-codex-{phase}.md else - cat /tmp/gsd-review-prompt-{phase}.md | codex exec --ephemeral --dangerously-bypass-hook-trust --skip-git-repo-check - 2>/dev/null > /tmp/gsd-review-codex-{phase}.md + cat /tmp/gsd-review-prompt-{phase}.md | codex exec --ephemeral $CODEX_BYPASS_FLAG --skip-git-repo-check - 2>/tmp/gsd-review-codex-{phase}.err > /tmp/gsd-review-codex-{phase}.md +fi +if [ ! -s /tmp/gsd-review-codex-{phase}.md ]; then + echo "Codex review failed or returned empty output. stderr:" > /tmp/gsd-review-codex-{phase}.md + cat /tmp/gsd-review-codex-{phase}.err >> /tmp/gsd-review-codex-{phase}.md fi ``` diff --git a/tests/enh-773-codex-exec-automation-flags.test.cjs b/tests/enh-773-codex-exec-automation-flags.test.cjs index c3697de74..68779f187 100644 --- a/tests/enh-773-codex-exec-automation-flags.test.cjs +++ b/tests/enh-773-codex-exec-automation-flags.test.cjs @@ -15,10 +15,12 @@ describe('enh-773: automated codex exec invocations include --ephemeral and --da 'utf8' ); - // Extract all codex exec invocation lines from code fences + // Extract codex exec INVOCATION lines from code fences. The #1115 capability + // probe (`codex exec --help | grep …`) is not an automation invocation, so it + // is excluded from the per-invocation flag assertions below. const codexExecLines = workflow .split('\n') - .filter((line) => line.includes('codex exec')); + .filter((line) => line.includes('codex exec') && !line.includes('codex exec --help')); test('review.md contains at least one codex exec invocation', () => { assert.ok( @@ -36,15 +38,46 @@ describe('enh-773: automated codex exec invocations include --ephemeral and --da } }); - test('every codex exec invocation includes --dangerously-bypass-hook-trust', () => { + test('#1115: the hook-trust bypass is capability-gated, not passed unconditionally', () => { + // --dangerously-bypass-hook-trust only exists on codex-cli >= 0.137.0. It must + // be probed (`codex exec --help | grep`) and applied via $CODEX_BYPASS_FLAG so + // older installs do not fail with "unexpected argument" (a silent empty review). + assert.ok( + /codex exec --help[^\n]*grep[^\n]*--dangerously-bypass-hook-trust/.test(workflow), + 'review.md must capability-probe --dangerously-bypass-hook-trust via `codex exec --help | grep`' + ); + assert.ok( + workflow.includes('CODEX_BYPASS_FLAG="--dangerously-bypass-hook-trust"'), + 'the probe must set CODEX_BYPASS_FLAG to the flag when the CLI supports it' + ); for (const line of codexExecLines) { assert.ok( - line.includes('--dangerously-bypass-hook-trust'), - `codex exec invocation is missing --dangerously-bypass-hook-trust:\n ${line.trim()}` + line.includes('$CODEX_BYPASS_FLAG'), + `codex exec invocation must apply the capability-gated $CODEX_BYPASS_FLAG, not an unconditional flag:\n ${line.trim()}` + ); + // …and must NOT also pass the literal flag (that would reintroduce #1115). + assert.ok( + !line.includes('--dangerously-bypass-hook-trust'), + `codex exec invocation must not pass the literal --dangerously-bypass-hook-trust (use the gated $CODEX_BYPASS_FLAG):\n ${line.trim()}` ); } }); + test('#1115: codex review failures are surfaced, not silently swallowed', () => { + // stderr must be captured (not discarded to /dev/null) and an empty output + // must be replaced with a diagnostic, so a broken reviewer is reported. + for (const line of codexExecLines) { + assert.ok( + !line.includes('2>/dev/null'), + `codex exec must not discard stderr to /dev/null:\n ${line.trim()}` + ); + } + assert.ok( + /\[ ! -s \/tmp\/gsd-review-codex-\{phase\}\.md \]/.test(workflow), + 'review.md must guard against an empty codex review output and surface the failure' + ); + }); + test('--ephemeral appears before the prompt argument (flag ordering)', () => { for (const line of codexExecLines) { const ephemeralPos = line.indexOf('--ephemeral'); diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index 40bbd0cc6..38cec2690 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -62,7 +62,7 @@ "remove-phase.md": 8469, "remove-workspace.md": 7507, "resume-project.md": 15288, - "review.md": 37079, + "review.md": 38031, "scan.md": 7688, "secure-phase.md": 12187, "session-report.md": 4044,