fix(workspace): scope the binding cache and skill snapshot to the account

[saravmajestic]

The bug

The binding cache and the workspace skill snapshot were both keyed on tenant|apiUrl. Two accounts on one tenant therefore shared them.

User A links a project to their private workspace, and its skills land in .altimate-code/skill/_workspace. The credentials are then switched to user B on the same tenant — a shared analytics box, a service account, or /connect with a colleague's key while pairing. Within the validation window, B's sessions read A's cached binding and load A's private workspace's skills, with no visibility check of their own. On the server a private workspace is visible only to its owner, so B should never have received any of it.

The identity memo was already per account, and the pin validation in state.ts already scopes on a credential digest for exactly this reason. The binding cache and the skill snapshot were the parts still keyed on the tenant alone — which is how accountScopedKey and accountKeyOf both came to be named for an account neither of them included.

The fix

• state.ts — the scope carries the credential digest, the cache file records which account wrote it, and a file belonging to another account is neither read nor appended to. CACHE_VERSION 1 → 2: a v1 file cannot be attributed to anyone, so it is discarded rather than migrated. The cache is an offline fallback, so the cost is one re-validation per project.
• skill-sync.ts — the manifest and the in-memory sync stamp carry the account, and a snapshot written by another account is deleted. Ignoring it would leave another user's private skill content sitting readable in a shared checkout.
• src/skill/index.ts — and the read side does not depend on that deletion having happened. Discovery serves a match under .altimate-code/skill/_workspace only when that project's manifest carries the current credential's digest. See below: the purge alone was not enough.
• One definition each for the digest and for the scope string. identity.ts was rebuilding that string by hand, so it compared a two-part value against a three-part one and rendered every cached binding as unknown — caught by an existing test, and the reason the builder now lives in one place.

Why the purge alone was not enough

An earlier revision of this description said a foreign snapshot "is deleted before anything loads it". That was wrong, and review caught it. Three paths get around the purge:

• The prompt path refreshes the skill registry before it polls (session/prompt.ts), so a stale snapshot is loaded on the very turn that goes on to delete it.
• Two processes sharing one checkout have no ordering between them at all: A can republish _workspace after B's purge.
• Within one process, inFlight is keyed on the directory, so a caller who switched credentials mid-sync joins the other account's run.

A later re-sync repairs the disk but cannot retract what already reached a prompt. So discovery asks the question itself — one manifest read per project, not per skill file — and fails closed: unattributable, unreadable, and unreadable-credentials all withhold. Withholding costs a poll interval; serving another account's private skill cannot be taken back. A project that never used the feature has no managed path among its matches, so the module is not even loaded — the same shape as the opt-out gate in session/prompt.ts.

inFlight stays keyed on the directory rather than on directory + account. Keying it per account would let two credentials write one tree concurrently — the thing the gate exists to prevent — and it cannot help the cross-process case anyway. With reading gated on its own, a joined run's stale changed flag is no longer a disclosure, only a wasted poll; the comment at the join says so.

The trade-off, stated plainly

A credential change re-validates instead of inheriting. That includes rotating your own key, which cannot be told from a different user by the key alone. The cost is one server round trip per project, and no cached binding while offline until it succeeds.

Stated precisely, because an earlier draft understated it: there is one bindings file, and it records the single account that wrote it. So a write by another account does not merely shadow the previous one's row for that project — it replaces the file, evicting every project's row. Alternating between two accounts therefore re-validates each project on each switch, not once.

This is worth calling out because the repo has been bitten by the other side of it: skill-publish.ts deliberately moved the published-id ledger away from key-digest scoping, because a rotation orphaned every published skill — "the user is the identity". That reasoning is right for the ledger, where the id belongs to a person. It is the wrong way round for the cache, where the question is whether this credential may see that workspace. Re-homing the cache after a server confirmation (the pattern the ledger uses) would remove the re-download and the offline window; it is a follow-up, not a blocker.

Verification

bun test test/altimate/workspace/ test/skill/skill.test.ts — 791 pass, 1 fail. The one failure is flushPendingSyncs waits for a sync a short-lived process would abandon, which fails identically with main's sources in place (measured, not assumed). Typecheck clean for every file touched.

Full suite, since this change now touches src/skill/index.ts: 14,251 pass of 15,121 across 717 files, with an identical set of 11 pre-existing failures before and after this commit — the same mcp.headers, HttpApi and flushPendingSyncs timeouts, name for name.

New tests, in state-account-scope.test.ts and skill-sync.test.ts:

