fix(approvals,rest): tell the truth about approval writes — lock policy, dropped fields, and the batch create ingress (#3794, #3835) - #3834
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 104 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
This was referenced Jul 28, 2026
github-actions
Bot
added
documentation
baozhoutao
changed the title
…ropped fields (#3794) An approval flow reported record writability wrong in both directions: what the user could change said "locked", and what they couldn't said "updated successfully". Both halves were missing signal, not wrong behaviour — the server did the right thing and told nobody. `rowFromRequest` now emits `locks_record`, read from the same `node_config_json` snapshot the `beforeUpdate` lock hook reads, with the same default-true. A client could previously only see "a request is pending" and had to guess whether that locked the record; the Console guessed "locked" every time, which hides the whole point of a `lockRecord: false` node. `POST /batch` (cross-object transactional batch) never wired `onFieldsDropped`, so the one write path the Console's master-detail record form takes was also the one path with no write-observability — a `readonlyWhen` strip there was completely silent. It now collects per-op events and returns them as a top-level `droppedFields` list tagged with each operation's index; omitted when empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dFields (#3794) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ppedFields (#3794) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y` is enforced (#3835) `POST /batch` called `ql.insert` directly, and the engine's INSERT path is static-`readonly`-exempt by design (#3413) — the strip that stops a non-system caller from seeding a read-only column lives at the protocol's create ingress (#3043). So the same forged value was dropped on `POST /data/:object` and written through on `/batch`: one rule, two answers. Create ops now go through `p.createData`, the ingress itself, rather than a second copy of the strip at the REST layer. One ingress means a future change to its policy covers the batch for free, and the carve-outs it already encodes stay intact — the platform-object exemption (a `sys_`/`managedBy` object's own guard must REJECT a forged value, not silently swallow it) and the `isSystem` exemption. `trxCtx` is passed as the context, so the insert still joins the batch transaction and `$ref` resolution is unaffected; the ingress's `droppedFields` fold into the batch's per-op list. Update ops are untouched — the engine enforces both strips on its update path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ock_record` main shipped the same feature first. #3815 (`a6c3f3806`, #3814/objectui#2902) landed `ApprovalRequestRow.lock_record`, computed `cfg?.lockRecord !== false` off the `node_config_json` snapshot — byte-for-byte the rule this branch's `locks_record` computes, from the same snapshot, with the same default. Only the test file collided textually. `approval-service.ts` and the spec contract auto-merged *clean*, which is the dangerous part: the result declared both fields, so `GET /api/v1/approvals/requests` would have shipped one policy under two names and every client would have had to guess which to read. Resolved toward main: `lock_record` is already merged and released in a changeset, so dropping it would be a breaking change layered on a duplicate. Removed from this branch: - `locks_record` on `rowFromRequest` (plugin-approvals) - `locks_record?: boolean` on `ApprovalRequestRow` (spec contract) - its 3 tests — main's 3 `lock_record` tests are a superset, covering `openNodeRequest` / `listRequests` / `getRequest` on all three cases - the `locks_record` half of the changeset, which main's `approvals-expose-lock-record.md` already describes The changeset is rewritten to the half that is still this branch's own (`droppedFields` on `POST /batch`) and renamed to match; `plugin-approvals` comes off its bump list, because after this resolution the branch no longer touches that package. #3835 (batch creates through the create ingress) is untouched and keeps its own changeset. Console follow-up, NOT covered here: objectstack-ai/objectui#2914 is still open and reads `locks_record`. It must be repointed at `lock_record` or the band falls back to "assume locked" — the exact bug #3794 filed. Verified after resolution: build 71/71 · spec 6836 · rest 419 (incl. all 5 new batch tests) · plugin-approvals 318 (incl. main's 3 `lock_record` tests) · metadata-protocol 71 · no generated-artifact drift. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Merged
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3794 (the framework half — the Console half is objectstack-ai/objectui#2914) and #3835.
An approval flow reported record writability wrong in both directions: what the user could change said "locked", and what they couldn't said "updated successfully". Neither was wrong behaviour — the server did the right thing and told nobody. Pulling on the second thread turned up a third problem on the same write path, where the server was not doing the right thing (#3835).
1.
locks_recordon the approval request (#3794 problem 1)The
beforeUpdatelock hook decides on exactly one thing: the node config snapshot'slockRecord(=== false⇒ the update goes through). Nothing exposed it, so a client saw only "a request is pending" and had to guess whether that means locked — and the Console guessed "locked", every time. AlockRecord: falsenode exists precisely so an approver can amend the record while deciding on it; painting "Locked for approval" over that hides the whole feature and approvers never try.rowFromRequestnow emitslocks_record, read from the same snapshot the hook reads with the same default-true, so the two cannot disagree. It flows throughgetRequest/listRequestsand thereforeGET /api/v1/approvals/requests. Optional in the contract for version skew (absent ⇒ assume locked).2.
droppedFieldsonPOST /batch(#3794 problem 2)The engine strips writes to
readonly(#2948) andreadonlyWhen-locked (#3042) fields and completes the write without them. Every write path already reports what it dropped (#3431/#3455) — except the cross-object transactional batch, which never wiredonFieldsDroppedat all.That is not a marginal path: it is the Console record form's save for a master-detail record, i.e. the one surface where a user edits a
readonlyWhenfield. They changed it, the form said "updated successfully", the value never moved, and nothing anywhere said so — the exact symptom in #3794.The response now carries a top-level
droppedFieldslist, each event tagged with theindexof the operation that produced it (resultsentries are bare record echoes, with no envelope to hang a per-row list on). Omitted entirely when nothing was dropped, so the shape stays backward-compatible; the batch still commits either way — a strip is legal semantics, not an error.3. …and that batch was not enforcing
readonlyon create at all (#3835)Wiring up (2) surfaced it:
POST /batchcalledql.insertdirectly, and the engine's INSERT path is static-readonly-exempt by design (#3413 — the #3043 strip deliberately lives one layer up, at the protocol's create ingress). Soreadonlymeant two different things depending on which create endpoint you used.Measured on the showcase,
showcase_contact.lead_score(readonly: true), same signed-in non-system user, identical payload:POST /data/showcase_contactlead_score = null,droppedFieldsreportedPOST /batch(action: 'create')lead_score = 999written, nothing saidnull,droppedFieldsreportedCreate ops now go through
p.createData— the ingress itself — rather than a second copy of the strip at the REST layer. Routing beats re-implementing here for a concrete reason:stripReadonlyForInsertcarries a deliberate scope carve-out a copy would have lost. Platform objects (sys_/managedBy) are exempt on purpose, because their own guards must reject a forged value with 403 (ADR-0086'smanaged_by/package_id, #3004'sowner_idanchor) — silently swallowing it would eat the very payload the guard exists to refuse.isSystemis exempt for the same class of reason. That also rules out the "sink the strip into the engine" option floated in #3835: the engine has no notion of that boundary. One create ingress, and a future change to its policy covers the batch for free (AGENTS.md PD #12).Mechanics that had to keep working, and do:
trxCtxis passed as the context so the insert joins this batch's transaction and rolls back with it;{ $ref: <opIndex> }still resolves (verified live — an invoice + line batch produced a line pointing at the invoice created in the same request); the ingress'sdroppedFieldsfold into the per-op list from (2).Update ops are untouched throughout — the engine enforces both strips on its own update path.
Tests
New: 3 in
approval-service.test.ts(locks_recorddefault-true, explicitfalse, omitted key — throughopenNodeRequest/listRequests/getRequest), 5 inrest-batch-endpoint.test.ts(tagged drop events; omit-when-clean; creates go through the ingress with the transaction context; a forgedreadonlycolumn never reaches the engine and is reported;$refstill resolves through the ingress).Green:
spec6736 ·rest413 ·plugin-approvals280 ·metadata-protocol71.Browser-verified on the showcase
Same signed-in user,
pnpm devshowcase + the objectui console dev server:showcase_invoice_signoff(lockRecord: false), pendinglocks_record: false; band reads "审批中（可编辑）"; PATCH 200showcase_dynamic_approvallead_review (lockRecord: true), pendinglocks_record: true; band reads "审批中已锁定"; PATCH 400 RECORD_LOCKED状态 → Paid+税率in one savetax_rateunchanged, and the Console now warns which field did not landlead_scorePOST /data/:object🤖 Generated with Claude Code