Skip to content

Bump internal pins to v1.2 and document working-directory - #16

Merged
feng-shiplight merged 2 commits into
mainfrom
pin-bump-v1.2
Aug 12, 2026
Merged

Bump internal pins to v1.2 and document working-directory#16
feng-shiplight merged 2 commits into
mainfrom
pin-bump-v1.2

Conversation

@feng-shiplight

Copy link
Copy Markdown
Contributor

Follow-up to #15. That PR added working-directory but could not make it work: the four internal uses: pins still named 61ec8ca, a commit whose action.yml and setup/action.yml do 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 so main never described the feature as usable while it wasn't.

Ending the tag drift

The old # v1.1 comments named a version that had moved. On origin, v1.1 resolves to c3b9a4d, twelve commits ahead of the 61ec8ca the pins referenced, so the comment and the SHA disagreed. Two changes stop that recurring:

  • Consumer-facing README examples now use the release tag rather than a SHA. A documented SHA must be re-edited every release and had already drifted twice (5697e5961ec8ca → 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.
  • The versioning section now states the rule the internal pins actually follow: they name the previous commit, since a commit cannot pin itself, and the release tag is cut on the commit that bumps them.

That second point is what makes # v1.2 honest 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 what v1.2 will tag.

Scope

Only .github/workflows/triage.yml (4 pin lines) and README.md (docs). No behavior change beyond the pins taking effect.

After merge

Tag v1.2 on this PR's merge commit — not on bc24593, since that commit's own triage.yml still carries the old pins. Do not move v1.1.

Verification

37 regression tests from #15 still pass against the updated workflow.

🤖 Generated with Claude Code

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>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.1

After:

uses: ShiplightAI/ci-triage/.github/workflows/triage.yml@v1.2
uses: ShiplightAI/ci-triage/upload-report@v1.2

This 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:

  1. The upload-report action has no such constraint; it could stay SHA-pinned in examples.
  2. 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.yml pins point to bc24593, which is verified to carry the working-directory input declaration. The # v1.2 comment 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 in run: shells. The working-directory validation 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-artifact all use full SHA pins.
  • Monorepo docs are accurate and complete. The new working-directory section correctly describes the project-relative semantics for allowed-paths and target_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>
@feng-shiplight

Copy link
Copy Markdown
Contributor Author

Pushed 47c58cc.

[M1] mutable consumer refs — you're right, reverted. The strongest evidence is in this repo's own history rather than in principle: v1.1 was moved from 61ec8ca to c3b9a4d after the pins referencing it were written. That is exactly the drift that started this work, so a bare tag ref is mutable in practice here, and making the primary copy-paste examples mutable would have been a real downgrade.

Taking both parts of the suggested resolution:

  • upload-report is SHA-pinned again in both examples (bc24593… # v1.2). It has no circular-dependency constraint, and its code at that SHA is byte-identical to what v1.2 will tag.
  • The reusable-workflow ref keeps the tag, since that one genuinely cannot self-pin, but the immutable form is no longer buried in the versioning section. The quick-start now carries the git ls-remote one-liner to resolve the tag and the SHA form to paste, immediately under the example.

The versioning section now says to pin by SHA rather than recommending the tag, and states why: tags here have moved.

[L1] v1.2 does not resolve yet — documented rather than papered over. The versioning section now records that the tag must be cut immediately after this merges, and that the documented tag does not resolve in between. That ordering is forced: the tag has to name this PR's merge commit, because bc24593's own triage.yml still carries the old pins, so tagging earlier would ship the broken pipeline.

[L2] extra_env heredoc delimiter — not taken here. Agreed it is worth guarding, and thank you for noting it is untouched by this PR. It is unrelated to the pin bump, and this PR is deliberately confined to the four pin lines plus docs so the release diff stays auditable. Happy to take it as a separate PR if you want it before v1.2 rather than after.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-paths and target_file are project-relative, and the upload-report snippet in the new section uses a SHA pin (bc24593).
  • working-directory validation (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.

@feng-shiplight
feng-shiplight merged commit 5f0d835 into main Aug 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant