Skip to content

Enhance async tool execution and error handling in Hermes agent for A… - #19

Merged
teknium1 merged 1 commit into
mainfrom
atropos-hermes-agent
Feb 8, 2026
Merged

Enhance async tool execution and error handling in Hermes agent for A…#19
teknium1 merged 1 commit into
mainfrom
atropos-hermes-agent

Conversation

@teknium1

@teknium1 teknium1 commented Feb 8, 2026

Copy link
Copy Markdown
Contributor

…tropos integration

  • Updated .gitignore to exclude testlogs directory.
  • Refactored handle_web_function_call in model_tools.py to support running async functions in existing event loops, improving compatibility with Atropos.
  • Introduced a thread pool executor in agent_loop.py for running synchronous tool calls that internally use asyncio.run(), preventing deadlocks.
  • Added ToolError class to track tool execution errors, enhancing error reporting during agent loops.
  • Updated wandb_log method in hermes_base_env.py to log tool error statistics for better monitoring.
  • Implemented patches in patches.py to ensure async-safe operation of tools within Atropos's event loop.
  • Enhanced ToolContext and terminal_tool.py to utilize the new async handling, improving overall tool execution reliability.

…tropos integration

- Updated `.gitignore` to exclude `testlogs` directory.
- Refactored `handle_web_function_call` in `model_tools.py` to support running async functions in existing event loops, improving compatibility with Atropos.
- Introduced a thread pool executor in `agent_loop.py` for running synchronous tool calls that internally use `asyncio.run()`, preventing deadlocks.
- Added `ToolError` class to track tool execution errors, enhancing error reporting during agent loops.
- Updated `wandb_log` method in `hermes_base_env.py` to log tool error statistics for better monitoring.
- Implemented patches in `patches.py` to ensure async-safe operation of tools within Atropos's event loop.
- Enhanced `ToolContext` and `terminal_tool.py` to utilize the new async handling, improving overall tool execution reliability.
@teknium1
teknium1 merged commit fa76a33 into main Feb 8, 2026
h4x3rotab pushed a commit to Clawdi-AI/hermes-agent that referenced this pull request Apr 10, 2026
Fix model filtering for newer OpenClaw config schema
h4x3rotab pushed a commit to Clawdi-AI/hermes-agent that referenced this pull request Apr 10, 2026
…ch#19)

Replaces static isFeatureAvailable() with useFeatureAvailable() React hook that fetches from /api/gateway-status. Fixes enhanced features always showing 'unavailable' after client hydration. Credit: @ClintMoody
malaiwah pushed a commit to malaiwah/hermes-agent that referenced this pull request Apr 13, 2026
- connection.py: cap header read at 8KB to prevent DoS from malicious handler
- handler.py: use .find() instead of `in` + .index() to eliminate race in patch
- handler.py: add truncated field to execute response when output exceeds 50KB
- server.py: include error data field in formatted error messages
- test: add timeout to test client recv, handle TimeoutExpired in close

Fixes issues NousResearch#1, NousResearch#4, NousResearch#5, NousResearch#6, NousResearch#8, NousResearch#10 from Qwen 3.5 peer review on PR NousResearch#19.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Post-merge review. I found one backend regression that would have been a changes-request before merge, plus one testing suggestion. See inline comment and summary comment for details.

Comment thread tools/file_tools.py
image=image,
cwd=cwd,
timeout=config["timeout"],
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: _create_environment() now respects the selected backend, but this call never passes ssh_config. When TERMINAL_ENV=ssh, file tools fail immediately with ValueError: SSH environment requires ssh_host and ssh_user to be configured, even if those env vars are set, because _create_environment() only reads them through its explicit ssh_config argument. Please thread the same SSH config that terminal_tool() passes here and add a regression test for non-local backends.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Reviewed 💬 (2 issues, 0 suggestions)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

I reviewed the diff locally against main and ran the available terminal-backend test script on this checkout:

  • python3 -m py_compile environments/agent_loop.py environments/hermes_base_env.py environments/patches.py environments/tool_context.py model_tools.py tools/file_tools.py tools/terminal_tool.py
  • pytest tests/test_modal_terminal.py -q ✅ (6 passed, with PytestReturnNotNoneWarning warnings from the test file itself)

⚠️ Warnings

  • tools/file_tools.py:59_get_file_ops() now creates non-local environments without forwarding the backend-specific parameters that terminal_tool() passes (task_id, ssh_config, and container config). That means file tools no longer mirror terminal-tool backend semantics: SSH backends fail immediately because _create_environment() requires ssh_host/ssh_user, and docker/modal lose per-task identity/resource settings.
  • environments/tool_context.py:123terminal() and call_tool() were updated to use the async-safe thread helper, but the common convenience wrappers (read_file, write_file, search) still dispatch directly via handle_function_call(...). If verifier code uses those wrappers with modal/docker backends, it can still take the old non-async-safe path this PR is trying to eliminate.

✅ Looks Good

  • The new ToolError tracking in environments/agent_loop.py makes rollout failures much easier to debug.
  • Moving terminal environment creation out from under _env_lock in tools/terminal_tool.py is a good concurrency fix and should reduce lock contention during slow sandbox startup.
  • The dedicated environments/patches.py module keeps the Atropos-specific compatibility logic isolated instead of scattering one-off workarounds across the codebase.

I also tried to submit this as an inline PR review, but GitHub rejected line-thread creation on the already-merged PR with 422 Unprocessable Entity: Line could not be resolved, so I’m leaving the review as a top-level comment instead.


Reviewed by Hermes Agent

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Reviewed 💬 (1 issue, 1 suggestion)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

Post-merge note: this PR is already merged, so I left an informational review instead of a formal “request changes”. Pre-merge, I would have blocked on the item below.

🔴 Critical

  • tools/file_tools.py:64file_tools._get_file_ops() now creates environments for the configured backend, but it does not pass ssh_config into _create_environment(). Repro: with TERMINAL_ENV=ssh, TERMINAL_SSH_HOST=example.com, and TERMINAL_SSH_USER=tester, calling _get_file_ops('review-ssh-repro') raises ValueError: SSH environment requires ssh_host and ssh_user to be configured. This breaks file tools on SSH-backed tasks even when terminal config is valid.

💡 Suggestions

  • Add a regression test that exercises file tools on at least one non-local backend path (SSH is the clearest one). The current test coverage did not catch the backend handoff regression above.

✅ Looks Good

  • Moving environment creation out of the terminal lock is a sensible concurrency improvement for slow backends like Modal/Docker.
  • The added tool-error tracking in the agent loop should make Atropos failures much easier to debug.
  • py_compile passes for all touched Python files, so there are no obvious syntax regressions in the patch itself.

Reviewed by Hermes Agent

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Reviewed after merge. I found 1 follow-up issue and 1 warning worth addressing.

Critical

  • environments/hermes_base_env.py:374train/tool_error_details logs raw tool arguments into Weights & Biases. Those arguments can contain credentials, tokens, file paths, or command fragments from failing tool calls. This pushes potentially sensitive runtime data into external telemetry.
    Suggestion: redact/summarize arguments before logging, or gate detailed error logging behind an explicit debug flag.

Warnings

  • environments/tool_context.py:52-60_run_tool_in_thread() detects a running event loop, then immediately blocks that same async path with future.result(timeout=300) on a one-off ThreadPoolExecutor. In compute_reward() this can stall the event loop and reduce rollout concurrency instead of preserving async-safety.
    Suggestion: make this path awaitable and use await loop.run_in_executor(...), or use a shared executor from the async caller rather than blocking inside the helper.

Suggestions

  • environments/agent_loop.py:302-305 — tool-error detection only records returned JSON errors when exit_code is present and negative. Tools that return {\"error\": ...} without exit_code, or with a non-negative exit code, will be missed in telemetry.
    Suggestion: treat any non-empty error field as an error, then optionally attach exit_code as extra metadata.

Looks Good

  • Moving slow environment creation out of the global lock in terminal_tool.py and file_tools.py should reduce contention across parallel rollouts.
  • The ToolError structure makes post-run diagnosis much easier than relying on raw logs alone.

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (2 warnings, 1 suggestion)

Reviewed post-merge. I ran python3 -m py_compile on the touched modules and python3 -m pytest tests/test_modal_terminal.py tests/test_web_tools.py -q locally; both passed.

Warnings

  • environments/agent_loop.py:304 — tool-error accounting only records JSON results when both error and a negative exit_code are present. Several tools return {"error": ...} without an exit_code (for example write_file_tool in this PR), so the new W&B metrics will under-report real failures.
  • environments/tool_context.py:279cleanup() flips HERMES_QUIET in os.environ, which is process-global. With concurrent rollouts this can mute unrelated tasks while one rollout is cleaning up its browser session.

Suggestions

  • Add a focused regression test around error aggregation / async cleanup paths so the Atropos-specific behavior is covered without relying only on the existing modal/web smoke tests.

Looks Good

  • Moving environment creation out of _env_lock in terminal_tool/file_tools is the right direction for avoiding rollout-wide stalls.
  • The Atropos compatibility shim is well-scoped and the updated tool-path smoke tests still pass locally.

if isinstance(result_data, dict):
err = result_data.get("error")
exit_code = result_data.get("exit_code")
if err and exit_code and exit_code < 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToolError aggregation currently requires both error and a negative exit_code, but several tools return {"error": ...} without any exit_code (for example write_file_tool). Those failures won't make it into result.tool_errors, so the new W&B metrics under-count the exact cases this PR is trying to surface. I'd key off error first and only use exit_code for extra classification.

# Suppress browser_tool's noisy debug prints during cleanup.
# The cleanup still runs (safe), it just doesn't spam the console.
_prev_quiet = os.environ.get("HERMES_QUIET")
os.environ["HERMES_QUIET"] = "1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutates os.environ, so the quiet flag becomes process-global for the duration of the cleanup. In Atropos runs with concurrent rollouts, another task can lose terminal/browser logs while this cleanup is in progress. It would be safer to thread a quiet flag into cleanup_browser() (or suppress logging locally) instead of flipping a shared env var.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Comment (2 warnings, 1 suggestion)

Reviewed post-merge. I ran python3 -m py_compile on the touched modules and python3 -m pytest tests/test_modal_terminal.py tests/test_web_tools.py -q locally; both passed.

Warnings

  • environments/agent_loop.py:304 — tool-error accounting only records JSON results when both error and a negative exit_code are present. Several tools return {"error": ...} without an exit_code (for example write_file_tool in this PR), so the new W&B metrics will under-report real failures.
  • environments/tool_context.py:279cleanup() flips HERMES_QUIET in os.environ, which is process-global. With concurrent rollouts this can mute unrelated tasks while one rollout is cleaning up its browser session.

Suggestions

  • Add a focused regression test around error aggregation / async cleanup paths so the Atropos-specific behavior is covered without relying only on the existing modal/web smoke tests.

Looks Good

  • Moving environment creation out of _env_lock in terminal_tool/file_tools is the right direction for avoiding rollout-wide stalls.
  • The Atropos compatibility shim is well-scoped and the updated tool-path smoke tests still pass locally.

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Post-merge follow-up review. I re-checked the PR diff locally, ran py_compile on the touched Python files, and ran pytest tests/test_modal_terminal.py -q (6 passed).

Findings

  • Critical — environments/hermes_base_env.py:374: train/tool_error_details logs raw tool arguments to W&B. Failing tool arguments can contain tokens, credentials, local paths, or command fragments, so this pushes potentially sensitive runtime data into external telemetry. Please redact/summarize arguments before logging or gate full details behind an explicit debug flag.
  • Warning — environments/tool_context.py:52-60: _run_tool_in_thread() detects the running event loop and then blocks it with future.result(timeout=300) on a one-off executor. That avoids nested asyncio.run(), but it also serializes verifier work and imposes a hidden 5-minute cap on tool calls made through ToolContext. An awaitable run_in_executor path would preserve async-safety without blocking the loop.

The concurrency cleanup in tools/terminal_tool.py and the new ToolError struct in environments/agent_loop.py both look solid.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Reviewed 💬 (1 critical, 1 warning)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

🔴 Critical

  • environments/hermes_base_env.py:374wandb_log() includes raw tool arguments in train/tool_error_details. Failed tool-call args can carry secrets, local paths, or shell fragments, so this can leak sensitive runtime data into external telemetry.

⚠️ Warnings

  • environments/tool_context.py:52-60_run_tool_in_thread() avoids nested event loops, but it blocks the active async path with future.result(timeout=300). That reduces verifier concurrency and introduces an undocumented 300s ceiling for ToolContext tool calls.

✅ Looks Good

  • ToolError tracking in environments/agent_loop.py is a real debugging improvement.
  • Creating terminal environments outside the global lock should reduce contention for slow backends.
  • The changed Python files compile cleanly, and pytest tests/test_modal_terminal.py -q passed on the reviewed checkout.

Reviewed by Hermes Agent

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Changes Requested 🔴 (1 critical, 2 warnings)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

Retrospective review on the merged PR.

🔴 Critical

  • tools/file_tools.py:54-64_get_file_ops() creates a new environment with cwd = config["cwd"] before terminal_tool() runs, but it does not apply the per-task local workdir isolation that terminal_tool() adds in tools/terminal_tool.py:1334-1344. If a rollout hits a file tool first, later terminal calls reuse that already-created environment and run in the shared base cwd instead of the task-specific scratch dir. I reproduced this locally: calling _get_file_ops('abc12345') first, then terminal_tool('pwd', task_id='abc12345'), returned /home/runlvl instead of the generated hermes-abc12345-* directory. Suggestion: move the local-task-workdir selection into a shared environment-creation helper used by both terminal and file tools.

⚠️ Warnings

  • environments/tool_context.py:160-162ToolContext.search() passes {"query": query, ...} into handle_function_call("search", ...), but model_tools.py:1585-1595 only reads pattern. That means verifier code calling ctx.search("needle") silently drops the actual search term and falls back to an empty pattern. Suggestion: pass pattern=query here.
  • environments/tool_context.py:277-289 — cleanup now mutates the process-global HERMES_QUIET env var around cleanup_browser(). tools/terminal_tool.py:1337 also reads HERMES_QUIET to decide whether to create per-task local workdirs, so parallel rollouts can observe this temporary global toggle and change behavior nondeterministically. Suggestion: avoid global env mutation here; make browser cleanup accept an explicit quiet flag instead.

💡 Suggestions

  • tests/test_modal_terminal.py — the added modal checks are written as test_* functions that return True/False instead of asserting, so pytest does not fail when a condition regresses. Converting these to real assertions would give the async/threading changes meaningful coverage.

✅ Looks Good

  • Moving environment creation out from under _env_lock in terminal_tool() is the right direction for reducing lock contention during slow backend startup.
  • The new ToolError capture in the agent loop gives better observability than only logging exceptions.

Reviewed by Hermes Agent

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Changes Requested 🔴 (2 issues, 1 suggestion)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

🔴 Critical

  • environments/tool_context.py:122 — Only ToolContext.terminal() and call_tool() use the new async-safe execution path. Other ToolContext helpers still invoke handle_function_call() directly, so file/web/browser verification code can still re-enter nested event loops.

⚠️ Warnings

  • environments/agent_loop.py:304 — Tool error telemetry only captures JSON results where exit_code < 0. Tools that return {"error": ...} without an exit_code are missed, which undercuts the new error-reporting behavior.

💡 Suggestions

  • Add regression coverage for these async/tool-error paths. This patch changes concurrency behavior across terminal/file/tool execution, but the PR does not add tests for the new failure modes.

✅ Looks Good

  • The worktree/bootstrap changes are directionally right: pushing tool execution off the Atropos event loop is the correct place to attack the deadlock.
  • Passing the managed-server tool-call parser explicitly is a good compatibility improvement for Phase 2.

Reviewed by Hermes Agent

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: ToolContext.terminal() was moved onto a worker thread, but the other convenience wrappers still call handle_function_call() directly. That means reward code using read_file, write_file, search, web_extract, or browser helpers can still hit the same nested-event-loop path this PR is trying to eliminate. Please route those wrappers through the same async-safe helper so the fix applies consistently across the ToolContext API.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Warning: the new telemetry only records failures when exit_code exists and is negative. Several Hermes tools (for example read_file / write_file / search) return {"error": ...} without an exit_code, so those failures disappear from the metrics even though this PR is explicitly improving error reporting. Consider logging any truthy error, then optionally using exit_code only to refine severity.

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Post-merge follow-up review. I re-read the PR diff locally, ran python3 -m py_compile on the touched Python modules, and ran python3 -m pytest tests/test_modal_terminal.py -q (6 passed). I also checked for focused coverage around the new Atropos-specific error/logging paths and did not find any dedicated tests.

error_summaries = []
for err in self._tool_error_buffer:
error_summaries.append(
f"[turn {err['turn']}] {err['tool']}({err['args'][:80]}) -> {err['error'][:150]}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: train/tool_error_details sends raw tool arguments to W&B. Those arguments can contain tokens, secrets, local paths, or command fragments from failed tool calls, so this change can exfiltrate sensitive runtime data into external telemetry. Please redact/summarize arguments before logging, or gate full payloads behind an explicit debug-only flag.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Reviewed 💬 (1 critical, 1 warning, 1 suggestion)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

🔴 Critical

  • environments/hermes_base_env.py:374wandb_log() includes raw tool arguments in train/tool_error_details. Failed tool calls can carry secrets, tokens, local paths, or command fragments, so this leaks sensitive runtime data into external telemetry. Suggestion: redact arguments by default and only emit full payloads behind an explicit debug flag.

⚠️ Warnings

  • environments/tool_context.py:279cleanup() toggles HERMES_QUIET in os.environ, which is process-global. With concurrent rollouts, one task can inadvertently mute logging for unrelated tasks while browser cleanup is running.

💡 Suggestions

  • Add a focused regression test for the new Atropos-only paths (tool_error_buffer / W&B logging and cleanup quieting). I found coverage for the Modal backend smoke path, but no dedicated tests for these new branches.

✅ Looks Good

  • Moving terminal/file environment creation out of _env_lock is the right fix for rollout-wide stalls during slow backend startup.
  • The async compatibility shim is narrowly scoped, and the touched modules still compile cleanly; python3 -m pytest tests/test_modal_terminal.py -q passed locally.

Reviewed by Hermes Agent

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Reviewed 💬 (2 warnings, 1 suggestion)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

⚠️ Warnings

  • environments/tool_context.py:122read_file() / write_file() / search() still bypass the new thread-handoff path and call handle_function_call() directly. In async reward functions, Modal/Docker-backed file operations can still trip the nested-event-loop problem this PR is trying to solve.
  • tools/file_tools.py:30_get_file_ops() returns cached file ops without updating _last_activity, so the cleanup thread can reap an environment that is still actively being used through file tools.

💡 Suggestions

  • environments/agent_loop.py:304 — tool-error tracking only counts results with both error and exit_code < 0, which skips structured tool failures that omit exit_code. Counting any top-level error would make the new metrics more reliable.

✅ Looks Good

  • Moving slow environment creation outside _env_lock is the right direction and should reduce contention across concurrent rollouts.
  • The Atropos-specific async compatibility work is well scoped instead of leaking changes through unrelated call sites.

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

I found one blocking issue in the new async/file-tool environment setup and one follow-up test gap.

Blocking

  • tools/file_tools.py:54 bypasses the per-task local workdir logic that terminal_tool() applies. If a rollout hits a file tool before the terminal tool, the environment is created directly in the shared base cwd, and later terminal calls reuse that already-created environment. That breaks rollout isolation for local backends.

Follow-up

  • Please add a regression test for the sequence write_file/read_file -> terminal('pwd') on the local backend so this stays covered.

Validation

  • python3 -m pytest -q tests/test_modal_terminal.py tests/test_web_tools.py
  • Manual repro on this branch showed write_file_tool() creating the env at the shared base cwd, and a subsequent terminal_tool('pwd') staying in that shared directory instead of a per-task subdirectory.

Comment thread tools/file_tools.py
else:
image = ""

cwd = config["cwd"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this creates the file-tool environment straight from config['cwd'] and never applies the local per-task workdir logic from terminal_tool() (_task_workdirs at lines 1337-1344 there). If a rollout uses a file tool before terminal, all local rollouts can land in the shared base cwd, and later terminal calls will reuse that already-created shared environment.

Comment thread tools/file_tools.py
if not os.getenv("HERMES_QUIET"):
print(f"[FileTools] Creating new {env_type} environment for task {task_id[:8]}...", flush=True)

new_env = _create_environment(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path also skips the mkdir(parents=True, exist_ok=True) that terminal_tool() does for local task workdirs. In my local repro, TERMINAL_CWD=/tmp/hermes-filetools-review caused write_file_tool() to fail with ENOENT on the first write because the cwd did not exist yet. Please either reuse the same local-workdir creation path as terminal_tool() or explicitly create/validate cwd here.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Changes Requested (1 blocking issue, 1 follow-up)

Critical

  • tools/file_tools.py:54 — file tools create local environments without the per-task workdir isolation used by terminal_tool(). If file tools run before terminal, rollout state can leak through a shared cwd.

Warnings

  • tools/file_tools.py:59 — the new file-tool path also assumes cwd already exists; with a missing TERMINAL_CWD, the first write can fail with ENOENT.

Suggestions

  • Add a regression test that exercises write_file/read_file before terminal('pwd') on the local backend.

Looks Good

  • The async-safe thread offloading for terminal/web paths is directionally right.
  • Targeted checks passed: python3 -m pytest -q tests/test_modal_terminal.py tests/test_web_tools.py

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Reviewed the merged PR patch directly.

⚠️ Warnings

  • environments/tool_context.py:122read_file() / write_file() / search() still bypass the new thread-handoff path and call handle_function_call() directly. In async reward functions, Modal/Docker-backed file operations can still trip the nested-event-loop problem this PR is trying to solve.
  • tools/file_tools.py:30_get_file_ops() returns cached file ops without updating _last_activity, so the cleanup thread can reap an environment that is still actively being used through file tools.

💡 Suggestion

  • environments/agent_loop.py:304 — tool-error tracking only counts results with both error and exit_code < 0, which skips structured tool failures that omit exit_code. Counting any top-level error would make the new metrics more reliable.

✅ Looks Good

  • Moving slow environment creation outside _env_lock is the right direction and should reduce contention across concurrent rollouts.
  • The Atropos-specific async compatibility work is well scoped instead of leaking changes through unrelated call sites.

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-merge review: I found one correctness issue in the new Atropos environment plumbing. See the inline note for details.

Comment thread tools/file_tools.py
else:
image = ""

cwd = config["cwd"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path now creates the backing environment with the shared config cwd. terminal_tool() still gives local backends a per-task subdirectory when !HERMES_QUIET, so if a rollout hits read_file/write_file before terminal, the task ends up bound to the shared cwd and later terminal calls reuse that environment. That breaks the isolation guarantee in the comment above (parallel tasks from overwriting each other's files). Can we reuse the same local-workdir derivation here (or centralize env creation) and add a regression test for the file-tool-first path?

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Comment (1 issue)

⚠️ Warnings

  • tools/file_tools.py:54 — local task isolation can be bypassed when the file tools create the environment before terminal_tool() runs. In that order, the task binds to the shared configured cwd instead of the per-task local workdir, so parallel rollouts can bleed into each other.

💡 Suggestions

  • Reuse the same local-workdir derivation in file tool environment creation, or centralize environment creation so terminal_tool() and file_tools cannot diverge.
  • Add a regression test that exercises the file-tool-first path under local backend.

✅ Looks Good

  • The general approach for moving async-backed tool execution off the Atropos event loop is sound.
  • The added tool error collection/logging makes failures much easier to diagnose.

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed after checkout. I found two non-blocking issues worth following up:

  1. environments/agent_loop.py only records tool errors when exit_code < 0, so ordinary command failures (for example a verifier/test returning exit code 1) won't show up in the new telemetry.
  2. environments/tool_context.py creates a brand-new ThreadPoolExecutor(max_workers=1) for every async-context tool call instead of reusing the module-level executor, which adds avoidable thread churn during rollouts.

Modal terminal smoke tests passed locally (tests/test_modal_terminal.py). Full pytest --collect-only still hits unrelated existing OPENAI_API_KEY collection errors in other tests.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Reviewed 💬 (0 blocking issues, 2 suggestions)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

🔴 Critical

  • None.

⚠️ Warnings

  • environments/agent_loop.py:304 — tool-error telemetry only records results when exit_code < 0, so normal command failures with positive exit codes are skipped.

💡 Suggestions

  • environments/tool_context.py:56_run_tool_in_thread() creates a fresh ThreadPoolExecutor(max_workers=1) on every async-context call instead of reusing the module-level executor. Reusing one pool would avoid per-call thread churn during parallel rollouts.
  • tools/file_tools.py:122write_file_tool() prints errors unconditionally, which can get noisy in training runs. Consider routing this through the logger or respecting HERMES_QUIET like the other new environment-status prints.

✅ Looks Good

  • The thread-offload strategy in terminal_tool.py and file_tools.py removes the obvious _env_lock bottleneck around slow environment creation.
  • The Modal smoke suite passed locally with pytest -q tests/test_modal_terminal.py.
  • The Atropos-specific patching is isolated in environments/patches.py, which keeps the compatibility workaround contained.

Notes

  • pytest --collect-only -q tests still fails on unrelated pre-existing collection errors because some tests require OPENAI_API_KEY.

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Post-merge review. I checked the PR head locally, ran python3 -m py_compile on the touched Python modules, and ran pytest tests/test_modal_terminal.py -q (6 passed). tests/test_web_tools.py did not run in this environment because the optional firecrawl dependency is missing.

I found two issues worth addressing in follow-up: one correctness problem in tool-error accounting and one telemetry/privacy concern in W&B logging. See inline comments for details.

if isinstance(result_data, dict):
err = result_data.get("error")
exit_code = result_data.get("exit_code")
if err and exit_code and exit_code < 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This only records tool failures when the JSON result has both error and a negative exit_code. Several tool paths return {"error": ...} without any exit_code (for example write_file_tool in this PR), so those failures never make it into tool_errors and the new W&B metrics undercount real tool failures.

error_summaries = []
for err in self._tool_error_buffer:
error_summaries.append(
f"[turn {err['turn']}] {err['tool']}({err['args'][:80]}) -> {err['error'][:150]}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ This pushes raw tool arguments into W&B (train/tool_error_details). Failed tool args can include credentials, local paths, shell fragments, or other sensitive runtime data. Please redact or summarize arguments before sending them to external telemetry.

@runlvl

runlvl commented Apr 18, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Comment (2 warnings)

Reviewed post-merge.

Warnings

  • environments/agent_loop.py:304 — tool-error accounting only records failures when a tool returns both error and a negative exit_code, so some real tool failures are silently excluded from tool_errors and the new W&B metrics.
  • environments/hermes_base_env.py:374train/tool_error_details includes raw tool arguments, which can leak sensitive runtime data into external telemetry.

Suggestions

  • Add a focused regression test for tool-error aggregation so {"error": ...}-only tool responses are covered.
  • Consider redacting arguments before logging or gating full argument capture behind an explicit debug flag.

Looks Good

  • python3 -m py_compile passed for the touched Python modules.
  • pytest tests/test_modal_terminal.py -q passed locally (6 passed).
  • Moving environment creation out of the terminal/file tool locks is a solid concurrency improvement.

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Post-merge review on the current PR patch.

🔴 Critical

  • tools/file_tools.py:54-64 — creates the local environment directly at and caches it before gets a chance to apply its per-task local-workdir isolation (). I reproduced this on the PR checkout with : calling first and then returned the shared base cwd instead of the generated task directory. That means the file-tool-first path can bypass rollout isolation and leak state across parallel tasks.

💡 Suggestion

  • Move local task-workdir selection into a shared environment-creation helper used by both and , then add a regression test for the sequence on the local backend.

✅ Validation

  • Manual repro on the PR checkout confirmed the isolation mismatch described above.

Reviewed by Hermes Agent

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (1 new warning, no duplicate inline comments repeated)

I reviewed the merged PR diff locally, ran python3 -m py_compile on the touched Python modules, and ran python3 -m pytest -q tests/test_modal_terminal.py (6 passed, with existing PytestReturnNotNoneWarning warnings in that test file).

Warnings

  • environments/hermes_base_env.py:180 — setting os.environ["TERMINAL_ENV"] in __init__ makes backend selection process-global. If multiple HermesAgentBaseEnv instances run concurrently with different terminal_backend values, they can overwrite each other and route tool calls to the wrong backend.

Coverage note

  • I could not find automated tests covering concurrent env instances with different terminal_backend values; current coverage is still centered on the single-backend modal smoke test.

I only posted the new inline note below to avoid duplicating issues already covered elsewhere on this PR.

@@ -172,10 +178,14 @@ def __init__(
# Set terminal backend environment variable so hermes tools pick it up
if config.terminal_backend:
os.environ["TERMINAL_ENV"] = config.terminal_backend

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Warning: this stores the selected backend in process-global os.environ, so concurrent HermesAgentBaseEnv instances can overwrite each other. If one rollout sets TERMINAL_ENV=modal while another expects local or ssh, later tool calls read whichever value won the race and can run against the wrong backend. Please keep backend selection task/env-local (for example on the env/context) instead of mutating a shared env var, and add a regression test that exercises two env instances with different terminal_backend values in the same process.

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hermes Agent Review

Post-merge re-review using the REST flow (with gh only as a token source).

What I checked:

  • git diff 578a5fb6..d999d987 on the PR snapshot
  • python3 -m py_compile on the touched Python modules
  • pytest -q -o addopts='' tests/test_modal_terminal.py tests/test_web_tools.py
    • Result: 6 passed, 6 warnings (PytestReturnNotNoneWarning in tests/test_modal_terminal.py)

Review result:

  • I did not add new inline comments in this pass.
  • The substantive issues I can still defend from the diff are already covered by existing line comments on this PR (file-tool backend parity, async timeout/error handling, raw command/tool-arg logging, and missing regression coverage for the new async paths).
  • I’m avoiding duplicate inline noise on a merged PR.

Net: reviewed again, no new non-duplicate line comments to add beyond the issues already called out in-thread.

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Reviewed 💬 (0 new inline comments, 1 coverage warning)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

Reviewed retrospectively after merge. I did not add duplicate inline comments because the existing runlvl review history on this PR already covers the main correctness regressions in tools/file_tools.py, environments/agent_loop.py, environments/tool_context.py, and environments/hermes_base_env.py.

⚠️ Warnings

  • Coverage gap: the new async bridge paths in environments/tool_context.py:44-63, model_tools.py:1194-1204, and environments/patches.py are still only partially covered from what I could verify in this historical checkout. I could not find focused tests importing environments/patches.py, ToolContext, or handle_web_function_call() under a running event loop, so the Atropos-specific behavior is still not directly regression-tested here.

✅ Looks Good

  • python3 -m py_compile environments/agent_loop.py environments/hermes_base_env.py environments/patches.py environments/terminal_test_env.py environments/tool_context.py model_tools.py tools/file_tools.py tools/terminal_tool.py
  • pytest -q -o addopts='' tests/test_modal_terminal.py ✅ (6 passed)
  • I checked for prior review coverage before posting and intentionally skipped duplicate line comments.

Test Notes

  • pytest -q -o addopts='' tests/test_web_tools.py could not be collected on this host because firecrawl is not installed.
  • The newer tests/run_agent/... coverage from the current tree does not exist in this historical PR checkout, so it was not applicable evidence for PR #19 itself.

Reviewed by Hermes Agent via REST API (no gh pr ... workflow used).

@runlvl runlvl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Changes Requested 🔴 (1 issue, 1 suggestion)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

⚠️ Warnings

  • tools/file_tools.py:54_get_file_ops() now creates environments using the configured backend, but it does not mirror terminal_tool's per-task local workdir logic. If a rollout hits a file tool before terminal_tool, the task is created directly in the shared cwd instead of hermes-<task>-*, so parallel local rollouts can still overwrite each other's files. I reproduced this on the PR head: read_file_tool(..., task_id='taskB') created an env with cwd='.', taskB was absent from _task_workdirs, and a subsequent terminal_tool('pwd', task_id='taskB') resolved to the shared repo root.

💡 Suggestions

  • Add a regression test for the file-first path (e.g. local backend + read_file_tool/write_file_tool before any terminal call) and for the new async-safe execution path. There are no test changes in this PR, so the concurrency fix is currently unguarded.

✅ Looks Good

  • The separation between loop-level tool execution changes, Modal patching, and wandb error logging is easy to follow.
  • py_compile passes for all touched Python files.

Reviewed by Hermes Agent

Comment thread tools/file_tools.py
else:
image = ""

cwd = config["cwd"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Warning: this path bypasses the per-task local workdir logic that terminal_tool() applies in tools/terminal_tool.py:1334-1344. If a rollout touches a file tool first, the environment is created directly in the shared cwd instead of a hermes-<task>-* subdirectory, so parallel local rollouts can still clobber each other's files. I reproduced that on this PR head by calling read_file_tool(..., task_id='taskB') first: the env cwd stayed . and _task_workdirs never got an entry for taskB.

@runlvl

runlvl commented Apr 19, 2026

Copy link
Copy Markdown

Code Review Summary

Verdict: Changes Requested 🔴 (1 issue, 1 suggestion)

PR: #19 — Enhance async tool execution and error handling in Hermes agent for Atropos integration
Author: @teknium1
Files changed: 9 (+540 -64)

⚠️ Warnings

  • tools/file_tools.py:54_get_file_ops() now creates environments using the configured backend, but it does not mirror terminal_tool's per-task local workdir logic. If a rollout hits a file tool before terminal_tool, the task is created directly in the shared cwd instead of hermes-<task>-*, so parallel local rollouts can still overwrite each other's files.

💡 Suggestions

  • Add a regression test for the file-first path (local backend + read_file_tool/write_file_tool before any terminal call) and for the new async-safe execution path. There are no test changes in this PR, so the concurrency fix is currently unguarded.

✅ Looks Good

  • The separation between loop-level tool execution changes, Modal patching, and wandb error logging is easy to follow.
  • py_compile passes for all touched Python files.

Reviewed by Hermes Agent

angelburgosrosado pushed a commit to angelburgosrosado/hermes-agent that referenced this pull request Apr 27, 2026
…gent

Enhance async tool execution and error handling in Hermes agent for A…
begjb pushed a commit to begjb/hermes-agent that referenced this pull request May 29, 2026
…doffs

The respawn guard's 'active_pr' check correctly prevents duplicate PR
creation when a worker is respawned on a task that already has a PR
URL in its comment history. But the same signal misfires for
review-drain workflows: when an author blocks a task with
'review-required: ...' after posting the PR URL in a comment, the
dispatcher must spawn a reviewer worker on the same task. The PR URL
in comments is the handoff payload, not a duplicate-PR risk.

Observed today on jetminds board: t_b522b69c (PR NousResearch#19) and t_509f152c
(PR NousResearch#17) both looped through respawn_guarded:active_pr → jm-scan-drift
re-block → routing fire → respawn_guarded:active_pr. The block_message
accumulated three '[auto-reblocked by jm-scan-drift after drift
detected]' bracket-suffixes before the loop was caught.

Fix: introduce _review_required_handoff_active(conn, task_id) — a
narrow predicate that reads the task's most-recent 'blocked' event's
reason payload and returns True iff it starts with 'review-required:'.
check_respawn_guard consults the predicate; when active, the
active_pr branch is short-circuited and the spawn proceeds.

The exception is scoped to the single most-recent blocked event, not
a substring scan of full history. Once the reviewer drains and a
'completed' event lands after the block, subsequent re-spawns revert
to the normal active_pr behavior (verified by
test_respawn_guard_review_exception_does_not_leak_past_completion).

Tests: 6 new (review-exception suppress, non-review still guards,
exception expires post-completion, fresh-task baseline, prefix-only
match, leading-whitespace tolerance). 21/21 respawn-guard tests pass;
164/164 test_kanban_db.py file passes. No upstream behavior changed
for non-review-drain workflows.

Refs: substrate task t_ee014b80 (engineering-lead, 2026-05-21)
JetMinds-local; per hermes-fork-patches discipline
xzmzm added a commit to xzmzm/hermes-agent that referenced this pull request Jun 15, 2026
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
…gent

Enhance async tool execution and error handling in Hermes agent for A…
amlord added a commit to NuggetsLtd/hermes-agent that referenced this pull request Jul 16, 2026
…ce test

review follow-ups on NousResearch#19:
- normalise ""/whitespace idempotency_key to NULL in create_task; exclude
  '' from the unique partial index and the migration dedupe pass so keyless
  creates never collide (a second stored-'' insert used to raise IntegrityError)
- move the race test barrier between create_task's fast-path SELECT and its
  INSERT (via _new_task_id) so the recovery path is exercised deterministically
- add empty-key regression test + migration dedupe coverage for pre-existing
  active duplicate keys

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Soju06 added a commit to Soju06/hermes-agent that referenced this pull request Jul 22, 2026
…rn thread (patch NousResearch#19)

Turn-waterfall tracing showed llm.accounting at p50 3.3ms / p95 70.4ms
per API call (historically up to 299ms into a cold 6.8GB state.db):
conversation_loop ran SessionDB.update_token_counts() synchronously on
the turn thread after EVERY API call inside the tool loop — a BEGIN
IMMEDIATE sessions UPDATE plus a session_model_usage upsert.

SessionDB grows a single-writer background queue:

- queue_token_counts(session_id, **kwargs): same signature/semantics as
  update_token_counts, but the critical path is a deque append + condvar
  notify (measured p50 0.0015ms / p95 0.0036ms vs 2.6ms/7.7ms for the
  sync UPDATE on a WARM tiny db). A dedicated daemon thread
  ("session-db-token-writer", started lazily on first enqueue) applies
  deltas in enqueue order via the existing update_token_counts →
  _execute_write path, so SQLite access keeps the established
  self._lock / BEGIN IMMEDIATE / jitter-retry discipline unchanged.
- Coalescing: when a backlog forms, consecutive deltas for the same
  (session, model, cost_status, cost_source, pricing_version,
  billing_provider/base_url/mode) route merge into one UPDATE — token/
  api_call fields sum, cost fields sum None-preservingly (an all-None
  run stays None so COALESCE keeps the stored value). Route equality is
  required precisely because those fields feed COALESCE backfill,
  last-non-None-wins status fields, and the per-model usage attribution
  key — a merged apply is provably equivalent to sequential applies
  (equivalence test compares full sessions + session_model_usage rows).
  absolute=True deltas (gateway cumulative overwrites) never merge and
  act as ordering barriers; only ADJACENT deltas merge, so a mid-session
  /model switch attributes exactly as before.
- Read-your-writes: flush_token_counts(timeout=5) blocks until drained
  (attribute-check no-op when idle; writer sets busy BEFORE popping the
  queue so the lock-free fast path can never miss an in-flight batch).
  Reader audit — flush added at: get_session, list_sessions_rich,
  _get_session_rich_row, list_gateway_sessions (MCP listings, /status,
  channel directory), InsightsEngine.generate (getattr-guarded: tests
  hand it raw connections). In-memory per-turn counters
  (agent.session_estimated_cost_usd etc.) are updated synchronously as
  before, so live turn displays never see the queue. Resume-path row
  readers (find-by-key) run at turn start when the previous finalize
  already drained — left unflushed deliberately.
- Durability: AIAgent._persist_session (turn finalize + every error-exit
  persist point) flushes; SessionDB.close() stops+drains the writer
  before the WAL checkpoint; an atexit hook (registered when the writer
  first starts) drains at interpreter shutdown. Worst-case crash loss:
  the in-flight call's delta — same window as the old inline write. If
  the writer is stopped/dead, flush_token_counts drains on the caller's
  thread instead of losing deltas.
- Failure isolation: apply errors are logged (logger.warning) and never
  raise into a turn; the writer thread survives and keeps applying.

Call sites routed through the queue: conversation_loop.py ~2423 (the
hot per-call site — its _ensure_db_session row-existence retry stays at
enqueue time, and update_token_counts itself still does the idempotent
INSERT OR IGNORE before applying) and both codex_runtime.py sites
(~132 api-call-count-only, ~213 usage+cost). The llm.accounting span
now measures the enqueue; span name and cache_pct tag are untouched
(tag reads the response, not the DB).

Estimated impact: removes p50 3.3ms / p95 70ms (cold-cache tail up to
~300ms) per API call from the turn thread; multi-tool turns with N API
calls save N× that. The write itself still happens (same total DB work)
but off-thread and often coalesced into fewer transactions.

Tests: tests/agent/test_async_token_accounting.py (13 tests: strict
enqueue-order across sessions, absolute-as-barrier semantics, backlog
coalescing with exact sums incl. session_model_usage, coalesced-vs-
sequential row equivalence, unit merge rules, None-cost preservation,
get_session read-your-writes under a slow writer, empty-flush fast
path, drain-on-caller after writer stop, close() drain survives reopen,
atexit idempotence, _persist_session drain via real AIAgent, writer
failure logs + survives). tests/run_agent/test_token_persistence_non_cli.py
updated to the queue_token_counts contract. Suites for every touched
file green: tests/test_hermes_state.py, tests/test_sql_injection.py,
tests/agent/test_insights.py, test_codex_app_server_persist.py,
test_turn_context.py, test_api_content_sidecar.py,
test_gateway_turn_sidecar.py, turn-finalizer suites,
tests/cli/test_cli_insights_command.py + shutdown-memory suites,
tests/cron/test_codex_execution_paths.py, tests/gateway
(prompt_tail_freeze, 13121 shutdown flush, shutdown memory),
tests/hermes_cli/test_web_server.py, tests/tui_gateway
finalize-persist, tests/tools/test_interrupt.py, and the full
tests/run_agent tree. Only failures: test_pre_tool_session_id.py (3)
and test_413_compression.py (1) — verified pre-existing via clean-tree
(git stash) baseline runs.

Origin: local-author
Upstream-PR: none
Patch-State: local-only

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Soju06 added a commit to Soju06/hermes-agent that referenced this pull request Jul 22, 2026
…ousResearch#19 review)

Three review findings against the queue_token_counts patch, all
reproduced before fixing:

1. Route-switch reordering (medium): update_session_model /
   update_session_billing_route / update_session_meta write the sessions
   row synchronously, bypassing the delta queue. A first-of-session
   delta enqueued before a /model switch could apply AFTER the switch
   UPDATE; update_token_counts then saw api_call_count == 0 plus a
   model/provider mismatch and its first_accounted_route branch
   unconditionally rewrote model/billing_provider/base_url back to the
   pre-switch route — permanently, until the next switch. Pre-patch the
   delta committed on the turn thread before the switch command could
   run. Fix: the three sync route writers call flush_token_counts()
   first, restoring that happens-before as a queue barrier.

2. flush vs stop-flagged writer (low): flush_token_counts treated a
   stop-flagged but still-running writer as dead — it could drain newer
   deltas on the caller's thread concurrently with (and before) the
   writer's in-flight older batch, and return True while that batch was
   unapplied. A live stop-flagged writer drains the queue itself before
   exiting, so flush now trusts any live writer and only takes the
   leftovers when the thread is dead or never started; liveness is
   re-checked on every wakeup because the writer can exit mid-wait with
   deltas enqueued after its final empty-queue check.

3. atexit leak (low): atexit.register(self._drain_token_queue_at_exit)
   held a strong reference (bound method) to every SessionDB that ever
   enqueued, pinning closed instances and their sqlite connection
   objects until interpreter exit in multi-open/close processes.
   close() now unregisters the hook (bound-method equality makes
   atexit.unregister match exactly our registration).

Tests: three regressions added to tests/agent/test_async_token_accounting.py
(switch-barrier row state incl. per-model attribution staying on the
pre-switch route, flush timing out instead of bypassing a busy
stop-flagged writer + in-order drain after release, closed instance
collectable via weakref+gc). Suite now 16 passed. Baseline-compared
runs: tests/test_hermes_state.py, tests/test_sql_injection.py,
tests/agent/test_insights.py, tests/acp/test_session_db_private_access.py,
tests/run_agent/test_token_persistence_non_cli.py,
tests/test_tui_gateway_server.py, tests/gateway model-switch suites,
full tests/agent and tests/run_agent trees — failure sets byte-identical
to the clean-tree baseline (144 pre-existing in tests/agent transports,
3 in test_tui_gateway_server.py, 4 in tests/run_agent).

Origin: local-author
Upstream-PR: none
Patch-State: local-only

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MarcoFernstaedt pushed a commit to MarcoFernstaedt/hermes-agent that referenced this pull request Aug 1, 2026
Partial response to the FAIL-HOLD. Eight of twenty findings; the rest are named
in the reply, not silently dropped.

**NousResearch#9, and a second defect it exposed.** The review is right and it was my
error: `App.tsx` added a `<main>` around a `PageHeaderProvider` that already
rendered one, and my test audited a hand-written stand-in shell rather than the
real one — the exact fixture failure this project keeps hitting, committed by
me while criticising it. The landmark now lives where it always lived, and
`ThreePane`/`VaultPage` render labelled sections instead of competing
landmarks. Rendering the *real* provider then immediately found more: eleven
components shipped their own `h1` under a provider that already had one, so
every one of those routes announced two page titles. All demoted to `h2`.

**NousResearch#18.** The `/api/ws` exclusion was wrong for the reason given — ASGI HTTP
middleware never sees a WebSocket scope, so it protected nothing about the
upgrade and only stripped headers from the 404s and auth rejections under that
prefix. Removed, with a `websocket_connect` regression proving the upgrade
still completes or closes deliberately.

**NousResearch#4.** Claims carry an attempt token and every write compare-and-swaps on it.
The adversarial sequence the review demonstrated — A claims, A expires, B
claims, stale A settles B's row — is now a test, and A's settle returns False.
Losers receive no token at all, so they cannot write anything.

**NousResearch#3.** A provider exception is no longer `failed`. Gmail accepting a message
and then dropping the response is indistinguishable from never receiving it, so
the outcome is `ambiguous`: blocked, not retryable, and never unblocked by
elapsed time — time is not evidence about an external system. Compose failures
stay `failed` because they provably precede dispatch. Messages carry a
deterministic broker-controlled RFC Message-ID so reconciliation is a lookup
rather than a content comparison. Three of my own tests asserted the old
behaviour; they were asserting the duplicate-mail bug and are rewritten.

**NousResearch#7.** Reversal owner tokens fence every terminal write, and reconciliation
clears the owner — without that a worker declared abandoned could return and
match its own token. An inverse exception is now `reversal_unknown` rather than
`done`: `apply` runs outside the journal transaction, so a half-applied inverse
raises exactly as a never-started one does, and calling it retryable invited a
second inverse over a partial first.

**NousResearch#5.** Subjects are out of durable audit rows, and provider diagnostics are
reduced to type plus a bounded excerpt — a mail API's error text frequently
quotes the recipients and subject back.

**NousResearch#8, honestly.** The shared open path adds an explicit `busy_timeout`, bounded
retry around the journal-mode switch, and `BEGIN IMMEDIATE` for migration. But
**I could not reproduce the reported failure**: 360 concurrent constructions
per store, zero `database is locked`, on the old path as well as the new. The
tests are a regression guard, not proof the fix works, and the test file says
so. The claim belongs to whoever can reproduce the original numbers.

**NousResearch#19 partial.** Evidence will be rebound to the final candidate once the
remaining findings are answered; rebinding it now would date it again.

Gates: typecheck 0, eslint 0, 74 files / 695 frontend tests; candidate backend
249 passed; named backend gate 142 passed, 0 failed.

Not merged to main: the review is a FAIL-HOLD and the remaining findings —
tier enforcement, registry dispatch bypass, undo wiring, native resume, New
Chat, free-form clarify, approval acknowledgement, reconnect, sensor delivery —
are not addressed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nu2Qaq5Y7EScuooGz8co34
ethenotethan added a commit to ethenotethan/harness that referenced this pull request Aug 4, 2026
…i) (NousResearch#19)

test_wiki.py failed at collection on main — `from tui_gateway import wiki`,
but the module was renamed to `wiki_api` (functions `wiki_scan`/`wiki_page`,
not `wiki.scan`/`wiki.page`). This has been red on main and blocks unrelated
PRs since the whole shard errors on collection.

Fix the import and update assertions to the module's actual behavior,
verified against the current wiki_api:
- wiki_page returns {frontmatter, body, path} — assert `path`, not a
  nonexistent `id` field.
- a page with no frontmatter gets the default type "concept" (was asserting
  the old "page" default).
Scan, link extraction, path-traversal guards, and _parse_frontmatter cases
are unchanged and still pass.

11 passed; ruff clean.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Meraniya pushed a commit to Meraniya/hermes-agent that referenced this pull request Aug 6, 2026
…s) (NousResearch#19)

* feat(slack): add channel list/invite helpers for "all channels" coverage

The Slack Socket Mode adapter (bidirectional) and launchd LaunchAgent
already exist. The missing piece was reproducible "all channels" wiring:
a Slack bot only sees/posts in channels it has joined, and there's no
bulk-add API.

Add two CLI helpers:
- `hermes slack channels` — list channels and report which ones the bot
  is / isn't a member of (audits coverage, reports gaps).
- `hermes slack invite [--all|--channel] [--user-token] [--dry-run]` —
  self-join public channels via conversations.join; invite the bot to
  private channels via conversations.invite when a user token is given.

Also:
- add `channels:join` bot scope to the generated manifest so self-join works
- add `hermes slack manifest --yaml` (Slack accepts JSON or YAML)
- document SLACK_USER_TOKEN, the connections:write app-token requirement,
  and the invite-per-channel vs read-all (user token) trade-off
- tests for channel listing, invite paths, and manifest rendering

* fix(nix): update web npm-deps hash to match committed lockfile

The fetchNpmDeps hash in nix/web.nix drifted from web/package-lock.json
(pre-existing on main after an earlier npm dependabot bump), failing the
`nix` build check. Update to the hash computed by the nix build itself.

---------

Co-authored-by: Claude <noreply@anthropic.com>
lenardhuebner88-rgb pushed a commit to lenardhuebner88-rgb/hermes-agent that referenced this pull request Aug 10, 2026
…rückgestellt, NousResearch#19 gesplittet, Beifang notiert

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

ohzie commented Aug 15, 2026

Copy link
Copy Markdown

Closed unmerged: the captain replaced this robot-specific rollout-validation architecture with a single generic reusable Hermes deployment/rollback playbook.

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.

3 participants