fix: bash streaming deadlock, tool over-count, and lint debt - #110
Merged
Conversation
Mechanical cleanup of the 16 modernize findings reported by golangci-lint, applied with `golangci-lint run --fix` and reviewed hunk by hunk: - 13x slicesbackward: reverse index loops become slices.Backward - 2x stringscut: strings.SplitN(s, sep, 2)[0] becomes strings.Cut - 1x reflecttypeassert: Interface().(T) becomes reflect.TypeAssert[T] No behaviour change. Loops that used the index after the range statement keep it via the two-variable form; the rest discard it. Two blank lines the autofixer left behind where it removed an element binding were cleaned up by hand.
The startup banner reported "extensions N tools" when no extension was loaded. Kit.GetExtensionToolCount returned len(agent.extraTools), but that slice is a shared bucket for everything that is neither a core tool nor an MCP tool: extension tools, the built-in activate_skill tool, SDK-supplied Options.ExtraTools and anything added at runtime via AddTools. Loading a single skill was therefore reported as one extension tool, which any user with skills in ~/.agents/skills and no extensions would see. The count now comes from the extension runner, which is the actual source of truth and already backs recomposeExtraTools. Agent.GetExtensionToolCount is renamed to GetExtraToolCount to describe what it really returns; the Agent holds no reference to the extension runner, which is the structural reason it could never answer this question correctly. Also fixes the second display path (internal/ui/factory.go), which printed "Loaded N extension tools" from the same value.
The streaming bash path caps a single output line at 1 MB. A longer line makes scanner.Scan() stop, and the loop ignored scanner.Err(), so the rest of the pipe was never read. The child process then blocked writing to a full pipe and cmd.Wait() never returned: the call hung until the command timeout fired, discarding every byte of output and reporting a timeout instead. Reproduced with a 2 MB single line, which hangs indefinitely without a timeout and is not exotic input — minified JSON, a packed bundle or `cat` of a binary all produce one. The scan error is now checked. The remainder of the pipe is drained to io.Discard so the child can exit, and the truncation is reported inline rather than failing silently. Surfaced by gopls (missing scanner.Err check); the deadlock behind it was not obvious from the diagnostic alone. Pre-existing since 3fc0ad9.
|
Connected to Huly®: KIT-111 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change replaces manual reverse-index loops with ChangesTool count and bash streaming
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR fixes bash streaming hangs and incorrect extension-tool counts without changing execution authority or public APIs. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant executeBashStreaming
participant Shell
participant streamOutput
participant OutputCallback
executeBashStreaming->>Shell: Start command with output pipe
Shell->>streamOutput: Write command output
streamOutput->>OutputCallback: Emit scanned chunks
streamOutput->>streamOutput: Detect scanner error
streamOutput->>Shell: Drain remaining pipe data
streamOutput->>OutputCallback: Emit output truncated notice
executeBashStreaming->>Shell: Wait for process exit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
Author
CI note:
|
12 tasks
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 branch clears the three items I deferred while working on
--bare(#109). Two turned out to be real bugs rather than cosmetic debt.Bash streaming deadlock (
internal/core/bash.go). The streaming bash path caps a single output line at 1 MB. A longer line makesscanner.Scan()stop, and the loop ignoredscanner.Err(), so the rest of the pipe was never read. The child process then blocked writing to a full pipe andcmd.Wait()never returned — the call hung until the command timeout fired, discarding all output and reporting a timeout instead. A 2 MB single line reproduces it onmaster; minified JSON, a packed bundle orcatof a binary all produce one. The scan error is now checked, the remainder is drained toio.Discardso the child can exit, and the truncation is reported inline.Extension tool over-count (
pkg/kit/kit.go). The startup banner showedextensions N toolswhen no extension was loaded.Kit.GetExtensionToolCountreturnedlen(agent.extraTools), but that slice is a shared bucket for everything that is neither a core tool nor an MCP tool — extension tools, the built-inactivate_skilltool, SDK-suppliedOptions.ExtraTools, and runtimeAddTools. Any user with a skill in~/.agents/skillsand no extensions saw it. The count now comes from the extension runner, which is the actual source of truth and already backsrecomposeExtraTools.Lint debt. The 16
modernizefindings reported bygolangci-lint, applied with--fixand reviewed hunk by hunk. No behaviour change.Type of Change
Checklist
go vet,gofmtclean;golangci-lintnow reports 0 issues, down from 16)go test -race ./...)Additional Information
Commits
dbf01822chore:16 modernize fixes — mechanical, no behaviour changecafa2841fix:extension tool count162b908dfix:bash streaming deadlockEach is independently reviewable, and the branch reads best commit by commit rather than as a squashed diff.
Both fixes were verified against the unfixed code
Neither test is a green-run-only assertion — each was confirmed to fail before the fix:
bash.gomakesTestBashStreaming_ReportsOversizedLinefail withdid not return within 30s (deadlocked on an undrained pipe?); with the fix it completes in 0.01s.kit.gomakes the tool-count tests fail withgot 1andgot 2— exactly the reported symptom.Both count tests carry premise guards (the skill really loaded, the tools really were added) so a passing
0cannot come from an empty fixture.Files
Added
internal/core/bash_streaming_test.go— 3 tests: oversized line, normal output control, boundary case just under the cappkg/kit/tool_count_test.go— 2 tests: skill tool excluded, SDK extra tools excludedModified — behavioural
internal/core/bash.go— drain the pipe and report truncation on scan errorpkg/kit/kit.go— count from the extension runnerinternal/agent/agent.go—GetExtensionToolCountrenamed toGetExtraToolCountModified — mechanical (modernize only)
internal/app/app.go,internal/compaction/compaction.go,internal/core/edit.go,internal/session/tree_manager.go,internal/ui/{model,scrolllist,tree_selector}.go,internal/ui/{activity,block_contract}_test.go,internal/ui/imagepreview/imagepreview_test.go,pkg/extensions/test/harness.goNotes for the reviewer
Agent.GetExtraToolCountnow has zero callers. It was the oldGetExtensionToolCount, renamed to describe what it actually returns. I kept it as a documented sibling toGetExtraToolsand as a warning against reusing the bucket for an extension count, but it is dead code on an internal type — happy to delete it if you would rather not carry it.slices.Backwardconversions that use the index (compaction.go,tree_manager.go) keep the two-variable form because the index escapes the loop (cut := i + 1,compactionIndex = i). The rest discard it.TestBashStreaming_LongButUnderLimitasserts on the streamed chunk, not the response.buildBashResponseapplies its own deliberate display truncation (defaultMaxLineLencaps a line at 2000 chars), which is a separate intended mechanism.Backward compatibility
No public SDK signature changed.
Kit.GetExtensionToolCountkeeps its name and semantics — it now returns the number it always claimed to. The renamed method is on aninternal/type, so it is not part of the SDK contract.Scope
This branch bundles one mechanical cleanup and two unrelated bug fixes, held together by provenance (all deferred from #109) rather than by subject. Happy to split the bash deadlock fix into its own PR if you would prefer it tracked separately — it is the most severe change here and touches the bash tool's output path.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests