mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(coding-agent): start when a configured package fails to install (#11304)
fix(coding-agent): start when a configured package fails to install A package in settings.json that could not be installed (registry unreachable, package missing, failing git prepare step) crashed startup with an uncaught rejection and a raw Node stack. Resolving packages now catches the install failure, skips that package the same way offline mode does, and reports it as an extension warning: Warning: Extension package "npm:foo": failed to install (...). Run "pi remove npm:foo" to remove it from settings.json. Project packages point at `pi remove <source> --local`. `-e` sources say to check the source instead. Pi tries the install again on the next start or /reload. --help and --list-models now print runtime diagnostics, so the warning shows there too. pi config prints it as well. pi install and pi update --extensions still fail with a non-zero exit. closes #11088
This commit is contained in:
parent
9ffd27cfaa
commit
6dfb546785
8 changed files with 211 additions and 20 deletions
|
|
@ -16,6 +16,8 @@ pi install ./local-package
|
|||
|
||||
`pi list` shows configured packages. Use `pi remove <source>` to remove one and `pi update --extensions` to reconcile package installations. See [Command Line](cli.md#package-commands) for every package command and option.
|
||||
|
||||
If a configured package fails to install at startup, for example because npm cannot reach the registry, Pi warns about it, skips that package's resources, and keeps running. Pi tries to install it again on the next start or `/reload`. `pi install` and `pi update --extensions` still report the failure and exit non-zero.
|
||||
|
||||
Personal installs are written to `~/.pi/agent/settings.json`. Add `--local` or `-l` to write the package declaration to `.pi/settings.json`. Pi reads declarations from that file only after project trust is granted.
|
||||
|
||||
Project packages are installed and loaded only after project trust is resolved. Packages can execute extension code and can include skills that instruct the model to run programs. Review third-party package source before installing it. Review project package declarations before granting project trust.
|
||||
|
|
|
|||
|
|
@ -90,6 +90,15 @@ export interface ResolvedPaths {
|
|||
skills: ResolvedResource[];
|
||||
prompts: ResolvedResource[];
|
||||
themes: ResolvedResource[];
|
||||
/** Configured packages that were skipped because installing them failed. */
|
||||
installFailures?: PackageInstallFailure[];
|
||||
}
|
||||
|
||||
/** A configured package that could not be installed while resolving resources. */
|
||||
export interface PackageInstallFailure {
|
||||
source: string;
|
||||
scope: SourceScope;
|
||||
message: string;
|
||||
}
|
||||
|
||||
export type MissingSourceAction = "install" | "skip" | "error";
|
||||
|
|
@ -939,7 +948,7 @@ export class DefaultPackageManager implements PackageManager {
|
|||
|
||||
// Dedupe: project scope wins over global for same package identity
|
||||
const packageSources = this.dedupePackages(allPackages);
|
||||
await this.resolvePackageSources(packageSources, accumulator, onMissing);
|
||||
const installFailures = await this.resolvePackageSources(packageSources, accumulator, onMissing);
|
||||
|
||||
const globalBaseDir = this.agentDir;
|
||||
const projectBaseDir = join(this.cwd, CONFIG_DIR_NAME);
|
||||
|
|
@ -991,7 +1000,7 @@ export class DefaultPackageManager implements PackageManager {
|
|||
);
|
||||
}
|
||||
|
||||
return this.toResolvedPaths(accumulator);
|
||||
return { ...this.toResolvedPaths(accumulator), installFailures };
|
||||
}
|
||||
|
||||
async resolveExtensionSources(
|
||||
|
|
@ -1007,8 +1016,8 @@ export class DefaultPackageManager implements PackageManager {
|
|||
const packageSources = sources
|
||||
.filter((source) => !source.startsWith(BUILTIN_PATH_PREFIX))
|
||||
.map((source) => ({ pkg: source as PackageSource, scope }));
|
||||
await this.resolvePackageSources(packageSources, accumulator);
|
||||
return this.toResolvedPaths(accumulator);
|
||||
const installFailures = await this.resolvePackageSources(packageSources, accumulator);
|
||||
return { ...this.toResolvedPaths(accumulator), installFailures };
|
||||
}
|
||||
|
||||
listConfiguredPackages(): ConfiguredPackage[] {
|
||||
|
|
@ -1289,7 +1298,8 @@ export class DefaultPackageManager implements PackageManager {
|
|||
sources: Array<{ pkg: PackageSource; scope: SourceScope }>,
|
||||
accumulator: ResourceAccumulator,
|
||||
onMissing?: (source: string) => Promise<MissingSourceAction>,
|
||||
): Promise<void> {
|
||||
): Promise<PackageInstallFailure[]> {
|
||||
const installFailures: PackageInstallFailure[] = [];
|
||||
for (const { pkg, scope } of sources) {
|
||||
const sourceStr = typeof pkg === "string" ? pkg : pkg.source;
|
||||
const filter = typeof pkg === "object" ? pkg : undefined;
|
||||
|
|
@ -1307,15 +1317,24 @@ export class DefaultPackageManager implements PackageManager {
|
|||
|
||||
const installMissing = async (): Promise<boolean> => {
|
||||
if (isOfflineModeEnabled()) return false;
|
||||
if (!onMissing) {
|
||||
if (onMissing) {
|
||||
const action = await onMissing(resolvedSource);
|
||||
if (action === "skip") return false;
|
||||
if (action === "error") throw new Error(`Missing source: ${resolvedSource}`);
|
||||
}
|
||||
// A source that cannot be installed (no network, missing package, failing git prepare step)
|
||||
// must not abort startup. Skip its resources, like offline mode does, and report the failure.
|
||||
try {
|
||||
await this.installParsedSource(parsed, resolvedScope);
|
||||
return true;
|
||||
} catch (error) {
|
||||
installFailures.push({
|
||||
source: resolvedSource,
|
||||
scope: resolvedScope,
|
||||
message: error instanceof Error ? error.message : String(error),
|
||||
});
|
||||
return false;
|
||||
}
|
||||
const action = await onMissing(resolvedSource);
|
||||
if (action === "skip") return false;
|
||||
if (action === "error") throw new Error(`Missing source: ${resolvedSource}`);
|
||||
await this.installParsedSource(parsed, resolvedScope);
|
||||
return true;
|
||||
};
|
||||
|
||||
if (parsed.type === "npm") {
|
||||
|
|
@ -1346,6 +1365,7 @@ export class DefaultPackageManager implements PackageManager {
|
|||
this.collectPackageResources(installedPath, accumulator, filter, metadata);
|
||||
}
|
||||
}
|
||||
return installFailures;
|
||||
}
|
||||
|
||||
private findAutoloadDeltaBase(
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
|
|||
import { basename, dirname, join, resolve, sep } from "node:path";
|
||||
import { detectCapabilities, getTerminalColorMode, type TerminalColorMode } from "@earendil-works/pi-tui";
|
||||
import chalk from "chalk";
|
||||
import { CONFIG_DIR_NAME } from "../config.ts";
|
||||
import { APP_NAME, CONFIG_DIR_NAME } from "../config.ts";
|
||||
import { loadThemeFromPath, type Theme } from "../modes/interactive/theme/theme.ts";
|
||||
import type { ResourceDiagnostic } from "./diagnostics.ts";
|
||||
|
||||
|
|
@ -25,7 +25,12 @@ import type {
|
|||
LoadExtensionsResult,
|
||||
} from "./extensions/types.ts";
|
||||
import { findGitPaths } from "./footer-data-provider.ts";
|
||||
import { DefaultPackageManager, type PathMetadata, type ResolvedResource } from "./package-manager.ts";
|
||||
import {
|
||||
DefaultPackageManager,
|
||||
type PackageInstallFailure,
|
||||
type PathMetadata,
|
||||
type ResolvedResource,
|
||||
} from "./package-manager.ts";
|
||||
import type { PromptTemplate } from "./prompt-templates.ts";
|
||||
import { loadPromptTemplates } from "./prompt-templates.ts";
|
||||
import { SettingsManager } from "./settings-manager.ts";
|
||||
|
|
@ -96,6 +101,28 @@ function collectExtensionPackageWarnings(
|
|||
return warnings;
|
||||
}
|
||||
|
||||
/** Describe a package that was skipped because it could not be installed, including how to drop it. */
|
||||
function describePackageInstallFailure(failure: PackageInstallFailure): string {
|
||||
const hint =
|
||||
failure.scope === "temporary"
|
||||
? "Check the source and try again."
|
||||
: `Run "${APP_NAME} remove ${failure.source}${failure.scope === "project" ? " --local" : ""}" to remove it from settings.json.`;
|
||||
return `failed to install (${failure.message}). ${hint}`;
|
||||
}
|
||||
|
||||
/** One-line startup warning for a package that could not be installed. */
|
||||
export function formatPackageInstallWarning(failure: PackageInstallFailure): string {
|
||||
return `Extension package "${failure.source}": ${describePackageInstallFailure(failure)}`;
|
||||
}
|
||||
|
||||
function collectPackageInstallWarnings(
|
||||
...failures: Array<PackageInstallFailure[] | undefined>
|
||||
): Array<{ path: string; warning: string }> {
|
||||
return failures.flatMap((entries) =>
|
||||
(entries ?? []).map((failure) => ({ path: failure.source, warning: describePackageInstallFailure(failure) })),
|
||||
);
|
||||
}
|
||||
|
||||
function mergeExtensionWarnings(
|
||||
result: LoadExtensionsResult,
|
||||
warnings: Array<{ path: string; warning: string }>,
|
||||
|
|
@ -530,6 +557,10 @@ export class DefaultResourceLoader implements ResourceLoader {
|
|||
const cliExtensionPaths = await this.packageManager.resolveExtensionSources(this.additionalExtensionPaths, {
|
||||
temporary: true,
|
||||
});
|
||||
const installWarnings = collectPackageInstallWarnings(
|
||||
resolvedPaths.installFailures,
|
||||
cliExtensionPaths.installFailures,
|
||||
);
|
||||
// Kept on the instance so post-reload passes (extendResources) can still resolve package metadata.
|
||||
this.resourceMetadataByPath = new Map();
|
||||
const metadataByPath = this.resourceMetadataByPath;
|
||||
|
|
@ -584,7 +615,7 @@ export class DefaultResourceLoader implements ResourceLoader {
|
|||
|
||||
const packageWarnings = collectExtensionPackageWarnings(extensionPaths, metadataByPath);
|
||||
const extensionsResult = await this.loadFinalExtensionSet(extensionPaths, preTrustExtensions);
|
||||
mergeExtensionWarnings(extensionsResult, packageWarnings);
|
||||
mergeExtensionWarnings(extensionsResult, [...installWarnings, ...packageWarnings]);
|
||||
for (const p of this.additionalExtensionPaths) {
|
||||
if (isLocalPath(p)) {
|
||||
const resolved = this.resolveResourcePath(p);
|
||||
|
|
@ -696,8 +727,12 @@ export class DefaultResourceLoader implements ResourceLoader {
|
|||
]),
|
||||
);
|
||||
const packageWarnings = collectExtensionPackageWarnings(extensionPaths, metadataByPath);
|
||||
const installWarnings = collectPackageInstallWarnings(
|
||||
resolvedPaths.installFailures,
|
||||
cliExtensionPaths.installFailures,
|
||||
);
|
||||
const extensionsResult = await loadExtensionsCached(extensionPaths, this.cwd, this.eventBus);
|
||||
mergeExtensionWarnings(extensionsResult, packageWarnings);
|
||||
mergeExtensionWarnings(extensionsResult, [...installWarnings, ...packageWarnings]);
|
||||
if (!options.includeInlineFactories) {
|
||||
return extensionsResult;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -236,6 +236,7 @@ export {
|
|||
type ModelRuntimeAuthOverrides,
|
||||
} from "./core/model-runtime.ts";
|
||||
export type {
|
||||
PackageInstallFailure,
|
||||
PackageManager,
|
||||
PathMetadata,
|
||||
ProgressCallback,
|
||||
|
|
|
|||
|
|
@ -875,8 +875,11 @@ export async function main(args: string[], options?: MainOptions) {
|
|||
applyHttpProxySettings(settingsManager.getGlobalSettings().httpProxy);
|
||||
configureHttpDispatcher(settingsManager.getHttpIdleTimeoutMs());
|
||||
|
||||
const startupDiagnostics = deduplicateDiagnostics([...startupSettingsDiagnostics, ...runtime.diagnostics]);
|
||||
const hasRuntimeErrors = runtime.diagnostics.some((diagnostic) => diagnostic.type === "error");
|
||||
|
||||
if (parsed.help) {
|
||||
reportDiagnostics(startupSettingsDiagnostics);
|
||||
reportDiagnostics(startupDiagnostics);
|
||||
const extensionFlags = resourceLoader
|
||||
.getExtensions()
|
||||
.extensions.flatMap((extension) => Array.from(extension.flags.values()));
|
||||
|
|
@ -885,7 +888,7 @@ export async function main(args: string[], options?: MainOptions) {
|
|||
}
|
||||
|
||||
if (parsed.listModels !== undefined) {
|
||||
reportDiagnostics(startupSettingsDiagnostics);
|
||||
reportDiagnostics(startupDiagnostics);
|
||||
const searchPattern = typeof parsed.listModels === "string" ? parsed.listModels : undefined;
|
||||
await listModels(modelRuntime, searchPattern, AbortSignal.timeout(15_000));
|
||||
process.exit(0);
|
||||
|
|
@ -912,8 +915,6 @@ export async function main(args: string[], options?: MainOptions) {
|
|||
}
|
||||
|
||||
time("resolveModelScope");
|
||||
const startupDiagnostics = deduplicateDiagnostics([...startupSettingsDiagnostics, ...runtime.diagnostics]);
|
||||
const hasRuntimeErrors = runtime.diagnostics.some((diagnostic) => diagnostic.type === "error");
|
||||
if (appMode !== "interactive" || hasRuntimeErrors) {
|
||||
reportDiagnostics(startupDiagnostics);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -31,7 +31,7 @@ import type { InlineExtension } from "./core/extensions/types.ts";
|
|||
import { ModelRuntime } from "./core/model-runtime.ts";
|
||||
import { DefaultPackageManager } from "./core/package-manager.ts";
|
||||
import { type AppMode, resolveProjectTrusted } from "./core/project-trust.ts";
|
||||
import { DefaultResourceLoader, isBuiltinExtension } from "./core/resource-loader.ts";
|
||||
import { DefaultResourceLoader, formatPackageInstallWarning, isBuiltinExtension } from "./core/resource-loader.ts";
|
||||
import { SettingsManager } from "./core/settings-manager.ts";
|
||||
import { hasTrustRequiringProjectResources, ProjectTrustStore } from "./core/trust-manager.ts";
|
||||
import { spawnProcess, spawnProcessSync, waitForChildProcess } from "./utils/child-process.ts";
|
||||
|
|
@ -876,6 +876,14 @@ export async function handleConfigCommand(
|
|||
const projectResolvedPaths = settingsManager.isProjectTrusted()
|
||||
? await new DefaultPackageManager({ cwd, agentDir, settingsManager, builtinExtensions }).resolve()
|
||||
: globalResolvedPaths;
|
||||
const installWarnings = new Set(
|
||||
[...(globalResolvedPaths.installFailures ?? []), ...(projectResolvedPaths.installFailures ?? [])].map(
|
||||
formatPackageInstallWarning,
|
||||
),
|
||||
);
|
||||
for (const warning of installWarnings) {
|
||||
console.error(chalk.yellow(`Warning: ${warning}`));
|
||||
}
|
||||
|
||||
await selectConfig({
|
||||
resolvedPaths: { global: globalResolvedPaths, project: projectResolvedPaths },
|
||||
|
|
|
|||
|
|
@ -2406,6 +2406,46 @@ export default function(api) { api.registerTool({ name: "test", description: "te
|
|||
});
|
||||
});
|
||||
|
||||
describe("package install failures", () => {
|
||||
// #11088
|
||||
it("should skip a package source that fails to install and report it", async () => {
|
||||
settingsManager.setProjectPackages(["npm:broken-package"]);
|
||||
const installParsedSource = vi
|
||||
.spyOn(packageManager as any, "installParsedSource")
|
||||
.mockRejectedValue(new Error("npm install broken-package failed with code 1"));
|
||||
|
||||
const result = await packageManager.resolve();
|
||||
|
||||
expect(installParsedSource).toHaveBeenCalledTimes(1);
|
||||
expect(result.installFailures).toEqual([
|
||||
{
|
||||
source: "npm:broken-package",
|
||||
scope: "project",
|
||||
message: "npm install broken-package failed with code 1",
|
||||
},
|
||||
]);
|
||||
expect(result.extensions).toEqual([]);
|
||||
});
|
||||
|
||||
it("should not report install failures for sources that are skipped", async () => {
|
||||
settingsManager.setProjectPackages(["npm:missing-package"]);
|
||||
const installParsedSource = vi.spyOn(packageManager as any, "installParsedSource");
|
||||
|
||||
const result = await packageManager.resolve(async () => "skip");
|
||||
|
||||
expect(installParsedSource).not.toHaveBeenCalled();
|
||||
expect(result.installFailures).toEqual([]);
|
||||
});
|
||||
|
||||
it("should keep a missing source an error when the caller asks for one", async () => {
|
||||
settingsManager.setProjectPackages(["npm:missing-package"]);
|
||||
|
||||
await expect(packageManager.resolve(async () => "error")).rejects.toThrow(
|
||||
"Missing source: npm:missing-package",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("offline mode and network timeouts", () => {
|
||||
it("should update npm range packages using the configured spec", async () => {
|
||||
const installedPath = join(tempDir, ".pi", "npm", "node_modules", "example");
|
||||
|
|
|
|||
|
|
@ -84,6 +84,90 @@ describe("DefaultResourceLoader", () => {
|
|||
]);
|
||||
});
|
||||
|
||||
// #11088
|
||||
it("should warn and keep loading when a configured package cannot be installed", async () => {
|
||||
vi.stubEnv("PI_OFFLINE", "");
|
||||
const loader = new DefaultResourceLoader({
|
||||
cwd,
|
||||
agentDir,
|
||||
settingsManager: SettingsManager.inMemory({
|
||||
packages: ["npm:broken-package"],
|
||||
// Any non-zero exit stands in for an npm install that cannot reach the registry.
|
||||
npmCommand: [process.execPath, "-e", "process.exit(1)"],
|
||||
}),
|
||||
});
|
||||
|
||||
await loader.reload();
|
||||
|
||||
const warnings = loader.getExtensions().warnings ?? [];
|
||||
expect(warnings).toHaveLength(1);
|
||||
expect(warnings[0].path).toBe("npm:broken-package");
|
||||
expect(warnings[0].warning).toMatch(/^failed to install \(.*failed with code 1\)\./);
|
||||
expect(warnings[0].warning).toContain('Run "pi remove npm:broken-package" to remove it from settings.json.');
|
||||
});
|
||||
|
||||
// #11088
|
||||
it("should point temporary extension sources at the source instead of pi remove", async () => {
|
||||
vi.stubEnv("PI_OFFLINE", "");
|
||||
const loader = new DefaultResourceLoader({
|
||||
cwd,
|
||||
agentDir,
|
||||
additionalExtensionPaths: ["npm:broken-package"],
|
||||
settingsManager: SettingsManager.inMemory({
|
||||
npmCommand: [process.execPath, "-e", "process.exit(1)"],
|
||||
}),
|
||||
});
|
||||
|
||||
await loader.reload();
|
||||
|
||||
const warnings = loader.getExtensions().warnings ?? [];
|
||||
expect(warnings).toHaveLength(1);
|
||||
expect(warnings[0].path).toBe("npm:broken-package");
|
||||
expect(warnings[0].warning).toContain("Check the source and try again.");
|
||||
expect(warnings[0].warning).not.toContain("pi remove");
|
||||
});
|
||||
|
||||
// #11088
|
||||
it("should point project extension packages at pi remove --local", async () => {
|
||||
vi.stubEnv("PI_OFFLINE", "");
|
||||
writeFileSync(
|
||||
join(agentDir, "settings.json"),
|
||||
JSON.stringify({ npmCommand: [process.execPath, "-e", "process.exit(1)"] }),
|
||||
);
|
||||
mkdirSync(join(cwd, ".pi"), { recursive: true });
|
||||
writeFileSync(join(cwd, ".pi", "settings.json"), JSON.stringify({ packages: ["npm:broken-package"] }));
|
||||
const loader = new DefaultResourceLoader({
|
||||
cwd,
|
||||
agentDir,
|
||||
settingsManager: SettingsManager.create(cwd, agentDir, { projectTrusted: true }),
|
||||
});
|
||||
|
||||
await loader.reload();
|
||||
|
||||
const warnings = loader.getExtensions().warnings ?? [];
|
||||
expect(warnings).toHaveLength(1);
|
||||
expect(warnings[0].warning).toContain('Run "pi remove npm:broken-package --local"');
|
||||
});
|
||||
|
||||
// #11088
|
||||
it("should warn once about a failing install when project trust is resolved", async () => {
|
||||
vi.stubEnv("PI_OFFLINE", "");
|
||||
const loader = new DefaultResourceLoader({
|
||||
cwd,
|
||||
agentDir,
|
||||
settingsManager: SettingsManager.inMemory({
|
||||
packages: ["npm:broken-package"],
|
||||
npmCommand: [process.execPath, "-e", "process.exit(1)"],
|
||||
}),
|
||||
});
|
||||
|
||||
await loader.reload({ resolveProjectTrust: async () => false });
|
||||
|
||||
const warnings = loader.getExtensions().warnings ?? [];
|
||||
expect(warnings).toHaveLength(1);
|
||||
expect(warnings[0].path).toBe("npm:broken-package");
|
||||
});
|
||||
|
||||
it("should fail when an extension package manifest cannot be parsed", async () => {
|
||||
const packageRoot = join(tempDir, "invalid-extension-package");
|
||||
const extensionsDir = join(packageRoot, "extensions");
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue