Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion libs/code/deepagents_code/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -14044,8 +14044,13 @@ async def _enter_service_api_key(
)
if result == AuthResult.SAVED:
self._notice_registry.remove(entry.key)
# The modal's own success toast already confirms the save and names
# the provider. This path can't activate the key in-session (unlike
# the Tavily flow, which calls `apply_stored_service_credentials`),
# so surface only the restart hint the modal can't — repeating the
# "saved" confirmation here would just stack a duplicate toast.
self.notify(
f"Saved {service} API key. Restart to apply.",
"Restart to apply your new key.",
severity="information",
timeout=6,
markup=False,
Expand Down
47 changes: 41 additions & 6 deletions libs/code/deepagents_code/auth_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,39 @@ class WriteOutcome:
"""User-visible warning strings (e.g., chmod failures). Empty on success."""


@dataclass(frozen=True, slots=True)
class DeleteOutcome:
"""Result of a credential delete that may have warnings to surface.

A delete rewrites the whole store, so it can hit the same chmod failures as
a write. `warnings` carries them symmetrically with `WriteOutcome` so a
caller's "removed" confirmation doesn't paper over a store the delete
failed to lock down to owner-only.
"""

removed: bool
"""`True` if a credential was removed, `False` if none was stored."""

warnings: tuple[str, ...] = field(default_factory=tuple)
"""chmod-failure warnings from the rewrite. Always empty for a no-op delete
(`removed=False`), which performs no write."""

def __post_init__(self) -> None:
"""Reject the one field combination the docstring forbids.

A no-op delete performs no write, so it can raise no chmod warnings.
Enforcing it here makes "no warnings when nothing was removed" a
construction-time guarantee rather than a producer-side convention a
future edit (or a second producer) could quietly break.

Raises:
ValueError: If `warnings` is non-empty while `removed` is `False`.
"""
if self.warnings and not self.removed:
msg = "DeleteOutcome cannot carry warnings when removed=False"
raise ValueError(msg)


def auth_path() -> Path:
"""Return the resolved path to the credential store (`auth.json`).

Expand Down Expand Up @@ -473,14 +506,16 @@ def set_stored_key(
return WriteOutcome(warnings=warnings)


def delete_stored_key(provider: str) -> bool:
def delete_stored_key(provider: str) -> DeleteOutcome:
"""Remove a stored credential for `provider`.

Args:
provider: Provider identifier.

Returns:
`True` if a credential was removed, `False` if none was stored.
A `DeleteOutcome` whose `removed` flag reports whether a credential was
present, and whose `warnings` tuple lists chmod failures from the
rewrite (empty for a no-op delete, which performs no write).

Raises:
RuntimeError: If the credential file is corrupt and cannot be read, or
Expand All @@ -489,16 +524,16 @@ def delete_stored_key(provider: str) -> bool:
""" # noqa: DOC502 - re-raised from `_read_raw`/`_write_raw_or_raise`
data = _read_raw()
if data is None:
return False
return DeleteOutcome(removed=False)
creds = data.get("credentials")
if not isinstance(creds, dict) or provider not in creds:
return False
return DeleteOutcome(removed=False)
del creds[provider]
data["version"] = _STORAGE_VERSION
data["credentials"] = creds
_write_raw_or_raise(data)
warnings = _write_raw_or_raise(data)
logger.debug("Deleted credential for provider %s", provider)
return True
return DeleteOutcome(removed=True, warnings=warnings)


def list_configured_providers() -> list[str]:
Expand Down
11 changes: 7 additions & 4 deletions libs/code/deepagents_code/client/commands/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
reading from stdin so the key never lands in shell history or argv, and it
refuses an interactive TTY (use `--from-env` instead) so an accidental
invocation cannot hang waiting on input.
- `set` routes through `auth_store.set_stored_key`, so chmod warnings from the
same `WriteOutcome` path the TUI uses are surfaced on stderr.
- `set` and `remove` route through `auth_store`, so chmod warnings from the
store rewrite (the same path the TUI uses) are surfaced on stderr.

Help rendering for a bare `auth` invocation is served by `ui.show_auth_help`,
which does not import this module. The heavy `model_config` imports here are
Expand Down Expand Up @@ -515,11 +515,14 @@ def _run_remove(provider: str) -> int:
from deepagents_code import auth_store

try:
removed = auth_store.delete_stored_key(provider)
result = auth_store.delete_stored_key(provider)
except RuntimeError as exc:
print(f"Error: {exc}", file=sys.stderr) # noqa: T201
return 1
if removed:
# Surface chmod warnings on the rewritten store, symmetric with `set`.
for warning in result.warnings:
print(f"Warning: {warning}", file=sys.stderr) # noqa: T201
if result.removed:
print(f"Removed stored credential for {provider}.") # noqa: T201
else:
print(f"No stored credential for {provider}.") # noqa: T201
Expand Down
40 changes: 36 additions & 4 deletions libs/code/deepagents_code/tui/widgets/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -1286,6 +1286,20 @@ def on_input_submitted(self, event: Input.Submitted) -> None:
if self._is_langsmith:
apply_stored_langsmith_auth(replace_project=True)
clear_caches()
if not outcome.warnings:
# Only claim a clean save when the store locked the file down. When
# chmod warnings fired above, they *are* the outcome message — an
# extra "success" toast on top would compete with (and visually
# bury) the one signal that the key isn't secured.
provider_label = provider_display_name(self._provider, self._config)
# `markup=False`: a configured display name can contain markup
# metacharacters (e.g. `[`) that must not be interpreted here. The
# same guard applies to every interpolated toast below.
self.app.notify(
f"Successfully saved key for {provider_label}.",
severity="information",
markup=False,
)
self.dismiss(AuthResult.SAVED)

def action_cancel(self) -> None:
Expand Down Expand Up @@ -1318,23 +1332,41 @@ def _on_delete_confirmed(self, confirmed: bool | None) -> None:
if not confirmed:
return
try:
removed = auth_store.delete_stored_key(self._provider)
result = auth_store.delete_stored_key(self._provider)
except RuntimeError as exc:
logger.warning(
"Failed to delete credential for %s: %s", self._provider, exc
)
self._show_error("Could not delete credential: $exc", exc=str(exc))
return
if not removed:
for warning in result.warnings:
# The rewritten store still holds other providers' secrets, so a
# chmod failure here is the same security regression as on save —
# surface it rather than dropping it on the floor.
self.app.notify(warning, severity="warning", markup=False)
# Toast after `clear_caches` (like the save path) so the confirmation
# reflects fully-settled state rather than firing before the cache is
# invalidated.
clear_caches()
if not result.removed:
# The entry was gone — likely a concurrent delete from another
# app instance. Surface that fact so "delete" UX doesn't lie when
# nothing actually happened on disk.
provider_label = provider_display_name(self._provider, self._config)
self.app.notify(
f"No stored credential for {self._provider} — already removed.",
f"No stored credential for {provider_label} — already removed.",
severity="information",
markup=False,
)
elif not result.warnings:
# Mirror the save path: a silent successful delete gives no
# confirmation, and the toast is suppressed when warnings fired.
provider_label = provider_display_name(self._provider, self._config)
self.app.notify(
f"Successfully removed key for {provider_label}.",
severity="information",
markup=False,
)
clear_caches()
self.dismiss(AuthResult.DELETED)

def _show_error(self, template: str, /, **substitutions: str) -> None:
Expand Down
23 changes: 23 additions & 0 deletions libs/code/tests/unit_tests/client/commands/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,29 @@ def test_remove_absent_is_noop(self, capsys: pytest.CaptureFixture[str]) -> None
assert code == 0
assert "No stored credential for anthropic." in capsys.readouterr().out

def test_remove_surfaces_chmod_warning(
self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]
) -> None:
"""A chmod failure on the delete rewrite is surfaced on stderr, like `set`."""
auth_store.set_stored_key("anthropic", "sk-ant")
original_chmod = Path.chmod

def _deny_chmod(self: Path, mode: int) -> None:
if self.name == "auth.json":
msg = "simulated chmod denial"
raise OSError(msg)
original_chmod(self, mode)

# Deny chmod only for the delete rewrite, not the seeding write above.
monkeypatch.setattr(Path, "chmod", _deny_chmod)
code = run_auth_command(_ns(auth_command="remove", provider="anthropic"))
assert code == 0
captured = capsys.readouterr()
assert auth_store.get_stored_key("anthropic") is None
assert "Warning:" in captured.err
assert "world-readable" in captured.err
assert "Removed stored credential for anthropic." in captured.out

def test_remove_corrupt_store_errors(
self, capsys: pytest.CaptureFixture[str]
) -> None:
Expand Down
7 changes: 5 additions & 2 deletions libs/code/tests/unit_tests/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -14979,9 +14979,12 @@ async def test_enter_api_key_saved_removes_entry_and_notifies(self) -> None:
assert isinstance(screen, AuthPromptScreen)
assert screen._provider == "tavily"
assert screen._env_var == "TAVILY_API_KEY"
# ... and on save the stale notice is gone and the user is told to restart.
# ... and on save the stale notice is gone and the user is told to
# restart. The modal owns the "saved" confirmation now, so this path
# emits only the restart hint — no duplicate "Saved ... API key" toast.
assert app._notice_registry.get("dep:tavily") is None
assert any("Restart to apply." in m for m in messages)
assert any(m == "Restart to apply your new key." for m in messages)
assert not any("Saved" in m and "API key" in m for m in messages)

async def test_enter_api_key_unknown_service_is_a_noop(
self, caplog: pytest.LogCaptureFixture
Expand Down
47 changes: 43 additions & 4 deletions libs/code/tests/unit_tests/test_auth_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,14 +182,22 @@ def test_malformed_base_url_is_dropped_and_logged(
assert any("malformed base_url" in r.getMessage() for r in caplog.records)

def test_delete_returns_true_when_removed(self) -> None:
"""Deleting an existing entry returns `True` and clears the value."""
"""Deleting an existing entry reports `removed=True` and clears the value."""
auth_store.set_stored_key("openai", "k")
assert auth_store.delete_stored_key("openai") is True
assert auth_store.delete_stored_key("openai").removed is True
assert auth_store.get_stored_key("openai") is None

def test_delete_missing_returns_false(self) -> None:
"""Deleting an unknown provider is a no-op."""
assert auth_store.delete_stored_key("anthropic") is False
"""Deleting an unknown provider is a no-op that reports `removed=False`."""
outcome = auth_store.delete_stored_key("anthropic")
assert outcome.removed is False
# A no-op performs no write, so it can carry no chmod warnings.
assert outcome.warnings == ()

def test_delete_outcome_rejects_warnings_without_removal(self) -> None:
"""The type refuses the illegal `removed=False` + warnings combination."""
with pytest.raises(ValueError, match="cannot carry warnings"):
auth_store.DeleteOutcome(removed=False, warnings=("boom",))


@pytest.mark.usefixtures("fake_home")
Expand Down Expand Up @@ -251,6 +259,37 @@ def test_clean_save_returns_no_warnings(self, fake_home: Path) -> None: # noqa:
outcome = auth_store.set_stored_key("anthropic", "k")
assert outcome.warnings == ()

def test_delete_chmod_failure_returned_as_warning(
self,
fake_home: Path, # noqa: ARG002 - fixture activates the temp state dir
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""A chmod that can't lock down the delete rewrite shows up in DeleteOutcome."""
from pathlib import Path as _Path

auth_store.set_stored_key("anthropic", "k")
original_chmod = _Path.chmod

def deny_file_chmod(self: _Path, mode: int) -> None:
if self.name == "auth.json":
msg = "simulated chmod denial"
raise OSError(msg)
original_chmod(self, mode)

# Deny chmod only for the delete rewrite, not the seeding write above.
monkeypatch.setattr(_Path, "chmod", deny_file_chmod)
outcome = auth_store.delete_stored_key("anthropic")
assert outcome.removed is True
assert any("0600" in w for w in outcome.warnings)
assert any("simulated chmod denial" in w for w in outcome.warnings)

def test_clean_delete_returns_no_warnings(self, fake_home: Path) -> None: # noqa: ARG002 - fixture activates the temp state dir
"""A successful delete reports an empty warnings tuple."""
auth_store.set_stored_key("anthropic", "k")
outcome = auth_store.delete_stored_key("anthropic")
assert outcome.removed is True
assert outcome.warnings == ()


@pytest.mark.usefixtures("fake_home")
class TestCorruption:
Expand Down
Loading
Loading