Repository navigation
Propagate an unresolvable path in the shared script launcher so gates cannot silently skip - #63
Conversation
… gate isMainInvocation caught a realpathSync failure and returned false. runIfMain reads false as "not the entry point", so the script it guards never runs and a required release check exits 0 having done nothing — the one failure these gates exist to prevent. Letting realpathSync throw makes that a loud non-zero exit instead. The docstring argued the right principle and then did the opposite: it said a release script that no-ops without erroring is worse than one that throws, and used "fail closed" to name the branch that no-ops. Here the crash is the safe outcome. The both-sides realpathSync comparison is kept. It exists to normalise Windows drive-letter casing, which is a separate and correct concern, so only the catch is removed. A test now asserts the throw, so the guard cannot regress silently.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThis PR changes the shared script launcher so an unresolvable entry path causes the process to fail loudly instead of silently skipping the guarded gate, and adds a regression test plus pm tracking artifacts documenting the defect. Sequence diagram for isMainInvocation error propagation in the shared script launchersequenceDiagram
actor Developer
participant npm
participant docstring_gate
participant script_launcher as isMainInvocation
participant fs as realpathSync
Developer->>npm: npm run docstring
npm->>docstring_gate: start
docstring_gate->>script_launcher: isMainInvocation(argv, moduleUrl)
alt entry path resolves
script_launcher->>fs: realpathSync(argv[1])
fs-->>script_launcher: resolved_entry_path
script_launcher->>fs: realpathSync(fileURLToPath(moduleUrl))
fs-->>script_launcher: resolved_module_path
script_launcher-->>docstring_gate: true or false
opt true
docstring_gate->>docstring_gate: run main gate
end
else entry path does not resolve (ENOENT)
script_launcher->>fs: realpathSync(argv[1])
fs-->>script_launcher: ENOENT error
script_launcher-->>docstring_gate: propagate ENOENT
docstring_gate-->>npm: non-zero exit (gate fails loudly)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 40 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
Walkthrough
ChangesLauncher error propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
The guard had been refactored into the shared script launcher, so a survey that searched scripts/docstring-gate.ts found nothing here and reported the repository clean.
|
@coderabbitai full review Reviewer context — the premise matters more than the diff here:
Please check specifically:
|
Rate Limit Exceeded
|
Greptile SummaryThis PR makes the shared script launcher propagate path-resolution failures so the docstring release gate exits non-zero rather than silently skipping.
Confidence Score: 5/5The PR appears safe to merge because the launcher now fails loudly on unresolvable paths while preserving normalized matching and resolvable non-match behavior. The only production launcher consumer is the docstring gate, the propagated exception produces the intended non-zero release-check failure, existing tests retain coverage of true and false path comparisons, and coverage thresholds and test inclusion remain unchanged.
|
| Filename | Overview |
|---|---|
| scripts/script-launcher.ts | Removes the catch that converted path-resolution failures to false while retaining both normalized path resolutions and the distinct-path behavior. |
| test/docstring-gate.test.ts | Adds an included, isolated regression test confirming that ENOENT propagates; existing tests continue to cover matching and different resolvable paths. |
| .agents/pm/issues/pm-linear-lonp.toon | Records the launcher defect, intended failure semantics, and rationale for retaining both-side path normalization. |
| .agents/pm/history/pm-linear-lonp.jsonl | Adds the corresponding issue history without affecting runtime behavior. |
Reviews (1): Last reviewed commit: "docs(pm): record why the fleet sweep mis..." | Re-trigger Greptile
|
@coderabbitai review Re-requesting because your last pass on this PR predates the current head (or was cut short by the hourly rate limit), and a green check from a rate-limited run is not a review. Flagging it explicitly rather than reading the silence as approval. Since your last look, the substantive change is that the symlink regression test now puts the symlink in |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR diff summary =='
git diff --stat HEAD^ HEAD
echo
echo '== PR diff =='
git diff --find-renames --find-copies HEAD^ HEAD -- \
scripts/script-launcher.ts \
test/docstring-gate.test.ts \
.agents/pm/history/pm-linear-lonp.jsonl \
.agents/pm/issues/pm-linear-lonp.toon
echo
echo '== Launcher outline =='
ast-grep outline scripts/script-launcher.ts --items all || true
echo
echo '== Launcher source =='
cat -n scripts/script-launcher.ts
echo
echo '== Relevant test outline =='
ast-grep outline test/docstring-gate.test.ts --items all || true
echo
echo '== Relevant test source =='
cat -n test/docstring-gate.test.ts
echo
echo '== Coverage and test configuration candidates =='
fd -HI -t f '^(package\.json|vitest\.config\..*|jest\.config\..*|nyc\.config\..*|\.nycrc.*|c8rc.*|tsconfig.*\.json)$' . \
-E node_modules -E .git | sort
echo
echo '== Coverage-related configuration =='
rg -n -i -C 4 '(coverage|threshold|lines|branches|functions|statements)' \
-g 'package.json' \
-g 'vitest.config.*' \
-g 'jest.config.*' \
-g 'nyc.config.*' \
-g '.nycrc*' \
-g 'c8rc.*' \
-g 'tsconfig*.json' . || true
echo
echo '== Shared launcher imports and calls =='
rg -n -P -C 4 '(from\s+[\'"][^\'"]*script-launcher|require\([\'"][^\'"]*script-launcher|(?:runIfMain|isMainInvocation)\s*\()' \
-g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' . \
-g '!node_modules/**' -g '!coverage/**'
echo
echo '== Changed coverage configuration files, if any =='
git diff --name-status HEAD^ HEAD -- \
'package.json' 'vitest.config.*' 'jest.config.*' 'nyc.config.*' '.nycrc*' 'c8rc.*' 'tsconfig*.json'Length of output: 24302 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Repository state and refs =='
git status --short --branch
git remote -v
git branch -a --no-color
echo
echo '== Recent commits =='
git log --oneline --decorate -12
echo
echo '== Candidate base refs and merge bases =='
for ref in origin/main origin/master main master; do
if git rev-parse --verify --quiet "$ref" >/dev/null; then
printf '%s: ' "$ref"
git merge-base HEAD "$ref"
fi
done
base=''
for ref in origin/main origin/master main master; do
if git rev-parse --verify --quiet "$ref" >/dev/null; then
base=$(git merge-base HEAD "$ref")
printf '\n== Full diff against %s (%s) ==\n' "$ref" "$base"
git diff --stat "$base" HEAD
git diff --name-status "$base" HEAD
break
fi
done
if [ -z "$base" ]; then
echo 'No standard base ref exists; inspect GitHub PR metadata.'
gh pr view 63 --repo unbraind/pm-linear --json baseRefName,baseRefOid,headRefName,headRefOid,commits
else
echo
echo '== Relevant full PR diff =='
git diff --find-renames "$base" HEAD -- scripts/script-launcher.ts test/docstring-gate.test.ts package.json
echo
echo '== Coverage configuration changes in full PR =='
git diff --name-status "$base" HEAD -- package.json tsconfig.json tsconfig.test.json
fi
echo
echo '== Direct imports and calls of the shared launcher =='
rg -n -C 4 -e 'script-launcher' -e 'runIfMain[[:space:]]*\(' -e 'isMainInvocation[[:space:]]*\(' \
-g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' \
-g '!node_modules/**' -g '!coverage/**' .
echo
echo '== TypeScript files that identify themselves as gates or execute via runIfMain =='
rg -n -i -C 3 -e 'gate' -e 'runIfMain[[:space:]]*\(' \
-g 'scripts/*.ts' -g '!node_modules/**' scripts test || true
echo
echo '== Symlink-related test code at current HEAD =='
rg -n -i -C 5 'symlink|moduleUrl|pathToFileURL' test scripts \
-g '*.ts' -g '*.mts' -g '*.cts'Length of output: 40811 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== GitHub PR `#63` metadata =='
gh pr view 63 --repo unbraind/pm-linear \
--json number,url,state,baseRefName,baseRefOid,headRefName,headRefOid,commits
echo
echo '== GitHub PR `#63` changed files =='
gh api repos/unbraind/pm-linear/pulls/63/files --paginate \
--jq '.[] | {filename, status, sha, additions, deletions, patch}'
echo
echo '== Symlink API and test-text search across the complete PR range =='
git log --oneline 6e2b02b..HEAD
git log -S'symlink' --oneline 6e2b02b..HEAD -- test/docstring-gate.test.ts scripts/script-launcher.ts || true
rg -n -i -C 3 'symlinkSync|symlink|readlink|moduleUrl' \
test scripts -g '*.ts' -g '*.mts' -g '*.cts' || true
echo
echo '== Functional commit test diff =='
git show --format=fuller --find-renames 773f095 -- test/docstring-gate.test.ts scripts/script-launcher.ts
echo
echo '== All runIfMain call sites, with definitions excluded =='
rg -n -P -C 3 '\brunIfMain\s*\(' \
-g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' \
-g '!scripts/script-launcher.ts' -g '!node_modules/**' -g '!coverage/**' .Length of output: 23929
Finding
Checks completed
|
|
Merging. CodeRabbit's review is current against this head and answered every question I asked, including the one that mattered most — it independently confirmed the revert-proof:
That matches what I measured locally, and it is the property worth having: a test that fails when the thing it guards is removed. It also confirmed the two things I was most worried about breaking — the dual-path One observation from its report worth recording rather than acting on here: it noted this repo's coverage thresholds are lines 70, branches 80, functions 80 — below the fleet's 100/100/100/100 mandate. That is pre-existing and out of scope for this PR, and it is already tracked as a fleet coverage-gap epic. Flagging it so the number in that report is not read as an endorsement of the threshold. |
The defect
isMainInvocationinscripts/script-launcher.tscaught arealpathSyncfailure andreturned
false.runIfMainreadsfalseas "not the entry point", so the script it guardsnever runs — and
npm run docstringexits 0 having scanned nothing. A required releasecheck reporting success without doing its job is the single failure this gate exists to
prevent. Letting
realpathSyncthrow turns it into a loud non-zero exit.The docstring argued the right principle and then described the opposite behaviour: it said
"a release script that no-ops without erroring is worse than one that throws" and then used
fail closed to name the branch that no-ops. Here the crash is the safe outcome.
Why this repo was missed by the fleet sweep
The fleet-wide fix for this defect surveyed
scripts/docstring-gate.tsfor the guard. Thisrepo had refactored it into a shared
script-launcher.ts, with the gate only re-exportingit, so the survey found nothing and reported the repo clean. Factoring a defect out of the
file people search makes it invisible to the search.
What was deliberately kept
Unlike the copies elsewhere, this variant resolves both sides through
realpathSynctonormalise Windows drive-letter casing. That reasoning is sound, so only the
catchwasremoved — the fix does not reintroduce the casing bug the both-sides comparison prevents.
Verification
npm test— 71 tests, all passnpm run coverage— thresholds met, unchangednpm run changelog:check— passesisMainInvocation(["node","/nonexistent/x.ts"], ...)now throwsENOENTpm items
Summary by Sourcery
Ensure the shared script launcher fails loudly when entry paths cannot be resolved so release gates cannot silently skip.
Bug Fixes:
Enhancements:
Tests:
Chores:
Summary by cubic
Propagate unresolvable entry paths in the shared script launcher so gates cannot silently skip. Release checks now fail loudly (non-zero) instead of exiting 0 when the entry path can’t be resolved (Linear pm-linear-lonp).
isMainInvocation: letrealpathSyncerrors bubble instead of returning false; still returns false only when paths resolve to different files.realpathSynccomparison to normalize Windows drive-letter casing; add a test that asserts the throw on ENOENT.Written for commit 773f095. Summary will update on new commits.