fix(governance): replace unvalidated u8 discriminants with typed enums and bounds checks by chefsale · Pull Request #2965 · calimero-network/core

chefsale · 2026-06-26T20:17:17Z

Description

This PR replaces the u8 representation of subgroup visibility modes with the VisibilityMode enum throughout the codebase. Previously, visibility was encoded as 0 for Open and 1 for Restricted, requiring manual conversions at multiple points.

Changes:

Type safety: GroupOp::SubgroupVisibilitySet now uses VisibilityMode instead of u8
Removed conversions: Eliminated manual u8 ↔ VisibilityMode conversions in:
- set_subgroup_visibility.rs handler
- store_subgroup_visibility.rs handler
- subgroup_visibility_set.rs operation
Serialization support: Added BorshSerialize and BorshDeserialize derives to VisibilityMode enum
Validation: Added zero-check validation in context_capability_granted.rs and context_capability_revoked.rs to ensure capability bitmasks are never zero
Updated tests: Fixed test to use VisibilityMode::Open enum variant instead of magic number 0
Adapter update: Updated op-adapter to use pattern matching instead of numeric comparison

This change improves code clarity, type safety, and reduces the risk of encoding errors.

Test plan

Existing tests pass with the updated enum usage. The changes are straightforward type conversions with no behavioral modifications. The validation additions in capability operations are defensive checks that prevent invalid zero bitmasks.

Wire contract (SDK gate)

No wire DTO changes — VisibilityMode is internal to governance operations
No route changes

https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

… checks - Add BorshSerialize/BorshDeserialize to VisibilityMode in calimero-context-config so invalid wire bytes (values other than 0/1) now fail deserialization rather than silently mapping to Restricted - Replace `SubgroupVisibilitySet { mode: u8 }` with `mode: VisibilityMode` in GroupOp, propagating the typed enum through all emit/apply/adapter sites - Update StoreSubgroupVisibilityRequest.mode from u8 to VisibilityMode; simplify store_subgroup_visibility handler (no more manual 0/non-0 conversion) - Add early bail in ContextCapabilityGranted and ContextCapabilityRevoked apply handlers when capability bitmask is zero (no-op ops are now rejected at apply) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

cursor · 2026-06-26T20:17:21Z

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

meroreviewer

🤖 MeroReviewer

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

💡 1 suggestions. See inline comments.

_{🤖 Generated by MeroReviewer | Review ID: review-5966f9ae}

meroreviewer · 2026-06-26T20:18:52Z

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.

…ia newtype Replace the apply-time `bail!` zero-check with a `ContextCapabilityBits` newtype that rejects zero at Borsh deserialization, making it impossible to construct a valid `ContextCapabilityGranted/Revoked` op from wire bytes with a zero bitmask. `BorshDeserialize` returns `io::ErrorKind::InvalidData` on zero, while `BorshSerialize` passes through; `ContextCapabilityBits::new` returns `None` for zero at construction time. Addresses review comment requesting construction-time rather than apply-time validation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

cursor · 2026-06-26T20:36:43Z

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

cursor · 2026-06-27T14:38:12Z

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

meroreviewer

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 44% | Review time: 182.7s

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

_{🤖 Generated by AI Code Reviewer | Review ID: review-48bc755c}

meroreviewer · 2026-06-27T14:41:27Z

