From f39a0c6366e15d74b59e596a9bea61d16356aff9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 26 Oct 2026 10:10:37 +0000 Subject: [PATCH] chore(template-sync): sync to v1.6.1 --- .claude/README.md | 5 +- .github/scripts/README.md | 30 ++++- .github/scripts/assemble_dist.py | 45 +++++-- .github/scripts/find_moderation_result.sh | 3 +- .github/scripts/ruff.toml | 3 +- .github/scripts/template_sync.py | 48 ++++++-- .github/scripts/test_moderation_scripts.py | 4 + .github/scripts/test_release_scripts.py | 134 +++++++++++++++++++++ .github/scripts/test_script_syntax.py | 24 ++++ .github/scripts/test_template_sync.py | 110 +++++++++++++++-- .github/scripts/verify_release_tag.sh | 22 +++- .github/template-sync-state.json | 2 +- .github/workflows/template-sync.yml | 14 ++- docs/TEMPLATE.md | 39 +++++- scripts/README.md | 5 +- scripts/develop.sh | 18 ++- 16 files changed, 459 insertions(+), 47 deletions(-) diff --git a/.claude/README.md b/.claude/README.md index 2e06f4b..7010d33 100644 --- a/.claude/README.md +++ b/.claude/README.md @@ -58,7 +58,10 @@ this directory). Base lists each of these skills one by one in its sync manifest, so an instance adds its own as `skills//` and lists that directory (as `.claude/skills//`, or `.claude/agents/.md` for an - agent) under `ignore` in `.github/template-sync-manifest.local.yml`. That + agent) under `ignore` in `.github/template-sync-manifest.local.yml` + (base's own entries are `replace` tier, one line per skill and agent; a + deleted one keeps its line as a tombstone so sync removes it from + instances, see `../docs/TEMPLATE.md`, "Instance extension points"). That entry is what keeps the completeness check passing; sync never reads the local file. Don't reuse a base (or removed base) skill or agent name, since sync would overwrite or remove it. diff --git a/.github/scripts/README.md b/.github/scripts/README.md index 76fede9..3f3deab 100644 --- a/.github/scripts/README.md +++ b/.github/scripts/README.md @@ -21,8 +21,13 @@ `../template-sync-state.json`. Its `check-apply` subcommand cross-checks `apply`'s exit status against the summary it wrote, so a crash that exits 1 is not read as merge conflicts. A merge-tier file the instance - never changed takes the new version outright; a binary one the instance - did change is reported as a conflict and left as it is. It catches the broad `apply` failure with + never changed takes the new version outright (content and executable bit, + keeping its other mode bits); a merged file keeps its own mode. A binary + one the instance did change is reported as a conflict and left as it is; + since it has no conflict markers, it is also listed in the summary's + `merge.binary`, labelled in the PR body and annotated with `::warning::` in + the job log. The workflow's apply step writes only `check-apply`'s validated + `conflicts=0|1` line to `$GITHUB_OUTPUT`. `apply` catches its broad failure with `logging.exception` rather than a `noqa`, so instances that don't select ruff's `BLE` rules see no unused-`noqa` finding. `apply` is not atomic: on exit 2, files written before the error stay on @@ -51,7 +56,9 @@ `.` member cannot matter: the `data` extraction filter sets a directory's mode to `None`, so no member restricts the staging directory mid-run (a test pins this). A failed run, including a collision found in a later leg, - discards the staging directory and leaves `dist/` untouched. + discards the staging directory and leaves `dist/` untouched. A corrupt or + truncated tarball (including a cut-off compressed one) is an `::error::` + naming the file, not a traceback. `has_make_target.sh` runs no recipe, but make still evaluates `$(shell ...)` and `include` while parsing the Makefile. `test_release_scripts.py` tests both, `verify_release_tag.sh`, and that @@ -74,20 +81,31 @@ supersedes the instance-level `select ALL` remarks above for these files. `test_script_syntax.py` parses every script with `ast.parse(feature_version=(3, 12))` and checks `ruff.toml` names the same - version. Bump both together if the runner's `python3` changes. + version. The runner image's `python3` is not read at test time (that would + need the network), so `test_script_syntax.py` also maps each runner label + the workflows default to (`ubuntu-24.04`) to its Python: when Renovate bumps + that label the test fails until you look up the new image's `python3` + (actions/runner-images) and update `RUNNER_IMAGE_PYTHON`, `RUNNER_PYTHON` and + `ruff.toml` together. - `verify_release_tag.sh` — the `release` job's check that an already published release's tag points at the commit the run built (fails closed; retries only transient lookup errors, six attempts over about two minutes, and never a missing tag or a missing token, which it recognises by `gh`'s own "set/populate the GH_TOKEN environment variable" message, not - by any stderr that names `GH_TOKEN`). Tested against a fake `gh` in + by any stderr that names `GH_TOKEN`). `TAG_LOOKUP_ATTEMPTS` (1-99) and `TAG_LOOKUP_DELAY` (0-9999, no leading zeros) are validated up + front, every attempt's stderr is logged labelled with its attempt number, + and a 404 is not retried even on a just-created tag: GitHub documents no + read-after-write lag for refs, so the error says to re-run once the tag is + visible. Tested against a fake `gh` in `test_release_scripts.py`. - `find_moderation_result.sh` / `test_moderation_scripts.py` — the check step shared by `../workflows/moderate-bug-fix-apply.yml` and `../workflows/moderate-feature-build-apply.yml`: asks the API for the `moderation-result` artifact by name (so it is found however many artifacts the run has) and writes `found=true|false`. It fails on an - expired artifact (a produced result that can no longer be applied), an + expired artifact (a produced result that can no longer be applied; the + message says to re-run the worker run, since re-running the apply run + cannot bring the artifact back), an API error, and an empty or unrecognised response. The tests run it against a fake `gh`. - `run_claude.sh` — invokes `claude -p` for one issue-moderation stage diff --git a/.github/scripts/assemble_dist.py b/.github/scripts/assemble_dist.py index 4e71afd..acc94c9 100755 --- a/.github/scripts/assemble_dist.py +++ b/.github/scripts/assemble_dist.py @@ -23,12 +23,15 @@ from __future__ import annotations +import gzip import json +import lzma import re import shutil import sys import tarfile import tempfile +import zlib from pathlib import Path, PurePosixPath from typing import TYPE_CHECKING @@ -39,6 +42,10 @@ _ARCH = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]*") +# What reading a corrupt or truncated (compressed) tarball raises . +_CORRUPT = (tarfile.TarError, EOFError, zlib.error, lzma.LZMAError, gzip.BadGzipFile) + + class AssembleError(Exception): """ARCHES_JSON is invalid, a tarball is missing, or two legs collide on a path.""" @@ -150,18 +157,32 @@ def assemble(arches: list[str], parts: Path, dist: Path) -> None: stage = Path(tempfile.mkdtemp(prefix=f".{dist.name}.", dir=dist.parent)) try: for arch, tarball in zip(names, tarballs, strict=True): - with tarfile.open(tarball) as tar: - # The "data" filter refuses unsafe links and absolute paths. - try: - tar.extractall( - stage, members=_checked_members(tar, arch, leaves, dirs), filter="data" - ) - except tarfile.FilterError as exc: - msg = ( - f"{tarball.name}: {exc}; release assets may only contain regular " - "files, directories and links that stay inside dist/" - ) - raise AssembleError(msg) from exc + try: + with tarfile.open(tarball) as tar: + # The "data" filter refuses unsafe links and absolute paths. + try: + tar.extractall( + stage, members=_checked_members(tar, arch, leaves, dirs), filter="data" + ) + except tarfile.FilterError as exc: + msg = ( + f"{tarball.name}: {exc}; release assets may only contain regular " + "files, directories and links that stay inside dist/" + ) + raise AssembleError(msg) from exc + except _CORRUPT as exc: + # The collision check reads the tarball lazily, so a corrupt or + # truncated one surfaces here, deep inside extraction. + msg = ( + f"could not read {tarball.name} ({type(exc).__name__}: {exc}); " + "it may be corrupt or truncated" + ) + raise AssembleError(msg) from exc + except OSError as exc: + # bz2 corruption is a bare OSError; so is a full disk. Name the file + # without claiming which. + msg = f"{tarball.name}: {exc}" + raise AssembleError(msg) from exc # mkdtemp creates 0700, and dist/ must be readable to the other user the # devcontainer runs as. The "data" filter sets every directory's mode to # None, so a "." member (`tar -C dist -cf x.tar .`) never restricts the diff --git a/.github/scripts/find_moderation_result.sh b/.github/scripts/find_moderation_result.sh index 3dffd0b..2e9ff05 100755 --- a/.github/scripts/find_moderation_result.sh +++ b/.github/scripts/find_moderation_result.sh @@ -24,7 +24,8 @@ state=$(gh api "repos/${repo}/actions/runs/${run_id}/artifacts?name=moderation-r case "$state" in found) found=true ;; expired) - echo "::error::The moderation-result artifact of run ${run_id} has expired before it was applied; re-run the worker workflow." >&2 + run_url="${GITHUB_SERVER_URL:-https://github.com}/${repo}/actions/runs/${run_id}" + echo "::error::The moderation-result artifact of run ${run_id} expired before it was applied, so this result is lost and nothing was changed. Re-running this apply run cannot bring it back. To redo it, re-run the worker run (${run_url}) with Re-run all jobs; its fresh artifact triggers a new apply run." >&2 exit 1 ;; missing) diff --git a/.github/scripts/ruff.toml b/.github/scripts/ruff.toml index 92e96e4..dc0a302 100644 --- a/.github/scripts/ruff.toml +++ b/.github/scripts/ruff.toml @@ -15,7 +15,8 @@ # `.github/scripts/` entry in ../template-sync-manifest.yml). # # Keep `target-version` equal to the runner's `python3` minor version. -# test_script_syntax.py parses every script against that version. +# test_script_syntax.py parses every script against that version and fails +# when the workflows' default runner label changes, so this is not forgotten. target-version = "py312" line-length = 100 diff --git a/.github/scripts/template_sync.py b/.github/scripts/template_sync.py index 8d8f1c9..c7d132c 100644 --- a/.github/scripts/template_sync.py +++ b/.github/scripts/template_sync.py @@ -304,9 +304,15 @@ def apply_merge( ``merged`` lists only files whose content actually changed, so running the same step again (an instance already at the new version) reports nothing. A path the instance links elsewhere is left alone, as in the replace tier. + ``binary`` lists the conflicts that are binary files (a subset of + ``conflicts``): they get no conflict markers, so nothing in the tree shows + them. A merged existing file keeps its own mode; one the instance never + changed takes the new version's content and executable bit, as in the + replace tier, and keeps its other mode bits. """ merged: list[str] = [] conflicts: list[str] = [] + binary: list[str] = [] for path in sorted(paths): new_file = new_root / path old_file = old_root / path @@ -320,6 +326,8 @@ def apply_merge( shutil.copy2(new_file, instance_file) merged.append(path) continue + # Probed up front, the same way merge_file decides to decline a binary file. + binary_input = any(is_binary(p) for p in (instance_file, old_file, new_file)) base_content = old_file.read_bytes() if old_file.exists() else b"" new_content = new_file.read_bytes() if new_content == base_content: @@ -330,7 +338,11 @@ def apply_merge( if old_file.exists() and before == base_content: # Untouched by the instance: take the new version outright. Git # would do the same for text; a binary file can only be merged this way. - shutil.copy2(new_file, instance_file) + # Content and the executable bit, as in the replace tier, but not the + # template's other mode bits (copy2 would overwrite them). + shutil.copyfile(new_file, instance_file) + if is_executable(new_file) != is_executable(instance_file): + sync_executable_bit(new_file, instance_file) merged.append(path) continue if old_file.exists(): @@ -342,9 +354,17 @@ def apply_merge( conflicted = merge_file(instance_file, Path(empty_base.name), new_file) if conflicted: conflicts.append(path) + if binary_input: + binary.append(path) elif instance_file.read_bytes() != before: merged.append(path) - return {"merged": merged, "conflicts": conflicts} + return {"merged": merged, "conflicts": conflicts, "binary": binary} + + +def _annotation_escape(value: str, *, prop: bool = False) -> str: + """Percent-encode ``value`` for a GitHub workflow command (property or message).""" + value = value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + return value.replace(":", "%3A").replace(",", "%2C") if prop else value def cmd_apply(args: argparse.Namespace) -> int: @@ -377,6 +397,13 @@ def cmd_apply(args: argparse.Namespace) -> int: log.exception("apply failed") print(f"error: {type(err).__name__}: {err}", file=sys.stderr) return EXIT_ERROR + for path in summary["merge"].get("binary", []): + # No markers in a binary file, so say it where the job log is read. + print( + f"::warning file={_annotation_escape(path, prop=True)}::binary merge conflict, " + "left at the instance's version; take the template's new version by hand", + file=sys.stderr, + ) print(json.dumps(summary, indent=2)) return EXIT_CONFLICTS if summary["merge"]["conflicts"] else 0 @@ -400,7 +427,8 @@ def cmd_check_apply(args: argparse.Namespace) -> int: agrees = isinstance(conflicts, list) and bool(conflicts) == ( args.exit_code == EXIT_CONFLICTS ) - except (OSError, ValueError, KeyError, TypeError): + except (OSError, ValueError, KeyError, TypeError, RecursionError): + # RecursionError: json.loads on a pathologically nested file. agrees = False if not agrees: print( @@ -422,14 +450,20 @@ def pr_body(summary: dict[str, dict[str, list[str]]], old_tag: str, new_tag: str lines.append(f"- replace (removed upstream): `{path}`") for path in summary["merge"]["merged"]: lines.append(f"- merge (clean): `{path}`") + binary = set(summary["merge"].get("binary", [])) for path in summary["merge"]["conflicts"]: - lines.append(f"- merge (**conflict, needs manual resolution**): `{path}`") + if path in binary: + lines.append( + f"- merge (**binary conflict, no markers; kept at this repo's version, " + f"take the new one from the template's new tag by hand**): `{path}`" + ) + else: + lines.append(f"- merge (**conflict, needs manual resolution**): `{path}`") if summary["merge"]["conflicts"]: lines += [ "", - "One or more files above have `<<<<<<<` conflict markers to resolve before merging " - "(a binary file has none: it is left at the instance's version, so take the new " - "one from the template's new tag).", + "Resolve every conflict above before merging: text files have `<<<<<<<` markers; " + "a binary file has none (it is left at the instance's version).", ] if "pyproject.toml" in summary["merge"]["merged"] + summary["merge"]["conflicts"]: lines += [ diff --git a/.github/scripts/test_moderation_scripts.py b/.github/scripts/test_moderation_scripts.py index b47a1f3..a41744d 100644 --- a/.github/scripts/test_moderation_scripts.py +++ b/.github/scripts/test_moderation_scripts.py @@ -89,6 +89,10 @@ def test_expired_artifact_fails_the_script(self) -> None: self.assertIn("::error::", result.stderr) self.assertIn("expired", result.stderr) self.assertFalse((self.tmp / "output.txt").exists()) + # Actionable: names what to re-run, and that re-running the apply run won't help. + self.assertIn("Re-run all jobs", result.stderr) + self.assertIn("/actions/runs/", result.stderr) + self.assertIn("cannot bring it back", result.stderr) def test_live_artifact_wins_over_an_expired_one(self) -> None: """A re-run's fresh artifact is found even if an older one expired.""" diff --git a/.github/scripts/test_release_scripts.py b/.github/scripts/test_release_scripts.py index d3f2843..56bd045 100644 --- a/.github/scripts/test_release_scripts.py +++ b/.github/scripts/test_release_scripts.py @@ -2,6 +2,7 @@ from __future__ import annotations +import contextlib import io import itertools import os @@ -149,6 +150,40 @@ def test_corrupt_tarball_is_reported(self) -> None: (self.parts / "assets-amd64.tar").write_bytes(b"not a tar") self.assertEqual(assemble_dist.main(["x", '["amd64"]', str(self.parts), str(self.dist)]), 1) + def test_truncated_compressed_tarball_names_the_file(self) -> None: + """A truncated gzip tarball is an ``::error::`` with its file name, not a traceback.""" + # Regression: EOFError from the lazy read during extraction escaped main(). + src = self.parts / "src" + src.mkdir() + (src / "f.txt").write_bytes(os.urandom(50_000).hex().encode()) + full = self.parts / "full.tgz" + with tarfile.open(full, "w:gz") as tar: + tar.add(src / "f.txt", arcname="f.txt") + (self.parts / "assets-amd64.tar").write_bytes(full.read_bytes()[:300]) + out = io.StringIO() + with contextlib.redirect_stdout(out): + status = assemble_dist.main(["x", '["amd64"]', str(self.parts), str(self.dist)]) + self.assertEqual(status, 1) + self.assertIn("::error::could not read assets-amd64.tar", out.getvalue()) + self.assertFalse(self.dist.exists()) + + def test_corrupt_bz2_tarball_names_the_file(self) -> None: + """bz2 corruption mid-stream is reported with the file name, whatever it raises.""" + src = self.parts / "f.txt" + src.write_bytes(os.urandom(2_000_000).hex().encode()) # several bz2 blocks + full = self.parts / "full.tbz" + with tarfile.open(full, "w:bz2") as tar: + tar.add(src, arcname="f.txt") + data = bytearray(full.read_bytes()) + data[-3000:-2980] = b"\xff" * 20 + (self.parts / "assets-amd64.tar").write_bytes(bytes(data)) + out = io.StringIO() + with contextlib.redirect_stdout(out): + status = assemble_dist.main(["x", '["amd64"]', str(self.parts), str(self.dist)]) + self.assertEqual(status, 1) + # EOFError, OSError or ReadError, depending on where the damage lands. + self.assertRegex(out.getvalue(), r"^::error::.*assets-amd64\.tar") + def test_invalid_arches_are_reported(self) -> None: """ARCHES_JSON must be a non-empty array of plain arch names.""" for raw in ("[]", '"amd64"', '{"amd64": 1}', "[1]", '[""]', '["../x"]', '["a/b"]', "nope"): @@ -532,6 +567,42 @@ def test_attempts_and_delay_are_configurable(self) -> None: self.assertEqual(self.count(), 3) self.assertEqual(self.sleeps(), [1, 2]) + def test_invalid_knobs_fail_before_any_lookup(self) -> None: + """A non-numeric or zero attempt count, or a non-numeric delay, is rejected up front.""" + self.calls(f"echo {SHA}") + for name, value in ( + ("TAG_LOOKUP_ATTEMPTS", "0"), + ("TAG_LOOKUP_ATTEMPTS", "abc"), + ("TAG_LOOKUP_ATTEMPTS", "-1"), + ("TAG_LOOKUP_DELAY", "1.5"), + ("TAG_LOOKUP_DELAY", "x"), + ("TAG_LOOKUP_DELAY", "-3"), + ("TAG_LOOKUP_DELAY", "08"), + ("TAG_LOOKUP_ATTEMPTS", "100"), + ): + with self.subTest(name=name, value=value): + result = self.run_script(**{name: value}) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertIn(f"::error::{name} must be", result.stderr) + self.assertFalse((self.tmp / "count").exists()) + + def test_zero_delay_is_allowed(self) -> None: + """A delay of 0 is valid (no wait between attempts).""" + self.calls(f"echo {SHA}") + self.assertEqual(self.run_script(TAG_LOOKUP_DELAY="0").returncode, 0) + + def test_every_attempts_stderr_is_logged(self) -> None: + """Earlier attempts' errors stay in the log, labelled with their attempt.""" + self.calls( + "echo 'gh: Bad Gateway (HTTP 502)' >&2; exit 1", + "echo 'error connecting to api.github.com' >&2; exit 1", + f"echo {SHA}", + ) + result = self.run_script() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("attempt 1/6: gh: Bad Gateway (HTTP 502)", result.stderr) + self.assertIn("attempt 2/6: error connecting to api.github.com", result.stderr) + def test_workflow_uses_the_script(self) -> None: """release.yml delegates to the script instead of an inline lookup.""" text = (SCRIPTS.parent / "workflows" / "release.yml").read_text() @@ -586,6 +657,69 @@ def test_failed_installer_download_names_the_installer(self) -> None: text_calls = re.findall(r"^download_installer (\S+) ", text, re.MULTILINE) self.assertEqual(text_calls, ["uv", "prek", "claude"]) + def run_download(self, curl_body: str) -> tuple[subprocess.CompletedProcess[str], int]: + """Run ``download_installer`` against a fake ``curl``; return the process and curl calls.""" + text = (SCRIPTS.parents[1] / "scripts" / "develop.sh").read_text() + parts = [] + for name in ("tmp_path", "fetch", "download_installer"): + body = re.search(rf"^{name}\(\) \{{\n.*?^\}}\n", text, re.MULTILINE | re.DOTALL) + self.assertIsNotNone(body, name) + parts.append(body.group(0)) # type: ignore[union-attr] + with tempfile.TemporaryDirectory() as tmp: + for name, body in ( + ("curl", f'#!/usr/bin/env bash\necho x >> "{tmp}/calls"\n{curl_body}\n'), + ("sleep", "#!/usr/bin/env bash\nexit 0\n"), + ): + path = Path(tmp) / name + path.write_text(body) + path.chmod(0o755) + script = ( + 'set -euo pipefail\ncleanup_paths=()\ntmp_result=""\ninstaller_script=""\n' + + "".join(parts) + + 'download_installer uv "https://example.invalid/uv-installer.sh"\n' + + 'echo "installed: $(cat "$installer_script")"\n' + ) + result = subprocess.run( # noqa: S603 -- fixed argv + ["bash", "-c", script], # noqa: S607 + env={**os.environ, "PATH": f"{tmp}{os.pathsep}{os.environ['PATH']}"}, + capture_output=True, + text=True, + check=False, + timeout=30, + ) + calls = (Path(tmp) / "calls").read_text().count("x") + return result, calls + + def test_transient_curl_failures_are_retried_by_fetch(self) -> None: + """DNS (6) and connect (7) failures retry inside fetch, not abort.""" + for status in (6, 7): + with self.subTest(status=status): + # Fail twice, then write the installer to the -o target. + body = ( + f'n=$(wc -l < "$(dirname "$0")/calls")\n' + f"if [ $n -le 2 ]; then echo 'curl: ({status})' >&2; exit {status}; fi\n" + 'while [ $# -gt 0 ]; do [ "$1" = -o ] && out=$2; shift; done\n' + 'echo script > "$out"\n' + ) + result, calls = self.run_download(body) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(calls, 3) + self.assertIn("installed: script", result.stdout) + + def test_persistent_curl_failure_gives_up_after_bounded_retries(self) -> None: + """A DNS failure that never clears aborts after 4 tries, naming the installer.""" + result, calls = self.run_download("exit 6") + self.assertEqual(result.returncode, 6, result.stderr) + self.assertEqual(calls, 4) + self.assertIn("uv installer", result.stderr) + + def test_empty_installer_download_fails(self) -> None: + """A download that leaves an empty file fails instead of installing nothing.""" + body = 'while [ $# -gt 0 ]; do [ "$1" = -o ] && out=$2; shift; done\n: > "$out"\n' + result, _ = self.run_download(body) + self.assertNotEqual(result.returncode, 0) + self.assertIn("installer is empty", result.stderr) + if __name__ == "__main__": unittest.main() diff --git a/.github/scripts/test_script_syntax.py b/.github/scripts/test_script_syntax.py index 2e70287..fc01f2e 100644 --- a/.github/scripts/test_script_syntax.py +++ b/.github/scripts/test_script_syntax.py @@ -15,6 +15,11 @@ SCRIPTS = Path(__file__).resolve().parent RUNNER_PYTHON = (3, 12) +# The system python3 of each runner image the workflows may default to +# (actions/runner-images, the image's *-Readme.md). Add the new image here, +# together with RUNNER_PYTHON and ruff.toml, when Renovate bumps the label. +RUNNER_IMAGE_PYTHON = {"ubuntu-24.04": (3, 12), "ubuntu-24.04-arm": (3, 12)} +WORKFLOWS = SCRIPTS.parent / "workflows" class ScriptsParseOnRunnerPythonTest(unittest.TestCase): @@ -41,6 +46,25 @@ def test_ruff_target_matches_the_runner_python(self) -> None: self.fail("ruff.toml has no target-version") self.assertEqual((int(match.group(1)), int(match.group(2))), RUNNER_PYTHON) + def test_workflows_default_to_a_runner_image_with_that_python(self) -> None: + """Bumping the default runner label fails here until the pin is revisited.""" + labels = set() + for path in sorted(WORKFLOWS.glob("*.yml")): + text = path.read_text(encoding="utf-8") + # Every quoted ubuntu-* label (CI_RUNNER / CI_RUNNER_AMD64 fallbacks and + # the release matrix's arm leg). + labels.update(re.findall(r"'(ubuntu-[^']+)'", text)) + self.assertTrue(labels) + for label in sorted(labels): + with self.subTest(runner=label): + self.assertEqual( + RUNNER_IMAGE_PYTHON.get(label), + RUNNER_PYTHON, + f"workflows default to {label}: look up its system python3 in " + "actions/runner-images, then update RUNNER_IMAGE_PYTHON, RUNNER_PYTHON " + "and ruff.toml's target-version together", + ) + if __name__ == "__main__": unittest.main() diff --git a/.github/scripts/test_template_sync.py b/.github/scripts/test_template_sync.py index aa80325..e5525a9 100644 --- a/.github/scripts/test_template_sync.py +++ b/.github/scripts/test_template_sync.py @@ -402,16 +402,44 @@ def apply(self) -> dict[str, list[str]]: def test_second_run_changes_and_reports_nothing(self) -> None: """Re-running the same old->new sync leaves the tree alone and lists no files.""" # Regression: a re-run reported every merge-tier file as "merged" again. - self.assertEqual(self.apply(), {"merged": ["m.txt"], "conflicts": []}) + self.assertEqual(self.apply(), {"merged": ["m.txt"], "conflicts": [], "binary": []}) merged = (self.instance / "m.txt").read_bytes() self.assertEqual(merged, b"a\nB\nc\nd\n") - self.assertEqual(self.apply(), {"merged": [], "conflicts": []}) + self.assertEqual(self.apply(), {"merged": [], "conflicts": [], "binary": []}) self.assertEqual((self.instance / "m.txt").read_bytes(), merged) def test_instance_already_equal_to_new_is_not_reported(self) -> None: """A file the instance already has at the new version is not "merged".""" write(self.instance / "m.txt", b"a\nB\nc\n", 0o644) - self.assertEqual(self.apply(), {"merged": [], "conflicts": []}) + self.assertEqual(self.apply(), {"merged": [], "conflicts": [], "binary": []}) + + def test_untouched_instance_file_keeps_its_other_mode_bits(self) -> None: + """Taking the new version outright rewrites content, not the instance's mode.""" + # Regression: copy2 copied the template's whole mode over the instance file's. + write(self.instance / "m.txt", b"a\nb\nc\n", 0o664) + self.assertEqual(self.apply()["merged"], ["m.txt"]) + self.assertEqual((self.instance / "m.txt").read_bytes(), b"a\nB\nc\n") + self.assertEqual(stat.S_IMODE((self.instance / "m.txt").stat().st_mode), 0o664) + + def test_untouched_instance_file_gains_the_upstream_executable_bit(self) -> None: + """Like the replace tier, an upstream 644 -> 755 change reaches an untouched file.""" + write(self.new / "m.txt", b"a\nB\nc\n", 0o755) + write(self.instance / "m.txt", b"a\nb\nc\n", 0o664) + self.assertEqual(self.apply()["merged"], ["m.txt"]) + self.assertEqual(stat.S_IMODE((self.instance / "m.txt").stat().st_mode), 0o775) + + def test_merged_file_keeps_its_mode_like_the_fast_path(self) -> None: + """Both ways of updating an existing file leave its mode alone.""" + write(self.instance / "m.txt", b"a\nb\nc\nd\n", 0o755) + self.assertEqual(self.apply()["merged"], ["m.txt"]) + self.assertEqual(stat.S_IMODE((self.instance / "m.txt").stat().st_mode), 0o755) + + def test_text_conflict_is_not_listed_as_binary(self) -> None: + """A conflict with markers is not a binary one.""" + write(self.instance / "m.txt", b"a\nZ\nc\n", 0o644) + result = self.apply() + self.assertEqual(result["conflicts"], ["m.txt"]) + self.assertEqual(result["binary"], []) def test_symlinked_instance_file_is_not_merged_through(self) -> None: """Like the replace tier, a deliberately linked instance file is left alone.""" @@ -419,7 +447,7 @@ def test_symlinked_instance_file_is_not_merged_through(self) -> None: target.write_bytes(b"a\nb\nc\nd\n") (self.instance / "m.txt").unlink() (self.instance / "m.txt").symlink_to(target) - self.assertEqual(self.apply(), {"merged": [], "conflicts": []}) + self.assertEqual(self.apply(), {"merged": [], "conflicts": [], "binary": []}) self.assertEqual(target.read_bytes(), b"a\nb\nc\nd\n") def test_dangling_symlink_is_not_written_through(self) -> None: @@ -427,7 +455,7 @@ def test_dangling_symlink_is_not_written_through(self) -> None: (self.instance / "m.txt").unlink() dangling = self.root / "nowhere.txt" (self.instance / "m.txt").symlink_to(dangling) - self.assertEqual(self.apply(), {"merged": [], "conflicts": []}) + self.assertEqual(self.apply(), {"merged": [], "conflicts": [], "binary": []}) self.assertFalse(dangling.exists()) @@ -474,7 +502,7 @@ def test_binary_file_is_a_conflict_and_left_untouched(self) -> None: write(new / "b.bin", b"a\x00c\n", 0o644) write(instance / "b.bin", b"a\x00d\n", 0o644) result = template_sync.apply_merge(old, new, instance, {"b.bin"}) - self.assertEqual(result, {"merged": [], "conflicts": ["b.bin"]}) + self.assertEqual(result, {"merged": [], "conflicts": ["b.bin"], "binary": ["b.bin"]}) self.assertEqual((instance / "b.bin").read_bytes(), b"a\x00d\n") def test_binary_file_untouched_by_the_instance_is_updated(self) -> None: @@ -484,7 +512,7 @@ def test_binary_file_untouched_by_the_instance_is_updated(self) -> None: write(new / "b.bin", b"a\x00c\n", 0o644) write(instance / "b.bin", b"a\x00b\n", 0o644) result = template_sync.apply_merge(old, new, instance, {"b.bin"}) - self.assertEqual(result, {"merged": ["b.bin"], "conflicts": []}) + self.assertEqual(result, {"merged": ["b.bin"], "conflicts": [], "binary": []}) self.assertEqual((instance / "b.bin").read_bytes(), b"a\x00c\n") def test_binary_file_already_at_the_new_version_is_skipped(self) -> None: @@ -494,7 +522,7 @@ def test_binary_file_already_at_the_new_version_is_skipped(self) -> None: write(new / "b.bin", b"a\x00c\n", 0o644) write(instance / "b.bin", b"a\x00c\n", 0o644) result = template_sync.apply_merge(old, new, instance, {"b.bin"}) - self.assertEqual(result, {"merged": [], "conflicts": []}) + self.assertEqual(result, {"merged": [], "conflicts": [], "binary": []}) def test_binary_new_file_without_prior_version_is_a_conflict(self) -> None: """A newly merge-tier binary file has no base; it is still a conflict, not an error.""" @@ -636,6 +664,11 @@ def test_malformed_summary_fails(self) -> None: self.summary.write_text('{"merge": []}') self.assertEqual(self.check(1)[0], 2) + def test_deeply_nested_summary_fails_instead_of_crashing(self) -> None: + """A summary that overflows the JSON parser's recursion limit is rejected.""" + self.summary.write_text("[" * 200_000) + self.assertEqual(self.check(1), (2, "")) + def test_error_exit_fails(self) -> None: """Exit 2 (or any other status) fails even with a valid summary.""" self.write_summary([]) @@ -731,6 +764,22 @@ def test_crash_exiting_1_fails_closed(self) -> None: self.assertEqual(result.returncode, 2, result.stderr) self.assertEqual(out, "") + def test_only_the_validated_line_reaches_github_output(self) -> None: + """Extra stdout from check-apply is never written to ``$GITHUB_OUTPUT``.""" + fake = self.FAKE.replace( + "import template_sync\n", + "if sys.argv[1] == 'check-apply':\n" + " print('conflicts=0\\nevil=1')\n" + " raise SystemExit(0)\n" + "import template_sync\n", + ) + self.assertNotEqual(fake, self.FAKE) # the replace must have applied + with mock.patch.object(ApplyStepTest, "FAKE", fake): + result, out = self.run_step(0, self.CLEAN) + self.assertEqual(out, "") + self.assertEqual(result.returncode, 1, result.stderr) + self.assertIn("unexpected line", result.stderr) + def test_summary_is_not_printed_as_valid_before_validation(self) -> None: """A rejected summary is printed only under a "not validated" label.""" result, _ = self.run_step(2, self.CLEAN) @@ -739,6 +788,51 @@ def test_summary_is_not_printed_as_valid_before_validation(self) -> None: self.assertLess(text.index("not validated"), text.index(self.CLEAN)) +class BinaryConflictVisibilityTest(unittest.TestCase): + """A binary conflict has no markers, so the PR body and the job log must say so.""" + + SUMMARY = { + "replace": {"updated": [], "removed": []}, + "merge": {"merged": [], "conflicts": ["b.bin", "t.txt"], "binary": ["b.bin"]}, + } + + def test_pr_body_labels_the_binary_conflict(self) -> None: + """Each conflict line says which kind it is.""" + body = template_sync.pr_body(self.SUMMARY, "v1.0.0", "v1.1.0", "o/t") + self.assertIn("binary conflict, no markers", body.split("`b.bin`")[0].splitlines()[-1]) + self.assertIn("conflict, needs manual resolution**): `t.txt`", body) + + def test_pr_body_accepts_a_summary_without_the_binary_key(self) -> None: + """A summary from an older apply still renders.""" + old = {**self.SUMMARY, "merge": {"merged": [], "conflicts": ["t.txt"]}} + self.assertIn("`t.txt`", template_sync.pr_body(old, "v1.0.0", "v1.1.0", "o/t")) + + def test_apply_warns_about_the_binary_conflict(self) -> None: + """``apply`` prints a ``::warning::`` annotation naming the file.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + old, new, instance = (root / n for n in ("old", "new", "instance")) + write(old / "b.bin", b"a\x00b\n", 0o644) + write(new / "b.bin", b"a\x00c\n", 0o644) + write(instance / "b.bin", b"a\x00d\n", 0o644) + manifest = root / "manifest.yml" + manifest.write_text("merge:\n - b.bin\n") + args = argparse.Namespace( + manifest=manifest, old_ref=old, new_ref=new, instance_root=instance + ) + err = io.StringIO() + with contextlib.redirect_stdout(io.StringIO()), contextlib.redirect_stderr(err): + status = template_sync.cmd_apply(args) + self.assertEqual(status, template_sync.EXIT_CONFLICTS) + self.assertIn("::warning file=b.bin::binary merge conflict", err.getvalue()) + + def test_warning_escapes_the_path(self) -> None: + """Characters that end an annotation property are percent-encoded.""" + self.assertEqual( + template_sync._annotation_escape("a,b:c%d\n.bin", prop=True), "a%2Cb%3Ac%25d%0A.bin" + ) + + _SCRIPT_PATH_RE = re.compile(r"(? }" @@ -24,6 +28,18 @@ attempts="${TAG_LOOKUP_ATTEMPTS:-6}" delay="${TAG_LOOKUP_DELAY:-5}" max_delay=60 +# Both knobs are used in arithmetic and `sleep`: reject anything else up front +# (fail closed), rather than let `set -e` die on a bash arithmetic error or +# misread a zero-padded 08 as octal, or loop zero times and report "after 0 attempt(s)". +if ! [[ "$attempts" =~ ^[1-9][0-9]?$ ]]; then + echo "::error::TAG_LOOKUP_ATTEMPTS must be a positive integer (at most 99), got '${attempts}'." >&2 + exit 1 +fi +if ! [[ "$delay" =~ ^(0|[1-9][0-9]{0,3})$ ]]; then + echo "::error::TAG_LOOKUP_DELAY must be a non-negative integer of seconds without leading zeros (at most 9999), got '${delay}'." >&2 + exit 1 +fi + err_file=$(mktemp) trap 'rm -f "$err_file"' EXIT @@ -40,7 +56,9 @@ for ((attempt = 1; attempt <= attempts; attempt++)); do break fi tag_sha= - cat "$err_file" >&2 + # Every attempt's stderr goes to the log, labelled; only the last one decides + # the final message below. + sed "s/^/attempt ${attempt}\/${attempts}: /" "$err_file" >&2 # `gh` missing, or run without a token: retrying cannot fix either. gh's # own messages for the latter (cli/cli) are "set the GH_TOKEN environment # variable" (automation, Actions) and "populate the GH_TOKEN environment diff --git a/.github/template-sync-state.json b/.github/template-sync-state.json index 33904fa..0ed9dd8 100644 --- a/.github/template-sync-state.json +++ b/.github/template-sync-state.json @@ -1,4 +1,4 @@ { "template_repo": "craftainer/template-docker", - "synced_tag": "v1.6.0" + "synced_tag": "v1.6.1" } diff --git a/.github/workflows/template-sync.yml b/.github/workflows/template-sync.yml index 9846c11..6ebb496 100644 --- a/.github/workflows/template-sync.yml +++ b/.github/workflows/template-sync.yml @@ -150,9 +150,19 @@ jobs: # 1, so check-apply trusts 0 or 1 only when the summary agrees. It # fails the job here, before the sync state is advanced, and the # summary is printed as valid only once it has agreed. + # Only the validated `conflicts=0|1` line reaches $GITHUB_OUTPUT. check_rc=0 - python3 template_sync.py check-apply --exit-code "$rc" \ - --summary /tmp/apply-summary.json >> "$GITHUB_OUTPUT" || check_rc=$? + check_out=$(python3 template_sync.py check-apply --exit-code "$rc" \ + --summary /tmp/apply-summary.json) || check_rc=$? + case "$check_out" in + conflicts=0 | conflicts=1) [ "$check_rc" -ne 0 ] || echo "$check_out" >> "$GITHUB_OUTPUT" ;; + *) + if [ "$check_rc" -eq 0 ]; then + echo "::error::check-apply printed an unexpected line; not trusting it." >&2 + check_rc=1 + fi + ;; + esac if [ "$rc" -gt 1 ] || [ "$check_rc" -ne 0 ]; then echo "::group::apply stdout (not validated)" cat /tmp/apply-summary.json || true diff --git a/docs/TEMPLATE.md b/docs/TEMPLATE.md index fde3be4..78b8be5 100644 --- a/docs/TEMPLATE.md +++ b/docs/TEMPLATE.md @@ -229,6 +229,17 @@ under that version (detected only here, after the per-arch images of both runs w pushed). GitHub keeps at most one waiting run per group, and a newer waiting run replaces it. +A release that exists on GitHub counts as complete: whatever failed after it +is finished by re-running the failed jobs, never by cutting another version. + +**Opting into a registry.** The `PYPI_PUBLISH` (`testpypi` or `pypi`), +`NPM_PUBLISH` and `CARGO_PUBLISH` repository variables are what opt an +instance into a registry; with none set, `publish-packages` and +`verify-published` do not run. `NPM_TOKEN` and `CARGO_REGISTRY_TOKEN` are +only for the first release of a package, before trusted publishing is set up +at the registry. See `.github/workflows/README.md`, +"Release variables and secrets". + Re-run only the *failed jobs* of a run. "Re-run all jobs" runs `version` again, which sees the tag the first attempt created and computes the next version, so it cuts another release instead of finishing the first. @@ -272,7 +283,9 @@ more files to `dist/`, and the assembled `dist/` (uploaded at the end of the more files, but they are attached to the GitHub release only: `publish-packages` and `verify-published` do not see them. A path that two legs both produce (the same file name, a file in one and a directory of -that name in another, or a symlink) fails the assembly. +that name in another, or a symlink) fails the assembly. The assembly also +refuses to run when `dist/` already exists and is not empty, so stray files +cannot end up in a release. `release.yml` then runs `gh release create` with every file left in `dist/`. @@ -542,6 +555,24 @@ is meant to run once per tag pair: the state file moves to the new tag afterwards. Running it again is harmless for files already merged cleanly, but it merges a hand-resolved conflict again and re-creates its markers. +Further sync behaviour, detailed in `.github/scripts/README.md`: + +- `apply` is not atomic. CI discards the checkout and does not advance the + state on an error; after a failed manual run, reset the working tree with + git before trying again. +- A binary `merge`-tier file the instance changed cannot be merged. It is + kept as it is, reported as a conflict, listed as `merge.binary` in the + summary and labelled in the sync PR. +- The moderation `-apply` workflows fail on an expired `moderation-result` + artifact instead of passing: a produced result was lost, so re-run the + worker run (see `.github/workflows/README.md`, "Issue moderation"). +- The scripts under `.github/scripts/` run on the runner's system `python3`. + Base ships `.github/scripts/ruff.toml` (`replace` tier), pinned to that + Python's `target-version`, and `test_script_syntax.py` guards it with + `ast.parse(feature_version=...)`. A Python instance must not override or + remove it: its own `pyproject.toml` targets a newer Python and ruff would + rewrite those scripts into syntax the runner cannot parse. + An instance that isn't itself a template shouldn't edit that manifest to list its own files (`src/`, say): it's `merge`-tier, so edits would conflict with later syncs. It lists them under an `ignore:` key in the @@ -574,6 +605,12 @@ template-base └── template-react ``` +Naming: a library repo is `lib--` and an app repo is +`template-`; the registry package is `craftainer-` on PyPI and +crates.io and `@craftainer/` on npm (`lib-crud-sdk-ts` publishes +`@craftainer/crud-sdk`). Libraries declare `GPL-3.0-or-later` in their package +manifest; base itself ships the GPL v3 text in `LICENSE`. + Each mid-layer template is an instance of base that is itself a template. It adds one kind of artifact to base's release contract and keeps everything else from base: diff --git a/scripts/README.md b/scripts/README.md index ae168db..a6480e0 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -11,7 +11,10 @@ that download for themselves run through `retry` (the installer script itself is fetched once, by `fetch`, so the two layers of retries do not multiply; a failed download names the installer and URL before the script - aborts), so CI's shared-IP HTTP 429s don't fail the image build. Local builds need no token. + aborts, an empty one is rejected, and `fetch` itself retries DNS and + connect failures (curl exit 6/7) and truncated transfers (18/52/55/56) up + to 4 times; timeouts are left to curl's own `--retry`), so CI's shared-IP + HTTP 429s don't fail the image build. Local builds need no token. - `post-create.sh` — the devcontainer's `postCreateCommand`: git `safe.directory`, `prek install`, then every `post-create.d/*.sh`. - `develop.d/`, `post-create.d/`, `runner-setup.d/` — instance hook diff --git a/scripts/develop.sh b/scripts/develop.sh index 98cb9cf..dad18db 100644 --- a/scripts/develop.sh +++ b/scripts/develop.sh @@ -80,9 +80,14 @@ fetch() { esac fi # curl's --retry (+ --retry-connrefused) covers timeouts, refused - # connections and HTTP 408/429/5xx; resets and truncated transfers - # (exit 18/52/55/56) are retried here, so a permanent HTTP error (-f, - # exit 22), DNS or TLS failure still fails at once. Callers must write + # connections and HTTP 408/429/5xx; DNS and connect failures (exit 6/7), + # resets and truncated transfers (exit 18/52/55/56) are retried here, so a + # permanent HTTP error (-f, exit 22) or a TLS failure still fails at once. + # A timeout (28) is not: curl has already spent its --retry-max-time on it, + # and looping again would multiply that. At most 4 tries here, each with + # curl's own retries inside: bounded, and `retry` never wraps a fetch. A transfer curl can tell is cut short (a Content-Length + # mismatch is exit 18, a cut chunked body 56) is therefore re-fetched + # here, not by re-running an installer on a broken file. Callers must write # to a file (-o): a re-run after a partial transfer would otherwise # duplicate stdout. local attempt=1 status=0 @@ -91,7 +96,7 @@ fetch() { curl -fsSL --retry 5 --retry-max-time 60 --retry-connrefused \ ${auth[@]+"${auth[@]}"} "$@" "$url" || status=$? case "$status" in - 18 | 52 | 55 | 56) + 6 | 7 | 18 | 52 | 55 | 56) if [ "$attempt" -lt 4 ]; then echo "curl exit $status; retrying ($attempt/3): $url" >&2 sleep $((attempt * 2)) @@ -142,6 +147,11 @@ download_installer() { installer_script="$tmp_result" chmod a+r "$installer_script" fetch_label="$1 installer" fetch "$2" -o "$installer_script" + # An empty 200 response would "succeed" at installing nothing. + if [ ! -s "$installer_script" ]; then + echo "downloaded $1 installer is empty: $2" >&2 + return 1 + fi } # The installers do their own downloads. prek's comes only from github.com,