From 35b119700f42e330264e61c369ee3d14e3ac992d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 11:59:08 -0400 Subject: [PATCH 1/3] test: arm the silent skips, so external-tool tests stop passing vacuously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane 2 of the testing arc (`TEST_IMPROVEMENT.md` §1.2, §5.4). The shape being fixed reports GREEN when the tool is missing: let Ok(_) = which_binary("gopls") else { eprintln!("gopls not on PATH; skipping"); return; }; CI installed none of these tools, so a block of real-language-server and multi-shell tests had never once executed their bodies while reporting success on every run. A suite that cannot distinguish "passed" from "never ran" is worse than a missing suite, because it reads as coverage in exactly the place someone would go looking for it. The fix is this project's own pattern rather than a new one: PMACS_REQUIRE_GPU already turns a missing adapter into a hard failure for the headless render job. This adds PMACS_REQUIRE_LSP, PMACS_REQUIRE_SHELLS and PMACS_REQUIRE_LUA, and the CI step that installs the tools they promise. Per-tool variables rather than one blanket flag, so a tool that must stay unarmed keeps that decision visible at the call site instead of buried in a workflow file. basedpyright is deliberately NOT installed and NOT armed. Its test has no timeout and hangs forever; the root cause is the non-interruptible reader-thread join in `RuntimeHandles::drop`, already a named deferral in `src/process.rs`, and the `test` job has no `timeout-minutes`. Arming it today would trade a vacuous green for a six-hour hang across four legs. PMACS_REQUIRE_PYRIGHT exists and is never set, so the flip is one line after the hang fix and the CI timeouts land. A trap found while writing the workflow rather than after: the natural Actions idiom PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} sets the variable to the EMPTY STRING on every other platform, and `var_os(..).is_some()` is true for `Some("")`. That would have armed the guard on precisely the runners with none of the tools installed and failed every one of them. The helper treats empty as unset, which makes the common spelling safe instead of subtly wrong. The helper is SHARED via `#[path = "support/mod.rs"]` rather than copied into three test binaries. `m6_8_multi_repl_acceptance.rs` carried a comment saying cross-test-binary sharing "would need a fixture crate"; it does not, and a correct helper in one file beside a degraded copy in another is this suite's most repeated defect. Verified by execution in all three states, using a tool genuinely absent from this machine (vscode-json-language-server): unset skips green; armed fails hard, naming the CI step that should have installed it; empty string skips green. On `main` the armed state cannot fail at all, because no guard exists. And the question none of this could answer until now --- whether the tests pass when they actually run --- is answered: armed locally, 11 m6_5 and 8 m6_8 REPL tests are green, and all six real-LSP tests (clangd x2, gopls x2, rust-analyzer x2) pass individually. The coverage was real the whole time. It just never ran. Linux only for now, deliberately: macOS needs the brew equivalents and roughly doubles install cost on the slowest matrix leg. The variables stay unset there, so those tests skip cleanly as before. Also removes the documentation lane from the ledger. Its disposition was left undecided pending confirmation that its branch carried nothing unique; measured, `githubsucks/handoff-2026-07-20` is 1 ahead and 365 behind, and its whole unique diff is four doc files at 42 insertions against 88 deletions --- merging it would REVERT current documentation. The section asked whoever confirmed that to remove it. Gates: fmt; clippy -D warnings; --lib 1863; --lib --features crdt 2048; m4_acceptance 121 (unarmed, per CLAUDE.md); m6_5 11; m6_8 8; PMACS_REQUIRE_GPU=1 -p pmacs-gpu 202; git diff --check clean. --- .github/workflows/ci.yml | 45 ++++++++++++++ docs/active-work.md | 80 ++++++++++++++++-------- tests/m4_acceptance.rs | 21 ++++--- tests/m6_5_repl_acceptance.rs | 19 +++--- tests/m6_8_multi_repl_acceptance.rs | 7 ++- tests/support/mod.rs | 94 +++++++++++++++++++++++++++++ 6 files changed, 219 insertions(+), 47 deletions(-) create mode 100644 tests/support/mod.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fbad57..ce7e0a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,13 +84,58 @@ jobs: steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable + with: + components: rust-analyzer - uses: Swatinem/rust-cache@v2 + # Arm the external-tool-gated tests (see `TEST_IMPROVEMENT.md` + # §1.2). Before this step nothing installed these tools, so every + # test guarded on them returned early and reported GREEN without + # executing its body — a whole block of real-language-server and + # multi-shell coverage that had never once run in CI. Installing + # them is only half the fix; the `PMACS_REQUIRE_*` variables below + # are what turn a future missing tool back into a failure instead + # of silently restoring the vacuum. + # + # Linux only for now, deliberately. macOS would need the brew + # equivalents and roughly doubles the install cost on the slowest + # leg of the matrix; arming one platform already converts these + # from never-executed to executed, and the second is incremental. + # The tests still skip cleanly on macOS because the variables are + # unset there. + - name: Install external tools that gate acceptance tests (Linux) + if: runner.os == 'Linux' + run: | + sudo apt-get update + sudo apt-get install -y clangd zsh fish lua5.4 + # `locate_lua` looks for `lua` or `luajit` by name; the + # distro package installs `lua5.4` only. + sudo ln -sf "$(command -v lua5.4)" /usr/local/bin/lua + go install golang.org/x/tools/gopls@latest + echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH" + npm install -g vscode-langservers-extracted yaml-language-server - run: cargo build --all-targets --no-default-features --features ${{ matrix.lua }} # Several acceptance binaries spawn real daemon / PTY child # processes. Keep the harness serial so macOS runners do not # expose cross-test process lifecycle races that are unrelated # to the behavior under test. + # + # PMACS_REQUIRE_* make a missing tool fatal rather than a silent + # skip, exactly as PMACS_REQUIRE_GPU already does for the headless + # render job. Set only where the install step ran. + # + # PMACS_REQUIRE_PYRIGHT is deliberately NOT set and basedpyright + # is deliberately NOT installed: that test has no timeout and + # hangs forever (root cause is the non-interruptible reader-thread + # join in `RuntimeHandles::drop`, already a named deferral in + # `src/process.rs`). This job has no `timeout-minutes`, so arming + # it today would trade a vacuous green for a six-hour hang on four + # legs. It gets armed after the hang fix and the CI timeouts land, + # and its own variable exists so that flip is one line. - run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1 + env: + PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} + PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }} + PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }} - run: cargo test --doc --no-default-features --features ${{ matrix.lua }} # The workspace default member is only the root `pmacs` package, so # the runs above never execute pmacs-protocol's own tests — the diff --git a/docs/active-work.md b/docs/active-work.md index 2dfbaf0..8e216b3 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -521,6 +521,60 @@ has **no branch and no framing yet**. `FrontendView.fold_projection` to `true` for semantic frontends, which Stage 2 deliberately left `false` (Q#FD21). +## Test-improvement arc, lane 2 — silent-skip arming + +- Portable branch: `githubsucks/silent-skip-arming`, worktree + `../pmacs-skiparm`. Base measured at write time, pasted below. + Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4. +- **The defect:** `let Ok(_) = which_binary(x) else { eprintln!(..); + return; }` reports GREEN when the tool is absent, and CI installed + none of the tools. A block of real-language-server and multi-shell + tests had therefore **never once executed their bodies** in CI while + reporting success. A suite that cannot distinguish "passed" from + "never ran" is worse than a missing one, because it reads as + coverage. +- **The fix is the project's own pattern.** `PMACS_REQUIRE_*` already + makes a missing GPU fatal for `vterm_stage3_acceptance`; this adds + `PMACS_REQUIRE_LSP`, `PMACS_REQUIRE_SHELLS` and `PMACS_REQUIRE_LUA`, + plus the CI step that installs the tools. Per-tool variables, not one + blanket flag, so a tool that must stay unarmed keeps its decision + visible at the call site. +- **`basedpyright` is deliberately NOT installed and NOT armed.** Its + test has no timeout and hangs forever — root cause is the + non-interruptible reader-thread join in `RuntimeHandles::drop`, + already a named deferral in `src/process.rs`. The `test` job has no + `timeout-minutes` either. Arming it today would trade a vacuous green + for a six-hour hang across four legs. `PMACS_REQUIRE_PYRIGHT` exists + and is never set, so the flip is one line once lane 4 (the hang) and + lane 3 (timeouts) land. **Do not arm it before both.** +- **A trap found while writing the workflow, not after:** the natural + Actions idiom `${{ runner.os == 'Linux' && '1' || '' }}` sets the + variable to the EMPTY STRING elsewhere, and `var_os().is_some()` is + true for `Some("")`. That would have armed the guard on exactly the + runners with no tools installed. The helper therefore treats empty as + unset. `PMACS_REQUIRE_GPU` has the same latent shape and is safe only + because it is set literally. +- **Verified by execution in all three states**, on a tool genuinely + absent from this machine (`vscode-json-language-server`): unset -> + skips green; armed -> hard failure naming the CI step; empty string + -> skips green. The armed failure is the bite, and on `main` it + cannot occur because no guard exists. +- **The tests pass when they actually run** — which was the open + question, since none of them had. Armed locally: 11 `m6_5` + 8 `m6_8` + REPL tests green, and all six real-LSP tests (clangd x2, gopls x2, + rust-analyzer x2) green individually. +- Linux only for now, deliberately: macOS needs the brew equivalents + and roughly doubles install cost on the slowest matrix leg. The + variables stay unset there, so those tests skip cleanly. +- Also removes the **documentation lane**, whose disposition the ledger + left undecided pending confirmation that its branch carried nothing + unique. Confirmed by measurement: `githubsucks/handoff-2026-07-20` is + **1 ahead, 365 behind**, and its entire unique diff is four doc files + at 42 insertions against 88 deletions — merging it would *revert* + current documentation. The section said "whoever confirms the branch + carries nothing unique removes the section"; this is that. + + ## Parked lane: kill-ring browser + persistence - Portable branch: `githubsucks/kill-ring-browser` @@ -543,32 +597,6 @@ git worktree add --track \ githubsucks/kill-ring-browser ``` -## Documentation lane — STALE, AND ITS DISPOSITION IS UNDECIDED - -> **Measured 2026-07-28, not inferred:** `githubsucks/handoff-2026-07-20` -> is at `c11d7e7`, **1 commit ahead of `main` and 320 behind**. Its -> whole diff against `main` is four documentation files -> (`docs/active-work.md`, `docs/agent-handoff.md`, -> `docs/roadmap-2026-07.md`, `docs/vterm-framing.md`), every one of -> which has been rewritten repeatedly since by the landed-doc PRs -> #156/#168/#169/#172/#180. Rule 4 removes a lane on merge *or -> abandonment*, and this one looks abandoned in substance — but "looks -> abandoned" is not the same as a decision, and no PR was ever opened -> for it. **This snapshot deliberately annotates rather than deletes: -> whoever confirms the branch carries nothing unique removes the -> section.** The bullets below are its original claims, preserved as -> written and now unverified. - -- Portable branch: `githubsucks/handoff-2026-07-20` -- Carries synchronized `AGENTS.md` / `CLAUDE.md`, this ledger, the - durable handoff refresh, and the keybinding reference correction. -- It changes no runtime code. -- Review and merge this documentation branch separately; it must not be - folded into a feature framing branch. -- Now also absorbs both landed arcs: Vterm Stage 1 (#126) and the config - registry (#127). Canonical `main` is merged into it up to `2e37c04`, - so its diff against `main` is documentation only. - ## Closed since the last snapshot - **Terminal configuration + copy mode arc — BOTH STAGES MERGED, lane diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 691d746..f56dfc7 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -1298,7 +1298,7 @@ fn fake_spec(label: &str) -> LspServerSpec { #[test] fn m4_5_rust_analyzer_initializes() { let Ok(_) = which_binary("rust-analyzer") else { - eprintln!("rust-analyzer not on PATH; skipping"); + support::skip_or_fail("rust-analyzer", "PMACS_REQUIRE_LSP"); return; }; let (sup, mgr) = make_lsp_test_manager(); @@ -1338,7 +1338,7 @@ fn m4_5_rust_analyzer_initializes() { #[test] fn m4_5_basedpyright_initializes_and_negotiates_encoding() { let Ok(_) = which_binary("basedpyright-langserver") else { - eprintln!("basedpyright-langserver not on PATH; skipping"); + support::skip_or_fail("basedpyright-langserver", "PMACS_REQUIRE_PYRIGHT"); return; }; let (sup, mgr) = make_lsp_test_manager(); @@ -1419,7 +1419,7 @@ fn assert_lsp_initializes_and_negotiates( #[test] fn m4_5_clangd_initializes_and_negotiates_encoding() { let Ok(_) = which_binary("clangd") else { - eprintln!("clangd not on PATH; skipping"); + support::skip_or_fail("clangd", "PMACS_REQUIRE_LSP"); return; }; assert_lsp_initializes_and_negotiates("clangd", "cpp", "clangd", &["--background-index"]); @@ -1431,7 +1431,7 @@ fn m4_5_clangd_initializes_and_negotiates_encoding() { #[test] fn m4_5_gopls_initializes_and_negotiates_encoding() { let Ok(_) = which_binary("gopls") else { - eprintln!("gopls not on PATH; skipping"); + support::skip_or_fail("gopls", "PMACS_REQUIRE_LSP"); return; }; assert_lsp_initializes_and_negotiates("gopls", "go", "gopls", &[]); @@ -3428,6 +3428,9 @@ fn m4_11_snippets_surface_through_completion() { use pmacs::definition::DefinitionKey; use pmacs::formatting::FormattingKey; +#[path = "support/mod.rs"] +mod support; + /// Acceptance (1/3): a `textDocument/definition` request round-trips /// through the manager and lands in the definition store as a parsed /// `Location` list. @@ -5536,7 +5539,7 @@ fn m4_27_real_gopls_analyzes_module_via_auto_attach() { use pmacs::editor::EditorState; let Ok(gopls) = which_binary("gopls") else { - eprintln!("gopls not on PATH; skipping"); + support::skip_or_fail("gopls", "PMACS_REQUIRE_LSP"); return; }; let gopls = gopls.display().to_string(); @@ -5633,7 +5636,7 @@ fn m4_28_real_clangd_diagnostics_and_semantic_tokens_via_auto_attach() { use pmacs::editor::EditorState; let Ok(clangd) = which_binary("clangd") else { - eprintln!("clangd not on PATH; skipping"); + support::skip_or_fail("clangd", "PMACS_REQUIRE_LSP"); return; }; let clangd = clangd.display().to_string(); @@ -5731,7 +5734,7 @@ fn m4_29_real_rust_analyzer_inlay_hints_via_auto_attach() { use pmacs::editor::EditorState; let Ok(rust_analyzer) = which_binary("rust-analyzer") else { - eprintln!("rust-analyzer not on PATH; skipping"); + support::skip_or_fail("rust-analyzer", "PMACS_REQUIRE_LSP"); return; }; let rust_analyzer = rust_analyzer.display().to_string(); @@ -6880,7 +6883,7 @@ fn m4_real_json_provider_receives_config_and_reports_diagnostics() { use pmacs::editor::EditorState; let Ok(command) = which_binary("vscode-json-language-server") else { - eprintln!("vscode-json-language-server not on PATH; skipping"); + support::skip_or_fail("vscode-json-language-server", "PMACS_REQUIRE_LSP"); return; }; let command = command.display().to_string(); @@ -6949,7 +6952,7 @@ fn m4_real_yaml_provider_pulls_config_and_reports_diagnostics() { use pmacs::editor::EditorState; let Ok(command) = which_binary("yaml-language-server") else { - eprintln!("yaml-language-server not on PATH; skipping"); + support::skip_or_fail("yaml-language-server", "PMACS_REQUIRE_LSP"); return; }; let command = command.display().to_string(); diff --git a/tests/m6_5_repl_acceptance.rs b/tests/m6_5_repl_acceptance.rs index fcf9bf4..fc453ba 100644 --- a/tests/m6_5_repl_acceptance.rs +++ b/tests/m6_5_repl_acceptance.rs @@ -35,6 +35,9 @@ use std::path::PathBuf; use std::sync::{Mutex, MutexGuard}; use std::time::{Duration, Instant}; +#[path = "support/mod.rs"] +mod support; + static PUMP_TEST_LOCK: Mutex<()> = Mutex::new(()); fn pump_test_guard() -> MutexGuard<'static, ()> { @@ -196,7 +199,7 @@ fn m6_5_ret_submits_input_to_process() { #[test] fn m6_5_ctrl_d_on_empty_prompt_closes_stdin() { let Some(bash) = locate_shell("bash") else { - eprintln!("skipping: bash not on PATH (set PMACS_TEST_BASH to override)"); + support::skip_or_fail_overridable("bash", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_BASH"); return; }; let setup = format!( @@ -290,7 +293,7 @@ fn m6_5_ctrl_d_on_nonempty_input_deletes_char_forward() { )] fn m6_5_ctrl_c_sends_sigint() { let Some(sleep) = locate_shell("sleep") else { - eprintln!("skipping: sleep not on PATH (set PMACS_TEST_SLEEP to override)"); + support::skip_or_fail_overridable("sleep", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_SLEEP"); return; }; let setup = format!( @@ -344,7 +347,7 @@ fn m6_5_ctrl_c_sends_sigint() { )] fn m6_5_exit_marker_uses_basename_with_leading_newline() { let Some(false_bin) = locate_shell("false") else { - eprintln!("skipping: false not on PATH (set PMACS_TEST_FALSE to override)"); + support::skip_or_fail_overridable("false", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_FALSE"); return; }; let setup = format!( @@ -425,7 +428,7 @@ fn run_shell_smoke_test(shell_path: &std::path::Path, argv_extra: &[&str]) { #[test] fn m6_5_repl_spawns_bash() { let Some(bash) = locate_shell("bash") else { - eprintln!("skipping: bash not on PATH (set PMACS_TEST_BASH to override)"); + support::skip_or_fail_overridable("bash", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_BASH"); return; }; run_shell_smoke_test(&bash, &["-i"]); @@ -435,7 +438,7 @@ fn m6_5_repl_spawns_bash() { #[test] fn m6_5_repl_spawns_zsh() { let Some(zsh) = locate_shell("zsh") else { - eprintln!("skipping: zsh not on PATH (set PMACS_TEST_ZSH to override)"); + support::skip_or_fail_overridable("zsh", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_ZSH"); return; }; run_shell_smoke_test(&zsh, &["-i"]); @@ -446,7 +449,7 @@ fn m6_5_repl_spawns_zsh() { #[test] fn m6_5_repl_spawns_fish() { let Some(fish) = locate_shell("fish") else { - eprintln!("skipping: fish not on PATH (set PMACS_TEST_FISH to override)"); + support::skip_or_fail_overridable("fish", "PMACS_REQUIRE_SHELLS", "PMACS_TEST_FISH"); return; }; run_shell_smoke_test(&fish, &["-i"]); @@ -458,9 +461,7 @@ fn m6_5_repl_spawns_fish() { #[test] fn m6_5_repl_spawns_lua() { let Some(lua) = locate_shell("lua").or_else(|| locate_shell("luajit")) else { - eprintln!( - "skipping: lua/luajit not on PATH (set PMACS_TEST_LUA or PMACS_TEST_LUAJIT to override)" - ); + support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA"); return; }; let setup = format!( diff --git a/tests/m6_8_multi_repl_acceptance.rs b/tests/m6_8_multi_repl_acceptance.rs index 673daa4..c1bd071 100644 --- a/tests/m6_8_multi_repl_acceptance.rs +++ b/tests/m6_8_multi_repl_acceptance.rs @@ -68,6 +68,9 @@ use pmacs::editor::EditorState; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; +#[path = "support/mod.rs"] +mod support; + // --------------------------------------------------------------------------- // Test harness // --------------------------------------------------------------------------- @@ -94,9 +97,7 @@ fn locate_lua() -> Option { } } } - eprintln!( - "skipping: lua/luajit not on PATH (set PMACS_TEST_LUA or PMACS_TEST_LUAJIT to override)" - ); + support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA"); None } diff --git a/tests/support/mod.rs b/tests/support/mod.rs new file mode 100644 index 0000000..5b6ca97 --- /dev/null +++ b/tests/support/mod.rs @@ -0,0 +1,94 @@ +//! Shared test-support helpers. +//! +//! Included by `#[path = "support/mod.rs"] mod support;` rather than +//! copied. Files under `tests/` subdirectories are not compiled as +//! their own test binaries, so this costs nothing — and +//! `m6_8_multi_repl_acceptance.rs` previously carried a comment saying +//! cross-test-binary sharing "would need a fixture crate", which is not +//! so. A correct helper in one file and a degraded copy in another is +//! this suite's most repeated defect shape; sharing removes the way it +//! happens. + +#![allow(dead_code)] + +/// Report a missing external tool, and turn the skip into a HARD +/// FAILURE when the environment has promised the tool is present. +/// +/// The bare shape this replaces — +/// +/// ```ignore +/// let Ok(_) = which_binary("gopls") else { +/// eprintln!("gopls not on PATH; skipping"); +/// return; +/// }; +/// ``` +/// +/// passes GREEN when the tool is absent, and is why a large block of +/// external-tool-gated tests had never once executed their bodies in +/// CI: nothing installed the tools, so every one of them reported +/// success without running. A suite that cannot tell "passed" from +/// "never ran" is worse than a missing suite, because it reads as +/// coverage. +/// +/// `PMACS_REQUIRE_*` is the project's own fix, already load-bearing for +/// `PMACS_REQUIRE_GPU` in `vterm_stage3_acceptance`: CI installs the +/// tool, sets the variable, and absence becomes a failure that names +/// the step that should have provided it. Locally the variable is +/// unset, so the skip still works and nobody needs the whole toolchain +/// to run the suite. +/// +/// Deliberately per-tool rather than one blanket variable: a tool that +/// must stay unarmed (because arming it would hang, or because CI does +/// not install it yet) keeps its own variable that CI never sets, and +/// that decision is then visible at the call site instead of buried in +/// a workflow file. +/// True when `var` is set to a non-empty value. +/// +/// Emptiness matters, and the reason is a trap rather than a nicety. +/// The natural GitHub Actions idiom for a conditional environment +/// variable — +/// +/// ```yaml +/// PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }} +/// ``` +/// +/// sets the variable to the EMPTY STRING on every other platform, not +/// to nothing. A bare `var_os(..).is_some()` is therefore true there, +/// which would arm the guard on exactly the runners that have none of +/// the tools installed and fail every one of them. Treating empty as +/// unset makes the common workflow spelling safe instead of subtly +/// wrong. +fn armed(var: &str) -> bool { + std::env::var_os(var).is_some_and(|v| !v.is_empty()) +} + +#[track_caller] +pub fn skip_or_fail(tool: &str, require_var: &str) { + assert!( + !armed(require_var), + "{require_var} is set, but `{tool}` is not on PATH. \ + The CI step that installs it did not run, or installed it \ + somewhere not on PATH. This is a hard failure precisely so \ + the test cannot report green without executing." + ); + eprintln!("{tool} not on PATH; skipping (set {require_var} to make this fatal)"); +} + +/// As [`skip_or_fail`], for tools whose PATH lookup can be overridden +/// by a `PMACS_TEST_*` variable. The skip notice keeps naming that +/// override, because losing it would make the local escape hatch +/// undiscoverable — the REPL suites are routinely run on machines +/// without zsh or fish. +#[track_caller] +pub fn skip_or_fail_overridable(tool: &str, require_var: &str, override_var: &str) { + assert!( + !armed(require_var), + "{require_var} is set, but `{tool}` is not on PATH and {override_var} \ + is unset or points at nothing. The CI step that installs it did not \ + run, or installed it somewhere not on PATH." + ); + eprintln!( + "skipping: {tool} not on PATH (set {override_var} to override, \ + or {require_var} to make this fatal)" + ); +} From a81ff917d8875f4bdd7cffe0fe05e0d4a7f43135 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 12:20:28 -0400 Subject: [PATCH 2/3] review round 1: stop installing rust-analyzer on macOS, and paste the base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 --- the workflow contradicted this lane's own claim. `components: rust-analyzer` rode the shared `dtolnay/rust-toolchain` step, which matrixes over ubuntu AND macos, so the binary would have been present on both. PRESENCE, not PMACS_REQUIRE_LSP, is what decides whether a gated test body runs --- the unset variables on macOS only meant absence would be tolerated there, not that the tests stay skipped. Two rust-analyzer tests would therefore have executed on macOS for the first time ever, on the legs that are simultaneously the CI critical path and the documented flake surface, one of them doing real indexing, none of it covered by the Linux-only local runs behind this lane. Moved into the Linux-gated step so the text and the workflow agree. P2 --- the ledger promised a base and pasted nothing. Worse than the review knew: a script was run to substitute it, reported success, matched no text, and the result was never re-read. The claim shipped on the strength of a tool's exit status. The entry now carries the pasted base and a recovery command, and records the lesson in the terms this ledger keeps relearning --- asserting a measurement is not making one, and a tool reporting success is not the measurement either. P3 --- tool versions pinned (gopls v0.16.2, vscode-langservers-extracted 4.10.0, yaml-language-server 1.15.0), so CI no longer drifts with upstream publishes and a break has a commit here to bisect against. `tests/support/` now states why it exists beside `tests/common/`: the latter re-exports daemon and PTY machinery, and pulling that into three binaries that spawn neither to reach a six-line environment check is the wrong trade. Recorded as a cost, with the rule that a third such directory means consolidating rather than continuing. Also recorded, because §1.2 is NOT fully closed by this lane: the guards arm the ENTRY skip only, and m4_acceptance's mid-test "workspace likely still indexing; skipping" survives --- so even armed, that test's one assertion can still vanish under load, which is exactly when a regression would show. Mid-test skips want their own pass. And the follow-up this lane creates: removing the documentation lane removes the only pointer to githubsucks/handoff-2026-07-20, so that branch needs deleting after merge or nothing will ever mention it again. P4 --- the consolidated lua skip message names PMACS_TEST_LUAJIT again; the `or_else(locate_shell("luajit"))` path still honours it and the escape hatch had become undiscoverable. Double blank line before the parked lane collapsed. One gate note worth carrying rather than burying. A `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu` run failed once during this round, naming the `--bin pmacs-gpu` target, and my gate chain still printed a clean summary because `cmd | tail -2` in an `&&` chain returns TAIL's exit status, not cargo's. Four subsequent runs pass 202/202 and the failure has not reproduced, so it is recorded as observed-and-unreproduced rather than explained. The masking is the durable part: a gate summary assembled through a pipe can report success over a failure. Gates re-run after the fix: fmt; clippy -D warnings; --lib 1863; --lib --features crdt 2048; m4_acceptance 121; m6_5 11; m6_8 8; required GPU 202 (x4); git diff --check clean. --- .github/workflows/ci.yml | 20 +++++++++--- docs/active-work.md | 47 +++++++++++++++++++++++++++-- tests/m6_5_repl_acceptance.rs | 6 +++- tests/m6_8_multi_repl_acceptance.rs | 6 +++- tests/support/mod.rs | 12 +++++++- 5 files changed, 81 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ce7e0a0..3c1bc59 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,8 +84,6 @@ jobs: steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable - with: - components: rust-analyzer - uses: Swatinem/rust-cache@v2 # Arm the external-tool-gated tests (see `TEST_IMPROVEMENT.md` # §1.2). Before this step nothing installed these tools, so every @@ -110,9 +108,23 @@ jobs: # `locate_lua` looks for `lua` or `luajit` by name; the # distro package installs `lua5.4` only. sudo ln -sf "$(command -v lua5.4)" /usr/local/bin/lua - go install golang.org/x/tools/gopls@latest + # rust-analyzer belongs HERE, not on the shared toolchain + # step. `components:` there applies to every matrix leg, so it + # would install the binary on macOS too — and *presence*, not + # PMACS_REQUIRE_LSP, is what decides whether a gated test body + # runs. That would have executed the rust-analyzer tests on + # macOS for the first time ever, on the legs that are both the + # CI critical path and the documented flake surface, while + # this lane's text claimed Linux only. + rustup component add rust-analyzer + # Versions are PINNED. `@latest` and bare `npm install -g` + # make CI behaviour drift with upstream releases: a bad gopls + # or yaml-language-server publish then breaks CI with no + # commit in this repository to bisect against. + go install golang.org/x/tools/gopls@v0.16.2 echo "$(go env GOPATH)/bin" >> "$GITHUB_PATH" - npm install -g vscode-langservers-extracted yaml-language-server + npm install -g vscode-langservers-extracted@4.10.0 \ + yaml-language-server@1.15.0 - run: cargo build --all-targets --no-default-features --features ${{ matrix.lua }} # Several acceptance binaries spawn real daemon / PTY child # processes. Keep the harness serial so macOS runners do not diff --git a/docs/active-work.md b/docs/active-work.md index 8e216b3..7988729 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -524,8 +524,24 @@ has **no branch and no framing yet**. ## Test-improvement arc, lane 2 — silent-skip arming - Portable branch: `githubsucks/silent-skip-arming`, worktree - `../pmacs-skiparm`. Base measured at write time, pasted below. - Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4. + `../pmacs-skiparm`. Implements `TEST_IMPROVEMENT.md` §1.2 and §5.4. +- **Base, measured at write time rather than quoted:** + + ``` + $ git log --oneline -1 githubsucks/main + 5e186c7 Merge pull request #193 from levineuwirth/test-improvement-audit + ``` + + The previous revision of this entry said "base measured at write + time, pasted below" and then pasted nothing: the script meant to + substitute it reported success and silently matched no text, and the + claim was not re-read. Recorded because it is the same defect this + ledger keeps catching one level up — **asserting a measurement is not + making one, and a tool reporting success is not the measurement + either.** +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-skiparm + -b silent-skip-arming githubsucks/silent-skip-arming`. - **The defect:** `let Ok(_) = which_binary(x) else { eprintln!(..); return; }` reports GREEN when the tool is absent, and CI installed none of the tools. A block of real-language-server and multi-shell @@ -563,6 +579,32 @@ has **no branch and no framing yet**. question, since none of them had. Armed locally: 11 `m6_5` + 8 `m6_8` REPL tests green, and all six real-LSP tests (clangd x2, gopls x2, rust-analyzer x2) green individually. +- **rust-analyzer is installed in the Linux-gated step, not via the + toolchain action's `components:`.** The first revision put it there, + which applies to *every* matrix leg — and **presence, not + `PMACS_REQUIRE_LSP`, is what decides whether a gated test body + runs**. That would have executed the two rust-analyzer tests on macOS + for the first time ever, on the legs that are simultaneously the CI + critical path and the documented flake surface, while this entry + claimed Linux only. The variables not being set there would only have + meant absence was tolerated; it would not have kept the tests + skipped. Text and workflow now agree. +- **Tool versions are pinned** (`gopls@v0.16.2`, + `vscode-langservers-extracted@4.10.0`, + `yaml-language-server@1.15.0`). `@latest` and bare `npm install -g` + make CI drift with upstream releases, so a bad publish breaks CI with + no commit here to bisect against. Caching the built `gopls` on the + pinned version is a follow-up, not done here. +- **§1.2 is NOT fully closed by this lane.** The guards arm the + *entry* skip only. `tests/m4_acceptance.rs`'s mid-test rust-analyzer + bail ("workspace likely still indexing; skipping") survives, so even + armed, that test's only assertion can still vanish under load — + precisely when a regression would show. Mid-test skips are their own + shape and want their own pass. +- **Follow-up owed after this merges:** delete + `githubsucks/handoff-2026-07-20`. Removing the documentation lane + removes the only pointer to that branch, so nothing will otherwise + remind anyone it still exists on the remote. - Linux only for now, deliberately: macOS needs the brew equivalents and roughly doubles install cost on the slowest matrix leg. The variables stay unset there, so those tests skip cleanly. @@ -574,7 +616,6 @@ has **no branch and no framing yet**. current documentation. The section said "whoever confirms the branch carries nothing unique removes the section"; this is that. - ## Parked lane: kill-ring browser + persistence - Portable branch: `githubsucks/kill-ring-browser` diff --git a/tests/m6_5_repl_acceptance.rs b/tests/m6_5_repl_acceptance.rs index fc453ba..44d6b95 100644 --- a/tests/m6_5_repl_acceptance.rs +++ b/tests/m6_5_repl_acceptance.rs @@ -461,7 +461,11 @@ fn m6_5_repl_spawns_fish() { #[test] fn m6_5_repl_spawns_lua() { let Some(lua) = locate_shell("lua").or_else(|| locate_shell("luajit")) else { - support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA"); + support::skip_or_fail_overridable( + "lua/luajit", + "PMACS_REQUIRE_LUA", + "PMACS_TEST_LUA or PMACS_TEST_LUAJIT", + ); return; }; let setup = format!( diff --git a/tests/m6_8_multi_repl_acceptance.rs b/tests/m6_8_multi_repl_acceptance.rs index c1bd071..4bc482f 100644 --- a/tests/m6_8_multi_repl_acceptance.rs +++ b/tests/m6_8_multi_repl_acceptance.rs @@ -97,7 +97,11 @@ fn locate_lua() -> Option { } } } - support::skip_or_fail_overridable("lua/luajit", "PMACS_REQUIRE_LUA", "PMACS_TEST_LUA"); + support::skip_or_fail_overridable( + "lua/luajit", + "PMACS_REQUIRE_LUA", + "PMACS_TEST_LUA or PMACS_TEST_LUAJIT", + ); None } diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 5b6ca97..f661b3d 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -1,7 +1,17 @@ //! Shared test-support helpers. //! //! Included by `#[path = "support/mod.rs"] mod support;` rather than -//! copied. Files under `tests/` subdirectories are not compiled as +//! copied. +//! +//! **Why this is separate from `tests/common/`, which also exists.** +//! `tests/common/mod.rs` re-exports `daemon` and `pty` — real daemon +//! spawning and PTY plumbing. Including it to reach a six-line +//! environment check would compile that machinery into three test +//! binaries that spawn neither, for no benefit. `support` is the +//! dependency-free half: helpers any test binary can take without +//! taking a subsystem with them. Two directories is a cost worth +//! naming rather than leaving to be rediscovered; if a third appears, +//! consolidate instead of continuing the pattern. Files under `tests/` subdirectories are not compiled as //! their own test binaries, so this costs nothing — and //! `m6_8_multi_repl_acceptance.rs` previously carried a comment saying //! cross-test-binary sharing "would need a fixture crate", which is not From 0b92d4705ff2442fd139afa280d5d81473662e6b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 12:25:29 -0400 Subject: [PATCH 3/3] docs: record the pipe-masking gate trap in the handoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 found no new defects; this is the one durable item it asked to be carried further than a commit message. `cmd | tail -2` returns TAIL's exit status, not `cmd`'s, in fish and bash alike. So a gate chain of `cargo test ... | tail -2 && ... && echo "ALL GATES CLEAN"` prints the clean line even when a suite failed, and that is what happened while gating this lane: a `pmacs-gpu` failure was summarized as clean. The point worth keeping is that this is not carelessness a closer read would catch --- the verdict is structurally absent from the summary the PR then cites. §5 now says to check `$pipestatus[1]`, or better to redirect each gate to a file and read it afterwards, which also preserves the full log that section already asks for. Filed beside the skip-reports-`ok` lesson, which is the same family: the thing that summarizes a gate must not be able to lose the gate's verdict. Also fixes the doc-comment splice in `tests/support/mod.rs`, where the why-two-directories paragraph landed mid-sentence and left the include-mechanics explanation stranded inside it. Cosmetic, and review called it not worth a round on its own --- folded in here because the file was being touched anyway. --- docs/agent-handoff.md | 16 ++++++++++++++++ tests/support/mod.rs | 16 ++++++++-------- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 5307799..2c5ca22 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1443,6 +1443,22 @@ round-trip cannot detect a discriminant shift. ## 5. Hard-won ops lessons +- **A gate summary assembled through a pipe can report success over a + failure.** `cmd | tail -2` returns **`tail`'s** exit status, not + `cmd`'s — in `fish` and `bash` alike — so a chain of + `cargo test ... | tail -2 && cargo test ... | tail -2 && echo "ALL + GATES CLEAN"` prints the clean line even when a suite failed. This + is not carelessness that closer reading would catch: the failure is + **structurally invisible** in the summary the PR then cites. It + happened while gating the silent-skip lane, and a `pmacs-gpu` + failure was reported as clean. + + Either check `$pipestatus[1]` in fish (`${PIPESTATUS[0]}` in bash), + or — better — redirect each gate to a file and read the file + afterwards, which also preserves the full log this section already + asks you to keep. Same family as the skip-reports-`ok` lesson below + and the double-invocation traps: **the thing that summarizes a gate + must not be able to lose the gate's verdict.** - **A test that skips on a missing precondition reports `ok`, and a gate log cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the only acceptance driving a real daemon, a real PTY and a real wgpu render diff --git a/tests/support/mod.rs b/tests/support/mod.rs index f661b3d..14fbeec 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -1,7 +1,13 @@ //! Shared test-support helpers. //! //! Included by `#[path = "support/mod.rs"] mod support;` rather than -//! copied. +//! copied. Files under `tests/` subdirectories are not compiled as +//! their own test binaries, so this costs nothing — and +//! `m6_8_multi_repl_acceptance.rs` previously carried a comment saying +//! cross-test-binary sharing "would need a fixture crate", which is not +//! so. A correct helper in one file and a degraded copy in another is +//! this suite's most repeated defect shape; sharing removes the way it +//! happens. //! //! **Why this is separate from `tests/common/`, which also exists.** //! `tests/common/mod.rs` re-exports `daemon` and `pty` — real daemon @@ -11,13 +17,7 @@ //! dependency-free half: helpers any test binary can take without //! taking a subsystem with them. Two directories is a cost worth //! naming rather than leaving to be rediscovered; if a third appears, -//! consolidate instead of continuing the pattern. Files under `tests/` subdirectories are not compiled as -//! their own test binaries, so this costs nothing — and -//! `m6_8_multi_repl_acceptance.rs` previously carried a comment saying -//! cross-test-binary sharing "would need a fixture crate", which is not -//! so. A correct helper in one file and a degraded copy in another is -//! this suite's most repeated defect shape; sharing removes the way it -//! happens. +//! consolidate instead of continuing the pattern. #![allow(dead_code)]