fix(hindsight): preserve existing keys on blank local_embedded setup

[alt-glitch]

Summary

• Preserve the existing HINDSIGHT_LLM_API_KEY when the local_embedded setup wizard is re-run and the API key prompt is left blank
• Read the current key from env var, $HERMES_HOME/.env, or embedded profile env before prompting, so blank input intentionally keeps it
• Preserve custom HINDSIGHT_TIMEOUT on reconfig instead of silently resetting to default (120)
• Harden save_config against malformed (non-dict) config.json

Root Cause

In post_setup(), env_writes["HINDSIGHT_LLM_API_KEY"] = llm_key was unconditional — blank input wrote an empty string to .env, destroying the existing key. The .env was rewritten before the profile env materialization step could use it as a fallback.

For timeout: self._config is None during post_setup(), so the existing timeout from .env was never read — it always fell back to _DEFAULT_TIMEOUT (120).

Attribution

Salvaged from PR #15309 (@poruru-code) and PR #15233 (@LeonSGP43). Key preservation logic and config hardening from #15309, combined with masked-key prompt UX from #15233. Both contributors' work is preserved — the first commit retains @poruru-code's authorship.

Test plan

• Existing test_local_embedded_setup_preserves_existing_key_when_input_left_blank now passes (was failing on main)
• New: non-default profile env fallback preserves key
• New: malformed config.json (JSON array) doesn't crash setup
• New: custom timeout (300) survives reconfig
• Full hindsight test suite: 78 passed

[alt-glitch]

@BugBot review

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 8877688. Configure here.}

[alt-glitch]

Supersedes #15309 (closed) and #15233 (open) — salvage PR combining both contributors' work with additional fixes for timeout preservation and malformed config.json. Related: #15465, #14163, #14011.

[nicoloboschi]

Already addressed in main via 64a497bf fix(hindsight): preserve setup config on blank input. Could you close?

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the blank-input Hindsight setup regression: #15309 preserves an existing API key and hardens saved-config handling, while #15503 incorporates that work, adds masked-key UX, checks additional key sources, and preserves a custom timeout. Both target the unconditional blank-key write that erased existing configuration, but #15503 also contains distinct timeout and configuration-hardening changes.

Related pull requests

• #15309 [closed] duplicate — (+81/-3) — duplicate and salvaged: The diff fixes the reported root cause by retaining the key from $HERMES_HOME/.env or the saved profile env and adds malformed-config and non-default-profile coverage. It remains relevant although closed because its implementation, tests, and authorship were salvaged into #15503; the author closed it as overlapping #15233.
• #15503 related — (+116/-6) — superseding salvage with residual deltas: The diff combines #15309's key-preservation and config-hardening work with masked-key UX, environment-variable lookup, and custom-timeout preservation. The COMMENTED Bugbot review found no new issues at commit 8877688b341070e58bc467567756e6e71106eb25, but contributor nicoloboschi subsequently reported that the underlying setup-preservation issue was addressed on main by 64a497bf fix(hindsight): preserve setup config on blank input; the remaining timeout and malformed-config deltas therefore need comparison against current main.

Duplicates

#15309 and #15503 substantially duplicate the same blank-input key-preservation fix; #15503 explicitly supersedes and salvages #15309, which is already closed.

Suggested consolidation

Author action: rebase onto main, or split out the part that can merge. Specifically, compare #15503 against 64a497bf fix(hindsight): preserve setup config on blank input, cited in the #15503 discussion by contributor nicoloboschi as the main implementation; if the key-preservation path is already covered, retain only demonstrably missing timeout-preservation or malformed-config hardening as a separate change and close #15503 in its current overlapping form. #15309 requires no further action because it is already closed and its relevant work was salvaged into #15503.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup15309 ["PRs duplicating each other"]
        P15309["PR #15309 (closed)"]
        P15503["PR #15503 (open)"]
    end
    class P15309 closed
    class P15503 open
    class P15503 target
    click P15309 "https://github.com/NousResearch/hermes-agent/pull/15309"
    click P15503 "https://github.com/NousResearch/hermes-agent/pull/15503"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 15 kB of PR diffs, 5 kB of issue/PR text, 3 kB of discussion (8 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[alt-glitch]

Reviewed with a call-graph-assisted pass (calldiff over plugins/memory/hindsight/) plus the full diff. The change reworks the local_embedded setup flow to preserve an existing LLM key and timeout across reconfiguration.

✅ Looks good

• The core fix is sound: setup now discovers an existing key from three fallbacks (env → ~/.hermes/.env → the embedded profile .env) and, when the user leaves the prompt blank, reuses it instead of writing an empty HINDSIGHT_LLM_API_KEY. This directly fixes the "reconfig wipes the key and the daemon fails to start" symptom.
• The masked prompt (current: ...abcd, blank to keep) is good UX and doesn't echo the secret.
• Hardening _save_config's JSON read with isinstance(parsed, dict) (line ~481) is a genuine correctness fix — a malformed config.json holding a list/scalar would previously blow up existing.update(values).
• Tests cover the key-preservation paths and the timeout-preservation path (test_local_embedded_setup_preserves_existing_timeout), asserting .env contents as contracts rather than snapshots.

🔴 Potential bug

• plugins/memory/hindsight/__init__.py:~632 — timeout_val = int(existing_timeout) .... existing_timeout can now come from .env (HINDSIGHT_TIMEOUT), which is an arbitrary string. If a user has a non-numeric or accidentally-quoted value there ("300", 5m, empty-after-strip-but-truthy), int() raises ValueError and aborts setup — a regression from the old code path, which only read from self._config (already typed). Suggest wrapping in try/except (TypeError, ValueError) and falling back to _DEFAULT_TIMEOUT. The existing tests only exercise a clean numeric value, so this path is uncovered — add a "garbage HINDSIGHT_TIMEOUT falls back to default" test.

🟡 Duplication / abstraction

• The "read key from env → hermes .env → embedded profile .env" cascade appears twice (the setup prompt block ~594-612 and the local_embedded materialize block ~668-675), each re-doing the JSON config.json merge + _embedded_profile_env_path lookup. calldiff shows the two subtrees are near-identical. Consider a small _resolve_existing_llm_key(hermes_home, config) helper so the two sites can't drift (note the profile-env lookup key is HINDSIGHT_API_LLM_API_KEY, which is easy to typo).

Verdict

Request changes (minor): the int() coercion on a .env-sourced string is a real crash risk. Otherwise a solid, well-tested fix.