feat(fix): auto-fix tools-duplicate by removing duplicate tool entries - #47
Conversation
Extends the --fix mode to handle the tools-duplicate diagnostic. Deduplicates allowed-tools: (space- or comma-separated string) and the legacy tools: field (block sequence or unquoted string), preserving first-occurrence order in both cases. Quoted string values are left unchanged since text-level parsing is not safe for them. Five new tests cover: space-separated allowed-tools, block-sequence tools, skip when already clean, dry-run, and notes population.
![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: a3dee26665
ℹ️ 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".
| * YAML line. Quoted values are left unchanged (safe-parse is not feasible). | ||
| */ | ||
| function deduplicateToolsStringLine(block: string, field: string): string { | ||
| const re = new RegExp(`^([ \\t]*${field}[ \\t]*:[ \\t]*)([^\\[\\r\\n].*)$`, "m"); |
There was a problem hiding this comment.
When a valid passthrough extension object contains an indented allowed-tools: key before the real top-level key, this regex matches and rewrites the nested value instead. For example, with settings:\n allowed-tools: Read Read\nallowed-tools: Bash Bash, --fix modifies settings.allowed-tools, reports the diagnostic fixed, and leaves the diagnosed top-level duplicate untouched. Require zero indentation when locating frontmatter tool fields.
Useful? React with 👍 / 👎.
| // allowed-tools is always a string per schema; handle unquoted inline form. | ||
| newBlock = deduplicateToolsStringLine(newBlock, "allowed-tools"); | ||
| // tools (legacy) may be a block sequence or an unquoted string. | ||
| newBlock = deduplicateToolsBlockSequence(newBlock, "tools"); | ||
| newBlock = deduplicateToolsStringLine(newBlock, "tools"); |
There was a problem hiding this comment.
For an actual CLI run on tools:\n - Read\n - Bash\n - Read, runChecks emits deprecated-tools-field before tools-duplicate, so the earlier fix renames the key to allowed-tools: before this coordinator runs. These lines only process block sequences named tools, causing the newly supported duplicate fix to be skipped in precisely the legacy block-sequence case it claims to handle; deduplicate before the rename or also handle the renamed buffered form.
Useful? React with 👍 / 👎.
1 participant
Why
The
tools-duplicatediagnostic fires when an author lists the same tool more than once inallowed-tools:or the legacytools:field. There is one obvious safe correction: remove the duplicate entries while preserving the order of first occurrence. This PR wires that fix into--fixmode, consistent with the four existing auto-corrections (name-drift,tool-fields-ambiguous,name-whitespace,deprecated-tools-field).What
src/fix.ts: newtools-duplicatebranch inapplyFixes; three private helpers:rewriteDeduplicateTools: top-level coordinator, extracts the frontmatter block and calls the two specialised rewriters.deduplicateToolsStringLine: handlesallowed-tools:(always a string per schema) and the string form oftools:- splits by space/comma (paren-aware), deduplicates, rejoins with the original separator.deduplicateToolsBlockSequence: handles thetools:block-sequence form (- itemlines) by skipping duplicate items.applyFixesJSDoc to document the new fix.Tests
Five new cases in
test/fix.test.tsunderapplyFixes - tools-duplicate:allowed-tools:(space-separated string)tools:block sequenceAll 197 tests pass.
Self-merge gate
Generated by Claude Code