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.
This commit is contained in:
can1357
2026-05-14 05:35:12 +02:00
parent ef5cbef51f
commit 786c25e6c7
7 changed files with 306 additions and 234 deletions
+2
View File
@@ -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:<name> [args]` invocations now show as compact `Steer: /skill:<name> [args]` / `Follow-up: /skill:<name> [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 `<file>:conflict://<N>` (or `<file>:conflict://*`) by mixing the `:conflicts` read selector with the `conflict://` scheme. The stripped `<file>:` 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.
+3 -219
View File
@@ -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<T> {
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<T> =
| { value?: null; error: ConfigError; status: "error" }
| { value: T; error?: undefined; status: "ok" }
| { value?: null; error?: unknown; status: "not-found" };
export class ConfigFile<T> implements IConfigFile<T> {
readonly #basePath: string;
#cache?: LoadResult<T>;
#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<T> {
if (!path || path === this.#basePath) return this;
const result = new ConfigFile<T>(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<T>): LoadResult<T> {
this.#cache = result;
return result;
}
tryLoad(): LoadResult<T> {
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
// =============================================================================
@@ -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<T> {
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<T> =
| { value?: null; error: ConfigError; status: "error" }
| { value: T; error?: undefined; status: "ok" }
| { value?: null; error?: unknown; status: "not-found" };
export class ConfigFile<T> implements IConfigFile<T> {
readonly #basePath: string;
#cache?: LoadResult<T>;
#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<T> {
if (!configPath || configPath === this.#basePath) return this;
const result = new ConfigFile<T>(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<T>): LoadResult<T> {
this.#cache = result;
return result;
}
tryLoad(): LoadResult<T> {
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;
}
}
@@ -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,
@@ -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<AgentSessionEvent, { type: "message_end" }>
| undefined;
const emitted = seen.find(
(event): event is Extract<AgentSessionEvent, { type: "message_end" }> => 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.
@@ -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<Uint8Array>): Promise<string> {
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<Uint8Array>),
readStream(proc.stderr as ReadableStream<Uint8Array>),
proc.exited,
]);
expect(exitCode).toBe(0);
});
});
@@ -68,9 +68,9 @@ function createStubInputControllerContext(opts: { skillCommands: Map<string, str
addToHistory: vi.fn(),
};
const enqueueCustomMessageDisplay = vi.fn((_text: string, _mode: "steer" | "followUp") => "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();
});
});