mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-27 16:05:07 +00:00
taodengandClaude Opus 5
be7c545c21
Review found two HIGH defects. Both confirmed independently before fixing.
1. The Classify step would have failed on EVERY run and produced no summary.
I wrote `set -uo pipefail # NOT -e`. GitHub's default shell is `bash -e {0}`, and
that line does not turn errexit OFF — it only ADDS pipefail. So any category counting
zero makes grep exit 1, pipefail propagates it through the pipeline, the command
substitution inherits it, and errexit kills the step. The all-clean case — the result
this workflow most wants to report — dies on the FIRST counter.
bash -e -c 'set -uo pipefail; x=$(echo hi | grep -c nomatch); echo REACHED'
-> exit=1, "REACHED" never printed
bash -e -c 'set +e -u -o pipefail; ...; echo REACHED'
-> REACHED, x=0
Now `set +e -u -o pipefail`, with the reason in the file so nobody "tidies" it back.
Verified by EXECUTING the extracted step under `bash -e` against real logs:
exit=0, 2836 bytes of summary. My stated verification was `bash -n`, which is a pure
syntax check and structurally cannot catch this. And because workflow_dispatch requires
the file on the default branch, the workflow could not have been run end-to-end before
merge — so nothing else would have caught it either.
2. The Node 22 story was a misattribution, and I had propagated it four places.
I claimed Node 22's `node:sqlite` ExperimentalWarning was read by the boot gate as
"server did not start", invalidating an earlier Linux run. The warning is real; the
causal claim is false. The predicate is
ltWait(() => buf.out.includes("listening on") || buf.exit != null)
stdout only. `buf.err` appears in the assertion MESSAGE, never in the condition, so a
stderr warning cannot fail it. Review also ran the full suite on Node 22.23.1: 462
passed, 0 failed, with the warning present in the logs.
What actually produced that noise floor is something I had already measured and then
failed to connect: pre-#204 fixed ports gave 246 EADDRINUSE and only 42/200 clean runs
on unmodified main. The warning was merely VISIBLE in the failure text — via
buf.err.slice(0,200) — and I read presence in the error message as causation. That is
the same error I have been correcting in others' findings all week.
The cost was not cosmetic: the input description told the next person that Node 22 was
confounded, which would have made them discard a perfectly usable arm. Node 22 is now
offered plainly, and the file states the correction so the wrong story does not survive
in the artifact that outlives this PR.
Also from review:
- `ref` input (MED-1): a null result on current main is uninterpretable, because #204
may already have fixed #203. Hunting `7f15921^` is the positive control.
- inputs go through `env:` (MED-2): free-text inputs were interpolated straight into the
script body. GitHub documents `inputs.*` as untrusted. Added `type: number`.
- #203's SIGNATURE, not its test name (MED-3): a CPU-starved runner blows the 9s ltWait
and emits the same "✗ boot gate REFUSES" line. #203 is closed=true + non-zero exit +
EMPTY stderr. Both counters are reported; the difference is contention. Demonstrated
live — a gate-mutation run scored gate=1, sig=0.
- full histogram instead of a hand-maintained category table: an enumerated list silently
drops the failure nobody thought of, which on a hunt is the interesting one. Bucketed on
a 72-char name prefix, NOT `sed 's/:.*//'` as suggested — test names contain colons, so
that collapses every `localToolsSafetyError: <case>` into one bucket (verified).
- per-run `timeout 300` and step-level timeout (MED-4), `permissions: contents: read`,
a `concurrency:` group, Node 25 in the choices.
- "15 concurrent server.mjs children" was wrong: 15 is TOTAL spawns; peak is 11-12
(review measured 11 x3, I measured 12) because the gate test's 3 cases and the 2 epoch
boots are awaited serially. Corrected.
- dropped the ephemeral-port note: after #204 ports come from ltFreePort(), so it is
stale and the inference now runs backwards.
server.mjs: unchanged (CI-only; ALIGNMENT.md requires no cli.js citation).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8