fix(governance): replace unvalidated u8 discriminants with typed enums and bounds checks

[chefsale]

Re-applies the typed-discriminant hardening from #2965 (reverted in #3032), with the build break that forced the revert fixed.

What this does

• GroupOp::SubgroupVisibilitySet.mode becomes VisibilityMode instead of a raw u8, and VisibilityMode gains BorshSerialize/BorshDeserialize. The wire layout is unchanged — a two-unit-variant enum serializes as the same 0/1 discriminant byte.
• ContextCapabilityGranted / ContextCapabilityRevoked.capability become a validated ContextCapabilityBits newtype that rejects a zero bitmask at Borsh deserialization, so an invalid capability op cannot be constructed from received bytes. A revoke that clears the last bit now deletes the member-capability row instead of storing a zero.
• Removes the manual u8 ↔ enum conversions in the visibility handlers and the apply op.

Why the original (#2965) was reverted — and why this one is safe

The first attempt did not update two capability-op test sites in governance-store/src/tests.rs that still passed capability: 0b1 (a raw integer). Those sites were added by a change that merged concurrently with #2965, so #2965 never saw them. Each was green in isolation; their combination failed to compile:

error[E0308]: mismatched types
    --> crates/governance-store/src/tests.rs:1292
     |     capability: 0b1,
     |                 ^^^ expected `ContextCapabilityBits`, found integer

This broke the test build, so #2965 was reverted in #3032. CI never built the two PRs together (each merged against a stale-green base). Those two sites now construct the value via ContextCapabilityBits::new(0b1).

Test plan

• cargo build --workspace --all-targets --tests — clean ✅ (the exact path that broke)

🤖 Generated with Claude Code

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 59.1s

🟡 2 warnings. See inline comments.

---

_{🤖 Generated by MeroReviewer | Review ID: review-95ac1a67}

[meroreviewer]

🟡 BorshDeserialize impl does not call the canonical constructor, duplicating zero-check logic

The custom BorshDeserialize reads a raw u8 and manually checks bits == 0, duplicating the invariant already encoded in ContextCapabilityBits::new. If the invariant ever changes (e.g. reserved bits must also be zero), the two sites can drift. The canonical pattern is to deserialize into the primitive, call Self::new(bits).ok_or(...), and return the error — keeping the invariant in one place.

Suggested fix:

Replace the body with: `let bits = u8::deserialize_reader(reader)?; Self::new(bits).ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "ContextCapabilityBits: capability bitmask must not be zero"))`

[meroreviewer]

🟡 ContextCapabilityBits does not implement Serde, limiting future serialization paths

ContextCapabilityBits derives BorshSerialize/BorshDeserialize and implements TryFrom/From, but has no Serde impls. The sibling VisibilityMode enum gained both Borsh and Serde derives in this PR. If ContextCapabilityBits ever appears in a JSON-serialized struct (e.g. API responses, config), callers will get an opaque compile error rather than a clear design choice. This is not a bug today, but the asymmetry with VisibilityMode is worth a deliberate decision.

Suggested fix:

Either add `#[derive(serde::Serialize, serde::Deserialize)]` with a transparent/u8 representation, or add a doc comment explicitly stating Serde is intentionally omitted and why.

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 AGENTS.md: AGENTS.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
• 🟡 CONTRIBUTING.md: CONTRIBUTING.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
• 🟡 architecture/auto-follow.html: Files matching crates/governance-store/** were changed but architecture/auto-follow.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/context.html: Files matching crates/context/** were changed but architecture/crates/context.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/dag.html: Files matching crates/op-adapter/** were changed but architecture/crates/dag.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/local-governance.html: Files matching crates/governance-store/** were changed but architecture/local-governance.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/membership-and-leave.html: Files matching crates/context/** were changed but architecture/membership-and-leave.html was not updated (per source_to_docs_mapping).
• 🟡 docs/: Static HTML docs in docs/ may need updating — architecture-impacting changes detected. On merge, update-docs will scan this directory and open a PR if any pages need to change.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 48% | Review time: 375.1s

🟡 1 warnings, 💡 2 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-5e1cc281}

[meroreviewer]

🟡 ContextCapabilityBits rejects zero at deserialization but BorshSerialize can still produce zero via bit-manipulation outside the type

The BorshDeserialize impl correctly rejects a zero bitmask on the wire. However, ContextCapabilityBits derives BorshSerialize automatically, which will serialize the inner u8 directly. The invariant (non-zero) is maintained at construction time via new/TryFrom, but there is no #[non_exhaustive] or private-field enforcement preventing future code from constructing a zero value through unsafe means or by adding a Default impl. The current code is safe, but the struct's invariant is only documented in comments — consider adding a debug_assert!(self.0 != 0) in get() or a const assertion to make the invariant machine-checked.

Suggested fix:

Add `debug_assert_ne!(self.0, 0, "ContextCapabilityBits invariant violated");` at the start of `get()` to catch any future invariant violations in debug builds.

[meroreviewer]

💡 Consider using NonZeroU8 as the inner type instead of a custom newtype

The ContextCapabilityBits newtype essentially reimplements NonZeroU8 — it wraps a u8, rejects zero at construction, and provides a get() method. The codebase already has a From<NonZeroU8> impl, which signals awareness of NonZeroU8. Using NonZeroU8 directly (or as the inner field) would eliminate the custom BorshDeserialize impl and the TryFrom<u8> impl, since NonZeroU8::new already returns Option<NonZeroU8>. The main trade-off is that NonZeroU8 doesn't implement BorshSerialize/BorshDeserialize out of the box, so a thin newtype is justified — but the new constructor returning Option<Self> instead of Result<Self, E> is inconsistent with the TryFrom<u8> impl that returns Result. Consider aligning: either make new return Result or remove TryFrom and use new().ok_or(...) at call sites.

Suggested fix:

Either keep `new` returning `Option<Self>` and remove the `TryFrom<u8>` impl (callers can use `.ok_or("...")` inline), or change `new` to return `Result<Self, &'static str>` and implement `TryFrom` in terms of it. Mixing both creates two parallel fallible construction paths with different error types.

[meroreviewer]

💡 Custom BorshDeserialize does not call borsh::BorshDeserialize via the derive path, risking future divergence

The manual BorshDeserialize impl delegates to u8::deserialize_reader, which is correct today. However, if the struct ever gains additional fields, the manual impl will silently stop deserializing them while the auto-derived version would. A comment explaining why the manual impl is necessary (to enforce the non-zero invariant) would prevent future maintainers from accidentally replacing it with a derive or adding fields without updating the impl.

Suggested fix:

Add a `// SAFETY:` or `// NOTE:` comment above the `impl BorshDeserialize` block explaining that the manual impl exists solely to enforce the non-zero invariant, and that any structural change to `ContextCapabilityBits` must update this impl accordingly.

[chefsale]

Thanks — went through the review, and also fixed a CI failure the reviewers' commit predated:

🔴 Fixed — golden-fixture decode failure (CI). group_op_discriminants_are_golden froze capability: 0 bytes for ordinals 17/18, which the new ContextCapabilityBits correctly rejects on the wire, so the golden decode failed. Updated the fixtures to a valid non-zero bitmask (1) — same byte length and discriminant, so the wire-stability intent is unchanged. (commit 8aa27d1)

✅ Applied — BorshDeserialize duplicates the zero-check (warning). deserialize_reader now routes through Self::new(bits).ok_or_else(...), so the zero-rejection invariant lives only in the constructor. (commit 8aa27d1)

ℹ️ NonZeroU8 as the inner type (suggestion). Fair — it is morally a NonZeroU8. I kept the newtype: it carries domain meaning (a capability bitmask, not just any non-zero byte), keeps the get()/masking surface explicit, and matches the reverted PR we're restoring. Worth a follow-up if we standardize on NonZeroU8 elsewhere, but out of scope for un-reverting.

ℹ️ Missing Serde impls (suggestion). Intentional. ContextCapabilityBits only ever crosses the Borsh governance-op wire; VisibilityMode needs Serde because it also flows through context-config's Serde surfaces. Adding Serde here would need a custom impl to preserve the non-zero invariant, with no consumer — YAGNI.

ℹ️ BorshSerialize could emit zero / manual impl divergence (warning + suggestion). A zero ContextCapabilityBits can't be constructed (every path goes through new/try_from/the validating deserializer), so serialize can't emit one. It's a single-field newtype, so the manual deserializer can't silently drift from added fields. Left as-is.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 45% | Review time: 112.1s

🟡 1 warnings, 💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-15c73c34}

[meroreviewer]

🟡 ContextCapabilityBits::new returns Option but callers must handle None

The new constructor returns Option<Self> and is marked #[must_use], which is correct. However, the TryFrom<u8> impl and From<NonZeroU8> impl are sound. The concern is that BorshDeserialize rejects zero on the wire, but BorshSerialize (derived) will happily serialize any ContextCapabilityBits(0) if one were ever constructed — which is impossible through the public API but worth noting. The real issue: there is no Serde impl, so if this type is ever used in a JSON context (e.g., RPC responses), callers will get an opaque serialization error rather than a clear validation message. This is a latent maintainability risk, not a current bug.

Suggested fix:

Consider adding a `serde` impl with a custom deserializer that mirrors the Borsh zero-rejection, or at minimum document that serde is intentionally omitted.

[meroreviewer]

💡 Consider using NonZeroU8 directly instead of a newtype

The ContextCapabilityBits newtype wraps a u8 with a non-zero invariant, which is exactly what std::num::NonZeroU8 provides. The PR already implements From<NonZeroU8> for ContextCapabilityBits, acknowledging this relationship. Using NonZeroU8 as the inner type (i.e., ContextCapabilityBits(NonZeroU8)) would let the compiler enforce the invariant at the type level, eliminate the manual BorshDeserialize implementation (since the zero-check would be structural), and make the new constructor a thin wrapper around NonZeroU8::new. This would reduce the surface area for bugs and remove the need for the custom deserializer entirely.

Suggested fix:

Change the struct to `pub struct ContextCapabilityBits(NonZeroU8);` and update `new`, `get`, and the `From` impl accordingly. The custom `BorshDeserialize` can then delegate to `NonZeroU8::deserialize_reader` and map the error, or you can implement it as `u8::deserialize_reader` + `NonZeroU8::new(...).ok_or(...)` — same logic, but the invariant is now structural.