fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123)#186
Open
beardthelion wants to merge 18 commits into
Open
Conversation
added 9 commits
July 10, 2026 23:59
gl read tools parsed node responses with `.json().await?` and never checked the HTTP status, so a visibility-gated 404/5xx body was handed back to the agent as the requested resource with Ok (INV-8 break). Add a shared `read_json` helper (http.rs): surfaces a non-2xx as an Err carrying the sanitized node message (INV-6, reusing sanitize_node_msg, now pub(crate)), and errors on a garbage 2xx body instead of a silent empty success. Route repo_get / repo_commits / repo_tree through it. Denial-path tests drive a gated 404 through call_tool and assert Err (not a fabricated repo / empty commit list); verified load-bearing by reverting the wiring to red.
Route `gl repo commits` / `gl repo labels` (repo.rs) and `gl pr`
list/view/diff/comments (pr.rs) through read_json, so a gated 404/5xx
surfaces as an error instead of an empty render ("No commits", "No pull
requests", "No diff"). The pr.rs mutation commands already status-check
and are left as-is. Denial-path test per read arm.
MCP pr_list / pr_view / pr_diff, webhook_list (the client half of #94), and issue_list; CLI `gl issue list`/`comments` and `gl webhook list`. All route through read_json so a gated denial surfaces as an error, not a fabricated or empty result. Denial-path tests for the MCP arms.
… status (#123) cmd_info: sanitize the node-advertised message before it reaches the terminal (INV-6), preserving the existing bare-404 wording. gl status: a gated 404 on the PR/issue dashboard sections now renders "unavailable (<status>)" instead of silently "no open PRs" (INV-8), without hard-failing the multi-section command. Classification for the rest of the U6 sweep (recorded, none converted): whoami repo-count lists the caller's OWN repos (best-effort, not a withheld-resource read); task/cert/bounty/peer reads are not repo-confidentiality surfaces. gl mutation commands already status-check.
Completeness-sweep catch: /api/v1/repos/{owner}/{name}/certs is a
repo-scoped gated read that rendered a denial as an empty cert list.
Route it through read_json.
…l tests (#123) Code review caught a missed gated read arm: `gl bounty list --repo o/n` and MCP bounty_list hit the authorize_repo_read-gated /api/v1/repos/{o}/{n}/bounties and rendered a denial as a silent-empty success (INV-8). Route both through read_json. Add denial-path tests for the converted-but-untested arms the review flagged: bounty (repo-scoped), cert (was zero coverage), MCP pr_view / pr_diff, CLI issue list, CLI webhook list.
…123) The gl status PR/issue sections rendered a gated 404 via inline println! that no test could reach (the sections only run with a gitlawb remote, and output isn't capturable). Extract the decision into a pure section_unavailable_line helper: non-2xx -> 'unavailable (<status>)' (INV-8), 2xx -> None (caller renders), transport error -> None (degrade silently, R5). Tested all three branches; the 404 case is load-bearing (proven RED when the guard is neutered).
Add read_json edge tests (empty 2xx body -> Err; non-2xx JSON without a message key -> 'request failed' fallback) and a denial test for the standalone `gl pr comments` arm. Every read_json-routed denial test is now load-bearing: mutating read_json to the pre-fix bare-parse turns all 18 arm denial tests + 7 helper error tests RED while the 271 happy-path tests stay green.
Contributor
📝 WalkthroughWalkthroughThe PR adds shared status-aware JSON handling, routes CLI and MCP HTTP responses through it, sanitizes node error messages, and adds tests ensuring denied responses become errors or unavailable status sections rather than fabricated or empty results. ChangesRead denial handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI_or_MCP
participant NodeClient
participant read_json
CLI_or_MCP->>NodeClient: request resource
NodeClient-->>read_json: HTTP response
read_json->>read_json: validate status and sanitize message
read_json-->>CLI_or_MCP: parsed JSON or error
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/gl/src/cert.rs (1)
207-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame missing mock-hit verification as the other new denial tests.
No
.expect(1)/m.assert()on the mock, so an unmatched request (mockito's default 501 for non-matches) would also makeread_jsonreturnErr, letting the test pass even if the anchored regex never actually matched the pathcmd_listbuilt.♻️ Proposed fix
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"); + _m.assert_async().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/cert.rs` around lines 207 - 229, Ensure the cmd_list_surfaces_denial_not_empty test verifies that its mock was hit exactly once by calling the mockito mock’s assert/expect(1) method after cmd_list completes, so unmatched requests cannot satisfy the test.crates/gl/src/issue.rs (1)
739-756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame missing mock-hit verification as the analogous bounty/cert denial tests.
Since mockito returns 501 for any unmatched request, and 501 is also non-2xx,
read_jsonstill errors regardless of whether the regex actually matched the pathcmd_listproduced — add.expect(1)/m.assert()so the test genuinely proves the gated issues-list path is exercised.♻️ Proposed fix
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"); + _m.assert_async().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/issue.rs` around lines 739 - 756, Add mock-hit verification to cmd_list_surfaces_denial_not_empty by retaining the mock handle and calling its assert method after cmd_list completes (or configuring it with expect(1)), ensuring the intended issues-list request matched the configured route.crates/gl/src/bounty.rs (1)
550-571: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDenial test doesn't verify the mocked endpoint was actually hit.
The mock has no
.expect(1)/m.assert(). Per mockito's own docs, Any calls to the Mockito server that are not matched will return 501 Not Implemented. Since 501 is also non-2xx,read_jsonwould still returnErreven if the URL built bycmd_listdidn't match the regex at all — so the test can pass without actually proving the repo-scoped bounties path is constructed/gated correctly.♻️ Proposed fix
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" ); + _m.assert_async().await;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/bounty.rs` around lines 550 - 571, Strengthen cmd_list_repo_scoped_surfaces_denial_not_empty by configuring the Mockito expectation to require exactly one request and explicitly asserting the mock was matched after cmd_list completes. Use the existing _m mock handle’s expectation/assertion API so the test fails if the repo-scoped /repos/alice/secret/bounties endpoint is not requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gl/src/bounty.rs`:
- Around line 550-571: Strengthen cmd_list_repo_scoped_surfaces_denial_not_empty
by configuring the Mockito expectation to require exactly one request and
explicitly asserting the mock was matched after cmd_list completes. Use the
existing _m mock handle’s expectation/assertion API so the test fails if the
repo-scoped /repos/alice/secret/bounties endpoint is not requested.
In `@crates/gl/src/cert.rs`:
- Around line 207-229: Ensure the cmd_list_surfaces_denial_not_empty test
verifies that its mock was hit exactly once by calling the mockito mock’s
assert/expect(1) method after cmd_list completes, so unmatched requests cannot
satisfy the test.
In `@crates/gl/src/issue.rs`:
- Around line 739-756: Add mock-hit verification to
cmd_list_surfaces_denial_not_empty by retaining the mock handle and calling its
assert method after cmd_list completes (or configuring it with expect(1)),
ensuring the intended issues-list request matched the configured route.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f7b1bd9-cb1b-46ef-a606-2bd697fa0fea
📒 Files selected for processing (10)
crates/gl/src/bounty.rscrates/gl/src/cert.rscrates/gl/src/http.rscrates/gl/src/issue.rscrates/gl/src/mcp.rscrates/gl/src/pr.rscrates/gl/src/repo.rscrates/gl/src/status.rscrates/gl/src/sync.rscrates/gl/src/webhook.rs
beardthelion
added
crate:gl
added 9 commits
July 11, 2026 13:07
…ching route can't pass vacuously (#123)
…er-error tests so a non-matching route can't pass vacuously (#123)
…nc, star, agent, protect, changelog) so a non-matching route can't pass vacuously (#123)
…ty show, issue show/close/comment, webhook create/delete) so a non-matching route can't pass vacuously (#123)
…l isn't rendered as success (#123)
…ndered as a successful tool result (#123)
…e denial isn't rendered as success (#123)
…son so a node error surfaces its status not a vague fallback (#123)
… per-helper denial tests that go RED without the conversion (#123)
beardthelion
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/gl/src/changelog.rs (1)
57-68: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMain changelog fetch still bypasses
read_json/sanitize_node_msg.The status check here is correct (no fabricated-success risk), but the error message is taken raw from
body["message"]without the sanitizationread_jsonnow applies elsewhere. Converging this call ontocrate::http::read_jsonwould close the same terminal-injection gap already fixed forsync triggerand the line-45 fetch in this same function.♻️ Suggested consolidation
- let resp = client - .get(&url) - .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!("changelog failed ({status}): {msg}"); - } + let body = crate::http::read_json( + client.get(&url).await.context("failed to connect to node")?, + "changelog", + ) + .await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/changelog.rs` around lines 57 - 68, Update the main changelog fetch in the surrounding function to use crate::http::read_json, reusing its sanitize_node_msg handling for the response error message while preserving the existing non-success status check and changelog failure behavior. Remove the direct resp.json parsing path so this fetch follows the same sanitized handling as the other calls.crates/gl/src/register.rs (1)
58-67: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
gl register's error path bypasses the newread_json/sanitize_node_msgsanitization.This function still hand-rolls status/message extraction instead of using
crate::http::read_json, so a hostile node'smessagefield is not passed throughsanitize_node_msgbefore being embedded in the error and printed to the terminal — unlike the now-convertedagent_registerMCP tool that hits the same/api/registerendpoint. Worth converging for consistency and to close the control-char/ANSI-injection gap already fixed elsewhere in this PR.♻️ Suggested consolidation
- let status = resp.status(); - let payload: Value = resp.json().await.context("invalid JSON response")?; - - if !status.is_success() { - let msg = payload - .get("message") - .and_then(|v| v.as_str()) - .unwrap_or("unknown error"); - anyhow::bail!("registration failed ({status}): {msg}"); - } + let payload = crate::http::read_json(resp, "registration").await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gl/src/register.rs` around lines 58 - 67, Update the registration flow in crates/gl/src/register.rs to use crate::http::read_json for the /api/register response instead of manually parsing the status and message from resp. Preserve the existing registration failure behavior while ensuring error messages pass through sanitize_node_msg via the shared helper, matching agent_register.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gl/src/changelog.rs`:
- Around line 57-68: Update the main changelog fetch in the surrounding function
to use crate::http::read_json, reusing its sanitize_node_msg handling for the
response error message while preserving the existing non-success status check
and changelog failure behavior. Remove the direct resp.json parsing path so this
fetch follows the same sanitized handling as the other calls.
In `@crates/gl/src/register.rs`:
- Around line 58-67: Update the registration flow in crates/gl/src/register.rs
to use crate::http::read_json for the /api/register response instead of manually
parsing the status and message from resp. Preserve the existing registration
failure behavior while ensuring error messages pass through sanitize_node_msg
via the shared helper, matching agent_register.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f429da4-22e0-47c0-956d-ea60ced0434a
📒 Files selected for processing (16)
crates/gl/src/agent.rscrates/gl/src/bounty.rscrates/gl/src/cert.rscrates/gl/src/changelog.rscrates/gl/src/issue.rscrates/gl/src/mcp.rscrates/gl/src/peer.rscrates/gl/src/pr.rscrates/gl/src/protect.rscrates/gl/src/register.rscrates/gl/src/repo.rscrates/gl/src/star.rscrates/gl/src/sync.rscrates/gl/src/task.rscrates/gl/src/visibility.rscrates/gl/src/webhook.rs
✅ Files skipped from review due to trivial changes (1)
- crates/gl/src/star.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/gl/src/bounty.rs
- crates/gl/src/cert.rs
- crates/gl/src/repo.rs
- crates/gl/src/pr.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
glclient commands and the MCP tools parsed HTTP responses without checking status, so a node 4xx/5xx (a gated denial, an auth failure, a server error) was deserialized and rendered as success: an empty list, a fabricated "0 stats", the error body printed as if it were data, or a silently swallowed denial. This routes every such surface throughcrate::http::read_json(or an explicit status check where the payload is not JSON) so a denial surfaces as anErrcarrying the node's status and message.INV-8: a gated denial must not be rendered as a result.
Surfaces
GET /helpers, which reported a vague "node missing DID" on a node error instead of the node's actual status.Left as-is on purpose: the diagnostic/fail-soft paths that intentionally degrade rather than abort (
gl node status/resolvedashboards,gl doctor, thepeer addpublic-URL lookup, the cert-show node-DID hint). Those show a clear degraded state, not fabricated data.Verification
Each surface has a denial test that drives a 4xx/5xx through the real client path and asserts the error carries the node's status. The production conversions were run RED against the pre-fix code (the arm rendered success) and GREEN after. Denial tests carry
.expect(1)mock-hit assertions so a non-matching route can't satisfy them vacuously, and those assertions were confirmed load-bearing (a wrong route drives them RED). Full gl suite green; fmt and clippy clean.Scope
This began as the read-tools fix and grew into the whole-client sweep once it was clear the same denial-as-success bug spanned the CLI, the MCP twin, and the resolve helpers. It is one coherent theme (#123 / INV-8), but it is a lot bigger than the original diff. Happy to split it into pieces if that reviews better.
Summary by CodeRabbit
gl statusnow explicitly marks non-success PRs/issues sections as unavailable with HTTP status.