fix(skill-sync): relax adopt name check#1875
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (58)
💤 Files with no reviewable changes (53)
✅ Files skipped from review due to trivial changes (5)
📝 WalkthroughWalkthroughThis PR removes the frontmatter name-mismatch validation during agent skill adoption preview, deriving conflict detection and target naming from the agent-facing skill entry name instead. It also revises SDD workflow documentation and skill definitions to separate feature, issue, architecture, and cleanup flows, and prunes numerous completed plan/task documents. ChangesSkill Name Mismatch Adoption
SDD Workflow Simplification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/main/presenter/skillSyncPresenter/index.test.ts (1)
1050-1092: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood regression test, but consider adding a negative-path case.
This covers the happy path (mismatched but still slug-valid
SKILL.mdname, no conflict). Given the concern raised inindex.tsaboutreadAdoptableSkillstill enforcingassertValidDeepChatSkillNameon the unused parsed name, a companion test where the frontmatternameis not a valid slug (e.g., contains spaces) would help confirm whether adoption should succeed in that case too.🤖 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 `@test/main/presenter/skillSyncPresenter/index.test.ts` around lines 1050 - 1092, Add a negative-path test for previewAdoptAgentSkill in skillSyncPresenter/index.test.ts to cover a SKILL.md frontmatter name that is not a valid slug (for example, contains spaces). Reuse the existing adoption preview setup around previewAdoptAgentSkill, toolScanner.scanTool, and fs.promises.readFile, but change the parsed name to an invalid one and assert the adoption preview still succeeds or fails as intended. This will verify the behavior of readAdoptableSkill and its assertValidDeepChatSkillName handling for the unused parsed name.
🤖 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/main/presenter/skillSyncPresenter/index.ts`:
- Line 822: The adoption loading path still blocks nonconforming source names,
causing `readAdoptableSkill()` to fail before rename handling. Update
`readAdoptableSkill()` in `src/main/presenter/skillSyncPresenter/index.ts` so it
only validates `description` and no longer rejects based on the parsed `name`;
treat `adoption.skill.name` as the source of truth and move any target-name
validation to the rename step in the adoption flow.
---
Nitpick comments:
In `@test/main/presenter/skillSyncPresenter/index.test.ts`:
- Around line 1050-1092: Add a negative-path test for previewAdoptAgentSkill in
skillSyncPresenter/index.test.ts to cover a SKILL.md frontmatter name that is
not a valid slug (for example, contains spaces). Reuse the existing adoption
preview setup around previewAdoptAgentSkill, toolScanner.scanTool, and
fs.promises.readFile, but change the parsed name to an invalid one and assert
the adoption preview still succeeds or fails as intended. This will verify the
behavior of readAdoptableSkill and its assertValidDeepChatSkillName handling for
the unused parsed name.
🪄 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: d4f49887-7b00-4064-a033-c0652f9d0125
📒 Files selected for processing (3)
docs/issues/skill-adopt-name-mismatch/spec.mdsrc/main/presenter/skillSyncPresenter/index.tstest/main/presenter/skillSyncPresenter/index.test.ts
Collaborator
Author
|
Handled review feedback:
Validation:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/skills/deepchat-sdd-cleanup/agents/openai.yaml:
- Line 4: The default_prompt for the deepchat-sdd-cleanup skill is too generic
and sounds like a routine post-implementation action; tighten it so the
manual-only gate is explicit. Update the wording in the openai.yaml entry for
default_prompt under the deepchat-sdd-cleanup skill to clearly state it should
only be used when the developer explicitly asks to clean, prune, or organize SDD
docs, keeping the intent aligned with the cleanup skill’s purpose.
🪄 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: 8b380a9c-313a-4ab9-8bc3-9335a35edf14
📒 Files selected for processing (13)
.agents/skills/deepchat-sdd-cleanup/SKILL.md.agents/skills/deepchat-sdd-cleanup/agents/openai.yaml.agents/skills/deepchat-sdd/SKILL.md.agents/skills/deepchat-sdd/agents/openai.yamlAGENTS.mddocs/README.mddocs/architecture/sdd-workflow-simplification/plan.mddocs/architecture/sdd-workflow-simplification/spec.mddocs/architecture/sdd-workflow-simplification/tasks.mddocs/issues/skill-adopt-name-mismatch/spec.mddocs/spec-driven-dev.mdsrc/main/presenter/skillSyncPresenter/index.tstest/main/presenter/skillSyncPresenter/index.test.ts
💤 Files with no reviewable changes (1)
- src/main/presenter/skillSyncPresenter/index.ts
✅ Files skipped from review due to trivial changes (7)
- docs/architecture/sdd-workflow-simplification/tasks.md
- .agents/skills/deepchat-sdd/agents/openai.yaml
- docs/architecture/sdd-workflow-simplification/plan.md
- .agents/skills/deepchat-sdd-cleanup/SKILL.md
- docs/README.md
- docs/issues/skill-adopt-name-mismatch/spec.md
- docs/spec-driven-dev.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/skillSyncPresenter/index.test.ts
Collaborator
Author
|
Handled the latest review feedback:
Validation:
|
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
1 participant
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
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Documentation