diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f2ec11..db3258a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## 0.7.3 · `agent create` kb_ref leak; log shell exceptions to file + +### Fixed + +- **`pais agent create` no longer crashes with `TypeError: agent_create() got an unexpected keyword argument 'kb_ref'`.** Regression from v0.7.2: the KB→index picker cascade stashes the picked KB into `PickerContext.answers["kb_ref"]` as scratch state, but the interactive dispatcher then fed the full `answers` dict through `spec.callback(**answers)` — and `agent_create` declares no `kb_ref` parameter. The dispatcher now filters `answers` to the set of declared `spec.params` before invoking, so any picker's scratch keys stay contained. Defense-in-depth — future pickers that stash side-channel state won't regress the same way. +- **Interactive shell failures now land in `~/.pais/logs/pais.log`.** Previously the top-level exception handlers in `enter_interactive` only printed `error: …` to the console; the traceback never reached the rotating file handler, so `pais doctor` / `pais logs tail` couldn't recover it for support. Both the flat-menu dispatch path and the workflow path now emit `shell.command_crashed` / `shell.workflow_crashed` (with full traceback) via the existing structlog logger before printing. + ## 0.7.2 · `agent create` KB→index picker cascade; hidden-param OptionInfo leak ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 363e7b7..b46fba1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "pais-sdk-cli" -version = "0.7.2" +version = "0.7.3" description = "Contract-first Python SDK + CLI for VMware Private AI Service (PAIS), with mock server for offline development" readme = "README.md" requires-python = ">=3.10" diff --git a/src/pais/__init__.py b/src/pais/__init__.py index 8bc294e..d936b27 100644 --- a/src/pais/__init__.py +++ b/src/pais/__init__.py @@ -26,4 +26,4 @@ "Settings", ] -__version__ = "0.7.2" +__version__ = "0.7.3" diff --git a/src/pais/cli/interactive.py b/src/pais/cli/interactive.py index d4447a4..025041a 100644 --- a/src/pais/cli/interactive.py +++ b/src/pais/cli/interactive.py @@ -41,6 +41,9 @@ ) from pais.config import Settings from pais.errors import PaisError +from pais.logging import get_logger + +log = get_logger("pais.cli.shell") # Commands that require a confirm prompt before dispatch + auto-pass yes=True. _DESTRUCTIVE: frozenset[tuple[str, ...]] = frozenset( @@ -112,8 +115,19 @@ def enter_interactive(app: typer.Typer) -> None: except KeyboardInterrupt: console.print("\n[dim]aborted; back to menu[/dim]\n") except PaisError as e: + log.error( + "shell.command_failed", + command=" ".join(choice.path), + error=str(e), + error_type=type(e).__name__, + ) console.print(f"[red]error:[/red] {e}\n") except Exception as e: # pragma: no cover + log.exception( + "shell.command_crashed", + command=" ".join(choice.path), + error_type=type(e).__name__, + ) console.print(f"[red]error:[/red] {type(e).__name__}: {e}\n") continue @@ -124,8 +138,19 @@ def enter_interactive(app: typer.Typer) -> None: except KeyboardInterrupt: console.print("\n[dim]aborted; back to menu[/dim]\n") except PaisError as e: + log.error( + "shell.workflow_failed", + workflow=type(workflow).__name__, + error=str(e), + error_type=type(e).__name__, + ) console.print(f"[red]error:[/red] {e}\n") except Exception as e: # pragma: no cover + log.exception( + "shell.workflow_crashed", + workflow=type(workflow).__name__, + error_type=type(e).__name__, + ) console.print(f"[red]error:[/red] {type(e).__name__}: {e}\n") @@ -244,8 +269,14 @@ def _dispatch(spec: CommandSpec, settings: Settings, console: Console) -> None: # Call the callback outside the client `with` so the command can build its # own client (the dispatched commands all do `with _client() as c: ...`). console.print(f"\n[bold]→ {spec.display}[/bold]\n") + # Pickers may stash scratch state (e.g. `kb_ref` for the KB→index cascade) + # into `ctx.answers` that the callback itself does not declare as a param. + # Filter to declared names so those scratch keys don't leak through as + # unexpected kwargs (regression from v0.7.2). + declared = {p.name for p in spec.params} + call_kwargs = {k: v for k, v in answers.items() if k in declared} try: - spec.callback(**answers) + spec.callback(**call_kwargs) except typer.Exit as e: if e.exit_code: console.print(f"[yellow](command exited with code {e.exit_code})[/yellow]") diff --git a/tests/test_interactive_dispatch.py b/tests/test_interactive_dispatch.py index 4ec9b42..7af2d97 100644 --- a/tests/test_interactive_dispatch.py +++ b/tests/test_interactive_dispatch.py @@ -201,6 +201,145 @@ def _recorder(**kwargs: Any) -> None: # kb_search_tool:` branch in agent_create trips and pydantic blows up on # ToolLink(tool_id=). assert captured.get("kb_search_tool") is None + # Regression v0.7.3: the cascade stashes `kb_ref` into ctx.answers as scratch + # state; it must NOT leak through as a kwarg to `agent_create` (which has no + # such param). Dispatch filters by spec.params before calling the callback. + assert "kb_ref" not in captured, ( + "kb_ref scratch key must not leak into callback kwargs (v0.7.3 regression)" + ) + + +def test_dispatch_filters_picker_scratch_keys( + fake_q: _FakeQuestionary, + isolated_cache: None, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Defense-in-depth: a picker that stashes an arbitrary scratch key into + `ctx.answers` must not crash the callback with an unexpected-kwarg TypeError. + The fix filters `answers` to declared `spec.params` before invoking.""" + from rich.console import Console + + from pais.cli._pickers import CANCEL as PICKER_CANCEL + from pais.cli._workflows import _base as _workflows_base + from pais.cli.interactive import _dispatch + + store = Store() + seed_client = PaisClient(FakeTransport(store)) + kb = seed_client.knowledge_bases.create(KnowledgeBaseCreate(name="kb-scratch")) + seed_client.indexes.create( + kb.id, + IndexCreate(name="ix-scratch", embeddings_model_endpoint="BAAI/bge-small-en-v1.5"), + ) + + def _build(_self: Any) -> PaisClient: + return PaisClient(FakeTransport(store)) + + monkeypatch.setattr(Settings, "build_client", _build) + monkeypatch.setattr(_workflows_base, "questionary", fake_q) + + specs = walk(app) + spec = next(s for s in specs if s.path == ("agent", "create")) + + # Strict callback — only accepts declared params. If any scratch key leaks + # through, Python raises TypeError and the test fails. + received: dict[str, Any] = {} + + def _strict( + name: str, + model: str, + instructions: str | None = None, + index_id: str | None = None, + index_top_n: int = 5, + index_similarity_cutoff: float = 0.0, + kb_search_tool: Any = None, + output: str = "table", + ) -> None: + received.update( + { + "name": name, + "model": model, + "index_id": index_id, + } + ) + + spec.callback = _strict # type: ignore[misc] + + # Override the index picker to stash a bogus scratch key, then also write a + # legitimate index_id answer. This simulates any current or future picker + # that uses ctx.answers as a side-channel. + def _picker_with_scratch(_path: tuple[str, ...], param_name: str) -> Any: + if param_name != "index_id": + return None + + def _inner(ctx: Any) -> Any: + ctx.answers["kb_ref"] = kb.id + ctx.answers["__bogus_scratch__"] = "leak-me" + idx = seed_client.indexes.list(kb.id).data[0] + return idx.id + + return _inner + + monkeypatch.setattr("pais.cli.interactive.picker_for", _picker_with_scratch) + + fake_q.script( + "my-agent", + "openai/gpt-oss-120b-4x · VLLM", + "✅ Go (commit)", + ) + + # Must not raise TypeError. + _dispatch(spec, Settings(), Console()) + + assert received["name"] == "my-agent" + assert PICKER_CANCEL is not None # import sanity + + +def test_shell_logs_command_exceptions(monkeypatch: pytest.MonkeyPatch) -> None: + """When a dispatched command raises, the top-level shell loop must log it + (v0.7.3). Without this, tracebacks never reach `~/.pais/logs/pais.log` and + `pais doctor` / `pais logs tail` can't recover them for support. + + We exercise the exception path by stubbing `_dispatch` to raise, then + driving `enter_interactive` through one flat-menu iteration + quit. The + assertion is on `interactive.log` which is the structlog logger the fix + added at module scope.""" + + logged: list[tuple[str, str, dict[str, Any]]] = [] + + class _RecorderLogger: + def error(self, event: str, **kw: Any) -> None: + logged.append(("error", event, kw)) + + def exception(self, event: str, **kw: Any) -> None: + logged.append(("exception", event, kw)) + + monkeypatch.setattr(interactive, "log", _RecorderLogger()) + + def _boom(*_a: Any, **_k: Any) -> None: + raise RuntimeError("synthetic failure for logging regression test") + + monkeypatch.setattr(interactive, "_dispatch", _boom) + + fq = _FakeQuestionary() + # Landing → flat menu pick (any non-quit) → after boom, back to landing → quit. + specs = walk(app) + first_non_quit = next( + f"{s.display:24s} {s.help or '—'}" for s in specs if s.path == ("kb", "list") + ) + fq.script( + "📋 all commands…", + first_non_quit, + "📋 all commands…", + "⏏ quit", + ) + monkeypatch.setattr(interactive, "questionary", fq) + monkeypatch.setattr(_landing, "questionary", fq) + + interactive.enter_interactive(app) + + assert any(evt == "shell.command_crashed" for _, evt, _ in logged), ( + f"RuntimeError from a dispatched command must be logged, got: {logged}" + ) def test_picker_status_label_lookup(fake_q: _FakeQuestionary, isolated_cache: None) -> None: diff --git a/uv.lock b/uv.lock index 92f94ee..a59f673 100644 --- a/uv.lock +++ b/uv.lock @@ -899,7 +899,7 @@ wheels = [ [[package]] name = "pais-sdk-cli" -version = "0.7.2" +version = "0.7.3" source = { editable = "." } dependencies = [ { name = "fastapi" },