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

Merge remote-tracking branch 'gh/main' into fix/6yvg-dz8c-json-float

Browse Source 
# Conflicts:
#	context/test-evidence/command.md
#	context/test-evidence/tooling.md
This commit is contained in:
Edward Langley
2026-09-22 22:19:30 -07:00
parent 95a35e8cce 608ba6a6c8
commit feefe17d53
28 changed files with 1947 additions and 372 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.agents/skills/pr-watch/SKILL.md
+18 -4
View File
@@ -10,26 +10,40 @@ scripts/pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec
```
```
exit 0 every check COMPLETED and none failed
exit 1 a check failed, or the PR is CONFLICTING
exit 0 every check COMPLETED, none failed, and MERGEABLE with a state of
CLEAN, UNSTABLE or HAS_HOOKS
exit 1 a check failed, or the PR is not mergeable (CONFLICTING, BLOCKED,
DIRTY, BEHIND, DRAFT, or a state it does not know)
exit 2 the head moved during the wait, or is not --expect
exit 3 timed out with checks still running
exit 3 timed out with checks still running, mergeability still UNKNOWN,
or still BLOCKED with every registered check green
exit 4 usage or environment error
```
**Exit 0 now means mergeable, not only green.** GitHub computes mergeability
lazily and reports `UNKNOWN` for a while after the base moves. It also reports
`BLOCKED` for the moment before a required check that `needs:` another job has
registered. The watch waits out both, so you do not need to re-check
`mergeable` by hand after it. **Exit 3 is not a pass.** It may be a required
check that never ran. Why each
state counts the way it does: `context/code/tooling.md`, "pr-watch and
mergeability" (improvise-edpl).
**Pass `--expect <sha>` whenever you know which commit you are asking about.**
It is the difference between "this PR is green" and "the thing I pushed is
green", and a PR here was once merged at a head that predated its own fix.
## Do not write the loop yourself
Three signals look like "finished" and are not:
Four signals look like "finished" and are not:
- **"nothing pending"** — also true when no check has registered yet. Require a
**non-zero total** as well.
- **`conclusion == null`** — `conclusion` is a *string*, empty while running.
Test `status == "COMPLETED"`.
- **a verdict from before the head moved** — re-check the head, or `--expect`.
- **`mergeable: UNKNOWN`** — GitHub has not computed it yet. It is not a
pass, and PRs #172 and #179 both went green on it.
`gh` is shell-aliased to a different tool in these shells; `pr-watch` locates a
real one itself. Do not reintroduce a bare `gh` call in a script here.
.agents/skills/wave/SKILL.md
+1 -1
View File
@@ -73,7 +73,7 @@ defect is a test that passes.
## Shepherding
- Wait on checks with `scripts/pr-watch <pr> --expect <sha>`. Never hand-roll a
poll; see the `pr-watch` skill for the three ways they report green while wrong.
poll; see the `pr-watch` skill for the four ways they report green while wrong.
- Review before merging. Copilot's quota may be exhausted and its request is a
**silent no-op**; an absent review and an unrequested one both read as
"pending". Dispatch an adversarial reviewer and give it the head SHA, the base,
context/code/cli.md
+50 -19
View File
@@ -161,7 +161,7 @@ limitation that is not new: `ExecuteCommand` returns one effect batch, so the
segments of `:a . b` all see the pre-batch cursor, exactly as a
`Binding::Sequence` does (gotchas 21 and 23).
## 8 — the interactive arm is `parse`, then context; it is never a second parser
## 8 — the interactive arm is `parse`, then the cursor; it is never a second parser
#151 fixed three instances of *a registry with two construction arms demotes
whichever caller reaches the weaker one*, one patch each. A post-merge review
@@ -175,31 +175,59 @@ and the key from the cursor. So a script that wrote 5 to `Region/East`
The shape rather than the instances is what changed. `CmdRegistry::register`
used to take two free functions, `parse(args)` and `interactive(args, ctx)`,
and nothing related them; each command re-derived its arguments twice and every
disagreement was silent. It now takes three pieces, and the interactive arm is
**built from `parse`**, not beside it:
disagreement was silent. The interactive arm is now **built from `parse`**, not
beside it:
- `parse(args) -> C` — the only function that reads arguments a caller supplied.
- `contextualize(C, &CmdContext) -> C` — fills the fields that are context and
never argument (a `Move`'s origin cursor, a `MovePanelCursor`'s bound). It is
handed the built command and **not the args**, so it cannot discard them.
- `fallback: Option<(args, ctx) -> C>` — reached **only** when `parse` refused.
This is where "no coordinates means the cursor" lives: `clear-cell` bare, and
`commit-cell-edit <mode>` bare, which is what the keymap binds.
- For `register_at_cursor` only, the registry itself then sets the command's
**cursor** — its `CursorState`, reached through `AtCursor::cursor_mut` — to the
live one. That is the whole of what the interactive arm adds to a parsed
command, and no registration supplies it.
Two consequences hold by construction rather than by test. **Whatever `parse`
accepts, the interactive arm accepts** — `contextualize` is infallible — so the
§7 gate cannot pass a segment the loop then refuses. And **an accepted argument
list cannot be overridden by the cursor**, because the only function that sees
both args and context never runs on an accepted list. `register_pure` and
`register_nullary` already had one arm; they are the `contextualize = identity,
fallback = None` case.
Three consequences hold by construction rather than by test. **Whatever `parse`
accepts, the interactive arm accepts** — nothing after `parse` can fail — so the
§7 gate cannot pass a segment the loop then refuses. **An accepted argument list
cannot be overridden by the cursor**, because the only function that sees both
args and context never runs on an accepted list. And **no field `parse` set can
be overwritten from context**, because nothing a registration writes can name
one: the cursor slot is typed `&mut CursorState`, and no command parses a
`CursorState` out of its arguments. `register_pure` and `register_nullary`
already had one arm; they are `register` with no fallback.
That last one is improvise-yf7p, and it is why the registry lost a parameter.
#171 took a third function, `contextualize(C, &CmdContext) -> C`, handed the
built command and not the args, and stated honestly that it could still
**overwrite** a parsed field — naming the field was "a visible line in review".
Review then injected five such overwrites on commands other than the one with a
test (yank, paste and drill-into-cell taking their key from the cursor over the
parsed one; toggle-panel-visibility ignoring its parsed panel; move-panel-cursor
clamping its parsed delta to ±1) and the whole workspace stayed green. A hole
guarded by review discipline plus one command's test is not guarded. Every
contextualizer in the tree fell into one of two shapes, and each had an answer
that removes the function rather than checking it:
- **The cursor** (`Move`, `EnterAdvance`, `CommitAndAdvance`). Always one
`CursorState` field, always `CursorState::from_ctx`. So the registry does it,
through a slot whose *type* is the guard: an `AtCursor` impl pointing at a
`CellKey` or a delta is a compile error, which was tried.
- **A read of context that belonged in `execute`** (`toggle-panel-visibility`'s
`currently_open`, `cycle-panel-focus`'s three flags, `move-panel-cursor`'s
cursor and bound). These copied context into the command only to read it back;
`execute` now reads `ctx` directly, the same answer `toggle-panel-and-focus`
got below. Those three are `register`/`register_nullary` now, so their
interactive arm is exactly `parse`.
What `execute` does with a parsed field is still the command's own logic and is
tested as such — `toggle_panel_visibility_reads_the_panel_it_names` and
`move_panel_cursor_moves_from_the_named_panels_own_cursor` fail if the panel or
the delta is ignored there. The difference is that the registry is no longer a
second place such a bug can be written.
What is still representable, stated rather than hidden:
- `contextualize` can overwrite a field `parse` set. It would have to name the
field, which is a visible line in review, where the old `|_args, ctx|` was an
invisible omission. `commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor`
fails if `commit-cell-edit`'s does.
- A `fallback` sees the args `parse` refused and could ignore some of them.
`commit-cell-edit`'s refuses anything but a lone mode, so the malformed
`commit-cell-edit editing 5` is an error on the `:` line — which has no pure
@@ -207,9 +235,12 @@ What is still representable, stated rather than hidden:
- The gate is **stricter** than the loop wherever a fallback exists: a bare
`clear-cell` is refused in a script. That is the safe direction — the whole
line is refused — and it is unchanged from before #151.
- A new command could declare a *second* `CursorState` field and parse it from
args; `cursor_mut` could then point at the wrong one. No command has one, and
a parsed cursor would be a new idea deserving its own argument.
`toggle-panel-and-focus` did not fit `contextualize`, because its pure arm
defaulted the omitted flags to `true` and a contextualizer cannot tell an
`toggle-panel-and-focus` did not fit #171's `contextualize` either, because its
pure arm defaulted the omitted flags to `true` and a contextualizer cannot tell an
omitted flag from an explicit one. So the omission is now in the type —
`target: Option<(bool, bool)>`, `None` meaning *toggle* — and the toggle is
resolved in `execute` against the same context. That is the honest home: the
context/code/command.md
+81 -7
View File
@@ -553,6 +553,78 @@ status line.
one, and the limit is checked in exactly one place — `Model::add_category` — so
nothing in the effect restates it.
### A model effect that cannot do what it names refuses: `App::refuse`, `on_view` and `refuse_and_stop` (improvise-bok4, cz8d)
`AddCategory` was the only wire-reachable model effect that aborted when it
failed. The others did one of two things:
- **Narrated.** `AddItem`, `AddItemInGroup`, `AddFormula`, `LoadModel` and
`ImportJsonHeadless` wrote a sentence to the *host's* `status_msg` and
carried on.
- **Swallowed.** Every view-addressed effect (`SetAxis`, `SetPageSelection`,
`ToggleGroup`, `HideItem`, `ShowItem`, `TransposeAxes`, `CycleAxis`,
`SetNumberFormat`, `TogglePruneEmpty`) did nothing when `view_mut_of` found
no such view. `DeleteView` discarded "cannot delete the last view" with
`let _`, and answered `Ok` for a view the document did not have
(improvise-40j4). `AddFormula` stored a formula for a category that was gone,
through `let _ = add_item(…)` (improvise-40j4).
Either way the batch ran to its end, and `Host::mutating_frame` reported the
frame **accepted**. That is clause 4's partial-and-silent shape, one layer below
the record tier that `broken_chain` guards. The reviewer measured two cases.
`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]` was accepted with
half of it a no-op. `[RemoveCategory("Region"), AddItem { category: "Region" }]`
was accepted too, and its reason went only to the host's own status line, so
the client was told nothing at all.
**One writer, and the abort is separate (improvise-cz8d).** improvise-c5ip
(PR #185) had meanwhile added `App::refuse`, which writes the reason, sets
`batch_refused` and does **not** abort. The combination is now:
- **`App::refuse` is the only writer** of a refusal. bok4's own `refused()`
helper, which aborted, is deleted.
- **`on_view(app, target, write)`** writes the named view, or calls
`App::refuse` naming the missing one.
- **`Host::mutating_frame` rejects on `last_batch_refused() ||
last_batch_aborted()`.** That is sound because each `ModelCommand` maps to a
one-effect batch, so either flag belongs to the mutation that raised it.
- **Aborting is an explicit extra step**, taken only where a later effect in a
key-path batch assumes the failed one happened. `refuse_and_stop` does it for
`LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and
`import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its
own inline abort (gotcha 21). The item, formula and view effects only refuse:
the frame is rolled back by the refusal alone. On the key path, a failed
`AddItem`, `AddFormula` or view edit no longer stops the rest of its batch,
which is what it did before bok4.
In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame
is undone whole, and the client is told, which is the channel the narrated
reason never had.
**This takes the ticket's first option, scoped to the effects the wire can
reach.** Failure is *reported* by the effect that knows it failed. The second
option, growing `broken_chain` to ask whether every view, category and item
target still resolves, would have been a second description of which targets
each effect needs. That second copy would be free to disagree with the effect
itself, which is gotcha 24's reason for observing rather than predicting.
Consequences:
- **A failed load is refused**, where it used to be accepted with a host status
line. So `a_load_that_fails_is_not_a_rebase` now witnesses the failure by the
refusal, not by `status_msg`. The frame's rollback also rolls the host's
status line back.
- **Every refusal site has a test that fails when it is reverted** to a
status-only write (improvise-9fpl). Most are one table,
`every_failing_model_command_refuses_its_whole_frame`. That matters because
these are exactly the lines a merge conflict could silently drop.
- **Not covered:** a no-op *inside* a view that does resolve. Examples are
`SetAxis` on a category the view does not have, `HideItem` of an absent
item, and `RemoveItem`/`RemoveCategory`/`RemoveFormula` of something already
gone. The removals are idempotent by intent. The others are the remaining
instances of this class, and each needs its own `View` method to say whether
it changed anything.
### `AddFormula` and non-`_Measure` targets
For non-`_Measure` targets the item is added to the category so it appears in the
@@ -1112,7 +1184,9 @@ emit `RebuildLayout`) first (gotcha 23).
`display_value_of` is the value an editor opened on `key` starts with —
unformatted, and so **not** `CellDisplay::text` (design-principles §4: display
rounding is view-only and must not feed back into the model). Crate-visible so
rounding is view-only and must not feed back into the model). It is also **not**
`CellValue`'s `Display`, which rounds to four places: both arms spell the value
with `CellValue::edit_text` (improvise-umi4, `core.md` §17). Crate-visible so
`project::cell_display` can put it on the wire as `CellDisplay::edit_text`; a
browser that un-formatted the drawn text instead would be a second derivation, and
a lossy one.
@@ -2492,12 +2566,12 @@ available for the rare command that needs to enumerate all views, such as the vi
panel.
The interactive arm (keymap dispatch, scripts, the `:` line) is built **from**
`parse`: `register` takes `parse`, a `contextualize` that fills context-only
fields of the command `parse` built without seeing the args, and an optional
`fallback` reached only for argument lists `parse` refused. So an accepted
argument list is never overridden by context (improvise-2bac; the argument is
`context/code/cli.md` §8). `register_pure` is the case with nothing to
contextualize and no fallback: both arms are `parse`.
`parse`: `register` takes `parse` and an optional `fallback` reached only for
argument lists `parse` refused, and `register_at_cursor` additionally has the
registry set the command's `CursorState` to the live cursor. No registration
writes any other field, so an accepted argument list is never overridden by
context (improvise-2bac, improvise-yf7p; the argument is `context/code/cli.md`
§8). `register_pure` is the case with no fallback: both arms are `parse`.
`write_buffer` is the effect that writes a named buffer — the mirror of
`read_buffer`, and the only place that knows "search" is not an entry in the buffers
context/code/core.md
+32
View File
@@ -1200,3 +1200,35 @@ off the `## Data` line and off `serde_json` and requires them equal. A second
home that happens to agree is harmless; one that drifts fails there, and
re-copying the tokens into `format.rs` with one letter changed was injected to
prove it does.
## 17. An editor opens on the value, not on its `Display` (improvise-umi4)
`CellValue`'s `Display` is a **screen** spelling: a non-whole number is written
`{:.4}`, so `1.23456789` reads `1.2346`. Every editor seed used to be that
string (`display_value_in`'s pivot arm through `get_cell().to_string()`, its
records arm through `records_display`, and `project::cell_display`'s records
arm spelling it again), so pressing `i` and then `Enter` with nothing typed
stored `1.2346`. The screen looked the same before and after, which is what
made it silent data loss rather than a visible bug. The browser seeds from the
projected `CellDisplay::edit_text`, which came from the same call, so it had
the same bug.
`CellValue::edit_text` is the one home of the seed. A number is written as the
**shortest decimal that parses back to the same `f64`**, which is what Rust's
`f64` `Display` already guarantees (no precision argument, no exponent). The
brief pointed at improvise-gp's `number_text` as the implementation to reuse.
It is not a second implementation of this one: it takes an `f32` and searches
fixed precisions for the shortest *f64* expansion of it, because a threshold
must route the same under f32 and f64 evaluation. For an `f64` the standard
library's spelling is the same answer with no search. A non-finite number takes
§16's token (`inf`, `-inf`, `nan`), which `typed()` also reads back, so the
editor and the `.improv` format spell it one way.
Text and errors keep `Display`. Text is written unchanged, and an error cell has
no edit text on the wire anyway. Display itself is untouched: the grid still
rounds (gotcha 8), only the seed changed.
What this does **not** fix: a text cell whose text reads as a number. `"007"`
seeds `"007"`, and `typed()` classifies that as `Number(7.0)`. The seed is
exact, but the classifier has no way to know the cell was text. That is
improvise-ixxx.
context/code/protocol.md
+39 -8
View File
@@ -230,11 +230,33 @@ guard below (improvise-c7mu). It removes three things a scan must not see.
composers, so no verdict was wrong yet. It also had a mirror: a comment
explaining that a `NOT_COMPOSED` variant is held back would have raised a
false "stale entry" alarm.
- **Test items inside a file.** A line scan that relies on rustfmt putting an
item's closing brace at the attribute's own indentation. It treats
`cfg(any(test, …))` as test code and `cfg(not(test))` as production.
Over-stripping **fails** the test rather than passing it, which is the safe
direction.
- **Test items inside a file.** Each test-only attribute's item is bounded by
**balanced brackets on the blanked code** (improvise-59q8). It ends at a `;`
at depth zero, or at the `}` closing its body unless `;`, `.`, `?` or `else`
follows. The first version was a line scan that relied on rustfmt putting an
item's closing brace at the attribute's own indentation. The #172 review
found three shapes it never bounded:
- an item ending in `];` or `);`;
- a `#[cfg(test)]` statement spanning lines;
- a one-line `#[cfg(test)] use x;`.
In each case the scan read on to the end of the file and **hid every line of
production code after it**, which is the silent direction for the forward
guard. An item the scan cannot bound is now a **panic**, never a scan to EOF.
**Which `cfg` is test-only is evaluated, not substring-matched.** The
predicate is read off the file's *text*, because a `feature` value is a
literal and blanked in the code. It is evaluated in three-valued logic for a
build without `test`: `test` is false, and so is `feature = "test-support"`,
which this repo never ships (improvise-26dm). Everything else a scan cannot
know, such as a target or an ordinary feature, is unknown. An item is
test-only when its predicate is **false**. So
`cfg(all(test, not(target_arch = "wasm32")))` is test code, which the old
"any line containing `not(`" rule got wrong. `cfg(any(test, feature =
"serde"))` and `cfg(not(test))` are production. An inner `#![cfg(test)]`
heading a file makes the whole file test code. One appearing anywhere else
is a panic, because this scan cannot bound an inline module's inner
attribute.
- **Test-only files.** A `#[cfg(test)] mod x;` makes `x.rs` and everything under
`x/` test code, even though nothing inside the file says so. Before
improvise-c7mu, this checker's own file and `improvise-io`'s
@@ -622,6 +644,13 @@ a value.
A browser found this: `tests/lockstep.rs` drove `i` through the host and the
client over the same grid, and they disagreed by a comma.
**It is the value's exact spelling, not its `Display`** (improvise-umi4).
`CellValue`'s `Display` writes a non-whole number `{:.4}`, so this field
carried `1.2346` for a stored `1.23456789`, and `i` then `Enter` with nothing
typed stored the rounded number in both front ends. Every arm that seeds an
editor now goes through `CellValue::edit_text` (`core.md` §17). That is the
shortest decimal that parses back to the same `f64`.
`numeric` and `error` are decided by the server for the same reason: it is
the side that knows which it was, and re-deriving client-side is how the two
ends drift apart. `CellDisplay` is deliberately not *drawn from* a `CellValue`
@@ -636,8 +665,9 @@ that `text` was formatted from: `evaluate_aggregated` over the view's
because the browser's `yy` has to copy what the TUI's `YankCell` copies, and
neither string on the cell can be turned back into it:
- `edit_text` is `CellValue::to_string()`, which writes a non-whole number
`{:.4}` — `1.23456789` becomes `1.2346`.
- `edit_text` was `CellValue::to_string()`, which writes a non-whole number
`{:.4}`, so `1.23456789` became `1.2346`. improvise-umi4 has since made it
exact, but the next bullet still holds.
- Re-parsing any text with `typed()` turns number-like text into a number —
`Text("007")` becomes `Number(7.0)`.
- In a pivot grid `edit_text` is the *stored* cell (`get_cell`), so an
@@ -647,7 +677,8 @@ So the value is **projected**, not re-derived (gotcha 47): a client that
wants the value asks for the value. It is kept separate from `edit_text`
rather than replacing it, because the two answer different questions — what
an editor starts with is a *string*, deliberately the one the TUI's buffer
holds, and changing it would change what the TUI's own editor shows.
holds. (That string was itself lossy until improvise-umi4. The fix changed what
the TUI's own editor shows, which was the point.)
In a records grid the TUI does **not** yank this value: `YankCell` and
`PasteCell` resolve the synthetic `(_Index, _Dim)` key, so the TUI yanks
context/code/tooling.md
+66
View File
@@ -259,6 +259,72 @@ the deploy's and pass. `README.md` therefore states the rule and deliberately
**does not list the jobs**: a second copy with nothing holding it to the first
is what went stale, twice.
### pr-watch and mergeability (improvise-edpl, improvise-zlj2)
`scripts/pr-watch` is what the orchestrator's merge procedure stands on, and
until improvise-edpl its verdict failed only on `mergeable == CONFLICTING`.
GitHub computes mergeability lazily: for a while after the base moves it reports
`UNKNOWN` for both `mergeable` and `mergeStateStatus`. PRs #172 and #179 both
exited 0 at `UNKNOWN/UNKNOWN … 0 failed`. A PR that has *just become*
conflicting is in exactly that window, so it could pass the watch. It is the
same shape as improvise-b4ks: an uncertain state read as success.
The rule now:
- **`UNKNOWN` in either field is not an answer, so it is waited on.** On
timeout it exits 3, the same code as a check still running. It never exits 0.
- **`MERGEABLE/BLOCKED` with no failed check is waited on too** (improvise-zlj2).
A required check that never runs therefore times out at exit 3, and does not
exit 1.
- **A known answer ends the wait early**: a failed check (exit 1 whatever
mergeability says, since waiting would turn a known red into a timeout), or
`mergeable == CONFLICTING`.
- **The pass condition is an allowlist**: `mergeable == MERGEABLE` *and*
`mergeStateStatus` in `CLEAN`, `UNSTABLE` or `HAS_HOOKS`. Anything else exits
1. That includes `null` and any state GitHub adds later, so the check fails
closed.
Why each state is where it is. The `main` ruleset (`gh api
repos/fiddlerwoaroof/improvise/rules/branches/main`, read 2026-09-22) requires
three status checks. It requires no approvals, and its required checks are not
strict.
- `BLOCKED` with a `MERGEABLE` flag means a required check has not passed. A
failing one is already in the rollup, so the script has exited 1 before it
reads the state. What is left is a required check that **has not registered
yet**, and every healthy push passes through that state. The required
`Browser harness (OPFS contract + shell)` check comes from the `browser` job,
which `needs: build`. GitHub creates its check run only after `build`
completes. For a second or three per push, then, every registered check is
`COMPLETED/SUCCESS`, the required one does not exist, and GitHub reports
`MERGEABLE/BLOCKED`. The first version of this rule refused `BLOCKED`, and so
exited 1 on a healthy PR whenever a poll landed in that gap. The reviewer put
that at 5–15% of watches at the 20s interval. An "unrun" required check and a
"not yet registered" one look the same from the rollup, so the state is
waited on. A check that genuinely never runs costs a timeout, exit 3, not a
false exit 1. An earlier version of this section said that nothing else
produces `BLOCKED` here. That was wrong: the `needs:` gap produces it.
- `DIRTY` can wear `MERGEABLE` for a moment while the two fields settle. It is
refused.
- `BEHIND` needs strict required checks, which this repo does not set. `DRAFT`
cannot be merged. Both are refused.
- `UNSTABLE` means a non-required check is unhappy. It stays a pass, as it was
before. It is safe because the loop only reads the state once every
registered check is `COMPLETED`, and any failed one has already exited 1. So
whatever `UNSTABLE` is reporting, it is not a check in the rollup that is
still running or red.
- `HAS_HOOKS` is GitHub Enterprise's "mergeable, with pre-receive hooks". It is
harmless to allow.
The suite enumerates the whole `mergeable × mergeStateStatus` table, plus an
unshipped value and `null`, rather than hand-picking cases. Why: improvise-90ea.
The suite also plays the `needs:` gap through, poll by poll: build running, then
build done with no harness check, then the harness running, then `CLEAN`. That
sequence must exit 0. The rollup it starts from is PR #189's, copied from
`gh pr view --json`. Each earlier poll is derived from it with `jq`, by dropping
entries or rewinding them to `IN_PROGRESS`.
### Why the Pages deploy fails on "Found 0 artifact(s)" (improvise-864a)
`actions/deploy-pages@v5` intermittently fails a push to `main` with *No
context/test-evidence/cli.md
+3 -2
View File
@@ -298,8 +298,9 @@ The unit-level form of the headless test, against `CmdRegistry::interactive`
directly: cursor on `Type=Food, Month=Jan`, a decoy `edit` buffer, and an
explicit `5` for `Type/Clothing Month/Feb`. It asserts the built command's key
and value. Rival (a) above — a `contextualize` that overwrites the parsed
fields — fails it; this is the one hole the new registry shape leaves
representable, so it is the one that needs a test.
fields — failed it. improvise-yf7p then removed `contextualize` altogether
(`context/code/cli.md` §8), so rival (a) no longer compiles; the test stays as
the behavioural statement of what the signature now guarantees.
### every_argument_list_the_gate_accepts_the_run_accepts_too
context/test-evidence/clients.md
+31
View File
@@ -4392,6 +4392,37 @@ Records mode is deliberately not covered. There the TUI yanks `None` and its
case there would assert the bug. Since improvise-6p8q's fix it is covered, by
`yy_then_p_in_records_mode_writes_the_record_the_host_writes` below.
### `an_untouched_browser_edit_stores_the_identical_value`
**improvise-umi4**, the browser half. The client seeds `i` from the projected
`CellDisplay::edit_text` (`resolve.rs::edit_or_drill`), which was
`App::display_value_in` and so `CellValue`'s rounded `Display`. The client
resolves `i` and `Enter` itself, and what it composed is applied to the host
through `apply_on` (`Checked::against` plus `apply_command`). The stored value
is then compared by `f64::to_bits`. The fixture puts the value at row 1,
column 1 of a 2×2 pivot grid, with values that `Display` rounds (the list in
`context/test-evidence/command.md`). A records grid is not driven, because the
browser declines records editing (`edit_or_drill`).
Failed before the fix on `Number(1.23456789)` stored as `Number(1.2346)`.
**Rival:** `CellValue::edit_text`'s number arm set back to `Display`. The test
failed on the same case.
The test is in-process, so it cannot see the JSON wire. A float that does not
survive `serde_json` is improvise-6oy4, a separate fix.
### `every_projected_edit_text_commits_back_to_its_value`
The projection contract behind the test above, for **both** grid kinds. Every
`Record` cell's `edit_text`, fed through `typed()`, must give back the cell's
`value`. It runs over the whole projected grid in pivot mode and in records
mode (entered with `R`). A guard asserts that the precise value was projected
at all. This test exists because `project::cell_display`'s records arm spelled
`edit_text` itself, apart from `display_value_in`, and no key-path test reaches
that arm from a browser. **Rival:** that arm restored to `Some(text.clone())`,
the display text. It failed with `records=true: edit text "1.2346" does not
commit back to Some(Number(1.23456789))`.
### yy_then_p_in_records_mode_writes_the_record_the_host_writes
improvise-6p8q in lockstep. The yank test in this file deliberately stayed on a
context/test-evidence/command.md
+144 -8
View File
@@ -3140,10 +3140,13 @@ Injected fault: hardcode the `category_panel_open` propagator to `true`. Fails.
### toggle_panel_visibility_reads_the_panel_it_names
The behavioural half, and the one that reaches the registry. Its interactive arm
passes `currently_open` straight through, so with exactly one panel open the command
built for each of the three names carries its own panel's flag. Both injections above
fail here too.
The behavioural half, and the one that reaches the registry. With exactly one panel
open, the command built for each of the three names and executed must emit
`SetPanelOpen` for **its own** panel, flipping **its own** flag. Both injections above
fail here too. Since improvise-yf7p the flag is read in `execute` rather than copied
in by a registry contextualizer, so the assertion moved from the built command's
`Debug` to its effects; it also names the panel, so an `execute` that ignores the
parsed panel (`ctx.panel_open(Panel::Formula)`) fails it — injected and run.
### a_focused_open_panel_closes_and_a_closed_one_opens
@@ -4728,19 +4731,52 @@ Its mirror, and the one that pins the wrap: from the first item `[` must land on
`SetPanelCursor` carries a panel *and* a cursor and neither was checked. Both
are now named, so moving the right cursor on the wrong panel fails.
### move_panel_cursor_moves_from_the_named_panels_own_cursor
improvise-yf7p. `move-panel-cursor formula 2` through the registry's interactive
arm, with the formula cursor at **1** of five formulas and the category and view
cursors at 0, must land on **3**. Before yf7p the cursor and bound were copied in
by a registry `contextualize`, and review showed one that clamped the parsed delta
to `delta.signum()` left the whole workspace green: every test moved by ±1, where
a clamp is invisible. They are read in `execute` now, and this test is what sees a
bug there. Rival `self.delta.signum()` in `execute` — injected, run, fails (lands
on 2). Reading another panel's cursor fails too, because the fixture seats them at
different values — the index-0 fixture could not separate them.
### delete_formula_at_cursor_with_formulas
Asserts the `RemoveFormula` names `Profit` in `Type` — the formula the cursor is
on — rather than merely that a formula was removed.
Asserts the `RemoveFormula` names the formula the cursor is on, rather than merely
that a formula was removed. PR #148 named the field, but its fixture was **one**
formula with the cursor at **0**, where "the formula under the cursor" and "the
first formula" are the same formula (improvise-y1av). The rival it could not see —
`DeleteFormulaAtCursor` reading index `0` instead of `ctx.formula_cursor` — passed
the **whole workspace**, so `d` in the formula panel deleting the wrong formula had
no guard anywhere. Now two formulas, cursor at **1**: it must remove `Tax` and must
not name `Profit`. Rival `let cursor = 0usize.min(..)` injected, full suite run:
this is the only test that fails.
### switch_view_at_cursor_with_valid_cursor
Names `Default`, the view under the cursor.
Names the view under the cursor. Same fixture defect as above (improvise-y1av):
`two_cat_model` has one view, so the cursor at 0 named `Default` whether or not
the command read it. Now `Default, Other` with the cursor at **1**, asserting
`Other`. Rival `get(cursor).and(first())` — in bounds only, so the out-of-bounds
arm is unchanged and `switch_view_at_cursor_out_of_bounds_returns_empty` cannot be
what catches it — injected and run: fails here, in
`delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor`, and in the two
`wasm-client/tests/lockstep.rs` tests that were previously the only guard.
### delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor
improvise-y1av. `Default, Other, Third` with the cursor at **1**: deletes `Other`
and steps the panel cursor back to 0. The same rival as above fails it.
### delete_view_at_cursor_zero_does_not_adjust_cursor
Names `Default` too. Its existing negative half — no `SetPanelCursor` at
position 0 — is unchanged.
position 0 — is unchanged. Position 0 is this test's subject, so it cannot
separate "the view under the cursor" from "the first view" and does not try to;
the test above does.
### commit_category_add_with_name_produces_add_effect
@@ -5006,6 +5042,31 @@ run alone with the ones before it removed, and each fails: the diff returns
test passes. See
`render::tests::a_cell_equals_another_exactly_when_the_wire_cannot_tell_them_apart`.
### ui::host::tests::a_frame_writing_a_view_its_own_first_command_deleted_is_refused
improvise-bok4. This is the reviewer's first measured case:
`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]`. Before the fix
the asker's reply was `[Commands([ForgetView("Scratch")]), Commands([Project…])]`
and then `Accepted`. The second command had done nothing, because
`view_mut_of` found no view and every view-addressed effect swallowed the
`None`. Now `on_view` refuses, the abort becomes
`Rejected(MutationRefused)`, and the assertion that `Scratch` still exists
proves the deletion was undone with the rest of the frame.
Its sibling, `a_frame_adding_an_item_to_a_category_its_first_command_removed_is_refused`,
is the reviewer's second case: `[RemoveCategory("Region"), AddItem { category:
"Region" }]`. It was accepted as well, with `AddItem`'s error written only to
the host's `status_msg`. Both tests failed first against the unfixed effects.
`deleting_the_documents_last_view_is_refused_rather_than_accepted` covers the
one-command case, which `DeleteView`'s `let _` swallowed. The fault injected
was to put the `let _` back. The test then failed with a projection batch
where the refusal should have been, and it was reverted.
The control is `a_frame_whose_commands_all_land_is_accepted`, the same two
commands in an order where both land. Without it, a host that refused every
two-command frame would pass all three tests.
### ui::reduce::vocabulary_is_complete::every_effect_is_on_the_wire_or_has_a_verdict
improvise-h3y7, the forward direction of the vocabulary derivation. It is the
@@ -5045,6 +5106,34 @@ bracket-matched `#[…]` before reading the name.
a doc comment, two attributes (one with nested brackets), and a wrapped call.
With the whitespace skip removed it fails with `[]`.
### `an_untouched_pivot_edit_stores_the_identical_value`
### `an_untouched_records_edit_stores_the_identical_value`
**improvise-umi4.** `App::display_value_in` seeded the edit buffer with
`CellValue`'s `Display`, which writes a non-whole number `{:.4}`. So `i` then
`Enter` with nothing typed stored `1.2346` over `1.23456789`, in pivot and in
records mode, and the screen looked the same before and after.
Both tests go through the real key path (`App::handle_key`, `i`, `Enter`) on a
2×2 grid. The cursor sits on (West, Revenue), which is row 1, column 1 in pivot
mode and row 1, column 3 (`[Region, +cat, Cost, Revenue, +]`) in records mode,
so "the cell under the cursor" is not "the first cell". The values are chosen
so that `Display` and the value disagree: `1.23456789`, `0.1 + 0.2`,
`98765.4321987`, `123456789012345.67` (under the `1e15` integer cutoff, so it
takes the rounding arm), `-0.00001234` (rounds to `-0.0000`), `-0.0`,
`-inf`, plus a text value. The comparison is `f64::to_bits`, so `-0.0` against
`0.0` counts as a difference.
Both failed before the fix, on `Number(1.23456789)` stored as `Number(1.2346)`.
**Rival injected:** `CellValue::edit_text`'s number arm set back to
`self.to_string()`, i.e. the display seed restored in its one home. Both tests
failed again on the same case. The fix was then restored and the suite re-run.
A text cell that reads as a number (`Text("007")`) is deliberately **not** in
the list. Its seed is exact (`"007"`), but `typed()` classifies the committed
text as `Number(7.0)`. Probed here: both tests fail on it in both modes. That
is the classifier, not the seed, and it is improvise-ixxx.
### yy_then_p_in_records_mode_copies_between_real_records
improvise-6p8q. At `gh/main` `9e78051` it failed at the yank: `left: None,
@@ -5111,6 +5200,53 @@ the first message does not mention drilling. At `0ed2e41` it failed with the
old text. The rival is restoring that text. Real drill paste, which means
staging a typed `CellValue`, is still open as improvise-48jd.
### ui::host::tests::every_failing_model_command_refuses_its_whole_frame
improvise-9fpl, found by PR #181's review. Eight of bok4's refusal sites had no
test: reverting any one of them to a status-only write failed nothing. They
were `AddItemInGroup`, `AddFormula`'s parse error, and all six
`ImportJsonHeadless` arms. Two improvise-40j4 cases were added to the same
table: `DeleteView` of an absent view, and `AddFormula` into an absent
category.
Each case sends a frame of two commands. The first, `AddItem { Region,
"Landed" }`, lands. The second fails. The test asserts that the asker hears
exactly `[Rejected(MutationRefused)]` and that `Landed` is gone. The fixture
has a second view, `Scratch`, because with one view `DeleteView("Ghost")` is
refused as "the last view" and the absent-name case is never reached. The
first run found exactly that: the case passed before the fix.
Faults injected by a script (`/tmp/wire2_inject2.py`, not committed): each of
the 14 refusal sites was reverted **alone** to a status-only write, after the
merge with #185. Every one failed at least one test. The eight that were
unwitnessed fail this test, `DeleteView`'s last-view arm fails
`deleting_the_documents_last_view_is_refused_rather_than_accepted`, `on_view`
fails `a_frame_writing_a_view_its_own_first_command_deleted_is_refused`, and
`LoadModel` fails both `a_load_that_fails_is_not_a_rebase` and
`a_failed_load_or_import_does_not_run_the_quit_after_it`. The file was
restored and compared.
### ui::host::tests::a_frame_adding_a_formula_to_a_category_its_first_command_removed_is_refused
improvise-40j4. `[RemoveCategory("Region"), AddFormula { "Total = 1", Region }]`
was accepted, and the document held a formula for a category that did not
exist; `format_md` wrote it and `parse_md` reloaded it. `AddFormula` ran
`let _ = add_item(…)` and then `add_formula` anyway. The test failed first with
the frame accepted. `AddFormula` now refuses when `add_item` errs, before
storing anything.
### ui::app::tests::a_failed_load_or_import_does_not_run_the_quit_after_it
improvise-cz8d. After #185, `App::refuse` does not abort. So on gh/main a failed
`load` refused, and the `force-quit` after it in the same batch still ran:
`load absent.improv . q` quit having loaded nothing. bok4's aborting helper had
prevented that, and the merge would have silently lost it. The test binds
`[load|import-json <absent>, force-quit]` as one sequence and asserts that the
refusal is recorded and the mode is not `Quit`. `import-json` failed first,
before `refuse_and_stop` was applied to its arms. For `load`, the injected fault
is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test
then fails with "`load absent.improv . q` quit having loaded nothing".
## JSON import numbers (improvise-6yvg)
### `a_json_import_stores_its_numbers_bit_for_bit`
context/test-evidence/protocol.md
+48
View File
@@ -1050,3 +1050,51 @@ Faults injected, one per half, each compiling only `improvise-protocol`:
A field added to an existing variant is caught too, but not in isolation,
because every constructor of that variant also fails. Reverted with
`git restore`.
### source_scan::tests::a_test_only_item_hides_nothing_after_it
improvise-59q8, found by the PR #172 review. The scanner's line-based
`strip_test_items` looked for `}` at the attribute's indentation, or for an item
line ending in `;`. So three shapes were never bounded, and it dropped
**everything to the end of the file**:
- a test-only `const … = &[\n … \n];`;
- a `#[cfg(test)] let x = foo(\n 1,\n);` statement;
- a one-line `#[cfg(test)] use foo::bar;`, where the attribute's own line is
also the item line.
In `vocabulary_is_complete` that silently hides every effect after such an
item. Each shape is a case here, asserting that the production code after it
(`Hidden1`, `Visible`) survives. Against the line scan all three failed; the
first printed `"impl X {\n"` and nothing else. They now pass because items are
bounded by balanced brackets on the blanked code.
`an_unbounded_test_only_item_is_refused_not_swallowed` pins the other half of
the acceptance criteria: an item that cannot be bounded panics instead of
consuming the rest of the file. Against the line scan it failed, because
nothing panicked.
### source_scan::tests::a_test_only_cfg_is_recognised_whatever_its_spelling
improvise-59q8, the other direction: test code counted as production, which
makes `vocabulary_is_composed` weaker. `cfg(all(test, not(target_arch =
"wasm32")))` was kept because the old rule rejected any line containing
`not(`. A leading `#![cfg(test)]` was kept because only `#[cfg(` was
recognised. An attribute split across lines was not seen at all. All three
failed against the old scanner. The predicate is now parsed and evaluated in
three-valued logic (`context/code/protocol.md` §2). The controls
(`cfg(not(test))`, `cfg(any(test, feature = "serde"))` and `cfg(target_arch =
…)`) must stay production, so a scanner that dropped every `cfg` item could
not pass.
A consequence to know about: `cfg(any(test, feature = "x"))` for an ordinary
feature is now production. Only `feature = "test-support"`, the one test
feature this repo has, counts as a test build.
`test_only_items_and_modules_are_not_production` was updated to that spelling.
### source_scan::tests::a_c_string_literal_is_blanked
improvise-59q8's minor case. In `cr#"x" Command::InRawC(2) "y"#` the `r` has a
word character before it, so the literal was lexed as two ordinary strings,
and the text between the inner quotes leaked. It failed as
`InRawC survived`. `c"…"` and `cr#"…"#` are now literals. A first version of
this test used a C string with no inner quotes and passed against the bug,
because the ordinary lexing happened to blank it too.
context/test-evidence/tooling.md
+71
View File
@@ -611,6 +611,77 @@ its `.base`, `.ledger`, `.removed` and `.stamped` siblings and `.lock`. `git
status --untracked-files=all` and `git add -A --dry-run` both show none of
them, and `git check-ignore -v` names `.gitignore:40` (`prolog-sessions/*`).
### The pr-watch mergeability cases — `scripts/pr-watch-test.sh`
improvise-edpl. The cases are injected faults in themselves. Like the other
`scripts/` suites, none of their names is a Rust `fn`, so the case list is the
evidence. The reasoning is in `context/code/tooling.md`, "pr-watch and
mergeability".
**The bug, reproduced before the fix.** Against the script as it stood at
`ae96b8c`, 19 of the 34 new cases failed and all 19 exited **0**.
`UNKNOWN that resolves to CONFLICTING fails` is PR #172's shape exactly: every
check green, `UNKNOWN/UNKNOWN` on the first poll, `CONFLICTING/DIRTY` on the
second. The old script broke out of the loop on the first poll and passed. The
fixtures are hand-built. They are the subset of the `gh pr view --json` shape
that the script reads: `headRefOid`, the two mergeability fields, and the
rollup's `status` and `conclusion`. They are not a captured payload. A stub that
errors instead would have exited 4, a wider shape than the fault.
**Rival 1: treat `UNKNOWN` as an answer.** The wait condition was replaced with
`if true`. 13 cases fail: every `UNKNOWN` row of the table,
`MERGEABLE/UNKNOWN`, the timeout case, and
`UNKNOWN that resolves to MERGEABLE/CLEAN passes`. That last one exits 1 under
this rival, so the fix cannot pass by refusing everything uncertain. It has to
wait. `UNKNOWN that resolves to CONFLICTING fails` still passes under this
rival, because the allowlist refuses `UNKNOWN` as well. Rival 2 does not
separate it either: its first poll is `UNKNOWN`, which rival 2 still waits on.
What separates it is the original fault, which combined both rivals. An earlier
version of this entry said rival 2 separated it. That was wrong.
**Rival 2: the old verdict.** The allowlist was replaced with `exit 0` after the
`MERGEABLE` test. Five cases fail: `MERGEABLE/BEHIND`, `DIRTY`, `DRAFT`,
`SOMETHING_NEW` and `null`. These are the states that look mergeable by the flag
and are not. At `128a2a0` it was six, with `BLOCKED` among them. Since
improvise-zlj2, `BLOCKED` is waited on, so this rival never reaches the verdict
for it. The entry below covers that.
`a failed check fails without waiting on mergeability` runs at `--timeout 0`.
A script that waited on `UNKNOWN` before reading the rollup would report that
known red as exit 3.
### The pr-watch needs-gap and pinned-head cases — `scripts/pr-watch-test.sh`
improvise-zlj2 and improvise-h3ni. These were found by review of PR #186 at
`128a2a0`, which was ruled do-not-merge.
**zlj2: a healthy PR exited 1.** The required `Browser harness` check run
exists only after the `build` job completes (`needs: build`). In that gap
GitHub reports `MERGEABLE/BLOCKED` while every registered check is green, and
the #186 rule refused `BLOCKED`. The rollup in the new cases is PR #189's,
captured verbatim with `gh pr view --json`. It has thirteen entries with every
field GitHub returns. Each gap state drops or rewinds entries of that rollup
with `jq`. Against `128a2a0` three cases failed, all with exit 1:
- `the needs-gap before the required check registers is not a failure`: the
healthy sequence should exit 0.
- `a required check that never registers is a timeout`: should exit 3.
- `table: MERGEABLE/BLOCKED`: should exit 3, not 1.
**Rival for zlj2:** remove `MERGEABLE/BLOCKED` from the wait. The same three
cases fail with exit 1. That rival is `128a2a0`'s code.
**h3ni: nothing pinned the head across the UNKNOWN wait.** The case is
`a head that moves during the UNKNOWN wait is not a pass`. Its polls are
UNKNOWN@a, then UNKNOWN@b, then CLEAN@b, and it must exit 2. It passed on the
first run, so it is a guard for behaviour the script already has, not a
reproduction.
**Rival for h3ni:** re-pin the head whenever `mergeable` is UNKNOWN. That case
alone fails, exiting 0. All 44 others pass. That is the review's point: the
older `a head that moves mid-wait is not a pass` starts from MERGEABLE, so it
cannot see this rival.
## Every JSON parser reads numbers exactly (improvise-dz8c)
### `every_crate_that_can_parse_json_enables_float_roundtrip`
crates/improvise-command/src/command/cmd/commit.rs
+125
View File
@@ -473,6 +473,131 @@ mod tests {
"bulk entry lands at the start of the new row"
);
}
/// Values whose `Display` (four decimals) and value disagree, plus text.
fn values_display_rounds() -> Vec<crate::model::cell::CellValue> {
use crate::model::cell::CellValue::{Number, Text};
vec![
Number(1.23456789),
Number(0.1 + 0.2),
Number(98765.4321987),
Number(123456789012345.67),
Number(-0.00001234),
Number(-0.0),
Number(f64::NEG_INFINITY),
Text("0.1 + 0.2".into()),
]
}
/// Two regions, two measures, with `value` at (West, Revenue): never index 0.
fn grid_holding(value: crate::model::cell::CellValue, records: bool) -> crate::ui::app::App {
use crate::model::cell::{CellKey, CellValue};
use crate::ui::app::App;
use crate::view::Axis;
let mut wb = Workbook::new("Seed");
wb.add_category("Region").unwrap();
for region in ["North", "West"] {
wb.add_item("Region", region).unwrap();
}
for measure in ["Cost", "Revenue"] {
wb.add_item("_Measure", measure).unwrap();
}
let key = |r: &str, m: &str| {
CellKey::new(vec![
("Region".into(), r.into()),
("_Measure".into(), m.into()),
])
};
wb.model
.set_cell(key("North", "Cost"), CellValue::Number(1.0));
wb.model
.set_cell(key("North", "Revenue"), CellValue::Number(2.0));
wb.model
.set_cell(key("West", "Cost"), CellValue::Number(3.0));
wb.model.set_cell(key("West", "Revenue"), value);
{
let v = wb.active_view_mut();
match records {
true => {
v.set_axis("_Index", Axis::Row);
v.set_axis("_Dim", Axis::Column);
v.set_axis("Region", Axis::None);
v.set_axis("_Measure", Axis::None);
}
false => {
v.set_axis("Region", Axis::Row);
v.set_axis("_Measure", Axis::Column);
}
}
}
let mut app = App::new(wb, None);
app.rebuild_layout();
assert_eq!(app.layout.is_records_mode(), records, "wrong fixture mode");
app
}
/// Open the editor on (West, Revenue) and commit it untouched; the stored value.
fn no_op_commit(records: bool, value: crate::model::cell::CellValue) {
use crate::model::cell::CellKey;
use improvise_protocol::key::KeyCode;
let mut app = grid_holding(value.clone(), records);
let target = CellKey::new(vec![
("Region".into(), "West".into()),
("_Measure".into(), "Revenue".into()),
]);
let at = match records {
true => (1, 3),
false => (1, 1),
};
app.cursor_mut().selected = at;
if records {
app.view_state.mode = AppMode::RecordsNormal;
assert_eq!(
app.layout.record_col(at.1),
Some(&RecordCol::Measure("Revenue".into())),
"the cursor is not on the Revenue column"
);
}
assert_eq!(
app.model_state.workbook.model.get_cell(&target),
Some(&value),
"the fixture does not hold the value under test"
);
press(&mut app, KeyCode::Char('i'));
assert!(
app.view_state.mode.is_editing(),
"`i` did not open an editor"
);
press(&mut app, KeyCode::Enter);
let stored = app.model_state.workbook.model.get_cell(&target).cloned();
let same = match (&stored, &value) {
(Some(CellValue::Number(a)), CellValue::Number(b)) => a.to_bits() == b.to_bits(),
(stored, value) => stored.as_ref() == Some(value),
};
assert!(
same,
"{} mode: i then Enter on {value:?} stored {stored:?}",
if records { "records" } else { "pivot" }
);
}
/// BUG (improvise-umi4) — see `context/test-evidence/command.md`.
#[test]
fn an_untouched_pivot_edit_stores_the_identical_value() {
for value in values_display_rounds() {
no_op_commit(false, value);
}
}
/// BUG (improvise-umi4) — see `context/test-evidence/command.md`.
#[test]
fn an_untouched_records_edit_stores_the_identical_value() {
for value in values_display_rounds() {
no_op_commit(true, value);
}
}
}
// ── Commit commands (mode-specific buffer consumers) ────────────────────────
crates/improvise-command/src/command/cmd/core.rs
+40 -13
View File
@@ -3,6 +3,8 @@ use std::fmt::Debug;
use crate::command::key::KeyCode;
use super::navigation::CursorState;
use crate::model::Model;
use crate::model::cell::{CellKey, CellValue};
use crate::ui::app::AppMode;
@@ -139,13 +141,15 @@ pub trait Cmd: Debug + Send + Sync {
/// Factory that constructs a Cmd from text arguments (headless/script).
pub type ParseFn = fn(&[String]) -> Result<Box<dyn Cmd>, String>;
/// Fills the context-only fields of a command `parse` already built. It never
/// sees the args, so it cannot discard them — `context/code/cli.md` §8.
pub type ContextualizeFn<C> = fn(C, &CmdContext) -> C;
/// Builds a command from context, reached ONLY for args `parse` refused.
pub type FallbackFn<C> = fn(&[String], &CmdContext) -> Result<C, String>;
/// A command whose one context-only field is the cursor it moves from. The
/// registry fills it, and no registration can — `context/code/cli.md` §8.
pub trait AtCursor: Cmd {
fn cursor_mut(&mut self) -> &mut CursorState;
}
type BoxParseFn = Box<dyn Fn(&[String]) -> Result<Box<dyn Cmd>, String>>;
type BoxInteractiveFn = Box<dyn Fn(&[String], &CmdContext) -> Result<Box<dyn Cmd>, String>>;
@@ -186,25 +190,48 @@ impl CmdRegistry {
name
}
/// Register a command whose interactive arm is `parse` then `contextualize`, so
/// args `parse` accepts are honoured whole — `context/code/cli.md` §8.
/// Register a command whose interactive arm is `parse`, reaching `fallback`
/// only where `parse` refused — `context/code/cli.md` §8.
pub fn register<C: Cmd + 'static>(
&mut self,
prototype: &C,
parse: fn(&[String]) -> Result<C, String>,
contextualize: ContextualizeFn<C>,
fallback: Option<FallbackFn<C>>,
) {
self.push(prototype.name(), parse, fallback, |_, _| {});
}
/// As [`register`](Self::register), then the interactive arm moves the
/// command's cursor to the live one. The cursor is the only field it writes.
pub fn register_at_cursor<C: AtCursor + 'static>(
&mut self,
prototype: &C,
parse: fn(&[String]) -> Result<C, String>,
fallback: Option<FallbackFn<C>>,
) {
self.push(prototype.name(), parse, fallback, |cmd, ctx| {
*cmd.cursor_mut() = CursorState::from_ctx(ctx);
});
}
fn push<C: Cmd + 'static>(
&mut self,
name: &'static str,
parse: fn(&[String]) -> Result<C, String>,
fallback: Option<FallbackFn<C>>,
at_cursor: fn(&mut C, &CmdContext),
) {
self.entries.push(CmdEntry {
name: prototype.name(),
name,
parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box<dyn Cmd>)),
interactive: Box::new(move |args, ctx| {
let built = match (parse(args), fallback) {
(Ok(cmd), _) => Ok(contextualize(cmd, ctx)),
(Err(_), Some(fallback)) => fallback(args, ctx),
(Err(err), None) => Err(err),
let mut built = match (parse(args), fallback) {
(Ok(cmd), _) => cmd,
(Err(_), Some(fallback)) => fallback(args, ctx)?,
(Err(err), None) => return Err(err),
};
built.map(|c| Box::new(c) as Box<dyn Cmd>)
at_cursor(&mut built, ctx);
Ok(Box::new(built) as Box<dyn Cmd>)
}),
});
}
crates/improvise-command/src/command/cmd/mod.rs
+18 -2
View File
@@ -77,8 +77,8 @@ pub(super) mod test_helpers {
display_value: {
let key = layout.cell_key(sr, sc);
key.as_ref()
.and_then(|k| workbook.model.get_cell(k).cloned())
.map(|v| v.to_string())
.and_then(|k| workbook.model.get_cell(k))
.map(crate::model::cell::CellValue::edit_text)
.unwrap_or_default()
},
visible_rows: 20,
@@ -117,6 +117,22 @@ pub(super) mod test_helpers {
wb
}
/// `two_cat_model` with one `_Measure` formula per target, in order.
pub fn with_formulas(targets: &[&str]) -> Workbook {
let mut wb = two_cat_model();
for target in targets {
wb.model.add_formula(crate::formula::ast::Formula {
raw: format!("{target} = 0"),
target: target.to_string(),
target_category: "_Measure".to_string(),
expr: crate::formula::ast::Expr::Number(0.0),
filter: None,
format: None,
});
}
wb
}
pub fn effects_debug(effects: &[Box<dyn Effect>]) -> String {
format!("{:?}", effects)
}
crates/improvise-command/src/command/cmd/panel.rs
+88 -74
View File
@@ -192,11 +192,7 @@ mod tests {
let layout = make_layout(&m);
let reg = make_registry();
let ctx = make_ctx(&m, &layout, &reg);
let cmd = CyclePanelFocus {
formula_open: false,
category_open: false,
view_open: false,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
assert!(effects.is_empty());
}
@@ -208,11 +204,7 @@ mod tests {
let reg = make_registry();
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.formula_panel_open = true;
let cmd = CyclePanelFocus {
formula_open: true,
category_open: false,
view_open: false,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
assert_eq!(effects.len(), 1);
let dbg = format!("{:?}", effects[0]);
@@ -231,11 +223,7 @@ mod tests {
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.formula_panel_open = true;
ctx.category_panel_open = true;
let cmd = CyclePanelFocus {
formula_open: true,
category_open: true,
view_open: false,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
assert_eq!(effects.len(), 1);
let dbg = effects_debug(&effects);
@@ -256,11 +244,7 @@ mod tests {
ctx.formula_panel_open = true;
ctx.category_panel_open = true;
ctx.mode = &AppMode::FormulaPanel;
let cmd = CyclePanelFocus {
formula_open: true,
category_open: true,
view_open: false,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
assert_eq!(effects.len(), 1);
let dbg = effects_debug(&effects);
@@ -281,11 +265,7 @@ mod tests {
ctx.category_panel_open = true;
ctx.view_panel_open = true;
ctx.mode = &AppMode::CategoryPanel;
let cmd = CyclePanelFocus {
formula_open: false,
category_open: true,
view_open: true,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
assert_eq!(effects.len(), 1);
let dbg = effects_debug(&effects);
@@ -305,11 +285,7 @@ mod tests {
ctx.formula_panel_open = true;
ctx.view_panel_open = true;
ctx.mode = &AppMode::ViewPanel;
let cmd = CyclePanelFocus {
formula_open: true,
category_open: false,
view_open: true,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
assert_eq!(effects.len(), 1);
let dbg = effects_debug(&effects);
@@ -329,11 +305,7 @@ mod tests {
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.category_panel_open = true;
ctx.mode = &AppMode::CategoryPanel;
let cmd = CyclePanelFocus {
formula_open: false,
category_open: true,
view_open: false,
};
let cmd = CyclePanelFocus;
let effects = cmd.execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
@@ -344,15 +316,13 @@ mod tests {
#[test]
fn move_panel_cursor_down_from_zero() {
let m = two_cat_model();
let m = with_formulas(&["A", "B"]);
let layout = make_layout(&m);
let reg = make_registry();
let ctx = make_ctx(&m, &layout, &reg);
let cmd = MovePanelCursor {
panel: effect::Panel::Formula,
delta: 1,
current: 0,
max: 5,
};
let effects = cmd.execute(&ctx);
assert_eq!(effects.len(), 1);
@@ -363,17 +333,35 @@ mod tests {
);
}
/// Evidence: `context/test-evidence/command.md`.
#[test]
fn move_panel_cursor_moves_from_the_named_panels_own_cursor() {
let m = with_formulas(&["A", "B", "C", "D", "E"]);
let layout = make_layout(&m);
let reg = make_registry();
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.formula_cursor = 1;
let args = ["formula".to_string(), "2".to_string()];
let effects = reg
.interactive("move-panel-cursor", &args, &ctx)
.expect("move-panel-cursor resolves")
.execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("SetPanelCursor { panel: Formula, cursor: 3 }"),
"must move from the formula cursor (1) by the parsed delta (2), got: {dbg}"
);
}
#[test]
fn move_panel_cursor_clamps_at_zero() {
let m = two_cat_model();
let m = with_formulas(&["A", "B"]);
let layout = make_layout(&m);
let reg = make_registry();
let ctx = make_ctx(&m, &layout, &reg);
let cmd = MovePanelCursor {
panel: effect::Panel::Formula,
delta: -1,
current: 0,
max: 5,
};
let effects = cmd.execute(&ctx);
assert!(effects.is_empty());
@@ -388,46 +376,55 @@ mod tests {
let cmd = MovePanelCursor {
panel: effect::Panel::Formula,
delta: 1,
current: 0,
max: 0,
};
let effects = cmd.execute(&ctx);
assert!(effects.is_empty());
}
/// Evidence: `context/test-evidence/command.md`.
#[test]
fn delete_formula_at_cursor_with_formulas() {
let mut m = two_cat_model();
m.model.add_formula(crate::formula::ast::Formula {
raw: "Profit = Revenue - Cost".to_string(),
target: "Profit".to_string(),
target_category: "Type".to_string(),
expr: crate::formula::ast::Expr::Number(0.0),
filter: None,
format: None,
});
for (target, raw) in [("Profit", "Profit = 1"), ("Tax", "Tax = 2")] {
m.model.add_formula(crate::formula::ast::Formula {
raw: raw.to_string(),
target: target.to_string(),
target_category: "Type".to_string(),
expr: crate::formula::ast::Expr::Number(0.0),
filter: None,
format: None,
});
}
let layout = make_layout(&m);
let reg = make_registry();
let ctx = make_ctx(&m, &layout, &reg);
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.formula_cursor = 1;
let effects = DeleteFormulaAtCursor.execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("RemoveFormula { target: \"Profit\", target_category: \"Type\" }"),
"must remove the formula AT THE CURSOR, got: {dbg}"
dbg.contains("RemoveFormula { target: \"Tax\", target_category: \"Type\" }"),
"must remove the formula AT THE CURSOR (Tax, index 1), got: {dbg}"
);
assert!(
!dbg.contains("\"Profit\""),
"removed the first formula, not the one under the cursor: {dbg}"
);
}
/// Evidence: `context/test-evidence/command.md`.
#[test]
fn switch_view_at_cursor_with_valid_cursor() {
let m = two_cat_model();
let mut m = two_cat_model();
m.create_view("Other");
let layout = make_layout(&m);
let reg = make_registry();
let ctx = make_ctx(&m, &layout, &reg);
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.view_panel_cursor = 1;
let effects = SwitchViewAtCursor.execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("SwitchView(\"Default\")"),
"must switch to the view UNDER THE CURSOR, got: {dbg}"
dbg.contains("SwitchView(\"Other\")"),
"must switch to the view UNDER THE CURSOR (Other, index 1), got: {dbg}"
);
assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}");
}
@@ -504,6 +501,27 @@ mod tests {
);
}
/// Evidence: `context/test-evidence/command.md`.
#[test]
fn delete_view_at_cursor_deletes_the_view_under_a_nonzero_cursor() {
let mut m = two_cat_model();
m.create_view("Other");
m.create_view("Third");
let layout = make_layout(&m);
let reg = make_registry();
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.view_panel_cursor = 1;
let dbg = effects_debug(&DeleteViewAtCursor.execute(&ctx));
assert!(
dbg.contains("DeleteView(\"Other\")"),
"must delete the view UNDER THE CURSOR (Other, index 1), got: {dbg}"
);
assert!(
dbg.contains("SetPanelCursor { panel: View, cursor: 0 }"),
"a delete off position 0 steps the cursor back, got: {dbg}"
);
}
#[test]
fn delete_view_at_cursor_zero_does_not_adjust_cursor() {
let m = two_cat_model();
@@ -559,16 +577,15 @@ impl Cmd for TogglePanelAndFocus {
#[derive(Debug)]
pub struct TogglePanelVisibility {
pub panel: Panel,
pub currently_open: bool,
}
impl Cmd for TogglePanelVisibility {
fn name(&self) -> &'static str {
"toggle-panel-visibility"
}
fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
vec![Box::new(effect::SetPanelOpen {
panel: self.panel,
open: !self.currently_open,
open: !ctx.panel_open(self.panel),
})]
}
}
@@ -576,20 +593,16 @@ impl Cmd for TogglePanelVisibility {
/// Tab through open panels Formula → Category → View, wrapping, starting from the
/// focused one. One open panel self-transitions; none open is a no-op.
#[derive(Debug)]
pub struct CyclePanelFocus {
pub formula_open: bool,
pub category_open: bool,
pub view_open: bool,
}
pub struct CyclePanelFocus;
impl Cmd for CyclePanelFocus {
fn name(&self) -> &'static str {
"cycle-panel-focus"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let order = [
(self.formula_open, AppMode::FormulaPanel),
(self.category_open, AppMode::CategoryPanel),
(self.view_open, AppMode::ViewPanel),
(ctx.formula_panel_open, AppMode::FormulaPanel),
(ctx.category_panel_open, AppMode::CategoryPanel),
(ctx.view_panel_open, AppMode::ViewPanel),
];
// Index of the first candidate: the panel after the focused one,
// or the first panel when invoked from a non-panel mode.
@@ -614,16 +627,17 @@ impl Cmd for CyclePanelFocus {
pub struct MovePanelCursor {
pub panel: Panel,
pub delta: i32,
pub current: usize,
pub max: usize,
}
impl Cmd for MovePanelCursor {
fn name(&self) -> &'static str {
"move-panel-cursor"
}
fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let cursor = self.current;
let max = self.max;
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let (cursor, max) = match self.panel {
Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()),
Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()),
Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()),
};
if max == 0 {
return vec![];
}
crates/improvise-command/src/command/cmd/registry.rs
+30 -90
View File
@@ -27,19 +27,21 @@ fn cursor_key(ctx: &CmdContext) -> Result<CellKey, String> {
ctx.cell_key().clone().ok_or("no cell at cursor".into())
}
/// A `Move` built from args takes the live cursor as its origin.
fn move_from_cursor(cmd: Move, ctx: &CmdContext) -> Move {
Move {
cursor: CursorState::from_ctx(ctx),
..cmd
impl AtCursor for Move {
fn cursor_mut(&mut self) -> &mut CursorState {
&mut self.cursor
}
}
/// A commit built from args still advances from the live cursor.
fn commit_from_cursor(cmd: CommitAndAdvance, ctx: &CmdContext) -> CommitAndAdvance {
CommitAndAdvance {
cursor: CursorState::from_ctx(ctx),
..cmd
impl AtCursor for EnterAdvance {
fn cursor_mut(&mut self) -> &mut CursorState {
&mut self.cursor
}
}
impl AtCursor for CommitAndAdvance {
fn cursor_mut(&mut self) -> &mut CursorState {
&mut self.cursor
}
}
@@ -61,11 +63,6 @@ fn commit_at_cursor(
})
}
/// For a command with no context-only fields: `parse`'s result is used as built.
fn as_parsed<C>(cmd: C, _ctx: &CmdContext) -> C {
cmd
}
/// Build the default command registry with all commands.
/// Registry names MUST match the `Cmd::name()` return value.
pub fn default_registry() -> CmdRegistry {
@@ -90,7 +87,6 @@ pub fn default_registry() -> CmdRegistry {
key: parse_cell_key_from_args(args),
})
},
as_parsed,
Some(|_, ctx| {
Ok(ClearCellCommand {
key: cursor_key(ctx)?,
@@ -121,7 +117,7 @@ pub fn default_registry() -> CmdRegistry {
r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse);
// ── Navigation (unified Move) ──────────────────────────────────────
r.register(
r.register_at_cursor(
&Move {
kind: MoveKind::Relative(0, 0),
cursor: CursorState::default(),
@@ -137,13 +133,12 @@ pub fn default_registry() -> CmdRegistry {
cmd_name: "move-selection",
})
},
move_from_cursor,
None,
);
// Jump-to-edge commands: first/last row/col
macro_rules! reg_jump {
($r:expr, $is_row:expr, $to_end:expr, $name:expr) => {
$r.register(
$r.register_at_cursor(
&Move {
kind: if $to_end {
MoveKind::ToEnd($is_row)
@@ -164,7 +159,6 @@ pub fn default_registry() -> CmdRegistry {
cmd_name: $name,
})
},
move_from_cursor,
None,
);
};
@@ -173,7 +167,7 @@ pub fn default_registry() -> CmdRegistry {
reg_jump!(r, true, true, "jump-last-row");
reg_jump!(r, false, false, "jump-first-col");
reg_jump!(r, false, true, "jump-last-col");
r.register(
r.register_at_cursor(
&Move {
kind: MoveKind::Relative(0, 0),
cursor: CursorState::default(),
@@ -188,10 +182,9 @@ pub fn default_registry() -> CmdRegistry {
cmd_name: "scroll-rows",
})
},
move_from_cursor,
None,
);
r.register(
r.register_at_cursor(
&Move {
kind: MoveKind::Page(0),
cursor: CursorState::default(),
@@ -206,10 +199,9 @@ pub fn default_registry() -> CmdRegistry {
cmd_name: "page-scroll",
})
},
move_from_cursor,
None,
);
r.register(
r.register_at_cursor(
&EnterAdvance {
cursor: CursorState::default(),
},
@@ -227,9 +219,6 @@ pub fn default_registry() -> CmdRegistry {
},
})
},
|_, ctx| EnterAdvance {
cursor: CursorState::from_ctx(ctx),
},
None,
);
@@ -246,7 +235,6 @@ pub fn default_registry() -> CmdRegistry {
key: parse_cell_key_from_args(args),
})
},
as_parsed,
Some(|_, ctx| {
Ok(YankCell {
key: cursor_key(ctx)?,
@@ -265,7 +253,6 @@ pub fn default_registry() -> CmdRegistry {
key: parse_cell_key_from_args(args),
})
},
as_parsed,
Some(|_, ctx| {
Ok(PasteCell {
key: cursor_key(ctx)?,
@@ -318,7 +305,6 @@ pub fn default_registry() -> CmdRegistry {
key: parse_cell_key_from_args(args),
})
},
as_parsed,
Some(|_, ctx| {
Ok(DrillIntoCell {
key: cursor_key(ctx)?,
@@ -356,77 +342,30 @@ pub fn default_registry() -> CmdRegistry {
});
Ok(TogglePanelAndFocus { panel, target })
},
as_parsed,
None,
);
r.register(
&TogglePanelVisibility {
panel: Panel::Formula,
currently_open: false,
},
|args| {
require_args(args, 1)?;
let panel = parse_panel(&args[0])?;
Ok(TogglePanelVisibility {
panel,
currently_open: false,
})
},
|cmd, ctx| TogglePanelVisibility {
currently_open: ctx.panel_open(cmd.panel),
..cmd
},
None,
);
r.register(
&CyclePanelFocus {
formula_open: false,
category_open: false,
view_open: false,
},
|_| {
Ok(CyclePanelFocus {
formula_open: false,
category_open: false,
view_open: false,
})
},
|_, ctx| CyclePanelFocus {
formula_open: ctx.formula_panel_open,
category_open: ctx.category_panel_open,
view_open: ctx.view_panel_open,
Ok(TogglePanelVisibility { panel })
},
None,
);
r.register_nullary(|| Box::new(CyclePanelFocus));
r.register(
&MovePanelCursor {
panel: Panel::Formula,
delta: 0,
current: 0,
max: 0,
},
|args| {
require_args(args, 2)?;
let panel = parse_panel(&args[0])?;
let delta = args[1].parse::<i32>().map_err(|e| e.to_string())?;
Ok(MovePanelCursor {
panel,
delta,
current: 0,
max: 0,
})
},
|cmd, ctx| {
let (current, max) = match cmd.panel {
Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()),
Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()),
Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()),
};
MovePanelCursor {
current,
max,
..cmd
}
Ok(MovePanelCursor { panel, delta })
},
None,
);
@@ -493,7 +432,7 @@ pub fn default_registry() -> CmdRegistry {
// ── Commit ───────────────────────────────────────────────────────────
// These take a mode-name arg as args[0]; the command never inspects ctx.mode.
r.register(
r.register_at_cursor(
&CommitAndAdvance {
key: CellKey::new(vec![]),
value: String::new(),
@@ -514,10 +453,9 @@ pub fn default_registry() -> CmdRegistry {
edit_mode,
})
},
commit_from_cursor,
Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Down)),
);
r.register(
r.register_at_cursor(
&CommitAndAdvance {
key: CellKey::new(vec![]),
value: String::new(),
@@ -526,7 +464,6 @@ pub fn default_registry() -> CmdRegistry {
edit_mode: AppMode::editing(),
},
|_| Err("commit-and-advance-right requires context".into()),
commit_from_cursor,
Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)),
);
r.register(
@@ -538,7 +475,6 @@ pub fn default_registry() -> CmdRegistry {
let kind = parse_commit_kind(&args[0], &AppMode::Normal)?;
Ok(CommitBuffer { kind })
},
as_parsed,
Some(|args, ctx| {
require_args(args, 1)?;
let kind = parse_commit_kind(&args[0], ctx.mode)?;
@@ -863,12 +799,16 @@ mod panel_open_tests {
let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE);
for asked in Panel::ALL {
let args = [asked.name().to_string()];
let cmd = ctx
let effects = ctx
.registry
.interactive("toggle-panel-visibility", &args, &ctx)
.expect("toggle-panel-visibility resolves");
let dbg = format!("{cmd:?}");
let expected = format!("currently_open: {}", asked == opened);
.expect("toggle-panel-visibility resolves")
.execute(&ctx);
let dbg = format!("{effects:?}");
let expected = format!(
"SetPanelOpen {{ panel: {asked:?}, open: {} }}",
asked != opened
);
assert!(
dbg.contains(&expected),
"with only {opened:?} open, toggling {asked:?} read another \
crates/improvise-command/src/ui/app.rs
+39 -2
View File
@@ -761,14 +761,14 @@ impl App {
match crate::view::synthetic_record_info(key) {
Some((idx, col)) => self
.staged_drill_edit(layout, idx, col)
.or_else(|| layout.resolve_display(key))
.or_else(|| layout.records_value(idx, col).map(|v| v.edit_text()))
.unwrap_or_default(),
None => self
.model_state
.workbook
.model
.get_cell(key)
.map(|v| v.to_string())
.map(CellValue::edit_text)
.unwrap_or_default(),
}
}
@@ -2990,6 +2990,43 @@ mod tests {
);
}
/// improvise-cz8d: a refusal need not abort, so a failed document swap must
/// ask for the abort itself. Evidence: `context/test-evidence/command.md`.
#[test]
fn a_failed_load_or_import_does_not_run_the_quit_after_it() {
use improvise_protocol::keymap::Keymap;
for (command, path) in [("load", "absent.improv"), ("import-json", "absent.json")] {
let mut app = App::new(Workbook::new("T"), None);
app.storage = Box::new(crate::storage::MemoryFs::new());
let key = KeyCode::Char('x');
let mods = KeyModifiers::NONE;
let mut km = Keymap::new();
km.bind_seq(
key,
mods,
vec![(command, vec![path.to_string()]), ("force-quit", vec![])],
);
let effects = {
let ctx = app.cmd_context(key, mods);
app.keymap_set.dispatch_transient(&km, &ctx, key, mods)
}
.expect("both steps resolve in the registry");
app.apply_effects(effects);
assert!(
app.last_batch_refused(),
"setup: `{command}` must actually have failed, got {:?}",
app.view_state.status_msg
);
assert_ne!(
app.view_state.mode,
AppMode::Quit,
"`{command} {path} . q` quit having loaded nothing"
);
}
}
/// A prefix is consumed whether or not the next key matches, so a stray key
/// after `g` does nothing rather than acting as a fresh Normal binding.
#[test]
crates/improvise-command/src/ui/effect.rs
+63 -31
View File
@@ -44,6 +44,31 @@ pub enum ViewTarget {
Named(String),
}
/// Refuse AND stop the batch, for a failed document swap: what follows it in
/// a `:` line assumes it happened, so `load x . q` must not quit (cz8d).
fn refuse_and_stop(app: &mut App, why: impl Into<String>) {
app.refuse(why);
app.abort_effects = true;
}
/// Write the view a geometry effect names, or refuse: a view the document no
/// longer has is a failure to report, not a no-op to swallow (improvise-bok4).
fn on_view(app: &mut App, target: &ViewTarget, write: impl FnOnce(&mut crate::view::View)) {
let missing = match app.view_mut_of(target) {
Some(view) => {
write(view);
None
}
None => Some(match target {
ViewTarget::Named(name) => format!("No view named \"{name}\""),
ViewTarget::Active => "No active view".to_string(),
}),
};
if let Some(why) = missing {
app.refuse(why);
}
}
/// A discrete state change produced by a command.
/// Effects know how to apply themselves to the App.
pub trait Effect: Debug {
@@ -183,11 +208,13 @@ impl Effect for AddFormula {
// grid; _Measure targets arrive via `measure_item_names()`.
if !crate::model::category::CategoryKind::of_name(&formula.target_category)
.is_measure()
{
let _ = app
&& let Err(e) = app
.model_state
.workbook
.add_item(&formula.target_category, &formula.target);
.add_item(&formula.target_category, &formula.target)
{
// Its category may be gone: store no formula for it (40j4).
return app.refuse(e.to_string());
}
app.model_state.workbook.model.add_formula(formula);
}
@@ -252,9 +279,9 @@ impl Effect for EnterEditAtCursor {
pub struct TogglePruneEmpty(pub ViewTarget);
impl Effect for TogglePruneEmpty {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.0) {
on_view(app, &self.0, |v| {
v.prune_empty = !v.prune_empty;
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -318,7 +345,14 @@ impl Effect for CreateView {
pub struct DeleteView(pub String);
impl Effect for DeleteView {
fn apply(&self, app: &mut App) {
let _ = app.model_state.workbook.delete_view(&self.0);
// `delete_view` answers Ok for a name it lacks, which is a failure to
// report, not a deletion (improvise-40j4).
if !app.model_state.workbook.views.contains_key(&self.0) {
return app.refuse(format!("No view named \"{}\"", self.0));
}
if let Err(e) = app.model_state.workbook.delete_view(&self.0) {
return app.refuse(e.to_string());
}
// This client may have been the one on it. Idempotent for every other
// view; the mirrors owe the same move (`Sessions::adopt_deleted_views`).
app.view_state.active_view = app.view_name().to_string();
@@ -402,9 +436,9 @@ pub struct SetAxis {
}
impl Effect for SetAxis {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.view) {
on_view(app, &self.view, |v| {
v.set_axis(&self.category, self.axis);
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -419,9 +453,9 @@ pub struct SetPageSelection {
}
impl Effect for SetPageSelection {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.view) {
on_view(app, &self.view, |v| {
v.set_page_selection(&self.category, &self.item);
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -436,9 +470,9 @@ pub struct ToggleGroup {
}
impl Effect for ToggleGroup {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.view) {
on_view(app, &self.view, |v| {
v.toggle_group_collapse(&self.category, &self.group);
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -453,9 +487,9 @@ pub struct HideItem {
}
impl Effect for HideItem {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.view) {
on_view(app, &self.view, |v| {
v.hide_item(&self.category, &self.item);
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -470,9 +504,9 @@ pub struct ShowItem {
}
impl Effect for ShowItem {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.view) {
on_view(app, &self.view, |v| {
v.show_item(&self.category, &self.item);
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -483,9 +517,9 @@ impl Effect for ShowItem {
pub struct TransposeAxes(pub ViewTarget);
impl Effect for TransposeAxes {
fn apply(&self, app: &mut App) {
if let Some(v) = app.view_mut_of(&self.0) {
on_view(app, &self.0, |v| {
v.transpose_axes();
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -497,9 +531,9 @@ pub struct CycleAxis(pub ViewTarget, pub String);
impl Effect for CycleAxis {
fn apply(&self, app: &mut App) {
let category = self.1.clone();
if let Some(v) = app.view_mut_of(&self.0) {
on_view(app, &self.0, |v| {
v.cycle_axis(&category);
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -511,9 +545,9 @@ pub struct SetNumberFormat(pub ViewTarget, pub String);
impl Effect for SetNumberFormat {
fn apply(&self, app: &mut App) {
let format = self.1.clone();
if let Some(v) = app.view_mut_of(&self.0) {
on_view(app, &self.0, |v| {
v.number_format = format;
}
});
}
fn kind(&self) -> EffectKind {
EffectKind::Model(ModelChange::LayoutGeometry)
@@ -934,9 +968,7 @@ impl Effect for LoadModel {
app.install_workbook(loaded);
app.view_state.status_msg = format!("Loaded from {}", self.0.display());
}
Err(e) => {
app.refuse(format!("Load error: {e}"));
}
Err(e) => refuse_and_stop(app, format!("Load error: {e}")),
}
}
fn kind(&self) -> EffectKind {
@@ -993,7 +1025,7 @@ impl Effect for ImportJsonHeadless {
let bytes = match app.storage.read(&self.path) {
Ok(b) => b,
Err(e) => {
app.refuse(format!("Cannot read '{}': {e}", self.path.display()));
refuse_and_stop(app, format!("Cannot read '{}': {e}", self.path.display()));
return;
}
};
@@ -1004,7 +1036,7 @@ impl Effect for ImportJsonHeadless {
(recs, raw)
}
Err(e) => {
app.refuse(format!("CSV error: {e}"));
refuse_and_stop(app, format!("CSV error: {e}"));
return;
}
}
@@ -1012,21 +1044,21 @@ impl Effect for ImportJsonHeadless {
let content = match app.storage.read(&self.path) {
Ok(c) => c,
Err(e) => {
app.refuse(format!("Cannot read '{}': {e}", self.path.display()));
refuse_and_stop(app, format!("Cannot read '{}': {e}", self.path.display()));
return;
}
};
let value: serde_json::Value = match serde_json::from_slice(&content) {
Ok(v) => v,
Err(e) => {
app.refuse(format!("JSON parse error: {e}"));
refuse_and_stop(app, format!("JSON parse error: {e}"));
return;
}
};
match json_import_records(&value, self.array_path.as_deref()) {
Ok(recs) => (recs, value),
Err(msg) => {
app.refuse(msg);
refuse_and_stop(app, msg);
return;
}
}
@@ -1065,7 +1097,7 @@ impl Effect for ImportJsonHeadless {
};
}
Err(e) => {
app.refuse(format!("Import error: {e}"));
refuse_and_stop(app, format!("Import error: {e}"));
}
}
}
crates/improvise-command/src/ui/host.rs
+289 -7
View File
@@ -166,11 +166,12 @@ impl Host {
if let Some(reason) = reduce::broken_chain(&self.app, cmd.command()) {
return self.undo_frame(id, undo, mirrors, reason);
}
// Only a mutation runs `apply_effects`, and only `apply_effects`
// resets the flag — so after anything else it is a STALE answer.
// Only a mutation runs `apply_effects`, which resets both flags — so
// after anything else they are STALE. A refusal need not abort (cz8d).
let mutation = matches!(cmd.command(), Command::Model { .. });
out.extend(self.command(id, cmd));
if mutation && self.app.last_batch_aborted() {
let failed = self.app.last_batch_refused() || self.app.last_batch_aborted();
if mutation && failed {
return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused);
}
}
@@ -1541,10 +1542,12 @@ mod tests {
.any(|f| matches!(f.msg, ServerFrame::Snapshot(_))),
"a failed load cost every client its cursors: {frames:?}"
);
assert!(
host.app().view_state.status_msg.starts_with("Load error"),
"the fixture did not actually fail the load: {:?}",
host.app().view_state.status_msg
// Since improvise-bok4 a failed load is REFUSED, so the refusal is the
// witness that it failed; the host's status line is rolled back with it.
assert_eq!(
to(&frames, asker),
vec![ServerFrame::Rejected(Rejection::MutationRefused)],
"the fixture did not actually fail the load"
);
}
@@ -1778,4 +1781,283 @@ mod tests {
let json = serde_json::to_string(&inbound).unwrap();
assert_eq!(serde_json::from_str::<ClientFrame>(&json).unwrap(), inbound);
}
/// Two model commands, each caught up against the document as the client
/// saw it, so no basis tier can refuse the frame first (improvise-bok4).
fn two_command_frame(host: &Host, first: ModelCommand, then: ModelCommand) -> ClientFrame {
ClientFrame::Commands(vec![
crate::ui::reduce::caught_up(host.app(), first),
crate::ui::reduce::caught_up(host.app(), then),
])
}
/// improvise-bok4, the view-addressed case: the second command's view is
/// gone. Evidence: `context/test-evidence/command.md`.
#[test]
fn a_frame_writing_a_view_its_own_first_command_deleted_is_refused() {
let mut host = host();
host.app_mut().model_state.workbook.create_view("Scratch");
let (asker, _) = host.connect(10, 10);
let answer = host.handle(
asker,
two_command_frame(
&host,
ModelCommand::DeleteView("Scratch".into()),
ModelCommand::SetNumberFormat {
view: "Scratch".into(),
format: ",.2f".into(),
},
),
);
assert_eq!(
to(&answer, asker),
vec![ServerFrame::Rejected(Rejection::MutationRefused)],
"half the frame did nothing and was reported accepted: {answer:?}"
);
assert!(
host.app()
.model_state
.workbook
.views
.contains_key("Scratch"),
"the refused frame's deletion must be undone"
);
}
/// improvise-bok4, the category-addressed case: it narrated its failure into
/// the HOST's status line and told the client nothing.
#[test]
fn a_frame_adding_an_item_to_a_category_its_first_command_removed_is_refused() {
let mut host = host();
let (asker, _) = host.connect(10, 10);
let answer = host.handle(
asker,
two_command_frame(
&host,
ModelCommand::RemoveCategory("Region".into()),
ModelCommand::AddItem {
category: "Region".into(),
item: "East".into(),
},
),
);
assert_eq!(
to(&answer, asker),
vec![ServerFrame::Rejected(Rejection::MutationRefused)],
"half the frame did nothing and was reported accepted: {answer:?}"
);
assert!(
host.app()
.model_state
.workbook
.model
.category("Region")
.is_some(),
"the refused frame's removal must be undone"
);
}
/// The one-command case of the same class: deleting the last view was
/// swallowed with `let _`, so the frame was accepted having done nothing.
#[test]
fn deleting_the_documents_last_view_is_refused_rather_than_accepted() {
let mut host = host();
let (asker, _) = host.connect(10, 10);
let only = host.app().view_name().to_string();
let answer = host.model(asker, ModelCommand::DeleteView(only.clone()));
assert_eq!(
to(&answer, asker),
vec![ServerFrame::Rejected(Rejection::MutationRefused)],
"a deletion the workbook refused was reported accepted: {answer:?}"
);
assert!(host.app().model_state.workbook.views.contains_key(&only));
}
/// The control: two commands that both land are accepted, so the refusals
/// above are about a failure and not about the frame's shape.
#[test]
fn a_frame_whose_commands_all_land_is_accepted() {
let mut host = host();
host.app_mut().model_state.workbook.create_view("Scratch");
let (asker, _) = host.connect(10, 10);
let answer = host.handle(
asker,
two_command_frame(
&host,
ModelCommand::SetNumberFormat {
view: "Scratch".into(),
format: ",.2f".into(),
},
ModelCommand::AddItem {
category: "Region".into(),
item: "East".into(),
},
),
);
assert!(
to(&answer, asker).contains(&ServerFrame::Accepted),
"a frame with nothing to refuse was refused: {answer:?}"
);
}
/// improvise-40j4: the formula was stored for a category the frame's own
/// first command removed, and it survived save and reload.
#[test]
fn a_frame_adding_a_formula_to_a_category_its_first_command_removed_is_refused() {
let mut host = host();
let (asker, _) = host.connect(10, 10);
let answer = host.handle(
asker,
two_command_frame(
&host,
ModelCommand::RemoveCategory("Region".into()),
ModelCommand::AddFormula {
raw: "Total = 1".into(),
target_category: "Region".into(),
},
),
);
assert_eq!(
to(&answer, asker),
vec![ServerFrame::Rejected(Rejection::MutationRefused)],
"a formula for a vanished category was accepted: {answer:?}"
);
let model = &host.app().model_state.workbook.model;
assert!(
model.category("Region").is_some(),
"the removal must be undone"
);
assert!(
model.formulas().iter().all(|f| f.target != "Total"),
"and the formula must not be stored"
);
}
/// A host whose storage holds one file per way `ImportJsonHeadless` can
/// fail, plus nothing at the two paths that must fail to read.
fn host_with_import_fixtures() -> Host {
let storage = MemoryFs::new();
let files: [(&str, &[u8]); 4] = [
// Every field numeric, so no category is accepted and the build fails.
("measures.json", br#"[{"x": 1}, {"x": 2}]"#),
("bad.json", b"{not json"),
("nested.json", br#"{"data": [{"Region": "North"}]}"#),
// A record that is not UTF-8 is a CSV read error, not a warning.
("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"),
];
for (name, bytes) in files {
crate::storage::Storage::write(&storage, Path::new(name), bytes)
.expect("fixture writes");
}
// A second view, so deleting an absent one is not refused as "the last".
let mut wb = workbook();
wb.create_view("Scratch");
let mut app = App::new(wb, None);
app.storage = Box::new(storage);
app.rebuild_layout();
Host::new(app)
}
fn import(path: &str, array_path: Option<&str>) -> ModelCommand {
ModelCommand::ImportJsonHeadless {
path: PathBuf::from(path),
model_name: None,
array_path: array_path.map(String::from),
}
}
/// improvise-9fpl: one case per refusal #181 converted that no other test
/// witnesses. Evidence: `context/test-evidence/command.md`.
#[test]
fn every_failing_model_command_refuses_its_whole_frame() {
let cases: Vec<(&str, ModelCommand)> = vec![
(
"AddItemInGroup into an absent category",
ModelCommand::AddItemInGroup {
category: "Ghost".into(),
item: "East".into(),
group: "Coastal".into(),
},
),
(
"AddFormula that does not parse",
ModelCommand::AddFormula {
raw: "Total = (".into(),
target_category: "_Measure".into(),
},
),
(
"ImportJsonHeadless of an absent JSON",
import("absent.json", None),
),
(
"ImportJsonHeadless of an absent CSV",
import("absent.csv", None),
),
(
"ImportJsonHeadless of a CSV that is not text",
import("bad.csv", None),
),
(
"ImportJsonHeadless of JSON that does not parse",
import("bad.json", None),
),
(
"ImportJsonHeadless at a path with no array",
import("nested.json", Some("nope")),
),
(
"ImportJsonHeadless that builds no model",
import("measures.json", None),
),
// improvise-40j4: `delete_view` answers Ok for a name it lacks.
(
"DeleteView of a view the document lacks",
ModelCommand::DeleteView("Ghost".into()),
),
(
"AddFormula into a category the document lacks",
ModelCommand::AddFormula {
raw: "Total = 1".into(),
target_category: "Ghost".into(),
},
),
];
for (what, failing) in cases {
let mut host = host_with_import_fixtures();
let (asker, _) = host.connect(10, 10);
let landed = ModelCommand::AddItem {
category: "Region".into(),
item: "Landed".into(),
};
let answer = host.handle(asker, two_command_frame(&host, landed, failing));
assert_eq!(
to(&answer, asker),
vec![ServerFrame::Rejected(Rejection::MutationRefused)],
"{what}: the frame was not refused: {answer:?}"
);
assert!(
!host
.app()
.model_state
.workbook
.model
.category("Region")
.is_some_and(|c| c.ordered_item_names().contains(&"Landed")),
"{what}: the half of the frame that landed was not undone"
);
}
}
}
crates/improvise-command/src/ui/project.rs
+1 -1
View File
@@ -277,7 +277,7 @@ fn cell_display(
// with `parse::<f64>()`, which called a coordinate `2024` a number.
let text = value.as_ref().map(|v| v.to_string()).unwrap_or_default();
return CellDisplay {
edit_text: Some(text.clone()),
edit_text: Some(value.as_ref().map(CellValue::edit_text).unwrap_or_default()),
text,
numeric: matches!(value, Some(CellValue::Number(_))),
error: matches!(value, Some(CellValue::Error(_))),
crates/improvise-core/src/model/cell.rs
+11
View File
@@ -134,6 +134,17 @@ impl CellValue {
}
}
impl CellValue {
/// The text an editor opens on, which commits back to this value unchanged.
/// `Display` rounds for the screen; this must not (`context/code/core.md` §17).
pub fn edit_text(&self) -> String {
match self {
CellValue::Number(n) => nonfinite_token(*n).map_or_else(|| n.to_string(), String::from),
CellValue::Text(_) | CellValue::Error(_) => self.to_string(),
}
}
}
impl std::fmt::Display for CellValue {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
crates/improvise-core/src/view/layout.rs
+3 -12
View File
@@ -656,13 +656,6 @@ impl GridLayout {
axis_label(&self.col_items, col)
}
/// Display string for a synthetic records-mode key; `None` for a pivot
/// key.
pub fn resolve_display(&self, key: &CellKey) -> Option<String> {
let (row, col) = synthetic_record_info(key)?;
self.records_display(row, col)
}
/// Display text for (row, col) in either mode.
pub fn display_text(
&self,
@@ -1045,7 +1038,7 @@ mod tests {
}
#[test]
fn records_mode_resolve_display_returns_values() {
fn records_mode_records_display_returns_values() {
let mut wb = records_workbook();
let v = wb.active_view_mut();
v.set_axis("_Index", Axis::Row);
@@ -1057,13 +1050,11 @@ mod tests {
// A measure column resolves to that measure's value
let value_col = cols.iter().position(|c| c == "Revenue").unwrap();
let key = layout.cell_key(0, value_col).unwrap();
assert_eq!(layout.resolve_display(&key).as_deref(), Some("100"));
assert_eq!(layout.records_display(0, value_col).as_deref(), Some("100"));
// Category column resolves to the coordinate value
let region_col = cols.iter().position(|c| c == "Region").unwrap();
let key = layout.cell_key(0, region_col).unwrap();
let display = layout.resolve_display(&key).unwrap();
let display = layout.records_display(0, region_col).unwrap();
assert!(
!display.is_empty(),
"Region column should resolve to a value"
crates/improvise-protocol/src/render.rs
+1 -1
View File
@@ -82,7 +82,7 @@ pub struct CellDisplay {
/// **typed** it, not as it is drawn. The difference corrupts data (§3).
pub edit_text: Option<String>,
/// The value the host evaluated here, unrounded and untyped-by-text: what
/// `yank` copies. Neither `text` nor `edit_text` can be re-parsed into it (§3).
/// `yank` copies. `text` cannot be re-parsed into it, nor can text like `007` (§3).
pub value: Option<CellValue>,
}
crates/improvise-protocol/src/source_scan.rs
+417 -75
View File
@@ -11,28 +11,28 @@ pub fn production_code(roots: &[PathBuf]) -> Vec<(PathBuf, String)> {
for root in roots {
collect_rust(root, &mut files);
}
let code: Vec<(PathBuf, String)> = files
let read: Vec<(PathBuf, Source)> = files
.into_iter()
.map(|file| {
let text = std::fs::read_to_string(&file).expect("a listed source reads");
let code = code_only(&text);
(file, code)
let source = Source::of(&text, &file);
(file, source)
})
.collect();
let test_modules: BTreeSet<PathBuf> = code
let test_modules: BTreeSet<PathBuf> = read
.iter()
.flat_map(|(file, code)| test_module_paths(file, code))
.flat_map(|(file, source)| test_module_paths(file, source))
.collect();
code.into_iter()
read.into_iter()
.filter(|(file, _)| !test_modules.iter().any(|m| is_module_file(m, file)))
.map(|(file, code)| (file, strip_test_items(&code)))
.map(|(file, source)| (file, source.production()))
.collect()
}
/// One file's text as [`production_code`] cleans it. Which *files* are test
/// modules needs their declaring file too, so only `production_code` knows.
pub fn production_text(text: &str) -> String {
strip_test_items(&code_only(text))
Source::of(text, Path::new("<text>")).production()
}
fn collect_rust(dir: &Path, into: &mut Vec<PathBuf>) {
@@ -55,83 +55,319 @@ fn is_module_file(m: &Path, file: &Path) -> bool {
file == m.with_extension("rs") || file.starts_with(m)
}
/// One file, read once. Its `cfg` predicates are read off the TEXT, whose
/// `feature` values are literals; every bracket off the code (improvise-59q8).
struct Source {
code: Vec<char>,
/// Test-only items, as `(attribute start, item start, item end)`.
items: Vec<(usize, usize, usize)>,
/// A leading `#![cfg(test)]`: the whole file is test code.
whole_file: bool,
}
impl Source {
fn of(text: &str, file: &Path) -> Self {
let code: Vec<char> = code_only(text).chars().collect();
let text: Vec<char> = text.chars().collect();
let (items, whole_file) = test_only_items(&text, &code, file);
Self {
code,
items,
whole_file,
}
}
/// The code with every test-only item removed.
fn production(&self) -> String {
if self.whole_file {
return String::new();
}
let mut kept = String::with_capacity(self.code.len());
let mut at = 0;
for &(start, _, end) in &self.items {
kept.extend(&self.code[at..start]);
at = end;
}
kept.extend(&self.code[at..]);
kept
}
fn item_code(&self, start: usize, end: usize) -> String {
self.code[start..end].iter().collect()
}
}
/// The out-of-line modules `file` declares under a test-only `cfg`, as paths
/// without extension (`#[cfg(test)] mod x;` is `<dir>/x`).
fn test_module_paths(file: &Path, code: &str) -> Vec<PathBuf> {
fn test_module_paths(file: &Path, source: &Source) -> Vec<PathBuf> {
let dir = match file.file_stem().and_then(|s| s.to_str()) {
Some("lib" | "main" | "mod") => file.parent().map(Path::to_path_buf),
Some(stem) => file.parent().map(|p| p.join(stem)),
None => None,
}
.expect("a source file has a directory and a stem");
let lines: Vec<&str> = code.lines().map(str::trim).collect();
let mut found = Vec::new();
for (i, line) in lines.iter().enumerate() {
if !is_test_cfg(line) {
continue;
}
let item = lines[i + 1..]
.iter()
.find(|l| !l.starts_with("#[") && !l.is_empty());
if let Some(name) = item.and_then(|l| out_of_line_mod(l)) {
found.push(dir.join(name));
}
}
found
source
.items
.iter()
.filter_map(|&(_, start, end)| out_of_line_mod(source.item_code(start, end).trim()))
.map(|name| dir.join(name))
.collect()
}
/// `mod x;`, `pub mod x;`, `pub(crate) mod x;` → `x`.
fn out_of_line_mod(line: &str) -> Option<&str> {
let rest = line.split_once("mod ")?.1.strip_suffix(';')?;
let prefix = line.split_once("mod ")?.0.trim();
(prefix.is_empty() || prefix.starts_with("pub")).then_some(rest.trim())
fn out_of_line_mod(item: &str) -> Option<String> {
let (prefix, rest) = item.split_once("mod ")?;
let name = rest.strip_suffix(';')?.trim();
let prefix = prefix.trim();
(prefix.is_empty() || prefix.starts_with("pub")).then(|| name.to_string())
}
/// A `cfg` attribute that compiles its item only under `test`. `cfg(not(test))`
/// is production, and `"test-support"` is a literal, already blanked.
fn is_test_cfg(line: &str) -> bool {
let line = line.trim();
line.starts_with("#[cfg(")
&& !line.contains("not(")
&& line
.split(|c: char| !c.is_alphanumeric() && c != '_')
.any(|word| word == "test")
/// Every test-only item in a file, bounded by balanced brackets on the code
/// rather than by indentation. An item it cannot bound is a panic, never EOF.
fn test_only_items(
text: &[char],
code: &[char],
file: &Path,
) -> (Vec<(usize, usize, usize)>, bool) {
let mut items = Vec::new();
let mut k = 0;
while k < code.len() {
let Some((inner, end)) = attribute_at(code, k) else {
k += 1;
continue;
};
let predicate: String = text[k..end].iter().collect();
if !is_test_only(&predicate) {
k = end;
continue;
}
if inner {
// An inner attribute heads the file or it heads an inline module;
// only the first is a shape this scan can honour.
let leading = code[..k].iter().all(|c| c.is_whitespace());
assert!(
leading,
"{}: an inner test-only cfg could not be bounded",
file.display()
);
return (Vec::new(), true);
}
let start = past_attributes(code, end);
let item_end = item_end(code, start).unwrap_or_else(|| {
panic!(
"{}: a test-only item at char {k} could not be bounded",
file.display()
)
});
items.push((k, start, item_end));
k = item_end;
}
(items, false)
}
/// Drop every test-only item. rustfmt puts an item's closing brace at the
/// attribute's own indentation, which is what makes this a line scan (§2).
fn strip_test_items(text: &str) -> String {
let lines: Vec<&str> = text.lines().collect();
let mut kept = String::new();
let mut i = 0;
while i < lines.len() {
let line = lines[i];
match is_test_cfg(line) {
false => {
kept.push_str(line);
kept.push('\n');
i += 1;
}
true => {
let indent = line.len() - line.trim_start().len();
let close = format!("{}}}", " ".repeat(indent));
i += 1;
while i < lines.len() && lines[i].trim_start().starts_with("#[") {
i += 1;
}
match lines.get(i).is_some_and(|l| l.trim_end().ends_with(';')) {
true => i += 1,
false => {
while i < lines.len() && lines[i].trim_end() != close {
i += 1;
}
i += 1;
}
/// An attribute starting at `k`: whether it is inner (`#!`) and where it ends.
fn attribute_at(code: &[char], k: usize) -> Option<(bool, usize)> {
if code[k] != '#' || (k > 0 && (code[k - 1].is_alphanumeric() || code[k - 1] == '_')) {
return None;
}
let inner = code.get(k + 1) == Some(&'!');
let open = k + 1 + usize::from(inner);
(code.get(open) == Some(&'[')).then(|| matching(code, open).map(|close| (inner, close + 1)))?
}
/// The index of the bracket closing the one at `open`, counting all three kinds.
fn matching(code: &[char], open: usize) -> Option<usize> {
let mut depth = 0usize;
for (k, &c) in code.iter().enumerate().skip(open) {
match c {
'(' | '[' | '{' => depth += 1,
')' | ']' | '}' => {
depth -= 1;
if depth == 0 {
return Some(k);
}
}
_ => {}
}
}
kept
None
}
/// Skip whitespace and any further outer attributes on the same item.
fn past_attributes(code: &[char], mut k: usize) -> usize {
loop {
while code.get(k).is_some_and(|c| c.is_whitespace()) {
k += 1;
}
let next = (k < code.len()).then(|| attribute_at(code, k)).flatten();
match next {
Some((false, end)) => k = end,
_ => return k,
}
}
}
/// Where the item starting at `start` ends: a `;` at depth zero, or the `}`
/// closing its body. `None` at the end of the file, which is not an end.
fn item_end(code: &[char], start: usize) -> Option<usize> {
let mut depth = 0usize;
let mut k = start;
while k < code.len() {
match code[k] {
'(' | '[' | '{' => depth += 1,
';' if depth == 0 => return Some(k + 1),
// The enclosing block closed first: the item was its last expression.
')' | ']' | '}' if depth == 0 => return Some(k),
')' | ']' => depth -= 1,
'}' => {
depth -= 1;
if depth == 0 && !continues(code, k + 1) {
return Some(k + 1);
}
}
_ => {}
}
k += 1;
}
None
}
/// Whether the item goes on after a `}` at depth zero: `S { .. };`, a method
/// call on a block, or an `if` with an `else`.
fn continues(code: &[char], after: usize) -> bool {
let rest: String = code[after..]
.iter()
.skip_while(|c| c.is_whitespace())
.take(5)
.collect();
rest.starts_with([';', '.', '?']) || rest.starts_with("else")
}
/// A `cfg` attribute whose predicate no shipped build satisfies. Evaluated,
/// not substring-matched (improvise-59q8): `context/code/protocol.md` §2.
fn is_test_only(attribute: &str) -> bool {
let tokens = cfg_tokens(attribute);
match tokens.as_slice() {
[Token::Word(cfg), Token::Open, rest @ ..] if cfg == "cfg" => {
let mut at = 0;
predicate(rest, &mut at) == Truth::No
}
_ => false,
}
}
/// What a `cfg` predicate is in a build without `test`. `Unknown` for what a
/// source scan cannot know, such as a target or an ordinary feature.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum Truth {
Yes,
No,
Unknown,
}
#[derive(Debug, Clone, PartialEq, Eq)]
enum Token {
Word(String),
Str(String),
Open,
Close,
Comma,
Equals,
}
/// The tokens of an attribute's text, from its first word: `#`, `!` and the
/// brackets around it are dropped.
fn cfg_tokens(attribute: &str) -> Vec<Token> {
let inner = attribute
.trim_start_matches('#')
.trim_start_matches('!')
.trim()
.trim_start_matches('[')
.trim_end()
.trim_end_matches(']');
let chars: Vec<char> = inner.chars().collect();
let mut tokens = Vec::new();
let mut k = 0;
while k < chars.len() {
let c = chars[k];
match c {
'(' => tokens.push(Token::Open),
')' => tokens.push(Token::Close),
',' => tokens.push(Token::Comma),
'=' => tokens.push(Token::Equals),
'"' => {
let end = quoted_end(&chars, k + 1, '"');
tokens.push(Token::Str(chars[k + 1..end - 1].iter().collect()));
k = end;
continue;
}
c if c.is_alphanumeric() || c == '_' => {
let word: String = chars[k..]
.iter()
.take_while(|c| c.is_alphanumeric() || **c == '_')
.collect();
k += word.chars().count();
tokens.push(Token::Word(word));
continue;
}
_ => {}
}
k += 1;
}
tokens
}
/// One predicate, advancing `at` past it. `test` is false; so is the
/// `test-support` feature, which this repo never ships (improvise-26dm).
fn predicate(tokens: &[Token], at: &mut usize) -> Truth {
let Some(Token::Word(word)) = tokens.get(*at) else {
return Truth::Unknown;
};
*at += 1;
match (word.as_str(), tokens.get(*at)) {
(op @ ("all" | "any" | "not"), Some(Token::Open)) => {
*at += 1;
let mut args = Vec::new();
while !matches!(tokens.get(*at), Some(Token::Close) | None) {
args.push(predicate(tokens, at));
if tokens.get(*at) == Some(&Token::Comma) {
*at += 1;
}
}
*at += 1;
combine(op, &args)
}
(key, Some(Token::Equals)) => {
*at += 1;
let value = match tokens.get(*at) {
Some(Token::Str(v)) => v.clone(),
_ => String::new(),
};
*at += 1;
match (key, value.as_str()) {
("feature", "test-support") => Truth::No,
_ => Truth::Unknown,
}
}
("test", _) => Truth::No,
_ => Truth::Unknown,
}
}
/// Kleene's three-valued `all`, `any` and `not`.
fn combine(op: &str, args: &[Truth]) -> Truth {
match op {
"all" if args.contains(&Truth::No) => Truth::No,
"all" if args.iter().all(|a| *a == Truth::Yes) => Truth::Yes,
"any" if args.contains(&Truth::Yes) => Truth::Yes,
"any" if args.iter().all(|a| *a == Truth::No) => Truth::No,
"not" => match args.first() {
Some(Truth::Yes) => Truth::No,
Some(Truth::No) => Truth::Yes,
_ => Truth::Unknown,
},
_ => Truth::Unknown,
}
}
/// `text` with comments, string, byte-string, raw-string and char literals
@@ -166,8 +402,8 @@ fn literal_or_comment_end(c: &[char], i: usize) -> Option<usize> {
(Some('/'), Some('/')) => Some(line_end(c, i)),
(Some('/'), Some('*')) => Some(block_end(c, i)),
(Some('"'), _) => Some(quoted_end(c, i + 1, '"')),
(Some('b'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')),
(Some('r' | 'b'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i),
(Some('b' | 'c'), Some('"')) if !word_before => Some(quoted_end(c, i + 2, '"')),
(Some('r' | 'b' | 'c'), Some('r' | '#' | '"')) if !word_before => raw_end(c, i),
(Some('\''), _) => char_end(c, i),
_ => None,
}
@@ -211,9 +447,10 @@ fn quoted_end(c: &[char], mut k: usize, quote: char) -> usize {
c.len()
}
/// `r"…"`, `r#"…"#`, `br#"…"#`. `None` when the `r` is only an identifier.
/// `r"…"`, `r#"…"#`, `br#"…"#`, `cr#"…"#`. `None` when the `r` is only an
/// identifier.
fn raw_end(c: &[char], i: usize) -> Option<usize> {
let mut k = i + usize::from(c[i] == 'b');
let mut k = i + usize::from(matches!(c[i], 'b' | 'c'));
if c.get(k) != Some(&'r') {
return None;
}
@@ -283,21 +520,20 @@ mod tests {
"}\n",
"#[cfg(test)]\n",
"mod out_of_line;\n",
"#[cfg(any(test, feature = \"x\"))]\n",
"#[cfg(any(test, feature = \"test-support\"))]\n",
"pub mod gated;\n",
"#[cfg(not(test))]\n",
"fn production_only() {}\n",
"fn also_kept() {}\n",
);
let code = code_only(text);
let stripped = strip_test_items(&code);
let stripped = production_text(text);
for kept in ["fn kept", "fn production_only", "fn also_kept"] {
assert!(stripped.contains(kept), "{kept} was dropped: {stripped}");
}
assert!(!stripped.contains("gone_inline"), "{stripped}");
let lib = Path::new("/c/src/lib.rs");
assert_eq!(
test_module_paths(lib, &code),
test_module_paths(lib, &Source::of(text, lib)),
vec![
PathBuf::from("/c/src/out_of_line"),
PathBuf::from("/c/src/gated")
@@ -305,7 +541,7 @@ mod tests {
);
let nested = Path::new("/c/src/ui/reduce.rs");
assert_eq!(
test_module_paths(nested, &code)[0],
test_module_paths(nested, &Source::of(text, nested))[0],
PathBuf::from("/c/src/ui/reduce/out_of_line")
);
assert!(is_module_file(
@@ -322,6 +558,112 @@ mod tests {
));
}
/// improvise-59q8, the shapes that HID production code: a test-only item
/// whose end the line scan could not find. Evidence: test-evidence/protocol.md.
#[test]
fn a_test_only_item_hides_nothing_after_it() {
let cases = [
(
"a multi-line item ending in `];`",
concat!(
"impl X {\n",
" #[cfg(test)]\n",
" const NAMES: &[&str] = &[\n",
" \"a\",\n",
" ];\n",
" fn prod() { let c = Command::Hidden1(1); }\n",
"}\n",
"fn after() { Command::Visible(2) }\n",
),
),
(
"a multi-line statement ending in `);`",
concat!(
"fn f() {\n",
" #[cfg(test)]\n",
" let x = foo(\n",
" 1,\n",
" );\n",
" let c = Command::Hidden1(1);\n",
"}\n",
"fn after() { Command::Visible(2) }\n",
),
),
(
"a one-line `#[cfg(test)] use`",
concat!(
"#[cfg(test)] use foo::bar;\n",
"fn prod() { Command::Hidden1(1) }\n",
"fn after() { Command::Visible(2) }\n",
),
),
];
for (what, text) in cases {
let kept = production_text(text);
for name in ["Hidden1", "Visible"] {
assert!(kept.contains(name), "{what}: {name} was hidden: {kept:?}");
}
}
}
/// improvise-59q8, the shapes that COUNTED test code as production.
#[test]
fn a_test_only_cfg_is_recognised_whatever_its_spelling() {
let cases = [
(
"`all(test, not(..))`",
"#[cfg(all(test, not(target_arch = \"wasm32\")))]\nfn t() { Command::TestOnly(1) }\n",
),
(
"an inner `#![cfg(test)]` heading the file",
"#![cfg(test)]\nfn t() { Command::TestOnly(1) }\n",
),
(
"an attribute split across lines",
"#[cfg(\n test\n)]\nfn t() { Command::TestOnly(1) }\n",
),
];
for (what, text) in cases {
let kept = production_text(text);
assert!(
!kept.contains("TestOnly"),
"{what} was read as production: {kept:?}"
);
}
// The controls: a predicate a shipped build can satisfy is production.
for text in [
"#[cfg(not(test))]\nfn p() { Command::Shipped(1) }\n",
"#[cfg(any(test, feature = \"serde\"))]\nfn p() { Command::Shipped(1) }\n",
"#[cfg(target_arch = \"wasm32\")]\nfn p() { Command::Shipped(1) }\n",
] {
assert!(
production_text(text).contains("Shipped"),
"{text:?} was dropped"
);
}
}
/// improvise-59q8's minor case: a C string literal leaked its contents.
#[test]
fn a_c_string_literal_is_blanked() {
// The raw one quotes inside itself, so lexing it as ordinary strings
// leaves the text BETWEEN the inner quotes in the open.
let kept = code_only(
"let a = c\"Command::InC(1)\"; let b = cr#\"x\" Command::InRawC(2) \"y\"#;\n",
);
for gone in ["InC", "InRawC"] {
assert!(!kept.contains(gone), "{gone} survived: {kept}");
}
}
/// A test-only item the scan cannot bound is REFUSED, never read to the
/// end of the file, which is what silently hid every shape above.
#[test]
#[should_panic(expected = "could not be bounded")]
fn an_unbounded_test_only_item_is_refused_not_swallowed() {
let _ = production_text("#[cfg(test)]\nfn broken() {\n Command::X(1)\n");
}
#[test]
fn comments_and_literals_are_blanked_and_code_is_kept() {
let text = concat!(
crates/improvise-wasm-client/tests/lockstep.rs
+120
View File
@@ -1830,3 +1830,123 @@ fn yy_over_an_aggregate_copies_the_sum_the_host_copies() {
"client and host yanked different values over an aggregate"
);
}
/// Two regions by two measures, `value` at (South, Revenue): row 1, column 1,
/// so "the cell under the cursor" is not "the first cell".
fn precise_host(value: improvise_protocol::CellValue) -> App {
let mut wb = Workbook::new("Test");
wb.add_category("Region").unwrap();
wb.add_item("Region", "North").unwrap();
wb.add_item("Region", "South").unwrap();
wb.add_item("_Measure", "Cost").unwrap();
wb.add_item("_Measure", "Revenue").unwrap();
wb.active_view_mut().set_axis("Region", Axis::Row);
wb.active_view_mut().set_axis("_Measure", Axis::Column);
for (region, measure, v) in [
("North", "Cost", improvise_protocol::CellValue::Number(1.0)),
(
"North",
"Revenue",
improvise_protocol::CellValue::Number(2.0),
),
("South", "Cost", improvise_protocol::CellValue::Number(3.0)),
("South", "Revenue", value),
] {
wb.model.set_cell(region_measure(region, measure), v);
}
let mut app = App::new(wb, None);
app.term_height = 2 + 8;
app.rebuild_layout();
app.cursor_mut().selected = (1, 1);
app
}
/// BUG (improvise-umi4) — see `context/test-evidence/clients.md`.
#[test]
fn an_untouched_browser_edit_stores_the_identical_value() {
use improvise_protocol::CellValue::{Number, Text};
let window = (2, 2);
for value in [
Number(1.23456789),
Number(0.1 + 0.2),
Number(98765.4321987),
Number(123456789012345.67),
Number(-0.00001234),
Number(-0.0),
Number(f64::NEG_INFINITY),
Text("0.1 + 0.2".into()),
] {
let mut host = precise_host(value.clone());
let mut client = client_ready_to_edit(&host, window);
for key in ["i", "Enter"] {
let event = BrowserKey::new(key).to_key_event().unwrap();
let KeyOutcome::Local(cmds) = resolve(&client, event) else {
panic!("{key}: the client must answer this key locally");
};
for cmd in &cmds {
client.fold(cmd);
}
apply_on(&mut host, &cmds);
}
assert!(
!client.view.mode.is_editing(),
"Enter did not commit, so nothing was written and this proves nothing"
);
let stored = host
.model_state
.workbook
.model
.get_cell(&region_measure("South", "Revenue"))
.cloned();
let same = match (&stored, &value) {
(Some(Number(a)), Number(b)) => a.to_bits() == b.to_bits(),
(stored, value) => stored.as_ref() == Some(value),
};
assert!(same, "i then Enter on {value:?} stored {stored:?}");
}
}
/// BUG (improvise-umi4) — see `context/test-evidence/clients.md`.
#[test]
fn every_projected_edit_text_commits_back_to_its_value() {
use improvise_protocol::CellValue::Number;
let value = Number(1.23456789);
for records in [false, true] {
let mut app = precise_host(value.clone());
if records {
reduce_key(
&mut app,
KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE),
);
}
assert_eq!(app.layout.is_records_mode(), records, "wrong fixture mode");
let grid = grid_of(
&app,
Viewport {
row_offset: 0,
col_offset: 0,
rows: 8,
cols: 8,
},
);
let cells: Vec<_> = grid
.cells
.iter()
.flatten()
.filter(|cell| matches!(cell.id, Some(improvise_protocol::CellId::Record(_))))
.collect();
assert!(
cells.iter().any(|cell| cell.value.as_ref() == Some(&value)),
"records={records}: the precise value was never projected, so this proves nothing"
);
for cell in cells {
let text = cell.edit_text.clone().unwrap_or_default();
assert_eq!(
improvise_protocol::typed(&text),
cell.value,
"records={records}: edit text {text:?} does not commit back to {:?}",
cell.value
);
}
}
}
scripts/pr-watch
+27 -15
View File
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# Waits for a PR's checks to finish and says what happened. Every rule in here
# is one this repo got wrong first; see context/code/pr-watch.md.
# is one this repo got wrong first; see context/code/tooling.md section 3.
set -euo pipefail
usage() {
@@ -11,10 +11,12 @@ usage: pr-watch <pr-number> [--expect <sha>] [--timeout <sec>] [--interval <sec>
--timeout <sec> give up after this long (default 1800)
--interval <sec> seconds between polls (default 20)
exit 0 every check COMPLETED and none failed
exit 1 a check failed, or the PR is not mergeable
exit 0 every check COMPLETED, none failed, and GitHub says MERGEABLE with a
state of CLEAN, UNSTABLE or HAS_HOOKS
exit 1 a check failed, or the PR is not mergeable (any other known state)
exit 2 the head moved during the wait, or is not --expect
exit 3 timed out with checks still running
exit 3 timed out with checks still running, or mergeability still UNKNOWN,
or still BLOCKED with every registered check green
exit 4 usage or environment error
EOF
exit 4
@@ -86,13 +88,28 @@ while :; do
total="$(printf '%s' "$json" | jqf '[.statusCheckRollup[]?] | length')"
running="$(printf '%s' "$json" | jqf '[.statusCheckRollup[]? | select(.status != "COMPLETED")] | length')"
failed="$(printf '%s' "$json" | jqf \
'[.statusCheckRollup[]? | select(.conclusion != "SUCCESS" and .conclusion != "SKIPPED" and .conclusion != "NEUTRAL")] | length')"
mergeable="$(printf '%s' "$json" | jqf '.mergeable')"
state="$(printf '%s' "$json" | jqf '.mergeStateStatus')"
# UNKNOWN and green-but-BLOCKED are waited on: see context/code/tooling.md,
# "pr-watch and mergeability" (improvise-edpl, improvise-zlj2).
if [ "$total" -gt 0 ] && [ "$running" -eq 0 ]; then
break
if [ "$failed" -gt 0 ] || [ "$mergeable" = CONFLICTING ]; then break; fi
case "$mergeable/$state" in
UNKNOWN/*|*/UNKNOWN|MERGEABLE/BLOCKED) ;;
*) break ;;
esac
fi
now=$(date +%s)
if [ $((now - started)) -ge "$TIMEOUT" ]; then
echo "pr-watch: timed out after ${TIMEOUT}s with $running of $total still running" >&2
if [ "$total" -gt 0 ] && [ "$running" -eq 0 ]; then
echo "pr-watch: timed out after ${TIMEOUT}s with mergeability $mergeable/$state" >&2
else
echo "pr-watch: timed out after ${TIMEOUT}s with $running of $total still running" >&2
fi
exit 3
fi
sleep "$INTERVAL"
@@ -100,15 +117,10 @@ done
printf '%s' "$json" | jqf '.statusCheckRollup[]? | select(.conclusion != "SKIPPED") | " \(.conclusion) \(.name)"'
failed="$(printf '%s' "$json" | jqf \
'[.statusCheckRollup[]? | select(.conclusion != "SUCCESS" and .conclusion != "SKIPPED" and .conclusion != "NEUTRAL")] | length')"
mergeable="$(printf '%s' "$json" | jqf '.mergeable')"
state="$(printf '%s' "$json" | jqf '.mergeStateStatus')"
echo "pr-watch: head=$head $mergeable/$state $total checks, $failed failed"
[ "$failed" -eq 0 ] || exit 1
# UNSTABLE means a non-required check is unhappy; it is not a merge blocker on
# its own, so only an outright non-mergeable state fails here.
[ "$mergeable" != "CONFLICTING" ] || exit 1
exit 0
# An allowlist, so a state GitHub adds later fails closed. DIRTY and BLOCKED
# can wear a MERGEABLE flag: context/code/tooling.md, "pr-watch and mergeability".
[ "$mergeable" = MERGEABLE ] || exit 1
case "$state" in CLEAN|UNSTABLE|HAS_HOOKS) exit 0 ;; *) exit 1 ;; esac
scripts/pr-watch-test.sh
+91
View File
@@ -73,5 +73,96 @@ run_case "a head that moves mid-wait is not a pass" 2 "$work/moved" --timeout 60
run_case "--expect refuses a different head" 2 "$work/green" --expect bbbbbbbb
run_case "--expect accepts a prefix of the head" 0 "$work/green" --expect aaaa
# improvise-edpl. GitHub computes mergeability lazily: for a while after the
# base moves it reports UNKNOWN, and PRs #172 and #179 both exited 0 there.
# This is the shape both had: every check green, mergeability unresolved.
green_rollup='[{"name":"build","status":"COMPLETED","conclusion":"SUCCESS"}]'
red_rollup='[{"name":"build","status":"COMPLETED","conclusion":"FAILURE"}]'
# $1 dir, $2 file, $3 mergeable, $4 mergeStateStatus, $5 rollup (JSON values)
state() {
mkdir -p "$1"
printf '{"headRefOid":"aaaaaaaaaaaa","mergeable":%s,"mergeStateStatus":%s,"statusCheckRollup":%s}\n' \
"$3" "$4" "$5" > "$1/$2"
}
state "$work/unk-conflict" 1.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup"
state "$work/unk-conflict" last.json '"CONFLICTING"' '"DIRTY"' "$green_rollup"
run_case "UNKNOWN that resolves to CONFLICTING fails" 1 "$work/unk-conflict" --timeout 60
state "$work/unk-clean" 1.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup"
state "$work/unk-clean" 2.json '"MERGEABLE"' '"UNKNOWN"' "$green_rollup"
state "$work/unk-clean" last.json '"MERGEABLE"' '"CLEAN"' "$green_rollup"
run_case "UNKNOWN that resolves to MERGEABLE/CLEAN passes" 0 "$work/unk-clean" --timeout 60
state "$work/unk-forever" last.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup"
run_case "UNKNOWN until the timeout is a timeout, not a pass" 3 "$work/unk-forever"
# A failed check fails whatever mergeability says; waiting on it would turn a
# known red into a timeout.
state "$work/unk-red" last.json '"UNKNOWN"' '"UNKNOWN"' "$red_rollup"
run_case "a failed check fails without waiting on mergeability" 1 "$work/unk-red" --timeout 0
# The whole table, enumerated rather than hand-picked, every check green. The
# rule it states is in context/code/tooling.md, "pr-watch and mergeability".
for m in MERGEABLE CONFLICTING UNKNOWN; do
for s in BEHIND BLOCKED CLEAN DIRTY DRAFT HAS_HOOKS UNKNOWN UNSTABLE SOMETHING_NEW null; do
if [ "$m" = CONFLICTING ]; then want=1
elif [ "$m" = UNKNOWN ] || [ "$s" = UNKNOWN ]; then want=3
else case "$s" in CLEAN|UNSTABLE|HAS_HOOKS) want=0 ;; BLOCKED) want=3 ;; *) want=1 ;; esac
fi
sv="\"$s\""; [ "$s" = null ] && sv=null
state "$work/table-$m-$s" last.json "\"$m\"" "$sv" "$green_rollup"
run_case "table: $m/$s" "$want" "$work/table-$m-$s" --timeout 0
done
done
# improvise-zlj2. The required "Browser harness" check comes from a job that
# `needs: build`, so its check run does not exist until build finishes. In that
# gap every registered check is green and GitHub says MERGEABLE/BLOCKED. The
# rollup below is PR #189's, verbatim; the gap states drop or rewind entries.
cat > "$work/rollup.jsonl" <<'EOF'
{"__typename":"CheckRun","completedAt":"2026-09-23T04:41:53Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406461","name":"Build all bundles","startedAt":"2026-09-23T04:40:15Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047406233","name":"plan","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"Release"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047482378","name":"build-local-artifacts (${{ join(matrix.targets, ', ') }})","startedAt":"2026-09-23T04:40:32Z","status":"COMPLETED","workflowName":"Release"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:42:22Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406506","name":"Test and lint (host)","startedAt":"2026-09-23T04:40:16Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047482733","name":"build-global-artifacts","startedAt":"2026-09-23T04:40:32Z","status":"COMPLETED","workflowName":"Release"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:40Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406409","name":"wasm32 check (browser bundle crates)","startedAt":"2026-09-23T04:40:15Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047482801","name":"host","startedAt":"2026-09-23T04:40:33Z","status":"COMPLETED","workflowName":"Release"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:25Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406415","name":"Browser JS unit tests (node --test)","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:32Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261564/job/107047483313","name":"announce","startedAt":"2026-09-23T04:40:33Z","status":"COMPLETED","workflowName":"Release"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:41:17Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406264","name":"Knowledge base consistency","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:40:39Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047406373","name":"Lint the harness crate","startedAt":"2026-09-23T04:40:14Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:44:17Z","conclusion":"SUCCESS","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107047753280","name":"Browser harness (OPFS contract + shell)","startedAt":"2026-09-23T04:41:56Z","status":"COMPLETED","workflowName":"CI"}
{"__typename":"CheckRun","completedAt":"2026-09-23T04:44:18Z","conclusion":"SKIPPED","detailsUrl":"https://github.com/fiddlerwoaroof/improvise/actions/runs/35819261571/job/107048259684","name":"Deploy to GitHub Pages","startedAt":"2026-09-23T04:44:18Z","status":"COMPLETED","workflowName":"CI"}
EOF
harness='Browser harness (OPFS contract + shell)'
deploy='Deploy to GitHub Pages'
# $1 dir, $2 file, $3 mergeable, $4 state, $5 jq filter over the rollup array
gap_state() {
state "$1" "$2" "\"$3\"" "\"$4\"" "$(jq -cs "$5" "$work/rollup.jsonl")"
}
build_running='map(if .name == "Build all bundles" then .status = "IN_PROGRESS" | .conclusion = "" else . end)'
not_yet="map(select(.name != \"$harness\" and .name != \"$deploy\"))"
harness_running="map(if .name == \"$harness\" then .status = \"IN_PROGRESS\" | .conclusion = \"\" else . end) | map(select(.name != \"$deploy\"))"
gap_state "$work/gap" 1.json MERGEABLE BLOCKED "$not_yet | $build_running"
gap_state "$work/gap" 2.json MERGEABLE BLOCKED "$not_yet"
gap_state "$work/gap" 3.json MERGEABLE BLOCKED "$harness_running"
gap_state "$work/gap" last.json MERGEABLE CLEAN '.'
run_case "the needs-gap before the required check registers is not a failure" 0 "$work/gap" --timeout 60
gap_state "$work/gap-never" last.json MERGEABLE BLOCKED "$not_yet"
run_case "a required check that never registers is a timeout" 3 "$work/gap-never"
# improvise-h3ni. The head must stay pinned across the UNKNOWN wait, or a
# verdict about b is reported as a verdict about a.
state "$work/unk-moved" 1.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup"
state "$work/unk-moved" 2.json '"UNKNOWN"' '"UNKNOWN"' "$green_rollup"
state "$work/unk-moved" last.json '"MERGEABLE"' '"CLEAN"' "$green_rollup"
for f in 2.json last.json; do
sed 's/aaaaaaaaaaaa/bbbbbbbbbbbb/' "$work/unk-moved/$f" > "$work/unk-moved/$f.new"
mv -f "$work/unk-moved/$f.new" "$work/unk-moved/$f"
done
run_case "a head that moves during the UNKNOWN wait is not a pass" 2 "$work/unk-moved" --timeout 60
printf '\npr-watch-test: %d passed, %d failed\n' "$pass" "$fail"
[ "$fail" -eq 0 ]