From fa979017ba534b850c75498e031c0ae73654c56f Mon Sep 17 00:00:00 2001 From: Vardhman Gupta Date: Thu, 5 Nov 2026 08:48:03 +0530 Subject: [PATCH 1/2] feat(pr-management): connect findings pane curation to render and polish review hooks Connect the interactive review findings pane to Step 7 render by writing curated findings and keep answers to disk beside the findings file, omitting skipped general observations from render serialization, and relocating the register doc comment. - Record findingsPath from Step 6 disposition command in state atom - Sync .curated.json and .curated.keep on action/edit changes - Omit unanchored findings with action skip in serializeFindingsForRender - Move register doc comment above export function register - Update review-flow.md Step 7a/7b to read curated files when present --- .../hooks/review-findings.test.ts | 150 ++++++++++++++++++ .../hooks/review-findings.ts | 134 +++++++++++++--- plugins/magpie-pr-management/hooks/types.d.ts | 1 + plugins/magpie-pr-management/hooks/types.ts | 1 + .../skills/code-review/review-flow.md | 7 +- 5 files changed, 267 insertions(+), 26 deletions(-) diff --git a/plugins/magpie-pr-management/hooks/review-findings.test.ts b/plugins/magpie-pr-management/hooks/review-findings.test.ts index aad6774eb..87688ecd7 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.test.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.test.ts @@ -24,6 +24,7 @@ import { formatActionIndicator, formatFindingDescription, formatKeepAnswer, + getCuratedFindingsPaths, extractFindingsPathFromCommand, extractPrNumberFromCommand, formatSeverityBadge, @@ -47,6 +48,7 @@ import { serializeFindingsForRender, setBulkActionInState, setReviewFindingsState, + syncCuratedStateToDisk, toggleFindingActionInState, truncateFilePath, truncateText, @@ -71,6 +73,17 @@ 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); + }, + _files: files, + }; return { state: { get: (atom: any) => { @@ -91,6 +104,10 @@ function createMockEngine(overrides: any = {}): any { ui: { invalidate: () => {}, }, + fs: { + ...defaultFs, + ...(overrides.fs || {}), + }, ...overrides, }; } @@ -1939,4 +1956,137 @@ 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 writes curated files upon 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 synced to disk + const curatedJson = await test$.fs.read('/work/repo/saved/findings-999.curated.json'); + assert(curatedJson.includes('main.py')); + + const curatedKeep = await test$.fs.read('/work/repo/saved/findings-999.curated.keep'); + assertStrictEqual(curatedKeep, 'ALL'); + }); + + 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..905d07634 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.ts @@ -806,7 +806,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 +839,49 @@ 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`, + }; +} + +/** + * 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<{ jsonPath: string; keepPath: string } | null> { + if (!state || !state.findingsPath || !state.findings) { + return null; + } + 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 { jsonPath: curatedJsonPath, keepPath: curatedKeepPath }; + } catch { + // Fail-open: do not throw or halt execution on file write failure + return null; + } +} + /** * Strips markdown code fences surrounding JSON payloads if present. * Matches code fences even if surrounded by conversational prose. @@ -984,13 +1031,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. * @@ -1036,6 +1076,7 @@ export async function handleUiPress($: any, e: any, next: any): Promise { if (targetIdx !== -1) { const updated = toggleFindingActionInState(state, targetIdx); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { handled: true }; } } @@ -1045,15 +1086,18 @@ export async function handleUiPress($: any, e: any, next: any): Promise { if (targetIdx !== -1) { const updated = toggleFindingActionInState(state, targetIdx); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { handled: true }; } } else if (element === 'accept-all') { const updated = setBulkActionInState(state, 'accept'); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { handled: true }; } else if (element === 'skip-all') { const updated = setBulkActionInState(state, 'skip'); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { handled: true }; } else if (element === 'prev-finding') { const updated = updateSelectedIndexInState(state, 'up'); @@ -1139,14 +1183,18 @@ export async function handleBashToolCall($: any, e: any, next: any): Promise 0) { const prNumber = extractPrNumberFromCommand(command); - await setReviewFindingsState($, { + const nextState: ReviewFindingsState = { prNumber, + findingsPath: effectivePath || rawPath, findings: parsed.findings, summary: parsed.summary, selectedIndex: 0, - }); + }; + await setReviewFindingsState($, nextState); + await syncCuratedStateToDisk($, nextState); } else { await setReviewFindingsState($, null); } @@ -1243,6 +1294,7 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str if (rawArgs === 'all') { const updated = setBulkActionInState(state, 'accept'); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { text: `Accepted all review findings. Keep parameter: ${formatKeepAnswer(updated.findings)}`, }; @@ -1251,26 +1303,37 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str if (rawArgs === 'none') { const updated = setBulkActionInState(state, 'skip'); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { text: 'Skipped all review findings. Keep parameter: NONE', }; } - const keepMatch = rawArgs.match(/^keep\s+([0-9,\s]+)$/i); + const keepMatch = rawArgs.match(/^keep\s+([a-zA-Z0-9,\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) ); + const generalIdentifiers = new Set( + parts.filter((p) => /^[gG]\d+$/.test(p)) + ); const anchored = state.findings.filter(isAnchored); const newFindings = state.findings.map((f) => { - if (!isAnchored(f)) return f; - const anchoredIndex = anchored.indexOf(f) + 1; - return { - ...f, - action: indicesToKeep.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), - }; + if (isAnchored(f)) { + const anchoredIndex = anchored.indexOf(f) + 1; + return { + ...f, + action: indicesToKeep.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), + }; + } + const unanchored = state.findings.filter((item) => !isAnchored(item)); + const genIdx = unanchored.indexOf(f) + 1; + if (generalIdentifiers.has(`g${genIdx}`) || generalIdentifiers.has(`G${genIdx}`)) { + return { ...f, action: 'accept' as const }; + } + return f; }); const updated: ReviewFindingsState = { @@ -1279,26 +1342,37 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str summary: calculateSummary(newFindings), }; await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { text: `Curated findings. Kept: ${Array.from(indicesToKeep).sort((a, b) => a - b).join(',')}. Keep parameter: ${formatKeepAnswer(updated.findings)}`, }; } - const dropMatch = rawArgs.match(/^drop\s+([0-9,\s]+)$/i); + const dropMatch = rawArgs.match(/^drop\s+([a-zA-Z0-9,\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) ); + const generalIdentifiers = new Set( + parts.filter((p) => /^[gG]\d+$/.test(p)) + ); const anchored = state.findings.filter(isAnchored); const newFindings = state.findings.map((f) => { - if (!isAnchored(f)) return f; - const anchoredIndex = anchored.indexOf(f) + 1; - return { - ...f, - action: indicesToDrop.has(anchoredIndex) ? ('skip' as const) : f.action, - }; + if (isAnchored(f)) { + const anchoredIndex = anchored.indexOf(f) + 1; + return { + ...f, + action: indicesToDrop.has(anchoredIndex) ? ('skip' as const) : f.action, + }; + } + const unanchored = state.findings.filter((item) => !isAnchored(item)); + const genIdx = unanchored.indexOf(f) + 1; + if (generalIdentifiers.has(`g${genIdx}`) || generalIdentifiers.has(`G${genIdx}`)) { + return { ...f, action: 'skip' as const }; + } + return f; }); const updated: ReviewFindingsState = { @@ -1307,6 +1381,7 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str summary: calculateSummary(newFindings), }; await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { text: `Dropped findings: ${Array.from(indicesToDrop).sort((a, b) => a - b).join(',')}. Keep parameter: ${formatKeepAnswer(updated.findings)}`, }; @@ -1323,6 +1398,7 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str body: newNote, }); await setReviewFindingsState($, updated); + await syncCuratedStateToDisk($, updated); return { text: `Updated finding #${idOrNum} (marked edit with custom note). Keep parameter: ${formatKeepAnswer(updated.findings)}`, }; @@ -1335,6 +1411,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..65a974eb0 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; diff --git a/plugins/magpie-pr-management/hooks/types.ts b/plugins/magpie-pr-management/hooks/types.ts index ba71c7679..3512b147e 100644 --- a/plugins/magpie-pr-management/hooks/types.ts +++ b/plugins/magpie-pr-management/hooks/types.ts @@ -53,6 +53,7 @@ export interface ReviewFindingsSummary { export interface ReviewFindingsState { prNumber?: number; prTitle?: string; + findingsPath?: string; findings: ReviewFinding[]; summary: ReviewFindingsSummary; selectedIndex: number; 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..b62c1af34 100644 --- a/plugins/magpie-pr-management/skills/code-review/review-flow.md +++ b/plugins/magpie-pr-management/skills/code-review/review-flow.md @@ -326,7 +326,7 @@ The answer is passed to `render --keep`; dropped comments fold into the body's * 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 `). 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. 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,11 @@ 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`. +When `.curated.keep` is present, pass its contents 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. --- From ed9df03c093f0d98139ebd385448b075afbdf21f Mon Sep 17 00:00:00 2001 From: Vardhman Gupta Date: Fri, 6 Nov 2026 09:11:56 +0530 Subject: [PATCH 2/2] fix(pr-management): address maintainer review feedback on findings curation bridge Address review comments from @potiuk on PR #2005: - Do not write curated files to disk upon initial disposition run - Reconcile and preserve existing maintainer curation across disposition runs - Clean up orphaned curated sidecars when findings become empty - Route in-pane button onPress callbacks through updateAndSyncReviewFindingsState - Enforce strict token validation for keep and drop commands and decouple gN observations - Surface sync errors in pane footer and status command - Clarify picker prompt precedence in review-flow.md Step 7b - Document $.fs.write unsandboxed boundary in docs/setup/secure-agent-setup.md --- docs/setup/secure-agent-setup.md | 15 + .../hooks/review-findings.test.ts | 283 +++++++++++- .../hooks/review-findings.ts | 417 +++++++++++++++--- plugins/magpie-pr-management/hooks/types.d.ts | 1 + plugins/magpie-pr-management/hooks/types.ts | 1 + .../skills/code-review/review-flow.md | 7 +- 6 files changed, 649 insertions(+), 75 deletions(-) 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 87688ecd7..7479e1069 100644 --- a/plugins/magpie-pr-management/hooks/review-findings.test.ts +++ b/plugins/magpie-pr-management/hooks/review-findings.test.ts @@ -20,7 +20,9 @@ import { describe, test } from 'claude-code/testing'; import { calculateSummary, + cleanCuratedFindingsFiles, cycleFindingAction, + findMatchingExistingFinding, formatActionIndicator, formatFindingDescription, formatKeepAnswer, @@ -39,6 +41,7 @@ import { normalizeSeverity, normalizeSource, parseFindings, + parseFindingTokens, resolveFindingIndex, resolveRelativePath, register, @@ -52,6 +55,7 @@ import { toggleFindingActionInState, truncateFilePath, truncateText, + updateAndSyncReviewFindingsState, updateFindingInState, updateSelectedIndexInState, } from './review-findings.ts'; @@ -82,6 +86,12 @@ function createMockEngine(overrides: any = {}): any { 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 { @@ -2014,7 +2024,7 @@ End of observations. assertStrictEqual(writtenKeep, 'ALL'); }); - test('handleBashToolCall sets findingsPath and writes curated files upon disposition', async () => { + 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' }, ]); @@ -2037,12 +2047,273 @@ End of observations. assert(state !== null); assertStrictEqual(state?.findingsPath, '/work/repo/saved/findings-999.json'); - // Verify curated files were synced to disk - const curatedJson = await test$.fs.read('/work/repo/saved/findings-999.curated.json'); - assert(curatedJson.includes('main.py')); + // 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')); - const curatedKeep = await test$.fs.read('/work/repo/saved/findings-999.curated.keep'); - assertStrictEqual(curatedKeep, 'ALL'); + // 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 () => { diff --git a/plugins/magpie-pr-management/hooks/review-findings.ts b/plugins/magpie-pr-management/hooks/review-findings.ts index 905d07634..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, @@ -858,6 +874,40 @@ export function getCuratedFindingsPaths(originalPath: string): { }; } +/** + * 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. @@ -865,9 +915,13 @@ export function getCuratedFindingsPaths(originalPath: string): { export async function syncCuratedStateToDisk( $: any, state: ReviewFindingsState | null -): Promise<{ jsonPath: string; keepPath: string } | null> { +): Promise<{ success: boolean; jsonPath?: string; keepPath?: string; error?: string }> { if (!state || !state.findingsPath || !state.findings) { - return null; + return { success: true }; + } + if (state.findings.length === 0) { + await cleanCuratedFindingsFiles($, state.findingsPath); + return { success: true }; } try { const { curatedJsonPath, curatedKeepPath } = getCuratedFindingsPaths(state.findingsPath); @@ -875,11 +929,47 @@ export async function syncCuratedStateToDisk( const keepContent = formatKeepAnswer(state.findings); await $.fs.write(curatedJsonPath, jsonContent); await $.fs.write(curatedKeepPath, keepContent); - return { jsonPath: curatedJsonPath, keepPath: curatedKeepPath }; - } catch { - // Fail-open: do not throw or halt execution on file write failure - return null; + 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; } /** @@ -1075,8 +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 syncCuratedStateToDisk($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } } @@ -1085,19 +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 syncCuratedStateToDisk($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } } else if (element === 'accept-all') { const updated = setBulkActionInState(state, 'accept'); - await setReviewFindingsState($, updated); - await syncCuratedStateToDisk($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } else if (element === 'skip-all') { const updated = setBulkActionInState(state, 'skip'); - await setReviewFindingsState($, updated); - await syncCuratedStateToDisk($, updated); + await updateAndSyncReviewFindingsState($, updated); return { handled: true }; } else if (element === 'prev-finding') { const updated = updateSelectedIndexInState(state, 'up'); @@ -1147,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). * @@ -1202,19 +1326,50 @@ export async function handleBashToolCall($: any, e: any, next: any): Promise 0) { const prNumber = extractPrNumberFromCommand(command); + + 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, findingsPath: effectivePath || rawPath, - findings: parsed.findings, - summary: parsed.summary, + findings: finalFindings, + summary: calculateSummary(finalFindings), selectedIndex: 0, }; await setReviewFindingsState($, nextState); - await syncCuratedStateToDisk($, 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); } } @@ -1229,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). * @@ -1288,52 +1511,78 @@ 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); - await syncCuratedStateToDisk($, 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); - await syncCuratedStateToDisk($, 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+([a-zA-Z0-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) - ); - const generalIdentifiers = new Set( - parts.filter((p) => /^[gG]\d+$/.test(p)) - ); + 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)) { + if (!hasAnchoredSpec) return f; const anchoredIndex = anchored.indexOf(f) + 1; return { ...f, - action: indicesToKeep.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), + action: anchoredIndices.has(anchoredIndex) ? ('accept' as const) : ('skip' as const), }; } - const unanchored = state.findings.filter((item) => !isAnchored(item)); - const genIdx = unanchored.indexOf(f) + 1; - if (generalIdentifiers.has(`g${genIdx}`) || generalIdentifiers.has(`G${genIdx}`)) { - return { ...f, action: 'accept' as const }; - } - return f; + if (!hasGeneralSpec) return f; + const genIndex = unanchored.indexOf(f) + 1; + return { + ...f, + action: generalIndices.has(genIndex) ? ('accept' as const) : ('skip' as const), + }; }); const updated: ReviewFindingsState = { @@ -1341,38 +1590,67 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str findings: newFindings, summary: calculateSummary(newFindings), }; - await setReviewFindingsState($, updated); - await syncCuratedStateToDisk($, 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+([a-zA-Z0-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) - ); - const generalIdentifiers = new Set( - parts.filter((p) => /^[gG]\d+$/.test(p)) - ); + 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)) { + if (!hasAnchoredSpec) return f; const anchoredIndex = anchored.indexOf(f) + 1; return { ...f, - action: indicesToDrop.has(anchoredIndex) ? ('skip' as const) : f.action, + action: anchoredIndices.has(anchoredIndex) ? ('skip' as const) : f.action, }; } - const unanchored = state.findings.filter((item) => !isAnchored(item)); - const genIdx = unanchored.indexOf(f) + 1; - if (generalIdentifiers.has(`g${genIdx}`) || generalIdentifiers.has(`G${genIdx}`)) { - return { ...f, action: 'skip' as const }; - } - return f; + if (!hasGeneralSpec) return f; + const genIndex = unanchored.indexOf(f) + 1; + return { + ...f, + action: generalIndices.has(genIndex) ? ('skip' as const) : f.action, + }; }); const updated: ReviewFindingsState = { @@ -1380,10 +1658,17 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str findings: newFindings, summary: calculateSummary(newFindings), }; - await setReviewFindingsState($, updated); - await syncCuratedStateToDisk($, 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}`, }; } @@ -1397,10 +1682,10 @@ export async function handleFindingsCommand($: any, e: any): Promise<{ text: str action: 'edit', body: newNote, }); - await setReviewFindingsState($, updated); - await syncCuratedStateToDisk($, 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.` }; diff --git a/plugins/magpie-pr-management/hooks/types.d.ts b/plugins/magpie-pr-management/hooks/types.d.ts index 65a974eb0..432cbda58 100644 --- a/plugins/magpie-pr-management/hooks/types.d.ts +++ b/plugins/magpie-pr-management/hooks/types.d.ts @@ -50,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 3512b147e..860661740 100644 --- a/plugins/magpie-pr-management/hooks/types.ts +++ b/plugins/magpie-pr-management/hooks/types.ts @@ -57,6 +57,7 @@ export interface ReviewFindingsState { 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 b62c1af34..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. -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. +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. @@ -344,7 +344,8 @@ uv run --project /tools/pr-management pr-management code-review rende ``` 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`. -When `.curated.keep` is present, pass its contents to `--keep`. + +**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.