fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'gh/main' into fix/fdk1-coordinate-rename

Browse Source 
# Conflicts:
#	context/test-evidence/command.md
This commit is contained in:
Edward Langley
2026-09-23 07:47:48 -07:00
parent 6319a4be89 12c81dd3d6
commit ee5cba8865
9 changed files with 307 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/test-evidence/command.md
+81
View File
@@ -5519,6 +5519,87 @@ cannot see the feature removed from `improvise-command` alone, because
`improvise-io`, a normal dependency, enables it in the same build. That case is
held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`).
## `ui/effect.rs` — `RecordRewrite`'s untested branches (improvise-phg1)
### renaming_a_drill_row_held_by_a_lone_measureless_cell_moves_that_cell
improvise-phg1(a). A row seeded by `o` (`AddRecordRow`) carries its coordinates
on one cell with no `_Measure`, and `RecordRewrite::of` moves that cell in its
`(false, true)` arm. Replacing the arm with `Vec::new()` left the whole workspace
green, and a renamed `o` row then left `CellKey([])` behind with no coordinates.
The gap predates improvise-uo2p, because `rename_record_coordinate` had the same
arm and nothing tested it. The fixture is that literal case: an empty-coordinate
cell on drill row 1, renamed. Under the rival it fails with `-/-/- = 0` still in
the store, and nothing else in the workspace fails.
### a_measure_a_sparse_drill_row_lacks_is_written_at_the_renamed_record
improvise-phg1(b). An edit to a measure the row has no cell for goes through
`of`'s second loop, which must write at the **renamed** key. Every uo2p fixture
row carried every measure, so that loop only ever ran without a rename, and
writing `row.cell_key(measure)` unrenamed brought uo2p's ghost record back
unseen. Here drill row 1 has `Revenue` but not `Cost`, and stages `Cost` and a
rename. Under the rival it fails with a `Clothing/Jan/Cost = 999` ghost, and
nothing else in the workspace fails.
### every_drill_row_reads_the_model_before_any_row_clears_it
improvise-phg1(c). `RecordRewrite::merged` puts every clear after every read.
The rival lets each row's clears land before the next row reads the model, but
defers every write to the end. The uo2p swap test cannot see **that** rival: a
row's clears aimed at its own old keys touch nothing another row reads. A wider
rival, where each row's clears *and* writes land before the next row reads, is
a different fault: the swap test does catch it (uo2p's rival 2, which fails
`a_drill_that_swaps_two_records_coordinates_swaps_them` alone). So this test
exists for the narrow rival only. What separates it is a clear aimed at
**another row's** key. A blank edit on a measure the row lacks clears that
measure at the row's *renamed* coordinates. So row 0 is renamed onto row 1's
coordinates and blanks `Profit`, which only row 1 has, and row 1 is renamed
away. Row 1's `Profit` must still arrive at `Food/Jan`. Under the rival it is
cleared before row 1 reads it, `Food/Jan/Profit = 5` is lost, and nothing else
in the workspace fails. This probe is the reviewer's, from the ticket.
### a_blank_edit_on_a_measure_a_renamed_row_lacks_clears_it_at_the_renamed_record
improvise-q2mq. `RecordRewrite::of`'s second loop handles the measures a row has
no cell for. Its **clear** arm must aim at the *renamed* key, exactly as its
write arm must (improvise-phg1(b)). The phg1 sparse-row test stages a value, so
it covers only the write arm. Aiming the clear arm alone at
`row.cell_key(measure)`, unrenamed, left the whole workspace green.
The probe is the reviewer's. Row 0 (`Food/Jan`, `Revenue = 1`) is renamed onto
`Clothing`, where row 1 already holds `Profit = 5`, and blanks `Profit`, which
row 0 lacks. The blank belongs to row 0's final record at `Clothing/Jan`, so the
store is exactly `{Clothing/Jan/Revenue = 1}`. Under the rival the clear lands
on `Food/Jan/Profit`, where there is nothing to clear, and
`Clothing/Jan/Profit = 5` survives. Injected alone into a full workspace run,
the rival fails this test and nothing else.
### a_fresh_sessions_slice_is_the_hosts_startup_slice
improvise-v8ab. A fresh session's slice is built twice: `App::with_storage`
builds the host's own, and `project.rs::fresh_view` builds a client's. Nothing
asserted they agree. `tests/lockstep.rs` seeds most of its clients from
`app.view_state.clone()`, and those could not see improvise-lxg0's phantom
frame or the next drift between the two. There are three exceptions.
`switching_between_a_records_view_and_a_pivot_view_settles_both_ways` and
`a_locally_resolved_page_turn_lands_where_the_host_would_put_it` go through
`Host::connect`, and the `published_basis` helper uses `Sessions::connect`.
None of them compares the connected slice with the host's. The test connects to two documents, both opening on `Second`, one with a `Default` view
and one without. It asserts the whole `ViewState` equals the host's, not only
the back stack, so any field the two constructors disagree on fails it. The
rival is the old fold, restored in `fresh_view`. It failed on the
`["Default", "Second"]` case: the only difference was `view_back_stack:
[ViewFrame { view_name: "Default", mode: Normal }]`.
The scope is **connect only**. A rebase deliberately resets the client's whole
slice, while `install_workbook` keeps the host's mode and panels, so equality
is not the contract there. `lockstep.rs` still copies `app.view_state`, and
that is deliberate. Its helpers seed a client *after* mutating the host (mode,
buffers, cursor), so a `Snapshot` would drop exactly the state those tests
stage. This test is what makes the copy faithful at startup.
## A coordinate rename on the wire (improvise-fdk1)
### a_write_to_a_record_its_own_frame_renamed_is_a_broken_chain
context/test-evidence/tooling.md
+44
View File
@@ -719,3 +719,47 @@ every target-specific entry, whose **normal** serde_json dependency lacks
The `checked.len() >= 2` assertion is the positive signal. If a cargo change
renamed the metadata fields, the check would find no serde_json dependency and
pass vacuously without it.
### The re-poll a second boundary could skip — `scripts/pr-reviews-test.sh` (improvise-qnpi)
**What broke.** "an inline comment arriving mid-wait is seen" failed once in
two identical runs of `checkers.sh` on a loaded machine. `pr-reviews --wait`
computed `deadline = now + WAIT` and then tested `now < deadline` *before* its
first re-poll. With `--wait 1`, a second boundary falling between those two
reads of the clock made the loop run zero times: exit 1 with one poll, whatever
the second poll would have shown.
**Reproduced under load.** A loop running that fixture with one busy loop per
core failed once, as `exit 1, polls=1`, around run 52; the fixed script ran
200 times under the same load with no failure. That is evidence of the
mechanism, not proof: the window is a few milliseconds per run.
**The case that proves it.** "a second passing before the first re-poll does
not skip it" puts a `date` on `PATH` that advances one second per read (a
loaded machine, compressed) and a no-op `sleep`. Against the old loop it fails
every time, exit 1 (22 passed, 1 failed). The fix checks the deadline *after*
each poll, so any `--wait` above 0 polls at least once more. A larger timeout
would not have helped: the case uses the shimmed clock, where only the order of
check and poll matters.
### The harness lock that every build rewrote — `harness/build.sh` (improvise-mkbp)
**What broke.** `harness/opfs-contract/Cargo.lock` pinned `pest 2.9.1` while
its path dependencies require `pest = "2.9.2"`, so the committed lock was
invalid for its own manifest and every cargo invocation rewrote it.
**Why both halves.** The resolved lock is committed, and `build.sh` now starts
with `cargo metadata --locked` against it. The lock alone would repeat the bug
at the next workspace dependency bump; `--locked` alone fails today. The check
is its own step because `wasm-pack build … -- --locked` does not work: with the
stale lock restored, that version exited 0 and still rewrote the lock, because
wasm-pack re-resolves before the flag reaches `cargo build`. The standalone
check, against the same stale lock, exits 101 in seconds and leaves the file
untouched.
**The assemble half.** `web/assemble.sh` starts with `rm -rf` of its output,
which is what keeps the deployed tree free of stale files, so it is not taught
to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html`
is served, naming the fix. Injected by running `web/assemble.sh harness/serve`
alone: 4 chromium failures, each carrying that message, where it used to fail
with no hint of the cause.
crates/improvise-command/src/ui/effect.rs
+120 -20
View File
@@ -3166,26 +3166,9 @@ mod tests {
/// Two records, each carrying Revenue and Cost: row 0 is Food/Jan (1, 2),
/// row 1 is Clothing/Jan (42, 17). Every edit below is staged on row 1 at least.
fn two_record_drill() -> App {
let mut app = test_app();
let cells = [
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
(
drill_cell("Clothing", "Jan", "Cost"),
CellValue::Number(17.0),
),
];
for (key, value) in cells.clone() {
app.model_state.workbook.model.set_cell(key, value);
}
let records = RecordRow::group(cells);
assert_eq!(records.len(), 2, "one row per coordinate tuple");
let app = drill_of(&two_record_drill_cells());
let records = &app.drill_state.as_ref().unwrap().records;
assert_eq!(records[1].coords.get("Type"), Some("Clothing"));
StartDrill(records).apply(&mut app);
app
}
@@ -3229,10 +3212,49 @@ mod tests {
/// Stage `edits` and leave the drill, in 32 fresh Apps: each `HashMap` gets its
/// own `RandomState`, and one map iterates the same way twice.
fn assert_drill_commits_to(edits: &[(usize, RecordCol, &str)], expected: &[&str]) {
let cells = two_record_drill_cells();
assert_drill_over(&cells, edits, expected);
}
fn two_record_drill_cells() -> Vec<(CellKey, CellValue)> {
vec![
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
(
drill_cell("Clothing", "Jan", "Cost"),
CellValue::Number(17.0),
),
]
}
/// A drill over exactly `cells`, which must group into two records.
fn drill_of(cells: &[(CellKey, CellValue)]) -> App {
let mut app = test_app();
for (key, value) in cells {
app.model_state
.workbook
.model
.set_cell(key.clone(), value.clone());
}
let records = RecordRow::group(cells.iter().cloned());
assert_eq!(records.len(), 2, "one row per coordinate tuple");
StartDrill(records).apply(&mut app);
app
}
fn assert_drill_over(
cells: &[(CellKey, CellValue)],
edits: &[(usize, RecordCol, &str)],
expected: &[&str],
) {
let expected: std::collections::BTreeSet<String> =
expected.iter().map(|s| s.to_string()).collect();
for attempt in 0..32 {
let mut app = two_record_drill();
let mut app = drill_of(cells);
for (row, col, value) in edits {
stage(&mut app, *row, col.clone(), value);
}
@@ -3241,6 +3263,84 @@ mod tests {
}
}
#[test]
fn renaming_a_drill_row_held_by_a_lone_measureless_cell_moves_that_cell() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(CellKey::new(vec![]), CellValue::Number(0.0)),
],
&[(1, category("Type"), "Drink")],
&["Food/Jan/Revenue = 1", "Drink/-/- = 0"],
);
}
#[test]
fn a_measure_a_sparse_drill_row_lacks_is_written_at_the_renamed_record() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
],
&[(1, measure("Cost"), "999"), (1, category("Type"), "Drink")],
&[
"Food/Jan/Revenue = 1",
"Food/Jan/Cost = 2",
"Drink/Jan/Revenue = 42",
"Drink/Jan/Cost = 999",
],
);
}
#[test]
fn every_drill_row_reads_the_model_before_any_row_clears_it() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(
drill_cell("Clothing", "Jan", "Revenue"),
CellValue::Number(42.0),
),
(
drill_cell("Clothing", "Jan", "Profit"),
CellValue::Number(5.0),
),
],
&[
(0, category("Type"), "Clothing"),
(0, measure("Profit"), ""),
(1, category("Type"), "Food"),
],
&[
"Clothing/Jan/Revenue = 1",
"Food/Jan/Revenue = 42",
"Food/Jan/Profit = 5",
],
);
}
#[test]
fn a_blank_edit_on_a_measure_a_renamed_row_lacks_clears_it_at_the_renamed_record() {
assert_drill_over(
&[
(drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)),
(
drill_cell("Clothing", "Jan", "Profit"),
CellValue::Number(5.0),
),
],
&[
(0, category("Type"), "Clothing"),
(0, measure("Profit"), ""),
],
&["Clothing/Jan/Revenue = 1"],
);
}
#[test]
fn a_drill_row_renamed_twice_and_edited_lands_as_one_record() {
assert_drill_commits_to(
crates/improvise-command/src/ui/project.rs
+12
View File
@@ -1606,6 +1606,18 @@ mod tests {
}
}
/// improvise-v8ab: a fresh client starts from the host's own startup slice.
/// See `context/test-evidence/command.md`.
#[test]
fn a_fresh_sessions_slice_is_the_hosts_startup_slice() {
for names in [&["Default", "Second"][..], &["Main", "Second"][..]] {
let app = App::new(opening_on(names, "Second"), None);
assert_eq!(app.view_state.active_view, "Second", "setup: {names:?}");
let (_, snapshot) = Sessions::default().connect(&app, 10, 10);
assert_eq!(snapshot.view, app.view_state, "{names:?}");
}
}
/// improvise-lxg0: a document swap reseeds through the same door.
#[test]
fn a_rebased_session_has_no_history_to_step_back_through() {
harness/build.sh
+11
View File
@@ -9,10 +9,21 @@
# half is the *deployed* arrangement rather than `web/` itself, because a
# harness that runs against a different tree from the one that ships is
# testing something nobody deploys (improvise-vvl3).
#
# Re-run THIS script, never `web/assemble.sh harness/serve` alone: assembling
# starts with `rm -rf` of its output, which deletes opfs/ and the fixtures
# (improvise-mkbp). opfs.spec.js says so if it finds opfs/ missing.
set -eu
root=$(cd "$(dirname "$0")/.." && pwd)
serve="$root/harness/serve"
# A stale opfs-contract/Cargo.lock fails HERE rather than being rewritten in
# every agent's tree (improvise-mkbp). wasm-pack cannot enforce this: it
# re-resolves the lock itself before any `-- --locked` reaches cargo build.
# Fix a failure with `cargo update -w` in harness/opfs-contract, and commit it.
cargo metadata --locked --format-version 1 \
--manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null
"$root/web/build.sh" >/dev/null
"$root/web/assemble.sh" "$serve" harness >/dev/null
harness/opfs-contract/Cargo.lock
Generated
+8 -8
View File
@@ -308,9 +308,9 @@ dependencies = [
[[package]]
name = "pest"
version = "2.9.1"
version = "2.9.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6d45aeb61b4bf818e12d4205f2466f8c4748f85f4fce0146d1c03d69d753f0ad"
checksum = "45d3aca230fad2e6f6317ca0a72724338c4960cb97168a85cdee66df4a9a21a8"
dependencies = [
"memchr",
"ucd-trie",
@@ -318,9 +318,9 @@ dependencies = [
[[package]]
name = "pest_derive"
version = "2.9.1"
version = "2.9.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "89cc5a242e25ed4e7704d0be240f2cfbe20a8c27e7e252d94835be93d92dc39f"
checksum = "284b60557f2c4a2e72ad3f2d34d42685a2fa4a6a61d0d2a10c0ae2a5e916c2cf"
dependencies = [
"pest",
"pest_generator",
@@ -328,9 +328,9 @@ dependencies = [
[[package]]
name = "pest_generator"
version = "2.9.1"
version = "2.9.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7abf21475cc3820fe4b2ca2dc2142902f67a02189f3b5b3a229f4febc01a43e5"
checksum = "1d9d1f08a115309ee99268cf85e5228e0e56aa9caf8841ec12866b6be07c3109"
dependencies = [
"pest",
"pest_meta",
@@ -341,9 +341,9 @@ dependencies = [
[[package]]
name = "pest_meta"
version = "2.9.1"
version = "2.9.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "adba4db388f687393c18c51348d44a41d870ca9df71a2c98172ea3035dc6936e"
checksum = "ed93ba1a9ffcca32130a5188701c81c0c49cf00d4b7c5007d5148951d743adcb"
dependencies = [
"pest",
]
harness/tests/opfs.spec.js
+5 -1
View File
@@ -23,7 +23,11 @@ const EXPECTED = [
test.describe("OpfsFs in a real browser", () => {
test.beforeEach(async ({ page, browserName }, testInfo) => {
await page.goto("/opfs/index.html");
const served = await page.goto("/opfs/index.html");
expect(
served?.ok(),
"harness/serve/opfs is missing: re-run harness/build.sh, not web/assemble.sh alone",
).toBe(true);
// The skip is conditional on the PROBE, not on the browser name, so it
// cannot quietly swallow a real regression. See clients.md.
scripts/pr-reviews
+6 -2
View File
@@ -9,7 +9,8 @@ usage() {
usage: pr-reviews <pr-number> [--wait <sec>] [--interval <sec>]
--wait <sec> wait up to this long for a review or an inline review
comment whose commit is the current head
comment whose commit is the current head; any --wait
above 0 polls at least once more after the first read
(default 0 = print what exists and exit)
--interval <sec> seconds between polls (default 30)
@@ -157,8 +158,10 @@ report "$json"
if has_head_review "$json"; then exit 0; fi
# The deadline is checked AFTER each poll: checked first, a second ticking over
# between two reads of the clock skipped the only re-poll (improvise-qnpi).
deadline=$(( $(date +%s) + WAIT ))
while [ "$(date +%s)" -lt "$deadline" ]; do
while :; do
sleep "$INTERVAL"
json=$(fetch)
if has_head_review "$json"; then
@@ -166,6 +169,7 @@ while [ "$(date +%s)" -lt "$deadline" ]; do
report "$json"
exit 0
fi
[ "$(date +%s)" -lt "$deadline" ] || break
done
printf '\npr-reviews: waited %ss; nothing against the current head.\n' "$WAIT"
scripts/pr-reviews-test.sh
+20
View File
@@ -197,6 +197,26 @@ mk "$d" last.issue.json <<'EOF'
EOF
run_case "an inline comment arriving mid-wait is seen" 0 "$d" --wait 1 --interval 1
# The same fixture on a clock that ticks one second per read: a loaded machine,
# compressed. A second passing between two reads must not cost the only poll.
clock="$work/clock"
mkdir -p "$clock"
cat > "$clock/date" <<'EOF'
#!/usr/bin/env bash
[ "${1:-}" = "+%s" ] || exec /bin/date "$@"
n=$(( $(cat "$CLOCK_TICKS" 2>/dev/null || echo 0) + 1 ))
echo "$n" > "$CLOCK_TICKS"
echo $(( 1000 + n ))
EOF
printf '#!/usr/bin/env bash\nexit 0\n' > "$clock/sleep"
chmod +x "$clock/date" "$clock/sleep"
rm -f "$d/.poll" "$work/ticks"
CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$d" \
"$REVIEWS" 1 --wait 1 --interval 1 > "$work/out" 2>&1
got=$?
[ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \
|| bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0"
# ---------------------------------------------------------------------------
# `gh api --paginate` emits one array PER PAGE, so a thread past the page size
# arrives as several concatenated documents. Passing that straight to