Skip to content

fix: prevent premature subagent termination, add attack planning framework, and fix TUI lag#702

Open
pennywise-01 wants to merge 510 commits into
usestrix:mainfrom
pennywise-01:main
Open

fix: prevent premature subagent termination, add attack planning framework, and fix TUI lag#702
pennywise-01 wants to merge 510 commits into
usestrix:mainfrom
pennywise-01:main

Conversation

@pennywise-01

Copy link
Copy Markdown

Summary

This PR fixes three bugs identified through systematic debugging:

  1. 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.

  2. 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.

  3. 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

  • Removed stop_agent as a suggested option in the finish_scan docstring pre-flight checklist
  • Removed "stopped" from the "safe to leave behind" list — only completed/crashed are safe
  • Updated rejection message to point to wait_for_message and explicitly prohibit stop_agent

skills/coordination/root_agent.md

  • Rewrote Completion section from 4-line generic checklist to explicit 8-step process
  • Added: "WAIT for all agents to self-terminate"
  • Added: "stopped agents were forcibly cancelled — their results are lost"
  • Added: "Never use stop_agent as a shortcut to bypass the active-agent check"

tools/agents_graph/tools.py

  • Added warning to stop_agent docstring: "NEVER use stop_agent to clear active children so you can call finish_scan"
  • Clarified that stop_agent is only for agents that are genuinely off-track or stuck

2. Attack Planning Framework

skills/coordination/root_agent.md

  • Added "Attack Planning" section with 3-step framework:
    • Step 1: Load vulnerability methodology via load_skill before planning
    • Step 2: Use think to reason through target technology, attack surface, technique selection, defense analysis, bypass strategies, chaining opportunities, and priority ordering
    • Step 3: Spawn targeted agents with informed task descriptions

agents/prompts/system_prompt.jinja

  • Added "ATTACK PLANNING METHODOLOGY" section to <vulnerability_focus> with 5-step process: load skill → think → match techniques to technology → plan bypasses → chain for impact
  • Replaced brute-force decomposition ("CREATE SUBAGENT for EACH vuln type × EACH component") with technology-aware agent spawning

3. TUI Lag Fixes

interface/tui/app.py

  • Batch SDK events: Buffer events and flush every 150ms instead of calling call_from_thread per event (~50/sec → ~6/sec message queue pressure)
  • Window event rendering: Only render last 200 events instead of full re-render
  • Reduce animation timer: 60ms → 150ms (2.5x less queue pressure, still smooth)

interface/tui/live_view.py

  • Index events by agent_id: dict[str, list[dict]] for O(1) lookup instead of O(n) list scan
  • Cap events per agent: MAX_EVENTS_PER_AGENT = 500 with automatic pruning of oldest events

0xallam and others added 30 commits January 21, 2026 20:27
Updates LiteLLM from ~1.80.7 to ~1.81.1 which includes
full support for z.ai (Zhipu AI) provider using the 'zai/model-name'
format. This enables Strix to work with z.ai subscription
credentials by setting STRIX_LLM="zai/glm-4.7" with appropriate
LLM_API_KEY and LLM_API_BASE environment variables.

Changes:
- Updated litellm version constraint in pyproject.toml
- No breaking changes to Strix API or configuration

Closes #ISSUE_ID (to be linked if applicable)

