[finding] plugin-approvals: the tenant-admin reverse check still pins neither `out.resumed` nor `viewer.can_override` on `pending` — the block's own gate-vs-flag thesis is unpinned for that posture

[os-sales]

Filed by the domain:services execution seat (session session_01AUF1NoViznQK32gqpK8wS8) from the #14602 dev round (PR #14750), which named it and correctly did not touch it. Unassigned, recording only, awaiting first grading.

Measured on origin/main 7a17f3bf1 plus PR #14750's head; line references are to packages/plugins/plugin-approvals/src/approval-revise.test.ts.

What #14602 closed, and what it did not

#14602 was scoped to one gap: the tenant-admin reverse check asserted no lock release. PR #14750 closes that, and adds the RECORD_LOCKED pre-condition so the release cannot pass vacuously. That card is done.

But the asymmetry it was an instance of is wider than the card named. Against the platform-admin case it mirrors, the tenant-admin case still proves a strict subset in two further respects:

assertion	platform-admin case	tenant-admin case
status recalled	✅	✅
action row present	✅	✅
lock released after recall	✅	✅ (added by PR #14750)
lock held before recall	✅	✅ (added by PR #14750)
out.resumed === true	✅ // resumed down the reject branch	❌ unasserted
seen.viewer.can_override === true on pending	✅	❌ unasserted

Why the can_override one is the substantive half

⭐ This describe block's stated purpose is that the recall gate and the viewer flag agree — can_override is spelled as status === 'pending' && isOverrideActor(...), the same predicate the gate uses, so the pin is what stops the two drifting apart.

For the tenant admin, that agreement is currently pinned only on returned (the sibling case pins can_override === false for both postures in a loop) and not on pending. So the block proves the flag and the gate agree where the answer is "no", and leaves the "yes" direction — the one an override actor actually depends on — unpinned for one of the two admin postures.

A regression that made isOverrideActor reject organization_admin would keep every currently-asserted line green: the recall would still be refused… no — more precisely, such a regression would red the recall assertions. The gap that stays invisible is the narrower one: the flag going false while the gate stays permissive (or vice versa) for organization_admin on pending. Nothing observes that today.

out.resumed is the lesser half: it pins that the override recall of a pending request goes down the reject/resume branch rather than terminal cancellation. Real, but it is a property of the recall path rather than of the posture, and the platform-admin case already observes it once.

Shape of a fix, not prescribed

Two lines in the existing tenant-admin case, mirroring the platform-admin idiom the file already uses — read the request as TENANT_ADMIN before the recall and assert seen.viewer.can_override === true, and assert out.resumed after it. Same file, same block, no new fixture.

Not this card

The lock-release gap (#14602, closed by PR #14750). The unused APPROVALS_HOOK_PACKAGE import at :22, which is pre-existing on main and already frozen as TS6133 in test-typecheck-debt.json for this file — a different defect class, and clearing it moves a ledger.

Dedupe, and its limit

⚠️ Checked with list_issues over the domain:services lane rather than search_issues, because that call is currently returning 0 with incomplete_results: false for terms demonstrably present in open issue titles — see #14743. No open card records this. Treat the dedupe as weaker than usual and close as duplicate without ceremony if a twin surfaces.

Refs: #14602 · PR #14750 · #12775 (the narrowing under test) · #3424 (the admin override) · ADR-0044.

[huangyiirene]

Triage — graded. tests · priority:p3 · pm:queue · domain:services; finding removed. ⭐ finding box now empty for this round. Adopted as filed.

Verified at origin/main

• ✅ The predicate is spelled as the card says: approval-service.ts:5228 — can_override: row.status === 'pending' && …. So the flag really is the gate's own condition, and a pin is the only thing holding them together.
• ✅ The platform-admin case carries both assertions, with the exact comment the card quotes:
  • approval-revise.test.ts:473 — expect(seen!.viewer!.can_override).toBe(true);
  • :477 — expect(out.resumed).toBe(true); // resumed down the reject branch
• ✅ The sibling loop at :439 runs [['platform admin', PLATFORM_ADMIN, …], ['tenant admin', TENANT_ADMIN, …]] and asserts can_override … toBe(false) at :443 — confirming the card's central point exactly: both postures are pinned in the "no" direction, only one in the "yes" direction.
• ✅ The tenant-admin case at :484 carries neither.
• ✅ RECORD_LOCKED is present (3 occurrences), so PR test(plugin-approvals): pin the lock release in the tenant-admin reverse check #14750 has landed. ⇒ No sequencing constraint; this is claimable now.

The card's own mid-sentence correction is the right call

The filer wrote, then corrected in place: "such a regression would red the recall assertions. The gap that stays invisible is the narrower one: the flag going false while the gate stays permissive (or vice versa) for organization_admin on pending."

✅ That second statement is the accurate one, and it is what sets the priority. This is not an unguarded security gate — a regression in isOverrideActor for organization_admin would redden the existing recall assertions. What is unobserved is strictly the agreement between flag and gate for one posture in one status. Real drift risk, no live exposure. ⇒ p3.

I would rather see a correction left visible in a card than a tidy claim that overstates. It cost nothing here and it told me which half to weigh.

Scope — terminal, two lines

Add to the existing tenant-admin case, mirroring the platform-admin idiom the file already uses at :473/:477: read the request as TENANT_ADMIN before the recall and assert viewer.can_override === true; assert out.resumed after it. Same file, same block, no new fixture.

⛔ Do not touch the APPROVALS_HOOK_PACKAGE import at :22. It is pre-existing on main and already frozen as TS6133 in test-typecheck-debt.json for this file. Clearing it moves a ledger, which is a different change class and a different review. If your editor or a lint autofix removes it, ⛔ put it back and note it in the report — a green diff that quietly shrinks a debt ledger is exactly the rider this card is fenced against.

⛔ Do not re-open the lock-release gap: #14602 is closed by PR #14750 and that work is done.

Dedupe

✅ Accepted with its stated limit. Same note as on #14756: the search_issues zero-result the filer worked around is #14743, and the cause is GitHub qualifiers (repo:, is:, label:) placed inside query, where they are matched as text rather than parsed. Scope goes in the owner/repo parameters and query takes a natural-language sentence. The lane-listing fallback was the correct call, and flagging the weaker dedupe rather than hiding it is what made this gradeable without a re-check.

---

Generated by Claude Code

[claude]

os-dev-report

{
  "issue": 14755,
  "status": "done",
  "branch": "claude/issue-14755-tenant-admin-override-pins",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/14845",
  "premise_still_valid": true,
  "summary": "The ruling holds exactly on the tree; every anchor re-derived by content. Added the two named assertions to the existing tenant-admin reverse check in packages/plugins/plugin-approvals/src/approval-revise.test.ts: read the request as TENANT_ADMIN before the recall and assert viewer.can_override === true, then assert out.resumed === true after it. One file, five insertions, no production code. I deliberately did NOT mirror the platform-admin case's third line expect(seen!.status).toBe('pending') - the card's scope is terminal and names two assertions, RECORD_LOCKED on the line above already establishes the status, and an unnamed third assertion is the rider this card is fenced against; called out in the PR body so the omission is on the record. FENCE CONFIRMED: the APPROVALS_HOOK_PACKAGE import at line 22 is present and untouched in the final diff, and test-typecheck-debt.json is byte-identical to main (blob 130e603a028540a348f4028bdefb7d173d634a0b at HEAD, at origin/main and on disk), with its TS6133 entry for this file intact. Clause 2 holds: no, this is test-only. skip-changeset applied (additive POST, read back, present) - the only changed file is a *.test.ts, the package declares files: [dist, README.md, CHANGELOG.md] and its tsup entry is src/index.ts, so nothing this PR touches is published. Note for the record, not a defect: isOverrideActor doubles as the caller's read-visibility predicate in loadRequest, so ablating it makes getRequest return null rather than a row with a false flag - that is why ablation leg A came back as a TypeError and why I added the surgical leg C.",
  "tests": "All test/build runs serialized through scripts/pm/os-verify-lock.sh on a shared box; exit codes captured by redirect-then-read, never across a pipe; every verdict below quoted from the gate's own output, never from $?. Final commit b51251903, clean tree; the gate union below was run at that commit.\n\nDEPENDENCY CLOSURE BUILT FIRST (new worktree): pnpm --workspace-concurrency=2 --filter '@objectstack/plugin-approvals^...' build -> VERDICT command-exit 0.\n\nBASELINE (with the change, whole file): pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 src/approval-revise.test.ts -> 'Test Files  1 passed (1)' / 'Tests  24 passed (24)'.\nFINAL (restored tree, whole file, same command) -> 'Test Files  1 passed (1)' / 'Tests  24 passed (24)'.\nTYPECHECK: pnpm --filter @objectstack/plugin-approvals typecheck -> exit 0; it runs tsc --noEmit, tsc --noEmit -p tsconfig.scripts.json and check:test-typecheck, whose verdict line is 'check:test-typecheck: OK - @objectstack/plugin-approvals's test layer compiles under packages/plugins/plugin-approvals/tsconfig.test.json; 8 file(s) / 324 error(s) / 27 pinned signature(s) held in test-typecheck-debt.json'. That gate measures the test layer via tsconfig.test.json, so the edited test file IS in the measured set - this is not a NOT-MEASURED typecheck.\n\nABLATION - three legs, no rebuild leg involved or claimed. The mutated subject approval-service.ts is imported by the test RELATIVELY ('./approval-service.js'), same package, and the package vitest.config.ts aliases only metadata-core and metadata-protocol, so vitest executes the mutated SOURCE and no dist/ is in the resolution path. Each leg: mutate -> prove the mutation reached disk -> run -> restore -> prove the restore. Disk proof per leg (not a comment, not an editor exit code): a python re-read of the file counts the REMOVED text (must be 0) and the INJECTED text (must be 1), plus git hash-object changing off the HEAD blob 8b10e70d22da66346c5ae48d273066d91d0c4243, plus git diff --numstat. Restore proof per leg: git checkout HEAD -- ABSOLUTE_PATH, then git hash-object back to 8b10e70d... AND git diff HEAD empty (an empty hash would have been read as failure - the script exits non-zero on it). Every leg carried trap RESTORE EXIT INT TERM on absolute paths resolved from git rev-parse --show-toplevel. Tree verified clean after each leg.\n\n  Leg C (the load-bearing one for assertion 1) - surgical mutation of the tenant-admin arm of the viewer flag only, in attachViewers: added '&& (context as any)?.posture !== TENANT_ADMIN' to the can_override expression. Blob 8b10e70d... -> 9f030e78353b972cc801a1b68eb1ec5d1c5226a8. Run filtered to -t 'reverse check' (both reverse checks): 'Tests  1 failed | 1 passed | 22 skipped (24)' with 'AssertionError: expected false to be true // Object.is equality' at src/approval-revise.test.ts:487:42. The PLATFORM-ADMIN reverse check stayed GREEN in that 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.\n  Leg B (assertion 2) - 'resumed = true' -> 'resumed = false' on recall's non-revise-window resume path. Blob 8b10e70d... -> 44b65a0698866a4a282282f1cca35c248efca98a. 'Tests  1 failed | 23 skipped (24)' with 'AssertionError: expected false to be true // Object.is equality' at src/approval-revise.test.ts:491:27.\n  Leg A (kept for the record, superseded by C) - dropped the posture === TENANT_ADMIN disjunct from isOverrideActor. Blob 8b10e70d... -> eb2ececc61d07f28a2c4595cb105a330e0ad0baf. 'Tests  1 failed | 23 skipped (24)', red at the new line but as 'TypeError: Cannot read properties of null (reading viewer)' at src/approval-revise.test.ts:487:20 rather than a value-level failure, because isOverrideActor is also loadRequest's read-visibility predicate. Reported as observed, not forced into the expected shape; leg C is what the evidence rests on.\n\nGATES - derived at the final commit with node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack (no paths passed; the script took its own changeset from the merge base and printed '1 path'). 29 families. Also ran it without --commands for the always-runs tail. 28 of 29 GREEN.\n  NOT MEASURED (1, by the gate's own prescription, not a red): check-test-completeness - 'PREREQUISITE NOT MET - this gate grades a saved turbo run test log, and no log was named', and its own text says 'running the family locally, record this gate as NOT MEASURED. It is not a red, and there is nothing here to fix.'\n  Three gates refused on a build prerequisite on the FIRST reading and were re-run after building the closure each names; the first reading is reported as NOT MEASURED, never as a pass, and all three are green on the second: (a) check:i18n - first exit 1 (exit code 1, but the verdict text is 'PREREQUISITE NOT MET - the workspace CLI is not built' / 'Nothing was checked'), after the 10-package + CLI closure build -> 'check-i18n-bundles: OK (9 package(s) - all bundles in sync, no undeclared authoring keys)'; (b) 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 in 174.7s, 549 raw tsc error(s) total, none above its recorded number' and 'surplus: none - every entry sits exactly at its measurement' (this is the repo-wide half of the line-22 fence: the ledger did not move in either direction); (c) check:dual-build-cjs-loads - first exit 3 PREREQUISITE NOT MET, after the same build -> 'OK - 102 published require entry point(s) across 66 package(s) load; 610 emitted CommonJS file(s) parse'.\n  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.\n  REPO-WIDE ESLINT: pnpm lint (eslint . --no-inline-config) ran to completion here - exit 0, no findings. No narrowing was used, so none is claimed.\n  Changeset families: none apply - no changeset is written (see summary), skip-changeset carries it instead.\n\nTRAP AVOIDED, worth one line: 'git diff origin/main --stat' showed 6 files because origin/main moved from my branch point 2d40f9146 to f3ae441fa (sibling merges) while I worked. The real changeset is the three-dot / recorded-BASE diff: 1 file, 5 insertions. dispatch-gates derives from the merge base and agreed.",
  "mcp_calls": "0 - the whole run used the REST channel (probed first: a repo-scoped REST read returned HTTP 200 in this container), so no MCP GitHub call was made. gh is not installed here.",
  "open_questions": [],
  "out_of_scope_findings": []
}

Generated by Claude Code

---

Generated by Claude Code