Repository navigation
fix(parser): avoid marking partially scanned caches complete (#912) - #1229
ozymandiashh merged 3 commits into
Conversation
Repro for the `parser.ts` candidate in getagentseal#912: a run filtered to one provider leaves every other provider unscanned and still marks the whole cache complete. Three lines in runParseInner tell it. Discovery is filtered — `discoverAllSessions(providerFilter)`. The loop over cached providers skips out-of-scope names — `if (providerFilter && providerFilter !== 'all' && providerFilter !== providerName) continue`. Then the stamp fires guarded only on readOnly / wasComplete / deferredForFirstPaint, with nothing about scope. That is worse than the silent zeros in getagentseal#874 and getagentseal#899, which lasted one run. This one is written to disk: once the cache is stamped, later launches stop coming back cold for those providers, so the gap stops looking like a gap. Test only, per the issue. It asserts its own premise first — that claude really was left unscanned — because otherwise the second assertion proves nothing. Mutation-sensitive, checked both ways rather than asserted: - as-is, the assertion fails with `expected true to be false` - adding `&& !scopedRun` to the stamp makes it pass - reverting makes it fail again Marked `it.fails` so the suite stays green while the defect is open. When the stamp learns about scope this test starts passing, vitest reports "Expect test to fail", and that is the signal to drop `.fails` and keep it as a plain regression test. Verified that flip too.
|
Thanks — the first invariant is reproduced, and I independently got the same flip at Before turning this into a fix, please tighten two things:
Please add either an assertion documenting that the next full run repairs the cache plus a real first-paint/snapshot repro demonstrating the transient mislabel, or another real caller-path repro that proves persistence. Once the scope is accurate, the conservative guard can come into this PR and |
Addresses review on getagentseal#1229. 1. Hermetic fixture. The scoped run is `parseAllSessions(undefined, 'codex')`, and Codex discovery falls back to CODEX_HOME / ~/.codex, so the test was reading the developer's real corpus. It now points CODEX_HOME at an empty temp root and restores the prior value (including unset) in teardown. 2. Claim narrowed. You are right that a subsequent all-provider refresh rediscovers the omitted provider, so "nothing re-derives them" was overstated. Dropped it. Added a test that pins the actual bound: after the scoped run, a plain `parseAllSessions()` repairs the cache and claude reappears. The mislabel is transient, not a persistent zero. The `it.fails` invariant is unchanged and still mutation-sensitive: guarding the stamp with `&& !scopedRun` flips it to a real pass (vitest then reports "Expect test to fail"), reverting flips it back. Ready for the guard to land here and `.fails` to become a normal regression test whenever you want to pull the fix into this PR.
|
Both addressed, pushed. 1. Hermetic. Good catch — the scoped run is 2. Claim narrowed — you are right, and I have dropped the overstatement. Your On the snapshot angle you offered as the alternative: I traced it and it is real — The |
|
Go ahead and bring the conservative scoped-run guard into this PR, remove it.fails, and keep the tests as normal regressions. The narrowed transient-impact claim is sufficient; no additional first-paint fixture is needed. Thanks. |
|
I implemented the guard, ran the full suite before pushing it, and it regresses four legitimate tests — so I have held it back rather than land a green-looking fix that breaks your snapshot path. Reporting precisely, because the blunt version is wrong and the reason matters. What breaksGuarding the stamp with
They all drive Why the simple guard is wrongThe two situations are genuinely different, and
The bug is not "a scoped run stamped complete." It is "a scoped run stamped complete while another provider had on-disk sessions it never scanned." When the scoped provider is the only one with data, stamping complete is right — which is exactly what your tests pin. The hard part: knowing an unscanned provider has sessions is precisely the discovery a scoped run skips for performance, so the cheap
That is an architecture call on your cache, and you know it far better than a first patch should assume. Which direction do you want? If (2), I am happy to sketch the per-provider flag and carry the snapshot logic with it. If (3), the branch already stands green as the reduced-claim regression tests you approved. I would rather ask than push a guard that quietly narrows what "complete" means for every other caller. (Full suite run locally, |
…plete A run scoped to one provider walks only that provider's sessions, yet the end-of-parse stamp marked the WHOLE cache complete regardless of scope (getagentseal#912). Because a complete cache stops being re-read as cold, the providers the scoped run skipped are never revisited — a wrong "done" written to disk, where the gap stops looking like a gap. Guard the stamp on real on-disk data, not on scoping alone: a scoped run still stamps the cache complete when every provider it skipped has no discoverable sessions (a single-provider machine — exactly what the warm-refresh snapshot tests rely on). Only when a skipped provider actually has sessions on disk is the whole-cache claim withheld. The extra check is a bounded directory walk, not a parse, so the scoping win holds. Drops it.fails on the getagentseal#912 regression; it now passes as an ordinary test.
|
Pushed the conservative guard ( Instead of gating on scoped-vs-
|
|
Friendly bump — this has been ready since the conservative guard landed in Recap of where it stands: the whole-cache Full suite is green. Nothing pending on my side. |
ozymandiashh
changed the title
|
Merged — thanks for the fix and for catching the snapshot regression in the initial guard. The focused tests and full CI are green. Appreciate the careful follow-through and your patience! |
A scan is scoped on two axes, provider and date range, but completeness was one whole-cache boolean. getagentseal#1229 stopped a scoped run from stamping it by adding a second, unfiltered discoverAllSessions to the stamp path, which left scoped users on a multi-provider machine permanently cold and paid an extra full discovery on every run. Completeness now lives on the provider section, with an optional completeFrom floor for the sources a date range filtered out. The whole-cache flag keeps its meaning (an unscoped scan finished) and is what a section with no flag of its own falls back to, so existing caches read exactly as before and no version bump is needed. Discovery reports which providers threw, and a provider that reported nothing because it failed is never marked complete.
2 participants
A provider-scoped parse could mark the whole session cache complete while another provider still had discoverable sessions that the run had not scanned. This change withholds the completeness stamp in that case, while preserving it on single-provider installations so warm-refresh snapshot fallback keeps working.
The additional discovery runs only for writable, previously incomplete scoped scans that did not defer files for first paint. It discovers sources across providers without parsing their full sessions; discovery may still read metadata or query databases. The demonstrated impact of the original bug is transient: a subsequent full refresh rediscovers the omitted provider and repairs the cache.
Adds ordinary regression tests for the omitted-provider case and recovery on a full refresh, with isolated provider roots. Addresses the provider-scoped completeness case in #912.
Validation at
0e8a379e5c5aacc2e6c3ac3fb8b0c6d6e240f9e0:main(eaed4240).