Skip to content

Fix SessionStart hook fallback and add adapter drift guards#257

Closed
affaan-m wants to merge 2 commits into
mainfrom
codex/adapter-drift-rules-skills
Closed

Fix SessionStart hook fallback and add adapter drift guards#257
affaan-m wants to merge 2 commits into
mainfrom
codex/adapter-drift-rules-skills

Conversation

@affaan-m

@affaan-m affaan-m commented Feb 20, 2026

Copy link
Copy Markdown
Owner

Summary

  • fix SessionStart hook path resolution when CLAUDE_PLUGIN_ROOT is missing by adding a fallback resolver in hooks/hooks.json
  • harden scripts/hooks/post-edit-format.js with formatter auto-detection (Biome/Prettier), monorepo config walking, and absolute path handling
  • update hook tests to allow SessionStart fallback resolver while keeping strict plugin-root checks for direct script hooks
  • add adapter drift guards for commands/rules/skills via scripts/ci/check-adapter-drift.js and sync helpers in scripts/sync/
  • wire adapter drift check into CI validate job and npm scripts (sync:commands, sync:rules-skills, sync:adapters, sync:check)

Validation

  • node scripts/ci/check-adapter-drift.js
  • node scripts/ci/validate-hooks.js
  • node scripts/ci/validate-commands.js
  • node tests/hooks/hooks.test.js
  • npm test

Notes

  • left plugins/miniclaw/ untouched (local untracked path) per instruction

Summary by CodeRabbit

  • New Features

    • Added sync commands to detect and apply drift between canonical sources and adapter copies
    • Added a CI guard to check adapter drift
  • Improvements

    • Hook system now resolves and runs session-start dynamically and auto-detects the project formatter
    • Test workflow now runs drift checks before validations
  • Documentation

    • Added Codex docs and agent guidance; updated README and install flow for Codex

@coderabbitai

coderabbitai Bot commented Feb 20, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The PR adds adapter-drift checks to CI, introduces sync scripts for commands/rules/skills with check mode, updates package scripts and installers for a new "codex" target, replaces the SessionStart hook with a dynamic resolver, enhances the post-edit formatter to auto-detect Biome/Prettier, updates related tests, and adds Codex docs (AGENTS.md, .codex/README.md) and README/install.sh codex support.

Changes

Cohort / File(s) Summary
CI & Drift Check
\.github/workflows/ci.yml, scripts/ci/check-adapter-drift.js
Added a CI step "Check adapter drift" that runs a new guard script aggregating drift reports from sync functions and failing CI when issues are detected.
Sync Utilities
scripts/sync/sync-commands.js, scripts/sync/sync-rules-skills.js
New sync scripts that compare canonical commands/, rules/, and skills/ sources to adapter copies (.cursor/.opencode), support --check mode, report drift, and export programmatic functions.
Package & Scripts
package.json
Added codex keywords and files, added scripts/sync/ to files, new npm scripts: sync:commands, sync:rules-skills, sync:adapters, sync:check, and prepended adapter-drift check to test.
Hook Files & Formatter
hooks/hooks.json, scripts/hooks/post-edit-format.js
SessionStart hook converted to a dynamic inline Node resolver that searches candidate base paths for scripts/hooks/session-start.js. Post-edit formatter replaced with project-aware detection of Biome or Prettier and runs the appropriate formatter.
Hook Tests
tests/hooks/hooks.test.js
Tests extended to accept the new SessionStart resolver pattern (CLAUDE_PLUGIN_ROOT + cwd fallback) in addition to direct script references.
Codex docs & installer
.codex/README.md, AGENTS.md, README.md, install.sh
Added Codex documentation files, updated README with Codex support, and extended install.sh to handle a new codex target (creates .codex, installs rules, copies AGENTS.md/MCP references).

Sequence Diagram(s)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐰
Adapters wander, drift, and play,
I hop in code to show the way.
Sync the mirrors, guard the night,
Formatter chooses left or right.
Small rabbit taps — CI beams bright. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.35% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix SessionStart hook fallback and add adapter drift guards' accurately captures the main changes: fixing a SessionStart fallback and adding drift guards for adapters.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/adapter-drift-rules-skills

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c58adced28

ℹ️ 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".

Comment thread hooks/hooks.json
{
"type": "command",
"command": "node \"${CLAUDE_PLUGIN_ROOT}/scripts/hooks/session-start.js\""
"command": "node -e \"const fs=require('fs'),path=require('path'),os=require('os'),cp=require('child_process');const candidates=[process.env.CLAUDE_PLUGIN_ROOT,process.env.CLAUDE_PROJECT_DIR,process.cwd(),path.join(os.homedir(),'.claude','plugins','everything-claude-code'),path.join(os.homedir(),'.claude','plugins','ecc-universal')].filter(Boolean);let script='';for(const base of candidates){const p=path.join(base,'scripts','hooks','session-start.js');if(fs.existsSync(p)){script=p;break}}if(!script){console.error('[Hook] SessionStart resolver: session-start.js not found');process.exit(0)}const r=cp.spawnSync(process.execPath,[script],{stdio:'inherit'});process.exit(r.status??0)\""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restrict SessionStart resolver to plugin install paths

The new resolver now searches CLAUDE_PROJECT_DIR and process.cwd() before the known plugin directories, so when CLAUDE_PLUGIN_ROOT is unset it will execute any project-local scripts/hooks/session-start.js it finds first. In normal usage cwd is the user’s repo, so this can run unrelated or untrusted repository code at session start instead of the bundled hook, changing behavior and creating a code-execution risk tied to repository contents.

Useful? React with 👍 / 👎.


try {
const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8'));
return Object.prototype.hasOwnProperty.call(pkg, 'prettier');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Detect Prettier installation from dependencies

hasPrettierInPackageJson only checks for a top-level prettier config key, so projects that install Prettier in dependencies/devDependencies and rely on default formatting are treated as having no formatter. In that case detectFormatter returns null and the hook skips formatting entirely, which is a regression from the prior behavior that always attempted npx prettier --write for JS/TS edits.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
hooks/hooks.json (1)

68-78: ⚠️ Potential issue | 🟡 Minor

SessionStart has a fallback resolver but SessionEnd and other hooks do not — inconsistent error handling.

SessionStart uses an inline resolver that gracefully handles missing CLAUDE_PLUGIN_ROOT by checking multiple fallback candidates. However, SessionEnd (line 152), PreCompact (line 62), Stop (line 140), and other hooks use ${CLAUDE_PLUGIN_ROOT} directly without fallback. If CLAUDE_PLUGIN_ROOT is unset, these hooks will expand to invalid paths and fail, while SessionStart continues. This creates an asymmetry where sessions can start but cannot cleanly end or compact.

Consider applying the same fallback resolver pattern to SessionEnd and other critical hooks, or document why only SessionStart requires this resilience.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hooks/hooks.json` around lines 68 - 78, SessionEnd, PreCompact, Stop and
similar hooks use ${CLAUDE_PLUGIN_ROOT} directly and will fail if that env var
is unset; update those hook entries to use the same fallback resolver logic as
SessionStart (search candidates like process.env.CLAUDE_PLUGIN_ROOT,
process.env.CLAUDE_PROJECT_DIR, process.cwd(), and ~/.claude/plugins/... and run
the resolved scripts or exit gracefully) or extract the lookup into a shared
inline resolver and call it from the SessionEnd/PreCompact/Stop hook commands so
each hook checks multiple candidates and logs a clear error + exits with the
resolver status instead of expanding invalid paths.
🧹 Nitpick comments (7)
package.json (1)

82-82: sync:adapters hardcodes npm run — may not respect the user's package manager.

The sync:adapters script calls npm run sync:commands && npm run sync:rules-skills, which always uses npm even if the user invokes it via pnpm run sync:adapters or yarn sync:adapters. While this is common practice in npm scripts and functionally works (npm is typically available), it doesn't align with the project's guideline to support configurable package manager selection.

Consider using the individual node commands directly to stay PM-agnostic:

♻️ PM-agnostic alternative
-    "sync:adapters": "npm run sync:commands && npm run sync:rules-skills",
+    "sync:adapters": "node scripts/sync/sync-commands.js && node scripts/sync/sync-rules-skills.js",

Based on learnings: "Support multiple package managers: npm, pnpm, yarn, bun, with configurable selection via CLAUDE_PACKAGE_MANAGER env var or project config"

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 82, The "sync:adapters" script currently invokes "npm
run sync:commands && npm run sync:rules-skills", which hardcodes npm; update the
"sync:adapters" package.json entry to be package-manager-agnostic by invoking
the underlying commands directly (e.g., call the node scripts or CLI entry
points the "sync:commands" and "sync:rules-skills" scripts run) instead of using
"npm run"; reference the script names "sync:adapters", "sync:commands", and
"sync:rules-skills" when locating the change and consider using the
CLAUDE_PACKAGE_MANAGER env var or equivalent project config if you choose to
preserve configurable PM-driven execution.
scripts/sync/sync-rules-skills.js (3)

195-203: In sync mode, drift with missing frontmatter is reported as an issue (not auto-fixed).

When checkOnly is false and content has drifted, but the target cursor rule lacks frontmatter, the script reports [rules] missing frontmatter in cursor rule instead of writing. This means non-override rules without frontmatter will never be auto-synced, only flagged. This is a reasonable safeguard but could surprise contributors who expect sync:rules-skills to fix all drift automatically.

A brief comment explaining the rationale (e.g., "frontmatter required to preserve Cursor metadata") would help.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/sync/sync-rules-skills.js` around lines 195 - 203, The code currently
treats drifted cursor rules without frontmatter as an issue and does not
auto-write them when checkOnly is false; add a brief inline comment above the
conditional that checks frontmatter (the block using checkOnly, frontmatter,
targetName, composeFrontmatterAndBody and fs.writeFileSync(targetPath,...))
explaining the rationale (e.g., "frontmatter required to preserve Cursor
metadata — do not auto-create/overwrite to avoid losing metadata; only flag for
manual fix"), so future readers understand why missing frontmatter prevents
automatic syncing; keep the comment short and colocated with this conditional.

86-98: Utility functions duplicated with sync-commands.js.

readNormalized, listMarkdownFiles, ensureDir, and copyFile are near-identical in both sync-commands.js (lines 72-88) and this file (lines 86-98, 100-103, 94-98, 157-160). Consider extracting these to a shared module (e.g., scripts/sync/sync-utils.js) to reduce maintenance burden.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/sync/sync-rules-skills.js` around lines 86 - 98, The utility
functions normalizeText, readNormalized, ensureDir, listMarkdownFiles, and
copyFile are duplicated between sync-rules-skills.js and sync-commands.js;
extract these common helpers into a new shared module (e.g.,
scripts/sync/sync-utils.js) and update both files to import them. Create
sync-utils.js exporting the functions (retain current implementations and
names), replace the local definitions in sync-rules-skills.js and
sync-commands.js with imports (e.g., const { normalizeText, readNormalized,
ensureDir, listMarkdownFiles, copyFile } = require('./sync-utils')), and run
tests to ensure no behavior changed. Ensure exported helpers handle the same
parameters and error behavior as the originals so callers like
readNormalized(...) and ensureDir(...) continue to work unchanged.

114-133: listFilesRecursive has no depth limit.

While skills directories are expected to be shallow, an unbounded recursion could cause issues with symlink cycles or deeply nested content. A depth cap (e.g., 10 levels) would add defensive safety.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/sync/sync-rules-skills.js` around lines 114 - 133, The recursive
helper listFilesRecursive currently has no depth limit; add defensive
depth-limiting by changing its signature to accept a currentDepth (default 0)
and a maxDepth (default 10), check if currentDepth >= maxDepth and return an
empty array when reached, and when recursing into subdirectories call
listFilesRecursive(absPath, nextRel, currentDepth + 1, maxDepth); update any
callers to pass the new parameters or rely on defaults. Ensure the function name
listFilesRecursive is updated in all call sites so recursion stops after 10
levels by default.
scripts/hooks/post-edit-format.js (1)

29-86: findProjectRoot and detectFormatter perform separate directory walks — minor redundancy.

Both functions walk up the directory tree independently. findProjectRoot stops at package.json, while detectFormatter walks all the way to the filesystem root looking for formatter configs. In practice, this is fine for a hook that runs once per edit, but if you ever consolidate, the formatter detection could subsume the project root discovery.

Also worth noting: Biome configs are checked before Prettier configs at each directory level (line 70 vs 74), so Biome takes priority when both are configured. If this is intentional, a brief comment would help future maintainers.

🤖 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 29 - 86, Both findProjectRoot
and detectFormatter independently walk the directory tree causing redundant
work; consolidate by performing a single upward walk (e.g., extract a shared
walk routine or make detectFormatter return both formatter and projectRoot) and
have findProjectRoot reuse it so we only traverse once; while doing so keep the
existing check order (biomeConfigs before prettierConfigs) but add a concise
comment near biomeConfigs/prettierConfigs in detectFormatter explaining that
Biome intentionally takes priority when both exist.
hooks/hooks.json (1)

74-74: Inline resolver is correct but fragile to maintain.

The 500+ character single-line node -e command on line 74 works but is difficult to read, debug, or modify. Consider extracting the resolver logic to a dedicated script (e.g., scripts/hooks/resolve-session-start.js) that itself handles the fallback. That script could then be referenced via a simpler inline path probe, or even embedded as a small wrapper that checks CLAUDE_PLUGIN_ROOT first and falls back to __dirname-based resolution.

The r.status??0 nullish coalescing is correct for handling a null status from a killed process.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hooks/hooks.json` at line 74, The inline 500+ char node -e resolver is
fragile; extract the logic into a dedicated resolver script (e.g.,
scripts/hooks/resolve-session-start.js) that performs the same candidate search
using CLAUDE_PLUGIN_ROOT, CLAUDE_PROJECT_DIR, process.cwd(), and homedir
fallbacks, finds scripts/hooks/session-start.js, spawns it and exits with
r.status ?? 0; then replace the long node -e command in hooks.json with a short
wrapper that executes that new resolve-session-start.js (or a small one-line
node call to it) so the resolution logic lives in a maintainable file and the
hooks.json entry simply references the resolver script.
scripts/sync/sync-commands.js (1)

145-146: Minor: prefer const with push or spread instead of let with concat.

allIssues and allUpdates are reassigned via concat. Using const with push(...result.issues) would be more idiomatic and avoid the let reassignment pattern.

♻️ Suggested change
-  let allIssues = [];
-  let allUpdates = [];
+  const allIssues = [];
+  const allUpdates = [];

   for (const target of TARGETS) {
     ensureDir(target.dir);
     const result = syncTarget(target, sourceFiles, checkOnly);
-    allIssues = allIssues.concat(result.issues);
-    allUpdates = allUpdates.concat(result.updates);
+    allIssues.push(...result.issues);
+    allUpdates.push(...result.updates);
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/sync/sync-commands.js` around lines 145 - 146, Change the mutable
reassignment of allIssues and allUpdates to use const and mutate the arrays
in-place instead of reassigning with concat: declare allIssues and allUpdates as
const, and where the code currently does allIssues =
allIssues.concat(result.issues) or allUpdates = allUpdates.concat(...), replace
those reassignments with in-place pushes (e.g., use push(...result.issues) or
push(...result.updates)) so the arrays are updated without reassigning; look for
references to allIssues and allUpdates in the sync logic in
scripts/sync/sync-commands.js to apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@hooks/hooks.json`:
- Around line 68-78: SessionEnd, PreCompact, Stop and similar hooks use
${CLAUDE_PLUGIN_ROOT} directly and will fail if that env var is unset; update
those hook entries to use the same fallback resolver logic as SessionStart
(search candidates like process.env.CLAUDE_PLUGIN_ROOT,
process.env.CLAUDE_PROJECT_DIR, process.cwd(), and ~/.claude/plugins/... and run
the resolved scripts or exit gracefully) or extract the lookup into a shared
inline resolver and call it from the SessionEnd/PreCompact/Stop hook commands so
each hook checks multiple candidates and logs a clear error + exits with the
resolver status instead of expanding invalid paths.

---

Nitpick comments:
In `@hooks/hooks.json`:
- Line 74: The inline 500+ char node -e resolver is fragile; extract the logic
into a dedicated resolver script (e.g., scripts/hooks/resolve-session-start.js)
that performs the same candidate search using CLAUDE_PLUGIN_ROOT,
CLAUDE_PROJECT_DIR, process.cwd(), and homedir fallbacks, finds
scripts/hooks/session-start.js, spawns it and exits with r.status ?? 0; then
replace the long node -e command in hooks.json with a short wrapper that
executes that new resolve-session-start.js (or a small one-line node call to it)
so the resolution logic lives in a maintainable file and the hooks.json entry
simply references the resolver script.

In `@package.json`:
- Line 82: The "sync:adapters" script currently invokes "npm run sync:commands
&& npm run sync:rules-skills", which hardcodes npm; update the "sync:adapters"
package.json entry to be package-manager-agnostic by invoking the underlying
commands directly (e.g., call the node scripts or CLI entry points the
"sync:commands" and "sync:rules-skills" scripts run) instead of using "npm run";
reference the script names "sync:adapters", "sync:commands", and
"sync:rules-skills" when locating the change and consider using the
CLAUDE_PACKAGE_MANAGER env var or equivalent project config if you choose to
preserve configurable PM-driven execution.

In `@scripts/hooks/post-edit-format.js`:
- Around line 29-86: Both findProjectRoot and detectFormatter independently walk
the directory tree causing redundant work; consolidate by performing a single
upward walk (e.g., extract a shared walk routine or make detectFormatter return
both formatter and projectRoot) and have findProjectRoot reuse it so we only
traverse once; while doing so keep the existing check order (biomeConfigs before
prettierConfigs) but add a concise comment near biomeConfigs/prettierConfigs in
detectFormatter explaining that Biome intentionally takes priority when both
exist.

In `@scripts/sync/sync-commands.js`:
- Around line 145-146: Change the mutable reassignment of allIssues and
allUpdates to use const and mutate the arrays in-place instead of reassigning
with concat: declare allIssues and allUpdates as const, and where the code
currently does allIssues = allIssues.concat(result.issues) or allUpdates =
allUpdates.concat(...), replace those reassignments with in-place pushes (e.g.,
use push(...result.issues) or push(...result.updates)) so the arrays are updated
without reassigning; look for references to allIssues and allUpdates in the sync
logic in scripts/sync/sync-commands.js to apply this change.

In `@scripts/sync/sync-rules-skills.js`:
- Around line 195-203: The code currently treats drifted cursor rules without
frontmatter as an issue and does not auto-write them when checkOnly is false;
add a brief inline comment above the conditional that checks frontmatter (the
block using checkOnly, frontmatter, targetName, composeFrontmatterAndBody and
fs.writeFileSync(targetPath,...)) explaining the rationale (e.g., "frontmatter
required to preserve Cursor metadata — do not auto-create/overwrite to avoid
losing metadata; only flag for manual fix"), so future readers understand why
missing frontmatter prevents automatic syncing; keep the comment short and
colocated with this conditional.
- Around line 86-98: The utility functions normalizeText, readNormalized,
ensureDir, listMarkdownFiles, and copyFile are duplicated between
sync-rules-skills.js and sync-commands.js; extract these common helpers into a
new shared module (e.g., scripts/sync/sync-utils.js) and update both files to
import them. Create sync-utils.js exporting the functions (retain current
implementations and names), replace the local definitions in
sync-rules-skills.js and sync-commands.js with imports (e.g., const {
normalizeText, readNormalized, ensureDir, listMarkdownFiles, copyFile } =
require('./sync-utils')), and run tests to ensure no behavior changed. Ensure
exported helpers handle the same parameters and error behavior as the originals
so callers like readNormalized(...) and ensureDir(...) continue to work
unchanged.
- Around line 114-133: The recursive helper listFilesRecursive currently has no
depth limit; add defensive depth-limiting by changing its signature to accept a
currentDepth (default 0) and a maxDepth (default 10), check if currentDepth >=
maxDepth and return an empty array when reached, and when recursing into
subdirectories call listFilesRecursive(absPath, nextRel, currentDepth + 1,
maxDepth); update any callers to pass the new parameters or rely on defaults.
Ensure the function name listFilesRecursive is updated in all call sites so
recursion stops after 10 levels by default.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
package.json (1)

86-86: sync:adapters hardcodes npm run; prefer direct node invocations.

Since both sub-scripts call node directly, the npm run indirection is unnecessary and inconsistent with the project's package-manager-agnostic style. Direct node invocations also avoid spawning an extra npm subprocess.

♻️ Proposed refactor
-    "sync:adapters": "npm run sync:commands && npm run sync:rules-skills",
+    "sync:adapters": "node scripts/sync/sync-commands.js && node scripts/sync/sync-rules-skills.js",

Based on learnings, scripts/**/*.js should support cross-platform compatibility and package-manager agnosticism (CLAUDE_PACKAGE_MANAGER env var / configurable PM selection); hardcoding npm run in composite scripts works against this.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 86, The composite script "sync:adapters" currently
shells out to "npm run" for the sub-scripts "sync:commands" and
"sync:rules-skills"; replace that indirection by calling the underlying Node
entry points directly (the same JS files those two scripts execute) or by using
the CLAUDE_PACKAGE_MANAGER-aware invocation used elsewhere so the script remains
package-manager-agnostic and cross-platform; update the "sync:adapters" value to
invoke the node processes directly (or the shared PM-aware helper) instead of
"npm run".
install.sh (2)

197-211: Language validation loop is duplicated across all three targets.

The for lang in "$@" block with path-traversal guard, missing-dir warning, and cp -r call is identical in the claude, cursor, and codex branches. Extracting it to a install_lang_rules <dest_rules_dir> function would remove the duplication.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@install.sh` around lines 197 - 211, Extract the repeated language-install
loop into a helper function named install_lang_rules that accepts a destination
rules dir parameter (e.g., install_lang_rules <dest_rules_dir>), move the
validation/guard (the regex ^[a-zA-Z0-9_-]+$), missing-dir warning (checking
"$RULES_DIR/$lang"), mkdir -p "$dest_rules_dir/$lang", and cp -r
"$RULES_DIR/$lang/." "$dest_rules_dir/$lang/" into that function, and replace
the three duplicated for lang in "$@" loops in the claude, cursor, and codex
branches with calls to install_lang_rules passing the corresponding target
directory (e.g., "$CLAUDE_RULES_DIR", "$CURSOR_RULES_DIR", "$CODEX_RULES_DIR")
so behavior remains identical.

213-214: mkdir -p "$DEST_DIR" is redundant.

.codex/ is already created at line 193 via mkdir -p "$CODEX_RULES_DIR/common" (which expands to mkdir -p ".codex/rules/common"). This line is always a no-op.

🧹 Proposed removal
     # --- MCP Reference ---
-    mkdir -p "$DEST_DIR"
     if [[ -f "$CODEX_MCP_SRC" ]]; then
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@install.sh` around lines 213 - 214, Remove the redundant directory creation:
delete the `mkdir -p "$DEST_DIR"` line (the `DEST_DIR` directory is already
created by the earlier `mkdir -p "$CODEX_RULES_DIR/common"` call which creates
`.codex/rules/common`), so remove that no-op to avoid duplicate commands and
keep the script concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 819-821: The README currently references `.codex/README.md` as if
it is copied into the user's project, but `ecc-install --target codex` does not
install that file (it only creates `.codex/rules/` and
`.codex/mcp-servers.json`); update the README.md to point to the canonical repo
copy instead of implying it is installed locally. Replace the misleading
`.codex/README.md` entry with a link to the repo file (e.g. mirror the Cursor
pattern used: `[.codex/README.md](.codex/README.md)` or an absolute repo path),
and adjust the Codex Agent Template entry if needed so both `Codex README` and
`Codex Agent Template` accurately reference files that live in the package/repo
rather than the installed project.

---

Nitpick comments:
In `@install.sh`:
- Around line 197-211: Extract the repeated language-install loop into a helper
function named install_lang_rules that accepts a destination rules dir parameter
(e.g., install_lang_rules <dest_rules_dir>), move the validation/guard (the
regex ^[a-zA-Z0-9_-]+$), missing-dir warning (checking "$RULES_DIR/$lang"),
mkdir -p "$dest_rules_dir/$lang", and cp -r "$RULES_DIR/$lang/."
"$dest_rules_dir/$lang/" into that function, and replace the three duplicated
for lang in "$@" loops in the claude, cursor, and codex branches with calls to
install_lang_rules passing the corresponding target directory (e.g.,
"$CLAUDE_RULES_DIR", "$CURSOR_RULES_DIR", "$CODEX_RULES_DIR") so behavior
remains identical.
- Around line 213-214: Remove the redundant directory creation: delete the
`mkdir -p "$DEST_DIR"` line (the `DEST_DIR` directory is already created by the
earlier `mkdir -p "$CODEX_RULES_DIR/common"` call which creates
`.codex/rules/common`), so remove that no-op to avoid duplicate commands and
keep the script concise.

In `@package.json`:
- Line 86: The composite script "sync:adapters" currently shells out to "npm
run" for the sub-scripts "sync:commands" and "sync:rules-skills"; replace that
indirection by calling the underlying Node entry points directly (the same JS
files those two scripts execute) or by using the CLAUDE_PACKAGE_MANAGER-aware
invocation used elsewhere so the script remains package-manager-agnostic and
cross-platform; update the "sync:adapters" value to invoke the node processes
directly (or the shared PM-aware helper) instead of "npm run".

Comment thread README.md
Comment on lines +819 to +821

- **Codex README**: `.codex/README.md`
- **Codex Agent Template**: `AGENTS.md`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

.codex/README.md isn't installed to the user's project .codex/ directory — the path reference is misleading.

ecc-install --target codex populates .codex/rules/ and .codex/mcp-servers.json but does not copy .codex/README.md. The file lives in the npm package / cloned repo only. The Cursor section handles this consistently by linking directly into the repo ([.cursor/README.md](.cursor/README.md)). Consider the same approach here to avoid confusion:

-  - **Codex README**: `.codex/README.md`
+  - **Codex README**: [`.codex/README.md`](.codex/README.md)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 819 - 821, The README currently references
`.codex/README.md` as if it is copied into the user's project, but `ecc-install
--target codex` does not install that file (it only creates `.codex/rules/` and
`.codex/mcp-servers.json`); update the README.md to point to the canonical repo
copy instead of implying it is installed locally. Replace the misleading
`.codex/README.md` entry with a link to the repo file (e.g. mirror the Cursor
pattern used: `[.codex/README.md](.codex/README.md)` or an absolute repo path),
and adjust the Codex Agent Template entry if needed so both `Codex README` and
`Codex Agent Template` accurately reference files that live in the package/repo
rather than the installed project.

@affaan-m

affaan-m commented Mar 2, 2026

Copy link
Copy Markdown
Owner Author

⚠️ CI Failure Report

All CI checks are failing on this PR. Issues detected:

Lint failures across all platforms/Node versions — the full test matrix is red.

Additionally, this PR has merge conflicts with main that need to be resolved before it can proceed.

AgentShield Scan also failed.

Please rebase on latest main and fix lint issues to get CI green. Happy to re-review once updated.

@affaan-m affaan-m closed this Mar 3, 2026
@affaan-m affaan-m deleted the codex/adapter-drift-rules-skills branch March 3, 2026 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant