Repository navigation
feat(id): signed themes and layered board views (#2813) - #3971
Conversation
Theme object v1: design tokens, closed layout enum, content-addressed asset refs, fork lineage. Board.theme_ref applies a theme by reference (theme published first; themes name no board, keeping the content-addressed graph acyclic). BoardGraph::compose_view projects the three layers - personal filter, board view, host view - under the transparency rule: hidden objects stay in the graph and are reported with the layer and record that hid them, and the board moderation log is public in every view. Custom HTML templates stay out of v1: a theme is data, never markup.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a3dd4884c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let BoardObject::Theme(theme) = &theme_object.object else { | ||
| bail!("board theme reference is not a theme object") | ||
| }; | ||
| Some(theme.clone()) |
There was a problem hiding this comment.
When layers.host_hidden contains the board's theme hash, compose_view still returns that theme's full body through BoardView.theme. Themes also bypass the later filtering loop because their board_ref() is None, so the suppression is never reported in hidden. This makes the host layer ineffective for a signed theme package that the host explicitly refuses to serve. Check the referenced theme against the applicable layers before including it in the projection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2333c6d. The resolved theme is now checked against the host layer (and the personal layer: personal hides + muted theme author) before it is returned; a suppressed theme yields theme: None and a hidden entry naming the theme hash and the suppressing layer. Covered by host_layer_can_suppress_the_board_theme.
| BoardObject::Tombstone(value) if &value.board == board => { | ||
| tombstones.entry(value.target.clone()).or_insert_with(|| hash.clone()); |
There was a problem hiding this comment.
When an author supersedes a tombstone targeting object A with another tombstone targeting object B, both objects remain hidden because this pass applies every historical tombstone. validate_links permits this same-author, same-kind replacement, and Supersede.replacement explicitly replaces the earlier object in a projection, but the superseded set is only checked later while selecting visible content. Resolve superseded records before gathering active tombstone effects so the obsolete tombstone does not continue hiding A.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2333c6d. compose_view now gathers the board's supersede relations in a first pass, and a superseded moderation action or tombstone record has no projection effect (it remains part of the signed history, and moderation actions remain in the public log). Covered by superseded_tombstone_loses_effect_and_keeps_its_signed_reason: superseding a tombstone on post A with one on post B makes A visible again and hides only B.
| .unwrap_or((hash.clone(), None)); | ||
| (Some(action), reason) | ||
| } else { | ||
| (tombstone.cloned(), Some("author tombstone".to_owned())) |
There was a problem hiding this comment.
When an author supplies a deletion reason in Tombstone.reason, the composed view replaces it with the literal "author tombstone". Consequently, consumers of BoardView.hidden lose the author's recorded explanation, contrary to HiddenObject.reason's documented purpose of reporting the reason carried by the hiding record. Retain the tombstone's reason alongside its hash when collecting tombstones and return that recorded value here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2333c6d. The tombstone collection now keeps the record's hash together with its signed reason, and the hidden entry reports that recorded reason verbatim (no more literal "author tombstone" replacement). Asserted in superseded_tombstone_loses_effect_and_keeps_its_signed_reason for both the original and the replacement tombstone.
| BoardObject::ModerationAction(value) if &value.board == board => { | ||
| moderation_log.push(hash.clone()); | ||
| match value.action { | ||
| ModerationActionKind::Hide => { | ||
| *hide_balance.entry(value.target.clone()).or_default() += 1; |
There was a problem hiding this comment.
After the owner revokes a moderator with a RoleAttestation that supersedes their grant, that former moderator can still sign new hides or restores that change this view. Graph insertion intentionally accepts historical grants through has_role, whose documentation explicitly delegates revocation enforcement to the view/serving layers, but this new projection counts every accepted moderation action without checking the author's current authority. Resolve the current role attestations before applying moderation effects, exempting the board owner, while retaining revoked moderators' actions in the public log.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2333c6d. compose_view now resolves the current moderator frontier before applying any moderation effect: role attestations superseded via their signed supersedes links are history, not authority (frontier follows the causal links, never arrival order), so a revoked moderator's hides/restores no longer change the projection. Their actions remain in the public moderation_log, as required. Covered by the new test view_enforces_moderator_revocation_but_keeps_the_public_log.
…(Codex review) Dispositions all four Codex findings on PR #3971: (P1) compose_view resolves the current moderator frontier via signed supersedes links - a revoked moderator's actions stay in the public log but lose effect; (P2) superseded moderation/tombstone records have no projection effects; (P2) hidden entries preserve the signed tombstone reason; (P2) host/personal layers can suppress the board theme, reported in hidden. Three new tests cover revocation, superseded tombstones with reasons, and theme suppression.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2333c6d429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let current_moderators: BTreeSet<String> = self | ||
| .objects | ||
| .iter() | ||
| .filter(|(hash, _)| !attestation_superseded.contains(hash)) |
There was a problem hiding this comment.
When the owner replaces a moderator grant G with a revocation R using a generic Supersede(G, R) record, rather than R's supersedes field, validate_links accepts the replacement and the projection removes G from visible objects. However, this filter ignores the general superseded set, so G still adds the moderator to current_moderators and their hide actions continue affecting the view. Exclude generically superseded grants when computing current authority so the two accepted replacement mechanisms do not produce contradictory moderation behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f6d45f. The authority frontier now excludes attestations in the general (active) superseded set as well as those named by an attestation's own supersedes field, so both accepted replacement mechanisms revoke identically. Covered by generic_supersede_of_a_grant_also_revokes_authority: grant replaced by a revocation through a generic Supersede record ends the moderator's hide effect while the action stays in the public log.
| if let BoardObject::Supersede(value) = &signed.object { | ||
| if &value.board == board { | ||
| superseded.insert(value.previous.clone()); |
There was a problem hiding this comment.
A Supersede record can itself be superseded: for same-author posts A/B/C/D, the graph accepts S1=Supersede(A,B), S2=Supersede(C,D), and S3=Supersede(S1,S2). This loop nevertheless applies S1 and suppresses A even though S1 has been replaced. Consequently, editing a relationship leaves its obsolete effect active, contrary to the stated projection rule that superseded records have no effect. Resolve which relationship records remain active before applying their edges.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f6d45f. Supersede relations are now resolved to an active frontier by fixed point — a record stays active while no active record names it as its previous version (supersede cycles are impossible; insertion rejects them) — and only active records contribute edges to the superseded set. Covered by superseded_supersede_record_has_no_effect using exactly the A/B/C/D + S1/S2/S3 construction from this finding: A and B are current again, C stays replaced by D.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…2813, Codex round 2) Two round-2 Codex findings on PR #3971: a grant replaced through a generic Supersede record (not the attestation supersedes field) now also leaves the authority frontier, and supersede relations are resolved to an active frontier by fixed point - a superseded supersede record contributes no edge. Two tests cover both mechanisms.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f6d45fe57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let next: BTreeSet<ArtifactHash> = active_records | ||
| .difference(&named) | ||
| .cloned() | ||
| .collect(); |
There was a problem hiding this comment.
When supersede records are replaced more than once, this loop permanently removes records that should become active again. For same-author posts A/B/C/D, the graph accepts S1=Supersede(A,B), S2=Supersede(C,D), S3=Supersede(S1,S2), and S4=Supersede(S3,S2). S4 disables S3, so S1 should reactivate and suppress A. Instead, the first iteration removes both S1 and S3, and active_records.difference(&named) can never restore S1, leaving A incorrectly visible. This two-level chain exposes a failure not covered by the existing one-level regression test. Resolve activity against the complete record set, allowing a record to reactivate when its suppressor becomes inactive.
Useful? React with 👍 / 👎.
Implements #2813 (queue task 5812), the phase after the signed board object graph v1 (#2812, PR #3892 — merged).
What
All in
pkgs/id/src/boards.rs, extending the existing signed-object substrate:BoardObject::Theme): design tokens (name-validatedBTreeMap, ≤64 tokens, values ≤512 bytes of plain data), a closed layout enum (list/grid/compact), content-addressed asset references (metadata only, no embedded payloads), andforked_fromfork lineage, validated at insert to name an existing theme (never itself).Board.theme_refapplies a theme by reference. Themes deliberately name no board: a theme that carried its board's hash could never be referenced from that board's own root hash, so themes are portable signed packages and the board root names one. The referenced theme must already be in the graph when the board is accepted (publish the theme first). Pre-theme_refv1 board bodies still decode (#[serde(default)]+ skip-if-none, covered by a test).BoardGraph::compose_view): board layer (signed moderation log + author tombstones + resolved theme), host layer (caller-supplied hashes the serving host will not serve), personal layer (one viewer's hides + muted authors). Transparency rule — hidden ≠ erased: hidden objects stay in the graph, are omitted fromobjects, and are reported inhiddenwith the layer and, for board-layer hides, the moderation action / tombstone and its reason.moderation_loglists every moderation action for the board in every view: the log is public.Custom HTML templates remain out of v1: a theme is data, never markup.
Tests
Five new tests in the boards module: theme fork lineage (missing / non-theme / self references), theme shape rejection (bad token names, empty values, duplicate assets),
theme_refresolution + rejection + legacy body decode, the layered hide → restore → re-hide cycle with transparency assertions, and personal/host layer hides with no board record.CI is the verification route for this crate (no builds on the sandbox); targeted
cargo test -p id boardsis the suite these tests live in.Need help on this PR? Tag
@codesmith-botwith what you need.