Repository navigation
Suggest tags belonging to the action being used - #2247
Conversation
|
A bit of a comment - It took me 4 times to create the PR - besides the first time (3 times from the template) - all the previous attemps were auto-closed as not following the template (but ... I did). Yes. It was generated with LLM (Disclosed) - but... for whatever reason it was auto closed few times before.. The 5th time succeeded. |
|
From looking at the closed PRs, it looks like none of them have the hidden marker that the bot checks for. Did you fill out the template on the web, or through some other mechanism? If you copy-pasted it that might explain it. (This is a line I'm still figuring out how to draw -- I don't mind people submitting LLM-assisted PRs, but I do want them to submit the PR itself manually and to fully affirm that they're read the template. The bot is my best attempt at that ATM, but it's definitely not perfect.) |
woodruffw
left a comment
woodruffw
left a comment
There was a problem hiding this comment.
Thanks @potiuk! The basic approach here seems reasonable to me, although I think it's unfortunate that repos use this pattern at all 😅 -- FWICT there's going to be a lot of edge cases with this, and I think it might also not interact super well with GitHub's upcoming actions lockfile feature.
One request: could you add a real-world (online) test that exercises this path? I think something under ref-version-mismatch would be fine.
|
(NB: I'm probably going to queue this for 1.30, since I'm planning on sending 1.29 out today.) |
59b75bd to
864f683
Compare
864f683 to
63aa7cc
Compare
A repository that ships several actions from subdirectories tags each one under its own prefix, so a single commit can carry save/v1.0.0, restore/v1.0.0 and allowlist-check/v1.0.0 at once. Choosing purely by length hands every action in such a repository whichever sibling happens to have the longest name, and offers that as an auto-fix, so accepting it writes another action's version into the workflow. Prefer a tag that names the action being used, keep the previous choice for everything else, and fall back to it when nothing names the action. Picking that tag needs to know which action within the repository is being used, which means `RepoRef` has to carry the subpath. It gains a `subpath()` accessor alongside `git_ref()` and `slug()`: a `uses:` clause answers from the `owner/repo/subpath@ref` it already parses, and a Git URL reference answers `None`, since a URL names a repository and nothing within it. `longest_tag_for_commit` then takes the subpath beside the slug rather than reconstructing owner and repo from it.
63aa7cc to
7e61883
Compare
2 participants
A repository that ships several actions from subdirectories tags each one under its own prefix, so a single commit can carry save/v1.0.0, restore/v1.0.0 and allowlist-check/v1.0.0 at once. Choosing purely by length hands every action in such a repository whichever sibling happens to have the longest name, and offers that as an auto-fix, so accepting it writes another action's version into the workflow.
Prefer a tag that names the action being used, keep the previous choice for everything else, and fall back to it when nothing names the action.
Pre-submission checks
Please check these boxes:
Mandatory: This PR corresponds to an issue (if not, please create
one first).
Having read the AI policy, I hereby disclose the use of an LLM or other
AI coding assistant in the creation of this PR. PRs will not be rejected
for using AI tools, but will be rejected for undisclosed use or
use that violates the policy.
Disclosure: Generated with help of LLM.
If a checkbox is not applicable, you can leave it unchecked.
Summary
Closes #2243
In a repository that ships several actions from subdirectories and tags each under its own prefix, when Zizmor suggests auto-fix for actions, the longest_tag_for_commit picked the longest tag name among every tag at a commit, without knowing which action inside the repository was being used. , one commit carries several tags, so every action there was told about whichever sibling had the longest name — and that was offered as an auto-fix.
Concretely, for apache/infrastructure-actions@61dcea11…, whose commit also carries restore/* and allowlist-check/* tags:
is pointed to by tag allowlist-check/v1.0.0
where the answer should be save/v1.0.0.
With this fix - the choice prefers a tag that names the action being used:
with a subpath, tags prefixed by the full subpath (stash/save/) or its leaf (save/);
without one, tags containing no /, so a root action does not use subdirectory action's tag;
otherwise the previous longest-tag-at-this-commit behaviour, unchanged, so repositories that do not use prefixed tags behave exactly as before — including the fallback where nothing names the action and a sibling is still better than no suggestion.
Validation was already correct and is untouched: commit_for_ref matches tag names exactly and peels annotated tags, so a correct # save/v1.0.0 comment resolves to the pinned commit and produces no finding. Only the suggestion side was wrong.
Test Plan
Added unit tests covering the pure function that resolves best match.
Note for review: 7 tests in known_vulnerable_actions fail on my machine both with and without this change (108 passed / 7 failed on a clean tree, 109 / 7 with it — the extra pass being the new test), so I have assumed they are environmental rather than related.