Skip to content

fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123) #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
beardthelion wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123) #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e179cd2
fix(gl): status-check the client read/write surfaces so a node denial…
Jul 22, 2026
0279707
fix(gl): report why the node-DID comparison was skipped in cert show
beardthelion Jul 28, 2026
ad0e35a
fix(gl): close two completeness-gate escapes; surface the node's erro…
beardthelion Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 16 additions & 25 deletions crates/gl/src/agent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;

use crate::http::NodeClient;
Expand Down Expand Up @@ -57,24 +56,16 @@ async fn cmd_list(node: String, capability: Option<String>) -> Result<()> {
.get(&path)
.await
.context("failed to connect to node")?;
let status = resp.status();

if status == reqwest::StatusCode::NOT_FOUND {
// Keep the bespoke 404 hint (older node without the agents API) ahead of
// read_json; every other status routes through it (capped + sanitized error).
if resp.status() == reqwest::StatusCode::NOT_FOUND {
anyhow::bail!(
"this node does not yet support the agents API (v0.3+)\n\
upgrade the node or check GITLAWB_NODE is pointing to the right server"
);
}

let body: Value = resp
.json()
.await
.context("invalid JSON from node — is GITLAWB_NODE correct?")?;

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("list agents failed ({status}): {msg}");
}
let body = crate::http::read_json(resp, "list agents").await?;

let agents = body["agents"].as_array().cloned().unwrap_or_default();

Expand Down Expand Up @@ -117,24 +108,15 @@ async fn cmd_show(did: String, node: String) -> Result<()> {
.get(&format!("/api/v1/agents/{did}"))
.await
.context("failed to connect to node")?;
let status = resp.status();

if status == reqwest::StatusCode::NOT_FOUND {
// Keep the bespoke 404 hint ahead of read_json; other statuses route through it.
if resp.status() == reqwest::StatusCode::NOT_FOUND {
anyhow::bail!(
"agent not found, or this node does not yet support the agents API (v0.3+)\n\
upgrade the node or check GITLAWB_NODE is pointing to the right server"
);
}

let agent: Value = resp
.json()
.await
.context("invalid JSON from node — is GITLAWB_NODE correct?")?;

if !status.is_success() {
let msg = agent["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("agent not found ({status}): {msg}");
}
let agent = crate::http::read_json(resp, "agent").await?;

let full_did = agent["did"].as_str().unwrap_or("?");
let trust = agent["trust_score"].as_f64().unwrap_or(0.0);
Expand Down Expand Up @@ -215,11 +197,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]
Expand All @@ -230,11 +215,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]
Expand All @@ -261,13 +249,16 @@ mod tests {
.with_status(404)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"agent not found"}"#)
.expect(1)
.create_async()
.await;

let err = cmd_show("did:key:z6MkMissing".to_string(), server.url())
.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]
Expand Down
146 changes: 96 additions & 50 deletions crates/gl/src/bounty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

use anyhow::{Context, Result};
use clap::{Args, Subcommand};
use serde_json::Value;
use std::path::PathBuf;

use crate::http::NodeClient;
Expand Down Expand Up @@ -189,13 +188,7 @@ async fn cmd_create(
.await
.context("failed to connect to node")?;

let status = resp.status();
let body: Value = resp.json().await.unwrap_or_default();

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("create bounty failed ({status}): {msg}");
}
let body = crate::http::read_json(resp, "create bounty").await?;

let id = body["id"].as_str().unwrap_or("?");
println!("Bounty created: {id}");
Expand Down Expand Up @@ -231,11 +224,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 {
Expand All @@ -259,13 +255,7 @@ async fn cmd_show(id: String, node: String, dir: Option<PathBuf>) -> Result<()>
let client = signed_client(&node, dir.as_deref());
let resp = client.get_authed(&format!("/api/v1/bounties/{id}")).await?;

let status = resp.status();
let body: Value = resp.json().await.unwrap_or_default();

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("not found");
anyhow::bail!("bounty not found: {msg}");
}
let body = crate::http::read_json(resp, "show bounty").await?;

println!("{}", serde_json::to_string_pretty(&body)?);
Ok(())
Expand All @@ -289,13 +279,7 @@ async fn cmd_claim(
)
.await?;

let status = resp.status();
let body: Value = resp.json().await.unwrap_or_default();

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("claim failed ({status}): {msg}");
}
let _ = crate::http::read_json(resp, "claim").await?;

println!("Bounty {id} claimed. Deadline starts now.");
Ok(())
Expand All @@ -314,13 +298,7 @@ async fn cmd_submit(id: String, pr: String, node: String, dir: Option<PathBuf>)
)
.await?;

let status = resp.status();
let body: Value = resp.json().await.unwrap_or_default();

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("submit failed ({status}): {msg}");
}
let _ = crate::http::read_json(resp, "submit").await?;

println!("Bounty {id} submitted with PR {pr}. Awaiting creator approval.");
Ok(())
Expand All @@ -344,13 +322,7 @@ async fn cmd_approve(
)
.await?;

let status = resp.status();
let body: Value = resp.json().await.unwrap_or_default();

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("approve failed ({status}): {msg}");
}
let _ = crate::http::read_json(resp, "approve").await?;

println!("Bounty {id} approved! Payout released to agent.");
Ok(())
Expand All @@ -369,22 +341,19 @@ async fn cmd_cancel(id: String, node: String, dir: Option<PathBuf>) -> Result<()
)
.await?;

let status = resp.status();
let body: Value = resp.json().await.unwrap_or_default();

if !status.is_success() {
let msg = body["message"].as_str().unwrap_or("unknown error");
anyhow::bail!("cancel failed ({status}): {msg}");
}
let _ = crate::http::read_json(resp, "cancel").await?;

println!("Bounty {id} cancelled. Tokens refunded.");
Ok(())
}

async fn cmd_stats(node: String) -> Result<()> {
let client = NodeClient::new(&node, None);
let resp = client.get("/api/v1/bounties/stats").await?;
let body: Value = resp.json().await.unwrap_or_default();
// Route through read_json so a node denial (403/404/5xx, incl. a non-JSON
// body) surfaces as an error instead of a fake `Bounty Stats` with every
// counter zeroed (#186, the denial-as-success class #123 fixes elsewhere).
let body =
crate::http::read_json(client.get("/api/v1/bounties/stats").await?, "bounty stats").await?;

let open = body["open"].as_i64().unwrap_or(0);
let claimed = body["claimed"].as_i64().unwrap_or(0);
Expand Down Expand Up @@ -521,13 +490,16 @@ 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_show("nonexistent".to_string(), server.url(), None)
.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]
Expand All @@ -544,4 +516,78 @@ mod tests {

cmd_stats(server.url()).await.unwrap();
}

/// #186 (F1): a node denial on the stats endpoint must surface as an error,
/// not print a fake `Bounty Stats` with every counter zeroed. RED before the
/// fix (unwrap_or_default + no status check -> Ok with zeros), GREEN after
/// (read_json bails on the non-2xx).
#[tokio::test]
async fn cmd_stats_surfaces_denial_not_fake_result() {
let mut server = mockito::Server::new_async().await;
let _m = server
.mock("GET", mockito::Matcher::Regex(r"/stats$".to_string()))
.with_status(403)
.with_header("content-type", "application/json")
.with_body(r#"{"message":"forbidden"}"#)
.create_async()
.await;

let err = cmd_stats(server.url())
.await
.expect_err("a 403 must be an error, not a zeroed fake result");
assert!(err.to_string().contains("403"), "err={err}");
}

/// #186 (F1): the same denial-as-error behavior across the non-2xx response
/// classes, not just 403 — a 404 and a 500 (incl. a non-JSON body) must each
/// surface as an error with the status, never a zeroed fake result.
#[tokio::test]
async fn cmd_stats_surfaces_denial_across_status_classes() {
for (status, body, json) in [
(404, r#"{"message":"not found"}"#, true),
(500, "internal boom", false),
] {
let mut server = mockito::Server::new_async().await;
let mut m = server
.mock("GET", mockito::Matcher::Regex(r"/stats$".to_string()))
.with_status(status)
.with_body(body);
if json {
m = m.with_header("content-type", "application/json");
}
let _m = m.create_async().await;

let err = cmd_stats(server.url()).await.unwrap_err().to_string();
assert!(
err.contains(&status.to_string()),
"status {status} must surface as an error, got: {err}"
);
}
}

#[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;
}
}
Loading
Loading