-
Notifications
You must be signed in to change notification settings - Fork 140
fix(cli): silence resource collision diagnostics on quiet startup #1177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown>) { | ||
| this[field] = diagnostics | ||
| return returnValue | ||
| } | ||
| prototype.__kimchiCollisionDiagnosticsPatchApplied = false | ||
| installCollisionDiagnosticsPatch() | ||
|
|
||
| const instance: Record<string, unknown> = { | ||
| 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]) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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[]) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The wrapped update methods call 💡 Suggestion: Access the setting defensively and degrade to upstream (unfiltered) behaviour when it is unavailable, e.g. |
||
| const result = original.apply(this, args) | ||
| const verbose = getParsedCliArgs().options.verbose === true | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️🔧 Maintainability
💡 Suggestion: Guard on the field being present before reassigning, e.g. |
||
| const quietStartup = this.settingsManager.getQuietStartup() | ||
| this[field] = filterCollisionDiagnostics(this[field] ?? [], { verbose, quietStartup }) | ||
| return result | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upgrade-guard test
targets existing synchronous upstream methodsonly asserts thatupdateSkillsFromPaths,updatePromptsFromPathsandupdateThemesFromPathsexist and stay synchronous. It does not validate the other two private assumptions the patch depends on: that constructed instances exposesettingsManager.getQuietStartup(), and that the real update methods still assign their diagnostics toskillDiagnostics/promptDiagnostics/themeDiagnosticsonthis. BecauserunPatchOverStubreplaces the method body with a stub that writes the field itself, an upstream refactor that moves diagnostics storage elsewhere would pass the whole suite while the filter silently stops doing anything (or crashes per the unguardedsettingsManageraccess).💡 Suggestion: Add one integration-style test that instantiates a real
DefaultResourceLoader(or otherwise inspects the real class) and asserts the instance carries asettingsManagerwith a callablegetQuietStartup, and that running a genuineupdateSkillsFromPathscall writes toskillDiagnostics, so an upstream upgrade fails the suite loudly instead of degrading in production.