Repository navigation
Move to imogen-sdk main@cb814ad: read wire booleans by spelling - #102
Merged
Merged
Conversation
A between-releases pin: cb814ad is a commit on SDK `main`, one ahead of the `v0.5.0` tag this repository was pinned to, and not itself a tag. `git submodule status` reads `v0.5.0-1-gcb814ad` rather than `(vX.Y.Z)`, which is legal between releases and is not legal at one — a consumer release has to move this to an SDK tag first. What it picks up is imogen-sdk#36. `AssetFilter` and `TimelineQuery` read their flags with `z.coerce.boolean()`, which calls every non-empty string true, and every port writes the filter as the string "false". So in the shipped v0.6.0 server `?trashed=false` answered with the trashed assets alone — the exact inversion of the request — `?favorite=false` returned only favourites, and an upload whose metadata said `favorite: false` favourited the photograph, which is the bulk-import case the SDK issue cites. The Rust client emits exactly these, so it was reachable from a shipped client. This could not be fixed on this side: the fields are declared in `@imogen/shared`, and redeclaring them here would put a second copy of the contract in the repository that does not own it. The pin is the fix. imogen-server#101 fixed the two booleans declared in this repository's own schemas, `includeHidden` and the vault's `covers`, behind a local `WireBoolean` written to be swappable for the SDK's export once it was pinned. It is not swapped here. The SDK's is a bare `z.union` with no custom message and no `.openapi()` annotation, so deferring to it would regress a refusal to zod's "Invalid input" — naming none of the spellings that would have worked, which `faces/api.test.ts` pins — and would publish the parameter as a four-branch `anyOf` instead of a boolean, which `app.test.ts` pins. Zod cannot retrofit a message onto a built union, so keeping either means building the union here. The parse semantics were already identical and stay so; the comment there now says why the copy survives instead of claiming the SDK has not got the fix yet. The new case in `sdk-contract.test.ts` drives the real client against the real server, which is where the two halves meet and the only place this defect is visible end to end. It was watched failing against the old pin — `?trashed=false` returned exactly the trashed photograph — and asserts on ids rather than counts, because a filter that was simply ignored would return the same count as one that works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the pin move: picking up imogen-sdk#36 quietly changed the published
`openapi.json`. `favorite`, `archived` and `trashed` on `GET /assets` went from
`{"type":["boolean","null"]}` to a four-branch `anyOf` — boolean, the empty string, JSON
null, and the four spellings — because the SDK's `WireBoolean` is a bare union with no
document annotation, while `includeHidden` and the vault's `covers` carry one and went on
publishing `boolean`. One idea, two shapes, in a document that is a published artifact:
a generated client would type these parameters as a union including null, and the
reference page would offer `?trashed=null`.
`documentWireBooleans` annotates the SDK's own field schema rather than restating it —
the thing annotated is whatever `shape[key]` already holds — so the contract stays in
`@imogen/shared` and nothing about what the server accepts moves. Only `GET /assets` needs
it: share's timeline routes are plain Hono handlers that contribute nothing to the
document, and the vault's `covers` is declared here.
It annotates with `.meta()` rather than `.openapi()`, which is why it is a function and
not three call sites. `.openapi()` is not zod's — `@hono/zod-openapi` patches it onto the
prototype on import, so a schema built in another package has it only if that import
happened to run first. It does under `bun run` and does not part-way through a full
`bun test`: the first attempt here passed `app.test.ts` alone and threw
`field.openapi is not a function` across six suites in the whole run. `.meta()` is zod's
own and produces a byte-identical parameter schema.
The document assertion in `app.test.ts` now covers an SDK-declared parameter as well as
the two local ones, and checks a neighbouring string field to catch an annotation that
flattens more than it should. That test is also what will catch a wire boolean the SDK
adds later, since a new one is not in the named list and would land as an `anyOf`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: `expect(notFavorite.items).toHaveLength(11)` could not fail for the reason it was written. Every fixture photograph is a non-favourite, so eleven is what a working filter, an ignored filter and an absent one all return — the test's own comment claimed ids were named to avoid exactly that trap, and then named them only for `trashed`. Now one photograph is favourited, `favorite=false` has to exclude it, and `favorite=true` has to return it alone; none of those hold if the parameter is ignored. `documentWireBooleans` takes `keyof T['shape']` rather than `string`, so an SDK that renames or drops one of these fields fails `bun run verify` instead of throwing at `createAssetRoutes()` — that is, at server boot. Two other review findings were checked and not acted on, both wrong about this tree: `/api/v1/timeline` and `/timeline/bucket` were reported as publishing `covers` and the filter flags as an `anyOf`, but those two paths publish no query parameters at all, before this branch as well as after — a real documentation gap, and a pre-existing one that this PR does not touch. And `.meta()` was reported to replace a field's registered metadata; in zod 4.4.3 it merges, so a `.describe()` the SDK adds later survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Retraction. The previous commit claimed `/api/v1/timeline` and `/timeline/bucket` publish no query parameters and dismissed a review finding on that basis. That was wrong, and wrong for a dull reason: those routes are mounted under `/api/v1/assets`, so the paths are `/api/v1/assets/timeline` and `/api/v1/assets/timeline/bucket`, and the probe that "confirmed" it had simply asked the document for a path that does not exist and been handed undefined. Asked properly, both publish `favorite`, `archived` and `trashed` as the four-branch `anyOf`, and the timeline publishes `covers` that way too — while `/vault/timeline` publishes `covers` as `boolean` in the same document. The reviewer was right twice and was talked past twice. Both routes now get `documentWireBooleans`, and `app.test.ts` asserts all three paths rather than one, so the next such claim has to survive a test rather than a grep. The two defects the pin move fixes that nothing yet exercised: - `AssetUploadMetadata.favorite` off a multipart body — the bulk-import case imogen-sdk#36 leads with, where an importer setting the flag honestly on every line favourited the whole library. Watched failing on the old pin: the photograph came back `favorite: true`. - `covers` on the asset timeline, which decides whether a bucket carries a cover id at all, so `covers=false` was building every cover anyway — the expensive half of the query, silently on. Watched failing on the old pin. `/vault/timeline` was never affected, its `covers` being local and fixed in imogen-server#101, which is why the vault suite stayed green throughout. Still outstanding, and deliberately not fixed here: the same fields nested in JSON request bodies (`AssetSelection.query`, six routes) publish the `anyOf`. Zod refuses `.extend()` on a schema carrying refinements and points at `.safeExtend()`, so annotating that one means reworking the selection contract shared by assets, albums and the vault. That is a refactor, not a pin move, and it belongs in its own PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last place the pin move changed the document. `AssetSelection.query` is `AssetFilter`, so the bodies of the bulk mutations — assets trash and restore, the album pair, the vault pair — advertised `""`, JSON null and `"0"`/`"1"` as spellings for `favorite`, `archived` and `trashed`; `UploadSessionCreate` did the same for `favorite`, that one being `AssetUploadMetadata` extended. A body is the worse half of this: a query string has only text to offer, but a client writing JSON has real booleans to hand and was being told to consider the alternatives. `.safeExtend()` rather than `.extend()` for the selection: zod refuses `.extend()` outright on an object carrying refinements, and `AssetSelection` carries the one enforcing exactly one of `assetIds` or `query`. The refinement, and its message, survive `.safeExtend()` — checked against the real schema across all four accept/refuse cases before this was written, since silently dropping that check would turn a 400 into a mutation of the whole library. The upload body needed no such care: `favorite` is a plain top-level field. `app.test.ts` now pins the body shapes as well as the parameters, so all three surfaces that the SDK's `WireBoolean` reaches are asserted rather than one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.safeExtend()` preserving `AssetSelection`'s `superRefine` was checked by hand and then trusted, which is the wrong half of the repository's own rule: the whole reason that call is `.safeExtend()` and not `.extend()` is that the latter drops refinements. If a zod bump ever loosened it the failure would be silent and expensive — a body carrying both `assetIds` and `except` would be accepted, and `selectionConditions` takes the `assetIds` branch without reading `except`, trashing exactly the photographs the caller asked to spare. Now asserted: the empty selection and the both-branches selection are refused, and a `"false"` inside the annotated filter still parses by its spelling. Also corrects a comment that said `AssetQuery` is what parses on the list route. It is a clone of it carrying document metadata — same fields, same rules, different object — and a reader chasing a parse discrepancy should not be sent looking at the wrong one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
ergofobe
added a commit
that referenced
this pull request
Sep 15, 2026
Moves the SDK pin from main@cb814ad to the v0.5.1 tag, and bumps every version
location. A consumer release must name an SDK release, so the pin had to stop
being a main commit before this could be tagged; v0.5.1 exists for that reason
and carries nothing but the fix below.
A patch: both changes since v0.6.0 are bug fixes.
What it ships:
- includeHidden=false was read as true, so hidden people were returned to
every client on every call (#100). Fixed server-side in #101, because that
schema is declared here rather than in @imogen/shared.
- The same defect in the SDK's own schemas meant ?trashed=false returned only
trashed assets, ?favorite=false only favourites, and an upload carrying
favorite=false was favourited (#102). That half could only be fixed by
moving the pin, which is what this release delivers to users.
The ^0.5.0 ranges still admit 0.5.1, so no manifest range moves and bun.lock
needs no regeneration -- verified both before and after.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves
imogen-sdkfrom thev0.5.0tag tocb814adon SDKmain— "Read a boolean onthe wire by its spelling, not by truthiness (#45)", which closes imogen-sdk#36.
This closes a live correctness bug in the shipped v0.6.0.
AssetFilterandTimelineQueryread their flags withz.coerce.boolean(), which calls every non-emptystring true, and every port writes the filter as the string
"false". So today:GET /api/v1/assets?trashed=falsereturns only trashed assets — the exact inversionof the request
?favorite=falsereturns only favouritesfavorite: falsefavourites the photograph,which is the bulk-import case the SDK issue leads with
?covers=falseon the timeline builds every cover anyway — the expensive half of thequery, silently on
The Rust client emits all of these, so it was reachable from a shipped client. It could not
be fixed on this side: the fields are declared in
@imogen/shared, and redeclaring themhere would put a second copy of the contract in the repository that does not own it. The
pin is the fix.
This PR closes no issue, which the rules allow for a pin move. The defect it fixes is
tracked in imogen-sdk#36, already closed there by the SDK commit this pins.
The pin
A between-releases pin: a commit on SDK
main, not a tag.git submodule statusreadsv0.5.0-1-gcb814ad, andmerge-base --is-ancestor cb814ad origin/mainsucceeds. Legal now;a release must move this to an SDK tag first, so a v0.6.1 needs an SDK tag cut before it.
Tests that moved, and why
No existing test asserted the old wrong behaviour — nothing had to be rewritten. Four cases
were added, each watched failing against the
v0.5.0pin first:a false filter on the wire means false, not true?trashed=falsereturned exactly the trashed photographuploading with favorite=false does not favourite the photographfavorite: truecovers=false on the timeline means no coversapp.test.tsThe first three live in
packages/server/src/api/sdk-contract.test.ts— the only place thetwo halves meet, and a pin move is when it earns its keep. It passes. Assertions name ids
rather than counts throughout: with one photograph of twelve marked, eleven is what a
working filter and an ignored one both return, so a count alone could not fail for the
right reason.
The local
WireBooleanis not swapped for the SDK'simogen-server#101 added a local
WireBooleanwritten to be swappable once the SDK's waspinned. Checked, and deliberately kept — the swap is not clean, and both losses are pinned
by existing tests:
z.union, so a refusal reads"Invalid input", naming none of the spellings that would have worked.faces/api.test.tsasserts the message. Zod cannot retrofit one onto a built union, sokeeping it means building the union here.
.openapi({ type: 'boolean' }). The SDK carries no annotation — it has noOpenAPI document to generate — and without it the parameter publishes a four-branch
anyOf.app.test.tsasserts the published shape.Parse semantics were already identical and stay so. The comment there now explains why the
copy survives instead of claiming the SDK has not got the fix.
Found during review, and fixed here
Picking up the SDK's fix silently regressed the published
openapi.json: the SDK'sWireBooleanis un-annotated, so every field reaching the document through it beganpublishing a four-branch
anyOf(boolean | "" | null | "0"|"1"|"true"|"false") while thelocally-declared
includeHiddenand vaultcoverswent on publishingboolean— one idea,two shapes, in the same document. A generated client would have typed these as unions
including JSON null.
documentWireBooleansre-annotates the SDK's own field schema rather than restating it,so the contract stays in
@imogen/sharedand nothing the server accepts moves. It nowcovers all three surfaces: the
/assetsand both timeline queries, the four bulk-mutationbodies via
AssetSelection.query, and the upload session body.Two mechanics worth flagging for review:
.meta(), not.openapi()..openapi()is not zod's —@hono/zod-openapipatches it onto the prototype on import, so a schema built in anotherpackage has it only if that import happened to run first. It does under
bun runand doesnot part-way through a full
bun test: the first attempt passedapp.test.tsalone andthrew
field.openapi is not a functionacross six suites in the whole run..safeExtend(), because zod refuses.extend()outright on anobject carrying refinements and
AssetSelectioncarries the one enforcing exactly one ofassetIdsorquery. Dropping it would be silent and expensive — a body with bothassetIdsandexceptwould be accepted andexceptnever read, trashing thephotographs the caller asked to spare — so that refinement is now asserted rather than
trusted.
Found, not fixed
POST /api/v1/assetsbody is a hand-written mirror ofAssetUploadMetadata(api/assets.ts), so it publishesfavorite: stringwhere thehandler parses a wire boolean, and a generated client sending
favorite=yesgets a 400the document never warned about. The mirror has already drifted independently: the
handler reads
str(form.filename), butfilenameis absent from the declared schema, soa documented-only client can never send it. Pre-existing; not touched here.
.meta()versus.openapi()metadata merging. Review suggested.meta()replaces afield's registry entry, which would silently drop a
.describe()the SDK adds later. Icould not reproduce that on the locked zod 4.4.3: chaining
.meta({type})onto a schemacarrying
.describe()and.meta({example})returned all three. Left as is; worth asecond opinion.
Verify
Run as one command, from the worktree:
biome and tsc clean, web build green, and no
bun.lockdiff — the three nested@imogen/sharedlines thatbun install --forcerewrites were discarded, as CONTRIBUTINGrequires.
🤖 Generated with Claude Code