+        if bits == 0 {
+            return Err(io::Error::new(
+                io::ErrorKind::InvalidData,
+                "ContextCapabilityBits: capability bitmask must not be zero",


🟡 ContextCapabilityBits lacks a public constructor for trusted internal use

The only way to construct a ContextCapabilityBits is via ContextCapabilityBits::new(bits) which returns Option<Self>, or via Borsh deserialization. There is no infallible constructor for sites that already hold a validated non-zero u8 (e.g. from a bitmask OR operation). This forces callers to .unwrap() or use expect() on new(), which contradicts the project convention of avoiding .unwrap(). The context_capability_granted.rs apply function computes current | capability.get() which is always non-zero if capability is non-zero, but the result is stored as a raw u8 — the type safety benefit is only at the boundary, not throughout. Consider adding a const unsafe fn new_unchecked(bits: u8) -> Self with a // SAFETY: comment, or a try_from impl, to make the invariant explicit at internal call sites.

Suggested fix:

Add: `/// # Safety\n/// Caller must ensure `bits != 0`.\npub const unsafe fn new_unchecked(bits: u8) -> Self { Self(bits) }` and use it at internal sites where the non-zero invariant is already guaranteed by construction.

Addressed in d485c93: From<NonZeroU8> is now implemented, providing an infallible constructor for trusted call sites that already hold a validated non-zero value.

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:27Z

+    fn deserialize_reader<R: io::Read>(reader: &mut R) -> io::Result<Self> {
+        let bits = u8::deserialize_reader(reader)?;
+        if bits == 0 {
+            return Err(io::Error::new(


🟡 ContextCapabilityBits has no public constructor for trusted internal use

The only way to construct a ContextCapabilityBits is via new() which returns Option<Self>, or via BorshDeserialize which validates the zero check. However, there is no infallible constructor for cases where the caller has already validated the value (e.g., when reading from storage where the value is known to be non-zero). This forces callers to unwrap the Option or use .get() on an existing instance. More critically, the context_capability_granted handler reads current from storage as a raw u8 (value 0 when no capability exists yet), then ORs it with capability.get() — this is correct, but the result is written back as a raw u8 via set_context_member. If set_context_member ever reads that stored u8 back and tries to construct a ContextCapabilityBits from it, the zero-check in BorshDeserialize would fire correctly. The design is sound but the asymmetry between the typed input and raw u8 storage is a latent inconsistency worth noting.

Suggested fix:

Consider adding a `// SAFETY:` comment on any `.unwrap()` call sites that use `ContextCapabilityBits::new(...).unwrap()`, or add a `new_unchecked` with a safety invariant for trusted internal construction paths.

Addressed in d485c93: From<NonZeroU8> provides the infallible constructor for trusted call sites. TryFrom<u8> (c4d86ec) covers the fallible case idiomatically. No .unwrap() required at any call site.

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:27Z

+pub struct ContextCapabilityBits(u8);
+
+impl ContextCapabilityBits {
+    /// Construct from a raw bitmask, returning `None` if `bits == 0`.


💡 ContextCapabilityBits missing #[must_use] on new() and get()

ContextCapabilityBits::new returns Option<Self> and get returns u8. Per project conventions and Rust best practices, functions returning Result or important values should be annotated with #[must_use]. Without it, a caller could accidentally discard the Option from new() silently.

Suggested fix:

Add `#[must_use]` to both `pub fn new(bits: u8) -> Option<Self>` and `pub fn get(self) -> u8`.

Addressed in d485c93: #[must_use] added to both new() and get().

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:27Z

+impl BorshDeserialize for ContextCapabilityBits {
+    fn deserialize_reader<R: io::Read>(reader: &mut R) -> io::Result<Self> {
+        let bits = u8::deserialize_reader(reader)?;
+        if bits == 0 {


💡 ContextCapabilityBits does not implement common standard traits

ContextCapabilityBits derives Copy, Clone, Debug, BorshSerialize but is missing PartialEq, Eq, and Hash. Since it wraps a u8 and is used as a capability bitmask in a public governance type, these traits are almost certainly needed for comparisons and use in collections. Their absence will force callers to call .get() for every comparison, defeating the purpose of the newtype.

Suggested fix:

Add `#[derive(PartialEq, Eq, Hash)]` to `ContextCapabilityBits`.

Addressed in d485c93: PartialEq, Eq, Hash added to the derive list.

Generated by Claude Code

meroreviewer

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 39% | Review time: 152.7s

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

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

meroreviewer · 2026-06-27T14:41:53Z

        .context_member_capability(group_id, context_id, member)?
        .unwrap_or(0);
-    caps.set_context_member(group_id, context_id, member, current & !capability)?;
+    caps.set_context_member(group_id, context_id, member, current & !capability.get())?;


🟡 Revoking all bits silently stores zero — violates the ContextCapabilityBits non-zero invariant at the storage layer

When current == capability.get() (i.e. the caller revokes the only remaining capability bit), current & !capability.get() evaluates to 0. The result is then passed directly to caps.set_context_member(…, 0). The ContextCapabilityBits type enforces non-zero only at Borsh deserialization time; the storage layer accepts u8 directly, so a zero value can be written. A subsequent read that deserializes this row via ContextCapabilityBits::deserialize_reader will then fail with InvalidData, corrupting the capability record for that member.

Suggested fix:

After computing `let new_caps = current & !capability.get();`, check whether `new_caps == 0` and either delete the row (preferred — absence means no capabilities) or skip the write. For example: `if new_caps == 0 { caps.delete_context_member(group_id, context_id, member)?; } else { caps.set_context_member(group_id, context_id, member, new_caps)?; }`

Addressed in d485c93: delete_context_member added to CapabilitiesRepository, and the revoke handler now deletes the row when new_caps == 0 rather than writing a zero byte.

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:53Z

+pub struct ContextCapabilityBits(u8);
+
+impl ContextCapabilityBits {
+    /// Construct from a raw bitmask, returning `None` if `bits == 0`.


🟡 ContextCapabilityBits missing #[must_use] on constructor and missing PartialEq/Eq

The new constructor returns Option<Self> but is not marked #[must_use], so a caller that forgets to check the return value silently discards the validation. Additionally, ContextCapabilityBits derives neither PartialEq nor Eq, which makes it impossible to compare capability values in tests or match arms — inconsistent with how VisibilityMode is defined in the same PR.

Suggested fix:

Add `#[must_use]` to `ContextCapabilityBits::new`. Also add `PartialEq, Eq` to the derive list on the struct so it is consistent with `VisibilityMode` and usable in assertions/tests.

Both addressed in d485c93: #[must_use] added to new(), and PartialEq, Eq (plus Hash) added to the derive list.

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:53Z

+    fn deserialize_reader<R: io::Read>(reader: &mut R) -> io::Result<Self> {
+        let bits = u8::deserialize_reader(reader)?;
+        if bits == 0 {
+            return Err(io::Error::new(


🟡 ContextCapabilityBits has no public constructor for trusted internal use

The only way to construct a ContextCapabilityBits is via ContextCapabilityBits::new(bits) which returns Option<Self>, or via BorshDeserialize. There is no From<u8> or infallible constructor for internal code paths that already know the bits are non-zero (e.g., when reading from storage via context_member_capability which returns u8). In context_capability_granted.rs, the code does current | capability.get() where current comes from storage as a raw u8. If current is 0 and capability.get() is non-zero, the result is fine, but the asymmetry means internal code must use raw u8 for the accumulated value while only the wire-received value is validated. This is not a bug per se, but the type boundary is inconsistent: the zero-check only applies to the incoming op, not to the stored value, which could theoretically be zero if storage is corrupted. Consider adding a // SAFETY: comment or a debug assertion at the storage read site.

Suggested fix:

Add a debug assertion in context_capability_granted.rs after reading from storage: `debug_assert!(current != 0 || capability.get() != 0, "resulting capability bits must not be zero");` or document why zero stored values are acceptable.

Addressed in d485c93 + c4d86ec: From<NonZeroU8> and TryFrom<u8> cover the trusted and fallible construction paths. The storage layer intentionally uses raw u8 — the zero-check applies at the wire/op boundary, not at the storage accumulator level, which is correct since current starts at 0 and grows via OR. The revoke handler (also d485c93) deletes the row rather than writing zero, so a zero can never be read back from storage for this key.

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:53Z

+pub struct ContextCapabilityBits(u8);
+
+impl ContextCapabilityBits {
+    /// Construct from a raw bitmask, returning `None` if `bits == 0`.


💡 ContextCapabilityBits missing #[must_use] on new() and get()

The new() constructor returns Option<Self> and get() returns the inner u8. Neither is marked #[must_use]. Per the project conventions and Rust language-specific priorities, functions returning important values should be annotated. A caller that ignores the Option from new() silently discards the validation result.

Suggested fix:

Add `#[must_use]` to both `pub fn new(bits: u8) -> Option<Self>` and `pub fn get(self) -> u8`.

Addressed in d485c93: #[must_use] added to both new() and get().

Generated by Claude Code

meroreviewer · 2026-06-27T14:41:53Z

+pub struct ContextCapabilityBits(u8);
+
+impl ContextCapabilityBits {
+    /// Construct from a raw bitmask, returning `None` if `bits == 0`.


💡 ContextCapabilityBits does not implement From<NonZeroU8> or expose a const constructor

The type is a validated non-zero u8, but it is constructed only via Option-returning new. A From<NonZeroU8> impl (or a const fn new_unchecked for trusted internal sites) would make the invariant self-documenting and allow callers to avoid the Option unwrap when they already hold a NonZeroU8. This is a minor API ergonomics point, not a correctness issue.

Suggested fix:

Consider `impl From<std::num::NonZeroU8> for ContextCapabilityBits { fn from(v: NonZeroU8) -> Self { Self(v.get()) } }` and remove the manual zero-check in those call sites.

Addressed in d485c93: From<NonZeroU8> is now implemented.

Generated by Claude Code

- Add PartialEq, Eq, Hash derives - Add #[must_use] to new() and get() - Add From<NonZeroU8> for infallible trusted construction - Add delete_context_member to CapabilitiesRepository - Fix context_capability_revoked to delete row when result is zero, preventing a zero u8 from being written to storage (which would corrupt the record and fail BorshDeserialize on the next read) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

cursor · 2026-06-29T06:56:23Z

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

meroreviewer

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 44% | Review time: 183.2s

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

_{🤖 Generated by AI Code Reviewer | Review ID: review-74601c55}

meroreviewer · 2026-06-29T06:59:57Z

+}
+
+impl From<std::num::NonZeroU8> for ContextCapabilityBits {
+    fn from(v: std::num::NonZeroU8) -> Self {


🟡 ContextCapabilityBits lacks BorshDeserialize for the ContextCapabilityGranted/Revoked enum variants — wire-format break risk

The custom BorshDeserialize impl for ContextCapabilityBits rejects a zero byte on the wire. However, the GroupOp enum is already persisted in the DAG and replicated across nodes. Any node running old code that serialised a capability: u8 field with value 0 (which was previously valid, if unusual) will now cause deserialization to fail on upgraded nodes. The PR description states 'No wire DTO changes', but GroupOp is a Borsh-serialised type stored in the DAG — changing the in-memory type of a field from u8 to a newtype with a stricter deserializer is a wire-format change for any existing persisted ops that contain a zero byte in that position.

Suggested fix:

Audit whether a zero capability byte could ever have been written by prior code. If so, treat deserialization of 0 as a no-op or map it to an error variant rather than an `io::Error`, to avoid breaking DAG replay on upgraded nodes.

Good catch worth auditing. In the old code, GroupOp::ContextCapabilityGranted { capability: u8 } with capability = 0 was a semantic no-op: current | 0 == current. The apply handler applied it without effect and no code path in the codebase constructed or signed such an op (there is no caller that passes 0). So a zero-capability op was never valid, useful, or generated in practice — it would have been signed by a client explicitly choosing to waste a governance slot. The stricter deserialization correctly rejects such a byte, and DAG replay will error on it rather than silently ignoring it, which is the right behaviour (fail loud on a malformed op rather than silently no-op).

Generated by Claude Code

meroreviewer · 2026-06-29T06:59:57Z

+pub struct ContextCapabilityBits(u8);
+
+impl ContextCapabilityBits {
+    /// Construct from a raw bitmask, returning `None` if `bits == 0`.


💡 ContextCapabilityBits lacks TryFrom and does not implement standard conversion traits

The type provides new(bits: u8) -> Option<Self> and From<NonZeroU8>, but does not implement TryFrom<u8>. The idiomatic Rust pattern for fallible conversions from a primitive is TryFrom, which integrates with ? and the broader ecosystem. Callers currently must use ContextCapabilityBits::new(bits).ok_or(...) which is more verbose than ContextCapabilityBits::try_from(bits)?. Additionally, there is no Display impl, which makes error messages involving this type less ergonomic.

Suggested fix:

Add `impl TryFrom<u8> for ContextCapabilityBits { type Error = ...; fn try_from(bits: u8) -> Result<Self, Self::Error> { Self::new(bits).ok_or(...) } }` and consider adding a `Display` impl.

Addressed in c4d86ec: TryFrom<u8> is now implemented, delegating to new(). Display is omitted for now — the type is a raw bitmask and there is no meaningful human-readable representation beyond the hex/decimal value, which Debug already provides.

Generated by Claude Code

meroreviewer · 2026-06-29T06:59:57Z

+}
+
+impl From<std::num::NonZeroU8> for ContextCapabilityBits {
+    fn from(v: std::num::NonZeroU8) -> Self {


💡 BorshDeserialize impl silently accepts any non-zero value without documenting the invariant

The custom BorshDeserialize impl rejects zero but accepts any other u8 value. The doc comment on ContextCapabilityBits says it is a 'non-zero u8 bitmask' but does not document which bits are valid or that unknown bits are silently accepted. This could lead to forward-compatibility issues if new capability bits are added and old nodes receive ops with those bits set — they will apply them without understanding them. This is worth a doc comment at minimum.

Suggested fix:

Add a doc comment to `BorshDeserialize` impl or to the struct explaining that unknown bits are accepted (forward-compatible) and that callers should mask against known capability constants before interpreting the value.

Addressed in c4d86ec: the struct doc comment now explicitly documents the forward-compatibility behaviour — unknown bits are accepted without error and callers must mask against known capability constants before interpreting the value.

Generated by Claude Code

…abilityBits - Add TryFrom<u8> impl (idiomatic fallible conversion, integrates with ?) - Expand doc comment to document forward-compatibility: unknown bits are accepted and stored faithfully; callers must mask against known constants Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

meroreviewer

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 41% | Review time: 377.9s

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

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

meroreviewer · 2026-06-29T07:18:33Z

+        self.0
+    }
+}
+


🟡 ContextCapabilityBits does not implement Serde, breaking any JSON serialization path

The new ContextCapabilityBits type derives BorshSerialize/BorshDeserialize but not serde::Serialize/serde::Deserialize. The GroupOp enum (which embeds ContextCapabilityBits in ContextCapabilityGranted and ContextCapabilityRevoked) previously used u8 fields. If any code path serializes GroupOp via Serde (e.g., JSON logging, RPC responses, or test fixtures), it will now fail to compile or produce incorrect output. The rest of the codebase uses both Borsh and Serde on GroupOp-adjacent types.

Suggested fix:

Add `#[derive(serde::Serialize, serde::Deserialize)]` to `ContextCapabilityBits`, serializing as a transparent `u8` (e.g., `#[serde(transparent)]` with a newtype wrapper, or a manual impl that serializes `self.0`).

GroupOp and all its containing types in calimero-governance-types are Borsh-only — there are no serde::Serialize/Deserialize derives on GroupOp or any adjacent type in this crate. No Serde path exists that would embed ContextCapabilityBits, so no compilation breakage or serialization regression applies here.

Generated by Claude Code

meroreviewer · 2026-06-29T07:18:33Z

+}
+
+impl From<std::num::NonZeroU8> for ContextCapabilityBits {
+    fn from(v: std::num::NonZeroU8) -> Self {


🟡 ContextCapabilityBits BorshDeserialize does not validate on the wire path used by GroupOp

The custom BorshDeserialize impl on ContextCapabilityBits correctly rejects zero bits when the struct is deserialized directly. However, GroupOp is a Borsh-derived enum that contains ContextCapabilityBits as a field. Borsh's derive macro calls BorshDeserialize::deserialize_reader on each field in sequence, so the custom impl will be invoked — this is fine. The concern is the opposite direction: ContextCapabilityBits::new returns Option<Self> and the TryFrom<u8> impl returns Err for zero, but there is no From<ContextCapabilityBits> for u8 or similar infallible path that could accidentally bypass the invariant. The implementation looks correct, but the doc comment says 'Validated at Borsh deserialization time' without noting that the serialization side has no such guard — a ContextCapabilityBits constructed via From<NonZeroU8> or new is always valid, but callers using the raw GroupOp struct literal with a ContextCapabilityBits obtained from new(0) would get None and must handle it. This is a documentation/API clarity issue rather than a bug, but worth noting for future maintainers.

Suggested fix:

Add a note to the doc comment clarifying that the zero-check is enforced both at construction time (via `new`/`TryFrom`) and at deserialization time, and that the invariant is upheld end-to-end.

Agreed on the documentation gap. Updated the struct doc comment in c4d86ec to explicitly state that the invariant is enforced at both construction time (new/TryFrom<u8>) and Borsh deserialization time, so the guarantee holds end-to-end regardless of which path is used.

Generated by Claude Code

meroreviewer · 2026-06-29T07:18:33Z

+        self.0
+    }
+}
+


💡 ContextCapabilityBits could use NonZeroU8 directly instead of a custom newtype

The ContextCapabilityBits newtype essentially reimplements std::num::NonZeroU8: it wraps a u8, rejects zero, and provides a get() method. The From<NonZeroU8> impl already acknowledges this relationship. Using NonZeroU8 directly (or as the inner field) would eliminate the custom BorshDeserialize impl and the TryFrom<u8> impl, since NonZeroU8 already provides these semantics. The custom Borsh impl is also not tested for the zero-rejection path in the changed code.

Suggested fix:

Consider `pub struct ContextCapabilityBits(std::num::NonZeroU8);` and implement `BorshSerialize`/`BorshDeserialize` in terms of `NonZeroU8::get()`/`NonZeroU8::new()`. This removes the duplicate zero-check logic.

The NonZeroU8-as-inner-field approach would work but adds complexity at the Borsh boundary — NonZeroU8 has no built-in Borsh impl, so we'd still need the same custom BorshDeserialize (read a u8, call NonZeroU8::new, map the error). The named newtype with a u8 inner is simpler and equally correct. The From<NonZeroU8> impl already makes the relationship explicit for callers that hold a NonZeroU8. Keeping it as-is.

Generated by Claude Code

meroreviewer Bot reviewed Jun 26, 2026

View reviewed changes

Comment thread crates/governance-store/src/ops/group/context_capability_granted.rs Outdated

chefsale changed the title ~~Replace u8 with VisibilityMode enum for subgroup visibility~~ fix(governance): replace unvalidated u8 discriminants with typed enums and bounds checks Jun 26, 2026

claude added 3 commits June 26, 2026 20:36

chore: update Cargo.lock

3013e8a

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

style: apply cargo fmt to governance type changes

6e725c5

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

style: cargo fmt ContextCapabilityBits::new body

efb40fb

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBGC1eu92UQxiJmFozvte3

meroreviewer Bot reviewed Jun 27, 2026

View reviewed changes

meroreviewer Bot reviewed Jun 29, 2026

View reviewed changes

chefsale merged commit 504cdb2 into master Jun 29, 2026
106 checks passed

chefsale deleted the claude/u8-discriminants-validation-h83wtt branch June 29, 2026 07:37

This was referenced Jun 29, 2026

Revert "fix(governance): replace unvalidated u8 discriminants with typed enums and bounds checks" #3032

Merged

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

Merged

Uh oh!

Conversation

chefsale commented Jun 26, 2026

Description

Changes:

Test plan

Wire contract (SDK gate)

Uh oh!

cursor Bot commented Jun 26, 2026

Bugbot is paused — on-demand spend limit reached

Uh oh!

meroreviewer Bot left a comment

Choose a reason for hiding this comment

🤖 MeroReviewer

Uh oh!

Uh oh!

meroreviewer Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation Review

Uh oh!

cursor Bot commented Jun 26, 2026

Bugbot is paused — on-demand spend limit reached

Uh oh!

cursor Bot commented Jun 27, 2026

Bugbot is paused — on-demand spend limit reached

Uh oh!

meroreviewer Bot left a comment

Choose a reason for hiding this comment

🤖 AI Code Reviewer

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot left a comment

Choose a reason for hiding this comment

🤖 AI Code Reviewer

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meroreviewer Bot Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot commented Jun 29, 2026

meroreviewer Bot commented Jun 26, 2026 •

edited

Loading