fix: prevent premature subagent termination, add attack planning framework, and fix TUI lag#702
fix: prevent premature subagent termination, add attack planning framework, and fix TUI lag#702pennywise-01 wants to merge 1 commit into
Conversation
…ework, and fix TUI lag Subagent stop bug: - Remove stop_agent as suggested option in finish_scan docstring and rejection message - Rewrite root_agent.md Completion section to require waiting for all agents - Add warning to stop_agent docstring against using it to clear active agents Attack planning: - Add Attack Planning section to root_agent.md with think + load_skill framework - Add ATTACK PLANNING METHODOLOGY to system_prompt.jinja vulnerability_focus - Replace brute-force decomposition with technology-aware agent spawning TUI lag: - Batch SDK events via 150ms flush timer instead of per-event call_from_thread - Index events by agent_id for O(1) lookup instead of O(n) list scan - Cap events per agent at 500 with automatic pruning of oldest events - Window event rendering to last 200 events instead of full re-render - Reduce animation timer from 60ms to 150ms
Greptile SummaryThis PR addresses three distinct bugs: premature subagent termination (by tightening
Confidence Score: 3/5The guidance-only and docstring changes are safe to ship; the TUI batching rework has a threading model error that can silently drop display events and calls a non-thread-safe Textual API from a background thread. Four of the six changed files are prompt templates and docstrings with no runtime risk. The TUI changes in app.py introduce a flush callback that always routes through the RuntimeError exception path (because call_from_thread is invoked on the main thread rather than a background thread), and a race window where events appended to the buffer between the snapshot copy and the clear are permanently lost from the display. Additionally, set_timer is called from the background scan thread without thread-safety guarantees. These issues affect only the TUI display layer, not scan results, but the batching mechanism as written doesn't behave as intended. strix/interface/tui/app.py — the event-batching and flush-timer implementation needs a second look for correct threading; strix/interface/tui/live_view.py — the O(n) self.events.remove() maintenance is worth cleaning up. Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
strix/interface/tui/app.py:1421-1431
**Flush timer always takes the RuntimeError fallback path**
`_flush_event_buffer` is invoked by a Textual timer, which means it runs on the Textual main thread. Calling `call_from_thread` from the main thread always raises `RuntimeError`, so `_record_sdk_events_batch` is dead code — every flush goes through the per-event fallback loop. Separately, there is a race window between `batch = self._event_buffer[:]` (snapshot) and `self._event_buffer.clear()`: the background SDK thread can append an event into the buffer after the snapshot is taken but before `clear()` executes. That event is removed from the buffer without being in `batch`, silently dropping it from the TUI display.
### Issue 2 of 3
strix/interface/tui/app.py:1416-1419
**`set_timer` called from a background thread**
`_capture_sdk_event` is passed as the `event_sink` callback and is invoked from the background asyncio scan loop, not the Textual main thread. `set_timer` is not documented as thread-safe in Textual; the supported path for scheduling work from a background thread is `call_from_thread`. Also, the `if self._event_flush_timer is None` guard and the subsequent assignment are not atomic, so two rapid callback invocations from the scan loop can both observe `None` and register duplicate flush timers.
### Issue 3 of 3
strix/interface/tui/live_view.py:286-293
**`self.events.remove(d)` is O(n) overhead on an otherwise-unused list**
After the migration to `_events_by_agent`, `self.events` is no longer queried anywhere — `events_for_agent` and `has_events_for_agent` both use the new index. The per-prune `list.remove()` call is an O(n) linear scan across the entire global event list just to keep `self.events` in sync with a structure nothing reads. For long scans with many active agents the list can grow to `MAX_EVENTS_PER_AGENT × num_agents` entries, making each pruning step increasingly expensive.
Reviews (1): Last reviewed commit: "fix: prevent premature subagent terminat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _flush_event_buffer(self) -> None: | ||
| self._event_flush_timer = None | ||
| if not self._event_buffer: | ||
| return | ||
| batch = self._event_buffer[:] | ||
| self._event_buffer.clear() | ||
| try: | ||
| self.call_from_thread(self._record_sdk_event, agent_id, event) | ||
| self.call_from_thread(self._record_sdk_events_batch, batch) | ||
| except RuntimeError: | ||
| self._record_sdk_event(agent_id, event) | ||
| for agent_id, event in batch: | ||
| self._record_sdk_event(agent_id, event) |
There was a problem hiding this comment.
_flush_event_buffer is invoked by a Textual timer, which means it runs on the Textual main thread. Calling call_from_thread from the main thread always raises RuntimeError, so _record_sdk_events_batch is dead code — every flush goes through the per-event fallback loop. Separately, there is a race window between batch = self._event_buffer[:] (snapshot) and self._event_buffer.clear(): the background SDK thread can append an event into the buffer after the snapshot is taken but before clear() executes. That event is removed from the buffer without being in batch, silently dropping it from the TUI display.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/interface/tui/app.py
Line: 1421-1431
Comment:
**Flush timer always takes the RuntimeError fallback path**
`_flush_event_buffer` is invoked by a Textual timer, which means it runs on the Textual main thread. Calling `call_from_thread` from the main thread always raises `RuntimeError`, so `_record_sdk_events_batch` is dead code — every flush goes through the per-event fallback loop. Separately, there is a race window between `batch = self._event_buffer[:]` (snapshot) and `self._event_buffer.clear()`: the background SDK thread can append an event into the buffer after the snapshot is taken but before `clear()` executes. That event is removed from the buffer without being in `batch`, silently dropping it from the TUI display.
How can I resolve this? If you propose a fix, please make it concise.| def _capture_sdk_event(self, agent_id: str, event: Any) -> None: | ||
| self._event_buffer.append((agent_id, event)) | ||
| if self._event_flush_timer is None: | ||
| self._event_flush_timer = self.set_timer(0.15, self._flush_event_buffer) |
There was a problem hiding this comment.
set_timer called from a background thread
_capture_sdk_event is passed as the event_sink callback and is invoked from the background asyncio scan loop, not the Textual main thread. set_timer is not documented as thread-safe in Textual; the supported path for scheduling work from a background thread is call_from_thread. Also, the if self._event_flush_timer is None guard and the subsequent assignment are not atomic, so two rapid callback invocations from the scan loop can both observe None and register duplicate flush timers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/interface/tui/app.py
Line: 1416-1419
Comment:
**`set_timer` called from a background thread**
`_capture_sdk_event` is passed as the `event_sink` callback and is invoked from the background asyncio scan loop, not the Textual main thread. `set_timer` is not documented as thread-safe in Textual; the supported path for scheduling work from a background thread is `call_from_thread`. Also, the `if self._event_flush_timer is None` guard and the subsequent assignment are not atomic, so two rapid callback invocations from the scan loop can both observe `None` and register duplicate flush timers.
How can I resolve this? If you propose a fix, please make it concise.| if len(agent_events) > MAX_EVENTS_PER_AGENT: | ||
| dropped = agent_events[:len(agent_events) - MAX_EVENTS_PER_AGENT] | ||
| for d in dropped: | ||
| try: | ||
| self.events.remove(d) | ||
| except ValueError: | ||
| pass | ||
| del agent_events[:len(agent_events) - MAX_EVENTS_PER_AGENT] |
There was a problem hiding this comment.
self.events.remove(d) is O(n) overhead on an otherwise-unused list
After the migration to _events_by_agent, self.events is no longer queried anywhere — events_for_agent and has_events_for_agent both use the new index. The per-prune list.remove() call is an O(n) linear scan across the entire global event list just to keep self.events in sync with a structure nothing reads. For long scans with many active agents the list can grow to MAX_EVENTS_PER_AGENT × num_agents entries, making each pruning step increasingly expensive.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/interface/tui/live_view.py
Line: 286-293
Comment:
**`self.events.remove(d)` is O(n) overhead on an otherwise-unused list**
After the migration to `_events_by_agent`, `self.events` is no longer queried anywhere — `events_for_agent` and `has_events_for_agent` both use the new index. The per-prune `list.remove()` call is an O(n) linear scan across the entire global event list just to keep `self.events` in sync with a structure nothing reads. For long scans with many active agents the list can grow to `MAX_EVENTS_PER_AGENT × num_agents` entries, making each pruning step increasingly expensive.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
This PR fixes three bugs identified through systematic debugging:
Root agent prematurely stops subagents — The root agent would call stop_agent on active children to bypass the active-agent check in finish_scan, orphaning their work and producing incomplete reports.
Agent doesn't think deeply about attack planning — The root agent received high-level vulnerability categories but no structured thinking framework. It blindly spawned agents for every vulnerability type instead of reasoning about what's relevant to the target.
TUI freezes during multi-agent scans — The TUI became unresponsive for 1-2 minutes when multiple subagents were running, due to event flooding, unbounded event lists, and aggressive timers.
Changes
1. Prevent Premature Subagent Termination
tools/finish/tool.py
completed/crashedare safeskills/coordination/root_agent.md
tools/agents_graph/tools.py
2. Attack Planning Framework
skills/coordination/root_agent.md
load_skillbefore planningthinkto reason through target technology, attack surface, technique selection, defense analysis, bypass strategies, chaining opportunities, and priority orderingagents/prompts/system_prompt.jinja
<vulnerability_focus>with 5-step process: load skill → think → match techniques to technology → plan bypasses → chain for impact3. TUI Lag Fixes
interface/tui/app.py
call_from_threadper event (~50/sec → ~6/sec message queue pressure)interface/tui/live_view.py
dict[str, list[dict]]for O(1) lookup instead of O(n) list scanMAX_EVENTS_PER_AGENT = 500with automatic pruning of oldest events