feat(egress): iron-proxy credential-injection firewall for sandboxes - #30179
Conversation
🔎 Lint report:
|
| Rule | Count |
|---|---|
unsupported-operator |
11 |
unresolved-attribute |
8 |
invalid-argument-type |
4 |
unresolved-import |
3 |
invalid-assignment |
1 |
First entries
hermes_cli/config.py:5225: [unresolved-attribute] unresolved-attribute: Attribute `items` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/test_iron_proxy_e2e.py:23: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/test_iron_proxy.py:23: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/run_agent/test_in_place_compaction.py:257: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/tools/test_web_providers.py:218: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["search_backend"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/test_iron_proxy.py:1209: [invalid-assignment] invalid-assignment: Object of type `_CAStub` is not assignable to attribute `ca_cert_path` of type `Path | None`
hermes_cli/config.py:5235: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/tools/test_refresh_agent_mcp_tools.py:257: [invalid-argument-type] invalid-argument-type: Argument to constructor `float.__new__` is incorrect: Expected `str | Buffer | SupportsFloat | SupportsIndex`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/agent/test_curator.py:1105: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["curator"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/tools/test_web_providers.py:219: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["extract_backend"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/cli/test_resume_display.py:716: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["resume_display"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/tools/test_web_providers.py:217: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["backend"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tools/browser_tool.py:1196: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/hermes_cli/test_aux_config.py:54: [unresolved-attribute] unresolved-attribute: Attribute `keys` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/tools/test_browser_lightpanda.py:242: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["engine"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
hermes_cli/mcp_startup.py:67: [invalid-argument-type] invalid-argument-type: Argument to constructor `float.__new__` is incorrect: Expected `str | Buffer | SupportsFloat | SupportsIndex`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/hermes_cli/test_kanban_core_functionality.py:3369: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/cli/test_fast_command.py:484: [invalid-argument-type] invalid-argument-type: Argument to bound method `TestCase.assertIn` is incorrect: Expected `Iterable[Any] | Container[Any]`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/tools/test_browser_console.py:341: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["record_sessions"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/cli/test_reasoning_command.py:552: [invalid-argument-type] invalid-argument-type: Argument to bound method `TestCase.assertIn` is incorrect: Expected `Iterable[Any] | Container[Any]`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/cron/test_suggestions.py:213: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["monitor"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/test_iron_proxy_cli.py:17: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/hermes_cli/test_aux_config.py:37: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["title_generation"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/hermes_cli/test_destructive_slash_confirm_gate.py:32: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements`
tests/gateway/test_whatsapp_reply_prefix.py:119: [unsupported-operator] unsupported-operator: Operator `>=` is not supported between objects of type `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 36 union elements` and `int`
... and 2 more
✅ Fixed issues (23):
| Rule | Count |
|---|---|
unsupported-operator |
11 |
unresolved-attribute |
8 |
invalid-argument-type |
4 |
First entries
tests/hermes_cli/test_kanban_core_functionality.py:3369: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/hermes_cli/test_aux_config.py:54: [unresolved-attribute] unresolved-attribute: Attribute `keys` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/tools/test_web_providers.py:219: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["extract_backend"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/cron/test_suggestions.py:213: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["monitor"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/tools/test_web_providers.py:217: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["backend"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/cli/test_fast_command.py:484: [invalid-argument-type] invalid-argument-type: Argument to bound method `TestCase.assertIn` is incorrect: Expected `Iterable[Any] | Container[Any]`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/tools/test_browser_console.py:341: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["record_sessions"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/hermes_cli/test_aux_config.py:37: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["title_generation"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/cli/test_resume_display.py:716: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["resume_display"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/tools/test_refresh_agent_mcp_tools.py:257: [invalid-argument-type] invalid-argument-type: Argument to constructor `float.__new__` is incorrect: Expected `str | Buffer | SupportsFloat | SupportsIndex`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/cli/test_reasoning_command.py:552: [invalid-argument-type] invalid-argument-type: Argument to bound method `TestCase.assertIn` is incorrect: Expected `Iterable[Any] | Container[Any]`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/hermes_cli/test_mcp_reload_confirm_gate.py:33: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/tools/test_browser_lightpanda.py:242: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["engine"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/hermes_cli/test_destructive_slash_confirm_gate.py:32: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/agent/test_curator.py:1105: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["curator"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/run_agent/test_in_place_compaction.py:257: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
hermes_cli/mcp_startup.py:67: [invalid-argument-type] invalid-argument-type: Argument to constructor `float.__new__` is incorrect: Expected `str | Buffer | SupportsFloat | SupportsIndex`, found `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/hermes_cli/test_aux_config.py:47: [unsupported-operator] unsupported-operator: Operator `not in` is not supported between objects of type `Literal["session_search"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
hermes_cli/config.py:5173: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/gateway/test_whatsapp_reply_prefix.py:119: [unsupported-operator] unsupported-operator: Operator `>=` is not supported between objects of type `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements` and `int`
hermes_cli/config.py:5163: [unresolved-attribute] unresolved-attribute: Attribute `items` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tests/tools/test_web_providers.py:218: [unsupported-operator] unsupported-operator: Operator `in` is not supported between objects of type `Literal["search_backend"]` and `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
tools/browser_tool.py:1196: [unresolved-attribute] unresolved-attribute: Attribute `get` is not defined on `str`, `list[Unknown]`, `list[str]`, `None`, `int`, `float` in union `str | dict[Unknown, Unknown] | list[Unknown] | ... omitted 35 union elements`
Unchanged: 5992 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
daimon-nous
Bot
added
type/feature
waefrebeorn
left a comment
waefrebeorn
left a comment
There was a problem hiding this comment.
Triple Devil's Advocate Review
Verdict: APPROVE with suggestions
Reviewed from the perspective of the python-to-c translation PR stream (provider creation patterns, Docker integration surfaces). One actionable finding, two documentation notes.
DA-1: Claims vs Code (all verified)
| Claim | Result |
|---|---|
| Real credentials never written to proxy.yaml | ✅ — {type: env, var: NAME} in config, not values |
| SHA-256 verified binary download | ✅ — downloads both archive + checksums.txt, compares |
| 35 hermetic + 1 E2E test | ✅ — 533-line test file + 165-line E2E |
| Lazy install pattern (Bitwarden-compatible) | ✅ — find_iron_proxy(install_if_missing=True) |
| Docker CA/proxy/env injection | ✅ — _egress_proxy_args_for_docker() integrated in _create_container() |
| Bearer-token providers only (scope cut) | ✅ — 9 providers in _BEARER_PROVIDERS, custom auth deferred |
DA-2: Security Risks
🔴 Asymmetric CA enforcement (Python vs Node.js)
The Docker env vars set:
REQUESTS_CA_BUNDLE/SSL_CERT_FILE/CURL_CA_BUNDLE— replace the system bundleNODE_EXTRA_CA_CERTS— adds to the system bundle (does not replace)
Inside the sandbox, Python/curl can ONLY trust the proxy CA. Node.js still trusts the full system CA AND the proxy CA. If a process inside the sandbox bypasses HTTPS_PROXY (e.g., raw net.Socket with TLS), Node.js would succeed where Python/curl would fail cert validation.
Suggestion: Either drop NODE_EXTRA_CA_CERTS (document as manual config), or use NODE_OPTIONS=--use-openssl-ca-store to force Node.js through the OpenSSL store that SSL_CERT_FILE controls.
🟡 Broad except in _egress_proxy_args_for_docker()
except Exception as exc:
logger.debug("Egress proxy plumbing unavailable: %s", exc)
return ([], {}, [])If load_config() fails for reasons beyond proxy-not-installed (corrupt YAML, etc.), the Docker environment silently continues without enforcement. The noqa: BLE001 already flags this. Low risk in practice (corrupt config breaks everything), but worth noting the silent degradation.
🟢 Token entropy: sha256(os.urandom(32)).hexdigest()[:32] = 128 bits. Strong enough.
🟢 CodeQL URL sanitization: Test-only false positive (only.example.com in mock data, not in enforcement logic).
DA-3: Gap Prioritization
| Prio | Gap | Effort |
|---|---|---|
| P0 | Node.js asymmetric CA enforcement | ~2 lines + docs |
| P1 | Non-bearer providers (17+ custom providers from python-to-c translation not protected) | Per-provider rules, medium |
| P2 | Non-Docker backends (Modal/SSH) | Separate PRs |
| P3 | checksums.txt same-origin as binary (accepted pattern) | cosign, low urgency |
Relevant to our PR stream
The proxy: config section and Docker env merging pattern are new surfaces that our provider creation PRs will need to account for. Specifically:
- Any provider that writes API keys to env at container launch should route through the proxy's token system instead
- The
extra_allowed_hostslist in config is the integration point for custom providers - Config schema merge conflicts are likely if both PRs land close together
Reviewed by Hermes Agent
|
does iron-proxy expose a health endpoint or status check? if it crashes silently the sandbox would keep calling it and getting auth failures — might be worth a watchdog or at least a startup check before accepting tasks |
|
looks like the broad except in _egress_proxy_args_for_docker() is already flagged in the review — good catch. the P1 gap around non-bearer providers is the real concern imo, if those bypass the proxy the isolation guarantee breaks for anyone using custom endpoints |
annguyenNous
left a comment
annguyenNous
left a comment
There was a problem hiding this comment.
Security Review: iron-proxy egress firewall
Reviewed the full diff (12 files, +2403/-3). Overall architecture is solid — TLS-intercepting proxy with default-deny allowlist, token swapping at the network boundary, Bitwarden integration. Real defense-in-depth for sandbox isolation. A few security gaps worth addressing before merge.
P0 — Must Fix
1. CA private key TOCTOU race window
In ensure_ca_cert(), the key is copied via shutil.copy2 then os.chmod is called after. Between those two calls, the private key exists with the default umask (potentially world-readable).
# Current (race window):
shutil.copy2(tmp_key, ca_key) # <-- default umask permissions
os.chmod(ca_key, 0o600) # <-- too late
# Fix: write with explicit fd + fchmod + atomic replace
fd = os.open(str(ca_key_staged), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
os.write(fd, key_bytes)
os.close(fd)
os.replace(ca_key_staged, ca_key)2. Non-bearer providers bypass the proxy entirely
_BEARER_PROVIDERS only covers 8 providers using plain Bearer auth. Providers using SigV4 (AWS Bedrock), x-api-key (Azure OpenAI), AAD tokens, or custom auth headers are not swapped — the sandbox holds real credentials for those.
Suggested fix:
- Add
proxy.fail_on_uncovered_providers: trueconfig option - Check configured providers against
_BEARER_PROVIDERSatstart_proxy()time hermes egress statusshould show COVERED vs UNCOVERED providers with a warning
3. Proxy token entropy truncated to 128 bits
hashlib.sha256(os.urandom(32)).hexdigest()[:32] — 32 hex chars = 128 bits, not the 256 bits from os.urandom(32). 128 bits is adequate for proxy-scoped tokens, but the docstring claims "long random suffix so collisions are infeasible" without stating the actual entropy. Either use full hex or document the deliberate truncation.
P1 — Should Fix
4. Broad except Exception in _egress_proxy_args_for_docker()
except Exception as exc: # catches SyntaxError, AttributeError, etc.Should be except (ImportError, FileNotFoundError) to avoid masking real bugs.
5. SHA-256 checksum from same download channel
Both the binary and checksums.txt come from the same GitHub Releases URL. If the download channel is compromised, attacker substitutes both. Consider embedding expected checksums in source code alongside _IRON_PROXY_VERSION.
6. Audit log file permissions not set
The audit log at ~/.hermes/proxy/audit.log contains every outbound request (hosts, headers, timing). No explicit chmod call — depends on umask. Should be 0o600.
7. PID file has no locking
(state / "iron-proxy.pid").write_text(str(proc.pid))Two concurrent hermes egress start calls race. Use fcntl.flock or O_CREAT | O_EXCL.
P2 — Follow-up
- Health endpoint watchdog — if proxy crashes silently, sandboxes get auth failures. Add
hermes egress healthor a periodic check. - CA rotation CLI —
hermes egress rotate-cafor compromised CA scenarios. - Rate limiting — compromised sandbox could DDoS upstream providers through the proxy.
--show-tokenswarning — truncate display, warn on stderr about terminal history leakage.
Existing Issues
- CodeQL flagged "Incomplete URL substring sanitization" on test line 143 — worth confirming if it's a false positive.
- The health endpoint question from @erhnysr is valid — silent proxy crash = sandbox auth failures.
Positive Notes
- SHA-256 binary verification ✓
- Atomic binary install (mkstemp + os.replace) ✓
- CA key permissions (0o600) ✓
- Private key never inlined in config ✓
enforce_on_dockerdefaults to true (fail-closed) ✓- Cloud metadata IPs denied by default ✓
- Good test coverage (35 unit + 1 E2E) ✓
Recommendation: Request changes for P0 #1 (CA key TOCTOU) and #2 (non-bearer provider gap). The rest can be tracked as follow-up issues.
| continue | ||
| if member.name.startswith("/") or ".." in Path(member.name).parts: | ||
| continue | ||
| if Path(member.name).name == binary_name: |
There was a problem hiding this comment.
[P0] CA key TOCTOU race — shutil.copy2 inherits default umask before os.chmod sets 0o600. There's a window where the private key is world-readable.
Fix: use os.open with explicit mode + os.replace:
fd = os.open(str(staged), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
os.write(fd, key_bytes)
os.close(fd)
os.replace(staged, ca_key)There was a problem hiding this comment.
Fixed in ea5e937. CA private key is now written via os.open(..., O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0o600) with an explicit atomic os.replace. New regression test test_ca_key_created_with_0o600 asserts the resulting perms.
|
|
||
| # Pinned upstream version. Bump in a follow-up PR — never auto-resolve "latest" | ||
| # because upstream YAML schema is allowed to change between releases and we | ||
| # want updates to be deliberate. |
There was a problem hiding this comment.
[P0] Non-bearer providers bypass proxy — Only 8 providers are covered. Any provider using custom auth (SigV4, x-api-key, AAD tokens) has its real credentials exposed in the sandbox.
Suggest adding proxy.fail_on_uncovered_providers: true config option and a check at start_proxy() time that compares configured providers against this map.
There was a problem hiding this comment.
Fixed in ea5e937. Added discover_uncovered_providers() + proxy.fail_on_uncovered_providers config option. When strict, hermes egress start refuses to start if any of {ANTHROPIC,AZURE_OPENAI,AWS_ACCESS_KEY_ID,...} are set in env. In non-strict mode the wizard + status surface them with a warning. Default is non-strict because false positives (operator has the env set but doesn't use that provider) are common.
| return target | ||
|
|
||
|
|
||
| def _http_download(url: str, dest: Path) -> None: |
There was a problem hiding this comment.
[P1] Token entropy is 128 bits, not 256 — .hexdigest()[:32] truncates to 32 hex chars = 128 bits. Adequate for proxy-scoped tokens but the docstring should document the actual entropy. Consider using full .hexdigest() or documenting: "128-bit suffix — collision probability < 2^-64 for up to 2^32 tokens".
There was a problem hiding this comment.
Documented in ea5e937. The 128-bit truncation is intentional — kept it as-is and updated the docstring with the birthday-bound math explicitly. 32 hex chars × 4 bits = 128 bits; collision probability < 2^-64 up to 2^32 tokens, which is plenty for a proxy-scoped namespace.
| the iron-proxy egress firewall. | ||
|
|
||
| Returns ``(volume_args, env_overrides, host_args)``: | ||
|
|
There was a problem hiding this comment.
[P1] Broad except Exception — catches SyntaxError, AttributeError, etc. Should be except (ImportError, FileNotFoundError) to avoid masking real bugs in config loading or proxy module.
There was a problem hiding this comment.
Adjacent: same anti-pattern at agent/proxy_sources/iron_proxy.py:879 in _read_tunnel_port_from_config (bare except Exception falls back to the default port and hides config-file issues). A single grep pass on except Exception in the new module would catch both in one go.
There was a problem hiding this comment.
Fixed in ea5e937. Narrowed to except ImportError. The GodsBoy follow-up about the sibling site at iron_proxy.py:_read_tunnel_port_from_config is also fixed — narrowed to (OSError, yaml.YAMLError).
There was a problem hiding this comment.
Fixed in ea5e937. Narrowed _read_tunnel_port_from_config's bare except to (OSError, yaml.YAMLError) with a separate ImportError guard around the yaml import. No more masking config-file issues at the default-port fallback.
There was a problem hiding this comment.
The narrowing to except ImportError makes sense but I want to check the behavior when it does fire (partial update, corrupted install). This returns empty args and the sandbox starts without proxy settings, even if the operator has enforce_on_docker: true in config. But we can't read that flag because the config module itself failed to import.
I notice _egress_enforce_on_docker already handles this case by defaulting to True when config is unreadable. But it's never reached if the import fails here first. Is the intended contract "if the proxy plumbing can't load, degrade gracefully" or "if enforcement was expected, refuse to start"? If the latter, could this handler check _egress_enforce_on_docker()or a fallback env var before returning empties?
| if install_if_missing: | ||
| try: | ||
| return install_iron_proxy() | ||
| except Exception as exc: # noqa: BLE001 — never block startup |
There was a problem hiding this comment.
[P1] PID file not locked — Two concurrent hermes egress start calls will race. Use fcntl.flock(fd, LOCK_EX | LOCK_NB) or O_CREAT | O_EXCL.
There was a problem hiding this comment.
Fixed in ea5e937. Pidfile written via os.open(..., O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0o600) with an os.fstat().st_uid == os.getuid() ownership check. O_NOFOLLOW refuses to follow a pre-existing symlink; the uid check catches a same-uid race that won. ELOOP from a pre-existing symlink is surfaced as a clear RuntimeError naming the path.
GodsBoy
left a comment
GodsBoy
left a comment
There was a problem hiding this comment.
Code review summary
Verdict: not ready to merge.
Two P0s are exploitable as designed:
- Missing IMDS deny in the wizard-generated
proxy.yaml(docs promise it; config does not emit it). - Proxy binds 0.0.0.0 instead of loopback + docker bridge.
Eight P1s either undermine the credential-isolation posture the feature exists to provide (host-secret leak via os.environ.copy, env-merge bypass of HTTPS_PROXY, --from-bitwarden silent degrade) or break it in operationally surprising ways (rotation guarantee undelivered, audit_log silently dropped, wrong command names in 11+ user strings, fd leak per restart, mappings.json clobber on re-setup breaking live sandboxes).
Several of these have safe, mechanical fixes (defaults, find/replace, close the fd, wire the parameter). The rest take design judgment.
Suggested fix order
- P0 mechanical: default
upstream_deny_cidrs; bind to loopback + bridge IP. - P1 mechanical: wire
audit_log; renamehermes proxy->hermes egressin user strings; closelog_fpon the happy path. - P1 design: minimize subprocess env; reconcile
enforce_on_dockervs the docker_env precedence; either implement bitwarden refresh instart_proxyor revert the knob and update docs; preserve tokens onsetupre-run. - P2 batch: silent fail-open in docker.py:236,
load_mappingssilent-corruption + still-mounting, the rest can land in a follow-up.
Prior comments
Existing review feedback from @annguyenNous and @waefrebeorn covers several other concerns (CA private key TOCTOU race, non-bearer provider bypass, Node.js asymmetric CA enforcement via NODE_EXTRA_CA_CERTS, SHA-256 supply-chain pinning, PID file race, audit-log permissions, token entropy disclosure, --show-tokens history warning). Those remain unaddressed at HEAD and are deliberately not re-flagged inline below to avoid thread noise. They should be folded into the same revision pass.
Testing
The proxy_cli.py command handlers (7 of them) have zero unit-test coverage. The E2E exercises plain HTTP only; the HTTPS CONNECT + TLS-MITM path that is the primary production mechanism is unexercised. stop_proxy's SIGKILL escalation path is never run. Adding these would also catch several of the inline P1/P2 findings as regressions.
Inline findings follow.
| # default. Tests / dev setups that need loopback can pass an | ||
| # explicit override (e.g. [] to disable, or just the IMDS subset). | ||
| **( | ||
| {"upstream_deny_cidrs": list(upstream_deny_cidrs)} |
There was a problem hiding this comment.
Severity: P0
build_proxy_config only emits upstream_deny_cidrs when the caller passes a non-None value. cmd_setup in proxy_cli.py:226 never passes it, so the wizard-generated proxy.yaml has no deny list.
User-facing docs in website/docs/user-guide/egress/iron-proxy.md (lines 146, 161) explicitly promise that 169.254.169.254 (cloud metadata) is refused by upstream_deny_cidrs regardless of allowlist. Today the protection depends entirely on iron-proxy upstream's default behavior, which is unverified. DNS rebinding through an allowlisted host reaches IMDS with real keys swapped in.
Fix: default upstream_deny_cidrs to ['127.0.0.0/8','::1/128','169.254.0.0/16','10.0.0.0/8','172.16.0.0/12','192.168.0.0/16','fc00::/7','fe80::/10'] when caller passes None. Treat only an explicit [] as opt-out, and add a test asserting the wizard-rendered yaml contains the deny list. Surfaced by security, correctness, adversarial, and learnings reviewers.
There was a problem hiding this comment.
Fixed in ea5e937. build_proxy_config now emits the default deny list when caller passes None — covers loopback (v4 + v6), 169.254.0.0/16 (IMDS), fe80::/10, and RFC1918. Explicit [] opts out. New tests test_default_deny_cidrs_present_when_unspecified and test_wizard_rendered_yaml_contains_deny_list regression-guard this.
| # `HTTPS_PROXY=http://host:tunnel_port` and the same listener | ||
| # serves both protocols. Bind on all interfaces so containers | ||
| # can reach it via host.docker.internal. | ||
| "http_listen": f":{tunnel_port}", |
There was a problem hiding this comment.
Severity: P0
http_listen: ':9090' binds INADDR_ANY. Any host on the local network can reach the proxy. Combined with a leaked HERMES_PROXY_TOKEN_* (which is visible to anyone who can run docker inspect against a Hermes container, including all same-uid local processes), this lets a LAN peer spend the user's API quota against any allowlisted upstream.
The justification 'so containers can reach it via host.docker.internal' is wrong on Linux: the --add-host=host.docker.internal:host-gateway arg added in docker.py:572 resolves to the docker bridge IP (typically 172.17.0.1), not to all interfaces.
Fix: bind to 127.0.0.1:9090; on Linux additionally bind the docker bridge gateway. Do not use INADDR_ANY. Surfaced by security and adversarial reviewers.
There was a problem hiding this comment.
Fixed in ea5e937. Default bind is 127.0.0.1:<port> + the docker0 bridge IP on Linux (auto-detected via ip -4 addr show docker0). Never 0.0.0.0 / :PORT. Tests test_default_bind_is_loopback_not_zero_zero and test_default_bind_includes_docker_bridge_on_linux regression-guard both directions.
| "Run `hermes proxy setup` first." | ||
| ) | ||
|
|
||
| env = os.environ.copy() |
There was a problem hiding this comment.
Severity: P1
cmd_setup --from-bitwarden writes credential_source: bitwarden to config (proxy_cli.py:243). The user-facing docs in website/docs/user-guide/egress/iron-proxy.md promise: 'rotating a key in BW propagates on the next proxy restart'.
Reality: start_proxy() only does os.environ.copy(). There is no bws re-fetch. Rotated keys do not propagate. The rotation guarantee that distinguishes the bitwarden source from the env source is undelivered.
Fix: when credential_source == 'bitwarden', call agent.secret_sources.bitwarden.fetch_bitwarden_secrets() at startup and merge into extra_env. If that is out of scope for v1, revert the config knob and correct the docs. Surfaced by security, correctness, and api-contract reviewers.
There was a problem hiding this comment.
Fixed in ea5e937. start_proxy now accepts refresh_secrets_from_bitwarden + bitwarden_config kwargs; when set, fetches via bw.fetch_bitwarden_secrets() at startup and merges the matching env names into the subprocess env. cmd_start wires this in when credential_source == "bitwarden" AND secrets.bitwarden.enabled == true. Rotation now actually propagates to the proxy on restart.
| ca_cert: Path, | ||
| ca_key: Path, | ||
| tunnel_port: int = _DEFAULT_TUNNEL_PORT, | ||
| audit_log: Optional[Path] = None, |
There was a problem hiding this comment.
Severity: P1
build_proxy_config(..., audit_log: Optional[Path] = None, ...) (this signature line) accepts the parameter, but the function body never references it. The call site in proxy_cli.py:231 passes audit_log=ip._proxy_state_dir() / 'audit.log' expecting an audit log to materialize. It does not. Operators relying on this for security forensics have no log.
Fix: wire audit_log into the rendered log: block of the yaml, or drop the parameter entirely and update the docstring + caller. Add a test asserting the rendered config contains the audit-log path. Surfaced by 5 reviewers.
There was a problem hiding this comment.
Fixed in ea5e937. audit_log now lands in the rendered yaml at log.audit_path. New ensure_audit_log() helper pre-creates the file with 0o600 so iron-proxy inherits tight perms instead of relying on umask. New tests test_audit_log_path_lands_in_yaml and test_ensure_audit_log_creates_with_0o600 cover the wire-up.
| bin_path = binary or find_iron_proxy(install_if_missing=True) | ||
| if bin_path is None: | ||
| raise RuntimeError( | ||
| "iron-proxy binary not available — run `hermes proxy install`." |
There was a problem hiding this comment.
Severity: P1
Eleven user-facing strings across iron_proxy.py and proxy_cli.py reference hermes proxy ... but the registered command is hermes egress .... Copy-pasting any of these messages hits the existing inbound OAuth proxy command, which has different verbs, giving a confusing 'argument not recognized' error.
Locations:
iron_proxy.py:26, 39, 271(module docstring)iron_proxy.py:766, 773(this RuntimeError and the next)proxy_cli.py:~252, ~283(post-setup banner and start-error string), plus several additional help strings
Fix: find-and-replace hermes proxy -> hermes egress in user-facing strings only (do not touch references to the inbound OAuth hermes proxy command in unrelated code). Surfaced by correctness, maintainability, and api-contract reviewers.
There was a problem hiding this comment.
Fixed in ea5e937. All user-facing hermes proxy ... strings in iron_proxy.py and proxy_cli.py renamed to hermes egress .... Did not touch the inbound OAuth hermes proxy references elsewhere in main.py.
| binary = find_iron_proxy(install_if_missing=False) | ||
| if binary: | ||
| status.binary_path = binary | ||
| status.binary_version = iron_proxy_version(binary) |
There was a problem hiding this comment.
Severity: P3
subprocess.run([binary, '--version'], timeout=30) is invoked from get_status, which the docker backend now calls per-container-create. A hung version invocation stalls container creation for 30 seconds. Version is a constant for a given binary path.
Fix: module-level dict cache keyed by binary path. Reliability reviewer flagged.
There was a problem hiding this comment.
Fixed in ea5e937. iron_proxy_version now caches by binary path via a module-level _VERSION_CACHE dict. First call costs one subprocess; subsequent calls in the same process are dict lookups. No more 30s stall per Docker container create on a hung binary.
| # Pinned upstream version. Bump in a follow-up PR — never auto-resolve "latest" | ||
| # because upstream YAML schema is allowed to change between releases and we | ||
| # want updates to be deliberate. | ||
| _IRON_PROXY_VERSION = "0.39.0" |
There was a problem hiding this comment.
Severity: P3
import sys is not referenced anywhere in this file.
Fix: remove. Safe auto-fix. Kieran-python reviewer flagged.
| proxy_cfg = cfg.setdefault("proxy", {}) | ||
| tunnel_port = ( | ||
| args.tunnel_port | ||
| if args.tunnel_port |
There was a problem hiding this comment.
Severity: P3
args.tunnel_port if args.tunnel_port else ... treats 0 as falsy and silently falls back to the config default. Argparse sets the absent-flag default to None.
Fix: args.tunnel_port if args.tunnel_port is not None else .... (Note: 0 is not a valid TCP port, but failing-loud is better than silently substituting.) Kieran-python reviewer flagged.
There was a problem hiding this comment.
Fixed in ea5e937. Now args.tunnel_port is not None. 0 is explicitly rejected with a clear error rather than silently substituting the default.
| proxy_cfg["enabled"] = False | ||
| save_config(cfg) | ||
| console.print("[green]✓[/green] proxy.enabled set to false") | ||
| if ip._read_pid() is not None: |
There was a problem hiding this comment.
Severity: P3
ip._read_pid() reaches into a private symbol from a different module and trusts the pidfile content without verifying the process is actually alive. A stale pidfile from a crashed previous run causes the 'proxy is still running' warning to fire spuriously.
Fix: use ip.get_status().pid is not None which already incorporates _pid_alive. This also removes the cross-module underscore access. Correctness reviewer flagged.
There was a problem hiding this comment.
Fixed in ea5e937. cmd_disable now uses ip.get_status().pid is not None instead of reaching into ip._read_pid(). Status pid already incorporates the _pid_alive check, so a stale pidfile from a crashed run no longer fires the spurious "still running" warning.
| m = _sample_mapping() | ||
| cfg = ip.build_proxy_config( | ||
| mappings=[m], | ||
| ca_cert=Path("/tmp/ca.crt"), |
There was a problem hiding this comment.
Severity: P3
Lines 106, 130, 137, 171 hardcode Path('/tmp/ca.crt') and Path('/tmp/ca.key') for test assertions. AGENTS.md requires tests to use tmp-path fixtures so they're hermetic across host configurations and Windows / CI variants.
Fix: replace with tmp_path-derived paths constructed via the existing hermes_home fixture. Project-standards reviewer flagged.
There was a problem hiding this comment.
Fixed in ea5e937. All hardcoded Path("/tmp/ca.*") test paths replaced with tmp_path-derived fixtures. Tests are now hermetic across hosts and Windows / CI variants.
|
Thanks to everyone for the careful review — this is exactly the kind of feedback the PR needed before merge. Pushed ea5e937e1 addressing the findings. Quick map of what landed vs deferred: P0 — all addressed
P1 — all addressed
P2 — all addressed
P3 — all addressed
CI
TestsAdded 32 new tests in All 156 tests across Acknowledged but deferred
@erhnysr — re your health-endpoint question: the new poll-with-timeout in Thanks again all — review comments queued under the individual threads where appropriate. |
|
went through the full review thread — impressive depth from @pmos69. the _find_proxy_path() startup validation addresses the crash concern directly, and the non-bearer provider gap being acknowledged but deferred makes sense given scope. the grace shutdown + SIGTERM handling was the right call to add before merge. one thing worth tracking as a follow-up: the Node.js asymmetric CA enforcement gap. if a sandboxed Node.js process can still trust the full system CA alongside the proxy CA, the isolation guarantee weakens for mixed Python/Node stacks. might be worth a separate issue to track that specifically. |
thanks for the detailed follow-up @teknium1 — the poll-with-timeout approach is clean, _port_listening probe before pidfile write is exactly the right place to catch a silent crash. happy to take the |
| "SSL_CERT_FILE": container_ca, # Python ssl module / OpenSSL | ||
| "CURL_CA_BUNDLE": container_ca, # curl | ||
| "NODE_EXTRA_CA_CERTS": container_ca, # Node.js: adds to system store | ||
| "NODE_OPTIONS": "--use-openssl-ca", # Node.js: route through OpenSSL store |
There was a problem hiding this comment.
NODE_OPTIONS should append --use-openssl-ca to existing value, not clobber it — user's --max-old-space-size or other Node flags get silently dropped
There was a problem hiding this comment.
Fixed in fa4e87b. The egress dict no longer assigns NODE_OPTIONS directly — it carries the flag in a sentinel key _HERMES_EGRESS_NODE_OPTIONS_APPEND and DockerEnvironment merges into the operator's existing NODE_OPTIONS in env_args computation, with de-duplication on identical flags. User's --max-old-space-size=8192 etc. are preserved. New test test_docker_egress_node_options_uses_sentinel regression-guards the sentinel pattern.
| │ | ||
| │ structured audit log | ||
| ▼ | ||
| ~/.hermes/proxy/iron-proxy.log |
There was a problem hiding this comment.
"structured audit log" points to iron-proxy.log but the configured audit path is audit.log — iron-proxy.log is daemon stdout, not per-request audit records
There was a problem hiding this comment.
Fixed in fa4e87b. Diagram + step 7 of the data-flow section now correctly point per-request audit records at ~/.hermes/proxy/audit.log, with iron-proxy.log (daemon stdout/stderr) called out as a separate file role.
|
Saw the X ask and had some free time, so I took a pass through current HEAD and the existing review threads. Security is not my specialty, so treat this more as a Hermes operator/runtime-boundary review than a formal AppSec review. Not trying to duplicate the earlier P0/P1 findings, but a few fail-closed edges stood out to me.
Direction still looks right to me: moving real provider keys out of the sandbox is the correct defense-in-depth layer. These are mostly about making the protection boundary fail closed and match what |
GodsBoy
left a comment
GodsBoy
left a comment
There was a problem hiding this comment.
Second-round review - fix delta 7a7449213..4833acf0
Re-reviewed the iron-proxy fix delta (commits 128a6837b and 4833acf04) against the prior round (24 findings posted by us + the threads from @annguyenNous, @waefrebeorn, @arshkumarsingh, @stephenschoettler, @erhnysr, and CodeQL). All prior findings checked individually against the new source; the fix commit cleanly addressed the vast majority. This pass surfaces only NEW issues introduced by the fix delta or gaps the fix exposed.
Strictly deduplicated against:
- All 24 of our prior posted inline findings (Teknium's reply threads).
- @arshkumarsingh - NODE_OPTIONS clobber (
docker.py:306) and audit-log doc inconsistency (iron-proxy.md:127). These remain open and we agree with both; refraining from refiling. - @stephenschoettler - Bitwarden silent degrade at proxy start (E1),
docker_envmapped-provider collision (E2), PID nonce process-local across CLI invocations (E3). All three confirmed still applicable on current HEAD; refraining from refiling. Posting one P1 finding below on a different mode of the samecredential_sourcesilent-degrade class (re-setup overwriting bitwarden mode). - @erhnysr - health endpoint concern (addressed by poll-with-timeout).
- CodeQL clear-text logging - verified
4833acf04complete; no other sinks remain in the file.
Findings posted: 18 - 0 P0 / 4 P1 / 9 P2 / 5 P3.
Verdict
Not ready to merge yet. The four P1 findings are concrete pre-merge work:
- The
_detect_docker_bridge_ipparser regression risk on the bind-policy fix. cmd_setupoverwritingcredential_sourceon re-run (silently breaks the Bitwarden rotation guarantee the docs make).fail_on_uncovered_providersdocumented default contradicting actual default (fail-open while module docstring claims fail-closed).start_proxytreating grace-window expiry as success and writing a pidfile for a daemon that never bound the port.
The P2 set is mostly defensive symmetry gaps (the same PR's other hardenings should be applied consistently - O_NOFOLLOW on iron-proxy.log, pidfile race, ensure_audit_log failing loud, IPv6 deny-list adjacency) plus reliability holes around the startup sequence. None of the P2s individually block merge but several represent the same threat model the rest of the PR explicitly defends against; addressing them together keeps the security boundary coherent.
P3s are foot-guns and test-coverage gaps on already-landed fixes (PID-recycle defense untested, _reset_for_tests is a lie, hardcoded /tmp paths reintroduced in the new CLI test file).
Solid work overall on the fix round. Once these land, the PR is in good shape.
Residual risks not surfaced as inline findings
- iron-proxy in-memory secret zeroisation (the Go binary holds swapped-in real upstream credentials; out of scope for this PR but worth tracking).
- Token rotation does not invalidate already-loaded proxy state -
hermes egress setup --rotate-tokenswrites new mappings but the running iron-proxy still has the old config; requires separate stop/start. Document or auto-restart. _PROXY_SUBPROCESS_ENV_ALLOWLISTomitsRUST_LOG,RUST_BACKTRACE,GOMAXPROCS,GOMEMLIMIT,XDG_*- debugging cost only.
Coverage notes
The new tests in test_iron_proxy.py (32) and test_iron_proxy_cli.py (19) cover most of the fix delta's positive paths, but the following security-relevant new code paths are mocked over rather than exercised: _detect_docker_bridge_ip parser body, _pid_proc_starttime field-index math, stop_proxy SIGTERM-then-recycle suppression path, pidfile O_NOFOLLOW + st_uid rejection, _build_proxy_subprocess_env Bitwarden refresh exception paths, _VERSION_CACHE invalidation semantics.
| if tok == "inet" and i + 1 < len(parts): | ||
| ip = parts[i + 1].split("/")[0] | ||
| # cheap sanity: four dotted parts. | ||
| if ip.count(".") == 3: |
There was a problem hiding this comment.
Severity: P1 (security regression risk on the bind-policy fix #2)
The only sanity check before returning the parsed IP is if ip.count(".") == 3. Strings like 0.0.0.0, 999.999.999.999, 224.0.0.1, 255.255.255.255, even aa.bb.cc.dd would pass and get returned as the docker0 bridge bind address.
Failure mode: a hostile ip shim earlier on the operator's PATH, a kernel/iproute2 version that prefixes the inet line differently, or a misconfigured docker0 with 0.0.0.0 could cause start_proxy to add a public-bind address to proxy.http_listen. That re-opens exactly the LAN exposure that prior P0 #2 just closed (the proxy + a leaked sandbox token = quota theft from any LAN peer).
Also note: _PROXY_SUBPROCESS_ENV_ALLOWLIST includes PATH, so a same-uid attacker can plant ip in a writable PATH entry; combined with this parser, that's a path to convert "shell access on host" into "remote unauthenticated proxy access".
Fix: validate with stdlib instead of a digit-count heuristic:
import ipaddress
try:
addr = ipaddress.IPv4Address(ip)
except ValueError:
continue
if addr.is_unspecified or addr.is_loopback or addr.is_multicast or addr.is_reserved:
continue
return str(addr)Optional defense-in-depth: shutil.which("ip") at module load and skip detection when not in /usr/sbin / /sbin.
There was a problem hiding this comment.
Fixed in fa4e87b. Replaced the count(".") == 3 heuristic with ipaddress.IPv4Address validation + reject is_unspecified / is_loopback / is_multicast / is_reserved / is_link_local / is_global. 9 new parametrized tests in test_detect_docker_bridge_ip_rejects_dangerous cover 0.0.0.0, 127.0.0.1, 224.0.0.1, 240.0.0.0, 169.254.0.1, 8.8.8.8, 999.999.999.999, and aa.bb.cc.dd. The hostile ip shim path you flagged is closed.
| proxy_cfg["enabled"] = True | ||
| proxy_cfg.setdefault("auto_install", True) | ||
| proxy_cfg.setdefault("enforce_on_docker", True) | ||
| proxy_cfg["credential_source"] = "bitwarden" if args.from_bitwarden else "env" |
There was a problem hiding this comment.
Severity: P1 (silently breaks the Bitwarden-refresh promise the PR's own docs make)
This line was changed in the fix commit from setdefault to a direct assignment:
proxy_cfg["credential_source"] = "bitwarden" if args.from_bitwarden else "env"Operator workflow that breaks:
- Initial
hermes egress setup --from-bitwarden→credential_source: bitwarden. Refresh is wired intocmd_startat line 404. - Later, operator runs
hermes egress setup(no flag) to add a newly-installed provider. This line silently rewritescredential_sourceto"env". - Next
hermes egress startno longer passesrefresh_secrets_from_bitwarden=True. Rotating a key in Bitwarden no longer reaches the proxy. The docs still promise rotation works.
This is the same class of issue as prior finding #10 (--from-bitwarden silent degrade), except this one is in the re-setup path rather than the first setup path. Adjacent surface, novel location.
Fix: restore setdefault, OR detect the downgrade and either warn loudly or require --no-bitwarden to be explicit:
existing_source = proxy_cfg.get("credential_source")
if args.from_bitwarden:
proxy_cfg["credential_source"] = "bitwarden"
elif existing_source == "bitwarden":
console.print("[yellow]Keeping credential_source=bitwarden from existing config. "
"Pass --no-bitwarden to switch back to env-based credentials.[/yellow]")
else:
proxy_cfg["credential_source"] = "env"There was a problem hiding this comment.
Fixed in fa4e87b. Restored preservation semantics — re-running hermes egress setup without --from-bitwarden no longer silently downgrades credential_source back to env. Added an explicit --no-bitwarden flag for the deliberate-switch case (prints a confirmation message when the existing source was bitwarden). Otherwise the existing mode is preserved and the wizard surfaces the decision (dim text) so the operator sees that we kept it.
| # Providers whose env-var names we recognize but whose API uses a non-bearer | ||
| # auth scheme (x-api-key, AAD/OAuth, SigV4, custom signatures). When any of | ||
| # these env vars are present at proxy-start time AND | ||
| # ``proxy.fail_on_uncovered_providers`` is true (default), ``start_proxy`` |
There was a problem hiding this comment.
Severity: P1 (operators reading the module believe non-bearer providers fail-loud; they don't)
Module-level comment at line 135:
"...AND
proxy.fail_on_uncovered_providersis true (default),start_proxyrefuses to start."
Reality:
hermes_cli/proxy_cli.py:341-proxy_cfg.setdefault("fail_on_uncovered_providers", False)hermes_cli/proxy_cli.py:387-if bool(proxy_cfg.get("fail_on_uncovered_providers", False)):hermes_cli/config.pyDEFAULT_CONFIG - also False.
So the wizard ships fail-OPEN and the runtime check is gated false-by-default. An operator who reads the module docstring believes that having ANTHROPIC_API_KEY in their host env will refuse-to-start; they get the silent-fall-through path instead, and the sandbox boots with real x-api-key bypassing the proxy. This is exactly the threat surface the comment claims to defend.
Fix (pick one - they have different implications, so this needs your judgment):
- Safer default: flip default to
Trueeverywhere (setdefault incmd_setup,.get(..., True)incmd_start, DEFAULT_CONFIG entry). Matches docstring; matches the spirit of "this is a security feature, default fail-closed". Cost: more operator friction on first run if they happen to have any provider env vars set. - Truth-in-advertising: leave default at False, but update the docstring and the user docs to say "default false; opt-in via
proxy.fail_on_uncovered_providers: truein config".
The current state (docstring promises fail-closed; behavior is fail-open) is the bad option.
There was a problem hiding this comment.
Fixed in fa4e87b via the truth-in-advertising path + tier split. The module docstring now correctly states default=False. Separately, _NON_BEARER_PROVIDERS is split into a strict LLM-specific tier (_LLM_SPECIFIC_NON_BEARER_PROVIDERS — Anthropic / Azure / Gemini) that BLOCKS start when fail_on_uncovered_providers: true, vs a generic uncovered tier (AWS_*, GCP appdefault) that's surfaced as warnings only. New discover_blocked_providers() returns the strict subset. Test test_blocked_providers_subset_of_uncovered regression-guards the subset relationship. Operators with terraform/gcloud configured no longer hit refuse-start for unrelated tooling.
| ) | ||
| if _port_listening("127.0.0.1", tunnel_port): | ||
| break | ||
| time.sleep(0.1) |
There was a problem hiding this comment.
Severity: P1
The poll loop at lines 1271-1281:
deadline = time.time() + _STARTUP_GRACE_SECONDS
while time.time() < deadline:
if proc.poll() is not None:
...raise...
if _port_listening("127.0.0.1", tunnel_port):
break
time.sleep(0.1)If the binary is still running but never binds within 5s (slow start, port collision masked by deferred bind, address-already-in-use after a SO_REUSEADDR window, kernel pause), the loop exits via while time.time() < deadline reaching false. There is NO post-loop check that the port actually came up - only that the process is still running (line 1283). The function then writes the pidfile (line 1295) and get_status() returns with pid set but listening=False. Docker container creation (which calls get_status and only checks status.binary_path / status.config_path, not listening) proceeds, and every upstream request 401s inside the sandbox with no clear signal back to the operator.
Additionally: if the binary IS hung (running but unresponsive), it gets a recorded pidfile and orphans on the next start because _pid_alive returns True.
Fix: require port-listening for success. After the loop, explicitly:
if not _port_listening("127.0.0.1", tunnel_port):
# Kill the child so we don't orphan a non-listening daemon.
try:
proc.terminate()
proc.wait(timeout=2)
except (OSError, subprocess.TimeoutExpired):
try:
proc.kill()
except OSError:
pass
tail = _tail_log(log_path, lines=20)
raise RuntimeError(
f"iron-proxy did not bind 127.0.0.1:{tunnel_port} within "
f"{_STARTUP_GRACE_SECONDS}s. Last log lines:\n{tail}"
)There was a problem hiding this comment.
Fixed in fa4e87b. Refactored the poll loop into a do-while shape, require listening=True for success, kill the child + unlink the pidfile on every failure path (deadline-expired, process exited, KeyboardInterrupt). Existing test test_start_proxy_writes_pidfile_when_alive updated to mock _port_listening=True to exercise the success path under the new contract.
| # immediately after Popen (the child has its own dup). Without the | ||
| # close-on-success path, every restart leaked one fd in the Hermes | ||
| # process. | ||
| log_fd = os.open( |
There was a problem hiding this comment.
Severity: P2 (symmetry gap with the same PR's hardenings)
ensure_audit_log (line 845) uses O_NOFOLLOW to refuse a planted symlink. The pidfile open (line 1295) uses O_NOFOLLOW. But the daemon stdout/stderr log opened here:
log_fd = os.open(
str(log_path),
os.O_WRONLY | os.O_CREAT | os.O_APPEND,
0o600,
)does NOT include O_NOFOLLOW. A same-uid attacker who plants ~/.hermes/proxy/iron-proxy.log as a symlink to, say, ~/.ssh/authorized_keys causes the daemon to append iron-proxy diagnostic output to that file across every hermes egress start. Limited but not zero; same threat model as the pidfile defense.
Fix: mirror the audit-log pattern at line 845-847:
open_flags = os.O_WRONLY | os.O_CREAT | os.O_APPEND
if hasattr(os, "O_NOFOLLOW"):
open_flags |= os.O_NOFOLLOW
log_fd = os.open(str(log_path), open_flags, 0o600)Optionally also add the os.fstat(fd).st_uid == os.getuid() check that the pidfile path uses.
There was a problem hiding this comment.
Fixed in fa4e87b. Log file open mirrors the pidfile + audit-log hardenings: O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW, explicit 0o600 mode, os.fchmod to tighten if pre-existing, and os.fstat().st_uid == os.getuid() ownership check. ELOOP on a planted symlink surfaces as a RuntimeError naming the path.
| "from secrets.bitwarden config instead of the current env. Fails " | ||
| "loudly if BW is unreachable rather than silently falling back.", | ||
| ) | ||
| setup.add_argument( |
There was a problem hiding this comment.
Severity: P3 (foot-gun UX)
--rotate-tokens on its own is a useful escape hatch (operator suspects token compromise, wants to mint fresh ones), but two ergonomics issues:
-
No confirmation, no backup. Running with the flag on a healthy install rotates every token immediately. Every running sandbox loses its mappings on next request, no warning, no undo. An accidental rerun (history scroll-back, tmux paste) is unrecoverable.
-
Silent no-op on first-time setup.
merge_mappings(existing=[], discovered=..., rotate=True)is indistinguishable fromrotate=False(no overlap to rotate). The "tokens rotated" warning at proxy_cli.py:264 only fires whenexistingis non-empty. Operator who deliberately requested rotation on a fresh setup gets no feedback that the flag was a no-op.
Fix:
if args.rotate_tokens and existing_mappings:
if not console.input(
"[yellow]This will invalidate proxy tokens in every running "
"sandbox. Type 'rotate' to confirm: [/yellow]"
).strip().lower() == "rotate":
console.print("[yellow]Cancelled.[/yellow]")
return 1
# Backup before write.
backup = mappings_path.with_suffix(f".rotated-{int(time.time())}")
shutil.copy2(mappings_path, backup)
console.print(f" [dim]backup: {backup}[/dim]")
if args.rotate_tokens and not existing_mappings:
console.print(
"[dim]Note: --rotate-tokens is a no-op on first-time setup "
"(no existing tokens to rotate).[/dim]"
)There was a problem hiding this comment.
Fixed in fa4e87b. When --rotate-tokens is passed AND there are existing mappings AND stdin is a tty, the wizard now prompts for explicit rotate confirmation; non-tty (CI) skips the prompt since the flag was clearly deliberate. Before any overwrite we copy mappings.json to a sibling mappings.json.rotated-<YYYYMMDDTHHmmss> and print the path so manual recovery is possible. When --rotate-tokens is passed with no existing mappings, we print a Note: ... no-op dim message so the operator sees feedback.
|
|
||
| def _reset_for_tests() -> None: | ||
| """No-op today — kept symmetric with bitwarden._reset_cache_for_tests.""" | ||
|
|
There was a problem hiding this comment.
Severity: P3 (test-isolation contract gap)
The helper is currently:
def _reset_for_tests() -> None:
"""No-op today - kept symmetric with bitwarden._reset_cache_for_tests."""
return NoneBut this module now owns two mutable module-level globals introduced by the fix commit:
_VERSION_CACHE(line ~210) - caches subprocess output keyed by binary path._proxy_nonce(line ~1057, set instart_proxy) - a strong-proof token for_pid_alive.
Today the repo's tests run each file in its own subprocess (per AGENTS.md), so leakage is bounded. But any in-process caller (a future ipython notebook test, a pytest -p no:xdist invocation, anyone importing this module from a non-pytest script) will see whichever cached version was probed first, regardless of subsequent install_iron_proxy(force=True) calls in the same process.
Fix: populate the helper to live up to its name:
def _reset_for_tests() -> None:
"""Clear module-level caches so tests get a fresh start."""
global _proxy_nonce
_VERSION_CACHE.clear()
_proxy_nonce = NoneEither that, or delete the helper and its symmetric-with-bitwarden docstring, since the symmetry it claims is currently a lie.
There was a problem hiding this comment.
Fixed in fa4e87b. _reset_for_tests() now actually clears _VERSION_CACHE and _proxy_nonce. Test test_reset_for_tests_clears_version_cache_and_nonce regression-guards by populating both and asserting they're empty after the call.
|
|
||
|
|
||
| def test_cmd_install_success_returns_0(hermes_home, monkeypatch): | ||
| monkeypatch.setattr(ip, "install_iron_proxy", lambda **kw: Path("/tmp/iron-proxy")) |
There was a problem hiding this comment.
Severity: P3 (project-standards regression)
The fix commit just replaced hardcoded Path('/tmp/ca.*') paths in test_iron_proxy.py with tmp_path-derived fixtures (closing prior finding #24). The brand-new sibling file tests/test_iron_proxy_cli.py added in the same commit reintroduces the pattern:
tests/test_iron_proxy_cli.py:60-Path("/tmp/iron-proxy")(binary_path)tests/test_iron_proxy_cli.py:91-Path("/tmp/proxy.yaml")tests/test_iron_proxy_cli.py:121-Path("/tmp/iron-proxy")tests/test_iron_proxy_cli.py:147-Path("/tmp/iron-proxy")andPath("/tmp/proxy.yaml")tests/test_iron_proxy_cli.py:170-Path("/tmp/proxy.yaml")tests/test_iron_proxy_cli.py:338-Path("/tmp/iron-proxy")
These are mostly mock return values rather than files actually written, but the standards reason for the prior fix (hermeticity, cross-host portability, Windows / CI variants) applies equally - and the regression risk is the next test author copy-pastes from this file and writes a real Path("/tmp/foo") they DO open.
Fix: each test already takes the hermes_home fixture (tmp_path-backed) or can take tmp_path directly. Replace each Path("/tmp/iron-proxy") with tmp_path / "iron-proxy" and Path("/tmp/proxy.yaml") with tmp_path / "proxy.yaml". Pure mechanical change.
There was a problem hiding this comment.
Fixed in fa4e87b. All Path("/tmp/iron-proxy") and Path("/tmp/proxy.yaml") literals in tests/test_iron_proxy_cli.py replaced with hermes_home / "..." (the existing tmp_path-backed fixture). Mechanical sed pass; no behavioral change.
|
|
||
| def _build_proxy_subprocess_env( | ||
| *, | ||
| extra_env: Optional[Dict[str, str]] = None, |
There was a problem hiding this comment.
Severity: P3 (dead code; cleanup)
_build_proxy_subprocess_env accepts extra_env: Optional[Dict[str, str]] = None (line 1329), receives it from start_proxy(..., extra_env: Optional[Dict[str, str]] = None, ...) (line 1168), threaded through at line 1210, but there is no caller in the repo passing a non-None value. Grep confirms.
Two failure modes:
- A future caller starts passing
extra_envexpecting the override to win - but_PROXY_SUBPROCESS_ENV_STRIPapplies AFTER the merge, soextra_env={"HTTPS_PROXY": "..."}would be silently dropped. The contract is ambiguous because there's no test that pins it. - API surface bloat. The kwarg appears in
__all__indirectly viastart_proxy. New caller has to read the implementation to figure out what it does.
Fix: remove the parameter from both signatures, the call site, and the docstrings. If a real use case appears later, add it back with a test that pins precedence semantics.
There was a problem hiding this comment.
Acknowledged but kept in fa4e87b. Removing extra_env is a breaking change for any out-of-tree caller, and the kwarg does work correctly (the precedence — caller-overrides win, then strip — is documented in the docstring). If we see a concrete bug from the ambiguity, we'll add a test that pins the contract. Filed as a separate cleanup so the BC implications get a proper discussion.
| # Capture starttime BEFORE signalling so we can compare after the | ||
| # grace window — if the pid got recycled mid-wait, the starttime | ||
| # changes and we abort the SIGKILL. | ||
| starttime_before = _pid_proc_starttime(pid) |
There was a problem hiding this comment.
Severity: P3 (test coverage gap on a security-relevant fix)
Prior finding #16 was upgraded to "stop_proxy now captures /proc/<pid>/stat[21] before SIGTERM and re-verifies after the grace window; SIGKILL is suppressed if starttime changed." That fix is correct, but:
_pid_proc_starttime(the helper that parses/proc/<pid>/statfield 22 - 1-indexed - past the parenthesised comm) has no direct unit test. The field-index math is the kind of subtle thing that breaks silently when someone refactors. Acommcontaining literal)characters (perfectly legal - trygcc -DTEST -o ")) tail" main.c; or a kernel thread[migration/0]with brackets) shifts the field count.- The
stop_proxySIGTERM → starttime-recompare → suppress-SIGKILL path has no test. The PID-recycle scenario is hard to reproduce but easy to fake-test: monkeypatch_pid_proc_starttimeto return different values on call N vs N+1 and assert SIGKILL is suppressed.
Fix: add the missing tests. Two focused tests:
def test_pid_proc_starttime_parses_comm_with_parens(tmp_path, monkeypatch):
# Simulate /proc/<pid>/stat with a comm containing ')' - e.g. "((bad))"
# and assert _pid_proc_starttime returns the field after the LAST ')'.
...
def test_stop_proxy_suppresses_sigkill_on_pid_recycle(monkeypatch):
# Make _pid_proc_starttime return X first, then Y after SIGTERM.
# Assert SIGKILL is NOT issued.
...These guard the security property of fix #16 across refactors.
There was a problem hiding this comment.
Fixed in fa4e87b. Two new tests:
test_pid_proc_starttime_parses_comm_with_parens— synthetic/proc/<pid>/statwith a comm containing)and space characters; asserts the parser returns the correct starttime via therfind(')')path.test_stop_proxy_suppresses_sigkill_on_pid_recycle— mocks_pid_proc_starttimeto return different values before/after the SIGTERM grace, asserts SIGTERM is sent but SIGKILL is NOT (recycled detection).
|
Thanks @GodsBoy @stephenschoettler @arshkumarsingh — all three rounds of feedback addressed in fa4e87b25. GodsBoy P1 — all 4 addressed
GodsBoy P2 — all 9 addressed
GodsBoy P3 — 3 of 5 addressed
@stephenschoettler — all 3 findings addressed
@arshkumarsingh — both addressed
Tests12 new tests in Residual risks acknowledged (not addressed in this round)
Threads queued under the individual findings. |
…ment API Both wired against features the iron-proxy author (@mslipper) confirmed on PR NousResearch#30179 — and both verified present in the pinned v0.39.0 source. Header-auth providers (match_headers): - New _HEADER_AUTH_PROVIDERS: Anthropic native (x-api-key), Azure OpenAI (api-key on *.openai.azure.com / *.cognitiveservices / *.services.ai), Gemini (x-goog-api-key + ?key= query param via match_query). - TokenMapping grows match_headers + alias_env_names; per-provider header sets flow into the secrets rules; mappings.json roundtrips them (legacy files load with the Authorization default). - GEMINI_API_KEY / GOOGLE_API_KEY collapse into ONE mapping (two require-rules on the same host would reject each other); the sandbox gets the token under both names, and the proxy child env mirrors the alias into the canonical name when only the alias is set. - Docker backend injects alias env names alongside canonical ones. - The fail-closed tier is now empty, so fail_on_uncovered_providers and discover_blocked_providers are deleted (dead toggle otherwise); _NON_BEARER_PROVIDERS shrinks to genuinely-unswappable signature auth (AWS SigV4, GCP service-account OAuth) — warn-only, as before. Management API (hot reload): - Generated proxy.yaml enables the v0.39 management listener: loopback only at tunnel_port+2, bearer key from HERMES_IRON_PROXY_MGMT_KEY. - Key minted at setup (management.token, 0600); start_proxy injects it (v0.39 refuses to start when api_key_env is empty). - hermes egress reload -> POST /v1/reload: re-reads proxy.yaml and atomically swaps the pipeline; 422 leaves the running ruleset untouched; actionable errors for not-running / pre-management config / key mismatch. Secrets changes still require restart (daemon env is read at spawn) — the CLI says so. Validation: 218/218 unit+CLI+docker tests; 3/3 gated live E2E against the real v0.39.0 binary (Authorization swap, x-api-key swap, live reload with token rotation on the same pid). Docs updated.
feat(egress): iron-proxy credential-injection firewall for sandboxes
Revert "feat(egress): iron-proxy credential-injection firewall" (NousResearch#30179)
…lel.py (NousResearch#43646) * fix(ci): append filesystem forensics when a per-file pytest run exhausts exit-4 retries A PR-added test file (tests/test_iron_proxy.py, PR NousResearch#30179) repeatedly failed exactly one CI shard with 'ERROR: file or directory not found' across 4 runs (including a fresh merge SHA on fresh runners), while the identical slice passes locally against the same merge commit and a tree-integrity watcher confirms no sibling test mutates the repo. Three unrelated branches showed the same one-shard signature the same day. We currently cannot attribute these because the log only carries pytest's exit-4 line. This adds a forensics block to the captured output when exit-4 survives the retry loop: - does the file exist NOW (post-retries) - parent dir entry count + similarly-named entries - git status --porcelain dirty-entry count + first 10 entries Zero behavior change: rc stays 4, retries unchanged, forensics wrapped in a broad try/except so they can never mask the failure. Two new tests cover the exhausted-retries and genuinely-missing paths. * chore: drop the two forensics tests — ship the runner change only
…ment API Both wired against features the iron-proxy author (@mslipper) confirmed on PR NousResearch#30179 — and both verified present in the pinned v0.39.0 source. Header-auth providers (match_headers): - New _HEADER_AUTH_PROVIDERS: Anthropic native (x-api-key), Azure OpenAI (api-key on *.openai.azure.com / *.cognitiveservices / *.services.ai), Gemini (x-goog-api-key + ?key= query param via match_query). - TokenMapping grows match_headers + alias_env_names; per-provider header sets flow into the secrets rules; mappings.json roundtrips them (legacy files load with the Authorization default). - GEMINI_API_KEY / GOOGLE_API_KEY collapse into ONE mapping (two require-rules on the same host would reject each other); the sandbox gets the token under both names, and the proxy child env mirrors the alias into the canonical name when only the alias is set. - Docker backend injects alias env names alongside canonical ones. - The fail-closed tier is now empty, so fail_on_uncovered_providers and discover_blocked_providers are deleted (dead toggle otherwise); _NON_BEARER_PROVIDERS shrinks to genuinely-unswappable signature auth (AWS SigV4, GCP service-account OAuth) — warn-only, as before. Management API (hot reload): - Generated proxy.yaml enables the v0.39 management listener: loopback only at tunnel_port+2, bearer key from HERMES_IRON_PROXY_MGMT_KEY. - Key minted at setup (management.token, 0600); start_proxy injects it (v0.39 refuses to start when api_key_env is empty). - hermes egress reload -> POST /v1/reload: re-reads proxy.yaml and atomically swaps the pipeline; 422 leaves the running ruleset untouched; actionable errors for not-running / pre-management config / key mismatch. Secrets changes still require restart (daemon env is read at spawn) — the CLI says so. Validation: 218/218 unit+CLI+docker tests; 3/3 gated live E2E against the real v0.39.0 binary (Authorization swap, x-api-key swap, live reload with token rotation on the same pid). Docs updated.
feat(egress): iron-proxy credential-injection firewall for sandboxes
Revert "feat(egress): iron-proxy credential-injection firewall" (NousResearch#30179)
…lel.py (NousResearch#43646) * fix(ci): append filesystem forensics when a per-file pytest run exhausts exit-4 retries A PR-added test file (tests/test_iron_proxy.py, PR NousResearch#30179) repeatedly failed exactly one CI shard with 'ERROR: file or directory not found' across 4 runs (including a fresh merge SHA on fresh runners), while the identical slice passes locally against the same merge commit and a tree-integrity watcher confirms no sibling test mutates the repo. Three unrelated branches showed the same one-shard signature the same day. We currently cannot attribute these because the log only carries pytest's exit-4 line. This adds a forensics block to the captured output when exit-4 survives the retry loop: - does the file exist NOW (post-retries) - parent dir entry count + similarly-named entries - git status --porcelain dirty-entry count + first 10 entries Zero behavior change: rc stays 4, retries unchanged, forensics wrapped in a broad try/except so they can never mask the failure. Two new tests cover the exhausted-retries and genuinely-missing paths. * chore: drop the two forensics tests — ship the runner change only
…lel.py (NousResearch#43646) * fix(ci): append filesystem forensics when a per-file pytest run exhausts exit-4 retries A PR-added test file (tests/test_iron_proxy.py, PR NousResearch#30179) repeatedly failed exactly one CI shard with 'ERROR: file or directory not found' across 4 runs (including a fresh merge SHA on fresh runners), while the identical slice passes locally against the same merge commit and a tree-integrity watcher confirms no sibling test mutates the repo. Three unrelated branches showed the same one-shard signature the same day. We currently cannot attribute these because the log only carries pytest's exit-4 line. This adds a forensics block to the captured output when exit-4 survives the retry loop: - does the file exist NOW (post-retries) - parent dir entry count + similarly-named entries - git status --porcelain dirty-entry count + first 10 entries Zero behavior change: rc stays 4, retries unchanged, forensics wrapped in a broad try/except so they can never mask the failure. Two new tests cover the exhausted-retries and genuinely-missing paths. * chore: drop the two forensics tests — ship the runner change only
…ment API Both wired against features the iron-proxy author (@mslipper) confirmed on PR NousResearch#30179 — and both verified present in the pinned v0.39.0 source. Header-auth providers (match_headers): - New _HEADER_AUTH_PROVIDERS: Anthropic native (x-api-key), Azure OpenAI (api-key on *.openai.azure.com / *.cognitiveservices / *.services.ai), Gemini (x-goog-api-key + ?key= query param via match_query). - TokenMapping grows match_headers + alias_env_names; per-provider header sets flow into the secrets rules; mappings.json roundtrips them (legacy files load with the Authorization default). - GEMINI_API_KEY / GOOGLE_API_KEY collapse into ONE mapping (two require-rules on the same host would reject each other); the sandbox gets the token under both names, and the proxy child env mirrors the alias into the canonical name when only the alias is set. - Docker backend injects alias env names alongside canonical ones. - The fail-closed tier is now empty, so fail_on_uncovered_providers and discover_blocked_providers are deleted (dead toggle otherwise); _NON_BEARER_PROVIDERS shrinks to genuinely-unswappable signature auth (AWS SigV4, GCP service-account OAuth) — warn-only, as before. Management API (hot reload): - Generated proxy.yaml enables the v0.39 management listener: loopback only at tunnel_port+2, bearer key from HERMES_IRON_PROXY_MGMT_KEY. - Key minted at setup (management.token, 0600); start_proxy injects it (v0.39 refuses to start when api_key_env is empty). - hermes egress reload -> POST /v1/reload: re-reads proxy.yaml and atomically swaps the pipeline; 422 leaves the running ruleset untouched; actionable errors for not-running / pre-management config / key mismatch. Secrets changes still require restart (daemon env is read at spawn) — the CLI says so. Validation: 218/218 unit+CLI+docker tests; 3/3 gated live E2E against the real v0.39.0 binary (Authorization swap, x-api-key swap, live reload with token rotation on the same pid). Docs updated.
feat(egress): iron-proxy credential-injection firewall for sandboxes
Revert "feat(egress): iron-proxy credential-injection firewall" (NousResearch#30179)
|
@erhnysr The re-land is in — PR #70848 merged today (rebase of the #58489 revert, reconciled with current main). Please open it as a standalone PR whenever you're ready — the scoped design you described (one-shot check + |
|
Great news on the reland — thanks @teknium1. Now that #70848 is in and Noted on the port-probe detail from the review history — I'll make sure any health/status paths read the configured bind host via |
Adds a lightweight health check for the iron-proxy daemon. Usage: hermes egress health # one-shot check hermes egress health --watch # poll until healthy hermes egress health --timeout 30 # fail after 30s Exit codes: 0 proxy is up and listening 1 pid exists but port not accepting connections 2 proxy not running or not configured The liveness report uses the configured bind host from _read_http_listen_from_config() rather than a hardcoded 127.0.0.1. On Linux the daemon binds the docker bridge gateway (e.g. 172.17.0.1) so sandboxes can reach it; a loopback-only probe would report a perfectly healthy daemon as dead. get_status() already probes that host for status.listening, and we surface the same host:port in the output so the reported address matches what was tested. Falls back to loopback only when no proxy.yaml exists (matching get_status()). Closes the health-endpoint follow-up raised in PR NousResearch#30179 review. 9 tests added in test_proxy_cli_health.py.
|
@teknium1 Following the reland, I've opened the health subcommand as a standalone PR: #71135. It's rebased onto current main, and the liveness probe reads the configured bind host via |
* fix(memory-setup): sanitize .env values in the core writer too
Widens the salvaged .env injection fix (#50315) to the sibling site it
missed: hermes_cli/memory_setup.py::_write_env_vars is the near-identical
core writer the openviking plugin's copy was forked from, is fed directly
by interactive _prompt() (pasted API keys), and is reused by other memory
plugins (e.g. supermemory imports it). A pasted secret with an embedded
CR/LF injected an arbitrary extra KEY=VALUE line on the next read.
Same _env_line_safe() treatment as the plugin writer (strip every
str.splitlines() separator + NUL), matching config.save_env_value's
existing newline strip. Mutation-checked: reverting the sanitizer makes
the new regression tests fail.
* fix(cron): tick every served profile's cron store under multiplex_profiles (#69377)
Under multiplex_profiles, the gateway starts a single InProcessCronScheduler
bound to the process-global HERMES_HOME (the default profile's home), so
only that profile's cron/jobs.json is ticked. A job registered from a
secondary-profile session lands in <profile>/cron/jobs.json, reports a valid
next_run_at — and never fires.
Changes:
1. cron/scheduler_provider.py — InProcessCronScheduler.start() now accepts
an optional profile_homes kwarg (list of (name, Path) tuples). When set,
_start_multiplex() iterates tick() over each profile home using
use_cron_store(), so every served profile's cron store is ticked on
every tick cycle. Heartbeats and interrupted-execution recovery are also
scoped per profile via use_cron_store().
2. gateway/run.py — start_gateway() now resolves profiles_to_serve(multiplex=True)
when multiplex_profiles is on and passes them to the cron scheduler as
profile_homes. Only applies to InProcessCronScheduler (the built-in);
external providers are unchanged.
3. cron/jobs.py — record_ticker_heartbeat(), get_ticker_heartbeat_age(), and
get_ticker_success_age() now resolve paths via _current_cron_store()
instead of module-level TICKER_HEARTBEAT_FILE / TICKER_SUCCESS_FILE
constants. This makes heartbeats correctly scoped per profile, so
'hermes cron status' reflects liveness for every profile independently
under multiplex_profiles.
4. tests/cron/test_scheduler_provider.py — two new tests:
- test_multiplex_ticker_ticks_each_profile_once: verifies tick() is called
once per profile per tick cycle.
- test_multiplex_heartbeat_scoped_per_profile: verifies heartbeat files
are written to each profile's cron store.
* fix(cron): scope hermes_home override per-profile in multiplex ticker
The multiplex cron path only used use_cron_store() to scope storage paths
(jobs.json, heartbeat files), but _get_lock_paths() and the agent execution
path in cron/scheduler.py resolve via _get_hermes_home() → get_hermes_home()
which checks _HERMES_HOME_OVERRIDE, a separate ContextVar. Without
set_hermes_home_override(), the .tick.lock, config.yaml, .env, and secrets
all resolved to the default profile instead of the per-profile home.
This matches the web_server.py pattern (line 11994) which sets both
set_hermes_home_override(home) AND use_cron_store(home), and the
_profile_runtime_scope pattern used for the multiplexed inbound path.
Found via 3-agent parallel review of salvaged PR #69529.
* feat(honcho): add OAuth device-code login (RFC 8628) for headless environments
Adds a device authorization grant flow alongside the existing loopback
OAuth flow, so `hermes setup` can connect to Honcho cloud from SSH and
other no-browser environments.
- oauth.py: new HTTP seams — _http_post_form_status (non-raising, since
RFC 8628 polling reads the OAuth error off a 400) and _http_get_json
for the RFC 8414 metadata probe
- oauth_flow.py: DeviceCode, request_device_code, poll_for_token with
slow_down backoff (+5s, capped at 60s) bounded by expires_in, typed
errors (AccessDenied, DeviceCodeExpired, AuthorizationTimeout), and
supports_device_login (fail-closed metadata gate); device flow ends in
the same install_grant tail as loopback so refresh/status work
unchanged
- oauth_flow.py: loopback callback now serves a "sign-in was not
completed" page on consent cancel instead of the success page
- cli.py: cloud menu offers oauth / device / apikey; the device option
only appears when the host advertises the grant, and becomes the
default when no browser is detected
- 18 new tests covering the full flow against a local fake AS, backoff
schedule, error mapping, deadline bound, metadata gate, and wizard
branches
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(honcho): default device-code poll interval to 5s when AS omits it
RFC 8628 §3.2 makes the device-authorization `interval` optional with a
client-side default of 5 seconds. request_device_code required it, so a
compliant AS that omitted it hit the malformed-response path and the flow
could never complete. Fall back to 5s and cover it with a regression test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(skills): align skill directory names with frontmatter name
Salvaged from PR #42788 by @Love-JourneY, re-applied at current locations
(audiocraft and segment-anything have since moved to optional-skills/):
- skills/mlops/inference/vllm -> serving-llms-vllm
- skills/mlops/evaluation/lm-evaluation-harness -> evaluating-llms-harness
- optional-skills/mlops/models/segment-anything -> segment-anything-model
- optional-skills/creative/audiocraft -> audiocraft-audio-generation
Directory name != frontmatter name breaks skill_view() lookup by dir
name and causes hermes update sync re-seeding duplicates (#42786).
The authoring guide calls this out as Pitfall #8.
Fixes #42786
* docs(skills): update pages, catalogs, sidebar for skill dir renames
Auto-gen page slugs, catalog rows/paths, sidebar entries, and zh-Hans
mirrors follow the directory renames. Also updates the install path
official/creative/audiocraft -> official/creative/audiocraft-audio-generation
in the songwriting-and-ai-music pointer section.
* docs(skills): fix broken related_skills references (#37338)
Salvaged from PR #38820 by @bedirhancode, re-applied at current skill
locations (obliteratus and s6 moved to optional-skills/ since the PR):
- research-paper-writing: drop ml-paper-writing (never existed)
- touchdesigner-mcp: drop native-mcp (consolidated) + hermes-video (never existed)
- obliteratus: vllm -> serving-llms-vllm, gguf -> llama-cpp
- s6-container-supervision: drop hermes-agent-dev (not a repo skill)
4 of the original 8 hunks were dropped: heartmula already fixed in
#70453; native-mcp SKILL.md deleted from main; architecture-diagram and
comfyui hunks removed refs to concept-diagrams and
stable-diffusion-image-generation, which are valid optional skills.
* docs(skills): fix remaining 13 broken related_skills refs repo-wide
Widening pass on top of the #38820 salvage: a full-graph audit of every
SKILL.md (bundled + optional) found 13 more references to skills that
no longer exist. Classes:
- deleted in the 38d3c49aaf bundled-skill cleanup: generative-widgets,
spotify, cloudflared-quick-tunnel, webhook-subscriptions,
debugging-hermes-tui-commands -> dropped
- native-mcp absorbed into the hermes-agent hub skill -> re-pointed
- toolset names that were never skills: browser, image_gen -> dropped
Audit now reports zero broken related_skills references.
* docs(skills): sync generated pages + zh-Hans mirrors for related_skills fixes
* fix(tools): enforce 60-char description limit for skills
MAX_DESCRIPTION_LENGTH was set to 1024, but the documented skill-
authoring standard specifies <=60 characters. The model generates
descriptions up to 202 chars because the validation allows 1024.
Lower MAX_DESCRIPTION_LENGTH from 1024 to 60 to match the documented
standard. The system-prompt skill index already truncates to 60 chars,
so over-length descriptions lose their routing signal past char 60.
Fixes #52367
* fix(skills): scope 60-char description enforcement to the create path
The blanket MAX_DESCRIPTION_LENGTH=1024->60 change is narrowed:
create-time validation now rejects new skills whose description
exceeds SKILL_PROMPT_DESC_LIMIT (60) with actionable guidance, while
edit/patch paths stay permissive (warning via system_prompt_preview)
so existing over-limit skills remain maintainable. Runtime display
truncation in skills_tool is left at 1024 (display behavior is a
separate concern from authoring validation).
Boundary tests: 60 accepted, 61 rejected at create; edit/patch on
over-budget skills still succeed.
* fix(tui_gateway): bind the branched agent to the parent profile's home + state.db
session.branch wrote the child ROW into the parent's profile db but
built the live agent with the launch defaults: _make_agent fell back to
_get_db() and no HERMES_HOME override was active. The branched agent's
own message flushes — and any later compression rotation it performed —
therefore landed back on the launch profile, splitting the lineage one
turn after the branch. Mirror session.create/resume: open the parent
profile's SessionDB for the agent and hold the home override across the
build, so config/skills/memory resolve to the profile too.
Spotted in #70605's sibling implementation of the same fix.
Co-authored-by: HexLab98 <liruixinch@outlook.com>
* fix(skills): sync mlops structured-output/vectordb skills to current APIs
Five optional mlops skills documented removed pre-major-version APIs. Verified each against upstream and rewrote to the current form:
- outlines: pre-1.0 outlines.generate.*/models.transformers -> v1 from_transformers + model(prompt, output_type)
- guidance: models.Anthropic (nonexistent in 0.3.x) -> Transformers backend; grammar-string -> guidance.json(); noted constrained gen needs local logits
- pinecone: pip install pinecone-client (deprecated) -> pinecone; removed bogus alpha= query kwarg, pre-scale hybrid vectors
- qdrant: client.search()/search_batch() (removed) -> query_points()/query_batch_points()
- modal: container_idle_timeout/concurrency_limit/allow_concurrent_inputs -> scaledown_window/max_containers/@modal.concurrent; floor bumped to modal>=1.0
* fix(skills): sync mlops training/model-infra skills to current APIs
Seven optional mlops training skills had stale APIs, config paths, image locations, and requirement pins. Verified against upstream and corrected:
- torchtitan: removed TOML train_configs paths (replaced upstream by config registry)
- trl-fine-tuning: PPO removed from TRL 1.x -> GRPO/RLOO; SFTTrainer tokenizer= -> processing_class
- flash-attention: torch.backends.cuda.sdp_kernel (deprecated) -> torch.nn.attention.sdpa_kernel; corrected false FA3/FP8-in-pip claim (FA2 only)
- accelerate: DeepSpeedPlugin instance not raw dict; --config_file expects accelerate YAML; auto_wrap_policy -> transformer_based_wrap
- saelens: v6 nested training config (sae=/logger=); from_pretrained tuple -> from_pretrained_with_cfg_and_sparsity
- tensorrt-llm: Docker Hub image 404 -> NGC nvcr.io; rc pin -> GA; CUDA req updated
- nemo-curator: pip extras renamed; repo moved to NVIDIA-NeMo/Curator; 1.x pipeline rewrite noted
* fix(skills): sync coding-agent CLI skills to current flags/packages
Four coding-agent CLI skills drifted from their live CLIs. Verified against live --help/npm and corrected:
- codex: --full-auto deprecated -> --sandbox workspace-write; --yolo -> --dangerously-bypass-approvals-and-sandbox (yolo kept as noted alias)
- claude-code: --effort levels low/medium/high/xhigh/max (dropped removed 'auto', added 'xhigh'); fixed stray table cell
- grok: --session-id is UUID-only for new sessions (cannot resume by name); rewrote the Session Continuation example; noted --max-turns now exists
- blackbox: wrong npm package (@blackboxai/cli is unrelated) -> @blackbox_ai/blackbox-cli; removed dead source-repo link and phantom session/info subcommands
* docs(design-md): sync skill with @google/design.md CLI 0.3.0
The design-md skill documented the Apr 2026 (0.1.x) CLI behavior, which
has since drifted:
- Lint rules: the skill listed 7 rules that no longer exist by those
names (duplicate-section, invalid-color, wcag-contrast,
unknown-component-property); the 0.3.0 linter runs 9 rules
(contrast-ratio, orphaned-tokens, missing-primary, missing-typography,
section-order, unknown-key, token-summary, missing-sections,
broken-ref). Verified against live lint output.
- Colors: any CSS color is now valid (oklch/rgb/named), not hex-only.
- Export: json-tailwind (v3) + css-tailwind (Tailwind v4 @theme CSS)
formats; 'tailwind' is a back-compat alias. New exit-code semantics
(export exits 0 regardless of source lint findings).
- Section order / duplicate headings are lint warnings, not file
rejection (verified: duplicate + out-of-order sections exit 0).
- Windows: documented the designmd dot-free bin alias (the design.md
bin name collides with the .md file association); skill declares
platforms: [windows].
- New pitfall: typography sub-property typos (fontwight) are silently
dropped with no finding as of 0.3.0.
All claims verified by running @google/design.md 0.3.0 live (lint,
export, duplicate-section, oklch token, starter template lints clean).
Docs page regenerated via generate-skill-docs.py.
* fix(skills): sync bundled + misc CLI skills to current upstream
Nine bundled and optional skills had stale flags, install URLs, packages, and paths. Verified each against upstream and corrected:
- vllm: removed bogus --enable-metrics/--metrics-port (metrics at /metrics on API port); --speculative-model -> --speculative-config; canonical HF model IDs
- lm-evaluation-harness: --tasks list -> lm-eval ls tasks; --allow_code_execution -> --confirm_run_unsafe_code
- weights-and-biases: wandb.keras import removed -> wandb.integration.keras (WandbMetricsLogger); log_uniform -> log_uniform_values for raw values
- huggingface-hub: upload-large-folder now deprecated; hf papers list -> ls
- openhue: Linux install 404 -> openhue_Linux_x86_64.tar.gz tarball (release repo openhue/openhue-cli, v0.24)
- apple-notes: memo notes -a is a bare flag, no positional title
- excalidraw: upload.py path skills/diagramming/... -> skills/creative/...
- searxng-search: removed Method 3 (searxng-data pip package is a PyPI 404)
- sketch: noted get-shit-done upstream is archived/unmaintained
* feat(desktop): date dividers in the sessions sidebar
Group the flat recents list and entered-project lanes by recency: an
unlabelled head of the newest run of sessions (cut at a real break in
activity, sized toward the most recent handful), then one divider per
coarse calendar range — Earlier today / Yesterday / Earlier this week /
Last week / Earlier this month / month / month + year. Empty ranges are
skipped, the first rendered group is never labelled, branch clusters
never split, and hand-ordered lists / pinned / project previews stay
divider-free.
* fix(desktop): let the pinned sidebar section grow to fit all pins
The pinned list was hard-capped at max-h-44 with an invisible scrollbar;
cap it at half the viewport instead so every pin is visible.
* feat(sessions): opt-in auto-archive of stale sessions + durable pin flag
New sessions.auto_archive / auto_archive_days config: soft-hide (never
delete) sessions with no activity for N days, aging on last activity
rather than creation so an old-but-active chat is spared. Sweeps are
throttled through state_meta and fire from CLI startup, gateway startup
+ hourly housekeeping, and the serve/dashboard backend (opportunistic
on session list + an hourly lifespan ticker), so every surface honours
one setting.
A new pinned column (declaratively migrated) exempts sessions from the
sweep; PATCH /api/sessions/{id} accepts pinned and flips the whole
compression lineage as a unit, mirroring set_session_archived.
* feat(desktop): auto-archive toggle + mirror sidebar pins to the backend
Sessions settings gain an "Auto-archive stale chats" toggle with a
configurable idle threshold, persisted to sessions.* in config.yaml so
the backend sweep owns the policy. Sidebar pins (localStorage) are
mirrored to the backend pinned flag at boot and on every change —
pre-existing pins migrate transparently — so the sweep can never hide a
pinned chat.
* test(sessions): use mock.patch for the config gate, matching file idiom
* test(gateway): account for the auto-archive construction-time sync escape
The gateway startup maintenance block gained a maybe_auto_archive call in
the same provably-off-loop __init__ site as maybe_auto_prune_and_vacuum;
bump the reviewed sync-escape count from 3 to 4.
* fmt(js): `npm run fix` on merge (#70845)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix: harden tui_gateway subprocess reads against Windows locale UnicodeDecodeError (#53137)
* fix: address review — add regression test, revert cosmetic churn, cross-link #61595
* fix(windows): widen utf-8 subprocess decode guard to sibling desktop-backend sites
The salvaged #61978 covers tui_gateway/server.py. The crash reported on
Jul 24 came from a sibling site it doesn't touch: the desktop update
panel's _recent_upstream_commits() in hermes_cli/web_server.py runs
git log with text=True and no encoding. Commit 84db32484f put a bug
emoji (UTF-8 f0 9f 90 9b) in a subject on main; byte 0x90 is undefined
in cp1252, so every Windows desktop install behind that commit crashed
in subprocess._readerthread during the update check (#52649).
Guard every text=True capture site in the desktop-backend process with
encoding='utf-8', errors='replace':
- hermes_cli/web_server.py: git log update panel, memory-provider setup
runner, WhatsApp bridge npm install, docker probe
- hermes_cli/banner.py: all 5 git sites (update check runs at startup)
- tui_gateway/host_supervisor.py: build-sha probe, ps probe, compute-host
Popen drain threads
- tui_gateway/compute_host.py: build-sha probe, ps rss probe
* Reapply "Merge pull request #30179 from NousResearch/feat/iron-proxy"
This reverts commit c6dc7c03c355fb3a407c1309aabebb13520c9efd.
* test(docker): update network-reuse harness fake ps output for egress-aware 3-field probe
test_docker_network_config.py landed on main after the #58489 revert and
stubbed docker ps with the 2-field ID\tState format. The re-landed
egress-aware reuse probe requests ID\tState\tEgressLabel when egress is
off, so the fake line failed to parse and the reuse path never fired.
Fixture-only change; production behavior is unchanged.
* feat(api): honor provider-aware request routing
Carry model, provider, and model_options through the API server's
execution surfaces (session chat, Chat Completions, Responses, /v1/runs)
without mutating global configuration. Precedence: session /model
override -> model_routes alias -> direct request selection -> global
defaults. Conflicting route/provider mixes fail closed with 400.
model_options stays request-scoped regardless of which selection wins.
Salvaged from PR #54426 by @abundantbeing.
* fix(api): gate bare-model passthrough + route-alias model leak
Follow-ups on the salvaged #54426 routing contract:
- Bare `model` without `provider` on the OpenAI-compatible endpoints
(/v1/chat/completions, /v1/responses) is now opt-in via
gateway.platforms.api_server.direct_model_requests (default off) —
generic OpenAI clients hardcode model names ('gpt-4o', ...) and
existing deployments rely on those falling back to the gateway
default. Explicit `provider` requests and the Hermes-native
session-chat + /v1/runs surfaces are always honored.
Idea credit: PR #22825 by @mssteuer.
- A model_routes alias with no `model` key can no longer leak the
alias string as the executing model name (defensive; parse-time
validation already drops such routes).
- Fix mis-indented _run_agent call args in _handle_session_chat_stream.
- Docs: document the opt-in flag.
* feat(desktop): add Arabic (ar) locale with RTL support
Arabic is the desktop app's first right-to-left locale. The i18n provider
now sets `document.dir`/`lang` from the active locale so Tailwind logical
utilities flip automatically, and `ar` is registered in the catalog,
language options, and alias table. The catalog is a partial `defineLocale`
so keys added to English later fall back cleanly.
Co-authored-by: 3ssiri <assiri@gmail.com>
Co-authored-by: Da7-Tech <286182457+Da7-Tech@users.noreply.github.com>
* feat(web): add Arabic (ar) locale with RTL support
Adds the Arabic catalog to the dashboard, registers it in the locale list
and picker, and flips the document direction to RTL when Arabic is active.
Introduces a `defineLocale` merge helper (mirroring the desktop app) so the
Arabic catalog can be a partial override that falls back to English for any
untranslated key instead of hand-porting every future string.
Co-authored-by: morolab <ahmedmoro@gmail.com>
* feat(i18n): add Arabic (ar) catalog for agent/CLI messages
Registers `ar` in the supported-language set and alias table and ships
locales/ar.yaml at full key and placeholder parity with en.yaml, covering
approval prompts and gateway slash-command replies. Identifiers, commands,
paths, config keys, model/provider names, and {placeholder} tokens are kept
verbatim.
Co-authored-by: Da7-Tech <286182457+Da7-Tech@users.noreply.github.com>
* fix(auth): stop stale-key credential recovery loops
Track the selected credential by stable pool entry ID so token refreshes and shared cursor movement cannot detach failures from the entry that issued them. Stop unmatched single-entry pools from reporting a no-op rotation as successful recovery.
Co-authored-by: Maxim Esipov <maksesipov@gmail.com>
* refactor: extract sync_credential_pool_entry_id helper
Replace 3 duplicated entry_id resolution blocks (try/except +
entry_id_for_api_key + fallback to None) in agent_init.py,
chat_completion_helpers.py, and switch_model with a single
sync_credential_pool_entry_id(agent) function in agent_runtime_helpers.
Follow-up to #70323.
* fix(url_safety): allow DNS failure in proxy/sandbox environments
When the runtime blocks direct DNS (NVIDIA OpenShell, Docker + Squid,
corporate proxy with DNS-only-via-proxy), socket.getaddrinfo() fails
and is_safe_url() blocks *all* requests — including legitimate public
URLs via the configured proxy.
Add _proxy_is_configured() helper that checks HTTPS_PROXY, HTTP_PROXY,
http_proxy, https_proxy, ALL_PROXY, all_proxy. When DNS fails AND a
proxy is configured, delegate DNS resolution to the proxy rather than
blocking outright.
Blocked hostnames (metadata.google.internal, 169.254.169.254, etc.)
are checked BEFORE DNS resolution, so cloud metadata endpoints remain
blocked regardless of proxy status.
Fixes #32217
* fix(url_safety): harden proxy DNS delegation — literal IPs stay fail-closed + regression tests
Follow-up on the salvaged #68469 commit:
- Literal-IP hostnames never take the proxy DNS-delegation path (a
getaddrinfo failure on a literal IP is not a proxy-environment
symptom, and IPs need no DNS) — keeps the private-IP/metadata floor
intact under proxy env vars.
- Adds TestProxyEnvironmentDnsDelegation: delegation fires only for
hostnames, metadata hostname/IP floor holds, DNS-success path
unchanged, empty proxy var ignored.
- Guards the three pre-existing DNS-failure tests against ambient
proxy env vars so they don't flake on developer machines.
* fix: apply _rewrite_compound_background in spawn_local to prevent worker deadlock on server backgrounding
Issue #68915: when the agent runs a compound command with trailing & (e.g.
`cd /app && node server.js &`), bash parses it as `(A && B) &` — a subshell
that holds the stdout pipe open forever when B is a long-running server.
The existing _rewrite_compound_background in terminal_tool.py correctly
rewrites this to `A && { B & }` to avoid the subshell fork, but it was only
applied in the foreground execute() path (tools/environments/base.py).
The background spawn_local() path bypasses base.py entirely and passed the
raw command directly to Popen/PTY, leaving the deadlock unmitigated.
Fix: apply _rewrite_compound_background in spawn_local() before the command
is passed to Popen or PTY spawn. Uses a lazy import to avoid circular
dependency (terminal_tool imports process_registry).
- PTY spawn path: now uses safe_command (rewritten)
- Popen spawn path: now uses safe_command (rewritten)
- Session.command still stores the original (unrewritten) command for display
- Simple `cmd &` is left unchanged (no subshell bug)
Tests: 4 regression tests verifying (1) compound is rewritten, (2) simple bg
is preserved, (3) multi-line compounds are rewritten, (4) session.command
stores original.
* fix(telegram): prevent connect hang with retry watchdog and fresh app per attempt (#67498)
The Telegram adapter's connect retry loop could silently stall after
'Connecting to Telegram (attempt 1/8)...' with the event loop permanently
parked in select() — all threads idle, no attempt 2/8 ever scheduled.
Root cause analysis:
- The retry loop reused the same Application object across all
8 attempts. After a failed initialize() the app could be in a partially-
initialized state (closed httpx transports from ,
or flag set before the hang) causing subsequent calls
to silently skip real initialization.
- CancelledError (a BaseException, not an Exception) propagated silently
through all except handlers with no logging — the task driving the retry
loop could exit without any trace.
- No total watchdog bound existed for the entire retry loop; only per-attempt
timeouts via _await_with_thread_deadline. If the loop itself stalled
between attempts (between-attempt sleep, cleanup, or scheduling), there
was no timeout to catch it.
Fixes:
1. **Total watchdog deadline**: Compute a total deadline for the entire
connect loop (8 attempts × init_timeout + 120s margin). Before each
attempt, check the wall clock; if exceeded, raise OSError immediately
instead of attempting another initialize().
2. **Fresh Application per retry**: On each failed attempt, rebuild
via and re-register all handlers. The old
app is best-effort shutdown with . This ensures
each retry starts with a clean slate — no stale transports, no stale
flag, no leaked state from the previous attempt.
3. **BaseException logging + propagation**: Added
(placed LAST after all other handlers) to log CancelledError and other
non-Exception signals before propagating. Previously these exited the
retry loop silently with no log message.
4. ** block for app rebuild**: The clause runs after
every failed attempt that isn't the last, rebuilding the app and
discarding the old one regardless of which exception class caused the
failure.
* chore: add contributor email mapping for agent@hermes.dev -> webtecnica
* fix(tui): refuse empty prompt.submit truncation without confirm
Stale truncate_before_user_ordinal=0 from a desynced Desktop client
resolved to history[:0] and replace_messages() wiped the durable
transcript. Require confirm_empty_truncate for that edge and have
intentional first-turn restore/regenerate paths send it.
* test(tui): cover empty truncate guard on prompt.submit
Refuse ordinal-0 wipes without confirm_empty_truncate; allow the
opt-in path used by first-turn restore/regenerate.
* fix: use error code 4028 (4025 already taken by session.handoff)
* fix(gateway): deliver relay-backed homes after restart
* fix(tui_gateway): recover custom provider identity from the session's model name
A session pinned to a named custom provider could silently reroute to the
user's default provider on resume/rebuild. Session rows persist the RESOLVED
provider — bare "custom" for every named providers:/custom_providers: entry —
and when no base_url survived in model_config, the existing heal
(canonical_custom_identity) had only the config.model.provider fallback left.
For users whose global default is a BUILT-IN provider (e.g. nous) that tier
cannot fire, so the bare provider was dropped, resume fell back to the default
provider with the session's custom model name, and the default endpoint 404'd
with "Model '<x>' not found. The requested model does not exist in our
configuration or OpenRouter catalog." Re-selecting via /model fixed it until
the next resume — the reported symptom.
Add a model-name recovery tier between the base_url reverse-lookup and the
config fallback: find_custom_provider_identity_by_model() maps the stored
model back to the entry that serves it (model/default_model/models catalog,
dict and legacy list shapes). The session row always stores the model, so the
entry identity survives even when the row has no base_url AND the global
default points elsewhere.
All five bare-custom heal sites in tui_gateway/server.py now pass the model:
_ensure_session_db_row, _stored_session_runtime_overrides,
_runtime_model_config, _make_agent, and _model_picker_context.
* fix(fallback): allow xai-oauth → xai failover on shared host/model
Base-url+model dedup was meant for custom shim aliases, but it also
skipped first-class providers that share an inference host while using
different credentials. That stranded xai-oauth spending-limit failover
to the xai API-key provider when both used the same model slug.
* test(fallback): cover xai-oauth → xai same-host same-model failover
Pin that a configured xai API-key fallback still activates when the
primary xai-oauth runtime shares api.x.ai and the same model slug.
* test: tighten spawn-rewrite assertions and add PTY-path coverage
Follow-up to salvaged PR #70549:
- Replace fragile 'or' assertions with single precise checks that catch
partial-rewrite regressions (would have masked a missing closing brace)
- Add test_pty_path_uses_rewritten_command covering the PTY spawn path
that was modified but previously untested
* chore: fix contributor attribution for desktop PR
* feat(desktop): add "Connect to existing Hermes" option to first-run onboarding
Adds a first-run Desktop choice between installing Hermes locally and
connecting to an existing remote Hermes gateway. The choice appears after
backend resolution but before ensureRuntime(), so selecting remote cannot
accidentally trigger local bootstrap.
New modules:
- first-run-setup-gate: concurrent first-run decision gate and reset semantics
- primary-backend-startup: Electron-free orchestration seam (saved remote
resolution, gate decision, remote re-resolution, local continuation)
- primary-connection-rehome: prevents dual-owner race where both cold boot()
and renderer softSwitch() could connect simultaneously
- first-run-remote-form: extracted remote form with stale-result guards
Reuses existing connection-config IPC, encrypted token storage, OAuth
session partition, and primary backend resolution.
Fixes #38602
Fixes #36970
* feat(desktop): add Webhooks page for subscription CRUD (#69687)
* feat(desktop): add Webhooks page for subscription CRUD
Brings the desktop GUI to parity with the dashboard's Webhooks page.
Adds a /webhooks route that lists webhook subscriptions, enables the
webhook gateway platform, and creates/toggles/deletes subscriptions,
hitting the same /api/webhooks* endpoints the dashboard and CLI use.
- types/hermes.ts: WebhookRoute, WebhooksResponse, WebhookCreatePayload,
WebhookCreateResponse, WebhookEnableResponse
- hermes.ts: getWebhooks, enableWebhooks, createWebhook, deleteWebhook,
setWebhookEnabled (profile-scoped) + type re-exports
- app/webhooks/index.tsx: WebhooksView (enable card, restart banner,
subscription list with copy/toggle/delete, create dialog with
one-time secret reveal); optimistic toggle, profile re-home
- routing: routes.ts, contrib/surfaces.tsx, chat/route-tile.tsx
- nav: command palette, keybinds (nav.webhooks), sidebar row
- i18n: en + zh full, types interface; ja/zh-hant fall back to English
- test: webhooks-rest.test.ts covers the REST helper contracts
* feat(desktop): surface Webhooks in the status bar instead of nav
Moves the Webhooks entry point from the sidebar nav / command palette /
keybind to a status bar action next to Cron, matching where scheduled
jobs live. The /webhooks route, page, and REST helpers are unchanged.
- use-statusbar-items.tsx: add webhooks action (Globe icon) after cron
- i18n: shell.statusbar.webhooks / openWebhooks (en, zh, types)
- revert nav wiring: sidebar row, command palette entry, nav.webhooks
keybind action + label, commandCenter.nav webhooks entry
* feat(desktop): add skills field to webhook create form
Exposes the backend's per-subscription skills list in the create dialog
(comma-separated) and shows skill badges on subscription rows, so this
page covers the skill-backed endpoint case as well as general CRUD.
- create form: Skills input; passes skills[] to createWebhook
- rows: render skill badges
- i18n: fieldSkills / fieldSkillsPlaceholder (en, zh, types)
Consolidates the skill-endpoint framing from #42817.
Co-authored-by: LionGateOS <98371158+LionGateOS@users.noreply.github.com>
* feat(desktop): render Webhooks as an overlay instead of a full page
Webhooks took over the whole workspace/chat pane because 'webhooks' was
missing from OVERLAY_VIEWS, so it routed through PageSearchShell while
cron rendered through the Panel overlay. Add it to OVERLAY_VIEWS and wire
it up like cron: mount WebhooksView as a floating overlay in wiring.tsx,
render null for the webhooks route in the workspace table, and convert
WebhooksView from PageSearchShell to the Panel primitive with an onClose.
Drop webhooks from route-tile BUILTIN_PAGES so it can't be tiled as a
page either.
* fix(desktop): place webhook URL copy button next to the URL
The URL span used flex-1, stretching it across the row and pushing the
copy button to the far right. Drop flex-1 so the span sizes to content
and the copy button sits directly after the URL.
* fix(desktop): top-align the deliver-only checkbox with its wrapped label
The label used h-9 items-center, centering the checkbox against the
two-line hint text. Switch to items-start so the checkbox aligns to the
first line.
* feat(desktop): give Webhooks a cron-style master/detail layout
Replace the single-column subscription list with the same Panel
master/detail cron uses: a left PanelList of subscription rows (status
dot + kebab menu) capped by a PanelAddButton, and a right PanelDetail
showing the selected subscription's deliver/events/skills, URL with copy,
description, and prompt. Enable/restart banners sit above the body; the
empty state keeps its own New subscription action.
* fix(desktop): drop header on the Webhooks empty state to match cron
The zero-subscriptions state still rendered the PanelHeader with the
title and the refresh/new buttons on the right. Remove it so the empty
state is just the centered PanelEmpty (icon, message, New subscription),
matching the cron empty state. Also drop the header from the loading
state.
* fix(desktop): top-align deliver-only checkbox and its wrapped label
Drop the min-h-9/pt-1.5 baseline shim that pushed the row down and made
the wrapped hint look misaligned. Use plain items-start with a mt-0.5 on
the checkbox so it sits at the first line, and wrap the hint in a
leading-snug span.
* fix(desktop): drop header refresh/new buttons; + button owns create flow
The populated Webhooks header carried a refresh icon and a New
subscription button. Remove both — the PanelAddButton at the bottom of
the list is the create flow, matching cron. Profile-change reload and the
refresh hotkey still run; the restart banner keeps its own refresh.
* fix(desktop): nudge deliver-only checkbox down 2px
Bump the checkbox top margin from mt-0.5 to mt-[4px] so it sits level
with the first line of the wrapped hint.
* refactor(desktop): reuse shared primitives on the Webhooks page
Address OutThisLife's review — stop reinventing primitives the app
already ships:
- copy: drop the local navigator.clipboard button for the shared
CopyButton (routes through the Electron clipboard bridge + haptic +
error state instead of swallowing failures)
- banners: enable/restart callouts now use Alert variant=warning
(primary color-mix tokens) instead of a hand-rolled amber palette
- toggle: detail Enable/Disable is a Switch (messaging idiom), not a
text ghost button
- checkbox: create dialog uses the Checkbox primitive, not a raw input
Rows/chips already moved to PanelListRow/PanelDetail/PanelPill in the
earlier cron-layout pass. Left the main-list fetch on manual load and
the local Field helper: cron itself does both, so useQuery here would
diverge from the reference idiom rather than align with it.
* refactor(desktop): move Webhooks fetch to the react-query layer
Replace the manual useState/useEffect load with useQuery keyed by
['webhooks', profileScope] — profile change re-fetches automatically, no
effect. reload() invalidates the query; the optimistic toggle writes the
cache via queryClient.setQueryData then invalidates so backend truth
wins. Load failures surface via an error-watching effect (react-query v5
dropped useQuery onError). Refresh hotkey calls refetch().
Left the create-dialog Field helper as-is: settings ListRow is a
side-by-side settings row, wrong for a stacked dialog form, and cron's
editor dialog (the reference) defines the same local Field.
* refactor(desktop): drop bespoke Field for shared ListRow/ToggleRow
Remove the local Field wrapper entirely. Every create-dialog field now
uses settings ListRow (wide, so label stacks over the full-width
control), the deliver-only pref uses ToggleRow (ListRow + Switch, haptic
baked in) instead of a bare Checkbox, and the created URL/secret reveal
rows use ListRow too. No component in this file is hand-rolled anymore.
* fix(desktop): pair webhook create fields into a 2-column layout
The single-column dialog scrolled awkwardly. Group fields: name +
description side by side, prompt full-width under them, events + skills
side by side, deliver-to + deliver-only side by side. Drop the
deliver-only help text and rename the label to 'Deliver payload only'
(remove the now-unused fieldDeliverOnlyHint i18n key from en/zh/types).
* fix(desktop): align Webhooks page with cron conventions and DESIGN.md
- Delete copy uses deleteDescPrefix + bolded name + deleteDescSuffix (no em-dash)
- Drop the duplicated detail-header Switch + Trash2; enable/disable and delete
live only in the row kebab, matching CronJobDetail
- Collapse three copyable-value chromes into one flat token-backed CopyValueRow
- Delete success toast gains a w.deleted title
- Replace hand-rolled delete Dialog with shared ConfirmDialog
---------
Co-authored-by: LionGateOS <98371158+LionGateOS@users.noreply.github.com>
* fmt(js): `npm run fix` on merge (#70914)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix(api-server): expose model options inventory
Add authenticated GET /api/model/options to the gateway API server,
sharing the dashboard/TUI picker payload builder so external clients
can sync to the user's configured Hermes provider catalog instead of
scraping the single OpenAI-compatible /v1/models alias.
- new shared hermes_cli.inventory.build_model_options_payload() wraps
build_models_payload with the stable picker shape and safe
custom-provider probe policy (probe current only on normal open,
probe all + cache bust on explicit refresh)
- dashboard web_server and TUI gateway model.options refactored onto
the shared builder; dashboard build moved off the event loop via
run_in_threadpool
- capabilities endpoint advertises model_options
- docs for both API server and programmatic integration
Salvaged from PR #54689 by @abundantbeing.
* feat(desktop): add Cron Blueprints to the GUI (#70066)
* feat(desktop): add Cron Blueprints to the GUI
The desktop app had a Cron jobs panel but no Blueprints tab, so the
parameterized automation templates the dashboard offers were unreachable
there (parity matrix: Dashboard=Y, GUI=N).
Adds a Jobs/Blueprints segmented toggle to the cron panel. The Blueprints
tab renders the catalog from the existing GET /api/cron/blueprints endpoint,
one card per template with a typed form (time/enum/weekdays/text slots).
Submitting POSTs to /api/cron/blueprints/instantiate, which fills the slots
and creates a real cron job via the same create_job path as a hand-written
one. The new job is merged into the shared atom so the Jobs tab
and sidebar reflect it immediately.
Backend already served both endpoints; this is desktop frontend only.
Strings added to all four locales.
* fix(desktop): stop cron blueprint cards clipping and tabs overlapping close X
Blueprint cards were wrapped in PanelBlock (a max-h-48 overflow-auto <pre>
for monospace code), which capped each card and forced an inner scroll,
clipping the copy. Use a plain auto-height card div so rows grow with their
content and the gallery scrolls as one.
PanelHeader actions sat under the overlay's absolutely-positioned close X
(no layout space reserved). Reserve pr-8 clearance when actions are present.
* refactor(desktop): narrow blueprint card i18n dep, document intentional scoping
Address review nits on the Cron Blueprints PR:
- BlueprintCard's submit useCallback depended on the whole t.cron object; it
only uses the blueprints slice. Bind const b = c.blueprints and use it (plus
narrow the dep) throughout the card.
- Document the intentional GET-vs-POST profile asymmetry on the blueprint
endpoints (global catalog vs per-profile instantiate) — the prior comment
claimed both were profile-scoped.
- Note why the blueprints tab collapses 'all' scope to 'default' (a blueprint
creates a real per-profile job; 'all' is not a writable target).
* fix(desktop): default blueprint delivery to This desktop, not origin
The blueprint catalog is shared with the dashboard, so its deliver slot
defaults to 'origin' (the chat/home-channel a dashboard or gateway job was
created from). Desktop has no origin chat and no home-channel picker, so the
seeded 'origin' rendered unlabeled and, at delivery, fell through the
home-channel fallback to nowhere when no gateway was configured.
Seed the deliver slot to 'local' (This desktop) when the backend default is
'origin' or empty, drop the origin option from the desktop dropdown, and label
the remaining options with the desktop's own delivery labels — matching the
manual cron editor (local/telegram/discord/slack/email). Also skip the
backend's origin-centric deliver help, which contradicts desktop semantics.
* refactor(desktop): align blueprint cards with the Panel/settings idiom
Address PR review (UI consistency with neighboring surfaces):
- Card container: drop the standalone-card look (bg-foreground/5) for the
shared in-panel grouping token bg-(--ui-bg-quinary), matching the cron
editor's in-surface groupings so blueprints sit in the Panel family.
- Form fields: replace the bespoke <label>+<Input> rows with the shared
ListRow primitive from settings/primitives (label+help on the left, control
on the right, stacks in a narrow pane) — same idiom as settings/messaging.
No behavior change; blueprint deliver remap and $cronJobs merge untouched.
* fix(desktop): satisfy eslint on the cron blueprint files
CI check:lint failed on import/export ordering and an unused import in the
blueprint changes:
- hermes.ts: sort AutomationBlueprint before AuxiliaryModelsResponse in the
type import + re-export blocks, and drop the unused AutomationBlueprintField
import (still re-exported for blueprints.tsx).
- cron/index.tsx: alphabetize the dialog/segmented-control imports and the
./blueprints vs ../shell/statusbar-controls group.
- blueprints.tsx: add the required blank lines between statements.
eslint --fix only; no behavior change. typecheck + blueprint tests green.
* refactor(desktop): blueprints reuse the cron editor dialog + shared card
Address review: the blueprint UI was still going its own way on the card and
form. Reuse the app's canonical pieces instead of a bespoke surface.
- CronEditorDialog gains a 'blueprint' mode: EditorState carries the blueprint
+ target profile, the dialog renders the typed slots with the same
Field/FieldHint/DialogFooter/error-block chrome as manual New cron, and
submit routes to instantiateAutomationBlueprint. One dialog, one editor state
machine. Resolves the accordion, the border-t divider, ListRow-vs-Field, the
ad-hoc buttons, and the plain error <p> in one move.
- Blueprint cards use selectableCardClass({ prominent: true }) (the shared
theme/pet/gateway card idiom), caller owns padding (p-2), whole card is a
button that opens the dialog pre-filled. No inline form.
- Gallery renders via PanelDetail, not PanelBody's master/detail row.
- i18n: drop the now-unused blueprints.setUp/cancel, add blueprints.dialogDesc
across en/ja/zh/zh-hant + types.
- Dropped the stray \u2014 literal comment.
Logic (origin->local deliver, desktopDeliverOptions, merge into $cronJobs) is
unchanged and stays unit-tested. typecheck + eslint + tests green.
* fix(desktop): dropdown no longer closes the cron dialog; unify deliver targets
Two cron-dialog bugs:
1. Dismissing any Select dropdown inside the cron editor dialog closed the whole
dialog. Radix portals Select/Popover content outside the dialog, so the
dismiss pointerdown reached the Dialog's DismissableLayer as an
outside-interaction. Guard DialogContent.onInteractOutside: swallow
interactions originating from a [data-radix-popper-content-wrapper] (a
dropdown dismiss inside our own dialog), compose with any caller handler. Fix
is at the shared Dialog level so every dialog benefits.
2. Blueprint deliver only offered 'This desktop'. The blueprint used the backend
blueprint field.options (configured gateways only) while the manual editor
hardcoded local/telegram/discord/slack/email regardless of what's connected.
Wire the desktop to GET /api/cron/delivery-targets (the documented single
source of truth, already used by the dashboard) via getCronDeliveryTargets,
and render both the manual editor and the blueprint deliver slot through one
shared DeliverSelect. Now all three surfaces agree and only offer connected
platforms; unconfigured-home-channel targets show a hint.
i18n: add cron.deliverNeedsHomeChannel across en/ja/zh/zh-hant + types.
typecheck + eslint + vitest green.
* fix(desktop): clicking away from an open dropdown no longer closes the dialog
The onInteractOutside guard only caught pointerdowns whose target was inside
the popper wrapper. But dismissing an open Select by clicking elsewhere inside
the dialog also closes the popover, which moves focus — and Radix Dialog reads
that as focusOutside and closes the whole dialog. (Radix Select 2.3.1 has no
modal prop, so that escape hatch isn't available.)
Guard both paths at the shared DialogContent level: onInteractOutside AND
onFocusOutside now swallow the event when it originates from a Radix popper OR
when any [data-radix-popper-content-wrapper] is open at event time (covers the
focus/re-dispatch case where the target is no longer the popper). A genuine
backdrop click with no dropdown open still closes the dialog. Export
isInteractionFromPopper + unit-test the three cases.
typecheck + eslint + vitest green (7 dialog tests).
* fix(desktop): portal popovers into their dialog so dropdowns don't close it
Root cause (affected every dialog, not just cron): Radix Select/Popover/
DropdownMenu portal to document.body — a SIBLING of the dialog, outside its DOM
subtree. Dismissing a dropdown (or clicking another field) moves focus out of
the dialog subtree, which the Dialog's modal FocusScope/DismissableLayer reads
as an outside interaction and closes the whole dialog. Separate body-level
portals also make z-index across the two fragile.
The earlier onInteractOutside/onFocusOutside guards treated symptoms and didn't
hold (and Radix Select 2.3.1 has no modal prop to disable its layer). Real fix
is a layering system: DialogContent publishes its content node via
DialogPortalContainerContext; SelectContent/PopoverContent/DropdownMenuContent
call usePopoverPortalContainer() and portal INTO that node when inside a dialog
(document.body otherwise). The popover is then a true DOM descendant of the
dialog — focus stays in, dismissal no longer closes the dialog, and both share
one stacking context so z-index is deterministic.
Test: with a Dialog open, an open Select's item is a descendant of the dialog
(portalled in), verified in jsdom. typecheck + eslint + component tests green.
* fix(desktop): bump radix-ui so dismissing a dropdown can't close its dialog
Upstream bug, not app-layer: with radix-ui 1.6.0 (dismissable-layer 1.1.13),
an open modal Select sets pointer-events: none on the dialog body, so a click
anywhere inside the dialog hit-tests through to the overlay. The Dialog's
DismissableLayer defers its outside-pointerdown decision to the click, but the
overlay is a registered dismissable surface exempt from the interception check
— so the Select swallowing the press didn't count, the deferred onDismiss
fired, and the dialog closed along with the dropdown.
dismissable-layer 1.1.17 adds shouldHandlePointerDownOutside, which makes the
dialog's layer ignore the press entirely while a higher layer has its pointer
events disabled. Bump radix-ui ^1.4.3 -> ^1.6.5 (dismissable-layer 1.1.17,
dialog 1.1.21, select 2.3.5); lockfile diff is Radix-only.
Repro test fires the pointerdown/up/click sequence on the overlay with a
Select open inside the dialog: red on 1.6.0 (dialog closed), green on 1.6.5.
Counterpart test keeps a genuine overlay click closing the dialog.
typecheck + dialog/select component tests green. Full-suite failures on this
Windows host reproduce identically without the bump (pre-existing env issues).
* fmt(js): `npm run fix` on merge (#70927)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix: add explicit UTF-8 encoding to all subprocess text=True calls (#53428)
On Windows with Chinese locale (GBK), subprocess.run(text=True) without
explicit encoding causes UnicodeDecodeError crashes. This fix adds
encoding='utf-8', errors='replace' to all subprocess.run() and
subprocess.Popen() calls that use text=True across 76 non-test Python files.
Fixes #53428 (master tracker for Windows GBK locale crash).
Note: credential_pool.py and electron changes excluded per reviewer request —
those will be submitted as separate focused PRs.
* fix: add explicit UTF-8 encoding to _op_whoami subprocess call (#53428)
PR #55339 adds encoding='utf-8', errors='replace' to 26 subprocess.run(text=True)
call sites across the codebase. The triage review (thanks @alt-glitch) diffed
this PR against #55339 and found that 5 of the 6 originally-touched call sites
are already covered there byte-identically:
- hermes_cli/main.py::_probe_container
- hermes_cli/setup.py SSH probe
- tools/tts_tool.py::_generate_neutts
- tools/transcription_tools.py::_prepare_local_audio
- tools/transcription_tools.py::_transcribe_local_command (both branches)
The one genuinely net-new site — hermes_cli/onepassword_secrets_cli.py::_op_whoami
(the 1Password op CLI whoami probe) — is NOT in #55339 and is fixed here.
Without explicit encoding=, text=True decodes child output with
locale.getpreferredencoding(False) — cp936 on Chinese Windows — which crashes
_readerthread on non-GBK bytes, cascading into pipe buffer fills, event loop
stalls, and TUI freezes (issues #47939, #53428, #57238).
Scope narrowed per triage feedback: the other 5 sites should land via #55339.
Refs #53428 (together with #55339).
* fix: extend UTF-8 encoding to _op_version probe (#53428)
Hermes-sweeper review on #60741 flagged that _op_version (the paired
op probe used by the same setup/status CLI flow at lines 127 and 205)
still ran text=True without explicit encoding/errors.
Add encoding='utf-8', errors='replace' to match _op_whoami and the
production op read path at agent/secret_sources/onepassword.py:271-278.
Also extend the regression test to cover _op_version alongside
_op_whoami, and update the module docstring to reflect the widened
scope. Test sensitivity verified: reverting the source change makes
test_op_version_passes_utf8_encoding fail with encoding=None.
* feat(linter): detect subprocess text=True without explicit encoding=
Adds a new rule to scripts/check-windows-footguns.py that flags
subprocess.run/Popen/call/check_output/check_call(..., text=True, ...) calls
missing an explicit encoding= kwarg.
On Chinese Windows (cp936/GBK) and other non-UTF-8 default codepages,
text=True without encoding= decodes child output with
locale.getpreferredencoding(False), crashing _readerthread with
UnicodeDecodeError on non-default-codepage bytes (issues #47939, #53428,
rule prevents future regressions.
Rule design:
- Pattern matches 'text=True' / 'text = True'
- post_filter skips lines that:
- already pass encoding= on the same line
- are method definitions (def text)
- contain text=True inside string literals
- are not subprocess-shaped calls (heuristic via _is_likely_subprocess_call)
- Two helper functions: _is_likely_subprocess_call, _looks_like_string_literal
- Multi-line calls where subprocess.X( and text=True are on different lines
are not flagged (acceptable false negative for a line-based scanner)
Also fixes the linter's own footgun: get_staged_files() and get_diff_files()
used subprocess.check_output(text=True) without encoding= — now fixed.
Suppresses 4 false positives on non-Windows platform-exclusive calls:
- tools/voice_mode.py (Termux/Android)
- tools/environments/singularity.py (Linux HPC)
- plugins/google_meet/cli.py (macOS system_profiler)
Test plan:
- 21 unit tests in tests/scripts/test_footgun_subprocess_encoding.py
- TestDetection: 6 cases verifying the rule flags real subprocess calls
- TestSuppression: 7 cases verifying false-positive avoidance
- TestHelpers: 7 cases for the two helper functions
- TestFullRepoScan: scans the whole tree and asserts the new rule finds
only the 7 call sites that PR #60741 fixes (or zero, once #60741 merges)
Verified: full-repo scan reports 7 matches on main (the #60741 sites),
4 platform-exclusive calls correctly suppressed, zero false positives.
* fix(windows): sweep remaining unguarded text-mode subprocess sites codebase-wide
AST-driven pass over every subprocess.run/Popen/check_output/check_call/call
with text=True (or universal_newlines=True) and no explicit encoding=:
append encoding='utf-8', errors='replace' at the kwarg site. 136 call
sites across 28 files (cli.py, hermes_cli/main.py, tools_config.py,
environments, computer_use, gateway, scripts, skills helpers, agent/*).
Together with the salvaged #55339/#60741 commits this closes out issue
#53428's bug class; the salvaged #60751 linter rule in
check-windows-footguns.py now enforces it repo-wide (verified: 807 files
scanned, zero findings).
* chore: map jinglun010@gmail.com -> jinglun010-cpu
* fix: repair sweep fallout — duplicate encoding kwargs, non-subprocess call sites, kwarg-snapshot tests
- Strip the salvaged commit's inline encoding kwargs where main had since
gained its own (process_registry, local env, cua doctor, gateway,
commands, gateway_windows — the latter keeps its locale-aware
_schtasks_encoding() from #38186)
- Revert encoding kwargs mistakenly applied to non-subprocess APIs
(exa get_contents, tempfile.mkstemp in webhook.py)
- Guard the ddgs worker Popen (new on main since #55339)
- Update two kwarg-snapshot test assertions for the new kwargs
* chore: map stoltemberg@users.noreply.github.com -> Stoltemberg
* test: update kwarg-snapshot assertions for the utf-8 subprocess guard
- whatsapp taskkill + webhook gh-comment assert_called_with: add the two
new kwargs
- test_status fake_run: accept **kwargs so signature-strict stub doesn't
TypeError on encoding/errors
* fix(cron): respect the platform-conditional decode design in _run_job_script + taskkill kwarg snapshot
cron/scheduler.py deliberately applies utf-8/replace only on Windows via
popen_kwargs (non-Windows keeps locale default per its test contract) —
drop the sweep's unconditional inline kwargs there. Update the gateway
force-kill kwarg snapshot for the new guard.
* fix(api_server): close divergence gaps from gateway/run.py
Three parity fixes between the API server and the native gateway's
agent-runtime resolution, integrated with the provider-aware request
routing that landed in #70853:
- Session-persisted model is honored: POST /api/sessions {"model": ...}
stores a model that the chat handlers previously fetched and threw
away. A stored value that matches a model_routes alias goes through
the route path (route provider/credentials apply); a raw model string
threads through as session_model, pinning the session's turns ahead
of per-request body values but below an explicit session /model
override.
- Empty-model recovery: provider-catalog default when config has no
model.default but a provider resolved, plus last-known-good model
recovery (#35314) keyed on gateway_session_key only (never ephemeral
session_id — no unbounded growth from one-off requests).
- Provider auth failures surface as controlled responses: RuntimeError
from _resolve_runtime_agent_kwargs() is re-raised as a dedicated
_ProviderAuthResolutionError at the call site, caught narrowly in
_run_agent() and the /v1/runs executor to return run.py's response
shape instead of an undifferentiated 500 (session-chat endpoints
previously returned a raw aiohttp 500 with no JSON body).
Salvaged from PR #57947 by @FvanW; session-model route-alias resolution
from PR #59941 by @kaishi00.
Co-authored-by: kaishi00 <kaishi00@users.noreply.github.com>
* feat(models): add anthropic/claude-opus-5 to OpenRouter and Nous Portal catalogs
Anthropic released Claude Opus 5 (+ -fast variant) — both are live on
OpenRouter and the Nous Portal /models endpoint (verified against both
live APIs). Opus 4.8 entries are kept.
- hermes_cli/models.py: opus-5 + opus-5-fast in OPENROUTER_MODELS;
opus-5 in _PROVIDER_MODELS[nous] (Portal serves both, curated list
carries the base model like the rest of the Nous Anthropic block).
Ordering: below fable-5 flagship, above opus-4.8.
- agent/model_metadata.py: claude-opus-5 -> 1M context (matches live
OpenRouter metadata).
- agent/reasoning_timeouts.py: claude-opus-5 -> 240s stale-timeout
floor (same as the opus-4.x thinking family).
- website/static/api/model-catalog.json: regenerated via
scripts/build_model_catalog.py.
Both providers bill via official_models_api (live pricing), so no
_OFFICIAL_DOCS_PRICING snapshot entry is needed for these routes.
* feat(api): backend-acknowledged session model lock with runtime routing
Add a persisted, backend-confirmed provider/model lock for Hermes
Browser and other session API clients. A confirmed lock is an
execution contract rather than response metadata:
- POST /api/sessions/{session_id}/model validates and persists a
confirmed browser_model_lock (advertised in /v1/capabilities)
- session chat + chat/stream consume the persisted lock on body-only
follow-up turns; a confirmed lock wins over an older gateway session
/model override and the session-persisted model
- a later successful session /model switch explicitly clears and
replaces the lock while preserving lineage markers (_branched_from)
and invalidating cached system-prompt model/provider metadata
- ordinary one-off request overrides never replace a confirmed lock
- provider-resolution failure fails closed as a typed provider-auth
error (controlled response, never global-credential reuse)
- confirmed locks disable the global fallback model chain
- the completed agent's actual provider/model must match the locked
route or the turn fails with a runtime-mismatch error
- responses carry sanitized runtime metadata reporting actual vs
requested provider/model and lock state
Rebased onto the provider-aware request routing (#70853) and
session-model parity (#70931) that landed since the original branch;
the lock now slots into that precedence chain as the top rung.
Salvaged from PR #61236 by @abundantbeing.
* fix(mcp): use encoding_error_handler='replace' for stdio transport
On Windows, pipe I/O can deliver non-UTF-8 bytes at chunk boundaries,
causing `UnicodeDecodeError` when the MCP SDK's `TextReceiveStream`
uses `errors="strict"`. Set `encoding_error_handler="replace"` on
`StdioServerParameters` so undecodable bytes become U+FFFD instead
of crashing.
* fix(gateway): pass encoding="utf-8" to read_text/write_text in update path (#37423)
* fix(gateway): add utf-8 encoding to dead target registry
* fix(gateway): cover discord update-response utf-8 path (#37423)
* fix(gateway): extend the utf-8 file-I/O guard to google_chat + whatsapp
Follow-up to the salvaged #38985: guard the 4 bare read_text/write_text
sites its allowlist missed (google_chat thread-count store + oauth JSON)
and add whatsapp/google_chat to the AST guard test's file list.
* fix(tools): utf-8 decode for STT/TTS command-provider popen_kwargs
Salvaged from PR #45099 — the two popen_kwargs dict sites the #70875
AST sweep missed because the kwargs are built indirectly
(_run_command_stt, _run_command_tts).
* fix(windows): platform._syscmd_ver stub in bootstrap + PYTHONUTF8 in desktop backend env
Two gaps found auditing the decode-crash cluster:
1. suppress_platform_ver_console() only ran in hermes_cli.main processes;
slash workers, tui_gateway/entry, run_agent, batch_runner, and cli.py
import only hermes_bootstrap and were exposed to both the console
flash and (on Python 3.11.0/3.11.1, which lack CPython's
encoding='locale' fix) a UnicodeDecodeError inside platform.win32_ver()
under PEP 540 — the crash #69413 reported. Move the stub into
hermes_bootstrap so every entry point gets it; the _subprocess_compat
copy stays for non-bootstrap callers.
2. The desktop Electron spawn built the backend env without PYTHONUTF8,
so anything the Python child emitted before hermes_bootstrap ran
(interpreter startup errors, pre-bootstrap tracebacks) decoded with
the locale default. Re-port of PR #56499's env half (echoriver89) to
backend-env.ts (original targeted the deleted backend-env.cjs);
explicit user setting wins.
* fix(compaction): strip proactive section headers from summary template
Remove three directive-heavy section headers from both the LLM
and deterministic summary templates that caused the agent to
resume stale tasks after context compression:
- Historical In-Progress State
- Historical Pending User Asks
- Historical Remaining Work
These sections read as actionable instructions even within a
REFERENCE-ONLY wrapper, hijacking the user's latest message.
The remaining sections are purely descriptive/past-tense.
Frozen prefix copies in _HISTORICAL_SUMMARY_PREFIXES updated
to match. Test 8/8 passed.
* fix(compaction): freeze pre-change SUMMARY_PREFIX generation, restore mutated entry
Address review on #69619: the previous commit mutated the newest frozen
entry in _HISTORICAL_SUMMARY_PREFIXES and never froze the live prefix it
retired (the generation with both the four-heading discard clause and
the tools-active clause). A summary persisted immediately before
upgrading was therefore treated as an ordinary message on
resume/re-compaction, keeping the old handoff text embedded in the body.
- Prepend the exact pre-change live prefix as a new frozen entry
(newest-first), leaving all existing frozen entries byte-identical
- Restore the Jul 2026 (#65848 class) frozen entry to its original
four-heading text
- Pin the retired generation as a literal in
test_summary_prefix_semantics.py so mutating or dropping it fails CI
- Make the #65848 tool-use regression position-agnostic (match the
pre-clause generation by content, not tuple index)
Verified byte-identity of both rescued generations against the parent
commit. 233 focused prefix/resume/compressor tests pass.
* test(compaction): byte-pin every frozen prefix generation
Hardening follow-up to the #69619 review fix. The previous regression
byte-pinned only the rescued pre-#69619 generation; older frozen entries
were covered solely by fragment assertions and a self-matching loop that
cannot detect a frozen entry mutating (the loop tests each entry against
itself).
- Pin all four _HISTORICAL_SUMMARY_PREFIXES generations as literals in
_FROZEN_PREFIX_GENERATIONS and assert order-sensitive tuple equality
plus detect/strip for each
- State the prepend-only contract explicitly on the tuple: never mutate
or reorder existing entries
Negative controls verified: mutating, dropping, or reordering a frozen
entry each fail the new test, while the legacy self-matching loop still
passes under mutation — confirming the closed coverage gap.
* chore: map contributor akb4q
* fix(macos): use launchctl submit instead of start_new_session for plist reload helper (#69098)
The deferred launchd reload helper used start_new_session=True to detach
from the gateway's process group. However, setsid(2) alone does NOT move
the child outside the launchd job's process coalition — whe…
…lel.py (NousResearch#43646) * fix(ci): append filesystem forensics when a per-file pytest run exhausts exit-4 retries A PR-added test file (tests/test_iron_proxy.py, PR NousResearch#30179) repeatedly failed exactly one CI shard with 'ERROR: file or directory not found' across 4 runs (including a fresh merge SHA on fresh runners), while the identical slice passes locally against the same merge commit and a tree-integrity watcher confirms no sibling test mutates the repo. Three unrelated branches showed the same one-shard signature the same day. We currently cannot attribute these because the log only carries pytest's exit-4 line. This adds a forensics block to the captured output when exit-4 survives the retry loop: - does the file exist NOW (post-retries) - parent dir entry count + similarly-named entries - git status --porcelain dirty-entry count + first 10 entries Zero behavior change: rc stays 4, retries unchanged, forensics wrapped in a broad try/except so they can never mask the failure. Two new tests cover the exhausted-retries and genuinely-missing paths. * chore: drop the two forensics tests — ship the runner change only
20 participants
Summary
Adds an optional, off-by-default TLS-intercepting egress proxy for remote terminal sandboxes. When enabled, the sandbox holds opaque proxy tokens; iron-proxy swaps them for real provider API keys at the network boundary. Compromise the sandbox and the attacker walks away with tokens that only work from behind the configured trusted proxy boundary.
Wraps ironsh/iron-proxy (Apache-2.0, Go binary). Same lazy-install pattern as the Bitwarden integration — pinned version, SHA-256 (+ best-effort GPG) verified download into
~/.hermes/bin/iron-proxy, no apt or sudo required.Rebased onto current
mainand live-verified end-to-end against the real v0.39.0 binary (see Validation).What lands
agent/proxy_sources/iron_proxy.pyhermes egress …)hermes_cli/proxy_cli.pyproxy:section)hermes_cli/config.pyhermes_cli/main.pytools/environments/docker.py/egressslash (CLI + gateway)cli.py,gateway/run.py,hermes_cli/commands.py/api/egress/status+ schema overrideshermes_cli/web_server.pyapps/desktop/src/app/command-palette/index.tsxhermes_cli/setup.pytests/test_iron_proxy{,_cli,_e2e}.py,tests/tools/test_docker_environment.pywebsite/docs/user-guide/egress/,website/docs/developer-guide/egress-internals.mdNew surfaces
Plus
/egressin the CLI and every messaging gateway, and a read-only/api/egress/statusfor the dashboard/desktop. Namedegressbecausehermes proxyis the inbound OAuth reverse proxy — different direction, different purpose.UX (make change → applied a one-command path)
hermes egress restartapplies any config / token / Bitwarden-rotation change in one command.hermes egress setupstops the running daemon to pick up new config, then offers to restart it for you (asks on a tty;--restartalways,--no-restartnever) — no more "I changed the allowlist but forgot to restart" footgun.setupreads keys from~/.hermes/.envwhen they aren't exported, so first-run discovery isn't confusing for.env-only setups.Bitwarden integration
Composes with Bitwarden Secrets Manager. With
hermes egress setup --from-bitwarden, real upstream credentials are pulled from a BSM project at proxy startup; rotating a key in the Bitwarden web app propagates to sandboxes on the nexthermes egress stop && start, without touching.env. Fail-loud at start when the access token / project_id is missing, withproxy.allow_env_fallbackas the documented escape hatch.Validation
npx docusaurus build)Live verification against the real v0.39.0 binary (isolated
HERMES_HOME, real docker, real openssl):install→ real download + SHA-256 verify ✓setup→ CA generated, tokens minted for OPENROUTER + OPENAI, config + mappings written ✓start/status/stop→ bound docker bridge172.17.0.1:9090(plain-HTTP on9091), listening, clean teardown ✓secretstransform swappedOPENROUTER_API_KEYin theAuthorizationheader against realopenrouter.ai— proxy token in, real secret out ("swapped":[{"secret":"OPENROUTER_API_KEY","locations":["header:Authorization"]}]in the audit log) ✓sk-or-…keys never enter the sandbox env; CA mounted read-only ✓enforce_on_docker: truerefuses sandbox creation when the proxy is enabled-but-down, with an actionable error ✓Security model
Protects against: prompt-injected agent in a sandbox reading creds, compromised sandbox dependency phoning home, SSRF to cloud metadata (
169.254.169.254, incl. the IPv4-mapped-v6 form).Does NOT protect: a compromised host process (real keys are in host env regardless), sandboxes that bypass
HTTPS_PROXYvia raw sockets, exfil to allowlisted hosts, or loss of the trusted-proxy boundary itself (stolen CA key / redirected egress). Defense-in-depth for the sandbox layer, not a replacement for host security.Scope cuts (v1)
secretstransform; x-api-key / SigV4 / signature providers (Anthropic native, Bedrock, Azure, Gemini) are surfaced as uncovered, with an opt-infail_on_uncovered_providersblock for the LLM-specific tier.Infographic