fix(ci): make the server test diagnose itself and re-arm the vacuous gates - #26
Merged
Merged
Conversation
test_server.jl:44 asserted a bare boolean built by a loop whose `catch` was empty, so "the server crashed at startup" and "the server is not up yet" were indistinguishable, the subprocess was never checked with `process_exited`, and its stdout/stderr went nowhere. Grepping the whole 4,122-line CI log for any server output returned nothing at all — the single failing assertion carried no evidence whatsoever. Now: stdout/stderr are captured to files, the wait loop breaks early on `process_exited`, and on failure the test logs the exact command, the exit code (or "still running"), and the last 40 lines of each stream. Also drops `--compiled-modules=no` from the spawned server's argv. Measured on julia 1.12.5, `Base.julia_cmd()` propagates both `--compiled-modules=no` and `--code-coverage=user` (not `-t`), so the child was loading Oxygen, DuckDB and the pipeline with precompilation disabled inside a ~30s readiness budget. `--code-coverage=user` is deliberately KEPT: the SIGINT in the `finally` block exists precisely so the child flushes its coverage data, and stripping the flag would silently delete the server-route coverage that shutdown path was written to collect. This diagnoses the failure; it does not claim to fix it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
Three of this repo's four checks reported success while proving nothing. 1. Repo hygiene could not fail. 67f2faa added `continue-on-error: true` at job level AND on all four steps, and turned the commit-convention `::error::` into a `::warning::` above an unchanged `exit 1`. Its reason was sound — upstream does not gate on SPDX/format/lint/commit-convention — but the blanket form also disarmed the check here, where it is the only thing watching. Replaced with `${{ github.repository != 'hyperpolymath/MetaManifold-WebUI' }}`: a `pull_request` run evaluates `github.repository` as the BASE repo, so the checks stay advisory for the owner's PR and enforcing on this fork. `::error::` restored, since the step does in fact exit 1. 2. Gate triage reported success on pushes having triaged nothing. All three substantive steps take <owner>/<repo> <pr-number> and were individually gated on `pull_request`, leaving a push to run only a squabbler build and a bundled fixture — then report "Gate triage: success". Gated at job level instead and the now-redundant per-step guards removed. `!cancelled()` is kept, conjoined rather than replaced: without it the job would skip whenever `test` fails, which is the exact case gate triage exists for. Accepted cost: the push leg was also a squabbler-regression canary, and that canary is gone. 3. The cladistic-explorer step was a permanent no-op. `if [ -f test/unit/test_clade_cumulus.jl ]` guarded a file that has never existed on main (only src/analysis/clade_cumulus.jl), so the step always took the else branch and always reported success. Removed. Its sibling analysis-config step lost the same `if [ -f … ]` wrapper: the file exists, and if it is ever deleted the step should go red rather than quietly pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
Five of the six cheap wins from the recon. Each was verified to still hold
against the current tree before being touched.
README
- The CI badge pointed at JoshuaJewell/MetaManifold-WebUI, so the front page
of this fork displayed upstream's build status rather than its own.
- The Julia badge and the prerequisites line both claimed ">= 1.0" while
mise.toml, the CI matrix and test_install_pins.jl all pin 1.12.5 exactly.
- The hero <img> pointed at .github/screenshots/hero.png; that directory has
never existed, so it rendered as a broken image. Block removed.
(The clone URL still points upstream deliberately — that is the project's
canonical home, not drift.)
LICENSES/AGPL-3.0-only.txt
Three SPDX identifiers are in use — AGPL-3.0-only, MPL-2.0, CC-BY-SA-4.0 —
but only the latter two had licence texts, so a strict REUSE lint failed
even though the full AGPL text sits at the repository root. Added from
LICENSE verbatim.
.gitignore
Dropped six `!web/dist/...` un-ignore rules left over from 4ea9ed0. No web/
directory exists anywhere in this repository.
src/MetaManifold.jl
Included analysis/analysis_config.jl, a six-line shim, instead of the
1,615-line canonical analysis/AnalysisConfig.jl. Now includes the canonical
file directly. The shim stays for external callers but is off the load path
and now says so — including both would define module AnalysisConfig twice.
Verified: `using MetaManifold` precompiles and loads, AnalysisConfig defined.
The milestone-3 note describing the old two-hop arrangement is updated.
NOT done, deliberately: `frontend/package.json` test:e2e. Adding Playwright
would mean an unpinned network fetch, lockfile churn and a large non-bun
toolchain, and docs/testing/infrastructure.md already records the e2e lane as
knowingly unprovisioned — "a heavyweight CI provisioning decision reserved for
a later prompt". Overriding a deferred decision is not a cheap win.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
.git is 269.35 MiB of a 282 MB checkout against a ~12 MB working tree, and
`git count-objects -vH` reports prune-packable 0 / garbage 0, so `git gc`
reclaims none of it. This script is the remedy, written out in full and
deliberately NOT executed: it refuses without --i-have-read-the-warnings,
operates on a fresh clone in a scratch directory, and never pushes.
Measuring the pack first corrected the plan in two ways.
1. data/MiSeq_SOP/ holds 170.80 MiB across 54 blobs and is the single
largest item — larger than data/VESPA_pool (143.17 MiB) and
data/Multiplex_pool (71.56 MiB), the only two paths originally slated
for removal. Stripping just those two would have left the biggest
offender untouched.
It also cannot be stripped wholesale. It splits cleanly:
163.09 MiB in 48 uncompressed *.fastq blobs, none of which are present
in the working tree, and 7.71 MiB in 6 *.fastq.gz blobs which ARE live
fixtures, deliberately re-whitelisted at .gitignore:294-304. The strip
rule is therefore a regex anchored on \.fastq$ so .gz never matches.
2. "Nothing junk is committed — no node_modules" is true of the working
tree and false of history: 1,322 node_modules blobs (18.84 MiB) are
reachable, as is 20.13 MiB under a web/ directory that no longer exists.
Full accounting, aggregate blob bytes across all refs:
170.80 MiB 54 data/MiSeq_SOP/ (163.09 dead / 7.71 live)
143.17 MiB 6 data/VESPA_pool/ dead
71.56 MiB 2 data/Multiplex_pool/ dead
20.13 MiB 29 web/ dead
18.84 MiB 1322 **/node_modules/ dead
13.67 MiB 1381 everything else the actual repository
Removing the five dead classes drops ~416.79 MiB of blob content and leaves
~21.4 MiB. That is roughly double what the two-pool plan would have reclaimed.
The script does not push because the push is the dangerous half: this fork's
`main` IS the head branch of JoshuaJewell#6, so a force
push rewrites an open pull request belonging to someone else and invalidates
every existing clone. That stays a human decision, and a coordinated one.
git-filter-repo is not installed on this machine; the script checks for it
and prints the pipx install line rather than falling back to filter-branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
Found by running scripts/check-spdx.sh locally after giving the hygiene gate teeth again. The file arrived in 7782b65 without a header, and the check that would have caught it was non-blocking at the time, so it went in unnoticed. This is the vacuous-gate cost made concrete: the gate was not merely uninformative, it was already concealing a live violation. check-spdx now reports OK across 265 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
The readiness loop's own comment claimed a "30 s budget". That number was an
INFERENCE written in the declarative voice of a measurement, and it was wrong.
Measured 2026-09-21 against a dead port, HTTP.jl 1.11.0: a refused connection
on localhost does NOT return immediately. HTTP.jl's default retry layer treats
ECONNREFUSED as recoverable and retries it with backoff INSIDE a single
HTTP.get call, so one probe costs 1.8-2.1 s (0.63 s with retry=false), not ~0.
The real budget was therefore ~165 s, clocked at 2m44.8s against a server that
was alive but never listening. Both naive arithmetics -- "readtimeout x 60 =
90 s" and "sleep x 60 = 30 s" -- were wrong, because the per-probe cost is set
by a library layer the loop's author never sees.
Fixed so the number in the comment is true by construction rather than by
arithmetic:
* retry=false on the probe. This loop IS the retry; a retry layer nested
inside a retry loop makes the budget unknowable.
* a wall-clock deadline instead of an iteration count, so probe cost can
drift without silently changing what the test waits for.
* the failure report prints MEASURED elapsed time, not the constant.
The budget is 300 s, deliberately ABOVE the ~165 s the old loop effectively
allowed -- that is the budget which already proved insufficient in CI, so
adopting a smaller number would have been a tolerance regression dressed up as
a fix. It is spent only on the failure path; the loop exits the moment the
server binds.
This also weakens the cold-load hypothesis for the CI red: a server that fails
to bind in 2 3/4 minutes fits "slow cold load" considerably worse than one
failing in 30 s.
Verified by killing mutants, not by a green suite:
* crash mutant (error() at the top of server.jl) -> report gives the child
command, exited early: true, exit code 1, and the stderr tail carrying the
error; loop breaks in 4.4 s via process_exited rather than polling out.
* hang mutant (alive, never listening) -> "still running after 120.0 s",
both log files EMPTY handled, SIGINT cleanup still reaps the child.
* unmutated, exact CI flags -> Server smoke tests | 8 pass | 47.1s.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
The script printed a line about the working tree being unchanged and then compared nothing. An inert assertion is worse than no assertion: it occupies the place a reader expects a guard to be, so it is read as one. Record BEFORE_TREE alongside BEFORE_HEAD and compare it to the post-rewrite HEAD tree. The invariant is exact, not approximate: every path in the strip set is already absent from HEAD, so the checked-out tree MUST come out byte-identical. Any difference means a --path or --path-regex caught a LIVE file, and the run must not be pushed. That is the specific danger in this strip set. data/MiSeq_SOP/ splits into 163.09 MiB of dead uncompressed *.fastq and 7.71 MiB of LIVE *.fastq.gz fixtures, deliberately re-whitelisted at .gitignore:294-304. The regex is anchored '\.fastq$' so .gz cannot match -- but "the regex looks right" is not evidence, and the tree comparison is. The script remains unexecuted and still refuses to run without --i-have-read-the-warnings. git-filter-repo is not installed on this machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
hyperpolymath
marked this pull request as ready for review
September 21, 2026 14:25
hyperpolymath
added a commit
that referenced
this pull request
Sep 21, 2026
…gates (#26) ## What this does Seven commits on top of `cc1ccc65`, targeting the fork's `main`. 1. **Makes the server test diagnose itself.** `test/integration/test_server.jl` captured no subprocess output, never checked `process_exited`, and swallowed every exception in a bare `catch` — so `@test ready` was one silent boolean. It now captures stdout/stderr, reports the exit code or the measured elapsed time, and prints the log tail on failure. 2. **Sizes the readiness wait by wall-clock deadline, not iteration count.** HTTP.jl 1.11.0 retries `ECONNREFUSED` *inside* a single `HTTP.get`, so a refused localhost probe costs 1.8–2.1 s, not ~0. The old 60-iteration loop was ~165 s, not the ~30 s its comment claimed. Now `retry=false` plus `READINESS_BUDGET_SECONDS = 300`, deliberately above the ~165 s that already failed in CI so this is not a tolerance regression dressed up as a fix. 3. **Gives the three vacuous checks teeth without blocking upstream.** Five `continue-on-error: true` became `${{ github.repository != 'hyperpolymath/MetaManifold-WebUI' }}`. A `pull_request` run evaluates `github.repository` as the **base** repo, so the gate is advisory for the upstream contribution and enforcing on this fork. Re-arming it immediately exposed a live SPDX violation, fixed here. 4. Doc/licence drift: missing AGPL text, dead `web/` ignore rules, README corrections. 5. A prepared, **never-run** history-rewrite script. It cannot push — the force-push commands are printed instructions inside a heredoc. ## Verification Three mutants, each confirming a different branch of the new diagnostic: | mutant | report | wall clock | |---|---|---| | forced startup crash | `exited early: true`, `exit code: 1`, stderr tail captured | 4.4 s | | alive but never listening | `still running after <N> s`, both logs empty | full budget | | real run, exact CI flags | `Server smoke tests \| 8 pass` | 38.6 s | ⚠ Honest limit: the ~165 s CI failure was never reproduced locally, so this does not prove 300 s is enough on the runner. If it still reds, the report will now say why. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Seven commits on top of
81136ef7, targeting the fork'smain.Makes the server test diagnose itself.
test/integration/test_server.jlcaptured nosubprocess output, never checked
process_exited, and swallowed every exception in a barecatch— so@test readywas one silent boolean. It now captures stdout/stderr, reports theexit code or the measured elapsed time, and prints the log tail on failure.
Sizes the readiness wait by wall-clock deadline, not iteration count. HTTP.jl 1.11.0
retries
ECONNREFUSEDinside a singleHTTP.get, so a refused localhost probe costs1.8–2.1 s, not ~0. The old 60-iteration loop was ~165 s, not the ~30 s its comment claimed.
Now
retry=falseplusREADINESS_BUDGET_SECONDS = 300, deliberately above the ~165 s thatalready failed in CI so this is not a tolerance regression dressed up as a fix.
Gives the three vacuous checks teeth without blocking upstream. Five
continue-on-error: truebecame${{ github.repository != 'hyperpolymath/MetaManifold-WebUI' }}. Apull_requestrunevaluates
github.repositoryas the base repo, so the gate is advisory for the upstreamcontribution and enforcing on this fork. Re-arming it immediately exposed a live SPDX
violation, fixed here.
Doc/licence drift: missing AGPL text, dead
web/ignore rules, README corrections.A prepared, never-run history-rewrite script. It cannot push — the force-push commands
are printed instructions inside a heredoc.
Verification
Three mutants, each confirming a different branch of the new diagnostic:
exited early: true,exit code: 1, stderr tail capturedstill running after <N> s, both logs emptyServer smoke tests | 8 pass⚠ Honest limit: the ~165 s CI failure was never reproduced locally, so this does not prove 300 s
is enough on the runner. If it still reds, the report will now say why.
🤖 Generated with Claude Code
https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm