From a48cc719a4c8a124215626a6c53173d230e278fa Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 14:10:56 +0000 Subject: [PATCH] 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