Skip to content

fix(release): the two bugs the v1.0.0 tag found in the release machinery, at their general form - #327

Merged
stellarfeline merged 1 commit into
mainfrom
fix/release-path-first-exercise
Aug 7, 2026
Merged

fix(release): the two bugs the v1.0.0 tag found in the release machinery, at their general form#327
stellarfeline merged 1 commit into
mainfrom
fix/release-path-first-exercise

Conversation

@stellarfeline

Copy link
Copy Markdown
Owner

The v1.0.0 tag ran engine-release.yml for the first time (Actions run
31205291235). Nothing was published and no credential was ever fetched — both
failures 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 of
emitted 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:

build-release-binaries: 'x86_64-pc-windows-msvc' is not in versions.toml [engine].targets

The triple is in versions.toml and is in the matrix. Python's text-mode stdout
writes \r\n on Windows.
The shelf's target list reaches bash through a python3
heredoc that prints it, read back with IFS= read -r — which strips the \n and
keeps the \r. Every target arrived as x86_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 that
reasoned 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 and
that writes to stdout pins its newline
, with one line after the imports:

sys.stdout.reconfigure(newline="\n")

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.yml writes a python3 heredoc straight
    into $GITHUB_OUTPUT
    . A \r there corrupts the content pin for every job that
    consumes it. It has shell: bash and no runner of its own, so it was one
    runs-on: windows-* away from being live.
  • tools/crates-io-publish.sh builds the crates.io sparse-index URL path from a
    heredoc. Same class, on the publish path.

New gate: tools/check-python-shell-newlines.py. Out of scope by rule, never by
allowlist
: python3 script.py (a committed .py is not a shell boundary), programs
with no print( (they answer by exit status), and python inside docker 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:

tools/check-publishable.sh: line 79: .../target/package-log.txt: No such file or directory
  FAIL cargo package failed:
sed: can't read .../target/package-log.txt

cargo package had not failed. cargo package had never run. The runner restored
no cache, so target/ did not exist; cmd >"$LOG" is opened by the shell before
cmd is executed, so the redirect failed, the subshell died, the if took the else
branch, and the else branch then seded the file whose absence was the finding.

The general form is worth more than the instance:

An error path must not depend on an artifact the error may have prevented from
existing.

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:

  • Root cause, repo-wide: tools/check-shell-redirect-dirs.py requires every
    >/>> that writes into a directory to have that directory guaranteed first
    (mkdir -p covering it, mkdir naming it, a mktemp -d, a directory tracked in the
    repo, or an always-present one). Variables are resolved through their literal
    assignments, so hoisting the path into LOG= does not hide it, and > inside a
    quoted string is text, not a redirection.
  • Honest reporting: emit_log distinguishes no log ("the redirect never opened
    it, so cargo package DID NOT RUN") from empty log ("ran and wrote nothing") from
    a 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-build
check. Its $SCRATCH comes from mktemp -d so the directory was already safe, but its
else-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.py doing
sys.stdout.reconfigure(newline="\r\n"), which is precisely what a Windows
interpreter's text-mode stdout does. Crucially the fix overrides the shim (site
initialisation runs first), so the shim reproduces the platform, not the bug.

$ PYTHONPATH=$WINSIM bash tools/build-release-binaries.sh --list-targets | od -c   # before
...   a   p   p   l   e   -   d   a   r   w   i   n  \r  \n   x   8
6   _   6   4   -   p   c   -   w   i   n   d   o   w   s   -   m   s   v   c  \r  \n

$ PYTHONPATH=$WINSIM bash tools/build-release-binaries.sh --target x86_64-pc-windows-msvc
build-release-binaries: 'x86_64-pc-windows-msvc' is not in versions.toml [engine].targets
exit=1

— byte-identical to the CI failure. After the fix, the same two commands give clean
\n and the build proceeds past the membership check into rustup/cargo.

Bug 2. A fresh worktree has no target/, which is exactly the runner's state:

$ bash tools/check-publishable.sh --allow-dirty          # before
== 1. both crates package ==
tools/check-publishable.sh: line 79: .../target/package-log.txt: No such file or directory
  FAIL cargo package failed:
sed: .../target/package-log.txt: No such file or directory

After the fix, cargo package runs and the report is about cargo package.

Both gates, run against the tree as the release ran it (git stash, run, pop):

check-python-shell-newlines: 19 finding(s) across 23 file(s)   → after: OK — 19 programs, all pinned
check-shell-redirect-dirs:    1 finding(s) across 18 scripts   → after: OK — 12 redirects, all guaranteed
    tools/check-publishable.sh:80: redirect into `$ROOT/target`, which nothing in this
    script guarantees exists

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 a
directory 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.py and
tools/tests/test_check_shell_redirect_dirs.py — 29 tests, picked up by the existing
i18n translation tool (pytest) job (it runs all of tools/tests). Both behaviour
and gate are guarded:

  • the real build-release-binaries.sh under the simulated Windows interpreter, asserting
    the 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;
  • the real check-publishable.sh against a shimmed cargo, asserting the report never
    contains a sed: error, names the actual exit status, says "is empty" for a silent
    command and "DID NOT RUN" when the redirect could not be opened;
  • table-driven verdict tests for both gates, including the function-scoped heredoc that
    an invocation-site checker would miss, and mkdir -p a/b/c covering a while plain
    mkdir a does not cover a/b.

Verified red before the fix: against the pre-fix tree, 6 failed, 23 passed — the
four 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, alongside
check-shell-pipe-shortcircuit.py. tools/check-required-contexts.py still 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:

== 1. both crates package ==
   (delvec v1.0.0, delvewright-dsl v0.1.0)
  ok   cargo package -p delvewright-dsl -p delvec

== 2. the manifest crates.io will serve ==
  ok   delvec: 0 dependency `path` keys survive packaging (2 target path(s), which are fine)
  ok   delvec: declares description / license / repository / readme
  ok   delvewright-dsl: 0 dependency `path` keys survive packaging (43 target path(s))
  ok   delvewright-dsl: declares description / license / repository / readme
  ok   delvec depends on delvewright-dsl '=0.1.0' (== versions.toml dsl_crate_req)
  ok   path-only dev-dependency delvewright-grammar is stripped from the packaged manifest

== 3. the packaged tarball builds standing alone ==
  ok   extracted delvec-1.0.0.crate builds `delvec` with no workspace and no path dep
  ok   the standalone binary reports 'delvec 1.0.0, dsl 0.9.0, mc 1.21.11'

check-publishable: 2 crate(s) packaged, 1 standalone build, 1.0.0 / 0.1.0
check-publishable: OK — `cargo install delvec` has everything it needs

No third bug. Both crates package, the packaged delvec tarball builds with no
workspace above it and delvewright-dsl supplied from the packaged DSL tarball, and the
standalone 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 be
uploaded.

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/.

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
@stellarfeline
stellarfeline merged commit 6b1f3da into main Aug 7, 2026
12 checks passed
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.

1 participant