Quarantine retired main-entry fixtures - #287
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTest fixtures across daemon, project-takeover, process-args, and tmux runtime-manager tests are updated to reference Changeslauncher-bin.js migration in tests
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/daemon.test.ts (1)
153-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated helper diverges from project-takeover.test.ts's version.
This
currentProjectServiceArgsderives--project-idviaproj-${basename(root)}, while the same-named helper insrc/project-takeover.test.ts(lines 23-28) derives it viagetProjectIdFor(projectRoot). Both mirror the retiredmain.js→launcher-bin.jsmigration but with different project-id logic under an identical function name — confusing for future maintainers scanning across files. Consider consolidating into a shared test-utils helper if both derivations are meant to represent the same production identity contract.🤖 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 `@src/daemon.test.ts` around lines 153 - 158, The duplicated currentProjectServiceArgs helper in daemon.test.ts is diverging from the same-named helper in project-takeover.test.ts because it builds --project-id differently. Update the daemon.test.ts helper to use the same project identity logic as getProjectIdFor(projectRoot) (or extract both uses into a shared test-utils helper) so the launcher-bin.js arguments stay consistent across tests and the identity contract is defined in one place.src/one-shot-node-inventory.test.ts (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented self-avoidance trick is fragile.
Splitting
"dist/" + "main.js"avoids this test file itself matchingretiredMainEntrypoint(since this file isn't inallowedRetiredMainEntrypointFiles). Without a comment, a future reformatting/refactor could collapse this into a literal string and silently break the guardrail (the test would then fail against itself, or worse, someone "fixes" it by adding this file to the allowlist, weakening the check).💬 Suggested clarifying comment
const skippedDeclarationFiles = [/\.d\.ts$/]; -const retiredMainEntrypoint = "dist/" + "main.js"; +// Built via concatenation so this file's own source text doesn't match the +// literal string below, avoiding the need to allowlist this test itself. +const retiredMainEntrypoint = "dist/" + "main.js";🤖 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 `@src/one-shot-node-inventory.test.ts` around lines 13 - 14, The `retiredMainEntrypoint` self-avoidance trick in `src/one-shot-node-inventory.test.ts` is fragile because the split string can be collapsed by a future refactor and break the guardrail. Add a brief clarifying comment near `retiredMainEntrypoint` explaining that `"dist/" + "main.js"` is intentionally split so this test file does not match its own retired-entrypoint check, and keep the existing `skippedDeclarationFiles`/`allowedRetiredMainEntrypointFiles` logic unchanged.
🤖 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.
Nitpick comments:
In `@src/daemon.test.ts`:
- Around line 153-158: The duplicated currentProjectServiceArgs helper in
daemon.test.ts is diverging from the same-named helper in
project-takeover.test.ts because it builds --project-id differently. Update the
daemon.test.ts helper to use the same project identity logic as
getProjectIdFor(projectRoot) (or extract both uses into a shared test-utils
helper) so the launcher-bin.js arguments stay consistent across tests and the
identity contract is defined in one place.
In `@src/one-shot-node-inventory.test.ts`:
- Around line 13-14: The `retiredMainEntrypoint` self-avoidance trick in
`src/one-shot-node-inventory.test.ts` is fragile because the split string can be
collapsed by a future refactor and break the guardrail. Add a brief clarifying
comment near `retiredMainEntrypoint` explaining that `"dist/" + "main.js"` is
intentionally split so this test file does not match its own retired-entrypoint
check, and keep the existing
`skippedDeclarationFiles`/`allowedRetiredMainEntrypointFiles` logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9490aad4-d78a-4f42-a2b6-15b17462bda4
📒 Files selected for processing (5)
src/daemon.test.tssrc/one-shot-node-inventory.test.tssrc/process-args.test.tssrc/project-takeover.test.tssrc/tmux/runtime-manager.test.ts
|
Independent review finding fixed in 1bef118: the retired main-entry inventory guard is now occurrence-based, catches both slash-path and path-join forms, and only allows the specific legacy/repair/artifact occurrences instead of allowlisting whole files. |
|
CodeRabbit summary notes fixed in 15b5dd3: daemon test fixtures now use the same getProjectIdFor path as project-takeover, and the retired-entrypoint scan has an explicit comment explaining the split string. |
Summary
Verification
Summary by CodeRabbit
Bug Fixes
Tests