fix: v0.7.3 — agent create kb_ref kwarg leak; log shell exceptions

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

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"

src/pais/__init__.py

@@ -26,4 +26,4 @@
     "Settings",
 ]
 
-__version__ = "0.7.2"
+__version__ = "0.7.3"

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]")

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=<OptionInfo>).
     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: