diff --git a/src/cli.ts b/src/cli.ts index a7a1f3783..c121f15f6 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -167,6 +167,7 @@ import resourceToolBlockerExtension from "./resources/tool-blocker.js" import { runSetupWizard } from "./setup-wizard.js" import { setAvailableModels } from "./startup-context.js" import { probeTerminalBackground } from "./terminal-bg-probe.js" +import { installCollisionDiagnosticsPatch } from "./upstream-collision-diagnostics-patch.js" import { installInlineCompactPatch } from "./upstream-inline-compact-patch.js" import { installCompactionRecoveryPatch, installInfrastructureRetryPatch } from "./upstream-retry-patch.js" import { @@ -187,6 +188,9 @@ installPiNativeCompatibilityShim() // InteractiveMode instance is constructed. applyInteractiveErrorSurfacePatch() applyInteractiveModelSessionPatch() +// Hide non-actionable resource collisions from the startup report unless the +// user asked for a verbose startup before the first loader update emits them. +installCollisionDiagnosticsPatch() function getSubcommand(args: string[]): string { if (args.includes("--version") || args.includes("-v")) return "version" diff --git a/src/upstream-collision-diagnostics-patch.test.ts b/src/upstream-collision-diagnostics-patch.test.ts new file mode 100644 index 000000000..0998fd7be --- /dev/null +++ b/src/upstream-collision-diagnostics-patch.test.ts @@ -0,0 +1,132 @@ +import { DefaultResourceLoader, type ResourceDiagnostic } from "@earendil-works/pi-coding-agent" +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" +import { installCollisionDiagnosticsPatch } from "./upstream-collision-diagnostics-patch.js" + +const { getParsedCliArgs } = vi.hoisted(() => ({ getParsedCliArgs: vi.fn() })) +vi.mock("./cli-args.js", () => ({ getParsedCliArgs })) + +const DIAGNOSTIC_SINKS = [ + ["updateSkillsFromPaths", "skillDiagnostics"], + ["updatePromptsFromPaths", "promptDiagnostics"], + ["updateThemesFromPaths", "themeDiagnostics"], +] as const + +const collision: ResourceDiagnostic = { + type: "collision", + message: 'skill "duplicated" collides', + collision: { + resourceType: "skill", + name: "duplicated", + winnerPath: "/winner/SKILL.md", + loserPath: "/loser/SKILL.md", + }, +} +const warning: ResourceDiagnostic = { + type: "warning", + message: "description exceeds 1024 characters (1273)", + path: "/tmp/skill/SKILL.md", +} +const error: ResourceDiagnostic = { + type: "error", + message: "resource failed to load", + path: "/tmp/broken/SKILL.md", +} + +// biome-ignore lint/suspicious/noExplicitAny: private upstream prototype adapter +const prototype = DefaultResourceLoader.prototype as any +const originalMethods = new Map(DIAGNOSTIC_SINKS.map(([method]) => [method, prototype[method]])) + +let quietStartup = true +function setVerbose(verbose: boolean): void { + getParsedCliArgs.mockReturnValue({ options: { verbose }, positionals: [] }) +} + +function resetPrototype(): void { + for (const [method] of DIAGNOSTIC_SINKS) { + prototype[method] = originalMethods.get(method) + } + prototype.__kimchiCollisionDiagnosticsPatchApplied = false +} + +function runPatchOverStub( + method: string, + field: string, + diagnostics: ResourceDiagnostic[], + returnValue: unknown = undefined, +): { returned: unknown; diagnostics: unknown } { + prototype[method] = function stubInner(this: Record) { + this[field] = diagnostics + return returnValue + } + prototype.__kimchiCollisionDiagnosticsPatchApplied = false + installCollisionDiagnosticsPatch() + + const instance: Record = { + settingsManager: { getQuietStartup: () => quietStartup }, + } + const returned = prototype[method].call(instance, [], new Map()) + return { returned, diagnostics: instance[field] } +} + +describe("installCollisionDiagnosticsPatch", () => { + beforeEach(() => { + setVerbose(false) + quietStartup = true + resetPrototype() + installCollisionDiagnosticsPatch() + }) + + afterEach(() => { + resetPrototype() + vi.restoreAllMocks() + }) + + it("targets existing synchronous upstream methods", () => { + for (const [method] of DIAGNOSTIC_SINKS) { + const original = originalMethods.get(method) + expect(typeof original, `${method} must exist upstream`).toBe("function") + expect(original.constructor.name, `${method} must remain synchronous`).not.toBe("AsyncFunction") + } + }) + + it("is idempotent so repeated bootstrap does not stack wrappers", () => { + const wrapped = prototype.updateSkillsFromPaths + installCollisionDiagnosticsPatch() + expect(prototype.updateSkillsFromPaths).toBe(wrapped) + }) + + it("drops only collision diagnostics on quiet startup and preserves return values", () => { + const returnValue = { skills: [], diagnostics: [] } + const { returned, diagnostics } = runPatchOverStub( + "updateSkillsFromPaths", + "skillDiagnostics", + [warning, collision, error], + returnValue, + ) + + expect(returned).toBe(returnValue) + expect(diagnostics).toEqual([warning, error]) + }) + + it("keeps collision diagnostics and order when startup is verbose", () => { + setVerbose(true) + const input = [collision, warning, error] + const { diagnostics } = runPatchOverStub("updateSkillsFromPaths", "skillDiagnostics", input) + + expect(diagnostics).toEqual(input) + expect(diagnostics).not.toBe(input) + }) + + it("keeps collision diagnostics when quietStartup is disabled in settings", () => { + quietStartup = false + const input = [collision, warning] + const { diagnostics } = runPatchOverStub("updateSkillsFromPaths", "skillDiagnostics", input) + + expect(diagnostics).toEqual(input) + }) + + it.each(DIAGNOSTIC_SINKS)("filters %s into %s", (method, field) => { + const { diagnostics } = runPatchOverStub(method, field, [collision, warning]) + expect(diagnostics).toEqual([warning]) + }) +}) diff --git a/src/upstream-collision-diagnostics-patch.ts b/src/upstream-collision-diagnostics-patch.ts new file mode 100644 index 000000000..a7fef5433 --- /dev/null +++ b/src/upstream-collision-diagnostics-patch.ts @@ -0,0 +1,77 @@ +import { DefaultResourceLoader, type ResourceDiagnostic } from "@earendil-works/pi-coding-agent" +import { getParsedCliArgs } from "./cli-args.js" + +/** + * Startup diagnostics come in two flavours. A `collision` says two resources + * shared a name and precedence already picked a winner — a valid resource + * loaded, so there is nothing for the user to do. Everything else reports a + * resource that is actually broken, e.g. a skill whose description exceeds the + * 1024-character limit. + * + * Pi renders both unconditionally at startup (its `showDiagnosticsWhenQuiet` + * bypasses the `quietStartup` setting), which buries the actionable reports + * under collision noise. We drop only the collisions, and only when startup + * is actually quiet: the `quietStartup` setting is on and the user has not + * forced a verbose startup with `--verbose`. + * + * Because the filter mutates the stored diagnostics (there is no render-time + * hook — see below), keeping this gate in sync with Pi's render condition + * matters: users who disabled `quietStartup` opted into the full report and + * must keep their collision diagnostics. + */ +function filterCollisionDiagnostics( + diagnostics: readonly ResourceDiagnostic[], + options: { verbose: boolean; quietStartup: boolean }, +): ResourceDiagnostic[] { + if (options.verbose || !options.quietStartup) return [...diagnostics] + return diagnostics.filter((diagnostic) => diagnostic.type !== "collision") +} + +/** + * The loader methods that resolve a resource kind, paired with the field each + * one writes its diagnostics to. + */ +const DIAGNOSTIC_SINKS = [ + ["updateSkillsFromPaths", "skillDiagnostics"], + ["updatePromptsFromPaths", "promptDiagnostics"], + ["updateThemesFromPaths", "themeDiagnostics"], +] as const + +/** + * Drop non-actionable collision diagnostics from Pi's startup report when + * startup is quiet (`quietStartup` on, no `--verbose`). + * + * Pi exposes `skillsOverride` / `promptsOverride` / `themesOverride` for + * post-processing, but upstream `main()` builds the loader itself and accepts + * only `extensionFactories`, so the interactive path cannot thread them. + * Prototype accessors do not work because these hooks are class fields and + * every instance gets an own property that shadows the prototype. + * + * We therefore wrap the methods that publish diagnostics and filter what they + * stored. This runs after any caller-supplied `*Override`, so an explicit + * override (e.g. ACP's) still applies first. + * + * This adapter intentionally depends on private upstream names and assumes the + * update methods remain synchronous and keep assigning the fields listed + * above. The co-located tests guard both assumptions when Pi is upgraded. + */ +export function installCollisionDiagnosticsPatch(): void { + // biome-ignore lint/suspicious/noExplicitAny: private upstream prototype adapter + const prototype = DefaultResourceLoader.prototype as any + if (prototype.__kimchiCollisionDiagnosticsPatchApplied) return + prototype.__kimchiCollisionDiagnosticsPatchApplied = true + + for (const [method, field] of DIAGNOSTIC_SINKS) { + const original = prototype[method] + if (typeof original !== "function") continue + + // biome-ignore lint/suspicious/noExplicitAny: private upstream prototype adapter + prototype[method] = function patchedUpdate(this: any, ...args: unknown[]) { + const result = original.apply(this, args) + const verbose = getParsedCliArgs().options.verbose === true + const quietStartup = this.settingsManager.getQuietStartup() + this[field] = filterCollisionDiagnostics(this[field] ?? [], { verbose, quietStartup }) + return result + } + } +}