From 4227bee1efcccc0fe8f98029fb49e3910a4b82da Mon Sep 17 00:00:00 2001 From: banteg <4562643+banteg@users.noreply.github.com> Date: Thu, 8 Jan 2026 18:16:55 +0400 Subject: [PATCH] fix(config): harden onboarding and command menu (#70) --- src/takopi/cli.py | 17 +++--- src/takopi/settings.py | 16 ++++++ src/takopi/telegram/bridge.py | 4 +- src/takopi/telegram/onboarding.py | 35 +++++++++++-- tests/test_exec_bridge.py | 18 ++++--- tests/test_onboarding.py | 5 +- tests/test_onboarding_interactive.py | 78 ++++++++++++++++++++++++++++ tests/test_projects_config.py | 24 +++++++++ tests/test_settings.py | 36 +++++++++++++ 9 files changed, 206 insertions(+), 27 deletions(-) diff --git a/src/takopi/cli.py b/src/takopi/cli.py index b8e2daa..de29ea9 100644 --- a/src/takopi/cli.py +++ b/src/takopi/cli.py @@ -11,6 +11,7 @@ import typer from . import __version__ from .backends import EngineBackend from .config import ConfigError, load_or_init_config, write_config +from .config_migrations import migrate_config from .engines import get_backend, list_backends from .lockfile import LockError, LockHandle, acquire_lock, token_fingerprint from .logging import get_logger, setup_logging @@ -19,7 +20,6 @@ from .settings import ( TakopiSettings, load_settings, load_settings_if_exists, - require_telegram, validate_settings_data, ) from .transports import SetupResult, get_transport, list_transports @@ -54,14 +54,6 @@ def _resolve_transport_id(override: str | None) -> str: return raw.strip() -def load_and_validate_config( - path: str | Path | None = None, -) -> tuple[TakopiSettings, Path, str, int]: - settings, config_path = load_settings(path) - token, chat_id = require_telegram(settings, config_path) - return settings, config_path, token, chat_id - - def acquire_config_lock(config_path: Path, token: str | None) -> LockHandle: fingerprint = token_fingerprint(token) if token else None try: @@ -199,7 +191,8 @@ def _should_run_interactive() -> bool: def _setup_needs_config(setup: SetupResult) -> bool: - return any(issue.title == "create a config" for issue in setup.issues) + config_titles = {"create a config", "configure telegram"} + return any(issue.title in config_titles for issue in setup.issues) def _fail_missing_config(path: Path) -> None: @@ -361,6 +354,10 @@ def init( ) -> None: """Register the current repo as a Takopi project.""" config, config_path = load_or_init_config() + if config_path.exists(): + applied = migrate_config(config, config_path=config_path) + if applied: + write_config(config, config_path) cwd = Path.cwd() project_path = resolve_main_worktree_root(cwd) or cwd diff --git a/src/takopi/settings.py b/src/takopi/settings.py index 8607804..605e8e2 100644 --- a/src/takopi/settings.py +++ b/src/takopi/settings.py @@ -169,6 +169,22 @@ class TakopiSettings(BaseSettings): ) return raw + def transport_config( + self, transport_id: str, *, config_path: Path + ) -> dict[str, Any]: + if transport_id == "telegram": + return self.transports.telegram.model_dump() + extra = self.transports.model_extra or {} + raw = extra.get(transport_id) + if raw is None: + return {} + if not isinstance(raw, dict): + raise ConfigError( + f"Invalid `transports.{transport_id}` in {config_path}; " + "expected a table." + ) + return raw + def to_projects_config( self, *, diff --git a/src/takopi/telegram/bridge.py b/src/takopi/telegram/bridge.py index 5f64304..7d4ac45 100644 --- a/src/takopi/telegram/bridge.py +++ b/src/takopi/telegram/bridge.py @@ -296,7 +296,7 @@ def _build_bot_commands( cmd = entry.engine.lower() if cmd in seen: continue - commands.append({"command": cmd, "description": f"start {cmd}"}) + commands.append({"command": cmd, "description": f"use agent: {cmd}"}) seen.add(cmd) for alias, project in projects.projects.items(): cmd = alias.lower() @@ -308,7 +308,7 @@ def _build_bot_commands( alias=project.alias, ) continue - commands.append({"command": cmd, "description": f"project {cmd}"}) + commands.append({"command": cmd, "description": f"work on: {cmd}"}) seen.add(cmd) if "cancel" not in seen: commands.append({"command": "cancel", "description": "cancel run"}) diff --git a/src/takopi/telegram/onboarding.py b/src/takopi/telegram/onboarding.py index 2a55f74..b74f3ce 100644 --- a/src/takopi/telegram/onboarding.py +++ b/src/takopi/telegram/onboarding.py @@ -68,8 +68,12 @@ def _display_path(path: Path) -> str: return str(path) -def config_issue(path: Path) -> SetupIssue: - return SetupIssue("create a config", (f" {_display_path(path)}",)) +_CREATE_CONFIG_TITLE = "create a config" +_CONFIGURE_TELEGRAM_TITLE = "configure telegram" + + +def config_issue(path: Path, *, title: str) -> SetupIssue: + return SetupIssue(title, (f" {_display_path(path)}",)) def check_setup( @@ -91,10 +95,15 @@ def check_setup( try: require_telegram(settings, config_path) except ConfigError: - issues.append(config_issue(config_path)) + issues.append(config_issue(config_path, title=_CONFIGURE_TELEGRAM_TITLE)) except ConfigError: issues.extend(backend_issues) - issues.append(config_issue(config_path)) + title = ( + _CONFIGURE_TELEGRAM_TITLE + if config_path.exists() and config_path.is_file() + else _CREATE_CONFIG_TITLE + ) + issues.append(config_issue(config_path, title=title)) return SetupResult(issues=issues, config_path=config_path) issues.extend(backend_issues) @@ -413,6 +422,9 @@ def interactive_setup(*, force: bool) -> bool: return False else: console.print("no agents found on PATH. install one to continue.") + save_anyway = _confirm("save config anyway?", default=False) + if not save_anyway: + return False config_preview = _render_config( _mask_token(token), @@ -434,7 +446,20 @@ def interactive_setup(*, force: bool) -> bool: raw_config: dict[str, Any] = {} if config_path.exists(): - raw_config = read_raw_toml(config_path) + try: + raw_config = read_raw_toml(config_path) + except ConfigError as exc: + console.print(f"[yellow]warning:[/] config is malformed: {exc}") + backup = config_path.with_suffix(".toml.bak") + try: + shutil.copyfile(config_path, backup) + except OSError as copy_exc: + console.print( + f"[yellow]warning:[/] failed to back up config: {copy_exc}" + ) + else: + console.print(f" backed up to {_display_path(backup)}") + raw_config = {} merged = dict(raw_config) if default_engine is not None: merged["default_engine"] = default_engine diff --git a/tests/test_exec_bridge.py b/tests/test_exec_bridge.py index 7813408..b05bbd8 100644 --- a/tests/test_exec_bridge.py +++ b/tests/test_exec_bridge.py @@ -9,6 +9,7 @@ from takopi.model import EngineId, ResumeToken, TakopiEvent from takopi.telegram.render import prepare_telegram from takopi.runners.codex import CodexRunner from takopi.runners.mock import Advance, Emit, Raise, Return, ScriptRunner, Wait +from takopi.settings import load_settings, require_telegram from takopi.transport import MessageRef, RenderedMessage, SendOptions from tests.factories import action_completed, action_started @@ -76,8 +77,8 @@ def _return_runner( ) -def test_load_and_validate_config_rejects_empty_token(tmp_path) -> None: - from takopi import cli +def test_require_telegram_rejects_empty_token(tmp_path) -> None: + from takopi.config import ConfigError config_path = tmp_path / "takopi.toml" config_path.write_text( @@ -86,12 +87,13 @@ def test_load_and_validate_config_rejects_empty_token(tmp_path) -> None: encoding="utf-8", ) - with pytest.raises(cli.ConfigError, match="bot token"): - cli.load_and_validate_config(config_path) + with pytest.raises(ConfigError, match="bot token"): + settings, _ = load_settings(config_path) + require_telegram(settings, config_path) -def test_load_and_validate_config_rejects_string_chat_id(tmp_path) -> None: - from takopi import cli +def test_load_settings_rejects_string_chat_id(tmp_path) -> None: + from takopi.config import ConfigError config_path = tmp_path / "takopi.toml" config_path.write_text( @@ -100,8 +102,8 @@ def test_load_and_validate_config_rejects_string_chat_id(tmp_path) -> None: encoding="utf-8", ) - with pytest.raises(cli.ConfigError, match="chat_id"): - cli.load_and_validate_config(config_path) + with pytest.raises(ConfigError, match="chat_id"): + load_settings(config_path) def test_codex_extract_resume_finds_command() -> None: diff --git a/tests/test_onboarding.py b/tests/test_onboarding.py index 4beb000..7c0c8c3 100644 --- a/tests/test_onboarding.py +++ b/tests/test_onboarding.py @@ -32,9 +32,10 @@ def test_check_setup_marks_missing_codex(monkeypatch, tmp_path: Path) -> None: assert result.ok is False -def test_check_setup_marks_missing_config(monkeypatch) -> None: +def test_check_setup_marks_missing_config(monkeypatch, tmp_path: Path) -> None: backend = engines.get_backend("codex") monkeypatch.setattr(onboarding.shutil, "which", lambda _name: "/usr/bin/codex") + monkeypatch.setattr(onboarding, "HOME_CONFIG_PATH", tmp_path / "takopi.toml") def _raise() -> None: raise onboarding.ConfigError("Missing config file") @@ -68,4 +69,4 @@ def test_check_setup_marks_invalid_chat_id(monkeypatch, tmp_path: Path) -> None: result = onboarding.check_setup(backend) titles = {issue.title for issue in result.issues} - assert "create a config" in titles + assert "configure telegram" in titles diff --git a/tests/test_onboarding_interactive.py b/tests/test_onboarding_interactive.py index bc883ce..7eeab0c 100644 --- a/tests/test_onboarding_interactive.py +++ b/tests/test_onboarding_interactive.py @@ -148,3 +148,81 @@ def test_interactive_setup_preserves_projects(monkeypatch, tmp_path) -> None: saved = config_path.read_text(encoding="utf-8") assert "[projects.z80]" in saved assert 'path = "/tmp/repo"' in saved + + +def test_interactive_setup_no_agents_aborts(monkeypatch, tmp_path) -> None: + config_path = tmp_path / "takopi.toml" + monkeypatch.setattr(onboarding, "HOME_CONFIG_PATH", config_path) + + backend = EngineBackend(id="codex", build_runner=lambda _cfg, _path: None) + monkeypatch.setattr(onboarding, "list_backends", lambda: [backend]) + monkeypatch.setattr(onboarding.shutil, "which", lambda _cmd: None) + + monkeypatch.setattr(onboarding, "_confirm", _queue_values([True, False])) + monkeypatch.setattr( + onboarding.questionary, "password", _queue(["123456789:ABCdef"]) + ) + + def _fake_run(func, *args, **kwargs): + if func is onboarding._get_bot_info: + return {"username": "my_bot"} + if func is onboarding._wait_for_chat: + return onboarding.ChatInfo( + chat_id=123, + username="alice", + title=None, + first_name="Alice", + last_name=None, + chat_type="private", + ) + if func is onboarding._send_confirmation: + return True + raise AssertionError(f"unexpected anyio.run target: {func}") + + monkeypatch.setattr(onboarding.anyio, "run", _fake_run) + + assert onboarding.interactive_setup(force=False) is False + assert not config_path.exists() + + +def test_interactive_setup_recovers_from_malformed_toml(monkeypatch, tmp_path) -> None: + config_path = tmp_path / "takopi.toml" + bad_toml = 'transport = "telegram"\n[transports\n' + config_path.write_text(bad_toml, encoding="utf-8") + monkeypatch.setattr(onboarding, "HOME_CONFIG_PATH", config_path) + + backend = EngineBackend(id="codex", build_runner=lambda _cfg, _path: None) + monkeypatch.setattr(onboarding, "list_backends", lambda: [backend]) + monkeypatch.setattr(onboarding.shutil, "which", lambda _cmd: "/usr/bin/codex") + + monkeypatch.setattr(onboarding, "_confirm", _queue_values([True, True, True])) + monkeypatch.setattr( + onboarding.questionary, "password", _queue(["123456789:ABCdef"]) + ) + monkeypatch.setattr(onboarding.questionary, "select", _queue(["codex"])) + + def _fake_run(func, *args, **kwargs): + if func is onboarding._get_bot_info: + return {"username": "my_bot"} + if func is onboarding._wait_for_chat: + return onboarding.ChatInfo( + chat_id=123, + username="alice", + title=None, + first_name="Alice", + last_name=None, + chat_type="private", + ) + if func is onboarding._send_confirmation: + return True + raise AssertionError(f"unexpected anyio.run target: {func}") + + monkeypatch.setattr(onboarding.anyio, "run", _fake_run) + + assert onboarding.interactive_setup(force=True) is True + backup = config_path.with_suffix(".toml.bak") + assert backup.exists() + assert backup.read_text(encoding="utf-8") == bad_toml + saved = config_path.read_text(encoding="utf-8") + assert "[transports.telegram]" in saved + assert 'bot_token = "123456789:ABCdef"' in saved diff --git a/tests/test_projects_config.py b/tests/test_projects_config.py index bb20121..0c330b1 100644 --- a/tests/test_projects_config.py +++ b/tests/test_projects_config.py @@ -5,6 +5,7 @@ from typer.testing import CliRunner from takopi import cli from takopi.config import ConfigError +from takopi.config_store import read_raw_toml from takopi.settings import TakopiSettings @@ -50,6 +51,29 @@ def test_init_writes_project(monkeypatch, tmp_path) -> None: assert 'worktree_base = "main"' in saved +def test_init_migrates_legacy_config(monkeypatch, tmp_path) -> None: + config_path = tmp_path / "takopi.toml" + config_path.write_text('bot_token = "token"\nchat_id = 123\n', encoding="utf-8") + monkeypatch.setattr("takopi.config.HOME_CONFIG_PATH", config_path) + monkeypatch.setattr(cli, "resolve_default_base", lambda _: "main") + + repo_path = tmp_path / "repo" + repo_path.mkdir() + monkeypatch.chdir(repo_path) + + runner = CliRunner() + result = runner.invoke(cli.app, ["init", "z80"]) + assert result.exit_code == 0 + + raw = read_raw_toml(config_path) + assert "bot_token" not in raw + assert "chat_id" not in raw + assert raw["transport"] == "telegram" + assert raw["transports"]["telegram"]["bot_token"] == "token" + assert raw["transports"]["telegram"]["chat_id"] == 123 + assert "z80" in raw.get("projects", {}) + + def test_projects_default_engine_unknown() -> None: config = {"projects": {"z80": {"path": "/tmp/repo", "default_engine": "nope"}}} settings = TakopiSettings.model_validate(config) diff --git a/tests/test_settings.py b/tests/test_settings.py index 8508faf..d2680f0 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -136,6 +136,42 @@ def test_engine_config_none_and_invalid(tmp_path: Path) -> None: settings.engine_config("codex", config_path=config_path) +def test_transport_config_telegram_and_extra(tmp_path: Path) -> None: + config_path = tmp_path / "takopi.toml" + settings = TakopiSettings.model_validate( + { + "transport": "telegram", + "transports": {"telegram": {"bot_token": "token", "chat_id": 123}}, + } + ) + telegram = settings.transport_config("telegram", config_path=config_path) + assert telegram["bot_token"] == "token" + assert telegram["chat_id"] == 123 + + settings = TakopiSettings.model_validate( + { + "transport": "telegram", + "transports": { + "telegram": {"bot_token": "token", "chat_id": 123}, + "discord": None, + }, + } + ) + assert settings.transport_config("discord", config_path=config_path) == {} + + settings = TakopiSettings.model_validate( + { + "transport": "telegram", + "transports": { + "telegram": {"bot_token": "token", "chat_id": 123}, + "discord": "nope", + }, + } + ) + with pytest.raises(ConfigError, match="transports.discord"): + settings.transport_config("discord", config_path=config_path) + + def test_bot_token_none_allowed() -> None: settings = TakopiSettings.model_validate( {