Skip to content

fix(sqlite): register explicit datetime adapter, close deprecation warning - #266

Open
cdeust wants to merge 4 commits into
mainfrom
fix-sqlite-datetime-adapter-260
Open

fix(sqlite): register explicit datetime adapter, close deprecation warning#266
cdeust wants to merge 4 commits into
mainfrom
fix-sqlite-datetime-adapter-260

Conversation

@cdeust

@cdeust cdeust commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

sqlite_compat.py relied on sqlite3's implicit default datetime adapter, deprecated as of Python 3.12 and firing on 3 tests (test_consolidate.py::test_with_memories, ::test_protected_memories_skip_compression, test_memory_lifecycle.py::test_store_consolidate_recall). The root cause: cascade.py::_update_stage_entered binds a raw datetime.datetime object as a SQL parameter instead of an ISO string — confirmed the sole such call site in this codebase by instrumenting all three execute/executemany paths in sqlite_compat.py and running the full suite against it.

Fix: sqlite_compat.py registers an explicit sqlite3.register_adapter(datetime, _adapt_datetime_iso) callback (the sanctioned Python-docs recipe the deprecation warning itself points to). It writes the same "T"-separated .isoformat() spelling every other datetime write path in this codebase already uses (sqlite_store._now_iso(), etc.) — a single canonical wire format instead of two. Old rows on disk (written in the deprecated adapter's space-separated spelling) keep reading correctly: datetime.fromisoformat() — the read path every consumer here uses — parses both spellings to an identical value (verified empirically and pinned by a test), so no migration is required.

A pyproject.toml filterwarnings entry turns this specific DeprecationWarning into a hard failure going forward — a regression tripwire, not a silence.

Boy-scout (coding-standards.md §14)

sqlite_compat.py was already 335 lines — over this repo's 300-line file cap — before this change touched it. Split the pure SQL-dialect translation logic (_translate_sql/_returning_was_stripped/_SUPPORTS_RETURNING) into a new sqlite_sql_translate.py module (behaviour-preserving, byte-identical logic); sqlite_compat.py is now 232 lines. Updated the two existing tests that monkeypatched _SUPPORTS_RETURNING to target the module that actually defines it (patching the old re-exported copy would have been a silent no-op).

Running scoped mutation testing (mutmut, coding-standards.md §12) against the touched sqlite_compat.py surfaced 13 pre-existing gaps in _CompatCursor/_CompatExecutingCursor/PsycopgCompatConnection field wiring (not the datetime fix itself) — added targeted tests for all of them. One mutant (executemany's self.lastrowid = None) is a documented equivalent: sqlite3.Cursor.lastrowid is only meaningful after a single-row execute() INSERT per the Python docs, so it is always None after executemany() regardless — no input can distinguish the mutant from the real code.

A separate, unrelated mutation gap in _translate_sql itself (20 survivors, verbatim regex-translation code that predates this change entirely — confirmed identical source + identical tests as before the split) is outside this fix's blast radius and filed as #265 rather than folded in here, per §14.3.

Completion Ledger

Path introduced Asserting test
_adapt_datetime_iso returns tz-aware .isoformat() TestAdaptDatetimeIso::test_returns_plain_isoformat
_adapt_datetime_iso on a naive datetime TestAdaptDatetimeIso::test_naive_datetime_round_trips_through_isoformat
No DeprecationWarning via PsycopgCompatConnection.execute test_connection_execute_with_raw_datetime_param
No DeprecationWarning via cursor().execute test_cursor_execute_with_raw_datetime_param
No DeprecationWarning via cursor().executemany test_executemany_with_raw_datetime_params
Old (space-sep) and new (T-sep) spellings parse identically test_old_spelling_and_new_spelling_parse_identically
Pre-fix row on disk still reads correctly post-fix test_pre_fix_row_on_disk_still_reads_correctly
_CompatCursor.had_returning default is False test_compat_cursor_had_returning_defaults_to_false
_CompatCursor.rowcount mirrors the wrapped cursor test_compat_cursor_rowcount_mirrors_the_wrapped_cursor
_CompatCursor.fetchone synthesises the exact "id" key test_compat_cursor_synthesises_the_exact_id_key_when_flagged
fetchone requires flag AND lastrowid (not OR) test_compat_cursor_needs_both_flag_and_lastrowid_to_synthesise
executemany clears _had_returning, reports real rowcount test_executemany_clears_had_returning_and_reports_real_rowcount
PsycopgCompatConnection.execute computes had_returning from the real SQL test_connection_execute_computes_had_returning_from_the_actual_sql
executescript runs every statement verbatim test_executescript_runs_every_statement_verbatim
enable_load_extension forwards the flag verbatim test_enable_load_extension_forwards_the_flag_verbatim
filterwarnings turns the deprecation into an error proven by -W error::DeprecationWarning full-suite run (green)

Test plan

  • Reproduced the bug (instrumented execute/executemany, confirmed cascade.py::_update_stage_entered is the sole raw-datetime call site via a full-suite run)
  • tests_py/infrastructure/test_sqlite_datetime_adapter_260.py (7 tests) — fails on pre-fix code (verified: ImportError on _adapt_datetime_iso, and the DeprecationWarning reproduces standalone against the old code)
  • tests_py/handlers/test_wiki_pipeline_sqlite.py (+12 tests, 2 monkeypatch retargets)
  • Scoped mutation testing: sqlite_compat.py → 0 unaccounted survivors (1 documented equivalent); sqlite_sql_translate.py → 20 pre-existing survivors filed as sqlite_sql_translate.py: 20 surviving mutants in _translate_sql / _returning_was_stripped #265
  • ruff check + ruff format --check on all touched files
  • pyright on both touched production modules — 0 errors
  • Full suite (SQLite backend): 6373 passed, 81 skipped, 0 warnings (up from 6352 baseline + 21 new tests)
  • tests_py/handlers/test_consolidate.py + tests_py/integration/test_memory_lifecycle.py green under -W error::DeprecationWarning

Closes #260

🤖 Generated with Claude Code

https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u

cdeust and others added 4 commits July 30, 2026 03:01
@cdeust @claude
fix(sqlite): register explicit datetime adapter, close deprecation wa…
dd0027c 
…rning

sqlite_compat.py relied on sqlite3's implicit default datetime adapter,
deprecated as of Python 3.12. The one call site binding a raw
`datetime.datetime` parameter is cascade.py::_update_stage_entered
(confirmed the sole such site in this codebase via an instrumented
full-suite run). Registers an explicit `sqlite3.register_adapter`
callback that writes the same "T"-separated ISO-8601 spelling every
other datetime write path in this codebase already uses; old rows
(written in the deprecated adapter's space-separated spelling) keep
reading correctly via `datetime.fromisoformat`, which parses both
spellings identically, so no migration is needed.

Boy-scout: sqlite_compat.py was already 335 lines, over the repo's
300-line cap, before this change touched it. Split the pure SQL-dialect
translation logic (_translate_sql/_returning_was_stripped) into a new
sqlite_sql_translate.py module, bringing sqlite_compat.py to 232 lines.
Also added missing test coverage for _CompatCursor/PsycopgCompatConnection
field wiring surfaced by a scoped mutation run against the touched file
(0 unaccounted survivors; one documented equivalent mutant, see
test_executemany_clears_had_returning_and_reports_real_rowcount).

A pre-existing, unrelated mutation gap in _translate_sql itself (20
survivors, verbatim code predating this change) is filed as #265 per
coding-standards.md §14.3 rather than folded in here.

Closes #260

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
@cdeust @claude
fix(ci): merge main, remove docker-smoke stdin-EOF race, refresh test…
1d4cc3b 
…-count docs

Merges origin/main (3 commits ahead: #250 json_native mutants, #252
temporal timezone fix, #253 tree-sitter-language-pack pin) to pick up
its own doc-count bumps before recomputing the canonical figure, per
the standing rule that count-carrying doc hunks resolve on merits (main's
current figure + this branch's own prose), never a wholesale side pick.

Root-caused the Docker Smoke failure (run 30485406920): the container
answered `initialize` (id=1) then exited ~4s later without ever
answering `tools/list` (id=3), no protocol error either. Reproduced 0/5
locally (idle machine) but recurs on loaded CI runners, including on an
unrelated PR (#249, run 30495787124) with the identical signature —
confirming it is not a regression in this PR's sqlite_compat.py changes.
Read mcp/server/stdio.py: stdin_reader's `async for line in stdin` loop
closes its channel writer the instant stdin reaches EOF, and nothing in
the SDK's session lifecycle blocks that shutdown on an in-flight request
handler — `tools/list` does strictly more work than `initialize` (tool
registry enumeration, upstream-availability probes) so it is the one
that loses the race under load. Root-cause fix: drive the container's
stdin through a FIFO instead of a pipe from a process that exits
immediately, and hold the write end open — polling stdout for the
response (or a bounded ~55s wait) — before sending the real EOF. This
removes the race instead of retrying past it.

Recomputed the canonical test count on the merged tree (6564, measured
via `pytest --collect-only -q`) and refreshed every doc site
check_doc_claims.py enforces (README, CONTRIBUTING, CLAUDE.md,
ASSURANCE-CASE, .bestpractices.json) plus the self-hosted test-count
badge (regenerated via scripts/generate_repo_badges.py, not hand-edited).
Verified: `check_doc_claims.py --test-count 6564` and
`generate_repo_badges.py --check --test-count 6564` both pass; the
merged tree's own test subset (sqlite datetime adapter, wiki pipeline,
temporal, json_native, typecheck-parity, infrastructure + handlers —
1967 tests) is green; ruff check/format clean; shellcheck clean on the
modified script.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
@cdeust
docs(changelog): add issue #260 entry for the sqlite datetime adapter…
1ab0951 
… fix

The datetime-adapter fix (82844f3) shipped without a CHANGELOG entry,
which coding-standards.md §13.1 H4 requires for any consumer-observable
behavior change (here: the deprecation warning disappears and the wire
format is documented). Adds the entry in the same style/detail as its
neighbors, cross-referencing #260 and #265.

Refs #260
@cdeust @claude
docs: recompute canonical test count to 6565 after rebase onto main
3c18dab 
Rebasing fix-sqlite-datetime-adapter-260 onto origin/main (56f2f4f) picked
up 4 more commits than the 6564-test figure this branch's own merge commit
(1d4cc3b) had recomputed. Re-measured on the fully rebased tree via
`pytest --collect-only -q`: 6565 tests, 0 collection errors. Verified with
`scripts/check_doc_claims.py --test-count 6565` (pass) and
`scripts/generate_repo_badges.py --test-count 6565` (regenerates
assets/badge-tests.svg to match; `--check` confirms no drift after).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u
@cdeust
cdeust force-pushed the fix-sqlite-datetime-adapter-260 branch from 60cee39 to 3c18dab Compare July 30, 2026 01:18
This was referenced Jul 30, 2026
Merged
Merged
cdeust added a commit that referenced this pull request Jul 30, 2026
@cdeust @claude
fix(stdio): drain in-flight request handlers before closing write str…
3d74332 
…eam on read-EOF

Docker Smoke intermittently reported no tools/list response with no
exception and no JSON-RPC error frame -- reproduced on main (CI run
30504042295, attempt 1 failed / attempt 2 succeeded on the identical
commit), on PR #254, and on PR #266.

Root cause is upstream, in mcp 1.29.0: BaseSession._receive_loop
(mcp/shared/session.py) closes the write stream unconditionally the
instant stdin reaches EOF, even when a request dispatched from an
earlier line in the same batch (tools/list after initialize) is still
running in its own task and has not called respond() yet.
Server._handle_request catches the resulting ClosedResourceError and
logs it via logger.debug() on a logger with zero handlers by default,
so the drop is completely silent. All three JSON-RPC lines are always
fully read (measured parsed_count == 3 on every trial, pass and fail
alike) -- nothing is ever left unread; only the already-computed
response is lost. fastmcp 3.4.5's LowLevelServer.run override removes
the base SDK's own `finally: tg.cancel_scope.cancel()` mitigation with
nothing in its place, so Cortex's stdio entry point inherited the
hazard unmitigated.

Fixed at Cortex's composition root:
mcp_server/infrastructure/stdio_transport.py interposes a write-stream
proxy that no-ops the SDK's premature aclose() and closes the real
stream only after the low-level server's run() call has returned --
which, by anyio task-group join semantics, is only once every
dispatched handler has had its own chance to respond.
mcp_server/__main__.py::main() now drives stdio through this wrapper
instead of mcp.run(transport="stdio") directly.

Regression tests at the SDK boundary
(tests_py/infrastructure/test_stdio_transport.py): one test reproduces
the drop against the bare upstream call directly (a permanent
characterization of the upstream defect), a second drives the
identical race through the fix and asserts the response survives --
verified to fail against the pre-fix code. A scoped mutation run
(scripts/mutation_check.sh) found further gaps, hardened in
test_stdio_transport_wiring.py (the `stateless` parameter's actual
MCP-lifecycle effect, and the outer run_stdio_drained wrapper's
banner/transport-context-var/log-message wiring); final scoped
mutation score: 42/43 killed, 1 documented equivalent.

scripts/docker_smoke.sh gains a second, independent hardening: its
timeout/gtimeout wrapper was itself measured not to reliably stop a
genuinely hung container (SIGTERM to the docker run CLIENT process
does not reliably reach the CONTAINER). A --cidfile-based watchdog now
docker kills the actual container ID after the same 60s budget,
verified against a deliberately hanging test image (fails in exactly
60s, no leaked container) and a deliberately broken/exiting one (fails
immediately) -- both directions proven stable across repeated runs, on
both the timeout-available and no-timeout-binary code paths.

Boy-scout: MIN_TOOL_COUNT's default and source comment had drifted to
49 (citing a test name that no longer exists) against the true current
baseline of 52 -- corrected in the same change, along with the
resulting test-count doc claims (6550 -> 6562 across README,
CONTRIBUTING, CLAUDE.md, the assurance case, .bestpractices.json, and
the tests badge).

Co-Authored-By: Claude <noreply@anthropic.com>
cdeust added a commit that referenced this pull request Jul 30, 2026
@cdeust @claude
fix(stdio): drain in-flight request handlers before closing write str…
4d77e6f 
…eam on read-EOF

Docker Smoke intermittently reported no tools/list response with no
exception and no JSON-RPC error frame -- reproduced on main (CI run
30504042295, attempt 1 failed / attempt 2 succeeded on the identical
commit), on PR #254, and on PR #266.

Root cause is upstream, in mcp 1.29.0: BaseSession._receive_loop
(mcp/shared/session.py) closes the write stream unconditionally the
instant stdin reaches EOF, even when a request dispatched from an
earlier line in the same batch (tools/list after initialize) is still
running in its own task and has not called respond() yet.
Server._handle_request catches the resulting ClosedResourceError and
logs it via logger.debug() on a logger with zero handlers by default,
so the drop is completely silent. All three JSON-RPC lines are always
fully read (measured parsed_count == 3 on every trial, pass and fail
alike) -- nothing is ever left unread; only the already-computed
response is lost. fastmcp 3.4.5's LowLevelServer.run override removes
the base SDK's own `finally: tg.cancel_scope.cancel()` mitigation with
nothing in its place, so Cortex's stdio entry point inherited the
hazard unmitigated.

Fixed at Cortex's composition root:
mcp_server/infrastructure/stdio_transport.py interposes a write-stream
proxy that no-ops the SDK's premature aclose() and closes the real
stream only after the low-level server's run() call has returned --
which, by anyio task-group join semantics, is only once every
dispatched handler has had its own chance to respond.
mcp_server/__main__.py::main() now drives stdio through this wrapper
instead of mcp.run(transport="stdio") directly.

Regression tests at the SDK boundary
(tests_py/infrastructure/test_stdio_transport.py): one test reproduces
the drop against the bare upstream call directly (a permanent
characterization of the upstream defect), a second drives the
identical race through the fix and asserts the response survives --
verified to fail against the pre-fix code. A scoped mutation run
(scripts/mutation_check.sh) found further gaps, hardened in
test_stdio_transport_wiring.py (the `stateless` parameter's actual
MCP-lifecycle effect, and the outer run_stdio_drained wrapper's
banner/transport-context-var/log-message wiring); final scoped
mutation score: 42/43 killed, 1 documented equivalent.

scripts/docker_smoke.sh gains a second, independent hardening: its
timeout/gtimeout wrapper was itself measured not to reliably stop a
genuinely hung container (SIGTERM to the docker run CLIENT process
does not reliably reach the CONTAINER). A --cidfile-based watchdog now
docker kills the actual container ID after the same 60s budget,
verified against a deliberately hanging test image (fails in exactly
60s, no leaked container) and a deliberately broken/exiting one (fails
immediately) -- both directions proven stable across repeated runs, on
both the timeout-available and no-timeout-binary code paths.

Boy-scout: MIN_TOOL_COUNT's default and source comment had drifted to
49 (citing a test name that no longer exists) against the true current
baseline of 52 -- corrected in the same change, along with the
resulting test-count doc claims (6550 -> 6562 across README,
CONTRIBUTING, CLAUDE.md, the assurance case, .bestpractices.json, and
the tests badge).

Co-Authored-By: Claude <noreply@anthropic.com>
cdeust added a commit that referenced this pull request Jul 30, 2026
@cdeust @claude
fix(stdio): drain in-flight request handlers before closing write str…
fccfbbb 
…eam on read-EOF

Docker Smoke intermittently reported no tools/list response with no
exception and no JSON-RPC error frame -- reproduced on main (CI run
30504042295, attempt 1 failed / attempt 2 succeeded on the identical
commit), on PR #254, and on PR #266.

Root cause is upstream, in mcp 1.29.0: BaseSession._receive_loop
(mcp/shared/session.py) closes the write stream unconditionally the
instant stdin reaches EOF, even when a request dispatched from an
earlier line in the same batch (tools/list after initialize) is still
running in its own task and has not called respond() yet.
Server._handle_request catches the resulting ClosedResourceError and
logs it via logger.debug() on a logger with zero handlers by default,
so the drop is completely silent. All three JSON-RPC lines are always
fully read (measured parsed_count == 3 on every trial, pass and fail
alike) -- nothing is ever left unread; only the already-computed
response is lost. fastmcp 3.4.5's LowLevelServer.run override removes
the base SDK's own `finally: tg.cancel_scope.cancel()` mitigation with
nothing in its place, so Cortex's stdio entry point inherited the
hazard unmitigated.

Fixed at Cortex's composition root:
mcp_server/infrastructure/stdio_transport.py interposes a write-stream
proxy that no-ops the SDK's premature aclose() and closes the real
stream only after the low-level server's run() call has returned --
which, by anyio task-group join semantics, is only once every
dispatched handler has had its own chance to respond.
mcp_server/__main__.py::main() now drives stdio through this wrapper
instead of mcp.run(transport="stdio") directly.

Regression tests at the SDK boundary
(tests_py/infrastructure/test_stdio_transport.py): one test reproduces
the drop against the bare upstream call directly (a permanent
characterization of the upstream defect), a second drives the
identical race through the fix and asserts the response survives --
verified to fail against the pre-fix code. A scoped mutation run
(scripts/mutation_check.sh) found further gaps, hardened in
test_stdio_transport_wiring.py (the `stateless` parameter's actual
MCP-lifecycle effect, and the outer run_stdio_drained wrapper's
banner/transport-context-var/log-message wiring); final scoped
mutation score: 42/43 killed, 1 documented equivalent.

scripts/docker_smoke.sh gains a second, independent hardening: its
timeout/gtimeout wrapper was itself measured not to reliably stop a
genuinely hung container (SIGTERM to the docker run CLIENT process
does not reliably reach the CONTAINER). A --cidfile-based watchdog now
docker kills the actual container ID after the same 60s budget,
verified against a deliberately hanging test image (fails in exactly
60s, no leaked container) and a deliberately broken/exiting one (fails
immediately) -- both directions proven stable across repeated runs, on
both the timeout-available and no-timeout-binary code paths.

Boy-scout: MIN_TOOL_COUNT's default and source comment had drifted to
49 (citing a test name that no longer exists) against the true current
baseline of 52 -- corrected in the same change, along with the
resulting test-count doc claims (6550 -> 6562 across README,
CONTRIBUTING, CLAUDE.md, the assurance case, .bestpractices.json, and
the tests badge).

Co-Authored-By: Claude <noreply@anthropic.com>
cdeust added a commit that referenced this pull request Jul 30, 2026
@cdeust @claude
fix(stdio): drain in-flight request handlers before closing write str…
2590922 
…eam on read-EOF (#284)

* fix(stdio): drain in-flight request handlers before closing write stream on read-EOF

Docker Smoke intermittently reported no tools/list response with no
exception and no JSON-RPC error frame -- reproduced on main (CI run
30504042295, attempt 1 failed / attempt 2 succeeded on the identical
commit), on PR #254, and on PR #266.

Root cause is upstream, in mcp 1.29.0: BaseSession._receive_loop
(mcp/shared/session.py) closes the write stream unconditionally the
instant stdin reaches EOF, even when a request dispatched from an
earlier line in the same batch (tools/list after initialize) is still
running in its own task and has not called respond() yet.
Server._handle_request catches the resulting ClosedResourceError and
logs it via logger.debug() on a logger with zero handlers by default,
so the drop is completely silent. All three JSON-RPC lines are always
fully read (measured parsed_count == 3 on every trial, pass and fail
alike) -- nothing is ever left unread; only the already-computed
response is lost. fastmcp 3.4.5's LowLevelServer.run override removes
the base SDK's own `finally: tg.cancel_scope.cancel()` mitigation with
nothing in its place, so Cortex's stdio entry point inherited the
hazard unmitigated.

Fixed at Cortex's composition root:
mcp_server/infrastructure/stdio_transport.py interposes a write-stream
proxy that no-ops the SDK's premature aclose() and closes the real
stream only after the low-level server's run() call has returned --
which, by anyio task-group join semantics, is only once every
dispatched handler has had its own chance to respond.
mcp_server/__main__.py::main() now drives stdio through this wrapper
instead of mcp.run(transport="stdio") directly.

Regression tests at the SDK boundary
(tests_py/infrastructure/test_stdio_transport.py): one test reproduces
the drop against the bare upstream call directly (a permanent
characterization of the upstream defect), a second drives the
identical race through the fix and asserts the response survives --
verified to fail against the pre-fix code. A scoped mutation run
(scripts/mutation_check.sh) found further gaps, hardened in
test_stdio_transport_wiring.py (the `stateless` parameter's actual
MCP-lifecycle effect, and the outer run_stdio_drained wrapper's
banner/transport-context-var/log-message wiring); final scoped
mutation score: 42/43 killed, 1 documented equivalent.

scripts/docker_smoke.sh gains a second, independent hardening: its
timeout/gtimeout wrapper was itself measured not to reliably stop a
genuinely hung container (SIGTERM to the docker run CLIENT process
does not reliably reach the CONTAINER). A --cidfile-based watchdog now
docker kills the actual container ID after the same 60s budget,
verified against a deliberately hanging test image (fails in exactly
60s, no leaked container) and a deliberately broken/exiting one (fails
immediately) -- both directions proven stable across repeated runs, on
both the timeout-available and no-timeout-binary code paths.

Boy-scout: MIN_TOOL_COUNT's default and source comment had drifted to
49 (citing a test name that no longer exists) against the true current
baseline of 52 -- corrected in the same change, along with the
resulting test-count doc claims (6550 -> 6562 across README,
CONTRIBUTING, CLAUDE.md, the assurance case, .bestpractices.json, and
the tests badge).

Co-Authored-By: Claude <noreply@anthropic.com>

* chore(docs): reconcile test-count claims after rebase onto main (6600)

Rebasing fix-stdio-drain-on-eof onto current main (#283 landed since this
branch was authored, itself bumping the count to 6588) put the doc-count
claims six files carry out of sync with the rebased tree. Recomputed from
a real collection on this tree (not a delta against main's advertised
figure, and not the #287 local/CI-gap arithmetic from an earlier,
abandoned resolution attempt on this same branch, which does not apply
here):

  .venv/bin/python -m pytest --collect-only -q  ->  6600 tests collected

main's 6588 + this branch's 12 new tests (test_stdio_transport.py,
test_stdio_transport_wiring.py) = 6600, confirming the arithmetic.

* fix(stdio): resolve show_banner from fastmcp.settings when omitted (§13.2 review finding)

run_stdio_drained's show_banner default hardcoded True, diverging from the
call site it replaces: mcp.run(transport="stdio") goes through
TransportMixin.run_async, which resolves show_banner=None to
fastmcp.settings.show_server_banner (fastmcp==3.4.5, transport.py L56-57)
*before* reaching run_stdio_async -- run_stdio_async's own literal `True`
default is never actually exercised on that path. Mirroring only
run_stdio_async's signature dropped that settings resolution, so a user
who disabled the banner via FASTMCP_SHOW_SERVER_BANNER=false got it
printed on every stdio launch regardless.

Fix: show_banner defaults to None and resolves fastmcp.settings
.show_server_banner exactly where run_async does, before the explicit
argument would be checked, so an explicit argument still overrides it.

Tests (tests_py/infrastructure/test_stdio_transport_wiring.py):
- test_omitted_show_banner_resolves_from_fastmcp_settings_true / _false:
  pins the resolution in both directions; the _false variant fails
  against the pre-fix hardcoded-True default (verified locally by
  reverting the fix and re-running just that test).
- test_explicit_show_banner_wins_over_the_setting: an explicit argument
  is not clobbered by the setting.

Replaces the now-stale test_omitted_show_banner_defaults_to_showing_it,
which asserted the pre-fix (buggy) behavior as if it were the contract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u

* chore(docs): correct test-count claims to 6623 (real post-rebase collection)

The prior docs-reconciliation commit's count (6621) was computed while a
rebase conflict resolution was still in progress -- before this branch's
stdio_transport.py banner fix (and its net +2 tests) had been replayed on
top. Recomputed from a clean `pytest --collect-only -q` on the fully
rebased tree: 6623. Both drift gates verified clean at this count:

  python3 scripts/check_doc_claims.py --test-count 6623   -> doc claims OK
  python3 scripts/generate_repo_badges.py --test-count 6623
  python3 scripts/generate_repo_badges.py --check --test-count 6623
    -> badges OK (5 checked)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u

* docs: note the show_banner review-fix in the Unreleased CHANGELOG entry

H4 (coding-standards.md §13.1): the banner-default correction is an
observable-behavior fix landing in the same unreleased change as the
stdio drain-on-EOF fix it's part of; appended to that entry rather than
opening a new bullet since it's the same not-yet-released feature.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Yu6EnWspTfqHoGkExyS6u

* refactor: Extract Function _resolve_show_banner from run_stdio_drained

run_stdio_drained measured at 54 signature-to-close lines (was 37 before
740951f), breaching both coding-standards.md §4.2's 50-line hard cap and
this repo's own stricter 40-line/method convention (CLAUDE.md:76) -- the
show_banner-resolution fix in 740951f added ~20 lines of docstring/citation
alongside 2 lines of logic. High-stakes module (async coordination /
stream-close-ordering primitive): no 20% flexibility applies.

Extract Function (Fowler 2018 Ch. 6): show_banner resolution + its sourced
citation move into _resolve_show_banner(show_banner: bool | None) -> bool.
The citation is relocated, not deleted (coding-standards.md §8 requires a
source for every claim).

While relocating it: the citation's two upstream line-number references
(fastmcp/server/mixins/transport.py L56-57 / L184-186) were verified
against the actually-installed fastmcp==3.4.5 in .venv and found to be a
consistent -32-line offset from the real run_async (L88-89) / run_stdio_async
(L216-218) locations -- corrected in place rather than carried forward
unchecked.

Before: run_stdio_drained 54 lines.
After: run_stdio_drained 39 lines, _resolve_show_banner 27 lines.
Tests: 732 passed, 5 skipped before and after (tests_py/infrastructure/);
0 modified, 0 added. ruff check/format clean.

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: Extract Function _run_mcp_with_guarded_stream from _run_low_level_drained

Boy-scout finding (coding-standards.md §14): while measuring run_stdio_drained
for the previous commit, _run_low_level_drained (same file, introduced in this
PR's own fccfbbb, not pre-existing) measured at 57 signature-to-close lines --
over coding-standards.md §4.2's 50-line hard cap and this repo's own stricter
40-line/method convention (CLAUDE.md:76). High-stakes module: no flexibility
applies. Fixed in the same PR as a separate commit per §14.1's "separate
commit when it aids review."

Extract Function (Fowler 2018 Ch. 6): the mcp._mcp_server.run(...) call, its
cast() and the cast()-equivalence citation (already mutation-verified
equivalent, scripts/mutation_check.sh) move into
_run_mcp_with_guarded_stream(...). The citation is relocated, not deleted
(coding-standards.md §8).

Before: _run_low_level_drained 57 lines.
After: _run_low_level_drained 39 lines, _run_mcp_with_guarded_stream 37 lines.
Tests: 732 passed, 5 skipped before and after (tests_py/infrastructure/);
0 modified, 0 added. ruff check/format + pyright (scoped to the touched
file, 0 errors) clean.

Scoped mutation run (scripts/mutation_check.sh pattern, mutmut 3.x) against
the full touched file: 54 mutants, 53 killed, 1 survivor --
x__run_mcp_with_guarded_stream__mutmut_9 (cast(None, guarded) in place of
cast(MemoryObjectSendStream[SessionMessage], guarded)) -- the same
documented-equivalent mutant already named in the relocated citation and in
the Unreleased CHANGELOG entry (typing.cast's type argument is never read at
runtime); not a new gap introduced by this extraction.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs: note the run_stdio_drained size-refactor + citation fix in CHANGELOG

Documents the two Extract Function refactors (_resolve_show_banner,
_run_mcp_with_guarded_stream) and the fastmcp line-number citation
correction in the same Unreleased entry the original stdio-drain fix and
its show_banner follow-up already live in.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sqlite_compat.py relies on the deprecated stdlib datetime adapter (3 DeprecationWarnings in the suite)

1 participant

@cdeust