From e965b15cbfa5fb583da899c915f58f8628865935 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:36:17 +0000 Subject: [PATCH 1/6] docs: the recurring upstream sync, its standing resolutions, and a guard on where the delta lives (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The third child of #197, written immediately after performing the sync it describes, while the findings are still first-hand. docs/UPSTREAM-SYNC.md carries the procedure and the six standing resolutions, each with the issue that decided it, so they are not re-argued every sync. The parts that are not obvious from the outside, and that the 0.6.0 sync paid to learn: * THE AUDIT STEP. `git merge` takes upstream's side wherever only upstream moved a region, so a function upstream ADDED to a file this tree owns arrives with no conflict and no question. Reviewing the hunks cannot find it — four reviewers read the same diff and each found a different subset. That was eight runtime `gh` call sites in three files and two file types. * THE SAME MECHANIC APPLIES TO STATE. A resolved region can remove a producer whose consumers auto-merged, and those consumers degrade to empty rather than erroring, so nothing goes red. Three such seams in one sync. * VERIFY WHERE IT RUNS. "Green locally" was wrong three times, for three different reasons: shellcheck-all lints TRACKED files so a new file's first lint is meaningless; CI pins shellcheck 0.10.0; and the runner's jq 1.6 exits 0 where 1.7 exits 4 on `jq -e` with empty input — which was not a test problem but a guard accepting an unreadable read. * TEST THE MERGE RESULT. Forgejo tests heads, never what two branches produce together, and two green PRs did produce a red tree in this sync. * AFTER MERGING, CHECK THE SWEEP RECONCILED SOMETHING. The first post-merge run was green and had done nothing. .upstream-ref records the carried commit in machine-readable form beside the CHANGELOG's prose. test/upstream-delta.test.sh asserts every forge-DECIDING file is named in the inventory — offline, comment-aware, and refusing rather than skipping when the ref is missing. Shim CONSUMERS are allowed by name, so a seventh consumer is silent and a seventh decider is not. docs/CONSUMERS.md now states that two ceremonies answer to the same version number and how a consumer says which one it pinned. Must-fail, both from the issue's test plan: scattering a forge_detect branch into an unlisted file reds the guard; blanking .upstream-ref reds it too. test/run.sh 29 files 0 failed under jq 1.7 and jq 1.6; shellcheck 0.10.0, actionlint, self-ref, marker, vendored and changelog-armed all clean. Refs #200 --- .upstream-ref | 4 + changelog.d/200.md | 31 ++++++ docs/CONSUMERS.md | 20 ++++ docs/UPSTREAM-SYNC.md | 192 ++++++++++++++++++++++++++++++++++++ test/upstream-delta.test.sh | 164 ++++++++++++++++++++++++++++++ 5 files changed, 411 insertions(+) create mode 100644 .upstream-ref create mode 100644 changelog.d/200.md create mode 100644 docs/UPSTREAM-SYNC.md create mode 100644 test/upstream-delta.test.sh diff --git a/.upstream-ref b/.upstream-ref new file mode 100644 index 0000000..2a4b4db --- /dev/null +++ b/.upstream-ref @@ -0,0 +1,4 @@ +# The upstream commit this tree carries (docs/UPSTREAM-SYNC.md). +# Updated by each sync; read by test/upstream-delta.test.sh. +# github.com/heavy-duty/ceremony +8c3a4d1 diff --git a/changelog.d/200.md b/changelog.d/200.md new file mode 100644 index 0000000..3aa9a18 --- /dev/null +++ b/changelog.d/200.md @@ -0,0 +1,31 @@ +### Added + +- `docs/UPSTREAM-SYNC.md` — the recurring upstream sync as a runbook: the + standing resolutions, which side wins each and the issue that decided it + (#200). + +- It names the step the 0.6.0 sync nearly shipped without: auditing what the + merge brought in that did **not** conflict. `git merge` asks no question + about a function upstream added to a file this tree owns (#200). + +- It records that the same mechanic applies to state, not just to call sites: a + resolved region can silently remove a producer whose consumers auto-merged, + and every one of those consumers degrades to empty rather than erroring + (#200). + +- It says to verify with the runner's tooling, because "green locally" was + wrong three times in one sync — untracked files, a pinned linter, and a + pinned `jq` whose empty-input exit code differs (#200). + +- It says to test the merge RESULT when other PRs are open: Forgejo tests + heads, so two green branches can produce a red tree (#200). + +- `.upstream-ref` records the upstream commit this tree carries, in + machine-readable form beside the CHANGELOG's prose (#200). + +- `test/upstream-delta.test.sh` fails the PR that scatters forge branching into + a file the inventory does not name — offline, and refusing rather than + skipping when the recorded ref is missing (#200). + +- `docs/CONSUMERS.md` states that two ceremonies answer to the same version + number, and how a consumer says which one it pinned (#200). diff --git a/docs/CONSUMERS.md b/docs/CONSUMERS.md index 682a61b..0454f32 100644 --- a/docs/CONSUMERS.md +++ b/docs/CONSUMERS.md @@ -687,6 +687,26 @@ Bumping the pin re-syncs the mirror in the same PR — ## Version pinning +**Two ceremonies answer to the same version number.** `heavy-duty/ceremony` +exists on GitHub and on `forgejo.heavyduty.builders`, and the forge tree tracks +upstream's version numbers deliberately (ceremony#197 D2) — so `0.6.0` names a +different tree on each, differing by the forge-compatibility delta. They are +not forks that drifted: the forge tree carries upstream's content and adds to +it (`docs/UPSTREAM-SYNC.md`). + +What that means for a consumer: + +- **Name the forge you pinned, not just the tag.** `heavy-duty/ceremony@0.6.0` + is ambiguous on its own; the host in your `uses:` line is what disambiguates + it, so do not describe your pin anywhere without it. +- **A tag that exists upstream may not exist here yet.** The forge tree's + `CEREMONY_SELF_REF` takes upstream's number as soon as the sync lands, which + is *before* the release ceremony cuts that tag here. Do not bump a pin to a + version whose tag you have not confirmed on the forge you consume from. +- **The forge tree's `CHANGELOG.md` header names the upstream commit it + carries**, and `.upstream-ref` records the same SHA. That is how you tell + which `0.6.0` you are actually running. + - **Pin an exact ceremony release tag** — `@0.1.0`, never a branch and never a moving major pointer: the family pins things and reviews updates ([#1 D2](https://github.com/heavy-duty/ceremony/issues/1)). diff --git a/docs/UPSTREAM-SYNC.md b/docs/UPSTREAM-SYNC.md new file mode 100644 index 0000000..3393b22 --- /dev/null +++ b/docs/UPSTREAM-SYNC.md @@ -0,0 +1,192 @@ +# Syncing this tree with upstream ceremony + +`heavy-duty/ceremony` exists on two forges and they diverge in opposite +directions on purpose: + +- **upstream** — `github.com/heavy-duty/ceremony`, where new ceremony features + are written. **Read-only from here.** No issue, PR, comment, review or + release is ever created there. +- **this tree** — `forgejo.heavyduty.builders/heavy-duty/ceremony`, which + carries upstream's content plus the forge-compatibility delta and never + writes back. + +This document is the procedure for bringing upstream's work across. It is +written to be followed without prior context; where it states a resolution, the +resolution is standing and does not get re-decided each sync. + +Worked example throughout: the `0.6.0` sync (#197, #198), which merged upstream +`8c3a4d1` onto `dad99dd` and took four heads to get green. + +## The standing resolutions + +These recur every sync. They are decided; re-deciding them is the cost this +list exists to remove. + +| what | which side wins | decided by | +|---|---|---| +| `VERSION` | **upstream** — this tree tracks upstream's version numbers | #197 D2 | +| `CEREMONY_SELF_REF` (both carriers) | **upstream** | #197 D2 | +| `.github/labels.conf` | **this tree** — upstream's roster names identities that do not exist here | #195 | +| `drills/*.md` | **this tree** — a drill record is a record of a run *here* | #198 | +| `CHANGELOG.md` | **both**, upstream's new sections above this tree's | #198 | +| a section for a version **both** trees released | **this tree's** — ours is the published body of the tag that exists here | #198 | + +Two consequences worth stating plainly: + +- **Two trees answer to the same version number**, differing by the forge + delta. That is accepted, not accidental (#197 D2). The mitigation is + provenance in prose: `CHANGELOG.md`'s header names the upstream commit this + tree carries, and `.upstream-ref` records it in machine-readable form. +- **A tag that exists upstream may not exist here.** `CEREMONY_SELF_REF` takes + upstream's number, and both workflows carry the self-consumption bypass + (`if: github.repository != 'heavy-duty/ceremony'`), so ceremony's own CI is + unaffected. But **no consumer may bump its pin to that number until the + release ceremony cuts the tag here.** + +## The procedure + +### 1. Add the upstream remote, read-only, and confirm the merge base + +```sh +git remote add gh https://github.com/heavy-duty/ceremony.git # if absent +git fetch gh +git merge-base main gh/main +``` + +**Confirm the merge base against `.upstream-ref` before merging anything.** If +it is not what the last sync recorded, something moved — stop and re-measure +rather than proceeding. A sync that starts from an unexpected base is a sync +whose conflict count means nothing. + +### 2. Merge, never rebase + +```sh +git merge gh/main +``` + +One merge commit, conflicts resolved once (#197 D1). Rebasing the forge-only +commits onto upstream would rewrite every SHA, re-resolve the same conflicts +once per commit, and break any pin to them. A fresh re-import would discard the +provenance in this repo's issue comments, which is where its documentation +actually lives. + +### 3. Resolve the conflicts + +Apply the standing resolutions above. What is left is genuinely new and needs +judgement — in the `0.6.0` sync that was 5 hunks of 18. + +### 4. Audit what the merge brought in that did NOT conflict + +**This is the step the `0.6.0` sync nearly shipped without, and the one this +document exists for.** + +`git merge` takes upstream's side wherever only upstream moved a region. So a +function upstream *added* to a file this tree already owns arrives with **no +conflict and no question asked**. Reviewing the conflict hunks cannot find +them: four reviewers read the same diff and each found a different subset. + +In the `0.6.0` sync that was **eight** runtime `gh` call sites, in three files +and two file types, every one of which #188 had previously removed. + +So, after resolving: + +```sh +bash test/no-runtime-gh.test.sh +``` + +That guard is the mechanical form of #197's acceptance bar — no runtime `gh` +outside `lib/forge-github.sh` unless the file declares +`CEREMONY_FORGE_CLIENT=gh` **and** refuses when it cannot run. Do not satisfy +it by adding an exemption; a declaration without a refusal is a permission slip +for `gh: command not found`. + +Then check the **variables** the same way, because the same mechanic applies to +state: if a conflicted region assigns something that auto-merged code consumes, +resolving it "to this tree's side" silently removes the producer. Every one of +those consumers degrades to empty rather than erroring, so nothing goes red. +The `0.6.0` sync had three such seams. Enumerate what each resolved region +assigns, and confirm each still has a producer. + +### 5. Port or declare every new `gh` call site + +Where a `forge_*` verb exists, port it in the merge itself. Where none does, +the file **declares** `CEREMONY_FORGE_CLIENT=gh` and refuses loudly, and the +port gets its own issue (#199 for `refs-not-closing`, #205 for the sweep +dispatch). "Never 'probably github'" applies to the sync as much as to a +runtime probe. + +A workflow cannot call `forge_preflight`, so it declares in its `env:` block +and refuses inline — deciding the **forge** first and the **binary** second. A +guard that only asks whether `gh` is installed passes the moment a runner image +ships it. + +### 6. Record the provenance + +- `CHANGELOG.md`'s header: which upstream commit this tree now carries. +- `.upstream-ref`: the same SHA, machine-readable, checked by + `test/upstream-delta.test.sh`. +- A `changelog.d/` fragment for the sync issue. + +### 7. Verify — and verify where it will actually run + +`test/run.sh` green on your machine is the weakest of the checks below. The +`0.6.0` sync was "green locally" and red on the runner **three times, for three +different reasons**: + +| what was green locally | why the runner disagreed | +|---|---| +| `shellcheck-all.sh` | it lints **tracked** files, and the new guard was untracked | +| the whole suite | CI pins **shellcheck 0.10.0**; a different local version reports differently | +| `issue_payload_valid` | `jq -e` on empty input exits **4** on jq 1.7 and **0** on jq 1.6 — and the runner image ships 1.6 | + +That last one was not a test problem: on jq 1.6 the guard that refuses an +unreadable read was *accepting* one. **The distance between your environment +and the runner's is part of the sync's risk surface, not an inconvenience.** + +So verify with the runner's own tooling: + +```sh +git add -A # or shellcheck sees nothing new +CEREMONY_REQUIRE_NPM=1 CEREMONY_REQUIRE_YQ=1 bash test/run.sh +bash .github/scripts/shellcheck-all.sh # pinned 0.10.0, as ci.yml installs +bash .github/scripts/actionlint-all.sh +bash .github/scripts/self-ref-check.sh +bash .github/scripts/marker-check.sh +bash .github/scripts/vendored-check.sh +bash actions/changelog-armed/changelog-armed.sh +``` + +and run the suite once under the runner's `jq` as well as your own. + +**And check the merge result, not just the head.** Forgejo tests branch heads; +it never tests what two branches produce together. Two PRs can each be green +and their merge red — that happened in this sync, because a rule the sync +*introduces* (#262's terminal citation) was not met by a fragment written +against the base that lacks it. If other PRs are open, merge them together +locally and run the suite on the result. + +### 8. After it merges + +The sync issue uses `Refs`, not `Closes`, and stays open until a real sweep on +the merged `main` is linked to it. A green workflow run is not that evidence by +itself: verify the run actually **reconciled** something. In this sync the +first post-merge run was green and had done nothing at all, because upstream's +restructure moved reconcile behind a dispatch this forge cannot perform. + +## Where the forge delta lives + +Forge-specific behaviour is confined to the files below. Keeping it there is +what makes each sync cost 18 hunks instead of hundreds, and +`test/upstream-delta.test.sh` fails the PR that scatters it into a new file. + +| file | what is forge-specific about it | +|---|---| +| `lib/forge.sh` | the selector: `forge_detect`, `forge_client`, `forge_preflight` | +| `lib/forge-github.sh` | the gh backend — the one file allowed to speak `gh` | +| `lib/forge-forgejo.sh` | the Forgejo backend, `/api/v1` over curl + jq | +| `lib/closes_references.sh` | the closing-keyword parser that replaced GraphQL | +| `.github/labels.conf` | this instance's roster | +| `drills/` | records of runs on this instance | + +If a sync needs forge branching somewhere else, that is a design decision, not +a detail: add the file to the inventory in the same PR, with the reason. diff --git a/test/upstream-delta.test.sh b/test/upstream-delta.test.sh new file mode 100644 index 0000000..35b56b0 --- /dev/null +++ b/test/upstream-delta.test.sh @@ -0,0 +1,164 @@ +#!/usr/bin/env bash +# The delta-inventory guard (#200, enforcing #197 D3's standing constraint). +# +# Forge-specific behaviour lives in the files docs/UPSTREAM-SYNC.md names, +# and nowhere else. +# +# WHY. This tree carries upstream's content plus a forge delta, forever — the +# sync is recurring and upstream is read-only (#197 D3). What makes that +# tractable is not luck: the 0.6.0 sync cost 18 conflict hunks across 10 files +# because the delta sits in six files upstream never touches. Scatter +# `forge_detect` into a seventh and every future sync pays for it, in a place +# nobody chose. +# +# So this fails on the PR that scatters it, rather than on the sync after next +# — the same reason changelog-monotonic guards a property no single tree can +# be asked about. +# +# IT RUNS OFFLINE, AND IT DOES NOT PASS BY ABSENCE. Tests must not need the +# network, and a guard that goes green when it cannot see its input is the +# blind-sweep shape this repo keeps writing issues about. So the inventory is +# read from the document and the tree is scanned for forge markers — both local +# — and a missing or empty `.upstream-ref` is a REFUSAL, not a skip. +set -u + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +# shellcheck source=test/harness.sh +. "$ROOT/test/harness.sh" + +DOC="$ROOT/docs/UPSTREAM-SYNC.md" +REF="$ROOT/.upstream-ref" + +# --- the recorded upstream ref ------------------------------------------------ + +check "the sync document exists" 0 "" test -f "$DOC" +check "the upstream ref is recorded" 0 "" test -f "$REF" + +recorded_ref() { grep -vE '^[[:space:]]*(#|$)' "$REF" | head -n1; } +ref_is_recorded() { [ -n "$(recorded_ref)" ]; } +check "...and is not blank — an unrecorded ref is a refusal, not a skip" 0 "" \ + ref_is_recorded +ref_looks_like_sha() { printf '%s' "$(recorded_ref)" | grep -qE '^[0-9a-f]{7,40}$'; } +check "...and looks like a commit SHA" 0 "" ref_looks_like_sha + +# The ancestry half needs upstream's objects, which a CI clone of THIS repo +# does not have. It is therefore conditional — but it says so out loud rather +# than passing quietly, because "could not check" and "checked and fine" are +# the two states this repo exists to keep apart. +ancestry_state() { + if git -C "$ROOT" cat-file -e "$(recorded_ref)^{commit}" 2>/dev/null; then + git -C "$ROOT" merge-base --is-ancestor "$(recorded_ref)" HEAD 2>/dev/null \ + && echo ANCESTOR || echo NOT-ANCESTOR + else + echo UNVERIFIABLE-HERE + fi +} +state="$(ancestry_state)" +printf 'upstream-delta: recorded ref %s is %s\n' "$(recorded_ref)" "$state" +ancestry_acceptable() { [ "$state" = ANCESTOR ] || [ "$state" = UNVERIFIABLE-HERE ]; } +check "the recorded ref is an ancestor of HEAD, or is honestly reported as unverifiable" 0 "" \ + ancestry_acceptable + +# --- the inventory ------------------------------------------------------------ + +# The document's inventory table: the first `code` span of each table row in +# the "Where the forge delta lives" section. +inventory() { + awk '/^## Where the forge delta lives/ { on = 1; next } + on && /^## / { on = 0 } + on && /^\| `/ { gsub(/^\| `/, ""); sub(/`.*$/, ""); print }' "$DOC" +} + +inventory_size() { inventory | wc -l; } +inventory_is_populated() { [ "$(inventory_size)" -ge 6 ]; } +check "the inventory names at least the six known delta files" 0 "" \ + inventory_is_populated + +in_inventory() { # $1 = repo-relative path + local entry + while IFS= read -r entry; do + [ -n "$entry" ] || continue + case "$1" in "$entry" | "$entry"*) return 0 ;; esac + done < <(inventory) + return 1 +} +check "a listed file is recognised" 0 "" in_inventory lib/forge.sh +check "...including one listed as a directory" 0 "" in_inventory drills/0.4.1.md +check "an unlisted file is not" 1 "" in_inventory lib/version.sh + +# --- the scan ----------------------------------------------------------------- + +# What "forge-specific" means, mechanically: the selector's own verbs and the +# environment override that drives them. Comments are stripped — this tree +# documents the forge split at length, and a guard that read prose as evidence +# would flag every file that merely explains the design. +FORGE_MARKERS='forge_detect|forge_select|forge_preflight|forge_client|CEREMONY_FORGE\b|CEREMONY_FORGE_CLIENT' + +forge_specific_files() { + local f rel + for f in "$ROOT"/lib/*.sh "$ROOT"/actions/*/*.sh "$ROOT"/bin/* \ + "$ROOT"/.github/scripts/*.sh; do + [ -f "$f" ] || continue + rel="${f#"$ROOT"/}" + sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$f" \ + | grep -qE "$FORGE_MARKERS" && printf '%s\n' "$rel" + done + return 0 +} + +# A call site that merely USES the shim is not forge-specific — every +# reconciler calls forge_preflight and that is the point of the shim. What the +# inventory is about is where the forge is DECIDED or BRANCHED ON. So the scan +# reports files carrying the markers, and the assertion below allows the shim's +# own consumers explicitly, by name, so adding a seventh consumer is silent but +# adding a seventh DECIDER is not. +SHIM_CONSUMERS='actions/issueflow-reconcile/issueflow-reconcile.sh +actions/labels-reconcile/labels-reconcile.sh +actions/labels-scope/labels-scope.sh +actions/refs-not-closing/run.sh +lib/facts.sh +lib/ruling.sh +lib/attention.sh' + +unlisted_deciders() { + local rel + while IFS= read -r rel; do + [ -n "$rel" ] || continue + grep -qxF "$rel" <<<"$SHIM_CONSUMERS" && continue + in_inventory "$rel" && continue + printf '%s\n' "$rel" + done < <(forge_specific_files) +} + +no_unlisted() { + local found + found="$(unlisted_deciders)" + [ -z "$found" ] || { + printf 'forge-specific but not in docs/UPSTREAM-SYNC.md:\n' >&2 + printf ' %s\n' "$found" >&2 + return 1 + } +} +check "every forge-deciding file is named in the inventory" 0 "" no_unlisted + +# --- teeth -------------------------------------------------------------------- +# The sweep above is a property of the whole tree and cannot be made to fail +# without editing it, so the predicates are driven directly. + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +# shellcheck disable=SC2016 # fixture CONTENT: the literal text a scanned file would hold +printf '%s\n' '#!/usr/bin/env bash' 'case "$(forge_detect)" in forgejo) : ;; esac' \ + >"$TMP/scattered.sh" +# Byte-identical to the strip forge_specific_files uses: a helper that +# normalised differently would be testing a predicate the scan does not have. +marker_seen() { + sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$1" | grep -qE "$FORGE_MARKERS" +} +check "a new file branching on forge_detect is seen" 0 "" marker_seen "$TMP/scattered.sh" +printf '%s\n' '#!/usr/bin/env bash' '# forge_detect used to live here' >"$TMP/prose.sh" +check "prose about forge_detect is not" 1 "" marker_seen "$TMP/prose.sh" +check "...and such a file would not be in the inventory" 1 "" in_inventory scattered.sh + +summary From f3f7538d15c8db9a45547589dae6cee63eccc2b7 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:42:21 +0000 Subject: [PATCH 2/6] docs(upstream-sync): stale in-flight branches, and auditing post-merge runs by executed steps (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl's two additions (#5697), both measured in the #198 sync rather than anticipated. Every branch open across a sync is stale afterwards: Forgejo never re-tests an open PR when main moves under it, so #206 and #207's green 22-file suites were about a tree that no longer existed once the 28-file one landed — and #206's fragment was individually green while making the combined tree red under a rule the sync itself introduces. The runbook now says to update each in-flight branch from the newly synced main, or check them in a scratch merge, and that a prior approval is evidence about the tree it was given on. And post-merge runs are audited by executed steps, never by colour: inventory what the sync changed about triggers and jobs, read which job actually ran, and treat a green refusal path as evidence for that path only. Run 326 was green and had reconciled nothing. Both failures happened with the no-runtime-gh guard green and CI green, so the runbook says that too. Refs #200 --- changelog.d/200.md | 8 ++++-- docs/UPSTREAM-SYNC.md | 60 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/changelog.d/200.md b/changelog.d/200.md index 3aa9a18..ced2b20 100644 --- a/changelog.d/200.md +++ b/changelog.d/200.md @@ -17,8 +17,12 @@ wrong three times in one sync — untracked files, a pinned linter, and a pinned `jq` whose empty-input exit code differs (#200). -- It says to test the merge RESULT when other PRs are open: Forgejo tests - heads, so two green branches can produce a red tree (#200). +- It says every branch open across a sync is stale afterwards — Forgejo never + re-tests an open PR when main moves, so a prior approval is evidence about a + tree that no longer exists (#200). + +- It says to audit post-merge runs by executed steps rather than colour, and to + inventory what the sync changed about workflow triggers and jobs first (#200). - `.upstream-ref` records the upstream commit this tree carries, in machine-readable form beside the CHANGELOG's prose (#200). diff --git a/docs/UPSTREAM-SYNC.md b/docs/UPSTREAM-SYNC.md index 3393b22..f4ae16f 100644 --- a/docs/UPSTREAM-SYNC.md +++ b/docs/UPSTREAM-SYNC.md @@ -158,20 +158,58 @@ bash actions/changelog-armed/changelog-armed.sh and run the suite once under the runner's `jq` as well as your own. -**And check the merge result, not just the head.** Forgejo tests branch heads; -it never tests what two branches produce together. Two PRs can each be green -and their merge red — that happened in this sync, because a rule the sync -*introduces* (#262's terminal citation) was not met by a fragment written -against the base that lacks it. If other PRs are open, merge them together -locally and run the suite on the result. +### Every branch that was open during the sync is now stale -### 8. After it merges +Forgejo tests branch heads; it never tests what two branches produce together, +and it never re-tests an open PR when `main` moves under it. So after a sync +lands, **every PR that was open across it is green against a tree that no +longer exists** — its run did not contain the test files and rules the sync +introduced. + +Both halves of that bit in this sync: + +- `#206` and `#207` were cut from the pre-sync base. Their green suites had 22 + test files; the merged tree has 28. +- `#206`'s changelog fragment was individually green and made the **combined** + tree red, because the terminal-citation rule (#262) arrives *with* the sync + and the fragment was written against a base without it. + +So, for each PR still open: + +```sh +git merge origin/main # in the branch — do not rewrite its commits +CEREMONY_REQUIRE_NPM=1 CEREMONY_REQUIRE_YQ=1 bash test/run.sh +``` + +or, if you are only checking rather than updating, merge them into a scratch +worktree together and run the full current suite and static guards there. A +prior approval is evidence about the tree it was given on; after a sync it is +not evidence about the tree the operator would merge. + +### 8. After it merges — audit by executed steps, never by colour The sync issue uses `Refs`, not `Closes`, and stays open until a real sweep on -the merged `main` is linked to it. A green workflow run is not that evidence by -itself: verify the run actually **reconciled** something. In this sync the -first post-merge run was green and had done nothing at all, because upstream's -restructure moved reconcile behind a dispatch this forge cannot perform. +the merged `main` is linked to it. + +**A green run is not that evidence.** In this sync the first post-merge run was +green and had reconciled *nothing*: upstream's #209 restructure moved reconcile +out of the labels caller and behind a dispatch this forge cannot perform, so +the only job that ran was the refusal. Green, correct, and proof of the +refusal path only. + +So before citing any run: + +1. **Inventory what the sync changed about workflow triggers and jobs** — which + jobs exist now, which events fire them, and which of those this forge can + actually serve. A restructure upstream can move work between workflows + without touching a line of the code that does it. +2. **Read the run's executed steps**, not its status. Name the job that did the + thing, and quote the line that shows it did. +3. A green *skipped-or-refusing* path is valid evidence **for that path**, and + never evidence that the work happened. + +Neither of these is caught by the no-runtime-`gh` scan in step 4: in this sync +both failures occurred with that guard green and CI green. ## Where the forge delta lives From 634e7a35287d16584c11fc56823776e6c44557a8 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:51:21 +0000 Subject: [PATCH 3/6] fix(upstream-delta): the object is mandatory, the scan covers every governed surface, and the teeth drive the real guard (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl's five points. Three were correctness, and one of them found that my must-fail cases could not fail. 1. THE OBJECT IS MANDATORY. UNVERIFIABLE-HERE is gone: a missing ref, an absent object and a non-ancestor are three distinct refusals. The ref is now the FULL 40-character SHA, and ci.yml fetches exactly that object before the suite. "Runs offline" means the TEST reads local evidence; it never meant CI may omit the evidence and pass. 2. THE SCAN COVERS WHAT THE INVENTORY CLAIMS. It walked shell under four globs and never looked at workflows, .github/labels.conf or drills/ — three categories the inventory governs. Widened, and it immediately found four real blind spots on merged main: refs-not-closing's declaration, labels.yml's inline forge decision, refs-guard.yml's GitHub-only scheduling and release-exercise.yml's pinned CEREMONY_FORGE. All four are now inventory entries with the issue that removes them, because a delta location with no exit is indistinguishable from one nobody noticed. A file that DECLARES a client is no longer exempt as a "consumer" — only files that merely CALL the shim are. 3. THE TEETH NOW DRIVE THE GUARD. They asserted the predicates separately and never invoked no_unlisted, so the guard could have been `return 0` and both must-fail rows would still have passed. SCAN_ROOT is a parameter now and the cases build a tree, add an unlisted decider — shell AND workflow, so coverage cannot regress to the old glob — and assert the real top-level check fails naming it. Replacing no_unlisted with `return 0` reds five. 4. PATH MATCHING, NOT PREFIX MATCHING. `drills/` accepted `drills-old/x` and `lib/forge.sh` accepted `lib/forge.sh.backup`. Exact for files, `dir/` for directories, with both negative boundaries covered. 5. THE IMMUTABLE SHA IS CAPTURED AT FETCH. The runbook now takes upstream_sha=$(git rev-parse gh/main) once and merges and records that value. This is not hypothetical: while this PR was in review upstream moved from 8c3a4d1 to 08e2912, and re-reading gh/main at recording time would have written a commit this tree does not contain. I caught that by walking into it. test/run.sh 29/29; upstream-delta 21/21; shellcheck 0.10.0, actionlint, changelog-armed clean. Refs #200 --- .github/workflows/ci.yml | 13 +++ .upstream-ref | 7 +- changelog.d/200.md | 12 ++- docs/UPSTREAM-SYNC.md | 30 ++++++- test/upstream-delta.test.sh | 166 +++++++++++++++++++++++++----------- 5 files changed, 170 insertions(+), 58 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2b65061..2a05151 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,6 +52,19 @@ jobs: # at the root that nobody added to docs/VENDORED.txt is invisible to # every consumer's docs-sync, so it fails CI here instead. run: bash .github/scripts/vendored-check.sh + - name: Fetch the recorded upstream commit + # test/upstream-delta.test.sh REFUSES when the recorded object is + # absent rather than calling it unverifiable (#200). "Runs offline" + # means the test reads local evidence — it does not mean CI may omit + # the evidence and pass. This step supplies it; the test never reaches + # the network itself. + run: | + ref="$(grep -vE '^[[:space:]]*(#|$)' .upstream-ref | head -n1)" + git fetch --no-tags --depth=1 \ + https://github.com/heavy-duty/ceremony.git "$ref" || { + echo "::error::could not fetch the recorded upstream commit $ref" >&2 + exit 1 + } - name: Tests env: # The npm-backed version_write case may skip locally when npm is diff --git a/.upstream-ref b/.upstream-ref index 2a4b4db..2337fa2 100644 --- a/.upstream-ref +++ b/.upstream-ref @@ -1,4 +1,7 @@ # The upstream commit this tree carries (docs/UPSTREAM-SYNC.md). -# Updated by each sync; read by test/upstream-delta.test.sh. +# Full 40-char SHA, immutable: captured at fetch, merged, then recorded — +# NOT re-read from gh/main later, which moves. Read by +# test/upstream-delta.test.sh, which REFUSES when the object is absent +# rather than calling it unverifiable. # github.com/heavy-duty/ceremony -8c3a4d1 +8c3a4d1dee2bdb5ac06a632a285bb65ab2615214 diff --git a/changelog.d/200.md b/changelog.d/200.md index ced2b20..16add90 100644 --- a/changelog.d/200.md +++ b/changelog.d/200.md @@ -28,8 +28,16 @@ machine-readable form beside the CHANGELOG's prose (#200). - `test/upstream-delta.test.sh` fails the PR that scatters forge branching into - a file the inventory does not name — offline, and refusing rather than - skipping when the recorded ref is missing (#200). + a file the inventory does not name — across shell, workflows, `labels.conf` + and `drills/`, not shell alone (#200). + +- It refuses when the recorded commit is missing, absent from the object store, + or not an ancestor — three distinct refusals, none of them a skip. `ci.yml` + fetches that exact object so the test reads local evidence without CI + omitting it (#200). + +- Its mutation cases drive the real check against a constructed tree, so + replacing the guard with `return 0` reds five of them (#200). - `docs/CONSUMERS.md` states that two ceremonies answer to the same version number, and how a consumer says which one it pinned (#200). diff --git a/docs/UPSTREAM-SYNC.md b/docs/UPSTREAM-SYNC.md index f4ae16f..8377e42 100644 --- a/docs/UPSTREAM-SYNC.md +++ b/docs/UPSTREAM-SYNC.md @@ -50,9 +50,17 @@ Two consequences worth stating plainly: ```sh git remote add gh https://github.com/heavy-duty/ceremony.git # if absent git fetch gh -git merge-base main gh/main +upstream_sha="$(git rev-parse gh/main)" # capture ONCE, in full +git merge-base main "$upstream_sha" ``` +**Capture the full SHA immediately and use that value everywhere after** — the +merge, the provenance, the `.upstream-ref` write. `gh/main` is a moving +pointer: while this sync was being reviewed upstream advanced from `8c3a4d1` +to `08e2912`, and re-reading `gh/main` at recording time would have written a +commit this tree does not contain. The recorded ref is *what was merged*, never +*what upstream is now*. + **Confirm the merge base against `.upstream-ref` before merging anything.** If it is not what the last sync recorded, something moved — stop and re-measure rather than proceeding. A sync that starts from an unexpected base is a sync @@ -61,7 +69,7 @@ whose conflict count means nothing. ### 2. Merge, never rebase ```sh -git merge gh/main +git merge "$upstream_sha" ``` One merge commit, conflicts resolved once (#197 D1). Rebasing the forge-only @@ -123,8 +131,10 @@ ships it. ### 6. Record the provenance - `CHANGELOG.md`'s header: which upstream commit this tree now carries. -- `.upstream-ref`: the same SHA, machine-readable, checked by - `test/upstream-delta.test.sh`. +- `.upstream-ref`: the same **full 40-character** SHA, machine-readable, + checked by `test/upstream-delta.test.sh` — which refuses when the object is + absent or is not an ancestor, rather than reporting it unverifiable. `ci.yml` + fetches that exact object before the suite runs. - A `changelog.d/` fragment for the sync issue. ### 7. Verify — and verify where it will actually run @@ -225,6 +235,18 @@ what makes each sync cost 18 hunks instead of hundreds, and | `lib/closes_references.sh` | the closing-keyword parser that replaced GraphQL | | `.github/labels.conf` | this instance's roster | | `drills/` | records of runs on this instance | +| `actions/refs-not-closing/run.sh` | declares `CEREMONY_FORGE_CLIENT=gh` — its gather is GraphQL, which Forgejo does not serve. #199 removes the declaration | +| `.github/workflows/labels.yml` | the sweep dispatch decides the forge inline and declares a client; a workflow has no shell to call `forge_preflight` from. #205 ports it | +| `.github/workflows/refs-guard.yml` | schedules its job on GitHub only, so an action that can only refuse here does not stand red. #199 removes the gate | +| `.github/workflows/release-exercise.yml` | pins `CEREMONY_FORGE: github` deliberately: the exercise drives the GitHub path | + +Four of those are **temporary** and say which issue removes them. That is the +point of listing them rather than exempting them: a forge-delta location with +no exit is indistinguishable from one nobody noticed. + +A file that merely **calls** the shim is not a delta location — every +reconciler and `release.yml` call `forge_select`, and that is what the shim is +for. A file that **decides** or **declares** is, and belongs here. If a sync needs forge branching somewhere else, that is a design decision, not a detail: add the file to the inventory in the same PR, with the reason. diff --git a/test/upstream-delta.test.sh b/test/upstream-delta.test.sh index 35b56b0..4299f38 100644 --- a/test/upstream-delta.test.sh +++ b/test/upstream-delta.test.sh @@ -34,30 +34,29 @@ REF="$ROOT/.upstream-ref" check "the sync document exists" 0 "" test -f "$DOC" check "the upstream ref is recorded" 0 "" test -f "$REF" +# THE OBJECT IS MANDATORY. An earlier head accepted "UNVERIFIABLE-HERE" when +# upstream's commit was not in the local store, which contradicted this issue's +# own "must not pass by absence" and made the check decorative in exactly the +# environment it matters in (@codex-reviewer-andresmgsl, #200 review). +# +# "Runs offline" means the TEST reads local evidence, not that CI may omit the +# evidence and pass. ci.yml fetches the recorded object before running the +# suite; if it is missing here, that is a refusal. recorded_ref() { grep -vE '^[[:space:]]*(#|$)' "$REF" | head -n1; } ref_is_recorded() { [ -n "$(recorded_ref)" ]; } check "...and is not blank — an unrecorded ref is a refusal, not a skip" 0 "" \ ref_is_recorded -ref_looks_like_sha() { printf '%s' "$(recorded_ref)" | grep -qE '^[0-9a-f]{7,40}$'; } -check "...and looks like a commit SHA" 0 "" ref_looks_like_sha +# Full 40, not a short prefix: a short SHA is ambiguous across a growing repo +# and cannot be compared byte-wise between the runbook, the CHANGELOG and here. +ref_is_full_sha() { printf '%s' "$(recorded_ref)" | grep -qE '^[0-9a-f]{40}$'; } +check "...and is a FULL 40-character SHA" 0 "" ref_is_full_sha -# The ancestry half needs upstream's objects, which a CI clone of THIS repo -# does not have. It is therefore conditional — but it says so out loud rather -# than passing quietly, because "could not check" and "checked and fine" are -# the two states this repo exists to keep apart. -ancestry_state() { - if git -C "$ROOT" cat-file -e "$(recorded_ref)^{commit}" 2>/dev/null; then - git -C "$ROOT" merge-base --is-ancestor "$(recorded_ref)" HEAD 2>/dev/null \ - && echo ANCESTOR || echo NOT-ANCESTOR - else - echo UNVERIFIABLE-HERE - fi -} -state="$(ancestry_state)" -printf 'upstream-delta: recorded ref %s is %s\n' "$(recorded_ref)" "$state" -ancestry_acceptable() { [ "$state" = ANCESTOR ] || [ "$state" = UNVERIFIABLE-HERE ]; } -check "the recorded ref is an ancestor of HEAD, or is honestly reported as unverifiable" 0 "" \ - ancestry_acceptable +ref_object_present() { git -C "$ROOT" cat-file -e "$(recorded_ref)^{commit}" 2>/dev/null; } +check "the recorded commit is present locally — absent is a refusal, not a skip" 0 "" \ + ref_object_present +ref_is_ancestor() { git -C "$ROOT" merge-base --is-ancestor "$(recorded_ref)" HEAD 2>/dev/null; } +check "...and is an ancestor of HEAD, so main really carries what is recorded" 0 "" \ + ref_is_ancestor # --- the inventory ------------------------------------------------------------ @@ -74,11 +73,17 @@ inventory_is_populated() { [ "$(inventory_size)" -ge 6 ]; } check "the inventory names at least the six known delta files" 0 "" \ inventory_is_populated +# Exact for a file entry, `dir/` + one path separator for a directory entry. +# Prefix matching accepted `drills-old/x` for `drills/` and `lib/forge.sh.backup` +# for `lib/forge.sh` (@codex-reviewer-andresmgsl, #200 review). in_inventory() { # $1 = repo-relative path local entry while IFS= read -r entry; do [ -n "$entry" ] || continue - case "$1" in "$entry" | "$entry"*) return 0 ;; esac + case "$entry" in + */) case "$1" in "$entry"*) return 0 ;; esac ;; + *) [ "$1" = "$entry" ] && return 0 ;; + esac done < <(inventory) return 1 } @@ -88,43 +93,62 @@ check "an unlisted file is not" 1 "" in_inventory lib/version.sh # --- the scan ----------------------------------------------------------------- -# What "forge-specific" means, mechanically: the selector's own verbs and the -# environment override that drives them. Comments are stripped — this tree -# documents the forge split at length, and a guard that read prose as evidence -# would flag every file that merely explains the design. -FORGE_MARKERS='forge_detect|forge_select|forge_preflight|forge_client|CEREMONY_FORGE\b|CEREMONY_FORGE_CLIENT' +# What "forge-specific" means, mechanically: the selector's own verbs, the +# environment override that drives them, and — on a workflow, which has no +# shell to call the selector from — a forge decision written inline. +# +# The earlier version scanned only shell under lib/ actions/ bin/ .github/scripts. +# That missed three categories the inventory itself claims to govern: workflows, +# .github/labels.conf and drills/. Merged main already had blind spots there — +# labels.yml decides on GITHUB_SERVER_URL and declares a client, refs-guard.yml +# carries a positive GitHub-only scheduling decision +# (@codex-reviewer-andresmgsl, #200 review). +FORGE_MARKERS='forge_detect|forge_select|forge_preflight|forge_client|CEREMONY_FORGE\b|CEREMONY_FORGE_CLIENT|GITHUB_SERVER_URL|github\.server_url' + +# SCAN_ROOT is a parameter so the teeth below can drive the REAL check against +# a constructed tree. The previous mutation proved the predicates and never +# invoked no_unlisted — the guard could have been `return 0` and still passed. +scan_root() { printf '%s' "${SCAN_ROOT:-$ROOT}"; } forge_specific_files() { - local f rel - for f in "$ROOT"/lib/*.sh "$ROOT"/actions/*/*.sh "$ROOT"/bin/* \ - "$ROOT"/.github/scripts/*.sh; do + local root f rel + root="$(scan_root)" + for f in "$root"/lib/*.sh "$root"/actions/*/*.sh "$root"/bin/* \ + "$root"/.github/scripts/*.sh "$root"/.github/workflows/*.yml \ + "$root"/.github/labels.conf "$root"/drills/*.md; do [ -f "$f" ] || continue - rel="${f#"$ROOT"/}" + rel="${f#"$root"/}" sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$f" \ | grep -qE "$FORGE_MARKERS" && printf '%s\n' "$rel" done return 0 } -# A call site that merely USES the shim is not forge-specific — every -# reconciler calls forge_preflight and that is the point of the shim. What the -# inventory is about is where the forge is DECIDED or BRANCHED ON. So the scan -# reports files carrying the markers, and the assertion below allows the shim's -# own consumers explicitly, by name, so adding a seventh consumer is silent but -# adding a seventh DECIDER is not. +# A file that merely CALLS the shim is not where the forge is decided — every +# reconciler calls forge_preflight and that is the point of the shim. A file +# that DECLARES a client is a different thing and is NOT exempt: it is a +# deliberate forge-delta location and belongs in the inventory +# (@codex-reviewer-andresmgsl). SHIM_CONSUMERS='actions/issueflow-reconcile/issueflow-reconcile.sh actions/labels-reconcile/labels-reconcile.sh actions/labels-scope/labels-scope.sh -actions/refs-not-closing/run.sh lib/facts.sh lib/ruling.sh -lib/attention.sh' +lib/attention.sh +.github/workflows/release.yml' + +declares_a_client() { + sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$(scan_root)/$1" \ + | grep -qE 'CEREMONY_FORGE_CLIENT[=:]' +} unlisted_deciders() { local rel while IFS= read -r rel; do [ -n "$rel" ] || continue - grep -qxF "$rel" <<<"$SHIM_CONSUMERS" && continue + if grep -qxF "$rel" <<<"$SHIM_CONSUMERS" && ! declares_a_client "$rel"; then + continue + fi in_inventory "$rel" && continue printf '%s\n' "$rel" done < <(forge_specific_files) @@ -142,23 +166,65 @@ no_unlisted() { check "every forge-deciding file is named in the inventory" 0 "" no_unlisted # --- teeth -------------------------------------------------------------------- -# The sweep above is a property of the whole tree and cannot be made to fail -# without editing it, so the predicates are driven directly. +# These drive the REAL top-level check against a constructed tree, via +# SCAN_ROOT. The earlier version asserted the predicates separately and never +# invoked no_unlisted — so the guard could have been replaced with `return 0` +# and both "must-fail" rows would still have passed +# (@codex-reviewer-andresmgsl, #200 review). A mutation test that cannot fail +# when the thing it guards is deleted is the shape this repo keeps filing +# issues about. TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT +fixture_tree() { # -> a tree the scan walks, with no forge-deciding files + local t="$TMP/tree" + rm -rf "$t" + mkdir -p "$t/lib" "$t/actions/x" "$t/bin" "$t/.github/scripts" \ + "$t/.github/workflows" "$t/drills" + printf '#!/usr/bin/env bash\necho hello\n' >"$t/lib/plain.sh" + printf 'name: ci\non: [push]\n' >"$t/.github/workflows/plain.yml" + printf '%s' "$t" +} + +root="$(fixture_tree)" +clean_tree_passes() { SCAN_ROOT="$root" no_unlisted; } +check "the guard is green on a tree with no forge decisions" 0 "" clean_tree_passes + +# MUST FAIL: a shell file that decides the forge, in no inventory entry. # shellcheck disable=SC2016 # fixture CONTENT: the literal text a scanned file would hold printf '%s\n' '#!/usr/bin/env bash' 'case "$(forge_detect)" in forgejo) : ;; esac' \ - >"$TMP/scattered.sh" -# Byte-identical to the strip forge_specific_files uses: a helper that -# normalised differently would be testing a predicate the scan does not have. -marker_seen() { - sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$1" | grep -qE "$FORGE_MARKERS" -} -check "a new file branching on forge_detect is seen" 0 "" marker_seen "$TMP/scattered.sh" -printf '%s\n' '#!/usr/bin/env bash' '# forge_detect used to live here' >"$TMP/prose.sh" -check "prose about forge_detect is not" 1 "" marker_seen "$TMP/prose.sh" -check "...and such a file would not be in the inventory" 1 "" in_inventory scattered.sh + >"$root/lib/scattered.sh" +scattered_shell() { SCAN_ROOT="$root" no_unlisted; } +check "a new SHELL file deciding the forge fails the real guard" 1 "" scattered_shell +check "...and the failure names the file" 1 "lib/scattered.sh" scattered_shell +rm -f "$root/lib/scattered.sh" + +# MUST FAIL on a NON-SHELL surface too, so coverage cannot regress to the old +# shell-only glob: a workflow deciding on the server URL. +# shellcheck disable=SC2016 # fixture CONTENT: the literal text a scanned file would hold +printf '%s\n' 'name: x' 'on: [push]' 'jobs:' ' j:' ' steps:' \ + ' - run: [ "$GITHUB_SERVER_URL" = https://github.com ] || exit 0' \ + >"$root/.github/workflows/scattered.yml" +scattered_workflow() { SCAN_ROOT="$root" no_unlisted; } +check "a new WORKFLOW deciding the forge fails it too" 1 "" scattered_workflow +check "...naming that file" 1 ".github/workflows/scattered.yml" scattered_workflow +rm -f "$root/.github/workflows/scattered.yml" + +# A declaration is a delta location even in a file that would otherwise read as +# a shim consumer, so the consumer allow-list cannot hide one. +mkdir -p "$root/lib" +printf '%s\n' '#!/usr/bin/env bash' 'CEREMONY_FORGE_CLIENT=gh' >"$root/lib/facts.sh" +declared_not_exempt() { SCAN_ROOT="$root" no_unlisted; } +check "a shim consumer that DECLARES a client is not exempt" 1 "lib/facts.sh" \ + declared_not_exempt +rm -f "$root/lib/facts.sh" +check "...and the tree is green again once it is gone" 0 "" clean_tree_passes + +# Path matching, both boundaries. +check "a directory entry does not match a sibling with the same prefix" 1 "" \ + in_inventory drills-old/0.4.1.md +check "a file entry does not match a longer path" 1 "" in_inventory lib/forge.sh.backup +check "...while the real ones still match" 0 "" in_inventory drills/0.4.1.md summary From f3a1336d426d9d34f01ae008dc36776383b700d2 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:58:32 +0000 Subject: [PATCH 4/6] fix(upstream-delta): discovery derives from the tree, not from a glob list (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl did not argue this one, he reproduced it: an `actions/*/action.yml` declaring CEREMONY_FORGE_CLIENT and a workflow written `.yaml` rather than `.yml`, both invisible to the hand-picked globs, guard still 21/21 green. The first is not an edge case — `actions/*/action.yml` is this repository's normal composite structure and a client declaration there IS a forge decision. The second shows `*.yml` was never a complete workflow surface. So discovery walks the tree and EXCLUDES by class rather than enumerating directories, depths and extensions. Excluding is the safer default: a new file type arrives scanned rather than invisible. Out of scope are .git/, test/ (whose harness asserts these tokens by design), changelog.d/ and *.md — prose, including drills/, which stays in the inventory because its records are forge-specific by CONTENT while a record mentioning a selector verb in prose is not a decision. Both of his reproductions are now fixtures driving the real no_unlisted, and restricting discovery back to *.sh reds four cases. The documentation claim is aligned with what the guard does rather than what the table implies: it checks forge DECISIONS in executable and configuration files; it is not a diff against upstream, so drills/ and labels.conf are listed by judgement rather than found by scan. Saying otherwise made labels.conf and drills/ look like evidence of completeness while action.yml was invisible. upstream-delta 24/24; test/run.sh 29/29; shellcheck 0.10.0, actionlint and changelog-armed clean. Refs #200 --- changelog.d/200.md | 7 ++-- docs/UPSTREAM-SYNC.md | 14 ++++++++ test/upstream-delta.test.sh | 68 ++++++++++++++++++++++++++++++++----- 3 files changed, 77 insertions(+), 12 deletions(-) diff --git a/changelog.d/200.md b/changelog.d/200.md index 16add90..52ca386 100644 --- a/changelog.d/200.md +++ b/changelog.d/200.md @@ -27,9 +27,10 @@ - `.upstream-ref` records the upstream commit this tree carries, in machine-readable form beside the CHANGELOG's prose (#200). -- `test/upstream-delta.test.sh` fails the PR that scatters forge branching into - a file the inventory does not name — across shell, workflows, `labels.conf` - and `drills/`, not shell alone (#200). +- `test/upstream-delta.test.sh` fails the PR that scatters a forge decision + into a file the inventory does not name. Discovery is derived from the tree, + so a composite `action.yml` or a `.yaml` workflow is seen without anyone + remembering to add a glob (#200). - It refuses when the recorded commit is missing, absent from the object store, or not an ancestor — three distinct refusals, none of them a skip. `ci.yml` diff --git a/docs/UPSTREAM-SYNC.md b/docs/UPSTREAM-SYNC.md index 8377e42..e111fe3 100644 --- a/docs/UPSTREAM-SYNC.md +++ b/docs/UPSTREAM-SYNC.md @@ -227,6 +227,20 @@ Forge-specific behaviour is confined to the files below. Keeping it there is what makes each sync cost 18 hunks instead of hundreds, and `test/upstream-delta.test.sh` fails the PR that scatters it into a new file. +**What that guard actually checks**, stated precisely so the table is not read +as a stronger promise than it is: it walks every tracked file except prose +(`*.md`), the test harness and `changelog.d/`, and flags any that **decides** +the forge — the selector's verbs, `CEREMONY_FORGE*`, or a server-URL comparison +written inline. Discovery is derived from the tree rather than from a list of +directories and extensions, so a composite `action.yml` or a `.yaml` workflow +is seen without anyone remembering to add it. + +It is a check on *forge decisions in executable and configuration files*. It is +**not** a diff against upstream, so it cannot see a file that differs from +upstream for some other forge-specific reason — `drills/` and +`.github/labels.conf` are in the table for that kind of reason and are listed +by judgement, not by scan. + | file | what is forge-specific about it | |---|---| | `lib/forge.sh` | the selector: `forge_detect`, `forge_client`, `forge_preflight` | diff --git a/test/upstream-delta.test.sh b/test/upstream-delta.test.sh index 4299f38..a9a1c96 100644 --- a/test/upstream-delta.test.sh +++ b/test/upstream-delta.test.sh @@ -110,17 +110,43 @@ FORGE_MARKERS='forge_detect|forge_select|forge_preflight|forge_client|CEREMONY_F # invoked no_unlisted — the guard could have been `return 0` and still passed. scan_root() { printf '%s' "${SCAN_ROOT:-$ROOT}"; } +# DISCOVERY IS DERIVED FROM THE TREE, not from a list of directories, depths +# and extensions. The earlier version hand-picked five globs and therefore +# could not see `actions/*/action.yml` — this repository's normal composite +# structure, where a client declaration is exactly a forge decision — or a +# workflow written `.yaml` rather than `.yml`. @codex-reviewer-andresmgsl +# constructed both and the guard stayed 21/21 green, which is the whole +# argument against maintaining a glob list. +# +# So: walk everything, then EXCLUDE by class, and let content classify the +# rest. Excluding is safer than including because a new file type arrives +# scanned rather than invisible. +# +# .git/ not source +# test/ the harness stubs and asserts these tokens by design +# changelog.d/ prose fragments +# *.md prose. `drills/` stays in the INVENTORY because its records +# are forge-specific by content, but a record mentioning a +# selector verb in prose is not a decision, and scanning prose +# for decisions is the mistake this guard's own comment +# handling exists to avoid. +scanned_paths() { + local root; root="$(scan_root)" + find "$root" -type f \ + -not -path "$root/.git/*" \ + -not -path "$root/test/*" \ + -not -path "$root/changelog.d/*" \ + -not -name '*.md' \ + 2>/dev/null | sed "s|^$root/||" | sort +} + forge_specific_files() { - local root f rel - root="$(scan_root)" - for f in "$root"/lib/*.sh "$root"/actions/*/*.sh "$root"/bin/* \ - "$root"/.github/scripts/*.sh "$root"/.github/workflows/*.yml \ - "$root"/.github/labels.conf "$root"/drills/*.md; do - [ -f "$f" ] || continue - rel="${f#"$root"/}" - sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$f" \ + local root rel; root="$(scan_root)" + while IFS= read -r rel; do + [ -n "$rel" ] || continue + sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$root/$rel" 2>/dev/null \ | grep -qE "$FORGE_MARKERS" && printf '%s\n' "$rel" - done + done < <(scanned_paths) return 0 } @@ -211,6 +237,30 @@ check "a new WORKFLOW deciding the forge fails it too" 1 "" scattered_workflow check "...naming that file" 1 ".github/workflows/scattered.yml" scattered_workflow rm -f "$root/.github/workflows/scattered.yml" +# @codex-reviewer-andresmgsl's two reproductions, verbatim as fixtures. Both +# passed the hand-picked-glob version 21/21, which is why discovery is derived +# from the tree now. Deleting a discovery class must make these red. +mkdir -p "$root/actions/unlisted-forge-decision" +printf '%s\n' 'name: x' 'runs:' ' using: composite' ' steps:' \ + ' - shell: bash' ' env:' ' CEREMONY_FORGE_CLIENT: gh' \ + ' run: true' >"$root/actions/unlisted-forge-decision/action.yml" +composite_action_seen() { SCAN_ROOT="$root" no_unlisted; } +check "a forge declaration in actions/*/action.yml fails the guard" 1 \ + "actions/unlisted-forge-decision/action.yml" composite_action_seen +rm -rf "$root/actions/unlisted-forge-decision" + +# ...and a workflow written .yaml rather than .yml — `*.yml` was never a +# complete workflow surface. +# shellcheck disable=SC2016 # fixture CONTENT: the literal text a scanned file would hold +printf '%s\n' 'name: x' 'on: [push]' 'jobs:' ' j:' \ + " if: github.server_url == 'https://github.com'" ' steps: []' \ + >"$root/.github/workflows/unlisted-forge-decision.yaml" +yaml_workflow_seen() { SCAN_ROOT="$root" no_unlisted; } +check "...and one in a .yaml workflow does too" 1 \ + ".github/workflows/unlisted-forge-decision.yaml" yaml_workflow_seen +rm -f "$root/.github/workflows/unlisted-forge-decision.yaml" +check "...leaving the fixture tree green again" 0 "" clean_tree_passes + # A declaration is a delta location even in a file that would otherwise read as # a shim consumer, so the consumer allow-list cannot hide one. mkdir -p "$root/lib" From a48cc719a4c8a124215626a6c53173d230e278fa Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 14:10:56 +0000 Subject: [PATCH 5/6] fix(upstream-delta): discovery is git's index, not the filesystem (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl reproduced it again, with one file: scanned_paths() said "tracked" and used `find`, which walks the working directory and knows nothing about the index. Not pedantry — ci.yml extracts shellcheck.tar.xz, actionlint.tar.gz and their binaries INTO the checkout before the suite runs, and any developer cache sits there too. Today none happens to carry a matching marker; that is luck, not a property, and a false red on a downloaded tarball would be indistinguishable from a real finding. `git ls-files -z` makes "tracked" executable rather than prose. The fixtures become tiny git repositories, because a fixture that is only a directory is invisible to ls-files and every must-fail below it would have passed vacuously — the same trap as the earlier teeth that never invoked the guard. Plus the negative case he asked for: an untracked marker-bearing cache file is ignored, and the moment it is TRACKED the guard sees it. Reverting discovery to find reds three. Branch updated from merged main (e236318, now carrying !206) before verifying: upstream-delta 28/28, test/run.sh 29/29, shellcheck 0.10.0 clean. Refs #200 --- changelog.d/200.md | 4 +++ test/upstream-delta.test.sh | 66 ++++++++++++++++++++++++++----------- 2 files changed, 50 insertions(+), 20 deletions(-) diff --git a/changelog.d/200.md b/changelog.d/200.md index 52ca386..acfada8 100644 --- a/changelog.d/200.md +++ b/changelog.d/200.md @@ -32,6 +32,10 @@ so a composite `action.yml` or a `.yaml` workflow is seen without anyone remembering to add a glob (#200). +- Discovery is git's, not the filesystem's: `ls-files`, so the tarballs `ci.yml` + extracts into the checkout and any developer cache are not parsed as source + (#200). + - It refuses when the recorded commit is missing, absent from the object store, or not an ancestor — three distinct refusals, none of them a skip. `ci.yml` fetches that exact object so the test reads local evidence without CI diff --git a/test/upstream-delta.test.sh b/test/upstream-delta.test.sh index a9a1c96..a0debbe 100644 --- a/test/upstream-delta.test.sh +++ b/test/upstream-delta.test.sh @@ -122,7 +122,6 @@ scan_root() { printf '%s' "${SCAN_ROOT:-$ROOT}"; } # rest. Excluding is safer than including because a new file type arrives # scanned rather than invisible. # -# .git/ not source # test/ the harness stubs and asserts these tokens by design # changelog.d/ prose fragments # *.md prose. `drills/` stays in the INVENTORY because its records @@ -130,14 +129,21 @@ scan_root() { printf '%s' "${SCAN_ROOT:-$ROOT}"; } # selector verb in prose is not a decision, and scanning prose # for decisions is the mistake this guard's own comment # handling exists to avoid. +# +# DISCOVERY IS GIT'S, NOT THE FILESYSTEM'S. An earlier head said "tracked" and +# used `find`, which walks the working directory and knows nothing about the +# index. That is not pedantry: `ci.yml` extracts shellcheck and actionlint +# tarballs INTO the checkout before the suite runs, and any developer cache +# sits there too. @codex-reviewer-andresmgsl reproduced a false red with one +# untracked file. `git ls-files -z` makes "tracked" executable rather than +# prose. scanned_paths() { local root; root="$(scan_root)" - find "$root" -type f \ - -not -path "$root/.git/*" \ - -not -path "$root/test/*" \ - -not -path "$root/changelog.d/*" \ - -not -name '*.md' \ - 2>/dev/null | sed "s|^$root/||" | sort + git -C "$root" ls-files -z 2>/dev/null \ + | tr '\0' '\n' \ + | grep -vE '^(test/|changelog\.d/)' \ + | grep -vE '\.md$' \ + | sort } forge_specific_files() { @@ -203,15 +209,20 @@ check "every forge-deciding file is named in the inventory" 0 "" no_unlisted TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT -fixture_tree() { # -> a tree the scan walks, with no forge-deciding files +# A tiny GIT repository, because discovery is git's now: a fixture that is only +# a directory would be invisible to `ls-files` and every must-fail below would +# pass vacuously. +fixture_tree() { # -> a tracked tree with no forge-deciding files local t="$TMP/tree" rm -rf "$t" - mkdir -p "$t/lib" "$t/actions/x" "$t/bin" "$t/.github/scripts" \ - "$t/.github/workflows" "$t/drills" + mkdir -p "$t/lib" "$t/.github/workflows" printf '#!/usr/bin/env bash\necho hello\n' >"$t/lib/plain.sh" printf 'name: ci\non: [push]\n' >"$t/.github/workflows/plain.yml" + git -C "$t" init -q 2>/dev/null + git -C "$t" add -A 2>/dev/null printf '%s' "$t" } +track() { git -C "$root" add -A 2>/dev/null; } root="$(fixture_tree)" clean_tree_passes() { SCAN_ROOT="$root" no_unlisted; } @@ -221,10 +232,10 @@ check "the guard is green on a tree with no forge decisions" 0 "" clean_tree_pas # shellcheck disable=SC2016 # fixture CONTENT: the literal text a scanned file would hold printf '%s\n' '#!/usr/bin/env bash' 'case "$(forge_detect)" in forgejo) : ;; esac' \ >"$root/lib/scattered.sh" -scattered_shell() { SCAN_ROOT="$root" no_unlisted; } +track; scattered_shell() { SCAN_ROOT="$root" no_unlisted; } check "a new SHELL file deciding the forge fails the real guard" 1 "" scattered_shell check "...and the failure names the file" 1 "lib/scattered.sh" scattered_shell -rm -f "$root/lib/scattered.sh" +rm -f "$root/lib/scattered.sh"; track # MUST FAIL on a NON-SHELL surface too, so coverage cannot regress to the old # shell-only glob: a workflow deciding on the server URL. @@ -232,10 +243,10 @@ rm -f "$root/lib/scattered.sh" printf '%s\n' 'name: x' 'on: [push]' 'jobs:' ' j:' ' steps:' \ ' - run: [ "$GITHUB_SERVER_URL" = https://github.com ] || exit 0' \ >"$root/.github/workflows/scattered.yml" -scattered_workflow() { SCAN_ROOT="$root" no_unlisted; } +track; scattered_workflow() { SCAN_ROOT="$root" no_unlisted; } check "a new WORKFLOW deciding the forge fails it too" 1 "" scattered_workflow check "...naming that file" 1 ".github/workflows/scattered.yml" scattered_workflow -rm -f "$root/.github/workflows/scattered.yml" +rm -f "$root/.github/workflows/scattered.yml"; track # @codex-reviewer-andresmgsl's two reproductions, verbatim as fixtures. Both # passed the hand-picked-glob version 21/21, which is why discovery is derived @@ -244,10 +255,10 @@ mkdir -p "$root/actions/unlisted-forge-decision" printf '%s\n' 'name: x' 'runs:' ' using: composite' ' steps:' \ ' - shell: bash' ' env:' ' CEREMONY_FORGE_CLIENT: gh' \ ' run: true' >"$root/actions/unlisted-forge-decision/action.yml" -composite_action_seen() { SCAN_ROOT="$root" no_unlisted; } +track; composite_action_seen() { SCAN_ROOT="$root" no_unlisted; } check "a forge declaration in actions/*/action.yml fails the guard" 1 \ "actions/unlisted-forge-decision/action.yml" composite_action_seen -rm -rf "$root/actions/unlisted-forge-decision" +rm -rf "$root/actions/unlisted-forge-decision"; track # ...and a workflow written .yaml rather than .yml — `*.yml` was never a # complete workflow surface. @@ -255,22 +266,37 @@ rm -rf "$root/actions/unlisted-forge-decision" printf '%s\n' 'name: x' 'on: [push]' 'jobs:' ' j:' \ " if: github.server_url == 'https://github.com'" ' steps: []' \ >"$root/.github/workflows/unlisted-forge-decision.yaml" -yaml_workflow_seen() { SCAN_ROOT="$root" no_unlisted; } +track; yaml_workflow_seen() { SCAN_ROOT="$root" no_unlisted; } check "...and one in a .yaml workflow does too" 1 \ ".github/workflows/unlisted-forge-decision.yaml" yaml_workflow_seen -rm -f "$root/.github/workflows/unlisted-forge-decision.yaml" +rm -f "$root/.github/workflows/unlisted-forge-decision.yaml"; track check "...leaving the fixture tree green again" 0 "" clean_tree_passes # A declaration is a delta location even in a file that would otherwise read as # a shim consumer, so the consumer allow-list cannot hide one. mkdir -p "$root/lib" printf '%s\n' '#!/usr/bin/env bash' 'CEREMONY_FORGE_CLIENT=gh' >"$root/lib/facts.sh" -declared_not_exempt() { SCAN_ROOT="$root" no_unlisted; } +track; declared_not_exempt() { SCAN_ROOT="$root" no_unlisted; } check "a shim consumer that DECLARES a client is not exempt" 1 "lib/facts.sh" \ declared_not_exempt -rm -f "$root/lib/facts.sh" +rm -f "$root/lib/facts.sh"; track +track check "...and the tree is green again once it is gone" 0 "" clean_tree_passes +# UNTRACKED input is not source. ci.yml extracts shellcheck and actionlint +# tarballs into the checkout before the suite runs, and a developer cache sits +# there too; parsing either is a false red on something outside the repository +# property (@codex-reviewer-andresmgsl, #200 review — reproduced with one file). +printf 'CEREMONY_FORGE_CLIENT=gh\n' >"$root/local-tool-cache.txt" +check "an UNTRACKED marker-bearing file is ignored" 0 "" clean_tree_passes +check "...and is still ignored once it carries a decision" 0 "" clean_tree_passes +track +check "...but the moment it is TRACKED the guard sees it" 1 "local-tool-cache.txt" \ + clean_tree_passes +git -C "$root" rm -q --cached local-tool-cache.txt 2>/dev/null +rm -f "$root/local-tool-cache.txt" +check "...and removing it restores green" 0 "" clean_tree_passes + # Path matching, both boundaries. check "a directory entry does not match a sibling with the same prefix" 1 "" \ in_inventory drills-old/0.4.1.md From fc24fa4b78daf6f7669a10f17ef6681b36248769 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 15:15:35 +0000 Subject: [PATCH 6/6] docs(upstream-sync): the inventory names docs-sync, which #201 makes forge-deciding (#200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by combining all five open PRs and running the suite on the result — which is the check this PR's own runbook adds, catching a real break the first time it was applied at scale. !203 (#201) makes actions/docs-sync/docs-sync.sh decide the forge from GITHUB_SERVER_URL, because it was fetching the doctrine mirror from a hard-coded github.com. This PR's guard requires every forge-deciding file to be named in the inventory. Both are individually green; together the tree is red: forge-specific but not in docs/UPSTREAM-SYNC.md: actions/docs-sync/docs-sync.sh The entry belongs here rather than in !203: the inventory is this PR's artifact, and !203 is a bug fix that should not have to know about a guard absent from its base. Adding it early is harmless — the guard checks that deciding files ARE listed, not that listed files decide — and correct the moment both land. Five-way combined tree after this: 28 test files 0 failed under the runner's jq 1.6, shellcheck 0.10.0, actionlint, self-ref, marker, vendored and changelog-armed all clean. Refs #200 --- docs/UPSTREAM-SYNC.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/UPSTREAM-SYNC.md b/docs/UPSTREAM-SYNC.md index e111fe3..84bbd8b 100644 --- a/docs/UPSTREAM-SYNC.md +++ b/docs/UPSTREAM-SYNC.md @@ -253,6 +253,7 @@ by judgement, not by scan. | `.github/workflows/labels.yml` | the sweep dispatch decides the forge inline and declares a client; a workflow has no shell to call `forge_preflight` from. #205 ports it | | `.github/workflows/refs-guard.yml` | schedules its job on GitHub only, so an action that can only refuse here does not stand red. #199 removes the gate | | `.github/workflows/release-exercise.yml` | pins `CEREMONY_FORGE: github` deliberately: the exercise drives the GitHub path | +| `actions/docs-sync/docs-sync.sh` | fetches the doctrine mirror from the forge in `GITHUB_SERVER_URL`, and refuses rather than guessing one (#201) | Four of those are **temporary** and say which issue removes them. That is the point of listing them rather than exempting them: a forge-delta location with