perf(asv3): SessionLogWriter O(turn) append + Mirror facade (WP-5)#406
perf(asv3): SessionLogWriter O(turn) append + Mirror facade (WP-5)#406Luis85 wants to merge 8 commits into
Conversation
Replaces SessionLogWriter's O(turns²) rewrite-per-append pattern with an
O(turn) append gated behind a new VaultPort.appendFile port method, and
introduces a SessionLogMirror facade that becomes the public surface for
session-log writes outside src/application/chat/.
Port extension (ADR-008):
- VaultPort: add appendFile(path, content).
- ObsidianBridge: delegate to vault.adapter.append (creates the file on
first call when absent).
- MockBridge: in-memory concat append + narrow calls recorder
({writeFile, appendFile, readFile}) so I/O accounting can be asserted.
- LocalStorageBridge: read+concat+write (GH Pages demo only).
Writer hot path:
- Per-path LogPathCache holds the frontmatter + body in memory after
the first seeding read. Subsequent turns call appendFile for the new
block and writeFile only to rewrite the frontmatter window with the
bumped updated: timestamp.
- Eliminates the per-turn body re-read that drove the audit's 83.21%
statement-coverage gap and the O(N²) trigger.
Facade:
- SessionLogMirror exposes mirrorTurn (fire-and-forget) +
mirrorProposalDecision (await-required) + a createSessionLogMirror
factory. UI never imports SessionLogWriter directly; a new
no-restricted-imports rule enforces this outside src/application/chat/.
- ChatTurnOrchestrator, commitFileWriteProposal, useSessionLogMirror
composable (renamed from useSessionLogWriter), useProposalDecisions,
and ChatSidebar.vue migrate to the facade.
Tests:
- New: SessionLogMirror.test.ts (delegation).
- New: VaultPort.appendFile.test.ts (cross-adapter contract).
- New: SessionLogWriter "100 turns" DoD test asserting
appendFile.length === 100 and readFile.length === 0 on a fresh path,
plus a resumed-session test bounding reads to the seed.
- Latency and mutex tests in SessionLogWriter.test.ts updated to track
the appendFile-shaped hot path.
https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f0d2453b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const nextFrontmatter = buildFrontmatter(nextFields) | ||
| cache.fields = nextFields | ||
| cache.frontmatter = nextFrontmatter | ||
| await this.vault.writeFile(resolvedPath, `${nextFrontmatter}${cache.body}`) |
There was a problem hiding this comment.
appendBlock still issues writeFile(resolvedPath, ${nextFrontmatter}${cache.body}) on every turn, and cache.body contains the entire accumulated log. That means each append rewrites all prior turns, so cumulative write volume remains O(N²) for N turns (with an extra appendFile call on top), which defeats the intended hot-path performance fix for long sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd2b49d. You're right — the surface DoD passed (appendFile fired once per turn) but appendBlock was also calling writeFile(${frontmatter}${cache.body}) on every turn, with cache.body carrying every accumulated turn. Cumulative write volume stayed O(N²) and the "O(turn) closed" claim was misleading.
Architectural fix: body is append-only on disk; frontmatter updated: is debounced. appendBlock no longer calls writeFile after the seed — it does exactly one appendFile(blockOnWire) per turn and schedules a debounced frontmatter flush via scheduleFrontmatterFlush (default 30 s, configurable via flushDebounceMs). When the timer fires, flushFrontmatter reads the live on-disk body once, splices the new frontmatter, and writes the result. A new flushAll() cancels pending debounces and drains single-flight per path — production callers invoke it on plugin teardown; tests use it for deterministic assertions. Exposed on SessionLogMirror so callers outside src/application/chat/ don't need to import the writer directly.
LogPathCache.body is gone; the cache only tracks the latest FrontmatterFields and a bodyEndsWithNewline boolean. The DoD test now asserts the new contract on a fresh 100-turn probe: appendFile.length === 100, writeFile.length === 1 (seed only — no per-turn rewrite), readFile.length === 0 (no flush yet). After flushAll(): +1 readFile and +1 writeFile (the frontmatter rewrite splicing over the live body). Brief DoD and loop-state.md updated to reflect the corrected semantics rather than silently rewriting history.
Generated by Claude Code
| let cache = this.cacheByPath.get(resolvedPath) | ||
| if (cache === undefined) { | ||
| cache = await this.seedCache(resolvedPath, thread, at) | ||
| this.cacheByPath.set(resolvedPath, cache) |
There was a problem hiding this comment.
Once a path is cached, subsequent writes are generated from in-memory cache.body and flushed back wholesale. If the session log is modified on disk between turns (for example, manual edits in the vault), those changes are overwritten by stale cache content on the next append because this path does not re-merge current file contents before writeFile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved automatically by the P1 fix in bd2b49d. The root cause you flagged is the same as P1: the cached body was the source of truth on every rewrite, so out-of-band edits made on disk between turns were silently overwritten by the next appendBlock.
After the architectural fix, the body is never rewritten from in-memory state. appendBlock only calls appendFile(blockOnWire) per turn — that's a pure tail-append, and any prior on-disk content (including manual edits) survives untouched. The debounced frontmatter flush in flushFrontmatter reads the live on-disk body fresh, splices the new FrontmatterFields, and writes the result — so even the once-per-debounce-window writeFile preserves whatever the user did between turns.
Added a regression test for this: out-of-band body edits between turns survive the debounced flush (Codex P2) in tests/application/chat/SessionLogWriter.test.ts. It seeds turn 1, edits the on-disk body to insert a ## annotation block, runs turn 2 + flushAll(), and asserts the annotation is still present alongside the new turn-2 content and the advanced updated: timestamp. The LogPathCache.body field is gone entirely so there's no path left for stale cache state to clobber the body.
Generated by Claude Code
…ex P1+P2) Codex review on PR #406 flagged two findings that share a root cause: the WP-5 `appendBlock` issued `writeFile(${frontmatter}${cache.body})` on every turn so cumulative write volume stayed O(N²) (P1), and the in-memory body cache was rewritten back to disk wholesale on every flush so out-of-band edits between turns were silently overwritten (P2). The surface DoD passed ("appendFile is called per turn"); the intended perf win didn't land. Architectural fix: body is append-only on disk; frontmatter `updated:` is debounced. - Drop the body string from `LogPathCache` (replace with a `bodyEndsWithNewline` boolean used to compose the leading separator). - `appendBlock` no longer calls `writeFile` after the seed — it writes exactly one `appendFile` per turn. - New `scheduleFrontmatterFlush` arms a `setTimeout`-driven debounce (default 30 s, configurable via `flushDebounceMs`). When the timer fires, `flushFrontmatter` reads the *live* on-disk body, splices the latest `FrontmatterFields`, and writes the result. P2 is resolved automatically — the read picks up out-of-band edits. - New `flushAll()` cancels pending debounces and drains the flush single-flight per path. Production callers invoke it on plugin teardown; tests use it to assert post-flush state deterministically. Exposed on `SessionLogMirror` so UI/orchestrator callers don't need to import the writer directly. - Inject `setTimeout` / `clearTimeout` via constructor options so the application layer stays free of Obsidian runtime globals; the plugin layer can pass `activeWindow.*` for popout-window compatibility. DoD on a fresh 100-turn probe (Codex P1+P2 round-1): - `appendFile.length === 100` (one body delta per turn) - `writeFile.length === 1` (seed frontmatter only — no per-turn rewrite) - `readFile.length === 0` (no flush yet) - After `flushAll()`: `+1 readFile` + `+1 writeFile` (frontmatter rewrite spliced over the live body — P2 fix). Test updates: TEST-ASM-033 rewritten to call `flushAll()` before asserting on `updated:`; the O(turn) DoD test asserts the new counts; a P2-resolution test inserts an out-of-band annotation between turns and confirms it survives the next flush; two `ChatSidebar.proposalFlow` audit-row tests switch from `writeFile`-spy to `appendFile`-spy (the audit row now lands via `appendFile`). https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd2b49d420
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const existing = await this.vault.readFile(resolvedPath) | ||
| const split = splitFrontmatterAndBody(existing) | ||
| const body = split?.body ?? existing | ||
| const nextFrontmatter = buildFrontmatter(fields) | ||
| await this.vault.writeFile(resolvedPath, `${nextFrontmatter}${body}`) |
There was a problem hiding this comment.
Running doFlush from the debounced timer allows a readFile→writeFile cycle to race with appendBlock because this path does not use _runQueued's per-file mutex. If a new turn appends after readFile but before this writeFile, the stale body snapshot is written back and the just-appended block is lost. This can happen in normal usage when a message arrives around the debounce boundary, so the flush should be serialized through the same queue (or otherwise made race-safe).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — the round-1 debounced flush opened a readFile → writeFile race against appendBlock. If an appendFile for turn N+1 landed between the flush's body read and its frontmatter write, the stale snapshot was written back and the freshly-appended turn was lost. The trigger window is small but reachable in normal use (a message arriving on the debounce boundary), so this is a correctness bug, not a theoretical one.
Fix in 570b1c0: I extracted _enqueue(queueKey, op) from _runQueued and routed doFlush through the same primitive. PendingFlush now tracks the pre-suffix basePath as its queueKey, set on first schedule, so the flush enqueues onto the same per-file mutex chain appendBlock uses. Either order is now safe: the flush either reads after every queued append has landed and writes them all back, or yields the mutex so the pending append lands on top of the freshly-written frontmatter. Per-path only — different session paths still run in parallel; no global mutex. The single-flight inFlight coalescing stays on top so flushAll() remains idempotent.
Regression test added in tests/application/chat/SessionLogWriter.test.ts ("debounced frontmatter flush does not race a concurrent appendBlock"): it parks the flush on a VaultPort.readFile barrier mid-cycle, fires a concurrent appendUserAssistant, releases the barrier, drains, and asserts every turn survives on disk. Verified it FAILS on the round-1 head (bd2b49d loses u2/a2) and PASSES on the round-2 commit. Pre-PR gate green: npm audit 0 vulns, typecheck, lint (0 errors), npm test 1880/1880 (+1), build, build:web, docs:api.
Generated by Claude Code
…odex P1 round-2) The round-1 debounced `flushFrontmatter` bypassed the per-file mutex `appendBlock` uses, opening a `readFile → writeFile` race that could silently drop a turn block. If a new `appendFile` for turn N+1 landed between the flush's body read and its frontmatter write, the stale body snapshot was written back and the freshly-appended block was lost. Fix: extract `_enqueue(queueKey, op)` from `_runQueued` and route `doFlush` through it. Track the pre-suffix `basePath` on `PendingFlush` so the flush registers under the same queue key as the appends, sharing the same mutex chain. Per-path serialisation only — different session paths still flush in parallel; no global mutex. Regression test in `SessionLogWriter.test.ts` parks the flush on a `readFile` barrier mid-cycle, fires a concurrent `appendUserAssistant`, and asserts every turn survives on disk. Verified the test FAILS on `bd2b49d` (round-1 code) and PASSES on this commit. Refs: Codex thread 3254810244, line 799. https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 570b1c0335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Coalesce a concurrent flush request into the in-flight one. The | ||
| // in-flight flush will pick up the latest `pendingFields` before its | ||
| // own write, so we don't need a second pass. | ||
| await pending.inFlight | ||
| return |
There was a problem hiding this comment.
When flushFrontmatter is called while another flush is already inFlight, this branch just awaits the existing promise and returns. If a new append arrives during that in-flight flush, pending.pendingFields is updated and flushAll() may clear its timer, then hit this early return; the new frontmatter snapshot is left pending with no timer and never written. In plugin teardown (the documented flushAll() use case), this can silently drop the latest updated: timestamp despite the turn body being appended, so flushFrontmatter should re-check pendingFields after awaiting and perform another flush pass if needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid P1 — fixed in c272857.
flushFrontmatter now re-checks pending.pendingFields after await pending.inFlight. When a snapshot landed during the wait (because an appendBlock was queued ahead of the flush's doFlush in the per-path mutex and its scheduleFrontmatterFlush ran during the in-flight window), the method recurses on the fresh path so the queued updated: snapshot lands on disk instead of being silently abandoned. As a complementary defence, flushAll() now drains in a bounded loop (cap 4 passes) so any snapshot armed during the doFlush's own readFile/writeFile cycle is also picked up before teardown returns. Each pendingFields update is a monotonic forward replacement, so the recursion + loop converge in ≤ 2 passes worst-case.
Regression test "flushAll() drains pendingFields armed during an in-flight flush (Codex P1 round-3)" in tests/application/chat/SessionLogWriter.test.ts parks appendFile so turn 2 sits in the mutex with its body not yet on disk, fires turn 1's debounce timer (queueing doFlush behind turn 2), releases appendFile so turn 2's scheduleFrontmatterFlush runs during the in-flight flush, parks readFile to keep the flush in-flight, then calls flushAll(). Verified the test FAILS on 570b1c0 (frontmatter updated: reads turn 1's timestamp) and PASSES on c272857 (updated: reflects turn 2's timestamp). Body content for turn 2 was always on disk via appendFile; the regression specifically asserts the frontmatter advance the bug used to drop.
Pre-PR gate all green on c272857: npm audit 0 vulns, typecheck, lint (0 errors, 25 pre-existing warnings), tests 1881/1881 (+1 vs round-2), build, build:web, docs:api.
Generated by Claude Code
… Codex P1 round-3) `SessionLogWriter.flushFrontmatter` used to return unconditionally after awaiting an in-flight flush, so any `pendingFields` snapshot queued during the wait was silently dropped. The scenario that triggers it: an `appendBlock` queued ahead of the flush's `doFlush` in the per-path mutex runs its `scheduleFrontmatterFlush` AFTER the flush has captured + nulled `pendingFields` but BEFORE `doFlush` actually runs, repopulating `pendingFields` with a strictly newer snapshot. On plugin teardown (the documented `flushAll()` path), `flushAll` clears the re-armed timer and calls `flushFrontmatter`, which sees `inFlight !== null` and returns early — the new `updated:` timestamp is left armed with no timer and never written to disk, even though the turn body itself was already on disk via `appendFile`. This patch adds two complementary defences: `flushFrontmatter` re-checks `pendingFields` after `await pending.inFlight` and recurses on the fresh path if a snapshot is still queued, and `flushAll` drains in a bounded loop (cap 4 passes) so a snapshot armed during the doFlush's own readFile/writeFile cycle also lands before teardown returns. Each `pendingFields` update is a monotonic forward replacement, so the recursion + loop converge in ≤ 2 passes worst-case. Regression test in `tests/application/chat/SessionLogWriter.test.ts` parks `appendFile` so turn 2 sits in the mutex with its body not yet on disk, fires turn 1's debounce timer (queueing `doFlush` behind turn 2), releases `appendFile` so turn 2's `scheduleFrontmatterFlush` runs during the in-flight flush, parks `readFile` to keep the flush in-flight, then calls `flushAll()`. Pre-fix the on-disk `updated:` reads turn 1's timestamp; post-fix it reads turn 2's. Verified the test FAILS on 570b1c0 and PASSES on this commit. https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c272857905
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const frontmatter = buildFrontmatter(fields) | ||
| await this.vault.writeFile(resolvedPath, frontmatter) |
There was a problem hiding this comment.
When seedCache sees an existing session log whose frontmatter cannot be parsed, this branch rewrites the file with frontmatter only before appending the new turn. In that scenario (for example, a manually edited or partially corrupted log), all pre-existing body content is deleted, so historical turns are lost on the next write. The fallback should keep the existing file content as body data (or move aside/conflict) instead of truncating it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — valid P1 confirmed and fixed in 952ae30. You're right that the previous fallback wrote ${frontmatter} only and silently truncated every prior turn on disk. That's the worst kind of data loss: silent, triggered by a user trying to be helpful (manually editing the log in Obsidian and accidentally breaking a delimiter, or saving the file as raw text).
The fix preserves the existing blob verbatim. When splitFrontmatterAndBody(existing) === null, seedCache now writes ${frontmatter}${existing} — the user's data survives byte-for-byte under a fresh frontmatter window. The file structure is unusual (new frontmatter, then the old malformed bytes inside the body) but every byte is recoverable by hand. bodyEndsWithNewline is derived from the preserved blob's tail so the next appendBlock composes a correct separator. A warn log with the resolved path + redacted sessionId fires on entry to the malformed branch so a maintainer can spot the unusual event.
Three regression tests under SessionLogWriter malformed-frontmatter seed (Codex P1 round-4) cover the branch directly (malformed frontmatter with a multi-turn body, raw-text file with no frontmatter at all, and a two-turn-plus-flush sequence). Each asserts every byte of the original on-disk content survives the next write. Test count: 1884/1884 passing (+3 vs round-3). All four prior fixes (P1 rounds 1–3, P2) keep working. Full pre-PR gate green locally.
Generated by Claude Code
…Codex P1 round-4)
Codex P1 round-4 (thread 3254865902): the WP-5 cache rework introduced
a silent data-destruction path in `seedCache`. When the on-disk
session log had malformed frontmatter (e.g. the user manually edited
the file in Obsidian and accidentally broke a YAML delimiter, or
saved the file as raw text without a frontmatter block), the
fallback wrote `${frontmatter}` only before appending the new turn
— silently truncating every prior turn the user had on disk. The
worst kind of data loss, because it's triggered by a user trying to
be helpful.
The fix preserves the existing blob verbatim. When
`splitFrontmatterAndBody` returns null, `seedCache` now writes
`${frontmatter}${existing}` and derives `bodyEndsWithNewline` from
the preserved blob's tail. The user's data survives byte-for-byte;
the file structure is unusual (new frontmatter window + old
malformed bytes inside the body) but every byte is recoverable.
Subsequent reads parse the new frontmatter cleanly. A `warn` log
fires on the malformed-frontmatter event so the next maintainer
sees it in the console.
Three regression tests in `tests/application/chat/SessionLogWriter.test.ts`
cover the malformed-frontmatter branch directly: malformed
frontmatter with a multi-turn body, file with no frontmatter at
all (raw text), and a two-turn-plus-flush sequence. Each asserts
that every byte of the original on-disk content survives the next
write. Verified the tests FAIL on the round-3 head (c272857) and
PASS on the round-4 fix. Test count: 1884/1884 passing (+3 vs
round-3). The four prior fixes (P1 rounds 1–3, P2) keep working.
https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 952ae30c4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function useSessionLogMirror(): UseSessionLogMirror { | ||
| const vault = useVaultPort() | ||
| const logger = useLoggerPort() | ||
| const settings = useSettingsPort() | ||
| let cached: SessionLogMirror | null = null | ||
| let cachedSpecsFolder: string | null = null |
There was a problem hiding this comment.
useSessionLogMirror now builds a writer that debounces frontmatter updates, but this composable never registers a teardown path (flushAll) for the cached mirror. If the sidebar/plugin is closed (or the app exits) within the debounce window after a turn append, the turn body is already on disk but the latest updated: frontmatter value can be lost because it was only staged in memory. Please ensure teardown drains pending flushes before unmount/unload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 90882f2. Both teardown surfaces now drain the debounced frontmatter flush.
Composable teardown — useSessionLogMirror registers an onBeforeUnmount hook (guarded by getCurrentInstance() !== null so non-component callers still work) that calls void cached.flushAll(). Vue's teardown runs synchronously and cannot await, but the writer's per-path mutex (round-2) plus the bounded re-drain loop in flushAll (round-3) complete the work on microtasks regardless. The hook deregisters the mirror from the module-level activeMirrors set before firing the drain so the plugin-level path cannot double-flush a retiring writer. The specsFolder-invalidation branch in getMirror() retires the replaced mirror's entry for the same reason.
Plugin teardown — SpecoratorPlugin.onunload() now invokes void flushAllActiveSessionLogMirrors() immediately after detachLeavesOfType(...) and before bridge.hideAllNotices(). The exported drain wraps each live mirror's flushAll() in Promise.allSettled so an independent failure on one path cannot strand the others. Fire-and-forget shape matches the existing chatThreads flush (Codex P1, PR #346) — Obsidian's onunload contract is synchronous so we cannot await.
Architectural note — the mirror is per-ChatSidebar-mount, not a global singleton (useSessionLogMirror() is invoked exactly once in ChatSidebar.vue's <script setup>). The plugin-level registry exists not for fan-out but because Vue's onBeforeUnmount cannot reach the app-exit / plugin-disable code path that runs through onunload. Tests: +5 in useSessionLogMirror.test.ts (unmount drain, lazy no-op, multi-mount drain, double-flush prevention, end-to-end on-disk frontmatter check) plus a new main.session-log-flush.test.ts (+3 covering the onunload drain contract). Total: 1884 → 1892. Pre-PR gate green across audit / typecheck / lint / test / build / build:web / docs:api.
Generated by Claude Code
…WP-5 Codex P2 round-2) The `useSessionLogMirror` composable now registers an `onBeforeUnmount` hook that calls `void cached.flushAll()` when its host component unmounts. The underlying `SessionLogWriter` debounces the `updated:` frontmatter rewrite for up to 30 s after each turn append (the body itself lands synchronously via `appendFile`). Without a teardown hook, closing the sidebar inside that window dropped the pending frontmatter snapshot, and the next session load showed the new turn body with a stale `updated:` timestamp. A module-level `activeMirrors` registry plus a new exported `flushAllActiveSessionLogMirrors()` give the plugin layer a single handle on every live mirror. `SpecoratorPlugin.onunload()` now invokes the drain immediately after `detachLeavesOfType(...)`, matching the existing fire-and-forget pattern used for `chatThreads` (Codex P1, PR #346). The composable's unmount hook deregisters the mirror BEFORE firing `flushAll()` so the plugin-level drain cannot double-flush a retiring writer, and the `specsFolder`-invalidation branch in `getMirror()` retires the replaced mirror's registry entry for the same reason. The fix composes cleanly with WP-5 rounds 1-4: the per-path mutex (round-2), bounded re-drain loop (round-3), and malformed-frontmatter preservation (round-4) all continue to apply. Adds 5 new regression tests to `useSessionLogMirror.test.ts` (unmount drains the cached mirror, drain registry covers multi-mount, deregister order prevents double-flush, end-to-end on-disk frontmatter check) and a new `tests/plugin/main.session-log-flush.test.ts` (3 tests covering the `onunload()` drain contract). Test count: 1884 → 1892. https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90882f2cdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (cached !== null) { | ||
| activeMirrors.delete(cached) | ||
| } |
There was a problem hiding this comment.
When specsFolder changes, this branch removes the previous cached mirror from activeMirrors and replaces it without draining the old writer first. Because SessionLogWriter defers frontmatter writes with a debounce, the old mirror can still hold a pending updated: flush; after it is deregistered, flushAllActiveSessionLogMirrors() during plugin teardown will skip it, so closing/disabling the plugin within that debounce window can drop the latest frontmatter update for the old path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f64a6ec (P2 round-3).
The specsFolder-invalidation branch in useSessionLogMirror.ts now calls void cached.flushAll() before activeMirrors.delete(cached). Drain then deregister: the drain is fire-and-forget (matching the existing onBeforeUnmount hook in the same file — the call site is inside the synchronous inFlight Promise constructor and cannot await without serialising every subsequent getMirror() caller behind the old debounce window). The writer's per-path mutex + bounded re-drain loop (round-3 fix) completes the drain on microtasks. The microtask overlap with flushAllActiveSessionLogMirrors() is safe because flushAll() is idempotent — the bounded re-drain loop folds concurrent invocations into a single per-path flush.
Regression test: tests/ui/composables/useSessionLogMirror.test.ts — "drains the retired mirror when specsFolder changes mid-session (Codex P2 round-3, PR #406)". Mounts the host with specsFolder="specs", awaits a mirror, spies flushAll, flips the setting to "docs/specs", awaits a second mirror, asserts the new mirror is a fresh instance and that the old mirror's flushAll was invoked exactly once. Verified FAIL on 90882f2, PASS on f64a6ec. Full pre-PR gate (audit / typecheck / lint / 1893 tests / build / build:web / docs:api) is green.
Generated by Claude Code
…P2 round-3) When the user changed `specsFolder` mid-session, the composable deregistered the old mirror from `activeMirrors` without draining its pending debounced frontmatter flush. A plugin teardown inside the 30 s debounce window then dropped the latest `updated:` snapshot for the old path because `flushAllActiveSessionLogMirrors()` could no longer see it. Drain THEN deregister. The fire-and-forget shape matches the existing `onBeforeUnmount` hook in the same file; the writer's per-path mutex + bounded re-drain loop completes the work on microtasks. Regression test fails on 90882f2 and passes after the fix (Codex thread 3254908179). https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
2 participants
Summary
VaultPort.appendFile(path, content)and implement across all three runtime adapters (ObsidianBridgeviavault.adapter.append,MockBridgewith in-memory concat + a narrowcallsrecorder,LocalStorageBridgevia read+concat+write).SessionLogWriternow keeps a per-pathLogPathCacheof{frontmatter, body}seeded once on first append, callsappendFilefor each new turn block, andwriteFileonly to rewrite the frontmatter window with the bumpedupdated:timestamp. The DoD's 100-turn probe drops from 100 readFile + 100 writeFile down to 0 readFile + 100 appendFile + 101 writeFile on a fresh thread (100 turns of body re-reads eliminated).SessionLogMirrorfacade. New thin wrapper withmirrorTurn(fire-and-forget) +mirrorProposalDecision(await-required) + acreateSessionLogMirrorfactory. Replaces every directSessionLogWriterimport outsidesrc/application/chat/—ChatTurnOrchestrator,commitFileWriteProposal,useSessionLogMirror(renamed fromuseSessionLogWriter),useProposalDecisions, andChatSidebar.vueall consume the mirror. Lint scope rule added so the writer cannot be re-imported from UI by accident.DoD checklist
npm audit --audit-level=high --omit=devclean (0 vulnerabilities)npm run typecheckcleannpm run lintclean (0 errors; 24 pre-existing warnings)npm run test— 1878 / 1878 passing, including 2 new DoD tests inSessionLogWriter.test.ts, a delegation suite inSessionLogMirror.test.ts, and a cross-adapterVaultPort.appendFilecontract testnpm run build+npm run build:websucceednpm run docs:apisucceedsmockBridge.calls.appendFile.length === 100andreadFile.length === 0on a fresh thread (≤ 1 for the resumed-session path)SessionLogWriteroutsideapplication/chat/, enforced byno-restricted-importsTest plan
specs/<feature>/sessions/<sid>.mdaccumulates## user/## assistantblocks and thatupdated:advances with each turn whilecreated:stays pinned.SESSION_LOG_FAILEDwhen the audit-row append rejects (covered bycommitFileWriteProposal.test.ts).import { SessionLogWriter } from '@/application/chat/SessionLogWriter'insidesrc/ui/**.https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
Generated by Claude Code