Repository navigation
fix(setup): reconcile security issue-import caller grants - #1749
Conversation
|
Hi @potiuk , The fix for #1731 is ready for review. What changed:
Validation completed:
One clarification: although the issue mentions |
35f6178 to
259ac68
Compare
potiuk
left a comment
There was a problem hiding this comment.
Making reconcile check the import callers' grants, with an item-by-item confirmation and no implicit policy creation, is the right shape for #1731; the one thing to change is where the required grants come from, since a second copy of them will drift as soon as #1750 lands.
Smaller observations
- See inline comments on
plugins/magpie-setup/skills/setup/config.md:273,plugins/magpie-setup/skills/setup/reconcile.md:158.
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.
|
@KatalKavya96 I rebased this branch onto the current |
potiuk
left a comment
There was a problem hiding this comment.
A follow-up after a second read of the same commit: three more gaps in where and when the import-grant check runs, all minor. One correction to my earlier review as well: after the rebase, config.md does have ## Step 3a — Security tracker backend, and the grants block sits under it, so my comment on reconcile.md:158 saying the section does not exist was wrong; I have resolved it. Linking the anchor (config.md#step-3a--security-tracker-backend) would still help.
Smaller observations
- See inline comments on
plugins/magpie-setup/skills/setup/config.md:274,plugins/magpie-setup/skills/setup/config.md:284,plugins/magpie-setup/skills/setup/reconcile.md:153.
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.
potiuk
left a comment
There was a problem hiding this comment.
Marking this as changes requested, so the PR's status matches where it stands: the points in my earlier review (#1749 (review)) are still open and need a change, or a reply on their threads. This review adds nothing new to them.
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
left a comment
There was a problem hiding this comment.
Thanks for the fix, @KatalKavya96. The branch hasn't changed since my rebase on 2026-10-23 (it's still at 259ac688), so the inline threads from my earlier review are still open. The main one is still where the required grants come from. tools/vetted-ops/README.md already has these [callers] entries, and #1750 is about to change them. If config.md and reconcile.md point at that block instead of restating it, the copies can't drift. That also fixes the Jira / local-files gap for issue-import-from-md. The other two threads are about where the check runs: it currently only runs from Step 3a, and Step 3b still stamps the import skill when the grant diff is declined.
Two more things for the next push:
mainhas since added a### Personal tool configssubsection toconfig.mdat the spot where this PR inserts its block, so please rebase onto currentmainfirst, and re-check the setup case counts.- The
case-4-import-policy-grantsfixture tells the model what to return:report.mdlists each caller's required operations and ends with "Do not modify the policy or write a clean reconciliation stamp. Return the missing caller/grant proposals only." With that in the input, the case passes no matter whatreconcile.mdsays. Please remove those instruction lines. Ideally, give the README[callers]excerpt as context and let the model work out the missing grants, and add a case where the user declines to check that no clean stamp is written.
Separately: reconcile now writes the vetted-ops policy itself. Elsewhere it hands config gaps to setup config, and config.md now says the user saves the policy from a draft. Handing off to setup config <skill>, or showing the diff for the user to save, would keep that consistent.
|
@KatalKavya96, checking in: the head is still the rebase from 2026-10-23, and five review threads are open. The one to settle first is where the grants live. The The other threads still apply too: the check sitting inside Step 3a, the clean stamp after a declined grant diff, the fixed map missing
|
# Conflicts: # plugins/magpie-setup/skills/setup/config.md # tools/skill-evals/README.md # tools/skill-evals/evals/setup/README.md
|
@KatalKavya96, I merged
|
|
@potiuk I've pushed
Validation: All repository hooks passed, and all 5 step-reconcile model evaluations passed (5/5). Thanks for the detailed reviews! |
potiuk
left a comment
There was a problem hiding this comment.
Thanks for the update. The pointer to the canonical [callers] block, the fixed Step 3a anchor and the stamp rule in config are the right direction, but the change now loses grants main gives today, so it needs another round (inline below).
Three more, on the evals and merge order:
- Case 4 (
step-reconcile) now quotes a README excerpt that no longer matches this PR's README: it shows"security-issue-import" = ["rollup-append"]and expects "proposerollup-appendonly", while the README now listsissue-list-with-bodiestoo. - Case 5 does not test the decline. Its expected output is the same as a plain missing-grant case, and the extracted section (
reconcile.md§ The sweep) stops before the Step 2 decline handling, so a model that ignores the decline still passes. It needs Step 2 in the extraction, or an output field that records the decline. - The
step-reconcilerow intools/skill-evals/evals/setup/README.mdhas a stray cell (…) |, and a declined…). - Merge order: the README grants
issue-list-with-bodies, which only exists once #1750 lands; please keep this PR behind #1750, and note in the Jira / local-files lines that it is GitHub-only.
The threads were resolved without a reply on each; please answer on the thread when you push, so the review history shows what changed where.
the backend grants for the other security callers are gone (plugins/magpie-setup/skills/setup/config.md:328)
Grant the security callers
issue-create,issue-search,issue-list-label,issue-reopen,issue-body,issue-attach,tracker-doctor,tracker-pendingandtracker-publishin the same diff.
For caller permissions, use the canonical [vetted-ops `[callers]` example](../../../../tools/vetted-ops/README.md#configuration), including only the operations applicable to the selected backend.
This replaces main's instruction to grant the security callers their backend operations, but the README block it points at only lists additions for the three import callers. On Jira or local-files, setup config no longer tells the user to grant sync, fix, cve-allocate, deduplicate and the rest tracker-doctor, issue-body, issue-attach or tracker-publish, and those skills stop mid-run with "operation not permitted". Please keep the guidance for every security caller, either here or in the README block.
the import callers' backend list is incomplete (tools/vetted-ops/README.md:226)
Backend-specific additions for EACH of the three security-issue-import
callers above (keep all operations already listed for that caller):
# jira: tracker-doctor, issue-body, issue-attach
On a non-GitHub backend every tracker command the import skills run is a vetted operation, gated per caller. Besides these three, the import callers create trackers (issue-create, issue-import/apply.md, issue-import-from-md) and search for duplicates (issue-search, issue-list / issue-list-label, issue-comments, issue-view). A policy reconciled from this list cannot create a tracker on Jira or local-files. Please list the operations the three skills actually call on each backend.
Smaller observations
- See inline comments on
plugins/magpie-setup/skills/setup/config.md:347,plugins/magpie-setup/skills/setup/reconcile.md:235.
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.
|
Hi @potiuk, thanks for the detailed review! I've pushed another round of fixes in
Validation: All 8 reconciliation evaluations pass. Documentation, lint, formatting, and type-check hooks pass. The remaining local workspace pytest failures are unrelated macOS Unix-socket path limitations and an existing upstream template-drift mismatch; the affected socket tests pass with a shorter pytest base path. I've also kept the The changes are ready for another review. Thanks again! |
… Jira security-issue-import-from-md and security-issue-import-from-pr read the issue's labels with issue-view before regenerating the CVE JSON on a non-GitHub tracker, so a policy reconciled from the README example stopped at that step with "operation not permitted". Add issue-view to both callers' Jira lists (local-files inherits it), in both README copies. Generated-by: Claude Opus 5
potiuk
left a comment
There was a problem hiding this comment.
Thanks for working through every point, @KatalKavya96 — this round settles them all: the grants now come from the one [callers] example, every security caller keeps its backend guidance, the decline and the saved-policy check have their own eval suite, and the replies on each thread make the history easy to follow.
One small gap was left, and I fixed it on your branch rather than ask for another round (8dd0216):
tools/vetted-ops/README.mdandplugins/magpie-vetted-ops/tools/vetted-ops/README.md: addedissue-viewto the Jira lists forsecurity-issue-import-from-mdandsecurity-issue-import-from-pr. Both skills read the issue's labels withissue-viewbefore regenerating the CVE JSON on a non-GitHub tracker (issue-import-from-md/SKILL.md:170,issue-import-from-pr/SKILL.md:162), so without it a policy reconciled from this list stopped at that step with "operation not permitted".security-issue-importalready had it.
Please pull before pushing again.
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.
Summary
setup reconcileto detect missing callers or operations, propose a minimal policy diff, and require explicit confirmation before applying changes.Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-filespassesstep-reconcileevaluation: 4 passed, 0 failedcase-4-import-policy-grantsregression fixture addedRFC-AI-0004 compliance
Linked issues
Closes #1731
Notes for reviewers
The current import workflows require only:
security-issue-import:rollup-appendsecurity-issue-import-from-md:rollup-appendsecurity-issue-import-from-pr:pr-view-with-body,rollup-appendAlthough the issue mentions
body-field-set, no import workflow currently invokes it, so this PR intentionally does not grant it.The vetted-ops dispatcher and tracker procedures are unchanged. Policy updates remain subject to explicit user confirmation.
AI assistance was used for investigation, patch drafting, and review. Changes were applied and validated locally by the contributor.