Skip to content

lint: split space-separated bare names, drop the dead fixedNames - #130

Merged
NagyVikt merged 1 commit into
mainfrom
fix/skill-linter-review-followups
Aug 7, 2026
Merged

lint: split space-separated bare names, drop the dead fixedNames#130
NagyVikt merged 1 commit into
mainfrom
fix/skill-linter-review-followups

Conversation

@NagyVikt

@NagyVikt NagyVikt commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #128. Reviewing that PR turned up two defects in its own change; it merged before the fixes landed, so both are live on main.

HIGH — space-separated bare names regressed

The pre-#128 code split on /[,\s]+/. readAllowedTools split on commas only:

Input on main today expected
allowed-tools: nmap, curl Bash(nmap:*), Bash(curl:*) same
allowed-tools: nmap curl Bash(nmap curl:*) Bash(nmap:*), Bash(curl:*)

Confirmed by executing applyFixes on both shapes, not by reading.

splitToolNames restores the old behaviour but keeps a part containing ( whole, so Bash(git diff:*) is not torn in half — a case the original only survived by early-returning on any wrapped value, which also meant it fixed nothing in a mixed list. So this is strictly better than either version.

MEDIUM — fixedNames is dead code

The fix closure recomputes the same map from a fresh parse, so the outer binding is never read. bun run lint flags it:

! This variable fixedNames is unused.
  > 335 │   const fixedNames = parsed.names.map((n) =>

#128 passed CI only because unused variables are a warning, not an error. Removed. The wrap predicate had been written out three times, which is how the dead copy went unnoticed; it is now isWellFormedTool + wrapTool.

Blast radius on the corpus: none

Verification

Check Result
bun test 3091 pass / 1 skip / 0 fail
bun run typecheck clean
bun run lint clean; the warning #128 added is gone
skill-linter.test.ts 94 tests (12 from #128, +4 here)

🤖 Generated with Claude Code

Two defects the review of #128 turned up in that PR, after it had merged.

Space-separated bare names regressed. The pre-#128 code split on /[,\s]+/;
readAllowedTools split on commas only, so `allowed-tools: nmap curl` produced
the single bogus `Bash(nmap curl:*)` instead of two entries. splitToolNames
restores the old behaviour but leaves a part containing `(` whole, so
`Bash(git diff:*)` stays one tool rather than being torn in half -- something
the original only avoided by early-returning on any wrapped value, which also
meant it fixed nothing in a mixed list.

fixedNames was computed and never read: the fix closure recomputes the same
map from a fresh parse. `bun run lint` flags it, and #128 passed CI only
because unused variables are a warning. Removed, and the wrap predicate that
had been written out three times is now isWellFormedTool + wrapTool.

4 more regression tests: space-separated names, a wrapper containing a space,
and flow-sequence brackets.

Neither defect reached opencue/skills#21: no skill carries a bare multi-word
allowed-tools value, and on its own base the repaired fixer reproduces that
PR byte-for-byte.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant