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 reporting with clearer messages for unsupported APIs, permission errors, server failures, and connection issues.
  • Error responses now safely handle oversized response bodies and sanitize unsafe characters.
  • Added coverage for common registration failure scenarios.

[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: 1 minute

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: ac7815bf-5fcc-464b-a94a-d59495be788a

📥 Commits

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

⛔ 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 unsupported API responses, HTTP failures, and transport errors. Error bodies are bounded and sanitized using 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	Adds explicit handling for 404, other HTTP failures, and transport errors, including bounded and sanitized response messages, with tests for each behavior.

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

Possibly related PRs

• Gitlawb/node#161: Introduces related sync error-handling helpers used by this change.
• Gitlawb/node#186: Contains related sync helper visibility and status-error handling changes.

Suggested labels: subsystem:identity, subsystem:api

Suggested reviewers: jatmn

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The pub(crate) visibility changes in crates/gl/src/sync.rs are unrelated to the whoami bug fix.	Move the sync.rs visibility edits to a separate PR or explain them in the issue scope if they are required.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main whoami fix and matches the changed behavior.
Description check	✅ Passed	The description follows the template and includes summary, motivation, change details, verification, and checklist items.
Linked Issues check	✅ Passed	The whoami changes and tests satisfy #220 by mapping only 404 to registered:false and surfacing other failures.
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.