Skip to content

fix: reset tool aggregator whenever something other than a tool card gets inserted - #870

Open
kishore280 wants to merge 7 commits into
Nano-Collective:mainfrom
kishore280:fix/856-tool-aggregator-phase-reset
Open

fix: reset tool aggregator whenever something other than a tool card gets inserted#870
kishore280 wants to merge 7 commits into
Nano-Collective:mainfrom
kishore280:fix/856-tool-aggregator-phase-reset

Conversation

@kishore280

@kishore280 kishore280 commented Aug 13, 2026

Copy link
Copy Markdown

Fixes #856.

currentAggregator only reset at the end of a turn, not when a thought or text chunk interrupted a tool-call phase. Tool → Thought → Tool merged the second tool into the first card instead of starting a fresh one.

Review found the same gap in two more spots: a mutating-tool edit card and a plan update also skip the reset, so Tool → Edit → Tool hits the same bug. Pulled the check into one closeAggregatorIfIdle() helper instead of four copies of it.

close() was also re-expanding manually collapsed cards - toggle(force) never used the force argument, so close()'s toggle(false) just flipped whatever state it was already in. Fixed to match ThoughtAggregator's toggle, which already handles force correctly.

Testing the footer-per-turn change from the earlier review turned up two more bugs, both with repro tests: an old turn's copy button could grab a newer turn's text, and interrupting a still-pending tool could duplicate its card with a stuck spinner.

@akramcodez

Copy link
Copy Markdown
Collaborator

Thanks @kishore280 for opening the PR! Please add a changeset to your PR as well.

@akramcodez akramcodez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kishore280, thanks for the fix! I tested it and the tool-call phase separation looks good. I noticed one small regression though: the copy button and timestamp footer can now appear multiple times within the same turn after each text chunk. Please keep a single footer per turn and move it to the latest text block as the response streams. Once that's fixed, this should be good to go.

image

@kishore280
kishore280 force-pushed the fix/856-tool-aggregator-phase-reset branch from d4c0b17 to e4c3bb4 Compare August 13, 2026 12:48
@kishore280

Copy link
Copy Markdown
Author

Pushed 3 commits. First one is the footer-per-turn change from the review.

The other two are bugs I found while testing it, not extra scope:

  • copy button on an old turn's footer was grabbing the newest turn's text instead of its own — found while testing the footer reuse
  • interrupting a tool call before it finished (thought/text arriving while a tool was still pending) could duplicate its card with a stuck spinner — pre-existing regression from the original [Feature] Preserve tool-call boundaries between thought blocks #856 fix, caught with a repro test

jsdom repros:

# bug 2 — cross-turn footer copy
clicking footer A returned "Response B" -> now correctly returns "Response A"

# bug 3 — duplicate tool card on interrupt
interrupting a pending tool created 2 cards + 2 aggregators -> now 1 of each

…ator-phase-reset

# Conflicts:
#	assets/nanocoder-vscode.vsix
#	plugins/vscode/media/chat-panel.js
@will-lamerton

Copy link
Copy Markdown
Member

Hey @kishore280 - nice catch on the root cause, and mirroring the currentThoughtBox reset is the right shape. Two things before merge:

1. The reset misses the edit-card and plan-card paths. Mutating tools (write_file, replace_file_content, etc.) append straight to messagesContainer via createEditCard and never touch currentAggregator, same for handlePlanUpdate. Since ACP runs a round's tools sequentially, read_file → write_file → read_file puts the second read back into the aggregator above the edit card, which is the same out-of-order symptom #856 describes. Suggest pulling the reset into a helper and calling it from all four insertion points:

function closeAggregatorIfIdle() {
	if (currentAggregator && !aggregatorHasPendingTools(currentAggregator)) {
		currentAggregator.close();
		currentAggregator = null;
	}
}

2. close() re-expands a manually collapsed card. close() calls this.toggle(false) but toggle() takes no argument and just flips isOpen. Pre-existing, but this PR moves close() from once-per-turn to once-per-transition, so it's much easier to hit now. close() { if (this.isOpen) this.toggle(); } covers it.

Minor: the footer rework in 653885cd/e0b41b9a looks correct but isn't mentioned in the title, body, or changeset. Worth a line in the changeset since it changes copy-button behavior, plus a Closes #856 reference to match the other changesets.

@kishore280 kishore280 changed the title fix: reset tool aggregator when a thought or message interrupts a tool-call phase fix: reset tool aggregator whenever something other than a tool card gets inserted Aug 13, 2026
…thought/message

Also fixes close() re-expanding a manually collapsed card.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Preserve tool-call boundaries between thought blocks

3 participants