mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
a23cc2c25eb334fe873efbd5c1951d7199ec3cef
1652 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
a23cc2c25e |
ci: remove claude workflows
The @claude mention responder (claude.yml) and the automatic PR review (claude-code-review.yml) have been unreliable and are a frequent source of CI breakage. Drop both; core CI (ci.yml, docker-publish, publish, understone-example, vendor-js) is untouched and nothing else in the tree references them. |
||
|
Patrick Buckley
|
079257967d |
refactor(ui): extract _paintProjectPicker (dedupe modal/dashboard)
Review of #868 flagged the sync-paint + refresh + required/optional hint block as copy-pasted between showNewWsModal and _loadDashboardOptionsLists, already diverging structurally, so a future tweak could drift and silently re-introduce the FOUC on the missed surface. Collapse both into a shared _paintProjectPicker(sel, hint, {fork}) -- the modal passes the fork flag, the dashboard never forks. Guards re-pointed at the helper + a new one pins its sync-before-async pattern. |
||
|
Patrick Buckley
|
4079542447 |
fix(ui): paint composer project/persona pickers from warm cache (no FOUC)
The new-workstream modal, the dashboard composer, and the console launcher painted their project and persona <select>s only inside the async refresh().then(...) callback, so each open flashed an empty/stale dropdown for a network round-trip even though the client caches are already warmed at startup. Paint synchronously from the warm cache first, then refresh-and-repaint (still catches items created elsewhere). On a cold cache the sync paint is a no-op the async fills, so it is never worse than before. Both project paints reuse the same _populateProjectSelect + reconcile, so the require_project strict-picker invariant (never auto-select a real project into a possibly-shared one) is unchanged; persona reuses _populatePersonaSelect, which preserves a mid-window pick and only applies the kind default when nothing valid is selected. Also folds in two deferred require_project polish items in the same code: the dashboard Project label now shows the "required"/"optional" hint (parity with the modal), and _reconcileRequiredProjectSelection reuses the projectChoices() list its caller already built instead of recomputing it. Models/skills selectors are a separate follow-up (no client cache today). |
||
|
Patrick Buckley
|
8ce94360ae | docs(projects): requireProject() advisory is safe to read synchronously | ||
|
Patrick Buckley
|
c019ab41d7 |
feat(server): opt-in server.require_project gate
Add an opt-in, default-off `server.require_project` setting. When an admin enables it, creating an interactive chat is refused unless it is filed under a project. The feature is inert and byte-identical when off, and can only ever fail toward "off" (a missing config store or unset key reads as disabled). - settings_registry: server.require_project (bool, default False, live read). - auth: require_project_enabled + require_project_denies_create predicates (service scope / coordinator token_source exempt; NOT admin.coordinator), plus REQUIRE_PROJECT_ERROR / REQUIRE_PROJECT_CODE. - node create gate via a declarative cfg.create_gate_require_project (wired True on the interactive mount only; coordinator spawns stay ungated). - fork/resume: a fork's project is structurally its source's. Any explicit project_id is discarded, so a fork can never be re-filed under an unrelated project (which would move its copied history across a tenancy boundary). Inaccessible / projectless / nonexistent sources are uniform on body and status, so there is no cross-tenant oracle. - console cluster-create proxy surfaces only the coded require_project 400 and masks every other node outcome (401/429/3xx/5xx, un-coded 400) to a sanitized 502, guarding both body reads. - list_projects advisory field + projects.js requireProject() (fail-open). - fresh-create project picker requires an explicit project choice under the flag (no silent auto-select); forks hide the picker (inheritance is server-enforced) and get an accurate refusal message. - tests: predicate matrix, resume-inheritance oracle discriminators, console masking, and end-to-end node-gate mount wiring. |
||
|
Patrick Buckley
|
cdc360bd98 |
fix(server): sanitize the retry closure's error display
The retry (_run) closure emitted the raw str(exc) to ui.on_error, so a credential-bearing base-URL in a backend ConnectError (https://user:pass@host) crossed into the dashboard SSE — the confidentiality floor _record_fatal_error enforces, bypassed here. Sanitize the display inline with the same sanitize_error_text redactor. This is separable from the reused-session stale-flag hazard that keeps _run off ensure_error_recorded: that hazard is about recording / idempotency (deferred to #865); this is only the display string. The double state emit and the pre-try no-persist remain in #865. Adds a focused test that a retry-error's on_error is redacted. Flagged by review on #866. |
||
|
Patrick Buckley
|
3af80907c7 |
fix(server): init-message worker exits to error, not idle, on first-turn failure
The initial-message worker (_run_initial) collapsed both cancel and backend-error exits into one `except (Exception, GenerationCancelled)` arm that always stamped state=idle, clobbering the state=error that session.send's _record_fatal_error had persisted+emitted. A spawned child's first-turn backend failure (unreachable model server, exhausted quota, auth error) therefore read as an empty, successful turn — the coordinator's wait/inspect surface reads last_error only for state=='error' — and the real error surfaced only after a manual nudge re-ran the turn synchronously. Split the arm: cancel -> idle, exception -> error. The failed child now settles at state=error and the first wait_for_workstream returns the enriched backend error inline. Also fixes the same latent bug for scheduled tasks, which dispatch through the same endpoint and closure. A failed first turn is deliberately terminal for automated wakes: it settles to a non-ready error terminal, not the idle ready-set that timer/watch wakes recur to, so explicit user/coordinator action reactivates it rather than a silent auto-retry (a self-healing wake-from-error would be a separate wake-gate change). The exception arm routes through a new ChatSession.ensure_error_recorded: a no-op when send already recorded the error in-line (the common backend-boundary path — no duplicate state emit), and the recorder when a pre-try exception (model-registry refresh, user-turn append, system-message recompose) bypassed send's own handler, so state=error always carries a meaningful last_error. Its idempotency guard (_has_persisted_error) is session-lifetime, so ensure_error_recorded is scoped to _run_initial's FRESH first-turn session only; the docstring spells out why a session-reuse caller (retry, /send, coord send, wake) must not route through it until the per-turn error-recorded signal of #865 lands. The other half of making an errored workstream cheap for a model to handle is a stable identifier: the enriched backend error now leads with the model ALIAS the coordinator references everywhere (list_nodes, spawn) and annotates the backend id for the operator — "model=DeepSeek-V4-Flash (id=deepseek-v4-flash)" — so a model routing around a failed model correlates it against those surfaces without a lookup, instead of burning reasoning tokens reconciling the alias against a backend id it never sees anywhere else. Collapses to one token when the alias and id coincide. Tests (TestInitialWorkerFailureState) assert the coordinator-visible manager state and the persisted last_error across the matrix — common- backend and pre-try errors both settle error with a readable last_error; cancel-to-idle settles idle with no error recorded. De-forks the create-app fixture and uses the shared monotonic wait_until helper. The completion-notification honesty surface and the error-recording hygiene of the other send-worker closures (retry / main send / coord send / wake) are deferred to #865. |
||
|
Patrick Buckley
|
9dea2c89f1 |
chore(sdk): regenerate openapi-console.json
The console OpenAPI spec had drifted from build_console_spec(): the committed file was last generated at 1.7.0rc1 and was missing the persona and project_id workstream-creation fields (Personas and Projects, both 1.7) plus the version bump to 1.8.0a2. Regenerate via sdk/typescript/scripts/generate-types.py to resync. Spec-only; no console API behavior change (openapi-server.json was already current). |
||
|
Patrick Buckley
|
515d372a14 |
fix(compaction): validate retry_in backoff before rendering the retry note
updateCompactionProgress coerced evt.retry_in with Number() and rendered it unguarded, while the sibling part/total path two lines below is finiteness- validated — a malformed backoff would render "retrying in NaNs". Validate retry_in the same way (finite, non-negative), and keep the error text regardless: the error is the load-bearing half of the note, so an unparseable duration drops to "retrying (error)…" rather than suppressing the whole arm. Addresses PR review feedback on the compaction reducer. |
||
|
Patrick Buckley
|
abe053f507 |
fix(compaction): review round 11 — settle-helper null-guard to the chokepoint
The settleSendResponse extraction left the two panes' call sites diverging on
the null-guard: interactive passed bare `data`, the coordinator passed
`data || {}` — reintroducing the copy-paste variation the shared helper existed
to erase. If a /send 2xx body were ever non-object JSON, the unknown/"ok"
fall-through would deref `data.attached_ids` and paint an already-delivered
message as a connection error; the endpoint always returns an object, so this
is a latent divergence, not a live bug.
Normalize the body once at the helper entry (`data = data || {}`) so both call
sites pass bare `data` and stay byte-identical, and every internal deref plus
any future caller is covered by the single chokepoint. The node settle-harness
gains a null-body case — red without the fix, since the call-arg evaluation
throws before the stub runs.
|
||
|
Patrick Buckley
|
1e86f068cd |
chore(compaction): review round 10 — cleanups from the first correctness-clean round
- INTERJECTION_CAP_CHARS joins PENDING_SENDS_MAX in workstream.py: the 2000-char interjection cap was triplicated (queue_message's truncation, the defer-fidelity refusal, the test fake) and already drifting in measurement — the defer check deliberately measures RAW text (raw >= cleaned since parse_priority only strips, so it can only over-refuse into a full-fidelity fresh spawn, never admit a truncation), now stated in a comment. The four unrelated 2000s (notify tool, recall preview, summary formatting, agent step cap) stay deliberately unlinked — they are different contracts. - The changelog's ~110-line compaction bullet is split into six per-seam bullets matching house style, and the Breaking (1.8) compaction-event notice moved under "### Changed" where integrators scanning bullet heads will actually see it (cross-referenced both ways with the pre-1.8 embedder compat bullet). - SpawnMetricsHook takes (ui) only: the request parameter was threaded through the whole dispatch-attempt path solely to be ignored by both installed impls; the stale "coord wires None" claims in the rewritten comment blocks are corrected too. - The attachments tests' Mock-hardening block lives once in _harden_ws_mock() — deliberately excluding _worker_running, which each fixture chooses per scenario (one relies on the truthy auto-Mock). - Two hand-rolled poll loops become wait_until (file convention, diagnostic timeout) and the orphaned time import goes with them. |
||
|
Patrick Buckley
|
d280db514e |
fix(compaction): review round 9 — drain-exit ownership, missed-edge settle, pre-turn hook guard
Three point-guards from the ceiling round (no primitive took a hit;
correctness yield halved at identical review sensitivity):
- The drain's clean-exit wake moved OUT of the function-level try: it
runs after the drain has already retired its slot, so a raise out of
the wake (the dispatcher re-raises Thread.start failures) could reach
the last-resort handler and clear a slot this thread no longer owned —
nulling a successor drain's live registration and letting two drains
service one list. The wake now runs post-try under its own guard
(mirroring _retry_pending_wake), only on the clean-exit path, and the
last-resort slot-clear is identity-guarded like every sibling exit
seam. The except arm needed a function-local threading import: the
module-top import is TYPE_CHECKING-only, so the guard would have
NameErrored inside the handler with strict mypy fully green.
- The shared settle helper promotes a non-deferred chip that binds onto
an already-idle pane: its only sweep fired mid-POST (unbound then) and
no message_dispatched ever comes for non-deferred sends, so the chip
stayed a permanently retractable "queued" bubble for a delivered
message. Keyed on post-bind chip state (also catching a raced folded
settle bind just reconciled) and skipping dismiss-in-flight chips —
the sweep's own aria-busy discipline. Pinned behaviorally: the helper
now executes under node (a 4-row missed-edge matrix), possible since
the consumer-less window bridge is gone.
- _claim_generation's on_generation_claimed emission is call-guarded:
it sits on send()'s pre-turn path, before the user turn is appended
and before the fatal handler's coverage, so a raising override
degrades to a lost latch-break instead of silently dropping every
user message on that session.
Cleanups: /command's transport catch and status-less non-2xx bodies are
loud now (threading {ok, status} through the parse — deliberately no
throw-on-!ok pre-gate, since the busy and error arms ride 409/503);
PENDING_SENDS_MAX lives in workstream.py and ChatSession._QUEUE_MAX
aliases it (one backpressure bound, structurally incapable of
diverging); the send handler's not-ok arm uses _queue_full_response();
the dead window.createQueueController bridge is deleted and the file
header's consumer map corrected.
|
||
|
Patrick Buckley
|
1224b02d03 |
fix(compaction): review round 8 — seam obligations become primitives
Eight rounds of findings against the defer-and-drain seam shared one
generator: N sites each hand-copying M obligations (spawn discipline,
the order-barrier pair, backpressure, best-effort emission, the client
settle matrix), with every review finding an empty (site x obligation)
cell. This round makes each obligation a single primitive:
- The order barrier is Workstream.send_barrier_active() — one
definition of the two-term pair (pending entries OR drain alive),
consulted by the /send route, the coordinator adapter, and the
queued-nudge wake gate, which previously carried only the list term
and let a synthetic wake jump an acknowledged send during the
claimed-entry window. _PendingSend moved to workstream.py beside the
invariant that justifies the drain-alive term; the pending fields got
precise types and worker_kind became a Literal, so a typo'd
"command" comparison is now a type error instead of a silently
never-firing defer guard.
- _defer_send probes the barrier before constructing anything, bounds
acceptance at 10 pending (the interjection queue's own backpressure
contract — unbounded acceptance pinned message + attachment bytes
per entry for a whole command window and then ran one unattended
turn each), and spawns the drain with rollback: a Thread.start
failure pops the just-accepted entry and answers the retryable
queue_full instead of 500ing after registration (a phantom the
client could neither see nor retract, dispatched later as duplicate
turns). start() deliberately stays inside the lock, unlike
session_worker's outside-lock discipline: this slot is
is_alive()-gated, false for a constructed-but-unstarted thread, so
an outside-lock start would open a double-drain window.
- A /command whose worker never spawned answers 503
{"status": "error"} (spec + docs + a pane error arm) instead of the
generic 200 ok that told SDK callers their /clear ran.
- The compaction lifecycle emitter is raise-proof at its single
dispatch tail: a raising duck-typed hook degrades to a lost render,
never a lost end event — previously a raising on_error or a raising
failed-end emit left every pane a frozen progress bar, and a raising
SUCCESS end after the committed swap fabricated a failed end.
- The client settle matrix lives once: composer_queue's
settleSendResponse owns every /send response arm for both panes
(the near-verbatim twins were already drifting), parsePriority is
shared, and the busy stamp is centralized in setBusy(b, source) with
"server" as the fail-safe default. Deferred sends release the
composer (no worker exists for them; retracting the chip no longer
strands the pane in Stop mode), queue_full on an idle-looking pane
removes the optimistic bubble and restores busy (the refusal can now
fire with no worker and no drain to ever emit a state event), and
the pre-bind settle buffer is TTL-based — a burst of deferred
dispatches parked this tab's own raced settle first, where the old
size cap evicted exactly it.
- The command backstop / console proxy timeout inequality is enforced
by a test importing both named constants (both proxy_client
constructions, startup and the mTLS re-create); the compaction card
wears blue (magenta is reserved for the MCP surface); the redundant
TerminalUI.on_compaction override is gone (the inherited protocol
default is the policy site).
|
||
|
Patrick Buckley
|
5511ab9a35 |
fix(session-worker): release the slot claim when Thread.start itself fails
If thread creation raised (thread exhaustion, MemoryError), the dispatcher had already claimed the worker slot under ws._lock — but the flag's only clearer is _runner's finally, on a thread that never started. The workstream then looked idle forever (no state change ever fired) while every subsequent dispatch took the reuse path into a queue no worker would drain, until an operator force-cancel. Roll the claim back under the lock (identity-guarded, like _runner's own clear, so a concurrent force-cancel's successor is never clobbered) and re-raise. Re-raise rather than return False: callers' crash paths — the deferred-send drain's per-iteration handler with its backoff — are shaped for exceptions, and a False would masquerade as queue-full backpressure and mislabel the wake gate's refusal log. worker_kind is left stale, as documented (every reader conjoins _worker_running). Affected every dispatch path: sends, wakes, retries, the deferred-send drain, and workstream init. |
||
|
Patrick Buckley
|
fd5d3efb43 |
fix(compaction): review round 7 — drain crash/order/settle rows, protocol-default fallback, bool event-id guard
Completes the defer-and-drain seam against the matrix rows round 6 never
enumerated (the defer contract itself took no hits):
- Crash row: a claimed entry survives a dispatch crash — the
per-iteration handler re-inserts it at head (claim-flagged so a
claim-section failure can't duplicate it), backs off ~1s, retries.
The last-resort handler spawns no successor (Thread.start fails under
the exact exhaustion that reaches it): the route's ensure-drain stays
the single spawn site, so single-flight is structural and a dead
drain revives on the next defer.
- Order row: the pending list is the order authority. The /send route
pre-checks pending/drain-alive under the same lock acquisition that
appends (one _defer_send helper serves the barrier and command-window
triggers); the coordinator adapter refuses via its return value; the
queued-nudge wake gate yields to pending sends and is re-armed by the
drain's clean exit — which covers lists emptied by pure retraction —
as well as every deferred turn's exit; retry-after-rewind is a
documented accepted overtake; init/create is fresh-ws-by-construction.
- Client settle row: queued responses carry "deferred": true
(SendResponse + regenerated openapi-server.json, status enumeration
completed); bind(el, msgId, {deferred, attachedCount}) replaces the
_deferredAttachments expando; the idle sweep skips deferred and
unbound chips; the shared dispatch attempt emits pane-tier
message_dispatched (folded: true for interjection fold-ins — the chip
clears only its deferred flag and keeps a live x while DELETE still
genuinely retracts); settles that beat bind() park in a bounded
buffer; an idle-thinking pane retro-converts its optimistic bubble
into a real queued chip instead of presenting a parked message as
sent. Rejection polling waits on the slot flags — one dispatch
attempt per slot-state change, not 4 Hz.
- SessionUI.on_compaction's protocol stub became a real default body
(the classic on_info rendering): explicit subclasses inherit protocol
members as real methods, which defeated _compaction_event's getattr
fallback for exactly the pre-1.8 embedders it serves.
- _coerce_event_id() rejects bools (isinstance(True, int) is True) at
all three duck-typed event-id coercions: the compaction marker stamp,
on_system_turn's persisted return, and _ui_event_id.
- Quick-command backstop 60s -> 25s, under the console proxy's 30s so
the degraded "running" answer can traverse a proxied pane (which now
surfaces it); /resume docs drop the fictional history SSE event
(clear_ui + REST re-fetch is the contract); /send response docs match
the wire.
|
||
|
Patrick Buckley
|
e99673eb0c |
fix(compaction): review round 6 — defer-and-drain send windows, workstream-scoped notify, ERROR badge survives /compact
Replace park-and-abandon /send semantics with defer-and-drain: a send
landing in a command window is answered {status: queued, msg_id}
immediately and dispatched full-fidelity by a per-workstream drain
thread when the window closes. Parking encoded client disconnect as
message retraction — true only for the composer's ✕-abort; every
bounded caller (coordinator client and console proxy at timeout=30,
SDKs, stock proxies) timed out and lost its message for the whole
window, and the compensating client machinery was racy (one-shot
sendAbortMs sample) and over-broad (_sendAbort fired on the
interjection path, dispatching dismissed messages while showing a
connection error). Dismissal is now uniformly bind() → DELETE, with a
fall-through that retracts pending entries; retracting an
attachment-bearing deferred send surfaces the discarded-attachments
consequence. The drain claims entries under ws._lock immediately
before dispatch (DELETE can never remove an in-flight message),
refuses the truncating interjection fallback for oversized or
attachment entries atomically inside the enqueue callback, and never
gives up while the workstream lives; durability is documented as
node-local at-most-once. sendAbortMs, _sendAbort, the 600s bound and
the park loop are deleted; route and drain share one dispatch
implementation (spawn metrics included).
Also: the initial-send completion notify is un-gated from slot
ownership (_fire_notify_targets has exactly one call site — successor
turns never notify, so the round-5 guard prevented a duplicate that
cannot exist while converting force-cancel into permanent notification
loss for scheduled workstreams); /compact on an ERROR workstream
restores the badge instead of stamping idle over it; duck-typed
SessionUIs without on_compaction get the classic on_info lines back
via a shared renderer (superseded OK ends included — a committed swap
must never be silent; pre-1.8 SSE clients are deliberately not
dual-emitted, documented as a 1.8 breaking change); failed-end notice
suppression is computed once by the emitter as a notice bool on the
end event (SDK py+ts), replacing the hand-synced cli/JS policy while
the panes keep their pane-local card-ownership clause.
|
||
|
Patrick Buckley
|
1dbf7f410c |
feat(compaction): lifecycle events, web progress card, history re-render
Compaction becomes visible: a first-class 'compaction' SSE lifecycle (start/progress/end, compaction_id-correlated, superseded-flagged ends) replaces the loose info lines; both web panes render a progress-bar card that settles into a persistent result card, re-rendered after reload via the /history projection of the compaction marker row. Slash commands echo as command chips instead of fake user turns. The enabling rework: /command dispatches onto the workstream worker slot (the old inline path blocked the node's event loop for whole compactions and let /clear interleave with live turns). Busy refusals answer 409; quick commands are awaited loop-natively with a 60s backstop; /compact is fire-and-forget. Sends during a command window park in the /send route and dispatch full-fidelity afterwards — the interjection queue (length cap, cross-user guard, identity-swap hazards) is unreachable there — with a compaction-aware client abort bound shared by both panes. compact_now() carries send()'s full generation discipline; Stop aborts the in-flight summary HTTP stream via a generation-scoped cancel ref; force-abandoned compactions retire at their next checkpoint and their stragglers are fenced off every surface (panes, pill latch, CLI). Every session retry backoff is cancel-aware via one shared helper. Docs, OpenAPI spec, and both SDKs updated. Verified: 9457-test non-live suite, JS pin suites, headless-Chrome reducer harness; five unprimed multi-agent review rounds (correctness trend 15/6/6/4/4) with plan-level design passes on every fix round. |
||
|
Sanjay Santhanam
|
82676080a4 |
fix(session): describe skill selections accurately
Use neutral "set" wording for operator skill markers so re-selecting the current skill does not falsely claim a change. Update the regression expectation for the persisted marker. |
||
|
Sanjay Santhanam
|
a64cd25807 |
fix(session): record operator skill changes
Operator-driven /skill changes were only shown in the UI, leaving no trajectory marker for the model. Persist a system turn for named skill changes and clears, with regression coverage for both paths. |
||
|
renovate[bot]
|
8240d2c00d | chore(deps): update helm release postgresql to ~18.8.0 | ||
|
renovate[bot]
|
8f8c2f4ca3 | chore(deps): update github actions | ||
|
renovate[bot]
|
7b1f77dda7 | chore(deps): lock file maintenance | ||
|
renovate[bot]
|
72229cac26 | chore(deps): update ghcr.io/astral-sh/uv docker tag to v0.11.29 | ||
|
renovate[bot]
|
09c5475b0e | chore(deps): update actions/setup-node action to v7 | ||
|
Patrick Buckley
|
0e6a99e0f1 | chore: bump version to 1.8.0a2 v1.8.0a2 | ||
|
Patrick Buckley
|
84577ee530 |
fix(mcp): PR #844 review — correct success docstring, back out the dead admin pill
Copilot review feedback (all three valid): - _record_refresh_success docstring still claimed the push-driven single-kind refresh calls it — round 8 deliberately stopped that (a single kind can't declare a server-scoped 'ok'). Docstring now states the full-pass-only contract and points at the push path's _record closure for why. - The admin refresh pill's skipped-tint logic (admin.js) and its .mcp-refresh-pill-skip CSS were dead code: /v1/api/_internal/mcp-status strips last_refresh_at/last_refresh_outcome via the read-scope projection, so admin.js never sets newestRefreshAt and the pill block never runs. Backed both out; the whole pill fix (whitelist the fields with a read-scope-coarsened outcome, THEN the color logic + CSS) now lives in #843. The CHANGELOG's false 'the admin console's refresh pill paints…' claim is dropped — the /mcp refresh CLI and the 202-skipped endpoint (which read last_refresh_outcome directly, not via the strip) still work and remain documented. The 5 github-code-quality 'statement has no effect' comments are the known PR #840 false-positive class (the scanner reads 'await <name>' as a valueless expression); each flagged await is load-bearing (drains a parked runner so the next assertion is non-vacuous, delivers a cancellation, or awaits a _noop to fabricate a done owner_task) — no code change. Refs #839, #843 |
||
|
Patrick Buckley
|
80e7b9e9ca |
fix(mcp): review round 8 — push-success can't declare health, first-notify never debounced
- A single-kind push SUCCESS no longer clears the server error pill or stamps 'ok': _last_error / _last_refresh are server-scoped but a push refreshes only ONE kind, so a tools-failing server must not go green because its prompts push succeeded (a wrong-healthy window, bounded by the health tick — but a real 200-OK lie). Only a full pass declares 'ok'; the failure's armed health-tick retry runs it. This reverts the over-reach of round 7's push-success outcome write (a self-inflicted regression) — net simpler. - The (server, kind) debounce uses a None sentinel, not a 0.0 default: time.monotonic() counts from boot, so on a node whose process started < _NOTIFICATION_DEBOUNCE (5s) after boot, the 0.0 compare would debounce the VERY FIRST push — dropped with no recovery on the pool path. Absent stamp = never refreshed = always admit. - _record_refresh_skipped completes the outcome-helper set: the three inline 'skipped' stamps now share one config-gated helper (with _record_refresh_success / _record_refresh_failure), and the reconnect-success branch routes through _record_refresh_success — no more hand-copied gates to drift. - The per-message refreshers dict + on_debounce_drop closure are built ONCE per handler (both static and pool), not on every server->client message before the isinstance/debounce/coalesce early-returns. Accepted (documented): an operator /mcp refresh that finds the connect lock busy skips + arms the retry rather than waiting (waiting re-introduces the refresh-budget exhaustion busy-skip exists to prevent). 4 findings refuted. Suite 9408 green. Refs #839 |
||
|
Patrick Buckley
|
52dcb6a47b |
fix(mcp): review round 7 — consolidate the refresh-outcome write path
All three round-7 findings shared one root cause: last_refresh_outcome (the single source of truth for the CLI / endpoint / admin pill) was written inconsistently — ungated writes scattered across _refresh_server and _refresh_all, never written by the push path, never popped on removal. Consolidate every static outcome write through two config-gated helpers so the invariant holds: _last_refresh[name] exists IFF the server is configured and has a real outcome. - _record_refresh_failure now stamps the (config-gated) error:<Class> outcome; _record_refresh_success is its twin (gated ok stamp + pill clear). The ungated writes inside _refresh_server (both the internal error write and the success write) and _refresh_all's except are removed — routed through the helpers. A failure observed for a just-removed server no longer leaves a permanent stale error: row. - The push-driven refresh path (_run_static_notification_refresh._record) now records the outcome on BOTH success and failure, not just the error pill — a green 'ok' outcome no longer persists under a red error row after a push fails, and a successful push clears a prior error. - remove_server_sync pops _last_refresh (via _clear_static_push_state markers=True); a session drop KEEPS it (the outcome persists across a reconnect — only removal clears it). The removed-mid-pass branch drops any stale row too, so last_refresh_outcome doesn't report a departed server's prior 'ok'. _reap_bounded's pending-task concern was reviewed and REFUTED (a pending child on external cancel during shutdown is correctly left to loop teardown). Declined the per-notification refreshers-dict allocation cleanup: trivial (a 3-entry dict on a rare debounced path), and the late binding is deliberate for test overrides + mypy attribute checks. Tests: push-refresh success/failure write the outcome, removal pops it, session drop keeps it, failure for a removed server leaves no stale row; the 3 TestLastRefreshTracking tests updated to the split contract (_refresh_server propagates, the caller records). Suite 9407 green. Refs #839 |
||
|
Patrick Buckley
|
86aeb43120 |
fix(mcp): review round 6 — close the refresh-outcome reporting residuals
Three residual gaps in the round-5 skip-outcome threading, all in _refresh_all's other reconnect branches plus the endpoint ordering: - The disconnected-server reconnect DEFERRAL (_ensure_static_connected returns None: a sibling call in flight on the old stack, lock not held) returned None without stamping 'skipped', so the endpoint and pill read the STALE prior 'ok' and reported a never-run refresh as current. Now stamps 'skipped' like every other skip branch. - A server removed from config between the top-of-loop session check and the cfg lookup fell through to with results[name] UNSET, omitting it from the returned dict — an operator refreshing that one server saw a bare 'refresh complete' with no line. Now reports None so it renders. - internal_mcp_refresh_one checked 'skipped' BEFORE the error pill, so a skip on a server carrying a live error returned a benign 202 instead of 500 — a status-code-keyed caller would treat an erroring server as healthy-but-busy. Error is now checked first. - _reap_bounded swallowed an external CancelledError (shutdown / an operator cancel of the refresh runner) — it now re-raises after a best-effort exception retrieval, honouring the cancel. Dropped the unneeded asyncio.shield in the process. Tests: deferral stamps skipped, removed-mid-pass reported not omitted, endpoint error-beats-skip → 500, reap re-raises external cancel. Suite 9403 green. Refs #839 |
||
|
Patrick Buckley
|
748f670fe8 |
fix(mcp): review round 5 — thread the refresh outcome to every operator surface
The 'skipped'/None refresh sentinel added in round 4 was only half
threaded: consumers still misreported it. Unify all operator surfaces
on ONE source of truth — the per-server last_refresh_outcome ('ok' /
'skipped' / 'error:<Class>') — exposed via a new last_refresh_outcome()
accessor:
- _refresh_all returns None (not ([], [])) for a FAILURE too, so a
failed refresh is never rendered as 'no changes' (the pre-#839 lie
the sentinel exists to close); None is disambiguated skipped-vs-failed
by the outcome. ([], []) now strictly means 'ran, no changes'.
- /mcp refresh renders skip ('skipped — retry scheduled') and failure
('refresh failed (error:X)') distinctly from 'no changes'.
- The node-internal refresh endpoint returns 202 'skipped' instead of a
misleading 200 'ok' for a refresh that never ran (the busy-lock skip);
it reads the outcome from the manager accessor because the public
status projection deliberately whitelists last_refresh_outcome out.
- admin.js paints 'skipped' with a neutral info pill
(.mcp-refresh-pill-skip), not the error-red any-non-'ok' used to get.
- _admit_list_changed rolls back BOTH the coalesce marker and the
debounce stamp when scheduling raises, so a same-kind push in the
window afterward isn't debounced against a refresh that never spawned
(the pool path has no on_debounce_drop recovery).
Tests: endpoint 202-skip, CLI skip/failure render, _refresh_all
failure→None + outcome, spawn-failure stamp+marker rollback. Suite
9399 green.
NOTE filed #843: the admin refresh pill's data (last_refresh_at/outcome)
is stripped by BOTH status projections and never reaches admin.js — a
pre-existing latent bug (the pill has never rendered); the admin.js
color fix here is correct-when-reachable. Out of #839 scope (the read
projection strips it for a privacy reason that needs its own coarsening
decision).
Refs #839
|
||
|
Patrick Buckley
|
1a80466369 |
fix(mcp): review round 4 — removal/reconcile lifecycle, honest skip reporting, same-kind debounce recovery
reconcile_sync no longer abandons a DB-driven removal that timed out: both the removal loop and the config-update loop keep the name in _db_managed (and skip the follow-on add) when remove_server_sync returns its mutated-nothing False, so the next pass retries instead of the deleted/reconfigured server serving stale tools until restart. remove_server_sync is now cancel-safe end to end: it FORCE-drops the session before queueing (parked push runners bail at their session gate instead of serializing ≤30s list calls ahead of the removal — the noisy #839 server was exactly the one whose runners could starve its own removal), and wraps the post-lock cleanup in try/finally so a caller-timeout cancel landing mid-teardown still completes the state pop, catalog rebuild, and lock retirement rather than stranding a config-gone ghost catalog. Config survives a park-cancel, so the health loop recovers it. _refresh_all reports None (not a fake ([], [])) for a busy-skip or supersede, stamps a 'skipped' status row, and /mcp refresh renders it distinctly — the operator is no longer told a never-refreshed server is current. A same-kind push lost to the debounce window (the prior runner already finished; the server won't re-announce) arms the health-tick retry, closing the one staleness hole the per-kind debounce still had; a push covered by a queued runner does not arm (no lost change). Static resource/prompt catalogs are capped at connect discovery and every refresh. _list_resource_pair's reap is bounded so a future SDK cancel-regression can't wedge the lock. Cleanups: _arm_refresh_retry (retry-arm gate, ×3), _spawn_full_refresh (discard+spawn, ×3), _popen_mcp_server (live-server spawn, ×2), the tautological stamp-arithmetic TestNotificationDebounce deleted. Suite 9395 green. Refs #839 |
||
|
Patrick Buckley
|
53f11454ad |
fix(mcp): review round 3 — cap static catalogs, atomic removal, unify the list_changed protocol twins
- Static resource/prompt catalogs are now size-capped at connect
discovery AND on every refresh (mirrors the pool twins and the static
tools path): a misbehaving server's push ran uncapped through the new
spawned refresh path and could balloon the shared node's merged
catalogs on every notification.
- remove_server_sync mutates NOTHING outside the per-name lock: the
up-front config pop meant a removal cancelled while parked (behind
the push-refresh runners that now share this lock) left a
half-removed server — config gone, session and published catalogs
alive, no driver able to reconnect or cleanly re-remove. A timed-out
removal is now honestly retryable.
- _refresh_all's DISCONNECTED branch busy-skips too (parking inside
_ensure_static_connected burned the pass's 30s budget on one
mid-reconnect server), and a busy-skip on either branch ARMS the
health-tick retry — an operator-requested refresh can no longer be
silently dropped with output indistinguishable from 'no changes'.
- reconnect_sync drops the session before queueing on the lock (FORCE
semantics already rebuilt live sessions): parked push runners bail
at their session gate instead of serializing up to one 30s list call
per kind ahead of the operator's recovery action. Residual: one
mid-list holder can still precede the 45s attempt; a timed-out
reconnect is honest and retryable.
- _refresh_server's supersede check gains the session arm: a spawned
retry/post-reconnect pass racing an eviction skipped instead of
manufacturing a false 'not connected' error pill (and a re-arm loop)
for a self-healing condition.
- The list_changed protocol twins are UNIFIED (Closes #842): the
admission half (_admit_list_changed) and the runner half
(_run_list_changed_refresh) each exist once as plain parametrized
methods — values and small closures, no factory layer (mcp v2 drops
the factory pattern; the two thin message_handler closures remain
only as SDK-v1 bindings). The one true asymmetry — coalesce-marker
ownership on the superseded path — is a documented boolean: pool
markers are only ever cleared by their runner; static markers are
cleared by remove_server_sync, so a present marker belongs to the
re-added generation. Both runners keep their names and signatures;
the notification suites pass unchanged.
- Cleanups: per-kind staleness rechecks stripped from the static
refreshers (unreachable under the lock discipline — the MUST-hold-
lock contract is documented instead); _run_hl (5th run-on-loop copy)
replaced at 44 call sites; _poll_until centralizes the live-test
wait loops; docs no longer describe the periodic refresh tier
removed in
|
||
|
Patrick Buckley
|
f8f191686f |
fix(mcp): review round 2 — busy-skip the refresh pass, fail-fast list pairs, health-tick refresh retry
- _refresh_server never parks on a held connect lock: the holder is itself a catalog publisher whose publish supersedes the pass, and parking burned refresh_sync's whole 30s budget on ONE busy server (a reconnect attempt holds the lock up to 45s), failing the operator pass for every healthy server queued behind it. Busy → skip (None), no publish, no status writes; the identity/state recheck stays as belt-and-braces for the one-tick check→acquire race. - _list_resource_pair: the ONE copy of the paired resources/templates list protocol (both twins). Fail-fast — a fast real error (auth / method rejection) surfaces as ITSELF instead of being masked behind a hung sibling's eventual 30s TimeoutError — with the survivor CANCELLED and REAPED inside the timeout scope, never left detached on the shared session. - Health-tick refresh retry: there is NO periodic refresh pass (removed in eb2a119d; the docs still claimed the 4h tier — fixed), so a push refresh that failed while the transport stayed up had no automatic recovery and the shared catalog stayed stale for every user until an operator intervened. Failures and busy-skips arm _static_refresh_retry via the shared recorder; the health tick drains it with one bounded, lock-serialized full pass per tick; success, session drops, removal, and the post-reconnect spawns clear it. This also un-latches the error pill: the retry's completion clears it within a tick. - _record_refresh_failure: the bearer-redaction policy (type + message, never exc_info) lives exactly once; all three refresh-failure sites route through it. - Static runner discards its coalesce marker only AFTER the lock-identity check: on the superseded path a marker present in the set belongs to the re-added generation's parked runner, and discarding it would mint duplicates past the one-parked-runner bound (the pool runner deliberately differs — nothing else clears pool markers, so its marker is its own to release). - _clear_static_push_state: the ONE (server, kind) keyspace walk for stamps + retry flag (+ markers on removal). - Tests: busy-skip, superseded-no-status, fail-fast + reap (<5s bound), retry arm/drain/re-arm/clear quartet, logged-wrapper contract updated to the shared recorder's arg shape; vacuous stamp-math test deleted (behavioral per-kind coverage retained); _free_port/_wait_tcp_ready/_wait_session_live hoisted to conftest for both live tests. Refs #839 |
||
|
Patrick Buckley
|
aefcf53405 |
fix(mcp): review round 1 — supersede retired-lock refreshes, per-kind debounce, complete gather pairs
- _refresh_server: post-acquire lock-identity + state-existence recheck; a pass superseded by remove (or remove + re-add) returns None and writes NO status — it must not run its list calls as a second, unserialized publisher against the re-add's discovery wiring, resurrect status rows for a removed server, or stamp a false "ok" over a generation it never refreshed. _refresh_all treats None as a deliberate skip (no breaker success record). - Debounce stamps are per (server, kind) on BOTH paths: refreshes are kind-scoped, so a server-scoped stamp dropped a different-kind notification inside the window outright — a tools push swallowed the prompts push 100ms behind it, and nothing observed the prompt change until the server pushed that kind again. Teardown pops loop the kinds; remove_server_sync also discards the server's coalesce markers so a parked old-generation runner's marker cannot coalesce away a re-added server's first push. - Resource refreshers (static + pool) gather with return_exceptions=True: fail-fast gather left the surviving list call running detached — outside the timeout scope and the lock serialization — as an unbounded in-flight request on the shared session. - Spawned post-reconnect refreshes route through _refresh_server_logged: the re-raise escaped into _spawn_background's done-callback, whose exc_info log serializes the chained httpx.Request carrying the configured bearer for auth_type=static servers; _refresh_all's except drops exc_info for the same reason. Failure diagnostics widen to "Type: message" in logs and the error pill — the message text is header-free; only the serialized chain leaks. - Accepted + documented: connect-lock contention on dispatch reconnects is bounded to one in-flight list call (parked runners bail instantly post-eviction); the error pill persists until the next COMPLETED refresh (a notification's arrival proves nothing about whether the failure resolved). - Tests: per-kind debounce independence, superseded-pass writes nothing, gather-sibling completion, logged-wrapper swallow with the exc_info channel asserted SILENT, remove clears markers; _run_on_loop/_drain_background hoisted to conftest (4 drifted copies); proc.kill() portability in the live push test. Runner-twin dedup (static/pool protocol duplication) deferred to #842. Refs #839 |
||
|
Patrick Buckley
|
37144991c9 |
fix(mcp): spawn static list_changed refreshes off the receive loop
The static-path notification handler awaited its catalog refresh inline in the SDK's receive loop, but the refresh issues a request on the same session — a request whose response only that (now parked) loop could route. The refresh never completed, and every user's calls on the shared per-node session stalled behind it, unbounded, until the health loop's ping timeout tore the transport down — which was also the only way a pushed catalog change ever landed. Port of the pool-path protocol (#836) onto the static primitives: - Refreshes are debounce-gated, coalesced per (server, kind), and spawned as tracked tasks; the runner serializes on the per-name connect lock so a refresh, a connect's discovery wiring, and the manual/periodic _refresh_server pass can never publish out of order (the remove -> re-add race is closed by lock identity, the static twin of the pool's entry-identity check). - The coalesce marker is cleared at lock-acquire so a change the in-flight list missed spawns exactly one successor; the finally discard is gated on non-acquisition so it never clobbers that successor's marker. - The debounce stamp survives a failed refresh (throttle over lost window) and every teardown/eviction path now pops it via the paired _drop_static_session_and_stamp, so a reconnected transport's first notification refreshes immediately. - All three static list calls are bounded by _CONNECT_TIMEOUT and discard their result if the state entry was replaced mid-flight; the resource pair rides one gather (mirrors the pool sibling). - Failure logging is (Exception, BaseExceptionGroup) type-name-only: an escaping group reaches _spawn_background's exc_info log, which serializes the chained httpx request carrying the configured bearer for auth_type=static servers; the recorded operator error string is type-name-only for the same reason. Non-list-changed notifications no longer clear the server's error pill (that pop was accidental — only a completed refresh proves anything). Includes a live end-to-end repro (FastMCP subprocess pushing tools/list_changed through a real receive loop): pre-fix the triggering call itself deadlocks (verified against main), post-fix it completes with the catalog landing on the original session, no teardown. Closes #839 |
||
|
Patrick Buckley
|
b2f53d329b |
chore(ci): drop review-event triggers from claude.yml
Bot PR reviews (Copilot, code-quality) fired pull_request_review and pull_request_review_comment runs that always gate out but pile up as awaiting-approval clutter. @claude stays invocable via issue and PR conversation comments, the only path actually used. |
||
|
Patrick Buckley
|
6f991d6aff | chore: bump version to 1.8.0a1 v1.8.0a1 | ||
|
Patrick Buckley
|
7d4d76e097 |
fix(providers): PR review — orphan deltas arm the finish shim, test style
- Orphan argument deltas count as delivered output for the finish_reason_optional shim, exactly as they count as a streamed signal for the terminal harvest: a lax Responses server that never announces items AND never sends a terminal event still delivered its tool call — with the tolerance declared that is a completion, not an IncompleteStreamError. (Review caught the shim/harvest inconsistency the round-9 fix introduced.) - Test style: single import style for the model_turn module, assert on a local instead of a call expression, drop a pass-through lambda. |
||
|
Patrick Buckley
|
747177a76c |
fix(providers): review round 9 — orphan/harvest collision, shared shim gate, retired-id rationale
Correctness: - Responses: orphan argument deltas (streamed without any output_item.added) now count as a streamed tool-call signal, so the terminal harvest stands down instead of re-emitting the same call onto the same slot — the reproduced collision concatenated the arguments JSON into an unparseable double copy. Cleanup / documentation: - finish_shim_due in _protocol is THE gate for the lax-server finish shim — one predicate (and one definition of 'delivered output') for all three adapter families, so the same capability flag cannot acquire per-family completion semantics. - The Responses error/response.failed branches share one failure tail (only code/message extraction differs) — the same server failure can never become retryable through one event type and fatal through the other, pre- or post-terminal. - _format_refusal pins the refusal rendering the streamed event and the terminal harvest both use. - The capability-table floor comment and CHANGELOG Removed entry now state the real rationale: OpenAI has RETIRED the pruned ids from the API — the rows described unreachable contracts, not unpopular ones. - CHANGELOG names the stream-entitlement break class (verified-org streaming, pre-stream_options gateway api-versions) with its serving-side remediation; deliberately no non-streaming fallback. - docs/architecture.md retry section describes the collapsed transport: the two stacked retry ladders, IncompleteStreamError / ResponsesStreamFailedError retryability, finish_reason_optional remediation; stale non-streaming mentions updated (+ puml). - Anthropic whole-block emission carries its residual hybrid-gateway bet as an explicit comment. Held on standing rulings: post-finish usage forfeiture (keep result + warn, rounds 4/8), session merge_usage twin and StreamAbortRef twin (#832), stream_options wire delta (round 2, caveat now names Azure). |
||
|
Patrick Buckley
|
49d33d8594 |
fix(providers): review round 8 — under-streaming gateway parity, abort-race close, usage-blip visibility
Correctness: - Responses: output that exists ONLY in the terminal payload (buffering gateways that never fire output_text.delta / output_item.added) now reaches CompletionResult.content and tool_calls — the retired non-streaming _parse_response read this same payload, so the drain must too instead of returning a clean-looking empty success (blank compaction summary, silently-skipped tool call). Gated on nothing of that kind having streamed; refusal parts render as the streaming branch does. - Anthropic: content pre-populated inside content_block_start (whole- block lax-gateway emission — the real API sends start blocks empty) is emitted for text, thinking, and tool_use input, type-guarded like _reasoning_text so duck-typed blocks can't leak non-strings. - Responses: a response.completed payload that OMITS status maps to "stop" via the event type, matching the payload-less branch — the empty-string status read as 'length' and fired truncation policies on complete output. - model_turn: the drain-retry loop re-checks cancel_ref.aborted after the backoff sleep — an abort landing mid-sleep now kills the abandoned worker with the original failure instead of issuing one more full request behind the deadline's back. - drain_stream: the post-finish transport-blip tolerance logs a warning naming whether usage was captured — the kept result may report usage=None (chat-lane usage trails the finish reason) and that spend was vanishing from usage accounting with no signal. Cleanup: - ChatSession's inline tool-call fold adopts accumulate_tool_call_delta (drop-in — same ToolCallDelta semantics), so THE merge rule now has one implementation across the chat loop, drain_stream, and the Google capture; the helper's mirror-mandate docstring is retired. - The task-agent _api_call contract comment reconciles the two retry layers (sub-harness owns request-level policy; model_turn owns drain-time re-issue) instead of claiming model_turn is policy-free. - Anthropic's three terminal-emission sites share one _attach_terminal_blocks helper — replay fidelity can't depend on which terminal path a stream took. - Responses create_streaming resolves capabilities once. Held on standing rulings: o-series capability-row removal (4th report; deliberate break, release-noted), StreamAbortRef/_CancelRef unification (#832; docstring mirror-mandate). |
||
|
Patrick Buckley
|
8fa0e7a29e |
fix(providers): review round 7 — id-disciplined slots, all-lane finish tolerance, retry backoff
Correctness:
- ToolCallSlotter: a slot whose id is KNOWN never splits on an id-less
delta — on an id-disciplined server new calls arrive with ids, so an
id-less fragment (the call's FIRST name announcement included) is
always a continuation. Round-6 regression: {id} → {name} → {args}
emission split into an unnamed id-bearing call plus a nameless twin.
Also: a name arriving for a slot with no name yet never splits
(args-first emission), and a bare same-name delta after complete
arguments merges as a redundant footer instead of minting a phantom
zero-argument call that would re-run a side-effecting tool.
- finish_reason_optional is honored on every drained lane, not just
Chat Completions: Anthropic shims a missing message_delta
stop_reason + message_stop pair, Responses a missing terminal event
(both with collected blocks riding the shimmed finish) — the
documented capabilities-JSON remediation now works on the
anthropic-compatible/responses-compat gateways it was written for,
matching the retired non-streaming paths' tolerance.
- Responses: an in-band error/response.failed frame arriving AFTER the
terminal event is teardown noise — log and end the stream instead of
raising away a generation already in hand (the in-band twin of
drain_stream's post-finish transport-blip tolerance).
- model_turn drain retries pace like the SDK request retry they
replace: 0.5s base, doubling, ±50% jitter — instant re-issues
re-hit the still-active rate limit/overload and synchronize into
fleet-scale retry bursts.
- Responses slot bookkeeping survives lax servers: slots minted by a
counter (len(dict) collided calls after a duplicate/empty item-id
overwrite), orphan argument deltas route to the most recently
announced call instead of hardwired slot 0.
Cleanup:
- on_tool_call_delta now receives the normalized ToolCallDelta plus the
raw SDK delta — Google's capture accumulates the exact bytes the
mirror sees (the byte-identical extraction no longer exists twice).
- _ArgsScanner feeds only fully id-less slots (its verdict is never
consulted for id'd slots — dominant-case hot path).
- Anthropic retryable set hoisted to a class constant (per-access
frozenset allocation, same pattern already fixed on Responses).
- GoogleProvider class docstring names the hook-based capture instead
of the deleted _extract_tool_calls override.
|
||
|
Patrick Buckley
|
16647db1b0 |
fix(providers): review round 6 — strict-by-default finish gate, slotter v3, drain retry
Correctness: - The chat-lane finish shim is now armed only by an operator-declared finish_reason_optional capability (model-definition capabilities JSON). Default lanes treat a clean finish-less end as died-mid-generation (retryable) — SSE cannot distinguish lax-server completion from a worker dying behind a clean-closing proxy, and the default must catch truncation rather than bless it. When armed, reasoning-only output counts as a completed generation (parity with the retired non-streaming path's finish_reason-or-stop default). - ToolCallSlotter v3: id-less call-boundary decisions now consult argument JSON completeness (incremental scanner) and name identity instead of a boolean has-args gate. Fixes both residual id-less ambiguities: two zero-argument whole-delta parallel calls no longer fuse (silently dropping an action), and redundant per-fragment name headers no longer split one call into malformed half-JSON calls. - model_turn re-issues transient mid-stream deaths (provider's retryable_error_names, raised while draining) up to twice — the new home of the SDK request-level retry the non-streaming transport gave every single-shot lane (judge, title, perception, compaction). Request-time failures keep the SDK's own policy; an aborted cancel_ref suppresses re-issue (StreamAbortRef gains .aborted). Cleanup: - One slotter drives both the normalized mirror and Google's raw fidelity capture via an on_tool_call_delta hook — raw/mirror slot parity is structural now, not a maintained invariant. - accumulate_tool_call_delta in _protocol.py is THE tool-call merge rule; drain_stream and the Google capture use it (session's copy is #832's tracked adoption). - Responses terminal rebuild only runs when the terminal payload can disagree with the .done-collected items (truncation or count mismatch); on rebuild, annotations are replaced, not re-extended. - Responses retryable set precomputed at class creation. Held on standing rulings: o-series capability-row removal (deliberate, release-noted with remediation), StreamAbortRef/_CancelRef unification (#832; docstrings mandate mirroring until then). |
||
|
Patrick Buckley
|
91c46051d9 |
feat(providers): drop o-series and pre-5.4 GPT-5 capability rows
The OpenAI commercial capability table floor is now gpt-5.4: o1, o1-mini, o3, o3-mini, o3-pro, o4-mini, gpt-5, gpt-5-mini, gpt-5-nano, gpt-5-pro, gpt-5.1, gpt-5.1-codex-max, gpt-5.2, gpt-5.2-pro, and gpt-5.3 are effectively unused in the field. The gpt-5-search-api row (different product surface) and the audio/STT/TTS rows stay. A legacy id now resolves to OPENAI_DEFAULT (temperature sent, no declared effort vocabulary, 200K window) — which those models may reject; the remediation is the model definition's capabilities JSON or a current model, release-noted under Unreleased → Removed. This also retires the transport-collapse review's thrice-reported "stream-rejecting o1-era models are stranded" finding by removing its subject: no row in the table describes a non-streaming model anymore. Tests migrate to 5.4-era equivalents that pin the same behaviors: always-reasoning temperature suppression and off-list effort snap (gpt-5.4-pro for gpt-5-pro/o3), explicit-none forwarding (gpt-5.4 for gpt-5.1), empty-effort-vocabulary knob drop (gpt-5-search-api for o1-mini), and the longest-prefix shadow hazard (gpt-5.4-pro vs gpt-5.4 for codex-max vs gpt-5.1). |
||
|
Patrick Buckley
|
3a28dc2f16 |
fix(providers): review round 5 — same-id fragment merge, post-finish blip tolerance, chat finish shim
Correctness: - ToolCallSlotter's reannounce split is gated to ID-LESS deltas: id equality proves the same call, so compat servers that repeat the id+name header on every argument fragment merge back into one call with valid JSON (round 4's ungated heuristic split them into duplicate half-JSON calls — execution-confirmed by the review). The residual id-less repeat-name-per-fragment shape is documented as inherently ambiguous; ids are the only disambiguator. - drain_stream keeps a completed result when the transport blips AFTER the finish reason (trailing usage chunk / citation footer window): the generation is in hand, so forfeit the trailing metadata instead of discarding a fully-delivered verdict or re-paying a compaction. - The chat iterator shims finish_reason="stop" when a stream ends CLEANLY after delivering content or tool calls — the deleted non-streaming `or "stop"` default for lax finish-reason-less servers, now safe to restore because abrupt deaths surface as httpx.TransportError (round 4) rather than clean exhaustion. This supersedes the round-3 keep-the-gate ruling: the httpx catch changed the calculus, and the Anthropic/Responses lanes already got their marker-based shims. Empty/reasoning-only streams still fail the complete-or-error gate. Two streaming tests gained the shim chunk. Dispositions held: o1-era stream-rejecting models (third re-report) stay a release-note remediation per the earlier ruling. Cleanup: the two Responses terminal branches collapse into one path (status derived from the event type when the payload is missing — also fixes the end-of-stream debug log reporting finish_reason=None for completed lax streams); the annotations walk is one shared helper (the two copies had already diverged on None-content guarding); _raise_responses_failure is annotated NoReturn; scripts/livepass.py drops the phantom supports_streaming key; test_model_registry's capture helpers ride scripted_chat_client; _openai_stream_chunk points at its fake_chat_stream shape-twin for future consolidation. |
||
|
Patrick Buckley
|
cf7cfe8932 |
fix(providers): review round 4 — wire-error retryability, tap/mirror slot parity, terminal completeness
Correctness: - drain_stream chains raw httpx.TransportError from stream iteration into retryable IncompleteStreamError (original type+message preserved via __cause__): streaming moved the body read out of the SDK's APIConnectionError-wrapped request, so mid-body connection drops and read timeouts — retried transparently on 1.7 — were escaping every single-shot retry loop as instantly-fatal raw httpx names. - The index remap is extracted as ToolCallSlotter and GoogleProvider's raw tap slots THROUGH IT over the same delta sequence as the base iterator: round 3's mirror-side de-fusion had left the tap keying by wire index, so a degenerate stream produced 2 mirror calls vs 1 fused raw dict — _prepare_messages' length gate then silently dropped the thought_signature lane (400 on signature-strict Gemini models). - The slotter also splits ID-LESS degenerate parallel calls: a delta announcing a name for a slot that already accumulated arguments is a second whole call, not a fragment (fragmented single calls pinned unaffected). - A payload-less Responses terminal event keeps the provider_blocks already collected from output_item.done events (they came from the stream, not the missing payload); only usage is genuinely lost. - The truncation-rebuild path walks the terminal output's message annotations, so truncated web-search turns keep their Sources footer (the in-flight item never received output_item.done). Cleanup: one _raise_responses_failure ladder serves both in-band failure shapes (error events + response.failed); IncompleteStreamError joins the public providers export (docstrings tell callers to catch it); the de-fusion tests ride the file's existing _openai_stream_chunk helpers instead of a third hand-rolled SSE fake; the dead if-response guard in the terminal branch is gone. Deferred with note: classifying IncompleteStreamError once at the retry-predicate consultation site instead of per-provider strings is #832 territory (the predicate lives in ChatSession); the six-lane parametrized test guards the listing until then. |
||
|
Patrick Buckley
|
56b7674dfa |
fix(providers): review round 3 — in-band error events, terminal-marker tolerance, adapter-owned de-fusion
Correctness: - Responses _iter_stream handles the SDK's in-band `error` SSE event (ResponseErrorEvent is YIELDED, not raised, and no response.failed need follow): the real API code/message now surfaces — code-gated for retryability like response.failed — instead of the stream exhausting finish-less and hiding the cause behind a retried IncompleteStreamError. - Anthropic message_stop supplies a missing stop_reason: it is a genuine terminal marker, so a compat /v1/messages shim whose message_delta omits stop_reason completes (blocks intact) rather than failing a generation that arrived — tolerance the retired non-streaming default provided, restored without weakening the died-mid-response gate. - A Responses terminal event without its response payload still emits the finish reason its type implies (lax compat servers), losing only usage/blocks rather than the whole result. Dispositions held (documented, not re-coded): the complete-or-error gate stays for finish-less Chat Completions streams — indistinguishable in-band from a died generation, and silent partial-storage is the worse failure; CHANGELOG now names the shape and each provider's accepted terminal markers. supports_streaming deletion and the stream_options wire delta were ruled earlier and keep their release-note remediations. Cleanup: index-degenerate de-fusion MOVED from drain_stream into the chat adapter's iterator (mirroring the Anthropic iterator's index assignment) so the interactive loop is fixed too and the drain returns to a plain mirror of the main-loop accumulator; a parametrized test locks "IncompleteStreamError is retryable" across all six provider lanes instead of trusting per-adapter memory; scripted_anthropic_client joins scripted_chat_client (shared _ScriptedClient class, no function attrs) and the two remaining hand-rolled anthropic closures convert. |
||
|
Patrick Buckley
|
3ffa8b9057 |
fix(providers): review round 2 — complete-or-error drain, code-gated retries, truncation-safe blocks
Correctness (3 confirmed + 2 plausible, all fixed): - drain_stream now raises typed, retryable IncompleteStreamError when a stream exhausts without any finish reason — every adapter emits one on a healthy stream, so its absence means the generation died mid-response behind a cleanly-closing proxy. This restores the retired transport's complete-or-error contract (a half-generated compaction summary was previously returned as finish=stop and stored, silently replacing real history) and DELETES round 1's suffix-info fold: with no finish-less success path there is nothing to classify, so a trailing status ping can never be stored as content either. - Index-degenerate parallel tool calls get distinct slots: a delta whose id differs from its slot's opens a new call (id-less fragments still follow their index's current call), so historical compat servers that emit every parallel call at index 0 no longer fuse distinct calls into concatenated garbage arguments. Result order stays index-sorted (stable) like the retired array parse. - response.failed retryability is code-gated: only transient codes (server_error, rate_limit_exceeded) raise the retryable typed error; deterministic rejections (invalid prompt, image fetch, policy) raise plain RuntimeError and stop retry loops on attempt zero instead of running the full backoff ladder against a doomed request. - Terminal Responses events rebuild provider_blocks from response.output when present: the item being generated at max_output_tokens truncation never receives output_item.done, and storing a reasoning item without its required following item made the next turn's replay a 400. - merge_usage's base case uses dataclasses.replace so a future UsageInfo field can't be silently zeroed on drained lanes. Cleanup: run_abortable_with_deadline bundles the three-point abort wiring (ref + cancel_ref + on_abandon) so it cannot be half-wired — both judges converted; scripted_chat_client hoists the 14 chat-lane fake_create closures (call scripts + .calls recording replace per-test counter cells); fake_chat_stream gains reasoning=, collapsing the reasoning-capture suite's hand-rolled chunk shape; FakeAnthropicBlock hoists the duplicated _Block test class; the class and judge PlantUML diagrams drop the retired create_completion flow. Also converts test_model_registry's agent-model fakes, which returned legacy response objects that iterated as EMPTY streams — they only passed through the old drain's silent finish=stop default, exactly the hazard the new gate exists to catch. |
||
|
Patrick Buckley
|
08580f25f9 |
fix(providers): review round 1 — streaming parity gaps the collapse exposed
Correctness (4 confirmed + 1 plausible fixed, 2 accepted+documented): - Anthropic _iter_anthropic_stream handles citations_delta: text-block citations now ride the raw block into provider_blocks, as replay requires (the retired non-streaming lane preserved them via model_dump; the streaming lane dropped them — a pre-existing main-loop gap the collapse would have extended to single-shot lanes). - Anthropic text blocks separate with "\n" at each subsequent block start, restoring the retired lane's "\n".join rendering on drained lanes AND un-fusing streamed web-search responses in the chat loop. - response.failed raises typed ResponsesStreamFailedError, listed in the provider's retryable_error_names — retry loops treat an in-band failure like the wire errors it stands in for instead of hard-stopping on a bare RuntimeError (judges keep their heuristic fallback after retries). - drain_stream folds a finish-less stream's terminal citations footer (suffix rule: pre-finish info invalidated by any later payload), so lax compat servers that never send finish_reason keep their Sources. - usage max-merge extracted as merge_usage() in _protocol.py — the one definition drain uses now and the session's inline consumer adopts on #832. Accepted + release-noted instead of coded around: strict pre-2024 compat servers that 400 on stream_options (such a server already cannot serve the chat loop; CHANGELOG caveat extended), and repeated-index parallel tool-call merging on legacy compat servers (identical to the main loop's accumulator semantics; a shared guard belongs in the #832 unification). Cleanup: run_with_deadline grows on_abandon (best-effort, cannot mask the deadline error) and both judges drop the copy-pasted abort choreography; StreamAbortRef documents the _CancelRef adoption plan; test_model_turn's fake replays through the shared as_stream adapter; docs/architecture.md drops the retired Protocol row. Tests: refusal handler pinned (was advertised, untested); typed-failed retryability; citations capture; text-block separator (plus the mixed text+search expectation updated for the separator chunk); finish-less citation fold; on_abandon firing matrix; StreamAbortRef arrival race. |
||
|
Patrick Buckley
|
1e7ad7bcb6 |
feat(providers): one transport — drain create_streaming, retire create_completion (#831)
Every single-shot lane (model_turn: judges, titles, compaction, web-fetch extraction, perception, eval, optimizer) now samples through the provider's streaming entry and accumulates via a shared drain_stream(), deleting create_completion from the Protocol and all three adapters (xai/google inherit). Request shaping can no longer drift between the two consumption styles, and callers keep the exact CompletionResult contract. The drain mirrors the main loop's proven chunk semantics: per-field max-merge for usage (Anthropic splits prompt/completion across message_start/message_delta), tool-call assembly by delta index, provider_blocks from the terminal emission, trailing citation info folded back into content (byte-matching the old format_citations append), mid-stream status pings dropped. Also in this change: - model_turn grows cancel_ref; both judges wire their run_with_deadline abandon paths to a new StreamAbortRef (deadline.py) that closes the SDK stream — a timed-out judge call now aborts its HTTP read instead of pinning a daemon thread until the next upstream chunk. The append hook covers the arrival race, mirroring ChatSession._CancelRef. - Responses streaming gains the response.incomplete terminal handler (truncated runs were mislabeled finish=stop and lost final usage AND collected provider_blocks) and a refusal handler ([Refused: …] content, matching the retired non-streaming rendering). Both also fix the main chat loop, which shared the gaps. - supports_streaming capability flag deleted (zero readers) along with its admin capability tile; o1-era models that reject streaming need a model alias pointing at a current model (release-noted). - Helpers that existed only for the deleted transport go with it: Responses._parse_response, chat/google._extract_tool_calls. Known behavioral deltas (release-noted): OpenAI-compatible servers that ignore stream_options.include_usage stop producing usage rows on these lanes; multiple Anthropic text blocks concatenate without the old "\n" joint (matching the main loop); model_turn lanes no longer risk client read-timeouts on long generations — the reason the Anthropic adapter already drained a stream internally. Tests: new test_drain_stream.py pins the accumulator rules; shared fakes (as_stream, fake_chat_stream, fake_anthropic_stream) migrate 11 suites to the streaming transport, with the task-agent and adapter suites now exercising the real _iter_stream + drain path end to end. |
||
|
Patrick Buckley
|
a66e9d456d |
fix(mcp): gate the marker release on non-acquisition; structure the paired protocols
Close the round-8 review findings: - The refresh runner's finally-discard releases the coalesce marker ONLY when the lock was never acquired (cancelled while parked). After the at-acquire discard, a marker present at exit belongs to the successor spawned during the in-flight list call — discarding it unconditionally let the handler mint one extra runner per debounce window while the lock was congested, reopening the unbounded runner FIFO the marker exists to bound. - The observe-before-lookup preamble lives once in _pool_lookup_checked (snapshot taken synchronously before the lookup await, render paired with the convergence drop) instead of verbatim in all three dispatchers — the ordering contract is now structural rather than comment discipline. - drop_session is paired with its debounce-stamp pop in _drop_session_and_stamp, shared by the eviction, teardown, and owner-death paths; the shutdown sweep clears the pool notification stamp dict and the coalesce marker set alongside the other pool state. - _mcp_tools_change_seq is initialized unconditionally for every session kind, so the attribute's existence no longer encodes whether an MCP client was wired at construction. |