feat(plugins): expose requester context to tool hooks#111190
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 19, 2026, 1:28 PM ET / 17:28 UTC. Summary PR surface: Source +54, Tests +41, Docs +130. Total +225 across 11 files. Reproducibility: yes. from source: start an active shared run as an owner, steer it with a lower-privileged allowed sender, then trigger a tool call; the review trace shows the hook context retains the original requester snapshot. Review metrics: 1 noteworthy metric.
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
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Make requester identity message-scoped for every tool invocation, or reject cross-requester steering for policy-protected runs; add adversarial coverage for owner-started runs steered by lower-privileged senders before documenting this as an authorization mechanism. Do we have a high-confidence way to reproduce the issue? Yes, from source: start an active shared run as an owner, steer it with a lower-privileged allowed sender, then trigger a tool call; the review trace shows the hook context retains the original requester snapshot. Is this the best way to solve the issue? No. Forwarding requester facts only at tool-set construction is not safe for active runs that accept later steered messages; the hook context must be refreshed per causative message or cross-requester steering must be rejected. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6fb3aae404f6. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +54, Tests +41, Docs +130. Total +225 across 11 files. View PR surface stats
Security concerns:
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
Review history (1 earlier review cycle)
|
f46f8cb to
0324573
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0324573079
ℹ️ 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 requester = { | ||
| ...(turnSourceChannel ? { channel: turnSourceChannel } : {}), | ||
| ...(options?.agentAccountId ? { accountId: options.agentAccountId } : {}), | ||
| ...(options?.senderId ? { senderId: options.senderId } : {}), | ||
| ...(options?.senderIsOwner !== undefined ? { senderIsOwner: options.senderIsOwner } : {}), | ||
| ...(options?.memberRoleIds?.length ? { roleIds: [...options.memberRoleIds] } : {}), |
There was a problem hiding this comment.
Refresh requester context for steered messages
When an owner starts a shared group run and a lower-privileged allowed sender posts while it remains active, the default steering path injects that sender's prompt into the existing run (src/auto-reply/reply/agent-runner.ts:1412) without rebuilding its tools or passing sender identity to the queue handle (src/agents/embedded-agent-runner/run/attempt-stream-prepare.ts:352). Because this requester object is captured only when the tool set is created, every subsequent before_tool_call still sees the original owner's senderIsOwner, sender ID, and roles, allowing the documented sender-aware policy to authorize owner-only tools prompted by the steered sender. Carry requester identity with each steered message and refresh the hook context, or reject steering across requester identities.
Useful? React with 👍 / 👎.
|
Merged via squash.
|
* feat(plugins): expose requester context to tool hooks * chore(plugin-sdk): restore generated API baseline * chore: drop release-owned changelog entry * docs: refresh documentation map
Related: #103780
What Problem This Solves
Operators can authorize several people to talk to one agent, but
before_tool_callhooks cannot currently distinguish the requester who initiated a run. That prevents a small deployment-local policy file from giving owners broad access while constraining maintainers to safer tools and message actions.Why This Change Was Made
Expose host-derived channel, account, sender, owner, and provider-role facts through the existing
ctx.requesterhook context. The same context now reaches managed tools and Codex-nativePreToolUsecalls. No new authorization configuration schema is added; documentation shows one standalone plugin file that owns the policy and an existing agent binding that routes/fixto the protected agent.This is the minimal replacement for the broad experimental prototype in #103780. That draft remains explicitly DO NOT LAND.
User Impact
Plugin authors and operators can enforce per-requester tool policy in one TypeScript file. Requester facts are optional and fail-closed examples treat missing identity as unproven. Existing channel admission, sandbox, approval, and owner-only restrictions still apply and cannot be bypassed by the hook.
Evidence
pnpm plugin-sdk:api:check: passed.pnpm plugin-sdk:surface:check: passed with the public export budget unchanged.pnpm check:changed: passed, including core/extension typecheck and lint lanes.pnpm build: passed./fixrouting, and the copy-paste plugin contract.AI-assisted. I understand the requester provenance, routing, and enforcement boundaries in this change.