Skip to content

fix(plugin-approvals): render the non-submitter recall refusal through the operation-message catalog - #12725

Merged
os-litant merged 1 commit into
mainfrom
claude/issue-11993-approval-recall-refusal-message
Aug 27, 2026
Merged

fix(plugin-approvals): render the non-submitter recall refusal through the operation-message catalog#12725
os-litant merged 1 commit into
mainfrom
claude/issue-11993-approval-recall-refusal-message

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Part of #11993 — this addresses the card's SECOND expectation only (the refusal message). The first (button visibility) is measured below and deliberately not implemented here, so the card stays open.

The defect

A non-submitter who opened a record someone else had submitted for approval, clicked Recall and was correctly refused read the reason in English regardless of their own locale.

ApprovalService.recall threw one hardcoded sentence. handleApprovalError in @objectstack/rest maps this service's CODE: message throws onto the wire by testing the prefix for the status and then stripping it, shipping what remains as the 403 body's human-readable error — and Console splices that under its own localized label. So an operator in a fully Chinese deployment read a Chinese prefix glued onto English prose they could not act on.

Note the card's own quoted string is stale and was corrected on the card before dispatch: the message reported against 17.2.0 is a repo-wide zero-hit today. Re-measured on origin/main at 15bf9e859, the live refusal was FORBIDDEN: only the submitter may recall this request in packages/plugins/plugin-approvals/src/approval-service.ts. Still English, still spliced — the defect held, only the sentence had moved.

The change

The refusal renders through the shared Operation Message Catalog (@objectstack/spec/system) under the approval_recall_not_submitter key that #12493 landed for it — the mechanism triage ruled (shape A) and the one the catalog's own header names, barring both a package-local string table and a second mechanism for a second producer. Same errors. 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. plugin-security's userFacingDenialMessage is the sibling consumer this mirrors.

The developer's half — the actor and submitter ids the catalog sentence deliberately omits, because the throw site knows the submitter only as an opaque user id — moves to the log, structured, where a developer reads it and a user never does.

The one addition beyond the emitter, named

ApprovalServiceOptions gains an optional messageTranslator, and ApprovalsServicePlugin wires it lazily. This is not incidental scope: the spec half's own changeset states that once the consumer halves land, "any deployment translation defining the two errors.* keys takes effect then". Without a translator reaching renderOperationMessage, the override address the catalog documents is inert for this emitter — a declared-but-unenforced surface. The shape is copied from the two providers already in this plugin's start() (tenancyPosture, the field-visibility source) and from plugin-security's per-call i18n resolution, for the same reason all of them are lazy: the i18n service is contributed by another plugin (ADR-0029 D8) and may start later. A stack with no i18n service still renders the built-in catalog in the caller's locale.

Not changed: who may recall

