fix(commitperclip): stop security gate from hanging the review check (#7847)

## Thinking Path

> - Paperclip is the open source app people use to manage AI agents for
work
> - The commitperclip review workflow runs a security gate as part of CI
on every PR
> - The security script's header promises it always exits 0 and stays
silent/informational, but PRs that triggered a flag were failing with a
5-minute timeout
> - Two compounding bugs: `findExistingDraftAdvisory` paginated without
an upper bound, and the workflow step did not have `continue-on-error:
true`, so any hang inside the script turned into a hard `review` check
failure that blocked merge
> - This pull request caps the advisory pagination at 20 pages and adds
`continue-on-error: true` to the workflow step, aligning runtime
behavior with the script's documented "always exit 0" contract
> - The benefit is that future PRs flagged by the security gate no
longer block merge on a 5-minute timeout, and the gate stays
silent/informational as intended

## Linked Issues or Issue Description

Fixes: #7849

## What Changed

- `.github/workflows/commitperclip-review.yml`: added
`continue-on-error: true` to the `Run security gates` step so a hang or
non-zero exit cannot fail the `review` check (matches the script's
documented "always exit 0" contract).
- `.github/scripts/check-pr-security.mjs`: capped
`findExistingDraftAdvisory` pagination at 20 pages (= 2000 advisories)
and short-circuited with a `console.warn` when the cap is hit; if no
match is found within the cap, callers will simply create a new draft
instead of hanging forever.
- `.github/scripts/tests/check-pr-security.test.mjs`: added a test
asserting the pagination cap is enforced.

## Verification

- `node .github/scripts/tests/check-pr-security.test.mjs` — 31/31 pass,
including the new cap test.
- Step-level guarantee: `continue-on-error: true` makes the `Run
security gates` step non-blocking for the job, so even an unexpected
hang/timeout in this step can no longer fail the `review` check.

## Risks

- Low risk. Pagination cap is a defensive bound; the worst case is a
duplicate draft advisory (acceptable — the workflow continues).
`continue-on-error: true` is exactly what the script header already
promised; the workflow now matches its stated contract.

## Model Used

- Claude (claude-opus-4-7), extended thinking, tool use

## Checklist

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

Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Devin Foley
2026-06-09 15:29:25 -07:00
committed by GitHub
parent 468edd8b22
commit 47bd02647c
4 changed files with 127 additions and 13 deletions
@@ -10,9 +10,11 @@ import {
scanSupplyChain,
scanTestPatterns,
scanSensitivePaths,
startScriptWatchdog,
syncDraftAdvisory,
validateSensitivePaths,
} from '../check-pr-security.mjs';
import { ghFetch } from '../get-bot-token.mjs';
// ── scanSecrets ──────────────────────────────────────────────────────────────
@@ -143,6 +145,19 @@ test('findExistingDraftAdvisory: returns null when no matching draft advisory ex
assert.equal(advisory, null);
});
test('findExistingDraftAdvisory: bails out at the page cap so a large backlog cannot hang the workflow', async () => {
let pageCount = 0;
const fakeFetch = async () => {
pageCount += 1;
return Array.from({ length: 100 }, (_, i) => ({ summary: `Unrelated advisory ${pageCount}-${i}` }));
};
const advisory = await findExistingDraftAdvisory(fakeFetch, 'token', 'paperclipai/paperclip', 6469);
assert.equal(advisory, null);
assert.equal(pageCount, 20, `expected pagination to run exactly 20 pages (the cap), got ${pageCount}`);
});
test('syncDraftAdvisory: patches an existing advisory with the latest flags', async () => {
const calls = [];
const flags = [
@@ -318,3 +333,48 @@ test('scanSensitivePaths: ignores removed files even on sensitive paths', () =>
const files = [{ filename: 'server/src/routes/agents.ts', status: 'removed' }];
assert.equal(scanSensitivePaths(files).length, 0);
});
// ── startScriptWatchdog ──────────────────────────────────────────────────────
test('startScriptWatchdog: fires exit(0) when the wall-clock budget is exceeded', async () => {
let exitCode = null;
const fakeExit = (code) => { exitCode = code; };
startScriptWatchdog(20, fakeExit);
await new Promise((resolve) => setTimeout(resolve, 60));
assert.equal(exitCode, 0, 'watchdog should have exited with code 0 by now');
});
test('startScriptWatchdog: cleared timer never fires', async () => {
let exitCode = null;
const fakeExit = (code) => { exitCode = code; };
const timer = startScriptWatchdog(20, fakeExit);
clearTimeout(timer);
await new Promise((resolve) => setTimeout(resolve, 60));
assert.equal(exitCode, null, 'cleared watchdog must not call exit');
});
// ── ghFetch timeout ──────────────────────────────────────────────────────────
test('ghFetch: aborts the request when the per-call timeout elapses', async () => {
const originalFetch = globalThis.fetch;
// Replace global fetch with one that respects the AbortSignal but never resolves on its own.
globalThis.fetch = (_url, init) => new Promise((_resolve, reject) => {
init?.signal?.addEventListener('abort', () => {
const err = new Error('aborted');
err.name = 'AbortError';
reject(err);
}, { once: true });
});
try {
const start = Date.now();
await assert.rejects(
ghFetch('/repos/example/example/security-advisories', 'token', { timeoutMs: 30 }),
/aborted|abort/i,
);
const elapsed = Date.now() - start;
assert.ok(elapsed < 500, `ghFetch should abort within the timeout, took ${elapsed}ms`);
} finally {
globalThis.fetch = originalFetch;
}
});