Harden extmgr package flows and migrate tooling to Biome#11
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughReplaces ESLint/Prettier with Biome; makes package identity normalization cwd-aware and canonicalizes local paths; refactors UnifiedItem into discriminated union; implements UI-loader fallback for tasks; propagates scope-aware identities across discovery/management/install flows; updates tests and CI/hooks accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader as runTaskWithLoader
participant UI as ctx.ui.custom
participant Task
rect rgba(100, 200, 150, 0.5)
Note over Client,Task: Custom UI available → returns success
Client->>Loader: runTaskWithLoader(task, config)
Loader->>UI: ctx.ui.custom(startedTask)
UI->>Loader: { type: "ok", value: T }
Loader->>Client: result.value
end
rect rgba(150, 100, 200, 0.5)
Note over Client,Task: No custom UI → run without loader
Client->>Loader: runTaskWithLoader(task, config)
Loader->>Loader: !hasCustomUI => runTaskWithoutLoader(task)
Loader->>Task: execute
Task-->>Loader: result
Loader->>Client: result
end
rect rgba(200, 150, 100, 0.5)
Note over Client,Task: Custom UI degrades (returns undefined)
Client->>Loader: runTaskWithLoader(task, config)
Loader->>UI: ctx.ui.custom(startedTask)
UI->>Loader: undefined
alt config.fallbackWithoutLoader
Loader->>Loader: runTaskWithoutLoader(task)
Loader->>Task: execute
Task-->>Loader: result
Loader->>Client: result
else no fallback
Loader->>Client: undefined
end
end
sequenceDiagram
participant Manager as Package Manager
participant Source as normalizePackageIdentity
participant Discovery
participant Catalog
rect rgba(100, 150, 200, 0.5)
Note over Manager,Catalog: Scope-aware identity normalization
Manager->>Source: normalizePackageIdentity(source, { cwd: ctx.cwd OR getAgentDir() })
Source->>Source: expand file://, ~/, resolve ./ ../ relative to cwd
Source-->>Manager: normalized identity
Manager->>Discovery: isSourceInstalled / match installed identities
Discovery->>Catalog: dedupeInstalledPackages(installed, cwd)
Catalog-->>Discovery: deduped list
Discovery-->>Manager: matched package(s)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/packages/management.ts (1)
60-66:⚠️ Potential issue | 🟠 MajorNormalize both sides of the update check the same way.
Line 60 switched the requested package identity to
cwd-aware normalization, but Line 65 still normalizesupdate.sourcewithout that context. Relative local sources can now compare unequal here, causingupdatePackageInternal()to incorrectly report “already up to date” and skip the actual update.Suggested fix
- const hasUpdate = updates.some( - (update) => normalizePackageIdentity(update.source) === updateIdentity - ); + const hasUpdate = updates.some( + (update) => packageIdentity(update.source, { cwd: ctx.cwd }) === updateIdentity + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/packages/management.ts` around lines 60 - 66, The comparison is inconsistent: updateIdentity is normalized with cwd-aware options but update.source is not, causing relative/local sources to mismatch and making updatePackageInternal() wrongly skip updates; fix by normalizing update.source the same way (call normalizePackageIdentity(update.source, { cwd: ctx.cwd }) when computing hasUpdate) so both sides use identical cwd-aware normalization before the comparison with updateIdentity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/pre-commit:
- Around line 10-12: The pre-commit hook should not use exec to run "corepack
pnpm run check" because exec replaces the shell and prevents the fallback to
direct "pnpm run check" if corepack is broken; change the logic so you first
detect corepack (command -v corepack) and then validate it (for example by
running a harmless check like "corepack pnpm --version" or running "corepack
pnpm run check" in a conditional) and only exec into corepack when that
validation succeeds, otherwise fall back to running "pnpm run check" directly so
the script can recover when corepack exists but is not functional.
In `@package.json`:
- Line 55: The package.json currently pins pnpm via the "packageManager" field
which conflicts with the CI workflow that also pins a pnpm version causing
ERR_PNPM_BAD_PM_VERSION; choose a single source of truth by removing or
unpinning the "packageManager" entry in package.json (the "packageManager" key)
or alternatively remove the pnpm pin from the CI workflow so only one location
controls the pnpm version—update whichever source you keep to the desired pnpm
version and ensure the other no longer specifies pnpm to avoid mismatches.
In `@README.md`:
- Line 159: Update the README tip that currently reads "After saving package
extension config, reload pi to apply changes." to use consistent product casing
and command phrasing by changing "pi" to "Pi" and referencing the reload command
as "/reload" (i.e., "After saving package extension config, run /reload to apply
changes."), locating the line that contains the exact string "After saving
package extension config, reload pi to apply changes." and replacing it
accordingly.
In `@src/packages/management.ts`:
- Around line 347-354: The removal logic computes identity =
packageIdentity(source, { cwd: ctx.cwd }) but compares installed entries using
project cwd or getAgentDir(), causing mismatches for globally installed local
packages; update the removal check (the later block around where removal is
decided, currently lines ~401-405) to reproduce the same candidate normalization
as the initial installed filter: generate the same packageIdentity variations
(using { cwd: ctx.cwd } and, when appropriate, { resolvedPath, cwd:
getAgentDir() }) or re-run the same installed.filter comparison against identity
so the comparison uses identical scope roots; reference packageIdentity,
identity, installed, ctx.cwd and getAgentDir() when applying the fix.
In `@src/utils/format.ts`:
- Around line 6-9: The truncate function can return a string longer than
maxLength when maxLength <= 3; update truncate to first handle small maxLength
by returning text.slice(0, maxLength) when maxLength <= 3 (no ellipsis),
otherwise keep the existing behavior (slice to maxLength - 3 and append '...');
modify the truncate function accordingly so its output length never exceeds
maxLength.
---
Outside diff comments:
In `@src/packages/management.ts`:
- Around line 60-66: The comparison is inconsistent: updateIdentity is
normalized with cwd-aware options but update.source is not, causing
relative/local sources to mismatch and making updatePackageInternal() wrongly
skip updates; fix by normalizing update.source the same way (call
normalizePackageIdentity(update.source, { cwd: ctx.cwd }) when computing
hasUpdate) so both sides use identical cwd-aware normalization before the
comparison with updateIdentity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40d59ec1-e0d2-4230-9c17-99b57fe9b988
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
.github/workflows/ci.yml.husky/pre-commit.prettierignore.prettierrcREADME.mdbiome.jsondocs/index.htmleslint.config.jspackage.jsonsrc/commands/auto-update.tssrc/commands/cache.tssrc/commands/history.tssrc/commands/install.tssrc/commands/registry.tssrc/commands/types.tssrc/extensions/discovery.tssrc/index.tssrc/packages/catalog.tssrc/packages/discovery.tssrc/packages/extensions.tssrc/packages/install.tssrc/packages/management.tssrc/types/index.tssrc/ui/async-task.tssrc/ui/footer.tssrc/ui/help.tssrc/ui/package-config.tssrc/ui/remote.tssrc/ui/theme.tssrc/ui/unified.tssrc/utils/auto-update.tssrc/utils/cache.tssrc/utils/command.tssrc/utils/format.tssrc/utils/history.tssrc/utils/mode.tssrc/utils/notify.tssrc/utils/npm-exec.tssrc/utils/package-source.tssrc/utils/retry.tssrc/utils/settings.tssrc/utils/status.tssrc/utils/ui-helpers.tstest/async-task.test.tstest/auto-update.test.tstest/cache-history.test.tstest/command-orchestration.test.tstest/command-tokenizer.test.tstest/discovery-parser.test.tstest/helpers/mocks.tstest/helpers/package-catalog.tstest/install-remove.test.tstest/npm-exec.test.tstest/package-config.test.tstest/package-extensions.test.tstest/remote-ui.test.tstest/rpc-mode.test.tstest/search-regression.test.tstest/settings-list.test.tstest/unified-items.test.ts
💤 Files with no reviewable changes (3)
- .prettierrc
- .prettierignore
- eslint.config.js
| }, | ||
| "author": "ayagmar", | ||
| "license": "MIT", | ||
| "packageManager": "[email protected]", |
There was a problem hiding this comment.
Make pnpm versioning single-sourced.
Line 55 adds a pinned packageManager, but CI is still pinning pnpm separately and the workflow is already failing with ERR_PNPM_BAD_PM_VERSION. As written, this PR blocks the pipeline; please let either package.json or the GitHub Action own the pnpm version, not both.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 55, The package.json currently pins pnpm via the
"packageManager" field which conflicts with the CI workflow that also pins a
pnpm version causing ERR_PNPM_BAD_PM_VERSION; choose a single source of truth by
removing or unpinning the "packageManager" entry in package.json (the
"packageManager" key) or alternatively remove the pnpm pin from the CI workflow
so only one location controls the pnpm version—update whichever source you keep
to the desired pnpm version and ensure the other no longer specifies pnpm to
avoid mismatches.
Validate corepack before execing the pre-commit check and let CD use the packageManager-pinned pnpm version. Keep removal identity matching consistent across project and global relative local package sources, clamp truncate() for very small limits, and refresh the related docs/tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Configuration Changes
Bug Fixes / UX
Documentation
Tests