review round 2: assert the CRDT capability defaults, not their round-trip

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) <noreply@anthropic.com>
This commit is contained in:
Levi Neuwirth 2026-08-01 11:37:19 -04:00
parent 6519bc3461
commit 71a1ebd4b7
No known key found for this signature in database
3 changed files with 123 additions and 8 deletions

View File

@ -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

View File

@ -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:

View File

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