mirror of
https://github.com/usestrix/strix.git
synced 2026-08-16 09:26:39 +02:00
fix(llm): make logout-all atomic and persist provider in run record
Second review pass: - strix auth logout (all providers) now holds the shared store lock across every provider removal, so a concurrent save/refresh cannot leave one credential behind while reporting all removed. - _persist_run_record writes subscription_provider alongside auth_mode, matching ReportState, so resumed runs keep their original provider label.
This commit is contained in:
@@ -22,7 +22,7 @@ from rich.console import Console
|
||||
from rich.panel import Panel
|
||||
from rich.text import Text
|
||||
|
||||
from strix.config import codex, grok, load_settings
|
||||
from strix.config import codex, grok, load_settings, subscription_store
|
||||
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -346,8 +346,11 @@ def _logout(console: Console, argv: list[str]) -> int:
|
||||
return int(exc.code or 2)
|
||||
|
||||
if args.provider is None:
|
||||
for provider in _PROVIDERS.values():
|
||||
provider.module.logout()
|
||||
# Hold the store lock across every provider so a concurrent save/refresh
|
||||
# can't slip a credential back in between removals (logout-all is atomic).
|
||||
with subscription_store.guard(codex.AUTH_PATH):
|
||||
for provider in _PROVIDERS.values():
|
||||
provider.module.logout()
|
||||
console.print("[green]Signed out.[/] Stored subscription credentials removed.")
|
||||
return 0
|
||||
|
||||
|
||||
@@ -790,13 +790,15 @@ Examples:
|
||||
def _persist_run_record(args: argparse.Namespace) -> None:
|
||||
run_dir = run_dir_for(args.run_name)
|
||||
run_dir.mkdir(parents=True, exist_ok=True)
|
||||
model = load_settings().llm.model
|
||||
run_record = {
|
||||
"run_id": args.run_name,
|
||||
"run_name": args.run_name,
|
||||
"status": "running",
|
||||
"start_time": datetime.now(UTC).isoformat(),
|
||||
"end_time": None,
|
||||
"auth_mode": subscription.auth_mode(load_settings().llm.model),
|
||||
"auth_mode": subscription.auth_mode(model),
|
||||
"subscription_provider": subscription.provider_label(model),
|
||||
"targets_info": args.targets_info,
|
||||
"scan_mode": args.scan_mode,
|
||||
"instruction": args.instruction,
|
||||
|
||||
@@ -95,6 +95,31 @@ def test_model_subcommand_removed() -> None:
|
||||
assert auth_cli.run_auth(["model", "gpt-5.5"]) == 2
|
||||
|
||||
|
||||
def _sign_in_both() -> None:
|
||||
codex.save_record({"type": "oauth", "access": "c", "refresh": "r", "account_id": "a"})
|
||||
grok.save_record({"type": "oauth", "access": "g", "refresh": "r"})
|
||||
|
||||
|
||||
def test_logout_all_removes_every_provider() -> None:
|
||||
_sign_in_both()
|
||||
assert codex.is_authenticated()
|
||||
assert grok.is_authenticated()
|
||||
|
||||
assert auth_cli.run_auth(["logout"]) == 0
|
||||
|
||||
assert not codex.is_authenticated()
|
||||
assert not grok.is_authenticated()
|
||||
|
||||
|
||||
def test_logout_single_provider_leaves_the_other() -> None:
|
||||
_sign_in_both()
|
||||
|
||||
assert auth_cli.run_auth(["logout", "grok"]) == 0
|
||||
|
||||
assert codex.is_authenticated()
|
||||
assert not grok.is_authenticated()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("provider", ["chatgpt", "codex", "ChatGPT"])
|
||||
def test_login_accepts_provider_aliases(provider: str, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
reached = {"flow": False}
|
||||
|
||||
@@ -2,6 +2,8 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import importlib
|
||||
from unittest import mock
|
||||
|
||||
from agents.models.openai_chatcompletions import OpenAIChatCompletionsModel
|
||||
@@ -12,6 +14,11 @@ from strix.interface import utils
|
||||
from strix.report import state as state_mod
|
||||
|
||||
|
||||
# ``strix.interface.__init__`` binds ``main`` to the entrypoint function, so
|
||||
# ``from strix.interface import main`` returns that function, not the module.
|
||||
main_mod = importlib.import_module("strix.interface.main")
|
||||
|
||||
|
||||
def test_grok_prefix_routes_to_chat_completions(monkeypatch) -> None: # type: ignore[no-untyped-def]
|
||||
client = mock.MagicMock()
|
||||
monkeypatch.setattr(grok, "get_subscription_client", lambda: client)
|
||||
@@ -69,3 +76,29 @@ def test_subscription_label_prefers_persisted_provider(monkeypatch) -> None: #
|
||||
# hardcoded default).
|
||||
fresh = mock.MagicMock(run_record={})
|
||||
assert utils._subscription_label(fresh) == "ChatGPT subscription"
|
||||
|
||||
|
||||
def test_persisted_run_record_carries_provider(tmp_path, monkeypatch) -> None: # type: ignore[no-untyped-def]
|
||||
settings = mock.MagicMock()
|
||||
settings.llm.model = "grok/grok-4"
|
||||
monkeypatch.setattr(main_mod, "load_settings", lambda: settings)
|
||||
monkeypatch.setattr(main_mod, "run_dir_for", lambda _name: tmp_path)
|
||||
captured: dict[str, object] = {}
|
||||
monkeypatch.setattr(main_mod, "write_run_record", lambda _dir, rec: captured.update(rec))
|
||||
|
||||
args = argparse.Namespace(
|
||||
run_name="run-test",
|
||||
targets_info=[],
|
||||
scan_mode="scan",
|
||||
instruction=None,
|
||||
non_interactive=True,
|
||||
local_sources=[],
|
||||
diff_scope={"active": False},
|
||||
scope_mode="mode",
|
||||
diff_base=None,
|
||||
)
|
||||
main_mod._persist_run_record(args)
|
||||
|
||||
# The resume/viewer record must carry the provider so resumed runs stay labeled.
|
||||
assert captured["auth_mode"] == "subscription"
|
||||
assert captured["subscription_provider"] == "Grok"
|
||||
|
||||
Reference in New Issue
Block a user