-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5526
bhamodi · open, request changes drafted · reviewed at 9ac8013f72b · view on GitHub
Verdict: request changes — the pin is right and it has no updater behind it
The review asks for .github/dependabot.yml to point at this directory in the same PR. It did not merge because Dependabot does not scan .github/actions/, so the pin is permanent: the action freezes at its 3 August build in the two jobs that publish to npm, and the property the PR exists to protect starts decaying the day it lands.
.github/actions/setup runs inside release.yml's publish and canary jobs (release.yml:78, :138), both holding id-token: write for npm trusted publishing with provenance. Until this PR it resolved pnpm/action-setup@v6, a mutable tag: whoever owns that tag upstream can move it, and the next provenanced publish would run the moved code under this repo's npm identity. This is the honest "defect nobody has hit" exception, with the reachable state named.
Verifying the SHA is the safety review here, since a fork head touching a privileged CI file is the exact shape of a supply-chain attack. The v6 ref dereferences to an annotated tag and then to the pinned commit, tagged 2026-08-03, identical to v6.0.10, the latest release; the commit itself is a README change upstream. Nothing was installed, built or executed from this branch.
(1 decision · 1 changed line of 6)
-
pnpm/action-setup@v6becomes the commit hash with a# v6marker.
A tag is a label someone can move; a commit hash is not. The publish jobs currently trust a label, so the code they run is whatever the label points at on the day they run. Naming the hash instead means those jobs run the same bytes every time, and nobody upstream can change them afterwards. The other five added lines are the comment explaining it, nothing is piggybacked, and the # v6 marker is the format the repo's other pins already use.
It lives with its owner and reaches every seam, because all 25 call sites across 17 workflows route through this one composite. It matches a convention that is written down rather than assumed — crowdin-upload.yml:33 carries the sentence "pinned by SHA per repo policy."
Nobody sees anything today. v6 resolves to the pinned commit right now, so CI executes byte-identical action code before and after. No end user of an app built on Astryx, and no builder writing against the library, is reached at all.
The person this reaches is the next maintainer of this repo, and what they get is worse than what they have. Today @v6 collects upstream patches on its own; after this, the only mechanism is the sentence the PR writes into the file — "Bump by updating the SHA to the new tag's commit" — addressed to nobody in particular.
What landing it newly exposes: the repo has a Dependabot blind spot over .github/actions/, harmless today because everything in that file floats on a tag. This PR is what makes it consequential, so it belongs to this change rather than to inherited debt.
No API change. Nothing exported, no prop, no type, no signature, no default.
n/a — structural only, and not source at all. The style grep over the changed file returns 0, and no theme target is added, removed or renamed.
Nothing new ossifies: one reference string replaces another in a private CI file.
No behaviour change, and this is what was walked to conclude it. The pinned SHA and v6 are the same commit — the annotated tag dereferences to it, and so does v6.0.10 — so the action code CI executes is identical. The composite's inputs, its step order, and the packageManager-derived pnpm version are untouched. The state walk is not reachable: the diff adds no state, no default, no conditional, no early return.
- API — no. Nothing compiles against a workflow file.
- Visual — no. Nothing renders; no outer box exists to grow.
- Theme — no. No target, token or override is involved.
Zero effects — YAML, not React. No listener, no observer, no layout read. No bundle delta: the PR's own analysis report finds no component packages changed. CI runtime is unchanged — same action, same commit, same download, same pnpm store cache.
None, and the reason is the acceptable one: not renderable — a CI config file with no DOM, no CSS, no component. Corroborated three ways: the style grep returns 0, the PR's analysis report finds no new or modified components, and CI reports its visual and a11y jobs as skipping on this head.
Nothing here is touched, and this is what was checked: aria-|role=|useTranslator|t('@astryx over the changed file → 0. No rendered element, no user- or AT-facing string, no catalog key, no direction. The a11y job skipped this head and the baseline file is untouched by the diff; with no CSS, no mirroring question exists.
request changes, on one sentence you could say to the author's face: pinning here freezes the action forever, because Dependabot does not look in .github/actions/.
1. The pin has no updater behind it, in a file Dependabot demonstrably
cannot see
→ pnpm/action-setup is frozen at the 3 Aug build in both npm-publishing
jobs, permanently and silently — the property the PR exists to
protect decays from the day it lands
· .github/actions/setup/action.yml:33
2. [not blocking] "the one exception" is not quite right
→ a cross-repo action in another Meta repo is also on a mutable tag,
in a job holding id-token: write
· .github/workflows/internal-registry.yml:27
Finding 1 is confirmed three independent ways, each a different method. Reading the same file: actions/setup-node@v6 sits five lines below the pin while pr-comment.yml:38 is on v7. Git history: #4641, the 3 August github-actions group bump, touched three workflow files and skipped this one, which had existed since 11 July. The action's own history: Dependabot last bumped pnpm/action-setup in #2492, across eight workflow files, before #3753 moved it into the composite, and it has not touched it since.
The fix is cited rather than designed: .github/dependabot.yml already has the github-actions ecosystem entry, and #4641 proves Dependabot rewrites SHA pins with version markers when it can see the file. The ask is to point it at this directory.
The finding belongs to impact and is raised there, because the slot that noticed it owns it: this is a change in who maintains the reference, which is a question about a person rather than a breaking axis or a cost. No design call is involved — CI hardening against a convention already written down.
Three things found and not spent on the author: the pinned commit is the latest release, so there is no version delta today and the freeze is prospective rather than current; 25 call sites reach this composite across 17 workflows, which is every CI job in the repo; and the repo's own review gate is pending on community contribution, so this cannot auto-merge whatever the verdict.
Not verified: Dependabot was not run — the blind spot is inferred from this repo's own history, three independent confirmations, rather than from executing the updater — and the proposed directories: snippet was not checked against Dependabot's schema. The shape is proposed rather than tested; the finding does not depend on it, only the convenience of the snippet does.
Thanks for this — pinning the publish path is the right instinct, and the SHA checks out.
Dependabot doesn't look inside
.github/actions/:actions/setup-node@v6five lines below is still a major behind the rest of the repo. So this pin is permanent — nothing will ever move it, in the two jobs that publish to npm, and the bump note you added is the whole maintenance story.Ideally I'd like the updater pointed at this file in the same PR:
- package-ecosystem: "github-actions" directories: - "/" - "/.github/actions/setup"(The cross-repo action in
internal-registry.ymlis on a mutable tag too, so this isn't quite the last one.)Does adding that directory work for you here? If you'd rather talk it through with someone, we're in Discord.
Inline: .github/actions/setup/action.yml:33 — Dependabot doesn't scan this directory. Might need a directories entry in dependabot.yml. · :38 — Hmm, this one's still v6 — pr-comment.yml went to v7 on 3 Aug.
One review, two gate passes.
-
Gate 1 — wrong on the bucket, right on the block. The block was real and confirmed three ways, but the draft called the author a contributor and then handed him a Discord link without recording the evidence that decides which he is — the permission API returning
read, absence from both owners files, ten merged PRs including three in.github/. That call decides both the verdict's default and whether the link belongs, and unrecorded it reads as a coin flip. Six wording violations went with it: the comment restated the author's own SHA verification back to him, announced a finding instead of being one, carried lineage he cannot act on, used the same date three times in 142 words, ended on the Discord clause rather than on the ask, and put a second finding he cannot act on in this PR between the ask's code block and the ask itself. - Gate 2 — clean. 142 words became 114 by deletion: the SHA restatement is one clause, the group-bump lineage moved out of the comment, and the operative last instruction is the ask, with the sign-off after it. Both inlines name a problem and hedge; both anchors were opened at the PR head; the head was re-fetched and unmoved; the diff was grepped to confirm the author had not already made the change being asked for; and the fix is cited rather than invented, with the repo's own SHA-pinning convention quoted from the file that states it.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines. The presentation's merge trailer is a hold whose content is finding 1 — the pin freezes the action until Dependabot can see the file. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5526) line goes in before the attribution.