From 71a1ebd4b708fe510c2bab84a55ea19776f184f2 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 1 Aug 2026 11:37:19 -0400 Subject: [PATCH] review round 2: assert the CRDT capability defaults, not their round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding (P2), and it is this lane's own defect class one level down. Round 1 added a `-p pmacs-protocol --features crdt` step so that crate compiles both ways. That step EXECUTED `InstanceCapabilities::default` in both configurations and asserted NOTHING about it: the crate's only use of that value is a transport round-trip, and a round-trip is invariant to the values. An all-false default — or one whose three fields disagreed with each other — encodes, decodes, and passes identically in both builds. Running code is not testing it. That is the same sentence this whole lane is about, and round 1 committed the smaller version of it while fixing the larger one. Three tests now pin the defaults, and the split is the point: * under `crdt`: multi_frontend, crdt_replica and semantic_render all default true. Advertising false on a CRDT build would strand every frontend in single-frontend mode. * without `crdt`: all three default false. Advertising true would be wire-protocol false advertising — those code paths are conditionally compiled out. * FrontendCapabilities::default is all-false in BOTH builds, and this test is DELIBERATELY NOT feature-gated. That third one pins an asymmetry nothing else did. FrontendCapabilities derives Default and is feature-INVARIANT, while InstanceCapabilities is feature-DEPENDENT. It is load-bearing rather than an oversight: an instance advertises what it can do, a frontend OPTS IN through the negotiation handshake, and a v1 frontend has no local CRDT state regardless of how the crate it links was compiled. Making the frontend side track the feature would have frontends claiming support they do not have. A future edit that "makes them consistent" now fails a test that says why not to. All three fields are asserted separately rather than by comparing whole structs, because they track one `cfg!` and a change flipping only some of them is exactly the regression worth catching. Bite-verified rather than assumed: mutating `multi_frontend` to a literal false gives `FAILED. 18 passed; 1 failed` with the expected assertion message; restoring returns 19/19. Both configurations now report 19 tests, up from 17, with the correct cfg-gated test running in each. Left untested and recorded instead: `InstanceCapabilities::crdt_replica` carries `#[serde(default = "default_true")]`, a THIRD default mechanism that is unconditional and therefore disagrees with the `Default` impl in a non-CRDT build. Exercising it needs a self-describing format and this crate's only serde dependency is postcard, which is not one. Adding serde_json as a dev-dependency to test a divergence this lane did not introduce is scope creep. Also corrects a note that round 1 made stale: the ledger's "do not subtract the two jobs' totals" figures (3,485 and 3,747) were measured BEFORE round 1 added the protocol step, and round 2 adds two tests to that crate. Expected totals are now 3,487 and 3,766. The root-package census is untouched at 3,467 / 3,746 — the new tests live in a sibling crate, which is precisely the region scripts/feature-census cannot see. Verified: fmt, diff-check, clippy on pmacs-protocol in both feature configurations, and workspace clippy --features crdt --keep-going. Co-Authored-By: Claude Opus 5 (1M context) --- docs/active-work.md | 21 ++++++--- docs/ci-crdt-coverage-framing.md | 34 +++++++++++++- pmacs-protocol/src/message.rs | 76 ++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 8 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 64b20d2..a29554a 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -172,13 +172,20 @@ in the primary checkout on the laptop, not a worktree. `basedpyright` not skipped as it is locally) plus 1 doc test, minus the 30 ignored: 3,716 + 1 = 3,717. The job demonstrably compiled and ran the crdt corpus rather than reporting green over nothing. -- **Do not compare the two jobs' raw totals.** `Test (ubuntu/luajit)` - reports 3,485 and `Test (crdt)` 3,747 — a difference of 262, not 279. - The jobs run different *sets*: the non-crdt job adds pmacs-protocol's - 17 tests, and 3,467 + 1 + 17 = 3,485 against 3,746 + 1 = 3,747. The - dark count is the all-targets comparison, 3,746 − 3,467 = **279**. - A reviewer who subtracts the job totals gets a wrong number that looks - plausible. +- **Do not compare the two jobs' raw totals.** In the first run + `Test (ubuntu/luajit)` reported 3,485 and `Test (crdt)` 3,747 — a + difference of 262, not 279, because the jobs ran different *sets*: + 3,467 + 1 doc + 17 protocol = 3,485, against 3,746 + 1 doc = 3,747. + **The dark count is the all-targets comparison, 3,746 − 3,467 = 279.** + A reviewer who subtracts job totals gets a wrong number that looks + entirely plausible. + - **Those two figures are already superseded** and are kept only to + explain the trap. Review round 1 added a pmacs-protocol step to + `crdt-test`, and round 2 added two capability tests to that crate, + so the expected totals are now **3,487** (3,467 + 1 + 19) and + **3,766** (3,746 + 1 + 19). The root-package census is untouched at + 3,467 / 3,746 — the new tests live in a sibling crate, which is + exactly the region `scripts/feature-census` cannot see. - **Root cause, unchanged:** `.github/workflows/ci.yml` never enabled the `crdt` feature anywhere. Every `#[cfg(feature = "crdt")]` test was diff --git a/docs/ci-crdt-coverage-framing.md b/docs/ci-crdt-coverage-framing.md index c4fe246..c53bbfc 100644 --- a/docs/ci-crdt-coverage-framing.md +++ b/docs/ci-crdt-coverage-framing.md @@ -1,6 +1,6 @@ # Framing — the CRDT half of the test corpus is dark in CI -**Revision 4.** Status: **PR #209 open, all 14 checks green on the first +**Revision 5.** Status: **PR #209 open, all 14 checks green on the first run.** Branch `ci-crdt-coverage`, based on `githubsucks/main` @ `4223dd3` (#208). Approved at revision 2. @@ -22,6 +22,38 @@ run.** Branch `ci-crdt-coverage`, based on `githubsucks/main` @ feature can matter to a crate a per-test census scores as unaffected**, and the script's header now says so. +**Revision 4 → 5** closes review round 2, and it is this lane's own +defect class one level down. + +- **Running the code was not testing it.** Round 1's + `-p pmacs-protocol --features crdt` step *executed* + `InstanceCapabilities::default` in both configurations, but the crate's + only use of that value was a **transport round-trip** — and a + round-trip is invariant to the values. An all-false default, or one + whose three fields disagreed, encodes and decodes just as happily and + passes in both builds. The step added execution and no assertion. +- **Three tests now pin it**, and their split matters: one asserts all + three fields `true` under `crdt`, one asserts all three `false` + without it, and a third — **deliberately not feature-gated** — asserts + `FrontendCapabilities::default` is all-false in *both* builds. +- **That third test pins an asymmetry nothing else did.** + `FrontendCapabilities` derives `Default` and is feature-**invariant**; + `InstanceCapabilities` is feature-**dependent**. This is load-bearing, + not an oversight: an instance advertises what it can do, a frontend + **opts in** through negotiation, and a v1 frontend has no local CRDT + state regardless of how the crate it links was compiled. +- **Bite-verified rather than assumed**: mutating `multi_frontend` to a + literal `false` produced `FAILED. 18 passed; 1 failed` with the + expected assertion message; restoring returned 19/19. +- **Left untested, deliberately:** `InstanceCapabilities::crdt_replica` + carries `#[serde(default = "default_true")]`, a *third* default + mechanism that is unconditional and therefore disagrees with the + `Default` impl in a non-CRDT build. Exercising it needs a + self-describing format, and this crate's only serde dependency is + postcard, which is not one. Adding `serde_json` as a dev-dependency to + test a divergence this lane did not introduce is scope creep; it is + recorded here instead. + **Revision 2 → 3** records implementation findings, not a new design round. Three things changed: diff --git a/pmacs-protocol/src/message.rs b/pmacs-protocol/src/message.rs index bf95001..92ccb7a 100644 --- a/pmacs-protocol/src/message.rs +++ b/pmacs-protocol/src/message.rs @@ -2296,3 +2296,79 @@ pub enum InitialTargetResult { message: String, }, } + +#[cfg(test)] +mod capability_default_tests { + use super::{FrontendCapabilities, InstanceCapabilities}; + + // These exist because running code is not testing it. + // + // The `crdt` feature reaches this crate for exactly one purpose: + // `InstanceCapabilities::default` reads `cfg!(feature = "crdt")`, + // so the same source produces different advertised capabilities in + // the two builds. Until these tests, the only thing exercising that + // default was a transport round-trip, and a round-trip is INVARIANT + // TO THE VALUES — an all-false default, or one where the three + // fields disagreed with each other, encodes and decodes just as + // happily and passes in both configurations. + // + // So the crate was being COMPILED both ways without either set of + // values being asserted. That is the same defect class this lane + // exists to close, one level down: the CI step that runs + // pmacs-protocol under `crdt` executed this code but checked + // nothing about it. + + /// The instance advertises CRDT capability exactly when it was + /// built with the feature. Advertising `true` on a non-CRDT build + /// would be wire-protocol false advertising — the CRDT paths are + /// conditionally compiled out — and advertising `false` on a CRDT + /// build would strand every frontend in single-frontend mode. + #[cfg(feature = "crdt")] + #[test] + fn instance_capability_defaults_are_enabled_under_crdt() { + let caps = InstanceCapabilities::default(); + assert!(caps.multi_frontend, "multi_frontend must default true"); + assert!(caps.crdt_replica, "crdt_replica must default true"); + assert!(caps.semantic_render, "semantic_render must default true"); + } + + /// The non-CRDT counterpart. All three track one `cfg!`, so a + /// change that flipped only some of them would leave the daemon + /// advertising a capability whose code paths are compiled out. + #[cfg(not(feature = "crdt"))] + #[test] + fn instance_capability_defaults_are_disabled_without_crdt() { + let caps = InstanceCapabilities::default(); + assert!(!caps.multi_frontend, "multi_frontend must default false"); + assert!(!caps.crdt_replica, "crdt_replica must default false"); + assert!( + !caps.semantic_render, + "semantic_render must default false; a semantic session is \ + necessarily a text replica and a non-CRDT build hosts neither" + ); + } + + /// **Deliberately NOT feature-gated** — this asserts the same thing + /// in both builds, which is the point. + /// + /// `FrontendCapabilities` derives `Default`, so it is + /// feature-INVARIANT while `InstanceCapabilities` is + /// feature-DEPENDENT. That asymmetry is load-bearing rather than an + /// oversight: an instance advertises what it can do, while a + /// frontend OPTS IN through the negotiation handshake, and a v1 + /// frontend has no local CRDT state regardless of how the crate it + /// links was compiled. Making this one track the feature would have + /// frontends claiming support they do not have. + #[test] + fn frontend_capability_defaults_do_not_track_the_crdt_feature() { + let caps = FrontendCapabilities::default(); + assert!( + !caps.multi_frontend, + "frontend multi_frontend must default false in BOTH builds" + ); + assert!( + !caps.crdt_replica, + "frontend crdt_replica must default false in BOTH builds" + ); + } +}