fix: make adding, indexing, and renaming sources work - #100
Merged
Conversation
…, bad-layer quarantine) Three failures a user hit on the shipped 0.5.0 Mac app, written as the gate before the fix. The suite is RED on purpose — Phases 1-3 make it green without touching the assertions. 1. An open Obsidian vault never finishes indexing. The layer-root fs.watch is unfiltered, and every event debounces into invalidateIndex, which cancels and restarts the running job; .obsidian/workspace.json is rewritten the whole time the vault is open. A control host indexes the identical corpus untouched first, so a failure here is the watcher and not a slow machine — and the observation window is derived from that measurement rather than fixed, so a loaded runner buys time instead of a false failure. 2. Renaming a source re-reads it from scratch. The index cache key is JSON.stringify(layer), so a rename mints a new key and the source blinks to zero concepts. The observer watches the whole window, not the end of it: a re-index that finishes inside five seconds still blanked the graph on the way through. 3. One malformed layer answers 500 on every route, /api/settings included — the screen a user would need to fix the bad source. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Review found the fixture size was doing load-bearing work silently: at INDEX_STABILITY_NOTES=800 the control settles in ~740ms and assertion 1 goes GREEN against an unfixed engine, because a corpus that fast finishes inside the gap between watcher restarts. Two preflight guards close both ways that can happen, and both fail loudly rather than skipping — a gate that quietly stops gating is worse than no gate. - Control floor: a control pass must clear 4x the watcher's 250ms debounce. Below that the suite reports the corpus as too small, names the measured and required times, and suggests a scaled INDEX_STABILITY_NOTES. The existing READY_TIMEOUT_MS derivation protects a slow machine from a false failure; this protects a fast one from a false pass. - Recursive-watch probe: service.mjs silently falls back to a non-recursive fs.watch when the recursive call throws (inotify exhaustion on a busy runner is the realistic way in), and in that state no write under .obsidian/ ever reaches the watcher. The probe writes into a subdirectory of a watched temp dir and requires an event. Verified to fire on Linux with the fallback simulated; macOS delivers subdirectory events either way, so the probe passes there on merit. Also moves the suite to the end of the test chain. It is red for the duration of the fix work, and && meant npm run eval, test:metrics and test:release-workflow were unreachable behind it — the retrieval eval in particular has to stay runnable while the engine changes. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…get test Two numbers behind the field report that the shipped 0.5.0 Mac app feels laggy, written as the gate before the fix. The fixture is the realistic one: 3,000 documents at 10-50KB (89MB, avg 30.5KB), because small documents hide the per-document CPU cost both assertions exist to measure. GRAPH LATENCY — RED, and the whole of the reported lag. buildGraph resolves every concept and runs an exact BPE countTokens on every request, uncached. Three consecutive calls come in at 4945ms / 5045ms / 5028ms against a 150ms budget. The flat profile is the tell: there is nothing to warm up. The console polls this route every 900ms while indexing. RESPONSIVENESS UNDER INDEXING — GREEN, and kept as a live SLO gate. The plan expected this to be red on the theory that YIELD_EVERY=25 lets index work starve the serving loop. It does not: snapshotSource awaits source.loadConcept(id), and both files.mjs and okf-local.mjs reach fsp for it, so the loop is released on EVERY document. The 25-document yield only bites where loadConcept is a Map lookup — snapshotView inside buildGraph. Measured over a 5.7s index: p50 1.67ms, p95 2.90ms, max 3.60ms against 50ms/250ms. Keeping the assertion is still worth it — it goes red the day someone raises YIELD_EVERY, drops a yield, or makes loadConcept synchronous. Three guards, because a fixture that is too small makes both assertions go green against an unfixed engine — verified, not theorised: at 200 documents of 2KB the graph budget passes at 26ms with the bug fully present. Corpus weight (bytes and average document size), window length and sample count, and a concept-count check that the graph being timed is the whole corpus. Each fails loudly with a scaled re-run knob, and any assertion that passes while a guard is down is labelled UNGATED so a green line cannot be quoted as evidence. An idle control probe runs first and reports ~1.4ms, so a high number during indexing would be starvation rather than measurement overhead. The window is bounded by polling /api/graph at 500ms — cheap only because buildGraph skips sources that have not finished indexing, so the readiness signal costs under a tenth of a percent of the window. The probes issued from the moment that poll turns expensive are excluded and reported: the server keeps building the graph that poll triggered, and those two samples were the source of every stray ~100ms max in this window. Excluding them takes the measured max from ~113ms to ~3.6ms, which is what makes the assertion safe on a slower CI box. Also prints, as diagnostics rather than assertions, the same probe under the console's real traffic. That margin is ~1.4x today and would flake in CI; whoever memoizes buildGraph should revisit promoting it. Appended after index-stability, keeping the property that Phase 0's gates sit at the end of the &&-joined chain. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…RINUSE A run killed with SIGKILL (a harness timeout, an interrupted CI job) never fires its EXIT trap, so its hosts outlive it and hold 8841-8843 or 8861. The next run then dies deep inside startup with a bare EADDRINUSE that names neither the cause nor the fix; two such orphans were found holding index-stability's ports during Phase 0. Trap INT and TERM as well as EXIT so the ordinary interrupt paths do clean up, and preflight the ports so the unrecoverable case (SIGKILL) reports the port, the reason, and the exact command to find the process. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
walkDocs and probeDocs are one long await from a caller's point of view, so cancelling the work around them cancelled nothing: the walk ran to the end of the tree regardless. A layer whose index was cancelled and restarted therefore accumulated one abandoned walk per restart, all of them competing for the same disk and the same event loop as the job that replaced them. Both now take an optional AbortSignal and check it once per directory — a readdir is the unit of work, and per-entry checking would cost more than it saves. The signal's own reason is rethrown, so whoever awaits learns whether the walk was superseded or timed out rather than getting a generic error. The adapters that own a disk walk (okf-local, files) accept the signal through listConceptIds and pass it down; withGitSync forwards whatever it is handed so a live layer's walk is abortable too. Every argument is optional, so an adapter that ignores it — github, mcp, the memoizing cache wrapper, and any caller outside the service — behaves exactly as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Two field reports against the shipped 0.5.0 Mac app, one root cause each, both
in the same code path. packages/core/tests/index-stability-test.sh was written
first and is the gate; assertions 1 and 2 go green here, assertion 3 (bad-layer
quarantine) is a separate change and stays red.
ADDING AN OBSIDIAN VAULT HUNG FOREVER. Obsidian rewrites
.obsidian/workspace.json every few hundred milliseconds for as long as a vault
is open. Every watcher event debounced 250ms into invalidateIndex, which
CANCELLED the running index and started a new one — and 300ms is far shorter
than any real index takes, so a 3,000-note vault sawtoothed (1375 loaded, 150,
1950, ...) and never reached ready. Measured: the same vault indexes in 2.8s
untouched.
Two fixes, because either alone is a half-answer. The watcher now filters
exactly what walkDocs filters — dot-entries and node_modules, plus a
known-uninteresting extension where the basename gives one — so the file that
caused this never reaches invalidateIndex at all. And an invalidation that
lands mid-index no longer cancels: it marks the entry dirty and the job starts
exactly one follow-up pass when it lands, success or failure. Filtering keeps
this particular churn out; coalescing is what makes a restart storm impossible
rather than merely unlikely, for the genuine edits a filter must let through.
A document rewritten faster than one pass completes now keeps its full
previous snapshot readable throughout instead of showing zero concepts.
A null filename (macOS reports one) still invalidates, and so does an
extensionless entry: a folder rename delivers the folder, never the documents
inside it. A missed edit is a wrong answer; a needless re-index is only work.
RENAMING A SOURCE HUNG ON "Saving...". The index key was JSON.stringify(layer),
which includes name and level, so a rename minted a new key: pruneIndexes
dropped the finished entry and ensureIndexes started over with no previous
snapshot, blanking the source to 0 concepts and re-reading every file for a
change that touched no content. The key is now the layer's content identity —
every field except name and level, serialized key-order-independently so a
manifest rewrite that only reorders fields costs nothing. Both excluded fields
are safe to exclude because the snapshot holds only {ids, concepts, tokens} and
snapshotView reads name and level off the live source object at resolve time,
so a rename or a precedence change lands immediately at zero cost. A denylist
rather than a list of known identity fields, deliberately: a source option
added later must invalidate the index until someone proves otherwise.
The 0-concept window is closed for the key changes that remain legitimate, too
— an indexing limit edited in Settings, a credential arriving. Those orphan an
entry whose content identity is untouched, so pruneIndexes hands its snapshot
to the replacement, which serves it through the re-read it does still owe.
Also here: the index job's abort signal now reaches listConceptIds, so a
cancelled index stops walking instead of finishing in the background; the
add-form's 5s folder probe aborts its walk at the deadline rather than
answering the form and scanning on; the watcher map is keyed by root with the
layer name resolved when an event fires, fixing a stale closure that made a
renamed layer's watcher invalidate a name matching nothing; and the silent
fallback to non-recursive watching says so on stderr, because in that state an
edit inside a subfolder is never noticed at all. The comment claiming recursive
fs.watch is unsupported on Linux was stale — it has worked since Node 20.13.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
… silently partial Three failures a user hit on the shipped 0.5.0 Mac app, none of which the engine had any way to survive. One bad layer bricked every endpoint. Manifest validation throws inside openSources(), which every route calls, so a single hand-edited layer made /api/settings and /api/graph answer 500 — including the screens needed to find and fix it. The read path now quarantines a layer that fails validation instead: it is removed from the manifest the engine reads, so it can never be built, spawned, watched, or reached by the file APIs, and survives only as an inert error source that gives it a broken row in the graph. Strictly subtractive. Rules about how layers RELATE (duplicate name, second live layer) stay fatal — quarantining one of a well-formed pair would mean silently picking a winner, which would be more permissive than today. Writes still validate the whole manifest strictly, so nothing invalid is persisted. A 500MB .md was read whole into a JS string. Local adapters had no per-file ceiling at all, where the github adapter has capped since it was written. walkDocs and both local loadConcept paths now stat before reading and skip past 2,000,000 bytes — the same number the file editor already refused to open, now one constant. A permission-blocked subfolder indexed silently partial: the walk skipped it and the source reported "ok" with quietly missing documents. Skipping is still right, but it is now recorded. Both cases ride the snapshot to a per-source `warnings` count and human-readable `warningMessages`, rendered in the console's source detail. Also raises the default per-source time budget from 30s to 120s. The old value was set against test corpora; a large vault on iCloud or Dropbox spends its first index waiting on the file provider and came back as an error row that read like an app bug. Indexing is background work nothing blocks on. index-stability-test.sh assertion 3 is the gate for the first of these and is unmodified. The other two are covered by new failing-first assertions in files-source-test.sh (adapter level) and setup-robustness-test.sh (the row a user actually sees). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…atus buildGraph resolved every concept and ran an exact BPE countTokens over the whole merged corpus on every request, uncached. Measured: 4.8s per call on an 89MB fixture, 14.6s on a real 139MB Obsidian-shaped vault — flat across consecutive calls, because there was nothing to warm. The console polls this route every 900ms while indexing, and it polls it because /api/graph was the ONLY endpoint carrying index progress. That pairing is the whole of the field-reported lag, so both halves are fixed here. Making the rebuild cheap, not just memoized — a memo alone would still stall for 14s on every miss, and a miss happens exactly when an index lands, i.e. when the user is watching: - snapshotSource keeps the per-concept token count it already computes, taken over the merged shape (mergeConcepts of the single contributor). For any concept only one source contributes, resolveConcept reduces to exactly that call, so the number is reusable verbatim rather than an approximation. Genuinely merged concepts fall back to a cache keyed by their contributors' precedence and snapshot generation, so a re-index of one layer only re-encodes what that layer touches. - The concept rows and resolvedTokens are memoized on the (name, level, snapshot generation) triple of the sources they read. The key is recomputed from live state on every request rather than cleared by an invalidation event: snapshots are immutable and generations unique, so there is no trigger to forget. Progress, health and warnings are still rebuilt per request, so a loaded counter ticking through an index does not throw the expensive half away. GET /api/status is O(sources) by construction — index progress and per-source state, no resolve, no tokenize — and carries a `generation` counter that moves whenever the graph payload would differ, so a client can poll it and refetch the heavy routes only on a real move. /api/graph reports the same counter. After: 22ms cold / 2ms warm on the 89MB fixture, 64ms cold / 2-3ms warm and sub-millisecond /api/status on the 139MB vault. resolvedTokens is byte-identical to before on both. A source's own `tokens` total now counts what it contributes rather than its raw file text (tombstones, blank-line padding, sections sharing an anchor) — the same derivation resolvedTokens uses, which is the point of showing them side by side. The index path is untouched: it awaits real I/O per document and does not starve the serving loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…ache withCache took no arguments on listConceptIds, so a layer with a `cache` block lost both things that ride on that call. The wrapper is applied to any kind — a local vault included — so the walk became unabortable (a cancelled index kept reading to the end, and a churning layer stacked one live walk per cancelled job) and `notes` never reached walkDocs, which meant an oversized document or a permission-blocked subtree produced a source that was missing documents and reported zero warnings. That is the silently-partial failure the warnings exist to close. Arguments now go through, and the notes are cached with the ids so the warnings survive a cache hit rather than flickering off on the second read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…ename path.extname called a directory a file whenever its name contained a dot, and directories routinely do: "Archive 2024.10", "notes.old", "Project.v2". Moving a folder of notes into a watched vault delivers ONE event, naming the folder — so the watcher dropped the only event there was and every document inside stayed out of the index until something else happened to invalidate the layer. Silent data loss, against a needless re-index in the other direction. Non-document extensions still filter (an image dropped into attachments/ costs nothing), and a path that is already gone counts as a change: the index may well hold what used to be there. The stat runs after the dot-entry filter, so .obsidian churn never reaches it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…alid
Three defects in one state machine, fixed as one model (index-keys.mjs):
identity — what a layer reads. `kind` now has a reserved slot instead of
sharing a flat object with the layer's own fields, where a field
literally named `kind` (not reserved, and written by the desktop app's
settings sync) overwrote it. Two layers then produced ONE identity,
which was the index key, so one entry served both rows: a folder
reported another source's documents as its own and won the merge with
them. Every user field now nests a level down, and the null-prototype
object keeps __proto__ stored rather than swallowed.
validity — identity plus the policy that governed the read: the indexing
settings and the credential epoch.
key — validity plus the layer row, uniquified. Unique per row by
construction rather than by argument.
Orphaned entries are now ADOPTED (moved to the new key) when their
validity still matches, instead of being mined for a snapshot and
restarted. A rename costs nothing at all, and because the move happens
inside openSources, no consumer can arrive "first" and miss it — the
handoff used to live in ensureIndexes alone, so a watcher-driven
invalidateIndex after a re-key blanked the source to 0 concepts. An entry
orphaned BY a policy change matches nothing and is dropped: lowering a
document cap no longer keeps serving the over-cap answer (forever, when
the re-index then failed), and Disconnect no longer keeps serving a
private repo's content.
Refreshing is now visible and bounded. A pass with a previous snapshot
reports `status: "ready"` plus an additive `indexing.refreshing`, so a
background re-read never flips a usable source — or the console's spinner
— back to unready; `awaitIndexes` waits on the pass rather than on status.
A pass dirtied mid-flight owes exactly one follow-up, and that follow-up
waits out a quiet period as long as the last pass took (1-15s), so
sustained editing costs one extra pass instead of an unbounded chain of
full re-walks. `indexing.passes` reports the count, which is the only way
to see a re-index storm from outside.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
A user added a 3,000-note Obsidian vault to the shipped Mac app and it
"showed and then it just kind of hung there". The engine was fine. The UI
was lying: a still-indexing source rendered as "synced · 0 concepts", the
add wizard declared success off the POST alone, and a failing background
refresh gave up after three tries and said nothing at all.
Per-source truth
- adaptSources maps the engine's `status: "indexing"` to a real indexing
state and carries `indexing.{phase, loaded, total, refreshing}` through,
so "synced · 0 concepts" is now unreachable for a source being read.
- mergeSourceStatus folds a cheap status pass into the rows the views hold,
so a Sources row tracks the toolbar instead of holding whatever phase the
source started in.
- Warning chips count `warnings` (the true count) rather than
`warningMessages.length` (capped at 10).
A shell-wide activity control
- The bare "Indexing N" badge becomes a compact progress affordance in the
toolbar, visible from every destination, with a popover listing each task
(source, phase, loaded/total, elapsed). Ready-and-refreshing is rendered
as a quiet note, never as a spinner in front of data the user already has.
No more silent give-up
- The poll retries at capped backoff forever, keeps `indexingSources`, and
exposes `refreshError` + `retryNow()` — a dismissible banner and an
attention-toned indicator. `apiFetch` no longer awaits the desktop token
without a deadline; one stalled IPC used to poison the whole session.
Cheap polling
- /api/status at 900ms (5s idle), heavy payloads only when the content moved,
paused while the document is hidden. Measured on the 3,000-note vault:
24 status calls and 2 resolve-alls, where the old loop issued 24 × 150MB.
The wizard confirms the add landed
- After the POST it polls /api/status until the named source appears and
shows its live status card. A lost response and a 409 are now resolved by
asking the engine, not by assuming; Finish no longer blocks on /api/graph.
Also fixes a pre-existing crash the silent give-up was hiding: a plain note
with no heading resolves to `heading: null`, and headingText called .replace
on it — which took the page down on any Obsidian-shaped vault.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
The suite asserted that a mid-index source is never described as finished, but nothing drove the other half — a source actually reaching `ready`. That gap is why a frozen "Scanning / Indexing" row read as a plausible product bug: there was no test anyone could point at to say the poll path settles. Drives App against one fake engine whose state advances and which answers /api/status, /api/graph and /api/resolve-all from it, so the test cannot pass by mocking the two routes into disagreeing. Asserts the rendered row walks Scanning -> Reading N/3,000 -> 3000 concepts, that the toolbar indicator clears itself, and that the heavy payload is fetched once at bootstrap and exactly once more when the snapshot lands. Verified to fail against three separate injuries: dropping the mergeSourceStatus fold (row sticks on "Scanning"), disabling the refetch gate (concepts never arrive), and retaining a finished task (toolbar indicator never clears). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
siracusa5
force-pushed
the
c/context-cake-source-audit-a3b7ea
branch
from
August 7, 2026 13:19
d815c7a to
cbef7d8
Compare
…cy audit CVE-2026-59870 (GHSA-5p4m-2wfm-xmqj) covers js-yaml 4.0.0-4.3.0, and the lockfile pinned 4.3.0 transitively through electron-updater — a shipped dependency, so `npm audit --omit=dev --audit-level=high` fails the desktop job and with it the required gate. 4.3.1 carries the fix and already satisfies electron-updater's own `^4.1.0` range, so this is a lockfile bump rather than an override: no package.json change, no semver exception to remember later. Unrelated to the rest of this branch; it is here because the advisory was published against the existing tree and blocks CI on any branch that touches it. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
A documentation review of the branch found three statements in the root CLAUDE.md that this work itself falsified, plus user-facing docs that describe behavior that no longer exists. Self-inflicted, in CLAUDE.md: - "keyed by its layer config + settings" contradicted the bullet directly beneath it, which correctly says name and level are excluded and the credential epoch is included. Removed the superseded sentence. - "carries a generation counter that moves whenever the graph payload would differ" overstated the contract: bumpGeneration deliberately excludes indexing.elapsedMs and passes, and the console additionally compares a content signature, so a generation move is necessary but not sufficient. - "npm test ends with the eval" stopped being true when this branch appended three suites after it. The point of the gotcha — a ranking regression fails the build — is unchanged. User-facing: the manifest reference now documents the per-document 2 MB ceiling and the permission-skip warnings, and gains a section on what happens when a layer is invalid, since a hand-edited manifest is exactly how someone meets quarantine. /api/status is named where /api/graph already was, in the console and playground READMEs and the playground tour. Also: a manifest.mjs row in the key-file table (it gained two load-bearing exports here), BackgroundActivity.tsx in the console's key files, and the passes field on IndexProgress. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
The poll committed `generation` and `signature` before awaiting the heavy read. A rejected readAll — a resolve-all timeout on a large vault, a transient 500 — therefore left the gate looking satisfied: the next tick saw nothing moved, skipped the retry, and took the success path, which cleared the refresh banner and stamped lastRefreshAt. The console then showed pre-edit concepts and conflicts for the rest of the session, since contentSignature deliberately excludes document content and so nothing reopens the gate on its own. The gate now advances in exactly one place: past every await in readAll, where the payload has actually landed. A refetch that was attempted and did not land is remembered as owed, so a signature that flaps back to its old value cannot close the gate either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
writeSectionApi read the target with fsp.readFile and no size check, while
the read and write file routes both gate on MAX_EDITABLE_BYTES. Measured
against a 30MB .md inside a layer root: PUT /api/section answered
{"ok":true} and left 64 bytes on disk. That is a document the indexer
refuses to read — one the cascade never served — destroyed by an editor
that was never able to show it.
The stat is already in hand, so the cap is checked before the read, with
writeFileApi's 413. Refusing beats skipping the layer: resolving a section
into every layer except the oversized one only mints a new partial
disagreement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
quarantineInvalidLayers rebuilt profiles with plain assignment, so a profile named __proto__ set the prototype of the new container instead of becoming an own key. Object.entries stopped seeing it, the re-validation passed, and the quarantined read accepted a manifest the strict read rejects — the app loading what the CLI and MCP paths refuse. A dropped layer carrying a reserved key diverged the same way. assertSafeKeys now runs before anything is copied, which lands the caller on the authoritative strict error, and the profile accumulator uses defineProperty so a reserved id could not reach the prototype even if it got that far. Object.prototype was never polluted and the smuggled profile was unreachable; this closes the divergence, not an escalation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
dismissedRefreshError was never cleared, so one Dismiss muted that wording
for the whole session — including a fresh outage after a clean recovery,
which usually reads identically ("socket hang up"). The comment beside it
already said dismissal was per message; now the recovery makes it so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
The main process answers `service?.token ?? ''`, so a service that is not up yet resolves the token IPC with an empty string. desktopToken memoized that as success: every later /api call went out unauthenticated, the service 401'd each one, and nothing ever asked again. Not a bypass — the engine still refuses — but it bricks the session with no retry path. An empty token now throws, which drops the memo the same way a timeout does, so the next call asks the main process again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
15 tasks
isIndexableFile asks the filesystem whether an event names a directory, because inferring file-ness from path.extname swallowed folder-drop events and every document inside stayed invisible. That part is right. The statSync was not: it runs inside the fs.watch callback, on the event loop of a service whose entire premise is that no slow source blocks it. On an iCloud, Dropbox or SMB-backed vault — the same mounts that put the per-source budget at 120s — one stat blocks every request for as long as the mount takes to answer. Same rule, awaited. onChange already defers its real work behind a 250ms debounce, so the extra turn costs nothing, and it re-reads the watcher entry afterwards in case the source was removed while the filesystem was answering. The watch callback swallows a rejection rather than letting one fs event take the process down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
buildGraph read its source rows before awaiting the resolve and computed `generation` after it. A source whose index landed during that await therefore arrived already accounted for in the number, while its row still said "indexing, 0 concepts" — and the row's own progress block, read live, said "ready" at the same time. The console stored that generation, the next /api/status computed the identical one, `moved` stayed false, and the refetch that would have shown the landed source was never issued. The field report is "a source I added shows 0 concepts forever." pinEntry takes one observation of a source — snapshot, status, error, progress, adapter health — and both the payload and the generation are built from it. bumpGeneration now takes pinned observations rather than live entries, so there is no way to compute a number for a state the caller's payload does not contain. statusApi pins the same way (it is synchronous, so nothing moves, but the two routes must agree on what a generation means), and resolveAllApi pins for the same reason buildGraph does: its loop spans many event-loop turns and `indexingSources` has to name the state its concepts came from. The test drives the window on purpose. A remote layer is held at its first API call until the /api/graph request lands, which puts all of its remaining round trips inside the resolve; the local layer is sized so that loop yields far more times than the remote one needs to finish. The assertion is the invariant, not the timing: when the payload disagrees with what /api/status reports a moment later, the two generations must differ, or a client can never learn the difference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
mergeSourceStatus folds a cheap /api/status pass into rows the views already hold. It moved `status` and left `error` behind, so a source that flipped to error between heavy refetches rendered the word "error" with the block that says why still hidden — that block is gated on `error`. Warnings had the mirror problem: they describe a snapshot, and a row with none left the "indexed with N things left out" note hanging over from the last good read. `refreshing` is now normalized with `=== true`, matching adaptSources. The two write the same field on the same row, and the documented "returns the original array when nothing moved" identity contract only holds if they agree on how a missing flag reads. lastErrorAt/lastSuccessAt are deliberately untouched: /api/status carries no health timestamps, and inventing one would be a worse lie than a stale one. A status flip moves the engine's generation, so /api/graph fills them in on the next pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…neration `graph.generation` is optional on the wire. Against an engine that serves /api/status without it, readAll left the gate at undefined on every pass: `moved` was permanently true, and a settled, idle app pulled the whole corpus back every five seconds. The status route's number is the authority in that case — both routes report the same counter, and it is the one the gate compares against — so readAll takes it as a fallback from the poll that triggered the read. `?? generation` alone does not close this: the branch that commits a status generation is exactly the branch a permanently-true `moved` keeps you out of, so the loop would have continued. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
`if (idle) return null` runs during render, so when indexing finished the focused button and its popover unmounted in the same commit — and a focused node removed that way drops focus to <body>. A keyboard user standing on the indicator was silently returned to the top of the document, mid-task, with no event to explain it. Retirement is now two steps inside a layout effect: notice `idle`, hand focus to the next control in the toolbar (falling back to the header as a last resort), then unmount. Both steps land before paint, so nothing extra reaches the screen, and an app that has never had background work still renders nothing at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…us call fetchStatus answered null for a 404, a 500, a socket error and its own 4s timeout alike, and the success step's watcher read that one answer as "this engine has no /api/status": setWatched(null), return, done. One blip three seconds into a 3,000-note index retired the live cards for the rest of their life while the source was still reading. probeStatus keeps the two apart. 'absent' is a property of the engine and will not change while the wizard is open; 'failed' is a property of one request and says nothing about the next, so the watch keeps its rows on screen and asks again — capped at five consecutive failures so an engine that is genuinely gone stops being polled. The progress line also stops being a live region. It re-renders every 900ms, and role="status" meant a screen reader read the counter out on every tick for the whole of an index; App.tsx states the opposite doctrine and the activity popover's TaskRow already follows it. Now a progressbar with aria-valuetext, so the number is taken on request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Quarantine made a hand-edited layer visible without making it fixable. Every mutation route reads through mutateContextManifest, which reads STRICTLY, so with an invalid layer present POST/PATCH/DELETE /api/sources and PATCH /api/settings all threw before doing anything — including the one removal that would have made the manifest valid again. The app could show you the problem and nothing else. repairContextManifest is the door that opens: the single mutation allowed to READ a manifest holding an invalid layer, because it is the one that takes one out. Nothing about what may be PERSISTED moves. The write is still writeContextManifest, so the result must pass validateContextManifest in full; a repair that does not leave the manifest valid is refused and the file on disk is untouched. Both tolerant readers now share one private reader, so the door can never accept a manifest the read path would reject — a duplicate layer name, a second live layer, a broken profiles block all stay fatal. And a repair may only remove: no layers array may come out of the callback longer than it went in. The callback gets the RAW manifest, not the quarantined one. Handing it the cleaned manifest would delete every OTHER broken layer from the user's file as a side effect of removing the one they asked about. For the same reason quarantine records now carry `index` and removeSourceApi removes by index rather than by name: a broken layer that also reuses a healthy layer's name is shown as "seed (2)", and a name filter would have taken the healthy `seed` with it. ?name= repeats, and that is not a convenience. Only a valid manifest may be written, so with two invalid entries, removing either alone is refused — the other still fails validation. Removing them in one transaction is the only shape that both repairs the file and keeps the write strict; measured against three bad layers, the single-name form is a dead end from which the app can never recover. A request that asks for too little answers 409 naming what blocked it, never a 500. Console: rows carry `quarantined`, distinct from a source that failed to READ (createErrorSource now takes the flag explicitly — the two were indistinguishable from status alone, and only one of them can be acted on). On an invalid row Rename and Sync are gone, since both could only fail; the panel says the entry is not a working source, and Remove names every other invalid entry before the click rather than quietly sweeping rows nobody selected. PATCH /api/settings stays a strict write — quietly rewriting a manifest read around a bad layer is how a hand-edited layer gets dropped without being asked about — but it answered 500 with a layer validation error on the Settings screen, which tells the user nothing about where to go. It now answers 409 pointing at Sources, where the repair lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Review of the parent commit found the repair half-built. The write rewrites the whole manifest and only a valid one is ever saved, so an invalid entry refuses the removal of a perfectly healthy source exactly as it refuses the removal of another invalid one. The engine already answered that 409 with the remedy — name them in the same request — but the console only offered the sweep on an invalid row, so a user with one bad layer could not remove anything at all and was left staring at a message about a row that has nothing wrong with it. Every removal now carries the invalid entries with it, named in the confirm panel before the click. Same informed-consent shape as before, one row wider. The service test gains the mixed case directly: a healthy source refused on its own, then coming out together with the invalid entry beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Two independent changes answer "an open vault never settles" — onChange stops forwarding events the walk would never read (isSkippedPath), and invalidateIndex coalesces instead of cancel-and-restart. Assertion 1 gated only the second: with isSkippedPath deleted the suite stays fully green, so a cleanup that dropped the filter would have shipped an open vault re-walking every note on a timer. Assertion 1b measures the filter on its own terms. indexing.passes is already on the /api/graph row, so a settled source under churn it should be ignoring has a pass delta of exactly zero. The churn is shaped to isolate isSkippedPath from isIndexableFile: workspace.json is rejected by both, but a directory under .obsidian is accepted by isIndexableFile whatever the timing, so the dot-segment check is the only thing left. Verified by reverting the guard against a scratch copy — 1b reports "indexing.passes 1 -> 3" while assertion 1 still passes. Same file, second problem: CONTROL_FLOOR_MS only suggested a bigger fixture, so a runner faster than ~1.65x an M-series laptop turned a healthy engine red until a human re-ran it by hand. The control now rebuilds the corpus from its own measurement and re-times once; the loud failure stays as the fallback if even that is too fast. A non-numeric measurement now means 0 rather than 30000, which used to clear the very floor it was meant to trip. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
sourceLanded polled /api/status four times and called absence `false`, justified by a comment claiming the manifest write completes before the POST returns. It does not: addSourceApi writes the manifest LAST, after gitCloneOrPull and the MCP/github-rest probes. Add a large private repo, apiFetch aborts at its 60s deadline while the clone runs on, and the wizard threw "is not in the cascade, so nothing was added" — then the clone finished, the source WAS added, and a retry reported "already exists". Two contradictory messages for one successful add. The three-valued answer already existed. A request that hit its own deadline is now `null` (could not tell) rather than `false`, and the copy for it says the add may still be completing and points at Sources instead of asserting nothing happened. A definite request failure still means absence is evidence, and still says so. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Three gate-suite landmines, all of the same shape: a healthy engine going red for a reason that is not the engine. graph-latency's MIN_WINDOW_MS=3400 sits 1.8x under a measured 6151ms window, and the guard only SUGGESTED a bigger corpus — a runner that fast left the build red until a human re-ran it by hand. The corpus is now rebuilt from the measured window and re-taken once, replacing the undersized one so assertion 2 still describes the graph it measures. Lowering the floor would have re-opened the false-pass hole the floor exists to close, so the floor stays and the fixture moves. The loud failure remains as the fallback, and maxDocFiles gained headroom so a scaled corpus cannot trip a doc cap instead. setup-robustness asserted conceptCount == 2 BEFORE checking `id -u`, and that count only holds because chmod 000 hides a file. Running as root — the default in many CI images — failed with "readable documents missing" and only then explained why. The root check now comes first and SKIPS the two assertions that are literally about a permission error, running the rest against the counts root actually produces (3 concepts, 1 warning). Failing a healthy engine on root CI is how a suite gets muted. index-keys had a comment teaching the opposite of the invariant: a rename DOES re-key (index-keys.mjs puts the name in the key); the case constructed changes level, and what makes a rename free is adoptIndexes moving the entry. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
Two accessibility defects in the toolbar's background-work control. The trigger's aria-label was the full ticking description, rebuilt on every 900ms poll. Several screen readers re-announce a focused button whose accessible name changes, so a keyboard user standing on this control was read a fresh sentence four times a minute for the length of an index. The name is now built from the shape of the work — how many sources, indexing or refreshing, failing or not — and never from loaded/total/percent/elapsed, so it moves on a transition and not on a tick. The counts stay on `title`, where they are hover detail. Nothing announced a background REFRESH. The engine reports a refreshing source as `ready`, so it never reaches load.indexingSources and App.tsx's announcer never sees it: a sighted user watched this control appear and fill while a screen-reader user was told nothing. A polite region here now announces that one pair of transitions — never the aggregate ones the shell already speaks, and never a tick. It is rendered whether or not the control is, because a live region only announces content that changes while it is already in the document, and "the refresh finished" is exactly the transition where this control is on its way out. The progressbar rows stay progressbars, with the reasoning written down: a polite region around them would read the counter out on every tick. Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com>
…ea' into c/context-cake-source-audit-a3b7ea Signed-off-by: John Siracusa <siracusa5@users.noreply.github.com> # Conflicts: # packages/core/tests/manifest.test.mjs # packages/core/tests/service-test.sh
6 tasks
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.
Summary
A field test of the shipped 0.5.0 Mac app failed at the first mile: adding an Obsidian vault appeared to work and then hung without ever being added, renaming a git-backed source hung on "Saving…", and the app felt laggy throughout. Four parallel audits plus empirical reproduction found these were four independent defects stacked on each other, not one bug.
This PR fixes all of them and adds the regression tests whose absence let them ship. The headline root cause of "it showed and then just hung there": the console declared
headingText(heading: string)and called.replaceon it, but the engine legitimately returnsheading: nullfor a note with no#line — the normal shape of an Obsidian note, since Obsidian titles by filename. ThatTypeErrorkilled the render, and the store's silent give-up (three retries, then permanent silence) hid it.Changes
Indexing survives a vault that is being used
.obsidian/workspace.json— which Obsidian rewrites continuously while open — cancelled and restarted the index. Events are now filtered by the walker's own skip rules, and a dirtied pass coalesces into one follow-up behind an adaptive quiet period instead of chaining restarts forever.readywith an additiveindexing.refreshingflag while it re-reads, rather than flipping back to "indexing".Rename stopped re-reading the corpus
JSON.stringify(layer), which includesnameandlevel, so a rename minted a new key and re-read every file with no snapshot. Keys are now built from content identity (index-keys.mjs), and an orphaned entry is moved to its new key when its validity matches. Rename is now ~8ms with zero documents re-read./api/graphno longer costs secondsGET /api/status— O(sources), no resolve, no tokenize. Previously/api/graphwas the only route carrying index progress, which is precisely why the console polled the most expensive endpoint in the system.One bad layer no longer bricks the app
readContextManifest←openSources(), and every route callsopenSources(), so the whole API returned 500. Invalid layers are now quarantined on the read path as inert error rows. Writes still validate strictly; nothing invalid is ever constructed or persisted.The UI tells the truth
generationcounter moves; polling pauses on a hidden document.Hardening
sourceBudgetMsdefault raised 30s → 120s for large vaults on cloud storage.Test Plan
npm testexits 0, zero failuresindex-stability-test.sh(churn, rename, quarantine) andgraph-latency-test.sh(responsiveness SLO + graph budget). Both include guards that fail loudly if the fixture is too small to gate the bug — at the size originally planned, the churn assertion passed with the bug present.index-lifecycle-test.shcovers per-layer keys, snapshot handoff, policy re-keys, dotted directories, bounded refreshnpm run smokeOK.obsidian/churn), driving the actual console UI against the actual engineMeasured before → after:
/api/graphon a 139MB vaultNotes
snapshotSourceawaits real async I/O per document, so the loop is released every document, not every 25. A planned worker-thread rewrite was demoted rather than built, because it would have hidden a cost that should be deleted.YIELD_EVERYis untouched.kindcould shadowidentity.kindand make two different sources share one index snapshot, serving one source's documents under another's name and precedence level./api/graphpinned its payload at one instant but computedgenerationafter the await, so a client could store a counter describing data it never received and then never refetch; and the console committed its poll gate before the heavy refetch, so a failed refetch reported success and froze the page. Both are fixed and gated by tests..obsidianwatcher filter was ungated — deleting it left both new suites green, because the coalescing fix alone satisfied them. There is now an assertion that fails if the filter is removed.packages/coreremains dependency-free. Resolver behavior and the MCP tools baseline are unchanged.