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" + ); + } +}