Caught on the sibling mort PR (gadfly/sonnet) and the same mistake was
here: map iteration is randomised, so the matrix printed its cells in a
different order every run and two failing runs could not be diffed. The
"which placements are bare" lookup was a second map keyed by name, which
also let a placement and its expectation drift apart by a typo.
Both are now one slice of {name, bare, build}, so the expectation sits
on the row it belongs to and the order is fixed.
Three consecutive rounds found the same rule broken in a different
orientation — content after the pointer, then content before it in the
same sentence — because each round's cases only covered the direction
that round was about, and each fix was then tested only in the direction
I had just thought of. A fourth patch was not the answer; the missing
thing was a harness.
TestBareAbovePointerOrientations crosses every pointer form with every
position content can occupy: alone, before in the same sentence, before
in its own sentence, on its own line, as a list item, after in each of
those, and behind a filler opener. 36 cells, one assertion — bare IFF
there is no content — and each cell first asserts the input really is a
pointer, so a mistyped fixture fails loudly instead of passing vacuously.
It earned itself immediately: three cells failed on the first run, and
the bug was mine and shipped. clauseBoundaryChars gained the em dash last
round, and the cut did start = k + 1 — but LastIndexAny returns the BYTE
index of the boundary rune, and an em dash is three bytes. "Done — as
shown above." sliced mid-rune, left a stray continuation byte in the
remainder that no trim removes, and a genuinely bare pointer stopped
being recovered. Now advances by the rune's width.
Break-check: sixteen mutations, each killed by a named test, control
survives. Two had to be reformulated after the harness started failing
mutations that were only "killed" by breaking the build — including the
one for this very fix.
My round-3 fix has the same defect it fixed, mirrored. bareAbovePointer
cut back to the previous FULL STOP, not the previous clause, so anything
sharing the reference's sentence ahead of it was swallowed:
"Ship Tuesday, as shown above." bare=true
"OK. The verdict is guilty, as detailed above." bare=true
"The answer is sixty minutes, as computed above." bare=true
All three read as pure pointers and were replaced by an earlier turn.
The comment called it a clause cut; the code cut a sentence. It survived
a round because every case I had written put the answer AFTER the
pointer — I fixed the direction I had thought of and tested only that.
clauseBoundaryChars now includes , ; : and the em dash.
Three more, all real:
- pointsAbove and bareAbovePointer each did their own FindStringIndex
plus offset check, so the two could disagree about what a pointer is.
One aboveRefLoc now answers that.
- fillerWords was extracted to stop drift while its separator class was
still copy-pasted beside it — the same defect one token over. fillerSep
shares it too.
- A missing blank comment line ran two paragraphs together.
The padded 96-byte "bare pointer" fixture is gone rather than bent a
third time. Its premise does not survive the rule: a genuinely bare
pointer is its own clause and nothing else, so it is SHORT, and the two
recovery bars cannot disagree about a short terminal. I had twice
reshaped that string to keep a case alive whose scenario the code no
longer admits. The modeBackRef path stays covered by the bare-pointer
cases in TestIsWeakFinal and the "(Already answered above.)" end-to-end.
Break-check: fifteen mutations including a regression to the sentence
cut, each killed by a named test; the harness now also fails a mutation
that only breaks the build.
Round 2 fixed this class on isSummaryCloser and left its twin standing
on isWeakFinal. Three of the four findings are that twin, and every one
of them reproduced:
"That's the chain above. Ship Tuesday." weak=true
"See the summary above. Option B wins." weak=true
"Anything above 100 boils. See the note above." weak=true
"Anything above, say, 40 degrees is a problem." weak=true
Each of those is 37-58 bytes with the answer sitting right next to the
pointer, and each was disposable — a >=200-byte earlier turn replaced it
and "Ship Tuesday" went in the bin. pointsAbove asks whether a pointer is
PRESENT; disposability needs it to be ALL THERE IS.
bareAbovePointer cuts the reference's own clause — from the end of the
previous sentence through the match — and requires what remains to be
filler: nothing, punctuation, or a throat-clearing "Done —". Cutting the
clause rather than testing position is what makes the other two shapes
safe for free: a mixed terminal keeps its first sentence, and a
comparative "above," with an interjection keeps the rest of its own.
The fourth finding (kimi) is the same idea one level up:
compressionMarkerRe matched anywhere, so "Given the analysis above, the
bottom line is that we need a different vendor" read as an announced
summary when it is a conclusion. The marker must now OPEN a sentence.
Both classes now enforce one rule from opposite ends: a terminal is
disposable only when it carries no answer of its own — proved in
isWeakFinal by nothing standing beside the pointer, and in
isSummaryCloser by the model declaring what stands beside it a
compression.
One of my own round-1b fixtures had to change: "…is above, so there is
no point repeating it" is a pure pointer to a human, but prose after the
reference is indistinguishable from an answer, so the rule correctly
stops treating it as disposable. Replaced with a genuinely bare 96-byte
pointer, still sized so the two recovery bars disagree about it.
Break-check: thirteen mutations, each killed by a named test, control
survives. M12 initially "passed" by failing to compile — reformulated so
it builds, and TestIsWeakFinal/prepositional-then-deictic-with-content
kills it properly.
Four findings, all real.
**A deictic closer now needs a compression marker too** (opus,
correctness). The pointsAbove branch of isSummaryCloser required only a
back-reference, so a terminal like "Given the analysis above, I
recommend option B because X" — a pointer followed by a CONCLUSION the
earlier turn never contained — would be discarded in favour of that
turn, throwing away the answer. The ack shape does not have this problem
because "Citations are logged." carries nothing; a bare pointer does not
carry that guarantee.
Both halves are now required: the pointer says the full answer is
elsewhere, and compressionMarkerRe ("Short version:", "TL;DR", "In
short") is the model saying what sits beside it is a condensation rather
than new reasoning. #1611's closer has both. A marker without a pointer
stays out of scope for the reason already documented — a user who asked
for brevity gets exactly that shape. Unmatched closers keep today's
behaviour, so the narrowing fails closed.
**CRLF** (opus, error-handling): "above\r\n" was not clause-final, so a
CRLF transcript quietly lost every line-final deictic. CR joins LF in the
terminator set.
**A comment wrapped mid-phrase** so that "// -style" read as a list
marker (sonnet) — reflowed as part of rewriting that doc block.
**Process provenance in a test comment** (sonnet): "(gadfly, 3 models)"
is an execution log, not an invariant. The lesson survives, the
attribution does not.
Break-check is ten mutations now — dropping CR and dropping the
compression requirement are each killed by their own named cases — with
the control surviving.
Round 2 (glm): the isSummaryCloser comment still explained itself in
terms of "the whole isBackRef class" — a symbol deleted in f97c2b7. It
named the right distinction with the wrong noun. Reworded to name
backRefRe directly; no isBackRef occurrences remain in the package.
Sonnet's three findings, all on the tests rather than the rule:
- The "at the bound" case sat at index 115 (and 101 after the first
round), never at 120 itself, so <= vs < was never exercised. Both sides
of the boundary are now pinned — index 120 accepted, 121 rejected — and
an off-by-one mutation is killed by the new case.
- A SHORT (<=120-byte) deictic closer takes the modeBackRef bar, not the
summary closer's mandatory dwarf ratio, even though it can carry a
scrap of answer content. That asymmetry is deliberate and pre-existing
(a "see above" closer has always had it): inside 120 bytes there is no
room for both a pointer and a real answer. Pinned with a case sized so
the two bars actually DISAGREE — 3x98 = 294 > the 275-byte prior turn,
so the summary bar would reject what the back-ref bar accepts. The
first draft of this fixture was 83 bytes and passed under either bar,
proving nothing.
- The isBackRef doc finding was sonnet's fourth independent report of it;
already fixed in the previous commit.
Break-check now runs eight mutations, including the off-by-one and a
swap of the classifier ordering; each is killed by a named test and the
control survives.
Three of four reviewers independently found the same defect: `\b` holds
between "above" and "-", so the literal hyphen in aboveRefRe's terminator
class made every hyphenated compound clause-final. "above-average",
"above-board", "above-ground" all read as deictic pointers, putting a
legitimate short closer at risk of being discarded. My own test used the
SPACE-separated "above average" — it tested the neighbour, not the named
path, which is exactly why the class survived a round of review. Hyphen
dropped from the class; the four compounds are now table cases.
Two more findings, both real:
- backRefHeadChars and weakFinalMaxChars were two unlinked 120 literals
that the comment called "the same guard". Now defined by reference,
with a test pinning the identity.
- isBackRef claimed to be the shared extension point for both shapes but
had a single caller — isSummaryCloser deliberately uses pointsAbove
directly, because backRefRe's fixed phrases match ANYWHERE and are only
safe under the 120-byte weak cap. Folding the two together for
tidiness would widen the gate, not deduplicate it. Helper deleted, the
doc moved onto pointsAbove where the real sharing is, and it now says
why backRefRe is not shared.
Also shared the front-loaded-analysis fixture between the table and the
end-to-end test (kimi), and de-hollowed the offset-bound cases: they were
sized as backRefHeadChars±n, so they moved with the constant they were
meant to pin — a break-check that widened the bound to 100000 sailed
through. Literal lengths now, plus an explicit identity assertion.
Break-check: six mutations, each killed by a named test; control survives.
mort issue #1611: run 8eea3e82 front-loaded a 2,245-char analysis into
its cite-call turn and closed with 220 bytes — "Done — that's the full
chain above. Short version: …". The user got the 220 bytes and a
pointer at a chain that was never posted.
finalOutput already had three shapes for this pathology, and the closer
matched none of them: too long for the weak-final cap (220 > 120), no
citations heading, and no "Citations are logged." ack to open the
summary-closer class. So it was delivered verbatim.
The three shapes were each a separate vocabulary of ack phrases, which
is why a fourth phrasing walked straight through. Two of them are really
one signal — the terminal DEFERS, telling us the answer is somewhere the
user cannot see — differing only in whether the terminal also carries
content of its own. That signal is now isBackRef, shared by both, so a
new phrasing is added once and covered in the bare and the
"+ compression" variant at the same time.
Its open-ended half is aboveRefRe: a DEICTIC "above", separated from the
preposition by what follows the word. The deictic use ends its clause
("that's the full chain above.", "as shown above,"); the preposition
always continues into a noun phrase ("above 100°C", "above the fold",
"above all, …"). pointsAbove additionally requires the reference in the
terminal's first 120 bytes — with almost no text before it in THIS
message, it cannot be pointing at the message's own content.
isSummaryCloser now opens on either the citations ack or pointsAbove.
Recovery is unchanged: the mandatory dwarf ratio and the user-message
scan boundary still gate it, so a closer only loses to a prior turn in
the same user turn that is clearly the fuller original.
Break-checked: reverting either classifier, dropping the clause-final
rule, or dropping the offset bound each kills a named test; the
unmutated control survives.
A `git add -A` in a checkout with a worktree under .claude/worktrees/ records
it as a gitlink — a submodule pointer no clone can resolve. That happened in
gadfly today; this is the same one-line prevention, applied before it happens
here.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The progress entry was written before four review rounds reshaped the change:
it credited openaiCompatScheme alone, listed the tests as six per-provider
cases, and mentioned a captureRT detail that has since moved. Rewritten to
match what merges — registerOpenAICompatBuiltin owning both halves,
envKeyForProvider as the single LLM_<NAME> definition, and the shared table
that every OpenAI-compat built-in is now checked against.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
DSN.Scheme's list named kimi but not qwen — the same sibling drift this PR
keeps finding, in a doc comment this time (I updated the README's scheme list
and not this one). Added qwen, and llama-swap/llama-swaps while there, since
they were missing too.
envKeyForProvider's example ran backwards: "LLM_M1 → m1" describes registry
naming, not what the function does. Now reads name → variable in one
direction.
Not taking the third: the const block's alignment is gofmt's own output
(gofmt -l is empty), and the uneven padding is forced by the doc comments that
split the block into alignment groups. glm-5.2 reached that same conclusion in
round 2 before flagging it here.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Three findings, and the first two are the same recurring shape.
envKeyForProvider (env.go) is now the single definition of the LLM_<NAME>
form. It lived in two places — lazy resolution in registry.go and the
missing-key hint in openaiCompatScheme — with a comment on the second asserting
it matched the first. A comment is not enforcement: if either had drifted, a
keyless DSN target would have named a variable that does nothing, and nothing
would have failed.
The kimi and qwen test files had become near-identical, which is round 1's
finding at the level above it: I deduped the fixtures, then left two parallel
suites asserting the same four things. They are now ONE table in
builtin_openaicompat_test.go — endpoint + credential, missing key fails closed
naming its own variable and never reaching the network, the name:// DSN
reaching another host, and a keyless DSN naming LLM_<NAME> instead of the
built-in's key. Adding an OpenAI-compat built-in is a table row that
immediately owes all four; builtin_kimi_test.go is deleted because the table
covers it. Only genuinely qwen-specific tests remain in the qwen file: the
reverse credential leak and the reasoning_effort wire claim ADR-0027 rests on.
Also trimmed ProviderQwen's doc comment, which restated the ADR-0027 rationale
already given at the registration site.
The break-check suite caught its own rot again — two mutations went stale when
these tests were renamed, and the landed-check reported them loudly instead of
passing them off as green. Now 9 cases, including one that drifts
envKeyForProvider to prove the shared helper is load-bearing. 9/9 apply and are
caught.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Same class of finding as round 1, one level in: I factored the DSN-scheme half
of the kimi/qwen duplication into openaiCompatScheme and left the eager
provider half copy-pasted, so a third built-in still had six lines to clone —
including both credential rules, which is exactly the pair you do not want
re-typed.
registerOpenAICompatBuiltin now installs both halves from one call. The rules
that matter hold by construction for every future caller: WithAPIKey passed
unconditionally (an unset key must not fall through to OPENAI_API_KEY), and
WithAPIKeyName naming that same variable in the 401 hint. Registering kimi and
qwen is now one line each.
Also fixed a cross-reference the ADR got wrong: Qwen's image-input caveat is
README matrix footnote ⁴, not ³ — ³ is kimi's. I wrote "³, shared with kimi"
in the ADR and then gave Qwen its own footnote in the README.
The break-check harness needed fixing before any of this could be trusted:
three of its mutations targeted lines this refactor moved, so they matched
nothing, the code was never broken, and the suite reported "test still passed"
— identical output to a test that genuinely misses the bug. Mutations are now
verified to have landed (sha before/after) and the suite fails loudly if one
doesn't. Two new cases cover the helper: dropping the unconditional WithAPIKey,
and dropping the scheme-half registration. 8/8 apply and are caught.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Both findings were the same one, and both were fair: the PR that retires two
byte-identical DSN factories into openaiCompatScheme then copy-pasted the test
fixtures. qwenResponse was byte-identical to kimiResponse, and the single-key
env-lookup closure appeared three times in the new file (plus a fourth in the
kimi file, which neither reviewer was looking at).
Fixed for the class rather than for qwen: captureRT, the canned Chat
Completions body (now chatCompletionOK), and a new singleKeyEnv helper move to
builtin_openaicompat_test.go, owned by no single provider. The kimi tests adopt
them too, so the next OpenAI-compat built-in has nothing left to copy — the
same argument the production helper makes.
Also aligned the test model ids to the current Model Studio names
(qwen3.8-max / qwen3.7-plus), which the docs already cited. One reviewer called
those ids fictional and named the 2025 ones instead; they shipped 2026-08-03
and 2026-05-21 respectively, so that finding is stale model knowledge, not a
defect — but having tests and prose name the same models removes the smell that
prompted it. A dotted id also now proves it passes through verbatim.
Break-checked again after the refactor: all six mutations still fail their test.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Adds the `qwen` built-in provider and the `qwen://` DSN scheme, keyed by
QWEN_API_KEY and defaulting to Model Studio's international host. Like kimi
(ADR-0026) it is `provider/openai` pointed elsewhere — no new client.
Model Studio serves the same models over two protocols, so the real decision
was which wire format to speak. ADR-0027 records why it is the OpenAI one:
down the anthropic client `ReasoningEffort` is ignored by design, structured
output rides the first-party `output_config.format` mechanism the shim does
not implement, and cached-token accounting reads Anthropic-only usage fields.
Each of those fails silently rather than loudly, which is what makes the
choice worth writing down. The shim stays reachable ad hoc via an
`anthropic://` DSN.
The kimi and qwen DSN factories were byte-identical, so they now share one
`openaiCompatScheme` helper: the "credential comes from the DSN token, and
the missing-key hint names LLM_<NAME>" rules hold by construction instead of
by copy.
Tests are hermetic and break-checked (all six fail on a deliberate mutation),
including the reverse credential leak — a visible QWEN_API_KEY must not
authenticate the openai built-in — and reasoning_effort asserted on the wire
body, which is the ADR's load-bearing claim.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The 2/4 finding is right: after writeImagePart started passing an explicit
filename stem, initImageFilename had no caller in video.go, and its
"conditioning frame" doc no longer described its one remaining user
(lipsync.go's avatar image). A one-line wrapper that survives only to be
misdescribed is not indirection worth keeping.
lipsync now calls imageFilename(mime, "frame") directly, and imageFilename's
doc lists the real bases — including WHY the video keyframes need distinct
ones: a backend that stages uploads by filename would otherwise have the
second overwrite the first.
Not taken: consolidating the first/last-frame rationale to a single canonical
site. The copies address different readers — the wire encoding (provider), the
contract's undetectable-support caveat (videogen), and the mode table (README)
— and last round's finding was a doc pointing at a note that did not exist.
Trading duplication for cross-references is what produced that.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
- README documented only t2v/i2v. Now a table of the four keyframe
combinations, plus the undetectable-support caveat, which is the one thing a
caller cannot work out for itself.
- The LastImage doc comment said "see the note on LastImage support in
provider/llamaswap" — there was no such note. A pointer to something that
does not exist is worse than no pointer; the comment is now self-contained.
- Generate's doc described only input_reference; it now names
input_reference_last and explains why an unsupporting backend returns a clip
rather than an error.
The 2/4 finding (writeImagePart reusing the "frame" base for both parts) was
already fixed in dbc9689 — from the receiving end, where the consequence is
concrete rather than stylistic: ComfyUI stages uploads by FILENAME with
overwrite=true, so a shared name means the second clobbers the first and both
keyframes resolve to one image.
Not taken: initImageFilename's name is no longer misleading (writeImagePart
stopped calling it), and it is still used by lipsync.go so it is not dead.
The empty-LastImage test stays standalone — it mirrors the existing standalone
empty-InitImage coverage rather than a table this file does not have.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
Caught while writing the receiving end. Distinct multipart field names are not
sufficient: backends stage an uploaded frame under a name derived from the
FILENAME, and our own ComfyUI shim posts to /upload/image with overwrite=true.
Both parts were sending initImageFilename(mime) — literally "frame.png" for
each — so the second upload would have clobbered the first and BOTH keyframe
inputs would have resolved to the same stored image.
The failure mode is the worst kind: a clip pinned at both ends to the same
frame renders cleanly, returns 200, and looks like the feature not working
rather than like a bug. Nothing upstream or downstream would report a fault.
writeImagePart now takes the filename stem (frame / frame_last), and the test
asserts the two arrive under different filenames.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
videogen.Request gains LastImage alongside InitImage, so one Request covers
t2v, i2v and FL2V without a mode flag. With InitImage it pins both ends of the
clip; alone it pins the destination and lets the backend invent the approach.
The llamaswap provider sends it as a SEPARATE `input_reference_last` part
rather than a second `input_reference`. Multipart permits repeated names, but
then which frame is first and which is last depends on part ORDER — an
ordering contract invisible in the payload, that nothing notices breaking. A
backend that does not know the new name ignores the part, the same degradation
as any other unknown field.
Both parts go through one writeImagePart helper so their encoding cannot
drift, and an empty LastImage is rejected up front exactly as InitImage
already is.
Support is per-model and deliberately NOT advertised in this contract: a
backend that ignores a trailing keyframe returns an ordinary clip, which is
indistinguishable from success. The doc comment says so, because a caller that
needs to know whether the pin took effect has to establish that out of band —
and the mort side gates on a convar for exactly this reason.
Motivated by mort's #1567 (long-form video): with both ends pinned, drift
becomes structurally bounded inside each shot instead of compounding across an
autoregressive chain.
Tests break-checked: sending the last frame under the shared name fails both
the distinct-name assertion and the last-alone case.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
All tidiness, no behavior change: the leading-marker class is one shared
constant for citationLabelRe and summaryCloserRe (hand-copying it is how
'+' went missing the first time); the deliberate 'all' duplication across
summaryCopulas/summaryArticle is now stated at both sites; the weak-final
switch case assigns modeBackRef explicitly; test comments state the
constraint they guard instead of which reviewer asked for them.
Co-Authored-By: Claude Fable 5 <[email protected]>
Two behavioral fixes from the review:
- modeSummary's backward scan now stops at the most recent user message.
With the dwarf ratio rejecting the current turn's 1x-3x answer, the old
unbounded scan could walk into WithHistory content and resurrect a stale
answer to a DIFFERENT question — strictly worse than keeping the closer
(opus, correctness). Other modes keep their historical unbounded scan.
- A terminal matching BOTH the ack shape and a back-reference is now
classified back-ref: it carries no answer content, so the looser bar is
the right one (opus, error-handling).
Plus the nits: summaryCloserRe assembled from named fragments, the leading
marker class gains '+' (parity with citationLabelRe), verb-first form takes
'all the', dwarf ratio hoisted into one named local, and the 151-vs-153
char/byte comment inaccuracy corrected.
Co-Authored-By: Claude Fable 5 <[email protected]>
A third degenerate terminal shape from the glm-5.2 cite pattern: the model
front-loads its full answer into the cite-call turn, then closes with a
bookkeeping ack plus a one-line compression ("Citations are logged. Short
version: ..."). mort run b3cb9ee9 delivered 151 chars of a 2,089-char
answer this way — the closer was neither a back-reference (over the 120
cap, no back-ref phrase) nor a citations addendum (no label-colon, no
links), so finalOutput let it stand.
isSummaryCloser keys on the ack sentence alone (the verb must end the
sentence, so prose about citations never matches; a compression marker
without the ack is deliberately out of scope), and the new modeSummary
recovery bar makes the 3x dwarf ratio mandatory at every length: unlike a
back-reference this closer carries real answer content, so it is only
displaced by the clearly-fuller original it compressed.
The citations/back-ref bool becomes a three-way recoveryMode; existing
behavior for both old modes is unchanged.
Co-Authored-By: Claude Fable 5 <[email protected]>
ListFaces now carries head yaw, so a caller choosing WHICH face to swap can
see the thing that decides whether the swap will read — not only learn it
afterwards from the swap report. In the run that prompted this the target's
three faces sat at -82, -8 and -11 degrees; only the first was hopeless, and
nothing in a bounding box said so.
A face swap always returns an image and always looks like success. Whether the
likeness actually transferred is a different question, and until now nothing in
the response answered it — so a caller wanting to know went and asked a vision
model instead. That is wrong in precisely the cases that matter: shown a jogger
in a Georgetown cap holding McDonald's cups, a VLM answers "Bill Clinton"
whoever's face is on him. In the run that prompted this it reported failure on
six consecutive CORRECT swaps (measured afterwards at 0.79-0.84 cosine), and
the caller burned 21 minutes chasing a problem that did not exist.
Result.SwappedFaces now carries, per replaced face: pixel size, the target
image's dimensions, head yaw, and cosine similarity between the source face and
the face actually present in the output.
Yaw and FractionOfImage are the two that explain the complaint. The swap in
question replaced a 138px face in a 1010px-wide photo — 14% of the width,
correct and invisible at a glance — and elsewhere a face turned -82 degrees,
where the features carrying identity are edge-on and any swap reads as a
generic person. Same code on a 168px face in a 385px picture (44%, yaw 2) is
unmistakable. None of that was inferable from a bounding box.
Typed on Result rather than stuffed into Raw: a caller has to act on this, and
a value reachable only by type-asserting an `any` is one nobody finds in time.
doRawHeaders is doRaw with the whole header instead of only Content-Type; doRaw
delegates to it, so the other 25 call sites are untouched and there is still
one place where the status check and the size cap live.
A missing or malformed header yields nil, not an error — an older shim sends no
header, and a swap that produced a good image must not fail because the
diagnostics beside it were unreadable. Covered for absent/garbage/wrong-type,
and the parse is break-checked.
Gadfly on #23, blocking, 2/2 agreement — and it is the exact defect this
whole line of work has been about: a call that succeeds while handing back
the wrong bytes.
sniffImageMIME falls back to image/png when detection is inconclusive, and
the guard only consulted Content-Type. A response with NO Content-Type
therefore skipped the check entirely and was labelled a PNG. The shim answers
JSON on a semantic miss (no face found in the source or target), which is
precisely the body that would have sailed through as a successful image.
The check now validates the BYTES — http.DetectContentType must say image/ —
and the reported MIME prefers the server's own label only when that label is
itself an image type. Break-checked by restoring the header-only condition,
which fails the new test.
Also from that review:
- index is documented as ignored under all=true, so a negative one is no
longer rejected there; it is still rejected when it would actually be
sent, and both halves are tested.
- initImageFilename (video.go) was imageFilename with the base fixed to
"frame" and now delegates to it — two copies of one extension table is
how they drift.
- DetectedFace carried Width/Height alongside Box, two sources of truth for
one fact that can disagree after any transform. Now a Size() method
derived from Box.
- a dead `apiErr` in the test (declared, then `_ = apiErr`) was an
abandoned errors.As check; it is wired up and now asserts callers can
classify the error.
- swapImg duplicated editInit verbatim; removed.
Not taken: adding a FaceSwapProvider/ModelOption surface to match the other
optional imagegen capabilities (single-model finding). There are no options
to carry yet, and inventing an empty option type to look symmetrical would be
API surface with nothing behind it. Worth revisiting when a real knob exists.
Measured against the instruction-edit models on 2026-07-31: asking a diffusion
model to put a SPECIFIC person's face into a photo does not work by any route.
qwen-image-edit returns the picture essentially unchanged whether asked by
name, by attribute, or by supplying the portrait as a second reference image;
flux-kontext replaces the face with a different generic person. Identity
transfer is a detect/align/blend pipeline, not a better prompt, so it gets its
own interface rather than more Edit options.
imagegen.FaceSwapper is optional and type-asserted, like Editor — a provider
that cannot do this must not have Edit quietly stand in for it.
ListFaces is part of the interface, not a convenience: a caller asked to
change "the man on the right" needs a stable way to NAME one face, and pixel
boxes let it check its own choice. The llamaswap shim orders faces left to
right for exactly that reason (insightface's own order is score-ranked and
unstable between near-identical images), and a malformed box is a protocol
error rather than a zero-filled struct, because a wrong box aims the swap at
the wrong person.
The provider is the first here to POST more than one file, so buildMultipart
gained buildMultipartFiles and now delegates to it — one writer loop, so the
two cannot drift in how they escape names or terminate the body.
index and all are mutually exclusive ON THE WIRE: the shim ignores index under
all=true, and sending both would imply a precedence the caller cannot see.
A JSON body is refused rather than returned as image bytes — the shim answers
JSON on a semantic miss (no face in the source), and handing that back as a
picture would report success while delivering a file that is not one.
FLUX.1 Kontext and Qwen-Image-Edit are a different kind of edit from img2img
and reach sd-server by a different path, and nothing in imagegen could
express it: EditRequest only had Init, which is noised and denoised back
under the prompt.
Measured against FLUX.1-Kontext on the netherstorm host 2026-07-30, on a
synthetic scene with a red rectangle, a blue rectangle and a flat background,
prompted "change the blue rectangle on the right to bright green, keep
everything else exactly the same":
via init_images (the only path that existed)
right rect (60,60,200) -> (47,82,228) still blue, instruction ignored
left rect (200,60,60) -> (229,43,50) drifted
background (150,200,240) -> (154,211,229) drifted
via extra_images (this change)
right rect (60,60,200) -> (70,254,4) green, as asked
left rect (200,60,60) -> (204,57,57) intact
background (150,200,240) -> (151,202,247) intact
No mask, no strength, no compositing — the model is handed the picture as
conditioning and the prompt as an instruction about it.
EditRequest.RefImages selects the path; when set, Init/Mask/Strength are
ignored rather than rejected, so a caller handing the same request to
whichever model is configured gets the better result on a Kontext-class model
instead of an error. The provider posts /sdapi/v1/txt2img with extra_images
(sd-server reads that field on both routes into gen_params.ref_images, where
the CLI's -r/--ref-image also lands); there is no init latent to denoise, so
sending one would only add noise to a pipeline that does not want any.
An all-empty reference set is refused: it would otherwise degrade into a
plain txt2img and render the prompt from scratch, which is not the request.
Merge the parallel callCounts/lastResults maps into one repeatState struct
map (removes the "two maps in sync" smell + double lookup), drop the dead
i<len(results) bounds branch that contradicted the documented index invariant,
and note in-code that exact-string result equality is a deliberate err-toward-
not-tripping choice (a hung job whose poll reports a ticking field is left to
MaxRuntime / the job ceiling rather than risking a false kill of real progress).
No behavior change; guard tests still green.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01HgEuVfZJN9mhRhzEsMEVog
The maxSameCallRepeats guard counted identical (name+arguments) tool calls
across a run and tripped ErrToolLoop past the ceiling — regardless of whether
each call made progress. This killed legitimate polling of long-running
background jobs: code_exec_poll must be called with identical args (same
job_id), so a render/encode that needs more than N polls was guillotined
mid-flight even as each poll returned an advancing result (elapsed/status
moving forward).
Only count an identical call toward the trip when its RESULT is unchanged
from the previous identical call. A call whose result keeps changing is
progress and resets its count; a genuinely stuck call returning the same
output still trips. This can never trip more than before, only less, and
covers every idempotent poller with no per-tool configuration — matching the
progress-over-usage thesis behind the stall-detection work.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01HgEuVfZJN9mhRhzEsMEVog
- kimi:// DSN scheme: missing-credential hint now names the LLM_<NAME> env
var that defines the provider (its token comes from the DSN, not
KIMI_API_KEY), matching providerFor's lazy-resolution key form. Fixes the
correctness/error-handling findings that the old hint misdirected users to
set KIMI_API_KEY when the fix is adding a token to the DSN.
- parse_test.go: add kimi to TestBuiltinsResolve. (llama-swap stays excluded
and is now documented — its no-URL built-in errors at Model() construction,
not just on use, so it can't resolve there; the finding's llama-swap half
was a false lead the test surfaced.)
- Add TestKimiSchemeMissingToken covering the corrected hint.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add a first-class `kimi` provider and `kimi://` DSN scheme for Moonshot AI's
OpenAI-compatible Chat Completions API. Both reuse provider/openai (no new
client, mirroring llama-swap's chat path). Default endpoint is the
international host; the China endpoint is reachable via a kimi:// LLM_* DSN.
- Credential is KIMI_API_KEY, read through the registry's injected envLookup
so it stays hermetically testable. WithAPIKey is passed unconditionally so
an unset KIMI_API_KEY can never fall through to the openai client's
OPENAI_API_KEY default.
- New openai.WithAPIKeyName option customizes the missing-key error hint
(default OPENAI_API_KEY); kimi names KIMI_API_KEY.
- Hermetic tests: built-in base URL + bearer, missing-key hint names
KIMI_API_KEY with no OPENAI fallthrough and no network hit, kimi:// scheme
round-trips against the China host.
- Docs in sync: README built-in table + DSN scheme list + support matrix,
.env.example, env.go DSN doc, ADR-0026 (+ index, backfilling 0024/0025),
progress.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Picks up gadfly's opencode/<model> engine — the reusable at this ref pins the
sha-bb98fae reviewer image that bundles the OpenCode CLI. Also correct the pin
comment: this is an immutable sha pin, not the v1 tag it claimed to track.
- SubmitChain rejects NaN/±Inf segment seconds with ErrUnsupported
(previously an obscure json.Marshal error; NaN fails every comparison
and +Inf passed the >= 0 check).
- ChainStatus skips segment entries with no usable id — JSON null
(which no-op-unmarshals into a string, previously appending ""),
empty strings, and id-less objects; the unfiltered list survives in
Raw. ChainJob.SegmentIDs doc now also says ChainSegmentResult takes
the segment index, not an id string.
- jobPath rejects '%' in job ids — %2F/%2E%2E percent-escapes decode
back into path structure server-side, bypassing the literal check on
this upstream-echoed value.
- singleVideoResult moves to video.go next to videoMIME, and the two
remaining hand-rolled copies of the video-result validation
(videoModel.Generate, Interpolate) now use it — one validation, one
message shape.
- videogen.LipSyncer renamed to videogen.Lipsyncer for consistency with
the rest of the surface's Lipsync* naming (LipsyncProvider,
LipsyncModel, LipsyncRequest); not yet consumed downstream, so the
rename is free now and never again.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
- speakWithReference now validates the response IS audio via the same
audioResultMIME sniff the sfx/enhance surfaces use — a 2xx JSON soft
error or HTML proxy page was previously wrapped up as audio/wav bytes.
- audioResultMIME moves to audio.go (next to speechMIME; it was defined
in sfx.go but shared by enhance/clone) and learns the Ogg container
normalization (application/ogg -> audio/ogg).
- Stems zip unpack gains entry-count (16) and total-decompressed (1GB)
caps on top of the existing per-entry cap — the per-entry bound alone
still let a many-entry bomb multiply up.
- sanitizeFilename drops NUL and both path separators too, so upload
metadata can never smuggle directory structure to a file-writing shim.
- New maxAudioResponseBytes (256MB) for bodies that ARE one audio clip
(clone, enhance, sfx): a long WAV legitimately passes the 64MB JSON
cap.
- speakWithReference local renamed path -> upPath (naming parity with
stems/enhance).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
- Segment: reject NaN thresholds (NaN fails every comparison, so it
passed the [0,1] range check and reached the shim as the literal
string "NaN"); ±Inf were already caught by the range comparisons,
now covered by tests too.
- upstreamPath: reject '%' in model ids — %2F/%2E%2E percent-escapes
decode back into path structure server-side, bypassing the literal
/?#/.. rejection. Ids never legitimately contain '%'.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
- audio.StemSeparator/StemSeparationProvider: Demucs zip transport via
POST /upstream/<id>/v1/stems (Mode two -> two_stems=vocals; model +
format fields); bounded zip unpack, entry name -> stem, ext -> MIME.
- SFXModel reuses musicgen against the sync /upstream/<id>/v1/sfx route
(JSON prompt/seconds/steps/cfg_scale/seed -> WAV); musicgen.Request
gains CFGScale.
- audio.SpeechEnhancer/SpeechEnhancementProvider:
POST /upstream/<id>/v1/enhance -> WAV (result reuses SpeechResult).
- SpeechRequest.ReferenceAudio/ReferenceMIME (+WithReferenceAudio):
llamaswap switches to the chatterbox clone route
POST /upstream/<id>/v1/audio/speech/upload (input + voice_file),
wav MIME fallback.
- TranscriptionRequest.Translate (+WithTranslate): translate=true form
field, language=auto forced when no explicit hint (whisper.cpp default
en would skip translation).
- httptest contract tests (zip unpack, clone-route switch, translate +
auto-language injection); ADR-0024 (index row deferred — MJ-A backfills
the ADR index table and parallel edits would conflict).
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
