feat(pr-management): add review findings data model and parser (part of #1887) - #1920

Thanks for splitting #1887 into three, Kaap10; a small, well-tested parser first is the right foundation for the pane. Since the pane's output is what decides which comments get posted, though, the bridge to render has to agree with it exactly, and two places do not yet (inline below): the keep answer numbers comments over a different list than render does, and an edited finding is dropped from the answer. Both would post a review the maintainer did not pick.

The other inline comments are smaller and fine in the same push.

One question rather than a finding: register adds a pass-through session.start handler, so every session of every magpie-pr-management user loads this module before anything uses it. If it is there only so the mod is wired up and its tests run in CI, a short comment saying so would help; otherwise the hooks.json entry could wait for PR-2, when there is a pane to show.

the keep answer numbers comments differently from render (plugins/magpie-pr-management/hooks/review-findings.ts:160)

Converts finding action states into inline-picker keep answer format

• compatible with code-review render keep parameter.

const anchored = findings.filter(
    (f) => f.line !== null && f.line !== undefined && f.line >= 1
  );

render numbers the inline comments over the raw findings file, anchored = [f for f in findings if f.get("line")] (tools/pr-management/src/pr_management/code_review/cli.py), while this numbers them over the parsed list. The two disagree whenever the parser normalises: a finding carrying lineNumber or line_number is anchored here and not there, "0" is anchored there and not here, and an item the parser drops as "not an object" shifts every later index. One disagreement moves 1,3 onto different comments, so the review posts comments the maintainer skipped and drops ones they kept. Please number over exactly the list render reads (the pane writes the normalised findings file that render then consumes, or the anchoring predicate is render's on the raw item), and add a test that runs both rules over the same input.

an edited finding is dropped from the review (plugins/magpie-pr-management/hooks/review-findings.ts:171)

The inline picker's answer: A, N, 1,3 (keep), -2,-3 (drop), E 2 (edit).

if (item && item.action === 'accept') {
      acceptedIndices.push(i + 1);

Only accept counts as kept, so a finding the maintainer marked edit is left out of the answer and never posted. In the picker grammar an edit keeps the comment (E 2 keeps every comment and edits number 2). Please treat edit as kept here, and leave how the edited text reaches render to PR-3, with a test for a mixed accept/edit/skip list.

Smaller observations

• See inline comments on plugins/magpie-pr-management/hooks/review-findings.ts:108, plugins/magpie-pr-management/hooks/review-findings.ts:47, plugins/magpie-pr-management/hooks/review-findings.ts:77, plugins/magpie-pr-management/hooks/review-findings.ts:270.

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.

Thanks for the quick turnaround — the edit, unknown-action, severity, malformed-line and indentation fixes are all in and tested, and the session.start docstring answers my question. Rather than send this back for another round, I rebased the branch onto main and pushed one fixup commit, abe9a4e, for what was left (please pull before pushing again):

• hooks/review-findings.ts — serializeFindingsForRender wrote body / rule / fixup, which render never reads, so every inline comment rendered from the file would have come out as a bare title. It now keeps every field of the original finding (explanation, rule_id, quoted_rule, excerpt, suggestion_block, side, reviewers, …), drops the parser-only aliases, and overrides only the normalised file / line / severity; a finding that arrived with only body / rule gets them as explanation / rule_id.
• hooks/review-findings.test.ts — a full Step-4 finding now round-trips with every key render reads, and the parity test runs render's own rule over the serialised file on the inputs where the raw keys diverge (lineNumber / line_number, the string "0", a non-object item). 21 tests pass under claude plugin test.
• hooks/review-findings.ts — a numeric non-integer or negative line and a non-string severity are now recorded in errors, like their string counterparts.
• .codex-plugin/plugin.json — picks up the version main moved to in #1934 (check-family-plugins --fix), which would otherwise have failed the main build after merge.

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.