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 1 commit 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
7ce0ea5 to
af1c67d
Compare
|
@tmchow, ready for another round of review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af1c67d75e
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
af1c67d to
c0b55e8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b55e8e74
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
c0b55e8 to
bcb1538
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcb15383e4
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
bcb1538 to
2268bd4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2268bd4814
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
2268bd4 to
5c9c3ea
Compare
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c9c3eabe5
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
5c9c3ea to
8e0cd6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e0cd6ec16
ℹ️ 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".
|
@tmchow I don't think the last codereview brought something to fix/implement (left a comment above). |
|
Heads up — #334 overlaps with part of this work but is complementary, not competing. What #334 does that's relevant here:
What this PR does that #334 doesn't:
Overlap to resolve when merging both:
The two PRs should merge fine with minor conflict resolution. Main thing to check: make sure the zero-arg Task regex fix ( |
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
8e0cd6e to
8cf1c90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff8c20ae3
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
5ff8c20 to
93c236b
Compare
tmchow
left a comment
tmchow
left a comment
There was a problem hiding this comment.
Found 1 issue:
appendCompatibilityNoteIfNeededis now applied to agent skill files, not just prompts
Previously, appendCompatibilityNoteIfNeeded was called only inside convertPrompt — agents went through transformContentForPi without it. The PR moves appendCompatibilityNoteIfNeeded inside transformPiBodyContent (line 85), which is now called for both prompts and agents. Any agent whose body mentions "mcp" will get the MCPorter compatibility note appended to its generated SKILL.md. This note contains user-facing guidance (mcporter_list, mcporter_call) that makes sense in human-invoked prompts but is inappropriate in machine-read agent skill files.
compound-engineering-plugin/src/utils/pi-skills.ts
Lines 84 to 86 in 93c236b
Called for agents here:
compound-engineering-plugin/src/converters/claude-to-pi.ts
Lines 103 to 105 in 93c236b
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
93c236b to
f8fd18e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8fd18ea9c
ℹ️ 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".
|
@tmchow let me know if there's anything else to address. |
|
@dragosdm rebase against main to resolve issues again. |
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
f8fd18e to
0bff8ea
Compare
|
@tmchow rebased |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bff8ea6d7
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
0bff8ea to
a5f936c
Compare
|
codex comment was valid and fixed with last commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f936c821
ℹ️ 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".
src/utils/pi-skills.ts
Outdated
| : `${prefix}Run ${PI_CE_SUBAGENT_TOOL} with agent="${normalizedAgent}".` | ||
| }) | ||
|
|
||
| result = result.replace(/\bRun subagent with agent="/g, `Run ${PI_CE_SUBAGENT_TOOL} with agent="`) |
There was a problem hiding this comment.
transformPiBodyContent() only renames the tool token in existing Run subagent with agent="..." lines, but it never normalizes/remaps the agent value through nameMaps. When agent/skill names are normalized or deduplicated for Pi (for example code_review -> code-review-2), these pre-existing subagent invocations are converted to ce_subagent while still pointing at the old name, which causes runtime "skill not found" failures for converted/synced content that already used explicit subagent syntax instead of Task ....
Useful? React with 👍 / 👎.
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
a5f936c to
b822162
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b822162c42
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
b822162 to
adc3af7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adc3af7961
ℹ️ 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".
src/utils/pi-skills.ts
Outdated
| const updated = formatFrontmatter( | ||
| { ...parsed.data, name: targetName }, | ||
| transformPiBodyContent(parsed.body, nameMaps), |
There was a problem hiding this comment.
rewriteSkillFileForPi now reserializes every parsed SKILL.md frontmatter via formatFrontmatter, but that formatter only supports scalars/arrays and stringifies plain objects (via String(value) in src/utils/frontmatter.ts) to "[object Object]". In any skill whose frontmatter includes nested maps (for example metadata: or structured config), install/sync to Pi will silently corrupt those fields, which can break downstream tooling that reads that metadata.
Useful? React with 👍 / 👎.
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
adc3af7 to
e753560
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7535609ea
ℹ️ 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".
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
e753560 to
d1c5082
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c5082add
ℹ️ 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".
- Normalize Pi skill names to lowercase-hyphen form (ce:plan → ce-plan) - Truncate normalized names to 60 chars to stay within Pi's 64-char limit - Rename CE compat tool from subagent to ce_subagent to avoid pi-subagents collision - Rewrite Task calls, slash commands, and tool refs in copied/synced Pi content - Support zero-argument Task calls (Task agent() with no args) - Resolve Task, /skill:, and slash refs against deduped name maps in converter and sync - Two-pass name allocation for skills, agents, and prompts to handle collisions - Sort skills, agents, and commands before dedup for deterministic allocation across runs - Use locale-independent codepoint sort for cross-platform stability - Pass prompt maps from sync orchestrator into skill sync for cross-reference resolution - Fall through /skill: refs to agent map since Pi uses a flat skills/ namespace - Thread name maps through writer path so copied skills use deduped names - Rewrite frontmatterless skill bodies during Pi copy - Fallback-rewrite malformed-frontmatter skill bodies (body only, preserving broken YAML) - Apply MCPorter compatibility note only to prompts, not agent skill files - Handle dangling symlinks during skill dir cleanup using lstat instead of access - Back up existing skill dirs before overwrite, retain most recent backup - Skip symlinks during backup cleanup to prevent unintended deletions - Strip doubled quotes from Task args and tighten subagent regex to avoid false positives - Use sentinel comment for MCPorter note idempotency - Write compat extension during skills-only sync - Guard parseFrontmatter calls against malformed YAML with warnings
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
d1c5082 to
f20ff61
Compare
|
@tmchow shall we do another round? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f20ff61069
ℹ️ 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".
| if (activeRealDirs.has(resolvedPath)) { | ||
| throw cyclicPiSkillSymlinkError(sourcePath) | ||
| } | ||
| await copyDirForPiMaterialization(resolvedPath, targetPath, activeRealDirs) |
There was a problem hiding this comment.
The new Pi materialization path follows symlinks via realpath and then recursively copies the resolved location, but it never verifies that the target stays inside the source skill directory. A skill containing a symlink like assets -> / (or any external path) will copy arbitrary host files into the Pi output, which is a security and performance regression from the prior behavior that ignored symlinks. Add a containment check (or keep symlinks as links) before recursing/copying resolved targets.
Useful? React with 👍 / 👎.
|
@dragosdm there’s so much thrashing going on this PR that here’s what I suggest you do aside from just resolving that one comment from the codex review above: do a thorough AI code review asking it to review all the classes of PR feedback that you got and all the bugs and make sure that it’s poking holes in all the surrounding areas because the same type of feel like he’s coming back up just in different ways |
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: