[Kaap10]

Summary

• Add the presentational interactive findings pane UI mod for magpie-pr-management (Part 2 of 3 for feat(pr-management): add interactive review findings pane Claude Code mod (part of #1497) #1887, following merged PR feat(pr-management): add review findings data model and parser (part of #1887) #1920, parent issue Explore Claude Code mods across the skill families #1497).
• In this stage (Part 2 of 3), the pane renders test fixture payloads; in a real session nothing fills it until Part 3 wires the live session feed from the code-review skill flow.
• Deliver renderFindingsPane via ui.render using Ink Box and Text primitives to display categorized findings (anchored comments numbered to match formatKeepAnswer, plus unanchored general observations).
• Provide visual formatting helpers including color-coded severity badges ([BLOCKING], [MAJOR], [MINOR], [NIT]), action toggling indicators ([x] Accept, [ ] Skip, [E] Edit), and deep path truncation.
• Declare self-contained ambient types in plugins/magpie-pr-management/hooks/types.d.ts for PluginState['magpie-pr-management'].reviewFindingsState satisfying strict mod manifest validation.

Type of change

• Skill change (.claude/skills/<name>/) — eval fixtures updated below
• 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 UI component (plugins/magpie-pr-management/hooks/)

Test plan

• Strict static mod manifest validation passes cleanly: claude plugin validate --strict plugins/magpie-pr-management.
• Mod unit test suite expanded from 14 to 34 tests passing cleanly: claude plugin test plugins/magpie-pr-management (34 pass, 0 fail).
• Sibling family mods (magpie-security, magpie-setup, magpie-utilities) strictly validated.
• Local pre-commit hygiene checks verified (LF line endings, zero trailing whitespace, clean EOF, no typos, no emojis, no double hyphens).

RFC-AI-0004 compliance

• HITL — any new mutation is gated on explicit user confirmation
• Sandbox — no new unrestricted host access; mod uses zero node built-ins and complies with Claude Code mod sandbox constraints
• Vendor neutrality — mod is strictly additive
• Conversational + correctable — review findings remain fully inspectable and correctable
• Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
• Privacy LLM — private content does not reach a non-approved LLM; redactor invoked where needed

Linked issues

• Part of feat(pr-management): add interactive review findings pane Claude Code mod (part of #1497) #1887 (feat(pr-management): add interactive review findings pane Claude Code mod)
• Follows feat(pr-management): add review findings data model and parser (part of #1887) #1920 (Part 1 of feat(pr-management): add interactive review findings pane Claude Code mod (part of #1497) #1887)
• Related to Explore Claude Code mods across the skill families #1497 (Explore Claude Code mods across the skill families)

Notes for reviewers (optional)

This is PR-2 of the 3-part split for #1887:

• PR-1 (Merged feat(pr-management): add review findings data model and parser (part of #1887) #1920): Data model, findings parser, Codex compatibility manifests, and render serialization bridge.
• PR-2 (This PR): Presentational UI component (renderFindingsPane), severity badges, action indicators, path truncation, and ambient type definitions.
• PR-3 (Upcoming): Interactive action toggling handlers and review-flow.md Step 7a skill integration.

[Kaap10]

up for review!

[potiuk]

Oh. Wow. Let me dog-food and review it using itself

[potiuk]

This looks really good — the pane is cleanly structured, numbers inline comments exactly as formatKeepAnswer and render do, and falls through to the next hook on every failure path. I had hoped to dog-food it and run this very review through the pane, but as part 2 of 3 nothing feeds it yet (state only arrives from a session.start payload Claude Code does not send), so this review was done the usual way; that is expected at this stage, and the first inline comment is only about saying so. The rest are small: one handler registered on two surfaces, escaping PR-quoted text before it reaches the terminal, and a few nits. Looking forward to part 3.

Smaller observations

• See inline comments on plugins/magpie-pr-management/hooks/review-findings.ts:746, plugins/magpie-pr-management/hooks/review-findings.ts:766, plugins/magpie-pr-management/hooks/review-findings.ts:253, plugins/magpie-pr-management/hooks/review-findings.ts:331, plugins/magpie-pr-management/hooks/review-findings.ts:308, plugins/magpie-pr-management/hooks/types.d.ts:20.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

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

[Kaap10]

Thanks for the thorough review, @potiuk!

1. State hydration (review-findings.ts:746): Routed session.start through setReviewFindingsState so $.ui.invalidate fires, and noted in the docstring and PR description that payload hydration is scoped to PR-2 test fixtures (PR-3 wires the live session feed).
2. Single surface (review-findings.ts:766): Dropped AbovePrompt registration to avoid double rendering; registered solely on ui.render (Pane) and aligned docstrings.
3. Content sanitization (review-findings.ts:253): Added sanitizeFindingText helper stripping ANSI CSI/OSC escapes and C0/C1 control characters from file, rule, and body before reaching Text or the fallback, with a unit test verifying plain text rendering.
4. UI text (review-findings.ts:331): Removed (scroll or select in PR-3) note; now displays clean count: ... showing 10 of 14 findings.
5. Pane and fallback agreement (review-findings.ts:308): Unified description formatting via formatFindingDescription (rule — body with truncation) across both surfaces.
6. State type synchronization (types.d.ts:20): Kept ambient declaration self-contained as required by strict plugin validation, added cross-referencing comments, and added an automated parity unit test.

[potiuk]

Thanks for working through the last round: the single surface, the sanitising and the overflow text are all in place. A few things are still open, mostly around state that only the tests exercise.

Smaller observations

• PR description — The docstring now says the session feed comes in part 3, but the Summary still describes a pane that renders findings. In a real session nothing fills it until part 3; one line in the Summary saying so would match the code.
• See inline comments on plugins/magpie-pr-management/hooks/review-findings.test.ts:875, plugins/magpie-pr-management/hooks/review-findings.ts:60, plugins/magpie-pr-management/hooks/review-findings.ts:153, plugins/magpie-pr-management/hooks/review-findings.ts:134.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

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

[Kaap10]

Thanks @potiuk for the round 2 review! All points addressed:

1. State atom isolation: Removed the module-level currentFindingsState fallback entirely. Production code now strictly reads and writes the reviewFindingsState atom, and session.start resets the atom to null if the event carries no findings.
2. Terminal fallback: Dropped formatFindingsTerminalFallback and its test cases until Part 3 introduces an active caller.
3. Line breaks sanitization: Replaced runs of \r, \n, and \t with a single space before control-character stripping, preserving word boundaries (Line1 Line2).
4. Type contract sync test: Dropped the tautological parity test block and cleaned the header comments in types.ts and types.d.ts.
5. PR Description: Clarified in the summary that in Part 2 the pane renders test fixtures, and Part 3 wires the live session feed.

[potiuk]

Thanks for the third round, @Kaap10 — the pane now reads only the plugin atom, the unused terminal fallback and the tautological parity test are gone, line breaks become spaces before control characters are stripped, and the Summary says plainly that the pane stays empty until part 3 wires the session feed. I've resolved the six threads from the earlier rounds against 568cf20.

Keeping the ambient copy in types.d.ts is fine given strict plugin validation needs it self-contained; the keep-in-sync comments in both files are the right guard until a check can read the file.

Next time, a short reply on each thread when you push makes it easier to see what changed where.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

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