fix(gl): only report registered: false on 404 in whoami (#220)

[Gravirei]

Summary

gl whoami now only reports registered: false on a genuine 404. All other non-2xx responses (403, 429, 5xx) and transport failures surface an error instead of fabricating an unregistered verdict.

Motivation & context

Closes #220

gl whoami routed every non-2xx response and transport error to registered = Some(false), meaning a 403/429/5xx or network failure for an existing identity was presented as unregistered. Only a 404 from GET /api/v1/agents/{did} means the agent does not exist.

Kind of change

• Bug fix

What changed

• crates/gl/src/whoami.rs — split the match arm in the agent lookup:
  • 404 → registered = Some(false) (agent not found on node)
  • Other non-2xx (403, 500, etc.) → bail with the HTTP status and response message
  • Transport errors → bail with the connection error
• Added tests for 403, 500, and transport error scenarios asserting the command does not silently register as unregistered

How a reviewer can verify

cargo test -p gl -- whoami

All 269 tests pass.

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally
• New behavior is covered by tests (required for fixes)
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (fix(...))

Summary by CodeRabbit

• Bug Fixes
  • Improved node-registration error handling with clearer behavior for not-found, forbidden, server failures, and unreachable/transport issues.
  • Error output now uses capped response bodies, extracts JSON message/error when available, and sanitizes terminal control characters.
  • Parsed capability strings are now sanitized to prevent unsafe terminal output.
• Tests
  • Added async tests for 403/500 messaging, unreachable-node transport failure, capped handling of very large error bodies, and terminal character sanitization.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

Thanks for the contribution. A couple of things will help us review this faster:

• This changes Rust source but no tests changed. Tests are required for fixes and strongly encouraged for features.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

[coderabbitai]

Warning

Review limit reached

@Gravirei, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1259852a-d351-49d9-84b1-052a16c9b795

📥 Commits

Reviewing files that changed from the base of the PR and between ece80cf and ab59d56.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

• crates/gl/src/sync.rs
• crates/gl/src/whoami.rs

📝 Walkthrough

Walkthrough

gl whoami now distinguishes genuine unregistration from HTTP failures and transport errors. Error bodies are bounded and sanitized through crate-visible helpers, with tests covering status codes, transport failures, large bodies, and control characters.

Changes

Agent lookup error handling

Layer / File(s)	Summary
Shared error helpers crates/gl/src/sync.rs	Makes response-body reading and error-message sanitization helpers available across the crate without changing their logic.
Node lookup error classification and tests crates/gl/src/whoami.rs	Sanitizes capabilities, handles 404 responses as unregistered, and fails explicitly for other HTTP statuses and transport errors with bounded, sanitized messages and dedicated tests.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• Gitlawb/node#161: Makes the sync error helpers crate-visible for reuse by whoami.
• Gitlawb/node#186: Contains related status-aware, bounded, and sanitized error handling.

Suggested labels: needs-tests, crate:gl, kind:bug, subsystem:identity

Suggested reviewers: jatmn

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main whoami fix and matches the changeset.
Description check	✅ Passed	The description follows the template and covers summary, motivation, change type, concrete changes, and verification steps.
Linked Issues check	✅ Passed	The changes satisfy #220 by treating only 404 as unregistered and surfacing other non-2xx and transport failures with tests.
Out of Scope Changes check	✅ Passed	The additional sync.rs visibility and sanitization/body-capping helpers are supportive of the whoami error-handling fix, not unrelated churn.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Bound and sanitize the node-supplied error body before displaying it
  crates/gl/src/whoami.rs:59
  This new branch calls Response::json() on every non-404 response and interpolates the remote message directly into the CLI error. A node chosen via --node/GITLAWB_NODE can return a huge JSON body, which reqwest buffers in full before parsing, exhausting the CLI's memory; it can also put ANSI/OSC or bidi controls in message, which are decoded and emitted to the terminal. This is newly reachable because the previous code discarded error bodies. Use the existing capped-read and terminal-sanitization pattern in sync.rs (and add hostile-body/control-character coverage) before including any remote text in the diagnostic.

• [P2] Do not treat every route-level 404 as proof of an unregistered identity
  crates/gl/src/whoami.rs:54
  A 404 also occurs when GITLAWB_NODE points at an older node or a wrong/misrouted server that does not implement /api/v1/agents/{did}. In that case this command still emits registered: false, recreating the false verdict the PR is intended to eliminate. gl agent show already recognizes this ambiguity and tells the user the node may not support the agents API. Preserve that distinction here (or otherwise verify endpoint support) rather than treating an undifferentiated 404 as an authoritative registration result.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/gl/src/whoami.rs (1)

246-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the response-message contract.

These tests only verify the status. Also assert "forbidden" and "internal error" so they protect the required status-and-message error output.

Also applies to: 273-275

🤖 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/whoami.rs` around lines 246 - 248, Update the error assertions
in the tests around run(args).await.unwrap_err() to verify the response message
includes the required text in addition to status 403. Assert that the formatted
error contains both “forbidden” and “internal error”, preserving the existing
diagnostic output for failures.

🤖 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/whoami.rs`:
- Around line 55-60: Update the 404 response branch in the whoami request
handling to set registered = Some(false) and continue through the successful
result path instead of calling bail!. Restore
test_whoami_with_node_not_registered to assert a successful response with the
agent marked unregistered, replacing its unwrap_err() expectation.

---

Nitpick comments:
In `@crates/gl/src/whoami.rs`:
- Around line 246-248: Update the error assertions in the tests around
run(args).await.unwrap_err() to verify the response message includes the
required text in addition to status 403. Assert that the formatted error
contains both “forbidden” and “internal error”, preserving the existing
diagnostic output for failures.

🪄 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: 74b2127a-1bdd-4210-9b87-b9fa1244aeea

📥 Commits

Reviewing files that changed from the base of the PR and between d5ec469 and 36eadc5.

📒 Files selected for processing (2)

• crates/gl/src/sync.rs
• crates/gl/src/whoami.rs

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P2] Do not treat an undifferentiated 404 as proof of an unregistered identity
  crates/gl/src/whoami.rs:55
  This endpoint-level 404 is also what an older node, a reverse-proxy mistake, or another misrouted GITLAWB_NODE returns when /api/v1/agents/{did} is not implemented. gl whoami therefore exits successfully and emits registered: false for an identity whose registration was never queried—the false verdict this change is intended to eliminate. gl agent show already preserves this ambiguity for the identical route; retain that error path here (or establish endpoint support before reporting false) and cover the unsupported-node case separately from a genuine missing agent.

