Skip to content

fix(tools): drop a materialized foreground terminal heartbeat - #119203

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-119196
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-119196

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

Providers that materialize every advertised tool-schema property send heartbeat=60 (the schema minimum) on ordinary foreground terminal calls. The dispatch wrapper refused those calls before execution ("notify/heartbeat only apply to background commands..."), so the model looped on the same rejected call — or followed the error's background=true suggestion and flooded short commands with completion notifications (#119196).

heartbeat only rides a tracked background process's completion delivery path; a foreground value has no execution meaning. This PR normalizes it away on foreground calls instead of rejecting: the materialized full-schema shape (background=false, notify=false, heartbeat=60, pty=false) now executes once in the foreground with no tracked process and no notification arming. The teaching rejection for notify/watch_patterns/notify_on_complete on foreground calls is preserved (those are opt-in notification intent, not mechanically materialized values), and background heartbeat behavior is untouched.

Related Issue

Fixes #119196

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/terminal_tool.py: in _handle_terminal, drop heartbeat to 0 when background is false instead of including it in the foreground rejection condition; update the rejection message (now "notify only applies...") and the surrounding comment to match.
  • tests/tools/test_process_heartbeat.py: rework the dispatch invariant test — the full materialized foreground shape now must execute once with heartbeat=0/notify_on_complete=False/no watch_patterns, while the background heartbeat → notify_on_complete=True mapping is unchanged.

How to Test

  1. Deterministic validator repro from the issue (no live model needed):
import json
from tools.terminal_tool import _handle_terminal
d = json.loads(_handle_terminal({"command": "pwd", "background": False, "notify": False,
                                 "heartbeat": 60, "pty": False, "timeout": 20}))

Before: d["error"] is the "notify/heartbeat only apply to background commands" validation error. After: d["error"] is None, d["exit_code"] == 0 — the command executes once in the foreground (observed result: exit_code=0, output=/private/tmp/...).
2. Foreground notify=True is still refused with the corrected-call teaching error (observed: "notify only applies to background commands (foreground results return directly). Either drop it, or run as terminal(command=..., background=true, notify=...).").
3. Background heartbeat is unchanged: {"command": "sleep 1", "background": True, "heartbeat": 120} spawns a tracked process (observed: session_id=proc_...) and the dispatch test asserts heartbeat=120 + notify_on_complete=True.
4. pytest tests/tools/test_process_heartbeat.py tests/tools/test_notify_on_complete.py tests/tools/test_watch_patterns.py tests/tools/test_terminal_tool.py -q → 60 passed, 1 skipped (linux-only case).
5. pytest tests/tools/ -k terminal tests/tools/test_code_execution.py -q → 395 passed, 2 skipped; pytest tests/tools/test_code_execution.py -q → 44 passed. ruff check on both changed files passes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (full terminal/process/code-execution surfaces in tests/tools/ as listed under How to Test)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS (darwin 25.4, Apple Silicon)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (comment updated in place)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide (pure argument normalization, no platform-specific code)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A (schema already says "With background=true:"; the handler-side normalization is the fix)

Providers that materialize every advertised property send
heartbeat=60 (the schema minimum) on ordinary foreground terminal
calls; the handler refused them, looping the model on a validation
error (or baiting it into background=true for short commands).
Heartbeat only rides a tracked background process, so a foreground
value has no meaning — normalize it away and keep the teaching
rejection for notify/watch_patterns.

Fixes NousResearch#119196
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management duplicate This issue or pull request already exists labels Sep 22, 2026
@liuhao1024

Copy link
Copy Markdown
Author

Withdrawing this PR in favor of #119201, which was opened ~6 minutes earlier and implements the same mechanism: dropping heartbeat from the foreground refuse-condition and normalizing it to heartbeat = 0 for foreground dispatch (plus the same invariant-test flip). At the mechanism level this PR has no unique delta, and with #119202 also live (reporter's schema-level fix), keeping a third identical-mechanism PR open only adds review burden.

Two small deltas from my verification pass that #119201 may want to absorb:

  1. Error message: after this change heartbeat is no longer part of the foreground refusal, so the message "notify/heartbeat only apply to background commands..." is now stale — heartbeat can no longer trigger it. I updated it to "notify only applies to background commands..." and confirmed the foreground notify=True refusal still asserts against "background" in tests.
  2. Test assertions: the materialized-shape dispatch test can additionally assert pty=False / timeout=20 survive dispatch and that no watch_patterns key leaks into the captured call, which pins the full schema-materialization shape rather than just the heartbeat/notify pair.

For the maintainers: #119202 (schema minimum: 0 + default: 0) and the handler-side tolerance in #119201 are complementary rather than conflicting — the schema change stops providers from materializing heartbeat=60 in the first place, while the handler-side change stays robust for any provider that still materializes defaults. Either way, one of the two earlier PRs covers this issue; mine adds nothing beyond the notes above.

Thanks @KoNit-K and @tmaarcxs for the fast turnaround.

@liuhao1024

Copy link
Copy Markdown
Author

Closing in favor of #119201 (earlier, same mechanism). Notes for the maintainers are in my comment above.

@liuhao1024 liuhao1024 closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: terminal heartbeat schema causes foreground validation loops and notification flooding

2 participants