Skip to content

fix(45,49): make the red gates report what they found, then fix what they found #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hyperpolymath merged 2 commits into from
Sep 25, 2026
Merged

fix(45,49): make the red gates report what they found, then fix what they found #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2a4be1
ci(45): stop the new gates dying before they report what they found
hyperpolymath Sep 25, 2026
05a0dbf
fix(49): the three shellcheck findings, in the template rather than t…
hyperpolymath Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 108 additions & 48 deletions .github/workflows/launcher-artefacts.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: MPL-2.0
# Launcher artefact gates — the three checks that belong to THIS repo and
# cannot live in the estate's rust-ci-reusable.yml.
# Launcher artefact gates — the checks that belong to THIS repo and cannot live
# in the estate's rust-ci-reusable.yml.
#
# 1. test-count floor (#45 AC6). `cargo test` exits 0 on a suite that
# collected nothing, so "the job is green" is not evidence any test ran.
Expand All @@ -9,15 +9,27 @@
# `.tera` source is not valid shell, so linting the template file cannot
# work — only a minted artefact can be linted. And a linter that finds
# zero files to lint has failed, not passed, so that case exits non-zero.
# 3. fixture currency (#48 AC5, #49 AC6). The committed deed-dialect fixture
# must be byte-identical to what today's mint emits, so the artefacts the
# scanners read are the artefacts the tool actually produces.
# 3. fixture currency (#48 AC5, #49 AC6). The committed deed fixture must be
# byte-identical to what today's mint emits, so the artefacts the scanners
# read are the artefacts the tool actually produces.
#
# The committed PRE-phase-2 fixture
# (minted-2026-09-22_stapeln-launcher.sh) is deliberately neither linted nor
# currency-checked: it is a frozen historical artefact, the evidence that a
# launcher minted by the old emitter still reads (#40). See
# crates/launcher-common/tests/fixtures/metadata_block/README.adoc.
#
# ⚠ Two conventions every step below follows, both learned the hard way:
#
# * Each `run:` starts with `set +e`. GitHub invokes bash with `-eo pipefail`,
# so a command that is EXPECTED to be able to fail — `cargo fmt --check` on a
# dirty tree, `cargo test` with a failing test, `diff` on drifted files —
# kills the step before it can report what it found. The first version of this
# file died that way and reported only "Process completed with exit code 1".
# Each step handles its own status and ends in an explicit `exit`.
# * Each step sources `$RUNNER_TEMP/annotate.sh`. A failure whose detail lives
# only in the run log is a failure a reviewer cannot act on, and the log is
# not reachable from every environment; annotations are.
name: Launcher artefacts

on:
Expand Down Expand Up @@ -57,46 +69,80 @@ jobs:
uses: dtolnay/rust-toolchain@02cb101ec7c40f2c49e1d9714d64511d8e1b74de # stable
with:
toolchain: stable
components: clippy, rustfmt

- name: Cache cargo registry and build
uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2

- name: Install the annotation helper
shell: bash
run: |
set +e
cat > "$RUNNER_TEMP/annotate.sh" <<'HELPER'
# annotate TITLE FILE [MAXLINES]
# Re-emit the head of FILE as a single check annotation, percent-encoded
# so GitHub renders the line breaks. Titles must not contain "::".
annotate() {
local title="$1" file="$2" max="${3:-60}" body
body=$(awk -v max="$max" '
NR <= max {
gsub(/%/, "%25"); gsub(/\r/, "%0D")
printf "%s%s", sep, $0; sep = "%0A"
}
END { if (NR > max) printf "%s… %d more line(s) in the run log", sep, NR - max }
' "$file")
echo "::error title=${title}::${body}"
}
HELPER
# shellcheck disable=SC1091
. "$RUNNER_TEMP/annotate.sh"
type annotate >/dev/null && echo "✓ annotate helper installed"
exit 0

- name: Report rustfmt drift as an annotation
shell: bash
run: |
set +e
set -uo pipefail
# Diagnostic, not a second gate: `cargo fmt --all -- --check` is gated
# by the estate reusable (rust-ci.yml), and this step never fails the
# job. Its output is a DIFF, and a diff is only actionable where a
# reviewer reads it — the run log is not that place — so it is
# re-emitted as a check annotation.
. "$RUNNER_TEMP/annotate.sh"
# Diagnostic, not a second gate: formatting is gated by the estate
# reusable (rust-ci.yml) and this step never fails the job.
cargo fmt --all -- --check > "$RUNNER_TEMP/fmt.diff" 2>&1
if [ -s "$RUNNER_TEMP/fmt.diff" ]; then
echo "rustfmt wants $(wc -l < "$RUNNER_TEMP/fmt.diff") line(s) changed"
awk 'NR <= 80 { gsub(/%/, "%25"); gsub(/\r/, "%0D"); printf "%s%s", sep, $0; sep = "%0A" }
END { if (NR > 80) printf "%s… %d more line(s) in the run log", sep, NR - 80 }' \
"$RUNNER_TEMP/fmt.diff" > "$RUNNER_TEMP/fmt.ann"
echo "::error title=cargo fmt --check drift::$(cat "$RUNNER_TEMP/fmt.ann")"
echo "rustfmt wants changes ($(wc -l < "$RUNNER_TEMP/fmt.diff") line(s)):"
cat "$RUNNER_TEMP/fmt.diff"
annotate "cargo fmt --check drift" "$RUNNER_TEMP/fmt.diff" 80
else
echo "✓ cargo fmt --all -- --check is clean"
fi
exit 0

- name: Build every target
run: cargo build --locked --all-targets
shell: bash
run: |
set +e
. "$RUNNER_TEMP/annotate.sh"
cargo build --locked --all-targets 2>&1 | tee "$RUNNER_TEMP/build.log"
STATUS="${PIPESTATUS[0]}"
if [ "$STATUS" -ne 0 ]; then
annotate "cargo build --all-targets failed" "$RUNNER_TEMP/build.log" 60
fi
exit "$STATUS"

- name: Run the suite and count what actually ran
shell: bash
run: |
set +e
set -uo pipefail
. "$RUNNER_TEMP/annotate.sh"
LOG="$RUNNER_TEMP/cargo-test.log"
cargo test --locked --all 2>&1 | tee "$LOG"
STATUS="${PIPESTATUS[0]}"
set -e

# Sum the `N passed` of every `test result:` line the run printed —
# one per test binary, so a suite that silently stopped collecting
# in one crate still shows up in the total.
# one line per test binary, so a crate that silently stopped
# collecting shows up as a shortfall in the total rather than as a
# smaller number of lines.
read -r PASSED FAILED < <(awk '
/^test result:/ {
for (i = 1; i <= NF; i++) {
Expand All @@ -112,30 +158,32 @@ jobs:
{
echo "## Launcher artefact gates"
echo ""
echo "- \`cargo test --all\`: **$PASSED passed**, $FAILED failed (floor $TEST_COUNT_FLOOR)"
echo "- cargo test --all: **$PASSED passed**, $FAILED failed (floor $TEST_COUNT_FLOOR)"
} >> "$GITHUB_STEP_SUMMARY"

if [ "$STATUS" -ne 0 ]; then
sed -n '/^failures:$/,$p' "$LOG" | head -100 > "$RUNNER_TEMP/failures.txt"
awk '{ gsub(/%/, "%25"); gsub(/\r/, "%0D"); printf "%s%s", sep, $0; sep = "%0A" }' \
"$RUNNER_TEMP/failures.txt" > "$RUNNER_TEMP/failures.ann"
echo "::error title=cargo test exited $STATUS::$(cat "$RUNNER_TEMP/failures.ann")"
exit 1
sed -n '/^failures:$/,$p' "$LOG" | head -120 > "$RUNNER_TEMP/failures.txt"
if [ ! -s "$RUNNER_TEMP/failures.txt" ]; then
tail -60 "$LOG" > "$RUNNER_TEMP/failures.txt"
fi
annotate "cargo test failed" "$RUNNER_TEMP/failures.txt" 100
exit "$STATUS"
fi
if [ "$FAILED" -ne 0 ]; then
echo "::error::$FAILED test(s) failed"
exit 1
fi
if [ "$PASSED" -lt "$TEST_COUNT_FLOOR" ]; then
echo "::error::only $PASSED tests ran, floor is $TEST_COUNT_FLOOR — a suite that \
collects nothing exits 0, so this gate is what makes 'green' mean 'ran' (#45 AC6)"
echo "::error title=test-count floor::only $PASSED tests ran; the floor is $TEST_COUNT_FLOOR. A suite that collects nothing exits 0, so this gate is what makes green mean ran (#45 AC6)."
exit 1
fi
exit 0

- name: Mint a launcher from the committed fixture config
shell: bash
run: |
set -euo pipefail
set +e
set -uo pipefail
OUT="$RUNNER_TEMP/minted"
mkdir -p "$OUT"
# `--stdout` so nothing is written next to the fixture config.
Expand All @@ -145,30 +193,40 @@ jobs:
./target/debug/launch-scaffolder mint "$FIXTURE_CONFIG" --stdout \
| sed -E 's|^CONFIG_FILE=.*|CONFIG_FILE=""|' \
> "$OUT/stapeln-launcher.sh"
STATUS="${PIPESTATUS[0]}"
if [ "$STATUS" -ne 0 ]; then
echo "::error title=mint failed::launch-scaffolder mint exited $STATUS"
exit "$STATUS"
fi
chmod +x "$OUT/stapeln-launcher.sh"
echo "minted $(wc -l < "$OUT/stapeln-launcher.sh") lines to $OUT/stapeln-launcher.sh"
echo "minted $(wc -l < "$OUT/stapeln-launcher.sh") lines"
exit 0

- name: Committed fixture is byte-identical to a fresh mint
shell: bash
run: |
set -euo pipefail
set +e
set -uo pipefail
. "$RUNNER_TEMP/annotate.sh"
MINTED="$RUNNER_TEMP/minted/stapeln-launcher.sh"
if diff -u "$FIXTURE_SCRIPT" "$MINTED" > "$RUNNER_TEMP/fixture.diff"; then
diff -u "$FIXTURE_SCRIPT" "$MINTED" > "$RUNNER_TEMP/fixture.diff"
STATUS=$?
if [ "$STATUS" -eq 0 ]; then
echo "✓ $FIXTURE_SCRIPT is byte-identical to a fresh mint"
else
cat "$RUNNER_TEMP/fixture.diff"
awk 'NR <= 80 { gsub(/%/, "%25"); gsub(/\r/, "%0D"); printf "%s%s", sep, $0; sep = "%0A" }' \
"$RUNNER_TEMP/fixture.diff" > "$RUNNER_TEMP/fixture.ann"
echo "::error title=$FIXTURE_SCRIPT has drifted from what mint emits today::$(cat "$RUNNER_TEMP/fixture.ann")"
echo "::notice::re-mint it rather than editing it by hand — see \
crates/launcher-common/tests/fixtures/metadata_block/README.adoc"
exit 1
exit 0
fi
cat "$RUNNER_TEMP/fixture.diff"
annotate "committed fixture drifted from what mint emits today" \
"$RUNNER_TEMP/fixture.diff" 80
echo "::notice::re-mint it rather than editing it by hand — see crates/launcher-common/tests/fixtures/metadata_block/README.adoc"
exit 1

- name: Shellcheck the minted launcher
shell: bash
run: |
set -euo pipefail
set +e
set -uo pipefail
. "$RUNNER_TEMP/annotate.sh"
command -v shellcheck >/dev/null || {
echo "::error::shellcheck is not installed on the runner"; exit 1; }
shellcheck --version | sed -n '2,3p'
Expand All @@ -177,29 +235,30 @@ jobs:

# #49 AC5: zero files is a failure, not a pass.
if [ "${#FILES[@]}" -eq 0 ]; then
echo "::error::the shellcheck gate found ZERO files to lint — a linter \
with nothing to lint has failed, not passed"
echo "::error title=nothing to lint::the shellcheck gate found ZERO files to lint — a linter with nothing to lint has failed, not passed"
exit 1
fi
printf 'linting %d minted artefact(s):\n' "${#FILES[@]}"
printf ' %s\n' "${FILES[@]}"

# Hard gate: nothing at warning or above (#49 AC2).
if ! shellcheck --format=gcc --severity=warning "${FILES[@]}" > "$RUNNER_TEMP/sc-warning.txt"; then
shellcheck --format=gcc --severity=warning "${FILES[@]}" \
> "$RUNNER_TEMP/sc-warning.txt" 2>&1
if [ -s "$RUNNER_TEMP/sc-warning.txt" ]; then
cat "$RUNNER_TEMP/sc-warning.txt"
echo "::error::shellcheck found warning-or-above findings in a freshly \
minted launcher; the fix belongs in templates/launcher.sh.tera, never in \
the committed fixtures"
annotate "shellcheck findings in a freshly minted launcher" \
"$RUNNER_TEMP/sc-warning.txt" 60
echo "::notice::the fix belongs in templates/launcher.sh.tera, never in the committed fixtures"
exit 1
fi

# Pinned gate: the three codes #49 fixed must stay fixed at ANY
# severity, so a reclassification cannot hide a regression.
shellcheck --format=gcc "${FILES[@]}" > "$RUNNER_TEMP/sc-all.txt" || true
shellcheck --format=gcc "${FILES[@]}" > "$RUNNER_TEMP/sc-all.txt" 2>&1
for code in $BANNED_SHELLCHECK_CODES; do
if grep -q "\[$code\]" "$RUNNER_TEMP/sc-all.txt"; then
grep "\[$code\]" "$RUNNER_TEMP/sc-all.txt"
echo "::error::$code is back in a freshly minted launcher (#49)"
echo "::error title=$code is back::a freshly minted launcher trips $code again (#49)"
exit 1
fi
done
Expand All @@ -209,3 +268,4 @@ jobs:
echo "--- remaining sub-warning notes, for the record ---"
cat "$RUNNER_TEMP/sc-all.txt"
fi
exit 0
63 changes: 62 additions & 1 deletion crates/launcher-common/src/template.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,10 @@ pub fn render(
"app_version",
config.project.version.as_deref().unwrap_or("1.0.0"),
);
ctx.insert("app_license", config.project.license.as_deref().unwrap_or("MPL-2.0"));
ctx.insert(
"app_license",
config.project.license.as_deref().unwrap_or("MPL-2.0"),
);

// --- [repo] --------------------------------------------------------
ctx.insert("repo_dir", &config.repo.path);
Expand All @@ -88,13 +91,35 @@ pub fn render(
ctx.insert("has_url", &(config.runtime.url.is_some()));

// Default URL/port fallbacks so Tera `{{ url }}` never explodes.
//
// The port gets ONE answer in the generated script. With no explicit
// `[runtime].url` the template emits `APP_PORT` and composes the URL from
// it; with one, it emits the URL and no `APP_PORT` at all, because a second
// spelling of the port could only disagree with the URL the launcher
// actually dials (#49 AC3 — shellcheck saw the unused variable, the defect
// was the duplicate).
let port = config.runtime.port.unwrap_or(0);
ctx.insert("app_port", &port);
let url_string = match (&config.runtime.url, config.runtime.port) {
(Some(u), _) => u.clone(),
(None, Some(p)) => format!("http://localhost:{p}"),
(None, None) => String::new(),
};
// A config that sets both and has them disagree is almost certainly a
// mistake, and the generated script no longer carries the second value that
// would once have shown it — so say so at mint time instead.
let disagreement = config
.runtime
.url
.as_deref()
.zip(config.runtime.port)
.filter(|(url, p)| !url.contains(&format!(":{p}")));
if let Some((url, p)) = disagreement {
tracing::warn!(
"[runtime].url = {url} does not carry [runtime].port = {p}; the URL wins, so \
the port is not emitted into the launcher separately"
);
}
ctx.insert("url", &url_string);

// PID / log file defaults follow the standard's pattern when unset.
Expand Down Expand Up @@ -470,4 +495,40 @@ mod tests {
"shebang must be on line 1 of the rendered launcher"
);
}

/// The port has exactly one spelling in the generated script (#49 AC3).
///
/// `APP_PORT` used to be emitted into every `server-url` launcher and read
/// by none of them — shellcheck SC2034 — while the `URL` line above it
/// hardcoded the same port. The unused variable was the symptom; two answers
/// to one question was the defect, and the two could silently disagree.
#[test]
fn the_port_has_exactly_one_spelling_in_the_generated_script() {
let std_ = LauncherStandard::baked().expect("baked standard should parse");

// No explicit URL, so APP_PORT is emitted — because something reads it:
// the very next line composes the URL out of it.
let composed = stapeln_config();
assert_eq!(composed.runtime.url, None, "fixture config sets no url");
let script = render(&composed, &std_, None).expect("renders");
assert!(
script.contains("APP_PORT=\"4010\""),
"the composed arm must state the port it composes from"
);
assert!(
script.contains("URL=\"http://localhost:${APP_PORT}\""),
"and must compose the URL from it, so the two cannot disagree"
);

// An explicit URL is the whole answer: the port is inside it, and a
// second `APP_PORT=` beside it could only contradict it.
let mut explicit = stapeln_config();
explicit.runtime.url = Some("http://localhost:4010".into());
let script = render(&explicit, &std_, None).expect("renders");
assert!(
!script.contains("APP_PORT="),
"a launcher with an explicit [runtime].url must not also state the port"
);
assert!(script.contains("URL=\"http://localhost:4010\""));
}
}
Loading
Loading