[beardthelion]

Re-reviewed the current head. The error-arm hardening is correct: the non-2xx body is read through the capped-read helper (streams and stops at 8 KB, never buffers via .json()/.text()) and displayed through sanitize_node_msg, both reused from sync.rs, so the earlier bound-and-sanitize ask is genuinely handled. The 404-to-bail choice is a sound call too, matching gl agent show's existing 404 behavior. Two issues remain before this is ready, one code and one test, both small. Findings below.

Findings

• [P2] Sanitize node-supplied capabilities on the success path too
  crates/gl/src/whoami.rs:112
println!("Caps: {}", capabilities.join(", ")) prints arbitrary strings from the node's 200 response straight to the terminal with no sanitization, while the error arms this PR added are careful to strip controls. A hostile node selected via --node/GITLAWB_NODE can return a 200 whose capabilities array holds an OSC title-set sequence (valid JSON \uXXXX escapes decode to real control bytes), and it reaches the terminal verbatim -- the same terminal-escape class the PR defends against on the error path. It is pre-existing (main prints it the same way), but this PR is the one adding node-output sanitization to this file, so the sibling display sink belongs in scope. Route it through sanitize_node_msg like the error arm. JSON mode is fine (serde escapes controls); human mode is the hole.

• [P2] The control-character test does not exercise a real control byte, so it never protects the sanitizer
  crates/gl/src/whoami.rs:349
  The mock body writes \\u{1b} (double backslash), so the bytes are the literal 6-character text \u{1b}, not a 0x1b ESC. That is also not valid JSON (\u{1b} is not a JSON escape; JSON uses the form backslash-u-XXXX, four hex digits, no braces), so the parse fails and msg falls back to the raw text, which contains no control byte at all. The assertions contains("owned") and !contains(ESC byte) are both trivially true, and I confirmed by execution that removing the sanitize_node_msg call at line 65 leaves this test GREEN. Since the point of adding it was control-character coverage, it currently gives false confidence. Send real controls via valid JSON escapes (backslash-u001b / backslash-u0007 / backslash-u202e for ESC/BEL/RTL): with that body the test is GREEN with the sanitizer and RED without it ("ESC control char leaked"), verified both ways. (The helper itself is genuinely covered by sync.rs's unit test, so production behavior is safe -- only this integration test is vacuous.)

• [P3] The body-size-cap test does not assert the cap
  crates/gl/src/whoami.rs:307
  Its only assertion is contains("502"). sanitize_node_msg truncates to 200 chars regardless, so the error string is identical whether the read stops at 8 KB or buffers the full 100 KB -- reverting read_body_capped to resp.text() keeps it green. The real read bound is covered by sync.rs's read_body_capped_bounds_the_read; assert the actual cap here or drop the misleading name.

• [P3] Update the description and drop the dead registered: false rendering
  crates/gl/src/whoami.rs:106
  The PR title and body say it reports registered: false on a 404, but the code now bails on 404 and registered is only ever None or Some(true), so the human "no" branch and the json!(false) output are unreachable. That is the intended anti-fabrication behavior, not a defect, but the description contradicts the code and the dead branch is worth removing. Also minor: the transport-Err arm at :79 bails with an unsanitized {e} -- low risk (a reqwest error carries the dialed URL and source, not the node's body), noted for completeness.

Core error-handling logic is correct and the 404 design is a reasonable maintainer call; the two P2s are the blockers and both are small.

[jatmn]

Blocking findings

1. [P2] Capability strings from the success path are printed unsanitized

File: crates/gl/src/whoami.rs
Lines: 42–46 (extraction), 112 (display)

The 200-OK branch extracts capabilities from the node response and prints them directly:

println!("Caps:       {}", capabilities.join(", "));

None of the strings pass through sanitize_node_msg. A hostile or compromised node can therefore inject ANSI/OSC escape sequences, clickable hyperlinks, bidirectional format controls (e.g. U+202E), or extremely long strings into the user’s terminal. This is the same terminal-injection class (INV-6) that the PR defends against on the error path.

Why it matters: The PR adds node-output sanitization to whoami.rs; leaving the sibling success-path display sink unfiltered is inconsistent and exploitable. JSON output is safer because serde_json escapes C0/C1 controls, but bidi format controls are still emitted literally, and there is no length cap.

Suggested fix: Sanitize every capability before display, e.g.:

let capabilities: Vec<String> = caps
    .iter()
    .filter_map(|c| c.as_str().map(sanitize_node_msg))
    .collect();

and, if desired, apply the same treatment before serializing the JSON output.

---

2. [P2] The control-character sanitization test does not exercise real control bytes

File: crates/gl/src/whoami.rs
Lines: 337–367, specifically line 349

test_whoami_server_error_sanitizes_controls builds the mock body as:

.with_body("{\"message\":\"\\u{1b}[31mowned\\u{07}\\u{202e}evil\"}")

The source contains \\u{1b} (two backslashes), which becomes the literal six-character text \u{1b} in the request body, not the ESC control byte (U+001B). The same applies to \u{07} and \u{202e}. JSON does not recognize the brace-escape syntax, so serde_json::from_str fails; the code falls back to the raw body as the message. Because no real control characters are ever present, sanitize_node_msg is not actually tested on dangerous input, yet the test passes.

Why it matters: This test claims to guard INV-6 for the whoami error path, but a regression that broke sanitize_node_msg would not be caught. The analogous sync.rs test (trigger_sanitizes_control_chars_in_node_error) correctly uses the JSON escape form \u001b / \u0007, so serde decodes real control bytes before sanitization.

Suggested fix: Use valid JSON escapes (or literal control characters) so the mock body contains real ESC, BEL, and RLO bytes:

.with_body("{\"message\":\"\u{001b}[31mowned\u{0007}\u{202e}evil\"}")

and keep the existing assertions that those bytes do not leak into the formatted error.

---

Non-blocking but recommended findings

3. [P2 → P1 if intentional] PR description and code disagree on 404 behavior

File: crates/gl/src/whoami.rs
Lines: 55–59

The PR title and description say gl whoami should "only report registered: false on a genuine 404". The current checkout instead bails on every 404 with the agent.rs-style message:

agent not found, or this node does not yet support the agents API (v0.3+)
upgrade the node or check GITLAWB_NODE is pointing to the right server

Because the command errors out, registered is never Some(false), so the human "Registered: no" and JSON "registered": false output branches are unreachable. The single 404 test (test_whoami_with_node_not_registered) now asserts an error rather than a successful unregistered result.

The bail-on-404 choice is defensible: it matches gl agent show and avoids the false-registered verdict that an old/misrouted node would otherwise produce. But the PR summary and test name should be updated to reflect that. If the original registered: false behavior is still intended, the 404 branch needs to distinguish a genuine missing agent from an unsupported node before reporting success.

Suggested action: Either update the PR title/description and rename test_whoami_with_node_not_registered, or restore a registered = Some(false) success path with explicit endpoint-support verification.

---

4. [P3] Body-size-cap test does not prove the body is capped

File: crates/gl/src/whoami.rs
Lines: 307–334

test_whoami_server_error_caps_body_size only asserts that the error contains "502". The analogous sync.rs test also asserts s.len() < 500, which proves the surfaced message stays bounded end-to-end. Without that second assertion, an unbounded body read would still pass because sanitize_node_msg caps the displayed message at 200 characters.

Suggested fix: Add an assertion on the final error-message length, mirroring sync.rs.

---

5. [P3] Success-path body reads remain unbounded

File: crates/gl/src/whoami.rs
Lines: 39, 50

The 200-OK branch uses resp.json().await for the agent info and again for the repo count. Unlike the error path, there is no read_body_capped guard. This is pre-existing behavior, but it is now inconsistent with the carefully bounded error handling added by this PR.

Suggested fix: Either cap the success-path reads or document why unbounded 200 bodies are acceptable for whoami.

---

6. [P3] Test mocks are created but never asserted

File: crates/gl/src/whoami.rs
Lines: 167–184, 202–209, 232–239, 263–270, 315–321, 345–351, 378–395

Most whoami tests hold mockito server handles but do not call .assert(). Positive tests also do not capture or inspect stdout. A regression that short-circuited before calling the node, or that silently dropped a secondary request such as the repo-count lookup, would not fail the suite.

Suggested fix: Add .assert() to the relevant mock handles and assert on the printed output for at least the happy-path tests.

---

[beardthelion]

Read the diff against issue #220's actual fix direction and against the existing 404 conventions elsewhere in gl. The 403/500/transport half of the fix is correct and well-tested. The 404 arm inverts the fix's own stated intent.

Findings

• [P1] 404 now errors instead of setting registered:false, regressing the primary use case
  crates/gl/src/whoami.rs (404 match arm)
  Issue #220's fix direction: "distinguish a 404 (registered: false) from other non-2xx and transport errors, mirroring status::trust_line." The PR body's own "What changed" section says the same: "404 -> registered = Some(false)." The shipped 404 arm instead calls bail!(...) and never assigns registered. Before this PR, a 404 already produced registered: false (the bug was that every other non-2xx and transport error did too). After this PR, gl whoami --json for a genuinely unregistered identity errors out with no JSON instead of reporting {"registered": false} — that's the command's normal, everyday case, not an edge case. The rewritten test_whoami_with_node_not_registered makes this explicit: same 404 mock as before, but now asserts run(args).await.unwrap_err() with "expected ambiguous 404 error."
  There's a plausible reason this happened: crates/gl/src/agent.rs's cmd_show has a byte-identical bail-on-404 message, pre-existing on main. But that's a display command where a 404 genuinely has nothing to show; whoami is a status check where a negative answer is a valid result, and the issue explicitly asks it to follow status.rs's convention instead. Looks like the wrong sibling got mirrored.
  Fix: in the 404 arm, set registered = Some(false); instead of bail!, and revert test_whoami_with_node_not_registered to assert success with registered: false.

[coderabbitai]

Actionable comments posted: 1

🤖 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/whoami.rs`:
- Line 347: Update the sanitization fixture body in the relevant whoami test to
use JSON-valid Unicode escapes for the ESC, BEL, and bidi characters, rather
than Rust-style \u{...} escapes. Ensure the response is parsed as JSON so the
assertions exercise removal of the actual control and bidi characters instead of
relying on raw-text fallback.

🪄 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 Plus

Run ID: edeaf86d-cf99-461f-bef4-abb392ff299a

📥 Commits

Reviewing files that changed from the base of the PR and between 36eadc5 and cda114a.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

• crates/gl/src/sync.rs
• crates/gl/src/whoami.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/gl/src/sync.rs

Superseded by a fresh review on 596da41.

[beardthelion]

Re-reviewed 596da41 end to end against a mock node, not just the diff. The 404 fix is right this time: a 404 now yields Registered: no and {"registered": false} with exit 0, and the other arms surface the failure instead of fabricating a verdict. I also confirmed the control-character test became load-bearing (removing the sanitize_node_msg call turns it red), and that the new error arm emits zero control bytes for a 500 carrying ESC/BEL/RLO.

One blocking item is left, and it is the one I asked for two rounds ago.

Findings

• [P2] Sanitize node-supplied capabilities where they are extracted, not only on the error path
  crates/gl/src/whoami.rs:45
  A node returning 200 with capabilities: ["\u001b]0;PWNED\u0007repo:write", "\u202egnitirw-tfel"] puts those bytes straight on the terminal. I ran it: the Caps: line comes out as 033 ] 0 ; P W N E D \a ... followed by a raw 342 200 256, so a hostile --node/GITLAWB_NODE rewrites the terminal title and reverses the rendered line. --json is not a refuge either: serde escapes the C0 bytes but passes U+202E through raw. Sanitizing at the extraction site covers both output modes at once:

  capabilities = caps
      .iter()
      .filter_map(|c| c.as_str().map(sanitize_node_msg))
      .collect();

  I compiled that and re-ran both probes: zero control bytes and zero U+202E in human mode and in --json, with cargo test -p gl at 296 passed. The leak predates this PR, but this is the PR that teaches whoami.rs to sanitize node output, and the sibling display sink is two lines away.

Non-blocking, take or leave:

• [P3] test_whoami_server_error_caps_body_size still does not assert what its name claims
  crates/gl/src/whoami.rs:328
  I swapped read_body_capped(resp, 8 * 1024) for an unbounded resp.text() and the suite stayed green, that test included. sanitize_node_msg truncates to 200 chars, so display.len() < 1000 holds whether or not the read cap fires. The read bound is genuinely covered by sync.rs's read_body_capped_bounds_the_read, so either rename this one to what it actually pins (the displayed message stays bounded) or drop it.

• [P3] Tighten the error-message extraction for two shapes I hit
  crates/gl/src/whoami.rs:64
.get("message").or_else(|| v.get("error")) chains on key presence, not on string-ness, so a body of {"message":null,"error":"boom"} prints the whole raw JSON instead of boom. Chaining .and_then(|m| m.as_str()) into each side fixes it. Separately, an empty body on a 502 renders as agent lookup failed (502 Bad Gateway): with a dangling colon; worth bailing without the suffix when the sanitized message is empty. The same or_else shape exists in sync.rs:53.

• [P3] The 2xx side still fabricates a verdict, which is the other half of #220
  crates/gl/src/whoami.rs:39
is_success() plus resp.json().unwrap_or_default() means a 204, or a 200 carrying a proxy's HTML error page, prints "registered": true. I confirmed both. That is the misrouted-node case from the other direction, and it is pre-existing on main, so I am not asking you to fix it here. Flagging it because this PR is what makes the asymmetry visible.

• [P3] Drop the Cargo.lock hunk
  Cargo.lock
  It moves every workspace crate to 0.6.0 while touching no manifest, and main's crates are already at 0.7.0, so it lands stale on merge. Nothing in CI builds with --locked, so it is not buying anything.

The core is sound and the error handling reads well. Fix the capability sanitization and I will approve.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Rebase and regenerate the stale lockfile
  Cargo.lock:3303
  The branch predates the 0.7.0 release, so this hunk changes all five workspace package entries to 0.6.0 while current main keeps the corresponding manifests at 0.7.0. The actual merge tree retains those 0.7.0 manifests but applies these 0.6.0 lock entries; cargo check --locked -p gl then fails because Cargo needs to update Cargo.lock. Please rebase and regenerate the lockfile (or drop this unrelated hunk) so lockfile-enforcing CI and release builds can run.

[jatmn]

LGTM

[beardthelion]

Re-reviewed 559703d against a hostile mock node. The capability sanitization I asked for two rounds ago is in and it works: a node returning an ESC ] 0 ; PWNED BEL sequence and a U+202E override in capabilities now produces zero control bytes and zero U+202E in human mode and in --json, with benign capabilities untouched. The 404 split and the bounded, sanitized error arm are right. One thing keeps this from an approval.

Findings

• [P2] Ship a regression test for the capability sanitization
  crates/gl/src/whoami.rs:45
  I dropped .map(sanitize_node_msg) from that line and cargo test -p gl came back 304 passed, 0 failed. The only two tests that touch capabilities feed git:push and git:pull, so nothing in the suite can see the escape. Mirror test_whoami_server_error_sanitizes_controls, which does go red when its target is removed: serve a 200 whose capabilities carry an ESC/BEL sequence and a U+202E, and assert the output is clean. Give --json its own arm, because serde escapes the C0 bytes on its own but passes U+202E through raw, so a C0-only assertion would pass while the bidi override still ships.

• [P3] Drop the Cargo.lock hunk, and disregard the P1 framing on it
  Cargo.lock:3303
  I ran cargo metadata --locked on both trees: it exits 101 on current main (manifests at 0.7.0, lockfile still at 0.5.1) and 101 on this branch. The hunk neither causes nor fixes a lockfile-enforcing build, so it is not a merge blocker. It is still unrelated churn, and it moves five entries to 0.6.0 while this branch's own crates/gl/Cargo.toml says 0.7.0. The real repair is #243; just drop the hunk here.

• [P3] Sanitize the node URL in the new transport arm
  crates/gl/src/whoami.rs:76
bail!("agent lookup failed: {e}") renders the URL the client built, and I confirmed an ESC/BEL/U+202E in GITLAWB_NODE reaches the terminal intact. Lower severity than the node-response paths, since the value comes from the caller's own flag or environment rather than off the wire, but this arm is new here (main swallowed the error) and the sanitizer is already imported two lines up.

• [P3] test_whoami_server_error_caps_body_size still does not pin the read cap
  crates/gl/src/whoami.rs:299
  Same as last round: sanitize_node_msg truncates to 200 chars, so display.len() < 1000 holds with or without the read bound. sync.rs's read_body_capped_bounds_the_read is what actually covers the mechanism. Rename this to what it pins or drop it.

The 200-arm behavior (a 204 or an HTML proxy page still printing registered: true, and the two unbounded body reads) is byte-identical to main, so it stays out of scope here as before.

Superseded by my review on 559703d; dismissing so the state reflects the current head.

[jatmn]

LGTM on ab59d56. The capability-regression gap from the last round is closed, the 404 versus other-error split matches issue #220, error bodies are capped and sanitized on the new failure arms, and cargo test -p gl whoami passes locally (11 tests). I do not see merge-blocking defects on the current head.

Optional polish if you want to tighten things further — none of these block merge:

• Transport-error sanitization (consistency): crates/gl/src/whoami.rs:80-81 — HTTP error bodies go through sanitize_node_msg, but transport failures still bail with raw {e}, which can include the dialed URL from NodeClient::get (crates/gl/src/http.rs:45). Low risk because the URL comes from the caller’s own --node / GITLAWB_NODE, not the wire; still a small inconsistency in a file that now sanitizes other node-facing output.

• 404 test output assertion (coverage): crates/gl/src/whoami.rs:197-218 — test_whoami_with_node_not_registered only checks success, not Registered: no / "registered": false. The implementation is correct; this is pre-existing test style, not a product bug. Asserting output via run_to_writer would better pin the core claim if you are touching tests anyway.

Superseded by a re-review of ab59d56.

[beardthelion]

Re-reviewed ab59d56 against a mock node and against mutations of the production lines. The capability regression test I asked for last round is in and it is genuinely load-bearing: dropping .map(sanitize_node_msg) turns it RED on the ESC/BEL assertion, and a C0-only sanitizer (removing the bidi filter) turns it RED on the U+202E assertion in both output modes. The 404 split works end to end too, with Registered: no and {"registered": false} at exit 0 on a 404 mock and a hard error at exit 1 on a 403. One thing keeps this from an approval, and it is small.

Findings

• [P2] Assert the printed verdict in test_whoami_with_node_not_registered
crates/gl/src/whoami.rs:218
  The test ends at run(args).await.unwrap() with no assertion on output, so it cannot tell registered = Some(false) from registered = None. I replaced the whole 404 arm body with {} and the full suite came back 305 passed, 0 failed. That leaves the behavior issue #220 is actually about with zero coverage, on the one arm of this PR that already went the wrong way once mid-review. The run_to_writer seam you added makes this two lines: capture into a Vec<u8>, assert out.contains("Registered: no"), then repeat with json: true and assert "registered": false. I ran exactly that, GREEN on the current code and RED with the arm gutted.

The rest I would take but will not block on:

• [P3] Keep the transport error's cause chain
  crates/gl/src/whoami.rs:81
bail!("agent lookup failed: {e}") builds a fresh error from the Display of e, which prints only anyhow's outermost context, and NodeClient::get set that to GET {url}. A dead port and an unresolvable host both render as agent lookup failed: GET http://.../api/v1/agents/did:key:... and nothing else. Surfacing the failure instead of fabricating a verdict is half this PR; saying what failed is the other half. return Err(e).context("agent lookup failed") gets there: I compiled it and the two cases separate into tcp connect error / Connection refused (os error 111) and dns error / failed to lookup address information.

• [P3] Take the stdout lock after the network phase
  crates/gl/src/whoami.rs:26
run locks stdout before load_keypair_from_dir and holds it across both HTTP round trips, though nothing is written until the render block. StdoutLock is !Send, so the run future is now !Send as well: a fn assert_send<T: Send>(_: T) {} probe over run(args) fails to compile with "future is not Send as this value is used across an await". It builds today only because main.rs:183 awaits it directly. Locking inside run_to_writer after the match, or passing std::io::stdout() unlocked, costs nothing and drops both the stall and the constraint.

• [P3] Rename or drop test_whoami_server_error_caps_body_size
crates/gl/src/whoami.rs:304
  Third round on this one. I swapped read_body_capped(resp, 8 * 1024) for an unbounded resp.text() and the suite stayed green, that test included, because sanitize_node_msg truncates to 200 chars either way. sync.rs's read_body_capped_bounds_the_read is what covers the mechanism.

• [P3] Sanitize the node URL in the transport arm
  crates/gl/src/whoami.rs:81
  Unchanged from last round: --node $'http://127.0.0.1:1/\x1b]0;PWNED\x07‮gnitirw' puts the ESC, BEL and U+202E on the terminal intact. The value comes from the caller's own flag or environment rather than off the wire, so it is the lowest-risk sink in the file, but the sanitizer is imported two lines up and this arm is new here.

Withdrawing the Cargo.lock ask from last round: it was right when the hunk said 0.6.0 and wrong now. cargo metadata --locked exits 0 on this branch and 101 on current main, whose lockfile still records 0.5.1 against 0.7.0 manifests, so the hunk moves the lockfile toward the manifests rather than away. Leave it. It will conflict with #243, which touches the same file, and #186 and #261 both move crates/gl/src/sync.rs, so expect a rebase either way.

The 2xx path (an unbounded resp.json() and a repos lookup with no status check) is byte-identical to main, so it stays out of scope here as before.