[code-simplifier] Simplify claude_tools.go: use getOrCreateToolMap and clearer isClaudeToolName#35855
Conversation
…deToolName - Replace manual get-or-create blocks in expandNeutralToolsToClaudeTools with the existing getOrCreateToolMap helper, removing 20 lines of duplicated logic - Simplify isClaudeToolName to a direct byte comparison (toolName[0] >= 'A' && toolName[0] <= 'Z') instead of the opaque strings.HasPrefix + ToUpper pattern; all Claude tool names are ASCII so the behavior is identical Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Only pkg/workflow/claude_tools.go was changed (5 additions, 29 deletions). Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35855 does not have the 'implementation' label (has_implementation_label=false) and has only 5 new lines of code in business logic directories (default_business_additions=5, threshold=100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — changes are correct and improve readability.
📋 Key Themes & Highlights
Positive Highlights
- ✅
getOrCreateToolMapcorrectly inserts newly-created maps into the parent (container[key] = created), so removing the now-redundantresult["claude"] = claudeSectionandclaudeSection["allowed"] = claudeAllowedlines is safe — the implicit contract is already verified by the existing helper test. - ✅ The
isClaudeToolNamerewrite (toolName[0] >= 'A' && toolName[0] <= 'Z') is clearer and equally correct for ASCII Claude tool names. - ✅ Net -24 lines with zero behaviour change — exactly the right kind of cleanup PR.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.1M
There was a problem hiding this comment.
Review: Simplification is correct; one implicit contract worth noting
The refactoring is functionally equivalent — getOrCreateToolMap does write the new map back into its container (container[key] = created), so removing the explicit result["claude"] = claudeSection / claudeSection["allowed"] = claudeAllowed lines is safe today.
⚠️ Implicit side-effect contract introduced
The deleted write-back lines were self-documenting: they made it obvious that claudeSection was intentionally placed into result, and claudeAllowed into claudeSection. With those lines gone, correctness is silently coupled to the side-effect behaviour of getOrCreateToolMap (the container[key] = created assignment in the create path).
If someone later changes getOrCreateToolMap to lazy-init (returns a detached map, writes back only on mutation), all Claude tool configuration would be silently dropped with no compile-time or test-time signal. A short comment calling this out would preserve the original intent:
// getOrCreateToolMap inserts newly-created maps into their parent container,
// so claudeSection and claudeAllowed are already wired into result on return.
claudeSection := getOrCreateToolMap(result, "claude")
claudeAllowed := getOrCreateToolMap(claudeSection, "allowed")The isClaudeToolName byte-index change is correct; the updated comment accurately documents the intentional ASCII-only narrowing.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.2M
Summary
Refactors
pkg/workflow/claude_tools.goto reduce boilerplate and tighten a character-classification helper.Changes
pkg/workflow/claude_tools.goexpandNeutralToolsToClaudeTools— replaced inline map-creation boilerplate with a newgetOrCreateToolMaphelper; removed redundant explicit writes back to the result map.isClaudeToolName— replaced thestrings.ToUppercall with a direct ASCII byte-range check, removing an unnecessary allocation and making the intent clearer.Motivation
The previous implementation scattered repeated map-initialisation logic across the function body and relied on
strings.ToUpperfor a simple ASCII range check. The refactor keeps the logic in one place (getOrCreateToolMap) and uses the idiomatic Go byte-range test (c >= 'A' && c <= 'Z') to avoid a string allocation on every call.Impact
pkg/workflow/claude_tools.goChecklist
isClaudeToolNamenow allocation-free for the common path