feat(tools): add Tenki cloud sandbox terminal backend - #947
Conversation
|
@tenki-reviewer review this PR — full review please. This mirrors upstream NousResearch#64190 (new Tenki cloud sandbox terminal backend). |
|
Review Complete Files Reviewed: 47 By Severity:
PR introduces critical gateway startup failures from missing modules/imports, silently downgrades security defaults, and contains snapshot-store race conditions that risk data loss in the Tenki sandbox backend. Files Reviewed (47 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🔴 Critical (87/100) — 4 critical findings, 4 high, 6 medium · 12674 LOC across 47 files
Critical Blockers (4)
- Gateway crashes on startup with
ModuleNotFoundErrorfor the missinggateway/cwd_placeholder.pymodule (line 1759) build_bundle_invocation_messagecalled with unexpectedplatform=parameter causingTypeErrorat line 10193AsyncSessionStoreimported fromgateway.sessionbut never defined —ImportErrorat gateway startup (line 1788)strip_stale_dangerous_confirmationsandis_dangerous_confirmationare unused imports that don't exist (line 1039)
Security Downgrades (2)
approvals.modedefault switched from'manual'to'smart'without migration — silently weakens security posture for existing users (line 2549)- Memory provider setup runs install commands from
plugin.yamlwithshell=Truewithout sanitization (line 4671)
Data Integrity Risks (3)
- Snapshot store: non-atomic read-modify-write loses snapshot references under concurrent access (
base.py:169) - Tenki: sandbox termination during
wait_durablekills both snapshot and the durability promise (tenki.py:1047) - TOCTOU race on
self._sandboxbetween_start_processand cancel in Tenki environment (tenki.py:860)
Other Issues
display.show_reasoningdefault flipped without migration (line 1808)atomic_config_writeis dead code defeating its single-chokepoint design (line 6761)- RPC token leaked in shell command string for remote code execution backends (
code_execution_tool.py:993) - Streaming force-flush guard inconsistent causing garbled tables (
cli.py:5907)
| strip_stale_dangerous_confirmations as _strip_stale_dangerous_confirmations, | ||
| is_dangerous_confirmation as _is_dangerous_confirmation, | ||
| ) |
There was a problem hiding this comment.
🔴 Imported functions strip_stale_dangerous_confirmations and is_dangerous_confirmation do not exist — gateway crashes on startup (bug)
gateway/run.py imports strip_stale_dangerous_confirmations and is_dangerous_confirmation from agent.replay_cleanup (lines 1039-1040), but these functions are not defined in agent/replay_cleanup.py or anywhere else in the repository. The import is at module level, so any code path that imports gateway/run.py will immediately raise ImportError. The gateway cannot start. These functions are referenced at lines 938 and 18779 for stripping stale dangerous-confirmation text from replay history, but those call sites are unreachable due to the module-level import failure.
💡 Suggestion: Either add the missing function definitions to agent/replay_cleanup.py, or remove the imports and call sites if the feature is not yet ready.
📋 Prompt for AI Agents
In gateway/run.py lines 1035-1041, the import statement references two functions that do not exist. Either: (a) add strip_stale_dangerous_confirmations and is_dangerous_confirmation function definitions to agent/replay_cleanup.py, or (b) remove the two imports from the from-agent.replay_cleanup import block and remove the two call sites at lines 938-940 and 18779-18781, then remove the unused _is_dangerous_confirmation alias.
| # by the config bridge above). Placeholder values are resolved per-backend — | ||
| # see gateway/cwd_placeholder.py for the three-case contract (local vs docker | ||
| # mount-off vs docker mount-on). MESSAGING_CWD is a backward-compat fallback. | ||
| from gateway.cwd_placeholder import CWD_PLACEHOLDERS, resolve_placeholder_terminal_cwd |
There was a problem hiding this comment.
🔴 Missing gateway/cwd_placeholder.py module — gateway crashes on startup with ModuleNotFoundError (bug)
This PR added imports of CWD_PLACEHOLDERS and resolve_placeholder_terminal_cwd from gateway.cwd_placeholder in both gateway/run.py (line 1759) and tests/gateway/test_config_cwd_bridge.py (line 15). However, the file gateway/cwd_placeholder.py was never created or included. The gateway/run.py import is at module level (not inside a try/except), so the gateway process fails to start. The module is expected to provide placeholder CWD resolution logic (local vs docker mount-off vs docker mount-on) and a CWD_PLACEHOLDERS set.
💡 Suggestion: Create gateway/cwd_placeholder.py with the CWD_PLACEHOLDERS set and resolve_placeholder_terminal_cwd function implementing the three-case contract described in the comment.
📋 Prompt for AI Agents
Create the file gateway/cwd_placeholder.py implementing: (1) CWD_PLACEHOLDERS — a set of placeholder strings like {'.', 'auto', 'cwd'}, and (2) resolve_placeholder_terminal_cwd(configured_cwd, terminal_backend, messaging_cwd, docker_mount_cwd_to_workspace, home_fallback) — a function that resolves the terminal CWD based on backend: for SSH return None (preserve remote home); for docker with mount, return workspace path; for local/docker without mount, return home_fallback or messaging_cwd as fallback.
| @@ -1669,6 +1785,7 @@ def _profile_runtime_scope(profile_home: "Path"): | |||
| load_gateway_config, | |||
| ) | |||
| from gateway.session import ( | |||
| AsyncSessionStore, | |||
There was a problem hiding this comment.
🔴 AsyncSessionStore imported from gateway.session but never defined — ImportError at gateway startup (bug)
gateway/run.py:1788 imports AsyncSessionStore from gateway.session. gateway/session.py defines only four classes (SessionSource, SessionContext, SessionEntry, SessionStore at lines 122, 258, 603, 913) — no AsyncSessionStore exists. The class is used in __init__ at line 2899 (self.async_session = AsyncSessionStore(...)) and accessed via the async_session_store property at line 10846. This cross-file omission causes a hard ImportError at module load time, preventing any gateway process from starting.
💡 Suggestion: Define the AsyncSessionStore class in gateway/session.py as an async-safe facade around SessionStore that mirrors its key methods using asyncio.to_thread().
📋 Prompt for AI Agents
In gateway/session.py, add an AsyncSessionStore class that wraps SessionStore methods with asyncio.to_thread() for each store operation called from async contexts in gateway/run.py. Required methods include: get_or_create_session, load_transcript, append_to_transcript, switch_session, reset_session, mark_resume_pending, clear_resume_pending, update_session, rewrite_transcript, save, ensure_loaded, has_any_sessions, suspend_recently_active, has_platform_message_id, prune_old_entries, record_gateway_session_peer, set_expiry_finalized, is_session_expired, is_session_finalizable. Wire the import at gateway/run.py:1788 to the correct location.
| bundle_result = build_bundle_invocation_message( | ||
| bundle_key, user_instruction, task_id=_quick_key | ||
| bundle_key, user_instruction, task_id=_quick_key, | ||
| platform=_bundle_plat, |
There was a problem hiding this comment.
🔴 build_bundle_invocation_message called with unexpected platform= parameter — TypeError at runtime (bug)
gateway/run.py:10191-10194 calls build_bundle_invocation_message(bundle_key, user_instruction, task_id=_quick_key, platform=_bundle_plat). The function in agent/skill_bundles.py:253-257 has signature (cmd_key: str, user_instruction: str = '', task_id: str | None = None) — no platform keyword argument. This raises TypeError: build_bundle_invocation_message() got an unexpected keyword argument 'platform' whenever a bundle slash command is dispatched in the gateway.
💡 Suggestion: Either add a platform parameter to build_bundle_invocation_message in agent/skill_bundles.py and forward it for per-platform skill-disabled checking, or remove the platform= kwarg from the gateway call site.
📋 Prompt for AI Agents
In agent/skill_bundles.py:253, add platform: Optional[str] = None to build_bundle_invocation_message's signature and forward it to the _load_skill_payload / skill-disabled checks inside the loop that loads each bundle skill. The gateway already resolves the platform correctly at line 10190. Alternatively, if the platform gate is not needed for bundles, remove the platform= argument from the call at gateway/run.py:10193.
| "approvals": { | ||
| "mode": "manual", | ||
| "mode": "smart", |
There was a problem hiding this comment.
🟠 approvals.mode default changed from 'manual' to 'smart' without migration — silent security posture downgrade (security)
DEFAULT_CONFIG['approvals']['mode'] in hermes_cli/config.py line 2550 changed from 'manual' (always prompt user for dangerous commands) to 'smart' (use auxiliary LLM to auto-approve low-risk commands). No corresponding migration was added. Users whose config.yaml does not have an explicit approvals.mode key — which is the normal state because save_config strips keys matching the old default — will silently shift from always-prompt to auto-approve on the next config load. The cron_mode remains 'deny' (safe for cron). This is a security posture downgrade: dangerous commands that would have required manual approval will now be auto-approved by the auxiliary LLM.
💡 Suggestion: Add a migration step that writes approvals.mode='manual' for existing configs that do not have the approvals section at all, preserving the old manual-approval behavior for existing users. New installs should get the new 'smart' default.
📋 Prompt for AI Agents
In migrate_config() in hermes_cli/config.py: add a migration block (if current_ver < 34) that reads the raw config; if approvals is not a dict or approvals.mode is absent, set approvals.mode='manual' and persist. This gates the behavior change behind an explicit version bump, giving existing users the old behavior and new users the new default.
| def warm_environment_probe_async() -> None: | ||
| """Kick off the probe in a background thread so the first | ||
| system-prompt build doesn't pay the ~0.5s of subprocess calls | ||
| (python3/pip/PEP-668 version checks) on the time-to-first-token | ||
| critical path. | ||
|
|
||
| Idempotent and fail-safe. The prompt-build call to | ||
| ``get_environment_probe_line`` takes the same ``_CACHE_LOCK``, so it | ||
| blocks only for whatever remains of an in-flight warm instead of | ||
| recomputing. Called from agent init (all platforms); safe to call | ||
| from anywhere. | ||
| """ | ||
| global _warm_started | ||
| if _warm_started or _CACHED_LINE is not None: | ||
| return | ||
| _warm_started = True | ||
| threading.Thread( | ||
| target=get_environment_probe_line, | ||
| name="env-probe-warm", | ||
| daemon=True, | ||
| ).start() |
There was a problem hiding this comment.
🟡 warm_environment_probe_async is dead code — env probe latency never reduced (bug)
The function warm_environment_probe_async() was added to tools/env_probe.py:247-267 to spawn a daemon background thread that pre-warms the environment probe cache so the first system-prompt build doesn't pay the ~0.5s of subprocess calls (python3/pip/PEP-668 version checks) on the time-to-first-token critical path. However, the function is never called — it has zero imports and zero callers anywhere in the codebase. The only consumer of env_probe is agent/system_prompt.py:342 which calls get_environment_probe_line() directly, blocking synchronously. The latency improvement intended by this function never takes effect.
💡 Suggestion: Call warm_environment_probe_async() from the agent initialization path — e.g., during AIAgent.__init__ or during the CLI's run() prewarm phase — so the warmup thread overlaps with other startup work and the probe is cached before the first system prompt is built.
📋 Prompt for AI Agents
In agent/prompt_builder.py or run_agent.py (AIAgent.init), add an early call to tools.env_probe.warm_environment_probe_async() so the background thread begins warming the probe cache during agent initialization, before the system prompt build calls get_environment_probe_line(). Keep the import lazy (inside the function) to avoid import-order issues.
| install = _run_setup_command( | ||
| install_cmd, | ||
| display=install_cmd, | ||
| shell=True, | ||
| timeout=300, | ||
| ) |
There was a problem hiding this comment.
🟡 Memory provider setup executes install commands from plugin.yaml with shell=True without sanitization (security)
The dashboard memory provider setup flow (POST /api/memory/providers/{name}/setup) reads external dependency install commands from the plugin's plugin.yaml manifest and passes them to _run_setup_command(install_cmd, shell=True) at hermes_cli/web_server.py:4671-4674. The check command from the same YAML section is safely split with shlex.split(), but the install command is not. If a malicious or compromised plugin.yaml is present on disk, the dashboard API will execute arbitrary shell commands with the server process's privileges. The provider name parameter is validated against a regex, so path traversal to arbitrary YAML files is blocked, limiting reachability.
💡 Suggestion: Use shlex.split(install_cmd) like the check_cmd path, or validate that the install command string does not contain shell metacharacters before passing to shell=True.
📋 Prompt for AI Agents
In hermes_cli/web_server.py, in _install_memory_provider_external_dependencies (around line 4669-4676), change the install command execution to use shlex.split(install_cmd) instead of passing the raw string with shell=True. If the install_cmd genuinely requires shell features, document why and implement input validation that rejects unsafe metacharacters.
| self._ensure_sandbox() | ||
| flag = "-lc" if login else "-c" | ||
| start = getattr(self._sandbox, "start", None) | ||
| if not callable(start): | ||
| kwargs: dict[str, Any] = {"timeout": timeout, "env": self._sandbox_env()} | ||
| if stdin_data is not None: | ||
| kwargs["input"] = stdin_data | ||
| result = self._sandbox.exec("bash", flag, cmd_string, **kwargs) |
There was a problem hiding this comment.
🟠 TOCTOU race on self._sandbox between _start_process and cancel in Tenki environment (bug)
In TenkiEnvironment, _start_process() (line 860-867) calls _ensure_sandbox() which populates self._sandbox under self._lock, then reads self._sandbox after the lock is released at line 862 and again at line 867. Concurrently, cancel() (line 932-937) can acquire self._lock and set self._sandbox = None. This creates a TOCTOU window where _start_process executes self._sandbox.exec() on a None reference, causing AttributeError. The interleaving: daemon thread runs _start_process → _ensure_sandbox() completes and releases lock → agent loop times out → cancel() acquires lock, sets self._sandbox = None → _start_process continues to line 867 and crashes.
💡 Suggestion: Capture self._sandbox into a local variable immediately after _ensure_sandbox() returns, and use the local reference throughout _start_process. Alternatively, hold self._lock for the entire sandbox usage block.
| self._ensure_sandbox() | |
| flag = "-lc" if login else "-c" | |
| start = getattr(self._sandbox, "start", None) | |
| if not callable(start): | |
| kwargs: dict[str, Any] = {"timeout": timeout, "env": self._sandbox_env()} | |
| if stdin_data is not None: | |
| kwargs["input"] = stdin_data | |
| result = self._sandbox.exec("bash", flag, cmd_string, **kwargs) | |
| self._ensure_sandbox() | |
| sandbox = self._sandbox | |
| flag = "-lc" if login else "-c" | |
| start = getattr(sandbox, "start", None) | |
| if not callable(start): | |
| kwargs: dict[str, Any] = {"timeout": timeout, "env": self._sandbox_env()} | |
| if stdin_data is not None: | |
| kwargs["input"] = stdin_data | |
| result = sandbox.exec("bash", flag, cmd_string, **kwargs) |
📋 Prompt for AI Agents
In tools/environments/tenki.py, in _start_process method (around line 860), add sandbox = self._sandbox immediately after self._ensure_sandbox(), then replace all subsequent self._sandbox references with the local sandbox variable. This prevents the TOCTOU race with cancel() which can set self._sandbox = None from another thread.
| f"HERMES_RPC_DIR={shlex.quote(f'{sandbox_dir}/rpc')} " | ||
| f"HERMES_RPC_TOKEN={shlex.quote(rpc_token)} " |
There was a problem hiding this comment.
🟡 RPC token leaked in shell command string for remote code execution backends (security)
In tools/code_execution_tool.py, the _execute_remote function generates an RPC token and embeds it into a shell command prefix (env_prefix) that is passed to env.execute(). For Docker backends, this resolves to docker exec <container> bash -c "HERMES_RPC_TOKEN=<token> python3 script.py". The token appears in the Docker exec process's command-line arguments, visible in ps aux and /proc/<pid>/cmdline on the host. The RPC token gates IPC between the code execution sandbox and the parent Hermes process; an attacker with host-level process-listing access could read the token and forge RPC requests. For local execution, the token is correctly placed only in the child process environment, not on the command line.
💡 Suggestion: Pass the RPC token to the remote backend via the environment mechanism exclusively (e.g., use docker exec --env HERMES_RPC_TOKEN=... instead of embedding it in the shell command string).
📋 Prompt for AI Agents
In tools/code_execution_tool.py _execute_remote(), replace the env_prefix approach that embeds HERMES_RPC_TOKEN in the shell command string. For Docker backends, extend the execute call to accept environment variables injected via --env or -e flags. For other remote backends, use their native environment-injection API.
| if ( | ||
| self._stream_buf | ||
| and not self._in_stream_table | ||
| and not self._stream_buf.lstrip().startswith("|") | ||
| ): |
There was a problem hiding this comment.
🟡 Streaming force-flush guard inconsistent with table-row detection — garbled tables without leading pipes (bug)
The streaming force-flush logic in _emit_stream_text (cli.py:5898-5923) wraps long partial lines at terminal width. It exempts table rows via not self._stream_buf.lstrip().startswith('|') to avoid breaking markdown tables mid-row. However, the table-detection function looks_like_table_row (agent/markdown_tables.py:83-102) also recognizes rows without a leading | — any line containing ≥2 pipe characters is treated as a potential table row and buffered for realignment. When a model emits a wide table in the no-leading-pipe style (e.g., Col1 | Col2 | Col3 | Col4), and the row exceeds terminal width before a newline arrives, the force-flush wraps the partial row, emitting the first ~40 chars as plain text. The remaining partial row then enters the table buffer and gets realigned alone, producing garbled column alignment.
💡 Suggestion: Replace the force-flush guard with a check that mirrors looks_like_table_row: skip force-flush when the partial buffer contains at least 2 pipe characters or starts with a pipe.
📋 Prompt for AI Agents
In cli.py, in _emit_stream_text, change the force-flush guard condition (around line 5910) from not self._stream_buf.lstrip().startswith('|') to self._stream_buf.lstrip().count('|') < 2 and not self._stream_buf.lstrip().startswith('|'). This mirrors the detection logic in agent/markdown_tables.py:looks_like_table_row which treats rows with ≥2 pipes as table rows regardless of leading pipe presence.
1 participant
What does this PR do?
Adds Tenki as a seventh terminal execution backend alongside
local,docker,ssh,singularity,modal, anddaytona. Withterminal.backend: "tenki", Hermes creates Tenki cloud sandboxes on demand for the terminal tool, file tools, andexecute_code, and terminates them on cleanup by default. Pause/resume persistence across sessions is opt-in viacontainer_persistent: true.The integration deliberately follows the existing modal/daytona pattern: an optional extra (
tenki-sandbox==0.1.1) that is lazy-installed on first use, aBaseEnvironmentsubclass intools/environments/, and the same setup-wizard / doctor / status / gateway wiring. Along the way it also deduplicates the three previously copy-pasted container-config dicts (terminal, file tools, execute_code) into one shared_container_config_from_env_config()helper, so future backends only need to touch one place.Security hardening is built in rather than bolted on:
terminal.tenki_forward_env, and forwarding the control-plane token logs a warning.agent.secret_scope, so an active profile scope wins over process-globalos.environand the shared machine CLI login is skipped when a profile scope is authoritative.TENKI_AUTH_TOKEN/TENKI_API_KEYare stripped from spawned subprocess environments (provider blocklist + always-strip tier), matching modal/daytona.Known follow-up (pre-existing and backend-agnostic, not introduced here): the process-global terminal environment cache (
_active_environments, keyed"default") is not profile-scoped, so under the multiplexing gateway forwarded credentials are not isolated across profiles. Documented in the credential-forwarding notes and tracked separately.Related Issue
No existing issue — this is a new backend integration in the same vein as the open E2B (NousResearch#18348) and Sprites (NousResearch#30112) backend PRs. Happy to open a tracking issue if maintainers prefer.
Type of Change
Changes Made
Core backend
tools/environments/tenki.py(new) —TenkiEnvironment: sandbox lifecycle, exec, pause/resume persistence, remote file sync-backtools/tenki_config.py(new) — profile-scope-aware resolution of auth token, workspace, project, and API endpoint from the Tenki CLI config or environmenttools/terminal_tool.py,tools/file_tools.py,tools/code_execution_tool.py— backend wiring; shared_container_config_from_env_config()helper replaces the three duplicated container-config dictstools/environments/__init__.py,tools/environments/base.py,tools/environments/local.py— registration and base-class support/home/tenki/*) as a valid cwdSecurity
tools/approval.py,tools/env_probe.py,tools/file_operations.py—TENKI_AUTH_TOKEN/TENKI_API_KEYadded to the provider blocklist and always-strip tierterminal.tenki_forward_env(see above)CLI / UX
hermes_cli/setup.py— setup-wizard option for the Tenki backendhermes_cli/doctor.py,hermes_cli/status.py— Tenki auth/SDK checks and backend statuscli.py,gateway/run.py,hermes_cli/config.py— config plumbing; blanktenki_api_endpointdefault in both config loaders so the documented env/CLI fallback is reachablePackaging
pyproject.toml,uv.lock,tools/lazy_deps.py,nix/packages.nix— optionaltenkiextra (tenki-sandbox==0.1.1), lazy-installed like modal/daytonaDocs & config
cli-config.yaml.example— "OPTION 7: Tenki cloud execution" block with alltenki_*keyswebsite/docs/— configuration guide, environment-variable reference, security notes, architecture pageAGENTS.md,CONTRIBUTING.md— backend lists updated to include tenkiTests
tests/tools/test_tenki_environment.py(new, ~1,300 lines) — lifecycle, exec, persistence, durability gate, restore classification, profile scopingtest_terminal_config_env_sync.py,test_terminal_requirements.py,test_terminal_tool_requirements.py,test_file_tools_container_config.py,test_parse_env_var.py,test_container_cwd_sanitize.py,test_local_env_blocklist.py,test_hardline_blocklist.py,test_command_guards.py,tests/hermes_cli/test_setup.py, and othersHow to Test
pip install 'hermes-agent[tenki]'(or let lazy install handle it on first use) and authenticate viatenki loginorTENKI_AUTH_TOKEN/TENKI_API_KEY.cli-config.yaml, setterminal.backend: "tenki"(see the new OPTION 7 block incli-config.yaml.examplefor all keys).hermes -q "run uname -a in the terminal"— a Tenki sandbox is created on demand and terminated on cleanup. Exercise file tools andexecute_codethe same way.hermes doctorandhermes statusreport Tenki SDK/auth state and backend status.container_persistent: true, run a session, exit, run again — the sandbox pauses on cleanup and resumes on the next session.pytest tests/ -q— full suite passes.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (one environment-specific failure on macOS,test_approval.py::TestDetectDangerousRm::test_nonrecursive_verification_artifact_cleanup_is_not_dangerous, fails identically at the merge-base without this change — pre-existing, unrelated)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Targeted run of every test file touched by this PR:
Full suite via
scripts/run_tests.sh(same as CI) also run on macOS 26.5.1: green except one pre-existing, environment-specific failure (tests/tools/test_approval.py::TestDetectDangerousRm::test_nonrecursive_verification_artifact_cleanup_is_not_dangerous), which fails identically at the merge-base (c44de9985) without this change.Mirror-of: NousResearch#64190
NousResearch#64190