Repository navigation
refactor: brand the request id - #675
jakedipity wants to merge 5 commits into
Conversation
Greptile SummaryThe PR replaces bare request-ID strings with a dedicated
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| crates/aura/src/config.rs | Defines the branded request identifier and its transparent string representation. |
| crates/aura/src/mcp/client.rs | Separates Aura request identifiers from rmcp protocol request identifiers in MCP operations. |
| crates/aura/src/request_cancellation.rs | Migrates cancellation-registry keys and APIs from strings to RequestId. |
| crates/aura/src/tool_event_broker.rs | Migrates request-scoped tool-event broker keys and calls to the branded identifier. |
| crates/aura/src/session_store/record.rs | Stores RequestId directly and pins its enduring bare-string JSON contract with a focused test. |
| crates/aura-web-server/src/streaming/handlers.rs | Threads the branded request identifier through the web server’s streaming request lifecycle. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
HTTP[HTTP / A2A request] --> RID[RequestId newtype]
RID --> STREAM[Streaming and SSE routing]
RID --> HITL[HITL approval gates]
RID --> BROKERS[Progress and tool-event brokers]
RID --> CANCEL[Request and MCP cancellation]
RID --> STORE[Session and approval storage]
RID --> SCRATCH[Per-request scratchpad]
STORE -->|serde transparent| JSON[Bare-string persisted shape]
Reviews (4): Last reviewed commit: "refactor: brand the request id" | Re-trigger Greptile
14cb706 to
0de55be
Compare
d7cdb5a to
7d9cf2a
Compare
0a897b9 to
d3df8a6
Compare
7d9cf2a to
4827e82
Compare
d3df8a6 to
daccaa7
Compare
4827e82 to
b78a932
Compare
daccaa7 to
55faa90
Compare
Shearerbeard
left a comment
There was a problem hiding this comment.
My only feedback here is that lib.rs is to general - if were starting to consolidate code around building our domain types and events lets just call it domain
Shearerbeard
left a comment
There was a problem hiding this comment.
This looks good - as we progress we might have to better module classify identifiers a cross application concerns (like RequestId) vs agent specific new types like AgentId. There are probably a few cross cutting concerns to belong in a root domain vs a specific one. Also a good place to start looking for duplicated construction and validation code if it exists (I don't see that here - just the kind of thing I like to think ahead on when designing by types)
30b2685 to
8c3a670
Compare
The base branch was changed.
b78a932 to
3ec8a18
Compare
RequestId is a newtype that only RequestId::generate, RequestId::for_a2a_task, and RequestId::for_slack_message construct. It replaces the String alias and the bare request id strings in HITL, MCP cancellation, the session stores, scratchpad storage, StreamingAgent, and the web server. A run's id is its request's id. A parked approval's ApprovalOwner is the request that raised it, the orchestration run that parked it, or no one for an agent built without a request. Stored records and the webhook payload keep the bare-string request_id field. Signed-off-by: Jacob Hull <jacob@planethull.com>
StreamingRequestHook takes a StreamKey, either the run's own stream (Run(RequestId)) or an orchestration task attempt (Attempt). Only a Run stream owns the run's tool-call queue and sends it tool events, and only an Attempt has a park cell. Both rules were string comparisons that held because an attempt key never equals a request id. Signed-off-by: Jacob Hull <jacob@planethull.com>
The HITL gate and RequestApprovalTool read the owner of a live approval from the run they are bound to, whose id is the request id. They no longer take a request id of their own. AgentRuntimeConfig.request_id is removed, along with the request_id parameter of RigBuilder::build_agent, build_streaming_agent_with_headers, and build_streaming_agent_with_tools that only fed it. Agent::stream_prompt_with_timeout and stream_chat_with_timeout build no run, so approvals raised through them would have no owner. They are crate-private, which leaves StreamingAgent::stream as the public way to stream an agent. The orchestrator parses its run's id once and holds it as a RunId, along with its persistence's session id. The park guard, the worker approval scope, and the sweep of a run's parked approvals all read them there, so worker_scope is synchronous and create_worker builds its scope through it. An agent that streams with a request id now owns its approvals under that id, including one built through AgentBuilder. A gate with no run bound raises unowned approvals. Signed-off-by: Jacob Hull <jacob@planethull.com>
A run cancels the approvals it raised, in the registry and the store, when its event stream drops. AgentRun carries the sweep, and Agent::stream and the orchestrator factory attach it whenever the HITL route parks approvals in a registry. Chat completions, A2A tasks, and Slack runs all stream through StreamingAgent::stream, so each gets the cleanup. A2A tasks and Slack runs previously left their approvals decidable until they expired. The chat handler's RequestResourceGuard and CompletionConfig's pending_approvals, which only fed it, are removed. TaskCancelEntry drops its request_id, since cancel() derives it from the task id. Signed-off-by: Jacob Hull <jacob@planethull.com>
Agent::cancel_mcp_requests had no callers, and it was the only caller of McpManager::cancel_all_for_request, so both are removed. Every cancel goes through cancel_and_close. McpClient::cancel_all_for_request is now private to it. Signed-off-by: Jacob Hull <jacob@planethull.com>
3ec8a18 to
4be9654
Compare
|
@jakedipity heads-up, there's some overlap between this PR and the #778 stack (#794 envelope, then #796 run id, then #797, which fixes #790). #795 isn't related. The main collision: #796 and this PR both re-type FWIW from dry-run merges: this PR already conflicts with Here's an end state I'm thinking about. What are your thoughts?
What I'd change in #796 to meet you halfway (not pushed yet):
I'm deliberately leaving the typed params, registry keys, If this PR lands first:
If my stack (#794) lands first: on top of the #719 rebase you need anyway, you'd:
I'm leaning toward the stack landing first, since #796 is mergeable now and you need the #719 rebase regardless, but I'm totally fine going the other way if you'd rather land first. Happy to noodle on it together too! 🙏 And if you've got a strong reason to keep the readable prefixes in the id itself, I think that may be worth hashing out on #778 before either of us moves? Thoughts? |
|
@justintime4tea Thanks for mapping this out. I agree with the end state, and I'm happy for your stack to land first. I owe the #719 rebase either way, and reworking once onto I had some local changes not pushed remotely:
Once your stack lands, I'll rework this PR into typing the run's boundaries on top of
|
RequestId becomes a newtype over String instead of a type alias, so a bare string can no longer stand in for a request id. It threads through SSE routing, the tool-event broker, MCP cancellation, HITL approval gates, the session store, and the scratchpad's per-request directory.
The brokers and the cancellation registry key their maps by RequestId rather than String. In mcp/client.rs, rmcp's own RequestId is imported as McpRequestId, so the HTTP request id and the MCP protocol request id are no longer the same type in signatures that take both.
The webhook payload and stored approval records keep their bare-string shape, which serde(transparent) guarantees and a record test now pins.