fix(install): prefix language-specific rules in manual install docs (rebased #651)#690
fix(install): prefix language-specific rules in manual install docs (rebased #651)#690
Conversation
📝 WalkthroughWalkthroughDocumentation across multiple files was updated to reflect changes in rule installation behavior: AGENTS.md had a skill count correction, and the configure-ecc skill documentation (English, Japanese, and Chinese versions) was updated to explain that language-specific rules are now copied with prefixed filenames to prevent overwriting common rules. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis docs-only PR fixes a real bug in the manual install instructions across the EN, JA-JP, and ZH-CN Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Manual Install: Start] --> B[Copy common rules]
B --> B1["cp -r $ECC_ROOT/rules/common/* $TARGET/rules/\n(unquoted — unchanged by this PR)"]
A --> C{Language rules selected?}
C -- TypeScript --> D1["for f in ECC_ROOT/rules/typescript/*\n [ -e f ] && cp f TARGET/rules/typescript-basename-f"]
C -- Python --> D2["for f in ECC_ROOT/rules/python/*\n [ -e f ] && cp f TARGET/rules/python-basename-f"]
C -- Go --> D3["for f in ECC_ROOT/rules/golang/*\n [ -e f ] && cp f TARGET/rules/golang-basename-f"]
D1 --> E[Result: typescript-coding-style.md]
D2 --> F[Result: python-coding-style.md]
D3 --> G[Result: golang-coding-style.md]
B1 --> H[Result: coding-style.md preserved]
E & F & G & H --> I[No common rule overwritten ✓]
Last reviewed commit: "fix: sync catalog co..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4747b80513
ℹ️ 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".
| for f in "$ECC_ROOT"/rules/typescript/*; do [ -e "$f" ] && cp -r "$f" "$TARGET/rules/typescript-$(basename "$f")"; done # if selected | ||
| for f in "$ECC_ROOT"/rules/python/*; do [ -e "$f" ] && cp -r "$f" "$TARGET/rules/python-$(basename "$f")"; done # if selected | ||
| for f in "$ECC_ROOT"/rules/golang/*; do [ -e "$f" ] && cp -r "$f" "$TARGET/rules/golang-$(basename "$f")"; done # if selected |
There was a problem hiding this comment.
When configure-ecc is used to install rules into ~/.claude or .claude, these loops rename rules/<language>/*.md into flat files like typescript-coding-style.md. That no longer matches the supported Claude layout: the production installer keeps rules/common/... and rules/<language>/... directories (scripts/lib/install-executor.js:235-260, verified by tests/scripts/install-apply.test.js:92-95), and rules/README.md:45-49 explicitly warns that flattening breaks the ../common/ references embedded in the language rule files. Users who follow this new guidance will end up with a manual install that diverges from the tested installer behavior and loses those cross-references.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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="AGENTS.md">
<violation number="1" location="AGENTS.md:3">
P3: Keep the skill count consistent within AGENTS.md; this change updates the header to 113 but the Project Structure section still says 109.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
affaan-m
force-pushed
the
community/pr-651-install-prefix
branch
from
4747b80 to
f20fdff
Compare
…ommon rules The configure-ecc skill's Step 3 flat-copies language-specific rule files (e.g., rules/python/*) into the same $TARGET/rules/ directory as common rules. Since both directories contain identically-named files (coding-style.md, security.md, testing.md, hooks.md, patterns.md), the last language installed silently overwrites common rules. Fix by prefixing language-specific files during copy (e.g., python-coding-style.md, golang-testing.md). This matches the approach already used by the Antigravity install target adapter. Also updates the Japanese and Chinese translations of the skill, and adds a troubleshooting note about the prefixed naming convention. Closes #432 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ands Address review feedback: - Add `-r` flag to `cp` to handle potential subdirectories - Quote `$ECC_ROOT` in glob to prevent word-splitting on paths with spaces - Add `[ -e "$f" ]` guard to skip when glob doesn't match anything Applied consistently across EN, JA, and ZH skill docs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
skills/configure-ecc/SKILL.md (2)
199-199: Consider hardening the skills copy for consistency.Similar to the common rules copy, this skills copy command lacks quotes around
$ECC_ROOT. While less critical than the rules (no collision risk), adding quotes would handle paths with spaces consistently across all copy operations in this document.🛡️ Optional hardening
-cp -r $ECC_ROOT/skills/<skill-name> $TARGET/skills/ +cp -r "$ECC_ROOT/skills/<skill-name>" "$TARGET/skills/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/configure-ecc/SKILL.md` at line 199, The cp command shown (cp -r $ECC_ROOT/skills/<skill-name> $TARGET/skills/) can break on paths with spaces; update the command used to copy skills by quoting the variables (e.g., wrap $ECC_ROOT/skills/<skill-name> and $TARGET/skills/ in double quotes) so cp handles whitespace consistently—apply the same quoting style used elsewhere in the document to keep behavior uniform.
222-222: Consider hardening the common rules copy for consistency.The language-specific rule copy commands (lines 224-229) quote
$ECC_ROOTand guard against empty globs, but this common rules copy does not. For consistency and robustness (handling spaces in$ECC_ROOTand empty source directories), consider applying the same pattern.🛡️ Optional hardening
-cp -r $ECC_ROOT/rules/common/* $TARGET/rules/ +for f in "$ECC_ROOT"/rules/common/*; do [ -e "$f" ] && cp -r "$f" "$TARGET/rules/$(basename "$f")"; done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/configure-ecc/SKILL.md` at line 222, The cp invocation copying common rules uses unquoted $ECC_ROOT and no guard against empty globs; update the copy to mirror the language-specific pattern by quoting paths and protecting against empty source globs or missing directories (e.g., check that "$ECC_ROOT/rules/common" exists and has files or use a nullglob-safe loop) so that cp uses "$ECC_ROOT"/rules/common/* and "$TARGET"/rules/ safely and avoids failures when paths contain spaces or the source is empty; adjust the cp command or replace with a guarded loop in SKILL.md where the common rules are copied.docs/zh-CN/skills/configure-ecc/SKILL.md (1)
225-225: Consider hardening the common rules copy for consistency.As with the English version, the language-specific copy commands use robust quoting and guards, but this common rules copy does not. Applying the same pattern would improve consistency.
🛡️ Optional hardening
-cp -r $ECC_ROOT/rules/common/* $TARGET/rules/ +for f in "$ECC_ROOT"/rules/common/*; do [ -e "$f" ] && cp -r "$f" "$TARGET/rules/$(basename "$f")"; done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/zh-CN/skills/configure-ecc/SKILL.md` at line 225, The cp command cp -r $ECC_ROOT/rules/common/* $TARGET/rules/ is unguarded and unquoted; update it to check that ECC_ROOT and TARGET are set, create the destination with mkdir -p "$TARGET/rules", and use a quoted, safer copy form such as cp -a "$ECC_ROOT/rules/common/." "$TARGET/rules/" (or rsync -a "$ECC_ROOT/rules/common/" "$TARGET/rules/") so globs and paths with spaces are handled and empty source directories don’t break the script.docs/ja-JP/skills/configure-ecc/SKILL.md (1)
158-158: Consider hardening the common rules copy for consistency.As with the English version, the language-specific copy commands use robust quoting and guards, but this common rules copy does not. Applying the same pattern would improve consistency.
🛡️ Optional hardening
-cp -r $ECC_ROOT/rules/common/* $TARGET/rules/ +for f in "$ECC_ROOT"/rules/common/*; do [ -e "$f" ] && cp -r "$f" "$TARGET/rules/$(basename "$f")"; done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ja-JP/skills/configure-ecc/SKILL.md` at line 158, Replace the brittle cp line (cp -r $ECC_ROOT/rules/common/* $TARGET/rules/) with a hardened guarded sequence: check existence of the source directory (e.g., if [ -d "$ECC_ROOT/rules/common" ]; then ... fi), ensure target exists (mkdir -p "$TARGET/rules"), and use quoted, robust copy such as cp -a "$ECC_ROOT/rules/common/." "$TARGET/rules/" so variables ECC_ROOT and TARGET are quoted, hidden files are preserved, and the operation fails less unexpectedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 145: The skill count is inconsistent: update the "skills/ — 109 workflow
skills and domain knowledge" entry to reflect the correct total of 113 skills so
it matches the header (line 3) and PR objectives; locate the string "skills/ —
109 workflow skills and domain knowledge" and change the numeric value to 113
and verify any other summary lines referencing total skills are consistent.
---
Nitpick comments:
In `@docs/ja-JP/skills/configure-ecc/SKILL.md`:
- Line 158: Replace the brittle cp line (cp -r $ECC_ROOT/rules/common/*
$TARGET/rules/) with a hardened guarded sequence: check existence of the source
directory (e.g., if [ -d "$ECC_ROOT/rules/common" ]; then ... fi), ensure target
exists (mkdir -p "$TARGET/rules"), and use quoted, robust copy such as cp -a
"$ECC_ROOT/rules/common/." "$TARGET/rules/" so variables ECC_ROOT and TARGET are
quoted, hidden files are preserved, and the operation fails less unexpectedly.
In `@docs/zh-CN/skills/configure-ecc/SKILL.md`:
- Line 225: The cp command cp -r $ECC_ROOT/rules/common/* $TARGET/rules/ is
unguarded and unquoted; update it to check that ECC_ROOT and TARGET are set,
create the destination with mkdir -p "$TARGET/rules", and use a quoted, safer
copy form such as cp -a "$ECC_ROOT/rules/common/." "$TARGET/rules/" (or rsync -a
"$ECC_ROOT/rules/common/" "$TARGET/rules/") so globs and paths with spaces are
handled and empty source directories don’t break the script.
In `@skills/configure-ecc/SKILL.md`:
- Line 199: The cp command shown (cp -r $ECC_ROOT/skills/<skill-name>
$TARGET/skills/) can break on paths with spaces; update the command used to copy
skills by quoting the variables (e.g., wrap $ECC_ROOT/skills/<skill-name> and
$TARGET/skills/ in double quotes) so cp handles whitespace consistently—apply
the same quoting style used elsewhere in the document to keep behavior uniform.
- Line 222: The cp invocation copying common rules uses unquoted $ECC_ROOT and
no guard against empty globs; update the copy to mirror the language-specific
pattern by quoting paths and protecting against empty source globs or missing
directories (e.g., check that "$ECC_ROOT/rules/common" exists and has files or
use a nullglob-safe loop) so that cp uses "$ECC_ROOT"/rules/common/* and
"$TARGET"/rules/ safely and avoids failures when paths contain spaces or the
source is empty; adjust the cp command or replace with a guarded loop in
SKILL.md where the common rules are copied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9635700-4a42-49c9-94f3-0e479b41dce2
📒 Files selected for processing (4)
AGENTS.mddocs/ja-JP/skills/configure-ecc/SKILL.mddocs/zh-CN/skills/configure-ecc/SKILL.mdskills/configure-ecc/SKILL.md
| ``` | ||
| agents/ — 27 specialized subagents | ||
| skills/ — 113 workflow skills and domain knowledge | ||
| skills/ — 109 workflow skills and domain knowledge |
There was a problem hiding this comment.
Inconsistent skill count with line 3.
Line 3 declares "113 skills" but this line shows "109 workflow skills". According to the PR objectives, the target is 113 skills across the catalog.
📊 Proposed fix
-skills/ — 109 workflow skills and domain knowledge
+skills/ — 113 workflow skills and domain knowledge📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| skills/ — 109 workflow skills and domain knowledge | |
| skills/ — 113 workflow skills and domain knowledge |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 145, The skill count is inconsistent: update the "skills/
— 109 workflow skills and domain knowledge" entry to reflect the correct total
of 113 skills so it matches the header (line 3) and PR objectives; locate the
string "skills/ — 109 workflow skills and domain knowledge" and change the
numeric value to 113 and verify any other summary lines referencing total skills
are consistent.
f20fdff to
6a76c1e
Compare
|
Closing — docs-only change, automated installer already handles this correctly. |
2 participants
Summary
configure-eccskill docs (EN, JA, ZH) to prefix language-specific rules during manual install, preventing overwrites of common rules (e.g.,coding-style.mdbecomestypescript-coding-style.md)install-apply.js) already handles this correctly: Claude target uses subdirectories (rules/typescript/), Antigravity target uses prefixed filenames (typescript-coding-style.md)Original PR
Closes #651
Test plan
Summary by cubic
Prevents language-specific rules from overwriting common rules during manual install by prefixing filenames in flat copies, and hardens copy commands. Also syncs catalog counts in
AGENTS.md.typescript-coding-style.md) and add a troubleshooting note; applied to EN/JA/ZHconfigure-eccdocs.cp -r, quote$ECC_ROOT, and guard empty globs with[ -e "$f" ]; updateAGENTS.mdcounts.Written for commit 6a76c1e. Summary will update on new commits.
Summary by CodeRabbit
typescript-coding-style.md), preventing common rule files from being overwritten by language-specific variants.