h-dv/code-index

Watch

Fork 0

The release job's fixture-count constant blames the package for the workflow's own staleness: "the package declares 5 fixtures" when it is release.yml that declares 5 #239

New issue

Closed

opened 2026-09-09 12:40:39 +02:00 by buildagent · 1 comment

buildagent commented

2026-09-09 12:40:39 +02:00

Member

Reported by the de.h-dv.timeline author (w100-sys) after diffing package-plugin-timeline against package-plugin at their own initiative. They raised it explicitly as "worth a line in the issue queue", not as a blocker, and said they would not add a fixture without warning us either way. Verified on master before filing.

Measured

Both package jobs hard-code the fixture count of the package they pack:

.forgejo/workflows/release.yml:2847   if [ "${GRADED}" -ne 2 ] || [ "${UNGRADED}" -ne 0 ]; then   # xaml
.forgejo/workflows/release.yml:3339   if [ "${GRADED}" -ne 5 ] || [ "${UNGRADED}" -ne 0 ]; then   # timeline

So de.h-dv.timeline adding a sixth fixture reds our release job. That coupling is consistent between the two jobs rather than a slip, and the count check itself is right — C1 without C2 is a run, not a grading, and a published package must declare an expectation for every fixture. The check should stay.

The part that is actually wrong

The error message attributes the constant to the wrong party:

::error::the package declares 5 fixtures and ${GRADED} were graded (${UNGRADED} not graded).

The package does not declare 5. release.yml:3339 declares 5. When a package legitimately grows to six fixtures, this fires and tells the reader the package is defective — sending a package author to audit a manifest that is correct, over a stale constant in our workflow that they cannot see and did not write.

This is the disclosure discipline applied everywhere else in this project, one layer out: a number rendered as if it came from the subject when it came from the checker. The same message on the xaml job is equally wrong, just less likely to fire because we own that package and would move both together.

Two fixes, and the first is the real one

