From 9925317654ae9743aaa0978e62e817b929800b31 Mon Sep 17 00:00:00 2001 From: Drswith <49299002+Drswith@users.noreply.github.com> Date: Wed, 1 Jul 2026 01:07:38 +0000 Subject: [PATCH] fix(lifecycle): fail closed on inconclusive npm ghost probes npm global list probing returned undefined on any non-zero exit without parsing stdout, so ghost uninstall recovery could clear tracked state while the managed package was still installed. Use scoped npm list probes with present/absent/unknown outcomes and only recover ghost state when absence is confirmed. --- .../.openspec.yaml | 2 + .../design.md | 30 +++++ .../proposal.md | 24 ++++ .../specs/agent-update/spec.md | 34 +++++ .../tasks.md | 10 ++ src/package-manager/index.ts | 5 + src/package-manager/installers.ts | 7 + src/package-manager/npm.ts | 43 +++++- test/package-manager/index.test.ts | 47 +++++++ test/package-manager/npm.test.ts | 122 ++++++++++++++++++ 10 files changed, 318 insertions(+), 6 deletions(-) create mode 100644 openspec/changes/fix-npm-ghost-probe-false-positive/.openspec.yaml create mode 100644 openspec/changes/fix-npm-ghost-probe-false-positive/design.md create mode 100644 openspec/changes/fix-npm-ghost-probe-false-positive/proposal.md create mode 100644 openspec/changes/fix-npm-ghost-probe-false-positive/specs/agent-update/spec.md create mode 100644 openspec/changes/fix-npm-ghost-probe-false-positive/tasks.md diff --git a/openspec/changes/fix-npm-ghost-probe-false-positive/.openspec.yaml b/openspec/changes/fix-npm-ghost-probe-false-positive/.openspec.yaml new file mode 100644 index 00000000..e7cc357c --- /dev/null +++ b/openspec/changes/fix-npm-ghost-probe-false-positive/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-01 diff --git a/openspec/changes/fix-npm-ghost-probe-false-positive/design.md b/openspec/changes/fix-npm-ghost-probe-false-positive/design.md new file mode 100644 index 00000000..00e19270 --- /dev/null +++ b/openspec/changes/fix-npm-ghost-probe-false-positive/design.md @@ -0,0 +1,30 @@ +## Context + +`isManagedPackageAbsent()` delegates to installer `getInstalledVersion` hooks. npm's implementation runs `npm list -g --depth=0 --json` and returns `undefined` whenever the command exits non-zero, even when stdout contains valid JSON showing the target package is still installed. + +## Goals / Non-Goals + +**Goals** + +- Confirm npm package absence only when a scoped probe parses valid JSON and the package is missing. +- Fail closed on inconclusive npm probes during ghost recovery. +- Keep version probing behavior stable for callers that only need a version string. + +**Non-Goals** + +- Rework Windows deferred binary self-upgrade semantics. +- Add doctor-only repair flows. +- Change bun or other installer ghost recovery beyond existing behavior. + +## Decisions + +- Add an npm `probePackagePresence` helper returning `present`, `absent`, or `unknown`. +- Use scoped `npm list -g --depth=0 --json` and parse stdout regardless of exit code when JSON is valid. +- Treat a matching dependency key as present even when its version is unreadable, and treat structured npm error payloads as unknown. +- Extend the managed installer interface with optional `probePackagePresence` and use it in `isManagedPackageAbsent()` when available. +- Fall back to the existing `getInstalledVersion === undefined` check for installers without the new hook. + +## Risks / Trade-offs + +- [Risk] Scoped npm list still returns inconclusive output on some broken npm installs. → Mitigation: return `unknown` and skip ghost recovery. +- [Risk] Extra probe hook adds interface surface. → Mitigation: optional hook used only for absence confirmation. diff --git a/openspec/changes/fix-npm-ghost-probe-false-positive/proposal.md b/openspec/changes/fix-npm-ghost-probe-false-positive/proposal.md new file mode 100644 index 00000000..886554f5 --- /dev/null +++ b/openspec/changes/fix-npm-ghost-probe-false-positive/proposal.md @@ -0,0 +1,24 @@ +## Why + +The recent ghost-state uninstall recovery path treats npm `getInstalledVersion` returning `undefined` as confirmed package absence. npm's global `list` probe returns `undefined` on any non-zero exit code without parsing stdout, so a broken global dependency tree or probe failure can clear tracked state while the agent package remains installed. + +## What Changes + +- Harden npm managed package presence probing for ghost uninstall recovery. +- Distinguish confirmed absence from inconclusive probe results before clearing state. +- Add regression tests for npm probe failure and confirmed-absence paths. + +## Capabilities + +### New Capabilities + +- None. + +### Modified Capabilities + +- `agent-update`: ghost uninstall recovery must not clear state when npm presence probing is inconclusive. + +## Impact + +- Affected code: `src/package-manager/npm.ts`, `src/package-manager/installers.ts`, `src/package-manager/index.ts`, related tests. +- No CLI flags, schema version, or command catalog changes. diff --git a/openspec/changes/fix-npm-ghost-probe-false-positive/specs/agent-update/spec.md b/openspec/changes/fix-npm-ghost-probe-false-positive/specs/agent-update/spec.md new file mode 100644 index 00000000..54f9b14e --- /dev/null +++ b/openspec/changes/fix-npm-ghost-probe-false-positive/specs/agent-update/spec.md @@ -0,0 +1,34 @@ +## MODIFIED Requirements + +### Requirement: Uninstall MUST recover ghost managed install state + +When a managed package is no longer installed but Quantex still records install state, uninstall SHALL clear the stale state entry after confirming package absence through the recorded installer. + +#### Scenario: Retrying uninstall after package removal succeeded but state persistence failed + +- **GIVEN** an agent has recorded managed install state +- **AND** the managed package is no longer installed on the system +- **AND** the recorded package manager is available and can confirm absence +- **WHEN** the user runs `quantex uninstall ` +- **AND** the managed package-manager uninstall command reports failure +- **THEN** Quantex removes the installed-agent state entry +- **AND** the uninstall command reports success + +#### Scenario: Ghost recovery does not run when the package manager is unavailable + +- **GIVEN** an agent has recorded managed install state +- **AND** the recorded package manager is unavailable +- **WHEN** the user runs `quantex uninstall ` +- **AND** the managed package-manager uninstall command reports failure +- **THEN** Quantex does not remove the installed-agent state entry +- **AND** the uninstall command reports failure + +#### Scenario: Ghost recovery does not run when npm presence probing is inconclusive + +- **GIVEN** an agent has recorded managed install state with install type `npm` +- **AND** npm is available +- **AND** npm global presence probing cannot confirm whether the package is installed or absent +- **WHEN** the user runs `quantex uninstall ` +- **AND** the managed package-manager uninstall command reports failure +- **THEN** Quantex does not remove the installed-agent state entry +- **AND** the uninstall command reports failure diff --git a/openspec/changes/fix-npm-ghost-probe-false-positive/tasks.md b/openspec/changes/fix-npm-ghost-probe-false-positive/tasks.md new file mode 100644 index 00000000..4b753d22 --- /dev/null +++ b/openspec/changes/fix-npm-ghost-probe-false-positive/tasks.md @@ -0,0 +1,10 @@ +## 1. Implementation + +- [x] 1.1 Add npm scoped presence probing with `present` / `absent` / `unknown` outcomes +- [x] 1.2 Wire optional installer `probePackagePresence` into ghost recovery +- [x] 1.3 Add regression tests for npm inconclusive probe and confirmed absence paths +- [x] 1.4 Fail closed for structured npm errors and package entries with unreadable versions + +## 2. Validation + +- [x] 2.1 Run `bun run lint`, `bun run format:check`, `bun run typecheck`, and `bun run test` diff --git a/src/package-manager/index.ts b/src/package-manager/index.ts index 760d1b45..d0d942de 100644 --- a/src/package-manager/index.ts +++ b/src/package-manager/index.ts @@ -350,6 +350,11 @@ async function isManagedPackageAbsent( const packageName = resolveManagedPackageName(state, agent) if (!packageName) return false + if (installer.probePackagePresence) { + const presence = await installer.probePackagePresence(packageName, state.packageTargetKind) + return presence === 'absent' + } + const installedVersion = await installer.getInstalledVersion(packageName, state.packageTargetKind) return installedVersion === undefined } diff --git a/src/package-manager/installers.ts b/src/package-manager/installers.ts index 894d4fa9..4a4c7489 100644 --- a/src/package-manager/installers.ts +++ b/src/package-manager/installers.ts @@ -34,9 +34,15 @@ export interface ManagedInstallerUpdateOptions { packageInstallArgs?: string[] } +export type ManagedPackagePresenceProbe = 'present' | 'absent' | 'unknown' + export interface ManagedInstaller { type: ManagedInstallType getInstalledVersion?: (packageName: string, packageTargetKind?: PackageTargetKind) => Promise + probePackagePresence?: ( + packageName: string, + packageTargetKind?: PackageTargetKind, + ) => Promise isAvailable: () => Promise install: ( packageName: string, @@ -123,6 +129,7 @@ const managedInstallers: Record = { npm: { type: 'npm', getInstalledVersion: async packageName => npmPm.getInstalledVersion(packageName), + probePackagePresence: async packageName => npmPm.probePackagePresence(packageName), isAvailable: async () => isNpmAvailable(), install: async packageName => npmPm.install(packageName), uninstall: async packageName => npmPm.uninstall(packageName), diff --git a/src/package-manager/npm.ts b/src/package-manager/npm.ts index ce93629e..d8c36c7e 100644 --- a/src/package-manager/npm.ts +++ b/src/package-manager/npm.ts @@ -79,21 +79,52 @@ export async function uninstall(packageName: string): Promise { } } -export async function getInstalledVersion(packageName: string): Promise { +export type PackagePresenceProbe = 'present' | 'absent' | 'unknown' + +interface PackagePresenceResult { + presence: PackagePresenceProbe + version?: string +} + +async function readPackagePresence(packageName: string): Promise { try { - const proc = spawnCommand(['npm', 'list', '-g', '--depth=0', '--json'], { + const proc = spawnCommand(['npm', 'list', '-g', packageName, '--depth=0', '--json'], { stdio: ['ignore', 'pipe', 'ignore'], }) - const { exitCode, stdout } = await readProcessOutput(proc) + const { stdout } = await readProcessOutput(proc) + if (!stdout.trim()) return { presence: 'unknown' } + + try { + const data = JSON.parse(stdout) as { + dependencies?: Record + error?: unknown + } + if (!data || typeof data !== 'object' || Array.isArray(data) || Object.hasOwn(data, 'error')) { + return { presence: 'unknown' } + } - if (exitCode !== 0) return undefined + if (data.dependencies && Object.hasOwn(data.dependencies, packageName)) { + const version = parseGlobalPackageVersion(stdout, packageName) + return version ? { presence: 'present', version } : { presence: 'present' } + } - return parseGlobalPackageVersion(stdout, packageName) + return { presence: 'absent' } + } catch { + return { presence: 'unknown' } + } } catch { - return undefined + return { presence: 'unknown' } } } +export async function probePackagePresence(packageName: string): Promise { + return (await readPackagePresence(packageName)).presence +} + +export async function getInstalledVersion(packageName: string): Promise { + return (await readPackagePresence(packageName)).version +} + export function parseGlobalPackageVersion(output: string, packageName: string): string | undefined { const data = JSON.parse(output) as { dependencies?: Record diff --git a/test/package-manager/index.test.ts b/test/package-manager/index.test.ts index cbdf01f0..e44e7e94 100644 --- a/test/package-manager/index.test.ts +++ b/test/package-manager/index.test.ts @@ -43,6 +43,7 @@ const npmInstallSpy = vi.spyOn(npmPm, 'install') const npmUpdateSpy = vi.spyOn(npmPm, 'update') const npmUpdateManySpy = vi.spyOn(npmPm, 'updateMany') const npmUninstallSpy = vi.spyOn(npmPm, 'uninstall') +const npmProbePackagePresenceSpy = vi.spyOn(npmPm, 'probePackagePresence') const uvInstallSpy = vi.spyOn(uvPm, 'install') const uvUpdateSpy = vi.spyOn(uvPm, 'update') const uvUpdateManySpy = vi.spyOn(uvPm, 'updateMany') @@ -96,6 +97,7 @@ beforeEach(() => { npmUpdateSpy.mockClear() npmUpdateManySpy.mockClear() npmUninstallSpy.mockClear() + npmProbePackagePresenceSpy.mockClear() uvInstallSpy.mockClear() uvUpdateSpy.mockClear() uvUpdateManySpy.mockClear() @@ -152,6 +154,7 @@ afterAll(() => { npmUpdateSpy.mockRestore() npmUpdateManySpy.mockRestore() npmUninstallSpy.mockRestore() + npmProbePackagePresenceSpy.mockRestore() uvInstallSpy.mockRestore() uvUpdateSpy.mockRestore() uvUpdateManySpy.mockRestore() @@ -1049,4 +1052,48 @@ describe('uninstallAgent', () => { expect(bunGetInstalledVersionSpy).not.toHaveBeenCalled() expect(removeInstalledAgentStateSpy).not.toHaveBeenCalled() }) + + it('recovers npm ghost state when uninstall fails but absence is confirmed', async () => { + getInstalledAgentStateSpy.mockResolvedValue({ + agentName: 'test-agent', + installType: 'npm', + packageName: 'test-pkg', + }) + isNpmSpy.mockResolvedValue(true) + npmUninstallSpy.mockResolvedValue(false) + npmProbePackagePresenceSpy.mockResolvedValue('absent') + + expect(await uninstallAgent(testAgent)).toBe(true) + expect(npmUninstallSpy).toHaveBeenCalledWith('test-pkg') + expect(npmProbePackagePresenceSpy).toHaveBeenCalledWith('test-pkg') + expect(removeInstalledAgentStateSpy).toHaveBeenCalledWith('test-agent') + }) + + it('does not recover npm ghost state when presence probing is inconclusive', async () => { + getInstalledAgentStateSpy.mockResolvedValue({ + agentName: 'test-agent', + installType: 'npm', + packageName: 'test-pkg', + }) + isNpmSpy.mockResolvedValue(true) + npmUninstallSpy.mockResolvedValue(false) + npmProbePackagePresenceSpy.mockResolvedValue('unknown') + + expect(await uninstallAgent(testAgent)).toBe(false) + expect(removeInstalledAgentStateSpy).not.toHaveBeenCalled() + }) + + it('does not recover npm ghost state when the package is still installed', async () => { + getInstalledAgentStateSpy.mockResolvedValue({ + agentName: 'test-agent', + installType: 'npm', + packageName: 'test-pkg', + }) + isNpmSpy.mockResolvedValue(true) + npmUninstallSpy.mockResolvedValue(false) + npmProbePackagePresenceSpy.mockResolvedValue('present') + + expect(await uninstallAgent(testAgent)).toBe(false) + expect(removeInstalledAgentStateSpy).not.toHaveBeenCalled() + }) }) diff --git a/test/package-manager/npm.test.ts b/test/package-manager/npm.test.ts index 1361d005..af1dcf7a 100644 --- a/test/package-manager/npm.test.ts +++ b/test/package-manager/npm.test.ts @@ -93,3 +93,125 @@ describe('npm uninstall', () => { expect(await uninstall('some-package')).toBe(false) }) }) + +function createListProc(exitCode: number, stdout: string) { + return { + exited: Promise.resolve(), + exitCode, + stdout, + } +} + +describe('parseGlobalPackageVersion', () => { + it('parses scoped and unscoped packages from npm global list JSON', async () => { + const { parseGlobalPackageVersion } = await import('../../src/package-manager/npm') + const output = JSON.stringify({ + dependencies: { + 'test-pkg': { version: '1.2.3' }, + '@scope/name': { version: '4.5.6' }, + }, + }) + + expect(parseGlobalPackageVersion(output, 'test-pkg')).toBe('1.2.3') + expect(parseGlobalPackageVersion(output, '@scope/name')).toBe('4.5.6') + expect(parseGlobalPackageVersion(output, 'missing-package')).toBeUndefined() + }) +}) + +describe('probePackagePresence', () => { + it('returns present when scoped npm list JSON includes the package', async () => { + const { probePackagePresence } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue( + createListProc( + 1, + JSON.stringify({ + dependencies: { + 'test-pkg': { version: '1.2.3' }, + }, + }), + ), + ) + + expect(await probePackagePresence('test-pkg')).toBe('present') + expect(mockSpawn).toHaveBeenCalledWith(['npm', 'list', '-g', 'test-pkg', '--depth=0', '--json'], expect.any(Object)) + }) + + it('returns absent when scoped npm list JSON omits the package', async () => { + const { probePackagePresence } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue( + createListProc( + 1, + JSON.stringify({ + dependencies: {}, + }), + ), + ) + + expect(await probePackagePresence('test-pkg')).toBe('absent') + }) + + it('returns present when the package entry exists without a readable version', async () => { + const { probePackagePresence } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue( + createListProc( + 1, + JSON.stringify({ + dependencies: { + 'test-pkg': { invalid: true }, + }, + }), + ), + ) + + expect(await probePackagePresence('test-pkg')).toBe('present') + }) + + it('returns unknown when npm reports a structured error', async () => { + const { probePackagePresence } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue( + createListProc( + 1, + JSON.stringify({ + error: { + code: 'EJSONPARSE', + summary: 'Invalid package metadata', + }, + }), + ), + ) + + expect(await probePackagePresence('test-pkg')).toBe('unknown') + }) + + it('returns unknown when npm list output is empty', async () => { + const { probePackagePresence } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue(createListProc(1, '')) + + expect(await probePackagePresence('test-pkg')).toBe('unknown') + }) + + it('returns unknown when npm list output is not valid JSON', async () => { + const { probePackagePresence } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue(createListProc(1, 'npm ERR! broken')) + + expect(await probePackagePresence('test-pkg')).toBe('unknown') + }) +}) + +describe('getInstalledVersion', () => { + it('returns the version when scoped npm list JSON includes the package', async () => { + const { getInstalledVersion } = await import('../../src/package-manager/npm') + mockSpawn.mockReturnValue( + createListProc( + 1, + JSON.stringify({ + dependencies: { + 'test-pkg': { version: '2.0.0' }, + }, + }), + ), + ) + + expect(await getInstalledVersion('test-pkg')).toBe('2.0.0') + }) +})