huskies: merge 1011

server/src/http/mcp/shell_tools/script.rs

@@ -477,13 +477,15 @@ pub(crate) async fn tool_run_lint(args: &Value, ctx: &AppContext) -> Result<Stri
 
 // ── run_check ────────────────────────────────────────────────────────────────
 
-/// Fast compile-only check (`script/check`).
+/// Fast quality gate (`script/check`).
 ///
-/// Runs `script/check` (expected to be `cargo check --tests --workspace`) in
-/// the agent's worktree and returns the **full, untruncated** stdout + stderr
-/// so that every compiler diagnostic is visible to the caller. Unlike
-/// `run_build` and `run_lint`, output is never truncated — compile errors must
-/// be readable in their entirety for fast iteration feedback.
+/// Delegates to `script/check` verbatim — whatever checks that script runs
+/// (fmt, clippy, doc-coverage, etc.) are what this tool runs. Cargo
+/// diagnostics are returned in structured `errors`/`warnings` arrays; the raw
+/// `output` field (truncated to the last `MAX_OUTPUT_LINES` lines) is always
+/// included so that non-cargo failures (e.g. `source-map-check`) surface even
+/// when they produce no cargo-format diagnostics. Pass `verbose: true` for the
+/// full untruncated output.
 pub(crate) async fn tool_run_check(args: &Value, ctx: &AppContext) -> Result<String, String> {
     let project_root = ctx.services.agents.get_project_root(&ctx.state)?;
 
@@ -534,11 +536,14 @@ pub(crate) async fn tool_run_check(args: &Value, ctx: &AppContext) -> Result<Str
     .map_err(|e| format!("Serialization error: {e}"))
 }
 
-/// Shared response builder for tools that wrap cargo / rustc output. By
-/// default returns parsed `errors` + `warnings` arrays plus a one-line
-/// summary; the raw `output` is only included when `verbose` is true. This
-/// keeps the MCP response under the token cap for runs with many errors
-/// (bug 886).
+/// Shared response builder for tools that wrap cargo / rustc output.
+///
+/// Returns parsed `errors` + `warnings` arrays (cargo format) plus a one-line
+/// summary. The raw `output` is always included: truncated to the last
+/// `MAX_OUTPUT_LINES` lines by default (keeps the token budget sane for large
+/// cargo runs — bug 886), or full and untruncated when `verbose` is true. The
+/// always-present `output` field means non-cargo failures (e.g.
+/// `source-map-check`) surface even when they produce no parseable diagnostics.
 fn build_diagnostic_response(
     passed: bool,
     exit_code: i32,
@@ -550,7 +555,12 @@ fn build_diagnostic_response(
     let summary = summarise(&diags);
     let errors: Vec<&_> = diags.iter().filter(|d| d.kind == "error").collect();
     let warnings: Vec<&_> = diags.iter().filter(|d| d.kind == "warning").collect();
-    let mut payload = json!({
+    let output = if verbose {
+        raw_output.to_string()
+    } else {
+        truncate_output(raw_output, MAX_OUTPUT_LINES)
+    };
+    json!({
         "passed": passed,
         "exit_code": exit_code,
         "errors": errors,
@@ -559,11 +569,8 @@ fn build_diagnostic_response(
             "{} error(s), {} warning(s)",
             summary.error_count, summary.warning_count
         ),
-    });
-    if verbose {
-        payload["output"] = json!(raw_output);
-    }
-    payload
+        "output": output,
+    })
 }
 
 #[cfg(test)]
@@ -815,10 +822,9 @@ mod tests {
 
     #[tokio::test]
     async fn tool_run_check_returns_parsed_errors_on_nonzero_exit() {
-        // Bug 886: rather than dumping the entire cargo log into `output`
-        // (which routinely exceeds the MCP token cap), tool_run_check now
-        // parses errors / warnings into structured arrays. Raw output is
-        // available behind `verbose: true`.
+        // Cargo diagnostics are parsed into structured `errors`/`warnings`
+        // arrays. The raw `output` is always present (truncated at
+        // MAX_OUTPUT_LINES in default mode, full in verbose mode).
         let tmp = tempfile::tempdir().unwrap();
         let script_dir = tmp.path().join("script");
         std::fs::create_dir_all(&script_dir).unwrap();
@@ -836,14 +842,14 @@ mod tests {
 
         let ctx = test_ctx(tmp.path());
 
-        // Default mode: no `output` field, structured `errors` array, summary.
+        // Default mode: truncated `output` is present, structured `errors` array, summary.
         let result = tool_run_check(&json!({}), &ctx).await.unwrap();
         let parsed: serde_json::Value = serde_json::from_str(&result).unwrap();
         assert_eq!(parsed["passed"], false);
         assert_eq!(parsed["exit_code"], 1);
         assert!(
-            parsed.get("output").map(|v| v.is_null()).unwrap_or(true),
-            "default mode must not include raw `output`"
+            parsed["output"].as_str().is_some(),
+            "default mode must include `output`"
         );
         let errors = parsed["errors"].as_array().expect("errors array");
         assert_eq!(errors.len(), 150, "all 150 errors should be parsed");
@@ -862,14 +868,14 @@ mod tests {
             summary.contains("150 error"),
             "summary mentions error count: {summary}"
         );
-        // Default response should be small even with 150 errors.
+        // Response should stay compact even with 150 errors + truncated output.
         assert!(
             result.len() < 50_000,
-            "default response should be compact (was {} bytes)",
+            "response should be compact (was {} bytes)",
             result.len()
         );
 
-        // Verbose mode: raw output is included.
+        // Verbose mode: full untruncated output.
         let result_v = tool_run_check(&json!({"verbose": true}), &ctx)
             .await
             .unwrap();
@@ -881,6 +887,56 @@ mod tests {
         assert!(output.contains("error[E150]"), "verbose contains last line");
     }
 
+    #[tokio::test]
+    async fn tool_run_check_surfaces_doc_coverage_error() {
+        // Parity test: when script/check fails because source-map-check finds
+        // missing doc comments, the failure message must be visible in the
+        // response even though it produces no cargo-format diagnostics.
+        let tmp = tempfile::tempdir().unwrap();
+        let script_dir = tmp.path().join("script");
+        std::fs::create_dir_all(&script_dir).unwrap();
+        let script_path = script_dir.join("check");
+        std::fs::write(
+            &script_path,
+            "#!/usr/bin/env bash\n\
+             echo '=== Checking doc coverage on changed files ==='\n\
+             echo 'Doc coverage check failed. Add doc comments to the following items before committing:' >&2\n\
+             echo '  Add a `///` doc comment above `pub fn new` in server/src/foo.rs:10' >&2\n\
+             exit 1\n",
+        )
+        .unwrap();
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+            std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o755)).unwrap();
+        }
+
+        let ctx = test_ctx(tmp.path());
+        let result = tool_run_check(&json!({}), &ctx).await.unwrap();
+        let parsed: serde_json::Value = serde_json::from_str(&result).unwrap();
+
+        assert_eq!(parsed["passed"], false, "check must report failure");
+        assert_eq!(parsed["exit_code"], 1);
+
+        // No cargo-format diagnostics — errors array is empty.
+        let errors = parsed["errors"].as_array().expect("errors array");
+        assert!(
+            errors.is_empty(),
+            "no cargo diagnostics expected for a source-map-check failure"
+        );
+
+        // But the raw failure text must surface in `output`.
+        let output = parsed["output"].as_str().expect("output must be present");
+        assert!(
+            output.contains("Doc coverage check failed"),
+            "doc-coverage failure must be visible in output: {output}"
+        );
+        assert!(
+            output.contains("pub fn new"),
+            "specific missing-doc item must be visible: {output}"
+        );
+    }
+
     #[tokio::test]
     async fn tool_run_check_passes_when_script_exits_zero() {
         let tmp = tempfile::tempdir().unwrap();