fix(#1115): capability-probe codex hook-trust bypass flag in /gsd:review; fail loud on empty output (#1122)

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 <noreply@anthropic.com>

.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)

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
 ```
 

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');

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,