fix: harden hook portability and plugin docs#371
Conversation
📝 WalkthroughWalkthroughThis PR updates hooks, shell scripts, continuous-learning tooling, tests, and documentation: it adds project-root and package-manager-aware formatter runner logic, improves dev-server command parsing to avoid heredoc false positives, makes Python invocation portable across platforms, derives plugin/script roots more robustly, clarifies agent/install docs, and expands tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as post-edit-format.js
participant FR as findProjectRoot()
participant FM as detectFormatter()
participant GR as getFormatterRunner()
participant PM as PackageManager
participant Exec as FormatterExec
Hook->>FR: findProjectRoot(filePath)
FR->>FR: walk parents for PROJECT_ROOT_MARKERS
FR-->>Hook: projectRoot
Hook->>FM: detectFormatter(projectRoot)
FM-->>Hook: formatterType (biome / prettier)
Hook->>GR: getFormatterRunner(projectRoot)
GR->>PM: detect package manager / node_modules bin
PM-->>GR: chosen runner (npx / pnpm dlx / bunx / etc.)
GR->>GR: getRunnerBin() (apply .cmd on Windows)
GR-->>Hook: {bin, prefixArgs}
Hook->>Exec: construct command with bin + prefix + formatter args
Exec-->>Hook: run formatter (Windows-aware exec)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 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/hooks/observe.sh">
<violation number="1" location="skills/continuous-learning-v2/hooks/observe.sh:86">
P1: This re-assignment bypasses the `command -v` validation performed by `resolve_python_cmd()`. If `CLV2_PYTHON_CMD` is set to a non-existent binary, `resolve_python_cmd` correctly falls back to `python3`/`python`, but this line overrides `PYTHON_CMD` back to the invalid value because `${CLV2_PYTHON_CMD:-...}` only checks for empty/unset, not executability.
Since `detect-project.sh` stores its validated result in `_CLV2_PYTHON_CMD` (underscore-prefixed), either reference that variable or remove this line since `PYTHON_CMD` is already correctly resolved.</violation>
</file>
<file name="skills/continuous-learning-v2/scripts/detect-project.sh">
<violation number="1" location="skills/continuous-learning-v2/scripts/detect-project.sh:46">
P2: The resolved Python path is stored in `_CLV2_PYTHON_CMD`, but the export targets `CLV2_PYTHON_CMD` (no leading underscore) which is never assigned the resolved value. This means the export is always empty, so child processes and re-sources will never hit the `CLV2_PYTHON_CMD` override fast-path in `_clv2_resolve_python_cmd`—they'll re-probe `command -v` every time.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Source shared project detection helper | ||
| # This sets: PROJECT_ID, PROJECT_NAME, PROJECT_ROOT, PROJECT_DIR | ||
| source "${SKILL_ROOT}/scripts/detect-project.sh" | ||
| PYTHON_CMD="${CLV2_PYTHON_CMD:-$PYTHON_CMD}" |
There was a problem hiding this comment.
P1: This re-assignment bypasses the command -v validation performed by resolve_python_cmd(). If CLV2_PYTHON_CMD is set to a non-existent binary, resolve_python_cmd correctly falls back to python3/python, but this line overrides PYTHON_CMD back to the invalid value because ${CLV2_PYTHON_CMD:-...} only checks for empty/unset, not executability.
Since detect-project.sh stores its validated result in _CLV2_PYTHON_CMD (underscore-prefixed), either reference that variable or remove this line since PYTHON_CMD is already correctly resolved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/continuous-learning-v2/hooks/observe.sh, line 86:
<comment>This re-assignment bypasses the `command -v` validation performed by `resolve_python_cmd()`. If `CLV2_PYTHON_CMD` is set to a non-existent binary, `resolve_python_cmd` correctly falls back to `python3`/`python`, but this line overrides `PYTHON_CMD` back to the invalid value because `${CLV2_PYTHON_CMD:-...}` only checks for empty/unset, not executability.
Since `detect-project.sh` stores its validated result in `_CLV2_PYTHON_CMD` (underscore-prefixed), either reference that variable or remove this line since `PYTHON_CMD` is already correctly resolved.</comment>
<file context>
@@ -58,6 +83,7 @@ SKILL_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
# Source shared project detection helper
# This sets: PROJECT_ID, PROJECT_NAME, PROJECT_ROOT, PROJECT_DIR
source "${SKILL_ROOT}/scripts/detect-project.sh"
+PYTHON_CMD="${CLV2_PYTHON_CMD:-$PYTHON_CMD}"
# ─────────────────────────────────────────────
</file context>
| PYTHON_CMD="${CLV2_PYTHON_CMD:-$PYTHON_CMD}" | |
| PYTHON_CMD="${_CLV2_PYTHON_CMD:-$PYTHON_CMD}" |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1737a42a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| _CLV2_PYTHON_CMD="$(_clv2_resolve_python_cmd 2>/dev/null || true)" | ||
| export CLV2_PYTHON_CMD |
There was a problem hiding this comment.
detect-project.sh resolves the interpreter into _CLV2_PYTHON_CMD, but the new export CLV2_PYTHON_CMD line never assigns that value to CLV2_PYTHON_CMD. In a normal shell where CLV2_PYTHON_CMD is initially unset, scripts that consume CLV2_PYTHON_CMD (notably start-observer.sh) now believe Python is unavailable, skip reading config.json, and fall back to defaults (including observer.enabled=false) even when python3 exists.
Useful? React with 👍 / 👎.
| if (!commandWord || !DEV_COMMAND_WORDS.has(commandWord)) { | ||
| return false; |
There was a problem hiding this comment.
The new commandWord gate introduces false negatives: commands like nohup npm run dev, /usr/bin/env npm run dev, or /usr/bin/npm run dev now pass because their leading token is not in DEV_COMMAND_WORDS, even though they still launch a dev server outside tmux. Before this change, these were blocked by the regex check; now the policy can be bypassed with common wrappers.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
skills/iterative-retrieval/SKILL.md (1)
211-211: Consider adding specific agent paths for consistency.The change correctly removes the misleading
~/.claude/agents/reference and clarifies that agents are bundled with ECC. However, for consistency with the commands/*.md files (e.g.,commands/plan.mdline 115 specifiesagents/planner.md), consider listing specific agent file paths if this note refers to particular agents.If this is intentionally a general reference to all agents, the current wording is acceptable.
✨ Optional specificity improvement
If specific agents are relevant to this skill, consider:
-- Agent definitions bundled with ECC (manual install path: `agents/`) +- Agent definitions bundled with ECC (manual install paths: `agents/planner.md`, `agents/code-reviewer.md`, etc.)Otherwise, the current general reference is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/iterative-retrieval/SKILL.md` at line 211, Update the note "Agent definitions bundled with ECC (manual install path: `agents/`)" in SKILL.md to either list the exact agent filenames referenced elsewhere for consistency (e.g., agents/planner.md, agents/retriever.md — matching the pattern used in commands/*.md such as commands/plan.md) or explicitly state that it is a general reference to all agents; change the text to include the chosen specific agent paths or add a clarifying phrase that this refers to the entire agents/ directory so readers know whether to look for particular files like agents/planner.md.
🤖 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/hooks/post-edit-format.js`:
- Around line 51-65: The current findProjectRoot stops at the first
PROJECT_ROOT_MARKERS match (package.json) which causes formatter detection to
miss ancestor BIOME/PRETTIER configs; change findProjectRoot to walk up from
startDir scanning each directory for BIOME_CONFIGS and PRETTIER_CONFIGS first
and return immediately if any formatter config is found, but if you encounter a
PROJECT_ROOT_MARKERS entry (e.g., package.json) store that directory as a
fallback and continue walking; after the loop return the nearest stored
PACKAGE_JSON directory if no formatter config was found, otherwise return
startDir as before. Ensure references to PROJECT_ROOT_MARKERS, BIOME_CONFIGS,
PRETTIER_CONFIGS, and findProjectRoot are used so the updated logic locates
formatter configs preferentially while treating package.json as fallback.
In `@scripts/hooks/pre-bash-dev-server-block.js`:
- Around line 7-18: The allowlist in DEV_COMMAND_WORDS is too broad and includes
shell interpreters causing false positives; update the code to remove shell
interpreter entries ('bash', 'sh', 'zsh', 'fish') from DEV_COMMAND_WORDS so
getLeadingCommandWord() returns only real dev-launchers (npm, pnpm, yarn, bun,
npx, tmux) before applying devPattern.test(segment); alternatively (if you
prefer keeping shells) modify getLeadingCommandWord()/the calling logic to
detect shell wrappers and extract the -c/-lc payload (parse the quoted/ heredoc
payload) and run devPattern.test() against that extracted payload instead—apply
one of these fixes and update/ add tests covering a bash -lc '...heredoc...'
case to prevent regressions.
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 45-46: The script assigns the resolved interpreter to a local
variable _CLV2_PYTHON_CMD but exports CLV2_PYTHON_CMD without setting it, so
downstream scripts (observe.sh, start-observer.sh) get an empty/stale value; fix
by assigning the resolved path into CLV2_PYTHON_CMD before exporting (use the
result of calling _clv2_resolve_python_cmd, preserving the 2>/dev/null || true
behavior) so that CLV2_PYTHON_CMD holds the actual interpreter path when
exported.
In `@tests/hooks/hooks.test.js`:
- Around line 1683-1693: Add a runtime check in tests/hooks/hooks.test.js to
source the detect-project.sh script and assert that the exported CLV2_PYTHON_CMD
is non-empty at runtime (not just present as text); specifically, after reading
detectProjectSource, spawn a shell that sources detect-project.sh and echoes or
returns CLV2_PYTHON_CMD, then assert the result is a non-empty string. Also
validate the underlying bug by checking that the resolved variable name
_CLV2_PYTHON_CMD is actually exported to CLV2_PYTHON_CMD in detect-project.sh
(fix the script if necessary by exporting the correct variable), referencing
detect-project.sh, CLV2_PYTHON_CMD, and _CLV2_PYTHON_CMD so the test catches the
missing export.
---
Nitpick comments:
In `@skills/iterative-retrieval/SKILL.md`:
- Line 211: Update the note "Agent definitions bundled with ECC (manual install
path: `agents/`)" in SKILL.md to either list the exact agent filenames
referenced elsewhere for consistency (e.g., agents/planner.md,
agents/retriever.md — matching the pattern used in commands/*.md such as
commands/plan.md) or explicitly state that it is a general reference to all
agents; change the text to include the chosen specific agent paths or add a
clarifying phrase that this refers to the entire agents/ directory so readers
know whether to look for particular files like agents/planner.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 793b6989-9ee4-474c-ac5d-7bf742985892
📒 Files selected for processing (11)
commands/e2e.mdcommands/plan.mdcommands/tdd.mdscripts/hooks/post-edit-format.jsscripts/hooks/pre-bash-dev-server-block.jsscripts/hooks/run-with-flags-shell.shskills/continuous-learning-v2/agents/start-observer.shskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shskills/iterative-retrieval/SKILL.mdtests/hooks/hooks.test.js
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 (1)
.opencode/index.ts (1)
52-56:⚠️ Potential issue | 🟠 MajorUpdate the schema to include the
featuresobject or remove undocumented metadata properties.The exported metadata at line 56 adds
features.configAssetsand other nested properties (hookEvents,customTools), butschemas/plugin.schema.jsondoes not define afeaturesobject at all. The schema only recognizes:agents,author,description,homepage,keywords,license,name,repository,skills,version.This is a contract mismatch: either the schema should be updated to document the
featuresshape (includingconfigAssets,hookEvents, andcustomTools), or the metadata should not advertise capabilities that fall outside the schema definition. If external tools or future OpenCode versions validate plugin metadata against this schema, the undocumented fields will be rejected or silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/index.ts around lines 52 - 56, The exported metadata includes a top-level features object (the features property in the module export) that is not declared in the plugin schema (schemas/plugin.schema.json); update the schema to declare a features object with the nested properties seen in the export (e.g., agents: integer, commands: integer, skills: integer, configAssets: boolean, hookEvents: boolean, customTools: boolean) including descriptions and any required/optional constraints, or alternatively remove those undocumented properties from the exported metadata so it conforms to the existing schema; locate the features object in the export and either remove those keys or add a corresponding "features" definition in the schema to resolve the contract mismatch.
🧹 Nitpick comments (1)
tests/hooks/hooks.test.js (1)
99-105: Consider defensive JSON parsing inreadCommandLog.If the log file contains a malformed line (e.g., from a partially written entry during a crash),
JSON.parse(line)will throw and fail the test unexpectedly. Wrapping in try-catch would make tests more robust.🛡️ Proposed defensive parsing
function readCommandLog(logFile) { if (!fs.existsSync(logFile)) return []; return fs.readFileSync(logFile, 'utf8') .split('\n') .filter(Boolean) - .map(line => JSON.parse(line)); + .map(line => { + try { + return JSON.parse(line); + } catch { + return null; + } + }) + .filter(Boolean); }🤖 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 99 - 105, The readCommandLog function can throw when JSON.parse encounters a malformed line; modify readCommandLog to defensively parse each non-empty line using try/catch (inside the map or by reducing), skipping or ignoring lines that fail to parse so the function returns only successfully parsed objects instead of throwing; keep the existing fs.existsSync and readFileSync flow and ensure the return shape remains an array of parsed entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/README.md:
- Around line 40-42: The README omission: update the manual setup instructions
to explicitly include copying the `.opencode/instructions/` directory and adding
the corresponding `instructions` config entry alongside `agent` and `command`;
modify the list that currently mentions only `.opencode/commands/`,
`.opencode/prompts/` and `agent` / `command` so it also references
`.opencode/instructions/` and the `instructions` array in the project config to
ensure copied instruction assets are loaded.
In `@README.md`:
- Around line 1056-1058: Update the README blurb about wiring OpenCode to
explicitly mention adding the "instructions" entries in opencode.json in
addition to "agent" and "command" so the curated instruction/skill bundle is
loaded; reference the opencode.json keys ("agent", "command", "instructions")
and instruct users to copy the bundled .opencode/ assets and include the
"instructions" section when copying/wiring the config to avoid a partial setup.
---
Outside diff comments:
In @.opencode/index.ts:
- Around line 52-56: The exported metadata includes a top-level features object
(the features property in the module export) that is not declared in the plugin
schema (schemas/plugin.schema.json); update the schema to declare a features
object with the nested properties seen in the export (e.g., agents: integer,
commands: integer, skills: integer, configAssets: boolean, hookEvents: boolean,
customTools: boolean) including descriptions and any required/optional
constraints, or alternatively remove those undocumented properties from the
exported metadata so it conforms to the existing schema; locate the features
object in the export and either remove those keys or add a corresponding
"features" definition in the schema to resolve the contract mismatch.
---
Nitpick comments:
In `@tests/hooks/hooks.test.js`:
- Around line 99-105: The readCommandLog function can throw when JSON.parse
encounters a malformed line; modify readCommandLog to defensively parse each
non-empty line using try/catch (inside the map or by reducing), skipping or
ignoring lines that fail to parse so the function returns only successfully
parsed objects instead of throwing; keep the existing fs.existsSync and
readFileSync flow and ensure the return shape remains an array of parsed
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d046be68-b28e-44e7-8af1-12159c2a8bc7
📒 Files selected for processing (5)
.opencode/MIGRATION.md.opencode/README.md.opencode/index.tsREADME.mdtests/hooks/hooks.test.js
✅ Files skipped from review due to trivial changes (1)
- .opencode/MIGRATION.md
There was a problem hiding this comment.
2 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="tests/hooks/hooks.test.js">
<violation number="1" location="tests/hooks/hooks.test.js:1760">
P2: Dead ternary — both branches evaluate to `'bash'`. If the intent was to vary the shell on Windows, fix the win32 branch; otherwise remove the conditional.</violation>
</file>
<file name="scripts/hooks/post-edit-format.js">
<violation number="1" location="scripts/hooks/post-edit-format.js:124">
P1: Potential command injection on Windows: `spawnSync` with `shell: true` passes the unsanitized `filePath` through `cmd.exe`, where shell metacharacters (`&`, `|`, `>`, etc.) in the path can execute arbitrary commands. Validate or quote the file path before passing it to a shell-enabled spawn.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| function runFormatterCommand(cmd, projectRoot) { | ||
| if (process.platform === 'win32' && cmd.bin.endsWith('.cmd')) { | ||
| const result = spawnSync(cmd.bin, cmd.args, { |
There was a problem hiding this comment.
P1: Potential command injection on Windows: spawnSync with shell: true passes the unsanitized filePath through cmd.exe, where shell metacharacters (&, |, >, etc.) in the path can execute arbitrary commands. Validate or quote the file path before passing it to a shell-enabled spawn.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/post-edit-format.js, line 124:
<comment>Potential command injection on Windows: `spawnSync` with `shell: true` passes the unsanitized `filePath` through `cmd.exe`, where shell metacharacters (`&`, `|`, `>`, etc.) in the path can execute arbitrary commands. Validate or quote the file path before passing it to a shell-enabled spawn.</comment>
<file context>
@@ -114,6 +119,33 @@ function getFormatterCommand(formatter, filePath, projectRoot) {
+function runFormatterCommand(cmd, projectRoot) {
+ if (process.platform === 'win32' && cmd.bin.endsWith('.cmd')) {
+ const result = spawnSync(cmd.bin, cmd.args, {
+ cwd: projectRoot,
+ shell: true,
</file context>
| 'printf "%s\\n" "${CLV2_PYTHON_CMD:-}"' | ||
| ].join('; '); | ||
|
|
||
| const shell = process.platform === 'win32' ? 'bash' : 'bash'; |
There was a problem hiding this comment.
P2: Dead ternary — both branches evaluate to 'bash'. If the intent was to vary the shell on Windows, fix the win32 branch; otherwise remove the conditional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/hooks/hooks.test.js, line 1760:
<comment>Dead ternary — both branches evaluate to `'bash'`. If the intent was to vary the shell on Windows, fix the win32 branch; otherwise remove the conditional.</comment>
<file context>
@@ -1710,6 +1750,33 @@ async function runTests() {
+ 'printf "%s\\n" "${CLV2_PYTHON_CMD:-}"'
+ ].join('; ');
+
+ const shell = process.platform === 'win32' ? 'bash' : 'bash';
+ const proc = spawn(shell, ['-lc', shellCommand], {
+ env: process.env,
</file context>
| const shell = process.platform === 'win32' ? 'bash' : 'bash'; | |
| const shell = 'bash'; |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.opencode/README.md:
- Around line 40-42: The README's manual-copy instructions omit the skills/
directory, causing missing SKILL.md files referenced by instructions; update the
sentence that lists `.opencode/commands/`, `.opencode/prompts/`,
`.opencode/instructions/` to also include `.opencode/skills/` and mention
copying the `skills` config entries (or explicitly state to copy the entire
`.opencode/skills/` tree) so the `instructions` entries referencing
`skills/.../SKILL.md` resolve correctly.
- Around line 44-47: The README currently shows only the npm-specific example
"npx ecc-install typescript"; update that example to include
package-manager-specific variants or a neutral instruction: add lines showing
"npx ecc-install typescript", "bunx ecc-install typescript", and "yarn dlx
ecc-install typescript" (or replace the single example with a
package-manager-agnostic note like "use your package manager's exec/dlx command
to run ecc-install"), referencing the ecc-install command so Bun and Yarn users
have clear examples.
In `@scripts/hooks/post-edit-format.js`:
- Around line 93-117: The runner currently always uses the package-manager
execCmd (e.g., "pnpm dlx" / "yarn dlx") which bypasses project-local binaries;
update getFormatterCommand to prefer a project-local binary under
projectRoot/node_modules/.bin (check existence/readability using fs.existsSync
or fs.access) for the formatter binary name ("biome" for `@biomejs/biome`,
"prettier" for prettier) and return that binary (no prefix) when present; only
if the local binary is missing, fall back to the existing behavior that uses
getFormatterRunner(projectRoot) with runner.bin and runner.prefix plus the
package name/args. Ensure you reference getFormatterRunner and keep
formatter-specific package names (`@biomejs/biome` and prettier) when constructing
fallback args.
In `@tests/hooks/hooks.test.js`:
- Around line 1753-1778: The test currently requires CLV2_PYTHON_CMD to be
non-empty after sourcing detect-project.sh which fails on systems with no
python; change the test so it always asserts the script sources cleanly (keep
the assert.strictEqual(code, 0, ...)) but only assert that stdout.trim().length
> 0 when a Python interpreter is actually available: before asserting on
CLV2_PYTHON_CMD, detect interpreter availability (e.g., probe for `python3` then
`python` via a synchronous which/command check using child_process.spawnSync or
similar), and if an interpreter is found assert the exported CLV2_PYTHON_CMD
matches that resolved command or is non-empty; if no interpreter is found, skip
the non-empty assertion (or assert that the export is empty). Update the
asyncTest block that sources detect-project.sh and references CLV2_PYTHON_CMD
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dffa7d99-d406-4f7d-b82b-118883f25e7a
📒 Files selected for processing (7)
.opencode/README.mdREADME.mdschemas/plugin.schema.jsonscripts/hooks/post-edit-format.jsscripts/hooks/pre-bash-dev-server-block.jsskills/continuous-learning-v2/scripts/detect-project.shtests/hooks/hooks.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- scripts/hooks/pre-bash-dev-server-block.js
| It does **not** auto-register the full ECC command/agent/instruction catalog in your project config. For the full OpenCode setup, either: | ||
| - run OpenCode inside this repository, or | ||
| - copy the relevant `.opencode/commands/`, `.opencode/prompts/`, `.opencode/instructions/`, and the `instructions`, `agent`, and `command` config entries into your own project |
There was a problem hiding this comment.
Include skills/ in the manual-copy instructions.
Line 42 still leaves out the skills/ assets that the documented instructions entries reference later in this README (skills/.../SKILL.md on Lines 193-195). As written, the “full OpenCode setup” path can still produce missing instruction files.
✏️ Suggested wording
-It does **not** auto-register the full ECC command/agent/instruction catalog in your project config. For the full OpenCode setup, either:
+It does **not** auto-register the full ECC command/agent/instruction catalog in your project config. For the full OpenCode setup, either:
- run OpenCode inside this repository, or
-- copy the relevant `.opencode/commands/`, `.opencode/prompts/`, `.opencode/instructions/`, and the `instructions`, `agent`, and `command` config entries into your own project
+- copy the relevant `.opencode/commands/`, `.opencode/prompts/`, `.opencode/instructions/`, `skills/`, and the `instructions`, `agent`, and `command` config entries into your own project🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/README.md around lines 40 - 42, The README's manual-copy
instructions omit the skills/ directory, causing missing SKILL.md files
referenced by instructions; update the sentence that lists
`.opencode/commands/`, `.opencode/prompts/`, `.opencode/instructions/` to also
include `.opencode/skills/` and mention copying the `skills` config entries (or
explicitly state to copy the entire `.opencode/skills/` tree) so the
`instructions` entries referencing `skills/.../SKILL.md` resolve correctly.
| After installation, the `ecc-install` CLI is also available: | ||
|
|
||
| ```bash | ||
| npx ecc-install typescript |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the .opencode/README.md file exists and examine the relevant section
find . -name "README.md" -path "*opencode*" | head -20Repository: affaan-m/everything-claude-code
Length of output: 96
🏁 Script executed:
# Also, let's look for the specific file mentioned
git ls-files | grep -i "opencode" | grep -i readmeRepository: affaan-m/everything-claude-code
Length of output: 94
🏁 Script executed:
# Check the repository structure around opencode directory
fd "README" | grep -E "(opencode|ecc)" | head -20Repository: affaan-m/everything-claude-code
Length of output: 57
🏁 Script executed:
# Read the .opencode/README.md file to see the actual content around lines 44-47
cat -n .opencode/README.md | head -60Repository: affaan-m/everything-claude-code
Length of output: 2064
🏁 Script executed:
# Check if there are other examples of package manager invocation patterns in documentation
rg "npm|bun|yarn|pnpm" .opencode/README.md | head -20Repository: affaan-m/everything-claude-code
Length of output: 460
🏁 Script executed:
# Also check the main README for how package managers are handled
rg -A2 -B2 "npx|bunx|yarn" README.md | head -30Repository: affaan-m/everything-claude-code
Length of output: 853
Provide package-manager-specific examples for the ecc-install command.
Line 15 documents support for "npm/bun/yarn", but the example on line 47 uses only npx ecc-install typescript, which is npm-specific. Bun users should see bunx ecc-install typescript, and Yarn users may need alternative syntax. Either show all three variants or use a package-manager-agnostic instruction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.opencode/README.md around lines 44 - 47, The README currently shows only
the npm-specific example "npx ecc-install typescript"; update that example to
include package-manager-specific variants or a neutral instruction: add lines
showing "npx ecc-install typescript", "bunx ecc-install typescript", and "yarn
dlx ecc-install typescript" (or replace the single example with a
package-manager-agnostic note like "use your package manager's exec/dlx command
to run ecc-install"), referencing the ecc-install command so Bun and Yarn users
have clear examples.
| function getFormatterRunner(projectRoot) { | ||
| const pm = getPackageManager({ projectDir: projectRoot }); | ||
| const execCmd = pm?.config?.execCmd || 'npx'; | ||
| const [bin = 'npx', ...prefix] = execCmd.split(/\s+/).filter(Boolean); | ||
|
|
||
| return { | ||
| bin: getRunnerBin(bin), | ||
| prefix | ||
| }; | ||
| } | ||
|
|
||
| function getFormatterCommand(formatter, filePath, projectRoot) { | ||
| const runner = getFormatterRunner(projectRoot); | ||
|
|
||
| if (formatter === 'biome') { | ||
| return { bin: npxBin, args: ['@biomejs/biome', 'format', '--write', filePath] }; | ||
| return { | ||
| bin: runner.bin, | ||
| args: [...runner.prefix, '@biomejs/biome', 'format', '--write', filePath] | ||
| }; | ||
| } | ||
| if (formatter === 'prettier') { | ||
| return { bin: npxBin, args: ['prettier', '--write', filePath] }; | ||
| return { | ||
| bin: runner.bin, | ||
| args: [...runner.prefix, 'prettier', '--write', filePath] | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "post-edit-format.js" -type fRepository: affaan-m/everything-claude-code
Length of output: 110
🏁 Script executed:
find . -name "package-manager.js" -type fRepository: affaan-m/everything-claude-code
Length of output: 107
🏁 Script executed:
wc -l ./scripts/hooks/post-edit-format.jsRepository: affaan-m/everything-claude-code
Length of output: 114
🏁 Script executed:
cat -n ./scripts/hooks/post-edit-format.jsRepository: affaan-m/everything-claude-code
Length of output: 5699
🏁 Script executed:
cat -n ./scripts/lib/package-manager.jsRepository: affaan-m/everything-claude-code
Length of output: 15103
Prefer the project-local formatter before dlx/bunx fallback.
getPackageManager() returns execCmd values like pnpm dlx and yarn dlx, so this code unconditionally uses those runners instead of checking the repo's installed formatter first. Unlike npx, pnpm dlx and yarn dlx ignore local node_modules/.bin/ binaries and always download the package. This can pull a different formatter version, miss local plugins, or fail offline even though the project already has the formatter pinned. Probe node_modules/.bin/ first, and only fall back to the package-manager runner when the local binary is missing.
Possible fix
function getFormatterCommand(formatter, filePath, projectRoot) {
+ const localBinName = process.platform === 'win32'
+ ? (formatter === 'biome' ? 'biome.cmd' : 'prettier.cmd')
+ : (formatter === 'biome' ? 'biome' : 'prettier');
+ const localBin = path.join(projectRoot, 'node_modules', '.bin', localBinName);
+
+ if (fs.existsSync(localBin)) {
+ return formatter === 'biome'
+ ? { bin: localBin, args: ['format', '--write', filePath] }
+ : { bin: localBin, args: ['--write', filePath] };
+ }
+
const runner = getFormatterRunner(projectRoot);
if (formatter === 'biome') {
return {
bin: runner.bin,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/post-edit-format.js` around lines 93 - 117, The runner
currently always uses the package-manager execCmd (e.g., "pnpm dlx" / "yarn
dlx") which bypasses project-local binaries; update getFormatterCommand to
prefer a project-local binary under projectRoot/node_modules/.bin (check
existence/readability using fs.existsSync or fs.access) for the formatter binary
name ("biome" for `@biomejs/biome`, "prettier" for prettier) and return that
binary (no prefix) when present; only if the local binary is missing, fall back
to the existing behavior that uses getFormatterRunner(projectRoot) with
runner.bin and runner.prefix plus the package name/args. Ensure you reference
getFormatterRunner and keep formatter-specific package names (`@biomejs/biome` and
prettier) when constructing fallback args.
| if (await asyncTest('detect-project exports the resolved Python command for downstream scripts', async () => { | ||
| const detectProjectPath = path.join(__dirname, '..', '..', 'skills', 'continuous-learning-v2', 'scripts', 'detect-project.sh'); | ||
| const shellCommand = [ | ||
| `source "${detectProjectPath}" >/dev/null 2>&1`, | ||
| 'printf "%s\\n" "${CLV2_PYTHON_CMD:-}"' | ||
| ].join('; '); | ||
|
|
||
| const shell = process.platform === 'win32' ? 'bash' : 'bash'; | ||
| const proc = spawn(shell, ['-lc', shellCommand], { | ||
| env: process.env, | ||
| stdio: ['ignore', 'pipe', 'pipe'] | ||
| }); | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
| proc.stdout.on('data', data => stdout += data); | ||
| proc.stderr.on('data', data => stderr += data); | ||
|
|
||
| const code = await new Promise((resolve, reject) => { | ||
| proc.on('close', resolve); | ||
| proc.on('error', reject); | ||
| }); | ||
|
|
||
| assert.strictEqual(code, 0, `detect-project.sh should source cleanly, stderr: ${stderr}`); | ||
| assert.ok(stdout.trim().length > 0, 'CLV2_PYTHON_CMD should export a resolved interpreter path'); | ||
| })) passed++; else failed++; |
There was a problem hiding this comment.
Don’t require Python in the portability regression test.
This PR’s shell behavior is supposed to degrade cleanly when neither python3 nor python is available, but this assertion makes the suite fail on exactly that supported path by requiring CLV2_PYTHON_CMD to be non-empty. Gate the assertion on interpreter availability, or assert that sourcing succeeds and the exported value matches the best available command when one exists.
Possible fix
- assert.ok(stdout.trim().length > 0, 'CLV2_PYTHON_CMD should export a resolved interpreter path');
+ const resolved = stdout.trim();
+ const hasPython = await new Promise((resolve, reject) => {
+ const probe = spawn('bash', ['-lc', 'command -v python3 >/dev/null 2>&1 || command -v python >/dev/null 2>&1'], {
+ env: process.env,
+ stdio: ['ignore', 'ignore', 'ignore']
+ });
+ probe.on('close', code => resolve(code === 0));
+ probe.on('error', reject);
+ });
+
+ if (hasPython) {
+ assert.ok(resolved.length > 0, 'CLV2_PYTHON_CMD should export a resolved interpreter path');
+ } else {
+ assert.strictEqual(resolved, '', 'CLV2_PYTHON_CMD should stay empty when no interpreter is available');
+ }🤖 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 1753 - 1778, The test currently
requires CLV2_PYTHON_CMD to be non-empty after sourcing detect-project.sh which
fails on systems with no python; change the test so it always asserts the script
sources cleanly (keep the assert.strictEqual(code, 0, ...)) but only assert that
stdout.trim().length > 0 when a Python interpreter is actually available: before
asserting on CLV2_PYTHON_CMD, detect interpreter availability (e.g., probe for
`python3` then `python` via a synchronous which/command check using
child_process.spawnSync or similar), and if an interpreter is found assert the
exported CLV2_PYTHON_CMD matches that resolved command or is non-empty; if no
interpreter is found, skip the non-empty assertion (or assert that the export is
empty). Update the asyncTest block that sources detect-project.sh and references
CLV2_PYTHON_CMD accordingly.
1 participant
Summary
~/.claude/agentsand related pathsplugin: ["ecc-universal"]is documented as hooks/tools-only unless the project also adopts the bundled ECC.opencodeconfigNotes
#352is fixed in the shipped legacy blocker script via direct regression coverage; the activeauto-tmux-dev.jshook behavior remains unchanged#339was already fixed onmainand has been closed separately after verification#320is addressed as a docs/package-accuracy fix; the npm package ships the plugin module and reference assets, but OpenCode still needs project config for commands/agents/instructionsValidation
npm testnode tests/hooks/hooks.test.jsnode tests/integration/hooks.test.jsnode scripts/ci/validate-hooks.jsnpm --prefix .opencode cinpm --prefix .opencode run buildCloses #320
Closes #349
Closes #352
Closes #362
Closes #363
Closes #368
Summary by CodeRabbit
Documentation
Improvements
New
Tests