Repository navigation
Conversation
#16950) BaseAgent._handle_communication_request built its AgentRequest from the payload and never read the header, so a peer's request was indistinguishable from the agent's own -- the point where authorization needs the originator is where it was dropped. And MessageHeader.sender is restamped on every send, correctly, so once one relay happened the originator was unrecoverable. - MessageHeader gains originator (set once, never overwritten by a relay) and chain (every hop, originator first). - protocols/message_origin.py: stamp() records each hop; acting_for() and a context variable make relays structural -- a message sent while handling a request continues that request's chain, even through helpers that never saw the inbound message. origin_of() reads an inbound header, treating a pre-field header's sender as its originator, never as nobody. - AgentRequest gains originator and chain; the handler fills them and runs process_request inside acting_for. Nothing authenticates these fields: until #16962, the bus's trust boundary is Redis write access (#16946, owner decision 4). The if-main demo moves to protocols/agent_communication_demo.py to pay for the lines in a file at its size ceiling (805 -> 750).
…so it cannot do what the parent is held from (#16950) _handle_delegate_tool passed the child only parent_agent_id (for a log line) and auth_role; the child was built from its own profile alone, so the parent's approval gates and work item were dropped. A parent held from write_file by its work item's "writing files" gate could delegate the write, and the child ran it unapproved. security/authority.py is the common form ruled on #16950 (F1): the four authority surfaces side by side, each with its own meet -- restrictions (approval gates, forbidden tools) by union, grants (RBAC permissions, A2A capabilities) by intersection, and a surface a hop does not use is top, never bottom. chat_workflow/run_authority.py reads a run's authority and what a child inherits from it. - internal engine: the child ctx carries the parent's work item, the union of both runs' gates, and the parent's authority, which the forbidden-work seam now unions into the child's boundary; - claude_code engine: it runs its own tool loop and cannot ask for approval, so the parent's gated tools and boundary are refused there (fail closed). Its only other reach, AutoBot's MCP server, exposes reads. Removes #16958's strict-xfail marker in the same commit as the fix.
…#16950) The delegate handler now passes the parent run (parent=ctx) so the child can inherit its authority; run_delegated_subtask reads the parent's agent_id from it for the GH#11266 dispatch log. The handler test asserted the old parent_agent_id keyword, so it now asserts the id on the run it receives. A new test checks that the log still names the parent when only the parent run is passed, so the observability contract holds end to end.
…entity (#16950) build_governed_identity trusted the agent_id in whatever dict it was handed, and an executor (unbounded) id resolves to no boundary at all. No peer's context reaches it today (traced on #16950), but nothing structural stopped a future path from forwarding one. - session_role.apply_role now marks the role it pins under PINNED_ROLE_CONTEXT_KEY, and removes a client-supplied marker, mirroring apply_auth_role's write-or-remove rule. - build_governed_identity honours an executor id only when the overlay pinned it. An unpinned executor claim is renamed to an id no profile holds, so it resolves to the default boundary with a warning, as an unknown id does (GH#13588). - A client naming a *bounded* profile is still honoured: that only restricts its own run, and GH#11186's self-restriction was a recorded decision (test_apply_role_none_leaves_context_unchanged), kept. - A guard pins the builder's three callers, so a new producer has to show where its context came from.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ication.py at the 749 lines black leaves (#16950)
|
Review verdict: BLOCK — the 🔴 Unmapped gated tokens are silently dropped, and
|
| Token | Gate it belongs to | Result |
|---|---|---|
git_push, git_commit, git_force_push |
pushing commits | dropped |
git_reset |
discarding local changes | dropped |
http_post, http_delete, send_request |
sending externally | dropped |
rotate_credentials |
rotating credentials | dropped |
publish |
publishing | dropped |
_build_cli_command is a pure blocklist, with Bash available by default. So a parent whose work item holds "pushing commits" delegates to a claude_code child whose --disallowedTools never contains Bash, and that child runs git push, git reset --hard or curl -X POST through Bash — doing exactly what the parent was held from doing.
The shipped profiles do not mask it: research_agent, documentation_agent and coordination_agent forbid only INFRA_AND_SHELL_TOOLS, which contains none of these tokens, so Bash stays open whatever the child's own profile says.
The internal engine is correct, which is why this is specific to one path. _run_internal_subagent puts the raw category into requires_approval_before, and enforce_work_item_approval matches the actually-dispatched tool name against it — no lossy translation, so every category holds.
Why a gate needs a different rule from a profile boundary
For a profile's own forbidden_work, dropping an unmapped token is a documented, accepted behaviour: the profile's other tokens still constrain the agent. A gate has no such fallback — it is the only thing standing between the child and the action. And the child cannot ask a human for approval, which is exactly why this PR refuses gated tools on this engine rather than holding them. A refusal that cannot be expressed must fall back to refusing the coarse tool, not to allowing everything.
Fix
On the gate path, deny by default: if any gated or forbidden token has no finer CLI mapping, add Bash to --disallowedTools. Leave the profile-boundary path's existing behaviour alone. Then add a run_authority_test.py case per category — pushing commits, discarding local changes, sending externally, rotating credentials, publishing — each asserting Bash is disallowed, mirroring test_a_gate_the_parent_holds_is_refused_to_the_child. Today only "writing files" and "destructive operations" are exercised, which is why CI is green.
The consequence is intended: a claude_code child of a parent held on pushing or external sending gets no Bash. If the work needs to push, the parent — with a human — does it.
This is separate from the owner's decision to accept an unenforceable Bash gap for Company OS org roles on claude_code. That concerns a role's standing permissions. This is a specific work item explicitly holding an action for a human, and a delegated child must not be able to take it.
Verified sound
- The meet, in both directions.
security/authority.pyunionsapproval_gatesandforbidden_toolsand intersectspermissionsandcapabilities, withNoneas ⊤, never ⊥. Tested on both sides:test_a_hop_without_capabilities_does_not_zero_a_chain_that_has_them, plus the ⊥-versus-⊤ contrast and commutativity.test_a_tool_the_parent_forbade_is_blocked_for_the_childhas a control proving the block comes from inheritance. - Trusted producer. An executor
agent_idis honoured only whenapply_rolepinned it; a forged pin does not survive the overlay; the three producers are locked by an AST test. The retained self-restriction exception can only narrow: an unpinned id becomesunpinned:<id>and falls to the default forbidden set. - test(security): a delegated subagent runs a write its parent is held from (#16950) #16958's xfail marker is removed, and the test now asserts the real
enforce_work_item_approvalholds the child's write. - Ratchets only lowered:
tool_handler.py3729 → 3728, andagent_communication.py805 → 749, via a genuine extraction of the demo into its own module. Identical in both registries. - No swallowed errors,
get_loggerin every new module, functions within limits.
Labelled blocked until the gate path fails closed.
…ash when no finer tool covers it (#16950) Review of #16966: forbidden_to_claude_tools drops any token with no claude_code tool, and only write_file/edit_file and the shell group map. git_push, git_commit, git_reset, http_post, send_request, rotate_credentials and publish all mapped to None and were filtered out, so a parent held on "pushing commits" would hand a Bash-keeping child nothing that stops `git push`. For a profile boundary, dropping an unmappable token is the accepted behaviour -- other tokens still bound the agent. A gate has no such fallback. claude_tools_refusing() keeps the two paths apart: the child's own profile is mapped as before, and a token the parent holds (gated or forbidden) with no finer mapping disallows Bash. The held action then happens in the parent, with a human. The gap was latent, not live: every bounded profile forbids the shell group, which maps to Bash, so today's children already lose it. The tests model a Bash-keeping child with an empty boundary and assert every approval category takes Bash away, plus the control that a child with nothing held keeps it.
|
Fixed in Tests: Correcting the scenario: the
The description's |
…y file containing "test" (#16950) Review of #16969 found `"test" not in name` in its guard's module filter; the same filter was here. It skipped production modules too -- on this tree testing_coverage_analyzer.py and testing_pattern_analyzer.py were never scanned -- so a producer added to one would pass the guard unexamined. The filter now matches pytest.ini's python_files naming (test_*.py, *_test.py) and conftest.py, with a control that a production name containing "test" is scanned.
|
Code-review pass on Positive: does NOT invent a competing agent-identity scheme. Two findings before merge:
None of this is a regression — it's solid incremental work — but (1) blocks merge outright per house CI rules, and (2)/(3) should be resolved or explicitly re-scoped before this is relied on as proof of the security property. |
…g-test' into issue-16966-wt
…g-test' into issue-16966-sync-wt
|
Why this was closed: GitHub closed it automatically when its base branch was deleted. It was not a review decision and not a carry. This PR was stacked on Nothing is lost:
A sweep of every unmerged PR closed since 2026-09-18 whose base branch is gone found this as the only uncarried case. |
1 participant
Refs #16950
Stacked on #16958 (base
issue-16950-delegation-laundering-test). #16958 is the strict-xfail negative control; this PR removes its marker in the fix commit. Merge #16958 first, then retarget this PR tomain.Thinking Path
This builds the parts of #16946's identity model (as corrected, with the owner's decisions) that do not depend on still-open forks.
main: delegation dropped the parent's approval gates and work item._handle_delegate_toolpassed the child onlyparent_agent_id(used for a log line) andauth_role. The child was then built from its own profile alone. So a parent held fromwrite_filecould delegate the write, and the child ran it unapproved.BaseAgent._handle_communication_requestnever read the header, so a peer's request was indistinguishable from the agent's own. Andsenderis restamped on every send, so after one relay the originator could not be recovered.build_governed_identitytrusted whateveragent_idit was handed. An executor id means no boundary at all. No peer context reaches it today (traced on security(agents): an agent refused an action can get another agent to do it — no per-peer identity #16950), but nothing structural stopped a future path from forwarding one.The common form, per ruling F1:
security/authority.pykeeps the four authority surfaces side by side, each with its own meet.What Changed
771e06032MessageHeader.originator(set once, never overwritten by a relay) andchain.protocols/message_origin.pymakes relays structural: a send made while handling a request continues that chain, even through helpers that never saw the inbound message.AgentRequest.originator/chainare filled by the handler. Theif __name__demo moves toprotocols/agent_communication_demo.pyto pay for the lines (805 → 750)3d9f3ba9aAuthorityand its meet.chat_workflow/run_authority.pyreads a run's authority and what a child inherits. Internal engine: the child carries the parent's work item, the union of both runs' gates, and the parent's authority, which the forbidden-work seam unions into the child's boundary.claude_codeengine: it runs its own tool loop and cannot ask for approval, so the parent's gated tools and boundary are refused there (fail closed). Corrected in3a5674e84: that claim was overstated as first written. Tokens with no claude_code tool (git_push,http_post,rotate_credentials,publish, …) were dropped by the mapping. A token the parent holds with no finer tool now takesBashaway; see the row below. Its only other reach, AutoBot's MCP server, exposes knowledge, memory and agent-list reads only (mcp_server/autobot_server.py:655-849). #16958's xfail marker is removed in this commit.5349f789c3a5674e84claude_tools_refusing(): the child's profile boundary is mapped as before, where dropping an unmappable token is the accepted behaviour. A token the parent holds with no finer claude tool disallowsBash, because a gate has no fallback. The gap was latent: every bounded profile forbids the shell group, so today's children already loseBash. The tests model aBash-keeping child and assert that every approval category takesBashaway, with a control89f82f4d0,17ca3de36message_origin_test.py::test_two_real_round_trips_carry_the_originator_to_the_last_hop: A asks B, and B, while handling it, asks C; both replies come back. The realsend_request,send_message/stamp,_handle_message,BaseAgent._handle_communication_requestandsend_responserun on every hop, over the JSON wire format. Only the transport is a stand-in that delivers byrecipient, because the real channels deliver a request to its sender, not its recipient (#16986). C sees originator A and chain[A, B]although B's onward request was built with no originator. Each delivery runs in a fresh context, as a receiver's poller would, so no origin leaks from the sender. The base branch is merged in (it was 6 commits behind)8e8596aefapply_rolemarks the role it pins and removes a client-supplied marker, the same write-or-remove rule asapply_auth_role.build_governed_identityhonours an executor id only when the overlay pinned it; an unpinned claim gets the default boundary. A client naming a bounded profile is still honoured: that is GH#11186's recorded self-restriction (test_apply_role_none_leaves_context_unchanged), kept. A guard pins the builder's three callersTrust boundary (owner decision 4): the new header fields are not authenticated. Until #16962 lands, the agent bus's trust boundary is Redis write access: a process with it can forge
senderandoriginatoras any registered agent. This PR fixes the confused deputy (a well-behaved agent acting with its own authority on another's behalf), not raw-bus forgery.Not in this PR:
a2a-executor, the peer's authority in the orchestrator, and security(a2a): three of four trust-matrix capabilities are enforced nowhere — a level that denies them denies nothing #16957. It follows as a separate stacked PR, because the trust re-key raises a migration question that is with the coordinator and the owner.Authority.permissionsis in and tested, but no path produces a set different from the role yet. It is wired with the first producer, the Company OS hop, which is pending owner decision F3.TOPon all four surfaces, and peer actions (classify_request,add_knowledge, …) are not in the chat-tool vocabulary thatresolve_forbidden_toolsrestricts. The requester's authority needs a decision first (agents: peer-channel authorization — decide the requester's authority, then enforce the meet at _handle_communication_request #16998,needs-decision). It is also blocked by agents: the peer channel never delivers to the recipient — a request lands in the sender's own inbox and the sender answers itself #16986, the channel not routing to the recipient.Acceptance criteria
Refs, notCloses. Nothing is ticked; the merger verifies against merged code.Authority.meet, laundering test)chat_workflow/delegation_laundering_test.py::test_the_child_is_held_as_its_parent_iswas XFAIL onmain(test(security): a delegated subagent runs a write its parent is held from (#16950) #16958) and passes here with the marker removed.Verification
delegation_laundering_test.py(marker removed),message_origin_test.py,run_authority_test.py,security/authority_test.py,governed_identity_producer_test.py,delegation_test.py, andtest_agent_registry_fail_closed.py. That includesrun_authority_test.py's controls: without the inheritance, research_agent may callhttp_getand theclaude_codechild mayWrite/Edit.protocols/agent_communication.py805 → 750 andchat_workflow/tool_handler.py3729 → 3728. Both files are updated in both baselines.Single-issue rationale
This is the core of #16950. The A2A half is split into #16969 (the coordinator approved the split) so that each stays one honest review. It closes nothing because #16950's A2A criteria, AC3 and the follow-ups are still open, and #16950 closes when those land.
Model Used
Claude Opus 5 (
claude-opus-5)