Skip to content

fix: copy command sidecar directories during install#172

Open
trevor-vaughan wants to merge 1 commit into
LobsterTrap:mainfrom
trevor-vaughan:fix/command-sidecars
Open

fix: copy command sidecar directories during install#172
trevor-vaughan wants to merge 1 commit into
LobsterTrap:mainfrom
trevor-vaughan:fix/command-sidecars

Conversation

@trevor-vaughan

@trevor-vaughan trevor-vaughan commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Multi-file commands pair an entry file (commands/.md) with a co-named directory (commands//) of supporting files. Only the entry file was copied to the target, so relative-path references from the entry into the sidecar directory dangled at runtime.

_generate_passthrough_command now copies the sidecar directory next to the entry file, replacing any stale copy on reinstall. remove_command cleans up both the entry and the sidecar directory on uninstall.

  • Add tests for sidecar copy, no-sidecar no-op, overwrite on reinstall, and removal (ClaudeCode and OpenCode targets)
  • Document multi-file command structure in AGENTS.md

Fixes: #171

Checklist

  • Tests pass (pytest)
  • Linting passes (ruff check src tests)
  • Type checking passes (ty check)

AI Disclosure

  • Claude Code was used during this PR

Summary by CodeRabbit

  • New Features

    • Install full module trees per target under modules//, support co-named sidecar directories for multi-file commands, and inject visible module-dir preambles into generated skills/commands/agents (Gemini CLI remains a TOML exception). Per-target module path resolution added for user/project scopes.
  • Documentation

    • Expanded module structure, path behavior, installation pipeline, CLI reference, and authoring guides to document install layout and preamble behavior.
  • Tests

    • Expanded coverage for module-tree install/uninstall lifecycle, sidecar handling, preamble injection, TOML parsing, and per-target integration.
  • Chores

    • .gitignore updated to ignore docs/superpowers/; lint target now runs an additional type-check step.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Copies full module content trees into assistant-specific modules//, injects boundary module-dir preambles into generated skills/agents/commands when provided, synchronizes co-named command sidecar directories on install/uninstall, adds per-target get_module_path helpers, and updates docs and tests.

Changes

Module install and multi-file command support

Layer / File(s) Summary
Documentation and README updates
AGENTS.md, README.md, docs/*, .gitignore, mk/dev.mk
Document module-dir preamble, per-target install paths, CLI reference updates, multi-file command sidecar layout, installation pipeline, and add ty check to lint.
Base target: preamble helpers, passthrough & agent frontmatter
src/lola/targets/base.py
Add _build_module_dir_preamble / _inject_preamble and _resolve_skill_md_path, extend _generate_passthrough_command to inject preamble and copy/remove co-named command sidecar directories, extend _generate_agent_with_frontmatter to inject preamble, and update abstract method signatures to accept module_dir.
Per-target wiring: module path getters & module_dir threading
src/lola/targets/claude_code.py, src/lola/targets/cursor.py, src/lola/targets/gemini.py, src/lola/targets/openclaw.py, src/lola/targets/opencode.py
Add get_module_path helpers, accept module_dir in generate APIs, and apply _inject_preamble when writing SKILL.md or agent/frontmatter where applicable.
Install/uninstall orchestration and module-tree helpers
src/lola/targets/install.py
Add _install_module_tree and _uninstall_module_tree, call them from install/uninstall flows, pass module_dir into install helpers, tighten hook/error handling, and refactor summary formatting.
Tests: unit, integration, module-tree lifecycle and preamble behavior
tests/*
Extend tests to cover sidecar copy/overwrite/removal, module-dir preamble injection/omission across targets, installer hook error cases, instruction regeneration/removal, and end-to-end module-tree install/uninstall flows.
Misc: lint step
mk/dev.mk
Run ty check as part of the lint target.

Sequence Diagram

sequenceDiagram
  participant CLI as Installer (install_to_assistant)
  participant InstallerHelper as _install_module_tree
  participant Target as AssistantTarget.generate_*
  participant Helper as _inject_preamble
  participant FS as Filesystem
  CLI->>InstallerHelper: copy module tree -> modules/<name>/
  InstallerHelper->>FS: create modules/<name>/ with nested files
  CLI->>Target: call generate_skill/generate_command/generate_agent(module_dir=modules/<name>/)
  Target->>Helper: pass content + module_dir
  Helper->>Target: return injected content
  Target->>FS: write generated files and copy/remove sidecar directories
  CLI->>Target: on uninstall -> _uninstall_module_tree -> remove modules/<name> and installed artifacts
Loading

Estimated code review effort:
🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs:

Suggested labels:
bug

Suggested reviewers:

  • mrbrandao

"— a rabbit wrote —
I hopped through module trees,
I left a preamble breeze,
Sidecars carried in my pack,
Clean uninstalls bring them back,
Hooray — modules travel with ease! 🐰"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes substantial scope expansion beyond the core sidecar-copy fix: module tree installation/uninstallation, module_dir preamble injection, and module path resolution across all target implementations. The PR scope has significantly expanded to include module-tree installation and preamble injection features. Consider splitting module-related changes into a separate PR to isolate the sidecar-copy fix.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly describes the main fix: copying command sidecar directories during installation, which directly addresses the root cause identified in issue #171.
Linked Issues check ✅ Passed The PR fully implements the requirements from issue #171: command sidecar directory copying in _generate_passthrough_command, removal on uninstall, tests for sidecar scenarios, and documentation updates in AGENTS.md.
Docstring Coverage ✅ Passed Docstring coverage is 88.49% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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: d39bfa5d80

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/lola/targets/base.py

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🧹 Nitpick comments (1)
tests/test_targets.py (1)

217-270: ⚡ Quick win

Add Cursor sidecar and OpenCode sidecar-removal coverage to complete the contract.

These tests validate the shared behavior well, but there’s still no explicit Cursor sidecar test and no OpenCode-side removal assertion (important because OpenCodeTarget.remove_command overrides then delegates to super()).

Also applies to: 961-975

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_targets.py` around lines 217 - 270, Add two tests mirroring the
existing ClaudeCodeTarget sidecar behavior: one that ensures the Cursor sidecar
directory (create a co-named directory next to the command source with nested
files) is copied when calling the Cursor target's generate_command (reference
CursorTarget or the Cursor-specific target class), and another that ensures
OpenCodeTarget.remove_command actually removes the sidecar directory when
uninstalling (assert that after calling OpenCodeTarget.remove_command the
co-named directory under the destination no longer exists). Implement these
tests analogous to test_generate_command_copies_sidecar_directory and
test_generate_command_overwrites_existing_sidecar so they create source
sidecars, call the appropriate target methods, then assert presence/absence of
files; reference the generate_command method and OpenCodeTarget.remove_command
to locate the relevant behavior to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lola/targets/base.py`:
- Around line 783-788: The current logic may call shutil.rmtree(sidecar_dest)
even when sidecar_dest is a file or symlink, causing an exception before
shutil.copytree; update the branch around sidecar_dest to check its type: if
sidecar_dest.exists() then if sidecar_dest.is_dir() call
shutil.rmtree(sidecar_dest), elif sidecar_dest.is_file() or
sidecar_dest.is_symlink() call sidecar_dest.unlink() (or os.remove), otherwise
handle other special file types appropriately, then proceed to
shutil.copytree(sidecar_src, sidecar_dest); apply this change where sidecar_src
and sidecar_dest are set.
- Around line 364-367: The code unconditionally removes the sidecar_dir in
remove_command(), affecting non-passthrough targets; change it to only remove
the co-named sidecar for passthrough/file-based command targets by gating the
shutil.rmtree call behind a check (e.g., if getattr(self, "is_passthrough",
False) or getattr(self, "file_based", False) or isinstance(self,
PassthroughCommandTarget)): compute sidecar_dir = dest_dir / Path(filename).stem
as before but only call sidecar_dir.is_dir() and shutil.rmtree(sidecar_dir) when
that passthrough/file-based predicate is true; update or add the minimal boolean
property/method on relevant subclasses instead of altering unrelated targets
that inherit BaseAssistantTarget.

---

Nitpick comments:
In `@tests/test_targets.py`:
- Around line 217-270: Add two tests mirroring the existing ClaudeCodeTarget
sidecar behavior: one that ensures the Cursor sidecar directory (create a
co-named directory next to the command source with nested files) is copied when
calling the Cursor target's generate_command (reference CursorTarget or the
Cursor-specific target class), and another that ensures
OpenCodeTarget.remove_command actually removes the sidecar directory when
uninstalling (assert that after calling OpenCodeTarget.remove_command the
co-named directory under the destination no longer exists). Implement these
tests analogous to test_generate_command_copies_sidecar_directory and
test_generate_command_overwrites_existing_sidecar so they create source
sidecars, call the appropriate target methods, then assert presence/absence of
files; reference the generate_command method and OpenCodeTarget.remove_command
to locate the relevant behavior to test.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f4da590d-29a3-41ca-8a74-2c8aec685be8

📥 Commits

Reviewing files that changed from the base of the PR and between 55b3770 and d39bfa5.

📒 Files selected for processing (3)
  • AGENTS.md
  • src/lola/targets/base.py
  • tests/test_targets.py

Comment thread src/lola/targets/base.py
Comment thread src/lola/targets/base.py
@trevor-vaughan trevor-vaughan force-pushed the fix/command-sidecars branch 4 times, most recently from 7a6dda5 to cd7cf1f Compare June 3, 2026 02:15

@coderabbitai coderabbitai 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.

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (2)
src/lola/targets/base.py (1)

95-103: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

This abstract API change is currently breaking typecheck.

CI is already failing with invalid overrides in tests/test_targets_base.py for these AssistantTarget methods. The remaining subclasses/test doubles need to be updated to the new signatures before merge, otherwise this contract expansion keeps the PR red.

Also applies to: 116-124, 139-147, 163-168

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lola/targets/base.py` around lines 95 - 103, The base API for
generate_skill has expanded to include project_path: str | None = None and a
keyword-only module_dir: Path | None = None, which breaks existing overrides;
update every AssistantTarget subclass and test double that overrides
generate_skill to add the new parameters with matching type annotations and
defaults and forward or use them as appropriate, and make identical signature
fixes to the other base methods referenced (the overrides corresponding to the
diffs at 116-124, 139-147, 163-168) so their signatures match the base class and
keep the return type bool.
tests/test_targets_base.py (1)

45-104: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fix MockTarget method signatures to match AssistantTarget

tests/test_targets_base.py defines MockTarget.generate_skill(), generate_command(), and generate_agent() with kw-only parameter _module_dir, which doesn’t match AssistantTarget’s kw-only module_dir: Path | None (and the override stubs are missing matching type/return annotations). This is why invalid-method-override is triggered and why module_dir=... keyword calls would be rejected by this mock.

Suggested fix pattern
-    def generate_skill(self, _source_path, _dest_path, _skill_name, _project_path=None, *, module_dir=None):
+    def generate_skill(
+        self,
+        _source_path: Path,
+        _dest_path: Path,
+        _skill_name: str,
+        _project_path: str | None = None,
+        *,
+        module_dir: Path | None = None,
+    ) -> bool:
         return True

-    def generate_command(self, _source_path, _dest_dir, _cmd_name, _module_name, *, module_dir=None):
+    def generate_command(
+        self,
+        _source_path: Path,
+        _dest_dir: Path,
+        _cmd_name: str,
+        _module_name: str,
+        *,
+        module_dir: Path | None = None,
+    ) -> bool:
         return True

-    def generate_agent(
-        self,
-        _source_path,
-        _dest_dir,
-        _agent_name,
-        _module_name,
-        *,
-        _module_dir=None,
-    ):
+    def generate_agent(
+        self,
+        _source_path: Path,
+        _dest_dir: Path,
+        _agent_name: str,
+        _module_name: str,
+        *,
+        module_dir: Path | None = None,
+    ) -> bool:
         return True

Apply the same “signature + return type” alignment to the other stubbed overrides in this block (remove_skill, remove_instructions, generate_skills_batch, get_command_filename, get_agent_filename, generate_mcps, remove_mcps, remove_command, remove_agent), and then rerun ruff check.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_targets_base.py` around lines 45 - 104, Update MockTarget method
signatures to exactly match AssistantTarget: change kw-only parameter names to
module_dir: Path | None (not _module_dir), add matching return type annotations
(e.g., -> bool or -> Path | str where appropriate) and align parameter types for
generate_skill, generate_command, generate_agent, generate_instructions,
remove_skill, remove_instructions, generate_skills_batch, get_command_filename,
get_agent_filename, generate_mcps, remove_mcps, remove_command, and
remove_agent; ensure default/kw-only placement is preserved so calls like
module_dir=... are accepted and then run ruff check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/dev-guide/architecture.md`:
- Around line 45-46: Update the architecture doc's numbered list (the "Skills"
and "Commands" items) to qualify that copying/registering of sidecar directories
applies only to passthrough/file-based command targets; explicitly note that
Gemini commands are generated as TOML and do not install markdown sidecar
directories so the description matches the implementation and PR objective.

In `@docs/guides/creating-modules.md`:
- Around line 88-99: The fenced code block showing the module directory tree is
missing a language tag; update the opening fence from ``` to a language label
(for example ```text) for the block that contains the my-module/module directory
tree so markdownlint stops flagging it.
- Around line 101-102: The current text promises a boundary-delimited module
directory preamble for every generated command, but Gemini emits TOML commands
so that statement is too broad; update the sentence that begins "Generated
skill, command, and agent files receive a boundary-delimited module directory
preamble..." to explicitly scope the preamble to generated skills, agents, and
file-based markdown command outputs (e.g., "Generated skill and agent files, and
file-based markdown commands, receive..."), and add a clarifying clause that
Gemini-emitted TOML command files do not include the markdown preamble.

In `@src/lola/targets/install.py`:
- Around line 227-230: The Cursor branch currently checks for a nonexistent
`.mdc` file so installed Cursor skills are never detected; update the check in
the function that uses target.name (used by _install_skills()) to detect Cursor
installs by testing the actual install directory or SKILL.md file that
CursorTarget.generate_skill() writes (e.g. (skill_dest / skill_name /
"SKILL.md").exists() or (skill_dest / skill_name).exists()), so the code will
correctly honor force=False and prompt/skip overwrites.

---

Outside diff comments:
In `@src/lola/targets/base.py`:
- Around line 95-103: The base API for generate_skill has expanded to include
project_path: str | None = None and a keyword-only module_dir: Path | None =
None, which breaks existing overrides; update every AssistantTarget subclass and
test double that overrides generate_skill to add the new parameters with
matching type annotations and defaults and forward or use them as appropriate,
and make identical signature fixes to the other base methods referenced (the
overrides corresponding to the diffs at 116-124, 139-147, 163-168) so their
signatures match the base class and keep the return type bool.

In `@tests/test_targets_base.py`:
- Around line 45-104: Update MockTarget method signatures to exactly match
AssistantTarget: change kw-only parameter names to module_dir: Path | None (not
_module_dir), add matching return type annotations (e.g., -> bool or -> Path |
str where appropriate) and align parameter types for generate_skill,
generate_command, generate_agent, generate_instructions, remove_skill,
remove_instructions, generate_skills_batch, get_command_filename,
get_agent_filename, generate_mcps, remove_mcps, remove_command, and
remove_agent; ensure default/kw-only placement is preserved so calls like
module_dir=... are accepted and then run ruff check.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0bd3331b-5503-486d-99fe-c0e352579fbe

📥 Commits

Reviewing files that changed from the base of the PR and between 5ebac01 and a329bdd.

📒 Files selected for processing (18)
  • .gitignore
  • AGENTS.md
  • README.md
  • docs/dev-guide/architecture.md
  • docs/guides/creating-modules.md
  • docs/guides/modules.md
  • src/lola/targets/base.py
  • src/lola/targets/claude_code.py
  • src/lola/targets/cursor.py
  • src/lola/targets/gemini.py
  • src/lola/targets/install.py
  • src/lola/targets/openclaw.py
  • src/lola/targets/opencode.py
  • tests/test_cli_install.py
  • tests/test_installer.py
  • tests/test_instructions.py
  • tests/test_targets.py
  • tests/test_targets_base.py
✅ Files skipped from review due to trivial changes (4)
  • .gitignore
  • docs/guides/modules.md
  • README.md
  • AGENTS.md

Comment thread docs/dev-guide/architecture.md Outdated
Comment thread docs/guides/creating-modules.md Outdated
Comment thread docs/guides/creating-modules.md Outdated
Comment thread src/lola/targets/install.py Outdated
@trevor-vaughan trevor-vaughan force-pushed the fix/command-sidecars branch 5 times, most recently from 16c1d87 to ed9626b Compare June 3, 2026 20:54
@trevor-vaughan

Copy link
Copy Markdown
Contributor Author

Had to refactor this a couple of times based on experimentation. It turns out that LLMs will ignore blocks of text in the lola comment blocks as unimportant when compared to other prose. The current version seems to work as I expect with at least Sonnet and Opus able to follow the instructions correctly the first time.

@trevor-vaughan trevor-vaughan force-pushed the fix/command-sidecars branch from ed9626b to 0bfb743 Compare June 5, 2026 15:36
@trevor-vaughan

Copy link
Copy Markdown
Contributor Author

Rebased

@mrbrandao mrbrandao self-requested a review June 11, 2026 19:41
@mrbrandao mrbrandao self-assigned this Jun 11, 2026
@mrbrandao mrbrandao added the enhancement New feature or request label Jun 11, 2026
@trevor-vaughan

Copy link
Copy Markdown
Contributor Author

Rebased

@mrbrandao mrbrandao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks good to me, thanks for adding this.
I small note, would be cool to mention also

Comment thread README.md Outdated
@mrbrandao

Copy link
Copy Markdown
Collaborator

This code is good to merge; the only issue is more related to the markdown formatter, which would be better to have in a separate PR, to avoid the noise.
Everything else is good. Please adjust the formatter and send it again, so we can merge it.
Thanks for the improvements here in the tests, by the way.

Install now copies the full module content tree to each target's
modules/<name>/ directory, keeping internal relative paths intact
(convention packs, reference docs, etc.).

Commands can now have co-named sidecar directories. A command like
commands/review.md can ship with commands/review/ holding supporting
files that get copied alongside it.

Every installed asset (skill, command, agent) gets a plain-text
module-dir preamble block between frontmatter and body:

  Module root: <path>
  This is the installed root of the module. Resolve all relative
  file references in this document against the path above.
  Example: packs/foo.md -> <path>/packs/foo.md

Gemini CLI commands use TOML, so they skip the preamble. The
ManagedSectionTarget writes a Markdown-formatted module directory
line in the skills batch section instead.

Also adds `ty check` to `make lint` to match CI.

Fixes: LobsterTrap#171
@trevor-vaughan trevor-vaughan removed their assignment Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] - Multi-file commands install without their sidecar directory

2 participants