diff --git a/docs/setup/secure-agent-setup.md b/docs/setup/secure-agent-setup.md index e8b3ee870..40115e20f 100644 --- a/docs/setup/secure-agent-setup.md +++ b/docs/setup/secure-agent-setup.md @@ -40,6 +40,8 @@ - [Term canonicalization](#term-canonicalization) - [Scoping and fail-closed gates](#scoping-and-fail-closed-gates) - [Trade-offs and runtime characteristics](#trade-offs-and-runtime-characteristics) + - [Review findings curation mod (Claude Code plugin / hook)](#review-findings-curation-mod-claude-code-plugin--hook) + - [Unsandboxed filesystem writes (`$.fs.write`)](#unsandboxed-filesystem-writes-fswrite) - [Sandbox-error hint hook](#sandbox-error-hint-hook) - [Why install it](#why-install-it) - [Why install it user-scope, not project-scope](#why-install-it-user-scope-not-project-scope-1) @@ -2015,6 +2017,32 @@ static scope. guard against accidental leaks; inline environment variables passed to the command line (e.g. `MAGPIE_ALLOW_EMBARGO_TERMS=1 git commit ...`) are ignored by the hook. +## Review findings curation mod (Claude Code plugin / hook) + +For Claude Code harnesses using the `magpie-pr-management` plugin, the review findings interactive pane mod ([`plugins/magpie-pr-management/hooks/review-findings.ts`](../../plugins/magpie-pr-management/hooks/review-findings.ts)) provides an interactive terminal overview of findings during PR review triage and synchronizes maintainer curation decisions directly to disk beside the findings file. + +### Unsandboxed filesystem writes (`$.fs.write`) + +As established in [`docs/when-to-use-mods.md`](../when-to-use-mods.md), Claude Code mod hooks run outside the container sandbox with direct user privileges. +The review findings mod introduces the framework's first use of `$.fs.write` in a mod hook. + +- **Rationale**: The mod communicates maintainer curation decisions (action toggles, inline keep selections, and edited review bodies) to the downstream deterministic CLI tool (`pr-management code-review render`) running in subsequent skill steps. + Writing sidecars avoids spawning background daemons, opening IPC sockets, or modifying global environment state. +- **Write boundaries and unsandboxed execution**: Because Claude Code mod hooks execute outside the container sandbox with user privileges, `$.fs.write` targets the host filesystem directly. + The write destination is the directory of the `--findings` path intercepted from the preceding `code-review disposition` command (typically the session or scratch directory). + Sidecar creation is strictly restricted to sibling files (`.curated.json` and `.curated.keep`) beside an existing findings file. + Invalidation markers update existing sidecar paths when curation is cleaned up or when findings become empty. + To prevent symlink traversal outside the intended directory, writes refuse to follow paths that exist as symbolic links or non-regular files. + The fixed `.curated.*` suffixes and existence precheck ensure executable scripts, dotfiles, or unrelated system files are never overwritten. +- **Fail-open design and clean fallback**: Filesystem write operations are non-blocking and fail open. + If `$.fs.write` encounters permission errors, read-only volumes, or quota exhaustion, execution never halts or crashes. + Instead, the failure records `syncError` in state, displays an explicit warning in the interactive pane footer and `/magpie-findings status`, and invalidates partial sidecars on disk. + Downstream skill steps fall back cleanly to original findings and the interactive sequential text picker prompt. +- **Self-invalidation lifecycle**: Because the Claude Code mod filesystem API provides no `remove` or `unlink` methods, sidecars manage their lifecycle via deterministic self-invalidation rather than file deletion. + The curated JSON payload embeds the active PR number and the source findings file's SHA-256 digest in a metadata envelope. + When a write fails, when findings become empty, or when a disposition re-run detects a mismatched PR number, the hook writes explicit invalidation markers to disk. + Downstream CLI tools (`render`) validate the PR number and source hash directly before loading `.curated.json`, ensuring stale, orphaned, or invalidated curation files are refused or bypassed. + ## Sandbox-error hint hook Companion to the *Sandbox-bypass visibility hook* above โ€” a diff --git a/plugins/magpie-pr-management/hooks/review-findings.test.ts b/plugins/magpie-pr-management/hooks/review-findings.test.ts index aad6774eb..caeaa723a 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.test.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.test.ts @@ -20,10 +20,14 @@ import { describe, test } from 'claude-code/testing'; import { calculateSummary, + cleanCuratedFindingsFiles, + computeFindingsHash, cycleFindingAction, + findMatchingExistingFinding, formatActionIndicator, formatFindingDescription, formatKeepAnswer, + getCuratedFindingsPaths, extractFindingsPathFromCommand, extractPrNumberFromCommand, formatSeverityBadge, @@ -32,12 +36,14 @@ import { handleFindingsCommand, handleUiPress, isAnchored, + isCuratedSidecarValid, normalizeAction, normalizeFilePath, normalizeLine, normalizeSeverity, normalizeSource, parseFindings, + parseFindingTokens, resolveFindingIndex, resolveRelativePath, register, @@ -47,9 +53,11 @@ import { serializeFindingsForRender, setBulkActionInState, setReviewFindingsState, + syncCuratedStateToDisk, toggleFindingActionInState, truncateFilePath, truncateText, + updateAndSyncReviewFindingsState, updateFindingInState, updateSelectedIndexInState, } from './review-findings.ts'; @@ -71,6 +79,20 @@ function assertStrictEqual(actual: any, expected: any, message?: string): void { function createMockEngine(overrides: any = {}): any { const store = new Map(); + const files = new Map(); + const defaultFs = { + read: async (path: string) => { + if (files.has(path)) return files.get(path); + throw new Error(`ENOENT: ${path}`); + }, + write: async (path: string, content: string) => { + files.set(path, content); + }, + exists: async (path: string) => { + return files.has(path); + }, + _files: files, + }; return { state: { get: (atom: any) => { @@ -92,6 +114,10 @@ function createMockEngine(overrides: any = {}): any { invalidate: () => {}, }, ...overrides, + fs: { + ...defaultFs, + ...(overrides.fs || {}), + }, }; } @@ -1939,4 +1965,777 @@ End of observations. } }); }); + + describe('PR-4 Curated findings bridge and general observation filtering', () => { + test('getCuratedFindingsPaths derives .curated.json and .curated.keep paths', () => { + const pathsJson = getCuratedFindingsPaths('saved/findings-1887.json'); + assertStrictEqual(pathsJson.curatedJsonPath, 'saved/findings-1887.curated.json'); + assertStrictEqual(pathsJson.curatedKeepPath, 'saved/findings-1887.curated.keep'); + + const pathsNoExt = getCuratedFindingsPaths('saved/findings-1887'); + assertStrictEqual(pathsNoExt.curatedJsonPath, 'saved/findings-1887.curated.json'); + assertStrictEqual(pathsNoExt.curatedKeepPath, 'saved/findings-1887.curated.keep'); + }); + + test('serializeFindingsForRender omits skipped unanchored findings while preserving anchored findings', () => { + const findings: ReviewFinding[] = [ + { id: 'inline-1', file: 'a.py', line: 10, severity: 'minor', body: 'Inline 1', action: 'accept' }, + { id: 'gen-1', file: 'README.md', line: null, severity: 'major', body: 'General 1', action: 'skip' }, + { id: 'inline-2', file: 'b.py', line: 20, severity: 'nit', body: 'Inline 2', action: 'skip' }, + { id: 'gen-2', file: 'CONTRIBUTING.md', line: null, severity: 'minor', body: 'General 2', action: 'accept' }, + ]; + + const serialized = serializeFindingsForRender(findings); + const parsed = JSON.parse(serialized); + + // Anchored findings: inline-1 (accept) and inline-2 (skip) are both in the file + // (their posting/folding is controlled by render --keep) + // Unanchored findings: gen-1 (skip) is omitted, gen-2 (accept) is preserved + assertStrictEqual(parsed.length, 3); + assertStrictEqual(parsed[0].file, 'a.py'); + assertStrictEqual(parsed[1].file, 'b.py'); + assertStrictEqual(parsed[2].file, 'CONTRIBUTING.md'); + }); + + test('syncCuratedStateToDisk writes both curated JSON and keep answer to disk', async () => { + const test$ = createMockEngine(); + const state: ReviewFindingsState = { + prNumber: 1887, + findingsPath: '/work/saved/findings-1887.json', + findings: [ + { id: 'f1', file: 'a.py', line: 10, severity: 'minor', body: 'Note 1', action: 'accept' }, + { id: 'f2', file: 'b.py', line: 20, severity: 'nit', body: 'Note 2', action: 'edit' }, + ], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + test$.fs._files.set('/work/saved/findings-1887.json', '[]'); + const result = await syncCuratedStateToDisk(test$, state); + assert(result !== null); + assertStrictEqual(result?.jsonPath, '/work/saved/findings-1887.curated.json'); + assertStrictEqual(result?.keepPath, '/work/saved/findings-1887.curated.keep'); + + const writtenJson = await test$.fs.read('/work/saved/findings-1887.curated.json'); + const parsed = JSON.parse(writtenJson); + assertStrictEqual(parsed.schema_version, 1); + assertStrictEqual(parsed.pr, 1887); + assertStrictEqual(parsed.findings.length, 2); + + const writtenKeep = await test$.fs.read('/work/saved/findings-1887.curated.keep'); + assertStrictEqual(writtenKeep, 'ALL'); + }); + + test('handleBashToolCall sets findingsPath and hydrates state, but does NOT write curated files upon initial disposition', async () => { + const sampleFindings = JSON.stringify([ + { file: 'main.py', line: 42, severity: 'minor', explanation: 'Needs fix' }, + ]); + + const test$ = createMockEngine({ + session: { + root: async () => '/work/repo', + cwd: async () => '/work/repo', + }, + }); + test$.fs._files.set('/work/repo/saved/findings-999.json', sampleFindings); + + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-999.json --pr 999' }, + () => {} + ); + + const state = await getReviewFindingsState(test$); + assert(state !== null); + assertStrictEqual(state?.findingsPath, '/work/repo/saved/findings-999.json'); + + // Verify curated files were NOT written upfront on initial disposition + assert(!test$.fs._files.has('/work/repo/saved/findings-999.curated.json')); + assert(!test$.fs._files.has('/work/repo/saved/findings-999.curated.keep')); + }); + + test('renderFindingsPane button onPress callbacks update state and sync curated files to disk', async () => { + const mockButtonCallbacks = new Map Promise>(); + const MockButton = (props: any) => { + if (props?.key && typeof props?.onPress === 'function') { + mockButtonCallbacks.set(props.key, props.onPress); + } + return { type: 'button', ...props }; + }; + + const test$ = createMockEngine({ + ui: { + resolve: () => ({ + Box: (props: any) => props, + Text: (props: any) => props, + Button: MockButton, + }), + }, + }); + + const state: ReviewFindingsState = { + prNumber: 1887, + findingsPath: '/work/saved/findings-1887.json', + findings: [ + { id: 'f1', file: 'a.py', line: 10, severity: 'minor', body: 'Inline 1', action: 'accept' }, + { id: 'f2', file: 'b.py', line: 20, severity: 'nit', body: 'Inline 2', action: 'accept' }, + { id: 'g1', file: 'README.md', line: null, severity: 'minor', body: 'General 1', action: 'accept' }, + ], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + test$.fs._files.set('/work/saved/findings-1887.json', '[]'); + await setReviewFindingsState(test$, state); + + renderFindingsPane(test$, {}, state); + + // Trigger [None] button onPress + const skipAllCb = mockButtonCallbacks.get('skip-all'); + assert(typeof skipAllCb === 'function', 'skip-all button callback must be registered'); + await skipAllCb!(); + + let curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[0].action, 'skip'); + assertStrictEqual(curState?.findings[1].action, 'skip'); + + // Verify disk sync happened from button onPress + const writtenKeep = await test$.fs.read('/work/saved/findings-1887.curated.keep'); + assertStrictEqual(writtenKeep, 'NONE'); + + // Trigger row toggle onPress + renderFindingsPane(test$, {}, curState!); + const toggleCb = mockButtonCallbacks.get('toggle-1'); + assert(typeof toggleCb === 'function', 'toggle-1 button callback must be registered'); + await toggleCb!(); + + curState = await getReviewFindingsState(test$); + // 'skip' toggles to 'edit' + assertStrictEqual(curState?.findings[0].action, 'edit'); + + const updatedKeep = await test$.fs.read('/work/saved/findings-1887.curated.keep'); + assertStrictEqual(updatedKeep, '1'); + }); + + test('handleBashToolCall preserves existing maintainer curation across disposition re-runs', async () => { + const initialFindings = JSON.stringify([ + { file: 'auth.py', line: 15, severity: 'major', explanation: 'Original note 1', rule: 'sec-rule-1' }, + { file: 'auth.py', line: 55, severity: 'minor', explanation: 'Original note 2', rule: 'style-rule-2' }, + ]); + + const test$ = createMockEngine({ + session: { + root: async () => '/work/repo', + cwd: async () => '/work/repo', + }, + }); + test$.fs._files.set('/work/repo/saved/findings-123.json', initialFindings); + + // 1. Initial disposition runs + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-123.json --pr 123' }, + () => {} + ); + + // 2. Maintainer curates: drops finding 1 and edits finding 2 + await handleFindingsCommand(test$, { args: 'drop 1' }); + await handleFindingsCommand(test$, { args: 'edit 2 Sanitized custom explanation' }); + + let state = await getReviewFindingsState(test$); + assertStrictEqual(state?.findings[0].action, 'skip'); + assertStrictEqual(state?.findings[1].action, 'edit'); + assertStrictEqual(state?.findings[1].body, 'Sanitized custom explanation'); + + // 3. Disposition re-runs (e.g. after maintainer [E]dits or checks CI) + const updatedFindings = JSON.stringify([ + { file: 'auth.py', line: 15, severity: 'major', explanation: 'Fresh parser note 1', rule: 'sec-rule-1' }, + { file: 'auth.py', line: 55, severity: 'minor', explanation: 'Fresh parser note 2', rule: 'style-rule-2' }, + { file: 'auth.py', line: 99, severity: 'nit', explanation: 'Brand new finding', rule: 'nit-rule-3' }, + ]); + test$.fs._files.set('/work/repo/saved/findings-123.json', updatedFindings); + + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-123.json --pr 123' }, + () => {} + ); + + state = await getReviewFindingsState(test$); + assert(state !== null); + assertStrictEqual(state?.findings.length, 3); + // Finding 1 preserved as 'skip' + assertStrictEqual(state?.findings[0].action, 'skip'); + // Finding 2 preserved as 'edit' with custom body + assertStrictEqual(state?.findings[1].action, 'edit'); + assertStrictEqual(state?.findings[1].body, 'Sanitized custom explanation'); + // Finding 3 is new, defaults to 'accept' + assertStrictEqual(state?.findings[2].action, 'accept'); + }); + + test('disposition cleans up orphaned curated files when findings become empty', async () => { + const test$ = createMockEngine({ + session: { + root: async () => '/work/repo', + cwd: async () => '/work/repo', + }, + }); + test$.fs._files.set('/work/repo/saved/findings-empty.json', '[]'); + test$.fs._files.set('/work/repo/saved/findings-empty.curated.json', '[{"file":"old.py"}]'); + test$.fs._files.set('/work/repo/saved/findings-empty.curated.keep', '1'); + + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-empty.json --pr 404' }, + () => {} + ); + + const state = await getReviewFindingsState(test$); + assertStrictEqual(state, null); + + // Verify orphaned sidecars were marked as invalidated on disk + const jsonContent = await test$.fs.read('/work/repo/saved/findings-empty.curated.json'); + const parsed = JSON.parse(jsonContent); + assertStrictEqual(parsed.invalidated, true); + assertStrictEqual(parsed.findings.length, 0); + + const keepContent = await test$.fs.read('/work/repo/saved/findings-empty.curated.keep'); + assertStrictEqual(keepContent, 'INVALIDATED'); + }); + + test('strict token validation rejects letters and decouples gN in keep and drop commands', async () => { + const test$ = createMockEngine(); + const state: ReviewFindingsState = { + prNumber: 2005, + findingsPath: '/work/saved/findings-2005.json', + findings: [ + { id: 'f1', file: 'a.py', line: 10, severity: 'minor', body: 'Inline 1', action: 'accept' }, + { id: 'f2', file: 'b.py', line: 20, severity: 'major', body: 'Inline 2', action: 'accept' }, + { id: 'g1', file: 'README.md', line: null, severity: 'minor', body: 'General 1', action: 'accept' }, + ], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + test$.fs._files.set('/work/saved/findings-1887.json', '[]'); + await setReviewFindingsState(test$, state); + + // 1. keep abc must fail validation and NOT touch state + const badKeepRes = await handleFindingsCommand(test$, { args: 'keep abc' }); + assert(badKeepRes.text.startsWith('Error: Invalid token "abc"')); + let curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[0].action, 'accept'); + assertStrictEqual(curState?.findings[1].action, 'accept'); + + // 2. drop abc must fail validation and NOT touch state + const badDropRes = await handleFindingsCommand(test$, { args: 'drop abc' }); + assert(badDropRes.text.startsWith('Error: Invalid token "abc"')); + + // 3. keep g1 must ONLY modify general observations, leaving anchored findings untouched + await handleFindingsCommand(test$, { args: 'keep g1' }); + curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[0].action, 'accept', 'Anchored 1 must remain accept'); + assertStrictEqual(curState?.findings[1].action, 'accept', 'Anchored 2 must remain accept'); + assertStrictEqual(curState?.findings[2].action, 'accept', 'General 1 must remain accept'); + + // 4. keep 1 must ONLY modify anchored findings, leaving general observations untouched + await handleFindingsCommand(test$, { args: 'keep 1' }); + curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[0].action, 'accept', 'Anchored 1 kept'); + assertStrictEqual(curState?.findings[1].action, 'skip', 'Anchored 2 skipped'); + assertStrictEqual(curState?.findings[2].action, 'accept', 'General 1 must remain accept'); + + // 5. drop g1 must mark general observation as skip without touching anchored + await handleFindingsCommand(test$, { args: 'drop g1' }); + curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[0].action, 'accept'); + assertStrictEqual(curState?.findings[1].action, 'skip'); + assertStrictEqual(curState?.findings[2].action, 'skip'); + + // 6. keep 1, g1 updates both + await handleFindingsCommand(test$, { args: 'keep 1, g1' }); + curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[0].action, 'accept'); + assertStrictEqual(curState?.findings[1].action, 'skip'); + assertStrictEqual(curState?.findings[2].action, 'accept'); + }); + + test('sync error records syncError on state and displays warning in pane and status', async () => { + const writtenFiles = new Set(); + let warningNodeFound = false; + const MockText = (props: any) => { + if (props?.children && props.children[0]?.includes('Warning: Curation sync to disk failed')) { + warningNodeFound = true; + } + return { type: 'text', ...props }; + }; + + const writtenFileContents = new Map(); + const failingTest$ = createMockEngine({ + fs: { + exists: async () => true, + read: async () => '[]', + write: async (path: string, content: string) => { + if (path.endsWith('.keep')) { + throw new Error('EACCES: permission denied'); + } + writtenFileContents.set(path, content); + }, + }, + ui: { + resolve: () => ({ + Box: (props: any) => props, + Text: MockText, + }), + }, + }); + + const state: ReviewFindingsState = { + prNumber: 2005, + findingsPath: '/work/readonly/findings.json', + findings: [ + { id: 'f1', file: 'a.py', line: 10, severity: 'minor', body: 'Inline 1', action: 'accept' }, + ], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + await setReviewFindingsState(failingTest$, state); + + // Perform curation action that triggers write + const res = await handleFindingsCommand(failingTest$, { args: 'none' }); + assert(res.text.includes('Warning: sync to disk failed: EACCES: permission denied')); + + const curState = await getReviewFindingsState(failingTest$); + assert(curState?.syncError !== null && curState?.syncError !== undefined); + assert(curState?.syncError.includes('EACCES: permission denied')); + + // Verify partial file was marked as invalidated on disk so Step 7b falls back to original findings + const jsonContent = writtenFileContents.get('/work/readonly/findings.curated.json'); + assert(jsonContent !== undefined); + const parsed = JSON.parse(jsonContent!); + assertStrictEqual(parsed.invalidated, true); + assertStrictEqual(parsed.findings.length, 0); + + // Verify warning renders in /magpie-findings status + const statusRes = await handleFindingsCommand(failingTest$, { args: 'status' }); + assert(statusRes.text.includes('Warning: Curation sync to disk failed')); + + // Verify warning renders in pane Box + renderFindingsPane(failingTest$, {}, curState!); + assert(warningNodeFound, 'Warning node must be rendered in pane when syncError is set'); + }); + + test('handleUiPress and handleFindingsCommand update curated files on disk', async () => { + const test$ = createMockEngine(); + const state: ReviewFindingsState = { + prNumber: 1887, + findingsPath: '/work/saved/findings-1887.json', + findings: [ + { id: 'f1', file: 'a.py', line: 10, severity: 'minor', body: 'Inline 1', action: 'accept' }, + { id: 'g1', file: 'README.md', line: null, severity: 'minor', body: 'General 1', action: 'accept' }, + ], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + test$.fs._files.set('/work/saved/findings-1887.json', '[]'); + await setReviewFindingsState(test$, state); + await syncCuratedStateToDisk(test$, state); + + // Button click to toggle general finding to skip + await handleUiPress(test$, { element: 'toggle-general-1' }, () => {}); + + let curatedJson = await test$.fs.read('/work/saved/findings-1887.curated.json'); + let parsed = JSON.parse(curatedJson); + // General finding g1 is skipped, so omitted from curated JSON + assertStrictEqual(parsed.findings.length, 1); + assertStrictEqual(parsed.findings[0].file, 'a.py'); + + // Slash command to edit inline 1 + await handleFindingsCommand(test$, { args: 'edit 1 Maintainer customized review note' }); + + curatedJson = await test$.fs.read('/work/saved/findings-1887.curated.json'); + parsed = JSON.parse(curatedJson); + assertStrictEqual(parsed.findings[0].explanation, 'Maintainer customized review note'); + + // Drop command with general identifier + await handleFindingsCommand(test$, { args: 'drop g1' }); + const curState = await getReviewFindingsState(test$); + assertStrictEqual(curState?.findings[1].action, 'skip'); + + // Keep command with general identifier: edited finding 1 retains 'edit', unanchored g1 becomes 'accept' + await handleFindingsCommand(test$, { args: 'keep 1, g1' }); + const keptState = await getReviewFindingsState(test$); + assertStrictEqual(keptState?.findings[0].action, 'edit'); + assertStrictEqual(keptState?.findings[1].action, 'accept'); + }); + + test('serializeFindingsForRender on edit deletes base.comment and overrides explanation', () => { + const rawFindingWithComment = { + file: 'api.py', + line: 42, + severity: 'minor', + comment: 'Raw unedited inline comment from detector', + explanation: 'Raw explanation', + suggestion: '-foo()\n+bar()', + }; + + const findings: ReviewFinding[] = [ + { + id: 'f1', + file: 'api.py', + line: 42, + severity: 'minor', + body: 'Maintainer revised human comment', + action: 'edit', + raw: rawFindingWithComment, + }, + ]; + + const serialized = serializeFindingsForRender(findings); + const parsed = JSON.parse(serialized); + assertStrictEqual(parsed.length, 1); + // base.comment must be deleted on edit so render does not fall back to unedited raw comment + assertStrictEqual(parsed[0].comment, undefined); + assertStrictEqual(parsed[0].suggestion, undefined); + assertStrictEqual(parsed[0].explanation, 'Maintainer revised human comment'); + + // Envelope mode + const envelopeSerialized = serializeFindingsForRender(findings, { + pr: 2005, + sourceHash: 'testhash123', + }); + const envelopeParsed = JSON.parse(envelopeSerialized); + assertStrictEqual(envelopeParsed.schema_version, 1); + assertStrictEqual(envelopeParsed.pr, 2005); + assertStrictEqual(envelopeParsed.source_sha256, 'testhash123'); + assertStrictEqual(envelopeParsed.findings[0].comment, undefined); + assertStrictEqual(envelopeParsed.findings[0].explanation, 'Maintainer revised human comment'); + }); + + test('isCuratedSidecarValid verifies schema, PR number, hash, and invalidation marker', () => { + const sourceContent = JSON.stringify([{ file: 'main.py', line: 1 }]); + const sourceHash = computeFindingsHash(sourceContent); + + const validPayload = JSON.stringify({ + schema_version: 1, + pr: 2005, + source_sha256: sourceHash, + findings: [{ file: 'main.py', line: 1 }], + }); + assert(isCuratedSidecarValid(validPayload, 2005, sourceContent)); + + // Mismatched PR number + assert(!isCuratedSidecarValid(validPayload, 999, sourceContent)); + + // Mismatched source content / hash + assert(!isCuratedSidecarValid(validPayload, 2005, 'different source content')); + + // Explicitly invalidated marker + const invalidatedPayload = JSON.stringify({ + schema_version: 1, + pr: null, + source_sha256: null, + invalidated: true, + findings: [], + }); + assert(!isCuratedSidecarValid(invalidatedPayload, 2005, sourceContent)); + + // Null, empty, or malformed JSON + assert(!isCuratedSidecarValid(null, 2005, sourceContent)); + assert(!isCuratedSidecarValid('', 2005, sourceContent)); + assert(!isCuratedSidecarValid('not json', 2005, sourceContent)); + }); + + test('cleanCuratedFindingsFiles does not create sidecar files when none exist on disk', async () => { + const test$ = createMockEngine(); + // Neither findings file nor curated sidecars exist in test$.fs._files + await cleanCuratedFindingsFiles(test$, '/work/nonexistent/findings.json'); + assert(!test$.fs._files.has('/work/nonexistent/findings.curated.json')); + assert(!test$.fs._files.has('/work/nonexistent/findings.curated.keep')); + }); + + test('handleBashToolCall re-syncs curated sidecars on disposition re-run after edit', async () => { + const test$ = createMockEngine({ + session: { + root: async () => '/work/repo', + cwd: async () => '/work/repo', + }, + }); + + const initialSource = JSON.stringify([ + { file: 'app.py', line: 10, severity: 'minor', explanation: 'Initial note 1' }, + { file: 'app.py', line: 20, severity: 'minor', explanation: 'Initial note 2' }, + ]); + test$.fs._files.set('/work/repo/saved/findings-run.json', initialSource); + + // 1. Initial disposition (does NOT write sidecars) + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-run.json --pr 100' }, + () => {} + ); + assert(!test$.fs._files.has('/work/repo/saved/findings-run.curated.json')); + + // 2. Maintainer curates (which writes sidecars to disk) + await handleFindingsCommand(test$, { args: 'drop 1' }); + assert(test$.fs._files.has('/work/repo/saved/findings-run.curated.json')); + let curatedOnDisk = JSON.parse(await test$.fs.read('/work/repo/saved/findings-run.curated.json')); + assertStrictEqual(curatedOnDisk.source_sha256, computeFindingsHash(initialSource)); + + // 3. Maintainer edits raw findings file (e.g. via [E]dit) and re-runs disposition + const editedSource = JSON.stringify([ + { file: 'app.py', line: 10, severity: 'minor', explanation: 'Rewritten note 1' }, + { file: 'app.py', line: 20, severity: 'minor', explanation: 'Rewritten note 2' }, + ]); + test$.fs._files.set('/work/repo/saved/findings-run.json', editedSource); + + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-run.json --pr 100' }, + () => {} + ); + + // Verify sidecars on disk were re-synced with editedSource's new hash and curation preserved + curatedOnDisk = JSON.parse(await test$.fs.read('/work/repo/saved/findings-run.curated.json')); + assertStrictEqual(curatedOnDisk.pr, 100); + assertStrictEqual(curatedOnDisk.source_sha256, computeFindingsHash(editedSource)); + assertStrictEqual(curatedOnDisk.findings.length, 2); + }); + + test('handleBashToolCall invalidates curated sidecars on PR mismatch when path is reused', async () => { + const test$ = createMockEngine({ + session: { + root: async () => '/work/repo', + cwd: async () => '/work/repo', + }, + }); + + // Sidecars pre-exist for PR 100 + test$.fs._files.set( + '/work/repo/saved/findings-shared.json', + JSON.stringify([{ file: 'a.py', line: 5, severity: 'minor', explanation: 'A' }]) + ); + test$.fs._files.set( + '/work/repo/saved/findings-shared.curated.json', + JSON.stringify({ schema_version: 1, pr: 100, source_sha256: 'old', findings: [] }) + ); + test$.fs._files.set('/work/repo/saved/findings-shared.curated.keep', '1'); + + // Re-running disposition on a different PR (PR 200) with the same shared findings path + await handleBashToolCall( + test$, + { command: 'uv run pr-management code-review disposition --findings saved/findings-shared.json --pr 200' }, + () => {} + ); + + // Existing sidecar on disk must be marked as invalidated + const jsonContent = await test$.fs.read('/work/repo/saved/findings-shared.curated.json'); + const parsed = JSON.parse(jsonContent); + assertStrictEqual(parsed.invalidated, true); + + const keepContent = await test$.fs.read('/work/repo/saved/findings-shared.curated.keep'); + assertStrictEqual(keepContent, 'INVALIDATED'); + }); + + test('syncCuratedStateToDisk refuses to write sidecars when source file does not exist', async () => { + const test$ = createMockEngine(); + const state: ReviewFindingsState = { + prNumber: 300, + findingsPath: '/work/nonexistent/findings.json', + findings: [{ id: 'f1', file: 'a.py', line: 1, severity: 'minor', body: 'A', action: 'accept' }], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + const result = await syncCuratedStateToDisk(test$, state); + assertStrictEqual(result.success, false); + assert(result.error?.includes('Source findings file does not exist')); + }); + + test('computeFindingsHash matches known-answer SHA-256 vectors', () => { + // 1. Empty string NIST vector + assertStrictEqual( + computeFindingsHash(''), + 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855' + ); + + // 2. Short ASCII NIST vector 'abc' + assertStrictEqual( + computeFindingsHash('abc'), + 'ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad' + ); + + // 3. Multi-block input (56-byte NIST vector) + assertStrictEqual( + computeFindingsHash('abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq'), + '248d6a61d20638b8e5c026930c3e6039a33ce45964ff2167f6ecedd419db06c1' + ); + + // 4. Non-ASCII UTF-8 string with multilingual characters and emoji + assertStrictEqual( + computeFindingsHash('cafรฉ ๐Ÿš€ ใ“ใ‚“ใซใกใฏ'), + '7b935a45fb0ed2a99402a41d1786be3becb6f4b9a431714a62905399f142a049' + ); + }); + + test('keep and All retain edit action on edited findings', async () => { + const test$ = createMockEngine(); + const state: ReviewFindingsState = { + prNumber: 10, + findingsPath: '/work/saved/findings.json', + findings: [ + { id: 'f1', file: 'a.py', line: 1, severity: 'minor', body: 'Original 1', action: 'accept' }, + { id: 'f2', file: 'a.py', line: 2, severity: 'minor', body: 'Maintainer custom note', action: 'edit' }, + { id: 'f3', file: 'a.py', line: 3, severity: 'minor', body: 'Original 3', action: 'accept' }, + ], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + test$.fs._files.set('/work/saved/findings.json', '[]'); + await setReviewFindingsState(test$, state); + + // 1. Run keep 1,2: f2 must stay 'edit', f1 'accept', f3 'skip' + await handleFindingsCommand(test$, { args: 'keep 1,2' }); + let cur = await getReviewFindingsState(test$); + assertStrictEqual(cur?.findings[0].action, 'accept'); + assertStrictEqual(cur?.findings[1].action, 'edit'); + assertStrictEqual(cur?.findings[2].action, 'skip'); + + // 2. Press All: f2 must stay 'edit', f3 becomes 'accept' + await handleFindingsCommand(test$, { args: 'all' }); + cur = await getReviewFindingsState(test$); + assertStrictEqual(cur?.findings[0].action, 'accept'); + assertStrictEqual(cur?.findings[1].action, 'edit'); + assertStrictEqual(cur?.findings[2].action, 'accept'); + + // 3. Serialized render output preserves maintainer note + const jsonStr = await test$.fs.read('/work/saved/findings.curated.json'); + const parsed = JSON.parse(jsonStr); + assertStrictEqual(parsed.findings[1].explanation, 'Maintainer custom note'); + }); + + test('serializeFindingsForRender on edit drops suggestion_block and fixup', () => { + const rawWithSuggestions = { + file: 'core.py', + line: 5, + severity: 'minor', + explanation: 'Initial text', + suggestion: '-old\n+new', + comment: 'Inline comment', + suggestion_block: '```suggestion\nnew_code()\n```', + fixup: 'new_code()', + }; + + const finding: ReviewFinding = { + id: 'f1', + file: 'core.py', + line: 5, + severity: 'minor', + body: 'Maintainer customized explanation', + action: 'edit', + raw: rawWithSuggestions, + }; + + const out = serializeFindingsForRender([finding]); + const parsed = JSON.parse(out); + const serialized = Array.isArray(parsed) ? parsed[0] : parsed.findings[0]; + + assertStrictEqual(serialized.explanation, 'Maintainer customized explanation'); + assertStrictEqual(serialized.suggestion, undefined); + assertStrictEqual(serialized.comment, undefined); + assertStrictEqual(serialized.suggestion_block, undefined); + assertStrictEqual(serialized.fixup, undefined); + }); + + test('findMatchingExistingFinding does not inherit edit when finding is replaced and consumes each match once', async () => { + const existing: ReviewFinding[] = [ + { + id: 'old-1', + file: 'scheduler/job.py', + line: 10, + severity: 'minor', + rule: 'style-guideline', + body: 'Customized edit note on style', + action: 'edit', + }, + ]; + + // Replaced finding at the same file and line, but different rule and body + const replacedFinding: ReviewFinding = { + id: 'new-1', + file: 'scheduler/job.py', + line: 10, + severity: 'blocking', + rule: 'security-vulnerability', + body: 'Direct security issue', + action: 'accept', + }; + + const consumed = new Set(); + const match = findMatchingExistingFinding(replacedFinding, existing, consumed); + // Must NOT match because rule and body differ + assertStrictEqual(match, undefined); + assertStrictEqual(consumed.size, 0); + + // Verify single consumption: when two incoming findings match the same existing finding, only the first consumes it + const sameFinding1: ReviewFinding = { + id: 'new-a', + file: 'scheduler/job.py', + line: 10, + severity: 'minor', + rule: 'style-guideline', + body: 'Customized edit note on style', + action: 'accept', + }; + const sameFinding2: ReviewFinding = { + id: 'new-b', + file: 'scheduler/job.py', + line: 10, + severity: 'minor', + rule: 'style-guideline', + body: 'Customized edit note on style', + action: 'accept', + }; + + const match1 = findMatchingExistingFinding(sameFinding1, existing, consumed); + assert(match1 !== undefined); + assertStrictEqual(match1?.action, 'edit'); + assertStrictEqual(consumed.size, 1); + + const match2 = findMatchingExistingFinding(sameFinding2, existing, consumed); + // Already consumed, so second finding gets undefined + assertStrictEqual(match2, undefined); + }); + + test('syncCuratedStateToDisk refuses to write to symlinks or non-regular files', async () => { + const test$ = createMockEngine({ + fs: { + stat: async (p: string) => { + if (p.endsWith('.curated.json')) { + return { isFile: () => false, isSymbolicLink: () => true }; + } + return { isFile: () => true, isSymbolicLink: () => false }; + }, + }, + }); + + test$.fs._files.set('/work/saved/findings.json', '[]'); + test$.fs._files.set('/work/saved/findings.curated.json', 'symlink_target'); + + const state: ReviewFindingsState = { + prNumber: 42, + findingsPath: '/work/saved/findings.json', + findings: [{ id: 'f1', file: 'a.py', line: 1, severity: 'minor', body: 'A', action: 'accept' }], + summary: calculateSummary([]), + selectedIndex: 0, + }; + + const res = await syncCuratedStateToDisk(test$, state); + assertStrictEqual(res.success, false); + assert(res.error?.includes('Refusing to write to symlink')); + }); + }); }); diff --git a/plugins/magpie-pr-management/hooks/review-findings.ts b/plugins/magpie-pr-management/hooks/review-findings.ts index 5254ad903..467e23fba 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.ts @@ -28,6 +28,135 @@ import type { ReviewFindingsSummary, } from './types.ts'; +export interface CuratedFindingsPayload { + schema_version: 1; + pr?: number | null; + source_sha256?: string | null; + invalidated?: boolean; + findings: Record[]; +} + +/** + * Computes deterministic SHA-256 hex digest of UTF-8 string in pure JS/TS + * without external module imports, adhering to mod import boundaries. + */ +export function computeFindingsHash(content: string): string { + const ascii = unescape(encodeURIComponent(content)); + const mathPow = Math.pow; + const maxWord = mathPow(2, 32); + const words: number[] = []; + const asciiBitLength = ascii.length * 8; + const hash: number[] = []; + const k: number[] = []; + let primeCounter = 0; + const isComposite: Record = {}; + + for (let candidate = 2; primeCounter < 64; candidate += 1) { + if (!isComposite[candidate]) { + for (let i = 0; i < 313; i += candidate) { + isComposite[i] = candidate; + } + hash[primeCounter] = (mathPow(candidate, 0.5) * maxWord) | 0; + k[primeCounter] = (mathPow(candidate, 1 / 3) * maxWord) | 0; + primeCounter += 1; + } + } + + let padded = ascii + '\x80'; + while (padded.length % 64 !== 56) { + padded += '\x00'; + } + + for (let i = 0; i < padded.length; i += 1) { + const code = padded.charCodeAt(i); + words[i >> 2] |= code << ((3 - (i % 4)) * 8); + } + + words[words.length] = (asciiBitLength / maxWord) | 0; + words[words.length] = asciiBitLength; + + for (let j = 0; j < words.length; ) { + const w = words.slice(j, (j += 16)); + const oldHash = [...hash]; + for (let i = 0; i < 64; i += 1) { + const w15 = w[i - 15]; + const w2 = w[i - 2]; + const a = hash[0]; + const e = hash[4]; + const s1 = ((e >>> 6) | (e << 26)) ^ ((e >>> 11) | (e << 21)) ^ ((e >>> 25) | (e << 7)); + const ch = (e & hash[5]) ^ (~e & hash[6]); + const s0 = ((a >>> 2) | (a << 30)) ^ ((a >>> 13) | (a << 19)) ^ ((a >>> 22) | (a << 10)); + const maj = (a & hash[1]) ^ (a & hash[2]) ^ (hash[1] & hash[2]); + + const wi = + i < 16 + ? w[i] + : ((w[i - 16] + + (((w15 >>> 7) | (w15 << 25)) ^ ((w15 >>> 18) | (w15 << 14)) ^ (w15 >>> 3)) + + w[i - 7] + + (((w2 >>> 17) | (w2 << 15)) ^ ((w2 >>> 19) | (w2 << 13)) ^ (w2 >>> 10))) | + 0); + w[i] = wi; + + const temp1 = (hash[7] + s1 + ch + k[i] + wi) | 0; + const temp2 = (s0 + maj) | 0; + + hash[7] = hash[6]; + hash[6] = hash[5]; + hash[5] = hash[4]; + hash[4] = (hash[3] + temp1) | 0; + hash[3] = hash[2]; + hash[2] = hash[1]; + hash[1] = hash[0]; + hash[0] = (temp1 + temp2) | 0; + } + for (let i = 0; i < 8; i += 1) { + hash[i] = (hash[i] + oldHash[i]) | 0; + } + } + + let result = ''; + for (let i = 0; i < 8; i += 1) { + for (let j = 3; j >= 0; j -= 1) { + const b = (hash[i] >>> (j * 8)) & 255; + result += (b < 16 ? '0' : '') + b.toString(16); + } + } + return result; +} + +/** + * Validates whether a curated findings sidecar JSON content matches the current + * active PR number and source findings file content without being invalidated. + */ +export function isCuratedSidecarValid( + curatedJsonContent: string | null | undefined, + prNumber: number | null | undefined, + sourceFileContent: string +): boolean { + if (!curatedJsonContent) return false; + try { + const parsed = JSON.parse(curatedJsonContent); + if (!parsed || typeof parsed !== 'object') return false; + if (parsed.invalidated === true) return false; + if ( + parsed.pr !== undefined && + parsed.pr !== null && + prNumber !== undefined && + prNumber !== null + ) { + if (Number(parsed.pr) !== Number(prNumber)) return false; + } + const expectedHash = computeFindingsHash(sourceFileContent); + if (parsed.source_sha256 && parsed.source_sha256 !== expectedHash) { + return false; + } + return Array.isArray(parsed.findings); + } catch { + return false; + } +} + export const reviewFindingsState = atom( { plugin: 'magpie-pr-management', key: 'reviewFindingsState' }, null as ReviewFindingsState | null @@ -221,7 +350,7 @@ export function renderFindingsPane($: any, e: any, state: ReviewFindingsState): label: actionSpec.label.slice(0, 3), onPress: async () => { const updated = toggleFindingActionInState(state, globalIdx); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); }, }) : Text({ @@ -297,7 +426,7 @@ export function renderFindingsPane($: any, e: any, state: ReviewFindingsState): label: actionSpec.label.slice(0, 3), onPress: async () => { const updated = toggleFindingActionInState(state, globalIdx); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); }, }) : Text({ @@ -371,7 +500,7 @@ export function renderFindingsPane($: any, e: any, state: ReviewFindingsState): label: '[All]', onPress: async () => { const updated = setBulkActionInState(state, 'accept'); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); }, }), Text({ children: [' '] }), @@ -380,7 +509,7 @@ export function renderFindingsPane($: any, e: any, state: ReviewFindingsState): label: '[None]', onPress: async () => { const updated = setBulkActionInState(state, 'skip'); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); }, }), Text({ children: [' '] }), @@ -420,6 +549,22 @@ export function renderFindingsPane($: any, e: any, state: ReviewFindingsState): ); } + if (state.syncError) { + contentBoxes.push( + Box({ + paddingTop: 1, + children: [ + Text({ + children: [ + `[!] Warning: Curation sync to disk failed (${state.syncError}). Sidecars invalidated; Step 7b will use original findings.`, + ], + color: 'warning', + }), + ], + }) + ); + } + return Box({ flexDirection: 'column', padding: 1, @@ -756,6 +901,9 @@ export function setBulkActionInState( if (anchoredOnly && !isAnchored(f)) { return f; } + if (action === 'accept' && f.action === 'edit') { + return f; + } return { ...f, action }; }); return { @@ -805,8 +953,15 @@ const ALIAS_KEYS = ['path', 'filename', 'lineNumber', 'line_number', 'level', 'a * for them. Render anchors on a truthy `line`, so with `line` normalised to * an integer >= 1 or null, its numbering over this file is formatKeepAnswer's. */ -export function serializeFindingsForRender(findings: readonly ReviewFinding[]): string { - const payload = findings.map((f) => { +export function serializeFindingsForRender( + findings: readonly ReviewFinding[], + options?: { pr?: number | null; sourceHash?: string | null } +): string { + // Anchored findings are preserved (rendered or folded via render --keep). + // Unanchored general findings marked 'skip' are omitted so render does not post them. + const activeFindings = findings.filter((f) => isAnchored(f) || f.action !== 'skip'); + + const payload = activeFindings.map((f) => { const raw = f.raw; const base: Record = typeof raw === 'object' && raw !== null && !Array.isArray(raw) @@ -817,6 +972,9 @@ export function serializeFindingsForRender(findings: readonly ReviewFinding[]): } if (f.action === 'edit') { delete base.suggestion; + delete base.comment; + delete base.suggestion_block; + delete base.fixup; if (f.body) { base.explanation = f.body; } @@ -832,9 +990,220 @@ export function serializeFindingsForRender(findings: readonly ReviewFinding[]): } return { ...base, file: f.file, line: f.line, severity: f.severity }; }); + + if (options && (options.pr !== undefined || options.sourceHash !== undefined)) { + const envelope: CuratedFindingsPayload = { + schema_version: 1, + pr: options.pr ?? null, + source_sha256: options.sourceHash ?? null, + findings: payload, + }; + return JSON.stringify(envelope, null, 2); + } + return JSON.stringify(payload, null, 2); } +/** + * Derives the file paths for curated findings and keep recommendation beside original findings. + */ +export function getCuratedFindingsPaths(originalPath: string): { + curatedJsonPath: string; + curatedKeepPath: string; +} { + if (originalPath.endsWith('.json')) { + return { + curatedJsonPath: originalPath.replace(/\.json$/, '.curated.json'), + curatedKeepPath: originalPath.replace(/\.json$/, '.curated.keep'), + }; + } + return { + curatedJsonPath: `${originalPath}.curated.json`, + curatedKeepPath: `${originalPath}.curated.keep`, + }; +} + +/** + * Marks curated findings files (.curated.json and .curated.keep) as invalidated. + * Called when findings become empty, stale, or during cleanup after write failures. + * Only writes invalidation markers if curated sidecar files already exist on disk, + * ensuring orphaned files are not created beside non-existent findings files. + */ +export async function cleanCuratedFindingsFiles( + $: any, + originalPath: string +): Promise { + if (!originalPath) return; + try { + const { curatedJsonPath, curatedKeepPath } = getCuratedFindingsPaths(originalPath); + let jsonExists = false; + let keepExists = false; + try { + jsonExists = await $.fs.exists(curatedJsonPath); + } catch { + jsonExists = false; + } + try { + keepExists = await $.fs.exists(curatedKeepPath); + } catch { + keepExists = false; + } + + if (!jsonExists && !keepExists) { + return; + } + + if (jsonExists) { + try { + const stat = await $.fs.stat(curatedJsonPath); + const isSymlink = + stat && + ((typeof stat.isSymbolicLink === 'function' && stat.isSymbolicLink()) || + stat.isSymbolicLink === true || + (typeof stat.isFile === 'function' && !stat.isFile())); + if (isSymlink) return; + } catch { + // fail-open + } + const invalidEnvelope: CuratedFindingsPayload = { + schema_version: 1, + pr: null, + source_sha256: null, + invalidated: true, + findings: [], + }; + await $.fs.write(curatedJsonPath, JSON.stringify(invalidEnvelope, null, 2)); + } + if (keepExists) { + try { + const stat = await $.fs.stat(curatedKeepPath); + const isSymlink = + stat && + ((typeof stat.isSymbolicLink === 'function' && stat.isSymbolicLink()) || + stat.isSymbolicLink === true || + (typeof stat.isFile === 'function' && !stat.isFile())); + if (isSymlink) return; + } catch { + // fail-open + } + await $.fs.write(curatedKeepPath, 'INVALIDATED'); + } + } catch { + // Fail-open + } +} + +/** + * Writes curated findings payload (.curated.json) and keep parameter (.curated.keep) + * to disk beside the original findings file using $.fs.write. + */ +export async function syncCuratedStateToDisk( + $: any, + state: ReviewFindingsState | null, + sourceContent?: string +): Promise<{ success: boolean; jsonPath?: string; keepPath?: string; error?: string }> { + if (!state || !state.findingsPath || !state.findings) { + return { success: true }; + } + if (state.findings.length === 0) { + await cleanCuratedFindingsFiles($, state.findingsPath); + return { success: true }; + } + try { + // Verify that the original source findings file exists before writing sidecars + const exists = await $.fs.exists(state.findingsPath); + if (!exists) { + await cleanCuratedFindingsFiles($, state.findingsPath); + return { + success: false, + error: `Source findings file does not exist: ${state.findingsPath}`, + }; + } + + let rawSource = sourceContent; + if (rawSource === undefined) { + try { + rawSource = await $.fs.read(state.findingsPath); + } catch { + rawSource = ''; + } + } + const sourceHash = state.sourceHash || computeFindingsHash(rawSource || ''); + + const { curatedJsonPath, curatedKeepPath } = getCuratedFindingsPaths(state.findingsPath); + + for (const targetPath of [curatedJsonPath, curatedKeepPath]) { + try { + const exists = await $.fs.exists(targetPath); + if (exists) { + const stat = await $.fs.stat(targetPath); + const isSymlink = + stat && + ((typeof stat.isSymbolicLink === 'function' && stat.isSymbolicLink()) || + stat.isSymbolicLink === true || + (typeof stat.isFile === 'function' && !stat.isFile())); + if (isSymlink) { + return { + success: false, + error: `Refusing to write to symlink or non-regular file: ${targetPath}`, + }; + } + } + } catch { + // fail-open + } + } + + const jsonContent = serializeFindingsForRender(state.findings, { + pr: state.prNumber, + sourceHash, + }); + const keepContent = formatKeepAnswer(state.findings); + await $.fs.write(curatedJsonPath, jsonContent); + await $.fs.write(curatedKeepPath, keepContent); + return { success: true, jsonPath: curatedJsonPath, keepPath: curatedKeepPath }; + } catch (err: any) { + // Fail-open: invalidate sidecars on disk so Step 7b falls back to original findings + try { + await cleanCuratedFindingsFiles($, state.findingsPath); + } catch { + // Safe no-op + } + return { + success: false, + error: err?.message || String(err), + }; + } +} + +/** + * Updates reviewFindingsState atom and synchronizes curated findings to disk. + * If disk sync fails, records syncError in state for display in pane footer. + */ +export async function updateAndSyncReviewFindingsState( + $: any, + updatedState: ReviewFindingsState | null +): Promise<{ success: boolean; error?: string }> { + if (!updatedState) { + await setReviewFindingsState($, null); + return { success: true }; + } + const syncResult = await syncCuratedStateToDisk($, updatedState); + if (!syncResult.success) { + updatedState = { + ...updatedState, + syncError: syncResult.error || 'Failed to write curated files to disk', + }; + } else if (updatedState.syncError) { + updatedState = { + ...updatedState, + syncError: null, + }; + } + await setReviewFindingsState($, updatedState); + return syncResult; +} + /** * Strips markdown code fences surrounding JSON payloads if present. * Matches code fences even if surrounded by conversational prose. @@ -914,7 +1283,8 @@ export function parseFindings(input: string | unknown): ParseFindingsResult { const file = normalizeFilePath(item.file ?? item.path ?? item.filename); const line = normalizeLine(item.line ?? item.lineNumber ?? item.line_number, errors); const severity = normalizeSeverity(item.severity ?? item.level, errors); - const rawBody = item.body ?? item.explanation ?? item.suggestion ?? item.title ?? ''; + const rawBody = + item.body ?? item.explanation ?? item.comment ?? item.suggestion ?? item.title ?? ''; const body = String(rawBody).trim(); const rawRule = item.rule ?? item.rule_id ?? item.category ?? ''; const rule = String(rawRule).trim() || undefined; @@ -984,13 +1354,6 @@ export async function handleRenderPane($: any, e: any, next: any): Promise } } -/** - * Event hook registration entry point for Claude Code mod. - * - * Registers: - * - session.start: lifecycle hook for test fixture hydration in PR-2 (PR-3 wires the live session feed). - * - ui.render (Pane): renders the interactive review findings pane when state is populated. - */ /** * UI press event handler for Claude Code mod. * @@ -1035,7 +1398,7 @@ export async function handleUiPress($: any, e: any, next: any): Promise { const targetIdx = state.findings.indexOf(target); if (targetIdx !== -1) { const updated = toggleFindingActionInState(state, targetIdx); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } } @@ -1044,16 +1407,16 @@ export async function handleUiPress($: any, e: any, next: any): Promise { const targetIdx = resolveFindingIndex(state, idOrNum); if (targetIdx !== -1) { const updated = toggleFindingActionInState(state, targetIdx); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } } else if (element === 'accept-all') { const updated = setBulkActionInState(state, 'accept'); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } else if (element === 'skip-all') { const updated = setBulkActionInState(state, 'skip'); - await setReviewFindingsState($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } else if (element === 'prev-finding') { const updated = updateSelectedIndexInState(state, 'up'); @@ -1103,6 +1466,48 @@ export function resolveRelativePath(baseDir: string, filePath: string): string { return `${cleanBase}/${cleanPath}`; } +/** + * Matches an incoming finding to an existing finding from prior state + * to preserve maintainer curation across disposition re-runs. + */ +export function findMatchingExistingFinding( + target: ReviewFinding, + existingFindings: readonly ReviewFinding[], + consumedIndices?: Set +): ReviewFinding | undefined { + if (target.id && !target.id.startsWith('finding-')) { + const idx = existingFindings.findIndex( + (e, i) => (!consumedIndices || !consumedIndices.has(i)) && e.id === target.id + ); + if (idx !== -1) { + if (consumedIndices) consumedIndices.add(idx); + return existingFindings[idx]; + } + } + if (isAnchored(target)) { + const idx = existingFindings.findIndex((e, i) => { + if (consumedIndices && consumedIndices.has(i)) return false; + if (!isAnchored(e) || e.file !== target.file || e.line !== target.line) return false; + return Boolean((target.rule && e.rule === target.rule) || e.body === target.body); + }); + if (idx !== -1) { + if (consumedIndices) consumedIndices.add(idx); + return existingFindings[idx]; + } + } else { + const idx = existingFindings.findIndex((e, i) => { + if (consumedIndices && consumedIndices.has(i)) return false; + if (isAnchored(e) || e.file !== target.file) return false; + return Boolean((target.rule && e.rule === target.rule) || e.body === target.body); + }); + if (idx !== -1) { + if (consumedIndices) consumedIndices.add(idx); + return existingFindings[idx]; + } + } + return undefined; +} + /** * Bash tool.call event handler for Claude Code mod (Fail-Open). * @@ -1139,14 +1544,18 @@ export async function handleBashToolCall($: any, e: any, next: any): Promise 0) { const prNumber = extractPrNumberFromCommand(command); - await setReviewFindingsState($, { + + let existingState: ReviewFindingsState | null = null; + try { + existingState = await read($, reviewFindingsState); + } catch { + existingState = null; + } + + let finalFindings = parsed.findings; + if ( + existingState && + existingState.findings && + existingState.findings.length > 0 && + (!existingState.prNumber || !prNumber || existingState.prNumber === prNumber) + ) { + const consumed = new Set(); + finalFindings = parsed.findings.map((f) => { + const match = findMatchingExistingFinding(f, existingState.findings, consumed); + if (match) { + return { + ...f, + action: match.action, + body: match.action === 'edit' ? match.body : f.body, + }; + } + return f; + }); + } + + const targetFindingsFile = effectivePath || rawPath; + const sourceHash = computeFindingsHash(fileContent); + + const nextState: ReviewFindingsState = { prNumber, - findings: parsed.findings, - summary: parsed.summary, + findingsPath: targetFindingsFile, + sourceHash, + findings: finalFindings, + summary: calculateSummary(finalFindings), selectedIndex: 0, - }); + }; + await setReviewFindingsState($, nextState); + + // Check if curated sidecars already exist on disk from earlier curation + const { curatedJsonPath } = getCuratedFindingsPaths(targetFindingsFile); + let hasExistingCuratedJson = false; + try { + hasExistingCuratedJson = await $.fs.exists(curatedJsonPath); + } catch { + hasExistingCuratedJson = false; + } + + if (hasExistingCuratedJson) { + let existingDiskJson: any = null; + try { + const rawCurated = await $.fs.read(curatedJsonPath); + existingDiskJson = JSON.parse(rawCurated); + } catch { + existingDiskJson = null; + } + + const isAlreadyInvalidated = + !existingDiskJson || + typeof existingDiskJson !== 'object' || + existingDiskJson.invalidated === true; + + const matchesPr = + !isAlreadyInvalidated && + existingDiskJson.pr !== undefined && + existingDiskJson.pr !== null && + prNumber !== undefined && + prNumber !== null && + Number(existingDiskJson.pr) === Number(prNumber); + + if (matchesPr && finalFindings.length > 0) { + // Re-sync sidecars with merged curation state and updated source hash + await syncCuratedStateToDisk($, nextState, fileContent); + } else if (!isAlreadyInvalidated) { + // PR mismatch or empty findings: invalidate sidecars on disk + await cleanCuratedFindingsFiles($, targetFindingsFile); + } + } + // NOTE: Never write .curated.* files at initial disposition. + // Curated files are only written when curated sidecars exist or when curated in UI. } else { + await cleanCuratedFindingsFiles($, effectivePath || rawPath); await setReviewFindingsState($, null); } } else { + await cleanCuratedFindingsFiles($, effectivePath || rawPath); await setReviewFindingsState($, null); } } @@ -1178,6 +1666,74 @@ export async function handleBashToolCall($: any, e: any, next: any): Promise; + generalIndices: Set; +} + +/** + * Validates and parses finding indices for /magpie-findings keep and drop. + * Strictly requires every token to be an integer >= 1 (e.g. 1, 3) or general observation (e.g. g1, g2). + * Rejects arbitrary strings (e.g. abc). + */ +export function parseFindingTokens(raw: string): ParsedFindingTokens { + const parts = raw.split(',').map((p) => p.trim()).filter(Boolean); + if (parts.length === 0) { + return { + valid: false, + error: 'No finding indices specified', + anchoredIndices: new Set(), + generalIndices: new Set(), + }; + } + + const anchoredIndices = new Set(); + const generalIndices = new Set(); + + for (const token of parts) { + if (/^\d+$/.test(token)) { + const num = Number.parseInt(token, 10); + if (num >= 1) { + anchoredIndices.add(num); + } else { + return { + valid: false, + error: `Invalid index "${token}": indices must be >= 1`, + anchoredIndices: new Set(), + generalIndices: new Set(), + }; + } + } else if (/^[gG]\d+$/.test(token)) { + const num = Number.parseInt(token.slice(1), 10); + if (num >= 1) { + generalIndices.add(num); + } else { + return { + valid: false, + error: `Invalid general index "${token}": indices must be >= 1`, + anchoredIndices: new Set(), + generalIndices: new Set(), + }; + } + } else { + return { + valid: false, + error: `Invalid token "${token}". Only numeric indices (e.g. 1, 3) and general observation indices (e.g. g1, g2) are allowed.`, + anchoredIndices: new Set(), + generalIndices: new Set(), + }; + } + } + + return { + valid: true, + anchoredIndices, + generalIndices, + }; +} + /** * Slash command handler for /magpie-findings (zero-token). * @@ -1237,39 +1793,79 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str lines.push(` ${indicator.label.slice(0, 3)} g${i + 1}. [${f.severity.toUpperCase()}] ${f.file} - ${truncateText(f.body, 60)}`); } } + if (state.syncError) { + lines.push(''); + lines.push(`[!] Warning: Curation sync to disk failed (${state.syncError}).`); + } return { text: lines.join('\n') }; } if (rawArgs === 'all') { const updated = setBulkActionInState(state, 'accept'); - await setReviewFindingsState($, updated); + const syncRes = await updateAndSyncReviewFindingsState($, updated); + const warnSuffix = syncRes.error ? ` (Warning: sync to disk failed: ${syncRes.error})` : ''; return { - text: `Accepted all review findings. Keep parameter: ${formatKeepAnswer(updated.findings)}`, + text: `Accepted all review findings. Keep parameter: ${formatKeepAnswer(updated.findings)}${warnSuffix}`, }; } if (rawArgs === 'none') { const updated = setBulkActionInState(state, 'skip'); - await setReviewFindingsState($, updated); + const syncRes = await updateAndSyncReviewFindingsState($, updated); + const warnSuffix = syncRes.error ? ` (Warning: sync to disk failed: ${syncRes.error})` : ''; return { - text: 'Skipped all review findings. Keep parameter: NONE', + text: `Skipped all review findings. Keep parameter: NONE${warnSuffix}`, }; } - const keepMatch = rawArgs.match(/^keep\s+([0-9,\s]+)$/i); + const keepMatch = rawArgs.match(/^keep(?:\s+([\s\S]+))?$/i); if (keepMatch) { - const parts = keepMatch[1].split(',').map((p) => p.trim()).filter(Boolean); - const indicesToKeep = new Set( - parts.map((p) => Number.parseInt(p, 10)).filter((n) => Number.isInteger(n) && n >= 1) - ); + if (!keepMatch[1] || keepMatch[1].trim() === '') { + return { text: 'Usage: /magpie-findings keep (e.g. 1,3 or g1)' }; + } + const parsedTokens = parseFindingTokens(keepMatch[1]); + if (!parsedTokens.valid) { + return { text: `Error: ${parsedTokens.error}` }; + } + const { anchoredIndices, generalIndices } = parsedTokens; const anchored = state.findings.filter(isAnchored); + const unanchored = state.findings.filter((f) => !isAnchored(f)); + + for (const idx of anchoredIndices) { + if (idx > anchored.length) { + return { + text: `Error: Anchored finding index ${idx} out of range (1-${anchored.length})`, + }; + } + } + for (const gIdx of generalIndices) { + if (gIdx > unanchored.length) { + return { + text: `Error: General observation index g${gIdx} out of range (g1-g${unanchored.length})`, + }; + } + } + + const hasAnchoredSpec = anchoredIndices.size > 0; + const hasGeneralSpec = generalIndices.size > 0; + const newFindings = state.findings.map((f) => { - if (!isAnchored(f)) return f; - const anchoredIndex = anchored.indexOf(f) + 1; + if (isAnchored(f)) { + if (!hasAnchoredSpec) return f; + const anchoredIndex = anchored.indexOf(f) + 1; + const isKept = anchoredIndices.has(anchoredIndex); + return { + ...f, + action: isKept ? (f.action === 'edit' ? ('edit' as const) : ('accept' as const)) : ('skip' as const), + }; + } + if (!hasGeneralSpec) return f; + const genIndex = unanchored.indexOf(f) + 1; + const isKept = generalIndices.has(genIndex); return { ...f, - action: indicesToKeep.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), + action: isKept ? (f.action === 'edit' ? ('edit' as const) : ('accept' as const)) : ('skip' as const), }; }); @@ -1278,26 +1874,66 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str findings: newFindings, summary: calculateSummary(newFindings), }; - await setReviewFindingsState($, updated); + const syncRes = await updateAndSyncReviewFindingsState($, updated); + const keptList: string[] = []; + if (hasAnchoredSpec) { + keptList.push(...Array.from(anchoredIndices).sort((a, b) => a - b).map(String)); + } + if (hasGeneralSpec) { + keptList.push(...Array.from(generalIndices).sort((a, b) => a - b).map((g) => `g${g}`)); + } + const warnSuffix = syncRes.error ? ` (Warning: sync to disk failed: ${syncRes.error})` : ''; return { - text: `Curated findings. Kept: ${Array.from(indicesToKeep).sort((a, b) => a - b).join(',')}. Keep parameter: ${formatKeepAnswer(updated.findings)}`, + text: `Curated findings. Kept: ${keptList.join(',')}. Keep parameter: ${formatKeepAnswer(updated.findings)}${warnSuffix}`, }; } - const dropMatch = rawArgs.match(/^drop\s+([0-9,\s]+)$/i); + const dropMatch = rawArgs.match(/^drop(?:\s+([\s\S]+))?$/i); if (dropMatch) { - const parts = dropMatch[1].split(',').map((p) => p.trim()).filter(Boolean); - const indicesToDrop = new Set( - parts.map((p) => Number.parseInt(p, 10)).filter((n) => Number.isInteger(n) && n >= 1) - ); + if (!dropMatch[1] || dropMatch[1].trim() === '') { + return { text: 'Usage: /magpie-findings drop (e.g. 2 or g1)' }; + } + const parsedTokens = parseFindingTokens(dropMatch[1]); + if (!parsedTokens.valid) { + return { text: `Error: ${parsedTokens.error}` }; + } + const { anchoredIndices, generalIndices } = parsedTokens; const anchored = state.findings.filter(isAnchored); + const unanchored = state.findings.filter((f) => !isAnchored(f)); + + for (const idx of anchoredIndices) { + if (idx > anchored.length) { + return { + text: `Error: Anchored finding index ${idx} out of range (1-${anchored.length})`, + }; + } + } + for (const gIdx of generalIndices) { + if (gIdx > unanchored.length) { + return { + text: `Error: General observation index g${gIdx} out of range (g1-g${unanchored.length})`, + }; + } + } + + const hasAnchoredSpec = anchoredIndices.size > 0; + const hasGeneralSpec = generalIndices.size > 0; + const newFindings = state.findings.map((f) => { - if (!isAnchored(f)) return f; - const anchoredIndex = anchored.indexOf(f) + 1; + if (isAnchored(f)) { + if (!hasAnchoredSpec) return f; + const anchoredIndex = anchored.indexOf(f) + 1; + return { + ...f, + action: anchoredIndices.has(anchoredIndex) ? ('skip' as const) : f.action, + }; + } + if (!hasGeneralSpec) return f; + const genIndex = unanchored.indexOf(f) + 1; return { ...f, - action: indicesToDrop.has(anchoredIndex) ? ('skip' as const) : f.action, + action: generalIndices.has(genIndex) ? ('skip' as const) : f.action, }; }); @@ -1306,9 +1942,17 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str findings: newFindings, summary: calculateSummary(newFindings), }; - await setReviewFindingsState($, updated); + const syncRes = await updateAndSyncReviewFindingsState($, updated); + const droppedList: string[] = []; + if (hasAnchoredSpec) { + droppedList.push(...Array.from(anchoredIndices).sort((a, b) => a - b).map(String)); + } + if (hasGeneralSpec) { + droppedList.push(...Array.from(generalIndices).sort((a, b) => a - b).map((g) => `g${g}`)); + } + const warnSuffix = syncRes.error ? ` (Warning: sync to disk failed: ${syncRes.error})` : ''; return { - text: `Dropped findings: ${Array.from(indicesToDrop).sort((a, b) => a - b).join(',')}. Keep parameter: ${formatKeepAnswer(updated.findings)}`, + text: `Dropped findings: ${droppedList.join(',')}. Keep parameter: ${formatKeepAnswer(updated.findings)}${warnSuffix}`, }; } @@ -1322,9 +1966,10 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str action: 'edit', body: newNote, }); - await setReviewFindingsState($, updated); + const syncRes = await updateAndSyncReviewFindingsState($, updated); + const warnSuffix = syncRes.error ? ` (Warning: sync to disk failed: ${syncRes.error})` : ''; return { - text: `Updated finding #${idOrNum} (marked edit with custom note). Keep parameter: ${formatKeepAnswer(updated.findings)}`, + text: `Updated finding #${idOrNum} (marked edit with custom note). Keep parameter: ${formatKeepAnswer(updated.findings)}${warnSuffix}`, }; } return { text: `Finding #${idOrNum} not found in active review findings.` }; @@ -1335,6 +1980,16 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str }; } +/** + * Event hook registration entry point for Claude Code mod. + * + * Registers: + * - session.start: command registration and test fixture hydration + * - ui.render (Pane): renders the interactive review findings pane when state is populated + * - ui.press: handles button press interactions from the review findings pane + * - tool.call (Bash): intercepts code-review disposition to hydrate findings from disk + * - command.run (magpie-findings): zero-token command for inspecting and curating findings + */ export function register(on: any): void { on('session.start', async ($: any, e: any, next: any) => { try { diff --git a/plugins/magpie-pr-management/hooks/types.d.ts b/plugins/magpie-pr-management/hooks/types.d.ts index bc6c458a2..16862efd1 100644 --- a/plugins/magpie-pr-management/hooks/types.d.ts +++ b/plugins/magpie-pr-management/hooks/types.d.ts @@ -27,6 +27,8 @@ declare module 'claude-code' { reviewFindingsState: { prNumber?: number; prTitle?: string; + findingsPath?: string; + sourceHash?: string | null; findings: Array<{ id: string; file: string; @@ -49,6 +51,7 @@ declare module 'claude-code' { unanchoredCount: number; }; selectedIndex: number; + syncError?: string | null; } | null; }; } diff --git a/plugins/magpie-pr-management/hooks/types.ts b/plugins/magpie-pr-management/hooks/types.ts index ba71c7679..93dea46d2 100644 --- a/plugins/magpie-pr-management/hooks/types.ts +++ b/plugins/magpie-pr-management/hooks/types.ts @@ -53,9 +53,12 @@ export interface ReviewFindingsSummary { export interface ReviewFindingsState { prNumber?: number; prTitle?: string; + findingsPath?: string; + sourceHash?: string | null; findings: ReviewFinding[]; summary: ReviewFindingsSummary; selectedIndex: number; + syncError?: string | null; } export interface ParseFindingsResult { diff --git a/plugins/magpie-pr-management/skills/code-review/review-flow.md b/plugins/magpie-pr-management/skills/code-review/review-flow.md index b5f981a24..c4c85f761 100644 --- a/plugins/magpie-pr-management/skills/code-review/review-flow.md +++ b/plugins/magpie-pr-management/skills/code-review/review-flow.md @@ -318,15 +318,19 @@ Proposed inline comments (all enabled by default): [x] 2. providers/foo/hook.py:189 โ€” minor [x] 3. providers/foo/tests/test_hook.py:33 โ€” nit -Pick which to post: [A]ll (default), [N]one, 1,3 (keep), -2 (drop), [E 2] (edit), [Q]uit +Pick which to post: [A]ll (default without curation), [N]one, 1,3 (keep), -2 (drop), [E 2] (edit), [Q]uit ``` -The answer is passed to `render --keep`; dropped comments fold into the body's *Smaller observations*. The picker is skipped when there is nothing anchored, or under `inline:off`. +The answer is passed to `render --keep`; dropped comments fold into the body's *Smaller observations*. +The picker is skipped when there is nothing anchored, or under `inline:off`. +When `.curated.keep` is valid from interactive curation, pressing Enter (empty answer) accepts that curated selection as the default; otherwise, pressing Enter defaults to `[A]ll`. Under Claude Code with the `magpie-pr-management` plugin enabled, the interactive findings pane mod automatically hydrates upon running Step 6 (`disposition --findings `) and renders findings above the prompt as an interactive visual overview during review triage. -Maintainers can inspect findings and track action toggles (`accept`, `skip`, `edit`) via interactive buttons or the zero-token `/magpie-findings` slash command (`status`, `all`, `none`, `keep `, `drop `, `edit `). +Maintainers can inspect findings and track action toggles (`accept`, `skip`, `edit`) via interactive buttons or the zero-token `/magpie-findings` slash command (`status`, `all`, `none`, `keep `, `drop `, `edit `), where `` can address anchored inline comments (`1`, `3`) and general observations (`g1`, `g2`). The resulting keep recommendation is displayed in the pane footer and in `/magpie-findings status` for easy entry into the picker prompt. -The pane and the command do not change what is posted: only the answer typed at the picker reaches `render`, so a note from `/magpie-findings edit` has to be entered again there with `E `. +When findings are curated in the pane or via `/magpie-findings`, the mod syncs the serialized findings payload to `.curated.json` and the keep answer to `.curated.keep` beside the findings file. +Initial `disposition` execution never writes curated sidecars upfront. +Re-running `disposition` after editing raw findings re-syncs existing sidecars from the merged curation state. Per the vendor-neutral additive-only contract (`docs/when-to-use-mods.md`), this mod is entirely additive. In Codex, Antigravity, Cursor, and headless CI environments where mod hooks are unsupported or absent, the skill seamlessly continues using the standard sequential text picker prompt above. @@ -343,8 +347,16 @@ uv run --project /tools/pr-management pr-management code-review rende [--security-note ] [--inline off] ``` +When `.curated.json` is present beside ``, `render` validates its PR and source hash directly, using the curated findings when valid and falling back to `` otherwise. + +**Picker precedence**: The maintainer's typed answer at the Step 7a text picker prompt always takes precedence. +If the maintainer enters an answer (e.g. `1,3`, `None`, `-2`), pass that answer directly to `--keep `. +When findings were curated in the interactive pane or via `/magpie-findings`, `.curated.keep` holds that curated set. +An empty answer (pressing Enter) accepts `.curated.keep` when valid, and defaults to `All` otherwise. + It assembles the body in the template order ([`posting.md`](posting.md#review-body--template-structure)), backtick-quotes every handle, places each kept inline comment by `line` + `side` (listing any the diff cannot anchor โ€” those fold into the body), appends the verbatim footer variant and verifies it. -Show the full body and the inline count; hold for explicit confirmation. Substantive edits re-render and re-confirm. +Show the full body and the inline count; hold for explicit confirmation. +Substantive edits re-render and re-confirm. --- diff --git a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py index d9f3df903..7ad86bb16 100644 --- a/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py +++ b/plugins/magpie-security-tools/tools/pr-management/src/pr_management/code_review/cli.py @@ -27,6 +27,7 @@ import argparse import datetime as dt +import hashlib import json import re import shlex @@ -388,9 +389,50 @@ def _doc_name(category: str) -> str: return re.sub(r"[^a-z0-9]+", "-", category.lower().split(" (")[0]).strip("-") +def _load_findings(path: Path | str | None, pr_number: int | None = None) -> list[dict[str, Any]]: + if not path: + return [] + p = Path(path) + if not p.exists(): + return [] + + raw = json.loads(p.read_text(encoding="utf-8")) + if isinstance(raw, dict): + if raw.get("invalidated"): + raise SystemExit("pr-management: curated findings envelope is marked invalidated") + if pr_number is not None and "pr" in raw and raw["pr"] != pr_number: + raise SystemExit( + f"pr-management: curated findings envelope PR mismatch (envelope has PR {raw['pr']}, expected {pr_number})" + ) + if p.name.endswith(".curated.json"): + source_file = p.with_name(p.name[: -len(".curated.json")]) + if source_file.exists() and raw.get("source_sha256"): + expected_sha = hashlib.sha256(source_file.read_bytes()).hexdigest() + if raw["source_sha256"] != expected_sha: + raise SystemExit("pr-management: curated findings envelope source hash mismatch") + return raw.get("findings", []) + + if isinstance(raw, list): + curated_path = p.with_name(f"{p.name}.curated.json") + if curated_path.exists(): + try: + curated_raw = json.loads(curated_path.read_text(encoding="utf-8")) + if isinstance(curated_raw, dict) and not curated_raw.get("invalidated"): + pr_match = pr_number is None or curated_raw.get("pr") == pr_number + source_bytes = p.read_bytes() + hash_match = curated_raw.get("source_sha256") == hashlib.sha256(source_bytes).hexdigest() + if pr_match and hash_match and "findings" in curated_raw: + return curated_raw["findings"] + except (json.JSONDecodeError, OSError, UnicodeDecodeError): + # An unreadable, malformed, or inaccessible sidecar safely falls back to raw findings. + pass + return raw + return [] + + def _disposition(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]: pr = data.load_pr(args.saved_dir / f"cr-pr-{args.pr}.json") - findings = json.loads(Path(args.findings).read_text(encoding="utf-8")) if args.findings else [] + findings = _load_findings(args.findings, pr_number=args.pr) result = _decide(args, cfg, pr, findings, args.viewer) result["docs"] = [ f"classifications/disposition-{result['disposition'].lower().replace('_', '-')}.md", @@ -520,7 +562,7 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]: saved: Path = args.saved_dir pr = data.load_pr(saved / f"cr-pr-{args.pr}.json") files = difflib.parse((saved / f"diff-{args.pr}.patch").read_text(encoding="utf-8", errors="replace")) - findings = json.loads(Path(args.findings).read_text(encoding="utf-8")) + findings = _load_findings(args.findings, pr_number=args.pr) summary = Path(args.summary).read_text(encoding="utf-8").strip() anchored = [f for f in findings if f.get("line")] picked = review_body.pick(args.keep or "A", len(anchored)) if args.inline == "on" else {"keep": []} @@ -592,7 +634,7 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]: args, cfg, pr, - json.loads(Path(args.findings).read_text(encoding="utf-8")), + findings, args.viewer, ) if not decision["auto_merge"]["eligible"]: diff --git a/tools/pr-management/src/pr_management/code_review/cli.py b/tools/pr-management/src/pr_management/code_review/cli.py index d9f3df903..7ad86bb16 100644 --- a/tools/pr-management/src/pr_management/code_review/cli.py +++ b/tools/pr-management/src/pr_management/code_review/cli.py @@ -27,6 +27,7 @@ import argparse import datetime as dt +import hashlib import json import re import shlex @@ -388,9 +389,50 @@ def _doc_name(category: str) -> str: return re.sub(r"[^a-z0-9]+", "-", category.lower().split(" (")[0]).strip("-") +def _load_findings(path: Path | str | None, pr_number: int | None = None) -> list[dict[str, Any]]: + if not path: + return [] + p = Path(path) + if not p.exists(): + return [] + + raw = json.loads(p.read_text(encoding="utf-8")) + if isinstance(raw, dict): + if raw.get("invalidated"): + raise SystemExit("pr-management: curated findings envelope is marked invalidated") + if pr_number is not None and "pr" in raw and raw["pr"] != pr_number: + raise SystemExit( + f"pr-management: curated findings envelope PR mismatch (envelope has PR {raw['pr']}, expected {pr_number})" + ) + if p.name.endswith(".curated.json"): + source_file = p.with_name(p.name[: -len(".curated.json")]) + if source_file.exists() and raw.get("source_sha256"): + expected_sha = hashlib.sha256(source_file.read_bytes()).hexdigest() + if raw["source_sha256"] != expected_sha: + raise SystemExit("pr-management: curated findings envelope source hash mismatch") + return raw.get("findings", []) + + if isinstance(raw, list): + curated_path = p.with_name(f"{p.name}.curated.json") + if curated_path.exists(): + try: + curated_raw = json.loads(curated_path.read_text(encoding="utf-8")) + if isinstance(curated_raw, dict) and not curated_raw.get("invalidated"): + pr_match = pr_number is None or curated_raw.get("pr") == pr_number + source_bytes = p.read_bytes() + hash_match = curated_raw.get("source_sha256") == hashlib.sha256(source_bytes).hexdigest() + if pr_match and hash_match and "findings" in curated_raw: + return curated_raw["findings"] + except (json.JSONDecodeError, OSError, UnicodeDecodeError): + # An unreadable, malformed, or inaccessible sidecar safely falls back to raw findings. + pass + return raw + return [] + + def _disposition(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]: pr = data.load_pr(args.saved_dir / f"cr-pr-{args.pr}.json") - findings = json.loads(Path(args.findings).read_text(encoding="utf-8")) if args.findings else [] + findings = _load_findings(args.findings, pr_number=args.pr) result = _decide(args, cfg, pr, findings, args.viewer) result["docs"] = [ f"classifications/disposition-{result['disposition'].lower().replace('_', '-')}.md", @@ -520,7 +562,7 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]: saved: Path = args.saved_dir pr = data.load_pr(saved / f"cr-pr-{args.pr}.json") files = difflib.parse((saved / f"diff-{args.pr}.patch").read_text(encoding="utf-8", errors="replace")) - findings = json.loads(Path(args.findings).read_text(encoding="utf-8")) + findings = _load_findings(args.findings, pr_number=args.pr) summary = Path(args.summary).read_text(encoding="utf-8").strip() anchored = [f for f in findings if f.get("line")] picked = review_body.pick(args.keep or "A", len(anchored)) if args.inline == "on" else {"keep": []} @@ -592,7 +634,7 @@ def _render(args: argparse.Namespace, cfg: config.Config) -> dict[str, Any]: args, cfg, pr, - json.loads(Path(args.findings).read_text(encoding="utf-8")), + findings, args.viewer, ) if not decision["auto_merge"]["eligible"]: diff --git a/tools/pr-management/tests/code_review/test_cli.py b/tools/pr-management/tests/code_review/test_cli.py index 9a96f5c72..37521594d 100644 --- a/tools/pr-management/tests/code_review/test_cli.py +++ b/tools/pr-management/tests/code_review/test_cli.py @@ -19,6 +19,7 @@ from __future__ import annotations import base64 +import hashlib import json import shlex from pathlib import Path @@ -216,6 +217,295 @@ def test_context_then_disposition_then_render( assert "Apache Product maintainer" in body and "https://example.org/contrib" in body +def test_disposition_and_render_with_envelope_findings( + capsys: pytest.CaptureFixture[str], project: Path, saved: Path, tmp_path: Path +) -> None: + envelope_findings = tmp_path / "envelope_findings.json" + envelope_findings.write_text( + json.dumps( + { + "schema_version": 1, + "pr": 7, + "source_sha256": "abcdef1234567890", + "findings": [ + { + "file": "scheduler/job.py", + "line": 2, + "severity": "minor", + "rule": "naming-convention", + "explanation": "Prefer a verb phrase for function names.", + "suggestion": "-def process():\n+def process_batch():", + } + ], + } + ) + ) + dis = _run( + capsys, + "--config-dir", + str(project), + "code-review", + "disposition", + "--saved-dir", + str(saved), + "--pr", + "7", + "--viewer", + "alice", + "--findings", + str(envelope_findings), + ) + assert dis["disposition"] == "COMMENT" + summary = tmp_path / "summary_envelope.txt" + summary.write_text("One naming nit inline.") + out = _run( + capsys, + "--config-dir", + str(project), + "code-review", + "render", + "--saved-dir", + str(saved), + "--pr", + "7", + "--findings", + str(envelope_findings), + "--summary", + str(summary), + "--disposition", + "COMMENT", + "--out-dir", + str(tmp_path / "out_env"), + "--permission", + "write", + ) + assert out["footer_verified"] and out["inline_threads"] == 1 + + +def test_render_auto_picks_valid_curated_sidecar( + capsys: pytest.CaptureFixture[str], project: Path, saved: Path, tmp_path: Path +) -> None: + findings_file = tmp_path / "raw_findings.json" + raw_content = json.dumps( + [ + { + "file": "scheduler/job.py", + "line": 2, + "severity": "minor", + "rule": "naming-convention", + "explanation": "Original explanation.", + } + ] + ) + findings_file.write_text(raw_content, encoding="utf-8") + source_sha256 = hashlib.sha256(findings_file.read_bytes()).hexdigest() + + curated_file = tmp_path / "raw_findings.json.curated.json" + curated_file.write_text( + json.dumps( + { + "schema_version": 1, + "pr": 7, + "source_sha256": source_sha256, + "findings": [ + { + "file": "scheduler/job.py", + "line": 2, + "severity": "minor", + "rule": "naming-convention", + "explanation": "Curated maintainer explanation.", + } + ], + } + ), + encoding="utf-8", + ) + + summary = tmp_path / "summary_autopick.txt" + summary.write_text("One naming nit inline.") + out = _run( + capsys, + "--config-dir", + str(project), + "code-review", + "render", + "--saved-dir", + str(saved), + "--pr", + "7", + "--findings", + str(findings_file), + "--summary", + str(summary), + "--disposition", + "COMMENT", + "--out-dir", + str(tmp_path / "out_autopick"), + "--permission", + "write", + ) + assert out["footer_verified"] and out["inline_threads"] == 1 + payload = json.loads(Path(out["payload_file"]).read_text(encoding="utf-8")) + thread_body = payload["variables"]["threads"][0]["body"] + assert "Curated maintainer explanation." in thread_body + assert "Original explanation." not in thread_body + + +def test_render_falls_back_when_curated_sidecar_has_mismatched_sha( + capsys: pytest.CaptureFixture[str], project: Path, saved: Path, tmp_path: Path +) -> None: + findings_file = tmp_path / "raw_findings_mismatch.json" + raw_content = json.dumps( + [ + { + "file": "scheduler/job.py", + "line": 2, + "severity": "minor", + "rule": "naming-convention", + "explanation": "Original explanation fallback.", + } + ] + ) + findings_file.write_text(raw_content, encoding="utf-8") + + curated_file = tmp_path / "raw_findings_mismatch.json.curated.json" + curated_file.write_text( + json.dumps( + { + "schema_version": 1, + "pr": 7, + "source_sha256": "stale_hash_that_does_not_match", + "findings": [ + { + "file": "scheduler/job.py", + "line": 2, + "severity": "minor", + "rule": "naming-convention", + "explanation": "Stale sidecar explanation.", + } + ], + } + ), + encoding="utf-8", + ) + + summary = tmp_path / "summary_mismatch.txt" + summary.write_text("One naming nit inline.") + out = _run( + capsys, + "--config-dir", + str(project), + "code-review", + "render", + "--saved-dir", + str(saved), + "--pr", + "7", + "--findings", + str(findings_file), + "--summary", + str(summary), + "--disposition", + "COMMENT", + "--out-dir", + str(tmp_path / "out_mismatch"), + "--permission", + "write", + ) + assert out["footer_verified"] and out["inline_threads"] == 1 + payload = json.loads(Path(out["payload_file"]).read_text(encoding="utf-8")) + thread_body = payload["variables"]["threads"][0]["body"] + assert "Original explanation fallback." in thread_body + assert "Stale sidecar explanation." not in thread_body + + +def test_render_refuses_invalidated_curated_envelope( + capsys: pytest.CaptureFixture[str], project: Path, saved: Path, tmp_path: Path +) -> None: + invalidated_file = tmp_path / "findings.curated.json" + invalidated_file.write_text( + json.dumps( + { + "schema_version": 1, + "pr": 7, + "invalidated": True, + "findings": [], + } + ), + encoding="utf-8", + ) + summary = tmp_path / "summary_inv.txt" + summary.write_text("Summary line.") + with pytest.raises(SystemExit) as exc: + _run( + capsys, + "--config-dir", + str(project), + "code-review", + "render", + "--saved-dir", + str(saved), + "--pr", + "7", + "--findings", + str(invalidated_file), + "--summary", + str(summary), + "--disposition", + "COMMENT", + "--out-dir", + str(tmp_path / "out_inv"), + ) + assert "invalidated" in str(exc.value) + + +def test_render_refuses_wrong_pr_curated_envelope( + capsys: pytest.CaptureFixture[str], project: Path, saved: Path, tmp_path: Path +) -> None: + wrong_pr_file = tmp_path / "findings_wrong_pr.curated.json" + wrong_pr_file.write_text( + json.dumps( + { + "schema_version": 1, + "pr": 999, + "findings": [ + { + "file": "scheduler/job.py", + "line": 2, + "severity": "minor", + "rule": "naming-convention", + "explanation": "Wrong PR finding.", + } + ], + } + ), + encoding="utf-8", + ) + summary = tmp_path / "summary_wrong_pr.txt" + summary.write_text("Summary line.") + with pytest.raises(SystemExit) as exc: + _run( + capsys, + "--config-dir", + str(project), + "code-review", + "render", + "--saved-dir", + str(saved), + "--pr", + "7", + "--findings", + str(wrong_pr_file), + "--summary", + str(summary), + "--disposition", + "COMMENT", + "--out-dir", + str(tmp_path / "out_wrong_pr"), + ) + assert "PR mismatch" in str(exc.value) + + def test_guard_needs_the_liveness_read_then_checks_the_head( capsys: pytest.CaptureFixture[str], project: Path, saved: Path ) -> None: