fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123)#186
Conversation
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces centralized, status-aware JSON response handling through ChangesHTTP denial handling
Estimated code review effort: 4 (Complex) | ~70 minutes Sequence Diagram(s)sequenceDiagram
participant Handler as CLI or MCP handler
participant NodeClient
participant read_json
Handler->>NodeClient: Request resource
NodeClient-->>read_json: HTTP response
alt 2xx
read_json->>read_json: Parse JSON
read_json-->>Handler: Parsed value
else non-2xx
read_json->>read_json: Read capped and sanitize message
read_json-->>Handler: Error with status and message
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
…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
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
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Status-check the CLI bounty-stats response
crates/gl/src/bounty.rs:387
cmd_statsstill deserializes/api/v1/bounties/statswithunwrap_or_default()and never checks the HTTP status. Consequently, a 403/404/500 (including a non-JSON body) is printed as a successfulBounty Statsresult with every counter set to zero. This is the same denial-as-fake-result behavior the PR fixes elsewhere, and the MCP twin already routes this endpoint throughread_json. Route this CLI request throughread_jsonand add a non-2xx regression test. -
[P2] Bound error bodies in the shared response helper
crates/gl/src/http.rs:204
The newread_jsonhelper callsresp.json()on every non-2xx response before it extracts and truncatesmessage. That buffers and parses the complete response, so a configured or malicious node can stream an arbitrarily large valid JSON error body and exhaust the CLI/MCP process's memory across the newly migrated calls. The 200-character display cap does not bound allocation, andsync::read_body_cappedalready implements the required capped-read pattern for the same hostile-node error path. Read a small capped body before parsing its message, then add a large-error-body regression test.
F1: cmd_stats deserialized /api/v1/bounties/stats with unwrap_or_default() and never checked the HTTP status, so a 403/404/5xx (or non-JSON body) printed a successful Bounty Stats with every counter zeroed — the denial-as-fake-result class this PR fixes elsewhere. Route it through read_json (like the sibling bounty commands and the MCP twin). F2: read_json called resp.json() on every non-2xx response, buffering and parsing the whole body before the 200-char DISPLAY cap applied, so a hostile node could stream an arbitrarily large valid JSON error and exhaust the process's memory. Read a capped body (read_body_capped, 8 KiB — the same bound the sync error path uses) and best-effort extract message from it; a message-less or non-JSON body still falls back to the status alone. read_body_capped is now pub(crate). RED->GREEN: cmd_stats_surfaces_denial_not_fake_result (403 -> Err, was Ok+zeros). read_json_bounds_the_error_body_read (a message placed past the cap is ABSENT from the error; was surfaced by the full parse). All existing read_json + stats tests stay green; gl suite 339, fmt + clippy clean.
|
Both addressed on Stats status check (F1). Bounded error body (F2). Each fix is RED->GREEN with the guard reverted to confirm it is load-bearing. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Apply the capped/sanitized error path to the remaining manual status handlers
crates/gl/src/bounty.rs:193
This command (and other changed manual handlers such asagent,issue,pr,repo,register,star,protect,visibility, andwebhook) callsresp.json()before inspecting a non-2xx status. A configured or malicious node can therefore return an arbitrarily large valid JSON error and makeglbuffer it in full, and several of these paths print itsmessagewithout the new terminal-control sanitization. That leaves the exact hostile-error-body failure mode thathttp::read_jsonnow documents and caps. Check the status before a bounded error read (or route these arms through the shared helper) and retain the command-specific success handling.
…fore-parse) ~40 handler call sites still parsed resp.json() before checking status, so a hostile/misconfigured node could make gl buffer an arbitrarily large JSON error body (unbounded) and print the node message unsanitized. Route them all through crate::http::read_json (status-first, capped+sanitized error, JSON-or-Err on 2xx). Two GET / node-info reads (peer cmd_add, cert cmd_show) had NO status check at all — a denied node-info was consumed as success (INV-8); they now surface the denial as Err. Bespoke branches preserved: agent 404 'node does not support agents API' hint, repo cmd_info 404 'repository not found'. 341 gl tests green.
…status gate Adversarial coverage for the #186 read_json conversions: - peer cmd_add / cert cmd_show: a denied `GET /` node-info now surfaces as an Err instead of being parsed as success (INV-8). RED without the read_json conversion. - star cmd_add: a hostile 500 carrying terminal-control + bidi bytes and a long message reaches the terminal neither verbatim (sanitized) nor unbounded. - new tests/no_parse_before_status.rs: a source-level completeness gate that fails if any converted handler reintroduces a resp.json()-before-status bypass. Proven load-bearing (reverting one converted site flags it RED). Pre-existing bypasses in files this PR did not touch (init/mirror/profile/quickstart/whoami) are known debt, out of this gate's scope.
The #186 restructure of cmd_info kept the bespoke 404 'repository not found' message ahead of read_json and routed every other status through it. Pin both: a 404 still yields the bespoke text; a 500 yields read_json's 'repo info failed (500)'.
|
Done at Routed every remaining manual-status handler through While scoping it I found two sites the finding didn't name that are worse than the P2: Bespoke branches preserved: the To stop this class from creeping back I added
One scope note: the gate surfaced the same bypass idiom in @jatmn ready for another look. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@crates/gl/src/star.rs`:
- Around line 343-369: Update the test around cmd_add to retain the created mock
handle and assert it was hit after the command completes. Ensure the assertion
verifies the /star mock received the expected PUT request before evaluating the
sanitized error contents, so a route mismatch cannot satisfy the test.
In `@crates/gl/tests/no_parse_before_status.rs`:
- Around line 23-37: Add "task" to the CONVERTED list in
no_parse_before_status.rs so the gate covers the converted task handler and
detects parse-before-status regressions in task.rs.
- Around line 50-56: Update the parse detection in the test around the
`.json().await` check to structurally identify direct Response::json calls,
including turbofish arguments and split-line method chains, while excluding
calls routed through read_json. Preserve the existing status-window validation
and any explicitly intended exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5404c35-a02e-4774-a405-afc3816b310c
📒 Files selected for processing (14)
crates/gl/src/agent.rscrates/gl/src/bounty.rscrates/gl/src/cert.rscrates/gl/src/changelog.rscrates/gl/src/issue.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/visibility.rscrates/gl/src/webhook.rscrates/gl/tests/no_parse_before_status.rs
…ile mock hit (#186) Address the three CodeRabbit threads on the #186 client-read-status work, all INV-21 (load-bearing / completeness) findings. no_parse_before_status.rs: the gate hand-listed the converted handlers, and the list had already drifted (task, mcp, and sync all call read_json but were absent, so a parse-before-status regression in them would not fail the gate). Derive the scanned set from the source tree instead: every src/*.rs that references read_json (minus http.rs, the definition site), so a converted handler auto-enrolls. The known out-of-scope debt (init/mirror/profile/ quickstart/whoami) uses raw resp.json().await and no read_json, so the derivation excludes it without a skip list. Detection now anchors on .json( and .json::<, catching turbofish resp.json::<Value>().await and split-line chains the old bare .json().await substring missed, still keyed on an is_success() that lands after the parse so status-first probes stay green. star.rs: the cmd_add hostile-response test never asserted the mock was hit, so a route mismatch would serve mockito's fallback and satisfy every assertion without exercising the sanitization. Add .expect(1) and _m.assert_async().
|
@jatmn the manual-handler status finding was already addressed on the current head (e3e21e0); your review was against 85dfbd2, before that conversion pass landed. On the current head every handler you named routes its error read through The only raw parses left are three in The three CodeRabbit threads on this head are also resolved: the completeness gate now derives its scanned set from |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve the fail-soft peer URL fallback
crates/gl/src/peer.rs:96
gl peer addonly needsGET /to improve the URL it advertises; the existingpublic_urlfallback intentionally uses--nodewhen that optional lookup has no usable value. Routing the lookup throughread_jsonnow returns before the remote/api/v1/peers/announcerequest whenever the local root endpoint returns a JSON 4xx/5xx, even though that remote announcement can still succeed with the fallback URL. This regresses the command's degraded-mode behavior and contradicts the PR's stated decision to leave the peer-add public-URL lookup fail-soft. Keep the remote announcement reachable on an unavailable local node-info response, while retaining status checking for the actual announce request. -
[P2] Do not make the post-fetch certificate hint fatal
crates/gl/src/cert.rs:140
The certificate has already been fetched and printed when this ancillaryGET /runs; it is used only to compare the certificate's node DID with the current node and print offline-verification guidance. A JSON 4xx/5xx response previously left the DID empty and emitted the mismatch/degraded hint, butread_json(...)?now abortsgl cert showafter its partial certificate output and omits that guidance. The PR explicitly identifies this node-DID hint as a diagnostic/fail-soft path that should stay unchanged. Handle an unavailable node-info lookup as a degraded comparison instead of failing the completed certificate-show operation.
The local GET / in cmd_add only improves the URL we advertise; a response-level failure now falls back to --node with a stderr note instead of aborting before the announce, which keeps its fail-closed read_json check. Guards pin the fallback (500 and non-JSON body), the announce denial, and the public_url success path.
The post-print GET / only feeds the node-DID comparison hint; a response-level failure now prints an explicit could-not-compare note with the offline-verification guidance and the command completes. The certificate fetch keeps its fail-closed read_json check, pinned by a denied-cert guard whose GET / mock asserts expect(0).
…ricate a DID mismatch (#186) Review findings on the fail-soft restoration: the follow-up local peer-list add printed an unconditional success line over an ignored POST result (now status-gated, with a stderr note on failure, still fail-soft); a 200 node-info body carrying no usable DID fell into the empty-string comparison and printed a fabricated 'does not match ()' warning (now routed to the could-not-compare hint via the extracted did_check_report helper, whose unit tests pin all four outcomes); the restructured mismatch branch gained an end-to-end test; the denied- announce test's info mock expectation is now asserted.
|
Both addressed on Fail-soft peer URL fallback. The local Non-fatal cert hint. The boundary, named: response-level failures degrade (any non-2xx, a 2xx with a malformed body, and for cert show a 2xx carrying no usable Hardening in the same push: the follow-up local peer-list add no longer prints "Added to local peer list." over an ignored POST result (status-gated, still best-effort), and the cert DID verdict selection moved into a small |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Keep the regression gate's converted-handler set independent of
read_jsonusage
crates/gl/tests/no_parse_before_status.rs:33
The test chooses files to inspect only when their current text containsread_json. A full regression of a converted single-call handler therefore removes that marker and drops the file from the scan: for example, restoringregister.rsto its previousresp.json().awaitfollowed byis_success()leaves noread_jsonreference in that file, while the test still has other handlers and passes. This defeats the stated guard against exactly the parse-before-status regression the PR is adding it to catch. Keep an expected set of converted handler files (and assert it is scanned), or derive that set from an independent source, so removal of the final helper call fails the test.
…ail closed both ways The gate derived its scanned set from read_json usage, but a converted handler with a single node call (register.rs) reverted to resp.json().await loses the marker, drops out of the derived set, and its bypass goes unscanned. Pin the converted surface as CONVERTED_IN_186 and assert the derived read_json set EQUALS it, failing closed in both directions: a pinned file that drops out was reverted off read_json (RED), and a file using read_json that is not pinned is an unenrolled conversion whose own later revert would escape the same way (RED until it is added). Equality extends the protection to handlers converted after #186, not just the original sixteen. Verified by execution: reverting register.rs's one read_json call trips the deconverted assert; a new read_json handler left unpinned trips the unpinned assert; a partial revert in a file that keeps read_json is still caught by the offender scan; and the pre-change gate passed on the register.rs revert.
Fixed on Verified by execution: reverting register.rs's one call trips the deconverted assert, an unpinned |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Surface failed trust lookups instead of saying the identity is unregistered
crates/gl/src/status.rs:40
This branch still sends every non-2xx response fromGET /api/v1/agents/{short_key}to thenot registeredoutput. Thus a 403, 429, or 5xx for an existing identity is presented as a registration-state result and tells the user to rungl register, while this PR now explicitly renders the analogous PR/issue status failures as unavailable. Keep the fail-soft behavior if desired, but distinguish a genuine not-found response from other HTTP failures and report those as unavailable rather than fabricating an unregistered state.
2 participants
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
Bug Fixes
gl statusnow reports “unavailable ()” for non-success PRs/issues responses.Tests