From 37f160cab6ea4605617f545fb09d4e790afe63b4 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 3 Jun 2026 14:25:07 +0200 Subject: [PATCH] refactor(coding-agent): simplify project trust changes --- .../coding-agent/src/core/package-manager.ts | 2 - .../coding-agent/src/core/settings-manager.ts | 106 +++++++----------- .../src/modes/interactive/interactive-mode.ts | 13 ++- packages/coding-agent/test/args.test.ts | 12 -- .../test/package-command-paths.test.ts | 56 +-------- .../coding-agent/test/resource-loader.test.ts | 16 +-- .../coding-agent/test/trust-manager.test.ts | 24 ---- 7 files changed, 52 insertions(+), 177 deletions(-) diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 4a1e04785..7efd32285 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -822,7 +822,6 @@ export class DefaultPackageManager implements PackageManager { } getInstalledPath(source: string, scope: "user" | "project"): string | undefined { - this.assertProjectPiTrustedForScope(scope); const parsed = this.parseSource(source); if (parsed.type === "npm") { const path = this.getNpmInstallPath(parsed, scope); @@ -1293,7 +1292,6 @@ export class DefaultPackageManager implements PackageManager { } private async installParsedSource(parsed: ParsedSource, scope: SourceScope): Promise { - this.assertProjectPiTrustedForScope(scope); if (parsed.type === "npm") { await this.installNpm(parsed, scope, scope === "temporary"); return; diff --git a/packages/coding-agent/src/core/settings-manager.ts b/packages/coding-agent/src/core/settings-manager.ts index 7315a41f6..4f590a58f 100644 --- a/packages/coding-agent/src/core/settings-manager.ts +++ b/packages/coding-agent/src/core/settings-manager.ts @@ -165,8 +165,6 @@ export interface SettingsManagerCreateOptions { export interface SettingsStorage { withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void; - setProjectConfigTrusted?(trusted: boolean): void; - isProjectConfigTrusted?(): boolean; } export interface SettingsError { @@ -177,22 +175,12 @@ export interface SettingsError { export class FileSettingsStorage implements SettingsStorage { private globalSettingsPath: string; private projectSettingsPath: string; - private projectConfigTrusted: boolean; - constructor(cwd: string, agentDir: string, options: SettingsManagerCreateOptions = {}) { + constructor(cwd: string, agentDir: string) { const resolvedCwd = resolvePath(cwd); const resolvedAgentDir = resolvePath(agentDir); this.globalSettingsPath = join(resolvedAgentDir, "settings.json"); this.projectSettingsPath = join(resolvedCwd, CONFIG_DIR_NAME, "settings.json"); - this.projectConfigTrusted = options.projectConfigTrusted ?? true; - } - - setProjectConfigTrusted(trusted: boolean): void { - this.projectConfigTrusted = trusted; - } - - isProjectConfigTrusted(): boolean { - return this.projectConfigTrusted; } private acquireLockSyncWithRetry(path: string): () => void { @@ -223,10 +211,6 @@ export class FileSettingsStorage implements SettingsStorage { } withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void { - if (scope === "project" && !this.projectConfigTrusted) { - throw new Error("Project .pi is not trusted; refusing to access project settings"); - } - const path = scope === "global" ? this.globalSettingsPath : this.projectSettingsPath; const dir = dirname(path); @@ -260,21 +244,8 @@ export class FileSettingsStorage implements SettingsStorage { export class InMemorySettingsStorage implements SettingsStorage { private global: string | undefined; private project: string | undefined; - private projectConfigTrusted = true; - - setProjectConfigTrusted(trusted: boolean): void { - this.projectConfigTrusted = trusted; - } - - isProjectConfigTrusted(): boolean { - return this.projectConfigTrusted; - } withLock(scope: SettingsScope, fn: (current: string | undefined) => string | undefined): void { - if (scope === "project" && !this.projectConfigTrusted) { - throw new Error("Project .pi is not trusted; refusing to access project settings"); - } - const current = scope === "global" ? this.global : this.project; const next = fn(current); if (next !== undefined) { @@ -327,15 +298,15 @@ export class SettingsManager { agentDir: string = getAgentDir(), options: SettingsManagerCreateOptions = {}, ): SettingsManager { - const storage = new FileSettingsStorage(cwd, agentDir, options); - return SettingsManager.fromStorage(storage); + const storage = new FileSettingsStorage(cwd, agentDir); + return SettingsManager.fromStorage(storage, options); } /** Create a SettingsManager from an arbitrary storage backend */ - static fromStorage(storage: SettingsStorage): SettingsManager { - const projectConfigTrusted = storage.isProjectConfigTrusted?.() ?? true; + static fromStorage(storage: SettingsStorage, options: SettingsManagerCreateOptions = {}): SettingsManager { + const projectConfigTrusted = options.projectConfigTrusted ?? true; const globalLoad = SettingsManager.tryLoadFromStorage(storage, "global"); - const projectLoad = SettingsManager.tryLoadFromStorage(storage, "project"); + const projectLoad = SettingsManager.tryLoadFromStorage(storage, "project", projectConfigTrusted); const initialErrors: SettingsError[] = []; if (globalLoad.error) { initialErrors.push({ scope: "global", error: globalLoad.error }); @@ -363,8 +334,12 @@ export class SettingsManager { return SettingsManager.fromStorage(storage); } - private static loadFromStorage(storage: SettingsStorage, scope: SettingsScope): Settings { - if (scope === "project" && storage.isProjectConfigTrusted?.() === false) { + private static loadFromStorage( + storage: SettingsStorage, + scope: SettingsScope, + projectConfigTrusted = true, + ): Settings { + if (scope === "project" && !projectConfigTrusted) { return {}; } @@ -384,9 +359,10 @@ export class SettingsManager { private static tryLoadFromStorage( storage: SettingsStorage, scope: SettingsScope, + projectConfigTrusted = true, ): { settings: Settings; error: Error | null } { try { - return { settings: SettingsManager.loadFromStorage(storage, scope), error: null }; + return { settings: SettingsManager.loadFromStorage(storage, scope, projectConfigTrusted), error: null }; } catch (error) { return { settings: {}, error: error as Error }; } @@ -468,7 +444,6 @@ export class SettingsManager { setProjectConfigTrusted(trusted: boolean): void { this.projectConfigTrusted = trusted; - this.storage.setProjectConfigTrusted?.(trusted); if (!trusted) { this.projectSettings = {}; this.projectSettingsLoadError = null; @@ -494,7 +469,7 @@ export class SettingsManager { this.modifiedProjectFields.clear(); this.modifiedProjectNestedFields.clear(); - const projectLoad = SettingsManager.tryLoadFromStorage(this.storage, "project"); + const projectLoad = SettingsManager.tryLoadFromStorage(this.storage, "project", this.projectConfigTrusted); if (!projectLoad.error) { this.projectSettings = projectLoad.settings; this.projectSettingsLoadError = null; @@ -558,6 +533,9 @@ export class SettingsManager { private enqueueWrite(scope: SettingsScope, task: () => void): void { this.writeQueue = this.writeQueue .then(() => { + if (scope === "project") { + this.assertProjectConfigTrustedForWrite(); + } task(); this.clearModifiedScope(scope); }) @@ -638,6 +616,14 @@ export class SettingsManager { }); } + private updateProjectSettings(field: keyof Settings, update: (settings: Settings) => void): void { + this.assertProjectConfigTrustedForWrite(); + const projectSettings = structuredClone(this.projectSettings); + update(projectSettings); + this.markProjectModified(field); + this.saveProjectSettings(projectSettings); + } + async flush(): Promise { await this.writeQueue; } @@ -908,11 +894,9 @@ export class SettingsManager { } setProjectPackages(packages: PackageSource[]): void { - this.assertProjectConfigTrustedForWrite(); - const projectSettings = structuredClone(this.projectSettings); - projectSettings.packages = packages; - this.markProjectModified("packages"); - this.saveProjectSettings(projectSettings); + this.updateProjectSettings("packages", (settings) => { + settings.packages = packages; + }); } getExtensionPaths(): string[] { @@ -926,11 +910,9 @@ export class SettingsManager { } setProjectExtensionPaths(paths: string[]): void { - this.assertProjectConfigTrustedForWrite(); - const projectSettings = structuredClone(this.projectSettings); - projectSettings.extensions = paths; - this.markProjectModified("extensions"); - this.saveProjectSettings(projectSettings); + this.updateProjectSettings("extensions", (settings) => { + settings.extensions = paths; + }); } getSkillPaths(): string[] { @@ -944,11 +926,9 @@ export class SettingsManager { } setProjectSkillPaths(paths: string[]): void { - this.assertProjectConfigTrustedForWrite(); - const projectSettings = structuredClone(this.projectSettings); - projectSettings.skills = paths; - this.markProjectModified("skills"); - this.saveProjectSettings(projectSettings); + this.updateProjectSettings("skills", (settings) => { + settings.skills = paths; + }); } getPromptTemplatePaths(): string[] { @@ -962,11 +942,9 @@ export class SettingsManager { } setProjectPromptTemplatePaths(paths: string[]): void { - this.assertProjectConfigTrustedForWrite(); - const projectSettings = structuredClone(this.projectSettings); - projectSettings.prompts = paths; - this.markProjectModified("prompts"); - this.saveProjectSettings(projectSettings); + this.updateProjectSettings("prompts", (settings) => { + settings.prompts = paths; + }); } getThemePaths(): string[] { @@ -980,11 +958,9 @@ export class SettingsManager { } setProjectThemePaths(paths: string[]): void { - this.assertProjectConfigTrustedForWrite(); - const projectSettings = structuredClone(this.projectSettings); - projectSettings.themes = paths; - this.markProjectModified("themes"); - this.saveProjectSettings(projectSettings); + this.updateProjectSettings("themes", (settings) => { + settings.themes = paths; + }); } getEnableSkillCommands(): boolean { diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 65918f3d7..dcd67074e 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -4947,9 +4947,10 @@ export class InteractiveMode { return "ask"; } - private async selectTrustDecision(): Promise { - const trustStore = new ProjectTrustStore(getAgentDir()); - const cwd = this.sessionManager.getCwd(); + private async selectTrustDecision( + trustStore: ProjectTrustStore, + cwd: string, + ): Promise { const current = this.formatTrustDecision(trustStore.get(cwd)); const choice = await this.showExtensionSelector( `Trust project .pi directory?\nCurrent setting: ${current}\nLoad .pi from ${cwd}?\nWarning: Project extensions can execute code.`, @@ -4977,10 +4978,12 @@ export class InteractiveMode { return; } + const trustStore = new ProjectTrustStore(getAgentDir()); + const cwd = this.sessionManager.getCwd(); const rawArg = text === "/trust" ? "" : text.slice("/trust".length).trim().toLowerCase(); let decision: ProjectTrustDecision | undefined; if (!rawArg) { - decision = await this.selectTrustDecision(); + decision = await this.selectTrustDecision(trustStore, cwd); } else if (rawArg === "yes") { decision = true; } else if (rawArg === "no") { @@ -4996,8 +4999,6 @@ export class InteractiveMode { return; } - const trustStore = new ProjectTrustStore(getAgentDir()); - const cwd = this.sessionManager.getCwd(); trustStore.set(cwd, decision); const projectPiTrusted = this.options.forceProjectPiTrust === true || decision === true; this.settingsManager.setProjectConfigTrusted(projectPiTrusted); diff --git a/packages/coding-agent/test/args.test.ts b/packages/coding-agent/test/args.test.ts index c58a09f4c..78fd832c8 100644 --- a/packages/coding-agent/test/args.test.ts +++ b/packages/coding-agent/test/args.test.ts @@ -300,18 +300,6 @@ describe("parseArgs", () => { }); }); - describe("--force flag", () => { - test("parses --force flag", () => { - const result = parseArgs(["--force"]); - expect(result.force).toBe(true); - }); - - test("parses -f shorthand", () => { - const result = parseArgs(["-f"]); - expect(result.force).toBe(true); - }); - }); - describe("--offline flag", () => { test("parses --offline flag", () => { const result = parseArgs(["--offline"]); diff --git a/packages/coding-agent/test/package-command-paths.test.ts b/packages/coding-agent/test/package-command-paths.test.ts index 723512738..68605bdac 100644 --- a/packages/coding-agent/test/package-command-paths.test.ts +++ b/packages/coding-agent/test/package-command-paths.test.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -214,60 +214,6 @@ describe("package commands", () => { } }); - it("does not treat update --force as a project trust override", async () => { - const globalPrefix = join(tempDir, "global-prefix"); - const selfPackageDir = join(globalPrefix, "lib", "node_modules", "@earendil-works", "pi-coding-agent"); - const fakeNpmPath = join(tempDir, "fake-npm-record.cjs"); - const recordPath = join(tempDir, "update-force-records.json"); - mkdirSync(selfPackageDir, { recursive: true }); - mkdirSync(join(projectDir, ".pi"), { recursive: true }); - writeFileSync( - fakeNpmPath, - `const fs=require("node:fs"),path=require("node:path"),args=process.argv.slice(2),prefix=args[args.indexOf("--prefix")+1]; -if(args.includes("root")) { - console.log(path.join(prefix,"lib","node_modules")); - process.exit(0); -} -const records=fs.existsSync(${JSON.stringify(recordPath)})?JSON.parse(fs.readFileSync(${JSON.stringify(recordPath)},"utf-8")):[]; -records.push(args); -fs.writeFileSync(${JSON.stringify(recordPath)},JSON.stringify(records)); -`, - ); - writeFileSync( - join(agentDir, "settings.json"), - JSON.stringify({ npmCommand: [originalExecPath, fakeNpmPath, "--prefix", globalPrefix] }, null, 2), - ); - writeFileSync( - join(projectDir, ".pi", "settings.json"), - JSON.stringify({ packages: ["npm:@project/pkg"] }, null, 2), - ); - process.env.PI_PACKAGE_DIR = selfPackageDir; - Object.defineProperty(process, "execPath", { - value: join(selfPackageDir, "dist", "cli.js"), - configurable: true, - }); - const fetchMock = vi.fn(); - vi.stubGlobal("fetch", fetchMock); - - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - - try { - await expect(main(["update", "--force"])).resolves.toBeUndefined(); - - expect(process.exitCode).toBeUndefined(); - expect(errorSpy).not.toHaveBeenCalled(); - expect(fetchMock).not.toHaveBeenCalled(); - expect(existsSync(recordPath)).toBe(true); - const recordedCalls = JSON.parse(readFileSync(recordPath, "utf-8")) as string[][]; - expect(recordedCalls.some((args) => args.some((arg) => arg.includes("@project/pkg")))).toBe(false); - expect(recordedCalls).toEqual([expect.arrayContaining(["install", "-g", PACKAGE_NAME])]); - } finally { - logSpy.mockRestore(); - errorSpy.mockRestore(); - } - }); - it("uses global npmCommand and current package name for forced self updates without checking the api", async () => { const globalPrefix = join(tempDir, "global-prefix"); const projectPrefix = join(tempDir, "project-prefix"); diff --git a/packages/coding-agent/test/resource-loader.test.ts b/packages/coding-agent/test/resource-loader.test.ts index 4a0894e3a..0a7ac6994 100644 --- a/packages/coding-agent/test/resource-loader.test.ts +++ b/packages/coding-agent/test/resource-loader.test.ts @@ -329,19 +329,6 @@ Content`, expect(loader.getSystemPrompt()).toBe("You are a helpful assistant."); }); - it("should skip .pi SYSTEM.md when project .pi is not trusted", async () => { - const piDir = join(cwd, ".pi"); - mkdirSync(piDir, { recursive: true }); - writeFileSync(join(piDir, "SYSTEM.md"), "Project system prompt."); - writeFileSync(join(agentDir, "SYSTEM.md"), "Global system prompt."); - const settingsManager = SettingsManager.create(cwd, agentDir, { projectConfigTrusted: false }); - - const loader = new DefaultResourceLoader({ cwd, agentDir, settingsManager }); - await loader.reload(); - - expect(loader.getSystemPrompt()).toBe("Global system prompt."); - }); - it("should skip .pi resources when project .pi is not trusted", async () => { const piDir = join(cwd, ".pi"); const extensionsDir = join(piDir, "extensions"); @@ -352,6 +339,8 @@ Content`, mkdirSync(skillDir, { recursive: true }); mkdirSync(promptsDir, { recursive: true }); mkdirSync(themesDir, { recursive: true }); + writeFileSync(join(piDir, "SYSTEM.md"), "Project system prompt."); + writeFileSync(join(agentDir, "SYSTEM.md"), "Global system prompt."); writeFileSync(join(extensionsDir, "project.ts"), `throw new Error("should not load");`); writeFileSync( join(skillDir, "SKILL.md"), @@ -372,6 +361,7 @@ Project skill content`, const loader = new DefaultResourceLoader({ cwd, agentDir, settingsManager }); await loader.reload(); + expect(loader.getSystemPrompt()).toBe("Global system prompt."); expect(loader.getExtensions().extensions).toHaveLength(0); expect(loader.getExtensions().errors).toEqual([]); expect(loader.getSkills().skills.some((skill) => skill.name === "project-skill")).toBe(false); diff --git a/packages/coding-agent/test/trust-manager.test.ts b/packages/coding-agent/test/trust-manager.test.ts index 06c82edbf..e4abfb62f 100644 --- a/packages/coding-agent/test/trust-manager.test.ts +++ b/packages/coding-agent/test/trust-manager.test.ts @@ -1,7 +1,6 @@ import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import lockfile from "proper-lockfile"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { hasProjectPiDirectory, ProjectTrustStore } from "../src/core/trust-manager.ts"; @@ -44,29 +43,6 @@ describe("ProjectTrustStore", () => { expect(readFileSync(trustPath, "utf-8")).toBe("{not json"); }); - it("does not read trust.json while another process holds the trust lock", () => { - const trustPath = join(agentDir, "trust.json"); - writeFileSync(trustPath, "{partial", "utf-8"); - const release = lockfile.lockSync(agentDir, { realpath: false, lockfilePath: `${trustPath}.lock` }); - const store = new ProjectTrustStore(agentDir); - - try { - let error: unknown; - try { - store.get(cwd); - } catch (caught) { - error = caught; - } - - expect(error).toBeInstanceOf(Error); - expect((error as { code?: unknown }).code).toBe("ELOCKED"); - } finally { - release(); - } - - expect(() => store.get(cwd)).toThrow(/Failed to read trust store/); - }); - it("detects project .pi directories", () => { expect(hasProjectPiDirectory(cwd)).toBe(false);