From 786c25e6c7389018a3394db63aff4416261a2b45 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 14 May 2026 05:35:12 +0200 Subject: [PATCH] fix(coding-agent): resolved coding-agent cli --help startup init cycle - Fixed root `cli --help` startup by preventing the config/model-registry initialization cycle. - Extracted config validation, migration, and loading logic from config.ts into config/config-file.ts. - Added ConfigFile helpers for migration, validated JSON/JSONC/YAML loading, status caching, and reset. - Added a regression test that runs `cli.ts --help` with temp HOME/XDG env paths and expects exit code 0. --- packages/coding-agent/CHANGELOG.md | 2 + packages/coding-agent/src/config.ts | 222 +----------------- .../coding-agent/src/config/config-file.ts | 210 +++++++++++++++++ .../coding-agent/src/config/model-registry.ts | 2 +- .../test/agent-session-silent-abort.test.ts | 12 +- .../test/cli-help-load-order.test.ts | 68 ++++++ .../test/input-controller-skill-queue.test.ts | 24 +- 7 files changed, 306 insertions(+), 234 deletions(-) create mode 100644 packages/coding-agent/src/config/config-file.ts create mode 100644 packages/coding-agent/test/cli-help-load-order.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 51ffce1f5..7137ef26c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## [Unreleased] + ### Breaking Changes - Removed the dedicated `exit_plan_mode` tool and its prompt, requiring plan-mode completion to use the existing `resolve` tool path instead @@ -29,6 +30,7 @@ ### Fixed +- Fixed `--help` startup to avoid a config/model-registry load cycle so the root CLI help command now exits successfully in a clean environment - Queued `/skill: [args]` invocations now show as compact `Steer: /skill: [args]` / `Follow-up: /skill: [args]` chips in the pending-messages bar and disappear when the agent consumes the queued message (parity with plain-text steer/follow-up). Previously the queued skill was invisible while queued and rendered as a full skill block at consumption with no chip ever appearing. - Plan-mode "Approve and compact context" no longer surfaces a red "Operation aborted" line on the plan-mode assistant message; the silent transition into compaction now renders cleanly on both live and replay paths. Real user-cancel aborts on unrelated turns and the existing "Compaction cancelled" path are unchanged. - Auto-recover conflict-resolution `write`/`read` paths that the agent malformed as `:conflict://` (or `:conflict://*`) by mixing the `:conflicts` read selector with the `conflict://` scheme. The stripped `:` prefix is stored on `ParsedConflictUri.recoveredPrefix` and, for writes, surfaces as a trailing note in the result text so the agent learns the correct shape. Clean `conflict://…` URIs are unchanged. diff --git a/packages/coding-agent/src/config.ts b/packages/coding-agent/src/config.ts index 488928f5d..0637cc817 100644 --- a/packages/coding-agent/src/config.ts +++ b/packages/coding-agent/src/config.ts @@ -1,20 +1,11 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; -import { - CONFIG_DIR_NAME, - getAgentDir, - getConfigAgentDirName, - getProjectDir, - isEnoent, - logger, -} from "@oh-my-pi/pi-utils"; -import type { TSchema } from "@sinclair/typebox"; -import { Value } from "@sinclair/typebox/value"; -import type { ErrorObject } from "ajv"; -import { JSONC, YAML } from "bun"; +import { CONFIG_DIR_NAME, getConfigAgentDirName, getProjectDir } from "@oh-my-pi/pi-utils"; import { expandTilde } from "./tools/path-utils"; +export * from "./config/config-file"; + const priorityList = [ { dir: CONFIG_DIR_NAME, globalAgentDir: getConfigAgentDirName }, { dir: ".claude" }, @@ -53,213 +44,6 @@ export function getChangelogPath(): string { return path.resolve(path.join(getPackageDir(), "CHANGELOG.md")); } -// ============================================================================= -// User Config Paths (~/.omp/agent/*) -// ============================================================================= - -function migrateJsonToYml(jsonPath: string, ymlPath: string) { - try { - if (fs.existsSync(ymlPath)) return; - if (!fs.existsSync(jsonPath)) return; - - const content = fs.readFileSync(jsonPath, "utf-8"); - const parsed = JSON.parse(content); - if (!parsed) { - logger.warn("migrateJsonToYml: invalid json structure", { path: jsonPath }); - return; - } - fs.writeFileSync(ymlPath, YAML.stringify(parsed, null, 2)); - } catch (error) { - logger.warn("migrateJsonToYml: migration failed", { error: String(error) }); - } -} - -export interface IConfigFile { - readonly id: string; - readonly schema: TSchema; - path?(): string; - load(): T | null; - invalidate?(): void; -} - -export class ConfigError extends Error { - readonly #message: string; - constructor( - public readonly id: string, - public readonly schemaErrors: ErrorObject[] | null | undefined, - public readonly other?: { err: unknown; stage: string }, - ) { - let messages: string[] | undefined; - let cause: any | undefined; - let klass: string; - - if (schemaErrors) { - klass = "Schema"; - messages = schemaErrors.map(e => `${e.instancePath || "root"}: ${e.message}`); - } else if (other) { - klass = other.stage; - if (other.err instanceof Error) { - messages = [other.err.message]; - cause = other.err; - } else { - messages = [String(other.err)]; - } - } else { - klass = "Unknown"; - } - - const title = `Failed to load config file ${id}, ${klass} error:`; - let message: string; - switch (messages?.length ?? 0) { - case 0: - message = title.slice(0, -1); - break; - case 1: - message = `${title} ${messages![0]}`; - break; - default: - message = `${title}\n${messages!.map(m => ` - ${m}`).join("\n")}`; - break; - } - - super(message, { cause }); - this.name = "LoadError"; - this.#message = message; - } - - get message(): string { - return this.#message; - } - - toString(): string { - return this.message; - } -} - -export type LoadStatus = "ok" | "error" | "not-found"; - -export type LoadResult = - | { value?: null; error: ConfigError; status: "error" } - | { value: T; error?: undefined; status: "ok" } - | { value?: null; error?: unknown; status: "not-found" }; - -export class ConfigFile implements IConfigFile { - readonly #basePath: string; - #cache?: LoadResult; - #auxValidate?: (value: T) => void; - - constructor( - readonly id: string, - readonly schema: TSchema, - configPath: string = path.join(getAgentDir(), `${id}.yml`), - ) { - this.#basePath = configPath; - if (configPath.endsWith(".yml")) { - const jsonPath = `${configPath.slice(0, -4)}.json`; - migrateJsonToYml(jsonPath, configPath); - } else if (configPath.endsWith(".yaml")) { - const jsonPath = `${configPath.slice(0, -5)}.json`; - migrateJsonToYml(jsonPath, configPath); - } else if (configPath.endsWith(".json") || configPath.endsWith(".jsonc")) { - // JSON configs are still supported without migration. - } else { - throw new Error(`Invalid config file path: ${configPath}`); - } - } - - relocate(path?: string): ConfigFile { - if (!path || path === this.#basePath) return this; - const result = new ConfigFile(this.id, this.schema, path); - result.#auxValidate = this.#auxValidate; - return result; - } - - getMtimeMs(): number | null { - try { - return fs.statSync(this.path()).mtimeMs; - } catch (err) { - if (isEnoent(err)) return null; - throw err; - } - } - - withValidation(name: string, validate: (value: T) => void): this { - const prev = this.#auxValidate; - this.#auxValidate = (value: T) => { - prev?.(value); - try { - validate(value); - } catch (error) { - throw new ConfigError(this.id, undefined, { err: error, stage: `Validate(${name})` }); - } - }; - return this; - } - - createDefault() { - return Value.Default(this.schema, [], undefined) as T; - } - - #storeCache(result: LoadResult): LoadResult { - this.#cache = result; - return result; - } - - tryLoad(): LoadResult { - if (this.#cache) return this.#cache; - - try { - const content = fs.readFileSync(this.path(), "utf-8").trim(); - - let parsed: unknown; - if (this.#basePath.endsWith(".json") || this.#basePath.endsWith(".jsonc")) { - parsed = JSONC.parse(content); - } else if (this.#basePath.endsWith(".yml") || this.#basePath.endsWith(".yaml")) { - parsed = YAML.parse(content); - } else { - throw new Error(`Invalid config file path: ${this.#basePath}`); - } - - if (!Value.Check(this.schema, parsed)) { - const schemaErrors: ErrorObject[] = []; - for (const err of Value.Errors(this.schema, parsed)) { - schemaErrors.push({ instancePath: err.path, message: err.message } as ErrorObject); - if (schemaErrors.length >= 50) break; - } - const error = new ConfigError(this.id, schemaErrors); - logger.warn("Failed to parse config file", { path: this.path(), error }); - return this.#storeCache({ error, status: "error" }); - } - return this.#storeCache({ value: parsed as T, status: "ok" }); - } catch (error) { - if (isEnoent(error)) { - return this.#storeCache({ status: "not-found" }); - } - logger.warn("Failed to parse config file", { path: this.path(), error }); - return this.#storeCache({ - error: new ConfigError(this.id, undefined, { err: error, stage: "Unexpected" }), - status: "error", - }); - } - } - - load(): T | null { - return this.tryLoad().value ?? null; - } - - loadOrDefault(): T { - return this.tryLoad().value ?? this.createDefault(); - } - - path(): string { - return this.#basePath; - } - - invalidate() { - this.#cache = undefined; - } -} - // ============================================================================= // Multi-Config Directory Helpers // ============================================================================= diff --git a/packages/coding-agent/src/config/config-file.ts b/packages/coding-agent/src/config/config-file.ts new file mode 100644 index 000000000..d9b6ba411 --- /dev/null +++ b/packages/coding-agent/src/config/config-file.ts @@ -0,0 +1,210 @@ +import * as fs from "node:fs"; +import * as path from "node:path"; +import { getAgentDir, isEnoent, logger } from "@oh-my-pi/pi-utils"; +import type { TSchema } from "@sinclair/typebox"; +import { Value } from "@sinclair/typebox/value"; +import type { ErrorObject } from "ajv"; +import { JSONC, YAML } from "bun"; + +function migrateJsonToYml(jsonPath: string, ymlPath: string) { + try { + if (fs.existsSync(ymlPath)) return; + if (!fs.existsSync(jsonPath)) return; + + const content = fs.readFileSync(jsonPath, "utf-8"); + const parsed = JSON.parse(content); + if (!parsed) { + logger.warn("migrateJsonToYml: invalid json structure", { path: jsonPath }); + return; + } + fs.writeFileSync(ymlPath, YAML.stringify(parsed, null, 2)); + } catch (error) { + logger.warn("migrateJsonToYml: migration failed", { error: String(error) }); + } +} + +export interface IConfigFile { + readonly id: string; + readonly schema: TSchema; + path?(): string; + load(): T | null; + invalidate?(): void; +} + +export class ConfigError extends Error { + readonly #message: string; + constructor( + public readonly id: string, + public readonly schemaErrors: ErrorObject[] | null | undefined, + public readonly other?: { err: unknown; stage: string }, + ) { + let messages: string[] | undefined; + let cause: Error | undefined; + let klass: string; + + if (schemaErrors) { + klass = "Schema"; + messages = schemaErrors.map(e => `${e.instancePath || "root"}: ${e.message}`); + } else if (other) { + klass = other.stage; + if (other.err instanceof Error) { + messages = [other.err.message]; + cause = other.err; + } else { + messages = [String(other.err)]; + } + } else { + klass = "Unknown"; + } + + const title = `Failed to load config file ${id}, ${klass} error:`; + let message: string; + switch (messages?.length ?? 0) { + case 0: + message = title.slice(0, -1); + break; + case 1: + message = `${title} ${messages![0]}`; + break; + default: + message = `${title}\n${messages!.map(m => ` - ${m}`).join("\n")}`; + break; + } + + super(message, { cause }); + this.name = "LoadError"; + this.#message = message; + } + + get message(): string { + return this.#message; + } + + toString(): string { + return this.message; + } +} + +export type LoadStatus = "ok" | "error" | "not-found"; + +export type LoadResult = + | { value?: null; error: ConfigError; status: "error" } + | { value: T; error?: undefined; status: "ok" } + | { value?: null; error?: unknown; status: "not-found" }; + +export class ConfigFile implements IConfigFile { + readonly #basePath: string; + #cache?: LoadResult; + #auxValidate?: (value: T) => void; + + constructor( + readonly id: string, + readonly schema: TSchema, + configPath: string = path.join(getAgentDir(), `${id}.yml`), + ) { + this.#basePath = configPath; + if (configPath.endsWith(".yml")) { + const jsonPath = `${configPath.slice(0, -4)}.json`; + migrateJsonToYml(jsonPath, configPath); + } else if (configPath.endsWith(".yaml")) { + const jsonPath = `${configPath.slice(0, -5)}.json`; + migrateJsonToYml(jsonPath, configPath); + } else if (configPath.endsWith(".json") || configPath.endsWith(".jsonc")) { + // JSON configs are still supported without migration. + } else { + throw new Error(`Invalid config file path: ${configPath}`); + } + } + + relocate(configPath?: string): ConfigFile { + if (!configPath || configPath === this.#basePath) return this; + const result = new ConfigFile(this.id, this.schema, configPath); + result.#auxValidate = this.#auxValidate; + return result; + } + + getMtimeMs(): number | null { + try { + return fs.statSync(this.path()).mtimeMs; + } catch (err) { + if (isEnoent(err)) return null; + throw err; + } + } + + withValidation(name: string, validate: (value: T) => void): this { + const prev = this.#auxValidate; + this.#auxValidate = (value: T) => { + prev?.(value); + try { + validate(value); + } catch (error) { + throw new ConfigError(this.id, undefined, { err: error, stage: `Validate(${name})` }); + } + }; + return this; + } + + createDefault(): T { + return Value.Default(this.schema, [], undefined) as T; + } + + #storeCache(result: LoadResult): LoadResult { + this.#cache = result; + return result; + } + + tryLoad(): LoadResult { + if (this.#cache) return this.#cache; + + try { + const content = fs.readFileSync(this.path(), "utf-8").trim(); + + let parsed: unknown; + if (this.#basePath.endsWith(".json") || this.#basePath.endsWith(".jsonc")) { + parsed = JSONC.parse(content); + } else if (this.#basePath.endsWith(".yml") || this.#basePath.endsWith(".yaml")) { + parsed = YAML.parse(content); + } else { + throw new Error(`Invalid config file path: ${this.#basePath}`); + } + + if (!Value.Check(this.schema, parsed)) { + const schemaErrors: ErrorObject[] = []; + for (const err of Value.Errors(this.schema, parsed)) { + schemaErrors.push({ instancePath: err.path, message: err.message } as ErrorObject); + if (schemaErrors.length >= 50) break; + } + const error = new ConfigError(this.id, schemaErrors); + logger.warn("Failed to parse config file", { path: this.path(), error }); + return this.#storeCache({ error, status: "error" }); + } + return this.#storeCache({ value: parsed as T, status: "ok" }); + } catch (error) { + if (isEnoent(error)) { + return this.#storeCache({ status: "not-found" }); + } + logger.warn("Failed to parse config file", { path: this.path(), error }); + return this.#storeCache({ + error: new ConfigError(this.id, undefined, { err: error, stage: "Unexpected" }), + status: "error", + }); + } + } + + load(): T | null { + return this.tryLoad().value ?? null; + } + + loadOrDefault(): T { + return this.tryLoad().value ?? this.createDefault(); + } + + path(): string { + return this.#basePath; + } + + invalidate() { + this.#cache = undefined; + } +} diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index 9cffc53f3..bdcf1ff97 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -31,10 +31,10 @@ import { registerOAuthProvider, unregisterOAuthProviders } from "@oh-my-pi/pi-ai import type { OAuthCredentials, OAuthLoginCallbacks } from "@oh-my-pi/pi-ai/utils/oauth/types"; import { isRecord, logger } from "@oh-my-pi/pi-utils"; import { type Static, Type } from "@sinclair/typebox"; -import { type ConfigError, ConfigFile } from "../config"; import { parseModelString, resolveProviderModelReference } from "../config/model-resolver"; import { isValidThemeColor, type ThemeColor } from "../modes/theme/theme"; import type { AuthStorage, OAuthCredential } from "../session/auth-storage"; +import { type ConfigError, ConfigFile } from "./config-file"; import { buildCanonicalModelIndex, type CanonicalModelIndex, diff --git a/packages/coding-agent/test/agent-session-silent-abort.test.ts b/packages/coding-agent/test/agent-session-silent-abort.test.ts index 6aa433fa6..6e38d0994 100644 --- a/packages/coding-agent/test/agent-session-silent-abort.test.ts +++ b/packages/coding-agent/test/agent-session-silent-abort.test.ts @@ -203,12 +203,14 @@ describe("AgentSession silent-abort marker stamping", () => { // The emitted display event ALSO carries the marker because the spread copy // happened AFTER the stamp. - const emitted = seen.find(e => e.type === "message_end") as - | Extract - | undefined; + const emitted = seen.find( + (event): event is Extract => event.type === "message_end", + ); expect(emitted).toBeDefined(); - // biome-ignore lint/style/noNonNullAssertion: just asserted defined above - const emittedMessage = emitted!.message; + if (!emitted) { + throw new Error("expected a message_end event to be emitted"); + } + const emittedMessage = emitted.message; // `message_end` events are typed against AgentMessage (union over // custom/exec/etc. roles too); narrow by asserting `role` so the // `errorMessage` / `content` accesses below type-check. diff --git a/packages/coding-agent/test/cli-help-load-order.test.ts b/packages/coding-agent/test/cli-help-load-order.test.ts new file mode 100644 index 000000000..5de9743bc --- /dev/null +++ b/packages/coding-agent/test/cli-help-load-order.test.ts @@ -0,0 +1,68 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; + +const repoRoot = path.resolve(import.meta.dir, "..", "..", ".."); +const cliEntry = path.join(repoRoot, "packages", "coding-agent", "src", "cli.ts"); + +let cleanupRoot: string | undefined; + +async function readStream(stream: ReadableStream): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + let text = ""; + try { + while (true) { + const { value, done } = await reader.read(); + if (done) break; + text += decoder.decode(value, { stream: true }); + } + return text + decoder.decode(); + } finally { + reader.releaseLock(); + } +} + +afterEach(async () => { + if (cleanupRoot) { + await fs.rm(cleanupRoot, { recursive: true, force: true }); + cleanupRoot = undefined; + } +}); + +describe("CLI help load order", () => { + it("loads the root help command without tripping config/model-registry cycles", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-help-load-order-")); + cleanupRoot = root; + const home = path.join(root, "home"); + const xdg = path.join(root, "xdg"); + const agentDir = path.join(root, "agent"); + await fs.mkdir(home, { recursive: true }); + await fs.mkdir(xdg, { recursive: true }); + await fs.mkdir(agentDir, { recursive: true }); + + const proc = Bun.spawn([process.execPath, cliEntry, "--help"], { + cwd: repoRoot, + stdout: "pipe", + stderr: "pipe", + env: { + ...process.env, + HOME: home, + XDG_CONFIG_HOME: xdg, + XDG_DATA_HOME: xdg, + PI_CODING_AGENT_DIR: agentDir, + PI_NO_TITLE: "1", + NO_COLOR: "1", + }, + }); + + const [, , exitCode] = await Promise.all([ + readStream(proc.stdout as ReadableStream), + readStream(proc.stderr as ReadableStream), + proc.exited, + ]); + + expect(exitCode).toBe(0); + }); +}); diff --git a/packages/coding-agent/test/input-controller-skill-queue.test.ts b/packages/coding-agent/test/input-controller-skill-queue.test.ts index 06029652c..11d24d13b 100644 --- a/packages/coding-agent/test/input-controller-skill-queue.test.ts +++ b/packages/coding-agent/test/input-controller-skill-queue.test.ts @@ -68,9 +68,9 @@ function createStubInputControllerContext(opts: { skillCommands: Map "sk-test-0"); - // Annotate parameters so `mock.calls[N]` is typed as a tuple (not `[]`) — - // avoids TS2352/TS2493 when casting `.calls[0]` to a destructured shape. - const promptCustomMessage = vi.fn(async (_message: { details?: SkillPromptDetails }, _options?: unknown) => {}); + // Annotate parameters so `mock.calls[N]` is typed as a tuple (not `[]`) and + // `message` carries required skill prompt details for assertion below. + const promptCustomMessage = vi.fn(async (_message: { details: SkillPromptDetails }, _options?: unknown) => {}); const updatePendingMessagesDisplay = vi.fn(); const requestRender = vi.fn(); const showError = vi.fn(); @@ -136,8 +136,10 @@ describe("InputController #invokeSkillCommand (E1-E3)", () => { expect(promptCustomMessage).toHaveBeenCalledTimes(1); const firstCall = promptCustomMessage.mock.calls[0]; expect(firstCall).toBeDefined(); - // biome-ignore lint/style/noNonNullAssertion: just asserted non-undefined - const messageArg = firstCall![0] as { details: SkillPromptDetails }; + if (!firstCall) { + throw new Error("expected promptCustomMessage to be called"); + } + const messageArg = firstCall[0]; expect(messageArg.details.__pendingDisplayTag).toBe("sk-test-0"); }); @@ -157,8 +159,10 @@ describe("InputController #invokeSkillCommand (E1-E3)", () => { const firstCall = promptCustomMessage.mock.calls[0]; expect(firstCall).toBeDefined(); - // biome-ignore lint/style/noNonNullAssertion: just asserted non-undefined - const messageArg = firstCall![0] as { details: SkillPromptDetails }; + if (!firstCall) { + throw new Error("expected promptCustomMessage to be called"); + } + const messageArg = firstCall[0]; expect(messageArg.details.__pendingDisplayTag).toBe("sk-test-0"); }); @@ -176,8 +180,10 @@ describe("InputController #invokeSkillCommand (E1-E3)", () => { expect(enqueueCustomMessageDisplay).not.toHaveBeenCalled(); const firstCall = promptCustomMessage.mock.calls[0]; expect(firstCall).toBeDefined(); - // biome-ignore lint/style/noNonNullAssertion: just asserted non-undefined - const messageArg = firstCall![0] as { details: SkillPromptDetails }; + if (!firstCall) { + throw new Error("expected promptCustomMessage to be called"); + } + const messageArg = firstCall[0]; expect(messageArg.details.__pendingDisplayTag).toBeUndefined(); }); });