merge: integrate main @ b7bf2c6 after PR 190 review round 2
Keep both active-work lanes while taking the silent-skip arming and generated-buffer framing changes from current main. The resource-op lane retains its round-2 fixes and updates its recorded merge-base.
This commit is contained in:
commit
ef5bd89e84
|
|
@ -85,12 +85,69 @@ jobs:
|
|||
- uses: actions/checkout@v4
|
||||
- uses: dtolnay/rust-toolchain@stable
|
||||
- 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
|
||||
# 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@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
|
||||
# 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
|
||||
|
|
|
|||
|
|
@ -13,10 +13,10 @@ precondition, satisfied deliberately rather than deferred. The
|
|||
bottom-panel lane is not removed: 2B-2 landing leaves 2B-3 and Stage 3
|
||||
ahead of it, so the lane is rewritten to the remaining plan.
|
||||
|
||||
**One open PR has no lane here: #188**, the generated-buffer
|
||||
immutability framing. Its lane lives on its own branch and arrives with
|
||||
it, which is deliberate — with several PRs open, a lane written here for
|
||||
work that lands elsewhere re-conflicts on every merge.
|
||||
**#188's lane arrived with #188**, which is the point: with several PRs
|
||||
open, a lane written on `main` for work that lands elsewhere
|
||||
re-conflicts on every merge. Written on its own branch it costs one
|
||||
conflict, at the merge that would have happened anyway.
|
||||
|
||||
The PTY terminate
|
||||
diagnostic (#176) was the last lane retained past its merge — retained
|
||||
|
|
@ -532,7 +532,7 @@ has **no branch and no framing yet**.
|
|||
|
||||
```
|
||||
$ git merge-base HEAD githubsucks/main
|
||||
5e186c798236e244a0a6f68d5df79458b4856106
|
||||
b7bf2c664475c36b60cf7c0361ea75cd3c3b2315
|
||||
```
|
||||
|
||||
That is the whole durable fact, and it is deliberately the ONLY
|
||||
|
|
@ -545,11 +545,12 @@ has **no branch and no framing yet**.
|
|||
still meaningful.
|
||||
|
||||
**Re-measure the merge-base too before relying on it.** `main` moved
|
||||
TWICE while this lane's round-1 fixes were being written — #192 and
|
||||
then #193. `main` has branch protection now, so a stale base is not
|
||||
merely untidy: all 12 checks must pass on the merging head, and a
|
||||
conflicting PR builds no merge ref at all, so a green run from before
|
||||
the move reads as current when it is not.
|
||||
twice while this lane's round-1 fixes were being written (#192 and
|
||||
#193), then twice more during round 2 (#188 and #194). This branch
|
||||
integrates through #194. `main` has branch protection now, so a stale
|
||||
base is not merely untidy: all 12 checks must pass on the merging head,
|
||||
and a conflicting PR builds no merge ref at all, so a green run from
|
||||
before the move reads as current when it is not.
|
||||
- **The framing's §8 branch plan is superseded and cannot be followed.**
|
||||
It says "one PR — #186, which becomes the implementation PR", written
|
||||
when #186 was still open. #186 merged as framing-only, so the
|
||||
|
|
@ -654,7 +655,304 @@ has **no branch and no framing yet**.
|
|||
`git fetch githubsucks && git worktree add ../pmacs-rd-impl
|
||||
-b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`.
|
||||
|
||||
## Generated-buffer immutability framing lane — PR #188 OPEN, PROPOSED
|
||||
|
||||
- Portable branch: `githubsucks/generated-buffer-immutability`; worktree
|
||||
`../pmacs-generated-immutability`. **PR #188**, base `main`, forked from
|
||||
`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,
|
||||
twenty-two P1, ten P2). Not approved. Do not implement, do not merge.**
|
||||
- **Stage 1 implementation is PR #191, open. The boundary is explicit
|
||||
and has already been needed twice:** #188 owns the **acceptance
|
||||
contract**; #191 **adopts** criteria and may not restate, narrow, or
|
||||
reclassify them. Where an implementation finds a criterion impossible,
|
||||
the framing is revised and re-approved first. The **selection-anchor
|
||||
clamp** is Q#GB6's to specify and #191's to implement, and both must
|
||||
describe the same rule. Round 5 found #191 had locally restated Stage 1
|
||||
criteria 5 and 7 while #188 still carried the originals — a divergence
|
||||
neither lane's gates can catch, because each is green against its own
|
||||
description.
|
||||
- **Round 5's two corrections that other lanes need:**
|
||||
- **`Window::Selection::anchor` is an unclamped byte position and it
|
||||
PANICS**, not merely dangles. `Window::region` (`src/window.rs:472-479`)
|
||||
clamps neither endpoint; `region_bytes` (`src/editor_core.rs:4184-4191`)
|
||||
hands the result to `Rope::slice`, which asserts at `src/rope.rs:145`.
|
||||
Reproduced by #191: select 0..30, shrink the buffer to two bytes,
|
||||
copy. The fix is **clamp-or-clear** in **both** `notify_buffer_edit`
|
||||
(`src/editor_core.rs:1836-1850`, clamps nothing today) and
|
||||
`rebuild_views_for` (`:1865-1882`, clamps cursor and view_top but not
|
||||
selection) — and the rule is already in the tree for the terminal's
|
||||
own selection type at `src/terminal/view.rs:715-721`. A stale anchor
|
||||
also reaches the presence broadcast (`src/presence.rs:122-123`).
|
||||
- **`Buffer`'s `on_edit` broadcast stops at the first error in FOUR
|
||||
places**, so a view later in attach order keeps pre-edit offsets:
|
||||
`src/buffer.rs:1288` (`apply_edit` / `apply_edit_skip_intercepts`),
|
||||
`:1250` (the no-op early-return arm), `:1033`
|
||||
(`apply_remote_crdt_op` — the replica import path), and `:1543`
|
||||
(`broadcast_on_edit`, i.e. **undo and redo**). Any lane relying on a
|
||||
buffer-attached view staying in step with the rope is affected.
|
||||
- **What it frames.** The class-wide half of the `set_generated_contents`
|
||||
invariant that `docs/agent-handoff.md` §4 and `COHERENCE.md` §14 both
|
||||
record as unfinished: `Buffer::undo` gates on `ensure_writable()`
|
||||
(`src/buffer.rs:1302`) and never consults the intercept chain, so the
|
||||
`add_intercept`-plus-`bypass_intercept` idiom leaves the rope writable
|
||||
and every affected buffer emptiable. All five families were reproduced
|
||||
by execution at `ad41cf1`, not inferred; the transcripts are in the
|
||||
document's §0 and §2.
|
||||
- **Recommended primitive:** `Buffer::apply_generated_edit(op)`, exposed
|
||||
as a `{ generated = true }` option on the existing Lua mutators, with
|
||||
`set_generated_contents` reimplemented as its whole-buffer wrapper. It
|
||||
is the only candidate in which the buffer is never observably unlocked.
|
||||
**Revision 3 pins the transaction** (framing §3.4): its **own**
|
||||
`run_buffer_edit` arm — **not** the bypass arm, which calls
|
||||
`begin_edit`, which calls `ensure_writable` first (`src/buffer.rs:725`)
|
||||
and would refuse every generated write to a locked buffer — one
|
||||
`&mut Buffer` method with every exit named.
|
||||
**Revision 4 replaces revision 3's cleanup predicate.** Cleanup is
|
||||
driven by an explicit five-variant `GeneratedOutcome` reported by the
|
||||
apply, **not** inferred from `revision`. Inferring it was wrong three
|
||||
ways: a successful no-op (`src/buffer.rs:1245-1253` returns `Ok`
|
||||
without bumping `revision`) kept history the contract forbids; a CRDT
|
||||
mid-transaction failure happens **upstream of `revision` entirely**
|
||||
(`:1140-1163`), so it was neither cleaned nor detected; and the
|
||||
unconditional relock **locked a fresh buffer that was never
|
||||
successfully written**. `NoOp` clears, `Rejected` restores the entry
|
||||
lock state, `Diverged` clears nothing and surfaces. **Revision 5 keeps
|
||||
the five outcomes but preserves the `Edit` in
|
||||
`AppliedThenFailed { edit, error }`: the borrow-free Lua finisher fans
|
||||
it out to window caches and replica mirrors before returning the
|
||||
error.** Collapsing to `Result` inside `Buffer` was too early.
|
||||
**Revision 6 replaces the delete→insert enumeration with a
|
||||
`crdt_mutated` flag**, because `export_updates_since` can fail after
|
||||
every successful CRDT op shape, and changes all four `Buffer`
|
||||
broadcast loops to continue-and-retain-first-error.
|
||||
**Revision 7 installs quarantine at common divergence detection
|
||||
before generated or ordinary callers map the outcome.** The monotonic
|
||||
poison blocks all three CRDT snapshot exporters, daemon-origin
|
||||
queueing and every later owner-generated write; criteria 16c and 16d
|
||||
pin the containment boundary. Repair remains deferred.
|
||||
- **Two stages, two PRs.** Stage 1 — listview ownership fix **plus its
|
||||
identity-routing fix in the same PR**, dired and listview adopting the
|
||||
shipped primitive, the window-coordinate clamp, and the fold decision.
|
||||
Stage 2 — the new primitive, compile's nine write sites, the search
|
||||
panel's four, compile/search ownership + routing, the path-backed
|
||||
refusal plus `mark_clean`, and the terminal-only
|
||||
`identity_protected` guard. **No Lua unlock ships.**
|
||||
- **Nine facts from this lane that other lanes need before it merges:**
|
||||
- **`bypass_intercept` is the wrong inventory key.** It misses
|
||||
`*buffer-list*`, `*help*` and `*workers*`, which are generated with
|
||||
plain writes and no intercept at all. `docs/agent-handoff.md` §4's
|
||||
four-row table inherits that blind spot — **and undercounts by one**:
|
||||
`src/help.rs:354` `replace_help_buffer` is a fifth writer mechanism
|
||||
(own find-or-create, `Buffer::apply_edit`, own `mark_clean`) writing
|
||||
the **same** `*help*` buffer as `default.lua:1239`, which does not
|
||||
mark clean. Two owners, one buffer, two copies of the name constant
|
||||
across the FFI boundary.
|
||||
- **`COHERENCE.md` §14's listview consumer list was wrong and is now
|
||||
FIXED** — PR #189 (`main` @ `7586905`) landed exactly the correction
|
||||
this lane measured. Nothing owed. Recorded so it is not re-asserted.
|
||||
- **Three writers adopt any buffer sharing their name** —
|
||||
`listview.lua:95`, `compile.lua:263`, `default.lua:861-868` — against
|
||||
a rule the tree already states at `terminal.lua:300-305` and
|
||||
implements at `dired.lua:476-504`. Measured: a foreign
|
||||
`*references*` is clobbered and left permanently un-editable, and a
|
||||
`pmacs.compile.run` that **raises on validation** still leaves a
|
||||
foreign `*compilation*` un-editable. Today `M-x buffer.undo` — this
|
||||
arc's bug — is the only recovery, so the arc must not lock these
|
||||
buffers before fixing ownership.
|
||||
- **Disambiguating a name breaks the sites that read one.** Census in
|
||||
framing §2.10: 19 units across 14 grep lines, two genuinely broken.
|
||||
`listview.lua:44`'s `panels[d.name]` (written under the *requested*
|
||||
name at `:97`, read under the *actual* name) has **four** consumers,
|
||||
and the fourth — `listview.open:118-123`'s never-capture-a-panel
|
||||
guard — fails **inverted and silently**, capturing a panel as its own
|
||||
`q` target. `compile.lua:216`'s `is_generated_buffer` has two.
|
||||
`compile.lua`'s `slots` is **not** affected: keyed by a module
|
||||
constant at both ends, with `slot_for_buffer` id-based.
|
||||
- **`read_only` is one boolean serving THREE policies** (framing
|
||||
§2.11): the generated lock; terminal identity
|
||||
(`src/terminal/session.rs:305`); and, as a *reader*,
|
||||
`src/lua_bindings/fold.rs:313`'s "is this a document buffer" test,
|
||||
pinned by `tests/folding_acceptance.rs:570`. Consequence for any
|
||||
lane: **locking a buffer silently disables `pmacs.fold.fold` on it**,
|
||||
with the status `fold rejected: not a document buffer`.
|
||||
- **The SHIPPED `set_generated_contents` can overwrite a live terminal
|
||||
identity buffer.** It does `self.read_only = false` unconditionally
|
||||
(`src/buffer.rs:546`), so it lifts a lock it did not install, writes,
|
||||
and re-locks. Present on `main`, untested, unframed anywhere before
|
||||
revision 4. Refused in Stage 2 by the `identity_protected` field —
|
||||
an **intrinsic** flag marked once by a crate-private monotonic
|
||||
`mark_identity_protected()` in `TerminalSession::open`, never written
|
||||
by `set_read_only`. Revision 3 tried to infer this from the lock's
|
||||
provenance instead; that broke the lift-and-restore idiom at
|
||||
`tests/terminal_copy_mode_acceptance.rs:578-584`, and the general
|
||||
lesson is that a **derived** fact must be maintained by every
|
||||
mutation of what it derives from — and `set_read_only` is `pub`.
|
||||
- **`acc16e` is `crdt`-gated and is the only shipped consumer of the
|
||||
lift-and-restore idiom.** `cargo test --test
|
||||
terminal_copy_mode_acceptance` **without** `--features crdt` never
|
||||
compiles it, so a green run of that suite proves nothing about the
|
||||
seam. Any lane touching `read_only` semantics must run it with the
|
||||
feature and confirm `acc16e` is in the count.
|
||||
- **`identity_protected` is not generated-lock provenance.** Revision
|
||||
4 tried to use “not a terminal identity buffer” as proof that the
|
||||
generated primitive installed the lock; it is not. Revision 5
|
||||
therefore removes `pmacs.buffer.unlock_generated` from the arc
|
||||
entirely. Wdired's future generated→editable transition remains
|
||||
dired Stage 3 work and must be owner-specific or use the eventual
|
||||
lock-policy enum.
|
||||
- **The CRDT `Replace` mid-transaction divergence is real and
|
||||
unowned.** `crdt.delete` then `crdt.insert` (`src/buffer.rs:1140-1163`);
|
||||
if the first succeeds and the second fails, the code's own comment
|
||||
says "the CRDT is mid-transaction ... This is an invariant
|
||||
violation." It reaches `apply_edit` and `apply_edit_skip_intercepts`
|
||||
today and is reported as an ordinary `CrdtRejected`, so nothing
|
||||
distinguishes it. This lane names and contains it; **repair is
|
||||
deferred and unowned.** Revision 6 makes classification total with a
|
||||
`crdt_mutated` flag: any later error, including
|
||||
`export_updates_since` after `Insert`, `Delete` or `Replace`, is
|
||||
`Diverged`. Revision 7 installs a common monotonic poison before
|
||||
either generated or ordinary API mapping, blocks
|
||||
`initial_target_snapshot`, `send_buffer_snapshots`,
|
||||
`export_buffer_snapshot` and `queue_daemon_origin_crdt_op`, and
|
||||
rejects every later owner-generated write with `CrdtQuarantined`.
|
||||
Criteria 16c and 16d fault-inject the complete boundary under
|
||||
`cargo test --lib --features crdt`; there is no public
|
||||
fault-injection API and no four-variant fallback.
|
||||
- **Overlap warning.** Stage 2 touches `src/lua_bindings/mod.rs`'s buffer
|
||||
mutator bindings and `src/buffer.rs`. Do not run it concurrently with
|
||||
the `apply_resource_op` lane or the bottom-panel 2B work without
|
||||
assigning those files to one lane first. The framing itself touches
|
||||
neither.
|
||||
- **Cross-lane, settled, not re-decided here.** #186 owns the urgent
|
||||
pre-filesystem refusal for synchronous `apply_resource_op`; #171 later
|
||||
owns full post-delete lifecycle reconciliation, including the async
|
||||
race where a buffer becomes modified after dired dispatch. **#171's
|
||||
Q#DR25 is deferred INTO this lane** — confirmed against #171 revision 7
|
||||
(`fd7ae37`), which states that dired's listing becoming immutable is
|
||||
"owned by the `generated-buffer-immutability` lane" and that "Stage 2
|
||||
does not implement it, does not gate on it, and carries no acceptance
|
||||
for it." This lane's Stage 1 claims that work. **Neither ordering
|
||||
conflicts**: #171 Stage 2b changes `paint`'s callers, this lane changes
|
||||
`paint` itself. Revisions 1 and 2 of this framing never mentioned
|
||||
Q#DR25 at all; revision 3 §9b records it.
|
||||
- **Re-measured at `ad41cf1` while scouting: 276 CRDT-dark tests**
|
||||
(3,251 vs 3,527), by
|
||||
`cargo test --all-targets --no-default-features --features lua54[,crdt] -- --list | grep -c ': test$'`.
|
||||
Recorded here because the section above asks for exactly that and
|
||||
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 2 — silent-skip arming
|
||||
|
||||
- Portable branch: `githubsucks/silent-skip-arming`, worktree
|
||||
`../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
|
||||
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.
|
||||
- **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.
|
||||
- **Not this lane's to fix, recorded so it is not mistaken for
|
||||
oversight:** the generated-buffer immutability lane above still reads
|
||||
"PR #188 OPEN, PROPOSED" and #188 has merged. Rule 4 forbids
|
||||
relabelling it and permits removal only once its durable facts reach
|
||||
`docs/agent-handoff.md`, which #188 did not touch — it changed the
|
||||
framing and this ledger only. So the absorption is genuinely owed,
|
||||
and the natural carrier is the arc's own next PR (#191, Stage 1),
|
||||
not a testing lane reaching across into someone else's arc.
|
||||
- **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.
|
||||
- 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`
|
||||
|
|
@ -677,32 +975,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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load Diff
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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,8 +461,10 @@ 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 or PMACS_TEST_LUAJIT",
|
||||
);
|
||||
return;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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,8 +97,10 @@ fn locate_lua() -> Option<PathBuf> {
|
|||
}
|
||||
}
|
||||
}
|
||||
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 or PMACS_TEST_LUAJIT",
|
||||
);
|
||||
None
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,104 @@
|
|||
//! 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.
|
||||
//!
|
||||
//! **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.
|
||||
|
||||
#![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)"
|
||||
);
|
||||
}
|
||||
Loading…
Reference in New Issue