fix: implement the 2026-09-06 audit (P1+P2), and the root cause behind its flaky gates - #1370
Merged
Conversation
Audit 20260906, findings F1-F3 and B1-B3. Each carries a regression test verified to fail against the previous code. F1 - Batch Save All silently replaced existing files. The loop built `folder/title.md` and handed it to the ordinary overwrite writer, so choosing a folder that already held `Untitled-1.md` replaced a document the user need not even have open, and two same-titled tabs collided with each other. Destinations are now reserved up front with a new `create_file_exclusive` command: one kernel-serialized O_EXCL create, so the claim and the test are one operation and a concurrent creator cannot win the gap. F2 - Restart continued after the recovery snapshot failed to persist. The user agreed to "restart and restore my unsaved documents"; without a snapshot the second half is not on offer. The failure is propagated and the process stays alive. Tauri's `prevent_exit` ignores RESTART_EXIT_CODE, so the coordinated quit handler could not have rescued the buffers either. F3 - A late autosave reverted a completed Save As. Saves are serialized per PATH, so a write to old.md and a Save As to new.md are different queues by design; the post-save step then re-pointed the document unconditionally, so whichever finished last named it. Identity changes are now ordered per DOCUMENT, claimed at submission so the user's most recent choice wins whatever order the writes complete in. B1 - A failed replacement could delete the original on Windows. The remove-then-retry fallback fired on ANY persist error, including one caused by the source temp file being held open, so the deletion succeeded while both renames failed. Its premise was false: NamedTempFile::persist already passes overwrite:true, so MoveFileExW carries MOVEFILE_REPLACE_EXISTING and an existing target never needed removing. B2 - Saving through a symlink replaced the link. A rename onto an alias makes it a regular file and leaves the real document untouched, while the save reports success. The document-save boundary resolves the referent first; internal writes deliberately do not, so a planted link cannot redirect them. B3 - A successful save discarded Finder tags. The replacement carried permission bits across the new inode but not extended attributes, so a tagged note fell out of the user's tag-based organization on an ordinary edit.
…iscard Audit 20260906, findings B4-B7. B5 - Duplicate tab IDs erased a distinct unsaved document. Step 1 of the repair was `retain(|tab| seen_ids.insert(tab.id))`: every later tab sharing an ID was dropped without looking at its content or dirty flag, while the duplicate-PATH rule directly below carefully preserved anything carrying work. A corrupted or migration-produced session holding two different unsaved buffers under one ID lost the second before the frontend saw it, and a successful restore then deleted the session file. The safe-duplicate rule now applies to IDs too; what differs is the remedy, since the frontend keys tabs by ID, so a survivor is given a fresh identity rather than discarded. B6 - A strict whole-session parse prevented the per-item salvage the frontend already implements. One `null` in the tab array, or a cosmetic `sidebar_width: 260.5` against a u32, rejected every healthy unsaved document beside it and returned the same `None` as "there is no session". Salvage now runs when the strict parse fails: inspect as raw JSON, drop or repair individual items, then build the typed structure. The typed schema stays strict, so an ordinary read keeps every guarantee it had. Both halves report provenance. A lossy repair of the MAIN file used to be indistinguishable from ordinary main data, so a successful restore deleted the evidence; `lossy_repair` now travels with the payload the same way `recovered_from_backup` does. B4 - Restoring a session whose original main window was closed duplicated its first surviving window. The main source and the secondaries were two independent queries - `find(is_main_window)` with a `first()` fallback, and a separate `filter(!is_main_window)` - which overlapped whenever no window carried the flag. Closing `main` and carrying on in a `doc-*` window produces exactly that, so N saved windows came back as N+1. The partition is now one choice by index, extracted as a pure function so the invariant is testable without a native window builder. B7 - An extreme persisted timestamp panicked in debug. `now - i64::MIN` overflows; the neighbouring max-age multiplication was already guarded.
Audit 20260906, findings F4-F5, plus two the restored fuzz oracle exposed. F4 - Checkpoint redo survived a new edit, so Redo overwrote the new branch. The native editor drops its redo stack on any edit; the CHECKPOINT stack could not, because an ordinary keystroke never reaches the history store. After undoing to a checkpoint and typing, native redo depth was 0 while unified redo still held the pre-undo text and replaced what was just written. A redo entry now records the branch point the undo left the document on, and a redo from an abandoned branch is discarded rather than applied. Detecting staleness beats classifying edit origins: native undo/redo reach the same setter as typing, so an origin rule can be fooled and a content check cannot. F5 - Enter threw when the selection spanned two empty paragraphs. `TransformError: Inserted content deeper than insertion position`. Tiptap computes canSplit against the pre-deletion document, deletes the selection, then splits at the mapped position without recomputing. Present in 3.31.3 and reproducible on vanilla StarterKit, so VMark carries a narrow override that delegates the cross-block case to ProseMirror's own splitBlock, which does the same work in the correct order. Strikethrough delimiters were emitted where GFM cannot parse them back. `plain~~* word~~tail` reparses as literal text: the opening ~~ is followed by punctuation while preceded by an alphanumeric, which the flanking rules read as a closer position. The mark was lost and four tildes the user never typed entered their document. This was already a known open defect, pinned `it.fails` from fuzz seed 42 with the diagnosis that "delimiter policy must consider flanking context, not only edge whitespace", and the note that a fix should flip it RED-to-GREEN. It does. The remedy is remark's own for emphasis: character-reference the offending neighbour, so the run flanks and the text decodes back to exactly what it was. That encoding then split astral characters across their surrogate pair. mdast-util-to-markdown encodes by UTF-16 code unit, so an emoji beside such a delimiter came out as `�` plus a raw low surrogate and reparsed as U+FFFD. Upstream, and older than the delete handler - plain `**bold**` reproduces it with no strikethrough anywhere. The pair is re-encoded as one code point, in both directions, unconditionally and outside the size-capped cosmetic pass. The fuzz oracle now states its normalization contract. A mark cannot begin or end with whitespace in markdown, so the serializer must move it out; comparing documents without that normalization reported a correct serializer as broken. Its own cursor was also placing carets between surrogate halves, manufacturing documents no editor can produce. Also upgrades the Tiptap family 3.30.0 -> 3.31.3 for GHSA-cp6q-959q-f8rh. It does not fix F5, which was verified against 3.31.3 directly.
Audit 20260906, findings C1-C9. `scripts/check-ci-docs-filter.test.mjs` now EXECUTES the aggregate shell blocks across the whole matrix of job results rather than grepping them for a substring - the previous assertion checked spelling, and the behaviour it stood in for was wrong. C1 - Both required aggregates passed when change detection failed. The `changes` job was not in `frontend`'s needs at all, and `skipped` was accepted unconditionally - so a failed filter skipped every app tier, each skip read as a pass, and the check went green having tested nothing. Executing the old block with STATIC=success and the rest `skipped` returns 0. A skip is now a pass only when the filter SUCCEEDED and explicitly said the tier does not apply. C2 - The release smoke test had never run once. GitHub does not start workflows from events created with GITHUB_TOKEN, so the `release: published` trigger never fired for a token-published release; the API reported total_count 0 over its entire history. A gate that never runs looks exactly like one that always passes. It is dispatched explicitly now, and a new gate test asserts the producer-to-consumer wiring. C3 - Dependency advisories could not block a merge. npm audit was continue-on-error at --audit-level=critical, for a pnpm-410 problem that no longer reproduces, and cargo audit reported into no required check. Now blocking against a reviewed allowlist that ratchets two ways: an unlisted moderate-or-worse advisory fails, and an acceptance fails once its advisory is gone. All 17 current entries are dev/tooling chains with the reason each is unreachable in the shipped app. C4 - A manual release built whatever the default branch held and published it under the requested version, because checkout never resolved the tag and the no-republish guard was skipped for dispatch. The tag is resolved and checked out, the tree's five version files must agree with it, and the guard applies to both triggers. C5 - Audit auto-merge trusted any PR comment. The verdict was the last matching line across all comments, so anyone could post the approval marker and override a failed verifier, and an old approval survived a new push. It is now bound to the verifier's identity, this run's window, and the head SHA that was actually verified. C6 - Homebrew publication could go backwards. No concurrency group, and the downgrade guard ran before up to 15 minutes of waiting and downloads that then refreshed the tap - so a slower run could overwrite a newer cask. Publication is serialized and the version re-checked immediately before mutation. Downloads also used bare `curl -L`, which writes an error body and exits 0, so a 404 could be hashed and published as the checksum. C7 - The website was built and linted only during deployment, i.e. after merge. Added as a path-filtered required PR tier. C8 - The weekly soak had failed all four recorded runs and stopped in the first step, so the pathological-input and downloaded-corpus tiers had never executed. The three suites are independent now, failures report into a rolling issue, and the workflow declares itself to the liveness checker - which had been reporting every opted-in gate healthy while this one produced no verdict at all. C9 - Fresh-clone instructions required Node 20 against engines >=22 and omitted the sidecar build; AGENTS.md linked to an architecture file that has never existed.
…media Audit 20260906, findings MCP-C01 through C05 and M01. MCP-C01 - A failed Slidev startup crashed the whole knowledge-base server. Startup bookkeeping lived on a promise DERIVED from the startup one (`pending.then(...).finally(...)`). `start()` awaited the original, so the route returned 500 correctly - but the derived chain rejected with nobody observing it, and Node terminates the process on an unhandled rejection. One malformed deck took down the KB and every preview sharing it. The lifecycle is one awaited promise now, so there is no second chain to go unobserved; retry and same-deck coalescing are unchanged. MCP-C04 - Shutdown did not own in-flight starts. `stopAll` snapshotted only completed servers, so a delayed startup inserted itself afterwards: the manager reported 0 servers, then 1 once the pending start resolved, leaving an unproxied server running past the shutdown meant to end it. Both cancellers signal through `starting` and wait, so a late server is closed rather than registered. MCP-C02 - Ordinary markdown links lost their session. The token propagator selected `a[href^="/"]`, which matches no relative link - so [Next](B.md) and [Parent](../B.md) navigated without ?s and returned 401 inside the cookie-blocked in-app iframe. Only wiki-links, which emit absolute /note/ URLs, had ever worked. Every link is considered now, and resolved against the DOCUMENT rather than the origin, or "B.md" on /note/dir/A.md would resolve to /B.md. The shipped script is executed against a stub DOM in test rather than grepped. MCP-C03 - Local images were not served at all. `` resolves under /note/, which serves only paths the walker indexed, and the walker indexes markdown - so the image 404'd with a valid session. Relaxing that index gate would have been the wrong fix: it is what keeps hidden and ignored files unreachable. Local media gets its own route with its own narrow policy - containment, a media extension allowlist, no hidden segments, files only - and image URLs are rewritten server-side, because the browser starts fetching while the HTML is still parsing and a client-side rewrite always loses that race. MCP-C05 - Opening a second workspace logged the first one out. Cookies are scoped by host, never by port, so every workspace server on 127.0.0.1 shared one cookie name while minting incompatible tokens. The name is derived from the workspace root: distinct per workspace, and stable across restarts, so a relaunch replaces its cookie instead of leaving another in the jar. MCP-M01 - A reconnect flush stripped the deadline from requests it had not sent. The queue was detached before the first send while the timeout callback looked for its entry IN that queue, so every entry lost its deadline the instant a flush began - though the sends are serial and only the first was ever in flight. Ownership transfers one entry at a time now. Library-only: queueWhileDisconnected defaults false and the bundled CLI does not set it. Also fixes a pre-existing type-narrowing error in runtime.test.ts.
…exist
Every wall-clock bound in this repo has needed raising, repeatedly: the app
tier 5s->20s, the gate tier 5s->20s->60s, the pathological ceiling
60s->180s. Each was fixed by measuring healthy runs on a quiet box and
adding headroom, which is what turns a liveness bound into a performance
assertion - it then fails on any machine slower than the one it was
calibrated on, and the remedy is to measure again and nudge.
The cause is one level down. `maxWorkers()` was `availableParallelism() *
1.6`, and `availableParallelism()` reports how many cores EXIST, never how
many are idle. On a dedicated runner those agree and the measured ratio is
right. On a developer machine they do not: this box sat at load average 41
across 31 sessions - other projects' language servers, browsers, a second
agent - while the pool still sized itself for a quiet 10-core machine and
started 16 workers on top.
Measured on one commit, changing only the worker count:
16 workers -> 3 test files failed, a DIFFERENT three on each run
(agentSnapshot/ariaParity once, WorkflowEngineSlot/
CopyButton the next - whichever landed on a starved worker)
3 workers -> 1644 files, 38074 tests, 0 failures, every bound untouched,
and 1-minute load fell from 41 to 14
The changing failure set was the tell: not broken tests, starved workers.
So the pool is sized from cores MINUS the load already on them. The clamp
is one-sided - it can only shrink the pool, and only when the machine is
busy - so a quiet runner (load ~ 0) gets precisely the previous behaviour
and CI is unaffected by construction. `loadavg()` returns 0 where it is
not implemented, which yields the unchanged ratio rather than a wrong
guess. The tradeoff is honest: on a busy machine this is slower (884s vs
265s) and correct, instead of faster and wrong.
The timeouts become one shared LIVENESS_TIMEOUT_MS rather than five copies
that drifted, set from what is unambiguously a hang instead of from how
long healthy work takes. Also removes an `expect(Date.now() - started)
.toBeLessThan(5_000)` that was redundant with an exact 40,002-read bound
two lines above it, and was the only non-deterministic thing in that test.
Assertions, in the APP tier so they still report when the gate tier is the
broken thing: the pool never exceeds the idle ceiling, never drops below
MIN_WORKERS, and vitest.shared.ts must still read loadavg; every tier uses
the shared timeout and the server configs carry no numeric literal. Both
were verified by reverting and watching them fail - the worker regression
is silent, since core-count sizing only misbehaves on a busy machine and
the symptom appears in unrelated tests.
All five version sources plus the derived src-tauri/Cargo.lock, together, so the About dialog and the MCP health check cannot disagree and `cargo build --locked` does not fail on a stale lockfile. Folded into this branch rather than a standalone bump PR: the changes being released are still in flight here, and a separate PR would pay a full CI cycle to change five strings.
Splitting `storage.test.rs` moved every test into `read_session.test.rs` and left behind a file containing only imports and a helper. `cargo test` compiles that happily; `clippy -D warnings` does not, and CI's rust-test job failed on three unused-item errors. Found by CI rather than locally because clippy ran BEFORE the split and was not re-run after it — `cargo test` passing is not the same signal. The helper was already duplicated into `read_session.test.rs` during the split, so nothing is lost. storage.rs keeps no sibling test module and says why.
`use std::fs` in atomic_replace.rs became dead on Windows when the destructive remove-then-retry fallback was deleted (audit 20260906, B1): its only remaining user is `preserve_target_permissions`, which is #[cfg(unix)]. Under `-D warnings` a dead import is an error, and this is a platform local cargo never builds — CI's Windows leg found it. Gating the production import then broke the TEST file, which inherited `fs` through `use super::*` and reads files on every platform. It gets its own import; the same applies to link_target.test.rs, whose `atomic_replace` import serves only the #[cfg(unix)] symlink round-trip. Found the second half with `bash scripts/check-cross-target.sh`, which exists for exactly this and reproduces the Windows compile in about a minute. Running it after the first fix — rather than pushing and waiting for another CI round trip — is the point of having it.
Removing the destructive remove-then-retry (audit 20260906, B1) also removed an accident it was providing. `MoveFileExW` needs delete access to the file it replaces and returns ERROR_ACCESS_DENIED while any other handle holds it — an antivirus scanner mid-scan, a backup agent, or simply another thread reading the document. The old fallback got a second attempt that usually landed in the gap; without it, CI's Windows leg failed immediately on `app_paths::test_atomic_write_no_partial_content`, which races 200 writes against 200 reads of one file, with os error 5. The retry comes back on the one property that made the old one dangerous: this retries `persist` ITSELF, which is atomic and replaces in place. The target holds its previous bytes throughout, and if every attempt fails the file is exactly as it was. The old path removed the target first, so a subsequent failure left nothing at all — which is the data loss B1 was about. Bounded at 8 attempts over ~127ms, and only for a transient kind; any other error returns immediately with its original detail. Windows-only: rename(2) has no sharing concept, so retrying on Unix could only delay a real error. The B1 test still asserts the destructive path is gone — a locked SOURCE temp file cannot be retried past, and the target must survive. A second test now holds the TARGET open without FILE_SHARE_DELETE from another thread and releases it mid-flight, so the ride-out is proven rather than assumed. Verified with `bash scripts/check-cross-target.sh`, which compiles for x86_64-pc-windows-gnu in about a minute.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the 2026-09-06 repository audit through P2, plus the P3s, plus what fixing them uncovered. Every finding carries a regression test verified to fail against the previous code.
The audit's findings
mainwindow was closed duplicated a window · B5 duplicate tab IDs erased a distinct unsaved document · B6 a strict whole-session parse blocked the per-item salvage the frontend already implements · B7 an extreme timestamp panicked in debugWhat fixing them uncovered
A known open defect, closed. Restoring the fuzz oracle (C8) exposed that
~~delimiters were emitted where GFM cannot parse them back —plain~~* word~~tailreparses as literal text, losing the mark and putting four tildes the user never typed into their document. This was already pinnedit.failsfrom fuzz seed 42, with the diagnosis that "delimiter policy must consider flanking context, not only edge whitespace" and a note that a fix should flip it RED-to-GREEN. It does.The encoding that fixes it had its own bug:
mdast-util-to-markdowncharacter-references by UTF-16 code unit, so an emoji beside such a delimiter split across its surrogate pair and reparsed as U+FFFD. Upstream, and reproducible with plain**bold**and no strikethrough anywhere.Why every timeout in this repo keeps needing to be raised.
maxWorkers()wasavailableParallelism() * 1.6, andavailableParallelism()reports cores that exist, never cores that are idle. Proven by controlled experiment — same commit, same bounds, only the worker count changed:The changing failure set was the tell: not broken tests, starved workers. The pool is now sized from cores minus load, clamped one-sided so a quiet runner gets precisely the previous behaviour and CI is unaffected by construction.
Notes
unifiedHistory.test.ts6 → 0,coordinator.rs590 → 579,createServer.ts411 → 384, plus a stale entry pruned.strike("word ")unrepresentable. That is a deliberate policy decision, not a pure bug fix. It does not weaken text-loss detection — the normalization only moves characters across a mark boundary, never removes them.Verification
pnpm check:allgreen end to end: 67/67 gate files (1,268 tests), 1,644 app files (38,077 tests), MCP sidecar 654, content server 219, 26 static gates. Rust separately: 2,535 tests,clippy -D warnings,cargo fmt --check.Version bumped to 0.9.65 on this branch rather than in a standalone PR, per
40-version-bump.md— the changes being released are still in flight here.