fix(plugins): carry pure applies into runs and honor grants - #6187
fix(plugins): carry pure applies into runs and honor grants#6187itlackey wants to merge 2 commits into
Conversation
|
Thanks @itlackey — this is a nicely scoped fix for the two issue paths in #6186: carrying pure plugin applies into the next run, and preserving persisted capability grants instead of rebuilding defaults. I've routed it for pool review, and because it changes a user-facing run path we'll also keep the QA gate in place before merge.
|
lefarcen
added
size/M
lefarcen
added
the
needs-validation
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we’ll loop QA in once it’s merge-ready (and design/product have signed off, where applicable). |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@itlackey I reviewed the full changed-range diff and verified both bug paths: pure plugin applies now carry the plugin identity and inputs into run creation when no snapshot ID exists, while persisted capability grants and revocations remain authoritative with the prompt-injection floor intact. The shared request contract, both web send paths, daemon trust/apply behavior, and regression coverage are consistent. Local validation passed with repository guard, root typecheck, 126 focused web tests, 500 daemon plugin tests, and the contracts typecheck. Thank you for the tight reproduction and thoughtfully scoped fix—nice work! 🙌
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Thanks @itlackey — the current head is approved on code review now. I’ve queued QA on the affected run/plugin path next, and once that validation is in this should be in good shape to merge. |
|
Thank you for the quick response and getting this into the queue. Let me know if you need anything from me. |
|
Thanks @itlackey — nothing else needed from you at the moment. We’re just waiting on the QA pass for this head now, and we’ll follow up here if anything comes up during validation. |
AmyShang-alt
added
validated
AmyShang-alt
left a comment
AmyShang-alt
left a comment
There was a problem hiding this comment.
QA 验收通过。\n\n验证证据：\n- Web 定向测试：126/126 passed\n- daemon 插件测试：69 files，500/500 passed\n- contracts：build + 246/246 tests passed\n- pnpm guard passed\n- 全仓 pnpm typecheck passed\n- daemon build passed\n- Electron + daemon 真实运行验证通过\n- 官方插件 design-system-airtable：pure apply 返回空 snapshotId，POST /api/runs 携带 pluginId/pluginInputs，创建并 pin 新 snapshot\n- 真实 Open Design AMR/deepseek-v4-flash run 成功，产出并预览 airtable-landing-test.html\n- Codex CLI 真实 run 也成功，聊天记录已持久化可见\n\nPR head: 0c5bf38
|
@itlackey friendly reminder: this PR appears to be waiting on author action (failing CI) and has had no human activity for more than 3 days. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no human activity for more than 5 days may be closed automatically, but they can be reopened when work resumes. |
|
@mrcfps or @AmyShang-alt can someone re-run the skipped CI to unblock this PR? |
|
Thanks for flagging this, @itlackey — I’ve re-ran the CI workflow for the current head so the skipped jobs can be evaluated again. Once the new run settles, we’ll have a clearer read on whether anything else is needed to unblock merge. |
Visual regression reviewHead: 4 changed · 42 unchanged · 0 new without baseline · 0 failed Changed cases
Unchanged cases
Visual diff is advisory only and does not block merging. |
|
@lefarcen could you push this through the last check? |
|
Thanks @itlackey — I checked the current head and the rerun is green again, and QA is already validated here. I don’t see anything else pending from your side at this point; this is on the maintainer side for the final merge step. |
|
@itlackey — there's a merge conflict blocking this PR from landing. GitHub's merge state is currently To unblock: git fetch upstream main
git rebase upstream/main
# resolve any conflicts that surface, then
git push --force-with-lease origin fix/plugin-snapshot-capabilitiesOnce the rebase lands and CI passes green, this should be merge-ready — no further review needed from your side. |
d7aae41 to
13ddd70
Compare
|
@lefarcen I have rebased and pushed. Let me know if you see any issues. |
|
@itlackey — the rebase landed cleanly, thanks for turning that around quickly. I've re-requested mrcfps's review on this head since the previous approval was on the old commit; CI is running and should resolve shortly. Nothing else needed from your side at this point. 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@itlackey Thanks for a focused, well-tested fix on a high-impact plugin path. I reviewed the full changed-range diff for pure-apply run transport and capability grant/revoke behavior.
What I verified
- Pure apply with an empty
snapshotIdnow forwardspluginId+pluginInputsfrom both ProjectView send paths throughstreamViaDaemon, so/api/runscan create and pin a snapshot instead of dropping the selection or reusing an old pin. applyPlugintreats persistedcapabilitiesGrantedas authoritative (with theprompt:injectfloor), so restricted pipeline grants stick and trusted revocations are not resurrected.pipeline:*is accepted byvalidateCapabilityList, matching the capability required for plugins with pipeline stages.- Contracts, web provider, and daemon behavior stay aligned; regression coverage on web + daemon covers both bug paths.
No actionable findings from this implementation pass — nice work. 🙌
🔁 Powered by Looper · runner=reviewer · agent=grok-build · An autonomous AI dev team for your GitHub repos.
|
@lefarcen can we push this through before the main branch changes again? It seems like its stuck in a loop of approvals/CI for a while. I wasn't sure if you could kick off the final approval/checks, but thought I'd ping you in case we can get this one merged. |
|
@itlackey friendly reminder: this PR appears to be waiting on author action (merge conflict) and has had no human activity for more than 3 days. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no human activity for more than 5 days may be closed automatically, but they can be reopened when work resumes. |
2 similar comments
|
@itlackey friendly reminder: this PR appears to be waiting on author action (merge conflict) and has had no human activity for more than 3 days. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no human activity for more than 5 days may be closed automatically, but they can be reopened when work resumes. |
|
@itlackey friendly reminder: this PR appears to be waiting on author action (merge conflict) and has had no human activity for more than 3 days. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no human activity for more than 5 days may be closed automatically, but they can be reopened when work resumes. |
|
I will update this when we are sure it will be merged |
|
Got it. I checked the current head and there isn’t a new code-side issue I need to flag here — the blocker at the moment is that GitHub is still reporting this branch as conflicting with Once that conflict is cleared again, this should be back in mergeable shape. |
|
@itlackey friendly reminder: this PR appears to be waiting on author action (outstanding trusted reviewer or maintainer feedback, merge conflict) and has had no human activity for more than 3 days. When you have a chance, please reply here or push an update. To keep the queue manageable, PRs with no human activity for more than 5 days may be closed automatically, but they can be reopened when work resumes. |
4 participants
Fixes #6186
Why
I hit this while validating a custom pipeline plugin against an imported existing project. Pure apply correctly avoids persistence, but the web send path dropped the plugin identity when its snapshot ID was empty, so the subsequent run could omit the selected plugin or reuse an older project pin.
The same plugin also exposed a capability-state bug: restricted installs could not persist the derived
pipeline:*requirement, and apply recomputed trust defaults instead of honoring stored grants and revocations.What users will see
Selecting a plugin in an existing project now carries that plugin into the next run, where the daemon creates and pins the snapshot. Restricted pipeline plugins can persist
pipeline:*, and apply respects the stored grant set without restoring revoked capabilities.Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-packflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md-> Code style)Screenshots
Not applicable; this changes run transport and capability resolution without visual UI changes.
Bug fix verification
apps/web/tests/providers/sse.test.ts,apps/web/tests/components/ProjectView.run-isolation.test.tsx,apps/daemon/tests/plugins-trust.test.ts, andapps/daemon/tests/plugins-apply.test.ts.upstream/mainand pass on this branch: yes.Validation
pnpm guardpnpm typecheckpnpm exec vitest run -c vitest.config.ts tests/providers/sse.test.ts tests/components/ProjectView.run-isolation.test.tsx --maxWorkers=2fromapps/web(126passed)pnpm exec vitest run -c vitest.config.ts tests/plugins-*.test.tsfromapps/daemon(500passed)pnpm --filter @open-design/contracts test(246passed)pnpm --filter @open-design/daemon buildpnpm --filter @open-design/web build