Skip to content

docs(projects): add Magpie's own pr-management-code-review criteria - #1042

Open
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:docs/magpie-review-criteria
Open

docs(projects): add Magpie's own pr-management-code-review criteria#1042
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:docs/magpie-review-criteria

Conversation

@potiuk

@potiuk potiuk commented Jul 30, 2026

Copy link
Copy Markdown
Member

Summary

  • pr-management-code-review resolves its rule sources from <project-config>/pr-management-code-review-criteria.md, but Magpie — which self-adopts the framework — never carried that file. Only projects/_template/ had it, so running the skill against apache/magpie silently fell back to a smaller default rule set instead of the project's own rules.
  • Adds projects/magpie/pr-management-code-review-criteria.md as a navigation map, not a rule set: every row points at a section that already exists in AGENTS.md, PRINCIPLES.md, CONTRIBUTING.md, or docs/editorial-guidelines.md. No rule text is duplicated and nothing moves out of AGENTS.md — per the framework's own guidance that summaries drift while links don't.
  • Found while running a review pass over the open-PR queue: the skill warned that the adopter config was missing and fell back to defaults for all seven PRs reviewed.

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: adopter config under projects/magpie/ (the live config, not the template)

Test plan

  • prek run --all-files passes
  • For Python packages touched: uv run pytest / ruff check / mypy passes — no Python touched
  • For Groovy bridges touched: command-line invocation tested end-to-end — n/a
  • For skill changes: eval suite passes for the affected skill — no skill logic changed; this is config the skill reads
  • For skill behaviour changes: a new or updated eval fixture is included — no behaviour change; see note below
  • Other: every link and #anchor in the new file is verified by the lychee and markdownlint (MD051) hooks — this was the main correctness risk, since the file is almost entirely cross-references into AGENTS.md and PRINCIPLES.md anchors.

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — no new unrestricted host access; network reach declared in the adapter
  • Vendor neutrality — this file is where concrete Magpie values are supposed to live, per PRINCIPLES §12 ("the framework is project-agnostic; concrete names live in adopter config"). It moves nothing concrete into the framework.
  • Conversational + correctable — the file is adopter config, so a project can tune its own review criteria map without touching framework code.
  • 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

None — the gap was found during a review session rather than reported. Happy to file a tracking issue retroactively if that's preferred.

Notes for reviewers (optional)

Three notes worth a second opinion:

  1. Two canonical categories have no row. Database / query correctness and UI (React/TypeScript) don't apply — Magpie ships neither, so a finding in either would be a mis-classification rather than something to link. Worth knowing the trade-off: criteria.md says a category with no anchor row makes the skill "fall back to a plain reference and surface the missing anchor as a one-line warning at the top of the review". If that warning turns out to be noisy in practice, the fix belongs in the framework — the skill arguably shouldn't warn about categories an adopter has deliberately declined — rather than in this file.

  2. Navigation map vs. moving text out of AGENTS.md. I read the intent as "point at the criteria that already exist" rather than "relocate them", since the template calls this file a navigation map and criteria.md explicitly says to link rather than summarise. AGENTS.md is unchanged. If you'd rather the review-specific rules physically moved here, that's a different (larger) refactor and I'd do it separately.

  3. Anchor targets for the vaguer categories. Code qualityAGENTS.md#before-submitting and API correctnessAGENTS.md#reusable-skills are the best fits I found, but they're the two I'm least sure about; Magpie has no standalone review-instructions doc to anchor into. Architecture-boundary and generated-file findings route to the numbered PRINCIPLES entries (§12, §13), which I'm confident about.

Also adds a Magpie-specific anchor block for categories this repo's reviews actually need but the canonical list doesn't have: placeholder convention, external-content-as-data, privacy-LLM routing, labelling, vendor neutrality, and the inline-comment review default.

`pr-management-code-review` resolves its rule sources from
`<project-config>/pr-management-code-review-criteria.md`, but Magpie —
which self-adopts the framework — never carried that file. Only
`projects/_template/` had it, so running the skill against apache/magpie
silently fell back to a smaller default rule set instead of the project's
own rules. Found while running a review pass over the open-PR queue.

This is a navigation map, not a rule set: every row points at a section
that already exists in AGENTS.md, PRINCIPLES.md, CONTRIBUTING.md, or
docs/editorial-guidelines.md, so the skill can quote the source rule
verbatim. No rule text is duplicated and nothing moves out of AGENTS.md —
per the framework's own guidance that summaries drift while links do not.

Notable choices:

- Architecture-boundary and generated-file findings route to the
  numbered PRINCIPLES entries (§12 project-agnosticism, §13
  snapshot-plus-override) since that is where those rules live.
- Categories that do not apply to this repository (database/query
  correctness, UI React/TypeScript) have no row: Magpie ships neither a
  database layer nor a UI, so a finding in either category would be a
  mis-classification rather than something to link.
- A Magpie-specific block adds the categories this repo's reviews
  actually need — placeholder convention, external-content-as-data,
  privacy-LLM routing, labelling, vendor neutrality, and the
  inline-comment review default.
- Backports are recorded as not applicable: Magpie releases from a
  single `main` and keeps no release-train branches.

Every link and `#anchor` is verified — lychee and markdownlint pass, and
the full `prek run --all-files` suite is green.

Generated-by: Claude Code (Opus 5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant