diff --git a/docs/active-work.md b/docs/active-work.md index c7ec98a..b00ed4d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -607,22 +607,66 @@ Edit tool, which errors on mismatch, for anything load-bearing. ### Verification (delivered) Everything framing §7 sketched, plus three groups it did not anticipate -(framing §7.1). The exact gate run, all green on this machine: +(framing §7.1). + +**The gate list below is the one that FAILED to catch this lane's CI +red, and it is kept only to show the hole.** "The touched acceptance +suites" is selected from the diff, and a `PROTOCOL_VERSION` bump breaks +version-assertion tests that appear nowhere in it. Five failed on CI's +first round. Use **`cargo test --tests --no-fail-fast`** on any +protocol bump — `--no-fail-fast` because cargo stops at the first +failing target, which is why CI showed one failure and the local +full-corpus run then found three more. Recorded in +`docs/agent-handoff.md`. ``` cargo fmt --check cargo clippy --workspace --all-targets -- -D warnings +cargo clippy --workspace --all-targets --features crdt -- -D warnings cargo test --lib # 1917 / 0 cargo test --lib --features crdt # 2102 / 0 -cargo test --test line_wrap_acceptance # 6 / 0 -cargo test --test long_line_readable_acceptance # 2 / 0 -cargo test --test folding_acceptance # 21 / 0 -cargo test --test full_grid_resync_acceptance # 1 / 0 -cargo test --test m4_acceptance -- --skip basedpyright +cargo test --tests --no-fail-fast # 108 targets, exit 0 +cargo test --tests --features crdt --no-fail-fast # 108 targets, exit 0 PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu # 228 / 0 git diff --check ``` +**Both configurations, and that mattered.** The default sweep went +clean while `--features crdt` still had **three** failures, in +crdt-gated real-daemon tests (`vterm_stage3_acceptance` ×2, +`bottom_panel_stage2b_gpu_acceptance` ×1) that assert on a real +socket's negotiated version. This is the handoff's existing "a local +sweep is blind to whichever feature configuration it does not build" +lesson, hit again by a different lane — so **eight** version +assertions broke in total, not five. + +The eight version-assertion failures, and what each one was: + +| test | was | why it broke | +|---|---|---| +| `acc51_a_v20_peer_..._even_when_capable` (×5 jobs) | `PROTOCOL_VERSION - 1` | **bug** — an absolute contract ("below the panel version") as arithmetic on a moving constant. Now `PANEL_MIN_VERSION - 1`, the idiom `src/` already used in five places | +| `the_baseline_stays_and_the_counter_offer_activates` | `PANEL_MIN_VERSION == PROTOCOL_VERSION` | **bug** — asserted a coincidence true only while panels were the newest feature. Now the two durable bounds | +| `the_panel_stage_takes_protocol_v21` | `PROTOCOL_VERSION == 21` | **bug** — the current wire as a proxy for the panel stage's own version, in a test whose name says which it means | +| `a54_real_daemon_..._panel_hosted_terminal` *(crdt)* | `session_protocol_version == "21"` | **bug** — the negotiated session version is *this binary's* wire, so the literal held only while panels were newest. Now `PROTOCOL_VERSION`, plus an explicit `>= PANEL_MIN_VERSION` for the capability the literal carried implicitly | +| `a37_real_daemon_..._one_terminal_session` *(crdt)* | `session_protocol_version == "21"` | **bug** — same | +| `a13_17_26_..._version_cost` | `PROTOCOL_VERSION == 21`, `6..=21`, `!supported(22)` | **tripwire working as designed** — it says in its own comment that it tracks the current wire. Took the conscious edit | +| `the_baseline_stays_...` (same test) | `PROTOCOL_VERSION == 21` | **tripwire working as designed** | +| `terminal_mode_keeps_reporting_presence_...` *(crdt)* | `PROTOCOL_VERSION == 21` | **tripwire working as designed** | + +**Five bugs and three tripwires.** The distinction is the useful part. +A tripwire that fires on a bump is doing its job, and "fixing" it means +editing it deliberately — the baseline pin `ADVERTISED_PROTOCOL_VERSION +== 20` is the one that must *never* be edited, and it never fired. + +The five bugs share one shape: **an absolute contract expressed as +arithmetic on, or equality with, a moving constant.** `PROTOCOL_VERSION +- 1` for "below the panel version"; `PANEL_MIN_VERSION == +PROTOCOL_VERSION` for a coincidence; `PROTOCOL_VERSION == 21` standing +in for the panel stage's own version; `"21"` for a negotiated session +version. Each was true when written and silently false afterwards. +`src/` already had the right idiom — `PANEL_MIN_VERSION - 1`, five +occurrences — and every outlier was in `tests/`. + `tests/long_line_readable_acceptance.rs` is the one that answers the **report** rather than a mechanism: the shipped binary, a real PTY, a line 200 columns wide in an 80-column terminal, and an assertion that @@ -650,6 +694,13 @@ establish that. Logged there as **U1**, explicitly *not* matched against A1 (also GPU-headless-under-load) because matching requires a selector and fragments this occurrence does not have. +**And one more, logged as U2.** +`process::tests::m6_1_pty_raw_mode_disables_kernel_echo` failed once +during a full `--tests --no-fail-fast` run and did not reproduce in a +later full sweep (108 targets, exit 0) or 3 isolated `--lib` runs. It +is in no registry row, so it is a new incident; leaked +`pmacs --daemon` processes remain an unexcluded rival explanation. + ### Not in scope Horizontal scroll in full (Stage 4). `M-q` / auto-fill / reflow. Word diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index e94db13..48a29fd 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -404,6 +404,35 @@ someone forgot. old. They are a rival explanation for any load-sensitive local failure, so **check `pgrep -f "pmacs --daemon"` before trusting a local red**. Lane recorded in `docs/active-work.md`. +- **A `PROTOCOL_VERSION` bump's blast radius is every version-sensitive + test, and NONE of them appear in the diff.** "The touched acceptance + suites" is the standing gate, and for a protocol bump it is the wrong + selector: long-lines Stage 3 bumped v21→v22, ran the suites it had + edited, and broke **eight** version assertions across six suites. CI + showed exactly **one**, because **cargo stops at the first failing + target**; the rest surfaced only afterwards, and one at a time would + have cost four more red rounds. + + **On any protocol bump run `cargo test --tests --no-fail-fast` in + BOTH feature configurations.** `--no-fail-fast` because of the + stop-at-first-target behavior above, and `--features crdt` because + three of the eight were in crdt-gated real-daemon tests that assert + on a live socket's negotiated version — invisible to a default sweep, + which is the blindness the bullet below already names. + + **Sort the failures before fixing them.** A *tripwire* + (`assert_eq!(PROTOCOL_VERSION, N)`) is meant to fire and takes a + deliberate edit; three of the eight were these, and the one pin that + must NEVER be edited — `ADVERTISED_PROTOCOL_VERSION == 20` — did not + fire. The other five were defects sharing one shape: **an absolute + contract expressed as arithmetic on, or equality with, a moving + constant.** `PROTOCOL_VERSION - 1` for "below the panel version"; + `PANEL_MIN_VERSION == PROTOCOL_VERSION` for a coincidence true only + while panels were newest; `PROTOCOL_VERSION == 21` standing in for + the panel stage's own version; `"21"` for a negotiated session + version. Anchor on the constant the contract *names* — `src/` already + spelled it `PANEL_MIN_VERSION - 1` in five places, and every outlier + was in `tests/`. - **A local sweep is blind to whichever feature configuration it does not build.** Stage 3's census and every verification sweep ran `--features luajit` WITHOUT `crdt`, so no crdt-gated suite was diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 07917f3..0f2154b 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -423,6 +423,21 @@ without a name there is nothing to call intermittent. with `grep -E "FAILED|panicked|test result"`, which keeps failure context, or capture the full log to a file and summarize from it. +### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, one local occurrence + +Has a selector, which U1 lacks — but still no fragments, so it cannot +be matched either. Recorded so a recurrence is recognisable. + +| field | value | +|---|---| +| **selector** | `--lib process::tests::m6_1_pty_raw_mode_disables_kernel_echo` | +| **job / flavor** | local (Linux), during `cargo test --tests --no-fail-fast` — the lib target alongside a full PTY-heavy corpus | +| **required fragments** | **none captured** — output was filtered to the `FAILED` line | +| **status** | **new incident, unreproduced** | +| **what IS established** | it failed once (`1916 passed; 1 failed`), in no registry row, under a full-corpus run | +| **what is NOT** | any mechanism. Not reproduced in a later full `--tests --no-fail-fast` sweep (108 targets, exit 0) nor in 3 isolated `--lib` runs (1917/0 each) | +| **rival explanation not excluded** | leaked `pmacs --daemon` processes, which the handoff names as a standing confound for any load-sensitive local red | + **The retirements are not occurrences and do not close the log.** R1 and R3 stay live, and each retired row keeps its signature so a later red matching one reopens it. diff --git a/tests/bottom_panel_stage2b_daemon_acceptance.rs b/tests/bottom_panel_stage2b_daemon_acceptance.rs index cc80422..e73109a 100644 --- a/tests/bottom_panel_stage2b_daemon_acceptance.rs +++ b/tests/bottom_panel_stage2b_daemon_acceptance.rs @@ -32,7 +32,7 @@ use pmacs::editor_core::GeometryUpdate; use pmacs::protocol::{FrontendId, InstanceMessage, PROTOCOL_VERSION}; use pmacs::semantic_render::SemanticRenderState; use pmacs::window::{FrontendView, Layout, Window, WindowId}; -use pmacs_protocol::panel::{PanelFrame, PanelFramePayload}; +use pmacs_protocol::panel::{PANEL_MIN_VERSION, PanelFrame, PanelFramePayload}; // --------------------------------------------------------------------------- // Harness @@ -740,10 +740,25 @@ fn acc51_a_pre_panel_semantic_frontend_is_never_sent_a_panel_frame() { ); } +/// A peer below [`PANEL_MIN_VERSION`] gets no `PanelFrame` even when the +/// daemon could build one for it. +/// +/// **Anchored on `PANEL_MIN_VERSION - 1`, not `PROTOCOL_VERSION - 1`.** +/// The original spelling was the latter, which expressed an *absolute* +/// contract — "older than the version that introduced panel frames" — +/// as arithmetic on a *moving* constant. It held only while +/// `PROTOCOL_VERSION` happened to equal `PANEL_MIN_VERSION`, and the +/// long-lines lane's bump to v22 made `PROTOCOL_VERSION - 1` equal +/// `PANEL_MIN_VERSION` exactly: the fixture's "old" peer became +/// panel-capable, so the daemon correctly sent a frame and the test +/// correctly failed. The production code was never wrong. +/// +/// `src/daemon.rs` and `pmacs-gpu/src/main.rs` already spell this +/// `PANEL_MIN_VERSION - 1` in five places; this was the one outlier. #[test] -fn acc51_a_v20_peer_is_sent_no_panel_frame_even_when_capable() { +fn acc51_a_sub_panel_version_peer_is_sent_no_panel_frame_even_when_capable() { let mut session = Session::new(); - session.render = SemanticRenderState::for_peer(FID, PROTOCOL_VERSION - 1); + session.render = SemanticRenderState::for_peer(FID, PANEL_MIN_VERSION - 1); session.declare(1, ROWS, COLS); open_panel(&session, "*panel*", 4); diff --git a/tests/bottom_panel_stage2b_gpu_acceptance.rs b/tests/bottom_panel_stage2b_gpu_acceptance.rs index e11493b..257680a 100644 --- a/tests/bottom_panel_stage2b_gpu_acceptance.rs +++ b/tests/bottom_panel_stage2b_gpu_acceptance.rs @@ -336,13 +336,31 @@ fn one_daemon_serves_a_v21_panel_session_and_a_shipped_v20_client() { /// and the counter-offer is what reaches the current wire. #[test] fn the_baseline_stays_and_the_counter_offer_activates() { - assert_eq!(PROTOCOL_VERSION, 21); + // A deliberate tripwire: bumping the wire must be a conscious edit + // here, not a silent one. v22 is `LineWrapFacts` (long-lines Stage 3). + assert_eq!(PROTOCOL_VERSION, 22); assert_eq!( ADVERTISED_PROTOCOL_VERSION, 20, "moving this is the incompatible act the mechanism exists to avoid" ); const { assert!(PROTOCOL_VERSION > ADVERTISED_PROTOCOL_VERSION) }; - assert_eq!(PANEL_MIN_VERSION, PROTOCOL_VERSION); + + // Panel frames are gated ABOVE the advertised floor and at or below + // this binary's wire. Both halves are durable properties of the + // gate. + // + // This replaces `assert_eq!(PANEL_MIN_VERSION, PROTOCOL_VERSION)`, + // which asserted a **coincidence**: panel frames were the newest + // feature when it was written, so their minimum happened to equal + // the current wire. Any later feature falsifies that — v22 is the + // first, and the equality would have had to be edited on every + // subsequent bump while telling a reader something that was never + // the contract. + // `const` blocks, matching the line above: these are compile-time + // constants, so a runtime `assert!` is both a clippy error and a + // weaker check than the language already offers. + const { assert!(PANEL_MIN_VERSION > ADVERTISED_PROTOCOL_VERSION) }; + const { assert!(PANEL_MIN_VERSION <= PROTOCOL_VERSION) }; // The current baseline is answered with this binary's own version. assert_eq!( @@ -584,8 +602,24 @@ fn a54_real_daemon_real_pty_and_headless_gpu_render_one_panel_hosted_terminal() "a deadline-driven pass must not read as success: {text}" ); // The activation, end to end on a real socket. - assert_eq!(fact("session_protocol_version"), "21", "{text}"); + // + // The session version is compared against `PROTOCOL_VERSION` rather + // than the literal "21" it used to pin: what the counter-offer + // activates is *this binary's* wire, so the literal was only ever + // correct while the panel stage was the newest one. The baseline + // stays a literal, because 20 not moving IS the claim. + assert_eq!( + fact("session_protocol_version"), + PROTOCOL_VERSION.to_string(), + "{text}" + ); assert_eq!(fact("baseline_protocol_version"), "20", "{text}"); + // …and that negotiated version is panel-capable, which is the part + // "21" used to carry implicitly. + assert!( + number("session_protocol_version") >= PANEL_MIN_VERSION, + "the negotiated wire must reach the panel minimum: {text}" + ); // The band is real: declared, projected, focused, and carrying the child. assert!( diff --git a/tests/bottom_panel_stage2b_protocol_acceptance.rs b/tests/bottom_panel_stage2b_protocol_acceptance.rs index 55bbc12..2117de0 100644 --- a/tests/bottom_panel_stage2b_protocol_acceptance.rs +++ b/tests/bottom_panel_stage2b_protocol_acceptance.rs @@ -14,7 +14,7 @@ use pmacs_protocol::message::{ AttachRequest, FrontendEvent, Hello, InstanceMessage, Modifiers, MouseButton, MouseKind, }; use pmacs_protocol::panel::{ - MAX_PANEL_VISIBLE_CELLS, PanelFrame, PanelFrameError, PanelFramePayload, + MAX_PANEL_VISIBLE_CELLS, PANEL_MIN_VERSION, PanelFrame, PanelFrameError, PanelFramePayload, }; use pmacs_protocol::terminal::{ MAX_TERMINAL_COLS, TerminalFrame, TerminalFrameError, TerminalProcessState, @@ -107,8 +107,18 @@ fn terminal_frame(rows: u32, cols: u32) -> TerminalFrame { #[test] fn the_panel_stage_takes_protocol_v21() { - assert_eq!(PROTOCOL_VERSION, 21); + // The panel stage's own version, which does not move when a later + // feature appends to the wire. + // + // This was `assert_eq!(PROTOCOL_VERSION, 21)` — the CURRENT wire + // used as a proxy for the panel stage's version. The two were equal + // only until the next feature landed (v22, `LineWrapFacts`), and the + // proxy then failed in a test whose own name says what it means to + // pin. `PANEL_MIN_VERSION` is that constant. + assert_eq!(PANEL_MIN_VERSION, 21); assert!(SUPPORTED_PROTOCOL_VERSIONS.contains(&21)); + // This binary must be able to speak the stage it implements. + const { assert!(PROTOCOL_VERSION >= PANEL_MIN_VERSION) }; // The advertised version is a compatibility BASELINE, and Stage 2B-3 // made that permanent rather than temporary: the server-first Hello // reaches an already-shipped frontend before that frontend can send diff --git a/tests/statusline_segments_acceptance.rs b/tests/statusline_segments_acceptance.rs index 2df0161..b583c83 100644 --- a/tests/statusline_segments_acceptance.rs +++ b/tests/statusline_segments_acceptance.rs @@ -788,15 +788,21 @@ fn a12_builtin_lsp_provider_tracks_real_attachment_and_unknown_label() { fn a13_17_26_protocol_semantic_init_late_join_and_version_cost() { // Vterm Stage 3 appended the terminal family as v19; GPU initial targets // appended the semantic bootstrap family as v20; bottom-panel Stage 2B-1 - // appended the panel family as v21. This acceptance owns the STATUSLINE + // appended the panel family as v21; long-lines Stage 3 appended + // `LineWrapFacts` as v22. This acceptance owns the STATUSLINE // variant's placement and gate, so it tracks the current wire version // rather than pinning 18: the v18 floor it actually cares about is asserted // below and in `peer_accepts_statusline_message`. - assert_eq!(PROTOCOL_VERSION, 21); - for version in 6..=21 { + // + // Tracking the current wire is deliberate, so every bump edits these + // three lines on purpose. The ceiling assertion is the load-bearing + // one — it says the supported set ENDS here, which is what makes an + // accidentally-widened set a failure rather than a silent pass. + assert_eq!(PROTOCOL_VERSION, 22); + for version in 6..=22 { assert!(is_supported_protocol_version(version)); } - assert!(!is_supported_protocol_version(22)); + assert!(!is_supported_protocol_version(23)); let sample = InstanceMessage::StatuslineSegments { buffer_id: BufferId::from_raw(9), left: vec![StatuslineSegment { diff --git a/tests/vterm_stage3_acceptance.rs b/tests/vterm_stage3_acceptance.rs index 0305146..b5a71c5 100644 --- a/tests/vterm_stage3_acceptance.rs +++ b/tests/vterm_stage3_acceptance.rs @@ -737,10 +737,18 @@ fn a37_real_daemon_real_pty_and_headless_gpu_render_one_terminal_session() { // which is exactly the incompatible change this mechanism exists to // avoid — and asserting only the baseline would pass with the whole // activation missing. + // Compared against `PROTOCOL_VERSION`, not the literal "21": the + // counter-offer activates THIS BINARY's wire, so the literal held + // only while the panel stage was the newest one (long-lines Stage 3 + // appended v22). The baseline assertion below stays literal, + // because 20 not moving is the actual claim. + // Fully qualified: this file imports `PROTOCOL_VERSION` inside a + // different test's scope, not at module level. + let session_version = pmacs_protocol::PROTOCOL_VERSION.to_string(); assert_eq!( facts.get("session_protocol_version").copied(), - Some("21"), - "the real client must negotiate the v21 panel wire: {text}" + Some(session_version.as_str()), + "the real client must negotiate this binary's wire: {text}" ); assert_eq!( facts.get("baseline_protocol_version").copied(), @@ -880,7 +888,9 @@ fn terminal_mode_keeps_reporting_presence_so_peers_drop_the_stale_caret() { panic!("timed out waiting for {what}"); } - assert_eq!(PROTOCOL_VERSION, 21); + // Tripwire: a wire bump must be a conscious edit here. v22 is + // `LineWrapFacts` (long-lines Stage 3). + assert_eq!(PROTOCOL_VERSION, 22); let daemon = common::daemon::TestDaemon::spawn_with_env_and_init( &[ ("PMACS_INSTANCE_SEMANTIC_RENDER", "1"),