merge: integrate main @ 5e186c7 after revision 7
Absorb #193 without rewriting the reviewed revision 6 or revision 7 commits. Resolve docs/active-work.md by retaining PR #188's proposed lane and accepting main's removal of the landed bite lane. Record that the intervening base change touches no product source, so revision 7's source readings remain current.
This commit is contained in:
commit
f6dad71190
|
|
@ -0,0 +1,507 @@
|
|||
# Testing audit — gaps and shortcomings
|
||||
|
||||
Audited 2026-07-29 at `7a3a55d` (branch `journey-stage1a-directory-open`,
|
||||
even with `main` for everything cited here). Scope: testing coverage and
|
||||
test quality only, plus CI-pipeline improvements (added mid-audit at the
|
||||
user's request). This document is the raw material for a future testing
|
||||
arc; each numbered lane in §8 still needs its own framing doc per the
|
||||
workflow, and coherence-affecting lanes must state their `COHERENCE.md`
|
||||
§20 impact there, not here.
|
||||
|
||||
**Every count in this file is a reading, not a constant.** The dark-test
|
||||
census in `docs/active-work.md` ("The CRDT half of the test corpus is
|
||||
dark in CI — NEEDS A LANE") moved three times in three weeks. Re-measure
|
||||
before acting; the appendix gives the commands.
|
||||
|
||||
Method: five parallel read-only audits (dark/gated tests; unit-coverage
|
||||
map; Lua-layer coverage; test-quality/flake patterns; CI-vs-local-gate
|
||||
delta), findings cross-checked against `docs/active-work.md`,
|
||||
`docs/agent-handoff.md` §3/§5, and the memory of prior review rounds.
|
||||
Claims that accuse an existing doc of being wrong were re-verified by
|
||||
hand before inclusion.
|
||||
|
||||
---
|
||||
|
||||
## 0. Baseline: what exists
|
||||
|
||||
- Root `pmacs` package: ~2,030 unit tests in `src/`, 92 integration
|
||||
targets in `tests/` (~93k lines, ~1,426 tests), shared harness in
|
||||
`tests/common/` (daemon + PTY, both high quality).
|
||||
- `pmacs-gpu`: 202 unit tests, including a real headless wgpu render
|
||||
harness with ~20 pixel-asserting tests. No `tests/` dir; bin-only, so
|
||||
doctests are structurally impossible.
|
||||
- `pmacs-protocol`: 17 unit tests (12 transport, 5 terminal). No
|
||||
`tests/` dir.
|
||||
- `builtin/`: 16,445 lines of Lua across 31 files. No Lua test
|
||||
framework of any kind; all coverage is indirect via Rust `eval()`.
|
||||
- Property tests: 5 proptest sites, all in the root crate (CRDT
|
||||
convergence, rope≡CRDT projection, text_view inverse, keymap stack,
|
||||
manifest round-trip). One checked-in regression seed (`crdt.txt`).
|
||||
No fuzzing anywhere.
|
||||
- CI: one workflow, 8 jobs / 12 legs, ~14.5 min wall per run, triggers
|
||||
on `push:main` + `pull_request` only. No coverage measurement, no
|
||||
scheduled runs, no branch protection on `main` (verified via API:
|
||||
404, so every job is advisory).
|
||||
|
||||
The suite is unusually thoughtful in places — the daemon harness's
|
||||
connect-based readiness probe, the `PMACS_REQUIRE_GPU` hard-fail
|
||||
pattern, `dual_mode_test!`, the encoding-stability pins, `scripts/bite`
|
||||
itself. Most defects below are **inconsistent application of patterns
|
||||
the project already invented**: the correct helper exists in one file
|
||||
and a degraded copy exists in another.
|
||||
|
||||
---
|
||||
|
||||
## 1. Dark tests — things that never run
|
||||
|
||||
### 1.1 CRDT (known; already a scoped lane — do not re-scope here)
|
||||
|
||||
`docs/active-work.md` §"The CRDT half of the test corpus is dark in CI"
|
||||
carries the full census (273 dark at `74301d1`: 185 library + 88
|
||||
integration), the two-part fix shape, and the two standing obstacles
|
||||
(`clippy --features crdt` is red on `main`; the `setsid_escapee` ~1-in-5
|
||||
load flake). Defer to it. What this audit **adds** to that lane:
|
||||
|
||||
- **Nothing anywhere compiles the 8 `#![cfg(feature = "crdt")]`
|
||||
integration targets** — not CI, and not the local gates either:
|
||||
`cargo test --lib --features crdt` builds the library only. The
|
||||
`*_crdt_acceptance.rs` / `m10_*` files are type-checked exclusively
|
||||
on developer machines that happen to run them by name. CI builds
|
||||
them as empty green binaries, which reads as "passed" in the log.
|
||||
- `tests/m10_11_perf.rs` puts its `#![cfg(feature = "crdt")]` at line
|
||||
60, after the module doc — any future head-of-file scan for the gate
|
||||
will misclassify it (the "never classify from truncated output" trap,
|
||||
now in file-layout form).
|
||||
- `src/buffer.rs:2984` — the parked CRDT no-op-replace undo bug is
|
||||
doubly dark (`#[ignore]` + crdt) and its own comment says un-ignoring
|
||||
it is step one of the fix. It needs a named owner or it will sit
|
||||
forever.
|
||||
|
||||
### 1.2 Vacuous-green skips: tests that pass without executing
|
||||
|
||||
The `let Ok(_) = which_binary(x) else { eprintln!(...); return; }` shape
|
||||
passes green when the tool is absent. **21 external-tool-gated tests; 15
|
||||
have never executed their bodies in CI** because `ci.yml` installs none
|
||||
of the tools (the only `apt-get install` in the workflow is lavapipe):
|
||||
|
||||
- All 9 real-language-server tests in `tests/m4_acceptance.rs`
|
||||
(rust-analyzer ×2, basedpyright, clangd ×2, gopls ×2, json-ls,
|
||||
yaml-ls) — the only real-LSP coverage in the suite.
|
||||
- The entire M6.8 multi-REPL acceptance (6/6 tests, gated on a
|
||||
lua/luajit binary) plus the zsh/fish/lua shell-spawn tests in
|
||||
`tests/m6_5_repl_acceptance.rs`.
|
||||
|
||||
Same pathology, already ledger-documented: `a37` in
|
||||
`tests/vterm_stage3_acceptance.rs` is triple-dark (crdt-gated; skips
|
||||
when `pmacs-gpu` isn't built; `PMACS_REQUIRE_GPU` is only set on a job
|
||||
that tests a *different package*). It has never executed in CI.
|
||||
|
||||
**The project already owns the fix pattern**: `PMACS_REQUIRE_GPU`
|
||||
promotes a silent skip to a hard failure, and CI arms it on the
|
||||
`gpu-render` job. There is no `PMACS_REQUIRE_LSP` / `PMACS_REQUIRE_SHELLS`
|
||||
analogue, and no CI step installs the cheap tools (gopls, clangd, zsh,
|
||||
fish, lua are all one `apt-get`/`brew` line; basedpyright is an npm
|
||||
install).
|
||||
|
||||
Related mid-test variant: `tests/m4_acceptance.rs:5783-5789` skips its
|
||||
only assertion when rust-analyzer is "likely still indexing" — the
|
||||
assertion vanishes exactly when the system is slow, which is when a
|
||||
regression would show.
|
||||
|
||||
### 1.3 Ignored tests owned by nobody
|
||||
|
||||
27 `#[ignore]` tests; 11 run in the four `--ignored` CI jobs; **16 run
|
||||
nowhere**:
|
||||
|
||||
- `src/rope.rs:1101,1112,1130` (3 perf smokes) and `src/buffer.rs:2984`
|
||||
— `cargo test --lib` never passes `--ignored`; these are dead code
|
||||
that still compiles.
|
||||
- `tests/m3_acceptance.rs:218` (`m3_6_grep_kernel_under_2s_on_8_cores`)
|
||||
— `#[ignore]` + needs `PMACS_KERNEL_PATH` + no job names
|
||||
`m3_acceptance --ignored`.
|
||||
- The m10_2/m10_11 perf and doubled-PTY operator tests (crdt-gated on
|
||||
top of `#[ignore]`; the doubled-PTY trio is deliberately
|
||||
operator-invoked — fine, but nothing records when it last ran).
|
||||
|
||||
### 1.4 CI runs weakened versions of the gates it does run
|
||||
|
||||
CI green currently certifies less than the spec text implies:
|
||||
|
||||
- `async_runtime_soak_lifecycle_stable` runs at **10 s vs its 3600 s
|
||||
spec gate** (`PMACS_SOAK_SECS`); the hour-long soak has never run.
|
||||
- M6 ingest gate: CI floor 64 MB/s vs the 100 MB/s default; cancel
|
||||
gate: 30 trials/500 ms vs 100/5000. Documented in `ci.yml` comments
|
||||
as a runner-budget tradeoff, but nothing anywhere runs the full
|
||||
profile, ever.
|
||||
- The soak's FD and RSS assertions silently no-op on macOS
|
||||
(`/proc`-dependent) while the test reports green.
|
||||
|
||||
---
|
||||
|
||||
## 2. Coverage gaps by layer
|
||||
|
||||
### 2.1 pmacs-protocol: the wire format is tested from the wrong crate
|
||||
|
||||
`pmacs-protocol/src/message.rs` — 2,100 lines: `InstanceMessage`,
|
||||
`FrontendEvent`, `Hello`, capability negotiation, version constants —
|
||||
has **zero tests in its own crate**. The real coverage (117 tests:
|
||||
postcard round-trips, v16–v19 encoding-stability pins, negotiation
|
||||
matrix) lives in the root package's `src/protocol.rs`. Consequences:
|
||||
|
||||
- `cargo test -p pmacs-protocol` certifies almost nothing (17 tests).
|
||||
- A future consumer that depends on `pmacs-protocol` alone inherits an
|
||||
effectively untested crate.
|
||||
- CI's `-p pmacs-protocol --all-targets` step cannot run doctests
|
||||
(`--all-targets` excludes them) — zero exist today, so this is a trap
|
||||
armed for the first doctest someone writes.
|
||||
|
||||
Also: local `cargo fmt --check` (no `--all`) formats the root package
|
||||
only, and local clippy via `--workspace` covers `pmacs-protocol` while
|
||||
CI's clippy never does — each side has a hole the other covers.
|
||||
|
||||
### 2.2 Root crate: zero-test production files
|
||||
|
||||
Largest files with **no `#[cfg(test)]` at all**:
|
||||
`src/terminal/session.rs` (854 — the process/session registry),
|
||||
`src/lua_bindings/window_panel.rs` (796), `src/lua_bindings/mcp.rs`
|
||||
(602), `src/lua_bindings/index.rs` (395), `src/lua_bindings/fold.rs`
|
||||
(390), `src/lua_bindings/diag.rs` (237), `src/audit/rules.rs` (154).
|
||||
Thinnest large files: `src/mcp.rs` (2.5 tests/kloc),
|
||||
`src/lua_bindings/mod.rs` (13,955 prod lines, 10.6/kloc — also the
|
||||
F-016 split candidate), `src/daemon.rs` (9.7/kloc). The lua_bindings
|
||||
submodules are integration-covered via acceptance suites but have no
|
||||
unit-level pins; per the project's own review history ("pin where
|
||||
production CALLS"), that leaves the binding seam itself unpinned.
|
||||
|
||||
The LSP client (`src/lsp.rs`) unit-tests only wire mechanics (framing,
|
||||
URIs, position encoding); server lifecycle, capability handling, and
|
||||
the diagnostics pipeline are covered solely through the fake-LSP
|
||||
acceptance path — plus the 9 real-server tests that never run (§1.2).
|
||||
|
||||
### 2.3 pmacs-gpu: the live half is untested
|
||||
|
||||
Well covered: headless render, math layout/parse, attach negotiation,
|
||||
coalescing, minimap, tab projection. Untested: the winit
|
||||
`ApplicationHandler` event loop (one test), the live surface/swapchain
|
||||
`render()` path (only the offscreen twin is exercised), clipboard
|
||||
(`arboard` — zero tests reference cut/copy/paste), font-database
|
||||
sanitization beyond the four bundled fixture TTFs, edge-scroll/context
|
||||
menu/minibuffer interactive drivers. No proptests in the crate.
|
||||
|
||||
### 2.4 The Lua layer: 16.4k lines, no test framework, one universal
|
||||
guarantee
|
||||
|
||||
Every `EditorState::new()` evals all 30 runtime chunks, so every Rust
|
||||
test is a *load/syntax* smoke for all Lua. Behavior coverage is via
|
||||
per-file `eval()` helpers **copy-pasted into 67 of 92 test files** (823
|
||||
call sites) poking module namespaces directly. Findings:
|
||||
|
||||
- **`pmacs.error` is a test-stub-only API — confirmed and worse than
|
||||
the memory of it.** ~35 references across 9 builtin files, every one
|
||||
guarded (`if pmacs.error then` / `pcall`). Production never assigns
|
||||
it; the only assignments are 10 stubs inside test code. The tests
|
||||
that assert into those stubs prove the message is *formatted*, not
|
||||
that anyone receives it — all would stay green if the channel were
|
||||
deleted. `dired.lua:56`, `lsp.lua:1659`, `lean.lua:171` acknowledge
|
||||
this in comments and route around it via `set_status`. The
|
||||
`COHERENCE.md` tally ("fifteen call sites") is stale.
|
||||
- **56 statement-position `pcall`s discard errors entirely**, 25 of
|
||||
them in `lsp.lua`'s hot path (`did_change`, `did_open`, inlay/token
|
||||
pulls). A server going silent after a failed `did_change` is
|
||||
unobservable — including to the test suite.
|
||||
- **Interaction islands with zero test contact**: `menus/default.lua`
|
||||
(6 of 11 commands never referenced anywhere in the tree),
|
||||
`keymaps/default.lua` (no suite pins the default binding table — a
|
||||
silent rebinding regression only surfaces if an unrelated suite
|
||||
happens to type that key), `window.lua` and `fold.lua` (10/10
|
||||
registered commands unnamed in tests; their frontend/point-resolution
|
||||
wrapper bodies are bypassed by the Rust-level tests underneath),
|
||||
`workers.cancel-at-point` and `recent-files` (never mentioned).
|
||||
- **Command-name coverage is the honest metric and it is low**:
|
||||
`lsp.lua` registers 15 commands, 12 never named by any test;
|
||||
`commands/default.lua` registers 80, 35 never named.
|
||||
- `builtin/api/packages.lua` is never loaded or parsed by anything; its
|
||||
single "test" greps it as a string for three substrings.
|
||||
- `lean_abbrev.lua` (1,883 lines, 11% of the Lua tree) has ~5
|
||||
assertions; generated data, low risk, but zero drift detection
|
||||
against its generator.
|
||||
|
||||
### 2.5 No fuzzing, and two ideal targets
|
||||
|
||||
No cargo-fuzz/AFL/libfuzzer anywhere. The two highest-value targets are
|
||||
exactly the shapes fuzzing is best at: `pmacs_protocol::transport::
|
||||
read_message` (length-prefixed decoder with a `MAX_FRAME_BYTES` guard;
|
||||
12 hand tests) and `src/ansi.rs` (VT escape parser; 44 hand tests).
|
||||
Both parse attacker-adjacent input (a compromised frontend socket; PTY
|
||||
output from arbitrary child processes). The existing 30 s
|
||||
`fuzz_no_crashes` in `tests/acceptance.rs` fuzzes buffer edits only.
|
||||
|
||||
---
|
||||
|
||||
## 3. Test-quality defects (races, hangs, silent lies)
|
||||
|
||||
Ranked; each is a known-bitten shape recurring in a new place.
|
||||
|
||||
1. **`RuntimeHandles::drop` joins a non-interruptible reader with no
|
||||
timeout** (`src/process.rs:631-642` + `:1874`). This is the real
|
||||
basedpyright hang: a PATH'd server leaving a descendant holding the
|
||||
stdout write end blocks `read()` forever, below every test-level
|
||||
deadline. It is a **product** defect the skip merely hides; the fix
|
||||
(give non-group readers the `O_NONBLOCK`+poll loop that group
|
||||
readers already have) is named as a deferral at
|
||||
`src/process.rs:1937-1940`. Fixing it retires the `--skip
|
||||
basedpyright` clause from the gates and the latent CI stall (§4).
|
||||
2. **`wait_for_file` without a non-empty guard**
|
||||
(`tests/vterm_stage2_acceptance.rs:723`) — returns on the first
|
||||
successful read, including the zero-byte window of the non-atomic
|
||||
writers it waits on; four call sites then assert exact bytes. The
|
||||
fixed variant (`&& !bytes.is_empty()`) already exists at
|
||||
`tests/bottom_panel_stage1_acceptance.rs:2307` — someone was bitten
|
||||
and patched only the local copy. The vterm file's own panic message
|
||||
("published but malformed") describes the race it causes. Cheapest
|
||||
high-value fix in this audit.
|
||||
3. **Fixed-iteration `settle()` as the sole synchronization before
|
||||
strict assertions about a real forked LSP child** — 16–20 ms total,
|
||||
no predicate, no deadline: `tests/lsp_multi_root_acceptance.rs:125`,
|
||||
`tests/lsp_dispatch_seams_acceptance.rs:105`,
|
||||
`tests/lean4_server_acceptance.rs:112`; ~58 `settle()` calls gating
|
||||
~68 tests. The correct predicate-plus-deadline pump exists at
|
||||
`tests/journey_acceptance.rs:63`, `tests/dired_acceptance.rs:107`,
|
||||
and (exemplary, with diagnostic dumps) `tests/m10_11_acceptance.rs:316`.
|
||||
4. **`drain_until`/`drain_lsp_until` return partial results on timeout
|
||||
instead of panicking, and 15 call sites discard the result** (`let _
|
||||
= drain_lsp_until(...)`). A test whose awaited event never arrives
|
||||
proceeds unsynchronized and fails elsewhere — or passes for the
|
||||
wrong reason. `tests/m9_4_acceptance.rs:121` already does it right
|
||||
(panic with the job id). Also note these helpers are effectful
|
||||
(ticking reaps children, `take_events` consumes) — the known
|
||||
`drain_until` trap — and at least one assertion
|
||||
(`tests/m4_acceptance.rs:1070-1084`) is load-bearing on the side
|
||||
effect of a *discarded* call.
|
||||
5. **Git fixtures inherit the ambient global gitconfig** (9 files, e.g.
|
||||
`tests/m8_7_acceptance.rs:87`, `tests/m7_review_acceptance.rs:30`).
|
||||
A developer with `commit.gpgsign = true` gets an unbounded pinentry
|
||||
hang inside `git commit`. One `GIT_CONFIG_GLOBAL=/dev/null` +
|
||||
`GIT_CONFIG_NOSYSTEM=1` pair per fixture closes it.
|
||||
6. **Bare sleeps with strict assertions after**: 20 sleep-then-assert
|
||||
sites outside poll loops, notably `tests/m5_5_acceptance.rs:184`
|
||||
(300 ms for daemon slot-clear), `tests/m4_acceptance.rs:7093`
|
||||
(120 ms against a 75 ms coalescing window — 45 ms of margin), and
|
||||
negative assertions after fixed waits
|
||||
(`tests/lean4_server_acceptance.rs:493` acc28,
|
||||
`tests/m8_7_acceptance.rs:786`) that pass if the thing simply
|
||||
hasn't happened *yet* — each needs a positive-control sentinel.
|
||||
7. **Unit tests that require serial execution without declaring it**:
|
||||
`src/file_io.rs:427` mutates process-wide cwd (`set_current_dir`)
|
||||
while concurrent tests resolve relative paths through
|
||||
`current_dir()` fallbacks; `/proc`-global probes in
|
||||
`tests/worker_shutdown_acceptance.rs` and the M6.6 RSS gate.
|
||||
8. **Smaller but real**: no `set_read_timeout` in
|
||||
`tests/gpu_invocation_acceptance.rs:165`'s hand-rolled
|
||||
`wait_for_daemon` (the shared harness's version at
|
||||
`tests/common/daemon.rs:200` has one, with a comment saying why); a
|
||||
2 s 100%-CPU busy-spin at
|
||||
`tests/statusline_segments_acceptance.rs:1016` that manufactures
|
||||
load for its siblings; leaked orphan daemons on timeout in
|
||||
`tests/m5_7_acceptance.rs:211` plus a PID-reuse-unsafe `kill(pid,0)`
|
||||
probe; `PmacsPty::output()` clones an unbounded buffer on every
|
||||
20 ms poll (`tests/common/pty.rs:55`); `sleep(1100ms)` in a unit
|
||||
test (`src/file_io.rs:420`).
|
||||
|
||||
**The serialization contract is CI-only and it cuts both ways.**
|
||||
`--test-threads=1` appears at five points in `ci.yml` and nowhere in
|
||||
the documented local gates, `.cargo/`, or any hook. So the process-
|
||||
lifecycle races CI's comment admits to dodging are structurally never
|
||||
caught (masked in CI, dismissed as local flakes), while
|
||||
serial-dependent tests (the `settle()` family, `/proc` probes, the cwd
|
||||
test) pass in CI and flake for whoever runs the documented local gate.
|
||||
|
||||
---
|
||||
|
||||
## 4. Tooling and infrastructure gaps
|
||||
|
||||
- **`scripts/bite` has no positive control.** It never runs the tests
|
||||
against the *current* tree, so a test that fails everywhere — e.g.
|
||||
broken, or environmentally skipped-then-failing — reports `bite: OK`.
|
||||
Given §1.2's silent skips, a bite claim on a PATH-gated test is
|
||||
meaningless. ~5-line fix: require `cargo test "$@"` to pass on the
|
||||
unmodified tree first.
|
||||
- **`docs/agent-handoff.md` (§5, "A fix must be COMMITTED before it is
|
||||
bitten") misdescribes bite's mechanism** — verified: it claims
|
||||
restore-by-`git checkout --` (revert to HEAD, destroying uncommitted
|
||||
work), but the script has restored from a `mktemp` copy via `cp` +
|
||||
`trap` since its only commit. The commit-first rule stays sound for
|
||||
two *different* reasons (SIGKILL bypasses the trap; biting an old
|
||||
version of your own uncommitted claim is circular), but the stated
|
||||
mechanism will mislead an agent reasoning about what is safe. Fix
|
||||
the doc, keep the rule.
|
||||
- **Bite-verification is honor-system.** It's step 4 of the working
|
||||
method and appears in eight framing docs as prose claims, but
|
||||
nothing re-checks a bite after the code moves. (A CI mode is
|
||||
probably overkill; recording the bitten ref+command in the framing
|
||||
doc would at least make claims re-runnable.)
|
||||
- **No coverage measurement, Rust or Lua.** No tarpaulin/llvm-cov/
|
||||
grcov/luacov anywhere. For Lua it's worse than absent: Rust line
|
||||
coverage of `include_str!` chunks would report the *eval* line, not
|
||||
which Lua branches ran. §2.4's command-name coverage is currently the
|
||||
only measurable proxy.
|
||||
- **No enforcement layer between "documented gates" and "what runs"**:
|
||||
the gate list exists as prose in three places (CLAUDE.md, AGENTS.md,
|
||||
handoff §3 — the handoff adds a full-workspace sweep the other two
|
||||
omit). No script runs the suite, so drift among the three copies is
|
||||
invisible. A `scripts/gates` that executes the canonical list would
|
||||
make the prose testable.
|
||||
|
||||
---
|
||||
|
||||
## 5. CI: correctness hardening
|
||||
|
||||
(Findings that change what CI *certifies*; speedups are §6.)
|
||||
|
||||
1. **Branch protection is off** — every job is advisory; a red run
|
||||
merges as easily as a green one. Turn on required checks for the
|
||||
cheap deterministic jobs at minimum (fmt, clippy, ubuntu test legs).
|
||||
2. **No job timeouts except m6** (15 min). Everything else inherits
|
||||
360 min. The day a runner image ships any of the PATH-gated tools
|
||||
(§1.2), the basedpyright-class hang burns 6 h × 4 matrix legs with
|
||||
no signal. `timeout-minutes: 25` on every job is free insurance.
|
||||
3. **The crdt lane** (already scoped in `docs/active-work.md`): CI leg
|
||||
for `--features luajit,crdt`, GPU-requiring crdt suites onto
|
||||
`gpu-render` with `PMACS_REQUIRE_GPU=1` stated as a requirement (or
|
||||
a37 runs vacuously), clippy-crdt fixed first. Not re-scoped here.
|
||||
4. **Arm the skips it already runs**: install gopls/clangd/zsh/fish/
|
||||
lua on the ubuntu test leg (one apt line + one `go install`/npm
|
||||
line) and add `PMACS_REQUIRE_LSP`/`PMACS_REQUIRE_SHELLS` env
|
||||
guards mirroring `PMACS_REQUIRE_GPU`, set only on legs where the
|
||||
tools are installed. This converts 15 permanently-vacuous tests
|
||||
into real CI coverage.
|
||||
5. **A scheduled (cron) job** for what per-PR CI can't afford: the
|
||||
3600 s soak, full M6 cancel/ingest profiles, the `--ignored` perf
|
||||
smokes nothing runs (§1.3), and N repetitions of the known
|
||||
load-sensitive suites as a flake canary. Nightly, ubuntu-only,
|
||||
`workflow_dispatch` for manual runs. This also gives intermittent
|
||||
failures a place to burn in instead of surfacing mid-review.
|
||||
6. **One deliberately-parallel leg** (default test threads, ubuntu,
|
||||
allowed-to-fail at first) so the races serialization currently
|
||||
masks become visible on purpose instead of as local flakes.
|
||||
7. **`pmacs-protocol` clippy** — add `-p pmacs-protocol` to the CI
|
||||
clippy job (local `--workspace` covers it; CI never has).
|
||||
|
||||
## 6. CI: speedups and efficiency
|
||||
|
||||
Measured baseline (run 30454133846, warm cache): wall ~14.5 min;
|
||||
critical path is `Test (macos-latest/luajit)` at ~14.6 min, macos/lua54
|
||||
~10 min, ubuntu test legs ~6.7 min, perf gates 2–3.5 min, everything
|
||||
else ≤1.5 min.
|
||||
|
||||
1. **No `concurrency` group** — force-pushes and the ledger-contention
|
||||
rebase treadmill leave superseded runs burning to completion. Add
|
||||
`concurrency: {group: ci-${{ github.ref }}, cancel-in-progress:
|
||||
true}` guarded to PRs. Likely the single largest minutes saving
|
||||
given the project's rebase-heavy flow, and macOS minutes are the
|
||||
expensive ones.
|
||||
2. **Split serial from parallel on the test job.** `--test-threads=1`
|
||||
serializes ~3,200 tests for the sake of a handful of
|
||||
process-lifecycle suites. Splitting into `cargo test --lib` (parallel)
|
||||
+ integration targets (serial) would cut the critical-path macOS leg
|
||||
substantially; nextest (below) is the cleaner version of the same
|
||||
move.
|
||||
3. **Consider cargo-nextest** for the test job: per-test process
|
||||
isolation retires the cwd/`/proc`/env hazards of §3.7 *and* enables
|
||||
safe parallelism; per-test timeouts retire the hang class at the
|
||||
runner level; automatic retries with failure reporting make the
|
||||
known load-flakes visible instead of rerun-by-hand; JUnit output
|
||||
for free. This is the one tool that converts several §3 classes
|
||||
from "fix each site" to "fixed by construction".
|
||||
4. **Trim the macOS matrix if minutes matter**: the lua-flavor cfg
|
||||
audit found zero flavor-gated tests — the 2×2 matrix only varies
|
||||
mlua's backend. macOS × lua54 has caught real macOS bugs
|
||||
(`m4_5` config-sink race was lua54/macOS-shaped), so don't drop
|
||||
macOS — but macOS × one flavor + ubuntu × both retains the platform
|
||||
signal at ¾ the macOS cost. Decide from flake history, not a priori.
|
||||
5. **Redundant work, minor**: `cargo build --all-targets` before
|
||||
`cargo test --all-targets` is a near-no-op (incremental) but adds
|
||||
log noise; `-p pmacs-protocol` runs identically on all 4 legs
|
||||
(comment acknowledges; it's seconds); the gpu-render apt-get runs
|
||||
every job (~20 s; cacheable via `actions/cache` on the deb pool if
|
||||
it ever grows).
|
||||
6. **Perf gates on shared runners** already run loosened profiles
|
||||
(§1.4) and are the most variance-exposed jobs. If they start
|
||||
flaking, prefer trend-tracking (store the measured numbers as
|
||||
artifacts, gate on regression-vs-window) over further threshold
|
||||
loosening — a threshold loose enough to never flake certifies
|
||||
nothing.
|
||||
|
||||
---
|
||||
|
||||
## 7. What is deliberately out of scope
|
||||
|
||||
- Windows: no `cfg(windows)` code exists; unix-only sockets. Not a
|
||||
testing gap until the product targets it.
|
||||
- `src/bin/pmacs_fake_lsp.rs` / `pmacs_fake_mcp.rs` (3,145 untested
|
||||
lines): test fixtures; testing the fakes is circular. Their fidelity
|
||||
is instead covered by §1.2's real-server tests — which is an argument
|
||||
for §5.4, not for unit-testing the fakes.
|
||||
- The lean4/latex suites' stub-based design (`lake_stub`,
|
||||
vendored grammar): verified deliberate and CI-real; not a gap.
|
||||
|
||||
## 8. Candidate lanes for the arc (priority order)
|
||||
|
||||
1. **CRDT CI lane** — already scoped in `docs/active-work.md`; first
|
||||
task there is deliberate-vs-accidental classification. Blockers:
|
||||
clippy-crdt red, a37 vacuity, first-run flake budget.
|
||||
2. **Silent-skip arming** (§1.2, §5.4) — install tools + `PMACS_REQUIRE_*`
|
||||
guards. Small, mechanical, converts 15 vacuous tests to real ones.
|
||||
Independent of lane 1.
|
||||
3. **CI hardening/speedup** (§5.1–2, §6.1–3) — timeouts, concurrency,
|
||||
branch protection, serial/parallel split or nextest. No product code.
|
||||
4. **The reader-join hang** (§3.1) — the one product fix in this list;
|
||||
retires the basedpyright skip everywhere. Already a named deferral
|
||||
in `src/process.rs`.
|
||||
5. **Race-shape cleanup sweep** (§3.2–6) — port the already-invented
|
||||
fixes (non-empty guard, predicate pumps, panic-on-timeout, git env
|
||||
pinning) to the degraded copies. Mostly mechanical; each fix should
|
||||
be bite-verified, which motivates doing lane 6 first or alongside.
|
||||
6. **`scripts/bite` positive control + handoff correction** (§4) —
|
||||
tiny, but raises the trust ceiling on every other lane's
|
||||
verification claims.
|
||||
7. **Lua error-channel decision** (§2.4) — either define `pmacs.error`
|
||||
in production or delete the 35 dead references and standardize on
|
||||
`set_status`; then a sweep of the 56 discarded pcalls. This is a
|
||||
product-coherence question (COHERENCE.md owns the framing) as much
|
||||
as a testing one.
|
||||
8. **Protocol-crate self-sufficiency** (§2.1) — move/duplicate the
|
||||
round-trip + negotiation pins into `pmacs-protocol`; add its clippy
|
||||
to CI. Mechanical but wide.
|
||||
9. **Fuzz targets** (§2.5) — `read_message` + ANSI parser, run on the
|
||||
cron job (lane 3's infrastructure).
|
||||
10. **Coverage floor for the untested-file list** (§2.2, §2.3) and
|
||||
**Lua interaction-island pins** (§2.4: menus, default keymap,
|
||||
window/fold wrappers) — largest and least mechanical; needs its
|
||||
own framing to avoid writing biteless tests, per the project's own
|
||||
"new tests need their own bite" history.
|
||||
|
||||
---
|
||||
|
||||
## Appendix: re-measurement commands
|
||||
|
||||
```sh
|
||||
# Dark-test census (re-measure, never quote):
|
||||
cargo test --all-targets --no-default-features --features luajit -- --list | grep -c ': test'
|
||||
cargo test --all-targets --no-default-features --features luajit,crdt -- --list | grep -c ': test'
|
||||
|
||||
# Ignored tests that no job runs:
|
||||
grep -rn '#\[ignore' src/ tests/ --include='*.rs'
|
||||
|
||||
# Silent-skip sites:
|
||||
grep -rn 'which_binary\|not on PATH; skipping' tests/
|
||||
|
||||
# pmacs.error references (production never assigns it):
|
||||
grep -rn 'pmacs\.error\b' builtin/ --include='*.lua'
|
||||
|
||||
# Discarded statement-position pcalls:
|
||||
grep -rn '^\s*pcall(' builtin/ --include='*.lua'
|
||||
|
||||
# Per-job CI timing for a run:
|
||||
gh run view <run-id> --json jobs --jq '.jobs[] | "\(.name): \(.startedAt) -> \(.completedAt)"'
|
||||
```
|
||||
|
|
@ -525,11 +525,13 @@ has **no branch and no framing yet**.
|
|||
|
||||
- Portable branch: `githubsucks/generated-buffer-immutability`; worktree
|
||||
`../pmacs-generated-immutability`. **PR #188**, base `main`, forked from
|
||||
`githubsucks/main` @ `ad41cf1`, **integrated to `64883eb` at merge
|
||||
commit `76cfaac`** — #189 (clean), then #186 and #171
|
||||
(`docs/active-work.md` conflict), then #187 (the same file again, after
|
||||
it removed the two landed framing lanes). Revision 6 was reviewed at
|
||||
head `55c3061`; revision 7 closes that round. Framing only —
|
||||
`githubsucks/main` @ `ad41cf1`, **integrated through `5e186c7`** —
|
||||
#189 (clean), then #186 and #171 (`docs/active-work.md` conflict),
|
||||
then #187 (the same file again, after it removed the two landed
|
||||
framing lanes), #192 at merge commit `76cfaac`, and #193
|
||||
(`docs/active-work.md` conflict again) after revision 7's first push.
|
||||
Revision 6 was reviewed at head `55c3061`; revision 7 closes that
|
||||
round. Framing only —
|
||||
`docs/generated-buffer-immutability-framing.md`, revision 7, plus this
|
||||
lane. **No runtime code, no protocol change.**
|
||||
- **PROPOSED — six review rounds closed (thirty-two findings,
|
||||
|
|
@ -715,61 +717,6 @@ has **no branch and no framing yet**.
|
|||
warns against quoting a stale figure; it does not replace that
|
||||
section's per-target census, which was not re-derived.
|
||||
|
||||
## Test-improvement arc, lane 6 — `scripts/bite` positive control
|
||||
|
||||
- Portable branch: `githubsucks/bite-positive-control`, worktree
|
||||
`../pmacs-bite`, based on `main` @ `300cbc4`. Tooling and
|
||||
documentation only — **no product code, no tests changed.**
|
||||
- **Self-contained deliberately.** An earlier draft of this entry
|
||||
scoped the lane by citing a `TEST_IMPROVEMENT.md` at the repo root.
|
||||
That file is **untracked and machine-local** — not on `main`, not in
|
||||
this PR's tree — so the citation was a dangling reference for any
|
||||
other machine, which is exactly what this ledger exists to prevent.
|
||||
The lane is described here on its own terms instead. If that audit
|
||||
is to scope later lanes it needs committing first, in its own PR.
|
||||
- Sequenced ahead of the rest of the testing work because every later
|
||||
lane's evidence is bite-shaped. A verifier that cannot fail is not
|
||||
worth more than the claims it certifies.
|
||||
- **The defect:** the script ran only the swapped tree, so a failing
|
||||
swapped run was the only thing it checked. A test that fails
|
||||
everywhere — a typo, an unrelated compile break — therefore printed
|
||||
`bite: OK`. Now it asserts the named tests pass against the working
|
||||
tree **and** that at least one ran (a filter matching nothing exits
|
||||
0, so passing alone is insufficient), exiting 3 as `NO CONTROL`
|
||||
otherwise, and it labels `OK (assertion)` versus `OK (COMPILE)`.
|
||||
- **Validated by execution, not by reading**, and the results are
|
||||
listed with the one arm that was NOT reproduced:
|
||||
- zero-match filter -> `NO CONTROL`, exit 3;
|
||||
- genuine bite -> `control OK` then `OK (assertion)`, exit 0;
|
||||
- test broken so it fails on BOTH trees -> `NO CONTROL`, exit 3.
|
||||
**The old script printed `bite: OK` here**, which is the whole
|
||||
point of the lane;
|
||||
- assertion failure whose own output prints `error[E0308]` and
|
||||
`error: could not compile` at column 0 -> still `OK (assertion)`,
|
||||
which is why classification reads libtest's summary rather than
|
||||
compiler text;
|
||||
- swapped file that will not build -> `OK (COMPILE)`;
|
||||
- **`INCONCLUSIVE (MIXED)` was not reproduced.** Its assumed trigger
|
||||
— `--test A --test B` where B's swapped file fails to build — was
|
||||
tested and **does not reach it**: cargo builds every named target
|
||||
before running any, so B's failure stops A from running and the
|
||||
run yields no summary at all (the COMPILE arm). The arm stays as
|
||||
defence for the genuinely reachable causes (doc-tests failing to
|
||||
compile after lib tests pass; a harness dying after its summary),
|
||||
and the script says it was not manufactured.
|
||||
- **Handoff correction, verified rather than inherited.**
|
||||
`docs/agent-handoff.md` §5 claimed the script "restores by `git
|
||||
checkout --`, which reverts the file to HEAD", destroying
|
||||
uncommitted work. False: it copies to a `mktemp` path before the
|
||||
swap and restores from that copy under an `EXIT INT TERM` trap. The
|
||||
commit-before-gating rule stands on other grounds and is kept; the
|
||||
false mechanism is removed, because it would push the next reader
|
||||
toward `git stash` — the repo-global trap the script exists to
|
||||
avoid.
|
||||
- Recovery from a clean checkout:
|
||||
`git fetch githubsucks && git worktree add ../pmacs-bite
|
||||
-b bite-positive-control githubsucks/bite-positive-control`.
|
||||
|
||||
## Parked lane: kill-ring browser + persistence
|
||||
|
||||
- Portable branch: `githubsucks/kill-ring-browser`
|
||||
|
|
|
|||
|
|
@ -3,11 +3,14 @@
|
|||
**PROPOSED — needs explicit user approval before implementation. DO NOT
|
||||
implement, DO NOT merge.**
|
||||
|
||||
**Revision 7 — answers review round 6 on `55c3061`, against canonical
|
||||
`githubsucks/main` @ `64883eb`, 2026-07-29.** The base has not moved
|
||||
since revision 6; this revision changes the quarantine state machine,
|
||||
five acceptance classifications and the cross-machine ledger because
|
||||
the reviewed design did not contain every route it claimed to contain.
|
||||
**Revision 7 — answers review round 6 on `55c3061`, authored against
|
||||
canonical `githubsucks/main` @ `64883eb` and integrated through
|
||||
`5e186c7`, 2026-07-29.** #193 landed after revision 7's first push at
|
||||
`34041a4`; `64883eb..5e186c7` changes only `TEST_IMPROVEMENT.md` and
|
||||
`docs/active-work.md`, so the source readings and citations below
|
||||
survive. This revision changes the quarantine state machine, five
|
||||
acceptance classifications and the cross-machine ledger because the
|
||||
reviewed design did not contain every route it claimed to contain.
|
||||
|
||||
## Revision 7
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue