36ade35240c2cdf49cd9a65415f75b9ce3bdbb63
978 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Edward Langley
|
fb2122f23d |
fix: commit the high-water mark unconditionally, coalesce groups by name
Copilot's sixth pass; both correct, and the second is a front-end divergence of exactly the kind this PR exists to remove. **The high-water mark was committed only when the extent changed.** A group count that rose while a header line fell by the same amount left `next.rows` equal, so the larger count was discarded and a later repaint under-reserved — the DOM-scroll bug again. `frameCost` is now assigned unconditionally and only `sendViewport()` is gated on the extent moving. **`col_groups` started a new run when `collapsed` differed; the TUI does not.** `src/ui/grid.rs` breaks a run on `group_name != prev_group` alone. Collapse is category-scoped, so two adjacent groups from different categories may legally share a name and differ in it, and the browser would then have drawn a second indicator the TUI suppresses. Coalesced by name, with the run taking its first group's indicator, as the TUI paints it. `column_groups_coalesce_by_name_as_the_tui_does` pins it; restoring the `(name, collapsed)` key splits the span and fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
affb9a1812 |
fix(harness): make the two expected-failures deterministic, and unstale 4 docs
The browser job **failed in CI** on the previous head while passing locally, with both `test.fail` tests reporting "expected to fail, but passed". That is the worst possible outcome for an expected-failure: it is a check that is green when the bug is present and red when it is absent, so a flaky one inverts CI's meaning rather than merely being noisy. Copilot independently flagged the same test from a different direction on the same head. Two timing assumptions, both wrong off my machine: **A closed port is not a deterministic "refused before open".** Port 9's `close` timing is a property of the host's network stack, and on the GitHub runner it did not arrive inside the window, so `#status` still read "cannot reach" and the assertion passed. Replaced with the harness's **own http server**: it accepts the TCP connection and then fails the WebSocket upgrade, so `error` and `close` both fire promptly, everywhere. The page cannot tell the two apart and neither can `web/app.js` — in both cases the socket never opened. **A timer does not order a close after an open.** The watchdog test's mock closed 300ms in; on a slow host that can land before the page sees `open`, in which case `armWatchdog` never runs and the test passes having exercised nothing. Both mocks now close on the page's **first frame**, which the page sends only after `transport.ready` resolves — so "opened, then dropped" is true by construction rather than by luck. And the setup assertions now assert the **precondition of the bug** rather than that something happened: the m18f test polls the recorded `#status` history for "cannot reach" before enabling `test.fail`, because a bare non-empty check is already satisfied by "connecting to …" (Copilot's exact point). Verified three ways: `--repeat-each=3` green; breaking `paintStatus` gives 2 real failures, not expected ones; simulating PR #21's fix still flips the m18f guard to "expected to fail, but passed". **Four checked-in explanations this branch made false**, also Copilot's, and gotcha 48's third rule for the second time in this PR — when a fact is stated in more than one place, changing it is a grep, not an edit: - `worker-server/src/lib.rs` said `storage_contract` "has still never been run" against `OpfsFs` and that browser verification is deferred. - `bindings.rs` said it is "the only part of this crate that no test in the repository can reach". Still true of the shim, for a narrower reason now worth stating: the harness drives `OpfsFs` directly and never instantiates it. - `vfs.rs::holds_a_document` and its test both said `attach` indexes the zero-byte entry it creates. It no longer does, which is the whole of the fix three commits ago — so the check is now belt and braces rather than the mechanism, and says so. `web/app.js` and `web/worker.js` carry the same stale "untested JavaScript" claim and are **not this unit's files**; reported for the orchestrator rather than edited across the boundary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
d8a090288b |
fix(fit): round four — a panic, a split status bar, and the question restated
Four of five fixed. The fifth is the most valuable thing in the review.
**A fitted rule means something different in an aggregated cell, and that is
improvise-4zl7's blocking question in its general form.** A tree is fitted on
individual *records*; an improvise formula is evaluated at a `CellKey`, and in a
cell that aggregates over a hidden category `evaluate_aggregated` aggregates
each `Expr::Ref` **first** and then evaluates the `IF`. Linear formulas survive
that — `SUM(Revenue) - SUM(Cost)` is `SUM(Revenue - Cost)` — and a tree does
not: aggregate-then-route is not route-then-aggregate. 4zl7 asks what a
*categorical* split means where its coordinate is aggregated away; this is the
same question for **every** fitted tree, numeric splits included, and it makes
the bead larger and more clearly blocking rather than smaller.
Characterized rather than answered, because answering it is the user's call.
Worth recording what the fixture actually does: the aggregated cell comes back
`Error("ref:Cost")`, not a wrongly-routed number — the loud failure this repo
prefers, and *not* something to rely on, since nothing states it. The assertion
is therefore the negative (it is not the sum of the per-record predictions), so
it covers both outcomes and fails the day aggregation becomes record-wise.
**An infinite target panicked out of a function that returns `Result`.** A
`.improv` cell holding `inf` parses as an `f64`; `standardize` turns an infinite
target into NaN leaves and `number_text` asserts. Verified by disabling the new
guard and watching `number_text wants a finite value, got inf`. The two
directions differ and the doc says so: an infinite *feature* poisons its range
and quietly yields a constant leaf instead. A panic and a meaningless rule, both
refused now. `NaN` needs no arm — `is_missing` already spells `"NaN"` as
missing, so it is imputed away.
**Only the success line was flattened**; refusals interpolate document names
too, and a name may legally contain a newline — so a failed fit could split the
status bar, which is when a user is most reading it. `one_line` is public and
the command's error path uses it.
**"a constant — no feature matters" overclaimed.** An empty importance ranking
establishes that no shuffle raised the error, which a constant tree satisfies
and so does a tree whose splits do not matter on these rows. It now reports what
the data established: "nothing measurable — no feature changed the error".
**Declined:** the record budget runs after `feature_table` has already grouped
and sorted, and `RecordRow::group` linear-scans (`improvise-core/src/view/
layout.rs:43`) so grouping is O(cells x rows). Correct — but that cost is
pre-existing and shared with records mode, it is in `improvise-core` which is
outside this unit's boundary, and threading a limit into `feature_table` changes
a signature `dataset()` and its tests share. Filed as an indexed-grouping ticket.
1230 passing (+2); clippy, fmt and the wasm gate clean; CLI byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
c9a4961d12 |
fix(command): the document-mutation classifier is exhaustive
Copilot's fourth round. The substantive one is in its summary rather than in a thread: `mutates_the_document` was a `matches!`, so a **future** `Command` variant would answer `false` in silence — and silence is precisely the failure mode here, since an uncounted document mutation walks straight past the atomicity guard and is applied against a document a sibling has already changed. That is the same shape as the three bugs this branch has already been caught by: a rule stated over the variants that came to mind. It is now an exhaustive `match`, like `records_named`, so a new variant fails the build until it says which it is. **Verified by adding one**: a probe variant on `Command` fails to compile at this function, not merely in the protocol crate's own fold. Three documentation findings from the same round, all correct: - A stray `**` left an unmatched emphasis span that broke the rustdoc for the rest of the paragraph — in the doc explaining the rule, which is the one people will read. - `Rejection`'s "the client re-composes against it" is true of the staleness class only. A failed save is a fact about the host and a frame carrying two mutations is answered by splitting it; neither has a fresher basis to re-compose against. Now scoped. - The end-to-end test's own doc described the rule as counting `Command::Model`, one commit after it started counting `SwitchView` too — so the test that protects the contract mis-stated it. 1208 passing; clippy, fmt and the wasm gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
ff351ee014 |
fix: reserve the group rows monotonically, and bulk the ancestry compare
Copilot's fifth pass; both correct. **Inline group rows are back in `frameCost`.** Omitting them was the same failure I had just fixed for the header block: they take real table height, so any viewport containing one asked for more data rows than `#grid` could show, and the DOM takes its own scroll position while the client's cursor offsets come from the larger window. Copilot's own suggestion answers the oscillation objection — the reservation is a **high-water mark**, which cannot oscillate because it only rises, and it rises in the safe direction: over-reserving leaves blank space, under-reserving is the bug. It resets on a view change so one dense document does not make every later view pessimistic. **`same_prefix` is replaced by `AxisFrame::shares_ancestry`**, the bulk form, for the same reason `header_labels` and `groups` are bulk: called per (column, level) it scanned the frame twice apiece, quadratic in the columns drawn and growing with the horizontal offset, where the pre-anr9 widget's materialized lookup was O(1). `same_prefix` is **deleted** rather than kept beside it — two answers to one question is the thing this PR exists to remove, and keeping a per-position form that nothing calls would be an invitation. `shared_ancestry_widens_as_ the_level_rises` checks both arms, because a version comparing whole tuples passes the leaf arm and a version comparing only the outermost label passes the outer one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
98e0c59ab9 |
chore(beads): close WU-L's four beads, correct improvise-1ey, file the date limit
improvise-odbc, improvise-7yx, improvise-g6s1 and improvise-q0vb closed by PR #22, with the reasoning on each. improvise-odbc's close records that the ticket's premise was FALSE and that testing it is what made the ticket cheap: quote_name mirrors bare_name character for character, so the round-trip property was complete and the generator needed nothing. The file had been hand-damaged through the v2025-04-09 migration. improvise-1ey's design field named bank-info.improv, which PR #22 deleted. roadmap.org:194 is generated from that field, so the fix belongs at the bd source and the file is regenerated — Copilot found the stale line and WU-L correctly declined to hand-edit a generated file. improvise-l8rh filed: the ambiguous-date heuristic reads American-first, so a European CSV imports with days and months swapped. Pinned rather than changed on the user's ruling — but it is now wrong DETERMINISTICALLY, which PR #22's HashSet fix is what bought. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
359c157774 |
chore: delete foo.improv, the second unloadable tracked document
Sibling of bank-info.improv, deleted in PR #22. Both were force-added past .gitignore's `*.improv`, and both are hand-written fakes with no generator. foo.improv is worse: it has no version line at all, so it cannot parse under any grammar version. Note tests/shipped_documents_load.rs walks examples/ and web/ — the two directories .gitignore un-ignores — so neither root file was ever covered by it. Deleting them is what keeps that true rather than an omission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Ed Langley
|
7350a772e7 |
Merge pull request #22 from fiddlerwoaroof/wave/l-import-and-tests
WU-L: import, persistence and the test gaps (improvise-odbc, improvise-7yx, improvise-g6s1, improvise-q0vb). Includes a determinism fix nobody asked for: analyze_records iterated a HashSet, so the same CSV imported twice produced two different documents and sometimes a different detected date format. |
||
|
Edward Langley
|
a17ec9efac |
fix: two Copilot findings — a stale column index, and a false negative arm
**A schema change shifted the grid under its own advance.** `add_category` inserts the new coordinate column immediately before `+cat`, so every index from `+cat` rightwards moves by one — while `CommitAndAdvance` computed its advance from `ctx`, the *pre-commit* layout (gotcha 23). On the last row `Enter` wraps to `col + 1`, which after the insert is `+cat` again. The failure is not "lands one column off": it **reopens category creation**, so a user naming two categories in a row never reaches the column they made and gets an unbounded `+cat` loop. `Tab` reached it the same way. A schema change that inserts a column now does not advance at all, which is the useful answer as well as the safe one — the insert happens exactly where the cursor is, so staying put lands on the new column. The measure `+` shares none of this and not by luck: it is the last column, so there is no index to its right to shift into, and both advances already special-case the bottom-right edge. `inserts_a_column` asks the *layout* what the column is rather than re-deriving it from the value, so gotcha 1 keeps one answer. Both arms were watched failing first, and both now assert the column landed on rather than merely "not `+cat`" — a `assert_ne` alone would pass anywhere. **And a negative arm that was asserting something false.** `a_prompt_this_client_cannot_commit_is_never_entered` used `:` to show that `strands_the_user` had not degenerated into "refuse every mode". But `:` is itself a dead room: it opens locally and types locally, while Enter is `[execute-command, clear-buffer]` and `step` has no `execute-command` arm, so the frame is dropped. The arm was **certifying the behaviour it existed to rule out**, which is worse than having no arm at all. `CommandMode` therefore joins the predicate. It costs no working surface — every ex-command behind `:` is inert for the same single reason — and the distinction from the two prompts is kept visible because it says who removes the line: they are dead *by construction* (no `ModelCommand` can name `Save`/`SaveAs`/`ExportCsv`), this is dead *by omission*, and improvise-e24v is scheduled to implement `execute-command` client-side. Listing it is therefore also the named gap gotcha 47 asks for: closing e24v has to come to this line and delete it, which a comment could not force. `the_command_line_cannot_be_committed_by_this_client` is the evidence rather than the assertion. The arm itself now has **two halves**, and the first version of the rewrite had neither — it pressed Enter in `Editing`, which never consults `strands_the_user`, so "refuse every mode" still passed. Half one enters a mode through `enter-mode` and so exercises the predicate; half two presses Enter and requires a real `Command::Model` back, because the property is finishing rather than entering. `?` is honest as half one where `:` was not: Help has no commit, so nothing typed into it can be swallowed. Proved in both directions: "refuse everything" fails half one, "refuse nothing" fails the positive arms and the named-gap list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
cded59acb7 |
docs: the rule widened and its public statement did not follow
`ClientFrame::Commands` — the doc a third transport reads first — still said "at most one `Command::Model`" after the rule became "at most one document mutation", which understates it by exactly the case the previous commit was fixing, and would have led a new client straight into it. Found by grepping the rule's own wording rather than by a test, which is gotcha 48's third paragraph for the third time on this branch: when a fact is stated in more than one place, changing it is a grep, not an edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
436faf8d1c |
fix: SwitchView is a document mutation, and a save frame is not retried
Copilot's third round of PR #26, and the first finding is the **third** time the same mistake has been caught here: an argument that holds for the command that came to mind. **1. `SwitchView` counts.** I excluded it on the grounds that it carries no basis and a record surrogate is view-independent, so a switch cannot invalidate an edit beside it. That is true of `SetCell` and `ClearCell` — and false of *every other* model command, because `TransposeAxes`, `SetAxis`, `HideItem`, `SetNumberFormat` and their siblings all act on the **active view**. So `[SwitchView("Second"), TransposeAxes]` passed both checks and then transposed `Second` rather than the view the transpose was composed against. The rule is now "at most one **document mutation** per frame", where `mutates_the_document` is the two `Command` variants `Sessions::apply` routes away from the mirror. Deliberately the *document* question rather than "does it carry a basis", which is exactly the distinction I got wrong. No client here batches a `SwitchView` with anything, so it costs nothing. **2. An explicit `ClientFrame::Save` is not also auto-persisted.** A failed write leaves `dirty` set — deliberately, so the next frame retries — and the worker's automatic trigger runs after *every* frame. A save frame therefore attempted the same write twice. On a permanent failure that is duplicate I/O; on a transient one it is worse, because the answer was composed from the first attempt, so the client was told `SaveFailed` about a document that was now on disk. A per-frame question is the right shape here precisely because the automatic write exists for edits **nobody asked** to save. **3.** `Rejection`'s summary line now says it answers a *frame*, since two of its three classes are not refused commands at all. Evidence, both faults injected: - `a_frame_carrying_two_document_mutations_is_refused_whole` gains a third shape. Dropping `SwitchView` from `mutates_the_document` fails it. - `an_explicit_save_is_not_retried_by_the_automatic_write` asserts the **number of write attempts**, which took a counting, always-failing `Storage`. Both properties are load-bearing: "it was refused" is true whether the write happened once or twice, and a *successful* save clears `dirty` so the redundant attempt never happens — the bug is invisible from both the refusal and the happy path. Restoring the retry gives 2 where the test requires 1. `a_refused_batch_leaves_the_commands_before_it_unapplied` now leads with `SetSelected` rather than `SwitchView`, which the new rule would refuse first. That is not a concession: a view command is the shape a real frame has, and it is still observable in the asker's mirror. 1208 passing; clippy, fmt and the wasm gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
3c36a3b57f |
test(harness): Firefox runs the contract too, and test.fail stops hiding setup
Two Copilot findings on PR #24, both correct, both applied. **`test.fail` was masking setup failures.** Applied before navigation, it made any earlier failure — the page not loading, the script not starting, `#status` never updating — an *expected* failure, so CI stayed green through it. That is precisely the check-the-checker defect these tests exist to catch, wearing the name of its cure. Both tests now assert their setup first and apply `test.fail` immediately before the one assertion the known bug owns. Verified by breaking the setup (making `paintStatus` a no-op): before, 2 passed; after, **2 failed**. **Firefox now runs the whole battery, and it passes.** improvise-xf1's acceptance criterion names Chrome, Firefox and Safari, and the reason is real rather than ceremonial: `createSyncAccessHandle` is the one API this design rests on (gotcha 28) and each engine implemented it separately, so Chromium alone was one engine's opinion. `storage_contract` against `OpfsFs` now passes on **two** engines. **Safari is not covered, and the criterion is met for two of three.** Playwright's WebKit build provides no OPFS at all — `navigator.storage.getDirectory()` itself rejects with `UnknownError` — so there is nothing there for `OpfsFs` to be right or wrong about. Rather than delete the project and quietly narrow the claim, it is kept and the spec skips on a **runtime probe**: the day that build gains OPFS the checks start running with no CI change. The skip is conditional on the probe and not on the browser name, and an unexpected skip on any other engine **fails** with "that is a regression, not a known gap" — a silently skipped check being the failure this harness exists to prevent. The shell spec stays Chromium-only on purpose: it is about `web/app.js`'s own logic, which is engine-independent, and running `routeWebSocket` plus the worker handshake three times is slower and flakier to no benefit. 14 passing, 3 skipped, 36s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Ed Langley
|
90ec5c3fc8 |
Merge pull request #28 from fiddlerwoaroof/chore/orch-beads
chore(beads): the orchestrator pass — closures, deferrals, and the ADR 003 rewrite |
||
|
Edward Langley
|
a1ee203b00 | Merge main into wave/g-command-layer (PR #21 browser shell, PR #20) | ||
|
Edward Langley
|
9aef855d5f |
fix: the viewport reserves the real frame, and a fallback width counts cells
Copilot's two "previously missed" findings, both correct. **`extent()` reserved one header row and one header column.** That was right until this PR: the column header is now one line per level plus an optional group line, and the row header is `row_levels` columns wide. Announcing a viewport the page cannot show matters because `#grid` has `overflow: auto` — the DOM would take a scroll position of its own while the client's cursor offsets were computed from the larger window, putting the selected cell off screen. The cost is measured off the painted model and re-announced when it changes. Inline group *rows* are deliberately **not** counted, although they occupy space: they depend on how many rows the window asked for, so counting them would be a feedback loop — a smaller window crosses fewer groups and asks for a larger one. The three facts that are counted depend only on the axes. The residue is a little over-fetch, the direction `ROW_PX` already errs in. **`fit_to_labels` measured its fallback width in Unicode scalars.** It is documented as the width a column is drawn at while `ColWidths` catches up, and the authoritative widths are in display cells, so `日本` came out two cells wide where it needs four. Now measured with `UnicodeWidthStr`, like `header_widths` and like the TUI. `a_column_without_its_width_yet_is_measured_in_display_cells` pins it, and restoring `chars().count()` fails it with `[2]` against `[4]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
bd3f89a74c |
chore(beads): close the four WU-I beads with their reasoning
improvise-oai9 closed on evidence with no code written — the report() helper
was already in the tree, and the fault was injected ({e:#} → {e}) before
closing rather than trusting the description.
improvise-r4lo, improvise-taxx and improvise-o8fa closed by PR #21. Two of the
three record that the TICKET was wrong rather than merely that the bug is
fixed: r4lo's offered escape hatch does not exist, and taxx's proposed
readyState mechanism cannot work because both cases arrive as code 1006 with
wasClean:false.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
553b2cb90d |
chore(beads): file the CSWSH hole, the records-mode minibuffer exit, and route vvl3's assertions
From WU-I (PR #21) and WU-G (PR #27) hand-backs. Searched the open backlog first; none of these duplicates an existing bead. improvise-kc7n P2 — improvise-ws-server never checks Origin. Verified independently: `grep -rni origin crates/improvise-ws-server/src/` returns nothing. A WebSocket handshake is exempt from the same-origin policy, so any page an allowed user visits can drive their session from their own allowed address — the attack an IP allow-list structurally cannot see, because the request genuinely is from an allowed client. Loopback is not exempt and has no proxy in front of it, so the local path is the least protected. Related to improvise-kpnr rather than blocked by it: authentication and CSWSH are different questions and an answer to either leaves the other open. improvise-sihr P2 — every minibuffer mode entered from records mode exits to plain Normal while the layout is still records. Affects `:`, Ctrl-E and Ctrl-W; only the last is new. Declined in PR #27 because fixing one arm of a three-arm rule is not a fix, and characterized there instead. improvise-vvl3 also gains the four assertions improvise-taxx's fix owes, so that ticket need not re-derive them — including the one that must fail against the previous unconditional close handler, which is the guard and not the decoration. Recorded there because vvl3 now owns the JS test-infrastructure decision that PR #21 declined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
f45240c2e9 | Merge main into chore/orch-beads | ||
|
Ed Langley
|
e49da85184 |
Merge pull request #21 from fiddlerwoaroof/wave/i-browser-shell
WU-I: browser shell, worker diagnostics, and the transport story (improvise-oai9, improvise-r4lo, improvise-taxx, improvise-o8fa). |
||
|
Edward Langley
|
f535fba2f7 |
fix(fit): round three — two bugs in my own new guards, and a correction
Copilot's third pass. Four fixed, one declined, and two of the four were defects in the checks the *previous* commit added. **The role check read positions where positions do not hold.** `preprocess` `continue`s past an entirely-missing column **without pushing a `ColumnSpec`**, so `Dataset::columns` is not index-aligned with the header once anything is dropped: one empty category shifts every later spec left, and the positional `i >= category_columns` test then read the surviving numeric measure as a category and refused a perfectly good fit. The role now comes from the *header* — which `feature_table` built and which never shifts — and the plan is consulted by name. Proved by re-injecting the positional form and watching `an_empty_category_before_a_measure_does_not_break_the_role_check` fail alone. **And the collision test never built the collision.** Adding a category named `Cost` with items puts no `Cost` coordinate on the existing records, so `feature_table` emitted an empty column, `preprocess` dropped it, and there was no duplicate name to find — the test passed on the positional bug instead. The records carry the coordinate now, the fixture asserts the header really has two `Cost` columns before fitting, and the assertion names the ambiguity diagnostic so it fails if that check is removed. Two findings, one root: a guard written against a premise nobody checked. **`encoded_width` treated every column as categorical**, so a numeric measure with 65 distinct values was refused as 65 features, while a date or a column with missing cells could expand past the estimate. It now approximates `preprocess`'s inference, and — the part that matters — the **exact** width check is restored after preprocessing. The preflight is advisory, and says so: its only job is to refuse a rows × rows one-hot before the memory is spent, and a drift in either direction is bounded by the exact check behind it. **Nothing bounded the work by record count.** `MAX_FEATURES` bounds the width; every stage scales with rows on the thread that draws the frame. `MAX_RECORDS` (20,000 — about a second at the measured 105 ms/2,000, roughly linear) is the default of a new `FitRequest::max_records`, so a host that has somewhere else to run a fit can raise it rather than being told no. **Declined, and it corrects something I got wrong.** `Spec::ranges` is computed over all rows and the same spec goes into every training fold, so the thresholds the search samples and the branches `simplify` prunes are bounded by a held-out extremum — which reaches a fit over *complete numeric measures* too. My earlier reply on the imputation thread said "the case that currently stores leaks nothing"; that was wrong, and the doc comment on `Quality` now says so. Still declined here for the same reason: closing it needs a `cv_regress` that takes a raw table and a plan-builder, which is a `gp-tree` API change and the same one improvise-4zl7 will want. 1228 passing (+3); clippy, fmt and the wasm gate clean; CLI output still byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
44295940ad |
test(persistence): corrupt the gzip CRC, so the checksum test tests the checksum
Copilot's fourth finding on PR #22, and it was right about more than it claimed. `a_gzip_document_with_a_flipped_byte_fails_its_checksum` flipped a byte at `len / 2`, inside the DEFLATE payload, and asserted only `ErrorKind::InvalidInput` — which a *malformed compressed stream* also produces. The name was therefore a guess about which of two failures the arbitrary index happened to hit. Probed it before changing anything: flipping any byte in 10..40 gives `corrupt deflate stream` **thirty times out of thirty** and a checksum error never. `len / 2` happened to land somewhere that fails the checksum, so the test had been passing for a reason nobody chose. Now the flip is in the CRC32 field — the first four of the eight-byte gzip trailer — which leaves the compressed stream decodable, so the checksum is the only thing left to fail. And since both failures wear `InvalidInput`, the kind cannot separate them: the cause's own message must say `checksum`. That is asserted on the source error rather than on the `anyhow::Error`, because anyhow's `to_string` shows the outermost context, which is the sentence naming the document. Watched failing with the flip moved back into the payload (`got corrupt deflate stream`), and the sibling truncation test is untouched and still `UnexpectedEof`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
f91c5a77db |
docs: an allow-list is access control, not authentication
Copilot's seventh pass, reporting no new open findings; this is the one carried item that is a real defect and it is cheap. The closing note called the IP and Origin allow-lists "the auth above", in a section whose opening sentence is that the server has NO AUTHENTICATION OF ANY KIND. That muddies the exact distinction the section is built on: an allow-list answers WHICH CLIENTS AND WHICH ORIGINS MAY CONNECT, never WHO is connecting. Calling it auth invites a reader to think the hole is closed when what has been added is a perimeter. Checked every other use of the word in the section rather than editing the one line flagged, per the grep rule this PR earned: "no authentication of any kind" (the server), "HTTP basic auth" (genuinely authentication, and being ruled out), "neither authenticates anything by default" (the tunnels) and `--basic-auth` (a product flag) are all correct as written. One site, one fix. The distinction is also exactly what improvise-kpnr has to decide, so the note now says so. 1193 passing, unchanged; no code touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
16ba617b27 |
test(browser): a group header is not a grid row, and the write must not shift
Copilot's fourth pass, and the same coordinate confusion as the `app.js` one — this time in a test helper, where it is worse: `first_editable` returned the index of a row in `RenderModel::rows`, which counts group headers, and handed it to `Client::typed_cell`, whose row argument does not. Both are `usize`, so nothing complained, and the fixture had no groups so the two coincided. `FirstEditable` now carries **both** numbers and each call site takes the one it means. `a_grouped_row_axis_does_not_shift_the_write` is the fixture that separates them: `West` ungrouped and `East` under `Coastal`, so the header falls *between* the rows and the second row is visual 2, grid 1. A header *before* the first row would not have done — the frame drops it (`AxisFrame::body_from`) and the two coincide again, which is the trap this test exists to avoid falling into itself. Injected: passing `visual_row` to `typed_cell` leaves `East` unwritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
2ae6a42918 |
test(browser): the real batch producer must still be accepted end to end
The previous commit made `CheckedBatch::against` refuse a frame carrying more than one `Command::Model`. Refusing is the easy half to get right. The expensive half is not tightening the rule until the browser silently stops working — and **nothing in the workspace would have noticed**, because the only real producer of a multi-command frame is `improvise-wasm-client::keys::commit`, which no test in `improvise-command` can reach. A commit keypress is exactly the shape at risk: one `SetCell` plus the mode change, the scroll offsets and the re-seeded buffer around it. So this drives a **real keypress** through a **real `Host`** in the end-to-end test that already exists for that seam, and asserts three things rather than one: - the frame really is the shape under test — several commands, exactly one a mutation — because if `commit` ever stopped batching, a test that only checked the outcome would keep passing while testing nothing; - nothing in the answer is a `ServerFrame::Rejected`; - **and the cell actually moved**, since "not refused" alone would pass against a host that dropped the frame on the floor. The expected value is the *appended* one. `i` seeds the edit buffer from the cell it lands on, exactly as the TUI does, so typing `888` over `1200` gives `1200888` — and asserting that rather than `888` is what makes this a round trip rather than a coincidence, since `888` would also be produced by a client that ignored the seed. Proved by injecting the over-tightening it exists to catch: narrowing the rule to "any mutation in a multi-command frame" fails this test and nothing else in the suite. 1207 passing; clippy, fmt and the wasm gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
f90d1757be |
test(harness): the four assertions the close-listener fix owes, and a third bug
The harness is the one place a line of `web/*.js` is executed, not only the one place `storage_contract` reaches `OpfsFs`. PR #21 declined Copilot's request for regression tests on its close-listener fix because there was no JavaScript test infrastructure in this repository at all — no `package.json`, no runner, nothing in CI running a line of `web/app.js` — and standing up a parallel one beside this harness would have been two answers to one question. That decline is only honest if the assertions land here, so here they are, and `playwright.config.js` now says out loud that a change to `web/*.js` belongs in this spec directory. Playwright rather than a headless-DOM runner, because what goes wrong in these files is a real Worker, a real WebSocket, real OPFS and real event ordering — none of which a DOM shim has. `close` fires on a connection that never opened and on one that was established and dropped, and nothing on the event tells them apart: `readyState` is already CLOSED in any close listener, and both arrive as 1006 with `wasClean: false`. So: 1. **close-before-open must leave "cannot reach" standing.** `test.fail`, because the close handler on this branch is unconditional and overwrites it (improvise-m18f). That IS the regression guard, and it is aimed at the implementation actually here rather than at a mutant invented for the occasion — against an unconditional handler, "always say cannot reach" would pass and prove nothing. Verified by simulating PR #21's `opened` flag in the served copy: the test passes and Playwright reports "expected to fail, but passed", so the fix cannot land silently either. 2. **open-then-close must read "disconnected".** Green under both implementations — verified under the simulated fix too, since this is the arm that makes suppressing `close` wrong. 3. **The watchdog must stay out of the never-opened path.** Green, but for a reason its own comment does not lead you to expect: `armWatchdog` runs only after `transport.ready` resolves, which happens on `open`, so on a socket that never opened the watchdog is never armed at all. Asserted on the same page load as the "cannot reach" check, since both need the same 9-second wait. 4. **The watchdog must not overwrite a mid-session disconnect.** It does. Found by measuring rather than predicted, and it is a **third** bug: on the open-then-close arm the watchdog is armed, nothing disarms it, and 8s later "disconnected" is replaced by "accepted the connection but sent no document" — which is false, the peer connected and then went away. Same class as the bug `setShellFailure` fixed (a watchdog must be disarmed, not pre-empted) and it **survives** improvise-m18f's fix, which is about which message `close` writes rather than about the timer still running — confirmed under the simulated fix, where this one still fails. `test.fail` and filed; `web/app.js` is not this unit's file. Measured before asserting, which is how 3 and 4 came out opposite to the expectation they were handed: never-opened: connecting… -> cannot reach -> disconnected open-then-close: connecting… -> connected -> disconnected -> accepted the connection but sent no document 11 passing in the harness, 32s. The two `test.fail` entries are expected failures, so the suite is green and both bugs are pinned in a form that goes red the moment either is fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
858a46e1a8 |
docs: qualify the loopback claim, in both directions
Copilot's sixth pass, and it is my own round-4 failure repeating: I wrote that "loopback is not a boundary a browser enforces" and that ANY open page can reach ws://127.0.0.1 -- an absolute where the evidence supports a qualified claim. Same shape as asserting a WS handshake carries no credential. CHECKED RATHER THAN TAKEN, and the finding is itself overstated. Copilot says "current Chromium gates WebSocket access from a public origin to localhost behind Local Network Access". Chrome's own documentation (developer.chrome.com /blog/local-network-access) says the opposite for the case at hand: WEBSOCKETS WERE NOT YET GATED when the permission prompt began shipping, with coverage planned. So the mitigation exists and is rolling out; whether it covers THIS socket, in a given browser at a given version, is exactly what neither of us can state and what no test here can settle. The text now says that, and keeps two gaps that are structural rather than version-dependent: LNA is a Chromium feature, so other browsers need not behave alike; and it engages only for a PUBLIC origin, so the locally-served page this paragraph is about falls outside it entirely. The threat is kept, the reassurance is not offered, and the durable fix is named -- the server checking Origin, which it does not do. Grepped the same claim through the rest of the section, per the rule this PR earned: the two remaining "any hostile page" statements are about the REMOTE wss:// endpoint, which LNA does not gate at all (it gates public->local), so they are correct unqualified and were left alone. The distinction is the whole point of the fix. 1193 passing, unchanged; no code touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
120286a44b |
fix(command): a frame carries at most one document mutation
Copilot's second review of PR #26 found that the previous commit fixed the **example** and not the **rule**, and it is right. Refusing only the document-*replacing* case left the general one open, and the general one is worse because it is silent: [RemoveItem("North"), SetCell(the North record)] Both commands pass the pre-frame check. `RemoveItem` applies and deletes the record. `model_effects` then resolves the already-answered `SetCell` against the mutated document, finds nothing, and returns an **empty effect batch**. The frame is reported as accepted having applied only its first command, and the client believes its edit landed. Worse, that is a **regression the batch check introduced**: under the old per-command check the `SetCell` would have been refused with `Rejection::RecordGone`, which is the correct answer. So the rule is now the count rather than a list of dangerous variants: **a frame may carry at most one `Command::Model`**, refused with `Rejection::FrameNotAtomic` (which replaces `DocumentReplacedMidFrame`, and `replaces_the_document` goes with it). **This costs nothing real, and that is the argument for it rather than an excuse.** Two model mutations in one frame could never both be honestly based: `BasisCache::claim` hands back what the server last *published*, and the server publishes between batches and never during one, so the second command's claim necessarily describes a document that stops existing when the first lands. Refusing the pair is that incoherence said out loud. The batch shape that actually exists — `SetSelected` plus the offsets `viewport_effects` returns, optionally around one edit — is untouched, because a view command carries no basis and cannot invalidate one. `Command::SwitchView` is deliberately **not** counted although it moves the document: it carries no basis at all (ADR 001 names that hole and leaves it open) and a record surrogate is view-independent, so a switch cannot invalidate an edit beside it. Counting it would reject useful frames for staleness their own siblings caused — the mistake in the other direction, and the one that made per-command re-checking wrong to begin with. With the count in place, atomicity stops being an argument: at most one command in a frame can be refused, and it is refused before anything runs. Also from the same review, all correct: - `App::batch_aborted` named `Sessions::apply_save`, a function that no longer exists; the consumer is `Host::save`. That matters more than a stale name, because the singleton-batch identity that makes the flag readable at all is only true at *that* call site. - The "two classes" taxonomy on `Rejection` and the "two causes" one on `ServerFrame::Rejected` each sat immediately above an enum that could report a third. Both now enumerate three — staleness, a request the host cannot honour, and a frame that cannot be answered as one unit — and say that the variants rather than the sentence are the list to match. Evidence: `a_frame_carrying_two_document_mutations_is_refused_whole` drives **both** shapes — the structural one Copilot named and the `[LoadModel, TransposeAxes]` one the previous commit handled — because a guard written for one shape is exactly what got this wrong twice (`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`). Two faults injected and watched to fail: disabling the count fails the refusal arm, and widening it to `> 0` fails the **accepting** arm, which drives one mutation surrounded by the view commands a resolved keypress really batches with it. 1206 passing (the baseline moved to 1198 when `main` merged in); clippy, fmt and the wasm gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
6a929c1d21 |
feat(records): a +cat column, so a schema can be built without leaving the grid
improvise-2zq step 2, on the user's ruling. Step 1 made this coherent by stopping `on_category_added` from ejecting the user; this adds the column. Typing a name into `+cat` creates that category, and because `GridLayout::records_columns` filters on `kind.is_regular()` and never looks at items or at an axis, the coordinate column appears immediately and is typeable on the next keystroke. **The two questions the ruling left open, and the answers.** *Where the column goes.* Not a second trailing column, and not one `+` disambiguated by a prefix. A prefix would put a sigil into the name space — and a category and a measure may legitimately share a name, which is the exact ambiguity `_Dim` already carries a column *index* rather than a label to avoid (gotcha 1). A key was rejected too: the name still has to be typed somewhere, and a minibuffer lands in the records-exit class bug this PR characterizes but does not fix. So each `+` sits adjacent to what it adds to — `[coords…, +cat, measures…, +]`. Position tells the two apart instead of a label, which is the clutter the ticket was worried about. It costs a mid-grid column index, and the consequence worth naming is that a **blank** records grid now opens with the cursor on `+cat` rather than `+`: a record needs coordinates before it needs measures, so that order is defensible, but it does change what the first keystroke in an empty grid does. *Whether committing also sets the coordinate on the current row.* It does not. It is symmetric with the measure `+`, which creates an empty column and writes no value; and setting it would move **every** measure cell in the row onto the new coordinate (gotcha 1) — a data migration across N cells attached to the same Enter that made a schema change, in an app with no undo. The follow-up edit is one column away. **The refusal is visible, which took a change of order.** `MAX_CATEGORIES` is reachable here rather than theoretical. `AddCategory` now aborts the batch on refusal — same reason a failed save does (gotcha 21): what follows assumes the category exists, and the trailing `EnterEditAtCursor` would re-open the minibuffer *over* the message, since `draw_bottom_bar` renders the minibuffer instead of the status line. And `CommitAndAdvance` now emits its mode change **before** the commit rather than after, so an abort cannot eat it. That is the third time this PR has applied one rule — tidy-up goes where an abort cannot reach it — and the comment at that site had already described the pattern for the *advance* failing; the commit failing was simply not considered. The limit itself is still checked in exactly one place, `Model::add_category`. Evidence. Three new tests drive real keys: `the_new_category_column_creates_a_category_and_its_column` (the column is usable immediately), `..._writes_no_coordinate` (the deliberate answer to question 2, asserted over every row), and `..._refuses_past_the_limit_where_the_user_can_see_it`, which asserts the mode has **no minibuffer** — proved discriminating by swapping the commit back in front of the mode change and watching it report "Maximum of 12 categories reached" from inside `RecordsEditing`, where nothing would have drawn it. Four existing characterization tests were updated knowingly, not incidentally: the shared records fixture's column list (which accounted for 8 of the 9 initial failures on its own), the core layout and persistence smoke expectations, and the blank-model bootstrap, which now has to step to the `+` column and so records the cursor-position consequence above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
6a45edcfc9 |
fix(fit): six more Copilot findings, and a false premise of my own
The second review round. All six correct; five fixed, one documented and filed.
**My fixture's determinism premise was false, and the comment asserting it was
the worst part.** Both `document()` fixtures left one regular category with a
single item and claimed the constant one-hot had a degenerate `(1, 1)` range
that `simplify` would prune, so the champion "could not" reach an unresolvable
feature. `data.rs::build_categorical` pushes `(0.0, 1.0)` for **every** one-hot
whatever the column holds, so `simplify` cannot know it is constant — the
success tests were passing on seed luck, and three comments, the PR body and a
proposed repo-map gotcha all repeated the claim. Every category in both fixtures
is now a `Label`, so the only feature is the numeric measure `Cost`, which is
also the only shape a fit can currently store.
**Resolving a name is not the same question as meaning the right column.** A
numeric category called `Cost` encodes to a feature called `Cost`; with a
`_Measure` item of the same name the emitted `Expr::Ref("Cost")` resolves
cleanly — to the measure — and the stored rule reads a different column from the
one it learned. `fitted_formula` deliberately asks only whether a name can be
*spelled*, which is right for that check and wrong as the only one. So the
question is now also asked of `preprocess`'s own plan: a used feature is
accepted only where `Dataset::columns` says it is a raw `Numeric` **measure**
column, which rules out one-hots, `.missing` flags, date parts and category
coordinates by role rather than by spelling. A document naming the same thing as
both a category and a measure is refused outright. Deliberately *not* a
reconstruction of the expansion order index-by-index — that is improvise-4zl7's
job and its ticket warns against a second implementation.
**The width guard was on the wrong side of the cost.** It ran after `preprocess`
had already materialised the rows × rows one-hot matrix it was meant to prevent.
`encoded_width` now counts what the expansion *would* produce, from the raw
table, without producing it.
**`importance_ranking` misreported small importances.** `.max(1e-9)` on the
divisor is right for the only input `fit_csv` ever had and wrong as a contract:
a lone importance of `1e-10` divided by the floor and reported 10%. Importances
this small are ordinary — error differences over a standardized target — so the
zero case returns early and the shares sum to 100% at every scale.
**A name can contain a newline.** `.improv` pipe quoting makes it legal, so a
document could hold a measure whose name split the TUI status bar in two.
`status_line` flattens the composed line once, rather than sanitising each
interpolated part and forgetting one.
**Declined and documented on `Quality`:** preprocessing runs over all rows
before `cv_regress` picks folds, so imputation means and categorical
vocabularies leak from the validation fold and `Measured` is slightly
optimistic. Bounded — a fit over complete numeric measures, the only shape that
currently stores, leaks nothing — and closing it means preprocessing per
training fold, which `cv_regress` cannot express because it takes rows that are
already encoded. That is a `gp-tree` API change, not a line here.
1225 passing (+5); clippy, fmt and the wasm gate clean; the CLI's output still
byte-identical, which is the check that gates the `importance_ranking` change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
61984ec46a |
docs(ws-server): its usage example named the document this branch deleted
Copilot, PR #22, third pass — a dangling reference, and one my own deletion created, so it is mine to clean up (AGENTS.md §3) even though the file is outside this unit's stated boundary. Flagged in the hand-back. `improvise-ws-server`'s module doc opens with a copy-pasteable command line and a URL path derived from the file stem, all three naming `bank-info.improv`. Pointed at `examples/demo.improv` instead — a document that exists, and one `tests/shipped_documents_load.rs` now proves loads, so the example cannot go stale the same way without a test saying so. Doc comment only; no code, no behaviour, no test moved. Still mentioning the file, deliberately: the two test doc-comments that explain why it was deleted. Not touched, and not mine to touch: `roadmap.org` line 194 and the `improvise-1ey` / `improvise-cr3` issue bodies in `.beads/` name it as the demo document to point the browser at. Reported rather than edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
ae199d431a |
docs: an address check cannot see a cross-site socket
Copilot's fifth pass, and the first finding in this PR that is a genuine security hole rather than a wrong sentence. Verified: there is NO reference to Origin anywhere in crates/improvise-ws-server -- the server accepts every handshake. A WebSocket is EXEMPT FROM THE SAME-ORIGIN POLICY; there is no CORS preflight on a handshake. So any hostile page an allowed user visits can open a socket to the endpoint FROM THAT USER'S OWN ALLOWED ADDRESS and read and edit the document. The IP allow-list I recommended last round is precisely the control that cannot see this, because by then the request really does come from an allowed client. Only the Origin header separates them. This is the same shape as the previous three rounds and it is worth naming as such: I reasoned about WHO MAY REACH the endpoint and not about WHAT MAY DRIVE the browser that reaches it -- one more conclusion carried without its reasoning. "Who may connect" is two questions, and the recipe answered one. So: * the prose states the two questions and says an Origin allow-list is needed IN ADDITION TO, never instead of, whichever control is chosen; * the Caddyfile carries both checks, with the second commented as explicitly not redundant with the first; * the `header` matcher syntax is checked against caddyserver.com/docs/ caddyfile/matchers (`header <field> [<value> ...]`, negatable inside a named matcher) rather than written from memory -- the round-3 rule; * and the LOCAL pairing paragraph is corrected too, because the same fact applies there and I had called it unreservedly fine: loopback is not a boundary a browser enforces, so any page open in that browser can reach ws://127.0.0.1:8787 while the server runs. The server-side half -- ws-server should check Origin itself, since a proxy is not present in the local case at all -- is reported for filing rather than built here. It is a change to a crate this unit does not own. 1193 passing, unchanged; no code touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
fceedf9f0d |
perf(protocol): build the header tables once per frame, not per position
Copilot's third pass, and correct: `item`, `label`, `shows_level` and `group_of` each scan `entries` from the start, and both renderers were calling them per position per level. That is quadratic in the frame length, and in the TUI it grows with the absolute scroll offset — a regression against the pre-improvise-anr9 widget, which materialized the row tuples once precisely to avoid it. `AxisFrame::header_labels(first_rendered)` answers the whole suppressed label table in one pass, and `AxisFrame::groups()` answers the group of every position in one pass. Both renderers call those; `shows_level` and `group_of` remain for single lookups. The suppression rule does **not** gain a second copy: it moves out to one free `shows(items, ..)` over materialized items, and `shows_level` and `header_labels` are its two entry points — a lookup and a whole frame. `fit_to_labels` materializes once for the same reason. Suite unchanged at 1232 passing, including `the_whole_frame_is_unchanged`, which is the exact-buffer characterization and so is the check that this stayed a pure hoist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
8173b87723 |
fix(view): a virtual on an axis is a configuration, not a vacancy
improvise-2zq step 1. Naming a category in records mode ejected the user from the grid they were standing in, and it is a live bug independent of the inline-column work it blocks. **What the old rule was.** `View::on_category_added` seats a new regular category on the first axis no *regular* category holds — Row, then Column, then Page — and, to get there, **bumped any virtual off that axis** on the assumption that a virtual sitting on Row or Column was a placeholder. It never is, and the assumption cost two live bugs: * `_Index` on Row together with `_Dim` on Column *is* records mode. The only production writers of those axes are `ToggleRecordsMode` and `DrillIntoCell`, so a virtual is on Row only when the view is a records or drill view. Bumping `_Index` therefore turned `N`, a name, `Enter` into a silent return to a pivot. * `_Measure` on Column is an ordinary user configuration — it is what `web/demo.improv` ships — and adding a second regular category kicked it off the axis, making every measure vanish from the grid. **What the new rule is, including for a non-records view**, since this changes how *any* view seats a category and not just the records one: * A virtual is **never moved**. That is the whole fix; both bugs above are the same mistake pointed at different virtuals. * A **records view** seats the new category on `Axis::None`. That is where records mode keeps every other regular category, and `GridLayout::records_columns` filters on `kind.is_regular()` and never looks at an axis, so the column appears immediately either way — `None` is the answer that also leaves the view a records view. * A **pivot view** is unchanged in the case that matters: Row, then Column, then Page, counting only regular occupancy. The one observable difference is that a new category now *joins* a virtual already on the target axis, nesting a header, instead of evicting it. `View::is_records_layout()` is new and is the **one** definition of "is this a records view" — `GridLayout` now asks it instead of re-deriving the same `_Index`-on-Row/`_Dim`-on-Column test, because two answers to one question is the disease this repo keeps curing (improvise-edp) and `on_category_added` had to ask it too. **Evidence, and the probe that shaped the fix.** Before designing it I broke `on_category_added` in two directions, as the method that has been paying off all wave. Removing the virtual bump entirely fails **zero** tests; seating every category on `None` fails **twelve**. So the auto-assignment is load-bearing and well covered, while the bump was completely unguarded — which is how it came to be wrong about two different virtuals with nothing noticing. `naming_a_category_in_records_mode_does_not_eject_the_user` drives `R`, `N`, a name, `Enter` through real keypresses, because that is how the bug was found. Two unit tests cover the rule directly, and `a_pivot_view_still_seats_categories_row_then_column_then_page` is the negative arm — without it "always seat on None" passes everything else. All four were run against the restored old rule: the three positives fail and the negative arm passes, which is exactly the split being claimed. Note for improvise-9mo9 (gotcha 24, another agent's): this does not change whether `AddCategory` renumbers the grid — it still does, and it still does not call `reset_cursor`. It changes only which axis the new category lands on. `AddCategory` keeps its `LayoutGeometry` kind, which is right for the same reason as before: `Workbook::add_category` rewrites every view's axis map. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
aa1534e078 |
chore(beads): file five follow-ups surfaced by WU-G
All from PR #27's hand-back, searched against the open backlog first and none of them duplicates. improvise-mai9 P2 — a status message written while a minibuffer is open is never drawn. draw_bottom_bar renders the minibuffer INSTEAD of the status line, so improvise-6t3 carefully ordered a failure message to win a race to a surface nobody can see. Copilot then found the regression this causes, in the same PR that documented the mechanism. improvise-z05r P3 — CommitExport does not bail on an empty path and ExportCsv does not abort the batch: the export arms of what PR #27 fixed for save. Third instance of the same asymmetry, so the structural fix may beat the third patch. improvise-g9jb P3 — the text-entry mode list is hand-written in two crates and checked against AppMode in neither, which now decides whether a browser refuses to open a prompt it cannot commit. improvise-zx8t P4 — draw.rs::mode_style matches AppMode with a wildcard, so the repo map's "the compiler names every site" promise holds in five places and not the sixth. improvise-rw3a P3 — the open half of improvise-lae: whether ZZ routes into the new save prompt, and what ZZ means once it is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
fb507b9855 |
fix(opfs): a directory is not a document refresh could not measure
Copilot's second round on PR #24, and it found the hole in the previous commit's own argument. That commit said an entry `refresh` cannot measure counts as a document, "because the one cause is another tab holding the exclusive handle". There is a second cause, and I missed it: **`keys()` yields directories as well as files**, and `getFileHandle` on a directory rejects with `TypeMismatchError`. So a directory would have been listed as a document, and one can never be attached or read — the exact picker bug the zero-byte filter exists to remove, reintroduced by the fix for it. The two failures now get opposite answers, which is the whole of `holds_bytes`: - **`getFileHandle` fails** — the name is not a file. A directory, or an entry removed between `keys()` and here. Not a document; skip it. - **`getFile` fails on a handle `getFileHandle` already returned** — the exclusive-lock case, a second tab of the same origin. That one still fails open, for the reasons the previous commit gives. Argued *and* tested this time. `a_directory_entry_is_not_a_document` in the harness makes a real directory in OPFS — `OpfsFs` has no directory vocabulary at all, so the harness reaches `getDirectoryHandle` itself — and asserts it is not listed, with a real file written beside it as the precondition, so the check cannot pass by the directory never having been created or by `keys()` walking nothing. Proved discriminating by flipping the `None` arm back to `true`: it fails with `refresh() listed a directory as a document: ["a-folder"]`. Adding it also exposed that three checks were reading the leftovers of the check before them — the battery broke the moment one was inserted in the middle. Every check now wipes first, so the order of the battery is not load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
c23f7db3f4 |
fix(cmd): a : line's tidy-up runs where an abort cannot reach it
Copilot round 2, and it is a regression this branch amplified. A failed
`:w <path>` set `abort_effects` (
|
||
|
Edward Langley
|
1c98aa0929 |
docs: ambient credentials versus challenge-response, which is the real rule
Copilot's fourth pass, and it caught this text CONTRADICTING ITSELF three paragraphs apart -- in prose I wrote in a single commit while fixing the previous round's contradiction. The Caddyfile comment said "the browser cannot send a credential on a WebSocket handshake". That is FALSE, and the same section already said so: the Cloudflare Access bullet explains that Access works by a session COOKIE the handshake carries. A WebSocket handshake is an HTTP request; the browser attaches ambient credentials to it automatically. The true rule, which is sharper than the one it replaces and explains both halves instead of one: AMBIENT credentials are carried -- cookies for that host above all, which is why an identity-aware proxy works at all. CHALLENGE-RESPONSE cannot be satisfied -- the page can set no header (there is no Authorization argument to `new WebSocket`) and can answer no prompt (a handshake surfaces no login dialog and fails to script as a bare connection error). So basic auth is still ruled out, for a REASON rather than by an over-broad invariant, and Access is no longer an unexplained exception to it. Two consequences carried through the rest of the section, on the grep discipline the last round earned: * mTLS is no longer asserted to fail. The certificate IS offered at the TLS layer rather than by the page; only the selection UI is doubtful, and that is now marked as not verified here rather than stated. * "a query parameter is the one credential new WebSocket can carry" restated the same over-broad claim in miniature, and becomes "the one place the page can put a credential DELIBERATELY -- everything else it sends is ambient." 1193 passing, unchanged; no code touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
9a93c388a4 |
test(wizard): assert the cell padding against a sentinel, not against the hint
Copilot's third finding on PR #22, and it is right about the thing this repo cares most about: my combining-mark case passed for the wrong reason. Drawing through the widget cannot see the *under*-padding fault at all. The hint runs first and pads the row itself, so the cells a combining-mark message fails to reach hold spaces, not anything visible — and "does the hint survive" is satisfied either way. Worse, the check looked as though it had been proved, because the loop panicked on the wide-character case before it ever reached the combining one. That is `doctrine(an_injected_fault_must_be_the_fault_you_named)` from the other end: the injection fired, on a different arm than the one being claimed. Replaced with a direct test of `set_bottom_line` over a row pre-filled with `X`, two cells wider than the panel so the border is a real cell rather than an artefact of the assertion. Every interior cell must be the text or a space; the two border cells must still be `X`. Both faults then show up as a surviving `X` — one inside the panel, one outside it: wide, chars().count(): ["X","广"," ","度",…," "," "] border eaten combining, same: ["X","é","é","é"," "×12,"X","X","X","X"] 3 uncleared Each case was watched failing **on its own**, with the other removed, so neither is hiding behind the other's panic. `short` is kept as the ASCII arm that must stay green throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
802dfc6f91 | Merge main into wave/g-command-layer (PR #20: live_records ordering, totality gaps, grid_updates totals) | ||
|
Edward Langley
|
ed201e83de |
test: the column header check is exact, not a contains
Copilot's second pass, and correct: the check was one-directional. It filtered out the labels the browser blanked and then asked whether each surviving one appeared *somewhere* on the TUI's line — so a `col_levels` that over-suppressed was skipped by the filter, and a label painted under the wrong column still matched. It now cuts the painted line into its columns by the cache's own `col_widths` past the row header — the same geometry the widget lays them out with — and compares the whole vector. Both faults Copilot named were injected and watched to fail: blanking a label the TUI paints, and reversing the column order within a level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
847cde33cc | Merge remote-tracking branch 'gh/main' into wave/c-host-and-save | ||
|
Edward Langley
|
f4600eb277 |
docs: stop giving ngrok a recipe I have not run
Copilot's third pass, on the flag I introduced LAST round while fixing the flag I introduced the round before. Two consecutive rounds of writing an access control into the README without running it is exactly the doctrine committed in |
||
|
Edward Langley
|
3de90074a9 |
fix(fit): four Copilot findings, including a reachable panic
All six findings on PR #25 are correct. Four are fixed here; two are declined with reasons and documented on `fit_measure` rather than left silent. **A legal document could panic the command.** `preprocess` drops an entirely missing column, so a document with no regular category and one empty measure encodes to *zero* features — and `random_tree` then draws `rng.below(0)`, which `SplitMix64` answers with `0`, and `sample_thresh` indexes `spec.ranges[0]`. Refused now, and the refusal carries the warnings naming what was dropped. Proved by disabling the guard and watching `index out of bounds: the len is 0 but the index is 0` at `gp-tree/src/lib.rs:218`. **Nothing bounded the feature *width*.** `FitConfig` bounds the search; a regular category with one distinct value per record one-hots to one column per record, and permutation importance is `repeats × rows × features` evaluations — quadratic in records, on the thread that draws the next frame. The 6/12/105 ms benchmark this PR quoted does *not* cover that shape, because its fixture marks the row-identity category as a `Label`. `MAX_FEATURES = 64` refuses, and names `Label` as the fix. The test had to grow past 64 records to be honest: at twelve the document is refused anyway, by `fitted_formula` noticing `Tag=r0` — a different rule, which would have made the test pass with no guard at all. **`FitOutcome` did not meet its own reproducibility contract.** It carried `seed` + `config`, which is the pair gotcha 37b names — and that pair is sufficient for `gp_tree::data::Model`, whose other knobs are not in the loop. Here they are: cross-validation draws from the *same* generator `fit` then draws from, so `cv_folds` moves the champion and not merely the evidence. It carries the whole `FitRequest` now, with a test that changing only `cv_folds` changes the rule. **The status line advertised a seed the parser rejected.** `status_line` prints `{:#x}` and tells the user that reproduces the fit; `parse` took decimal only, so the one instruction it gives failed on the value it had just shown. The test round-trips through `status_line` rather than a hand-written `"0x…"`, because a literal would still pass if the printing changed — which is the half that breaks. **Declined, both documented on `fit_measure` and to be filed.** A mean-imputed feature is read *raw* by the stored formula, so a record missing that value evaluates to something other than what the fit predicted for it; closing that means choosing between refusing any feature with a missing cell — which would refuse most real documents, imputation being why `preprocess` imputes — and an `IFMISSING`-shaped primitive the formula language does not have. That is a product decision of the same kind as improvise-4zl7. And a boundary value can route differently in the formula than in the fit, because training narrows `f64` to `f32` while the formula compares the original: pre-existing in the emitter (improvise-izk), and the fix would rewrite the bit-level characterization tests that are the only evidence the routing argument holds. 1220 passing (+4); clippy, fmt and the wasm gate clean; the CLI's output still byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
3def0e8521 |
Merge main into wave/i-browser-shell
Picks up PR #20 and the beads export in
|
||
|
Edward Langley
|
373965c596 |
fix(protocol): saving is a frame, and a frame may not replace its own basis
Copilot's review of PR #26 found two real defects, both of which this branch
introduced, and both of which are the same mistake in different clothes: an
argument that held for the case I tested and not for the case I did not.
**1. `Command::Save` put a runtime-fallible operation inside a transactional
batch.** `Host::commands` guarantees a frame is applied whole or not at all,
but `apply_save` could still answer `Applied::Rejected { SaveFailed }` from
*inside* that loop — so `[Save, SetCell]` reported a failed write and then
applied the edit anyway, and `[SetCell, Save]` reported "refused" for a
frame that had fully applied. A client reading `ServerFrame::Rejected` could
not tell which. My own doc on `apply_checked` said it "cannot return
`Applied::Rejected`", and that was simply false.
**Saving is now `ClientFrame::Save`, not `Command::Save`.** That is the
honest home for it and I would argue it even without the bug: a `Command` is
something the shared state folds, and saving mutates no shared state and no
client's slice — `reduce_view` had nothing to fold, and `Checked` had no
basis to answer for, so both arms were shims. As a frame it sits beside
`ClientFrame::Viewport`, which is the other session input no `Command`
carries, and **it cannot be composed with a mutation at all**: the batching
question stops being a rule and becomes unconstructible
(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`). The user's
ruling was "add Save to the wire"; it is on the wire, one layer out.
`Host::save` answers both refusals directly, so `Checked` goes back to being
about ADR 001 staleness and nothing else. The behaviour a client sees is
unchanged: `NoDocumentPath` decided before anything runs, `SaveFailed` read
off `App::last_batch_aborted` after a batch that is exactly `[Save]`, both
addressed to the asker alone, success silent.
**2. The documented "residue" was under-stated, and the argument only covered
the command I had tested.** A frame whose earlier command replaces the
document leaves its later ones checked against the outgoing one. I argued
that was harmless because a `SetCell` names its cell by surrogate (ADR 002)
and an outgoing surrogate resolves to nothing in the incoming document. True
— and it covers exactly one command. `TransposeAxes` names no surrogate, so
`[LoadModel, TransposeAxes]` passed every check against the outgoing `App`
and then reshaped a freshly loaded workbook under a basis describing the
previous one: precisely the cross-document mutation the generation tier
exists to reject.
Re-checking the remainder is not available as a fix — it rejects *after* the
load has landed, which is the partial application the batch check exists to
prevent. So `CheckedBatch::against` **refuses the frame whole**, with
`Rejection::DocumentReplacedMidFrame`. `replaces_the_document` is an
exhaustive match beside `records_named`, so a new `WholeWorkbook` variant
fails the build until it classifies itself.
This is the one place here where the *variant* is the right question rather
than "did the thing move" (gotchas 24, 39): a client composing a frame that
**may** invalidate its own basis half way through has composed an incoherent
claim whether or not the load succeeds.
**3. Copilot also caught three stale documents**, two already fixed in
|
||
|
Edward Langley
|
4abcdaaa64 |
chore(beads): close improvise-2yc2 and improvise-hkt6, file three follow-ups
CI / Test and lint (host) (push) Has been cancelled
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
CI / Knowledge base consistency (push) Has been cancelled
CI / Browser bundle + static site (push) Has been cancelled
CI / Deploy to GitHub Pages (push) Has been cancelled
Closed by PR #20 (WU-K), with the reasoning on each bead rather than in a commit nobody will read again. Filed from WU-I's hand-back: improvise-kpnr improvise-ws-server has no authentication, and the reverse-proxy path the README now documents removes the loopback binding that was its only control. improvise-j7v6 the typed-string classification lives in two crates, and its guard test states the obligation in its name while asserting three outcomes by hand. Filed from WU-K's hand-back: improvise-44ad ColWidths is redundant under the label fold — tried and reverted, because the test it would rewrite is gotcha 33(c)'s standing evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
afa4a17921 |
fix: two Copilot findings on the save prompt — a dead room and a hidden error
Both correct, both applied, and both wider than the line they were raised on. **A prompt a browser cannot commit is never entered.** `enter-mode` is trivially resolvable in the wasm client, so `Ctrl-W` opened `SavePrompt` there — and Enter then resolved `[commit-save, clear-buffer]`, whose first step has no arm in `keys::step`, went `Upstream`, and was dropped by `Host::handle`'s empty vec. The user types a path into a prompt that swallows it. `doctrine(an_upstream_key_is_a_black_hole)` already says an upstream key does nothing; what this adds is that a *doorway* to such a key is worse than the key, because the door costs one inert keypress and the room costs work the user believes was done. `Ctrl-E` into `ExportPrompt` had the identical trap and predates this branch, so both are fixed: fixing one arm of a rule with two is the shape `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)` names. Neither is an omission to close later — `Save`, `SaveAs` and `ExportCsv` are all `ViewOnly`, so no `ModelCommand` names them and no client on any transport can ask a host to write a file (gotcha 45). Named as a predicate, `strands_the_user`, rather than matched at the call site (`doctrine(two_questions_one_enum)`): "can this client finish what entering the mode starts" is a different question from "is this a text-entry mode". **And the TUI reported a failed save where nothing draws it.** `SaveAs` aborts the batch on failure, so with `CommitSave` emitting `[SaveAs, ChangeMode(Normal)]` the app stayed in `SavePrompt` — where `draw_bottom_bar` renders the minibuffer *instead of* the status line, so the "Save error:" set a statement earlier was never drawn. The save appeared to do nothing at all. Leading with the mode change puts the error in `Normal`, where the bottom row is the status bar. It costs the typed path, which the welded `clear-buffer` takes on success anyway and `EnterMode` clears on the next `Ctrl-W` regardless (gotcha 11). Evidence, both injected. `a_prompt_this_client_cannot_commit_is_never_entered` fails on both arms when `strands_the_user` returns false, and carries a negative arm (`:` still opens the command line) so "refuse every mode" cannot pass it. The end-to-end test now asserts the mode *left* the prompt — the opposite of what it asserted an hour ago, deliberately — and fails when the two effects are swapped back. Copilot's third finding, that `context/repo-map.md` and `prolog-sessions/kb.pl` still say 16 AppMode variants and seven text-entry modes, is correct and is **not** applied here: both files are orchestrator-only in this wave. The exact edits are in the hand-back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
5bfa747942 |
fix(assemble): verify each cache-bust stamp on its own, and rename a test
Two Copilot findings on PR #24, one applied as asked and one applied only in part, with the measurement that says why. **The combined `grep` was a real hole.** `grep -n "?v=" a b c d` succeeds when *any one* file matches, so under `set -e` three failed substitutions still produced a "successful" assembly. Those greps are the only guard against the generated wasm-pack glue changing shape — which is the exact failure this cache-busting exists to end, twice, during improvise-9x6 — so each stamped reference is now checked on its own and names itself when it is the one missing. Inherited from the inline CI step rather than written here, but it became mine when it became a script. Proved by pointing one `sed` at a pattern that cannot match: the old form exited 0, the new one exits 1 naming the file. **The repaint count was the right objection to the wrong remedy.** Copilot asked for "count the relevant grid mutations/repaints and prove the assertion by removing the guard". Done, and it does not discriminate: with `if (closed) return` removed from `edit`'s `close`, `close` still runs exactly twice, `typed_cell` is still called exactly once, `#grid` still takes exactly two childList mutations, and the final text is identical. The only difference is one extra *entry* into `repaint()` that produces no DOM change — because the second close is `close(false)`, so it does not re-send, and everything else it does is idempotent. Which is exactly what improvise-hwpi meant by "harmless before, but only by accident". Counting `repaint()` entries would need a test hook inside `web/app.js`: a page that knows it is being tested. So the half of the finding that stands is the **name**, which claimed more than the body checked — the same defect as `open_and_refresh_see_an_empty_origin_root` two commits ago. Renamed to `committing an edit sends one frame and shows the host's value`, and the measurement is recorded in the comment so nobody repeats it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Ed Langley
|
775e427403 |
Merge pull request #20 from fiddlerwoaroof/wave/k-core-totality
CI / Test and lint (host) (push) Has been cancelled
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
CI / Knowledge base consistency (push) Has been cancelled
CI / Browser bundle + static site (push) Has been cancelled
CI / Deploy to GitHub Pages (push) Has been cancelled
WU-K: decide live_records' order, and close three totality gaps (improvise-2yc2, improvise-hkt6). |
||
|
Edward Langley
|
7c4b90c537 |
fix: four Copilot findings on the grid frame
All four were right; two more were right and are declined on the threads. **A frame's pre-window headers were O(offset).** `axis_frame` accumulated every group header above the window and flushed the run at the first in-window item, so a deep scroll past many groups serialized headers that `body_from` discards on arrival. Only the *nearest* one can ever be answered with, so only that one is kept. **`web/app.js` captured the row counter, not its value.** `r` is one binding shared by every click-handler closure, so after the loop every editable cell would have called `edit` with the last data row. The group-aware counter was right; capturing it was not. It is a bug this PR introduced — the old code took `r` as a `forEach` parameter, which is per-call. **The agreement suite drove one arm.** It only read `rows`, and its fixture had no grouped column axis, so `col_levels` and `col_groups` could regress behind a green suite (`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`). The fixture now groups two of three months under `Q1` and leaves `Sep` outside, with `_Measure` giving a second column level, and `the_two_front_ends_draw_the_same_column_header` checks the levels against the painted header lines and the spans against the column count. One span per column, and a dropped level suppression, were each injected and watched to fail. **The group branch did not compare the label.** It asserted only that the painted text began with an indicator, so any group name passed. It now compares the whole painted header against the browser's glyph-plus-label. The column test also *demonstrated* the finding this PR declines: `Q1` spans all six columns including the two under the ungrouped `Sep`, because `expand_category` emits a header on entering a group and no boundary on leaving one. That is `GridLayout`'s rule and the TUI paints the same span. It is asserted as it is, with the defect and the corrected expectation written next to it, and stated on `AxisFrame::group_of`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |