Conversation
|
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e2a54f7024
ℹ️ 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".
| skillDirName: skillName, | ||
| root, | ||
| }); | ||
| if (!rootIsBundle) { |
There was a problem hiding this comment.
Workspace/session catalogs are built with RuntimeSkillDiscovery in program.ts, not this FileSkillDiscovery, and its top-level loop at runtimeSkillDiscovery.ts:142-158 still registers every sibling .md file after loading the root SKILL.md. Consequently, the reported plugin layout continues to expose GLOSSARY-FORMAT as a phantom skill during normal session use; mirror this guard in the runtime scanner (and cover that production path) as well.
Useful? React with 👍 / 👎.
A plugin root that is itself a bundle (it has its own SKILL.md) still ran the flat top-level .md loop, so payload documents like GLOSSARY-FORMAT.md were registered as standalone skills alongside the bundle. Skip that loop when the root dir has its own SKILL.md; loose top-level .md files in a root without one keep registering, matching the existing pinned behavior. Fixes MoonshotAI#3740
e2a54f7 to
ea36f09
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Rebased: the branch was accidentally stacked on the unrelated #2581 head, which made the diff show 100 files. Now it is the single skill commit on current main (2 files, as intended). Content unchanged; |
commit: |
1 participant
What this PR does
When a plugin root is itself a bundle (its directory has its own
SKILL.md), skill discovery no longer runs the flat top-level.mdloop in that directory, so payload documents beside the bundle (the mattpocock-styleGLOSSARY-FORMAT.mdcase from the legacy v1 scanner) stop being registered as standalone skills. Loose top-level.mdfiles in a root that has noSKILL.mdkeep registering exactly as before, which the existing test for that shape still pins.Why it's needed
Filed as #3740 with a reproduction on current main: discovering a root laid out as
teach/SKILL.mdplusteach/GLOSSARY-FORMAT.mdreturns bothteachand a phantomGLOSSARY-FORMATskill, where the v1 issue's expectation is just the bundle. The phantom entries pollute the skill catalog and can shadow real skills on name collision.How to verify
Run the catalog suite:
pnpm vitest run test/features/skill/catalog/. The new regression case lays out a plugin bundle root with a payload document and asserts only the bundle is discovered; the previously pinned flat-.mdcase (root withoutSKILL.md) still passes, as do all 240 tests across the skill and plugin suites.Tested on
macOS, Node 24 (pnpm 10.33.0).
Linked Issues
Fixes #3740