chore: promote staging to staging-promote/c8f87537-24658689024 (2026-04-20 14:07 UTC) - #2733
Merged
Merged
Conversation
…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 #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 #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 #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).
github-actions
Bot
added
scope: tool/mcp
Code reviewFound 4 issues:
The code calls provider_extension: self.server_name.as_str().to_string(),Since
The pattern appears identically in McpServerName::new(&raw).unwrap_or_else(|e| {
tracing::debug!(...);
McpServerName::new("unknown")
.expect("'unknown' is a valid...")
})Recommend extracting to a helper method (e.g.,
The code has TODO comments indicating that
Line 111 checks Overall: The PR is well-architected and security-sound. All issues are improvements to reduce duplication and allocations; no bugs or security vulnerabilities detected. 🤖 Generated with Claude Code |
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-promotion from staging CI
Batch range:
7fb41555a9e55677d1aaea29ca567a5b369c2b05..038853f8ee651cce156565aab9a50294f550d778Promotion branch:
staging-promote/038853f8-24671092908Base:
staging-promote/c8f87537-24658689024Triggered by: Staging CI batch at 2026-04-20 14:07 UTC
Commits in this batch (42):
onboardfails with "Failed to save settings to database", butironclawstarts successfully and applies migrations #846) (fix(setup): run migrations during onboard when DATABASE_URL preset (#846) #2309)Current commits in this promotion (1)
Current base:
staging-promote/c8f87537-24658689024Current head:
staging-promote/038853f8-24671092908Current range:
origin/staging-promote/c8f87537-24658689024..origin/staging-promote/038853f8-24671092908Auto-updated by staging promotion metadata workflow
Waiting for gates:
Auto-created by staging-ci workflow