Skip to content

fix(coverage-gate): a test may declare its own instrumented budget - #3605

Closed
TustinOC wants to merge 1 commit into
sonichi:mainfrom
TustinOC:fix/coverage-gate-per-file-timeout
Closed

TustinOC wants to merge 1 commit into
sonichi:mainfrom
TustinOC:fix/coverage-gate-per-file-timeout

Conversation

@TustinOC

Copy link
Copy Markdown
Contributor

Closes the outbox-race half of #3146.

On why I opened this rather than waiting. I listed three levers there and said I would open whichever was chosen. Nobody chose one — @john-the-dev deliberately deferred the remedy to the issue, and no maintainer has weighed in — while the cap is now gating four branches. So this is the lever I argued for, built. If you prefer raising the global cap or scaling the test's rounds, close this; it cost little.

The defect

One wall-clock ceiling covers every file in the suite. scripts/coverage-gate.sh builds a single prefix and hands it to every lane:

COVGATE_TIMEOUT="timeout -k 5 ${COVERAGE_GATE_FILE_TIMEOUT:-120}"

So a concurrency test whose cost is spawning instrumented subprocesses is measured against a budget sized for ordinary unit tests. It passes its own assertion and is then killed — from the failing job on #3587:

✖ test TIMED OUT under instrumentation (>120s): tests/outbox-race.test.py
  12 rounds x 12 concurrent PROCESSES racing one item
  winners per round: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
  rounds with != 1 winner: 0  (exclusion holds)

Scope (@john-the-dev, all 300 Coverage Gate runs over ~47h): 8 of 49 gate failures are this timeout, across 4 distinct branches — only 3 on the PR it was found on. And 212 successes in the same window, so it is not deterministic; it sits just under the line most of the time.

The magnitude, measured

tests/outbox-race.test.py, uninstrumented, this laptop:   rc=0  wall=1.1s
same file, instrumented, CI:                              >120s (killed)

Two orders of magnitude. A CI runner is not 100x slower than a laptop, so coverage run per subprocess is what dominates — the test does about a second of real work.

The change

scripts/coverage-file-timeout.sh solely owns the budget policy: both the runner and the gate's report message resolve through it, so the cap that gets reported cannot drift from the cap that was applied. A file opts out of the shared default in its first 25 lines:

# coverage-gate: timeout=300

Everything undeclared keeps 120s, and COVERAGE_GATE_FILE_TIMEOUT still moves that default. Malformed declarations fall through rather than disabling the cap — timeout=0 especially, which a lenient parser would turn into "no limit".

Before / after

At the parent commit, with the test kept and the implementation reverted:

Ran 11 tests — FAILED (failures=2, errors=12)

At HEAD:

Ran 11 tests in 0.524s — OK

The constructed command is asserted with a timeout stub on PATH, so it holds on a host without coreutils rather than silently proving nothing:

declared.test.py   ARGS: -k 5 420 python3 -m coverage run /tmp/declared.test.py
plain.test.py      ARGS: -k 5 120 python3 -m coverage run /tmp/plain.test.py
zero.test.py       ARGS: -k 5 120 python3 -m coverage run /tmp/zero.test.py

That stub is load-bearing: my first attempt at this evidence ran a 5s sleep against a declared 1s budget and got rc=0, which I nearly read as success. This host has no timeout binary, so the script had correctly degraded to running uncapped — the run proved the opposite of what I wanted it to.

Checks

review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
ruff check                                        All checks passed!
tests/coverage-gate-per-file-timeout.test.py      PASS (11 new)
tests/ci-covers-every-python-test.test.py         PASS
tests/check-python39-compat.test.py               PASS
tests/cwd-lint.test.py                            PASS
bash -n on both shell files                       clean

Not verified locally: shellcheck — not installed on this host, so CI's -S error pass is the first real run of it. Flagging that rather than implying I checked.

One wall-clock ceiling covers every file in the suite, so a concurrency test
that spends its time spawning instrumented subprocesses is measured against a
budget sized for ordinary unit tests. `tests/outbox-race.test.py` runs 36
rounds of 12-way process concurrency: ~1s uninstrumented, past 120s under
`coverage run`. It passes its own exclusion assertion and is then killed, so a
re-run cannot clear it and the cap became a shared merge gate.

Adds `scripts/coverage-file-timeout.sh`, which owns the budget policy alone —
the runner and the gate's report both resolve through it, so the reported cap
cannot drift from the applied one. A file opts out of the default with
`# coverage-gate: timeout=N` in its first 25 lines; everything undeclared keeps
120s, and `COVERAGE_GATE_FILE_TIMEOUT` still moves that default.

Malformed declarations fall through to the default rather than disabling the
cap — `timeout=0` in particular must not be read as "no limit".

Refs sonichi#3146.
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@TustinOC

Copy link
Copy Markdown
Contributor Author

Live evidence, since fixture tests are the weak kind of proof for a CI-path change: the gate this PR modifies ran the full instrumented suite on this branch and nothing timed out.

Coverage job 99401552103, head 8b4f79a4:

$ grep -c "TIMED OUT" <job log>
0

coverage-gate: running Python suite under instrumentation...
coverage-gate: 8 test case(s) SKIPPED in this environment.
coverage-gate: enforcing 95% on lines changed vs origin/main

It reached the enforcement step, which it only does when every file completed. On the same repo, the unmodified gate killed tests/outbox-race.test.py three times in a row on #3587 — including once on a freshly base-merged tree, which is what ruled out a flake.

So this is the modified resolver running in CI against the real suite, not a stub.

All four gates green, including the one I said I could not verify locally:

shellcheck over shell scripts     SUCCESS
typecheck + node & shell tests    SUCCESS
python standalone tests           SUCCESS
diff coverage >= 95% (python)     SUCCESS

shellcheck -S error passing is the answer to the caveat in the body — it had not been run anywhere before this job, and I would rather record that it now has than leave the flag standing.

One thing this does not show: that 300s is the right number. The run proves the file fits inside its declared budget on this runner, not what its actual duration was — the gate still reports pass/fail rather than elapsed time. Sizing it properly still wants the per-test duration logging discussed in #3146.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The fix is proven against the actual failure mode, not just happy-path green, and the design puts the policy in one place.

Bug confirmed independently of this PR. My #3146 measurement is the scope you quoted, so I am not citing your evidence back at you as confirmation: 8 of 49 Coverage Gate failures over ~47h, 4 distinct branches, 212 successes in the same window. The single COVERAGE_GATE_FILE_TIMEOUT prefix on main is real and is what those branches hit.

Verified the fix actually clears it, at your head — this is the check I most wanted, because the body proves 120s was too small without proving 300s is enough:

#3605  diff coverage >= 95% (python)  SUCCESS
job log: no "TIMED OUT" line
         8 skipped: browser-persistent, check-python39-compat,
                    runtime-api-instance-key-fs, workspace-default

outbox-race is not among the skipped, so it ran and completed inside 300s rather than being quietly excluded. That distinction is the whole ballgame — a green gate with the test skipped would look identical at the rollup.

The no-drift claim holds structurally, which is why I think this is the right lever. Both consumers resolve through the same script — runner at coverage-gate.sh:108, report via --print at :134 — so the cap that gets reported cannot diverge from the cap that was applied. That property is what a raised global cap or rescaled rounds would not have given you.

Parser bounds look deliberately chosen: the regex is fully anchored (a prose mention of the marker cannot match), [0-9]{1,4} bounds any declaration at 9999s, and timeout=0 falls through to the default instead of reading as "no limit".

Worst-case disruption, named: this touches CI scripting only — no runtime or product path — so no user-facing behavior changes and there is no state-format or rolling-upgrade surface. The worst case is a file declaring up to 9999s (~2.8h) and holding a runner; that is bounded by the 4-digit regex and requires an in-repo line that shows up in review. The command -v timeout absent branch still runs uncapped, but that was already true on main, so it is not a regression this PR introduces.


Nit, non-blocking — this PR does newly introduce a space-in-path fragility. COVGATE_TIMEOUT is consumed unquoted at coverage-gate.sh:124 to word-split, which was safe when the value was timeout -k 5 120 but now carries a path:

path WITH space:     COVGATE_TIMEOUT=[bash /tmp/cov gate space/scripts/coverage-file-timeout.sh]
                     bash: /tmp/cov: No such file or directory   rc=127
path WITHOUT space:  ran   ok

CI is unaffected — it invokes bash scripts/coverage-gate.sh, so dirname yields the relative scripts. It only bites a clone whose absolute path contains a space, and then rc=127 reports as "test failed under instrumentation" for every file, which is a confusing way to learn about it. Worth a follow-up rather than a respin.

Not blocking on it, and not asking you to re-run anything: the CI-relevant path is correct as it stands.

@TustinOC

Copy link
Copy Markdown
Contributor Author

Closing this. The remedy it implements was already proposed in #3527, argued twice, and retracted by its own proposer with evidence — two days before I built it. I did not find that thread because I searched for PRs referencing #3146 and #3579 and never searched issues for the subject, outbox-race. That is the exact failure @john-the-dev's #3602 exists to prevent, and my #3587 is its worked example.

@yixuan-ag2, 2026-08-29, in #3527:

Correcting my own proposed remedy: this is not one pathological file, and a per-file allowance would not have helped the case I just hit. […] today's tally across my PRs is three distinct files hitting the cap or failing

A per-file budget hands out exemptions one file at a time while the cause keeps producing new ones. That is the wrong layer, and the thread had already reached that conclusion.

My stated mechanism was also wrong, which matters more than the PR. I claimed instrumentation dominates, from 1.1s uninstrumented on my laptop against >120s instrumented in CI. That comparison confounds two variables and I reasoned past the confound rather than resolving it. @qingyun-wu had already measured it properly — same machine, both ways:

plain          rc=0  wall=2.2s
instrumented   rc=0  wall=2.4s

~9% overhead, not 100x. The real arithmetic is in #3527:

coverage-gate.sh:99   COVGATE_WORKERS = nproc     → 4 lanes on a 4-vCPU ubuntu-latest
outbox-race.test.py:35  N, rounds = 24, 12        → mp.Pool(24)

One lane spawning a 24-process pool while three other lanes run, on 4 vCPUs. My laptop finished in ~1s because it has the cores; the runner does not. So it is CPU oversubscription, and raising this file's ceiling would leave the oversubscription intact and the other two files failing.

What I'd keep from this, and it is small: the gate reports a cap it does not resolve per file, so if a per-file budget is ever wanted for some other reason, scripts/coverage-file-timeout.sh in this branch is a working single-owner implementation. It should not land as this fix.

Sorry for the noise, @john-the-dev — you approved this on its own terms and the terms were wrong. I'm correcting #3146 and linking it to #3527, which nobody has done yet.

@TustinOC TustinOC closed this Aug 31, 2026
john-the-dev added a commit that referenced this pull request Aug 31, 2026
…differs

Found by running this tool against the incident it is meant to prevent.

#3605 was built today to fix a problem #3527 had already analysed and whose
remedy its own proposer had retracted two days earlier. Its author said they
missed the thread because they searched for PRs referencing the issue numbers
and never searched for the subject, `outbox-race` — the failure this PR exists
to close.

Run on #3587, the tool does find it:

  ci-triage: 1 failing check(s) on #3587:
    ✖ diff coverage >= 95% (python)
    subjects named in the failure text: tests/outbox-race.test.py
    tests/outbox-race.test.py: 1 open issue(s) — read before diagnosing
        #3527 coverage gate: outbox-race.test.py deterministically exceeds ...

But it found it on the issue BODY, not the title:

  subject extracted:  tests/outbox-race.test.py
  in TITLE? False   ("coverage gate: outbox-race.test.py deterministically ...")
  in BODY?  True    ("`tests/outbox-race.test.py` exceeds the coverage gate's ...")

