feat(pr-management): connect findings pane curation to render and polish review hooks by Kaap10 · Pull Request #2005 · apache/magpie

Kaap10 · 2026-11-05T03:23:16Z

Summary

Follow-up to #1992 and maintainer review on #2005 for the magpie-pr-management code-review mod:

Connect curated findings to render: Syncs <file>.curated.json and <file>.curated.keep beside findings upon maintainer curation actions, allowing Step 7 render to consume curated comments directly.
Picker prompt precedence: Step 7b explicitly clarifies that the maintainer's typed answer at Step 7a text picker always takes precedence over sidecar recommendations.
In-pane button synchronization: Routes all row toggle, All, and None button onPress callbacks through updateAndSyncReviewFindingsState to keep UI state and disk in sync.
Preserve curation across disposition: Reconciles incoming findings with existing state across disposition re-runs to preserve maintainer actions and customized bodies.
Failure visibility and orphan cleanup: Surfaces syncError in pane footer and /magpie-findings status if disk writes fail, cleans up partial files on write errors, and removes orphaned sidecars when findings are empty.
Strict token parsing: Strictly validates keep and drop arguments (^\d+$ or ^[gG]\d+$), rejects arbitrary strings like abc, and decouples gN general observations from anchored inline comments.
Document $.fs.write boundary: Added a dedicated subsection in docs/setup/secure-agent-setup.md detailing unsandboxed mod write boundaries, rationale, locality constraints, and fail-open design.

Type of change

Skill change (.claude/skills/<name>/)
Tool / bridge contract (tools/<system>/*.md)
Python package (tools/*/ with pyproject.toml)
Groovy reference impl
Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
Documentation (docs/, README.md, CONTRIBUTING.md)
Project template (projects/_template/)
CI / dev loop (prek, workflows, validators)
Other: Claude Code mod plugin hooks & tests (plugins/magpie-pr-management/hooks/)

Test plan

claude plugin test plugins/magpie-pr-management passes all 71 unit and integration tests (0 failures).
claude plugin validate --strict plugins/magpie-pr-management passes static analysis with 0 errors and 0 warnings.
python tools/dev/add-license-headers.py --check passes cleanly.
In-pane button onPress callbacks verified to sync state and disk files.
Disposition re-run verified to retain existing maintainer curation without writing sidecars upfront.
Token parser verified to reject invalid tokens (keep abc) and decouple general observations (keep g1).
Write failures verified to record syncError, clean up partial files, and display visible warnings.
Line endings verified strictly LF (crlf=0) across all touched files.

RFC-AI-0004 compliance

HITL — The interactive pane and /magpie-findings command curate drafted findings for the maintainer; review posting remains gated on maintainer confirmation in Step 7b and Step 8.
Sandbox — Files are written only to the local session path via $.fs.write.
Vendor neutrality — Step 7a/7b document the .curated.* path for the Claude Code mod while preserving the standard sequential text picker prompt for other harnesses.
Conversational + correctable — Maintainers can adjust or reset actions at any point before running render.
Write-access discipline — No automated GitHub review postings.
Privacy LLM — Zero tokens consumed by the interactive pane and /magpie-findings command.

Linked issues

Refs #1887
Follow-up to #1992

Notes for reviewers (optional)

Directly addresses @potiuk's review comments on PR #2005:

Initial disposition execution no longer writes curated files upfront; files are written only upon maintainer curation. Step 7b explicitly states that typed picker answers take precedence.
In-pane buttons (onPress callbacks for row toggles, All, None) now route through updateAndSyncReviewFindingsState.
Disposition re-runs preserve maintainer actions and customized review bodies across runs.
Failed writes surface syncError in pane footer and /magpie-findings status, partial files are cleaned up on write errors, and orphaned sidecars are purged when findings become empty.
keep and drop enforce strict token validation and decouple general observations (gN) from anchored inline comments.
Added documentation in docs/setup/secure-agent-setup.md covering the $.fs.write unsandboxed host boundary.

…ish 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 <file>.curated.json and <file>.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

Kaap10 · 2026-11-05T03:33:55Z

up for review

potiuk

