Skip to content

test(approvals): pin the tenant-admin can_override and resumed postures on pending override-recall - #14845

Queued
claude[bot] wants to merge 1 commit into
mainfrom
claude/issue-14755-tenant-admin-override-pins
Queued

test(approvals): pin the tenant-admin can_override and resumed postures on pending override-recall#14845
claude[bot] wants to merge 1 commit into
mainfrom
claude/issue-14755-tenant-admin-override-pins

Conversation

@claude

@claude claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #14755

Test-only. Two assertions added to the existing tenant-admin reverse check in
packages/plugins/plugin-approvals/src/approval-revise.test.ts, mirroring the
platform-admin idiom the same block already uses.

What was missing

The #3424 override-recall block pins both postures in the "no" direction:
the shared loop over ['platform admin', 'tenant admin'] asserts
viewer.can_override === false on a returned request for each. In the "yes"
direction only the platform-admin case was pinned — it reads the request as
PLATFORM_ADMIN and asserts viewer.can_override === true, then asserts
out.resumed === true after the recall. The tenant-admin reverse check drove
the recall but asserted neither, so nothing held the tenant-admin arm of the
flag the gate is spelled against, and nothing held that its recall resumes down
the reject branch rather than terminally cancelling.

The change

+      // The viewer flag the gate agrees with, read by the tenant admin itself.
+      const seen = await service.getRequest(req.id, TENANT_ADMIN);
+      expect(seen!.viewer!.can_override).toBe(true);
+
       const out = await service.recall(req.id, { actorId: 'org_owner' }, TENANT_ADMIN);
       expect(out.request.status).toBe('recalled');
+      expect(out.resumed).toBe(true);                                   // resumed down the reject branch

One file, five insertions, no production code touched.

Deliberately not mirrored: the platform-admin case's expect(seen!.status).toBe('pending').
The card's scope is terminal and names exactly two assertions; pendingRequest()
plus the RECORD_LOCKED assertion on the line above already establish the status,
and a third assertion nobody asked for is the kind of rider this card is fenced
against. Called out here so the omission is a decision on the record rather than
an oversight.

The :22 fence — confirmed intact

APPROVALS_HOOK_PACKAGE at line 22 is untouched and still present:

22:import { bindApprovalLockHook, APPROVALS_HOOK_PACKAGE } from './lifecycle-hooks.js';

packages/plugins/plugin-approvals/test-typecheck-debt.json is byte-identical
to main — the same blob 130e603a028540a348f4028bdefb7d173d634a0b at HEAD, at
origin/main, and on disk. Its TS6133 entry for this file
("TS6133: 'APPROVALS_HOOK_PACKAGE' is declared but its value is never read.": 1)
is unchanged, and both ledger gates agree it did not move:

  • check:test-typecheckOK ... 8 file(s) / 324 error(s) / 27 pinned signature(s) held in test-typecheck-debt.json
  • check:type-check-debtOK — 21 ledger entr(ies) re-measured in 174.7s, 549 raw tsc error(s) total, none above its recorded number. / surplus: none — every entry sits exactly at its measurement

No ledger was shrunk, raised, or regenerated.

Ablation — each new assertion proven able to fail

Three legs, each: mutate, prove the mutation reached disk (removed-text count 0
and injected-text count 1 on a re-read of the file, plus a changed
git hash-object), run, restore with git checkout HEAD -- ABS_PATH, prove the
restore by blob equality against the HEAD blob and an empty git diff HEAD.
Every leg carried a trap ... EXIT INT TERM restore on absolute paths. The
mutated subject is approval-service.ts, imported by the test relatively
(./approval-service.js), so vitest executes the mutated source — no rebuild leg
is involved and none is claimed. HEAD blob of the mutated file, before and after
every leg: 8b10e70d22da66346c5ae48d273066d91d0c4243.

Leg C — expect(seen!.viewer!.can_override).toBe(true) (surgical: only the
tenant-admin arm of the viewer flag, in attachViewers). Blob
8b10e70d…9f030e78….

Tests  1 failed | 1 passed | 22 skipped (24)
AssertionError: expected false to be true // Object.is equality
 ❯ src/approval-revise.test.ts:487:42

The platform-admin reverse check stayed green in the same run while the
tenant-admin one went red — so the new assertion pins the tenant-admin posture
specifically, and nothing else in the file covered it.

Leg B — expect(out.resumed).toBe(true) (resumed = trueresumed = false
on recall's non-revise-window resume path). Blob 8b10e70d…44b65a06….

Tests  1 failed | 23 skipped (24)
AssertionError: expected false to be true // Object.is equality
 ❯ src/approval-revise.test.ts:491:27

Leg A — the posture === 'TENANT_ADMIN' arm of isOverrideActor (kept for the
record; superseded by leg C). Blob 8b10e70d…eb2ececc….

Tests  1 failed | 23 skipped (24)
TypeError: Cannot read properties of null (reading 'viewer')
 ❯ src/approval-revise.test.ts:487:20

Red at the new line, but as a TypeError rather than a value-level failure:
ablating isOverrideActor also removes the caller's read visibility
(loadRequest consults the same predicate), so getRequest returns null. Leg C
was added to get the clean value-level failure, and it is the leg the evidence
rests on.

Test runs

All runs pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 src/approval-revise.test.ts,
serialized through scripts/pm/os-verify-lock.sh on a shared box.

run result
baseline (with the change, whole file) Test Files 1 passed (1) / Tests 24 passed (24)
final, restored tree, whole file Test Files 1 passed (1) / Tests 24 passed (24)

pnpm --filter @objectstack/plugin-approvals typecheck — exit 0
(tsc --noEmit + tsc --noEmit -p tsconfig.scripts.json + check:test-typecheck).

Gates

Derived at the final commit with
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack
(the script takes its own changeset from the merge base: 1 path). 29 families.
All results below are at b51251903, which is the final commit.

28 of 29 green. The one remaining is NOT MEASURED by the gate's own
prescription, not red:

  • check-test-completenessPREREQUISITE NOT MET — this gate grades a saved turbo run test log, and no log was named. Its own text: "running the family locally, record this gate as NOT MEASURED. It is not a red, and there is nothing here to fix." CI tees the log and passes the path, so CI's reading is unaffected.

Three gates first returned a prerequisite refusal and were re-run after building
the closure each one names; all three are green on the second reading, and the
first reading is reported as NOT MEASURED rather than as a pass:

  • check:i18n — first exit 1 with PREREQUISITE NOT MET — the workspace CLI is not built (exit code 1, but the verdict text says nothing was checked). After the closure build: check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys).
  • check:type-check-debt — first exit 3, PREREQUISITE NOT MET. After turbo run build --filter='./packages/*' --filter='./packages/*/*': OK — 21 ledger entr(ies) re-measured ... none above its recorded number.
  • check:dual-build-cjs-loads — first exit 3, PREREQUISITE NOT MET — this gate reads built output. After the same build: OK — 102 published require entry point(s) across 66 package(s) load; 610 emitted CommonJS file(s) parse.

The other 25 were green on the first reading: check-ci-filter-parity,
check-comment-mask-adoption, check-cross-package-test-inputs,
check-keyed-text-bounds, check-plugin-teardown-shape, check-shard-attestation,
check-system-context-census, check-tenant-audit-census,
check-undeclared-dep-imports, docs-audit/check-affected-docs,
docs-audit/check-drift-comment, check:cross-package-test-inputs,
check:doc-authoring, check:engine-double-contract, check:i18n-stale-fill,
check:logger-receiver-detach, check:objectql-double-limit,
check:page-declaration-shape, check:published-files,
check:query-options-erasure, check:slot-lookup, check:test-source-alias,
check:type-check-coverage, check:type-source-resolution, check:where-matcher.

Exit codes were captured by redirect-then-read, never across a pipe, and each
verdict above is quoted from the gate's own output rather than inferred from $?.

Repo-wide pnpm lint (eslint . --no-inline-config) also ran to completion here:
exit 0, no findings. No narrowing was needed, so none is claimed.

No changeset — skip-changeset

This PR publishes nothing from any released package. The only changed file is a
*.test.ts; @objectstack/plugin-approvals declares files: ["dist", "README.md", "CHANGELOG.md"]
and its tsup entry is src/index.ts, so the test never reaches dist/ or the
tarball. check:published-files is green. The skip-changeset label is applied
for that reason, additively, and read back.

Sequencing

The record-lock release path this test depends on is already on main (the
RECORD_LOCKED assertions in this block pass), so there was no blocker here. This
PR does not reopen the lock-release gap and does not touch that work.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code

…ding recall

The tenant-admin reverse check exercised the #3424 override recall but
asserted neither of the two postures its platform-admin sibling pins: the
`viewer.can_override` flag the gate is spelled against, and `resumed` down
the reject branch. Mirror the platform-admin idiom in the same block: read
the request as the tenant admin before the recall and assert the flag, then
assert `out.resumed` after it.

Test-only. No production code changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@claude claude Bot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 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 — 0 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 f3ae441fa2582e0b38e834b76353db0fe729fce5packageMentionDocs.

os-sales commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Landing — PM seat domain:services, R21. Undrafted and auto-merge armed 05:39:11Z on head b5125190354d48ec60a6209f6e1c8a8ce61ff8b0.

Verified before arming, at job level rather than from an aggregate:

The auto-merge confirmation echoes method: MERGE even though SQUASH was requested — the known cosmetic field on a merge-queue repository (finding #14798). ⛔ Not to be "fixed" by disabling and re-enabling.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xs skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

2 participants