A CI log names `tests/x.test.py`; a human titles the issue `x.test.py`. So the
match survived only because that body happened to carry the full path — had the
issue named the file in its title alone, the tool would have missed the exact
case it is credited with catching.

Falls back to the basename when the full path finds nothing, guarded so it does
not become a wildcard: only when a `/` was actually stripped, only for names
longer than 6 chars, and still filtered by literal containment rather than
`gh --search` rank.

Verified: removing the fallback makes case (y) fail; case (z) pins that an
unrelated issue is still not matched. Coverage of scripts/ci-triage.py remains
125 statements, 0 missed, 100%.
john-the-dev added a commit that referenced this pull request Sep 3, 2026
…3602)

* feat(ci-triage): map a red check to the issue already filed about it

A recurring CI failure is a pointer into the record, not new information — the
ones that recur are precisely the ones already filed. But the check that goes
red is named for its DETECTOR ("diff coverage >= 95% (python)") while the issue
is titled after the SUBJECT ("outbox-race.test.py"), so searching the check name
returns nothing and the failure reads as novel every time.

Measured cost of not having this: a full triage pass spent re-deriving #3527 —
local-vs-CI timing, the missing `concurrency = multiprocessing`, and the
lane/pool oversubscription arithmetic. #3527 has carried all three since 08-29,
in a thread 14 comments deep, and its title is literally the arithmetic. One
issue search would have replaced the whole investigation.

`scripts/ci-triage.py <PR>` lists failing checks, extracts file subjects from
the PR's bot comments and — for gates that say "see the job log" — from the
failed job log, then searches open issues per subject.

Two properties the output depends on:

- Subjects are ranked by BLAME, not by position. A suite log names every
  skipped and passing file, so whole-text matching ranks noise alongside the
  culprit; lines carrying a failure marker are the ones that accuse a file.
  Pinned by a control that puts the accused file LAST in the text.
- Every gh failure path returns None, never []. A triage tool that reports
  "no open issue" when the search failed is worse than no tool.

Verified on the motivating case: `ci-triage.py 3587` surfaces #3527 as the first
hit on the first subject.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(ci-triage): report nothing rather than a confidently wrong pointer

Dogfooding on a second real failure (#3588) exposed two ways the first cut
misleads, both worse than silence because they carry the tool's authority:

1. With no accusing line, blame-ranking had nothing to rank and fell back to
   whole-text order — so the "subjects" were the shell known-failures allowlist
   and names from a listing, none of them the cause.
2. `gh issue list --search` RANKS, it does not match. `sutando-migrate.test.sh`
   returned #3112 ("graceful-restart lease loss"), which shares tokens and
   nothing else.

Now: subjects come only from lines carrying a failure marker, and an issue is
reported only if it names the subject literally in its title or body. When
neither holds the tool says so and stops.

Measured on the motivating case (#3587): subjects 6 -> 1, issues 2 -> 1, and the
one that remains is #3527 — the correct one. The dropped #3498 was a rank-only
match.

Test (c) is inverted deliberately: benign mentions are now excluded rather than
ranked lower. New (f2) pins that a log with no accusing line yields nothing, and
(k) pins that a rank-only issue is dropped while a literal match is kept —
mutation-verified by reverting the filter.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(ci-triage): read annotations — that is where ::error:: output actually is

Found by using the tool on #3588 and getting nothing useful. Its one real
failure was `FAILED: tests/runtime-tui-reference-client.test.py (exit 1)`, and
that string appears in the job's ANNOTATIONS. The job log holds the workflow's
echoed script — including the literal `::error::` lines it is about to emit —
so grepping the log surfaces the source and not the failure, which reads as
"the cause is invisible" rather than "you are reading the wrong artifact".

Order is comments -> annotations -> log, cheapest and most-specific first.

Test (e2) pins the extraction against the verbatim annotation text captured
from job 99387881539, rather than a line I invented to match the regex.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(ci-triage): a CANCELLED job is capacity, not a defect to triage

Found by causing one. Rerunning a failed job on #3588 while a run was still
active made the concurrency group cancel the in-flight jobs, and the rollup went
from 2 red to 3 red — none of which were test failures. The repo's own workflow
already annotates this correctly:

    python-standalone-tests was CANCELLED (job cap or superseded run)
      — capacity, not a test failure

Reporting a cancelled job as a failing check sends the reader after a cause that
does not exist, which is the same wrong-pointer failure this tool exists to
prevent. Only FAILURE and TIMED_OUT are triaged now.

Test (j2) pins it against a rollup carrying one FAILURE, one CANCELLED and one
SUCCESS — mutation-verified by re-adding CANCELLED to the predicate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test: cover ci-triage.py's read path (42.9% -> 100% diff coverage)

The coverage gate was red on this PR: scripts/ci-triage.py sat at 42.9% diff
coverage with 68 missing lines. Only subjects_from_text — the pure function —
had tests; every network-touching helper was unexercised.

Each helper already takes an injected `run` callable, so the whole read path is
reachable without a network. main() is covered by patching subprocess.run.

Added 14 cases over _gh, failing_checks, failure_text, _run_ids,
annotation_text, log_text, open_issues_for and main. They pin the property the
script exists for: a lookup that did NOT answer must read as UNKNOWN, never as
a confident "nothing filed".

Two of the new tests initially could not fail — mutation testing caught it:

  - dropping annotation_text's non-failed-job guard: the fixture returned rc=1
    for the green job, so _gh nulled it regardless. The green job now returns a
    distinct message that must be absent from the result.
  - dropping _gh's returncode check: _R(1) carries empty stdout, so the body
    parse returned None anyway. Added m2) — rc!=0 with VALID json, which is the
    real shape of a gh error body.

Both mutations now produce a FAIL line; verified by re-applying each against
the patched tests.

Coverage of scripts/ci-triage.py: 119 stmts, 0 missed, 100%.

* style: keep the new test comments under the 2-line prose cap

review-checks flagged two comment blocks I added in the previous commit:

  prose-cap: tests/ci-triage-subjects.test.py:206 comment block is 4 lines (cap 2)
  prose-cap: tests/ci-triage-subjects.test.py:246 comment block is 3 lines (cap 2)

CLAUDE.md is explicit — at most 2 lines, only what the code cannot state
itself. Both blocks were narrating what the test names already say. Trimmed to
the constraint each one actually encodes; the reasoning belongs in the PR body.

Tests unchanged and still passing; comments do not affect coverage.

* fix(ci-triage): match a known issue by basename when the path prefix differs

Found by running this tool against the incident it is meant to prevent.

#3605 was built today to fix a problem #3527 had already analysed and whose
remedy its own proposer had retracted two days earlier. Its author said they
missed the thread because they searched for PRs referencing the issue numbers
and never searched for the subject, `outbox-race` — the failure this PR exists
to close.

Run on #3587, the tool does find it:

  ci-triage: 1 failing check(s) on #3587:
    ✖ diff coverage >= 95% (python)
    subjects named in the failure text: tests/outbox-race.test.py
    tests/outbox-race.test.py: 1 open issue(s) — read before diagnosing
        #3527 coverage gate: outbox-race.test.py deterministically exceeds ...

But it found it on the issue BODY, not the title:

  subject extracted:  tests/outbox-race.test.py
  in TITLE? False   ("coverage gate: outbox-race.test.py deterministically ...")
  in BODY?  True    ("`tests/outbox-race.test.py` exceeds the coverage gate's ...")

A CI log names `tests/x.test.py`; a human titles the issue `x.test.py`. So the
match survived only because that body happened to carry the full path — had the
issue named the file in its title alone, the tool would have missed the exact
case it is credited with catching.

Falls back to the basename when the full path finds nothing, guarded so it does
not become a wildcard: only when a `/` was actually stripped, only for names
longer than 6 chars, and still filtered by literal containment rather than
`gh --search` rank.

Verified: removing the fallback makes case (y) fail; case (z) pins that an
unrelated issue is still not matched. Coverage of scripts/ci-triage.py remains
125 statements, 0 missed, 100%.

* style: keep the basename-fallback comment under the 2-line prose cap

review-checks flagged tests/ci-triage-subjects.test.py:281 at 3 lines (cap 2).
Second time I have tripped this lint on this branch; the constraint the comment
encodes fits in two lines and the narrative belongs in the PR body.

* fix(ci-triage): read the subject from BOT comments only

Found by running the tool on its own PR while a check was red.

The real failure was prose-cap naming tests/ci-triage-subjects.test.py. The
tool reported two entirely different subjects:

  ci-triage: 1 failing check(s) on #3602:
    ✖ typecheck + node & shell tests
    subjects named in the failure text: tests/outbox-race.test.py,
                                        tests/discord-bridge-archive-failure-keeps-the-file.test.py

Both came from PR comments — including this tool's own quoted output in a
comment I had posted an hour earlier. `failure_text` joined every comment body,
so a human or agent comment quoting an unrelated failure supplies filenames that
then read as the CURRENT failure's subject, with the tool's authority behind
them. That is worse than reporting nothing: it sends the reader at a file that
has no connection to the red check.

The docstring already said BOT comments — the code did not implement it. Gates
that report to the PR rather than the log post as `github-actions`; prose does
not. Filtering to bot authors keeps the case the fallback exists for and drops
the noise.

Verified: case (bb) fails when the filter is removed. Four pre-existing fixtures
built comments with no author and were correctly excluded by the new filter —
they now carry a bot author so they still exercise the path they were written
for, rather than being loosened to pass.

Not a defect, checked and excluded: `log_text` returned 0 chars on the same run.
That is `gh run view --log` refusing while the run is still in progress, not a
bug in the fallback.

Coverage of scripts/ci-triage.py: 130 statements, 0 missed, 100%.

* fix(ci-triage): a filename is not an accusation, and a lint hit is

Found by running the tool on #3606 while it was red. It was wrong in BOTH
directions on the same input:

  reported: 6 subjects, every one a file whose NAME contains "failure"
  actual:   prose-cap on tests/injection-guard-sweep.test.py

  _BLAMED vs a line merely LISTING *-failure-*.test.py   -> True   (false +)
  _BLAMED vs the real prose-cap accusation               -> False  (false -)

Two independent causes:

1. `_BLAMED` matches "FAIL" case-insensitively, and this repo has test files
   named `*-failure-*.test.py`. Any line listing them read as an accusation of
   every file on it. Now the blame test runs against the line with its PATHS
   REMOVED, so a filename can no longer accuse itself.

2. The blame vocabulary is test-runner output — ✖, FAILED, Traceback. Lint
   output carries none of it: `tool: path:LINE message`. So a real lint finding
   was invisible. Added that shape.

Live on #3606 after the fix: one subject, `tests/injection-guard-sweep.test.py`,
which is the file prose-cap actually named.

Mutation-verified separately, since one fix could have masked the other:
removing the path-strip fails (cc); removing the lint shape fails (dd); (ee)
pins that an ordinary ✖ accusation still resolves with its path intact.
Coverage of scripts/ci-triage.py: 134 statements, 0 missed, 100%.

* style: keep the path-strip comment under the 2-line prose cap

My own pre-push check printed this violation and I pushed anyway — the commit
ran unconditionally after it. A check that reports without gating is not a
mechanism. Now gated on its exit.

* fix(ci-triage): triage a commit status in ERROR, not only FAILURE

A rollup unions two shapes: CheckRuns carry `conclusion`, legacy commit
statuses carry `state`, and `state` has a distinct `error` for a reporter
that crashed. The predicate handled FAILURE only, so an errored external
gate read as "no failing checks" — the exact green-on-no-answer this tool
exists to prevent. Also pins per_page=100 on the jobs and annotations
fetches, which default to 30 and would silently truncate a >30-job run.

Reported by @sutando-qingyun-001 with a positive control.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
Co-authored-by: john-the-dev <ruiwangwarm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants