Uh oh!
There was an error while loading. Please reload this page.
feat(client): let meta.saveItem send the query string its route already reads - #11714
feat(client): let meta.saveItem send the query string its route already reads#11714claude[bot] wants to merge 2 commits into
Conversation
…dy reads The Phase 3a-destructive gate refuses with `409 DESTRUCTIVE_CHANGE` and ends `— re-submit with ?force=true to proceed.` Both REST `PUT` doors read `?force` and thread it, so the clause is true of an HTTP caller. It was false of a first-party SDK caller: both `saveItem` declarations built a bare path and a body and sent no query string at all, so doing exactly what the refusal said returned the identical refusal and the only remedy was raw `fetch`. Adds an optional `SaveMetaItemOptions` bag — `force`, `packageId`, `mode` — matching `getItem`'s house shape on the same object and covering exactly the three parameters `PUT /api/v1/meta/:type/:name` reads. One exported type and one query builder are shared by the unscoped client and the environment-scoped twin so the two cannot drift; an options-less call builds a byte-identical URL to before. Fixes#11391 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
📓 Docs Drift CheckThis PR changes 1 package(s): 26 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 3 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 9d86fc6266aeac7f31a29fcff307ba25d3f5a2f5 && git checkout 9d86fc6266aeac7f31a29fcff307ba25d3f5a2f5
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5f124a219c038dbcb941273df3ff7e85f2672bf4 228c54d4b4780629d0a1e3e9a2fc2bea499a17d2 && git checkout -B drift-repro 5f124a219c038dbcb941273df3ff7e85f2672bf4 && git merge --no-ff 228c54d4b4780629d0a1e3e9a2fc2bea499a17d2
node scripts/docs-audit/affected-docs.mjs --json 5f124a219c038dbcb941273df3ff7e85f2672bf4
|
os-zhuang
commented
Aug 24, 2026
PM review — substance ACCEPTED. One small change requested before arming, one note.
What I verified myself
⭐ The honest accounting of the four non-red pins is the best thing in this report and I checked it rather than taking it. ⭐ Also worth recording: you discarded a failing instrument instead of trusting its zero.
|
Review follow-up. This PR exposes `mode` on a client that addresses BOTH
`PUT` doors, and `PUT /meta/:type/:section/:name` never reads the parameter
while its single-segment twin does — so a compound-name save with
`{ mode: 'draft' }` is ignored and PUBLISHED LIVE, answered 200, with no
signal at the call site. That is this card's own defect shape one parameter
over, and it became reachable only because this PR made `mode` settable.
Measured over the compound handler's body (rest-server.ts 6646-6824): `force`
and `package` ARE both read and threaded there; `mode` is the only one of the
three that is not (zero hits, reverse-checked with `compoundName`). The
docstring therefore names the gap narrowly and states why the remedy is NOT
to refuse the whole bag on a compound name — that would break the two
parameters that work in order to warn about the one that does not. Threading
it is the route's decision, filed as #11712 rather than guessed at here.
Also renames the two new locals from `qs` to `query`. Measured on this file:
of 37 `const qs =` bindings, 27 hold a bare `params.toString()`, 8 hold a
string already carrying its `?`, and 2 hold a `URLSearchParams` object — one
name, three meanings. These carry their `?`, so they say so.
Comments and local names only; no behaviour change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqRReview addressed at The measurement you asked for first: |
| Parameter | Read on the compound door? | Threaded? |
|---|---|---|
force | yes — const forceRaw = req.query?.force (+117) | yes (+166), since #11095 |
package | yes — const packageRaw = req.query?.package (+121) | yes (+167) |
mode | no — zero hits over the whole handler | no |
Zero-hit discipline held: the mode zero was reverse-checked over the same 6646-6824 slice with compoundName (2 hits, +62 and +141) — independently known present because this door is defined by building a compound name from two segments, established by a different grep than the one under test, and not a substring of mode. Contrast leg included: the single-segment twin does read it ('force', 'package', 'mode' in its repeated-param list at 5469+52, threaded at 5469+152). Full block is in the PR body.
That result changed the docstring I wrote. Because force and packageId both work on the compound door, the note explicitly forbids the tempting "defence" of refusing the whole options bag on a compound name — that would break the two parameters that work in order to warn about the one that does not. Without the package measurement I might well have written that guard.
The mode docstring
Names the gap, says it is answered 200 and published live with no call-site signal, points at #11712, and says why it is not repaired from the client (threading it is the route's decision; a client-side guess would be a second place the two doors disagree).
The qs note — taken, but the reason is stronger than "different in two places"
I started to write "every other qs here is a bare params.toString()" and then measured it, which was wrong: of 37const qs = bindings in this file, 27 hold a bare params.toString(), 8 hold a string that already carries its own ?, and 2 hold a URLSearchParams object. One name, three meanings — a reader genuinely cannot tell from ${qs} whether a ? is already there, and guessing wrong builds …name??force=true or …nameforce=true. The two new locals are query, and the comment states the measured census rather than the claim I nearly shipped.
Verification at the new head
Rebuilt the dependency closure first (the worktree was recreated, so dist/ was absent — judging anything before that would have been reading stale output). All at 228c54d4b, clean tree:
pnpm --filter @objectstack/client test—Test Files 24 passed (24)/Tests 330 passed (330)pnpm --filter @objectstack/client typecheck— OK- full repo
eslint . --no-inline-config— 5159 files, 0 errors, 0 warnings - ratchet families re-run at the new head:
slot-lookup,query-options-erasure,where-matcher,engine-double-contract,type-check-coverage,published-files,test-source-alias,type-source-resolution,cross-package-test-inputs— all green, all reporting "no files added" against their baselines - changeset gates +
check:nul-bytes— green
No new pins: this change is comments and two local names, so there is no behaviour to pin and nothing new to show failing. The 13 existing pins still pass unchanged.
One thing to know about this PR's body
Editing it appended a second, bare attribution footer beneath my session-URL one — a live instance of #11273. The body now carries both. I left it rather than PATCHing again, since another edit is what produces another one; my session-URL footer (the form that survives) is present and correct. Flagging it so it is not read as drift.
Still draft, auto-merge not enabled, needs:contract-review untouched.
Generated by Claude Code
Generated by Claude Code
os-zhuang
commented
Aug 24, 2026
Follow-up verified. Both items land as asked, and the measurement earned its keep.
⭐ The Zero-hit discipline on the ⭐ The No new pins, said plainly. Comments and two local names cannot have a red-before leg, and you said so rather than padding with a vacuous one. Correct. CI at |
| green | 29 |
legitimately skipped by filter | 2 (Console Pin Gate, Build Docs) |
| still running | 1 — Test Core (1/6) (id 97465504375) |
| cancelled | 1 — No other open PR may claim the same single-writer path (id 97465426005, 14:20:36) |
cancelled means the gate did not run, and a gate that did not run is not measured — glossing that as "all green" is the reading error this repo's discipline forbids. It is discharged here for a specific reason, not waved through: a later run of the same-named check (id 97465738279, 14:20:50) completed success. Superseded duplicate, measured in the newer run. Same shape recorded on #11387 earlier today.
What is left, and it is not yours
Test Core (1/6)to finish — I read it by name, not off a rollup.needs:contract-review. Clause ② isyeshere (three newly-exposed SDK parameters), the label is hung on this PR and on card The first-party SDK'smeta.saveItemsends no query string at all, so the?force=truethe destructive 409 prescribes is unreachable through@objectstack/clienton either REST door #11391, and ⛔ this seat does not clear it — the review chain does. Green CI does not arm this PR; the gate does.
⛔ Nothing further requested. Do not mark ready, do not arm.
Noted separately: your attribution-footer readings, including the one you found and corrected on your own filings, are now consolidated on #11273 — I added the edit-path leg, where the strip disguises itself as de-duplication and my own write-up on #8343 got the mechanism wrong until your create-path measurement corrected it.
Generated by Claude Code
os-zhuang
commented
Aug 24, 2026
CI half discharged. ⇒ The only thing between this PR and arming is ⭐ For whoever picks this up, including a later instance of this seat: read the chain's verdict comment, not the label. A cleared gate and a gate never applied look identical on the label alone, and an absent gate is a release, not a red light. On the verdict: re-confirm the head has not moved off Generated by Claude Code |
os-zhuang
commented
Aug 24, 2026
⛔ HOLDING — |
| reading | result |
|---|---|
| PR labels | documentation, size/m, tests, tooling — needs:contract-review absent |
| card #11391 labels | pm:dispatched, domain:cli — absent there too (symmetric) |
| PR comments | 5 total; the last is mine, 5396739324 at 14:34:41Z |
get_reviews | [] |
get_review_comments | 0 threads |
| card #11391 comments | last is the dev's report, 13:49:27Z |
| head | unchanged at 228c54d4b |
No verdict. No review. No review thread. On either object.
Why this is a hold and not a release
The state model says an absent gate label is a release, not a red light — 「被剥不是红灯是放行」 — and that is the reading I would normally apply without hesitation. It does not apply here, for a reason the same clause gives: 「被剥」與「從未掛過」在證據上不可區分. Read-back is the only way to notice, and read-back has now noticed an absence with nothing behind it. 「⛔ 禁止自查放行」 forbids clearing without a review; a clear with no recorded review is indistinguishable from exactly that.
⭐ And the decisive reason is about me, not the label: this seat is the party that gains from the gate being clear. That is the standing warning on this seat's own post — a seat that would gain from a gate not applying is not neutral about whether it applies — and it was written after this seat got it wrong once already. Reading an unexplained absence as permission, when permission is what I want, is the failure it names.
The asymmetry settles it. Arming lands a contract-tier change (three newly-exposed published SDK parameters) with no recorded review — the precise harm the gate exists to prevent. Holding costs an hour.
What this is NOT
⛔ Not a claim that anything improper happened, and ⛔ no actor is inferred. I have no timeline read that attributes the removal, and after getting an attribution badly wrong earlier today on this same lane I am not guessing from an absence. Three other lanes' PRs still carry needs:contract-review right now (#11767, #11743, #11716), so this is not a global strip — which is itself evidence for a deliberate, targeted clear, i.e. most likely the chain ran and simply left no record.
⛔ Not re-hanging the label either.#11399 is a filed finding about exactly that error — a cleared gate re-hung as "dropped" by a checker that read labels and review objects but not the verdict comment. Re-hanging a spent condition reinstates an expired premise as a current one, which is the mirror mistake and no better.
What unblocks this
Any one of:
- The chain (or whoever cleared it) posts its verdict here — then I re-confirm the head, flip ready and arm immediately.
- The maintainer says the clear stands — same outcome, and the provenance goes in this thread.
- Someone points me at where verdicts are actually recorded, if it is not the PR thread. That would make this a documentation gap rather than a missing review, and I would say so.
Raised with the maintainer in this lane's report. ⛔ Not nagged further; the PR sits green and unarmed until one of the three lands. The same hold applies to #11731 (cards #11683 / #11684), whose gate cleared the same way in the same window.
Generated by Claude Code
Fixes#11391
The Phase 3a-destructive gate refuses a metadata save with
409 DESTRUCTIVE_CHANGEand ends the message— re-submit with ?force=true to proceed.Both RESTPUTdoors read?forceoff the query string and thread it, so that sentence is true of an HTTP caller. It was false of a first-party SDK caller: bothmeta.saveItemdeclarations built a bare path and a body and sent no query string at all. A caller who did literally what the refusal prescribed got the identical refusal back, forever, and the only way to act on it was to abandon@objectstack/clientfor rawfetch.This is Branch A of the maintainer's premise-hung ruling on #11391, selected by the measurement below.
Step 1 — the measurement that selected the branch
The ruling hung the branch on a named falsifiable premise: measure whether Studio and the CLI perform metadata saves through
meta.saveItem(vs raw HTTP). Premise TRUE ⇒ A; FALSE ⇒ C; neither cleanly ⇒ report the fork and build nothing.Result: premise TRUE, decisively, on both named surfaces.
Surfaces measured
packages/cli(this repo)client.meta.saveItem— the only metadata-write pathobjectstack-ai/objectui@e52fac0(public, cloned read-only)client.meta.saveItem— 21 production call sitesNo surface was unreachable. The Studio frontend is not in this repo; it was measured in a real read-only clone of the sibling
objectuirepository rather than assumed. Every command and its raw output is below.Tree A —
objectstack-ai/objectstack@4ceae8ab0os meta register— the CLI's metadata-write command — is a thin wrapper: read a JSON file, takemetadata.name, callclient.meta.saveItem(type, name, metadata). There is no second path.Zero-hit claim: the CLI performs no raw-HTTP metadata save.
Reverse-check of that zero hit — with a term independently known present in the searched tree, and not a substring of the term under test (
meta/).api/v1/auth/device/codewas surfaced by a separate grep forfetch(over the same tree before this claim was made, so its presence was established independently:The instrument produces positives over
packages/cli/src, so the zero above is a real negative. (The CLI's otherfetchcall sites are auth, datasource introspection, and plugin/package publish/install — none of them a metadata save.)Tree B —
objectstack-ai/objectui@e52fac0It is
@objectstack/client— this package — that they consume.objectuistates the same finding about itself, in a comment that enumerates the callers exhaustively (packages/data-objectstack/src/index.ts:2300):The SDK is specifically a destructive-edit path, not merely a save path
This is the part that decides A vs C, so it was measured rather than inferred. Studio's field designer replaces an object's whole
fieldsarray and saves it back through the SDK (MetadataService.saveFields,app-shell):Removing a field in the designer and saving is exactly what raises the destructive 409.
deleteObjectlikewise writes{ enabled: false, _deleted: true }throughsaveItem. So the refusal an operator hits in Studio is raised on a save that came through the door this PR repairs.Two honest complications, reported rather than smoothed over
The grep for raw-HTTP
PUTs inobjectuidid not come back empty, and neither hit is scored as clean:packages/data-objectstack/src/metadata-client.ts:809— a hand-rolled second client class,MetadataClient.save, which composes exactly the three query parameters this PR adds:MetadataClientSaveOptionsalso settles themodespelling:'draft' | 'publish', where'publish'emits nothing. This PR matches it.packages/app-shell/src/views/metadata-admin/external/api.ts:189—importObjectDraftdoes one raw-HTTPPUTto/api/v1/meta/object/:namewith no query string, on the external-datasource import path. One call site, and it sends no parameters at all, so it is not a caller that chose raw HTTP to get the query string.Neither moves the premise off TRUE: the question the ruling asked is whether the SDK is a real destructive-edit path, and it is the only path for the CLI and the path for 21 of 22 metadata saves in Studio.
This repo's own QA checklist independently records the defect from the other side — it instructs its authors to drop out of the SDK precisely because of the missing query string (
docs/qa/platform-checklist/areas/attachments-storage.json:61):Follow-up measurement — which of the three parameters reach the COMPOUND door
Asked in review, because this PR exposes
modeon a client that addresses bothPUTdoors and one of its own pins shows the client building a compound-name query string. The census is over the compound handler's body,packages/rest/src/rest-server.tslines 6646-6824 (6646this.routeManager.register({, 6648path: ${metaPath}/:type/:section/:name, 6820 its summary):The reverse-check term is
compoundName, independently known present because this door is defined by building a compound name from two path segments; it was established by a different grep than the one under test and is not a substring ofmode.Answer:
?packageIS threaded on the compound door.modeis the only one of the three that is not. So the warning stays narrow and #11712 reads correctly as filed — it is a one-parameter gap, not two.That matters for more than the wording. Because
forceandpackageIdboth reach both doors, the client must not defend against this by refusing the whole options bag on a compound name: that would break the two parameters that work in order to warn about the one that does not. Themodedocstring now says so explicitly, so the next reader does not "harden" it into a regression.Why Branch C was not available even as a fallback
Recorded because it is load-bearing for the selection, not to argue the ruling. C asks for face-aware remedy text that names something an SDK caller can do.
destructiveChangeRemedyswitches onMetadataWriteFace, and that value is stated by the server —packages/metadata-protocol/src/protocol.tssays so and pins it in both face-inventory suites:An SDK save and a raw
fetchsave are the same HTTP request arriving at the same door with face'meta-envelope'. There is no signal to branch on, so "SDK-flavoured wording" is not implementable without inventing a client-kind channel — itself a new surface. The parameter had to become reachable.Step 2 — what changed (Branch A)
One optional options bag on both
saveItemdeclarations, carrying exactly the three parametersPUT /api/v1/meta/:type/:namereads:force?: boolean?force=truefalse/undefinedomit the parameter.packageId?: string?package=<id>getItem/getItemson the same object.mode?: 'draft' | 'publish'?mode=draft'publish'is the default said out loud and sends nothing.Never sending
?force=falseis a hazard avoided, not tidiness: the door refuses a repeatedforcebecause a repeated value arrives as an array and a non-empty array is truthy, so an opt-OUT reaching the wire twice would switch the destructive guard on. A client that cannot emit the opt-out spelling is clear of that edge by construction.modecarries a warning about the compound-name door. The measurement above found that?mode=draftreaches only the single-segment route: against a compound name (saveItem('object', 'views/all_leads', item, { mode: 'draft' })) it is ignored and the write is published live, answered 200, with no signal at the call site. That is this card's own defect shape one parameter over, and it did not exist before this PR becausemodewas unreachable at all. The gap is filed as #11712 and deliberately not repaired from the client: threading it is the route's decision, and a client-side guess would be a second place the two doors disagree. The docstring names it, and also says why the fix is not to refuse the bag on compound names —forceandpackageIdboth work there.One rename, no behaviour. The two new locals are
query, notqs. Measured on this file: of 37const qs =bindings, 27 hold a bareparams.toString(), 8 hold a string that already carries its own?, and 2 hold aURLSearchParamsobject. One name, three meanings — a reader cannot tell from${qs}whether a?is already there. These values carry theirs, so they get a name that says so.Both declarations move together, sharing ONE exported
SaveMetaItemOptionstype and ONEmetaSaveQuerybuilder rather than a literal copied into each. They are the same method on two clients reaching one pair of routes — the scoped mount is the sameregisterForBasecall replayed under/environments/:environmentId, verified inrest-server.ts, so it reads the same three parameters — and a bag spelled twice is the next divergence waiting to be introduced.Backward compatible. An options-less call builds a byte-identical URL to before (
'', not a trailing?), and that is pinned rather than asserted.Anti-vacuity: every pin shown failing without the behaviour
13 pins added. Reverse-verification reverted only
packages/client/src/index.tstoorigin/main, keeping the pins, then restored it via anEXIT INT TERMtrap. The mutation was confirmed on disk by anchored greps in both directions before the run — the injected text absent, the removed text back — because an editing tool's exit code is not evidence a change landed:No rebuild gates this ablation, and that is a property of the wiring rather than an omission:
client.test.tsimports the subject asfrom './index'— a relative source import, not a packageexportsresolution — so nodist/sits between the edit and the run. The restore leg was confirmed the same way (git status --shortempty against the commit).RED — source reverted, pins kept:
GREEN — with the behaviour:
Test Files 24 passed (24)·Tests 330 passed (330).9 of the 13 go red, and the other 4 must not. Those four (
force: falsesends nothing,mode: 'publish'sends nothing, an empty bag, and a 3-argument call) are backward-compatibility guards: they assert the URL is unchanged, so they pass in both states by design. Reporting them as red-before would be false; they earn their place by pinning that this PR did not move the old behaviour, and the 9 above are the ones that measure the new behaviour.The load-bearing pin is on the URL the client builds, deliberately — a pin that only checked the method accepts an option would stay green against a client that swallows it, which is the same defect one layer in.
The remedy loop is pinned end to end: a save is refused, and the caller then does literally what the refusal says. It asserts the envelope (
code+ status) rather than that something threw — a bare.toThrow()stays green against any error at all:One deviation from the house minimum, stated rather than papered over: the assertion reads
httpStatus, notstatus. On this path the SDK parks the numeric onerr.httpStatus(error.httpStatus = res.statusin the shared error builder);err.statusis set only on the auth-login branch, so assertingstatushere would pinundefined === undefinedand pass against anything.Clause ② — yes
This widens the published SDK surface with three newly-exposed parameters, so it is contract-review tier. The changeset names all three (
force,packageId,mode) with their wire spellings and their omission semantics.Checks run
All re-run at
228c54d4b, this branch's head after the review changes — the tree was clean (git status --shortempty) for every run below, so no check reports on a tree that has since moved.pnpm --filter @objectstack/client testTest Files 24 passed (24)·Tests 330 passed (330)pnpm --filter @objectstack/client typecheck0 file(s) / 0 error(s)intest-typecheck-debt.jsonpnpm lint(full repo,eslint . --no-inline-config)pnpm --filter '@objectstack/client^...' buildVERDICT command-exit 0(dependency closure, built before any judgement)check:cross-package-test-inputscheck:query-options-erasurecheck:engine-double-contractcheck:where-matchercheck:type-check-coveragecheck:published-files·check:slot-lookup·check:test-source-alias·check:type-source-resolutioncheck:changeset-gate-self-tests·check:objectui-changesetcheck-adr-0087-registration·check-changeset-no-major·check-empty-changesetcheck-plugin-teardown-shape·release-rehearsal-clone --self-testcheck:nul-bytesThe gate family was re-derived from the actual change set rather than recalled:
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, which reported its own provenance (naming the repo and the commit it derived from) and resolved the change set itself against the merge base.check:type-check-debt --re-measurewas not run locally — it needs the whole workspace closure built and CI runs it regardless.Out of scope, filed unassigned
Both were found while measuring this card and are deliberately not touched here.
PUT /meta/:type/:section/:namenever threads?mode=draft, while its single-segment twin does — the fourth divergence closed, a fifth left open #11712 — the compound-namePUT /meta/:type/:section/:namenever threads?mode=draft, while its single-segment twin does. A server-side route defect inpackages/rest, outside this card's declared client-side surface;?mode=draftagainst a compound name is silently published live and answered 200.meta.saveItem's own docstring tells the caller to echoversionback as anIf-Matchheader — the SDK offers no way to send one, on either declaration #11713 —meta.saveItem's own docstring tells callers to echoversionback as anIf-Matchheader, and the SDK offers no way to send one. The same defect shape as this card, one carrier over. Not folded in: the ruling enumerated exactly three query parameters, and a fourth carrier is a contract decision of its own rather than a mechanical extension of this one.packages/specwas not touched.Generated by Claude Code
Generated by Claude Code