refactor: dedupe cross-package logic and remove dead code from audit#58
Merged
Conversation
- internal/models: LoadModelSettingsFromConfig (zero refs) - internal/prompts: PromptTemplate.ExpandWithArgs (zero refs) - internal/app: NewMessageStore (tests migrated to NewMessageStoreWithMessages) - internal/config: HasEnvVars (+ its test) - internal/core: ContextWithSudoPassword (test migrated to context.WithValue)
NewTreeManagerAdapter and InitTreeSession now spell their signatures with the public kit.TreeManager alias instead of internal/session.TreeManager, so go doc renders domain types rather than internal paths.
coreToolKinds + toolKindFor were duplicated verbatim in internal/extensions/wrapper.go and pkg/kit/events.go, risking silent divergence between extension events and SDK events. Single source of truth now lives in internal/extensions/toolkinds.go; pkg/kit re-exports the constants.
The 'is the active Anthropic credential a stored OAuth token' check was copy-pasted at 5 sites, all prefix-matching the magic string 'stored OAuth' produced in internal/auth. Now: - internal/auth: new CredentialSourceOAuth constant + IsAnthropicOAuth() - internal/ui: new UpdateUsageTrackerForModel(); CreateUsageTracker and SetupCLI share lookupTrackableModel (SetupCLI no longer re-inlines the tracker construction) - cmd/root.go + cmd/extension_context.go: verbatim-duplicated tracker refresh blocks replaced with ui.UpdateUsageTrackerForModel - pkg/kit isAnthropicOAuth delegates to auth.IsAnthropicOAuth - internal/models compares source against the constant
- ExtractModelFromPath mis-parsed model IDs containing '/' (e.g. 'openrouter/meta/llama' -> 'meta'); it now delegates to RemoveProviderFromModel and is deprecated alongside ExtractProviderFromPath (-> GetCurrentProvider) - parseFields delegated to prompts.ParseCommandArgs so extension argument parsing and builtin prompt-template parsing share one quote/escape grammar; ParseCommandArgs now also splits on tabs (superset of both previous tokenizers)
internal/skills and pkg/kit/template_bridge each had their own grammar:
skills rejected '{{ name }}' (whitespace) but allowed digit-first names;
the bridge was the opposite. A template behaved differently depending on
whether it was loaded as a skill prompt or via the extension API.
internal/skills is now the single engine using the superset grammar
(\{\{\s*(\w+)\s*\}\}); pkg/kit ParseTemplate/RenderTemplate are thin
adapters over it. Expand is now regex-based so whitespace placeholders
expand consistently; missing variables are still left as-is.
The model-selector handler (ModelSelectedMsg) and /model slash command duplicated the full switch sequence (thinking-level fallback, setModel, display-state update, preference persistence, ModelChange emit) and had already drifted in ordering. Both now call a single switchModel method. Display state is still updated directly (no prog.Send from Update).
cmd/extension_context.go and internal/acpserver/session.go each built a giant extensions.Context literal, duplicating ~15 delegation closures (GetContextStats, GetMessages, AppendEntry, options, SetModel core, Complete, SpawnSubagent, ...) that had to be kept in sync by hand. New data-access fields had to be wired in both places or ACP-mode extensions silently got nil function fields. extbridge.BaseContext now provides the headless half; both call sites overlay only their UI-specific closures. As a side effect ACP mode gains previously-missing APIs (state, tree navigation, skills, template parsing, model resolution) that were nil before. The interactive TUI keeps its exact SetModel/ReloadExtensions ordering via overrides.
ExecuteTool repeated the OAuth-error/re-auth/retry stanza verbatim twice (sync and task-augmented paths) and the marshal-and-wrap stanza four times. Both are now single helpers with identical error strings, so a fix to OAuth retry or error categorization applies everywhere at once.
handleShareCommand repeated the close/remove/print/return cleanup chain four times across its temp-file write error paths. File assembly now lives in buildShareFile with a single deferred cleanup on error.
…uting from runNormalMode runNormalMode opened with ~150 lines of policy logic (flag-combination validation, persisted model/thinking-level preference restoration, and two subtle --provider-url model-rewrite rules). These are now standalone functions (validateModeFlags, restorePersistedPreferences, applyProviderURLRouting) so the routing policy is independently readable and testable. Behaviour unchanged; ordering preserved.
|
Connected to Huly®: KIT-59 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR extracts a headless extension BaseContext, composes TUI and ACP overlays from it, centralizes Anthropic OAuth detection and tool-kind classification, unifies template/argument parsing, consolidates model usage-tracking, and removes several small exported helpers. ChangesExtension Context Refactoring and API Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/model.go (1)
4202-4207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle fallback thinking-level apply failures before persisting UI state.
At Line 4202,
m.thinkingLevelis updated and preference persistence is queued even ifm.setThinkingLevel(...)fails at Line 4204. That can leave UI/prefs claiming a level the runtime never accepted.Suggested fix
- m.thinkingLevel = string(fallback) - if m.setThinkingLevel != nil { - _ = m.setThinkingLevel(string(fallback)) - } - go func() { _ = prefs.SaveThinkingLevelPreference(string(fallback)) }() + nextLevel := string(fallback) + if m.setThinkingLevel != nil { + if err := m.setThinkingLevel(nextLevel); err != nil { + m.printSystemMessage(fmt.Sprintf("Failed to apply thinking fallback %q: %v", nextLevel, err)) + } else { + m.thinkingLevel = nextLevel + go func() { _ = prefs.SaveThinkingLevelPreference(nextLevel) }() + } + } else { + m.thinkingLevel = nextLevel + go func() { _ = prefs.SaveThinkingLevelPreference(nextLevel) }() + }🤖 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/ui/model.go` around lines 4202 - 4207, The code updates m.thinkingLevel and kicks off prefs.SaveThinkingLevelPreference even when m.setThinkingLevel may fail; instead call m.setThinkingLevel(string(fallback)) first, check its error return, and only on success update m.thinkingLevel and asynchronously persist via prefs.SaveThinkingLevelPreference; if m.setThinkingLevel returns an error, do not change m.thinkingLevel or write prefs (optionally surface the error to the UI). Ensure you reference and adjust the calls around m.setThinkingLevel, m.thinkingLevel, and prefs.SaveThinkingLevelPreference so persistence only occurs after a successful set.
🤖 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 `@internal/extbridge/context.go`:
- Around line 117-133: The exported TreeNode currently returns the internal
Children slice reference (in GetTreeNode calling kitInstance.GetTreeNode),
allowing extensions to mutate internal state; fix by making a deep copy of the
Children slice (and each child element as needed) before assigning to
extensions.TreeNode.Children so the returned node has its own backing slice;
apply the same deep-copy approach to the other similar exporter that returns
Children (lines 134-150) so neither GetTreeNode nor the other exporter exposes
internal slice backing.
In `@internal/tools/mcp.go`:
- Around line 739-747: marshalToolResult currently dereferences result.IsError
without checking for nil; add a nil guard at the start of marshalToolResult to
avoid panic by returning a clear error when the incoming *mcp.CallToolResult is
nil. Specifically, in marshalToolResult check if result == nil and return nil,
fmt.Errorf("nil *mcp.CallToolResult") (or similar), then proceed to json.Marshal
and build the MCPToolResult (setting Content and IsError) only after the nil
check.
In `@pkg/kit/events.go`:
- Around line 109-110: Update the exported godoc for the ToolKind* constants to
remove the internal package name "internal/extensions" and instead use a generic
description (e.g., "the canonical classification is defined in the internal
implementation; these constants re-export that classification so SDK and
extension events agree"). Edit the comment immediately above the ToolKind*
constants (referencing the ToolKind* symbol names) to avoid mentioning internal
package paths and keep the intent: that these constants mirror the
canonical/internal classification without naming the internal package.
In `@pkg/kit/template_bridge.go`:
- Around line 18-19: The godoc for the exported ParseTemplate function currently
references an internal package path ("internal/skills"); remove that internal
package reference and rephrase the comment to avoid internal paths (e.g.,
"shared with skill prompt templates" or "shared with the project's skill prompt
templates") so the comment remains informative without mentioning internal
packages—update the comment above ParseTemplate in template_bridge.go
accordingly.
---
Outside diff comments:
In `@internal/ui/model.go`:
- Around line 4202-4207: The code updates m.thinkingLevel and kicks off
prefs.SaveThinkingLevelPreference even when m.setThinkingLevel may fail; instead
call m.setThinkingLevel(string(fallback)) first, check its error return, and
only on success update m.thinkingLevel and asynchronously persist via
prefs.SaveThinkingLevelPreference; if m.setThinkingLevel returns an error, do
not change m.thinkingLevel or write prefs (optionally surface the error to the
UI). Ensure you reference and adjust the calls around m.setThinkingLevel,
m.thinkingLevel, and prefs.SaveThinkingLevelPreference so persistence only
occurs after a successful set.
🪄 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: 15dce816-9f39-4a0b-900f-19d8332dfd89
📒 Files selected for processing (25)
cmd/extension_context.gocmd/root.gointernal/acpserver/session.gointernal/app/messages.gointernal/app/messages_test.gointernal/auth/credentials.gointernal/config/substitution.gointernal/config/substitution_test.gointernal/core/bash.gointernal/core/bash_test.gointernal/extbridge/context.gointernal/extensions/toolkinds.gointernal/extensions/wrapper.gointernal/models/custom.gointernal/models/providers.gointernal/prompts/template.gointernal/skills/templates.gointernal/tools/mcp.gointernal/ui/factory.gointernal/ui/model.gopkg/kit/adapter.gopkg/kit/events.gopkg/kit/extensions_bridge.gopkg/kit/kit.gopkg/kit/template_bridge.go
💤 Files with no reviewable changes (5)
- internal/config/substitution_test.go
- internal/core/bash.go
- internal/config/substitution.go
- internal/app/messages.go
- internal/models/custom.go
- pkg/kit: remove internal package paths from exported godoc on ParseTemplate and the ToolKind* constants (SDK doc surface must not reference internal packages) - internal/tools: guard marshalToolResult against a nil CallToolResult (json.Marshal(nil) succeeds as 'null', then result.IsError panics if a client returns nil result with nil error) Skipped the TreeNode Children deep-copy suggestion: the slice already comes from TreeManager.GetChildren which returns a fresh copy per call into a throwaway intermediate, so no internal state is exposed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the actionable findings from a full read-only code audit of the repository: dead-code removal, consolidation of seven verified duplication clusters, and low-risk refactors of the largest functions. Net result is −109 lines (892 insertions / 1,001 deletions) while adding shared infrastructure that removes copy-paste drift between
cmd/,internal/acpserver/,internal/ui/, andpkg/kit/.Two of the consolidations fix real bugs:
pkg/kit.ExtractModelFromPathmis-parsed model IDs containing/(e.g.openrouter/meta/llama→"meta"). It now delegates toRemoveProviderFromModeland is deprecated alongsideExtractProviderFromPathper the SDK deprecation policy.extbridge.BaseContextprovides the headless half ofextensions.Context;cmd/extension_context.goandinternal/acpserver/session.gonow overlay only their UI-specific closures instead of maintaining two ~150-line literals by hand.Other highlights, grounded in the commits:
LoadModelSettingsFromConfig,PromptTemplate.ExpandWithArgs,NewMessageStore,HasEnvVars,ContextWithSudoPassword); verified against SDK reachability and the Yaegi symbol table before deletion.coreToolKinds/toolKindForexisted verbatim in bothinternal/extensions/wrapper.goandpkg/kit/events.go, risking divergentToolKindvalues between extension events and SDK events. Single source now lives ininternal/extensions/toolkinds.go;pkg/kitre-exports the constants."stored OAuth"magic-string prefix match was copy-pasted at 5 sites. Now oneauth.CredentialSourceOAuthconstant +auth.IsAnthropicOAuth(), and a sharedui.UpdateUsageTrackerForModel()replaces two verbatim tracker-refresh blocks incmd/.{{variable}}grammars (whitespace tolerance vs first-char rules) — the same template rendered differently depending on entry point. One engine ininternal/skillsnow uses the superset grammar;pkg/kitParseTemplate/RenderTemplate are thin adapters. Same treatment for the quote/escape argument tokenizer (parseFields→prompts.ParseCommandArgs).switchModel(model selector +/modelcommand shared one drifted sequence),withOAuthRetry/marshalToolResultin MCP tool execution (2× retry stanza, 4× marshal stanza),buildShareFilewith defer-based cleanup (4× cleanup chain), andvalidateModeFlags/restorePersistedPreferences/applyProviderURLRoutingout of the 663-linerunNormalMode.All changes respect the documented invariants in AGENTS.md: no
prog.Send()fromUpdate(), closures crossing the Yaegi boundary remain anonymous literals, no interfaces in extension-facing types, and no dependency-name leakage into SDK exports.Type of Change
How Has This Been Tested?
go test -race ./...(all packages, green) plusgo vetandstaticcheck(clean)--provider-urlmodel rewriting (custom · gemma-test/custom · custom),/modeldirect switch + selector-overlay switch withOnModelChangeevents, OAuth cost suppression ($0.00 across 6.6K tokens with tracker refreshes),OnToolCallkind classification (bash→execute,read→read),/sharehappy path against a fakeghshim (system-prompt entry injected after header) and in-memory early-return pathinitialize+session/new): previously-nil extension APIs now work —state=OK template=OK tokenizer=OK treenav=OK skills=OK stats=OK{{name}}and{{ name }}), tokenizer (quotes, backslash escapes, tab splitting), andBaseContextdelegation from inside the interpreterChecklist
go fmt,go vet, staticcheck clean)Additional Information
Added
internal/extbridge/context.go— shared headlessextensions.Contextbuilder (BaseContext)internal/extensions/toolkinds.go— canonicalToolKind*constants +ToolKindForBackward compatibility
ExtractProviderFromPath/ExtractModelFromPathkept as deprecated wrappers;NewTreeManagerAdapter/InitTreeSessionsignatures now spell the existingkit.TreeManageralias (same type, cleanergo doc)prompts.ParseCommandArgsnow also splits on tabs; the template grammar now accepts whitespace inside{{ }}everywhere (previously accepted in only one of the two engines)internal/symbols had zero references outside their own testsKnown pre-existing issue surfaced during testing (not addressed here)
kit acp -e <file>does not propagate the-eflag into ACP's isolated per-session config store; only global/project-local extensions load in ACP mode. Worth a follow-up issue.Summary by CodeRabbit