fix: declared env keys against legacy stores, and the eval gate names the failure - #43
Merged
Merged
Conversation
…lares env `legacy_exec_fingerprints` emits four frozen historical key shapes. Two of them — the 0.3.0 and 0.2.x generations — predate `env` joining the exec key and so have no `env` member to carry the digest. Offered to a provider that declares `env`, they recompute identically for every declared value, so a suite re-pointed at a different backend adopts the old backend's answers for as long as the store has pre-0.3.1 ancestors to offer. On a shared tier it is worse than stale: the run writes answers under keys nobody can tell from real ones. Since 0.5.0 folds the env digest into the live key specifically so declared env participates, the probe list quietly reintroduced the collision the live key space was designed to prevent — and only for cells old enough to have a legacy ancestor, which is why it reads as working. Withhold the two shapes when `env_digest` is `Some`. The frozen literals are untouched; only whether they are emitted changes. A provider declaring no `env` is the case they exist for and keeps them, so nothing that was legitimately reachable becomes unreachable. The invariant is now asserted from both ends: a unit test that two declared digests share no probe, and an integration test over the whole lookup set a cold case triggers — the half `a_changed_declared_env_busts` structurally could not reach, since it populates the cache by running and therefore only ever lays down live keys. Closes #40
The eval step's comment claimed "No `set -e`", but GitHub expands
`shell: bash` to `bash --noprofile --norc -e -o pipefail {0}` — the `-e` comes
from the runner, not the script. A non-zero CLI exit therefore aborted the
step at the invocation, so `code=$?` and the `$GITHUB_OUTPUT` block never ran.
Everything downstream degraded from there. The gate read an empty `CODE`, fell
through to its `${CODE:-3}` default, and annotated every failure as
`domarinn infrastructure error (exit 3)` — including a plain regression that
exited 1. The artifact upload, gated on `results-path`, was skipped exactly
when a failing run's JUnit report is worth having, and the PR comment fell back
to its placeholder.
That inverts the CLI's contract at the only place a CI consumer reads it: a
reviewer looking at a real regression is told the harness broke, and the
natural response is to re-run rather than investigate.
Capture with `|| code=$?` and initialise `code=0` for the success path, which
`set -u` requires. The `${CODE:-3}` default stays — a genuinely missing code is
a harness problem; it just can no longer double as the normal path.
Guarded by a new test rather than by reading, since nothing in this repository
runs the action and a break in it is invisible to CI. It lifts the real `run:`
scripts out of action.yml and executes them under the interpreter GitHub uses,
against a stub binary that exits on demand: every documented code must survive
as a step output, and 1 and 3 must not render the same. A third test pins the
assumptions the other two reproduce — the declared shell, and the set of inputs
the step reads — so neither can drift into asserting about a step the runner no
longer runs.
Closes #41
5 tasks
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.
Fixes two silent bugs, both filed with reproductions. Closes #40 and #41.
#40 — a declared
envgave an exec provider no cache-key separationOn a miss the runner probes four frozen historical key shapes. Two of them — the 0.3.0 and 0.2.x generations — predate
envjoining the exec key and so have noenvmember to carry the digest. Offered to a provider that declaresenv, they recompute identically for every declared value, so a suite re-pointed at a different backend adopts the old backend's answers for as long as the store has pre-0.3.1 ancestors. On a shared tier it is worse than stale: the run writes answers under keys nobody can tell from real ones.0.5.0 folds the env digest into the live key specifically so declared env participates; the probe list quietly reintroduced the collision that was designed to prevent — and only for cells old enough to have a legacy ancestor, which is why it reads as working.
Fix: withhold those two shapes when
env_digestisSome. The frozen literals are untouched; only whether they are emitted changes. A provider declaring noenvis the case they exist for and keeps them, so nothing legitimately reachable becomes unreachable.The invariant the issue asks for is asserted from both ends — a unit test that two declared digests share no probe, and an integration test over the whole lookup set a cold case triggers. That second half is what
a_changed_declared_env_bustsstructurally could not reach: it populates the cache by running, so it only ever lays down live keys and a legacy-only collision stays invisible to it.#41 — the eval gate reported exit 3 for every failure
The eval step's comment claimed "No
set -e", but GitHub expandsshell: bashtobash --noprofile --norc -e -o pipefail {0}— the-ecomes from the runner. A non-zero CLI exit aborted the step at the invocation, socode=$?and the$GITHUB_OUTPUTblock never ran. The gate then read an emptyCODE, fell through to${CODE:-3}, and annotated every failure as an infrastructure error; the artifact upload, gated onresults-path, was skipped exactly when a failing run's JUnit report is worth having.That inverts the CLI's contract at the only place a CI consumer reads it: a reviewer looking at a real regression is told the harness broke, and the natural response is to re-run rather than investigate.
Fix:
|| code=$?, withcode=0for the success path thatset -urequires. The${CODE:-3}default stays — a genuinely missing code is a harness problem; it just can no longer double as the normal path.One correction to the issue report: the
[ -n "$X" ] && args+=(…)lines are safe under-e. Bash exempts a non-final command in an&&list, verified by running GitHub's exact interpreter. The bare invocation was the only abort site, so no churn there.Guarding it
Nothing in this repo runs the action — it is published for downstream workflows — so its shell was never executed by CI and a break in it is invisible here. The new
crates/domarinn-cli/tests/action_contract.rslifts the realrun:scripts out ofaction.ymland executes them under the interpreter GitHub uses, against a stub binary that exits on demand:results-pathincluded;1and3produce different gate annotations and different job statuses;Running it under a plain
bash script.shwould pass against the broken action, which is why the runner's invocation is reproduced verbatim.Verification
Both fixes were written test-first and confirmed to fail before the change: the cache tests reported two declared environments resolving to an identical probe key, and the action test reported a stub exiting
1being gated as exit3with noexit-codewritten at all.cargo test --workspace— greencargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all --check— cleanNo manual
CHANGELOG.mdedit; release-please generates it from the commit subjects.