fix: address full-codebase review findings (21 fixes) - #1
Merged
Merged
Conversation
AgentMessage carried only four of the five ChatMessage fields, so a persisted native-tool-calling assistant turn lost its tool_calls on reload and orphaned the paired role="tool" replies on session resume. Add a nullable JSON tool_calls column (JSONB on Postgres), write it in append and reconstruct it in get_messages using the same payload shape the Redis backend persists via model_dump_json; NULL (including every row written before the column existed) reads back as None. The column is additive and delivered through the existing consumer-owned Alembic path — the SDK still ships no migrations.
switch_branch wrote the :active pointer with a bare SET, stripping any TTL the key carried (and leaving a first-time key with none), and fork created the :branches registry hash via HSETNX/HSET with no EXPIRE — so both keys could outlive a TTL-bounded session. switch_branch now writes the pointer with SET ... EX and fork re-applies the session TTL to the registry, matching the sliding-window model append maintains.
The docstring promised a TTL applies "when a positive integer" but the code checked only "is not None": with ttl_seconds=0 the EXPIRE sharing the append's MULTI/EXEC deleted every session key immediately on each append. The constructor now raises ValueError for ttl_seconds <= 0.
… batches
The runner's audit/hook correlation used single pending_action/pending_call
slots on the assumption of one tool in flight at a time. The loop's
MultiAction branch emits N ActionEvents, then N ToolStartedEvents, then N
terminal events (all in call order), so the first terminal event inherited
the LAST call's call_id/args and every other call was audited with args={}.
Pending state is now keyed by call_id: ActionEvents (which carry no
call_id) are claimed FIFO by the ToolStartedEvents as they arrive, then
stored under that call's call_id until its terminal event pops the entry.
The single-call path is the N=1 case of the same flow and is unchanged.
…nd audit Registry.invoke re-raises AgentSdkError subclasses other than ToolNotFound/ToolTimeout (e.g. MCPError), and AgentLoop.run documents that they propagate — but the runner caught only CancelledError. Such a run exited with terminated_by="interrupted", no error audit event, no on_error call, and on_run_end(error=None), contradicting the audit contract. The runner now catches AgentSdkError, emits the error audit event, fires on_error with the exception, records it for on_run_end, sets terminated_by="sdk_error", and re-raises so the caller still sees it. StateStoreError is excluded — the persist sites already handle it. Also reconciles the terminated_by enumeration with reality: adds the previously undocumented "interrupted" fallback and corrects the "cancelled" entry (aclose() surfaces as GeneratorExit → interrupted; "cancelled" is task cancellation only).
The tool_invocation audit payload embedded pending args verbatim, while audit/protocol.py forbids credentials/raw secrets in payloads and _emit_audit's docstring claimed payloads are "lengths/counts and tool metadata only". Tool args routinely carry secrets and PII, and the payload is persisted and logged as-is. The payload's "args" field is now the non-content summary from _args_metadata: sorted argument keys with each value's type name and length (len=None for unsized values) — the same value-dropping discipline as the MCP client's header redaction. The on_tool_start hook still receives the full args; only the persisted audit payload is reduced.
invoke_hook logged error_message=str(exc) on hook failure. Hooks receive
high-value objects — on_run_start gets the raw user_message, on_llm_call
the full ChatRequest, on_tool_start the tool args — so a hook raising
ValueError(f"unexpected: {request.messages}") dumped conversation content
into a WARNING log line.
The hook.invoke_failed log now carries hook_name and error_type only,
matching the existing logging discipline of MCPClient._apply_tool_error_hook
(whose DELIBERATE DIVERGENCE comment is updated — the divergence is gone).
… into ParserError json.loads raises RecursionError on deeply nested input, escaping the ParserError-only contract the loop relies on; both decode attempts in the JSON-mode and prose-mode parsers now wrap it with error_phase=json_decode / action_input_decode.
The JSON parser's truthiness check accepted names like ' ', producing a ThoughtAction the registry could never match; the name is now stripped like the prose parser's Action: header and a blank-after-strip name takes the same schema_validation path as an empty one.
A callable auth provider that raised (e.g. a down token endpoint surfacing as httpx.ConnectError) escaped the module's MCPError-only contract and, via Registry.invoke, was downgraded to a model-recoverable ToolResult instead of staying fatal. The exception type name only is captured in context — never its text, which may carry credential material; CancelledError still propagates untouched.
…eption groups
A CancelledError racing a real transport error arrives as a leaf of a mixed
BaseExceptionGroup and was translated into MCPError('MCP transport error:
CancelledError'), breaking the SDK's cancellation contract. BaseException
leaves that are not Exception now re-raise untouched before classification.
Both providers kept only properties/required and dropped $defs, so a nested BaseModel parameter reached the model as a dangling $ref; the loop ships only the four function-calling fields and providers reject unknown top-level keys, so the refs are now inlined via a shared resolver. Recursive models (no finite inline expansion) are rejected at decoration time for @tool and take the defensive empty-schema fallback for untrusted MCP server schemas.
…-set injected header The previous test injected an httpx.AsyncClient that ALREADY carried the User-Agent header, then asserted the captured request carried it — it passed even with the transport's user_agent plumbing deleted (verified by mutation). Rewrite it to drive the real owned-client path: intercept the transport's httpx.AsyncClient construction (keeping its headers/timeout, swapping only the network transport for the strict mock) and assert the header the server receives. Add a companion test pinning the injected-client contract: the transport merges only resolved auth headers onto an external client, so user_agent is a documented no-op there and the injected client's own header is what the server sees.
…nti-rot guard Neither MAINTAINING.md's "three standing decisions" pointer nor dependabot.yml's fallback-recording clause had a target: no coding_guidelines.md exists anywhere in this repo's history, and the _LOCAL_PATH_IN_BACKTICKS guard only matched src//tests/ paths, so the rot was invisible to it. - Drop both clauses (they pointed at nothing; there is no decisions doc to re-point to). - Re-point the submodule-import-path row's attribution: the never-existent api_pattern.md citation becomes the package docstring in src/fifty_agent_sdk/__init__.py, which does declare __all__ the API contract. - Widen the guard regex to also match backticked root-level *.md docs so this class is caught going forward (verified: README.md/CHANGELOG.md citations resolve, no false positives, and a coding_guidelines.md-style span now matches the pattern).
Nothing compared refs/tags/vX.Y.Z with project.version, so a mistyped tag silently published a mismatched artifact. Add a verify step at the top of the build job (before packaging) that extracts both and fails the job on mismatch. Skipped on workflow_dispatch, where the ref is a branch.
Every run emitted PytestDeprecationWarning: asyncio_default_fixture_loop_scope is unset. Set it explicitly to "function" — the current default — pinning the behavior against a future default change per the TD-004 policy. Verified gone: the full suite is green under -W error::DeprecationWarning.
- README's "What's new in 1.2.0" section was three releases stale at 1.5.0; retitle to the version-agnostic "Highlights". - CHANGELOG's [1.0.0] entry had no date while every other entry is dated; add 2026-06-19, the date of the chore(release): cut v1.0.0 commit. - Add an Unreleased section summarizing the fixes already on this branch, grouped per Keep a Changelog, calling out the consumer-visible audit tool_invocation payload change (args: raw dict -> per-key type/length metadata) for AuditSink implementors.
Contain JSON decode failures, restore Python 3.11 typing, synchronize Redis session expiry, and bound MCP schema expansion. closes #BR-013 closes #BR-014 closes #BR-015 closes #BR-016 closes #BR-017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the actionable findings from a full codebase review — 21 commits, one per fix. Full suite green (794 passed, 2 skipped),
ruff check/ruff format/mypy --strictclean.Data loss / correctness (
state,parser,tools)fix(state):SqlStateStore silently droppedChatMessage.tool_calls— persisted native-tool-calling assistant messages lost theirtool_callson reload, orphaning pairedrole="tool"replies. Adds a nullable JSON column; legacy NULL rows read back asNone.fix(state):Redisswitch_branch/forkwrote the:activepointer and:branchesregistry without TTL, leaking keys and enabling a "ghost branch" write path after trunk expiry.fix(state):ttl_seconds=0/negative madeEXPIRE 0silently delete every session key on each append — now rejected withValueError.fix(parser):deeply nested LLM output escaped theParserErrorcontract as a rawRecursionError, breaking the loop's exactly-one-FinalEventguarantee.fix(parser):whitespace-onlytool_nameaccepted in JSON mode.fix(tools):$defswere dropped while$refs survived schema translation — the LLM/provider received dangling references for nestedBaseModelparameters. Refs are now inlined (cycle-safe).Audit / observability integrity (
runner,observability)fix(runner):tool-invocation audit payloads and hooks used single pending slots — wrongcall_id/args for every MultiAction batch. Now correlated percall_id.fix(runner):a fatalAgentSdkErrorescaping the loop was invisible to hooks/audit (terminated_by="interrupted", noon_error, no audit event). New"sdk_error"path emits audit + hooks before re-raising.fix(runner):audittool_invocationpayloads carried full tool args verbatim — contradicting the "payloads MUST NOT contain credentials" contract. Args are now per-key type/length metadata only.AuditSinkimplementors readingpayload["args"].fix(observability):hook failures loggedstr(exc), which can embed hook arguments (user message, fullChatRequest). Now logs the exception type only.MCP error contract
fix(mcp):an auth callable that raises escaped the all-errors-become-MCPErrorcontract and was downgraded to a model-recoverable tool failure.fix(mcp):aCancelledErrorleaf inside aBaseExceptionGroupwas swallowed intoMCPError; non-Exceptionleaves now re-raise.API gaps (
llm,loop)feat(llm):OpenAICompatibleClientgainsaclose()+ async context manager (owned vs injectedhttp_clientownership documented).feat(llm):tool_choiceaccepts the documented specific-tool dict form.feat(llm):temperature=Noneomits the parameter entirely (needed for reasoning models that reject it); default stays0.0, wire-compatible.fix(loop):stream=True+native_tools_enabled=Truewas silently broken (streamed tool-call deltas are dropped by design) — now rejected withValueErrorat construction.Tests / CI / docs
test(mcp):theuser_agenttest asserted a header it had set itself; rewritten to exercise the owned-client path (mutation-verified).docs:danglingcoding_guidelines.mdreferences removed; anti-rot guard widened so it can catch that class (also exposed and fixed a deadapi_pattern.mdreference).ci:release workflow now fails when the tag doesn't matchpyproject.toml's version.build:asyncio_default_fixture_loop_scopepinned (silences deprecation warning; prevents silent drift per TD-004).docs:stale README heading retitled,[1.0.0]dated,## [Unreleased]added with these changes.Deliberately not included
Minor findings from the review that are doc-drift or hardening (dead code in
MCPClient.aclose, registryTimeoutErrormisclassification message,error.databounding, etc.) — can follow in a separate PR.