From 89818b94e7b9ab59430b8b7da850ea5cd2f8ef30 Mon Sep 17 00:00:00 2001 From: Qize Liu Date: Mon, 17 Aug 2026 16:31:02 +0800 Subject: [PATCH 1/6] Fix git source update preflight and checkout refresh --- .../src/services/source-authority-service.ts | 3 + .../src/services/source-checkout-service.ts | 112 ++++++++-- .../tests/source-authority-service.test.ts | 5 + .../src/tests/source-checkout-service.test.ts | 192 +++++++++++++++++- .../query/src/tests/source-lifecycle.test.ts | 19 +- 5 files changed, 305 insertions(+), 26 deletions(-) diff --git a/packages/core-engine/src/services/source-authority-service.ts b/packages/core-engine/src/services/source-authority-service.ts index 04b965d..35b0ec5 100644 --- a/packages/core-engine/src/services/source-authority-service.ts +++ b/packages/core-engine/src/services/source-authority-service.ts @@ -333,6 +333,7 @@ export class SourceAuthorityService { try { const remoteCommit = await this.options.checkoutService.readGitRemoteHeadCommit( source.locator, + lock.originBranch ? { branch: lock.originBranch } : {}, ); if (!remoteCommit) { precheckFallbackSourceIds.push(sourceId); @@ -374,8 +375,10 @@ export class SourceAuthorityService { options: { sourceIdOverride: sourceId, displayNameOverride: source.displayName, + ...(lock.originBranch ? { originBranch: lock.originBranch } : {}), }, checkoutPath: tempCheckoutPath, + existingCheckoutPath: lock.localPath, allowEmptyLeafs: true, }); if (!prepared.ok) { diff --git a/packages/core-engine/src/services/source-checkout-service.ts b/packages/core-engine/src/services/source-checkout-service.ts index 469ac47..67113f9 100644 --- a/packages/core-engine/src/services/source-checkout-service.ts +++ b/packages/core-engine/src/services/source-checkout-service.ts @@ -97,6 +97,7 @@ type SourceResolution = { clawhubSlug?: string; requestedVersion?: string; versionMode?: "pinned" | "floating"; + originBranch?: string; }; export class SourceCheckoutService { @@ -154,6 +155,7 @@ export class SourceCheckoutService { options?: AddSourceOptions; existingSources?: Array<{ id: string; kind?: SourceKind; locator: string; displayName: string }>; checkoutPath?: string; + existingCheckoutPath?: string; suffix?: string; allowEmptyLeafs?: boolean; } = {}, @@ -171,7 +173,7 @@ export class SourceCheckoutService { await ensureDir(path.dirname(checkoutPath)); try { - await this.fetchSource(resolved, checkoutPath); + await this.fetchSource(resolved, checkoutPath, input.existingCheckoutPath); } catch (error) { await removePath(checkoutPath); return fail({ @@ -187,7 +189,9 @@ export class SourceCheckoutService { resolved.displayName, checkoutPath, resolved.requestedPath, - options, + resolved.originBranch && !options.originBranch + ? { ...options, originBranch: resolved.originBranch } + : options, input.allowEmptyLeafs === undefined ? {} : { allowEmptyLeafs: input.allowEmptyLeafs }, ); if (!snapshot.ok) { @@ -256,11 +260,16 @@ export class SourceCheckoutService { }, snapshot.warnings); } - async readGitRemoteHeadCommit(locator: string): Promise { + async readGitRemoteHeadCommit( + locator: string, + options: { branch?: string } = {}, + ): Promise { if (!(await isGitAvailable())) { return undefined; } + const gitLocator = await this.normalizeLocator(locator); + const parseCommitSha = (raw: string): string | undefined => { const line = raw .split(/\r?\n/) @@ -270,7 +279,8 @@ export class SourceCheckoutService { return sha && /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(sha) ? sha : undefined; }; - const headOutput = await git(["ls-remote", locator, "HEAD"], { timeoutMs: 5_000 }); + const remoteRef = this.remoteRef(options.branch); + const headOutput = await git(["ls-remote", gitLocator, remoteRef], { timeoutMs: 5_000 }); return parseCommitSha(headOutput); } @@ -328,6 +338,7 @@ export class SourceCheckoutService { displayName: options.displayNameOverride ?? deriveDisplayName(treeLocator.repoLocator), sourceId: options.sourceIdOverride ?? deriveSourceId(treeLocator.repoLocator), ...(requestedPath ? { requestedPath } : {}), + ...(treeLocator.originBranch ? { originBranch: treeLocator.originBranch } : {}), }; } @@ -344,6 +355,7 @@ export class SourceCheckoutService { displayName: options.displayNameOverride ?? deriveDisplayName(shorthandLocator.repoLocator), sourceId: options.sourceIdOverride ?? deriveSourceId(shorthandLocator.repoLocator), ...(requestedPath ? { requestedPath } : {}), + ...(options.originBranch ? { originBranch: options.originBranch } : {}), }; } @@ -376,6 +388,7 @@ export class SourceCheckoutService { displayName: options.displayNameOverride ?? deriveDisplayName(locator), sourceId: options.sourceIdOverride ?? deriveSourceId(locator), ...(requestedPath ? { requestedPath } : {}), + ...(options.originBranch ? { originBranch: options.originBranch } : {}), }; } @@ -463,6 +476,7 @@ export class SourceCheckoutService { private async fetchSource( source: SourceResolution, checkoutPath: string, + existingCheckoutPath?: string, ): Promise { if (source.kind === "local") { await copyDirectory(source.localPath!, checkoutPath); @@ -470,7 +484,23 @@ export class SourceCheckoutService { } if (source.kind === "git") { - await this.fetchGitSource(source.gitLocator!, checkoutPath); + if ( + existingCheckoutPath + && await this.isGitRepositoryPath(existingCheckoutPath) + ) { + try { + await this.refreshGitSourceFromExisting( + source.gitLocator!, + existingCheckoutPath, + checkoutPath, + source.originBranch, + ); + return; + } catch { + await removePath(checkoutPath).catch(() => {}); + } + } + await this.fetchGitSource(source.gitLocator!, checkoutPath, source.originBranch); return; } @@ -485,6 +515,31 @@ export class SourceCheckoutService { } } + private async refreshGitSourceFromExisting( + locator: string, + existingCheckoutPath: string, + checkoutPath: string, + originBranch?: string, + ): Promise { + await removePath(checkoutPath).catch(() => {}); + await git([ + "clone", + "--local", + "--no-checkout", + existingCheckoutPath, + checkoutPath, + ]); + await git(["remote", "set-url", "origin", locator], { cwd: checkoutPath }); + const remoteRef = this.remoteRef(originBranch); + await git(["fetch", "--depth", "1", "origin", remoteRef], { cwd: checkoutPath }); + await git(["checkout", "--detach", "--force", "FETCH_HEAD"], { cwd: checkoutPath }); + } + + private remoteRef(originBranch?: string): string { + const branch = originBranch?.trim(); + return branch ? `refs/heads/${branch}` : "HEAD"; + } + private async buildSnapshot( kind: SourceCheckoutKind, sourceId: string, @@ -674,7 +729,7 @@ export class SourceCheckoutService { private parseTreeLocator( locator: string, - ): { repoLocator: string; requestedPath?: string } | null { + ): { repoLocator: string; requestedPath?: string; originBranch?: string } | null { try { const url = new URL(locator); @@ -686,14 +741,16 @@ export class SourceCheckoutService { const owner = parts[0]; const repo = parts[1]; + const originBranch = parts[3]; const requestedPath = parts.slice(4).join("/"); - if (!owner || !repo || !requestedPath) { + if (!owner || !repo || !originBranch || !requestedPath) { return null; } return { repoLocator: `https://github.com/${owner}/${repo}.git`, requestedPath, + originBranch, }; } @@ -705,11 +762,13 @@ export class SourceCheckoutService { return null; } + const originBranch = parts[markerIndex + 2]; const requestedPath = parts.slice(markerIndex + 3).join("/"); return { repoLocator: `https://gitlab.com/${parts.slice(0, markerIndex).join("/")}.git`, ...(requestedPath ? { requestedPath } : {}), + ...(originBranch ? { originBranch } : {}), }; } } catch { @@ -802,14 +861,18 @@ export class SourceCheckoutService { return `${normalizedBase}/${normalizedChild}`; } - private async fetchGitSource(locator: string, checkoutPath: string): Promise { + private async fetchGitSource( + locator: string, + checkoutPath: string, + originBranch?: string, + ): Promise { if (!(await isGitAvailable())) { - await this.fetchGitArchive(locator, checkoutPath); + await this.fetchGitArchive(locator, checkoutPath, originBranch); return; } try { - await this.cloneGitWithRetries(locator, checkoutPath); + await this.cloneGitWithRetries(locator, checkoutPath, originBranch); return; } catch { // Prefer HTTPS fallback when SSH/clone URL variants fail, then archive. @@ -818,23 +881,34 @@ export class SourceCheckoutService { const fallbackLocator = this.resolveGitCloneFallbackLocator(locator); if (fallbackLocator) { try { - await this.cloneGitWithRetries(fallbackLocator, checkoutPath); + await this.cloneGitWithRetries(fallbackLocator, checkoutPath, originBranch); return; } catch { await removePath(checkoutPath); - await this.fetchGitArchive(fallbackLocator, checkoutPath); + await this.fetchGitArchive(fallbackLocator, checkoutPath, originBranch); return; } } await removePath(checkoutPath); - await this.fetchGitArchive(locator, checkoutPath); + await this.fetchGitArchive(locator, checkoutPath, originBranch); } - private async cloneGitWithRetries(locator: string, checkoutPath: string): Promise { + private async cloneGitWithRetries( + locator: string, + checkoutPath: string, + originBranch?: string, + ): Promise { await withNetworkRetries(async () => { await removePath(checkoutPath).catch(() => {}); - await git(["clone", "--depth", "1", locator, checkoutPath]); + await git([ + "clone", + "--depth", + "1", + ...(originBranch ? ["--branch", originBranch] : []), + locator, + checkoutPath, + ]); }, { attempts: 2 }); } @@ -855,11 +929,9 @@ export class SourceCheckoutService { try { await ensureDir(tempRoot); - const branchCandidates = [ - preferredBranch, - "main", - "master", - ].filter((value, index, values): value is string => Boolean(value) && values.indexOf(value) === index); + const branchCandidates = preferredBranch + ? [preferredBranch] + : ["main", "master"]; let lastError: Error | undefined; for (const branch of branchCandidates) { diff --git a/packages/core-engine/src/tests/source-authority-service.test.ts b/packages/core-engine/src/tests/source-authority-service.test.ts index 997a9bd..0b76195 100644 --- a/packages/core-engine/src/tests/source-authority-service.test.ts +++ b/packages/core-engine/src/tests/source-authority-service.test.ts @@ -265,6 +265,7 @@ describe.sequential("SourceAuthorityService", () => { }], invalidLeafs: [], commitSha: "same-sha", + originBranch: "release", }, }); expect(committed.ok).toBe(true); @@ -287,6 +288,10 @@ describe.sequential("SourceAuthorityService", () => { return; } expect(prepareSourceCheckout).not.toHaveBeenCalled(); + expect(checkoutService.readGitRemoteHeadCommit).toHaveBeenCalledWith( + "https://github.com/acme/skills.git", + { branch: "release" }, + ); expect(updated.data.updated).toEqual([ expect.objectContaining({ sourceId: "git-unchanged", diff --git a/packages/core-engine/src/tests/source-checkout-service.test.ts b/packages/core-engine/src/tests/source-checkout-service.test.ts index 04a236d..cf693cf 100644 --- a/packages/core-engine/src/tests/source-checkout-service.test.ts +++ b/packages/core-engine/src/tests/source-checkout-service.test.ts @@ -59,6 +59,42 @@ describe.sequential("SourceCheckoutService", () => { ); }); + test("reads remote HEAD commit for GitHub shorthand locators", async () => { + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); + const git = vi.spyOn(gitUtils, "git").mockResolvedValue( + "0123456789abcdef0123456789abcdef01234567\tHEAD", + ); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + await expect(service.readGitRemoteHeadCommit("acme/skills")) + .resolves.toBe("0123456789abcdef0123456789abcdef01234567"); + expect(git).toHaveBeenCalledWith( + ["ls-remote", "https://github.com/acme/skills.git", "HEAD"], + { timeoutMs: 5_000 }, + ); + }); + + test("reads the configured remote branch commit", async () => { + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); + const git = vi.spyOn(gitUtils, "git").mockResolvedValue( + "89abcdef0123456789abcdef0123456789abcdef\trefs/heads/release", + ); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + await expect(service.readGitRemoteHeadCommit("acme/skills", { branch: "release" })) + .resolves.toBe("89abcdef0123456789abcdef0123456789abcdef"); + expect(git).toHaveBeenCalledWith( + ["ls-remote", "https://github.com/acme/skills.git", "refs/heads/release"], + { timeoutMs: 5_000 }, + ); + }); + test("accepts SHA-256 remote HEAD commits", async () => { const sha256 = "0123456789abcdef".repeat(4); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); @@ -122,6 +158,152 @@ describe.sequential("SourceCheckoutService", () => { await expect(fs.access(path.join(sandbox.stateRoot, "collections.json"))).rejects.toThrow(); }); + test("refreshes a git checkout from its existing object store", async () => { + const existingCheckoutPath = path.join(sandbox.sandboxRoot, "existing-checkout"); + const checkoutPath = path.join(sandbox.stateRoot, "source", "git", ".update-existing"); + await fs.mkdir(path.join(existingCheckoutPath, ".git"), { recursive: true }); + await fs.mkdir(path.join(existingCheckoutPath, "skills", "one"), { recursive: true }); + await fs.writeFile( + path.join(existingCheckoutPath, "skills", "one", "SKILL.md"), + skillDoc("one", "Old."), + "utf8", + ); + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); + vi.spyOn(gitUtils, "git").mockImplementation(async (args, options) => { + if ( + args[0] === "clone" + && args[1] === "--local" + && args[2] === "--no-checkout" + && args[3] === existingCheckoutPath + && args[4] === checkoutPath + ) { + await fs.cp(existingCheckoutPath, checkoutPath, { recursive: true }); + return ""; + } + if (args[0] === "remote" && args[1] === "set-url" && options?.cwd === checkoutPath) { + return ""; + } + if (args[0] === "fetch" && options?.cwd === checkoutPath) { + return ""; + } + if (args[0] === "checkout" && options?.cwd === checkoutPath) { + await fs.writeFile( + path.join(checkoutPath, "skills", "one", "SKILL.md"), + skillDoc("one", "New."), + "utf8", + ); + return ""; + } + if (args[0] === "rev-parse" && args[1] === "HEAD" && options?.cwd === checkoutPath) { + return "fedcba9876543210fedcba9876543210fedcba98"; + } + throw new Error(`Unexpected git call: ${args.join(" ")}`); + }); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + const prepared = await service.prepareSourceCheckout( + "git@example.test:acme/skills.git", + { + checkoutPath, + existingCheckoutPath, + options: { sourceIdOverride: "git-existing", originBranch: "release" }, + }, + ); + + expect(prepared.ok).toBe(true); + if (!prepared.ok) { + return; + } + expect(prepared.data.commitSha).toBe("fedcba9876543210fedcba9876543210fedcba98"); + await expect(fs.readFile( + path.join(checkoutPath, "skills", "one", "SKILL.md"), + "utf8", + )).resolves.toContain("New."); + }); + + test("falls back to a clean clone when the existing checkout cannot be refreshed", async () => { + const existingCheckoutPath = path.join(sandbox.sandboxRoot, "broken-existing-checkout"); + const upstreamRepo = await createRepo(sandbox.sandboxRoot, { + "skills/one/SKILL.md": skillDoc("one", "Fresh."), + }); + const checkoutPath = path.join(sandbox.stateRoot, "source", "git", ".update-fallback"); + await fs.mkdir(path.join(existingCheckoutPath, ".git"), { recursive: true }); + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); + const git = vi.spyOn(gitUtils, "git").mockImplementation(async (args, options) => { + if (args[0] === "clone" && args[1] === "--local") { + throw new Error("local clone failed"); + } + if ( + args[0] === "clone" + && args[1] === "--depth" + && args[3] === "--branch" + && args[4] === "release" + && args[5] === "https://github.com/acme/skills.git" + && args[6] === checkoutPath + ) { + await fs.cp(upstreamRepo, checkoutPath, { recursive: true }); + return ""; + } + if (args[0] === "rev-parse" && args[1] === "HEAD" && options?.cwd === checkoutPath) { + return "abcdef0123456789abcdef0123456789abcdef01"; + } + throw new Error(`Unexpected git call: ${args.join(" ")}`); + }); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + const prepared = await service.prepareSourceCheckout( + "https://github.com/acme/skills.git", + { + checkoutPath, + existingCheckoutPath, + options: { sourceIdOverride: "git-fallback", originBranch: "release" }, + }, + ); + + expect(prepared.ok).toBe(true); + expect(git).toHaveBeenCalledWith([ + "clone", + "--depth", + "1", + "--branch", + "release", + "https://github.com/acme/skills.git", + checkoutPath, + ]); + await expect(fs.readFile( + path.join(checkoutPath, "skills", "one", "SKILL.md"), + "utf8", + )).resolves.toContain("Fresh."); + }); + + test("does not switch branches when a locked branch archive is unavailable", async () => { + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(false); + const fetch = vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response("missing", { status: 404 }), + ); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + const prepared = await service.prepareSourceCheckout( + "https://github.com/acme/skills.git", + { options: { sourceIdOverride: "locked-release", originBranch: "release" } }, + ); + + expect(prepared.ok).toBe(false); + expect(fetch).toHaveBeenCalledTimes(1); + expect(fetch.mock.calls[0]?.[0]).toBe( + "https://github.com/acme/skills/archive/refs/heads/release.zip", + ); + }); + test("rejects skill leafs with escaping symlinks before they can be deployed", async () => { const repoPath = await createRepo(sandbox.sandboxRoot, { "skills/unsafe/SKILL.md": skillDoc("unsafe", "Unsafe skill."), @@ -152,8 +334,13 @@ describe.sequential("SourceCheckoutService", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { - if (args[0] === "clone" && args[3] === "https://github.com/vercel-labs/skills.git") { - await fs.cp(upstreamRepo, args[4]!, { recursive: true }); + if ( + args[0] === "clone" + && args[3] === "--branch" + && args[4] === "main" + && args[5] === "https://github.com/vercel-labs/skills.git" + ) { + await fs.cp(upstreamRepo, args[6]!, { recursive: true }); return ""; } @@ -178,6 +365,7 @@ describe.sequential("SourceCheckoutService", () => { return; } expect(prepared.data.kind).toBe("git"); + expect(prepared.data.originBranch).toBe("main"); expect(prepared.data.requestedPath).toBe("skills/find-skills"); expect(prepared.data.checkoutPath).toContain(`${path.sep}source${path.sep}git${path.sep}`); expect(prepared.data.leafs.map((leaf) => leaf.id)).toEqual([ diff --git a/packages/query/src/tests/source-lifecycle.test.ts b/packages/query/src/tests/source-lifecycle.test.ts index 9984da2..030d4c2 100644 --- a/packages/query/src/tests/source-lifecycle.test.ts +++ b/packages/query/src/tests/source-lifecycle.test.ts @@ -879,8 +879,13 @@ describe.sequential("source lifecycle", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { - if (args[0] === "clone" && args[3] === "https://github.com/vercel-labs/skills.git") { - await fs.cp(upstreamRepo, args[4]!, { recursive: true }); + if ( + args[0] === "clone" + && args[3] === "--branch" + && args[4] === "main" + && args[5] === "https://github.com/vercel-labs/skills.git" + ) { + await fs.cp(upstreamRepo, args[6]!, { recursive: true }); return ""; } @@ -908,6 +913,7 @@ describe.sequential("source lifecycle", () => { const lock = state.lockFile.sources[result.data.manifest.id]; expect(source?.kind).toBe("git"); expect(lock?.revision.provider).toBe("git"); + expect(lock?.originBranch).toBe("main"); expect(lock?.localPath).toBe( app.store.getSourceCheckoutPath("git", result.data.manifest.id), ); @@ -919,8 +925,13 @@ describe.sequential("source lifecycle", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { - if (args[0] === "clone" && args[3] === "https://github.com/vercel-labs/skills.git") { - await fs.cp(upstreamRepo, args[4]!, { recursive: true }); + if ( + args[0] === "clone" + && args[3] === "--branch" + && args[4] === "main" + && args[5] === "https://github.com/vercel-labs/skills.git" + ) { + await fs.cp(upstreamRepo, args[6]!, { recursive: true }); return ""; } From 64dbf430fe0aeba33460df2cff7cff4159f44b92 Mon Sep 17 00:00:00 2001 From: Qize Liu Date: Tue, 18 Aug 2026 02:17:54 +0800 Subject: [PATCH 2/6] Harden long-running source updates --- .../Runtime/Bridge/BridgeClient.swift | 26 +++++- .../BridgeClientExecutionTests.swift | 18 ++++ .../integration/src/tests/fs-utils.test.ts | 85 ++++++++++++++++++- packages/integration/src/utils/fs.ts | 58 ++++++++++++- 4 files changed, 178 insertions(+), 9 deletions(-) diff --git a/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift b/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift index 4591e4b..7c83c6e 100644 --- a/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift +++ b/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift @@ -108,16 +108,20 @@ final class BridgeClient: @unchecked Sendable { private let mutationCoordinator = MutationCoordinator() private let commandTimeoutMilliseconds: UInt64 private let importCommandTimeoutMilliseconds: UInt64 + private let updateCommandTimeoutMilliseconds: UInt64? private let commandTimeoutGraceMilliseconds: UInt64 init( commandTimeoutMilliseconds: UInt64 = 60_000, - // Network-heavy import/update/add work often needs more than 3 minutes on unstable links. + // Network-heavy import/add work often needs more than 3 minutes on unstable links. importCommandTimeoutMilliseconds: UInt64 = 300_000, + // Each update step is bounded by the helper; the source count is not. + updateCommandTimeoutMilliseconds: UInt64? = nil, commandTimeoutGraceMilliseconds: UInt64 = 1_000 ) { self.commandTimeoutMilliseconds = commandTimeoutMilliseconds self.importCommandTimeoutMilliseconds = importCommandTimeoutMilliseconds + self.updateCommandTimeoutMilliseconds = updateCommandTimeoutMilliseconds self.commandTimeoutGraceMilliseconds = commandTimeoutGraceMilliseconds } @@ -433,6 +437,9 @@ final class BridgeClient: @unchecked Sendable { ) if !didExit { + guard let activeTimeoutMilliseconds else { + preconditionFailure("An unbounded process wait cannot time out") + } outputPipe.fileHandleForReading.readabilityHandler = nil errorPipe.fileHandleForReading.readabilityHandler = nil process.terminationHandler = nil @@ -493,8 +500,11 @@ final class BridgeClient: @unchecked Sendable { throw BridgeClientError.commandFailed(message, response: response) } - private func timeoutMilliseconds(for command: BridgeCommand) -> UInt64 { - command.usesExtendedNetworkTimeout + private func timeoutMilliseconds(for command: BridgeCommand) -> UInt64? { + if command == .update { + return updateCommandTimeoutMilliseconds + } + return command.usesExtendedNetworkTimeout ? importCommandTimeoutMilliseconds : commandTimeoutMilliseconds } @@ -502,8 +512,16 @@ final class BridgeClient: @unchecked Sendable { private func waitForProcessExit( _ process: Process, state: ProcessExitWaitState, - timeoutMilliseconds: UInt64 + timeoutMilliseconds: UInt64? ) async -> Bool { + guard let timeoutMilliseconds else { + let outcome = await withCheckedContinuation { continuation in + state.setContinuation(continuation) + } + process.terminationHandler = nil + return outcome == .exited + } + let timeoutTask = Task { try? await Task.sleep(nanoseconds: Self.nanoseconds(fromMilliseconds: timeoutMilliseconds)) state.resolve(.timedOut) diff --git a/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift b/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift index 7e1c55a..46f8687 100644 --- a/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift +++ b/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift @@ -85,6 +85,24 @@ final class BridgeClientExecutionTests: XCTestCase { XCTAssertTrue(response.ok) } + func testUpdateUsesDedicatedLongRunningCommandTimeout() async throws { + let fixture = try SlowBridgeFixture.install(delayMilliseconds: 100) + self.fixture = fixture + + let bridge = await MainActor.run { + BridgeClient( + commandTimeoutMilliseconds: 25, + importCommandTimeoutMilliseconds: 50, + updateCommandTimeoutMilliseconds: 150 + ) + } + + let response = try await bridge.updateSources(["hugohe3-ppt-master"]) + + XCTAssertEqual(response.command, BridgeCommand.update) + XCTAssertTrue(response.ok) + } + func testTimedOutHelperIsForceKilledWhenItIgnoresTerminate() async throws { let fixture = try StubbornBridgeFixture.install() stubbornFixture = fixture diff --git a/packages/integration/src/tests/fs-utils.test.ts b/packages/integration/src/tests/fs-utils.test.ts index 752805c..d191dd7 100644 --- a/packages/integration/src/tests/fs-utils.test.ts +++ b/packages/integration/src/tests/fs-utils.test.ts @@ -1,8 +1,8 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, test } from "vitest"; -import { hashDirectory } from "../utils/fs.js"; +import { afterEach, describe, expect, test, vi } from "vitest"; +import { hashDirectory, withFileLock } from "../utils/fs.js"; describe("hashDirectory", () => { const roots: string[] = []; @@ -30,4 +30,85 @@ describe("hashDirectory", () => { await expect(hashDirectory(root, { symlinkPolicy: "preserve-safe" })).rejects.toThrow("Unsafe symbolic link"); }); + + test("reclaims a recent lock owned by a dead process", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "skill-flow-fs-")); + roots.push(root); + const lockPath = path.join(root, ".mutation.lock"); + await fs.mkdir(lockPath); + await fs.writeFile( + path.join(lockPath, "owner.json"), + `${JSON.stringify({ pid: 2_147_483_647, acquiredAt: new Date().toISOString() })}\n`, + "utf8", + ); + + await expect(withFileLock( + lockPath, + async () => "acquired", + { pollMs: 5, staleMs: 60_000, timeoutMs: 30 }, + )).resolves.toBe("acquired"); + }); + + test("does not reclaim a recent lock owned by a live process", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "skill-flow-fs-")); + roots.push(root); + const lockPath = path.join(root, ".mutation.lock"); + await fs.mkdir(lockPath); + await fs.writeFile( + path.join(lockPath, "owner.json"), + `${JSON.stringify({ pid: process.pid, acquiredAt: new Date().toISOString() })}\n`, + "utf8", + ); + + await expect(withFileLock( + lockPath, + async () => "unexpected", + { pollMs: 5, staleMs: 60_000, timeoutMs: 20 }, + )).rejects.toThrow("Timed out waiting for state lock"); + }); + + test("does not reclaim a stale lock owned by a live process", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "skill-flow-fs-")); + roots.push(root); + const lockPath = path.join(root, ".mutation.lock"); + await fs.mkdir(lockPath); + await fs.writeFile( + path.join(lockPath, "owner.json"), + `${JSON.stringify({ pid: process.pid, acquiredAt: new Date(0).toISOString() })}\n`, + "utf8", + ); + await fs.utimes(lockPath, new Date(0), new Date(0)); + + await expect(withFileLock( + lockPath, + async () => "unexpected", + { pollMs: 5, staleMs: 1, timeoutMs: 20 }, + )).rejects.toThrow("Timed out waiting for state lock"); + }); + + test("treats a permission-denied owner probe as a live process", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "skill-flow-fs-")); + roots.push(root); + const lockPath = path.join(root, ".mutation.lock"); + await fs.mkdir(lockPath); + await fs.writeFile( + path.join(lockPath, "owner.json"), + `${JSON.stringify({ pid: 42, acquiredAt: new Date(0).toISOString() })}\n`, + "utf8", + ); + await fs.utimes(lockPath, new Date(0), new Date(0)); + const kill = vi.spyOn(process, "kill").mockImplementation(() => { + throw Object.assign(new Error("Operation not permitted"), { code: "EPERM" }); + }); + + try { + await expect(withFileLock( + lockPath, + async () => "unexpected", + { pollMs: 5, staleMs: 1, timeoutMs: 20 }, + )).rejects.toThrow("Timed out waiting for state lock"); + } finally { + kill.mockRestore(); + } + }); }); diff --git a/packages/integration/src/utils/fs.ts b/packages/integration/src/utils/fs.ts index a13164a..7a97207 100644 --- a/packages/integration/src/utils/fs.ts +++ b/packages/integration/src/utils/fs.ts @@ -63,9 +63,11 @@ export async function withFileLock( throw error; } - const stale = await isLockStale(lockPath, staleMs); - if (stale) { - await fs.rm(lockPath, { recursive: true, force: true }); + const ownerState = await readLockOwnerState(lockPath); + const reclaimable = ownerState === "dead" + || (ownerState === "unknown" && await isLockStale(lockPath, staleMs)); + if (reclaimable) { + await reclaimFileLock(lockPath); continue; } @@ -195,6 +197,56 @@ async function isLockStale(lockPath: string, staleMs: number) { } } +async function readLockOwnerState( + lockPath: string, +): Promise<"alive" | "dead" | "unknown"> { + const owner = await readJsonFile( + path.join(lockPath, "owner.json"), + null, + ); + const pid = owner?.pid; + if (!Number.isSafeInteger(pid) || !pid || pid <= 0) { + return "unknown"; + } + + try { + process.kill(pid, 0); + return "alive"; + } catch (error) { + if ( + typeof error === "object" + && error !== null + && "code" in error + ) { + if (error.code === "ESRCH") { + return "dead"; + } + if (error.code === "EPERM") { + return "alive"; + } + } + return "unknown"; + } +} + +async function reclaimFileLock(lockPath: string): Promise { + const quarantinePath = `${lockPath}.reclaim-${process.pid}-${crypto.randomUUID()}`; + try { + await fs.rename(lockPath, quarantinePath); + } catch (error) { + if ( + typeof error === "object" + && error !== null + && "code" in error + && (error.code === "ENOENT" || error.code === "EEXIST") + ) { + return; + } + throw error; + } + await fs.rm(quarantinePath, { recursive: true, force: true }); +} + function isAlreadyExistsError(error: unknown) { return ( typeof error === "object" && From 4bb0b375409258df08c68621813b478728e5ab84 Mon Sep 17 00:00:00 2001 From: Qize Liu Date: Tue, 18 Aug 2026 14:28:19 +0800 Subject: [PATCH 3/6] Align source updates with design boundaries --- .../Runtime/Bridge/BridgeClient.swift | 45 ++++++----- .../BridgeClientExecutionTests.swift | 62 +++++++++++++- .../src/services/source-authority-service.ts | 13 +++ .../src/services/source-checkout-service.ts | 80 ++++++++++--------- .../tests/source-authority-service.test.ts | 51 ++++++++++++ .../src/tests/source-checkout-service.test.ts | 69 ++++++++++++++++ .../integration/src/utils/github-catalog.ts | 67 ++++++++++++++++ .../query/src/tests/source-lifecycle.test.ts | 13 +++ 8 files changed, 340 insertions(+), 60 deletions(-) diff --git a/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift b/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift index 7c83c6e..bc21b4e 100644 --- a/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift +++ b/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift @@ -108,20 +108,23 @@ final class BridgeClient: @unchecked Sendable { private let mutationCoordinator = MutationCoordinator() private let commandTimeoutMilliseconds: UInt64 private let importCommandTimeoutMilliseconds: UInt64 - private let updateCommandTimeoutMilliseconds: UInt64? + private let updateSourceTimeoutMilliseconds: UInt64 + private let updateCommandMaximumTimeoutMilliseconds: UInt64 private let commandTimeoutGraceMilliseconds: UInt64 init( commandTimeoutMilliseconds: UInt64 = 60_000, // Network-heavy import/add work often needs more than 3 minutes on unstable links. importCommandTimeoutMilliseconds: UInt64 = 300_000, - // Each update step is bounded by the helper; the source count is not. - updateCommandTimeoutMilliseconds: UInt64? = nil, + // One source receives five minutes; selected updates scale to a 15-minute ceiling. + updateSourceTimeoutMilliseconds: UInt64 = 300_000, + updateCommandMaximumTimeoutMilliseconds: UInt64 = 900_000, commandTimeoutGraceMilliseconds: UInt64 = 1_000 ) { self.commandTimeoutMilliseconds = commandTimeoutMilliseconds self.importCommandTimeoutMilliseconds = importCommandTimeoutMilliseconds - self.updateCommandTimeoutMilliseconds = updateCommandTimeoutMilliseconds + self.updateSourceTimeoutMilliseconds = updateSourceTimeoutMilliseconds + self.updateCommandMaximumTimeoutMilliseconds = updateCommandMaximumTimeoutMilliseconds self.commandTimeoutGraceMilliseconds = commandTimeoutGraceMilliseconds } @@ -429,7 +432,7 @@ final class BridgeClient: @unchecked Sendable { inputPipe.fileHandleForWriting.write(requestData) inputPipe.fileHandleForWriting.closeFile() - let activeTimeoutMilliseconds = timeoutMilliseconds(for: command) + let activeTimeoutMilliseconds = timeoutMilliseconds(for: command, payload: payload) let didExit = await waitForProcessExit( process, state: exitWaitState, @@ -437,9 +440,6 @@ final class BridgeClient: @unchecked Sendable { ) if !didExit { - guard let activeTimeoutMilliseconds else { - preconditionFailure("An unbounded process wait cannot time out") - } outputPipe.fileHandleForReading.readabilityHandler = nil errorPipe.fileHandleForReading.readabilityHandler = nil process.terminationHandler = nil @@ -500,9 +500,24 @@ final class BridgeClient: @unchecked Sendable { throw BridgeClientError.commandFailed(message, response: response) } - private func timeoutMilliseconds(for command: BridgeCommand) -> UInt64? { + private func timeoutMilliseconds( + for command: BridgeCommand, + payload: [String: AnyCodable]? + ) -> UInt64 { if command == .update { - return updateCommandTimeoutMilliseconds + guard + let sourceIds = payload?["sourceIds"]?.value as? [String], + !sourceIds.isEmpty + else { + return updateCommandMaximumTimeoutMilliseconds + } + let sourceCount = UInt64(Set(sourceIds).count) + let (scaledTimeout, overflow) = updateSourceTimeoutMilliseconds + .multipliedReportingOverflow(by: sourceCount) + return min( + overflow ? updateCommandMaximumTimeoutMilliseconds : scaledTimeout, + updateCommandMaximumTimeoutMilliseconds + ) } return command.usesExtendedNetworkTimeout ? importCommandTimeoutMilliseconds @@ -512,16 +527,8 @@ final class BridgeClient: @unchecked Sendable { private func waitForProcessExit( _ process: Process, state: ProcessExitWaitState, - timeoutMilliseconds: UInt64? + timeoutMilliseconds: UInt64 ) async -> Bool { - guard let timeoutMilliseconds else { - let outcome = await withCheckedContinuation { continuation in - state.setContinuation(continuation) - } - process.terminationHandler = nil - return outcome == .exited - } - let timeoutTask = Task { try? await Task.sleep(nanoseconds: Self.nanoseconds(fromMilliseconds: timeoutMilliseconds)) state.resolve(.timedOut) diff --git a/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift b/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift index 46f8687..a901aa6 100644 --- a/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift +++ b/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift @@ -85,7 +85,7 @@ final class BridgeClientExecutionTests: XCTestCase { XCTAssertTrue(response.ok) } - func testUpdateUsesDedicatedLongRunningCommandTimeout() async throws { + func testUpdateTimeoutScalesWithSelectedSourceCount() async throws { let fixture = try SlowBridgeFixture.install(delayMilliseconds: 100) self.fixture = fixture @@ -93,16 +93,72 @@ final class BridgeClientExecutionTests: XCTestCase { BridgeClient( commandTimeoutMilliseconds: 25, importCommandTimeoutMilliseconds: 50, - updateCommandTimeoutMilliseconds: 150 + updateSourceTimeoutMilliseconds: 75, + updateCommandMaximumTimeoutMilliseconds: 150 ) } - let response = try await bridge.updateSources(["hugohe3-ppt-master"]) + let response = try await bridge.updateSources(["alpha", "beta"]) XCTAssertEqual(response.command, BridgeCommand.update) XCTAssertTrue(response.ok) } + func testSingleSourceUpdateUsesOneSourceBudget() async throws { + let fixture = try SlowBridgeFixture.install(delayMilliseconds: 100) + self.fixture = fixture + + let bridge = await MainActor.run { + BridgeClient( + updateSourceTimeoutMilliseconds: 50, + updateCommandMaximumTimeoutMilliseconds: 150 + ) + } + + do { + _ = try await bridge.updateSources(["alpha"]) + XCTFail("Expected one source update to use one source budget.") + } catch { + XCTAssertEqual(error.localizedDescription, "Operation timed out after 50ms.") + } + } + + func testUpdateAllUsesMaximumUpdateBudget() async throws { + let fixture = try SlowBridgeFixture.install(delayMilliseconds: 100) + self.fixture = fixture + + let bridge = await MainActor.run { + BridgeClient( + updateSourceTimeoutMilliseconds: 50, + updateCommandMaximumTimeoutMilliseconds: 150 + ) + } + + let response = try await bridge.updateAll() + + XCTAssertEqual(response.command, BridgeCommand.update) + XCTAssertTrue(response.ok) + } + + func testSelectedUpdateBudgetDoesNotExceedMaximum() async throws { + let fixture = try SlowBridgeFixture.install(delayMilliseconds: 150) + self.fixture = fixture + + let bridge = await MainActor.run { + BridgeClient( + updateSourceTimeoutMilliseconds: 50, + updateCommandMaximumTimeoutMilliseconds: 100 + ) + } + + do { + _ = try await bridge.updateSources(["alpha", "beta", "gamma", "delta"]) + XCTFail("Expected selected update budget to stop at its maximum.") + } catch { + XCTAssertEqual(error.localizedDescription, "Operation timed out after 100ms.") + } + } + func testTimedOutHelperIsForceKilledWhenItIgnoresTerminate() async throws { let fixture = try StubbornBridgeFixture.install() stubbornFixture = fixture diff --git a/packages/core-engine/src/services/source-authority-service.ts b/packages/core-engine/src/services/source-authority-service.ts index 35b0ec5..347cafd 100644 --- a/packages/core-engine/src/services/source-authority-service.ts +++ b/packages/core-engine/src/services/source-authority-service.ts @@ -327,6 +327,19 @@ export class SourceAuthorityService { continue; } + const sourceRoot = path.join(this.options.stateStore.rootPath, "source"); + const expectedCheckoutPath = path.join(sourceRoot, source.kind, sourceId); + const normalizedLocalPath = path.resolve(lock.localPath); + if ( + normalizedLocalPath !== path.resolve(expectedCheckoutPath) + || !isPathInside(sourceRoot, normalizedLocalPath) + ) { + return fail({ + code: "SOURCE_CHECKOUT_PATH_INVALID", + message: `Refusing to update checkout with mismatched managed path: ${lock.localPath}`, + }, warnings); + } + const lockedCommit = this.readLockedCommitSha(lock.revision); let repairReason: SourceRepairReason | undefined; if (source.kind === "git" && lockedCommit) { diff --git a/packages/core-engine/src/services/source-checkout-service.ts b/packages/core-engine/src/services/source-checkout-service.ts index 67113f9..2e3c6bc 100644 --- a/packages/core-engine/src/services/source-checkout-service.ts +++ b/packages/core-engine/src/services/source-checkout-service.ts @@ -26,6 +26,7 @@ import { fetchWithTimeout, withNetworkRetries, } from "@skill-flow/integration/utils/fetch-timeout"; +import { resolveGitHubTreePath } from "@skill-flow/integration/utils/github-catalog"; import { git, isGitAvailable } from "@skill-flow/integration/utils/git"; import { parseGitHubRepo, parseHostedGitRepo } from "@skill-flow/integration/utils/naming"; import { fail, ok } from "@skill-flow/integration/utils/result"; @@ -325,7 +326,7 @@ export class SourceCheckoutService { }; } - const treeLocator = this.parseTreeLocator(trimmed); + const treeLocator = await this.parseTreeLocator(trimmed); if (treeLocator) { const requestedPath = this.joinRequestedPaths( treeLocator.requestedPath, @@ -727,52 +728,55 @@ export class SourceCheckoutService { return "git"; } - private parseTreeLocator( + private async parseTreeLocator( locator: string, - ): { repoLocator: string; requestedPath?: string; originBranch?: string } | null { + ): Promise<{ repoLocator: string; requestedPath?: string; originBranch?: string } | null> { + let url: URL; try { - const url = new URL(locator); - - const parts = url.pathname.split("/").filter(Boolean); - if (url.hostname === "github.com") { - if (parts.length < 5 || parts[2] !== "tree") { - return null; - } + url = new URL(locator); + } catch { + return null; + } - const owner = parts[0]; - const repo = parts[1]; - const originBranch = parts[3]; - const requestedPath = parts.slice(4).join("/"); - if (!owner || !repo || !originBranch || !requestedPath) { - return null; - } + const parts = url.pathname.split("/").filter(Boolean); + if (url.hostname === "github.com") { + if (parts.length < 5 || parts[2] !== "tree") { + return null; + } - return { - repoLocator: `https://github.com/${owner}/${repo}.git`, - requestedPath, - originBranch, - }; + const owner = parts[0]; + const repo = parts[1]; + const treePath = parts.slice(3).join("/"); + if (!owner || !repo || !treePath) { + return null; } - if (url.hostname === "gitlab.com") { - const markerIndex = parts.findIndex( - (segment, index) => segment === "-" && parts[index + 1] === "tree", - ); - if (markerIndex < 2) { - return null; - } + const repoLocator = `https://github.com/${owner}/${repo}.git`; + const resolvedTreePath = await resolveGitHubTreePath(repoLocator, treePath); - const originBranch = parts[markerIndex + 2]; - const requestedPath = parts.slice(markerIndex + 3).join("/"); + return { + repoLocator, + requestedPath: resolvedTreePath.requestedPath, + originBranch: resolvedTreePath.branch, + }; + } - return { - repoLocator: `https://gitlab.com/${parts.slice(0, markerIndex).join("/")}.git`, - ...(requestedPath ? { requestedPath } : {}), - ...(originBranch ? { originBranch } : {}), - }; + if (url.hostname === "gitlab.com") { + const markerIndex = parts.findIndex( + (segment, index) => segment === "-" && parts[index + 1] === "tree", + ); + if (markerIndex < 2) { + return null; } - } catch { - return null; + + const originBranch = parts[markerIndex + 2]; + const requestedPath = parts.slice(markerIndex + 3).join("/"); + + return { + repoLocator: `https://gitlab.com/${parts.slice(0, markerIndex).join("/")}.git`, + ...(requestedPath ? { requestedPath } : {}), + ...(originBranch ? { originBranch } : {}), + }; } return null; diff --git a/packages/core-engine/src/tests/source-authority-service.test.ts b/packages/core-engine/src/tests/source-authority-service.test.ts index 0b76195..029e2d9 100644 --- a/packages/core-engine/src/tests/source-authority-service.test.ts +++ b/packages/core-engine/src/tests/source-authority-service.test.ts @@ -217,6 +217,45 @@ describe.sequential("SourceAuthorityService", () => { ]); }); + test("refuses to update a source whose checkout path does not match its v2 identity", async () => { + const repoPath = await createRepo(sandbox.sandboxRoot, { + "skills/one/SKILL.md": skillDoc("one", "One."), + }); + const externalPath = path.join(sandbox.sandboxRoot, "external-checkout"); + await fs.mkdir(externalPath, { recursive: true }); + await fs.writeFile(path.join(externalPath, "sentinel.txt"), "keep", "utf8"); + const stateStore = new StateStore(sandbox.stateRoot); + await stateStore.init(); + const checkoutService = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + const service = new SourceAuthorityService({ stateStore, checkoutService }); + const added = await service.addSource(repoPath, { + sourceIdOverride: "unsafe-update-source", + }); + expect(added.ok).toBe(true); + + const state = await stateStore.readState(); + state.lockFile.sources["unsafe-update-source"] = { + ...state.lockFile.sources["unsafe-update-source"]!, + localPath: externalPath, + }; + await stateStore.writeState(state); + const prepareSourceCheckout = vi.spyOn(checkoutService, "prepareSourceCheckout"); + + const updated = await service.updateSources(["unsafe-update-source"]); + + expect(updated.ok).toBe(false); + if (updated.ok) { + return; + } + expect(updated.errors[0]?.code).toBe("SOURCE_CHECKOUT_PATH_INVALID"); + expect(prepareSourceCheckout).not.toHaveBeenCalled(); + await expect(fs.readFile(path.join(externalPath, "sentinel.txt"), "utf8")) + .resolves.toBe("keep"); + }); + test("updateSources skips healthy git sources and repairs local drift", async () => { const stateStore = new StateStore(sandbox.stateRoot); await stateStore.init(); @@ -389,6 +428,17 @@ describe.sequential("SourceAuthorityService", () => { } const state = await stateStore.readState(); + const gitCheckoutPath = path.join( + sandbox.stateRoot, + "source", + "git", + "git-fallback", + ); + await fs.mkdir(path.dirname(gitCheckoutPath), { recursive: true }); + await fs.rename( + state.lockFile.sources["git-fallback"]!.localPath, + gitCheckoutPath, + ); state.manifest.sources[0] = { ...state.manifest.sources[0]!, kind: "git", @@ -396,6 +446,7 @@ describe.sequential("SourceAuthorityService", () => { }; state.lockFile.sources["git-fallback"] = { ...state.lockFile.sources["git-fallback"]!, + localPath: gitCheckoutPath, revision: { provider: "git", commit: "same-sha", capturedAt: new Date().toISOString() }, }; await stateStore.writeState(state); diff --git a/packages/core-engine/src/tests/source-checkout-service.test.ts b/packages/core-engine/src/tests/source-checkout-service.test.ts index cf693cf..4a21d33 100644 --- a/packages/core-engine/src/tests/source-checkout-service.test.ts +++ b/packages/core-engine/src/tests/source-checkout-service.test.ts @@ -59,6 +59,72 @@ describe.sequential("SourceCheckoutService", () => { ); }); + test("resolves the longest matching GitHub branch in a tree URL", async () => { + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); + vi.spyOn(gitUtils, "git").mockResolvedValue([ + "0123456789abcdef0123456789abcdef01234567\trefs/heads/feature", + "89abcdef0123456789abcdef0123456789abcdef\trefs/heads/feature/foo", + ].join("\n")); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + await expect(service.resolveSource( + "https://github.com/acme/skills/tree/feature/foo/skills/one", + {}, + )).resolves.toMatchObject({ + kind: "git", + locator: "https://github.com/acme/skills.git", + originBranch: "feature/foo", + requestedPath: "skills/one", + }); + }); + + test("uses the GitHub API to resolve a tree branch when git is unavailable", async () => { + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(false); + const fetch = vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => { + const url = String(input); + return new Response("", { + status: url.endsWith("/branches/feature%2Ffoo") ? 200 : 404, + }); + }); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + await expect(service.resolveSource( + "https://github.com/acme/skills/tree/feature/foo/skills/one", + {}, + )).resolves.toMatchObject({ + originBranch: "feature/foo", + requestedPath: "skills/one", + }); + expect(fetch).toHaveBeenCalledWith( + "https://api.github.com/repos/acme/skills/branches/feature%2Ffoo%2Fskills", + expect.any(Object), + ); + expect(fetch).toHaveBeenCalledWith( + "https://api.github.com/repos/acme/skills/branches/feature%2Ffoo", + expect.any(Object), + ); + }); + + test("rejects a GitHub tree URL when its branch cannot be resolved", async () => { + vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(false); + vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("", { status: 404 })); + const service = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + + await expect(service.resolveSource( + "https://github.com/acme/skills/tree/missing/skills/one", + {}, + )).rejects.toThrow("Unable to resolve a GitHub branch"); + }); + test("reads remote HEAD commit for GitHub shorthand locators", async () => { vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); const git = vi.spyOn(gitUtils, "git").mockResolvedValue( @@ -334,6 +400,9 @@ describe.sequential("SourceCheckoutService", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { + if (args[0] === "ls-remote" && args[1] === "--heads") { + return "test-commit-sha\trefs/heads/main"; + } if ( args[0] === "clone" && args[3] === "--branch" diff --git a/packages/integration/src/utils/github-catalog.ts b/packages/integration/src/utils/github-catalog.ts index 60e64d2..edaf912 100644 --- a/packages/integration/src/utils/github-catalog.ts +++ b/packages/integration/src/utils/github-catalog.ts @@ -1,5 +1,6 @@ import type { SourceStats } from "@skill-flow/domain/types"; import { fetchWithTimeout } from "./fetch-timeout.js"; +import { git, isGitAvailable } from "./git.js"; import { parseGitHubRepo } from "./naming.js"; type GitHubTreeResponse = { @@ -20,6 +21,72 @@ type GitHubRepoResponse = { pushed_at?: string; }; +export type GitHubTreePathResolution = { + branch: string; + requestedPath: string; +}; + +export async function resolveGitHubTreePath( + locator: string, + treePath: string, +): Promise { + const repo = parseGitHubRepo(locator); + if (!repo) { + throw new Error(`Unsupported GitHub locator '${locator}'.`); + } + + const segments = treePath + .split("/") + .filter(Boolean) + .map((segment) => decodeURIComponent(segment)); + const candidates = Array.from( + { length: Math.max(segments.length - 1, 0) }, + (_, index) => { + const branchSegmentCount = segments.length - index - 1; + return { + branch: segments.slice(0, branchSegmentCount).join("/"), + requestedPath: segments.slice(branchSegmentCount).join("/"), + }; + }, + ); + if (candidates.length === 0) { + throw new Error(`GitHub tree URL '${treePath}' must include a branch and repository path.`); + } + + if (await isGitAvailable()) { + try { + const output = await git(["ls-remote", "--heads", locator], { timeoutMs: 5_000 }); + const branches = new Set( + output + .split(/\r?\n/) + .map((line) => line.trim().match(/\srefs\/heads\/(.+)$/)?.[1]) + .filter((branch): branch is string => Boolean(branch)), + ); + const matched = candidates.find((candidate) => branches.has(candidate.branch)); + if (matched) { + return matched; + } + } catch { + // GitHub's API keeps tree URL imports available when git probing fails. + } + } + + for (const candidate of candidates) { + const response = await fetchWithTimeout( + `https://api.github.com/repos/${repo.owner}/${repo.repo}/branches/${encodeURIComponent(candidate.branch)}`, + { headers: buildGitHubHeaders() }, + ); + if (response.ok) { + return candidate; + } + if (response.status !== 404) { + throw new Error(`GitHub branch request failed with ${response.status}.`); + } + } + + throw new Error(`Unable to resolve a GitHub branch from tree path '${treePath}'.`); +} + export async function fetchGitHubSkillPaths( locator: string, branch: string, diff --git a/packages/query/src/tests/source-lifecycle.test.ts b/packages/query/src/tests/source-lifecycle.test.ts index 030d4c2..3e1a164 100644 --- a/packages/query/src/tests/source-lifecycle.test.ts +++ b/packages/query/src/tests/source-lifecycle.test.ts @@ -879,6 +879,9 @@ describe.sequential("source lifecycle", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { + if (args[0] === "ls-remote" && args[1] === "--heads") { + return "test-commit-sha\trefs/heads/main"; + } if ( args[0] === "clone" && args[3] === "--branch" @@ -925,6 +928,9 @@ describe.sequential("source lifecycle", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { + if (args[0] === "ls-remote" && args[1] === "--heads") { + return "test-commit-sha\trefs/heads/main"; + } if ( args[0] === "clone" && args[3] === "--branch" @@ -1509,6 +1515,12 @@ description: | return; } const commit = await gitUtils.git(["rev-parse", "HEAD"], { cwd: repoPath }); + const gitCheckoutPath = path.join( + sandbox.stateRoot, + "source", + "git", + sourceId, + ); await v2(app).writeState({ ...before, manifest: { @@ -1523,6 +1535,7 @@ description: | ...before.lockFile.sources, [sourceId]: { ...before.lockFile.sources[sourceId]!, + localPath: gitCheckoutPath, revision: { provider: "git", commit, capturedAt: new Date().toISOString() }, }, }, From e371ba1015198f4f075aa526cd420ce7a854bd3b Mon Sep 17 00:00:00 2001 From: Qize Liu Date: Tue, 18 Aug 2026 14:28:25 +0800 Subject: [PATCH 4/6] Document git source update boundaries --- docs/FEATURE_INDEX.md | 1 + docs/PRODUCT.md | 4 +- docs/contracts/README.md | 6 ++ ...6-03-import-timeout-and-feedback-design.md | 12 ++- ...-18-git-source-update-boundaries-design.md | 74 +++++++++++++++++++ 5 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md diff --git a/docs/FEATURE_INDEX.md b/docs/FEATURE_INDEX.md index 1012c70..7e802f8 100644 --- a/docs/FEATURE_INDEX.md +++ b/docs/FEATURE_INDEX.md @@ -18,6 +18,7 @@ | Desktop DMG update flow | Planned | - | [plan](superpowers/plans/2026-06-24-desktop-dmg-update-flow.md) | - | | Local scan import source path policy | Planned | [spec](superpowers/specs/2026-06-24-local-scan-import-source-path-policy-design.md) | [plan](superpowers/plans/2026-06-24-local-scan-import-source-path-policy.md) | - | | CLI migration usability | Implemented | [spec](superpowers/specs/2026-06-26-cli-migration-usability-design.md) | [plan](superpowers/plans/2026-06-26-cli-migration-usability.md) | - | +| Git source update boundaries | Implemented | [spec](superpowers/specs/2026-08-18-git-source-update-boundaries-design.md) | - | Focused source, query, and desktop bridge tests | | Plugin assets and target routing | Issue | - | [analysis](issues/ISSUE_8_plugin_assets_and_target_routing.md), [user report](issues/ISSUE_8_plugin_assets_and_target_routing_user_report.md) | - | | Group card state matrix | Verification | - | - | [matrix](verification/GROUP_CARD_STATE_MATRIX.md) | | Mount lifecycle edge cases | Verification | - | - | [matrix](verification/mount-lifecycle-matrix.md) | diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index 911efe5..07deddd 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -31,6 +31,9 @@ Skill Flow 用来把分散在不同来源的 AI agent skills 管理成可检查 - `lock.json` 记录解析结果、source snapshot 和部署投影。 - Target 目录是生成输出,不是事实源。 - macOS 桌面端依赖 CLI bridge 协议,不另建第二套状态模型。 +- GitHub tree URL 的 branch/path 边界通过远端分支确认;无法确认时明确失败,不按路径片段猜测。 +- Managed update 只读写 `~/.skillflow/source//` 的规范 checkout;lock 路径不匹配时拒绝更新。 +- 桌面端更新始终有界:每个明确选择的 source 预算 5 分钟,总上限 15 分钟;全部更新使用 15 分钟。 ## 非目标 @@ -48,4 +51,3 @@ Skill Flow 用来把分散在不同来源的 AI agent skills 管理成可检查 - bridge protocol request/response。 - desktop bridge payload 或打包产物行为。 - README、release notes 中承诺的用户流程。 - diff --git a/docs/contracts/README.md b/docs/contracts/README.md index cda0ea3..b441530 100644 --- a/docs/contracts/README.md +++ b/docs/contracts/README.md @@ -52,6 +52,12 @@ save-settings Bridge changes are external changes. Update parser tests, CLI bridge behavior, desktop bridge models, and release/user docs when changing this surface. +Desktop helper execution is always time-bounded. Ordinary commands use 60 +seconds, import/add commands use 5 minutes, and managed update scales by the +number of explicitly selected sources at 5 minutes each with a 15-minute +ceiling. Update-all uses the 15-minute ceiling. These budgets are desktop +process behavior and do not change the protocol payload shape. + When adding, removing, or renaming a bridge command: 1. Update `BRIDGE_COMMAND_NAMES` in `packages/shared-types/src/protocol.ts`. diff --git a/docs/superpowers/specs/2026-06-03-import-timeout-and-feedback-design.md b/docs/superpowers/specs/2026-06-03-import-timeout-and-feedback-design.md index e7acc38..236e7c7 100644 --- a/docs/superpowers/specs/2026-06-03-import-timeout-and-feedback-design.md +++ b/docs/superpowers/specs/2026-06-03-import-timeout-and-feedback-design.md @@ -39,9 +39,15 @@ The Import page seeds recommended cards from bundled `recommendations.json`. Car ### Bridge Timeout -Add a fixed timeout to `BridgeClient.send`, with a default of 60 seconds. If the helper process has not exited by then, terminate it, clear pipe handlers, and throw `BridgeClientError.timeout(timeoutMs)`. +Add a bounded timeout to `BridgeClient.send`. Ordinary commands use 60 seconds, +network-heavy import/add commands use 5 minutes, and managed update uses 5 +minutes per explicitly selected source with a 15-minute ceiling. Update-all +uses the 15-minute ceiling. If the helper process has not exited by its active +budget, terminate it, clear pipe handlers, and throw +`BridgeClientError.timeout(timeoutMs)`. -The timeout should cover all bridge commands. This is acceptable because desktop actions should not wait indefinitely. Long commands can later receive command-specific budgets if needed. +The timeout must cover all bridge commands. Command-specific budgets may be +longer than the ordinary default, but no desktop action may wait indefinitely. Implementation detail: @@ -112,7 +118,7 @@ If the Swift test harness cannot easily spawn a hanging helper, cover bridge tim ## Risks -- A 60 second bridge timeout can interrupt a legitimately slow import on poor networks. This is acceptable for desktop UX because indefinite loading is worse, and the user can retry. +- A bounded bridge timeout can interrupt a legitimately slow network action. Import and update receive larger command-specific budgets, while indefinite loading remains disallowed. - Timeout behavior in Swift subprocess handling must avoid leaving pipe readability handlers attached. - Adding a fetch helper touches shared integration code. Keep the helper small and avoid broad refactors. diff --git a/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md b/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md new file mode 100644 index 0000000..90f66a7 --- /dev/null +++ b/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md @@ -0,0 +1,74 @@ +# Git Source Update Boundaries Design + +## Goal + +Make managed source updates faster without weakening the ownership, locator, +or desktop process boundaries documented by the upstream architecture. + +## Scope + +This change is limited to managed source update behavior: + +- preserve the remote-ref precheck and reuse an existing managed Git object store; +- resolve GitHub tree URLs against confirmed remote branch names; +- reject update when a managed lock points outside its canonical checkout path; +- keep every desktop bridge update bounded. + +It does not expand GitLab tree URL behavior and does not change reconcile, +repair, deployment, or external-source lifecycle rules. + +## Contracts + +### Managed checkout ownership + +Before any update preflight, checkout reuse, rename, or replacement, a managed +lock path must equal: + +```text +/source// +``` + +The resolved path must also remain inside `/source`. A mismatch +returns `SOURCE_CHECKOUT_PATH_INVALID` without reading or modifying that path. +External sources continue to leave the managed update path before this check. + +### GitHub tree URLs + +The text after `/tree/` is ambiguous when branch names contain `/`. Resolve it +by generating branch/path splits from longest branch candidate to shortest: + +1. If Git is available, compare candidates with `git ls-remote --heads`. +2. Otherwise, or when the Git probe fails, confirm candidates through the + GitHub branch API. +3. Use the longest confirmed branch and keep the remaining suffix as the + requested repository path. +4. If no branch is confirmed, fail clearly instead of guessing. + +The confirmed branch is stored as `originBranch` and reused by precheck, +clone, fetch, and archive fallback. + +### Desktop update budget + +Every bridge command remains time-bounded. Managed update uses a dedicated +budget based on the distinct selected source count: + +- one source: 5 minutes; +- two sources: 10 minutes; +- three or more sources: 15 minutes; +- update all: 15 minutes. + +Tests may inject shorter budgets. Timeout termination and output handling stay +the same as other bridge commands. + +## Acceptance tests + +1. A GitHub tree URL whose branch is `feature/foo` resolves that complete + branch and preserves the remaining skill path. +2. GitHub API fallback provides the same resolution when Git is unavailable. +3. An unresolvable tree branch fails instead of using the first path segment. +4. Managed update rejects a mismatched `lock.localPath` before checkout + preparation and leaves the referenced directory untouched. +5. One- and two-source desktop updates receive 5- and 10-minute budgets, while + update-all and larger selections never exceed 15 minutes. +6. External sources remain excluded from managed update before any checkout + operation. From 8ecd92cce2fc3364a717755a7be5026ec67109b6 Mon Sep 17 00:00:00 2001 From: Qize Liu Date: Tue, 18 Aug 2026 22:42:43 +0800 Subject: [PATCH 5/6] Narrow git update fast path to managed checkouts --- docs/PRODUCT.md | 3 +- ...-18-git-source-update-boundaries-design.md | 61 +++++----- .../src/services/source-authority-service.ts | 63 +++++++++- .../src/services/source-checkout-service.ts | 107 ++++++++--------- .../tests/source-authority-service.test.ts | 38 ++++++ .../src/tests/source-checkout-service.test.ts | 108 +++++------------- .../integration/src/utils/github-catalog.ts | 67 ----------- .../query/src/tests/source-lifecycle.test.ts | 25 +--- 8 files changed, 220 insertions(+), 252 deletions(-) diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index 07deddd..00dc97c 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -31,8 +31,7 @@ Skill Flow 用来把分散在不同来源的 AI agent skills 管理成可检查 - `lock.json` 记录解析结果、source snapshot 和部署投影。 - Target 目录是生成输出,不是事实源。 - macOS 桌面端依赖 CLI bridge 协议,不另建第二套状态模型。 -- GitHub tree URL 的 branch/path 边界通过远端分支确认;无法确认时明确失败,不按路径片段猜测。 -- Managed update 只读写 `~/.skillflow/source//` 的规范 checkout;lock 路径不匹配时拒绝更新。 +- Managed update 只读写 `~/.skillflow/source//` 的规范 checkout;lock 路径不匹配或 checkout 路径链包含符号链接时拒绝更新。 - 桌面端更新始终有界:每个明确选择的 source 预算 5 分钟,总上限 15 分钟;全部更新使用 15 分钟。 ## 非目标 diff --git a/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md b/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md index 90f66a7..1e6ef93 100644 --- a/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md +++ b/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md @@ -10,12 +10,13 @@ or desktop process boundaries documented by the upstream architecture. This change is limited to managed source update behavior: - preserve the remote-ref precheck and reuse an existing managed Git object store; -- resolve GitHub tree URLs against confirmed remote branch names; -- reject update when a managed lock points outside its canonical checkout path; +- preserve an already-recorded `originBranch` throughout the update; +- reject update when a managed lock points outside its canonical checkout path + or that path uses symbolic links; - keep every desktop bridge update bounded. -It does not expand GitLab tree URL behavior and does not change reconcile, -repair, deployment, or external-source lifecycle rules. +It does not change GitHub or GitLab locator parsing, reconcile, repair, +deployment, or external-source lifecycle rules. ## Contracts @@ -28,24 +29,30 @@ lock path must equal: /source// ``` -The resolved path must also remain inside `/source`. A mismatch -returns `SOURCE_CHECKOUT_PATH_INVALID` without reading or modifying that path. -External sources continue to leave the managed update path before this check. +The resolved path must also remain inside `/source`. The managed +path components must not be symbolic links, and their real paths must remain +under the real source root. A mismatch returns +`SOURCE_CHECKOUT_PATH_INVALID` without reading or modifying the referenced +checkout. External sources continue to leave the managed update path before +this check. -### GitHub tree URLs +### Git update path -The text after `/tree/` is ambiguous when branch names contain `/`. Resolve it -by generating branch/path splits from longest branch candidate to shortest: +When a lock has an `originBranch`, remote precheck, local-object fetch, clean +clone, and archive fallback use that exact branch. Failure must not silently +switch to `main` or `master`. Locks without `originBranch` retain the upstream +default-branch behavior. -1. If Git is available, compare candidates with `git ls-remote --heads`. -2. Otherwise, or when the Git probe fails, confirm candidates through the - GitHub branch API. -3. Use the longest confirmed branch and keep the remaining suffix as the - requested repository path. -4. If no branch is confirmed, fail clearly instead of guessing. +For a managed Git checkout, first attempt a local no-checkout clone to reuse +its object store, then fetch the remote ref and detach at `FETCH_HEAD`. If this +optimization fails, remove the temporary checkout and use the upstream clean +clone, HTTPS fallback, and archive fallback sequence. -The confirmed branch is stored as `originBranch` and reused by precheck, -clone, fetch, and archive fallback. +### Mutation lock ownership + +A lock with a live PID is never reclaimed solely because its timestamp has +crossed the stale threshold. A dead owner may be reclaimed immediately, while +missing or unreadable ownership metadata retains the upstream age-based rule. ### Desktop update budget @@ -62,13 +69,15 @@ the same as other bridge commands. ## Acceptance tests -1. A GitHub tree URL whose branch is `feature/foo` resolves that complete - branch and preserves the remaining skill path. -2. GitHub API fallback provides the same resolution when Git is unavailable. -3. An unresolvable tree branch fails instead of using the first path segment. -4. Managed update rejects a mismatched `lock.localPath` before checkout - preparation and leaves the referenced directory untouched. -5. One- and two-source desktop updates receive 5- and 10-minute budgets, while +1. An unchanged remote commit skips checkout preparation. +2. A changed Git source reuses the existing managed object store. +3. A failed local reuse falls back to the upstream clean-fetch sequence. +4. A recorded `originBranch` is used by every update fetch path without + switching to a default branch. +5. Managed update rejects mismatched and symbolic-link checkout paths before + checkout preparation and leaves referenced external directories untouched. +6. A live process lock is not reclaimed after five minutes. +7. One- and two-source desktop updates receive 5- and 10-minute budgets, while update-all and larger selections never exceed 15 minutes. -6. External sources remain excluded from managed update before any checkout +8. External sources remain excluded from managed update before any checkout operation. diff --git a/packages/core-engine/src/services/source-authority-service.ts b/packages/core-engine/src/services/source-authority-service.ts index 347cafd..029306f 100644 --- a/packages/core-engine/src/services/source-authority-service.ts +++ b/packages/core-engine/src/services/source-authority-service.ts @@ -329,14 +329,14 @@ export class SourceAuthorityService { const sourceRoot = path.join(this.options.stateStore.rootPath, "source"); const expectedCheckoutPath = path.join(sourceRoot, source.kind, sourceId); - const normalizedLocalPath = path.resolve(lock.localPath); - if ( - normalizedLocalPath !== path.resolve(expectedCheckoutPath) - || !isPathInside(sourceRoot, normalizedLocalPath) - ) { + if (!await this.isManagedCheckoutPathValid( + sourceRoot, + expectedCheckoutPath, + lock.localPath, + )) { return fail({ code: "SOURCE_CHECKOUT_PATH_INVALID", - message: `Refusing to update checkout with mismatched managed path: ${lock.localPath}`, + message: `Refusing to update checkout with invalid managed path: ${lock.localPath}`, }, warnings); } @@ -392,6 +392,7 @@ export class SourceAuthorityService { }, checkoutPath: tempCheckoutPath, existingCheckoutPath: lock.localPath, + ...(lock.originBranch ? { updateBranch: lock.originBranch } : {}), allowEmptyLeafs: true, }); if (!prepared.ok) { @@ -488,6 +489,56 @@ export class SourceAuthorityService { }, warnings); } + private async isManagedCheckoutPathValid( + sourceRoot: string, + expectedCheckoutPath: string, + localPath: string, + ): Promise { + const normalizedLocalPath = path.resolve(localPath); + if ( + normalizedLocalPath !== path.resolve(expectedCheckoutPath) + || !isPathInside(sourceRoot, normalizedLocalPath) + ) { + return false; + } + + const kindRoot = path.dirname(expectedCheckoutPath); + for (const managedPath of [sourceRoot, kindRoot]) { + try { + if ((await fs.lstat(managedPath)).isSymbolicLink()) { + return false; + } + } catch { + return false; + } + } + + const checkoutExists = await pathExists(expectedCheckoutPath); + if (checkoutExists) { + try { + if ((await fs.lstat(expectedCheckoutPath)).isSymbolicLink()) { + return false; + } + } catch { + return false; + } + } + + try { + const realSourceRoot = await fs.realpath(sourceRoot); + const realKindRoot = await fs.realpath(kindRoot); + if (!isPathInside(realSourceRoot, realKindRoot)) { + return false; + } + if (!checkoutExists) { + return true; + } + return isPathInside(realSourceRoot, await fs.realpath(expectedCheckoutPath)); + } catch { + return false; + } + } + async reconcileInventory( sourceIds?: string[], options: { force?: boolean } = {}, diff --git a/packages/core-engine/src/services/source-checkout-service.ts b/packages/core-engine/src/services/source-checkout-service.ts index 2e3c6bc..b5ac72c 100644 --- a/packages/core-engine/src/services/source-checkout-service.ts +++ b/packages/core-engine/src/services/source-checkout-service.ts @@ -26,7 +26,6 @@ import { fetchWithTimeout, withNetworkRetries, } from "@skill-flow/integration/utils/fetch-timeout"; -import { resolveGitHubTreePath } from "@skill-flow/integration/utils/github-catalog"; import { git, isGitAvailable } from "@skill-flow/integration/utils/git"; import { parseGitHubRepo, parseHostedGitRepo } from "@skill-flow/integration/utils/naming"; import { fail, ok } from "@skill-flow/integration/utils/result"; @@ -98,7 +97,6 @@ type SourceResolution = { clawhubSlug?: string; requestedVersion?: string; versionMode?: "pinned" | "floating"; - originBranch?: string; }; export class SourceCheckoutService { @@ -157,6 +155,7 @@ export class SourceCheckoutService { existingSources?: Array<{ id: string; kind?: SourceKind; locator: string; displayName: string }>; checkoutPath?: string; existingCheckoutPath?: string; + updateBranch?: string; suffix?: string; allowEmptyLeafs?: boolean; } = {}, @@ -174,7 +173,12 @@ export class SourceCheckoutService { await ensureDir(path.dirname(checkoutPath)); try { - await this.fetchSource(resolved, checkoutPath, input.existingCheckoutPath); + await this.fetchSource( + resolved, + checkoutPath, + input.existingCheckoutPath, + input.updateBranch, + ); } catch (error) { await removePath(checkoutPath); return fail({ @@ -190,9 +194,7 @@ export class SourceCheckoutService { resolved.displayName, checkoutPath, resolved.requestedPath, - resolved.originBranch && !options.originBranch - ? { ...options, originBranch: resolved.originBranch } - : options, + options, input.allowEmptyLeafs === undefined ? {} : { allowEmptyLeafs: input.allowEmptyLeafs }, ); if (!snapshot.ok) { @@ -326,7 +328,7 @@ export class SourceCheckoutService { }; } - const treeLocator = await this.parseTreeLocator(trimmed); + const treeLocator = this.parseTreeLocator(trimmed); if (treeLocator) { const requestedPath = this.joinRequestedPaths( treeLocator.requestedPath, @@ -339,7 +341,6 @@ export class SourceCheckoutService { displayName: options.displayNameOverride ?? deriveDisplayName(treeLocator.repoLocator), sourceId: options.sourceIdOverride ?? deriveSourceId(treeLocator.repoLocator), ...(requestedPath ? { requestedPath } : {}), - ...(treeLocator.originBranch ? { originBranch: treeLocator.originBranch } : {}), }; } @@ -356,7 +357,6 @@ export class SourceCheckoutService { displayName: options.displayNameOverride ?? deriveDisplayName(shorthandLocator.repoLocator), sourceId: options.sourceIdOverride ?? deriveSourceId(shorthandLocator.repoLocator), ...(requestedPath ? { requestedPath } : {}), - ...(options.originBranch ? { originBranch: options.originBranch } : {}), }; } @@ -389,7 +389,6 @@ export class SourceCheckoutService { displayName: options.displayNameOverride ?? deriveDisplayName(locator), sourceId: options.sourceIdOverride ?? deriveSourceId(locator), ...(requestedPath ? { requestedPath } : {}), - ...(options.originBranch ? { originBranch: options.originBranch } : {}), }; } @@ -478,6 +477,7 @@ export class SourceCheckoutService { source: SourceResolution, checkoutPath: string, existingCheckoutPath?: string, + updateBranch?: string, ): Promise { if (source.kind === "local") { await copyDirectory(source.localPath!, checkoutPath); @@ -494,14 +494,14 @@ export class SourceCheckoutService { source.gitLocator!, existingCheckoutPath, checkoutPath, - source.originBranch, + updateBranch, ); return; } catch { await removePath(checkoutPath).catch(() => {}); } } - await this.fetchGitSource(source.gitLocator!, checkoutPath, source.originBranch); + await this.fetchGitSource(source.gitLocator!, checkoutPath, updateBranch); return; } @@ -728,55 +728,48 @@ export class SourceCheckoutService { return "git"; } - private async parseTreeLocator( + private parseTreeLocator( locator: string, - ): Promise<{ repoLocator: string; requestedPath?: string; originBranch?: string } | null> { - let url: URL; + ): { repoLocator: string; requestedPath?: string } | null { try { - url = new URL(locator); - } catch { - return null; - } + const url = new URL(locator); - const parts = url.pathname.split("/").filter(Boolean); - if (url.hostname === "github.com") { - if (parts.length < 5 || parts[2] !== "tree") { - return null; - } + const parts = url.pathname.split("/").filter(Boolean); + if (url.hostname === "github.com") { + if (parts.length < 5 || parts[2] !== "tree") { + return null; + } - const owner = parts[0]; - const repo = parts[1]; - const treePath = parts.slice(3).join("/"); - if (!owner || !repo || !treePath) { - return null; + const owner = parts[0]; + const repo = parts[1]; + const requestedPath = parts.slice(4).join("/"); + if (!owner || !repo || !requestedPath) { + return null; + } + + return { + repoLocator: `https://github.com/${owner}/${repo}.git`, + requestedPath, + }; } - const repoLocator = `https://github.com/${owner}/${repo}.git`; - const resolvedTreePath = await resolveGitHubTreePath(repoLocator, treePath); + if (url.hostname === "gitlab.com") { + const markerIndex = parts.findIndex( + (segment, index) => segment === "-" && parts[index + 1] === "tree", + ); + if (markerIndex < 2) { + return null; + } - return { - repoLocator, - requestedPath: resolvedTreePath.requestedPath, - originBranch: resolvedTreePath.branch, - }; - } + const requestedPath = parts.slice(markerIndex + 3).join("/"); - if (url.hostname === "gitlab.com") { - const markerIndex = parts.findIndex( - (segment, index) => segment === "-" && parts[index + 1] === "tree", - ); - if (markerIndex < 2) { - return null; + return { + repoLocator: `https://gitlab.com/${parts.slice(0, markerIndex).join("/")}.git`, + ...(requestedPath ? { requestedPath } : {}), + }; } - - const originBranch = parts[markerIndex + 2]; - const requestedPath = parts.slice(markerIndex + 3).join("/"); - - return { - repoLocator: `https://gitlab.com/${parts.slice(0, markerIndex).join("/")}.git`, - ...(requestedPath ? { requestedPath } : {}), - ...(originBranch ? { originBranch } : {}), - }; + } catch { + return null; } return null; @@ -913,6 +906,16 @@ export class SourceCheckoutService { locator, checkoutPath, ]); + if (originBranch) { + await git( + ["fetch", "--depth", "1", "origin", this.remoteRef(originBranch)], + { cwd: checkoutPath }, + ); + await git( + ["checkout", "--detach", "--force", "FETCH_HEAD"], + { cwd: checkoutPath }, + ); + } }, { attempts: 2 }); } diff --git a/packages/core-engine/src/tests/source-authority-service.test.ts b/packages/core-engine/src/tests/source-authority-service.test.ts index 029e2d9..5692316 100644 --- a/packages/core-engine/src/tests/source-authority-service.test.ts +++ b/packages/core-engine/src/tests/source-authority-service.test.ts @@ -256,6 +256,44 @@ describe.sequential("SourceAuthorityService", () => { .resolves.toBe("keep"); }); + test("refuses to update a canonical checkout path that is a symbolic link", async () => { + const repoPath = await createRepo(sandbox.sandboxRoot, { + "skills/one/SKILL.md": skillDoc("one", "One."), + }); + const externalPath = path.join(sandbox.sandboxRoot, "external-symlink-target"); + await fs.mkdir(externalPath, { recursive: true }); + await fs.writeFile(path.join(externalPath, "sentinel.txt"), "keep", "utf8"); + const stateStore = new StateStore(sandbox.stateRoot); + await stateStore.init(); + const checkoutService = new SourceCheckoutService({ + sourceRoot: path.join(sandbox.stateRoot, "source"), + inventoryService: new InventoryService(), + }); + const service = new SourceAuthorityService({ stateStore, checkoutService }); + const added = await service.addSource(repoPath, { + sourceIdOverride: "symlink-update-source", + }); + expect(added.ok).toBe(true); + if (!added.ok) { + return; + } + + await fs.rm(added.data.lock.localPath, { recursive: true, force: true }); + await fs.symlink(externalPath, added.data.lock.localPath); + const prepareSourceCheckout = vi.spyOn(checkoutService, "prepareSourceCheckout"); + + const updated = await service.updateSources(["symlink-update-source"]); + + expect(updated.ok).toBe(false); + if (updated.ok) { + return; + } + expect(updated.errors[0]?.code).toBe("SOURCE_CHECKOUT_PATH_INVALID"); + expect(prepareSourceCheckout).not.toHaveBeenCalled(); + await expect(fs.readFile(path.join(externalPath, "sentinel.txt"), "utf8")) + .resolves.toBe("keep"); + }); + test("updateSources skips healthy git sources and repairs local drift", async () => { const stateStore = new StateStore(sandbox.stateRoot); await stateStore.init(); diff --git a/packages/core-engine/src/tests/source-checkout-service.test.ts b/packages/core-engine/src/tests/source-checkout-service.test.ts index 4a21d33..d406077 100644 --- a/packages/core-engine/src/tests/source-checkout-service.test.ts +++ b/packages/core-engine/src/tests/source-checkout-service.test.ts @@ -59,72 +59,6 @@ describe.sequential("SourceCheckoutService", () => { ); }); - test("resolves the longest matching GitHub branch in a tree URL", async () => { - vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); - vi.spyOn(gitUtils, "git").mockResolvedValue([ - "0123456789abcdef0123456789abcdef01234567\trefs/heads/feature", - "89abcdef0123456789abcdef0123456789abcdef\trefs/heads/feature/foo", - ].join("\n")); - const service = new SourceCheckoutService({ - sourceRoot: path.join(sandbox.stateRoot, "source"), - inventoryService: new InventoryService(), - }); - - await expect(service.resolveSource( - "https://github.com/acme/skills/tree/feature/foo/skills/one", - {}, - )).resolves.toMatchObject({ - kind: "git", - locator: "https://github.com/acme/skills.git", - originBranch: "feature/foo", - requestedPath: "skills/one", - }); - }); - - test("uses the GitHub API to resolve a tree branch when git is unavailable", async () => { - vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(false); - const fetch = vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => { - const url = String(input); - return new Response("", { - status: url.endsWith("/branches/feature%2Ffoo") ? 200 : 404, - }); - }); - const service = new SourceCheckoutService({ - sourceRoot: path.join(sandbox.stateRoot, "source"), - inventoryService: new InventoryService(), - }); - - await expect(service.resolveSource( - "https://github.com/acme/skills/tree/feature/foo/skills/one", - {}, - )).resolves.toMatchObject({ - originBranch: "feature/foo", - requestedPath: "skills/one", - }); - expect(fetch).toHaveBeenCalledWith( - "https://api.github.com/repos/acme/skills/branches/feature%2Ffoo%2Fskills", - expect.any(Object), - ); - expect(fetch).toHaveBeenCalledWith( - "https://api.github.com/repos/acme/skills/branches/feature%2Ffoo", - expect.any(Object), - ); - }); - - test("rejects a GitHub tree URL when its branch cannot be resolved", async () => { - vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(false); - vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("", { status: 404 })); - const service = new SourceCheckoutService({ - sourceRoot: path.join(sandbox.stateRoot, "source"), - inventoryService: new InventoryService(), - }); - - await expect(service.resolveSource( - "https://github.com/acme/skills/tree/missing/skills/one", - {}, - )).rejects.toThrow("Unable to resolve a GitHub branch"); - }); - test("reads remote HEAD commit for GitHub shorthand locators", async () => { vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); const git = vi.spyOn(gitUtils, "git").mockResolvedValue( @@ -275,6 +209,7 @@ describe.sequential("SourceCheckoutService", () => { { checkoutPath, existingCheckoutPath, + updateBranch: "release", options: { sourceIdOverride: "git-existing", originBranch: "release" }, }, ); @@ -284,6 +219,10 @@ describe.sequential("SourceCheckoutService", () => { return; } expect(prepared.data.commitSha).toBe("fedcba9876543210fedcba9876543210fedcba98"); + expect(gitUtils.git).toHaveBeenCalledWith( + ["fetch", "--depth", "1", "origin", "refs/heads/release"], + { cwd: checkoutPath }, + ); await expect(fs.readFile( path.join(checkoutPath, "skills", "one", "SKILL.md"), "utf8", @@ -313,6 +252,20 @@ describe.sequential("SourceCheckoutService", () => { await fs.cp(upstreamRepo, checkoutPath, { recursive: true }); return ""; } + if ( + args[0] === "fetch" + && args[4] === "refs/heads/release" + && options?.cwd === checkoutPath + ) { + return ""; + } + if ( + args[0] === "checkout" + && args[1] === "--detach" + && options?.cwd === checkoutPath + ) { + return ""; + } if (args[0] === "rev-parse" && args[1] === "HEAD" && options?.cwd === checkoutPath) { return "abcdef0123456789abcdef0123456789abcdef01"; } @@ -328,6 +281,7 @@ describe.sequential("SourceCheckoutService", () => { { checkoutPath, existingCheckoutPath, + updateBranch: "release", options: { sourceIdOverride: "git-fallback", originBranch: "release" }, }, ); @@ -342,6 +296,10 @@ describe.sequential("SourceCheckoutService", () => { "https://github.com/acme/skills.git", checkoutPath, ]); + expect(git).toHaveBeenCalledWith( + ["fetch", "--depth", "1", "origin", "refs/heads/release"], + { cwd: checkoutPath }, + ); await expect(fs.readFile( path.join(checkoutPath, "skills", "one", "SKILL.md"), "utf8", @@ -360,7 +318,10 @@ describe.sequential("SourceCheckoutService", () => { const prepared = await service.prepareSourceCheckout( "https://github.com/acme/skills.git", - { options: { sourceIdOverride: "locked-release", originBranch: "release" } }, + { + updateBranch: "release", + options: { sourceIdOverride: "locked-release", originBranch: "release" }, + }, ); expect(prepared.ok).toBe(false); @@ -400,16 +361,8 @@ describe.sequential("SourceCheckoutService", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { - if (args[0] === "ls-remote" && args[1] === "--heads") { - return "test-commit-sha\trefs/heads/main"; - } - if ( - args[0] === "clone" - && args[3] === "--branch" - && args[4] === "main" - && args[5] === "https://github.com/vercel-labs/skills.git" - ) { - await fs.cp(upstreamRepo, args[6]!, { recursive: true }); + if (args[0] === "clone" && args[3] === "https://github.com/vercel-labs/skills.git") { + await fs.cp(upstreamRepo, args[4]!, { recursive: true }); return ""; } @@ -434,7 +387,6 @@ describe.sequential("SourceCheckoutService", () => { return; } expect(prepared.data.kind).toBe("git"); - expect(prepared.data.originBranch).toBe("main"); expect(prepared.data.requestedPath).toBe("skills/find-skills"); expect(prepared.data.checkoutPath).toContain(`${path.sep}source${path.sep}git${path.sep}`); expect(prepared.data.leafs.map((leaf) => leaf.id)).toEqual([ diff --git a/packages/integration/src/utils/github-catalog.ts b/packages/integration/src/utils/github-catalog.ts index edaf912..60e64d2 100644 --- a/packages/integration/src/utils/github-catalog.ts +++ b/packages/integration/src/utils/github-catalog.ts @@ -1,6 +1,5 @@ import type { SourceStats } from "@skill-flow/domain/types"; import { fetchWithTimeout } from "./fetch-timeout.js"; -import { git, isGitAvailable } from "./git.js"; import { parseGitHubRepo } from "./naming.js"; type GitHubTreeResponse = { @@ -21,72 +20,6 @@ type GitHubRepoResponse = { pushed_at?: string; }; -export type GitHubTreePathResolution = { - branch: string; - requestedPath: string; -}; - -export async function resolveGitHubTreePath( - locator: string, - treePath: string, -): Promise { - const repo = parseGitHubRepo(locator); - if (!repo) { - throw new Error(`Unsupported GitHub locator '${locator}'.`); - } - - const segments = treePath - .split("/") - .filter(Boolean) - .map((segment) => decodeURIComponent(segment)); - const candidates = Array.from( - { length: Math.max(segments.length - 1, 0) }, - (_, index) => { - const branchSegmentCount = segments.length - index - 1; - return { - branch: segments.slice(0, branchSegmentCount).join("/"), - requestedPath: segments.slice(branchSegmentCount).join("/"), - }; - }, - ); - if (candidates.length === 0) { - throw new Error(`GitHub tree URL '${treePath}' must include a branch and repository path.`); - } - - if (await isGitAvailable()) { - try { - const output = await git(["ls-remote", "--heads", locator], { timeoutMs: 5_000 }); - const branches = new Set( - output - .split(/\r?\n/) - .map((line) => line.trim().match(/\srefs\/heads\/(.+)$/)?.[1]) - .filter((branch): branch is string => Boolean(branch)), - ); - const matched = candidates.find((candidate) => branches.has(candidate.branch)); - if (matched) { - return matched; - } - } catch { - // GitHub's API keeps tree URL imports available when git probing fails. - } - } - - for (const candidate of candidates) { - const response = await fetchWithTimeout( - `https://api.github.com/repos/${repo.owner}/${repo.repo}/branches/${encodeURIComponent(candidate.branch)}`, - { headers: buildGitHubHeaders() }, - ); - if (response.ok) { - return candidate; - } - if (response.status !== 404) { - throw new Error(`GitHub branch request failed with ${response.status}.`); - } - } - - throw new Error(`Unable to resolve a GitHub branch from tree path '${treePath}'.`); -} - export async function fetchGitHubSkillPaths( locator: string, branch: string, diff --git a/packages/query/src/tests/source-lifecycle.test.ts b/packages/query/src/tests/source-lifecycle.test.ts index 3e1a164..8ca8d74 100644 --- a/packages/query/src/tests/source-lifecycle.test.ts +++ b/packages/query/src/tests/source-lifecycle.test.ts @@ -879,16 +879,8 @@ describe.sequential("source lifecycle", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { - if (args[0] === "ls-remote" && args[1] === "--heads") { - return "test-commit-sha\trefs/heads/main"; - } - if ( - args[0] === "clone" - && args[3] === "--branch" - && args[4] === "main" - && args[5] === "https://github.com/vercel-labs/skills.git" - ) { - await fs.cp(upstreamRepo, args[6]!, { recursive: true }); + if (args[0] === "clone" && args[3] === "https://github.com/vercel-labs/skills.git") { + await fs.cp(upstreamRepo, args[4]!, { recursive: true }); return ""; } @@ -916,7 +908,6 @@ describe.sequential("source lifecycle", () => { const lock = state.lockFile.sources[result.data.manifest.id]; expect(source?.kind).toBe("git"); expect(lock?.revision.provider).toBe("git"); - expect(lock?.originBranch).toBe("main"); expect(lock?.localPath).toBe( app.store.getSourceCheckoutPath("git", result.data.manifest.id), ); @@ -928,16 +919,8 @@ describe.sequential("source lifecycle", () => { }); vi.spyOn(gitUtils, "isGitAvailable").mockResolvedValue(true); vi.spyOn(gitUtils, "git").mockImplementation(async (args) => { - if (args[0] === "ls-remote" && args[1] === "--heads") { - return "test-commit-sha\trefs/heads/main"; - } - if ( - args[0] === "clone" - && args[3] === "--branch" - && args[4] === "main" - && args[5] === "https://github.com/vercel-labs/skills.git" - ) { - await fs.cp(upstreamRepo, args[6]!, { recursive: true }); + if (args[0] === "clone" && args[3] === "https://github.com/vercel-labs/skills.git") { + await fs.cp(upstreamRepo, args[4]!, { recursive: true }); return ""; } From a530c6ab55494fe853910fecedc7e9765a12bbc3 Mon Sep 17 00:00:00 2001 From: VintLin Date: Fri, 21 Aug 2026 15:46:11 +0800 Subject: [PATCH 6/6] =?UTF-8?q?=E4=BF=AE=E5=A4=8D\=E6=81=A2=E5=A4=8D?= =?UTF-8?q?=E7=BC=BA=E5=A4=B1=E6=89=98=E7=AE=A1=E7=9B=AE=E5=BD=95=E7=9A=84?= =?UTF-8?q?=E6=9D=A5=E6=BA=90=E6=9B=B4=E6=96=B0=E4=BF=AE=E5=A4=8D=E8=B7=AF?= =?UTF-8?q?=E5=BE=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 问题删除整个 source kind 目录后更新被错误判定为非法路径 修复仅拒绝已存在的符号链接或越界组件,并补充 missing-checkout 回归测试 --- .../src/services/source-authority-service.ts | 40 ++++++++++++------- .../tests/source-authority-service.test.ts | 4 +- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/packages/core-engine/src/services/source-authority-service.ts b/packages/core-engine/src/services/source-authority-service.ts index 029306f..2df01b2 100644 --- a/packages/core-engine/src/services/source-authority-service.ts +++ b/packages/core-engine/src/services/source-authority-service.ts @@ -503,30 +503,40 @@ export class SourceAuthorityService { } const kindRoot = path.dirname(expectedCheckoutPath); - for (const managedPath of [sourceRoot, kindRoot]) { + const existingManagedPaths = new Set(); + for (const managedPath of [sourceRoot, kindRoot, expectedCheckoutPath]) { try { - if ((await fs.lstat(managedPath)).isSymbolicLink()) { + const stats = await fs.lstat(managedPath); + if (stats.isSymbolicLink()) { return false; } - } catch { - return false; - } - } - - const checkoutExists = await pathExists(expectedCheckoutPath); - if (checkoutExists) { - try { - if ((await fs.lstat(expectedCheckoutPath)).isSymbolicLink()) { + existingManagedPaths.add(managedPath); + } catch (error) { + if ( + typeof error !== "object" + || error === null + || !("code" in error) + || error.code !== "ENOENT" + ) { return false; } - } catch { - return false; } } + const sourceRootExists = existingManagedPaths.has(sourceRoot); + const kindRootExists = existingManagedPaths.has(kindRoot); + const checkoutExists = existingManagedPaths.has(expectedCheckoutPath); + try { - const realSourceRoot = await fs.realpath(sourceRoot); - const realKindRoot = await fs.realpath(kindRoot); + const realSourceRoot = sourceRootExists + ? await fs.realpath(sourceRoot) + : path.join( + await fs.realpath(path.dirname(sourceRoot)), + path.basename(sourceRoot), + ); + const realKindRoot = kindRootExists + ? await fs.realpath(kindRoot) + : path.join(realSourceRoot, path.basename(kindRoot)); if (!isPathInside(realSourceRoot, realKindRoot)) { return false; } diff --git a/packages/core-engine/src/tests/source-authority-service.test.ts b/packages/core-engine/src/tests/source-authority-service.test.ts index 5692316..84fff1b 100644 --- a/packages/core-engine/src/tests/source-authority-service.test.ts +++ b/packages/core-engine/src/tests/source-authority-service.test.ts @@ -294,7 +294,7 @@ describe.sequential("SourceAuthorityService", () => { .resolves.toBe("keep"); }); - test("updateSources skips healthy git sources and repairs local drift", async () => { + test("updateSources skips healthy git sources and repairs a missing managed directory or local drift", async () => { const stateStore = new StateStore(sandbox.stateRoot); await stateStore.init(); const checkoutService = new SourceCheckoutService({ @@ -380,7 +380,7 @@ describe.sequential("SourceAuthorityService", () => { "git-unchanged:skills/one", ]); - await fs.rm(state.lockFile.sources["git-unchanged"]!.localPath, { + await fs.rm(path.dirname(state.lockFile.sources["git-unchanged"]!.localPath), { recursive: true, force: true, });