Repository navigation
fix(setup): reconcile security issue-import caller grants - #1749
KatalKavya96 wants to merge 1 commit into
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.
| and write it only on the user's confirmation; the policy is a security control, not configuration this step may change unasked. | ||
| Grant the security callers `issue-create`, `issue-search`, `issue-list-label`, `issue-reopen`, `issue-body`, `issue-attach`, `tracker-doctor`, `tracker-pending` and `tracker-publish` in the same diff. | ||
|
|
||
| For the security issue-import workflows, also check the policy's `[callers]` |
There was a problem hiding this comment.
These grants already live in the [callers] example in tools/vetted-ops/README.md, and #1750 adds issue-list-with-bodies there. With a second copy here, whichever PR merges second leaves this stale, and reconcile's "do not add other permissions" would then refuse the grant import needs. Please point Step 3 / check 3a at the README block instead of restating it.
|
@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.
| Grant the security callers `issue-create`, `issue-search`, `issue-list-label`, `issue-reopen`, `issue-body`, `issue-attach`, `tracker-doctor`, `tracker-pending` and `tracker-publish` in the same diff. | ||
|
|
||
| For the security issue-import workflows, also check the policy's `[callers]` | ||
| table. When those workflows are configured, propose exactly these grants: |
There was a problem hiding this comment.
This sits inside Step 3a, which line 217 runs only when tracker.platform is unset or TODO, so setup config on a project whose tracker is already configured never checks the import grants. Move it out of Step 3a, or say config leaves it to reconcile.
|
|
||
| These are separate caller identities. Do not substitute one for another or | ||
| grant `body-field-set` to an import caller: none of these workflows invokes it. | ||
| Show the proposed policy diff and apply it **only on explicit user confirmation**. |
There was a problem hiding this comment.
reconcile.md 3a says a declined grant must not be stamped clean, but Step 3b here stamps every skill whose requires_config resolves, so declining this diff still records the import skill as reconciled. Please carry the same rule into Step 3b for the import callers.
| name>` requires `<file>`, which is not configured"* — propose | ||
| `/magpie-setup:setup config <skill>` for it. | ||
|
|
||
| 3a. **Import caller policy grants.** When an existing vetted-ops policy |
There was a problem hiding this comment.
On Jira / local-files, issue-import-from-md also needs tracker-doctor, issue-body, tracker-pending and tracker-publish under its own caller (issue-import-from-md/SKILL.md:160-171). With this fixed mapping and "do not add other permissions", reconcile passes a policy the import cannot run with. Another reason to source the grants from the vetted-ops README.
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.
| - `security-issue-import-from-pr` requires `pr-view-with-body` | ||
| and `rollup-append`. | ||
|
|
||
| No one has confirmed changes to the vetted-ops policy. |
There was a problem hiding this comment.
These lines tell the model the expected result (no policy change, no clean stamp, proposals only), so the case can't fail on the skill text. Please drop them, and either drop the "requires …" list above or replace it with the vetted-ops README [callers] excerpt so the model has to derive the missing grants.
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.