Skip to content

chore(ci): file prod container scans under main - #1434

Merged
jfrench9 merged 1 commit into
mainfrom
chore/container-scan-ref
Sep 18, 2026
Merged

jfrench9 merged 1 commit into
mainfrom
chore/container-scan-ref

Conversation

@jfrench9

Copy link
Copy Markdown
Member

Summary

Prod deploys from a version tag, so the Trivy container scans in build.yml were filed under refs/tags/v*. Code scanning only shows the default branch, so the container alerts on main never got updated by a normal release. They only updated on a one-off deploy from main.

That's why the soupsieve alerts are still open after v1.12.8 shipped the fix. v1.12.8's scan dropped both findings, but it was filed under refs/tags/v1.12.8, and the latest scan on main is still from the v1.12.7 one-off. This PR files prod's container scans under main, so those alerts describe what is deployed.

Changes

  • .github/workflows/build.yml
    • New optional input sarif_ref (default empty).
    • Both container-scan uploads (container-scan-api, container-scan-lambda) now pass ref: ${{ inputs.sarif_ref || github.ref }} and sha: ${{ github.sha }}. When sarif_ref is empty, this behaves as before.
  • .github/workflows/prod.yml: passes sarif_ref: refs/heads/main.
  • Staging: unchanged. It keeps its own ref, since feature branches are tested there and shouldn't update the alerts on main.
  • Dependency scan: unchanged. test-ci.yml already runs dependency-scan on main.

For review: upload-sarif describes sha as the latest commit on the ref. On a tag deploy it will be the release commit, which is on main but not the latest commit there.

Breaking Changes

None. CI only; the API and SDKs are unaffected.

Testing

  • actionlint on build.yml, prod.yml and staging.yml: clean.
  • Pre-commit hooks (ruff, format, basedpyright): passed.
  • just test-all: not run. The change is workflow-only.
  • Not yet checked end to end. That needs the next prod deploy from a tag. After it, confirm:
    • the container-scan-api and container-scan-lambda scans are filed under refs/heads/main (gh api repos/RoboFinSystems/robosystems/code-scanning/analyses);
    • the two open soupsieve alerts close.

Certification

  • I have the right to submit this work under the Apache 2.0 license, and do so. Where any part of it is owned by my employer, I have their permission.

🤖 Generated with Claude Code

Prod deploys from a version tag, so its Trivy container scans were filed
under refs/tags/v*, and code scanning only shows the default branch. The
alerts on main were left describing the last one-off main deploy rather
than what is running in prod. Prod now files the container-scan-api and
container-scan-lambda uploads under refs/heads/main; staging keeps its
own ref.
@github-actions github-actions Bot added the change:normal Normal change: automated review documented on the pull request before merge label Sep 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Sep 18, 2026

Copy link
Copy Markdown

Claude finished @jfrench9's task in 1m 51s —— View job


Claude finished @jfrench9's task


Review

The mechanics of the change are sound: sarif_ref defaults to "", both container-scan-api (.github/workflows/build.yml:293-299) and container-scan-lambda (.github/workflows/build.yml:422-429) upload steps symmetrically pass ref: ${{ inputs.sarif_ref || github.ref }} / sha: ${{ github.sha }}, the empty-string fallback behaves correctly in GH Actions expression evaluation, security-events: write permission is already present on both scan jobs, and staging.yml is confirmed untouched (it never sets sarif_ref, so it keeps filing under its own ref as intended).

One correctness question worth resolving before you rely on this, tied to the "for review" note in the PR body about sha:

The PR's premise is that a tag-triggered prod deploy's commit "is on main." That holds for an ordinary release, since release/x.y.z branches off main and (absent further changes) the tagged commit is the same SHA as a commit on main. But per .claude/commands/hotfix.md, hotfixes land via a twin PR that cherry-picks the main PR's commits onto the release branch — a cherry-pick produces a new commit object, so the release-branch twin's SHA is never identical to (or a descendant of) the commit that merged into main. Production also deploys directly from release/* branches for hotfixes (prod.yml:156), not just from tags.

Concretely: after any hotfix ships, the deployed github.sha for that release is not an ancestor of main. Uploading it with ref: refs/heads/main sends GitHub's code-scanning API a (ref, sha) pair where the sha isn't actually reachable from that ref's history — which risks the alert not displaying correctly (or at all) under the main branch view, defeating the goal of this PR for exactly the releases most likely to need a correct security posture on main (hotfixes are typically fixes, including possibly security fixes).

This doesn't need to block the PR — the testing section already flags this as unverified pending a real tag deploy — but it's worth explicitly checking against a post-hotfix release too, not just a normal one, since that's the scenario where the sha/ref mismatch would actually occur. If it turns out to be a problem, one option is dropping the sha override and just using ref: refs/heads/main alone (letting the action default sha to github.sha only when no override is needed), or resolving main's current tip SHA explicitly rather than reusing github.sha.

No other issues found — the diff is minimal, workflow-only, and matches its stated scope (staging/dependency-scan untouched, both container scans updated symmetrically).

@jfrench9
jfrench9 merged commit 8d6d036 into main Sep 18, 2026
8 checks passed
@jfrench9
jfrench9 deleted the chore/container-scan-ref branch September 18, 2026 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:normal Normal change: automated review documented on the pull request before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant