diff --git a/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift b/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift index 4591e4b..bc21b4e 100644 --- a/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift +++ b/apps/desktop-mac/Sources/DesktopApp/Runtime/Bridge/BridgeClient.swift @@ -108,16 +108,23 @@ final class BridgeClient: @unchecked Sendable { private let mutationCoordinator = MutationCoordinator() private let commandTimeoutMilliseconds: UInt64 private let importCommandTimeoutMilliseconds: UInt64 + private let updateSourceTimeoutMilliseconds: UInt64 + private let updateCommandMaximumTimeoutMilliseconds: 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, + // 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.updateSourceTimeoutMilliseconds = updateSourceTimeoutMilliseconds + self.updateCommandMaximumTimeoutMilliseconds = updateCommandMaximumTimeoutMilliseconds self.commandTimeoutGraceMilliseconds = commandTimeoutGraceMilliseconds } @@ -425,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, @@ -493,8 +500,26 @@ 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, + payload: [String: AnyCodable]? + ) -> UInt64 { + if command == .update { + 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 : commandTimeoutMilliseconds } diff --git a/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift b/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift index 7e1c55a..a901aa6 100644 --- a/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift +++ b/apps/desktop-mac/Tests/SkillFlowDesktopTests/BridgeClientExecutionTests.swift @@ -85,6 +85,80 @@ final class BridgeClientExecutionTests: XCTestCase { XCTAssertTrue(response.ok) } + func testUpdateTimeoutScalesWithSelectedSourceCount() async throws { + let fixture = try SlowBridgeFixture.install(delayMilliseconds: 100) + self.fixture = fixture + + let bridge = await MainActor.run { + BridgeClient( + commandTimeoutMilliseconds: 25, + importCommandTimeoutMilliseconds: 50, + updateSourceTimeoutMilliseconds: 75, + updateCommandMaximumTimeoutMilliseconds: 150 + ) + } + + 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/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..00dc97c 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -31,6 +31,8 @@ Skill Flow 用来把分散在不同来源的 AI agent skills 管理成可检查 - `lock.json` 记录解析结果、source snapshot 和部署投影。 - Target 目录是生成输出，不是事实源。 - macOS 桌面端依赖 CLI bridge 协议，不另建第二套状态模型。 +- Managed update 只读写 `~/.skillflow/source//` 的规范 checkout；lock 路径不匹配或 checkout 路径链包含符号链接时拒绝更新。 +- 桌面端更新始终有界：每个明确选择的 source 预算 5 分钟，总上限 15 分钟；全部更新使用 15 分钟。 ## 非目标 @@ -48,4 +50,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..1e6ef93 --- /dev/null +++ b/docs/superpowers/specs/2026-08-18-git-source-update-boundaries-design.md @@ -0,0 +1,83 @@ +# 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; +- 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 change GitHub or GitLab locator parsing, 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`. 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. + +### Git update path + +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. + +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. + +### 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 + +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. 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. +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 04b965d..2df01b2 100644 --- a/packages/core-engine/src/services/source-authority-service.ts +++ b/packages/core-engine/src/services/source-authority-service.ts @@ -327,12 +327,26 @@ export class SourceAuthorityService { continue; } + const sourceRoot = path.join(this.options.stateStore.rootPath, "source"); + const expectedCheckoutPath = path.join(sourceRoot, source.kind, sourceId); + if (!await this.isManagedCheckoutPathValid( + sourceRoot, + expectedCheckoutPath, + lock.localPath, + )) { + return fail({ + code: "SOURCE_CHECKOUT_PATH_INVALID", + message: `Refusing to update checkout with invalid managed path: ${lock.localPath}`, + }, warnings); + } + const lockedCommit = this.readLockedCommitSha(lock.revision); let repairReason: SourceRepairReason | undefined; if (source.kind === "git" && lockedCommit) { try { const remoteCommit = await this.options.checkoutService.readGitRemoteHeadCommit( source.locator, + lock.originBranch ? { branch: lock.originBranch } : {}, ); if (!remoteCommit) { precheckFallbackSourceIds.push(sourceId); @@ -374,8 +388,11 @@ export class SourceAuthorityService { options: { sourceIdOverride: sourceId, displayNameOverride: source.displayName, + ...(lock.originBranch ? { originBranch: lock.originBranch } : {}), }, checkoutPath: tempCheckoutPath, + existingCheckoutPath: lock.localPath, + ...(lock.originBranch ? { updateBranch: lock.originBranch } : {}), allowEmptyLeafs: true, }); if (!prepared.ok) { @@ -472,6 +489,66 @@ 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); + const existingManagedPaths = new Set(); + for (const managedPath of [sourceRoot, kindRoot, expectedCheckoutPath]) { + try { + const stats = await fs.lstat(managedPath); + if (stats.isSymbolicLink()) { + return false; + } + existingManagedPaths.add(managedPath); + } catch (error) { + if ( + typeof error !== "object" + || error === null + || !("code" in error) + || error.code !== "ENOENT" + ) { + return false; + } + } + } + + const sourceRootExists = existingManagedPaths.has(sourceRoot); + const kindRootExists = existingManagedPaths.has(kindRoot); + const checkoutExists = existingManagedPaths.has(expectedCheckoutPath); + + try { + 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; + } + 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 469ac47..b5ac72c 100644 --- a/packages/core-engine/src/services/source-checkout-service.ts +++ b/packages/core-engine/src/services/source-checkout-service.ts @@ -154,6 +154,8 @@ export class SourceCheckoutService { options?: AddSourceOptions; existingSources?: Array<{ id: string; kind?: SourceKind; locator: string; displayName: string }>; checkoutPath?: string; + existingCheckoutPath?: string; + updateBranch?: string; suffix?: string; allowEmptyLeafs?: boolean; } = {}, @@ -171,7 +173,12 @@ export class SourceCheckoutService { await ensureDir(path.dirname(checkoutPath)); try { - await this.fetchSource(resolved, checkoutPath); + await this.fetchSource( + resolved, + checkoutPath, + input.existingCheckoutPath, + input.updateBranch, + ); } catch (error) { await removePath(checkoutPath); return fail({ @@ -256,11 +263,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 +282,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); } @@ -463,6 +476,8 @@ export class SourceCheckoutService { private async fetchSource( source: SourceResolution, checkoutPath: string, + existingCheckoutPath?: string, + updateBranch?: string, ): Promise { if (source.kind === "local") { await copyDirectory(source.localPath!, checkoutPath); @@ -470,7 +485,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, + updateBranch, + ); + return; + } catch { + await removePath(checkoutPath).catch(() => {}); + } + } + await this.fetchGitSource(source.gitLocator!, checkoutPath, updateBranch); return; } @@ -485,6 +516,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, @@ -802,14 +858,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 +878,44 @@ 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, + ]); + if (originBranch) { + await git( + ["fetch", "--depth", "1", "origin", this.remoteRef(originBranch)], + { cwd: checkoutPath }, + ); + await git( + ["checkout", "--detach", "--force", "FETCH_HEAD"], + { cwd: checkoutPath }, + ); + } }, { attempts: 2 }); } @@ -855,11 +936,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..84fff1b 100644 --- a/packages/core-engine/src/tests/source-authority-service.test.ts +++ b/packages/core-engine/src/tests/source-authority-service.test.ts @@ -217,7 +217,84 @@ describe.sequential("SourceAuthorityService", () => { ]); }); - test("updateSources skips healthy git sources and repairs local drift", async () => { + 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("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 a missing managed directory or local drift", async () => { const stateStore = new StateStore(sandbox.stateRoot); await stateStore.init(); const checkoutService = new SourceCheckoutService({ @@ -265,6 +342,7 @@ describe.sequential("SourceAuthorityService", () => { }], invalidLeafs: [], commitSha: "same-sha", + originBranch: "release", }, }); expect(committed.ok).toBe(true); @@ -287,6 +365,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", @@ -298,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, }); @@ -384,6 +466,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", @@ -391,6 +484,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 04a236d..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,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,179 @@ 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, + updateBranch: "release", + options: { sourceIdOverride: "git-existing", originBranch: "release" }, + }, + ); + + expect(prepared.ok).toBe(true); + if (!prepared.ok) { + 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", + )).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] === "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"; + } + 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, + updateBranch: "release", + 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, + ]); + expect(git).toHaveBeenCalledWith( + ["fetch", "--depth", "1", "origin", "refs/heads/release"], + { cwd: 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", + { + updateBranch: "release", + 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."), 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" && diff --git a/packages/query/src/tests/source-lifecycle.test.ts b/packages/query/src/tests/source-lifecycle.test.ts index 9984da2..8ca8d74 100644 --- a/packages/query/src/tests/source-lifecycle.test.ts +++ b/packages/query/src/tests/source-lifecycle.test.ts @@ -1498,6 +1498,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: { @@ -1512,6 +1518,7 @@ description: | ...before.lockFile.sources, [sourceId]: { ...before.lockFile.sources[sourceId]!, + localPath: gitCheckoutPath, revision: { provider: "git", commit, capturedAt: new Date().toISOString() }, }, },