Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) by zhuangjianguo · Pull Request #13569 · objectstack-ai/objectstack · GitHub
Skip to content

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382) - #13569

Merged
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation
Aug 31, 2026
Merged

fix(metadata-protocol): compare OCC version tokens as instants, not spellings (#13382)#13569
zhuangjianguo merged 4 commits into
mainfrom
claude/issue-13382-occ-version-token-normalisation

Conversation

@zhuangjianguo

@zhuangjianguozhuangjianguo commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes#13382

On Postgres — the production default driver — every guarded save answered 409 CONCURRENT_UPDATE, including on records nobody had ever touched, which made the Console record-edit dialog unusable. SQLite development environments were completely unaffected, which is why it survived.

Reproduced, then fixed, against a real PostgreSQL 16

A PostgreSQL 16 server was provisioned in this container and driven end to end through the real ObjectQL engine and ObjectStackProtocolImplementation. This is not a SQLite pass presented as verification — SQLite is shown beside it precisely as the control that stays green either way.

Before, with the card's own TZ=Asia/Shanghai:

### driver-sql / better-sqlite3 (the camouflage)
raw updated_at : String "2026-08-31T01:05:04.408Z"
GET serves : "2026-08-31T01:05:04.408Z"
guarded PATCH : ACCEPTED
### driver-sql / postgres (LIVE)
raw updated_at : Date "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"
GET serves : "2026-08-31T01:05:04.454Z"
guarded PATCH : CONCURRENT_UPDATE 409
409 currentVersion: "Mon Aug 31 2026 09:05:04 GMT+0800 (China Standard Time)"

After, the same harness under three process timezones — Asia/Shanghai, CI's America/New_York, and UTC — reports ACCEPTED on both drivers in all three, and the seven controls below hold on Postgres and on SQLite alike:

controlverdict
uncontested save, ISO token the GET servedACCEPTED
the same token, RFC-7232 quoted (If-Match)ACCEPTED
stale token after a real racing writeREFUSED CONCURRENT_UPDATE
that 409's currentVersion echoed straight backACCEPTED
a pre-fixString(updated_at) tokenACCEPTED (no regression)
a token one millisecond from the recordREFUSED CONCURRENT_UPDATE
an opaque, non-temporal tokenREFUSED CONCURRENT_UPDATE

Where it actually is

packages/metadata-protocol/src/protocol.tsnotpackages/rest / packages/driver-sql, which the card's prose names. Located on 70fe54891e, since the two comments on the card disagree and both were measured on a moving file:

packages/metadata-protocol/src/protocol.ts:1378 function normaliseVersionToken(v: unknown)
packages/metadata-protocol/src/protocol.ts:10012 const expected = normaliseVersionToken(expectedVersion)
packages/metadata-protocol/src/protocol.ts:10015 const currentVersion = normaliseVersionToken(current.updated_at)
packages/metadata-protocol/src/protocol.ts:10037 if (!normaliseVersionToken(expectedVersion)) return; // the DELETE door

The definition is at :1378 as triage said; the call sites had moved to :10012 / :10015, and there is a fourth site — assertVersionMatch, the guarded-DELETE door, which the anchoring note did not name.

The input domain, enumerated from what the drivers actually return

Measured through engine.findOne — the read probeRecord makes — not listed from memory:

what reaches the seam as updated_atdriversnormalised to
JS Datedriver-sql on Postgres (measured, live) and MySQL (withUtcSession sets mysql2 timezone: 'Z'; the driver keeps timestamptz / DATETIME(3) as DatedeliberatelywithPostgresCalendarDayAsText says so); driver-mongodb (stamps new Date(), BSON round-trips it)toISOString()
string, canonical ISO-8601 UTC with msdriver-memory (measured), driver-sql on SQLite (measured), driver-turso, driver-sqlite-wasmitself, re-derived through Date
number, epoch millisecondsa pre-canonical or hand-migrated SQLite column — measured reachable: the driver's legacy-datetime repair keys on declared Field.datetime columns, and datetimeFields[table] is empty for the engine-injected audit columns, so an INTEGER updated_at passes through unrepaired and arrives here as 1756602000123new Date(ms).toISOString()
null / undefinedtimestamps disabled, or no such columnno check runs (unchanged)
anything elsean opaque version a host stamps into the columncompared verbatim (unchanged)

Normalisation lands on the absolute instant and reads no process timezone: only a string carrying an explicit Z or a numeric offset is parsed as an instant. A zone-less date-time such as 2026-08-30 18:19:25.947 would be read as local time, so it is deliberately left opaque and compared verbatim — pinned by its own test.

Blast radius (the Zone 2 question)

normaliseVersionTokenis the only place a record-data version token is compared or emitted. The repo has three other version lanes, and none can carry this defect: metadata items compare a sha256: content hash (parentVersion / ADR-0008), cluster KV compares a monotonic bigint, and metadata HTTP caching uses a content ETag. packages/rest forwards the token and passes currentVersion through without reading it.

But the PM's "one seam, one behaviour" reading is half wrong in a way that matters: the seam has two limbs, and the broken representation was not merely compared internally — it was published. Repairing only the comparison would have left the 409 serving a Date.toString() whose milliseconds are already gone, i.e. a token that can never match the record it names — converting a false conflict into an unresolvable one on the exact path (InlineEditSaveBar and occSave re-key their retry to conflict.currentVersion) that is today the only way Postgres users can save at all. Both limbs are fixed here.

That published change is wire-visible, and it was measured rather than assumed:

  • content/docs/api/wire-format.mdx already documents currentVersion as an ISO-8601 UTC timestamp — the fix moves the implementation toward the documented contract, it does not change it.
  • The pinned objectui sibling treats the token as an opaque echo ({ ifMatch: conflict.currentVersion }) and only formats it for display; nothing there constructs or parses the Date.toString() spelling.
  • No test in either repo pins that spelling — every fixture is ISO or deliberately opaque (v2, rowversion-7).

The accept side is strictly widening, and that is a property, not a hope. When either token is not an instant the comparison falls back to the verbatim strings, exactly as before; so any pair that matched before still matches, and no token a client sends today starts being refused — including a client still holding a pre-fix 409's Date.toString() value across the upgrade (control 5 above, and a test). Only two spellings of one instant change verdict, from conflict to match.

Clause 2 — my own reading

FIRES, and I concur with the PM; no upward revision. It changes when a guarded PATCH/DELETE is accepted versus rejected with 409, and it changes a published wire field — contract accept/reject behaviour on the content limb. No packages/spec/src/** path and no new authorable surface. Worth putting to the reviewer: on both limbs this is a repair toward an already-documented contract, and the accept set only grows — but that is an argument for the review, not a reason to downgrade it. Please review at CONTRACT_REVIEW_TIER before this is flipped ready.

Tests

packages/metadata-protocol/src/protocol.occ-version-token-instant.test.ts — 23 tests. They pin the property, not the spelling: a table of spelling pairs each naming one instant, a three-zone sweep with a non-vacuity control asserting the zones really do move the broken spelling, millisecond-resolution conflicts, the published token round-tripping, and the widening guarantee, checked against the pre-fix comparison rather than asserted (see the patch round below). No existing test was skipped, disabled, quarantined, allow-listed, deleted or re-baselined — the diff touches no existing test file, and the existing OCC pins (objectql/src/protocol-data.test.ts, metadata-protocol/src/protocol.update-path-id-wins.test.ts, rest/src/rest.test.ts) pass unmodified. None of them encoded the broken comparison: every fixture was ISO-on-both-sides, which is exactly why they were green throughout.

Ablation, re-run on the committed implementation at 14c197f87e: canonicalVersionInstant neutralised so every token reports "not an instant", reducing the seam to the old verbatim compare. Mutation proved on disk by grep of both the injected marker (1) and the deleted anchor text (0), and by the blob hash moving 6a08f324 to ddd2081b. Result: 11 of 23 fail — and the 12 that stay green are precisely the ones encoding pre-fix behaviour that must not change (the SQLite ISO-on-both-sides row, the opaque tokens, both empty-token opt-outs). The differential sweep fails on its own non-vacuity assertion — "the corpus must contain at least one pair this change newly accepts" — which is the honest reason: under the mutation the accept set stops growing. No rebuild leg: the suite reaches the subject through a relative source import, proven by the mutation reddening it with no build at all. Restored under trap ... EXIT INT TERM with an absolute repo root, proved by whole-tree git status --porcelain empty, git diff HEAD empty, and the blob hash back to 6a08f324.

Gates

Whole derived family re-run at the final commit 14c197f87enode scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, now 64 commands (the docs edit below pulls in 21 more families), 62 green. Plus the whole-repo pnpm lint (eslint . --no-inline-config, exit 0 — not a narrowed run), pnpm --filter @objectstack/metadata-protocol test (2040 passed, 10 skipped, 0 failed), pnpm check:type-check-debt (29 ledger entries re-measured, none above its recorded number), and check-nul-bytes (7522 files, clean).

Two report PREREQUISITE NOT MET (exit 3), which is NOT MEASURED and not a red: check-test-completeness needs a saved turbo run test log that only CI produces, and check-half-states needs a real GitHub credential this container does not have.

scripts/engine-double-contract.pinned.json grew by 3 rows: the gate retains a pin per (file, verb) and asked for the new suite's double to be recorded — --write, 3 added, 0 lost. That is new pinned coverage, the opposite of a relaxed baseline.

Patch round — two items, one validated push

Both reproduced first, then fixed, then shown passing. Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L

1. check-system-context-census line rot.content/docs/permissions/system-context.mdx anchors elevation reads by line number, and this PR pushed row 21's read down. Verified a pure shift before repairing rather than assumed, because the gate refuses to guess a population change and only rewrites rot: the anchored statement is byte-identical across the move (sha256 db885f75...), the isSystem occurrence count in the file is 9 on both trees, and the new line is the first statement of stripReadonlyForInsert — exactly what row 21 describes. Repaired with the gate's own --fix; no false refusal, so #13490 did not reproduce here. One anchor rewritten, no prose touched, nothing baselined or exempted. It shifted a second time when the remedy below added ten lines above the same function, which the final-commit gate run caught; both re-anchors are folded in.

2. Contract review finding: If-Match: "" flipped ACCEPT to 409. Real, and mine. An empty entity-tag is empty only after the RFC-7232 quotes come off; the pre-fix seam returned the bare string, so that case handed every caller a falsy '' and they short-circuited into "no token supplied". Wrapping the result in an object made it always truthy, so the check began running and '' mismatched verbatim. Remedy is the conservative one — re-apply the emptiness test after the strip, byte-identical to 70fe54891e. Whether an empty entity-tag should be able to disable OCC is a contract question and is filed separately; a p1 bug-fix PR does not silently install a new rejection.

The widening claim is no longer prose. It was believed by three readers while one token had already flipped, so the suite now carries a differential sweep: 8 stored shapes by 15 client tokens, each pair judged against the pre-fix comparison reproduced verbatim, failing on any pair accepted before and refused now. Run against the unfixed tree it reports exactly the five "" pairs and nothing else — which is how the review finding is known to be the whole regression rather than one instance of a class. The suite is now 23 tests, all passing.

What this does not cover

There is no automated live-Postgres coverage of this seam. The Temporal Conformance (live PG + MySQL) job runs pnpm --filter @objectstack/driver-sql test, and @objectstack/metadata-protocol has no driver dependency and must not grow one, so the live evidence above is a measurement I made rather than a pin the repo now carries. Recorded as a finding in #13567 (out of scope here: it is a CI-topology decision, not a bug fix).

…pellings (#13382)
On Postgres — the production default driver — every guarded save answered
409 CONCURRENT_UPDATE, including on records nobody had ever touched. The OCC
gate read `updated_at` through `String(v)`; on a Date-returning driver that is
`Sun Aug 30 2026 18:19:25 GMT+0800 (China Standard Time)` — milliseconds
dropped, process timezone baked in — compared as a string against the
`2026-08-30T10:19:25.947Z` the client echoed back from its own GET. One
instant, two spellings. SQLite returns canonical ISO text, so both sides
matched by accident and development environments stayed green.
Both tokens are now normalised to one representation — a canonical absolute
instant — before comparison, and the `currentVersion` a 409 publishes is that
same canonical instant: what the wire format documents, and the token the
conflict dialog echoes back as its next If-Match.
Strictly widening: when either side is not an instant the verbatim comparison
still runs, so no token accepted before is refused now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
`check:engine-double-contract` retains a pin per (file, verb). The regression
suite added for #13382 carries a fake engine whose delete/update/findOne route
through the producer's own dispatch predicates, so the ledger has to learn
about it or it never protects the file. `--write`, 3 rows added, 0 lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 8 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/concepts/metadata-lifecycle.mdx(via ObjectStackProtocolImplementation (symbol))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via ObjectStackProtocolImplementation (symbol))
  • content/docs/releases/v17.mdx(via ObjectStackProtocolImplementation (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156packageMentionDocs.

Which tree this was computed on

This run read content/docs from ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 — the merge of head 14c197f87ed384e24757413a2362c7687d1e94ac into base 9c120f0308d5aed3c26f53c406054a95a631d156, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ff4d0f267fef8baddf98020fc59a8f15f2ec5c52 && git checkout ff4d0f267fef8baddf98020fc59a8f15f2ec5c52
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9c120f0308d5aed3c26f53c406054a95a631d156 14c197f87ed384e24757413a2362c7687d1e94ac && git checkout -B drift-repro 9c120f0308d5aed3c26f53c406054a95a631d156 && git merge --no-ff 14c197f87ed384e24757413a2362c7687d1e94ac
node scripts/docs-audit/affected-docs.mjs --json 9c120f0308d5aed3c26f53c406054a95a631d156

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 9c120f0308d5aed3c26f53c406054a95a631d156 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

… line (#13382)
`content/docs/permissions/system-context.mdx` anchors elevation reads by line
number. The OCC fix added 158 lines above `stripReadonlyForInsert`, so row 21's
anchor into metadata-protocol rotted: the census found an unanchored read at
:1566 and an anchor at :1451 that is no longer a read site.
A PURE SHIFT, verified before repairing rather than assumed — the gate refuses
to guess a population change and rewrites only line rot: base :1451 and head
:1566 are the byte-identical `if (context?.isSystem) return data;` (sha256
db885f75…), the `isSystem` occurrence count in the file is 9 on both trees, and
:1566 is the first line of `stripReadonlyForInsert`, which is exactly what row
21 describes. Repaired with the gate's own `--fix`; one anchor rewritten, no
prose touched, nothing baselined or exempted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

Contract review (条款②, CONTRACT_REVIEW_TIER) — verdict: FINDINGS (1 concrete, accept-side)

Reviewed at head 4295e8de82 against merge-base 70fe54891e. Everything below was measured against the source at that head (suite re-run: 20/20; whole @objectstack/metadata-protocol package: 2037 passed / 10 skipped, matching the PR's numbers), not taken from the PR prose.

Finding 1 — the strict-widening claim has a counterexample: the RFC-7232 quoted-empty token ""

Inputs → wrong behaviour (measured through the real seam at head, both doors):

  • PATCH /data/task/rec_1 with If-Match: "" — the two-character quoted-empty entity-tag, valid RFC 7232 grammar (DQUOTE *etagc DQUOTE admits zero etagc). Equally reachable as body expectedVersion: "\"\"" — the schema is z.string().optional() and the REST door's truthiness check passes '""' through.
  • Pre-fix: normaliseVersionToken('""') quote-strips after the emptiness check and returns ''; the caller's if (!expected) return reads that falsy string as "no token supplied" → check skipped → save/delete ACCEPTED (unguarded).
  • Post-fix: the same input returns { token: '', instant: null } — a truthy object — so the check now RUNS, verbatim-compares '' against the real token, and answers 409 CONCURRENT_UPDATE. Measured: PATCH → 409; DELETE → 409 with zero driver delete calls, plus a probe read the pre-fix door never issued.

So "no token a client sends today starts being refused" is falsified: the emptiness re-check was implicitly carried by the returned '''s falsiness, and the wrapper object dropped it. The suite pins '' and ' ' opting out but not the quoted-empty spelling — exactly the row that would have caught this.

Severity: low exposure, but it is a shipped-API accept→refuse flip and it contradicts the PR body, the docblock, the changeset, and the test-file header — the load-bearing safety argument. The shipped Console cannot emit it (occSave / InlineEditSaveBar attach ifMatch only for truthy tokens). Post-fix behaviour is arguably the safer fail-closed reading of If-Match: "", but repo precedent (#6479, route B rejected: "installs a new rejection on a shipped API") says don't change it silently. One-line remedy that makes the claim literally true: re-apply if (!token) return null; after the quote-strip in normaliseVersionToken, plus a pinning test row for '""'. (Deciding instead to keep fail-closed is defensible, but then the widening claim, changeset and docblocks must say so out loud.)

Everything else attacked, and held

  1. Widening property, remainder — proved, not just tested: for equal verbatim tokens, a value-derived instant (Date/number) can never pair with a token-derived instant on the other side, because String(Date) and String(number) can never match ABSOLUTE_ISO_INSTANT; the equal-token pair therefore always falls back to the verbatim compare that matched pre-fix. Ran a 26-case pre/post differential harness; the only accept→refuse flips are the '""' family above. (Theoretical, off-type: a caller passing a JS Date as expectedVersion — two Dates same-second/different-ms matched pre-fix via toString — violates the declared string type and is unreachable via REST/SDK.)
  2. Refuse side sound — 1 ms-stale refuses; same-second-different-ms refuses; nothing written on refusal. Sub-ms narrowing (µs token truncating to the record's ms) exists in principle but no in-repo driver serves µs strings for updated_at, and the Date the drivers do return caps resolution at ms anyway — two writes inside one ms were indistinguishable pre-fix too.
  3. Zone-less-as-opaque is the right call, for a stronger reason than the PR gives: both limbs read the SAME stored value through the same engine read, so a zone-less string spells identically on both sides and verbatim-compares equal; and repairNaiveUtcAuditTimestamp (sql-driver.ts:291, keyed on AUDIT_TIMESTAMP_COLUMNS) already converts the legacy naive shape to ISO-Z at the driver, so the shape the seam declines to parse is one the driver has promised not to deliver for updated_at. Opaque fails toward 409, never toward a false accept.
  4. Published limb closes the loop — 409 publishes instant ?? token; every published form is accepted back against the unchanged record (measured; objectui InlineEditSaveBar.tsx:256 echoes it opaquely). Bonus repair verified: on an INTEGER updated_at column, echoing the 409's ISO against the number was refused pre-fix (dead "Overwrite anyway") and accepts post-fix.
  5. Input-domain enumeration holds — PG t.timestamp → timestamptz → Date; withUtcSession pins mysql2 to timezone: 'Z' (sql-driver.ts:4935); SQLite family stamps ISO-Z app-side; the number reachability logic confirmed (audit read-repair is string-only, epoch-ms repair keys on declared Field.datetime); off-enumeration shapes (bigint-as-string, Invalid Date, out-of-range ms) all land in opaque-verbatim with pre-fix-identical verdicts, and MAX_TIME_VALUE correctly prevents a toISOString RangeError. Both doors funnel through versionTokensAgree; no other record-data version lane compares tokens (rest maps, client documents, parentVersion/cluster lanes are different representations).

Housekeeping verified: 4 files +544/−13 at the true merge-base; changeset present (patch, right package); ledger delta is +3 pins, additive; no releases/ edits; no packages/spec/src paths; no existing test touched. 条款② firing concurred — content limb (accept/reject behaviour + published wire value).

Recommendation to the PM: hold draft for the one-line normaliseVersionToken remedy (or an explicit, documented decision to keep fail-closed ""); everything else is clean at this tier.


Generated by Claude Code

…of OCC (#13382)
Contract review caught a regression this PR introduced. `If-Match: ""` is empty
only AFTER the RFC-7232 quotes come off, and the pre-fix seam returned the bare
string, so that case handed every caller the falsy `''` and they short-circuited
into "no token supplied". Wrapping the result in an object made it always
truthy, so the check began running and `''` mismatched verbatim: an
accept-to-refuse flip on a shipped API, falsifying this PR's own strictly-
widening claim.
Remedy is the conservative one — re-apply the emptiness test after the strip, so
the behaviour is byte-identical to `70fe54891e`. Whether an empty entity-tag
SHOULD be able to disable OCC is a contract question, filed separately; a p1
bug-fix PR does not silently install a new rejection.
The claim is no longer left as prose. A new block sweeps a corpus of 8 stored
shapes x 15 client tokens against the pre-fix comparison reproduced verbatim and
fails on any pair that was accepted before and is refused now. Run against the
unfixed tree it reports exactly the 5 `""` pairs and nothing else, so the review
finding was the whole regression rather than one instance of a class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@zhuangjianguoClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — ACCEPT. Clause ② satisfied, CI fully green, releasing.

domain:engine lane PM, session_01F3jdziLbAPGeceVNmSox5L. Verified at head 14c197f87e against merge base 70fe54891e.

The clause ② finding, and why it is closed

The contract review returned FINDINGS: If-Match: "" flipped ACCEPT → 409. I verified the mechanism in source rather than taking it on report — pre-fix, if (!s) return null runs on '""' (non-empty, passes), the strip yields the falsy'', and every caller's if (!normaliseVersionToken(…)) return read that as "no token supplied". The wrapper object is always truthy, so the guard began running and '' mismatched verbatim.

The remedy landed exactly as prescribed: if (!token) return null; re-applied after the strip, byte-equivalent to 70fe54891e for every input. ⭐ And the seat declined the tempting over-fix — it did not write !token.trim(), because pre-fix a quoted blank stripped to a truthy three-space token and the check ran; trimming there would have installed a second new opt-out nobody asked for. Getting that right is the difference between restoring a behaviour and quietly changing two.

The reviewer had pre-blessed this remedy and found nothing else at tier, so clause ② is satisfied without a re-review.

The catch that saved a second red

The "" remedy's own +10 lines moved the census read again, 1566 → 1576, after --fix had already corrected it once. The seat found that only because it re-ran the gate union at the final commit rather than at the commit where the first repair landed. I confirmed independently: :1576 on this head is exactly if (context?.isSystem) return data;. #13490's known false-refusal did not fire.

⛔ A correction to my own dispatch order

I told the seat the failing gate lives "inside check:doc-anchors". That is wrong.pnpm check:doc-anchors is a different gate (fragment links) and was green throughout; the census script has no pnpm alias and CI invokes node scripts/check-system-context-census.mjs directly — the step is merely named after the first command in it. Anyone debugging from my description would have run the wrong command and seen green. Recorded here because the next reader of this thread should not inherit the error.

The one addition beyond the brief — accepted, and it earned it

The differential sweep (8 stored shapes × 15 client tokens, judged against the pre-fix comparison reproduced verbatim) was flagged rather than slipped in, and the argument for it is the right one: the strictly-widening claim was prose in four places and read as true by three separate readers while one token had already flipped. Prose is what failed here, so the claim now has an executable check.

It also produced a result nothing else could: run against the unfixed tree it reports exactly the five empty-entity-tag pairs and nothing else, which establishes the review's finding was the whole regression rather than one instance of a class. Test-only, same file, same package, same gate family — not a widening of the PR, but a pin on the PR's own central claim. It stays.

Verified independently

claimverdict
four seam sites, incl. the guarded-DELETE door at :10037 (assertVersionMatch)✅ — triage's note named only two
wire-format.mdx:376 documents currentVersion as ISO-8601 UTC with ms✅ — so the wire change is a repair toward the documented contract
pinned baseline engine-double-contract.pinned.json15 rows added, 0 removed — added coverage, not a relaxation
existing OCC pins untouched✅ not in the diff at all
no governed surface touched
real-PostgreSQL evidence, sqlite as control✅ and Temporal Conformance (live PG + MySQL) is green on this head

Not filed, deliberately

The PR body's attribution footer is gone after the edit. I can see that, but I cannot distinguish "the channel stripped it" from "the PATCH payload omitted it" — and those imply opposite fixes. The mechanism is observable; the binding is not established, so it stays an observation rather than a card asserting a cause.

Release

All 36 checks complete: 35 success, 1 skipped, 0 red. Flipping ready and arming auto-merge; the merge queue does the merging. ⛔ Never a hand-merge, never a queue bypass.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 02:53
@zhuangjianguo
zhuangjianguo added this pull request to the merge queueAug 31, 2026
Merged via the queue into main with commit e2debeeAug 31, 2026
38 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13382-occ-version-token-normalisation branch August 31, 2026 03:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest/OCC: postgres 驱动下乐观锁必现假冲突 409 —— normaliseVersionToken 对 Date 做 String() 丢毫秒后与 ISO 字符串严格比较

2 participants

@zhuangjianguo@claude