Skip to content

fix(agent): nudge past progress-placeholder responses after tool call… - #57610

Open
ithelpm wants to merge 3 commits into
NousResearch:mainfrom
ithelpm:fix/42503-post-tool-placeholder
Open

fix(agent): nudge past progress-placeholder responses after tool call…#57610
ithelpm wants to merge 3 commits into
NousResearch:mainfrom
ithelpm:fix/42503-post-tool-placeholder

Conversation

@ithelpm

@ithelpm ithelpm commented Jul 3, 2026

Copy link
Copy Markdown

…s (#42503)

After executing tool calls, some models (observed cross-provider, most often local/smaller models) close the turn with a short progress note ("Working on it...", "I'll now update the file…") instead of continuing to the next step or delivering the result. The conversation loop treats any non-empty no-tool-call response as the final answer, so the task silently ends unfinished — no error, no hint (#42503).

The existing intermediate-ack continuation deliberately skips turns that already ran tools, and the post-tool EMPTY-response nudge only covers blank content, so this failure mode fell between the two recoveries.

Fix, mirroring the empty-response nudge:

  • detect progress placeholders after a tool round (short, non-question, explicit progress phrase OR future commitment with an open ending)
  • nudge the model to continue, capped at 2 per tool round
  • if the nudges are exhausted, keep the note but mark the turn post_tool_placeholder_response so the turn-completion explainer appends a visible "Incomplete turn" footer instead of ending silently
  • flag the synthetic nudge pair as ephemeral scaffolding so it is popped before the real final response and never persisted

What does this PR do?

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

How to Test

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
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform:

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

  • This skill is broadly useful to most users (if bundled) — see Contributing Guide
  • SKILL.md follows the standard format (frontmatter, trigger conditions, steps, pitfalls)
  • No external dependencies that aren't already available (prefer stdlib, curl, existing Hermes tools)
  • I've tested the skill end-to-end: hermes --toolsets skills -q "Use the X skill to do Y"

Screenshots / Logs

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists labels Jul 3, 2026

@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 targeting the post-tool placeholder gap from #42503; current main still takes non-empty no-tool replies through the normal final-response path (agent/conversation_loop.py:4941-5272), and the existing intent-ack tests explicitly exclude histories where a tool already ran (tests/agent/test_intent_ack_continuation.py:166-174).

Problems

  • agent/conversation_loop.py:4995-5007 adds synthetic scaffold rows, but the cleanup at :5023-5033 removes only a trailing suffix. If the nudge produces another tool call, those rows become interior. agent/turn_finalizer.py:175 saves trajectories before session persistence, and run_agent.py:2034 converts the unfiltered messages, so save_trajectories=True records the supposedly ephemeral rows.

Suggested changes

  • Filter/remove this scaffold from trajectory output as well as session persistence.
  • Add the tool-call-after-nudge regression; the current persistence test patches _persist_session, so it does not cover the buried-scaffold case.

Automated hermes-sweeper review.

assistant_message, "incomplete"
)
interim_msg["_post_tool_placeholder_synthetic"] = True
messages.append(interim_msg)

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.

Blocking: if this nudge leads to another tool call, this flagged pair becomes interior rather than trailing, so the cleanup at lines 5023-5033 cannot remove it. The finalizer saves trajectories before session persistence filtering, so save_trajectories=True records these supposedly ephemeral rows. Filter/remove them from trajectory output and add a tool-call-after-nudge regression.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 15, 2026
ithelpm and others added 2 commits July 16, 2026 11:19
NousResearch#42503)

After executing tool calls, some models (observed cross-provider, most
often local/smaller models) close the turn with a short progress note
("Working on it...", "I'll now update the file…") instead of continuing
to the next step or delivering the result. The conversation loop treats
any non-empty no-tool-call response as the final answer, so the task
silently ends unfinished — no error, no hint (NousResearch#42503).

The existing intermediate-ack continuation deliberately skips turns that
already ran tools, and the post-tool EMPTY-response nudge only covers
blank content, so this failure mode fell between the two recoveries.

Fix, mirroring the empty-response nudge:
- detect progress placeholders after a tool round (short, non-question,
  explicit progress phrase OR future commitment with an open ending)
- nudge the model to continue, capped at 2 per tool round
- if the nudges are exhausted, keep the note but mark the turn
  post_tool_placeholder_response so the turn-completion explainer
  appends a visible "Incomplete turn" footer instead of ending silently
- flag the synthetic nudge pair as ephemeral scaffolding so it is
  popped before the real final response and never persisted

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…trajectories

Review follow-up (NousResearch#57610): the terminal scaffold pop before the final
response only strips a trailing suffix. When the placeholder nudge
succeeds via ANOTHER tool call, the synthetic assistant/user pair gets
buried under the new tool turns, survives in the live context (replayed
to the model on every later call), and — because trajectory conversion
ran on the unfiltered message list — save_trajectories=True recorded the
fake turns as if the model had really said them.

Two-part fix:
- conversation_loop: pop the _post_tool_placeholder_synthetic pair in
  the tool-call branch, before the new assistant tool-call turn is
  appended — the nudge has served its purpose, so the rows never become
  interior in the first place.
- convert_to_trajectory_format: sweep ALL ephemeral scaffolding flags at
  the top, mirroring the per-message filter the session-store flush
  already applies. This makes the "scaffolding never persists" invariant
  structural for trajectories too (same philosophy as NousResearch#57491's terminal
  sweep) instead of depending on every loop path cleaning up positionally.

Adds the tool-call-after-nudge regression the review asked for: one
end-to-end loop test asserting no buried synthetic rows remain, and one
direct conversion test asserting an interior scaffold pair never reaches
trajectory output.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ithelpm
ithelpm force-pushed the fix/42503-post-tool-placeholder branch from 07d2186 to 4d17ac5 Compare July 16, 2026 03:34
@ithelpm

ithelpm commented Jul 16, 2026

Copy link
Copy Markdown
Author

Thanks for the catch - the trailing-suffix pop indeed missed the case where the nudge succeeds via another tool call, leaving the synthetic pair buried under the new tool turns and leaking into save_trajectories=True output.

Addressed in the follow-up commit (branch also rebased onto current main):

  • Tool-call branch pop (agent/conversation_loop.py): the _post_tool_placeholder_synthetic pair is now popped in the tool-call path, before the new assistant tool-call turn is appended, so the rows can never become interior in the first place and also stop being replayed to the model in the live context.
  • Structural trajectory sweep (agent/agent_runtime_helpers.py::convert_to_trajectory_format): all _EPHEMERAL_SCAFFOLDING_FLAGS rows are filtered at the top of the conversion, mirroring the per-message filter the session-store flush already applies - same philosophy as Rotation compression: _db_persisted marker copied via messages[i].copy() skips bulk flush to child session (cached gateway regression since #50372) #57491's terminal sweep, so trajectory hygiene no longer depends on every loop path cleaning up positionally. This also covers the pre-existing empty-response/prefill scaffolds if they ever get buried the same way.
  • Regression tests: test_tool_call_after_nudge_leaves_no_buried_scaffolding runs the tools -> placeholder -> nudge -> tools -> answer sequence end-to-end and asserts no synthetic rows survive in the message list; test_trajectory_conversion_drops_buried_scaffolding feeds an interior scaffold pair directly into the trajectory conversion (no _persist_session patching involved) and asserts it never reaches the output.

Self-review follow-up to the buried-scaffold fix: the tool-call branch
cleaned prefill rows and placeholder-nudge rows with two sequential
single-flag loops. When the recoveries interleave — tool round →
thinking-only response (prefill row) → progress placeholder (nudge
pair) → tool calls — the stack is [prefill, placeholder, nudge]: the
prefill loop sees the nudge on top and no-ops, the placeholder loop
then pops the pair and only NOW exposes the prefill row, which its
loop can no longer reach. The stranded row replays as a fake assistant
turn (and a consecutive-assistant pair) in the live context on every
later call. Reproduced empirically before the fix.

Merge the two pops into one loop over both flags, mirroring the
combined terminal pop on the final-response path; _had_prefill is set
only when a prefill row is actually popped, so the retry-counter reset
semantics are unchanged. Adds the interleaved-recovery regression.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ithelpm

ithelpm commented Jul 16, 2026

Copy link
Copy Markdown
Author

Self-review addendum: the tool-call-path cleanup had an interleaving gap. With tool round -> thinking-only response (prefill row) -> progress placeholder (nudge pair) -> tool calls, the stack is [prefill, placeholder, nudge] and two sequential single-flag pop loops strand the prefill row beneath the popped nudge pair - it then replays as a fake assistant turn (and a consecutive-assistant pair) in the live context. Reproduced empirically before fixing. This gap only exists because this PR's nudge pair can sit on top of a prefill row, so it is fixed here rather than in a separate PR.

Fixed in 2d541ee by merging the pops into one loop over both flags, mirroring the combined terminal pop on the final-response path (_had_prefill is only set when a prefill row is actually popped, so the retry-counter reset semantics are unchanged). Added test_interleaved_prefill_and_placeholder_scaffolds_all_popped covering the exact sequence.

@yingliang-zhang

Copy link
Copy Markdown
Contributor

Draft Comment for PR #57610

Status: DRAFT — needs user review before posting


Thanks for this PR — it addresses a real gap (#42503) that we've also been working on locally.

We have a complementary implementation that covers an adjacent failure mode not addressed by the progress-placeholder detection in this PR:

Signature A — lost tool-call preamble. When the model emits a short text ending with : or (e.g. "接下来读取文件:") with finish_reason=stop and no tool calls, right after a tool round completed. This is distinct from the progress-placeholder case (#57610's scope) — it's a truncated tool-call narration where the tool_calls array was lost. The upstream _dropped_toolcall_nudge path only covers finish_reason=tool_calls with an empty tool_calls array, not finish_reason=stop with a text preamble.

CJK intent marker corpus. Our implementation includes ~30 CJK intent markers (继续/接下来/下一步/然后/开始/...) with done/wait/question exclusion lists that were production-hardened over several weeks of testing with K3 and GLM models. These could extend #57610's English-only phrase detection.

What we'd like to contribute:

  1. The Signature A detection (colon-preamble + tool-round gate) as a follow-up to this PR
  2. The CJK marker corpus + exclusion lists as a locale extension
  3. A force_display parameter on _emit_interim_assistant_message (fixes the streaming-buffer overwrite issue from the closed fix: persist and emit agent response during verification stop loop #62676 — ensures verify-on-stop candidates survive as permanent UI bubbles on messaging gateways)

What we won't do:

  • Submit a competing PR — we'd like to coordinate with this PR as the merge vehicle
  • Scope-creep this PR — we'll file as a separate follow-up after merge

Would this sequencing work for you? Happy to share the code or more details if useful.

yingliang-zhang added a commit to yingliang-zhang/hermes-agent that referenced this pull request Aug 16, 2026
…ool calls

When an LLM produces finish_reason=stop with text indicating intent to
continue (a colon-preamble that lost its tool_calls, or a narrated
continuation) right after a tool round, the conversation silently ends
without executing the intended tool call (NousResearch#42503).

Extract the inlined detection logic into a dedicated agent/false_stop.py
module following the verification_stop.py / kanban_stop.py pattern:

- build_false_stop_nudge(): pure function returning nudge text or None
- false_stop_detection_enabled(): config + env gate, mirrors
  verify_on_stop_enabled() with surface-aware "auto" default
- Module-level marker constants (24 CJK + 13 English intent markers,
  done/wait/question exclusion guards) — previously rebuilt per iteration

Two detection signatures:
- Signature A: short colon-preamble (<120 chars) after a tool round
- Signature B: narrated continuation (≤200 chars, CJK ending, intent
  markers, no done/wait/question markers) after a tool round

Both gated by _was_in_tool_round (last 8 messages). Bounded to 2
nudges per turn, resets on tool round and genuine completion. Synthetic
nudge flagged _false_stop_synthetic in _EPHEMERAL_SCAFFOLDING_FLAGS
for transcript hygiene.

Complementary to intent_ack_continuation (turn-start, not post-tool)
and NousResearch#57610 (progress-placeholder text).

Config: agent.false_stop_detection: "auto" (on for CLI/TUI/desktop/
codex/local, off for messaging platforms). Env: HERMES_FALSE_STOP_DETECTION.

Tests: 39 tests in 5 categories (gating, Signature A, Signature B,
budget/reset, transcript hygiene). No regressions on verification_stop,
verification_stop_caching, or verification_continuation_budget.
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 P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants