diff --git a/.changeset/green-owls-jump.md b/.changeset/green-owls-jump.md new file mode 100644 index 0000000..aee39d7 --- /dev/null +++ b/.changeset/green-owls-jump.md @@ -0,0 +1,16 @@ +--- +"layne": patch +--- + +fix(config-validator): accept scanner keys in $global and validate removeOnException label key + +`KNOWN_GLOBAL_KEYS` was missing `semgrep`, `trufflehog`, `claude`, and +`piAgent`, so using `$global.semgrep` (functional since the previous +config-inheritance fix) would cause `npm run validate-config` to report +an unknown key error. Added all four scanner keys and wired their +respective validators (`validateScanner`, `validateClaude`, +`validatePiAgent`) inside `validateGlobal`. + +`validateLabels` was also missing `removeOnException` from its checked +key list, even though the worker reads and applies it. The key is now +validated alongside the other five label keys. diff --git a/.changeset/lemon-impalas-yawn.md b/.changeset/lemon-impalas-yawn.md new file mode 100644 index 0000000..b32b213 --- /dev/null +++ b/.changeset/lemon-impalas-yawn.md @@ -0,0 +1,5 @@ +--- +"layne": patch +--- + +fix: claude prompt mode now signals an error when the API response is truncated by max_tokens instead of silently returning empty findings diff --git a/.changeset/orange-foxes-shine.md b/.changeset/orange-foxes-shine.md new file mode 100644 index 0000000..6c17b34 --- /dev/null +++ b/.changeset/orange-foxes-shine.md @@ -0,0 +1,13 @@ +--- +"layne": patch +--- + +fix(semgrep): capture end.line so multi-line findings span the full annotation range + +Semgrep emits both `start.line` and `end.line` in its JSON output, but +the adapter only read `start.line` and left `startLine`/`endLine` unset. +Every Semgrep annotation therefore collapsed to a single line even for +rules that match multi-line constructs (e.g. multi-line function calls, +object literals, imports). Added `end?: { line: number }` to the raw +result interface and populate `startLine`/`endLine` in `toFinding`. +Falls back to `startLine` when `end` is absent. diff --git a/.changeset/silver-hawks-run.md b/.changeset/silver-hawks-run.md new file mode 100644 index 0000000..0b16353 --- /dev/null +++ b/.changeset/silver-hawks-run.md @@ -0,0 +1,12 @@ +--- +"layne": patch +--- + +fix(server): confirmation comment only says "Re-running scan..." when a scan is actually queued + +The issue_comment handler only re-enqueues a scan when the latest check +run conclusion is 'failure'. However the confirmation comment always +ended with "Re-running scan..." regardless of whether a scan was queued. +Users who approved an exception while the check run was in a success or +neutral state (or when no check run existed) saw a misleading message. +The suffix is now conditional on the scan actually being enqueued. diff --git a/.changeset/vast-pandas-trade.md b/.changeset/vast-pandas-trade.md new file mode 100644 index 0000000..1405937 --- /dev/null +++ b/.changeset/vast-pandas-trade.md @@ -0,0 +1,5 @@ +--- +"layne": patch +--- + +fix: $global scanner config (semgrep, trufflehog, claude, piAgent) is now correctly inherited by per-repo entries diff --git a/src/__tests__/adapters/claude.test.ts b/src/__tests__/adapters/claude.test.ts index 9eb5562..f95f85a 100644 --- a/src/__tests__/adapters/claude.test.ts +++ b/src/__tests__/adapters/claude.test.ts @@ -221,6 +221,27 @@ describe('runClaude()', () => { expect(findings).toEqual([]); }); + it('logs an error when stop_reason is max_tokens instead of silently dropping findings', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockReadFile.mockResolvedValueOnce(DUMMY_CONTENT); + mockCreate.mockResolvedValueOnce({ + stop_reason: 'max_tokens', + content: [], + }); + + await runClaude({ + workspacePath: WORKSPACE, + changedFiles: CHANGED_FILES, + toolConfig: ENABLED_CONFIG, + }); + + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('max_tokens'), + ); + consoleSpy.mockRestore(); +}); + + it('passes the configured model to the API', async () => { mockReadFile.mockResolvedValueOnce(DUMMY_CONTENT); mockCreate.mockResolvedValueOnce(cleanResponse()); diff --git a/src/__tests__/adapters/semgrep.test.ts b/src/__tests__/adapters/semgrep.test.ts index a635096..1eebefa 100644 --- a/src/__tests__/adapters/semgrep.test.ts +++ b/src/__tests__/adapters/semgrep.test.ts @@ -238,4 +238,29 @@ describe('runSemgrep()', () => { const jsonIdx = args.indexOf('--json'); expect(jsonIdx).toBe(scanIdx + 1); }); + + describe('startLine / endLine from Semgrep end.line', () => { + it('sets startLine and endLine from start.line and end.line on a single-line finding', async () => { + stubStdout(semgrepOutput([SEMGREP_RESULT])); + const [f] = await runSemgrep({ workspacePath: '/tmp/ws', changedFiles: CHANGED_FILES }); + expect(f.startLine).toBe(10); + expect(f.endLine).toBe(10); + }); + + it('sets startLine and endLine from start.line and end.line on a multi-line finding', async () => { + const multiLine = { ...SEMGREP_RESULT, start: { line: 10, col: 1 }, end: { line: 14, col: 1 } }; + stubStdout(semgrepOutput([multiLine])); + const [f] = await runSemgrep({ workspacePath: '/tmp/ws', changedFiles: CHANGED_FILES }); + expect(f.startLine).toBe(10); + expect(f.endLine).toBe(14); + }); + + it('falls back endLine to startLine when end is missing from Semgrep output', async () => { + const noEnd = { check_id: SEMGREP_RESULT.check_id, path: SEMGREP_RESULT.path, start: { line: 7 }, extra: SEMGREP_RESULT.extra }; + stubStdout(semgrepOutput([noEnd])); + const [f] = await runSemgrep({ workspacePath: '/tmp/ws', changedFiles: CHANGED_FILES }); + expect(f.startLine).toBe(7); + expect(f.endLine).toBe(7); + }); + }); }); diff --git a/src/__tests__/config-validator.test.ts b/src/__tests__/config-validator.test.ts new file mode 100644 index 0000000..7aad123 --- /dev/null +++ b/src/__tests__/config-validator.test.ts @@ -0,0 +1,83 @@ +import { describe, it, expect } from 'vitest'; +import { validateConfig } from '../config-validator.js'; + +describe('validateConfig()', () => { + describe('$global scanner keys', () => { + it('accepts $global.semgrep without flagging it as an unknown key', () => { + const result = validateConfig({ + '$global': { semgrep: { extraArgs: ['--config', 'p/owasp-top-ten'] } }, + }); + expect(result.valid).toBe(true); + }); + + it('accepts $global.trufflehog without flagging it as an unknown key', () => { + const result = validateConfig({ + '$global': { trufflehog: { enabled: false } }, + }); + expect(result.valid).toBe(true); + }); + + it('accepts $global.claude without flagging it as an unknown key', () => { + const result = validateConfig({ + '$global': { claude: { enabled: true, model: 'claude-sonnet-4-6' } }, + }); + expect(result.valid).toBe(true); + }); + + it('accepts $global.piAgent without flagging it as an unknown key', () => { + const result = validateConfig({ + '$global': { piAgent: { enabled: false, model: 'claude-opus-4-6' } }, + }); + expect(result.valid).toBe(true); + }); + + it('still rejects a genuinely unknown key in $global', () => { + const result = validateConfig({ + '$global': { typo_key: true }, + }); + expect(result.valid).toBe(false); + expect((result as { valid: false; errors: string[] }).errors[0]).toMatch(/unknown key/); + }); + + it('validates the contents of $global.semgrep (rejects bad extraArgs)', () => { + const result = validateConfig({ + '$global': { semgrep: { extraArgs: [123] } }, + }); + expect(result.valid).toBe(false); + expect((result as { valid: false; errors: string[] }).errors[0]).toMatch(/extraArgs/); + }); + + it('validates the contents of $global.claude (rejects non-claude model)', () => { + const result = validateConfig({ + '$global': { claude: { enabled: true, model: 'gpt-4o' } }, + }); + expect(result.valid).toBe(false); + expect((result as { valid: false; errors: string[] }).errors[0]).toMatch(/model/); + }); + }); + + describe('validateLabels — removeOnException', () => { + it('accepts labels.removeOnException as a valid key', () => { + const result = validateConfig({ + 'acme/app': { labels: { removeOnException: ['security-exception'] } }, + }); + expect(result.valid).toBe(true); + }); + + it('rejects labels.removeOnException when not an array', () => { + const result = validateConfig({ + 'acme/app': { labels: { removeOnException: 'security-exception' } }, + }); + expect(result.valid).toBe(false); + expect((result as { valid: false; errors: string[] }).errors[0]).toMatch(/removeOnException/); + }); + + it('rejects labels.removeOnException when items are not strings', () => { + const result = validateConfig({ + 'acme/app': { labels: { removeOnException: [42] } }, + }); + expect(result.valid).toBe(false); + expect((result as { valid: false; errors: string[] }).errors[0]).toMatch(/removeOnException/); + }); + }); +}); diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index c801950..54be422 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -409,6 +409,24 @@ describe('loadScanConfig()', () => { // --- piAgent defaults --- + it('inherits $global semgrep config when the repo has no semgrep block', async () => { + vi.mocked(readFile).mockResolvedValueOnce(JSON.stringify({ + '$global': { semgrep: { extraArgs: ['--config', 'p/owasp-top-ten'] } }, + 'acme/frontend': { mode: 'diff_only' }, + })); + const config = await loadScanConfig({ owner: 'acme', repo: 'frontend' }); + expect((config.semgrep as { extraArgs: string[] }).extraArgs).toEqual(['--config', 'p/owasp-top-ten']); +}); + +it('per-repo semgrep overrides $global semgrep at the key level', async () => { + vi.mocked(readFile).mockResolvedValueOnce(JSON.stringify({ + '$global': { semgrep: { extraArgs: ['--config', 'p/owasp-top-ten'] } }, + 'acme/payments': { semgrep: { extraArgs: ['--config', 'p/python'] } }, + })); + const config = await loadScanConfig({ owner: 'acme', repo: 'payments' }); + expect((config.semgrep as { extraArgs: string[] }).extraArgs).toEqual(['--config', 'p/python']); +}); + it('DEFAULT_CONFIG.piAgent has timeoutMinutes 10', () => { expect((DEFAULT_CONFIG.piAgent as Record).timeoutMinutes).toBe(10); }); @@ -437,3 +455,4 @@ describe('loadScanConfig()', () => { expect((config.piAgent as Record).timeoutMinutes).toBe(15); }); }); + diff --git a/src/__tests__/server.test.ts b/src/__tests__/server.test.ts index bfcdcae..8dcd270 100644 --- a/src/__tests__/server.test.ts +++ b/src/__tests__/server.test.ts @@ -1142,6 +1142,39 @@ describe('issue_comment handler', () => { expect(scanQueue.add).not.toHaveBeenCalled(); }); + it('confirmation comment says "Re-running scan..." when check run is in failure state', async () => { + (getLatestCheckRun as ReturnType).mockResolvedValue({ conclusion: 'failure' }); + + await processWebhookRequest(webhookRequest( + commentPayload(), { event: 'issue_comment' } + )); + + const [call] = (createPrComment as ReturnType).mock.calls as [{ body: string }][]; + expect(call[0].body).toContain('Re-running scan'); + }); + + it('confirmation comment does NOT say "Re-running scan..." when check run did not fail', async () => { + (getLatestCheckRun as ReturnType).mockResolvedValue({ conclusion: 'success' }); + + await processWebhookRequest(webhookRequest( + commentPayload(), { event: 'issue_comment' } + )); + + const [call] = (createPrComment as ReturnType).mock.calls as [{ body: string }][]; + expect(call[0].body).not.toContain('Re-running scan'); + }); + + it('confirmation comment does NOT say "Re-running scan..." when there is no check run', async () => { + (getLatestCheckRun as ReturnType).mockResolvedValue(null); + + await processWebhookRequest(webhookRequest( + commentPayload(), { event: 'issue_comment' } + )); + + const [call] = (createPrComment as ReturnType).mock.calls as [{ body: string }][]; + expect(call[0].body).not.toContain('Re-running scan'); + }); + it('does not store exceptions or enqueue scan when PR is already merged', async () => { (getPullRequest as ReturnType).mockResolvedValueOnce({ state: 'closed', diff --git a/src/adapters/claude.ts b/src/adapters/claude.ts index 20a0203..5f6bd27 100644 --- a/src/adapters/claude.ts +++ b/src/adapters/claude.ts @@ -218,6 +218,11 @@ async function scanBatchWithPrompt( tool_choice: { type: 'any' }, }); + if (response.stop_reason === 'max_tokens') { + console.error('[claude] API error during scan batch (prompt mode): response truncated — max_tokens reached, findings may be incomplete'); + return { error: true }; + } + return extractFindings(response.content); } catch (err) { console.error('[claude] API error during scan batch (prompt mode):', (err as Error).message ?? err); diff --git a/src/adapters/semgrep.ts b/src/adapters/semgrep.ts index 259802e..1598bf2 100644 --- a/src/adapters/semgrep.ts +++ b/src/adapters/semgrep.ts @@ -59,6 +59,7 @@ interface SemgrepRawResult { check_id: string; path: string; start?: { line: number }; + end?: { line: number }; extra?: { severity?: string; message?: string }; } @@ -69,12 +70,16 @@ const SEVERITY_MAP: Record = { }; function toFinding(result: SemgrepRawResult, workspacePath: string): SemgrepFinding { + const startLine = result.start?.line ?? 1; + const endLine = result.end?.line ?? startLine; return { - file: stripPrefix(result.path, workspacePath), - line: result.start?.line ?? 1, - severity: SEVERITY_MAP[result.extra?.severity ?? ''] ?? 'low', - message: result.extra?.message ?? 'Semgrep finding', - ruleId: result.check_id, - tool: 'semgrep', + file: stripPrefix(result.path, workspacePath), + line: startLine, + startLine, + endLine, + severity: SEVERITY_MAP[result.extra?.severity ?? ''] ?? 'low', + message: result.extra?.message ?? 'Semgrep finding', + ruleId: result.check_id, + tool: 'semgrep', }; } diff --git a/src/config-validator.ts b/src/config-validator.ts index a0d99fc..d349aa9 100644 --- a/src/config-validator.ts +++ b/src/config-validator.ts @@ -9,7 +9,7 @@ */ const KNOWN_REPO_KEYS = new Set(['mode', 'contextLines', 'timeoutMinutes', 'semgrep', 'trufflehog', 'claude', 'piAgent', 'notifications', 'labels', 'trigger', 'comment', 'exceptionApprovers']); -const KNOWN_GLOBAL_KEYS = new Set(['mode', 'contextLines', 'timeoutMinutes', 'notifications', 'labels', 'trigger', 'comment', 'exceptionApprovers']); +const KNOWN_GLOBAL_KEYS = new Set(['mode', 'contextLines', 'timeoutMinutes', 'semgrep', 'trufflehog', 'claude', 'piAgent', 'notifications', 'labels', 'trigger', 'comment', 'exceptionApprovers']); const VALID_MODES = new Set(['changed_files', 'diff_only']); const VALID_TRIGGER_ONS = new Set(['pull_request', 'workflow_run', 'workflow_job']); const VALID_CONCLUSIONS = new Set(['success', 'failure', 'neutral', 'cancelled', 'skipped', 'timed_out', 'action_required']); @@ -57,6 +57,10 @@ function validateGlobal(block: Record, ctx: string, errors: str } } validateScanMode(block, ctx, errors); + if (block['semgrep'] !== undefined) validateScanner(block['semgrep'], `${ctx}.semgrep`, errors); + if (block['trufflehog'] !== undefined) validateScanner(block['trufflehog'], `${ctx}.trufflehog`, errors); + if (block['claude'] !== undefined) validateClaude(block['claude'], `${ctx}.claude`, errors); + if (block['piAgent'] !== undefined) validatePiAgent(block['piAgent'], `${ctx}.piAgent`, errors); if (block['notifications'] !== undefined) validateNotifications(block['notifications'], `${ctx}.notifications`, errors); if (block['labels'] !== undefined) validateLabels(block['labels'], `${ctx}.labels`, errors); if (block['trigger'] !== undefined) validateTrigger(block['trigger'], `${ctx}.trigger`, errors); @@ -251,7 +255,7 @@ function validateComment(block: unknown, ctx: string, errors: string[]): void { function validateLabels(block: unknown, ctx: string, errors: string[]): void { if (typeof block !== 'object' || block === null) { errors.push(`${ctx}: must be an object`); return; } const b = block as Record; - for (const key of ['onFailure', 'removeOnFailure', 'onSuccess', 'removeOnSuccess', 'onException']) { + for (const key of ['onFailure', 'removeOnFailure', 'onSuccess', 'removeOnSuccess', 'onException', 'removeOnException']) { if (b[key] === undefined) continue; if (!Array.isArray(b[key])) errors.push(`${ctx}.${key}: must be an array`); diff --git a/src/config.ts b/src/config.ts index cf7438d..bb7fd75 100644 --- a/src/config.ts +++ b/src/config.ts @@ -95,10 +95,10 @@ export async function loadScanConfig({ owner, repo }: { owner: string; repo: str mode: repoOverrides.mode ?? globalConfig.mode ?? DEFAULT_CONFIG.mode, contextLines: repoOverrides.contextLines ?? globalConfig.contextLines ?? DEFAULT_CONFIG.contextLines, timeoutMinutes: repoOverrides.timeoutMinutes ?? globalConfig.timeoutMinutes ?? DEFAULT_CONFIG.timeoutMinutes, - semgrep: { ...DEFAULT_CONFIG.semgrep, ...(repoOverrides.semgrep ?? {}) }, - trufflehog: { ...DEFAULT_CONFIG.trufflehog, ...(repoOverrides.trufflehog ?? {}) }, - claude: { ...DEFAULT_CONFIG.claude, ...(repoOverrides.claude ?? {}) }, - piAgent: { ...DEFAULT_CONFIG.piAgent, ...(repoOverrides.piAgent ?? {}) }, + semgrep: { ...DEFAULT_CONFIG.semgrep, ...(globalConfig.semgrep ?? {}), ...(repoOverrides.semgrep ?? {}) }, + trufflehog:{ ...DEFAULT_CONFIG.trufflehog, ...(globalConfig.trufflehog ?? {}), ...(repoOverrides.trufflehog ?? {}) }, + claude: { ...DEFAULT_CONFIG.claude, ...(globalConfig.claude ?? {}), ...(repoOverrides.claude ?? {}) }, + piAgent: { ...DEFAULT_CONFIG.piAgent, ...(globalConfig.piAgent ?? {}), ...(repoOverrides.piAgent ?? {}) }, notifications: { ...globalNotifications, ...repoNotifications }, labels: { ...globalLabels, ...repoLabels }, trigger: { ...DEFAULT_CONFIG.trigger, ...globalTrigger, ...(repoOverrides.trigger ?? {}) }, diff --git a/src/server.ts b/src/server.ts index 4d19075..f6d4f1c 100644 --- a/src/server.ts +++ b/src/server.ts @@ -262,6 +262,7 @@ async function handleIssueComment(payload: Record): Promise<{ s }); const checkRunData = checkRun as { conclusion?: string } | null; + let scanEnqueued = false; if (checkRunData?.conclusion === 'failure') { const jobId = getJobId(repo.full_name, issueData.number, headSha); await enqueueScan({ @@ -276,16 +277,17 @@ async function handleIssueComment(payload: Record): Promise<{ s jobId, action: 'issue_comment', triggeredByException: true, - }).catch(err => console.error(`[server] Failed to enqueue scan: ${(err as Error).message}`)); + }).then(() => { scanEnqueued = true; }).catch(err => console.error(`[server] Failed to enqueue scan: ${(err as Error).message}`)); } const idList = parsed.ids.join(', '); + const confirmationSuffix = scanEnqueued ? ' Re-running scan...' : ''; await createPrComment({ installationId: (installation as { id: number }).id, owner: repo.owner.login, repo: repo.name, prNumber: issueData.number, - body: `✅ Exception recorded for ${idList} by @${commenter}: "${parsed.reason}". Re-running scan...`, + body: `✅ Exception recorded for ${idList} by @${commenter}: "${parsed.reason}".${confirmationSuffix}`, }).catch(err => console.error(`[server] Failed to post confirmation: ${(err as Error).message}`)); return { status: 200, body: 'Accepted' };