fix: address full-codebase review findings (21 fixes)

.github/dependabot.yml

@@ -16,9 +16,7 @@
 # graph -> Dependabot should list this manifest. If it does not detect the PEP
 # 621 setuptools [project.optional-dependencies] table, nothing here runs and
 # the exact pins freeze silently — which is the exact rot this file exists to
-# prevent. On non-detection, fall back to a manual quarterly bump and record
-# that in the coding_guidelines.md Decisions row, so the obligation survives
-# outside this file.
+# prevent. On non-detection, fall back to a manual quarterly bump.
 
 version: 2
 

.github/workflows/release.yml

@@ -71,6 +71,21 @@ jobs:
         with:
           python-version: "3.12"
 
+      # A mistyped tag would silently publish an artifact whose version does
+      # not match the tag it was released as. Compare refs/tags/vX.Y.Z against
+      # pyproject's project.version BEFORE building, so the failure is cheap
+      # and nothing mismatched reaches the artifact. Skipped on
+      # workflow_dispatch (the ref is a branch, not a tag).
+      - name: Verify tag matches pyproject version
+        if: startsWith(github.ref, 'refs/tags/v')
+        run: |
+          tag="${GITHUB_REF_NAME#v}"
+          version="$(grep -m1 '^version = ' pyproject.toml | cut -d'\"' -f2)"
+          if [ "$tag" != "$version" ]; then
+            echo "::error::tag ${GITHUB_REF_NAME} does not match pyproject version ${version}"
+            exit 1
+          fi
+
       - name: Build sdist + wheel
         run: |
           python -m pip install --upgrade pip build

CHANGELOG.md

@@ -4,6 +4,82 @@ All notable changes to `fifty-agent-sdk` are documented here. The format is
 based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this
 project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Added
+- `OpenAICompatibleClient` gains `aclose()` and async context-manager support,
+  so the underlying httpx client can be disposed deterministically.
+- `ChatRequest.tool_choice` accepts the specific-tool dict form (forcing one
+  named tool), alongside the existing string forms.
+- `temperature=None` omits the parameter from the request body instead of
+  sending it, letting a provider's own default apply.
+
+### Fixed
+- Python 3.11 package imports work with the named-tool `tool_choice` form:
+  `TypedDict` now comes from the directly-declared `typing-extensions`
+  dependency, as required by Pydantic on Python versions below 3.12. (BR-014)
+- All five direct `json.loads` boundaries now contain bare `ValueError` as
+  `ParserError` or `LLMError`, including CPython's oversized-integer guard,
+  while preserving the existing malformed-syntax phases, messages, context,
+  and exception chaining. (BR-013)
+- Parser recursion-limit regressions now inject `RecursionError`
+  deterministically instead of relying on interpreter-specific behavior from
+  a 100,000-level JSON value, restoring portable Python 3.14 coverage.
+  (BR-017)
+- **Audit payload shape change (consumer-visible for `AuditSink`
+  implementors):** the `tool_invocation` payload's `args` field no longer
+  embeds the raw argument dict — it is now per-key metadata (sorted argument
+  keys with each value's type name and length, `len=None` for unsized
+  values), closing a leak of secrets/PII into persisted audit payloads. The
+  `on_tool_start` hook still receives the full args.
+- The runner now correlates tool invocations by `call_id` across
+  `MultiAction` batches — previously the first terminal event inherited the
+  last call's `call_id`/args and every other call was audited with
+  `args={}`.
+- A fatal `AgentSdkError` escaping the loop (e.g. `MCPError`) is now
+  surfaced to hooks and audit before re-raising: the error audit event is
+  emitted, `on_error` fires, and the run records
+  `terminated_by="sdk_error"` instead of exiting as `"interrupted"` with
+  `on_run_end(error=None)`.
+- Hook-failure logs now carry `hook_name` and `error_type` only, never
+  `str(exc)` — a raising hook could previously dump conversation content
+  into a WARNING log line.
+- `SqlStateStore` persists `ChatMessage.tool_calls` (nullable JSON column;
+  JSONB on Postgres), so a persisted native-tool-calling assistant turn no
+  longer loses its `tool_calls` — and orphans the paired `role="tool"`
+  replies — on session resume. Additive, via the existing consumer-owned
+  migration path; the SDK still ships no migrations.
+- `RedisStateStore` rejects non-positive `ttl_seconds` at construction —
+  `ttl_seconds=0` previously made the append's `EXPIRE` delete every session
+  key immediately.
+- Every Redis mutation (`append`, `fork`, `switch_branch`, and
+  `truncate_after`) now runs through one optimistic transaction that refreshes
+  every session key to the same sliding TTL. Registry/active/message-key
+  conflicts retry from a fresh snapshot up to a bounded limit, preventing
+  metadata keys from outliving their message lists. (BR-015)
+- The loop rejects `stream=True` combined with `native_tools_enabled` at
+  construction instead of misbehaving later.
+- A `RecursionError` from pathologically nested JSON is translated into
+  `ParserError` (`error_phase` `json_decode` / `action_input_decode`)
+  instead of escaping the parser contract.
+- The JSON-mode parser strips `tool_name` and rejects a whitespace-only one,
+  which previously produced a `ThoughtAction` the registry could never
+  match.
+- An MCP auth callable that raises (e.g. a down token endpoint) is
+  translated into `MCPError` — only the exception type name is captured,
+  never its text — instead of escaping the MCPError-only contract and being
+  downgraded to a model-recoverable `ToolResult`.
+- A `CancelledError` arriving as a leaf of a mixed transport
+  `BaseExceptionGroup` re-raises untouched instead of being translated into
+  `MCPError`, restoring the cancellation contract.
+- Tool schemas sent to the LLM now have `#/$defs` references inlined, so a
+  nested `BaseModel` parameter no longer reaches the model as a dangling
+  `$ref`; recursive models are rejected at decoration time for `@tool` and
+  fall back to an empty schema for untrusted MCP server schemas. Expansion is
+  additionally capped at 10,000 total resolver visits, preventing acyclic
+  fan-out from exhausting memory; MCP fallback logs contain only stable
+  reason/type metadata, never remote schema text. (BR-016)
+
 ## [1.5.0] - 2026-07-30
 
 ### Added
