fix(gl): report quickstart registration failure when the node returns no UCAN (#355)

[beardthelion]

Summary

gl quickstart printed "Registered successfully" and "UCAN saved to ..." on any 2xx from /api/register, including a non-JSON body where resp.json() falls back to Value::Null. Nothing was written, and a stale ucan.json from another node stayed in place while the output claimed success.

Motivation & context

Closes #355

Kind of change

• Bug fix
• Feature
• Security fix
• Docs
• Tests / CI
• Refactor (no behavior change)
• Breaking or protocol change (issue required first)

What changed

• crates/gl/src/quickstart.rs: the success lines moved inside the !ucan.is_empty() guard that already covered the write. A 2xx with no usable ucan now prints a failure row with a retry hint instead.

How a reviewer can verify

Ran the built binary against a stub node returning 200 text/html on /api/register:

• before: Registered successfully / UCAN saved to <path> with no ucan.json written
• after: Registration returned no UCAN (unexpected response body), still no ucan.json, exit continues to step 3 as before

cargo test -p gl

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test -p gl passes locally
• New behavior is covered by tests where a seam exists; the only observable change here is the printed line, which the binary run above verifies directly
• cargo fmt --all and cargo clippy -p gl --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
• Docs / .env.example updated if behavior or config changed (or N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

• Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
• Discussed in an issue before implementation
• Backward-compatible with existing nodes and previously signed history

Notes for reviewers

Overlaps the #354 fix PR in quickstart.rs's register arm: whichever merges second needs a small rebase.

Summary by CodeRabbit

• Bug Fixes
  • Improved gl quickstart handling when registration succeeds without a usable UCAN.
  • Displays a failure message and retry command instead of reporting success.
  • Shows trust score and UCAN-saved messages only when a valid UCAN is received.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Essentials

Run ID: 3a79727d-4365-467e-805b-28756a8a7142

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44f9 and a418c28.

📒 Files selected for processing (1)

• crates/gl/src/quickstart.rs

Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.

---

📝 Walkthrough

Walkthrough

gl quickstart now treats a registration response without a non-empty UCAN as unexpected. It reports failure and provides a retry command. Trust and success messages appear only when a usable UCAN is returned.

Changes

Quickstart registration

Layer / File(s)	Summary
Registration response validation crates/gl/src/quickstart.rs	Success output, trust score output, and UCAN path output now require a non-empty UCAN. Missing UCAN responses report an unexpected response and provide a retry command.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to a418c

Quickstart now reports registration failure for unusable responses while preserving the existing continuation behavior, with no identified merge-blocking impact.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Issue #355 requires quickstart to report registration failure when a 2xx response has no usable UCAN and to avoid claiming that a token was saved. quickstart.rs now writes the record before printing…	When a 2xx registration response has no usable UCAN, remove or invalidate an existing ucan.json for another node, or otherwise prevent later commands from consuming it. Add an automated regression test for this stale-token case, in additi…
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fix: gl quickstart must report registration failure when the node returns no UCAN. It is specific and relevant to the main change.
Description check	✅ Passed	The description covers the summary, motivation, bug-fix classification, concrete changes, verification steps, test results, review checklist, compatibility, and reviewer notes. It provides sufficient …
Out of Scope Changes check	✅ Passed	The reviewed change is limited to the registration response handling in crates/gl/src/quickstart.rs. It changes success output, trust-score output, and retry failure output that directly implement i…

Full details: Linked Issues check

Explanation

Issue #355 requires quickstart to report registration failure when a 2xx response has no usable UCAN and to avoid claiming that a token was saved. quickstart.rs now writes the record before printing success, gates the trust score and success messages on a non-empty UCAN, prints a failure with a retry command, and continues to step 3. However, when ucan.json already belongs to another node, the no-UCAN branch leaves that stale file in place. Later commands can still use the stale token, so the misleading registration state remains. The summary reports stub verification and cargo test -p gl; no separate automated regression test is shown in the reviewed changes.

Resolution

When a 2xx registration response has no usable UCAN, remove or invalidate an existing ucan.json for another node, or otherwise prevent later commands from consuming it. Add an automated regression test for this stale-token case, in addition to the no-UCAN response cases.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[greptile-apps]

Greptile Summary

This PR corrects gl quickstart output for successful HTTP responses that contain no usable UCAN.

• Success and saved-path messages are now emitted only after a non-empty UCAN is persisted.
• Empty, missing, or non-JSON UCAN responses now produce a registration-failure message and retry guidance.

Confidence Score: 5/5

The PR appears safe to merge and correctly prevents a no-UCAN response from being reported as successful registration.

The new branch accurately reflects whether a usable UCAN was saved and preserves the command’s established behavior of continuing after registration failures; no actionable regression was identified.

Important Files Changed

Filename	Overview
crates/gl/src/quickstart.rs	Aligns registration status output with the existing UCAN persistence guard while preserving quickstart’s intentionally non-fatal control flow.

_{Reviews (1): Last reviewed commit: "fix(gl): report quickstart registration ..." | Re-trigger Greptile}

[jatmn]

I found one issue to address before this is ready.

Merge readiness

GitHub reports this head as mergeable, but review is required and cargo audit failed. The diff has no dependency or lockfile change, and the same audit job failed on PR #459 from the same main base, so the failure appears unrelated to this patch; its exact advisory log was unavailable through GitHub CLI. Resolve or explicitly triage that check before merge. Current main is still bfc44f9, the captured merge base, so this branch does not currently need a rebase. PR #459 changes the same quickstart registration hunk to secure secret-file writes; if it merges first, resolve the overlap and recheck the combined behavior.

Findings

🔵 P3 — Do not direct the retry to a different identity

📍 Where: crates/gl/src/quickstart.rs:120, the new no-UCAN failure branch.

💥 What fails: Run gl quickstart --dir /custom/identity --node N against a node that returns 2xx without a UCAN. The new instruction says gl register --node N. Following it makes gl register read ~/.gitlawb/identity.pem instead of /custom/identity/identity.pem. The retry fails before contacting the node when the default identity is absent; if one exists, it can register a different DID and save its UCAN in the wrong directory. A local binary stub reproduced the new instruction with a custom --dir and no UCAN write.

🔎 Root cause: The new recovery instruction carries the selected node but drops the identity directory that quickstart used to create and send the registration request. register::run has its own --dir option and otherwise loads the default identity.

📜 Stated contract:

The new operator output says “You can retry with: gl register --node …”; both commands document --dir as “Identity directory (default: ~/.gitlawb).”

🏷️ Attribution: PR-introduced on this response path. At merge base and live main (bfc44f9), a 2xx without UCAN printed false success and no retry command. At head (a418c28), the newly added retry command omits --dir. The older non-2xx and transport hints have the same omission, but their output is unchanged by this PR.

📌 In this PR:

• The no-UCAN branch at line 120 — omits the selected identity directory. There are no other changed retry hints, docs, or generated operator surfaces in this diff.

🔒 Unchanged on main: The old quickstart error hints, gl register identity loading, and the other-node UCAN record behavior are outside this finding.

🔧 Required correction: Remove this new retry instruction, or make it preserve quickstart's selected --dir if the hint remains. If printed, the directory must be one shell argument, including paths with spaces. Add focused regression evidence that a no-UCAN response with a custom directory reports failure without directing the user to a different identity. Keep the existing nonfatal continuation to repo creation.

🛠️ Author fix: Close the false recovery instruction on the one in-diff branch and cover that behavior. The accepted issue requires a failure warning; it does not require a retry command. Do not rebuild gl register, the shared UCAN decoder, or unrelated quickstart failure paths for this patch.

🚫 Out of scope: Rewriting the pre-existing gl register no-UCAN behavior, clearing a valid UCAN for another node, or making registration failure fatal.

Validation

cargo fmt --all -- --check passed. cargo test -p gl --locked passed 363 tests; none exercises quickstart's new output branch. A local stub returned non-JSON 200 for registration: quickstart printed registration failure, printed the incorrect retry instruction with a custom --dir, wrote no UCAN, and exited 0 through its established nonfatal flow. Four blind full-diff searches and a separate coverage-gap search were reconciled against the current head, merge base, and live target.

[euxaristia]

Faithful to #355: the success prints moved inside the non-empty guard with a failure row and retry hint. No automated test, but the changed seam is print-only and verified by binary run; acceptable.