Uh oh!
There was an error while loading. Please reload this page.
State SaveReportInput's requirements at the reports.save door - #12421
Merged
Conversation
….save door
`IReportService.saveReport` takes a `SaveReportInput`, on which `name`,
`object` and `query` are all required. Nothing on the path said so: the SDK
method declared its parameter `any` and the route forwarded `req.body ?? {}`
unchecked, so the requirement held only as far as each reports implementation
chose to re-derive it privately.
- `client.reports.save` now takes `SaveReportInput` instead of `any`.
- `POST /api/v1/reports` refuses a body missing any of the three required keys,
and a `query` that is not a `ReportQuery` envelope, with 400 /
VALIDATION_FAILED — ordered after the existing 501 for an unmounted service.
The query-less literal that `client.test.ts` had been constructing invisibly is
preserved verbatim and becomes a `@ts-expect-error` pin asserting the refusal.
The REST pass-through test is re-driven through a door-valid body so it keeps
pinning the service-raised VALIDATION_FAILED mapping instead of going vacuous.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCdThe door check wrote its own 400 body. That put a second VALIDATION_FAILED construction site on one route — so the same refusal could reach a client in two different envelopes depending on whether the door or the service raised it — and added two non-conforming bodies to the `check:route-envelope` ratchet, which only ticks down (stringError 46 vs 44, siblingCode 71 vs 69). It now throws `VALIDATION_FAILED: …` from inside the existing try, so the route's single `handleValidation` builds the body exactly as it already did for a service-raised refusal. No new response body; the ratchet is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
Contributor
📓 Docs Drift Check
What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Aug 26, 2026
Open
This was referenced Aug 26, 2026
Merged
This was referenced Aug 26, 2026
This was referenced Aug 27, 2026
Merged
os-litant
marked this pull request as ready for review
August 27, 2026 04:15
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Fixes#11926
IReportService.saveReporttakes aSaveReportInput, on whichname,objectandqueryare all required. Nothing on the path said so. This states it at the door.What I measured (every line number in the card had moved)
Re-measured on
origin/main@cdbd9204b. The card was measured at1f6d04703; three of its four coordinates are stale, and I report what I measured rather than reconciling:cdbd9204bpackages/client/src/index.ts:4185packages/rest/src/rest-server.ts:10293packages/client/src/client.test.ts:352packages/spec/src/contracts/report-service.ts:89The reproduction, before any fix. Binding the parameter to
SaveReportInputand leaving the test untouched reproduced the card's evidence exactly — same code, same message, moved line:All three halves of the card's central claim hold, verified independently: the SDK parameter was
any; the route forwardedreq.body ?? {}unchecked; and there is no schema at the door — zero report Zod schemas inpackages/spec/src, against a positive control of 171 hits forMetadataTypeSchemaon the same instrument, so the zero is a reading rather than a broken grep.One thing the card overstates, and reviewers should have it. The card says a query-less report is "refused, stored half-built, or throws depending entirely on which reports implementation is mounted". The bundled implementation does not leave it open —
packages/plugins/plugin-reports/src/report-service.ts:398already refuses:So no deployment running the bundled reports service changes behaviour; what moves is the layer that produces the refusal. The defect is real and is exactly the three halves above — the requirement was re-derived privately by each implementation instead of being stated once at the seam — but the runtime blast radius is third-party
IReportServiceimplementations, not the default stack.The fix
client.reports.savetakesSaveReportInputinstead ofany. Omitting a required key is now a compile error at the call site. The SDK adds no runtime validation — it is a transport, not a second validator.POST /api/v1/reportsrefuses a body missingname,objectorquery, and aquerythat is not aReportQueryenvelope, with400/VALIDATION_FAILED— so a JavaScript orcurlcaller that never sees the type is refused too.Two ordering decisions worth review:
501for an unmounted reports service. "No reports service on this deployment" is a deployment fact and outranks anything about the body. The existing 501 test pins this.VALIDATION_FAILED: …into the route's existinghandleValidationrather than writing its own body. Writing a second body made one route answer the same refusal in two different envelopes depending on who raised it, and put two new non-conforming bodies on thecheck:route-enveloperatchet, which only ticks down (stringError46 vs 44,siblingCode71 vs 69). Routing through the single existing construction site keeps the ratchet flat and the two 400s byte-identical. That gate is green here; the second commit is the conversion.The fixture at
client.test.ts— reasoned about, not repairedPer the card and the claim, the query-less fixture is evidence, not noise, and adding a
queryto it would have silenced the signal without closing anything. It is neither silenced nor left alone:reports.save pins POST /reports) keeps its job and now runs on a contract-valid input, which is what it was always actually asserting.@ts-expect-error.That turns the evidence into a bidirectional pin.
client.test.tsis compiled bytsconfig.test.json, which this package'stypecheckscript names, and it carries notest-typecheck-debt.jsonentry — so it is not a phantom@ts-expect-errorin a file no tsc program reads. Widen the parameter back toanyand tsc reds withTS2578: Unused '@ts-expect-error' directive(measured, below).One REST fixture needed the same triage:
POST /reports surfaces VALIDATION_FAILED as 400drovebody: {}, which the door now refuses before the service is consulted. Its assertions would have stayed green while pinning the door instead of the pass-through they exist for — green for the wrong reason. It is re-driven through a door-valid body with a service-level throw, and now assertssaveReportwas actually called so it cannot go vacuous silently again.Reverse verification
Both legs mutate, prove the mutation landed on disk by grepping the injected and removed text and comparing blob hashes, run, then restore under a
trapwith absolute paths — restore proved bygit diff HEADempty and a byte-identical blob hash, not by an exit code. Neither leg needs a rebuild: both tests import the mutated file through a package-local relative specifier (./index,./rest-server), not through a dependency'sexports, so nodistis in the resolution path.anyTS2578 Unused '@ts-expect-error' directiveatclient.test.ts:402(+TS6196unused import)2 failed / 226 passed (228)— exactly the two refusal pinsThe "accepts an empty query object" pin correctly stays green under the second leg: it asserts non-refusal, so ablating the refusal cannot break it. That asymmetry is the point of having it — it guards the door against drifting from "present and an object" into "present and non-empty".
Verification
All on
9d4791c2d, the final commit.pnpm --filter @objectstack/rest test— 147 files / 2331 tests passedpnpm --filter @objectstack/client test— 25 files / 347 tests passedpnpm --filter @objectstack/client typecheck— green; gate's own line:check:test-typecheck: OK — @objectstack/client's test layer compiles under packages/client/tsconfig.test.json; 0 file(s) / 0 error(s)pnpm --filter @objectstack/rest typecheck— green.packages/rest/tsconfig.jsonexcludes**/*.test.ts, and--listFilesreturns 0 hits forrest.test.ts, so that green says nothing about the edited REST test file. It does not need to — the REST pins are runtime assertions; only the client pin is type-level, and that file is in a program.pnpm lint(full repo,eslint . --no-inline-config) — green, no narrowing claimednode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(5 changed paths, merge basecdbd9204b). Green:check:route-envelope,check:dispatcher-error-vocabulary,check:authz-resolver,check:empty-changeset,check:changeset-gate-self-tests,check:objectui-changeset,check:published-files,check:page-declaration-shape,check:slot-lookup,check:test-source-alias,check:type-source-resolution,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check:type-check-coverage,check:nul-bytes,check:skill-examples,check-comment-mask-adoption,check-adr-0087-registration,check-changeset-no-major,check-cross-package-test-inputs,check-plugin-teardown-shape,check-ci-filter-parity,check-affected-docs, andcheck:type-check-debt --re-measure(32 ledger entries, none above recorded — run after building the closure, since it refuses outright on an unbuilt one rather than measure a different world).Scope
packages/spec/src/contracts/report-service.tsis untouched — the contract is being honoured, not changed. The six adjacentany-typed SDK methods are untouched. No new issue filed for them: the card already lists them, and re-filing would duplicate it.needs:contract-reviewgate on the card. The reviewable decision is precise: anIReportServiceimplementation that treatedqueryas optional and defaulted it can no longer receive a query-less body, even though the contract it implements has always declaredqueryrequired. That is the accept/reject change, named rather than assumed away.Generated by Claude Code