Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Looks good! No obvious issues found.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary\n\nLooks good! No obvious TODO/FIXME comments found.\n\n---\nReviewed by Hermes Agent
|
Thanks for the focused regression fix. Current main's output-limit terminal returns bypass the The shared helper restores that contract across the targeted paths without mutating conversation history, toolsets, or the system prompt. The added tests cover representative text truncation, truncated tool calls, Codex incomplete-output exhaustion, and provider output-cap rejection. The complete patch also passed Automated hermes-sweeper review. |
62f0833 to
25cf8cb
Compare
dac7d53 to
0c02e0d
Compare
|
Prepared a provenance-only refresh of the exact two-commit series. The previous head |
0c02e0d to
d8194b2
Compare
|
Superseding my earlier provenance-only refresh note: PR #63764 is now rebased and conflict-free at exact head The only textual conflict was the helper insertion point in Verification on the published SHA:
GitHub final state: |
SummaryFourteen PRs address or reference four related failure-accounting and truncation/stream-recovery issues: #38460/#38553 account for truncated-response usage, #38479/#38553/#39115 refund optimistic API-call counts, #45919/#45920/#45922/#45971/#46003/#46016 repair Anthropic partial-stream recovery, and #63764 preserves late steering across terminal exits. #47280/#68041/#73028 address the adjacent empty-stub persistence failure, with #73028 providing the merged implementation. Related pull requests
Duplicates#39115 substantially duplicates #38479; #38553 overlaps both #38479 and #38460 but bundles the two accounting causes. #45920 and #45971 duplicate #45919, while #46003 and #46016 overlap its core fix; #45922 shares that core but differs on empty-recovery semantics. #47280 and #68041 were superseded by merged #73028. Suggested consolidationKeep #63764 open with a salvage path: preserve its shared pending-steer terminal-result contract and focused regressions, consistent with the visible keep_open/high-salvage verdict. For the other clusters, retain #38479's narrow refund path, require #38460/#38553 author action to rebase or split canonical persisted accounting, and keep #45919/#45922 open only long enough to resolve the documented empty-recovery policy while rebasing the chosen implementation around current shared post-processing; the already closed duplicate and superseded PRs can remain closed along the chains above. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I59084(["issue #59084 (closed)"])
P63764["PR #63764 (open)"]
P63764 -->|best fix| I59084
class I59084 closed
class P63764 open
class P63764 best
class P63764 target
click I59084 "https://github.com/NousResearch/hermes-agent/issues/59084"
click P63764 "https://github.com/NousResearch/hermes-agent/pull/63764"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 14 pull requests and 4 issues in this complex. Each diff was read against this issue; Assessment working set: 167 kB of PR diffs, 56 kB of issue/PR text, 26 kB of discussion (38 comments), 21 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
6391822 to
5345f8b
Compare
|
Refreshed onto current |
|
CI follow-up pushed as |
b0e128e to
b05c706
Compare
Seal steer acceptance atomically at terminal result boundaries, preserve rejected CLI and TUI steers as next-turn work, and keep gateway adapter slots distinct from durable FIFO state. Add race, ingress, profile-key, command-provenance, and depth-cap regressions.
b05c706 to
e426eb5
Compare
What does this PR do?
Preserves
/steermessages across terminal turn boundaries instead of losing them in the race between ingress and result finalization.A cached agent now opens a distinct steer-acceptance generation for each
run_conversation()call. Terminal result creation seals that generation and drains its pending steer under the same lock used byAIAgent.steer(). This gives the race two lossless outcomes: a steer that wins the lock is returned inresult["pending_steer"]; a steer that arrives after sealing is rejected by the completed turn so its ingress can preserve it as next-turn work.The contract is complete across the classic CLI, TUI RPC, and messaging gateway:
/steerrejection appends the non-empty payload behind older_pending_inputFIFO work;session.steerrejection queues the payload for the next turn without hard-interrupting the completed run;/stop) retains steer provenance and runs as user input rather than being redispatched as a command;A live overlap search found open PR #67226, but it addresses the TUI
busy_input_mode="interrupt"ingress path and bundles unrelated webhook/session changes. It does not implement this terminal-result ownership contract.The refreshed candidate is the two-commit series headed by
b0e128e85f39890737b912a1da42e862039b393d(treee9aadab974bbd893c560d8fca7b8e8a86fc2a8a9), based directly on frozen upstreammain936dd7346fd7fd8107af1ce7fc019c07c001c1bdon 2026-08-11. The atomic follow-upb0e128e85f39890737b912a1da42e862039b393drepairs the CI fixtures without changing production code.Hard-interrupt returns remain intentionally excluded because
clear_interrupt()discards pending steer by design.codex_app_serveralso remains excluded because it owns its loop and uses nativeturn/steer.Related Issue
Fixes #59084
Type of Change
Changes Made
agent/agent_init.py,run_agent.py_pending_steer_lock;agent/conversation_loop.py,agent/turn_finalizer.pycli.py,tui_gateway/methods_session.py/steerandsession.steerpayloads as next-turn work;gateway/platforms/base.py,gateway/run.py,gateway/slash_commands.py_execute_tool_calls()tests explicitly open a steer generation, while adapter-owned active/pending slots use the adapter's physical key and durable runner state keeps its profile-qualified key;How to Test
Re-run the three files that failed in CI run
31522209927:env -u PYTHONPATH -u VIRTUAL_ENV \ HERMES_PYTHON=/home/jakub/.cache/hermes-pr-audit-venv/bin/python \ scripts/run_tests.sh \ tests/gateway/test_multiplex_busy_input_mode.py \ tests/gateway/test_telegram_voice_v0_regressions.py \ tests/run_agent/test_tool_batch_segmentation.py \ -qExact-tree result: 50 passed, 0 failed, 1 Windows-only skip. The repaired fixtures now exercise the same generation lifecycle and dual-key ownership boundaries as production.
Run those three files together with the prior terminal-steer, gateway FIFO, CLI/TUI ingress, provider-exit, and conflict-side regression lanes:
env -u PYTHONPATH -u VIRTUAL_ENV \ HERMES_PYTHON=/home/jakub/.cache/hermes-pr-audit-venv/bin/python \ scripts/run_tests.sh \ tests/gateway/test_multiplex_busy_input_mode.py \ tests/gateway/test_telegram_voice_v0_regressions.py \ tests/run_agent/test_tool_batch_segmentation.py \ tests/run_agent/test_413_compression.py \ tests/run_agent/test_partial_stream_finish_reason.py \ tests/run_agent/test_run_agent.py \ tests/run_agent/test_run_agent_codex_responses.py \ tests/run_agent/test_steer.py \ tests/gateway/test_terminal_steer_fifo.py \ tests/gateway/test_queue_consumption.py \ tests/gateway/test_steer_fifo_overwrite.py \ tests/gateway/test_busy_session_ack.py \ tests/gateway/test_tts_media_routing.py \ tests/gateway/test_pending_event_none.py \ tests/gateway/test_goal_continuation_drain.py \ tests/gateway/test_goal_max_turns_config.py \ tests/gateway/test_goal_status_notice.py \ tests/gateway/test_active_session_text_merge.py \ tests/gateway/test_run_progress_topics.py \ tests/gateway/test_busy_session_auth_bypass.py \ tests/cli/test_cli_init.py \ tests/test_tui_gateway_server.py \ tests/agent/test_turn_retry_state.py \ tests/hermes_cli/test_copilot_token_exchange.py \ -qExact-tree result: 1071 passed, 0 failed, 2 Windows-only skips across 24 files.
Parse
run_conversation()and classify every direct return.Result: 32 direct returns — 26 shared terminal-helper exits, 4 deliberate hard-interrupt exits, 1 native
codex_app_serverexit, 1 normal finalizer exit, and 0 unexplained.Run static, diff, and attribution hygiene:
env -u PYTHONPATH -u VIRTUAL_ENV \ /home/jakub/.cache/hermes-pr-audit-venv/bin/python -m ruff check \ tests/gateway/test_multiplex_busy_input_mode.py \ tests/gateway/test_telegram_voice_v0_regressions.py \ tests/run_agent/test_tool_batch_segmentation.py /home/jakub/.cache/hermes-pr-audit-venv/bin/python -m compileall -q \ tests/gateway/test_multiplex_busy_input_mode.py \ tests/gateway/test_telegram_voice_v0_regressions.py \ tests/run_agent/test_tool_batch_segmentation.py git diff --check 936dd7346fd7fd8107af1ce7fc019c07c001c1bd scripts/audit_pr_attribution.pyExact-tree result: Ruff passed; compileall passed; diff-check passed; all contributor emails mapped.
Run the Windows-footgun diff scan. It reports two bare
read_text()calls intests/run_agent/test_run_agent_codex_responses.py; both lines already exist unchanged on exact upstream base936dd7346fd7fd8107af1ce7fc019c07c001c1bd, and this candidate introduces neither match.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — N/A; no user-facing configuration or API changescli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AScreenshots / Logs
Not applicable. This is a control-flow and queue-ownership fix with deterministic automated regression coverage.
Caveats
pytest tests/ -qsuite was not rerun. The exact committed tree passed the three formerly red CI files (50 passed) and the combined 24-file focused lane (1071 passed), plus Ruff, compileall, AST return classification, diff hygiene, and attribution audit.31522209927was against parent5345f8b434efd99c284870609509c7557902c327. Its 10 failures were stale direct-seam fixtures now repaired by follow-upb0e128e85f39890737b912a1da42e862039b393d; remote CI for that follow-up awaits a push.codex_app_servercontinues to use its native steering protocol.2026-08-21 current-main refresh
This PR remains necessary: current
maindoes not preserve terminal/steerownership across the reported truncation/finalization race. The series was rebased onto exact upstreammainfc9cbc872d8050c22f1192b16bc5ff4aed471e10; the refreshed candidate ise426eb5e460ea9a262bcbf64feedd6bf68edc8f2. The port preserves current-main profile-scoped adapter busy lanes and makes every producer and consumer derive the adapter slot through the same profile-aware key helper. Final review follow-ups also route/goal resumethrough the transport-owning profile adapter and its physical pending slot, instead of the default adapter namespace.Verified locally on the exact candidate: the 18 changed test files (611 passed, two Windows skips), the focused goal-resume regression (5 passed), Ruff across all changed Python files,
compileall, attribution audit, andgit diff --check. The same lane on the parent before the final focused follow-up passed 1192 tests with two Windows skips. Pytest reports one existing non-fatal_BarrierDB.flush_token_countsthread warning from a test already present unchanged on the pinned base.