The gate is byte-identical — the submitter, or a privileged admin releasing a stuck record (#3424). Only the sentence changed, and the FORBIDDEN: prefix the REST layer maps to 403 is untouched. Three control tests pin all three answers, and one pins the prefix surviving the strip.

Verification

Gate union and suite run at 3e5a7bc11, the branch head.

  • pnpm --filter @objectstack/plugin-approvals exec vitest run34 files / 621 tests passed (11 of them new).
  • pnpm --filter @objectstack/plugin-approvals run typecheck — clean. Scope stated honestly: --listFiles confirms it covers approval-service.ts and approvals-plugin.ts (1 hit each) and not the new test file (0 hits) — this package's tsconfig excludes **/*.test.ts. The test file's type coverage comes from check:type-check-debt --re-measure instead, which reports the package's TEST_DEBT unchanged.
  • Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack on the actual changed set (re-derived after scripts/engine-double-contract.pinned.json entered it, which pulled in six further families — all run). Green: engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, type-check-debt, i18n, i18n-stale-fill, nul-bytes, changeset-gate-self-tests, cross-package-test-inputs, objectql-double-limit, objectui-changeset, page-declaration-shape, pm-half-states, published-files, slot-lookup, test-source-alias, type-source-resolution, agent-test-spelling, bash32-floor, cli-command-ids, entry-guard, parse-guard, pnpm-filter-targets, plus check-adr-0087-registration, check-changeset-no-major, check-ci-filter-parity, check-comment-mask-adoption, check-cross-package-test-inputs, check-empty-changeset, check-plugin-teardown-shape, release-rehearsal-clone --self-test.
  • NOT MEASURED, not red: scripts/pm/check-half-states.mjs refuses with PREREQUISITE NOT MET (no valid GitHub credential in this container, exit 3 — its own text says nothing was swept). CI runs it.
  • check:engine-double-contract initially failed on the new test file's fake engine. Repaired the self-serve way — delete() and update() now open with assertEngineDeleteDispatch / assertEngineUpdateDispatch, and the pinned ledger learned the two new rows via --write. The shrink-only baseline was not touched.

Ablation

Reverted the catalog call to the old hardcoded throw, leaving everything else (including the developer log) in place.

  • Mutation proved on disk before any verdict was read: catalog-call sites 1 -> 0, legacy-string sites 0 -> 1, blob hash 6be2255... to c2ee2c7....
  • No rebuild leg is needed for this pair and that is measured, not assumed: the test imports the subject relatively (./approval-service.js), so vitest transforms src/approval-service.ts itself — demonstrated when a malformed doc comment in that source file surfaced as a vitest transform parse error naming src/approval-service.ts. @objectstack/spec does resolve through exports to dist/, and it was built first; the freshly built dist/system/index.js was confirmed to carry the key (4 locale hits) before anything was read.
  • Result — direction RED as predicted, and specifically on the locale row: 6 failed / 5 passed. The zh-CN assertion reported Received: "FORBIDDEN: only the submitter may recall this request" against the expected Chinese sentence — the reported defect, reproduced. The 5 that stayed green are exactly the ones the mutation does not reach: the code-prefix row, the developer-log row, and the three permission-boundary controls.
  • Restore leg proved the same way, not by its exit code: restored blob 6be2255... equals the HEAD blob byte-for-byte, git diff HEAD empty, git status clean, marker counts back to 1 / 0.

What this PR does NOT do — the card's first expectation

The card asks that a non-submitter not see a live Recall button at all. Measured rather than assumed, and the reading is that this repo's server-side half is already correct, and already pinned:

  • sys-approval-request.object.ts declares approval_recall with visible gating on record.viewer.is_submitter == true.
  • viewer.is_submitter is server-computed in ApprovalService.attachViewers as a straight owner check against the caller's userId.
  • sys-approval-request.object.test.ts already pins it: "gates on the server-computed viewer block ([P3] approvals: server-computed viewer capability on getRequest (precise approver gating for declared actions) #3310): approver actions on can_act, submitter levers on is_submitter", asserting approval_recall contains record.viewer.is_submitter.
  • The label in the report, 「撤回审批」, is a repo-wide zero-hit; the platform's own action label is Recall (zh-CN 「撤回」), and exactly one metadata declaration in this repo targets the recall endpoint.

So the button in the report is not rendered from this package's action metadata, and nothing server-side here decides its visibility. Whatever renders it is a console surface that does not honour record.viewer.is_submitter — the half triage already transferred to objectui#6464. Absorbing that into this PR would be a cross-repo change, so it is left where it was routed. I could not read objectui#6464 from this seat (that repo is not attached to this session), so its current state is worth confirming before the card is closed.

Out-of-scope findings, filed unassigned

Changeset

.changeset/approval-recall-refusal-localized.md, minor. Argued rather than assumed: this ships changed user-facing refusal text in a published package, and ApprovalServiceOptions — publicly exported from the package index — gains an optional field, which is additive public API rather than a pure patch.


Generated by Claude Code

…h the operation-message catalog
A non-submitter who clicked Recall on someone else's pending approval was
correctly refused, but read the reason in English regardless of their own
locale: the emitter threw one hardcoded sentence, `@objectstack/rest`'s
`handleApprovalError` ships it as the 403 body's human-readable `error`, and
Console splices that under its own localized label — so an operator in a fully
Chinese deployment saw a Chinese prefix glued onto English prose (#11993).
The refusal now renders through the shared Operation Message Catalog in
`@objectstack/spec/system` under the `approval_recall_not_submitter` key that
landed for it, the same mechanism plugin-security's denial gates use: same
`errors.<key>` override address, same resolution ladder, same guarantee that a
misbehaving i18n service cannot turn a 403 into a 500. The developer's half —
the actor and submitter ids the catalog sentence deliberately omits — moves to
the log.
`ApprovalServiceOptions` gains an optional `messageTranslator`, resolved lazily
and wired by `ApprovalsServicePlugin` alongside `tenancyPosture` and the
field-visibility source, because the i18n service is contributed by another
plugin and may start later. It is what makes the catalog's documented override
address take effect for this emitter.
Not changed: who may recall. The gate is byte-identical (the submitter, or a
privileged admin releasing a stuck record) and the `FORBIDDEN:` prefix the REST
layer maps to 403 is untouched. Both are pinned by new tests, alongside the
zh-CN rendering an operator actually reads.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 8 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/flows.mdx(via ApprovalService (symbol))
  • content/docs/data-modeling/import-mappings.mdx(via /:object/import/jobs (route))
  • content/docs/protocol/objectql/state-machine.mdx(via /:object/import/jobs (route))
What this run could not see
  • 3 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 — 5 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 d29e42f8b1b44b92ea58c1d05e619f27c027c3a9packageMentionDocs.

Which tree this was computed on

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

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs d29e42f8b1b44b92ea58c1d05e619f27c027c3a9 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 27, 2026
@os-litant
os-litant marked this pull request as ready for review August 27, 2026 14:23
@os-litant
os-litant enabled auto-merge August 27, 2026 14:23
@os-litant
os-litant added this pull request to the merge queueAug 27, 2026
Merged via the queue into main with commit 3690922Aug 27, 2026
34 checks passed
@os-litant
os-litant deleted the claude/issue-11993-approval-recall-refusal-message branch August 27, 2026 15:06
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-litant@claude