Skip to content

Commit 0a982df

Browse files
saravmajesticclaude
andcommitted
fix(workspace): fail closed on an unresolvable path, and stop purging on a keyless credential
Two more from review, both real. An unresolvable match was falling back to its own path, which answers "ordinary skill" — so an alias whose target was merely unreadable at that instant, such as during a concurrent snapshot swap, escaped the gate; if the target became readable again before the file was read, its content loaded with no account check at all. A path that cannot be resolved cannot be attributed, so it is withheld. 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, so nothing replaced what it had just deleted. The purge is now guarded on an account it could actually compute — the same rule the disconnect and lookup-failure branches already follow: unknown never destroys a snapshot. Also from review: - `state-account-scope.test.ts` redirected `Global.Path.state` through `XDG_STATE_HOME`, which is folded into a module-load-time constant and therefore only works if this file imports Global first — which a file sharing a worker cannot arrange. It uses `OPENCODE_TEST_STATE_HOME`, the getter-backed override, and asserts the redirect is in effect so the suite cannot silently pass against the real state directory. - The v1-upgrade test asserted the tree was rebuilt but not that the rebuilt manifest is attributable; it now pins `version` and `account`. - The marker identity test varied everything except the account; an account-mismatched marker is now in its `other` list. - Stale comment: the unbind branch no longer says discovery does not consult the manifest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC
1 parent a2af6a0 commit 0a982df

6 files changed

Lines changed: 122 additions & 19 deletions

File tree

‎packages/opencode/src/altimate/workspace/skill-sync.ts‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,15 +1032,20 @@ export async function syncSkills(directory: string): Promise<SyncResult> {
10321032
// to an account with no binding here left the previous tenant's skills on
10331033
// disk and loading into prompts, with every retry hitting the same return.
10341034
const credsForPurge = await AltimateApi.getCredentials().catch(() => null)
1035-
if (credsForPurge) {
1035+
// A credential with no key names no account, so nothing here can be judged
1036+
// foreign — and the sync below will fail for the same reason, so there is
1037+
// no replacement coming. Purging on it deleted a good snapshot and put
1038+
// nothing back. Unknown never destroys a snapshot, the same rule the
1039+
// disconnect and lookup-failure branches follow. (review)
1040+
const purgeAccount = credsForPurge?.altimateApiKey
1041+
? credentialDigest(
1042+
credsForPurge.altimateUrl,
1043+
credsForPurge.altimateInstanceName,
1044+
credsForPurge.altimateApiKey,
1045+
)
1046+
: null
1047+
if (credsForPurge && purgeAccount) {
10361048
const priorManifest = await readManifest(canon)
1037-
const purgeAccount = credsForPurge.altimateApiKey
1038-
? credentialDigest(
1039-
credsForPurge.altimateUrl,
1040-
credsForPurge.altimateInstanceName,
1041-
credsForPurge.altimateApiKey,
1042-
)
1043-
: null
10441049
// Ours but unattributable — a v1 tree, which recorded no account — is
10451050
// dropped too. It could have been fetched by anyone, so being in this
10461051
// directory is not grounds to serve it.
@@ -1062,9 +1067,10 @@ export async function syncSkills(directory: string): Promise<SyncResult> {
10621067
// machine, cleared state) would otherwise never get its workspace's skills.
10631068
const outcome = await resolveBindingOutcome(canon)
10641069
if (outcome.status !== "bound") {
1065-
// A CONFIRMED unbind must take the snapshot out of service — discovery
1066-
// does not consult the manifest, so leaving it keeps serving a workspace
1067-
// this project is no longer attached to. "Unknown" must not: a lookup
1070+
// A CONFIRMED unbind must take the snapshot out of service. Discovery's
1071+
// account gate does not help here: the snapshot is still this account's,
1072+
// it is the project that is no longer attached, so leaving it keeps
1073+
// serving a workspace this project has left. "Unknown" must not: a lookup
10681074
// failure is not evidence of anything, and deleting on it would wipe a
10691075
// snapshot on a network blip.
10701076
if (outcome.status === "unbound") {

‎packages/opencode/src/skill/index.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -309,16 +309,34 @@ export function snapshotProjectOf(real: string): string | null {
309309
* the manifest or the credentials cannot be read the snapshot is withheld,
310310
* which costs a poll interval, where serving it cannot be taken back. */
311311
const withoutForeignSnapshots = Effect.fnUntraced(function* (matches: string[]) {
312+
// `null` means "could not be resolved", and it is withheld rather than
313+
// served. Falling back to the matched path would answer "ordinary skill" for
314+
// an alias whose target merely happened to be unreadable at this instant,
315+
// such as during a concurrent snapshot swap — and if the target became
316+
// readable again before `add` ran, its content loaded with no account check
317+
// at all. A path that cannot be resolved cannot be attributed. (review)
312318
const real = yield* Effect.promise(() =>
313-
Promise.all(matches.map((match) => fsp.realpath(match).catch(() => match))),
319+
Promise.all(
320+
matches.map((match) =>
321+
fsp.realpath(match).then(
322+
(resolved) => resolved as string | null,
323+
() => null,
324+
),
325+
),
326+
),
314327
)
315328

329+
const unresolved = matches.filter((_, i) => real[i] === null || real[i] === undefined)
330+
if (unresolved.length > 0)
331+
yield* Effect.logWarning("withholding skills whose paths could not be resolved", { paths: unresolved })
332+
316333
const projects = new Set<string>()
317334
for (const candidate of real) {
335+
if (!candidate) continue
318336
const project = snapshotProjectOf(candidate)
319337
if (project !== null) projects.add(project)
320338
}
321-
if (projects.size === 0) return matches
339+
if (projects.size === 0 && unresolved.length === 0) return matches
322340

323341
const ours = new Map<string, boolean>()
324342
for (const project of projects) {
@@ -335,7 +353,9 @@ const withoutForeignSnapshots = Effect.fnUntraced(function* (matches: string[])
335353
}
336354

337355
return matches.filter((_, i) => {
338-
const project = snapshotProjectOf(real[i]!)
356+
const candidate = real[i]
357+
if (!candidate) return false
358+
const project = snapshotProjectOf(candidate)
339359
return project === null || ours.get(project) === true
340360
})
341361
})

‎packages/opencode/test/altimate/workspace/skill-sync.test.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1957,6 +1957,12 @@ describe("workspace skill sync", () => {
19571957

19581958
expect(existsSync(path.join(managed, "old-skill", "SKILL.md"))).toBe(false)
19591959
expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true)
1960+
// And the replacement is attributable, which is the property this change
1961+
// exists for: asserting only that the tree was rebuilt would still pass if
1962+
// the v2 write omitted or mangled `account`. (review)
1963+
const rewritten = JSON.parse(readFileSync(path.join(managed, ".manifest.json"), "utf8"))
1964+
expect(rewritten.version).toBe(2)
1965+
expect(rewritten.account).toBe(FIXTURE_ACCOUNT)
19601966
})
19611967

19621968
test("a purge re-syncs immediately instead of waiting out the poll interval", async () => {
@@ -1972,6 +1978,29 @@ describe("workspace skill sync", () => {
19721978
expect(await recentlySynced(project)).toBe(false)
19731979
})
19741980

1981+
test("a credential with no key purges nothing", async () => {
1982+
// The purge compared the manifest's account against a digest that is null
1983+
// when no key resolves, so every valid snapshot looked foreign — and the
1984+
// sync below then failed for the same missing key, so nothing replaced it.
1985+
// Unknown never destroys a snapshot. (review)
1986+
serve({ "pub-1": { "SKILL.md": "one" } })
1987+
await syncSkills(project)
1988+
expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true)
1989+
1990+
const credsFile = path.join(SANDBOX, "home", ".altimate", "altimate.json")
1991+
const saved = readFileSync(credsFile, "utf8")
1992+
try {
1993+
writeFileSync(
1994+
credsFile,
1995+
JSON.stringify({ altimateUrl: API_URL, altimateInstanceName: TENANT, altimateApiKey: "" }),
1996+
)
1997+
await syncSkills(project)
1998+
expect(existsSync(skillFile("pub-1", "SKILL.md"))).toBe(true)
1999+
} finally {
2000+
writeFileSync(credsFile, saved)
2001+
}
2002+
})
2003+
19752004
test("an accountless v2 manifest is not ours to delete", async () => {
19762005
// Raised in review. v1 leniency is the migration path; a v2 manifest with
19772006
// no account can only be damaged or hand-written, and claiming it would

‎packages/opencode/test/altimate/workspace/state-account-scope.test.ts‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,15 @@ import { mkdirSync, rmSync, writeFileSync } from "node:fs"
1111
import path from "node:path"
1212
import os from "node:os"
1313

14-
const ORIGINAL_XDG_STATE_HOME = process.env.XDG_STATE_HOME
14+
// `OPENCODE_TEST_STATE_HOME`, not `XDG_STATE_HOME`. `Global.Path.state` is a
15+
// getter that reads this on every access, so it holds however the module graph
16+
// was loaded. `XDG_STATE_HOME` is folded into a module-load-time const, so it
17+
// only works if this file happens to import Global first — and test files share
18+
// a worker, so that is not something a file can arrange. (review)
19+
const ORIGINAL_STATE_HOME = process.env.OPENCODE_TEST_STATE_HOME
1520
const SANDBOX = path.join(os.tmpdir(), `altimate-state-account-${process.pid}-${Date.now()}`)
1621
mkdirSync(path.join(SANDBOX, "state"), { recursive: true })
17-
process.env.XDG_STATE_HOME = path.join(SANDBOX, "state")
22+
process.env.OPENCODE_TEST_STATE_HOME = path.join(SANDBOX, "state")
1823

1924
const { recordApprovedBinding, readLocalBinding, clearLocalBinding, cachePath, credentialDigest } =
2025
await import("../../../src/altimate/workspace/state")
@@ -56,12 +61,19 @@ afterEach(() => {
5661
})
5762

5863
afterAll(() => {
59-
if (ORIGINAL_XDG_STATE_HOME === undefined) delete process.env.XDG_STATE_HOME
60-
else process.env.XDG_STATE_HOME = ORIGINAL_XDG_STATE_HOME
64+
if (ORIGINAL_STATE_HOME === undefined) delete process.env.OPENCODE_TEST_STATE_HOME
65+
else process.env.OPENCODE_TEST_STATE_HOME = ORIGINAL_STATE_HOME
6166
rmSync(SANDBOX, { recursive: true, force: true })
6267
})
6368

6469
describe("binding cache is scoped to the account, not the tenant", () => {
70+
test("the sandbox redirect is in effect", () => {
71+
// Guards the guard: if `Global.Path.state` were not redirected, every test
72+
// below would still pass while reading and writing the real developer's
73+
// state directory. (review)
74+
expect(cachePath().startsWith(SANDBOX)).toBe(true)
75+
})
76+
6577
test("a second user on the same tenant does not inherit the first user's workspace", async () => {
6678
// The reported bug: A links this project to their private workspace, the
6779
// credentials are switched to B on the same tenant, and B's session read

‎packages/opencode/test/skill/release-v0.12.0-adversarial.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,14 +337,18 @@ describe("v0.12.0 adversarial: lastSuccessfulSyncAt against a hand-edited marker
337337
}
338338
})
339339

340-
test("a valid marker for a different workspace, tenant or API host is not this binding's", async () => {
340+
test("a valid marker for a different workspace, tenant, API host or account is not this binding's", async () => {
341341
const at = 1700000000000
342342
const good = { at, ...binding }
343343
expect(await lastSuccessfulSyncAt(withMarker(JSON.stringify(good)), binding)).toBe(at)
344344
for (const other of [
345345
{ ...good, datamateId: 8 },
346346
{ ...good, tenant: "ACME" },
347347
{ ...good, apiUrl: "https://api.example/" },
348+
// Two accounts granted the SAME workspace on the same host: everything
349+
// else matches, so the account is the only thing that can tell the first
350+
// user's sync age from the second's. (review)
351+
{ ...good, account: "acct-8" },
348352
]) {
349353
expect(await lastSuccessfulSyncAt(withMarker(JSON.stringify(other)), binding)).toBeNull()
350354
}

‎packages/opencode/test/skill/skill.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -809,6 +809,38 @@ description: A skill in the .opencode/skills directory.
809809
),
810810
)
811811

812+
// A path that cannot be resolved cannot be attributed, so it is withheld.
813+
// Falling back to the matched path answered "ordinary skill" for a broken or
814+
// momentarily unreadable alias — and one whose target became readable again
815+
// before the file was read would have loaded with no account check. (review)
816+
it.live("a skill whose path cannot be resolved is withheld, not assumed innocent", () =>
817+
provideTmpdirInstance(
818+
(dir) =>
819+
withHome(
820+
dir,
821+
Effect.gen(function* () {
822+
yield* Effect.promise(() => writeSnapshot(dir, FIXTURE_ACCOUNT))
823+
// A dangling link where a skill would be. `realpath` cannot resolve
824+
// it, and the glob still matches it because the leaf name does.
825+
yield* Effect.promise(async () => {
826+
await fs.mkdir(path.join(dir, ".opencode", "skill", "ghost"), { recursive: true })
827+
await fs.symlink(
828+
path.join(dir, "nowhere", "SKILL.md"),
829+
path.join(dir, ".opencode", "skill", "ghost", "SKILL.md"),
830+
)
831+
})
832+
833+
const skill = yield* Skill.Service
834+
const all = yield* skill.all()
835+
// The real snapshot is unaffected: withholding is per path.
836+
expect(all.find((s) => s.name === "workspace-synced")).toBeDefined()
837+
expect((yield* skill.dirs()).some((d) => d.endsWith("ghost"))).toBe(false)
838+
}),
839+
),
840+
{ git: true },
841+
),
842+
)
843+
812844
// A snapshot from before the manifest carried an account cannot be
813845
// attributed to anyone, so it is withheld rather than served to whoever
814846
// happens to be logged in. The sync drops and re-fetches it.

0 commit comments

Comments
 (0)