fix(release): the two bugs the v1.0.0 tag found in the release machinery, at their general form - #327
Merged
Merged
Conversation
The first-ever run of engine-release.yml (Actions 31205291235) failed twice upstream of the credentialled job — nothing was published, no credential was fetched — but both failures are real bugs in the release machinery, on the one path the eleven green checks on #318 could not exercise. 1. Python's text-mode stdout writes `\r\n` on Windows, so every value a `python3` heredoc handed to bash arrived with a trailing `\r`. The shelf rejected `x86_64-pc-windows-msvc` as "not in versions.toml [engine].targets" on the msvc runner and only there. The general form is not "targets" and not "captured invocations" either: the site that broke is a heredoc inside a shell FUNCTION captured at three call sites, so the rule is that EVERY inline python writing to stdout pins `newline="\n"` where the value is produced. 19 such sites found and fixed, including one writing straight into `$GITHUB_OUTPUT` and one building the crates.io index URL. New gate: tools/check-python-shell-newlines.py. 2. `cmd >"$LOG"` is opened by the shell before `cmd` runs, so on a runner with no build cache the redirect failed, `cargo package` never ran, and the else branch `sed`ed the log whose absence was the finding — reporting "cargo package failed" about a command that had not been executed. The general form: an error path must not depend on an artifact the error may have prevented from existing. Root cause removed repo-wide by tools/check-shell-redirect-dirs.py; the failure branch now names a missing or empty log instead of quoting one it does not have. Both reds reproduced locally before the fix — Windows simulated faithfully via a sitecustomize that reconfigures stdout to CRLF, which the fix overrides — and guarded by 29 tests in the existing tools/tests suite (6 red pre-fix). Both gates are steps in an existing CI job, so no new required status context. Nothing in crates/ is touched: emitted datapack output is byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjQ5p1Kv5MrkGPumi7yXWL
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.
The
v1.0.0tag ranengine-release.ymlfor the first time (Actions run31205291235). Nothing was published and no credential was ever fetched — bothfailures are upstream of the credentialled job, which is the design working. But the
run found two real bugs in the release machinery, on the one path the eleven green
checks on #318 could not exercise: the release path itself.
Both are fixed at their general form. Neither touches
crates/, so no byte ofemitted datapack moves; this is CI/release tooling only.
Bug 1 — a value that compared unequal to itself, on one runner and only one
shelf (x86_64-pc-windows-msvc)failed, and only that target of five:The triple is in
versions.tomland is in the matrix. Python's text-mode stdoutwrites
\r\non Windows. The shelf's target list reaches bash through apython3heredoc that
prints it, read back withIFS= read -r— which strips the\nandkeeps the
\r. Every target arrived asx86_64-pc-windows-msvc\r, so[ "$k" = "$t" ]was false forever. The four unix targets were green, which is exactly why nobody saw it.
The general form is not "targets", and it is not "captured invocations" either. The
site that broke is a heredoc inside a shell function with no redirect, no pipe and no
$(anywhere near it — the capture happens at three separate call sites. A checker thatreasoned about the invocation would have passed the one bug it exists to catch. So the
rule is: every inline python a repo shell script or workflow
run:block executes andthat writes to stdout pins its newline, with one line after the imports:
That costs nothing on a stream nobody reads, and it fixes the value where it is
produced rather than at each of N consumers.
The survey found 19 such sites across
tools/,validation/,.github/workflows/and
.github/actions/— all now pinned. Two beyond the reported one are worth naming:.github/actions/checkout-content/action.ymlwrites apython3heredoc straightinto
$GITHUB_OUTPUT. A\rthere corrupts the content pin for every job thatconsumes it. It has
shell: bashand no runner of its own, so it was oneruns-on: windows-*away from being live.tools/crates-io-publish.shbuilds the crates.io sparse-index URL path from aheredoc. Same class, on the publish path.
New gate:
tools/check-python-shell-newlines.py. Out of scope by rule, never byallowlist:
python3 script.py(a committed.pyis not a shell boundary), programswith no
print((they answer by exit status), and python insidedocker run/docker exec(a pinned Linux image by construction).Bug 2 — a failure report about a command that never ran
crates.io preflight (no credential)failed with:cargo packagehad not failed.cargo packagehad never run. The runner restoredno cache, so
target/did not exist;cmd >"$LOG"is opened by the shell beforecmdis executed, so the redirect failed, the subshell died, theiftook the elsebranch, and the else branch then
seded the file whose absence was the finding.The general form is worth more than the instance:
The report was not merely unhelpful — it was wrong about what happened, and it named
an innocent command, which sends the next reader to
cargo.Both halves are fixed, and neither substitutes for the other:
tools/check-shell-redirect-dirs.pyrequires every>/>>that writes into a directory to have that directory guaranteed first(
mkdir -pcovering it,mkdirnaming it, amktemp -d, a directory tracked in therepo, or an always-present one). Variables are resolved through their literal
assignments, so hoisting the path into
LOG=does not hide it, and>inside aquoted string is text, not a redirection.
emit_logdistinguishes no log ("the redirect never openedit, so
cargo packageDID NOT RUN") from empty log ("ran and wrote nothing") froma log it can actually quote, and the report now names the real exit status.
Same shape elsewhere: exactly one more,
check-publishable.sh's standalone-buildcheck. Its
$SCRATCHcomes frommktemp -dso the directory was already safe, but itselse-branch had the same dishonest read; it got the same treatment.
The reds I watched happen
Bug 1. A Windows runner is not available locally, so the platform is simulated
faithfully rather than approximated: a
sitecustomize.pydoingsys.stdout.reconfigure(newline="\r\n"), which is precisely what a Windowsinterpreter's text-mode stdout does. Crucially the fix overrides the shim (site
initialisation runs first), so the shim reproduces the platform, not the bug.
— byte-identical to the CI failure. After the fix, the same two commands give clean
\nand the build proceeds past the membership check intorustup/cargo.Bug 2. A fresh worktree has no
target/, which is exactly the runner's state:After the fix,
cargo packageruns and the report is aboutcargo package.Both gates, run against the tree as the release ran it (
git stash, run, pop):The redirect gate finds exactly one thing in the whole repo, and it is the bug. Its
first draft reported eight more; every one was a
>inside a quoted string or adirectory that exists by construction, so the scanner was made quote-aware and taught
about repo-tracked directories and image-provided roots — a gate with false positives is
a gate that stops being read.
Regression guards (checks, not comments)
tools/tests/test_check_python_shell_newlines.pyandtools/tests/test_check_shell_redirect_dirs.py— 29 tests, picked up by the existingi18n translation tool (pytest)job (it runs all oftools/tests). Both behaviourand gate are guarded:
build-release-binaries.shunder the simulated Windows interpreter, assertingthe target list is byte-identical to the LF run — plus a check that the shim really
emits CRLF, so the test cannot be vacuous, and that a bogus triple still errors, so the
membership check it protects is still live;
check-publishable.shagainst a shimmedcargo, asserting the report nevercontains a
sed:error, names the actual exit status, says "is empty" for a silentcommand and "DID NOT RUN" when the redirect could not be opened;
an invocation-site checker would miss, and
mkdir -p a/b/ccoveringawhile plainmkdir adoes not covera/b.Verified red before the fix: against the pre-fix tree,
6 failed, 23 passed— thefour functional tests reproduce the original wrong messages verbatim. After:
29 passed.No new CI job, so no new required status context and no branch-protection dance: both
gates are steps in the existing
docs (local link check)job, alongsidecheck-shell-pipe-shortcircuit.py.tools/check-required-contexts.pystill passes.Obligation 3 — do the two crates actually package?
That step never executed in CI, so it was still unknown. Run locally against the real
tree,
bash tools/check-publishable.sh --allow-dirty:No third bug. Both crates package, the packaged
delvectarball builds with noworkspace above it and
delvewright-dslsupplied from the packaged DSL tarball, and thestandalone binary reports the version the release claims.
tools/crates-io-publish.sh --plan(no credential) agrees: both crates are absent from the index and would beuploaded.
Also verified locally
All 11 repo lint gates,
validation/check-versions.sh(two of whose heredocs I edited)and
tools/crates-io-publish.sh --plan(three edited) all pass.Out of scope, untouched
Re-tagging or re-running the release; the release workflow's approval/environment design
(ADR-0017 §4 — the credential path is not touched); anything in
crates/.