Skip to content
Merged
23 changes: 23 additions & 0 deletions src/agent/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,29 @@ impl Session {
self.threads.entry(thread_id).or_insert(thread)
}

/// Create a new thread with a specific UUID.
///
/// Used when the caller provides a pre-determined thread ID (e.g. the
/// Responses API generates UUIDs that must be preserved for later lookup).
pub fn create_thread_with_id(&mut self, id: Uuid, channel: Option<&str>) -> &mut Thread {
if self.threads.contains_key(&id) {
tracing::debug!(
thread_id = %id,
"create_thread_with_id: UUID already exists, reusing existing thread"
);
// Return existing thread without mutating active_thread or
// last_active_at — avoids disrupting a concurrent conversation.
return self
.threads
.entry(id)
.or_insert_with(|| Thread::with_id(id, self.id, channel));
}
let thread = Thread::with_id(id, self.id, channel);
self.active_thread = Some(id);
self.last_active_at = Utc::now();
self.threads.entry(id).or_insert(thread)
}

/// Get the active thread.
pub fn active_thread(&self) -> Option<&Thread> {
self.active_thread.and_then(|id| self.threads.get(&id))
Expand Down
25 changes: 23 additions & 2 deletions src/agent/session_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,31 @@ impl SessionManager {
}
}

// Create new thread (always create a new one for a new key)
// Create new thread (always create a new one for a new key).
// If the external_thread_id is a valid UUID AND it isn't already
// mapped to a different ThreadKey, adopt it as the internal thread ID
// so callers (e.g. the Responses API) can look up conversations by
// the same UUID they encoded in the response ID.
let thread_id = {
// Check under read lock: only adopt ext_uuid if no other key
// maps to it (prevents aliasing two keys to the same thread).
let safe_ext_uuid = if let Some(uuid) = ext_uuid {
let thread_map = self.thread_map.read().await;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — Non-blocking: TOCTOU between read lock and write lock

The read lock at line 209 checks that uuid isn't already mapped, then drops the lock. The thread is created and the mapping inserted later under a separate write lock. Between the two, another task could map the same UUID.

The existing adoption path (lines 178-198 above) handles this correctly with a double-check under the write lock (if !thread_map.values().any(|&v| v == ext_uuid) at line 186). This new creation path doesn't follow the same pattern.

Practical risk is very low since the Responses API generates Uuid::new_v4(), but it's inconsistent with the codebase's own concurrency discipline. Consider adding a re-check under the write lock when inserting the mapping, matching the pattern at line 186.

if thread_map.values().any(|&v| v == uuid) {
None // Already mapped elsewhere — generate a new UUID
} else {
Comment on lines +208 to +212

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new-thread path, safe_ext_uuid is decided under a thread_map.read() lock, but the mapping is inserted later (outside the session lock) without a write-locked re-check. Another concurrent resolve_thread_with_parsed_uuid for the same user can slip in between thread creation and thread_map.insert(...), and adopt the same external UUID for a different ThreadKey, aliasing keys to the same thread UUID. Consider either (a) holding the session mutex until after thread_map.insert(...), or (b) re-checking under thread_map.write() right before insert and falling back to a freshly generated UUID if the external UUID became claimed.

Copilot uses AI. Check for mistakes.
Some(uuid)
}
} else {
None
};

let mut sess = session.lock().await;
let thread = sess.create_thread(Some(channel));
let thread = if let Some(uuid) = safe_ext_uuid {
sess.create_thread_with_id(uuid, Some(channel))
} else {
sess.create_thread(Some(channel))
};
Comment on lines +200 to +224

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In resolve_thread_with_parsed_uuid, the create-new-thread path uses ext_uuid directly. That bypasses the earlier “UUID adoption check” that ensures the UUID isn’t already mapped to a different ThreadKey (only adoptable_uuid is guarded). If external_thread_id parses as a UUID that is already mapped elsewhere, this will still call create_thread_with_id and effectively create/alias a second thread with the same UUID, corrupting thread_map/undo_managers invariants. Use the guarded adoptable_uuid (or re-check under the write lock) before adopting ext_uuid as the internal thread id on the new-thread path.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d79e590 — re-checks under read lock that the UUID is not already mapped to another ThreadKey before adopting.

Comment on lines +200 to +224

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This create-new-thread path adopts any UUID-shaped external_thread_id as the internal thread_id (when not already mapped). That effectively lets callers pick internal thread UUIDs for any channel, increasing the chance of UUID collisions across users/tasks and widening the trust boundary beyond the intended Responses API use-case. Consider restricting UUID adoption-on-create to trusted channels/paths (e.g., only channel == "gateway", or only when invoked from a trusted caller), while keeping the existing “adopt UUID if it already exists in the session” behavior.

Copilot uses AI. Check for mistakes.
thread.id
};

Expand Down
17 changes: 17 additions & 0 deletions src/agent/thread_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,23 @@ impl Agent {
};

if requires_preexisting_uuid_thread(&message.channel) {
// Allow new thread creation only from the Responses API.
// Both checks are required:
// - channel == "gateway": server-set, unforgeable by WASM
// - metadata.source == "responses_api": set server-side in
// create_response_handler, not controllable by the web UI
// chat which also uses the gateway channel
let is_responses_api = message.channel == "gateway"
&& message.metadata.get("source").and_then(|v| v.as_str())
== Some("responses_api");
if !exists && is_responses_api {
tracing::debug!(
user = %message.user_id,
thread_id = %thread_uuid,
"Allowing new thread from gateway (Responses API)"
);
return None;
}
Comment on lines 265 to +282

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new exception for requires_preexisting_uuid_thread allows any gateway message with a non-existent UUID to proceed, but the PR description says this should only be allowed when metadata.source == "responses_api". As written, regular gateway traffic can now bypass the “pre-existing conversation UUID” requirement just by choosing a fresh UUID. Consider additionally gating this branch on message.metadata["source"] == "responses_api" (or equivalent) so the relaxation only applies to the Responses API path.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gateway channel is only accessible via the web server's authenticated routes — WASM and external channels cannot use it. The check !exists && message.channel == "gateway" means: only allow new thread creation when (a) the conversation does not exist in the DB yet, AND (b) the request came through the authenticated gateway. Existing conversations owned by other users are rejected by the conversation_belongs_to_user check above this code.

tracing::warn!(
user = %message.user_id,
channel = %message.channel,
Expand Down
176 changes: 138 additions & 38 deletions src/channels/web/responses_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,17 @@ pub struct ResponsesRequest {
pub tools: Option<Vec<ResponsesTool>>,
#[serde(default)]
pub tool_choice: Option<serde_json::Value>,
/// IronClaw extension: structured context injected into the agent's conversation.
///
/// NOT part of the OpenAI Responses API spec — IronClaw extension.
/// The `context` alias is kept for convenience but may collide with
/// a future OpenAI field; prefer `x_context`.
///
/// Used by integrations to pass structured data (notification responses,
/// approval status). Should be a flat `{key: {flat_object}}` structure;
/// nested objects are serialized as raw JSON. Max 10 KB.
#[serde(default, alias = "context")]
pub x_context: Option<serde_json::Value>,
}

fn default_model() -> String {
Expand Down Expand Up @@ -299,6 +310,35 @@ fn make_item_id() -> String {
format!("item_{}", Uuid::new_v4().simple())
}

/// Format structured context as a human-readable prefix for the agent.
fn format_context(ctx: &serde_json::Value) -> String {
let obj = match ctx.as_object() {
Some(o) => o,
None => return format!("[Context: {}]", ctx),
};
let mut parts = Vec::new();
for (key, value) in obj {
let detail = match value.as_object() {
Some(inner) => {
let fields: Vec<String> = inner
.iter()
.map(|(k, v)| {
let s = match v {
serde_json::Value::String(s) => s.clone(),
other => other.to_string(),
};
format!("{k}: {s}")
})
.collect();
format!("[Context: {key} \u{2014} {}]", fields.join(", "))
}
None => format!("[Context: {key}: {value}]"),
};
parts.push(detail);
}
parts.join("\n")
}

/// Extract the user message text from the input.
fn extract_user_content(input: &ResponsesInput) -> Result<String, String> {
match input {
Expand Down Expand Up @@ -622,9 +662,24 @@ pub async fn create_response_handler(
));
}

let content = extract_user_content(&req.input)
let mut content = extract_user_content(&req.input)
.map_err(|e| api_error(StatusCode::BAD_REQUEST, e, "invalid_request_error"))?;

// Prepend structured context (e.g. notification approval/rejection).
// Enforce a 10 KB size limit to prevent context window exhaustion.
if let Some(ref ctx) = req.x_context {
let ctx_bytes = serde_json::to_string(ctx).map(|s| s.len()).unwrap_or(0);
if ctx_bytes > 10 * 1024 {
return Err(api_error(
StatusCode::BAD_REQUEST,
format!("x_context exceeds 10 KB limit ({ctx_bytes} bytes)"),
"invalid_request_error",
));
}
let prefix = format_context(ctx);
content = format!("<user-context>\n{prefix}\n</user-context>\n\n{content}");
Comment on lines +669 to +680

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 10 KB limit is enforced on serde_json::to_string(ctx).len(), but the actual injected prompt prefix is format_context(ctx) plus wrapper tags/newlines. For many keys/fields, the formatted prefix can exceed the serialized JSON size (and therefore exceed 10 KB) even though this check passes. Consider enforcing the limit on prefix.len() (or on the total bytes added to content) so the guard matches what’s actually inserted into the conversation context window.

Suggested change
// Enforce a 10 KB size limit to prevent context window exhaustion.
if let Some(ref ctx) = req.x_context {
let ctx_bytes = serde_json::to_string(ctx).map(|s| s.len()).unwrap_or(0);
if ctx_bytes > 10 * 1024 {
return Err(api_error(
StatusCode::BAD_REQUEST,
format!("x_context exceeds 10 KB limit ({ctx_bytes} bytes)"),
"invalid_request_error",
));
}
let prefix = format_context(ctx);
content = format!("<user-context>\n{prefix}\n</user-context>\n\n{content}");
// Enforce a 10 KB size limit on the exact bytes injected into the prompt
// to prevent context window exhaustion.
if let Some(ref ctx) = req.x_context {
let prefix = format_context(ctx);
let wrapped_prefix = format!("<user-context>\n{prefix}\n</user-context>\n\n");
let prefix_bytes = wrapped_prefix.len();
if prefix_bytes > 10 * 1024 {
return Err(api_error(
StatusCode::BAD_REQUEST,
format!("x_context exceeds 10 KB limit ({prefix_bytes} bytes)"),
"invalid_request_error",
));
}
content = format!("{wrapped_prefix}{content}");

Copilot uses AI. Check for mistakes.
}

// Resolve or create thread.
let thread_uuid = match &req.previous_response_id {
Some(prev_id) => {
Expand All @@ -640,13 +695,17 @@ pub async fn create_response_handler(
let response_uuid = Uuid::new_v4();

// Build the message for the agent loop.
let mut metadata = serde_json::json!({
"thread_id": &thread_id_str,
"user_id": &user.user_id,
"source": "responses_api",
});
if let Some(ref ctx) = req.x_context {
metadata["context"] = ctx.clone();
}
let msg = IncomingMessage::new("gateway", &user.user_id, &content)
.with_thread(&thread_id_str)
.with_metadata(serde_json::json!({
"thread_id": &thread_id_str,
"user_id": &user.user_id,
"source": "responses_api",
}));
.with_metadata(metadata);

let resp_id = encode_response_id(&response_uuid, &thread_uuid);
let model = req.model.clone();
Expand Down Expand Up @@ -974,50 +1033,67 @@ async fn streaming_worker(
content.clone()
};
if !text.is_empty() {
match message_output_index {
Some(idx) => {
acc.output[idx] = ResponseOutputItem::Message {
id: make_item_id(),
role: "assistant".to_string(),
content: vec![MessageContent::OutputText { text }],
};
if let Some(item) = acc.output.get(idx) {
emit(
&tx,
"response.output_item.done",
&ResponseStreamEvent::OutputItemDone {
output_index: idx,
item: item.clone(),
},
);
}
}
let idx = match message_output_index {
Some(i) => i,
None => {
let idx = acc.output.len();
let item = ResponseOutputItem::Message {
// Create the output item first.
let i = acc.output.len();
let placeholder = ResponseOutputItem::Message {
id: make_item_id(),
role: "assistant".to_string(),
content: vec![MessageContent::OutputText { text }],
content: vec![MessageContent::OutputText {
text: String::new(),
}],
};
emit(
&tx,
"response.output_item.added",
&ResponseStreamEvent::OutputItemAdded {
output_index: idx,
item: item.clone(),
output_index: i,
item: placeholder.clone(),
},
);
emit(
&tx,
"response.output_item.done",
&ResponseStreamEvent::OutputItemDone {
output_index: idx,
item: item.clone(),
},
);
acc.output.push(item);
acc.output.push(placeholder);
i
}
};

// Emit the full text as a delta so streaming clients
// receive it via response.output_text.delta, but only
// when StreamChunks haven't already delivered the content.
if acc.text_chunks.is_empty() {
emit(
&tx,
"response.output_text.delta",
&ResponseStreamEvent::OutputTextDelta {
output_index: idx,
content_index: 0,
delta: text.clone(),
},
);
}
Comment on lines +1061 to 1074

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new streaming behavior conditionally skips emitting response.output_text.delta when StreamChunk events already delivered content (if acc.text_chunks.is_empty()). There isn’t currently a unit/integration test asserting that no duplicate OutputTextDelta is emitted in the “chunks + terminal Response” path. Adding a test around streaming_worker (or extracting the emission decision into a testable helper) would prevent regressions where clients see duplicated text again.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard if acc.text_chunks.is_empty() ensures: if StreamChunk events delivered content incrementally, the final Response event does NOT re-emit the full text as a duplicate delta. If no StreamChunks arrived (non-streaming LLM), the fallback single-delta path fires. The Response event always updates acc.output[idx] with the authoritative final text regardless — only the delta emission is conditional.


// Reuse the placeholder's ID so added→done correlation works.
let item_id =
if let Some(ResponseOutputItem::Message { id, .. }) = acc.output.get(idx) {
id.clone()
} else {
make_item_id()
};
let item = ResponseOutputItem::Message {
id: item_id,
role: "assistant".to_string(),
content: vec![MessageContent::OutputText { text }],
};
Comment on lines +1083 to +1087

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The id of the ResponseOutputItem::Message is being regenerated here using make_item_id(). However, a placeholder for this item was already created with a different ID, either in the StreamChunk handler or earlier in this Response handler. This leads to an inconsistency where the id of the item in the response.output_item.added event is different from the id in the response.output_item.done event. The item ID should remain stable throughout its lifecycle.

To fix this, you should reuse the ID from the placeholder item that already exists in acc.output[idx].

                    let item_id = if let Some(ResponseOutputItem::Message { id, .. }) = acc.output.get(idx) {
                        id.clone()
                    } else {
                        // This path should not be taken if a placeholder was correctly inserted.
                        make_item_id()
                    };
                    let item = ResponseOutputItem::Message {
                        id: item_id,
                        role: "assistant".to_string(),
                        content: vec![MessageContent::OutputText { text }],
                    };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d79e590 — now reuses the placeholder ID from acc.output[idx] instead of calling make_item_id() again.

acc.output[idx] = item.clone();
emit(
&tx,
"response.output_item.done",
&ResponseStreamEvent::OutputItemDone {
output_index: idx,
item,
},
);
Comment on lines +1039 to +1096

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated streaming finalization logic (placeholder output_item.added, conditional output_text.delta emission based on acc.text_chunks.is_empty(), and ID reuse for output_item.done) isn’t covered by unit tests. Since this behavior is client-visible and previously regressed (duplicate text / ID mismatch), add a focused test that drives streaming_worker (or an extracted helper) through both paths: (1) StreamChunk-delivered text (no terminal full-text delta) and (2) no StreamChunks (single full-text delta), asserting the emitted SSE event sequence and stable item IDs.

Copilot uses AI. Check for mistakes.
}
}

Expand Down Expand Up @@ -1472,4 +1548,28 @@ mod tests {
let json = serde_json::to_string(&ResponseStatus::Completed).expect("serialize");
assert_eq!(json, "\"completed\"");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — Non-blocking: Missing regression tests for 3 of 4 fixes

These two format_context tests are good, but the other three fixes lack automated regression coverage:

  1. Thread creation bypass (thread_ops.rs): No test verifying gateway messages with non-existent UUIDs return None from maybe_hydrate_thread
  2. UUID adoption (session_manager.rs): No test exercising create_thread_with_id through resolve_thread
  3. Streaming delta dedup: No test verifying the text_chunks.is_empty() guard prevents duplicate deltas

Per project rules, bug fixes should include tests that would have caught the bug. Could be a follow-up PR.

#[test]
fn format_context_notification_response() {
let ctx = serde_json::json!({
"notification_response": {
"notification_id": "msg_123",
"action": "approved",
"score": 72
}
});
let result = format_context(&ctx);
assert!(result.contains("[Context: notification_response"));
assert!(result.contains("notification_id: msg_123"));
assert!(result.contains("action: approved"));
assert!(result.contains("score: 72"));
}

#[test]
fn format_context_simple_value() {
let ctx = serde_json::json!({"status": "ok"});
let result = format_context(&ctx);
assert!(result.contains("status"));
assert!(result.contains("ok"));
}
}
Loading