feat(rpc): add a reusable engine assembly and a working-directory guard - #357
Open
ZuyiZhou wants to merge 1 commit into
Open
feat(rpc): add a reusable engine assembly and a working-directory guard#357ZuyiZhou wants to merge 1 commit into
ZuyiZhou wants to merge 1 commit into
Conversation
Two pieces a transport that is not a socket needs before it can serve turns, plus the parameter that lets it pick its own delivery channel. build_rpc_stack is the engine half of the socket path's startup: an AgentLoop, the three prompt brokers, a SubscriptionEmitter, the spine scheduler, the method registrations and a teardown, with the transport left to the caller. The socket path deliberately does NOT call it, and the reason is in the module docstring: RpcServer is constructed FROM the dispatcher and only then can provide send_frame, while this function creates the dispatcher itself. Inverting that changes the startup path every terminal session depends on and buys nothing until a second socket transport exists. What this assembly does not wire, compared with a fuller one, is listed in the same docstring rather than left as a diff to interpret: no DAG progress sink, sub-agent delivery sink or MCP event sink, because nothing here sets them; no direct-chat target map; and no per-connection scoping of the approval and question brokers, because one process serves one client here, so a broadcast IS the one client. The channel is a parameter now, and it has to reach BOTH collaborators. The delivery outlet is registered under the channel name and turn.send stamps it on every turn it submits as source.channel; the hub routes a deliverable by that name, so giving it to one side and not the other loses every turn with no error anywhere. A test pins that. The default is the value both sides independently hardcoded before, so existing callers are unchanged. approval_responder replaces the transport shell approvals are asked over, without touching the classification, the one-command scope or the absence of any always-allow state. The local broker is still built either way, because approval.respond is registered from it. workdir.validate_override is the guard a caller needs before running anything in a directory somebody else named. The refused answers are agent home itself, any ancestor of it, and three of its subtrees, each for a different reason written at the check. The ancestor case is the sharp one: the realistic ancestor is the instance data directory holding provider keys and tokens, and a per-turn checkpoint that adds everything under the working directory would commit them into a shadow repo. Paths are resolved before comparison, so a symlink into a protected subtree is refused too. Only the guard is here. The per-channel default roots and the per-turn binding that belong to a full working-directory feature are left out, because nothing on this side asks where a turn should run yet. Co-authored-by: Claude (claude-opus-5[1m]) <[email protected]>
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two pieces a transport that is not a socket needs before it can serve turns, plus the parameter that lets it pick its own delivery channel.
build_rpc_stackis the engine half of the socket path's startup: an AgentLoop, the three prompt brokers, aSubscriptionEmitter, the spine scheduler, the method registrations and a teardown, with the transport left to the caller.The socket path deliberately does NOT call it, and the reason is in the module docstring:
RpcServeris constructed FROM the dispatcher and only then can providesend_frame, while this function creates the dispatcher itself. Inverting that changes the startup path every terminal session depends on and buys nothing until a second socket transport exists. So the duplication is bounded and the reason is written down rather than left for somebody to rediscover.What this assembly does not wire is listed in the same docstring rather than left as a diff to interpret: no DAG progress sink, sub-agent delivery sink or MCP event sink, because nothing here sets them; no direct-chat target map; and no per-connection scoping of the approval and question brokers, because one process serves one client here, so a broadcast IS the one client.
The channel is a parameter now, and it has to reach BOTH collaborators. The delivery outlet is registered under the channel name and
turn.sendstamps it on every turn it submits assource.channel; the hub routes a deliverable by that name, so giving it to one side and not the other loses every turn with no error anywhere. A test pins that. The default is the value both sides independently hardcoded before, so existing callers are unchanged.approval_responderreplaces the transport shell approvals are asked over, without touching the classification, the one-command scope, or the absence of any always-allow state. The local broker is still built either way, becauseapproval.respondis registered from it.workdir.validate_overrideis the guard a caller needs before running anything in a directory somebody else named. The refused answers are agent home itself, any ancestor of it, and three of its subtrees, each for a different reason written at the check. The ancestor case is the sharp one: the realistic ancestor is the instance data directory holding provider keys and tokens, and a per-turn checkpoint that adds everything under the working directory would commit them into a shadow repo. Paths are resolved before comparison, so a symlink into a protected subtree is refused too.Only the guard is here. The per-channel default roots and the per-turn binding that belong to a full working-directory feature are left out, because nothing on this side asks where a turn should run yet.
Both modules have their tests as the only caller today; the production caller is a stdio protocol transport in the next PR. Landing them separately keeps that one reviewable.
Stacked on #356, which is itself stacked on #353. Merge order: 353, 356, then this.
Reviewers: 0xKT, gloryfromca.
Type
Verification
Commands run and their results:
uv run --frozen --all-extras pytest -q-> 6748 passed, 19 failed. The same 19 that fail on the base branch:test_provider_rates.py(15) andtest_agent_loop_usage_sink.py(4), pricing-table and network-cache tests unrelated to this change.uv run --frozen python scripts/coverage_gate.py diff --base-ref feat/spine_turn_correlation-> 100.00% (113/113 executable changed lines). The first run of this gate failed at 81.42%: the uncovered lines were the memory-backend start and stop, the cron startup-drop fan-out, and the teardown's guarded failure paths, none of which the first set of fakes reached. Six cases were added for them.uv run --frozen python scripts/coverage_gate.py ratchet-> line +1.14pp, branch +1.61pp, PASS.uv run --frozen ruff check raven/ tests/andruff format --check-> clean.The channel test was checked to fail without its fix: dropping
default_channel=channelfrom the registration call turns it red.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
Two new modules and three new keyword arguments, all defaulted. No existing call site changes behaviour: the channel default is the value both sides already hardcoded, and
approval_responder=Nonekeeps the locally built broker.The one thing to look at is the teardown ordering in
build_rpc_stack: the memory backend is drained before it is stopped, because stopping without draining loses what the last turn wrote, and the stop is what releases the embedded index lock the next process needs. Each step is guarded on its own so a failure in one does not skip the rest; a test drives a teardown where cron stop, the spine teardown and the backend stop all raise, and asserts the drain is still reached.Rollback is a plain revert. Nothing imports either module in production yet, so a revert cannot break a caller.
Related Issues
N/A