fix/windows ci validation profile#19
Merged
Merged
Conversation
- 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
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
- Broadens isPlanApprovalResolveInput to match any action==='apply' object regardless of extra.title shape - Closes gap where malformed extra.title (object, missing, null) bypassed the native resolve guard - Adds unit tests for string title, object title, missing extra, missing title, and non-apply actions
- Adds a getDoctorRecommendations entry advising users to upgrade OMP to at least 15.1.7 - Covers the new tip with a focused unit test
… README - Updates skills/context-mode/SKILL.md with OMP version floor - Adds OMP >=15.1.7 compatibility note to README
- Bumps lastAnalyzedVersion to 15.1.7 and supipowersVersionAtLastAudit to 2.2.2 - Replaces audit prose covering 15.1.0-15.1.3 with findings for 15.1.3-15.1.7 - Documents two tracked compatibility risks and six no-impact entries with evidence citations
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPR consolidates four independent functional changes: an OMP version audit bump from 15.1.3 to 15.1.7 with updated compatibility analysis and doctor recommendations; new cross-platform skill reference documentation for platform-specific dependencies, process management, and shell utilities with static validation; a complete MemPalace git post-commit hook management system for repository indexing; and a simplification of the planning-mode resolve-apply guard predicate. ChangesOMP Version Audit and Doctor Recommendations
Cross-Platform Skill References and Validation
MemPalace Git Hook Management
Planning Mode Resolve Guard Simplification
Context-Mode Skill Documentation
Sequence Diagram(s)sequenceDiagram
participant PlanningSession
participant registerPlanningAskToolGuard
participant isResolveApplyInput
participant GuardReason
PlanningSession->>registerPlanningAskToolGuard: tool_call event for resolve
registerPlanningAskToolGuard->>isResolveApplyInput: Check if input.action === "apply"
alt action is "apply"
isResolveApplyInput-->>registerPlanningAskToolGuard: true
registerPlanningAskToolGuard->>GuardReason: Emit "Native OMP plan approval" reason
GuardReason-->>registerPlanningAskToolGuard: Block tool_call
else action is not "apply" (e.g., "discard", or missing)
isResolveApplyInput-->>registerPlanningAskToolGuard: false
registerPlanningAskToolGuard-->>PlanningSession: Allow tool_call
end
registerPlanningAskToolGuard-->>PlanningSession: Blocked or allowed
sequenceDiagram
participant Caller
participant InstallMempalacePostCommitHook
participant resolveHookContext
participant Git as git rev-parse
participant FileOps as Filesystem
participant buildPostCommitHookScript
participant buildReindexRunnerScript
Caller->>InstallMempalacePostCommitHook: installMempalacePostCommitHook(options)
InstallMempalacePostCommitHook->>resolveHookContext: Resolve repo root & hook dir
resolveHookContext->>Git: git rev-parse --show-toplevel
resolveHookContext->>Git: git config core.hooksPath (or .git/hooks default)
InstallMempalacePostCommitHook->>FileOps: Read existing post-commit
FileOps-->>InstallMempalacePostCommitHook: Existing hook content or null
InstallMempalacePostCommitHook->>buildPostCommitHookScript: Generate managed hook script
buildPostCommitHookScript-->>InstallMempalacePostCommitHook: Shell script (chains .user hook)
InstallMempalacePostCommitHook->>buildReindexRunnerScript: Generate Python reindex runner
buildReindexRunnerScript-->>InstallMempalacePostCommitHook: Python script (git diff-tree→bridge)
InstallMempalacePostCommitHook->>FileOps: Write/update post-commit and runner
FileOps-->>InstallMempalacePostCommitHook: installed|already-installed|upgraded|chained-user-hook
InstallMempalacePostCommitHook-->>Caller: MempalacePostCommitHookInstallResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 skipped: no ESLint configuration detected in root package.json. To enable, add 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: 7
🧹 Nitpick comments (4)
tests/harness/stages/implement-apply.test.ts (1)
257-266: ⚡ Quick winTighten
shell: bashassertion to theverify-pr-sourcejob.Current check is global and may pass even if
verify-pr-sourceloses the shell override.Suggested test hardening
- expect(workflow).toContain("shell: bash"); + expect(workflow).toMatch(/verify-pr-source:[\s\S]*?shell:\s*bash/);🤖 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 `@tests/harness/stages/implement-apply.test.ts` around lines 257 - 266, The global assertion for "shell: bash" is too loose; modify the test in tests/harness/stages/implement-apply.test.ts so it verifies the shell override specifically within the verify-pr-source job. Instead of the broad expect(workflow).toContain("shell: bash"), parse the workflow YAML (e.g., using js-yaml) or use a regex to locate the "verify-pr-source" job block (look for "verify-pr-source" or "verify-pr-source:" in the workflow string) and assert that that job's steps include a shell entry equal to "bash" (or that the job block contains "shell: bash"), referencing the workflow variable and the "verify-pr-source" identifier in your change..agents/skills/cross-platform-compatibility/references/file-permissions.md (1)
35-41: ⚡ Quick winDocument that
modeonly applies on Unix-like systems.The
modeparameter infs.writeFileSyncis ignored on Windows. Consider adding a comment or documentation note thatcreateWithPermissionseffectively only sets permissions on Unix-like platforms.📝 Suggested documentation enhancement
- // Create file with specific permissions (Unix) + // Create file with specific permissions (Unix only - mode is ignored on Windows) static createWithPermissions( filepath: string, content: string, mode: number = 0o644, ): void { fs.writeFileSync(filepath, content, { mode }); }🤖 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/file-permissions.md around lines 35 - 41, Update the documentation/comment for createWithPermissions to state that the mode argument passed to fs.writeFileSync only affects Unix-like systems (Windows ignores file mode), and annotate the createWithPermissions(filepath, content, mode) implementation accordingly; mention that on Windows the function will write the file but not modify permissions, and optionally suggest using platform-specific APIs (or a no-op) if callers need explicit Windows ACL handling..agents/skills/cross-platform-compatibility/references/testing-across-platforms.md (2)
36-46: ⚡ Quick winDuplicate step names reduce workflow clarity.
All three platform-specific test steps share the name "Platform-specific tests". While only one runs per matrix job, this makes the workflow definition harder to read and maintain.
♻️ Suggested improvement
- - name: Platform-specific tests + - name: Run Windows-specific tests if: runner.os == 'Windows' run: npm run test:windows - - name: Platform-specific tests + - name: Run macOS-specific tests if: runner.os == 'macOS' run: npm run test:macos - - name: Platform-specific tests + - name: Run Linux-specific tests if: runner.os == 'Linux' run: npm run test:linux🤖 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/testing-across-platforms.md around lines 36 - 46, The three workflow steps all use the identical step name "Platform-specific tests", which reduces clarity; update each step to a unique, descriptive name such as "Windows platform tests", "macOS platform tests", and "Linux platform tests" (these are the step name values in the YAML for the three platform-specific steps) so the job matrix and logs are explicit and easier to maintain.
70-90: 💤 Low valueTest API syntax may not be framework-portable.
The
describe.skipIf()syntax (lines 70, 80) is not standard in Jest or Mocha. This appears to be Vitest or Bun test syntax. Developers using other frameworks would need to adapt this pattern.📝 Framework-agnostic alternative
For broader compatibility, consider documenting the Jest equivalent:
// Framework-agnostic conditional test suites (Platform.isWindows ? describe.skip : describe)("Unix-only tests", () => { it("should work with symlinks", () => { // Symlink tests }); it("should handle file permissions", () => { // Permission tests }); }); (!Platform.isWindows ? describe.skip : describe)("Windows-only tests", () => { it("should work with UNC paths", () => { // UNC path tests }); it("should handle drive letters", () => { // Drive letter tests }); });Or add a note indicating this is Vitest/Bun syntax.
🤖 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/testing-across-platforms.md around lines 70 - 90, The current use of describe.skipIf (e.g., describe.skipIf and Platform.isWindows) is not portable across Jest/Mocha; replace those calls with a framework-agnostic conditional describe expression (for example use the Platform.isWindows ? describe.skip : describe pattern for the Unix-only block and the inverse for the Windows-only block) or add a short note above the examples stating that describe.skipIf is Vitest/Bun-specific and show the equivalent Jest/Mocha pattern so consumers know how to adapt describe.skipIf(Platform.isWindows) and describe.skipIf(!Platform.isWindows).
🤖 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/platform-specific-dependencies.md:
- Around line 34-47: The function useFSEvents mixes ESM export with CommonJS
require() which will fail in ESM-only runtimes; update useFSEvents to use
dynamic imports (import()) instead of require() when loading "fsevents" and
"chokidar", wrap each dynamic import in try/catch to handle missing modules, and
if both imports fail return a safe fallback (e.g., return a small wrapper around
fs.watch or null) so the caller can degrade gracefully; reference the
useFSEvents function, the process.platform === "darwin" check, and the current
require("fsevents")/require("chokidar") calls when making the change.
In
@.agents/skills/cross-platform-compatibility/references/process-management.md:
- Around line 17-24: The static kill(pid: number, signal?: string) method
currently fire-and-forgets the Windows path that calls spawn("taskkill", ...);
update it to capture the spawned child process and handle its outcomes: attach
an 'error' handler to surface spawn failures and an 'exit' (or 'close') handler
to detect non-zero exit codes from taskkill, and surface those failures to the
caller (e.g., by rejecting/throwing or returning a failed boolean). Modify the
kill signature (or overload) so callers can await or check the result on
Windows, and keep the existing process.kill(path) branch for non-Windows
unchanged; reference the kill method, spawn call, and use taskkill exit code to
determine success.
- Around line 27-40: The spawnCommand function is vulnerable because it sets
shell: true while passing an argv array (in both the Windows branch that calls
spawn("cmd", ["/c", command, ...args], ...) and the non-Windows branch
spawn(command, args, ...)), which allows unescaped argument injection; fix by
removing shell: true in both spawn calls so arguments are passed directly to the
child process, or if shell semantics are required, switch to building a single,
properly shell-escaped command string and pass that (or use execFile/exec with
explicit escaping) instead of passing an args array with shell: true—update the
spawnCommand implementation accordingly.
In @.agents/skills/cross-platform-compatibility/references/shell-commands.md:
- Around line 14-24: The example execute method uses execAsync with a raw
command string (method execute and call to execAsync) which creates a shell
injection risk; update the example by adding a clear warning comment above
execute about untrusted input and provide/replace with a safer pattern (e.g.,
add a new executeSafe function that uses execFileAsync with a command + args
signature instead of shell interpretation), or modify callers to split command
and args and call execFileAsync; reference the existing getShell() usage so the
warning explains when shell mode is being used and include guidance to validate
or escape inputs if shell is unavoidable.
- Around line 35-58: The example methods listFiles, clearScreen, and openFile
build shell strings with user-controlled directory/filepath and are vulnerable
to command injection; change them to call a non-shell spawning API (e.g.,
execFile/spawn with argv array and shell: false) from the underlying execute
implementation so arguments are passed as discrete args rather than interpolated
into a shell, validate/normalize the incoming paths (e.g., using path.resolve
and whitelist checks) before spawning, and update the shared execute method to
accept an argv array or opts object and use it for listFiles, clearScreen, and
openFile instead of string templates.
In `@src/mempalace/git-hook.ts`:
- Around line 380-382: The code currently creates the runner file via
writeExecutableFile(runnerPath, buildReindexRunnerScript()) before the user-hook
conflict check, causing partial-install artifacts when the install fails with
user_hook_conflict; change the flow so the runner is only written after the
conflict check succeeds (or ensure it is removed on failure). Concretely, move
or defer the call to writeExecutableFile (and any similar calls in the 392-399
range) until after the user hook conflict check succeeds (the logic that detects
user_hook_conflict), or add cleanup logic that deletes runnerPath when a
user_hook_conflict failure is detected so no artifacts remain on failed install.
- Around line 96-103: The current logic treats any failed gitValue call for
"core.hooksPath" as if the key were unset; update the handling of
coreHooksPathResult (from gitValue(exec, repoRoot, ["config", "--get",
"core.hooksPath"])) to distinguish a genuine unset key (git exit code 1) from
other failures: if coreHooksPathResult.ok is false and the underlying error/code
is not the "unset" exit code, throw or return that error (propagate the failure)
instead of falling back to the default hooks path; otherwise proceed to set
coreHooksPath from coreHooksPathResult.value and compute hooksDir with
resolveMaybeRelative(repoRoot, coreHooksPath) or fallback to
path.join(resolveMaybeRelative(repoRoot, commonDirResult.value), "hooks").
---
Nitpick comments:
In @.agents/skills/cross-platform-compatibility/references/file-permissions.md:
- Around line 35-41: Update the documentation/comment for createWithPermissions
to state that the mode argument passed to fs.writeFileSync only affects
Unix-like systems (Windows ignores file mode), and annotate the
createWithPermissions(filepath, content, mode) implementation accordingly;
mention that on Windows the function will write the file but not modify
permissions, and optionally suggest using platform-specific APIs (or a no-op) if
callers need explicit Windows ACL handling.
In
@.agents/skills/cross-platform-compatibility/references/testing-across-platforms.md:
- Around line 36-46: The three workflow steps all use the identical step name
"Platform-specific tests", which reduces clarity; update each step to a unique,
descriptive name such as "Windows platform tests", "macOS platform tests", and
"Linux platform tests" (these are the step name values in the YAML for the three
platform-specific steps) so the job matrix and logs are explicit and easier to
maintain.
- Around line 70-90: The current use of describe.skipIf (e.g., describe.skipIf
and Platform.isWindows) is not portable across Jest/Mocha; replace those calls
with a framework-agnostic conditional describe expression (for example use the
Platform.isWindows ? describe.skip : describe pattern for the Unix-only block
and the inverse for the Windows-only block) or add a short note above the
examples stating that describe.skipIf is Vitest/Bun-specific and show the
equivalent Jest/Mocha pattern so consumers know how to adapt
describe.skipIf(Platform.isWindows) and describe.skipIf(!Platform.isWindows).
In `@tests/harness/stages/implement-apply.test.ts`:
- Around line 257-266: The global assertion for "shell: bash" is too loose;
modify the test in tests/harness/stages/implement-apply.test.ts so it verifies
the shell override specifically within the verify-pr-source job. Instead of the
broad expect(workflow).toContain("shell: bash"), parse the workflow YAML (e.g.,
using js-yaml) or use a regex to locate the "verify-pr-source" job block (look
for "verify-pr-source" or "verify-pr-source:" in the workflow string) and assert
that that job's steps include a shell entry equal to "bash" (or that the job
block contains "shell: bash"), referencing the workflow variable and the
"verify-pr-source" identifier in your 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: e0f3cc4f-8ac4-488a-a1ba-9f5e257a8ace
📒 Files selected for processing (60)
.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.yml.omp/omp-audit-config.jsonOMP_CHANGELOG_AUDIT.mdREADME.mdpackage.jsonskills-lock.jsonskills/context-mode/SKILL.mdsrc/ci.tssrc/commands/doctor.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/planning/planning-ask-tool.tssrc/storage/plans.tssrc/text.tssrc/types.tssrc/utils/editor.tstests/commands/doctor.test.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/planning/planning-ask-tool.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
- process-management.md: drop shell: true with argv array (DEP0190); capture taskkill child to surface spawn errors and non-zero exits. - shell-commands.md: warn against passing untrusted input to exec; add executeSafe(execFile) and rewrite listFiles/clearScreen/openFile to use execFile so caller paths cannot be shell-interpreted. - mempalace/git-hook: distinguish 'core.hooksPath unset' (git exit 1) from real config errors; propagate the latter as git_failed. - mempalace/git-hook: defer reindex runner write until after the user_hook_conflict check so failed installs leave no artifacts. - tests: cover git_failed branch and assert runner absence on conflict.
Owner
Author
|
/review |
ESM `export` with CommonJS `require()` cannot coexist under "type": "module"; switch the cross-platform-compatibility example to async + dynamic imports with a node:fs/promises#watch fallback so copying the snippet does not produce a ReferenceError.
Owner
Author
|
/review |
…e examples - Make ProcessUtils.kill async so taskkill failures propagate as rejected Promises instead of being lost in event-emitter callbacks - Replace cmd.exe dir/start builtins with fs/promises readdir and explorer.exe to avoid shell-parsing caller-supplied paths - Replace platform-branched clearScreen with a direct ANSI escape write
… patterns - Verify process-management.md uses async/await kill with execFileAsync so taskkill errors are catchable - Verify shell-commands.md uses readdir instead of cmd /c dir and explorer.exe instead of cmd /c start
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 by CodeRabbit
New Features
Documentation
Tests