Propagate the docstring gate entry guard fix so an unresolvable entry cannot pass the gate - #59
Conversation
…ing the gate The isMainInvocation guard caught realpathSync errors and returned false. When argv[1] could not be resolved, the top-level selector called the no-op placeholder instead of main, so npm run docstring exited 0 having scanned nothing — a mandatory release gate reporting success without doing its job. The corrected implementation propagates the realpathSync error. The case requires argv[1] to stop resolving after Node has already loaded this file, so in practice it means the environment is broken, and a broken environment must not silently satisfy a gate. Crashing loudly is the safe outcome.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThis PR changes the docstring gate’s main-invocation guard so that an unresolvable entry path now propagates the underlying filesystem error instead of silently declining to run, updates tests to assert the new failure mode, and records the fix in the changelog and project management artifacts. Sequence diagram for docstring gate main-invocation error propagationsequenceDiagram
actor Developer
participant npm
participant NodeProcess
participant DocstringGateScript
participant Filesystem
Developer->>npm: npm run docstring
npm->>NodeProcess: start docstring-gate entry
NodeProcess->>DocstringGateScript: invoke isMainInvocation(argv, moduleUrl)
DocstringGateScript->>Filesystem: realpathSync(argv[1])
Filesystem-->>DocstringGateScript: ENOENT error
Note right of DocstringGateScript: realpathSync error is propagated
DocstringGateScript-->>NodeProcess: throw ENOENT
NodeProcess-->>npm: non-zero exit
npm-->>Developer: docstring gate fails loudly (scan not run)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Summary by CodeRabbit
WalkthroughThe docstring gate now canonicalizes the entry path and module URL. It propagates ChangesDocstring gate fix
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 |
Greptile SummaryThe PR makes the docstring gate fail loudly when its entry path cannot be resolved and canonicalizes both entry-point paths for symlink-preserving Node configurations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| scripts/docstring-gate.ts | Propagates path-resolution failures and compares canonical paths on both sides, completing the previously requested entry-guard fix. |
| test/docstring-gate.test.ts | Covers ENOENT propagation and the symlinked moduleUrl behavior produced by Node symlink-preservation flags. |
| CHANGELOG.md | Records the docstring-gate fix in the unreleased changelog. |
Reviews (9): Last reviewed commit: "refactor(gate): drop the import left unu..." | Re-trigger Greptile
|
@coderabbitai full review Reviewer context — this is a small change with an inverted premise behind it, so please read
The original comment and its test both used the phrase "fail closed" to mean "does not Specific things worth checking:
|
Rate Limit Exceeded
|
…gate Greptile and CodeRabbit independently flagged the same hole in the fix from the previous commit, on two different repositories. Comparing `pathToFileURL(realpathSync(entry)).href` against a raw `moduleUrl` resolves only one side. That is sufficient under Node's defaults, where the ESM loader realpaths a module before recording `import.meta.url`. Under `--preserve-symlinks` or `--preserve-symlinks-main` it is not: `moduleUrl` keeps the symlink while `realpathSync(entry)` resolves it, so a direct invocation through a symlink compares unequal, the selector calls the placeholder, and `npm run docstring` exits 0 without scanning. That is the exact silent skip this function exists to prevent, reintroduced by a launch flag. Measured rather than argued. With `moduleUrl` holding the symlink path: both-sides (new): true one-sided (old): false Canonicalising both sides costs one syscall and removes the dependence on how Node was launched. The tests also now use `process.execPath` rather than the literal "node", so the argv matches a real invocation on systems where the binary is named differently, and assert on `error.code === "ENOENT"` rather than matching the message text, which is not part of Node's contract.
|
@coderabbitai full review New commits since the last pass. All previous threads on this PR are replied to, voted on and What changed and why, because the second round came from you rather than from me: Greptile (on pm-beads) and CodeRabbit (on pm-slack) independently found that the fix in the Measured with Both sides are now canonicalised. Also applied from Sourcery on sibling PRs: What is most worth your attention this round:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff summary ==='
git diff --stat HEAD^ HEAD
echo
echo '=== Changed files ==='
git diff --name-only HEAD^ HEAD
echo
echo '=== Latest diff ==='
git diff --no-ext-diff --unified=80 HEAD^ HEAD -- \
scripts/docstring-gate.ts \
test/docstring-gate.test.ts \
package.json \
CHANGELOG.md \
.agents/pm/issues/pm-graph-r8nz.toon \
.agents/pm/history/pm-graph-r8nz.jsonl
echo
echo '=== scripts/docstring-gate.ts outline ==='
ast-grep outline scripts/docstring-gate.ts --items all --view expanded || true
echo
echo '=== test/docstring-gate.test.ts outline ==='
ast-grep outline test/docstring-gate.test.ts --items all --view expanded || true
echo
echo '=== isMainInvocation definitions and call sites ==='
rg -n -C 5 --glob '!node_modules/**' \
'\bisMainInvocation\s*(?:\(|[:=])|isMainInvocation' \
scripts test package.json . 2>/dev/null || true
echo
echo '=== Coverage settings and thresholds ==='
rg -n -i -C 5 --glob '!node_modules/**' \
'(coverageThreshold|thresholds?|coverage.*(?:line|branch|function|statement)|(?:line|branch|function|statement).*(?:coverage|threshold))' \
. 2>/dev/null || true
echo
echo '=== Test and docstring scripts ==='
rg -n -C 3 '"(?:test|docstring|coverage)"\s*:' package.json 2>/dev/null || trueLength of output: 53881 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Refs and candidate PR base ==='
git branch --show-current
git show-ref --verify --quiet refs/remotes/origin/main && echo 'origin/main is available' || true
base="$(git merge-base HEAD origin/main)"
printf 'merge base: %s\n' "$base"
echo
echo '=== Complete PR changed files ==='
git diff --name-status "$base" HEAD
echo
echo '=== Complete PR diff: executable and configuration files ==='
git diff --no-ext-diff --unified=50 "$base" HEAD -- \
scripts/docstring-gate.ts \
test/docstring-gate.test.ts \
package.json \
scripts/coverage-gate.ts
echo
echo '=== All symlink-preservation and URL/path guard tests ==='
rg -n -i -C 6 --glob '!node_modules/**' \
'(preserve-symlinks|preserve-symlinks-main|symlinkSync|fileURLToPath|isMainInvocation)' \
test scripts
echo
echo '=== Full current guard-test section ==='
sed -n '135,190p' test/docstring-gate.test.ts
echo
echo '=== Coverage configuration changed in complete PR ==='
git diff --no-ext-diff --unified=20 "$base" HEAD -- package.json scripts/coverage-gate.tsLength of output: 32989
I found one test gap.
The implementation itself is correct. It canonicalises both paths, so a real direct invocation returns The complete PR range does not modify
|
…ails The existing symlink test passes argv[1] as the link and moduleUrl as the REAL path, which the old one-sided comparison pathToFileURL(realpathSync (entry)).href === moduleUrl also satisfied — it could not distinguish the fixed implementation from the broken one. The new test puts the symlink in moduleUrl (pathToFileURL(link).href), which is what Node records in import.meta.url under --preserve-symlinks / --preserve-symlinks-main. The old comparison resolves argv[1] to the real path and compares it to the symlink URL, which is false, so the gate silently skips. The canonicalized comparison resolves both sides through realpathSync and returns true. Measured for this repo: reverting to the old one-sided comparison makes the new test fail (fail 1), restoring the canonicalization makes it pass (fail 0). The existing symlink test is retained — argv[1] arriving through an npm bin shim is a separate real case.
![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 `@scripts/docstring-gate.ts`:
- Around line 116-127: Update the explanatory comment near the canonicalization
logic to remove the claim that resolving both paths costs one syscall; describe
the cost only as additional filesystem work or omit the cost statement. Keep the
behavior and rationale for canonicalizing both sides unchanged, including the
realpathSync calls in the direct-invocation gate.
🪄 Autofix
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 Plus
Run ID: 165f2b37-ebac-4291-be96-6a493d20cc4b
📒 Files selected for processing (4)
.agents/pm/history/pm-graph-r8nz.jsonl.agents/pm/issues/pm-graph-r8nz.toonscripts/docstring-gate.tstest/docstring-gate.test.ts
|
New commit since the last pass. (Deliberately not What changed: CodeRabbit found that the symlink regression test added in the previous The added test puts the symlink in The original symlink test is kept — Worth your attention: this is the second time in this series that something which looked |
CodeRabbit flagged that the comment said canonicalising both sides costs one syscall while the function calls realpathSync twice, and each resolution can itself require several filesystem operations. The claim was mine and it was copied into every adopting repository along with the fix. The accurate statement is that it adds a second realpathSync. What the comment is actually justifying is the removal of a dependence on how Node was launched, and that argument does not need a cost figure to stand.
|
One more commit: a comment-only correction, no behaviour change. CodeRabbit found that the JSDoc claimed canonicalising both sides "costs one syscall" while Worth noting how far it travelled: the claim was mine and was copied verbatim into 14 Nothing else changed in this commit. If you see any other claim in this file that is stated |
|
Final review pass — no further changes are planned for this PR. Since your last look the only delta is a comment-only correction: the JSDoc claimed State of this PR:
One correction to this PR's own description, raised by you on pm-github#32 and applicable If you have no further findings, this is ready to merge. |
DeepScan flagged one new issue on these PRs and this is it: switching to `realpathSync(entry) === realpathSync(fileURLToPath(moduleUrl))` removed the last use of `pathToFileURL` in this file, but the import stayed. Nothing else caught it. These packages have no lint script, and typecheck does not enable noUnusedLocals, so the only gate that saw it was the advisory one.
|
One more commit, and it came from DeepScan — the advisory check that was reporting Switching the entry guard to Nothing mandatory caught it. These packages have no Tracked as a fleet gap rather than patched here: every package should either have a lint Verified after the change: typecheck, tests, docstring gate and |
|
The cleanup is clean. The fleet gap framing is exactly right. The advisory caught a cosmetic issue (unused import), not a behavioral one, and doing so only because the substantive change happened to leave a dead symbol behind. A One thing worth noting for the tracking issue: |
|
@coderabbitai review Final pass — no further changes planned. The only delta since your last review is the |
|
|
|
@coderabbitai review Requesting a review of the current head. No further changes are planned for this PR. The commits since your last look are the ones you and Greptile asked for: removing the import left unused by the two-sided comparison (DeepScan caught it; nothing mandatory did, since these packages have no lint script and typecheck does not enable All threads are replied to, voted on and resolved; required checks are green; the regression test is revert-proofed (reverting the implementation makes it fail). |
Rate Limit Exceeded
|
|
@coderabbitai review Requesting a review of the current head. No further changes are planned. The automatic review of this head returned a rate-limit notice rather than a review, and the incremental system then treats the commit as already reviewed, so it never retries on its own. The green check therefore reflects an attempt, not a review. The diff is the same change already reviewed and merged on seven sibling PRs. All threads are replied to, voted on and resolved; required checks are green. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
if git show-ref --verify --quiet refs/remotes/origin/main; then
base="$(git merge-base HEAD origin/main)"
else
base="$(git rev-parse HEAD^)"
fi
printf '=== Base ===\n%s\n' "$base"
printf '\n=== Changed files ===\n'
git diff --name-status "$base" HEAD
printf '\n=== Guard and test diff ===\n'
git diff --no-ext-diff --unified=80 "$base" HEAD -- \
scripts/docstring-gate.ts \
test/docstring-gate.test.ts
printf '\n=== Current guard outline ===\n'
ast-grep outline scripts/docstring-gate.ts --items all --view expanded
printf '\n=== Current guard implementation ===\n'
rg -n -C 30 --glob '!node_modules/**' \
'function isMainInvocation|const isMainInvocation|isMainInvocation\s*=' \
scripts/docstring-gate.ts
printf '\n=== Current guard test cases ===\n'
rg -n -C 18 --glob '!node_modules/**' \
'isMainInvocation|preserve-symlinks|symlinkSync|ENOENT|realpathSync' \
test/docstring-gate.test.ts
printf '\n=== Coverage configuration changes ===\n'
git diff --no-ext-diff --unified=40 "$base" HEAD -- \
package.json \
scripts/coverage-gate.ts \
.c8rc \
.nycrc \
vitest.config.ts \
jest.config.js
printf '\n=== Current coverage threshold definitions ===\n'
rg -n -i -C 4 --glob '!node_modules/**' \
'(coverageThreshold|thresholds?|coverage.*(?:line|branch|function|statement)|(?:line|branch|function|statement).*(?:coverage|threshold))' \
package.json scripts . 2>/dev/null || trueLength of output: 46009
The regression test places the symlink in The PR does not modify coverage configuration. The thresholds remain 81 lines, 77 branches, and 88 functions. I performed a static review. I did not run repository code in this sandbox. You are interacting with an AI system. |
|
@coderabbitai review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Merging. CodeRabbit's full review of the current head finished with no findings; all threads resolved; required checks green. This repo genuinely moved one-sided → both-sided, so the canonicalization is substantive here rather than only the Note on the review itself: a plain |
|
Tip For best results, initiate chat on the files or code changes.
The two-sided canonicalization prevents the
You are interacting with an AI system. |
1 participant
Propagate the docstring gate entry guard fix
Summary
The
isMainInvocationguard inscripts/docstring-gate.tscaughtrealpathSyncerrors and returned
false. Whenargv[1]could not be resolved, the top-levelselector called the no-op placeholder instead of
main, sonpm run docstringexited 0 having scanned nothing — a mandatory release gate reporting success
without doing its job.
The corrected implementation propagates the
realpathSyncerror. A brokenenvironment must not silently satisfy a gate; crashing loudly is the safe outcome.
Changes
scripts/docstring-gate.ts:isMainInvocationnow propagatesrealpathSyncerrors instead of catching them and returning
false. The JSDoc is updated todocument the new
@throwscontract and the rationale.test/docstring-gate.test.ts: the test assertingfalsefor an unresolvableargv[1]is replaced with one assertingassert.throws(..., /ENOENT/).Verification
npm test— 216 tests passnpm run docstring— exits 0, prints the "N file(s), N declaration(s)" lineisMainInvocation(["node","/nonexistent/x.ts"],"file:///x")throws
ENOENT(printed "GOOD: threw ENOENT")pm item
pm-graph-r8nz
Summary by Sourcery
Ensure the docstring gate fails loudly when its entry script path cannot be resolved instead of silently skipping the gate.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by cubic
Prevent silent skips of the docstring gate.
isMainInvocationnow canonicalizes both the entry path andmoduleUrland propagatesrealpathSyncerrors; also drops an unused import so symlinked or unresolvable entries fail loudly.argv[1]andmoduleUrlwithrealpathSync; propagate errors. Tests useprocess.execPath, assertENOENT, and pin the symlinkedmoduleUrlcase.pathToFileURLimport.Written for commit 949af97. Summary will update on new commits.