fix(hooks): guard doc-file-warning stdin listeners behind require.main#2358
fix(hooks): guard doc-file-warning stdin listeners behind require.main#2358sshworld wants to merge 2 commits into
Conversation
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (18)**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
Files:
{package.json,*.config.js,scripts/**/*.js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
scripts/**/*.js📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,json,env*}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{jsx,tsx,js,ts}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,env*}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt,cpp,c,fs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,jsx,tsx,py,java,go,rs,kt}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{jsx,tsx,html,js,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{scripts,bin}/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe hook moves stdin fallback handling into an exported ChangesDoc file warning hook
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
| Filename | Overview |
|---|---|
| scripts/hooks/doc-file-warning.js | Moves stdin listeners and data accumulator into a gated main() function, exports both run() and main(), and adds require.main === module guard — cleanly eliminating the listener-leak bug. |
| scripts/hooks/pre-write-doc-warn.js | Updated backward-compat shim to explicitly call main() instead of relying on the now-removed module-scope side effect; correct because this file is always a subprocess entrypoint and never required in-process. |
| tests/hooks/doc-file-warning.test.js | Adds three regression tests: listener count assertion on require(), in-process run() classification, and end-to-end shim verification via spawnSync — covers all three changed code paths. |
Reviews (3): Last reviewed commit: "docs(hooks): add JSDoc for doc-file-warn..." | Re-trigger Greptile
doc-file-warning.js registered process.stdin data/end listeners at module scope while also exporting run(). run-with-flags.js require()s any hook that exports run() for its in-process fast path, so importing this hook attached stray stdin listeners to the dispatcher process, corrupting the PreToolUse stdout JSON contract. This is the exact failure run-with-flags' own SAFETY comment warns about, and 24 sibling hooks already guard against it. - Move the stdin entrypoint into main() and gate it behind require.main === module - pre-write-doc-warn.js now calls main() explicitly instead of relying on the import side effect - Add regression tests: require() attaches no stdin listeners, run()/main() stay exported, and the pre-write-doc-warn shim still warns
b4d581e to
17fa10d
Compare
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Satisfies the docstring-coverage pre-merge check; documents the stdin entrypoint and why it must not run on require().
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
daltino
left a comment
There was a problem hiding this comment.
This PR improves the doc-file-warning hook by conditionally guarding stdin listeners with a require.main check, which prevents them from unintentionally activating when the file is imported. The developer also added a safeguard test to verify that importing the module in-process does not add stdin listeners. The changes are concise and adhere to the contribution guidelines. Nicely done!
Closes #2357
Problem
doc-file-warning.jsattachesprocess.stdindata/endlisteners at module scope and also exportsrun().run-with-flags.jsrequire()s any hook that exportsrun()for its in-process fast path, so importing this hook leaks stdin listeners into the dispatcher process and corrupts the PreToolUse stdout JSON — the exact case its own SAFETY comment warns about. 24 sibling hooks already guard this withrequire.main === module; this one didn't.Repro on current
main:Changes
doc-file-warning.js: move the stdin entrypoint intomain(), export it, and gate it behindrequire.main === module.pre-write-doc-warn.js: callmain()explicitly instead of relying on the import side effect (so the backward-compat entrypoint keeps working).doc-file-warning.test.js: regression coverage —require()attaches no stdin listeners,run()/main()stay exported, and thepre-write-doc-warnshim still emits its warning.Testing
node tests/hooks/doc-file-warning.test.js→ 63/63node tests/run-all.js→ 2894/2894node scripts/ci/validate-hooks.js→ pass