From f987f95d6a4e3ba5c3278cec0adda7149f6a3ecc Mon Sep 17 00:00:00 2001 From: Harjoth Khara Date: Tue, 13 Oct 2026 17:20:36 -0700 Subject: [PATCH] fix(cli): guard service install against a missing managed shim (#13985) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The `paperclipai` CLI can run Paperclip as a background service (launchd on macOS, a systemd user unit on Linux), and that service definition always launches the managed-install shim > - #12148 made onboarding create the managed install first (`ensureServiceShim()`), or refuse with a repair path, when the shim does not exist > - The standalone `paperclipai service install` command never got that guard: it calls `manager.install()` directly, reports `installed: true`, and leaves the supervisor crash-looping on a missing binary (for example after a global npm install) > - This pull request runs the same onboarding guard in `service install` before any service definition is written > - The benefit is that `service install` either produces a service that can start, or fails with the same repair instructions onboarding gives, and never reports success for a service that cannot run ## Linked Issues or Issue Description - Fixes: #13841 - Refs #12148 (the onboarding-path fix this extends), #11755 and #11620 (related global-npm / service issues, not addressed here) ## What Changed - `cli/src/onboard-service.ts`: moved the existing onboarding shim guard, unchanged, into an exported `ensureServiceShim()` so both entry points share it. It takes an optional `installIfMissing` flag that defaults to `true`, so onboarding behaves exactly as before. When only the shim is missing (a managed install is recorded and its current payload is still present), the guard now runs the installer's existing `smokePayload()` check (the payload must run and report its recorded version), then rewrites the shim in place with the existing `writeManagedShim()` instead of reinstalling. A payload that fails the check gets no new shim; the command stops with the repair hint. The recorded version and update channel stay unchanged, with no download and no payload pruning. Otherwise the existing onboarding fallback (reinstall the recorded git revision, or install this CLI's release version) is unchanged. - `cli/src/commands/service.ts`: `service install` now calls `ensureServiceShim()` after the service manager is resolved and before `manager.install()`. If the shim cannot be made available, the command throws `Background service not installed: . Run \`paperclipai install\`, then \`paperclipai service install\`.` (exit code 1) and writes no service definition. - With `--json`, the command only checks for the shim and does not start a managed install. `installCommand()` prints progress to stdout, which would corrupt the machine-readable result. JSON callers get a clean non-zero exit with the repair hint; interactive callers get the same behavior as onboarding (create the managed install, then the service). The JSON output shape and all existing flags are unchanged. - Docs: no change needed. `doc/INSTALLING.md` already says the service uses the managed-install shim; this makes the command enforce that. - Tests: a new command-level test (`service-install-command.test.ts`) and one `ensureServiceShim` case in `onboard-service.test.ts`. ## Verification - `pnpm exec vitest run cli/src/__tests__/service-install-command.test.ts` on unmodified `master` (efce9356): 3/3 tests fail (the guard is never called, and the missing-shim command resolves instead of rejecting). With this change: pass. - `pnpm exec vitest run cli/src/__tests__/service-install-command.test.ts cli/src/__tests__/onboard-service.test.ts cli/src/__tests__/service-health-check.test.ts`: 36/36 pass. - The tests cover: the guard runs before `manager.install` (call order); `--no-start-now` / `--no-start-on-login` are still forwarded; `installed: true` JSON is printed only on success; a failed guard rejects with the repair message, never calls `manager.install`, and prints nothing; `--json` uses check-only mode; check-only mode never calls `installCommand`; with a managed payload present, the repair smoke-tests the payload, then restores the shim in place without calling the installer, and rechecks the executable; a payload that fails the smoke test gets no shim and no reinstall. - `pnpm --filter paperclipai typecheck` and `pnpm --filter paperclipai build`: pass. - Repo-wide `pnpm -r typecheck` and `pnpm build` stop at the unchanged `paperclip-runner` package because `cargo` is not installed locally. They fail the same way on unmodified `master`, so CI is the authority there. - `pnpm test:run` locally: 32 server failures. 30 reproduce on unmodified `master` (AI-connection/auth and worktree-provisioning cases). The other 2 pass when run alone on both `master` and this branch. None are in `cli/`. - Manual check for reviewers: with no `~/.local/bin/paperclipai`, `paperclipai service install --json` now exits 1 with the repair hint and writes no LaunchAgent/unit. Without `--json`, it creates the managed install first, then the service. ## Risks - Low risk. Behavior changes only when the managed shim is missing, which is exactly the case that used to produce a crash-looping service. - Behavior shift: in that case, `service install --json` now fails instead of reporting a service that cannot start. Scripts that relied on the old false success will see exit code 1 and the repair instructions. - Onboarding is unchanged. The guard body moved into a named function, and onboarding calls it with the same defaults. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Implementation and tests: OpenAI Codex, model `gpt-6-astra` (agentic coding with tool use and code execution; context window not reported by the runtime). - Plan critique before implementation: OpenAI `gpt-5.6-sol` at high reasoning effort (read-only). - Coordination, scoping and PR description: Anthropic Claude Opus 5.5 (`claude-opus-5-5`, 1M context, tool use). - Independent code review of the exact head: Anthropic `claude-sonnet-5` at medium effort (read-only), no findings. - A human contributor directed the work and owns the change. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> --- cli/src/__tests__/onboard-service.test.ts | 96 +++++++++++++- .../__tests__/service-install-command.test.ts | 68 ++++++++++ cli/src/commands/service.ts | 11 ++ cli/src/onboard-service.ts | 121 ++++++++++-------- 4 files changed, 242 insertions(+), 54 deletions(-) create mode 100644 cli/src/__tests__/service-install-command.test.ts diff --git a/cli/src/__tests__/onboard-service.test.ts b/cli/src/__tests__/onboard-service.test.ts index 92697fc675c..58abea274e8 100644 --- a/cli/src/__tests__/onboard-service.test.ts +++ b/cli/src/__tests__/onboard-service.test.ts @@ -1,5 +1,8 @@ -import { describe, expect, it, vi } from "vitest"; +import fs from "node:fs"; +import path from "node:path"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { + ensureServiceShim, handleOnboardService, handoffToOnboardedService, isInstallableReleaseVersion, @@ -7,6 +10,97 @@ import { shouldOfferForegroundStart, } from "../onboard-service.js"; +import { readInstallManifest, resolveInstallStorePaths, writeManagedShim } from "../install-store.js"; +import { installCommand, runCommandWithDiagnostics, smokePayload } from "../commands/install.js"; +import { isExecutableFile, resolveServiceShimPath } from "../services/service-manager.js"; + +vi.mock("../version.js", () => ({ packageVersion: "2026.924.0" })); +vi.mock("../install-store.js", async (importOriginal) => ({ + ...await importOriginal(), + readInstallManifest: vi.fn(), + writeManagedShim: vi.fn(), +})); +vi.mock("../commands/install.js", () => ({ + installCommand: vi.fn(), + runCommandWithDiagnostics: vi.fn(), + smokePayload: vi.fn(), +})); +vi.mock("../services/service-manager.js", async (importOriginal) => ({ + ...await importOriginal(), + isExecutableFile: vi.fn(), +})); + +beforeEach(() => { + vi.clearAllMocks(); +}); + +it("does not install a missing shim when installation is disabled", async () => { + vi.mocked(isExecutableFile).mockResolvedValue(false); + + await expect(ensureServiceShim({ installIfMissing: false })).resolves.toEqual({ + ok: false, + installedNow: false, + reason: `no executable exists at ${resolveServiceShimPath()}`, + }); + expect(installCommand).not.toHaveBeenCalled(); +}); + +it("restores a missing shim without reinstalling the managed payload", async () => { + vi.mocked(readInstallManifest).mockReturnValueOnce({ + schemaVersion: 1, + source: "npm", + version: "2026.824.1", + channel: "latest", + payloadPath: "/managed/payload", + installedAt: "2026-08-24T00:00:00.000Z", + previous: [], + }); + vi.mocked(isExecutableFile).mockResolvedValueOnce(false).mockResolvedValueOnce(true); + + const entrypoint = path.join(resolveInstallStorePaths().currentPath, "node_modules", "paperclipai", "dist", "index.js"); + const existsSync = vi.spyOn(fs, "existsSync").mockImplementation((filePath) => filePath === entrypoint); + try { + await expect(ensureServiceShim()).resolves.toEqual({ ok: true, installedNow: true }); + expect(smokePayload).toHaveBeenCalledExactlyOnceWith( + resolveInstallStorePaths().currentPath, "2026.824.1", runCommandWithDiagnostics, + ); + expect(writeManagedShim).toHaveBeenCalledExactlyOnceWith(); + expect(vi.mocked(smokePayload).mock.invocationCallOrder[0]) + .toBeLessThan(vi.mocked(writeManagedShim).mock.invocationCallOrder[0]); + expect(installCommand).not.toHaveBeenCalled(); + } finally { + existsSync.mockRestore(); + } +}); + +it("does not restore a shim when the managed payload smoke check fails", async () => { + vi.mocked(readInstallManifest).mockReturnValueOnce({ + schemaVersion: 1, + source: "npm", + version: "2026.824.1", + channel: "latest", + payloadPath: "/managed/payload", + installedAt: "2026-08-24T00:00:00.000Z", + previous: [], + }); + vi.mocked(isExecutableFile).mockResolvedValue(false); + vi.mocked(smokePayload).mockRejectedValueOnce(new Error("payload dependency is missing")); + + const entrypoint = path.join(resolveInstallStorePaths().currentPath, "node_modules", "paperclipai", "dist", "index.js"); + const existsSync = vi.spyOn(fs, "existsSync").mockImplementation((filePath) => filePath === entrypoint); + try { + await expect(ensureServiceShim()).resolves.toEqual({ + ok: false, + installedNow: false, + reason: "payload dependency is missing", + }); + expect(writeManagedShim).not.toHaveBeenCalled(); + expect(installCommand).not.toHaveBeenCalled(); + } finally { + existsSync.mockRestore(); + } +}); + function dashboardConfig(overrides: { host?: string; port?: number; diff --git a/cli/src/__tests__/service-install-command.test.ts b/cli/src/__tests__/service-install-command.test.ts new file mode 100644 index 00000000000..170f126e835 --- /dev/null +++ b/cli/src/__tests__/service-install-command.test.ts @@ -0,0 +1,68 @@ +import { Command } from "commander"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { registerServiceCommands } from "../commands/service.js"; +import { ensureServiceShim } from "../onboard-service.js"; +import { detectServiceManager, type ServiceManager } from "../services/service-manager.js"; + +vi.mock("../onboard-service.js", () => ({ ensureServiceShim: vi.fn() })); +vi.mock("../services/service-manager.js", () => ({ detectServiceManager: vi.fn() })); + +const install = vi.fn(async () => ({ changed: true })); + +function runInstall(...args: string[]) { + const program = new Command(); + registerServiceCommands(program); + return program.parseAsync(["service", "install", ...args], { from: "user" }); +} + +describe("service install command", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.mocked(ensureServiceShim).mockResolvedValue({ ok: true, installedNow: false }); + vi.mocked(detectServiceManager).mockResolvedValue({ + supported: true, + manager: { + platform: "systemd", + serviceName: "paperclipai.service", + definitionPath: "/service/paperclipai.service", + install, + } as unknown as ServiceManager, + }); + }); + + afterEach(() => vi.restoreAllMocks()); + + it("checks the shim before installing and preserves JSON output and start flags", async () => { + await runInstall("--json", "--no-start-now", "--no-start-on-login"); + + expect(ensureServiceShim).toHaveBeenCalledWith({ installIfMissing: false }); + expect(vi.mocked(ensureServiceShim).mock.invocationCallOrder[0]).toBeLessThan(install.mock.invocationCallOrder[0]!); + expect(install).toHaveBeenCalledWith({ startNow: false, startOnLogin: false }); + expect(console.log).toHaveBeenCalledExactlyOnceWith(JSON.stringify({ + installed: true, + changed: true, + platform: "systemd", + serviceName: "paperclipai.service", + definitionPath: "/service/paperclipai.service", + lingerEnabled: false, + }, null, 2)); + }); + + it("rejects a missing shim without installing or printing success JSON", async () => { + vi.mocked(ensureServiceShim).mockResolvedValue({ ok: false, installedNow: false, reason: "missing shim" }); + + await expect(runInstall("--json")).rejects.toThrow( + "Background service not installed: missing shim. Run `paperclipai install`, then `paperclipai service install`.", + ); + expect(install).not.toHaveBeenCalled(); + expect(console.log).not.toHaveBeenCalled(); + }); + + it("allows creating the managed shim without --json", async () => { + await runInstall(); + + expect(ensureServiceShim).toHaveBeenCalledWith({ installIfMissing: true }); + expect(install).toHaveBeenCalledWith({ startNow: true, startOnLogin: true }); + }); +}); diff --git a/cli/src/commands/service.ts b/cli/src/commands/service.ts index 8e8a689dc6a..2f16dc88b63 100644 --- a/cli/src/commands/service.ts +++ b/cli/src/commands/service.ts @@ -5,6 +5,7 @@ import type { Command } from "commander"; import { readConfig, resolveConfigPath } from "../config/store.js"; import { resolvePaperclipInstanceId, resolvePaperclipInstanceRoot } from "../config/home.js"; import { detectServiceManager, type ServiceManager, type ServiceStatus } from "../services/service-manager.js"; +import { ensureServiceShim } from "../onboard-service.js"; import { buildLocalHealthUrl } from "../utils/health-url.js"; type CommonOptions = { instance?: string; json?: boolean }; @@ -172,6 +173,16 @@ export function registerServiceCommands(program: Command): void { .option("--enable-linger", "Allow systemd startup without an active login session", false) .action(async (opts) => { const manager = await resolveManager(opts); if (!manager) return; + const shim = await ensureServiceShim({ installIfMissing: !opts.json }); + if (!shim.ok) { + throw new Error( + `Background service not installed: ${shim.reason ?? "the managed install could not be completed"}. ` + + "Run `paperclipai install`, then `paperclipai service install`.", + ); + } + if (shim.installedNow) { + p.log.success("Installed the managed paperclipai payload and command shim for the service."); + } const result = await manager.install({ startNow: opts.startNow, startOnLogin: opts.startOnLogin }); let lingerEnabled = false; if (manager.enableLinger) { diff --git a/cli/src/onboard-service.ts b/cli/src/onboard-service.ts index 52a115953bb..b5b800926da 100644 --- a/cli/src/onboard-service.ts +++ b/cli/src/onboard-service.ts @@ -1,14 +1,16 @@ +import fs from "node:fs"; import path from "node:path"; import * as p from "@clack/prompts"; import pc from "picocolors"; import type { PaperclipConfig } from "./config/schema.js"; import { openUrl } from "./client/board-auth.js"; -import { installCommand } from "./commands/install.js"; +import { installCommand, runCommandWithDiagnostics, smokePayload } from "./commands/install.js"; import { resolvePaperclipInstanceId } from "./config/home.js"; import { readRuntimeInfo, type PaperclipRuntimeInfo } from "./runtime-info.js"; import { readInstallManifest, resolveInstallStorePaths, + writeManagedShim, type InstallManifest, } from "./install-store.js"; import { @@ -133,62 +135,75 @@ type OnboardServiceDependencies = { warn: (message: string) => void; }; -const defaultDependencies: OnboardServiceDependencies = { - detect: (instanceId) => detectServiceManager({ instanceId }), - // The service definition targets the managed shim. An ephemeral run (npx) - // never lays it down, so installing the service without this step creates - // a definition that crash-loops on a missing binary. - ensureServiceShim: async () => { - const shimPath = resolveServiceShimPath(); - if (await isExecutableFile(shimPath)) { - return { ok: true, installedNow: false }; - } - const storeShimPath = resolveInstallStorePaths().shimPath; - if (path.resolve(shimPath) !== path.resolve(storeShimPath)) { - return { - ok: false, - installedNow: false, - reason: `no executable exists at ${shimPath} (PAPERCLIP_SHIM_PATH), and it is outside the managed install store`, - }; - } - let manifest: InstallManifest | null = null; - try { - manifest = readInstallManifest(); - } catch {} - try { - if (manifest?.source === "git" && manifest.repo) { - // A managed git payload must be preserved as-is: reinstall the - // exact revision the manifest records, not an npm release. - await installCommand({ repo: manifest.repo, ref: manifest.sha ?? manifest.ref, yes: true }); - } else if (isInstallableReleaseVersion(packageVersion)) { - // packageVersion, not cliVersion: a managed executable's cliVersion - // carries provenance text that is not an installable npm spec. - await installCommand({ version: packageVersion, yes: true }); - } else { - return { - ok: false, - installedNow: false, - reason: - `this build reports version ${packageVersion}, which is not an installable release; ` + - "run `paperclipai install` (or `paperclipai install --repo --ref ` for source builds) first", - }; - } - } catch (error) { - return { - ok: false, - installedNow: false, - reason: error instanceof Error ? error.message : String(error), - }; - } - if (await isExecutableFile(shimPath)) { - return { ok: true, installedNow: true }; - } +// The service definition targets the managed shim. An ephemeral run (npx) +// never lays it down, so installing the service without this step creates +// a definition that crash-loops on a missing binary. +export async function ensureServiceShim(options: { installIfMissing?: boolean } = {}): Promise { + const shimPath = resolveServiceShimPath(); + if (await isExecutableFile(shimPath)) { + return { ok: true, installedNow: false }; + } + if (options.installIfMissing === false) { + return { ok: false, installedNow: false, reason: `no executable exists at ${shimPath}` }; + } + const storeShimPath = resolveInstallStorePaths().shimPath; + if (path.resolve(shimPath) !== path.resolve(storeShimPath)) { return { ok: false, installedNow: false, - reason: `the managed install completed but no executable shim appeared at ${shimPath}`, + reason: `no executable exists at ${shimPath} (PAPERCLIP_SHIM_PATH), and it is outside the managed install store`, }; - }, + } + let manifest: InstallManifest | null = null; + try { + manifest = readInstallManifest(); + } catch {} + try { + if ( + manifest && + fs.existsSync(path.join(resolveInstallStorePaths().currentPath, "node_modules", "paperclipai", "dist", "index.js")) + ) { + // Only the shim is missing: smoke-test and restore it for the recorded payload, + // keeping its version and update channel instead of reinstalling. + await smokePayload(resolveInstallStorePaths().currentPath, manifest.version, runCommandWithDiagnostics); + writeManagedShim(); + } else if (manifest?.source === "git" && manifest.repo) { + // A managed git payload must be preserved as-is: reinstall the + // exact revision the manifest records, not an npm release. + await installCommand({ repo: manifest.repo, ref: manifest.sha ?? manifest.ref, yes: true }); + } else if (isInstallableReleaseVersion(packageVersion)) { + // packageVersion, not cliVersion: a managed executable's cliVersion + // carries provenance text that is not an installable npm spec. + await installCommand({ version: packageVersion, yes: true }); + } else { + return { + ok: false, + installedNow: false, + reason: + `this build reports version ${packageVersion}, which is not an installable release; ` + + "run `paperclipai install` (or `paperclipai install --repo --ref ` for source builds) first", + }; + } + } catch (error) { + return { + ok: false, + installedNow: false, + reason: error instanceof Error ? error.message : String(error), + }; + } + if (await isExecutableFile(shimPath)) { + return { ok: true, installedNow: true }; + } + return { + ok: false, + installedNow: false, + reason: `the managed install completed but no executable shim appeared at ${shimPath}`, + }; +} + +const defaultDependencies: OnboardServiceDependencies = { + detect: (instanceId) => detectServiceManager({ instanceId }), + ensureServiceShim: () => ensureServiceShim(), confirm: async () => { const answer = await p.confirm({ message: "Install Paperclip as a background service?",