Skip to content

chore(cli): remove duplicated skill token helpers - #3267

Merged
Astro-Han merged 1 commit into
apache:mainfrom
seanxuu:chore/remove-cli-skill-token-duplicate
Aug 20, 2026
Merged

chore(cli): remove duplicated skill token helpers#3267
Astro-Han merged 1 commit into
apache:mainfrom
seanxuu:chore/remove-cli-skill-token-duplicate

Conversation

@seanxuu

@seanxuu seanxuu commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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.ts
  • npm --workspace maka-agent run typecheck
  • npm --workspace maka-agent test (315 passing)

AI use

  • No generative tool made a substantive contribution

Tool(s) and scope: Codex assisted with code analysis, patch preparation, and local verification. I reviewed and submitted the change.

…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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 Astro-Han mentioned this pull request Aug 20, 2026
2 tasks

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit 9d4392c into apache:main Aug 20, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants