Skip to content

fix(agent): defer background review during TUI turns - #36187

Open
OmarB97 wants to merge 1 commit into
NousResearch:mainfrom
OmarB97:fix/background-review-idle-gate
Open

fix(agent): defer background review during TUI turns#36187
OmarB97 wants to merge 1 commit into
NousResearch:mainfrom
OmarB97:fix/background-review-idle-gate

Conversation

@OmarB97

@OmarB97 OmarB97 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Why

Phone/TUI users can send the next foreground turn immediately after a response. Background memory/skill review currently starts immediately after the prior turn, so local model sessions can end up running a self-review fork and the user's next turn at the same time. On local dflash/taro this showed up as slow or abruptly stalled phone sessions.

What changed

  • Track a foreground-turn generation counter on each agent.
  • Pass that generation into background review.
  • Give TUI sessions a 60s idle window before starting background review.
  • Cancel the review fork before the model call if another foreground turn starts during that idle window.
  • Keep non-TUI behavior unchanged unless HERMES_BACKGROUND_REVIEW_IDLE_DELAY_SECONDS is explicitly set.

Verification

  • python3 -m py_compile run_agent.py agent/agent_init.py agent/conversation_loop.py agent/background_review.py tests/run_agent/test_background_review.py
  • python3 -m pytest -o addopts='' tests/run_agent/test_background_review.py tests/run_agent/test_background_review_cache_parity.py tests/run_agent/test_background_review_toolset_restriction.py -q
  • python3 -m pytest -o addopts='' tests/run_agent/test_codex_app_server_integration.py::TestRunConversationCodexPath::test_background_review_skill_trigger_fires_above_threshold tests/run_agent/test_codex_app_server_integration.py::TestRunConversationCodexPath::test_background_review_signature_never_breaks -q
  • git diff --check

Notes

This keeps background review as best-effort self-improvement, but stops it from racing the operator in interactive TUI sessions.

@OmarB97

OmarB97 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Fork mirror PR: OmarB97#59

@OmarB97

OmarB97 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Replacement fork mirror PR: OmarB97#60 (PR #59 was closed because it conflicted with fork main).

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels Jun 1, 2026
@OmarB97

OmarB97 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Replacement fork mirror PR is open and mergeable: OmarB97#60

Note: the earlier fork PR #59 was closed because it was based on upstream/main and conflicted with the fork mirror branch history. #60 is rebuilt from fork main and cherry-picks the same fix.

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — automated review passed. No security, quality, or test coverage issues detected.

@OmarB97
OmarB97 force-pushed the fix/background-review-idle-gate branch from 89a082a to c3abf0c Compare June 9, 2026 23:16
OmarB97 pushed a commit to OmarB97/hermes-agent that referenced this pull request Jun 10, 2026
Rebased onto upstream/main as a single commit to refresh PR NousResearch#36187.
@OmarB97
OmarB97 force-pushed the fix/background-review-idle-gate branch from c3abf0c to 4f318c5 Compare June 10, 2026 00:10
OmarB97 added a commit to OmarB97/hermes-agent that referenced this pull request Jun 15, 2026
fix(agent): defer background review during TUI turns (fork-ship NousResearch#36187)
@OmarB97
OmarB97 force-pushed the fix/background-review-idle-gate branch from 4f318c5 to 686a585 Compare July 5, 2026 18:48
@OmarB97

OmarB97 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @mxnstrexgl.

Status as of 2026-07-11: the branch has been kept current — refreshed onto upstream/main on 2026-07-05 (686a5859, conflict resolution only, no behavior change) and GitHub still reports it mergeable against today's main. I also re-verified current main still has no idle gate in agent/background_review.py (_run_review_in_thread launches the review regardless of foreground turn state), so the fix here — the foreground-turn generation check plus idle delay before the review model call — is still needed as-is.

Ready for maintainer merge whenever it fits the queue.

@teknium1 teknium1 left a comment

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.

Thanks for the focused race fix. Current main does start the review immediately after a completed response (agent/turn_finalizer.py:500-508) and the fork reaches review_agent.run_conversation() without a foreground-state gate (agent/background_review.py:830-846), so the generation-based cancellation is addressing a real TUI problem. Standalone and embedded TUI sessions are tagged "tui" by tui_gateway/server.py:2116-2139.

Problems

  • agent/agent_init.py:462 in this diff adds HERMES_BACKGROUND_REVIEW_IDLE_DELAY_SECONDS. AGENTS.md:102-107 requires non-secret behavioral settings, including timeouts, to live in config.yaml, not a new HERMES_* environment variable.

Suggested changes

  • Put the override in DEFAULT_CONFIG and document it; hermes_cli/config.py:1723-1730 already owns auxiliary.background_review settings.
  • Add an initialization-path test for the TUI default/config propagation. The added tests manually set the private delay field, so they do not verify the platform-derived 60-second default.

Automated hermes-sweeper review.

Comment thread agent/agent_init.py
agent._foreground_turn_generation = 0
agent._background_review_idle_delay_seconds = 60.0 if platform == "tui" else 0.0
try:
_bg_idle_raw = os.getenv("HERMES_BACKGROUND_REVIEW_IDLE_DELAY_SECONDS")

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.

HERMES_BACKGROUND_REVIEW_IDLE_DELAY_SECONDS is a new non-secret behavioral configuration input. AGENTS.md:102-107 requires timeouts and feature settings to use config.yaml; move this override into DEFAULT_CONFIG (and document it) rather than reading a new environment variable directly.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@GaelicThunder

Copy link
Copy Markdown

Supporting data point from a single-slot local rig (GB10, DeepSeek-V4-Flash via a ds4 fork, one FIFO slot on :30001): we hit the same contention this PR addresses, but from sources the foreground-turn generation counter can't see — the review fork of one session racing other sessions/platforms and non-Hermes consumers of the same engine (observed as unexplained full-context replays at 03:14/04:12 during an overnight autonomous session).

Filed #80215 with measurements and a complementary engine-level probe (localhost /metrics: vllm:num_requests_running/waiting, ds4 gauges; None → never gated, cloud unchanged) that composes with the idle-delay + generation check here rather than replacing it. Happy to rebase that on top of this PR if it lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants