Skip to content

Follow-ups from PR #2314 review: flattenedCommandNaming convert wiring, docs wording, and tests #2323

Description

@dyoshikawa

Background

PR #2314 added the flattenedCommandNaming config option ("basename" | "path") so that nested command files flattened for single-namespace tools (e.g. Cursor) can use collision-resistant path-derived names. The review found no merge blockers, so the PR was merged with the following non-blocking findings deferred to this issue.

Details

  1. (mid) rulesync convert ignores the option. buildCommandsStrategy in src/lib/convert.ts (~line 259) constructs CommandsProcessor without passing config.getFlattenedCommandNaming(), so the config value is silently ignored on the convert path and flattening falls back to "basename". A user with "path" configured gets collision-free names from rulesync generate but colliding names from rulesync convert. One-line fix.
  2. (low) Overpromising wording. The comment in src/types/features.ts (~line 33) and the docs in docs/guide/configuration.md + skills/rulesync/configuration.md say "path" keeps flattened names unique, but collisions are still possible (a literal pj-test.md vs pj/test.md both map to pj-test.md, last-write-wins with a warning). Soften to something like "reduces collisions; the collision warning still applies". Copilot's review comments on the PR flagged the same thing.
  3. (low) Missing config-resolution tests. There is no test asserting that flattenedCommandNaming from a config file reaches Config, that rulesync.local.jsonc overrides the base value, or that an invalid value (e.g. "foo") is rejected by ConfigFileSchema. The comparable sources option has such coverage in config.test.ts.
  4. (low) Hand-rolled path splitting. The flattening in src/features/commands/commands-processor.ts uses relativeFilePath.split(/[\\/]/).join("-"); per the coding guidelines, toPosixPath(relativeFilePath).split("/") from src/utils/file.ts is the conventional approach.
  5. (low) Missing migration note in docs. Switching from "basename" to "path" renames the generated files (e.g. test.mdpj-test.md); without delete: true the stale old flat-named files remain, so the tool shows both old and new commands. Docs should mention this.
  6. (low, design note) Top-level key vs featureOptions. The option was added as a top-level config key rather than under the existing per-target/per-feature featureOptions mechanism, which would have given per-tool control and automatic propagation to the convert path (mitigating item 1). Precedent exists either way (simulateCommands is top-level), but this should be revisited before the shape ossifies — moving it later would be a breaking change.

Solution / Next Steps

  • Wire config.getFlattenedCommandNaming() into the CommandsProcessor construction in src/lib/convert.ts and add a regression test for the convert path.
  • Soften the uniqueness wording in src/types/features.ts, docs/guide/configuration.md, and skills/rulesync/configuration.md (keep docs/skills in sync via scripts/sync-skill-docs.ts).
  • Add config-resolution tests for the new option (file → Config, local override, invalid value rejection).
  • Switch the flattening to toPosixPath(...).split("/").
  • Add a one-sentence migration note about stale files and delete: true to the docs.
  • Decide whether flattenedCommandNaming should move under featureOptions before the next major release; if yes, plan the deprecation.

Metadata

Metadata

Assignees

No one assigned

    Labels

    improvementmaintainer-scrapRough notes for AI implementation. Not for human eyes.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions