Add package install recovery and collision plans - #136
Conversation
Bundle pm-e48i for GitHub issues #133 and #134. Add structured npm 404 recovery for npm:<name> package installs, including fallback_candidates and next_best_command for unpublished first-party packages. Add package doctor triage.collision_plan for extension registration collisions with deterministic deactivate candidates and command/action feature-loss hints. Preserve command-specific recovery metadata when the top-level CLI enriches error context, document the new JSON surfaces, and update pm-changelog output. Verification: focused unit tests, pnpm typecheck, pnpm build, pnpm contracts:check, pnpm quality:static, pnpm security:scan, pnpm audit --audit-level low, full coverage 2531 tests at 100%, pm health, pm validate history drift, and isolated temp smoke.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 44 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR delivers two complementary error-recovery and diagnostics features: (1) deterministic npm 404 fallback hints with structured recovery candidates and next-best commands for missing packages, and (2) machine-readable extension collision planning in doctor output with ranked remediation candidates and feature-loss estimates. Both features share an extended error recovery payload infrastructure and improve autonomous workflow reliability. Changesnpm install fallback hints and extension collision planning
Sequence DiagramsequenceDiagram
participant CLI as CLI/pm install
participant Resolver as InstallSourceResolver
participant Registry as npm Registry
participant ErrorHandler as ErrorHandler
participant Output as JSON/Display Output
CLI->>Resolver: resolveInstallSource(npm:spec)
Resolver->>Registry: npm pack (404)
Registry-->>Resolver: 404 Not Found error
Resolver->>Resolver: detectNpmNotFound()
Resolver->>Resolver: buildRecoveryPayload(fallback_candidates, next_best_command)
Resolver->>ErrorHandler: throw PmCliError(NOT_FOUND, recovery)
ErrorHandler->>ErrorHandler: normalizeRecoveryPayload()
ErrorHandler->>ErrorHandler: renderRecoveryBundle()
ErrorHandler->>Output: formatted error with recovery hints
Output-->>CLI: structured recovery (candidates + next command)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
To use Codex here, create a Codex account and connect to github. |
Compress the new package recovery and collision-plan notes plus related-docs links so docs/EXTENSIONS.md stays below the docs-skills gate line limit.
|
Updated after CI docs-size failure; docs gate passes locally now. Re-running review agents for the new head. @codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
To use Codex here, create a Codex account and connect to github. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/EXTENSIONS.md (1)
1-460:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublic docs length gate is currently blocking this PR.
docs/EXTENSIONS.mdis now 460 lines, but CI enforces a max of 450 for public docs, so Docs/skills fails. Please trim or split ~10+ lines (for example, move detailed examples to a secondary doc and link to it) to restore a green gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/EXTENSIONS.md` around lines 1 - 460, The doc exceeds the 450-line public docs gate; trim ~10+ lines by extracting verbose example blocks or long lists into a secondary doc and replacing them with brief links. For example, move the "Runnable Examples" list and/or the long "Package Sources" and "Bundled first-party packages" install examples into a new appendix doc, then replace those sections in EXTENSIONS.md with a one-line pointer and update related references (e.g., the "Runnable Examples" heading, the "Package Sources" code examples, and the bulleted bundled packages list) so the file drops under 450 lines while preserving links and README anchors.Sources: Coding guidelines, Pipeline failures
🧹 Nitpick comments (2)
tests/unit/cli-main-errors.spec.ts (1)
58-64: ⚡ Quick winAssert fallback candidate
reasonto lock the full recovery contract.The test claims command-specific fallback recovery is preserved, but it currently doesn’t validate
fallback_candidates[].reason, so that field could regress unnoticed.Proposed assertion tightening
fallback_candidates: [ { source: "github.com/unbraind/pm-brief", command: "pm install --project github.com/unbraind/pm-brief", + reason: "canonical first-party GitHub repository fallback", }, ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/cli-main-errors.spec.ts` around lines 58 - 64, Add an assertion in the test that validates the fallback candidate's reason field so the recovery contract is locked down; locate where the test inspects fallback_candidates (and next_best_command) in tests/unit/cli-main-errors.spec.ts and assert that fallback_candidates[0].reason equals the expected descriptive string for the command-specific fallback (alongside the existing checks for fallback_candidates and next_best_command) to prevent silent regressions.tests/unit/extension-command.spec.ts (1)
863-873: ⚡ Quick winAssert the full recovery payload shape, not just a subset.
The current matcher omits
attempted_command,normalized_args, and fallbackreason, so regressions in those fields won’t be caught on this runtime path.Proposed assertion tightening
context: { code: "npm_package_not_found", recovery: { + attempted_command: + "pm install --project npm:pm-definitely-missing-for-fallback-test-zzzzzz", + normalized_args: [ + "install", + "--project", + "npm:pm-definitely-missing-for-fallback-test-zzzzzz", + ], next_best_command: "pm install --project github.com/unbraind/pm-definitely-missing-for-fallback-test-zzzzzz", fallback_candidates: [ { source: "github.com/unbraind/pm-definitely-missing-for-fallback-test-zzzzzz", command: "pm install --project github.com/unbraind/pm-definitely-missing-for-fallback-test-zzzzzz", + reason: + "canonical first-party GitHub repository fallback for unpublished pm packages", }, ], }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/extension-command.spec.ts` around lines 863 - 873, The test currently only asserts parts of the recovery object; update the assertion in tests/unit/extension-command.spec.ts to validate the full recovery payload shape (include attempted_command, normalized_args, and each fallback candidate's reason in addition to next_best_command and fallback_candidates) instead of a partial match—locate the expectation that checks the "recovery" object (likely using toMatchObject or similar) and expand it to assert these exact fields and their expected values/types so regressions in attempted_command, normalized_args, and fallback.reason are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/extension/install-sources.ts`:
- Around line 199-207: The current isNpmNotFoundError helper is too broad (it
matches any "not found") and causes unrelated failures to be remapped to
npm_package_not_found; change isNpmNotFoundError to only detect explicit npm
registry signatures (e.g., "npm ERR! code E404", "404 not found", "is not in
this registry") and remove the generic "not found" check, then add a new helper
(e.g., isNpmPackNotFoundError) that includes the broader "not found" string and
use that new helper only in the catch block that wraps the npm pack/extraction
flow (the npm pack failure path), leaving all other call sites to use the
stricter isNpmNotFoundError to avoid mislabeling unrelated errors.
- Around line 214-240: buildNpmNotFoundRecovery is unconditionally emitting a
first-party GitHub fallback (githubSource/nextBestCommand) even for
non-first-party specs; change it to only add the githubSource, next_best_command
and fallback_candidates when the spec is clearly a first-party package (e.g.,
derived packageName has the expected first-party scope or org like "`@unbraind/`"
or otherwise matches your project's first-party naming convention). Use
npmPackageNameFromSpec to derive packageName, check its scope/org or pattern,
and only populate githubSource, nextBestCommand and the
fallback_candidates/recovery fields when that check passes; otherwise omit those
first-party-specific suggestions. Ensure references to githubSource and
nextBestCommand are only used after the guard so you don't emit misleading
commands for third-party packages.
In `@tests/unit/extension-command.spec.ts`:
- Around line 857-876: The test "classifies missing npm package installs with
deterministic fallback recovery" currently relies on a live npm 404; instead
stub or mock the npm resolution path so resolveInstallSource and any underlying
npm resolution function (e.g., the module/function that performs npm registry
lookups or the command runner used by resolveInstallSource) returns a synthetic
"package not found" error with the same shape (exitCode EXIT_CODE.NOT_FOUND and
context.code "npm_package_not_found" plus the expected recovery payload).
Replace the direct
parseExtensionInstallSource("npm:pm-definitely-missing-for-fallback-test-zzzzzz")
live-lookup behavior with a test seam/mocked response for the npm resolution and
assert the identical rejection shape from resolveInstallSource.
---
Outside diff comments:
In `@docs/EXTENSIONS.md`:
- Around line 1-460: The doc exceeds the 450-line public docs gate; trim ~10+
lines by extracting verbose example blocks or long lists into a secondary doc
and replacing them with brief links. For example, move the "Runnable Examples"
list and/or the long "Package Sources" and "Bundled first-party packages"
install examples into a new appendix doc, then replace those sections in
EXTENSIONS.md with a one-line pointer and update related references (e.g., the
"Runnable Examples" heading, the "Package Sources" code examples, and the
bulleted bundled packages list) so the file drops under 450 lines while
preserving links and README anchors.
---
Nitpick comments:
In `@tests/unit/cli-main-errors.spec.ts`:
- Around line 58-64: Add an assertion in the test that validates the fallback
candidate's reason field so the recovery contract is locked down; locate where
the test inspects fallback_candidates (and next_best_command) in
tests/unit/cli-main-errors.spec.ts and assert that fallback_candidates[0].reason
equals the expected descriptive string for the command-specific fallback
(alongside the existing checks for fallback_candidates and next_best_command) to
prevent silent regressions.
In `@tests/unit/extension-command.spec.ts`:
- Around line 863-873: The test currently only asserts parts of the recovery
object; update the assertion in tests/unit/extension-command.spec.ts to validate
the full recovery payload shape (include attempted_command, normalized_args, and
each fallback candidate's reason in addition to next_best_command and
fallback_candidates) instead of a partial match—locate the expectation that
checks the "recovery" object (likely using toMatchObject or similar) and expand
it to assert these exact fields and their expected values/types so regressions
in attempted_command, normalized_args, and fallback.reason are caught.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d55ede82-682e-4e7a-845f-9f2c33166a02
📒 Files selected for processing (13)
.agents/pm/history/pm-e48i.jsonl.agents/pm/tasks/pm-e48i.toonCHANGELOG.mddocs/EXTENSIONS.mdsrc/cli/commands/extension.tssrc/cli/commands/extension/doctor.tssrc/cli/commands/extension/install-sources.tssrc/cli/error-guidance.tssrc/cli/main.tssrc/core/shared/errors.tstests/unit/cli-main-errors.spec.tstests/unit/error-guidance.spec.tstests/unit/extension-command.spec.ts
Address CodeRabbit feedback on PR #136. Narrow npm not-found classification, keep broad generic matching inside the npm-pack wrapper, and only emit first-party GitHub fallback candidates for @unbrained/pm-* or pm-* package names. Replace the live registry 404 test with synthetic wrapper coverage and assert the full recovery payload, including attempted_command, normalized_args, fallback reason, and next_best_command. Verification: focused unit tests, typecheck, docs-skills/static/security gates, contracts/changelog checks, isolated temp smoke, and full 100% coverage.
|
Addressed CodeRabbit feedback: narrowed npm 404 classification, gated first-party GitHub fallback suggestions, removed live registry dependency from tests, and tightened recovery payload assertions. Re-running review agents for the new head. @codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
To use Codex here, create a Codex account and connect to github. |
|
🧠 Learnings used✅ Action performedReview finished.
|
1 participant
Summary
Verification
Tracker
@codex review
/gemini review
@gemini-code-assist