Fix/lint and test errors#542
Conversation
Add cross-platform path normalization and home-dir resolution across shell hooks and scripts, improve JS utilities and orchestrator logic, and update tests/docs. Key changes: - Docs: update agent/skill/command counts in AGENTS.md and README.md. - Shell scripts: add path normalization helpers (support WSL/Cygwin/Windows paths) in orchestrate-codex-worker.sh, observe.sh, and detect-project.sh; ensure HOME/USERPROFILE/CLAUDE_PROJECT_DIR are normalized and exported. - JS: make observation error/feedback checks robust for undefined/null; improve template variable quoting and reuse buildTemplateVariables in tmux-worktree-orchestrator; enforce unique worker slugs; enhance getHomeDir to prefer HOME/USERPROFILE/HOMEDRIVE+HOMEPATH before os.homedir(). - detect-project.sh: normalize env paths and return resolved python command via command -v. - Tests: fix shebang removal regex in validators.test.js, remove unused import in skill-dashboard.test.js, add retrying cleanupTestDir helper and use it in orchestrate-codex-worker tests. - Minor: remove extra blank line in a Rust SKILL.md. These changes improve cross-platform reliability (especially on Windows/WSL/Cygwin), prevent duplicate worker names, and harden tests and utilities.
Export resolved project vars and make shell tests robust on Windows. - Export PROJECT_ID/PROJECT_NAME/PROJECT_ROOT/PROJECT_DIR from detect-project.sh so downstream scripts can access them when sourced. - Add shellQuote helper and Windows-specific spawn logic in hooks.test.js to export test env vars into a bash -lc invocation, preserving proper quoting and allowing env propagation when running under win32 shells. - Replace fragile printf reads with env | grep ... to reliably obtain exported variables from the spawned shell. - Ensure HOME and USERPROFILE are set for the test environment on Windows before sourcing detect-project.sh. - Add toBashPath helper in orchestrate-codex-worker.test.js and use it when invoking the script so Windows paths are converted for bash. These changes make the test suite and scripts behave correctly on Windows (e.g., Git Bash/WSL) and avoid variable scoping/quoting issues when spawning bash from Node.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces cross-platform path normalization across shell scripts and JavaScript files, adds shell-safe placeholder validation for launcher commands, enhances home directory resolution on Windows, refines skill observation input validation, and updates test infrastructure to handle Windows path conventions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR improves cross-platform reliability by adding Key changes:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Shell script receives path arg] --> B{Path starts with /?\nUnix-style?}
B -- Yes --> G[Return as-is]
B -- No --> C{cygpath available?}
C -- Yes --> D[cygpath -u → /cygdrive/c/...]
C -- No --> E{wslpath available?}
E -- Yes --> F[wslpath -a → /mnt/c/...]
E -- No --> H{Matches Windows\ndrive pattern?\nC:\\ or C:/}
H -- Yes --> I[Manual: lowercase drive\n+ replace \\ with /\n→ /mnt/c/...]
H -- No --> J[Return input unchanged]
D --> K[Normalized Unix path]
F --> K
I --> K
G --> K
J --> K
K --> L[Task / Handoff / Status file paths\nnow safe for Unix bash]
style D fill:#d4edda
style F fill:#d4edda
style I fill:#fff3cd
style I stroke:#ffc107
Last reviewed commit: "Merge branch 'main' ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving cross-platform reliability (Windows/WSL/macOS/Linux) for shell + Node tooling by normalizing paths/home directories, tightening orchestration plan handling, and updating tests/docs to match current repository counts.
Changes:
- Added/updated path normalization and environment exporting across shell scripts and Node-based test helpers for better Windows/WSL compatibility.
- Hardened orchestration plan generation (unique worker slugs; consistent templating variables/quoting).
- Updated tests, minor utilities/error handling, and documentation counts (agents/skills/commands).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/scripts/orchestrate-codex-worker.test.js | Adds Windows path conversion + robust temp-dir cleanup for the worker launcher test. |
| tests/lib/skill-dashboard.test.js | Removes an unused import. |
| tests/hooks/hooks.test.js | Adds bash-path roundtripping + shell-quoting; improves Windows env setup when spawning bash. |
| tests/ci/validators.test.js | Makes shebang removal more robust (handles CRLF and leading whitespace). |
| skills/rust-patterns/SKILL.md | Removes an extra blank line. |
| skills/continuous-learning-v2/scripts/detect-project.sh | Normalizes HOME/USERPROFILE/CLAUDE_PROJECT_DIR; exports PROJECT_* vars; resolves python via command -v. |
| skills/continuous-learning-v2/hooks/observe.sh | Normalizes HOME/USERPROFILE and stdin-provided cwd prior to project detection. |
| scripts/orchestrate-codex-worker.sh | Adds path normalization for script arguments. |
| scripts/lib/utils.js | Makes getHomeDir() prefer env-provided home locations before os.homedir(). |
| scripts/lib/tmux-worktree-orchestrator.js | Ensures template vars include quoted forms; enforces unique worker slugs. |
| scripts/lib/skill-improvement/observations.js | Clarifies null/undefined handling for error and feedback. |
| README.md | Updates published counts (agents/skills/commands). |
| AGENTS.md | Updates published counts and structure overview. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| normalize_path() { | ||
| local input="$1" | ||
|
|
||
| if [[ -z "$input" ]]; then | ||
| printf '%s\n' '' | ||
| return 0 | ||
| fi | ||
|
|
||
| case "$input" in | ||
| /*) | ||
| printf '%s\n' "$input" | ||
| return 0 | ||
| ;; | ||
| esac | ||
|
|
||
| if command -v wslpath >/dev/null 2>&1; then | ||
| wslpath -a "$input" 2>/dev/null && return 0 | ||
| fi | ||
|
|
||
| if command -v cygpath >/dev/null 2>&1; then | ||
| cygpath -u "$input" 2>/dev/null && return 0 | ||
| fi |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/continuous-learning-v2/scripts/detect-project.sh">
<violation number="1" location="skills/continuous-learning-v2/scripts/detect-project.sh:73">
P2: Normalizing CLAUDE_PROJECT_DIR changes the path string used for the fallback project hash, but there’s no migration for old path-based hashes. Local or env-selected projects can silently get a new project_id and lose their existing per-project state.</violation>
</file>
<file name="AGENTS.md">
<violation number="1" location="AGENTS.md:3">
P2: AGENTS.md is internally inconsistent: it claims 25 agents, but the Available Agents table documents only 22.</violation>
</file>
<file name="scripts/orchestrate-codex-worker.sh">
<violation number="1" location="scripts/orchestrate-codex-worker.sh:31">
P2: Drive-letter paths are forcibly rewritten to WSL `/mnt/...` form, preventing `cygpath` conversion and breaking Windows Bash environments that are not WSL.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:194">
P3: README has conflicting capability totals (25/57/108 vs 21/52/102) across sections, causing inconsistent documentation.</violation>
</file>
<file name="scripts/lib/tmux-worktree-orchestrator.js">
<violation number="1" location="scripts/lib/tmux-worktree-orchestrator.js:214">
P2: Shell-safe placeholders were added, but launcher templates are not validated/migrated, so raw path placeholders are still rendered into shell command strings and remain unsafe.</violation>
</file>
<file name="tests/scripts/orchestrate-codex-worker.test.js">
<violation number="1" location="tests/scripts/orchestrate-codex-worker.test.js:17">
P2: Windows paths are converted to WSL-only `/mnt/<drive>` format while invoking an unspecified `bash`, making the test fragile/failing on non-WSL Windows bash environments.</violation>
</file>
<file name="tests/ci/validators.test.js">
<violation number="1" location="tests/ci/validators.test.js:71">
P2: CRLF shebang fix is only partially applied; other wrapper helpers still use old regex and can fail under `node -e` (notably `runCatalogValidator` after prepending code).</violation>
</file>
<file name="scripts/lib/utils.js">
<violation number="1" location="scripts/lib/utils.js:21">
P1: `getHomeDir()` now returns Windows bash-style `HOME` values unchanged, causing incorrect `.claude` paths when joined with `path` on win32.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/orchestrate-codex-worker.sh">
<violation number="1" location="scripts/orchestrate-codex-worker.sh:13">
P1: A nested redefinition of `normalize_path` causes the function to emit no output for non-empty input, so `task_file` is set to an empty string and the script falsely reports the task file as unreadable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This reverts commit 6c84259.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/lib/skill-improvement/observations.js (1)
33-41:⚠️ Potential issue | 🟡 MinorGuard
inputandinput.skillbefore readingpath.The new null-handling still leaves a raw
TypeErrorhere when callers omitinputorskill, because Line 36 dereferencesinput.skill.pathunconditionally.As per coding guidelines, "Always validate all user input before processing at system boundaries".Suggested fix
function createSkillObservation(input) { + if (!input || typeof input !== 'object' || !input.skill || typeof input.skill !== 'object') { + throw new Error('skill must be an object'); + } + const task = ensureString(input.task, 'task'); const skillId = ensureString(input.skill && input.skill.id, 'skill.id'); const skillPath = typeof input.skill.path === 'string' && input.skill.path.trim().length > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/skill-improvement/observations.js` around lines 33 - 41, In createSkillObservation, avoid dereferencing input.skill.path when input or input.skill may be missing: first validate/guard that input and input.skill exist before reading path (e.g., check input && input.skill or use optional chaining) and only call .trim() when typeof input.skill.path === 'string' and input.skill is present; set skillPath to null when input or input.skill or input.skill.path are absent to preserve existing behavior and keep other fields (task, success, error, feedback) unchanged.tests/ci/validators.test.js (1)
67-71:⚠️ Potential issue | 🟠 MajorApply the CRLF-safe shebang stripping to the other wrapper helpers.
runValidatorWithDirs()andrunCatalogValidator()still use/^#!.*\n/, so the Windows CRLF failure is only fixed for one execution path here. Any validator exercised through those helpers can still fail to eval undernode -e.Suggested fix
+function stripShebang(source) { + return source.replace(/^#![^\r\n]*[\r\n]+/, '').trimStart(); +} + function runValidatorWithDir(validatorName, dirConstant, overridePath) { const validatorPath = path.join(validatorsDir, `${validatorName}.js`); @@ - source = source.replace(/^#![^\r\n]*[\r\n]+/, '').trimStart(); + source = stripShebang(source); @@ function runValidatorWithDirs(validatorName, overrides) { const validatorPath = path.join(validatorsDir, `${validatorName}.js`); let source = fs.readFileSync(validatorPath, 'utf8'); - source = source.replace(/^#!.*\n/, ''); + source = stripShebang(source); @@ function runCatalogValidator(overrides = {}) { const validatorPath = path.join(validatorsDir, 'catalog.js'); let source = fs.readFileSync(validatorPath, 'utf8'); - source = source.replace(/^#!.*\n/, ''); + source = stripShebang(source);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ci/validators.test.js` around lines 67 - 71, Update the shebang-stripping regex in the other wrapper helpers so they are CRLF-safe: locate runValidatorWithDirs and runCatalogValidator and replace their /^#!.*\n/ usage with the same pattern used in the shown diff (/^#![^\r\n]*[\r\n]+/), and preserve the subsequent trimStart() behavior so validators evaluated via node -e won't break on Windows CRLF line endings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 81-84: _clv2_resolve_python_cmd currently only checks
CLV2_PYTHON_CMD with command -v which ignores filesystem paths (e.g. Windows
C:\...); update the function to first normalize CLV2_PYTHON_CMD using
_clv2_normalize_path, then test whether the resulting path is an executable file
(e.g. test -x or similar) and return that path if valid; if not valid, fall back
to using command -v on the original (or normalized) name and only then return
failure—refer to CLV2_PYTHON_CMD and the
_clv2_resolve_python_cmd/_clv2_normalize_path helpers when making the change.
In `@tests/hooks/hooks.test.js`:
- Around line 23-35: fromBashPath currently only recognizes Linux WSL-style
/mnt/<drive>/... paths; update the function so its regex handles Cygwin-style
/cygdrive/<drive>/... as well (e.g. match either "/mnt/<letter>" or
"/cygdrive/<letter>") and keep the existing remainder capture and replacement
logic (driveLetter, remainder) so the function still returns "C:\\path\\to" on
win32; adjust the match expression in fromBashPath and ensure remainder defaults
to '' and forward slashes are converted to backslashes as before.
---
Outside diff comments:
In `@scripts/lib/skill-improvement/observations.js`:
- Around line 33-41: In createSkillObservation, avoid dereferencing
input.skill.path when input or input.skill may be missing: first validate/guard
that input and input.skill exist before reading path (e.g., check input &&
input.skill or use optional chaining) and only call .trim() when typeof
input.skill.path === 'string' and input.skill is present; set skillPath to null
when input or input.skill or input.skill.path are absent to preserve existing
behavior and keep other fields (task, success, error, feedback) unchanged.
In `@tests/ci/validators.test.js`:
- Around line 67-71: Update the shebang-stripping regex in the other wrapper
helpers so they are CRLF-safe: locate runValidatorWithDirs and
runCatalogValidator and replace their /^#!.*\n/ usage with the same pattern used
in the shown diff (/^#![^\r\n]*[\r\n]+/), and preserve the subsequent
trimStart() behavior so validators evaluated via node -e won't break on Windows
CRLF line endings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c9f1945-e451-4a9b-aa08-7d1174a0184f
📒 Files selected for processing (13)
AGENTS.mdREADME.mdscripts/lib/skill-improvement/observations.jsscripts/lib/tmux-worktree-orchestrator.jsscripts/lib/utils.jsscripts/orchestrate-codex-worker.shskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shskills/rust-patterns/SKILL.mdtests/ci/validators.test.jstests/hooks/hooks.test.jstests/lib/skill-dashboard.test.jstests/scripts/orchestrate-codex-worker.test.js
💤 Files with no reviewable changes (2)
- skills/rust-patterns/SKILL.md
- tests/lib/skill-dashboard.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/hooks/hooks.test.js (1)
23-35:⚠️ Potential issue | 🟠 MajorSupport Cygwin-style paths in
fromBashPath.On Line 28,
fromBashPathonly accepts/mnt/<drive>/..., so Windows runs that emit/cygdrive/<drive>/...won’t normalize correctly.Proposed fix
function fromBashPath(filePath) { if (process.platform !== 'win32') { return filePath; } - const match = String(filePath).match(/^\/mnt\/([a-z])(?:\/(.*))?$/i); + const normalized = String(filePath); + const match = + normalized.match(/^\/mnt\/([a-z])(?:\/(.*))?$/i) || + normalized.match(/^\/cygdrive\/([a-z])(?:\/(.*))?$/i); if (!match) { return filePath; } const [, driveLetter, remainder = ''] = match; return `${driveLetter.toUpperCase()}:\\${remainder.replace(/\//g, '\\')}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 23 - 35, The fromBashPath function only handles WSL-style paths (/mnt/<drive>/...), so update fromBashPath to also detect and normalize Cygwin-style paths like /cygdrive/<drive>/<path> (in addition to the existing /mnt/<drive>/...), e.g., adjust the match logic (or add a second regex) to accept /^\/(?:mnt|cygdrive)\/([a-z])(?:\/(.*))?$/i, then convert the drive letter to uppercase and replace forward slashes with backslashes before returning (use the existing driveLetter/remainder handling and the same remainder.replace(/\//g, '\\') behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/orchestrate-codex-worker.sh`:
- Around line 13-57: The normalize_path function is accidentally defined twice
(an outer wrapper that only checks empty input and an inner full
implementation), causing early returns and empty outputs; fix by removing the
outer duplicate wrapper so there is a single normalize_path implementation that
performs the empty-input check, WSL/Cygwin conversions, Windows-drive handling
(drive/local rest logic using drive="${input%%:*}" and rest="${input#?:}"), and
final printf, ensuring normalize_path returns the normalized path for non-empty
inputs used by task_file, handoff_file, and status_file.
---
Duplicate comments:
In `@tests/hooks/hooks.test.js`:
- Around line 23-35: The fromBashPath function only handles WSL-style paths
(/mnt/<drive>/...), so update fromBashPath to also detect and normalize
Cygwin-style paths like /cygdrive/<drive>/<path> (in addition to the existing
/mnt/<drive>/...), e.g., adjust the match logic (or add a second regex) to
accept /^\/(?:mnt|cygdrive)\/([a-z])(?:\/(.*))?$/i, then convert the drive
letter to uppercase and replace forward slashes with backslashes before
returning (use the existing driveLetter/remainder handling and the same
remainder.replace(/\//g, '\\') behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fb28943-6703-478e-b198-24477447ef3f
📒 Files selected for processing (2)
scripts/orchestrate-codex-worker.shtests/hooks/hooks.test.js
scripts/orchestrate-codex-worker.sh
Outdated
| normalize_path() { | ||
| local input="$1" | ||
|
|
||
| if [[ -z "$input" ]]; then | ||
| printf '%s\n' '' | ||
| return 0 | ||
| fi | ||
|
|
||
| normalize_path() { | ||
| local input="$1" | ||
|
|
||
| if [[ -z "$input" ]]; then | ||
| printf '%s\n' '' | ||
| return 0 | ||
| fi | ||
|
|
||
| case "$input" in | ||
| /*) | ||
| printf '%s\n' "$input" | ||
| return 0 | ||
| ;; | ||
| esac | ||
|
|
||
| if command -v wslpath >/dev/null 2>&1; then | ||
| wslpath -a "$input" 2>/dev/null && return 0 | ||
| fi | ||
|
|
||
| if command -v cygpath >/dev/null 2>&1; then | ||
| cygpath -u "$input" 2>/dev/null && return 0 | ||
| fi | ||
|
|
||
| case "$input" in | ||
| [A-Za-z]:\\*|[A-Za-z]:/*) | ||
| local drive="${input%%:*}" | ||
| local rest="${input#?:}" | ||
| drive="$(printf '%s' "$drive" | tr '[:upper:]' '[:lower:]')" | ||
| rest="${rest//\\//}" | ||
| printf '/mnt/%s%s\n' "$drive" "$rest" | ||
| return 0 | ||
| ;; | ||
| esac | ||
|
|
||
| printf '%s\n' "$input" | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Duplicate nested function definition breaks path normalization.
The normalize_path function is defined twice in a nested manner. The outer function (starting at line 13) contains only the empty-input check, then defines an inner function (starting at line 21) without ever calling it. For any non-empty input, the outer function exits without producing output, causing task_file, handoff_file, and status_file to become empty strings.
This is likely a merge or copy-paste error and explains the pipeline failure.
🐛 Proposed fix: Remove the duplicate outer wrapper
-normalize_path() {
- local input="$1"
-
- if [[ -z "$input" ]]; then
- printf '%s\n' ''
- return 0
- fi
-
normalize_path() {
local input="$1"
if [[ -z "$input" ]]; then
printf '%s\n' ''
return 0
fi
case "$input" in
/*)
printf '%s\n' "$input"
return 0
;;
esac
if command -v wslpath >/dev/null 2>&1; then
wslpath -a "$input" 2>/dev/null && return 0
fi
if command -v cygpath >/dev/null 2>&1; then
cygpath -u "$input" 2>/dev/null && return 0
fi
case "$input" in
[A-Za-z]:\\*|[A-Za-z]:/*)
local drive="${input%%:*}"
local rest="${input#?:}"
drive="$(printf '%s' "$drive" | tr '[:upper:]' '[:lower:]')"
rest="${rest//\\//}"
printf '/mnt/%s%s\n' "$drive" "$rest"
return 0
;;
esac
printf '%s\n' "$input"
}
-}📝 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.
| normalize_path() { | |
| local input="$1" | |
| if [[ -z "$input" ]]; then | |
| printf '%s\n' '' | |
| return 0 | |
| fi | |
| normalize_path() { | |
| local input="$1" | |
| if [[ -z "$input" ]]; then | |
| printf '%s\n' '' | |
| return 0 | |
| fi | |
| case "$input" in | |
| /*) | |
| printf '%s\n' "$input" | |
| return 0 | |
| ;; | |
| esac | |
| if command -v wslpath >/dev/null 2>&1; then | |
| wslpath -a "$input" 2>/dev/null && return 0 | |
| fi | |
| if command -v cygpath >/dev/null 2>&1; then | |
| cygpath -u "$input" 2>/dev/null && return 0 | |
| fi | |
| case "$input" in | |
| [A-Za-z]:\\*|[A-Za-z]:/*) | |
| local drive="${input%%:*}" | |
| local rest="${input#?:}" | |
| drive="$(printf '%s' "$drive" | tr '[:upper:]' '[:lower:]')" | |
| rest="${rest//\\//}" | |
| printf '/mnt/%s%s\n' "$drive" "$rest" | |
| return 0 | |
| ;; | |
| esac | |
| printf '%s\n' "$input" | |
| } | |
| } | |
| normalize_path() { | |
| local input="$1" | |
| if [[ -z "$input" ]]; then | |
| printf '%s\n' '' | |
| return 0 | |
| fi | |
| case "$input" in | |
| /*) | |
| printf '%s\n' "$input" | |
| return 0 | |
| ;; | |
| esac | |
| if command -v wslpath >/dev/null 2>&1; then | |
| wslpath -a "$input" 2>/dev/null && return 0 | |
| fi | |
| if command -v cygpath >/dev/null 2>&1; then | |
| cygpath -u "$input" 2>/dev/null && return 0 | |
| fi | |
| case "$input" in | |
| [A-Za-z]:\\*|[A-Za-z]:/*) | |
| local drive="${input%%:*}" | |
| local rest="${input#?:}" | |
| drive="$(printf '%s' "$drive" | tr '[:upper:]' '[:lower:]')" | |
| rest="${rest//\\//}" | |
| printf '/mnt/%s%s\n' "$drive" "$rest" | |
| return 0 | |
| ;; | |
| esac | |
| printf '%s\n' "$input" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/orchestrate-codex-worker.sh` around lines 13 - 57, The normalize_path
function is accidentally defined twice (an outer wrapper that only checks empty
input and an inner full implementation), causing early returns and empty
outputs; fix by removing the outer duplicate wrapper so there is a single
normalize_path implementation that performs the empty-input check, WSL/Cygwin
conversions, Windows-drive handling (drive/local rest logic using
drive="${input%%:*}" and rest="${input#?:}"), and final printf, ensuring
normalize_path returns the normalized path for non-empty inputs used by
task_file, handoff_file, and status_file.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| function validateLauncherCommand(template) { | ||
| const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS) | ||
| .filter(([placeholder]) => template.includes(`{${placeholder}}`)) | ||
| .map(([placeholder, safePlaceholder]) => `{${placeholder}} -> {${safePlaceholder}}`); | ||
|
|
||
| if (unsafePlaceholders.length > 0) { | ||
| throw new Error( | ||
| `launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateLauncherCommand can be bypassed with _raw path placeholders
buildTemplateVariables generates three keys per path variable: {key}, {key_raw} (both unquoted), and {key_sh} (shell-quoted). validateLauncherCommand only blocks {task_file}, {handoff_file}, etc. — it does not check the {task_file_raw} / {handoff_file_raw} variants, which have the exact same unquoted value. A launcher command that uses {task_file_raw} instead of {task_file} will bypass this safety check entirely while still being vulnerable to shell injection from whitespace or special characters in paths.
The _raw variants should also be validated:
function validateLauncherCommand(template) {
const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS)
.flatMap(([placeholder, safePlaceholder]) => [
template.includes(`{${placeholder}}`) ? `{${placeholder}} -> {${safePlaceholder}}` : null,
template.includes(`{${placeholder}_raw}`) ? `{${placeholder}_raw} -> {${safePlaceholder}}` : null
])
.filter(Boolean);
if (unsafePlaceholders.length > 0) {
throw new Error(
`launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}`
);
}
}
There was a problem hiding this comment.
6 issues found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/hooks/cost-tracker.test.js">
<violation number="1" location="tests/hooks/cost-tracker.test.js:42">
P2: First test is not isolated: it still uses real HOME/USERPROFILE and can write to the runner’s actual `~/.claude/metrics/costs.jsonl`.</violation>
</file>
<file name="tests/hooks/hooks.test.js">
<violation number="1" location="tests/hooks/hooks.test.js:45">
P1: Cygwin is detected but not actually supported in path conversion: `/cygdrive/<drive>/...` is never produced or parsed.</violation>
<violation number="2" location="tests/hooks/hooks.test.js:54">
P3: `fromBashPath` should also handle `/cygdrive/<drive>/...` so Cygwin-formatted shell paths are converted back to native Windows paths before filesystem checks.</violation>
</file>
<file name="docs/zh-CN/commands/python-review.md">
<violation number="1" location="docs/zh-CN/commands/python-review.md:318">
P3: The table row uses `<code>x \| None</code>`, which can render a literal backslash and display invalid Python syntax in docs.</violation>
</file>
<file name="scripts/lib/tmux-worktree-orchestrator.js">
<violation number="1" location="scripts/lib/tmux-worktree-orchestrator.js:58">
P1: Launcher placeholder validation can be bypassed with `{..._raw}` path placeholders, defeating the new shell-safety guard for commands executed via shell/tmux.</violation>
</file>
<file name="tests/scripts/orchestrate-codex-worker.test.js">
<violation number="1" location="tests/scripts/orchestrate-codex-worker.test.js:43">
P2: Cygwin is detected but converted using `/c/...` paths instead of default `/cygdrive/c/...`, which can break script path resolution on Cygwin.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -39,6 +39,10 @@ function runScript(input, envOverrides = {}) { | |||
| return { code: result.status || 0, stdout: result.stdout || '', stderr: result.stderr || '' }; | |||
There was a problem hiding this comment.
P2: First test is not isolated: it still uses real HOME/USERPROFILE and can write to the runner’s actual ~/.claude/metrics/costs.jsonl.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/hooks/cost-tracker.test.js, line 42:
<comment>First test is not isolated: it still uses real HOME/USERPROFILE and can write to the runner’s actual `~/.claude/metrics/costs.jsonl`.</comment>
<file context>
@@ -39,6 +39,10 @@ function runScript(input, envOverrides = {}) {
return { code: result.status || 0, stdout: result.stdout || '', stderr: result.stderr || '' };
}
+function withHomeEnv(homeDir) {
+ return { HOME: homeDir, USERPROFILE: homeDir };
+}
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
skills/continuous-learning-v2/scripts/detect-project.sh (1)
83-86:⚠️ Potential issue | 🟠 Major
CLV2_PYTHON_CMDpath handling is still incomplete for Windows-style explicit paths.Line 84 only checks
CLV2_PYTHON_CMDviacommand -v. Values likeC:\Python311\python.exeare not normalized/validated as executable paths before resolution, so configured interpreters can be ignored.💡 Proposed fix
_clv2_resolve_python_cmd() { - if [ -n "${CLV2_PYTHON_CMD:-}" ] && command -v "$CLV2_PYTHON_CMD" >/dev/null 2>&1; then - command -v "$CLV2_PYTHON_CMD" - return 0 + if [ -n "${CLV2_PYTHON_CMD:-}" ]; then + local requested_python="$(_clv2_normalize_path "$CLV2_PYTHON_CMD")" + if [ -x "$requested_python" ]; then + printf '%s\n' "$requested_python" + return 0 + fi + if command -v "$CLV2_PYTHON_CMD" >/dev/null 2>&1; then + command -v "$CLV2_PYTHON_CMD" + return 0 + fi fiRun this read-only check to confirm normalization is currently missing inside
_clv2_resolve_python_cmd:#!/bin/bash set -euo pipefail file="skills/continuous-learning-v2/scripts/detect-project.sh" echo "=== _clv2_resolve_python_cmd body ===" awk ' /_clv2_resolve_python_cmd\(\)/ {in_fn=1} in_fn {print NR ":" $0} in_fn && /^}/ {exit} ' "$file" echo echo "=== Expectation check ===" if rg -n '_clv2_normalize_path "\$CLV2_PYTHON_CMD"' "$file" >/dev/null; then echo "OK: normalization call found for CLV2_PYTHON_CMD" else echo "MISSING: no _clv2_normalize_path call for CLV2_PYTHON_CMD in resolver" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/detect-project.sh` around lines 83 - 86, _clv2_resolve_python_cmd currently only uses command -v on CLV2_PYTHON_CMD, which skips Windows-style explicit paths like C:\Python311\python.exe; update _clv2_resolve_python_cmd to first call _clv2_normalize_path on the CLV2_PYTHON_CMD value, then validate the normalized path (e.g., test that it exists/is executable or falls back to command -v) before returning it; reference the function name _clv2_resolve_python_cmd and the helper _clv2_normalize_path so the resolver normalizes and validates CLV2_PYTHON_CMD values prior to resolution.tests/hooks/hooks.test.js (1)
54-60:⚠️ Potential issue | 🟠 Major
fromBashPathstill misses Cygwin/cygdrive/<drive>/...paths.Line 54 only matches
/mnt/<drive>/...and/<drive>/.... Cygwin-style outputs are still left unconverted, which can break Windows assertions.💡 Proposed fix
- const match = String(filePath).match(/^\/(?:mnt\/)?([a-z])(?:\/(.*))?$/i); + const match = String(filePath).match(/^\/(?:(?:mnt|cygdrive)\/)?([a-z])(?:\/(.*))?$/i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 54 - 60, The fromBashPath path conversion currently only detects "/mnt/<drive>/" and "/<drive>/" forms and misses Cygwin-style "/cygdrive/<drive>/..." paths; update the regex in the match assignment (the line defining match using String(filePath).match(...)) to also accept an optional "cygdrive/" segment (e.g., allow /^\/(?:cygdrive\/)?(?:mnt\/)?([a-z])(?:\/(.*))?$/i or equivalent), keep the existing guard for leading '//' and then continue to extract driveLetter and remainder as before and convert slashes to backslashes so Cygwin paths are correctly converted by fromBashPath.
🧹 Nitpick comments (2)
skills/continuous-learning-v2/scripts/detect-project.sh (1)
187-189: Avoid a single literal fallback project id.Using
"fallback"for all hash failures can merge unrelated projects into the same state directory. Prefer a deterministic, input-derived fallback to avoid collisions.💡 Proposed adjustment
if [ -z "$project_id" ]; then - project_id="fallback" + if command -v cksum >/dev/null 2>&1; then + project_id="$(printf '%s' "$hash_input" | cksum | awk '{print $1}' | cut -c1-12)" + else + project_id="global" + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/detect-project.sh` around lines 187 - 189, The code sets project_id to the literal "fallback" on hash failures which can collide; instead compute a deterministic fallback derived from available input (e.g., repository URL, current working directory, or another unique input) and use a truncated checksum as the id. Replace the hardcoded assignment (project_id="fallback") with logic that generates a hash (for example using sha1sum or md5sum over "$REPO_URL" or "$PWD"), truncate to a safe length (e.g., 8 chars) and set project_id to a prefix plus that hash (e.g., "fallback-<trunc_hash>") so each input maps to its own deterministic fallback project_id.skills/continuous-learning-v2/hooks/observe.sh (1)
15-50: Consider extracting shared path normalization to avoid duplication.This
normalize_shell_pathfunction is nearly identical to_clv2_normalize_pathindetect-project.sh(lines 22-60). Both handle the same path formats in the same order:
- Empty input → empty output
- Already-absolute Unix paths (
/...) → passthrough- Cygwin paths via
cygpath -u- WSL paths via
wslpath -a- Windows drive paths (
C:\...orC:/...) →/mnt/<drive>/...Consider extracting this into a shared file (e.g.,
scripts/lib/normalize-path.sh) that both scripts can source, reducing maintenance burden when the normalization logic needs updates.Additionally, since
observe.shnormalizesSTDIN_CWD(line 120) before exporting it asCLAUDE_PROJECT_DIR(line 124), anddetect-project.shre-normalizesCLAUDE_PROJECT_DIRon entry (lines 74-76), the path gets normalized twice. While this is safe (normalized paths start with/and pass through unchanged), it's redundant work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 15 - 50, The two nearly identical path normalization routines (normalize_shell_path in observe.sh and _clv2_normalize_path in detect-project.sh) should be consolidated into a single shared script (e.g., scripts/lib/normalize-path.sh) that exports one function name (choose normalize_shell_path or normalize_path) and then source that shared file from both observe.sh and detect-project.sh; update observe.sh to call the shared function when normalizing STDIN_CWD before exporting CLAUDE_PROJECT_DIR, and remove the redundant re-normalization in detect-project.sh (or vice versa) so CLAUDE_PROJECT_DIR is normalized only once; reference the existing symbols normalize_shell_path, _clv2_normalize_path, STDIN_CWD and CLAUDE_PROJECT_DIR when making these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lib/tmux-worktree-orchestrator.js`:
- Around line 246-249: The code currently calls
validateLauncherCommand(launcherCommand) after only a truthiness check, which
causes non-string truthy values to break (e.g., template.includes error); update
the check to first assert launcherCommand is a non-empty string (e.g., typeof
launcherCommand === 'string' && launcherCommand.trim() !== '') before calling
validateLauncherCommand, and throw the same Error(`Worker ${workerName} is
missing a launcherCommand`) or a clear type-specific error if it fails, or
alternatively add that string/type validation inside the validateLauncherCommand
function so it guards against non-string inputs.
- Around line 37-66: validateLauncherCommand currently only checks bare
placeholders (e.g., {repo_root}) against unsafe use but buildTemplateVariables
also produces *_raw aliases (e.g., repo_root_raw, worktree_path_raw,
task_file_raw) which bypass the check; update validateLauncherCommand to also
detect placeholders with the "_raw" suffix by extending SHELL_PATH_PLACEHOLDERS
checks to include both the original keys and their "_raw" variants (or test for
template.includes(`{${placeholder}_raw}`) as well) and throw the same error
message mapping them to the corresponding safe "{..._sh}" placeholder; also add
a regression test that asserts templates containing any "{..._raw}" (e.g.,
"{repo_root_raw}", "{task_file_raw}", "{worktree_path_raw}") are rejected by
validateLauncherCommand.
---
Duplicate comments:
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 83-86: _clv2_resolve_python_cmd currently only uses command -v on
CLV2_PYTHON_CMD, which skips Windows-style explicit paths like
C:\Python311\python.exe; update _clv2_resolve_python_cmd to first call
_clv2_normalize_path on the CLV2_PYTHON_CMD value, then validate the normalized
path (e.g., test that it exists/is executable or falls back to command -v)
before returning it; reference the function name _clv2_resolve_python_cmd and
the helper _clv2_normalize_path so the resolver normalizes and validates
CLV2_PYTHON_CMD values prior to resolution.
In `@tests/hooks/hooks.test.js`:
- Around line 54-60: The fromBashPath path conversion currently only detects
"/mnt/<drive>/" and "/<drive>/" forms and misses Cygwin-style
"/cygdrive/<drive>/..." paths; update the regex in the match assignment (the
line defining match using String(filePath).match(...)) to also accept an
optional "cygdrive/" segment (e.g., allow
/^\/(?:cygdrive\/)?(?:mnt\/)?([a-z])(?:\/(.*))?$/i or equivalent), keep the
existing guard for leading '//' and then continue to extract driveLetter and
remainder as before and convert slashes to backslashes so Cygwin paths are
correctly converted by fromBashPath.
---
Nitpick comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 15-50: The two nearly identical path normalization routines
(normalize_shell_path in observe.sh and _clv2_normalize_path in
detect-project.sh) should be consolidated into a single shared script (e.g.,
scripts/lib/normalize-path.sh) that exports one function name (choose
normalize_shell_path or normalize_path) and then source that shared file from
both observe.sh and detect-project.sh; update observe.sh to call the shared
function when normalizing STDIN_CWD before exporting CLAUDE_PROJECT_DIR, and
remove the redundant re-normalization in detect-project.sh (or vice versa) so
CLAUDE_PROJECT_DIR is normalized only once; reference the existing symbols
normalize_shell_path, _clv2_normalize_path, STDIN_CWD and CLAUDE_PROJECT_DIR
when making these edits.
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 187-189: The code sets project_id to the literal "fallback" on
hash failures which can collide; instead compute a deterministic fallback
derived from available input (e.g., repository URL, current working directory,
or another unique input) and use a truncated checksum as the id. Replace the
hardcoded assignment (project_id="fallback") with logic that generates a hash
(for example using sha1sum or md5sum over "$REPO_URL" or "$PWD"), truncate to a
safe length (e.g., 8 chars) and set project_id to a prefix plus that hash (e.g.,
"fallback-<trunc_hash>") so each input maps to its own deterministic fallback
project_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc252790-7ea4-4711-b3cc-483793715d45
📒 Files selected for processing (17)
.gitattributesAGENTS.mddocs/zh-CN/CODE_OF_CONDUCT.mddocs/zh-CN/commands/python-review.mdscripts/lib/tmux-worktree-orchestrator.jsscripts/lib/utils.jsscripts/orchestrate-codex-worker.shscripts/orchestrate-worktrees.jsskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shskills/dmux-workflows/SKILL.mdtests/ci/validators.test.jstests/hooks/cost-tracker.test.jstests/hooks/hooks.test.jstests/lib/tmux-worktree-orchestrator.test.jstests/lib/utils.test.jstests/scripts/orchestrate-codex-worker.test.js
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/orchestrate-codex-worker.sh
- tests/scripts/orchestrate-codex-worker.test.js
- tests/ci/validators.test.js
| function buildTemplateVariables(values) { | ||
| return Object.entries(values).reduce((accumulator, [key, value]) => { | ||
| const stringValue = String(value); | ||
| const quotedValue = shellQuote(stringValue); | ||
|
|
||
| accumulator[key] = stringValue; | ||
| accumulator[`${key}_raw`] = stringValue; | ||
| accumulator[`${key}_sh`] = quotedValue; | ||
| accumulator[`${key}_sh`] = shellQuote(stringValue); | ||
| return accumulator; | ||
| }, {}); | ||
| } | ||
|
|
||
| const SHELL_PATH_PLACEHOLDERS = { | ||
| handoff_file: 'handoff_file_sh', | ||
| repo_root: 'repo_root_sh', | ||
| status_file: 'status_file_sh', | ||
| task_file: 'task_file_sh', | ||
| worktree_path: 'worktree_path_sh' | ||
| }; | ||
|
|
||
| function validateLauncherCommand(template) { | ||
| const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS) | ||
| .filter(([placeholder]) => template.includes(`{${placeholder}}`)) | ||
| .map(([placeholder, safePlaceholder]) => `{${placeholder}} -> {${safePlaceholder}}`); | ||
|
|
||
| if (unsafePlaceholders.length > 0) { | ||
| throw new Error( | ||
| `launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Block *_raw path aliases in launcherCommand too.
buildTemplateVariables() still emits repo_root_raw, worktree_path_raw, task_file_raw, etc., but validateLauncherCommand() only rejects the bare names. A template like bash {repo_root_raw}/scripts/orchestrate-codex-worker.sh {task_file_raw} now passes validation, which reintroduces the unquoted-path breakage this hardening is trying to prevent and leaves a shell-safety gap for paths containing metacharacters. Please add a regression test for the _raw variants when you patch this.
🔒 Suggested hardening
function validateLauncherCommand(template) {
const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS)
- .filter(([placeholder]) => template.includes(`{${placeholder}}`))
- .map(([placeholder, safePlaceholder]) => `{${placeholder}} -> {${safePlaceholder}}`);
+ .flatMap(([placeholder, safePlaceholder]) =>
+ [`{${placeholder}}`, `{${placeholder}_raw}`]
+ .filter(token => template.includes(token))
+ .map(token => `${token} -> {${safePlaceholder}}`)
+ );
if (unsafePlaceholders.length > 0) {
throw new Error(
`launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}`
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/tmux-worktree-orchestrator.js` around lines 37 - 66,
validateLauncherCommand currently only checks bare placeholders (e.g.,
{repo_root}) against unsafe use but buildTemplateVariables also produces *_raw
aliases (e.g., repo_root_raw, worktree_path_raw, task_file_raw) which bypass the
check; update validateLauncherCommand to also detect placeholders with the
"_raw" suffix by extending SHELL_PATH_PLACEHOLDERS checks to include both the
original keys and their "_raw" variants (or test for
template.includes(`{${placeholder}_raw}`) as well) and throw the same error
message mapping them to the corresponding safe "{..._sh}" placeholder; also add
a regression test that asserts templates containing any "{..._raw}" (e.g.,
"{repo_root_raw}", "{task_file_raw}", "{worktree_path_raw}") are rejected by
validateLauncherCommand.
| if (!launcherCommand) { | ||
| throw new Error(`Worker ${workerName} is missing a launcherCommand`); | ||
| } | ||
| validateLauncherCommand(launcherCommand); |
There was a problem hiding this comment.
Validate launcherCommand is a string before calling the placeholder validator.
A truthy non-string value from plan.json (for example 123 or {}) now fails with template.includes is not a function instead of the existing clear launcher-command validation. Keep the type/non-empty check ahead of validateLauncherCommand() or move it into that helper.
🛠️ Suggested fix
- if (!launcherCommand) {
- throw new Error(`Worker ${workerName} is missing a launcherCommand`);
- }
- validateLauncherCommand(launcherCommand);
+ if (typeof launcherCommand !== 'string' || launcherCommand.trim().length === 0) {
+ throw new Error(`Worker ${workerName} launcherCommand must be a non-empty string`);
+ }
+ validateLauncherCommand(launcherCommand);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/tmux-worktree-orchestrator.js` around lines 246 - 249, The code
currently calls validateLauncherCommand(launcherCommand) after only a truthiness
check, which causes non-string truthy values to break (e.g., template.includes
error); update the check to first assert launcherCommand is a non-empty string
(e.g., typeof launcherCommand === 'string' && launcherCommand.trim() !== '')
before calling validateLauncherCommand, and throw the same Error(`Worker
${workerName} is missing a launcherCommand`) or a clear type-specific error if
it fails, or alternatively add that string/type validation inside the
validateLauncherCommand function so it guards against non-string inputs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
4 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/zh-CN/commands/python-review.md">
<violation number="1">
P2: Table cell now contains an unescaped pipe due to escaped backticks, which can break Markdown table rendering and fails to format `x | None` as inline code.</violation>
</file>
<file name="skills/continuous-learning-v2/scripts/detect-project.sh">
<violation number="1" location="skills/continuous-learning-v2/scripts/detect-project.sh:88">
P1: `CLV2_PYTHON_CMD` now prefers normalized filesystem paths over PATH lookup, allowing bare names (e.g., `python3`) to resolve to and execute repo-local binaries on WSL/Cygwin-like setups.</violation>
</file>
<file name="tests/scripts/orchestrate-codex-worker.test.js">
<violation number="1" location="tests/scripts/orchestrate-codex-worker.test.js:47">
P2: Windows bash path conversion hard-codes `/cygdrive/` based on `cygpath` presence, which can mis-handle MSYS/Git Bash and non-default Cygwin prefixes, causing script path resolution failures.</violation>
</file>
<file name="tests/hooks/hooks.test.js">
<violation number="1" location="tests/hooks/hooks.test.js:49">
P2: Windows bash path-style detection is too coarse: treating any shell with `cygpath` as Cygwin can force `/cygdrive/...` paths in MSYS/Git Bash environments that use `/c/...`, breaking shell-script test execution.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| local normalized_python_cmd="" | ||
| normalized_python_cmd="$(_clv2_normalize_path "$CLV2_PYTHON_CMD")" | ||
|
|
||
| if [ -f "$normalized_python_cmd" ] && [ -x "$normalized_python_cmd" ]; then |
There was a problem hiding this comment.
P1: CLV2_PYTHON_CMD now prefers normalized filesystem paths over PATH lookup, allowing bare names (e.g., python3) to resolve to and execute repo-local binaries on WSL/Cygwin-like setups.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/continuous-learning-v2/scripts/detect-project.sh, line 88:
<comment>`CLV2_PYTHON_CMD` now prefers normalized filesystem paths over PATH lookup, allowing bare names (e.g., `python3`) to resolve to and execute repo-local binaries on WSL/Cygwin-like setups.</comment>
<file context>
@@ -81,9 +81,24 @@ _CLV2_PROJECTS_DIR="${_CLV2_HOMUNCULUS_DIR}/projects"
+ local normalized_python_cmd=""
+ normalized_python_cmd="$(_clv2_normalize_path "$CLV2_PYTHON_CMD")"
+
+ if [ -f "$normalized_python_cmd" ] && [ -x "$normalized_python_cmd" ]; then
+ printf '%s\n' "$normalized_python_cmd"
+ return 0
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/continuous-learning-v2/scripts/detect-project.sh (1)
208-214:⚠️ Potential issue | 🟠 Major
mv-based legacy migration can break active observer processes.Line 213 moves project directories in-place.
start-observer.shexports a cachedPROJECT_DIRto long-lived background workers, so moving that directory mid-run can invalidate their path and cause write failures or split state.Suggested fix
if [ "$legacy_hash_input" != "$hash_input" ]; then local legacy_id="" legacy_id="$(_clv2_hash_identifier "$legacy_hash_input" 2>/dev/null || true)" if [ -n "$legacy_id" ] && [ -d "${_CLV2_PROJECTS_DIR}/${legacy_id}" ] && [ ! -d "${_CLV2_PROJECTS_DIR}/${project_id}" ]; then - # Migrate legacy directory to new hash - mv "${_CLV2_PROJECTS_DIR}/${legacy_id}" "${_CLV2_PROJECTS_DIR}/${project_id}" 2>/dev/null || project_id="$legacy_id" + # Reuse legacy state in-band; avoid moving directories while daemons may hold cached paths. + project_id="$legacy_id" fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/scripts/detect-project.sh` around lines 208 - 214, The in-place mv of ${_CLV2_PROJECTS_DIR}/${legacy_id} to ${_CLV2_PROJECTS_DIR}/${project_id} can break long-lived observers that have exported PROJECT_DIR; instead, preserve the legacy path and make the new hashed path point to it: detect when legacy_id exists and project_id does not, create the target by copying/syncing data (e.g., rsync -a or cp -a from "${_CLV2_PROJECTS_DIR}/${legacy_id}" to "${_CLV2_PROJECTS_DIR}/${project_id}" or simply create a symlink), and then create a symlink from "${_CLV2_PROJECTS_DIR}/${project_id}" to "${_CLV2_PROJECTS_DIR}/${legacy_id}" (or copy then leave legacy intact) rather than using mv in the block that references legacy_id, project_id, and _clv2_hash_identifier so observers keep a valid PROJECT_DIR.
🧹 Nitpick comments (1)
tests/scripts/orchestrate-codex-worker.test.js (1)
37-50: Consider renaming local variable to avoid shadowing module-levelbashPathStyle.Line 43 declares
const bashPathStylewhich shadows the module-levellet bashPathStylefrom line 10. While functionally correct (the return value fromgetBashPathStyle()is captured), this can be confusing for readers.♻️ Suggested rename for clarity
function toBashPath(filePath) { if (process.platform !== 'win32') { return filePath; } const normalized = String(filePath).replace(/\\/g, '/'); - const bashPathStyle = getBashPathStyle(); - const drivePrefix = bashPathStyle === 'wsl' + const pathStyle = getBashPathStyle(); + const drivePrefix = pathStyle === 'wsl' ? '/mnt/' - : bashPathStyle === 'cygwin' + : pathStyle === 'cygwin' ? '/cygdrive/' : '/'; return normalized.replace(/^([A-Za-z]):/, (_, driveLetter) => `${drivePrefix}${driveLetter.toLowerCase()}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/orchestrate-codex-worker.test.js` around lines 37 - 50, The local variable bashPathStyle inside the toBashPath function shadows the module-level let bashPathStyle; rename the local binding returned from getBashPathStyle() (for example to localBashPathStyle or style) to avoid shadowing and confusion—update the usage in the ternary that computes drivePrefix and any subsequent references in toBashPath, leaving the module-level bashPathStyle unchanged and ensuring getBashPathStyle() is still called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 199-204: When _clv2_hash_identifier fails and project_id is empty,
avoid assigning the constant "fallback"; instead generate a unique fallback
identifier to prevent collisions by appending a UUID or entropy-based suffix.
Replace the assignment for project_id in detect-project.sh so that after the
_clv2_hash_identifier call you set project_id to something like
"fallback-<uuid-or-random>" using /proc/sys/kernel/random/uuid or uuidgen if
available, falling back to a timestamp+RANDOM combo; ensure this logic is used
only when project_id is empty so the existing _clv2_hash_identifier outcome
stays unchanged.
---
Outside diff comments:
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 208-214: The in-place mv of ${_CLV2_PROJECTS_DIR}/${legacy_id} to
${_CLV2_PROJECTS_DIR}/${project_id} can break long-lived observers that have
exported PROJECT_DIR; instead, preserve the legacy path and make the new hashed
path point to it: detect when legacy_id exists and project_id does not, create
the target by copying/syncing data (e.g., rsync -a or cp -a from
"${_CLV2_PROJECTS_DIR}/${legacy_id}" to "${_CLV2_PROJECTS_DIR}/${project_id}" or
simply create a symlink), and then create a symlink from
"${_CLV2_PROJECTS_DIR}/${project_id}" to "${_CLV2_PROJECTS_DIR}/${legacy_id}"
(or copy then leave legacy intact) rather than using mv in the block that
references legacy_id, project_id, and _clv2_hash_identifier so observers keep a
valid PROJECT_DIR.
---
Nitpick comments:
In `@tests/scripts/orchestrate-codex-worker.test.js`:
- Around line 37-50: The local variable bashPathStyle inside the toBashPath
function shadows the module-level let bashPathStyle; rename the local binding
returned from getBashPathStyle() (for example to localBashPathStyle or style) to
avoid shadowing and confusion—update the usage in the ternary that computes
drivePrefix and any subsequent references in toBashPath, leaving the
module-level bashPathStyle unchanged and ensuring getBashPathStyle() is still
called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6296e16f-07ff-4821-9b23-a6d25de22c1e
📒 Files selected for processing (6)
docs/zh-CN/CODE_OF_CONDUCT.mdscripts/lib/skill-improvement/observations.jsskills/continuous-learning-v2/scripts/detect-project.shtests/hooks/hooks.test.jstests/lib/skill-improvement.test.jstests/scripts/orchestrate-codex-worker.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/lib/skill-improvement/observations.js
- docs/zh-CN/CODE_OF_CONDUCT.md
| project_id="$(_clv2_hash_identifier "$hash_input" 2>/dev/null || true)" | ||
|
|
||
| # Fallback if Python is unavailable or hash generation failed. | ||
| if [ -z "$project_id" ]; then | ||
| project_id=$(printf '%s' "$hash_input" | shasum -a 256 2>/dev/null | cut -c1-12 || \ | ||
| printf '%s' "$hash_input" | sha256sum 2>/dev/null | cut -c1-12 || \ | ||
| echo "fallback") | ||
| project_id="fallback" | ||
| fi |
There was a problem hiding this comment.
Avoid constant project_id fallback collisions.
When hashing is unavailable, Line 203 sets every project to fallback, which merges unrelated project state and can overwrite registry metadata.
Suggested fix
# Fallback if Python is unavailable or hash generation failed.
if [ -z "$project_id" ]; then
- project_id="fallback"
+ if command -v cksum >/dev/null 2>&1; then
+ project_id="$(printf '%s' "$hash_input" | cksum | awk '{print $1}' | cut -c1-12)"
+ else
+ # Last-resort deterministic fallback; still project-specific
+ project_id="$(printf '%s' "$hash_input" | tr -cd '[:alnum:]' | cut -c1-12)"
+ fi
fi📝 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.
| project_id="$(_clv2_hash_identifier "$hash_input" 2>/dev/null || true)" | |
| # Fallback if Python is unavailable or hash generation failed. | |
| if [ -z "$project_id" ]; then | |
| project_id=$(printf '%s' "$hash_input" | shasum -a 256 2>/dev/null | cut -c1-12 || \ | |
| printf '%s' "$hash_input" | sha256sum 2>/dev/null | cut -c1-12 || \ | |
| echo "fallback") | |
| project_id="fallback" | |
| fi | |
| project_id="$(_clv2_hash_identifier "$hash_input" 2>/dev/null || true)" | |
| # Fallback if Python is unavailable or hash generation failed. | |
| if [ -z "$project_id" ]; then | |
| if command -v cksum >/dev/null 2>&1; then | |
| project_id="$(printf '%s' "$hash_input" | cksum | awk '{print $1}' | cut -c1-12)" | |
| else | |
| # Last-resort deterministic fallback; still project-specific | |
| project_id="$(printf '%s' "$hash_input" | tr -cd '[:alnum:]' | cut -c1-12)" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/continuous-learning-v2/scripts/detect-project.sh` around lines 199 -
204, When _clv2_hash_identifier fails and project_id is empty, avoid assigning
the constant "fallback"; instead generate a unique fallback identifier to
prevent collisions by appending a UUID or entropy-based suffix. Replace the
assignment for project_id in detect-project.sh so that after the
_clv2_hash_identifier call you set project_id to something like
"fallback-<uuid-or-random>" using /proc/sys/kernel/random/uuid or uuidgen if
available, falling back to a timestamp+RANDOM combo; ensure this logic is used
only when project_id is empty so the existing _clv2_hash_identifier outcome
stays unchanged.
| function validateLauncherCommand(template) { | ||
| const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS) | ||
| .filter(([placeholder]) => template.includes(`{${placeholder}}`)) | ||
| .map(([placeholder, safePlaceholder]) => `{${placeholder}} -> {${safePlaceholder}}`); | ||
|
|
||
| if (unsafePlaceholders.length > 0) { | ||
| throw new Error( | ||
| `launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateLauncherCommand does not block _raw placeholder variants
buildTemplateVariables registers three keys per path variable: {key} (unquoted), {key_raw} (also unquoted, identical value), and {key_sh} (shell-quoted). validateLauncherCommand only checks for the bare {task_file}, {handoff_file}, etc. forms. A launcherCommand that uses {task_file_raw} instead of {task_file} will pass this validation while still exposing the raw, unquoted path to the shell — making it trivially bypassable.
The _raw variants carry the same injection risk as the base names and should also be rejected:
function validateLauncherCommand(template) {
const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS)
.flatMap(([placeholder, safePlaceholder]) => {
const unsafe = [];
if (template.includes(`{${placeholder}}`)) unsafe.push(`{${placeholder}} -> {${safePlaceholder}}`);
if (template.includes(`{${placeholder}_raw}`)) unsafe.push(`{${placeholder}_raw} -> {${safePlaceholder}}`);
return unsafe;
});
if (unsafePlaceholders.length > 0) {
throw new Error(
`launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}`
);
}
}Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| throw error; | ||
| } | ||
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 50); | ||
| } |
There was a problem hiding this comment.
Atomics.wait throws TypeError on the main thread
Atomics.wait is only permitted inside Worker threads. When called from the main Node.js thread it throws:
TypeError: Main thread is not allowed to wait on condition variables.
This means that every time a retryable error (EPERM, EBUSY, ENOTEMPTY) occurs, the catch block will itself throw before the retry loop can continue, completely defeating the intent of the retry logic.
The canonical synchronous-sleep workaround in Node.js is a tight Atomics.waitAsync/setImmediate loop or, more simply, using Atomics.waitAsync combined with an async function. However, since cleanupTestDir is called from a synchronous test function, the simplest fix is to use a busy-wait with a small timeout or adopt the same Atomics.waitAsync-based helper used elsewhere in the project:
| } | |
| Atomics.waitAsync(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 50).value; |
Or, if the test runner already has an async sleep helper available, replace with:
const end = Date.now() + 50;
while (Date.now() < end) { /* busy-wait */ }|
|
||
| function validateLauncherCommand(template) { | ||
| const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS) | ||
| .filter(([placeholder]) => template.includes(`{${placeholder}}`)) | ||
| .map(([placeholder, safePlaceholder]) => `{${placeholder}} -> {${safePlaceholder}}`); | ||
|
|
||
| if (unsafePlaceholders.length > 0) { | ||
| throw new Error( | ||
| `launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| function buildSessionBannerCommand(sessionName, coordinationDir) { |
There was a problem hiding this comment.
validateLauncherCommand does not block _raw placeholder variants
buildTemplateVariables registers three keys per path variable: {key} (unquoted), {key_raw} (also unquoted, identical value), and {key_sh} (shell-quoted). The validator only checks the bare {task_file}, {handoff_file}, etc. forms. A launcherCommand that uses {task_file_raw} instead of {task_file} will pass validation while still injecting the raw, unquoted path into the shell — making the check trivially bypassable.
The _raw variants carry the same injection risk and should be blocked too:
function validateLauncherCommand(template) {
const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS)
.flatMap(([placeholder, safePlaceholder]) => {
const unsafe = [];
if (template.includes(`{${placeholder}}`))
unsafe.push(`{${placeholder}} -> {${safePlaceholder}}`);
if (template.includes(`{${placeholder}_raw}`))
unsafe.push(`{${placeholder}_raw} -> {${safePlaceholder}}`);
return unsafe;
});
if (unsafePlaceholders.length > 0) {
throw new Error(
`launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}`
);
}
}| return `${driveRoot}${remainder.replace(/\//g, '\\')}`; | ||
| } | ||
|
|
||
| function toBashPath(filePath) { | ||
| if (process.platform !== 'win32') { | ||
| return filePath; | ||
| } | ||
|
|
||
| const cacheKey = String(filePath); | ||
| if (bashPathCache.has(cacheKey)) { | ||
| return bashPathCache.get(cacheKey); | ||
| } |
There was a problem hiding this comment.
fallbackFromBashPath misidentifies short Unix paths as Windows drive paths
The second regex /^\/([a-z])(?:\/(.*))?$/i matches any absolute Unix path whose first path component is a single ASCII letter. For example, /tmp/build would match with drive letter t and remainder mp/build, yielding T:\mp\build. This silent data corruption affects any non-Windows caller that passes a generic Unix path (e.g. /tmp, /usr) when neither cygpath nor wslpath is available.
A guard that verifies the inferred drive root actually exists on the Windows filesystem already partially mitigates this (the fs.existsSync(driveRoot) check), but it still silently corrupts paths where coincidentally a Windows drive with that letter does exist (e.g. a T: drive on a developer machine).
The regex should require that the single-letter component is preceded and followed only by / at the root, and ideally be restricted to well-known drive-letter conventions rather than the full [a-z] range, or at minimum document clearly that this function must only be called for known bash-format drive paths:
// Only apply bare-drive heuristic when the path looks like a MSYS2/Git-Bash drive root
const bareDriveMatch = normalized.match(/^\/([a-z])\/(.*)$/i);
if (!bareDriveMatch) {
return filePath;
}This form requires at least one character after /<letter>/, ruling out short paths like /t while still matching /c/Users/foo.
|
Thanks for the update. This PR currently has merge conflicts with main. Please rebase on the latest main and push an updated branch, then we can continue review. |
|
Analysis Failed
Troubleshooting
Retry: |
Merged in main (Y) |
| if (!retryableCodes.has(error.code) || attempt === 4) { | ||
| throw error; | ||
| } | ||
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 50); |
There was a problem hiding this comment.
Atomics.wait throws on the Node.js main thread
Atomics.wait is forbidden on the main thread in Node.js and unconditionally throws:
TypeError: Main thread is not allowed to wait on condition variables.
This means that as soon as any of the retryable error codes (EPERM, EBUSY, ENOTEMPTY) is hit on the first four attempts, the catch block itself throws TypeError before the retry loop can continue, completely defeating the retry intent and causing the enclosing test to fail with a confusing error message.
A cross-platform synchronous sleep that works on the main thread:
// Busy-wait for ~50ms without Atomics.wait
const end = Date.now() + 50;
while (Date.now() < end) { /* spin */ }Or use spawnSync with a short sleep call for a more idle wait:
spawnSync(process.platform === 'win32' ? 'ping' : 'sleep',
process.platform === 'win32' ? ['127.0.0.1', '-n', '1'] : ['0.05'],
{ stdio: 'ignore' });| function validateLauncherCommand(template) { | ||
| const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS) | ||
| .filter(([placeholder]) => template.includes(`{${placeholder}}`)) | ||
| .map(([placeholder, safePlaceholder]) => `{${placeholder}} -> {${safePlaceholder}}`); | ||
|
|
||
| if (unsafePlaceholders.length > 0) { | ||
| throw new Error( | ||
| `launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
validateLauncherCommand can be bypassed with _raw placeholder variants
buildTemplateVariables registers three keys per path variable: {key} (unquoted), {key_raw} (also unquoted, identical value), and {key_sh} (shell-quoted). The current validator only inspects the bare names (task_file, handoff_file, etc.) and ignores the _raw variants. A launcher command using {task_file_raw} instead of {task_file} passes validation while still injecting the raw, unquoted path into the shell — making the safety check trivially bypassable.
The _raw variants carry the same injection risk and must also be blocked:
function validateLauncherCommand(template) {
const unsafePlaceholders = Object.entries(SHELL_PATH_PLACEHOLDERS)
.flatMap(([placeholder, safePlaceholder]) => {
const unsafe = [];
if (template.includes(`{${placeholder}}`))
unsafe.push(`{${placeholder}} -> {${safePlaceholder}}`);
if (template.includes(`{${placeholder}_raw}`))
unsafe.push(`{${placeholder}_raw} -> {${safePlaceholder}}`);
return unsafe;
});
if (unsafePlaceholders.length > 0) {
throw new Error(
`launcherCommand must use shell-safe path placeholders: ${unsafePlaceholders.join(', ')}`
);
}
}| if (!fs.existsSync(driveRoot)) { | ||
| return filePath; | ||
| } | ||
|
|
||
| return `${driveRoot}${remainder.replace(/\//g, '\\')}`; | ||
| } | ||
|
|
||
| function toBashPath(filePath) { | ||
| if (process.platform !== 'win32') { | ||
| return filePath; | ||
| } | ||
|
|
There was a problem hiding this comment.
fallbackFromBashPath misidentifies short Unix paths as Windows drive letters
The second regex /^\/([a-z])(?:\/(.*))?$/i matches any absolute Unix path whose first path component is a single ASCII letter. For example, /tmp/build would match with a drive letter of t and a remainder of mp/build, potentially yielding T:\mp\build if a T: drive happens to exist on the host.
The fs.existsSync(driveRoot) guard reduces the likelihood, but it does not eliminate it — on a machine with a T: drive, /tmp would silently be corrupted to T:\mp. Since the function is part of a shared test library, any future caller passing a generic Unix path hits this footgun.
Consider restricting the regex to only the two well-known drive-letter positions (single letter followed by / or end-of-string at the root) and tightening the character class, or add an explicit guard that rejects paths where the inferred drive root cannot plausibly correspond to the original input:
// Only treat /X/... as a Windows drive path when the inferred
// Windows path actually exists and looks like a drive root (e.g. C:\)
const bareDriveMatch = normalized.match(/^\/([a-zA-Z])(\/.*)?$/);
if (!bareDriveMatch) {
return filePath;
}
const [, driveLetter, rest = ''] = bareDriveMatch;
const driveRoot = `${driveLetter.toUpperCase()}:\\`;
// Require the drive to exist AND that the remainder is non-empty or explicitly empty
if (!fs.existsSync(driveRoot) || (rest.length === 0 && normalized.length > 2)) {
return filePath;
}
|
Thanks for merging main, but there are still merge conflicts showing on this PR. Could you rebase on the latest main (several PRs were just merged) and force-push? The only CI failure is a flaky Windows/Node22/yarn test which shouldn't block merge. |
|
Closing — superseded by recent changes. Lint and test fixes have been addressed in later PRs. |
What Changed
This pull request introduces several improvements to cross-platform path normalization and environment handling, particularly for shell scripts and Node.js utilities. It also updates documentation to reflect new agent, skill, and command counts, and makes enhancements to test robustness and error handling. The most important changes are grouped below.
Cross-platform path normalization and environment handling:
orchestrate-codex-worker.sh,continuous-learning-v2/hooks/observe.sh,continuous-learning-v2/scripts/detect-project.sh) to handle Windows, WSL, and Unix paths, and ensure environment variables likeHOME,USERPROFILE, andCLAUDE_PROJECT_DIRare consistently normalized and exported.fromBashPath,toBashPath, andshellQuotefunctions intests/hooks/hooks.test.js, and added similar logic totests/scripts/orchestrate-codex-worker.test.jsfor test reliability across platforms.getHomeDirinscripts/lib/utils.jsto more reliably detect the user's home directory across platforms.Orchestration and worker management enhancements:
buildOrchestrationPlaninscripts/lib/tmux-worktree-orchestrator.jsto enforce unique worker slugs, use the newbuildTemplateVariablesfor quoting, and ensure template variables are consistently handled.Testing and validation improvements:
Documentation updates:
AGENTS.mdandREADME.mdto reflect new totals (25 agents, 108 skills, 57 commands), and revised project structure descriptions.Miscellaneous improvements:
scripts/lib/skill-improvement/observations.jsforerrorandfeedbackfields.skills/rust-patterns/SKILL.mdand removal of unused import intests/lib/skill-dashboard.test.js.These changes collectively improve reliability, portability, and clarity for both shell and Node.js workflows, and ensure the documentation matches the current capabilities of the project.
Why This Change
Testing Done
node tests/run-all.js)Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
Documentation
Summary by cubic
Fix cross-platform path and env handling so shell hooks, the orchestrator, and Node tests work reliably on Windows/WSL/Cygwin. Enforces shell-safe
*_shpath placeholders, exports resolved project vars, and hardens Windows bash path helpers and assertions.Bug Fixes
scripts/orchestrate-codex-worker.sh,skills/continuous-learning-v2/hooks/observe.sh, andskills/continuous-learning-v2/scripts/detect-project.sh; normalize/exportHOME/USERPROFILE/CLAUDE_PROJECT_DIR; fixSTDIN_CWD.detect-project.sh: resolve Python viacommand -v(supports absolute paths); exportPROJECT_ID/PROJECT_NAME/PROJECT_ROOT/PROJECT_DIR; migrate legacy env-hash directories when normalization changes the ID; prefer Python hashing with shasum fallbacks.*_shpath placeholders (validatelauncherCommand); usebuildTemplateVariableswithshellQuote; updatescripts/orchestrate-worktrees.jsusage to prefer*_sh.getHomeDirprecedence; stricter input validation inobservations.js; add.gitattributesto force LF on*.sh; docs: update examples to use*_shand add new agents inAGENTS.md.Tests
bash -lcwith env exported via safe quoting; factortoBashPath/fromBashPathintotests/lib/bash-paths.jswithcygpath/wslpathand caching; read exports viaenv | grep; canonicalize realpaths in assertions; retryingcleanupTestDir; setHOMEandUSERPROFILE.*_shplaceholder enforcement, legacy env-hash migration on normalized env paths, shebang stripping in validators,getHomeDirbehavior, observation input validation, and bash path handling in worker/orchestrator flows.Written for commit 75eab79. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation