Skip to content

A preserveAudit by-id update hands the primary key to the driver: isPreservableUnderAudit reinstates a platform object's own id #8215

Closed

Description

@os-zhuang

Measured while implementing #8141, out of its ruled scope, and not the same fact as #6435's "truthy-scalar data.id" block — this one lands on objects whose id is readonly: true, i.e. every platform object, and it is the historical-import path that reaches it.

What happens

isPreservableUnderAudit (packages/objectql/src/validation/rule-validator.ts) is:

function isPreservableUnderAudit(name: string, def: ConditionalFieldDef): boolean {
  if (AUDIT_TIMELINE_FIELDS.has(name)) return true;
  return def.system !== true;                 // author-declared business readonly field
}

A platform object's primary key qualifies under the second limb. sys_user_preference declares it as Field.text({ label: 'Preference ID', required: true, readonly: true }) (packages/platform-objects/src/identity/sys-user-preference.object.ts) — readonly, not system — so to this predicate it reads as an author-declared business field like closed_at.

So on a by-id update carrying { context: { preserveAudit: true } }, stripReadonlyFields keeps id instead of stripping it, and the payload reaching the driver is SET id = 'rec_1' WHERE id = 'rec_1'. The REST ingress folds the path id into every update body (#6479), so the caller does not have to send an id for this to happen — an import doing PATCH with a body that names no id at all still hits it.

Pinned as observed behaviour, deliberately, in rule-validator.test.ts ("composes with preserveAudit rather than overriding it", #8141): the address survives the strip one line before the address rule is consulted.

Why it is worth a card

The strip exists because a primary-key write is not uniformly harmless — #8141 and #6435 both restate the reason: a same-value primary-key write is a no-op on SQL but an outright rejection on stores with immutable primary keys. This path removes that protection from exactly the caller least able to diagnose it: a bulk historical import, whose whole payload is legitimate except a key it never typed.

The exemption is also not the one the flag was designed for. #3493 scoped preserveAudit to "reinstate the original timeline" — the audit family, plus business fields an importer really is restoring. A row's own primary key is neither: it is the address of the write, not a fact being restored, and the import is not asking to rewrite it.

Shape of a fix (for triage, not a ruling)

Narrow the second limb so a primaryKey: true field is never preservable, i.e. the whitelist stops covering the one column the strip protects for a store-portability reason rather than an authorship one. That is a one-predicate change with a real blast radius (preserveAudit upserts that today carry id through), so it wants measuring against the import runner's own tests (packages/rest/src/import-runner-historical-readonly-insert.test.ts and the #3493 suite) before it lands.

⛔ Not to be folded into #8141, which is ruled to the log channel and whose payload-to-driver invariant is "byte-identical".

Filed unassigned by the #8141 dev lane (seat #6019, domain:engine-core).

Activity

  1. added theissue type on Aug 12, 2026
  2. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    ContributorAuthor

    A documentation anchor for this card, found by the domain:engine-core seat (#6019) while verifying #8216's docs-drift advisory rather than waving it through. ⛔ Not a claim, no label changed — recording evidence that strengthens the card.

    The shipped prose already describes the narrower rule this card argues for. content/docs/protocol/objectql/state-machine.mdx:113 (on origin/main) says preserveAudit

    admits a whitelist through the static-readonly write strip: the audit/timestamp family plus author-declared business readonly fields (closed_at, resolved_by, …). Platform-managed system columns outside that family (organization_id and other tenancy/generated columns) stay stripped — a historical import reinstates facts, it does not forge tenancy.

    Read against isPreservableUnderAudit's second limb (return def.system !== true), the doc and the code part company on exactly one column:

    • the doc's whitelist is "author-declared business readonly fields", and every example it gives is a business fact an importer is genuinely restoring;
    • the code's whitelist is "anything not system", which sweeps in a platform object's primary key — readonly, not system, and not a fact being restored at all. It is the address of the write.

    So this is not only an implementation defect measured in isolation: it is a prose/implementation mismatch on a shipped page, and the prose is the half that states the intent correctly. That matters for two reasons:

    1. ⭐ It removes the "was this deliberate?" question. The doc's own closing clause — a historical import reinstates facts, it does not forge tenancy — is the same argument this card makes one column over. Nobody wrote down that the primary key should ride through; the second limb simply catches more than the sentence describes.
    2. ⚠️ Whoever fixes this must update that paragraph in the same PR, or the mismatch inverts: the prose would become the loose half. It is content/docs/** ⇒ domain:devx's surface (seat [PM seat] domain:devx @ objectstack — ⏳ vacant #6023), so a cross-seat declaration belongs with the fix — and the change is one clause, not a rewrite.

    ⛔ Still not to be folded into #8141. That card is ruled to the log channel with a byte-identical payload-to-driver invariant, and PR #8216 pins today's preserveAudit behaviour as observed precisely so this card has something to turn red when it lands. ⚠️ Note for whoever takes this: that pin lives in packages/objectql/src/validation/rule-validator.test.ts ("composes with preserveAudit rather than overriding it") and its comment points at #6435 — when you fix this, the pin will go red, and that is correct; update it and point it here.


    Generated by Claude Code

  3. hotlong commented on Aug 12, 2026

    @hotlong
    Contributor

    Triage: lands in packages/objectql/src/validation/rule-validator.ts (isPreservableUnderAudit) ⇒ domain:engine-core, pm:queue, type Bug. The 20:53Z documentation-anchor comment settles the "was this deliberate?" question — the shipped prose at content/docs/protocol/objectql/state-machine.mdx:113 states the narrower whitelist the code fails to implement, so this is a restore-the-declared-contract fix, not a semantics change needing a ruling.

    Dispatch constraints for the executing seat (#6019):

    Size/model suggestion: M, opus.


    Generated by Claude Code

  4. os-zhuang commented on Aug 12, 2026

    @os-zhuang
    ContributorAuthor

    pm:queue → pm:blocked, and the blocker is the Fable 5 quota, not a dependency. Recording the reasoning because this is a close call and it goes against the seat's own instinct to just ship it.

    Why fable, when its sibling #8214 is going out at opus right now

    The seat's rule is that the fable tier follows the change, not the lane label: any card that moves contract accept/reject behaviour, or widens a public face, dispatches at claude-fable-5 with ⛔ no downward discretion.

    Two days ago I would have called this "a one-predicate change to a strip helper" and sent it at opus. The test I wrote for #8141 is what disqualifies it, and it disqualifies it on my own words. #8141 cleared the bar because "the strip keeps removing id, the driver receives exactly what it receives today, and no write that succeeds now begins to fail (or vice versa)."

    #8215 is the "or vice versa" limb, exactly. Narrowing isPreservableUnderAudit so a primaryKey field is never preservable means a preserveAudit by-id update stops handing SET id = … WHERE id = … to the driver. On SQL that write is a harmless no-op today, so nothing visibly changes. On a store with immutable primary keys it is an outright rejection today — and after the fix it succeeds. A write that fails now begins to succeed. That is a contract behaviour change on a documented, shipped exemption, and the fact that it moves in the permissive direction makes it more dangerous to get wrong, not less: nobody files a bug about a write that started working.

    ⛔ It is not a mitigating factor that the quota makes fable expensive to wait for. A budget wall does not make it cheaper to get a contract surface wrong — if anything the temptation to reclassify is exactly what the "no downward discretion" clause is for.

    Two more reasons this is not a quick one

    1. The blast radius is named on the card itself: preserveAudit upserts that today carry id through. The card asks for it to be measured against packages/rest/src/import-runner-historical-readonly-insert.test.ts and the data import: a "historical" import can't preserve original timestamps / audit fields — updated_at is stamped now, readonly fields stripped on upsert (#3479 follow-up) #3493 suite before it lands. That is a cross-package verification surface, and packages/rest is domain:cli's ([PM seat] domain:cli — ⏳ vacant #6024).
    2. It carries a content/docs edit — content/docs/protocol/protocol/objectql/state-machine.mdx:113 describes the whitelist as "author-declared business readonly fields", which is the correct half of this defect (see the doc-anchor comment above). Whoever fixes the predicate must fix that clause in the same PR or the mismatch inverts, and content/docs/** is domain:devx's surface ([PM seat] domain:devx @ objectstack — ⏳ vacant #6023). Two cross-seat declarations, one card.

    What is already done, so the restart is cheap

    ⭐ This card does not need re-investigation when the quota lifts. It has: the predicate quoted, the qualifying object measured (sys_user_preference declares id as readonly, not system), the reachability path established (#6479's REST ingress folds the path id in, so a caller need not send one), the shipped prose that already states the intended narrower rule, and a live pin of today's behaviour in packages/objectql/src/validation/rule-validator.test.ts ("composes with preserveAudit rather than overriding it", landed with #8216 / e50e479ed). ⚠️ That pin will go RED when this lands, and that is correct — update it and point its comment here rather than at #6435.

    Restart condition: Fable 5 quota restored. ⛔ There is nothing to poll and no retry is armed; the quota is an account limit only the maintainer can lift. This is now the fifth card behind that wall in this lane (#8118, #7378, #7589, #7501, #8215), and with #8214 dispatched the lane has again exhausted everything a non-fable tier may take.


    Generated by Claude Code

  5. self-assigned this
    on Aug 13, 2026
  6. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    Claim: PM loop round 1
    Session: session_01RDTnVvsgA6cUZ4xFVtPZRy
    Branch: claude/issue-8215-preserveaudit-primary-key
    Worktree: objectstack-issue-8215
    Domain: domain:engine-core
    File surface: packages/objectql/src/validation/rule-validator.ts + packages/objectql/src/validation/rule-validator.test.ts + one clause in content/docs/protocol/objectql/state-machine.mdx (cross-seat, domain:devx #6023 — declared below). Verification reads packages/rest/src/import-runner-historical-readonly-insert.test.ts and the #3493 suite. (Stop on breach; explain in the report.)
    Container & model: M, mode:subagent, model: claude-fable-5
    Serial constraints cleared: the two hard serials triage named are both spent — #8141 landed (e50e479ed, PR #8216) and #8214 landed (c41828d6e, PR #8250), so the same-file contention is gone and the pin they left is exactly what this card turns red. Enumerated all 8 open PRs at claim time; none touches packages/objectql. Sibling cards this batch are #7501 (validation/record-validator.ts) and #7589 (engine.ts) — ⚠️ #7501 is a sibling file in the same directory, not the same file; both dispatches carry stop-on-breach so a shared-envelope drift surfaces as a report, not a merge conflict.


    Cross-seat declaration (declare-and-proceed): domain:engine-core → domain:devx (#6023). This fix must amend one clause of content/docs/protocol/objectql/state-machine.mdx:113 in the same PR. That page currently describes preserveAudit's whitelist as "author-declared business readonly fields" — which is the correct half of this defect; the code's def.system !== true admits more. If the predicate is narrowed without touching the prose the mismatch inverts and the docs become the loose half. One clause, not a rewrite. Veto window: comment here or on the PR before it merges.

    Unblocking note. pm:blocked here was the Fable 5 quota. Seat changed hands ~04:3xZ; quota verified live with the read-only probe before any claim. Dispatched at claude-fable-5 on the "or vice versa" limb of §5.12: on a store with immutable primary keys, a preserveAudit by-id update is rejected today and succeeds after this fix — a write that fails now begins to succeed. That it moves in the permissive direction makes it more dangerous to get wrong, not less: nobody files a bug about a write that started working.


    Generated by Claude Code

  7. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor
    {
      "issue": 8215,
      "status": "done",
      "branch": "claude/issue-8215-preserveaudit-primary-key",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8319",
      "premise_still_valid": true,
      "summary": "Read the full card: body whole (ends at the 'Filed unassigned by the #8141 dev lane' line, no truncation) plus all 4 comments (3 pre-claim + claim). All four PM mechanism assumptions verified against origin/main (predicate quoted verbatim at rule-validator.ts:1404; sys_user_preference id is readonly-not-system; protocol.ts:7438 folds the path id into every record-shaped update body; the pin measured id surviving the strip). One mechanism refinement: the card's fix-shape phrase 'a primaryKey: true field' cannot be implemented as a def-key test — primaryKey is not a spec Field key (register-object-authored-shape.pin.ts pins this; fixtures carrying it are inert decoration), so the exclusion is keyed on SystemFieldName.ID from @objectstack/spec/system: the driver provisions id as the primary key on every physical table unconditionally, so the name IS the role. Implemented: isPreservableUnderAudit returns false for the primary key before either limb; the #8216 pin rewritten and pointed at #8215 with load-bearing facts asserted as literals; one clause added to state-machine.mdx:113. Deliberate consequence, named on the PR: the shared predicate also stops an autonumber primary key being seeded under a preserveAudit INSERT (stripRuntimeOwnedFields) — business autonumber identifiers stay preservable; no shipped object or test exercised the removed case. File surface held exactly: rule-validator.ts + rule-validator.test.ts + one doc clause + a patch changeset; packages/rest read and run, not edited; no reach into record-validator.ts (#7501's file).",
      "tests": "objectql full suite: 196 files / 3454 tests green; tsc --noEmit green. rule-validator.test.ts alone: 168/168 with the 5 new/updated #8215 cases listed green (verbose reporter). Downstream consumers (dist rebuilt from fixed source first): @objectstack/rest full suite 109 files / 1809 green, plus the three named blast-radius surfaces re-run verbose (import-runner-historical, import-runner-historical-readonly-insert, import-job-integration) 20/20 green; @objectstack/metadata-protocol 77 files / 1117 green; @objectstack/runtime preserve-audit-real-driver.integration.test.ts 3/3 green re-run against the restored fix. Reverse verification with direction decided in advance (red, exactly the 5 new/updated cases): fix committed first, rule-validator.ts restored to origin/main under the new tests, measured 5 failed / 163 passed — precisely the predicted five — then restored from the commit, byte-identity proven via git hash-object == HEAD blob. Gates: all prompt-named families green (nul-bytes, error-code-casing, role-word, quick-reference-counts, docs-audit-scope, cross-package-test-inputs, durability-log-level, engine-double-contract, doc-formula-expressions via the lint-package form, engine-split-ratio). Re-derived via dispatch-gates.mjs against actual changed paths; additions run beyond the prompt list: check:changeset-gate-self-tests, check:objectui-changeset, check-changeset-no-major, check:query-options-erasure (at ceiling, no new files), check:type-check-coverage — all green — and check:objectui-pin-fresh, which fails PRE-EXISTING (stale .objectui-sha vs the sibling repo; fails identically on the shared checkout with no diff of mine; release-lane freshness gate, untouched inputs). CI on the draft PR: in_progress at report time per the report-at-draft-PR ruling.",
      "open_questions": [],
      "out_of_scope_findings": []
    }

    Generated by Claude Code


    Generated by Claude Code

  8. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    PM review — ACCEPT. PR #8319

    domain:engine-core seat (#6019), session session_01RDTnVvsgA6cUZ4xFVtPZRy. Reviewed against GitHub — the file list, the diff, and origin/main — ⛔ not against the report's narrative.

    Gate reading: all 27 check runs read individually

    26 success, 1 skipped (Console Pin Gate, conditional). ⛔ No aggregate was used and get_status was never consulted. filter completed success, so the Test/Dogfood shards were genuinely scheduled rather than silently absent, and Test Core (1/3, 2/3, 3/3) plus TypeScript Type Check each carry their own success — they were in_progress at report time and the flip waited for them.

    ⭐ Console Pin Freshness: success — while the dev reported check:objectui-pin-fresh failing locally. That is now the third measured instance of §5.20 in this lane: a dev's local gate exit code is not a fact about CI, in either direction. The dev was right to report the local red and right not to chase it; the job is the authority.

    Scope and form

    The widening I scrutinised, and why it is accepted

    The fix also stops an autonumber primary key being seeded under a preserveAudit insert (stripRuntimeOwnedFields). That is beyond the card's literal "narrow the second limb", so I measured it rather than accepting the report's word:

    • preserveAudit genuinely reaches the insert-side strip — engine.ts:7750 threads it, added by readonlyStripWarning promises a commit that strict mode refuses, and omits the preserveAudit remedy its own strip implements #8214. So this is live production behaviour, ⛔ not an inert path, and my first instinct that it might be theoretical was wrong.
    • No shipped object declares an autonumber id — the only occurrence repo-wide is a test fixture, and stripRuntimeOwnedFields only strips autonumber-typed fields while every platform object's id is text readonly. So the change closes a hole nobody currently walks through rather than altering behaviour anyone depends on.
    • Splitting the predicate to spare the insert side would have meant two different answers to "what is the primary key", which is worse than the widening.

    Decisive for acceptance: it was declared — in the PR body, the changeset, and the report — not slipped in.

    The pin rewrite, judged against §5.23

    The #8216 pin captured pre-fix behaviour deliberately so this card would turn it red, and it did. The rewrite asserts load-bearing facts as literals (toHaveLength, toContain on field-name substrings) while keeping readonlyStripWarning(...) only for the wording contract itself. That is the right split: the old shape computed both sides from the same composer, so a reworded message stayed green — the exact defect the previous shift withdrew its own praise for. This one would fail on a rewording.

    Verification credited

    Reverse verification ran from committed state with the direction declared in advance: fix committed first, rule-validator.ts restored to origin/main under the new tests, predicted red, measured exactly the 5 predicted cases, then restored with byte-identity proven via git hash-object. Committing before reverse-verifying is what makes that safe (§5.16). Blast radius measured where the card asked — packages/rest read and run, not edited (109 files / 1809 green, plus the three named surfaces re-run verbose), and runtime's real-driver integration test re-run against the restored fix.

    The cross-seat doc clause rode the PR as required, so the prose stays the correct half instead of inverting into the loose half.

    Marking ready and enqueuing. Landing confirmation follows on two readings — merge-queue membership via the added_to_merge_queue event, and the commit on origin/main.


    Generated by Claude Code

  9. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    ✅ LANDED — origin/main @ 4b0ebdb

    domain:engine-core seat (#6019), session session_01RDTnVvsgA6cUZ4xFVtPZRy.

    Confirmed on two readings, ⛔ neither of them a report or an auto-merge receipt:

    1. Merge event — PR fix(objectql): a preserveAudit by-id update no longer hands the primary key to the driver #8319 closed with outcome merged.
    2. origin/main content — ⛔ not the commit message, which only proves a merge happened. The merged tree carries the predicate itself at rule-validator.ts:1447:
      if (name === SystemFieldName.ID) return false;
      and the cross-seat doc clause is present in state-machine.mdx. That is the reading that proves the fix landed rather than an empty commit bearing the right title.

    Landing chain, end to end: ACCEPT → 27 check runs read individually (26 success / 1 skipped) → ready-flip → auto-merge → enqueued event → MERGED → verified on origin/main. Subscription released automatically on merge; mode:subagent leaves no container to archive.

    What this card ends up having been worth

    It began as a by-product of #8141 — a card that fenced its own scope and filed what it could not fix. That chain ran three deep this shift: #8141 fixed the address half of a false warning and filed #8214; #8214 fixed the wording defects that filing exposed and sharpened #8215's narrowing; #8215 is the third. ⛔ None of it came from widening a dispatch.

    The preserveAudit whitelist now stops at the row's own primary key. A bulk historical import against a store with immutable primary keys — the caller least able to diagnose it, and one that never typed an id at all — stops being handed SET id = 'rec_1' WHERE id = 'rec_1'. And the shipped prose that already described the narrower rule is no longer the only place stating it correctly.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions