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".
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
|
@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".
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".
src/utils/pi-skills.ts
Outdated
| 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 |
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
f20ff61 to
f8ea622
Compare
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
f8ea622 to
08418ea
Compare
|
@tmchow I've done several sessions of improvements and bug squashing. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08418ea77f
ℹ️ 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/sync/pi.ts
Outdated
| const publishedSkills = new Set(results.filter((result) => result.status === "published").map((result) => result.sourceName)) | ||
| const publishedPrompts = new Set(results.filter((result) => result.status === "published").map((result) => result.sourceName)) |
There was a problem hiding this comment.
stabilizeSameRunQualifiedDependencies builds both publishedSkills and publishedPrompts from the same results array, but this helper is called separately for skills and prompts. That means cross-type dependencies are always treated as missing (for example, a prompt with Task claude-home:... will never see its depended-on skill as published in this pass), so valid artifacts get downgraded to retryable and omitted from managed nameMaps/artifacts even though files were written.
Useful? React with 👍 / 👎.
| await writeFileAtomicIfChanged({ | ||
| filePath: targetPath, | ||
| content: await fs.readFile(materializedSourcePath), | ||
| }) |
There was a problem hiding this comment.
Materialized file copies are written without passing through the source mode, and writeFileAtomicIfChanged defaults to 0644. As a result, executable files inside skills lose their execute bit after copy/materialization, which can break skills that run bundled scripts directly (e.g., ./scripts/tool.sh) after install/sync.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 096278d165
ℹ️ 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
| for (const match of text.matchAll(/\/skill:claude-home:([^\s)]+)/g)) { | ||
| if (match[1]) skills.add(match[1]) |
There was a problem hiding this comment.
The dependency scanner captures trailing punctuation as part of the referenced name (for example /skill:claude-home:bad-skill, becomes bad-skill,), but demoteBlockedSameRunDependencies does exact key lookups in activeSkillMap/activePromptMap. That means sibling-drop retries are skipped for common sentence punctuation, so an artifact can stay published with rewritten unqualified links to a prompt/skill that was actually removed in the same sync pass.
Useful? React with 👍 / 👎.
src/utils/pi-skills.ts
Outdated
| for (const match of text.matchAll(/Task\s+claude-home:([^\s(]+)\s*\(/g)) { | ||
| if (match[1]) skills.add(match[1]) |
There was a problem hiding this comment.
collectPiSameRunDependencies runs regexes over raw markdown without skipping fenced/indented code or quoted examples, unlike transformPiBodyContent which intentionally avoids rewriting those regions. As a result, example text like Task claude-home:... inside documentation snippets is treated as a real dependency, which can incorrectly demote otherwise valid artifacts to retryable and leave them unpublished when the referenced sibling is blocked.
Useful? React with 👍 / 👎.
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
096278d to
3fadd2b
Compare
dragosdm
force-pushed
the
fix/pi-skill-name-normalization
branch
from
3fadd2b to
fcc30d9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc30d985d
ℹ️ 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".
|
|
||
| function normalizePiTaskAgentName(value: string, nameMaps?: PiNameMaps, options?: PiTransformOptions): string { | ||
| const trimmed = value.trim() | ||
| if (options?.rejectUnresolvedFirstPartyQualifiedRefs && trimmed.startsWith("claude-home:")) { |
There was a problem hiding this comment.
normalizePiTaskAgentName only performs the unresolved-first-party guard for claude-home: prefixes, so when rejectUnresolvedFirstPartyQualifiedRefs is enabled (as in Pi install/sync), a Task compound-engineering:... ref with no mapping falls through to leaf normalization instead of failing. That silently rewrites to an unqualified local agent name and can dispatch the wrong skill (or a non-existent one) rather than being marked retryable/unsupported.
Useful? React with 👍 / 👎.
| if (previousArtifact) { | ||
| await removeSkillDirectoryIfExists(targetDir) | ||
| } |
There was a problem hiding this comment.
The install hook deletes targetDir whenever previousArtifact exists, but copySkillDirForPi invokes this hook for both replace and incremental modes. In incremental mode, the planned ops only include diffs, so removing the directory first can drop unchanged files from the skill tree and leave a partial/corrupted output after the update.
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: