refactor(types): adopt MissionId in router + introduce McpServerName by ilblackdragon · Pull Request #2681 · nearai/ironclaw

ilblackdragon · 2026-04-19T14:39:25Z

Summary

PR 4 of 4 in the type-safety refactor series — closes the type gap for two identifiers flagged in the recent audit.

src/bridge/router.rs response DTO (EngineMissionInfo.id) now carries engine's MissionId(Uuid) instead of String. MissionId is already a tuple-struct newtype in ironclaw_engine, so serde serializes it as a bare UUID string — the on-wire contract is unchanged.
New McpServerName newtype in crates/ironclaw_common/src/identity.rs encapsulates the allowlist validation originally added as a free function in fix(mcp): validate server names with strict allowlist (fixes #1882) #2400 (alphanumeric, dash, underscore; rejects shell metacharacters, path separators, dots, NUL bytes). #[serde(transparent)] preserves the on-disk shape of ~/.ironclaw/mcp-servers.json.
McpServerConfig::validate() now delegates its name check to McpServerName::new, so the allowlist lives in exactly one place.
McpSessionManager keys its session map by &McpServerName instead of &str — the primary site that benefits from the type-system enforcement, since a session lookup keyed by the wrong string is the exact bug shape .claude/rules/types.md calls out.
create_client_from_config re-validates through McpServerName::new after hyphen folding so malformed legacy configs fail fast.
McpClient stores a typed McpServerName; the HttpMcpTransport.server_name field is also typed so session_manager.update_session_id(&self.server_name, ...) compiles. Both expose .as_str() accessors for log / format-string use.

Scope limits respected

Per the task's 15-file ceiling for the MCP side: McpServerConfig.name is still String on the wire (legacy persisted rows, many test call sites using intentionally-invalid names, many callers), and the stdio / unix / process / auth transports still carry String. Those sites are annotated with TODO(type-safety PR 4 of 4) comments referencing this PR.

Test plan

cargo check --all-features — passes
cargo clippy --all-features --tests — zero warnings
cargo test -p ironclaw_common — 53/53 pass (includes 10 new McpServerName cases)
cargo test --lib tools::mcp — 225/225 pass
cargo test --lib — 5162/5162 pass
Integration tier (cargo test --features integration) — not run in this environment (no PostgreSQL)

gemini-code-assist

Code Review

This pull request introduces the McpServerName newtype to enforce validation rules (alphanumeric, dash, underscore, and length limits) for MCP server identifiers across the codebase. It updates session management, configuration validation, and client initialization to use this typed identity, while also updating EngineMissionInfo to use a typed MissionId. Feedback focuses on improving efficiency by avoiding redundant allocations in TryFrom<String> and factory validation, as well as ensuring consistent hyphen-to-underscore normalization during client creation.

gemini-code-assist · 2026-04-19T14:42:46Z

+impl TryFrom<String> for McpServerName {
+    type Error = McpServerNameError;
+    fn try_from(value: String) -> Result<Self, Self::Error> {
+        Self::new(value)
+    }
+}


The TryFrom implementation for McpServerName is inefficient because it calls Self::new(), which takes a reference and performs an additional allocation via to_string(). Since the input is already an owned String, it should be validated and wrapped directly to avoid unnecessary overhead.

impl TryFrom<String> for McpServerName { type Error = McpServerNameError; fn try_from(value: String) -> Result<Self, Self::Error> { if value.is_empty() { return Err(McpServerNameError::Empty); } if value.len() > MAX_MCP_SERVER_NAME_LEN { return Err(McpServerNameError::TooLong); } if !value.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') { return Err(McpServerNameError::InvalidChar(value)); } Ok(Self(value)) } }

References

When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like replace that allocate a new string. This avoids unnecessary allocations for already-invalid inputs.

Fixed in ea1f629. TryFrom now validates directly on the owned String without the re-allocation round-trip.

gemini-code-assist · 2026-04-19T14:42:46Z

        let url: String = server_url.into();
-        let name = extract_server_name(&url);
-        let transport = Arc::new(HttpMcpTransport::new(url.clone(), name.clone()));
+        let name_str = extract_server_name(&url);


There is an inconsistency in how server names are normalized. McpClient::new uses extract_server_name which preserves hyphens, while McpClient::new_with_name and create_client_from_config fold hyphens to underscores. Consider applying hyphen folding here as well for consistency. To avoid unnecessary allocations, check if the string contains a hyphen before calling replace.

Suggested change

let name_str = extract_server_name(&url);

let name_raw = extract_server_name(&url);

let name_str = if name_raw.contains('-') {

name_raw.replace('-', "_")

} else {

name_raw

};

References

When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like replace that allocate a new string. This avoids unnecessary allocations for already-invalid inputs.

Fixed in ea1f629. McpClient::new now applies the hyphen fold consistently (only when a hyphen is present to avoid unnecessary allocation).

gemini-code-assist · 2026-04-19T14:42:46Z

+    McpServerName::new(&server_name).map_err(|e| McpFactoryError::InvalidConfig {
+        name: server_name.clone(),
+        reason: e.to_string(),
+    })?;


The result of McpServerName::new(&server_name) is discarded here, meaning the validation and internal allocation are performed only to be thrown away. Since server_name is cloned multiple times later in this function, it would be more efficient to capture the validated McpServerName and use it in the subsequent logic (e.g., passing it to new_with_transport), or at least avoid the redundant allocation if only validation is required.

References

When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like replace that allocate a new string. This avoids unnecessary allocations for already-invalid inputs.

For inputs that require validation (like timezone strings), prefer creating a newtype wrapper that validates on construction. This 'Parse, Don't Validate' approach ensures type safety and centralizes validation at system boundaries.

Fixed in ea1f629. Factory now captures the validated McpServerName and threads it through downstream where feasible; remaining String uses tagged with TODO for the follow-up pass.

Copilot

Pull request overview

This PR continues the type-safety refactor by (1) threading the engine’s typed MissionId through the bridge mission DTOs and (2) introducing a typed McpServerName identity with centralized allowlist validation, then using it to key MCP session state and reduce stringly-typed lookups.

Changes:

Update bridge mission DTOs (EngineMissionInfo.id) to carry ironclaw_engine::MissionId instead of String while preserving JSON wire shape.
Add McpServerName (+ error type and max-length constant) to ironclaw_common::identity, and delegate MCP config validation to it.
Key McpSessionManager by McpServerName and propagate typed names through MCP client/transport plumbing.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/tools/mcp/session.rs`	Change session map keys from `String` to `McpServerName` and update APIs/tests accordingly.
`src/tools/mcp/http_transport.rs`	Store typed `server_name: McpServerName` so session-manager lookups are keyed by the newtype.
`src/tools/mcp/factory.rs`	Re-validate normalized server names via `McpServerName::new` in the factory path; update test to use typed names for session manager calls.
`src/tools/mcp/config.rs`	Delegate `McpServerConfig::validate()` server-name validation to `McpServerName::new`.
`src/tools/mcp/client.rs`	Store `server_name` as `McpServerName`, add typed accessor, and adapt call sites to pass `&str` via `.as_str()`.
`src/bridge/router.rs`	Use `ironclaw_engine::MissionId` in mission DTOs and remove string round-tripping.
`crates/ironclaw_common/src/lib.rs`	Re-export `McpServerName`, `McpServerNameError`, and `MAX_MCP_SERVER_NAME_LEN`.
`crates/ironclaw_common/src/identity.rs`	Introduce `McpServerName` newtype, validation rules, errors, and unit tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-19T14:44:40Z

+        // `extract_server_name` is a heuristic URL parser that may emit
+        // names outside the strict allowlist (e.g. "unknown" when the URL
+        // cannot be parsed). `from_trusted` accepts whatever it produced;
+        // validation ran upstream in `McpServerConfig::validate` for the
+        // production path.
+        let name = McpServerName::from_trusted(name_str);


McpClient::new constructs server_name via McpServerName::from_trusted(extract_server_name(...)), but extract_server_name can still produce characters outside the allowlist (e.g. IPv6 hostnames include :). That defeats the goal of using McpServerName as an allowlist-gated identifier and can later produce invalid tool-name prefixes / session keys. Consider attempting McpServerName::new(&name_str) here (and falling back to a known-safe value like "unknown" or a sanitized/escaped form) so this constructor cannot create an unchecked name.

Suggested change

// `extract_server_name` is a heuristic URL parser that may emit

// names outside the strict allowlist (e.g. "unknown" when the URL

// cannot be parsed). `from_trusted` accepts whatever it produced;

// validation ran upstream in `McpServerConfig::validate` for the

// production path.

let name = McpServerName::from_trusted(name_str);

// `extract_server_name` is a heuristic URL parser, so validate its

// output before constructing an `McpServerName`. Fall back to a

// known-safe identifier if the extracted value is outside the

// allowlist.

let name = McpServerName::new(&name_str).unwrap_or_else(|_| {

McpServerName::new("unknown")

.expect("hardcoded fallback MCP server name must be valid")

});

Fixed in ea1f629. McpClient::new now validates through McpServerName::new and falls back to a canonical "unknown" value for invalid extracted names (e.g. IPv6 hosts). Debug-log on fallback. Added a test covering the IPv6 case.

Copilot · 2026-04-19T14:44:41Z

+        // Preserve historical hyphen-to-underscore folding so session
+        // keys match `create_client_from_config`'s canonicalization.
+        let raw: String = server_name.into().replace('-', "_");
+        let name = McpServerName::from_trusted(raw);


new_with_name takes a caller-provided string but wraps it with McpServerName::from_trusted, which allows invalid characters to enter the typed server_name field. This undermines the newtype’s purpose and can cause invalid tool-name prefixes / confusing session behavior. Prefer validating with McpServerName::new (and either returning an error, panicking in debug, or falling back to a safe canonical name) rather than marking arbitrary input as trusted.

Suggested change

let name = McpServerName::from_trusted(raw);

let name = match McpServerName::new(raw.clone()) {

Ok(name) => name,

Err(_) => {

debug_assert!(

false,

"new_with_name received invalid MCP server name after normalization: {raw}"

);

McpServerName::new("unknown".to_string())

.expect("hardcoded fallback MCP server name must be valid")

}

};

Fixed in ea1f629. new_with_name now validates after the hyphen fold with the same safe-fallback pattern — from_trusted is no longer used for caller-provided input.

Copilot · 2026-04-19T14:44:41Z

+/// typically require `^[a-zA-Z0-9_-]+$`), as components of secret-store keys
+/// (e.g. `mcp_<name>_access_token`), and as filesystem-adjacent identifiers.
+/// 64 bytes matches the shared `MAX_NAME_LEN` used for other identity names.
+pub const MAX_MCP_SERVER_NAME_LEN: usize = 64;


MAX_MCP_SERVER_NAME_LEN is documented as matching MAX_NAME_LEN but is duplicated as a literal 64. To prevent drift, consider defining it in terms of MAX_NAME_LEN (or otherwise reusing the shared constant) so any future change only needs to happen in one place.

Suggested change

pub const MAX_MCP_SERVER_NAME_LEN: usize = 64;

pub const MAX_MCP_SERVER_NAME_LEN: usize = MAX_NAME_LEN;

Fixed in ea1f629. MAX_MCP_SERVER_NAME_LEN is now an alias of MAX_NAME_LEN to prevent drift.

- src/bridge/router.rs response DTO now uses engine's MissionId(Uuid) instead of String - McpServerName newtype in ironclaw_common encapsulates the allowlist validation added in #2400 Closes the type gap for two identifiers flagged in the recent audit of string-typed values in the system.

…struction - McpClient::new and new_with_name: validate through McpServerName::new with a canonical "unknown" fallback + debug log, instead of from_trusted. Closes two HIGH-severity allowlist bypasses (e.g. IPv6 bracketed hosts and caller-supplied bad names). - McpClient::new: apply the same hyphen→underscore fold as the other constructors (only when a hyphen is present) for consistency. - identity.rs: extract validate_mcp_server_name() helper so TryFrom<String> consumes the owned buffer without re-allocating; MAX_MCP_SERVER_NAME_LEN aliased to MAX_NAME_LEN to prevent drift. - factory.rs: capture validated McpServerName and thread .as_str() through hottest downstream uses; TODO left for full threading. - Adds regression tests covering the IPv6-host and invalid-caller-name paths against the caller (McpClient::new / new_with_name).

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T03:56:38Z

    pub fn new(server_url: impl Into<String>, server_name: impl Into<String>) -> Self {
        Self {
            server_url: server_url.into(),
-            server_name: server_name.into(),
+            server_name: McpServerName::from_trusted(server_name.into()),


HttpMcpTransport::new accepts an arbitrary string for server_name but wraps it with McpServerName::from_trusted(...), so invalid/unvalidated names can be stored in the typed field (and later used for session-manager keys / tool-name prefixes). Since McpServerName::new already permits - and _, the hyphen-folding rationale in the doc comment doesn’t require bypassing validation. Consider validating here (e.g., McpServerName::new(...) with a safe fallback like "unknown" or a debug-assert) or changing the constructor to accept McpServerName directly once call sites migrate.

Fixed in 3c73970. HttpMcpTransport::new now validates through McpServerName::new with the same "unknown" canonical fallback + debug-log pattern used in McpClient::new and new_with_name. Two tests added: new_falls_back_on_invalid_server_name and new_preserves_valid_server_name.

… calls The no-panics CI check flagged two `.expect()` calls on `McpServerName::new("unknown")` fallbacks inside `McpClient::new` and `McpClient::new_with_name`. The literal `"unknown"` always satisfies the alnum-only validation rule, so the call is infallible. Document that with an inline `// safety: ...` comment per the project's panic-check suppression convention.

…d-and-mcp-server-name

…allback

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T05:25:27Z

+        // Caller (factory) already canonicalized and validated the name
+        // via `McpServerConfig::validate`; `from_trusted` preserves the
+        // hyphen-folded form. TODO(type-safety PR 4 of 4): accept
+        // `McpServerName` here directly.
+        let name = McpServerName::from_trusted(server_name.into());


new_with_transport wraps the caller-provided server_name with McpServerName::from_trusted, so an unvalidated string can enter the typed server_name field. If a caller passes an invalid name, the client can end up generating invalid tool prefixes and (when a session manager is present) create cache keys that violate the allowlist invariants. Consider validating here via McpServerName::new (with a safe fallback like "unknown" or returning a Result) so this constructor can't create an unchecked McpServerName.

Suggested change

// Caller (factory) already canonicalized and validated the name

// via `McpServerConfig::validate`; `from_trusted` preserves the

// hyphen-folded form. TODO(type-safety PR 4 of 4): accept

// `McpServerName` here directly.

let name = McpServerName::from_trusted(server_name.into());

// Validate the caller-provided name before storing it in the typed

// field. Fall back to a known-safe value to preserve the current

// constructor signature and behavior.

let server_name = server_name.into();

let name = McpServerName::new(server_name)

.unwrap_or_else(|_| McpServerName::from_trusted("unknown".to_string()));

Fixed in 6f1a77f. new_with_transport now validates the caller-provided name via McpServerName::new with the canonical "unknown" fallback + debug log, matching the pattern in new, new_with_name, and HttpMcpTransport::new. from_trusted is no longer used for caller-provided input. Added new_with_transport_falls_back_on_invalid_server_name + new_with_transport_preserves_valid_server_name regression tests.

Copilot · 2026-04-20T05:25:28Z

+            // TODO(type-safety PR 4 of 4): switch `McpServerConfig.name`
+            // to `McpServerName` so we can move this through without
+            // `from_trusted`. See PR description for deferred sites.
+            server_name: McpServerName::from_trusted(config.name.clone()),
            next_id: AtomicU64::new(1),


new_with_config stores server_name via McpServerName::from_trusted(config.name.clone()) while HttpMcpTransport::new independently validates (and may fall back to "unknown"). If config.name is invalid, the client's server_name and the transport/session-manager key can diverge, breaking Mcp-Session-Id tracking and tool-name prefixes. Consider constructing a single validated McpServerName here (same fallback/behavior as new/new_with_name) and using it for both the client field and the transport constructor.

Fixed in 6f1a77f. new_with_config now validates config.name once via McpServerName::new (with the same canonical "unknown" fallback) and threads the single validated name into both HttpMcpTransport::new and the client's typed field — they can no longer diverge. Regression tests new_with_config_falls_back_on_invalid_server_name and new_with_config_client_and_transport_server_name_agree pin the behavior.

Copilot · 2026-04-20T05:25:28Z

+            // TODO(type-safety PR 4 of 4): switch `McpServerConfig.name`
+            // to `McpServerName` so we can move this through without
+            // `from_trusted`.
+            server_name: McpServerName::from_trusted(config.name.clone()),
            next_id: AtomicU64::new(1),


new_authenticated sets server_name with McpServerName::from_trusted(config.name.clone()), but the HttpMcpTransport::new(config.name.clone(), ..) path validates and may fall back to a different canonical value (e.g. "unknown"). That creates an internal inconsistency where the transport updates session IDs under one key while the client looks them up under another, breaking session reuse and potentially tool prefixing. Suggest deriving a single validated McpServerName once (optionally applying the same hyphen-folding as the factory) and using it consistently for both the client field and the transport input.

Fixed in 6f1a77f. new_authenticated now validates config.name once with the canonical "unknown" fallback and passes the validated name into HttpMcpTransport::new — the OAuth path no longer risks session-id writes under one key with lookups under another. Regression tests new_authenticated_falls_back_on_invalid_server_name + new_authenticated_preserves_valid_server_name added.

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>

…fallback Address PR #2681 Copilot review comments 3108427003, 3108427048, and 3108427073. The three remaining constructors (`new_with_transport`, `new_with_config`, `new_authenticated`) were wrapping caller-provided names with `McpServerName::from_trusted`, allowing invalid values to enter the typed field. Beyond bypassing the allowlist, the `_with_config` and `_authenticated` paths also diverged from `HttpMcpTransport::new`, which independently validates and falls back to `"unknown"` — so an invalid name could leave the client keyed one way and the transport another, silently breaking `Mcp-Session-Id` tracking. Each constructor now runs the same canonicalize-with-fallback pattern used by `McpClient::new`, `new_with_name`, and `HttpMcpTransport::new`, and threads the single validated name into both the transport and the client's typed field so the two cannot diverge. Regression tests added (per .claude/rules/testing.md, "Test Through the Caller"): invalid config/transport inputs fall back to "unknown"; valid inputs survive; client and transport names agree for both cases.

…d-and-mcp-server-name # Conflicts: # crates/ironclaw_common/src/identity.rs # crates/ironclaw_common/src/lib.rs

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serrrfirat · 2026-04-20T12:30:05Z

+        // validation. The allowlist itself originated in #2400 as
+        // defence against shell-metacharacter injection when the name is
+        // interpolated into secret keys or tool-name prefixes.
+        McpServerName::new(&self.name).map_err(|e| ConfigError::InvalidConfig {


Medium Severity

This makes McpServerConfig::validate() inherit the new McpServerName 64-character cap, but the old validation here only enforced non-empty plus [A-Za-z0-9_-]. That means an existing persisted MCP server name longer than 64 chars will now fail validation and get silently dropped during load via the retain(...) paths in load_mcp_servers_from() / load_mcp_servers_from_db().

So this is not just a stricter new-input check — it is a backward-compat regression for existing mcp-servers.json / DB-backed configs.

Suggested fix: keep load-time compatibility for legacy overlength names (or add an explicit migration path) instead of filtering them out on read, and add a regression test that loads a persisted >64-character server name.

Fixed in e219184. The load paths (load_mcp_servers_from + load_mcp_servers_from_db) now in-place truncate overlong legacy names at a char boundary and emit a warn! documenting the migration, rather than silently dropping them via retain(...). Invalid-char cases still drop, matching pre-PR behavior for that class. Regression tests test_load_truncates_legacy_overlong_server_names (ASCII happy path, truncated to exactly the cap) and test_load_truncation_is_char_boundary_safe (multi-byte UTF-8 straddling byte 64 must not panic) added.

serrrfirat · 2026-04-20T12:30:06Z

+            // TODO(type-safety PR 4 of 4): switch `McpServerConfig.name`
+            // to `McpServerName` so we can move this through without
+            // `from_trusted`.
+            server_name: McpServerName::from_trusted(config.name.clone()),


Medium Severity

new_authenticated() and HttpMcpTransport::new() now disagree about invalid server names. The transport validates config.name and falls back to "unknown" on failure, but the client stores the same config.name via McpServerName::from_trusted(...).

If a direct caller passes an invalid or legacy name, the transport will update/read Mcp-Session-Id state under "unknown" while the client later looks up / terminates / marks initialization under the raw name. That breaks session reuse and initialization tracking in a very non-obvious way.

Suggested fix: validate/canonicalize once and pass the same McpServerName into both the transport and the client, or make new_authenticated() reject invalid names outright. A regression test for the authenticated constructor path would also help.

Already resolved in 6f1a77f (predates this review). new_authenticated now validates config.name once via McpServerName::new with the canonical "unknown" fallback and threads the single validated name into both HttpMcpTransport::new and the client's typed field — they can no longer diverge. Regression tests new_authenticated_falls_back_on_invalid_server_name and new_authenticated_preserves_valid_server_name pin the behavior.

This branch is a re-fork: a fresh checkout of upstream nearai/ironclaw at 640dcbd with the Terminal-3 fork delta re-applied on top. A convergence audit verified that every fork-owned file in this branch already matches staging byte-for-byte, or differs only by a recorded, explained decision (correct nearai#2681 spelling, the V31 migration numbering, and upstream-evolved code where upstream restructured). origin/staging at merge time is exactly cffa86d — the commit this branch already accounts for — so there are no new staging changes to port. The merge is therefore resolved wholly in favour of this branch's tree (verified ours-resolution); it introduces no content changes relative to the first parent and exists only to record the histories as merged so the PR becomes mergeable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…earai#2681) * refactor(types): adopt MissionId in router + introduce McpServerName - src/bridge/router.rs response DTO now uses engine's MissionId(Uuid) instead of String - McpServerName newtype in ironclaw_common encapsulates the allowlist validation added in nearai#2400 Closes the type gap for two identifiers flagged in the recent audit of string-typed values in the system. * refactor(mcp): address review feedback — validate server names at construction - McpClient::new and new_with_name: validate through McpServerName::new with a canonical "unknown" fallback + debug log, instead of from_trusted. Closes two HIGH-severity allowlist bypasses (e.g. IPv6 bracketed hosts and caller-supplied bad names). - McpClient::new: apply the same hyphen→underscore fold as the other constructors (only when a hyphen is present) for consistency. - identity.rs: extract validate_mcp_server_name() helper so TryFrom<String> consumes the owned buffer without re-allocating; MAX_MCP_SERVER_NAME_LEN aliased to MAX_NAME_LEN to prevent drift. - factory.rs: capture validated McpServerName and thread .as_str() through hottest downstream uses; TODO left for full threading. - Adds regression tests covering the IPv6-host and invalid-caller-name paths against the caller (McpClient::new / new_with_name). * fix(mcp): annotate panic-safe McpServerName::new("unknown") .expect() calls The no-panics CI check flagged two `.expect()` calls on `McpServerName::new("unknown")` fallbacks inside `McpClient::new` and `McpClient::new_with_name`. The literal `"unknown"` always satisfies the alnum-only validation rule, so the call is infallible. Document that with an inline `// safety: ...` comment per the project's panic-check suppression convention. * refactor(mcp): validate HttpMcpTransport::new server_name with safe fallback * refactor(mcp): validate McpClient constructor server names with safe fallback Address PR nearai#2681 Copilot review comments 3108427003, 3108427048, and 3108427073. The three remaining constructors (`new_with_transport`, `new_with_config`, `new_authenticated`) were wrapping caller-provided names with `McpServerName::from_trusted`, allowing invalid values to enter the typed field. Beyond bypassing the allowlist, the `_with_config` and `_authenticated` paths also diverged from `HttpMcpTransport::new`, which independently validates and falls back to `"unknown"` — so an invalid name could leave the client keyed one way and the transport another, silently breaking `Mcp-Session-Id` tracking. Each constructor now runs the same canonicalize-with-fallback pattern used by `McpClient::new`, `new_with_name`, and `HttpMcpTransport::new`, and threads the single validated name into both the transport and the client's typed field so the two cannot diverge. Regression tests added (per .claude/rules/testing.md, "Test Through the Caller"): invalid config/transport inputs fall back to "unknown"; valid inputs survive; client and transport names agree for both cases. * fix(mcp): migrate legacy overlong server names at load instead of dropping Address PR nearai#2681 review comment 3110617080. Before the `McpServerName` newtype landed, `McpServerConfig::validate()` only enforced non-empty + `[A-Za-z0-9_-]`. Delegating validation to `McpServerName::new` added a 64-byte length cap — and `load_mcp_servers_from*` silently dropped invalid configs via `retain(...)`, so a legacy persisted server name >64 bytes would vanish from the loaded config on upgrade. The load paths now in-place truncate overlong names at a char boundary (safe even if the pre-validation string contains multi-byte UTF-8), emit a `warn!` documenting the migration, and let the (now cap-satisfying) entry pass validation. Invalid-char cases still drop via `retain`, matching the pre-PR behavior for that class. Regression tests cover both the happy path (ASCII overlong name kept, truncated to exactly the cap) and char-boundary safety (multi-byte sequence straddling byte 64 must not panic).

* 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>

Copilot AI review requested due to automatic review settings April 19, 2026 14:39

github-actions Bot added scope: tool/mcp MCP client size: XL 500+ changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels Apr 19, 2026

Copilot started reviewing on behalf of ilblackdragon April 19, 2026 14:39 View session

gemini-code-assist Bot reviewed Apr 19, 2026

View reviewed changes

Copilot AI reviewed Apr 19, 2026

View reviewed changes

ilblackdragon added 2 commits April 20, 2026 12:08

ilblackdragon force-pushed the refactor/mission-id-and-mcp-server-name branch from 2456c6b to ea1f629 Compare April 20, 2026 03:14

Merge origin/staging into refactor/mission-id-and-mcp-server-name

3a936a5

Copilot AI review requested due to automatic review settings April 20, 2026 03:52

Copilot started reviewing on behalf of ilblackdragon April 20, 2026 03:52 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

ilblackdragon added 3 commits April 20, 2026 12:58

Merge remote-tracking branch 'origin/staging' into refactor/mission-i…

0e5154e

…d-and-mcp-server-name

refactor(mcp): validate HttpMcpTransport::new server_name with safe f…

3c73970

…allback

Copilot AI review requested due to automatic review settings April 20, 2026 05:19

Copilot started reviewing on behalf of ilblackdragon April 20, 2026 05:19 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

ilblackdragon mentioned this pull request Apr 20, 2026

docs(rules): add review-driven guidance for Claude Code #2714

Merged

4 tasks

ilblackdragon added 2 commits April 20, 2026 20:51

Merge remote-tracking branch 'origin/staging' into refactor/mission-i…

ba4392f

…d-and-mcp-server-name # Conflicts: # crates/ironclaw_common/src/identity.rs # crates/ironclaw_common/src/lib.rs

Copilot AI review requested due to automatic review settings April 20, 2026 12:02

Copilot started reviewing on behalf of ilblackdragon April 20, 2026 12:03 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

serrrfirat reviewed Apr 20, 2026

View reviewed changes

ironclaw-ci Bot mentioned this pull request Apr 30, 2026

chore: promote staging to main (2026-04-30 16:44 UTC) #3121

Closed

abbyshekit mentioned this pull request Jun 5, 2026

feat(reborn): add OpenAI-compatible product refs #4489

Merged

5 of 14 tasks

-        let name_str = extract_server_name(&url);
+        let name_raw = extract_server_name(&url);
+        let name_str = if name_raw.contains('-') {
+            name_raw.replace('-', "_")
+        } else {
+            name_raw
+        };

-        // `extract_server_name` is a heuristic URL parser that may emit
-        // names outside the strict allowlist (e.g. "unknown" when the URL
-        // cannot be parsed). `from_trusted` accepts whatever it produced;
-        // validation ran upstream in `McpServerConfig::validate` for the
-        // production path.
-        let name = McpServerName::from_trusted(name_str);
+        // `extract_server_name` is a heuristic URL parser, so validate its
+        // output before constructing an `McpServerName`. Fall back to a
+        // known-safe identifier if the extracted value is outside the
+        // allowlist.
+        let name = McpServerName::new(&name_str).unwrap_or_else(|_| {
+            McpServerName::new("unknown")
+                .expect("hardcoded fallback MCP server name must be valid")
+        });

-        let name = McpServerName::from_trusted(raw);
+        let name = match McpServerName::new(raw.clone()) {
+            Ok(name) => name,
+            Err(_) => {
+                debug_assert!(
+                    false,
+                    "new_with_name received invalid MCP server name after normalization: {raw}"
+                );
+                McpServerName::new("unknown".to_string())
+                    .expect("hardcoded fallback MCP server name must be valid")
+            }
+        };

	pub const MAX_MCP_SERVER_NAME_LEN: usize = 64;
	pub const MAX_MCP_SERVER_NAME_LEN: usize = MAX_NAME_LEN;

-        // Caller (factory) already canonicalized and validated the name
-        // via `McpServerConfig::validate`; `from_trusted` preserves the
-        // hyphen-folded form. TODO(type-safety PR 4 of 4): accept
-        // `McpServerName` here directly.
-        let name = McpServerName::from_trusted(server_name.into());
+        // Validate the caller-provided name before storing it in the typed
+        // field. Fall back to a known-safe value to preserve the current
+        // constructor signature and behavior.
+        let server_name = server_name.into();
+        let name = McpServerName::new(server_name)
+            .unwrap_or_else(|_| McpServerName::from_trusted("unknown".to_string()));

Conversation

ilblackdragon commented Apr 19, 2026

Summary

Scope limits respected

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment