feat: add Trivy SCA autofix reusable workflow - #123
Conversation
Lock the shared reusable workflow, pluggable SCA engines, and consumer contract before implementation. Co-authored-by: Cursor <cursoragent@cursor.com>
Task-by-task plan for the prompt composite, three SCA engines, reusable workflow, and consumer docs. Co-authored-by: Cursor <cursoragent@cursor.com>
The reusable workflow cannot read sibling files from shared-actions; the playbook is copied via github.action_path. Co-authored-by: Cursor <cursoragent@cursor.com>
No shell, no GitHub token; Write is scoped to JS manifests/lockfiles and the result file. The caller still owns git and the PR. Co-authored-by: Cursor <cursoragent@cursor.com>
Same claude-code-action SHA as review, no Bash, no github_token. Write is unscoped at the CLI; the workflow allowlist is the gate. Co-authored-by: Cursor <cursoragent@cursor.com>
workspace-write sandbox so package.json is writable; Bash still denied. Keep the Ubuntu 24.04 bwrap userns workaround from the review engine. Co-authored-by: Cursor <cursoragent@cursor.com>
Trigger on a consumer Trivy failure, re-scan master, dispatch a pluggable engine, and open one PAT-authored PR. Slack on every outcome. Co-authored-by: Cursor <cursoragent@cursor.com>
Caller YAML, engine switch, PAT requirement, and the manual pilot plan. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Dependency ReviewThe following issues were found:
License Issues.github/workflows/trivy-sca-autofix.yml
OpenSSF Scorecard
Scanned Manifest Files.github/workflows/trivy-sca-autofix.yml
|
|
Claude finished @sparsh-deriv's task in 9m 0s —— View job I'll analyze this and get back to you. |
Engine strip of AGENTS.md/CLAUDE.md was failing the allowlist on typical consumer repos. Grok Write/Edit now follow the output directory; checkout no longer uses the write PAT. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the Kimi review (this repo does not run a Grok PR-review job — only Kimi + Claude). Done
Left as-is
|
|
Claude finished @sparsh-deriv's task in 1m 27s —— View job I'll analyze this and get back to you. |
claude-code-action at the pinned SHA does not require github_token; cache-dir is a real Trivy input. Spec/plan now defer to the workflow README instead of stale YAML snapshots. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up on the remaining Kimi (3) and Claude (2) items: Verified in-repo (no behavior change for
Not changing
|
|
Claude finished @sparsh-deriv's task in 3m 28s —— View job I'll analyze this and get back to you. |
Consumer docs stay in TRIVY_SCA_AUTOFIX_README.md; the design/plan files are not part of the shipped action. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @sparsh-deriv's task in 1m 33s —— View job I'll analyze this and get back to you. |
ako-deriv
left a comment
ako-deriv
left a comment
There was a problem hiding this comment.
Review
I read the full diff, ran tests/trivy-sca-autofix-contract.sh against this branch (99/99 pass), and compared the design against dependonme-bot.yml.
The security posture is genuinely strong. Shell-less and token-less engines, env -i PAT isolation, hook bypass, the exfil guard, the allowlist gate, persist-credentials: false — the earlier Kimi/Claude rounds clearly did their job on that surface. I found nothing to add there.
The gap is that the happy path has never executed, and I do not think it can currently succeed for most repos. My findings are almost entirely functional rather than security:
- The two Trivy scans measure different surfaces —
node_modulesexists for the second one and is not skipped, so theAFTER -eq 0gate may be unreachable and no PR ever opens. pnpm installcannot succeed in CI — pnpm defaults--frozen-lockfileto true whenCIis set, and the engine has just changedpackage.json. The pnpm path is dead.- The zero-findings gate contradicts
prompt.md— the playbook tells the engine to leave unfixable CVEs unfixed and record them, but that PR body can never be created. Repos with one unfixable HIGH get no benefit plus a "could not clear gate" Slack message on every red run. - Monorepos get an inconsistent commit or a permanently red gate — one
install_dir(picked nondeterministically viahead -1) versusWrite(**/package.json)permission for the engines. The README oversells this as nested-lockfile support.
Plus: a successful PR can report failure to Slack (gh pr view null), the Edit-less kimi/anthropic engines are permitted to whole-file Write multi-MB lockfiles for no benefit, and about a third of the contract test's 99 checks assert that a comment string exists rather than that anything behaves — which is why they have not caught any of the above.
Details inline. Requesting changes on 1-4; the rest are cheap.
Happy to take the fixes for #2, #5 and the minors as a patch on top of this branch if that is faster than round-tripping.
The after-scan included node_modules, pnpm froze the lockfile under CI=true, and gh pr view ran on a null id after a successful create. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @sparsh-deriv's task in 6m 24s —— View job I'll analyze this and get back to you. |
ako-deriv
left a comment
ako-deriv
left a comment
There was a problem hiding this comment.
Follow-up on d19b17c3 — blockers cleared
Dismissed my CHANGES_REQUESTED; the four blockers are genuinely fixed and I verified each against the new head. Details in the dismissal note. Both suites pass on d19b17c3: contract 121, logic 30.
Nice touches beyond what I raised — the node_modules problem got fixed twice (skip-dirs merge and a post-install sweep), the reduction gate threads both counts through the commit message, PR title, body and Slack, and the kimi/grok prompt heredocs moved to proper env expansion rather than a cosmetic patch.
Two non-blocking follow-ups, neither a merge blocker:
-
The lockfile rule is enforced on one of three engines. Permissions were tightened everywhere, but kimi is the only engine with a catch-all
deny Write; grok's rules table has no write deny at all, and anthropic'sWriteis unscoped by design with only the prompt asking it to behave. Since the restore loop runs afternpm install, a whole-file lockfileWritestill reaches the installer. A four-linegit checkout --step before the refresh makes it a workflow invariant for all three at once — inline. -
The new logic test asserts against its own copy of the logic. Right target list, real behavioural coverage, but the seven helpers are hand-copied from the workflow rather than shared with it, so the workflow can drift green. Extracting them to a
sourced lib would fix that — inline.
Plus two nits (skip-dirs recursion, --base on the edit path).
On the one thing no amount of review can settle: the test-plan checkboxes in the description are still unchecked, and the happy path has never executed end-to-end. Everything I have flagged across both rounds came from reading, not running. I would want case 2 from the manual plan (red Trivy, dirty master, partial reduction -> PR opens with both counts in the title) to have actually happened on a pilot repo before this goes live for consumers pinned at @master.
Happy to take either follow-up as a patch if that is faster.
Grok and Anthropic cannot deny Write the way Kimi does, so a truncated lockfile still reached npm/pnpm. Helpers ship next to the playbook composite because a reusable workflow cannot source this repo's tree. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @sparsh-deriv's task in 3m 14s —— View job I'll analyze this and get back to you. |
ako-deriv
left a comment
ako-deriv
left a comment
There was a problem hiding this comment.
Third pass on f60799ae — both follow-ups fixed
Verified on the new head: contract 124 and logic 36, all green.
Both items from the last review are done, and both improved on what I proposed:
- Lockfile restore —
discard_engine_lockfile_edits()distinguishes tracked lockfiles (git checkout --) from ones the engine created (rm -f, vials-files --error-unmatch). My suggestedgit checkout -- '*yarn.lock'glob would have missed the created-file case entirely. It also has a real integration test against a temp git repo covering all three outcomes — tracked lockfile restored, untracked removed,package.jsonedit preserved. - Shared lib — my suggestion was wrong.
.github/scripts/sca-lib.shcould never work, because a reusable workflow checks out the caller, not shared-actions. Shipping viatrivy_sca_autofix_prompt/lib.sh→github.action_path→/tmp/sca-lib.shis the only available mechanism. Every duplicated helper is now gone and the logic tests source the shipped file, so drift is structurally impossible.detect_ingot deduplicated too, which I had missed.
Nits also picked up: node_modules,**/node_modules, and --base "$BASE_BRANCH" on the edit path.
Four comments, none blocking. The first is a correction of my own: the README's blast-radius warning was deleted in d19b17c3 because I criticised the contract test that grepped for it — I was flagging the test, not the documentation, and that warning is now considerably more warranted than when it was written. lib.sh moved the allowlist, loop-skip and reduction gate behind a mutable @master pin. Restoring that sentence, and updating the Dogfood note to name trivy_sca_autofix_prompt as the ref that must be repointed first, is the substantive ask.
The other two are a lost diagnostic message and four lines of dead code.
Unchanged from my last two rounds, and the only thing I'd still hold out for: the test-plan boxes in the description are all unchecked. Three review passes have hardened this thoroughly on paper, but nothing in it has executed. The refactor moved the security-critical logic behind @master, which makes a real pilot run more valuable rather than less — case 2 (red Trivy, dirty master, partial reduction → PR with both counts in the title) is the one that would actually validate the design.
| SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} | ||
| ``` | ||
|
|
||
| Every consumer in this org pins `@master`, so that is the documented policy — changes are live on merge. The SCA engines and `send_slack_notification` run with the LLM key in scope, and the same job later uses `AUTOFIX_GITHUB_TOKEN` for push/PR creation. |
There was a problem hiding this comment.
This paragraph used to end with a sentence that should come back — and its removal is my fault.
In d19b17c3 the clause "treat edits to those composites like edits to this workflow" was deleted from here. It went because I flagged the contract test's grep -q "treat edits to those composites" as prose-grep theater. My criticism was of the test, not the documentation — removing the grep was right, removing the sentence was not.
And it matters much more now than when it was written. As of f60799ae, trivy_sca_autofix_prompt@master no longer just holds prompt text — it ships lib.sh, which holds the allowlist regex, loop-skip, the reduction gate, skip-dirs resolution, and lockfile discarding. It is copied in at step 3 and every later step sources it.
So a bad edit to lib.sh now silently changes the security gate for every consumer pinned at @master, and breaks them before checkout rather than degrading. Suggest restoring the warning and naming the file:
The SCA engines and
send_slack_notificationrun with the LLM key in scope, and the same job later usesAUTOFIX_GITHUB_TOKENfor push/PR creation.trivy_sca_autofix_prompt/lib.shcarries the allowlist, loop-skip and reduction-gate logic for every consumer — treat edits to it like edits to this workflow.
There was a problem hiding this comment.
Restored. The usage paragraph now names trivy_sca_autofix_prompt/lib.sh and says to treat edits to it like edits to this workflow.
|
|
||
| ## Dogfood note | ||
|
|
||
| Composite action refs inside this workflow resolve `@master` even when the reusable workflow is called from a feature branch. To pilot a new engine before merge, temporarily point the dispatch `uses:` steps in `trivy-sca-autofix.yml` at `@<branch>` and revert before merging to `master`. |
There was a problem hiding this comment.
This note is now incomplete in a way that will bite whoever runs the pilot.
It says to point "the dispatch uses: steps" at @<branch> — i.e. the engine composites. But as of f60799ae the critical ref is trivy_sca_autofix_prompt@master (L100 of the workflow): @master has no lib.sh, so Copy autofix playbook fails its own [[ -s "$LIB" ]] guard and the job dies at Loop skip before reaching any engine.
Anyone following this note verbatim will hit that and read it as a broken workflow rather than a stale pin. Worth saying explicitly that trivy_sca_autofix_prompt must be repointed too, first, because the lib is load-bearing for control flow now.
Credit where due: the [[ -s "$LIB" ]] guard means this fails loudly with a clear message instead of source-ing nothing and silently skipping the loop-skip check. That was the right call.
There was a problem hiding this comment.
Dogfood note now says to retarget trivy_sca_autofix_prompt first (lib.sh is missing on @master), then the engine uses: pins, and revert all of them before merge.
| AFTER="$(jq '[.Results[]? | .Vulnerabilities[]?] | length' /tmp/trivy-after.json)" | ||
| echo "count=$AFTER" >> "$GITHUB_OUTPUT" | ||
| echo "📊 Trivy findings after fix: $AFTER (was $BEFORE)" | ||
| if ! should_open_pr "$BEFORE" "$AFTER"; then |
There was a problem hiding this comment.
Small diagnostic regression from the lib extraction.
should_open_pr folds the =~ ^[0-9]+$ validation into the same boolean as the reduction comparison, so the distinct "could not fix (invalid finding count)" Slack message that d19b17c3 added is gone. A malformed jq result now reports "could not clear gate (<garbage> remain, was 12)" — presenting an unparseable value as if it were a count.
Two ways out, either is fine:
- keep the numeric guard in this step before calling
should_open_pr, or - have
should_open_prreturn2for unparseable vs1for not-reduced, and branch on it here.
Not worth a round trip on its own, but worth folding into the next commit — "gate not reduced" and "I could not read the scan output" are different operational events and only one of them means the engine did its job.
There was a problem hiding this comment.
Numeric guard is back in the after-scan step before should_open_pr. Unparseable jq output posts "could not fix (invalid finding count)"; a real non-reduction still posts "could not clear gate (N remain, was M)".
| DEFAULT_MAX_CONTEXT="500000" | ||
| DEFAULT_CLI_VERSION="1.0.5" | ||
| ;; | ||
| *) |
There was a problem hiding this comment.
Dead code: normalize_engine at L130 already rejects anything outside kimi|anthropic|grok and exits, so this *) arm is unreachable. Safe to drop — the error message and Slack string are duplicated in the || block above it.
There was a problem hiding this comment.
Dropped the *) arm. Unknown engines still fail in normalize_engine with the same Slack string.
| #!/usr/bin/env bash | ||
| # Shared SCA autofix helpers. Copied to /tmp/sca-lib.sh via github.action_path | ||
| # (reusable workflows check out the caller, so this cannot live at a path in | ||
| # the consumer repo). Sourced by trivy-sca-autofix.yml and the logic tests. |
There was a problem hiding this comment.
Worth recording here as well as in the README, since this file is where someone will land when they go to edit it:
this lib is delivered to consumers through trivy_sca_autofix_prompt@master, so every function here is live for every consumer the moment it merges — no pinning, no staging. It now carries the allowlist regex, the loop-skip condition, and the reduction gate, i.e. the three things that decide whether a PR gets opened and what lands in it.
A line in this header saying so would put the warning in front of the person changing the file, not just the person reading the docs. Something like:
# WARNING: shipped to consumers at @master via trivy_sca_autofix_prompt.
# Changes here are live for every consumer on merge and alter the security
# gate (allowlist, loop-skip, reduction gate). Treat as workflow code.Separately — the mechanism itself is the right one, and better than the .github/scripts/ path I suggested, which could never have worked: a reusable workflow checks out the caller, so it cannot source this repo's tree. Delivering through github.action_path in the composite that was already copying the playbook is the only route available. Good call.
There was a problem hiding this comment.
Added the @master blast-radius warning at the top of lib.sh.
lib.sh is live at @master for every consumer, so the README, dogfood note, and lib header say so. The after-scan reports unparseable jq output separately from a gate that did not reduce. Dead resolve *) arm removed; logic tests cover non-numeric finding counts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Model: 🤖 Kimi PR Review Complete🔄 Follow-up Review Summary3 of 5 issues from the previous review have been resolved. The two outstanding Low items from the last round were not addressed: the seven drift-guard contract checks were not restored (the incremental diff never touches Recommendation: APPROVE 🔴 Critical Issues (BLOCK MERGE)None. 🟠 High Priority IssuesNone. 🟡 Medium Priority IssuesNone. 🟢 Low Priority Issues🟢 1. Contract checks locking the two review-mandated verification comments were removed while the comments still ship —
|
| Severity | File | Lines |
|---|---|---|
| LOW | tests/trivy-sca-autofix-contract.sh |
20-28, 61-75, 94-154, 156-166 |
❌ Problematic Code (still absent; the incremental diff does not touch this file):
# removed two rounds ago, still missing from the ANTH / WF sections:
- check 'grep -q "github_token is optional" "$ANTH"' "anthropic SCA records github_token SHA verification"
- check 'grep -q "Real input on aquasecurity/trivy-action" "$WF"' "trivy cache-dir documented as a real input at the pin"
# removed last round, still missing while the guarded text ships:
- check 'grep -q "caller regenerates lockfiles" "$PROMPT"' "prompt says caller regenerates lockfiles"
- check 'grep -q "AFTER < BEFORE" "$DOC"' "docs describe the reduction gate"
- check 'grep -q "queued" "$DOC"' "docs mention queued concurrency drops skip Slack"
- check 'grep -q "force-with-lease" "$DOC"' "docs record force without lease"
- check 'grep -q "not a full monorepo product" "$DOC"' "docs do not oversell monorepo support"📋 Issue: Unchanged since the last review. The two original guards lock in the source-verification comments that resolved the earlier github_token/cache-dir issues; those comments still ship (.github/actions/ai_sca_engine_anthropic/action.yml:90-95, .github/workflows/trivy-sca-autofix.yml:253-255,485-487). The prompt still says "The caller regenerates lockfiles after you finish" (prompt.md:21) and the README still contains AFTER < BEFORE (:79), queued (:81), --force-with-lease (:78), and not a full monorepo product (:71). Every one of these greps would still pass today — the guarded content was not removed, only the tripwires were.
✅ Fix (re-add the checks — all pass against the current files):
# in the PROMPT section:
check 'grep -q "caller regenerates lockfiles" "$PROMPT"' "prompt says caller regenerates lockfiles"
# in the ANTH section:
check 'grep -q "github_token is optional" "$ANTH"' "anthropic SCA records github_token SHA verification"
# in the WF section:
check 'grep -q "Real input on aquasecurity/trivy-action" "$WF"' "trivy cache-dir documented as a real input at the pin"
# in the DOC section:
check 'grep -q "AFTER < BEFORE" "$DOC"' "docs describe the reduction gate"
check 'grep -q "queued" "$DOC"' "docs mention queued concurrency drops skip Slack"
check 'grep -q "force-with-lease" "$DOC"' "docs record force without lease"
check 'grep -q "not a full monorepo product" "$DOC"' "docs do not oversell monorepo support"💡 Explanation: Zero-cost restoration of the lock-in the earlier fixes relied on; alternatively, state in the PR why these drift guards were intentionally dropped.
🟢 2. PR description test plan still asserts the old clean-gate behavior that the reduction gate replaced — .github/workflows/trivy-sca-autofix.yml:499-508
Details
| Severity | File | Lines |
|---|---|---|
| LOW | .github/workflows/trivy-sca-autofix.yml (behavior) vs. PR description (stale text) |
499-508 |
❌ Problematic Code:
if ! should_open_pr "$BEFORE" "$AFTER"; then
echo "❌ Gate not reduced — not opening a PR"(should_open_pr in .github/actions/trivy_sca_autofix_prompt/lib.sh:53-58 requires [[ "$after" -lt "$before" ]])
versus the PR description's test plan, still present verbatim: "Second Trivy still red → no PR, Slack 'could not clear gate'".
📋 Issue: Unchanged since the last review. The gate is "after-scan must be lower than before", so a still-red-but-reduced scan opens a PR (title/body carry both counts). The shipped README documents this correctly (TRIVY_SCA_AUTOFIX_README.md:79), but the PR description's test plan still describes the old all-or-nothing gate and contradicts the diff. This is a description edit, not a code change — no commit can fix it.
✅ Fix (edit the PR description test-plan line):
- Second Trivy still red → no PR, Slack "could not clear gate"
+ Second Trivy not reduced (AFTER >= BEFORE) → no PR, Slack "could not clear gate";
+ reduced-but-still-red → PR opened with before/after counts in title and body
💡 Explanation: Brings the stated intent in line with the shipped gate at trivy-sca-autofix.yml:504-508 and the README's documented behavior; code itself needs no change.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 0 | — |
| 🟡 Medium | 0 | — |
| 🟢 Low | 2 | Removed drift-guard contract checks (unchanged this round); stale PR-description test plan |
Resolved since previous review: 3 of 5 tracked issues remain fixed (no re-openings). Still present: 2 Low — both carried over, neither addressed in the incremental commits.
Recommendations
- Re-add the removed contract checks (Low issue 1 — seven greps, all passing against current files) or note in the PR why the drift guards were intentionally dropped.
- Update the PR description's test-plan line for the reduction gate (Low issue 2) so pilot expectations match the code.
- After merge, run the dogfood already in the test plan (
engine: anthropic/engine: grokvia the@masterrefs) — still the last unexecuted assumption; it also exercises the unconditional playbook/lib copy anddiscard_engine_lockfile_editsbefore the trusted install. - Carried over, still optional: the engine comment "Job token is contents:read" (
.github/actions/ai_sca_engine_anthropic/action.yml:95) understates the ambient token's scope — the job also grantspull-requests: write(.github/workflows/trivy-sca-autofix.yml:82-84). The documented mitigations (no Bash, GitHub MCP denied,classify_inline_comments: false) are unaffected.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
|
Claude finished @sparsh-deriv's task in 1m 45s —— View job I'll analyze this and get back to you. |
🤖 Claude PR Review CompleteModel: SummaryAll 1 issue from the previous review has been resolved. The base-retargeting fix carries forward unchanged in this round. This round's diff is small and purely additive/cleanup: a blast-radius warning comment at the top of
No regressions found in this round's changes. Recommendation: APPROVE Summary Table
Recommendations
Auto Fix Claude Reviews
|
ako-deriv
left a comment
ako-deriv
left a comment
There was a problem hiding this comment.
Drive-by review from the deriv-api-v2 side — we run two workflows that open PRs the same way (docs-content-sync.yml, product-updates-sync.yml), so this is mostly "we hit these two, here's what they cost us". The shape here already matches ours (commit → push → gh pr list --head → create-or-edit, --body-file, PR number from the URL), and the hardening is ahead of ours — env -i with pinned binaries, core.hooksPath=/dev/null, and grepping the body for the key and PAT before posting. We'll be stealing those back.
Two substantive suggestions, one blocking-ish:
-
Mint a GitHub App token instead of requiring a PAT (non-blocking, but worth it before consumers onboard). A reusable workflow runs in the caller's context, so
actions/create-github-app-tokenwith noowner/repositoriesscopes itself to the calling repo automatically. That gets you a repo-scoped hour-lived credential and PRs authored by the App instead of one person's account — and it still triggers CI, which is the whole reason the PAT is there. KeepingAUTOFIX_GITHUB_TOKENas an optional fallback also keepstests/trivy-sca-autofix-contract.sh:109and:161green, which a straight swap would not. -
The
--forcepush will eat a reviewer's commit (this is the one I'd fix before merge). The README is upfront about it, butchore/trivy-sca-autofixis a dependency-bump PR humans are expected to amend — pin a transitive version, add an override, fix a peer-dep conflict. A second Trivy failure then silently discards that.Heads-up on the obvious fix:
--force-with-leasedoes not help here. We tried exactly that and verified it on a throwaway repo — the lease only catches a ref moving after the local remote-tracking ref was set, and you have to fetch first for the lease to have anything to compare against, which makes it pass and clobber anyway. Fetch-then-lease discarded a reviewer's commit exactly as plain--forcedid. We refuse on authorship instead (product-updates-sync.yml:390has the writeup).
Neither changes the architecture — the engine boundary, the allowlist, and the reduction gate all look right to me.
| AUTOFIX_GITHUB_TOKEN: | ||
| description: "PAT with contents:write and pull-requests:write that triggers workflows" | ||
| required: true |
There was a problem hiding this comment.
Make the PAT an optional fallback and accept App credentials, so a consumer can onboard without storing a long-lived personal credential.
| AUTOFIX_GITHUB_TOKEN: | |
| description: "PAT with contents:write and pull-requests:write that triggers workflows" | |
| required: true | |
| AUTOFIX_GITHUB_TOKEN: | |
| description: "Fallback PAT with contents:write and pull-requests:write. Prefer GHAPP_* below." | |
| required: false | |
| GHAPP_CLIENT_ID: | |
| description: "GitHub App client id. Mints a repo-scoped PR token that triggers workflows." | |
| required: false | |
| GHAPP_PRIVATE_KEY: | |
| description: "GitHub App private key." | |
| required: false |
The tradeoff to document: the App has to be installed on each consumer repo. For deriv-com that's a one-time org-level install and then no consumer stores a credential at all; a PAT works everywhere immediately but dies with its owner's offboarding.
Declaring all three as required: false and validating at runtime is the only option — a reusable workflow can't express "one of these two".
| - name: Commit and open or update PR | ||
| if: steps.skip.outputs.skip != 'true' && steps.findings.outputs.clean != 'true' | ||
| env: | ||
| GH_TOKEN: ${{ secrets.AUTOFIX_GITHUB_TOKEN }} |
There was a problem hiding this comment.
Resolve the token, preferring the App:
| GH_TOKEN: ${{ secrets.AUTOFIX_GITHUB_TOKEN }} | |
| GH_TOKEN: ${{ steps.app_token.outputs.token || secrets.AUTOFIX_GITHUB_TOKEN }} |
with a new step just above Commit and open or update PR:
- name: Mint GitHub App token for PR
id: app_token
if: steps.skip.outputs.skip != 'true' && steps.findings.outputs.clean != 'true' && secrets.GHAPP_CLIENT_ID != ''
uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0
with:
app-id: ${{ secrets.GHAPP_CLIENT_ID }}
private-key: ${{ secrets.GHAPP_PRIVATE_KEY }}
# no owner/repositories: defaults to the calling repoTwo follow-ons:
- Add an explicit guard at the top of the run block, before anything touches git —
[ -n "$GH_TOKEN" ] || { echo "::error::set GHAPP_CLIENT_ID/GHAPP_PRIVATE_KEY or AUTOFIX_GITHUB_TOKEN"; exit 1; }. With neither secret set you otherwise get aghauth failure ~60 lines later, after the commit has already been made. - The leak checks at 591 and 381 should cover whichever token was actually used, not just
AUTOFIX_GITHUB_TOKEN— an App token in the body is the same incident, and it's the one that'll be in play for most consumers. run_pat(557) becomes a misnomer once it may carry an App token;run_ghreads better. Purely cosmetic.
| TERM=dumb \ | ||
| "$@" | ||
| } | ||
| run_pat /usr/bin/git -c core.hooksPath=/dev/null push --force --no-verify origin "HEAD:refs/heads/${AUTOFIX_BRANCH}" |
There was a problem hiding this comment.
This overwrites any human commit on the singleton branch. Refuse on authorship rather than force-pushing blind:
| run_pat /usr/bin/git -c core.hooksPath=/dev/null push --force --no-verify origin "HEAD:refs/heads/${AUTOFIX_BRANCH}" | |
| BOT_EMAIL="github-actions[bot]@users.noreply.github.com" | |
| if run_pat /usr/bin/git fetch origin "+refs/heads/${AUTOFIX_BRANCH}:refs/remotes/origin/${AUTOFIX_BRANCH}" 2>/dev/null; then | |
| FOREIGN="$(/usr/bin/git log --format='%ae' "origin/${BASE_BRANCH}..origin/${AUTOFIX_BRANCH}" | grep -vFx "$BOT_EMAIL" | head -1 || true)" | |
| if [[ -n "$FOREIGN" ]]; then | |
| echo "❌ ${AUTOFIX_BRANCH} carries commits from ${FOREIGN} — refusing to force-push over them" | |
| printf '%s\n' "could not fix (autofix branch has human commits from ${FOREIGN})" > "$SLACK_MESSAGE_FILE" | |
| exit 1 | |
| fi | |
| echo "✅ ${AUTOFIX_BRANCH} is bot-only — safe to replace" | |
| fi | |
| run_pat /usr/bin/git -c core.hooksPath=/dev/null push --force --no-verify origin "HEAD:refs/heads/${AUTOFIX_BRANCH}" |
Because every outcome already posts to Slack, the refusal is visible rather than a silent skip, and the unblock is one human action — close the PR or delete the branch.
Worth stating plainly since it's counterintuitive: swapping in --force-with-lease here would not fix this. The lease only catches a ref that moved after the local remote-tracking ref was set; the fetch the lease needs in order to have a comparison point is what makes it pass. We verified on a throwaway repo that fetch-then-lease discarded a reviewer's commit exactly as plain --force did. The --force above stays correct once the authorship check is what's actually guarding it.
One note if you take this: the fetch has to run under run_pat (as written) so it uses the same credential helper as the push.
| | Secret | Description | Required | | ||
| |--------|-------------|----------| | ||
| | `LLM_API_KEY` | LiteLLM virtual key for the selected engine | ✅ | | ||
| | `AUTOFIX_GITHUB_TOKEN` | GitHub PAT with `contents:write` and `pull-requests:write` that **triggers workflows** on the new PR — `GITHUB_TOKEN` would not. The job token only needs `contents: read`; push and PR creation use this PAT via `GH_TOKEN`. | ✅ | |
There was a problem hiding this comment.
If the App-token suggestion lands, this table wants a row pair rather than one required PAT:
| | `AUTOFIX_GITHUB_TOKEN` | GitHub PAT with `contents:write` and `pull-requests:write` that **triggers workflows** on the new PR — `GITHUB_TOKEN` would not. The job token only needs `contents: read`; push and PR creation use this PAT via `GH_TOKEN`. | ✅ | | |
| | `GHAPP_CLIENT_ID` / `GHAPP_PRIVATE_KEY` | GitHub App credentials. Mints a repo-scoped, hour-lived token that **triggers workflows** on the new PR — `GITHUB_TOKEN` would not. Requires the App to be installed on the consumer repo. | ⬅️ one of | | |
| | `AUTOFIX_GITHUB_TOKEN` | Fallback PAT with `contents:write` and `pull-requests:write`, same trigger-workflows requirement. Long-lived and tied to one account — prefer the App. The job token only needs `contents: read`. | ⬅️ one of | |
The "PAT isolation" bullet at line 75 reads fine either way — the env -i isolation applies to whichever token is in scope — but "PAT" in the heading is worth generalising to "Token isolation".
| - **PAT isolation** — Checkout uses the job `GITHUB_TOKEN` with `persist-credentials: false` (no write PAT on fetch). Push and `gh` run under `env -i` with `/usr/bin/git` / `/usr/bin/gh` and a local `gh` credential helper. | ||
| - **Hook bypass** — Commit and push use `core.hooksPath=/dev/null` and `--no-verify` so consumer `.git/hooks` cannot run with secrets in env. | ||
| - **Labels best-effort** — PRs are created without `--label`; `trivy-autofix` and `security` are added afterward with `|| true` so missing repo labels do not fail after a force-push. | ||
| - **Force-push** — The singleton branch is pushed with `--force` (no `--force-with-lease`). This job owns that branch; a human commit on `chore/trivy-sca-autofix` will be overwritten. |
There was a problem hiding this comment.
| - **Force-push** — The singleton branch is pushed with `--force` (no `--force-with-lease`). This job owns that branch; a human commit on `chore/trivy-sca-autofix` will be overwritten. | |
| - **Force-push** — The singleton branch is force-pushed, but only after checking that every commit on it (vs `base_branch`) was authored by `github-actions[bot]`. A human commit on `chore/trivy-sca-autofix` makes the job refuse and report it to Slack; close the PR or delete the branch to let the next run regenerate the fix. `--force-with-lease` is deliberately not used — it does not protect against this (the fetch it needs makes the lease pass), so the authorship check is the real guard. |
Reason for spelling out the --force-with-lease part in the docs and not just a code comment: it's the first thing a reviewer will suggest, and it looks like it works.
2 participants
Summary
base_branch(defaultmaster), re-scan, bump JavaScript deps via a pluggable LLM engine (kimi|anthropic|grok), and open one PR intomaster. Humans merge; Slack on every outcome.ai_sca_engine_*) are separate from PR-review engines. Engines cannot commit/push; the workflow owns allowlist,--ignore-scripts, second Trivy gate, and PAT-based PR creation (AUTOFIX_GITHUB_TOKENso CI runs)./tmp,persist-credentials: false, hook-less commit/push, labels added after create, jobcontents: read, andgit/ghunderenv -iwith pinned binaries.Test plan
bash tests/trivy-sca-autofix-contract.sh(also runs fromlint-actions.ymlon this PR)actionlint/ lint-actions)needs: trivy+if: failure()caller from.github/workflows/TRIVY_SCA_AUTOFIX_README.mdmasteris clean → Slack “nothing to do”, no PRmasteris actually vulnerable → one PR onchore/trivy-sca-autofixwith only allowlisted lockfile/manifest changes@master(they will not see this branch until merge)Made with Cursor