From 0fe37f9f5f00e871f667bef17fb09db15e10e196 Mon Sep 17 00:00:00 2001 From: aaight Date: Wed, 15 Jul 2026 17:03:37 +0200 Subject: [PATCH] fix(review): degrade gracefully when CI check status is unavailable (MNG-1750) (#1494) Co-authored-by: Cascade Bot --- CLAUDE.md | 2 + src/agents/definitions/contextSteps.ts | 32 +++++++- src/gadgets/github/core/getPRChecks.ts | 28 +++++++ .../agents/definitions/contextSteps.test.ts | 76 +++++++++++++++++++ .../gadgets/github/core/getPRChecks.test.ts | 40 ++++++++++ 5 files changed, 174 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9de7ed7bd..af7c2c9c6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -167,6 +167,8 @@ GitHub's changed-file API is used for file enumeration and change counts, but co When review output misses something, check the `PR context prepared` log entry for `included` / `skipped` / `skipReasons`, `patchSources`, `totalDiffTokens`, `perFileTokenCap`, and `localGitMismatches` to confirm whether the file was visible to the agent and whether GitHub's API patch differed from the local patch. Also check context offload logs if the diff context was written under `.cascade/context/`. +CI check status is **informational, not fatal** (MNG-1750): the `fetchPRContextStep` boot step wraps only `getCheckSuiteStatus` in a try/catch. If the reviewer PAT lacks the **Actions: Read** permission the Actions API throws 403, and the `GetPRChecks` context injection degrades to an explicit "CI check status UNAVAILABLE" message (with the permission hint, deliberately distinct from "No CI checks configured") plus a `WARN CI check status unavailable` log — instead of killing the agent boot with a `BootFailureError`. PR details (`getPR`) and the diff (`getPRDiff`) stay fatal — a review without the PR itself is meaningless. + **cascade-tools shell-safety contract** — MNG-1059. cascade-tools commands that accept markdown/multiline payloads (`--body`, `--text`, `--description`, `--details`, `--comments`) declare a `--*-file ` companion via `cli.fileInputAlternatives`. Agents are instructed in the system prompt to prefer the file form when content contains backticks, code fences, `$(...)`, or newlines — shells expand those tokens even inside single quotes once they layer through `bash -c`, and newlines break argv parsing. The shared CLI factory at `src/gadgets/shared/cli/params.ts:rejectMultipleStdinConsumers` enforces the single-stdin-consumer invariant: only one `--*-file -` per command. Passing two stdin consumers (e.g. `--body-file - --comments-file -`) returns a structured `flag-parse` envelope with `error.flag: "body-file,comments-file"` and a hint to write one payload to a temp file — *before* any `readFileSync(0, ...)` call. The native-tool system prompt also renders a "cascade-tools shell-safety rules" section with safe heredoc / temp-file patterns. Prompt example rendering suppresses inline `--body '...'` examples for shell-sensitive content (backticks / `$(...)` / newlines) when a file-input companion exists, redirecting agents at the safer `--*-file ` form. ## Engines diff --git a/src/agents/definitions/contextSteps.ts b/src/agents/definitions/contextSteps.ts index 4d2021f19..02ef75e4f 100644 --- a/src/agents/definitions/contextSteps.ts +++ b/src/agents/definitions/contextSteps.ts @@ -5,7 +5,10 @@ * These are the building blocks composed by the YAML contextPipeline arrays. */ -import { formatCheckStatus } from '../../gadgets/github/core/getPRChecks.js'; +import { + formatCheckStatus, + formatCheckStatusUnavailable, +} from '../../gadgets/github/core/getPRChecks.js'; import { ListDirectory } from '../../gadgets/ListDirectory.js'; import { readStructuredWorkItemDetails, @@ -164,10 +167,31 @@ export async function fetchPRContextStep(params: FetchContextParams): Promise expect.objectContaining({ baseBranch: 'parent-feature' }), ); }); + + // MNG-1750: a reviewer PAT lacking the "Actions: Read" permission makes + // getCheckSuiteStatus throw 403. That must degrade to an informational + // injection instead of killing the whole agent boot (BootFailureError). + describe('MNG-1750 — graceful CI check-status degradation', () => { + beforeEach(() => { + mockGetPRDiff.mockResolvedValue([ + { + filename: 'src/a.ts', + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + patch: '@@ -1 +1 @@\n+x', + }, + ]); + }); + + it('proceeds when getCheckSuiteStatus throws 403 and injects an UNAVAILABLE signal', async () => { + const err = Object.assign(new Error('Resource not accessible by personal access token'), { + status: 403, + }); + mockGetCheckSuiteStatus.mockRejectedValue(err); + + const injections = await fetchPRContextStep(makePRParams()); // must NOT throw + + const checks = injections.find((i) => i.toolName === 'GetPRChecks'); + expect(checks).toBeDefined(); + expect(checks?.result as string).toContain('UNAVAILABLE'); + expect(checks?.result as string).toContain( + 'Resource not accessible by personal access token', + ); + expect(checks?.result as string).toContain('Actions: Read'); + expect(checks?.description).toBe('CI check status unavailable'); + }); + + it('logs a WARN carrying the upstream error message', async () => { + mockGetCheckSuiteStatus.mockRejectedValue( + new Error('Resource not accessible by personal access token'), + ); + + const params = makePRParams(); + await fetchPRContextStep(params); + + expect(params.logWriter).toHaveBeenCalledWith( + 'WARN', + 'CI check status unavailable', + expect.objectContaining({ error: 'Resource not accessible by personal access token' }), + ); + }); + + it('leaves the GetPRChecks injection unchanged on the success path', async () => { + mockGetCheckSuiteStatus.mockResolvedValue({ + totalCount: 1, + checkRuns: [{ name: 'build', status: 'completed', conclusion: 'success' }], + allPassing: true, + }); + + const injections = await fetchPRContextStep(makePRParams()); + + const checks = injections.find((i) => i.toolName === 'GetPRChecks'); + expect(checks?.result as string).toContain('PR #1092 Check Status: 1/1'); + expect(checks?.description).toBe('Pre-fetched CI check status'); + }); + + it('still throws when getPR fails (PR details stay fatal)', async () => { + mockGetPR.mockRejectedValueOnce(new Error('PR not found')); + await expect(fetchPRContextStep(makePRParams())).rejects.toThrow('PR not found'); + }); + + it('still throws when getPRDiff fails (diff stays fatal)', async () => { + mockGetPRDiff.mockReset(); + mockGetPRDiff.mockRejectedValueOnce(new Error('diff unavailable')); + await expect(fetchPRContextStep(makePRParams())).rejects.toThrow('diff unavailable'); + }); + }); }); diff --git a/tests/unit/gadgets/github/core/getPRChecks.test.ts b/tests/unit/gadgets/github/core/getPRChecks.test.ts index 7f9976319..d4bc6bf8d 100644 --- a/tests/unit/gadgets/github/core/getPRChecks.test.ts +++ b/tests/unit/gadgets/github/core/getPRChecks.test.ts @@ -9,6 +9,7 @@ vi.mock('../../../../../src/github/client.js', () => ({ import { formatCheckStatus, + formatCheckStatusUnavailable, getPRChecks, } from '../../../../../src/gadgets/github/core/getPRChecks.js'; import { githubClient } from '../../../../../src/github/client.js'; @@ -120,6 +121,45 @@ describe('formatCheckStatus', () => { }); }); +describe('formatCheckStatusUnavailable', () => { + it('includes the upstream error message', () => { + const result = formatCheckStatusUnavailable( + 7, + 'Resource not accessible by personal access token', + ); + expect(result).toContain('Resource not accessible by personal access token'); + }); + + it('includes the "Actions: Read" permission hint', () => { + const result = formatCheckStatusUnavailable(7, 'boom'); + expect(result).toContain('Actions: Read'); + }); + + it('references the PR number', () => { + const result = formatCheckStatusUnavailable(1234, 'boom'); + expect(result).toContain('PR #1234'); + }); + + it('is visibly distinct from the "No CI checks configured" message', () => { + const unavailable = formatCheckStatusUnavailable(7, 'boom'); + const noChecks = formatCheckStatus(7, { totalCount: 0, allPassing: true, checkRuns: [] }); + // The "unavailable" text carries the UNAVAILABLE marker; the "no checks" + // text does not. (The unavailable text intentionally *mentions* the + // "No CI checks configured" phrase to contrast against it, so the two are + // distinguished by the UNAVAILABLE marker and by not being equal.) + expect(unavailable).toContain('UNAVAILABLE'); + expect(noChecks).not.toContain('UNAVAILABLE'); + expect(unavailable).not.toBe(noChecks); + }); + + it('does not stringify error objects (takes a plain string only)', () => { + // The caller passes error.message, never the whole RequestError. This + // helper's signature enforces that: a string in, no [object Object]. + const result = formatCheckStatusUnavailable(1, 'plain message'); + expect(result).not.toContain('[object Object]'); + }); +}); + describe('getPRChecks', () => { beforeEach(() => { vi.resetAllMocks();