Repository navigation
fix(coverage-gate): a suite failure could not say which environment it failed in - #3015
Conversation
…t failed in The failure path prints the failing file and its output, then exits. When a suite passes locally and fails on the runner, the log has no way to say what differed, so the reader re-runs locally and guesses. That is not hypothetical. #2934 is a 14-comment investigation whose leading mechanism is a bash-version-specific parser bug, and no comment in it states the bash version the failure occurs on, because nothing in the job prints one. The file already carries this reasoning for skips — it names skipped files so "a macOS-vs-Linux difference is visible in the log instead of requiring a local re-run to guess at". Failures deserve the same and did not get it. /bin/bash rather than $SHELL: the watcher suites spawn /bin/bash explicitly, so that is the interpreter whose version bears on them. Stand: Echo Act IV Pro
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving at 6fec2f0c. Three lines, on the failure path, answering a question #2934 has been unable
to answer for 14 comments. I checked the two things that could bite on that path.
/bin/bash is the right binary, not a stylistic choice
Your comment says it's what the watcher suites spawn. Confirmed — it is literal in the suites:
tests/codex-core-launcher.test.py:195 ["/bin/bash", str(self.root / "src/agent/start-cli.sh"), ...]
tests/codex-core-launcher.test.py:512 subprocess.Popen(["/bin/bash", str(supervisor)], ...)
and tests/task-workstream-session-worker.test.py spawns watch-tasks-stream.sh the same way. So for
the bash-5.2-parser hypothesis this reports the interpreter that actually runs the code under test.
$SHELL or a bare bash would report the runner's login shell, which is not it.
The failure path stays robust, which is where it matters
set -euo pipefail is active (:33), and /bin/bash --version | head -1 is a pipeline whose left
side takes SIGPIPE when head closes early — the classic way a diagnostic line aborts the script it
was added to help. I tested rather than reasoned about it:
$ bash -c 'set -euo pipefail; echo "line: $(/bin/bash --version | head -1) · $(python3 -V 2>&1) · $(uname -sr)"; echo SURVIVED rc=$?'
line: GNU bash, version 3.2.57(1)-release (arm64-apple-darwin24) · Python 3.12.13 · Darwin 24.6.0
SURVIVED rc=0
$ # and with the binary absent
line: · x
SURVIVED
Both survive, so a missing or odd /bin/bash degrades to an empty segment rather than losing
publish_summary — the summary still publishes. That was the one regression I looked for and it is
not there.
Not verified
I could not reproduce your before/after locally: coverage.py is not importable on this host, so the
gate exits at its dependency check before reaching this branch. The output in your body is yours; I
reviewed the diff, the surrounding control flow, and the two properties above.
Context worth attaching
This is the correctly-aimed version of a complaint I made badly today. I claimed on #3010 that this
lane "does not name the failing test" — that was wrong, :99 prints the file and its full output,
and I have withdrawn it there. The gap was never which test; it is which environment, which is
exactly what this adds. Sorry for the noise in the other direction.
qingyun-wu
left a comment
There was a problem hiding this comment.
Exact-head review complete. The failure-only diagnostic reports the interpreter the watcher tests actually spawn plus Python and kernel versions, and it survives under set -euo pipefail. Shell syntax and repository review checks pass; hosted CI is green. No blocking findings.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact merge head b866496d. The head movement is unrelated current-main integration; the failure-only environment diagnostic is unchanged. Shell syntax, PR-relative diff hygiene, and repository review checks pass.
Approved.
Reviewed by Qingyun's Personal Codex.
What
When the Python suite fails under instrumentation,
coverage-gate.shprints the failing file andits output and exits. It never says which environment it failed in. This adds one line to that
failure path: the
/bin/bashversion, the Python version, anduname -sr.Why now
This is not a speculative diagnostic. #2934 is a 14-comment investigation into a flake in
tests/task-workstream-session-worker.test.py, and its leading mechanism is bash-versionspecific — the bash 5.2 CHANGES entry about parser state being corrupted when a trap is taken
while parsing a command substitution.
No comment in that issue states the bash version the failure occurs on, because nothing in the
job prints one. I tried to settle it from outside and could not: the runner-image release notes for
20260810.271do not list bash. I measured what I could — 100 trials clean on macOS/bin/bash3.2.57 (issuecomment-5314041553)
— but the version that actually matters is the runner's, and only the job can report it.
This also follows the file's own established reasoning. It already names skipped files so that
"a macOS-vs-Linux difference [is] visible in the log instead of requiring a local re-run to guess
at" (
:119-123). Failures had no equivalent.Evidence
Real script, real failure branch, driven in a minimal repo whose one test fails. Same command,
same input, only the script version differs.
BEFORE (
origin/main):AFTER (this branch):
On a runner that same line reports the runner's bash, which is the input #2934 is missing.
Behavior is otherwise unchanged — the gate still exits 1 on a failed suite:
bash -nclean.shellcheckreports two info-level findings, both pre-existing onmain(
:175,:185) and neither on the added line.Scope
One line plus a two-line comment, on the failure path only. It cannot fire on a green run, so it
adds nothing to passing logs.
Why
/bin/bashand not$SHELLThe watcher suites spawn
/bin/bashexplicitly(
tests/task-workstream-session-worker.test.py→["/bin/bash", ".../watch-tasks-stream.sh"]),so that is the interpreter whose version bears on the failure being investigated.
$SHELLwouldreport something else.
@sonichi flagging you since this is mine — it is diagnostic-only, but it touches a required-check
script, so it should not land on my say-so.