Add a dda command to automatically sync cursor rules in other coding agent files #179
Conversation
KevinFairise2
force-pushed
the
kfairise/dda-ai-rules-sync
branch
5 times, most recently
from
e34c12c to
746b659
Compare
KevinFairise2
force-pushed
the
kfairise/dda-ai-rules-sync
branch
from
746b659 to
406707b
Compare
There was a problem hiding this comment.
Thanks a lot for this ! Please ask for re-review once you've had another look :)
For the naming convention, I think ai is fine, but only if we choose to also merge all mcp commands under here (so we'd have dda ai mcp ...). @ofek what do you think ?
|
|
||
|
|
||
| @dynamic_group( | ||
| short_help="AI assistant tools and utilities", |
There was a problem hiding this comment.
nit: IIRC we prefer to use verbal phrases for help texts, so maybe something like
| short_help="AI assistant tools and utilities", | |
| short_help="Manage and configure AI assistants / utilities", |
would be better ?
| "--cursor-rules-path", | ||
| "-c", | ||
| "cursor_rules_path", | ||
| type=click.Path(exists=True, path_type=Path), |
There was a problem hiding this comment.
nit: should probably check that this is a directory and not a file
| type=click.Path(exists=True, path_type=Path), | |
| type=click.Path(exists=True, path_type=Path, file_okay=False), |
| ) | ||
| @pass_app | ||
| def cmd(app: Application, *, cursor_rules_path: Path, sync_targets: str, check_sync: bool) -> None: | ||
| cursor_rules_dir = Path(cursor_rules_path) |
There was a problem hiding this comment.
Not necessary ?
| cursor_rules_dir = Path(cursor_rules_path) |
Will also need to rename cursor_rules_dir -> cursor_rules_path
| def cmd(app: Application, *, cursor_rules_path: Path, sync_targets: str, check_sync: bool) -> None: | ||
| cursor_rules_dir = Path(cursor_rules_path) | ||
| targets_files = [Path(target) for target in sync_targets.split(",")] |
There was a problem hiding this comment.
Required changes with above nargs=-1 change. Also a good idea to check for len(sync_targets) > 0
| def cmd(app: Application, *, cursor_rules_path: Path, sync_targets: str, check_sync: bool) -> None: | |
| cursor_rules_dir = Path(cursor_rules_path) | |
| targets_files = [Path(target) for target in sync_targets.split(",")] | |
| def cmd(app: Application, *, cursor_rules_path: Path, sync_targets: tuple[Path, ...], check_sync: bool) -> None: | |
| cursor_rules_dir = Path(cursor_rules_path) | |
| if len(sync_targets) == 0: | |
| app.display_warning("Please specify at least one sync target") | |
| return |
Will also need to rename targets_files -> sync_targets
| for root, _, files in os.walk(cursor_rules_dir): | ||
| root_path = Path(root) | ||
| # More robust check for personal rules using path parts | ||
| if any(part == "personal" for part in root_path.parts): |
There was a problem hiding this comment.
| if any(part == "personal" for part in root_path.parts): | |
| if "personal" in root_path.parts: |
| for target_file in targets_files: | ||
| content = generate_content(rule_files, target_file) | ||
| if not target_file.exists() and content: | ||
| unsynced_targets.append(str(target_file)) |
There was a problem hiding this comment.
I'd also add a debug output in this branch, no reason why we wouldn't have one here since we have one everywhere else
| unsynced_targets.append(str(target_file)) | |
| unsynced_targets.append(str(target_file)) | |
| app.display_debug(f"Error: {target_file} does not exist !") |
| for target_file in targets_files: | ||
| content = generate_content(rule_files, target_file) | ||
| try: | ||
| app.display_info(f"Syncing {len(rule_files)} rule files to {target_file}") |
There was a problem hiding this comment.
| app.display_info(f"Syncing {len(rule_files)} rule files to {target_file}") | |
| app.display_debug(f"Syncing {len(rule_files)} rule files to {target_file}") |
| @click.option( | ||
| "--sync-targets", | ||
| "-s", | ||
| "sync_targets", | ||
| type=str, | ||
| help="Comma separated list of targets files to sync the rules to. Defaults to Claude.MD file.", | ||
| default="CLAUDE.md", | ||
| ) |
There was a problem hiding this comment.
I think we should avoid manually splitting along commas like this. Click has two methods for passing in an arbitrary number of arguments:
Option 1: option with multiple=True
By passing multiple=True you can pass the option multiple times, i.e.
dda ai rules sync -s CLAUDE.md -s AGENTs.mdSo we would have:
| @click.option( | |
| "--sync-targets", | |
| "-s", | |
| "sync_targets", | |
| type=str, | |
| help="Comma separated list of targets files to sync the rules to. Defaults to Claude.MD file.", | |
| default="CLAUDE.md", | |
| ) | |
| @click.option( | |
| "--sync-target", # Maybe we could simplify to --target as well ? | |
| "-s", | |
| "sync_targets", | |
| type=click.Path(dir_okay=False, writable=True, path_type=Path), | |
| multiple=True, | |
| help="Target file to sync the rules to. Defaults to CLAUDE.md file. Can be passed multiple times", | |
| default=[Path("CLAUDE.md")], | |
| ) |
Option 2: argument with nargs=-1
I personally prefer this option, as it seems more inline with what we are trying to do.
Instead of using an option we should use a click.argument and pass nargs=-1. This way we do not have to use -s to specify files to sync and can pass in an arbitrary number of files on the CLI, like this:
dda ai rules sync CLAUDE.md AGENTS.mdThe caveat is that we won't be able to set a default cleanly like this, as setting a default is not supported with nargs=-1. We also lose the help.
What we can do, however, is to add this to the body of our function:
if len(sync_targets) == 0:
sync_targets = (Path("CLAUDE.md"),)Thus we would get:
| @click.option( | |
| "--sync-targets", | |
| "-s", | |
| "sync_targets", | |
| type=str, | |
| help="Comma separated list of targets files to sync the rules to. Defaults to Claude.MD file.", | |
| default="CLAUDE.md", | |
| ) | |
| @click.parameter( | |
| "sync_targets", | |
| type=click.Path(dir_okay=False, writable=True, path_type=Path), | |
| nargs=-1 | |
| ) |
In either case this also changes the signature of the function below, see other comment
| @pytest.fixture(name="create_temp_file_or_dir") | ||
| def fixt_create_temp_file_or_dir(): | ||
| """Fixture to create and clean up temporary files and directories.""" | ||
| created_paths: list[Path] = [] | ||
|
|
||
| def _create_temp_file_or_dir(location: Path, *, force_file: bool = False) -> None: | ||
| for parent in reversed(location.parents): | ||
| # Create and keep track of created parent directories for cleanup | ||
| if not parent.exists(): | ||
| parent.mkdir() | ||
| created_paths.append(parent) | ||
|
|
||
| # Create the requested file or directory and keep track of it for cleanup | ||
| # Assume that if the file path does not have an extension, it is a directory | ||
| # The force_file flag can be used to override this behavior | ||
| if location.suffix == "" and not force_file: | ||
| location.mkdir() | ||
| else: | ||
| location.touch() | ||
| created_paths.append(location) | ||
|
|
||
| yield _create_temp_file_or_dir | ||
| for path in reversed(created_paths): | ||
| if path.exists(): | ||
| if path.is_dir(): | ||
| path.rmdir() | ||
| else: | ||
| path.unlink() | ||
|
|
||
|
|
||
| @pytest.fixture(name="create_cursor_rules") | ||
| def fixt_create_cursor_rules(create_temp_file_or_dir): | ||
| def _create_cursor_rules(rules_data: dict[str, str], cursor_rules_dir: Path) -> None: | ||
| """Create cursor rule files with given content.""" | ||
| for filename, content in rules_data.items(): | ||
| rule_file = cursor_rules_dir / filename | ||
| create_temp_file_or_dir(rule_file, force_file=True) | ||
| rule_file.write_text(content, encoding="utf-8") | ||
|
|
||
| return _create_cursor_rules |
There was a problem hiding this comment.
I think this is very similar to what I implemented in tests/cli/info/owners/test_code.py. Since the same code is now reused in multiple places, could we consider making these fixtures generic and sharing them by putting them in conftest.py ?
| return _create_cursor_rules | ||
|
|
||
|
|
||
| def test_sync_basic_functionality( |
There was a problem hiding this comment.
I think we can merge a few of these and use pytest.mark.parametrize. What do you think ?
Create a new
dda aitree used for AI and Coding agent related utilities.Implement
dda ai rules synccommand to automatically synchronize Cursor rules to other coding agent files.Default usage will pick all the rules in
.cursor/rulesand add them in CLAUDE.md file, so that claude can use the rules defined for Cursor.Also adds a
--check-syncflag to be used in the CI to check that the rules remain synchronized.