refactor: remove dead code and consolidate duplicated extension wiring#24
Merged
Conversation
Removes ~600 lines of unreferenced code surfaced by deadcode + manual
audit (none of it reachable from production code paths or test setup):
- internal/models/pool.go: ProviderPool was never wired into kitsetup
or the agent; the global pool singleton had zero callers.
- internal/ui/debug_logger.go: CLIDebugLogger was unreachable; debug
routing goes through internal/tools/buffered_logger.go instead.
- internal/ui/tool_approval_input.go: tea.Model never instantiated;
approvals are handled inline in model.go.
- internal/ui/cli.go: DisplayAssistantMessage / DisplayCancellation /
GetDebugLogger had zero callers (the *WithModel variant is what
event_handler.go uses).
- internal/ui/style/enhanced.go: Style{Card,Header,Subheader,Muted,
Success,Error,Warning,Info} + Create{Separator,ProgressBar} — none
used. CreateBadge stays (used by model.go).
- internal/ui/style/themes.go: RefreshThemeRegistry — never called.
- internal/ui/block_renderer.go: With{FullWidth,MarginTop,Padding{Left,
Right},Background,Foreground,Width} — option helpers nobody calls.
- internal/ui/render/blocks.go: UserBlock, ToolBlock — replaced by
inline rendering elsewhere; the test for UserBlock was rewritten to
directly exercise HighlightFileTokens (which is what the test really
cared about).
- internal/ui/commands/commands.go: GetAllCommandNames — no callers.
- internal/ui/message_items.go: NewTextMessageItem,
NewSystemMessageItem + the entire SystemMessageItem type — model.go
uses NewStyledMessageItem instead.
- internal/prompts/loader.go: Deduplicate — the loader does dedup
internally; standalone helper was unused.
- internal/models/cache_options.go: mergeProviderOptions + its
test-only consumer.
- internal/extensions/installer.go: Installer.GetInstalledPackages —
intended for a 'kit ext list' command that was never built.
- internal/extensions/manifest.go: saveManifestToScope,
saveManifestToPath, GetGlobalManifest, GetProjectManifest,
addEntryToManifest, removeEntryFromManifest — package-level
duplicates of *Installer methods. Tests rewritten to exercise the
live Installer methods instead, which fixes a latent path-resolution
inconsistency between manifestPathForScope and Installer.manifestPath
(the former hard-coded paths, the latter respects projectGitRoot).
- internal/extensions/subagent.go: SpawnSubagent + helpers
(generateSubagentID, findKitBinary, subagentJSONOutput). The
subprocess-spawn implementation is unreachable; production code
routes through kit.Kit.Subagent (in-process). Types
(SubagentConfig/Result/Handle/etc.) and the SubagentHandle methods
remain because they are exposed to extensions via Yaegi symbols and
the Context.SpawnSubagent field.
- cmd/root.go: LoadConfigWithEnvSubstitution — one-line wrapper around
kit.LoadConfigWithEnvSubstitution with zero callers.
go test -race ./... passes.
The same ~40-line block — building a kit.SubagentConfig, wrapping OnEvent through sdkEventToSubagentEvent, calling kitInstance.Subagent, and translating the SDK result into extensions.SubagentResult — was copy-pasted three times: * cmd/root.go (interactive TUI Context, line 1148) * cmd/root.go (post-SessionStart runtime Context, line 1446) * internal/acpserver/session.go (ACP server Context, line 154) A separate sdkEventToSubagentEvent function was duplicated byte-for-byte between cmd/root.go and internal/acpserver/session.go. Both are now consolidated in a new internal/extbridge package which is the only module-internal home that can legitimately import both pkg/kit/ (the public SDK) and internal/extensions/. cmd/ and internal/acpserver/ both import it, so SDK-event-to-extension-event schema changes only have one site to update. Also fixes pkg/kit/events.go godoc comment that named the underlying LLM library, per AGENTS.md 'No Dependency Name Leakage' rule for exported SDK symbols. go test -race ./... passes.
The previous runNormalMode contained two nearly-identical 400-line
extensions.Context literal expressions:
* the startup-time literal (cmd/root.go:853-1307) that buffered
Print* calls into startupExtensionMessages
* the runtime literal (cmd/root.go:1311-1605) that routed Print*
through appInstance.PrintFromExtension
Every other field — Compact, SendMultimodalMessage, the four prompt
factories, all 25+ data-access fields, all four bridge phases — was
duplicated byte-for-byte. Maintainers had to remember to update both
copies whenever an extension Context field was added.
cmd/root.go is now 1463 lines (was 2225). The new helper lives in
cmd/extension_context.go (455 lines, mostly the closures verbatim) and
returns an extensions.Context with every field populated except
Print/PrintInfo/PrintError, which each call site sets afterwards to
match its phase. This preserves AGENTS.md's 'function field bug'
guarantee — all assignments remain anonymous closure literals.
Output of 'kit --version' / 'kit --help' unchanged. Full test suite
passes.
The TestUserBlockHighlightsFileTokens test was rewritten to call HighlightFileTokens directly (UserBlock was deleted in the dead-code sweep). That left testTypography with no callers, so staticcheck U1000 flagged it.
|
Connected to Huly®: KIT-25 |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors extension context wiring by introducing a centralized builder function and subagent spawning bridge; consolidates manifest management from package-level to instance methods; removes a subagent subprocess implementation; and cleans up deprecated UI and configuration loader APIs across multiple files. ChangesExtension Context & Subagent Spawning Bridge
Manifest & Installer Refactoring
UI & Configuration API Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR involves substantial structural changes across extension context wiring (new builder + bridge), manifest persistence refactoring (package-level to instance methods), removal of subagent subprocess implementation (225 lines), and cleanup of multiple deprecated UI/configuration APIs. The changes span heterogeneous file types and introduce new inter-module dependencies (extbridge bridge), requiring careful verification of control flow, event translation semantics, manifest operation correctness across test cases, and validation that removed APIs are no longer referenced in the codebase.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (3)
internal/extensions/installer_test.go (1)
348-355: ⚡ Quick winPrefer
t.Setenvover manualos.Setenv/os.Unsetenv.If the test exits early (panic or
t.Fatalbefore the defer is registered),XDG_DATA_HOMEleaks to subsequent tests in the same process.t.Setenv(Go 1.17+) restores automatically on cleanup, even on panic.♻️ Proposed refactor
- if err := os.Setenv("XDG_DATA_HOME", tempDir); err != nil { - t.Fatalf("Setenv() error = %v", err) - } - defer func() { - if err := os.Unsetenv("XDG_DATA_HOME"); err != nil { - t.Logf("Unsetenv() error = %v", err) - } - }() + t.Setenv("XDG_DATA_HOME", tempDir)🤖 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/extensions/installer_test.go` around lines 348 - 355, Replace the manual os.Setenv/os.Unsetenv pattern in the test with t.Setenv to ensure automatic cleanup on test failure or panic: instead of calling os.Setenv("XDG_DATA_HOME", tempDir) and deferring os.Unsetenv, call t.Setenv("XDG_DATA_HOME", tempDir) (remove the defer block) so the testing framework restores the environment variable automatically; update the test in installer_test.go where these calls appear accordingly.cmd/extension_context.go (1)
219-243: 💤 Low valueReuse the parsed provider/model instead of parsing twice.
models.ParseModelString(modelString)is called once at line 220 and again at line 228 for the same input. Reusing the first result keeps the two branches consistent and trims an avoidable parse.♻️ Proposed tweak
- // Notify TUI so it updates model in status bar. - p, m, _ := models.ParseModelString(modelString) - appInstance.NotifyModelChanged(p, m) + // Notify TUI so it updates model in status bar. + newProvider, newModel, _ := models.ParseModelString(modelString) + appInstance.NotifyModelChanged(newProvider, newModel) @@ - // Update usage tracker with new model info for correct token counting. - if usageTracker != nil { - newProvider, newModel, _ := models.ParseModelString(modelString) - if newProvider != "unknown" && newModel != "unknown" && newProvider != "ollama" { + // Update usage tracker with new model info for correct token counting. + if usageTracker != nil { + if newProvider != "unknown" && newModel != "unknown" && newProvider != "ollama" {🤖 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 `@cmd/extension_context.go` around lines 219 - 243, The second call to models.ParseModelString(modelString) is redundant; reuse the previously parsed values p and m (from p, m, _ := models.ParseModelString(modelString)) when updating the usage tracker instead of parsing again into newProvider/newModel. Replace uses of newProvider/newModel with p/m in the usageTracker branch and keep the Anthropic OAuth check using p (provider) to set isOAuth, then call usageTracker.UpdateModelInfo(modelInfo, p, isOAuth).cmd/root.go (1)
845-883: ⚡ Quick winAvoid building the interactive extension context twice.
The two
buildInteractiveExtensionContextcalls take identicalextensionContextDeps; the only delta is the threePrint*fields. Re-running the entire builder right afterEmitSessionStartre-allocates every closure for fields you don't change. Just rebind the three print routes and re-set the context.♻️ Proposed simplification
- extCtx := buildInteractiveExtensionContext(extensionContextDeps{ - ctx: ctx, - cwd: cwd, - modelName: modelName, - interactive: positionalPrompt == "", - kitInstance: kitInstance, - appInstance: appInstance, - usageTracker: usageTracker, - }) - extCtx.Print = func(text string) { - // Capture messages during startup, print after startup banner. - startupExtensionMessages = append(startupExtensionMessages, text) - } - extCtx.PrintInfo = func(text string) { - startupExtensionMessages = append(startupExtensionMessages, text) - } - extCtx.PrintError = func(text string) { - startupExtensionMessages = append(startupExtensionMessages, text) - } - kitInstance.Extensions().SetContext(extCtx) - kitInstance.Extensions().EmitSessionStart() - - // Restore normal print functions for runtime use. - extCtx = buildInteractiveExtensionContext(extensionContextDeps{ - ctx: ctx, - cwd: cwd, - modelName: modelName, - interactive: positionalPrompt == "", - kitInstance: kitInstance, - appInstance: appInstance, - usageTracker: usageTracker, - }) - extCtx.Print = func(text string) { appInstance.PrintFromExtension("", text) } - extCtx.PrintInfo = func(text string) { appInstance.PrintFromExtension("info", text) } - extCtx.PrintError = func(text string) { appInstance.PrintFromExtension("error", text) } - kitInstance.Extensions().SetContext(extCtx) + extCtx := buildInteractiveExtensionContext(extensionContextDeps{ + ctx: ctx, + cwd: cwd, + modelName: modelName, + interactive: positionalPrompt == "", + kitInstance: kitInstance, + appInstance: appInstance, + usageTracker: usageTracker, + }) + // Phase 1: buffer extension prints during SessionStart so they + // land after the startup banner. + extCtx.Print = func(text string) { startupExtensionMessages = append(startupExtensionMessages, text) } + extCtx.PrintInfo = func(text string) { startupExtensionMessages = append(startupExtensionMessages, text) } + extCtx.PrintError = func(text string) { startupExtensionMessages = append(startupExtensionMessages, text) } + kitInstance.Extensions().SetContext(extCtx) + kitInstance.Extensions().EmitSessionStart() + + // Phase 2: route extension prints through the running app. + extCtx.Print = func(text string) { appInstance.PrintFromExtension("", text) } + extCtx.PrintInfo = func(text string) { appInstance.PrintFromExtension("info", text) } + extCtx.PrintError = func(text string) { appInstance.PrintFromExtension("error", text) } + kitInstance.Extensions().SetContext(extCtx)🤖 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 `@cmd/root.go` around lines 845 - 883, The code builds the same extension context twice with buildInteractiveExtensionContext around EmitSessionStart, causing unnecessary re-allocation; instead, build extCtx once, set the startup-print closures (using startupExtensionMessages) before kitInstance.Extensions().EmitSessionStart(), call SetContext and EmitSessionStart, then simply rebind extCtx.Print, extCtx.PrintInfo and extCtx.PrintError to appInstance.PrintFromExtension(...) and call kitInstance.Extensions().SetContext(extCtx) again—i.e., remove the second buildInteractiveExtensionContext call and only mutate the three Print* fields on the existing extCtx.
🤖 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 `@cmd/extension_context.go`:
- Line 50: Several extensions.Context fields are assigned bare method/function
references which must be replaced with anonymous closure literals that forward
arguments; replace assignments like PrintBlock:
appInstance.PrintBlockFromExtension, GetChildren: kitInstance.GetChildren,
GetAvailableSkills: kitInstance.DiscoverSkillsForExtension,
ParseTemplate/RenderTemplate/ParseArguments/SimpleParseArguments,
ResolveModelChain and CheckModelAvailable with func(...) { return
<original>(...) } literals that accept the same parameters and return the same
results so the values crossing the Yaegi boundary (via
kitInstance.Extensions().SetContext(...)) are concrete closures rather than bare
references.
---
Nitpick comments:
In `@cmd/extension_context.go`:
- Around line 219-243: The second call to models.ParseModelString(modelString)
is redundant; reuse the previously parsed values p and m (from p, m, _ :=
models.ParseModelString(modelString)) when updating the usage tracker instead of
parsing again into newProvider/newModel. Replace uses of newProvider/newModel
with p/m in the usageTracker branch and keep the Anthropic OAuth check using p
(provider) to set isOAuth, then call usageTracker.UpdateModelInfo(modelInfo, p,
isOAuth).
In `@cmd/root.go`:
- Around line 845-883: The code builds the same extension context twice with
buildInteractiveExtensionContext around EmitSessionStart, causing unnecessary
re-allocation; instead, build extCtx once, set the startup-print closures (using
startupExtensionMessages) before kitInstance.Extensions().EmitSessionStart(),
call SetContext and EmitSessionStart, then simply rebind extCtx.Print,
extCtx.PrintInfo and extCtx.PrintError to appInstance.PrintFromExtension(...)
and call kitInstance.Extensions().SetContext(extCtx) again—i.e., remove the
second buildInteractiveExtensionContext call and only mutate the three Print*
fields on the existing extCtx.
In `@internal/extensions/installer_test.go`:
- Around line 348-355: Replace the manual os.Setenv/os.Unsetenv pattern in the
test with t.Setenv to ensure automatic cleanup on test failure or panic: instead
of calling os.Setenv("XDG_DATA_HOME", tempDir) and deferring os.Unsetenv, call
t.Setenv("XDG_DATA_HOME", tempDir) (remove the defer block) so the testing
framework restores the environment variable automatically; update the test in
installer_test.go where these calls appear accordingly.
🪄 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: 1393a7d6-fab4-42ee-945d-537a6e35f9ec
📒 Files selected for processing (25)
.kit/prompts/code-audit.mdcmd/extension_context.gocmd/root.gointernal/acpserver/session.gointernal/extbridge/extbridge.gointernal/extensions/installer.gointernal/extensions/installer_test.gointernal/extensions/manifest.gointernal/extensions/subagent.gointernal/models/cache_options.gointernal/models/cache_options_test.gointernal/models/pool.gointernal/prompts/loader.gointernal/ui/block_renderer.gointernal/ui/cli.gointernal/ui/commands/commands.gointernal/ui/debug_logger.gointernal/ui/message_items.gointernal/ui/render/blocks.gointernal/ui/render/blocks_test.gointernal/ui/style/enhanced.gointernal/ui/style/styles.gointernal/ui/style/themes.gointernal/ui/tool_approval_input.gopkg/kit/events.go
💤 Files with no reviewable changes (16)
- internal/models/pool.go
- internal/ui/debug_logger.go
- internal/ui/style/enhanced.go
- internal/models/cache_options_test.go
- internal/ui/tool_approval_input.go
- internal/models/cache_options.go
- internal/prompts/loader.go
- internal/ui/block_renderer.go
- internal/ui/commands/commands.go
- internal/ui/style/styles.go
- internal/ui/style/themes.go
- internal/ui/message_items.go
- internal/extensions/subagent.go
- internal/extensions/installer.go
- internal/extensions/manifest.go
- internal/ui/cli.go
Per AGENTS.md 'Yaegi function field bug', named function/method
references assigned to extensions.Context fields return zero values
across the interpreter boundary. The two SetContext literals in
runNormalMode (now consolidated in buildInteractiveExtensionContext)
inherited 9 bare references that need to be anonymous closure literals:
PrintBlock, GetChildren, GetAvailableSkills, ParseTemplate,
RenderTemplate, ParseArguments, SimpleParseArguments,
ResolveModelChain, CheckModelAvailable
Each is now wrapped as 'func(args) ret { return <orig>(args) }'.
Behaviour unchanged in regular Go; Yaegi extensions that consume these
fields will now see callable closures instead of zero values.
Verified with go test -race ./...
Addresses two CodeRabbit feedback items on PR #24: * Docstring coverage warning (was 57.14%, threshold 80%): adds godoc comments to the four test functions added or substantially rewritten in this PR — TestLoadAndSaveManifest, TestAddAndRemoveFromManifest, TestFindInManifest, TestHighlightFileTokensInjectsANSI. * Quick-win nitpick: replaces the manual os.Setenv/os.Unsetenv + defer pattern in TestFindInManifest with t.Setenv, which restores the env var automatically on cleanup even on panic or t.Fatal. go test -race ./... still passes.
Contributor
Author
|
@CodeRabbit have a quick look please |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
This was referenced Jun 9, 2026
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
Read-only audit-driven cleanup of the repository. Removes ~1,300 lines
of unreferenced code, eliminates two long-standing duplication clusters,
and tightens one godoc-level dependency-name leak in the public SDK.
No behavioural change —
kit --version/kit --helpand the fulltest suite (
go test -race ./...) produce identical output before andafter.
The audit was scoped to
cmd/,internal/, andpkg/kit/. Findingswere verified against
go vet,staticcheck -checks=all, andgolang.org/x/tools/cmd/deadcodebefore any deletion.Type of Change
What changed
Dead code removed (3 entire files, ~1,300 lines):
internal/models/pool.go—ProviderPoolwas never wired intokitsetup.SetupAgentor any agent constructor.internal/ui/debug_logger.go—CLIDebugLoggerwas unreachable;debug routing goes through
internal/tools/buffered_logger.go.internal/ui/tool_approval_input.go—tea.Modelnever instantiated;approvals are handled inline in
model.go.Plus targeted deletions in:
internal/extensions/subagent.go—SpawnSubagentsubprocessimplementation + helpers (production code routes through
kit.Kit.Subagentin-process). Types stay; they're exposed toextensions via Yaegi symbols.
internal/extensions/manifest.go— six package-level helpers thatduplicated
*Installermethods. Tests rewritten to exercise the liveInstallermethods, which fixes a latent path-resolutioninconsistency between
manifestPathForScope(hard-coded paths) andInstaller.manifestPath(respectsprojectGitRoot).internal/ui/style/enhanced.go,block_renderer.go,render/blocks.go,commands/commands.go,style/themes.go,style/styles.go,cli.go,message_items.go,prompts/loader.go,models/cache_options.go,extensions/installer.go,cmd/root.go— individual unused functions and constructors. Eachhas zero non-test references; tests that existed only to exercise the
dead code were either rewritten to test the live equivalent or
dropped.
Duplication consolidated:
internal/extbridge/package — collapses the SDK→extension eventand subagent translation that was copy-pasted in three places
(
cmd/root.go× 2,internal/acpserver/session.go× 1). BothsdkEventToSubagentEventand the ~35-lineSpawnSubagentclosurenow live in one place; SDK event-schema changes only need to be made
once.
cmd/extension_context.gobuildInteractiveExtensionContexthelper — collapses two near-identical 400-line
extensions.Contextliteral expressions in
runNormalMode. Each call site now sets onlythe three
Print*fields that differ between the startup-bufferingphase and the runtime-routing phase.
cmd/root.goshrank from 2,225lines to 1,463 lines. All function fields remain anonymous closure
literals to preserve the AGENTS.md "function field bug" guarantee
for Yaegi.
SDK godoc fix:
pkg/kit/events.go:151—FinishReason*constants no longer mentionthe underlying LLM library by name in their godoc. Per AGENTS.md
"No Dependency Name Leakage", dependency names must not appear in
comments on exported SDK symbols.
Verification
go build ./...— cleango vet ./...— cleango test -race ./internal/... ./pkg/... ./cmd/...— all packages passstaticcheck -checks=all ./...— clean except for pre-existingST1000/ST1003/ST1020-22stylistic nits that were already onmaster
kit --versionandkit --help— output identicalgolang.org/x/tools/cmd/deadcode— remaining hits are all public SDKsurface (
pkg/kit/,pkg/extensions/test/) or symbols intentionallyretained for Yaegi/test-only use; documented in the audit report
Checklist
go vet,staticcheck)Additional information
Out of scope for this PR (audit recommended discussion before changing):
internal/auth/credentials.goSet*Credentials/validate*—test-only, but plausibly future SDK exposure. Left untouched.
internal/extensions/runner.go normalizeContext— 259-line nil-checkchain. Refactor risk around the AGENTS.md "function field bug"; left
for a focused follow-up.
cmd/root.go runNormalModeandinternal/ui/model.go AppModel.Updatemega-functions — flagged medium-risk in the audit; out of scope here.
pkg/kit/kit.goviper-singleton TODO — genuine architecture work.internal/extensionstypes in exported signatures — anarchitectural decision; recommend discussing before action.
Files added:
cmd/extension_context.gointernal/extbridge/extbridge.go.kit/prompts/code-audit.md(the audit prompt itself; harmless localasset, can be dropped on request)
Summary by CodeRabbit
Release Notes