Fix #2245: bug: masked apiKey written to config.yaml is never resolved on daemon restart — - #2246
Open
Memtensor-AI wants to merge 2 commits into
Open
Conversation
Bridge persists config.yaml with API keys masked to __memos_secret__
via maskSecrets() and strips empty secrets from patches via
stripEmptySecrets(), but nothing re-reads the real value back. On
daemon restart, loadConfig() treats the mask as the literal API key,
every LLM call fails auth, and the bridge restart-loops with
lastOkAt: null and skill.crystallize stuck.
Make resolveConfig() (the single choke point for both disk-loaded
and in-memory patched configs) walk SECRET_FIELD_PATHS after
pruneUnknown and before deepMerge:
- ${VAR} references resolve from process.env when the name matches
the allowlist ^[A-Z][A-Z0-9_]*_(API_KEY|TOKEN)$; other names emit
a warning and stay untouched.
- __memos_secret__ / empty apiKey leaves fall back to LLM_API_KEY
(or EMBEDDING_API_KEY for embedding.apiKey), then to
OPENCODE_GO_API_KEY / OPENCODE_ZEN_API_KEY for LLM-class fields
only. Embedding never inherits an LLM provider's key.
- Hub tokens (hub.teamToken, hub.userToken) require an explicit
${VAR} — no path-based env convention.
- Real values pass through unchanged; the caller's raw config
object is never mutated (resolution runs on the pruneUnknown copy).
Read-side only: on-disk write stays masked, so the security posture
of maskSecrets() is preserved.
Adds 10 unit tests under tests/unit/config/resolve-secret-env.test.ts
covering ${VAR} expansion, mask sentinel resolution, empty-string
fallback, per-path env conventions (embedding vs LLM channel
isolation), hub token ${VAR} path, allowlist enforcement,
non-mutation of the raw config, and negative cases (no env → mask
retained; unset ${VAR} → literal preserved; real values untouched).
Fixes MemTensor#2245
Memtensor-AI
added
ai:generated
3 tasks
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2246 ✅ OpenCodeReview: No comments generated. Looks good to me. Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 4 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
Address 4 findings from the open-code-review pass on PR MemTensor#2246: 1. hub.teamToken / hub.userToken are now resolved from the environment when masked with __memos_secret__ or written as empty strings. The previous `if (leaf !== "apiKey") continue` short-circuit silently perpetuated the original bug for hub tokens. 2. Emit a warning when a secret leaf references an env var that is not set (both the explicit ${VAR} form and the mask/empty form). Without this, a user who writes `apiKey: ${MY_API_KEY}` and forgets to export MY_API_KEY sees auth failures with no actionable log line. 3. Restrict the OPENCODE_GO_API_KEY / OPENCODE_ZEN_API_KEY generic fallback to the primary llm.apiKey. Per-component overrides (l3Llm.apiKey, skillEvolver.apiKey) and non-LLM secrets (embedding.apiKey, hub.*Token) must never silently borrow an unrelated provider's key — that causes cross-provider auth failures and unexpected billing when those components are pointed at a different provider than the shared llm settings. 4. Use an explicit `traversalOk` flag when walking SECRET_FIELD_PATHS so a partial traversal cannot leave `cursor` pointing at a shallower valid intermediate node that would then pass the isPlainObject check and cause `leaf` to be looked up on the wrong object. Today every entry is 2 levels deep so the bug is latent, but the flag makes the intent explicit and future-proofs against deeper paths being added. Env var derivation for masked/empty leaves now uses a camel→SNAKE transform on the last two path segments so every SECRET_FIELD_PATHS entry is resolvable by convention: embedding.apiKey → EMBEDDING_API_KEY llm.apiKey → LLM_API_KEY l3Llm.apiKey → L3_LLM_API_KEY skillEvolver.apiKey → SKILL_EVOLVER_API_KEY hub.teamToken → HUB_TEAM_TOKEN hub.userToken → HUB_USER_TOKEN Tests updated to cover the new hub-token resolution, the tightened fallback scope, and both warning cases. All 76 config tests pass; tsc --noEmit clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
✅ Automated Test Results: PASSEDAll tests passed (50/50 executed). memos_local_plugin/unit: 10/10, memos_python_core/changed-repo-python: 40/40. Duration: 10s [advisory, non-gating] AI-generated tests on branch test/auto-gen-152560a35a72794c-20260814065258: 146/149 passed, 3 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Memtensor-AI
added
status:ready
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixed #2245 — masked apiKey never resolved on daemon restart. The bridge persists
config.yamlwith API keys masked to__memos_secret__viamaskSecrets()and strips empty secrets from patches viastripEmptySecrets(), but nothing re-reads the real value back. On restart,loadConfig()treated the mask as the literal API key, every LLM call failed auth, and the bridge restart-looped withlastOkAt: nullandskill.crystallize.failed ... timed out after 120000 ms.Solution (read-side only, on-disk mask preserved):
resolveConfig()inapps/memos-local-plugin/core/config/index.tsnow walksSECRET_FIELD_PATHSafterpruneUnknownand beforedeepMerge, resolving each leaf via a newresolveSecretEnv()helper.${VAR}references expand fromprocess.envwhen the name matches the allowlist `^[A-Z][A-Z0-9_]*_(API_KEY|TOKEN)## Description(non-allowlisted names emit a warning and stay untouched).
__memos_secret__and empty-stringapiKeyleaves fall back toLLM_API_KEY(orEMBEDDING_API_KEYforembedding.apiKey), then toOPENCODE_GO_API_KEY/OPENCODE_ZEN_API_KEYfor LLM-class fields only — embedding never inherits an LLM provider's key. Hub tokens (hub.teamToken,hub.userToken) require an explicit${VAR}. Real values pass through unchanged; the caller's raw config object is never mutated. BecauseresolveConfig()is the single choke point for both disk-loaded and in-memory patched configs, this covers both paths.Tests: added
tests/unit/config/resolve-secret-env.test.tswith 10 unit tests covering${VAR}expansion, mask sentinel resolution, empty-string fallback, embedding vs LLM channel isolation across allSECRET_FIELD_PATHS, hub-token${VAR}path, allowlist enforcement (warning + no expansion for${HOME}), non-mutation of the raw config, and negative cases (no env → mask retained; unset${VAR}→ literal preserved; real values untouched).npx vitest run tests/unit/config→ 5 files / 71 tests passed.npx tsc -p tsconfig.json --noEmitclean.Note: PR #2235 already proposes the same read-side fix; this branch mirrors that approach and is ready to supersede or replace it. Reviewers: @whipser030, @hijzy.
Related Issue (Required): Fixes #2245
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests are pending.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist