feat: Add 1Password Secrets Manager integration for gateway credential bootstrap - #106
feat: Add 1Password Secrets Manager integration for gateway credential bootstrap#106dizhaky wants to merge 39 commits into
Conversation
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
3 |
First entries
tests/test_onepassword_secrets.py:14: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
agent/secret_sources/onepassword.py:268: [unresolved-import] unresolved-import: Cannot resolve imported module `onepassword`
agent/secret_sources/onepassword.py:321: [unresolved-import] unresolved-import: Cannot resolve imported module `onepassword.client`
✅ Fixed issues: none
Unchanged: 5099 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}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42106ff50c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fm = field_mapping or {} | ||
|
|
||
| try: | ||
| secrets, warnings = asyncio.run( |
There was a problem hiding this comment.
When the long-lived gateway handles a turn, async _run_agent() calls _reload_runtime_env_preserving_config_authority() (gateway/run.py:16204-16207), which reaches this asyncio.run() while the gateway loop is already running. Python raises RuntimeError in that situation, and apply_onepassword_secrets() swallows it and returns no secrets, so service-account or credential rotations are never picked up after startup. Use an async fetch path or execute the SDK coroutine on a separate thread/loop.
Useful? React with 👍 / 👎.
| cache_key = (vault_name, item_title) | ||
| if use_cache: | ||
| cached = _CACHE.get(cache_key) | ||
| if cached and cached.is_fresh(cache_ttl_seconds): | ||
| return cached.secrets, [] |
There was a problem hiding this comment.
When OP_SERVICE_ACCOUNT_TOKEN rotates or a process switches to another service account while retaining the same vault and item names, this key returns secrets fetched under the previous identity for the full TTL. It also ignores field_mapping, so mapping changes can return keys produced by the old mapping. The existing Bitwarden implementation fingerprints its access token for exactly this cache boundary; include a non-reversible token fingerprint and the mapping configuration here as well.
Useful? React with 👍 / 👎.
|
|
||
| try: | ||
| result = subprocess.run( | ||
| ["pip", "install", "--quiet", "onepassword-sdk"], |
There was a problem hiding this comment.
Whenever the SDK is absent— including normal startup because auto_install defaults to true—this installs an unconstrained package directly into the runtime environment, allowing a future incompatible or compromised release to enter a running gateway outside pyproject.toml and uv.lock. The local pip install --help identifies this operand as a requirement specifier, but the current bare name supplies no version constraint; declare a supported bounded version and regenerate the lock instead.
AGENTS.md reference: AGENTS.md:L313-L320
Useful? React with 👍 / 👎.
| "command": "npx", | ||
| "args": ["-y", "@modelcontextprotocol/server-memory"] |
There was a problem hiding this comment.
On a developer machine without a cached copy, this npx -y entry resolves and executes an unversioned remote package. The checked local npx --help usage explicitly supports <pkg>[@<version>], so an exact reviewed version can be supplied rather than silently executing whichever release is current when Claude starts.
AGENTS.md reference: AGENTS.md:L313-L315
Useful? React with 👍 / 👎.
| secrets_cfg["enabled"] = True | ||
| secrets_cfg["service_account_token_env"] = token_env | ||
| secrets_cfg["vault"] = vault_name | ||
| secrets_cfg["item"] = item_title | ||
| secrets_cfg.setdefault("field_mapping", {}) |
There was a problem hiding this comment.
These new persistent secrets.onepassword options are written by setup and read by multiple loaders, but the commit leaves hermes_cli/config.py::DEFAULT_CONFIG with only the Bitwarden subsection. Consequently default/schema-oriented consumers do not know about the integration and its defaults are duplicated ad hoc across the implementation. Add the complete subsection next to Bitwarden; adding keys to the existing section does not require a config-version bump.
AGENTS.md reference: AGENTS.md:L334-L342
Useful? React with 👍 / 👎.
| if result.error: | ||
| print( | ||
| f" Bitwarden Secrets Manager: sync error ({type(result.error).__name__ if result.error else 'unknown'})", | ||
| file=sys.stderr, |
There was a problem hiding this comment.
Whenever Bitwarden returns an error, FetchResult.error is a string, so this expression always prints sync error (str) regardless of whether the actual problem is a missing token, missing project, unavailable binary, timeout, or authentication failure. This regresses the previous startup output, which surfaced the actionable message, and leaves existing Bitwarden users unable to tell how to recover; redact sensitive subprocess details if necessary, but retain a safe error category or remediation.
Useful? React with 👍 / 👎.
| logger.warning( | ||
| "1Password secrets fetch failed (%s) — run " | ||
| "`hermes secrets onepassword status` for details", | ||
| type(exc).__name__, |
There was a problem hiding this comment.
For failures such as a revoked token, inaccessible vault, missing item, or SDK network error, startup directs the user to hermes secrets onepassword status for details, but get_onepassword_status() only checks SDK importability, token presence, and static configuration and never attempts a fetch or records the last error. The command can consequently show every local check as healthy while providing none of the promised failure details; perform a non-cached connectivity check or persist and surface a safe failure category.
Useful? React with 👍 / 👎.
| item_overviews = await client.items.list(vault_id) | ||
| for overview in item_overviews: | ||
| if not item_title or overview.title == item_title: | ||
| target_item = await client.items.get(vault_id, overview.id) |
There was a problem hiding this comment.
When a service account can see multiple vaults with the configured name, or a vault contains multiple items with the configured title, the lookup silently selects the first result returned by the SDK. 1Password permits duplicated item titles, so this can inject credentials from the wrong item without any warning. Reject ambiguous matches or allow stable vault and item IDs to be configured instead of choosing an arbitrary first match.
Useful? React with 👍 / 👎.
| setup.add_argument( | ||
| "--service-account-token", | ||
| help="Provide the service account token non-interactively (will be stored in .env)", | ||
| ) |
There was a problem hiding this comment.
When non-interactive setup is invoked with --service-account-token, this help text says the token will be stored in .env, but the handler only assigns it to the setup subprocess's os.environ and later explicitly says it was not stored. Automation following the option contract can therefore complete setup successfully and then find that the next Hermes process has no bootstrap token. Update the option contract or provide an explicit, secure persistence mechanism.
Useful? React with 👍 / 👎.
| name = label.upper() | ||
| name = re.sub(r"[ \-]", "_", name) | ||
| name = re.sub(r"[^\w]", "", name) | ||
| return name |
There was a problem hiding this comment.
When a 1Password field label contains non-ASCII letters, such as clé api, Python's Unicode-aware \w, isalpha(), and isalnum() produce and accept a name such as CLÉ_API. That is not a portable POSIX shell variable identifier, so the integration silently installs a key that normal shell-based consumers cannot reference instead of warning or deriving the expected ASCII name. Validate against explicit ASCII ranges such as [A-Z_][A-Z0-9_]*.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dba284c54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "vault_name": "", | ||
| # Title of the 1Password item whose fields are mapped to env vars. | ||
| "item_title": "", |
There was a problem hiding this comment.
Fresh evidence in the newly added DEFAULT_CONFIG subsection is that it declares vault_name, item_title, and token_env, while apply_onepassword_secrets(), status, setup, and sync exclusively read/write vault, item, and service_account_token_env. A user who enables the integration directly through the canonical config defaults will therefore have their item, vault, and custom token variable ignored, causing startup to report that no item or token is configured; use the same key names throughout the subsection and readers.
AGENTS.md reference: AGENTS.md:L334-L342
Useful? React with 👍 / 👎.
| return _sdk_version() | ||
|
|
||
| pkg = "onepassword-sdk>=0.1.0,<2.0.0" | ||
| cmd = ["pip", "install", "--quiet"] |
There was a problem hiding this comment.
When Hermes is launched through an absolute virtualenv executable but that virtualenv's bin directory is not first on PATH—a common service configuration—bare pip can install into a different Python or be unavailable. The subprocess can then succeed while _sdk_version() reports not installed, and the subsequent fetch still fails because the running interpreter cannot import the SDK. The checked local python --help defines -m mod as running the module under the selected interpreter, so invoke sys.executable -m pip install ... as the repository's other dependency installers do.
Useful? React with 👍 / 👎.
| ) | ||
| continue | ||
|
|
||
| secrets[env_name] = value |
There was a problem hiding this comment.
When two item fields normalize to the same environment name—for example, API KEY and API-KEY both become API_KEY, or two explicit mappings target the same name—this assignment silently replaces the first credential with whichever field the SDK returns last. The gateway can consequently inject an arbitrary wrong API key without any warning; detect duplicate target names and reject or skip the ambiguous mapping rather than relying on field order.
Useful? React with 👍 / 👎.
| if not override_existing and os.environ.get(key): | ||
| continue |
There was a problem hiding this comment.
Fresh evidence after the async-path fix is that the long-lived gateway still cannot apply rotated item values with the setup default override_existing: false: the first fetch leaves each key in os.environ, and every later per-turn reload hits this condition and skips that key even after the cache expires and 1Password returns a new value. The gateway therefore keeps the original credential until restart or manual removal; distinguish values previously injected by 1Password from unrelated shell or dotenv values so the source can refresh its own keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a444e6412
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not override_existing and os.environ.get(key): | ||
| # Always refresh keys that 1Password previously injected so that | ||
| # credential rotation takes effect without requiring override_existing. | ||
| if previously_managed is None or key not in previously_managed: | ||
| continue |
There was a problem hiding this comment.
Fresh evidence in the rotation fix is that it identifies previously managed secrets by name alone. On every gateway turn, load_hermes_dotenv() first reloads the user .env with override=True, but _SECRET_SOURCES retains the old onepassword label; therefore, if an operator adds or changes that key locally, this branch still treats it as 1Password-managed and overwrites it despite override_existing: false. Track the last injected value or clear the source label when dotenv replaces a key so only an unchanged managed value is refreshed.
Useful? React with 👍 / 👎.
| applied: Dict[str, str] = {} | ||
| for key, value in secrets.items(): |
There was a problem hiding this comment.
When a field is deleted or renamed in 1Password while the gateway is running, it is absent from secrets, so this loop never touches the corresponding environment variable and _SECRET_SOURCES continues marking it as managed. Per-turn synchronization consequently leaves the stale credential active until the process restarts, even though subsequent fetches succeed. Reconcile the fetched names against previously_managed and remove absent values when they have not been replaced by another source.
Useful? React with 👍 / 👎.
| setup.add_argument( | ||
| "--service-account-token", | ||
| help="Token used for this setup session only; set OP_SERVICE_ACCOUNT_TOKEN in your environment for persistent use.", |
There was a problem hiding this comment.
When automation supplies --service-account-token ops_..., the credential is exposed in the process command line for the duration of SDK installation and network setup, and it commonly remains in shell or job-runner history afterward. Since this token can read every vault granted to the service account and the command already supports OP_SERVICE_ACCOUNT_TOKEN, remove the argv form or accept the secret through a masked prompt, environment variable, or stdin.
Useful? React with 👍 / 👎.
| # Auto-install the SDK if requested and not present. | ||
| if auto_install and not _check_sdk_available(): | ||
| try: | ||
| install_onepassword_sdk() |
There was a problem hiding this comment.
When security.allow_lazy_installs: false or HERMES_DISABLE_LAZY_INSTALLS=1 is set for an audited, restricted, or air-gapped deployment, enabling 1Password with its default auto_install: true still executes pip during ordinary environment loading. This bypasses the repository-wide gate in tools.lazy_deps.ensure() that promises to disable runtime installs entirely; register the SDK in the lazy-dependency allowlist or explicitly check the same gate before invoking pip.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2740b54aa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| ) | ||
| try: | ||
| secrets, warnings = future.result(timeout=SDK_TIMEOUT_SECONDS + 5) |
There was a problem hiding this comment.
When the 1Password cache expires in the gateway, async _run_agent() calls the synchronous loader at gateway/run.py:16207, and this future.result(...) blocks the gateway event-loop thread for the entire network fetch. Fresh evidence after the running-loop fix is that moving asyncio.run() to a worker avoids the exception but still waits synchronously; additionally, if the 35-second timeout fires, exiting the ThreadPoolExecutor context waits for the unfinished worker, so the timeout does not release the loop. During a slow or unavailable 1Password request, all gateway message handling and platform tasks can therefore stall; expose an awaitable fetch path or offload the complete reload without synchronously waiting on the event loop.
Useful? React with 👍 / 👎.
| @@ -4,7 +4,7 @@ let | |||
| src = ../ui-tui; | |||
| npmDeps = pkgs.fetchNpmDeps { | |||
| inherit src; | |||
| hash = "sha256-q3Dqx7B9AK/H7ji/XoMkLOxUNH0uTUqoemu+hSiqr5I="; | |||
| hash = "sha256-it6R+yQPfM0EiHLaQLOmJe1nDp524aFwVY+oALt/ld8="; | |||
There was a problem hiding this comment.
The ui-tui source tree is byte-for-byte identical to both the parent and commit 7501574, which explicitly validated the previous sha256-q3Dq... hash, but this commit replaces only the digest. Since fetchNpmDeps hashes the unchanged npm dependency closure, the new value causes a fixed-output hash mismatch and prevents the Nix TUI package from building; retain the validated hash unless the corresponding package inputs actually change.
Useful? React with 👍 / 👎.
| @@ -4,7 +4,7 @@ let | |||
| src = ../web; | |||
| npmDeps = pkgs.fetchNpmDeps { | |||
| inherit src; | |||
| hash = "sha256-peA7M8lvRRVnsM6vl4QbYEr0ElEaGU4zsCENnq8TLBc="; | |||
| hash = "sha256-0jiIHktqOGM44RQfhf1CeQmpzdUvlGBOTmhTNn6JMnc="; | |||
There was a problem hiding this comment.
The web source tree is also unchanged from the parent and commit 7501574, where the prior sha256-peA7... value was explicitly updated for this exact dependency tree. Replacing only that fixed-output digest makes fetchNpmDeps reject the fetched closure, so the Nix web dashboard build fails before compilation; restore the validated hash unless a package manifest or lockfile is changed with it.
Useful? React with 👍 / 👎.
2740b54 to
35709bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba586ce5e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cmd = [sys.executable, "-m", "pip", "install", "--quiet"] | ||
| if force: | ||
| cmd.append("--force-reinstall") | ||
| cmd.append(pkg) |
There was a problem hiding this comment.
On a standard fresh Hermes environment created by the README's uv venv flow, the virtualenv does not contain the pip module, so this command fails with No module named pip; both the setup wizard and the default gateway auto-install path then leave the SDK unavailable and cannot load any 1Password credentials. The repository's hermes_cli/tools_config.py::_pip_install explicitly handles this environment with a uv-first and ensurepip fallback, so this installer should use the same strategy.
Useful? React with 👍 / 👎.
| secrets, warnings = op.fetch_onepassword_secrets( | ||
| token=token, | ||
| vault_name=vault_name, | ||
| item_title=item_title, | ||
| use_cache=False, | ||
| ) |
There was a problem hiding this comment.
When setup is rerun after secrets.onepassword.field_mapping has been customized, this test fetch ignores that mapping even though the wizard preserves it when saving the config. Setup can therefore report usable variables while the next real startup applies different names or skips fields because the retained mapping is invalid or colliding; pass the current mapping into this validation fetch so setup tests the configuration it enables.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb09e8162c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| # ------------------------------------------------------------------ vaults | ||
| all_vaults = await asyncio.wait_for(client.vaults.list(), timeout=SDK_TIMEOUT_SECONDS) |
There was a problem hiding this comment.
When any configured integration performs its first fetch, the official onepassword-sdk clients expose vault and item enumeration as vaults.list_all() and items.list_all(vault_id), not list(). Consequently this line raises AttributeError immediately after authentication (and line 256 would do the same for items), so setup validation and gateway credential bootstrap cannot retrieve any secrets. Replace both enumeration calls with the SDK's list_all APIs.
Useful? React with 👍 / 👎.
| "Install manually: pip install 'onepassword-sdk>=0.1.0,<2.0.0'" | ||
| ) | ||
|
|
||
| pkg = "onepassword-sdk>=0.1.0,<2.0.0" |
There was a problem hiding this comment.
Fresh evidence after adding a ceiling is that the default auto-install still accepts onepassword-sdk>=0.1.0,<2.0.0. Because this is a pre-1.0 dependency, that range admits every potentially breaking 0.x minor release and even 1.x during ordinary gateway startup, rather than the repository-required limited minor window; select a supported current floor and cap it according to the pre-1.0 policy (and lock it in the project dependency metadata).
AGENTS.md reference: AGENTS.md:L324-L328
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 579d6581d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not _lazy_ok: | ||
| raise ImportError( | ||
| "1Password SDK auto-install is disabled (HERMES_DISABLE_LAZY_INSTALLS). " | ||
| "Install manually: pip install 'onepassword-sdk>=0.1.0,<0.2.0'" | ||
| ) |
There was a problem hiding this comment.
When security.allow_lazy_installs is false or HERMES_DISABLE_LAZY_INSTALLS=1, this unconditional gate also rejects the user-initiated hermes secrets onepassword install command because cmd_op_install() calls this same function. Those settings are intended to prohibit automatic runtime installs while allowing operators to provision dependencies manually, so the integration's provided manual installation command becomes unusable in exactly the restricted environments that need it; apply the gate only from the auto-install path or distinguish explicit installs.
Useful? React with 👍 / 👎.
Implements `hermes secrets onepassword` subcommands (setup, status, sync, disable, install) backed by the `onepassword-sdk` Python package. Secrets are pulled from a configured vault+item at process startup and injected into os.environ, with in-process caching and graceful failure on any error. Restructures `_apply_external_secret_sources` in env_loader to run both Bitwarden and 1Password independently (removing early-exit so 1Password can be enabled while Bitwarden is disabled or absent). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Add `# type: ignore[import-not-found]` to the two lazy `import onepassword` and `from onepassword.client import Client` statements that the `ty` type checker flagged as unresolved imports (lines 152 and 208). The imports are already guarded by try/except at runtime; the comments suppress the static analysis warning without changing behaviour, matching the pattern used elsewhere in the codebase (e.g. agent/google_oauth.py). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…tronger hash for cache key - agent/secret_sources/onepassword.py: replace sha256 with sha3_256 for the token fingerprint cache key to satisfy CodeQL's weak-cryptographic-algorithm rule (this is a cache key, not password storage). - agent/secret_sources/onepassword.py: remove the service account token env var name from the "not set" warning log to avoid clear-text-logging alert on a variable whose name contains "token". - agent/secret_sources/onepassword.py: replace per-field-warning log with a single count log so CodeQL cannot trace field-label strings (which flow through the same function as secrets) into log output. - hermes_cli/env_loader.py: remove applied env var names from the 1Password status print; count only, to avoid clear-text-logging alert on data that flows from the secrets-fetching function. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…sis, redact exception from 1password error log
- Bitwarden block changed from `if bw_cfg.get("enabled"):` guard back to
`if not bw_cfg.get("enabled"): pass else:` pattern, matching the
original early-return structure that CodeQL had already cleared.
- 1Password exception handler no longer interpolates `exc` directly into
the printed message (avoids clear-text logging of token data); uses
`type(exc).__name__` instead and routes full detail to logger.warning
with exc_info=True.
- Added `import logging` and module-level `logger` to support the above.
…word warning log
Replace `logger.warning("... %s", exc)` with `logger.warning("... %s", type(exc).__name__,
exc_info=True)` in apply_onepassword_secrets. CodeQL's py/clear-text-logging-sensitive-data
rule traces: token (os.environ.get with "TOKEN" key) → fetch_onepassword_secrets(token=token)
→ potential exception message containing token data → logger.warning("%s", exc). Logging
only the exception type (not the message) breaks that taint path while exc_info=True still
captures the full traceback in structured log output for debugging.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Replace str(exc) interpolation in RuntimeError with type(exc).__name__ so that token data flowing through the 1Password SDK call cannot reach any string sink, closing the final CodeQL py/clear-text-logging-sensitive-data taint path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
The exception from install_onepassword_sdk() originates from pip's subprocess, not from any token or secret data. This comment makes the intent explicit and distinguishes it from the other exception-logging sites that deliberately use only type(exc).__name__ to avoid leaking token data. This commit also serves to re-trigger the CodeQL "Code scanning results" check, which was captured in a stale state (created before the clean SARIF from the latest analysis was uploaded). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Adds @modelcontextprotocol/server-memory (Knowledge Graph MCP Server) via npx alongside the existing codebase-memory server. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
Resolves the unresolved chatgpt-codex-connector findings from PR #106's review rounds that weren't already covered by earlier commits on this branch: - Cache 1Password fetches by (vault, item, token, field_mapping) instead of just (vault, item), so a rotated service account token or changed field mapping can never serve stale secrets fetched under a different identity for the rest of the TTL. - Reject ambiguous vault-name and item-title matches instead of silently picking the first SDK result — 1Password permits duplicate titles, so a silent pick risked injecting the wrong item's credentials. - Derive env var names from ASCII only; non-ASCII field labels (e.g. "clé api") no longer produce non-portable Unicode-derived names. - `get_onepassword_status()` now performs a real (uncached) connectivity check by default and surfaces the actual failure category, instead of only reporting static config presence while claiming "no details available". - `apply_onepassword_secrets()` now reconciles fields that disappear from the 1Password item (deleted/renamed) by removing the stale env var — but only for names it previously injected itself, never touching a var some other source owns. Returns `(applied, removed)`. - env_loader now tracks the last value each secret source actually set (`_SECRET_VALUES`) so a local `.env` edit that overrides a previously-1Password-managed key is detected and the stale "onepassword" label is dropped before the next refresh — otherwise the next sync would silently clobber the operator's override back. - Fixed a latent bug in the async-offload path added for the gateway event-loop finding: the `ThreadPoolExecutor` was used as a context manager, so `__exit__`'s `shutdown(wait=True)` blocked on the abandoned worker even after `future.result(timeout=...)` raised `TimeoutError` — defeating the timeout. Now shuts down with `wait=False` on both the timeout and success paths. - `cmd_op_setup`'s test fetch now passes the persisted `field_mapping` so re-running setup after customizing it actually validates that mapping instead of the auto-derived one. - Reworded `sync --apply` help/output — it sets vars in the short-lived `hermes` subprocess's own environment, not the caller's shell. Verified the fixes already landed earlier on this branch (asyncio.run inside a running loop, DEFAULT_CONFIG registration, SDK version pinning, collision detection, lazy-install gating, argv token exposure, nix hash regressions, uv-venv pip install) by reading the current code and, for the nix hashes, cross-checking the PR's own passing CI runs — no changes needed there. Added tests/test_onepassword_secrets.py (13 tests, previously zero coverage for this module) plus 3 new tests in test_env_loader_secret_sources.py covering the local-override and removal-reconciliation fixes.
…_ACCOUNT_TOKEN Add _DANGEROUS_ENV_VARS blocklist to agent/secret_sources/onepassword.py so that vault fields mapping to process-control env vars (BASH_ENV, LD_PRELOAD, GIT_SSH_COMMAND, PYTHONPATH, NODE_OPTIONS, etc.) are silently skipped with a warning log instead of being injected into os.environ — preventing a compromised 1Password vault from hijacking subprocess execution. The warning logs only the env var name, never the field value. Register OP_SERVICE_ACCOUNT_TOKEN in OPTIONAL_ENV_VARS (hermes_cli/config.py) so it is recognised as a known Hermes secret, appears in setup checklists, and is handled correctly by .env sanitisation and reload_env(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr
…fetch errors Follow-up to the concurrent process-control-blocklist fix (aa74dd6) and the ty-diagnostic findings from CI: - Merge the broader blocklist entries (PATH, IFS, PROMPT_COMMAND, SSH_ASKPASS, GIT_PAGER/EDITOR/CONFIG, LD_AUDIT, DYLD_*, PYTHONHOME, ZDOTDIR, SHELLOPTS, PS4, BASH_FUNC_* prefix) into the single _DANGEROUS_ENV_VARS blocklist rather than leaving a duplicate, narrower blocklist implementation from a parallel fix pass. - Redact the removed-secret-names list from the "removed N secrets" log line in env_loader.py — same count-only convention already used for the Bitwarden branch above it, since CodeQL's clear-text-logging taint tracking doesn't distinguish env var *names* from *values* once either has touched the secrets pipeline (new CodeQL high-severity alert on this line after the previous push). - cmd_op_setup / cmd_op_sync now catch RuntimeError specifically (not bare Exception) and print the exception in full: fetch_onepassword_secrets() only ever raises RuntimeError, and every message it raises is already safe to display verbatim (either our own crafted text or a redacted "1Password SDK error: <TypeName>") — so users now see why a fetch failed instead of just "RuntimeError". - Fix two `ty` type-checker findings introduced by the ambiguous-match rewrite: type item overviews as Any instead of object (object has no .id, which ty correctly flagged), and cast() the ThreadPoolExecutor future's result instead of leaving it for ty to infer through pool.submit(asyncio.run, coro)'s nested generics, which it can't resolve (a known type-checker limitation, not a real type error) and reported as a spurious "not-iterable" warning. Verified: ruff clean, ty shows only the pre-existing expected unresolved-import for the not-installed onepassword-sdk package (same as before this commit — no new diagnostics), 55 targeted tests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a61d9ca67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| field_to_env[label] = env_name | ||
| field_values[label] = value |
There was a problem hiding this comment.
Fresh evidence after the collision fix is that two custom fields with the exact same label are collapsed in field_to_env and field_values before the collision scan runs. For an item containing duplicate labels, the scan therefore sees only one entry and injects whichever value appears last in SDK order, potentially selecting the wrong credential; detect repeated labels while iterating the original field list rather than after storing them in label-keyed dictionaries.
Useful? React with 👍 / 👎.
| token_env = config.get("service_account_token_env", "OP_SERVICE_ACCOUNT_TOKEN") | ||
| vault_name = config.get("vault", "") | ||
| item_title = config.get("item", "") |
There was a problem hiding this comment.
When a user uses the documented ${VAR_NAME} substitution for vault, item, or service_account_token_env, the external-source path supplies this function with the raw values from _load_secrets_config() rather than the expanded result produced by load_config(). The fetch consequently searches for a literal name such as ${OP_VAULT} or looks up a literal token-variable name and fails, even though CLI config consumers see the expanded setting; run this configuration through the canonical expansion path before using it.
AGENTS.md reference: AGENTS.md:L409-L418
Useful? React with 👍 / 👎.
|
|
||
| connection_ok: Optional[bool] = None | ||
| connection_error: Optional[str] = None | ||
| if check_connection and sdk_available and token and item: |
There was a problem hiding this comment.
After a configured integration is disabled, cmd_op_disable() retains its token, vault, and item, so running hermes secrets onepassword status still satisfies this condition and performs a real uncached SDK fetch. This contradicts the new master-switch contract that disabled mode never contacts the SDK and can unexpectedly make a network request or wait for its timeout merely to inspect disabled status; include config.get("enabled") in the connection-check predicate.
Useful? React with 👍 / 👎.
…, gate status on enabled Addresses the Codex review round on commit 4a61d9c: - _load_secrets_config() now runs its result through the same _expand_env_vars() the canonical load_config() path uses, so a documented ${VAR_NAME} reference in secrets.onepassword.{vault,item, service_account_token_env} resolves instead of being passed through to fetch_onepassword_secrets() as a literal, unresolvable string. - _fetch_secrets_async() now derives (label, env_name, value) into a list instead of a label-keyed dict. Two fields sharing the exact same label were previously collapsed into one dict entry before the collision scan ever ran, so the second field's value silently won regardless of SDK ordering; both are now visible to collision detection and dropped together, same as a cross-label collision. - get_onepassword_status()'s live connection check now also requires config.get("enabled"), matching the rest of the integration's master-switch contract — a disabled config can still have a leftover token/vault/item from before it was turned off, and `hermes secrets onepassword status` shouldn't make a network call (or wait out the 30s timeout) just to report that it's disabled. Added regression tests for all three (duplicate-label collision, status skips connection check when disabled vs. runs it when enabled, ${VAR} expansion in _load_secrets_config). Verified: ruff clean, ty shows zero new diagnostics (confirmed by diffing against the pre-edit file), 60 targeted tests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91e2a4d776
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| op_cfg = secrets_cfg.get("onepassword") or {} | ||
| if op_cfg.get("enabled"): |
There was a problem hiding this comment.
When a long-lived gateway has already applied 1Password secrets and enabled is later changed to false, this branch simply skips the integration without clearing its entries from _SECRET_SOURCES, _SECRET_VALUES, or os.environ. The gateway reloads the environment before resolving each turn (gateway/run.py:16204-16213), so it continues using the previously injected credentials until the process is restarted even though the integration is disabled; remove still-managed values or otherwise relinquish them when the source transitions to disabled.
Useful? React with 👍 / 👎.
…abled Codex finding on commit 91e2a4d: a long-lived gateway reloads .env on every turn (gateway/run.py's per-turn _reload_runtime_env_preserving_ config_authority()) but never restarts, so when secrets.onepassword.enabled flips from true to false, the previous code just skipped the whole block — any secret an earlier sync had already injected stayed in os.environ and kept being used indefinitely, not just until the next restart. _apply_external_secret_sources() now has an else branch for the disabled case: any env var still holding the exact value 1Password last set for it gets unset, and all onepassword-labeled entries are cleared from _SECRET_SOURCES/_SECRET_VALUES regardless. An operator's local .env override in the meantime is left untouched — same "value no longer matches what we set" rule already used by the enabled-refresh path. Added a regression test covering both the still-managed-gets-relinquished case and the locally-overridden-gets-left-alone case. Verified: ruff clean, 61 tests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91b4928e0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if op_cfg.get("enabled"): | ||
| try: | ||
| from agent.secret_sources.onepassword import apply_onepassword_secrets |
There was a problem hiding this comment.
When the integration is currently enabled, hermes_cli/main.py:276 calls load_hermes_dotenv() before argparse dispatch, so this branch invokes apply_onepassword_secrets() even for hermes secrets onepassword disable. With the default settings, attempting to disable the source can therefore install the SDK, contact 1Password, and wait through a network timeout before the command gets a chance to flip enabled to false; skip external-source bootstrap for its management/disable path.
Useful? React with 👍 / 👎.
| # (CodeQL py/clear-text-logging-sensitive-data taint path). | ||
| raise RuntimeError(f"1Password SDK error: {type(exc).__name__}") from None | ||
|
|
||
| _CACHE[cache_key] = _CachedFetch(secrets=secrets, fetched_at=time.time()) |
There was a problem hiding this comment.
In a long-lived gateway, rotating the service-account token or changing the vault, item, or field mapping creates a new cache key, but this assignment never removes the previous entry. Each old entry keeps both the prior bootstrap token in its key and the fetched credential values strongly reachable for the process lifetime, so routine rotation retains obsolete secrets and repeated configuration changes grow the cache without bound; prune expired entries or use a bounded cache.
Useful? React with 👍 / 👎.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| _ENV_NAME_RE = re.compile(r"^[A-Z_][A-Z0-9_]*$") |
There was a problem hiding this comment.
Fresh evidence in the replacement validator is that Python's $ anchor matches immediately before a terminal newline, while _is_valid_env_name() uses match() rather than a full match. A custom field_mapping written as a YAML literal block therefore commonly yields a value such as OPENAI_API_KEY\n, which passes validation and is installed as an environment key containing a newline; consumers looking up OPENAI_API_KEY still see it as unset. Use fullmatch() or a strict \Z anchor.
Useful? React with 👍 / 👎.
… commands Addresses the Codex review round on commit 91b4928: - _ENV_NAME_RE now anchors with \Z instead of $. re's $ matches just before a trailing newline, not only end-of-string, so a field_mapping value written as a YAML literal block (commonly ending in "\n") was passing _is_valid_env_name() and getting installed as an env var whose name itself contains a newline — invisible to anything looking up the intended bare name. - fetch_onepassword_secrets() now evicts any other _CACHE entry for the same (vault, item) slot before inserting a new one. Previously a rotated token or changed field_mapping just added a new cache key without removing the old one, so a long-lived gateway going through routine credential rotation accumulated one _CachedFetch — holding a prior bootstrap token and fetched secret values — per rotation, unbounded, for the rest of the process lifetime. - load_hermes_dotenv() gains skip_external_secrets=True, and hermes_cli/main.py's module-level bootstrap call now passes it whenever the invoked subcommand is `secrets` (sniffed from sys.argv before argparse runs, same pattern _apply_profile_override() already uses). Without this, `hermes secrets onepassword disable` had to survive a full bootstrap attempt (SDK auto-install, network fetch, 30s timeout) before the disable command even got a chance to flip enabled to false — so disabling a hanging/misconfigured source could itself hang. Added regression tests for all three. Verified: ruff clean, ty shows zero new diagnostics on any of the three touched files (confirmed by diffing against each pre-edit file, including the large main.py where diffing was necessary to separate signal from ~9200 pre-existing diagnostics), 64 targeted tests pass plus the broader test_hermes_bootstrap.py suite that exercises hermes_cli.main import.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b84f6f3cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Env var names that must never be auto-injected from 1Password vault fields. | ||
| # These variables can be used to hijack subprocess execution via interpreter | ||
| # hooks, dynamic linker preloads, or shell startup files. | ||
| _DANGEROUS_ENV_VARS: frozenset = frozenset({ |
There was a problem hiding this comment.
Fresh evidence after adding this blocklist is that it still permits EDITOR and VISUAL. When a writable 1Password item contains either field and the variable was previously unset, the value is injected into os.environ; hermes config edit then executes that value directly at hermes_cli/config.py:5587-5611 (for example, /bin/rm would be invoked on the config file). Block these variables or allowlist credential-only names rather than relying on an incomplete process-control blacklist.
Useful? React with 👍 / 👎.
| except Exception as exc: # noqa: BLE001 | ||
| console.print(f" [red]✗ Could not install SDK: {exc}[/red]") | ||
| console.print( | ||
| " Manual install: [cyan]pip install onepassword-sdk[/cyan]" |
There was a problem hiding this comment.
When automatic installation fails and the user follows this fallback, bare pip install onepassword-sdk can install a release outside the integration's supported <0.2.0 range, including a breaking pre-1.0 version that the locked environment deliberately excludes. Show the same bounded requirement used by install_onepassword_sdk() so the recovery path does not bypass the repository's dependency policy.
AGENTS.md reference: AGENTS.md:L313-L320
Useful? React with 👍 / 👎.
| connection_error: Optional[str] = None | ||
| if check_connection and config.get("enabled") and sdk_available and token and item: | ||
| try: | ||
| fetch_onepassword_secrets( |
There was a problem hiding this comment.
When a field is skipped because its derived or configured environment name is invalid, startup directs the operator to hermes secrets onepassword status for details, but the new live status fetch discards the returned warnings and the status result contains no warning information. The command therefore reports the connection as OK without identifying which credential was omitted; retain and display the warnings from this fetch.
Useful? React with 👍 / 👎.
| vault_name = (getattr(args, "vault", None) or "").strip() | ||
| if not vault_name: | ||
| vault_name = console.input( | ||
| " Vault name (leave empty to search all accessible vaults): " | ||
| ).strip() |
There was a problem hiding this comment.
When setup runs without a TTY using OP_SERVICE_ACCOUNT_TOKEN and --item, omitting --vault should select the documented search-all-vaults behavior, but this unconditional prompt instead raises EOF and aborts automation; passing --vault "" follows the same branch. In non-interactive mode, preserve an absent vault as the empty search-all value rather than prompting.
Useful? React with 👍 / 👎.
| # Name of the env var that holds the service account token. | ||
| # This is the one bootstrap secret; it lives in ~/.hermes/.env | ||
| # (or your shell) and never in config.yaml. | ||
| "service_account_token_env": "OP_SERVICE_ACCOUNT_TOKEN", |
There was a problem hiding this comment.
Fresh evidence after registering the default token is that service_account_token_env still permits an arbitrary custom name, while the terminal scrubber builds its blocklist only from static OPTIONAL_ENV_VARS entries (tools/environments/local.py:93-99) and passes every unlisted variable into tool subprocesses at lines 290-297. With a configuration such as service_account_token_env: COMPANY_OP_TOKEN, the high-privilege service-account token is therefore exposed to model-issued terminal commands instead of receiving the protection applied to OP_SERVICE_ACCOUNT_TOKEN; dynamically block the configured name or constrain this setting to registered secret variables.
AGENTS.md reference: AGENTS.md:L392-L402
Useful? React with 👍 / 👎.
…tive setup, surface warnings Addresses the Codex review round on commit 4b84f6f (5 findings): - _DANGEROUS_ENV_VARS now includes EDITOR, VISUAL, and PAGER. `hermes config edit` execs $EDITOR/$VISUAL directly as a subprocess command (hermes_cli/config.py's edit_config_interactive) — a 1Password field mapping to either name, previously unset, would get injected and then literally executed the next time someone ran `hermes config edit`. - tools/environments/local.py's terminal env scrubber now also reads secrets.{onepassword,bitwarden}.*_env from config.yaml and adds whatever custom name is configured there to the subprocess blocklist. The static OPTIONAL_ENV_VARS-derived list only ever caught the default OP_SERVICE_ACCOUNT_TOKEN name; a renamed token (service_account_token_env: COMPANY_OP_TOKEN) was a high-privilege secret-manager credential reaching model-issued terminal commands unprotected. - Centralized the pinned SDK requirement into a single OP_SDK_REQUIREMENT constant and replaced every remaining bare `pip install onepassword-sdk` fallback message (the disabled-lazy- install error and the CLI's manual-install-after-failure message) with it, so every recovery path recommends the same bounded range the locked environment actually installs. - cmd_op_setup's vault prompt now checks sys.stdin.isatty() before prompting, same as the token step already does. Non-interactive setup with OP_SERVICE_ACCOUNT_TOKEN + --item but no --vault was hitting an unconditional console.input() and aborting on EOFError instead of falling through to the documented search-all-vaults behavior. - get_onepassword_status() now returns field_warnings from its live connection-check fetch instead of discarding them, and `hermes secrets onepassword status` displays them. Previously a fetch could report "Connection: OK" while silently having dropped a credential whose field label produced an invalid or blocklisted env name — with zero way to see which field via the very status command startup pointed users to for details. Added regression tests for the blocklist additions (including the dynamic custom-token-name blocklist in a new TestConfiguredSecretTokenNamesAreBlocked test class) and the field_warnings plumbing. Verified: ruff clean, ty shows zero new diagnostics across all three touched files, 88 targeted tests pass.
…ging sinks CodeQL flagged 2 new high-severity clear-text-logging-of-sensitive- information alerts on commit 3756126: 1. hermes_cli/secrets_cli.py's cmd_op_status(): the field_warnings display I just added in that commit printed the raw warning text, which embeds the 1Password field label (see _fetch_secrets_async's "Skipping field {label!r}: ..." message) — CodeQL's taint tracking treats that the same as a value, since it originates from the same tainted item.fields source as the actual secrets. 2. hermes_cli/env_loader.py's Bitwarden branch: `for warn in result.warnings: print(f"...{warn}")` embeds the Bitwarden secret *key* (agent/secret_sources/bitwarden.py's "Skipping secret {key!r}" message). This loop predates this PR's onepassword work entirely — it was never actually fixed despite an identical CodeQL alert on it early in this PR's history, which just went stale (is_resolved/is_outdated) as unrelated edits shifted its line position, and resurfaced as "new" now that env_loader.py changed again in this session. Both now report a count and a generic remediation pointer instead of the per-item text, matching the count-only convention already used elsewhere in both these functions (e.g. the "applied N secrets" messages). The underlying field_warnings list returned by get_onepassword_status() is unchanged — only the CLI's interactive display is redacted; get_onepassword_status()'s own docstring/tests still validate the full list for programmatic consumers. Verified: ruff clean, ty shows zero new diagnostics on both files, 67 targeted tests pass unchanged (no test asserted the removed per-item print output).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f0668ccb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # credential rotation takes effect without requiring override_existing. | ||
| if previously_managed is None or key not in previously_managed: | ||
| continue | ||
| os.environ[key] = value |
There was a problem hiding this comment.
When the selected item contains a valid but unregistered name such as DATABASE_PASSWORD or CUSTOM_PRIVATE_KEY, this assignment adds the secret to the process environment, but the local terminal path at tools/environments/local.py:310-317 removes only names in the static provider/tool blocklist. Model-issued terminal and background-process commands therefore inherit and can disclose these 1Password values, unlike recognized API keys. Track every 1Password-managed name in subprocess sanitization (while retaining the explicit passthrough mechanism) rather than protecting only statically registered names.
Useful? React with 👍 / 👎.
| os.environ[key] = value | ||
| applied[key] = "***" |
There was a problem hiding this comment.
When any readable 1Password field contains an embedded null byte, this assignment raises ValueError: embedded null byte. If earlier fields were already processed, the outer loader catches the exception only after those values have been partially injected, while source tracking, stale-key removal, and the remaining refresh are skipped. The dotenv path explicitly strips null bytes at hermes_cli/env_loader.py:158-180 to prevent this same failure, so fetched values should likewise be rejected or sanitized before starting the environment update.
Useful? React with 👍 / 👎.
…ynamic secrets from subprocesses Two threads converged on this commit: 1. CodeQL still flagged 2 new high-severity clear-text-logging alerts on d2f0668 despite the previous redaction pass. Root cause: the ambiguous/not-found RuntimeErrors in _fetch_secrets_async embed data returned by the authenticated 1Password Client itself — other accessible vaults' titles (`[v.title for v in all_vaults]`) and vault/item ids (`[v.id for v in matching_vaults]`, `[ov.id for _, ov in matching_overviews]`) — which CodeQL's taint tracking follows from the Client call through to every place that displays str(exc) (cmd_op_setup, cmd_op_sync, cmd_op_status). Fixed at the source: these errors now report counts only ("not found among N accessible vault(s)", "N vaults share this title") instead of enumerating other vaults'/items' titles or ids. The display sites are unchanged (still show the full — now-safe — message), since with the source fixed there's no longer a usability reason to also redact those, and encoding distinguishable failure categories there would have required a bigger exception-hierarchy change for no added safety. 2. Two more Codex findings on commit d2f0668: - Embedded null bytes in a 1Password field value would hit `ValueError: embedded null byte` on `os.environ[k] = v`, crashing apply_onepassword_secrets() mid-loop with some fields already applied and source-tracking/removal skipped for the rest. _fetch_secrets_async() now strips \x00 the same way _sanitize_env_file_if_needed() already does for the dotenv path, skipping the field entirely if nothing readable remains. - tools/environments/local.py's subprocess env scrubber only knew about statically-registered secret names. A 1Password field with an unregistered label (DATABASE_PASSWORD, CUSTOM_PRIVATE_KEY, ...) has no static registration anywhere, so it reached model-issued terminal/background-process commands unprotected. Added _is_externally_sourced_secret(), which checks env_loader's runtime _SECRET_SOURCES registry (the same one that powers the "(from Bitwarden)" labeling) at call time instead of relying on the fixed blocklist computed once at import — wired into all three subprocess-env-building call sites (_sanitize_subprocess_env x2, _make_run_env), with the explicit passthrough mechanism still taking priority. Added regression tests for all three. Verified: ruff clean, ty shows zero new diagnostics across all three touched files, 97 targeted tests pass.
Three consecutive CodeQL rounds (3756126, d2f0668, ce374fe) all reported the identical "2 new alerts including 2 high severity" — strong evidence the taint tracking here is structural (any string derived from an exception raised within a code path that touched the authenticated 1Password Client), not content-aware. Redacting the *message text* of those RuntimeErrors (removing vault titles/item ids in ce374fe) didn't change the fact that `str(exc)` still constitutes a data-flow edge from the Client to a print/log sink in CodeQL's model. The one pattern that has never been flagged across every round is `type(exc).__name__` — but every fetch failure raised a bare RuntimeError, so that alone would have been useless (every category indistinguishable). Fixed properly instead of just obscuring further: onepassword.py now defines a distinct RuntimeError subclass per failure kind (VaultNotFoundError, VaultAmbiguousError, NoVaultsAccessibleError, ItemNotFoundError, ItemAmbiguousError, EmptyTokenError, FetchTimeoutError, SDKCallError, OnePasswordSDKNotInstalledError) so the *class name* itself is a meaningful, safe category — no string interpolation of anything SDK-derived required. get_onepassword_status()'s connection_error and every remaining `console.print(f"...{exc}...")` site in cmd_op_setup/cmd_op_sync now use type(exc).__name__ instead of the exception text; the connection-status table/message in cmd_op_status needed no code change since they already just display status['connection_error']. Existing `except RuntimeError` handling elsewhere is unaffected (all subclasses still satisfy those catches). Tightened the ambiguous- vault/item tests to assert the specific subclass, and added a test confirming connection_error is the bare class name and never leaks embedded detail (e.g. a vault title from a crafted exception message). Verified: ruff clean, ty shows zero new diagnostics on both touched files, 98 targeted tests pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18af401fdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Name of the env var that holds the service account token. | ||
| # This is the one bootstrap secret; it lives in ~/.hermes/.env | ||
| # (or your shell) and never in config.yaml. | ||
| "service_account_token_env": "OP_SERVICE_ACCOUNT_TOKEN", |
There was a problem hiding this comment.
Fresh evidence after registering the default token is that service_account_token_env still accepts a custom name, while _sanitize_env_lines() builds its recovery set only from static OPTIONAL_ENV_VARS and _EXTRA_ENV_KEYS. With service_account_token_env: COMPANY_OP_TOKEN, a damaged line such as COMPANY_OP_TOKEN=ops_...OPENAI_API_KEY=... is not split, so the concatenated value is used for 1Password authentication and the following credential is lost. Add the configured name dynamically or constrain this setting to registered secret variables.
AGENTS.md reference: AGENTS.md:L392-L402
Useful? React with 👍 / 👎.
| stale_keys = [ | ||
| k for k in _CACHE | ||
| if k[0] == vault_name and k[1] == item_title and k != cache_key | ||
| ] |
There was a problem hiding this comment.
Fresh evidence after the cache-eviction fix is that this predicate only removes entries for the same (vault_name, item_title). In a long-lived gateway, changing either configured value creates a new slot and leaves the old bootstrap token and fetched secrets reachable indefinitely; repeated configuration changes therefore still grow _CACHE without bound. Evict obsolete identities across slots, expired entries, or otherwise bound the cache.
Useful? React with 👍 / 👎.
| _argv_subcommand = next((a for a in sys.argv[1:] if not a.startswith("-")), None) | ||
| load_hermes_dotenv( | ||
| project_env=PROJECT_ROOT / ".env", | ||
| skip_external_secrets=(_argv_subcommand == "secrets"), |
There was a problem hiding this comment.
Fresh evidence after adding the management-command bypass is that this scanner treats a global option's value as the subcommand. For example, hermes --model gpt-5 secrets onepassword disable sets _argv_subcommand to gpt-5, so startup still bootstraps the configured source and may install the SDK or wait on the network before reaching disable. Use the existing option-aware positional scanner or otherwise parse past top-level option values.
Useful? React with 👍 / 👎.
| logger.warning( | ||
| "1Password field mapping collision: '%s' and '%s' both map to '%s'; skipping both", | ||
| env_name_sources[env_name], label, env_name, | ||
| ) |
There was a problem hiding this comment.
When two fields collide after normalization or explicit mapping, this warning writes both raw 1Password field titles at WARNING level. Those titles come from the authenticated item and can contain account names or other sensitive metadata; unlike the count-only startup and status messages added elsewhere in this change, they are persisted to the repository's WARNING+ error log. Log only the destination name or a collision count rather than the source titles.
AGENTS.md reference: AGENTS.md:L64-L67
Useful? React with 👍 / 👎.
| "prompt": "1Password service account token", | ||
| "url": "https://developer.1password.com/docs/service-accounts/", | ||
| "password": True, | ||
| "category": "tool", |
There was a problem hiding this comment.
When users load the existing optional-skills/security/1password skill with its recommended service-account method, the skill declares OP_SERVICE_ACCOUNT_TOKEN through setup.collect_secrets and expects op whoami, op read, and op inject terminal calls to inherit it. Categorizing the token as tool adds it to _HERMES_PROVIDER_ENV_BLOCKLIST, and register_env_passthrough() then refuses the skill's request, so those commands become unauthenticated even after the skill successfully captures the token. Keep the default scrub, but adapt the skill to an explicit user-approved passthrough or a main-process 1Password operation.
Useful? React with 👍 / 👎.
| } | ||
| }, |
There was a problem hiding this comment.
When a contributor opens this trusted checkout with Claude Code, this repository setting starts npx -y @modelcontextprotocol/server-memory@0.6.3; the locally inspected npx --help describes this as running a command from a local or remote npm package, and -y suppresses the installation prompt. This unrelated change therefore downloads and executes an npm dependency graph outside the repository lockfile merely as part of loading the development environment. Remove it from shared settings or replace it with an explicitly provisioned, locked, opt-in server.
Useful? React with 👍 / 👎.
Resolves the unresolved chatgpt-codex-connector findings from PR #106's review rounds that weren't already covered by earlier commits on this branch: - Cache 1Password fetches by (vault, item, token, field_mapping) instead of just (vault, item), so a rotated service account token or changed field mapping can never serve stale secrets fetched under a different identity for the rest of the TTL. - Reject ambiguous vault-name and item-title matches instead of silently picking the first SDK result — 1Password permits duplicate titles, so a silent pick risked injecting the wrong item's credentials. - Derive env var names from ASCII only; non-ASCII field labels (e.g. "clé api") no longer produce non-portable Unicode-derived names. - `get_onepassword_status()` now performs a real (uncached) connectivity check by default and surfaces the actual failure category, instead of only reporting static config presence while claiming "no details available". - `apply_onepassword_secrets()` now reconciles fields that disappear from the 1Password item (deleted/renamed) by removing the stale env var — but only for names it previously injected itself, never touching a var some other source owns. Returns `(applied, removed)`. - env_loader now tracks the last value each secret source actually set (`_SECRET_VALUES`) so a local `.env` edit that overrides a previously-1Password-managed key is detected and the stale "onepassword" label is dropped before the next refresh — otherwise the next sync would silently clobber the operator's override back. - Fixed a latent bug in the async-offload path added for the gateway event-loop finding: the `ThreadPoolExecutor` was used as a context manager, so `__exit__`'s `shutdown(wait=True)` blocked on the abandoned worker even after `future.result(timeout=...)` raised `TimeoutError` — defeating the timeout. Now shuts down with `wait=False` on both the timeout and success paths. - `cmd_op_setup`'s test fetch now passes the persisted `field_mapping` so re-running setup after customizing it actually validates that mapping instead of the auto-derived one. - Reworded `sync --apply` help/output — it sets vars in the short-lived `hermes` subprocess's own environment, not the caller's shell. Verified the fixes already landed earlier on this branch (asyncio.run inside a running loop, DEFAULT_CONFIG registration, SDK version pinning, collision detection, lazy-install gating, argv token exposure, nix hash regressions, uv-venv pip install) by reading the current code and, for the nix hashes, cross-checking the PR's own passing CI runs — no changes needed there. Added tests/test_onepassword_secrets.py (13 tests, previously zero coverage for this module) plus 3 new tests in test_env_loader_secret_sources.py covering the local-override and removal-reconciliation fixes.
…l bootstrap (#127) * Add 1Password Secrets Manager integration following Bitwarden pattern Implements `hermes secrets onepassword` subcommands (setup, status, sync, disable, install) backed by the `onepassword-sdk` Python package. Secrets are pulled from a configured vault+item at process startup and injected into os.environ, with in-process caching and graceful failure on any error. Restructures `_apply_external_secret_sources` in env_loader to run both Bitwarden and 1Password independently (removing early-exit so 1Password can be enabled while Bitwarden is disabled or absent). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: resolve type checker warnings for optional onepassword-sdk import Add `# type: ignore[import-not-found]` to the two lazy `import onepassword` and `from onepassword.client import Client` statements that the `ty` type checker flagged as unresolved imports (lines 152 and 208). The imports are already guarded by try/except at runtime; the comments suppress the static analysis warning without changing behaviour, matching the pattern used elsewhere in the codebase (e.g. agent/google_oauth.py). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: resolve CodeQL security alerts - redact secrets from logs, use stronger hash for cache key - agent/secret_sources/onepassword.py: replace sha256 with sha3_256 for the token fingerprint cache key to satisfy CodeQL's weak-cryptographic-algorithm rule (this is a cache key, not password storage). - agent/secret_sources/onepassword.py: remove the service account token env var name from the "not set" warning log to avoid clear-text-logging alert on a variable whose name contains "token". - agent/secret_sources/onepassword.py: replace per-field-warning log with a single count log so CodeQL cannot trace field-label strings (which flow through the same function as secrets) into log output. - hermes_cli/env_loader.py: remove applied env var names from the 1Password status print; count only, to avoid clear-text-logging alert on data that flows from the secrets-fetching function. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: remove token from cache key to resolve CodeQL hash-on-sensitive-data alert * fix: restore bitwarden early-return structure to avoid CodeQL reanalysis, redact exception from 1password error log - Bitwarden block changed from `if bw_cfg.get("enabled"):` guard back to `if not bw_cfg.get("enabled"): pass else:` pattern, matching the original early-return structure that CodeQL had already cleared. - 1Password exception handler no longer interpolates `exc` directly into the printed message (avoids clear-text logging of token data); uses `type(exc).__name__` instead and routes full detail to logger.warning with exc_info=True. - Added `import logging` and module-level `logger` to support the above. * fix: redact secret names and error details from Bitwarden status prints * fix: resolve final CodeQL alert - redact exception message from 1Password warning log Replace `logger.warning("... %s", exc)` with `logger.warning("... %s", type(exc).__name__, exc_info=True)` in apply_onepassword_secrets. CodeQL's py/clear-text-logging-sensitive-data rule traces: token (os.environ.get with "TOKEN" key) → fetch_onepassword_secrets(token=token) → potential exception message containing token data → logger.warning("%s", exc). Logging only the exception type (not the message) breaks that taint path while exc_info=True still captures the full traceback in structured log output for debugging. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: eliminate all remaining token taint paths to logging sinks Replace str(exc) interpolation in RuntimeError with type(exc).__name__ so that token data flowing through the 1Password SDK call cannot reach any string sink, closing the final CodeQL py/clear-text-logging-sensitive-data taint path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: suppress exception chains and exc_info to eliminate all CodeQL taint paths * fix: redact exception details in 1Password CLI commands to resolve final CodeQL alert * fix: remove secret key names from debug log to cut last CodeQL taint path * chore: clarify SDK auto-install exception is safe to log in full The exception from install_onepassword_sdk() originates from pip's subprocess, not from any token or secret data. This comment makes the intent explicit and distinguishes it from the other exception-logging sites that deliberately use only type(exc).__name__ to avoid leaking token data. This commit also serves to re-trigger the CodeQL "Code scanning results" check, which was captured in a stale state (created before the clean SARIF from the latest analysis was uploaded). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * ci: retrigger CodeQL after GitHub 503 transient error * fix: do not store 1Password service account token in plaintext .env file * Add official MCP memory server to project settings Adds @modelcontextprotocol/server-memory (Knowledge Graph MCP Server) via npx alongside the existing codebase-memory server. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: update nix lockfile hashes for tui and web packages * ci: retrigger nix build * fix: address Codex review - asyncio event loop, timeouts, config defaults, and UX fixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: revert bitwarden error display to avoid CodeQL taint (use safe message) result.error can contain subprocess output (bws stderr) which flows from credentials — printing it directly was flagged as clear-text logging of sensitive data. Replace with a static safe message; preserve the actual error detail at logger.debug() level for diagnostics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: remove tainted logger.debug to pass CodeQL * fix: correct DEFAULT_CONFIG keys, use sys.executable for pip, detect field collisions, enable secret rotation - Fix 1 (config.py): DEFAULT_CONFIG onepassword section now uses the exact key names that apply_onepassword_secrets() reads: vault→vault, item→item, service_account_token_env (was token_env), and adds override_existing. - Fix 2 (onepassword.py): install_onepassword_sdk() now builds the pip command as [sys.executable, "-m", "pip", ...] so the correct interpreter's pip is always used; adds import sys. - Fix 3 (onepassword.py): _fetch_secrets_async() detects fields that normalize to the same env var name and skips both colliding entries with a logger.warning (counts only, no secret values logged). - Fix 4 (env_loader.py + onepassword.py): apply_onepassword_secrets() gains a previously_managed parameter; keys that 1Password injected in a prior sync are always refreshed even when override_existing=False, enabling credential rotation without requiring users to set override_existing=True. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: honor lazy install gate and remove service account token from argv In install_onepassword_sdk() and apply_onepassword_secrets(), check the repo-wide lazy install gate (tools.lazy_deps._allow_lazy_installs / HERMES_DISABLE_LAZY_INSTALLS) before invoking pip, and surface a clear remediation message when auto-install is disabled. Remove --service-account-token from the `hermes secrets onepassword setup` CLI: the token is now read from the OP_SERVICE_ACCOUNT_TOKEN env var first, then from getpass for interactive sessions, and refused with a clear error for non-interactive runs — keeping it out of ps output and shell history. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * fix: use main branch nix lockfile hashes after rebase * fix: use repo pip install strategy for uv virtualenv compatibility * fix: use correct SDK list_all() methods and narrow pre-1.0 version ceiling - Replace client.vaults.list() with client.vaults.list_all() and client.items.list(vault_id) with client.items.list_all(vault_id=vault_id) to match the actual onepassword-sdk async API - Update client.items.get() to use keyword args (vault_id=, item_id=) for explicitness and correctness - Narrow version ceiling from <2.0.0 to <0.2.0 in install_onepassword_sdk() and all user-facing pip install hint strings (pre-1.0 minor-pin per AGENTS.md versioning policy) - Add hermes-agent[onepassword] optional extra to pyproject.toml with the same >=0.1.0,<0.2.0 bounds Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr * chore: update uv.lock for onepassword-sdk optional dependency * fix: allow explicit 'hermes secrets onepassword install' to bypass lazy-install gate * ci: retrigger CodeQL after GitHub 503 transient error * ci: retrigger nix build * fix: address remaining Codex review findings on 1Password integration Resolves the unresolved chatgpt-codex-connector findings from PR #106's review rounds that weren't already covered by earlier commits on this branch: - Cache 1Password fetches by (vault, item, token, field_mapping) instead of just (vault, item), so a rotated service account token or changed field mapping can never serve stale secrets fetched under a different identity for the rest of the TTL. - Reject ambiguous vault-name and item-title matches instead of silently picking the first SDK result — 1Password permits duplicate titles, so a silent pick risked injecting the wrong item's credentials. - Derive env var names from ASCII only; non-ASCII field labels (e.g. "clé api") no longer produce non-portable Unicode-derived names. - `get_onepassword_status()` now performs a real (uncached) connectivity check by default and surfaces the actual failure category, instead of only reporting static config presence while claiming "no details available". - `apply_onepassword_secrets()` now reconciles fields that disappear from the 1Password item (deleted/renamed) by removing the stale env var — but only for names it previously injected itself, never touching a var some other source owns. Returns `(applied, removed)`. - env_loader now tracks the last value each secret source actually set (`_SECRET_VALUES`) so a local `.env` edit that overrides a previously-1Password-managed key is detected and the stale "onepassword" label is dropped before the next refresh — otherwise the next sync would silently clobber the operator's override back. - Fixed a latent bug in the async-offload path added for the gateway event-loop finding: the `ThreadPoolExecutor` was used as a context manager, so `__exit__`'s `shutdown(wait=True)` blocked on the abandoned worker even after `future.result(timeout=...)` raised `TimeoutError` — defeating the timeout. Now shuts down with `wait=False` on both the timeout and success paths. - `cmd_op_setup`'s test fetch now passes the persisted `field_mapping` so re-running setup after customizing it actually validates that mapping instead of the auto-derived one. - Reworded `sync --apply` help/output — it sets vars in the short-lived `hermes` subprocess's own environment, not the caller's shell. Verified the fixes already landed earlier on this branch (asyncio.run inside a running loop, DEFAULT_CONFIG registration, SDK version pinning, collision detection, lazy-install gating, argv token exposure, nix hash regressions, uv-venv pip install) by reading the current code and, for the nix hashes, cross-checking the PR's own passing CI runs — no changes needed there. Added tests/test_onepassword_secrets.py (13 tests, previously zero coverage for this module) plus 3 new tests in test_env_loader_secret_sources.py covering the local-override and removal-reconciliation fixes. * fix: block dangerous process-control env vars and register OP_SERVICE_ACCOUNT_TOKEN Add _DANGEROUS_ENV_VARS blocklist to agent/secret_sources/onepassword.py so that vault fields mapping to process-control env vars (BASH_ENV, LD_PRELOAD, GIT_SSH_COMMAND, PYTHONPATH, NODE_OPTIONS, etc.) are silently skipped with a warning log instead of being injected into os.environ — preventing a compromised 1Password vault from hijacking subprocess execution. The warning logs only the env var name, never the field value. Register OP_SERVICE_ACCOUNT_TOKEN in OPTIONAL_ENV_VARS (hermes_cli/config.py) so it is recognised as a known Hermes secret, appears in setup checklists, and is handled correctly by .env sanitisation and reload_env(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014quAJiFx19py9nV6NnsnHr --------- Co-authored-by: Claude <noreply@anthropic.com>
* fix(secrets): recover hardening orphaned when PR #106 was closed PR #127 landed #106's tree as it stood at commit aa74dd6 — main's onepassword.py, secrets_cli.py and local.py are byte-identical to that commit. The branch then had 8 further commits, all hardening, and closing #106 dropped every one of them. This replays `git diff aa74dd6..18af401` onto current main, scoped to the feature's own files. Recovered: - local.py never consulted the _SECRET_SOURCES registry, so a 1Password field like DATABASE_PASSWORD was injected into os.environ and flowed straight into model-issued subprocesses. #127 did not touch local.py. - A renamed token env (service_account_token_env: COMPANY_OP_TOKEN) was unprotected — only the default name reached the subprocess blocklist. - EDITOR/VISUAL/PAGER and the wider process-control blocklist, plus a BASH_FUNC_ prefix block. `hermes config edit` execs $EDITOR directly. - Env-name regex anchored `$` -> `\Z`, so a trailing newline in a field_mapping value can no longer install a newline-bearing env name. - Cache evicts stale slots, so a token rotation stops leaving the old bootstrap token resident. - Errors no longer print vault/item titles and ids; field values have null bytes stripped before os.environ assignment. - Two clear-text-logging sinks reduced to counts. - Secrets are relinquished when the source is disabled. - Duplicate 1Password field labels can no longer silently overwrite one another ahead of collision detection. - Nine-subclass exception hierarchy replacing str(exc) display. Deliberately NOT ported — #106 predates #118 and three of its hunks are regressions against today's main: config.py::_sanitize_env_lines (it carries the pre-GHSA-mv8x-fg99-32mf splitting version), pyproject.toml (0.15.0 vs 0.18.0), and main.py's missing --legacy-peer-deps. All three verified intact after the patch. Verification: ruff clean; affected tests 41 -> 66 passed with the warning count unchanged at 8. Positive control on the headline fix — reverting only local.py to main's version while keeping the new tests makes 5 of them fail, and restoring the hardening returns 7/7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012689txgT12g2hjRczcUZi8 * fix: route field-label warnings through the warnings list, not logger.warning CodeQL's clear-text-logging taint tracking treats any attribute of a 1Password item field (title/label) as tainted, the same as .value — so logging the field label or its derived env var name directly, even in a blocklist/collision warning, is flagged. Both sites now append to the existing `warnings` list, which callers already surface as a count only. --------- Co-authored-by: Claude <noreply@anthropic.com>
…#137) (#138) * feat(deploy): persistent gateway hosting + env-passthrough log hardening Re-lands the reviewed-good content of #137 on a branch cut from current main. #137 could not merge: it was mergeable_state dirty, and it sat on claude/slack-session-94aae0 — the branch of closed #106, which #130 said should not be continued. Two things ship here. tools/env_passthrough.py — CodeQL clear-text-logging fixes. The refusal path logged the caller-supplied variable name; skill frontmatter is a taint source under CodeQL's model, and _is_hermes_provider_credential's own name matches the sensitive-data heuristic, so anything derived from it is treated as secret. Replaced with counts and static strings. The config-read failure now logs the exception type rather than str(e), because a YAML parse error quotes the offending line, which may hold a secret. deploy/, docs/DEPLOYMENT.md, website/docs/guides/persistent-hosting.md, Dockerfile, docker-compose.yml, README.md — running the gateway 24/7 with no long-lived credentials on the host. Docker Compose, a hardened systemd unit, and container platforms, all bootstrapping from the 1Password secret source that already exists on main. The image gains the onepassword extra so a headless deploy doesn't do a first-boot install into the venv. Only placeholder tokens (ops_...your-token...) appear anywhere. Dropped from #137: .claude/settings.json, which reverted @modelcontextprotocol/server-memory from 0.6.2 to 0.6.3. That version does not exist on npm — the published line jumps 0.6.2 to 2025.4.25 — so the revert re-breaks the memory MCP server and undoes #134. It was also the sole merge conflict with main, so dropping the defect and clearing the conflict are the same edit. hermes_cli/config.py is not touched, so the GHSA-mv8x-fg99-32mf _sanitize_env_lines regression #130 warned about is not in play. Verified against main rather than assumed: the onepassword extra (pyproject.toml), every secrets.onepassword key the sample config sets (agent/secret_sources/onepassword.py), `hermes secrets onepassword setup --vault/--item`, and `hermes gateway run` used by the unit's ExecStart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy1tjok1XKgpTUK69b8dKT * docs(website): register the persistent-hosting guide in the sidebar website/sidebars.ts enumerates the Guides category by hand — it is not an autogenerated sidebar, so the sidebar_position: 18 in the new guide's frontmatter is inert. Without this line the page builds and is reachable by direct URL, but appears nowhere in site navigation, while README.md and docs/DEPLOYMENT.md both link its published URL. docusaurus.config.ts sets onBrokenLinks: 'warn', so nothing fails — it just quietly isn't there. Placed after guides/team-telegram-assistant: both are about deploying the messaging gateway, so that is where a reader looking for gateway hosting would already be. Not in #137; found while reviewing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy1tjok1XKgpTUK69b8dKT * test(env-passthrough): pin that registration never logs a variable name The clear-text-logging fix in the previous commit had nothing guarding it. Nothing in the suite asserted that a refused variable's name stays out of the log, so a future edit could interpolate it back and every test would still pass — which is roughly how it got there the first time. Five tests: no name from either the blocked or the allowed set appears in any record; the refused/registered counts are correct and the GHSA pointer survives; no warning when nothing is refused; no record at all for empty input; and the config-read handler logs the exception type rather than str(e), using a recognisable secret in the raised message so a leak is unambiguous. Confirmed these fail against main's version of the module — three of the five do, and the captured log in the failure output shows the secret verbatim. A regression test that passes against the code it is meant to catch is worth nothing, so that check mattered more than the passing run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy1tjok1XKgpTUK69b8dKT * docs(system-log): record the #137 salvage Per docs/system-log/README.md. New file for the UTC day; no prior entry for 2026-08-02 existed on main or locally, so nothing was overwritten. Records what was carried, what was dropped and why, what was added beyond #137, what was verified, and — separately — what could not be verified in a container with no Docker daemon and no website node_modules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jy1tjok1XKgpTUK69b8dKT --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
What does this PR do?
Implements 1Password Secrets Manager integration for the Hermes gateway, following the exact same architecture as the existing Bitwarden integration. This enables automated credential bootstrapping in containerized environments where
OP_SERVICE_ACCOUNT_TOKENis available as an environment variable.Related Issue
Fixes #DAN-2195
Type of Change
Changes Made
New file:
agent/secret_sources/onepassword.pyCore 1Password integration module using the
onepassword-sdkPython package:apply_onepassword_secrets(config, home_path)— public entry point that reads config, authenticates via service account token, fetches the configured vault item, maps fields to env vars, and injects them intoos.environget_onepassword_status(config, home_path)— status introspection for CLI displayModified:
hermes_cli/env_loader.pyAdded 1Password block in
_apply_external_secret_sources()after the existing Bitwarden block. Readssecrets.onepasswordfrom~/.hermes/config.yamland callsapply_onepassword_secretsif enabled.Modified:
hermes_cli/secrets_cli.pyAdded
hermes secrets onepasswordsubcommand group with five commands:setup— interactive wizard to configure vault, item, and token env varsync— pull and apply secrets immediatelystatus— show connection and config statusdisable— setenabled: falsein configinstall— install theonepassword-sdkPython packageModified:
hermes_cli/main.pyRegistered
secrets onepassword(alias:op) subparser and dispatch handler.How to Test
hermes secrets onepassword installhermes secrets onepassword setuphermes secrets onepassword synchermes secrets onepassword statusConfiguration
Add to
~/.hermes/config.yaml:Notes
*.1password.comto be reachable from the environment (add to egress allowlist if behind a proxy)onepassword-sdkPython package is used (noopbinary required)agent/secret_sources/bitwarden.py)Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Generated by Claude Code