From 506c10fdce1b50c85de8e23669594ac93f5863d4 Mon Sep 17 00:00:00 2001 From: Albert Lionelle Date: Tue, 9 Jun 2026 17:56:47 -0600 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20QoL=20=E2=80=94=20@-path=20guard,?= =?UTF-8?q?=20cache-handle=20errors,=20recommended=5Fmax=5Fplans?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issues 5–8 from the MCP tester report (quality-of-life): - 5: parse_yaml_source fast-fails when yaml_content starts with `@` (a caller mistaking it for an at-path reference). A leading `@` is a reserved YAML indicator and previously stalled the parser for the full MCP timeout; now it returns a directive error pointing at yaml_path / degree_id. - 6: YamlCache::insert only runs the O(n) expiry sweep once the cache passes a soft threshold (64) instead of on every write, so the write lock is held briefly in the common case (the intermittent cache_yaml timeouts). - 7: add YamlCache::lookup + CacheLookup so an aged-out handle reports "expired" distinctly from a never-minted "unknown" one (with re-cache hints); fix the cache_yaml description's stale "about 1 hour" TTL → 24 h. - 8: AnalysisResponse gains recommended_max_plans — the machine-readable companion to the truncation followup (None for full population, the current cap when CV-stable, else the doubled-and-capped next). Extracted shared complexity_cv / next_max_plans / recommend_max_plans helpers (DRY with the followup path) and a build_response_notes helper. Tests: @-prefix rejection, cache lookup unknown/expired/hit, and recommended_max_plans tracking truncation on a select-1-of-4 degree. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/mcp/cache.rs | 77 +++++++++++++++-- src/mcp/server.rs | 38 +++++---- src/mcp/tools/analyze.rs | 179 +++++++++++++++++++++++++++++++++------ src/mcp/tools/shared.rs | 31 +++++++ 4 files changed, 278 insertions(+), 47 deletions(-) diff --git a/src/mcp/cache.rs b/src/mcp/cache.rs index 3e7ed7a..ba2d5e3 100644 --- a/src/mcp/cache.rs +++ b/src/mcp/cache.rs @@ -41,6 +41,23 @@ pub const YAML_CACHE_PREFIX: &str = "cache:"; pub const YAML_CACHE_TTL: Duration = Duration::from_hours(24); const ARTIFACT_CACHE_CAPACITY: usize = 4; +/// Entry count past which [`YamlCache::insert`] runs the O(n) expiry sweep. +/// Keeps the common-case write cheap while still bounding cache growth. +const SWEEP_THRESHOLD: usize = 64; + +/// Outcome of looking up a `cache:` handle. +/// +/// Distinguishes a handle that was never minted from one that has aged out, so +/// callers can give a precise hint instead of a single "unknown or expired". +pub enum CacheLookup { + /// Handle is live; carries the body and the remaining TTL. + Hit(Arc, Duration), + /// Handle was minted but has passed its TTL. + Expired, + /// No such handle was ever minted (typo, or a different server process). + Unknown, +} + // ============================================================================ // YAML cache // ============================================================================ @@ -67,10 +84,16 @@ impl YamlCache { format!("{YAML_CACHE_PREFIX}{:016x}", hasher.finish()) } - /// Insert a YAML body, returning its handle. Sweeps expired entries as - /// a side-effect so the cache doesn't grow unboundedly under heavy use. + /// Insert a YAML body, returning its handle. + /// + /// The expiry sweep is O(n), so we only run it once the cache has grown + /// past `SWEEP_THRESHOLD` rather than on every write — keeping the write + /// lock held briefly in the common case (the field report's intermittent + /// `cache_yaml` timeouts) while still bounding growth under heavy use. pub fn insert(&mut self, body: String) -> String { - self.sweep_expired(); + if self.entries.len() >= SWEEP_THRESHOLD { + self.sweep_expired(); + } let handle = Self::handle_for(&body); self.entries.insert( handle.clone(), @@ -88,10 +111,24 @@ impl YamlCache { /// re-cache before it lapses mid-session. #[must_use] pub fn get(&self, handle: &str) -> Option<(Arc, Duration)> { - let entry = self.entries.get(handle)?; - let elapsed = entry.inserted_at.elapsed(); - let remaining = YAML_CACHE_TTL.checked_sub(elapsed)?; - Some((entry.body.clone(), remaining)) + match self.lookup(handle) { + CacheLookup::Hit(body, remaining) => Some((body, remaining)), + CacheLookup::Expired | CacheLookup::Unknown => None, + } + } + + /// Like [`Self::get`] but distinguishes an aged-out handle from one that + /// was never minted, so the caller can surface a precise hint. + #[must_use] + pub fn lookup(&self, handle: &str) -> CacheLookup { + let Some(entry) = self.entries.get(handle) else { + return CacheLookup::Unknown; + }; + YAML_CACHE_TTL + .checked_sub(entry.inserted_at.elapsed()) + .map_or(CacheLookup::Expired, |remaining| { + CacheLookup::Hit(entry.body.clone(), remaining) + }) } /// Current entry count. Exposed for the `cache_yaml` response so callers @@ -326,6 +363,32 @@ mod tests { assert!(cache.get("cache:deadbeefdeadbeef").is_none()); } + #[test] + fn test_lookup_distinguishes_unknown_expired_and_hit() { + let mut cache = YamlCache::default(); + // Never minted → Unknown. + assert!(matches!( + cache.lookup("cache:0000000000000000"), + CacheLookup::Unknown + )); + + // Fresh handle → Hit with a remaining TTL. + let handle = cache.insert("degree:\n id: lk\n".to_string()); + assert!(matches!(cache.lookup(&handle), CacheLookup::Hit(_, _))); + + // Backdate past the TTL window → Expired (distinct from Unknown), so + // the server can tell the caller to re-cache rather than guess a typo. + let stale = Instant::now() + .checked_sub(YAML_CACHE_TTL + Duration::from_secs(1)) + .expect("backdated instant supported on this platform"); + cache + .entries + .get_mut(&handle) + .expect("present after insert") + .inserted_at = stale; + assert!(matches!(cache.lookup(&handle), CacheLookup::Expired)); + } + #[test] fn test_make_artifact_key_order_independent_for_include_courses() { // Same set of courses in different orders → same cache key. diff --git a/src/mcp/server.rs b/src/mcp/server.rs index 870c9cf..3f6868e 100644 --- a/src/mcp/server.rs +++ b/src/mcp/server.rs @@ -311,7 +311,7 @@ impl NuAnalyticsMcpServer { /// Cache an inline YAML body and return a degree_id-style handle #[tool( - description = "Cache an inline degree YAML body in the server and return a handle (\"cache:{hex}\") that any other tool accepts as a `degree_id`. Removes the per-call repaste tax for hosted MCP clients whose filesystem the server can't see (yaml_path returns ENOENT in that setup). Handle is content-hashed and idempotent — caching the same body twice returns the same handle. TTL is about 1 hour. After caching: pass the handle as `degree_id` on validate_degree / audit_degree / analyze_degree / generate_degree_report / get_course_detail / render_plan_graph / find_courses_matching / degree_pipeline / compare_degrees." + description = "Cache an inline degree YAML body in the server and return a handle (\"cache:{hex}\") that any other tool accepts as a `degree_id`. Removes the per-call repaste tax for hosted MCP clients whose filesystem the server can't see (yaml_path returns ENOENT in that setup). Handle is content-hashed and idempotent — caching the same body twice returns the same handle. TTL is 24 hours (handles live only in this server process — a restart invalidates them). After caching: pass the handle as `degree_id` on validate_degree / audit_degree / analyze_degree / generate_degree_report / get_course_detail / render_plan_graph / find_courses_matching / degree_pipeline / compare_degrees." )] #[allow(clippy::unused_self)] fn cache_yaml(&self, Parameters(req): Parameters) -> String { @@ -592,19 +592,27 @@ impl NuAnalyticsMcpServer { id: &str, ) -> Result<(String, Option), String> { if id.starts_with(crate::mcp::cache::YAML_CACHE_PREFIX) { - let cache = crate::mcp::cache::YAML_CACHE + use crate::mcp::cache::CacheLookup; + // Resolve under the lock, then drop the guard before returning so we + // don't hold the cache mutex across the (owned) result construction. + let lookup = crate::mcp::cache::YAML_CACHE .lock() - .expect("yaml cache mutex poisoned"); - return cache.get(id).map_or_else( - || { - Err(serde_json::json!({ - "error": "Unknown or expired YAML cache handle", - "degree_id": id, - "hint": "Call cache_yaml(yaml_content=...) to mint a fresh handle. Cache TTL is 24 hours.", - }) - .to_string()) - }, - |(arc, remaining)| { + .expect("yaml cache mutex poisoned") + .lookup(id); + return match lookup { + CacheLookup::Expired => Err(serde_json::json!({ + "error": "YAML cache handle expired", + "degree_id": id, + "hint": "This handle was valid but aged out (TTL is 24 hours). Call cache_yaml(yaml_content=...) to mint a fresh one.", + }) + .to_string()), + CacheLookup::Unknown => Err(serde_json::json!({ + "error": "Unknown YAML cache handle", + "degree_id": id, + "hint": "No such handle was minted in this server process (a typo, or the server was restarted). Call cache_yaml(yaml_content=...) to mint one.", + }) + .to_string()), + CacheLookup::Hit(arc, remaining) => { Ok(( (*arc).to_string(), Some(CacheMeta { @@ -612,8 +620,8 @@ impl NuAnalyticsMcpServer { ttl_remaining_seconds: remaining.as_secs(), }), )) - }, - ); + } + }; } if let Some(yaml) = crate::mcp::tools::samples::yaml_for_key(id) { diff --git a/src/mcp/tools/analyze.rs b/src/mcp/tools/analyze.rs index 7cd6268..fba9753 100644 --- a/src/mcp/tools/analyze.rs +++ b/src/mcp/tools/analyze.rs @@ -345,6 +345,15 @@ pub struct AnalysisResponse { /// Excludes YAML parse / graph build / per-course-metrics shaping — /// the cost callers actually care about for budgeting future runs. pub time_elapsed_ms: u64, + + /// Machine-readable companion to the truncation follow-up: the `max_plans` + /// value worth using next. `None` when the run already covered the full + /// population (nothing to widen). Equals the current cap when complexity is + /// CV-stable (widening won't change the conclusions) and the + /// doubled-and-capped value otherwise — so agentic callers can adapt + /// without parsing the follow-up prose. + #[serde(skip_serializing_if = "Option::is_none")] + pub recommended_max_plans: Option, } // ============================================================================ @@ -634,6 +643,7 @@ fn parse_error_response(error: &str) -> AnalysisResponse { notes: vec![], time_limit_reached: false, time_elapsed_ms: 0, + recommended_max_plans: None, } } @@ -707,6 +717,25 @@ fn run_plan_analysis( (plans_processed, time_limit_reached) } +/// Free-form notes the analyze pass produced as side effects (duplicate +/// calc-ready suppression, clock-truncation). Empty when nothing notable +/// happened. +fn build_response_notes(artifacts: &AnalysisArtifacts) -> Vec { + let mut notes = Vec::new(); + if artifacts.selected.calc_ready_suppressed { + notes.push( + "calc-ready-shortest suppressed as structural duplicate of shortest-path".to_string(), + ); + } + if artifacts.time_limit_reached { + notes.push(format!( + "plan-generation loop stopped early at {} plans after {} ms — analysis_timeout_seconds tripped", + artifacts.plans_processed, artifacts.time_elapsed_ms, + )); + } + notes +} + /// Build the analysis response from a populated [`AnalysisArtifacts`] bundle. fn build_response( artifacts: &AnalysisArtifacts, @@ -788,18 +817,7 @@ fn build_response( } else { "random_uniform" }; - let mut notes = Vec::new(); - if artifacts.selected.calc_ready_suppressed { - notes.push( - "calc-ready-shortest suppressed as structural duplicate of shortest-path".to_string(), - ); - } - if artifacts.time_limit_reached { - notes.push(format!( - "plan-generation loop stopped early at {} plans after {} ms — analysis_timeout_seconds tripped", - artifacts.plans_processed, artifacts.time_elapsed_ms, - )); - } + let notes = build_response_notes(artifacts); AnalysisResponse { success: true, @@ -814,6 +832,11 @@ fn build_response( is_full_population, sampling_method, seed_used: artifacts.seed_used, + recommended_max_plans: recommend_max_plans( + artifacts, + Some(&complexity_stats), + was_truncated, + ), complexity: Some(complexity_stats), longest_delay: Some(metric_stats_json(°ree_stats.longest_delay)), total_credits: Some(metric_stats_json(°ree_stats.total_credits)), @@ -876,6 +899,44 @@ fn is_placeholder_course(id: &str) -> bool { /// tighten when callers report still seeing meaningful shifts above it. const STABLE_CV_THRESHOLD: f64 = 0.10; +/// Coefficient of variation of the complexity distribution, when computable +/// (`std_dev / |mean|`). `None` when there are no stats or the mean is ~0. +fn complexity_cv(complexity: Option<&MetricStatsJson>) -> Option { + complexity + .filter(|s| s.mean.abs() > f64::EPSILON) + .map(|s| s.std_dev / s.mean.abs()) +} + +/// The next `max_plans` worth trying to widen a truncated sample: double the +/// current cap, capped at the population estimate so we never recommend more +/// plans than exist. `saturating_mul` guards usize overflow; `.max(+1)` covers +/// the corner where doubling saturates back to the same value. +fn next_max_plans(artifacts: &AnalysisArtifacts) -> usize { + let doubled = artifacts + .max_plans + .saturating_mul(2) + .max(artifacts.max_plans + 1); + doubled.min(artifacts.stats.total_possible.max(artifacts.max_plans)) +} + +/// Machine-readable `max_plans` recommendation, mirroring the truncation +/// follow-up's logic: `None` for a full-population run (nothing to widen); the +/// current cap when complexity is CV-stable (widening won't move the +/// conclusions); otherwise the doubled-and-capped `next_max_plans`. +fn recommend_max_plans( + artifacts: &AnalysisArtifacts, + complexity: Option<&MetricStatsJson>, + was_truncated: bool, +) -> Option { + if !was_truncated { + return None; + } + match complexity_cv(complexity) { + Some(cv) if cv < STABLE_CV_THRESHOLD => Some(artifacts.max_plans), + _ => Some(next_max_plans(artifacts)), + } +} + /// Build follow-up suggestions for an analyze response. Triggered on three /// signals: truncated sample (rerun with higher cap when variance is still /// material), tiny full population (cheap to audit deeply), or long critical @@ -893,9 +954,7 @@ fn build_analysis_followups( // Coefficient of variation lets us decide whether bumping the cap // is worth the budget. A small CV (<10 %) means the medians have // stabilised — rerunning at 2× burns context for marginal change. - let cv = complexity - .filter(|s| s.mean.abs() > f64::EPSILON) - .map(|s| s.std_dev / s.mean.abs()); + let cv = complexity_cv(complexity); if let Some(cv) = cv { if cv < STABLE_CV_THRESHOLD { @@ -913,16 +972,8 @@ fn build_analysis_followups( } } - // Otherwise: suggest doubling, but capped at population_size so - // we never recommend a value larger than what exists. - // `saturating_mul(2)` guards against usize overflow on absurdly - // large caps; the `.max(+1)` guard catches the corner where - // doubling saturates back to the same value. - let doubled = artifacts - .max_plans - .saturating_mul(2) - .max(artifacts.max_plans + 1); - let next = doubled.min(artifacts.stats.total_possible.max(artifacts.max_plans)); + // Otherwise: suggest doubling, capped at the population estimate. + let next = next_max_plans(artifacts); let cv_note = cv.map_or_else(String::new, |cv| format!(" (CV={cv:.2})")); followups.push(ToolFollowup { tool: TOOL_ANALYZE_DEGREE, @@ -1364,6 +1415,84 @@ courses: assert!(!response.selected_plans.is_empty()); } + /// `recommended_max_plans` is the machine-readable companion to the + /// truncation follow-up: `None` for a full-population run, `Some` when the + /// run was capped. A select-1-of-4 degree has a 4-plan population. + #[test] + fn test_recommended_max_plans_tracks_truncation() { + const SELECT_YAML: &str = r#" +degree: + id: rec-max-plans + institution: Test University + program: Test Program + total_credits: 4 + gpa_minimum: 2.0 + +requirements: + pick: + name: Pick One + type: select + category: major + from: + courses: + - CS101 + - CS102 + - CS103 + - CS104 + count: 1 + +courses: + CS101: {title: A, prefix: CS, number: "101", credits: 4} + CS102: {title: B, prefix: CS, number: "102", credits: 4} + CS103: {title: C, prefix: CS, number: "103", credits: 4} + CS104: {title: D, prefix: CS, number: "104", credits: 4} +"#; + + // Capped below the population → truncated → a concrete recommendation. + let capped = execute( + SELECT_YAML, + Some(1), + None, + false, + None, + false, + false, + None, + None, + ); + assert!(capped.success, "error: {:?}", capped.error); + assert!( + capped.was_truncated, + "max_plans=1 vs a 4-plan population must truncate" + ); + let rec = capped + .recommended_max_plans + .expect("a truncated run must recommend a max_plans"); + assert!(rec >= 1, "recommendation must be a positive cap, got {rec}"); + + // Full population → nothing to widen → no recommendation. + let full = execute( + SELECT_YAML, + Some(50), + None, + false, + None, + false, + false, + None, + None, + ); + assert!(full.success, "error: {:?}", full.error); + assert!( + !full.was_truncated, + "max_plans=50 covers the 4-plan population" + ); + assert_eq!( + full.recommended_max_plans, None, + "a full-population run must not recommend widening" + ); + } + #[test] fn test_build_artifacts_populates_pipeline_outputs() { // Direct coverage of the shared pipeline entry point. Every artifact diff --git a/src/mcp/tools/shared.rs b/src/mcp/tools/shared.rs index de2a179..ff35f0d 100644 --- a/src/mcp/tools/shared.rs +++ b/src/mcp/tools/shared.rs @@ -114,6 +114,15 @@ pub fn parse_yaml_source( )); } if let Some(c) = yaml_content { + // A leading `@` is never valid degree YAML/JSON (it's a reserved YAML + // indicator) and almost always means the caller meant an at-path + // reference. Fail fast with a directive error rather than handing it to + // the parser, which previously stalled the whole tool call. + if c.trim_start().starts_with('@') { + return Err(error_json( + "yaml_content must be inline YAML/JSON, not a path reference (it starts with '@'). Use yaml_path for a file on the server, or degree_id for a cache: / stored program.", + )); + } return Ok(YamlSource::Content(c)); } if let Some(p) = yaml_path { @@ -414,6 +423,28 @@ mod tests { assert_eq!(out, r#"{"error":"something went wrong"}"#); } + #[test] + fn test_parse_yaml_source_rejects_at_prefixed_content() { + // A leading `@` means the caller mistook yaml_content for an at-path + // reference — fail fast with a directive error instead of stalling the + // YAML parser (the field-report hang). + let err = parse_yaml_source(Some("@/path/to/degree.yaml".to_string()), None, None) + .expect_err("@-prefixed yaml_content must be rejected"); + assert!( + err.contains("yaml_path") && err.contains('@'), + "error must redirect to yaml_path and name the '@': {err}" + ); + // Leading whitespace before the `@` is still caught. + assert!(parse_yaml_source(Some(" @foo".to_string()), None, None).is_err()); + } + + #[test] + fn test_parse_yaml_source_accepts_normal_content() { + let src = parse_yaml_source(Some("degree:\n id: x\n".to_string()), None, None) + .expect("normal yaml_content must be accepted"); + assert!(matches!(src, YamlSource::Content(_))); + } + #[test] fn test_format_yaml_context_includes_caret_and_surrounding_lines() { let yaml = "one\ntwo\nthree\nfour\nfive\nsix\nseven\n";