Skip to content

fix(setup): reconcile security issue-import caller grants - #1749

Open
KatalKavya96 wants to merge 1 commit into
apache:mainfrom
KatalKavya96:fix/security-issue-import-vetted-caller
Open

KatalKavya96 wants to merge 1 commit into
apache:mainfrom
KatalKavya96:fix/security-issue-import-vetted-caller

Conversation

@KatalKavya96

Copy link
Copy Markdown
Contributor

Summary

  • Fix missing vetted-ops caller permissions for the three security issue-import workflows by documenting their exact minimum grants in Setup configuration.
  • Extend setup reconcile to detect missing callers or operations, propose a minimal policy diff, and require explicit confirmation before applying changes.
  • Add a reconciliation evaluation fixture covering all three import callers, extend the evaluation output contract, and update case counts.

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: Setup skill instructions and reconciliation evaluation contract

Test plan

  • prek run --all-files passes
  • Workspace pytest, Ruff check, Ruff format, and mypy passed
  • Groovy bridges — not changed
  • Model-based step-reconcile evaluation: 4 passed, 0 failed
  • New case-4-import-policy-grants regression fixture added

RFC-AI-0004 compliance

  • HITL — policy edits require explicit confirmation
  • Sandbox — no new unrestricted access
  • Vendor neutrality — existing adopter policy conventions preserved
  • Conversational + correctable — missing grants presented individually
  • Write-access discipline — no automatic permission changes; existing grants preserved
  • Privacy LLM — not affected

Linked issues

Closes #1731

Notes for reviewers

The current import workflows require only:

  • security-issue-import: rollup-append
  • security-issue-import-from-md: rollup-append
  • security-issue-import-from-pr: pr-view-with-body, rollup-append

Although 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.

@KatalKavya96

Copy link
Copy Markdown
Contributor Author

Hi @potiuk ,

The fix for #1731 is ready for review.

What changed:

  • Documented the minimum vetted-ops permissions for all three security issue-import callers.
  • Extended setup reconcile to detect missing callers and operations in existing policies.
  • Preserved explicit user confirmation before policy edits, existing permissions, and unrelated caller configurations.
  • Added a regression fixture covering all three missing-grant scenarios.

Validation completed:

  • prek run --all-files — all hooks passed.
  • Model-based step-reconcile evaluation — 4/4 passed, 0 failed.

One clarification: although the issue mentions body-field-set, none of the current import workflows invokes it, so it was intentionally excluded to maintain least-privilege access.
Thanks!

@github-actions github-actions Bot added family:setup setup-* skills family:tools tools/* family:docs Docs, MISSION.md, READMEs capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) substrate:framework-dev Tool substrate: build / validate / eval the framework itself labels Oct 21, 2026
@potiuk
potiuk force-pushed the fix/security-issue-import-vetted-caller branch from 35f6178 to 259ac68 Compare October 23, 2026 07:45

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread plugins/magpie-setup/skills/setup/reconcile.md
@potiuk

potiuk commented Oct 23, 2026

Copy link
Copy Markdown
Member

@KatalKavya96 I rebased this branch onto the current main (now at 259ac688), and force-pushed it. I resolved the two conflicts in the setup eval counts: the setup suite is now 151 cases across 32 steps, in both tools/skill-evals/README.md and tools/skill-evals/evals/setup/README.md. Please pull (git pull --rebase or reset to the remote branch) before you push again, so the rebase isn't undone.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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**.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • main has since added a ### Personal tool configs subsection to config.md at the spot where this PR inserts its block, so please rebase onto current main first, and re-check the setup case counts.
  • The case-4-import-policy-grants fixture tells the model what to return: report.md lists 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 what reconcile.md says. 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) family:docs Docs, MISSION.md, READMEs family:setup setup-* skills family:tools tools/* substrate:framework-dev Tool substrate: build / validate / eval the framework itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import skills use a security-issue-import vetted-ops caller that no policy template declares

2 participants