Skip to content

Commit 2291a2e

Browse files
saravmajesticclaude
andcommitted
fix(workspace): attribute a skill by both ends of its path, not one
Moving attribution onto the resolved path closed the alias pointing INTO a snapshot and opened the reverse: a link inside a foreign snapshot whose target lives outside it resolves to a path with no managed component, so it read as an ordinary skill. A symlink crosses that boundary in either direction, so both ends are checked, and a match touching any managed project is served only if every one of them is ours. Verified separately for each end: gating on the resolved path alone fails the link-out test, gating on the matched path alone fails the link-in test, and both together pass. The mutations were confirmed applied rather than assumed from the edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC
1 parent 0a982df commit 2291a2e

2 files changed

Lines changed: 57 additions & 11 deletions

File tree

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

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -326,16 +326,30 @@ const withoutForeignSnapshots = Effect.fnUntraced(function* (matches: string[])
326326
),
327327
)
328328

329-
const unresolved = matches.filter((_, i) => real[i] === null || real[i] === undefined)
329+
const unresolved = matches.filter((_, i) => !real[i])
330330
if (unresolved.length > 0)
331331
yield* Effect.logWarning("withholding skills whose paths could not be resolved", { paths: unresolved })
332332

333-
const projects = new Set<string>()
334-
for (const candidate of real) {
335-
if (!candidate) continue
336-
const project = snapshotProjectOf(candidate)
337-
if (project !== null) projects.add(project)
333+
// BOTH ends, because a symlink crosses the boundary in either direction: one
334+
// pointing INTO a snapshot has no managed component in its matched path, and
335+
// one INSIDE a snapshot pointing out has none in its resolved path. Checking
336+
// only the resolved end traded the first bypass for the second. A match that
337+
// touches any managed project is served only if every one of them is ours.
338+
// (review)
339+
const projectsOf = (match: string, resolved: string | null): string[] => {
340+
const found = new Set<string>()
341+
for (const candidate of [match, resolved]) {
342+
if (!candidate) continue
343+
const project = snapshotProjectOf(candidate)
344+
if (project !== null) found.add(project)
345+
}
346+
return [...found]
338347
}
348+
349+
const projects = new Set<string>()
350+
matches.forEach((match, i) => {
351+
for (const project of projectsOf(match, real[i] ?? null)) projects.add(project)
352+
})
339353
if (projects.size === 0 && unresolved.length === 0) return matches
340354

341355
const ours = new Map<string, boolean>()
@@ -352,11 +366,10 @@ const withoutForeignSnapshots = Effect.fnUntraced(function* (matches: string[])
352366
ours.set(project, ok)
353367
}
354368

355-
return matches.filter((_, i) => {
356-
const candidate = real[i]
357-
if (!candidate) return false
358-
const project = snapshotProjectOf(candidate)
359-
return project === null || ours.get(project) === true
369+
return matches.filter((match, i) => {
370+
const resolved = real[i]
371+
if (!resolved) return false
372+
return projectsOf(match, resolved).every((project) => ours.get(project) === true)
360373
})
361374
})
362375
// altimate_change end

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

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

812+
// The reverse alias: a link INSIDE a foreign snapshot pointing at a file
813+
// outside it. Its resolved path carries no managed component, so gating on
814+
// the resolved end alone traded one bypass for another. (review)
815+
it.live("a link out of another account's snapshot is not a way out", () =>
816+
provideTmpdirInstance(
817+
(dir) =>
818+
withHome(
819+
dir,
820+
Effect.gen(function* () {
821+
yield* Effect.promise(() => writeSnapshot(dir, credentialDigest(API_URL, TENANT, "someone-else")))
822+
yield* Effect.promise(async () => {
823+
// The real content lives outside the snapshot; the snapshot only
824+
// links to it, so `realpath` leads away from `_workspace`.
825+
await fs.mkdir(path.join(dir, "elsewhere"), { recursive: true })
826+
await fs.writeFile(
827+
path.join(dir, "elsewhere", "SKILL.md"),
828+
`---\nname: linked-out\ndescription: Reached through a foreign snapshot.\n---\n\nBody.\n`,
829+
)
830+
await fs.mkdir(path.join(dir, MANAGED, "pub-linked"), { recursive: true })
831+
await fs.symlink(
832+
path.join(dir, "elsewhere", "SKILL.md"),
833+
path.join(dir, MANAGED, "pub-linked", "SKILL.md"),
834+
)
835+
})
836+
837+
const skill = yield* Skill.Service
838+
expect((yield* skill.all()).find((s) => s.name === "linked-out")).toBeUndefined()
839+
}),
840+
),
841+
{ git: true },
842+
),
843+
)
844+
812845
// A path that cannot be resolved cannot be attributed, so it is withheld.
813846
// Falling back to the matched path answered "ordinary skill" for a broken or
814847
// momentarily unreadable alias — and one whose target became readable again

0 commit comments

Comments
 (0)