Fix Leadtype CLI symlinked bin execution#34
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe CLI's direct-run detection was refactored to resolve real filesystem paths (realpath) instead of comparing URLs; tests were added to verify behavior with symlinked ChangesCLI symlink direct-run detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 `@packages/leadtype/src/cli.test.ts`:
- Around line 113-117: The symlink call in packages/leadtype/src/cli.test.ts
uses the "dir" type which fails on Windows CI; change it to pick "junction" on
Windows and "dir" elsewhere by deriving a link type (e.g., const linkType =
process.platform === "win32" ? "junction" : "dir") and pass that variable to
symlink; update the symlink invocation that references fixtureDir and "leadtype"
to use this platform-aware linkType so tests do not require elevated privileges
on Windows.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 894cde79-1654-465a-ad46-1d158e3b12fc
📒 Files selected for processing (3)
.changeset/fix-symlinked-cli-direct-run.mdpackages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/leadtype/src/cli.tspackages/leadtype/src/cli.test.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code
Files:
packages/leadtype/src/cli.tspackages/leadtype/src/cli.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/leadtype/src/cli.test.ts
🪛 markdownlint-cli2 (0.22.1)
.changeset/fix-symlinked-cli-direct-run.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (2)
packages/leadtype/src/cli.ts (1)
65-79: Direct-run canonicalization fix looks solid.Comparing resolved realpaths here correctly handles symlinked
.binand workspace entrypoints without changing CLI routing behavior.packages/leadtype/src/cli.test.ts (1)
120-126: Great regression assertions for both shim and linked CLI path.These two expectations directly verify the realpath-based
isDirectRun(...)behavior for the failing scenarios described in this PR.
KayleeWilliams
force-pushed
the
KayleeWilliams/run-change
branch
from
3bf46e2 to
21cd4d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/leadtype/src/cli.ts`:
- Around line 73-76: The exported helper isDirectRun currently uses inferred
types for its parameters and return type; update its signature to declare
explicit types (e.g., entry: string, moduleUrl: string) and an explicit boolean
return type while preserving the existing default values and behavior so the
function contract is clear for consumers and matches exported API expectations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 702f2bfb-19cc-4174-8ccc-90fbaa520cbf
📒 Files selected for processing (3)
.changeset/fix-symlinked-cli-direct-run.mdpackages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code
Files:
packages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/leadtype/src/cli.test.ts
🪛 markdownlint-cli2 (0.22.1)
.changeset/fix-symlinked-cli-direct-run.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (2)
.changeset/fix-symlinked-cli-direct-run.md (1)
1-5: Changeset entry is clear and release-note ready.This accurately communicates the CLI direct-run symlink/bin-shim fix and its user-facing impact.
packages/leadtype/src/cli.test.ts (1)
98-128: Great regression coverage for symlinked/bin-shim direct-run detection.The new case validates both
.binand symlinkeddist/cli.jsentry paths, and Line 107 keeps directory-link behavior Windows-safe.
| export function isDirectRun( | ||
| entry = process.argv[1], | ||
| moduleUrl = import.meta.url | ||
| ): boolean { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit parameter types to exported isDirectRun API.
Line 74 and Line 75 currently rely on inference; make the function contract explicit since this is an exported helper.
Proposed patch
export function isDirectRun(
- entry = process.argv[1],
- moduleUrl = import.meta.url
+ entry: string | undefined = process.argv[1],
+ moduleUrl: string = import.meta.url
): boolean {As per coding guidelines, **/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity.
🤖 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 `@packages/leadtype/src/cli.ts` around lines 73 - 76, The exported helper
isDirectRun currently uses inferred types for its parameters and return type;
update its signature to declare explicit types (e.g., entry: string, moduleUrl:
string) and an explicit boolean return type while preserving the existing
default values and behavior so the function contract is clear for consumers and
matches exported API expectations.
KayleeWilliams
force-pushed
the
KayleeWilliams/run-change
branch
from
21cd4d9 to
73d354c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/leadtype/src/cli.ts (1)
73-76: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit parameter types to exported
isDirectRun.Line 74 and Line 75 still rely on inference. For an exported helper, making the parameter contract explicit improves API clarity.
Suggested patch
export function isDirectRun( - entry = process.argv[1], - moduleUrl = import.meta.url + entry: string | undefined = process.argv[1], + moduleUrl: string = import.meta.url ): boolean {As per coding guidelines,
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity.🤖 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 `@packages/leadtype/src/cli.ts` around lines 73 - 76, The exported function isDirectRun currently relies on inferred parameter types for entry and moduleUrl; update its signature to declare explicit types (e.g., annotate entry and moduleUrl as strings) while keeping the boolean return type, so change the parameters for isDirectRun (entry, moduleUrl) to have explicit types to improve the public API clarity and satisfy the lint rule.
🤖 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.
Duplicate comments:
In `@packages/leadtype/src/cli.ts`:
- Around line 73-76: The exported function isDirectRun currently relies on
inferred parameter types for entry and moduleUrl; update its signature to
declare explicit types (e.g., annotate entry and moduleUrl as strings) while
keeping the boolean return type, so change the parameters for isDirectRun
(entry, moduleUrl) to have explicit types to improve the public API clarity and
satisfy the lint rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13036df2-5b92-40c4-bec0-06867a3bc120
📒 Files selected for processing (3)
.changeset/fix-symlinked-cli-direct-run.mdpackages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code
Files:
packages/leadtype/src/cli.test.tspackages/leadtype/src/cli.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/leadtype/src/cli.test.ts
🪛 markdownlint-cli2 (0.22.1)
.changeset/fix-symlinked-cli-direct-run.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (3)
.changeset/fix-symlinked-cli-direct-run.md (1)
5-5: Release note is clear and aligned with the fix.This changeset entry accurately describes the symlink/bin-shim direct-run behavior correction.
packages/leadtype/src/cli.ts (1)
65-71: Canonical-path comparison helper looks good.Using realpath with a fallback here is a solid, resilient approach for symlinked entrypoint detection.
packages/leadtype/src/cli.test.ts (1)
98-128: Excellent regression coverage for symlinked direct-run paths.This test meaningfully captures both
.binand symlinked package entrypoint scenarios and validates the realpath-based fix end-to-end.
1 participant
Summary
Fix the Leadtype CLI direct-run check so package-manager bin shims and symlinked workspace installs actually execute the CLI entrypoint.
Root Cause
The CLI compared
import.meta.urlagainstprocess.argv[1]after only resolving the argv path. Under pnpm and workspace symlinks,process.argv[1]can point at a.bin/leadtypeshim or symlinked package path whileimport.meta.urlpoints at the real package file, so the comparison failed andrunCli(...)was skipped.Changes
.bin/leadtypeand symlinkednode_modules/leadtype/dist/cli.jsentrypoints.leadtype.Validation
bun run --filter leadtype check-typesbun run --filter leadtype buildbun run --filter leadtype testbun run --filter leadtype lintbun testpassed, 165 tests across 19 filespnpm exec leadtype generate ...now runs and generates output instead of exiting silently