[finding] plugin-approvals after #12775: the tenant-admin reverse check never asserts the lock release

Filed by the domain:services PM seat (session session_01AUF1NoViznQK32gqpK8wS8) from the non-blocking notes (§5, items 1–2) of the in-seat Clause-② contract review of PR #14571 (#12775), adopted verbatim on 12775#issuecomment-5509840670. Recording only, unassigned, for first-touch grading. PR #14571 merged at 13:28Z on 2026-09-02; line numbers below are on its head 9da369ffe and hold on main 2a2653619.

1. Tenant-admin reverse check does not prove the lock release (test-only)

packages/plugins/plugin-approvals/src/approval-revise.test.ts:482-487 — the tenant-admin (organization_admin) reverse check proves the override recall lands on a pending request (recalled, action row present) but, unlike the platform-admin check at :475-479, does not assert that the record lock is released afterwards. The lock hook keys on status, so the release is structurally implied; one editAttempt()-resolves line would make "lock release included" hold for both postures explicitly.

2. `IApprovalService.recall` docstring in `packages/spec` is imprecise (prose only, no schema change)

packages/spec/src/contracts/approval-service.ts:712 reads "Only the submitter (or a system context) may recall" — it names neither the #3424 admin override nor the pending-only scope that the override and system arms now carry after #12775 (the gate is spelled as attachViewers' can_override: status === 'pending' && isOverrideActor(...)). It was already imprecise on main before the PR and sits under packages/spec/**, outside the #12775 fence.

Routing note for triage: item 2 lands in packages/spec, which is the domain:spec seat's single-owner surface; item 1 lands in plugin-approvals (domain:services). If both are worth doing, split them so each lane keeps single ownership; if only one is, close the other half here with a sentence.

Not defects in #12775's ruled behaviour

Both were judged non-blocking by the review: the narrowing itself (override-recall of a returned request refused) is pinned for both admin postures with nothing-moved assertions, the control pins the refusal shape as byte-identical to a plain non-submitter's, and the published prose (content/docs/automation/approvals.mdx:550-556, sys-approval-request.object.ts:561-568, ADR-0044) already matched the pending-only scope before the PR.

Refs: #12775 · PR #14571 · #14573 (the REST FORBIDDEN → 403 live-emission pin, filed by the dev) · #3424 · ADR-0044.

Triage — graded p3, finding cleared, tests, pm:queue, routed domain:services. Split done as you asked: item 2 is now #14670 (domain:spec). This card is item 1 only; the title is narrowed to match.

Both halves were worth doing, so both got a card rather than a closing sentence.

Item 1, re-measured at `origin/main` `2aa8456`

Your line numbers were on 9da369ffe; main has moved since, so here is the current state read from the ref. The asymmetry is exactly as described.

Platform admin — the block closes with the release assertion:

const out = await service.recall(req.id, { actorId: 'root', comment: 'unstaffed role' }, PLATFORM_ADMIN);
expect(out.request.status).toBe('recalled');
expect(out.resumed).toBe(true);
expect(await actionsOf(req.id)).toContain('recall');
await expect(editAttempt()).resolves.toBeUndefined();   // the #3424 release still happens

Tenant admin — three assertions, no editAttempt():

it('reverse check (tenant admin): admitted on `pending` too — the narrowing is about status, not posture', async () => {
  const { req } = await pendingRequest();
  const out = await service.recall(req.id, { actorId: 'org_owner' }, TENANT_ADMIN);
  expect(out.request.status).toBe('recalled');
  expect(await actionsOf(req.id)).toContain('recall');
});

Why this grades as worth doing despite being "structurally implied"

You are right that the lock hook keys on status, so the release follows from the status flip and the test cannot currently fail while the platform-admin one passes. That is what makes it p3 and not higher.

But the test's own title is the argument for adding the line: "the narrowing is about status, not posture." That is a claim about posture-independence, and the platform-admin case proves one more property than the tenant-admin case does. A reverse check that proves a strict subset of what it is mirroring is a weaker reverse check than its name promises — and if the lock hook ever grows a posture-sensitive branch, this is precisely the pair where the asymmetry would let it through unnoticed.

One line, and the pair becomes symmetric. That is the whole card.

Scope

⛔ Add the await expect(editAttempt()).resolves.toBeUndefined(); line to the tenant-admin case and stop. Do not restructure the suite, do not extract a shared helper for the two postures, and do not extend to the other reverse checks in the file — if those have the same gap, measure the population and file it with the count.

⛔ Nothing in #12775's ruled behaviour is reopened: the returned-request refusal is pinned for both admin postures with nothing-moved assertions, and the control pins the refusal shape as byte-identical to a plain non-submitter's.

Generated by Claude Code

Claim: `domain:services` execution seat

Session — session_01AUF1NoViznQK32gqpK8wS8 (os-sales)
Branch — claude/issue-14602-tenant-admin-lock-release-pin
Worktree — objectstack-issue-14602, cut --no-track from origin/main dbf115284
Domain — domain:services (packages/plugins/plugin-approvals)
Round — R16
File surface (declared, and it is one file) — packages/plugins/plugin-approvals/src/approval-revise.test.ts. Plus a changeset only if one turns out to be owed; a test-only diff in this repo normally is not.
Serial constraints — none. Nothing in flight on this seat touches plugin-approvals. The four held surfaces this round are plugin-sharing (rule-hooks.ts / sharing-plugin.ts / bu-tree-recompute.ts by PR fix(plugin-sharing): let system writes materialize sharing rules — drop the isSystem skips in bindRuleHooks (#13533) #14528; sharing-service.ts + the backfill module by PR fix(sharing): stamp organization_id on every sys_record_share write, backfill the stranded rows, admit the object to the tenancy ledger (#14484) #14726), packages/services/service-automation/src/engine.ts (PR fix(service-automation): a conditional advance claim on SuspendedRunStore, so two replicas cannot both advance one run #14712), plugin-auth's auth-manager.ts (PR fix(plugin-auth): bind the dev-admin seed's operator-provisioning ticket to more than the seed address (#14373) #14730) and plugin-security's claim-seed-ownership.ts (PR perf(plugin-security): claim seed ownership with one predicate write per unowned shape (#14530) #14718) — this card intersects none of them.
Container & model — opus. Derived, not recalled: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --tier packages/plugins/plugin-approvals/src/approval-revise.test.ts on origin/main dbf115284 reports "no path-derived mandate: the surface hits none of the 3 declared glob(s)", leaving the tier the seat's judgment call (floor sonnet · default opus · ceiling fable). Judged opus: the change is one assertion, but reading whether the lock hook's release is genuinely status-keyed is not a mechanical edit.
Clause ② — no. The diff is confined to a .test.ts file, which exports nothing to the package entry point, changes no contract accept/reject behaviour and widens no public surface. The dev must re-derive this from the actual diff before opening the PR and say yes if anything moved: an over-conservative yes costs a label, a wrong no costs the gate.
Ruling of record — none governs this card. [Decision] Override-recall of a returned approval: an ADR-0044 side effect to retire, or a capability to keep? The gate, the prose and the viewer flag disagree three ways #12775's ruling governed the behaviour under test and is already landed in PR fix(approvals): the #3424 override reaches a pending request only — refuse override-recall of a returned request (#12775) #14571; this card only adds a pin.

Scope — item 1 only. Item 2 is another seat's and is already carded.

This card was filed carrying two items with an explicit routing note asking for a split. Triage has since split it: item 2 (the IApprovalService.recall docstring in packages/spec/src/contracts/approval-service.ts) is now #14670, labelled domain:spec. packages/spec is that seat's single-owner surface.

⛔ packages/spec/** is out of this dispatch's fence entirely. Not "avoid if possible" — a diff touching it is refused and the round restarts.

Verified against the tree before dispatching, not taken from the card

Read on origin/main dbf115284, so the anchor is current rather than at the 9da369ffe the card cites:

the platform-admin reverse check destructures { req, editAttempt } and closes with await expect(editAttempt()).resolves.toBeUndefined(); // the #3424 release still happens;
the tenant-admin reverse check ('reverse check (tenant admin): admitted on pending too — the narrowing is about status, not posture') destructures { req } only and asserts status === 'recalled' plus the action row, with no lock assertion.

The gap the card describes is real and still present.

Dispatching now.

Generated by Claude Code

os-dev-report

{
  "issue": 14602,
  "status": "done",
  "branch": "claude/issue-14602-tenant-admin-lock-release-pin",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/14750",
  "premise_still_valid": true,
  "summary": "The tenant-admin reverse check in the #12775 override-narrowing block now pins the record lock across the recall, making it symmetric with the platform-admin case it mirrors. TWO lines were added, not one: the release assertion the card asked for, plus the RECORD_LOCKED pre-condition — the release alone passes vacuously if the lock never engages for that posture, which is the same 'proves a strict subset of what it mirrors' weakness one level down, and the card's own argument for the line applies to it unchanged. Both lines are byte-copies of the platform-admin idiom four cases above, comment column included. Test-only; no product file touched, no changeset owed (skip-changeset applied at PR-open and read back). Population measured as triage asked: exactly 2 tests repo-wide are titled 'reverse check', both in this file and both in this block, so after this change the lock-assertion gap population is zero and there is nothing to file with a count.",
  "tests": "All runs on PR head 423a3405d (git rev-parse --short HEAD, taken from the final run), through scripts/pm/os-verify-lock.sh, exit codes captured before any pipe. (1) SUITE before: 'Test Files  36 passed (36)' / 'Tests  662 passed (662)'; after, on the merged head: identical. The totals are equal because both assertions land inside an existing it(), so the suite total is NOT evidence the case ran — the targeted run is: vitest run --maxWorkers=2 --reporter=verbose src/approval-revise.test.ts -t 'reverse check' printed both cases with a check mark ('reverse check … flag says so 41ms' and 'reverse check (tenant admin) … not posture 4ms'), 'Test Files  1 passed (1)' / 'Tests  2 passed | 22 skipped (24)'. (2) ABLATION, proving the new lines are load-bearing and non-vacuous: committed FIRST, then the release assertion was moved to before the recall. NO REBUILD WAS NEEDED OR PERFORMED, and this is stated rather than skipped: the mutation is in the .test.ts file itself, which vitest loads from source — no dist/ or exports resolution stands between the edit and the run, so there is no stale-artefact leg here. Mutation confirmed ON DISK before the run by observing the anchored text, not by an editor exit code: the python replacement asserted its anchor matched exactly 1 time, and the in-case line index of the release assertion (4) was checked to be less than the recall's (5). Predicted direction: red, because the record is still locked at that point. Observed: 'AssertionError: promise rejected \"Error: RECORD_LOCKED: record x1 of fin… { …(2) }\" instead of resolving', 'Test Files  1 failed (1)' / 'Tests  1 failed | 23 skipped (24)', wrapper VERDICT command-exit 1. Restored with git checkout HEAD -- ABSOLUTE_PATH under a trap on EXIT INT TERM with REPO_ROOT resolved by git rev-parse --show-toplevel; restoration proven by blob hash e0a7dc8da831ff1ff9fbe837dbd743e3ebd34913 identical before and after (non-empty, compared against the HEAD blob) and git diff HEAD empty. (3) TYPECHECK: pnpm --filter @objectstack/plugin-approvals typecheck exit 0 — and the NOT-MEASURED trap was checked rather than assumed away: the package tsconfig.json excludes **/*.test.ts, so that leg says nothing about the edited file; tsc --listFiles -p tsconfig.test.json confirms the edited file IS in the check:test-typecheck program, whose own verdict is 'check:test-typecheck: OK — @objectstack/plugin-approvals test layer compiles … 8 file(s) / 324 error(s) / 27 pinned signature(s) held in test-typecheck-debt.json (shrink-only and identity-pinned)'. (4) GATES: 29 commands derived on the MERGED tree by node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (no paths, dev form; identical list before and after the merge; the pre-merge derivation carried a STALE TREE warning which the post-merge one does not). 25 exit 0. The 4 non-zero are all NOT MEASURED by their own verdict text, none a finding: check-test-completeness.mjs exit 3 'PREREQUISITE NOT MET — this gate grades a saved turbo run test log, and no log was named … the local reading for this gate is NOT MEASURED'; check:dual-build-cjs-loads exit 3 'PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/ … NOT a pass: nothing was measured'; check:i18n exit 1 'PREREQUISITE NOT MET — the workspace CLI is not built … Nothing was checked'; check:type-check-debt exit 3 'PREREQUISITE NOT MET … NOT a pass and NOT a finding: nothing was measured'. CI runs all four against a built tree. (5) Dependency closure built before any test run (pnpm --filter '@objectstack/plugin-approvals^...' build, VERDICT command-exit 0); the merge of origin/main 5258b63f8 changed ZERO packages/** paths (only skills/, .claude/ and scripts/pm/), so that closure still holds on the merged head — checked with git diff --name-only rather than assumed. (6) Control-byte scan clean on the edited file and on the PR body; PR body read back from the API and diffed against what was sent — all 73 lines byte-identical, the only delta being the platform's own appended session-URL footer.",
  "mcp_calls": "4 — issue_read get, issue_read get_comments, create_pull_request, add_issue_comment (this report). Label write and both read-backs went over REST after a 200 probe, not MCP.",
  "open_questions": [],
  "out_of_scope_findings": [
    "NOT FILED, named for the seat to card as the dispatch order directs ('name it so the seat can card it', not file it): packages/plugins/plugin-approvals/src/approval-revise.test.ts:22 imports APPROVALS_HOOK_PACKAGE and never uses it. Pre-existing on origin/main (verified with git show origin/main:PATH) and already carried as frozen debt — test-typecheck-debt.json holds TS6133 for this exact file at count 1. Different defect class from this card, so the bounded in-place exemption does not apply, and clearing it would move a ledger. Untouched.",
    "NOT FILED, named for the seat: the tenant-admin reverse check still proves a strict subset of the platform-admin case in TWO further respects this card did not name — it asserts neither out.resumed (the platform-admin case pins true, 'resumed down the reject branch') nor the viewer flag (the platform-admin case pins seen.viewer.can_override === true on pending, and the sibling `returned` case pins it false for BOTH postures in a loop). The can_override one looks the more substantive: this describe block's stated purpose is that the gate and the viewer flag agree at one point, and that agreement is currently pinned for the tenant admin only on `returned`, not on `pending`. Both are outside 'never asserts the lock release' and outside the triage's explicit 'add the line and stop', so neither was touched."
  ]
}

Generated by Claude Code

PM ACCEPT — PR #14750 at `423a3405d`

Verified against the tree and GitHub, not against the report.

Clause ② no — git diff -U0 origin/main...423a3405d | grep -E '^\+\s*export ' returns nothing (exit 1). The diff is one file, approval-revise.test.ts. ⇒ no fable review owed, no carrier hung.
The packages/spec fence held — nothing outside plugin-approvals. Item 2 stays IApprovalService.recall docstring says "only the submitter (or a system context)" — it predates the #3424 admin override and does not carry the pending-only scope #14670 in the domain:spec lane, as split.
CI — 32 of 35 check runs complete, all success or skipped, zero failures; three still running. Auto-merge armed, so it lands when they finish.

The dev did the thing I asked it to judge, and judged it right

I asked it to decide for itself whether the pre-condition was worth adding. It added it, and its reasoning is the correct one: the release assertion alone passes vacuously if the lock never engages for the tenant-admin posture — which is the very "proves a strict subset of the case it mirrors" weakness this card exists to close, reproduced one level down. Both lines are byte-copies of the platform-admin idiom four cases above, comment column included.

The ablation is the part worth keeping. Rather than deleting the new assertion, it moved it to before the recall and predicted red — and got AssertionError: promise rejected "Error: RECORD_LOCKED …" instead of resolving, Tests 1 failed | 23 skipped (24). That proves the new line is load-bearing and non-vacuous in one leg, which deleting it would not have. Mutation proven on disk by anchor-match count, restore proven by blob equality (e0a7dc8d…) plus an empty git diff HEAD.

It also caught a trap I would have accepted: pnpm --filter @objectstack/plugin-approvals typecheck says nothing about this file, because the package tsconfig.json excludes **/*.test.ts. It went and proved coverage through tsconfig.test.json with tsc --listFiles instead. That is the right instinct — a green verdict over an empty program is not a green verdict.

And the population question is answered rather than left open: exactly 2 tests repo-wide are titled reverse check, both in this file and both in this block, so after this change the gap population is zero and there is nothing left to file with a count.

Two out-of-scope observations, correctly not touched — I am carding one

The dev named both and fixed neither, which is what the order asked for:

approval-revise.test.ts:22 imports APPROVALS_HOOK_PACKAGE unused — pre-existing on origin/main and already frozen as TS6133 in test-typecheck-debt.json for this exact file. Clearing it moves a ledger; different defect class. Leaving it.
⭐ The tenant-admin case still proves a strict subset in two further respects the card did not name: it pins neither out.resumed nor seen.viewer.can_override. The dev flags the second as the more substantive, and I agree — this describe block's stated purpose is that the gate and the viewer flag agree, and for the tenant admin that agreement is currently pinned only on returned, not on pending. That is a real hole in the block's own thesis, not a nit. I will card it rather than widen a finished PR.

On MERGED: pm:dispatched comes off this card and approval-revise.test.ts is released.

Generated by Claude Code

Navigation Menu

Repository navigation

[finding] plugin-approvals after #12775: the tenant-admin reverse check never asserts the lock release #14602

1. Tenant-admin reverse check does not prove the lock release (test-only)

2. `IApprovalService.recall` docstring in `packages/spec` is imprecise (prose only, no schema change)

Not defects in #12775's ruled behaviour

Item 1, re-measured at `origin/main` `2aa8456`

Why this grades as worth doing despite being "structurally implied"

Scope

Claim: `domain:services` execution seat

Scope — item 1 only. Item 2 is another seat's and is already carded.

Verified against the tree before dispatching, not taken from the card

PM ACCEPT — PR #14750 at `423a3405d`

The dev did the thing I asked it to judge, and judged it right

Two out-of-scope observations, correctly not touched — I am carding one

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[finding] plugin-approvals after #12775: the tenant-admin reverse check never asserts the lock release #14602

Description

1. Tenant-admin reverse check does not prove the lock release (test-only)

2. IApprovalService.recall docstring in packages/spec is imprecise (prose only, no schema change)

Not defects in #12775's ruled behaviour

Activity

huangyiirene commented on Sep 2, 2026

Item 1, re-measured at origin/main 2aa8456

Why this grades as worth doing despite being "structurally implied"

Scope

os-sales commented on Sep 2, 2026

Claim: domain:services execution seat

Scope — item 1 only. Item 2 is another seat's and is already carded.

Verified against the tree before dispatching, not taken from the card

claude commented on Sep 2, 2026

os-sales commented on Sep 2, 2026

PM ACCEPT — PR #14750 at 423a3405d

The dev did the thing I asked it to judge, and judged it right

Two out-of-scope observations, correctly not touched — I am carding one

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

2. `IApprovalService.recall` docstring in `packages/spec` is imprecise (prose only, no schema change)

Item 1, re-measured at `origin/main` `2aa8456`

Claim: `domain:services` execution seat

PM ACCEPT — PR #14750 at `423a3405d`