fix(cli-repl): work around buggy Node.js autocomplete MONGOSH-2635 #2530
Conversation
Work around the fact that, in some versions of the Node.js REPL, tab completion may cause side effects.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a Node.js REPL autocomplete bug (Node.js bug #59774) where tab completion can inadvertently execute code and cause side effects.
- Implements detection mechanism for the Node.js bug through runtime testing
- Adds workaround by using older, safer regex patterns for completion when the bug is present
- Updates Node.js LTS version from 20.19.4 to 20.19.5
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cli-repl/src/node-repl-fix-completer-side-effects.ts | New module implementing bug detection and workaround logic |
| packages/cli-repl/src/mongosh-repl.ts | Integrates the workaround into the REPL completion system |
| packages/cli-repl/src/mongosh-repl.spec.ts | Adds test to verify functions aren't executed during completion |
| .evergreen/node-20-latest.json | Updates Node.js LTS version metadata |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /^\s*\.(\w*)$/.exec(line) !== null || | ||
| // https://github.com/nodejs/node/blob/07220230d9effcb4822d7a55563a86af3764540c/lib/repl.js#L1226 | ||
| /\brequire\s*\(\s*['"`](([\w@./:-]+\/)?(?:[\w@./:-]*))(?![^'"`])$/.exec( | ||
| line | ||
| ) !== null || | ||
| // https://github.com/nodejs/node/blob/07220230d9effcb4822d7a55563a86af3764540c/lib/repl.js#L1225 | ||
| /\bimport\s*\(\s*['"`](([\w@./:-]+\/)?(?:[\w@./:-]*))(?![^'"`])$/.exec( | ||
| line | ||
| ) !== null || |
There was a problem hiding this comment.
[nitpick] These complex regex patterns are duplicated from Node.js source and could be extracted into named constants with descriptive names like DOT_COMPLETION_REGEX, REQUIRE_COMPLETION_REGEX, and IMPORT_COMPLETION_REGEX to improve readability and maintainability.
Copilot uses AI. Check for mistakes.
| const simpleMatch = | ||
| /(?:[\w$'"`[{(](?:\w|\$|['"`\]})])*\??\.)*[a-zA-Z_$](?:\w|\$)*\??\.?$/.exec( | ||
| line | ||
| ); |
There was a problem hiding this comment.
[nitpick] This complex regex pattern should be extracted into a named constant like LEGACY_COMPLETION_REGEX with a comment explaining it uses the older, safer pattern from before the Node.js bug was introduced.
Copilot uses AI. Check for mistakes.
| let ranFunction = false; | ||
| repl.context.causeSideEffect = () => (ranFunction = true); | ||
| try { | ||
| await promisify(repl.completer)('causeSideEffect().x'); |
There was a problem hiding this comment.
I'm a bit confused by this, why is it trying to access .x in particular? Where is that coming from?
There was a problem hiding this comment.
I've added a comment here – it doesn't matter which property we try to autocomplete, the important bit is that is is trying to autocomplete a property of the function call result (which it shouldn't evaluate)
There was a problem hiding this comment.
got it, thank you! the comment is helpful too
Work around the fact that, in some versions of the Node.js REPL,
tab completion may cause side effects.
(First commit is #2529)