refactor(skills): remove unused embedded asset helpers#100
Conversation
The embedded asset source directory and agent-name helpers no longer add value now that callers use the bundled registry data directly. Removing them keeps the skills asset API smaller and leaves tests focused on the remaining public behavior. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis change removes two exported utility functions ( Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/application/commands/skills/embedded-assets.test.ts (1)
72-80: Usenode:pathhelpers instead of hardcoded/in test path construction.Line 72 currently assumes POSIX separators in the constructed path. Prefer
join()and then normalize for the assertion.Suggested diff
+import { join } from "node:path"; import { describe, expect, test } from "bun:test"; @@ - const sourceDirectory = `contrib/skills/${agentName}/${skillName}`; + const sourceDirectory = normalizePathForAssertion( + join("contrib", "skills", agentName, skillName), + ); const skillFiles = getBundledSkillFiles(skillName, agentName); @@ - `/${sourceDirectory}/`, + `/${sourceDirectory}/`,As per coding guidelines, "Never assume POSIX path separators in code, tests, snapshots, or assertions; use
node:pathhelpers (join(),resolve(),relative()) for path construction".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/embedded-assets.test.ts` around lines 72 - 80, The test assumes POSIX separators by checking normalizePathForAssertion(file.sourcePath).includes(`/${sourceDirectory}/`); change this to build the expected segment using node:path helpers: import join from 'node:path' (or path.join) and compute expectedSegment = normalizePathForAssertion(join(sourceDirectory, '')); then assert includes(expectedSegment) so the check uses OS-correct separators; update the assertion that follows getBundledSkillFiles and reference variables sourceDirectory, agentName, skillName and the helper normalizePathForAssertion to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/application/commands/skills/embedded-assets.test.ts`:
- Around line 72-80: The test assumes POSIX separators by checking
normalizePathForAssertion(file.sourcePath).includes(`/${sourceDirectory}/`);
change this to build the expected segment using node:path helpers: import join
from 'node:path' (or path.join) and compute expectedSegment =
normalizePathForAssertion(join(sourceDirectory, '')); then assert
includes(expectedSegment) so the check uses OS-correct separators; update the
assertion that follows getBundledSkillFiles and reference variables
sourceDirectory, agentName, skillName and the helper normalizePathForAssertion
to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1123dee9-e6d6-405d-9341-c1653971f164
📒 Files selected for processing (2)
src/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/embedded-assets.ts
💤 Files with no reviewable changes (1)
- src/application/commands/skills/embedded-assets.ts
The embedded asset source directory and agent-name helpers no longer
add value now that callers use the bundled registry data directly.
Removing them keeps the skills asset API smaller and leaves tests
focused on the remaining public behavior.