👍🏻

Actionable comments posted: 3

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/scripts/evidence-sign-unsigned.sh:
- Around line 44-45: The signer lookup in evidence-sign-unsigned.sh currently
masks `yq` or YAML parsing failures by falling back to "null", which can
misclassify signed pointers as unsigned. Update the logic around the `yq eval`
call that populates `signer` so failures from `yq` or invalid input cause the
script to fail closed instead of echoing "null", and keep the unsigned check in
the existing `signer` handling block.

In @.github/workflows/evidence-publish.yaml:
- Around line 98-115: The evidence publishing workflow currently loses
already-signed pointer patches if a later signing step fails, so the partial
work is never committed. Update the sign-and-commit flow in
evidence-publish.yaml around the “Sign unsigned evidence pointers” and “Commit
signed pointers” steps so that successful pointer patches are preserved and
committed even when a later signing operation errors; ensure the sign script or
its wrapper records partial success and the commit step still stages and pushes
any signed changes from recipes/evidence/.

In `@docs/contributor/evidence-publishing.md`:
- Around line 65-68: The example in the evidence publishing guide uses the wrong
commit-signing flag for external contributors. Update the command shown in the
contributor docs near the evidence-publishing example so it reflects the repo’s
real policy: use git commit -s for external contributors’ DCO sign-off, and only
mention git commit -S for NVIDIA org members if needed. Make the example wording
in that section consistent with the symbols and commands shown there, especially
git commit and the evidence/<slug>.yaml workflow.

📋 Multi-persona review — PR #1451

Method: 3 independent persona passes (Security/CI supply-chain · Bash/Reliability · Docs/DX), each finding independently confirmed, refuted, or re-tiered by a senior meta-reviewer against the resolved code (CLI source on main, the repo's required_signatures ruleset, and GitHub Actions if: semantics). Line links pinned to head 00228d4.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick · ✅ Confirmed non-issue

Overall assessment — Approve with comments (resolve the two 🟠 first)

The two-phase split — push the network-light leg locally with --no-sign, do the Fulcio-bound leg in CI where Sigstore is reachable — is a sound design, and the workflow_dispatch-on-the-fork framing genuinely defuses the worst class of CI risk (no pull_request_target, no untrusted-ref exposure). No hard blocker. Two Major issues are merge-relevant: a workflow step whose if: silently defeats the script's own fail-closed exit, and an unsigned bot commit-back that contradicts both the doc's "fully signed pointer" wording and the repo's required_signatures ruleset. The remaining items are robustness, a real naming-confusion trap, and polish. Notably, the scariest candidate — a signed-in-registry-but-un-patched-pointer race — was refuted on inspection of SignExisting.

✅ Confirmed non-issues (checked and cleared)

• No signed-in-registry-but-un-patched-pointer race. The highest-proposed-tier candidate was refuted: SignExisting calls ValidateSignablePointer (rejects a non-nil signer with ErrCodeConflict), and on success sets Attestations[0].Signer and writes the file. A successful sign therefore always mutates the pointer, so git diff --quiet cannot find "nothing to commit" after a real --no-sign pointer is signed; an idempotent re-sign returns non-zero and counts as failed, not signed.
• #aicr-evidence-sign, #aicr-evidence-publish, #aicr-validate, and ../design/007-recipe-evidence.md all resolve on current main (lychee-safe). CLI flag claims are accurate: --no-sign on both validate and evidence publish; positional bundle dir; evidence verify reports unsigned as a non-failing "pending signature".
• concurrency with cancel-in-progress: false is correct — a mid-sign cancel could orphan a signed bundle from its un-patched pointer.
• Fork-only workflow_dispatch trust model is sound: no pull_request_target, no untrusted-ref injection; actions/checkout and actions/setup-go are SHA-pinned; timeout-minutes: 20 set.

Summary

Recommendation: Approve with comments. No hard blocker. Fix the two 🟠 (one-line success() && guard; commit-back via peter-evans/create-pull-request, which also clears the persisted-token and bare-push items) before merge — the second could escalate to 🔴 if required_signatures blocks the PR merge. The 🟡/🔵 items are safe to address in the same pass or defer in writing.

_{Generated via a multi-persona + adversarial-meta-reviewer pass; findings independently re-derived from resolved code before surviving.}

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/evidence-publish.yaml:
- Around line 122-129: The workflow-generated commit in the evidence publishing
step is missing a DCO sign-off trailer, which can cause the PR-head commit to
fail DCO checks for external contributors. Update the commit creation in the
evidence publish workflow by using the existing git commit step with a sign-off
option so the generated commit includes a Signed-off-by trailer, and keep the
change localized around the git commit command in the evidence publishing job.

✅ Approve — with two non-blocking notes

Re-reviewed at head e17b4b3. The revision resolves 11 of 13 findings from the multi-persona pass, including both 🟠 Majors:

• F1 (commit-step if:) — now always() && … with documented intent: partial-failure still commits the pointers that did sign so the registry never holds a signed bundle with an unsigned committed pointer, while the job still fails to surface the error. Deliberate, well-reasoned.
• F2 (unsigned bot commit) — documented that the commit relies on the eventual squash-merge for the repo's commit-signing policy; [skip ci] dropped; doc softened from "fully signed pointer" to "a pointer."
• F4 yq fail-open → fixed (fail-closed on parse error). F5 persist-credentials: false + inline push token. F6 mikefarah-yq dependency documented. F7 non-fast-forward documented as recoverable. F9 id-token "default" wording corrected. F11 [skip ci] removed. F12 skipped added to $GITHUB_OUTPUT. F13 concurrent-push warning added. F14 go mod verify added.

Two stragglers remain, both non-blocking (inline) — neither holds up the approve. Nice work on the turnaround.

Tier legend: 🟡 Minor · 🔵 Nitpick

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/evidence-publish.yaml:
- Around line 94-101: The build step for the aicr binary assumes the output
directory already exists, so `go build -o ./bin/aicr` can fail in this workflow.
Update the workflow around the `Build aicr` step to create `./bin` first, either
by adding a directory creation command before `go build` or by reusing an
existing setup step, so the `go build` command in the `Build aicr` job succeeds
reliably.

✅ Approve — re-reviewed at head 96130c1.

All 13 findings from the multi-persona pass are now resolved, including the two stragglers from the prior round:

• F8 (publish-vs-sign naming) — doc now explains the filename: "The filename says publish because it anticipates a future auto-publish leg (see the workflow header); today it only signs." Exactly the note that closes the loop.
• F10 (nav vs H1 casing) — H1 aligned to Title Case to match the nav.

Bonus hardening since the last revision: the bot commit-back now does git commit -s (DCO sign-off matching the github-actions[bot] author), with the rationale documented inline; step 2 of the doc mirrors it. Verified against CONTRIBUTING.md — external contributors -s, NVIDIA org members -S — the doc's claim is accurate.

CI is green across Lint, Test, CLI E2E, Security Scan, Fern Check (anchors/lychee), and actionlint; tests / E2E is still in progress (merge stays gated on it). Clean approve — nice, thorough iteration. 🟢