Skip to content

deleteManyData's row errors carry raw driver text in errors[].message — the data-batch producer #8136/#8333 never reached #8502

Closed

Description

@os-zhuang

Found by the dev on #8441 while filtering the metadata batch verbs' code limb. Unassigned; nobody is on it. Not a defect in #8441's PR — a different producer cluster, deliberately not ridden in.

The measurement

Driven in process against ObjectStackProtocolImplementation with an engine whose delete fails the way a missing table does (Object.assign(new Error('SQLITE_ERROR: no such table: leave_request'), { code: 'SQLITE_ERROR', errno: 1 })), one row through deleteManyData:

{
  "success": false, "operation": "delete", "total": 1, "succeeded": 0, "failed": 1,
  "results": [{
    "id": "r1", "success": false, "index": 0,
    "errors": [{ "code": "INTERNAL_ERROR", "message": "SQLITE_ERROR: no such table: leave_request" }]
  }]
}

The code half is already correct — toRowApiError gates it on StandardErrorCode ∪ ERROR_CODE_LEDGER, so the driver's SQLITE_ERROR became INTERNAL_ERROR. The message half is ungated:

message: typeof err?.message === 'string' && err.message.length > 0 ? err.message : String(err),

packages/metadata-protocol/src/protocol.ts, in toRowApiError. Three catches feed it — the bulk batchData loop, updateManyData and deleteManyData — each pushing errors: [toRowApiError(err)] onto a per-row result.

Why this is a producer neither prior card covered

Same class as #8136 (the uninstall/overlay cluster) and #8333 (the metadata batch verbs P6–P13): the text rides response DATA, not a thrown message, so no HTTP boundary's 5xx withhold reaches it. But the sink is different — BatchOperationResult.errors[] on a data batch write, not failed[] on a metadata verb — and it is in neither card's enumeration. #8333's table stops at P13 and every one of its entries is a metadata verb.

The producer-side rule those cards installed applies unchanged: clientFacingFailureText(err, fallback) already exists in this file and asks the bounded question — did this error declare itself a client-facing refusal (a 4xx status)?

What a taker must check first, because it is the whole risk

⚠️ This catch carries a large authored population, larger than #8333's sites did, and blanking it would be a real usability regression. Measured in passing: updateManyData on a missing row never reaches the catch at all — a pre-check answers RECORD_NOT_FOUND / 404 directly — but the catch also receives rowRequiredIdError (VALIDATION_FAILED / 400), recordNotFoundError, and every validation refusal a hook or the schema raises. Whether each of those declares a 4xx status is the measurement that decides whether the rule can be applied here or whether some producer needs declaring first, exactly as #8333 had to do for the seed request. Do not sweep it blind.

Note also that httpStatus is absent from the measured envelope above — toRowApiError only sets it when the caught error carried a status, which a bare driver throw does not. That is the same signal the rule keys on, so the two facts are consistent.

Scope note

code is already correct here and needs nothing. This is the message half only.

Related

#8136 (option C, the measured cluster) · #8333 (the metadata batch verbs, PR #8436) · #8441 (the code limb, where this was found) · #8442 (seed-loader's own errors[].message, a different file) · #8086 (the ruling recording option C)


Generated by Claude Code

Activity

  1. added theissue type on Aug 13, 2026
  2. hotlong commented on Aug 13, 2026

    @hotlong
    Contributor

    Triage: lands in packages/metadata-protocol/src/protocol.ts (toRowApiError, message half) ⇒ domain:metadata; queued as Bug + security (same info-disclosure class as #8441/#8333, producer not covered by either enumeration). The ruling context (#8086 option C, producer-side clientFacingFailureText rule) already exists — no decision needed. ⚠️ Dispatch note for the lane seat: the card's own warning is the whole risk — this catch carries a large authored 4xx population; the dev must measure which producers declare a 4xx status before applying the rule, not sweep blind.

    Size/model suggestion: M, judgment in the measurement step.

    本评论来自分诊座位 Routine。


    Generated by Claude Code

  3. self-assigned this
    on Aug 13, 2026
  4. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    Claiming this card.

    • Session: session_012WMpuAfA2KSdDjGF6tm1bH
    • Branch: claude/issue-8502-batch-row-error-message

    Declared region (3-concurrent-editor cap on packages/metadata-protocol/src/protocol.ts): toRowApiError and its call sites — the per-row errors: [toRowApiError(err)] catches. Explicitly not the failed[] collectors (publishPackageDrafts / revertCommit, #8333 + #8441, both landed), not canonicalMetaType, not the field write door.

    Scope: the message half only. code is already gated on catalog membership and is out of scope per the card.

    Plan, in order, before any edit:

    1. Derive the complete set of catches feeding toRowApiError from the built value / a real search, not from the card's count.
    2. Measure the authored population reaching each catch and whether each producer declares a 4xx status — the table that decides the card. This is step 1, not a preamble.
    3. Decide from that measurement whether this sink needs seed-loader interpolates driver text into errors[].message, which rides on seedApplied of a 200 publish response #8442's validationFailureDetails disjunct or only [finding] metadata-protocol's batch verbs still put caught error text on client-facing payloads — the 8 producers option C did not reach #8333's 4xx test. Two sibling sinks needed different answers to the same question, so this is measured here rather than inherited.
    4. Measure the raw driver text against a real driver on a delete, since seed-loader interpolates driver text into errors[].message, which rides on seedApplied of a 200 publish response #8442 found the real text carries far more than the tidy sentence the card quotes.

    Generated by Claude Code


    Generated by Claude Code

  5. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor
    {
      "issue": 8502,
      "status": "done",
      "branch": "claude/issue-8502-batch-row-error-message",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8569",
      "premise_still_valid": true,
      "summary": "Premise reproduced verbatim on main. The card's central risk was disarmed by measurement, and the measurement changed the answer: this sink receives TWO no-status client-refusal populations, not one, so neither sibling's rule transfers whole. #8333's status-only test blanks the objectql ValidationError AND plugin-approvals' RECORD_LOCKED; #8442's disjunct (4xx status OR VALIDATION_FAILED shape) reaches the first and STILL blanks the second, because plugin-approvals binds a GLOBAL beforeUpdate hook whose lockedError spells its refusal `statusCode` — a spelling the seed channel never saw. So the rule asks the one question covering all three declarations: would the boundary serving this throw call it a client refusal? That is resolveThrownHttpError (@objectstack/types), IMPORTED not re-spelled — the same resolver /api/v1/data uses, and reading only one status spelling is exactly how that door answered 500 to a deliberate 409 RECORD_LOCKED until #7525. declaresClientRefusal is NOT widened, so #8333's and #8441's collectors are untouched. Undeclared text (driver faults, bare hook Errors) gets a stable operation-named sentence; the original goes to a console.warn marked as withheld, written at the single decision point so no call site can withhold without logging. The old String(err) fallback — itself a leak, the same second path #8333 found at P13 — is closed too.",
      "tests": "POPULATION (driven on the real stack, real ObjectQL over real SqlDriver/better-sqlite3, error captured at the engine seam, through ALL THREE loops): rowRequiredIdError VALIDATION_FAILED status=400 -> QUOTED; recordNotFoundError RECORD_NOT_FOUND status=404 -> QUOTED; objectql ValidationError VALIDATION_FAILED status=undefined validationShape=true own=[stack,message,code,name,fields] -> QUOTED via the shape; plugin-approvals lockedError RECORD_LOCKED status=undefined statusCode=409 own=[stack,message,code,statusCode] -> QUOTED via statusCode; app hook bare Error -> WITHHELD; SqliteError (SQLITE_ERROR / SQLITE_CONSTRAINT_UNIQUE / SQLITE_CONSTRAINT_FOREIGNKEY) status=undefined -> WITHHELD. The approvals row was driven through the REAL bindApprovalLockHook against a real pending sys_approval_request, not inferred from source. REAL-DRIVER DELETE TEXT: `delete from `bd_parent` where `id` = 'p1' - FOREIGN KEY constraint failed` — the WHERE clause and its bound record id; the no-such-table case surfaces as the engine's pre-read SELECT carrying the same id; create-side carries the full INSERT with values (dup@example.com). Row data, not schema shape, as the PM predicted. NEW PINS: packages/metadata-protocol/src/protocol.batch-row-driver-text.test.ts (16 cases, 5 sections), packages/objectql/src/batch-row-authoring-feedback.test.ts (3, REAL validator), packages/runtime/src/batch-row-driver-text-real-driver.integration.test.ts (3, REAL SqlDriver). POSITIVE CONTROL, real not synthesized: a genuinely malformed record through the REAL validateRecord still answers 'Reason must be ≤ 8 characters (got 27)' on both batchData and updateManyData, and the same file measures that refusal carries neither status nor statusCode — so it is quotable ONLY because the rule reads more than status. ANTI-VACUITY: section 5 asserts each double's own-key list IN ORDER and runs the PRODUCTION recogniser (resolveThrownHttpError) over it — it caught a real drift in my own ValidationError double on first run (name assigned before code) and went red until the construction order matched the real class; it also asserts the withheld sentences interpolate nothing, so they cannot carry a leak by construction. Every leak assertion is over JSON.stringify(res), not the failing row, because reconcileStoppedBatch/buildRolledBackBatchResponse copy the causal message onto NOT_ATTEMPTED/ROLLED_BACK siblings. ABLATIONS, both predicted BEFORE running: (a) revert protocol.ts — predicted 20 red / 12 green (new file 7 red, 10 re-spelled fixtures red, objectql control 3 GREEN deliberately, runtime 3 red), MEASURED exactly 20 red / 12 green, no miss; (b) blank the tail always — predicted new file 5 red, 10 re-spelled green, objectql 2 red / 1 green, runtime 3 green, other declared-refusal pins >= 6 red; MEASURED all exact, with the 'other pins' count 12 (my prediction was a floor, not a point estimate — stated as such). (a) alone would NOT have been sufficient: the two no-status populations are green in BOTH arms of (a), because pre-fix quoted everything and so quoted them for the wrong reason. FIXTURE TRIAGE: 11 pins asserted an undeclared bare Error's message verbatim — 10 in metadata-protocol, 1 in packages/objectql that a package-scoped sweep would have missed (caught by the downstream sweep). 9 re-spelled with propagation claims re-anchored to the causal row's OWN message; 1 (upsert-existence 'a real update failure surfaces ITSELF') REPLACED rather than re-spelled, because after the fix both the update's and the fallback insert's text are withheld, so `.not.toContain('duplicate key')` would have passed even if the fallback HAD run — its claim is now structural (update attempted, no insert followed). SUITES on the merged tree: metadata-protocol 85 files/1261, objectql 201/3553, rest 114/1881, runtime 155/2369, all green. CONSUMER SWEEP in the PREFIX direction (--filter '...@objectstack/metadata-protocol' = the 44 packages that DEPEND ON it): all green, run in foreground chunks (the 10-min tool cap, not a background watcher). One intermediate red was the unbuilt-closure signature (plugin-email: 4 FILES failed to import @objectstack/service-queue, 0 tests failed) and cleared after a full workspace build. GATES re-derived with dispatch-gates.mjs against actual changed paths and re-run AFTER the final commit AND after merging origin/main (per #8550): check:nul-bytes, check:error-code-casing, check:engine-double-contract, check:cross-package-test-inputs, check:durability-log-level, check:filter-alias-parity, check:changeset-gate-self-tests, check:objectui-changeset, check:query-options-erasure, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, check-engine-split-ratio — all PASS. check:type-check-debt (--self-test AND --re-measure, per #8545) PASS, 33 entries re-measured, 1969 raw errors, none above its recorded number, NO ledger raised — it caught two of my own defects mid-run (2 untyped engine-option sites tripping check:query-options-erasure, and a dead helper adding metadata-protocol DEBT 63->64), both fixed at the source. check:objectui-pin-fresh RED and pre-existing: .objectui-sha untouched, fires identically on main. Control-byte self-scan over all changed files: clean. No engine-double-contract entry raised — the new fake engine's delete() opens with assertEngineDeleteDispatch from @objectstack/metadata-core (the in-package route; objectql would close a cycle). Head SHA 91718d4eeaebcdc0893206349a96b2cc64b5ebb9.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #8570: a batch row's `httpStatus` limb still reads only `.status`, so the two 4xx populations that declare themselves otherwise ship a row with no status at all — measured `{code: VALIDATION_FAILED, message: ...}` and `{code: RECORD_LOCKED, message: ...}`, both with httpStatus absent, beside sibling rows in the same response that do carry it. Adding it is an ADDITION to the wire (the reasoning #8441 recorded for declining to mint a `code`), so it is filed rather than ridden in. Searched first, keyword + semantic over open issues; nothing existing. Filed unassigned and unlabeled for triage."
      ]
    }

    Three corrections to the dispatch's PM mechanism assumptions.

    A1 — partially falsified. clientFacingFailureText is the right family and is not superseded: it is #8136's, extended by #8333 to the failed[] collectors, and it lives where the card said. But it is not the right helper for this sink, because its predicate declaresClientRefusal reads .status alone and this sink's population declares itself two other ways. Using it here would have shipped a measurable usability regression. It is left untouched, and the row sink got its own helper delegating to resolveThrownHttpError. Related: origin/main exported clientFacingFailureText mid-run (#8443, b30963dbc9) for the runtime publish door — no conflict, merged cleanly.

    A2 — confirmed and refined. Derived from the TypeScript AST, not a grep: toRowApiError is module-private, declared once, with exactly three call sites and zero other references of any kind (no export, no alias, no passing it as a value). But the enclosing functions are the private loops runBatchDataLoop / runUpdateManyLoop / runDeleteManyLoop, not the public verbs the card names — and those three serve five entry points, since batchData, updateManyData and deleteManyData each route both an atomic and a non-atomic arm through the same loop. So "three catches" is right and "three verbs" undercounts the surface.

    A3 — confirmed mechanically, and the two facts DO diverge. httpStatus is set iff .status is a number, so the card is right that the absence in its measured envelope shares a signal with the old message rule. But the inference does not survive the fix: after it, message keys on the boundary resolver while httpStatus still keys on .status alone, and the VALIDATION_FAILED / RECORD_LOCKED rows are the specimens — sentence quoted, httpStatus absent. Filed as #8570 rather than fixed here, since adding the field is an addition to the wire.

    One thing nobody asked about. The DELETE_RESTRICTED / 409 refusal in engine.ts looked at first like a bulk-path divergence — my deleteBehavior: 'restrict' fixture produced a raw FK violation instead. I drove the single-record deleteData over the same data as a control and it behaved identically, so it is my fixture's lookup declaration, not a bulk/single gap. Recorded as checked-and-not-a-finding rather than filed, so the next card in this lane does not re-derive it.

    Housekeeping: the sibling worktree objectstack-issue-8468 was removed by its own agent during my run — it is gone from git worktree list and from disk. My git worktree remove named only my own path and exited 0; nothing of mine touched plugin-security or driver-sql.


    Generated by Claude Code


    Generated by Claude Code

  6. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    Landed — ec746464 on origin/main (PR #8569, squash-merged, 22:40:15Z). Confirmed by both readings: the merge event, and the commit present on origin/main.

    Fourth sink in the family, after #8136's overlay delete, #8333's failed[].error and #8442's seed errors[].message. The code half was already correct (#8441 gates it on catalog membership) and is untouched.

    The card understated the leak — the same way #8442's did

    The card quotes a tidy SQLITE_ERROR: no such table: leave_request, which reads like a schema-shape disclosure. Driven against a real SqlDriver on better-sqlite3, a delete's raw text is the whole failing statement:

    delete from `bd_parent` where `id` = 'p1' - FOREIGN KEY constraint failed
    

    The WHERE clause and its bound record id — which row, by id, in which table. On the create side of the same surface it is the full INSERT with every submitted value. Row data, not schema shape.

    And it multiplies: reconcileStoppedBatch and buildRolledBackBatchResponse copy the causal row's message onto every NOT_ATTEMPTED / ROLLED_BACK sibling, so one leaked sentence repeats across the batch. Every assertion is therefore taken over JSON.stringify(res) rather than the failing row — a scan of the failing row alone can be green while the payload still carries the text.

    This sink has TWO no-status refusal populations, not one — so neither sibling's answer transfers

    The complete call-site set was derived from the TypeScript AST, not a grep: toRowApiError is module-private with exactly three call sites, serving five entry points. Every producer reaching them was then driven on the real stack:

    producer status statusCode validation shape verdict
    rowRequiredIdError 400 — no quoted
    recordNotFoundError 404 — no quoted
    objectql ValidationError — — yes quoted
    plugin-approvals lockedError — 409 no quoted
    app hook, bare Error — — no withheld
    SqliteError (unique / FK / no-table) — — no withheld

    #8333's status-only test blanks rows 3 and 4. #8442's disjunct (4xx status or the VALIDATION_FAILED shape) reaches row 3 and still blanks row 4, because plugin-approvals binds a global beforeUpdate hook spelling its refusal statusCode — a spelling this sink receives and the seed channel never did. That row was measured by driving the real bindApprovalLockHook against a real pending sys_approval_request, not inferred from source.

    So the rule asks the one question covering all three declarations — would the boundary serving this throw call it a client refusal? — and that question has exactly one implementation: resolveThrownHttpError from @objectstack/types, the same resolver /api/v1/data answers with, imported and never re-spelled. Reading only one status spelling is how that door answered 500 to a deliberate 409 RECORD_LOCKED until #7525; a fourth local spelling here would have rebuilt that divergence one layer down, so a batch row and the single-record PATCH of the same object would disagree about the same error.

    declaresClientRefusal is not widened — #8333's limb is left exactly as it landed.

    What it costs, stated rather than discovered later

    A hook that refuses by throwing an undeclared Error no longer has its sentence echoed on the row. That is the measured cost of a positive list: at this sink an undeclared hook throw is indistinguishable from an undeclared driver throw, which is the whole hole. The remedy is at the producer and now has three accepted spellings — a 4xx status, a 4xx statusCode, or validationFailure(message, fields). The case is pinned rather than regretted.

    Also closed: the old String(err) fallback for an empty message, which renders Error: SQLITE_ERROR: … — the same second leak path #8333 found at its P13.

    Two things from the verification worth keeping

    Ablation (a) alone would not have been sufficient. Reverting protocol.ts leaves the two no-status populations green in both arms — pre-fix quoted everything, so it quoted them too, for the wrong reason. Only the over-broad "blank the tail always" direction separates "quotes what it should" from "quotes everything". Two ablation directions here test different properties; they are not redundant confirmation.

    A fixture triage caught a test this fix would have made vacuous. Of 11 pins asserting an undeclared bare Error's message verbatim (10 in metadata-protocol, one in packages/objectql that a package-scoped sweep would have missed), nine were re-spelled — but upsert-existence's "a real update failure surfaces ITSELF" discriminated by message, and after the fix both the update's and the fallback insert's text are withheld. Its .not.toContain('duplicate key') would have passed even if the fallback had run. Its claim is now carried structurally — the update was attempted, no insert followed — which is stronger than the string ever was.

    Residue


    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

bugSomething isn't workingsecurity

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions