chore(cli): remove duplicated skill token helpers - #3267
Conversation
…into pi-tui-pickers This commit deletes the skill-token module and moves the skill invocation prefix logic into pi-tui-pickers. The skill invocation prefix function is now defined directly in pi-tui-pickers, streamlining the codebase. Additionally, the import statement for SKILL_INVOCATION_TOKEN_SOURCE has been updated in skill-highlight-editor to reflect the new structure.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this is a real duplicate, not a cosmetic one. Reviewed exact head 7fdac7fc19d8e75cd11c96184ac6c85dbd226e47.
I confirmed the deletion is safe: parseSkillInvocationTokens and stripSkillInvocationTokens already live in @maka/runtime/skill-invocation and are what root-turn-coordinator and interactive-turn-coordinator actually call on the submit path, so the CLI copy had no consumers, and nothing else in the tree still references skill-token. Moving the one surviving helper next to its only caller and importing the grammar from Core is the right direction.
One follow-up inline: the module you deleted existed to keep the grammar in one place, and the helper that survived it still hardcodes its own copy of that grammar. No findings above P3. No CI checks are currently reported for this head.
Review disclosure: this review was prepared with Claude Code, which read the diff at this head and traced the removed helpers' consumers across the workspace. The human contributor reviewed it before posting.
| ): { prefix: string; query: string } | null { | ||
| const currentLine = lines[cursorLine] || ''; | ||
| const beforeCursor = currentLine.slice(0, cursorCol); | ||
| const match = /(?:^|\s)(\/skill:([A-Za-z0-9._-]*))$/.exec(beforeCursor); |
There was a problem hiding this comment.
[P3] Derive this prefix pattern from the shared grammar instead of restating it. The deleted module's stated purpose was that SKILL_INVOCATION_TOKEN_SOURCE in Core is the single owner of where a /skill: token starts and ends, and the highlighter next door still imports it — but this literal re-declares the id charset [A-Za-z0-9._-], so widening the charset in Core would leave autocomplete quietly refusing to offer names the rest of the system accepts. The divergence is pre-existing, not introduced here, but a PR whose whole point is removing the duplicated copy is the right place to close it. Build the regex from SKILL_INVOCATION_TOKEN_SOURCE with the trailing quantifier relaxed so an empty query still triggers, or at minimum add a test asserting both accept the same name charset.
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at 7fdac7fc19d8e75cd11c96184ac6c85dbd226e47. My review of this head found nothing above P3, so nothing here should hold the merge — the inline note stands as a suggestion, to take or leave.
The review disclosure on my earlier comment applies: it was prepared with Claude Code, and I reviewed the diff and the findings myself before posting. This approval is my own judgment.
Summary
Refs #3218.
Removes the unused CLI copy of skill-token parsing helpers. The remaining autocomplete helper now lives with its only caller, while highlighting imports the shared token grammar from Core.
Verification
npx biome check packages/cli/src/pi-tui-pickers.ts packages/cli/src/skill-highlight-editor.tsnpm --workspace maka-agent run typechecknpm --workspace maka-agent test(315 passing)AI use
Tool(s) and scope: Codex assisted with code analysis, patch preparation, and local verification. I reviewed and submitted the change.