docs: #300 review fixes — fdk1 clause, stacked-PR rule, run: shell scope, §7 heuristic, fifteen not sixteen
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
@@ -545,9 +545,7 @@ decision('improvise-6qet',a_collapsed_group_is_one_aggregate_row,'USER RULING 20
|
||||
decision('improvise-b4ks',fmt_per_root_by_member,'This repo has TWO workspace roots -- harness/opfs-contract declares its own [workspace] -- and BOTH obvious spellings fail in the direction that looks like success, each measured. \`cargo fmt --all\` (or --manifest-path <root> --all) CROSSES THE BOUNDARY, because --all formats local PATH DEPENDENCIES and harness/opfs-contract depends on improvise-io by path, so the inner root reported src/draw.rs; this is wider than the ticket note, which blamed only cd-then---all. And bare --manifest-path is BLIND TO MEMBERS, since the repo root is a package as well as a workspace -- an injected fault in crates/improvise-core exited 0. The only sound spelling is \`-p <each member>\` scoped by that root own manifest. Note \`cargo fmt -p opfs-contract\` FROM THE REPO ROOT exits 1 with \"not a member of the workspace\", indistinguishable from a real verdict, which is the ticket itself; from INSIDE that root it is correct, which is why CI harness-lint job was always sound while the root-level gate every brief prescribed was checking less than it claimed.').
|
||||
decision('improvise-f9qf',one_checker_entry_point,'scripts/checkers.sh is the ONE entry point for every check_the_checker, called verbatim by CI and by the local gate so there is no second invocation to drift. It SCRUBS THE ENVIRONMENT to an allowlist (env -i plus PATH/HOME/TMPDIR/NIX_*/GITHUB_*, extensible via CHECKERS_KEEP) and refuses any shell shebang in scripts/ that is not #!/usr/bin/env bash. It used to pin a bash >= 5.1 as well (ambient first, nix only when the ambient one was older, exit 3 when neither), and that half is GONE since #297 (improvise-3uow, 2026-09-24): CI moved to NixOS runners where every step runs in nix develop .#ci, and the user ruled that scripts must not care which bash or standard utilities are present. So the runner calls each suite with $BASH and an unchanged PATH, and a script needing a newer feature fails where it uses it. Which suites each CI job runs is the ci_suites table in checkers.sh (checkers.sh --ci <job>), held to checkers.sh --list by check-ci-graph.py. context/code/tooling.md section 5.').
|
||||
decision('improvise-f9qf',nix_shebang_declined_on_availability_not_speed,'Measured warm, 20 invocations: ambient bash -c true 5.1ms; nix shell nixpkgs#bash --command 61.8ms; nix-shell -p bash --run, THE MANPAGE FORM THE USER POINTED AT, 675.2ms -- 132x, and that inside git merge machinery for beads-merge-driver.sh. But the DECIDER was availability: when this was decided nix was installed on ONLY the kb job, so a nix shebang needed install-nix-action added to three more jobs, paid every run, to reach a bash those runners already shipped. zsh declined too -- not a shebang change (word splitting, 1-based arrays, globbing), sixteen scripts to read, and no zsh on the runner either. SUPERSEDED IN ITS PREMISE on 2026-09-24 (#287, #297, improvise-3uow): every CI job now runs on a self-hosted NixOS runner inside nix develop .#ci, so bash comes from the dev shell everywhere and no shebang needs to reach for nix. The decline stands, for a different reason: the dev shell already supplies the bash, so a nix shebang would add startup cost and buy nothing.').
|
||||
decision('pr-landing-2026-09-24',check_base_ref_before_merging,'Before merging ANY pull request, its baseRefName must be main. A stacked PR whose base PR has already merged still targets the stale base branch, and merging it lands the change there, not in main, while the merge reads as done. On 2026-09-24 sixteen approved stacked PRs (#255 to #286) merged into dead bases, and recovery took seven landing PRs (#289 to #295), each from a stack tip against main. The repository reported delete_branch_on_merge true when measured that day, and the stale bases were still on GitHub afterwards, so that setting is not the guard: the check is. Retarget with gh pr edit <n> --base main once the base PR has landed, then read the diff again. bd memory stacked-pr-merges-into-stale-base.').
|
||||
decision('pr-landing-2026-09-24',land_a_batch_as_one_integration_pr,'Approved PRs that land together land as ONE integration PR, not one at a time. Every PR appends its evidence at the end of a context/test-evidence file, so each single merge makes every other open PR conflict at EOF, and a conflicting PR gets no CI run at all. On 2026-09-24 the day went through #298 (nineteen PRs) and #299 (the browser-editor stack). This is what one PR per wave already meant; the EOF-append convention is what makes it a necessity rather than a preference.').
|
||||
decision('improvise-3uow',ci_takes_its_tools_from_the_ci_dev_shell,'User rulings 2026-09-24. CI runs on self-hosted NixOS microVM runners, and every step runs inside nix develop .#ci through the workflow default shell. A missing tool goes into the .#ci shell in flake.nix, never into a per-step nix shell wrapper. No hard-coded /bin or /usr/bin paths, since those hold only env and sh there. No bash-version floors, no tool-version probes: scripts and tests must not care which bash or standard utilities are present, so checkers.sh lost its bash 5.1 pin and the dev-shell toolchain-pin step left ci.yml (#297). Playwright browsers come from nix via PLAYWRIGHT_BROWSERS_PATH, so there is no browser install step; npm ci stays for @playwright/test. A test that fakes an environment builds it on purpose (a utilbin of linked tools) rather than assuming the runner holds anything. bd memory ci-is-version-agnostic.').
|
||||
decision('adr-process-2026-09-24',an_open_adr_pr_is_watched,'User 2026-09-24: an open ADR PR must be actively watched, so the steward agent acts on the user inline answers and on reviews as they arrive. An answer left unread for hours is a failure. The orchestrator keeps a background poll for non-agent comments and reviews since its last pass, and resumes the steward on each new item. Only the user explicit approval still merges an ADR; the watch is about latency, not authority. bd memory adr-pr-needs-a-watcher.').
|
||||
decision('improvise-wpxr',a_record_resolves_in_the_view_the_command_names,'A SetCell or ClearCell carries the view that drew it, and its RecordHash resolves only when the document has that view: the store index when the record holds cells, else that view layout, where an EMPTY record still exists as a drawn row. A view the document lacks drew nothing, stored or not, so the answer is RecordGone rather than a resolve_view fallback (Codex P2 on #281). RenameRecordCoordinate resolves in the store alone, since only a record holding cells can be renamed. protocol::named_records says which, per command, as NamedRecord::Drawn or NamedRecord::Stored, one exhaustive match.').
|
||||
decision('improvise-kimw',grids_memoise_one_layout_per_view_per_borrow,'App::grids() returns a Grids borrow that derives each named view GridLayout at most once for as long as it lives, so a command asking resolve_record, resolve_cell and unwritable of one view no longer re-derives the layout about five times (and, with formulas, clones and recomputes the model each time). The memo is only sound because it is a BORROW: hold one across a check, never across a mutation, since its layouts describe the model it borrowed.').
|
||||
@@ -559,6 +557,8 @@ decision('improvise-uln5',needs_the_host_is_one_list_held_three_ways,'resolve::N
|
||||
decision('improvise-n0pb',a_client_notice_queues_behind_in_flight,'ClientState::notify folds a client-side status as an outgoing frame at the tail of in_flight, so a rebase that rebuilds the view from settled and replays in_flight replays the notice too instead of erasing it. settle_status runs on every path that rebuilds the view, including rebase (improvise-0dbk), and gives a new message to the editor open as it arrives; EditorSession, that owner key, lives in improvise-protocol beside ViewState (improvise-su4u, improvise-1x9h).').
|
||||
decision('improvise-gk51',uneditable_is_one_decline_for_open_and_commit,'cmd::core::uneditable is the one decline for opening an editor (EnterEditAtCursor) and for committing one (CmdContext::committable_cell_key): a fold or other aggregate, a formula result, or CellId::NonItem, a stored value whose coordinate names no item of its category. Asking only at open let Enter-to-advance commit onto a collapsed group row (improvise-enz6), and projecting a NonItem cell as Record made the host refuse a write the projection had offered. It reads the CellId the projection uses, and the browser declined_at matches on that CellId, so both front ends refuse the same cells.').
|
||||
decision('improvise-i5z6',compare_the_slowest_binary_to_the_next_slowest,'The ratio to the median test binary is degenerate: most binaries print 0.00s at two decimals, so the median is zero and every ratio to it undefined. An outlier is instead a binary that stays well clear of the next few slowest in every run. improvise-formula lib was read that way (longest of 39 in all three runs) and had improvise-8shu own cause: a generator re-parsing its pest grammar per case, fixed by reading it once through a OnceLock (0.84 to 0.98s down to 0.06 to 0.07s).').
|
||||
decision('pr-landing-2026-09-24',check_base_ref_before_merging,'Before merging ANY pull request, its baseRefName must be main. A stacked PR whose base PR has already merged still targets the stale base branch, and merging it lands the change there, not in main, while the merge reads as done. On 2026-09-24 fifteen approved stacked PRs in #255 to #286 merged into a base whose own PR had already merged; recovery took seven landing PRs (#289 to #295), each from a stack tip against main, whose titles name exactly those fifteen. Four more in that range (#259, #263, #272, #273) merged into a non-main base whose PR was still open and reached main with it, which is stacking working as intended. The repository reported delete_branch_on_merge true when measured that day, and the stale bases were still on GitHub afterwards, so that setting is not the guard: the check is. Retarget with gh pr edit <n> --base main once the base PR has landed, then read the diff again. bd memory stacked-pr-merges-into-stale-base (which says sixteen).').
|
||||
decision('improvise-3uow',ci_takes_its_tools_from_the_ci_dev_shell,'User rulings 2026-09-24. CI runs on self-hosted NixOS microVM runners, and every run: step runs inside nix develop .#ci through the workflow defaults.run.shell; a uses: step (an action) does not go through a shell at all. A missing tool goes into the .#ci shell in flake.nix, never into a per-step nix shell wrapper. No hard-coded /bin or /usr/bin paths, since those hold only env and sh there. No bash-version floors, no tool-version probes: scripts and tests must not care which bash or standard utilities are present, so checkers.sh lost its bash 5.1 pin and the dev-shell toolchain-pin step left ci.yml (#297). Playwright browsers come from nix via PLAYWRIGHT_BROWSERS_PATH, so there is no browser install step; npm ci stays for @playwright/test. A test that fakes an environment builds it on purpose (a utilbin of linked tools) rather than assuming the runner holds anything. bd memory ci-is-version-agnostic.').
|
||||
wire_carries(command,both_directions,'user intent up, projection commands down').
|
||||
wire_carries(snapshot,on_connect_only,'improvise-q08: full ViewState + render cache filled from current viewport').
|
||||
wire_carries(command,both_directions,'user intent client->server, projections server->client').
|
||||
|
||||
Reference in New Issue
Block a user