fix(756): extend the dropped-step guard to docker-build.yml's required jobs, where a drop is fail-OPEN #768

Merged

timothy merged 8 commits from fix/756-dropped-step-guard-docker-build into main 2026-08-11 01:35:09 +02:00

Conversation 1 Commits 8 Files Changed 8

+1377 -32

timothy commented 2026-08-11 00:20:25 +02:00

Owner

Copy Link

Copy Source

fixes #756

A run: body the runner declines to interpolate is dropped, and the job still concludes success (#751, ci.workflow-run-body-no-expressions). #751 fixed that in review-verdict.yml, where the failure is fail-CLOSED. It left the two places where the same drop is fail-OPEN: Build & test (.NET) and EF migration integrity (SQLite + MySql) are the other two required contexts on main, so a dropped step there sends a required check green having done no work.

What landed

Per-step markers, not per-job. The issue proposed a marker written by each job's first step. That only proves the job began; the drop that costs something is Test, Build or a migration replay, all well past step one. So all 12 consequential run: steps in test and all 6 in migrations record that they started, and a trailing guard asserts the whole expected set. Advisory continue-on-error steps are exempt by construction.

The guard carries no if: — a deliberate departure from #751's always(). With a dozen steps, a genuine early failure legitimately skips every later one, and always() would announce a false "these steps never executed" on top of every ordinary red build. The default success() is correct because guard skipped ⇒ job red.

The cheaper, more general half too: a ban on the raw ${{ opener in test, migrations and build, which makes the class unreachable rather than merely caught. build is in because its Smoke + IPTV E2E runs after Build and push — a drop there publishes a release candidate that was never booted, and prod promotion pulls exactly that image. Its two payloads moved into env:, so the ban was free.

Measured, not argued — three live build-lane probes

Probe	Setup	Result
#765 job 8064	test, a step deliberately dropped	Guard caught it; the only ❌ in the job → red where it would have been green
#765 job 8065	migrations, untouched	All 6 expected step(s) executed → green
#766 job 8075	test, advisory step made to exit 1	Advisory ❌, guard still ran, All 12 expected step(s) executed → green

The third settles the one path where the if:-less design could have been a silent no-op. Both probe PRs are closed and their branches deleted.

Verification

510 tests green; 30 mutations killed across two harnesses; decisions-validate --base origin/main clean. Five review rounds across two model families (Codex + Claude), each starting cold.

Two findings were in the tests, not the code, and are worth naming: a fixture that never sent the guard its production gate value (false), hiding a one-token fail-open in the central predicate; and a positive control that passed whether or not the mechanism worked. A green suite was never the signal.

Known residual — tracked, not hidden

The build ban is enforced only by script-tests, which is on: pull_request and not a required context, so it is a review-time check rather than a fail-closed gate on the release path. Strictly better than before, not closed — #767, with the deferral reason (Smoke's if: carries a publish-ref condition the bucket protocol does not model) and an explicitly retracted cost estimate.

GITHUB_RUN_ATTEMPT is warn-and-default rather than refuse, because its presence is not established either way. The guard now prints Marker identity: … attempt=… (from the runner | DEFAULTED …) on stdout, so this PR's own run answers it — if it reads "from the runner", promoting it is a one-line follow-up.

fixes #756 A `run:` body the runner declines to interpolate is **dropped, and the job still concludes `success`** (#751, `ci.workflow-run-body-no-expressions`). #751 fixed that in `review-verdict.yml`, where the failure is fail-CLOSED. It left the two places where the same drop is **fail-OPEN**: `Build & test (.NET)` and `EF migration integrity (SQLite + MySql)` are the other two required contexts on `main`, so a dropped step there sends a required check green having done no work. ## What landed **Per-step markers, not per-job.** The issue proposed a marker written by each job's first step. That only proves the job *began*; the drop that costs something is `Test`, `Build` or a migration replay, all well past step one. So all 12 consequential `run:` steps in `test` and all 6 in `migrations` record that they started, and a trailing guard asserts the whole expected set. Advisory `continue-on-error` steps are exempt by construction. **The guard carries no `if:`** — a deliberate departure from #751's `always()`. With a dozen steps, a genuine early failure legitimately skips every later one, and `always()` would announce a false *"these steps never executed"* on top of every ordinary red build. The default `success()` is correct because **guard skipped ⇒ job red**. **The cheaper, more general half too:** a ban on the raw `${{` opener in `test`, `migrations` and `build`, which makes the class *unreachable* rather than merely caught. `build` is in because its `Smoke + IPTV E2E` runs **after** `Build and push` — a drop there publishes a release candidate that was never booted, and prod promotion pulls exactly that image. Its two payloads moved into `env:`, so the ban was free. ## Measured, not argued — three live build-lane probes | Probe | Setup | Result | |---|---|---| | #765 job 8064 | `test`, a step deliberately dropped | Guard caught it; the **only** ❌ in the job → red where it would have been green | | #765 job 8065 | `migrations`, untouched | `All 6 expected step(s) executed` → green | | #766 job 8075 | `test`, advisory step made to `exit 1` | Advisory ❌, **guard still ran**, `All 12 expected step(s) executed` → green | The third settles the one path where the `if:`-less design could have been a silent no-op. Both probe PRs are closed and their branches deleted. ## Verification 510 tests green; **30 mutations killed** across two harnesses; `decisions-validate --base origin/main` clean. Five review rounds across two model families (Codex + Claude), each starting cold. Two findings were in the *tests*, not the code, and are worth naming: a fixture that never sent the guard its production gate value (`false`), hiding a one-token fail-open in the central predicate; and a positive control that passed whether or not the mechanism worked. A green suite was never the signal. ## Known residual — tracked, not hidden The `build` ban is enforced only by `script-tests`, which is `on: pull_request` and **not** a required context, so it is a review-time check rather than a fail-closed gate on the release path. Strictly better than before, not closed — **#767**, with the deferral reason (`Smoke`'s `if:` carries a publish-ref condition the bucket protocol does not model) and an explicitly retracted cost estimate. `GITHUB_RUN_ATTEMPT` is warn-and-default rather than refuse, because its presence is not established either way. The guard now prints `Marker identity: … attempt=… (from the runner | DEFAULTED …)` on stdout, so **this PR's own run answers it** — if it reads "from the runner", promoting it is a one-line follow-up.

timothy added 7 commits 2026-08-11 00:20:26 +02:00

fix(756): per-step execution markers on docker-build.yml's two REQUIRED jobs ... 2c6b0a39de

A run: body the runner declines to interpolate is DROPPED and the job still
concludes success (#751). In review-verdict.yml that is fail-CLOSED; in
docker-build.yml's `test` and `migrations` — the other two required contexts on
main — it is fail-OPEN: the required check goes green having done no work.

Every consequential run: step now records that it began, and a trailing guard
fails the job when the expected set is incomplete. Per STEP, not per job as
proposed: a job-start marker only proves step 1 ran, and the dangerous drop is
`Test` or the migration replay.

Refs #756

docs(756): record the per-step marker contract and its build-lane measurement ... 7e769c1b1f

Adds ci.required-job-step-execution-markers, updates
ci.workflow-run-body-no-expressions (whose rule: named #756 as the open
residual and whose scope paragraph was tracking REQUIRED contexts rather
than one file), and documents the guard in docs/ci-cd.md.

Decisions-Edit: yes
Refs #756

fix(756): round-2 — nine findings from two independent cold reviews ... b0ed61d3d5

Marker identity no longer falls back silently: GITHUB_JOB/GITHUB_RUN_ID are
measured present and now REFUSE when absent, because the old nojob/norunid/1
defaults are REUSABLE and a stale marker there makes the guard PASS on a
dropped run. GITHUB_RUN_ATTEMPT is NOT measured (absent from the probe's
2.9MB log), so it warns and defaults rather than reddening every merge in the
repo over an assumption — these are required checks and over-strictness is not
free.

The delimiter ban now matches the raw `${{` OPENER, not a closed pair: an
unclosed opener triggers the same rewrite and evaded the 'absolute' rule. It
also covers `build`, whose Smoke + IPTV E2E step runs AFTER Build and push —
a drop there publishes a release candidate that was never booted and that
prod promotion then pulls. Its two payloads moved into env:, so the ban was
free. The record's claim that non-required jobs are 'a smaller cost' was
false for exactly that step.

Also: reject any marked step whose if: the guard protocol cannot model (it was
silently bucketed --always, which would redden a required context on a run
where the step is legitimately skipped); assert the guard's env VALUES, not
just its names; re-check anti-vacuity AFTER the skip gate narrows the set
(reproduced: --always empty + gate fired printed 'All 0 expected step(s)
executed' and exited 0); parametrise the gate tests over both jobs; correct the
migrations guard comment, which was copied verbatim from test's and named keys
migrations does not have.

Docs: restore the build-lane measurement, which a dirty-tree `git checkout -b`
had swept onto a probe branch, and add the positive control measured in the
same run (migrations: All 6 expected step(s) executed, job success). Corrected
the RUNNER_TEMP rationale — that #751 measurement was taken on a job with no
container: and does not transfer to these two.

Decisions-Edit: yes
Refs #756

docs(756): measure the if:-less guard's one plausible counterexample ... daead0885b

The guard carries no if:, resting on 'guard skipped => job red'. The path
where that could be silently false is a continue-on-error step that FAILS: if
that flipped success(), the guard would be skipped on a still-green job — a
no-op produced by the very failure mode it exists to catch. Both advisory
steps sit immediately before it, so the path is live.

Probe PR #766 (run 1913 job 8075): the advisory step exits 1, the log carries
its ❌, and the guard STILL ran, reported 'All 12 expected step(s) executed',
and the job concluded success. The invariant holds. That run doubles as the
test job's twelve-key positive control on the build lane, which #765 could not
give (its test job was the negative case).

Decisions-Edit: yes
Refs #756

fix(756): round-3 — the harness could not see its own central predicate break ... 93115b1f27

The important one: no behavioural test ever drove the guard at the gate value
the runner actually sends. _env() left ETV_DOCS_ONLY/ETV_REVALIDATE_SKIP
UNSET, and unset happens to behave correctly. Cold review mutated the gate
from `= "true"` to `-n` and all 30 tests stayed GREEN, while the guard run
with the real environment reported 'Skip gate fired (docs_only=false) … All 2
expected step(s) executed' and exited 0 — Build, Test and both migration
replays unguarded on every ordinary run, with the guard announcing it had
proved everything. Read off the cited probe's own log: ETV_DOCS_ONLY: false.
_env() now defaults both to `false` and the positive control is parametrised
over false/empty/unset; the mutation is killed by the drop test on both jobs.

Also: the sibling record ci.workflow-run-body-no-expressions still asserted
that `build` interpolates legitimately and that 'a drop there fails no gate' —
the exact claim the new record was written to refute, and it is the `rule:`
field the catalog surfaces. Corrected, along with its occurrence count.

The measurement was off by one: ten marked steps follow the drop, eleven
markers survived in total (detect being the earlier eleventh). Three copies of
the retracted RUNNER_TEMP rationale and two of the retracted 'scoped to the
required pair' survived their own retraction — swept by subject. The script
also cited the probe as evidence for a claim (container-private /tmp) the
probe never took; that follows from `container:`, not from a measurement.

Relaxed the guard-env assertion from whole-dict equality to per-key, so an
added variable or ${{x}} spacing no longer false-reds a required check.

Honest residual, now stated in both docs and tracked as ersatztv#767: the
`build` ban is enforced only by script-tests, which is on: pull_request and
NOT a required context — so it is a review-time check, not a fail-closed gate
on the tag/release path. Not closed here because Smoke's if: carries a third
condition the bucket protocol does not model, and measuring a guard there
would mean cutting a real release tag.

Decisions-Edit: yes
Refs #756

fix(756): round-4 — a VOIDED trailer block, and three claims contradicted by evidence ... 52c76d97fb

The blocker: every one of the five commits carried `Decisions-Edit: yes`, and
git parsed NONE of them. `Refs #756` has no colon, so it is not a trailer, and
one bad line voids the whole block — `git log --format='%(trailers:key=...)'`
returned empty for all five and `decisions_validate.py --base origin/main`
failed. It would have surfaced as a red `decisions lifecycle`, which is the one
red this repo is standing-instructed to treat as a known flake. This commit's
trailer block is clean; `Refs:` now carries its colon.

Committed the gate value MATRIX (16 cases). Cold review showed round 3's
`gate-unset` parametrisation was vacuous in the same way round 3 itself had
just fixed: the positive control still passes when the gate wrongly skips,
because the guard prints "All 2 expected step(s) executed" and exits 0. Only a
test asserting skip-iff-exactly-true discriminates, and `(true, true)` — a
docs-only merge to main on an already-validated tree — had no coverage at all.

Three claims were wrong rather than merely stale:
  * The workflow comment said `build` gets no markers because "it is not a
    required context, so they would buy little" — the exact dismissal the
    record names as the claim not to repeat. Markers there would buy MORE than
    the ban, which only runs on pull_request.
  * The #767 deferral claimed measuring a guard on the release path "would mean
    cutting a real release tag". False: `build` runs on every push to main and
    Smoke's ref condition admits refs/heads/main.
  * "Both advisory steps" — there are three.

On GITHUB_RUN_ATTEMPT: cold review argued it IS set, from the absence of the
script's warning in the probe logs. That needs step stderr to reach the log,
and the control offered was an `::error::` this script writes to STDOUT — so
the premise was unestablished in both directions, and I did not adopt it.
Instead `assert` now prints its resolved identity on stdout every run, where
capture is not in question; read that line on this PR's run and promote the
variable if it says "from the runner". Adding that reporting introduced, and
the suite immediately caught, a real bug: marker_path's STDOUT *is* the path,
so the notice printed there landed inside it.

Decisions-Edit: yes
Refs: #756

fix(756): round-5 — kill three surviving mutations on the identity line ... bd152bc46f

The final review verdict was MERGEABLE, but demonstrated three mutations of
round 4's reporting line surviving a 50-green suite: deleting the echo,
mis-deriving the attempt (`${marker#*-}` for `##`), and inverting the
provenance test. That line is a documented contract — the record's mechanics:
asserts it — and the decision to promote GITHUB_RUN_ATTEMPT to the refuse
bucket is meant to be made by READING it off a run log. An unasserted
reporting line that the operator is told to trust is the same shape as the
rest of this issue. Two assertions; all three mutations now die.

Also: ETV_ATTEMPT_DEFAULTED was a non-local global with no else branch, so the
same name inherited from the environment made the line self-contradictory
(`attempt=5 (DEFAULTED — GITHUB_RUN_ATTEMPT unset)`). Reachable, since ETV_* is
a namespace the guard's own env: writes into. Computed inline now.

And the workflow comment claimed ci.required-job-step-execution-markers
"explicitly names" a dismissal it does not name — the record's do-not-repeat
sentence is about a different clause. A wrong cross-reference in a commit whose
subject is wrong claims about other files.

Refs: #756

timothy referenced this issue from a commit 2026-08-11 00:59:21 +02:00

fix(756): promote GITHUB_RUN_ATTEMPT to required — its presence is now MEASURED

timothy added 1 commit 2026-08-11 00:59:21 +02:00

fix(756): promote GITHUB_RUN_ATTEMPT to required — its presence is now MEASURED ... 0d0518d3cb

PR #768's own run answered the question the identity line was added to answer:

  Marker identity: job=test       run=1916 attempt=1 (from the runner)
  Marker identity: job=migrations run=1916 attempt=1 (from the runner)

Both required jobs, on the build lane, printed to stdout where capture is not
in question. So the variable IS exported here, the warn-and-default compromise
has served its purpose, and all three identity components now REFUSE rather
than falling back to a name other runs share. The residual that compromise
left — a rerun inheriting attempt 1's markers — no longer exists.

Recording HOW this was settled, because the first two attempts were worthless
and the next reader will be tempted by both. Grepping a job log for the
variable NAME proves nothing: logs do not dump the environment. Inferring it
from the ABSENCE of the script's "not set" warning proves nothing either — that
warning goes to stderr, and whether step stderr reaches a job log here was
itself never established (the control offered for it turned out to be an
::error:: this script writes to STDOUT). Only making the script report its own
resolved identity on stdout, and then reading it, settled anything.

The docs said "not established either way", which this run falsifies — so the
prose had to change regardless; promoting was the same edit surface.

The identity line is kept, now as standing evidence rather than an open
question, and is still asserted so it cannot rot into a line nobody can trust.

Decisions-Edit: yes
Refs: #756

timothy commented 2026-08-11 01:34:34 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 0d0518d

Five cold review rounds across two model families (Codex GPT-5.x, Claude Opus), each from a review-only brief. Final verdict MERGEABLE; its three findings were fixed in round 5 and the surviving mutations it demonstrated are now killed. Verification: 510 tests green, 30 mutations killed across two harnesses, decisions-validate clean. Behaviour measured live on the build lane in both directions (probe 765: drop caught, sole failure in the job; probe 766: failing continue-on-error step does not skip the guard) plus this PR's own run reporting All 12 and All 6 expected steps executed. Known residual tracked as 767: the build delimiter ban is review-time only, not fail-closed on the release path.

Review-verdict: MERGEABLE @ 0d0518d Five cold review rounds across two model families (Codex GPT-5.x, Claude Opus), each from a review-only brief. Final verdict MERGEABLE; its three findings were fixed in round 5 and the surviving mutations it demonstrated are now killed. Verification: 510 tests green, 30 mutations killed across two harnesses, decisions-validate clean. Behaviour measured live on the build lane in both directions (probe 765: drop caught, sole failure in the job; probe 766: failing continue-on-error step does not skip the guard) plus this PR's own run reporting All 12 and All 6 expected steps executed. Known residual tracked as 767: the build delimiter ban is review-time only, not fail-closed on the release path.

timothy merged commit 884ac8a7e9 into main 2026-08-11 01:35:09 +02:00

timothy deleted branch fix/756-dropped-step-guard-docker-build 2026-08-11 01:35:10 +02:00

timothy referenced this pull request 2026-08-11 01:35:59 +02:00

Extend the dropped-step guard to docker-build.yml's required jobs, where a dropped step is fail-OPEN #756

timothy referenced this pull request 2026-08-13 19:58:48 +02:00

Test the deny path with the production config value, and assert full-replace field lists #779

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#768