From f3a1336d426d9d34f01ae008dc36776383b700d2 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:58:32 +0000 Subject: [PATCH] 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"