diff --git a/libs/code/deepagents_code/app.py b/libs/code/deepagents_code/app.py index 3a17e3a5d83..ee71726bfe0 100644 --- a/libs/code/deepagents_code/app.py +++ b/libs/code/deepagents_code/app.py @@ -9374,13 +9374,42 @@ 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, + ) + await self._mount_message(UserMessage(command)) - await self._mount_message( - AppMessage( + 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 = ( + "Your stored LangSmith credential could not be read; the " + "credential file may be corrupt. Re-add the key via `/auth`." + ) + 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 f85bb5f15dd..bcff9f4725a 100644 --- a/libs/code/deepagents_code/config.py +++ b/libs/code/deepagents_code/config.py @@ -3103,6 +3103,95 @@ def get_langsmith_project_name() -> str | None: ) +@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). + 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`. + + 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. When several + overrides qualify, the first in `_TRACING_API_KEY_ENV_VARS` order is + returned. + + Returns: + A `LangsmithShadowResult`; see its fields for the three outcomes. + """ + from deepagents_code import auth_store + 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): + # 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 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 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: %s. The credential " + "file may be corrupt; re-add the key via /auth.", + exc, + ) + store_unreadable = True + return LangsmithShadowResult(store_unreadable=store_unreadable) + + 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..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,8 +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 "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 + + 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", + 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 - assert "LANGSMITH_API_KEY" not 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 "/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 bab71ccc061..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, @@ -47,6 +48,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 +1970,216 @@ 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.""" + 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.""" + monkeypatch.setenv("LANGSMITH_API_KEY", "lsv2_test") + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() + + 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", "") + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() + + 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() == LangsmithShadowResult( + shadowing_var="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", "") + auth_store.set_stored_key("langsmith", "lsv2_test") + 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") + assert langsmith_key_shadowed_by_empty_override() == LangsmithShadowResult() + + 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") + 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( + self, + fake_state_dir: Path, # noqa: ARG002 + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """With both overrides empty, name the one hiding the only real key. + + 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") + 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( + 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() == LangsmithShadowResult( + shadowing_var="DEEPAGENTS_CODE_LANGSMITH_API_KEY" + ) + + 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. + + `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") + 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. + + `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 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", "") + with ( + patch( + "deepagents_code.auth_store.get_stored_key", + side_effect=RuntimeError("bad json at line 3"), + ), + caplog.at_level(logging.WARNING, logger="deepagents_code.config"), + ): + 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: """Tests for LangSmith trace secret redaction configuration."""