Stop-review gate discards the failure reason on four paths; every failure surfaces as a DEP0190 warning

[Sumstead72]

Summary

stop-review-gate-hook.mjs discards the reason a stop-time review failed, on four independent paths. In practice every failure of the gate surfaces as an unrelated Node deprecation warning, which is indistinguishable from a broken toolchain and gives the user nothing to act on.

Observed on plugin version 1.0.4, Windows 11, Node v24.

1. The stderr || stdout fallback is unreachable

const detail = String(result.stderr || result.stdout || "").trim();

codex-companion.mjs prints a DEP0190 deprecation warning to stderr on every run, including successful ones:

(node:31720) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true ...
(Use `node --trace-deprecation ...` to show where the warning was created)

So result.stderr is always truthy and || result.stdout is dead code. Meanwhile runForegroundCommand reports a failed run by writing its JSON payload to stdout and propagating the exit status, adding nothing to stderr. The real reason is therefore always on the stream this expression can never read.

Reproduction — a child that fails exactly the way the companion does:

process.emitWarning('shell option true', { type: 'DeprecationWarning', code: 'DEP0190' });
console.log(JSON.stringify({ status: 1, rawOutput: 'THE REAL REASON' }));
process.exitCode = 1;

String(r.stderr || r.stdout) yields the deprecation warning. THE REAL REASON is discarded.

2. ?? treats an empty string as present

String(payload?.error ?? payload?.rawOutput ?? "")

?? falls through only on null/undefined, so an empty error string blocks rawOutput from ever being read. And an empty rawOutput produces an empty detail, which drops the entire payload — including status and threadId, the only identifiers for the failed turn — leaving the generic "task failed" message.

Also, error may be a structured object; String(obj) renders [object Object].

3. On exit ZERO, a non-verdict answer is replaced by a label

return {
  ok: false,
  reason: "The stop-time Codex review task returned an unexpected answer. ..."
};

This is the branch reached when the model returns something that is neither ALLOW: nor BLOCK: — which is exactly where a refusal, an error explanation, or a truncated answer arrives. The text is thrown away, so every such failure looks identical.

The JSON.parse catch has the same shape: it reports "returned invalid JSON" and discards the bytes.

4. BLOCK keeps only its first line

const reason = firstLine.slice("BLOCK:".length).trim() || text;

The body is dropped unless the headline is empty, so the file, the reproduction, and the reasoning are lost and the user receives a one-line assertion they must re-derive.

Suggested fix

Report both streams rather than choosing between one, and never replace text with a label:

• Filter Node warning lines out of stderr, then include both the filtered stderr and the stdout payload, each labelled — so neither can suppress the other. Filtering a fixed list of warning shapes and then still preferring stderr is not sufficient: any unrecognised stderr line re-triggers the same discard.
• Treat empty strings as absent when choosing between error and rawOutput, JSON.stringify a non-string value, and fall back to the full payload text so the detail can never be empty while the child produced output.
• Include the offending text in the "unexpected answer" and "invalid JSON" branches, bounded with an explicit truncation marker rather than dropped.
• Carry the BLOCK body alongside its headline.

Happy to open a PR if useful.

Note

I could not reproduce the underlying non-zero exit on demand — six controlled attempts (fresh thread, resumed thread, benign prompt, long benign, short prompt dense with shell commands, long and dense) all returned exit 0. That is a separate question from this report: whatever the cause, the gate should be able to state it.