Improve Windows CI validation speed and package local updates#17
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds cross-platform compatibility documentation and references; introduces CI profile orchestration; normalizes text I/O; centralizes MemPalace guidance; implements managed MemPalace post-commit git-hook and Python runner; makes platform-aware CLI/editor invocation; and adds platform-safety and integration tests. ChangesCross-Platform Compatibility & MemPalace Hooks
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/editor.ts (1)
44-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently swallow editor launch failures.
The empty catch drops actionable failure context on an async subprocess path. Either propagate or wrap with a clear error.
Proposed fix
export async function openInEditor( platform: Platform, filePath: string, hostPlatform: NodeJS.Platform = process.platform, ): Promise<void> { @@ - } catch { - // Editor open failed — non-fatal, file was still written + } catch (error) { + throw new Error( + `Failed to launch editor for "${filePath}": ${error instanceof Error ? error.message : String(error)}`, + ); } }As per coding guidelines, "Do not swallow async failures. Propagate or convert them with actionable context".
🤖 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 `@src/utils/editor.ts` around lines 44 - 64, The openInEditor function currently swallows all errors in its try/catch; change the catch to capture the thrown error (e.g. catch (err)) and either rethrow it or throw a new Error that wraps the original with actionable context including the target filePath and which invocation failed (reference editor, or the result from resolveDefaultEditorInvocation). Update the catch block around platform.exec in openInEditor to include the original error (err) and a clear message indicating the editor launch failed for filePath and which command/args were used (use invocation.command/invocation.args or tokens/cmd/args as appropriate) before rethrowing.
🧹 Nitpick comments (3)
.agents/skills/cross-platform-compatibility/references/shell-commands.md (1)
14-24: 💤 Low valueConsider whether stderr should always be logged as an error.
Many command-line tools write informational messages (warnings, progress) to stderr even on success. Unconditionally logging stderr to
console.errormay be misleading. Consider logging stderr only if the command fails, or using a different log level.🤖 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 @.agents/skills/cross-platform-compatibility/references/shell-commands.md around lines 14 - 24, The current static async execute(command: string) unconditionally calls console.error(stderr) even when the command succeeds; modify Execute's error handling so stderr is not always logged as an error: remove the unconditional console.error(stderr) and instead capture stderr and only log it on failure inside the catch block (include stderr in the thrown Error or processLogger.error), or log it at a lower level (console.warn or console.info) when the command resolves but stderr is non-empty; update references to execAsync and this.getShell() usage to preserve stdout return behavior (return stdout.trim()) and ensure any logged stderr includes the command context for the static async execute method..agents/skills/cross-platform-compatibility/references/process-management.md (1)
11-18: ⚡ Quick winHandle errors from the
taskkillspawn on Windows.The spawned
taskkillprocess on line 14 is not awaited or error-handled. If the kill fails (e.g., process not found, permission denied), this will silently fail.♻️ Proposed fix to handle taskkill errors
static kill(pid: number, signal?: string): void { if (process.platform === "win32") { // Windows doesn't support signals, use taskkill - spawn("taskkill", ["/pid", pid.toString(), "/f", "/t"]); + const child = spawn("taskkill", ["/pid", pid.toString(), "/f", "/t"]); + child.on("error", (err) => { + throw new Error(`Failed to kill process ${pid}: ${err.message}`); + }); } else { process.kill(pid, signal || "SIGTERM"); } }🤖 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 @.agents/skills/cross-platform-compatibility/references/process-management.md around lines 11 - 18, The Windows branch of static kill does not handle or await the spawned taskkill, so failures are silent; update the static kill(pid: number, signal?: string) implementation to capture the child process returned by spawn("taskkill", [...]) and either (a) use spawnSync for a synchronous call or (b) attach 'error' and 'close'/'exit' listeners (or await a promisified wrapper) to check the exit code and error, then surface failures (log via the appropriate logger or throw a descriptive Error) when taskkill fails (e.g., permission denied or process not found); reference the static kill method and the spawn("taskkill", ...) call when making the change..agents/skills/cross-platform-compatibility/references/platform-specific-dependencies.md (1)
34-47: ⚡ Quick winWrap the
chokidarfallback in try/catch for robustness.The
chokidarrequire on line 46 can throw if the package isn't installed. Since this example demonstrates graceful fallback patterns, the fallback itself should also handle missing dependencies to avoid defeating the purpose.♻️ Proposed fix to handle missing chokidar
export function useFSEvents() { try { // fsevents is macOS only if (process.platform === "darwin") { const fsevents = require("fsevents"); return fsevents; } } catch (error) { console.warn("fsevents not available, using fallback"); } - // Fallback to chokidar or fs.watch - return require("chokidar"); + // Fallback to chokidar or fs.watch + try { + return require("chokidar"); + } catch (error) { + console.warn("chokidar not available, using fs.watch"); + return require("fs").watch; + } }🤖 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 @.agents/skills/cross-platform-compatibility/references/platform-specific-dependencies.md around lines 34 - 47, The fallback require("chokidar") in useFSEvents should be wrapped in a try/catch so missing-chokidar doesn't throw; inside useFSEvents attempt to require("chokidar") in a try block and return it if present, and in the catch log a warning (consistent with the existing console.warn) and return a safe alternative (e.g., the built-in fs.watch wrapper or null) so callers still receive a graceful fallback instead of an exception.
🤖 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
@.agents/skills/cross-platform-compatibility/references/build-configuration.md:
- Around line 5-31: The build config currently uses the replace plugin to inline
process.platform (the replace call replacing "process.platform" with
JSON.stringify(process.platform)) which bakes in the build machine's platform
and breaks multi-platform bundles, and the plugin is not imported; update the
config so either (A) remove or avoid using the replace() substitution for
process.platform when producing a single cross-platform bundle and instead rely
on runtime platform checks or mark platform-specific modules (like "fsevents")
as external, or (B) if you intend platform-specific artifacts, explicitly import
replace from "`@rollup/plugin-replace`" and add a clear comment next to the
replace(...) block stating this produces a platform-specific build and must be
run per-target; ensure the code path references the replace symbol and the
externalization of "fsevents" is preserved.
In
@.agents/skills/cross-platform-compatibility/references/character-encoding.md:
- Around line 5-58: The file defines EncodingUtils (methods readFile, writeFile,
detectEncoding) but uses fs.readFileSync and fs.writeFileSync without importing
fs; add a top-level import for the fs module (e.g., import fs from "fs" or const
fs = require("fs")) so all uses of fs in EncodingUtils resolve, keeping the rest
of the implementation unchanged.
In
@.agents/skills/cross-platform-compatibility/references/process-management.md:
- Around line 37-52: The example uses execAsync in static async findProcess but
never imports or defines it; add a proper import or local wrapper for execAsync
(for example import { exec } from 'child_process' then promisify it or import
exec from 'child_process/promises' as execAsync) so findProcess can call
execAsync without runtime errors—update the top of the snippet to include the
execAsync definition/import referenced by findProcess.
In
@.agents/skills/cross-platform-compatibility/references/testing-across-platforms.md:
- Around line 52-66: The test uses Node built-ins path and os but never imports
them; add top-of-file imports for the path and os modules so path.join and
os.homedir() work (alongside the existing Platform import used in the
describe/it blocks), e.g. import path and import os at the top of the test file
where Platform is imported.
In @.agents/skills/cross-platform-compatibility/scripts/scaffold-tests.sh:
- Around line 7-8: The parameter expansion in scaffold-tests.sh uses invalid
double-brace syntax (SOURCE_FILE="${{1:?Usage: $0 <source_file> [--framework
jest|pytest|mocha]}}" and FRAMEWORK="${{2:-jest}}"); change both to standard
Bash single-brace expansions (use SOURCE_FILE="${1:?Usage: ...}" and
FRAMEWORK="${2:-jest}") so that the variables SOURCE_FILE and FRAMEWORK are
correctly expanded at runtime.
In `@src/context-mode/sandbox/executor.ts`:
- Around line 24-25: The constant MISSING_BASH_MESSAGE currently hardcodes
Windows-specific remediation; change it to be platform-aware by building the
message using process.platform (or a small helper like getMissingBashMessage) so
it suggests "Install Git for Windows or WSL" for 'win32', "Install Bash via
Homebrew (brew install bash) or enable Terminal" for 'darwin', and a generic
"install bash via your distro's package manager (apt/yum/pacman)" for 'linux'
(and a fallback generic instruction for other platforms); replace usages of
MISSING_BASH_MESSAGE with the new platform-aware constant or function.
---
Outside diff comments:
In `@src/utils/editor.ts`:
- Around line 44-64: The openInEditor function currently swallows all errors in
its try/catch; change the catch to capture the thrown error (e.g. catch (err))
and either rethrow it or throw a new Error that wraps the original with
actionable context including the target filePath and which invocation failed
(reference editor, or the result from resolveDefaultEditorInvocation). Update
the catch block around platform.exec in openInEditor to include the original
error (err) and a clear message indicating the editor launch failed for filePath
and which command/args were used (use invocation.command/invocation.args or
tokens/cmd/args as appropriate) before rethrowing.
---
Nitpick comments:
In
@.agents/skills/cross-platform-compatibility/references/platform-specific-dependencies.md:
- Around line 34-47: The fallback require("chokidar") in useFSEvents should be
wrapped in a try/catch so missing-chokidar doesn't throw; inside useFSEvents
attempt to require("chokidar") in a try block and return it if present, and in
the catch log a warning (consistent with the existing console.warn) and return a
safe alternative (e.g., the built-in fs.watch wrapper or null) so callers still
receive a graceful fallback instead of an exception.
In
@.agents/skills/cross-platform-compatibility/references/process-management.md:
- Around line 11-18: The Windows branch of static kill does not handle or await
the spawned taskkill, so failures are silent; update the static kill(pid:
number, signal?: string) implementation to capture the child process returned by
spawn("taskkill", [...]) and either (a) use spawnSync for a synchronous call or
(b) attach 'error' and 'close'/'exit' listeners (or await a promisified wrapper)
to check the exit code and error, then surface failures (log via the appropriate
logger or throw a descriptive Error) when taskkill fails (e.g., permission
denied or process not found); reference the static kill method and the
spawn("taskkill", ...) call when making the change.
In @.agents/skills/cross-platform-compatibility/references/shell-commands.md:
- Around line 14-24: The current static async execute(command: string)
unconditionally calls console.error(stderr) even when the command succeeds;
modify Execute's error handling so stderr is not always logged as an error:
remove the unconditional console.error(stderr) and instead capture stderr and
only log it on failure inside the catch block (include stderr in the thrown
Error or processLogger.error), or log it at a lower level (console.warn or
console.info) when the command resolves but stderr is non-empty; update
references to execAsync and this.getShell() usage to preserve stdout return
behavior (return stdout.trim()) and ensure any logged stderr includes the
command context for the static async execute method.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c11339e4-b21e-4637-a115-6380786c32e8
📒 Files selected for processing (52)
.agents/skills/cross-platform-compatibility/SKILL.md.agents/skills/cross-platform-compatibility/references/build-configuration.md.agents/skills/cross-platform-compatibility/references/character-encoding.md.agents/skills/cross-platform-compatibility/references/environment-variables.md.agents/skills/cross-platform-compatibility/references/file-path-handling.md.agents/skills/cross-platform-compatibility/references/file-permissions.md.agents/skills/cross-platform-compatibility/references/line-endings.md.agents/skills/cross-platform-compatibility/references/platform-detection.md.agents/skills/cross-platform-compatibility/references/platform-specific-dependencies.md.agents/skills/cross-platform-compatibility/references/process-management.md.agents/skills/cross-platform-compatibility/references/shell-commands.md.agents/skills/cross-platform-compatibility/references/testing-across-platforms.md.agents/skills/cross-platform-compatibility/scripts/scaffold-tests.sh.agents/skills/cross-platform-compatibility/templates/test-template.js.github/workflows/ci.ymlpackage.jsonskills-lock.jsonsrc/ci.tssrc/commands/memory.tssrc/config/defaults.tssrc/config/schema.tssrc/context-mode/sandbox/executor.tssrc/fix-pr/scripts/exec.tssrc/harness/stages/implement-apply.tssrc/harness/storage.tssrc/mempalace/contract.tssrc/mempalace/git-hook.tssrc/mempalace/hooks.tssrc/mempalace/tool.tssrc/mempalace/uv.tssrc/storage/plans.tssrc/text.tssrc/types.tssrc/utils/editor.tstests/commands/memory.test.tstests/context-mode/sandbox/executor.test.tstests/fix-pr/scripts/exec.test.tstests/harness/stages/implement-apply.test.tstests/harness/storage.test.tstests/mempalace/config.test.tstests/mempalace/git-hook.test.tstests/mempalace/hooks.test.tstests/mempalace/tool.test.tstests/mempalace/uv.test.tstests/platform/ci-matrix.test.tstests/platform/ci-plan.test.tstests/platform/context-storage-smoke.test.tstests/platform/paths.test.tstests/platform/static-hazards.test.tstests/storage/plan-artifacts.test.tstests/text.test.tstests/utils/editor.test.ts
- build-configuration.md: import @rollup/plugin-replace and warn
that the substitution bakes in build-host platform
- character-encoding.md: add missing fs import
- process-management.md: define execAsync via promisify(exec)
- testing-across-platforms.md: import path and os
- scaffold-tests.sh: fix invalid ${{...}} parameter expansions
- context-mode/sandbox/executor.ts: make MISSING_BASH_MESSAGE
platform-aware (Homebrew on darwin, distro package manager on
linux, Git for Windows/WSL on win32); update test to match
Owner
Author
|
/review |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.agents/skills/cross-platform-compatibility/references/process-management.md (1)
24-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid nested shell execution in
spawnCommand.Using
cmd /candshell: truein Line 27–Line 30 creates double-shell behavior and brittle argument escaping. Prefer one strategy: directspawnwithshell: falsefor arg-safe execution.Proposed fix
static spawnCommand(command: string, args: string[] = []): ChildProcess { if (process.platform === "win32") { - // Windows requires cmd.exe to run commands - return spawn("cmd", ["/c", command, ...args], { + // Use direct process spawn for safer argument handling + return spawn(command, args, { stdio: "inherit", - shell: true, + shell: false, }); } return spawn(command, args, { stdio: "inherit", - shell: true, + shell: false, }); }🤖 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 @.agents/skills/cross-platform-compatibility/references/process-management.md around lines 24 - 36, The spawnCommand implementation currently uses both cmd /c and shell: true causing double-shell and fragile escaping; update spawnCommand to avoid nested shells by removing shell: true and relying on direct spawn with shell:false (or omitted) so Windows still uses spawn("cmd", ["/c", command, ...args], { stdio: "inherit" }) and non-Windows uses spawn(command, args, { stdio: "inherit" }); reference the spawnCommand function, the spawn calls, and the process.platform === "win32" branch when making this change.
🤖 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
@.agents/skills/cross-platform-compatibility/references/process-management.md:
- Around line 52-53: The call to execAsync(`pgrep ${name}`) can reject with exit
code 1 when no process matches, causing findProcess to throw instead of
returning []; wrap the execAsync call in a try/catch inside the findProcess
implementation (or the function that contains the shown lines), and in the catch
if the error indicates exit code === 1 (or stdout/stderr shows "no match")
return an empty array; for any other error rethrow it. Also ensure the
successful path still splits stdout, filters falsy lines and maps to Number as
before.
- Around line 40-53: The findProcess function currently interpolates the
user-supplied name into shell commands causing command injection; change both
branches to use argument-safe process execution (e.g., spawn/spawnSync or
execFile) instead of a shell command: call tasklist with args (e.g., [" /FI",
`IMAGENAME eq ${name}`] as separate argv) on Windows and call pgrep with the
process name as a single argv on POSIX, then parse the resulting stdout the same
way; ensure you update references in static async findProcess(name: string) and
remove direct string interpolation into execAsync so the name is passed as an
argv not a shell string.
---
Duplicate comments:
In
@.agents/skills/cross-platform-compatibility/references/process-management.md:
- Around line 24-36: The spawnCommand implementation currently uses both cmd /c
and shell: true causing double-shell and fragile escaping; update spawnCommand
to avoid nested shells by removing shell: true and relying on direct spawn with
shell:false (or omitted) so Windows still uses spawn("cmd", ["/c", command,
...args], { stdio: "inherit" }) and non-Windows uses spawn(command, args, {
stdio: "inherit" }); reference the spawnCommand function, the spawn calls, and
the process.platform === "win32" branch when making this change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aad25dca-18d4-4f6a-a4f2-d5bba08de41e
📒 Files selected for processing (7)
.agents/skills/cross-platform-compatibility/references/build-configuration.md.agents/skills/cross-platform-compatibility/references/character-encoding.md.agents/skills/cross-platform-compatibility/references/process-management.md.agents/skills/cross-platform-compatibility/references/testing-across-platforms.md.agents/skills/cross-platform-compatibility/scripts/scaffold-tests.shsrc/context-mode/sandbox/executor.tstests/context-mode/sandbox/executor.test.ts
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/cross-platform-compatibility/references/build-configuration.md
- .agents/skills/cross-platform-compatibility/references/testing-across-platforms.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .agents/skills/cross-platform-compatibility/scripts/scaffold-tests.sh
- .agents/skills/cross-platform-compatibility/references/character-encoding.md
- src/context-mode/sandbox/executor.ts
- tests/context-mode/sandbox/executor.test.ts
process-management.md teaching example: - Switch from exec to execFile for argv-safe subprocess invocation - Validate process names against [\w.\-]+ as defense-in-depth - Handle pgrep exit code 1 (no match) as empty result rather than throw
Owner
Author
|
/review |
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
1 participant
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.
Summary
Verification
bun test --timeout 60000 tests/platform/ci-matrix.test.ts tests/platform/ci-plan.test.tsbun run test:windowsSUPIPOWERS_CI_PROFILE=windows-fast bun run cibun run cibun run buildSummary by CodeRabbit
New Features
Improvements
Tests