test: cover check-result.mjs - #15
Conversation
check-result.mjs is the availability gate — action.yml calls it as `node "$CHECK" "$1" "$rc"` and a wrong answer here is silent and expensive: converting a bad key, a gateway outage, or a CLI crash into a clean "no issues found" would clear a PR that was never reviewed. It had no tests. 19 tests over the four fail-closed paths and the one that must NOT fail: - engine exit code: checked before the file is read; nonzero, signal-shaped (137), and non-numeric codes all fail closed rather than coercing to 0. - result shape: missing file, unparseable JSON, absent `comments` key, and non-array `comments` are all "unavailable", never "empty". - partial review: any `warnings` entry fails closed and outranks findings, because partial coverage means the clean files were never checked. An empty or non-array `warnings` value is correctly ignored. - the success path: `comments: []` with no warnings is a REAL complete result and must exit 0, or every clean PR reads as broken. Also pins that stdout stays empty on every path, since the caller captures it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 344fc50027
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| test("a non-array warnings value is ignored rather than treated as partial", () => { |
There was a problem hiding this comment.
When the engine emits a truthy but non-array warnings value—for example after schema drift or malformed output—this assertion requires the availability gate to exit 0, even though it cannot determine whether the review was partial. That contradicts this test suite's fail-closed contract and protects the existing unsafe fallback from correction; the test should require malformed warnings to make the result unavailable.
Useful? React with 👍 / 👎.
| test("an omitted exit code is treated as 0", () => { | ||
| const r = run(writeResult(OK)); | ||
| assert.equal(r.status, 0, r.stderr); |
There was a problem hiding this comment.
If a caller is accidentally rewired to omit the second argument, this test requires the gate to treat the missing engine status as success and accept any parseable result file. That masks exactly the kind of caller/engine failure this availability gate is intended to catch, and it contradicts the documented check-result.mjs <result.json> <exit-code> interface; the contract test should make a missing exit code fail rather than pinning the current default-to-zero behavior.
Useful? React with 👍 / 👎.
1 participant
scripts/check-result.mjsis the availability gate —action.ymlcalls it asnode "$CHECK" "$1" "$rc"— and it had no tests. A wrong answer here is silent and expensive: converting a bad key, a gateway outage, or a CLI crash into a clean "no issues found" would clear a PR that was never actually reviewed.19 tests over the four fail-closed paths and the one case that must not be swept up with them.
Fail closed
Engine exit code - checked before the file is read; nonzero, signal-shaped (
137, the OOM/timeout kill), and non-numeric codes all fail closed rather than coercing to 0.Result shape - missing file, unparseable JSON, absent
commentskey, and non-arraycommentsare all reported as unavailable, never as empty.Partial review - any
warningsentry fails closed and outranks findings: partial coverage means the clean files were never checked, so a result with real findings and warnings is still not trustworthy. An empty or non-arraywarningsvalue is correctly ignored.Must pass
comments: []with no warnings is a real, complete result and has to exit 0. Otherwise every clean PR would read as broken. This is the case most at risk of being "fixed" into a failure by someone tightening the gate.Also pinned
stdout stays empty on every path, since the caller captures it. All diagnostics go to stderr.
Note
The
rcRaw || "0"default means an empty-string exit code is treated as success. That is not reachable from the current call site (action.ymlinitializesrc=0and only overwrites it via|| rc=$?, so it is always numeric), so it is left as-is and not pinned by a test.🤖 Generated with Claude Code