A provider that registered a disable form but no enable form was told
its shape was unknown and pointed at register_provider -- work it had
already done -- for a request that only ever needed the disable form.
The old bool code took the slot by direction; the tiered rewrite lost
that. Take the relevant field again, and keep "shape unknown" for the
case where both halves are missing, so the "cannot disable" wording
still owns the half-missing case.
The entity pages carry the rejected alternatives and why, so the next
reader does not have to reconstruct the argument: minimal patch cannot
fill a tier that does not exist, cherry's wire DSL buys four endpoint
protocols we do not have, and default_effort lost its only consumer the
moment enable_thinking=True became Effort.AUTO.
Codex argued `effort_fallback` had no verifiable consumer and could wait.
The human overruled that: swapping models is routine, and between "it
stops booting" and "it silently costs more" the caller deserves a middle
option it selects on purpose. So the mapping ships with this round.
issue #20 asks for one zhipu profile. Adding it does not fix what the
issue describes: GLM-5.3 mandates thinking (three sources agree, the
vendor included), so `none` is an undefined value we were sending, and
`medium` — the tier our minimax profile hardcodes — does not exist on
GLM, kimi or deepseek at all.
So the gap is the type, not the table. Capability becomes a tier list
where `none`'s presence answers "can it be turned off", and refusal
carries the cheapest tier that model does support — a refusal with no
way forward is what sent the caller to extra_body in the first place.
Reviewed by Codex, which caught two claims that were wrong: source-level
extra_body and enable_thinking already reach the cache key through the
model fingerprint, and the three reference projects are not in the
workspace, so "no callers" was a grep against absent directories.
Both reviews landed on the same line independently. _as_role swaps the
credentials in the DSN with a regex, and when the pattern does not match
it returned the string unchanged. Two shapes miss it: no inline
credentials, and a unix socket URL. Either one is a legal DSN.
What that costs is not a broken test. The sandbox builds, every
assertion still passes, and bare_dsn is now the admin connection, so the
worst-case case runs the real script with --apply as a superuser against
the shared table. The verifier ran that command as a dry run to see what
it would have done: target public.llm_calls, 11 rows to delete. The case
would still have gone red on the exit code, after the rows were gone.
It raises now. There is also a second check that connects and compares
current_user, because a successful string substitution is not the same
as connecting as that role -- PGUSER and friends still override. The
whole design rests on that connection having no grant on the shared
table; a string comparison is too thin a thing to rest it on.
That check has to stay inside the try. Past it the cleanup statements
have already been merged into the fixture-level stack, and unwinding
again runs DROP OWNED BY twice, which has no IF EXISTS.
The catalog probe took any SQL and ran it on the admin connection. The
design claims withholding the DSN makes the boundary structural; that
was only true of the connection string, not of the capability. It takes
SELECT now.
--table's schema half is restricted to plain identifiers. Not a
security fix, since the name goes through a parameter and _quote: the
help text says complex identifiers are unsupported and the code was
accepting them anyway.
Until now the target came from whatever search_path resolved to. The
script printed what it found, but that print and the DELETE happen in
the same run with nobody in between, so it only ever helped the person
who ran a dry-run first. Swap the role that runs it and "$user" can
resolve somewhere else entirely.
--table takes the whole qualified name and resolves it directly. The
table half has to be llm_calls: a version that accepts any name turns
one typo into a general purpose row deleter, and any table with a
created_at and a tenant_id would go through the same batched DELETE
without complaint.
The tests that run it now run as a role that owns its own scratch table
and holds nothing on the shared one, so the row-count snapshot could
go. What replaced it is a case that lets the script fall through to the
shared table on purpose and asserts it exits 2 having deleted nothing.
That one has no red-first path, since making it red means running it as
the superuser, which is the thing being prevented; the finding's probe
covers it instead.
Five of the new usage tests passed before the flag existed, because
argparse rejects an unknown --table with exit 1 and the word --table in
stderr, which is exactly what they asserted. They now also assert the
error is not "unrecognized", which is the difference between testing
the validation and testing argparse.
The plan's one irreversible risk is the worst-case test itself. It
deliberately lets the script fall through to the shared table, and the
account in .env is a superuser, so running it before the sandbox role
exists would delete every expired row in there. All eleven rows on that
table predate any cutoff the tests use.
That forces the order: factory, then the retention tests move onto an
owner role, and only then does the case get written. Review caught that
the original order also made the --table integration cases impossible
to fail first, since the tool would already be implemented by the time
they were written. Same fix resolves both.
The worst-case case has no red-first path at all. Turning it red means
running it as the superuser, which is the thing being prevented, so its
evidence is the probe in the finding instead, and the plan says so
rather than calling it verified.
One acceptance criterion in the design turned out to be unrunnable: the
hint line only prints on the Postgres branch, so no unit test that
never connects can assert it. Corrected in place.
The failing assertion was never testing the script. It compared the row
count of a table three other projects also write to, before and after
the run, and the failure it reported (61 rows became 12) landed while
the script under test was demonstrably confined to its own schema.
A row count cannot carry the property that assertion stood in for. It
goes red when anyone else writes, and green when an outside insert
happens to cancel out a wrong delete. The second half is the one that
was guarding against a dropped audit table.
So the property moves to where the database enforces it: the script
runs as a role that owns its scratch table and holds no grant at all on
the shared one. Falling back to public stops being something a later
assertion might notice and becomes permission denied. Measured, along
with the rest of the Postgres semantics this rests on.
The tool grows --table so the target stops being whatever search_path
resolves to. The table half is pinned to llm_calls: without that, one
typo turns a telemetry cleaner into a general purpose row deleter.
Codex raised six problems and all are folded in. The one place this
still disagrees with it is recorded with the reason.
The reasoning_tokens docstring was still teaching downstream to treat
None or 0 as no reasoning. The changelog and the schema page had both
been corrected; the docstring had not, and it is the copy that ships in
the wheel and shows up on hover. Someone writing a report from it would
have counted every real MiniMax reasoning call as not reasoning, which
is issue #16 all over again with the tests green.
The original wording stays, since reading pre-1.3.1 rows still needs
it. What follows it now says when it expired and what to read instead.
Two more places had drifted the same way: the changelog and the
architecture doc described the throttle and the cache fallback as they
were before this review, which is to say as the opposite of what the
code now does.
The claim that the two throttle sets would suppress each other does not
survive checking, as the mutation testing showed: their key spaces do
not overlap. Keeping them apart is still right, but for the honest
reason, which is that the two warnings have unrelated lifetimes.
The enum belonged in types.py all along: making LLMResponse field-typed
on a symbol defined in thinking.py would have had the innermost layer
import a decision module, and import-linter would have caught it only
after the code was written.
The 4.1 table claimed reconciliation could still catch a failed disable
while section 5 said UNKNOWN never speaks. UNKNOWN has no falsifying
power; the guarantee only covers observable paths, and the doc now says
so instead of pretending otherwise.
Section 12 was written against a misreading: _record already is the
single helper the ironclad rule asks for, so there was no debt to
decline. Landing sites had missed ports.py, whose record_llm_call
freezes 24 explicit params with no defaults, and cache.py, where
_rehydrate revives the enum as a bare string.
The design argued for 1.4.0 because a broken deep-path import hidden
behind a patch bump is a debt handed to downstream. The call is 1.3.1.
Since the version number no longer carries the warning, the CHANGELOG
has to: breaking items and their fixes go first in the entry, following
the 1.3.0 read-this-first form.
The four red e2e cases were blamed on MiniMax-M3 no longer reasoning.
Raw gateway probes show the opposite: M3 reasons fine (124 chars of
reasoning_content, prompt 194 to 216, completion 3 to 60). What changed
is that the MiniMax route stopped returning completion_tokens_details,
while qwen and deepseek still do on the same gateway and key. The
library already holds 185 chars of proof in LLMResponse.thinking and
never feeds it into any verdict.
The design turns that verdict into a first-class return value judged
from multiple signals, says UNKNOWN when a single response cannot tell,
and reconciles it against the capability table so a stale declaration
becomes a warning instead of a silent illusion.
ARCHITECTURE 7.8 gains the pool resource semantics, the two-sentence
failure verdict and the two config keys; the ownership rule lands in a
new 4.5 because it is a cross-subsystem discipline, not a telemetry
convention. CHANGELOG leads with the three items downstream must read
first: the 3.12 floor, the connection count going from 10 per client to
on demand, and aclose no longer closing injected components.
The design traces the incident to four stacked defects rather than one bad
default: the pool is the only resource in the library that pre-allocates,
the kill switch keys off which step failed instead of what failed, the
degraded state can neither recover nor be observed, and the ownership rules
make the sanctioned sharing path unusable.
The plan sequences the tracker ahead of the pool and failure work so every
commit stays green, and records two facts the implementer needs up front:
the pool-construction path has zero test coverage today, and the commit
gate runs the full suite plus a complexity ceiling.
Branch review caught the docs claiming something the code does not do.
CHANGELOG, README and the design's behaviour matrix all said a
force-opened source under circuit_open=wait waits out the full stall
window. It does not: the probe let through after each cooldown is a
real attempt, so it burns a max_attempts slot like any other, and a
401 source usually runs out of retry budget first -- reason is
retry_exhausted, not stalled. Which budget wins depends on
max_attempts against the cooldowns and the stall window.
The behaviour is right; only the prose was wrong. Charging the probe
to the retry budget is exactly the split issue #8 settled: the
question is who spends max_attempts, and a probe does send a real
request. A test now pins it so the claim cannot drift again.
Also drops the planned "woke up" log line. Each wait round already
logs on entry with its duration, and a still-blocked wake-up logs the
next round immediately, so a second line would only double the volume.
The breaker conflates "this source is unhealthy" with "kill this call
now". Limiter rejections already choose between wait and fail_fast;
breaker rejections had no such choice, so a single-source scope loses
its whole retry budget the moment the gate opens.
Design adds {SCOPE}__CIRCUIT_OPEN (default fail_fast, so existing
deployments keep their control flow) and pins retry_after_s to "time
until a *certain* retry moment" across all six gate exits. The latter
also fixes a separate bug the issue missed: a half-open rejection fed
the probe lease (up to 2x timeout) into the source cooldown memo, whose
set_until only moves forward -- so a recovered source stayed blacklisted
in-process long after the gate closed. That one bites multi-source
deployments too, it is just hidden when other sources absorb the load.
Human-approved 2026-08-19; both documents revised after Codex review.
Postgres requires a partitioned table's unique constraints to cover the
partition key, so ranging on created_at forces the primary key to
(call_id, created_at) -- and ON CONFLICT (call_id) DO NOTHING then
matches no constraint at all. The retention design claimed INSERT stays
transparent under partitioning; that holds for the routing, not for the
conflict target, and telemetry would have failed outright on any
partitioned deployment. The write drops its conflict target, which is
byte-equivalent on a plain table and legal on both.
The cap design gains the three emitter construction sites it has to
touch and the relationship to the 200-char caps embed and OCR already
carry: they stay, and the new cap is the stricter of the two. Covering
all three call paths is deliberate -- their rows land in one table, and
issue #11 settled that argument already.
Both open issues ask the same question from opposite sides: how much
power the library holds over a downstream database. #13 wants the
structural writes back, #12 wants the data retention back. The two
designs share one boundary -- the library does SELECT and INSERT plus
an optional CREATE, and everything that alters structure or deletes
rows belongs to the downstream, with the library obliged to print the
exact SQL they need to run.
Two findings shape #13 beyond what the issue argues. The precedents it
cites (Hangfire's lock queue, Prefect's multi-instance race, Alembic's
audit trail) all live on a shared production Postgres, while the SQLite
side is a local file with no DBA and no migration tool, so the defaults
split by backend rather than uniformly. And turning ALTER off only
works together with trimming the INSERT to the columns that exist:
without it a stale table drops every row instead of two columns, which
breaks the telemetry rule harder than the automatic ALTER ever did.
For #12 only the body cap touches library code; retention and access
control land in the README, because the sdist carries src and the
README alone -- a template that lives in the wiki is one a downstream
pip install cannot reach.
The design said three times that retention and the _BACKFILL question
would be filed separately, and neither had been. That is the failure
mode the release checklist already records: a closing step nobody does
and nobody notices. Filed as #12 and #13, and the design now names them
so a later reader can follow the thread instead of trusting a promise.
OcrClient emits through the same helper and its rows land in the same
table as chat rows. Covering only chat and embed would leave one table
holding rows that have a tenant and rows that never will, and the
issue's own irreversibility argument applies to those rows too.
The design said two paths because the issue said two paths. Corrected
at the source rather than only in the plan, so a later reader does not
find OCR work with no design behind it.
Eight tasks against the approved design, ordered so the port and both
telemetry backends land before the three call paths that feed them.
Writing the plan turned up a third telemetry path the design missed:
OcrClient emits through the same helper and builds its ChatRequest on
the spot, just as embedding does. OCR rows share the table with chat
rows, so leaving them out would put a hole in a multi-tenant caller's
audit trail, and the same irreversibility argument applies. Listed as
Task 6 and flagged as beyond the approved scope -- it may be dropped,
but only by stating the limitation in the CHANGELOG, not silently.
The integration task pins the issue's own argument as a test: build a
22-column table, open it with the current recorder, and assert the old
rows read back as the empty string rather than NULL -- NULL under an
RLS policy is invisible to everyone, not merely unassigned.
The embedding client does not go through the chat onion -- embed() runs
its own chain down to _emit(), which builds a ChatRequest on the spot
and so far only fills session_id and parent_call_id. Changing chat()
alone would have left every embed row with empty dimensions, which is
exactly what the issue's second request asks for.
The bigger find: the draft claimed serialization could not fail because
the entry check already restricts values to scalars. It can. A float
passes a naive type check and json.dumps writes it as the literal NaN,
which is not valid JSON and which JSONB rejects; the failure then lands
in the emitter's degrade path and turns a caller's input error into
silently dropped telemetry. Now rejected at the entry with isfinite and
again at serialization with allow_nan=False.
Also states the validation runs at both public entries, not just chat(),
and adds the RLS template the design had promised but never wrote down.
Issue #11 asks for a tenant column so a multi-tenant caller can isolate
rows in the database. Widened to caller-defined dimensions in general,
but only the caller's own: model name and friends keep their existing
columns, and the library writes nothing into the new container.
Two independent findings force tenant_id to be a real column rather than
a key inside JSON. An RLS policy on meta->>'tenant_id' parses fine, but
the planner discards statistics for non-LEAKPROOF functions under RLS,
and ->> is not marked leakproof; the pgsql-general report that hit this
ended up moving the indexed column out of JSONB. Separately, the planner
has no usable statistics for JSONB at all -- @> falls back to a
hardcoded 0.1% selectivity.
A configurable promoted-column whitelist is rejected: when two
downstreams infer different types for the same key, the second
ADD COLUMN is silently skipped by IF NOT EXISTS and the wrong type is
written from then on, without an error.
The library stops at the column plus a documented policy template. It
must never enable RLS itself -- with no matching policy that is
default-deny, which would silently fail every write for the two
downstreams that are not multi-tenant.
The 500-char head-only rule came from a single sample. k8s client-go
caps the same thing at 2048; reprlib keeps head and tail because the
text is meant to be read. Gateway error bodies are JSON whose code and
request_id sit at the very end, so a head-only cut drops exactly what
you need to chase the provider. Version pinned at 1.2.0, which forces
the README install pin off ==1.1.*.
"Truncate at cap and append the ellipsis" admits both 501 and 500 total
length; the two would desync test assertions from the telemetry length
promise. Cap is now the total including the marker.
Issue #10: the 400 body dies in _status_to_error, and telemetry only
writes str(exc), so adding a field alone would not make the refusal
queryable after the fact. Design keeps the summary in both the message
and a new base-class body_text, across every non-2xx branch and both
transports.
PostgreSQL checks the schema CREATE privilege before the IF NOT EXISTS
existence test, so an account with only table-level INSERT was denied on
CREATE TABLE IF NOT EXISTS even though the table was right there and
writable. The denial set _failed and the whole recorder went no-op for
the process lifetime, silently: 150+ calls downstream lost their latency,
token and cost rows with nothing but one warning to show for it.
The probe is the direct fix. The larger fix is the criterion: structural
degradation now means "provably cannot write" (pool creation failed, or
the table is absent and cannot be created), not "something threw during
init" -- a probe or acquire failure just skips the row and retries on the
next call.
SQLite stays as it is on purpose. Measured: it short-circuits the
statement at parse time, so it passes even under another connection's
EXCLUSIVE lock or on a read-only file. A probe there would buy nothing;
the docstring now says so to keep symmetry-minded future edits away.
Registration pages carry the chosen approach, why the split is by "which
budget the time consumes", the five rejected alternatives with reasons,
and the 3.6 correction found during independent verification.
Independent verification found the first cut had swapped one bug for a
worse one. The budgets were split by "did we send a request", so a 429
attempt counted as productive — but 429 is exempt from the retry budget,
so its time burned neither budget. Against a queueing gateway that holds
the request for the full timeout before answering 429, a call could hang
for 301 attempts / 25.2 hours, measured, versus 301 seconds before the
change.
The split is now by which budget the time consumes: time that burns
max_attempts is excluded from stall, time that does not (429 attempts
included) belongs to stall. Measured again: back to one attempt / 301s.
Only the chat loop needs this — embedding and ocr count 429 against
max_attempts unconditionally, so the gap never existed there. The stall
verdict moved into _stalled(), which both call sites had duplicated, to
keep __call__ under the complexity gate.
Six tasks: StallClock plus the chat loop, then embedding, ocr, the config
comments, the full-suite regression with doc sync, and independent
verification. Codex review raised four points, all confirmed and folded in:
a stale line reference in the fidelity section, explicit cancellation
acceptance for T2/T3 (the new attempting() wrapper now wraps their existing
cancel paths), a telemetry-boundary test pinning the design's claim that
telemetry jitter must not feed the stall verdict, and concrete test
construction for the embedding/ocr regressions.
Two internal-consistency fixes from the independent design review:
the 429 saturation argument wrongly claimed exponential backoff growth
(429 skips the retry budget, so max(fails, 1) pins the delay to the base
tier), and "productive" was defined as waiting on the response while the
StallClock actually wraps all of _attempt. The boundary is now stated as
_attempt itself, including per-attempt accounting and telemetry, with the
rationale that telemetry jitter must not participate in the stall verdict.
Issue #8: a single request that burns its full timeout_s also exhausts
stall_window_s, so the retry budget silently never applies. Root cause is
that both budgets charge the same wall-clock time. The design makes the two
budgets orthogonal — real attempts bill the retry budget, everything else
bills the stall budget — which drops the timeout_s / stall_window_s coupling
instead of guarding it with an assembly-time check.
Letting SourceNotConfiguredError through the gate wrappers opened a hole
the recheck caught: _record_quietly only degrades GovernanceBackendError,
so an assembly defect raised from the accounting side would now escape and
destroy a response from a call that had already genuinely succeeded. That
inverts the exact invariant _record_quietly exists to hold.
Widening _record_quietly is the right fix rather than narrowing the
wrappers, because that layer degrades by what the path is (accounting, the
call is already done) rather than by which error type shows up. Narrowing
would have left 4 of 9 wrapper methods as exceptions to a rule nobody can
remember.
No backend raises it from an accounting method today, so this is a
guardrail for whoever adds source-name validation to a breaker backend.
The stub that first reported this green was wrong: its record_success
lacked count_attempt, so it raised TypeError and the wrapper relabeled it.
Fixed signature, then the test failed as it should have.
Also finishes the three-to-five leak path correction across the four
remaining spots, including the wiki summary card that indexes this design.
Independent verification caught that the split shipped in the previous
commit did not actually hold on the only path production uses. The gate
wrappers re-raise GovernanceBackendError but nothing else, so
SourceNotConfiguredError fell into the following `except Exception` and
came back out as a governance_backend_down failure with retry_after_s=5.0.
A misconfigured source name would still retry forever and never surface.
The existing tests missed it because both of them call the private _cfg()
directly, one layer below the wrapper the governance loops actually go
through. The regression test goes through QuotaGate.
telemetry.py has to widen its terminal catch in the same commit: once the
wrapper stops relabeling the error, it is no longer a GovernanceBackendError,
and it is raised before any attempt exists, so the path would have recorded
no telemetry at all.
Also corrects the leak path count from three to five. QuotaGate.stats and
BreakerGate.retry_after_s are not wrapped by _record_quietly either.
Five tasks, with the ARCHITECTURE section 6.1 revision first so the code
never contradicts the single source of truth, and the reparenting kept
atomic because scope is a required keyword argument and any split would
leave an unrunnable tree.
The Codex review caught that the planned test evidence pointed at the
wrong stubs: the ones at test_backpressure.py:176-186 cover accounting-side
degradation, not the three gate paths that actually leak to callers, and
try_acquire and try_enter have no stub at all.
All three open decisions were settled as proposed: a dedicated
SourceNotConfiguredError so a misconfigured source reaches the dead
letter queue instead of retrying forever, a 5 second retry_after_s so a
backlog does not stampede a backend that is already down, and public
export so callers can alarm on assembly defects specifically.
Pins the ARCHITECTURE section 6.1 revision to land before or with the
implementation, since the new scope reason contradicts the current single
source of truth. Documents why SourceNotConfiguredError may sit outside
the four-way classification: that rule governs transport-translated call
failures, and the GatewayUnavailableError family already lives outside it.
Also collapses the ten per-field response ternaries in emit_attempt into
an _AttemptUsage view. They all expressed the same decision and pushed the
method to cyclomatic complexity C, which blocked the commit gate.
Fail-closed governance backend failures are semantically scope-level
unavailability, yet GovernanceBackendError sits directly under
PolyGatewayError, so callers writing only `except GatewayUnavailableError`
drop them into the catch-all bucket and burn their failure budget on a
fault that a restart would clear.
The design reparents it under GatewayUnavailableError with a new
governance_backend_down reason, splits the two "unknown source" sites into
a separate assembly-defect error so a misconfiguration still reaches the
dead letter queue, and picks a non-zero retry_after_s to avoid a
zero-delay retry storm against a backend that is already down.
The design claimed merging would immediately break dissect. It would
not: dissect keeps running whatever version it already has, and this
release does not touch it. What is true is narrower -- once dissect
moves to 1.0.6, the M2.7 scope will refuse to assemble.
Worth recording because the distinction is not academic here.
dissect/requirements.txt declares polygateway>=1.0.1,<1.1, a range
rather than a pin, so 1.0.6 satisfies it and any routine reinstall picks
it up without anyone deciding to upgrade. So it is not "breaks on
merge", it is "breaks on the next dependency install".
The paragraph now carries both corrections it went through, since a
claim about downstream impact that was wrong twice is worth leaving
visible rather than quietly rewriting.
iomgaa