Bump internal pins to v1.2 and document working-directory - #16
Conversation
Points the four internal `uses:` references at bc24593, the commit that actually declares `working-directory` on both composite actions. Until now they named 61ec8ca, which does not, so the input was silently ignored and the agents ran at the repository root. This is the commit that makes subdirectory support real, so the monorepo usage docs held back from the feature PR land here too. Also ends the tag drift that made the old pins misleading. The `# v1.1` comments claimed a version that had moved: on origin, v1.1 resolved to c3b9a4d, twelve commits ahead of the 61ec8ca the pins named. Two changes stop that recurring: - The consumer-facing README examples now reference the release tag rather than a SHA. A documented SHA has to be re-edited every release and had already drifted twice, and the reusable-workflow ref in particular cannot be a SHA in the commit that defines it, since that SHA does not exist yet. - The versioning section states the rule the internal pins actually follow: they name the previous commit, because a commit cannot pin itself, and the release tag is cut on the commit that bumps them. Verified here — the composite actions, upload-report and scripts are byte-identical between bc24593 and this commit, so `# v1.2` describes the code accurately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Review: Bump internal pins to v1.2 and document working-directory
Overview
This PR corrects a silent regression from #15: the four internal uses: pins in triage.yml still pointed at 61ec8ca, a commit whose composite actions did not yet declare the working-directory input, causing it to be silently dropped. The fix bumps those pins to bc24593 (the #15 merge commit). The README gains consumer-facing monorepo docs and switches its example refs from SHA-pinned to @v1.2 tag.
The SHA equivalence claim is verified: git diff bc2459322269c0a38a5e1c642845604480233005 HEAD -- action.yml setup/action.yml upload-report scripts/ produces no output, confirming the composite-action code at bc24593 is byte-identical to current HEAD.
Findings
MEDIUM — Consumer examples downgraded from immutable SHA pins to mutable tag refs
Files: README.md lines ~43, ~74, ~186
Before:
uses: ShiplightAI/ci-triage/.github/workflows/triage.yml@61ec8ca3143617d87c261226a43b92457ca79566 # v1.1
uses: ShiplightAI/ci-triage/upload-report@61ec8ca3143617d87c261226a43b92457ca79566 # v1.1After:
uses: ShiplightAI/ci-triage/.github/workflows/triage.yml@v1.2
uses: ShiplightAI/ci-triage/upload-report@v1.2This is a supply-chain security regression. The old SHA pins were immutable; @v1.2 can be silently moved to a different commit after consumers adopt it. Given this tool runs in consumer repos' privileged jobs (contents: write, live credentials), this is directly in scope of the stated review priority for supply-chain pinning.
The PR's justification — that SHA docs drift and the reusable-workflow ref cannot be a SHA in the same commit — is partly valid. The reusable-workflow ref in particular truly cannot self-pin (circular dependency). However:
- The
upload-reportaction has no such constraint; it could stay SHA-pinned in examples. - The reusable workflow could use the pattern
@v1.2 # SHA: <sha>in its primary example (tag ref, but the SHA documented beside it), making immutable pinning just as discoverable as the mutable form.
The versioning section does document how to substitute the SHA, but the primary copy-paste examples are mutable. Consumers in a hurry — or those who don't read past the quick-start — will adopt mutable refs.
Suggested resolution: Keep @v1.2 as the default form in examples (the ergonomics argument is sound), but add the SHA-with-comment form as a parallel example in the quick-start, not buried in the versioning section. For upload-report, prefer SHA-pinned primary examples since there's no circular-dependency constraint.
LOW — v1.2 tag does not exist at review time
File: README.md (all three example refs)
The README references @v1.2 but git tag --list 'v1*' returns nothing — the tag will be cut post-merge per the PR instructions. Until that happens, any consumer who copies the examples will get a GitHub Actions resolution failure.
This is a sequencing issue, not a code defect, but the merge-then-tag ordering must be followed precisely. Consider adding a checklist item or branch protection noting the tag must be cut immediately after merge.
LOW — extra_env heredoc delimiter not guarded against value collision
File: .github/workflows/triage.yml, "Apply extra-env" step (not changed by this PR, but worth noting for high-trust context)
echo "${key}<<__CITRIAGE_ENV_EOF__"
printf '%s\n' "$val"
echo "__CITRIAGE_ENV_EOF__"If a consumer accidentally sets a secret value that contains a line exactly equal to __CITRIAGE_ENV_EOF__, the GITHUB_ENV write would be truncated or malformed. The extra_env secret is consumer-controlled (trusted), so this is low severity, but the documentation says "single-line values only" without enforcement. A defensive guard (if printf '%s' "$val" | grep -qxF '__CITRIAGE_ENV_EOF__'; then echo "..."; exit 1; fi) would harden this step against misconfiguration.
What looks good
- Internal SHA pins are correct and consistent. All four
triage.ymlpins point tobc24593, which is verified to carry theworking-directoryinput declaration. The# v1.2comment is honest (the code at the pinned SHA is byte-identical to what HEAD will tag as v1.2). - No injection of untrusted data into run: blocks. All
github.event.*values enter steps as environment variables (env:assignments), not as inline${{ }}expressions inrun:shells. Theworking-directoryvalidation guard runs in both jobs before any privileged work. - No hardcoded repo names, secret names, URLs, or runner labels. All project-specific values flow through inputs/secrets. The workflow is properly generic.
- Least privilege is maintained. Triage job stays at
contents: read+actions: read; the write scope is confined to the autofix job. - Third-party action pins are SHA-pinned.
actions/checkout,actions/setup-node,actions/upload-artifact,actions/download-artifactall use full SHA pins. - Monorepo docs are accurate and complete. The new
working-directorysection correctly describes the project-relative semantics forallowed-pathsandtarget_file.
Summary
One MEDIUM issue (mutable consumer-example refs in high-trust CI tooling) blocks approval. The fix is straightforward: augment the primary examples with the SHA-pinned equivalent, or at least make the SHA form equally prominent. The LOW items are informational. The mechanical correctness of the pin bump itself is sound.
Review pushback on the previous commit was right, and the evidence is in this repo's own history: v1.1 was moved from 61ec8ca to c3b9a4d after the pins were written, so a bare tag ref is mutable in practice here, not just in theory. Making the copy-paste examples mutable would have been a real downgrade for a tool that runs in consumer repos with contents:write and live credentials. Both `upload-report` examples go back to a full SHA — that action has no circular-dependency constraint, and its code at bc24593 is byte-identical to what v1.2 tags. Only the reusable-workflow ref genuinely cannot be pinned here, because the correct SHA is the release commit that does not exist yet. That one keeps the tag, but the quick-start now shows the `git ls-remote` one-liner to resolve it and the SHA form to paste, instead of burying that in the versioning section. The versioning section now says pin by SHA rather than recommending the tag, and records that the tag must be cut immediately after the pin bump merges — between those two moments the documented tag does not resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed [M1] mutable consumer refs — you're right, reverted. The strongest evidence is in this repo's own history rather than in principle: Taking both parts of the suggested resolution:
The versioning section now says to pin by SHA rather than recommending the tag, and states why: tags here have moved. [L1] [L2] |
There was a problem hiding this comment.
Review summary
This PR bumps four internal uses: SHA pins from 61ec8ca (which pre-dates the working-directory input) to bc24593 (the merge commit of #15 that adds it), and expands the README with monorepo documentation. The fix is correct and the documentation additions are accurate.
I verified independently: git diff bc2459322269c0a38a5e1c642845604480233005 HEAD -- action.yml setup/action.yml upload-report/action.yml scripts/ is empty, so the composite-action code at the pinned SHA is byte-identical to HEAD. The working-directory input is declared in both action.yml and setup/action.yml at that SHA.
Findings
CRITICAL
None.
HIGH
None.
MEDIUM
None.
LOW — README quick-start now uses mutable @v1.2 tag for the reusable-workflow ref
Files: README.md (quick-start at line ~41, integration section at line ~200)
The two reusable-workflow examples changed from a (stale) SHA to the bare @v1.2 tag. This is the one ref that genuinely cannot be pre-resolved as a SHA at PR-creation time, and the PR description explains the constraint. The README's Security section now explicitly warns that bare tags are mutable in practice and provides the git ls-remote workaround. The old SHA had already drifted twice and was stale, so the new approach is arguably a net improvement in practice — but the primary copypasta pattern for consumers is now a mutable ref for the highest-trust entry point (workflow_call). Low severity given the documentation, but worth watching: tag v1.2 promptly after merge (the PR already calls this out) and consider whether future major versions can pre-resolve the SHA by staging the reusable-workflow change one commit ahead of the pin bump.
LOW — Brief unresolved-ref window between merge and tag
File: README.md
Between the moment this PR merges and the moment v1.2 is applied, any consumer who copies the quick-start will get an Actions "couldn't find remote ref" error. The PR description acknowledges this and asks for an immediate tag. A pre-merge tag would close the window entirely but requires the SHA to exist first — inherent constraint noted.
Pre-existing observations (not introduced by this PR)
extra_env key validation (triage.yml:413–421): the Apply extra-env step splits on the first = and writes the key directly to $GITHUB_ENV without validating key format. A key containing spaces or other shell metacharacters would corrupt the env file. Since extra_env is a consumer-controlled secret in a high-trust context this is low risk, but an explicit [[ "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]] guard would be more defensive.
Unquoted $SCOPED_PATHS in git diff call (triage.yml:580): word-splitting is intentional here, but paths from allowed-paths that contain spaces or glob characters would silently misfire. The consumer controls this input, so severity is low.
Positive observations
- All four internal pins are now consistently at
bc24593— no version split between the triage and autofix jobs. - Third-party action pins (
actions/checkout,actions/upload-artifact,actions/download-artifact) remain SHA-pinned throughout. - The new monorepo documentation correctly clarifies that
allowed-pathsandtarget_fileare project-relative, and theupload-reportsnippet in the new section uses a SHA pin (bc24593). working-directoryvalidation (absolute path,..segments, newline) is applied redundantly in the reusable workflow, composite actions, and again in the high-privilege autofix job — good defence-in-depth.- No hardcoded repo names, secret names, URLs, or runner labels are introduced.
Follow-up to #15. That PR added
working-directorybut could not make it work: the four internaluses:pins still named61ec8ca, a commit whoseaction.ymlandsetup/action.ymldo not declare the input. GitHub Actions only warns on an unknown composite-action input, so the value was silently dropped and both agents ran at the repository root.This PR points those pins at
bc24593— the merge commit of #15, which does declare it — and restores the monorepo usage docs that were deliberately held back somainnever described the feature as usable while it wasn't.Ending the tag drift
The old
# v1.1comments named a version that had moved. On origin,v1.1resolves toc3b9a4d, twelve commits ahead of the61ec8cathe pins referenced, so the comment and the SHA disagreed. Two changes stop that recurring:5697e59→61ec8ca→ stale). The reusable-workflow ref in particular cannot be a SHA in the commit that defines it, because that SHA does not exist until the commit is made. The versioning section still tells consumers how to substitute the SHA if they want an immutable pin.That second point is what makes
# v1.2honest here. Verified:git diff bc24593 HEAD -- action.yml setup/action.yml upload-report scripts/is empty, so the composite-action code at the pinned SHA is byte-identical to whatv1.2will tag.Scope
Only
.github/workflows/triage.yml(4 pin lines) andREADME.md(docs). No behavior change beyond the pins taking effect.After merge
Tag
v1.2on this PR's merge commit — not onbc24593, since that commit's owntriage.ymlstill carries the old pins. Do not movev1.1.Verification
37 regression tests from #15 still pass against the updated workflow.
🤖 Generated with Claude Code