feat(checks): add deprecated-tools-field rule for legacy tools: migration - #44
Conversation
…tion Warn when a skill uses only the legacy tools: field without allowed-tools:. The allowed-tools: field is the spec-supported tool allowlist; tools: is kept for backwards compatibility but new skills should use allowed-tools:. The new rule fires after tool-fields-ambiguous is checked so the two rules never overlap: tool-fields-ambiguous fires when both fields are present, deprecated-tools-field fires when only the legacy tools: field is present. Also documents tools-duplicate and deprecated-tools-field in the README rules table (both were implemented but not listed there). Update the "clean skill" baseline test to use allowed-tools: so it reflects current best practice and does not trigger the new warning.
![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: 50b80a4c1f
ℹ️ 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".
| return [ | ||
| { | ||
| severity: "warn", | ||
| rule: "deprecated-tools-field", |
There was a problem hiding this comment.
When a skill uses only tools: and users run --format sarif, this emitted deprecated-tools-field rule is absent from src/sarif.ts's static RULES catalog, so the reporter falls back to a synthetic rule whose name/description are just the id and clean SARIF catalogs omit the rule entirely. Please add SARIF metadata/tests for the new diagnostic so code-scanning consumers get the same stable catalog as other built-in rules.
Useful? React with 👍 / 👎.
1 participant
Why
Skills that use the legacy
tools:field instead of the spec-supportedallowed-tools:field will continue to work today, but the README already documentstools:as the legacy field and recommendsallowed-tools:. Authors who only seetool-fields-ambiguous(fired when both fields are present) get no signal if they never addedallowed-tools:in the first place. The newdeprecated-tools-fieldrule closes that gap: it fires whentools:is the only tool field declared and guides the author to migrate.This PR also documents
tools-duplicateand the newdeprecated-tools-fieldin the README rules table; both were implemented but missing from the documented rule list.What
src/checks.ts: addcheckDeprecatedToolsFieldfunction; call it inrunChecksbetweencheckToolFieldsAmbiguousandcheckToolsDuplicateso the two sibling rules never overlap (ambiguous fires when both present, deprecated fires when only legacy is present).test/checks.test.ts: update the "clean skill produces no diagnostics" baseline to useallowed-tools:(best practice); add 7 tests covering the new rule.README.md: add rows fordeprecated-tools-fieldandtools-duplicateto the rules table.Tests
npm run lint- clean (TypeScript noEmit)npm run build- cleannpm test- 186 tests pass (179 existing + 7 new)tools:only, no-op onallowed-tools:only, no-op when both fields present, no-op when no tool field, severity is warn, message mentionsallowed-tools:, no-op on invalid frontmatter.Self-merge gate
src/index.tsnot modified)test/checks.test.ts, 7 new tests)Generated by Claude Code