Skip to content

fix(cron): stop misclassifying valid final responses as failures - #1

Open
trevornk wants to merge 4334 commits into
mainfrom
fix/cron-false-failure-signaling
Open

trevornk wants to merge 4334 commits into
mainfrom
fix/cron-false-failure-signaling

Conversation

@trevornk

Copy link
Copy Markdown
Owner

Bug (kanban t_da02e022)

The Daily Quant Platform Dev Agent cron job (e099c871a5bf) reported a
"provider authentication error" in #alerts-oracle on 2026-07-10, but the
job had actually completed a full, valuable work cycle (arb-platform PR NousResearch#48
— a critical security fix for an unauthenticated public trading-control API).
The RuntimeError was just wrapping the successful report text.

Confirmed the same false-failure pattern in at least 3 other historical runs
(2026-07-03, 2026-07-06, 2026-07-07) via cron/output/e099c871a5bf/*.md —
scope is real, not a one-off.

Root cause

  1. A verify-on-stop / pre_verify nudge can continue the agent loop
    after a valid no-tool-call final_response has already been produced.
  2. If that continue lands exactly when the iteration budget is exhausted,
    the while loop's own condition (not an explicit in-body break) ends
    the turn — so _turn_exit_reason never advances off its "unknown"
    initializer, even though final_response holds a real, complete answer.
  3. turn_finalizer.finalize_turn only sets completed=True when
    turn_exit_reason starts with "text_response(" (or exit happened under
    max_iterations) — so this falloff produced completed=False with valid
    content.
  4. cron/scheduler.py then does raise RuntimeError(final_response_text)
    for any completed=False result, so the real report gets delivered to
    Discord dressed up as a failure.

Fix

Right before finalize_turn is invoked in conversation_loop.py: if
_turn_exit_reason is still "unknown", the turn wasn't failed/
interrupted, and final_response has real content (not the "(empty)"
sentinel), relabel the exit reason as a normal text_response(...)
completion. Genuine failure paths (budget_exhausted with no content,
interrupted, API error, empty_response_exhausted) are untouched — they
either set failed=True, leave final_response empty, or already claim a
specific _turn_exit_reason before this point runs.

Verification

  • python3 -c "import ast; ast.parse(open('agent/conversation_loop.py').read())" — syntax OK
  • Reviewed all call sites that set _turn_exit_reason to confirm this
    fallback only fires on the previously-unhandled silent-falloff case, not on
    any already-classified path.

Not touched

Does not change GitHub Actions billing block on trading-platform CI
(separate, tracked, expected to clear on its own).

simplast and others added 30 commits July 7, 2026 13:29
Salvage follow-up integrating PR NousResearch#30481 (@simplast) and PR NousResearch#57683
(@catbearlove1-lang) into the unified export surface:

- --format html: standalone self-contained HTML transcript (single
  session or multi-session with sidebar), works with all shared filters
  and --redact; requires a file output path.
- --only user-prompts: prompt-only export (jsonl records or md sections)
  via the shared session_export renderer; the separate export-prompts
  subcommand from the original PR is subsumed by this flag.
- AUTHOR_MAP entries for both contributors; docs EN + zh-Hans.
Opt-in discord.approval_mentions (config.yaml, bridged to
DISCORD_APPROVAL_MENTIONS) prepends <@id> mentions for numeric
allowlist entries to exec-approval prompts, with a scoped
AllowedMentions override (users only). Default off - no surprise
pings. Reapplied onto the content-mirror layout from NousResearch#60245: mentions
prepend to the visible content block and its truncation budget.

Original implementation from PR NousResearch#39719; commits arrived bot-authored,
re-attributed to the contributor.
`_save_anthropic_oauth_creds` wrote the Anthropic OAuth token file with
`os.replace(tmp, path)` followed by a post-hoc `chmod(0o600)`. Between the
rename and the chmod the token file existed at the default umask (0o644 on most
hosts) — a window in which another local user could read the access/refresh
tokens.

Write via `utils.atomic_json_write(..., mode=0o600)`, which creates the temp
with mode 0o600 *before* any content is written, fsyncs, atomically replaces,
preserves the existing file's owner, and cleans up its temp on failure. This
matches the `atomic_json_write(mode=0o600)` call already used elsewhere in this
module for the credential-pool write, and NousResearch#56644's owner preservation.

Tests updated for the new mechanism, plus a check that the write goes through
`atomic_json_write(mode=0o600)` (mutation-verified).
The salvaged SelfHostedBackend made self-hosted servers reachable via
mem0.json / MEM0_HOST, but the setup wizard still offered only Platform
and OSS — exactly the gap users hit (Discord report: 'At memory setup
there's only 2 options'). Adds a third wizard mode:

- interactive picker: Platform / Self-hosted server / Open Source
- non-interactive: hermes memory setup mem0 --mode selfhosted
  --host http://... [--api-key ...] [--dry-run]
- host -> mem0.json (behavioral), API key -> .env as MEM0_API_KEY
  (secret), optional key for AUTH_DISABLED servers
- best-effort reachability check against the server, non-fatal
- README + memory-providers docs updated with the wizard path
…t trace' (NousResearch#60507)

* feat(trace): upload sessions to HF Agent Trace Viewer

Salvage trace upload as a smaller CLI-first feature: deterministic Claude Code JSONL export, fail-closed redaction, lazy Hugging Face dependency, and no gateway slash-command wiring.

* chore(trace): drop external porting references from docstrings

Describe the trace-upload design in Hermes' own terms.

* feat(sessions): fold trace upload into 'sessions export --format trace'

Integrates the HF Agent Trace Viewer exporter (PR NousResearch#36145) onto the
unified export surface instead of a separate 'hermes trace' subcommand:

- --format trace: Claude Code JSONL to stdout/file, or one
  <id>.trace.jsonl per session for filtered bulk export; defaults to
  the most recent session when no --session-id/filters given.
- --upload pushes to the user's private HF traces dataset (--public to
  opt out of private); reads HF_TOKEN with guided setup when missing.
- traces are secret-redacted by default (force mode); --no-redact opts
  out after review; redaction failure blocks export (fail closed).
- hermes_cli/trace.py + subcommands/trace.py removed; agent/trace_upload.py
  is the single engine. Docs EN + zh-Hans; 4 new CLI tests.
…+ non-desktop picker opt-ins

Follow-up on the NousResearch#56966 salvage:

- is_provider_explicitly_configured(): an env-seeded credential-pool entry
  only counts as explicit while its env var still resolves to a usable
  secret. A stale auth.json entry left behind after the user deletes the
  var no longer keeps the provider in the picker forever (NousResearch#55790).
- TUI modelPicker + dashboard ModelPickerDialog/api.getModelOptions pass
  include_unconfigured=true explicitly, preserving their full-universe
  setup-affordance behavior now that the backend defaults to the
  configured subset.
- desktop lib/model-options.ts routes explicit_only through the shared
  requestModelOptions() helper (added on main after the PR branched).
- regression tests for ambient (gh_cli) pool sources, explicit manual/
  device-code sources, and stale vs live env-seeded entries.
…ion gate

_run_approval_gate's gateway branch only queued via submit_pending, so
plugin-escalated approvals never sent the interactive embed+buttons on
Discord/Telegram/Slack (NousResearch#59413) - the user was never notified and the
action stayed silently blocked. Mirror check_dangerous_command's path:
when a session notify callback is registered, run the blocking
_await_gateway_decision round-trip (redacted payload, once/session/
always persistence, deny/timeout produce definitive BLOCKED outcomes);
fall back to submit_pending only when no callback exists.

Fixes NousResearch#59413.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep-alive path when ?attach=<token> is present: PTY outlives the socket
via PTY_REGISTRY, reattaches on reconnect. No token = unchanged legacy
pump (_legacy_pump). detach (not close) on disconnect.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ChatPage sends ?attach=<localStorage token> so /chat reattaches to its
live PTY across refresh. onclose: 4410=process-exit (session ended),
4409=superseded (quiet), else transient -> auto-reconnect.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When an MCP stdio subprocess fails to connect (token expiry, port
contention, timeout), the run() reconnect loop retries with backoff.
Each retry calls _run_stdio() which spawns a new process pair, but the
previous failed pair was only detected as orphaned (added to
_orphan_stdio_pids) — never actually killed.  This caused rapid zombie
accumulation: 5 failed attempts × 2 procs each = 10 orphans competing
for the same port.

Add a _kill_orphaned_mcp_children() call at the top of _run_stdio(),
before the _snapshot_child_pids() baseline, so any orphans from prior
failed attempts are reaped before a new subprocess is spawned.

Fixes NousResearch#57355
Merge the two cherry-picked reap call sites into one unscoped sweep at
the top of _run_stdio (the unscoped sweep is a superset of the
per-server one), and run it via asyncio.to_thread so the 2s
SIGTERM->SIGKILL escalation cannot stall the shared MCP event loop.
A stdio MCP server that never completes `initialize` (e.g. emits a
non-JSON-RPC frame and then blocks on stdin) leaks a child process plus its
stdio pipes/pidfd on every discovery-retry cycle — unbounded, until the
gateway hits EMFILE and every new open()/spawn fails (NousResearch#59349).

Root cause (confirmed by instrumenting the live repro, and different from the
issue's own hypothesis): the spawned child IS captured in `new_pids`, so the
report's "new_pids empty at finally" guess is not it. The real cause is that
`session.initialize()` hangs forever on the garbage stream. `connect_timeout`
only bounds the caller's `.result()` wait on the foreground thread — it does
NOT cancel the `_run_stdio` coroutine on the background MCP loop. So the
coroutine is stuck at `await session.initialize()` permanently, its cleanup
`finally` never runs, the child is never reaped, and it stays invisible to the
orphan-reaper (whose `_orphan_stdio_pids` set never gets populated).

Fix: wrap `session.initialize()` in `asyncio.wait_for(..., connect_timeout)`
so a stalled handshake fails instead of hanging. The TimeoutError unwinds
through the SDK context managers (closing the child's stdin -> EOF -> exit)
and lets the existing `finally` reap any straggler. Cross-platform — no
signals/pgid/proc.

Scope: stdio only. The HTTP path has the same `await session.initialize()`
shape but spawns no subprocess (so it can't cause this leak) and already has
httpx transport timeouts.

Verified: the reporter's repro goes from unbounded growth to draining to zero;
added a hermetic regression test (fake transport whose `initialize()` hangs,
asserts the connect is bounded by connect_timeout) that fails on the pre-fix
code and passes on the fix; 566 existing MCP tests pass; ruff clean.

Repro confirmed on macOS (pipe FDs); the Linux-specific pidfd growth in the
report should be equivalent — the reporter offered to validate on Linux.

Closes NousResearch#59349
… + cancel abandoned start() task

Sibling sites of the same bug class as the salvaged stdio fix:
- SSE, streamable-HTTP (new + deprecated API) initialize() calls are now
  bounded by the same connect_timeout, so an endpoint that accepts the
  connection but never answers the handshake cannot park the run() task
  forever.
- start() now cancels its ensure_future'd run() task when the caller's
  connect timeout cancels start() itself — the orphaned-task leak was
  the root mechanism behind NousResearch#59349, and this closes the class for any
  future pre-ready hang.
lemonwan and others added 29 commits July 9, 2026 19:09
…onnect

The gateway reconnect watcher forwards is_reconnect=True to every
adapter.connect() call on every retry. Adapters whose signature omits
the kwarg raise TypeError at every reconnect attempt and stay silently
disconnected — the exact bug that shipped for QQAdapter and only
surfaced after messages stopped flowing on the QQ channel for hours.

This test statically parses every adapter.py under gateway/platforms/
and plugins/platforms/ (via AST, so third-party SDKs like slack_sdk,
matrix-nio, aiohttp, telegram, etc. are NOT required in the test env)
and asserts every *Adapter class with an async connect() accepts
is_reconnect — either as a keyword-only argument or absorbed by
**kwargs.

Also fixes plugins/platforms/wecom/callback_adapter.py:WecomCallbackAdapter,
which the new test caught as a second offender. Same class of bug: bare
'async def connect(self)' signature would die on the first reconnect.

Companion to NousResearch#59429 (which fixed the original QQAdapter offender).
Addresses Copilot review on NousResearch#61348: the HTML-escaped role, while safe from
injection (quotes are escaped), still contains whitespace when a crafted role
is supplied, which splits the class attribute into several unintended CSS
classes. Keep the escaped role for the display badge, and reduce the raw role
to a single safe CSS token (alnum/-/_) for the class name. Real roles
(user/assistant/system/tool) are unchanged, so the existing .message-<role>
rules still match.
_load_context_cache() returned None when context_length_cache.yaml
contained 'context_lengths:' (no value) — YAML parses this as
{'context_lengths': None} and dict.get(key, default) only returns
the default when the key is absent, not when the value is None.

This caused AttributeError in every downstream caller (issue NousResearch#47135).

Fix: use 'or {}' instead of default= so both absent key and
None value return an empty dict.

Fixes NousResearch#47135
…ousResearch#61797)

skills: null crashed with AttributeError, and a bare scalar
disabled: my-skill was split into a set of characters. Both now
normalize the same way agent.skill_utils._normalize_string_set does:
null -> empty set, scalar -> single-item set. Non-dict skills
sections are ignored.

Closes NousResearch#13026.
…ributeError

When stt.local, tts.edge, or other config subsections are explicitly set
to null in config.yaml (which happens by default on a fresh --voice
setup), stt_config.get('local', {}) returns None instead of {} because
YAML null preserves the key.  The chained .get('model') then crashes
with 'NoneType' object has no attribute 'get'.

Apply the defensive (x or {}) pattern to every place a config subsection
is read via .get('xxx', {}).  Covers local, edge, openai, mistral, and
elevenlabs subsections in both transcription_tools.py and tts_tool.py.

Closes NousResearch#47318
Sibling sites of the salvaged NousResearch#47334 fix: xai/openai/elevenlabs/gemini/
mistral/piper/neutts subsection reads in transcription_tools.py and
tts_tool.py used .get(key, {}) which passes a present-but-null value
through as None. All provider-subsection reads now use .get(key) or {}.

Providers without a DEFAULT_CONFIG entry (e.g. stt.xai) were still
receiving None even after the load_config() deep-merge fix, since the
merge can only fill sections that have defaults.
…les) (NousResearch#61816)

* test: deflake CI and dev-machine flaky tests in bulk

Fixes ten distinct flake sources found by mining recent CI failures and
running the full suite on a dev machine with real user state:

CI-observed races:
- tests/conftest.py live-system guard: allow signal 0 (pure liveness
  probe) through _guarded_kill/_guarded_killpg. psutil.pid_exists()
  probes a just-killed grandchild reparented to init; the subtree check
  fails for it and the guard RuntimeError'd
  test_entire_tree_is_sigkilled_not_just_parent intermittently on
  unrelated PRs.

Hermeticity flakes (fail on dev machines with real state, pass on CI):
- agent/coding_context.py: _marker_root() now skips the shared temp
  root (tempfile.gettempdir()) like it skips $HOME — a stray
  /tmp/package.json flipped every tmp_path test into the coding
  posture (9 failures in test_coding_context.py).
- test_agent_guardrails.py: pin MAX_CONCURRENT_CHILDREN=3 via autouse
  monkeypatch instead of freezing the user's real config value at
  import time (import-time vs call-time config mismatch).
- test_web_tools_config.py: TestCheckWebApiKey now neutralizes the
  ddgs package probe and registry providers — the optional ddgs
  package in a dev venv lit up the fallback backend.
- test_credential_pool.py: block claude_code/hermes-oauth credential
  autodiscovery in the two pool-merge tests that assert exact id
  lists (a real ~/.claude/.credentials.json seeded an extra entry).
- test_modal_sandbox_fixes.py: clear _permanent_approved /
  _session_approved — the user's real command_allowlist silently
  approved the guard-escalation commands under test.
- test_setup_irc.py: stub prompt_checklist to select only the IRC row;
  the non-TTY cancel fallback re-ran the real configured platforms'
  interactive setup_fn, which hit input() under captured stdin.
- test_doctor.py: TestGitHubTokenCheck now patches the module-level
  HERMES_HOME constant (the file's established pattern) instead of
  only setenv — doctor was running PRAGMA integrity_check against the
  real multi-GB state.db and blowing the 300s per-file budget.

Latent atexit-duplication (same _enter_buffered_busy class as NousResearch#34217):
- test_undo_command.py: drop importlib.reload(tui_gateway.server) in
  fixture teardown; reload re-registers the module's atexit hooks.
- test_session_platform_resolution.py: drop per-test reload of
  tui_gateway.server; every resolver reads env at call time.

* test: sentinel model value in ignore-user-config fallback assertion

With HERMES_IGNORE_USER_CONFIG=1, load_cli_config() falls back to the
repo-root cli-config.yaml (untracked, gitignored). On a dev machine that
file can legitimately set the same popular model the test hardcoded
(anthropic/claude-sonnet-4.6), flipping the != assertion locally while
CI (no cli-config.yaml) stayed green. Use an impossible sentinel model
name instead.
`_load_web_config()` is typed `-> dict` but returned `load_config().get("web",
{})`, which is `None` when the config has a present-but-null `web:` section
(YAML `web:` with no body). Every caller then does
`_load_web_config().get(...)` and raises `AttributeError: 'NoneType' object
has no attribute 'get'` — this hits `_get_backend`, `check_web_api_key`, and
the extract-char-limit reader.

Separately, `check_web_api_key()` read the backend as
`.get("backend", "").lower()`; a null `web.backend` value yields `None` (the
`""` default only applies when the key is absent), so `None.lower()` raised.
`check_web_api_key` is the `check_fn` gate for `web_search`/`web_extract`, so
this surfaced as an exception during tool-availability checking.

- Make `_load_web_config()` honor its `-> dict` contract (`... or {}`), fixing
  the null-`web:`-section crash at every call site.
- Guard the backend value in `check_web_api_key` with `or ""`, mirroring the
  existing guard in `_get_backend`.

Adds regression tests for both the null-backend-value and null-web-section
cases.
When config.yaml has known_plugin_toolsets set to null (or any value
mapped to None by the YAML loader), config.get returns None (dict.get
only falls back to the default when the key is absent, not when its
value is None). The subsequent set(known_map.get(platform, [])) then
crashes with TypeError: NoneType object is not iterable and the gateway
fails to start, even though no plugin toolsets are configured.

Add or-empty-dict and or-empty-list guards so a null/None value is
treated as empty instead of crashing the platform-tools resolver.
Sibling of the salvaged NousResearch#53196 read-path fix: setdefault() does not
replace a present-but-null key, so saving platform tools with
known_plugin_toolsets: null in config.yaml crashed on indexing None.
…p @mention delivery

Without this UA tag the Feishu server does not push group @mention events
over the WebSocket transport. The "channel" tag tells the server to use
the Channel protocol which enables group-message routing in addition to P2P
direct messages.

Root cause: FeishuWSClient was created without any UA signaling tag, so the
server defaulted to the basic DM-only push mode. Group @mention events were
silently dropped before reaching Hermes.

Fixes NousResearch#50656

Also adds a regression test verifying the UA tag is present in the
FeishuWSClient constructor call.
* fix(agent): reject malformed tool call arguments

* test(agent): expect malformed tool arguments to fail closed
dict.get(key, default) returns None (not the default) when the key
EXISTS with value None. The default only applies when the key is ABSENT.
Chained method calls (.strip(), .upper(), .count()) crash with
AttributeError on NoneType.

Fix two confirmed hits:
- auxiliary_client.py: custom provider base_url/api_key (config null)
- anthropic_adapter.py: text block content (API null response)

Pattern: .get(key, "").method() → (.get(key) or "").method()
Sibling sites of the salvaged NousResearch#55997 fix, all reading user-editable
config values through .get(key, '').method(): MoA slot provider/model
labels, gateway quick-command alias targets (2 sites), gateway.proxy_url,
and gateway.relay_url. Regression tests for the contributor's two sites
plus the MoA labels.
…nai/omniroute path (ref NousResearch#10575) [carried]

(cherry picked from commit fd5ac147e13d804be824f493ff5c2f5845250ddd)
…ry path [carried]

(cherry picked from commit 207c60782f5f8f8cc97dd930fbcde54ad98e07b3)
…ght/doctor) [carried]

(cherry picked from commit 707470ba781371999ea549f78e482ee191124b8f)
(cherry picked from commit 20a77addbdc2221283f1cf44195ff2351744152a)
hermes plugins update <name> rejected any plugin installed via the
operator-created cross-profile symlink pattern
(~/.hermes/profiles/<profile>/plugins/<name> -> ~/.hermes/plugins/<name>)
with 'resolves outside the plugins directory', because
_sanitize_plugin_name() always followed symlinks via Path.resolve()
before checking containment.

Add an allow_symlink parameter that, when set, returns the symlink
path itself instead of resolving through it. Scoped narrowly to
read/update call sites (cmd_update, dashboard_update_user_plugin);
install/create and remove paths are unchanged and still reject an
escaping symlink, since those are exactly the operations where
following a symlink would be a genuine escape/ambiguity risk. [carried]

(cherry picked from commit 03c3154030f7dbfefa72915fbada794ffaaf71f9)
A verify-on-stop / pre_verify nudge can 'continue' the agent loop after
a valid no-tool-call final_response was already produced. If that
continue lands exactly when the iteration budget is exhausted, the
while-loop's own condition (not an explicit break) ends the turn, so
_turn_exit_reason is left at its 'unknown' initializer even though
final_response holds a real, complete answer.

turn_finalizer.finalize_turn only marks completed=True when
turn_exit_reason starts with 'text_response(' (or the turn ended
under max_iterations), so this falloff produced completed=False with
valid content. cron/scheduler.py then wrapped that valid content in
raise RuntimeError(final_response_text), reporting successful cron
runs to Discord as failures.

Concrete instance: the Daily Quant Platform Dev Agent cron job
(e099c871a5bf) reported a 'provider authentication error' on
2026-07-10, but had actually completed a full work cycle (arb-platform
PR NousResearch#48, an unauthenticated-trading-control-API security fix) -- the
RuntimeError just wrapped the successful report text. Same pattern
confirmed in at least 3 other runs (2026-07-03, 07-06, 07-07) via
cron output history.

Fix: right before finalize_turn is called, if _turn_exit_reason is
still 'unknown' and the turn wasn't failed/interrupted and
final_response has real usable content (not the empty sentinel),
relabel the exit reason as a normal text_response completion so it is
delivered as a real response instead of raised as an exception. Real
failure paths (budget_exhausted-with-no-content, interrupted, error,
empty_response_exhausted) are untouched since they either set failed,
leave final_response empty, or already claim a specific
_turn_exit_reason before this point runs.
trevornk pushed a commit that referenced this pull request Aug 26, 2026
fal's post-trained H3 variant — #1-ranked quality/prompt adherence/
aesthetics, 5s 768p video in under 3 seconds, $0.04/s launch pricing.

- New minimax-h3-max family: minimax/h3-max/{text,image}-to-video
- Inherits base-H3 wire quirks (integer duration, i2v drops
  aspect_ratio) but caps at 768P (480P/768P enums, no 2K/4K) and
  declares seed on both endpoints
- New generic static_payload family flag: constant keys the endpoint
  requires on every request (H3 Max lists prompt_expansion_mode in its
  required array; sent as 'balanced')

Payload asserted against the endpoint OpenAPI schema; 73/73 targeted
tests green (surface matrix auto-covers the new family).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.