From 61b1062c5fae77349a860624f068dab13f3c6429 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 13:13:34 +0200 Subject: [PATCH 01/14] =?UTF-8?q?docs:=20frame=20the=20tree=20primitive=20?= =?UTF-8?q?=E2=80=94=20anchored=20on=20two=20real=20consumers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit COHERENCE.md §14 grades Tree as the last missing workbench primitive, and §20 Priority 5 names it as what remains after the bottom panel. Its argument is to build it once "before dired's directory view and the workers tree harden their own conventions". THIS FRAMING NARROWS THAT ARGUMENT DELIBERATELY. §14 lists six future consumers, and designing a shared primitive against six hypothetical ones is how you get a model that fits none. The scout found a better basis: one consumer already ships a tree and fakes it, and a second is already scoped and deliberately deferred. THE HIERARCHY ALREADY EXISTS AND IS ALREADY DISCARDED. `Symbol:: push_hier` walks a genuine LSP DocumentSymbol tree — it recurses on `children` — and flattens it, preserving `depth`, a `::`-joined parent chain, and document order with parents before children. `lsp.lua` then re-renders that depth as LEADING SPACES INSIDE THE ROW TEXT, under its own comment "FLAT with a `depth` field --- indent, don't recurse". So the outline has no collapse, no expand, no parent/child navigation, and every input a tree needs is already computed. It is the anchor consumer because it needs no new plumbing and its limitation is observable today rather than hypothetical. Dired is the second: it landed a flat listing for Emacs parity and deferred `i` insert-subdirectory in its own §13 — the restraint §14 credits, and what keeps the door open. The workers view is NOT a consumer yet: it is a Rust-generated text buffer raw-switched into the active window, with no rows. REFRESH RESTORES A LINE, NOT A NODE, and that is the crux rather than a detail. `listview.refresh` saves `cursor_line()`, rebuilds rows wholesale from a freshly produced array, and re-seats by walking `move_down`. Today that is a mild wrong-restore. Collapse breaks it outright, because expanding a node inserts rows ABOVE the cursor — and collapse state itself must survive refresh, which requires recognising "the same node" across two independently produced arrays. Neither `line_to_item` nor an opaque `item` can do that. This is why the stable-identity question decides whether selection and expansion survive a model update at all. Four questions are left genuinely open: extend listview versus a separate treeview (no leaning recorded — the scout found nothing that decides it); who owns collapse state; what a stable node identity is; and whether the row still carries pre-rendered text. On identity the scout did establish constraints: listview cannot derive one because `item` is opaque; the outline's parent chain plus name is nearly sufficient but collides on overloads; dired's path would be genuinely stable. So identity is almost certainly consumer-supplied, which makes it part of the public contract rather than an internal detail. NO INTERACTION ISLAND unless evidence forces one. Expand/collapse are buffer-local bindings on a generated buffer, exactly as RET/n/p/g/q already are. §6 grades islands "weak, and growing"; this must not add to that count, and if some behaviour cannot be expressed that way it is a finding to report rather than a licence. §1.6 states plainly what this document is: unlike the last two lanes there is no fallout to census and no baseline to diff, so it ARGUES a model rather than measuring one — the shape that has historically needed the most review rounds here. Framing only. Acceptance is explicitly not final pending the open questions. Co-Authored-By: Claude Opus 5 (1M context) --- docs/tree-primitive-framing.md | 298 +++++++++++++++++++++++++++++++++ 1 file changed, 298 insertions(+) create mode 100644 docs/tree-primitive-framing.md diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md new file mode 100644 index 0000000..b174c06 --- /dev/null +++ b/docs/tree-primitive-framing.md @@ -0,0 +1,298 @@ +# Framing — the tree primitive + +**Revision 1.** Status: framing only. No implementation. Scouted against +`githubsucks/main` @ `12f2970`. + +`COHERENCE.md` §14 grades **Tree ✗ — none**, and it is the last missing +workbench primitive. §20's Priority 5 names it as what remains after the +bottom panel, and its argument is specific: *"building it once before +dired's directory view and the workers tree harden their own conventions +is exactly this section's point."* + +**This document deliberately narrows that argument.** §14 lists six +future consumers — project files, symbol hierarchy, package dependency +graph, worker trees, git status — and designing a shared primitive +against six hypothetical consumers is how you get a model that fits +none. The scout found something better: **one consumer already ships a +tree and fakes it**, and a second is already scoped and deliberately +deferred. Those two are the design's evidence base. + +--- + +## 0. Coherence impact (COHERENCE §20) + +- **Concern: §14 Coherent Workbench Primitives.** Tree is the one + remaining ✗ in its inventory. This closes it for the two consumers + that exist and gives the rest an adoption path. +- **Journey steps touched:** none directly. Step 6 (LSP) gains a real + hierarchy view where it currently has indented text. +- **Interaction islands (§6): NONE, unless evidence forces one.** The + default is the ordinary **buffer-local keymap** idiom that listview, + compile, dired and terminal already use — a tree's expand/collapse + keys are buffer-local bindings on a generated buffer, not a new + dispatch shadow. §6 grades islands "weak, and growing"; this stage + must not add to that count. If some behaviour genuinely cannot be + expressed as a buffer-local binding, that is a finding to report, not + a licence to add an island. +- **Config registry adoption:** none proposed. If a preference emerges + (initial expansion depth, say), it enters the registry rather than + becoming a hardcoded constant — but nothing yet requires one. +- **Background-work attribution:** none. +- **Enables:** DAP's variables view, which is inherently a tree + (scopes → objects → fields) and would otherwise become the **third** + bespoke implementation. + +--- + +## 1. Ground truth (measured at `12f2970`) + +### 1.1 The LSP outline is a shipped tree consumer, faking it + +**The hierarchy already exists and is already discarded.** + +`Symbol::push_hier` (`src/symbol.rs:110`) walks a genuine LSP +`DocumentSymbol` tree — it recurses on `item.get("children")` — and +**flattens it** into `Vec`, preserving: + +- `depth: u32` — "Nesting depth in a hierarchical `DocumentSymbol` tree"; +- `parent` — `containerName` for flat shapes, or the parent chain joined + with `::` for hierarchical ones; +- document order, **parents before children** (`SymbolResponse.symbols`). + +`lsp.lua` then re-renders that depth as *leading spaces inside the row +text*: + +```lua +-- Arc 1b phase 2: a browsable *outline* panel. Symbols arrive +-- FLAT with a `depth` field --- indent, don't recurse. +text = string.format("%s%s [%s]", string.rep(" ", sym.depth or 0), sym.name, tag), +``` + +So the outline has **no collapse, no expand, no parent/child +navigation** — indentation is a string. Every input a tree needs is +already computed; only the view throws it away. + +**This is the anchor consumer.** It needs no new data plumbing, and its +limitation is observable today rather than hypothetical. + +### 1.2 dired declined to invent one, and its case is already scoped + +Dired Stage 1 landed a **flat** listing for Emacs parity and deferred +the recursive case explicitly: `docs/dired-framing.md` §13 names +**`i` insert-subdirectory (in-buffer recursive listing)** as deferred. + +§14 credits this directly: dired "landed **without** inventing one". That +restraint is what keeps the door open — and it means the second consumer +arrives with real requirements rather than a wishlist: a fixed-width +column contract (`pmacs.dired._layout`), a frozen test fixture, and +path-keyed entries. + +### 1.3 The workers view is NOT a consumer yet + +`editor.list-workers` is +`pmacs.window.switch_buffer(pmacs.workers.show())` — a **Rust-generated +text buffer, raw-switched** into the active window. It is not a listview +and has no rows. §14's "worker trees" is a future consumer, not a +current one. + +*(Incidental, and worth a separate look: that raw switch is the same +`switch_buffer` pattern that broke the outline under bottom-panel Stage +3. It is harmless while `*workers*` is not a panel, and inherits the +hazard the moment it becomes one.)* + +### 1.4 What listview's model would have to gain + +listview's contract today: + +```lua +pmacs.listview.open { + name = "*references*", + header = "12 references RET visit n/p move g refresh q quit", + rows = { { text = "src/foo.rs:12:4", item = }, ... }, + on_visit = function(item) ... end, + on_refresh = function() return rows end, +} +``` + +and `render` is: + +```lua +local lines = { p.header } +p.line_to_item = {} +for _, row in ipairs(rows) do + lines[#lines + 1] = row.text + p.line_to_item[#lines - 1] = row.item +end +pmacs.buffer.set_generated_contents(p.buffer, table.concat(lines, "\n")) +``` + +**A flat array plus a line→item map.** Depth appears nowhere; it is +baked into `row.text` before listview ever sees it. `item` is `` — +**opaque to listview by design**, which matters for §2's identity +question. + +### 1.5 Refresh restores a LINE, not a node — and that is the crux + +```lua +local saved = pmacs.editor.cursor_line() +local rows = p.on_refresh() or {} +render(p, rows) +... +seat_cursor(p, saved) +``` + +`refresh` saves a **line number**, rebuilds the rows wholesale from a +freshly-produced array, and re-seats by walking `move_down` that many +times. + +Today this is a mild wrong-restore: if the new list has a different +shape, the cursor lands on whatever row now occupies that line. For a +flat list of roughly stable shape, tolerable. + +**Collapse breaks it outright.** Expanding a node inserts rows *above* +the cursor, so a line-keyed restore lands somewhere unrelated. And +collapse state itself must survive refresh, which requires recognising +"the same node" across two independently-produced arrays — something +neither `line_to_item` nor an opaque `item` can do. + +**This is why Q#TR3 exists and why it is not a detail.** It decides +whether selection and expansion can survive a model update at all. + +### 1.6 What is NOT established + +- **Nothing is implemented or measured.** Unlike the last two lanes + there is no fallout to census: §14 grades Tree ✗, so there is no + existing behaviour to preserve and no baseline to diff. **This framing + argues a model rather than measuring one**, which is the shape that + has historically needed the most review rounds here (Lean Stage 3b + took six; the signal lane had three tolerance rules rejected in a + row). Treat its claims as proposals. +- **No consumer has asked for collapse.** The outline's limitation is + inferred from its structure and its own "indent, don't recurse" + comment, not from a user report. +- **dired's `i` has not been re-scouted** against current `main`; §13's + deferral is the only evidence that its requirements are as described. + +--- + +## 2. Questions + +All four are genuinely open. The first two the review already flagged as +open; the third is the one review added; the fourth follows from §1.4. + +- **Q#TR1 — extend `listview`, or add a separate `treeview`?** + Extending touches three shipped call sites and the `line_to_item` + contract, and risks making a working flat primitive worse for the two + consumers that do not need depth. A separate primitive avoids that but + is exactly the "second primitive" §14 warns about, and would duplicate + panel plumbing, `q`/`g` bindings, the Q#GB18 identity rule and the + generated-buffer write invariant. **No leaning recorded** — the scout + did not find evidence that decides it. +- **Q#TR2 — who owns collapse state?** Candidates: the primitive (keyed + by node id), the consumer (passed in with the rows each render), or + the buffer (as generated-buffer state). Consumer-owned keeps the + primitive stateless and makes refresh the consumer's problem; + primitive-owned centralises it and forces Q#TR3 to be answered first. +- **Q#TR3 — what is a stable node identity across refresh?** *(Added at + review.)* This determines whether **selection and expansion survive a + model update** (§1.5). Constraints the scout established: + - listview cannot derive one: `item` is `` and opaque. + - The outline's data is nearly sufficient — the `::`-joined parent + chain plus name — but **not unique**: overloads and same-named + siblings collide. + - dired's would be genuinely stable: the path. + - So identity is almost certainly **consumer-supplied**, which makes + it part of the primitive's public contract rather than an internal + detail. That is a real API commitment and should be decided + deliberately, not defaulted into. +- **Q#TR4 — does the row still carry pre-rendered `text`?** Today the + consumer formats indentation into the string. If the primitive owns + depth it should probably own indentation too — but the outline also + appends `[kind]` tags and dired has a fixed-width column contract, so + "the primitive renders the row" may not survive contact with either. + +--- + +## 3. Bets + +- **Bet 1 — the outline is a sufficient first consumer on its own.** Its + data already carries depth and parent; adopting it requires no LSP-side + change. *Falsified if adoption needs `Symbol` to change shape.* +- **Bet 2 — identity must be consumer-supplied** (§1.5, Q#TR3). + *Falsified if some derivable key proves both stable and unique across + the two consumers.* +- **Bet 3 — no interaction island is required.** Expand/collapse are + buffer-local bindings on a generated buffer, exactly as `RET`/`n`/`p`/ + `g`/`q` already are. *Falsified if some behaviour cannot be expressed + that way — which would be a finding worth reporting, not a licence.* + +--- + +## 4. Acceptance + +**Not final** — this framing argues a model, and the criteria cannot be +fixed until Q#TR1–TR3 are decided. The shapes they will take: + +1. The **LSP outline** renders through the primitive with **no + `string.rep` indentation in `lsp.lua`**, and `Symbol` is unchanged. +2. **Collapse and expand** work, and **collapse state survives + `g` refresh** — the criterion that Q#TR3 exists to make possible. +3. **Selection survives refresh by node, not by line** (§1.5). +4. **No new interaction island**: every tree key is a buffer-local + binding, and the dispatch-shadow count is unchanged. Asserted, not + assumed. +5. The **flat consumers are unaffected** — references, buffer-list and + the search panel render byte-identically, pinned by their existing + suites rather than by inspection. +6. **Q#GB18 identity holds**: a foreign buffer with the panel's name is + never adopted, and the primitive is keyed by handle, not name. +7. The **generated-buffer write invariant** is preserved + (`set_generated_contents`), including the rope lock and history rules + the listview suite already pins. + +--- + +## 5. Parked + +- **The other four §14 consumers** — project files, package dependency + graph, worker trees, git status. They adopt later; designing for them + now is the failure this framing avoids. +- **DAP's variables view.** The reason the primitive is worth building + before the debugger, and not part of it. +- **dired's `i` insert-subdirectory.** The second consumer, and the + right forcing function for the design — but it is its own stage with + its own framing, and dired Stage 2b/3 are ahead of it. +- **Making `*workers*` a listview/tree consumer** (§1.3), and the + raw-switch hazard noted there. +- **Tree rendering in the GPU frontend** beyond whatever the shared + generated-buffer path already gives. + +--- + +## 6. Gates + +The standing `CLAUDE.md` suite. The suites most likely to move are +`listview_acceptance` and `m4_acceptance` (the LSP outline and hover +panels are listview consumers — bottom-panel Stage 3 established that +transitive relationship the hard way). + +**Sweep both feature configurations.** Stage 3 shipped a broken +crdt-gated suite because every local sweep ran `--features luajit` +without `crdt`; and `--no-fail-fast` is required, or a multi-suite break +reports as one suite. + +--- + +## 7. Branch plan + +Not settled, because it depends on Q#TR1. Two shapes: + +- **If listview is extended:** one branch, with the flat-consumer + no-change proof (acceptance 5) landing *before* the outline adopts, so + a regression in references or buffer-list is attributable. +- **If a separate `treeview`:** the primitive and its first consumer are + separable, and the outline's adoption can be its own PR. + +Either way the outline adopts **before** dired's `i` is attempted: it is +the consumer whose data already fits, and it is the one that proves the +model without also needing a new listing mode. From cf4ac1c5ef963edd887f1904f52cbc680aa78ce2 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 13:22:24 +0200 Subject: [PATCH 02/14] =?UTF-8?q?docs(tree):=20revision=202=20=E2=80=94=20?= =?UTF-8?q?five=20review=20corrections,=20all=20verified=20in=20source?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REFRESH WAS UNSCOPED FOR THE ANCHOR CONSUMER, and this is the one that would have wasted implementation time. Two acceptance criteria rested on `g` refresh preserving collapse and selection. The outline's header offers "RET visit n/p move q quit" — NO `g` — it supplies no `on_refresh`, and `listview.refresh` opens `if not (p and p.on_refresh) then return end`. The criteria were unreachable for the only consumer that exists. Refresh is now out of scope, with the question it actually raises stated rather than hidden: an outline refresh means re-requesting textDocument/documentSymbol, which is an async round-trip with its own await, failure and staleness handling, and it raises who owns the result when it arrives against a buffer the user may have edited or left. That is LSP request-lifecycle work; bundling it here would make the tree lane responsible for it. Acceptance is re-scoped to what the primitive controls — collapse and selection surviving a RE-RENDER — and the refresh follow-on is parked with its precondition. ACCEPTANCE 1 DECIDED Q#TR4 WHILE CALLING IT OPEN. "No `string.rep` indentation in lsp.lua" commits to primitive-owned indentation, which is exactly the question Q#TR4 leaves unresolved. The criterion is representation-neutral now: the outline renders its hierarchy THROUGH the primitive rather than by pre-formatting it, and whether the primitive emits the indentation or the consumer still supplies a string alongside structural depth stays open. Q#TR1 MISREAD §14, and the correction changes the tradeoff rather than softening it. Revision 1 said a separate treeview would be "exactly the second primitive §14 warns about". §14 EXPLICITLY LISTS A TREE in the reusable set it wants, alongside virtual list. What it warns against is bespoke per-consumer plumbing — each subsystem inventing its own UI vocabulary. A treeview sharing the existing buffer/panel disciplines is not that; a tree hand-rolled inside lsp.lua would be. The real tradeoff is narrower and is recorded without a leaning, because the scout still found nothing that decides it. THE REGRESSION CRITERION NAMED THE WRONG CONSUMERS, and worse, named the exact ones §14 exists to correct. `*buffer-list*` and project search do NOT use listview; §14 measured three call sites, all in lsp.lua — `*references*`, `*outline*`, `*lsp-help*` — and calls the older claim a documentation error. Repeating it would have re-introduced a mistake that document was written to fix. The criterion now protects the actual siblings, and says what to do if the broader surfaces are ever in scope. CONSUMER ACCOUNTING TIGHTENED. Five named future consumers, not six. And the split that matters: ONE existing anchor consumer (the outline) plus ONE future constraint source (dired's deferred `i`) — dired constrains the design but cannot validate it, because nothing has been built against it. Calling them "two consumers that exist" overstated the evidence by exactly one. "Every input a tree needs" is qualified to "every input a tree needs to RENDER", since stable node identity is precisely what no existing field supplies. Framing only, still unapproved. Co-Authored-By: Claude Opus 5 (1M context) --- docs/tree-primitive-framing.md | 160 +++++++++++++++++++++++++++------ 1 file changed, 132 insertions(+), 28 deletions(-) diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index b174c06..1640191 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -1,7 +1,29 @@ # Framing — the tree primitive -**Revision 1.** Status: framing only. No implementation. Scouted against -`githubsucks/main` @ `12f2970`. +**Revision 2.** Status: framing only, **not yet approved**. No +implementation. Scouted against `githubsucks/main` @ `12f2970`. + +**Revision 1 → 2**, all from review, all verified against source before +applying: + +- **Refresh was unscoped for the anchor consumer.** Two criteria rested + on `g` refresh; the outline has no `g`, no `on_refresh`, and + `listview.refresh` no-ops without one. Refresh is now **out of scope**, + with the LSP re-request question it actually raises stated (§1.5a). +- **Acceptance 1 decided Q#TR4 while calling it open** — "no + `string.rep` in `lsp.lua`" commits to primitive-owned indentation. It + is representation-neutral now. +- **Q#TR1 misread §14.** A tree is not the "second primitive" §14 warns + against; §14 **explicitly lists one**. The warning is against bespoke + per-consumer plumbing. Retradeoffed without prejudging. +- **The regression criterion named the wrong consumers.** + `*buffer-list*` and project search do **not** use listview — §14 says + so and calls the older claim an error. The real siblings are + `*references*` and `*lsp-help*`. +- **Consumer accounting tightened**: five named future consumers, not + six; **one** existing anchor plus **one** future constraint source, + not "two that exist"; and "every input a tree needs" qualified, since + stable identity is exactly what is missing. `COHERENCE.md` §14 grades **Tree ✗ — none**, and it is the last missing workbench primitive. §20's Priority 5 names it as what remains after the @@ -9,21 +31,30 @@ bottom panel, and its argument is specific: *"building it once before dired's directory view and the workers tree harden their own conventions is exactly this section's point."* -**This document deliberately narrows that argument.** §14 lists six +**This document deliberately narrows that argument.** §14 names five future consumers — project files, symbol hierarchy, package dependency graph, worker trees, git status — and designing a shared primitive -against six hypothetical consumers is how you get a model that fits -none. The scout found something better: **one consumer already ships a -tree and fakes it**, and a second is already scoped and deliberately -deferred. Those two are the design's evidence base. +against five hypothetical ones is how you get a model that fits none. + +The scout found a narrower and firmer basis, and the distinction between +its two halves matters: + +- **One EXISTING anchor consumer.** The LSP outline already ships a + tree and fakes it (§1.1). It is the only consumer that exists today. +- **One FUTURE constraint source.** dired's `i` insert-subdirectory is + scoped and deliberately deferred (§1.2). It constrains the design; it + does not validate it, because nothing has been built against it. + +Calling these "two consumers that exist" would overstate the evidence by +exactly one. --- ## 0. Coherence impact (COHERENCE §20) - **Concern: §14 Coherent Workbench Primitives.** Tree is the one - remaining ✗ in its inventory. This closes it for the two consumers - that exist and gives the rest an adoption path. + remaining ✗ in its inventory. This closes it for the **one consumer + that exists** (the LSP outline) and gives the rest an adoption path. - **Journey steps touched:** none directly. Step 6 (LSP) gains a real hierarchy view where it currently has indented text. - **Interaction islands (§6): NONE, unless evidence forces one.** The @@ -69,8 +100,12 @@ text = string.format("%s%s [%s]", string.rep(" ", sym.depth or 0), sym.name, t ``` So the outline has **no collapse, no expand, no parent/child -navigation** — indentation is a string. Every input a tree needs is -already computed; only the view throws it away. +navigation** — indentation is a string. + +**Every input a tree needs to RENDER is already computed** — depth, +parent, and order — and only the view throws it away. That is not the +same as every input a tree needs: **stable node identity is missing** +(§1.5, Q#TR3), and it is the one input no existing field supplies. **This is the anchor consumer.** It needs no new data plumbing, and its limitation is observable today rather than hypothetical. @@ -82,8 +117,8 @@ the recursive case explicitly: `docs/dired-framing.md` §13 names **`i` insert-subdirectory (in-buffer recursive listing)** as deferred. §14 credits this directly: dired "landed **without** inventing one". That -restraint is what keeps the door open — and it means the second consumer -arrives with real requirements rather than a wishlist: a fixed-width +restraint is what keeps the door open — and it means this constraint +source carries real requirements rather than a wishlist: a fixed-width column contract (`pmacs.dired._layout`), a frozen test fixture, and path-keyed entries. @@ -158,6 +193,34 @@ neither `line_to_item` nor an opaque `item` can do. **This is why Q#TR3 exists and why it is not a detail.** It decides whether selection and expansion can survive a model update at all. +### 1.5a …but the ANCHOR CONSUMER HAS NO REFRESH AT ALL + +Revision 1 built two acceptance criteria on `g` refresh without checking +that the outline supports it. **It does not:** + +- its header offers `RET visit n/p move q quit` — **no `g`** + (`lsp.lua:2490`); +- it supplies **no `on_refresh`**; +- and `listview.refresh` opens `if not (p and p.on_refresh) then return + end` — **a no-op** for this panel (`listview.lua:262`). + +So "collapse state survives `g`" was unreachable for the only consumer +that exists. **Refresh is therefore out of scope for this stage** unless +someone first answers a question this framing does not: an outline +refresh means **re-requesting `textDocument/documentSymbol`**, which is +an async LSP round-trip with its own await, failure and staleness +handling — and it raises who owns the resulting state when the response +arrives against a buffer the user may have edited or left. + +That is a real feature (`*references*` has `g` and an `on_refresh`; the +outline never gained one), and it is **not** a tree concern. Bundling it +here would make the tree lane responsible for LSP request lifecycle. + +**Consequence for acceptance:** the criteria are re-scoped to what the +anchor consumer can actually exercise — collapse and selection surviving +**re-render**, which the primitive controls — and refresh-survival is +recorded as a follow-on for whoever gives the outline a refresh. + ### 1.6 What is NOT established - **Nothing is implemented or measured.** Unlike the last two lanes @@ -181,13 +244,31 @@ All four are genuinely open. The first two the review already flagged as open; the third is the one review added; the fourth follows from §1.4. - **Q#TR1 — extend `listview`, or add a separate `treeview`?** - Extending touches three shipped call sites and the `line_to_item` - contract, and risks making a working flat primitive worse for the two - consumers that do not need depth. A separate primitive avoids that but - is exactly the "second primitive" §14 warns about, and would duplicate - panel plumbing, `q`/`g` bindings, the Q#GB18 identity rule and the - generated-buffer write invariant. **No leaning recorded** — the scout - did not find evidence that decides it. + + **Revision 1 framed this wrongly and the correction changes the + tradeoff.** It claimed a separate treeview would be "exactly the + second primitive §14 warns about". §14 does not warn against a tree — + **it explicitly lists one**, alongside virtual list, in the reusable + set it wants: *"editable text view, virtual list, **tree**, structured + table, inspector…"* (`COHERENCE.md:1264`). A tree surface is a named + goal, not a violation. + + What §14 actually warns against is **bespoke per-consumer plumbing** — + each subsystem inventing its own UI vocabulary. A `treeview` that + shares the existing buffer/panel disciplines (generated-buffer writes, + Q#GB18 handle identity, panel placement, `q` quit-action) is not that; + a tree hand-rolled inside `lsp.lua` would be. + + So the real tradeoff is narrower: + - **Extending listview** touches three shipped call sites and the + `line_to_item` contract, and risks making a working flat primitive + worse for the consumers that do not need depth. + - **A separate treeview** keeps the flat primitive untouched, but must + *share* rather than *duplicate* the panel disciplines — and "shares + them" is an implementation claim that has to be verified, not + asserted. + + **No leaning recorded**; the scout found nothing that decides it. - **Q#TR2 — who owns collapse state?** Candidates: the primitive (keyed by node id), the consumer (passed in with the rows each render), or the buffer (as generated-buffer state). Consumer-owned keeps the @@ -233,17 +314,33 @@ open; the third is the one review added; the fourth follows from §1.4. **Not final** — this framing argues a model, and the criteria cannot be fixed until Q#TR1–TR3 are decided. The shapes they will take: -1. The **LSP outline** renders through the primitive with **no - `string.rep` indentation in `lsp.lua`**, and `Symbol` is unchanged. -2. **Collapse and expand** work, and **collapse state survives - `g` refresh** — the criterion that Q#TR3 exists to make possible. -3. **Selection survives refresh by node, not by line** (§1.5). +1. The **LSP outline renders its hierarchy through the primitive** + rather than by pre-formatting it into row text, and `Symbol` is + unchanged. **Representation-neutral on purpose:** whether the + primitive emits the indentation, or the consumer still supplies a + rendered string alongside structural depth, is **Q#TR4** and is not + decided here. Revision 1's wording ("no `string.rep` in `lsp.lua`") + committed to primitive-owned indentation while calling that question + open. +2. **Collapse and expand work**, and **collapse state survives a + re-render** — the primitive re-emitting the buffer from the same + model. *(Not "survives `g` refresh": the anchor consumer has no + refresh at all — §1.5a.)* +3. **Selection survives a re-render by node, not by line** (§1.5). 4. **No new interaction island**: every tree key is a buffer-local binding, and the dispatch-shadow count is unchanged. Asserted, not assumed. -5. The **flat consumers are unaffected** — references, buffer-list and - the search panel render byte-identically, pinned by their existing - suites rather than by inspection. +5. **listview's OTHER consumers are unaffected** — `*references*` and + `*lsp-help*`, which with `*outline*` are the **only three + `pmacs.listview.open` call sites, all in `lsp.lua`**. They render + byte-identically, pinned by their existing suites. + + **Revision 1 named `*buffer-list*` and project search here and was + wrong** — §14 measured that they do **not** use listview and calls + the earlier claim a documentation error (`COHERENCE.md:1286`). + Repeating it would have re-introduced a mistake that document exists + to correct. If the broader surfaces are ever in scope, their + independent render paths and tests must be named explicitly. 6. **Q#GB18 identity holds**: a foreign buffer with the panel's name is never adopted, and the primitive is keyed by handle, not name. 7. The **generated-buffer write invariant** is preserved @@ -266,6 +363,13 @@ fixed until Q#TR1–TR3 are decided. The shapes they will take: raw-switch hazard noted there. - **Tree rendering in the GPU frontend** beyond whatever the shared generated-buffer path already gives. +- **Giving the outline a refresh** (§1.5a). `*references*` has `g` and + an `on_refresh`; the outline never gained one. Adding it means + re-requesting `textDocument/documentSymbol` with its own await, + failure and staleness handling, and deciding who owns the result when + it arrives against a buffer the user may have edited. **That is LSP + request-lifecycle work, not tree work** — and only once it exists can + "collapse survives refresh" be a criterion rather than an aspiration. --- From 932b3ab179daeb55d4eb698c6b7cd65263a21a7d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 13:34:22 +0200 Subject: [PATCH 03/14] =?UTF-8?q?docs(tree):=20revision=203=20=E2=80=94=20?= =?UTF-8?q?an=20example=20is=20not=20a=20consumer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, all verified in source before applying, plus one found while verifying them. `*references*` HAS NO `g` AND NO `on_refresh`. Revision 2 asserted it twice. Its header is "RET visit n/p move q quit" and it supplies only `on_visit` (lsp.lua:2442). The consumer with refresh is `*lsp*` (lsp.status, lsp.lua:3004). The error is worth naming precisely because it will recur otherwise: I read `listview.lua`'s MODULE-DOCSTRING EXAMPLE, which illustrates the API using `name = "*references*"` and a header containing `g refresh`, and treated it as the real consumer. An example written to show the shape of an API is not evidence about any caller of it. The refresh-scoping conclusion is unaffected — it rested on the OUTLINE lacking refresh, which holds. THE BRANCH PLAN STILL NAMED buffer-list. Acceptance 5 had already been corrected for exactly that error in the previous round; the same claim survived one section further down. Fixing a mistake where it was reported is not the same as fixing it where it occurs. "BYTE-IDENTICALLY, PINNED BY THEIR EXISTING SUITES" WAS UNSUPPORTED. `listview_acceptance` says in its own header that the references panel needs a live LSP and is validated manually or via the m4 harness — it does not exercise `*references*` at all — and the m4 hover test asserts content PRESENCE, not byte-exact output. So the criterion claimed coverage that does not exist. It is now posed as a decision rather than patched: either byte-identity becomes a new test this stage writes (needing the fake-LSP harness for references), or the claim weakens to the substrate behaviours actually pinned. Leaning recorded toward writing the test, because a flat consumer silently gaining an indent column is precisely what this criterion exists to catch and content-presence would not see it. FOUND WHILE VERIFYING: §14's "exactly three `pmacs.listview.open` call sites" is STALE. There are four. `*lsp*` arrived with Journey Stage 1b-2 (#204), after §14's audit at ad41cf1, and it is the ONLY listview consumer with refresh — which is why §1.5a's conclusion holds rather than being luck. §14's line numbers have drifted too. Recorded in a new §1.3a so the next reader does not inherit "three". Framing only, still unapproved. Co-Authored-By: Claude Opus 5 (1M context) --- docs/tree-primitive-framing.md | 101 +++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 11 deletions(-) diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 1640191..785c646 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -1,8 +1,32 @@ # Framing — the tree primitive -**Revision 2.** Status: framing only, **not yet approved**. No +**Revision 3.** Status: framing only, **not yet approved**. No implementation. Scouted against `githubsucks/main` @ `12f2970`. +**Revision 2 → 3**, three further review findings, all verified in +source: + +- **`*references*` has no `g` and no `on_refresh`** — revision 2 claimed + it twice. The consumer with refresh is **`*lsp*`** (§1.5a). The error + came from reading `listview.lua`'s **module-docstring example**, which + illustrates the API using `*references*` with `g refresh` in the + header. An example is not a consumer. *The refresh-scoping conclusion + is unaffected — it depended on the OUTLINE lacking refresh, which + holds.* +- **The branch plan still said "references or buffer-list"** — the same + `*buffer-list*` error acceptance 5 had already been corrected for, + surviving one section further down. +- **"Byte-identically, pinned by their existing suites" was + unsupported.** `listview_acceptance` states in its own header that + `*references*` needs a live LSP and is not exercised there, and the m4 + hover test asserts content *presence*, not byte-exact output. + Acceptance 5 now poses that as a decision — write the byte-identity + test, or weaken the claim — with a leaning and its cost. + +**Found while verifying those: §14's "exactly three call sites" is +stale — there are FOUR** (§1.3a), and the fourth (`*lsp*`, added by +#204) is the only one with refresh. + **Revision 1 → 2**, all from review, all verified against source before applying: @@ -135,6 +159,28 @@ current one. 3. It is harmless while `*workers*` is not a panel, and inherits the hazard the moment it becomes one.)* +### 1.3a There are FOUR listview consumers now, not three + +§14 measured "exactly **three** `pmacs.listview.open` call sites, all +three in `builtin/runtime/lsp.lua`" at `ad41cf1`. **There are four**, and +the fourth matters here: + +| call site | panel | `on_visit` | `on_refresh` / `g` | +|---|---|---|---| +| `lsp.lua:2442` | `*references*` | yes | **no** | +| `lsp.lua:2488` | `*outline*` | yes | **no** | +| `lsp.lua:2924` | `*lsp-help*` | no | **no** | +| `lsp.lua:3004` | `*lsp*` (`lsp.status`) | no | **yes** | + +`*lsp*` arrived with Journey Stage 1b-2 (#204), after §14's audit. It is +**the only listview consumer with refresh at all**, which is why §1.5a's +scoping conclusion holds: refresh is a feature exactly one panel has, and +it is not the anchor. + +§14's line numbers have also drifted (`:2056`/`:2102`/`:2513` against +today's `:2442`/`:2488`/`:2924`). The count is the part that matters; +this is recorded so the next reader does not inherit "three". + ### 1.4 What listview's model would have to gain listview's contract today: @@ -212,9 +258,17 @@ an async LSP round-trip with its own await, failure and staleness handling — and it raises who owns the resulting state when the response arrives against a buffer the user may have edited or left. -That is a real feature (`*references*` has `g` and an `on_refresh`; the -outline never gained one), and it is **not** a tree concern. Bundling it -here would make the tree lane responsible for LSP request lifecycle. +That is a real feature — **`*lsp*` (`lsp.status`) has `g refresh` and an +`on_refresh`** (`lsp.lua:3004`); the outline never gained one — and it is +**not** a tree concern. Bundling it here would make the tree lane +responsible for LSP request lifecycle. + +*(Revision 2 attributed refresh to `*references*` twice. It has neither: +its header is `RET visit n/p move q quit` and it supplies only +`on_visit` (`lsp.lua:2442`). The error came from reading +`listview.lua`'s **module-docstring example**, which illustrates the API +using `name = "*references*"` and a header containing `g refresh` — an +example, not a consumer.)* **Consequence for acceptance:** the criteria are re-scoped to what the anchor consumer can actually exercise — collapse and selection surviving @@ -330,10 +384,32 @@ fixed until Q#TR1–TR3 are decided. The shapes they will take: 4. **No new interaction island**: every tree key is a buffer-local binding, and the dispatch-shadow count is unchanged. Asserted, not assumed. -5. **listview's OTHER consumers are unaffected** — `*references*` and - `*lsp-help*`, which with `*outline*` are the **only three - `pmacs.listview.open` call sites, all in `lsp.lua`**. They render - byte-identically, pinned by their existing suites. +5. **listview's OTHER consumers are unaffected** — `*references*`, + `*lsp-help*` and `*lsp*` (§1.3a: four call sites, all in `lsp.lua`). + + **What existing suites actually pin, stated honestly.** + `listview_acceptance` drives the substrate hermetically and says so + in its own header: *"The references panel itself needs a live LSP and + is validated manually / via the m4 harness"* — so **it does not + exercise `*references*` at all**. The m4 hover test asserts content + *presence*, not byte-exact output. **"Byte-identical, pinned by + existing suites" was therefore unsupported** for both panels named. + + So this criterion needs a decision, not a wording tweak: + - **either** byte-identity becomes a **new test requirement** this + stage writes — capturing each panel's rendered buffer before and + after and diffing it, which needs the m4 fake-LSP harness for + `*references*`; + - **or** the claim weakens to what is genuinely pinned today: + the substrate behaviours `listview_acceptance` covers (open, + navigate, visit, `q` restore, the read-only intercept, the + round-trip gate, refresh) plus content-presence for hover. + + **Leaning: write the byte-identity test**, because a flat consumer + silently gaining an indent column is exactly the regression this + criterion exists to catch, and content-presence would not see it. + Recorded as a leaning rather than a decision because it costs harness + work the stage has not scoped. **Revision 1 named `*buffer-list*` and project search here and was wrong** — §14 measured that they do **not** use listview and calls @@ -363,8 +439,8 @@ fixed until Q#TR1–TR3 are decided. The shapes they will take: raw-switch hazard noted there. - **Tree rendering in the GPU frontend** beyond whatever the shared generated-buffer path already gives. -- **Giving the outline a refresh** (§1.5a). `*references*` has `g` and - an `on_refresh`; the outline never gained one. Adding it means +- **Giving the outline a refresh** (§1.5a). **`*lsp*` has `g` and an + `on_refresh`**; the outline never gained one. Adding it means re-requesting `textDocument/documentSymbol` with its own await, failure and staleness handling, and deciding who owns the result when it arrives against a buffer the user may have edited. **That is LSP @@ -393,7 +469,10 @@ Not settled, because it depends on Q#TR1. Two shapes: - **If listview is extended:** one branch, with the flat-consumer no-change proof (acceptance 5) landing *before* the outline adopts, so - a regression in references or buffer-list is attributable. + a regression in `*references*`, `*lsp-help*` or `*lsp*` is + attributable. *(Revision 2 said "references or buffer-list" here — + the same `*buffer-list*` error acceptance 5 had already been corrected + for. `*buffer-list*` does not use listview.)* - **If a separate `treeview`:** the primitive and its first consumer are separable, and the outline's adoption can be its own PR. From 5186bfd67abe15ebbe60430475ef14a4d48cb9e2 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 13:53:28 +0200 Subject: [PATCH 04/14] =?UTF-8?q?docs(tree):=20revision=204=20=E2=80=94=20?= =?UTF-8?q?a=20bound=20key=20that=20does=20nothing=20is=20not=20an=20absen?= =?UTF-8?q?t=20key?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "NO `g`" WAS LITERALLY FALSE, in revisions 2 and 3 both. `bind_local_keymap` binds `g -> listview.refresh` on EVERY panel unconditionally (listview.lua:147). What three of the four consumers lack is an `on_refresh`; `listview.refresh` then returns immediately. I had been collapsing three distinct facts into one word: whether `g` is BOUND, whether refresh is ADVERTISED in the header, and whether refresh is FUNCTIONAL. The §1.3a table now separates them, because a reader checking "does the outline have g?" against the source would have found the framing wrong and had no way to tell which claim was the intended one. The consequence is worth recording on its own: THE OUTLINE HAS A DEAD REFRESH BINDING. `g` is bound, dispatched, and silently does nothing — no status, no feedback. That is a small UX wart independent of anything this framing proposes, and it is recorded rather than fixed here. COHERENCE.md §14 IS CORRECTED IN THIS BRANCH rather than deferred to implementation or split into its own lane. §25 is explicit that when a PR changes an audited claim, updating the file RIDES THAT PR — #204 added `*lsp*` and did not update the "exactly three call sites" measurement, so the correction rides the framing that found it. The ad41cf1 audit fact is retained as history rather than overwritten, with the current count of four and `*lsp*` named as the post-audit addition; §25 also says symbols are authoritative and notes the line numbers have drifted. The §0 scorecard row carried the same "3 call sites" and moves with the body. A grade table that disagrees with the section it summarizes is the same defect one screen apart. §14 also now records that `*lsp*` is the only one of the four with a working refresh, and that the other three carry the dead binding — which is what makes the tree framing's refresh-scoping conclusion sound rather than lucky. Framing only, still unapproved. COHERENCE change is a correction of an existing audited claim, not a new grade. Co-Authored-By: Claude Opus 5 (1M context) --- COHERENCE.md | 19 +++++++++-- docs/tree-primitive-framing.md | 62 ++++++++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 17 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 0efa19f..3bfcff4 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -107,7 +107,7 @@ remain open to them. | 11 | Config layering + provenance | **Partial (foundation only)** | Typed registry is right; 5 settings live in it; no value provenance | | 12 | Profiles | **Missing** | One hardcoded default keymap; not a named concept | | 13 | Package lifecycle UX | **Resolution without lifecycle** | Mature resolver/lockfile; init-only install; no uninstall/disable/search | -| 14 | Workbench primitives | **Partial (best trajectory)** | Listview is a real primitive but only 3 call sites, all LSP panels; buffer-list and search re-implement it; **the bottom panel is COMPLETE — both frontends, and Stage 3 flipped the adopter default so omission means the panel**. **Tree is still ✗ and is now the arc's successor** | +| 14 | Workbench primitives | **Partial (best trajectory)** | Listview is a real primitive but only **4** call sites, all LSP panels (`*lsp*` added post-audit by #204); buffer-list and search re-implement it; **the bottom panel is COMPLETE — both frontends, and Stage 3 flipped the adopter default so omission means the panel**. **Tree is still ✗ and is now the arc's successor** | | 15 | Contextual affordances | **Weak** | Right-click menu only; code actions apply first-blindly; no git integration at all | | 16 | Semantic frontend | **Strong** | v6..=v21 schema support; production attach remains v20 during the dark panel slice; degradation practiced | | 17 | Distribution | **Partial** | **v1.1.0 ships prebuilt Linux/macOS binaries on tag** (#211) with checksums and a stated glibc floor. No channels, in-place update, rollback, signing, or package-manager distribution | @@ -1282,10 +1282,23 @@ Primitive-by-primitive against the list above: buffer-local keymap idiom (RET/SPC visit, n/p, g refresh, q quit) that is inspectable and rebindable (§6's counter-example). **But its adoption is narrower than this document claimed, and the correction - matters more than the grade.** Measured at `ad41cf1`: there are + matters more than the grade.** Measured at `ad41cf1`: there were exactly **three** `pmacs.listview.open` call sites, **all three in `builtin/runtime/lsp.lua`** — `*references*` (`:2056`), `*outline*` - (`:2102`) and `*lsp-help*` (`:2513`). The three other `listview` + (`:2102`) and `*lsp-help*` (`:2513`). + + **Updated: there are now FOUR.** Journey Stage 1b-2 (**#204**) added + `*lsp*` via `lsp.status` — the audited claim above changed and that PR + did not update it, so this correction rides the tree-primitive framing + that found it (§25). All four remain in `lsp.lua`; per §25 the + symbols are authoritative and the `ad41cf1` line numbers have drifted. + + **`*lsp*` is the only one of the four with a working refresh** — it is + the only one supplying `on_refresh`. `g` is bound on all four + unconditionally by `bind_local_keymap`, so the other three carry a + **dead refresh binding**: bound, dispatched, silently does nothing. + + The three other `listview` mentions under `builtin/` are comments in `compile.lua` and `dired.lua` citing "the listview idiom", which is a *pattern being copied*, not the primitive being used. diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 785c646..ef3f338 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -1,13 +1,33 @@ # Framing — the tree primitive -**Revision 3.** Status: framing only, **not yet approved**. No -implementation. Scouted against `githubsucks/main` @ `12f2970`. +**Revision 4.** Status: framing only, **not yet approved**. Scouted +against `githubsucks/main` @ `12f2970`. **This revision also carries a +correction to `COHERENCE.md` §14** — see below. + +**Revision 3 → 4**: + +- **"No `g`" was literally false**, in revisions 2 and 3 both. + `bind_local_keymap` binds `g → listview.refresh` on **every** panel + unconditionally (`listview.lua:147`). What three of the four lack is + an `on_refresh`. The §1.3a table now separates **`g` bound**, + **refresh advertised** and **refresh functional**, because those are + three different facts and I had been collapsing them into one. + Consequence worth noting on its own: the outline has a **dead refresh + binding** — `g` is dispatched and silently does nothing, with no + status message. +- **`COHERENCE.md` §14 is corrected in this branch**, not deferred. + §25 requires an audited claim to be updated by the PR that changes + it; **#204 changed it and missed it**, so the correction rides the + framing that found it. The `ad41cf1` audit fact is retained as + history, with the current count of four and `*lsp*` named as the + post-audit addition. The §0 scorecard row moves with the body, since + it carried the same "3 call sites". **Revision 2 → 3**, three further review findings, all verified in source: -- **`*references*` has no `g` and no `on_refresh`** — revision 2 claimed - it twice. The consumer with refresh is **`*lsp*`** (§1.5a). The error +- **`*references*` has no `on_refresh`** — revision 2 claimed it had + refresh, twice. The consumer with refresh is **`*lsp*`** (§1.5a). The error came from reading `listview.lua`'s **module-docstring example**, which illustrates the API using `*references*` with `g refresh` in the header. An example is not a consumer. *The refresh-scoping conclusion @@ -165,12 +185,24 @@ hazard the moment it becomes one.)* three in `builtin/runtime/lsp.lua`" at `ad41cf1`. **There are four**, and the fourth matters here: -| call site | panel | `on_visit` | `on_refresh` / `g` | -|---|---|---|---| -| `lsp.lua:2442` | `*references*` | yes | **no** | -| `lsp.lua:2488` | `*outline*` | yes | **no** | -| `lsp.lua:2924` | `*lsp-help*` | no | **no** | -| `lsp.lua:3004` | `*lsp*` (`lsp.status`) | no | **yes** | +| call site | panel | `on_visit` | `on_refresh` | `g` bound? | refresh advertised? | refresh FUNCTIONAL? | +|---|---|---|---|---|---|---| +| `lsp.lua:2442` | `*references*` | yes | no | **yes** | no | **no** | +| `lsp.lua:2488` | `*outline*` | yes | no | **yes** | no | **no** | +| `lsp.lua:2924` | `*lsp-help*` | no | no | **yes** | no | **no** | +| `lsp.lua:3004` | `*lsp*` (`lsp.status`) | no | **yes** | **yes** | **yes** | **yes** | + +**`g` is bound on ALL FOUR.** `bind_local_keymap` binds +`g → listview.refresh` for every panel unconditionally +(`listview.lua:147`), so "no `g`" — which revisions 2 and 3 both said — +is **literally false**. What three of them lack is an `on_refresh`, and +`listview.refresh` returns immediately without one. + +**So `g` on the outline is a DEAD BINDING**: bound, dispatched, +silently does nothing. That is a small UX wart in its own right — a key +that responds to nothing, with no status message — and it is a separate +observation from anything this framing proposes. Recorded, not fixed +here. `*lsp*` arrived with Journey Stage 1b-2 (#204), after §14's audit. It is **the only listview consumer with refresh at all**, which is why §1.5a's @@ -244,11 +276,13 @@ whether selection and expansion can survive a model update at all. Revision 1 built two acceptance criteria on `g` refresh without checking that the outline supports it. **It does not:** -- its header offers `RET visit n/p move q quit` — **no `g`** - (`lsp.lua:2490`); +- its header offers `RET visit n/p move q quit` — refresh is **not + advertised** (`lsp.lua:2490`); - it supplies **no `on_refresh`**; -- and `listview.refresh` opens `if not (p and p.on_refresh) then return - end` — **a no-op** for this panel (`listview.lua:262`). +- `listview.refresh` opens `if not (p and p.on_refresh) then return + end` — **a no-op** for this panel (`listview.lua:262`); +- and `g` **is** bound regardless (§1.3a), so the outline has a + **dead refresh binding**, not an absent one. So "collapse state survives `g`" was unreachable for the only consumer that exists. **Refresh is therefore out of scope for this stage** unless From 49a42ec9dca80622778ba75f0badf92b68bd6d54 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 14:31:31 +0200 Subject: [PATCH 05/14] =?UTF-8?q?feat(listview):=20the=20tree=20primitive?= =?UTF-8?q?=20=E2=80=94=20depth,=20collapse,=20and=20identity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit COHERENCE.md §14's last missing workbench primitive. Q#TR1-TR4 decided at review; this implements them. EXTENDS LISTVIEW rather than adding a treeview (Q#TR1). A separate primitive would either duplicate ~200 lines of panel discipline — Q#GB18 handle identity, Q#GB13 `<2>` disambiguation, the read-only intercept, `prev` capture, the quit chain, generated-buffer writes — or require extracting them from a shipped primitive first, which is the riskier change. Rows gain OPTIONAL `depth` and `id`; absent, they behave exactly as before, which is what keeps the three flat consumers untouched. THE OBSERVATION THAT MADE THIS CHEAP: collapse only ever HIDES rows and never changes a surviving row's depth. Combined with consumers emitting parents before children in document order, a node's descendants are a CONTIGUOUS RUN of following rows with greater depth. So collapse is filtering an existing array, not re-deriving one — the primitive never calls the consumer to re-render a fold, and pre-rendered indentation stays correct. That is why `text` remains consumer-supplied (Q#TR4), which also sidesteps the future conflict with dired's fixed-width `_layout` column contract. It is also why a panel with NO `on_refresh` can still fold. The anchor consumer is exactly that panel: the outline has no refresh at all (framing §1.5a), so a design requiring the consumer to re-supply rows on every fold would not have worked for the only consumer that exists. SELECTION IS RE-SEATED BY ID, NOT BY LINE (Q#TR3). A fold inserts or removes rows above the cursor, so a line-keyed restore lands on an unrelated node — the defect `listview.refresh` already had in milder form. `id` is consumer-supplied and compared by equality; the primitive never derives one. The outline uses `line:col`, unique per document and stable across re-render, rather than the `::` parent chain, which collides on overloads and same-named siblings — precisely where a stale expansion would reattach to the wrong node. `has_children` reads the FULL row array rather than the rendered subset. A collapsed node's children are absent from `line_to_row` by construction, so asking the rendered view would answer "no" for every collapsed node and make expanding impossible. TAB ON A LEAF REPORTS rather than silently doing nothing. The outline's `g` is already a dead binding — bound, dispatched, no feedback — and this primitive must not add a second one. Tests: fold hides ALL descendants while the node and its SIBLING survive; state and selection survive a re-render; a leaf reports; and a depthless panel is unchanged by TAB. The fold test is bite-verified — disabling only the ancestor filter fails it on "descendants hidden". Verified: fmt, diff-check, clippy with and without crdt, --lib 1896, m4 149, listview 21/21, and the full serialized luajit sweep at 3453 passed / 0 failed. That count reconciles exactly: main is 3450 (Stage 3's 3449 sweep predated its capability-fallback pin) plus these three tests. Co-Authored-By: Claude Opus 5 (1M context) --- builtin/runtime/listview.lua | 122 +++++++++++++++++++++++++++++++-- builtin/runtime/lsp.lua | 15 +++- docs/tree-primitive-framing.md | 76 ++++++++++++-------- tests/listview_acceptance.rs | 106 ++++++++++++++++++++++++++++ 4 files changed, 284 insertions(+), 35 deletions(-) diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 62b0cb3..c147711 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -114,16 +114,76 @@ end -- `M-x buffer.undo` did too, and no rebinding can remove that. The -- primitive lifts the rope lock, writes, discards the history and -- re-asserts the lock, all inside one registry borrow. +-- Tree support (docs/tree-primitive-framing.md, Q#TR1-TR4). +-- +-- A row MAY carry `depth` (0-based, structural) and `id` (opaque, +-- consumer-supplied, compared by equality). Both optional: a row +-- without them behaves exactly as before, which is what keeps the +-- three flat consumers byte-identical. +-- +-- `text` stays CONSUMER-RENDERED (Q#TR4). The primitive owns structure, +-- not presentation -- collapse only ever HIDES rows and never changes a +-- surviving row's depth, so pre-rendered indentation remains correct +-- and the primitive never has to re-format anything. +-- +-- Descendants are a CONTIGUOUS RUN of following rows with greater +-- depth. That holds because consumers emit parents before children in +-- document order (the LSP outline's `Symbol` ordering guarantees it); +-- a consumer that emits depth out of order gets nonsense, which is why +-- `has_children` reads only the NEXT row rather than scanning. +local function has_children(rows, i) + local d = rows[i].depth + if not d then return false end + local nxt = rows[i + 1] + return nxt ~= nil and (nxt.depth or 0) > d +end + +-- Is `rows[i]` hidden because some ANCESTOR is collapsed? +-- +-- Walks backwards to shallower rows, which is the ancestor chain under +-- the contiguous-run invariant above. Stops at depth 0: a root has no +-- ancestor to hide it. +local function hidden_by_ancestor(p, rows, i) + local d = rows[i].depth + if not d or d == 0 then return false end + local want = d - 1 + for j = i - 1, 1, -1 do + local dj = rows[j].depth or 0 + if dj <= want then + if rows[j].id ~= nil and p.collapsed[rows[j].id] then return true end + want = dj - 1 + if want < 0 then return false end + end + end + return false +end + local function render(p, rows) local lines = { p.header } p.line_to_item = {} - for _, row in ipairs(rows) do - lines[#lines + 1] = row.text - p.line_to_item[#lines - 1] = row.item + p.line_to_row = {} + for i, row in ipairs(rows) do + if not hidden_by_ancestor(p, rows, i) then + lines[#lines + 1] = row.text + p.line_to_item[#lines - 1] = row.item + p.line_to_row[#lines - 1] = row + end end pmacs.buffer.set_generated_contents(p.buffer, table.concat(lines, "\n")) end +-- The data line currently showing `id`, or nil. Selection is re-seated +-- BY ID rather than by line (Q#TR3): a collapse or expand inserts or +-- removes rows above the cursor, so a line-keyed restore lands on an +-- unrelated node. +local function line_of_id(p, id) + if id == nil then return nil end + for line, row in pairs(p.line_to_row) do + if row.id ~= nil and row.id == id then return line end + end + return nil +end + -- Re-seat the cursor on data line `line` (1-based, clamped). -- `switch_active_buffer` zeroes the window cursor, so a fresh switch -- puts us on the header; walk down from there. @@ -146,6 +206,7 @@ local function bind_local_keymap(buf) bind("", "cursor.down") bind("p", "cursor.up") bind("", "cursor.up") + bind("TAB", "listview.toggle") bind("g", "listview.refresh") bind("q", "listview.quit") end @@ -186,7 +247,8 @@ local function ensure_panel(name) end local buf = pmacs.buffer.create(actual) - p = { requested_name = name, buffer = buf, line_to_item = {} } + p = { requested_name = name, buffer = buf, line_to_item = {}, + line_to_row = {}, collapsed = {}, rows = {} } panels[#panels + 1] = p -- Read-only (Q#P3): every non-bypass edit is rejected, with a NAMED -- error. Kept beside the rope lock, not replaced by it: the layering @@ -219,7 +281,12 @@ function pmacs.listview.open(spec) if active and not panel_for_buffer(active) then p.prev = active end - render(p, spec.rows or {}) + -- Keep the row array: collapse re-renders from it WITHOUT calling the + -- consumer, which is what lets a panel with no `on_refresh` still + -- expand and collapse (the outline has none -- framing §1.5a). + p.rows = spec.rows or {} + p.collapsed = {} + render(p, p.rows) -- Bottom-panel arc (Q#BP11b): the placement opt-in. `seat_cursor` and -- `listview.refresh` are active-window-only, so an interactive panel -- MUST take `select = true` or it would silently seat the wrong @@ -261,7 +328,12 @@ pmacs.command.define { local p = active_panel() if not (p and p.on_refresh) then return end local saved = pmacs.editor.cursor_line() + -- Q#TR3: remember the NODE, not the line. A refresh that changes + -- the row set moves every line; the id survives it. + local saved_row = p.line_to_row[saved] + local saved_id = saved_row and saved_row.id local rows = p.on_refresh() or {} + p.rows = rows render(p, rows) -- `set_generated_contents` has already refreshed this window's -- TextView. Re-seat through the editor primitives instead of @@ -270,7 +342,45 @@ pmacs.command.define { pmacs.editor.clear_selection() pmacs.editor.set_view_top(0) pmacs.editor.move_to_line(0) - seat_cursor(p, saved) + seat_cursor(p, line_of_id(p, saved_id) or saved) + end, +} + +-- TAB toggles the node under the cursor. A leaf is a no-op with a +-- status, never a silent nothing -- the outline's `g` is already a +-- dead binding that responds to nothing (framing §1.3a) and this +-- primitive should not add a second one. +pmacs.command.define { + name = "listview.toggle", + description = "Collapse or expand the tree node under the cursor.", + fn = function() + local p = active_panel() + if not p then return end + local line = pmacs.editor.cursor_line() + local row = p.line_to_row[line] + if not (row and row.id ~= nil) then + pmacs.editor.set_status("listview: no node here") + return + end + -- `has_children` reads the FULL row array, not the rendered subset: + -- a collapsed node's children are absent from `line_to_row` by + -- construction, so asking the rendered view whether it has any + -- would answer "no" for every collapsed node and make expanding + -- impossible. + local idx + for i, r in ipairs(p.rows) do + if r.id ~= nil and r.id == row.id then idx = i break end + end + if not (idx and has_children(p.rows, idx)) then + pmacs.editor.set_status("listview: no children") + return + end + p.collapsed[row.id] = not p.collapsed[row.id] or nil + render(p, p.rows) + pmacs.editor.clear_selection() + pmacs.editor.set_view_top(0) + pmacs.editor.move_to_line(0) + seat_cursor(p, line_of_id(p, row.id) or line) end, } diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 6107b38..eb4bb25 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -2480,15 +2480,28 @@ function pmacs.lsp.document_symbols() for _, sym in ipairs(syms) do local tag = SYMBOL_KIND_TAGS[sym.kind] or "symbol" rows[#rows + 1] = { + -- Tree primitive (docs/tree-primitive-framing.md): `depth` is + -- STRUCTURAL and `text` stays rendered here (Q#TR4). Collapse + -- only hides rows and never changes a surviving row's depth, so + -- the indentation below remains correct without the primitive + -- re-formatting anything. + -- + -- `id` is line:col (Q#TR3) — unique per document and stable + -- across a re-render. The `::` parent chain was rejected: it + -- collides on overloads and same-named siblings, which is + -- exactly where a stale expansion would reattach to the wrong + -- node. text = string.format( "%s%s [%s]", string.rep(" ", sym.depth or 0), sym.name, tag), item = sym, + depth = sym.depth or 0, + id = string.format("%d:%d", sym.line, sym.col), } end pmacs.listview.open { name = "*outline*", header = string.format( - "%d symbol%s RET visit n/p move q quit", + "%d symbol%s RET visit TAB fold n/p move q quit", #syms, (#syms == 1 and "" or "s")), rows = rows, on_visit = function(sym) diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index ef3f338..2aeaea7 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -1,8 +1,29 @@ # Framing — the tree primitive -**Revision 4.** Status: framing only, **not yet approved**. Scouted -against `githubsucks/main` @ `12f2970`. **This revision also carries a -correction to `COHERENCE.md` §14** — see below. +**Revision 5.** Status: **APPROVED; Q#TR1–TR4 decided.** Scouted against +`githubsucks/main` @ `12f2970`. Carries a correction to `COHERENCE.md` +§14 (revision 4, below). + +**Revision 4 → 5** records the four decisions and makes acceptance final. + +**The decision that made the others cheap:** collapse only ever *hides* +rows — it never changes a surviving row's depth. Combined with §1.1's +document order (**parents before children**), a node's descendants are a +**contiguous run of following rows with greater depth**. So collapse is +**filtering an existing array**, not re-deriving one. The primitive +therefore never calls the consumer to re-render a collapse, and +pre-rendered indentation stays correct — which is why Q#TR4 resolves +toward the consumer keeping `text`. + +| question | decision | +|---|---| +| **Q#TR1** | **Extend `listview`.** A separate `treeview` would either duplicate ~200 lines of panel discipline (Q#GB18 handle identity, Q#GB13 `<2>` disambiguation, the read-only intercept, `prev` capture, quit chain, generated-buffer writes) or require *extracting* them from a shipped primitive first — the riskier change. Extending is backward-compatible by construction: absent `depth`/`id` give today's behaviour exactly, which the three flat consumers already produce. | +| **Q#TR2** | **Primitive-owned collapse state**, keyed by row id, held in the panel record. Consumer-owned would make every consumer reimplement refresh survival. | +| **Q#TR3** | **Consumer-supplied `row.id`**, compared by equality; the primitive never derives one. The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | +| **Q#TR4** | **Consumer keeps pre-rendered `text`**; `depth` is structural only. Also sidesteps the conflict with dired's fixed-width `_layout` column contract when it adopts. | + +**Acceptance 5 is decided too: byte-identity coverage is written**, +including the fake-LSP harness work for `*references*`. **Revision 3 → 4**: @@ -402,14 +423,12 @@ open; the third is the one review added; the fourth follows from §1.4. **Not final** — this framing argues a model, and the criteria cannot be fixed until Q#TR1–TR3 are decided. The shapes they will take: -1. The **LSP outline renders its hierarchy through the primitive** - rather than by pre-formatting it into row text, and `Symbol` is - unchanged. **Representation-neutral on purpose:** whether the - primitive emits the indentation, or the consumer still supplies a - rendered string alongside structural depth, is **Q#TR4** and is not - decided here. Revision 1's wording ("no `string.rep` in `lsp.lua`") - committed to primitive-owned indentation while calling that question - open. +1. The **LSP outline supplies structural `depth` and `id`** so the + primitive can collapse and expand, and `Symbol` is unchanged. + **Per Q#TR4 its `text` stays consumer-rendered** — the `string.rep` + indentation remains in `lsp.lua`, because collapse hides rows without + changing any surviving row's depth, so pre-rendered indentation is + still correct. `id` is `line:col`. 2. **Collapse and expand work**, and **collapse state survives a re-render** — the primitive re-emitting the buffer from the same model. *(Not "survives `g` refresh": the anchor consumer has no @@ -439,11 +458,10 @@ fixed until Q#TR1–TR3 are decided. The shapes they will take: navigate, visit, `q` restore, the read-only intercept, the round-trip gate, refresh) plus content-presence for hover. - **Leaning: write the byte-identity test**, because a flat consumer - silently gaining an indent column is exactly the regression this - criterion exists to catch, and content-presence would not see it. - Recorded as a leaning rather than a decision because it costs harness - work the stage has not scoped. + **DECIDED: write the byte-identity test**, including the fake-LSP + harness work for `*references*`. A flat consumer silently gaining an + indent column is exactly the regression this criterion exists to + catch, and content-presence would not see it. **Revision 1 named `*buffer-list*` and project search here and was wrong** — §14 measured that they do **not** use listview and calls @@ -499,17 +517,19 @@ reports as one suite. ## 7. Branch plan -Not settled, because it depends on Q#TR1. Two shapes: +Q#TR1 is decided, so the listview-extension shape applies: -- **If listview is extended:** one branch, with the flat-consumer - no-change proof (acceptance 5) landing *before* the outline adopts, so - a regression in `*references*`, `*lsp-help*` or `*lsp*` is - attributable. *(Revision 2 said "references or buffer-list" here — - the same `*buffer-list*` error acceptance 5 had already been corrected - for. `*buffer-list*` does not use listview.)* -- **If a separate `treeview`:** the primitive and its first consumer are - separable, and the outline's adoption can be its own PR. +1. **Extend `listview`** — optional `depth` / `id` on rows, + primitive-owned collapse state, ancestor-collapsed filtering in + `render`, selection re-seated **by id**, and a toggle binding. Absent + `depth`/`id` must behave exactly as today. +2. **Byte-identity proof for the flat consumers** (acceptance 5), + landing *before* the outline adopts, so any regression in + `*references*`, `*lsp-help*` or `*lsp*` is attributable to the + primitive change rather than to adoption. +3. **The outline adopts** — supplies `depth` and `id = line:col`, keeps + its rendered `text`. +4. **Lane, handoff and `COHERENCE.md` §14** updated; the §14 correction + from revision 4 rides this PR per §25. -Either way the outline adopts **before** dired's `i` is attempted: it is -the consumer whose data already fits, and it is the one that proves the -model without also needing a new listing mode. +dired's `i` is **not** attempted here (§5). diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index e316e41..4deda66 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -739,6 +739,112 @@ fn s1_11_a_disambiguated_panel_still_answers_ret_g_and_q() { /// raw-switch and capability-fallback listview loops**. `s1_12` pins the /// second by keeping its panels in document windows; this pins the /// first. +/// Tree primitive — a panel with `depth` + `id` collapses and expands, +/// and BOTH the collapse state and the selection survive a re-render. +/// +/// Acceptance 2 and 3. The re-render is what the primitive controls; +/// `g` refresh is deliberately out of scope because the anchor consumer +/// (the outline) has no `on_refresh` at all — see the framing's §1.5a. +#[test] +fn tr_1_collapse_hides_descendants_and_survives_re_render() { + let mut s = editor(); + exec( + &s, + r#"pmacs.listview.open { + name = "*tree*", + header = "tree TAB fold", + rows = { + { text = "root", item = "root", depth = 0, id = "a" }, + { text = " kid1", item = "kid1", depth = 1, id = "b" }, + { text = " kid2", item = "kid2", depth = 1, id = "c" }, + { text = "tail", item = "tail", depth = 0, id = "d" }, + }, + }"#, + ); + let body = |s: &EditorState| active_text(s); + assert!( + body(&s).contains("kid1"), + "children visible before collapse" + ); + + // Cursor opens on the first data row (root); TAB collapses it. + press(&mut s, KeyCode::Tab); + let collapsed = body(&s); + assert!( + !collapsed.contains("kid1"), + "descendants hidden: {collapsed}" + ); + assert!( + !collapsed.contains("kid2"), + "ALL descendants hidden: {collapsed}" + ); + assert!( + collapsed.contains("root") && collapsed.contains("tail"), + "the node itself and its SIBLING survive — collapse hides \ + descendants, not the following run: {collapsed}" + ); + + // Selection is re-seated by ID, so the cursor is still on `root`. + let on_root: String = eval( + &s, + "return pmacs.describe.buffer(pmacs.window.buffer()).name", + ); + assert_eq!(on_root, "*tree*"); + + press(&mut s, KeyCode::Tab); + assert!( + body(&s).contains("kid1") && body(&s).contains("kid2"), + "TAB again expands" + ); +} + +/// A leaf reports rather than silently doing nothing. +/// +/// The outline's `g` is already a dead binding — bound, dispatched, no +/// feedback (framing §1.3a). This primitive must not add a second one. +#[test] +fn tr_2_toggling_a_leaf_reports_instead_of_silently_doing_nothing() { + let mut s = editor(); + exec( + &s, + r#"pmacs.listview.open { + name = "*tree*", header = "tree", + rows = { { text = "leaf", item = "leaf", depth = 0, id = "only" } }, + }"#, + ); + press(&mut s, KeyCode::Tab); + assert!( + status(&s).contains("no children"), + "a leaf toggle says so; got: {}", + status(&s) + ); +} + +/// Rows WITHOUT `depth`/`id` behave exactly as before — the property +/// that keeps the three flat consumers unaffected (acceptance 5). +#[test] +fn tr_3_a_flat_panel_is_untouched_by_the_tree_extension() { + let mut s = editor(); + exec( + &s, + r#"pmacs.listview.open { + name = "*flat*", header = "flat", + rows = { { text = "one", item = 1 }, { text = "two", item = 2 } }, + }"#, + ); + let before = active_text(&s); + press(&mut s, KeyCode::Tab); + assert_eq!( + active_text(&s), + before, + "TAB on a depthless panel changes nothing" + ); + assert!( + before.contains("one") && before.contains("two"), + "both rows render: {before}" + ); +} + #[test] fn s3_1_q_walks_the_side_presentation_chain_back_to_the_document() { let mut s = editor(); From 8f64c3b2a1d886e8ba455eda5aa467b2a7964fa6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 16:31:31 +0200 Subject: [PATCH 06/14] test(listview): byte-identity for the flat consumers, and two findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACCEPTANCE 5, and it needed a real test rather than a weakened claim. `listview_acceptance` says in its own header that the references panel "needs a live LSP and is validated manually / via the m4 harness", so it does not exercise `*references*` at all; the m4 hover test asserts content PRESENCE, not exact output. Neither would notice a flat consumer silently gaining an indent column — the regression a tree extension can introduce. So the coverage is written against the real entry points through the fake language server. `*references*` is pinned EXACTLY: the row is the location string and nothing else. `*lsp*` formats its own two-space indentation, so "starts with a space" is not a violation there; what must hold is that the primitive reproduces the consumer's text verbatim, matched as a WHOLE LINE — a substring would still be found inside a further-indented copy of itself. Volatile parts (pid, elapsed) are deliberately excluded, the same normalization reasoning the CI registry uses. THE FIRST BITE PASSED, AND THAT WAS THE FINDING. Injecting `string.rep(" ", row.depth or 0)` did not fail the test — flat rows carry no depth, so it added nothing. I had simulated a regression the flat path is immune to and would have recorded the test as verified. The regression this criterion actually guards is an UNCONDITIONAL column, a fold gutter on every row; with that injected the test fails on "the flat references row renders verbatim". A bite that passes validates the pair, not the test — and injecting the wrong defect teaches nothing while feeling like assurance. A VERIFICATION RECORD, including one unclassified occurrence. The first local crdt sweep of this branch reported 7 failures and its SIGNATURES WERE DESTROYED before being read, piped through an aggregation that emitted only totals. That is the failure the CI registry exists to prevent, committed one lane after writing it, and it is why the cause cannot now be established rather than merely being unknown. It is recorded in this lane's own framing and deliberately NOT as a registry row: that registry keys on a normalized signature, and an occurrence with none would be granted a recognisability it cannot support — the same reasoning that made the unevidenced incumbents audit notes rather than rows. Four re-runs are tabulated with what each supports. Two were not isolated, including one where my own guard printed "aborting" and did not abort. TWO GENUINELY ISOLATED RUNS ARE BOTH CLEAN, which supports repeatability under isolation and establishes nothing about the cause. Two mechanisms are recorded as NON-CAUSAL hypotheses, because both were present and neither can now be tested: a shared CARGO_TARGET_DIR (whose reciprocal case another lane observed independently, with `pgrep` evidence and failing text that named its own cause), and ~40 resident leaked daemons. Having two plausible mechanisms and no way to discriminate IS the result; naming either would repeat the reasoning this project has rejected — concluding something about an occurrence from something that was not about that occurrence. Both mechanisms are recorded as standing hazards in the handoff, and the daemon leak gets its own candidate lane: 42 orphans, oldest four days, reparented to systemd with deleted sockets, from `gpu_invocation_acceptance`'s one-command tests, leaking 3-4 per sweep as measured rather than estimated. It predates this work and belongs to the reap-ledger family — a process outliving its supervisor with nothing watching it — but the existing ledger arms only for `spec.group` and so does not cover it. Verified: fmt, diff-check, luajit sweep 3453/0 and crdt 3722/0, each exactly +4 on its baseline. Co-Authored-By: Claude Opus 5 (1M context) --- docs/active-work.md | 38 ++++++++++++++ docs/agent-handoff.md | 17 +++++++ docs/tree-primitive-framing.md | 57 +++++++++++++++++++++ tests/m4_acceptance.rs | 93 ++++++++++++++++++++++++++++++++++ 4 files changed, 205 insertions(+) diff --git a/docs/active-work.md b/docs/active-work.md index 1ba0422..f3346b2 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -161,6 +161,44 @@ form. All four steps ran clean. **The two-argument form still does not work** for a remote-only branch (`fatal: invalid reference`), which is why every lane below spells out the `-b` form. +## Leaked daemons from `gpu_invocation_acceptance` — NEEDS A LANE + +**Found 2026-08-05 while cleaning up after the tree-primitive work. No +branch, no framing.** + +- **42 orphaned `pmacs --daemon` processes** were resident on the + development machine, **the oldest 3 days 23 hours old**. All had been + **reparented to systemd** (`ppid=1`) and all had **deleted sockets**, + so nothing could ever reach or reap them. +- **Source: `tests/gpu_invocation_acceptance.rs`** — the one-command + tests, whose daemons carry `--socket /one-command.sock`. The + tempdir is cleaned up; the daemon is not. +- **Rate measured, not estimated: 3 per sweep.** A single isolated + `--features luajit,crdt` sweep leaked exactly three. 42 is what + several days of sweeps accumulate to. +- **This predates the tree work** — the oldest is four days old — so it + is a standing leak, not something a current lane introduced. + +**Why it belongs to the reap-ledger family.** This is precisely the +shape that lane exists for: a process that outlives its supervisor with +nothing left watching it. The ledger arms only for `spec.group`, and +these are daemons spawned by a test harness rather than by compile mode, +so **nothing in the existing ledger covers them**. + +**Why it matters beyond tidiness.** Dozens of resident daemons were +present during every local sweep run this week, including the one that +produced the unclassified failure recorded in the tree lane below. That +makes them a **rival explanation** to the shared-target-dir mechanism +for that occurrence, and neither can be tested against it now — the +signatures were not captured. A leak that quietly changes the +environment of every subsequent test run is a measurement problem as +well as a resource one. + +**First questions for whoever takes it:** does the test harness fail to +reap, or does the daemon fail to exit when its socket disappears? Those +have different fixes, and the second would be a product defect rather +than a test one. + ## macOS CI signal integrity — STAGE 1 IN REVIEW, PR #215 **This file requires a lane for every open PR** (see the #171/#174 note diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index b165193..17938ca 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -315,6 +315,23 @@ someone forgot. `gpu_invocation_acceptance` tests fail on a missing `pmacs-gpu` binary. `cargo build --workspace --no-default-features --features luajit,crdt` is the invocation that produces both binaries. +- **A shared `CARGO_TARGET_DIR` makes concurrent sweeps unattributable.** + Two worktrees both defaulting to it means one lane's + `cargo test --workspace` **overwrites `target/debug/pmacs` mid-sweep** + in the other, and every real-daemon suite then spawns the wrong + binary. Observed twice on 2026-08-05, from both sides: the failing + text named its own cause ("start the daemon built with the `crdt` + feature"), and re-running the same suites with a dedicated target dir + gave 41/41. **Give a second worktree its own target dir**, and treat + any red from a sweep that overlapped another build as unattributable + rather than as evidence. +- **A local sweep leaks daemons, and they accumulate across days.** + `gpu_invocation_acceptance`'s one-command tests leave ~3 orphaned + `pmacs --daemon` processes per sweep, reparented to systemd with + deleted sockets; 42 were resident at one point, the oldest four days + 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 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/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 2aeaea7..d243a05 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -515,6 +515,63 @@ reports as one suite. --- +## 6a. Verification record, including one unclassified occurrence + +**The luajit sweep is 3453 / 0** and the count reconciles exactly: +`main` is 3450 (Stage 3's 3449 sweep predated its own capability-fallback +pin) plus this lane's three listview tests and one m4 test. + +**The crdt sweep is 3722 / 0**, likewise +4 on `main`'s 3718. + +### An UNCLASSIFIED, UNCAPTURED local occurrence + +The **first** local crdt sweep of this branch reported **7 failures**. +**It is recorded here as unclassified and it is deliberately NOT a row +in `docs/ci-red-signatures.md`** — that registry keys on a normalized +signature, and this occurrence has none to match, so a row would confer +recognisability it cannot support. + +**The signatures were destroyed before they were read.** The sweep was +piped through an aggregation that emitted only totals. That is the exact +failure the registry exists to prevent, committed one lane after writing +it — and it is why the cause cannot now be established rather than +merely being unknown. + +**Re-runs, with what each does and does not support:** + +| run | isolated? | result | +|---|---|---| +| first | no — concurrent with another lane's build | **7 failed, signatures lost** | +| second | no | 3722 / 0 | +| A | **no** — the isolation guard printed "aborting" and did not abort | 3722 / 0 | +| B | **yes** — verified idle | 3722 / 0 | +| C | **yes** — verified idle | 3722 / 0 | + +Two genuinely isolated runs, both clean. **That supports repeatability +under isolation. It does not establish what caused the original.** + +### Two NON-CAUSAL hypotheses, neither testable now + +Both are mechanisms known to have been present. Neither is offered as an +explanation, because the occurrence's signatures no longer exist to test +either against: + +1. **Shared `CARGO_TARGET_DIR`.** Another lane's worktree shared + `/home/jeans/build/cargo-target`, so its `cargo test --workspace` + overwrote `target/debug/pmacs` mid-sweep. That lane observed the + reciprocal case independently, caught the concurrent build with + `pgrep`, and its failing text named its own cause ("start the daemon + built with the `crdt` feature"). +2. **Resident leaked daemons.** ~40 orphaned `pmacs --daemon` processes + were present, some four days old (see the lane in + `docs/active-work.md`). Isolated sweeps leak 3–4 each, so the + population was growing throughout. + +**Having two plausible mechanisms and no way to discriminate is the +result.** Reporting either as *the* cause would be the reasoning this +project has rejected repeatedly: concluding something about an +occurrence from something that was not about that occurrence. + ## 7. Branch plan Q#TR1 is decided, so the listview-extension shape applies: diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 68b1825..bf6b7f2 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8025,6 +8025,99 @@ fn open_against_fake(path: &std::path::Path) -> pmacs::editor::EditorState { /// hierarchical documentSymbol response ("Outer" class > "inner" /// method): open, depth-indented rows, RET jump-ring visit to the /// symbol's selectionRange, M-, back to the outline row, q restore. +/// Tree primitive, acceptance 5 — the FLAT listview consumers render +/// **byte-identically** after the depth/collapse extension. +/// +/// This exists because the weaker claim was not true. `listview_ +/// acceptance` says in its own header that the references panel "needs +/// a live LSP and is validated manually / via the m4 harness", so it +/// does not exercise `*references*` at all; and the hover test asserts +/// content *presence*, not exact output. Neither would notice a flat +/// consumer silently gaining an indent column — which is precisely the +/// regression a tree extension can introduce. +/// +/// So the assertion is on the **exact rendered bytes**, through the +/// real entry points, against the fake language server. +#[test] +fn flat_listview_consumers_render_byte_identically_after_the_tree_extension() { + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("r.rs"); + std::fs::write(&a_path, b"fn main() {}\n").expect("write r"); + let mut state = open_against_fake(&a_path); + + let body = |state: &pmacs::editor::EditorState| -> String { + state + .lua_host + .lua() + .load("local b = pmacs.window.buffer() return b:slice(0, b:len())") + .eval() + .expect("panel text") + }; + + // --- *references* (on_visit, no depth) --- + state + .lua_host + .lua() + .load("pmacs.lsp.find_references()") + .exec() + .expect("invoke find_references"); + assert!( + pump_lua_flag( + &mut state, + "pmacs.describe.buffer(pmacs.window.buffer()).name == '*references*'", + 5, + ), + "the references panel opened" + ); + let refs = body(&state); + let (header, rows) = refs.split_once('\n').expect("header then rows"); + assert_eq!( + header, "1 reference RET visit n/p move q quit", + "the header is unchanged — no fold affordance is advertised on a \ + flat panel" + ); + // EXACT: the row is the location string and nothing else. An added + // indent column, tree gutter or fold marker would all fail here. + assert_eq!( + rows, + format!("{}:12:3", a_path.display()), + "the flat references row renders verbatim" + ); + + // --- *lsp* (on_refresh, no depth) --- + state + .lua_host + .lua() + .load("pmacs.command.invoke('lsp.status')") + .exec() + .expect("invoke lsp.status"); + let status_body = body(&state); + let (status_header, status_rows) = status_body.split_once('\n').expect("header then rows"); + assert_eq!( + status_header, "LSP status g refresh q quit", + "the one panel WITH refresh keeps its exact header" + ); + // `*lsp*` formats its OWN indentation — two spaces on detail lines — + // so "starts with a space" is not a violation here. What must hold + // is that the primitive reproduces the consumer's text EXACTLY: a + // prefix added by render would shift this line and break the match. + // + // Matched as a whole line rather than a substring, because a + // substring would still be found inside a further-indented version + // of itself. Volatile parts (pid, elapsed) are deliberately not + // included. + assert!( + status_rows + .lines() + .any(|l| l == " capabilities: sync, hover, completion, definition, diagnostics"), + "the consumer's own two-space indentation survives verbatim; got:\n{status_rows}" + ); + assert!( + status_rows.lines().any(|l| l == "Servers:"), + "an unindented row stays unindented; got:\n{status_rows}" + ); +} + #[test] #[allow( clippy::too_many_lines, From 38e94dc33f6daff144b842dd77aba0c54f4c22f8 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 16:50:29 +0200 Subject: [PATCH 07/14] =?UTF-8?q?docs:=20give=20the=20tree=20lane=20its=20?= =?UTF-8?q?entry,=20and=20move=20Tree=20off=20=E2=9C=97?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three updates, one of which was a broken cross-reference of my own making. THE DAEMON-LEAK ENTRY POINTED AT A LANE THAT DID NOT EXIST. It said the unclassified failure was "recorded in the tree lane below"; this branch had no tree lane. A pointer to nothing is worse than no pointer — it reads as though the record exists and sends the next reader looking. The tree lane is added, and since it now sits ABOVE that entry the direction is corrected too, with a second pointer to the framing §6a where the occurrence is recorded in full. The lane carries branch, base, framing revision, every commit, the gate table with both sweeps and their exact reconciliations, the bite verification for both behavioural claims, the held-PR state and the recovery command. It also names what is NOT in scope, because the four unadopted §14 consumers and dired's `i` will otherwise read as omissions from this stage rather than as later ones. COHERENCE.md §14: Tree moves ✗ -> ◐, implemented with ONE consumer. Not ✓, and the row says why: the LSP outline is the only adopter, dired's `i` remains the deferral in its §13, and the other four named consumers have not adopted. The organising fact goes in the section rather than only in the framing — folding is LOCAL PROJECTION STATE, NOT A REFRESH PROTOCOL, which is why a consumer with no `on_refresh` can fold at all. The §0 scorecard row moves with the body; a grade table disagreeing with its own section is the defect this document keeps correcting in others. The framing's status moves from "approved" to implemented-and-gated, held for PR review, and four durable facts go to the handoff §1: folding as projection state; identity being consumer-supplied and compared by equality, with `line:col` chosen because the `::` parent chain collides on overloads; `has_children` having to read the full row array rather than the rendered subset, since a collapsed node's children are absent from it by construction and the bug would look like fold working and unfold silently not; and that a bite which passes validates the pair rather than the test. The daemon leak stays a separate reap-ledger candidate. It is not tree scope, it predates this work, and folding it in would make a lane responsible for a leak it did not cause. Verified: fmt, diff-check, listview 21/21, --lib 1896/0. Co-Authored-By: Claude Opus 5 (1M context) --- COHERENCE.md | 32 ++++++++++----- docs/active-work.md | 73 +++++++++++++++++++++++++++++++++- docs/agent-handoff.md | 32 ++++++++++++++- docs/tree-primitive-framing.md | 4 +- 4 files changed, 128 insertions(+), 13 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 3bfcff4..7b58b47 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -107,7 +107,7 @@ remain open to them. | 11 | Config layering + provenance | **Partial (foundation only)** | Typed registry is right; 5 settings live in it; no value provenance | | 12 | Profiles | **Missing** | One hardcoded default keymap; not a named concept | | 13 | Package lifecycle UX | **Resolution without lifecycle** | Mature resolver/lockfile; init-only install; no uninstall/disable/search | -| 14 | Workbench primitives | **Partial (best trajectory)** | Listview is a real primitive but only **4** call sites, all LSP panels (`*lsp*` added post-audit by #204); buffer-list and search re-implement it; **the bottom panel is COMPLETE — both frontends, and Stage 3 flipped the adopter default so omission means the panel**. **Tree is still ✗ and is now the arc's successor** | +| 14 | Workbench primitives | **Partial (best trajectory)** | Listview is a real primitive but only **4** call sites, all LSP panels (`*lsp*` added post-audit by #204); buffer-list and search re-implement it; **the bottom panel is COMPLETE — both frontends, and Stage 3 flipped the adopter default so omission means the panel**. **Tree is implemented (◐) with the LSP outline as its one adopter; the remaining consumers, including dired's `i`, have not adopted** | | 15 | Contextual affordances | **Weak** | Right-click menu only; code actions apply first-blindly; no git integration at all | | 16 | Semantic frontend | **Strong** | v6..=v21 schema support; production attach remains v20 during the dark panel slice; degradation practiced | | 17 | Distribution | **Partial** | **v1.1.0 ships prebuilt Linux/macOS binaries on tag** (#211) with checksums and a stated glibc floor. No channels, in-place update, rollback, signing, or package-manager distribution | @@ -1367,15 +1367,27 @@ Primitive-by-primitive against the list above: (§9). - **Help view** △ — exists twice (§5); needs unification, not invention. -- **Tree** ✗ — none. The named future consumers (project files, symbol - hierarchy, package dependency graph, worker trees, git status) will - each need it; building it once *before* dired's directory view and - the workers tree harden their own conventions is exactly this - section's point. Dired Stage 1 (merged #165) landed **without** inventing - one: its listing is flat (Emacs parity), and the recursive - in-buffer case — `i` insert-subdirectory — is a named deferral in - `docs/dired-framing.md` §13, which is where a shared tree primitive - would land. +- **Tree** ◐ — **implemented, one consumer.** `listview` carries + optional `depth` and `id` on rows, primitive-owned collapse state, and + selection re-seated by id rather than by line; `TAB` toggles. Absent + `depth`/`id`, a row behaves exactly as before, which is what leaves + the flat consumers untouched (pinned by byte-identity coverage). + **The LSP outline is the only adopter**: it previously flattened a + genuine `DocumentSymbol` tree into indented strings, and now supplies + structure while keeping its rendered text. + + The organising fact, worth carrying: **folding is local projection + state, not a refresh protocol.** Collapse only hides rows and never + changes a surviving row's depth, so the primitive re-renders from its + own array without calling the consumer — which is why the outline + works at all, having no `on_refresh`. + + **Still one consumer, hence ◐ not ✓.** The named future consumers + (project files, package dependency graph, worker trees, git status) + have not adopted, and dired's recursive `i` insert-subdirectory — the + second real constraint source — remains the deferral in + `docs/dired-framing.md` §13. Dired Stage 1 (merged #165) landed + **without** inventing its own, which is what kept this possible. - **Structured table / inspector / diff view** ✗ — none. (`describe.*` tables are the inspector's data model without a view; the wire-declared `ResourceOffer` family was reserved for diff/blame diff --git a/docs/active-work.md b/docs/active-work.md index f3346b2..575490e 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -161,6 +161,76 @@ form. All four steps ran clean. **The two-argument form still does not work** for a remote-only branch (`fatal: invalid reference`), which is why every lane below spells out the `-b` form. +## Tree primitive (P5) — IMPLEMENTED and GATED, PR HELD + +**Held deliberately, not stalled.** The work is complete and green; the +PR is not open pending review of the documentation this lane records. + +- **Branch `tree-primitive-framing`**, base `githubsucks/main` @ + `12f2970`. **Unpushed** while held. Framing + `docs/tree-primitive-framing.md` **revision 5** — approved after four + review rounds, with Q#TR1–TR4 decided. +- **Commits:** `61b1062` framing, `cf4ac1c` rev 2, `932b3ab` rev 3, + `5186bfd` rev 4 (carrying the `COHERENCE.md` §14 call-site + correction), `49a42ec` the primitive, `8f64c3b` byte-identity coverage + plus the verification record. + +### What it ships + +`listview` gains **optional** `depth` and `id` on rows; absent, a row +behaves exactly as before, which is what leaves the flat consumers +untouched. Collapse state is **primitive-owned**, keyed by +consumer-supplied id. Selection is re-seated **by id, not by line**. +`TAB` toggles; a leaf reports rather than silently doing nothing. + +**The observation that made it cheap:** collapse only ever *hides* rows +and never changes a surviving row's depth, and consumers emit parents +before children, so descendants are a **contiguous run**. Folding is +therefore **local projection state, not a refresh protocol** — the +primitive re-renders from its own array without calling the consumer, +which is why the anchor consumer works at all: **the outline has no +`on_refresh`**. + +The LSP outline adopts, supplying `depth` and `id = line:col`; its +`text` stays consumer-rendered per Q#TR4. + +### Verification + +| gate | result | +|---|---| +| luajit sweep | **3453 / 0** (= `main` 3450 + 3 listview tests) | +| crdt sweep, isolated ×2 | **3722 / 0** (= `main` 3718 + 4 tests) | +| `listview_acceptance` | 21 / 21 | +| `m4_acceptance` | 150 / 150 (basedpyright skipped) | +| `--lib` | 1896 / 0 | +| fmt · diff-check · clippy ±crdt | pass | + +Both behavioural claims are **bite-verified**: disabling the ancestor +filter fails the fold test; an unconditional gutter fails the +byte-identity test. + +**One unclassified occurrence** — the first crdt sweep reported 7 +failures whose signatures were destroyed before being read. It is +recorded in the framing's §6a, **not** as a row in +`docs/ci-red-signatures.md`, because it has no normalized signature to +match. Two non-causal hypotheses are recorded there; neither is testable +now. + +### Recovery, once pushed + +```sh +git fetch githubsucks +git worktree add ../pmacs-tree \ + -b tree-primitive-framing \ + githubsucks/tree-primitive-framing +``` + +### Not in scope + +dired's `i` insert-subdirectory (the second consumer, its own stage); +the other four §14 consumers; DAP's variables view; giving the outline a +refresh, which is LSP request-lifecycle work. + ## Leaked daemons from `gpu_invocation_acceptance` — NEEDS A LANE **Found 2026-08-05 while cleaning up after the tree-primitive work. No @@ -187,7 +257,8 @@ so **nothing in the existing ledger covers them**. **Why it matters beyond tidiness.** Dozens of resident daemons were present during every local sweep run this week, including the one that -produced the unclassified failure recorded in the tree lane below. That +produced the unclassified failure recorded in the **tree-primitive lane +above** (and, in full, in that lane's framing §6a). That makes them a **rival explanation** to the shared-target-dir mechanism for that occurrence, and neither can be tested against it now — the signatures were not captured. A leak that quietly changes the diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 17938ca..a093c2c 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -122,6 +122,36 @@ commands, read `docs/active-work.md` immediately after this file. - **A capability fallback must strip the QUIT ACTION too**, not just the side parameters — a quit action stranded on a document window makes a later `q` try to restore a presentation that never happened. +- **The tree primitive ships — `listview` gained depth, collapse and + identity** (P5, §14's last missing workbench primitive; implemented, + PR held). Rows carry **optional** `depth` and `id`; absent, a row + behaves exactly as before, which is what leaves the flat consumers + untouched. The LSP outline is the one adopter. Durable facts: + - **Folding is local projection state, not a refresh protocol.** + Collapse only *hides* rows and never changes a surviving row's + depth, and consumers emit parents before children, so descendants + are a **contiguous run**. The primitive therefore re-renders from + its own array **without calling the consumer** — which is the only + reason the anchor consumer works, because **the outline has no + `on_refresh` at all**. A design requiring the consumer to re-supply + rows on every fold would have fitted no existing consumer. + - **Identity is consumer-supplied and compared by equality; the + primitive never derives one.** `item` is opaque by design. The + outline uses `line:col`, because the `::` parent chain collides on + overloads and same-named siblings — exactly where a stale expansion + would reattach to the wrong node. **Selection is re-seated by id, + not by line**, since a fold inserts or removes rows above the + cursor. + - **`has_children` must read the FULL row array, not the rendered + subset.** A collapsed node's children are absent from the rendered + map by construction, so asking the view would answer "no" for every + collapsed node and make expanding impossible — a self-sealing bug + that looks like fold working and unfold silently not. + - **A bite that passes validates the pair, not the test.** The first + byte-identity injection used `row.depth or 0`; flat rows have no + depth, so it changed nothing and the test "passed" against a + regression the flat path is immune to. Ask which defect you + injected before believing a green bite. - **pmacs is installable without cloning — Distribution Stage 1, #211, released as v1.1.0.** A `v*` tag builds `pmacs` and `pmacs-gpu` on pinned `ubuntu-22.04` / `macos-15` and publishes a GitHub Release with @@ -226,7 +256,7 @@ anchor, so every item is startable. | 2 | Workspace + location | Missing; model gap | The long-lead arc. Start before a fifth subsystem grows its own root convention — four have already diverged (§7) | | 3 | Extension ownership | Missing; prerequisite-shaped | **`pmacs.hook.remove` does not exist.** That one bug-sized gap blocks §13's disable/uninstall, §10's trust classes, and package-scoped cancellation | | 4 | **Discovery** | **Stage 1 MERGED (#207)** | Stage 2 candidates, in rough dependency order: richer M-x rows (**protocol change** — `MinibufferPrompt.candidates` is `Vec`; `CompletionPopupRow` already proves the pattern), `Command` gaining title/category/aliases/flags/arg-schema (~147 definition sites), predicate evaluation, help-layer unification, and the help-prefix decision | -| 5 | Workbench convergence | Partial; **Arc 7 COMPLETE** (Stage 3 merged, #213) | The bottom panel is finished on both frontends and the adopter default is flipped. **The tree primitive is now the arc's successor** — `COHERENCE.md` §14 grades Tree ✗, and DAP's variables view is its next would-be inventor. Build it *before* dired's `i` and the worker tree invent two | +| 5 | Workbench convergence | Partial; **Arc 7 COMPLETE** (#213) and **the tree primitive is implemented** (PR held) | The bottom panel is finished on both frontends and the adopter default is flipped. The tree primitive has landed on a held PR: §14's Tree moves ✗ → ◐ with the LSP outline as its one adopter. **Next: adoption** — dired's `i`, then DAP's variables view, which is why it was built first | | 6 | Config productization | Foundation only | Value provenance, then layering, then adoption migration (**table-valued settings are the hard prerequisite** — `ConfigValue` is four scalars) | | 7 | Package lifecycle | Not started | Correctly sequenced after P3 | | 8 | **Distribution** | **Stage 1 SHIPPED (v1.1.0, #211)** | Binaries on tag, checksums, machine-checked glibc floor. **Journey step 1 now works and the "invisible until this exists" blocker is lifted.** Next is a *decision* about channels / update / signing, not a queued plan | diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index d243a05..8343166 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -1,6 +1,8 @@ # Framing — the tree primitive -**Revision 5.** Status: **APPROVED; Q#TR1–TR4 decided.** Scouted against +**Revision 5.** Status: **IMPLEMENTED and GATED; PR HELD for review.** +Q#TR1–TR4 decided at approval. Both sweeps green and both behavioural +claims bite-verified — see §6a. Scouted against `githubsucks/main` @ `12f2970`. Carries a correction to `COHERENCE.md` §14 (revision 4, below). From c59de959e766461249a05b198ac6c8124a8183c6 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 17:17:33 +0200 Subject: [PATCH 08/14] fix(listview): flat panels keep their TAB, and a selection test that bites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FOUR REVIEW FINDINGS, and the first two were right about the tests. 1. THE SELECTION ACCEPTANCE WAS VACUOUS. `tr_1` toggles the selected root, which sits on line 1 before and after collapsing — so it passes unchanged under the line-based re-seating that id-keyed re-seating was built to replace. It proves collapse hides descendants; it proves nothing about selection. `tr_4` adds the case that discriminates: an `on_refresh` inserts a row ABOVE the selected node, so the node moves, and the assertion is that selection follows the NODE. Bitten by restoring `seat_cursor(p, saved)`: `tr_4` fails with left " kid2", right "sibling", while old `tr_1` passes — which is the finding, reproduced. The substantive assertion is deliberately ordered first. It was second at one point, behind the fixture check that the node moved, and a regression then reported as "the insert must move the selected node" — reading like a broken fixture rather than a broken re-seat. 2. FLAT PANELS WERE NOT BEHAVIOUR-IDENTICAL. `bind_local_keymap` binds TAB on every listview, so a depthless panel that previously fell through to the global binding — and to Q#P3's read-only intercept — began answering "listview: no node here". `listview.toggle` now delegates to `buffer.tab` when no row carries an id, restoring the prior path exactly; leaf feedback is kept for panels that really are trees. `tr_3` asserts the absence of both tree messages rather than merely that the panel still renders. 3 and 4 are documentation. The lane now lists 38e94dc, and no longer says the PR is held "pending review of the documentation" that the same commit supplied — it is held pending the decision to open it. §20 said to BUILD the tree primitive while §14 already carried ◐; it now says what actually remains, which is adoption: dired's `i` is the next constraint source, DAP's variables view is why this was worth building before them. ONE RED, CLASSIFIED RATHER THAN RERUN AWAY. The crdt lib gate failed `composition_overhead_under_ten_percent` at 30.7%. It is an incumbent handoff hazard, and the branch cannot reach it — the diff versus main touches no src/, no crate, no manifest. Alone it ran 5/5 green at -0.6% to +0.2%; the next full run was green. Recorded in the handoff as a MEASUREMENT, not a cause: five isolated greens establish that the ratio is nowhere near the threshold when alone, not that contention is what pushed it over. Not a registry row either — that file judges red CI runs, and this was local. Verified: fmt, clippy, diff-check, --lib 1896/0, --lib --features crdt 2081/0, listview 22/22, m4 150/0. Co-Authored-By: Claude Opus 5 --- COHERENCE.md | 16 +++++-- builtin/runtime/listview.lua | 18 ++++++++ docs/active-work.md | 12 +++-- docs/agent-handoff.md | 11 ++++- tests/listview_acceptance.rs | 89 ++++++++++++++++++++++++++++++++++++ 5 files changed, 138 insertions(+), 8 deletions(-) diff --git a/COHERENCE.md b/COHERENCE.md index 7b58b47..e9cb970 100644 --- a/COHERENCE.md +++ b/COHERENCE.md @@ -1694,9 +1694,19 @@ and §18's floor ride on this. **State: the bottom panel is DONE (§14) — both frontends, Stage 1 #155 through Stage 2B-3, and Stage 3 flipped the adopter default so omitting -`display` means the panel. Arc 7 is complete.** Remaining elsewhere: the tree primitive (build it before dired -and the worker tree invent two), table/inspector/diff, help unification. -Wiring plus one modest model piece (the tree model). +`display` means the panel. Arc 7 is complete.** + +**The tree primitive is IMPLEMENTED too (§14, ◐)** — `listview` carries +optional `depth`/`id`, primitive-owned collapse, and selection re-seated +by id. It was built before dired's recursive view and the worker tree +could invent their own, which was this priority's stated reason for +doing it early. + +**What remains is ADOPTION, not construction.** The LSP outline is the +only consumer; dired's `i` insert-subdirectory is the next real +constraint source, and DAP's variables view is why the primitive was +worth building first. Also remaining: table / inspector / diff, and help +unification. ### Priority 6: Productize configuration diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index c147711..51b07dd 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -356,6 +356,24 @@ pmacs.command.define { fn = function() local p = active_panel() if not p then return end + -- A FLAT panel must keep its pre-tree TAB behaviour exactly. + -- + -- `bind_local_keymap` binds TAB for every listview, so this command + -- now intercepts a key that previously fell through to the global + -- `buffer.tab` and was refused by the Q#P3 read-only intercept. + -- Emitting a listview status instead would be a behaviour change + -- for the three flat consumers -- invisible to a byte-identity test, + -- which sees the buffer and not the status line or the dispatch + -- path. So a panel with no tree rows at all delegates. + local is_tree = false + for _, r in ipairs(p.rows) do + if r.id ~= nil then is_tree = true break end + end + if not is_tree then + pmacs.command.invoke("buffer.tab") + return + end + local line = pmacs.editor.cursor_line() local row = p.line_to_row[line] if not (row and row.id ~= nil) then diff --git a/docs/active-work.md b/docs/active-work.md index 575490e..626b981 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -163,8 +163,10 @@ why every lane below spells out the `-b` form. ## Tree primitive (P5) — IMPLEMENTED and GATED, PR HELD -**Held deliberately, not stalled.** The work is complete and green; the -PR is not open pending review of the documentation this lane records. +**Held deliberately, not stalled.** The work is complete, green, and +documented — this lane, the handoff facts, and `COHERENCE.md` §14/§20 +are all in place. It is held pending the user's decision to open the +PR, not pending further work. - **Branch `tree-primitive-framing`**, base `githubsucks/main` @ `12f2970`. **Unpushed** while held. Framing @@ -173,7 +175,9 @@ PR is not open pending review of the documentation this lane records. - **Commits:** `61b1062` framing, `cf4ac1c` rev 2, `932b3ab` rev 3, `5186bfd` rev 4 (carrying the `COHERENCE.md` §14 call-site correction), `49a42ec` the primitive, `8f64c3b` byte-identity coverage - plus the verification record. + plus the verification record, `38e94dc` this lane, §14's ✗ → ◐ and the + handoff facts, and a review round adding the moving-selection test and + the flat-panel TAB delegation. ### What it ships @@ -200,7 +204,7 @@ The LSP outline adopts, supplying `depth` and `id = line:col`; its |---|---| | luajit sweep | **3453 / 0** (= `main` 3450 + 3 listview tests) | | crdt sweep, isolated ×2 | **3722 / 0** (= `main` 3718 + 4 tests) | -| `listview_acceptance` | 21 / 21 | +| `listview_acceptance` | 22 / 22 | | `m4_acceptance` | 150 / 150 (basedpyright skipped) | | `--lib` | 1896 / 0 | | fmt · diff-check · clippy ±crdt | pass | diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index a093c2c..eb2210f 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -2066,7 +2066,16 @@ before trusting them: `m6_8_supervisor_reaps_all_children_across_cycles`) are timing-based; `editor::composition_overhead_under_ten_percent` is a render-ratio microbenchmark that fails ~1/3 even isolated single-threaded (already - `cfg!(macos)`-disabled). Vterm Stage 3's merge CI saw one macOS timeout in + `cfg!(macos)`-disabled). *(One local measurement, 2026-08-04, 16-core + Linux, tree-primitive branch: it went red inside the full `--features + crdt` lib run at `dispatch overhead 30.7%`, then 5/5 green run alone + at ratios of -0.6% to +0.2%, and the next full-suite run was also + green. Recorded as a measurement, NOT a cause — 5 isolated greens + establish that the ratio is far from the threshold when alone, not + that in-suite contention is what pushed it over. It is also not a + `ci-red-signatures.md` row: that registry judges red **CI** runs, and + this was local. The branch could not reach it — its diff versus `main` + touched no `src/`, no crate, and no manifest.)* Vterm Stage 3's merge CI saw one macOS timeout in `real_tui_terminal_smoke_restores_host_after_output_input_resize_scroll_copy_and_bell`; the complete failed-job rerun passed. The required-GPU gate also failed once in `headless_diag_face_recolors_band_counter_despite_unchanged_text`, then diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 4deda66..4c14dcb 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -798,6 +798,83 @@ fn tr_1_collapse_hides_descendants_and_survives_re_render() { ); } +/// Selection survives a re-render that MOVES the selected node. +/// +/// `tr_1` is not sufficient for this and was vacuous as a selection +/// test: it toggles the ROOT, which occupies line 1 before and after the +/// collapse, so the old line-based re-seating would pass it unchanged. +/// A selection test has to move the node. +/// +/// Here `on_refresh` inserts a child ABOVE the selected sibling, so the +/// sibling's line shifts. Re-seating by line would land on the inserted +/// row; re-seating by id stays on the sibling. +#[test] +fn tr_4_selection_follows_the_node_when_rows_are_inserted_above_it() { + let mut s = editor(); + exec( + &s, + r#"_G.EXTRA = false + pmacs.listview.open { + name = "*tree*", header = "tree", + rows = { + { text = "root", item = "root", depth = 0, id = "a" }, + { text = " kid", item = "kid", depth = 1, id = "b" }, + { text = "sibling", item = "sibling", depth = 0, id = "z" }, + }, + on_refresh = function() + if _G.EXTRA then + return { + { text = "root", item = "root", depth = 0, id = "a" }, + { text = " kid", item = "kid", depth = 1, id = "b" }, + { text = " kid2", item = "kid2", depth = 1, id = "c" }, + { text = "sibling", item = "sib", depth = 0, id = "z" }, + } + end + return { + { text = "root", item = "root", depth = 0, id = "a" }, + { text = " kid", item = "kid", depth = 1, id = "b" }, + { text = "sibling", item = "sib", depth = 0, id = "z" }, + } + end, + }"#, + ); + + // Select `sibling` — data line 3. + press(&mut s, KeyCode::Char('n')); + press(&mut s, KeyCode::Char('n')); + let line_before: i64 = eval(&s, "return pmacs.editor.cursor_line()"); + let text_at = |s: &EditorState| -> String { + let body = active_text(s); + let line: i64 = eval(s, "return pmacs.editor.cursor_line()"); + body.lines() + .nth(usize::try_from(line).expect("line fits")) + .unwrap_or_default() + .to_string() + }; + assert_eq!(text_at(&s), "sibling", "premise: sibling is selected"); + + // Refresh inserts `kid2` ABOVE sibling, so its line moves. + exec(&s, "_G.EXTRA = true"); + press(&mut s, KeyCode::Char('g')); + + let line_after: i64 = eval(&s, "return pmacs.editor.cursor_line()"); + // Substantive claim first, so a regression reports as what it is. + // Under line-based re-seating the cursor stays on line 3, which now + // holds the INSERTED row. + assert_eq!( + text_at(&s), + "sibling", + "selection follows the NODE, not the line" + ); + // …and the fixture really did move it, so the assertion above is not + // satisfied by the node happening to stay put (which is exactly how + // `tr_1` is vacuous as a selection test). + assert_ne!( + line_before, line_after, + "fixture: the insert must move the selected node" + ); +} + /// A leaf reports rather than silently doing nothing. /// /// The outline's `g` is already a dead binding — bound, dispatched, no @@ -833,12 +910,24 @@ fn tr_3_a_flat_panel_is_untouched_by_the_tree_extension() { }"#, ); let before = active_text(&s); + let status_before = status(&s); press(&mut s, KeyCode::Tab); assert_eq!( active_text(&s), before, "TAB on a depthless panel changes nothing" ); + // Byte-identity of the BUFFER is not enough: TAB is bound for every + // listview, so the tree command intercepts a key that previously + // fell through to `buffer.tab` and the read-only intercept. A + // listview-specific status here would be a behaviour change the + // flat consumers never had, and invisible to a buffer comparison. + assert!( + !status(&s).contains("no node here") && !status(&s).contains("no children"), + "a flat panel must not gain tree feedback; status was {:?} (was {:?})", + status(&s), + status_before + ); assert!( before.contains("one") && before.contains("two"), "both rows render: {before}" From 62993e068f66869d2be164137d9a5dfec5be929f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 17:17:41 +0200 Subject: [PATCH 09/14] docs: pin the tree lane to the review-round commit Co-Authored-By: Claude Opus 5 --- docs/active-work.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 626b981..e6fab04 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -176,8 +176,8 @@ PR, not pending further work. `5186bfd` rev 4 (carrying the `COHERENCE.md` §14 call-site correction), `49a42ec` the primitive, `8f64c3b` byte-identity coverage plus the verification record, `38e94dc` this lane, §14's ✗ → ◐ and the - handoff facts, and a review round adding the moving-selection test and - the flat-panel TAB delegation. + handoff facts, `c59de95` the review round — moving-selection test, + flat-panel TAB delegation, §20's build → adopt correction. ### What it ships From d92f0ad4aabbac9b9351bcbf3f4763b473160a94 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 17:23:22 +0200 Subject: [PATCH 10/14] docs: the tree lane was calling a pushed branch unpushed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It said "Unpushed while held" while githubsucks/tree-primitive-framing sat at the same tip as HEAD. Held means no PR is open; it does not mean the work is stranded locally, and the handoff's portability rule cares about the latter. The commit list also claimed to be exhaustive and then excluded the commit that updated it — a list of that shape is wrong the moment it is written. It is now the substantive arc, with `git log` named as the place to get the complete one, and the remote ref rather than a pinned SHA named as the authoritative tip: any edit to this lane advances past whatever SHA the lane records, including this one. Co-Authored-By: Claude Opus 5 --- docs/active-work.md | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index e6fab04..689962d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -169,15 +169,23 @@ are all in place. It is held pending the user's decision to open the PR, not pending further work. - **Branch `tree-primitive-framing`**, base `githubsucks/main` @ - `12f2970`. **Unpushed** while held. Framing + `12f2970`. **Pushed and in sync** — held means no PR is open, not + that the work is stranded locally, so it is portable per the + handoff's rule. `githubsucks/tree-primitive-framing` is the + authoritative tip; `62993e0` was it as of the line above, and any + edit to this lane advances past it, which is why the ref and not a + pinned SHA is the thing to trust. Recover with `git fetch githubsucks + && git checkout tree-primitive-framing`. Framing `docs/tree-primitive-framing.md` **revision 5** — approved after four review rounds, with Q#TR1–TR4 decided. -- **Commits:** `61b1062` framing, `cf4ac1c` rev 2, `932b3ab` rev 3, - `5186bfd` rev 4 (carrying the `COHERENCE.md` §14 call-site - correction), `49a42ec` the primitive, `8f64c3b` byte-identity coverage - plus the verification record, `38e94dc` this lane, §14's ✗ → ◐ and the - handoff facts, `c59de95` the review round — moving-selection test, - flat-panel TAB delegation, §20's build → adopt correction. +- **The substantive arc** (not an exhaustive log — read `git log + 12f2970..githubsucks/tree-primitive-framing` for that): `61b1062` + framing through `5186bfd` rev 4, which carried the `COHERENCE.md` §14 + call-site correction; `49a42ec` the primitive; `8f64c3b` + byte-identity coverage plus the verification record; `38e94dc` this + lane, §14's ✗ → ◐ and the handoff facts; `c59de95` the review round — + moving-selection witness, flat-panel TAB delegation, §20's build → + adopt correction. ### What it ships From f36b1fcf20bf9f09364034d6e36d77d80b6c572f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 17:29:27 +0200 Subject: [PATCH 11/14] docs: the tree lane records PR #217 The ledger requires a lane for every open PR, and this one described a hold that has ended. Co-Authored-By: Claude Opus 5 --- docs/active-work.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 689962d..933c76d 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -161,18 +161,18 @@ form. All four steps ran clean. **The two-argument form still does not work** for a remote-only branch (`fatal: invalid reference`), which is why every lane below spells out the `-b` form. -## Tree primitive (P5) — IMPLEMENTED and GATED, PR HELD +## Tree primitive (P5) — PR #217 OPEN, awaiting review -**Held deliberately, not stalled.** The work is complete, green, and -documented — this lane, the handoff facts, and `COHERENCE.md` §14/§20 -are all in place. It is held pending the user's decision to open the -PR, not pending further work. +**PR #217** — https://github.com/levineuwirth/pmacs/pull/217. It was +held through four framing rounds plus a review round; the hold is over +and the work is with the user. Full gate suite green at the tip, +including `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 221/0. - **Branch `tree-primitive-framing`**, base `githubsucks/main` @ `12f2970`. **Pushed and in sync** — held means no PR is open, not that the work is stranded locally, so it is portable per the handoff's rule. `githubsucks/tree-primitive-framing` is the - authoritative tip; `62993e0` was it as of the line above, and any + authoritative tip; `d92f0ad` was it as of the line above, and any edit to this lane advances past it, which is why the ref and not a pinned SHA is the thing to trust. Recover with `git fetch githubsucks && git checkout tree-primitive-framing`. Framing From 0055a3486762fc19a0b2819670f1bbb3854ca8fb Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 17:46:32 +0200 Subject: [PATCH 12/14] docs: a second local occurrence of the composition-overhead red MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It went red again in the full crdt lib run at the merged tip, this time at 10.3% — clearing the 10% budget by 0.3 points. Ten isolated runs are now green at -2.3% to +1.5%, and two full-suite runs at the same tips were green too. Still recorded as measurements rather than a cause. Two reds against two greens in-suite is intermittence, not a mechanism, and isolated greens reproduce nothing about a load-sensitive failure. The one thing the second occurrence does add is that the budget is marginal rather than comfortably clear. Co-Authored-By: Claude Opus 5 --- docs/agent-handoff.md | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index cf9d3d5..2b3e3c8 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -2079,16 +2079,20 @@ before trusting them: `m6_8_supervisor_reaps_all_children_across_cycles`) are timing-based; `editor::composition_overhead_under_ten_percent` is a render-ratio microbenchmark that fails ~1/3 even isolated single-threaded (already - `cfg!(macos)`-disabled). *(One local measurement, 2026-08-04, 16-core - Linux, tree-primitive branch: it went red inside the full `--features - crdt` lib run at `dispatch overhead 30.7%`, then 5/5 green run alone - at ratios of -0.6% to +0.2%, and the next full-suite run was also - green. Recorded as a measurement, NOT a cause — 5 isolated greens - establish that the ratio is far from the threshold when alone, not - that in-suite contention is what pushed it over. It is also not a - `ci-red-signatures.md` row: that registry judges red **CI** runs, and - this was local. The branch could not reach it — its diff versus `main` - touched no `src/`, no crate, and no manifest.)* Vterm Stage 3's merge CI saw one macOS timeout in + `cfg!(macos)`-disabled). *(Local measurements, 2026-08-05, 16-core + Linux, tree-primitive branch, which could not reach it — its diff + versus `main` touches no `src/`, no crate, no manifest. **Two reds, + both inside the full `--features crdt` lib run**, at `dispatch + overhead` 30.7% and 10.3%; two full-suite runs at the same tips were + green; and **10/10 green run alone**, ratios spanning -2.3% to + +1.5%. So the in-suite/isolated split has now been seen twice, and + the second red cleared the 10% budget by 0.3 points — the threshold + is marginal, not comfortably clear. Recorded as measurements, NOT a + cause: ten isolated greens establish that the ratio sits nowhere near + the threshold when alone, not that in-suite contention is what pushed + it over, and two reds against two greens in-suite is intermittence + rather than a mechanism. Not a `ci-red-signatures.md` row either — + that registry judges red **CI** runs, and these were local.)* Vterm Stage 3's merge CI saw one macOS timeout in `real_tui_terminal_smoke_restores_host_after_output_input_resize_scroll_copy_and_bell`; the complete failed-job rerun passed. The required-GPU gate also failed once in `headless_diag_face_recolors_band_counter_despite_unchanged_text`, then From 7e27de63d876588ff293f4eb24368f047a5cfba0 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 18:06:11 +0200 Subject: [PATCH 13/14] fix(listview): item was load-bearing, and the id contract was two contracts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TWO REVIEW FINDINGS, both real, and neither reachable from the existing tree tests. 1. `item` WAS EFFECTIVELY REQUIRED. `render` writes `line_to_item[n] = row.item`, so that map is SPARSE whenever a row omits the optional `item` — and `seat_cursor` took `#` of it. A display-only tree (a grouping node with `on_visit` unused, which the API explicitly allows) made that length 0, so the cursor never left the header, TAB found no row, and folding was unusable. It now counts visible rows explicitly. The old tests could not catch this because every one of them supplies `item`: under the reverted fix `tr_5` fails `left: 0 / right: 1` while `tr_1` still passes. 2. THE ID CONTRACT WAS TWO CONTRACTS. The docs said "opaque, compared by equality". Selection does compare with `==`, honouring `__eq` — but collapse state stores ids as TABLE KEYS, and Lua indexes tables by raw identity, consulting no metamethod. So a table id would satisfy one half and quietly fail the other: after a refresh minted fresh id tables, the cursor would be restored and the fold silently lost. A divergence that shows up as a missing fold, arbitrarily later, with nothing pointing back at the id. Narrowed rather than generalized. Equality-aware collapse lookup is the alternative and it is worse: `hidden_by_ancestor` runs per row, so it turns a linear render quadratic to support a key type no consumer has asked for. The contract is now the one both halves can honour — string or number, compared by value — enforced by `check_ids` where rows enter (`open` and `refresh`), so a bad id is a named error at the call site instead of a lost fold much later. Q#TR3 in the framing records the narrowing and why. Verified: fmt, clippy, diff-check, --lib 1897/0, crdt 2082/0, listview 24/24, m4 150/0, gpu 221/0. Both fixes bitten independently. Co-Authored-By: Claude Opus 5 --- builtin/runtime/listview.lua | 47 ++++++++++++++++++++--- docs/tree-primitive-framing.md | 2 +- tests/listview_acceptance.rs | 69 ++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 51b07dd..4d6cd00 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -116,8 +116,8 @@ end -- re-asserts the lock, all inside one registry borrow. -- Tree support (docs/tree-primitive-framing.md, Q#TR1-TR4). -- --- A row MAY carry `depth` (0-based, structural) and `id` (opaque, --- consumer-supplied, compared by equality). Both optional: a row +-- A row MAY carry `depth` (0-based, structural) and `id` (a STRING or +-- NUMBER, consumer-supplied, compared by value). Both optional: a row -- without them behaves exactly as before, which is what keeps the -- three flat consumers byte-identical. -- @@ -158,15 +158,46 @@ local function hidden_by_ancestor(p, rows, i) return false end +-- Ids must be scalars, and the reason is not fussiness about types. +-- Selection compares them with `==`, which honours `__eq`; collapse +-- state stores them as TABLE KEYS, and Lua indexes tables by raw +-- identity, consulting no metamethod. A table id would therefore +-- satisfy one and quietly fail the other: after a refresh minted fresh +-- id tables, selection would be restored and the fold would be lost. +-- +-- Equality-aware collapse lookup is the alternative, and it is worse +-- here: `hidden_by_ancestor` runs per row and would turn a linear +-- render quadratic to support a key type no consumer has wanted. So +-- the contract is narrowed to the one both halves can honour, and +-- enforced where rows enter rather than discovered as a lost fold. +local function check_ids(rows) + for i, row in ipairs(rows) do + local k = type(row.id) + if row.id ~= nil and k ~= "string" and k ~= "number" then + error(string.format( + "listview: row %d has a %s id; ids must be a string or number " + .. "(collapse state keys a table by identity, so a %s id would " + .. "lose its fold across a refresh)", i, k, k)) + end + end + return rows +end + local function render(p, rows) local lines = { p.header } + p.visible = 0 p.line_to_item = {} p.line_to_row = {} for i, row in ipairs(rows) do if not hidden_by_ancestor(p, rows, i) then lines[#lines + 1] = row.text + -- SPARSE BY CONSTRUCTION: `item` is optional, and a display-only + -- row (a grouping header in a tree, say) supplies none, so this + -- key is simply absent for it. Nothing may take `#` of this + -- table; `visible` below is the row count. p.line_to_item[#lines - 1] = row.item p.line_to_row[#lines - 1] = row + p.visible = #lines - 1 end end pmacs.buffer.set_generated_contents(p.buffer, table.concat(lines, "\n")) @@ -188,7 +219,11 @@ end -- `switch_active_buffer` zeroes the window cursor, so a fresh switch -- puts us on the header; walk down from there. local function seat_cursor(p, line) - local count = #p.line_to_item + -- `p.visible`, NOT `#p.line_to_item`: that map is sparse whenever a + -- row omits the optional `item`, and `#` on a sparse table is not + -- the row count. Reading it there left a tree of display-only rows + -- with the cursor stranded on the header, where TAB finds no node. + local count = p.visible or 0 if count == 0 then return end local target = math.max(1, math.min(line or 1, count)) for _ = 1, target do @@ -248,7 +283,7 @@ local function ensure_panel(name) local buf = pmacs.buffer.create(actual) p = { requested_name = name, buffer = buf, line_to_item = {}, - line_to_row = {}, collapsed = {}, rows = {} } + line_to_row = {}, collapsed = {}, rows = {}, visible = 0 } panels[#panels + 1] = p -- Read-only (Q#P3): every non-bypass edit is rejected, with a NAMED -- error. Kept beside the rope lock, not replaced by it: the layering @@ -284,7 +319,7 @@ function pmacs.listview.open(spec) -- Keep the row array: collapse re-renders from it WITHOUT calling the -- consumer, which is what lets a panel with no `on_refresh` still -- expand and collapse (the outline has none -- framing §1.5a). - p.rows = spec.rows or {} + p.rows = check_ids(spec.rows or {}) p.collapsed = {} render(p, p.rows) -- Bottom-panel arc (Q#BP11b): the placement opt-in. `seat_cursor` and @@ -332,7 +367,7 @@ pmacs.command.define { -- the row set moves every line; the id survives it. local saved_row = p.line_to_row[saved] local saved_id = saved_row and saved_row.id - local rows = p.on_refresh() or {} + local rows = check_ids(p.on_refresh() or {}) p.rows = rows render(p, rows) -- `set_generated_contents` has already refreshed this window's diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 8343166..7e82093 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -21,7 +21,7 @@ toward the consumer keeping `text`. |---|---| | **Q#TR1** | **Extend `listview`.** A separate `treeview` would either duplicate ~200 lines of panel discipline (Q#GB18 handle identity, Q#GB13 `<2>` disambiguation, the read-only intercept, `prev` capture, quit chain, generated-buffer writes) or require *extracting* them from a shipped primitive first — the riskier change. Extending is backward-compatible by construction: absent `depth`/`id` give today's behaviour exactly, which the three flat consumers already produce. | | **Q#TR2** | **Primitive-owned collapse state**, keyed by row id, held in the panel record. Consumer-owned would make every consumer reimplement refresh survival. | -| **Q#TR3** | **Consumer-supplied `row.id`**, compared by equality; the primitive never derives one. The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | +| **Q#TR3** | **Consumer-supplied `row.id`**, a **string or number**, compared by value; the primitive never derives one. *(Review narrowed this from "opaque": collapse state keys a table, and Lua table indexing ignores `__eq`, so an opaque id could restore selection while silently losing its fold. `check_ids` enforces it where rows enter.)* The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | | **Q#TR4** | **Consumer keeps pre-rendered `text`**; `depth` is structural only. Also sidesteps the conflict with dired's fixed-width `_layout` column contract when it adopts. | **Acceptance 5 is decided too: byte-identity coverage is written**, diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 4c14dcb..84245bf 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -1075,6 +1075,75 @@ fn s1_14_no_bypass_write_or_name_keyed_identity_remains() { ); } +/// A tree row need not carry `item` — `on_visit` is optional, so a +/// display-only node (a grouping header) is a legitimate row. The +/// cursor must still seat on it. +/// +/// This bit: `line_to_item` is SPARSE when rows omit `item`, and +/// `seat_cursor` took `#` of it. For an all-display-only tree that +/// length is 0, so the cursor never left the header — where TAB finds +/// no row and answers "no node here", making the tree unfoldable. +#[test] +fn tr_5_a_tree_of_display_only_rows_is_still_navigable_and_foldable() { + let mut s = editor(); + exec( + &s, + r#"pmacs.listview.open { + name = "*tr5*", + header = "display-only TAB fold", + rows = { + { text = "root", depth = 0, id = "r" }, + { text = " kid", depth = 1, id = "rk" }, + }, + }"#, + ); + + // Seated on a data row, not stranded on the header. + let line: i64 = eval(&s, "return pmacs.editor.cursor_line()"); + assert_eq!( + line, 1, + "cursor seats on the first data row despite no `item`" + ); + + press(&mut s, KeyCode::Tab); + assert!( + !status(&s).contains("no node here"), + "TAB found the node: {}", + status(&s) + ); + assert!( + !active_text(&s).contains("kid"), + "and folded it: {:?}", + active_text(&s) + ); +} + +/// Q#TR3's contract is that ids compare by value. Collapse state keys +/// a Lua table, and table indexing consults no `__eq`, so a non-scalar +/// id would compare equal for selection and unequal for folding: a +/// refresh would restore the cursor and silently lose the fold. The +/// contract is narrowed to scalars and enforced where rows enter, +/// rather than left to surface as a lost fold much later. +#[test] +fn tr_6_a_non_scalar_id_is_rejected_where_rows_enter() { + let s = editor(); + let err: String = eval( + &s, + r#"local ok, e = pcall(function() + pmacs.listview.open { + name = "*tr6*", + header = "h", + rows = { { text = "a", depth = 0, id = {} } }, + } + end) + return tostring(e)"#, + ); + assert!( + err.contains("ids must be a string or number"), + "rejected where rows enter, with a reason: {err}" + ); +} + // Isolated bootstrap storage roots (see the module docs): an // integration test is compiled without `cfg(test)`, so a raw // `EditorState::new()` would read the developer's real `init.lua` and From ef99b64f95ee0d3c06458b28a0204d9413226c19 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 23:06:25 +0200 Subject: [PATCH 14/14] fix(listview): ids must also be unique and not NaN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scalar contract said "identity" and enforced only "scalar", so two ways to hold an id that is not one survived. NaN passes `type(x) == "number"` and then errors at `p.collapsed[row.id]` with "table index is NaN" — the one scalar Lua accepts as a number and refuses as a key. Bitten with the check removed, it reports exactly that, from inside listview, naming no row. DUPLICATES do not merely collide. Every lookup here — `line_of_id`, and toggle's scan for the row index — resolves an id to the FIRST row bearing it, so selecting the second such row toggles the first and re-seats the cursor onto it: a stray jump with nothing pointing at the id. Bitten with the check removed, nothing is raised at all. Both are enforced in `check_ids`, where rows already enter, so the error names the offending row (and, for a duplicate, both of them) instead of surfacing as a low-level error or a wrong jump later. The error text says why, not just what, since the reason is not guessable from the rule. Verified: fmt, clippy, diff-check, --lib 1897/0, crdt 2082/0, listview 26/26, m4 150/0, gpu 221/0, bottom_panel_stage1 47/47. Co-Authored-By: Claude Opus 5 --- builtin/runtime/listview.lua | 58 ++++++++++++++++++++++++++-------- docs/tree-primitive-framing.md | 2 +- tests/listview_acceptance.rs | 52 ++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 14 deletions(-) diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 4d6cd00..a10cf57 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -158,26 +158,58 @@ local function hidden_by_ancestor(p, rows, i) return false end --- Ids must be scalars, and the reason is not fussiness about types. --- Selection compares them with `==`, which honours `__eq`; collapse --- state stores them as TABLE KEYS, and Lua indexes tables by raw --- identity, consulting no metamethod. A table id would therefore +-- Ids must be usable, unique table keys, and none of the three checks +-- below is fussiness about types. +-- +-- SCALAR. Selection compares ids with `==`, which honours `__eq`; +-- collapse state stores them as TABLE KEYS, and Lua indexes tables by +-- raw identity, consulting no metamethod. A table id would therefore -- satisfy one and quietly fail the other: after a refresh minted fresh -- id tables, selection would be restored and the fold would be lost. -- -- Equality-aware collapse lookup is the alternative, and it is worse -- here: `hidden_by_ancestor` runs per row and would turn a linear --- render quadratic to support a key type no consumer has wanted. So --- the contract is narrowed to the one both halves can honour, and --- enforced where rows enter rather than discovered as a lost fold. +-- render quadratic to support a key type no consumer has wanted. +-- +-- NOT NaN. `0/0` passes a `type(x) == "number"` test and then *errors* +-- at `p.collapsed[row.id]` with "table index is NaN" — the one scalar +-- Lua accepts as a number and refuses as a key. Caught here so the +-- report names the row, rather than surfacing on whichever later TAB +-- happens to reach it. +-- +-- UNIQUE. Every lookup here resolves an id to the FIRST row bearing +-- it, so duplicates do not merely collide: selecting the second such +-- row toggles the first and re-seats the cursor onto it. An id that +-- does not identify a node is not an id, and the contract's word for +-- itself is identity. +-- +-- All three are enforced where rows enter, so a bad id is a named +-- error at the call site instead of a lost fold or a stray jump later. local function check_ids(rows) + local seen = {} for i, row in ipairs(rows) do - local k = type(row.id) - if row.id ~= nil and k ~= "string" and k ~= "number" then - error(string.format( - "listview: row %d has a %s id; ids must be a string or number " - .. "(collapse state keys a table by identity, so a %s id would " - .. "lose its fold across a refresh)", i, k, k)) + local id, k = row.id, type(row.id) + if id ~= nil then + if k ~= "string" and k ~= "number" then + error(string.format( + "listview: row %d has a %s id; ids must be a string or number " + .. "(collapse state keys a table by identity, so a %s id would " + .. "lose its fold across a refresh)", i, k, k)) + end + if id ~= id then + error(string.format( + "listview: row %d has a NaN id; NaN is a number but not a " + .. "usable table key, and collapse state would raise " + .. "\"table index is NaN\" on the first fold", i)) + end + if seen[id] then + error(string.format( + "listview: rows %d and %d share the id %q; ids must be unique " + .. "(every lookup resolves to the first match, so selecting " + .. "the later row would toggle and re-seat the earlier one)", + seen[id], i, tostring(id))) + end + seen[id] = i end end return rows diff --git a/docs/tree-primitive-framing.md b/docs/tree-primitive-framing.md index 7e82093..6e58d9b 100644 --- a/docs/tree-primitive-framing.md +++ b/docs/tree-primitive-framing.md @@ -21,7 +21,7 @@ toward the consumer keeping `text`. |---|---| | **Q#TR1** | **Extend `listview`.** A separate `treeview` would either duplicate ~200 lines of panel discipline (Q#GB18 handle identity, Q#GB13 `<2>` disambiguation, the read-only intercept, `prev` capture, quit chain, generated-buffer writes) or require *extracting* them from a shipped primitive first — the riskier change. Extending is backward-compatible by construction: absent `depth`/`id` give today's behaviour exactly, which the three flat consumers already produce. | | **Q#TR2** | **Primitive-owned collapse state**, keyed by row id, held in the panel record. Consumer-owned would make every consumer reimplement refresh survival. | -| **Q#TR3** | **Consumer-supplied `row.id`**, a **string or number**, compared by value; the primitive never derives one. *(Review narrowed this from "opaque": collapse state keys a table, and Lua table indexing ignores `__eq`, so an opaque id could restore selection while silently losing its fold. `check_ids` enforces it where rows enter.)* The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | +| **Q#TR3** | **Consumer-supplied `row.id`**, a **string or number**, compared by value; the primitive never derives one. *(Review narrowed this from "opaque": collapse state keys a table, and Lua table indexing ignores `__eq`, so an opaque id could restore selection while silently losing its fold. `check_ids` enforces it where rows enter — and with it that ids are **unique** and **not NaN**, since every lookup takes the first match (so a duplicate makes selecting the later row toggle the earlier) and `0/0` is the one scalar Lua counts as a number and refuses as a table key.)* The outline uses **`line:col`** — unique per document and stable across re-render, where the `::` parent chain collides on overloads. | | **Q#TR4** | **Consumer keeps pre-rendered `text`**; `depth` is structural only. Also sidesteps the conflict with dired's fixed-width `_layout` column contract when it adopts. | **Acceptance 5 is decided too: byte-identity coverage is written**, diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 84245bf..e845fce 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -1144,6 +1144,58 @@ fn tr_6_a_non_scalar_id_is_rejected_where_rows_enter() { ); } +/// A NaN id passes `type(x) == "number"` and then errors at +/// `p.collapsed[row.id]` with "table index is NaN" — the one scalar +/// Lua accepts as a number and refuses as a table key. It must be +/// caught where rows enter, naming the row, rather than surfacing on +/// whichever later TAB happens to reach it. +#[test] +fn tr_7_a_nan_id_is_rejected_rather_than_erroring_on_the_first_fold() { + let s = editor(); + let err: String = eval( + &s, + r#"local ok, e = pcall(function() + pmacs.listview.open { + name = "*tr7*", + header = "h", + rows = { { text = "a", depth = 0, id = 0 / 0 } }, + } + end) + return tostring(e)"#, + ); + assert!( + err.contains("NaN id"), + "named at entry, not as a table-index error later: {err}" + ); +} + +/// Duplicate ids do not merely collide — every lookup resolves an id to +/// the FIRST row bearing it, so selecting the second toggles the first +/// and re-seats the cursor onto it. An id that does not identify a node +/// is not an id. +#[test] +fn tr_8_duplicate_ids_are_rejected_because_lookup_takes_the_first_match() { + let s = editor(); + let err: String = eval( + &s, + r#"local ok, e = pcall(function() + pmacs.listview.open { + name = "*tr8*", + header = "h", + rows = { + { text = "first", depth = 0, id = "same" }, + { text = "second", depth = 0, id = "same" }, + }, + } + end) + return tostring(e)"#, + ); + assert!( + err.contains("share the id") && err.contains("rows 1 and 2"), + "both offending rows named: {err}" + ); +} + // Isolated bootstrap storage roots (see the module docs): an // integration test is compiled without `cfg(test)`, so a raw // `EditorState::new()` would read the developer's real `init.lua` and