ci: make the four remaining green-no-matter-what checks able to fail - #3
Open
zah wants to merge 2 commits into
Open
ci: make the four remaining green-no-matter-what checks able to fail#3zah wants to merge 2 commits into
zah wants to merge 2 commits into
Conversation
The "Test reproducible builds" step could not detect an irreproducible build for two independent reasons: diff -r result-1 result-1-rebuild || echo "Build not reproducible ..." 1. A real difference was swallowed by `|| echo`: the step printed a message and still exited 0, so the job stayed green. 2. The "rebuild" invocations were plain `nix build` calls, which resolve to the store path that is already there instead of re-running the derivation. The two out-links therefore always pointed at the same store path and the comparison was vacuous even before the `|| echo`. The rebuild now uses `nix build --rebuild`, so Nix actually re-executes the derivation and compares the fresh output with the one in the store, and the comparison loop prints the full diff and exits non-zero when the outputs differ. This may turn the job red. If it does, the irreproducibility is pre-existing and was previously hidden, not introduced here.
Four checks in the pipeline reported success regardless of what they
observed. Each is now an assertion.
Component smoke test ("Test <component> basic functionality"): every
branch ended in `|| true`, so a binary that segfaults on startup passed
exactly like one that works. The long-running services are now required
to still be alive when `timeout` kills them (exit 124); the one-shot
CLIs are required to exit 0 from `--help`. Core dumps are disabled in
the step because a crash that is slow to dump core can outlive the
deadline and be misreported as the healthy 124.
Reproducibility check: the CI step and the `ci-docs-reproducibility`
Justfile recipe now share scripts/check-reproducibility.sh. The recipe
previously ran no reproducibility check at all, so `just ci-main` could
not catch a regression locally.
Artifact download ("Make binaries executable"): `chmod`/`ls` were both
`|| true`, so an empty or partial download passed and surfaced later as
a confusing integration-test failure. All five binaries are now
required to be present.
Coverage generation: `cargo install cargo-tarpaulin || true` has been
failing outright (cargo-platform needs rustc 1.91, the pinned toolchain
is 1.86), leaving `cargo tarpaulin` as "no such command" -- also
swallowed. No coverage has been produced for some time and the uploaded
number was fictional. cargo-tarpaulin now comes from the dev shell and
the report must exist. The upload to codecov.io stays advisory, since a
third-party outage should not fail the build.
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
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.
Follow-up to #2, which fixed the reproducibility gate. That review turned up four more checks in this pipeline that reported success no matter what they observed. This PR turns each one into an assertion.
One serious finding, described in full under item 4: coverage generation has been failing outright for some time, and
|| truehid it. The coverage number published for this repo is fictional.Item 1 — component smoke test could not detect a binary that never starts
ci.yml, jobbuild-matrix, step "Test<component>basic functionality". Every branch ended in|| true:A binary that segfaults on startup passed exactly like one that works.
Landed. The three long-running services must still be alive when
timeoutkills them; the one-shot CLIs must exit 0 from--help:Why 124 is the pass condition. GNU
timeoutreturns 124 only when the command was still running at the deadline and had to be killed — which for a server is exactly the healthy outcome. A process that exits on its own returns its own status (0, 1, 101, 139 …), and for a server that means it crashed or refused to start. Verified against every mode:That third-to-last case matters:
cpp-echo-servicereally does trap SIGTERM and exit 0 cleanly (Shutdown signal 15 receivedappears in its CI log).timeoutstill reports 124, so the assertion is correct for it.A trap found while verifying, and closed. A crashing binary that is slow to dump core can still be dying when the deadline fires, and
timeoutthen reports 124 — the healthy code — for a segfault. This is not hypothetical; it reproduced here. Running the step withoutulimit -c 0, five times, against a fixture that segfaults immediately:Hence the
ulimit -c 0line: with core dumps off, a crash is reported immediately and honestly, and the check stops being flaky in the one direction a check must never be flaky.Mutation verification — item 1
Step body extracted from the committed YAML with
yq, with${{ matrix.component }}substituted exactly as Actions does, run against fixture binaries:cpp-echo-servicebehaviour)--helpexits 0--helpexits 2Control — the old body against the same three failing fixtures: 0, 0, 0.
The assertion also fires on the real binaries
Run against the actual built components rather than fixtures:
That last line is a real "service failed to start", caught by the new assertion and invisible under the old one:
It is specific to the machine it ran on — port 3000 was already occupied there — not a defect in this repo; on the CI runner the agent binds 3000 and stays up for the full 10 s, which is why this item is expected to stay green. It is worth stating plainly, though, that this check now reports a genuine failure when a port is unavailable, because in that situation the service genuinely did not start. On ephemeral GitHub-hosted runners each job gets a clean VM, so there is no shared-port hazard; on a developer workstation a red result here is the truth, not noise.
Item 2 —
just ci-docs-reproducibilityran no reproducibility checkJustfile:212wasci-docs-reproducibility: check generate-docswith an empty body, sojust ci-maincould not catch a reproducibility regression locally no matter what.Landed. The check is now
scripts/check-reproducibility.sh(the body from #2, unchanged in behaviour), and both callers run it:ci-docs-reproducibility: check generate-docs @echo "=== Running Reproducibility Check ===" ./scripts/check-reproducibility.shMutation verification — item 2
scripts/check-reproducibility.shrun with a stubnixthat materializes the out-links from fixtures:rust-echo-serviceoutput differsattestation-agentoutput differs (content + stray file)Control: the old recipe had no body — no check ran, so the local command was incapable of failing.
Item 3 — a missing artifact download passed
An empty or partial download passed here and surfaced later as a confusing integration-test failure.
Landed. All five binaries must be present, and the failure message shows what actually arrived:
The list is not guesswork — the previous run's
lsoutput shows exactly these five arriving.Mutation verification — item 3
rust-echo-servicemissingno 'build' directory at all)Control — the old body against the same two failing fixtures: 0 and 0 (the second printing
chmod: cannot access 'build/bin/*'and passing anyway).Item 4 — coverage has been failing silently; the published number is fictional
This is the finding. The step was:
Both commands have been failing. From the most recent run's
Generate test coveragestep:cargo installcannot build a current cargo-tarpaulin against the pinned 1.86 toolchain, socargo tarpaulinis not a command, so nocobertura.xmlis written — andfail_ci_if_error: falsethen lets the Codecov step upload nothing without complaint. Every layer of this was suppressed, so coverage silently stopped being measured while continuing to look measured. Whatever coverage figure is associated with this repo does not come from this pipeline.Worth noting separately:
cargo installreached out to crates.io in the middle of an otherwise Nix-pinned build, which is both a supply-chain surface and a source of exactly this kind of drift.Landed, because it turned out to be fixable rather than merely reportable.
cargo-tarpaulinnow comes from the dev shell (flake.nix), and generation must succeed:Verified locally before landing — nixpkgs 25.05 ships cargo-tarpaulin 0.32.5, which works against the pinned toolchain:
What I deliberately left advisory, and why.
fail_ci_if_error: falseon the Codecov upload stays, now with a comment saying so. That step posts to a third-party service; an outage or rate limit at codecov.io is not a defect in this repository and should not block a merge. The assertion that actually matters — that coverage was measured and the report exists — now lives in the step above, where a failure means something about our code. Splitting it this way is what makes the advisory half defensible: nothing is hidden any more, because the thing being tolerated is now only the network hop.Mutation verification — item 4
Control — the old body against the same two failing fixtures: 0 and 0.
CI verdict: all 14 checks pass, and this time they did the work
Run
31977587449—conclusion=success. No badge flips green → red. Every item was measured against the current pipeline before being changed, which is why. The logs show each assertion actually executing rather than passing vacuously:Item 1 — every component reached a real verdict:
Item 2 — the shared script ran, and Nix genuinely re-executed both derivations:
Item 3 — all five binaries present and listed.
Item 4 — the headline. For the first time, this pipeline produced actual coverage numbers:
These are the first real measurements the pipeline has emitted. They are also worth reading on their own terms —
clients/rust-clientat 1.41% is effectively untested, and that fact was previously invisible behind a coverage step that could not fail. Nothing in this PR changes those numbers; it just stops them from being hidden.All new step bodies and
scripts/check-reproducibility.shpassshellcheck -s bash. Nocontinue-on-error, no|| true, no|| echo, no skips were added anywhere; the legitimatepkill … || truecleanup at the end of the integration-tests job is untouched.🤖 Generated with Claude Code