Archive-support blockers: never bind a source silently to the wrong document - #16
Merged
Conversation
…ssifier, abort-aware)
Eight tasks, each shipping a test that fails first — the branch's eight existing gates all pass, so green tests are not evidence here. Design decisions recorded: session-scoped sidecar for change_source persistence (a manifest would silently convert the session to a named collection and appear to lose its memory and data); a sourceDir->dataKey cache in the host rather than duplicating dataKeyForRef/toSlug/deriveRef across a package boundary; an explicit collection id separate from the display name; and timeout-as-abort inside completeWithRetry, since signal is the one mechanism every provider honours. Scope also absorbs the two issues the timeout fix makes reachable: the missing try/catch (Google throws when the signal is pre-aborted) and the timeout-vs-user-abort distinction (without it, retries silently become 0 for timeouts).
Pre-flight scan found the plan violating its own failing-test-first constraint in three places. Task 2's canary asserted a literal true where the real property is 'it terminated' — now a watchdog race. Task 3's test was specified to PASS before the fix, leaving blocker 1 with no failing test — now extracts a pure outputBaseDir so the inherited-source case is directly assertable. Task 1 is explicitly exempted, changing only text. Also folds in three Minor findings the previous SDD run recorded and never fixed, all in files this plan already touches.
setup.md ended with a literal <<<< that renders in the VS Code Getting Started walkthrough. workspace-templates.ts wrote 'design for resa resume' into every new workspace's SKILL.md. Also drops the two 0-byte MEMORY.MD files committed by dev-running the agent inside the repo, and ignores those paths so they cannot come back. Folds in two Minor findings the previous SDD run left unresolved: the task_batch source description claimed source is unused with images (it is forwarded, enabling that item's view tools), and loadImageAsPng duplicated downscaleToLimit's resize options literal.
The Google and Vertex providers never reference timeoutMs and honour only signal, so the 300s budget was a silent no-op on Gemini — a stalled expert held its batch slot forever and retries could not help, because the loop blocked on an attempt that never resolved. Each attempt now gets its own AbortController composed with the user's cancel signal. A timed-out attempt stays retryable (the loop tests the user signal, never the composed one) and is reported distinctly from a user cancel. Attempts that THROW instead of resolving are also retried and converted to an error response, since Google throws when the signal is pre-aborted — previously that would have escaped uncaught.
A timeout racing a genuinely successful attempt could discard the real response: runAttempt read the timer-set timedOut flag after await attempt(...), so a call that legitimately resolved right at the timeout boundary was still reported as timed out and retried. A usable response (stopReason neither "error" nor "aborted") is now authoritative over a late-firing timer, both in runAttempt/isRetryable and in expert-turn.ts's reporting order. Also added the missing synchronous already-aborted pre-check for the user signal (adding an abort listener to an already-aborted signal never fires, per WHATWG semantics), hardened the canary's user-abort mock to actually derive its result from the signal it receives instead of hardcoding stopReason "aborted", added a reset assertion to the timeout-then-success check, added a new check for the timer/success race, and restored the dropped chronos.expertRetries / chronos.expertRequestTimeout settings cross-reference comment.
Running the UI test with the repo root as cwd produces a root-level .vscode-test/ that the chronos-vscode-scoped rule did not cover, leaving a multi-hundred-MB VS Code download exposed to an accidental commit -- the same class of artifact as the MEMORY.MD files just removed.
A task_id follow-up inherits the session's source for viewing, but the output dir was chosen from params.source alone and fell back to the workspace root. Re-running an extraction wrote the file outside the source's data dir, left the original stale, and reported success.
Task 3 fixed output_file's blind spot to a task_id follow-up's inherited source but left details.source/the @path citation keyed off params.source alone. A sourceless follow-up still self-zooms via view_page/view_region against the inherited source, so its toolUses entries carried real page numbers under an undefined details.source — clicking that citation chip in the webview fell back to whatever source the panel currently has open, not the one actually viewed. Extract effectiveSourceRel (mirrors outputBaseDir's explicit-wins-over- inherited rule) and derive details.source/sourceRel from it instead of params.source directly.
…4, R5)
R4: the newest collection-canary.mjs block asserted a literal
"sources/Frankfurt_1864" for effectiveSourceRel, which relies on
node:path's relative() and returns platform-native separators
(backslash on win32) — three checks would false-fail there. Compose
the expectation with join("sources", "Frankfurt_1864") instead,
matching the older outputBaseDir block's convention of deriving
expectations from fixture primitives rather than literals.
R5 (subsumes R3): outputBaseDir and effectiveSourceRel each repeated
the explicit-wins-over-inherited precedence rule and an identical
broad `catch { return ""; }`. Extract the precedence rule into a
shared effectiveRef() helper, and the catch into a shared
resolveOrEmpty() helper, without merging the two functions (their
no-source fallbacks genuinely differ: ctx.workspaceDir vs "").
The catch stays an unconditional catch-all rather than narrowed by
error type: a future ambiguity error from resolveByAlias is never
silently lost end-to-end even though these two helpers swallow it,
because both call sites in createTaskTool sit adjacent to
runExpertTurn's own independent resolveSource call, which surfaces
whatever it throws as result.error. Documented this reasoning in
resolveOrEmpty's doc comment so the ambiguity-throw task isn't
surprised by it.
resolveByAlias returned the first member whose basename matched, so two nested sources sharing a basename were indistinguishable — the agent silently got one and wrote its extraction into that source's data dir. Mirrors resolveExpertModel, which already errors on an ambiguous id.
buildCollectionFromDiscovery clears and repopulates from sources/ on every session_start, so an out-of-tree source added via change_source was wiped with nothing to restore it from — refs the agent had been told to use started throwing mid-conversation. Additions now persist per-session in session-collections.json and are replayed after discovery. Selecting 'All sources' clears only the collection name, not the additions. Deletes session-source-store.ts, which had no importers left.
…, R2) - R1: /select-collection wiped extra members added via change_source in both its success branches (all-sources and named-collection), reproducing the exact bug Task 5 fixed for session_start. Extracted the replay loop into an exported replayExtraMembers() helper and call it at both sites. - R2: saveSessionExtraMember now guards against a non-array extraMembers in a hand-edited/corrupted store instead of spreading a string into characters or throwing on an object. - Extended collection-canary.mjs with checks for both (33 checks total).
My pre-flight scan checked only tasks 1-3 against the plan's own failing-test-first constraint. Task 7's UI-test step said 'add an assertion' with no step that runs it before the fix, so it could have shipped a test that passes either way.
…ry check
replayExtraMembers only depends on collection-context/session-collection-store,
so move it from extensions/index.ts to tools/collection-context.ts (no import
cycle: session-collection-store.ts imports only node:fs/node:path) - this stops
the canary from needing to load the full pi-package entrypoint to exercise
twelve lines (R4).
The "already-present ref is not overwritten by replay" canary check asserted
identity on a ref ("InTree") that was never actually reachable from
extraMembers, so it passed even with the has(ref) guard removed. Persist an
extra member whose deriveRef genuinely collides with a discovered source so
the guard is actually exercised (R5).
Replace the dead void'd archiveAfterFirstReplay capture with a real assertion
that a second replay-after-wipe reconstructs the same member fields (R6).
listCollections reported the in-JSON name and discarded the filename, then loadCollection resolved that value AS a filename — so a manifest whose name differed from its filename was unselectable, and restore failed the same way via a console.warn the user never saw. Adds an explicit id (the filename stem) used as the option value, the manifest lookup key, and the persisted value; name stays display-only. Migrates already-persisted display names to ids on load.
collectionKey derived its slug from the mutable display name, contradicting the id-as-stable-identity guarantee Task 6 established — renaming a manifest's "name" field silently relocated its entity index and memory file. Key on id instead, with the "all-sources" fallback preserved. Also corrects Selection.name's doc in session-collection-store.ts: the field has held a collection id since Task 6, not a display name, but the on-disk JSON key stays "name" for backward compatibility with existing session-collections.json entries and the legacy-read canary. Extracts resolveSessionCollectionSelection, a pure decision function, out of session_start's previously untested display-name migration branch, and adds canary coverage for it plus for the collectionKey fix.
The host derived basename(sourceDir) in previewSource/openViewLink while
the agent slugs nested refs (dataKeyForRef), so a citation click for
sources/city/X pointed the Data tab at data/X instead of data/city--X —
it silently emptied and latched the wrong value. The dropdown's
endsWith("/" + currentSource) matcher was likewise written for
basenames and stopped matching once currentSource became a slug.
Caches the data key the agent already sends as sourceName on every
show_page/page_list HTTP message, keyed by sourceDir, instead of
duplicating dataKeyForRef/toSlug/deriveRef across the chronos/
chronos-vscode package boundary (show_text doesn't carry sourceDir on
the wire, so it's left setting currentSourceName directly as before —
already correct since it comes straight from the agent). Citation
clicks now look up that cache (falling back to basename only for a
directory the agent has never named) instead of re-deriving it, and
the source dropdown compares a same-space dataKey sent alongside each
option instead of an endsWith hack.
Extends the UI test fixture with a nested source and a citation that
names it explicitly, and asserts the resulting currentSource is the
slug, not the directory basename.
…/F3) The host's dataKeyBySourceDir fallback used basename(sourceDir) whenever the agent hadn't yet named a directory this session (resuming a session, or citing a nested source the agent never show_page'd). That's actively wrong for nested sources: the agent slugs sources/city/X to data/city--X, so the fallback silently pointed the Data tab at a directory that doesn't exist. This made the dropdown's dataKey (postSources) permanently wrong for a cold nested source too. Add chronos-vscode/src/panel/data-key.ts, a dependency-free mirror of the agent's own deriveRef/dataKeyForRef/toSlug (chronos/tools/collection-context.ts, chronos/utils/source-discovery.ts), and use it as the fallback instead of basename. The agent's cached value still wins when present; this only covers the cold-cache case, correctly, for in-tree flat, in-tree nested (any depth), and out-of-tree sources alike. With a correct fallback, postSources() already computes the right key on a cold cache, so no re-fire is needed there. Add test/data-key-equivalence-test.mjs, which imports the agent's real compiled functions from chronos/dist and the host's mirror (compiled on the fly with esbuild) and asserts they agree across a table of cases, with no hardcoded expected key — this is what keeps the duplication honest if the agent's derivation ever changes. Also replace suite.js's hardcoded "city--Nested_1900" literal with the same dynamic derivation, and add a genuine cold-cache regression check (a second nested fixture source, sources/city/Nested_1875, that the mock pi never mentions).
…y (F1) buildCollectionFromDiscovery keyed collection members by discoverSources' platform-native s.name instead of routing it through deriveRef, which is the one place that normalizes \ to / -- on win32 this silently collapsed distinct nested sources with the same basename onto one data/ dir and left the agent's reported sourceName disagreeing with the host's already- normalized deriveDataKeyFallback. Fixed both sides: the agent derives the ref via deriveRef(workspaceDir, s.path), and chronos-vscode's discoverSources normalizes its own name the same way so /select-source's exact-ref match still works. Added a collection-canary.mjs case that reproduces the collision class of bug without needing Windows (a literal-backslash-named POSIX directory exercises the real buildCollectionFromDiscovery, and a synthetic win32-style ref pair fed into dataKeyForRef demonstrates the collision directly), plus a data-key-equivalence-test.mjs case sourced from chronos-vscode's actual discoverSources output rather than a hand-built path.
…d (F2) pi's fork (VS Code's "edit a past message") mints a brand-new session id, so loadSessionExtraMembers/loadSessionCollection for that id found nothing -- every change_source addition and collection narrowing silently evaporated on edit-and-resend, exactly the failure this store exists to fix for startup/switch/resume. session_start's previousSessionFile is a file PATH, not an id, so read the old session's id out of its own header (sessionIdFromFile) and copy its collection selection + extraMembers to the new id (carryForkedSessionState) before the rest of session_start runs. This makes the "startup/switch/resume/fork" doc comments in collection-context.ts and change-source.ts actually true instead of aspirational.
…name (F5) members.find((m) => m.ref === requested || basename(m.path) === requested) took whichever member matched EITHER condition first in sort order — with members "a/X" and "X", "a/X" sorts first and its basename matches, so /select-source X previewed the wrong document even though an exact ref "X" exists. Changed to two sequential finds (exact ref, then basename) so an exact ref always wins, matching resolveSource's precedence. Also corrected resolveByAlias's doc comment, which claimed to mirror /select-source's match exactly -- it mirrors precedence now, but still differs on an ambiguous basename (resolveByAlias throws; /select-source silently takes the first sorted match).
explicitSource ?? inheritedSource only falls through on null/undefined, so an explicit source: "" was treated as a real (empty) source rather than "absent", ignoring an inherited task_id follow-up source and writing output_file/details.source to the workspace root instead. Changed to ||.
Collections have never shipped -- zero collection files exist on dev or master -- so the display-name-to-id migration in resolveSessionCollectionSelection guarded a population of zero sessions, and it was the sole cause of a real hazard: a stored value that happens to equal a DIFFERENT collection's display name would silently resolve to the wrong collection, strictly worse than the pre-existing safe fallback to all-sources. Removed: - resolveSessionCollectionSelection's needsRewrite/display-name-resolution branch and its store-rewrite wiring in session_start. Kept the function itself as a pure id validator -- it's still meaningfully testable in isolation from the imperative load/rewrite side effects, matching the rest of this file's testing approach. - /select-collection's fallback that matched a requested value against c.name after c.id. Matches on id only now. The pre-existing silent fallback to all-sources when a stored id matches nothing is unchanged. Updated the R18 canary block to drop the deleted migration's assertions and added the hazard case as a passing check instead (a stored value equal only to a different collection's display name must stay on the auto-collection, not resolve to that collection).
… F6) test/suite.js and test/data-key-equivalence-test.mjs import chronos/dist to derive their expected data keys instead of hardcoding them, but `npm test` never built chronos/ — so on a clean checkout the import failed with an opaque ERR_MODULE_NOT_FOUND from inside the extension host, and with a stale dist the drift detectors agreed with themselves. - chronos-vscode: add build:agent; npm test now builds the agent, then runs the two host tests as well as the UI test. - chronos: add npm test running all four canaries (after a build, since they import the build output). - suite.js: name the missing build in the error instead of throwing a raw module-resolution failure. - Document all six test scripts (only two were listed), the stale-dist hazard, and the UI test's measured ~1-in-4 flake. - CLAUDE.md workspace layout: session-sources.json no longer exists; describe session-collections.json and its extraMembers replay. - Stop telling the model to construct data/_collections/<collection>/ itself — the resolved dir is injected into its system prompt.
Final-review wave 2. One functional defect, one regression this branch
introduced, one long-standing critical, and the fixes whose tests passed with the
fix removed.
change_source silently bound the agent to the WRONG document (critical). An
out-of-tree archive derives its ref from the basename, so /mnt/archive/F_1864
collided with an in-tree sources/F_1864: the add was a no-op while the tool still
reported success WITH THE ARCHIVE's page count and path, after which every
list_pages/task read the other document and both wrote into one data/ dir.
resolveByAlias — which throws on an ambiguous basename — was never reached,
because the collision happens before any lookup. It now refuses the add and says
which directory owns the ref, before any side effect (no mkdir, no sidecar write,
no viewer event). replayExtraMembers cannot refuse, so it warns instead of
skipping in silence.
The tool also now resolve()s its input, so one directory has one spelling. The
collision check is a string compare against a stored member path: a trailing
slash made "/a/S/" look like a different directory from "/a/S" and refused an add
whose named "owner" was that very directory. And basename("/a/S/.") is ".", which
produced ref "." and pointed a source's data dir at the workspace data/ ROOT
while reporting success.
deriveRef dropped a source from the catalog (regression, from the F1 commit).
Normalizing `\` unconditionally treats it as a separator on POSIX, where it is a
legal filename character: a dir named `city\Nested` folded onto the ref of a
genuinely nested `city/Nested`, and since members is keyed by ref, one of the two
silently vanished. Reachable from a Windows-made ZIP. Now only the PLATFORM
separator is translated, via a shared refFromRelative that takes the separator as
a parameter — which also makes the win32 branch executable from a POSIX host,
where the host half of that fix had no coverage at all. resolveByAlias folded `\`
the same way on its lenient `sources/`-prefixed path, resolving that spelling to
the wrong document; it now uses the same helper.
The explicit-vs-inherited source rule was half-fixed. expert-turn.ts still used
`??`, so `task({task_id, source: "", page_id})` resolved an output dir from the
inherited source while the turn itself failed "page_id requires a source" — and
without page_id the expert silently lost view_page/view_region. Both sites now
call one exported effectiveSourceRef, so the docblocks promising they cannot
disagree are structurally true rather than aspirational.
sessionIdFromFile read the ENTIRE session transcript while its docblock claimed
it read only the first line. Past Node's 512 MiB string limit readFileSync
throws, the bare catch swallows it, and the fork carry-forward silently loses
everything — on exactly the long sessions it exists to protect. Now a bounded
64 KiB header read (a header above that fails closed; pi's are ~200 bytes).
Coverage: /select-source's precedence and the fork guard were inline in the pi
entrypoint's closures, so the canaries asserted about retyped copies and stayed
green when the real code was reverted. Both are now exported
(pickMemberByRequest, forkedPreviousSessionFile) and the canaries drive those
functions. Note this covers the HELPERS, not the wiring: re-inlining the buggy
expression at the extensions/index.ts call sites, or deleting sources.ts's
normalization, still leaves the suites green. Asserting the wiring needs a
separator seam in discoverSources and is recorded as a follow-up.
Each fix is mutation-verified: reverting it turns canary checks red — the
deriveRef fold, the resolveByAlias fold, the dropped resolve(), effectiveSourceRef
back to `??`, the change_source refusal, a truncated header read, the
pickMemberByRequest single pass, and the host-side normalization (which formerly
left the equivalence test fully green when deleted).
Gate: 142 agent checks, 52 host checks (30 equivalence + 3 collection-id + 19
UI), all three typechecks green.
Deferred (in the ledger; they rename data dirs or need design): toSlug is not
injective so two distinct sources can still share one data/ dir; deriveRef's ".."
test is a substring rather than a path-segment test; a manifest-supplied `ref` is
still unnormalized.
Plans are working notes for one change. They go stale the moment their branch lands, and they dominated the diff they belonged to: the archive-support blocker plan was 1435 lines, the single largest file in its own PR, and plans together were 3004 of that PR's 8502 added lines — more than three times the source it actually changed. Specs (docs/superpowers/specs/) stay tracked. They record WHY a design was chosen, which stays useful after the branch is gone; the plan is just the step list for getting there. The files remain on disk, and their content stays in history — nothing is lost, it just stops arriving in review.
The UI-test job installed only chronos-vscode's dependencies, so `npm test` failed at its new first step — building chronos/ — with ~60 TS2591/TS2307 errors for missing @types/node and the pi packages. Reproduced locally by moving chronos/node_modules aside: byte-identical failure. That build step is not optional: test/suite.js and test/data-key-equivalence-test.mjs derive their expected nested-source data keys FROM chronos/dist instead of hardcoding them, so a missing dist fails opaquely inside the extension host and a stale one makes both agree with themselves. The job now installs the agent package and caches its lockfile too. Also: nothing in CI ran the agent's canaries — image downscaling, expert retry, timeout-as-abort, and the whole collection/ref/data-key/session-sidecar suite, ~140 assertions, were local-only. The build job runs them now. Fixes the stale comment claiming `npm test` is just esbuild + run-ui-test.mjs.
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.
Closes the blockers that stood between the archive-support work and
dev, plus everything three review rounds turned up in the fixes themselves.Why this branch exists
The unifying theme is that a source must never resolve to the wrong document silently. Several paths violated that, and two of them were introduced by earlier commits on this very branch.
The blockers (Tasks 1–7)
timeoutMs— the Google/Vertex providers ignoretimeoutMsentirely and honour onlysignal, so an abort is the only mechanism that bounds every provider.output_fileon atask_idfollow-up follows the inherited source instead of falling back to the workspace root.change_sourceadditions survivesession_start(startup/switch/resume/fork) and/select-collection, via a session sidecar.basename(sourceDir).What review found, and this fixes
change_sourcebound the agent to the wrong document. An out-of-tree archive derives its ref from the basename, so/mnt/archive/F_1864collided with in-treesources/F_1864: the add was a no-op while the tool reported success quoting the archive's page count and path. Every page tool then read the other document, both writing into onedata/dir. It now refuses before any side effect and names the directory that owns the ref.\unconditionally treats it as a separator on POSIX, where it is a legal filename character, so a dir namedcity\Nestedfolded onto nestedcity/Nested's ref and one of the two vanished. Reachable from a Windows-made ZIP. Only the platform separator is translated now.source: ""on a follow-up resolved an output dir from the inherited source while the turn itself failed"page_id requires a source". Both sites now call one shared function.sessionIdFromFileread the entire transcript while its docblock claimed otherwise; past Node's 512 MiB string limit it throws, the bare catch swallows it, and the fork carry-forward silently loses everything. Now a bounded 64 KiB header read./.spellings of one directory no longer produce a false refusal or a source whose data dir is the workspacedata/root.Testing
npm testin both packages: 142 agent checks, 52 host checks, all three typechecks green.npm testpreviously never built the agent package whose build output the tests derive their expected values from — a clean checkout failed with an opaqueERR_MODULE_NOT_FOUNDinside the extension host, and a staledistmade the drift detectors agree with themselves. It now builds it first, and all six test scripts are documented and wired (only two were listed before).Every fix is mutation-verified: reverting it turns canary checks red. That mattered — three fixes originally shipped with tests that passed with the fix removed, because the code under test was inlined in the pi entrypoint's closures and the canaries asserted about retyped copies.
Known limits, deliberately not addressed here
extensions/index.tscall sites, or deletingsources.ts's normalization, still leaves the suites green. Needs a separator seam indiscoverSources.toSlugis not injective, so two distinct sources can still share onedata/dir (city/Adressbuch 1864vscity/Adressbuch_1864). Fixing it renames data dirs and would orphan existing workspace output, so it wants a migration decision.deriveRef's".."test is a substring rather than a path-segment test (a parent named..citydrops a source).refbypasses normalization — the one input shape where the two packages genuinely disagree. Unreachable today; collections have never shipped.Merge as a real merge commit, not a squash, to keep the per-task history and its review trail.