@@ -217,7 +293,7 @@ extracted with its full commit history from the monorepo it was first built in.
 - Import root is now `fifty_agent_sdk` (was `agent_sdk`).
 - Distributed and published as `fifty-agent-sdk` on PyPI.
 
-## [1.0.0]
+## [1.0.0] - 2026-06-19
 
 Initial production release: custom ReACT loop, JSON-mode tool calling, a
 pluggable LLM client (any OpenAI-compatible endpoint), in-process + MCP tool

MAINTAINING.md

@@ -33,14 +33,15 @@ tells you **that** you must notify a consumer and **what** to tell them, but not
 **which files to grep** — for that, open the tracker.
 
 **Authoring rule (the anti-rot test depends on it).** Every path is backticked.
-Every path in this file is a path in **this** repo, starting `src/` or `tests/`,
-optionally with a `:line` suffix. `tests/test_maintaining.py` asserts that every
-one of them still exists. **Do not add consumer-repo paths** — see above.
+Every path in this file is a path in **this** repo, starting `src/` or
+`tests/` — or a root-level `*.md` doc — optionally with a `:line` suffix.
+`tests/test_maintaining.py` asserts that every one of them still exists. **Do
+not add consumer-repo paths** — see above.
 
 **A row requires a real, verified consumer**, even though it is described by
 role rather than named. Not "a seam somebody might implement" — an integration
-confirmed by grepping an actual checkout. This repo has three standing decisions
-of the form *never on speculation* (see `coding_guidelines.md` Decisions), and a
+confirmed by grepping an actual checkout. This repo has standing decisions
+of the form *never on speculation*, and a
 map padded with maybe-consumers is exactly the doc that rots. Seams with no
 confirmed consumer go in [Not mapped](#not-mapped), so their absence is a
 recorded decision rather than an oversight.
@@ -60,7 +61,7 @@ invented one.
 |----------|---------|-----------|-----------------|
 | **`state/sql.py` physical database schema** — **highest-consequence row** | The table, column, unique-constraint and index NAMES emitted by the `sql` extra's ORM: `agent_sessions` (`src/fifty_agent_sdk/state/sql.py:199`), `agent_messages` (`:280`), `agent_branches` (`:372`), `uq_agent_messages_session_branch_sequence` (`src/fifty_agent_sdk/state/sql.py:286`), `ix_agent_messages_session_branch_sequence` (`:289`), and the `ON DELETE CASCADE` FKs to `agent_sessions.session_id`. Exposed as `fifty_agent_sdk.sql_metadata`. The SDK ships ORM models and **deliberately does not own migrations** — `SqlStateStore` has no `create_all` / bootstrap and expects the tables to pre-exist. | **Consumer B** — hand-authors an Alembic chain against a **production** database, transcribed from this ORM column-for-column, and pins itself to an SDK version in that migration's own docstring. Its migrations drop and recreate the `uq_`/`ix_` pair **by name**. | **Any DDL-name change is a MAJOR break, and this repo catches only some of them.** `tests/state/test_sql.py` asserts against `sql_metadata` directly, not only through the ORM: a **table** rename fails `test_metadata_exposes_both_tables` (`tests/state/test_sql.py:640`) and a **column** rename fails `test_metadata_columns_match_schema` (`:648`), which compares the full column-name set of all three tables. **The constraint and index NAMES were the uncovered gap** — `test_metadata_unique_constraint_on_session_branch_sequence` (`:691`) matches on the constraint's *columns* and never on its `name=`. TD-003 closed that with `test_metadata_pins_constraint_and_index_names`, so a rename now fails here instead of silently desynchronising a production database. On any rename/drop/constraint change: bump MAJOR, say so in `CHANGELOG.md` with the old→new names, and notify Consumer B so a follow-on migration lands before the version bump. Additive nullable columns are safe (their table simply lacks them until they migrate). |
 | **MCP public surface** — pinned by `tests/mcp/test_interface_stability.py` | `MCPClient`, `MCPClientConfig`, `MCPToolDef`, `MCPToolErrorHook` (`src/fifty_agent_sdk/mcp/client.py`), `MCPProvider`, `RefreshSummary` (`src/fifty_agent_sdk/tools/mcp_provider.py`) — plus two couplings the pin does not express: the module path `fifty_agent_sdk.mcp.client`, and the fact that `on_tool_error` fires *inside* `MCPClient.invoke`. | **Consumer A** — **subclasses** `MCPClient` and overrides the public `invoke`, constructing it with the public `on_tool_error` hook; patches `fifty_agent_sdk.mcp.client.StreamableHttpTransport` by **string path** in its tests. **Consumer B** — *calls* `MCPClient` / `MCPClientConfig` / `MCPProvider` but does not subclass. | Update the pin in the SAME change that widens the surface, then sweep both consumers. **Subclassing is a tighter contract than calling:** A overriding `invoke` makes the method's name, its `(self, name, args)` shape, and where `on_tool_error` fires relative to it all load-bearing for them, while B would survive an internal re-order. Moving `mcp/client.py` breaks A's string-path test patch with an `AttributeError` naming no SDK file — keep a re-export at the old module path, or major-bump. |
-| **Submodule import paths** — no pin | `api_pattern.md` says everything is imported from the package root and "submodule paths work but are not the contract". In practice both consumers import through them in **src-tier** code: `fifty_agent_sdk.tools.protocol`, `fifty_agent_sdk.llm.types`, `fifty_agent_sdk.state.protocol`, `fifty_agent_sdk.streaming`, `fifty_agent_sdk.errors`. Two adjacent cases: `fifty_agent_sdk.observability.hooks` is reached this way only in a consumer **test**, and `fifty_agent_sdk.mcp.client` is never imported by submodule path — it appears only as a `patch()` **string**, which the MCP row covers. | **Consumer A** — three src-tier sites importing from `tools.protocol`. **Consumer B** — sites importing from `state.protocol`, `tools.protocol`, `llm.types` and `errors`; `streaming` is the widest at five src-tier sites. | The stated contract is root-only, so this coupling is **the consumers' risk, not a promise this repo made** — recorded here so the cost of moving a module is visible rather than surprising. **Do not infer a file's import style from the symbols it uses:** the same names are imported by root elsewhere in the same repos, so a sweep of this row must re-grep for `fifty_agent_sdk.` **with a trailing dot**, never for the symbol names. (TD-003's own first draft got this wrong on four files and CI stayed green — see [Known limitations](#known-limitations).) When relocating a module under `src/fifty_agent_sdk/`, prefer leaving a re-export at the old path for one MINOR; otherwise treat it as MAJOR and name the moved paths in `CHANGELOG.md`. |
+| **Submodule import paths** — no pin | The package docstring (`src/fifty_agent_sdk/__init__.py`) declares `fifty_agent_sdk.__all__` the API contract: everything is imported from the package root, and submodule paths work but are not the contract. In practice both consumers import through them in **src-tier** code: `fifty_agent_sdk.tools.protocol`, `fifty_agent_sdk.llm.types`, `fifty_agent_sdk.state.protocol`, `fifty_agent_sdk.streaming`, `fifty_agent_sdk.errors`. Two adjacent cases: `fifty_agent_sdk.observability.hooks` is reached this way only in a consumer **test**, and `fifty_agent_sdk.mcp.client` is never imported by submodule path — it appears only as a `patch()` **string**, which the MCP row covers. | **Consumer A** — three src-tier sites importing from `tools.protocol`. **Consumer B** — sites importing from `state.protocol`, `tools.protocol`, `llm.types` and `errors`; `streaming` is the widest at five src-tier sites. | The stated contract is root-only, so this coupling is **the consumers' risk, not a promise this repo made** — recorded here so the cost of moving a module is visible rather than surprising. **Do not infer a file's import style from the symbols it uses:** the same names are imported by root elsewhere in the same repos, so a sweep of this row must re-grep for `fifty_agent_sdk.` **with a trailing dot**, never for the symbol names. (TD-003's own first draft got this wrong on four files and CI stayed green — see [Known limitations](#known-limitations).) When relocating a module under `src/fifty_agent_sdk/`, prefer leaving a re-export at the old path for one MINOR; otherwise treat it as MAJOR and name the moved paths in `CHANGELOG.md`. |
 | **Protocol seams with a confirmed external implementor** — no pin | Structural typing means an external class satisfies `Tool` (`src/fifty_agent_sdk/tools/protocol.py`), `AuditSink` (`src/fifty_agent_sdk/audit/protocol.py`) or `LLMClient` (`src/fifty_agent_sdk/llm/protocol.py`) without importing or inheriting anything — and `Hooks` (`src/fifty_agent_sdk/observability/hooks.py`) is constructed by keyword. Widening a signature or adding a required protocol method breaks the implementor with **no** failure in this repo and no `runtime_checkable` failure there (`runtime_checkable` checks method *presence* only). | `Tool` — both consumers (A wraps tools for caching; B wraps one for gateway resilience). `AuditSink` — Consumer B. `LLMClient` — Consumer B, in a src-tier scripted client (not a test double). `Hooks` — both, each constructing it by keyword. | The only guard is the **consumer's own** `mypy` run, which happens after they upgrade. So: adding a protocol method or widening an existing signature is MAJOR; adding a keyword-only parameter with a default is MINOR and safe. New `Hooks` slots must default to `None` (the existing convention) or every keyword construction site breaks. Name any protocol change in `CHANGELOG.md` — it is the only notice these implementors get. |
 
 ## Private symbols

README.md

@@ -213,7 +213,7 @@ depends (→):
 legend: ▶ entry   ▢ package   name module   → depends
 ```
 
-## What's new in 1.2.0
+## Highlights
 
 - **branching** — first-class conversation branching on `StateStore`: `fork`, `list_branches`, `switch_branch`, branch-scoped `get_messages(..., branch_id=...)`, plus `BranchInfo` and `TRUNK_BRANCH_ID`. a session is now a tree of branches with an active head, and `append` writes to the active branch (the edit-a-message / regenerate model). implemented across memory, SQL, and Redis backends, data-additive and zero-migration: existing sessions read as the trunk branch. breaking for custom `StateStore` implementations: they must add the new methods.
 - **`StateStore.truncate_after(session_id, sequence, *, branch_id=None)`** — a destructive hard-delete of a branch's tail (messages with sequence > N), for redaction, retention, and rollback. only the target branch's own messages are removed (a `fork`'s inherited prefix is never touched), and it is idempotent: a no-op on an unknown session or branch.

pyproject.toml

@@ -31,6 +31,9 @@ classifiers = [
 
 dependencies = [
     "pydantic>=2.5.0",
+    # ChatRequest's named-tool choice uses TypedDict. Importing it from
+    # typing_extensions is required by Pydantic on Python 3.11.
+    "typing-extensions>=4.6.1",
     "httpx>=0.26.0",
     "structlog>=24.1.0",
     "openai>=1.30.0,<3.0.0",
@@ -128,6 +131,10 @@ exclude = ["tests/", "build/", ".venv/"]
 
 [tool.pytest.ini_options]
 asyncio_mode = "auto"
+# Pin the fixture loop scope explicitly (TD-004): "function" is pytest-asyncio's
+# current default, and setting it silences the unset-option deprecation warning
+# while pinning the behavior against a future default change.
+asyncio_default_fixture_loop_scope = "function"
 testpaths = ["tests"]
 python_files = ["test_*.py"]
 addopts = ["-v", "--tb=short", "--strict-markers", "--import-mode=importlib"]

src/fifty_agent_sdk/llm/__init__.py

@@ -14,6 +14,8 @@
     FinishReason,
     Role,
     ToolCall,
+    ToolChoiceFunction,
+    ToolChoiceFunctionName,
     Usage,
 )
 
@@ -26,5 +28,7 @@
     "OpenAICompatibleClient",
     "Role",
     "ToolCall",
+    "ToolChoiceFunction",
+    "ToolChoiceFunctionName",
     "Usage",
 ]