Review: Improve composability (upstream #269)

[msystemsuser]

auto-patch: on
auto-land: on

Replay of anttiharju#269.

[mergestorm-vortex]

Review progress ██████████ 7/7 files

Comment — found 2 issue(s) at 7c6887c.

Actionable comments posted: 2

🤖 Prompt for AI agents

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

Findings to address:
1. In `@.github/workflows/plan.yml` (line 145, Important):
   `fetch-tags: true`
   This replaces the explicit `git fetch origin 'refs/tags/*:refs/tags/*'` workaround for #258. The checkout here is shallow (no `fetch-depth`, so depth 1), and `actions/checkout`'s `fetch-tags` only lifts git's default tag-following (`--no-tags` is dropped, but the tags refspec is only added for `fetch-depth: 0`), i.e. it can fetch tags that point at the fetched commit rather than all tag refs. If the latest tag points at an older main commit (the normal case for a push-to-main run), `render.sh` (`tag="$(git tag --sort=-creatordate | head -n1)"`) sees no tags, `values.sh` bails out with `TAG=v0.0.0` and emits `TBD` checksums, so the rendered action / `Test action (release binary)` step is wrong or fails. Confirm this on a real run (or set `fetch-depth: 0`, or keep the explicit tag fetch) before relying on it.

2. In `@.release/find-changes-action/action.yml` (line 83, Important):
   `credentials="$(git config --file "$directory/.git/config" --get "includeIf.gitdir:$PWD/$directory/.git.path" || true)"`
   `git config --get` is a *literal key* lookup, so this only finds the value if the key string is byte-identical to what `actions/checkout` wrote into `.git/config` (the key depends on that action's internal convention — whether it records the gitdir as the working-tree path or with the `/.git` suffix, and whether `$PWD` matches the resolved path it used). If it differs in any way, `--get` returns empty, the `case` glob doesn't match, and `rm -f` never runs: the `$RUNNER_TEMP/git-credentials-*.config` file (which holds a live token for the repo) is left behind silently, while the new README section promises the action "removes the temporary directory and its checkout credentials after use". `rm -rf .tmp-anttiharju-compare-changes` does not help because that file lives outside the temp dir. Safer: read whatever key exists, e.g. `git config --file "$directory/.git/config" --get-regexp 'includeIf\.gitdir.*\.path' | awk '{print $2}'`, instead of reconstructing the key by convention.

---

🚢 Vortex specialist fleet

Security — completed · no findings

[mergestorm-vortex]

⚠️ Potential issue · Important

fetch-tags on a shallow checkout may not bring the tag render.sh needs

fetch-tags: true
This replaces the explicit git fetch origin 'refs/tags/*:refs/tags/*' workaround for anttiharju#258. The checkout here is shallow (no fetch-depth, so depth 1), and actions/checkout's fetch-tags only lifts git's default tag-following (--no-tags is dropped, but the tags refspec is only added for fetch-depth: 0), i.e. it can fetch tags that point at the fetched commit rather than all tag refs. If the latest tag points at an older main commit (the normal case for a push-to-main run), render.sh (tag="$(git tag --sort=-creatordate | head -n1)") sees no tags, values.sh bails out with TAG=v0.0.0 and emits TBD checksums, so the rendered action / Test action (release binary) step is wrong or fails. Confirm this on a real run (or set fetch-depth: 0, or keep the explicit tag fetch) before relying on it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

In `@.github/workflows/plan.yml` (line 145, Important):
`fetch-tags: true`
This replaces the explicit `git fetch origin 'refs/tags/*:refs/tags/*'` workaround for #258. The checkout here is shallow (no `fetch-depth`, so depth 1), and `actions/checkout`'s `fetch-tags` only lifts git's default tag-following (`--no-tags` is dropped, but the tags refspec is only added for `fetch-depth: 0`), i.e. it can fetch tags that point at the fetched commit rather than all tag refs. If the latest tag points at an older main commit (the normal case for a push-to-main run), `render.sh` (`tag="$(git tag --sort=-creatordate | head -n1)"`) sees no tags, `values.sh` bails out with `TAG=v0.0.0` and emits `TBD` checksums, so the rendered action / `Test action (release binary)` step is wrong or fails. Confirm this on a real run (or set `fetch-depth: 0`, or keep the explicit tag fetch) before relying on it.

[mergestorm-vortex]

⚠️ Potential issue · Important

Credential cleanup may silently no-op

credentials="$(git config --file "$directory/.git/config" --get "includeIf.gitdir:$PWD/$directory/.git.path" || true)"
git config --get is a literal key lookup, so this only finds the value if the key string is byte-identical to what actions/checkout wrote into .git/config (the key depends on that action's internal convention — whether it records the gitdir as the working-tree path or with the /.git suffix, and whether $PWD matches the resolved path it used). If it differs in any way, --get returns empty, the case glob doesn't match, and rm -f never runs: the $RUNNER_TEMP/git-credentials-*.config file (which holds a live token for the repo) is left behind silently, while the new README section promises the action "removes the temporary directory and its checkout credentials after use". rm -rf .tmp-anttiharju-compare-changes does not help because that file lives outside the temp dir. Safer: read whatever key exists, e.g. git config --file "$directory/.git/config" --get-regexp 'includeIf\.gitdir.*\.path' | awk '{print $2}', instead of reconstructing the key by convention.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid concrete bugs, skip the
rest with a brief reason, keep changes minimal, and validate. Skip Decision required,
policy forks, and "consider X" alternatives. Do not add new features, refactors, or
architecture beyond the fix; prefer the smallest diff.

In `@.release/find-changes-action/action.yml` (line 83, Important):
`credentials="$(git config --file "$directory/.git/config" --get "includeIf.gitdir:$PWD/$directory/.git.path" || true)"`
`git config --get` is a *literal key* lookup, so this only finds the value if the key string is byte-identical to what `actions/checkout` wrote into `.git/config` (the key depends on that action's internal convention — whether it records the gitdir as the working-tree path or with the `/.git` suffix, and whether `$PWD` matches the resolved path it used). If it differs in any way, `--get` returns empty, the `case` glob doesn't match, and `rm -f` never runs: the `$RUNNER_TEMP/git-credentials-*.config` file (which holds a live token for the repo) is left behind silently, while the new README section promises the action "removes the temporary directory and its checkout credentials after use". `rm -rf .tmp-anttiharju-compare-changes` does not help because that file lives outside the temp dir. Safer: read whatever key exists, e.g. `git config --file "$directory/.git/config" --get-regexp 'includeIf\.gitdir.*\.path' | awk '{print $2}'`, instead of reconstructing the key by convention.

[mergestorm-cyclone]

@mergestorm-cyclone Cyclone applied fixes and pushed to review/pr-269. Not verified: static checks were skipped (no applicable Node or Python checks for changed paths); pushed without a typecheck gate.

batch=review-5327037552 sha=d22f14c verify=skipped

[mergestorm-vortex]

Review progress ██████████ 7/7 files

Approve — reviewed files look good at d22f14c.

✅ All clear — nothing to fix.

[msystemsuser]

@mergestorm-tempest review

[mergestorm-tempest]

Tempest — deep systemic review

Orchestrator: DeepSeek V4.1 Flash

Verdict: clear — Tempest traced the systemic surface and found no blocking issues.

No error-severity finding survived; verdict is neutral with two warnings and one info note. The most important result: the new prepare/clean-up pair in find-changes-action is not fail-safe — a bare mkdir guard is followed by an unconditional always()-gated rm -rf, so the very collision the guard is meant to catch destroys the colliding directory in the consumer's workspace.

Review confidence

high — full diff inlined (11316 chars)

Systemic risks

[warning] Prepare checkout's non--p mkdir guard is followed by an unconditional rm -rf, so the collision it guards against destroys the colliding directory — .release/find-changes-action/action.yml:88

The new prepare/clean-up pair is not fail-safe. mkdir .tmp-... is written without -p (action.yml:54, with the comment "why would you have directory with this name in your repository?") as a guard, and the README documents the same assumption (README.md:133). The guard's failure is then followed by exactly the operation it protects against:

• Step order is install/verify (action.yml:32-50), then Prepare checkout (:51-55). If any earlier step fails, the temp directory is never created — yet Clean up checkout is gated only on if: always() (:77), not on "the action created this path", and ends with rm -rf .tmp-anttiharju-compare-changes (:88).
• When mkdir fails because a file/directory of that name already exists, the Checkout/Run steps are implicit success() steps and are skipped, while the always() clean-up still runs and deletes the pre-existing directory.

Named sequence: consumer repo (or an earlier step of the job) contains .tmp-anttiharju-compare-changes/ → Prepare checkout fails (File exists) → Clean up checkout runs → that directory is gone for every later step of the job. The composite action's own doc promises only that "The action also removes the temporary directory ... after use, including after a failed step" (README.md:133), not that it removes a directory it never created. Gating the rm -rf on the temp path having been created keeps the guard loud without deleting unrelated workspace content.

_{evidence: .release/find-changes-action/action.yml:51, .release/find-changes-action/action.yml:54, .release/find-changes-action/action.yml:77, .release/find-changes-action/action.yml:88, .release/find-changes-action/README.md:133}

---

[warning] Exact-version pins are frozen by dependabot's anttiharju/* ignore, and the published READMEs now advertise v0.12.19 verbatim — .github/dependabot.yml:29

This PR replaces the moving @v0 major tag with the concrete @v0.12.19 in 4 tracked files plus both rendered READMEs. Two independent mechanisms then stop those strings from ever moving again:

1. Dependabot is explicitly configured to ignore minor+patch updates for anttiharju/* (.github/dependabot.yml:29-32), so it will never propose 0.12.19 -> 0.12.20 (patch) or 0.12.19 -> 0.13.0 (minor); only a 1.0.0 bump would be proposed. Nothing in the release pipeline rewrites these pins either: the release job's steps are github/nur-packages/crate/homebrew-tap/release-action x2/documentation (build.yml:38-79), none of which touch this repo's own uses: strings.
2. .release/find-changes-action/README.md:43,120,122 and .release/compare-changes-action/README.md:26,28,33 are the source for the sibling repositories: render.sh copies every file of the package dir, README.md included, into the checkout of anttiharju/<pkg>-changes-action (render.sh:139) and .md files are excluded from substitution, i.e. copied byte-for-byte (render.sh:145), then committed there by .github/actions/release/action/action.yml:56,58-64. So the usage examples published with every future release will still tell users to pin @v0.12.19.

Decision required: keep @v0 floats for the two in-repo consumers of this repo's own actions, or exempt these two dependency names from the anttiharju/* ignore and/or drop the literal version from the README examples (the only place a version is hardcoded in user-facing text).

Interaction with verification: nothing in this repo renders or invokes .release/find-changes-action/.output (only compare-changes-action/.output is rendered at plan.yml:195,228 and exercised at plan.yml:199,208,232), so the only in-repo execution path for find-changes-action is the pinned released tag (plan.yml:35,150, example.yml:23). With the pin frozen, the checkout/clean-up rewrite in this PR has no feedback loop at all until someone manually bumps it.

_{evidence: .github/dependabot.yml:29, .github/dependabot.yml:30, .github/workflows/plan.yml:150, .github/actions/detect-changes/action.yml:11, .release/find-changes-action/README.md:43, .release/render.sh:139, .release/render.sh:145, .github/actions/release/action/action.yml:56}

---

[info] Unsupported events now fail before the binary, losing the binary's explicit event error — .release/find-changes-action/action.yml:71

The Run step has no if: and pins working-directory: .tmp-anttiharju-compare-changes/${{ github.repository }} (action.yml:71), but the only steps that create that path are the two checkouts gated on github.event_name == 'pull_request' || 'merge_group' (:56) and == 'push' (:63). On any other event (workflow_dispatch, schedule, repository_dispatch, release) no checkout runs, so the runner fails to start the step ("No such file or directory" for the working directory) instead of the previous behaviour, where the step ran in the workspace root and the binary itself reported find-changes only works on pull_request, merge_group, push events. (src/find/mod.rs:34).

Severity is info: the action never supported those events (its event gating matches src/find/mod.rs:9-35 exactly), all in-repo callers are pull_request/push only (plan.yml:3-8, build.yml:3-6, example.yml:3-4), and README.md:12 scopes the action to push/pull_request/merge_group. Nothing in this repo breaks; the cost is a diagnosability regression for consumers on other events. If unwanted, the Run step needs the same if: as the checkouts so the binary produces its own message.

_{evidence: .release/find-changes-action/action.yml:71, .release/find-changes-action/action.yml:56, .release/find-changes-action/action.yml:63, src/find/mod.rs:34}

Traced & cleared (9)

• Run step's working-directory may not exist on non-PR/push events; mkdir without -p; in-repo callers — Checkout gating (action.yml:56,63) matches the binary's supported event set exactly (src/find/mod.rs:9-35), so no supported-event caller can miss the checkout. All in-repo callers are PR/push (plan.yml:3-8, example.yml:3-4). Non-PR/push events were already unsupported; the only delta is the failure surfacing as a runner-level missing working dir instead of the binary's message — filed as info. The bare-mkdir guard's failure path is NOT clean (followed by the always()-gated rm -rf) — filed as a separate warning.
• enumerate remaining @v0 / floating refs; confirm v0.12.19 is a real pipeline-produced tag — rg across .github/.release: no compare-changes-action@v0 / find-changes-action@v0 remain. Remaining floats are anttiharju/actions/get-labels@v0, require-label@v0 (different repo), actions/cache@v6, actions/checkout@v7 — all permitted by the zizmor ref-pin policy (zizmor.yml:13-14). Tag shape: pipeline tags siblings as v<inputs.version> (.github/actions/release/action/action.yml:69-72) and this repo identically (release/github/action.yml:13-15,45), so v0.12.19 is the exact emitted shape. Whether 0.12.19 is the current release is not verifiable offline.
• consumers that relied on find-changes-action's old workspace checkout — Every in-repo consumer now performs its own checkout: plan.yml:29-32 before nix build .#ci (56-58) and docker push (78-116); validate job plan.yml:141-146 before git ls-files steps (:253,263,292) and before render.sh (:195,228); example.yml:16-19 before its shellcheck (:33). rg find-changes-action shows no other caller. Internal checkout keeps persist-credentials: true (action.yml:62,68) needed by the binary's git fetch (src/find/mod.rs:21-24); the new consumer checkouts correctly use persist-credentials: false.
• credentials cleanup reads includeIf key and only deletes $RUNNER_TEMP/git-credentials-*.config — Already covered by Vortex comments 3/6 on the literal git config --get key lookup, so not re-flagged. Surrounding logic is self-consistent: the checked-out repo's .git/config is removed unconditionally by the final rm -rf (action.yml:88) regardless of the includeIf match at :83, and the case at :85 is anchored to "$RUNNER_TEMP"/git-credentials-*.config, so residue is at most a stale includeIf entry pointing at a deleted file (git silently ignores missing includes), not a live credential.
• removed git fetch for tags vs. the new fetch-depth: 0 / fetch-tags: true — Same validate job: the replacement fetch-depth: 0, fetch-tags: true is on that job's checkout (plan.yml:141-146), which runs the steps needing tags — render.sh:47 (git tag --sort=-creatordate) and render.sh:62 (git describe) invoked at plan.yml:195,228. The release job's build.yml:33-36 checkout is untouched and needs no fetched tags (it creates/pushes the tag locally at release/github/action.yml:15,45 and takes version from gh release view). Checkout fetch mechanics already covered by Vortex 2/5.
• does GH_TOKEN addition to the release-action render step fix a real failure or mask a permissions problem — Fixes a real failure. render.sh sources values.sh on cache miss (render.sh:80); caches are generated, not tracked, so a fresh job always takes NO_CACHE=1 (render.sh:71) and runs values.sh, which calls gh api (values.sh:14) and gh release download (values.sh:23). Without a token the negated condition at values.sh:14 silently falls into the TBD branch that scripts/verify.sh:12 then rejects at consumer runtime — i.e. it would publish a broken sibling action, not fail loudly. Scope is sufficient (contents: write at build.yml:28-31), and three sibling render sites already set the same env — aligns an outlier.
• SHA pinning policy: composite action pins actions/checkout by SHA while workflows/READMEs use @v7 — No policy breach. .github/zizmor.yml:10-15 sets unpinned-uses actions/*: ref-pin, anttiharju/*: ref-pin, cachix/*: ref-pin, so tag refs are sanctioned (and zizmor . runs at plan.yml:294-297). The SHA pin in .release/find-changes-action/action.yml:58,65 (with # v7.0.1) is stricter than required and matches pre-existing style for actions/cache@55cc83... (:27). actionlint (plan.yml:258) and action-validator (:253) only check syntax; README examples are markdown, unparsed by zizmor.
• docs/implementation sync; are READMEs rendered (so manual edits could be overwritten); stale 'handles checkout' claims — .release/*/README.md are authored sources, not generated: the release path copies every file of the package dir into the sibling checkout and commits it (render.sh:139 + release/action/action.yml:48,56,58-64), so the new 'Checkout' section (find-changes-action/README.md:131-135) is a real published doc change, and .md is exempt from envsubst (render.sh:145). No remaining 'handles checkout' claim (rg 'handles checkout' -> no hits); docs/README.md:7 only links the marketplace page. The stale artifact that remains is the literal @v0.12.19 in the README examples — filed as the drift finding.
• repo-name collisions, destructive rm -rf, and whether the '*' .gitignore reliably hides the nested checkout — The .gitignore mechanism works: the file is written inside the temp dir (action.yml:55) with content *, matching every entry including the nested owner/repo checkout and its .git, so it is invisible to git status and git ls-files --others --exclude-standard (render.sh:139). The nested path is only two levels (action.yml:60,67) and both the mkdir and the final rm -rf stay within $GITHUB_WORKSPACE, so no path escapes. The collision case is where the design does not hold up — it deletes the colliding directory rather than failing safely (see the warning).

Out of scope / deferred (2)

• Whether v0.12.19 is the current release at this head cannot be verified offline — the snapshot has no git refs/network, and Cargo.toml intentionally carries no version (patched at release time in .github/actions/release/github/action.yml:22).
• The bare-mkdir guard ('why would you have a directory with this name in your repository?') is an intentional design assumption; whether to keep that assumption or make the clean-up conditional on path creation is a product decision, not a blocker.

_{Tempest reviews the bigger picture (cross-file contracts, config drift, platform limits, scale) and never patches. Fast diff review is handled by Vortex.}