diff --git a/docs/setup/secure-agent-setup.md b/docs/setup/secure-agent-setup.md index 1a15b9211..e5fcaf3ed 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,19 @@ 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. +- **Locality and sidecar constraints**: `$.fs.write` is strictly constrained to sibling sidecars (`.curated.json` and `.curated.keep`) placed directly beside an existing vetted findings file path discovered from a preceding `code-review disposition` command. The hook never targets arbitrary filesystem paths, user configuration files, dotfiles, or executable scripts. +- **Fail-open design**: 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 is recorded in state, an explicit warning is displayed in the interactive pane footer and `/magpie-findings status`, and downstream skill steps fall back cleanly to original findings and interactive sequential prompts. +- **Orphan cleanup**: When a re-run of `disposition` discovers that all findings have been resolved (empty findings list), pre-existing curated sidecar files are automatically cleaned up to prevent stale curation decisions from lingering on disk. + ## 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..7479e1069 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.test.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.test.ts @@ -20,10 +20,13 @@ import { describe, test } from 'claude-code/testing'; import { calculateSummary, + cleanCuratedFindingsFiles, cycleFindingAction, + findMatchingExistingFinding, formatActionIndicator, formatFindingDescription, formatKeepAnswer, + getCuratedFindingsPaths, extractFindingsPathFromCommand, extractPrNumberFromCommand, formatSeverityBadge, @@ -38,6 +41,7 @@ import { normalizeSeverity, normalizeSource, parseFindings, + parseFindingTokens, resolveFindingIndex, resolveRelativePath, register, @@ -47,9 +51,11 @@ import { serializeFindingsForRender, setBulkActionInState, setReviewFindingsState, + syncCuratedStateToDisk, toggleFindingActionInState, truncateFilePath, truncateText, + updateAndSyncReviewFindingsState, updateFindingInState, updateSelectedIndexInState, } from './review-findings.ts'; @@ -71,6 +77,23 @@ 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); + }, + remove: async (path: string) => { + files.delete(path); + }, + unlink: async (path: string) => { + files.delete(path); + }, + _files: files, + }; return { state: { get: (atom: any) => { @@ -91,6 +114,10 @@ function createMockEngine(overrides: any = {}): any { ui: { invalidate: () => {}, }, + fs: { + ...defaultFs, + ...(overrides.fs || {}), + }, ...overrides, }; } @@ -1939,4 +1966,398 @@ 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, + }; + + 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.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, + }; + + 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 files were cleaned up + assert(!test$.fs._files.has('/work/repo/saved/findings-empty.curated.json')); + assert(!test$.fs._files.has('/work/repo/saved/findings-empty.curated.keep')); + }); + + 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, + }; + + 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 failingTest$ = createMockEngine({ + fs: { + write: async (path: string) => { + if (path.endsWith('.keep')) { + throw new Error('EACCES: permission denied'); + } + writtenFiles.add(path); + }, + remove: async (path: string) => { + writtenFiles.delete(path); + }, + unlink: async (path: string) => { + writtenFiles.delete(path); + }, + }, + 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 cleaned up so files do not end up mismatched + assert(!writtenFiles.has('/work/readonly/findings.curated.json')); + + // 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, + }; + + 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.length, 1); + assertStrictEqual(parsed[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[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 + await handleFindingsCommand(test$, { args: 'keep 1, g1' }); + const keptState = await getReviewFindingsState(test$); + assertStrictEqual(keptState?.findings[0].action, 'accept'); + assertStrictEqual(keptState?.findings[1].action, 'accept'); + }); + }); }); diff --git a/plugins/magpie-pr-management/hooks/review-findings.ts b/plugins/magpie-pr-management/hooks/review-findings.ts index 5254ad903..bfa1d2abd 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.ts @@ -221,7 +221,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 +297,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 +371,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 +380,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 +420,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}). Step 7b will use original findings.`, + ], + color: 'warning', + }), + ], + }) + ); + } + return Box({ flexDirection: 'column', padding: 1, @@ -806,7 +822,11 @@ const ALIAS_KEYS = ['path', 'filename', 'lineNumber', 'line_number', 'level', 'a * 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) => { + // 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) @@ -835,6 +855,123 @@ export function serializeFindingsForRender(findings: readonly ReviewFinding[]): 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`, + }; +} + +/** + * Removes or clears curated findings files (.curated.json and .curated.keep). + * Called when findings become empty or stale. + */ +export async function cleanCuratedFindingsFiles( + $: any, + originalPath: string +): Promise { + if (!originalPath) return; + try { + const { curatedJsonPath, curatedKeepPath } = getCuratedFindingsPaths(originalPath); + try { + await $.fs.remove(curatedJsonPath); + } catch { + try { + await $.fs.unlink(curatedJsonPath); + } catch { + await $.fs.write(curatedJsonPath, '[]'); + } + } + try { + await $.fs.remove(curatedKeepPath); + } catch { + try { + await $.fs.unlink(curatedKeepPath); + } catch { + await $.fs.write(curatedKeepPath, 'NONE'); + } + } + } 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 +): 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 { + const { curatedJsonPath, curatedKeepPath } = getCuratedFindingsPaths(state.findingsPath); + const jsonContent = serializeFindingsForRender(state.findings); + 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: clean up partial files to prevent mismatched or stale sidecars + 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. @@ -984,13 +1121,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 +1165,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 +1174,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 +1233,44 @@ 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[] +): ReviewFinding | undefined { + if (target.id && !target.id.startsWith('finding-')) { + const idMatch = existingFindings.find((e) => e.id === target.id); + if (idMatch) return idMatch; + } + if (isAnchored(target)) { + const fileLineMatch = existingFindings.filter( + (e) => isAnchored(e) && e.file === target.file && e.line === target.line + ); + if (fileLineMatch.length === 1) { + return fileLineMatch[0]; + } + if (fileLineMatch.length > 1) { + const ruleOrBodyMatch = fileLineMatch.find( + (e) => (target.rule && e.rule === target.rule) || e.body === target.body + ); + if (ruleOrBodyMatch) return ruleOrBodyMatch; + return fileLineMatch[0]; + } + } else { + const unanchored = existingFindings.filter( + (e) => !isAnchored(e) && e.file === target.file + ); + const match = unanchored.find( + (e) => (target.rule && e.rule === target.rule) || e.body === target.body + ); + if (match) return match; + } + return undefined; +} + /** * Bash tool.call event handler for Claude Code mod (Fail-Open). * @@ -1139,14 +1307,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) + ) { + finalFindings = parsed.findings.map((f) => { + const match = findMatchingExistingFinding(f, existingState.findings); + if (match) { + return { + ...f, + action: match.action, + body: match.action === 'edit' ? match.body : f.body, + }; + } + return f; + }); + } + + const nextState: ReviewFindingsState = { prNumber, - findings: parsed.findings, - summary: parsed.summary, + findingsPath: effectivePath || rawPath, + findings: finalFindings, + summary: calculateSummary(finalFindings), selectedIndex: 0, - }); + }; + await setReviewFindingsState($, nextState); + // NOTE: Never write .curated.* files at initial disposition. + // Curated files are only written when the maintainer curates findings. } else { + await cleanCuratedFindingsFiles($, effectivePath || rawPath); await setReviewFindingsState($, null); } } else { + await cleanCuratedFindingsFiles($, effectivePath || rawPath); await setReviewFindingsState($, null); } } @@ -1178,6 +1384,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 +1511,77 @@ 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; + return { + ...f, + action: anchoredIndices.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), + }; + } + if (!hasGeneralSpec) return f; + const genIndex = unanchored.indexOf(f) + 1; return { ...f, - action: indicesToKeep.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), + action: generalIndices.has(genIndex) ? ('accept' as const) : ('skip' as const), }; }); @@ -1278,26 +1590,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 +1658,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 +1682,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 +1696,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..432cbda58 100644 --- a/plugins/magpie-pr-management/hooks/types.d.ts +++ b/plugins/magpie-pr-management/hooks/types.d.ts @@ -27,6 +27,7 @@ declare module 'claude-code' { reviewFindingsState: { prNumber?: number; prTitle?: string; + findingsPath?: string; findings: Array<{ id: string; file: string; @@ -49,6 +50,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..860661740 100644 --- a/plugins/magpie-pr-management/hooks/types.ts +++ b/plugins/magpie-pr-management/hooks/types.ts @@ -53,9 +53,11 @@ export interface ReviewFindingsSummary { export interface ReviewFindingsState { prNumber?: number; prTitle?: string; + findingsPath?: string; 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..6abfbc0d7 100644 --- a/plugins/magpie-pr-management/skills/code-review/review-flow.md +++ b/plugins/magpie-pr-management/skills/code-review/review-flow.md @@ -324,9 +324,9 @@ Pick which to post: [A]ll (default), [N]one, 1,3 (keep), -2 (drop), [E 2] (edit) 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`. 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. 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 +343,12 @@ uv run --project /tools/pr-management pr-management code-review rende [--security-note ] [--inline off] ``` +When `.curated.json` was written beside the findings file during Step 7a curation, pass that file to `--findings` so maintainer edits and omitted general observations reach `render`. + +**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 and is offered as the recommended default. If the maintainer accepts the default or runs non-interactively with a curated sidecar, pass the contents of `.curated.keep` to `--keep`. 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. ---