Thanks for picking up the #1992 notes: the docstring move and the skip filter are good. This change decides what actually gets posted, though, and the wiring can post something the maintainer dropped, so the inline comments below need answering first. Please also cover $.fs.write in docs/setup/secure-agent-setup.md: it runs outside the sandbox, this is its first use in a Magpie mod, and docs/when-to-use-mods.md requires those trade-offs to be documented.

The keep file overrides the picker (`plugins/magpie-pr-management/skills/code-review/review-flow.md:347`)

handleBashToolCall writes the curated files at disposition, before anyone curates, so .curated.keep normally holds ALL. This line then passes it whenever it is present. A maintainer who answers 1,3 at the picker still sends ALL, so an inline comment they dropped gets posted. The PR also removes the sentence saying only the picker's answer reaches render, and nothing now says which one wins. Please write these files only after a real curation action, and state which answer takes precedence.

The pane's own buttons do not sync (`plugins/magpie-pr-management/hooks/review-findings.ts:1079`)

The sync is added to handleUiPress, but the pane's row toggles and All/None buttons use their own onPress callbacks in renderFindingsPane (lines 222, 298, 372 and 381), and those call only setReviewFindingsState. The pane then shows one selection while render gets another. The new test calls handleUiPress directly, so it does not catch this. One shared helper that sets the state and syncs would cover both paths.

Re-running disposition wipes curation (`plugins/magpie-pr-management/hooks/review-findings.ts:1213`)

Every disposition re-reads the original findings and overwrites the curated files with fresh state. review-flow.md requires a re-run after [E]dit and after CI actions, so those ordinary paths lose the maintainer's edits and selections. Hydrating should not overwrite existing curation for the same findings.

Smaller observations

See inline comments on plugins/magpie-pr-management/hooks/review-findings.ts:880, plugins/magpie-pr-management/hooks/review-findings.ts:1312.

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

potiuk · 2026-11-05T15:26:50Z

 ```

+When `<file>.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 `<file>.curated.keep` is present, pass its contents to `--keep`.


The keep file overrides the picker

handleBashToolCall writes the curated files at disposition, before anyone curates, so .curated.keep normally holds ALL. This line then passes it whenever it is present. A maintainer who answers 1,3 at the picker still sends ALL, so an inline comment they dropped gets posted. The PR also removes the sentence saying only the picker's answer reaches render, and nothing now says which one wins. Please write these files only after a real curation action, and state which answer takes precedence.

Fixed. Initial disposition no longer writes curated files to disk upfront - they are only written after an explicit curation action in the pane or via command.

We also updated review-flow.md Step 7b to make the precedence rule clear: the answer typed by the maintainer at the Step 7a picker prompt always takes precedence and goes directly to --keep <answer>.

potiuk · 2026-11-05T15:26:50Z

      if (targetIdx !== -1) {
        const updated = toggleFindingActionInState(state, targetIdx);
        await setReviewFindingsState($, updated);
+        await syncCuratedStateToDisk($, updated);


The pane's own buttons do not sync

The sync is added to handleUiPress, but the pane's row toggles and All/None buttons use their own onPress callbacks in renderFindingsPane (lines 222, 298, 372 and 381), and those call only setReviewFindingsState. The pane then shows one selection while render gets another. The new test calls handleUiPress directly, so it does not catch this. One shared helper that sets the state and syncs would cover both paths.

Fixed. Created a shared helper updateAndSyncReviewFindingsState that updates the state atom and writes to disk.

All in-pane button onPress callbacks (row toggles, All, None) now route through this helper. Added a test that directly invokes these button callbacks to verify disk synchronization.

potiuk · 2026-11-05T15:26:50Z

-              });
+              };
+              await setReviewFindingsState($, nextState);
+              await syncCuratedStateToDisk($, nextState);


Re-running disposition wipes curation

Every disposition re-reads the original findings and overwrites the curated files with fresh state. review-flow.md requires a re-run after [E]dit and after CI actions, so those ordinary paths lose the maintainer's edits and selections. Hydrating should not overwrite existing curation for the same findings.

Fixed. Added findMatchingExistingFinding during disposition hydration.

When disposition re-runs after [E]dit or CI checks, it matches incoming findings to previous state by ID, file:line, rule, or body, preserving any existing actions (skip, edit) and customized notes.

potiuk · 2026-11-05T15:26:50Z

+    await $.fs.write(curatedKeepPath, keepContent);
+    return { jsonPath: curatedJsonPath, keepPath: curatedKeepPath };
+  } catch {
+    // Fail-open: do not throw or halt execution on file write failure


Failed writes are silent

A failed write returns null and every caller ignores it, while the two files are written separately, so they can end up mismatched or stale, and Step 7b trusts them. Please tell the maintainer when the sync fails. Also, when findings are rewritten to [], the earlier curated files stay behind and Step 7b can pick them up.

Fixed. Added syncError to state. If a disk write fails, an explicit warning is displayed in the pane footer and /magpie-findings status.

Also:

If a write fails midway, any partial file is cleaned up so .curated.json and .curated.keep never end up mismatched.

When findings become empty ([]), existing curated files are automatically deleted from disk so stale findings are not used.

potiuk · 2026-11-05T15:26:50Z

  }

-  const keepMatch = rawArgs.match(/^keep\s+([0-9,\s]+)$/i);
+  const keepMatch = rawArgs.match(/^keep\s+([a-zA-Z0-9,\s]+)$/i);


keep and drop now accept letters

Widening the pattern for g1 means keep abc, which used to be rejected, now skips every inline comment, and keep g1 does the same. Please reject tokens that are neither numbers nor gN, and decide whether keep g1 should touch anchored findings at all. review-flow.md should also mention the gN form.

Fixed. Added parseFindingTokens with strict validation: every token must match ^\d+$ or ^[gG]\d+$. Invalid strings like abc are rejected with an error message without changing state.

We also decoupled them:

keep 1,3 only updates anchored inline comments and leaves general observations untouched.

keep g1 only updates general observations and leaves anchored comments untouched.

Mentioned the gN form in review-flow.md Step 7a as well.

@potiuk

…ration bridge Address review comments from @potiuk on PR apache#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

@potiuk

…ration bridge Address review comments from @potiuk on PR apache#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

github-actions Bot added family:pr-management pr-management-* skills capability:review Deep per-item code review or contributor mentoring labels Nov 5, 2026

potiuk requested changes Nov 5, 2026

View reviewed changes

github-actions Bot added family:setup setup-* skills family:docs Docs, MISSION.md, READMEs labels Nov 6, 2026

Kaap10 force-pushed the feat/pr-management-curated-render-bridge branch from c5a4699 to ed9df03 Compare November 6, 2026 04:02

Kaap10 requested a review from potiuk November 6, 2026 04:12

Navigation Menu

Repository navigation

feat(pr-management): connect findings pane curation to render and polish review hooks - #2005

feat(pr-management): connect findings pane curation to render and polish review hooks#2005
Kaap10 wants to merge 2 commits into
apache:mainfrom
Kaap10:feat/pr-management-curated-render-bridge

Kaap10 commented Nov 5, 2026 •

edited

Loading

Uh oh!

Kaap10 commented Nov 5, 2026

Uh oh!

potiuk left a comment

Uh oh!

potiuk Nov 5, 2026

Uh oh!

Kaap10 Nov 6, 2026

Uh oh!

potiuk Nov 5, 2026

Uh oh!

Kaap10 Nov 6, 2026

Uh oh!

potiuk Nov 5, 2026

Uh oh!

Kaap10 Nov 6, 2026

Uh oh!

potiuk Nov 5, 2026

Uh oh!

Kaap10 Nov 6, 2026

Uh oh!

potiuk Nov 5, 2026

Uh oh!

Kaap10 Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Kaap10 commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Test plan

RFC-AI-0004 compliance

Linked issues

Notes for reviewers (optional)

Uh oh!

Kaap10 commented Nov 5, 2026

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

The keep file overrides the picker (plugins/magpie-pr-management/skills/code-review/review-flow.md:347)

The pane's own buttons do not sync (plugins/magpie-pr-management/hooks/review-findings.ts:1079)

Re-running disposition wipes curation (plugins/magpie-pr-management/hooks/review-findings.ts:1213)

Smaller observations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kaap10 commented Nov 5, 2026 •

edited

Loading

The keep file overrides the picker (`plugins/magpie-pr-management/skills/code-review/review-flow.md:347`)

The pane's own buttons do not sync (`plugins/magpie-pr-management/hooks/review-findings.ts:1079`)

Re-running disposition wipes curation (`plugins/magpie-pr-management/hooks/review-findings.ts:1213`)