Server comments describe the code to a stranger (#6221)
CI & Build / Python lint (push) Successful in 2s
CI & Build / Build now, or wait for Android? (push) Successful in 2s
Android / Build, or is the channel already serving this? (push) Successful in 3s
Android / Core and FFI clippy and tests (push) Skipped
Android / Kotlin + Rust (APK) (push) Skipped
Desktop (Tauri) / Build, or is the channel already serving this? (push) Successful in 2s
CI & Build / Web typecheck and unit tests (push) Successful in 9s
CI & Build / Python tests (push) Successful in 13s
Android / Build the server image (push) Skipped
Desktop (Tauri) / Web tests, clippy, Rust tests and rustfmt (push) Skipped
Desktop (Tauri) / Tauri desktop (Linux) (push) Skipped
Desktop (Tauri) / Windows installer (cross-compiled) (push) Skipped
Desktop (Tauri) / Update manifest (push) Skipped
CI & Build / integration (push) Successful in 1m30s
CI & Build / Build & push image (push) Successful in 32s
CI & Build / Python lint (push) Successful in 2s
CI & Build / Build now, or wait for Android? (push) Successful in 2s
Android / Build, or is the channel already serving this? (push) Successful in 3s
Android / Core and FFI clippy and tests (push) Skipped
Android / Kotlin + Rust (APK) (push) Skipped
Desktop (Tauri) / Build, or is the channel already serving this? (push) Successful in 2s
CI & Build / Web typecheck and unit tests (push) Successful in 9s
CI & Build / Python tests (push) Successful in 13s
Android / Build the server image (push) Skipped
Desktop (Tauri) / Web tests, clippy, Rust tests and rustfmt (push) Skipped
Desktop (Tauri) / Tauri desktop (Linux) (push) Skipped
Desktop (Tauri) / Windows installer (cross-compiled) (push) Skipped
Desktop (Tauri) / Update manifest (push) Skipped
CI & Build / integration (push) Successful in 1m30s
CI & Build / Build & push image (push) Successful in 32s
Rule 32 "Comments describe the product to a stranger" sweep over src/, alembic/ and tests/. Comments and docstrings now say what the code does and the reason as a present-tense constraint, briefly. Removed: - issue, task, note, rule, milestone and family-idea references - "used to" / before-and-after narration - quotes of decisions and incident stories from the deploying instance - paragraph-length arguments, where a line or two carries the fact Kept every hazard as a present-tense fact, e.g. timing oracles, the proxy hop direction, and migrations that must stay frozen. Also: - Three test names lose "still"/"used to"/"pre_m304" phrasing. - pyproject's description drops "FabledSword family". The history these comments carried stays in Scribe and in git log. A comment-stripping diff (Scribe snippet #6226 "codeonly.py — prove a diff changed only comments") shows no code changed apart from the renamed tests and the description string. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+33
-69
@@ -1,17 +1,8 @@
|
||||
"""The version derivation — `packaging/version.sh`.
|
||||
|
||||
These tests build their OWN git repo in a tmpdir rather than reading this one's
|
||||
history. Two reasons, and the second is the important one:
|
||||
|
||||
* They then need no `fetch-depth: 0` on the test lane, and cannot start passing or
|
||||
failing because somebody pushed.
|
||||
* They can commit to ONE artifact's file set at a time, which is the only way to
|
||||
assert the property this whole change exists for: that a Kotlin-only commit
|
||||
leaves the desktop's version alone. Against real history you can only observe
|
||||
whatever the last commits happened to touch.
|
||||
|
||||
Note 3127 §3 cites this repo as its example of the failure being fixed here — one
|
||||
generator feeding three artifacts, so a Rust-only commit re-versioned the phone.
|
||||
These tests build their own git repo in a tmpdir rather than reading this one's
|
||||
history, so they need no `fetch-depth: 0`, and can commit to one artifact's file set
|
||||
at a time: a Kotlin-only commit must leave the desktop's version alone.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -26,9 +17,8 @@ SCRIPT = Path(__file__).resolve().parent.parent / "packaging" / "version.sh"
|
||||
GUARD = Path(__file__).resolve().parent.parent / "packaging" / "guard-forward.sh"
|
||||
SHOULD_BUILD = Path(__file__).resolve().parent.parent / "packaging" / "should-build.sh"
|
||||
|
||||
# 2020-01-01T00:00:00Z, the counter epoch. Duplicated from the script deliberately:
|
||||
# a test that imported the value could not catch the value being changed, and moving
|
||||
# this epoch renumbers every artifact downwards (note 3127 §6.4).
|
||||
# 2020-01-01T00:00:00Z, the counter epoch. Duplicated from the script so a change to
|
||||
# it fails here: moving it renumbers every artifact downwards.
|
||||
EPOCH = 1577836800
|
||||
|
||||
|
||||
@@ -53,9 +43,8 @@ def commit(repo: Path, path: str, when: int) -> None:
|
||||
["git", "-C", str(repo), "-c", "user.email=t@t", "-c", "user.name=t",
|
||||
"commit", "-q", "-m", f"touch {path}"],
|
||||
check=True, capture_output=True, text=True,
|
||||
# EXTEND the environment rather than replacing it: a minimal env is enough
|
||||
# for git here but not necessarily inside the CI container, and a test that
|
||||
# fails only there is worse than no test.
|
||||
# Extend the environment rather than replacing it; the CI container may need
|
||||
# more than git does here.
|
||||
env={**os.environ,
|
||||
"GIT_AUTHOR_DATE": f"@{when} +0000", "GIT_COMMITTER_DATE": f"@{when} +0000"},
|
||||
)
|
||||
@@ -121,18 +110,17 @@ def test_a_shared_core_commit_moves_both(repo: Path) -> None:
|
||||
|
||||
|
||||
def test_the_server_set_contains_the_android_set(repo: Path) -> None:
|
||||
"""The image BAKES IN the APK, so an APK-only change changes what the image
|
||||
ships. Note 3127 §3's bundled-artifact trap; FC's web image embeds the extension
|
||||
the same way, and Roundtable needed a bespoke workflow for want of modelling it."""
|
||||
"""The image bakes in the APK, so an APK-only change changes what the image
|
||||
ships."""
|
||||
commit(repo, "android/app/src/Main.kt", 1787911200) # 10:00
|
||||
assert version(repo, "display", "server") == "2026.08.28.1000"
|
||||
assert "android" in version(repo, "paths", "server")
|
||||
|
||||
|
||||
def test_the_build_recipe_is_in_the_set(repo: Path) -> None:
|
||||
"""A workflow file is not shipped, but change a build flag and the bytes change
|
||||
while the source does not. Once step 6 skips a build whose version already
|
||||
exists, that combination serves the OLD artifact on a green run."""
|
||||
"""A workflow file is not shipped, but a build flag changes the bytes while the
|
||||
source does not. A build skipped because its version exists would then serve the
|
||||
old artifact on a green run."""
|
||||
commit(repo, ".forgejo/workflows/desktop.yml", 1787911200) # 10:00
|
||||
assert version(repo, "display", "desktop") == "2026.08.28.1000"
|
||||
|
||||
@@ -146,12 +134,8 @@ def test_the_answer_does_not_depend_on_the_caller_s_directory(repo: Path, subdir
|
||||
`desktop/src-tauri`, the Android build from `android`, the manifest from the root
|
||||
— so without an anchor the same request answers differently per caller.
|
||||
|
||||
This is not hypothetical and it is not a loud failure. Run 4796 produced THREE
|
||||
versions from one push: the desktop build said 1.0.3494522 while the manifest and
|
||||
the pacman packager said 1.0.3502131, because the build's pathspec matched
|
||||
`desktop/src-tauri/Cargo.toml` — a real file, six days stale. Non-empty, so the
|
||||
shallow-clone guard could not fire. The Android job failed loudly in the same run
|
||||
only because ITS pathspec happened to match nothing; same bug, luckier symptom."""
|
||||
It fails quietly: a relative pathspec can match a real, stale file from the
|
||||
desktop's directory and give one push several versions."""
|
||||
(repo / "desktop/src-tauri").mkdir(parents=True, exist_ok=True)
|
||||
(repo / "core").mkdir(parents=True, exist_ok=True)
|
||||
assert version(repo, "display", "desktop", subdir=subdir) == "2026.08.28.0900"
|
||||
@@ -171,7 +155,7 @@ def test_every_artifact_agrees_across_directories(repo: Path) -> None:
|
||||
# --- the ordering keys -------------------------------------------------------
|
||||
|
||||
def test_the_desktop_key_is_valid_semver(repo: Path) -> None:
|
||||
"""THE test that keeps the update channel alive. Tauri parses `latest.json`'s
|
||||
"""What keeps the update channel alive. Tauri parses `latest.json`'s
|
||||
version with the semver crate AT DESERIALIZATION — a string it cannot parse does
|
||||
not sort low, it makes the whole feed fail to load and every client report "no
|
||||
update available" forever. Exactly three numeric segments, no leading zeros."""
|
||||
@@ -188,9 +172,8 @@ def test_the_desktop_key_and_display_describe_one_build(repo: Path) -> None:
|
||||
|
||||
|
||||
def test_the_desktop_key_clears_what_is_already_installed(repo: Path) -> None:
|
||||
"""`0.0.<minutes>` reads best as "not a version" and would have stranded every
|
||||
dev user: minor 0 < 2 puts it below the installed `0.2.466` line, and "up to
|
||||
date" forever is the direction you cannot recover from."""
|
||||
"""Not `0.0.<minutes>`: minor 0 sorts below installed `0.2.x` builds, which would
|
||||
then read "up to date" forever."""
|
||||
key = tuple(int(p) for p in version(repo, "key", "desktop").split("."))
|
||||
assert key > (0, 2, 466)
|
||||
assert key > (0, 2, 999999) # and above any run number that line could reach
|
||||
@@ -206,8 +189,8 @@ def test_the_android_key_is_an_int_android_will_accept(repo: Path) -> None:
|
||||
|
||||
|
||||
def test_the_server_has_no_ordering_key(repo: Path) -> None:
|
||||
"""Nothing compares a server image — no updater, no install gate. §2: do not add
|
||||
an ordering key because the other artifacts have one."""
|
||||
"""Nothing compares a server image (no updater, no install gate), so it has no
|
||||
ordering key just because the other artifacts do."""
|
||||
r = subprocess.run(["sh", str(SCRIPT), "key", "server"],
|
||||
cwd=repo, capture_output=True, text=True)
|
||||
assert r.returncode != 0
|
||||
@@ -218,13 +201,9 @@ def test_the_server_has_no_ordering_key(repo: Path) -> None:
|
||||
|
||||
@pytest.mark.parametrize("what", ["display", "key", "paths"])
|
||||
def test_an_unknown_artifact_is_rejected(repo: Path, what: str) -> None:
|
||||
"""It was not. `paths_for`'s `exit 2` ran inside `$(paths_for ...)`, so it printed
|
||||
the error, returned an EMPTY pathspec — which matches everything — and answered
|
||||
`2026.08.28.0900` with exit 0. A confident version for an artifact that does not
|
||||
exist, which is the worst kind of wrong for a value nothing else can contradict.
|
||||
|
||||
Asserting on stdout as well as the exit code, because the exit code alone passed
|
||||
for `display` in the first version of this file while stdout carried a lie."""
|
||||
"""An `exit` inside `$(...)` ends only the subshell, so an empty pathspec (which
|
||||
matches everything) would answer with a confident version and exit 0. Asserts on
|
||||
stdout as well as the exit code."""
|
||||
r = subprocess.run(["sh", str(SCRIPT), what, "nope"],
|
||||
cwd=repo, capture_output=True, text=True)
|
||||
assert r.returncode != 0, f"{what} exited 0 with stdout={r.stdout!r}"
|
||||
@@ -233,17 +212,11 @@ def test_an_unknown_artifact_is_rejected(repo: Path, what: str) -> None:
|
||||
|
||||
@pytest.mark.parametrize("what", ["display", "key"])
|
||||
def test_no_matching_history_fails_rather_than_guessing(tmp_path: Path, what: str) -> None:
|
||||
"""The shallow-clone failure (note 3127 §6.1), which is the one that matters:
|
||||
depth-1 sees one commit, `git log -- <paths>` finds nothing for most artifacts,
|
||||
and a script that shrugged would emit a too-LOW version with the lane green.
|
||||
Too-low is unrecoverable — every installed client is stranded.
|
||||
"""A shallow clone sees one commit, so `git log -- <paths>` finds nothing for
|
||||
most artifacts, and a script that shrugged would emit a too-low version with the
|
||||
lane green. Too low is unrecoverable: every installed client is stranded.
|
||||
|
||||
BOTH REQUESTS, and the parametrize is the point rather than thoroughness. The
|
||||
first version of this guard `exit 1`-ed inside a function called as `$(...)`,
|
||||
which ends the SUBSHELL and not the script. `display` still failed — but only
|
||||
because `date` then choked on an empty string. `key` printed the error, emitted
|
||||
`1.0.-26297280`, and exited ZERO. One path was covered and the other was broken
|
||||
in exactly the way the guard existed to prevent."""
|
||||
Both requests, because an `exit` inside `$(...)` can fail one and not the other."""
|
||||
git(tmp_path, "init", "-q", "-b", "dev")
|
||||
commit(tmp_path, "README.md", 1787900400) # in no artifact's set
|
||||
r = subprocess.run(["sh", str(SCRIPT), what, "desktop"],
|
||||
@@ -267,9 +240,8 @@ def compare(a: str, b: str) -> bool:
|
||||
|
||||
|
||||
@pytest.mark.parametrize("a,b,expect_lt", [
|
||||
# THE trap: as text, "1.0.9" > "1.0.10". The comparison must be numeric
|
||||
# per dot-segment, which is what note 3127 §1 spells out and what a naive
|
||||
# `[ "$a" \< "$b" ]` would get exactly backwards.
|
||||
# As text, "1.0.9" > "1.0.10". The comparison must be numeric per dot-segment;
|
||||
# `[ "$a" \< "$b" ]` gets it backwards.
|
||||
("1.0.9", "1.0.10", True),
|
||||
("1.0.10", "1.0.9", False),
|
||||
# Equal is NOT less. Under commit time an unchanged source derives what it
|
||||
@@ -309,18 +281,11 @@ def test_the_guard_rejects_bad_invocations(args: list[str]) -> None:
|
||||
|
||||
# --- the backwards guard against a channel ------------------------------------
|
||||
#
|
||||
# The half of the guard that talks to a release feed, which the `compare` tests above
|
||||
# deliberately do not reach. Hermetic anyway: `curl` is shadowed on PATH by a stub, so
|
||||
# these assert what the guard does with an answer rather than what the forge returns.
|
||||
# The half of the guard that talks to a release feed. Hermetic: `curl` is shadowed on
|
||||
# PATH by a stub.
|
||||
#
|
||||
# WHY THIS SECTION EXISTS AT ALL. The empty-channel case — a channel that has never
|
||||
# published this artifact — is one the guard is written to PASS, and it says so in a
|
||||
# branch of its own. It did not: `published="$(published_for ...)"` under `set -e`
|
||||
# died on the substitution before that branch could run, because Android's lookup
|
||||
# ended in a `grep` that exits 1 on no match while its three siblings ended in a `sed`
|
||||
# that exits 0. Silent, because everything the pipeline printed went into the capture.
|
||||
# It took the Android lane down on the first merge to `main` (run 4857) and nothing
|
||||
# here would have caught it.
|
||||
# A channel that has never published an artifact must pass. Under `set -e`, a lookup
|
||||
# ending in a `grep` with no match kills the substitution before that branch runs.
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
@@ -361,8 +326,7 @@ def guard(repo: Path, env: dict[str, str], *args: str) -> subprocess.CompletedPr
|
||||
def test_an_empty_channel_passes_the_guard(repo: Path, curl_stub, artifact: str) -> None:
|
||||
"""Nothing published yet is not a backwards move — it is the first publish.
|
||||
|
||||
Failing here would block the very first build to reach a channel, which is exactly
|
||||
what happened to Android on `stable`.
|
||||
Failing here would block the first build to reach a channel.
|
||||
"""
|
||||
r = guard(repo, curl_stub(None), artifact, "stable")
|
||||
assert r.returncode == 0, f"exit={r.returncode} stdout={r.stdout!r} stderr={r.stderr!r}"
|
||||
|
||||
Reference in New Issue
Block a user