From 8963bb3bef91b13efaac1b59a6e787a29a05ad9b Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 11 Aug 2026 10:13:40 -0400 Subject: [PATCH] feat(skills): harden shared installation lifecycle --- .github/pull_request_template.md | 4 + ...-skill-sharing-implementation-checklist.md | 55 +++++------ docs/agent-skill-sharing-installation-plan.md | 11 ++- docs/reference/agent-skill-provider-paths.md | 38 ++++++++ .../agent-skill-sharing-upstream-boundary.md | 43 +++++++++ src/main/ipc/skill-cloud-ipc-handlers.ts | 40 +++++++- .../skill-install-request-service.test.ts | 61 +++++++++++- src/main/skills/skill-install-service.test.ts | 79 ++++++++++++++++ src/main/skills/skill-install-service.ts | 3 + .../skills/skill-install-transaction.test.ts | 94 +++++++++++++++++++ src/main/skills/skill-install-transaction.ts | 34 +++++-- .../skills/skill-package-creation.test.ts | 29 +++++- .../skills/skill-package-identity.test.ts | 32 ++++++- src/main/skills/skill-package-identity.ts | 3 + src/main/skills/skill-package-tar.test.ts | 64 +++++++++++++ src/main/skills/skill-package-tar.ts | 3 + src/main/skills/skill-remove-recovery.ts | 7 +- .../skills/skill-remove-transaction.test.ts | 45 +++++++++ src/main/skills/skill-remove-transaction.ts | 26 ++++- .../skills/skill-runtime-capability.test.ts | 43 +++++++++ src/main/skills/skill-runtime-capability.ts | 15 ++- src/preload/api-types.ts | 2 + src/preload/index.ts | 7 ++ .../skills/SkillInstallDialog.test.tsx | 60 +++++++++++- .../components/skills/SkillInstallDialog.tsx | 21 +++-- .../SkillInstallManagementDialog.test.tsx | 51 +++++++++- .../skills/SkillInstallManagementDialog.tsx | 30 ++++-- .../src/components/skills/SkillsPage.test.tsx | 10 +- .../skills/skill-install-progress-state.ts | 42 +++++++++ src/renderer/src/web/web-preload-api.ts | 1 + src/shared/skill-install-contract.test.ts | 46 +++++++++ src/shared/skill-sharing-contract.ts | 5 + 32 files changed, 934 insertions(+), 70 deletions(-) create mode 100644 docs/reference/agent-skill-provider-paths.md create mode 100644 docs/reference/agent-skill-sharing-upstream-boundary.md create mode 100644 src/main/skills/skill-package-tar.test.ts create mode 100644 src/main/skills/skill-runtime-capability.test.ts create mode 100644 src/renderer/src/components/skills/skill-install-progress-state.ts create mode 100644 src/shared/skill-install-contract.test.ts diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b69e2dcc233..9364b34151c 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -24,6 +24,10 @@ Confirm that the review explicitly checked cross-platform compatibility for macO Provide a basic security audit summary from your AI coding agent. Call out any input handling, command execution, path handling, auth, secrets, dependency, or IPC risks that were reviewed, plus any follow-up needed. +## Agent skill upstream boundary + +- [ ] Not applicable, or this change follows `docs/reference/agent-skill-sharing-upstream-boundary.md` and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation. + ## Notes Call out any platform-specific behavior, risks, or follow-up work. diff --git a/docs/agent-skill-sharing-implementation-checklist.md b/docs/agent-skill-sharing-implementation-checklist.md index 47321fbe91f..c35f356e40c 100644 --- a/docs/agent-skill-sharing-implementation-checklist.md +++ b/docs/agent-skill-sharing-implementation-checklist.md @@ -59,9 +59,9 @@ does not mean the surrounding phase is complete. - [x] Decide Orca will not depend on the upstream CLI or an unsupported programmatic API. - [x] Decide V1 accepts Orca package sources only; Git, npm, and community registries remain with existing tools. -- [ ] Add an architecture decision record covering the reference-only boundary and review it with +- [ ] Review the architecture decision record covering the reference-only boundary with engineering and legal owners. -- [ ] Add a review check that flags any proposed copied or mechanically translated upstream +- [x] Add a review check that flags any proposed copied or mechanically translated upstream material so attribution and licensing can be reconsidered before merge. - [ ] Assign desktop, runtime, Cloud API, Terraform, security, design, and release owners. - [ ] Confirm the first release ends after WSL and SSH support; keep team reconciliation as a later @@ -95,12 +95,12 @@ does not mean the surrounding phase is complete. - [x] Inventory current desktop IPC and remote skill RPC registration. - [x] Inventory existing SSH execution and file-transfer providers and identify the host-side installer entry point. -- [ ] Research official documentation for every initially supported agent's global and project +- [x] Research official documentation for every initially supported agent's global and project skill directories. - [ ] Verify each provider path with a real installation on every supported platform. -- [ ] Record whether each provider reads `.agents/skills` directly, how it is detected, and which +- [x] Record whether each provider reads `.agents/skills` directly, how it is detected, and which alias mechanisms it supports. -- [ ] Add a normal reviewed maintenance process for provider registry changes; do not create an +- [x] Add a normal reviewed maintenance process for provider registry changes; do not create an upstream synchronization job. ## 1. Define package and install contracts @@ -201,10 +201,10 @@ does not mean the surrounding phase is complete. - [x] Test executable-mode preservation and Windows's mode limitations. - [x] Test missing, malformed, and identity-mismatched `SKILL.md` files. - [ ] Test every size, count, and depth boundary at limit, one below, and one above. -- [ ] Test traversal, absolute paths, drive paths, Unicode/case collisions, duplicate paths, all +- [x] Test traversal, absolute paths, drive paths, Unicode/case collisions, duplicate paths, all rejected link and special-file types, and encrypted entries. - [x] Test truncated archives and invalid tar and content checksums. -- [ ] Fuzz archive path normalization and envelope parsing with bounded resources. +- [x] Fuzz archive path normalization and envelope parsing with bounded resources. ### Phase 2 package gate @@ -231,7 +231,7 @@ does not mean the surrounding phase is complete. - [x] Add `src/main/skills/skill-package-download.ts`. - [x] Add `src/main/skills/skill-install-service.ts`. -- [ ] Validate request shape, schema, capability, package identity, scope, destination, and policy +- [x] Validate request shape, schema, capability, package identity, scope, destination, and policy before download or mutation. - [x] Recover prior journals for the same destination before planning a new operation. - [x] Acquire a filesystem-backed cross-process lock keyed by canonical destination. @@ -303,7 +303,7 @@ does not mean the surrounding phase is complete. - [x] Add `src/main/skills/skill-provider-destinations.ts`. - [x] Add data-only records for Orca agent ID, canonical-root support, global/project resolvers, detection evidence, and platform alias support. -- [ ] Populate only provider paths independently verified from official documentation and real +- [x] Populate only provider paths independently verified from official documentation and real installations. - [x] Detect an agent before creating its provider-specific configuration root. - [x] Avoid modifying roots for agents that consume `.agents/skills` directly. @@ -356,14 +356,14 @@ does not mean the surrounding phase is complete. ### Recovery failure injection -- [ ] Inject failure before and after every journal transition. +- [x] Inject failure before and after every journal transition. - [x] Verify partial downloads and extraction staging are deleted. - [x] Verify a moved destination is restored from backup. - [x] Verify a placed destination without a receipt is completed or restored according to journal state. - [x] Verify a published receipt with an incomplete journal is finalized safely. -- [ ] Verify interrupted placement reconciliation preserves the canonical install and retries. -- [ ] Test cancellation during download, extraction, staging copy, commit, provenance, and +- [x] Verify interrupted placement reconciliation preserves the canonical install and retries. +- [x] Test cancellation during download, extraction, staging copy, commit, provenance, and placement reconciliation. ## 6. Provision GCP infrastructure @@ -393,10 +393,11 @@ does not mean the surrounding phase is complete. - [x] Keep object versioning disabled and record object generations in PostgreSQL. - [x] Add a one-day deletion lifecycle for `uploads/` quarantine objects only. - [x] Add exact approved production and development Orca origins to CORS; never use `*`. -- [ ] Allow only required signed POST upload and GET/HEAD download behavior and response headers. +- [x] Allow only required signed POST upload and GET/HEAD download behavior and response headers. - [x] Keep the bucket off public custom domains. - [x] Define immutable final keys as - `packages/v1/sha256///package.tar.gz`. + `packages/v1/sha256///package.tar.gz`, with the logical package digest + verified in object metadata and PostgreSQL. - [x] Define tenant-bound random quarantine keys as `uploads//package.tar.gz`. ### Database and secret @@ -428,13 +429,13 @@ does not mean the surrounding phase is complete. - [ ] Extend `orca-cloud-api` in `us-central1`; do not create a separate V1 worker service. - [x] Configure bucket, 40 MiB compressed limit, 15-minute upload TTL, five-minute download TTL, fixed finalize concurrency, and skill database URL. -- [ ] Reuse existing auth base URL and application CORS configuration. -- [ ] Stream validation with fixed buffers under the existing 512 MiB memory limit. +- [x] Reuse existing auth base URL and application CORS configuration. +- [x] Stream validation with fixed buffers under the existing 512 MiB memory limit. - [x] Add a small per-instance finalization semaphore and retryable `429` or `503` with `Retry-After` when saturated. -- [ ] Preserve current service scaling initially and tune only from measured CPU, latency, +- [x] Preserve current service scaling initially and tune only from measured CPU, latency, database, and error data. -- [ ] Define criteria for splitting finalization into a worker service if it harms existing API +- [x] Define criteria for splitting finalization into a worker service if it harms existing API traffic. ### Terraform review and apply @@ -495,7 +496,7 @@ does not mean the surrounding phase is complete. - [x] Stream once to calculate archive SHA-256 and validate envelope, manifest, paths, limits, and package digest. - [x] Promote with an if-absent generation precondition to the content-addressed final key. -- [ ] If the final key exists, verify recorded identity without exposing cross-tenant +- [x] If the final key exists, verify recorded identity without exposing cross-tenant deduplication. - [x] Publish the version and complete the upload in a PostgreSQL transaction. - [x] Delete quarantine bytes after success and rely on lifecycle cleanup for abandonment. @@ -519,15 +520,15 @@ does not mean the surrounding phase is complete. - [x] Set only response content type and safe attachment filename overrides. - [x] Grant no list or write capability. - [x] Return expected archive identity and byte count beside the grant for runtime verification. -- [ ] Test revocation immediately blocks new grants while already issued URLs expire within five +- [x] Test revocation immediately blocks new grants while already issued URLs expire within five minutes. ### Cloud tests - [x] Test organization, user, pinned-link, expiry, and revocation ACL behavior. -- [ ] Test membership removal between preview, grant, and download. +- [x] Test membership removal between preview, grant, and download. - [x] Test expired, reused, wrong-tenant, wrong-key, wrong-size, and wrong-hash uploads. -- [ ] Test malformed, oversized, and resource-exhausting archives during finalization. +- [x] Test malformed, oversized, and resource-exhausting archives during finalization. - [x] Test finalization semaphore saturation and retry behavior. - [x] Test concurrent idempotent mutations and partial database/GCS failures. - [ ] Test deduplication without cross-tenant timing or response disclosure. @@ -568,10 +569,10 @@ does not mean the surrounding phase is complete. - [x] Allow selection of connected machine, Git worktree, or plain folder workspace. - [x] Show detected-agent coverage and canonical/provider topology before commit. - [x] Show installed-state conflicts and explicit resolution choices. -- [ ] Show phase progress without exposing grants or local private paths. +- [x] Show phase progress without exposing grants or local private paths. - [x] Render installed, unchanged, updated, partial, conflict, unsupported, cancelled, and failed results with actionable recovery. -- [ ] Add incomplete-coverage retry. +- [x] Add incomplete-coverage retry. ### Skills page lifecycle @@ -618,7 +619,7 @@ does not mean the surrounding phase is complete. - [x] Hide or disable remote install when `skills.install.v1` is absent. - [x] Show a specific host-update-required message instead of attempting the RPC. - [x] Test new client/old server and old client/new server. -- [ ] Test optional-field omission in both directions. +- [x] Test optional-field omission in both directions. - [x] Test a capability changing after preview but before execution. - [x] Confirm older servers retain skill discovery and ignore no unknown stream opcode because none is introduced. @@ -770,11 +771,11 @@ directory was used for destructive failure injection. - [x] Add independent server-side flags for upload grants, download grants, and remote installation. -- [ ] Verify disabling Cloud grants does not break skill discovery or already installed skills. +- [x] Verify disabling Cloud grants does not break skill discovery or already installed skills. - [x] Document finalization saturation, GCS/IAM signing failure, database outage, corrupt package, orphan reconciliation, and rollback response. - [x] Document coordinated PostgreSQL point-in-time and GCS generation/soft-delete restoration. -- [ ] Document package/share deletion, organization departure, legal retention, and audit handling. +- [x] Document package/share deletion, organization departure, legal retention, and audit handling. - [x] Document how and when to split finalization into its own service. ## 13. Complete automated and end-to-end validation diff --git a/docs/agent-skill-sharing-installation-plan.md b/docs/agent-skill-sharing-installation-plan.md index ed706eec1cf..d90a691d47b 100644 --- a/docs/agent-skill-sharing-installation-plan.md +++ b/docs/agent-skill-sharing-installation-plan.md @@ -7,6 +7,12 @@ Last updated: 2026-08-11. Implementation checklist: [Agent skill sharing implementation checklist](./agent-skill-sharing-implementation-checklist.md). +Architecture decision: +[Agent skill sharing upstream boundary](./reference/agent-skill-sharing-upstream-boundary.md). + +Provider registry: +[Agent skill provider paths](./reference/agent-skill-provider-paths.md). + ## Decision summary Orca will own a focused skill packaging and installation pipeline for private sharing. @@ -295,7 +301,7 @@ switch without disrupting existing artifact links. ```text gs://onorca-cloud-skill-packages/ uploads//package.tar.gz - packages/v1/sha256///package.tar.gz + packages/v1/sha256///package.tar.gz ``` Rules: @@ -306,7 +312,8 @@ Rules: - The database records the final object's GCS generation, compressed size, archive SHA-256, and package digest. - Final objects are never overwritten. Promotion uses a generation-match precondition. -- If a final key already exists, finalization verifies its recorded identity before reusing it. +- If a final key already exists, finalization verifies its archive SHA-256 and logical package + digest metadata before reusing it. - Internal deduplication never changes tenant-scoped API responses or reveals that another organization already stored the same digest. - GCS object versioning remains off; immutable keys plus stored generations prevent accidental diff --git a/docs/reference/agent-skill-provider-paths.md b/docs/reference/agent-skill-provider-paths.md new file mode 100644 index 00000000000..293229ad70e --- /dev/null +++ b/docs/reference/agent-skill-provider-paths.md @@ -0,0 +1,38 @@ +# Agent skill provider paths + +Last verified: 2026-08-11. + +V1 supports only providers whose paths are independently established by official documentation. +The registry is deliberately small; it is not copied or synchronized from a community path table. + +| Provider | Detection | Global canonical support | Workspace support | Orca placement | +| --- | --- | --- | --- | --- | +| Codex | `codex` CLI found through Orca's host-owned PATH detection | Reads `$HOME/.agents/skills` directly | Reads `.agents/skills` from the current directory through the repository root | Canonical copy only | +| Claude Code | `claude` CLI found through Orca's host-owned PATH detection | Reads `$HOME/.claude/skills` | Reads `.claude/skills` from the launch directory through the repository root, plus nested directories as files are accessed | Relative directory symlink on POSIX, directory junction on Windows, or verified independent-copy fallback | + +Codex locations and symlink behavior are documented in the official OpenAI documentation: +[Build skills](https://learn.chatgpt.com/docs/build-skills#where-codex-loads-local-skills). + +Claude Code locations, precedence, parent traversal, live detection, and symlink behavior are +documented in the official Anthropic documentation: +[Extend Claude with skills](https://code.claude.com/docs/en/skills#where-skills-live). + +Codex therefore needs no provider-specific placement. Claude Code does not document +`.agents/skills` as a discovery root, so Orca reconciles its documented `.claude/skills` path back +to the canonical copy. Orca never replaces a path it does not own. If alias creation is unavailable, +the verified copy fallback is tracked in the install receipt so update and removal can detect drift. + +## Registry change process + +Every registry change requires normal code review and all of the following evidence: + +1. Link current official provider documentation for global and workspace paths. +2. Record whether the provider reads `.agents/skills` directly and its documented link behavior. +3. Verify global and folder-workspace discovery on macOS, Linux, native Windows, and WSL where the + provider supports those platforms. +4. Exercise local, paired-runtime, and SSH host-owned path resolution. +5. Test alias denial, broken owned aliases, independent-copy drift, update, rollback, and removal. +6. Update mixed-version capability evidence if the placement contract changes. + +Do not add automated upstream path-table synchronization. A provider release that changes discovery +semantics must enter through this review process. diff --git a/docs/reference/agent-skill-sharing-upstream-boundary.md b/docs/reference/agent-skill-sharing-upstream-boundary.md new file mode 100644 index 00000000000..bf2323d2895 --- /dev/null +++ b/docs/reference/agent-skill-sharing-upstream-boundary.md @@ -0,0 +1,43 @@ +# Agent skill sharing upstream boundary + +Status: proposed for formal engineering and legal review. + +Date: 2026-08-11. + +## Context + +Orca needs private, durable, cross-machine skill sharing with bounded archive ingestion, +host-owned destination resolution, crash-safe transactions, provenance, and mixed-version remote +support. `vercel-labs/skills` exposes a CLI and does not provide the Cloud authorization or local +transaction contract Orca requires. + +The behavioral assessment used upstream commit +`c6f69c631292444cc541ac6d91e2226b0ff247da`. + +## Decision + +Orca implements its package, Cloud, installation, provider-placement, and recovery behavior +independently. The upstream project is a behavioral reference only. + +Do not copy or mechanically translate upstream source, tests, fixtures, registry entries, provider +path tables, comments, or documentation. Derive provider paths from official provider +documentation and verify them with real installations. Orca does not depend on the upstream CLI, +npm package, or an unsupported programmatic API. + +If a future change proposes incorporating upstream material, stop and review the exact material, +license, attribution, notices, and maintenance implications before implementation or merge. + +## Consequences + +- Orca owns stability, security, compatibility, and maintenance of this narrower installer. +- There is no automatic upstream synchronization job. +- Similar behavior is acceptable when independently derived from requirements and official + provider contracts; textual or structural copying is not. +- Provider registry changes require normal code review plus official-documentation and real-host + evidence. +- The pull request template requires reviewers to confirm this boundary for relevant changes. + +## Review record + +Product chose the reference-only approach during planning. Formal engineering and legal reviewers +remain to be named before external rollout. diff --git a/src/main/ipc/skill-cloud-ipc-handlers.ts b/src/main/ipc/skill-cloud-ipc-handlers.ts index e8dfe685a5c..475b3c0bbaa 100644 --- a/src/main/ipc/skill-cloud-ipc-handlers.ts +++ b/src/main/ipc/skill-cloud-ipc-handlers.ts @@ -10,6 +10,7 @@ import { SkillInstallDestinationSchema, type SkillInstallRequest } from '../../shared/skill-install-contract' +import type { SkillInstallProgress } from '../../shared/skill-sharing-contract' import { SkillDiscoveryTargetSchema, type SkillDiscoveryResult } from '../../shared/skills' import type { SkillCloudDownloadGrant } from '../../shared/skill-cloud-contract' import type { OrcaRuntimeService } from '../runtime/orca-runtime' @@ -17,6 +18,7 @@ import { installSkillOnRemoteRuntime } from '../skills/skill-remote-install-serv import { classifySkillCloudInstallTarget } from '../skills/skill-cloud-install-target' import { SkillSharePreparationService } from '../skills/skill-share-preparation-service' import { skillInstallFailureFromError } from '../skills/skill-install-operation-error' +import { supportsSkillRuntimeInstall } from '../skills/skill-runtime-capability' import { callRuntimeEnvironment, getRuntimeEnvironmentStatus @@ -191,23 +193,53 @@ function registerCloudInstallHandlers(runtime: OrcaRuntimeService): void { ipcMain.handle('skills:createDownloadGrant', (_event, shareId: unknown) => runtime.createSkillDownloadGrant(z.string().min(1).max(128).parse(shareId), {}) ) - ipcMain.handle('skills:installShare', async (_event, value: unknown) => { - const input = shareInstallSchema.parse(value) + const sendProgress = ( + event: Electron.IpcMainInvokeEvent, + progress: SkillInstallProgress + ): void => { + if (!event.sender.isDestroyed()) { + event.sender.send('skills:installProgress', progress) + } + } + ipcMain.handle('skills:installShare', async (event, value: unknown) => { + const parsed = shareInstallSchema.parse(value) + const input = { ...parsed, operationId: parsed.operationId ?? randomUUID() } + sendProgress(event, { operationId: input.operationId, phase: 'authorizing' }) + if ( + input.environmentId && + !(await supportsSkillRuntimeInstall(app.getPath('userData'), input.environmentId)) + ) { + return { status: 'unsupported' as const, message: SKILL_INSTALL_UPDATE_REQUIRED_MESSAGE } + } const installTarget = await classifySkillCloudInstallTarget(runtime, input) const grant = await runtime.createSkillDownloadGrant(input.shareId, { versionId: input.versionId, installTarget }) + if (grant.status === 'ok') { + sendProgress(event, { operationId: input.operationId, phase: 'installing' }) + } return grant.status === 'ok' ? installGrant(runtime, grant.value, input) : grant }) - ipcMain.handle('skills:installPackageVersion', async (_event, value: unknown) => { - const input = packageVersionInstallSchema.parse(value) + ipcMain.handle('skills:installPackageVersion', async (event, value: unknown) => { + const parsed = packageVersionInstallSchema.parse(value) + const input = { ...parsed, operationId: parsed.operationId ?? randomUUID() } + sendProgress(event, { operationId: input.operationId, phase: 'authorizing' }) + if ( + input.environmentId && + !(await supportsSkillRuntimeInstall(app.getPath('userData'), input.environmentId)) + ) { + return { status: 'unsupported' as const, message: SKILL_INSTALL_UPDATE_REQUIRED_MESSAGE } + } const installTarget = await classifySkillCloudInstallTarget(runtime, input) const grant = await runtime.createSkillPackageVersionDownloadGrant( input.packageId, input.versionId, { installTarget } ) + if (grant.status === 'ok') { + sendProgress(event, { operationId: input.operationId, phase: 'installing' }) + } return grant.status === 'ok' ? installGrant(runtime, grant.value, input) : grant }) ipcMain.handle('skills:cancelInstall', async (_event, value: unknown) => { diff --git a/src/main/skills/skill-install-request-service.test.ts b/src/main/skills/skill-install-request-service.test.ts index aaf10885dbf..1645d54c806 100644 --- a/src/main/skills/skill-install-request-service.test.ts +++ b/src/main/skills/skill-install-request-service.test.ts @@ -1,5 +1,5 @@ import { createHash } from 'node:crypto' -import { mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' +import { lstat, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' @@ -91,6 +91,65 @@ describe('executeSkillInstallRequest', () => { ).rejects.toThrow('skill-install-local-ingress-rejected') }) + it('rejects invalid shape, identity, scope, destination, and ingress policy before download', async () => { + const { request, dependencies, archive, home } = await fixture() + const cases: { input: unknown; error: string }[] = [ + { input: { ...request, unexpected: true }, error: 'Unrecognized key' }, + { + input: { ...request, package: { ...request.package, packageDigest: 'invalid' } }, + error: 'Invalid string' + }, + { + input: { + ...request, + destination: { + scope: 'workspace', + worktreeId: 'worktree_1', + folderWorkspaceId: 'folder_1' + } + }, + error: 'Invalid input' + }, + { + input: { ...request, destination: { scope: 'global', environmentId: 'other' } }, + error: 'skill-install-environment-mismatch' + }, + { + input: { + ...request, + destination: { + scope: 'global', + executionTarget: { kind: 'ssh', connectionId: 'ssh_1' } + } + }, + error: 'skill-install-ssh-dispatch-required' + }, + { + input: { ...request, ingress: { kind: 'local-file', path: archive.archivePath } }, + error: 'skill-install-local-ingress-rejected' + }, + { + input: { + ...request, + ingress: { + ...request.ingress, + url: 'https://untrusted.test/package.tar.gz' + } + }, + error: 'skill-download-origin-rejected' + } + ] + + for (const sample of cases) { + vi.mocked(dependencies.fetcher).mockClear() + await expect(executeSkillInstallRequest(sample.input, dependencies)).rejects.toThrow( + sample.error + ) + expect(dependencies.fetcher).not.toHaveBeenCalled() + await expect(lstat(join(home, '.agents'))).rejects.toMatchObject({ code: 'ENOENT' }) + } + }) + it('returns a structured cancelled result without leaving partial ingress bytes', async () => { const { request, dependencies } = await fixture() const controller = new AbortController() diff --git a/src/main/skills/skill-install-service.test.ts b/src/main/skills/skill-install-service.test.ts index 71a9d47052d..aed3879ab93 100644 --- a/src/main/skills/skill-install-service.test.ts +++ b/src/main/skills/skill-install-service.test.ts @@ -202,4 +202,83 @@ describe('skill install service', () => { }) expect(retried.status).toBe('removed') }) + + it('preserves the committed canonical install when cancellation reaches provider placement', async () => { + const { root, input } = await fixture() + const controller = new AbortController() + const canonical = join(root, 'home', '.agents', 'skills', 'test-skill') + const cancellingFilesystem = { + ...nativeSkillInstallFilesystem, + rename: async (source: string, target: string): Promise => { + await nativeSkillInstallFilesystem.rename(source, target) + if (target === canonical) { + controller.abort() + } + } + } + + const interrupted = await installSharedSkill({ + ...input, + filesystem: cancellingFilesystem, + signal: controller.signal + }) + + expect(interrupted.status).toBe('partial') + expect(interrupted.placements).toEqual( + expect.arrayContaining([ + expect.objectContaining({ provider: 'agent-skills', status: 'installed' }), + expect.objectContaining({ + provider: 'claude', + status: 'skipped', + errorCategory: 'skill-placement-cancelled' + }) + ]) + ) + expect(interrupted.placements.some((placement) => placement.provider === 'codex')).toBe(false) + expect(await readFile(join(canonical, 'SKILL.md'), 'utf8')).toContain('# Test') + + const retried = await installSharedSkill({ ...input, operationId: 'operation_retry' }) + expect(retried.status).toBe('unchanged') + expect(await realpath(join(root, 'home', '.claude', 'skills', 'test-skill'))).toBe( + await realpath(canonical) + ) + }) + + it('cleans an interrupted provider copy and repairs coverage on retry', async () => { + const { root, input } = await fixture() + const canonical = join(root, 'home', '.agents', 'skills', 'test-skill') + const claude = join(root, 'home', '.claude', 'skills', 'test-skill') + let deniedCopyObservation = false + const interruptedFilesystem = { + ...nativeSkillInstallFilesystem, + createAlias: async () => { + throw new Error('injected-alias-denial') + }, + observeSkill: async (path: string) => { + if (path === claude && !deniedCopyObservation) { + deniedCopyObservation = true + throw new Error('injected-copy-interruption') + } + return nativeSkillInstallFilesystem.observeSkill(path) + } + } + + const interrupted = await installSharedSkill({ ...input, filesystem: interruptedFilesystem }) + expect(interrupted).toMatchObject({ + status: 'partial', + placements: expect.arrayContaining([ + expect.objectContaining({ + provider: 'claude', + status: 'failed', + errorCategory: 'skill-placement-create-failed' + }) + ]) + }) + await expect(lstat(claude)).rejects.toMatchObject({ code: 'ENOENT' }) + expect(await readFile(join(canonical, 'SKILL.md'), 'utf8')).toContain('# Test') + + const retried = await installSharedSkill({ ...input, operationId: 'operation_retry' }) + expect(retried.status).toBe('unchanged') + expect(await realpath(claude)).toBe(await realpath(canonical)) + }) }) diff --git a/src/main/skills/skill-install-service.ts b/src/main/skills/skill-install-service.ts index 4f39ac274f6..ff5f6beb693 100644 --- a/src/main/skills/skill-install-service.ts +++ b/src/main/skills/skill-install-service.ts @@ -107,6 +107,9 @@ export async function installSharedSkill( detectedProviders: input.detectedProviders }) for (const destination of destinations) { + if (destination.readsCanonicalRoot) { + continue + } if (input.signal?.aborted) { placements.push({ provider: destination.provider, diff --git a/src/main/skills/skill-install-transaction.test.ts b/src/main/skills/skill-install-transaction.test.ts index 6cdcb065b33..fae2f75cd3f 100644 --- a/src/main/skills/skill-install-transaction.test.ts +++ b/src/main/skills/skill-install-transaction.test.ts @@ -234,4 +234,98 @@ describe('skill install transaction', () => { await expect(lstat(join(root, 'state', 'journals'))).resolves.toBeDefined() expect(await readdir(join(root, 'state', 'journals'))).toEqual([]) }) + + it('removes staging bytes when cancellation arrives after the staging move', async () => { + const root = await temporaryDirectory() + const archive = await packageVersion(root, 'version_1', '# First') + const controller = new AbortController() + const cancellingFilesystem = { + ...nativeSkillInstallFilesystem, + rename: async (source: string, target: string): Promise => { + await nativeSkillInstallFilesystem.rename(source, target) + if (target.includes('.orca-staging-')) { + controller.abort() + } + } + } + + const result = await installLocalSkillPackage( + installInput(root, archive, { filesystem: cancellingFilesystem, signal: controller.signal }) + ) + + expect(result).toMatchObject({ + status: 'cancelled', + failure: { category: 'cancelled', retryable: true } + }) + await expect(lstat(join(root, 'skills', 'test-skill'))).rejects.toMatchObject({ + code: 'ENOENT' + }) + expect((await readdir(join(root, 'skills'))).filter((name) => name.includes('.orca-'))).toEqual( + [] + ) + }) + + it('finishes provenance safely when cancellation arrives after canonical commit', async () => { + const root = await temporaryDirectory() + const archive = await packageVersion(root, 'version_1', '# First') + const controller = new AbortController() + + const result = await installLocalSkillPackage( + installInput(root, archive, { signal: controller.signal }), + { + onJournalTransition: async (phase, boundary) => { + if (phase === 'receipt-published' && boundary === 'before') { + controller.abort() + } + } + } + ) + + expect(result.status).toBe('installed') + expect(await readFile(join(root, 'skills', 'test-skill', 'SKILL.md'), 'utf8')).toContain( + '# First' + ) + await expect( + readFile( + join( + root, + 'state', + 'receipts', + `${skillInstallStateKey(join(root, 'skills', 'test-skill'))}.json` + ), + 'utf8' + ) + ).resolves.toContain('version_1') + }) + + it.each( + ['prepared', 'backup-created', 'canonical-placed', 'receipt-published', 'complete'].flatMap( + (phase) => ['before', 'after'].map((boundary) => [phase, boundary] as const) + ) + )('recovers failure %s the %s journal transition', async (phase, boundary) => { + const root = await temporaryDirectory() + const first = await packageVersion(root, 'version_1', '# First') + const second = await packageVersion(root, 'version_2', '# Second') + await installLocalSkillPackage(installInput(root, first)) + + await expect( + installLocalSkillPackage(installInput(root, second), { + onJournalTransition: async (currentPhase, currentBoundary) => { + if (currentPhase === phase && currentBoundary === boundary) { + throw new Error(`injected-${phase}-${boundary}`) + } + } + }) + ).rejects.toThrow(`injected-${phase}-${boundary}`) + + const skillMarkdown = await readFile(join(root, 'skills', 'test-skill', 'SKILL.md'), 'utf8') + expect(skillMarkdown.includes('# First') || skillMarkdown.includes('# Second')).toBe(true) + expect((await readdir(join(root, 'skills'))).filter((name) => name.includes('.orca-'))).toEqual( + [] + ) + expect(await readdir(join(root, 'state', 'journals'))).toEqual([]) + await expect(installLocalSkillPackage(installInput(root, second))).resolves.toMatchObject({ + status: expect.stringMatching(/^(updated|unchanged)$/) + }) + }) }) diff --git a/src/main/skills/skill-install-transaction.ts b/src/main/skills/skill-install-transaction.ts index 6fa603279af..9c3b6eadd38 100644 --- a/src/main/skills/skill-install-transaction.ts +++ b/src/main/skills/skill-install-transaction.ts @@ -61,6 +61,15 @@ export type LocalSkillInstallPreview = { currentState: SkillCanonicalState } +export type SkillInstallJournalBoundary = 'before' | 'after' + +export type SkillInstallTransactionDependencies = { + onJournalTransition?: ( + phase: SkillInstallJournalV1['phase'], + boundary: SkillInstallJournalBoundary + ) => Promise +} + function lockPath(stateDirectory: string, canonicalPath: string): string { return join(stateDirectory, 'locks', `${skillInstallStateKey(canonicalPath)}.lock`) } @@ -91,6 +100,16 @@ function throwIfCancelled(signal?: AbortSignal): void { } } +async function persistJournalTransition( + statePath: string, + journal: SkillInstallJournalV1, + dependencies: SkillInstallTransactionDependencies +): Promise { + await dependencies.onJournalTransition?.(journal.phase, 'before') + await writeSkillStateFile(statePath, journal) + await dependencies.onJournalTransition?.(journal.phase, 'after') +} + export async function previewLocalSkillPackage( input: LocalSkillInstallInput ): Promise { @@ -113,7 +132,8 @@ export async function previewLocalSkillPackage( } export async function installLocalSkillPackage( - input: LocalSkillInstallInput + input: LocalSkillInstallInput, + dependencies: SkillInstallTransactionDependencies = {} ): Promise { const filesystem = input.filesystem ?? nativeSkillInstallFilesystem const extracted = await inspectExtractedPackage(input) @@ -176,7 +196,6 @@ export async function installLocalSkillPackage( if (destinationExists && !backupDigest) { return skillInstallConflictResult(input.operationId, extracted.manifest, state) } - await filesystem.rename(join(extracted.extractionPath, 'skill'), stagingPath) journal = { schemaVersion: 1, operation: 'install', @@ -191,7 +210,8 @@ export async function installLocalSkillPackage( receipt } const statePath = skillInstallJournalPath(input.stateDirectory, canonicalPath) - await writeSkillStateFile(statePath, journal) + await persistJournalTransition(statePath, journal, dependencies) + await filesystem.rename(join(extracted.extractionPath, 'skill'), stagingPath) throwIfCancelled(input.signal) const commitState = await inspectSkillCanonicalState({ canonicalPath, @@ -212,12 +232,12 @@ export async function installLocalSkillPackage( if (destinationExists) { await filesystem.rename(canonicalPath, backupPath) journal.phase = 'backup-created' - await writeSkillStateFile(statePath, journal) + await persistJournalTransition(statePath, journal, dependencies) throwIfCancelled(input.signal) } await filesystem.rename(stagingPath, canonicalPath) journal.phase = 'canonical-placed' - await writeSkillStateFile(statePath, journal) + await persistJournalTransition(statePath, journal, dependencies) if ( !(await skillInstallDestinationMatches( canonicalPath, @@ -230,10 +250,10 @@ export async function installLocalSkillPackage( } await writeSkillInstallReceipt(input.stateDirectory, receipt) journal.phase = 'receipt-published' - await writeSkillStateFile(statePath, journal) + await persistJournalTransition(statePath, journal, dependencies) await cleanSkillInstallJournalFiles(journal, filesystem) journal.phase = 'complete' - await writeSkillStateFile(statePath, journal) + await persistJournalTransition(statePath, journal, dependencies) await rm(statePath, { force: true }) return { operationId: input.operationId, diff --git a/src/main/skills/skill-package-creation.test.ts b/src/main/skills/skill-package-creation.test.ts index 67ec5608a8d..6d15ae1918b 100644 --- a/src/main/skills/skill-package-creation.test.ts +++ b/src/main/skills/skill-package-creation.test.ts @@ -1,4 +1,4 @@ -import { chmod, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { chmod, lstat, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { gunzipSync, gzipSync } from 'node:zlib' @@ -227,6 +227,33 @@ describe('skill package creation and extraction', () => { ).rejects.toThrow('skill-package-tar-truncated') }) + it('cancels during streamed extraction and removes partial bytes', async () => { + const root = await temporaryDirectory() + const created = await createSkillPackageArchive({ + sourceDirectory: await createSkill(root), + archivePath: join(root, 'package.tar.gz'), + packageId: 'package_1', + versionId: 'version_1' + }) + let checks = 0 + const signal = { + get aborted() { + checks += 1 + return checks >= 3 + } + } as AbortSignal + const destinationDirectory = join(root, 'cancelled-extraction') + + await expect( + extractSkillPackageArchive({ + archivePath: created.archivePath, + destinationDirectory, + signal + }) + ).rejects.toThrow('skill-install-cancelled') + await expect(lstat(destinationDirectory)).rejects.toMatchObject({ code: 'ENOENT' }) + }) + it('rejects content checksum and SKILL identity mismatches', async () => { const root = await temporaryDirectory() const created = await createSkillPackageArchive({ diff --git a/src/main/skills/skill-package-identity.test.ts b/src/main/skills/skill-package-identity.test.ts index 7d362be9e7d..031d0ab7530 100644 --- a/src/main/skills/skill-package-identity.test.ts +++ b/src/main/skills/skill-package-identity.test.ts @@ -1,6 +1,9 @@ -import { chmod, cp, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' +import { execFile } from 'node:child_process' +import { createServer } from 'node:net' +import { chmod, cp, link, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { promisify } from 'node:util' import { afterEach, describe, expect, it } from 'vitest' import { describeObservedSkillFile, @@ -11,6 +14,7 @@ import { } from './skill-package-identity' const temporaryDirectories: string[] = [] +const execFileAsync = promisify(execFile) async function temporarySkill(): Promise { const root = await mkdtemp(join(tmpdir(), 'orca-skill-freshness-')) @@ -92,6 +96,32 @@ describe('skill package identity', () => { ).rejects.toThrow('skill-package-entry-limit') }) + it.runIf(process.platform !== 'win32')('rejects hardlinks, FIFOs, and Unix sockets', async () => { + const root = await temporarySkill() + const source = join(root, 'SKILL.md') + await writeFile(source, 'skill') + await link(source, join(root, 'hardlink.md')) + await expect(observeSkillPackage(root)).rejects.toThrow('skill-package-link') + await rm(join(root, 'hardlink.md')) + + const fifo = join(root, 'named-pipe') + await execFileAsync('mkfifo', [fifo]) + await expect(observeSkillPackage(root)).rejects.toThrow('skill-package-special-file') + await rm(fifo) + + const socket = join(root, 'unix-socket') + const server = createServer() + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(socket, resolve) + }) + try { + await expect(observeSkillPackage(root)).rejects.toThrow('skill-package-special-file') + } finally { + await new Promise((resolve) => server.close(() => resolve())) + } + }) + it('ignores OS-authored sidecars so a browsed folder still matches its snapshot', async () => { const pristine = await temporarySkill() await writeFile(join(pristine, 'SKILL.md'), 'skill\n') diff --git a/src/main/skills/skill-package-identity.ts b/src/main/skills/skill-package-identity.ts index 11419fab771..f9581d0b792 100644 --- a/src/main/skills/skill-package-identity.ts +++ b/src/main/skills/skill-package-identity.ts @@ -234,6 +234,9 @@ export async function observeSkillPackage( } await visit(absolutePath, depth + 1) } else if (fileStat.isFile()) { + if (fileStat.nlink !== 1) { + throw new Error('skill-package-link') + } if (files.length >= limits.maximumFiles) { throw new Error('skill-package-file-count-limit') } diff --git a/src/main/skills/skill-package-tar.test.ts b/src/main/skills/skill-package-tar.test.ts new file mode 100644 index 00000000000..4354c5d0443 --- /dev/null +++ b/src/main/skills/skill-package-tar.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it } from 'vitest' +import { parseSkillTarHeader, SKILL_TAR_BLOCK_BYTES } from './skill-package-tar' + +function writeOctal(header: Buffer, offset: number, length: number, value: number): void { + header.write(`${value.toString(8).padStart(length - 1, '0')}\0`, offset, length, 'ascii') +} + +function refreshChecksum(header: Buffer): void { + header.fill(0x20, 148, 156) + const checksum = header.reduce((sum, byte) => sum + byte, 0) + header.write(`${checksum.toString(8).padStart(6, '0')}\0 `, 148, 8, 'ascii') +} + +function header(type = '0'): Buffer { + const value = Buffer.alloc(SKILL_TAR_BLOCK_BYTES) + value.write('skill/SKILL.md', 0, 'utf8') + writeOctal(value, 100, 8, 0o644) + writeOctal(value, 108, 8, 0) + writeOctal(value, 116, 8, 0) + writeOctal(value, 124, 12, 0) + writeOctal(value, 136, 12, 0) + value[156] = type.charCodeAt(0) + value.write('ustar\0', 257, 'ascii') + value.write('00', 263, 'ascii') + refreshChecksum(value) + return value +} + +describe('skill package tar envelope', () => { + it.each(['1', '2', '3', '4', '5', '6', '7', 'x', 'g', 'L', 'K', 's'])( + 'rejects non-regular tar entry type %s', + (type) => { + expect(() => parseSkillTarHeader(header(type))).toThrow('skill-package-tar-entry-type') + } + ) + + it('requires the deterministic ustar dialect', () => { + const legacy = header() + legacy.fill(0, 257, 265) + refreshChecksum(legacy) + expect(() => parseSkillTarHeader(legacy)).toThrow('skill-package-tar-format-invalid') + }) + + it('fuzzes fixed-size headers without unbounded parsing or non-Error failures', () => { + let state = 0x51a7e + const randomByte = (): number => { + state = (state * 1664525 + 1013904223) >>> 0 + return state & 0xff + } + for (let sample = 0; sample < 5_000; sample += 1) { + const value = Buffer.allocUnsafe(SKILL_TAR_BLOCK_BYTES) + for (let index = 0; index < value.length; index += 1) { + value[index] = randomByte() + } + refreshChecksum(value) + try { + const parsed = parseSkillTarHeader(value) + expect(parsed === null || Buffer.byteLength(parsed.path, 'utf8') <= 256).toBe(true) + } catch (error) { + expect(error).toBeInstanceOf(Error) + } + } + }) +}) diff --git a/src/main/skills/skill-package-tar.ts b/src/main/skills/skill-package-tar.ts index 2025f6949d1..f2a453fcff9 100644 --- a/src/main/skills/skill-package-tar.ts +++ b/src/main/skills/skill-package-tar.ts @@ -224,6 +224,9 @@ export function parseSkillTarHeader(header: Buffer): SkillTarReadEntry | null { return null } validateTarChecksum(header) + if (readString(header, 257, 6) !== 'ustar' || readString(header, 263, 2) !== '00') { + throw new Error('skill-package-tar-format-invalid') + } const type = header[156] if (type !== 0 && type !== '0'.charCodeAt(0)) { throw new Error('skill-package-tar-entry-type') diff --git a/src/main/skills/skill-remove-recovery.ts b/src/main/skills/skill-remove-recovery.ts index 0aeefd3b461..5bdde1dc969 100644 --- a/src/main/skills/skill-remove-recovery.ts +++ b/src/main/skills/skill-remove-recovery.ts @@ -3,7 +3,11 @@ import { basename, dirname, isAbsolute, join, relative, resolve } from 'node:pat import type { SkillPlacementResult } from '../../shared/skill-install-contract' import type { SkillInstallFilesystem } from './skill-install-filesystem' import { nativeSkillInstallFilesystem } from './skill-install-filesystem' -import { skillInstallStateKey, type SkillInstallReceiptV1 } from './skill-install-provenance' +import { + skillInstallStateKey, + writeSkillInstallReceipt, + type SkillInstallReceiptV1 +} from './skill-install-provenance' export type RemovalMove = { sourcePath: string @@ -190,5 +194,6 @@ export async function recoverSkillRemovalTransaction( await filesystem.rename(move.backupPath, move.sourcePath) } } + await writeSkillInstallReceipt(stateDirectory, journal.receipt) await rm(skillRemovalJournalPath(stateDirectory, canonicalPath), { force: true }) } diff --git a/src/main/skills/skill-remove-transaction.test.ts b/src/main/skills/skill-remove-transaction.test.ts index 32bb28966f1..96bb88f4ea4 100644 --- a/src/main/skills/skill-remove-transaction.test.ts +++ b/src/main/skills/skill-remove-transaction.test.ts @@ -11,6 +11,7 @@ import { import { installLocalSkillPackage } from './skill-install-transaction' import { recoverSkillRemovalTransaction, + removeLocalSharedSkill, skillRemovalJournalPath, type SkillRemovalJournalV1 } from './skill-remove-transaction' @@ -152,4 +153,48 @@ describe('skill removal recovery', () => { ).rejects.toThrow('skill-removal-recovery-conflict') expect(await readFile(join(backupPath, 'local.md'), 'utf8')).toBe('changed') }) + + it.each( + ['prepared', 'moving', 'receipt-removed'].flatMap((phase) => + ['before', 'after'].map((boundary) => [phase, boundary] as const) + ) + )('recovers failure %s the %s journal transition', async (phase, boundary) => { + const fixture = await installedFixture() + await expect( + removeLocalSharedSkill( + { + operationId: 'remove', + canonicalPath: fixture.canonicalPath, + stateDirectory: fixture.stateDirectory, + allowedProviderRoots: [] + }, + { + onJournalTransition: async (currentPhase, currentBoundary) => { + if (currentPhase === phase && currentBoundary === boundary) { + throw new Error(`injected-${phase}-${boundary}`) + } + } + } + ) + ).rejects.toThrow(`injected-${phase}-${boundary}`) + + const canonicalExists = await readFile(join(fixture.canonicalPath, 'SKILL.md'), 'utf8') + .then(() => true) + .catch(() => false) + const receipt = await readSkillInstallReceipt(fixture.stateDirectory, fixture.canonicalPath) + expect(Boolean(receipt)).toBe(canonicalExists) + await expect( + readFile(skillRemovalJournalPath(fixture.stateDirectory, fixture.canonicalPath)) + ).rejects.toMatchObject({ code: 'ENOENT' }) + if (canonicalExists) { + await expect( + removeLocalSharedSkill({ + operationId: 'retry-remove', + canonicalPath: fixture.canonicalPath, + stateDirectory: fixture.stateDirectory, + allowedProviderRoots: [] + }) + ).resolves.toMatchObject({ status: 'removed' }) + } + }) }) diff --git a/src/main/skills/skill-remove-transaction.ts b/src/main/skills/skill-remove-transaction.ts index b9be5c92061..2ccf91b23fd 100644 --- a/src/main/skills/skill-remove-transaction.ts +++ b/src/main/skills/skill-remove-transaction.ts @@ -38,6 +38,13 @@ export type LocalSkillRemovalInput = { filesystem?: SkillInstallFilesystem } +export type SkillRemovalTransactionDependencies = { + onJournalTransition?: ( + phase: SkillRemovalJournalV1['phase'], + boundary: 'before' | 'after' + ) => Promise +} + function lockPath(stateDirectory: string, canonicalPath: string): string { return join(stateDirectory, 'locks', `${skillInstallStateKey(canonicalPath)}.lock`) } @@ -84,8 +91,19 @@ async function inspectCanonicalRemoval( return observed?.observedDigest === receipt.packageDigest ? 'owned' : 'modified' } +async function persistRemovalJournal( + path: string, + journal: SkillRemovalJournalV1, + dependencies: SkillRemovalTransactionDependencies +): Promise { + await dependencies.onJournalTransition?.(journal.phase, 'before') + await writeSkillStateFile(path, journal) + await dependencies.onJournalTransition?.(journal.phase, 'after') +} + export async function removeLocalSharedSkill( - input: LocalSkillRemovalInput + input: LocalSkillRemovalInput, + dependencies: SkillRemovalTransactionDependencies = {} ): Promise { const filesystem = input.filesystem ?? nativeSkillInstallFilesystem const releaseLock = await acquireSkillInstallLock({ @@ -178,17 +196,17 @@ export async function removeLocalSharedSkill( allowedProviderRoots: [...input.allowedProviderRoots] } const statePath = skillRemovalJournalPath(input.stateDirectory, input.canonicalPath) - await writeSkillStateFile(statePath, journal) + await persistRemovalJournal(statePath, journal, dependencies) for (const move of moves) { journal.movedCount += 1 journal.phase = 'moving' - await writeSkillStateFile(statePath, journal) + await persistRemovalJournal(statePath, journal, dependencies) await filesystem.rename(move.sourcePath, move.backupPath) removedPlacements.push({ ...move.placement, status: 'removed' }) } await removeSkillInstallReceipt(input.stateDirectory, input.canonicalPath) journal.phase = 'receipt-removed' - await writeSkillStateFile(statePath, journal) + await persistRemovalJournal(statePath, journal, dependencies) await recoverSkillRemovalTransaction(input.stateDirectory, input.canonicalPath, filesystem) return { operationId: input.operationId, diff --git a/src/main/skills/skill-runtime-capability.test.ts b/src/main/skills/skill-runtime-capability.test.ts new file mode 100644 index 00000000000..e191eb4c6b1 --- /dev/null +++ b/src/main/skills/skill-runtime-capability.test.ts @@ -0,0 +1,43 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { getRuntimeEnvironmentStatus } from '../ipc/runtime-environment-transport-routing' +import { + supportsSkillRuntimeInstall, + supportsSkillRuntimeManagement +} from './skill-runtime-capability' + +vi.mock('../ipc/runtime-environment-transport-routing', () => ({ + getRuntimeEnvironmentStatus: vi.fn() +})) + +const status = vi.mocked(getRuntimeEnvironmentStatus) + +beforeEach(() => { + status.mockReset() +}) + +describe('skill runtime capability admission', () => { + it('admits install and management independently', async () => { + status.mockResolvedValue({ + ok: true, + result: { + capabilities: ['skills.install.v1'], + runtimeId: 'runtime_1' + } + } as never) + + await expect(supportsSkillRuntimeInstall('/state', 'environment_1')).resolves.toBe(true) + await expect(supportsSkillRuntimeManagement('/state', 'environment_1')).resolves.toBe(false) + }) + + it('fails closed for unavailable and capability-omitting hosts', async () => { + status + .mockResolvedValueOnce({ ok: false, error: { code: 'offline', message: 'offline' } } as never) + .mockResolvedValueOnce({ + ok: true, + result: { runtimeId: 'runtime_1', capabilities: [] } + } as never) + + await expect(supportsSkillRuntimeInstall('/state', 'environment_1')).resolves.toBe(false) + await expect(supportsSkillRuntimeInstall('/state', 'environment_1')).resolves.toBe(false) + }) +}) diff --git a/src/main/skills/skill-runtime-capability.ts b/src/main/skills/skill-runtime-capability.ts index e281afbd4f1..8d5959da25e 100644 --- a/src/main/skills/skill-runtime-capability.ts +++ b/src/main/skills/skill-runtime-capability.ts @@ -1,4 +1,7 @@ -import { SKILL_MANAGEMENT_CAPABILITY } from '../../shared/skill-install-capability' +import { + SKILL_INSTALL_CAPABILITY, + SKILL_MANAGEMENT_CAPABILITY +} from '../../shared/skill-install-capability' import { getRuntimeEnvironmentStatus } from '../ipc/runtime-environment-transport-routing' export async function supportsSkillRuntimeManagement( @@ -10,3 +13,13 @@ export async function supportsSkillRuntimeManagement( status.ok === true && status.result.capabilities?.includes(SKILL_MANAGEMENT_CAPABILITY) === true ) } + +export async function supportsSkillRuntimeInstall( + userDataPath: string, + environmentId: string +): Promise { + const status = await getRuntimeEnvironmentStatus(userDataPath, environmentId, 15_000) + return ( + status.ok === true && status.result.capabilities?.includes(SKILL_INSTALL_CAPABILITY) === true + ) +} diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index c2e06eaeda2..f88b4450a15 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -416,6 +416,7 @@ import type { SkillShareInstallInput, SkillShareInstallOperation, SkillInstallCancelInput, + SkillInstallProgress, SkillSharePreview, SkillShareProgress, SkillSharePublishInput, @@ -2598,6 +2599,7 @@ export type PreloadApi = { }) => Promise> deletePackage: (packageId: string) => Promise> listWslDistros: (environmentId?: string) => Promise + onInstallProgress: (callback: (progress: SkillInstallProgress) => void) => () => void onShareProgress: (callback: (progress: SkillShareProgress) => void) => () => void onUpdateRun: (callback: (run: SkillUpdateRun) => void) => () => void } diff --git a/src/preload/index.ts b/src/preload/index.ts index f11caf00a03..7b8390d566c 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -144,6 +144,7 @@ import type { SkillShareInstallInput, SkillShareInstallOperation, SkillInstallCancelInput, + SkillInstallProgress, SkillSharePreview, SkillShareProgress, SkillSharePublishInput, @@ -2565,6 +2566,12 @@ const api = { ipcRenderer.invoke('skills:deletePackage', packageId), listWslDistros: (environmentId?: string): Promise => ipcRenderer.invoke('skills:listWslDistros', environmentId), + onInstallProgress: (callback: (progress: SkillInstallProgress) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, progress: SkillInstallProgress): void => + callback(progress) + ipcRenderer.on('skills:installProgress', listener) + return () => ipcRenderer.removeListener('skills:installProgress', listener) + }, onShareProgress: (callback: (progress: SkillShareProgress) => void): (() => void) => { const listener = (_event: Electron.IpcRendererEvent, progress: SkillShareProgress): void => callback(progress) diff --git a/src/renderer/src/components/skills/SkillInstallDialog.test.tsx b/src/renderer/src/components/skills/SkillInstallDialog.test.tsx index c804cee9ffa..343536e8b98 100644 --- a/src/renderer/src/components/skills/SkillInstallDialog.test.tsx +++ b/src/renderer/src/components/skills/SkillInstallDialog.test.tsx @@ -44,6 +44,9 @@ function version(): SkillCloudVersion { } function installApi(previewInstall: ReturnType) { + let progressListener: + | ((progress: { operationId: string; phase: 'authorizing' | 'installing' }) => void) + | null = null return { resolveShare: vi .fn() @@ -60,7 +63,15 @@ function installApi(previewInstall: ReturnType) { } }), cancelInstall: vi.fn().mockResolvedValue({ cancelled: true }), - listWslDistros: vi.fn().mockResolvedValue([]) + listWslDistros: vi.fn().mockResolvedValue([]), + onInstallProgress: vi.fn((listener) => { + progressListener = listener + return () => { + progressListener = null + } + }), + emitInstallProgress: (progress: { operationId: string; phase: 'authorizing' | 'installing' }) => + progressListener?.(progress) } } @@ -202,4 +213,51 @@ describe('SkillInstallDialog', () => { await screen.findByText('Installation cancelled.') expect(screen.getByRole('button', { name: 'Retry install' })).toBeDefined() }) + + it('announces install phases and allows incomplete provider coverage to retry', async () => { + const previewInstall = vi.fn().mockResolvedValue({ + status: 'ok', + value: { + name: 'private-skill', + packageDigest: DIGEST, + destinationIdentity: 'local:global', + currentState: 'missing', + providers: [] + } + }) + let settleInstall: ((value: unknown) => void) | undefined + const skills = installApi(previewInstall) + skills.installShare.mockImplementation( + () => new Promise((resolve) => (settleInstall = resolve)) as never + ) + Object.defineProperty(window, 'api', { configurable: true, value: { skills } }) + render( undefined} />) + await inspectSkill() + + fireEvent.click(screen.getByRole('button', { name: 'Install skill' })) + await screen.findByText('Authorizing package access…') + const operationId = skills.installShare.mock.calls[0]?.[0].operationId + skills.emitInstallProgress({ operationId, phase: 'installing' }) + await screen.findByText('Downloading, verifying, and installing…') + settleInstall?.({ + status: 'ok', + value: { + operationId, + status: 'partial', + name: 'private-skill', + packageDigest: DIGEST, + placements: [ + { + provider: 'codex', + path: '/redacted-in-ui', + topology: 'provider-alias', + status: 'failed', + errorCategory: 'skill-placement-permission-denied' + } + ] + } + }) + + await screen.findByRole('button', { name: 'Retry install' }) + }) }) diff --git a/src/renderer/src/components/skills/SkillInstallDialog.tsx b/src/renderer/src/components/skills/SkillInstallDialog.tsx index 7d68cafafca..cf10dcd7ebb 100644 --- a/src/renderer/src/components/skills/SkillInstallDialog.tsx +++ b/src/renderer/src/components/skills/SkillInstallDialog.tsx @@ -25,6 +25,7 @@ import { SkillShareLinkInputForm } from './SkillInstallReviewContent' import { notifyInstalledAgentSkillsChanged } from '@/hooks/useInstalledAgentSkills' +import { useSkillInstallProgress } from './skill-install-progress-state' export function SkillInstallDialog({ open, @@ -54,7 +55,7 @@ export function SkillInstallDialog({ const [error, setError] = useState(null) const [result, setResult] = useState(null) const [destinationPreview, setDestinationPreview] = useState(null) - const [activeOperationId, setActiveOperationId] = useState(null) + const installProgress = useSkillInstallProgress() const workspaceChoices = useMemo( () => skillInstallWorkspaceChoices({ environmentId, folderWorkspaces, repos, worktreesByRepo }), @@ -153,7 +154,7 @@ export function SkillInstallDialog({ } } const operationId = crypto.randomUUID() - setActiveOperationId(operationId) + installProgress.begin(operationId) const operation = await window.api.skills.installShare({ shareId: preview.shareId, operationId, @@ -181,17 +182,17 @@ export function SkillInstallDialog({ console.warn('[skills] install failed:', cause) setError('Installation failed before Orca could verify the requested version.') } finally { - setActiveOperationId(null) + installProgress.finish() setBusy(false) } } const cancelInstall = async (): Promise => { - if (!activeOperationId) { + if (!installProgress.activeOperationId) { return } const cancelled = await window.api.skills.cancelInstall({ - operationId: activeOperationId, + operationId: installProgress.activeOperationId, ...(environmentId === 'local' || environmentId.startsWith('ssh:') ? {} : { environmentId }) }) if (!cancelled.cancelled) { @@ -271,16 +272,22 @@ export function SkillInstallDialog({ {error}

) : null} + {installProgress.phaseLabel ? ( +

+ {installProgress.phaseLabel} +

+ ) : null} - {busy && activeOperationId ? ( + {busy && installProgress.activeOperationId ? ( ) : null} - {preview && (!result || ['conflict', 'failed', 'cancelled'].includes(result.status)) ? ( + {preview && + (!result || ['conflict', 'partial', 'failed', 'cancelled'].includes(result.status)) ? ( - {activeOperationId ? ( + {installProgress.activeOperationId ? ( @@ -377,6 +384,11 @@ export function SkillInstallManagementDialog({ {skillInstallResultLabel(result)}

) : null} + {installProgress.phaseLabel ? ( +

+ {installProgress.phaseLabel} +

+ ) : null} {error ? (

{error} diff --git a/src/renderer/src/components/skills/SkillsPage.test.tsx b/src/renderer/src/components/skills/SkillsPage.test.tsx index 84bba21ef12..5e5b51e870c 100644 --- a/src/renderer/src/components/skills/SkillsPage.test.tsx +++ b/src/renderer/src/components/skills/SkillsPage.test.tsx @@ -37,6 +37,10 @@ function discoveryResult(names: string[]): SkillDiscoveryResult { return { skills: names.map(skill), sources: [], scannedAt: 1 } } +function skillsApi(discover: ReturnType) { + return { discover, onInstallProgress: () => () => undefined } +} + function deferred(): { promise: Promise; resolve: (value: T) => void } { let resolve!: (value: T) => void const promise = new Promise((resolvePromise) => { @@ -115,7 +119,7 @@ describe('SkillsPage', () => { ) Object.defineProperty(window, 'api', { configurable: true, - value: { skills: { discover }, runtimeEnvironments: { call } } + value: { skills: skillsApi(discover), runtimeEnvironments: { call } } }) setRuntimeOwner('env-1') @@ -142,7 +146,7 @@ describe('SkillsPage', () => { ) Object.defineProperty(window, 'api', { configurable: true, - value: { skills: { discover }, runtimeEnvironments: { call } } + value: { skills: skillsApi(discover), runtimeEnvironments: { call } } }) await renderPage() @@ -164,7 +168,7 @@ describe('SkillsPage', () => { const call = vi.fn() Object.defineProperty(window, 'api', { configurable: true, - value: { skills: { discover }, runtimeEnvironments: { call } } + value: { skills: skillsApi(discover), runtimeEnvironments: { call } } }) useAppStore.setState({ runtimeEnvironmentCatalogSettled: false }) diff --git a/src/renderer/src/components/skills/skill-install-progress-state.ts b/src/renderer/src/components/skills/skill-install-progress-state.ts new file mode 100644 index 00000000000..7451320a77d --- /dev/null +++ b/src/renderer/src/components/skills/skill-install-progress-state.ts @@ -0,0 +1,42 @@ +import { useEffect, useRef, useState } from 'react' + +const INSTALL_PHASE_LABELS = { + authorizing: 'Authorizing package access…', + installing: 'Downloading, verifying, and installing…' +} as const + +export function useSkillInstallProgress(): { + activeOperationId: string | null + phaseLabel: string | null + begin: (operationId: string) => void + finish: () => void +} { + const [activeOperationId, setActiveOperationId] = useState(null) + const activeOperationIdRef = useRef(null) + const [phase, setPhase] = useState(null) + + useEffect( + () => + window.api.skills.onInstallProgress((progress) => { + if (progress.operationId === activeOperationIdRef.current) { + setPhase(progress.phase) + } + }), + [] + ) + + return { + activeOperationId, + phaseLabel: phase ? INSTALL_PHASE_LABELS[phase] : null, + begin: (operationId) => { + activeOperationIdRef.current = operationId + setActiveOperationId(operationId) + setPhase('authorizing') + }, + finish: () => { + activeOperationIdRef.current = null + setActiveOperationId(null) + setPhase(null) + } + } +} diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index 9ee20276528..a4df8c02256 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -3100,6 +3100,7 @@ function createSkillsApi(): NonNullable['skills']> { deletePackage: () => Promise.reject(new Error('Skill package management requires the desktop app.')), listWslDistros: () => Promise.resolve([]), + onInstallProgress: () => () => {}, onShareProgress: () => () => {}, onUpdateRun: () => () => {} } diff --git a/src/shared/skill-install-contract.test.ts b/src/shared/skill-install-contract.test.ts new file mode 100644 index 00000000000..9a36f0dc16a --- /dev/null +++ b/src/shared/skill-install-contract.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest' +import { SkillInstallRequestSchema, SkillInstallResultSchema } from './skill-install-contract' + +const packageIdentity = { + packageId: 'package_1', + versionId: 'version_1', + packageDigest: 'a'.repeat(64), + archiveSha256: 'b'.repeat(64), + compressedBytes: 128 +} + +describe('skill install wire compatibility', () => { + it('accepts an older client request that omits every additive field', () => { + expect( + SkillInstallRequestSchema.parse({ + operationId: 'operation_1', + package: packageIdentity, + ingress: { + kind: 'download-grant', + url: 'https://storage.googleapis.com/package', + expiresAt: '2026-08-11T12:00:00.000Z' + }, + destination: { scope: 'global' } + }) + ).toMatchObject({ destination: { scope: 'global' } }) + }) + + it('accepts an older host result that omits additive failure and placement fields', () => { + expect( + SkillInstallResultSchema.parse({ + operationId: 'operation_1', + status: 'failed', + name: 'private-skill', + packageDigest: packageIdentity.packageDigest, + placements: [ + { + provider: 'claude', + path: '/host-owned/path', + topology: 'provider-alias', + status: 'failed' + } + ] + }) + ).toMatchObject({ status: 'failed', placements: [{ status: 'failed' }] }) + }) +}) diff --git a/src/shared/skill-sharing-contract.ts b/src/shared/skill-sharing-contract.ts index a14b06699fc..93a3ee97579 100644 --- a/src/shared/skill-sharing-contract.ts +++ b/src/shared/skill-sharing-contract.ts @@ -48,6 +48,11 @@ export type SkillShareProgress = { totalBytes: number } +export type SkillInstallProgress = { + operationId: string + phase: 'authorizing' | 'installing' +} + export type SkillShareInstallInput = { shareId: string operationId?: string