fix(ci): clear false-positive untrusted-checkout-toctou (CodeQL #35) and disclose fork+SHA at the integ approval gate

[nizar-lahlali]

Summary

Closes #896. Resolves CodeQL alert actions/untrusted-checkout-toctou/critical (#35) on .github/workflows/integ.yml.

The alert is a false positive: the checkout is already pinned to github.event.workflow_run.head_sha, an immutable commit SHA. The alert is an artifact of CodeQL's field-name heuristic — the job output named head_sha matches the mutable-ref regex /(head|branch|ref)/ on the head substring, so the SHA-pinned checkout is misclassified as a MutableRefCheckoutStep.

We clear it at the source (a classification fix), not by dismissing it.

Changes

1. Rename head_sha → commit_sha across all six sites (output declaration, both $GITHUB_OUTPUT writes, checkout ref, and the integ + report HEAD_SHA env). commit_sha matches the SHA heuristic /(sha|commit)/ only, so CodeQL classifies the step as SHACheckoutStep and alert fix(mem): fix various issues in mem #35 resolves to fixed. Pure rename — no value or control-flow change; the two human gates are untouched. A load-bearing comment at the output declaration warns against renaming it back.

2. Add a top-level run-name so the fork repo and head SHA appear in the run list and the deploy environment approval prompt. For workflow_run, GitHub attributes the run to the base repo, so the approver otherwise can't see which fork/commit they're authorizing without opening the resolve logs.

Acceptance criteria (issue #896)

• Alert fix(mem): fix various issues in mem #35 cleared by the rename, not dismissed (verify on next CodeQL scan of main after merge)
• integ run name shows <fork>@<sha> for workflow_run; readable for workflow_dispatch
• No change to the two human gates or to what code executes — git log -p is a pure rename plus the run-name addition
• security:gh-actions (zizmor) passes locally

Validation

• zizmor --offline .github/workflows/integ.yml → no findings
• YAML parses OK

Note

Replaces the earlier runtime-guard approach (force-pushed) — that added executed code, which #896 explicitly rules out of scope, and would have left the false positive open.

[isadeks]

Verdict: Approve (with non-blocking nits)

The rename is mechanically complete and value-preserving, neither human gate moves, and the run-name addition closes a real gap in the one control standing between fork-authored code and the deploy role. Everything below is comment accuracy, one cheap expression fix, and follow-up tracking.

Governance clean: #896 carries approved, the branch follows fix/<issue>-<slug>, and both required checks are green (build (agentcore), Secrets, deps, and workflow scan). mergeStateStatus: BLOCKED was REVIEW_REQUIRED only, not a failing check.

On the CodeQL evidence — the acceptance criterion is not confirmable pre-merge. It is tempting to read the PR-head analysis as proof: the /language:actions analysis at 45ebf723 reports results_count: 0 while main at 8d37737e reports 1. That inference does not hold. Every PR-ref actions analysis in this repo reports 0, including #895, #894 and #888, which do not touch integ.yml at all — PR analyses are diff-scoped, so the 0 carries no information about alert #35. The mechanism reasoning in #896 (quoting UntrustedCheckoutQuery.qll directly, including the needs.* / GitHubExpression asymmetry) is sound and I would merge on it, but the confirmation is genuinely post-merge. See N5.

---

Vision alignment

Fits. Tenet 7 (observable, attributable) is the direct hit: for a workflow_run run GitHub attributes the run to the base repo, so the approver authorizing fork code to run with AWS_ROLE_TO_ASSUME could not see which fork or commit they were authorizing without opening the resolve logs. That disclosure belongs exactly at the gate. Tenet 5 (isolation and least privilege) is preserved unchanged — permissions: {} (:49), per-job scopes, persist-credentials: false, environment: deploy, id-token: write are all byte-identical. Tenet 4 (fail closed on risk) is served by clearing the finding at the source rather than dismissing it, consistent with the posture #277 is circling. No tenet traded, no ADR owed.

Blocking issues

None.

Non-blocking suggestions

N1 — run-name renders integ — @ on the manual path, and the comment says otherwise (:7-8, :9-10)

The literal @ sits between the two expressions, so on workflow_dispatch only the operands go empty; GitHub falls back to the workflow name only when the whole run-name is empty, which this is not. So #896's criterion "remains readable for workflow_dispatch" is met only weakly, and "renders as a bare integ —" is wrong.

Worth fixing before merge rather than after, because a workflow_run-triggered workflow always executes the base repo's default-branch copy of the file — this expression cannot be exercised until it lands on main.

run-name: >-
  integ — ${{ github.event_name == 'workflow_run'
  && format('{0}@{1}', github.event.workflow_run.head_repository.full_name, github.event.workflow_run.head_sha)
  || format('manual @{0}', github.sha) }}

Optional hardening in the same line: head_repository.full_name is wholly fork-controlled and rendered unescaped, so aws-sampIes/sample-… (capital I) reads as first-party at a glance. Deriving the bit that matters from a comparison GitHub computes — ${{ github.event.workflow_run.head_repository.full_name == github.repository && 'BASE' || 'FORK' }} — cannot be spoofed by naming. Genuinely optional: reaching the prompt already requires a maintainer to have applied safe-to-test, the approver is directed at the diff (:209-212), and the run name strictly adds information versus today's nothing.

N2 — four accuracy fixes in the two new comment blocks (:3-8, :68-74)

This file's existing comments are unusually good — :30-35 enumerates every mitigation behind the zizmor suppression, :105-108 explains the EXIT trap's failure mode, :169-174 covers the set -e / if ! interaction. These two blocks should match that bar.

1. :70-73 presents the two regexes as mutually exclusive. head_sha matches both — per #896's own QL quote the SHA pattern is .*(head|sha|commit).* — and the mutable-ref match is what fires. As written it implies "contains sha ⇒ safe", which would bless head_commit_sha. State the real rule: the name must not contain head, branch, or ref.
2. :74 pins the rot-prone identifier. Alert numbers are repo-scoped and reassigned on reopen, and mean nothing to a reader without Security-tab access. Use the query id actions/untrusted-checkout-toctou/critical; keep #35 as parenthetical history.
3. :68-69 attributes the value to github.event.workflow_run.head_sha, but the dispatch path at :95 writes GITHUB_SHA. The immutability conclusion still holds; the parenthetical is wrong half the time. Add "or GITHUB_SHA on manual dispatch".
4. Add one clause recording that the surviving HEAD_SHA env/shell names (:86, :101, :255, :286) are deliberately unchanged, because CodeQL inspects only the checkout ref expression. A maintainer who reads "the name is load-bearing" and then finds five HEAD_SHAs has to guess whether the rename was incomplete, and could "finish" it in either direction.

Optional 5th: the block does not say where a regression would surface. Nothing local catches it — zizmor does not implement this heuristic — and CodeQL is non-blocking (N4).

N3 — assert the invariant the new name now asserts (:95, :102, or a first step in integ)

Worth stating why I rate this a nit rather than a blocker, because it can read either way. Pre-rename the query fired on the field name unconditionally, safe or not, so it carried no information and nothing was lost by silencing it. But nothing asserts immutability either, before or after — and a partial rename would have failed open: needs.resolve.outputs.commit_sha resolving to empty yields an empty ref: on a checkout whose repository: is the fork, which actions/checkout resolves to the fork's default-branch tip — unreviewed code under the deploy role, reported green.

Not reachable as written: commit_sha is written in the same step-branch as applicable=true on both paths (:95/:96, :102/:103) and integ gates on applicable == 'true' (:195). But [[ "$HEAD_SHA" =~ ^[0-9a-f]{40}$ ]] || exit 1 before the write converts that whole edit class from fail-open to fail-closed for two lines, and makes the name true rather than merely conventional. Same argument for asserting head_repo non-empty — an empty repository: input silently defaults to github.repository.

N4 — the cheap structural guard is a settings change, not code

CodeQL and Analyze (actions) run per-PR but are not in the main ruleset's required checks, so a rename back to head_sha would go red-but-mergeable — visible and ignorable rather than blocking. Adding CodeQL to the required checks subsumes most of the value of a custom guard.

If you want the guard too, the repo's pattern is cdk/test/constructs/agent-image-pins.test.ts:23-56 — read the file as text, assert every ref is pinned, plus an anti-vacuity test that it found the refs it claims to check. Frame the assertion as "the ref checked out into the privileged deploy environment must be an immutable commit SHA", not as "the output name must match /(sha|commit)/", which would pin a third-party linter's implementation detail. js-yaml is already a cdk dependency.

N5 — the description checks [x] Alert #35 cleared while parenthetically admitting it is post-merge-verifiable

Uncheck or annotate it, per the evidence note above.

Documentation

No docs/ source is touched, so Starlight mirror sync is correctly not applicable. Two gaps, both nits:

• docs/decisions/ADR-013-tiered-validation-pyramid.md:140 describes the two human gates and instructs the approver to review cdk/test/integ/**. This PR changes what that approver can see; one clause noting the run name now discloses <head repo>@<sha> keeps the design record current.
• ADR-013:132 and integ.yml:13-14 both claim integ-smoke is "a required status that blocks merge". It is not — the main ruleset requires only build (agentcore) and Secrets, deps, and workflow scan. #896 documents this as drift and scopes it out; it remains unfiled.

Issue tracking — the one item I would actually push on. #896's five out-of-scope findings say "each needs its own issue", and none has been filed. Two are security-relevant (the safe-to-test label being stale by design, so labelling at commit A passes the gate for unreviewed commit B; and the path filter reading live PR state rather than the commit under test). Two more surfaced during this review, both wrong-but-green and neither on that list:

• :137-150 — gh api "repos/$REPO/commits/$HEAD_SHA/pulls" 2>/dev/null || true makes a rate-limit or 5xx byte-identical to a legitimate "this commit has no PR". The exit 0 that follows is on a path where the EXIT trap is gated on rc -ne 0 (:126), so integ-smoke is never posted at all — a green workflow, a ::warning:: in the log, and a missing check. :175 gets this exactly right for the sibling /files call ("Fail loud on API error: a failed or truncated response must NOT fall through to a false-green skip"), which makes :142 the inconsistent outlier. Note workflow_run.pull_requests is empty for fork PRs, so :142 is the only resolver on the fork path this workflow exists to serve.
• :85 / :142 — pull_requests[0] picks arbitrarily when one head SHA heads two PRs (stacked branches, retargeted base). Commit statuses are keyed by SHA, not PR, so a docs-only sibling can post a green integ-smoke onto a PR whose cdk/** changes never ran.

Also: #896 carries approved/security/ci-cd/v1 but no priority label.

Tests & CI

No tests ship, and none are reasonably available: this repo has zero guards that parse .github/workflows/**. zizmor is a required check and does cover the file (mise.toml:214-217 → security-pr.yml:113, job "Secrets, deps, and workflow scan"), but it reasons about semantics rather than identifier spelling, so it is structurally blind to this diff in both directions — and the audit nearest this area is already suppressed inline at :30. .pre-commit-config.yaml:18's check-yaml is syntax-only. CodeQL is GitHub default setup, not a committed workflow.

Bootstrap synth coverage (ADR-002): not applicable — no construct, stack, handler, or CloudFormation resource type is touched.

run-name is unexercisable pre-merge for the reason in N1; its first live evaluation on both paths will be after this lands.

Review agents run

• code-reviewer — ran. Confirmed pure-rename by line accounting (6 deletions = 6 renamed lines; 22 insertions = 5 renamed + 8 output/comment + 9 run-name/comment); caught the integ — @ render and the CodeQL-evidence overstatement.
• comment-analyzer — ran. Source of most of N2.
• silent-failure-hunter — ran. Traced the empty-ref: hazard and the pre-existing swallowed-error paths. I downgraded its findings 1 and 3 from critical/high: both describe a hypothetical partial rename and a cosmetic dispatch title rather than defects in this diff. Its pre-existing findings are the valuable output and are folded in above.
• pr-test-analyzer — ran. Established the zero-workflow-guard picture and the required-vs-non-required CodeQL asymmetry (N4).
• security-review — ran. No findings at ≥0.7 confidence. Verified the rename is exhaustive (no resolve.outputs.head_sha survives anywhere in the repo, and job outputs are workflow-scoped so there is no external consumer), that run-name is a non-shell UI string over GitHub-charset-constrained values with no injection sink and no access to secrets, and that run-name and the checkout read the same frozen workflow_run payload fields (:10 vs :86-87 → :102-103 → :213-214), so the title cannot name one commit while another executes.
• type-design-analyzer — omitted: no types in the diff (YAML workflow only).

Human heuristics

• Proportionality — pass. 22/6 lines, 12 of the insertions comment, for a critical-severity finding plus a gate-disclosure gap. No abstraction introduced for a one-off.
• Coherence — concern (minor). One value now travels under two names in one file: the commit_sha output and HEAD_SHA at :86, :101, :255, :286. Deliberate and correct, but undocumented — N2 item 4.
• Clarity — concern. The four inaccuracies in N2 sit in the two blocks this PR adds, one of which is explicitly load-bearing, in a file whose other comments are a model of explaining why.
• Appropriateness — pass. The diagnosis was verified against the actual QL source rather than guessed, and the earlier runtime-guard approach was correctly abandoned as outside #896's scope (it would have added executed code and left the false positive open).

[scottschreckengaust]

LGTM