Repository navigation
Propagate the docstring gate entry guard fix so an unresolvable entry cannot pass the gate - #32
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 GuidePropagates the docstring gate main-invocation guard fix so that an unresolvable entry path now crashes loudly instead of silently skipping the mandatory docstring gate, aligning runtime behavior, tests, and changelog with the new contract. Sequence diagram for docstring gate main invocation guard behaviorsequenceDiagram
participant Process
participant DocstringGate as scripts_docstring_gate
participant FS as node_fs_realpathSync
participant URL as node_url_pathToFileURL
Process->>DocstringGate: isMainInvocation(process.argv, import.meta.url)
alt [argv[1] resolves]
DocstringGate->>FS: realpathSync(argv[1])
FS-->>DocstringGate: resolvedEntry
DocstringGate->>URL: pathToFileURL(resolvedEntry)
URL-->>DocstringGate: href
DocstringGate-->>Process: href === moduleUrl
opt [isMainInvocation returns true]
Process->>DocstringGate: main(root)
end
else [argv[1] cannot be resolved]
DocstringGate->>FS: realpathSync(argv[1])
FS-->>DocstringGate: ENOENT
DocstringGate-->>Process: ENOENT propagates
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughThe docstring gate now canonicalizes both ChangesDocstring gate entry guard
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 SummaryThis PR fixes
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| scripts/docstring-gate.ts | isMainInvocation simplified to a one-liner that canonicalises both argv[1] and moduleUrl through realpathSync, removing the try/catch that silently swallowed ENOENT and let the gate exit 0. Unused pathToFileURL import removed. JSDoc updated with accurate @throws contract. |
| test/docstring-gate.test.ts | Removed the now-incorrect assertion that an absent argv[1] returns false; replaced with an ENOENT-throws assertion that directly exercises the catch removal. Added a symlink test for the --preserve-symlinks case. Both new tests are structurally sound. |
| CHANGELOG.md | Adds a changelog entry for this fix under the current unreleased section, consistent with existing entries. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[process.argv and import.meta.url] --> B[isMainInvocation]
B --> C{argv1 undefined?}
C -- yes --> D[return false]
C -- no --> E[realpathSync of argv1]
E -- throws --> F[propagate error - non-zero exit]
E -- resolves --> G[realpathSync of fileURLToPath moduleUrl]
G -- throws --> F
G -- resolves --> H{paths equal?}
H -- yes --> I[call main - gate runs]
H -- no --> J[return false - test import skips gate]
Reviews (11): Last reviewed commit: "test(gate): drop a cross-reference to a ..." | 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 paths =='
git diff --name-status HEAD^ HEAD
echo
echo '== Current PR diff =='
git diff --unified=80 HEAD^ HEAD -- scripts/docstring-gate.ts test/docstring-gate.test.ts package.json CHANGELOG.md
echo
echo '== Source outline =='
ast-grep outline scripts/docstring-gate.ts --items all || true
echo
echo '== Test outline =='
ast-grep outline test/docstring-gate.test.ts --items all || true
echo
echo '== Package scripts and coverage-related configuration =='
if [ -f package.json ]; then
sed -n '/"scripts"[[:space:]]*:/,/^[[:space:]]*},\{0,1\}[[:space:]]*$/p' package.json
rg -n -i -C 3 'coverage|threshold|docstring' package.json
fi
echo
echo '== Repository coverage configuration files =='
fd -HI -t f '^(vitest|jest|nyc|c8|\.nycrc|package)\..*|^\.nycrc.*$|^tsconfig.*\.json$' . \
-E node_modules -E .git | while IFS= read -r file; do
if rg -q -i 'coverage|threshold' "$file"; then
echo "--- $file"
rg -n -i -C 3 'coverage|threshold' "$file"
fi
doneLength of output: 20679 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Invocation tests with line numbers =='
nl -ba test/docstring-gate.test.ts | sed -n '60,105p'
echo
echo '== Symlink fixture and invocation-case searches =='
rg -n -i -C 3 'symlink|isMainInvocation\s*\(' test scripts \
-g '!node_modules/**' -g '!dist/**'
echo
echo '== Coverage threshold diff confirmation =='
git diff --unified=8 HEAD^ HEAD -- package.jsonLength of output: 784 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Invocation tests with line numbers =='
awk 'NR >= 60 && NR <= 105 { printf "%6d %s\n", NR, $0 }' test/docstring-gate.test.ts
echo
echo '== Symlink fixture and invocation-case searches =='
rg -n -i -C 3 'symlink|isMainInvocation\s*\(' test scripts \
-g '!node_modules/**' -g '!dist/**' || true
echo
echo '== Coverage threshold diff confirmation =='
git diff --unified=8 HEAD^ HEAD -- package.jsonLength of output: 6396
P2 — Add a symlink regression case for Add a fixture where both The implementation in ✏️ Learnings added
✅ Action performedFull review finished. |
![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 `@test/docstring-gate.test.ts`:
- Around line 102-112: Add a regression test alongside the existing
isMainInvocation tests that creates a real script and symlink, passes the
symlink path as argv[1] and its file URL as moduleUrl, and asserts
isMainInvocation returns true. Ensure the test cleans up the temporary script
and symlink, covering canonicalization of both invocation arguments.
🪄 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: 1d1acb8c-a099-4d54-9ded-d60a1ef891f8
📒 Files selected for processing (5)
.agents/pm/history/pm-github-wb4q.jsonl.agents/pm/issues/pm-github-wb4q.toonCHANGELOG.mdscripts/docstring-gate.tstest/docstring-gate.test.ts
…ails The existing test (isMainInvocation resolves matching and non-matching scripts) did not use symlinks at all, so 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).
|
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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.
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 `@test/docstring-gate.test.ts`:
- Around line 103-107: Update the explanatory comment before the
symlink-preservation regression test to reference the preceding regular-path
test, which uses regular files and does not pass a symlink as argv[1]. Keep the
explanation focused on why the current moduleUrl-as-symlink case distinguishes
the two implementations.
🪄 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: fe874d04-f2b7-4742-ae52-e70aa773a992
📒 Files selected for processing (4)
.agents/pm/history/pm-github-wb4q.jsonl.agents/pm/issues/pm-github-wb4q.toonscripts/docstring-gate.tstest/docstring-gate.test.ts
|
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 file confirms the imports are now exactly right: The fleet gap analysis is accurate and the deferral is correct. The problem has two independent failure modes:
One thing worth noting for when the fleet fix lands: No action needed in this PR — the dead import is gone, the gate is correct, and the gap is tracked. |
|
@coderabbitai review Final pass — no further changes planned. The only delta since your last review is the |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR changes against base =='
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD
git diff --unified=40 "$(git merge-base HEAD origin/main)" HEAD -- \
scripts/docstring-gate.ts test/docstring-gate.test.ts package.json CHANGELOG.md
echo '== Entry-guard source map =='
ast-grep outline scripts/docstring-gate.ts --items all
echo '== isMainInvocation implementation =='
rg -n -A45 -B12 'function isMainInvocation|const isMainInvocation|isMainInvocation\s*=' scripts/docstring-gate.ts
echo '== Invocation tests =='
ast-grep outline test/docstring-gate.test.ts --items all
rg -n -A35 -B10 'isMainInvocation\s*\(|symlinkSync|ENOENT|preserve-symlinks' test/docstring-gate.test.ts
echo '== Coverage-related diff and current configuration =='
git diff --unified=20 "$(git merge-base HEAD origin/main)" HEAD -- package.json
rg -n -i -C3 'coverage|threshold' package.json . \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' \
-g 'package.json' -g 'vitest.config.*' -g 'vite.config.*' -g 'jest.config.*' -g '.nycrc*' || true
echo '== Unused pathToFileURL references in relevant source/test files =='
rg -n -C2 '\bpathToFileURL\b' scripts test -g '!node_modules/**' -g '!dist/**' || trueLength of output: 32257
P3 — Correct the stale test comment. Update or remove that sentence. The current regression assertion at Line 118 is valid and discriminates against the previous one-sided implementation. I confirmed the following:
|
CodeRabbit caught that the comment opens with "The symlink test above" while this file has no preceding symlink test - the regression test is the first and only one here. The wording was copied from a repository that does have both, so the rationale read as describing a test that is not present. Rewritten to state the property directly rather than by reference: a case that passes the link as argv[1] and the real path as moduleUrl cannot distinguish the two implementations, because realpathSync(link) resolves to the real path either way.
|
Merging. CodeRabbit's review is current against this head, all required checks are green, and every thread is replied to and resolved. A correction to this PR's own description, which Greptile raised on pm-github#32 and which applies here: Six repos were in that position (pm-beads, pm-gantt-chart, pm-github, pm-jira, pm-slack, pm-slack-standup); nine genuinely moved one-sided → both-sided. Worth recording: the intermediate commit here temporarily replaced the stronger implementation with the weaker "canonical" one before a later commit restored it. Net zero and it never shipped, but merging mid-series would have introduced the The symlink regression test is kept deliberately. It does not discriminate this diff, but it pins the both-sided property so a future refactor toward the one-sided reference cannot land silently — which is exactly the regression that just happened here mid-PR. |
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— 249 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-github-wb4q
Summary by Sourcery
Ensure the docstring gate process entry guard fails loudly when the entry script path cannot be resolved so a broken environment cannot silently skip the mandatory docstring check.
Bug Fixes:
Enhancements:
Documentation:
Chores:
Summary by cubic
Fixes the docstring gate to fail loudly on missing or invalid entry paths and makes the main check robust to symlink-preserving runtime flags. Clarifies comments and removes an unused
pathToFileURLimport.argv[1]andimport.meta.urlwithrealpathSyncbefore comparing, so--preserve-symlinks*cannot cause a silent skip.moduleUrl; useprocess.execPathand asserterror.code === "ENOENT"for an unresolvable entry; clarify a test comment.Written for commit 7992d51. Summary will update on new commits.