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

The Run security gates step was hitting the job-level 5-minute timeout
(timeout-minutes: 5) and cancelling the entire 'review' check, even when
the script's documented contract is 'always exit 0 — silent and
informational'.

continue-on-error: true on a step suppresses step failures but does NOT
override a job-level timeout. The script could hang in any unbounded API
call (security-advisories list pagination, the advisory POST/PATCH, etc.)
and take down the whole job.

Four layered defences so any single failure mode is contained:

- ghFetch: per-call AbortController timeout (default 15s) so no single
  GitHub API call can eat the workflow budget.
- check-pr-security: 90s wall-clock watchdog that exits 0 regardless of
  what the rest of the script is doing, matching the always-exit-0
  contract.
- findExistingDraftAdvisory: 20-page pagination cap with console.warn,
  falling through to create a fresh draft instead of looping forever.
- commitperclip-review.yml: timeout-minutes: 3 on the security step
  (combined with continue-on-error: true so the job stays green if it
  ever fires).

Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Devin FoleyandPaperclip committed 2026-06-09 15:06:37 -07:00
1 parent 0a2230b2ec
commit 00ae558b25
4 files changed
+127 -13

No files matched your search

@@ -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;
}
});