fix: normalize skill names and avoid subagent tool conflicts in pi#288
fix: normalize skill names and avoid subagent tool conflicts in pi#288dragosdm wants to merge 2 commits intoEveryInc:mainfrom
Conversation
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
993150a to
5570e1f
Compare
|
@kieranklaassen this is interesting suggestion because it also addresses cross platform compat with directory and skill names issues with colons not working with windows for example if we ever want folders to match skill names directly. Using a hyphen is likely a better way to go so we don't overload the usage of what Long term probably makes sense for us to to rename the skills but not worth blocking this now so Pi can get unblocked with the fix |
tmchow
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves the Pi target compatibility layer for Compound Engineering by ensuring Pi-safe skill/prompt naming and avoiding tool-name collisions with other Pi extensions (notably pi-subagents).
Changes:
- Normalize Pi skill/prompt/agent names into lowercase hyphenated form, with collision-safe suffixing.
- Materialize Pi skills (copy + rewrite) when name/body/frontmatter rewrites are required; otherwise keep symlink parity.
- Rename the Pi compat tool from
subagenttoce_subagentand rewrite Task-style delegation accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/sync-pi.test.ts | Adds coverage for Pi materialization behavior (invalid names, body rewrites, symlink replacement). |
| tests/pi-writer.test.ts | Verifies copied skill frontmatter name is rewritten to match Pi-safe directory and body rewrites use ce_subagent. |
| tests/pi-converter.test.ts | Updates expectations for normalized names and ce_subagent; adds collision normalization test. |
| src/utils/pi-skills.ts | New utilities for Pi name normalization, body rewrites, skill copying, and frontmatter rewriting. |
| src/templates/pi/compat-extension.ts | Renames registered Pi tool to ce_subagent and updates metadata. |
| src/targets/pi.ts | Writes Pi bundles using Pi-aware skill copying/rewriting and updates compatibility notes. |
| src/sync/pi.ts | Switches Pi sync to use a Pi-specific skills sync path. |
| src/sync/pi-skills.ts | New Pi-specific skills sync: normalize names, decide symlink vs materialize, handle collisions. |
| src/converters/claude-to-pi.ts | Uses shared Pi normalization/rewrites, normalizes copied skill names, and avoids name collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
@tmchow, how would you like to proceed: accept copilot suggestions or should I fix them? |
I don’t trust copilot’s solutions but trust it’s diagnosis 😆 do you mind looking looking at the flagged problems and implementing solutions? |
|
@tmchow I've pushed the commit with the fixes:
|
tmchow
left a comment
tmchow
left a comment
There was a problem hiding this comment.
Good work overall — the core logic is correct, fixes real pre-existing bugs (skill names reserved but not normalized in output, agent bodies never transformed for Pi), and has solid test coverage.
A few things to clean up before merging:
1. Dead export: isValidPiSkillName is never imported
src/utils/pi-skills.ts exports isValidPiSkillName but nothing imports it — not the sync path (which uses isValidSkillName from utils/symlink.ts), not the converter, not tests. Either remove it or wire it up where it belongs.
2. Backup directory accumulation
copySkillDirForPi creates timestamped .bak directories on every materialization but never cleans up old ones. Repeated installs/syncs will accumulate stale backups in the user's Pi skills directory indefinitely. Either clean up previous .bak dirs before creating a new one, or cap the number retained.
3. Run subagent with catch-all is too broad
result = result.replace(/\bRun subagent with\b/g, `Run ${PI_CE_SUBAGENT_TOOL} with`)This could false-positive on prose that coincidentally contains "Run subagent with". Tighten the pattern to match the actual generated format, e.g. Run subagent with agent= instead of just Run subagent with.
Also made two small improvements:
Squashed everything into a single commit. @tmchow back to you :) |
|
@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: 7ce0ea530b
ℹ️ 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".
| result = result.replace(taskPattern, (_match, prefix: string, agentName: string, args: string) => { | ||
| const normalizedAgent = normalizePiTaskAgentName(agentName) | ||
| const trimmedArgs = args.trim().replace(/\s+/g, " ") | ||
| return `${prefix}Run ${PI_CE_SUBAGENT_TOOL} with agent="${normalizedAgent}" and task="${trimmedArgs}".` |
There was a problem hiding this comment.
This replacement wraps args in a new task="..." string without handling the very common Task agent("...") form. When Pi rewrites copied skills, existing CE workflows such as plugins/compound-engineering/skills/ce-review/SKILL.md:522/:541 and plugins/compound-engineering/skills/test-xcode/SKILL.md:331 become task=""Run /test-browser ..."", so the delegated prompt is emitted with doubled quotes instead of the original task text after install --to pi or sync --target pi.
Useful? React with 👍 / 👎.
| return `${prefix}Run ${PI_CE_SUBAGENT_TOOL} with agent="${normalizedAgent}" and task="${trimmedArgs}".` | ||
| }) | ||
|
|
||
| result = result.replace(/\bRun subagent with\b/g, `Run ${PI_CE_SUBAGENT_TOOL} with`) |
There was a problem hiding this comment.
transformPiBodyContent() now runs over every copied/synced skill, not just CE-generated content, so this unconditional rename silently retargets any user-authored Pi skill that intentionally says Run subagent with ... to use the CE-only ce_subagent wrapper instead. In sync --target pi setups that also install pi-subagents, those skills will start calling the wrong tool and fail unless the referenced agent happens to exist as a CE Pi skill too.
Useful? React with 👍 / 👎.
| function normalizePiTaskAgentName(value: string): string { | ||
| const leafName = value.split(":").filter(Boolean).pop() ?? value | ||
| return normalizePiSkillName(leafName) |
There was a problem hiding this comment.
AGENTS.md explicitly requires fully-qualified agent names to avoid collisions with other installed plugins, but this helper drops everything before the last :. That means Task compound-engineering:review:security-sentinel(...) and Task other-plugin:review:security-sentinel(...) are both rewritten to agent="security-sentinel" during Pi install/sync, so a multi-plugin Pi environment can now dispatch the copied skill to the wrong agent.
Useful? React with 👍 / 👎.
Summary
Fix Pi compatibility for Compound Engineering by:
pi-subagentsby using a dedicated CE tool nameWhat changed
Pi skill-name normalization
SKILL.mdfrontmatter soname:matches the normalized target directoryExamples:
ce:plan->ce-plangenerate_command->generate-commandresolve_parallel->resolve-parallelPi subagent conflict avoidance
subagenttoce_subagentce_subagentsubagentavailable forpi-subagentsThis allows CE Pi installs to coexist with
pi-subagentsin the same runtime without tool-name conflicts.Implementation notes
Validation
Automated
Ran the full test suite locally
Result:
Manual smoke tests
Verified locally that:
Why this matters
Before this change, Pi installs/syncs could produce invalid skill names such as:
and CE’s Pi compatibility extension conflicted with pi-subagents because both registered subagent.
After this change: