fix(gl): sign the visibility-gated client reads so a repo owner can read their own private repo (#115)

[beardthelion]

Stacked on #186. Base is fix/issue-123-client-read-status-check, so this diff is the signing change alone. Draft until #186 lands.

Summary

Visibility-gated gl and MCP read commands sent unsigned requests, so a private repo's own owner was told it had no pull requests and no commits. They now present the caller's identity.

Motivation & context

Closes #115

gl pr list/view/diff/comments and gl repo commits loaded the caller's keypair, used it only to derive the owner segment, then built NodeClient with None and sent an unsigned GET. Those routes are gated by authorize_repo_read, which denies an anonymous caller with a 404. The client then parsed the error body without checking status, so the denial rendered as an empty list. The six MCP read arms had the same shape while already holding the keypair.

#186 fixes the rendering half. This fixes the identity half, and it has to sit on top rather than beside: these call sites sent no signature at all before, so the node's require_signature was unreachable for them. Now that they always sign, a client whose clock is more than 300s off gets a 400 clock_skew, and without a status check above the parse that renders as an empty list with exit 0. A public repo the user could always read would report itself empty. Merging the signing first would trade one silent wrong answer for another, on a larger population. The reverse order matters too: #186 rewrites these same expressions and its replacement keeps client.get(, so if it merged after this branch it would silently revert the signing.

Kind of change

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

What changed

Crate touched: gl.

• crates/gl/src/pr.rs: cmd_list, cmd_view, cmd_diff and cmd_comments pass Some(keypair) to NodeClient and call get_maybe_signed. The keypair was already loaded there with ?, so it was always available.
• crates/gl/src/repo.rs: cmd_commits builds its client with load_keypair_from_dir(dir.as_deref()).ok() and calls get_maybe_signed, so owner/name against a public repo still works with no identity on disk.
• crates/gl/src/mcp.rs: repo_get, repo_commits, pr_list, pr_view (both fetches) and pr_diff switch to get_maybe_signed. That client already held the keypair.
• Tests: the existing happy-path mocks now assert signature and signature-input, following protect.rs; cmd_diff gains its first test; a new test pins that the anonymous public-repo path sends no signature headers.

get_maybe_signed over get_signed is deliberate: these routes read anonymously for public repos, and get_signed would turn a working identity-less public read into an error.

How a reviewer can verify

cargo test -p gl --bin gl

# The signing is load-bearing, not decoration. Revert one site and watch it fail:
#   crates/gl/src/pr.rs, in cmd_list, change
#     NodeClient::new(&node, Some(keypair))  ->  NodeClient::new(&node, None)
#     client.get_maybe_signed(...)           ->  client.get(...)
cargo test -p gl --bin gl -- pr::tests
# 2 tests fail: test_cmd_list_no_prs, test_cmd_list_with_prs

# The anonymous path is pinned in the other direction. Make the keypair mandatory:
#   crates/gl/src/repo.rs, in cmd_commits, change
#     load_keypair_from_dir(dir.as_deref()).ok()
#     -> Some(load_keypair_from_dir(dir.as_deref())?)
cargo test -p gl --bin gl test_cmd_commits_anonymous_when_no_identity
# fails

Node side, traced to confirm this does more than change which error you get: optional_signature injects the DID, each handler lifts it into caller, authorize_repo_read passes it to visibility_check, and the owner is allowed on its first branch. The short key in the path versus the full DID in the signature keyid is absorbed by did_matches.

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally (via the pre-push hook, which runs it; cargo test -p gl --bin gl is 460 passed on this branch)
• 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 (feat(...), fix(...), docs(...))
• Docs / .env.example updated if behavior or config changed (or N/A). N/A: no config or documented behavior changes.
• Checked existing PRs so this isn't a duplicate (fix(gl): status-check the client read/write surfaces so a node denial surfaces as an error, not a fake result (#123) #186 is the adjacent one, and it is the base of this branch)

Protocol & signing impact

• Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
• Discussed in an issue before implementation (gl/MCP read commands send unsigned requests to visibility-gated endpoints, breaking them for private-repo owners #115, filed 2026-06-28)
• Backward-compatible with existing nodes and previously signed history

These routes already accept a signature through optional_signature, so an updated client talking to an older node is unaffected; the node treats the added headers exactly as it does for the routes gl already signed. No wire format changes, no new component in the signing base, no change to how anything is verified.

Notes for reviewers

Known gaps, stated rather than left for the next reader:

• The MCP arms have no signing test, and reverting all six leaves the suite green.
• MCP owner/repo arguments are interpolated into the path unvalidated. Now that these arms sign, an argument containing a literal / can aim a signed request at another owner-gated route under /api/v1/repos/. Worth fixing before the MCP half is relied on.
• The signed path is not percent-encoded, so a non-ASCII --branch, or a repo argument containing a space, signs a string the node cannot reproduce. The seam is in get_maybe_signed, not in any one call site.
• .ok() cannot distinguish a missing identity from an unreadable one, so a corrupt identity.pem silently downgrades to anonymous.
• repo_list, repo_tree, git_refs and gl status remain unsigned against the same class of route.
• This extends Signed requests are replayable for ~600s and are not bound to a target host #253 from the write path to the read path: signatures cover @method, @path and content-digest but not @authority, so a signature handed to a hostile --node is replayable elsewhere within the 300s window.
• A quarantined repo still 404s its own owner; signing cannot help there.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

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

[jatmn]

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

The signing hunks themselves look correct for the primary #115 paths: CLI PR commands derive owner from the loaded keypair and now call get_maybe_signed, and cmd_commits preserves the anonymous public-repo path via load_keypair_from_dir(..).ok(). The items below are what still block merge-ready status.

Merge readiness

• [P1] Rebase onto the current #186 branch tip and resolve the pr.rs conflict deliberately
  crates/gl/src/pr.rs
  GitHub still reports this draft PR as CONFLICTING / DIRTY. The three-dot merge-base is fba90038, but the live #186 tip is 31e78f60 (443 lines ahead, including more pr.rs tests and cmd_view sub-fetch degradation). A naive conflict resolution that keeps this branch’s older pr.rs would drop #186 behavior even though the signing diff itself is fine against its recorded base.

  Root cause: stacked branch fell behind its declared base while both PRs edited the same file.

  What to do:

  1. Wait for or rebase onto current fix/issue-123-client-read-status-check (31e78f60).
  2. In the conflict, keep both layers:
     • #186: read_json status-before-parse (already on your base) and live-tip cmd_view soft-fail for denied reviews/comments sub-fetches (match blocks + cmd_view_continues_when_* tests).
     • #392: NodeClient::new(..., Some(keypair)) / get_maybe_signed on the gated read call sites.
  3. Re-run cargo test -p gl --bin gl pr::tests on the resolved tree, not the pre-rebase head.
  4. Push and confirm GitHub is no longer CONFLICTING.

• [P1] Preserve the #186 → #392 landing order onto main
  PR body / commit message
  This branch is draft until #186 lands. The signing change is only safe relative to #186 because these call sites already route through read_json; without that layer, clock-skew and other non-2xx bodies can still render as empty success on signed reads.

  Root cause: #115 (identity) and #123/#186 (status-check) fix different halves of the same failure; this PR depends on the second half being on main first.

  What to do: merge #186 first, then rebase this branch onto main (or onto the post-#186 base) and re-review the resolved diff before undrafting.

Findings

• [P2] Restore the removed gl pr list / gl pr view denial regression tests
  crates/gl/src/pr.rs (tests section, formerly cmd_list_surfaces_denial_not_empty and cmd_view_surfaces_denial_not_stub)
  Relative to merge-base fba90038, this PR deletes two #123 guards that were added on the stacked #186 base. Production cmd_list and cmd_view still call read_json today, so behavior may still be correct — but a future revert to unsigned client.get or parse-before-status on just those two commands would no longer fail CI. Only cmd_diff / cmd_comments denial tests remain on head.

  Root cause: test-module edit churn while adding signature header mocks and test_cmd_diff_signs_request; the deletions look accidental, not an intentional scope change (production handlers were not simplified).

  What to do:

  1. Restore both tests from the stacked base verbatim, then update their mocks for the new contract:
     • keep identity via write_identity(&dir);
     • add .match_header("signature", mockito::Matcher::Any) and .match_header("signature-input", mockito::Matcher::Any) because these commands now always sign when identity is present;
     • keep the 404 body and assert!(result.is_err(), …) plus _m.assert_async() so the test still proves the mocked route was hit.
  2. Confirm revert-check: temporarily change cmd_list back to NodeClient::new(&node, None) + client.get and verify these two tests fail.

• [P2] Align MCP owner default with the signing keypair on the six changed read tools (or require owner explicitly)
  crates/gl/src/mcp.rs:695-897, crates/gl/src/mcp.rs:1313-1319; contrast webhook_list at mcp.rs:971-985 and test_webhook_list_default_owner_is_keypair_not_node_did
  This PR correctly switches repo_get, repo_commits, pr_list, pr_view, and pr_diff to get_maybe_signed, which fixes #115 when the caller supplies a matching owner (all current MCP tests do). However, those six arms still default the path owner via resolve_owner(), which falls back to the node root DID from GET /, while call_tool signs with the caller's keypair. That mismatch predates this PR for unsigned calls, but signing makes it sharper: a signed request can hit /api/v1/repos/{node_short}/… with a signature whose keyid is the user's DID. The same file already documents and fixes the opposite pattern for webhook_list — when owner is omitted on an owner-gated signed route, default to the keypair short DID, not the node DID.

  Root cause: owner-gated MCP reads need the path owner segment and the signer identity to refer to the same principal; resolve_owner() was written for node-scoped defaults before these routes were signed.

  What to do (pick one consistent approach):

  • Recommended (matches webhook_list): for the six changed tools only, when args["owner"] is absent and a keypair is present, default owner to the keypair short DID; keep resolve_owner() for unsigned / no-identity paths. Add a test mirroring test_webhook_list_default_owner_is_keypair_not_node_did for at least repo_get or pr_list.
  • Alternative: make owner required in the MCP schema for these tools and fail client-side with a clear error when omitted; document that private-repo reads must pass owner explicitly. Test both the required-owner error and the signed happy path.

  Do not change get_maybe_signed itself or the anonymous public-repo behavior in cmd_commits.

• [P2] Add signing regression tests for the six MCP read arms
  crates/gl/src/mcp.rs:695-897; pattern at test_webhook_list_via_mcp_signs_the_request (mcp.rs:1911-1943)
  You noted in the PR body that reverting all six MCP hunks leaves the suite green. That is accurate: existing MCP denial tests call with dir: None, so they exercise the unsigned path only. CLI/repo tests in this PR added signature header guards; MCP did not.

  Root cause: mutation tests were added for CLI/repo signing in this PR but not extended to the MCP surface, even though MCP is part of the #115 claim.

  What to do:

  1. For each of repo_get, repo_commits, pr_list, pr_view (both GETs if feasible), and pr_diff, add one happy-path test with a temp identity dir and mocks that require signature + signature-input, following test_webhook_list_via_mcp_signs_the_request.
  2. Optionally add one combined revert-check script comment in the PR (like the existing cmd_list revert instructions) showing that reverting one MCP arm fails its new test.
  3. Keep the existing unsigned denial tests — they still validate #186 error surfacing when no identity is present.

• [P3] Tighten test_cmd_comments_with_results to assert signing
  crates/gl/src/pr.rs:820-845
  In the same PR edit, test_cmd_comments_empty and the list/view happy-path mocks gained signature / signature-input matchers, but test_cmd_comments_with_results did not. Because cmd_comments now uses NodeClient::new(..., Some(keypair)) and get_maybe_signed, an unsigned regression would still pass this test.

  Root cause: incomplete application of the new signing test convention across sibling tests in one module.

  What to do: copy the two .match_header(...) lines from test_cmd_comments_empty onto this mock; no production change needed.

• [P3] Restore test_cmd_merge_success or drop the deletion from this PR
  crates/gl/src/pr.rs (tests section)
  Merge-base includes test_cmd_merge_success; head deletes it while cmd_merge production code is untouched. This is unrelated to #115 signing.

  Root cause: same test-section churn as the denial-test deletions — coverage removed without a corresponding production change.

  What to do: restore the test from merge-base unchanged, or if there is a deliberate reason to remove it, say so in the PR body and point to replacement coverage elsewhere.

Notes on items intentionally not raised as blockers

These were considered and left out of the findings list because they are pre-existing, author-documented, or out of this PR's scoped intent:

• MCP path injection / percent-encoding gaps (PR body "Known gaps")
• .ok() on cmd_commits identity load conflating missing vs corrupt PEM (documented; unchanged by signing hunk)
• Unsigned sibling routes (repo_list, repo_tree, git_refs, etc.) — explicitly out of scope
• @authority replay on read signatures — pre-existing http_sig.rs seam, noted in protocol section