Set subagent tool name in a single place#56
Conversation
This helps to keep the names in sync between packages and improves commit ef072f6. Signed-off-by: Egbert Eich <[email protected]>
|
Connected to Huly®: KIT-57 |
📝 WalkthroughWalkthroughThis PR introduces a shared constant ChangesSubagent Tool Name Constant
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/core/subagent.go (1)
13-13: 💤 Low valueConsider adding a godoc comment for the exported constant.
Since
SubagentToolNameis exported and used across packages, a brief comment would improve discoverability and clarify its purpose.📝 Suggested documentation
+// SubagentToolName is the canonical identifier for the subagent tool used +// to spawn in-process subagents. const SubagentToolName = "subagent"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/subagent.go` at line 13, Add a one-line GoDoc comment immediately above the exported constant SubagentToolName describing what the constant represents and how/where it is used (e.g., "SubagentToolName is the canonical tool name used by subagent components for identification in logs/registrations."). Ensure the comment begins with "SubagentToolName" so it is picked up by godoc and keep it concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/kit/kit.go`:
- Line 146: Replace all hardcoded "subagent" string literals with the canonical
constant core.SubagentToolName: update the map key in events.go (`"subagent":
ToolKindSubagent`), all comparisons in extensions_bridge.go (e.g., `ev.ToolName
!= "subagent"`), the switch/case in internal/ui/tool_renderers.go (`case
toolName == "subagent"`), and the mapping entry in
internal/extensions/wrapper.go (`"subagent": "agent"`). Make sure each file
imports the package that exposes SubagentToolName (core) and use
core.SubagentToolName everywhere instead of the literal.
---
Nitpick comments:
In `@internal/core/subagent.go`:
- Line 13: Add a one-line GoDoc comment immediately above the exported constant
SubagentToolName describing what the constant represents and how/where it is
used (e.g., "SubagentToolName is the canonical tool name used by subagent
components for identification in logs/registrations."). Ensure the comment
begins with "SubagentToolName" so it is picked up by godoc and keep it concise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 221a2597-b44b-4254-a22a-31ed037fa08b
📒 Files selected for processing (2)
internal/core/subagent.gopkg/kit/kit.go
| var tools []Tool | ||
| for _, t := range m.agent.GetTools() { | ||
| if t.Info().Name == "subagent" { | ||
| if t.Info().Name == core.SubagentToolName { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for hardcoded "subagent" string literals in Go files.
# Exclude the constant definition itself and test files.
rg -n --type=go '"subagent"' \
-g '!*_test.go' \
-C2Repository: mark3labs/kit
Length of output: 3280
Replace remaining hardcoded "subagent" string literals with core.SubagentToolName
Hardcoded "subagent" literals still exist in:
pkg/kit/events.go:"subagent": ToolKindSubagentpkg/kit/extensions_bridge.go:ev.ToolName != "subagent"(multiple checks)internal/ui/tool_renderers.go:case toolName == "subagent"internal/extensions/wrapper.go:"subagent": "agent"
internal/core/subagent.go is the expected constant definition; examples/extensions/kit-telegram/main.go and quoted occurrences in comments can be handled separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/kit/kit.go` at line 146, Replace all hardcoded "subagent" string literals
with the canonical constant core.SubagentToolName: update the map key in
events.go (`"subagent": ToolKindSubagent`), all comparisons in
extensions_bridge.go (e.g., `ev.ToolName != "subagent"`), the switch/case in
internal/ui/tool_renderers.go (`case toolName == "subagent"`), and the mapping
entry in internal/extensions/wrapper.go (`"subagent": "agent"`). Make sure each
file imports the package that exposes SubagentToolName (core) and use
core.SubagentToolName everywhere instead of the literal.
There was a problem hiding this comment.
Good catch! There are actually many more instances of hard-coded agent names in the source code - not only for "subagent". It does not make much sense to replace this one instance and leave all other cases untouched.
Unaware of the fact that there were many more such cases already, the original goal of this patch was to address the introduction of a hard-coded tool name in a package different from the one that creates this tool. This, however, is moot - unless fixed for all instances and all tool names throughout the code.
What would be the advantage of doing this?
- It would be easy to change the tool name without accidentally breaking things.
- Reduction in size of the resulting binary.
Judging from the present code, there is probably little intention to change core tool names and the size savings are likely insignificant if any.
So rather than attempting to address all instances of all core tool names, I'm inclined to drop this change.
There was a problem hiding this comment.
@e4t, your reasoning is sound. A partial fix that replaces one out of many hardcoded tool name literals doesn't improve maintainability in a meaningful way — it just creates inconsistency. If the goal were to make renaming safe, all occurrences across all packages would need to be covered under a single, consistent approach (e.g., a shared internal/core/toolnames.go exporting constants for every core tool). Given the apparent stability of these names and the modest practical benefit, dropping the change and keeping the codebase consistent as-is is a reasonable call.
If you'd like to track the broader refactor as a future task, I can open a GitHub issue for it. Just let me know.
🐇 (fair trade-off — consistency beats a half-measure)
🧠 Learnings used
Learnt from: ezynda3
Repo: mark3labs/kit PR: 42
File: pkg/kit/viper_isolation_test.go:192-205
Timestamp: 2026-06-02T11:40:48.379Z
Learning: In pkg/kit (Go, using spf13/viper), viper environment binding (e.g., viper.AutomaticEnv, SetEnvPrefix, SetEnvKeyReplacer) should only be set inside initConfig, and New() should only call initConfig when config is not skipped (e.g., !opts.SkipConfig && opts.CLI == nil). If Options.SkipConfig is true, the constructed per-instance viper store must be isolated so KIT_* env vars cannot influence it. When changing config initialization, preserve this guard behavior or update/extend viper_isolation_test.go accordingly.
|
Closing submission as it introduces more inconsistency than it fixes. Fixing this in a consistent way probably doesn't provide much benefit but may create more confusion. |
1 participant
This helps to keep the names in sync between packages and improves commit ef072f6.
Summary by CodeRabbit