docs(comments): rewrite comments across all crates to the guidelines
Sweep every first-party crate source (1956 .rs files) to the project comment guidelines: delete redundant restatements, decorative banners, change narration, and end-of-line comments; keep and tighten the crucial ones (invariants, bug rationale, SAFETY blocks, ported-source attribution). No functional code changed. Every edit is proven comment-only against the prior tree by a comment-stripping lexer (string/char/raw-string aware) plus a separate doctest-fence check. Where removing a comment made rustfmt or clippy want to re-lay-out adjacent code, the minimal triggering comment is restored so code tokens stay byte-identical. Gates green: cargo fmt --all --check (0 diffs), cargo check and cargo clippy --workspace --all-targets (0 warnings). Adds scripts/check_codegen_comment_guidelines.py — the enforcement gate for these guidelines (flags banners, end-of-line comments, change narration, and commented-out code).
This commit is contained in:
@@ -75,8 +75,10 @@ pub fn acp_bridge_transport(
|
||||
invoker: Arc<dyn AcpReverseInvoker>,
|
||||
invoke_timeout: Duration,
|
||||
) -> AcpBridgeTransport {
|
||||
let (agent_read, pump_write) = tokio::io::duplex(BRIDGE_BUF); // server -> client
|
||||
let (pump_read, agent_write) = tokio::io::duplex(BRIDGE_BUF); // client -> server
|
||||
// server -> client
|
||||
let (agent_read, pump_write) = tokio::io::duplex(BRIDGE_BUF);
|
||||
// client -> server
|
||||
let (pump_read, agent_write) = tokio::io::duplex(BRIDGE_BUF);
|
||||
tokio::spawn(pump(
|
||||
server_id,
|
||||
invoker,
|
||||
@@ -142,7 +144,8 @@ async fn read_requests(
|
||||
loop {
|
||||
line.clear();
|
||||
match reader.read_line(&mut line).await {
|
||||
Ok(0) | Err(_) => break, // rmcp closed its end
|
||||
// rmcp closed its end
|
||||
Ok(0) | Err(_) => break,
|
||||
Ok(_) => {}
|
||||
}
|
||||
// Reap finished invokes so the set stays bounded.
|
||||
@@ -202,7 +205,8 @@ async fn write_responses(
|
||||
.is_err()
|
||||
|| server_to_client.flush().await.is_err()
|
||||
{
|
||||
break; // rmcp closed its end
|
||||
// rmcp closed its end
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -124,7 +124,8 @@ impl McpCredentialStore {
|
||||
}
|
||||
let err = std::io::Error::last_os_error();
|
||||
if err.kind() == std::io::ErrorKind::Interrupted {
|
||||
continue; // Retry on EINTR.
|
||||
// Retry on EINTR.
|
||||
continue;
|
||||
}
|
||||
// Lock failed for another reason — fall back to non-atomic insert.
|
||||
self.insert_rmcp(server_name, server_url, creds);
|
||||
|
||||
@@ -9,7 +9,7 @@
|
||||
//! crate to 0.13 to satisfy `rmcp` triggers a cascade — an OpenTelemetry
|
||||
//! `HttpClient` adapter and cross-version test breakage when a crate
|
||||
//! carries both versions under a renamed `package = "reqwest"` alias.
|
||||
//! reqwest 0.13 is now a fully private impl detail of [`servers`]; no
|
||||
//! reqwest 0.13 is a fully private impl detail of [`servers`]; no
|
||||
//! re-export. Consumers reach `rmcp` model types through this namespace
|
||||
//! (`kigi_mcp::rmcp::*`).
|
||||
//!
|
||||
|
||||
@@ -341,7 +341,8 @@ mod tests {
|
||||
let handle = spawn_transport_liveness(
|
||||
"test-server".to_string(),
|
||||
client,
|
||||
Duration::from_secs(60), // Long interval so the first tick is far away.
|
||||
// Long interval so the first tick is far away.
|
||||
Duration::from_secs(60),
|
||||
tx,
|
||||
Arc::clone(&slot),
|
||||
);
|
||||
|
||||
@@ -135,13 +135,13 @@ impl ThrottleState {
|
||||
///
|
||||
/// Backoff and episode state are per instance; the [`WarnBudget`] is the
|
||||
/// caller's, so a rebuilt client does not warn again within the cooldown.
|
||||
// No `Debug` derive: rmcp's `AuthClient` (an inner type) is not `Debug`.
|
||||
#[derive(Clone)]
|
||||
pub struct McpHttpClient<C> {
|
||||
inner: C,
|
||||
server_name: Arc<str>,
|
||||
state: Arc<parking_lot::Mutex<ThrottleState>>,
|
||||
}
|
||||
// No `Debug` derive: rmcp's `AuthClient` (an inner type) is not `Debug`.
|
||||
|
||||
impl<C> McpHttpClient<C> {
|
||||
pub fn new(inner: C, server_name: impl Into<Arc<str>>, warn_budget: WarnBudget) -> Self {
|
||||
|
||||
@@ -28,14 +28,12 @@ const MCP_OAUTH_CLIENT_NAME: &str = "Kigi";
|
||||
/// a login completed in another window or process.
|
||||
const CREDENTIAL_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(2);
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Two-layer dedup: prevents duplicate browser tabs both within one process
|
||||
// (multiple async tasks / sessions) and across separate processes (leader
|
||||
// mode disabled, multiple `kigi` invocations).
|
||||
//
|
||||
// Layer 1 (cross-process): filesystem lock at $KIGI_SHARE_DIR/mcp_auth_{safe_name}.lock
|
||||
// Layer 2 (in-process): watch channel so only one task runs the flow
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// In-process in-flight auth tracker. Keyed by server name.
|
||||
/// Each entry has a generation counter so that when a forced override evicts
|
||||
@@ -69,7 +67,7 @@ pub async fn authenticate_mcp_server_dedup(
|
||||
byo_config: Option<&McpOAuthConfig>,
|
||||
force: bool,
|
||||
) -> Result<(), String> {
|
||||
// --- Layer 2: in-process dedup via watch channel ---
|
||||
// Layer 2: in-process dedup via watch channel
|
||||
let mut in_flight = IN_FLIGHT_AUTH.lock().await;
|
||||
|
||||
// Remove stale entries left by panicked leaders (sender dropped).
|
||||
@@ -116,7 +114,7 @@ pub async fn authenticate_mcp_server_dedup(
|
||||
in_flight.insert(server_name.to_string(), InFlightEntry { rx, generation });
|
||||
drop(in_flight);
|
||||
|
||||
// --- Layer 1: cross-process dedup via filesystem lock (Unix only) ---
|
||||
// Layer 1: cross-process dedup via filesystem lock (Unix only)
|
||||
// When force is set, skip the fs lock — the old leader may still hold it
|
||||
// and we don't want to block behind a stale browser flow.
|
||||
#[cfg(unix)]
|
||||
|
||||
@@ -5,10 +5,8 @@
|
||||
|
||||
use std::collections::HashMap;
|
||||
|
||||
/// OAuth configuration extracted from an MCP server's config.
|
||||
///
|
||||
/// Travels alongside `acp::McpServer` (which can't be extended since it's
|
||||
/// an external crate type). Keyed by server name in [`McpOAuthConfigMap`].
|
||||
/// Travels alongside `acp::McpServer` rather than inside it, because that is an
|
||||
/// external crate type and can't carry extra fields.
|
||||
#[derive(Debug, Clone, Default)]
|
||||
pub struct McpOAuthConfig {
|
||||
pub client_id: Option<String>,
|
||||
@@ -23,5 +21,5 @@ impl McpOAuthConfig {
|
||||
}
|
||||
}
|
||||
|
||||
/// Per-server OAuth configuration map, keyed by MCP server name.
|
||||
/// Keyed by MCP server name.
|
||||
pub type McpOAuthConfigMap = HashMap<String, McpOAuthConfig>;
|
||||
|
||||
@@ -106,7 +106,7 @@ pub fn sanitize_descriptor_segment(s: &str) -> String {
|
||||
pub struct McpConfigDiff {
|
||||
/// Server names that are new or had their config changed.
|
||||
pub added: Vec<McpServerName>,
|
||||
/// Server names that were removed or had their config changed (old instance torn down).
|
||||
/// Server names no longer present, or whose config changed (old instance torn down).
|
||||
pub removed: Vec<McpServerName>,
|
||||
/// Server names whose config is identical — clients kept alive.
|
||||
pub retained: Vec<McpServerName>,
|
||||
@@ -567,7 +567,7 @@ impl McpState {
|
||||
}
|
||||
|
||||
/// Diff-based config update: only tears down servers whose config changed
|
||||
/// or were removed, keeps healthy unchanged servers alive.
|
||||
/// or dropped out of the new list, keeps healthy unchanged servers alive.
|
||||
///
|
||||
/// Returns `None` if configs are identical (no work needed), or `Some(diff)`
|
||||
/// describing which servers to add/remove.
|
||||
@@ -984,7 +984,8 @@ pub(crate) fn mcp_servers_equal(a: &[acp::McpServer], b: &[acp::McpServer]) -> b
|
||||
// Compare JSON serializations
|
||||
match (serde_json::to_string(a), serde_json::to_string(b)) {
|
||||
(Ok(a_json), Ok(b_json)) => a_json == b_json,
|
||||
_ => false, // If serialization fails, assume not equal
|
||||
// If serialization fails, assume not equal
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1303,7 +1304,8 @@ impl McpTool {
|
||||
.and_then(|ui| ui.get("visibility"))
|
||||
.and_then(|v| v.as_array())
|
||||
.map(|arr| arr.iter().any(|s| s.as_str() == Some("model")))
|
||||
.unwrap_or(true); // default: visible to model
|
||||
// default: visible to model
|
||||
.unwrap_or(true);
|
||||
|
||||
Some(McpToolRegistration {
|
||||
name: qualified_name,
|
||||
@@ -1964,7 +1966,8 @@ where
|
||||
loop {
|
||||
let mut line = Vec::new();
|
||||
match self.read.read_until(b'\n', &mut line).await {
|
||||
Ok(0) => return None, // genuine end-of-stream
|
||||
// genuine end-of-stream
|
||||
Ok(0) => return None,
|
||||
Ok(_) => {}
|
||||
Err(e) => {
|
||||
tracing::debug!(
|
||||
@@ -2492,11 +2495,11 @@ pub struct McpClient {
|
||||
/// has been updated. See [`ClientState`] for the single-flight
|
||||
/// invariant this preserves.
|
||||
///
|
||||
/// Replaces the previous fail-fast
|
||||
/// `McpError::ClientError("MCP client already initializing")` branch
|
||||
/// which leaked into model-visible tool results whenever the model's
|
||||
/// first tool dispatch raced the session actor's background
|
||||
/// `get_tool_registrations` handshake.
|
||||
/// Parking here rather than failing fast keeps an
|
||||
/// `McpError::ClientError("MCP client already initializing")` out of
|
||||
/// model-visible tool results when the model's first tool dispatch
|
||||
/// races the session actor's background `get_tool_registrations`
|
||||
/// handshake.
|
||||
init_done: Notify,
|
||||
startup_timeout_sec: u64,
|
||||
tool_timeout_sec: u64,
|
||||
@@ -3494,7 +3497,7 @@ impl McpClient {
|
||||
|
||||
/// Wire a sender for [`McpClientEvent`]s emitted by this client.
|
||||
///
|
||||
/// Mutates the shared slot synchronously. All previously-cloned
|
||||
/// Mutates the shared slot synchronously. All already-cloned
|
||||
/// references (the [`KigiClientHandler`] handed to
|
||||
/// `client.serve`, the [`crate::liveness::spawn_transport_liveness`]
|
||||
/// task) read through the same Arc, so this is observed
|
||||
@@ -4618,7 +4621,8 @@ mod tests {
|
||||
// Same configs should return false
|
||||
let changed = state.update_configs(configs.clone());
|
||||
assert!(!changed);
|
||||
assert_eq!(state.generation, 0); // Generation should not change
|
||||
// Generation should not change
|
||||
assert_eq!(state.generation, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -4630,7 +4634,8 @@ mod tests {
|
||||
let new_configs = vec![make_stdio_server("test2", "/bin/test2")];
|
||||
let changed = state.update_configs(new_configs);
|
||||
assert!(changed);
|
||||
assert_eq!(state.generation, 1); // Generation should increment
|
||||
// Generation should increment
|
||||
assert_eq!(state.generation, 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -4986,7 +4991,8 @@ mod tests {
|
||||
|
||||
state.cancel_init();
|
||||
assert!(!state.is_initializing());
|
||||
assert!(!state.is_initialized()); // Should NOT be marked as initialized
|
||||
// Should NOT be marked as initialized
|
||||
assert!(!state.is_initialized());
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -5472,7 +5478,7 @@ mod tests {
|
||||
assert!(Arc::ptr_eq(c1, c2));
|
||||
}
|
||||
|
||||
// ── owned/shared split behavioral tests ─────────────────────────
|
||||
// owned/shared split behavioral tests
|
||||
|
||||
#[test]
|
||||
fn test_get_client_owned_overrides_shared() {
|
||||
@@ -5728,7 +5734,7 @@ mod tests {
|
||||
assert!(tool.into_registration().is_none());
|
||||
}
|
||||
|
||||
// ── is_retriable_transport_error tests ───────────────────────────
|
||||
// is_retriable_transport_error tests
|
||||
|
||||
#[test]
|
||||
fn test_is_retriable_transport_closed() {
|
||||
@@ -6289,7 +6295,7 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
// ── new_http stores http_config tests ────────────────────────────
|
||||
// new_http stores http_config tests
|
||||
|
||||
#[test]
|
||||
fn test_new_http_stores_http_config() {
|
||||
@@ -6314,7 +6320,7 @@ mod tests {
|
||||
assert!(client.http_config.is_none());
|
||||
}
|
||||
|
||||
// ── http_headers_match / refresh_managed_clients guard tests ─────
|
||||
// http_headers_match / refresh_managed_clients guard tests
|
||||
|
||||
#[test]
|
||||
fn http_headers_match_compares_full_set_order_insensitively() {
|
||||
@@ -6453,7 +6459,7 @@ mod tests {
|
||||
assert!(after.http_headers_match(&fresh));
|
||||
}
|
||||
|
||||
// ── reset_transport tests ────────────────────────────────────────
|
||||
// reset_transport tests
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_reset_transport_succeeds_for_http_client() {
|
||||
@@ -6640,8 +6646,10 @@ mod tests {
|
||||
// its duplex ends. The next `call_tool` therefore observes a real
|
||||
// `ServiceError::TransportClosed`.
|
||||
async fn dead_service() -> McpService {
|
||||
let (client_read, server_write) = tokio::io::duplex(64 * 1024); // server -> client
|
||||
let (server_read, client_write) = tokio::io::duplex(64 * 1024); // client -> server
|
||||
// server -> client
|
||||
let (client_read, server_write) = tokio::io::duplex(64 * 1024);
|
||||
// client -> server
|
||||
let (server_read, client_write) = tokio::io::duplex(64 * 1024);
|
||||
tokio::spawn(async move {
|
||||
let mut reader = BufReader::new(server_read);
|
||||
let mut writer = server_write;
|
||||
@@ -6915,7 +6923,8 @@ mod tests {
|
||||
expose_image_base64: Some(true),
|
||||
..Default::default()
|
||||
};
|
||||
let meta = McpServerMetaConfig::default(); // expose_image_base64 = None
|
||||
// expose_image_base64 = None
|
||||
let meta = McpServerMetaConfig::default();
|
||||
assert!(McpClient::load_expose_image_base64(
|
||||
Some(&overrides),
|
||||
Some(&meta)
|
||||
@@ -6946,10 +6955,8 @@ mod tests {
|
||||
assert!(!client_default.expose_image_base64());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// ensure_initialized single-flight + Notify behavior (regression
|
||||
// suite for the "MCP client already initializing" doom-loop).
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
/// `ensure_initialized` on a stub (no transport) must surface a
|
||||
/// clear, actionable configuration error — never the legacy
|
||||
@@ -7183,7 +7190,7 @@ mod tests {
|
||||
tokio::time::sleep(std::time::Duration::from_millis(50)).await;
|
||||
|
||||
match &*client.state.lock().await {
|
||||
ClientState::Pending(_) => {} // expected
|
||||
ClientState::Pending(_) => {}
|
||||
other => panic!(
|
||||
"expected Pending after holder abort + drop guard, found {}",
|
||||
state_label(other)
|
||||
@@ -7299,7 +7306,7 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
// -- is_healthy / state_kind --------------------------------------
|
||||
// is_healthy / state_kind
|
||||
//
|
||||
// These tests cover the cheap, non-blocking predicate. They focus
|
||||
// on the state-machine inspection: any
|
||||
@@ -7395,7 +7402,7 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
// -- KigiClientHandler --------------------------------------
|
||||
// KigiClientHandler
|
||||
//
|
||||
// The handler's notification routing is the only behavior worth
|
||||
// unit-testing here; `get_info` is a literal `info.clone()` and
|
||||
|
||||
@@ -5,23 +5,20 @@
|
||||
//! Reference these constants instead of re-typing the literals so the agent and
|
||||
//! SDK can't drift apart.
|
||||
|
||||
/// Forward tool-invocation method (client -> agent): `kigi/mcp/call`.
|
||||
///
|
||||
/// The pager/client asks the agent to invoke an MCP tool on a server the agent is
|
||||
/// connected to, outside the LLM loop. See `extensions::mcp::handle_call`.
|
||||
/// Forward tool invocation (client -> agent): the pager/client asks the agent to
|
||||
/// invoke a tool on an MCP server the agent is connected to, outside the LLM loop.
|
||||
/// See `extensions::mcp::handle_call`.
|
||||
pub const MCP_CALL: &str = "kigi/mcp/call";
|
||||
|
||||
/// Reverse zero-IPC tool-invocation method (agent -> client): `kigi/mcp/sdk_call`.
|
||||
///
|
||||
/// The agent invokes a tool that lives in the SDK's in-process MCP server by sending
|
||||
/// the MCP JSON-RPC message back to the client over the ACP reverse channel. Distinct
|
||||
/// from [`MCP_CALL`] so the two disjoint schemas don't share a method string for
|
||||
/// metrics/tracing. See the agent-side ACP invoker that handles this method.
|
||||
/// Reverse zero-IPC tool invocation (agent -> client): the agent invokes a tool
|
||||
/// living in the SDK's in-process MCP server by sending the MCP JSON-RPC message
|
||||
/// back over the ACP reverse channel. Distinct from [`MCP_CALL`] so the two
|
||||
/// disjoint schemas don't share a method string for metrics/tracing.
|
||||
pub const MCP_SDK_CALL: &str = "kigi/mcp/sdk_call";
|
||||
|
||||
/// `session/new` `_meta` key listing in-process SDK MCP servers: `kigi/mcp/servers`.
|
||||
/// `session/new` `_meta` key listing in-process SDK MCP servers.
|
||||
pub const MCP_SERVERS: &str = "kigi/mcp/servers";
|
||||
|
||||
/// `initialize` `_meta` capability flag advertising in-process SDK MCP support
|
||||
/// (enables the SDK's `transport="acp"`): `kigi/mcp/sdk`.
|
||||
/// `initialize` `_meta` capability flag advertising in-process SDK MCP support,
|
||||
/// which enables the SDK's `transport="acp"`.
|
||||
pub const MCP_SDK: &str = "kigi/mcp/sdk";
|
||||
|
||||
@@ -150,8 +150,8 @@ async fn throttled_client_bounds_the_flood() {
|
||||
);
|
||||
let client = ().serve(transport).await.expect("handshake against fake server should succeed");
|
||||
|
||||
// Checkpoint: backoff must engage early (instant, instant, 0.5s => 3-4
|
||||
// GETs by 1.2s; an unthrottled client would be in the hundreds).
|
||||
// The schedule is instant, instant, 0.5s, so 1.2s allows 3-4 GETs; an
|
||||
// unthrottled client would be in the hundreds by now.
|
||||
tokio::time::sleep(Duration::from_millis(1200)).await;
|
||||
let early = gets.load(Ordering::Relaxed);
|
||||
assert!(
|
||||
|
||||
Reference in New Issue
Block a user