Derive the expected count from the manifest — the package states its own fixtures, so the check can compare graded against what the package declares rather than against a literal. The check keeps all of its force and the coupling disappears. This is the one that matches how the rest of the release gate works (the digest is read from tests/packages/*.digest, never inlined — the author's own review confirmed grep -c '150ceb22' release.yml returns 0).
If the constant must stay, the message has to say where it lives: "release.yml expects N fixtures; the package graded M — if the package legitimately grew, update the constant at release.yml:NNNN." A refusal that does not name the file the reader must edit is a dead end.

Prefer 1. Fall back to 2 only with a stated reason why the manifest cannot be read at that point in the job.

What a fix must prove

A package that grows by one fixture packs successfully, with no workflow edit.
Anti-vacuity, both arms: a package whose fixtures are declared but NOT graded still reds (that is the property the check exists for, and it must survive the change), and a package with zero fixtures does not pass silently by making the comparison trivially true.
The message, on whichever failure survives, names the file and line a reader must change.
Both jobs are fixed by the same clause. This project's rule is one mechanism, not a fix per case — and there are exactly two call sites today precisely because it was written twice.

Provenance note

Worth recording how this was found, because it is the second time this method has paid: the author extracted both jobs, stripped comments and blanks, normalised every package name to PKG, and diffed the copy against its original rather than reading the copy. Step names came out identical one-for-one, every substantive difference was a legitimate adaptation, and the one thing left over was this. A dropped or stale guard is invisible in a copy and obvious in a diff.

Their review also found the timeline job's language assertion is stronger than the xaml original rather than merely adapted — it greps for both wire ids, so a package that silently lost one of its two languages reds there, a check the single-language xaml job cannot express.

#233 (the Loader.cs in the timeline scratch project, whose removal is that fix's regression test), #232.

Filed 2026-09-09 against master 915c850.

Reported by the `de.h-dv.timeline` author (w100-sys) after diffing `package-plugin-timeline` against `package-plugin` at their own initiative. They raised it explicitly as "worth a line in the issue queue", not as a blocker, and said they would not add a fixture without warning us either way. Verified on `master` before filing. ## Measured Both package jobs hard-code the fixture count of the package they pack: ``` .forgejo/workflows/release.yml:2847 if [ "${GRADED}" -ne 2 ] || [ "${UNGRADED}" -ne 0 ]; then # xaml .forgejo/workflows/release.yml:3339 if [ "${GRADED}" -ne 5 ] || [ "${UNGRADED}" -ne 0 ]; then # timeline ``` So `de.h-dv.timeline` adding a sixth fixture **reds our release job**. That coupling is consistent between the two jobs rather than a slip, and the count check itself is right — C1 without C2 is a run, not a grading, and a published package must declare an expectation for every fixture. The check should stay. ## The part that is actually wrong The error message attributes the constant to the wrong party: ``` ::error::the package declares 5 fixtures and ${GRADED} were graded (${UNGRADED} not graded). ``` **The package does not declare 5. `release.yml:3339` declares 5.** When a package legitimately grows to six fixtures, this fires and tells the reader the package is defective — sending a package author to audit a manifest that is correct, over a stale constant in *our* workflow that they cannot see and did not write. This is the disclosure discipline applied everywhere else in this project, one layer out: a number rendered as if it came from the subject when it came from the checker. The same message on the xaml job is equally wrong, just less likely to fire because we own that package and would move both together. ## Two fixes, and the first is the real one 1. **Derive the expected count from the manifest** — the package states its own fixtures, so the check can compare `graded` against what the package declares rather than against a literal. The check keeps all of its force and the coupling disappears. This is the one that matches how the rest of the release gate works (the digest is read from `tests/packages/*.digest`, never inlined — the author's own review confirmed `grep -c '150ceb22' release.yml` returns **0**). 2. **If the constant must stay**, the message has to say where it lives: *"release.yml expects N fixtures; the package graded M — if the package legitimately grew, update the constant at release.yml:NNNN."* A refusal that does not name the file the reader must edit is a dead end. Prefer 1. Fall back to 2 only with a stated reason why the manifest cannot be read at that point in the job. ## What a fix must prove - A package that grows by one fixture packs successfully, with no workflow edit. - **Anti-vacuity, both arms:** a package whose fixtures are declared but NOT graded still reds (that is the property the check exists for, and it must survive the change), and a package with zero fixtures does not pass silently by making the comparison trivially true. - The message, on whichever failure survives, names the file and line a reader must change. - Both jobs are fixed by the same clause. This project's rule is one mechanism, not a fix per case — and there are exactly two call sites today precisely because it was written twice. ## Provenance note Worth recording how this was found, because it is the second time this method has paid: the author extracted both jobs, stripped comments and blanks, normalised every package name to `PKG`, and **diffed the copy against its original** rather than reading the copy. Step names came out identical one-for-one, every substantive difference was a legitimate adaptation, and the one thing left over was this. A dropped or stale guard is invisible in a copy and obvious in a diff. Their review also found the timeline job's language assertion is *stronger* than the xaml original rather than merely adapted — it greps for both wire ids, so a package that silently lost one of its two languages reds there, a check the single-language xaml job cannot express. ## Related #233 (the `Loader.cs` in the timeline scratch project, whose removal is that fix's regression test), #232. Filed 2026-09-09 against `master` `915c850`.

buildagent commented

2026-09-09 22:06:20 +02:00

Author

Member

Fixed on master at b36d2c8 (merged 34b0fd5). And this issue understates the blast radius: there were FIVE sites, not two — and the one it missed is the one that ships a wrong number to operators.

The five

site	what it is
`release.yml:2847` / `:3339`	the two `GRADED` comparisons this issue names
`release.yml:2869` / `:3365`	`grep -qE '^conformance .passed C1 over 5 fixture$s$, C2 facts compared for 5'` — same defect, two more literals each*
`release.yml:3989`	`BODY+="The five fixtures shipped inside it are synthetic…"`

Fixing only the two named sites would have satisfied this issue's text and still failed its own requirement: a package grown to six fixtures reds at :3365, and publishes "The five fixtures" regardless. The reporter diffed the copied job and found the loudest instance; the quiet one fails no release — it just tells operators something untrue.

All five now go through one shared script, .forgejo/scripts/require_fixtures_graded.sh, following require_free_disk.sh's established pattern (same directory, same "one implementation, two doors, graded by a Rust test" idiom). Option 1 throughout — no fallback was needed: both package jobs already read the fingerprint and digest record from the checkout before their cd scratch, so the manifest is reachable exactly there.

An extra defect found while wiring the notes

That step has no set -e — its run: opens with apt-get, not set -euo pipefail. A failed derivation would have left the variable empty and published:

The  fixtures shipped inside it are synthetic…

Guarded with an explicit -z refusal, and the guard is graded (W7 → RED).

The both-sites proof, re-run by me

both_package_jobs_are_graded_by_the_same_clause lifts every invocation out of the YAML — jobs derived, never listed — substitutes only ${ROOT} and the transcript paths, and runs each job's own command. Failures are collected rather than asserted in the loop, so a shared-clause mutation is seen to hit both. Changing one line in the script:

BEFORE 178: if [ "$GRADED" -ne "$DECLARED" ]; then
AFTER  178: if [ "$GRADED" -eq "$DECLARED" ]; then

2 of 5 lifted command(s) wrong:
  package-plugin           (exit 1)  …/tests/packages/xaml/plugin.toml
  package-plugin-timeline  (exit 1)  …/tests/packages/timeline/plugin.toml

Because the jobs are derived from the file, a future package job is covered without anyone remembering to add it.

Two survivors that changed the work

Both reported as survivors first, then fixed — and this is the most instructive part:

W5 SURVIVED. The first offender rule was "a line mentioning fixture and carrying a digit". The five fixtures has no digit, so the tree's actual defect text walked straight through the gate written to catch it. Fixed with number words, matched whole so one cannot fire on none.

Then the widened rule turned the CORRECT line red: "The ${TL_FIXTURES} fixtures … the worked examples in the two format specifications". A gate that fires on the fixed text is worse than one that misses the broken text. Final rule: the number must sit within three tokens of the word it counts, and both halves are pinned in the_fixture_count_predicate_knows_a_count_from_a_number.

Also recorded honestly: S5 (-ne "$DECLARED" → -lt) reds the "too many" arm but SURVIVES a_package_that_grows_a_fixture_still_packs, because both of that test's arms still hold under -lt. And W2 (point one job at the other's manifest) reds the source-shape test but SURVIVES the executing one, which builds its transcript from the manifest the command itself names — so a wrong manifest is self-consistent. That division of labour is stated rather than papered over.

What this is NOT verified against

The release workflow cannot be dispatched from here, so none of this is measured:

that a runner reaches these lines, or that the checkout is present at that point (inferred from require_free_disk.sh being invoked by relative path in the same jobs);
that ${ROOT} holds the repo root at runtime — it is $(pwd) captured before cd scratch, beside the existing fingerprint read, but that is reasoning about the step, not a run of it;
that the script is executable on the runner image (100755 is committed; the image's sh is not exercised);
the largest hole: that real plugin check / plugin status output has the shape the staged transcripts imitate. The shape is read out of crates/cli/src/plugin.rs::facts_lines — a reading of the producer, not a recording of a run. If the real output indents differently the greps count 0, and the zero-floor does not fire because it guards the manifest side.

Two findings not fixed

1. The manifest's own prose is stale about itself. tests/packages/timeline/plugin.toml:6 says "two languages, and three graded fixtures" and :20 says "These three files are modelled on…" — while declaring five. Same stale count in tests/packages/README.md, crates/daemon/tests/timeline_package_e2e.rs:102 and ruby_package_e2e.rs. Not fixed deliberately: every byte under tests/packages/<pkg>/ is packed, so editing it is a package version bump plus a digest re-record — the same coupling class this issue is about, one level down.

2. A gate of ours has a population defect. posix_script_gate::no_test_spawns_a_checked_in_script_as_an_image uses a whole-file predicate (code.contains("Command::new")) for a call-scoped hazard. release_gate.rs has spawned git and bash for unrelated reasons for a long time; the moment this change put the string .forgejo/scripts/… into that file, it became an offender with no script being spawned. The gate was right about the code I had actually written, so it cost nothing real — but the rule will pull in any future file that merely quotes a script path beside an unrelated Command::new, and its only escape hatch is #![cfg(unix)], which the same message tells you not to use. Filing separately.

Verification

fmt clean · clippy --workspace --all-targets -D warnings clean · cargo test --workspace --no-fail-fast exit 0, 333 suites, 3619 tests, 0 panics · RUSTDOCFLAGS="-D warnings" cargo doc --document-private-items clean · YAML safe_load and bash -n over every touched run: body. Post-merge by me: release_fixture_gate 3 passed, release_gate 33 passed, both EXIT=0.

Closing.

Fixed on `master` at `b36d2c8` (merged `34b0fd5`). **And this issue understates the blast radius: there were FIVE sites, not two — and the one it missed is the one that ships a wrong number to operators.** ## The five | site | what it is | |---|---| | `release.yml:2847` / `:3339` | the two `GRADED` comparisons this issue names | | `release.yml:2869` / `:3365` | `grep -qE '^conformance .*passed C1 over 5 fixture$s$, C2 facts compared for 5'` — same defect, **two more literals each** | | `release.yml:3989` | `BODY+="The five fixtures shipped inside it are synthetic…"` | Fixing only the two named sites would have satisfied this issue's text **and still failed its own requirement**: a package grown to six fixtures reds at `:3365`, and publishes *"The five fixtures"* regardless. The reporter diffed the copied job and found the loudest instance; the quiet one fails no release — it just tells operators something untrue. All five now go through one shared script, `.forgejo/scripts/require_fixtures_graded.sh`, following `require_free_disk.sh`'s established pattern (same directory, same "one implementation, two doors, graded by a Rust test" idiom). Option 1 throughout — **no fallback was needed**: both package jobs already read the fingerprint and digest record from the checkout *before* their `cd scratch`, so the manifest is reachable exactly there. ## An extra defect found while wiring the notes That step has **no `set -e`** — its `run:` opens with `apt-get`, not `set -euo pipefail`. A failed derivation would have left the variable empty and published: ``` The fixtures shipped inside it are synthetic… ``` Guarded with an explicit `-z` refusal, and the guard is graded (W7 → RED). ## The both-sites proof, re-run by me `both_package_jobs_are_graded_by_the_same_clause` lifts every invocation **out of the YAML** — jobs derived, never listed — substitutes only `${ROOT}` and the transcript paths, and runs each job's own command. Failures are collected rather than asserted in the loop, so a shared-clause mutation is *seen* to hit both. Changing one line in the script: ``` BEFORE 178: if [ "$GRADED" -ne "$DECLARED" ]; then AFTER 178: if [ "$GRADED" -eq "$DECLARED" ]; then 2 of 5 lifted command(s) wrong: package-plugin (exit 1) …/tests/packages/xaml/plugin.toml package-plugin-timeline (exit 1) …/tests/packages/timeline/plugin.toml ``` Because the jobs are derived from the file, a **future** package job is covered without anyone remembering to add it. ## Two survivors that changed the work Both reported as survivors first, then fixed — and this is the most instructive part: **W5 SURVIVED.** The first offender rule was "a line mentioning `fixture` and carrying a **digit**". `The five fixtures` has no digit, so **the tree's actual defect text walked straight through the gate written to catch it.** Fixed with number *words*, matched whole so `one` cannot fire on `none`. **Then the widened rule turned the CORRECT line red:** `"The ${TL_FIXTURES} fixtures … the worked examples in the two format specifications"`. A gate that fires on the fixed text is worse than one that misses the broken text. Final rule: the number must sit within three tokens of the word it counts, and **both halves are pinned** in `the_fixture_count_predicate_knows_a_count_from_a_number`. Also recorded honestly: **S5** (`-ne "$DECLARED"` → `-lt`) reds the "too many" arm but **SURVIVES** `a_package_that_grows_a_fixture_still_packs`, because both of that test's arms still hold under `-lt`. And **W2** (point one job at the other's manifest) reds the source-shape test but **SURVIVES** the executing one, which builds its transcript from the manifest the command itself names — so a wrong manifest is self-consistent. That division of labour is stated rather than papered over. ## What this is NOT verified against The release workflow cannot be dispatched from here, so none of this is measured: - that a runner reaches these lines, or that the checkout is present at that point (inferred from `require_free_disk.sh` being invoked by relative path in the same jobs); - that `${ROOT}` holds the repo root at runtime — it is `$(pwd)` captured before `cd scratch`, beside the existing fingerprint read, but that is reasoning about the step, not a run of it; - that the script is executable on the runner image (`100755` is committed; the image's `sh` is not exercised); - **the largest hole:** that real `plugin check` / `plugin status` output has the shape the staged transcripts imitate. The shape is read out of `crates/cli/src/plugin.rs::facts_lines` — a reading of the producer, not a recording of a run. If the real output indents differently the greps count 0, and the zero-floor does not fire because it guards the *manifest* side. ## Two findings not fixed **1. The manifest's own prose is stale about itself.** `tests/packages/timeline/plugin.toml:6` says *"two languages, and three graded fixtures"* and `:20` says *"These three files are modelled on…"* — while declaring **five**. Same stale count in `tests/packages/README.md`, `crates/daemon/tests/timeline_package_e2e.rs:102` and `ruby_package_e2e.rs`. Not fixed deliberately: **every byte under `tests/packages/<pkg>/` is packed**, so editing it is a package version bump plus a digest re-record — the same coupling class this issue is about, one level down. **2. A gate of ours has a population defect.** `posix_script_gate::no_test_spawns_a_checked_in_script_as_an_image` uses a **whole-file** predicate (`code.contains("Command::new")`) for a **call-scoped** hazard. `release_gate.rs` has spawned `git` and `bash` for unrelated reasons for a long time; the moment this change put the string `.forgejo/scripts/…` into that file, it became an offender with no script being spawned. The gate was right about the code I had actually written, so it cost nothing real — but the rule will pull in any future file that merely *quotes* a script path beside an unrelated `Command::new`, and its only escape hatch is `#![cfg(unix)]`, which the same message tells you not to use. Filing separately. ## Verification `fmt` clean · `clippy --workspace --all-targets -D warnings` clean · `cargo test --workspace --no-fail-fast` **exit 0, 333 suites, 3619 tests, 0 panics** · `RUSTDOCFLAGS="-D warnings" cargo doc --document-private-items` clean · YAML `safe_load` and `bash -n` over every touched `run:` body. Post-merge by me: `release_fixture_gate` 3 passed, `release_gate` 33 passed, both EXIT=0. Closing.

buildagent closed this issue

2026-09-09 22:06:24 +02:00