Signed-off-by: legendevent <legendevent@users.noreply.github.com>
The memory compressor was calling litellm.completion() without passing
the api_key and api_base parameters, causing authentication errors when
LLM_API_KEY is set but provider-specific env vars (OPENAI_API_KEY, etc.)
are not. This matches the pattern used in dedupe.py.
Bumps [pypdf](https://github.com/py-pdf/pypdf) from 6.6.0 to 6.6.2.
- [Release notes](https://github.com/py-pdf/pypdf/releases)
- [Changelog](https://github.com/py-pdf/pypdf/blob/main/CHANGELOG.md)
- [Commits](py-pdf/pypdf@6.6.0...6.6.2)

---
updated-dependencies:
- dependency-name: pypdf
  dependency-version: 6.6.2
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Remove hardcoded github.com/gitlab.com/bitbucket.org host lists from
infer_target_type. Instead, detect git repositories on any host by
querying the standard /info/refs?service=git-upload-pack endpoint.

Works for any self-hosted git instance.
Improve the finish_scan tool schema to produce more professional
pentest reports: expand parameter descriptions with structural
guidance, rewrite recommendations example with proper urgency tiers
instead of Priority 0/1/2, fix duplicated section titles, and clean
up informal language.
Enable native text selection across tool components and agent messages
with automatic clipboard copy, toast notification, and decorative icon
stripping. Replace Padding wrappers with Text to support selection
across multiple renderables.
Bumps [pillow](https://github.com/python-pillow/Pillow) from 11.3.0 to 12.1.1.
- [Release notes](https://github.com/python-pillow/Pillow/releases)
- [Changelog](https://github.com/python-pillow/Pillow/blob/main/CHANGES.rst)
- [Commits](python-pillow/Pillow@11.3.0...12.1.1)

---
updated-dependencies:
- dependency-name: pillow
  dependency-version: 12.1.1
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [cryptography](https://github.com/pyca/cryptography) from 44.0.1 to 46.0.5.
- [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst)
- [Commits](pyca/cryptography@44.0.1...46.0.5)

---
updated-dependencies:
- dependency-name: cryptography
  dependency-version: 46.0.5
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [protobuf](https://github.com/protocolbuffers/protobuf) from 6.33.4 to 6.33.5.
- [Release notes](https://github.com/protocolbuffers/protobuf/releases)
- [Commits](https://github.com/protocolbuffers/protobuf/commits)

---
updated-dependencies:
- dependency-name: protobuf
  dependency-version: 6.33.5
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
… and CVSS

Replace 12 flat parameters (code_file, code_before, code_after, code_diff,
and 8 CVSS fields) with structured nested XML fields: code_locations with
co-located fix_before/fix_after per location, cvss_breakdown, and cwe.

This enables multi-file vulnerability locations, per-location fixes with
precise line numbers, data flow representation (source/sink), CWE
classification, and compatibility with GitHub/GitLab PR review APIs.
- Specify encoding="utf-8" in registry.py _load_xml_schema()
- Specify encoding="utf-8" in skills/__init__.py load_skills()
- Prevents cp949/shift_jis/cp1252 decoding errors on non-English Windows
… multi-part suggestions

Rewrote the code_locations parameter description to make fix_before/fix_after
semantics explicit: they are literal block-level replacements mapped directly
to GitHub/GitLab PR suggestion blocks. Added guidance for multi-part fixes
(separate locations for non-contiguous changes like imports + code), common
mistakes to avoid, and updated all examples to demonstrate multi-line ranges.
* feat: add to readme new keys

* feat: shoutout strix models, docs

* fix: mypy error

* fix: base api

* docs: update quickstart and models

* fixes: changes to docs

uniform api_key variable naming

* test: git commit hook

* nevermind it was nothing

* docs: Update default model to claude-sonnet-4.6 and improve Strix Router docs

- Replace gpt-5 and opus-4.6 defaults with claude-sonnet-4.6 across all docs and code
- Rewrite Strix Router (models.mdx) page with clearer structure and messaging
- Add Strix Router as recommended option in overview.mdx and quickstart prerequisites
- Update stale Claude 4.5 references to 4.6 in anthropic.mdx, openrouter.mdx, bug_report.md
- Fix install.sh links to point to models.strix.ai and correct docs URLs
- Update error message examples in main.py to use claude-sonnet-4-6

---------

Co-authored-by: 0xallam <ahmed39652003@gmail.com>
0xallam and others added 27 commits June 8, 2026 13:18
OpenAI's Responses API rejects reasoning.effort on non-reasoning
models like gpt-4o with `unsupported_parameter`, so any scan with
the default STRIX_REASONING_EFFORT=high against gpt-4o crashed at
the first model call. drop_params=True absorbs the rejected param
on LiteLLM-routed models but the SDK's native OpenAI path has no
equivalent.

Lift model_supports_reasoning to a public helper that strips
litellm/, any-llm/, openai/ prefixes and falls back to last-segment
lookup so prefixed forms like anthropic/claude-opus-4-7 resolve
through the bare model_cost entry. make_model_settings regains
model_name and skips Reasoning() when the registry doesn't confirm
support. uses_chat_completions_tool_schema reuses the same helper
(was duplicating the lookup under a misleading name).
Register a litellm.success_callback that captures kwargs['response_cost']
into a new observed-cost bucket on LLMUsageLedger. record() skips the
tokens-times-registry estimate for LiteLLM-routed models so we do not
double-count with the callback; OpenAI direct routes keep estimating
since LiteLLM is not invoked for them. Per-agent attribution for
LiteLLM-routed calls is apportioned by token share at to_record() time.
* fix: resolve pre-commit check failures

- Change RuntimeError to TypeError for type validation in report/writer.py
- Update pyupgrade to v3.21.2 for Python 3.14 compatibility

* feat(cli): add --max-budget-usd flag

Raises BudgetExceededError in ReportUsageHooks after each LLM call when
accumulated cost reaches the limit, with clean "stopped" status and
child-agent cancellation in non-interactive mode.

* test: add budget enforcement unit tests

7 tests covering no-budget, under-budget, at-limit, over-limit, error
message content, None report state, and exception hierarchy.
Also adds pytest/pytest-asyncio to dev deps and a mypy override for tests.

* fix(budget): validate positive budget and check the live cost ledger

Two hardening fixes for --max-budget-usd enforcement:

- Reject non-positive budgets. ReportUsageHooks now raises ValueError for
  max_budget_usd <= 0, and the CLI validates the flag via a custom argparse
  type so '--max-budget-usd 0' fails fast with a friendly message instead of
  silently killing the scan on the first model response.
- Read the live cost. The budget check now reads ReportState.get_total_llm_cost()
  (the live ledger) instead of the persisted run-record snapshot, so it stays
  accurate even when a usage save fails after a model call.

* fix(budget): stop the entire scan deterministically when the limit is hit

Previously a BudgetExceededError was handled per-agent: it was swallowed in
interactive mode (the loop kept waiting), a child's error escaped its detached
task as an unretrieved-exception warning, the parent was never released from
wait_for_message, and the stop was logged at ERROR with a traceback as if the
agent had failed.

Replace that with a single scan-wide signal on the coordinator:

- AgentCoordinator.trigger_budget_stop() sets a flag and wakes every parked
  agent; wait_for_message returns as soon as the flag is set.
- The run loops check coordinator.budget_stopped and raise to exit cleanly,
  marking themselves 'stopped'. The root's exception reaches run_strix_scan's
  handler, which cancels descendants and tears the scan down once; child
  exceptions are swallowed in their detached task.
- The budget stop is logged at INFO, not as a failure.

This is deterministic regardless of tree depth or which agent first sees the
limit, fixing the interactive/TUI hang where a deep agent's stop never reached
a parked root. Also re-raises BudgetExceededError explicitly in the stream
handler so it can't be mistaken for the LiteLLM 'after shutdown' race.

* fix(budget): treat a budget stop as a clean stop in the TUI

Add an explicit BudgetExceededError handler in the TUI scan thread so that, if
the error ever reaches it, the budget stop is logged as a graceful stop rather
than surfaced as a red scan error by the broad 'except Exception'. The runner
normally absorbs the error and returns cleanly, so this is defensive depth for
a money-spending feature.

* docs(cli): document --max-budget-usd behavior and limitations

Clarify that the budget is cumulative across all agents, checked after each
model response, that the scan stops cleanly (not as a failure), that the value
must be > 0, and that spend can slightly overshoot due to in-flight calls and
best-effort cost estimation.

* Apply suggestions from code review

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

---------

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
* fix: resolve pre-commit check failures

- Change RuntimeError to TypeError for type validation in report/writer.py
- Update pyupgrade to v3.21.2 for Python 3.14 compatibility

* chore: add pytest test infrastructure

Mirror the layout introduced on feature/438-token_budget: pytest +
pytest-asyncio dev deps, asyncio_mode auto, a tests.* mypy override, and
pytest in the mypy pre-commit hook deps so the tests/ package type-checks.

* feat: add --mount and large-target pre-flight for local repos (usestrix#492)

Large local targets were copied into the sandbox file-by-file via the SDK
LocalDir entry, which stalls on big repos and could leave /workspace empty.

- --mount <path> bind-mounts a host directory read-only at /workspace/<subdir>
  instead of copying it, bypassing the per-file stream.
- A size pre-flight (STRIX_MAX_LOCAL_COPY_MB, default 1024) fails fast with a
  clear message suggesting --mount when a non-mounted local target is too big.

* fix: reject empty --mount paths

An empty or whitespace-only --mount value resolves to the current working
directory and would silently bind-mount it into the sandbox. Reject it.

* fix: dedupe local targets so a dir is never both copied and mounted

If the same directory is passed via --target and --mount (or as duplicate
values), it previously produced two targets — copied AND bind-mounted, and
the copied one could trip the size pre-flight. Dedupe by resolved path,
preferring the bind mount.

* fix: treat non-positive STRIX_MAX_LOCAL_COPY_MB as disabled

Previously a value of 0 (or negative) made every local target count as
oversized, aborting all local scans. Now <= 0 disables the pre-flight.

* fix: log unreadable subtrees during size pre-flight

os.walk silently swallowed directory-listing errors, so a permission-denied
subtree could make a large repo under-count and slip past the pre-flight.
Surface such omissions via an onerror warning.

* docs: document --mount and STRIX_MAX_LOCAL_COPY_MB

Add CLI reference + example for --mount, document the size pre-flight env var,
note the read-only-is-not-a-hard-boundary caveat and that remote repos are not
size-checked, and clarify the backends docstring on when bind mounts apply.

* Update strix/interface/main.py


* Update strix/runtime/docker_client.py


---------
…trix#596)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…sestrix#637)

Line 72 was over-indented, causing an IndentationError on import of strix/report/writer.py and breaking main. Also bump the mirrors-mypy pre-commit hook to v1.17.1 to avoid the mypy 1.16.0 internal crash (python/mypy#19412) on openai/_client.py.
Co-authored-by: Alex Schapiro <46074070+bearsyankees@users.noreply.github.com>
* fix: capture cost for streamed LiteLLM responses

* docs: note LiteLLM streaming metadata callbacks
…ation, Django (usestrix#617)

* Add five community security skills for agent specialization

Expand coverage with OAuth flow testing, AWS misconfigurations, prototype
pollution, insecure deserialization, and Django framework playbooks.

* Address Greptile review feedback on AWS and deserialization skills

- Use head-bucket for S3 existence checks instead of duplicating s3 ls
- Add Node.js to insecure_deserialization frontmatter description

* Clarify S3 existence vs public listing checks in aws skill

Split unauthenticated enumeration into separate head-bucket/HTTP
and s3 ls steps with interpretation guidance per review.

* some tools ads

---------

Co-authored-by: bearsyankees <bearsyankees@gmail.com>
…sestrix#626)

* feat(report): SARIF 2.1.0 emitter for CI / code-scanning integration

Strix emits CSV + markdown + JSON but no SARIF, so findings can't feed
GitHub code-scanning, an ASPM, or any SARIF-consuming CI gate. Add a
stdlib-only emitter (strix/report/sarif.py) and always write findings.sarif
from ReportState._save_artifacts, beside the existing artifacts.

Design invariants (learned from running this in production):
- Stable partialFingerprints.primaryLocationLineHash per finding, so a
  re-scan that re-words a title doesn't churn code-scanning alert IDs.
- Class/category hashing so the same vuln class maps to a stable ruleId
  across scans rather than drifting.
- Findings with no code location anchor to SECURITY.md with a synthetic
  location marker instead of being silently dropped.
- Always emit (even with zero findings) so a clean re-scan overwrites a
  stale findings.sarif and code-scanning auto-resolves fixed alerts.
- tool.driver.version reports the strix package version.
- Fully isolated in its own try/except: a SARIF build error must never
  break the CSV/MD/run-record path.

Verified end-to-end on v1.0.4 against a SQLi/cmd-inj/weak-hash fixture:
3 findings -> valid SARIF 2.1.0, 3 results, real code locations, distinct
per-finding fingerprints.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(report): complete SARIF code scanning metadata

---------
Co-authored-by: bearsyankees <bearsyankees@gmail.com>
…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-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR addresses three distinct bugs: premature subagent termination (by tightening finish_scan and stop_agent docstrings and root-agent guidance), lack of structured attack planning (by adding a load-skill → think → spawn framework to both the system prompt and root-agent skill), and TUI lag during multi-agent scans (by batching SDK events and indexing events per agent).

  • Subagent termination fix: finish_scan and stop_agent docstrings now explicitly prohibit using stop_agent to bypass the active-agent guard; the root-agent completion checklist is expanded to an 8-step wait-then-finish process.
  • Attack planning framework: A 5-step methodology (load skill → think → match to technology → plan bypasses → chain) is injected into the system prompt and root-agent skill, replacing the prior blanket per-category agent spawning.
  • TUI performance: Events are buffered and flushed every 150ms instead of one-by-one; _events_by_agent provides O(1) per-agent lookups; a 500-event cap per agent prevents unbounded growth.

Confidence Score: 3/5

The 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

Filename Overview
strix/interface/tui/app.py Adds 150ms event-batching to cut TUI message-queue pressure; the flush callback always routes through the RuntimeError fallback (call_from_thread is called on the main thread) and has a race that can silently drop buffered events. Also calls set_timer from a background thread.
strix/interface/tui/live_view.py Adds O(1) per-agent event index and a 500-event cap per agent. Cap logic is correct for single-event appends, but the synchronisation of self.events via O(n) list.remove() is wasted work since self.events is no longer queried externally.
strix/tools/finish/tool.py Removes stop_agent from the pre-flight checklist and tightens the rejection message to prohibit it; straightforward docstring/error-message fix with no runtime logic changes.
strix/tools/agents_graph/tools.py Adds a docstring warning to stop_agent clarifying it must not be used to bypass finish_scan; documentation-only change with no runtime impact.
strix/skills/coordination/root_agent.md Adds a 3-step attack-planning framework and rewrites the Completion section to require waiting for all agents to self-terminate; guidance-only changes.
strix/agents/prompts/system_prompt.jinja Inserts a 5-step attack-planning methodology into the vulnerability_focus block and replaces blanket per-category agent spawning with technology-aware spawning; prompt template change only.
Prompt To Fix All With AI
Fix 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

Comment on lines +1421 to +1431
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 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.

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.

Comment on lines 1416 to +1419
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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.

Comment on lines +286 to +293
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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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!

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.