Test	Pins
a second user on the same tenant does not inherit the first user's workspace	the reported bug
the first user still reads their own binding after switching back	that the fix is not "reject everything"
a write by the other account evicts this one's rows entirely	that a write does not file B's row under A, and what the eviction actually costs
a cache file from before accounts were recorded is discarded	v1 is not trusted
the version bump alone rejects an older file	the format change itself, not just the missing field
the digest identifies the whole credential, not just the key	host and tenant are part of the account, and the key never appears
switching to another user on the SAME tenant drops the first user's skills	the snapshot half
a snapshot from another user is dropped even when the new user is bound	that the drop is the account comparison, not the unbound path
a snapshot another account fetched is not discovered	the read side, independent of whether the purge won the race
a snapshot whose manifest predates accounts is not discovered	that unattributable is withheld, not served to whoever is logged in
an accountless v2 manifest is not ours to delete	that ownership still requires attribution, so a hand-written file is not handed to a recursive delete

Mutation testing

Mutation	Caught
the cache read ignores the account	yes — 3
writes append to another account's file	yes — 1
the snapshot purge stops comparing accounts	yes — 1
the validator's account field check removed	no — see below
the second (foreign) account comparison removed	no — redundant with the purge above

Two survive, and both are honest rather than gaps. The validator's field check is shape validation that keeps the raw is CacheFile predicate truthful for a hand-edited v2 file; the version check is what rejects older files, and the account comparison in readCachedBinding is the security control. The foreign comparison's account clause is a second layer behind the purge that already fired — its datamateId clause is separately covered.

My own tests also caught a bug I introduced: writes were appending to a file owned by another account, so B's row landed in A's file and then failed B's own read.

Not in this change

The ticket's comments name two more instances of the same class, both needing a credential threaded through a call chain rather than a scope key: an account switch during an in-flight memory backfill, and /workspace → "Open in browser" resolving the workspace id and the URL under different credentials. Left as follow-ups so this stays reviewable.

🤖 Generated with Claude Code

---

Summary by cubic

Fixes a bug where the binding cache and workspace skill snapshot were keyed only on tenant|apiUrl, letting a second account on the same tenant read the first account's cached binding and private workspace skills with no visibility check.

• The cache file and snapshot manifest now record the credential digest of the account that wrote them; foreign or unattributable content is neither read, served, appended to, nor kept, and a foreign snapshot is deleted.
• Discovery fails closed: a snapshot is served only when its manifest carries the current credential's digest, with attribution checked on both ends of a symlink, unresolvable paths withheld, and projects at the filesystem root handled without special-casing.
• CACHE_VERSION bumps to 2 and the manifest to version 2; v1 files are discarded rather than migrated, while an upgraded v1 snapshot is replaced so skills refresh instead of being stuck.
• One digest definition and one scope-string builder now back the cache scope, the offline attach path, and the pin validation memo; registryStale, the sync stamp, and the sidebar's synced age all carry the account.
• A sync whose credentials change mid-download is rejected before anything is swapped into place, an unlink no longer drops another account's row, and a keyless credential purges nothing.
• After merging main, snapshotProjectOf now lives in the shared snapshot-path.ts module instead of duplicating the segment walk, keeping the collection and filter paths from drifting apart.

Trade-off: switching credentials — including rotating your own key — now re-validates instead of inheriting, at the cost of one server round trip and no cached binding while offline.

