Conversation
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a configurable audit root and consumer-aware detection to harness audit: new Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as /harness-audit CLI
participant Script as harness-audit.js
participant FS as Filesystem
participant Plugin as ECC Plugin (~/.claude)
User->>CLI: run /harness-audit [--root <path>]
CLI->>Script: invoke with parsed args (rootDir / options)
Script->>FS: read package.json and filesystem under rootDir
alt target == "repo"
Script->>Script: detectTargetMode() => "repo"
Script->>FS: run getRepoChecks(rootDir)
else target == "consumer"
Script->>Plugin: probe plugin install (findPluginInstall)
Script->>Script: detectTargetMode() => "consumer"
Script->>FS: run getConsumerChecks(rootDir)
end
Script->>Script: buildReport(including root_dir, target_mode, rubric_version)
Script-->>CLI: output JSON + human-readable summary
CLI-->>User: display report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Analysis Failed
Troubleshooting
Retry: |
![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: 1b4597a3d6
ℹ️ 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".
| path.join(rootDir, '.claude', 'plugins', 'everything-claude-code', '.claude-plugin', 'plugin.json'), | ||
| path.join(rootDir, '.claude', 'plugins', 'everything-claude-code', 'plugin.json'), | ||
| homeDir && path.join(homeDir, '.claude', 'plugins', 'everything-claude-code', '.claude-plugin', 'plugin.json'), | ||
| homeDir && path.join(homeDir, '.claude', 'plugins', 'everything-claude-code', 'plugin.json'), |
There was a problem hiding this comment.
findPluginInstall() only probes two home-directory layouts (~/.claude/plugins/everything-claude-code/...), but the repo’s own resolver (resolveEccRoot in scripts/lib/resolve-ecc-root.js) also treats paths like ~/.claude/plugins/everything-claude-code@everything-claude-code and ~/.claude/plugins/marketplace/everything-claude-code as valid installs. In those common install modes, consumer-plugin-install will be reported as failing even when ECC is correctly installed, which lowers scores and produces misleading remediation advice for consumer projects.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes #979 by changing the audit's default root from the hardcoded ECC repo directory to the current working directory, and adding auto-detection of ECC-repo vs consumer-project mode so that a tailored rubric is applied in each case. A new Key changes:
One new P2 finding: Four issues flagged in the previous review thread (crashing Confidence Score: 5/5Safe to merge; the only new finding is a P2 Windows portability gap in findPluginInstall, and all prior thread issues are pre-existing / non-blocking. The core logic — CWD-default root, detectTargetMode, consumer rubric, --root flag, unicode-safety refactor — is sound. The single new issue found (HOME vs USERPROFILE) is P2 and affects only Windows users of the consumer-plugin-install scoring path. All other new findings are P2 or lower, so per the confidence guidance the score stays at 5. scripts/harness-audit.js — findPluginInstall (line 372) and the four items from the prior review thread that remain open. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["harness-audit.js main()"] --> B["parseArgs()\n• --root (or AUDIT_ROOT env, or CWD)"]
B --> C["buildReport(scope, {rootDir})"]
C --> D["detectTargetMode(rootDir)"]
D -->|"package.json name == 'everything-claude-code'\nor secondary file heuristic"| E["target_mode = 'repo'"]
D -->|otherwise| F["target_mode = 'consumer'"]
E --> G["getRepoChecks(rootDir)\n⚠ throws if package.json missing"]
F --> H["getConsumerChecks(rootDir)\n• findPluginInstall\n• .claude/ overrides\n• AGENTS.md / CLAUDE.md\n• .mcp.json / settings.json\n• test suite / CI / evals\n• SECURITY.md / .gitignore\n• hook guardrails"]
G --> I["filter by scope"]
H --> I
I --> J["summarizeCategoryScores()"]
J --> K["Return report\n{scope, root_dir, target_mode,\noverall_score, max_score, checks}"]
K -->|"--format json"| L["JSON output"]
K -->|"--format text"| M["Text output\nwith header + top 3 actions"]
Reviews (3): Last reviewed commit: "test: isolate package-manager dependent ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| scopes: ['repo'], | ||
| path: '.claude/memory.md', | ||
| description: 'Project memory or durable notes are checked in', | ||
| pass: fileExists(rootDir, '.claude/memory.md') || countFiles(rootDir, 'docs/adr', '.md') > 0, |
There was a problem hiding this comment.
node_modules scanned when searching from repo root
hasFileWithExtension(rootDir, '.', ...) starts the recursive walk at the project root, which means it will descend into node_modules (thousands of files) when the check isn't short-circuited by the earlier conditions. The check only reaches this call when:
packageJson?.scripts?.testis not a string andcountFiles(rootDir, 'tests', '.test.js')returns 0
Any consumer project that has node_modules but no package.json test script and no tests/*.test.js files will trigger a full recursive scan. With a large dependency tree this will easily exceed the 10-second test timeout and degrade the UX significantly.
A simple fix is to limit the search to explicit, bounded directories (e.g. src/, test/, __tests__/) rather than .:
| pass: fileExists(rootDir, '.claude/memory.md') || countFiles(rootDir, 'docs/adr', '.md') > 0, | |
| pass: typeof packageJson?.scripts?.test === 'string' || countFiles(rootDir, 'tests', '.test.js') > 0 || hasFileWithExtension(rootDir, 'src', ['.spec.js', '.spec.ts', '.test.ts']) || hasFileWithExtension(rootDir, 'test', ['.spec.js', '.spec.ts', '.test.ts']), |
| if (arg === '--root') { | ||
| parsed.root = path.resolve(args[index + 1] || process.cwd()); | ||
| index += 1; | ||
| continue; |
There was a problem hiding this comment.
--root silently consumes adjacent flag when no path is given
When --root is provided without a value and the very next token is another flag (e.g. node harness-audit.js --root --format json), args[index + 1] is '--format', which becomes the resolved root path (something like /cwd/--format). index is then incremented to skip '--format', so json lands as a positional scope argument and throws "Invalid scope: json".
The same pre-existing pattern affects --format and --scope, so this is not unique to this PR, but since --root is a new user-facing option it's a good opportunity to add a guard:
| if (arg === '--root') { | |
| parsed.root = path.resolve(args[index + 1] || process.cwd()); | |
| index += 1; | |
| continue; | |
| if (arg === '--root') { | |
| const next = args[index + 1]; | |
| if (!next || next.startsWith('-')) { | |
| throw new Error('--root requires a path argument'); | |
| } | |
| parsed.root = path.resolve(next); | |
| index += 1; | |
| continue; | |
| } |
| scope: 'repo', | ||
| format: 'text', | ||
| help: false, | ||
| root: path.resolve(process.env.AUDIT_ROOT || process.cwd()), |
There was a problem hiding this comment.
AUDIT_ROOT env variable is undocumented
process.env.AUDIT_ROOT is used as a default root override but is not mentioned in the help text (the showHelp function) or in the command markdown files. Users who discover this via the source won't have any official reference for it.
Consider adding a one-liner to showHelp and to the --root documentation in commands/harness-audit.md:
Env: AUDIT_ROOT=<path> Alternative to --root for CI/scripting use.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 4 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="scripts/harness-audit.js">
<violation number="1" location="scripts/harness-audit.js:53">
P2: Validate that `--root` receives a non-flag value before consuming the next token. Without this guard, `--root --format json` resolves `--format` as a path and shifts `json` into scope parsing.</violation>
<violation number="2" location="scripts/harness-audit.js:190">
P2: `findPluginInstall` only uses `HOME`; when that env var is unset, user-level plugin installs are not detected and `consumer-plugin-install` can fail incorrectly.</violation>
<violation number="3" location="scripts/harness-audit.js:529">
P2: Avoid scanning the entire repository for `*.spec`/`*.test` files; this recursive `'.'` walk can be very slow on large projects.</violation>
</file>
<file name="commands/harness-audit.md">
<violation number="1" location="commands/harness-audit.md:11">
P3: The new `--root` option is documented in Usage/Engine but not in the `Arguments` section, leaving the command docs inconsistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (arg === '--root') { | ||
| parsed.root = path.resolve(args[index + 1] || process.cwd()); | ||
| index += 1; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
P2: Validate that --root receives a non-flag value before consuming the next token. Without this guard, --root --format json resolves --format as a path and shifts json into scope parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/harness-audit.js, line 53:
<comment>Validate that `--root` receives a non-flag value before consuming the next token. Without this guard, `--root --format json` resolves `--format` as a path and shifts `json` into scope parsing.</comment>
<file context>
@@ -51,6 +50,12 @@ function parseArgs(argv) {
continue;
}
+ if (arg === '--root') {
+ parsed.root = path.resolve(args[index + 1] || process.cwd());
+ index += 1;
</file context>
| if (arg === '--root') { | |
| parsed.root = path.resolve(args[index + 1] || process.cwd()); | |
| index += 1; | |
| continue; | |
| } | |
| if (arg === '--root') { | |
| const next = args[index + 1]; | |
| if (!next || next.startsWith('-')) { | |
| throw new Error('--root requires a path argument'); | |
| } | |
| parsed.root = path.resolve(next); | |
| index += 1; | |
| continue; | |
| } |
| scopes: ['repo'], | ||
| path: 'tests/', | ||
| description: 'The project has an automated test entrypoint', | ||
| pass: typeof packageJson?.scripts?.test === 'string' || countFiles(rootDir, 'tests', '.test.js') > 0 || hasFileWithExtension(rootDir, '.', ['.spec.js', '.spec.ts', '.test.ts']), |
There was a problem hiding this comment.
P2: Avoid scanning the entire repository for *.spec/*.test files; this recursive '.' walk can be very slow on large projects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/harness-audit.js, line 529:
<comment>Avoid scanning the entire repository for `*.spec`/`*.test` files; this recursive `'.'` walk can be very slow on large projects.</comment>
<file context>
@@ -381,12 +462,136 @@ function getChecks() {
+ scopes: ['repo'],
+ path: 'tests/',
+ description: 'The project has an automated test entrypoint',
+ pass: typeof packageJson?.scripts?.test === 'string' || countFiles(rootDir, 'tests', '.test.js') > 0 || hasFileWithExtension(rootDir, '.', ['.spec.js', '.spec.ts', '.test.ts']),
+ fix: 'Add a test script or checked-in tests so harness recommendations can be verified automatically.',
+ },
</file context>
| pass: typeof packageJson?.scripts?.test === 'string' || countFiles(rootDir, 'tests', '.test.js') > 0 || hasFileWithExtension(rootDir, '.', ['.spec.js', '.spec.ts', '.test.ts']), | |
| pass: typeof packageJson?.scripts?.test === 'string' || | |
| countFiles(rootDir, 'tests', '.test.js') > 0 || | |
| hasFileWithExtension(rootDir, 'tests', ['.spec.js', '.spec.ts', '.test.ts']) || | |
| hasFileWithExtension(rootDir, 'src', ['.spec.js', '.spec.ts', '.test.ts']), |
| } | ||
|
|
||
| function findPluginInstall(rootDir) { | ||
| const homeDir = process.env.HOME || ''; |
There was a problem hiding this comment.
P2: findPluginInstall only uses HOME; when that env var is unset, user-level plugin installs are not detected and consumer-plugin-install can fail incorrectly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/harness-audit.js, line 190:
<comment>`findPluginInstall` only uses `HOME`; when that env var is unset, user-level plugin installs are not detected and `consumer-plugin-install` can fail incorrectly.</comment>
<file context>
@@ -109,19 +119,90 @@ function countFiles(relativeDir, extension) {
+}
+
+function findPluginInstall(rootDir) {
+ const homeDir = process.env.HOME || '';
+ const candidates = [
+ path.join(rootDir, '.claude', 'plugins', 'everything-claude-code', '.claude-plugin', 'plugin.json'),
</file context>
| const homeDir = process.env.HOME || ''; | |
| const homeDir = process.env.HOME || process.env.USERPROFILE || ''; |
|
|
||
| - `scope` (optional): `repo` (default), `hooks`, `skills`, `commands`, `agents` | ||
| - `--format`: output style (`text` default, `json` for automation) | ||
| - `--root`: audit a specific path instead of the current working directory |
There was a problem hiding this comment.
P3: The new --root option is documented in Usage/Engine but not in the Arguments section, leaving the command docs inconsistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At commands/harness-audit.md, line 11:
<comment>The new `--root` option is documented in Usage/Engine but not in the `Arguments` section, leaving the command docs inconsistent.</comment>
<file context>
@@ -4,22 +4,23 @@ Run a deterministic repository harness audit and return a prioritized scorecard.
- `scope` (optional): `repo` (default), `hooks`, `skills`, `commands`, `agents`
- `--format`: output style (`text` default, `json` for automation)
+- `--root`: audit a specific path instead of the current working directory
## Deterministic Engine
</file context>
![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/harness-audit.js (2)
3-4:⚠️ Potential issue | 🟠 MajorUse a Windows-safe home directory lookup for plugin detection.
process.env.HOMEis often unset on Windows, soconsumer-plugin-installwill fail there even when ECC is installed. Preferos.homedir()or at least aHOME/USERPROFILEfallback.Suggested fix
const fs = require('fs'); +const os = require('os'); const path = require('path'); @@ function findPluginInstall(rootDir) { - const homeDir = process.env.HOME || ''; + const homeDir = process.env.HOME || process.env.USERPROFILE || os.homedir(); const candidates = [As per coding guidelines "
scripts/**/*.js: Provide cross-platform Node.js utilities supporting Windows, macOS, and Linux in scripts/ directory".Also applies to: 189-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/harness-audit.js` around lines 3 - 4, Replace direct usage of process.env.HOME with a Windows-safe home lookup: require the os module (os.homedir()) and use os.homedir() as the primary home directory, falling back to process.env.USERPROFILE or process.env.HOME if necessary; update all occurrences including the plugin detection logic (references to process.env.HOME and any code around lines 189-198 and the consumer-plugin-install check) to use this new lookup so plugin detection works on Windows, macOS, and Linux.
24-31:⚠️ Potential issue | 🟠 MajorValidate
--rootbefore building the report.A missing operand (
--root/--root=) or a bad--root/AUDIT_ROOTpath currently falls through tocwdor an “empty consumer project” score. That makes the new root selection audit the wrong target instead of failing fast.Suggested fix
+function assertDirectory(rootDir) { + if (!fs.existsSync(rootDir) || !fs.statSync(rootDir).isDirectory()) { + throw new Error(`Invalid root directory: ${rootDir}`); + } +} + function parseArgs(argv) { const args = argv.slice(2); const parsed = { scope: 'repo', format: 'text', help: false, - root: path.resolve(process.env.AUDIT_ROOT || process.cwd()), + root: process.env.AUDIT_ROOT || process.cwd(), }; @@ if (arg === '--root') { - parsed.root = path.resolve(args[index + 1] || process.cwd()); + const value = args[index + 1]; + if (!value || value.startsWith('-')) { + throw new Error('Missing value for --root'); + } + parsed.root = value; index += 1; continue; } @@ if (arg.startsWith('--root=')) { - parsed.root = path.resolve(arg.slice('--root='.length)); + const value = arg.slice('--root='.length); + if (!value) { + throw new Error('Missing value for --root'); + } + parsed.root = value; continue; } @@ + parsed.root = path.resolve(parsed.root); + assertDirectory(parsed.root); + return parsed; } @@ function buildReport(scope, options = {}) { const rootDir = path.resolve(options.rootDir || process.cwd()); + assertDirectory(rootDir); const targetMode = options.targetMode || detectTargetMode(rootDir);As per coding guidelines "Always validate all user input before processing at system boundaries" and "Fail fast with clear error messages when validation fails".
Also applies to: 53-57, 69-72, 81-85, 615-618
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/harness-audit.js` around lines 24 - 31, The parseArgs root resolution (variable root set via path.resolve(process.env.AUDIT_ROOT || process.cwd()) in parseArgs) must validate the provided --root/--root= value and AUDIT_ROOT before proceeding: if the CLI arg is present but empty or the resolved path does not exist or is not a directory, emit a clear error (console.error / processLogger) and exit non‑zero (or throw) so the audit fails fast; update parseArgs to detect an explicit empty --root, check fs.existsSync and fs.statSync.isDirectory for the resolved root, and apply the same validation wherever root is constructed/used (the other occurrences you noted) to avoid falling back silently to cwd or an empty project score.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/harness-audit.md`:
- Around line 7-11: Update the documented CLI contract to reflect consumer mode
and the new --root option: add --root to the $ARGUMENTS block and usage line
(`/harness-audit [scope] [--format text|json] [--root path]`), change the
example header from "Harness Audit (repo):" to be dynamic or indicate consumer
detection (e.g., "Harness Audit (consumer/auto-detected):"), and revise the
score/contract text so "scope=repo" no longer assumes a fixed 70-point maximum
but instead describes the current scoring behavior in consumer mode; mirror all
of these same edits into the .opencode copy so both files remain identical.
In `@scripts/harness-audit.js`:
- Around line 171-186: The detectTargetMode function currently uses presence of
scripts/harness-audit.js, .claude-plugin/plugin.json, agents, and skills to
infer "repo" which wrongly flags consumer repos that vendor those files; update
detectTargetMode to require an authoritative marker or explicit override
instead. Replace or augment the file-existence branch (the block using
fileExists(rootDir, 'scripts/harness-audit.js'), fileExists(rootDir,
'.claude-plugin/plugin.json'), fileExists(rootDir, 'agents'),
fileExists(rootDir, 'skills')) so it only returns 'repo' when a trusted marker
exists (e.g., packageJson.claudeRepo === true or a dedicated marker file like
.claude-repo) or when an explicit override/env var is set; keep the existing
packageJson?.name === 'everything-claude-code' check and use the same symbols
(detectTargetMode, safeParseJson, safeRead, fileExists) to locate and change the
logic. Ensure no plain file-presence heuristic alone determines repo mode.
---
Outside diff comments:
In `@scripts/harness-audit.js`:
- Around line 3-4: Replace direct usage of process.env.HOME with a Windows-safe
home lookup: require the os module (os.homedir()) and use os.homedir() as the
primary home directory, falling back to process.env.USERPROFILE or
process.env.HOME if necessary; update all occurrences including the plugin
detection logic (references to process.env.HOME and any code around lines
189-198 and the consumer-plugin-install check) to use this new lookup so plugin
detection works on Windows, macOS, and Linux.
- Around line 24-31: The parseArgs root resolution (variable root set via
path.resolve(process.env.AUDIT_ROOT || process.cwd()) in parseArgs) must
validate the provided --root/--root= value and AUDIT_ROOT before proceeding: if
the CLI arg is present but empty or the resolved path does not exist or is not a
directory, emit a clear error (console.error / processLogger) and exit non‑zero
(or throw) so the audit fails fast; update parseArgs to detect an explicit empty
--root, check fs.existsSync and fs.statSync.isDirectory for the resolved root,
and apply the same validation wherever root is constructed/used (the other
occurrences you noted) to avoid falling back silently to cwd or an empty project
score.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 611ff0e9-aab0-4ee2-8b16-22d14268a72f
📒 Files selected for processing (4)
.opencode/commands/harness-audit.mdcommands/harness-audit.mdscripts/harness-audit.jstests/scripts/harness-audit.test.js
| `/harness-audit [scope] [--format text|json] [--root path]` | ||
|
|
||
| - `scope` (optional): `repo` (default), `hooks`, `skills`, `commands`, `agents` | ||
| - `--format`: output style (`text` default, `json` for automation) | ||
| - `--root`: audit a specific path instead of the current working directory |
There was a problem hiding this comment.
Finish updating the documented output contract for consumer mode.
The new text introduces --root and consumer-project auto-detection, but the page still reflects the old CLI shape below: the $ARGUMENTS block omits --root, the example header is still Harness Audit (repo):, and the contract still implies scope=repo always means a 70-point max. Please update this file and the .opencode mirror together so the docs match the current output.
Also applies to: 23-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/harness-audit.md` around lines 7 - 11, Update the documented CLI
contract to reflect consumer mode and the new --root option: add --root to the
$ARGUMENTS block and usage line (`/harness-audit [scope] [--format text|json]
[--root path]`), change the example header from "Harness Audit (repo):" to be
dynamic or indicate consumer detection (e.g., "Harness Audit
(consumer/auto-detected):"), and revise the score/contract text so "scope=repo"
no longer assumes a fixed 70-point maximum but instead describes the current
scoring behavior in consumer mode; mirror all of these same edits into the
.opencode copy so both files remain identical.
| function detectTargetMode(rootDir) { | ||
| const packageJson = safeParseJson(safeRead(rootDir, 'package.json')); | ||
| if (packageJson?.name === 'everything-claude-code') { | ||
| return 'repo'; | ||
| } | ||
|
|
||
| if ( | ||
| fileExists(rootDir, 'scripts/harness-audit.js') && | ||
| fileExists(rootDir, '.claude-plugin/plugin.json') && | ||
| fileExists(rootDir, 'agents') && | ||
| fileExists(rootDir, 'skills') | ||
| ) { | ||
| return 'repo'; | ||
| } | ||
|
|
||
| return 'consumer'; |
There was a problem hiding this comment.
The repo-mode heuristic still matches copied ECC files.
Any consumer repo that vendors scripts/harness-audit.js, .claude-plugin/plugin.json, agents/, and skills/ will be forced back into repo mode. That recreates the copied-files false positive this PR is trying to remove, so mode detection needs to rely on an authoritative repo marker or an explicit override instead of the same file-presence heuristic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/harness-audit.js` around lines 171 - 186, The detectTargetMode
function currently uses presence of scripts/harness-audit.js,
.claude-plugin/plugin.json, agents, and skills to infer "repo" which wrongly
flags consumer repos that vendor those files; update detectTargetMode to require
an authoritative marker or explicit override instead. Replace or augment the
file-existence branch (the block using fileExists(rootDir,
'scripts/harness-audit.js'), fileExists(rootDir, '.claude-plugin/plugin.json'),
fileExists(rootDir, 'agents'), fileExists(rootDir, 'skills')) so it only returns
'repo' when a trusted marker exists (e.g., packageJson.claudeRepo === true or a
dedicated marker file like .claude-repo) or when an explicit override/env var is
set; keep the existing packageJson?.name === 'everything-claude-code' check and
use the same symbols (detectTargetMode, safeParseJson, safeRead, fileExists) to
locate and change the logic. Ensure no plain file-presence heuristic alone
determines repo mode.
|
Analysis Failed
Troubleshooting
Retry: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/ci/check-unicode-safety.js (1)
119-127: Consider using array accumulation for cleaner string building.String concatenation in a loop creates intermediate strings. While modern V8 optimizes this in many cases, the array + join pattern is more idiomatic for building strings from filtered characters.
♻️ Optional: Array-based approach
function stripDangerousInvisibleChars(text) { - let next = ''; - for (const char of text) { - if (!isDangerousInvisibleCodePoint(char.codePointAt(0))) { - next += char; - } - } - return next; + const chars = []; + for (const char of text) { + if (!isDangerousInvisibleCodePoint(char.codePointAt(0))) { + chars.push(char); + } + } + return chars.join(''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/check-unicode-safety.js` around lines 119 - 127, The stripDangerousInvisibleChars implementation should avoid repeated string concatenation; instead, collect allowed characters into an array and join at the end. Replace the current loop in stripDangerousInvisibleChars to push each char that passes isDangerousInvisibleCodePoint(char.codePointAt(0)) into a local array (e.g., parts) and return parts.join('') so string building is done via array accumulation for clarity and performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/scripts/codex-hooks.test.js`:
- Around line 121-122: Before running the migration in this test, seed the
legacy header "[mcp_servers.context7-mcp]" into the fixture and after the sync
run assert that the canonical header "[mcp_servers.context7]" exists and the
legacy header is removed; specifically update the setup that writes the fixture
(the code that produces the file at configPath) to include the legacy section,
then replace the single assert.match(syncedConfig,
/^\[mcp_servers\.context7\]$/m) with two checks: one asserting the canonical
header is present (e.g. assert.match(syncedConfig,
/^\[mcp_servers\.context7\]$/m)) and one asserting the legacy header is absent
(e.g. assert.notMatch or
assert.ok(!/^\[mcp_servers\.context7-mcp\]$/m.test(syncedConfig))). Ensure you
refer to the same variables used in the diff (configPath and syncedConfig) when
reading and asserting.
---
Nitpick comments:
In `@scripts/ci/check-unicode-safety.js`:
- Around line 119-127: The stripDangerousInvisibleChars implementation should
avoid repeated string concatenation; instead, collect allowed characters into an
array and join at the end. Replace the current loop in
stripDangerousInvisibleChars to push each char that passes
isDangerousInvisibleCodePoint(char.codePointAt(0)) into a local array (e.g.,
parts) and return parts.join('') so string building is done via array
accumulation for clarity and performance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 257ad7e1-251c-41ca-b887-7f7b7be7a91e
📒 Files selected for processing (4)
commands/learn-eval.mdcommands/multi-plan.mdscripts/ci/check-unicode-safety.jstests/scripts/codex-hooks.test.js
✅ Files skipped from review due to trivial changes (2)
- commands/learn-eval.md
- commands/multi-plan.md
| const syncedConfig = fs.readFileSync(configPath, 'utf8'); | ||
| assert.match(syncedConfig, /^\[mcp_servers\.context7\]$/m); |
There was a problem hiding this comment.
Strengthen migration assertion to avoid false positives
At Line 122, this can pass even if --update-mcp migration regresses, because the fixture already contains [mcp_servers.context7] before sync. Seed the legacy section ([mcp_servers.context7-mcp]) and assert canonical exists and legacy is removed after Line 119 runs.
Suggested test hardening
- '[mcp_servers.context7]',
+ '[mcp_servers.context7-mcp]',
'command = "npx"',
'args = ["-y", "@upstash/context7-mcp"]', const syncedConfig = fs.readFileSync(configPath, 'utf8');
assert.match(syncedConfig, /^\[mcp_servers\.context7\]$/m);
+ assert.doesNotMatch(syncedConfig, /^\[mcp_servers\.context7-mcp\]$/m);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/scripts/codex-hooks.test.js` around lines 121 - 122, Before running the
migration in this test, seed the legacy header "[mcp_servers.context7-mcp]" into
the fixture and after the sync run assert that the canonical header
"[mcp_servers.context7]" exists and the legacy header is removed; specifically
update the setup that writes the fixture (the code that produces the file at
configPath) to include the legacy section, then replace the single
assert.match(syncedConfig, /^\[mcp_servers\.context7\]$/m) with two checks: one
asserting the canonical header is present (e.g. assert.match(syncedConfig,
/^\[mcp_servers\.context7\]$/m)) and one asserting the legacy header is absent
(e.g. assert.notMatch or
assert.ok(!/^\[mcp_servers\.context7-mcp\]$/m.test(syncedConfig))). Ensure you
refer to the same variables used in the diff (configPath and syncedConfig) when
reading and asserting.
| } | ||
|
|
||
| function getRepoChecks(rootDir) { | ||
| const packageJson = JSON.parse(readText(rootDir, 'package.json')); |
There was a problem hiding this comment.
getRepoChecks throws when package.json is absent
detectTargetMode has a secondary detection path (checking for scripts/harness-audit.js, .claude-plugin/plugin.json, agents/, and skills/) that can return 'repo' even when no package.json exists. When buildReport then calls getRepoChecks(rootDir), this line throws ENOENT and crashes the process.
In contrast, the new getConsumerChecks uses safeParseJson(safeRead(...)) which never throws. Apply the same safe pattern here:
| const packageJson = JSON.parse(readText(rootDir, 'package.json')); | |
| const packageJson = safeParseJson(safeRead(rootDir, 'package.json')); |
With this change any subsequent checks that rely on packageJson data would just fail gracefully (returning false for their pass field) rather than crashing the audit run.
|
Analysis Failed
Troubleshooting
Retry: |
1 participant
Summary
--rootoption and add regression coverage for consumer-project executionTesting
Fixes #979
Summary by cubic
Audit now runs against the current working directory and auto-detects ECC repo vs consumer projects to apply the right rubric and scorecard, fixing #979. Also hardens CI by improving the Unicode-safety lint and isolating package-manager-dependent tests; use
--rootto audit another path.New Features
--root <path>to target a different directory.target_modeandroot_dir; header prints both.2026-03-30; docs updated; regression tests added for consumer-project execution.Migration
--rootwhen auditing another path.target_modeandroot_dir(additive).Written for commit 0124f6f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes