From e965b15cbfa5fb583da899c915f58f8628865935 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:36:17 +0000 Subject: [PATCH 01/11] 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 02/11] 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 03/11] 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 04/11] 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 05/11] 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 877e09e0159ae7f562f7b2fafb0c94aff80d0562 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 14:27:40 +0000 Subject: [PATCH 06/11] fix(issueflow): the board discriminator is .pull_request == null, not has() (#210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issueflow-reconcile has been blind on this forge since the 0.6.0 merge landed. Run 368 — #198's own post-merge acceptance probe — printed: issueflow: no open issues. issueflow: reconciled. over a board of nine. Every Forgejo entry CARRIES the `pull_request` key, valued null on an issue, so `select(has("pull_request") | not)` selects zero rows. Measured again today: #209 (an issue) has the key valued null; #208 and #207 (PRs) have it valued as objects. This is mine. #188 fixed exactly this and the file's own comment at :1113 states the rule, with :1121 already using it correctly. Resolving hunk 4 of the merge I took upstream's board block wholesale and carried the wrong discriminator into three sites — the gather, the release-body gather, and reconcile_issue_pass — in the PR whose stated purpose was to stop blind sweeps reporting success. Cost while it stood: no issue transitions, no claim reclaims, no nudges, no board flags — and no `post-merge` transitions, which is why #192 and #198 both still read `claimed` after their PRs merged, and why #198's own closure criterion could not complete. Two guards, because a comment did not hold: * A GATHER-LEVEL CASE against a Forgejo-shaped fixture — every entry carrying the key. The existing discriminator cases assert jq expressions in isolation and passed throughout this regression; they never ran the gather that uses them, which is precisely how it survived review. * A SOURCE PIN forbidding has("pull_request") on this surface, so a future sync cannot reintroduce it 40 lines below the comment forbidding it. Reverting the board gather reds both. Reverting reconcile_issue_pass reds the pin. test/run.sh 28/28 under jq 1.7 and jq 1.6; issueflow 503/503; shellcheck 0.10.0 and actionlint clean. Refs #210 --- .../issueflow-reconcile.sh | 10 ++-- changelog.d/210.md | 22 ++++++++ test/issueflow-reconcile.test.sh | 52 +++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 changelog.d/210.md diff --git a/actions/issueflow-reconcile/issueflow-reconcile.sh b/actions/issueflow-reconcile/issueflow-reconcile.sh index fbe3d16..f5386b5 100644 --- a/actions/issueflow-reconcile/issueflow-reconcile.sh +++ b/actions/issueflow-reconcile/issueflow-reconcile.sh @@ -1159,7 +1159,10 @@ reconcile_issue_pass() { # $1 = issue — one issue's whole pass, in its own sub || skip_issue "$n" "could not read the issue: $(read_failure_reason "$READ_FAILURE_STDERR")" issue_payload_valid "$n" <<<"$ISSUE_JSON" \ || skip_issue "$n" "the issue read answered a payload that is not issue #$n carrying a label array" - jq -e 'has("pull_request") | not' <<<"$ISSUE_JSON" >/dev/null || exit 0 + # `.pull_request == null`, never `has("pull_request") | not` (#188, #210): + # every Forgejo entry CARRIES the key, valued null on an issue, so the + # has() form selects zero rows here — silently, forever. + jq -e '.pull_request == null' <<<"$ISSUE_JSON" >/dev/null || exit 0 ISSUE_LABELS="$(jq -r '.labels[].name' <<<"$ISSUE_JSON")" reconcile_issue "$n" || exit $? commit_staged_effects @@ -1273,7 +1276,8 @@ main() { log "could not read the issue board: $(read_failure_reason "$READ_FAILURE_STDERR")" return 1 fi - BOARD_RECORDS="$(jq -r '.[] | select(has("pull_request") | not) + # `.pull_request == null`, never `has("pull_request") | not` (#188, #210). + BOARD_RECORDS="$(jq -r '.[] | select(.pull_request == null) | [(.number | tostring), ((.labels // []) | map(.name) | join(",")), (.title // "")] | @tsv' \ <<<"$board_json")" @@ -1284,7 +1288,7 @@ main() { # is exactly the emptied gate the release's own `blocked` -> `ready` # promotion answers, which is why a `ready` release leaves the flag # dormant rather than flagging the whole board. - release_bodies="$(jq -r '.[] | select(has("pull_request") | not) + release_bodies="$(jq -r '.[] | select(.pull_request == null) | select((.labels // []) | map(.name) | index("release")) | [(.number | tostring), ((.body // "") | gsub("[\t\r\n]"; " "))] | @tsv' \ <<<"$board_json")" diff --git a/changelog.d/210.md b/changelog.d/210.md new file mode 100644 index 0000000..afd7927 --- /dev/null +++ b/changelog.d/210.md @@ -0,0 +1,22 @@ +### Fixed + +- `issueflow-reconcile` sees this forge's issues again. The board gather used + `has("pull_request")`, and every Forgejo entry carries that key — so it + selected zero rows on every sweep while printing `reconciled.` (#210). + +- Three sites take `.pull_request == null`, the discriminator the file's own + comment already specified and that one of its four call sites already used + (#210). + +- `post-merge` transitions can fire again: they could not, because the sweep + saw no issues to transition (#210). + +### Added + +- A gather-level case drives the real board read against a Forgejo-shaped + fixture — every entry carrying the key. The existing discriminator cases + assert `jq` expressions in isolation and passed throughout this regression + (#210). + +- A source pin forbids `has("pull_request")` on this surface, because the rule + was stated in a comment and violated forty lines below it (#210). diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index c115695..512658d 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -1779,6 +1779,58 @@ check "a PR reads as a PR on either shape" 0 "pr" \ check "the old has() test misreads a forgejo issue as a PR" 0 "pr" \ bash -c 'echo "{\"number\":1,\"pull_request\":null}" | jq -e "has(\"pull_request\") | not" >/dev/null && echo issue || echo pr' +# ...AND THE GATHER, because the rows above assert jq expressions in isolation +# and passed all the way through #210 — a sweep that saw zero issues on every +# pass and printed `reconciled.` The 0.6.0 merge reintroduced the has() form in +# the board gather; these cases could not see it because they never ran it. +# +# The fixture is FORGEJO-SHAPED: every entry carries `pull_request`, valued +# null on an issue and an object on a PR. On a GitHub-shaped board (key absent +# on issues) both discriminators agree, which is why this needs its own board. +FORGEJO_BOARD="$TMP/forgejo-board" +mkdir -p "$FORGEJO_BOARD" +cp "$ARRIVAL/labels.conf" "$FORGEJO_BOARD/labels.conf" 2>/dev/null || true +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_pulls_state_open.json" +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_pulls_state_closed.json" +printf '%s\n' \ + '[{"number":60,"pull_request":null,"labels":[{"name":"ready"}],"title":"an issue"}, + {"number":61,"pull_request":{"merged":false},"labels":[],"title":"a pull request"}]' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_state_open.json" +jq -n --arg at "$(iso_at "$INOW")" \ + '{number:60,user:{login:"triage-one"},created_at:$at,body:"",pull_request:null, + labels:[{name:"ready"}],assignees:[]}' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_60.json" +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_60_comments.json" +forgejo_board_run() { + : >"$FORGEJO_BOARD/edits" + env PATH="$ARRIVAL/stub:$PATH" CEREMONY_FORGE=github GH_FIXTURES="$FORGEJO_BOARD" \ + ISSUEFLOW_NOW="$INOW" REPO=owner/repo LABELS_CONF="$ARRIVAL/labels.conf" \ + bash "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" 2>&1 +} +fjb_out="$(forgejo_board_run)" +check "a forgejo-shaped board is NOT read as empty" 1 "" \ + grep -qF 'issueflow: no open issues.' <<<"$fjb_out" +check "...the sweep completes over it" 0 "" \ + grep -qF 'issueflow: reconciled.' <<<"$fjb_out" +check "...and the PR row is still excluded from the issue set" 1 "" \ + grep -qE '^issueflow: #61' <<<"$fjb_out" + +# -- the rule is pinned at the source, because a comment did not hold -------- +# `.pull_request == null` is stated in this file's own header AND at +# issueflow-reconcile.sh:1113 — and the merge put `has("pull_request")` back 40 +# lines below that comment, in three places. Prose is not a guard (#210). +# In-process, not `bash -c`: a subshell cannot see this file's functions, and a +# pin that silently inspected nothing would be the same defect one level up. +no_has_pull_request() { + local hits + hits="$(sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' \ + "$ROOT/actions/issueflow-reconcile/issueflow-reconcile.sh" \ + | grep -n 'has("pull_request")')" + [ -z "$hits" ] || { printf 'executable has("pull_request") at:\n%s\n' "$hits" >&2; return 1; } +} +check "no executable has(\"pull_request\") survives on this surface" 0 "" \ + no_has_pull_request + # -- the OPEN-pull gather, at main() granularity ---------------------------- # The closed/merged half above proves one REST path; this proves the other, # which is a DIFFERENT pipeline: `.body | @base64` -> base64 -d -> From bada4ffff593625b36ff8bf173cc7b4f93649237 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 14:37:37 +0000 Subject: [PATCH 07/11] test(issueflow): each of the three sites is caught by behaviour, not only by the pin (#210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl's two scope items, applied before the first review round rather than after. 1. THE GUARD IS COMMENT-AWARE, WITH CONTROLS. It already stripped comments — it has to, because the #188 warning that explains why has("pull_request") is wrong contains the string. Without controls that was an untested property, and the pressure it creates is real: a raw grep would push a builder into deleting the very warning that prevents recurrence. Two fixtures now prove it: the explanatory comment is allowed, an executable jq filter is rejected. 2. ALL THREE SITES ARE DRIVEN BY BEHAVIOUR. The pin makes any revert red, but a pin proves a string is absent, not that each replacement means the intended thing: BOARD_RECORDS the forgejo-shaped board is not read as empty release_bodies an open `release` issue whose gate holds an open member makes a claimable NON-member draw a window flag — empty carriers, no flag, so the row discriminates the site instead of merely reaching it reconcile_issue_pass the scalar payload: key-present-null is an issue, object-valued is a PR, key-absent is still an issue The release_bodies row did NOT discriminate on its first write — it asserted an issue number that BOARD_RECORDS also produces, so reverting the site left it green. Caught by mutating each site separately rather than trusting the suite total. Mutation, per site: BOARD_RECORDS 3 red, release_bodies 2 red, reconcile_issue_pass 2 red. test/run.sh 28/28; issueflow 510/510; shellcheck 0.10.0 clean. Refs #210 --- changelog.d/210.md | 7 +++- test/issueflow-reconcile.test.sh | 58 ++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/changelog.d/210.md b/changelog.d/210.md index afd7927..ed46cf2 100644 --- a/changelog.d/210.md +++ b/changelog.d/210.md @@ -19,4 +19,9 @@ (#210). - A source pin forbids `has("pull_request")` on this surface, because the rule - was stated in a comment and violated forty lines below it (#210). + was stated in a comment and violated forty lines below it. It strips comments, + so the #188 warning that explains the trap is allowed to stay (#210). + +- All three sites are covered behaviourally, not only by the pin: the board + gather, the release-body gather through an observable window flag, and the + per-issue payload check (#210). diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 512658d..8dcca61 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -1815,6 +1815,47 @@ check "...the sweep completes over it" 0 "" \ check "...and the PR row is still excluded from the issue set" 1 "" \ grep -qE '^issueflow: #61' <<<"$fjb_out" +# release_bodies is the THIRD producer and has its own has() site. A `release` +# issue on a forgejo-shaped board must reach the window gather, or the #292 +# flags are decided over an empty set (@codex-reviewer-andresmgsl, #210). +printf '%s\n' \ + '[{"number":60,"pull_request":null,"labels":[{"name":"ready"}],"title":"an issue"}, + {"number":62,"pull_request":null,"labels":[{"name":"release"}],"title":"Release 9.9.9","body":"Blocked by #60."}, + {"number":63,"pull_request":null,"labels":[{"name":"ready"}],"title":"a claimable non-member"}, + {"number":61,"pull_request":{"merged":false},"labels":[],"title":"a pull request"}]' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_state_open.json" +jq -n --arg at "$(iso_at "$INOW")" \ + '{number:63,user:{login:"triage-one"},created_at:$at,body:"",pull_request:null, + labels:[{name:"ready"}],assignees:[]}' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_63.json" +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_63_comments.json" +jq -n --arg at "$(iso_at "$INOW")" \ + '{number:62,user:{login:"triage-one"},created_at:$at,body:"Blocked by #60.",pull_request:null, + labels:[{name:"release"}],assignees:[]}' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_62.json" +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_62_comments.json" +fjb2_out="$(forgejo_board_run)" +# The observable effect of release_bodies being NON-empty: an open `release` +# issue whose gate still holds an open member makes every claimable non-member +# draw a window flag. With that gather empty there are no carriers and no flag, +# so this row discriminates the site rather than merely reaching it. +check "a release issue on a forgejo-shaped board reaches the window gather" 0 "" \ + grep -qE '#63: window flag' <<<"$fjb2_out" +check "...and the sweep still completes" 0 "" \ + grep -qF 'issueflow: reconciled.' <<<"$fjb2_out" + +# The THIRD site is reconcile_issue_pass's per-issue payload check. Same key, +# scalar rather than a list: null means issue, an object means PR. +pass_disc() { # $1 = the payload -> the exit status the guard would take + jq -e '.pull_request == null' <<<"$1" >/dev/null && echo issue || echo pr +} +check "a forgejo issue payload (key present, null) reads as an issue" 0 "issue" \ + pass_disc '{"number":60,"pull_request":null}' +check "...and an object-valued one reads as a PR" 0 "pr" \ + pass_disc '{"number":61,"pull_request":{"merged":false}}' +check "...and a github-shaped payload (key absent) still reads as an issue" 0 "issue" \ + pass_disc '{"number":60}' + # -- the rule is pinned at the source, because a comment did not hold -------- # `.pull_request == null` is stated in this file's own header AND at # issueflow-reconcile.sh:1113 — and the merge put `has("pull_request")` back 40 @@ -1830,6 +1871,23 @@ no_has_pull_request() { } check "no executable has(\"pull_request\") survives on this surface" 0 "" \ no_has_pull_request +# The controls, because this guard MUST tolerate the #188 comment that explains +# why the form is wrong — a raw grep would either fail forever or pressure a +# builder into deleting the very warning that prevents recurrence +# (@codex-reviewer-andresmgsl, #210 review). +DTMP="$(mktemp -d)"; trap 'rm -rf "$DTMP"' EXIT +strip_and_find() { # $1 = file -> 0 when an EXECUTABLE use survives + sed 's/[[:space:]]#.*$//; s/^[[:space:]]*#.*$//' "$1" | grep -q 'has("pull_request")' +} +# shellcheck disable=SC2016 # fixture CONTENT: the literal text a scanned file would hold +printf '%s\n' '#!/usr/bin/env bash' \ + '# `.pull_request == null`, NOT has("pull_request") | not (#188)' \ + 'jq -e ".pull_request == null" <<<"$J"' >"$DTMP/prose.sh" +check "the explanatory #188 comment is allowed" 1 "" strip_and_find "$DTMP/prose.sh" +# single-quoted so the fixture holds the LITERAL form the guard looks for +printf '%s\n' '#!/usr/bin/env bash' \ + "jq -r '.[] | select(has(\"pull_request\") | not)' <<<\"\$J\"" >"$DTMP/exec.sh" +check "...while an executable jq filter is rejected" 0 "" strip_and_find "$DTMP/exec.sh" # -- the OPEN-pull gather, at main() granularity ---------------------------- # The closed/merged half above proves one REST path; this proves the other, From 087ea4a24bd5d948bf91f21cb8ea7e5f680ccb87 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 14:44:31 +0000 Subject: [PATCH 08/11] test(issueflow): each site observable through the real path, not through the expression it contains (#210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl's three, and items 1 and 2 were still open after bada4ff — his review predates that push, but only item 3 (release bodies) was actually answered by it. TRAVERSAL, not a non-empty gather. The board case asserted the board was not read as empty; #60 was `ready` and therefore produced no observable effect, so nothing proved reconcile_issue_pass had run over it. #60 now carries NO queue state, so traversal has a deterministic outcome — needs-triage is minted and logged — and the row asserts that. THE SCALAR SITE, through the real path. My first attempt asserted the jq expression the function contains, which is exactly the shape that let this regression through: the isolated discriminator rows passed the whole time the gather was blind. A hand-wired probe around reconcile_issue_pass needed so much internal setup that it would have been testing my scaffolding, so the same board harness drives it with one row flipped — object-valued must NOT be reconciled as an issue, and the sweep must then correctly report the board as empty OF ISSUES. Per-site mutation, all three now behavioural rather than pin-only: revert BOARD_RECORDS -> 4 red revert release_bodies -> 2 red revert reconcile_issue_pass -> 3 red test/run.sh 28/28; issueflow 510/510; shellcheck 0.10.0 clean. Refs #210 --- test/issueflow-reconcile.test.sh | 38 ++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index 8dcca61..f31b58d 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -1793,12 +1793,12 @@ cp "$ARRIVAL/labels.conf" "$FORGEJO_BOARD/labels.conf" 2>/dev/null || true printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_pulls_state_open.json" printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_pulls_state_closed.json" printf '%s\n' \ - '[{"number":60,"pull_request":null,"labels":[{"name":"ready"}],"title":"an issue"}, + '[{"number":60,"pull_request":null,"labels":[],"title":"an issue with no queue state"}, {"number":61,"pull_request":{"merged":false},"labels":[],"title":"a pull request"}]' \ >"$FORGEJO_BOARD/repos_owner_repo_issues_state_open.json" jq -n --arg at "$(iso_at "$INOW")" \ '{number:60,user:{login:"triage-one"},created_at:$at,body:"",pull_request:null, - labels:[{name:"ready"}],assignees:[]}' \ + labels:[],assignees:[]}' \ >"$FORGEJO_BOARD/repos_owner_repo_issues_60.json" printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_60_comments.json" forgejo_board_run() { @@ -1812,7 +1812,13 @@ check "a forgejo-shaped board is NOT read as empty" 1 "" \ grep -qF 'issueflow: no open issues.' <<<"$fjb_out" check "...the sweep completes over it" 0 "" \ grep -qF 'issueflow: reconciled.' <<<"$fjb_out" -check "...and the PR row is still excluded from the issue set" 1 "" \ +# TRAVERSAL, not merely a non-empty gather: the null-valued row has a +# deterministic outcome — no queue state means needs-triage is minted — so this +# proves reconcile_issue_pass actually ran over it, which "the board is not +# empty" does not (@codex-reviewer-andresmgsl, #210 review). +check "...the null-valued row is TRAVERSED, with an observable outcome" 0 "" \ + grep -qE '^issueflow: #60: needs-triage' <<<"$fjb_out" +check "...and the object-valued PR row is not reconciled as an issue" 1 "" \ grep -qE '^issueflow: #61' <<<"$fjb_out" # release_bodies is the THIRD producer and has its own has() site. A `release` @@ -1846,15 +1852,23 @@ check "...and the sweep still completes" 0 "" \ # The THIRD site is reconcile_issue_pass's per-issue payload check. Same key, # scalar rather than a list: null means issue, an object means PR. -pass_disc() { # $1 = the payload -> the exit status the guard would take - jq -e '.pull_request == null' <<<"$1" >/dev/null && echo issue || echo pr -} -check "a forgejo issue payload (key present, null) reads as an issue" 0 "issue" \ - pass_disc '{"number":60,"pull_request":null}' -check "...and an object-valued one reads as a PR" 0 "pr" \ - pass_disc '{"number":61,"pull_request":{"merged":false}}' -check "...and a github-shaped payload (key absent) still reads as an issue" 0 "issue" \ - pass_disc '{"number":60}' +# The scalar site, driven through the REAL path rather than through the jq +# expression it contains — asserting the expression in isolation is what let +# the regression through (@codex-reviewer-andresmgsl, #210 review). Same board +# harness, one row flipped: null-valued reconciles, object-valued stands down. +printf '%s\n' \ + '[{"number":64,"pull_request":{"merged":false},"labels":[],"title":"a PR wearing an issue row"}]' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_state_open.json" +jq -n --arg at "$(iso_at "$INOW")" \ + '{number:64,user:{login:"triage-one"},created_at:$at,body:"", + pull_request:{merged:false},labels:[],assignees:[]}' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_64.json" +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_64_comments.json" +fjb3_out="$(forgejo_board_run)" +check "an object-valued row is not reconciled as an issue by the pass" 1 "" \ + grep -qE '^issueflow: #64' <<<"$fjb3_out" +check "...and the sweep reports the board as empty of ISSUES, correctly" 0 "" \ + grep -qF 'issueflow: no open issues.' <<<"$fjb3_out" # -- the rule is pinned at the source, because a comment did not hold -------- # `.pull_request == null` is stated in this file's own header AND at From 5b78d292015256ae2c19810914cf170c38ed9b6c Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 14:49:01 +0000 Subject: [PATCH 09/11] =?UTF-8?q?test(issueflow):=20isolate=20the=20scalar?= =?UTF-8?q?=20guard=20=E2=80=94=20the=20list=20row=20admits,=20the=20paylo?= =?UTF-8?q?ad=20stands=20down=20(#210)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl found the subtlety my board fixture could not reach: BOARD_RECORDS filters an object-valued row out of the LIST before the per-issue guard ever sees it, so no board fixture alone can prove the scalar stand-down. My #64 case proved the gather excluded it, not that reconcile_issue_pass did. The fixture that isolates the site is a deliberate mismatch: the LIST row is null-valued, so the board gather admits #65 — and the INDIVIDUAL payload the sweep then fetches is object-valued. Only reconcile_issue_pass's own guard can stand that down. Three rows over the same number, so the guard cannot pass by standing everything down or by admitting everything: payload object-valued -> NOT reconciled payload null-valued -> reconciled payload key absent -> reconciled (the GitHub shape) Mutating ONLY the scalar predicate now reds three BEHAVIOURAL rows plus the pin, where before it red only the pin and a neighbour. pass_disc is gone: it repeated the predicate inside the test helper and never called production — which is the same isolated-expression trap, one layer down, in the fix for it. issueflow 512/512; test/run.sh 28/28; shellcheck 0.10.0 clean. Refs #210 --- test/issueflow-reconcile.test.sh | 45 +++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/test/issueflow-reconcile.test.sh b/test/issueflow-reconcile.test.sh index f31b58d..918b635 100644 --- a/test/issueflow-reconcile.test.sh +++ b/test/issueflow-reconcile.test.sh @@ -1852,23 +1852,44 @@ check "...and the sweep still completes" 0 "" \ # The THIRD site is reconcile_issue_pass's per-issue payload check. Same key, # scalar rather than a list: null means issue, an object means PR. -# The scalar site, driven through the REAL path rather than through the jq -# expression it contains — asserting the expression in isolation is what let -# the regression through (@codex-reviewer-andresmgsl, #210 review). Same board -# harness, one row flipped: null-valued reconciles, object-valued stands down. +# THE SCALAR SITE, ISOLATED. `BOARD_RECORDS` filters an object-valued row out +# of the LIST before the per-issue guard ever sees it, so a board fixture alone +# cannot prove the scalar stand-down (@codex-reviewer-andresmgsl, #210 review). +# +# The fixture that isolates it: the LIST row is null-valued, so the board +# gather admits it — and the INDIVIDUAL payload the sweep then fetches is +# object-valued. Only reconcile_issue_pass's own guard can stand that down. printf '%s\n' \ - '[{"number":64,"pull_request":{"merged":false},"labels":[],"title":"a PR wearing an issue row"}]' \ + '[{"number":65,"pull_request":null,"labels":[],"title":"list says issue, payload says PR"}]' \ >"$FORGEJO_BOARD/repos_owner_repo_issues_state_open.json" jq -n --arg at "$(iso_at "$INOW")" \ - '{number:64,user:{login:"triage-one"},created_at:$at,body:"", + '{number:65,user:{login:"triage-one"},created_at:$at,body:"", pull_request:{merged:false},labels:[],assignees:[]}' \ - >"$FORGEJO_BOARD/repos_owner_repo_issues_64.json" -printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_64_comments.json" + >"$FORGEJO_BOARD/repos_owner_repo_issues_65.json" +printf '[]\n' >"$FORGEJO_BOARD/repos_owner_repo_issues_65_comments.json" fjb3_out="$(forgejo_board_run)" -check "an object-valued row is not reconciled as an issue by the pass" 1 "" \ - grep -qE '^issueflow: #64' <<<"$fjb3_out" -check "...and the sweep reports the board as empty of ISSUES, correctly" 0 "" \ - grep -qF 'issueflow: no open issues.' <<<"$fjb3_out" +check "the per-issue guard stands down an object-valued payload" 1 "" \ + grep -qE '^issueflow: #65: needs-triage' <<<"$fjb3_out" +check "...and the sweep still completes" 0 "" \ + grep -qF 'issueflow: reconciled.' <<<"$fjb3_out" + +# The same fixture with a null-valued payload MUST reconcile — otherwise the row +# above would pass on a guard that stands everything down. +jq -n --arg at "$(iso_at "$INOW")" \ + '{number:65,user:{login:"triage-one"},created_at:$at,body:"", + pull_request:null,labels:[],assignees:[]}' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_65.json" +fjb4_out="$(forgejo_board_run)" +check "...while a null-valued payload at the same site reconciles" 0 "" \ + grep -qE '^issueflow: #65: needs-triage' <<<"$fjb4_out" + +# And the GitHub shape — key absent entirely — is still an issue. +jq -n --arg at "$(iso_at "$INOW")" \ + '{number:65,user:{login:"triage-one"},created_at:$at,body:"",labels:[],assignees:[]}' \ + >"$FORGEJO_BOARD/repos_owner_repo_issues_65.json" +fjb5_out="$(forgejo_board_run)" +check "...and a github-shaped payload (key absent) reconciles too" 0 "" \ + grep -qE '^issueflow: #65: needs-triage' <<<"$fjb5_out" # -- the rule is pinned at the source, because a comment did not hold -------- # `.pull_request == null` is stated in this file's own header AND at From a55fbaef158c619aeef15f69355ebb487b09cdba Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 15:01:32 +0000 Subject: [PATCH 10/11] =?UTF-8?q?fix(forge):=20forge=5Fcommit=5Fat=20?= =?UTF-8?q?=E2=80=94=20Forgejo=20serves=20a=20single=20commit=20at=20/git/?= =?UTF-8?q?commits/{sha}=20(#209)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the first post-merge sweep after the 0.6.0 merge — #198's own acceptance probe — not by review. Three PRs in one run: labels: #208: could not read the head commit's date: forge_api: HTTP 404 from 'GET repos/heavy-duty/ceremony/commits/f3a1336…' — blocker:unrequested not judged this pass Measured against this instance: forgejo repos/{o}/{r}/commits/{sha} -> 404 forgejo repos/{o}/{r}/git/commits/{sha} -> 200, date under `.created` github repos/{o}/{r}/commits/{sha} -> 200, date nested A fourth asymmetry, alongside the three lib/forge-forgejo.sh's header already records. #198 ported this call site onto the shim with GitHub's path unchanged — correct against GitHub, and the block it lives in (#236 D2) arrived WITH the merge, so nothing here had ever executed it. So it becomes a verb rather than a path at the call site: the caller wants one timestamp and should not have to know either shape. Cost while it stood was bounded and loud rather than silent — guarded_read refused and the sweep said so — but blocker:unrequested could never be judged on this forge. The tests pin each backend's PATH and FIELD, because a stubbed forge_api cannot catch a wrong path; that is exactly how this shipped and why it took a live sweep to find. Swapping the paths reds the forgejo pair; swapping the fields reds the github one. test/run.sh 28/28 under jq 1.7 and jq 1.6; forge-backends 124/124; shellcheck 0.10.0 and actionlint clean. Refs #209 --- actions/labels-reconcile/labels-reconcile.sh | 4 ++-- changelog.d/209.md | 16 +++++++++++++ lib/forge-forgejo.sh | 21 +++++++++++++++++ lib/forge-github.sh | 12 ++++++++++ test/forge-backends.test.sh | 24 ++++++++++++++++++++ 5 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 changelog.d/209.md diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index b6c49f1..449ab12 100755 --- a/actions/labels-reconcile/labels-reconcile.sh +++ b/actions/labels-reconcile/labels-reconcile.sh @@ -1094,8 +1094,8 @@ main() { HEAD_COMMIT_AT="" if [ "$DRAFT" != true ]; then HEAD_COMMIT_ERR_FILE="$(mktemp)" - HEAD_COMMIT_AT="$(forge_api "repos/$REPO/commits/$HEAD_SHA" \ - --jq '.commit.committer.date' 2>"$HEAD_COMMIT_ERR_FILE" || echo "")" + HEAD_COMMIT_AT="$(forge_commit_at "$HEAD_SHA" \ + 2>"$HEAD_COMMIT_ERR_FILE" || echo "")" HEAD_COMMIT_ERR="$(cat "$HEAD_COMMIT_ERR_FILE")" rm -f "$HEAD_COMMIT_ERR_FILE" case "$HEAD_COMMIT_AT" in diff --git a/changelog.d/209.md b/changelog.d/209.md new file mode 100644 index 0000000..320b984 --- /dev/null +++ b/changelog.d/209.md @@ -0,0 +1,16 @@ +### Fixed + +- `blocker:unrequested` is judged on this forge again. The head-commit date was + read from `repos/{o}/{r}/commits/{sha}`, which Forgejo answers **404** — so + every sweep degraded and left the blocker unjudged (#209). + +- `forge_commit_at` is a verb on both backends: GitHub serves a single commit at + the bare path with the date nested, Forgejo at `git/commits/{sha}` with it + under `.created`. The caller asks for one timestamp and knows neither shape + (#209). + +### Added + +- `test/forge-backends.test.sh` pins each backend's path **and** field, because + a stubbed `forge_api` cannot catch a wrong path — which is how this shipped + and why a live sweep was what found it (#209). diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index 0583f86..4331cc6 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -642,6 +642,27 @@ forge_release_exists() { # forge_commit_pulls — the pull requests whose merge produced , as # a JSON ARRAY in GitHub's shape. An empty array is a completed read that # found nothing; a non-zero exit is a read that did not complete. +# forge_commit_at — the commit's committer date, ISO-8601, or empty. +# +# THE FOURTH ASYMMETRY (#209), measured 2026-08-05: +# +# GET /repos/{o}/{r}/commits/{sha} -> 404 (200 on GitHub) +# GET /repos/{o}/{r}/git/commits/{sha} -> 200 date under `.created` +# +# Found by the first post-merge sweep after the 0.6.0 merge, not by review: +# #198 ported this call site onto the shim with GitHub's path unchanged, and +# the block it lives in had never executed here before. Every sweep printed +# `could not read the head commit's date` and left blocker:unrequested +# unjudged. +# +# `.created` and not `.commit.committer.date`: the /git/commits payload is the +# git object, whose top-level `created` is the committer date. The verb hides +# both differences so the caller keeps asking for one timestamp. +forge_commit_at() { + local sha="${1:?forge_commit_at: sha required}" + forge_api "repos/$REPO/git/commits/$sha" --jq '.created' +} + forge_commit_pulls() { local sha="${1:?forge_commit_pulls: sha required}" body code out body="$(mktemp)" diff --git a/lib/forge-github.sh b/lib/forge-github.sh index 36307e1..c2e268e 100644 --- a/lib/forge-github.sh +++ b/lib/forge-github.sh @@ -186,6 +186,18 @@ forge_release_exists() { # forge_commit_pulls — the pull requests whose merge produced , as # a JSON array. GitHub serves the array directly; the forgejo twin builds # one from its single-object endpoint so this call site is identical. +# forge_commit_at — the commit's committer date, ISO-8601, or empty. +# +# A VERB rather than a path at the call site, because the two forges do not +# agree on where a single commit lives: GitHub serves it at /commits/{sha}, +# Forgejo 404s there and serves it at /git/commits/{sha} with the timestamp +# under a different field (#209). The caller wants one timestamp; it should not +# have to know either shape. +forge_commit_at() { + local sha="${1:?forge_commit_at: sha required}" + forge_api "repos/$REPO/commits/$sha" --jq '.commit.committer.date' +} + forge_commit_pulls() { local sha="${1:?forge_commit_pulls: sha required}" errf out rc err errf="$(mktemp)" diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index fd44a84..d304218 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -775,6 +775,21 @@ writes_after() { "$@" >/dev/null 2>&1; cat "$WRITES"; } repo_empty_release() { REPO='' forge_release_exists 1.2.3; } repo_empty_pulls() { REPO='' forge_commit_pulls deadbeef; } +# forge_commit_at — the FOURTH asymmetry (#209). Forgejo 404s on /commits/{sha} +# and serves the object at /git/commits/{sha}, with the committer date under +# `.created` rather than `.commit.committer.date`. A stubbed forge_api cannot +# catch a wrong PATH, which is how #198 shipped GitHub's path here and every +# sweep printed `could not read the head commit's date`. +release_stub 200 '{"created":"2026-08-05T13:11:33Z","commit":{"committer":{"date":"WRONG"}}}' +check "forgejo: the commit date comes from .created" 0 "2026-08-05T13:11:33Z" \ + forge_commit_at deadbeef +fj_not_nested() { ! forge_commit_at deadbeef | grep -q WRONG; } +check "...and never from GitHub's nested field" 0 "" fj_not_nested +check "forgejo: it asks /git/commits/{sha}" 0 "git/commits/deadbeef" \ + writes_after forge_commit_at deadbeef +fj_not_bare_path() { ! grep -qE 'repos/o/r/commits/deadbeef( |$)' "$WRITES"; } +check "...and never the bare /commits/{sha}, which 404s here" 0 "" fj_not_bare_path + release_stub 200 '{"number":7,"merged_at":"2026-01-01T00:00:00Z","labels":[{"name":"release"}]}' check "forgejo: one PR object becomes a one-element array" 0 '"number":7' \ forge_commit_pulls deadbeef @@ -857,5 +872,14 @@ check "github: the tag goes to /git/refs" 0 "git/refs" \ gh_after forge_tag_create 1.2.3 cafebabe check "github: PRs behind a commit use the PLURAL path" 0 "commits/deadbeef/pulls" \ gh_after forge_commit_pulls deadbeef +# The other half of #209's asymmetry: GitHub serves a single commit at the bare +# path, with the date nested. Swapping the two backends' paths must red one of +# these two files, which is the whole point of pinning both. +check "github: a single commit is the BARE path" 0 "commits/deadbeef" \ + gh_after forge_commit_at deadbeef +gh_not_git_commits() { ! gh_after forge_commit_at deadbeef | grep -q 'git/commits'; } +check "...and never Forgejo's git/commits" 0 "" gh_not_git_commits +check "...reading the nested committer date" 0 "commit.committer.date" \ + gh_after forge_commit_at deadbeef summary From fc24fa4b78daf6f7669a10f17ef6681b36248769 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 15:15:35 +0000 Subject: [PATCH 11/11] 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