docs: ADR 001 — concurrent edits between clients
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

Records the user's decision: record-level atomicity (the coordinate tuple
excluding _Measure, which record_row_identity already names) with optimistic
concurrency per batch, and a basis tiered by ModelChange so a shape change
invalidates in-flight cell edits without any lock.

Documents why a causal clock was rejected: the server already serialises, so
what is needed is staleness detection (one counter per unit) rather than
concurrency detection (a vector per client) — and no clock answers the policy
question of whether an edit into a deleted category should survive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
This commit is contained in:
Edward Langley
2026-09-19 16:10:08 -07:00
parent be7ddf3f84
commit a08f24f323
2 changed files with 153 additions and 2 deletions
+3 -2
View File
@@ -21,7 +21,8 @@
{"_type":"issue","id":"improvise-km8","title":"1.2 Fix Cargo.toml metadata","description":"Add missing package fields: description, repository, homepage, documentation, readme, keywords, categories, license. Description: 'Terminal pivot-table modeling in the spirit of Lotus Improv'. Keywords: tui, pivot, spreadsheet, data, improv. Categories: command-line-utilities, visualization.","status":"closed","priority":1,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:06:48Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:20:17Z","close_reason":"Metadata already present: description, license, repository, homepage, readme, keywords, categories all populated.","dependency_count":0,"dependent_count":2,"comment_count":0}
{"_type":"issue","id":"improvise-902","title":"1.1 Audit and remove or update context/SPEC.md","description":"Compare context/SPEC.md against actual code. If spec contradicts code significantly, delete it. Optionally move salvageable conceptual content to docs/design-notes.md with a staleness disclaimer. Do not try to bring spec into sync.","status":"closed","priority":1,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:06:46Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:10:05Z","close_reason":"Audited SPEC.md against code. Found minor inaccuracies (storage internals, wizard step count, search mode representation) but no major architectural contradictions. Deleted SPEC.md since it was largely redundant with repo-map.md and design-principles.md. Salvaged product vision and non-goals into docs/design-notes.md with staleness disclaimer.","dependency_count":0,"dependent_count":1,"comment_count":0}
{"_type":"issue","id":"improvise-tik","title":"Phase 1: Repository hygiene","description":"Raise the quality floor of what a stranger sees when they land on the repo. Subtasks cover spec audit, Cargo.toml metadata, publish-readiness, CSV audit, and example files.","status":"closed","priority":1,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:45Z","created_by":"Edward Langley","updated_at":"2026-04-09T07:22:34Z","closed_at":"2026-04-09T07:22:34Z","close_reason":"All 5 subtasks complete: SPEC.md audited, Cargo.toml metadata fixed, publish dry-run clean, CSV audit done, example files created.","dependencies":[{"issue_id":"improvise-tik","depends_on_id":"improvise-2fr","type":"blocks","created_at":"2026-04-08T23:37:31Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-902","type":"blocks","created_at":"2026-04-08T23:37:30Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-bv1","type":"blocks","created_at":"2026-04-08T23:37:32Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-km8","type":"blocks","created_at":"2026-04-08T23:37:30Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-tik","depends_on_id":"improvise-n1h","type":"blocks","created_at":"2026-04-08T23:37:32Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":5,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-gvnb","title":"[bug] A geometry command resets the App cursor but not the session mirrors","description":"Found during improvise-kmgs (2026-09-19). Pre-existing for the key path; the model half of the wire vocabulary makes it reachable remotely, which is what raises it to P2.\n\nCycleAxis and TransposeAxes call app.reset_cursor() because renumbering rows or columns invalidates the cursor (gotcha 24). But each ClientSession keeps its OWN cursor in its mirrored ViewState, and nothing resets those. So a bystander is projected renumbered rows at its old scroll -- gotcha 24 cursor-vs-geometry mismatch, arriving through somebody else command.\n\nThis is the same SHAPE as improvise-kvyt (a view switch moved only the asking session mirror) and should be fixed the same way: the geometry change is addressed to no one session, so every mirror owes the reset. See doctrine(lockstep_binds_every_session_not_the_asker) -- ask of any shared value, who else derives something from it. Here the derived thing is the cursor whose validity depends on the grid shape.\n\nTest it the way kvyt was tested: a bystander session, not the one that issued the command.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T22:11:31Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:11:31Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-xwi6","title":"Concurrent edits: basis check (ADR 001, layer 1 — the correctness floor)","description":"Implements layer 1 of context/adr/001-concurrent-edits.md. User decision 2026-09-19: record-level atomicity with optimistic concurrency per batch, no causal clock.\n\nTHE FRAMING THAT MATTERS: the server already serialises. Sessions::apply -\u003e apply_command -\u003e App::apply_effects against one App, so a total order EXISTS. This ticket is not about ordering; it is about noticing that a client composed a batch against a basis that has since moved -- the same stale-basis shape as gotcha 24 (cursor) and gotcha 39 (session mirror), one level up.\n\nSCOPE:\n1. Version tiers, living beside document_generation on the authoritative App (they are shared document state, NOT ViewState -- gotcha 30):\n * document_generation -- exists, bumped by WholeWorkbook\n * layout_version -- bumped by any LayoutGeometry\n * record_version -- per record, bumped by CellData on that record\n A record is the coordinate tuple EXCLUDING _Measure: record_row_identity(wide, coord_group_excluding_measure), already in kb.pl. Not the cell -- gotcha 1 says editing a category column moves every measure cell in the row, so a cell-level unit is violated by a single legal edit. The unit must be at least as coarse as the coarsest legal write.\n2. ModelCommand carries the basis it was composed against.\n3. Sessions::apply rejects a stale batch. Applied grows a third case beside Diff and Rebased -- an enum so matching FORCES a host to handle it, which is gotcha 39 reasoning and the reason a bool was rejected there.\n4. Rejection is a normal outcome, not an error: the client already has the new state via projections and re-composes.\n\nWHY THE TIERS ARE THE POINT: a CellData batch is stale if LAYOUT moved, because the shape its keys hang off changed. That is the corruption class -- A deletes category Region while B commits a key naming Region=East -- detected without any lock. Reuses ModelChange so there is no second classification to drift from gotcha 35 table.\n\nTESTS: the arms, per doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only) -- a stale CellData batch after a layout change is rejected; a fresh one is not; a batch touching an untouched record is not; a WholeWorkbook bump invalidates everything. Beware doctrine(a_rename_can_hide_a_stale_key): hold identifiers constant and vary only what makes the basis stale.","status":"open","priority":2,"issue_type":"feature","owner":"el-github@elangley.org","created_at":"2026-09-19T23:09:53Z","created_by":"Edward Langley","updated_at":"2026-09-19T23:09:53Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-gvnb","title":"[bug] A geometry command resets the App cursor but not the session mirrors","description":"Found during improvise-kmgs (2026-09-19). Pre-existing for the key path; the model half of the wire vocabulary makes it reachable remotely, which is what raises it to P2.\n\nCycleAxis and TransposeAxes call app.reset_cursor() because renumbering rows or columns invalidates the cursor (gotcha 24). But each ClientSession keeps its OWN cursor in its mirrored ViewState, and nothing resets those. So a bystander is projected renumbered rows at its old scroll -- gotcha 24 cursor-vs-geometry mismatch, arriving through somebody else command.\n\nThis is the same SHAPE as improvise-kvyt (a view switch moved only the asking session mirror) and should be fixed the same way: the geometry change is addressed to no one session, so every mirror owes the reset. See doctrine(lockstep_binds_every_session_not_the_asker) -- ask of any shared value, who else derives something from it. Here the derived thing is the cursor whose validity depends on the grid shape.\n\nTest it the way kvyt was tested: a bystander session, not the one that issued the command.","notes":"\n--- CONFIRMED AGAIN on PR #11 by Copilot, and deliberately NOT fixed there ---\nThe PR agent reproduced it and declined to fix it in that PR with three\nreasons, all of which stand:\n (a) Applied::Diff.broadcast is a single Option\u003cCommand\u003e and there is NO\n cursor-reset command -- a ViewCursor is selected plus both offsets, so\n this needs a new wire variant, derived not invented.\n (b) it read the policy question as improvise-g0f, \"deliberately unanswered\".\n (c) A PARTIAL FIX IS WORSE THAN NONE: resetting only the server-side mirrors\n would make the diff logic believe each client already holds origin-based\n content and send nothing -- leaving clients stale PERMANENTLY instead of\n until their next scroll. Note that carefully when implementing.\nObserved effect today: Viewport::window clips (start = offset.min(total)), so\na bystander gets a BLANK grid rather than garbage -- blank until it scrolls.\n\n--- THE POLICY QUESTION IS ANSWERED; (b) rests on a stale reading ---\nimprovise-g0f is decided (per-client active view), and the standing principle\nsettles this case directly: THE MODEL IS SHARED, THE UI IS NOT. Two things\nthat sound alike and are not:\n * Another client transposing must NOT move my cursor -- where I am looking\n is UI, and it is mine.\n * It MUST invalidate my cursor, because the grid my cursor indexes just\n changed shape. Validity is derived from shared geometry even though\n position is private.\nSo the answer is neither \"reset everyone to origin\" (that moves my UI on\nsomeone else behalf) nor \"leave it\" (that indexes a grid that no longer has\nthose rows). It is: the geometry change invalidates every client cursor, and\neach client re-establishes its own -- which is exactly what a cursor-reset\nwire variant should mean, and why it is a variant rather than a broadcast of\nsomebody else position.\n\nSame family as improvise-kvyt and doctrine(lockstep_binds_every_session_not_the_asker):\nask of any shared value, who else DERIVES something from it. Here the derived\nthing is the validity of a private cursor.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T22:11:31Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:50:15Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-tym1","title":"[bug] folds(n, 0) panicked and mse divided by a length it had not scored","description":"Copilot finding on PR #10 (2026-09-19), partly real, fixed in 27a4cbd with both tests written first.\n\nfolds(n, 0) did not merely misbehave -- it PANICKED on out[pos % 0]. k arrives from a CLI flag, so zero is a value a host supplies, and gotcha 37 says gp-tree is a library a TUI or a wasm worker may embed, where a panic is not a usage error message. Now k.max(1), matching fit()s existing restarts.max(1).\n\nmse divided by x.len() rather than the number of rows it actually scored, so a length mismatch between the parallel slices silently skewed the result through zip truncation. Now it counts the rows it scored. Bit-identical where the slices match -- Sum for f32 is the same left fold -- so wave 3 nine-scenario byte-identical CLI evidence still holds.\n\nProven first: the folds test panicked; the mse test failed 16.912476 vs 17.802607.","status":"closed","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T22:01:09Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:01:10Z","closed_at":"2026-09-19T22:01:10Z","close_reason":"Fixed in 27a4cbd, merged to main in PR #10 as 336533b.","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-tom1","title":"Drill pending edits are invisible to remote clients, and there are now three answers to 'what text is in this cell'","description":"Wave-2 review (2026-09-19), CONFIRMED by reading.\n\nproject.rs cell_display resolves records-mode text through layout.records_display(row,col). The TUI grid widget overlays staged drill edits (src/ui/grid.rs, drill_state.pending_edits) and so does App::display_value_at_cursor. SetDrillPendingEdit is ViewOnly, so Dirty is empty too. A remote client in drill mode sees neither the staged edit nor any update for it, until ApplyAndClearDrill (CellData) lands.\n\nThis is the 'second, silently divergent description' the producer's own module header warns against: three answers to what text is in a cell (GridLayout::display_text, src/ui/grid.rs, project::cell_display) and only two agree.\n\nNote the tension with the user's principle 'the model is shared, the UI isn't': pending drill edits are UNCOMMITTED writes belonging to whoever staged them (that is why gotcha 30 keeps drill_state off ViewState). So the answer may legitimately be 'a remote client SHOULD NOT see them' -- in which case the defect is only the three-way divergence, not the invisibility. Decide which before coding.","notes":"\n--- DECIDED (user, 2026-09-19): NO ---\nA remote client must NOT see another client staged drill edits. This follows\nfrom the standing principle -- the model is shared, the UI is not -- and from\ngotcha 30, which keeps drill_state off ViewState precisely because pending\nedits are UNCOMMITTED WRITES BELONGING TO WHOEVER STAGED THEM. They become\neveryone business only at ApplyAndClearDrill, which is CellData and already\nreaches every client.\n\nSO THE INVISIBILITY IS CORRECT AND IS NOT THE BUG. Do not \"fix\" it by teaching\nthe producer to read drill_state; that would put one client uncommitted writes\non another client screen and contradict the decision.\n\nRE-SCOPED -- the defect that remains is the THREE-WAY DIVERGENCE in what text\nis in this cell:\n * App::display_value_of -- editable RAW text, resolves a staged drill\n edit, via layout.resolve_display\n * project::cell_display -- FORMATTED text + numeric/error flags; in\n records mode calls layout.records_display\n and then INFERS numeric-ness by\n text.parse::\u003cf64\u003e() -- the one place this\n module guesses rather than knows\n * src/ui/grid.rs -- formats for the terminal, also overlays\n pending edits\nThree answers, two of which agree. The cheap collapse identified during the\nreview is between the first two, on the records branch only: both want\n\"resolve a synthetic record key through the layout\", so cell_display records\narm can share the resolve_display path via a helper on GridLayout, keeping the\nPENDING-EDIT lookup out of it. That removes the duplicated resolution and,\nwith a real CellValue in hand, the parse::\u003cf64\u003e() inference -- while leaving\nthe staging map session-local and unread by the producer, as decided.\n\nIt is a behaviour change on the records projection, so it wants the failing\ntest first and a check that no existing records test changes meaning.","status":"open","priority":2,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T21:04:41Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:03:13Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-jytm","title":"Split active_view: document default vs per-client authoritative (improvise-g0f)","description":"Implements the user's improvise-g0f decision (2026-09-19): two clients MAY view different views.\n\nTODAY (gotcha 29): Workbook::active_view and ViewState::active_view are two copies of one fact, kept identical by App::switch_to_view and App::install_workbook, guarded by the_two_active_view_names_never_diverge. The second copy exists because reduce_view runs on a client holding no Workbook.\n\nTARGET: Workbook::active_view = the document's default, i.e. what a freshly connected client opens. ViewState::active_view = authoritative for that client. They may differ.\n\nWORK:\n1. Replace the_two_active_view_names_never_diverge with a test of the NEW contract -- do not simply delete it. A deliberate split with no guard is how this silently regresses. Assert that a fresh session opens the document default, and that one client switching does not move another.\n2. Re-examine SwitchView's EffectKind. Under lockstep it is a Model change; per-client it looks like ViewOnly (it must not reach other clients). Check what else SwitchView does before changing the kind -- gotcha 3, a mis-declared kind is silent.\n3. ClientSession::project must emit that client's whole widget set on a switch: every projection is view-dependent, and the grid mirror describes the OLD view.\n4. Persistence: Initial View in .improv is the document default, which is now exactly the right meaning -- confirm the writer still takes it from Workbook and not from any client.\n5. Update gotcha 29 in context/repo-map.md (orchestrator owns that file) and the kb decision recording the old lockstep rule.\n\nUnblocks improvise-cqq.","notes":"\n--- GUIDING PRINCIPLE (user, 2026-09-19) ---\n'The model is shared and the UI isn't.' g0f follows from this rather than\nbeing an independent choice: which view you are looking at is UI, not\ndocument. The principle is already embodied in EffectKind -- ViewOnly is UI\n(reaches nobody else), Model(...) is document (reaches everyone) -- so the\nsplit should make active_view agree with that existing rule rather than\nintroduce a new mechanism.\n\nDO NOT RULE OUT A FOLLOW MODE. The user explicitly wants the option of\nbinding two UIs together later (one client mirroring another's view, cursor\nand scroll) and explicitly does NOT want it implemented yet. Design\nconsequence for THIS ticket: keep per-client UI state addressable per\nsession -- ClientSession already does this -- so a follow relationship can\nlater be expressed as one session DERIVING its ViewState from another's.\nDo not 'simplify' by making any UI state globally shared to get follow\nbehaviour for free; that would trade the principle for the feature and is\nthe one shape that forecloses it.\n\n--- MUST REVISIT improvise-kvyt WHEN DOING THIS (2026-09-19) ---\nCopilot found on PR #10 that a view switch moved only the ASKING session mirror,\nstaling every bystander cursor: each client grid is built by grid_of from\nworkbook.active_view(), so a bystander left naming the outgoing view was\nprojected the NEW view rows positioned by the OLD view scroll. Fixed in 8475b2d\nby broadcasting the ACCEPTED view to every mirror, with Sessions::apply now\nreturning Applied { dirty, broadcast } so the host can forward the switch to the\nother clients, whose slices live on them.\n\nThat fix is correct ONLY for the current lockstep model. Under this ticket split\n-- active_view per-client, the model is shared and the UI is not -- a view switch\nbecomes the asker business alone and the broadcast either disappears or changes\nmeaning entirely. Do NOT keep it by inertia: re-derive whether anything still has\nto reach the other sessions when the asker switches, and if the answer is nothing,\ndelete the broadcast and the test that pins it rather than leaving a mechanism\nwhose reason has gone. If the answer is something (e.g. a follow-mode client\nbound to the switcher), that is the shape follow mode wants -- see the\nmodel-shared-ui-not-shared memory, which says explicitly not to foreclose it.","status":"open","priority":2,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T21:00:51Z","created_by":"Edward Langley","updated_at":"2026-09-19T21:53:13Z","dependency_count":0,"dependent_count":0,"comment_count":0}
@@ -118,7 +119,7 @@
{"_type":"issue","id":"improvise-pby","title":"2.2 Write the README","description":"Replace existing README.md with new one under 250 lines. Sections in order: Title, one-sentence pitch, inline demo GIF, why this exists (Lotus Improv reference), quick start, key bindings, installation (Nix/crates.io/prebuilt - no Homebrew), codebase overview, expectations disclaimer, license. No badges, TOC, or contributing guide.","status":"closed","priority":2,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:07:32Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:02:52Z","close_reason":"README written with all 10 required sections, committed. License updated to Apache-2.0 per user request.","dependency_count":0,"dependent_count":2,"comment_count":0}
{"_type":"issue","id":"improvise-ihv","title":"2.1 Add Nix tooling for asciinema, VHS, and cargo-dist","description":"Modify flake.nix to add pkgs.asciinema and pkgs.vhs to dev shell nativeBuildInputs. Add cargo-dist if packaged in nixpkgs. Add runtime deps for VHS (ttyd, ffmpeg) if needed. Verify tools work via nix develop --command. Add nix run app or shell script for demo recording workflow.","status":"closed","priority":2,"issue_type":"task","assignee":"Edward Langley","owner":"el-github@elangley.org","created_at":"2026-04-09T04:07:30Z","created_by":"Edward Langley","updated_at":"2026-07-30T23:06:31Z","closed_at":"2026-04-09T07:30:19Z","close_reason":"asciinema, vhs, cargo-dist added to flake.nix dev shell and verified. cargo-dist wrapper enables 'cargo dist' subcommand. scripts/record-demo.sh added.","dependency_count":0,"dependent_count":3,"comment_count":0}
{"_type":"issue","id":"improvise-yk4","title":"Phase 2: README and demo artifacts","description":"The main launch work. README is 80% of the launch, demo artifacts are 20%. Covers Nix tooling for asciinema/VHS, writing the README, creating demo GIF and asciinema casts, and verifying all artifacts.","status":"closed","priority":2,"issue_type":"feature","assignee":"spot","owner":"el-github@elangley.org","created_at":"2026-04-09T04:05:49Z","created_by":"Edward Langley","updated_at":"2026-04-14T07:47:43Z","closed_at":"2026-04-14T07:47:43Z","close_reason":"All 5 subtasks closed; artifacts verified present and tracked in git (README.md, docs/demo.gif, docs/demo.tape, docs/casts/*.cast).","dependencies":[{"issue_id":"improvise-yk4","depends_on_id":"improvise-abz","type":"blocks","created_at":"2026-04-08T23:37:37Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-d4w","type":"blocks","created_at":"2026-04-08T23:37:36Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-ihv","type":"blocks","created_at":"2026-04-08T23:37:34Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-odx","type":"blocks","created_at":"2026-04-08T23:37:35Z","created_by":"Edward Langley","metadata":"{}"},{"issue_id":"improvise-yk4","depends_on_id":"improvise-pby","type":"blocks","created_at":"2026-04-08T23:37:36Z","created_by":"Edward Langley","metadata":"{}"}],"dependency_count":5,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-ry89","title":"cv_regress/cv_classify with k=1 now return plausible numbers instead of NaN","description":"Wave-4 review (2026-09-19), CONFIRMED, LOW. A signal loss, not a wrong answer.\n\nmean_over(total, 0) == 0.0 replaced a 0.0/0.0 NaN, which is right everywhere it is used -- the reviewer looked for a case where an empty set WINS a comparison it should lose and found none (inside fit every candidate scores the same rows; cv_* pool pairs rather than reduce to a per-fold metric).\n\nBut with k=1, folds(n,1) puts every index in the single fold, so the training complement is EMPTY and the model trains on nothing. Pre-change, standardize(\u0026[]) gave mean = NaN and every pooled prediction was NaN -- the degenerate call announced itself. Post-change:\n k=1 pooled[0] = (1.48e-6, 8.0), nan_predictions = 0\nSo a library caller passing k=1 gets 37 near-zero cross-validated predictions from a tree fit on nothing, and anyone computing R2 off them gets a number rather than a NaN.\n\nexamples/fit_csv.rs guards with k \u003e= 2, so no shipped path hits it. Fix by documenting on cv_regress/cv_classify that k\u003c2 is meaningless, or by returning empty for k\u003c2. Note the folds doc argues max(1) is \"the answer the shape gives\" -- true for folds, and no longer true for the cross-validators.\n\nNIT from the same review: standardize_is_bit_for_bit_what_it_was is insensitive -- both perturbations of standardize left it green (one was caught by cv_regress instead). Its four witnesses do not separate accumulation orders on the 37-row fixture. Worth strengthening while here.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T22:18:04Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:18:04Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-ry89","title":"cv_regress/cv_classify with k=1 now return plausible numbers instead of NaN","description":"Wave-4 review (2026-09-19), CONFIRMED, LOW. A signal loss, not a wrong answer.\n\nmean_over(total, 0) == 0.0 replaced a 0.0/0.0 NaN, which is right everywhere it is used -- the reviewer looked for a case where an empty set WINS a comparison it should lose and found none (inside fit every candidate scores the same rows; cv_* pool pairs rather than reduce to a per-fold metric).\n\nBut with k=1, folds(n,1) puts every index in the single fold, so the training complement is EMPTY and the model trains on nothing. Pre-change, standardize(\u0026[]) gave mean = NaN and every pooled prediction was NaN -- the degenerate call announced itself. Post-change:\n k=1 pooled[0] = (1.48e-6, 8.0), nan_predictions = 0\nSo a library caller passing k=1 gets 37 near-zero cross-validated predictions from a tree fit on nothing, and anyone computing R2 off them gets a number rather than a NaN.\n\nexamples/fit_csv.rs guards with k \u003e= 2, so no shipped path hits it. Fix by documenting on cv_regress/cv_classify that k\u003c2 is meaningless, or by returning empty for k\u003c2. Note the folds doc argues max(1) is \"the answer the shape gives\" -- true for folds, and no longer true for the cross-validators.\n\nNIT from the same review: standardize_is_bit_for_bit_what_it_was is insensitive -- both perturbations of standardize left it green (one was caught by cv_regress instead). Its four witnesses do not separate accumulation orders on the 37-row fixture. Worth strengthening while here.","notes":"\n--- PARTIALLY ADDRESSED on PR #11 (2026-09-19) ---\nCopilot independently re-found this. The PR agent DOCUMENTED it rather than\nguarding it, and the reasoning is right: returning empty for k \u003c 2 is a\nbehaviour change to the exact functions scoring_known_answers.rs pins\nbit-for-bit, inside a PR whose entire claim is bit-identity. It deserves its\nown commit where it is the headline rather than a rider on a reshape.\n\nSTILL OPEN: decide guard vs document-only. If guarding, the known-answer\nvectors must be re-established afterwards, not assumed -- that is the whole\npoint of them.","status":"open","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T22:18:04Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:50:03Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-k2i4","title":"The two nullary model commands are unaddressed and act on one client's staged state","description":"Wave-4 review (2026-09-19), CONFIRMED, LOW. Worth a decision and a note rather than an immediate change, since multi-client is not wired.\n\nApplyAndClearDrill and AcceptWizardModel are safe no-ops when nothing is staged (both early-return), so a client cannot corrupt the document by sending them blind. Two residual issues:\n\n(a) SPURIOUS PROJECTIONS. Dirty comes from the EffectKind, not from whether anything happened, so a no-op still dirties everything at that level and reaches every client. Measured: ApplyAndClearDrill dirties grid+category_tree+formulas; AcceptWizardModel dirties those plus view_list+tile_bar, emitting a non-empty projection list for a no-op.\n\n(b) THE STAGED STATE IS GLOBAL AND THE COMMAND IS NOT ADDRESSED. Sessions::apply routes Command::Model with no SessionId, while drill_state and wizard live on the single authoritative App. So client B sending Model(ApplyAndClearDrill) commits client A uncommitted staged edits, and Model(AcceptWizardModel) installs the workbook A half-finished wizard built -- replacing the document under everyone.\n\nThat collides with the user ruling on improvise-tom1: staged drill edits belong to whoever staged them and must not be visible to others. Committing them on someone else behalf is the stronger version of the same violation. Gotcha 30 calls this state session-local residue; the command layer now lets an unaddressed message act on it.\n\nDECIDE: either these commands become addressed (a SessionId, and the effect acts only on that session staged state -- which means the staging map moves off the single App), or they are removed from the wire vocabulary and stay key-path only. The second is cheaper and consistent with tom1.","status":"open","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T22:18:03Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:18:03Z","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-ls3y","title":"[bug] A wire-driven SwitchView does not push the view history; the effect does","description":"Found during improvise-kmgs (2026-09-19), pre-existing, out of that ticket scope.\n\napply_command SwitchView arm calls app.switch_to_view(name) DIRECTLY, while effect::SwitchView first pushes a ViewFrame onto view_back_stack and clears the forward stack. So a switch driven over the wire is NOT undoable by ViewBack, and a key-driven one is -- two spellings of one user action that differ in whether history records it.\n\nSurfaced while building a ViewBack session test: the test had to walk forward through the EFFECT to fill the stack, because the command path never fills it. That asymmetry is the tell.\n\nDecide which is right rather than blindly making them agree: history is arguably UI state (the model is shared, the UI is not), in which case the per-client stack should be fed by whichever door the client used -- and then the command path is the one that is wrong. Note improvise-jytm will make active_view per-client, which touches the same ground.","status":"closed","priority":3,"issue_type":"bug","owner":"el-github@elangley.org","created_at":"2026-09-19T22:11:30Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:50:02Z","closed_at":"2026-09-19T22:50:02Z","close_reason":"Found again by Copilot on PR #11 and fixed in ee767f9, merged as eb0c271. apply_command performed Command::SwitchView itself, calling App::switch_to_view directly instead of running effect::SwitchView -- and the EFFECT is the only thing that pushes onto view_back_stack and clears view_forward_stack. It was also the last hand-rolled mutation left in apply_command, which sat badly against the PR own \"effects are the only mutation site\" thesis.\n\nWAVE 4 IS WHAT MADE IT BITE: ModelCommand::ViewBack pops exactly that stack, so a wire client could switch views and then be unable to go back, while the same two keypresses worked. Before ModelCommand there was no wire way to ask, so the missing push was invisible.\n\nDemonstrated first: a_wire_view_switch_records_history_the_way_a_key_switch_does failed with the document still on Second. Nothing else in the suite could see it -- switch_view_moves_the_document_and_the_client_together checks where a switch LANDED, not what it RECORDED.\n\nPart of Copilot suggestion was declined with reasons on the thread: the arm still returns a hand-written Dirty rather than Dirty::of(Model(LayoutGeometry)), because Dirty::of is a per-KIND over-approximation and a view switch is the one member of that kind which leaves the view list alone -- which view is ACTIVE is not in ViewListCache at all. switching_views_projects_the_axis_widgets_only pins that.","dependency_count":0,"dependent_count":0,"comment_count":0}
{"_type":"issue","id":"improvise-dkqn","title":"gp-tree::fit should take one slice of rows, not parallel x/y slices","description":"Filed from Copilot PR #10 round 2 (2026-09-19). The PR agent DECLINED the literal request and was right to; this is the real remedy.\n\nCopilot asked five public scoring functions (mse, cost, fit, cv_regress/cv_classify, permutation_importance) to enforce their basic preconditions -- i.e. validate that the parallel x and y slices have matching lengths. That was declined on the grounds that validating a parallel length is exactly the ASSERTED form improvise-n3wt removed from prune_axis_items in the very same PR (make the invariant unrepresentable, do not check it), and that Result on five functions is what AGENTS.md means by keeping Option/Result at the boundaries. The reasoning was posted on the PR thread citing both.\n\nTHE FIX IS A TYPE CHANGE: give these functions one slice of rows rather than parallel x/y. That removes the zip truncation Copilot identified -- the thing that made mse wrong -- and answers the empty-input NaN in standardize/misclass_rate/mse in the same move, because an empty row set is then one shape rather than four independent edge cases.\n\nCONSTRAINT: wave 3 established byte-identical CLI output across nine scenarios including predictions and the saved model. That evidence must be re-established after this change, not assumed -- the whole point of that harness is that a refactor of the search cannot be trusted on a green suite alone.","status":"closed","priority":3,"issue_type":"task","owner":"el-github@elangley.org","created_at":"2026-09-19T22:01:11Z","created_by":"Edward Langley","updated_at":"2026-09-19T22:11:20Z","closed_at":"2026-09-19T22:11:20Z","close_reason":"Row\u003cT\u003e { feat: Vec\u003cf32\u003e, target: T } in fit.rs -- generic in the target because the two supervised shapes differ only there (Row\u003cf32\u003e regression, Row\u003cusize\u003e class code). mse/cost/fit/misclass_rate/standardize/cv_regress/cv_classify/permutation_importance all take one slice, so the parallel lengths zip used to truncate silently cannot be built. Rejected: a Rows newtype with a checked constructor (validation wearing a type -- the thing the doctrine declines), a bare tuple slice, and Result on five signatures. The pairing is made at the boundary that BUILDS the data: preprocess walks one row index and the rows live inside Target, because a Dataset carrying rows and target separately just moves the zip up one level into every caller. Empty input answered once in mean_over -- no rows scored is no error found, not the 0.0/0.0 NaN that propagated through cost and sorted unpredictably against real costs.\n\nBIT-IDENTITY ESTABLISHED, NOT ASSERTED: known-answer vectors were captured from the UNMODIFIED code first -- raw f32::to_bits for every scoring path plus rng.next_u64() after each stochastic run, which is what pins the generator call order and count (SplitMix64::seed() returns the original seed and witnesses nothing). Those literals are now tests/scoring_known_answers.rs (6 tests) and pass unchanged. 37 rows is prime, so no fold count divides evenly and the short last fold is exercised. Nine-scenario CLI byte-identity re-established including model.json sha256. Note the CLI prints 3-4 decimals, which is exactly why the bit-level tests exist: the diff alone could not see a moved accumulation.","dependency_count":0,"dependent_count":0,"comment_count":0}
+150
View File
@@ -0,0 +1,150 @@
# ADR 001 — Concurrent edits between clients
**Status:** accepted (2026-09-19) · **Supersedes:** nothing · **Decided by:** the user
## Context
`improvise-cqq` made the client/server split real: one authoritative `App`, a
`ClientSession` mirror per client, and projections diffed against those mirrors.
`improvise-kmgs` then put the 28 model mutations on the wire. Two clients can now
edit the same document, and nothing decides what happens when they edit the same
part of it.
### There is already a total order
Every `ModelCommand` reaches the document through one path:
`Sessions::apply` → `apply_command` → `App::apply_effects`, against a single `App`.
So the server already serialises writes. **We do not need to build linear history —
it exists.**
What we lack is a way to notice that a client composed an edit against a picture of
the document that has since moved. This is a *stale basis* problem, not an ordering
problem — the same shape as the stale cursor (gotcha 24) and the stale session
mirror after a document swap (gotcha 39), one level up.
### What actually conflicts
1. **Two writers, same record.** Survivable under last-write-wins: someone's number
is lost, the document stays coherent.
2. **Shape change vs. cell edit.** A removes category `Region` while B commits an
edit keyed by `Region=East`. B's `CellKey` names a coordinate that no longer
exists. This is *corruption*, not a lost keystroke, and it is the class a
cell-level lock does not cover — holding `Region=East × Revenue` does not stop
anyone deleting `Region`.
3. **Formula vs. data.** A adds a formula targeting the measure B is typing into;
B's raw value is shadowed by a derived one.
### Machinery that already exists
- `App::document_generation` — bumped by `install_workbook`, consumed by
`ClientSession::project` to reseed a stale mirror (gotcha 39).
- `ModelChange::{CellData, LayoutGeometry, WholeWorkbook}` — the effect taxonomy,
already the single classification for what a mutation reaches (gotcha 35).
- `record_row_identity(wide, coord_group_excluding_measure)` — a *record* is the
coordinate tuple excluding `_Measure` (gotcha 1).
- Drill staging — `pending_edits` applied atomically by `ApplyAndClearDrill`, and
ruled **private to whoever staged them** (improvise-tom1).
## Decision
**Record-level atomicity with optimistic concurrency per batch.**
1. **The unit of atomicity is the record**, i.e. the coordinate tuple excluding
`_Measure` — the thing `record_row_identity` already names. Not the cell:
gotcha 1 says editing a category column in records mode moves *every* measure
cell in the row, so a cell-level unit would be violated by a single legal edit.
The unit must be at least as coarse as the coarsest legal write.
2. **The unit of commit is the batch** — one `Vec<Box<dyn Effect>>` applied by one
`apply_effects`. This is already atomic in practice: a `Binding::Sequence`
flattens into one batch, and a failed `Save` aborts the rest (gotcha 21).
3. **A batch carries the basis it was composed against**, and the server rejects it
if that basis moved. The basis is **tiered by `ModelChange`**, which is what
makes class 2 fall out for free:
| tier | bumped by | a batch is stale if |
|---|---|---|
| `document_generation` | `WholeWorkbook` | it moved at all |
| `layout_version` | `LayoutGeometry` | it moved at all |
| `record_version` | `CellData` on that record | any record the batch touches moved |
A `CellData` batch is stale if *layout* moved, because the shape its keys hang
off changed. That is class 2, detected without a lock.
4. **Rejection is a normal outcome, not an error.** The client already receives the
new state through projections; it re-composes against the new basis. `Applied`
grows a third case alongside `Diff` and `Rebased` — matching is what forces a
host to handle it (gotcha 39's reasoning).
### Optional second layer: advisory reservations
The staged-edit mechanism from drill, generalised: a client publishes the *keys* it
has staged so others can grey them out and warn early. The *values* stay private,
per improvise-tom1.
This layer is **advisory only**. If a reservation is lost, never arrives, or leaks
from a crashed client, the system degrades to "you find out at commit" — never to
corruption. That is what makes it safe to be sloppy about timeouts and disconnects,
and it is why correctness must not be built on it.
## Why not a causal clock
Considered and rejected, for four reasons.
1. **The server already serialises.** Causal clocks earn their keep where no single
node orders writes — peer-to-peer, or offline-capable clients merging on
reconnect. With an authoritative `App` we need *staleness detection* (one
counter per unit), not *concurrency detection* (a vector per client).
2. **A Lamport clock is insufficient.** It gives a total order but cannot tell you
two edits were concurrent, which is exactly the question.
3. **A vector clock is sufficient but does not answer the question that matters.**
It costs O(clients), needs entries reaped for departed clients, and when it
reports "these were concurrent" you still need a policy for class 2 — no clock
says whether B's edit into `Region=East` should survive A's deletion of `Region`.
The clock tells you there is a decision to make; it does not make it.
4. **CRDTs converge, but not necessarily to something meaningful.** A LWW-register
per cell is easy; the hard part is the *shape* — categories, items, formulas,
axis assignments — where convergence can mean an orphaned cell or a resurrected
category.
If offline editing ever becomes a goal, this decision should be revisited: that is
the requirement that makes a vector clock worth its cost, and it is a much larger
commitment than multi-client-while-connected.
## Consequences
**Good.** No lock to acquire, so no timeout, no lease renewal, no disconnect
handling, and no liveness risk — a crashed client cannot wedge anyone. Correctness
does not depend on the advisory layer. The tiered basis reuses `ModelChange`, so
there is no second classification to drift from gotcha 35's table. Class 2 is
detected rather than prevented, which is cheaper and does not require predicting
which shape changes matter.
**Bad.** A client can compose an edit and be told no; the UI must handle re-entry
gracefully rather than losing typing. Record-level granularity means two clients
editing *different measures of the same record* serialise against each other even
though their writes do not overlap — acceptable at the expected scale, and
refinable to `(record, measure)` if contention shows up. A busy `LayoutGeometry`
editor invalidates every in-flight `CellData` batch, which is correct but could feel
aggressive.
**Not solved here.** Class 3 (formula shadows a raw value) is detected — the formula
bumps `layout_version` — but the *policy* is unstated: the edit is rejected, and
whether that is the right answer for the user is a separate question. Nor does this
address a client that wants to keep editing while disconnected.
## Implementation order
Deliberately two tickets, so the correctness floor lands without waiting on the UX.
1. **Basis check.** Version tiers, `ModelCommand` carries its basis, `Applied` grows
the rejected case. Includes the decision of where the version counters live —
they are shared document state, so they belong beside `document_generation` on
the authoritative `App`, not on `ViewState`.
2. **Advisory reservations.** Generalise drill staging; publish key sets, keep values
private.
The nullary wire variants `ApplyAndClearDrill` and `AcceptWizardModel` must be
resolved first or alongside: they are currently unaddressed (no `SessionId`) while
acting on one client's staged state, which this design makes untenable.