fix(server): redact both sides of a diff query, and bind it to a channel (C27) - #375
Open
vieiralucas wants to merge 9 commits into
Open
fix(server): redact both sides of a diff query, and bind it to a channel (C27)#375vieiralucas wants to merge 9 commits into
vieiralucas wants to merge 9 commits into
Conversation
…nel (C27) `DiffQuery` resolved two states — two named versions, or two materialized branches — and diffed them raw, replying with `Change`s carrying full `core::path`s and the scalar values at them. Its gate was the room-read tier with the doc-ACL tier abstaining, and it was not channel-bound, so the subscription's zone scope was never consulted. A zone-limited reader read any withheld partition as a diff; `DiffKind::Branches` needed no versions to do it. The frame is now channel-keyed like a version fetch, and both sides go through `project_served_state` before the engine sees them — so a served change list is the diff of the two states that reader would itself have been handed, and a partition it may not read contributes no change at all. `Hub::diff_versions` / `diff_branches` take the per-side redaction as an argument, so a diff cannot reach the engine without its caller stating what it narrows by. `authz_room` had no `DiffQuery` arm, so a diff ran with no acting schema — zone declarations live there, so the zone projection would never have run.
… on an unreadable archive Two seams where the diff still did not match the version fetch it now mirrors. `Action::VersionRead`'s own definition says "branch/version/version-diff read", and nothing recorded one for a diff — so reading a version through a change list was the untraced way to read it. And an archived side that does not decode failed the query through `internal`, which drops the connection; C15 had already reasoned that one unreadable archive says nothing about the live stream a channel carries. Tests: the branch diffs gain positive controls so an empty answer cannot come from the two branches agreeing; a mark anchored in the hidden zone (the change kind that carries no path, which is why the redaction runs over states rather than over the change list); a structural add as the hidden change; an element-scoped deny whose target has left the live room, so the per-side index is what the test needs.
…herwise The arm's header still called an undecodable side a closing fault; `request_denied` and the clone comment still named the diff among the room-keyed requests; and the DECISIONS entry named a `narrowable` test this seam does not use and version management among the frames C34 covers.
… guard nothing reaches The wasm client suite only builds under the wasm target, so it kept the old room-keyed call and would have failed the `wasm-sdk` job alone. `narrow` stops being fallible. Its one refusal — an undecodable side — was observationally unreachable: with an empty index the projections hand the bytes on and the engine's own decode refuses the query with the same error, so nothing distinguished the guard from its absence. A diff has no way to serve a side it cannot read, which is the whole reason the fetch needs a guard here and this seam does not. Also: the audit case covers `DiffKind::Branches`, the cheaper path to the same content; and four claims are scoped to what the code delivers — the served states are not byte-identical to a fetch's (the frontier scrub differs, the change list does not carry one), a closure argument is a nudge rather than an enforcement, the two seams' refusals share a code rather than a message, and the two audit records are not taken at the same point. Files C35: `DiffResult` carries no channel, so with the answers now genuinely per-channel a result cannot be attributed to the query that asked for it.
…, and pin a branch-bound channel `Hub::materialize_branch` answers `None` for three unrelated situations — an unknown name, a durable base this node cannot decode, and `main` on a room with no materialized state — and `diff_branches` maps all three to `UnknownBranch`. Four sentences written in this PR claimed the seam tells them apart. They now say what it does; the collapse is filed as C36, since undoing it has to keep the publish path's refusal on the middle case. Also: the arm's own comment still made the "the two states this reader would have been served" claim the last commit scoped everywhere else; "both sides come off durable storage" was never true of a branch diff, whose `main` side is the live replica; and the frame's rustdoc listed one of its four failure replies. Tests: a channel bound to a branch other than `main` still narrows by its own zone scope; the channel-scope test gains an in-fixture control; the audit test asserts each query actually answered with a diff.
… two seams review found behind it `project_read_paths` claimed an annotation whose anchor sequence the walk cannot resolve is kept only by a whole-document reader. It is kept by any reader the *root* verdict admits, which a schema root grant plus a doc-ACL subtree deny produces — so an orphaned mark survives its region being cut, through the catch-up snapshot, the version fetch and the diff alike. A change list only makes it legible. Filed as C37 rather than fixed here: failing closed on an unresolved anchor has to be decided against the op stream, which resolves an unresolvable target the same way and must agree or the two joins diverge. Also files C38 — a log-shared branch materializes from the retained log alone, so it silently loses everything below a compaction floor — and gives C36 the follower case. The prose that promised a redacted region contributes no change now says how far the projections actually reach. Tests: the fetch-oracle equality in both doc-ACL deny shapes, where the read projection's rules (moved-in un-purge, leaf cuts, tuple and annotation retains) are the ones that could make a projected pair diff differently from two fetches.
…he prose that says otherwise The filing scoped the projections' shortfall to annotations. The cause reaches further: both projections build their purge set by walking the live tree, and a deleted container keeps its registry entry — displacement retains, deliberately — which `encode_state` then emits. A diff never renders that (it structures from the root); the version fetch and the catch-up snapshot serve the bytes. The annotation is the face a diff shows, because the mark diff reads the whole registry. Seven sites asserted the root fallback reaches "only a whole-document reader" — five in the server's op-read-path prose, one in a core spec, one already corrected. A root grant carved by a subtree deny passes the root query, so they now say what the code does; the unit that picks up C37 has to read the op seam's prose first, and it was arguing the seam was already safe. Also: C36's follower case is the first of its three answers reached differently, not a fourth; C38's publish loss needs a restore-as-branch to have made a log-shared branch the active HEAD, since a `main` HEAD materializes through the live replica.
… callers read it Three sites still asserted the fallback reaches "only a whole-document reader", including one inside `op_read_path`'s own body sixty lines below the rustdoc that now says the opposite, and the two the C37 unit lands on first — the redacted fan-out and `Hub::element_paths`. The C37 entry claimed the correction was exhaustive by naming the files; it no longer enumerates them. Also drops a write grant from the diff fixture's docstring that only the version fixture it was copied from installs, and rewraps two comment paragraphs an edit grew in place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Message::DiffQueryresolved two states —DiffKind::Versionsover two named versions,DiffKind::Branchesover twomaterialize_branchresults — and diffed them raw, replying withChanges carrying fullcore::paths and the scalar values at them. Its gate was the room-read tier with the doc-ACL tier abstaining, and it was not channel-bound, so the subscription's zone scope was never consulted and the caller need not have been subscribed at all. A zone-limited reader read any withheld partition as a diff (createva, wait for a write into the hidden zone, createvb, diff);DiffKind::Branchesneeded no versions to do it. Both are pinned as tests that fail againstmain.DiffQuery { channel, kind, a, b }, room resolved from the subscription like a version fetch; the reply stays keyed by that room. Chosen over a room-plus-zone gate of its own, which would put a second answer to "which partitions may this reader see" beside the one a bound channel holds — the driftzone_narrowingexists to prevent, and C15 pinned that a fetch narrows by the channel's set, not the actor's entitlement. Names the rule the surface already implied: a request serving a room's content is channel-keyed; one serving names or mutating may ride a room off the frame.project_served_statebefore the engine sees them, so a served change list is by constructionpath::diffof the two states that reader would itself have been handed — pinned directly against the fetch seam. Filtering the change list after the diff was rejected: a mark change carries no path at all, and a filter is a second home for the redaction rule.Hub::diff_versions/diff_branchestake the per-side redaction as an argument, so a diff cannot reach the engine without its caller stating what it narrows by. No recipient is passed — a diff is never adopted as state and a change list carries no frontier, so the scrub goes whole.authz_roomhad noDiffQueryarm, so a diff ran with no acting schema. Fail-closed for the gate, but zone declarations live in the schema, so the zone projection would never have run. The room-keyed management frames are still in that blind spot — filed as C34.Action::VersionReadwhen the change list goes out (the audit action's own definition already said "version-diff read", and the diff was the untraced way to read a version), and an archived side that does not decode refuses without closing — the engine always failed such a query, but throughinternal, which drops a live stream over an unreadable archive.Mutation sweep over every predicate introduced, each anchored to the
DiffQueryarm and asserted to land inside it, with the version-fetch suites as the sibling-seam control (they never move): no-narrowing 14 kills, drop-zone-scope 8 (zone half only), drop-acl-records 6 (doc-ACL half only), empty-gate-index 2 (the two element-scoped-deny tests), no-acting-schema 14, gateRead→Write7, drop-diff-audit 1, close-on-error 4. RecipientNone→Someis 0 kills by design — the documented claim is that a change list carries no frontier.Five cold review passes; each found something the previous approved, including a stale call in
crates/wasm/tests/web.rsthat only compiles under the wasm target and would have failed thewasm-sdkjob alone. Four adjacent defects are filed rather than folded in: C34 (room-keyed management frames authorize under no acting schema — fail-closed), C35 (DiffResultcarries no channel, so with the answers now per-channel a result cannot be attributed to its query), C36 (materialize_branch's three-wayNonecollapse reports an unreadable branch base as an absent branch), C37 (an annotation whose anchor has left the tree survives both read projections — the one region a redacted state still carries, reaching every state-serving seam, and the reason the "contributes no change at all" claim is now scoped), C38 (a log-shared branch materializes truncated past a compaction floor).Specs:
crates/server/tests/diff_projection.rs(17), reworkeddiff_query.rs(8, incl. a diff on an unbound channel), two undecodable-side cases inversion_unreadable.rs, a diff-audit case inaudit_query.rs, channel-keyed frames acrossprotocol_diff.rs/client_diff.rs/FFI/wasm/Go/Python/JS. Docs: ARCHITECTURE §Schema-Aware Diff, DECISIONS 2026-07-29, KANBAN C27→Done + C34–C38 filed.