fix(cli): resolve Windows storage 'EINVAL' and '/dev/tty' crashes, improve early debug logging#2052
Open
HumanBot000 wants to merge 6 commits into
Open
fix(cli): resolve Windows storage 'EINVAL' and '/dev/tty' crashes, improve early debug logging#2052HumanBot000 wants to merge 6 commits into
HumanBot000 wants to merge 6 commits into
Conversation
Contributor
|
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 (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2026-02-06T15:52:42.315ZApplied to files:
📚 Learning: 2026-03-31T02:12:43.093ZApplied to files:
📚 Learning: 2026-06-10T18:18:08.545ZApplied to files:
📚 Learning: 2026-06-10T18:18:09.253ZApplied to files:
🔇 Additional comments (16)
Summary by CodeRabbit
WalkthroughThe PR adds namespace-based ChangesDebug Namespace Mode
Windows Compatibility Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
LLxprt PR Review – PR #2052Issue AlignmentIssue #2051 reported three distinct problems on Windows:
PR #2052 addresses all three:
Side Effects
Code Quality
Overall: Correct and well-engineered. No obvious race conditions or data loss risks. Tests and CoverageCoverage impact: Increase New tests added:
Tests target actual behavior (file I/O, stream errors), not mock theater. The new migration tests and the tty error test directly validate the bug fixes described in #2051. VerdictReady — All three issues in #2051 are addressed with appropriate fixes and meaningful tests. The debug flag enhancement is a logical addition that improves developer experience. Changes are scoped to the diff with no unrelated modifications beyond a trivial whitespace fix. |
Author
If anyone has a running version on Windows, I'd appreciate if he told me how he has done it, because I couldn't even get it to work without this fix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/config/__tests__/profileBootstrap.test.ts (1)
166-179: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit tests for the new debug parsing contract (not just shape updates).
The suite now includes
debug: nullin fixtures, but it does not assert behavior for--debug,-d,--debug=<namespaces>, and false-like values (--debug=false). Add focusedparseBootstrapArgs()cases so bootstrap and runtime debug behavior stays locked.🤖 Prompt for 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. In `@packages/cli/src/config/__tests__/profileBootstrap.test.ts` around lines 166 - 179, The test fixture now includes the debug field but lacks explicit assertions on how parseBootstrapArgs() handles different debug flag formats. Add focused test cases for the parseBootstrapArgs() function that cover parsing behavior for `--debug`, `-d`, `--debug=<namespaces>`, and false-like values such as `--debug=false`. These tests should ensure the debug parsing contract is validated and the bootstrap and runtime debug behavior remains locked.
🤖 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 `@packages/cli/src/cli.tsx`:
- Around line 409-412: The namespaces string being split by comma in the
setCliConfig call for the ConfigurationManager.getInstance() method is not
sanitized, allowing whitespace around commas and empty tokens to pass through
and potentially cause silent failures in namespace matching. After splitting the
namespaces string by comma, trim whitespace from each resulting element and
filter out any empty strings before passing the cleaned array to the namespaces
property in setCliConfig.
In `@packages/cli/src/config/environmentLoader.ts`:
- Line 72: The debug mode check on line 72 using `!!argv.debug` converts the
debug argument to a boolean without considering that string values like "false"
or "0" are truthy in JavaScript, causing explicit disable flags to be treated as
enabled. Fix this by normalizing the argv.debug value before the double negation
to explicitly check for false-like string values (such as "false", "0", "no")
and convert them to actual boolean false before applying the `!!` operator, so
that `--debug=false` and `--debug=0` properly disable debug mode while still
allowing truthy string values to enable it.
In `@packages/storage/src/secure-store/secure-store.ts`:
- Around line 354-357: The secure store has two issues: First, replace the
encodeURIComponent call with a custom sanitizer function that escapes all
Windows-reserved characters (*, <, >, :, ", /, \, |, ?) to ensure cross-platform
filename safety. Second, implement dual-path fallback in the get(), has(), and
delete() methods (lines 644–652) to support legacy raw-key fallback files—these
methods should attempt to read from the encoded path first, and if that fails or
the file doesn't exist, fall back to trying the raw (unencoded) key path to
maintain backward compatibility with existing stored secrets.
---
Outside diff comments:
In `@packages/cli/src/config/__tests__/profileBootstrap.test.ts`:
- Around line 166-179: The test fixture now includes the debug field but lacks
explicit assertions on how parseBootstrapArgs() handles different debug flag
formats. Add focused test cases for the parseBootstrapArgs() function that cover
parsing behavior for `--debug`, `-d`, `--debug=<namespaces>`, and false-like
values such as `--debug=false`. These tests should ensure the debug parsing
contract is validated and the bootstrap and runtime debug behavior remains
locked.
🪄 Autofix (Beta)
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: 4194b198-930b-4ccd-be20-8e6c2041eb7d
📒 Files selected for processing (11)
docs/CONTRIBUTING.mdpackages/cli/src/cli.tsxpackages/cli/src/config/__tests__/profileBootstrap.test.tspackages/cli/src/config/cliArgParser.tspackages/cli/src/config/environmentLoader.tspackages/cli/src/config/profileBootstrap.tspackages/cli/src/config/yargsOptions.tspackages/cli/src/ui/utils/commandUtils.test.tspackages/cli/src/ui/utils/commandUtils.tspackages/storage/src/secure-store/secure-store.test.tspackages/storage/src/secure-store/secure-store.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/storage/src/secure-store/secure-store.tspackages/cli/src/ui/utils/commandUtils.tspackages/cli/src/ui/utils/commandUtils.test.tspackages/cli/src/config/environmentLoader.tspackages/storage/src/secure-store/secure-store.test.tspackages/cli/src/config/cliArgParser.tspackages/cli/src/config/yargsOptions.tspackages/cli/src/config/profileBootstrap.tspackages/cli/src/config/__tests__/profileBootstrap.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/storage/src/secure-store/secure-store.tspackages/cli/src/ui/utils/commandUtils.tspackages/cli/src/ui/utils/commandUtils.test.tspackages/cli/src/config/environmentLoader.tspackages/storage/src/secure-store/secure-store.test.tspackages/cli/src/config/cliArgParser.tspackages/cli/src/config/yargsOptions.tspackages/cli/src/config/profileBootstrap.tspackages/cli/src/config/__tests__/profileBootstrap.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/storage/src/secure-store/secure-store.tspackages/cli/src/ui/utils/commandUtils.tspackages/cli/src/ui/utils/commandUtils.test.tspackages/cli/src/config/environmentLoader.tspackages/storage/src/secure-store/secure-store.test.tspackages/cli/src/cli.tsxpackages/cli/src/config/cliArgParser.tspackages/cli/src/config/yargsOptions.tspackages/cli/src/config/profileBootstrap.tspackages/cli/src/config/__tests__/profileBootstrap.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/storage/src/secure-store/secure-store.tspackages/cli/src/ui/utils/commandUtils.tspackages/cli/src/ui/utils/commandUtils.test.tspackages/cli/src/config/environmentLoader.tspackages/storage/src/secure-store/secure-store.test.tspackages/cli/src/cli.tsxpackages/cli/src/config/cliArgParser.tspackages/cli/src/config/yargsOptions.tspackages/cli/src/config/profileBootstrap.tspackages/cli/src/config/__tests__/profileBootstrap.test.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/cli/src/config/__tests__/profileBootstrap.test.ts
🔇 Additional comments (8)
docs/CONTRIBUTING.md (1)
1-1: Clarify whether the symlink is intentional and configure Windows compatibility accordingly.The symlink to
../CONTRIBUTING.mdwas originally added upstream (commitcca41edcin gemini-cli) for documentation site purposes, but was marked SKIPPED in llxprt's merge audits. A few concerns:
- Intentionality unclear: The symlink appears in your repository, but was excluded from previous merges. Confirm whether it should be here.
- Windows Git configuration: No
core.symlinksis configured. On Windows, symlinks require elevated permissions or special Git settings; without configuration, clones may fail or convert the symlink to a regular file, creating inconsistencies.- No Windows CI coverage: The test matrix (
.github/workflows/ci.yml) runs onubuntu-latestandmacos-latestonly. Windows symlink behavior is untested.- Contradicts PR objectives: If this PR aims to improve Windows compatibility, a filesystem-level dependency on symlinks works against that goal.
Recommended action: Either remove the symlink and use a hardcopy of
CONTRIBUTING.mdindocs/, or document that symlink support is required and ensuregit config core.symlinks trueis set in Windows setup instructions. If keeping the symlink, add a Windows CI runner to test it.packages/cli/src/config/yargsOptions.ts (1)
146-151: LGTM!Also applies to: 199-204
packages/cli/src/config/cliArgParser.ts (1)
35-35: LGTM!Also applies to: 156-156
packages/cli/src/config/profileBootstrap.ts (1)
190-193: Verify intended--debugbehavior with positional prompt text.Line 192 uses the consumed next token as the debug value when present, so
llxprt --debug hellotreatshelloas debug namespaces (not prompt text). Please confirm this UX is intentional for backward compatibility with positional prompts.packages/cli/src/ui/utils/commandUtils.ts (1)
98-100: LGTM!packages/cli/src/ui/utils/commandUtils.test.ts (1)
420-437: LGTM!packages/storage/src/secure-store/secure-store.ts (1)
747-760: LGTM!packages/storage/src/secure-store/secure-store.test.ts (1)
396-413: LGTM!Also applies to: 694-699
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Collaborator
|
@HumanBot000 run npm run format before checkin |
Author
Sry for that. Will do that asap |
Collaborator
|
@HumanBot000 still failing lint (did you run npm run lint?) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
PR addresses onboarding problems for windows users as referenced in #2051
Dive Deeper
gemini:default.encto store OAuth authentication. On Windows, the colon:in filenames is exclusively reserved for drives./dev/ttyis only available on Unix-based systems. This is already checked insidepickTty()But under special circumstances, including a special setup of bash and docker on Windows, this check would fail (see Error: ENOENT: no such file or directory, open 'C:\dev\tty' #2051)Testing Matrix
Linked issues / bugs
Fixes #2051