Feat - Add GitHub Copilot target (#149)#154
Open
momostallion wants to merge 13 commits into
Open
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CopilotTarget implementing GitHub Copilot on-disk layouts (skills, prompts, agents, global instructions, MCP), wires it into TARGETS and exports, updates AGENTS.md, and adds pytest coverage for generation, removal, and path resolution. ChangesGitHub Copilot target support
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CopilotTarget
participant FileSystem
Caller->>CopilotTarget: generate_skill(source_path, dest_path, skill_name)
CopilotTarget->>FileSystem: read source SKILL.md
CopilotTarget->>CopilotTarget: parse frontmatter (name, description, applyTo/globs)
CopilotTarget->>FileSystem: create skill dest dir
CopilotTarget->>FileSystem: write SKILL.md (Copilot YAML + body)
CopilotTarget-->>Caller: return True/False
Caller->>CopilotTarget: remove_skill(dest_path, skill_name)
CopilotTarget->>FileSystem: delete skill dir and legacy instructions file if present
CopilotTarget-->>Caller: return True/False
Caller->>CopilotTarget: generate_command(...)
CopilotTarget->>FileSystem: write <cmd>.prompt.md (and remove legacy variant on uninstall)
CopilotTarget-->>Caller: return True/False
Caller->>CopilotTarget: generate_agent(...)
CopilotTarget->>FileSystem: write <agent>.agent.md (and remove legacy variant on uninstall)
CopilotTarget-->>Caller: return True/False
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_copilot_target.py (1)
193-214: ⚡ Quick winAdd a regression test for legacy
.instructions.mdcleanup.There’s no test that verifies removing
.../.github/instructions/<skill>.instructions.md. Adding one would guard backward-compat uninstall behavior.🤖 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_copilot_target.py` around lines 193 - 214, Add a regression test that verifies CopilotTarget.remove_skill removes legacy instruction files: create a test (e.g., test_remove_skill_removes_legacy_instructions) which sets up tmp_path / "skills" / "my-skill" with SKILL.md and also creates the legacy file at dest / ".github" / "instructions" / "my-skill.instructions.md", call CopilotTarget().remove_skill(dest, "my-skill"), assert it returns True and both the skill_dir and the legacy instructions file no longer exist; this ensures CopilotTarget.remove_skill handles legacy `.instructions.md` cleanup.
🤖 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 `@AGENTS.md`:
- Around line 148-151: Section heading currently claims "Agent frontmatter is
modified during generation" but the Copilot bullet describes changes to skill
frontmatter; update the documentation to avoid the semantic mismatch by either
renaming the section heading to mention both "agent and skill frontmatter" or
moving the Copilot bullet into the relevant "skill frontmatter" section so the
Copilot note references the correct scope; update any nearby headings or bullets
that refer to "agent" only to ensure consistency with the Copilot bullet and
verify the Claude Code and Cursor bullets remain under the agent-focused
portion.
In `@src/lola/targets/copilot.py`:
- Around line 109-114: The legacy_file path is constructed with a duplicated
".github" segment causing paths like ".../.github/.github/instructions/...";
update the construction of legacy_file (the variable currently set from
dest_path.parent / ".github" / "instructions" / f"{skill_name}.instructions.md")
to avoid adding the extra ".github" segment — e.g., use dest_path.parent /
"instructions" / f"{skill_name}.instructions.md" or otherwise derive the correct
repo root so legacy_file points to the single ".github/instructions" location;
modify the assignment of legacy_file accordingly to remove the duplicate
segment.
- Around line 80-90: The code builds Copilot frontmatter with "name +
description" required but currently proceeds when frontmatter["description"] is
missing; update the validation before constructing copilot_fm (using variables
skill_name and frontmatter) to enforce that frontmatter contains a non-empty
"description" — if missing/empty, raise a clear exception or return an error
(e.g., ValueError or a logged failure) to prevent writing SKILL.md with invalid
frontmatter; ensure copilot_fm always includes the description field when
proceeding.
In `@tests/test_copilot_target.py`:
- Around line 110-126: The test test_generate_skill_basic currently writes a
SKILL.md without required frontmatter; update it so the source SKILL.md includes
YAML frontmatter with both name and description (e.g., add a leading/frontmatter
block containing name: my-skill and description: "..." before the body) so
CopilotTarget.generate_skill is exercising the valid contract, then assert
result is True and check that both name and description appear in the generated
file; alternatively, if you intend to test rejection, change the assertion to
expect False and verify no output file was created, but keep the test name and
references to test_generate_skill_basic and CopilotTarget.generate_skill
consistent.
---
Nitpick comments:
In `@tests/test_copilot_target.py`:
- Around line 193-214: Add a regression test that verifies
CopilotTarget.remove_skill removes legacy instruction files: create a test
(e.g., test_remove_skill_removes_legacy_instructions) which sets up tmp_path /
"skills" / "my-skill" with SKILL.md and also creates the legacy file at dest /
".github" / "instructions" / "my-skill.instructions.md", call
CopilotTarget().remove_skill(dest, "my-skill"), assert it returns True and both
the skill_dir and the legacy instructions file no longer exist; this ensures
CopilotTarget.remove_skill handles legacy `.instructions.md` cleanup.
🪄 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: 70a605d0-6d82-4a2a-8a1c-4ab30f3cd9a2
📒 Files selected for processing (4)
AGENTS.mdsrc/lola/targets/__init__.pysrc/lola/targets/copilot.pytests/test_copilot_target.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lola/targets/copilot.py (1)
109-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid early return so legacy skill artifacts are always cleaned up.
Line 112 returns immediately after removing
skill_dir, so legacy*.instructions.mdcleanup is skipped when both files exist.Suggested fix
skill_dir = dest_path / skill_name + removed = False if skill_dir.exists(): shutil.rmtree(skill_dir) - return True + removed = True # Legacy cleanup: old .instructions.md format legacy_file = ( dest_path.parent / "instructions" / f"{skill_name}.instructions.md" ) if legacy_file.exists(): legacy_file.unlink() - return True - return False + removed = True + return removed🤖 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/copilot.py` around lines 109 - 120, The cleanup currently returns immediately after removing skill_dir (using shutil.rmtree), skipping legacy artifact removal; instead, remove the early return and implement a boolean flag (e.g., removed = False) that is set to True when skill_dir is removed and again when legacy_file (dest_path.parent / "instructions" / f"{skill_name}.instructions.md") is unlinked, so both cleanup steps run and the function returns True if either artifact was removed and False otherwise.
🤖 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.
Outside diff comments:
In `@src/lola/targets/copilot.py`:
- Around line 109-120: The cleanup currently returns immediately after removing
skill_dir (using shutil.rmtree), skipping legacy artifact removal; instead,
remove the early return and implement a boolean flag (e.g., removed = False)
that is set to True when skill_dir is removed and again when legacy_file
(dest_path.parent / "instructions" / f"{skill_name}.instructions.md") is
unlinked, so both cleanup steps run and the function returns True if either
artifact was removed and False otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f901f76-3eca-4c42-933d-cd749b25981b
📒 Files selected for processing (3)
AGENTS.mdsrc/lola/targets/copilot.pytests/test_copilot_target.py
momostallion
force-pushed
the
feat/copilot-target
branch
2 times, most recently
from
0356849 to
978b12d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@AGENTS.md`:
- Line 142: Docs mismatch: the Copilot section of AGENTS.md omits user-scope
command/agent paths in the table and incorrectly states that agent frontmatter
is modified while the implementation's generate_agent is a passthrough. Update
AGENTS.md to add user-scope paths for commands and agents to the table (include
the same user ~/.copilot/skills and ~/.copilot/agents patterns) and change the
frontmatter text to reflect the actual behavior of generate_agent (either state
that generate_agent is passthrough and does not modify frontmatter, or if you
prefer to change behavior instead, implement frontmatter modification inside the
generate_agent code path so the docs and implementation match). Ensure you
reference generate_agent and the "frontmatter" wording in the updated text so
reviewers can verify consistency.
In `@src/lola/targets/copilot.py`:
- Around line 47-53: Both user-scope paths are wrong: update
get_instructions_path to use the Copilot user folder (Path.home() / ".copilot" /
self.INSTRUCTIONS_FILE) instead of "~/.github/<...>", and update get_mcp_path to
write the user MCP to the Copilot user config (when scope == "user" return
Path.home() / ".copilot" / "mcp.json") instead of "~/.github/copilot/mcp.json";
modify the base calculation in both get_instructions_path and get_mcp_path to
choose these user locations when scope == "user".
🪄 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: 324f5be9-4caf-49e0-b447-63327e35267f
📒 Files selected for processing (4)
AGENTS.mdsrc/lola/targets/__init__.pysrc/lola/targets/copilot.pytests/test_copilot_target.py
mrbrandao
added
enhancement
momostallion
force-pushed
the
feat/copilot-target
branch
from
d11643a to
7993ea1
Compare
Collaborator
|
@codex review |
![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: 7993ea157d
ℹ️ 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".
momostallion
force-pushed
the
feat/copilot-target
branch
2 times, most recently
from
8b84efe to
ef12a48
Compare
momostallion
force-pushed
the
feat/copilot-target
branch
from
2b7a788 to
3c6fdd1
Compare
Contributor
Author
|
@SecKatie Hello! Just wondering if you had a chance to review this PR, I think I have made all the changes requested from the feedback. |
momostallion
force-pushed
the
feat/copilot-target
branch
from
3c6fdd1 to
659769a
Compare
Skills that include scripts, examples, or other resources alongside SKILL.md now have those files copied to the destination directory, matching the behavior of other file-based targets.
momostallion
force-pushed
the
feat/copilot-target
branch
from
659769a to
898a7a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add GitHub Copilot as a target assistant, split into two variants to match Copilot's two runtimes:
copilot-cli— GitHub Copilot CLIcopilot-vscode— Copilot in VS CodeBoth share the same project (
.github/) and user (~/.copilot/) files and differ in command and MCP handling at user scope.Layout:
.github/skills/<name>/SKILL.md(project) or~/.copilot/skills/<name>/SKILL.md(user), with requiredname+descriptionfrontmatter — both variants.agent.md(.github/agents/project,~/.copilot/agents/user) — both variants.prompt.mdcopilot-cli:.github/prompts/(project) and~/.copilot/prompts/(user)copilot-vscode:.github/prompts/(project only — VS Code has no user-scope prompts location)MCP handling:
copilot-cliwrites MCP servers with themcpServerskey (~/.copilot/mcp-config.jsonat user scope)copilot-vscodewrites to.vscode/mcp.jsonusing VS Code'sserverskeycopilot-vscodeat user scopecopilot-vscodeis preferred overcopilot-clito avoid writing the same project files twiceRelated Issues
Fixes #149
Test Plan
.github/idempotency case, and target metadatalola install rosa-skills -a copilot-vscode -s user— skills load in VS Code Copilot ChatChecklist
pytest) — 1003 passedruff check src tests+ruff format --check)ty check)AI Disclosure
AI-assisted with GitHub Copilot (Claude Opus 4.8)
Summary by CodeRabbit
New Features
Documentation
Tests