diff --git a/crates/gl/src/agent.rs b/crates/gl/src/agent.rs index d72c230..3f2becc 100644 --- a/crates/gl/src/agent.rs +++ b/crates/gl/src/agent.rs @@ -215,11 +215,14 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; let err = cmd_list(server.url(), None).await.unwrap_err(); assert!(err.to_string().contains("agents API")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -230,11 +233,14 @@ mod tests { .with_status(500) .with_header("content-type", "application/json") .with_body(r#"{"message":"internal error"}"#) + .expect(1) .create_async() .await; let err = cmd_list(server.url(), None).await.unwrap_err(); assert!(err.to_string().contains("list agents failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -261,6 +267,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"agent not found"}"#) + .expect(1) .create_async() .await; @@ -268,6 +275,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("agents API") || err.to_string().contains("not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] diff --git a/crates/gl/src/bounty.rs b/crates/gl/src/bounty.rs index ccc658a..93a99ef 100644 --- a/crates/gl/src/bounty.rs +++ b/crates/gl/src/bounty.rs @@ -231,11 +231,14 @@ async fn cmd_list( u }; - let resp = client - .get_authed(&url) - .await - .context("failed to connect to node")?; - let body: Value = resp.json().await.unwrap_or_default(); + let body = crate::http::read_json( + client + .get_authed(&url) + .await + .context("failed to connect to node")?, + "bounties", + ) + .await?; let bounties = body["bounties"].as_array(); if let Some(arr) = bounties { @@ -521,6 +524,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; @@ -528,6 +532,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -544,4 +550,30 @@ mod tests { cmd_stats(server.url()).await.unwrap(); } + + #[tokio::test] + async fn cmd_list_repo_scoped_surfaces_denial_not_empty() { + // `gl bounty list --repo owner/name` hits the gated /repos/{o}/{n}/bounties; + // a 404 must Err, not silently print nothing (INV-8). + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/repos/alice/secret/bounties".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list(Some("alice/secret".to_string()), None, server.url(), None).await; + assert!( + result.is_err(), + "bounty list --repo must Err on a gated 404" + ); + // Prove the gated repo-scoped path was actually requested: without this, + // an unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } } diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 09823d5..2914c8e 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -70,12 +70,7 @@ async fn resolve_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = signed_client(node, dir); - let info: Value = client - .get_authed("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get_authed("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing 'did'")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -88,12 +83,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let client = signed_client(&node, dir.as_deref()); let path = format!("/api/v1/repos/{owner}/{name}/certs"); - let resp: Value = client - .get_authed(&path) - .await? - .json() - .await - .context("failed to list certificates")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "certificates").await?; let certs = resp["certificates"].as_array().cloned().unwrap_or_default(); @@ -209,3 +199,52 @@ async fn resolve_cert_id(client: &NodeClient, owner: &str, name: &str, id: &str) _ => anyhow::bail!("certificate prefix {id} matches multiple certificates"), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A gated 404 on the repo-scoped certs read must Err, not print "No certificates". + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/certs$".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "cert list must Err on a gated 404"); + // Prove the gated certs path was actually requested: without this, an + // unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } + + #[tokio::test] + async fn resolve_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } +} diff --git a/crates/gl/src/changelog.rs b/crates/gl/src/changelog.rs index 8736894..d8006d8 100644 --- a/crates/gl/src/changelog.rs +++ b/crates/gl/src/changelog.rs @@ -42,12 +42,7 @@ pub async fn run(args: ChangelogArgs) -> Result<()> { did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = NodeClient::new(&args.node, None); - let info: Value = client - .get("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node missing DID")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -212,6 +207,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -223,6 +219,8 @@ mod tests { }; let err = run(args).await.unwrap_err(); assert!(err.to_string().contains("changelog failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -270,4 +268,31 @@ mod tests { let err = rt.block_on(run(args)).unwrap_err(); assert!(err.to_string().contains("no repo specified")); } + + #[tokio::test] + async fn resolve_via_run_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the inline GET / + // node-info fetch during resolution. A gated 404 there must Err (surfacing + // the status), proving the read_json conversion is load-bearing. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = run(ChangelogArgs { + repo: Some("noslash".to_string()), + limit: 20, + node: server.url(), + dir: Some(dir.path().to_path_buf()), + }) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index dc9f45d..10f6221 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -10,6 +10,7 @@ use anyhow::{Context, Result}; use gitlawb_core::http_sig::sign_request; use gitlawb_core::identity::Keypair; use icaptcha_client::IcaptchaCfg; +use serde_json::Value; /// Max times we'll fetch a fresh proof and retry a 403-iCaptcha response /// (absorbs proof expiry / first-seen replay). @@ -186,6 +187,37 @@ async fn obtain_proof(cfg: IcaptchaCfg) -> Result { .context("iCaptcha solver task panicked")? } +/// Read a JSON response, surfacing a node denial/error instead of parsing it as +/// the requested resource. On a non-2xx status it returns an `Err` carrying the +/// node's sanitized `message` (INV-6) plus the status; on success it parses the +/// body and propagates a parse error, so a truncated/garbage 2xx body is an error +/// rather than a silently-empty success (the denial-as-success bug #123 fixes). +/// `what` names the resource for the error text (e.g. "repo", "commits"). +/// +/// Callers must route gated reads through this rather than `resp.json().await?`: +/// the bare parse renders a gated 404/5xx body back as the resource (INV-8). +pub(crate) async fn read_json(resp: reqwest::Response, what: &str) -> Result { + let status = resp.status(); + if !status.is_success() { + // The error body may be non-JSON (503 degraded, 413 body-limit from + // middleware); tolerate it and fall back to the status alone. + let body: Value = resp.json().await.unwrap_or_default(); + let msg = body + .get("message") + .and_then(|m| m.as_str()) + .unwrap_or("request failed"); + anyhow::bail!( + "{what} failed ({status}): {}", + crate::sync::sanitize_node_msg(msg) + ); + } + // Success: a truncated/garbage 2xx body must be an `Err`, not `Ok(Null)` that + // a caller then renders as an empty success. + resp.json() + .await + .with_context(|| format!("invalid JSON in {what} response")) +} + #[cfg(test)] mod tests { use super::*; @@ -570,4 +602,115 @@ mod tests { ic.challenge.assert(); ic.answer.assert(); } + + // ── read_json (status-checked read; #123 / INV-8 / INV-6) ──────────── + + /// Drive a real `reqwest::Response` off a mockito mock so `read_json` sees an + /// actual HTTP status + body, the same shape the gated read arms produce. + async fn response_for( + server: &mut Server, + status: usize, + body: &str, + json: bool, + ) -> reqwest::Response { + let mut m = server.mock("GET", "/x").with_status(status).with_body(body); + if json { + m = m.with_header("content-type", "application/json"); + } + let _m = m.create_async().await; + NodeClient::new(server.url(), None).get("/x").await.unwrap() + } + + #[tokio::test] + async fn read_json_returns_body_on_2xx() { + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 200, + r#"{"name":"r","owner_did":"did:gitlawb:z"}"#, + true, + ) + .await; + let v = read_json(resp, "repo").await.unwrap(); + assert_eq!(v["name"], "r"); + } + + #[tokio::test] + async fn read_json_errs_on_404_surfacing_message_and_status() { + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"message":"repository 'o/r' not found"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("not found"), "err={err}"); + } + + #[tokio::test] + async fn read_json_errs_on_500_surfacing_message() { + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 500, r#"{"message":"internal boom"}"#, true).await; + let err = read_json(resp, "commits").await.unwrap_err().to_string(); + assert!(err.contains("500"), "err={err}"); + assert!(err.contains("internal boom"), "err={err}"); + } + + #[tokio::test] + async fn read_json_errs_on_non_json_error_body_with_fallback() { + // 503 with a plain-text (middleware) body: no `message` field to surface. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 503, "service unavailable", false).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("503"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + } + + #[tokio::test] + async fn read_json_sanitizes_control_and_bidi_in_message() { + // INV-6: a hostile node embeds ESC, BEL, and a right-to-left override in + // the error message; none may reach the terminal verbatim. + let mut server = Server::new_async().await; + let resp = response_for( + &mut server, + 404, + r#"{"message":"a\u001b[31mb\u0007c\u202ed"}"#, + true, + ) + .await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(!err.contains('\u{1b}'), "ESC leaked: {err:?}"); + assert!(!err.contains('\u{7}'), "BEL leaked: {err:?}"); + assert!(!err.contains('\u{202e}'), "bidi override leaked: {err:?}"); + } + + #[tokio::test] + async fn read_json_errs_on_garbage_2xx_body() { + // The #123 correctness point: a 200 with a non-JSON/truncated body must be + // an `Err`, NOT `Ok(Null)` that a caller renders as an empty success. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 200, "this is not json", false).await; + assert!(read_json(resp, "repo").await.is_err()); + } + + #[tokio::test] + async fn read_json_errs_on_empty_2xx_body() { + // A zero-byte 200 body must be an `Err`, not a silent `Ok(Null)`. + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 200, "", false).await; + assert!(read_json(resp, "repo").await.is_err()); + } + + #[tokio::test] + async fn read_json_errs_on_non_2xx_json_without_message() { + // A non-2xx JSON body that lacks a `message` key falls back to "request failed". + let mut server = Server::new_async().await; + let resp = response_for(&mut server, 403, r#"{"error":"forbidden"}"#, true).await; + let err = read_json(resp, "repo").await.unwrap_err().to_string(); + assert!(err.contains("403"), "err={err}"); + assert!(err.contains("request failed"), "err={err}"); + } } diff --git a/crates/gl/src/issue.rs b/crates/gl/src/issue.rs index 57bd694..e321ae9 100644 --- a/crates/gl/src/issue.rs +++ b/crates/gl/src/issue.rs @@ -142,12 +142,7 @@ async fn resolve_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = NodeClient::new(node, None); - let info: Value = client - .get("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing 'did'")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -221,12 +216,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let client = signed_client(&node, dir.as_deref()); let path = format!("/api/v1/repos/{owner}/{name}/issues"); - let resp: Value = client - .get_authed(&path) - .await? - .json() - .await - .context("failed to list issues")?; + let resp = crate::http::read_json(client.get_authed(&path).await?, "issues").await?; let issues = resp["issues"].as_array().cloned().unwrap_or_default(); @@ -353,14 +343,15 @@ async fn cmd_issue_comments( let (owner, name) = resolve_repo(&repo, &node, dir.as_deref()).await?; let client = signed_client(&node, dir.as_deref()); - let resp: Value = client - .get_authed(&format!( - "/api/v1/repos/{owner}/{name}/issues/{id}/comments" - )) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get_authed(&format!( + "/api/v1/repos/{owner}/{name}/issues/{id}/comments" + )) + .await?, + "issue comments", + ) + .await?; let comments = resp["comments"].as_array().cloned().unwrap_or_default(); if comments.is_empty() { @@ -490,6 +481,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -504,6 +496,8 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("repo not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] @@ -551,6 +545,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"issue not found"}"#) + .expect(1) .create_async() .await; @@ -564,6 +559,8 @@ mod tests { .unwrap_err(); assert!(err.to_string().contains("show failed"), "got: {err}"); assert!(err.to_string().contains("issue not found"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -612,6 +609,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"issue not found"}"#) + .expect(1) .create_async() .await; @@ -624,6 +622,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("close issue failed"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -670,6 +670,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"issue not found"}"#) + .expect(1) .create_async() .await; @@ -683,6 +684,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("issue not found"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -740,4 +743,48 @@ mod tests { .await .unwrap(); } + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A gated 404 on the issue-list read must Err, not print "No issues". + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/issues$".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "issue list must Err on a gated 404"); + // Prove the gated issues path was actually requested: without this, an + // unmatched route (mockito's 501, also non-2xx) would satisfy is_err(). + _m.assert_async().await; + } + + #[tokio::test] + async fn resolve_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index 0668ac9..6d41998 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -654,12 +654,13 @@ async fn call_tool( } "node_info" => { - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; Ok(serde_json::to_string_pretty(&info)?) } "node_health" => { - let health: Value = client.get("/health").await?.json().await?; + let health = + crate::http::read_json(client.get("/health").await?, "node health").await?; Ok(serde_json::to_string_pretty(&health)?) } @@ -670,39 +671,48 @@ async fn call_tool( "description": args["description"], "is_public": args["is_public"].as_bool().unwrap_or(true), }))?; - let resp: Value = client.post("/api/v1/repos", &body).await?.json().await?; + let resp = + crate::http::read_json(client.post("/api/v1/repos", &body).await?, "create repo") + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "repo_list" => { - let repos: Value = client.get("/api/v1/repos").await?.json().await?; + let repos = + crate::http::read_json(client.get("/api/v1/repos").await?, "list repos").await?; Ok(serde_json::to_string_pretty(&repos)?) } "repo_list_federated" => { - let result: Value = client.get("/api/v1/repos/federated").await?.json().await?; + let result = crate::http::read_json( + client.get("/api/v1/repos/federated").await?, + "list federated repos", + ) + .await?; Ok(serde_json::to_string_pretty(&result)?) } "repo_get" => { let name = args["name"].as_str().context("missing 'name'")?; let owner = resolve_owner(&args, &client).await?; - let repo: Value = client - .get(&format!("/api/v1/repos/{owner}/{name}")) - .await? - .json() - .await?; + let repo = crate::http::read_json( + client.get(&format!("/api/v1/repos/{owner}/{name}")).await?, + "repo", + ) + .await?; Ok(serde_json::to_string_pretty(&repo)?) } "repo_commits" => { let name = args["name"].as_str().context("missing 'name'")?; let owner = resolve_owner(&args, &client).await?; - let commits: Value = client - .get(&format!("/api/v1/repos/{owner}/{name}/commits")) - .await? - .json() - .await?; + let commits = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{name}/commits")) + .await?, + "commits", + ) + .await?; Ok(serde_json::to_string_pretty(&commits)?) } @@ -710,17 +720,19 @@ async fn call_tool( let name = args["name"].as_str().context("missing 'name'")?; let path = args["path"].as_str().unwrap_or(""); let owner = resolve_owner(&args, &client).await?; - let tree: Value = client - .get(&format!("/api/v1/repos/{owner}/{name}/tree/{path}")) - .await? - .json() - .await?; + let tree = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{name}/tree/{path}")) + .await?, + "tree", + ) + .await?; Ok(serde_json::to_string_pretty(&tree)?) } "repo_clone_url" => { let name = args["name"].as_str().context("missing 'name'")?; - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing DID")?; Ok(format!("gitlawb://{}/{}", did, name)) } @@ -736,7 +748,11 @@ async fn call_tool( "capabilities": caps, "model": args["model"], }))?; - let resp: Value = client.post("/api/register", &body).await?.json().await?; + let resp = crate::http::read_json( + client.post("/api/register", &body).await?, + "register agent", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -793,6 +809,10 @@ async fn call_tool( "/{owner}/{name}/info/refs?service=git-upload-pack" )) .await?; + let status = resp.status(); + if !status.is_success() { + anyhow::bail!("git refs failed ({status})"); + } let bytes = resp.bytes().await?; // Parse pkt-line refs let refs = parse_info_refs(&bytes); @@ -814,22 +834,26 @@ async fn call_tool( "source_branch": head, "target_branch": base, }))?; - let resp: Value = client - .post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &body) + .await?, + "create pull request", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "pr_list" => { let repo = args["repo"].as_str().context("missing 'repo'")?; let owner = resolve_owner(&args, &client).await?; - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) + .await?, + "pull requests", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -837,18 +861,22 @@ async fn call_tool( let repo = args["repo"].as_str().context("missing 'repo'")?; let number = args["number"].as_i64().context("missing 'number'")?; let owner = resolve_owner(&args, &client).await?; - let pr: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) - .await? - .json() - .await?; - let reviews: Value = client - .get(&format!( - "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" - )) - .await? - .json() - .await?; + let pr = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) + .await?, + "pull request", + ) + .await?; + let reviews = crate::http::read_json( + client + .get(&format!( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" + )) + .await?, + "reviews", + ) + .await?; Ok(serde_json::to_string_pretty( &json!({ "pr": pr, "reviews": reviews["reviews"] }), )?) @@ -858,11 +886,13 @@ async fn call_tool( let repo = args["repo"].as_str().context("missing 'repo'")?; let number = args["number"].as_i64().context("missing 'number'")?; let owner = resolve_owner(&args, &client).await?; - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) + .await?, + "diff", + ) + .await?; let diff = resp["diff"].as_str().unwrap_or("(empty diff)"); Ok(diff.to_string()) } @@ -879,14 +909,16 @@ async fn call_tool( "status": status, "body": args["body"], }))?; - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews"), - &body, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews"), + &body, + ) + .await?, + "submit review", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -895,14 +927,16 @@ async fn call_tool( let number = args["number"].as_i64().context("missing 'number'")?; let owner = resolve_owner(&args, &client).await?; let body = serde_json::to_vec(&json!({}))?; - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/merge"), - &body, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/merge"), + &body, + ) + .await?, + "merge pull request", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -922,22 +956,26 @@ async fn call_tool( "secret": args["secret"], "events": events, }))?; - let resp: Value = client - .post(&format!("/api/v1/repos/{owner}/{repo}/hooks"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{repo}/hooks"), &body) + .await?, + "create webhook", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "webhook_list" => { let repo = args["repo"].as_str().context("missing 'repo'")?; let owner = resolve_owner(&args, &client).await?; - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/hooks")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/hooks")) + .await?, + "webhooks", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -946,11 +984,13 @@ async fn call_tool( let id = args["id"].as_str().context("missing 'id'")?; let owner = resolve_owner(&args, &client).await?; let body = serde_json::to_vec(&json!({}))?; - let resp: Value = client - .delete(&format!("/api/v1/repos/{owner}/{repo}/hooks/{id}"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .delete(&format!("/api/v1/repos/{owner}/{repo}/hooks/{id}"), &body) + .await?, + "delete webhook", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -970,17 +1010,17 @@ async fn call_tool( } u }; - let resp: Value = client.get_authed(&url).await?.json().await?; + let resp = crate::http::read_json(client.get_authed(&url).await?, "bounties").await?; Ok(serde_json::to_string_pretty(&resp)?) } "bounty_show" => { let id = args["id"].as_str().context("missing 'id'")?; - let resp: Value = client - .get_authed(&format!("/api/v1/bounties/{id}")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client.get_authed(&format!("/api/v1/bounties/{id}")).await?, + "bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -993,28 +1033,32 @@ async fn call_tool( "issue_id": args.get("issue_id").and_then(|v| v.as_str()), "tx_hash": args.get("tx_hash").and_then(|v| v.as_str()), }); - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{name}/bounties"), - &serde_json::to_vec(&body)?, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{name}/bounties"), + &serde_json::to_vec(&body)?, + ) + .await?, + "create bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "bounty_claim" => { let id = args["id"].as_str().context("missing 'id'")?; let body = json!({ "wallet": args.get("wallet").and_then(|v| v.as_str()) }); - let resp: Value = client - .post( - &format!("/api/v1/bounties/{id}/claim"), - &serde_json::to_vec(&body)?, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/bounties/{id}/claim"), + &serde_json::to_vec(&body)?, + ) + .await?, + "claim bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1022,19 +1066,23 @@ async fn call_tool( let id = args["id"].as_str().context("missing 'id'")?; let pr_id = args["pr_id"].as_str().context("missing 'pr_id'")?; let body = json!({ "pr_id": pr_id }); - let resp: Value = client - .post( - &format!("/api/v1/bounties/{id}/submit"), - &serde_json::to_vec(&body)?, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/bounties/{id}/submit"), + &serde_json::to_vec(&body)?, + ) + .await?, + "submit bounty", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } "bounty_stats" => { - let resp: Value = client.get("/api/v1/bounties/stats").await?.json().await?; + let resp = + crate::http::read_json(client.get("/api/v1/bounties/stats").await?, "bounty stats") + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1048,7 +1096,7 @@ async fn call_tool( if let Some(a) = args.get("assignee_did").and_then(|v| v.as_str()) { path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); } - let resp: Value = client.get(&path).await?.json().await?; + let resp = crate::http::read_json(client.get(&path).await?, "list tasks").await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1065,7 +1113,9 @@ async fn call_tool( "deadline": args.get("deadline").and_then(|v| v.as_str()), "delegator_did": delegator_did, }))?; - let resp: Value = client.post("/api/v1/tasks", &body).await?.json().await?; + let resp = + crate::http::read_json(client.post("/api/v1/tasks", &body).await?, "create task") + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1074,11 +1124,13 @@ async fn call_tool( let assignee_did = kp.did().to_string(); let id = args["id"].as_str().context("missing 'id'")?; let body = serde_json::to_vec(&json!({ "assignee_did": assignee_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{id}/claim"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{id}/claim"), &body) + .await?, + "claim task", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1090,11 +1142,13 @@ async fn call_tool( "result": args.get("result").and_then(|v| v.as_str()), "by_did": by_did, }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{id}/complete"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{id}/complete"), &body) + .await?, + "complete task", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1164,11 +1218,13 @@ async fn call_tool( let owner = resolve_owner(&args, &client).await?; (owner, repo.to_string()) }; - let resp: Value = client - .get_authed(&format!("/api/v1/repos/{owner}/{name}/issues")) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .get_authed(&format!("/api/v1/repos/{owner}/{name}/issues")) + .await?, + "issues", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1188,11 +1244,13 @@ async fn call_tool( "title": title, "body": args.get("body").and_then(|v| v.as_str()), }))?; - let resp: Value = client - .post(&format!("/api/v1/repos/{owner}/{name}/issues"), &body) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/repos/{owner}/{name}/issues"), &body) + .await?, + "create issue", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1210,14 +1268,16 @@ async fn call_tool( (owner, repo.to_string()) }; let body = serde_json::to_vec(&json!({ "body": comment_body }))?; - let resp: Value = client - .post( - &format!("/api/v1/repos/{owner}/{name}/issues/{issue_id}/comments"), - &body, - ) - .await? - .json() - .await?; + let resp = crate::http::read_json( + client + .post( + &format!("/api/v1/repos/{owner}/{name}/issues/{issue_id}/comments"), + &body, + ) + .await?, + "comment on issue", + ) + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1232,7 +1292,7 @@ async fn resolve_owner(args: &Value, client: &NodeClient) -> Result { if let Some(o) = args.get("owner").and_then(|v| v.as_str()) { return Ok(o.to_string()); } - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node info missing DID")?; Ok(did.split(':').next_back().unwrap_or(did).to_string()) } @@ -1831,4 +1891,580 @@ mod tests { let count = tools.as_array().unwrap().len(); assert_eq!(count, 40, "expected 40 tools, got {count}"); } + + // ── Gated read arms surface node denials, not fabricated results (#123 / INV-8) ── + + #[tokio::test] + async fn repo_get_surfaces_denial_not_fabricated_repo() { + // The node returns the opaque 404 for a private repo the caller cannot read. + // The tool must Err surfacing it, NOT Ok with the error body serialized as the repo. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + + let result = call_tool( + "repo_get", + json!({"owner": "alice", "name": "secret"}), + &server.url(), + None, + ) + .await; + + let err = result + .expect_err("repo_get must Err on a 404, not fabricate a repo") + .to_string(); + assert!(err.contains("404"), "err={err}"); + assert!(err.contains("not found"), "err={err}"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn repo_get_returns_repo_on_200() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/myrepo") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"name":"myrepo","owner_did":"did:gitlawb:alice"}"#) + .create_async() + .await; + let result = call_tool( + "repo_get", + json!({"owner": "alice", "name": "myrepo"}), + &server.url(), + None, + ) + .await + .unwrap(); + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["name"], "myrepo"); + } + + #[tokio::test] + async fn repo_commits_surfaces_denial_not_empty_list() { + // Before the fix a 404 rendered as "no commits" (empty). Now it must Err. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/commits") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "repo_commits", + json!({"owner": "alice", "name": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "repo_commits must Err on 404"); + assert!(result.unwrap_err().to_string().contains("not found")); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn repo_tree_surfaces_denial_not_fabricated_tree() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/tree/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "repo_tree", + json!({"owner": "alice", "name": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "repo_tree must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn pr_list_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/pulls") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "pr_list", + json!({"owner": "alice", "repo": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "pr_list must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn webhook_list_surfaces_denial_not_hook_targets() { + // Client half of #94: a non-owner hooks read must surface the denial, + // never render the webhook target URLs as a result. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/hooks") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "webhook_list", + json!({"owner": "alice", "repo": "secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "webhook_list must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn issue_list_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/issues") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "issue_list", + json!({"repo": "alice/secret"}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "issue_list must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn pr_view_surfaces_denial_not_stub() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/pulls/1") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "pr_view", + json!({"owner": "alice", "repo": "secret", "number": 1}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "pr_view must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn pr_diff_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/repos/alice/secret/pulls/1/diff") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = call_tool( + "pr_diff", + json!({"owner": "alice", "repo": "secret", "number": 1}), + &server.url(), + None, + ) + .await; + assert!(result.is_err(), "pr_diff must Err on 404"); + // Prove the gated route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + // ── INV-8 across the MCP twin: every tool that renders a node response must + // surface a denial as an Err, not print the 4xx/5xx body as a success. ── + + /// Drive one MCP tool against a node returning a 404 and assert it Errs + /// (surfacing the denial) rather than returning the error body as a result. + /// `.expect(1)` proves the tool hit the intended endpoint, so a wrong-path + /// request (mockito's 501) can't satisfy the Err assertion vacuously. + async fn assert_tool_surfaces_denial( + tool: &str, + method: &str, + path: mockito::Matcher, + args: Value, + with_identity: bool, + ) { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + if with_identity { + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + } + let _m = server + .mock(method, path) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let dir_opt = with_identity.then(|| dir.path()); + let err = call_tool(tool, args, &server.url(), dir_opt) + .await + .unwrap_err(); + assert!( + err.to_string().contains("404"), + "{tool} must surface the 404 denial as an Err, got: {err}" + ); + _m.assert_async().await; + } + + #[tokio::test] + async fn node_info_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "node_info", + "GET", + mockito::Matcher::Regex(r"^/$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn node_health_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "node_health", + "GET", + mockito::Matcher::Regex(r"^/health$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "repo_create", + "POST", + mockito::Matcher::Regex(r"^/api/v1/repos$".to_string()), + json!({"name": "r"}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_list_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "repo_list", + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_list_federated_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "repo_list_federated", + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/federated$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn agent_register_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "agent_register", + "POST", + mockito::Matcher::Regex(r"^/api/register$".to_string()), + json!({}), + true, + ) + .await; + } + + #[tokio::test] + async fn pr_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "pr_create", + "POST", + mockito::Matcher::Regex(r"/pulls$".to_string()), + json!({"owner": "alice", "repo": "r", "head": "h", "title": "t"}), + true, + ) + .await; + } + + #[tokio::test] + async fn pr_review_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "pr_review", + "POST", + mockito::Matcher::Regex(r"/pulls/1/reviews$".to_string()), + json!({"owner": "alice", "repo": "r", "number": 1, "status": "approve"}), + true, + ) + .await; + } + + #[tokio::test] + async fn pr_merge_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "pr_merge", + "POST", + mockito::Matcher::Regex(r"/pulls/1/merge$".to_string()), + json!({"owner": "alice", "repo": "r", "number": 1}), + false, + ) + .await; + } + + #[tokio::test] + async fn webhook_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "webhook_create", + "POST", + mockito::Matcher::Regex(r"/hooks$".to_string()), + json!({"owner": "alice", "repo": "r", "url": "http://example.com"}), + true, + ) + .await; + } + + #[tokio::test] + async fn webhook_delete_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "webhook_delete", + "DELETE", + mockito::Matcher::Regex(r"/hooks/h1$".to_string()), + json!({"owner": "alice", "repo": "r", "id": "h1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_show_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_show", + "GET", + mockito::Matcher::Regex(r"/api/v1/bounties/b1$".to_string()), + json!({"id": "b1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_create", + "POST", + mockito::Matcher::Regex(r"/bounties$".to_string()), + json!({"repo": "a/b", "title": "t", "amount": 100}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_claim_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_claim", + "POST", + mockito::Matcher::Regex(r"/api/v1/bounties/b1/claim$".to_string()), + json!({"id": "b1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_submit_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_submit", + "POST", + mockito::Matcher::Regex(r"/api/v1/bounties/b1/submit$".to_string()), + json!({"id": "b1", "pr_id": "p1"}), + false, + ) + .await; + } + + #[tokio::test] + async fn bounty_stats_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "bounty_stats", + "GET", + mockito::Matcher::Regex(r"^/api/v1/bounties/stats$".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn task_list_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_list", + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + json!({}), + false, + ) + .await; + } + + #[tokio::test] + async fn task_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_create", + "POST", + mockito::Matcher::Regex(r"^/api/v1/tasks$".to_string()), + json!({"kind": "code-review"}), + true, + ) + .await; + } + + #[tokio::test] + async fn task_claim_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_claim", + "POST", + mockito::Matcher::Regex(r"/api/v1/tasks/t1/claim$".to_string()), + json!({"id": "t1"}), + true, + ) + .await; + } + + #[tokio::test] + async fn task_complete_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "task_complete", + "POST", + mockito::Matcher::Regex(r"/api/v1/tasks/t1/complete$".to_string()), + json!({"id": "t1"}), + true, + ) + .await; + } + + #[tokio::test] + async fn issue_create_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "issue_create", + "POST", + mockito::Matcher::Regex(r"/issues$".to_string()), + json!({"owner": "alice", "repo": "r", "title": "t"}), + true, + ) + .await; + } + + #[tokio::test] + async fn issue_comment_via_mcp_surfaces_denial() { + assert_tool_surfaces_denial( + "issue_comment", + "POST", + mockito::Matcher::Regex(r"/issues/i1/comments$".to_string()), + json!({"owner": "alice", "repo": "r", "issue_id": "i1", "body": "b"}), + true, + ) + .await; + } + + #[tokio::test] + async fn git_refs_via_mcp_surfaces_denial() { + // git_refs reads pkt-line bytes, not JSON, so a 404 body would otherwise + // parse to an empty ref list and render as a successful (empty) result. + assert_tool_surfaces_denial( + "git_refs", + "GET", + mockito::Matcher::Regex(r"/info/refs".to_string()), + json!({"owner": "alice", "name": "r"}), + false, + ) + .await; + } + + #[tokio::test] + async fn repo_clone_url_via_mcp_surfaces_denial() { + // repo_clone_url resolves the node DID via GET / (read_json). A gated 404 + // there must Err (surfacing the status), not fabricate a clone URL. + assert_tool_surfaces_denial( + "repo_clone_url", + "GET", + mockito::Matcher::Regex(r"^/$".to_string()), + json!({"name": "r"}), + false, + ) + .await; + } + + #[tokio::test] + async fn mcp_resolve_owner_surfaces_denial() { + // With no "owner" arg, resolve_owner GETs / for the node DID via read_json. + // A gated 404 there must Err (surfacing the status), proving the conversion + // is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_owner(&json!({}), &NodeClient::new(server.url(), None)) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/peer.rs b/crates/gl/src/peer.rs index e966364..2372736 100644 --- a/crates/gl/src/peer.rs +++ b/crates/gl/src/peer.rs @@ -63,12 +63,7 @@ pub async fn run(args: PeerArgs) -> Result<()> { async fn cmd_list(node: String) -> Result<()> { let client = NodeClient::new(&node, None); - let resp: Value = client - .get("/api/v1/peers") - .await? - .json() - .await - .context("failed to list peers")?; + let resp = crate::http::read_json(client.get("/api/v1/peers").await?, "peers").await?; let peers = resp["peers"].as_array().cloned().unwrap_or_default(); let count = resp["count"].as_u64().unwrap_or(peers.len() as u64); @@ -161,12 +156,7 @@ async fn cmd_add(peer_url: String, node: String, dir: Option) -> Result async fn cmd_ping(did: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); let path = format!("/api/v1/peers/{did}/ping"); - let resp: Value = client - .get(&path) - .await? - .json() - .await - .context("failed to ping peer")?; + let resp = crate::http::read_json(client.get(&path).await?, "ping peer").await?; let url = resp["http_url"].as_str().unwrap_or("?"); let reachable = resp["reachable"].as_bool().unwrap_or(false); @@ -186,12 +176,7 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); let encoded = urlencoding::encode(&did); let path = format!("/api/v1/resolve/{encoded}"); - let resp: Value = client - .get(&path) - .await? - .json() - .await - .context("failed to resolve DID")?; + let resp = crate::http::read_json(client.get(&path).await?, "resolve DID").await?; let source = resp["source"].as_str().unwrap_or("not found"); let http_url = resp["http_url"].as_str().unwrap_or("(none)"); @@ -210,3 +195,66 @@ async fn cmd_resolve(did: String, node: String) -> Result<()> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn cmd_list_surfaces_denial_not_empty() { + // A node error must Err, not print "No known peers" as if the list were empty. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/peers") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_list(server.url()).await.unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn cmd_ping_surfaces_denial_not_unreachable() { + // A node error must Err, not print a fabricated "unreachable" peer. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/api/v1/peers/peer1/ping") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"not found"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_ping("peer1".to_string(), server.url()) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn cmd_resolve_surfaces_denial_not_notfound() { + // A node error must Err, not print "Source: not found" as if resolved-absent. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/resolve/".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let err = cmd_resolve("peer1".to_string(), server.url()) + .await + .unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } +} diff --git a/crates/gl/src/pr.rs b/crates/gl/src/pr.rs index 8e0c4b7..c75864f 100644 --- a/crates/gl/src/pr.rs +++ b/crates/gl/src/pr.rs @@ -254,12 +254,13 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let owner = resolve_owner(&keypair); let client = NodeClient::new(&node, None); - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls")) + .await?, + "pull requests", + ) + .await?; let prs = resp["pulls"].as_array().cloned().unwrap_or_default(); if prs.is_empty() { @@ -298,12 +299,13 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) let owner = resolve_owner(&keypair); let client = NodeClient::new(&node, None); - let pr: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) - .await? - .json() - .await - .context("invalid JSON")?; + let pr = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}")) + .await?, + "pull request", + ) + .await?; let title = pr["title"].as_str().unwrap_or("?"); let status = pr["status"].as_str().unwrap_or("?"); @@ -321,14 +323,15 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) } // Show reviews - let reviews: Value = client - .get(&format!( - "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" - )) - .await? - .json() - .await - .context("invalid JSON")?; + let reviews = crate::http::read_json( + client + .get(&format!( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/reviews" + )) + .await?, + "reviews", + ) + .await?; let reviews = reviews["reviews"].as_array().cloned().unwrap_or_default(); if !reviews.is_empty() { println!("\nReviews ({}):", reviews.len()); @@ -354,14 +357,15 @@ async fn cmd_view(repo: String, number: u64, node: String, dir: Option) } // Show comments - let comments: Value = client - .get(&format!( - "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" - )) - .await? - .json() - .await - .context("invalid JSON")?; + let comments = crate::http::read_json( + client + .get(&format!( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/comments" + )) + .await?, + "comments", + ) + .await?; let comments = comments["comments"].as_array().cloned().unwrap_or_default(); if !comments.is_empty() { println!("\nComments ({}):", comments.len()); @@ -386,12 +390,13 @@ async fn cmd_diff(repo: String, number: u64, node: String, dir: Option) let owner = resolve_owner(&keypair); let client = NodeClient::new(&node, None); - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/pulls/{number}/diff")) + .await?, + "diff", + ) + .await?; let diff = resp["diff"].as_str().unwrap_or(""); if diff.is_empty() { @@ -506,14 +511,15 @@ async fn cmd_comments(repo: String, number: u64, node: String, dir: Option) -> Result<()> { let client = NodeClient::new(&node, None); let url = format!("/api/v1/repos?owner={owner}"); - let repos: Value = client - .get(&url) - .await? - .json() - .await - .context("failed to list repos")?; + let repos = crate::http::read_json(client.get(&url).await?, "list repos").await?; let repos = repos.as_array().context("expected array")?; if repos.is_empty() { @@ -354,7 +349,11 @@ async fn cmd_info(repo: String, node: String, dir: Option) -> Result<() .ok() .and_then(|v| v["message"].as_str().map(String::from)) .unwrap_or_else(|| "request failed".to_string()); - anyhow::bail!("repo info failed ({status}): {msg}"); + // Sanitize the node-advertised message before it reaches the terminal (INV-6). + anyhow::bail!( + "repo info failed ({status}): {}", + crate::sync::sanitize_node_msg(&msg) + ); } let r: Value = resp.json().await.context("parse repo info")?; @@ -511,12 +510,7 @@ pub(crate) async fn cmd_commits( }; let url = format!("/api/v1/repos/{owner}/{name}/commits?branch={branch}&limit={limit}"); - let resp: Value = client - .get(&url) - .await? - .json() - .await - .context("failed to fetch commits")?; + let resp = crate::http::read_json(client.get(&url).await?, "commits").await?; let commits = resp["commits"].as_array().cloned().unwrap_or_default(); if commits.is_empty() { @@ -660,12 +654,13 @@ async fn cmd_label_list(repo: String, node: String, dir: Option) -> Res let (owner, name) = resolve_owner_repo_pair(&repo, &node, dir.as_deref()).await?; let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); - let resp: Value = client - .get_authed(&format!("/api/v1/repos/{owner}/{name}/labels")) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get_authed(&format!("/api/v1/repos/{owner}/{name}/labels")) + .await?, + "labels", + ) + .await?; let labels = resp["labels"].as_array().cloned().unwrap_or_default(); if labels.is_empty() { @@ -815,6 +810,7 @@ mod tests { .with_status(409) .with_header("content-type", "application/json") .with_body(r#"{"message":"repository already exists"}"#) + .expect(1) .create_async() .await; @@ -830,6 +826,8 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("already exists")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] @@ -898,6 +896,33 @@ mod tests { _m.assert_async().await; } + #[tokio::test] + async fn cmd_list_repos_surfaces_denial() { + // A node error must Err with the status, not trip the vague "expected + // array" fallback that hides the node's actual response (INV-8). + let dir = TempDir::new().unwrap(); + write_identity(&dir); + + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos\?owner=".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_list(server.url(), Some(dir.path().to_path_buf())) + .await + .unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + #[tokio::test] async fn test_cmd_commits_empty() { let dir = TempDir::new().unwrap(); @@ -997,6 +1022,7 @@ mod tests { .with_status(400) .with_header("content-type", "application/json") .with_body(r#"{"message":"you already have a repo named myrepo"}"#) + .expect(1) .create_async() .await; @@ -1012,6 +1038,8 @@ mod tests { err.to_string().contains("already have a repo"), "got: {err}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } #[tokio::test] @@ -1279,6 +1307,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; @@ -1291,5 +1320,58 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("not found"), "got: {err}"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _repo.assert_async().await; + } + + // ── Gated CLI reads surface node denials, not empty renders (#123 / INV-8) ── + + #[tokio::test] + async fn cmd_commits_surfaces_denial_not_empty() { + // A gated 404 must Err, not print "No commits" as if the repo were empty. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/commits".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_commits( + "alice/secret".to_string(), + "main".to_string(), + 20, + server.url(), + None, + ) + .await; + assert!(result.is_err(), "cmd_commits must Err on 404"); + assert!(result.unwrap_err().to_string().contains("not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn cmd_label_list_surfaces_denial_not_empty() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"^/api/v1/repos/alice/secret/labels".to_string()), + ) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository 'alice/secret' not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_label_list("alice/secret".to_string(), server.url(), None).await; + assert!(result.is_err(), "cmd_label_list must Err on 404"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; } } diff --git a/crates/gl/src/star.rs b/crates/gl/src/star.rs index 120f824..4643c8b 100644 --- a/crates/gl/src/star.rs +++ b/crates/gl/src/star.rs @@ -230,6 +230,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -241,6 +242,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("star failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -287,6 +290,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"repo not found"}"#) + .expect(1) .create_async() .await; @@ -298,6 +302,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("unstar failed")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] diff --git a/crates/gl/src/status.rs b/crates/gl/src/status.rs index 2c77ada..02ac3a7 100644 --- a/crates/gl/src/status.rs +++ b/crates/gl/src/status.rs @@ -81,7 +81,9 @@ pub async fn run(args: StatusArgs) -> Result<()> { let pr_resp = client .get(&format!("/api/v1/repos/{short_owner}/{repo_name}/pulls")) .await; - if let Ok(r) = pr_resp { + if let Some(line) = section_unavailable_line("PRs", &pr_resp) { + println!("{line}"); + } else if let Ok(r) = pr_resp { if let Ok(body) = r.json::().await { let prs = body["pulls"].as_array().cloned().unwrap_or_default(); let open: Vec<_> = prs @@ -108,7 +110,9 @@ pub async fn run(args: StatusArgs) -> Result<()> { let issue_resp = client .get(&format!("/api/v1/repos/{short_owner}/{repo_name}/issues")) .await; - if let Ok(r) = issue_resp { + if let Some(line) = section_unavailable_line("issues", &issue_resp) { + println!("{line}"); + } else if let Ok(r) = issue_resp { if let Ok(body) = r.json::().await { let issues = body["issues"].as_array().cloned().unwrap_or_default(); let open: Vec<_> = issues @@ -136,6 +140,20 @@ pub async fn run(args: StatusArgs) -> Result<()> { Ok(()) } +/// The status line for a repo section (PRs / issues) whose gated read returned a +/// non-2xx: the denial must surface, never render as "no open ..." (INV-8). +/// Returns `None` for a success (the caller renders the body) or a transport error +/// (the section degrades silently — R5 — so the multi-section `gl status` never +/// hard-fails on one denied read). +fn section_unavailable_line(label: &str, resp: &Result) -> Option { + match resp { + Ok(r) if !r.status().is_success() => { + Some(format!(" {label:<10}unavailable ({})", r.status())) + } + _ => None, + } +} + /// Render a simple ASCII trust bar: 0.75 → "███░" fn trust_bar(score: f64) -> String { let filled = (score * 4.0).round() as usize; @@ -337,4 +355,48 @@ mod tests { fn trust_bar_quarter() { assert_eq!(trust_bar(0.25), "█░░░"); } + + // ── gl status section denial surfacing (#123 / INV-8, R5) ──────────── + + async fn get_response( + server: &mut mockito::Server, + status: usize, + ) -> Result { + let _m = server + .mock("GET", "/x") + .with_status(status) + .with_header("content-type", "application/json") + .with_body(r#"{"pulls":[]}"#) + .create_async() + .await; + NodeClient::new(server.url(), None).get("/x").await + } + + #[tokio::test] + async fn gated_section_surfaces_unavailable_not_empty() { + // A gated 404 on a status section must surface "unavailable", never be + // treated as "no open PRs" (INV-8). + let mut server = mockito::Server::new_async().await; + let resp = get_response(&mut server, 404).await; + assert_eq!( + section_unavailable_line("PRs", &resp), + Some(format!(" {:<10}unavailable (404 Not Found)", "PRs")) + ); + } + + #[tokio::test] + async fn success_section_returns_none_for_body_render() { + // A 2xx returns None so the caller renders the body as before. + let mut server = mockito::Server::new_async().await; + let resp = get_response(&mut server, 200).await; + assert!(section_unavailable_line("issues", &resp).is_none()); + } + + #[test] + fn transport_error_degrades_silently() { + // A transport error (not a status) must NOT surface — the section + // degrades silently so gl status never hard-fails on one bad read (R5). + let err: Result = Err(anyhow::anyhow!("connection refused")); + assert!(section_unavailable_line("PRs", &err).is_none()); + } } diff --git a/crates/gl/src/sync.rs b/crates/gl/src/sync.rs index 72fb275..23f0dd6 100644 --- a/crates/gl/src/sync.rs +++ b/crates/gl/src/sync.rs @@ -71,8 +71,9 @@ pub async fn run(args: SyncArgs) -> Result<()> { SyncCmd::Status => { let client = NodeClient::new(&args.node, None); // Just show peer list and node stats for now - let stats: serde_json::Value = client.get("/api/v1/stats").await?.json().await?; - let peers: serde_json::Value = client.get("/api/v1/peers").await?.json().await?; + let stats = + crate::http::read_json(client.get("/api/v1/stats").await?, "node stats").await?; + let peers = crate::http::read_json(client.get("/api/v1/peers").await?, "peers").await?; println!("Node stats:"); println!(" repos: {}", stats["repos"].as_i64().unwrap_or(0)); println!(" agents: {}", stats["agents"].as_i64().unwrap_or(0)); @@ -130,7 +131,7 @@ async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> String { /// reach the terminal verbatim (INV-6). We drop the C0/C1 control bytes (which /// defangs ANSI/OSC escapes) AND the Unicode bidi/format controls (which /// `char::is_control` does not cover — they can reorder the displayed line). -fn sanitize_node_msg(s: &str) -> String { +pub(crate) fn sanitize_node_msg(s: &str) -> String { s.chars() .filter(|c| !c.is_control() && !is_bidi_format(*c)) .take(200) @@ -203,6 +204,7 @@ mod tests { // Valid JSON: the parse-without-status-check bug deserializes this // into a zero-count success struct and prints "✓ sync triggered". .with_body(r#"{"message":"unauthorized"}"#) + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -211,6 +213,8 @@ mod tests { err.to_string().contains("401"), "expected 401 surfaced, got: {err}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -221,6 +225,7 @@ mod tests { .with_status(429) .with_header("content-type", "application/json") .with_body(r#"{"message":"slow down"}"#) + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -229,6 +234,8 @@ mod tests { err.to_string().contains("429"), "expected 429 surfaced, got: {err}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -245,6 +252,7 @@ mod tests { // BEL (\u0007); serde decodes them to real control bytes a naive // client would print. (The status-check bug fake-successes here.) .with_body("{\"message\":\"pwned\\u001b[31m\\u0007bad\"}") + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -256,6 +264,8 @@ mod tests { s.contains("pwned") && s.contains("bad"), "message text dropped: {s:?}" ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -302,6 +312,7 @@ mod tests { .mock("POST", "/api/v1/sync/trigger") .with_status(401) .with_body("A".repeat(2_000_000)) + .expect(1) .create_async() .await; let (args, _dir) = trigger_args(server.url()); @@ -313,6 +324,8 @@ mod tests { "error message not bounded: {} chars", s.len() ); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -343,4 +356,71 @@ mod tests { (0, 0) ); } + + #[tokio::test] + async fn status_surfaces_stats_denial_not_fake_zeros() { + // A node error on /stats must Err, not print "0 repos / 0 agents / 0 pushes". + let mut server = mockito::Server::new_async().await; + let stats = server + .mock("GET", "/api/v1/stats") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + // A peers mock so the pre-fix path reaches a clean success rather than a + // 501 on an unmocked route; after the fix the /stats denial bails first. + let _peers = server + .mock("GET", "/api/v1/peers") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"count":0,"peers":[]}"#) + .create_async() + .await; + let args = SyncArgs { + cmd: SyncCmd::Status, + node: server.url(), + dir: None, + }; + let err = run(args).await.unwrap_err(); + assert!( + err.to_string().contains("500"), + "expected 500 surfaced, got: {err}" + ); + stats.assert_async().await; + } + + #[tokio::test] + async fn status_surfaces_peers_denial() { + // /stats succeeds but /peers denies — the peers read must Err too, not + // print "Known peers: 0". + let mut server = mockito::Server::new_async().await; + let _stats = server + .mock("GET", "/api/v1/stats") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"repos":1,"agents":2,"pushes":3}"#) + .create_async() + .await; + let peers = server + .mock("GET", "/api/v1/peers") + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + let args = SyncArgs { + cmd: SyncCmd::Status, + node: server.url(), + dir: None, + }; + let err = run(args).await.unwrap_err(); + assert!( + err.to_string().contains("500"), + "expected 500 surfaced, got: {err}" + ); + peers.assert_async().await; + } } diff --git a/crates/gl/src/task.rs b/crates/gl/src/task.rs index c26cb35..5b0c3cf 100644 --- a/crates/gl/src/task.rs +++ b/crates/gl/src/task.rs @@ -166,13 +166,14 @@ async fn cmd_create( "delegator_did": delegator_did, }))?; - let resp: Value = client - .post("/api/v1/tasks", &body) - .await - .context("failed to create task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post("/api/v1/tasks", &body) + .await + .context("failed to create task")?, + "create task", + ) + .await?; print_json(&resp); Ok(()) } @@ -191,26 +192,25 @@ async fn cmd_list( if let Some(a) = &assignee_did { path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); } - let resp: Value = client - .get(&path) - .await - .context("failed to list tasks")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client.get(&path).await.context("failed to list tasks")?, + "list tasks", + ) + .await?; print_json(&resp); Ok(()) } async fn cmd_view(id: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); - let resp: Value = client - .get(&format!("/api/v1/tasks/{}", id)) - .await - .context("failed to get task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/tasks/{}", id)) + .await + .context("failed to get task")?, + "view task", + ) + .await?; print_json(&resp); Ok(()) } @@ -221,13 +221,14 @@ async fn cmd_claim(id: String, node: String, dir: Option) -> Result<()> let client = NodeClient::new(&node, Some(keypair)); let body = serde_json::to_vec(&json!({ "assignee_did": assignee_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{}/claim", id), &body) - .await - .context("failed to claim task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{}/claim", id), &body) + .await + .context("failed to claim task")?, + "claim task", + ) + .await?; print_json(&resp); Ok(()) } @@ -243,13 +244,14 @@ async fn cmd_complete( let client = NodeClient::new(&node, Some(keypair)); let body = serde_json::to_vec(&json!({ "result": result, "by_did": by_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{}/complete", id), &body) - .await - .context("failed to complete task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{}/complete", id), &body) + .await + .context("failed to complete task")?, + "complete task", + ) + .await?; print_json(&resp); Ok(()) } @@ -265,13 +267,14 @@ async fn cmd_fail( let client = NodeClient::new(&node, Some(keypair)); let body = serde_json::to_vec(&json!({ "reason": reason, "by_did": by_did }))?; - let resp: Value = client - .post(&format!("/api/v1/tasks/{}/fail", id), &body) - .await - .context("failed to fail task")? - .json() - .await - .context("invalid JSON response")?; + let resp = crate::http::read_json( + client + .post(&format!("/api/v1/tasks/{}/fail", id), &body) + .await + .context("failed to fail task")?, + "fail task", + ) + .await?; print_json(&resp); Ok(()) } @@ -355,11 +358,12 @@ mod tests { .with_status(500) .with_header("content-type", "application/json") .with_body(r#"{"message":"internal error"}"#) + .expect(1) .create_async() .await; - // Should still succeed (prints JSON, doesn't check status code) - cmd_create( + // A node 5xx must surface as an Err, not be printed as if it were a task. + let err = cmd_create( "deploy".to_string(), "agent:task".to_string(), None, @@ -371,7 +375,9 @@ mod tests { Some(dir.path().to_path_buf()), ) .await - .unwrap(); + .unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; } // ── list ───────────────────────────────────────────────────────── @@ -445,11 +451,16 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) + .expect(1) .create_async() .await; - // cmd_view doesn't check status — it prints the JSON - cmd_view("nope".to_string(), server.url()).await.unwrap(); + // A 404 must surface as an Err, not be printed as if it were a task. + let err = cmd_view("nope".to_string(), server.url()) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; } // ── claim ──────────────────────────────────────────────────────── @@ -601,4 +612,122 @@ mod tests { .await .unwrap(); } + + // ── denial surfaces (INV-8): a node 4xx/5xx must Err, not render as success ── + + #[tokio::test] + async fn test_list_tasks_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"boom"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_list(None, None, 50, server.url()).await.unwrap_err(); + assert!(err.to_string().contains("500"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn test_claim_task_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("POST", "/api/v1/tasks/task-x/claim") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"forbidden"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_claim( + "task-x".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("403"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn test_complete_task_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("POST", "/api/v1/tasks/task-x/complete") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"not found"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_complete( + "task-x".to_string(), + None, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } + + #[tokio::test] + async fn test_fail_task_surfaces_denial() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("POST", "/api/v1/tasks/task-x/fail") + .with_status(409) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"already terminal"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_fail( + "task-x".to_string(), + None, + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("409"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/visibility.rs b/crates/gl/src/visibility.rs index eef9ecb..56cbb73 100644 --- a/crates/gl/src/visibility.rs +++ b/crates/gl/src/visibility.rs @@ -87,12 +87,7 @@ async fn resolve_owner_repo( did.split(':').next_back().unwrap_or(&did).to_string() } else { let client = NodeClient::new(node, None); - let info: Value = client - .get("/") - .await? - .json() - .await - .context("failed to fetch node info")?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"].as_str().context("node missing DID")?; did.split(':').next_back().unwrap_or(did).to_string() }; @@ -307,4 +302,26 @@ mod tests { .await .unwrap(); } + + #[tokio::test] + async fn resolve_owner_repo_surfaces_denial() { + // A slash-free repo with an empty identity dir forces the GET / node-info + // fetch. A gated 404 there must Err (surfacing the status), proving the + // read_json conversion is load-bearing rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); // empty, no identity.pem, forces the GET / branch + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_owner_repo("noslash", &server.url(), Some(dir.path())) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; + } } diff --git a/crates/gl/src/webhook.rs b/crates/gl/src/webhook.rs index 45b80ec..30924d5 100644 --- a/crates/gl/src/webhook.rs +++ b/crates/gl/src/webhook.rs @@ -77,7 +77,7 @@ pub async fn run(args: WebhookArgs) -> Result<()> { } async fn resolve_owner(client: &NodeClient) -> Result { - let info: Value = client.get("/").await?.json().await?; + let info = crate::http::read_json(client.get("/").await?, "node info").await?; let did = info["did"] .as_str() .context("node missing DID")? @@ -147,12 +147,13 @@ async fn cmd_list(repo: String, node: String) -> Result<()> { let client = NodeClient::new(&node, None); let owner = resolve_owner(&client).await?; - let resp: Value = client - .get(&format!("/api/v1/repos/{owner}/{repo}/hooks")) - .await? - .json() - .await - .context("invalid JSON")?; + let resp = crate::http::read_json( + client + .get(&format!("/api/v1/repos/{owner}/{repo}/hooks")) + .await?, + "webhooks", + ) + .await?; let hooks = resp["webhooks"].as_array().cloned().unwrap_or_default(); if hooks.is_empty() { @@ -300,6 +301,7 @@ mod tests { .with_status(400) .with_header("content-type", "application/json") .with_body(r#"{"message":"invalid URL"}"#) + .expect(1) .create_async() .await; @@ -314,6 +316,8 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("invalid URL")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; } #[tokio::test] @@ -409,6 +413,7 @@ mod tests { .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"webhook not found"}"#) + .expect(1) .create_async() .await; @@ -421,5 +426,47 @@ mod tests { .await .unwrap_err(); assert!(err.to_string().contains("webhook not found")); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy the error assertion vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn test_list_webhooks_surfaces_denial() { + // Client half of #94: a gated 404 must Err, not print "No webhooks". + let mut server = mockito::Server::new_async().await; + let _root = mock_root(&mut server).await; + let _m = server + .mock("GET", mockito::Matcher::Regex(r"/hooks$".to_string())) + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"repository not found"}"#) + .expect(1) + .create_async() + .await; + let result = cmd_list("my-repo".to_string(), server.url()).await; + assert!(result.is_err(), "webhook list must Err on a gated 404"); + // Prove the mocked route was actually requested; a non-matching request (mockito's 501, also non-2xx) would otherwise satisfy is_err() vacuously. + _m.assert_async().await; + } + + #[tokio::test] + async fn resolve_owner_surfaces_denial() { + // resolve_owner always GETs / for the node DID. A gated 404 there must Err + // (surfacing the status), proving the read_json conversion is load-bearing + // rather than silently ignored. + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/") + .with_status(404) + .with_header("content-type", "application/json") + .with_body(r#"{"message":"denied"}"#) + .expect(1) + .create_async() + .await; + let err = resolve_owner(&crate::http::NodeClient::new(server.url(), None)) + .await + .unwrap_err(); + assert!(err.to_string().contains("404"), "got: {err}"); + _m.assert_async().await; } }