fix(node): fail closed when a recipient DID can't be resolved (#47)

[beardthelion]

Summary

encrypted_pin::encrypt_and_pin sealed a withheld blob to whatever subset of its recipient DIDs resolved locally and recorded the full intended set as covered, permanently locking out any reader whose DID could not be resolved at seal time. This makes sealing fail closed: resolve all recipients or skip the blob.

Motivation & context

Closes #47

did_to_key only resolves did:key; did:web/did:gitlawb (the canonical identity actors migrate to once DHT anchoring is live) and any malformed did:key return None. The old filter_map dropped those silently and sealed to the rest as long as one key resolved, then recorded the full DID set. Because the dedup compares the recorded set against the desired set, it matched on the next push and never re-sealed, so the dropped reader stayed locked out forever with no signal.

Kind of change

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

What changed

gitlawb-node (crates/gitlawb-node/src/encrypted_pin.rs):

• Add resolve_all_recipients: resolves every recipient DID all-or-nothing, returning Err(unresolved) listing the DIDs that failed. encrypt_and_pin now skips the blob on any failure instead of sealing to a partial set. Nothing partial is recorded, so the dedup stays honest and the blob re-seals on a later push once resolution is available (no automatic backfill).
• Bounded per-blob warn (the unresolved DIDs are user-controlled rule data; log a count plus a small sample, not an unbounded dump) with wording that stays neutral about the cause (a malformed did:key is not DHT-pending).
• One aggregate coverage warn per call, so a fully-migrated did:gitlawb org dropping to zero encrypted-pin coverage is a single greppable line rather than a per-oid scrape.
• Give the previously silent read_object and pin_git_object failure arms warn logs.

Fail-closed is deliberate: a blob with an unresolvable reader gets no encrypted recovery envelope until resolution exists, but it is never leaked in plaintext (it was already pinned as withheld upstream). This trades recovery coverage during a transition for a clean audit invariant: a recorded encrypted_blobs row always means every authorized reader can recover the blob.

How a reviewer can verify

cargo test -p gitlawb-node encrypted_pin
cargo test --workspace
cargo clippy --workspace --all-targets -- -D warnings

The core regression is mixed_set_with_did_gitlawb_fails_closed: a resolvable did:key subset plus one did:gitlawb must return Err naming only the unresolvable DID, never a sealable key set. malformed_did_key_fails_closed covers the corrupt-did:key case.

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 (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

This reads DID strings to resolve recipient keys but changes no signature, wire-format, or identity logic; existing envelopes and recorded recipient sets are untouched.

Notes for reviewers

The peer-sync path (sync.rs records an empty recipient set for replicated envelopes via an overwriting upsert) interacts with the dedup but self-heals on the next push; it is pre-existing and out of scope here. A typed resolution-failure reason (distinguishing "method not resolvable yet" from "malformed did:key") is a possible follow-up; fail-closed behavior is identical for both today.

Summary by CodeRabbit

• Improvements

  • Strengthened recipient handling to ensure all recipients are resolvable before proceeding, preventing partial processing when any recipient is invalid or unresolvable.
  • Improved behavior and messaging around skipped items, distinguishing between empty recipient sets, unresolvable recipients, and already-up-to-date recipients.
  • Refined pinning outcomes to better differentiate “no result” vs actual failures.

• Tests

  • Expanded coverage for recipient validation edge cases and decision paths to confirm fail-closed behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bdb08cd5-1d0a-4fe0-b3f4-5e6d3fe552d7

📥 Commits

Reviewing files that changed from the base of the PR and between eafdd87 and ca53afd.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/encrypted_pin.rs

---

📝 Walkthrough

Walkthrough

encrypt_and_pin transitions from fail-open partial sealing (via filter_map) to fail-closed resolution. A new resolve_all_recipients helper returns all keys or the unresolved DID set. A new SealPlan enum and plan_seal pure function make sealing decisions: seal only when all DIDs resolve, or skip with explicit variants for empty, unchanged, and unresolvable cases. The main loop increments skipped_unresolvable with bounded logging, refines pin error observability, and emits an aggregate warning. Unit tests validate all resolution and decision paths.

Changes

Fail-closed recipient resolution and sealing decisions

Layer / File(s)	Summary
resolve_all_recipients DID resolution helper crates/gitlawb-node/src/encrypted_pin.rs	Adds resolve_all_recipients(dids) returning Ok(Vec<VerifyingKey>) if all DIDs resolve, or Err(Vec<unresolved_dids>) if any fail. Treats did:web, did:gitlawb, and malformed did:key as unresolvable.
SealPlan decision boundary and plan_seal function crates/gitlawb-node/src/encrypted_pin.rs	Introduces SealPlan enum with variants Seal { keys, tag }, SkipNoRecipients, SkipUnresolvable(unresolved), and SkipUnchanged. Function plan_seal(node_seed, dids, stored_tag) gates sealing only when all recipients resolve; returns unresolved DIDs for fail-closed handling.
encrypt_and_pin refactor with fail-closed planning crates/gitlawb-node/src/encrypted_pin.rs	Computes stored recipients-tag, calls plan_seal to decide whether to seal. Increments skipped_unresolvable counter with bounded sampled logging when any DID is unresolvable. Refines pin_git_object handling to warn separately on "no CID returned" vs. "pin failed" outcomes. Emits an aggregate warning after the loop when any blobs were skipped due to unresolvable recipients.
Unit tests for all resolution and decision paths crates/gitlawb-node/src/encrypted_pin.rs	Adds tests for resolve_all_recipients covering all-resolvable, mixed resolvable/unresolvable (fail-closed), did:web, malformed did:key, empty DID set, and single-unresolvable-DID cases. Adds direct tests for plan_seal validating seal-on-all-resolve, fail-closed-on-any-unresolvable, skip-on-empty, skip-on-unchanged, and reseal-on-tag-change decisions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Gitlawb/node#40: Both PRs directly modify encrypt_and_pin's recipient DID handling and sealing workflow, including decision logic and per-blob skip/seal outcomes.

Suggested labels

kind:bug

Suggested reviewers

• kevincodex1

Poem

🐇 Hoppin' through the DID fields, resolving with care,
No partial seals shall slip past my glare!
Plan before seal—all recipients must align,
Fail closed and log clear, every path by design.
With SealPlan's decisiveness and tests verified true,
This rabbit guards encryption through and through! 🔐

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title directly describes the main change: implementing fail-closed behavior when recipient DIDs cannot be resolved, which is the core fix in the changeset.
Description check	✅ Passed	The PR description is comprehensive, covering summary, motivation, concrete changes, verification steps, and protocol impact. It fully adheres to the repository template and provides thorough context for reviewers.
Linked Issues check	✅ Passed	The PR fully satisfies issue #47's requirements: implements all-or-nothing DID resolution, logs unresolved DIDs with a bounded sample, adds aggregate warnings, and improves observability with warning logs in previously silent failure paths.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to the single file encrypted_pin.rs and directly address the fail-closed resolution bug. No unrelated refactoring or feature changes are present outside the stated objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/encrypted-pin-partial-recipients

---

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

[kevincodex1]

fix failing tests

[beardthelion]

Pushed ca53afd: that was the fmt gate, not the tests (stable + beta were already green). cargo fmt --all on encrypted_pin.rs, two cosmetic reflows. fmt + clippy and cargo audit both pass now. @kevincodex1

[jatmn]

Validation notes

• I checked the repo docs (README.md, docs/RUN-A-NODE.md) and CI config (.github/workflows/pr-checks.yml) for a stated Git requirement and did not find one.
• The test job runs on ubuntu-latest and executes cargo test --workspace, but it does not pin or document a Git version.
• In my review environment (Windows, git version 2.53.0.windows.2), cargo test -p gitlawb-node sync -- --nocapture failed because this Git build rejected git clone --mirror --filter=blob:limit=10g from crates/gitlawb-node/src/sync.rs:445-454.
• Since the repo does not document a supported Git version and CI runs on a different OS, I do not have enough evidence to call that a PR bug rather than an environment mismatch.

[beardthelion]

Good catch on the missing Git-version floor. That blob:limit filter has bitten portability before and the repo really should document a minimum. It's outside this PR's diff so I'll spin it into a separate issue rather than scope-creep here. Thanks for the thorough pass.

[beardthelion]

@kevincodex1 this is ready. CI is green across the board (stable/beta, fmt+clippy, build, audit, MSRV, CodeQL, CodeRabbit clean) and mergeable. jatmn's note is a portability observation about sync.rs, which this PR doesn't touch; I'm tracking it as a separate issue.