review round 1: record PR #209's state, and close a census blind spot
Review finding (P2): docs/active-work.md and docs/agent-handoff.md both still said "PR not yet open". This file's own rule is that an open PR gets a lane recording its state — "When a PR is opened, give it a lane", written after #171 drifted 153 commits while invisible here. Both now carry PR #209, its six commits, and the first CI run. That run is worth recording rather than merely citing, because it discharges the one risk the framing could not close locally. ALL 14 CHECKS GREEN, including both new jobs (Test (crdt) 12m20s, M10 Perf Gates (crdt) 5m40s) and the macOS/luajit leg that is the documented flake surface. This was the first time in the project's history that any of these tests executed in CI. ACCEPTANCE 8 HOLDS AGAINST THE REAL RUN. Test (crdt) reported 3,717 passed / 0 failed / 30 ignored: the 3,746 all-targets census (with basedpyright NOT skipped, as it is locally) plus one doc test, less the 30 ignored. The job demonstrably compiled and ran the crdt corpus rather than reporting green over nothing — which was the whole point of writing that criterion while the local sweep was already green. A trap for the next reader is recorded with it: DO NOT SUBTRACT THE TWO JOBS' TOTALS. Test (ubuntu/luajit) reports 3,485 and Test (crdt) 3,747, a difference of 262 rather than 279, because the jobs run different SETS — the non-crdt job adds pmacs-protocol's 17 tests. The dark count is the all-targets comparison, 3,746 - 3,467. The wrong number looks entirely plausible. Chasing that discrepancy found a real gap, and one this lane's own tool could never have surfaced: PMACS-PROTOCOL HAS ITS OWN `crdt` FEATURE. It gates no tests, so a per-test census reports 17 either way and scores the crate as unaffected — but it changes `cfg!(feature = "crdt")` EXPRESSIONS inside InstanceCapabilities::default and FrontendCapabilities::default, so those same 17 tests exercise different runtime values under it. CI had only ever run the non-crdt ones. Closed with an explicit `-p pmacs-protocol --all-targets --features crdt` step, verified 17/17. The blind spot is structural, not an omission, so it is documented at the tool rather than fixed in it: scripts/feature-census censuses the workspace DEFAULT MEMBER, because that is what a bare `cargo test --all-targets` builds. Sibling crates are invisible to it no matter what configs are passed. The durable lesson: A FEATURE CAN MATTER TO A CRATE A PER-TEST CENSUS SCORES AS UNAFFECTED. Check siblings by hand. Verified: ci.yml parses at 10 jobs, script syntax clean, fmt, diff-check, and the new protocol step green locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
57abcd940e
commit
6519bc3461
|
|
@ -340,6 +340,20 @@ jobs:
|
|||
PMACS_REQUIRE_SETSID: "1"
|
||||
PMACS_REQUIRE_BASH: "1"
|
||||
- run: cargo test --doc --no-default-features --features luajit,crdt
|
||||
# pmacs-protocol under `crdt`, which nothing else runs. Its own
|
||||
# feature is NOT inert: it gates no tests, so a test census sees
|
||||
# 17 either way, but it changes `cfg!(feature = "crdt")`
|
||||
# EXPRESSIONS inside `InstanceCapabilities::default` and
|
||||
# `FrontendCapabilities::default` — so the same 17 tests exercise
|
||||
# different runtime values. CI had only ever run the non-crdt
|
||||
# ones.
|
||||
#
|
||||
# This is also a blind spot of scripts/feature-census by
|
||||
# construction: it lists the workspace DEFAULT MEMBER (`pmacs`),
|
||||
# so no per-test census of the root package can ever surface a
|
||||
# sibling crate's coverage. The `test` job carries the same
|
||||
# explicit invocation for the same underlying reason.
|
||||
- run: cargo test -p pmacs-protocol --all-targets --features crdt
|
||||
|
||||
acceptance:
|
||||
name: M1 Acceptance Gates
|
||||
|
|
|
|||
|
|
@ -148,14 +148,38 @@ form. All four steps ran clean. **The two-argument form still does not
|
|||
work** for a remote-only branch (`fatal: invalid reference`), which is
|
||||
why every lane below spells out the `-b` form.
|
||||
|
||||
## CI CRDT coverage lane — IMPLEMENTED on `ci-crdt-coverage`, PR not yet open
|
||||
## CI CRDT coverage lane — **PR #209 OPEN, all 14 checks green**
|
||||
|
||||
**This lane had no branch and no owner from #166 until 2026-08-01.** It
|
||||
now has both. Framing:
|
||||
`docs/ci-crdt-coverage-framing.md` revision 3 (approved at revision 2).
|
||||
now has both, and a PR. Framing:
|
||||
`docs/ci-crdt-coverage-framing.md` revision 4 (approved at revision 2).
|
||||
Branch `ci-crdt-coverage` off `githubsucks/main` @ `4223dd3`, developed
|
||||
in the primary checkout on the laptop, not a worktree.
|
||||
|
||||
- **PR: <https://github.com/levineuwirth/pmacs/pull/209>**, opened
|
||||
2026-08-01, awaiting user review. Six commits: `7a9cf5b` clippy,
|
||||
`06abbac` m10-perf-gates, `7a8746d` crdt-test, `a776bc3`
|
||||
feature-census, `57abcd9` docs, plus a review-round commit for the
|
||||
pmacs-protocol gap below.
|
||||
- **First CI run: all 14 checks green** (run `30705124856`), including
|
||||
both new jobs — `Test (crdt)` 12m20s and `M10 Perf Gates (crdt)`
|
||||
5m40s — and the macOS/luajit leg that is the usual flake surface.
|
||||
**This was the first time in the project's history that any of these
|
||||
tests executed in CI.**
|
||||
- **Acceptance criterion 8 — the count reconciliation — HOLDS against
|
||||
the real run.** `Test (crdt)` reported **3,717 passed / 0 failed / 30
|
||||
ignored**. That is 3,746 (the `--all-targets` census, with
|
||||
`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.
|
||||
|
||||
- **Root cause, unchanged:** `.github/workflows/ci.yml` never enabled
|
||||
the `crdt` feature anywhere. Every `#[cfg(feature = "crdt")]` test was
|
||||
therefore **not compiled** in CI, not merely skipped — including the
|
||||
|
|
|
|||
|
|
@ -94,11 +94,14 @@ anchor, so every item is startable.
|
|||
|
||||
#### Open lanes (branch exists, work not finished)
|
||||
|
||||
- **CRDT half of the corpus is dark in CI — LANE OPENED 2026-08-01**
|
||||
(`ci-crdt-coverage`, `docs/ci-crdt-coverage-framing.md` rev 3;
|
||||
implemented, PR not yet open). CI never enabled `crdt`, so those tests
|
||||
were *not compiled*, not merely skipped — including 186 library tests
|
||||
behind a **required** `CLAUDE.md` gate CI had never run.
|
||||
- **CRDT half of the corpus is dark in CI — PR #209 OPEN, all 14 checks
|
||||
green** (`ci-crdt-coverage`, `docs/ci-crdt-coverage-framing.md` rev 4;
|
||||
opened 2026-08-01, awaiting user review). CI never enabled `crdt`, so
|
||||
those tests were *not compiled*, not merely skipped — including 186
|
||||
library tests behind a **required** `CLAUDE.md` gate CI had never run.
|
||||
The first run reported **3,717 passed / 0 failed / 30 ignored** in
|
||||
`Test (crdt)`, reconciling exactly to the census, so the job is
|
||||
demonstrably not vacuous.
|
||||
**Re-measure before quoting a number**, and there is now a tool:
|
||||
`scripts/feature-census luajit luajit,crdt`. Measured 279 dark at
|
||||
`4223dd3`; 275 recovered, 4 excluded with stated reasons. Two
|
||||
|
|
|
|||
|
|
@ -1,7 +1,26 @@
|
|||
# Framing — the CRDT half of the test corpus is dark in CI
|
||||
|
||||
**Revision 3.** Status: **implemented** on branch `ci-crdt-coverage`,
|
||||
based on `githubsucks/main` @ `4223dd3` (#208). Approved at revision 2.
|
||||
**Revision 4.** 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.
|
||||
|
||||
**Revision 3 → 4** records the first CI run and one review finding:
|
||||
|
||||
- **Acceptance 8 holds against the real run.** `Test (crdt)` reported
|
||||
**3,717 passed / 0 failed / 30 ignored** — 3,746 census (with
|
||||
`basedpyright` not skipped as it is locally) + 1 doc test − 30
|
||||
ignored. The job compiled and ran the corpus; it did not report green
|
||||
over nothing. §1.7's warning that the local green proved nothing about
|
||||
hosted runners is now **discharged by evidence** rather than still
|
||||
outstanding.
|
||||
- **A gap `feature-census` structurally could not see**: pmacs-protocol
|
||||
has its own `crdt` feature. It gates no tests — the census is 17
|
||||
either way — but it changes `cfg!(feature = "crdt")` *expressions*
|
||||
inside the capability defaults, so those 17 tests exercise different
|
||||
runtime values under it, and CI had only ever run the non-crdt ones.
|
||||
Closed by an explicit `-p pmacs-protocol --features crdt` step. **A
|
||||
feature can matter to a crate a per-test census scores as
|
||||
unaffected**, and the script's header now says so.
|
||||
|
||||
**Revision 2 → 3** records implementation findings, not a new design
|
||||
round. Three things changed:
|
||||
|
|
|
|||
|
|
@ -63,6 +63,20 @@
|
|||
# what a fix delivers, so ignored tests are counted separately via a
|
||||
# second `--list --ignored` pass.
|
||||
#
|
||||
# SCOPE LIMIT, and it is structural rather than an omission: this
|
||||
# censuses the workspace DEFAULT MEMBER only (`pmacs`), because that is
|
||||
# what a bare `cargo test --all-targets` builds. Sibling crates
|
||||
# --- pmacs-protocol, pmacs-gpu --- are invisible to it no matter what
|
||||
# configs are passed.
|
||||
#
|
||||
# That blind spot is not hypothetical. pmacs-protocol has its own
|
||||
# `crdt` feature which gates NO tests, so a census of it would report
|
||||
# zero dark either way --- yet the feature changes
|
||||
# `cfg!(feature = "crdt")` expressions inside the capability defaults,
|
||||
# so its 17 tests exercise different runtime values under it. A feature
|
||||
# can therefore matter to a crate this script would score as
|
||||
# unaffected. Check sibling crates by hand.
|
||||
#
|
||||
# CARGO_TERM_COLOR is pinned off so escape codes cannot break the
|
||||
# anchored matches, the same precaution and for the same reason as
|
||||
# scripts/bite.
|
||||
|
|
|
|||
Loading…
Reference in New Issue