Emit Trusted MCP App Identity on Tool-Call Items#27132
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
dbd8742 to
ec83a47
Compare
|
I have read the CLA Document and I hereby sign the CLA |
1341465 to
d839e6b
Compare
|
recheck |
dc75728 to
579e9fc
Compare
|
recheck |
579e9fc to
70af95d
Compare
|
Message from Martin's Codex agent: @mzeng-openai The PR was rebased onto current |
| tool: String, | ||
| status: McpToolCallStatus, | ||
| arguments: JsonValue, | ||
| connector_id: Option<String>, |
There was a problem hiding this comment.
instead of defining these as top-level fields, what do you think of this instead:
McpToolCall {
...
app_context: Option<McpToolCallAppContext>,
}
pub struct McpToolCallAppContext {
pub id: String,
pub link_id: Option<String>,
}
It has these benefits IMO:
- It keeps the generic MCP item fields generic. Top-level
connector_idandlink_idfields seem slightly misleading since they are only populated when using our first partycodex_appsMCP server. - It uses app-server’s public vocabulary: the API already has
app/listandAppInfo { id, ... }, where the “connector id” is already exposed to clients as the app id. - It lets clients correlate with
app/list.
There was a problem hiding this comment.
Message from Martin's Codex agent: This is technically coherent and aligns well with app/list/AppInfo.id. The one blocker is that the PR currently documents an external consuming-backend contract of { connectorId, linkId, tool }; nesting it as appContext: { id, linkId } changes that wire contract, and this repo cannot verify whether that consumer can migrate. I’m confirming that product/API decision with Martin before changing the shape. I’ll update this thread once resolved.
There was a problem hiding this comment.
haha rogue bot. please ignore. Let me look at the consumer side code more carefully. I considered this previously but was told it might require a lot of code changes on the consumer end, but I do like this solution a lot more.
There was a problem hiding this comment.
so my initial thought process was to also bake mcp_app_resource_uri into the McpToolCallAppContext which would also cause a lot of consumer side changes. reason being that thought it'd be better to keep it flat instead of having the app context with connectorID and linkID but mcp_app_resource_uri separated and causing additional confusion, as well as rollout safety & efficiency (timeline constraint)
I don't mind creating a follow-up PR that adds mcp_app_resource_uri to McpToolCallAppContext separately and keeps the code cleaner. WDYT?
| } | ||
|
|
||
| fn handle_mcp_tool_call_begin(&mut self, payload: &McpToolCallBeginEvent) { | ||
| // The canonical started item carries app identity that this deprecated event cannot. |
There was a problem hiding this comment.
let's avoid this complexity - we can just add the new fields to McpToolCallBeginEvent as well, and we won't need this
| } | ||
|
|
||
| #[test] | ||
| fn active_turn_snapshot_preserves_started_mcp_connector_id() { |
There was a problem hiding this comment.
once we do that, we can delete this test
| } | ||
|
|
||
| #[test] | ||
| fn mcp_tool_call_end_event_defaults_missing_connector_id() { |
There was a problem hiding this comment.
hm, is this test useful?
7b70871 to
a42d762
Compare
| pub struct McpToolCallAppContext { | ||
| pub connector_id: String, | ||
| pub link_id: Option<String>, | ||
| pub mcp_app_resource_uri: Option<String>, |
There was a problem hiding this comment.
nit: would we be able to call this just resource_uri here? I'm guessing mcp_app_ is redundant at this point?
There was a problem hiding this comment.
i think we should change this to rendering_uri but may require some downstream consumer changes. I'll add this as a follow-up PR
| } | ||
| codex_protocol::items::TurnItem::Sleep(_) => { | ||
| codex_protocol::items::TurnItem::Sleep(_) | ||
| | codex_protocol::items::TurnItem::McpToolCall(_) => { |
There was a problem hiding this comment.
I think we can revert this? We should switch thread_history.rs to turn items, similar to rollout files, but can be done as a separate PR. I think as is, the ThreadHistoryBuilder would be handling MCP tool calls twice (from both the legacy Begin/End events and the ItemStarted/Completed events)
There was a problem hiding this comment.
sg, i thought initially the swap would be a code clarity change, good callout.
owenlin0
left a comment
There was a problem hiding this comment.
two remaining comments but preapproving
721b28e to
d86d88a
Compare
d86d88a to
c56a747
Compare
Summary
appContextto app-server MCP tool-call items with trustedconnectorId,linkId, andmcpAppResourceUrimetadata.mcpAppResourceUritemporarily for client migration.The consumer contract is
{ appContext: { connectorId, linkId, mcpAppResourceUri }, tool }.Validation