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,