fix: dedupe persistent toolset-failure notifications#2943
Conversation
Collapse the two duplicated once-per-streak warning guards (Start and Tools listing) into a small reusable failureStreak type, removing four bool fields and the repeated fail/reset/shouldReport logic.
The agent-side once-per-streak guard already ensures a toolset-failure warning is emitted only once per streak, so the on-screen dedup in handleShow was redundant defense-in-depth. Dropping it restores the simpler notification component; an identical error reappearing after the previous one auto-hides (10s) is acceptable.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Reviewed pkg/tools/startable.go, pkg/tools/startable_test.go, pkg/agent/agent.go, and pkg/agent/agent_test.go.
Summary: The failureStreak helper is correctly implemented — fail() only sets active/pending on the first call of a streak, reset() clears both fields, and shouldReport() returns true exactly once per streak. Mutex discipline is sound across Start, Stop, ShouldReportFailure, and ShouldReportListFailure. The Tools() method calls the inner toolset outside the lock (correct — avoids holding the lock during a potentially slow RPC) and updates the streak under the lock before any caller can observe it in the sequential collectTools loop. The listFlappyToolSet test helper correctly increments callIdx unconditionally before the nil check, so error sequences are consumed faithfully. Test coverage addresses the once-per-streak guarantee, recovery/reset behavior, and the Stop path. No bugs found in the changed code.
aheritier
left a comment
There was a problem hiding this comment.
Approved. CI green across all checks; changes are sound with appropriate test coverage for the de-duplicated list warnings and failure-streak reset behavior. No blocking concerns.
Toolset initialization and Tools() listing failures were stacking persistent notifications in the TUI when a toolset remained down across multiple conversation turns. The root cause was that the agent side had no mechanism to suppress duplicate warnings, so the TUI received the same error on every turn and piled them on-screen.
This fix adds a once-per-streak guard to the StartableToolSet so a failure warning is emitted only once until the streak clears (either via a successful Start/Tools or an explicit Stop). The failureStreak type encapsulates the streak-tracking logic and is reused for both Start and Tools listing failures, keeping the warning deduplication uniform and reliable across all callers—including non-TUI consumers like ACP, which persist warnings into the transcript.
The TUI-layer dedup logic is removed as it becomes redundant; the agent-side guard is the single source of truth for this behavior.
Closes #2884.