diff --git a/apps/nec-gui/src/app_state.rs b/apps/nec-gui/src/app_state.rs index a397549..1f552ed 100644 --- a/apps/nec-gui/src/app_state.rs +++ b/apps/nec-gui/src/app_state.rs @@ -200,6 +200,20 @@ pub struct EditorState { pub error: Option, /// Result of the most recent Save, shown to the user. pub save_status: String, + /// **The file this document belongs to** — the path a plain `Save` writes to. + /// + /// Deliberately *not* `deck_path`. That field is global chrome, editable on + /// the Editor tab itself, and `Save` used to clone it live: load deck A, + /// retype the path to B without loading it, click Save, and A's text + /// truncated B (FND-103). The document's file is a different fact from the + /// path the chrome is pointing at, and conflating them cost an unrelated + /// file. + /// + /// `None` means the document has no file yet. Unreachable in the shipped UI + /// today — Save exists only once `loaded` is true, and only an accepted load + /// sets that — but `Save` refuses rather than guessing, so a future "New + /// deck" button cannot inherit the truncation. + pub file_path: Option, } /// State of the GPU 3-D viewport. The camera and mesh are pure data (rendered by @@ -644,6 +658,17 @@ impl AppState { self.editor_save_run } + /// The file a plain `Save` writes to, or `None` if this document has none. + /// + /// Exists so the decision is reachable by a test. It used to be one inline + /// `self.state.deck_path.clone()` in the binary's `spawn_save`, which no test + /// could see — which is why FND-103 shipped: the defect lived in the one + /// place the suite could not look, under a comment that said it did the right + /// thing ("write it back over the loaded path"). + pub fn save_target(&self) -> Option<&str> { + self.editor.file_path.as_deref() + } + /// As [`AppState::current_solve_run`], for the viewport's geometry leg. pub fn current_geometry_run(&self) -> Option { self.viewport.pending_geometry @@ -907,6 +932,23 @@ impl AppState { } Message::EditDeckLoaded(_, Ok(doc)) => { self.editor_load_run = None; + // A save still in flight is retired here too — but NOT by this + // arm. `refresh_editor_preview()`, called at the end of it, + // already clears both run ids unconditionally (FND-133/#445), so + // the sequence "Save A, load B, save completes" cannot rebind + // this document to A. The design review of this change predicted + // that hole and it does not exist; an explicit clear added here + // was removed after a sabotage showed it changed nothing. Pinned + // by `a_completed_load_retires_a_save_still_in_flight`. + // + // The path this document now belongs to. Not carried in the + // message: an ACCEPTED load implies `deck_path` is unchanged + // since the load was armed, because `DeckPathChanged` retires the + // load run — pinned by + // `a_deck_path_change_retires_a_load_spawned_for_the_old_path`. + // Empty is normalised to `None` so a state that never had a path + // does not acquire an empty one. + self.editor.file_path = Some(self.deck_path.clone()).filter(|p| !p.is_empty()); self.editor.doc = doc.clone(); self.editor.history.reset(); self.editor.loaded = true; @@ -1002,12 +1044,29 @@ impl AppState { self.refresh_editor_preview(); } Message::SaveDeck => { + // Decided here rather than in the binary, so it is reachable by a + // test: `spawn_save` follows the armed id and no longer chooses a + // path of its own. Falling back to `deck_path` would BE the + // defect — `to_deck_string()` succeeds on an empty document, so + // the fallback would truncate whatever path was typed to an empty + // deck. + if self.editor.file_path.is_none() { + self.editor.save_status = "This document has no file yet — use Save as…".into(); + return; + } self.editor.save_status = "Saving…".into(); let id = self.mint_run(); self.editor_save_run = Some(id); } Message::DeckSaved(_, Ok(path)) => { self.editor_save_run = None; + // Both save routes end here — the async task from `SaveDeck` and + // the inline write from "Save as…" — so binding the document to + // the file it was just written to fixes the second half of + // FND-103 for free: Save-as to C used to leave the document bound + // to whatever it was before, so the NEXT plain Save went back to + // the old file rather than to C. + self.editor.file_path = Some(path.clone()); self.editor.doc.mark_saved(); self.editor.save_status = format!("Saved to {path}"); } diff --git a/apps/nec-gui/src/main.rs b/apps/nec-gui/src/main.rs index d8deaf6..ef44b44 100644 --- a/apps/nec-gui/src/main.rs +++ b/apps/nec-gui/src/main.rs @@ -314,12 +314,21 @@ impl FnecGui { move |r| Message::EditDeckLoaded(run, r), ) } else if spawn_save { - // Render the edited deck and write it back over the loaded path. - let path = self.state.deck_path.clone(); - let run = self - .state - .current_edit_save_run() - .expect("the save was just armed"); + // Render the edited deck and write it back over the file this + // document belongs to — which is NOT `deck_path`. That field is + // chrome the user can retype without loading, and cloning it live + // meant "load A, retype B, Save" truncated B with A's text (FND-103). + // + // Both values come from the reducer now: it decides whether a save + // happens at all (a document with no file refuses and arms nothing) + // and which file it targets. This branch follows, and can no longer + // pick a path of its own. + let (Some(run), Some(path)) = ( + self.state.current_edit_save_run(), + self.state.save_target().map(str::to_owned), + ) else { + return Task::none(); + }; match self.state.editor.doc.to_deck_string() { Ok(text) => Task::perform( async move { @@ -1197,6 +1206,21 @@ impl FnecGui { }; let save_status = text(self.state.editor.save_status.clone()).width(Length::Fill); + // Which file `Save` will write to, stated rather than implied. + // + // Not decoration. `Save` targets the document's own file, which can + // differ from the deck path in the chrome above — retype that box, or + // use "Save as…", and the two diverge. Before FND-103 they could not + // diverge because Save simply used the chrome, which is exactly how it + // came to truncate an unrelated file. Fixing the target without showing + // it would leave the user unable to tell where Save goes except by + // clicking it. + let editing_line = text(match self.state.save_target() { + Some(p) => format!("Editing: {p}"), + None => "Editing: (no file yet — use Save as…)".to_string(), + }) + .width(Length::Fill); + // ── Sources & environment (EX/GN/LD/FR editors) ────────────────────── let add_bar = row![ text("Sources & environment"), @@ -1241,6 +1265,7 @@ impl FnecGui { status, controls, solve_line, + editing_line, save_status, ] .spacing(8) diff --git a/apps/nec-gui/tests/gui_smoke.rs b/apps/nec-gui/tests/gui_smoke.rs index 1771e02..d7148b1 100644 --- a/apps/nec-gui/tests/gui_smoke.rs +++ b/apps/nec-gui/tests/gui_smoke.rs @@ -1008,9 +1008,17 @@ FR 0 1 21.0 0 EN "; +/// The path `loaded_editor()` pretends the document came from. +const LOADED_FROM: &str = "/tmp/fnec-loaded-from.nec"; + fn loaded_editor() -> AppState { let mut state = AppState::default(); let doc = load_model_doc_str(EDITOR_DECK).expect("parse doc"); + // A path, because a load binds the document to whatever `deck_path` said + // when it was armed (FND-103). Without one every `Save` in this suite would + // exercise the has-no-file refusal instead of the save it means to test — + // passing for the wrong reason. + state.apply(&Message::DeckPathChanged(LOADED_FROM.into())); state.apply(&Message::EditDeckLoad); let run = state .current_edit_load_run() @@ -3126,3 +3134,148 @@ fn a_deck_with_no_frequency_is_refused_by_both_gui_seams() { ); } } + +/// `Save` writes to the file the document was loaded from, not to whatever the +/// deck-path box currently says. +/// +/// The defect (FND-103): the path box is global chrome, editable on the Editor +/// tab itself, and `spawn_save` cloned it live. Load deck A, retype the box to B +/// without loading it, click Save — and A's text truncated B. Ordinary click +/// sequence, default config, an unrelated file destroyed. +/// +/// The assertion is on `save_target()` rather than on a written file because the +/// write happens in the binary's `Task`. Extracting that decision into the +/// reducer is half the fix: the defect lived in the one place the suite could +/// not look, under a comment claiming it wrote "back over the loaded path". +#[test] +fn save_targets_the_loaded_file_not_the_retyped_path() { + let mut state = loaded_editor(); + assert_eq!( + state.save_target(), + Some(LOADED_FROM), + "a load must bind the document to the file it came from" + ); + + state.apply(&Message::DeckPathChanged( + "/tmp/fnec-unrelated-B.nec".into(), + )); + assert_eq!( + state.deck_path, "/tmp/fnec-unrelated-B.nec", + "the chrome follows the user's typing" + ); + assert_eq!( + state.save_target(), + Some(LOADED_FROM), + "but the document still belongs to the file it was loaded from — this is \ + the truncation FND-103 caused" + ); +} + +/// "Save as…" rebinds the document, so the next plain `Save` goes to the new +/// file. +/// +/// Not in the FND-103 row; found by measuring it. `BrowseSaveDeck` wrote the +/// file and marked the document clean but bound nothing, so after saving as C a +/// later `Save` went back to the previous file — which no editor does. +#[test] +fn save_as_rebinds_the_document_to_the_new_file() { + let mut state = loaded_editor(); + state.apply(&Message::BrowseSaveDeck); + let run = state + .current_edit_save_run() + .expect("Save as… arms a save run"); + state.apply(&Message::DeckSaved( + run, + Ok("/tmp/fnec-saved-as-C.nec".into()), + )); + + assert_eq!( + state.save_target(), + Some("/tmp/fnec-saved-as-C.nec"), + "after Save as… the document belongs to the new file" + ); + assert!( + !state.editor.doc.dirty, + "and is clean, as it already was before this change" + ); +} + +/// A load retires a save still in flight, so a completed save cannot rebind a +/// document it was not written from. +/// +/// The risk is real: run identity rejects a *superseded* run, and a save spawned +/// for file A is not superseded by a **load**. Save A is armed, the user loads +/// deck B, the load lands and binds the document to B, and then the older +/// `DeckSaved(Ok(A))` arrives — if it were accepted it would rebind B's document +/// to A, putting B's text one click of Save away from overwriting A. FND-103 +/// again, one load later. +/// +/// **It is already prevented, and not by this change.** The design review +/// predicted this hole; sabotage showed it does not exist. An accepted load ends +/// by calling `refresh_editor_preview()`, which clears both run ids +/// unconditionally (FND-133/#445) — so an explicit clear added to the load arm +/// changed nothing and was removed. What this test adds is coverage of the +/// **load** path for that mechanism, where +/// `an_edit_retires_a_deck_write_still_in_flight` covers the edit path: removing +/// the clear from `refresh_editor_preview` fails both. +#[test] +fn a_completed_load_retires_a_save_still_in_flight() { + let mut state = loaded_editor(); + state.apply(&Message::SaveDeck); + let stale_save = state + .current_edit_save_run() + .expect("SaveDeck arms the save"); + + // The user points at another deck and loads it before the write finishes. + state.apply(&Message::DeckPathChanged("/tmp/fnec-deck-B.nec".into())); + state.apply(&Message::EditDeckLoad); + let load = state + .current_edit_load_run() + .expect("EditDeckLoad arms the load"); + let doc_b = load_model_doc_str(EDITOR_DECK_ALT).expect("parse alt doc"); + state.apply(&Message::EditDeckLoaded(load, Ok(doc_b))); + assert_eq!( + state.save_target(), + Some("/tmp/fnec-deck-B.nec"), + "the load binds the document to its own file" + ); + + // The write for the PREVIOUS document now completes. + state.apply(&Message::DeckSaved(stale_save, Ok(LOADED_FROM.into()))); + assert_eq!( + state.save_target(), + Some("/tmp/fnec-deck-B.nec"), + "a save that completed for the previous document must not rebind this one" + ); +} + +/// A document with no file refuses to plain-`Save` rather than guessing. +/// +/// Unreachable in the shipped UI today — Save exists only once a deck is loaded, +/// and only an accepted load sets that — so this is a guard for the future "New +/// deck" button. It matters because the tempting fallback, "use `deck_path`", is +/// the defect: `to_deck_string()` succeeds on an empty document, so the fallback +/// would truncate whatever path was typed to an empty deck. +#[test] +fn a_document_with_no_file_refuses_to_save() { + let mut state = AppState::default(); + state.apply(&Message::DeckPathChanged( + "/tmp/fnec-typed-but-never-loaded.nec".into(), + )); + assert_eq!( + state.save_target(), + None, + "nothing was ever loaded or saved" + ); + + state.apply(&Message::SaveDeck); + assert!( + state.current_edit_save_run().is_none(), + "no save may be armed for a document with no file" + ); + assert!( + state.editor.save_status.contains("Save as"), + "and the user is told what to do instead: {}", + state.editor.save_status + ); +} diff --git a/docs/changelog.md b/docs/changelog.md index d184d6d..884c8fc 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -15,6 +15,22 @@ from 0.13.0 and earlier predate the Keep a Changelog headings and are left as wr ## [Unreleased] +### Fixed + +- **The GUI's `Save deck` writes to the file the document belongs to, not to + whatever the deck-path box says.** That box is global chrome, editable on the + Editor tab itself, and `Save` cloned it live — so loading deck A, retyping the + box to B without loading it, and clicking Save truncated B with A's text + (FND-103). Ordinary click sequence, default config, an unrelated file + destroyed. +- **`Save as…` now rebinds the document**, so a later `Save deck` goes to the + file just written rather than back to the previous one. Not recorded in the + ledger row; found by measuring it. +- The editor shows an **`Editing:`** line naming the file `Save` will write to. + The document's file and the chrome's deck path are two different facts and can + legitimately differ; before this they could not, because Save simply used the + chrome, which is how it came to truncate an unrelated file. + ### Changed - **A deck with no frequency at all is refused instead of silently succeeding.** diff --git a/docs/gui-guide.md b/docs/gui-guide.md index 3911b69..c136b0c 100644 --- a/docs/gui-guide.md +++ b/docs/gui-guide.md @@ -2,7 +2,7 @@ project: fnec-rust doc: docs/gui-guide.md status: living -last_updated: 2026-08-27 +last_updated: 2026-09-08 --- # fnec-gui user guide @@ -124,8 +124,15 @@ have yet, and each row's **Del** to remove one. - **Undo / Redo** (or `Ctrl+Z` / `Ctrl+Shift+Z` / `Ctrl+Y`) — full edit history; typing a value coalesces into one undo step. -- **Save deck** writes back over the loaded path; **Save as…** opens a native - save dialog. +- **Save deck** writes back over the file the document belongs to — the one it + was loaded from, or the one the last **Save as…** wrote. That file is shown on + the *Editing:* line under the editor, and it is **not** necessarily the deck + path in the box at the top: retyping that box points the Load/Solve chrome + somewhere else without moving the document. Until v0.18.0 `Save deck` used the + box, so loading deck A, retyping the box to B and clicking Save truncated B + with A's text (FND-103). +- **Save as…** opens a native save dialog and rebinds the document to the file it + writes, so a later **Save deck** goes there too. - **Apply + Solve** solves the edited in-memory deck and shows the impedance, without saving first. diff --git a/docs/project/findings-ledger.md b/docs/project/findings-ledger.md index 7b67ce0..6616681 100644 --- a/docs/project/findings-ledger.md +++ b/docs/project/findings-ledger.md @@ -42,6 +42,8 @@ An `open` row is not a failure — it is the point. What the process forbids is | ID | Found | State | Finding | Evidence / owner | |:---|:------|:------|:--------|:-----------------| +| FND-153 | 2026-09-08 | open | **[low] Saving an edited deck destroys its `--vars` template tokens.** The editor loads the *substituted* text (`main.rs` passes `vars_path` to the load), so the document holds `14.2` where the file on disk held `$FREQ`. Writing it back over the loaded path replaces a template with one of its instantiations, silently. | Found by fable's design review of #452, 2026-09-08; read, not executed. Pre-existing and not introduced by that change, but `EditorState::file_path` sharpens it: the document is now explicitly bound to the file it came from, and that file may be a template it can no longer represent. The honest fixes are to refuse to Save a document loaded with a vars file, or to round-trip the tokens; both are behaviour decisions. | +| FND-152 | 2026-09-08 | open | **[low] Both GUI deck writes are non-atomic.** `std::fs::write` truncates before writing at the `SaveDeck` task and at the inline `BrowseSaveDeck` write, so an interrupted save leaves a truncated or partial deck where a complete one was. | Split out of FND-103 by #452, which fixed that row's path-binding defects and deliberately did not touch this one: write-to-temp-then-rename is a separate change with its own failure modes (permissions, cross-device rename, leftover temp files) and no test in the suite currently exercises an interrupted write. `crates/nec_project`'s converter already reads before writing for the sibling reason, so the pattern exists in-tree to copy. | | FND-151 | 2026-09-08 | open | **[low] `pre_solve_error` cannot see the resolved frequency list, so the frequency checks are split across two seams.** `validate::frequency_error` validates the values of an `FR` card that exists, `validate::no_frequency_error` (added by #451) covers the case where no frequency was resolved from any source, and the worker has a third check on the wire frequency (`is_usable_frequency_mhz`, FND-098). All four frontends hold their resolved list before they call `pre_solve_error`, so the gate could take `freqs_hz: &[f64]` and fold all three into one. | Proposed by fable's design review of #451, 2026-09-08, and deliberately deferred out of that change: it is a four-frontend signature change, `diagnose` gains a parameter, and the GUI's `deck_warnings` placeholder (`solve.rs:396`, `unwrap_or(0.0)`) has to decide whether to pass an empty slice or `[0.0]`. Recorded so the split is a known state rather than an accident. | | FND-150 | 2026-09-08 | open | **[low] `fnec --hosts /nonexistent.toml` on a frequency-less deck exited 0 without reporting the missing file.** The early return for an empty frequency list preceded the hosts file being read, so a bad `--hosts` path was never diagnosed — the run simply succeeded in silence. | Found by fable's design review of #451, 2026-09-08, while enumerating what the early return was hiding. #451 makes that path exit 1 with the frequency reason, so the silent success is gone; whether a missing `--hosts` file should be reported *in preference to* the missing frequency is a separate ordering question, unexamined. | | FND-149 | 2026-09-07 | open | **[low] The toolchain is pinned in CI and nowhere else, so the pre-commit hook lints against whatever clippy the host has.** There is no `rust-toolchain.toml`; `.github/workflows/ci.yml` names `dtolnay/rust-toolchain@1.97.1` in seven places, and `.githooks/pre-commit` runs the workspace `cargo clippy -- -D warnings` against the installed toolchain. On a host one minor version ahead, three pre-existing `needless_late_init` sites in `crates/nec_solver/src/linear.rs` (and a fourth in `apps/nec-cli/src/solve_session.rs`) became hard errors, so **every** commit in the repo required `--no-verify` — a gate that has to be bypassed in order to commit. | Found 2026-09-07 on clippy 0.1.98 while committing #448. The four sites were fixed there, which clears the symptom and not the cause: the next clippy release does it again, and CI cannot see it because CI pins. The fix is a `rust-toolchain.toml` at 1.97.1 with CI reading it instead of naming a version seven times — one pin, one file, which is what `reproducible-builds` asks for. | @@ -90,7 +92,7 @@ An `open` row is not a failure — it is the point. What the process forbids is | FND-106 | 2026-08-28 | fixed | **[medium] FNEC_ACCEL_STUB_GPU is documented as a live testing control and manipulated by 14 test files, but no shipped code reads it** | Found by the 2026-08-28 whole-project audit (R38); confirmed by adversarial verification. cli-guide.md:749 documents FNEC_ACCEL_STUB_GPU as a live control; grep across all src trees returns ZERO hits (exit 1). ph7-chk-001-gpu-stub-retirement.md explicitly documents its removal from nec_accel. Exactly 14 test files still set/env_remove it -- genuine env manipulation, not comments, spot-checked in two. Inert plumbing for a control that no longer exists. Detail: `docs/dev/reviews/review-260828.md`. — fixed in PR #440: 53 inert manipulations of FNEC_ACCEL_STUB_GPU removed from 14 test files, along with the cli-guide line documenting it as a live control. Zero shipped code read it, and ph7-chk-001 documents its deliberate removal -- so two docs disagreed and the code sided with the one saying it was retired. | | FND-105 | 2026-08-28 | fixed | **[medium] CLI auto-exec 'startup execution probe' reads a hardwired stub, so auto mode can never select GPU; the real adapter check lives at a different seam** | Found by the 2026-08-28 whole-project audit (R37); confirmed by adversarial verification. dispatch_frequency_point is an unconditional FallbackToCpu with no cfg/feature gate, so gpu_available is compile-time-constant false and two auto branches are reachable only from hand-built test probes. Ran on this host (which has a working RADV GPU): 'gpu_available=false ... selected_exec=cpu' -- a constant presented as a probe result. Stale tracker confirmed. CORRECTION/DOWNGRADE: the finder framed this as users losing GPU speed, but warnings.rs records the GPU-resident solve as 0.04x-0.48x of CPU at every tested size, so declining it costs nothing. What remains is an honesty/dead-code defect. Also cli-guide.md:376 is stale in the OPPOSITE direction. Detail: `docs/dev/reviews/review-260828.md`. — fixed in PR #440 by renaming, not by wiring. The probe printed `gpu_available=false`, which reads as 'this machine has no GPU' and is FALSE here -- fnec's wgpu far-field kernels do use this machine's GPU. What the field reported was `dispatch_frequency_point`, an unconditional FallbackToCpu because PH7-CHK-004 is unwired, so a compile-time constant was presented as a probe result. Now `per_freq_gpu_dispatch` and `hybrid_gpu_lane_dispatch`, which is what it measures. Renaming rather than wiring is the benchmark's verdict, not laziness: warnings.rs records the GPU-resident solve at 0.04x-0.48x of CPU at every tested size, so declining it costs nothing. The row's own downgrade note said the same -- the defect was the claim, not the routing. | | FND-104 | 2026-08-28 | open | **[medium] hosts.toml scheduling overrides are parsed, documented as live controls, and read by nothing** | Found by the 2026-08-28 whole-project audit (R36); confirmed by adversarial verification. SshWorkerHandle never reads the two override fields, so detect_capability cannot apply them, contradicting probe_capability's doc comment which is simply false. assignment_weight/CapabilityCache/connect_all have only definitions, doc comments, re-exports and #[cfg(test)] references. No 'not yet wired' note anywhere. CORRECTION: the shipped path is NOT plain round-robin -- main.rs:786 calls dispatch_batch, a work-stealing pull loop that already self-balances by speed, so the practical loss is mostly the documented 'cap a shared node' case. Detail: `docs/dev/reviews/review-260828.md`. | -| FND-103 | 2026-08-28 | open | **[medium] Save writes the edited document to whatever deck_path currently says — not the file it was loaded from — and mark_saved clears the dirty flag for edits made during the async write; all deck writes are non-atomic** | Found by the 2026-08-28 whole-project audit (R34); confirmed by adversarial verification. spawn_save's own comment says 'write it back over the loaded path' but the code clones the LIVE deck_path; EditorState records no loaded-path field to bind to, and DeckPathChanged does not clear or reload the editor document. The path field is global chrome editable on the Editor tab itself, so load A / retype B / Save truncates B with A's text. Default config, ordinary click sequence. mark_saved race is real but sub-millisecond and encoded by an existing test. Non-atomic writes confirmed at three sites. CORRECTION: project_cmd.rs already reads before writing with an explicit comment, so in-place convert cannot lose data to a render failure. Detail: `docs/dev/reviews/review-260828.md`. | +| FND-103 | 2026-08-28 | fixed | **[medium] Save writes the edited document to whatever deck_path currently says — not the file it was loaded from — and mark_saved clears the dirty flag for edits made during the async write; all deck writes are non-atomic** | Found by the 2026-08-28 whole-project audit (R34); confirmed by adversarial verification. spawn_save's own comment says 'write it back over the loaded path' but the code clones the LIVE deck_path; EditorState records no loaded-path field to bind to, and DeckPathChanged does not clear or reload the editor document. The path field is global chrome editable on the Editor tab itself, so load A / retype B / Save truncates B with A's text. Default config, ordinary click sequence. mark_saved race is real but sub-millisecond and encoded by an existing test. Non-atomic writes confirmed at three sites. CORRECTION: project_cmd.rs already reads before writing with an explicit comment, so in-place convert cannot lose data to a render failure. Detail: `docs/dev/reviews/review-260828.md`. **Fixed in #452, and the row understated it by one defect.** Re-measured 2026-09-08 through `AppState::apply`: load A, retype the path to B, and the save target is B (the recorded defect) — but also, **Save-as to C left the document bound to its previous file, so the next plain Save went back there**, which no editor does and which this row does not mention. Both fall out of one fix: `EditorState::file_path` is the file the document belongs to, set by an accepted load and by `DeckSaved(Ok(path))` — the latter covering Save and Save-as alike — and `AppState::save_target()` names the decision so a test can reach it, which it could not before: the defect lived in the binary's `spawn_save` under a comment claiming it wrote "back over the loaded path". The GUI now shows the binding on an *Editing:* line, because a correct but invisible save target is the next row. **The design review predicted a hole that does not exist** — that a save completing after a load would rebind the new document to the old file — and sabotage disproved it: an accepted load ends in `refresh_editor_preview()`, which retires both run ids (FND-133/#445), so the explicit clear I had added changed nothing and was removed. **Not closed by this row:** the non-atomic `std::fs::write` at both save sites, which is FND-152. | | FND-102 | 2026-08-28 | fixed | **[medium] No panic containment in the worker plus retry-on-every-survivor means one poison task (deterministic crash) drains the entire pool** | Found by the 2026-08-28 whole-project audit (R32); confirmed by adversarial verification. No catch_unwind anywhere in crates/apps/bindings (rc=1), so a panic unwinds out of main with no result line; the design doc DOES define an `internal` code as 'Catch-all for unexpected panics', so the divergence is real. ssh_worker treats EOF as a dropped connection, reconnects and RESENDS THE SAME task; pool.rs then loops it over every remaining worker, ending 'all workers in pool failed'; main.rs:938 turns that into a whole-sweep FAILURE with no partial output. resource_exhausted exists only in a serde round-trip test and the enum decl -- no production path emits it. IMPORTANT NEGATIVE RESULT: the verifier drove 8 hostile decks (zero-length wire, zero/negative radius, zero segments, 1e300 coords, radius>>length, coincident wires) and ALL 8 returned clean result lines -- the solver's validation held. So no concrete poison deck is demonstrated today; only the OOM/abort class remains as... Detail: `docs/dev/reviews/review-260828.md`. — fixed in PR #439, re-scoped rather than downgraded. I had proposed downgrading it as unreachable, having capped the OOM vectors; the review showed the live vector is no longer a hostile deck but a LEGITIMATE one -- MAX_SEGMENTS permits a ~1.6 GB dense matrix, which OOM-kills a small remote host, repeats on resend, and catch_unwind cannot touch. Two parts. The amplifier: DispatchError splits on WHEN the worker died, so a task that two workers died HOLDING is blamed by name and the pool is kept, while a worker that was never reachable does not count against the task and it may keep looking for a live one. The budget is carried across both the batch and sequential loops, which previously reset it. The worker half: an unwinding panic is caught and emitted as `internal`, so it takes the hard-failure path instead of closing the channel. I HAD THIS BACKWARDS -- I thought catching it was cosmetic because a panic already surfaces as EOF, and EOF is exactly the problem, since it is what triggers reconnect-and-resend. Each half sabotage-verified separately, each failing exactly one test. Residual stated: a uniformly poison sweep still costs two workers per point. Coverage stated: not OOM, not abort, not a hung GPU device -- those are FND-101's deadline. | | FND-101 | 2026-08-28 | fixed | **[medium] SSH/local worker dispatch has no timeout of any kind: one worker that accepts a task and never writes a newline hangs read_line forever, and dispatch_batch's thread::scope then blocks the whole sweep** | Found by the 2026-08-28 whole-project audit (R31); confirmed by adversarial verification. Zero hits for sleep/Instant/timeout across ssh_worker/pool/controller; the only Duration in the crate is capability.rs's cache TTL. ConnectTimeout=5 covers connection setup only; no ServerAliveInterval. shutdown() calls unbounded child.wait(). Design doc specifies 'up to 2 seconds for graceful exit' and 'reconnect after a 5-second back-off' -- both prose-only. AMPLIFICATION CONFIRMED: thread::scope joins every thread before returning, so one wedged read_line withholds the whole batch including other workers' completed results. CORRECTION: the 'silently dropped TCP path' sub-claim is overstated -- kernel retransmission timeout eventually errors the socket (~15 min); only the wedged-remote-process case hangs forever. Downgrade: needs opt-in --hosts plus a half-dead peer; no wrong numbers. Detail: `docs/dev/reviews/review-260828.md`. — fixed in PR #439: three deadlines, each with its own reason. Solve 15 min, matched to the kernel TCP retransmission bound so the guarantee is 'never worse than a dead socket, bounded where it was infinite', and more than five times the slowest legitimate point on this hardware. Probe 30 s, because connect_all probes hosts SERIALLY at startup before any pool exists. Shutdown 2 s then kill, the figure the design doc had promised in prose while calling an unbounded wait. ONE reader thread per WORKER, not per dispatch: my own first proposal was per dispatch and would have leaked one per wedged worker; this one exits by itself, because a timeout evicts the handle, Drop kills the child, and that closes the pipe. RESIDUAL, recorded not hidden: only the READ is bounded. A write can still block if the child stops draining stdin and the task line exceeds the pipe buffer; covering it means moving the write into the reader thread. Noted at the seam. Gated on a stub that reads its task and never answers -- not a crash and not a disconnect, the only case that hung forever. Because sabotage would HANG rather than fail, the batch runs on a spawned thread and the test waits on its own bound, so removing the deadline fails in 30 s with a message naming the cause. | | FND-100 | 2026-08-28 | fixed | **[medium] extremes_mhz judges the untruncated sweep while frequencies_mhz truncates: one run warns 'only the first 100000 are solved' then refuses for point 400001 that would never run** | Found by the 2026-08-28 whole-project audit (R26); confirmed by adversarial verification. extremes_mhz indexes the card's FULL requested length while frequencies_mhz caps at MAX_FR_POINTS. Reproduced: FR 0 400001 0 0 30.0 -0.0001 warns 'only the first 100000 are solved' then refuses because the sweep 'reaches -10 MHz' -- a point it just said it would not solve. The 100000 solved points are all positive. Also reproduced the overflow variant. The doc comment claiming validator and expander 'cannot come to different answers' is false exactly when is_truncated(), and the existing test only exercises non-truncated cards. Detail: `docs/dev/reviews/review-260828.md`. — fixed in 0460377 (PR #432, squashed to 4e7170f) (shared frequency-usability gate; sabotage-verified). | diff --git a/docs/project/test-catalog.md b/docs/project/test-catalog.md index cba14b3..5bf1cfa 100644 --- a/docs/project/test-catalog.md +++ b/docs/project/test-catalog.md @@ -43,7 +43,7 @@ counts (measured, not estimated). Aggregate pass/fail is recorded separately in | `apps/nec-cli/tests/topology_fallback.rs` | 13 | Non-single-chain fallback across solver/pulse/exec/sinusoidal/loaded | DEC-010/011 | | `apps/nec-cli/tests/worker_gpu_exec.rs` | 1 | Distributed GPU dispatch through worker pool (mixed gpu/cpu) | PH7-CHK-004 | | `apps/nec-cli/tests/worker_integration.rs` | 7 | Hosts config, capability cache, subprocess round-trip | PH6-CHK-006/007 | -| `apps/nec-gui/tests/gui_smoke.rs` | 47 | Headless GUI state machine + solve pipeline | PRT-004, PH3-CHK-009/010/011 | +| `apps/nec-gui/tests/gui_smoke.rs` | 127 | Headless GUI state machine + solve pipeline; run-identity guards; editor save binding (FND-103) | PRT-004, PH3-CHK-009/010/011 | | `crates/nec_accel/tests/gpu_hallen_solve.rs` | 1 | Gate G7: GPU Z-fill + CPU Hallén solve end-to-end | PH5-CHK-007 | | `crates/nec_accel/tests/gpu_microbench.rs` | 1 | Microbench separates per-dispatch time from device init | PH7-CHK-002 | | `crates/nec_accel/tests/gpu_resident_solve.rs` | 1 | Fully GPU-resident Hallén fill+solve parity | PH7-CHK-003 | @@ -57,7 +57,7 @@ counts (measured, not estimated). Aggregate pass/fail is recorded separately in | `apps/nec-cli/tests/current_source_junction.rs` | 1 | CLI junctioned current source: split-dipole EX-4 feedpoint Z=V/i0 matches voltage-source Z (~2e-4) | PH9-CHK-002 | | `crates/nec_worker/tests/gpu_exec.rs` | 2 | Worker-level GPU execution vs CPU parity | PH7-CHK-004 | -Integration subtotal: **522** test +Integration subtotal: **526** test functions across the `tests/` binaries listed above. ## Unit tests (in `src/`) @@ -82,8 +82,8 @@ Unit subtotal: **567** `#[test]` functions. ## Totals -- **Test functions**: **1096** = 567 unit + 522 integration + **7 doctests**. -- **`cargo test --workspace` aggregate**: **1094 passing, 0 failed, 2 ignored**, +- **Test functions**: **1100** = 567 unit + 526 integration + **7 doctests**. +- **`cargo test --workspace` aggregate**: **1098 passing, 0 failed, 2 ignored**, measured 2026-09-07 — the authoritative pass count in [test-results.md](test-results.md). Doctests are counted separately on purpose. `cargo test --workspace -- --list`