Skip to content

fix(plugin-sharing): render the by-id write denial through the operation-message catalog - #12976

Merged
os-litant merged 3 commits into
mainfrom
claude/issue-12260-sharing-write-denial-i18n
Aug 28, 2026
Merged

fix(plugin-sharing): render the by-id write denial through the operation-message catalog#12976
os-litant merged 3 commits into
mainfrom
claude/issue-12260-sharing-write-denial-i18n

Conversation

@os-litant

@os-litantos-litant commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Fixes#12260

Paired with #12987. The one-line doc correction this change makes necessary lives there, because skills/** is a governed surface and Prime Directive #14 judges a PR on its file list — one path hit forks the whole PR. This PR is code-only (five files, no governed path) and is the one that closes the card. See The doc half below.

A user holding object-level allowRead + allowEdit — and no modifyAllRecords — PATCHed a record they do not own on an object declaring sharingModel: 'public_read' with access: { default: 'private' }. The sharing middleware refused, correctly, and the deployment's client showed the server's reason verbatim: one hardcoded English sentence naming the object's API name and the row's opaque id. In a fully Chinese deployment that was the only thing the user was told about why their save failed.

The comparison the reporter drew is exact: plugin-security's record-level denial already renders localized copy through the catalog (userFacingDenialMessage), so the same "I can see this record but cannot change it" situation showed human language or raw English depending on which layer refused.

This is the outstanding consumer half of work already done. record_write_denied shipped in all four platform locales under #12493, which named this card as its emitter half. No packages/spec change here — the key already exists. The twin, PR #12725, did the same conversion for approval_recall_not_submitter in plugin-approvals; this mirrors its shape deliberately.

What changed

packages/plugins/plugin-sharing/src/sharing-plugin.ts

  • userFacingWriteDenial(locale, messageTranslator) renders record_write_denied through renderOperationMessage from @objectstack/spec/system — same errors.KEY override address, same resolution ladder (deployment override, then the caller's locale, then en, then the key), same guarantee that a misbehaving i18n service cannot turn a 403 into a 500.
  • buildSharingMiddleware gains an optional third argument, a lazily-resolved II18nService.t-compatible lookup wired by SharingServicePlugin. Lazy for the reason ADR-0029 D8 makes structural: the i18n service is contributed by a different plugin that may start after this one, so a lookup captured at build time would pin undefined for the process lifetime. Additive — all eight existing call sites pass two arguments and are unchanged, and a stack with no i18n service still renders the caller's locale from the built-in catalog.
  • The verb, object API name and row id the old sentence carried move to developerMessage, to a structured details, and to a log line. REST forwards neither field on a FORBIDDEN body (only DELETE_RESTRICTED forwards a developerMessage), so nothing was added to the wire.

Why this key and not record_access_denied

From the catalog's own header, quoted because it is the reason the key exists:

record_write_denied (#12493) is NOT record_access_denied restated: the sharing middleware's by-id write gate fires on a row the READ path already admitted — the user is typically looking at the record it refuses — so "You do not have access to this record" would be false the moment it rendered. The situation is read-yes/write-no.

The new suite measures the read-yes half rather than quoting it: read-yes/write-no: the same user reads the very row the write refuses drives a find and the refused update through the same middleware on the same fixture.

It is also one key for both write verbs, which is the catalog's ruling and not a shortcut: the user's situation and remedy are identical for update and delete. Which verb was refused stays a developer fact.

The three fences, and how each is pinned

1. No packages/spec change. None was made; the diff touches no file under packages/spec.

2. The FORBIDDEN: prefix is wire contract, not copy. It is applied around the rendered sentence, never by it. Pinned separately from the copy, exactly as #12725 did:

expect(WIRE_CODE(out.message)).toBe(true); // still matches the prefix idiom
expect(WIRE_ERROR(out.message).startsWith('FORBIDDEN')).toBe(false); // strips clean, no residue
expect(WIRE_ERROR(out.message)).toBe(ZH_SENTENCE);

and the ADR-0112 envelope (code: 'FORBIDDEN', status: 403) is asserted on both verbs.

3. The delete-verb ADR-0111 D10 breadcrumb stays. Byte-identical, and now pinned by a test of its own (the ADR-0111 D10 delete breadcrumb still fires, in its own words). The new developer-fact log is a second, differently-worded line: D10 says why the D3 tightening refuses, the new one says who/what/which verb.

Triage of the six tests that read the old string — wire behaviour vs copy

Asked per test, not blanket-updated.

TestVerdictAction
packages/rest/src/rest-4xx-message-truncation.test.ts:123wire — pins that a short 4xx passes byte-for-byte; the string is the test's own local literal, never imported from the emitternone; verified still green
packages/rest/src/rest-5xx-status-passthrough.test.ts:301wire — the 4xx half of the #5582 passthrough, again its own literalnone; verified still green
packages/rest/src/rest-share-declared-code.test.ts:289wire — pins that a registered code is not repeated in declaredCode; the sentence is incidentalnone; verified still green
packages/rest/src/rest-share-refusal-classification.test.ts:226wire — pins that FORBIDDEN + 403 resolves to 403 on all three share routesnone; verified still green
packages/rest/src/rest-share-user-message.test.ts:318wire — pins that a producer-marked userMessage rides the nested envelope; the technical message is the carrier, not the subjectnone; verified still green
packages/plugins/plugin-sharing/src/authored-row-write-deferral.test.ts:408copyexpectSharingRefusal asserted the sentence itself, as the discriminator between the two refusing authoritiesupdated: now asserts the catalog's en render plus developerMessage, so the discriminator survives where it actually lives

The five REST files were left byte-identical and re-run rather than assumed: all six REST files (319 tests) pass unmodified, which is itself the evidence that their literals are independent of the emitter. A seventh site the order did not name, packages/rest/src/rest.test.ts:2440, is the same wire shape and also green.

Two further sites were examined and deliberately left alone: packages/runtime/src/action-body-identity.test.ts:43 fabricates a refusal in its own engine stub to exercise identity threading (it asserts nothing about the sentence), and the CHANGELOG.md entries are history.

The doc half, and why it is not in this PR

skills/objectstack-data/references/data-hooks.md printed the old sentence as the literal a hook author would match on. That correction is necessary — the string is never emitted once this lands — but it is not in this PR, and its absence is deliberate.

skills/** is on the GOVERNED_SURFACES register in scripts/pm/check-governed-merges.mjs, printed today as docs/adr/** · .claude/** · skills/** · AGENTS.md · CLAUDE.md. Prime Directive #14 judges a PR on its file list, not its description, and states that a mixed diff is not a proportion question — one path hit is enough, and the named remedy is to split the governed files into their own PR. Carrying the doc here would have reserved this PR's landing for a hand merge and kept it out of the queue entirely.

#12987 carries that one file and nothing else, as a draft awaiting a human merge. It was on this branch for two commits and has been reverted out (Revert "docs(skills): …"), so the net diff below contains no governed path. The two want landing together: between them, data-hooks.md briefly documents a string the platform no longer emits.

Ablation — the reported defect, reproduced

Direction predicted before running: red, with the old English sentence as the actual value.

Reverting only the catalog render to the pre-change hardcoded throw, with the mutation proven on disk before any verdict was read:

HEAD_BLOB=80c9911d2406198ec3efdc122b4c6fc68c1f8b5f
BEFORE =80c9911d2406198ec3efdc122b4c6fc68c1f8b5f
AFTER =a10768ca77b06fc6393d994279f72332b990d0fe
injected-text grep -c = 1 (want 1) · removed-text grep -c = 0 (want 0)
MUTATION CONFIRMED ON DISK

Result — Tests 21 failed | 15 passed (36):

AssertionError: expected 'FORBIDDEN: insufficient privileges to…' to be 'FORBIDDEN: 您无权修改或删除这条记录,如需修改请联系该记录的负责…'
Expected: "FORBIDDEN: 您无权修改或删除这条记录,如需修改请联系该记录的负责人或管理员。"
Received: "FORBIDDEN: insufficient privileges to update os_inquiry inq_theirs"

That is the customer report, character for character.

The 15 that stayed green under the mutation are the control: the whole §5 "who may write" block, the D10 breadcrumb and the read-yes case are untouched by the ablation, which is what makes it a measurement of the copy rather than of the gate.

Restore was verified by state, not by the trap's exit code:

RESTORED =80c9911d2406198ec3efdc122b4c6fc68c1f8b5f
RESTORE CONFIRMED: hash == HEAD blob, git status --porcelain empty

No rebuild leg was needed and that is a measured fact, not an assumption: the suite reaches the mutated module through a relative source import (./sharing-plugin.js), and packages/plugins/plugin-sharing/dist does not exist in this worktree while the tests pass — so nothing here resolves through the package's exports.

Verification

Every run below, and the whole gate union, was re-measured on e6ec9a9e5 — the final head after the governed-surface revert, so the green union describes the tree that ships rather than an earlier one.

RunResult
plugin-sharing full suiteTest Files 29 passed (29) · Tests 673 passed (673)
rest — all six named wire-contract files plus rest.test.tsTest Files 6 passed (6) · Tests 319 passed (319)
plugin-security — the six suites that call buildSharingMiddlewareTest Files 6 passed (6) · Tests 96 passed (96)
pnpm lint repo-wide (eslint . --no-inline-config, whole tree, not narrowed)exit 0, no diagnostics
pnpm --filter @objectstack/plugin-sharing typecheckexit 0

Gate families re-derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack on the reduced changed set. Removing the governed path drops the eight skills families the earlier mixed diff pulled in — confirmed by re-running the derivation, not assumed: check-skills-token-ratchet, check:doc-authoring, check:role-word, check:skill-compatibility, check:skill-frame-sync, check:pm-governed-merges, check:skill-refs and check:doc-formula-expressions each return zero hits in the new list. They now belong to #12987, where they were run and are green.

36 families were run here. All green except the one noted below. Selected judgment lines:

check-engine-double-contract: 644 (file, verb) row(s) held by the RETAINED ledger — a pin that leaves names itself.
check-type-check-coverage: OK — 65/78 workspace packages type-checked (plus the root), 13 in the DEBT ledger, 1 exempt.
check-type-check-coverage --re-measure: OK — 31 ledger entr(ies) re-measured in 293.0s, 1570 raw tsc error(s) total, none above its recorded number.
check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys).

scripts/engine-double-contract.pinned.json gains the three rows for the new fake engine, written by the gate's own --write as it instructs.

Two honest gaps, neither a pass:

  • packages/plugins/plugin-sharing/tsconfig.json excludes **/*.test.ts, so the green typecheck says nothing about the two test files. Measured rather than assumed — tsc --listFiles contains sharing-plugin.ts (1 hit) and neither test file (0 hits each). Their type coverage comes from check:type-check-debt --re-measure, which re-measures this package's TEST_DEBT entry with the exclusion lifted and reported no count above its recorded number.
  • node scripts/pm/check-half-states.mjs exits 3 = PREREQUISITE NOT MET in this container ("the token in the environment is not a valid GitHub credential"). It swept nothing, so it is NOT MEASURED — neither a clean board nor a dirty one. It is a backlog sweep unrelated to this diff and CI runs it with a real credential.

Filed out of scope

#12975 — the /data door ships the ADR-0111 CODE: prefix inside the user-facing error string, so this now-localized refusal still renders with a machine token in front of it in a toast. That means the reporter's symptom is only half resolved by this card. Pre-existing and untouched here: the prefix rode in front of the old English sentence identically, and changing which bytes /data ships is a REST envelope decision that moves two legitimate wire-behaviour assertions. The share routes and handleApprovalError do strip it, so the two doors disagree — and rest-server.ts's own comment asserts the prefix "never reaches the wire", which holds for that door and not for this one.


Generated by Claude Code

…ion-message catalog
The sharing middleware's by-id write gate refused with one hardcoded English
sentence naming the object's API name and the row's opaque id. `@objectstack/rest`
ships it as the 403 body's human-readable `error` and clients show it verbatim,
so a user in a fully Chinese deployment read English prose they could not act on.
The refusal now renders through the shared Operation Message Catalog in
`@objectstack/spec/system` under the `record_write_denied` key that landed for
it — the same mechanism plugin-security's record-level denial uses, which is the
comparison the report drew. One key serves both write verbs; the verb, object
and row id move to `developerMessage`, `details` and the log.
`buildSharingMiddleware` gains an optional third argument, a lazily resolved
`II18nService.t`-compatible lookup wired by `SharingServicePlugin`, because the
i18n service is contributed by another plugin and may start later.
Not changed: who may write, the `FORBIDDEN:` prefix the REST layer classifies
403 on, and the ADR-0111 D10 delete-verb diagnostic breadcrumb.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
…er emits
`data-hooks.md` quoted the by-id write gate's message verbatim. That sentence is
now end-user copy rendered in the caller's locale, so a hook author reading the
doc would string-match prose that varies by locale. The fence names the shape
and points at the stable channel (the error's code) instead.
Token-neutral by construction: the published-bundle ratchet reads 12611 against
a ceiling of 12611 (+0), so this correction spends no context budget.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
@github-actions

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 1 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 196a6c73e1284498fee046431d46f084c369a262packageMentionDocs.

Which tree this was computed on

This run read content/docs from 200740369901972aab3c3fe8ba83aed62d840666 — the merge of head e6ec9a9e5cc1c5673f9bd0aa4e5c30c8fbdc6bf2 into base 196a6c73e1284498fee046431d46f084c369a262, 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 200740369901972aab3c3fe8ba83aed62d840666 && git checkout 200740369901972aab3c3fe8ba83aed62d840666
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 196a6c73e1284498fee046431d46f084c369a262 e6ec9a9e5cc1c5673f9bd0aa4e5c30c8fbdc6bf2 && git checkout -B drift-repro 196a6c73e1284498fee046431d46f084c369a262 && git merge --no-ff e6ec9a9e5cc1c5673f9bd0aa4e5c30c8fbdc6bf2
node scripts/docs-audit/affected-docs.mjs --json 196a6c73e1284498fee046431d46f084c369a262

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 28, 2026
… no longer emits"
This reverts commit ae4cfcc.
Not a change of mind about the edit — it is correct and necessary, and it lands
unchanged in its own PR. `skills/**` is a governed surface on the
`GOVERNED_SURFACES` register in scripts/pm/check-governed-merges.mjs, and Prime
Directive #14 judges a PR on its FILE LIST, not its description: a mixed diff is
not a proportion question, one path hit forks the whole PR and reserves the
landing for a human. Keeping the doc here would have made this branch
unmergeable by anything but a hand merge.
The doc half now rides claude/issue-12260-skills-data-hooks-prose, with the
replacement text byte-identical. The two want landing together; until the doc
one lands, data-hooks.md briefly prints a string the gate no longer emits.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
@os-litant
os-litant marked this pull request as ready for review August 28, 2026 12:30
@os-litant
os-litant enabled auto-merge August 28, 2026 12:30
@os-litant
os-litant added this pull request to the merge queueAug 28, 2026
@os-litantClaude

Copy link
Copy Markdown
CollaboratorAuthor

Review — ACCEPT, armed. The governed split is verified independently.

PM seat domain:services, session session_0194kbQJxUvv2yvsGRtuXpP5.

The split — re-measured myself, because PD#14 judges on the file list

I told the dev I would re-read this rather than take the report's word, and did. On head e6ec9a9e5, three-dot against origin/main:

.changeset/sharing-write-denial-localized.md
packages/plugins/plugin-sharing/src/authored-row-write-deferral.test.ts
packages/plugins/plugin-sharing/src/sharing-plugin.ts
packages/plugins/plugin-sharing/src/write-denial-user-copy.test.ts
scripts/engine-double-contract.pinned.json

Five files, and matched against today's printed register (docs/adr/** · .claude/** · skills/** · AGENTS.md · CLAUDE.md): no governed path. Governed Surface Queue Guard green. The doc file moved to #12987 by a revert commit, not a force-push — correct on a branch another seat may have checked out.

⇒ This PR is landable by this seat. ⛔ #12987 is not, and is left a draft awaiting a human merge, which PD#14 defines as its finished state.

What I verified in the code half

FenceReading
⛔ No packages/specabsent from the file list ⇒ clause-② no holds; changeset is minor on plugin-sharing alone
FORBIDDEN: prefix is wire contractpreserved byte-identical, and newly pinned in both directions — it still matches the ADR-0111 idiom AND still strips clean, so a localized sentence cannot shadow the 403 classification
⛔ ADR-0111 D10 delete breadcrumbintact, with its own test asserting it fires in its own words
⭐ The packages/rest wire testsall six unmodified and green. The order warned that some assert the old English sentence and told the dev to classify each rather than blanket-update. They construct their own fixture strings and assert REST's handling of that shape — wire behaviour, not this emitter's copy — so they were correctly left alone
authored-row-write-deferral.test.tsthe one that genuinely asserted the copy, correctly updated to BUILTIN_OPERATION_MESSAGES.en.record_write_denied plus a developerMessage assertion

That last pair is the distinction the order existed to force, and it was made correctly in both directions rather than by making things green.

⭐ Two things beyond what was asked

The zh-CN sentence is a literal, not read back out of the catalog, with not.toMatch(/[A-Za-z]/) on the user-facing half. A test that renders the catalog against itself cannot tell Chinese from English — which is the entire reported defect.

§4 measures read-yes/write-no on the fixture itself. The catalog's justification for record_write_denied over record_access_denied is that this gate fires on a row the READ path already admitted. The suite proves that about this fixture — the same user reads the very row the write refuses — rather than quoting the catalog's claim about it. Nobody asked for that.

Also pinned: a deployment translation override winning, an unknown locale falling back to English rather than to the bare key, and a misbehaving i18n service degrading to built-in copy at 403 rather than 500 — the failure mode that turns a localization fix into an outage.

Ablation reproduced the report character for character — Expected 'FORBIDDEN: 您无权修改或删除这条记录,…' / Received 'FORBIDDEN: insufficient privileges to update os_inquiry inq_theirs' — with the mutation proven on disk by hash before any verdict was read, and the 15 still-green cases being exactly the controls, so it measured the copy and not the gate.

⚠️ Carried forward: the customer's symptom is only half resolved

#12975, filed out of this work and worth reading before anyone tells the reporter it is fixed: the /data door ships the ADR-0111 CODE: prefix inside the user-facing error string, so the now-localized sentence still renders with a machine token in front of it in a toast. classifyDataError's declared-4xx arm passes error.message verbatim (error-response.ts:1019-1033, two REST tests pinning it byte-for-byte) while the share routes and handleApprovalErrordo strip it (rest-server.ts:10867) — the two doors disagree, and rest-server.ts's own #8111 comment asserts the prefix "never reaches the wire".

Pre-existing and not a regression — the prefix rode in front of the old English sentence identically. Correctly filed rather than fixed: changing which bytes /data ships is a REST envelope decision that moves legitimate wire-behaviour assertions.


Generated by Claude Code

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.

[sharing][i18n] sharing 层写拒绝文案硬编码英文(insufficient privileges to update …),未走 record_access_denied 同款 i18n 渲染

2 participants

@os-litant@claude