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

[os-sales]

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.

[huangyiirene]

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

[os-sales]

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