Fix global state pollution and robustify PHP LSP resolution on Windows#327
Fix global state pollution and robustify PHP LSP resolution on Windows#327brovatten wants to merge 2 commits into
Conversation
Resolves the integration-tests (windows-latest, php) failure on main. - tests/test_vscode_constants.py: Added setUp/tearDown to restore global VSCODE_CONFIG and prevent test pollution. - vscode_constants.py: Refactored update_command_paths to avoid in-place modification and added defensive node prefixing. - tool_registry/manifest.py: Refactored resolve_config to rebuild commands cleanly, preventing duplicate 'node' arguments. - vscode_constants.py: Made find_runnable case-insensitive on Windows.
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
📝 WalkthroughWalkthroughRefactors command resolution to avoid in-place mutations, adds safer null checks and Windows case-insensitive matching for runnable detection, adjusts AWS Bedrock model identifiers, and makes tests isolate and restore the shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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. Review rate limit: 4/8 reviews remaining, refill in 24 minutes and 7 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tool_registry/manifest.py`:
- Around line 241-246: Check that original_cmd is non-empty and robustly detect
a node binary by comparing os.path.basename(original_cmd[0]).lower() (allowing
'node' or 'node.exe') before slicing; if the basename matches a node executable
and len(original_cmd) > 1, set flags = original_cmd[2:], otherwise set flags =
original_cmd[1:] (and ensure flags defaults to [] when original_cmd has length 1
or 0) so section["command"] = [node_path, js_entry] + flags cannot crash or
produce malformed arguments.
In `@vscode_constants.py`:
- Around line 24-29: The loop currently checks for the presence of "command" but
then assumes it's non-empty when creating cmd and later accessing cmd[0]. Add an
empty-list guard immediately after the existing key guard (e.g., after if
"command" not in value: continue) to skip entries where value["command"] is
falsy or has length 0 before creating cmd or calling cmd[0].lower(); reference
the variables value and cmd and the cmd[0].lower() access so you avoid
IndexError (also apply the same guard to the similar block around the other
occurrence covering lines 64–68).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bed05a5-2d78-4372-9255-398b6f0cd52a
📒 Files selected for processing (3)
tests/test_vscode_constants.pytool_registry/manifest.pyvscode_constants.py
| flags = original_cmd[1:] | ||
| if original_cmd[0] == "node" and len(original_cmd) > 1: | ||
| # If it was already prefixed with node, flags start at index 2 | ||
| flags = original_cmd[2:] | ||
|
|
||
| section["command"] = [node_path, js_entry] + flags |
There was a problem hiding this comment.
Harden node-prefix detection before slicing flags.
Line 242 only matches literal "node" and dereferences original_cmd[0] unguarded. If the command was previously prefixed with an absolute node path (or becomes empty from polluted state), rebuild can produce malformed args or crash.
Proposed fix
- flags = original_cmd[1:]
- if original_cmd[0] == "node" and len(original_cmd) > 1:
- # If it was already prefixed with node, flags start at index 2
- flags = original_cmd[2:]
+ flags_start = 1
+ if original_cmd:
+ first_cmd_lower = str(original_cmd[0]).lower()
+ is_node = (
+ first_cmd_lower == "node"
+ or first_cmd_lower.endswith("\\node.exe")
+ or first_cmd_lower.endswith("/node.exe")
+ or first_cmd_lower.endswith("\\node")
+ or first_cmd_lower.endswith("/node")
+ )
+ if is_node:
+ flags_start = 2
+ flags = original_cmd[flags_start:]🧰 Tools
🪛 Ruff (0.15.12)
[warning] 246-246: Consider [node_path, js_entry, *flags] instead of concatenation
Replace with [node_path, js_entry, *flags]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tool_registry/manifest.py` around lines 241 - 246, Check that original_cmd is
non-empty and robustly detect a node binary by comparing
os.path.basename(original_cmd[0]).lower() (allowing 'node' or 'node.exe') before
slicing; if the basename matches a node executable and len(original_cmd) > 1,
set flags = original_cmd[2:], otherwise set flags = original_cmd[1:] (and ensure
flags defaults to [] when original_cmd has length 1 or 0) so section["command"]
= [node_path, js_entry] + flags cannot crash or produce malformed arguments.
| if "command" not in value: | ||
| continue | ||
|
|
||
| # Create a new list to avoid in-place modification of the original | ||
| cmd = list(value["command"]) | ||
|
|
There was a problem hiding this comment.
Guard empty command lists before cmd[0] access.
If a config entry has "command": [], Line 67 (cmd[0].lower()) raises IndexError. Add an early empty-list guard alongside the missing-key guard.
Proposed fix
- cmd = list(value["command"])
+ cmd = list(value["command"])
+ if not cmd:
+ continueAlso applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vscode_constants.py` around lines 24 - 29, The loop currently checks for the
presence of "command" but then assumes it's non-empty when creating cmd and
later accessing cmd[0]. Add an empty-list guard immediately after the existing
key guard (e.g., after if "command" not in value: continue) to skip entries
where value["command"] is falsy or has length 0 before creating cmd or calling
cmd[0].lower(); reference the variables value and cmd and the cmd[0].lower()
access so you avoid IndexError (also apply the same guard to the similar block
around the other occurrence covering lines 64–68).
Switch the AWS provider's agent/parsing model IDs to the us.* inference-profile forms, which is what Bedrock requires in us-east-1 for Claude Sonnet 4.6 / Haiku 4.5 (calling the bare anthropic.* IDs returns ValidationException: invalid model identifier). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/llm_config.py`:
- Around line 164-165: The parsing_model value currently set as
parsing_model="us.anthropic.claude-haiku-4-5" is not the valid Bedrock Geo
inference ID; update the parsing_model constant to the full Bedrock Geo
inference ID for Claude Haiku 4.5 (include the version and reference suffix the
Converse API expects) so AWS initialization succeeds. Locate the parsing_model
assignment in agents/llm_config.py and replace the short name with the published
Geo inference ID string (keep agent_model unchanged). Ensure the new string
matches the exact format used by Bedrock (version + reference suffix) before
committing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| agent_model="us.anthropic.claude-sonnet-4-6", | ||
| parsing_model="us.anthropic.claude-haiku-4-5", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the official AWS Bedrock docs for "Claude Haiku 4.5", what is the exact US Geo inference model ID for the Converse API?
💡 Result:
The exact US Geo inference model ID for Claude Haiku 4.5 in the official AWS Bedrock documentation, for use with the Converse API, is us.anthropic.claude-haiku-4-5-20251001-v1:0.
Citations:
Use the full Bedrock Geo inference ID for Claude Haiku 4.5 (Line 165).
us.anthropic.claude-haiku-4-5 is not a valid Bedrock model ID for the Converse API; the published Geo ID must include the version and reference suffix. This will cause AWS parsing LLM initialization to fail at runtime.
Proposed fix
- parsing_model="us.anthropic.claude-haiku-4-5",
+ parsing_model="us.anthropic.claude-haiku-4-5-20251001-v1:0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent_model="us.anthropic.claude-sonnet-4-6", | |
| parsing_model="us.anthropic.claude-haiku-4-5", | |
| agent_model="us.anthropic.claude-sonnet-4-6", | |
| parsing_model="us.anthropic.claude-haiku-4-5-20251001-v1:0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agents/llm_config.py` around lines 164 - 165, The parsing_model value
currently set as parsing_model="us.anthropic.claude-haiku-4-5" is not the valid
Bedrock Geo inference ID; update the parsing_model constant to the full Bedrock
Geo inference ID for Claude Haiku 4.5 (include the version and reference suffix
the Converse API expects) so AWS initialization succeeds. Locate the
parsing_model assignment in agents/llm_config.py and replace the short name with
the published Geo inference ID string (keep agent_model unchanged). Ensure the
new string matches the exact format used by Bedrock (version + reference suffix)
before committing.
| if original_cmd[0] == "node" and len(original_cmd) > 1: | ||
| # If it was already prefixed with node, flags start at index 2 | ||
| flags = original_cmd[2:] |
There was a problem hiding this comment.
🔴 Node prefix detection in resolve_config only matches literal "node", not absolute paths
The check original_cmd[0] == "node" at tool_registry/manifest.py:242 only matches the literal string "node", but not absolute paths like "C:\Users\...\node.exe" or "/usr/local/bin/node". This is inconsistent with the comprehensive check in vscode_constants.py:68-74, which correctly handles absolute paths.
When the VSCode extension path is used (binary_location set), update_command_paths (vscode_constants.py:76) prepends CODEBOARDING_NODE_PATH (typically an absolute path on Windows) to the global VSCODE_CONFIG. Later, resolve_config deep-copies this already-modified config (tool_registry/manifest.py:209). Because original_cmd[0] is now e.g. "C:\VSCode\node.exe" instead of "node", the check fails and flags = original_cmd[1:] incorrectly includes the old JS entry file path. The resulting command becomes [new_node, new_js_entry, old_js_entry, "--stdio", ...] — the spurious old entry is passed as an argument to the LSP server, likely causing startup failure.
Example of polluted command
Original VSCODE_CONFIG after update_command_paths on Windows:
["C:\\VSCode\\node.exe", "/old/path/cli.mjs", "--stdio", "--log-level=2"]
After resolve_config NODE case:
[preferred_node, new_cli_mjs, "/old/path/cli.mjs", "--stdio", "--log-level=2"]
The old cli.mjs path leaks as a spurious argument.
Prompt for agents
The check `original_cmd[0] == "node"` at tool_registry/manifest.py:242 needs to be broadened to also match absolute paths to node binaries, consistent with the comprehensive check already implemented in vscode_constants.py:68-74. The check should detect whether original_cmd[0] is any form of a node binary path (literal "node", or paths ending in /node, \node, /node.exe, \node.exe).
Additionally, the same node-prefix-stripping logic needs to be applied in the fallback branches at lines 248 and 250 (the `else` branches), because those also use `original_cmd[1:]` which would include the old JS entry if a node prefix was present.
Consider extracting a helper function (e.g. `_is_node_binary(path: str) -> bool`) that can be shared between vscode_constants.py and tool_registry/manifest.py to avoid duplicating the detection logic. Alternatively, compute `flags` once before the `if js_entry and node_path` branch and reuse it in all branches.
Was this helpful? React with 👍 or 👎 to provide feedback.
TLDR: Was tired of the PHP test failing so tried to fix it.
Fixed a global state pollution bug where unit tests were permanently poisoning the shared LSP configuration, causing subsequent integration tests on Windows to fail. I refactored the command resolution logic to be idempotent and defensive, preventing broken commands like "node node" from being generated when the resolution runs multiple times. Finally, I made the file discovery utility case-insensitive on Windows to handle inconsistent directory casing that previously led to resolution failures
Summary by CodeRabbit
Bug Fixes
Tests
Chores