Skip to content

Commit 8ce1df0

Browse files
committed
fix(embedded): preserve shell environment variables when constructor defaults are empty
The HindsightEmbedded constructor included every parameter in the config dict even when the caller left them at their default of "". Those empty-string values travelled through the daemon-startup env pipeline and overwrote the caller's actual shell environment variables (os.environ) set via export or .env files, so HINDSIGHT_API_LLM_API_KEY (and every other key) was silently ignored unless passed explicitly to the constructor. Root cause: the HINDSIGHT_* propagation loop in _start_daemon used 'value is not None' as its guard, so an empty string was always propagated and overwrote any prior os.environ value. Changes (defence in depth): - embedded.py: _set_if_truthy helper — only add keys with truthy values to the config dict. An empty default no longer poisons the merged env. - daemon_embed_manager.py: guard propagation on 'value' (truthy) instead of 'value is not None', so even if an empty string reaches this loop it is harmlessly skipped. Closes #3253.
1 parent 96bd69c commit 8ce1df0

2 files changed

Lines changed: 35 additions & 30 deletions

File tree

hindsight-all/hindsight/embedded.py

Lines changed: 34 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,21 @@
4545
logger = logging.getLogger(__name__)
4646

4747

48+
def _set_if_truthy(target: dict[str, str]):
49+
"""Return a setter that only writes non-empty values to *target*.
50+
51+
Callers that rely on shell environment variables
52+
(``os.environ``) should not have those silently overridden by a
53+
constructor default of ``\"\"``. See issue #3253.
54+
"""
55+
56+
def _set(key: str, value: str) -> None:
57+
if value:
58+
target[key] = value
59+
60+
return _set
61+
62+
4863
class HindsightEmbedded:
4964
"""
5065
Hindsight client with automatic daemon lifecycle management.
@@ -110,20 +125,20 @@ def __init__(
110125
"""
111126
self.profile = profile
112127

113-
# Build config dict for daemon (matches CLI format)
114-
self.config = {
115-
"HINDSIGHT_API_LLM_PROVIDER": llm_provider,
116-
"HINDSIGHT_API_LLM_API_KEY": llm_api_key,
117-
"HINDSIGHT_API_LLM_MODEL": llm_model,
118-
"HINDSIGHT_API_LOG_LEVEL": log_level,
119-
"HINDSIGHT_EMBED_DAEMON_IDLE_TIMEOUT": str(idle_timeout),
120-
}
121-
122-
if llm_base_url:
123-
self.config["HINDSIGHT_API_LLM_BASE_URL"] = llm_base_url
124-
125-
if database_url:
126-
self.config["HINDSIGHT_EMBED_API_DATABASE_URL"] = database_url
128+
# Build config dict for daemon (matches CLI format).
129+
# Only include keys with actual values so that callers relying on shell
130+
# environment variables (os.environ) aren't silently overridden by empty
131+
# defaults. See issue #3253.
132+
self.config: dict[str, str] = {}
133+
_set_if = _set_if_truthy(self.config)
134+
135+
_set_if("HINDSIGHT_API_LLM_PROVIDER", llm_provider)
136+
_set_if("HINDSIGHT_API_LLM_API_KEY", llm_api_key)
137+
_set_if("HINDSIGHT_API_LLM_MODEL", llm_model)
138+
_set_if("HINDSIGHT_API_LOG_LEVEL", log_level)
139+
_set_if("HINDSIGHT_EMBED_DAEMON_IDLE_TIMEOUT", str(idle_timeout) if idle_timeout else "")
140+
_set_if("HINDSIGHT_API_LLM_BASE_URL", llm_base_url or "")
141+
_set_if("HINDSIGHT_EMBED_API_DATABASE_URL", database_url or "")
127142

128143
self._ui = ui
129144
self._ui_port = ui_port
@@ -175,17 +190,13 @@ def _ensure_started(self):
175190
self._started = False
176191

177192
if self._closed:
178-
raise RuntimeError(
179-
"Cannot use HindsightEmbedded after it has been closed"
180-
)
193+
raise RuntimeError("Cannot use HindsightEmbedded after it has been closed")
181194

182195
# Use embed manager interface for daemon management
183196
logger.info(f"Ensuring daemon is running for profile '{self.profile}'...")
184197
success = self._manager.ensure_running(self.config, self.profile)
185198
if not success:
186-
raise RuntimeError(
187-
f"Failed to start daemon for profile '{self.profile}'"
188-
)
199+
raise RuntimeError(f"Failed to start daemon for profile '{self.profile}'")
189200

190201
# Get daemon URL and create client
191202
daemon_url = self._manager.get_url(self.profile)
@@ -196,9 +207,7 @@ def _ensure_started(self):
196207
# Start UI if requested
197208
if self._ui:
198209
logger.info(f"Starting UI for profile '{self.profile}'...")
199-
ui_started = self._manager.start_ui(
200-
self.profile, self._ui_port, self._ui_hostname
201-
)
210+
ui_started = self._manager.start_ui(self.profile, self._ui_port, self._ui_hostname)
202211
if not ui_started:
203212
logger.warning(f"Failed to start UI for profile '{self.profile}'")
204213

@@ -219,8 +228,7 @@ def _cleanup(self, stop_daemon_on_close: bool = False):
219228
# Mark closed to prevent new operations but skip shared-state
220229
# teardown — the daemon's idle timeout handles the rest.
221230
logger.warning(
222-
"Cleanup lock acquisition timed out for profile '%s'; "
223-
"marking closed, daemon will idle-stop on its own",
231+
"Cleanup lock acquisition timed out for profile '%s'; marking closed, daemon will idle-stop on its own",
224232
self.profile,
225233
)
226234
self._closed = True
@@ -433,10 +441,7 @@ def url(self) -> str:
433441
def is_running(self) -> bool:
434442
"""Check if the client is initialized and the daemon is responsive."""
435443
return (
436-
self._started
437-
and not self._closed
438-
and self._client is not None
439-
and self._manager.is_running(self.profile)
444+
self._started and not self._closed and self._client is not None and self._manager.is_running(self.profile)
440445
)
441446

442447
@property

hindsight-embed/hindsight_embed/daemon_embed_manager.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,7 @@ def _start_daemon_locked(
540540
# HINDSIGHT_API_EMBEDDINGS_PROVIDER) are silently dropped because the
541541
# whitelist above only covers LLM/log/idle_timeout keys.
542542
for key, value in config.items():
543-
if key.startswith("HINDSIGHT_") and value is not None:
543+
if key.startswith("HINDSIGHT_") and value:
544544
env[key] = str(value)
545545

546546
# Use profile-specific database (check config for override)

0 commit comments

Comments
 (0)