fix: derive plugin media trust from metadata#86410
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed May 25, 2026, 9:12 AM ET / 13:12 UTC. Summary PR surface: Source +90, Tests +41, Docs +1. Total +132 across 13 files. Reproducibility: yes. Source inspection shows current main imports pluginRegistrationContractRegistry in the subscribe media helper, and the PR removes that path while adding focused media trust tests; I did not run a live gateway scenario in this read-only review. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance: Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land the metadata-derived trust design only after current-head real gateway or subscribe-path proof verifies trusted core and bundled media delivery, untrusted local-media blocking, and a green or known-unrelated CI state. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main imports pluginRegistrationContractRegistry in the subscribe media helper, and the PR removes that path while adding focused media trust tests; I did not run a live gateway scenario in this read-only review. Is this the best way to solve the issue? Yes, with proof. Deriving trust from bundled plugin tool metadata and the actual run tool surface is the maintainable direction, but the current PR still needs real runtime proof before merge because the touched path controls local attachment delivery. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 8fe4f34af224. Label changesLabel justifications:
Evidence reviewedPR surface: Source +90, Tests +41, Docs +1. Total +132 across 13 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07368f6ef9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pluginMetadataSnapshot = getCurrentPluginMetadataSnapshot({ | ||
| config: params.config, | ||
| env: process.env, | ||
| workspaceDir: effectiveWorkspace, | ||
| }); |
There was a problem hiding this comment.
Fall back when current metadata snapshot is unavailable
Deriving trustedPluginLocalMediaToolNames from getCurrentPluginMetadataSnapshot(...) here makes bundled-plugin local media trust depend on the global “current” snapshot, but that snapshot is intentionally cleared whenever plugin metadata is loaded from a derived source (see src/plugins/runtime/load-context.ts, which clears current state for derived snapshots). In that common startup path (e.g., no reusable persisted registry yet), this call returns undefined, collectTrustedPluginLocalMediaToolNames produces an empty set, and bundled plugin MEDIA: paths are stripped even though those tools were actually loaded for the run. This is a user-visible regression from the prior behavior where bundled plugin media remained deliverable.
Useful? React with 👍 / 👎.
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
07368f6 to
24e584b
Compare
Summary
Refs #84409.
Verification
node scripts/run-vitest.mjs src/agents/pi-embedded-subscribe.tools.media.test.ts src/agents/pi-embedded-subscribe.handlers.tools.media.test.ts src/agents/pi-embedded-subscribe.subscribe-embedded-pi-session.subscribeembeddedpisession.test.ts src/plugins/tools.optional.test.tsenv -u OPENCLAW_TESTBOX -u OPENCLAW_TESTBOX_REMOTE_RUN pnpm check:changed/Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode branch --base origin/mainReal behavior proof
Behavior addressed: bundled plugin local
MEDIA:paths no longer require importing the full plugin registry on subscription paths, and plugin trust is derived from plugin tool materialization metadata plus the actual run tool surface.Real environment tested: local source checkout on macOS, Node/Pnpm repo tooling.
Exact steps or command run after this patch: focused Vitest command above, changed-file gate above, and autoreview branch closeout.
Evidence after fix: focused tests passed 6 files / 247 tests; changed gate exited 0; final autoreview reported no accepted/actionable findings.
Observed result after fix: core media tools retain exact-name local media gating, bundled manifest-contract plugin tools such as
meeting_notescan pass local media only when included in the trusted run-local set, and duplicate suppression sees the same trusted set.What was not tested: packaged gateway end-to-end media delivery; this patch is covered by unit/subscription behavior tests plus type/lint/import-cycle gates.