Skip to content

Audit fixes for R6.6/R6.7/R6.8: 4 real bugs + 3 tests that could not fail#137

Merged
dovvnloading merged 1 commit into
mainfrom
qt-removal/r6-audit-fixes
Jul 26, 2026
Merged

Audit fixes for R6.6/R6.7/R6.8: 4 real bugs + 3 tests that could not fail#137
dovvnloading merged 1 commit into
mainfrom
qt-removal/r6-audit-fixes

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

A 6-dimension adversarial audit of the last four shipped commits (autosave, crash recovery, PNG export, npm audit fix) produced 24 findings. Each was handed to an independent skeptic told to refute it; 14 survived. This fixes every survivor that was a real defect.

Every fix is mutation-tested: a harness reverts each one and confirms the test claiming to cover it actually fails. 14/14 mutants killed.

Export PNG (R6.8)

The exported image silently depended on the viewer's live zoom. Node views derive collapse from React Flow's live store zoom (lodCollapsed = zoom < 0.5), so a user zoomed out to see their whole graph — the natural thing to do right before exporting it — got a PNG of title bars with no content. Overriding the clone's CSS transform can't fix that: the clone comes from DOM React already rendered collapsed. Now sets the live viewport to the export framing, waits a double-rAF for the repaint, captures, and restores the user's viewport in a finally. Cost is one frame of visible movement.

No error handling at all. toPng genuinely rejects when a node's image asset is unreachable (a state ImageNodeView already renders a placeholder for), and both call sites void the promise — so the button silently did nothing, with no way to tell a failed export from a slow one. Now caught and logged, matching ImageNodeView's own handleExportImage.

Output wasn't the documented size — 1920×devicePixelRatio (2880×1620 on a 150%-scaled Windows display). pixelRatio pinned to 1.

Autosave (R6.6)

Deleting the currently-open chat silently ended autosave protection. The change-guard compared content only, and deleting a row leaves the document's digest unchanged — so every later tick short-circuited and the recovery path was unreachable. The guard now compares chat_id too, and deleteChat clears the dangling current_chat_id.

The docstring described a mechanism that didn't exist. It claimed the guard covered "auto OR manual" via "register_autosave's own last_hash argument" — there was no such argument, and no manual path could reach the cell. Consequence: every manual Save and every loadChat was followed by a byte-identical rewrite that bumped updated_at and re-sorted the Chat Library. The cell is now genuinely shared — owned by register_chat_library, seeded by saveChat/loadChat, invalidated by deleteChat/newChat, and passed to register_autosave as a real argument.

Tick loop had no exception guard. Defence in depth, not a known bug — the audit walked every unguarded line and found no reachable escape today. But the task is never awaited and holds a strong reference for the process's life, so an escape would kill autosave with no signal anywhere.

Crash recovery (R6.7)

  • configure_logging replaced basicConfig without a StreamHandler, so python graphlink_desktop.py with a missing SPA build printed nothing and exited 1. stderr is back alongside the file handler.
  • mark_clean_exit unlinked unconditionally, so two concurrent instances corrupted each other's sentinel and a later crash went unreported. Only the pid that wrote it may remove it. The other half — a false crash notice while a sibling is live — is documented as knowingly unfixed: it needs a pid-liveness check with no safe portable form (os.kill signal-0 is POSIX-only; on Windows it would terminate the other process).
  • install_exception_handlers latched its idempotency flag before doing the work, so one failure disabled the hooks permanently with no retry. Set last now, matching configure_logging.

Three tests that looked like coverage but weren't

All verified by mutation:

  • test_autosave_tick_never_regenerates_an_existing_chats_title changed only a code node between ticks, which _resolve_seed_message ignores — so the regenerated title was byte-identical and the test passed whether or not the implementation preserved it.
  • No autosave test observed the bus: all three await bus.publish(...) calls could be replaced with pass and the file stayed green.
  • commands.test.ts asserted expect(() => run()).not.toThrow() on a voided async call — which holds for every implementation, including run: () => {}.

Verification

2370 pytest (+12), 1053 vitest (+4), tsc/eslint/build/codegen clean, Qt burn-down gate unchanged at 152.

Also live-driven against a scratch copy of a real ~/.graphlink/chats.db: 5 real legacy chats (up to 15 nodes) load and then tick without a rewrite, confirming the seeded digest survives a real load/save round trip — the one thing the unit tests couldn't prove. Real chats.db verified untouched.

Knowingly not fixed

An autosave tick holds the same mutation_guard user intents use, so a manual Save landing in that window is dropped with an "already in progress" warning. Verified real but measured at ~10-50ms once per 30s, with no data loss (the tick writes the same document). Fixing it properly means an awaited handoff rather than a reject — out of scope here.

…fail

A 6-dimension adversarial audit of the last four shipped commits produced 24
findings; each was handed to an independent skeptic told to refute it, and 14
survived. This fixes every survivor that was a real defect. Every fix is
mutation-tested: the mutation harness reverts each one and confirms the test
that claims to cover it actually fails (14/14 mutants killed).

Export PNG (R6.8):
- The exported image silently depended on the viewer's live zoom. Node views
  derive collapse from React Flow's live store zoom (lodCollapsed = zoom <
  0.5), so a user zoomed out to see their whole graph - the natural thing to
  do before exporting it - got a PNG of title bars with no content. Overriding
  the clone's CSS transform cannot fix that; the clone comes from DOM React
  already rendered collapsed. Now sets the live viewport to the export
  framing, waits a double-rAF for the repaint, captures, and restores the
  user's viewport in a finally.
- No error handling at all: toPng genuinely rejects when a node's image asset
  is unreachable (a state ImageNodeView already renders a placeholder for),
  and both call sites `void` the promise, so the button silently did nothing.
  Now caught and logged, matching ImageNodeView's own handleExportImage.
- Output was 1920*devicePixelRatio, not the documented fixed 1920x1080
  (2880x1620 on a 150%-scaled Windows display). pixelRatio is pinned to 1.

Autosave (R6.6):
- Deleting the currently-open chat silently ended autosave protection. The
  change-guard compared content only, and deleting a row leaves the document's
  digest unchanged, so every later tick short-circuited and the recovery path
  was unreachable. The guard now compares chat_id too, and deleteChat clears
  the dangling current_chat_id.
- autosave.py's docstring claimed the guard covered "auto OR manual" via
  "register_autosave's own last_hash argument" - there was no such argument
  and no manual path could reach the cell, so every manual Save and every
  loadChat was followed by a byte-identical rewrite that bumped updated_at and
  re-sorted the Chat Library. The cell is now genuinely shared: owned by
  register_chat_library, seeded by saveChat and loadChat, invalidated by
  deleteChat/newChat, and passed to register_autosave as a real argument.
- The tick loop had no exception guard. An audit walked every unguarded line
  and found no reachable escape today, so this is defence in depth, not a
  known bug - but the task is never awaited and holds a strong reference for
  the process's life, so an escape would kill autosave with no signal at all.

Crash recovery (R6.7):
- configure_logging replaced graphlink_desktop.py's basicConfig without a
  StreamHandler, so `python graphlink_desktop.py` with a missing SPA build
  printed nothing and exited 1. stderr is back alongside the file handler.
- mark_clean_exit unlinked unconditionally, so two concurrent instances
  corrupted each other's sentinel and a later crash went unreported. Only the
  pid that wrote the sentinel may remove it. The other half - a false crash
  notice when a sibling is live - is documented as knowingly unfixed; it needs
  a pid-liveness check with no safe portable form (os.kill signal-0 is
  POSIX-only; on Windows it would terminate the other instance).
- install_exception_handlers latched its idempotency flag before doing the
  work, so one failure disabled the hooks permanently with no retry. Set last,
  matching configure_logging.

Tests that looked like coverage but were not (all three verified by mutation):
- test_autosave_tick_never_regenerates_an_existing_chats_title changed only a
  code node between ticks, which _resolve_seed_message ignores, so the
  regenerated title was byte-identical and the test passed either way.
- No autosave test observed the bus: all three `await bus.publish(...)` calls
  could be replaced with `pass` and the file stayed green.
- commands.test.ts asserted `expect(() => run()).not.toThrow()` on a `void`ed
  async call, which holds for every implementation including `run: () => {}`.

Suites: 2370 pytest (+12), 1053 vitest (+4), tsc/eslint/build/codegen clean,
Qt burn-down gate unchanged at 152. Also live-driven against a scratch copy of
a real ~/.graphlink/chats.db: 5 real legacy chats (up to 15 nodes) load and
then tick without a rewrite, confirming the seeded digest survives a real
load/save round trip - the one thing the unit tests could not prove. The real
chats.db was verified untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dovvnloading
dovvnloading merged commit ff80dff into main Jul 26, 2026
2 checks passed
@dovvnloading
dovvnloading deleted the qt-removal/r6-audit-fixes branch July 26, 2026 01:22
dovvnloading added a commit that referenced this pull request Jul 26, 2026
…ship-aware (#138)

The last remaining confirmed finding from the R6.6/R6.7/R6.8 audit, deferred
out of #137 rather than bolted onto it.

chat_library.py's _mutation_in_progress flag serializes loadChat/saveChat/
newChat by DROPPING a blocked intent and warning "Another chat operation is
already in progress." That contract is honest when only user-initiated intents
can hold the flag - the user really did start two things. R6.6 then had a
background autosave task claim the same flag, and the asymmetry went unnoticed:
a tick that happened to be mid-write made the user's own Save/Load/New Chat
vanish, with a warning naming an operation they never started. A background
convenience feature must never beat the user to their own data.

The guard now records WHO holds it, and the two directions differ:
  user arrives, autosave holds     -> wait the tick out, then proceed
  user arrives, another user holds -> drop + warn, exactly as before
  autosave arrives, anyone holds   -> skip this interval, exactly as before

The wait is bounded (AUTOSAVE_YIELD_TIMEOUT_SECONDS = 2.0, ~40x the measured
10-50ms tick). A tick genuinely stuck on sqlite's own 30s lock timeout
degrades to the pre-fix drop rather than freezing the UI - but with a message
that names autosave instead of "another chat operation", which is what made
the original warning read as a bug. Every outcome is at least as good as
before; the common one is strictly better, in that the Save just works.

The guard's shape now lives in one factory, _new_mutation_guard(), so
chat_library, autosave and the tests cannot drift apart on it. Its release
Event is constructed with no running loop on purpose - safe since 3.10, and
register_chat_library really is called outside a loop, which is the exact
hazard that broke R6.6's own create_task call.

Mutation-tested, and the first attempt failed the bar. The initial test used a
hand-written double that reimplemented the claim/release itself, so mutating
the REAL _guarded_tick - dropping its owner tag or its release signal - left
the test green; the production wiring was never under test. Rewritten to drive
the real register_autosave loop with a deliberately slow save, bounded by
asyncio.wait_for so a lost wakeup fails instead of merely running slow. That
exposed a second gap: the original test only ever observed the FIRST tick to
claim, so removing _guarded_tick's released.clear() also survived - yet without
it, every tick after the first wakes a waiter on a STALE signal and drops the
user's intent exactly as before. A fix that works once then quietly stops is
worse than no fix, so there is now a test pinning the steady state too. All 5
mutants killed, including both that initially survived.

Suites: 2375 pytest (+5), 1053 vitest unchanged (no frontend touched),
tsc/eslint/build clean, Qt burn-down gate unchanged at 152. The chat-library
and autosave files were also re-driven against a scratch copy of a real
~/.graphlink/chats.db - 5 real legacy chats still load and tick without a
spurious rewrite through the rewritten guard. Real chats.db verified untouched.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant