refactor(channels): introduce ExternalThreadId newtype at channel boundary - #2685
Conversation
…ndary External channel thread ids (Telegram chat id, web UUID, Slack thread_ts) flow as raw Option<String> through IncomingMessage, StatusUpdate, and pending-gate store. Wraps them in a validated ExternalThreadId so the compiler distinguishes boundary-layer ids from the internal ThreadId(Uuid). Maps to bug pattern from #2349, #2444, #2517 where thread-id confusion crossed a layer silently.
github-actions
Bot
added
scope: agent
There was a problem hiding this comment.
Pull request overview
Introduces a typed ExternalThreadId at the channels boundary to prevent mixing external channel thread identifiers with internal engine ThreadId(Uuid), and updates affected message/gate plumbing accordingly.
Changes:
- Added
ExternalThreadId(+ error/limits) as a serde-transparent newtype inironclaw_common. - Replaced
Option<String>withOption<ExternalThreadId>on channel-boundary structs (IncomingMessage,OutgoingResponse,PendingGate). - Centralized pending-gate “wire thread id” selection via
PendingGate::effective_wire_thread_id()and updated call sites/tests.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Updates assertions to read ExternalThreadId as &str. |
| tests/telegram_auth_integration.rs | Updates thread id assertions for newtype. |
| src/tools/builtin/message.rs | Updates test assertions for newtype thread id. |
| src/gate/pending.rs | Switches scope_thread_id to ExternalThreadId and adds effective_wire_thread_id(). |
| src/channels/web/util.rs | Updates metadata insertion to read thread id via as_str(). |
| src/channels/web/tests/no_silent_drop.rs | Updates test construction to use ExternalThreadId. |
| src/channels/web/server.rs | Updates tests to assert on newtype thread id. |
| src/channels/web/platform/ws.rs | Updates WS tests to assert on newtype thread id. |
| src/channels/web/mod.rs | Converts ExternalThreadId to String for routing where required. |
| src/channels/wasm/wrapper.rs | Passes optional thread id to WASM as Option<&str>. |
| src/channels/tui.rs | Converts response thread id to Option<String> for TUI events; updates tests. |
| src/channels/signal.rs | Updates tests to use ExternalThreadId in messages. |
| src/channels/relay/channel.rs | Updates thread id extraction as Option<&str> via as_str(). |
| src/channels/channel.rs | Changes IncomingMessage.thread_id/OutgoingResponse.thread_id to ExternalThreadId and adds helpers. |
| src/bridge/router.rs | Uses effective_wire_thread_id() and adjusts scope thread handling for newtype. |
| src/agent/thread_ops.rs | Adds TODO comment about future ExternalThreadId plumbing. |
| src/agent/routine_engine.rs | Wraps routine thread ids at boundary via from_trusted. |
| src/agent/job_monitor.rs | Updates tests for ExternalThreadId. |
| src/agent/heartbeat.rs | Wraps heartbeat thread ids at boundary via from_trusted. |
| src/agent/agent_loop.rs | Converts ExternalThreadId to Option<String> for hook events. |
| crates/ironclaw_common/src/lib.rs | Re-exports ExternalThreadId + related constants/errors. |
| crates/ironclaw_common/src/identity.rs | Implements ExternalThreadId newtype, errors, constants, and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[error("external thread id exceeds {MAX_EXTERNAL_THREAD_ID_LEN} characters")] | ||
| TooLong, | ||
| #[error("external thread id must not contain NUL bytes")] |
There was a problem hiding this comment.
ExternalThreadIdError::TooLong says "exceeds … characters", but the validation uses s.len() which is a byte length in Rust (UTF-8). Please align the error message/docs with the actual constraint (bytes), or change the check to count Unicode scalar values if you truly mean characters.
There was a problem hiding this comment.
Fixed in 107160d. Updated error text and doc comments to say "bytes" (the check uses s.len() which is a byte count).
| impl TryFrom<String> for ExternalThreadId { | ||
| type Error = ExternalThreadIdError; | ||
| fn try_from(value: String) -> Result<Self, Self::Error> { | ||
| Self::new(value) |
There was a problem hiding this comment.
impl TryFrom<String> for ExternalThreadId currently calls Self::new(value), which clones the string again inside new (to_string()). Consider validating value.as_str() and, if valid, returning ExternalThreadId(value) to avoid an extra allocation/copy.
| Self::new(value) | |
| Self::validate(value.as_str())?; | |
| Ok(Self(value)) |
There was a problem hiding this comment.
Fixed in 107160d. Extracted a private validate(&str) helper on both ExternalThreadId and McpServerName; TryFrom now consumes the owned String without the to_string() round-trip.
| /// Set the thread ID. | ||
| /// | ||
| /// Accepts raw strings — the value is wrapped with | ||
| /// [`ExternalThreadId::from_trusted`] because callers are typically | ||
| /// channel adapters forwarding a value already emitted by their external | ||
| /// platform. The `conversation_scope_id` shadow mirrors the raw string. | ||
| pub fn with_thread(mut self, thread_id: impl Into<String>) -> Self { | ||
| let thread_id = thread_id.into(); | ||
| self.conversation_scope_id = Some(thread_id.clone()); | ||
| self.thread_id = Some(ExternalThreadId::from_trusted(thread_id)); | ||
| self |
There was a problem hiding this comment.
IncomingMessage::with_thread wraps the provided string with ExternalThreadId::from_trusted, which bypasses the newtype's validation. This method is used for external inputs (e.g. the HTTP webhook request thread_id), so NUL/empty/oversized values can still flow through. Consider adding a validating variant (e.g. try_with_thread(...) -> Result<Self, ExternalThreadIdError>) and migrating untrusted call sites to it, or have with_thread validate and introduce an explicit with_thread_unchecked for the escape hatch.
There was a problem hiding this comment.
Fixed in 107160d. Added try_with_thread (IncomingMessage) and try_in_thread (OutgoingResponse) validating variants returning Result<(), ExternalThreadIdError>. Migrated untrusted call sites: src/channels/http.rs (webhook, returns 400 on invalid) and src/channels/relay/channel.rs (logs warning, drops thread_id on invalid). Left with_thread/in_thread as trusted-path conveniences with updated docs pointing to try_ variants.
| /// Set the thread ID for the response. | ||
| /// | ||
| /// Accepts raw strings — the value is wrapped with | ||
| /// [`ExternalThreadId::from_trusted`] because callers typically forward | ||
| /// an identifier that a channel adapter already accepted. | ||
| pub fn in_thread(mut self, thread_id: impl Into<String>) -> Self { | ||
| self.thread_id = Some(thread_id.into()); | ||
| self.thread_id = Some(ExternalThreadId::from_trusted(thread_id.into())); | ||
| self |
There was a problem hiding this comment.
OutgoingResponse::in_thread also uses ExternalThreadId::from_trusted, so callers can attach invalid external thread identifiers even though ExternalThreadId::new exists. If responses can be influenced by untrusted metadata (e.g. relays), prefer a validating setter (returning Result) and reserve from_trusted for clearly-internal sources.
There was a problem hiding this comment.
Fixed in 107160d. Added try_with_thread (IncomingMessage) and try_in_thread (OutgoingResponse) validating variants returning Result<(), ExternalThreadIdError>. Migrated untrusted call sites: src/channels/http.rs (webhook, returns 400 on invalid) and src/channels/relay/channel.rs (logs warning, drops thread_id on invalid). Left with_thread/in_thread as trusted-path conveniences with updated docs pointing to try_ variants.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ExternalThreadId and McpServerName newtypes to provide typed boundaries and validation for external identifiers, replacing raw String usage throughout the agent, bridge, and channel modules. Review feedback highlights opportunities to improve technical accuracy in error messages by specifying "bytes" instead of "characters," and to optimize performance by extracting validation logic to avoid redundant allocations in TryFrom<String> implementations. Additionally, it is suggested to use #[serde(try_from = "...")] to ensure data integrity during deserialization from external sources.
| #[error("external thread id exceeds {MAX_EXTERNAL_THREAD_ID_LEN} characters")] | ||
| TooLong, |
There was a problem hiding this comment.
The error message mentions 'characters', but the implementation in ExternalThreadId::new uses s.len(), which returns the number of bytes in Rust. For UTF-8 strings, these are not equivalent. Please update the error message to say 'bytes' to be technically accurate and consistent with the implementation.
| #[error("external thread id exceeds {MAX_EXTERNAL_THREAD_ID_LEN} characters")] | |
| TooLong, | |
| #[error("external thread id exceeds {MAX_EXTERNAL_THREAD_ID_LEN} bytes")] |
There was a problem hiding this comment.
Fixed in 107160d. Updated error text and doc comments to say "bytes" (the check uses s.len() which is a byte count).
| #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub struct ExternalThreadId(String); |
There was a problem hiding this comment.
Using #[serde(transparent)] with a derived Deserialize implementation bypasses the validation logic defined in new(). This violates the general rule: 'When constructing a domain-specific type... from an external, untrusted source... use a validating constructor (e.g., ::new)'. If this type is used in any struct deserialized from external input, invalid data could enter the system. Consider using #[serde(try_from = "String")] to ensure validation at the boundary.
References
- When constructing a domain-specific type from an external, untrusted source, use a validating constructor (e.g., ::new) instead of a trusted constructor to ensure data is canonicalized and validated at the boundary.
There was a problem hiding this comment.
Declining this change. .claude/rules/types.md explicitly states: "Don't use #[serde(try_from = \"String\")] for identity newtypes without a migration plan. Existing persisted rows may not satisfy the current rule; transparent + explicit validation at construction preserves them while still gaining type distinctness." Callers that read from untrusted input already go through ::new or TryFrom<String> after deserialize (see e.g. the new try_with_thread/try_in_thread variants on IncomingMessage/OutgoingResponse).
| pub fn new(raw: impl AsRef<str>) -> Result<Self, ExternalThreadIdError> { | ||
| let s = raw.as_ref(); | ||
| if s.is_empty() { | ||
| return Err(ExternalThreadIdError::Empty); | ||
| } | ||
| if s.len() > MAX_EXTERNAL_THREAD_ID_LEN { | ||
| return Err(ExternalThreadIdError::TooLong); | ||
| } | ||
| if s.contains('\0') { | ||
| return Err(ExternalThreadIdError::ContainsNul); | ||
| } | ||
| Ok(Self(s.to_string())) | ||
| } |
There was a problem hiding this comment.
The current implementation of new and TryFrom<String> results in an unnecessary allocation when an owned String is provided, as new always calls to_string(). Extracting the validation logic into a private method allows TryFrom<String> to consume the string without re-allocating.
| pub fn new(raw: impl AsRef<str>) -> Result<Self, ExternalThreadIdError> { | |
| let s = raw.as_ref(); | |
| if s.is_empty() { | |
| return Err(ExternalThreadIdError::Empty); | |
| } | |
| if s.len() > MAX_EXTERNAL_THREAD_ID_LEN { | |
| return Err(ExternalThreadIdError::TooLong); | |
| } | |
| if s.contains('\0') { | |
| return Err(ExternalThreadIdError::ContainsNul); | |
| } | |
| Ok(Self(s.to_string())) | |
| } | |
| pub fn new(raw: impl AsRef<str>) -> Result<Self, ExternalThreadIdError> { | |
| let s = raw.as_ref(); | |
| Self::validate(s)?; | |
| Ok(Self(s.to_string())) | |
| } | |
| fn validate(s: &str) -> Result<(), ExternalThreadIdError> { | |
| if s.is_empty() { | |
| return Err(ExternalThreadIdError::Empty); | |
| } | |
| if s.len() > MAX_EXTERNAL_THREAD_ID_LEN { | |
| return Err(ExternalThreadIdError::TooLong); | |
| } | |
| if s.contains('\0') { | |
| return Err(ExternalThreadIdError::ContainsNul); | |
| } | |
| Ok(()) | |
| } |
References
- When canonicalizing a string, perform cheaper validation checks on a trimmed slice before performing more expensive operations that allocate a new string to avoid unnecessary allocations for already-invalid inputs.
There was a problem hiding this comment.
Fixed in 107160d. Extracted a private validate(&str) helper on both ExternalThreadId and McpServerName; TryFrom now consumes the owned String without the to_string() round-trip.
| impl TryFrom<String> for ExternalThreadId { | ||
| type Error = ExternalThreadIdError; | ||
| fn try_from(value: String) -> Result<Self, Self::Error> { | ||
| Self::new(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation performs an unnecessary string clone because Self::new calls to_string(). By using a shared validation method, you can wrap the existing String directly.
| impl TryFrom<String> for ExternalThreadId { | |
| type Error = ExternalThreadIdError; | |
| fn try_from(value: String) -> Result<Self, Self::Error> { | |
| Self::new(value) | |
| } | |
| } | |
| impl TryFrom<String> for ExternalThreadId { | |
| type Error = ExternalThreadIdError; | |
| fn try_from(value: String) -> Result<Self, Self::Error> { | |
| Self::validate(&value)?; | |
| Ok(Self(value)) | |
| } | |
| } |
References
- When canonicalizing a string, perform cheaper validation checks on a trimmed slice before performing more expensive operations that allocate a new string to avoid unnecessary allocations for already-invalid inputs.
There was a problem hiding this comment.
Fixed in 107160d. Extracted a private validate(&str) helper on both ExternalThreadId and McpServerName; TryFrom now consumes the owned String without the to_string() round-trip.
| #[error("MCP server name exceeds {MAX_MCP_SERVER_NAME_LEN} characters")] | ||
| TooLong, |
There was a problem hiding this comment.
Similar to ExternalThreadId, the error message here refers to 'characters' while the check uses s.len() (bytes). Please update for consistency.
| #[error("MCP server name exceeds {MAX_MCP_SERVER_NAME_LEN} characters")] | |
| TooLong, | |
| #[error("MCP server name exceeds {MAX_MCP_SERVER_NAME_LEN} bytes")] |
There was a problem hiding this comment.
Fixed in 107160d. Updated error text and doc comments to say "bytes" (the check uses s.len() which is a byte count).
| #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub struct McpServerName(String); |
There was a problem hiding this comment.
As with ExternalThreadId, deriving Deserialize with #[serde(transparent)] bypasses validation. Use #[serde(try_from = "String")] to enforce the alphanumeric/dash/underscore invariants when loading from external sources.
References
- When constructing a domain-specific type from an external, untrusted source, use a validating constructor (e.g., ::new) instead of a trusted constructor to ensure data is canonicalized and validated at the boundary.
There was a problem hiding this comment.
Declining this change. .claude/rules/types.md explicitly states: "Don't use #[serde(try_from = \"String\")] for identity newtypes without a migration plan. Existing persisted rows may not satisfy the current rule; transparent + explicit validation at construction preserves them while still gaining type distinctness." Callers that read from untrusted input already go through ::new or TryFrom<String> after deserialize (see e.g. the new try_with_thread/try_in_thread variants on IncomingMessage/OutgoingResponse).
| pub fn new(raw: impl AsRef<str>) -> Result<Self, McpServerNameError> { | ||
| let s = raw.as_ref(); | ||
| if s.is_empty() { | ||
| return Err(McpServerNameError::Empty); | ||
| } | ||
| if s.len() > MAX_MCP_SERVER_NAME_LEN { | ||
| return Err(McpServerNameError::TooLong); | ||
| } | ||
| if !s | ||
| .chars() | ||
| .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') | ||
| { | ||
| return Err(McpServerNameError::InvalidChar(s.to_string())); | ||
| } | ||
| Ok(Self(s.to_string())) | ||
| } |
There was a problem hiding this comment.
Extracting validation logic here will also prevent double allocations in the TryFrom<String> implementation for McpServerName.
pub fn new(raw: impl AsRef<str>) -> Result<Self, McpServerNameError> {
let s = raw.as_ref();
Self::validate(s)?;
Ok(Self(s.to_string()))
}
fn validate(s: &str) -> Result<(), McpServerNameError> {
if s.is_empty() {
return Err(McpServerNameError::Empty);
}
if s.len() > MAX_MCP_SERVER_NAME_LEN {
return Err(McpServerNameError::TooLong);
}
if !s
.chars()
.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
{
return Err(McpServerNameError::InvalidChar(s.to_string()));
}
Ok(())
}References
- When canonicalizing a string, perform cheaper validation checks on a trimmed slice before performing more expensive operations that allocate a new string to avoid unnecessary allocations for already-invalid inputs.
There was a problem hiding this comment.
Fixed in 107160d. Extracted a private validate(&str) helper on both ExternalThreadId and McpServerName; TryFrom now consumes the owned String without the to_string() round-trip.
| impl TryFrom<String> for McpServerName { | ||
| type Error = McpServerNameError; | ||
| fn try_from(value: String) -> Result<Self, Self::Error> { | ||
| Self::new(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize this TryFrom implementation to avoid cloning the string by calling a shared validation method.
| impl TryFrom<String> for McpServerName { | |
| type Error = McpServerNameError; | |
| fn try_from(value: String) -> Result<Self, Self::Error> { | |
| Self::new(value) | |
| } | |
| } | |
| impl TryFrom<String> for McpServerName { | |
| type Error = McpServerNameError; | |
| fn try_from(value: String) -> Result<Self, Self::Error> { | |
| Self::validate(&value)?; | |
| Ok(Self(value)) | |
| } | |
| } |
References
- When canonicalizing a string, perform cheaper validation checks on a trimmed slice before performing more expensive operations that allocate a new string to avoid unnecessary allocations for already-invalid inputs.
There was a problem hiding this comment.
Fixed in 107160d. Extracted a private validate(&str) helper on both ExternalThreadId and McpServerName; TryFrom now consumes the owned String without the to_string() round-trip.
…thread-id-newtype # Conflicts: # src/gate/pending.rs
Post-merge fix: a test added in staging (insert_and_notify_pending_gate_uses_extension_manager_for_auth_display_name) assigned a raw String to message.thread_id, but the field type became ExternalThreadId on this branch. Wrap with ExternalThreadId::from_trusted to match the other tests in the same module.
…thread-id-newtype # Conflicts: # crates/ironclaw_common/src/lib.rs # src/channels/channel.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Accepts raw strings — the value is wrapped with | ||
| /// [`ExternalThreadId::from_trusted`] because callers are typically | ||
| /// channel adapters forwarding a value already emitted by their external | ||
| /// platform. The `conversation_scope_id` shadow mirrors the raw string. | ||
| pub fn with_thread(mut self, thread_id: impl Into<String>) -> Self { | ||
| let thread_id = thread_id.into(); | ||
| self.conversation_scope_id = Some(thread_id.clone()); | ||
| self.thread_id = Some(ExternalThreadId::from_trusted(thread_id)); |
There was a problem hiding this comment.
IncomingMessage::with_thread wraps the raw thread_id via ExternalThreadId::from_trusted, which bypasses the newtype’s length/NUL/empty validation. This method is used with untrusted inputs (e.g. HTTP webhook req.thread_id), so it should validate (ExternalThreadId::new) and handle invalid values (e.g. drop thread_id and/or return an error) rather than accepting arbitrary strings.
| /// Accepts raw strings — the value is wrapped with | |
| /// [`ExternalThreadId::from_trusted`] because callers are typically | |
| /// channel adapters forwarding a value already emitted by their external | |
| /// platform. The `conversation_scope_id` shadow mirrors the raw string. | |
| pub fn with_thread(mut self, thread_id: impl Into<String>) -> Self { | |
| let thread_id = thread_id.into(); | |
| self.conversation_scope_id = Some(thread_id.clone()); | |
| self.thread_id = Some(ExternalThreadId::from_trusted(thread_id)); | |
| /// Accepts raw strings and validates them with [`ExternalThreadId::new`]. | |
| /// If the value is invalid, the thread ID is not set. When valid, the | |
| /// `conversation_scope_id` shadow mirrors the validated raw string. | |
| pub fn with_thread(mut self, thread_id: impl Into<String>) -> Self { | |
| let thread_id = thread_id.into(); | |
| if let Ok(thread_id) = ExternalThreadId::new(thread_id) { | |
| self.conversation_scope_id = Some(thread_id.as_str().to_string()); | |
| self.thread_id = Some(thread_id); | |
| } |
There was a problem hiding this comment.
Fixed in 107160d. Added try_with_thread (IncomingMessage) and try_in_thread (OutgoingResponse) validating variants returning Result<(), ExternalThreadIdError>. Migrated untrusted call sites: src/channels/http.rs (webhook, returns 400 on invalid) and src/channels/relay/channel.rs (logs warning, drops thread_id on invalid). Left with_thread/in_thread as trusted-path conveniences with updated docs pointing to try_ variants.
| /// Set the thread ID for the response. | ||
| /// | ||
| /// Accepts raw strings — the value is wrapped with | ||
| /// [`ExternalThreadId::from_trusted`] because callers typically forward | ||
| /// an identifier that a channel adapter already accepted. | ||
| pub fn in_thread(mut self, thread_id: impl Into<String>) -> Self { | ||
| self.thread_id = Some(thread_id.into()); | ||
| self.thread_id = Some(ExternalThreadId::from_trusted(thread_id.into())); | ||
| self | ||
| } |
There was a problem hiding this comment.
OutgoingResponse::in_thread also uses ExternalThreadId::from_trusted for raw strings, which means outbound routing can carry empty/NUL/oversized thread IDs without any guard. Consider validating here too (or adding a separate fallible constructor) so callers can’t accidentally propagate malformed external thread IDs across channel boundaries.
There was a problem hiding this comment.
Fixed in 107160d. Added try_with_thread (IncomingMessage) and try_in_thread (OutgoingResponse) validating variants returning Result<(), ExternalThreadIdError>. Migrated untrusted call sites: src/channels/http.rs (webhook, returns 400 on invalid) and src/channels/relay/channel.rs (logs warning, drops thread_id on invalid). Left with_thread/in_thread as trusted-path conveniences with updated docs pointing to try_ variants.
| request_id: gate.request_id.to_string(), | ||
| thread_id: gate | ||
| .scope_thread_id | ||
| .clone() | ||
| .as_ref() | ||
| .map(|t| t.as_str().to_string()) | ||
| .unwrap_or_else(|| gate.thread_id.to_string()), | ||
| gate_name: gate.gate_name.clone(), |
There was a problem hiding this comment.
PendingGateView::from re-implements the same fallback logic as PendingGate::effective_wire_thread_id(). Consider calling gate.effective_wire_thread_id() here to keep a single source of truth and avoid drift if the routing semantics change again.
There was a problem hiding this comment.
Fixed in 107160d. PendingGateView::from now calls gate.effective_wire_thread_id() — single source of truth.
…thread-id-newtype
…e, try_-variants, dedup pending-gate
…thread-id-newtype Resolves modify/delete conflict: staging deleted src/channels/web/server.rs (ironclaw#2599 stage 6) — the four thread-id test assertions this branch had added there already live at their new homes in src/channels/web/features/chat/mod.rs and features/pairing/mod.rs. Ported the thread_id.as_deref() -> .as_ref().map(|t| t.as_str()) fix to those new locations since incoming.thread_id is now Option<ExternalThreadId>.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/channels/relay/channel.rs:253
- RelayChannel builds a validated
msg.thread_idviatry_with_thread(candidate), but the metadata fieldthread_idis set toevent.thread_id.unwrap_or(&event.id)and can be empty / diverge from the chosen candidate (e.g., whenevent.idis empty and you fall back tochannel_id). This can later cause responses to be sent with an empty/incorrectthread_ts. Consider setting metadatathread_idfrom the samecandidateyou actually accept, and omit it when empty/invalid.
let mut msg = IncomingMessage::new(&relay_name, &event.sender_id, event.text())
.with_user_name(event.display_name())
.with_metadata(serde_json::json!({
"team_id": event.team_id(),
"channel_id": event.channel_id,
"sender_id": event.sender_id,
"sender_name": event.display_name(),
"event_type": event.event_type,
"thread_id": event.thread_id.as_deref().unwrap_or(&event.id),
"provider": event.provider,
}));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scope_thread_id: message | ||
| .conversation_scope() | ||
| .map(|s| ironclaw_common::ExternalThreadId::from_trusted(s.to_string())), |
There was a problem hiding this comment.
scope_thread_id is being set from message.conversation_scope() using ExternalThreadId::from_trusted, which bypasses validation. conversation_scope_id can be sourced from untrusted/extension-provided metadata (e.g., WASM-emitted routing targets), so this may allow empty/NUL/oversized values to be persisted into pending gates. Prefer constructing with ExternalThreadId::new(...) and, on error, log + store None (or otherwise handle the invalid scope) instead of bypassing validation here.
| scope_thread_id: message | |
| .conversation_scope() | |
| .map(|s| ironclaw_common::ExternalThreadId::from_trusted(s.to_string())), | |
| scope_thread_id: message.conversation_scope().and_then(|s| { | |
| let scope = s.to_string(); | |
| match ironclaw_common::ExternalThreadId::new(scope.clone()) { | |
| Ok(scope_thread_id) => Some(scope_thread_id), | |
| Err(err) => { | |
| tracing::warn!( | |
| thread_id = %thread_id, | |
| conversation_scope = %scope, | |
| error = %err, | |
| "dropping invalid conversation scope for pending authentication gate" | |
| ); | |
| None | |
| } | |
| } | |
| }), |
There was a problem hiding this comment.
Fixed in f9d1415. Same validation applied here — ExternalThreadId::new with debug log + None on error — closing the from_trusted bypass for WASM/metadata-sourced conversation_scope_id values.
| scope_thread_id: message | ||
| .conversation_scope() | ||
| .map(|s| ironclaw_common::ExternalThreadId::from_trusted(s.to_string())), |
There was a problem hiding this comment.
Same issue as above: scope_thread_id is built with ExternalThreadId::from_trusted from message.conversation_scope(), which can carry untrusted strings. Consider validating via ExternalThreadId::new and dropping/logging invalid scope ids rather than persisting unvalidated values into PendingGate.
| scope_thread_id: message | |
| .conversation_scope() | |
| .map(|s| ironclaw_common::ExternalThreadId::from_trusted(s.to_string())), | |
| scope_thread_id: message.conversation_scope().and_then(|s| { | |
| match ironclaw_common::ExternalThreadId::new(s.to_string()) { | |
| Ok(scope_thread_id) => Some(scope_thread_id), | |
| Err(e) => { | |
| tracing::debug!( | |
| scope = %s, | |
| error = %e, | |
| "dropping invalid conversation scope thread id" | |
| ); | |
| None | |
| } | |
| } | |
| }), |
There was a problem hiding this comment.
Fixed in f9d1415. scope_thread_id now validates via ExternalThreadId::new; invalid values are logged at debug and stored as None instead of being wrapped via from_trusted. Applied at both this call site and the authentication-fallback path at :3797.
| // Determine thread_id from response or metadata | ||
| let thread_id = response | ||
| .thread_id | ||
| .as_deref() | ||
| .as_ref() | ||
| .map(|t| t.as_str()) | ||
| .or_else(|| metadata.get("thread_id").and_then(|v| v.as_str())); |
There was a problem hiding this comment.
respond() currently derives thread_id from response.thread_id or metadata["thread_id"], but ignores the (now-typed/validated) msg.thread_id. This can produce incorrect routing when metadata is missing/stale. Prefer using msg.thread_id.as_ref().map(|t| t.as_str()) before falling back to metadata, and consider filtering out empty strings so you don't emit thread_ts: "".
There was a problem hiding this comment.
Fixed in f9d1415. respond() now prefers response.thread_id → msg.thread_id (the newly-validated ExternalThreadId) → metadata["thread_id"], with a trailing .filter(|s| !s.is_empty()) so we never emit thread_ts: "".
Synthesizes recurring patterns from ~30 merged PRs, 147 bot review comments (Copilot/Gemini), human reviews, and ~50 issues filed in the past 2 weeks. Each rule cites the motivating PR/issue numbers. New files: - error-handling.md — silent-failure taxonomy (unwrap_or_default, .ok()?, poisoned caches), persist-then-reload atomicity, channel-edge error mapping. (#2526, #2633, #2653, #2673, #2546, #2407, #2408) - agent-evidence.md — side-effect claims must cite tool evidence, empty-fast outputs are errors, external-effect tools must read back, setup UI round-trip. (#2544, #2580, #2582, #2541, #2545, #2411, #2543, #2586) - lifecycle.md — discovery vs. activation, terminal auth rejection, list_installed vs. list_active, deactivation unwinds, snapshot rehydrate must re-validate. (#2556, #2557, #2558, #2564, #2419, PR #2617, PR #2631) Extended: - types.md — from_trusted boundary rule, validated-newtype template with shared validate(&str), serde(try_from) required for validated types, wire-stable enums (no Debug; serde alias for migrations), canonical wire-contract field naming. (PR #2685, #2681, #2687, #2678, #2669, #2665, #2683, #2702) - safety-and-sandbox.md — every new ingress scans pre-transform/pre- injection, bounded resources (interners/streams/fan-out caps), cache keys must be complete. (#2491, #2676, #2470, #2633, #2673, #2710, PR #2702) - review-discipline.md — PR scope discipline, guardrail scripts are code (regression tests, grouped-import parsing, CI has_code inclusion), absolute-path ban in committed docs, stale comments after refactors. (PR #2668, #2628, #2680, #2687, #2647, #2689, #2701) All new files carry paths: frontmatter so they auto-load only on matching files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed msg.thread_id - router.rs: scope_thread_id written to PendingGate was wrapped via ExternalThreadId::from_trusted from message.conversation_scope(), which can carry untrusted WASM/metadata-sourced strings. Now validates via ExternalThreadId::new; invalid values log at debug and store None. Applied at both call sites (authentication-fallback path and generic gate-insertion path). - relay/channel.rs: respond() derived thread_id only from response or metadata — now also consults the validated msg.thread_id as the second fallback (before raw metadata) and filters empty strings so we never emit thread_ts: "" to Slack.
…ndary (nearai#2685) * refactor(channels): introduce ExternalThreadId newtype at channel boundary External channel thread ids (Telegram chat id, web UUID, Slack thread_ts) flow as raw Option<String> through IncomingMessage, StatusUpdate, and pending-gate store. Wraps them in a validated ExternalThreadId so the compiler distinguishes boundary-layer ids from the internal ThreadId(Uuid). Maps to bug pattern from nearai#2349, nearai#2444, nearai#2517 where thread-id confusion crossed a layer silently. * fix(bridge): adapt test thread_id to ExternalThreadId newtype Post-merge fix: a test added in staging (insert_and_notify_pending_gate_uses_extension_manager_for_auth_display_name) assigned a raw String to message.thread_id, but the field type became ExternalThreadId on this branch. Wrap with ExternalThreadId::from_trusted to match the other tests in the same module. * refactor(types): address review feedback — byte units, shared validate, try_-variants, dedup pending-gate * refactor(types): validate scope_thread_id + relay respond prefers typed msg.thread_id - router.rs: scope_thread_id written to PendingGate was wrapped via ExternalThreadId::from_trusted from message.conversation_scope(), which can carry untrusted WASM/metadata-sourced strings. Now validates via ExternalThreadId::new; invalid values log at debug and store None. Applied at both call sites (authentication-fallback path and generic gate-insertion path). - relay/channel.rs: respond() derived thread_id only from response or metadata — now also consults the validated msg.thread_id as the second fallback (before raw metadata) and filters empty strings so we never emit thread_ts: "" to Slack.
* docs(rules): add review-driven guidance for Claude Code Synthesizes recurring patterns from ~30 merged PRs, 147 bot review comments (Copilot/Gemini), human reviews, and ~50 issues filed in the past 2 weeks. Each rule cites the motivating PR/issue numbers. New files: - error-handling.md — silent-failure taxonomy (unwrap_or_default, .ok()?, poisoned caches), persist-then-reload atomicity, channel-edge error mapping. (nearai#2526, nearai#2633, nearai#2653, nearai#2673, nearai#2546, nearai#2407, nearai#2408) - agent-evidence.md — side-effect claims must cite tool evidence, empty-fast outputs are errors, external-effect tools must read back, setup UI round-trip. (nearai#2544, nearai#2580, nearai#2582, nearai#2541, nearai#2545, nearai#2411, nearai#2543, nearai#2586) - lifecycle.md — discovery vs. activation, terminal auth rejection, list_installed vs. list_active, deactivation unwinds, snapshot rehydrate must re-validate. (nearai#2556, nearai#2557, nearai#2558, nearai#2564, nearai#2419, PR nearai#2617, PR nearai#2631) Extended: - types.md — from_trusted boundary rule, validated-newtype template with shared validate(&str), serde(try_from) required for validated types, wire-stable enums (no Debug; serde alias for migrations), canonical wire-contract field naming. (PR nearai#2685, nearai#2681, nearai#2687, nearai#2678, nearai#2669, nearai#2665, nearai#2683, nearai#2702) - safety-and-sandbox.md — every new ingress scans pre-transform/pre- injection, bounded resources (interners/streams/fan-out caps), cache keys must be complete. (nearai#2491, nearai#2676, nearai#2470, nearai#2633, nearai#2673, nearai#2710, PR nearai#2702) - review-discipline.md — PR scope discipline, guardrail scripts are code (regression tests, grouped-import parsing, CI has_code inclusion), absolute-path ban in committed docs, stale comments after refactors. (PR nearai#2668, nearai#2628, nearai#2680, nearai#2687, nearai#2647, nearai#2689, nearai#2701) All new files carry paths: frontmatter so they auto-load only on matching files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(rules): split agent-evidence into prompt + code rule agent-evidence.md mixed two concerns: runtime agent instruction (what the LLM should do when concluding a turn) and code-enforcement rules (what the dispatcher, engine, and tools must implement). Rules under .claude/rules/ only guide Claude Code when editing the repo — the runtime agent never reads them. Splits the two: - crates/ironclaw_engine/prompts/codeact_postamble.md — new section "Evidence before claiming side effects". Sits next to the existing "FINAL() answer quality" guidance; loaded via include_str! in executor/prompt.rs (no Rust change needed). - .claude/rules/tool-evidence.md — renamed from agent-evidence.md, keeps only the code invariants (engine v2 side-effect gate, empty-fast ToolError::EmptyResult, external-effect tools must read back, setup UI round-trip). Prompt tests pass unchanged; the postamble addition is pure text. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * prompt: tighten evidence rule to FINAL() claims only, not tool use Live-test validation of the "Evidence before claiming side effects" section (added in the prior commit) showed it inhibited legitimate tool use. With the original wording, `zizmor_scan_v2` live-recording timed out at 302s with zero responses; reverting the postamble restored healthy behavior (88s run, 8 shell calls including `cargo install zizmor` and full workflow analysis). The original phrasing conflated two things: what the agent should claim and what tools it should call. The rule is only about the claim. Re-tunes the section to: - Open with an explicit "this does not restrict tool calls" scope. - Drop the "<1ms = failure" heuristic (too broad — normal tools like `tool_info(schema)` are legitimately fast). - Drop the full enumeration of forbidden side-effect verbs; keep the rule narrower and clearer. - Shorten the code example (remove redundant early-return). Re-tuned run: agent is active (shell calls, real reasoning), live recording completes in ~9s. The remaining test failure is a pre-existing assertion bug (exact `t == "shell"` match against tool strings that now carry arguments like `"shell(cmd)"`) — reproduces with the old postamble too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(live): fix tool-name assertions + re-record zizmor traces The two `zizmor_scan*` live tests had four broken tool-name assertions that silently failed to match: `tools.iter().any(|t| t == "shell")` against a tool list that now contains `"shell(cmd)"` strings (tool events carry args via `format_action_display_name` in `src/bridge/router.rs`). Two of the four were negative assertions checking for the absence of `tool_install` recovery loops — those silently passed even when a recovery loop actually ran. `sandbox_live_e2e.rs:203` already used the correct `t == "shell" || t.starts_with("shell(")` pattern; applied it consistently to all four sites. Verified live: - `IRONCLAW_LIVE_TEST=1 cargo test --test e2e_live -- zizmor_scan --ignored --test-threads=1` → 2 passed, 0 failed, 51.78s. Agent installs and runs zizmor end-to-end, producing real findings (exit code 14, dangerous triggers, excessive permissions, etc.). Traces re-recorded with the tuned postamble (commit 50d8517) and scrubbed: replaced `/home/illia/.cargo/bin/zizmor` with `/home/user/.cargo/bin/zizmor` per the developer-local-path ban in `.claude/rules/review-discipline.md`. No credentials, PII, or high-entropy secrets in either trace (only git SHAs from zizmor's workflow analysis output). Replay still passes: `cargo test --test e2e_live -- zizmor_scan --ignored` → 2/2 ok. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(replay): update zizmor_scan_v2 insta snapshot The engine v2 replay-snapshot gate (`engine_v2_tests::snapshot_zizmor_scan_v2`) failed against the re-recorded trace from 1691efe because the old snapshot encoded a broken run: - final_state: Failed - Missing Assistant message role - 3 issues: thread_failure (error), no_response (warning), llm_error (error) - 6 tool calls that never produced a final answer The new trace completes cleanly: - final_state: Done - System / User / Assistant roles present - 1 issue: mixed_mode (info) - 3 shell tool calls + successful `FINAL()` with real findings The snapshot was pinning a regression. Regenerated with `INSTA_UPDATE=always cargo test --test e2e_engine_v2 -- snapshot_zizmor_scan_v2`; passes on replay. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(rules): address PR nearai#2714 review feedback - review-discipline: reword "Doc Absolute Paths" as a review convention (pre-commit only scans .rs; the rule misleadingly claimed enforcement). - safety-and-sandbox: broaden `paths:` frontmatter to include the actual ingress owners (`bridge`, `channels`, `workspace`, `agent`, engine crate) so the rule auto-loads where it applies. - tool-evidence: mark the side-effect gate, empty-fast rule, and `unverified` flag as target/aspirational invariants — neither `ToolError::EmptyResult`, an `unverified` field on `ToolOutput`, nor a byte-count field on `ActionRecord` exist today. Point at concrete interim conventions (`ToolError::ExecutionFailed`, `unverified: true` in the JSON result body). - types: scope "Validated newtypes must gate Deserialize" to *new* types, document the `CredentialName`/`ExtensionName` exception (they intentionally use `#[serde(transparent)]` + derived `Deserialize` under the `serde_does_not_revalidate` test). Clarify the `from_trusted` trust boundary (trusted = typed upstream, untrusted = raw JSON field even if the field *name* is "registry entry"). Switch `new` template to `impl Into<String>` to avoid an unnecessary clone when an owned `String` is passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(rules): simplify types.md + split doc-hygiene; address review round 2 - types: collapse two templates into one canonical validated-newtype shape. New types use `#[serde(try_from = "String")]` with a shared `validate(&str)` helper — no more dual "transparent for some / try_from for others" guidance. `CredentialName`/`ExtensionName` are documented as the sole legacy exception (locked in by the `serde_does_not_revalidate` test); new code must not copy their `transparent` + `from_trusted` pattern. Removes the long "Using `from_trusted` safely" section and the separate "Validated newtypes must gate Deserialize" subsection that contradicted the Don'ts list. - doc-hygiene: new tiny rule file scoped to `**/*.md`, `**/*.py`, `docs/**` that carries the "no developer-local absolute paths in committed docs" convention. Removed from review-discipline.md where its `src/**/*.rs` scope meant the rule never loaded on the files it governed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(live): match hyphenated tool-install in attempted_relevant_tool The engine records `action_name` as the raw string the LLM emitted (`crates/ironclaw_engine/src/executor/structured.rs:381`), and the registry's lookup canonicalization only affects dispatch — not the name that reaches `StatusUpdate::ToolStarted`. The two other predicates in this file (`bad_recovery` at :420, `phase_b_recovery` at :531) already defend against both forms; this one should too, for consistency. Addresses PR nearai#2714 review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
PR 3 of 4 in the type-safety refactor series (enforcing
.claude/rules/types.md).ExternalThreadId(String)newtype incrates/ironclaw_common/src/identity.rswith validation (non-empty, ≤ 512 bytes, no NUL),from_trustedescape hatch,#[serde(transparent)]wire compatibility, and noFrom<String>/From<&str>/Derefto force explicit boundary crossings.Option<String>withOption<ExternalThreadId>on the channel-boundary structs:IncomingMessage.thread_id,OutgoingResponse.thread_id,PendingGate.scope_thread_id. Internal engineThreadId(Uuid)is untouched.PendingGate::effective_wire_thread_id()sobridge/routerstops repeatingscope_thread_id.clone().or_else(|| Some(thread_id.to_string()))at everyAppEvent::GateRequired/OnboardingStateDto::*build site.This makes it a compile error to confuse a Telegram chat id or web-UI thread string with the engine's internal
ThreadId(Uuid)— the exact shape behind #2349, #2444, #2517.Test plan
cargo test -p ironclaw_common— 61 passed, 0 failed (includes newExternalThreadIdserde/validation tests)cargo test --lib channels— 873 passed, 0 failedcargo test— full unit-test suite passescargo clippy --all-features --tests— 0 warningscargo check --all-features— cleancargo fmt --check— cleanNotes
AppEventwire payloads keepthread_id: Option<String>(they are the channel-boundary JSON contract). Conversion happens viaExternalThreadId::into()/effective_wire_thread_id().HookEvent,TuiEvent::Response, and the routine/heartbeat notification paths convert at the newtype boundary via.as_str().to_string()orfrom_trusted.src/agent/thread_ops.rs::maybe_hydrate_threadstill takes&str— a TODO is in place. Converting it requires movingSessionManager::resolve_thread'sOption<&str>parameter in lockstep, which is the next PR's scope.