fix: prevent /stop signal loss and empty provider credential corruption (two deep bugs) - #60018
Closed
isheng-eqi wants to merge 19 commits into
Closed
fix: prevent /stop signal loss and empty provider credential corruption (two deep bugs)#60018isheng-eqi wants to merge 19 commits into
isheng-eqi wants to merge 19 commits into
Conversation
…usResearch#58774) _restore_or_build_system_prompt unconditionally restored the session-DB stored prompt when it matched the current runtime identity, even when the caller set an explicit ephemeral_system_prompt (e.g. /personality). Check ephemeral_system_prompt before the stored-prompt fast path so a deliberate personality switch takes effect immediately instead of being silently ignored until the next fresh session.
…nto fix/58774-personality-ephemeral
…ibuteError (NousResearch#59845) The Copilot x-initiator injection block calls agent._is_copilot_url() without a getattr guard, unlike the sibling _is_user_initiated_turn check one line above. On some agent construction paths (module-reload, wrapper agents) _is_copilot_url may be missing, causing every API call in the conversation to fail with AttributeError and the cron job to error out. Wrap the call with getattr(agent, '_is_copilot_url', lambda: False)() so non-Copilot and partially-initialized agents fall through cleanly. Github-Issue:NousResearch#59845
…nt delivery The TUI notification poller (_notification_poller_loop) only watched process_registry.completion_queue, never polling kanban_notify_subs. Kanban task subscriptions with platform='tui' were therefore never delivered — the gateway's _kanban_notifier_watcher has no TUI adapter, and the TUI poller had no kanban polling logic. Add _poll_kanban_task_events() which mirrors the gateway watcher's pattern: list kanban_notify_subs for the session, claim unseen terminal events via kanban_db.claim_unseen_events_for_sub(), and emit status.update messages to the TUI session. Polled every ~5 seconds on the existing completion_queue.get() timeout path. Github-Issue:NousResearch#59960
…e kind The kanban_block tool schema documents all four block kinds and says 'kind' is optional, but goal_mode tasks silently rejected an omitted (or capability/transient) kind. This broke workers that followed the published schema contract. Two changes: 1. Update KANBAN_BLOCK_SCHEMA kind description to document the goal_mode restriction (only dependency/needs_input accepted, omit→needs_input). 2. Coerce kind=None to 'needs_input' in the goal_mode gate so workers that follow the schema's optional-kind contract don't get a hard error. capability/transient are still rejected for goal_mode. Github-Issue:NousResearch#59764
…ace partial skips When a kanban worker is spawned with --skills and ALL named skills are missing from the assignee profile, the CLI raised ValueError, causing the worker process to die. The dispatcher retried → crash-loop until the failure breaker gave up. Fix: when HERMES_KANBAN_TASK is set and all skills are missing, call kanban_block with a structured 'capability' error instead of raising. The task is blocked with a human-readable reason, no retry loop. Additionally, when only SOME skills are missing (graceful degradation path), add a kanban_comment so the card author can see the skip on the board instead of it being hidden in the worker log file. Github-Issue:NousResearch#59764
…viders _try_openrouter() and _try_nous() used ttl=60 when no credentials were configured, treating a permanent configuration state as a transient payment error. After 60s the mark expired and the provider was retried → failed identically → logged another WARNING. This flooded errors.log: on a session with only DeepSeek configured, 1,582 'marking unhealthy' and 791 'Nous unavailable' WARNINGs drowned out the 16 actual ERRORs. Use the default _AUX_UNHEALTHY_TTL_SECONDS (600s) when the provider is unavailable due to missing credentials rather than a transient payment/rate-limit error. The 60s ttl is preserved for genuine transient failures at the other call sites. Github-Issue:NousResearch#59984
whatsapp_cloud was present in _HOME_TARGET_ENV_VARS and the Platform enum, but missing from _KNOWN_DELIVERY_PLATFORMS. Cron jobs targeting whatsapp_cloud as a delivery platform would fail _is_known_delivery_platform() validation despite having a valid gateway adapter and home-target env var. Github-Issue:NousResearch#59988
…l_lines read_file reported inaccurate total_lines because wc -l counts newline characters, not actual lines. A file with N content lines and no trailing newline has N-1 newlines, so wc -l returns N-1. Replace wc -l with a Python one-liner that uses universal newline reading, which counts lines correctly regardless of trailing-newline convention. Github-Issue:NousResearch#59999
…rch tools Three guards against malformed tool inputs found through boundary testing: 1. write_file / patch: reject paths containing ASCII control characters (U+0000-U+001F) which can cause path-injection (the OS silently rewrites the filename, and the tool reports success for a different path than intended). 2. search_tool: reject empty-string pattern which ripgrep interprets as 'match every position', returning all content in the directory. 3. patch_tool: reject null bytes (\x00) in old_string / new_string. Null bytes corrupt files and break downstream tools that expect text content.
read_log() used offset=0 as a sentinel for 'show last N lines', making it impossible to explicitly request reading from the first line. Change the default to None: offset=None means 'last N lines', offset=0 means 'start from line 0'. Update the process() caller to pass offset=None (via args.get without a 0 default) when no offset is specified by the caller.
Two boundary-case fixes: 1. process_registry wait(): reject timeout <= 0 explicitly instead of silently coercing it to the default max_timeout. The schema declares minimum=1 but the handler didn't enforce it. 2. file_operations read(): when offset exceeds total_lines, return a clear hint instead of a misleading line-number prefix with empty content (e.g. '100|' for a 5-line file).
Two deep bugs found through systematic analysis of the streaming API call and fallback credential subsystems: 1. Interrupt signal loss (chat_completion_helpers.py): When the worker thread exits before the main thread's poll loop checks the interrupt flag (e.g. _call_anthropic() detects the flag and returns None), the while loop exits normally and the InterruptedError is never raised. /stop is silently swallowed. Fix: re-check _interrupt_requested after the while loop exits. 2. Empty provider bypasses credential guard (agent_runtime_helpers.py): recover_with_credential_pool() guards against cross-provider pool swaps with 'if current_provider and pool_provider and current != pool_provider'. When agent.provider is '' (valid unset state from agent_init.py:326), current_provider is falsy, the guard is skipped, and the pool swaps credentials onto an agent with empty provider. This is the root cause of the 'provider= model=' empty-string error. Fix: only skip the guard when pool_provider is empty (unscoped pool), not when agent provider is empty.
alt-glitch
added
type/bug
Collaborator
Omnibus PR bundling ~8 distinct fixes across subsystems; the title ("two deep bugs") understates the scope. Verified via Not a duplicate of #59363 — this is a broader competing approach that re-includes that fix plus 7 others. Related for reviewer navigation: #59363, #58774, #59764, #59845, #59960, #59999. A reviewer should decide whether to take the omnibus or the focused per-issue PRs; the bundled scope makes review harder. |
…gger The provider-mismatch guard now checks pool_provider and current_provider != pool_provider. MagicMock.provider returns a truthy child mock by default, which would trigger the guard and skip the pool recovery tests. Set pool.provider='' explicitly.
Collaborator
|
Thanks @isheng-eqi — both bugs here are real and well-diagnosed. Salvaged into #60120 (rebased onto current Two notes on what changed in the salvage:
Closing in favor of #60120. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two high-severity bugs found through systematic deep analysis of the streaming API call and credential fallback subsystems.
Bug 1: /stop silently swallowed — interrupt signal lost
Location:
agent/chat_completion_helpers.py,_interruptible_streaming_api_callRoot cause: When the worker thread exits before the main thread's poll loop checks the interrupt flag (e.g.
_call_anthropic()detects_interrupt_requestedand returnsNone), the while loop exits normally andInterruptedErroris never raised. The /stop signal is silently swallowed.Impact: User types /stop but the agent doesn't actually stop — it continues processing or returns silently with no response.
Fix: Re-check
agent._interrupt_requestedafter the while loop exits, so the interrupt is raised even when the worker thread self-terminated.Bug 2: Empty provider bypasses credential guard → corrupted agent state
Location:
agent/agent_runtime_helpers.py,recover_with_credential_pool()line 730-732Root cause: The guard
if current_provider and pool_provider and current != pool_provideruses Python truthiness. Whenagent.provideris""(valid unset state fromagent_init.py:326),current_provideris falsy, the guard is skipped. The pool swaps credentials (base_url/api_key) onto the agent without fixing the empty provider — leaving the agent in a permanently corrupted state withprovider="" model="".Impact: This is the root cause of the
provider= model=empty-string error seen in user logs. API calls fail with "you passed ." errors because the model name is empty.Fix: Only skip the guard when
pool_provideris empty (unscoped pool), not when agent provider is empty. Empty agent provider is treated as a mismatch because swapping credentials would corrupt the agent.Changes
agent/chat_completion_helpers.py: add post-worker interrupt checkagent/agent_runtime_helpers.py: fix provider guard to cover empty agent provider