From ccf361541517956256e92f3e18006935e03fdbaf Mon Sep 17 00:00:00 2001 From: Mason Daugherty <61371264+mdrxy@users.noreply.github.com> Date: Thu, 23 Jul 2026 14:47:40 +0000 Subject: [PATCH 1/3] fix(code): flag empty env override shadowing LangSmith key in `/trace` When a LangSmith key is stored via `/auth` (or set canonically) but a present-but-empty `DEEPAGENTS_CODE_LANGSMITH_API_KEY` shadows it, `/trace` reported the generic "not configured, run /auth" message even though a key was stored. Detect that shadow case and name the offending variable so the fix is obvious. Co-authored-by: open-swe[bot] --- libs/code/deepagents_code/app.py | 23 +++++-- libs/code/deepagents_code/config.py | 42 +++++++++++++ libs/code/tests/unit_tests/test_app.py | 30 +++++++++ libs/code/tests/unit_tests/test_config.py | 75 +++++++++++++++++++++++ 4 files changed, 165 insertions(+), 5 deletions(-) diff --git a/libs/code/deepagents_code/app.py b/libs/code/deepagents_code/app.py index fcb06e17f58..1815215853d 100644 --- a/libs/code/deepagents_code/app.py +++ b/libs/code/deepagents_code/app.py @@ -9375,13 +9375,26 @@ async def _handle_trace_command(self, command: str) -> None: ) return if not project_name: + from deepagents_code.config import ( + langsmith_key_shadowed_by_empty_override, + ) + + shadowing_var = await asyncio.to_thread( + langsmith_key_shadowed_by_empty_override + ) await self._mount_message(UserMessage(command)) - await self._mount_message( - AppMessage( + if shadowing_var: + message = ( + f"A LangSmith key is configured, but {shadowing_var} is set " + "to an empty value and is shadowing it, so tracing is off. " + f"Unset {shadowing_var} to use the stored key." + ) + else: + message = ( "LangSmith tracing is not configured. " - "Run `/auth` and select LangSmith to enable tracing.", - ), - ) + "Run `/auth` and select LangSmith to enable tracing." + ) + await self._mount_message(AppMessage(message)) return try: project_url = await asyncio.to_thread( diff --git a/libs/code/deepagents_code/config.py b/libs/code/deepagents_code/config.py index 59ec0163da4..321103872e7 100644 --- a/libs/code/deepagents_code/config.py +++ b/libs/code/deepagents_code/config.py @@ -3097,6 +3097,48 @@ def get_langsmith_project_name() -> str | None: ) +def langsmith_key_shadowed_by_empty_override() -> str | None: + """Return an empty prefixed override that is suppressing a LangSmith key. + + `/trace` shows a generic "not configured" hint whenever no key resolves, but + a common footgun is exporting `DEEPAGENTS_CODE_LANGSMITH_API_KEY=` (empty): + per `resolve_env_var`'s precedence a present-but-empty prefixed variable + shadows both the canonical env key and a `/auth`-stored key, so tracing + silently stays off even though a key is stored. Detecting this lets callers + point at the offending variable instead of sending the user to `/auth`. + + Returns: + The prefixed env var name shadowing the key, or `None` when no empty + override is suppressing an otherwise-available LangSmith key. + """ + from deepagents_code import auth_store + from deepagents_code.model_config import LANGSMITH_SERVICE, resolved_env_var_name + + empty_overrides = [ + resolved + for name in _TRACING_API_KEY_ENV_VARS + if (resolved := resolved_env_var_name(name)) != name + and not os.environ.get(resolved) + ] + if not empty_overrides: + return None + + # Only claim a shadow when a key is actually being suppressed: a canonical + # env key or a `/auth`-stored LangSmith key. Otherwise the empty override is + # shadowing nothing and the generic "not configured" hint is correct. + shadowed = any( + (os.environ.get(name) or "").strip() for name in _TRACING_API_KEY_ENV_VARS + ) + if not shadowed: + try: + shadowed = bool(auth_store.get_stored_key(LANGSMITH_SERVICE)) + except RuntimeError: + shadowed = False + if not shadowed: + return None + return empty_overrides[0] + + def is_langsmith_redaction_enabled() -> bool: """Return whether LangSmith secret redaction is enabled for agent traces.""" from deepagents_code.config_manifest import ( diff --git a/libs/code/tests/unit_tests/test_app.py b/libs/code/tests/unit_tests/test_app.py index 001285ea080..ab68b3f79e6 100644 --- a/libs/code/tests/unit_tests/test_app.py +++ b/libs/code/tests/unit_tests/test_app.py @@ -5477,6 +5477,36 @@ async def test_trace_shows_error_when_not_configured(self) -> None: assert "/auth" in rendered assert "LANGSMITH_API_KEY" not in rendered + async def test_trace_flags_key_shadowed_by_empty_override(self) -> None: + """Should name the empty override rather than send the user to /auth.""" + app = DeepAgentsApp() + async with app.run_test() as pilot: + await pilot.pause() + app._session_state = TextualSessionState() + + with ( + patch.dict( + "os.environ", + { + "DEEPAGENTS_CODE_LANGSMITH_API_KEY": "", + "LANGSMITH_API_KEY": "lsv2_test", + }, + clear=False, + ), + patch( + "deepagents_code.config.get_langsmith_project_name", + return_value=None, + ), + ): + await app._handle_trace_command("/trace") + await pilot.pause() + + app_msgs = app.query(AppMessage) + rendered = "\n".join(str(w._content) for w in app_msgs) + assert "DEEPAGENTS_CODE_LANGSMITH_API_KEY" in rendered + assert "shadowing" in rendered + assert "/auth" not in rendered + async def test_trace_shows_network_error_when_url_fetch_times_out(self) -> None: """Should distinguish a network/timeout failure from a config gap. diff --git a/libs/code/tests/unit_tests/test_config.py b/libs/code/tests/unit_tests/test_config.py index bab71ccc061..aa924f3574e 100644 --- a/libs/code/tests/unit_tests/test_config.py +++ b/libs/code/tests/unit_tests/test_config.py @@ -47,6 +47,7 @@ get_langsmith_project_name, is_http_url, is_langsmith_redaction_enabled, + langsmith_key_shadowed_by_empty_override, newline_shortcut, normalize_langsmith_endpoint, parse_shell_allow_list, @@ -1968,6 +1969,80 @@ def test_agrees_with_config_manifest_resolution(self) -> None: ) +class TestLangsmithKeyShadowedByEmptyOverride: + """Tests for langsmith_key_shadowed_by_empty_override().""" + + @pytest.fixture + def fake_state_dir(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Redirect the credential store into a temp directory.""" + state = tmp_path / ".state" + monkeypatch.setattr("deepagents_code.model_config.DEFAULT_STATE_DIR", state) + return state + + def test_returns_none_without_empty_override( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """No empty prefixed override means nothing is being shadowed.""" + for var in ( + "DEEPAGENTS_CODE_LANGSMITH_API_KEY", + "DEEPAGENTS_CODE_LANGCHAIN_API_KEY", + ): + monkeypatch.delenv(var, raising=False) + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") + assert langsmith_key_shadowed_by_empty_override() is None + + def test_returns_none_when_override_shadows_nothing( + self, + fake_state_dir: Path, # noqa: ARG002 + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """An empty override with no underlying key is not a shadow.""" + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + for var in ("LANGSMITH_API_KEY", "LANGCHAIN_API_KEY"): + monkeypatch.delenv(var, raising=False) + assert langsmith_key_shadowed_by_empty_override() is None + + def test_detects_shadowed_canonical_env_key( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """An empty override shadowing a canonical env key is reported.""" + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") + assert ( + langsmith_key_shadowed_by_empty_override() + == "DEEPAGENTS_CODE_LANGSMITH_API_KEY" + ) + + def test_detects_shadowed_stored_key( + self, + fake_state_dir: Path, # noqa: ARG002 + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """An empty override shadowing a `/auth`-stored key is reported.""" + from deepagents_code import auth_store + + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + for var in ("LANGSMITH_API_KEY", "LANGCHAIN_API_KEY"): + monkeypatch.delenv(var, raising=False) + auth_store.set_stored_key("langsmith", "lsv2_test") + assert ( + langsmith_key_shadowed_by_empty_override() + == "DEEPAGENTS_CODE_LANGSMITH_API_KEY" + ) + + def test_returns_none_when_override_carries_a_value( + self, + fake_state_dir: Path, # noqa: ARG002 + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """A non-empty prefixed override resolves normally, so no shadow.""" + from deepagents_code import auth_store + + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "lsv2_override") + auth_store.set_stored_key("langsmith", "lsv2_stored") + assert langsmith_key_shadowed_by_empty_override() is None + + class TestLangsmithSecretRedaction: """Tests for LangSmith trace secret redaction configuration.""" From f1a27631348540ab2dcf6dd1f999cc04e99a3595 Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Thu, 23 Jul 2026 13:48:45 -0400 Subject: [PATCH 2/3] fix(code): report only empty overrides that suppress LangSmith Match each empty prefixed override to the key it actually hides so `/trace` names the variable that restores a real key. --- libs/code/deepagents_code/config.py | 62 ++++++++------- libs/code/tests/unit_tests/test_config.py | 92 ++++++++++++++++++++++- 2 files changed, 123 insertions(+), 31 deletions(-) diff --git a/libs/code/deepagents_code/config.py b/libs/code/deepagents_code/config.py index 321103872e7..e9e24ef7527 100644 --- a/libs/code/deepagents_code/config.py +++ b/libs/code/deepagents_code/config.py @@ -3104,39 +3104,47 @@ def langsmith_key_shadowed_by_empty_override() -> str | None: a common footgun is exporting `DEEPAGENTS_CODE_LANGSMITH_API_KEY=` (empty): per `resolve_env_var`'s precedence a present-but-empty prefixed variable shadows both the canonical env key and a `/auth`-stored key, so tracing - silently stays off even though a key is stored. Detecting this lets callers - point at the offending variable instead of sending the user to `/auth`. + silently stays off even though a key is available. Detecting this lets + callers point at the offending variable instead of sending the user to + `/auth`. + + Each prefixed override is checked against the specific key it suppresses, so + the returned name is one that, once unset, actually lets a key resolve: its + canonical variant carries a value, or -- for `LANGSMITH_API_KEY`, the var + `/auth` bridges its stored key onto -- a stored key exists. An empty + override that shadows nothing is ignored, since the generic "not configured" + hint is correct there. When several overrides qualify, the first in + `_TRACING_API_KEY_ENV_VARS` order is returned. Returns: - The prefixed env var name shadowing the key, or `None` when no empty - override is suppressing an otherwise-available LangSmith key. + The prefixed env var name whose empty value is suppressing an available + LangSmith key, or `None` when no such override exists. """ from deepagents_code import auth_store from deepagents_code.model_config import LANGSMITH_SERVICE, resolved_env_var_name - empty_overrides = [ - resolved - for name in _TRACING_API_KEY_ENV_VARS - if (resolved := resolved_env_var_name(name)) != name - and not os.environ.get(resolved) - ] - if not empty_overrides: - return None - - # Only claim a shadow when a key is actually being suppressed: a canonical - # env key or a `/auth`-stored LangSmith key. Otherwise the empty override is - # shadowing nothing and the generic "not configured" hint is correct. - shadowed = any( - (os.environ.get(name) or "").strip() for name in _TRACING_API_KEY_ENV_VARS - ) - if not shadowed: - try: - shadowed = bool(auth_store.get_stored_key(LANGSMITH_SERVICE)) - except RuntimeError: - shadowed = False - if not shadowed: - return None - return empty_overrides[0] + for name in _TRACING_API_KEY_ENV_VARS: + resolved = resolved_env_var_name(name) + if resolved == name or os.environ.get(resolved): + # No prefixed override for this key, or the override carries a value: + # either way it is not an empty override suppressing this key. + continue + if (os.environ.get(name) or "").strip(): + # The empty override is hiding a value on the canonical variable. + return resolved + if name == "LANGSMITH_API_KEY": + # `/auth` bridges its stored key onto `LANGSMITH_API_KEY`, so an + # empty override for it also suppresses a stored key. + try: + if auth_store.get_stored_key(LANGSMITH_SERVICE): + return resolved + except RuntimeError: + logger.warning( + "Could not read the stored LangSmith credential while " + "checking for an empty-override shadow; the credential file " + "may be corrupt. Re-add the key via /auth." + ) + return None def is_langsmith_redaction_enabled() -> bool: diff --git a/libs/code/tests/unit_tests/test_config.py b/libs/code/tests/unit_tests/test_config.py index aa924f3574e..62def878008 100644 --- a/libs/code/tests/unit_tests/test_config.py +++ b/libs/code/tests/unit_tests/test_config.py @@ -2031,17 +2031,101 @@ def test_detects_shadowed_stored_key( ) def test_returns_none_when_override_carries_a_value( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A non-empty prefixed override resolves normally, so no shadow.""" + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "lsv2_override") + for var in ("LANGSMITH_API_KEY", "DEEPAGENTS_CODE_LANGCHAIN_API_KEY"): + monkeypatch.delenv(var, raising=False) + assert langsmith_key_shadowed_by_empty_override() is None + + def test_detects_shadowed_langchain_canonical_env_key( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The legacy `LANGCHAIN_API_KEY` override path is reported too.""" + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") + for var in ("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "LANGSMITH_API_KEY"): + monkeypatch.delenv(var, raising=False) + assert ( + langsmith_key_shadowed_by_empty_override() + == "DEEPAGENTS_CODE_LANGCHAIN_API_KEY" + ) + + def test_reports_the_override_that_actually_shadows_the_key( self, fake_state_dir: Path, # noqa: ARG002 monkeypatch: pytest.MonkeyPatch, ) -> None: - """A non-empty prefixed override resolves normally, so no shadow.""" - from deepagents_code import auth_store + """With both overrides empty, name the one hiding the only real key. - monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "lsv2_override") - auth_store.set_stored_key("langsmith", "lsv2_stored") + The `LANGSMITH` override is empty but shadows nothing (no canonical + value, no stored key); only `LANGCHAIN_API_KEY` carries a key, so + unsetting the `LANGCHAIN` override -- not the `LANGSMITH` one -- is what + restores tracing. The hint must name the `LANGCHAIN` override. + """ + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") + monkeypatch.delenv("LANGSMITH_API_KEY", raising=False) + assert ( + langsmith_key_shadowed_by_empty_override() + == "DEEPAGENTS_CODE_LANGCHAIN_API_KEY" + ) + + def test_prefers_langsmith_when_both_overrides_shadow_a_key( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """When both overrides genuinely shadow a key, `LANGSMITH` wins.""" + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") + monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") + assert ( + langsmith_key_shadowed_by_empty_override() + == "DEEPAGENTS_CODE_LANGSMITH_API_KEY" + ) + + def test_ignores_empty_override_that_shadows_nothing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """An empty override is not reported when a key resolves anyway. + + `LANGSMITH_API_KEY` resolves fine; the empty `LANGCHAIN` override hides + no key (no canonical `LANGCHAIN_API_KEY`). Tracing may still be off for + an unrelated reason (e.g. a missing tracing flag), but this override is + not the cause, so the generic hint -- not a false shadow claim -- is + correct. + """ + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") + for var in ("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "LANGCHAIN_API_KEY"): + monkeypatch.delenv(var, raising=False) assert langsmith_key_shadowed_by_empty_override() is None + def test_returns_none_when_stored_key_unreadable( + self, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + ) -> None: + """A corrupt credential store degrades to no-shadow and logs a warning.""" + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + for var in ( + "LANGSMITH_API_KEY", + "LANGCHAIN_API_KEY", + "DEEPAGENTS_CODE_LANGCHAIN_API_KEY", + ): + monkeypatch.delenv(var, raising=False) + with ( + patch( + "deepagents_code.auth_store.get_stored_key", + side_effect=RuntimeError("corrupt"), + ), + caplog.at_level(logging.WARNING, logger="deepagents_code.config"), + ): + assert langsmith_key_shadowed_by_empty_override() is None + assert any("empty-override shadow" in r.getMessage() for r in caplog.records) + class TestLangsmithSecretRedaction: """Tests for LangSmith trace secret redaction configuration.""" From 9e031cdce696be5d28c7e048fd58604842f02e14 Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Thu, 23 Jul 2026 15:07:58 -0400 Subject: [PATCH 3/3] fix(code): make /trace shadowed-key notice honest about tracing state The shadow notice checked only key availability, but /trace's gate needs a key AND a tracing flag, and resolves keys LANGSMITH-before-LANGCHAIN. The message could therefore tell users to unset an override that wouldn't enable tracing, and mislabel an env key as "stored". - Guard the shadow-check call in _handle_trace_command like its sibling: echo the command first, wrap to_thread in try/except so a failing best-effort diagnostic can't crash /trace. - Reword the notice: no promise that tracing turns on (flag caveat), drop the "stored key" wording that's wrong for env-supplied keys. - Only report an override that gates the effective key: early-return when a key already resolves under normal precedence. - Return LangsmithShadowResult distinguishing shadowing_var / store_unreadable / neither; surface a corruption hint instead of the generic one, and log the underlying RuntimeError text. - Attribute stored-key suppression to the skipped /auth bridge in the docstring; harden tests (autouse env clear, remediation assertions, loop-continuation past store errors). --- libs/code/deepagents_code/app.py | 30 +++-- libs/code/deepagents_code/config.py | 81 ++++++++---- libs/code/tests/unit_tests/test_app.py | 99 ++++++++++++--- libs/code/tests/unit_tests/test_config.py | 147 +++++++++++++++------- 4 files changed, 268 insertions(+), 89 deletions(-) diff --git a/libs/code/deepagents_code/app.py b/libs/code/deepagents_code/app.py index 8202e28ed20..ee71726bfe0 100644 --- a/libs/code/deepagents_code/app.py +++ b/libs/code/deepagents_code/app.py @@ -9375,18 +9375,34 @@ async def _handle_trace_command(self, command: str) -> None: return if not project_name: from deepagents_code.config import ( + LangsmithShadowResult, langsmith_key_shadowed_by_empty_override, ) - shadowing_var = await asyncio.to_thread( - langsmith_key_shadowed_by_empty_override - ) await self._mount_message(UserMessage(command)) - if shadowing_var: + try: + shadow = await asyncio.to_thread( + langsmith_key_shadowed_by_empty_override + ) + except Exception: + # A best-effort diagnostic must never take down `/trace`; fall + # back to the generic hint if the shadow check itself fails. + logger.exception( + "Failed to check for a shadowed LangSmith key for thread %s", + thread_id, + ) + shadow = LangsmithShadowResult() + if shadow.shadowing_var: + message = ( + f"A LangSmith key is available, but {shadow.shadowing_var} " + "is set to an empty value and is shadowing it, so tracing is " + f"off. Unset {shadow.shadowing_var} (and make sure LangSmith " + "tracing is enabled) to start tracing." + ) + elif shadow.store_unreadable: message = ( - f"A LangSmith key is configured, but {shadowing_var} is set " - "to an empty value and is shadowing it, so tracing is off. " - f"Unset {shadowing_var} to use the stored key." + "Your stored LangSmith credential could not be read; the " + "credential file may be corrupt. Re-add the key via `/auth`." ) else: message = ( diff --git a/libs/code/deepagents_code/config.py b/libs/code/deepagents_code/config.py index 8e42ee1d298..bcff9f4725a 100644 --- a/libs/code/deepagents_code/config.py +++ b/libs/code/deepagents_code/config.py @@ -3103,32 +3103,66 @@ def get_langsmith_project_name() -> str | None: ) -def langsmith_key_shadowed_by_empty_override() -> str | None: - """Return an empty prefixed override that is suppressing a LangSmith key. +@dataclass(frozen=True) +class LangsmithShadowResult: + """Why `/trace` found no LangSmith key, when an empty override is involved. + + Distinguishes the three states the caller renders differently: a specific + empty override is suppressing an available key (`shadowing_var`), the + credential store could not be read so a stored key can't be ruled out + (`store_unreadable`), or neither (both fields falsy -- the generic "not + configured" hint applies). + """ + + shadowing_var: str | None = None + """Prefixed env var whose empty value is suppressing an available key.""" + + store_unreadable: bool = False + """`True` when the `/auth` credential store raised while being checked.""" + + +def langsmith_key_shadowed_by_empty_override() -> LangsmithShadowResult: + """Report an empty prefixed override that is suppressing a LangSmith key. `/trace` shows a generic "not configured" hint whenever no key resolves, but - a common footgun is exporting `DEEPAGENTS_CODE_LANGSMITH_API_KEY=` (empty): - per `resolve_env_var`'s precedence a present-but-empty prefixed variable - shadows both the canonical env key and a `/auth`-stored key, so tracing - silently stays off even though a key is available. Detecting this lets - callers point at the offending variable instead of sending the user to + a common footgun is exporting `DEEPAGENTS_CODE_LANGSMITH_API_KEY=` (empty). + A present-but-empty prefixed variable suppresses a key two ways: per + `resolve_env_var`'s precedence it shadows the canonical env variable + directly, and -- because `apply_stored_service_credentials` skips the `/auth` + bridge onto `LANGSMITH_API_KEY` whenever the prefixed var is present -- it + also keeps a `/auth`-stored key from ever reaching the environment. Either + way tracing silently stays off even though a key is available. Detecting this + lets callers name the offending variable instead of sending the user to `/auth`. - Each prefixed override is checked against the specific key it suppresses, so + Only an override that actually gates the *effective* key is reported. If a + key already resolves under the normal `LANGSMITH_API_KEY`-before- + `LANGCHAIN_API_KEY` precedence, tracing is off for some other reason (a + missing tracing flag), no override is to blame, and nothing is reported. + Otherwise each override is checked against the specific key it suppresses, so the returned name is one that, once unset, actually lets a key resolve: its canonical variant carries a value, or -- for `LANGSMITH_API_KEY`, the var - `/auth` bridges its stored key onto -- a stored key exists. An empty - override that shadows nothing is ignored, since the generic "not configured" - hint is correct there. When several overrides qualify, the first in - `_TRACING_API_KEY_ENV_VARS` order is returned. + `/auth` bridges its stored key onto -- a stored key exists. When several + overrides qualify, the first in `_TRACING_API_KEY_ENV_VARS` order is + returned. Returns: - The prefixed env var name whose empty value is suppressing an available - LangSmith key, or `None` when no such override exists. + A `LangsmithShadowResult`; see its fields for the three outcomes. """ from deepagents_code import auth_store - from deepagents_code.model_config import LANGSMITH_SERVICE, resolved_env_var_name + from deepagents_code.model_config import ( + LANGSMITH_SERVICE, + resolve_env_var, + resolved_env_var_name, + ) + if resolve_env_var("LANGSMITH_API_KEY") or resolve_env_var("LANGCHAIN_API_KEY"): + # A key already resolves (matching `get_langsmith_project_name`'s key + # precedence), so no empty override is what's keeping tracing off, and + # unsetting one would change nothing. Defer to the generic hint. + return LangsmithShadowResult() + + store_unreadable = False for name in _TRACING_API_KEY_ENV_VARS: resolved = resolved_env_var_name(name) if resolved == name or os.environ.get(resolved): @@ -3137,20 +3171,25 @@ def langsmith_key_shadowed_by_empty_override() -> str | None: continue if (os.environ.get(name) or "").strip(): # The empty override is hiding a value on the canonical variable. - return resolved + return LangsmithShadowResult(shadowing_var=resolved) if name == "LANGSMITH_API_KEY": # `/auth` bridges its stored key onto `LANGSMITH_API_KEY`, so an # empty override for it also suppresses a stored key. try: if auth_store.get_stored_key(LANGSMITH_SERVICE): - return resolved - except RuntimeError: + return LangsmithShadowResult(shadowing_var=resolved) + except RuntimeError as exc: + # Can't confirm a stored key, but keep scanning: a later + # override may still name a concrete shadow. Only if none does + # do we surface the unreadable store to the caller. logger.warning( "Could not read the stored LangSmith credential while " - "checking for an empty-override shadow; the credential file " - "may be corrupt. Re-add the key via /auth." + "checking for an empty-override shadow: %s. The credential " + "file may be corrupt; re-add the key via /auth.", + exc, ) - return None + store_unreadable = True + return LangsmithShadowResult(store_unreadable=store_unreadable) def is_langsmith_redaction_enabled() -> bool: diff --git a/libs/code/tests/unit_tests/test_app.py b/libs/code/tests/unit_tests/test_app.py index ab68b3f79e6..4f91e8bd4af 100644 --- a/libs/code/tests/unit_tests/test_app.py +++ b/libs/code/tests/unit_tests/test_app.py @@ -5460,11 +5460,49 @@ async def test_trace_no_warning_when_message_lookup_fails(self) -> None: async def test_trace_shows_error_when_not_configured(self) -> None: """Should show configuration hint when LangSmith is not set up.""" + from deepagents_code.config import LangsmithShadowResult + app = DeepAgentsApp() async with app.run_test() as pilot: await pilot.pause() app._session_state = TextualSessionState() + with ( + patch( + "deepagents_code.config.get_langsmith_project_name", + return_value=None, + ), + # Pin the shadow check so this branch is independent of whatever + # tracing vars the test runner happens to have exported. + patch( + "deepagents_code.config.langsmith_key_shadowed_by_empty_override", + return_value=LangsmithShadowResult(), + ), + ): + await app._handle_trace_command("/trace") + await pilot.pause() + + app_msgs = app.query(AppMessage) + rendered = "\n".join(str(w._content) for w in app_msgs) + assert "/auth" in rendered + assert "LANGSMITH_API_KEY" not in rendered + + async def test_trace_flags_key_shadowed_by_empty_override( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Should name the empty override and how to fix it, not send to /auth.""" + app = DeepAgentsApp() + async with app.run_test() as pilot: + await pilot.pause() + app._session_state = TextualSessionState() + + # Clear the sibling tracing vars so the real helper reaches the + # canonical-env shadow branch deterministically. + for var in ("LANGCHAIN_API_KEY", "DEEPAGENTS_CODE_LANGCHAIN_API_KEY"): + monkeypatch.delenv(var, raising=False) + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") + with patch( "deepagents_code.config.get_langsmith_project_name", return_value=None, @@ -5474,38 +5512,71 @@ async def test_trace_shows_error_when_not_configured(self) -> None: app_msgs = app.query(AppMessage) rendered = "\n".join(str(w._content) for w in app_msgs) - assert "/auth" in rendered - assert "LANGSMITH_API_KEY" not in rendered + assert "DEEPAGENTS_CODE_LANGSMITH_API_KEY" in rendered + assert "shadowing" in rendered + # The actionable remediation must be present... + assert "Unset DEEPAGENTS_CODE_LANGSMITH_API_KEY" in rendered + # ...and it must not send the user to /auth or mislabel an env key + # as a "stored" key. + assert "/auth" not in rendered + assert "stored key" not in rendered + + async def test_trace_flags_unreadable_credential_store(self) -> None: + """A corrupt store surfaces a corruption hint, not the generic one.""" + from deepagents_code.config import LangsmithShadowResult - async def test_trace_flags_key_shadowed_by_empty_override(self) -> None: - """Should name the empty override rather than send the user to /auth.""" app = DeepAgentsApp() async with app.run_test() as pilot: await pilot.pause() app._session_state = TextualSessionState() with ( - patch.dict( - "os.environ", - { - "DEEPAGENTS_CODE_LANGSMITH_API_KEY": "", - "LANGSMITH_API_KEY": "lsv2_test", - }, - clear=False, + patch( + "deepagents_code.config.get_langsmith_project_name", + return_value=None, ), + patch( + "deepagents_code.config.langsmith_key_shadowed_by_empty_override", + return_value=LangsmithShadowResult(store_unreadable=True), + ), + ): + await app._handle_trace_command("/trace") + await pilot.pause() + + app_msgs = app.query(AppMessage) + rendered = "\n".join(str(w._content) for w in app_msgs) + assert "corrupt" in rendered + assert "/auth" in rendered + + async def test_trace_survives_shadow_check_failure(self) -> None: + """An unexpected error in the shadow check falls back to the generic hint. + + The check is a best-effort diagnostic; a raise from it (e.g. a lazy + import failure) must not crash `/trace` or drop the command echo. + """ + app = DeepAgentsApp() + async with app.run_test() as pilot: + await pilot.pause() + app._session_state = TextualSessionState() + + with ( patch( "deepagents_code.config.get_langsmith_project_name", return_value=None, ), + patch( + "deepagents_code.config.langsmith_key_shadowed_by_empty_override", + side_effect=RuntimeError("boom"), + ), ): await app._handle_trace_command("/trace") await pilot.pause() app_msgs = app.query(AppMessage) rendered = "\n".join(str(w._content) for w in app_msgs) - assert "DEEPAGENTS_CODE_LANGSMITH_API_KEY" in rendered - assert "shadowing" in rendered - assert "/auth" not in rendered + assert "/auth" in rendered + user_msgs = app.query(UserMessage) + assert any("/trace" in str(w._content) for w in user_msgs) async def test_trace_shows_network_error_when_url_fetch_times_out(self) -> None: """Should distinguish a network/timeout failure from a config gap. diff --git a/libs/code/tests/unit_tests/test_config.py b/libs/code/tests/unit_tests/test_config.py index 62def878008..9c584787b32 100644 --- a/libs/code/tests/unit_tests/test_config.py +++ b/libs/code/tests/unit_tests/test_config.py @@ -22,6 +22,7 @@ SHELL_ALLOW_ALL, LangSmithApiError, LangSmithProjectNotFoundError, + LangsmithShadowResult, ModelResult, Settings, _apply_default_langsmith_project, @@ -1972,6 +1973,22 @@ def test_agrees_with_config_manifest_resolution(self) -> None: class TestLangsmithKeyShadowedByEmptyOverride: """Tests for langsmith_key_shadowed_by_empty_override().""" + @pytest.fixture(autouse=True) + def _clear_tracing_env(self, monkeypatch: pytest.MonkeyPatch) -> None: + """Start each test from a clean slate for the four tracing key vars. + + Every case below sets only the vars it cares about; clearing the rest + up front keeps results independent of whatever the test runner happens + to have exported (e.g. a developer's own empty override). + """ + for var in ( + "LANGSMITH_API_KEY", + "LANGCHAIN_API_KEY", + "DEEPAGENTS_CODE_LANGSMITH_API_KEY", + "DEEPAGENTS_CODE_LANGCHAIN_API_KEY", + ): + monkeypatch.delenv(var, raising=False) + @pytest.fixture def fake_state_dir(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: """Redirect the credential store into a temp directory.""" @@ -1983,13 +2000,8 @@ def test_returns_none_without_empty_override( self, monkeypatch: pytest.MonkeyPatch ) -> None: """No empty prefixed override means nothing is being shadowed.""" - for var in ( - "DEEPAGENTS_CODE_LANGSMITH_API_KEY", - "DEEPAGENTS_CODE_LANGCHAIN_API_KEY", - ): - monkeypatch.delenv(var, raising=False) monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") - assert langsmith_key_shadowed_by_empty_override() is None + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() def test_returns_none_when_override_shadows_nothing( self, @@ -1998,9 +2010,7 @@ def test_returns_none_when_override_shadows_nothing( ) -> None: """An empty override with no underlying key is not a shadow.""" monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") - for var in ("LANGSMITH_API_KEY", "LANGCHAIN_API_KEY"): - monkeypatch.delenv(var, raising=False) - assert langsmith_key_shadowed_by_empty_override() is None + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() def test_detects_shadowed_canonical_env_key( self, monkeypatch: pytest.MonkeyPatch @@ -2008,9 +2018,8 @@ def test_detects_shadowed_canonical_env_key( """An empty override shadowing a canonical env key is reported.""" monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") - assert ( - langsmith_key_shadowed_by_empty_override() - == "DEEPAGENTS_CODE_LANGSMITH_API_KEY" + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGSMITH_API_KEY" ) def test_detects_shadowed_stored_key( @@ -2022,22 +2031,36 @@ def test_detects_shadowed_stored_key( from deepagents_code import auth_store monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") - for var in ("LANGSMITH_API_KEY", "LANGCHAIN_API_KEY"): - monkeypatch.delenv(var, raising=False) auth_store.set_stored_key("langsmith", "lsv2_test") - assert ( - langsmith_key_shadowed_by_empty_override() - == "DEEPAGENTS_CODE_LANGSMITH_API_KEY" + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGSMITH_API_KEY" ) + def test_langchain_override_does_not_consult_the_stored_key( + self, + fake_state_dir: Path, # noqa: ARG002 + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The stored-key bridge is `LANGSMITH`-only, so `LANGCHAIN` ignores it. + + `/auth` only bridges its stored key onto `LANGSMITH_API_KEY`, never + `LANGCHAIN_API_KEY`. An empty `LANGCHAIN` override with no canonical + `LANGCHAIN_API_KEY` therefore shadows nothing even when a key is stored, + so no shadow is reported. Pins the asymmetry against a future change that + wrongly makes the `LANGCHAIN` path consult the store. + """ + from deepagents_code import auth_store + + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + auth_store.set_stored_key("langsmith", "lsv2_test") + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() + def test_returns_none_when_override_carries_a_value( self, monkeypatch: pytest.MonkeyPatch ) -> None: """A non-empty prefixed override resolves normally, so no shadow.""" monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "lsv2_override") - for var in ("LANGSMITH_API_KEY", "DEEPAGENTS_CODE_LANGCHAIN_API_KEY"): - monkeypatch.delenv(var, raising=False) - assert langsmith_key_shadowed_by_empty_override() is None + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() def test_detects_shadowed_langchain_canonical_env_key( self, monkeypatch: pytest.MonkeyPatch @@ -2045,11 +2068,8 @@ def test_detects_shadowed_langchain_canonical_env_key( """The legacy `LANGCHAIN_API_KEY` override path is reported too.""" monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") - for var in ("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "LANGSMITH_API_KEY"): - monkeypatch.delenv(var, raising=False) - assert ( - langsmith_key_shadowed_by_empty_override() - == "DEEPAGENTS_CODE_LANGCHAIN_API_KEY" + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGCHAIN_API_KEY" ) def test_reports_the_override_that_actually_shadows_the_key( @@ -2067,10 +2087,8 @@ def test_reports_the_override_that_actually_shadows_the_key( monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") - monkeypatch.delenv("LANGSMITH_API_KEY", raising=False) - assert ( - langsmith_key_shadowed_by_empty_override() - == "DEEPAGENTS_CODE_LANGCHAIN_API_KEY" + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGCHAIN_API_KEY" ) def test_prefers_langsmith_when_both_overrides_shadow_a_key( @@ -2081,12 +2099,11 @@ def test_prefers_langsmith_when_both_overrides_shadow_a_key( monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") - assert ( - langsmith_key_shadowed_by_empty_override() - == "DEEPAGENTS_CODE_LANGSMITH_API_KEY" + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGSMITH_API_KEY" ) - def test_ignores_empty_override_that_shadows_nothing( + def test_ignores_empty_override_when_a_key_already_resolves( self, monkeypatch: pytest.MonkeyPatch ) -> None: """An empty override is not reported when a key resolves anyway. @@ -2099,32 +2116,68 @@ def test_ignores_empty_override_that_shadows_nothing( """ monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") - for var in ("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "LANGCHAIN_API_KEY"): - monkeypatch.delenv(var, raising=False) - assert langsmith_key_shadowed_by_empty_override() is None + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() + + def test_ignores_lower_precedence_override_when_langsmith_key_resolves( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A resolving `LANGSMITH` key silences a genuine `LANGCHAIN` shadow. - def test_returns_none_when_stored_key_unreadable( + `LANGSMITH_API_KEY` resolves and wins under precedence, so the effective + key is present and unsetting the empty `LANGCHAIN` override (which does + shadow `LANGCHAIN_API_KEY`) would change nothing. Reporting it would send + the user to unset the wrong variable, so nothing is reported. + """ + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_langsmith") + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_langchain") + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() + + def test_reports_store_unreadable_when_no_other_shadow_found( self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, ) -> None: - """A corrupt credential store degrades to no-shadow and logs a warning.""" + """A corrupt store with no other shadow surfaces `store_unreadable`. + + The warning must carry the underlying exception text (not a static + guess) so logs point at the real fault. + """ monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") - for var in ( - "LANGSMITH_API_KEY", - "LANGCHAIN_API_KEY", - "DEEPAGENTS_CODE_LANGCHAIN_API_KEY", - ): - monkeypatch.delenv(var, raising=False) with ( patch( "deepagents_code.auth_store.get_stored_key", - side_effect=RuntimeError("corrupt"), + side_effect=RuntimeError("bad json at line 3"), ), caplog.at_level(logging.WARNING, logger="deepagents_code.config"), ): - assert langsmith_key_shadowed_by_empty_override() is None - assert any("empty-override shadow" in r.getMessage() for r in caplog.records) + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + store_unreadable=True + ) + messages = [r.getMessage() for r in caplog.records] + assert any("empty-override shadow" in m for m in messages) + assert any("bad json at line 3" in m for m in messages) + + def test_unreadable_store_does_not_abort_scan_of_later_override( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A store error on `LANGSMITH` must not stop the `LANGCHAIN` check. + + `LANGSMITH`'s stored-key read raises, but an empty `LANGCHAIN` override + genuinely shadows a canonical `LANGCHAIN_API_KEY`. That concrete shadow + is the actionable answer and must win over the store uncertainty, which + proves the loop continued past the exception rather than bailing. + """ + monkeypatch.setenv("DEEPAGENTS_CODE_LANGSMITH_API_KEY", "") + monkeypatch.setenv("DEEPAGENTS_CODE_LANGCHAIN_API_KEY", "") + monkeypatch.setenv("LANGCHAIN_API_KEY", "lsv2_test") + with patch( + "deepagents_code.auth_store.get_stored_key", + side_effect=RuntimeError("corrupt"), + ): + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGCHAIN_API_KEY" + ) class TestLangsmithSecretRedaction: