feat: add --rulesync-dir flag to decouple source rules from output directory#1514
feat: add --rulesync-dir flag to decouple source rules from output directory#1514maxime-bentin-cko wants to merge 5 commits intodyoshikawa:mainfrom
Conversation
…ystem compatibility - Update commands-processor.test.ts to expect baseDir parameter in RulesyncCommand.fromFile() calls - Update mcp-processor.test.ts to expect baseDir parameter in RulesyncMcp.fromFile() calls - Fix setupTestDirectory to use $TMPDIR environment variable for sandbox-friendly test directory creation - Resolves EPERM filesystem permission errors in test suite by using sandboxed-allowed paths Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4a4ab3a to
e1eff88
Compare
There was a problem hiding this comment.
Pull request overview
Adds a --rulesync-dir option to decouple where Rulesync reads .rulesync/ sources from where it writes generated tool configs, enabling centralized/shared rule stores (e.g. ~/.aiglobal/.rulesync) while still generating into the working directory (or --base-dirs).
Changes:
- Add
rulesyncDirto config + resolver and propagate it through feature processor constructors and file loaders. - Update
rulesync generateCLI flags (--rulesync-dir,--base-dirs) and threadrulesyncDirthrough the generate pipeline. - Update tests and documentation to reflect the new source-directory behavior.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/file.ts | Allows absolute baseDirs by skipping traversal checks for absolute paths. |
| src/types/feature-processor.ts | Adds rulesyncDir to base FeatureProcessor state/constructor. |
| src/types/dir-feature-processor.ts | Adds rulesyncDir to base DirFeatureProcessor state/constructor. |
| src/test-utils/test-directories.ts | Changes temp test directory root selection (now considers TMPDIR). |
| src/lib/import.test.ts | Updates mock config to include getRulesyncDir. |
| src/lib/generate.ts | Passes rulesyncDir into each feature processor. |
| src/lib/generate.test.ts | Updates mock config to include getRulesyncDir. |
| src/index.test.ts | Updates mock config to include getRulesyncDir. |
| src/features/subagents/subagents-processor.ts | Loads subagent sources from rulesyncDir and passes it into parsing. |
| src/features/subagents/rulesync-subagent.ts | Makes source parsing baseDir-configurable (instead of hardcoded cwd). |
| src/features/skills/skills-processor.ts | Loads skills sources from rulesyncDir instead of cwd. |
| src/features/rules/rulesync-rule.ts | Makes rule source parsing baseDir-configurable. |
| src/features/rules/rules-processor.ts | Reads rule sources from rulesyncDir and passes it into parsing. |
| src/features/mcp/rulesync-mcp.ts | Adds baseDir support to MCP source parsing params. |
| src/features/mcp/mcp-processor.ts | Reads MCP source from rulesyncDir. |
| src/features/mcp/mcp-processor.test.ts | Updates expectation for MCP parsing call to include baseDir. |
| src/features/ignore/rulesync-ignore.ts | Makes ignore source parsing baseDir-configurable. |
| src/features/ignore/ignore-processor.ts | Reads ignore source from rulesyncDir. |
| src/features/commands/rulesync-command.ts | Makes command source parsing baseDir-configurable. |
| src/features/commands/commands-processor.ts | Reads command sources from rulesyncDir and passes it into parsing. |
| src/features/commands/commands-processor.test.ts | Updates expectations to include baseDir and absolute glob roots. |
| src/config/config.ts | Adds rulesyncDir to config schema/state with getRulesyncDir() accessor. |
| src/config/config-resolver.ts | Threads rulesyncDir through resolution (incl. config path base + global override). |
| src/cli/index.ts | Adds --rulesync-dir option and renames --base-dir → --base-dirs. |
| src/cli/commands/import.test.ts | Updates mock config to include getRulesyncDir. |
| src/cli/commands/generate.ts | Checks .rulesync existence based on config.getRulesyncDir(). |
| src/cli/commands/generate.test.ts | Updates mock config to include getRulesyncDir. |
| skills/rulesync/cli-commands.md | Documents --rulesync-dir and expands generate command reference. |
| docs/reference/cli-commands.md | Documents --rulesync-dir and expands generate command reference. |
| docs/guide/separate-rulesync-dir.md | New guide explaining centralized rules and interaction with --global. |
dyoshikawa
left a comment
There was a problem hiding this comment.
Thanks for this — decoupling the source .rulesync/ from the output dir is a real quality-of-life win for teams with shared rules. A few concerns before merge:
-
rulesyncDiris threaded into rules/ignore/mcp/commands/subagents/skills butHooksProcessorandPermissionsProcessorstill read fromprocess.cwd(), so a user runningrulesync generate --rulesync-dir ~/.aiglobalwill get a split-brain state where hooks and permissions silently come from CWD while everything else comes from the central dir. See comment onsrc/lib/generate.ts. -
Renaming
-b, --base-dirto-b, --base-dirsis a breaking CLI change that's unrelated to this feature and contradicts the PR description's "default behaviour unchanged" claim. See comment onsrc/cli/index.ts. -
configByFile.rulesyncDirappears to be unreachable becausemergeConfigsdoes not copy the field through — so users who put"rulesyncDir"inrulesync.jsoncwill have it silently dropped. See comment onsrc/config/config-resolver.ts. -
The new guide
docs/guide/separate-rulesync-dir.mdis not registered in the VitePress sidebar, which means it won't render on the docs site and — becausescripts/sync-skill-docs.tsiterates the sidebar — it will never be synced toskills/rulesync/. That breaks the repo invariant. -
The
test-directories.tschange touches the whole test suite and looks unrelated to the feature. Worth splitting off. -
No e2e coverage was added. The project convention is to keep an end-to-end happy-path per Tool × Feature matrix entry; an e2e that proves "read from dir A, write to dir B" through the actual CLI would be valuable.
On the security side nothing critical stood out, but validateBaseDir is now a no-op for absolute paths — see that comment.
| .option("--delete", "Delete all existing files in output directories before generating") | ||
| .option( | ||
| "-b, --base-dir <paths>", | ||
| "-b, --base-dirs <paths>", |
There was a problem hiding this comment.
Renaming -b, --base-dir to -b, --base-dirs is a breaking change for anyone already scripting --base-dir (commander will reject the old flag as unknown). The PR description says "default behaviour unchanged", which isn't quite true with this rename bundled in. Could we either keep the old name as a deprecated alias (commander supports defining both) or split the rename into its own release-noted PR?
| getDefaults().gitignoreDestination, | ||
| dryRun: dryRun ?? configByFile.dryRun ?? getDefaults().dryRun, | ||
| check: check ?? configByFile.check ?? getDefaults().check, | ||
| rulesyncDir: rulesyncDir ?? configByFile.rulesyncDir ?? getDefaults().rulesyncDir, |
There was a problem hiding this comment.
configByFile.rulesyncDir looks unreachable here: mergeConfigs (~lines 81-104) does not copy the rulesyncDir field through, so putting "rulesyncDir": "..." in rulesync.jsonc is silently dropped. Either extend mergeConfigs and add a test that covers config-file sourcing, or explicitly reject rulesyncDir at the schema level and document that this is CLI/programmatic only.
| // When --rulesync-dir is explicitly provided the user is decoupling source | ||
| // from output, so "global: true" from the config file must not apply unless | ||
| // the caller also explicitly passes --global. | ||
| const configGlobal = rulesyncDir !== undefined ? false : configByFile.global; |
There was a problem hiding this comment.
This silently drops configByFile.global whenever rulesyncDir is set — with no warning. A user whose central rulesync.jsonc sets "global": true and then runs rulesync generate --rulesync-dir ~/.aiglobal will find the output scope has changed out from under them. Consider emitting a logger.warn when we override, or — since the file is explicitly their central config — honouring the file value when --global isn't passed explicitly.
| checkPathTraversal({ relativePath: baseDir, intendedRootDir: process.cwd() }); | ||
| // Traversal check only applies to relative paths; absolute paths are | ||
| // explicitly provided by the caller and may point anywhere on the filesystem. | ||
| if (!isAbsolute(baseDir)) { |
There was a problem hiding this comment.
Before this change validateBaseDir rejected absolute paths that escape CWD. After this change any absolute path is accepted — including one passed via --base-dirs (since config-resolver.ts normalises all baseDirs to absolute before calling validateBaseDir). Net effect: the traversal check is effectively disabled for all baseDirs in practice. At minimum, please add unit tests in file.test.ts covering both the allowed absolute-path case and pathological inputs like /foo/../../etc. Consider resolving + normalising the path first and verifying the result still matches intent.
| }> { | ||
| const testsDir = join(originalCwd, "tmp", "tests"); | ||
| // Use TMPDIR environment variable to ensure writes are sandboxed-friendly | ||
| const root = process.env.TMPDIR || originalCwd; |
There was a problem hiding this comment.
This change is unrelated to --rulesync-dir and affects every test in the repo (the default root flips from ./tmp/tests to $TMPDIR). It also conflicts with .claude/rules/testing-guidelines.md, which pins the convention to ./tmp/tests/projects/{RANDOM}. If the sandbox filesystem restriction is real, fixing it by rewriting the shared helper feels like the wrong layer — could we either set TMPDIR in the sandbox env itself, or at minimum split this out into its own PR so it can be reviewed on its own merits?
| const baseDir = process.cwd(); | ||
| static async fromFile({ | ||
| baseDir = process.cwd(), | ||
| }: { baseDir?: string } = {}): Promise<RulesyncIgnore> { |
There was a problem hiding this comment.
Minor consistency nit: every other RulesyncXxx.fromFile in the repo uses a named parameter type (RulesyncFileFromFileParams, or a Pick<…> of it — see rulesync-mcp.ts, rulesync-subagent.ts). An inline anonymous type here breaks that pattern. Worth defining RulesyncIgnoreFromFileParams = Pick<RulesyncFileFromFileParams, "baseDir"> and using it.
| try { | ||
| const processor = new IgnoreProcessor({ | ||
| baseDir: baseDir === process.cwd() ? "." : baseDir, | ||
| rulesyncDir: config.getRulesyncDir(), |
There was a problem hiding this comment.
rulesyncDir is threaded into the processors here and in the other generateXxxCore functions, but generateHooksCore and generatePermissionsCore (further down in this file) still construct their processors without it. HooksProcessor.loadRulesyncFiles and PermissionsProcessor.loadRulesyncFiles still call RulesyncHooks.fromFile({ baseDir: process.cwd() }) / RulesyncPermissions.fromFile({ baseDir: process.cwd() }). Running with --rulesync-dir ~/.aiglobal will therefore read hooks/permissions from CWD while reading everything else from the central dir — a subtle, silent split. Please extend both processors and their matching RulesyncHooks.fromFile / RulesyncPermissions.fromFile the same way.
| @@ -0,0 +1,52 @@ | |||
| # Separate Rulesync Directory | |||
There was a problem hiding this comment.
This page isn't registered in docs/.vitepress/config.ts sidebar, so two things follow: (1) the page won't appear in the docs site navigation, and (2) scripts/sync-skill-docs.ts iterates the sidebar, so skills/rulesync/separate-rulesync-dir.md will never be generated. That breaks the repo's docs/ ↔ skills/rulesync/ sync invariant. Please add an entry under the Guide section and re-run the sync script.
Also: the "global is not applied" note at the end of this file is load-bearing behaviour — worth promoting it out of a blockquote into a ::: warning admonition so readers don't miss it.
|
Follow-up on the naming discussion — proposing this split: In scope for this PR: rename Deferred to a separate issue: rename the existing This means the two names will be temporarily asymmetric ( Does this split work for you? |
|
@dyoshikawa Yes that works for me. I will have time to work on that only on Friday. Cheers 😄 |
Problem
Currently
rulesync generatealways reads source rules from.rulesync/inside the current working directory. This forces users to duplicate their rules in every repository.Solution
Add a
--rulesync-dir <path>CLI flag that points to the parent of the.rulesync/directory. When set, all feature processors read source files from<rulesyncDir>/.rulesync/while generated output still lands in the working directory (or--base-dirspaths).This enables a single central rules store — e.g.
~/.aiglobal/.rulesync/— shared across any number of repositories:Changes
src/cli/index.ts): adds--rulesync-dir <path>option to thegeneratecommand; also fixes--base-dir→--base-dirs(plural, consistent with existing code)src/config/config.ts): addsrulesyncDirtoConfigParams/Config;getRulesyncDir()falls back toprocess.cwd()when not set (no breaking change)rules,ignore,mcp,commands,subagents,skills): accept and propagaterulesyncDirso source-file resolution is decoupled from the output base directorysrc/types/dir-feature-processor.ts,feature-processor.ts): exposerulesyncDiron processor option typesrulesyncDirthreaded throughgenerate.tsintegration layerChecklist
pnpm cicheck)--base-dirs,--dry-run,--check, and all--features