Conversation
Kimchi Code Review
Summary📊 Review Score: 84/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — The new test file gives good coverage of the filter semantics: quiet-startup filtering, 📝 Found 3 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 84/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — The new test file gives good coverage of the filter semantics: quiet-startup filtering, --verbose and quietStartup: false pass-through, per-sink coverage via it.each, idempotency of installCollisionDiagnosticsPatch, return-value preservation, and a guard that the three upstream methods exist and remain synchronous. The gap is that every test drives the wrapper with a hand-built stub instance and stub methods, so the real upstream assumptions that matter most on upgrade (settingsManager.getQuietStartup() existing on instances, and the real update methods assigning skillDiagnostics / promptDiagnostics / themeDiagnostics) are not exercised.
📝 Found 3 issue(s). See inline comments for details.
| 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.
The wrapped update methods call this.settingsManager.getQuietStartup() with no guard. settingsManager is a private upstream implementation detail of DefaultResourceLoader; if an upgrade of @earendil-works/pi-coding-agent renames it, makes it optional, or invokes these methods before it is assigned, every call to updateSkillsFromPaths, updatePromptsFromPaths and updateThemesFromPaths will throw a TypeError — breaking all skill/prompt/theme loading. That is a strictly worse failure mode than the collision noise this patch exists to hide, and the co-located tests would not catch it because they always pass a stub instance containing settingsManager.
💡 Suggestion: Access the setting defensively and degrade to upstream (unfiltered) behaviour when it is unavailable, e.g. const quietStartup = this.settingsManager?.getQuietStartup?.() === true. Combined with filterCollisionDiagnostics only filtering when quietStartup is true, any upstream shape change then silently restores the original diagnostics instead of crashing resource loading.
| }) | ||
|
|
||
| it("targets existing synchronous upstream methods", () => { | ||
| for (const [method] of DIAGNOSTIC_SINKS) { |
There was a problem hiding this comment.
The upgrade-guard test targets existing synchronous upstream methods only asserts that updateSkillsFromPaths, updatePromptsFromPaths and updateThemesFromPaths exist and stay synchronous. It does not validate the other two private assumptions the patch depends on: that constructed instances expose settingsManager.getQuietStartup(), and that the real update methods still assign their diagnostics to skillDiagnostics / promptDiagnostics / themeDiagnostics on this. Because runPatchOverStub replaces 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 unguarded settingsManager access).
💡 Suggestion: Add one integration-style test that instantiates a real DefaultResourceLoader (or otherwise inspects the real class) and asserts the instance carries a settingsManager with a callable getQuietStartup, and that running a genuine updateSkillsFromPaths call writes to skillDiagnostics, so an upstream upgrade fails the suite loudly instead of degrading in production.
| // 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 |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
this[field] ?? [] rewrites an unset diagnostics field from undefined to [] on every wrapped call. If upstream render logic ever distinguishes "no diagnostics computed yet" (undefined) from "computed, none found" ([]), this pass-through case stops being a no-op and changes rendering behaviour even when verbose is on or quietStartup is off.
💡 Suggestion: Guard on the field being present before reassigning, e.g. const stored = this[field]; if (Array.isArray(stored)) this[field] = filterCollisionDiagnostics(stored, { verbose, quietStartup }), so unset fields keep their original shape.
1 participant
Description
export-1789069003235.mp4
What does this PR do?
--verboseor quite startup is disabled they are properly displayed