fix: agent loop stability — stale reasoning, vision API key, TUI prompt - #25404
fix: agent loop stability — stale reasoning, vision API key, TUI prompt#25404LifeJiggy wants to merge 3 commits into
Conversation
|
I think the strict-vision OpenRouter path still drops the explicit API key. This PR teaches _resolve_strict_vision_backend() about explicit_api_key, but the strict vision call sites still appear to call it without the resolved key for OpenRouter/auto fallback. That means AUXILIARY_VISION_API_KEY or credential-pool resolved keys may never reach _try_openrouter() in those paths. Could we thread explicit_api_key=resolved_api_key through the strict vision OpenRouter call sites and add a focused regression test for that path? |
|
The reviewer's concern about the vision API key not reaching _try_openrouter() is addressed — all 3 call sites in resolve_vision_provider_client() now pass explicit_api_key=resolved_api_key from the task-level resolved credentials. |
|
@teknium1 PTAL |
|
@egilewski pTAL |
Tests are failing. And, I'm sorry, but my tokens are limited, so for now I'm focusing on the most important for me security issues. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the strict-vision credential path. One part remains useful on current main, but this branch now mixes it with two superseded fixes.
Problems
- The test edits at
tests/agent/test_auxiliary_client.py:2218only update mock signatures. They do not assert that a non-null resolved vision key reaches strict OpenRouter routing. Current main still drops that key:_resolve_strict_vision_backend()calls_try_openrouter(model=model)atagent/auxiliary_client.py:5215, whileresolve_vision_provider_client()has already resolved the task key at line 5274. - The stale-reasoning behavior is already fixed in
agent/turn_finalizer.py:382-397by ancestor commitefe1cb00c88234ab4c81055a8aac07689a315508; the destructive-confirmation path is already app-loop marshaled atcli.py:7579-7598by ancestor commit458a94e42568b332e8794ca8fbb8c8e1279160a3.
Suggested changes
- Salvage the strict-vision key forwarding only, and add a focused regression that proves the resolved key—not
OPENROUTER_API_KEY—reaches OpenRouter for strict vision routing.
Automated hermes-sweeper review.
|
Hi, thanks for the review feedback! This push addresses all raised concerns: Strict-vision key forwarding (auxiliary_client.py:3737):
Test coverage:
What was intentionally removed (stale/superseded):
|
…backend to OpenRouter Reconstruct fix/agent-loop-stability on clean main. The branch had stale wholesale cli.py/run_agent.py changes from an old codebase that caused merge conflicts. Salvaged only the core fix: Bug: _resolve_strict_vision_backend accepted explicit_api_key but called _try_openrouter(model=model) without forwarding it, so the resolved key was silently dropped and the env-var OPENROUTER_API_KEY fallback was used. Fix: forward explicit_api_key to _try_openrouter in all call paths. Add explicit_api_key param to _resolve_strict_vision_backend, update all callers in resolve_vision_provider_client to pass resolved_api_key. Regression test: TestStrictVisionKeyForwarding (4 tests) proves the resolved key reaches OpenRouter, not the env var. Enhancements: - Debug logging when resolved key is forwarded (len audit trail) - Debug logging when no backend matches a provider - End-to-end flow test from resolve_vision_provider_client through to _try_openrouter with a resolved key Also fixes test mocks for _resolve_strict_vision_backend signature (test_kimi_coding_skipped_falls_through_to_openrouter, test_kimi_coding_cn_skipped_too).
9f38066 to
8013c2a
Compare
|
@alt-glitch Conflicts resolved. The branch was rebased onto clean main — the stale cli.py and run_agent.py wholesale changes from the old codebase were dropped (those issues are already resolved by ancestor commits on main). Salvaged only the core fix: _resolve_strict_vision_backend now forwards explicit_api_key to _try_openrouter in all call paths, with debug logging. All 3 callers in resolve_vision_provider_client pass resolved_api_key. 4 regression tests (TestStrictVisionKeyForwarding) + 2 existing test mocks updated for the new signature. 303/304 pass (1 flaky timing test under xdist parallelism, pre-existing). |
…nto fix/agent-loop-stability
…signature
test_explicit_provider_override_still_wins asserts
called_once_with('nous', None) but we now pass
explicit_api_key=None as a kwarg. Update assertion to match.
|
The 2 CI failures in slices 2/8 and 7/8 are pre-existing on main — they're Windows path handling bugs in tests/agent/test_image_routing.py::TestExtractImageRefs (7 tests fail identically on clean main without our changes). None are related to this PR. Verified locally: all 327 tests across test_auxiliary_client.py + test_auxiliary_main_first.py pass (including the 4 new TestStrictVisionKeyForwarding tests and the 1 updated mock assertion). The test_image_routing.py failures are a separate issue — the tests expect POSIX-style paths on Windows. Happy to fix those in a follow-up if needed |
What does this PR do?
Three independent bug fixes improving agent loop reliability and CLI UX:
Stale reasoning no longer leaks across turns (Bug: stale reasoning reused when current turn has no reasoning_content #17052). When the current assistant response has no reasoning field (e.g. a simple "hello" reply after a complex tool-calling turn), the display now correctly shows no reasoning. Previously the code searched backwards for the most recent non-None reasoning, which picked up the tool-call step's reasoning from earlier in the same turn — causing confusing stale display.
Vision backend respects runtime API keys ([Bug]: resolve_provider_client ignores explicit_api_key when calling _try_openrouter() #18338). _resolve_strict_vision_backend() called _try_openrouter() without forwarding explicit_api_key, so the vision pipeline always fell back to OPENROUTER_API_KEY env var even when resolve_provider_client was called with a credential-pool key. Added the parameter and pass-through.
/clear//new//reset confirmation no longer drops keystrokes ([Bug]: /clear, /new, /reset, /undo confirmation prompt cannot be answered — keystrokes leak into chat composer #22958). _prompt_text_input() used run_in_terminal(input()) which fought prompt_toolkit's stdin ownership — keystrokes leaked into the agent composer instead of the confirmation prompt. Replaced with prompt_toolkit.prompt() which properly cooperates with the TUI event loop.
Related Issue
Fixes #17052, #18338, #22958
Type of Change
Changes Made
run_agent.py — Changed last_reasoning loop from if msg.get("role") == "assistant" and msg.get("reasoning") to if msg.get("role") == "assistant": last_reasoning = msg.get("reasoning"). Stops at the last assistant message; uses None when the last turn has no reasoning.
agent/auxiliary_client.py — Added explicit_api_key: str = None parameter to _resolve_strict_vision_backend() and forwarded it to _try_openrouter(explicit_api_key=...).
cli.py — Replaced run_in_terminal(_ask) with prompt_toolkit.shortcuts.prompt() in _prompt_text_input() when TUI is active. Raw input() fallback preserved for non-TUI modes (unit tests, non-interactive calls).
How to Test