fix(ci): security check-run must never claim an advisory it never filed

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• CI reliability / deploy-safety shell — specifically the security-review check-run that check-pr-security.mjs posts on every PR
• The draft-advisory POST and the check-run POST ran in an unawaited Promise.all, each swallowing its own failure independently. When the advisory POST 403'd (permission gap in the allyblockcast[bot] App installation), the check-run had no idea and asserted "Draft advisory filed for maintainer review" anyway
• A security gate that lies about where its findings went is worse than no gate — the surface a maintainer would check actively misdirects them to an artifact that doesn't exist, and the 20 detected flags survive only as a console.error line in a CI log nobody reads
• This pull request sequences the advisory sync ahead of the check-run post and threads the real outcome through, so the check-run output is truthful in every case, and inlines the flag list into output.text as a durable, human-visible record when the advisory can't be created
• The benefit is the security gate can never again silently lose findings behind a claim of coverage that isn't real, regardless of whether/when the advisory-creation permission gets granted

Linked Issues or Issue Description

Fixes: BLO-22692 (https://paperclip.blockcast.net/BLO/issues/BLO-22692) — no public GitHub issue, tracked in Paperclip only. Found while fixing CI on BLO-17980 / PR #1092.

What Changed

• .github/scripts/check-pr-security.mjs: sequence syncDraftAdvisory before postSecurityCheckRun (was an unawaited Promise.all where each call swallowed its own failure) so the check-run always knows the real outcome of the advisory sync.
• Added buildSecurityCheckRunOutput(hasFlags, flags, advisoryResult): a pure function producing the check-run output — links the advisory when the sync succeeded, states the failure and inlines the full flag list into output.text when it didn't, never claims an advisory exists that wasn't created.
• postSecurityCheckRun now takes { flags, advisoryResult } and delegates to buildSecurityCheckRunOutput — same call sites, no other consumers of this signature exist in the repo (checked).
• Extracted formatFlagList (previously inlined in buildAdvisoryPayload) so the advisory description and the check-run's inlined fallback render flags identically.
• Behavior is unchanged when there are no flags, and the gate still always exits 0 / posts conclusion: neutral — never blocks merge.

Verification

• node --test .github/scripts/tests/check-pr-security.test.mjs — 38/38 pass, including 3 new tests asserting the check-run summary never contains "Draft advisory filed" when the sync fails, that the flags (check, file, pattern) are inlined in output.text, and that the "all clear" / advisory-succeeded paths are unaffected.
• node --test .github/scripts/tests/*.mjs — full scripts suite, 157/157 pass.
• Confirmed no other file in the repo calls postSecurityCheckRun, syncDraftAdvisory, or buildAdvisoryPayload — the only consumer is check-pr-security.mjs's own main(), and the workflow step (commitperclip-review.yml) just runs the script with no downstream parsing of its output.

Risks

Low risk. This only changes what text lands in the security-review check-run's output — the check-run's name, status: completed, and conclusion (neutral/success) values, and the always-exit-0 contract, are all unchanged, so merge behavior for every PR is identical to before. The only externally-visible difference is that a maintainer reading the check-run after an advisory-sync failure now sees the real flags instead of a false claim.

Not addressed in this PR (deliberately, per BLO-22692's scope): the permission gap itself. Creating a repository security advisory needs a scope the allyblockcast[bot] App installation does not currently hold, and per BLO-18941 this class of change is a human/repo-admin gate, not something an agent can grant itself. I'm recording the decision and escalating it back through Paperclip (BLO-22692) rather than acting on it here — this PR makes the check-run truthful under either outcome of that decision (advisory granted later vs. advisory path eventually removed), so it isn't blocked on it.

Model Used

Claude Sonnet 5 (claude-sonnet-5, 1M context), no extended thinking, agentic tool use (file read/edit, bash test runs, GitHub MCP).

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — N/A, no UI change
• I have updated relevant documentation to reflect my changes — N/A, self-documenting script change
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending CI run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-18941
🔗 Paperclip issue: BLO-22692
🔗 Paperclip issue: BLO-17980

[allyblockcast]

🔗 Paperclip issue: BLO-18941
🔗 Paperclip issue: BLO-22692
🔗 Paperclip issue: BLO-17980

[allyblockcast]

@ally please review. Focus areas:

1. buildSecurityCheckRunOutput — confirm the three branches (advisory succeeded / advisory failed / no advisory attempted) never produce a summary claiming "Draft advisory filed" unless advisoryResult?.ok is true.
2. The main() sequencing change — advisory sync now runs before the check-run POST (was Promise.all), confirm this doesn't meaningfully slow the gate given the 90s watchdog and 3-minute workflow timeout.
3. Test coverage in .github/scripts/tests/check-pr-security.test.mjs — the 4 new/updated postSecurityCheckRun tests plus the buildSecurityCheckRunOutput unit test.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 764109b

Critical Issues (0)

Important Issues (1)

• [gstack/review] .github/scripts/check-pr-security.mjs:421 — Serializing the advisory sync ahead of the check-run can prevent the durable check-run from being posted at all. ghFetch has a 15-second timeout per request, but findExistingDraftAdvisory can make up to 20 sequential requests; the script-wide watchdog can therefore fire while awaiting advisory sync, before execution reaches postSecurityCheckRun. The previous Promise.all started the check-run immediately, so this is a failure-mode regression precisely when the advisory API is slow or degraded.
  • Give the complete advisory operation its own abortable wall-clock budget that leaves enough of the 90-second watchdog for the check-run POST, then convert expiry to { ok: false, error } and post the truthful inlined fallback. Add a test where advisory lookup exceeds that budget and assert the check-run is still attempted.

Suggestions (0)

Strengths

• buildSecurityCheckRunOutput only emits "Draft advisory filed" under advisoryResult?.ok; failed and unattempted paths explicitly deny creation and inline the flags.
• The exact-head targeted suite passes: 38 tests, 0 failures.
• Successful, failed, unattempted, and all-clear output branches are covered directly, and the extracted flag formatter keeps advisory and fallback rendering aligned.

Recommended Action

1. Address the Important issue this cycle.

The PR is authored by app/allyblockcast, so the Ally App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed the Important finding in f5bac5ed.

• Advisory pagination/create/update now receives a shared abort signal and runs inside a wall-clock budget derived from the 90s script watchdog.
• The flagged path reserves 20s for the check-run POST and converts advisory timeout into the existing truthful { ok: false, error } fallback.
• Added a regression test with a stalled advisory lookup that asserts /check-runs is still attempted.

Verification: node --test .github/scripts/tests/check-pr-security.test.mjs (39/39) and node --test .github/scripts/tests/*.mjs (149/149). I did not self-approve; the independent-author requirement remains.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f5bac5e

Prior Findings Dispositioned (1)

• prior:764109b important 1 — fixed — .github/scripts/check-pr-security.mjs:453 — The advisory operation now receives a shared abort signal and a wall-clock budget that reserves 20 seconds of the 90-second script watchdog for the subsequent check-run POST; the timeout regression reported on the earlier head is no longer present.

Critical Issues (0)

Important Issues (3)

• [gstack/review] .github/scripts/check-pr-security.mjs:310 — The advisory-failure fallback copies each flag's captured source line into check-run output. For secret-scan, that line contains the credential-like value itself, so failure of the private advisory path creates a second, durable disclosure surface in the broadly visible check result.
  • Use a separate redacted check-run formatter that never includes f.line or matched secret values, and add a test asserting a detected token is absent from the serialized check-run payload.
• [pr-review-toolkit] .github/scripts/check-pr-security.mjs:310 — The complete flag list is sent without bounding output.text to the Checks API's 65,535-character limit. A large flagged PR can therefore make the fallback POST fail with 422; because that failure is swallowed after advisory sync already failed, neither intended durable signal remains.
  • Truncate safely below the API limit, include an omitted-finding count, and test the serialized boundary with an oversized flag set.
• [native-codex] .github/scripts/check-pr-security.mjs:390 — Any rejected or aborted advisory request is classified as ok: false, after which line 304 states that the advisory "was not created." A POST can commit server-side before its response is lost or the client aborts, so transport failure makes advisory state unknown rather than proving non-creation.
  • Change failure copy to say the sync failed and advisory state is unknown, or perform a bounded reconciliation lookup before asserting absence. Add a test for a simulated POST that commits but rejects on abort.

Suggestions (0)

Strengths

• The shared abort signal and reserved watchdog budget correctly address the prior serial-timeout regression.
• Successful, failed, unattempted, all-clear, and advisory-timeout paths are covered directly.
• Exact-head targeted verification passes: 39 tests, 0 failures.

Recommended Action

1. Address the Important issues before merge.

The PR is authored by app/allyblockcast, so the Ally App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

Addressed all three Important findings in 9258c4fe5.

• Public check-run fallback formatting is now separate from the private advisory formatter and never includes captured source lines or matched secret values.
• output.text is capped at 60,000 characters, with per-finding bounds and an explicit omitted-finding count.
• Advisory transport failures now report that advisory state is unknown; a regression test simulates a POST committing before its response aborts.

Verification: node --test .github/scripts/tests/check-pr-security.test.mjs (41/41) and node --test .github/scripts/tests/*.mjs (151/151). CI is queued at exact head 9258c4fe5. I did not self-approve or close the PR; the independent-author gate remains.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9258c4f

Prior Findings Dispositioned (3)

• prior:f5bac5e important 1 — fixed — .github/scripts/check-pr-security.mjs:207 — The check-run formatter now emits only the check, file, pattern, and package metadata; it never serializes the captured flag.line containing a detected secret. The exact-head regression test at .github/scripts/tests/check-pr-security.test.mjs:233 verifies the token is absent from the posted payload.
• prior:f5bac5e important 2 — fixed — .github/scripts/check-pr-security.mjs:215 — Check-run text is now capped at 60,000 characters, each flag is independently bounded, and truncated output reports the omitted-finding count. The exact-head boundary test at .github/scripts/tests/check-pr-security.test.mjs:370 exercises an oversized flag set.
• prior:f5bac5e important 3 — fixed — .github/scripts/check-pr-security.mjs:337 — Advisory transport failures now say the sync failed and the advisory state is unknown rather than asserting non-creation. The exact-head committed-before-abort test at .github/scripts/tests/check-pr-security.test.mjs:325 covers the ambiguous POST outcome.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The fallback check-run remains durable and truthful across permission errors, advisory timeouts, and ambiguous transport failures without exposing captured secret values.
• The advisory budget preserves time for the check-run POST, while the shared abort signal reaches each paginated advisory lookup and the final POST/PATCH.
• Exact-head verification passes: 41 tests, 0 failures. Additional stress checks kept 100,000 findings to 59,996 characters with an omission count and confirmed secret redaction plus unknown-state wording.

Recommended Action

1. The implementation is clean on the reviewed head.

The PR is authored by app/allyblockcast, so the Ally App cannot review or approve its own PR. This exact head must be reopened under an independent author before an App approval is possible.