^{Written for commit 5a02c4c. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes
  • Workspace bindings, skill snapshots, and sync status are now scoped to the account’s credentials, keeping data separate even when accounts share a tenant and API URL.
  • Switching accounts no longer reuses another account’s cached workspace data or sync status. When syncing, snapshots from another account or older formats are replaced or removed.
  • Cached bindings are accepted only when they match the current account. Account-scoped skill syncing requires a valid account identity.

Self-review (ef4d253)

Reading the change as a whole rather than the last diff. This bug existed because two helpers were named for an account neither of them included, and the fix had left three more ways to be misled:

• tenantKey() now returned an account-scoped key — the same misnomer, just moved. Renamed accountKey().
• credentialDigest's doc had been orphaned onto scopeStringOf by the edit that introduced it, leaving the digest undocumented and the scope string described as a digest.
• CacheFile.account was documented as a "short digest of the API key" — it is the whole credential, unshortened, and the file header said the same.

state.ts also still computed a second, different digest inline for the pin validation: two answers in one file to the question this change is about, and the reason the cache and the validation beside it were scoped differently to begin with. The pin memo is in-memory, so its key format is free to change; it now uses credentialDigest like everything else, and createHash appears once in the file instead of twice.

Also confirmed during the pass:

• memory-index.ts was already account-scoped, so it is not another instance of this bug.
• The other hand-built scope strings (identity.ts, skill-publish.ts) are their own memo keys, not comparisons against readLocalBindingScoped's scope, so they are not at risk of the drift that broke identity.ts.

765 pass / 1 pre-existing fail, typecheck clean, tracker-leak check clean.

Self-review (71ebd25)

Read as a whole feature rather than as the last diff, the question this round was: the PR says the snapshot is deleted before anything loads it — is that true? It is not, and every claim that rested on it has been corrected above rather than quietly dropped.

• Traced the actual load order instead of assuming it. session/prompt.ts calls refreshRegistry() before recentlySynced/syncSkills, so on the flag-on path a foreign snapshot is loaded first and deleted second. src/skill/index.ts contains no manifest reference at all. The reviewer was right on both counts.
• Fixed the read side, not the ordering. Moving the purge earlier would have closed one of the three paths and left the cross-process one open, while adding an await to a turn-ordering-sensitive path that has already regressed a test once (documented in that file). Gating the read closes all three.
• Skill.dirs too, not just all(). The withheld set is rebuilt from the surviving matches rather than filtered out of state.dirs — dirs feeds the prompt, so a withheld skill leaving its directory behind would have been a half-fix. There is a test for it.
• Mutation-tested both new guards, and verified each mutation applied before trusting the result: removing the discovery filter fails the two new discovery tests; removing the accountless-v2 guard fails its test. Each was confirmed by grep, not assumed from the edit.
• Checked the cost. The fixture test a workspace-synced bundle layout is discovered as a real skill had no manifest and would have started failing — it now writes one, which is itself evidence the gate bites. No other test in 717 files changed behaviour.
• Regression-checked the whole suite, not just the touched directories, because src/skill/index.ts is on every session's path: the 11 failures are the same 11, name for name, before and after.

Honest residue: inFlight is still keyed on the directory, deliberately, and the reasoning is in the code at the join rather than left implicit — a per-account key would let two credentials write one tree at once and still would not help two processes.

Self-review (1ce2878)

The gate added in 71ebd25 was reviewed and three ways around it were found. All three were real; each is fixed with a test, and each test was verified by a mutation that was confirmed to have applied before its result was trusted.

• Symlinks. scan follows them, so a link from any other scanned skill directory into _workspace produced a match with no managed component in its own path. Attribution is now decided on the real path. Two tests, deliberately a pair: a link into a foreign snapshot is withheld, a link into our own is still served — so this withholds what is foreign rather than everything it cannot recognise at a glance.
• A project at /. at > 0 treated index 0 as "no project". Both the collection loop and the filter now call one snapshotProjectOf, so the two cannot drift apart; it is @internal-exported because a project at the filesystem root cannot be staged as a fixture.
• The cached registry. This was the one I had missed, and the likeliest to fire: registryStale keyed on the manifest mtime, which an account switch does not move, so discovery never re-ran and the gate was never asked. The registry identity now carries the account. The test also asserts that switching back is not stale, so this refreshes on a real change rather than refusing to settle.

Regression evidence, since this commit touches session/prompt.ts as well: full suite before and after, same flaky clusters and nothing new. The mcp HttpApi and session/loop failures reproduce identically at the parent commit in isolation (3–5 of them, varying per run, all 5s timeouts), and flushPendingSyncs reproduces with main's sources in place.

Honest note on what is not closed: the check-then-load window cubic raises is still a window — snapshotIsOurs answers, then the files are read. It is far narrower than the unchecked read it replaces, and closing it properly means reading a snapshot and its manifest as one immutable unit, which is a bigger change than this PR. It belongs with the other in-flight credential-switch items under "Not in this change".

Self-review (2291a2ea) — the gate, reviewed as its own feature

The discovery gate was added in 71ebd25 and then reviewed as hard as the rest of the PR. Five ways around it were found, all real, all fixed, each with a test whose mutation was confirmed to have applied:

Bypass	Why it worked	Now
A symlink into _workspace from another scanned skill directory	the matched path carried no managed component	attributed on the resolved path
A symlink out of _workspace to a file elsewhere	the resolved path carried no managed component	both ends checked; a match touching any managed project needs every one of them to be ours
A path realpath could not resolve	fell back to the matched path, which answers "ordinary skill"	withheld — what cannot be resolved cannot be attributed
A project opened at /	at > 0 read index 0 as "no project"	one snapshotProjectOf, which returns /
A credential switch with the snapshot untouched	registryStale keyed on mtime, so discovery never re-ran	the registry identity carries the account

The third and fourth of those were mine, introduced by the fix for the first. That is the honest shape of this round: each narrowing moved the boundary and the next review found what the move exposed. The symlink pair is now pinned from both directions independently — gating on either end alone fails the other end's test — which is the property that says the boundary is checked rather than merely moved again.

One more, unrelated to the gate and found in the same round: the pre-binding purge compared the manifest's account against a digest that is null whenever no API key resolves, so every valid snapshot looked foreign; the sync then failed for the same missing key and put nothing back. It is guarded on an account it could actually compute, the rule the disconnect and lookup-failure branches beside it already follow.

And one test that was not testing what it claimed: a snapshot from another user is dropped even when the new user is bound bound the second user to a different workspace id and served a different skill set, so an ordinary "not up to date" re-sync produced the same observable outcome — it passed with every account comparison removed. Same workspace id, identical listing, and it asserts the manifest's account changed hands.