readRowById swallows engine failures into null, so a store outage is indistinguishable from an absent row at every gate that probes with it

Found while implementing #7474 (splitting the assertControlledByParentWrite refusal legs). Filed rather than fixed: fixing it changes what several security gates answer during an outage, which is a wider decision than the ruled card.

What was measured

packages/plugins/plugin-security/src/security-plugin.ts — the shared single-row probe:

private async readRowById(object: string, id: unknown, context: any): Promise<Record< string, unknown > | null> {
  if (id == null || typeof this.ql?.findOne !== 'function') return null;
  try {
    const row = await this.ql.findOne(object, { where: { id }, context });
    return row && typeof row === 'object' ? (row as Record< string, unknown >) : null;
  } catch {
    return null;
  }
}

Three distinct facts collapse into one null: the row does not exist; the engine threw (driver down, table missing, timeout); no engine is wired at all. Every caller then reads null as "no such row".

Why it is more visible after #7474

Before that card, assertControlledByParentWrite's !row branch answered 403 PERMISSION_DENIED — requires edit access to its master record, which was untrue for all three causes equally. It now answers 404 RECORD_NOT_FOUND, which is exactly right for the first cause and newly wrong-in-a-specific-way for the other two: a driver outage is reported to the caller as "that record does not exist", an answer an SDK treats as terminal (do not retry, drop the id) rather than as a transient fault it should back off on. The platform has a code for this — ERR_DATASOURCE_UNAVAILABLE / 503 — and this path cannot reach it.

The same null also feeds getCallerPreImage, so the collapse is not local to the controlled-by-parent gate.

Direction, not a prescription

Distinguishing "absent" from "could not be read" in the probe's return (and letting an engine fault propagate rather than being flattened) is the shape; whether each caller should then fail closed on a fault, or surface the datasource error, is per-gate and is the part that needs deciding. Note the fail-closed direction is already the house answer next door — assertControlledByParentWrite's master-RLS probe catches and treats a throw as "not visible", and the sharing service throwing denies both faces (controlled-by-parent-sharing.test.ts, "fail-closed"). A probe that fails closed on the DETAIL row would answer 403, not 404.

Pointers

packages/plugins/plugin-security/src/security-plugin.ts — readRowById, getCallerPreImage, and the !row branch in assertControlledByParentWrite
assertControlledByParentWrite answers a metadata defect and a missing row with the same 403 PERMISSION_DENIED "requires edit access to its master record" #7474 — the split that made the 404 leg explicit

Triage: needs-user-decision + domain:identity.

Landing anchor (re-verified @ origin/main 1530870): packages/plugins/plugin-security/src/security-plugin.ts:3622 (readRowById, catch→null), feeding getCallerPreImage at :3640-3643 and gate callers at :3477/:3583/:4120 — the collapse is shared, as filed. Plugin-security ⇒ domain:identity.
Why decision-box: the probe-shape half (distinguish "absent" from "could not read") is mechanical, but the body is explicit that the deciding part is per-gate outage semantics — whether each security gate answers fail-closed (403) or surfaces ERR_DATASOURCE_UNAVAILABLE/503 during a store fault. That changes what the public API answers under outage across several gates: contract semantics, not a scoped fix. Once ruled, the execution card queues in this lane.
Dedup: assertControlledByParentWrite answers a metadata defect and a missing row with the same 403 PERMISSION_DENIED "requires edit access to its master record" #7474 (the discovery site, ruled card — deliberately excluded this), no other open card touches the readRowById collapse; The by-id write pre-image gate resolves the row under the caller's own read scope, so an app-authored widener is still dead on private even once checkAuthoredRowWrite admits it #7401 (pm:on-hold) is about read-scope on the pre-image gate, a different mechanism on a neighbouring path — cross-noted, not converged.
target:v17: no — fires only during a store outage; the steady-state behavior is correct. Not one of the four blocking classes.

本评论来自分诊座位 Routine（#5474 试点），不构成认领。

Generated by Claude Code

Maintainer ruling recorded 2026-08-11 (PM session, executing the maintainer's direct instruction in chat — verbatim: 「接受你的全部建议，请更新 issue 的状态和标签」, accepting the four-lens decision-inbox review in full).

Ruling: fail-closed. The probe distinguishes "row absent" from "could not be read"; an engine fault propagates instead of flattening to null. Gates answer fail-closed on a fault (refusal, or ERR_DATASOURCE_UNAVAILABLE/503 where surfacing the outage is the right answer) — never 404 for an outage, which teaches SDKs to treat a transient fault as a terminal absent-row. This aligns with the house posture already next door (the master-RLS probe and the sharing service are fail-closed).

Scope: readRowById + its callers (assertControlledByParentWrite, getCallerPreImage); per-caller posture decided within that scope.

State: needs-user-decision → pm:queue (domain:identity seat).

Generated by Claude Code

Claim: PM loop (domain:identity seat, #6022)
Session: session_01BVc1ekPpi6yaWywAUhfzfd
Branch: claude/issue-7505-readrowbyid-fail-closed
Worktree: objectstack-issue-7505
Domain: domain:identity
Mode & model: M card, mode:subagent (Claude_Code_Remote tools absent from the PM session this shift — same sanctioned fallback as #7555; report lands as an issue comment either way), model: opus.

Scope of record: the maintainer ruling above (08:08Z) — fail-closed. readRowById distinguishes "row absent" from "could not be read"; engine faults propagate instead of flattening to null; gates answer with refusal or ERR_DATASOURCE_UNAVAILABLE/503 on a fault, never 404 for an outage. Scope is readRowById + its callers (assertControlledByParentWrite, getCallerPreImage); per-caller posture decided within that scope, and any caller whose right answer is unclear gets escalated on this card rather than guessed.

Hot-file coordination: #7626 (maintainer direct-dispatch, 09:31Z, session session_01T4VrzFdnQETy7CUUnfxcan) is in flight on the SAME file, packages/plugins/plugin-security/src/security-plugin.ts, at expandSkipCrud ~:1206 and the isPrivate derivation ~:4314. This card's region is readRowById :3622 + callers :3477/:3583/:4120 and getCallerPreImage :3640-3643 — disjoint regions, no shared function. The dispatch prompt carries the standard in-flight-neighbor question plus a merge-main-before-ready instruction; cross-note posted on #7626.

Generated by Claude Code

{
  "issue": 7505,
  "status": "done",
  "premise_still_valid": true,
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7697",
  "branch": "claude/issue-7505-readrowbyid-fail-closed",
  "commits": [
    "a9dc673 fix(plugin-security): propagate engine faults from readRowById instead of flattening to null",
    "c32ae9b Merge remote-tracking branch 'origin/main' into claude/issue-7505-readrowbyid-fail-closed"
  ],
  "files_changed": [
    "packages/plugins/plugin-security/src/security-plugin.ts (4 behaviour changes + per-caller rationale)",
    "packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts (+5 cases, fault option on the store double)",
    "packages/plugins/plugin-security/src/store-fault-fail-closed.test.ts (NEW, 12 cases)",
    ".changeset/security-probe-fault-fail-closed.md"
  ],
  "summary": "readRowById no longer catches: an engine fault propagates out of the probe and out of the gate, so `null` now means only 'row genuinely absent'. The error is re-thrown exactly as the engine threw it rather than re-badged — objectql's DatasourceUnavailableError keeps ERR_DATASOURCE_UNAVAILABLE and mapDataError turns it into 503 — so the PR adds no new error class, no new code and no ADR-0112 ledger entry. Per-caller posture, all four decided within scope: (1) assertControlledByParentWrite no longer answers 404 for an outage; (2)+(3) the two admin-door provenance gates were fail-OPEN under fault (a swallowed fault read as 'not package/platform-managed' and ADMITTED the write) and now refuse; (4) the owner-anchor echo's `catch { unchanged = false }` is removed so the outage surfaces instead of a 403 accusing the caller of an ownership grab. Steady state is unchanged at every call site and pinned in the same describes.",
  "tests": {
    "new_cases": 17,
    "suites_run_all_green_on_top_of_merged_main": {
      "@objectstack/plugin-security": "47 files, 969 tests passed (was 46/952)",
      "@objectstack/runtime": "127 files, 2016 tests passed",
      "@objectstack/plugin-approvals": "21 files, 458 tests passed",
      "@objectstack/http-conformance": "4 files, 72 tests passed"
    },
    "consumer_sweep_direction": "PREFIX filter '...@objectstack/plugin-security' = DOWNSTREAM consumers (23 packages enumerated; ran the four integration-heavy ones that boot the plugin against a real engine)",
    "other_gates": "typecheck clean; eslint clean on the 3 changed source files; check:nul-bytes OK (7091 files) + targeted self-scan clean; check:engine-double-contract OK (the new double declares reads only, out of scope for the same reason the sibling file's is); check:error-code-casing OK",
    "note_on_envelope_assertions": "Rejection cases assert `code` plus the NEGATIVE envelope (not RECORD_NOT_FOUND/404, not PERMISSION_DENIED/403) rather than code+status. Deliberate: the real DatasourceUnavailableError declares no `status` — rest's mapDataError routes it to 503 off the CODE, pinned already in rest.test.ts. Asserting a status the producer never sets would let the cases pass against an error no engine can throw."
  },
  "ablation": {
    "method": "all four behaviour changes reverted in security-plugin.ts, both test files re-run",
    "result": "Tests 8 failed | 36 passed (44) — exactly the 8 fault-path cases flip red, all 36 steady-state cases stay green",
    "tests_that_flip": [
      "controlled-by-parent-sharing: the detail-row probe faulting propagates ERR_DATASOURCE_UNAVAILABLE, not 404",
      "controlled-by-parent-sharing: an engine error with NO code still never becomes a 404",
      "store-fault: FAULT (by id) package gate — outage propagates, does NOT read as 'no package row here'",
      "store-fault: FAULT (bulk filter) package gate — one gate cannot answer two ways for one outage",
      "store-fault: FAULT (by id) asset gate — outage propagates instead of admitting the delete",
      "store-fault: FAULT (bulk filter) asset gate — outage propagates there too",
      "store-fault: FAULT owner echo — outage propagates instead of being read as 'you are not the owner'",
      "store-fault: the fault is NOT memoized as a verdict — a retry re-probes the store"
    ],
    "headline_failure_verbatim": "AssertionError: expected 'RECORD_NOT_FOUND' to be 'ERR_DATASOURCE_UNAVAILABLE' — the reverted code reproduces the reported defect exactly",
    "tests_that_do_NOT_flip_by_design": "the 36 steady-state cases (absent row still 404, package row still 403, owner echo still tolerated, absent pre-image still denies, ordinary field-only update still admitted). That asymmetry is the point: it proves the fault path moved and nothing else did."
  },
  "neighbor_7626": {
    "answer": "No. My change alters no behaviour #7626's fix or tests can observe, and there is no textual conflict. Evidence below is measured, not asserted.",
    "textual": "git merge-tree --write-tree HEAD origin/claude/issue-7626-expand-crud-bypass auto-merges security-plugin.ts CLEANLY. Their hunks in that file are lines 1194-1310 (origin/main coords); mine are 1807, 3525, 3631, 3678, 4137, 4196. Nearest gap ~530 lines, zero overlap. No shared test file either: they touch security-plugin.test.ts, I touch controlled-by-parent-sharing.test.ts and a new file.",
    "behavioural": "Their region `expandSkipCrud` is gated on `opCtx.operation === 'find'` (grep: defined at :1206, used only at :1216 and :1274, all read-path). EVERY readRowById caller is on a WRITE path — assertPackageManagedWriteGate and assertSystemRowWriteGate accept only ['insert','update','delete','transfer','restore','purge']; getCallerPreImage is reached only from step 3.5 (insert/update owner anchor) and step 3.6 (insert/update RLS check); assertControlledByParentWrite is the by-id write gate. No operation can be in both regions at once, so a CRUD-gate pre-image probe under store fault cannot arise on their path.",
    "isPrivate_check": "The prompt listed '~:4314 isPrivate derivation' as their second region. Their LANDED patch does not touch it — they delete the waiver outright instead. I checked the one place where their work and mine could have met through shared vocabulary: secMeta.isPrivate feeds hasTransferGrant() at :1771-1772, which gates whether my changed owner-echo probe runs at all. Their only change to that vocabulary is packages/core/src/security/operation-private-keys.ts, and it is COMMENT-ONLY (6 lines, re-documenting __expandRead's new meaning). So hasTransferGrant() is unaffected.",
    "flagged_for_the_PM_not_mine_to_fix": "The same trial merge DOES conflict in packages/objectql/src/engine.ts — a file this PR does not touch. Their branch is not based on current main (merge-base cc3555e), and #7594 (245d1dc) changed engine.ts on main after their base. That conflict is between #7626 and main; raised only so it is not misattributed to this card.",
    "merge_state": "origin/main merged into this branch (c32ae9b) and the full plugin-security suite re-run green on top: 47 files / 969 tests. The merge brought in driver-mongodb/spec/metadata changes and touched zero plugin-security files."
  },
  "escalations": [],
  "deviations": [
    "SCOPE — one genuine widening, flagged for review. Beyond the four readRowById call sites, I also removed the sibling `.catch(() => null)` on the BULK-FILTER branch of both provenance gates. It is not a readRowById call, but it sits inside a direct caller and asks the same question of the same store: leaving it would make one gate answer 'refuse' for a single id and 'admit' for a filter during the same outage. Two lines each; trivially revertible if the PM judges it outside the ruling's scope.",
    "The 'no engine wired' limb (the third fact the issue names) now throws rather than returning null. Unreachable in a real deployment — start() registers no security middleware at all without a query engine — so its only reachable form is a non-conforming engine double, which must not manufacture a 404. Verified across all suites above: nothing depends on the old behaviour.",
    "DELIBERATE NON-CHANGE, pinned as a test rather than left silent: the master-visibility probe inside assertControlledByParentWrite still catches and answers 403. The two probes ask different questions — 'does this row exist' (an outage leaves it unanswered, and answering 'no' is the terminal lie this card removes) versus 'is this master visible to you under your own write policy' (whose fail-closed default genuinely IS 'not visible'). Both the issue body and the ruling name this probe as the house posture to MATCH, so I read it as decided, not as a site to change. Not escalated for that reason — but it is the one place where a fault still resolves to 403, so the PM may want to confirm.",
    "Reported at draft-PR time per the current contract; CI convergence, the ready-flip and auto-merge are the PM's. No idle-polling of CI."
  ],
  "changeset": ".changeset/security-probe-fault-fail-closed.md — patch bump on @objectstack/plugin-security, English, describes the behaviour change (store faults now propagate from permission pre-image probes instead of reading as absent rows), names the two fail-open gates, and states the migration-visible consequence: callers may now see 503 ERR_DATASOURCE_UNAVAILABLE where they previously saw 404, 403, or a silent success, but only while the datasource behind the probed object is down."
}

Generated by Claude Code

Review: ACCEPT — PR #7697 (PM session session_01BVc1ekPpi6yaWywAUhfzfd; reviewed the diff line-by-line, not the report).

Ruling conformance, verified per caller against the diff:

readRowById no longer catches; null now means only "id was never given" or "row genuinely absent". No new error class/code/ledger entry — the engine's own error (incl. ERR_DATASOURCE_UNAVAILABLE → 503 via mapDataError) propagates untouched. Matches the ruling verbatim, including "never 404 for an outage".
assertControlledByParentWrite: outage no longer manufactured into 404 RECORD_NOT_FOUND; the absent-row 404 is pinned unchanged in the same describe.
The two provenance gates (assertPackageManagedWriteGate, assertSystemRowWriteGate): the report's headline finding — both were fail-open under fault — is confirmed in the pre-image of the diff, and both now refuse by propagation. Steady-state pins on the same fixtures (package row still 403, admin-authored row still admitted) hold.
Owner-anchor echo: catch { unchanged = false } removed; the outage surfaces instead of a fabricated "changing record ownership" 403. The owner-enumeration oracle is untouched (absent and unreadable-by-caller still both null → deny) and the no-memoization-of-faults case pins the retry path.

Scope call (PM judgment, open veto window): the dev's one declared widening — removing the sibling .catch(() => null) on the bulk-filter branch of both provenance gates — is judged inside the ruling, not an extension of it. The ruling's operative sentence is "Gates answer fail-closed on a fault"; both branches are the same gate asking the same question of the same store, and keeping the catch would have the gate refuse a by-id write and admit the equivalent filter write during one outage. Two lines, ablation-covered, trivially severable if the maintainer disagrees — say so here and it comes out in a follow-up.

Deliberate non-change confirmed: the master-visibility probe keeps its catch→403. The ruling itself names that probe as the house posture to match; the asymmetry is now pinned by a dedicated test rather than left readable as an oversight. No escalation needed.

Verification shape: ablation flips exactly the 8 fault-path cases, 36 steady-state cases stay green — the asymmetry that makes the new tests pins rather than restatements. Suites green on top of merged origin/main: plugin-security 969, runtime 2016, plugin-approvals 458, http-conformance 72; consumer sweep ran the downstream prefix direction. Changeset present, patch on @objectstack/plugin-security, states the migration-visible consequence honestly (503 where there was 404/403/silent success, only during an outage).

Neighbor #7626: measured disjoint (git merge-tree clean on security-plugin.ts; their region is find-gated, every readRowById caller is write-path). Their branch's separate conflict with main in packages/objectql/src/engine.ts (from #7594) is flagged on their thread — not this card's.

Docs-drift advisory (12 pages): no doc edits needed — this PR changes only fault-path semantics; the referenced permission docs describe steady-state behavior, which is pinned unchanged. The changeset carries the outage-visible consequence for release notes.

Next: flipping #7697 ready and enabling auto-merge; the merge queue's full suite is the last gate.

Generated by Claude Code

Merge origin/main into claude/issue-7626-expand-crud-bypass

fix(plugin-security): propagate engine faults from readRowById instea…

test(plugin-security): the package-position gate pins move beside the…

Navigation Menu

Repository navigation

`readRowById` swallows engine failures into `null`, so a store outage is indistinguishable from an absent row at every gate that probes with it #7505

What was measured

Why it is more visible after #7474

Direction, not a prescription

Pointers

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

readRowById swallows engine failures into null, so a store outage is indistinguishable from an absent row at every gate that probes with it #7505

Description

What was measured

Why it is more visible after #7474

Direction, not a prescription

Pointers

Activity

os-zhuang commented on Aug 11, 2026

os-zhuang commented on Aug 11, 2026

os-help commented on Aug 11, 2026

os-help commented on Aug 11, 2026

os-help commented on Aug 11, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`readRowById` swallows engine failures into `null`, so a store outage is indistinguishable from an absent row at every gate that probes with it #7505