fix(#1698): capture codex review via --output-last-message, not stdout
The Codex reviewer in review.md captured the review by redirecting codex exec's stdout to the review file. On Windows, codex writes process-teardown output to stdout after the final agent message, so that noise was appended to a non-empty file and slipped past the `[ ! -s ]` empty-output guard as a silently polluted review (consumed by severity extraction and the plan-review-convergence gate). Capture the final message via codex's own `-o/--output-last-message <FILE>` and discard stdout. The #1115 contract is preserved (stderr to .err, capability-gated $CODEX_BYPASS_FLAG, --ephemeral, --skip-git-repo-check) and the empty-output fallback still fires when codex leaves no/empty output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -277,10 +277,15 @@ fi
|
||||
# $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.
|
||||
# Capture the review via codex's own `-o/--output-last-message <FILE>` (only the
|
||||
# final agent message) and discard stdout (#1698): on some platforms (Windows)
|
||||
# codex writes process-teardown output to stdout *after* the final message, and a
|
||||
# stdout redirect would append that noise to a non-empty file — slipping past the
|
||||
# `[ ! -s … ]` empty-output guard as a silently polluted review.
|
||||
if [ -n "$CODEX_MODEL" ] && [ "$CODEX_MODEL" != "null" ]; then
|
||||
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
|
||||
cat /tmp/gsd-review-prompt-{phase}.md | codex exec --ephemeral $CODEX_BYPASS_FLAG --model "$CODEX_MODEL" --skip-git-repo-check -o /tmp/gsd-review-codex-{phase}.md - 2>/tmp/gsd-review-codex-{phase}.err >/dev/null
|
||||
else
|
||||
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
|
||||
cat /tmp/gsd-review-prompt-{phase}.md | codex exec --ephemeral $CODEX_BYPASS_FLAG --skip-git-repo-check -o /tmp/gsd-review-codex-{phase}.md - 2>/tmp/gsd-review-codex-{phase}.err >/dev/null
|
||||
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
|
||||
|
||||
@@ -99,3 +99,42 @@ describe('enh-773: automated codex exec invocations include --ephemeral and --da
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('#1698 regression: codex review is captured via --output-last-message, not stdout', () => {
|
||||
// WHY: on some platforms (Windows) `codex exec` writes process-teardown output
|
||||
// to stdout *after* the final agent message. A `> FILE` stdout redirect appends
|
||||
// that noise to a non-empty file, so it slips past the `[ ! -s … ]` empty-output
|
||||
// guard and downstream consumers (severity extraction, the
|
||||
// plan-review-convergence "concerns resolved?" gate) parse a polluted review.
|
||||
// `-o/--output-last-message <FILE>` writes only the final message — robust on
|
||||
// every platform — so each codex invocation must capture via -o and discard stdout.
|
||||
const workflow = fs.readFileSync(
|
||||
path.join(process.cwd(), 'gsd-core', 'workflows', 'review.md'),
|
||||
'utf8'
|
||||
);
|
||||
const codexExecLines = workflow
|
||||
.split('\n')
|
||||
.filter((line) => line.includes('codex exec') && !line.includes('codex exec --help'));
|
||||
|
||||
test('every codex exec invocation captures the review via -o <FILE>', () => {
|
||||
for (const line of codexExecLines) {
|
||||
assert.ok(
|
||||
/\s-o\s+\/tmp\/gsd-review-codex-\{phase\}\.md\b/.test(line),
|
||||
`codex exec invocation must capture the review via -o /tmp/gsd-review-codex-{phase}.md:\n ${line.trim()}`
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('no codex exec invocation redirects stdout into the review file', () => {
|
||||
for (const line of codexExecLines) {
|
||||
assert.ok(
|
||||
!/>\s*\/tmp\/gsd-review-codex-\{phase\}\.md\b/.test(line),
|
||||
`codex exec must not redirect stdout into the review file (teardown noise pollutes it); use -o + >/dev/null:\n ${line.trim()}`
|
||||
);
|
||||
assert.ok(
|
||||
/>\s*\/dev\/null\b/.test(line),
|
||||
`codex exec must discard stdout to /dev/null so teardown output is not captured:\n ${line.trim()}`
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -63,7 +63,7 @@
|
||||
"remove-phase.md": 8469,
|
||||
"remove-workspace.md": 7507,
|
||||
"resume-project.md": 17226,
|
||||
"review.md": 40539,
|
||||
"review.md": 40956,
|
||||
"scan.md": 7688,
|
||||
"secure-phase.md": 13476,
|
||||
"session-report.md": 4044,
|
||||
|
||||
Reference in New Issue
Block a user