Skip to content

docs(spec): IApprovalService.recall names every actor who may recall, and scopes each one by status (#14670) - #15645

Merged
os-sales merged 1 commit into
mainfrom
claude/issue-14670-recall-docstring-override-and-scope
Sep 5, 2026
Merged

docs(spec): IApprovalService.recall names every actor who may recall, and scopes each one by status (#14670)#15645
os-sales merged 1 commit into
mainfrom
claude/issue-14670-recall-docstring-override-and-scope

Conversation

@claude

@claude claude Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #14670

Prose only — no key, no accepted value, no runtime behaviour moves. packages/spec and a changeset; nothing in plugin-approvals is touched.

Clause-②: no. Judged from content, not from the path. The path limb fires (SUSPECT_TIER_GLOBS declares packages/spec/src/**), but that declaration's own docblock calls the path reading "a HINT, never a verdict". This corrects a JSDoc block: nothing about the accept/reject set moves and no public surface widens. Lane precedent: #14586 claimed no for a describe-string correction.

Governed surface: no. node scripts/pm/check-governed-merges.mjs --test over the final file list, exit 0 — "0 of 2 path(s) hit the register".

The defect: one file disagreeing with itself

The card frames this as a docstring that "predates" the #3424 admin override. Measured, it is sharper — packages/spec/src/contracts/approval-service.ts already documented the override 387 lines above the sentence that denied it, and it names the very verb:

line (on origin/main d30ccb9bd) text
:325 on a *pending* request (approve / reject / reassign / recall it) despite
:338 can_override: boolean;
:497-:513 the admin-override audit path (via_override), naming #3424
:712 "Withdraw a pending request. Only the submitter (or a system context) may recall."

So :325 says an override actor may recall, and :712 says only the submitter may. That is the argument for fixing this rather than filing it as a nicety.

The second paragraph made it worse, not better. Structured as a general rule plus an ADR-0044 widening, it invites the reader to apply the widening to whoever the general rule admits — so a reader who takes both paragraphs together concludes an admin can recall a returned request. They cannot.

What the source actually says (verified, not inherited)

  1. The override arm. plugin-approvals/src/approval-service.ts:961, private isOverrideActor(context, requestOrg?). Its own docblock (:944-:952) enumerates the levers: "approve, reject, reassign it to a real approver, or recall it", and :956 adds "A system context always passes".

  2. The pending-only scope. recall()'s own gate, :2987:

    const overrideAdmits = raw.status === 'pending'
      && this.isOverrideActor(context, raw.organization_id ?? null);

    and the viewer flag it is spelled to match, attachViewers at :5283:

    can_override: row.status === 'pending'
      && this.isOverrideActor(context, (row as any).organization_id ?? null),

    Because isOverrideActor returns true on context.isSystem (:963), the system arm enters through the same gate and carries the same pending scope. The card says this ("the override and system arms"); it is worth restating because a rewrite that scopes only the override arm leaves "or a system context" hanging in the general rule and reproduces the defect on the other arm.

The published prose was already correct and is the tie-breaker: content/docs/automation/approvals.mdx:550-:559 — an admin "may act on any pending request — approve, reject, reassign it to a real approver, or recall it". This docstring was the one surface that had not kept up.

The fix

Status is now the axis, rather than a general rule with corrections appended. Each status carries its own admitted set, so the inheritance misreading is not available on a re-read:

A second stale docstring, in scope and named here

ApprovalRecallInput.actorId (:545) read /** Must be the request's submitter (or a system context). */ — the same false claim, on the same operation, 167 lines above the one the card cites. It is corrected in this PR under the bounded-in-place rule, all four conditions measured:

  1. Same defect class — verbatim the same omission (no override actor, no status scope), same verb.
  2. Mechanical, form pinned by existing evidence — the citations above, plus [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 maintainer ruling of 2026-09-02.
  3. No other claim holds the file — all 29 open PRs' file lists were scanned; zero touch any path matching approval. (The dispatch note's observation about feat(spec)!: duration-shaped number keys carry their unit in the key name — no-baseline gate + seven ADR-0087 renames (timeoutMs, ttlSeconds/ttlMs, *TimeoutSeconds) #15626 holding contracts/job-service.ts is consistent: same directory, different file.)
  4. Same gate family — same file, same package, no new verification surface.

Fixing only :712 would have left the contradiction alive on the very input type the corrected method takes.

Deliberately NOT changed

ApprovalSendBackInput.actorId (:572) and ApprovalResubmitInput.actorId (:601) carry the same "(or a system context)" phrasing and are correct. isOverrideActor has exactly five call sites in the service — decideNode (:2457), reassign (:3553), recall (:2988), attachViewers (:5284) and visibleRequestIds (:4865) — and neither sendBack nor resubmit is among them, so no override actor reaches either.

Verification

Everything below ran on the final commit, f50678c0e (git rev-parse --short HEAD), with a clean working tree, so the green is a reading of the tree being merged.

  • pnpm --filter @objectstack/spec exec vitest run --maxWorkers=2472 files / 12,686 tests passed; lock wrapper VERDICT command-exit 0.
  • pnpm --filter @objectstack/spec typecheckVERDICT command-exit 0 (tsc --noEmit, scripts project, and the test layer).
  • pnpm --filter '@objectstack/spec...' buildVERDICT command-exit 0; 34/34 declaration files emitted.
  • pnpm lint (whole repo, eslint . --no-inline-config) — exit 0. Run in full rather than narrowed.
  • Derived gate union — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (the script derived its own change set; 65 runnable families). All run families exit 0, each verified to have echoed its script name so a zero-match --filter could not pass silently. Includes check:api-surface, check:authorable-surface, check:docs, check:llms-txt, check:skill-refs, check:export-origins, check:declaration-map, check:liveness, check:variant-docs, check:strictness-ledger, check:doc-authoring, check:system-context-census, check:cross-package-test-inputs, check:published-files, and the changeset family (check-empty-changeset, check-changeset-no-major, check-adr-0087-registration, all with their self-tests).
  • node scripts/check-nul-bytes.mjs — OK, 7,572 files scanned, no raw control bytes.

The docstring ships. It is in the published declarations, not only in source — packages/spec/dist/contracts/index.d.ts:8307-:8325 carries the new text, which is why this PR takes a patch changeset rather than the skip-changeset label.

Two results that are NOT MEASURED, and are not being reported as green. check:doc-formula-expressions exits 3 — its own PREREQUISITE-NOT-MET code, because @objectstack/formula is not built in this worktree ("Nothing was measured", in the gate's own words). check:skill-examples exits 1 for the same reason one layer over (packages/client-react/dist holds no declarations); it is not in this card's derived gate list, and this diff adds no marked example block. CI builds the whole tree, so both are answered there.

One note for whoever runs these gates next

check:skill-examples left 233 extracted .ts files under packages/spec/.examples-build*/ when it aborted on that prerequisite, and scripts/docs-audit/affected-docs.mjs walks the filesystem without skipping those dirs — so the next gate in the sweep, check-affected-docs, failed its own self-test with a message pointing nowhere near the cause. Removing the residue turns it green again (568 self-test cases pass); reverting this PR's source change does not, which is how it was ruled out as mine. That interaction is already filed as #15446 and was independently reproduced here, so nothing new was filed for it.

Out of scope, filed


🤖 Generated with Claude Code

https://claude.ai/code/session_01G4138K1EG7kQ81FNba5Kp4


Generated by Claude Code

…l, scoped by status

The docstring said "Only the submitter (or a system context) may recall", then
widened to `returned` requests in a second paragraph. Both halves were wrong:

- The list was not exhaustive. A #3424 override actor (platform or tenant
  admin, holding no approver slot) may recall a `pending` request. The same
  file already documented that 387 lines above, where the docblock on
  `ApprovalRequestRow.viewer.can_override` spells the override's levers as
  `(approve / reject / reassign / recall it)`.
- The ADR-0044 widening read as if it applied to that whole list. It does not:
  the override and system arms are ANDed with `status === 'pending'` where they
  are computed, so neither reaches a `returned` request (#12775, maintainer
  ruling 2026-09-02). Abandoning a revision window is the submitter's alone.

Status is now the axis rather than a caveat appended to a general rule, so each
status carries its own admitted set. `ApprovalRecallInput.actorId` carried the
same stale sentence and is corrected with it. `ApprovalSendBackInput.actorId`
and `ApprovalResubmitInput.actorId` share the phrasing but are correct, since
`isOverrideActor` is never called from `sendBack` or `resubmit`.

Prose only: no key, no accepted value, no runtime behaviour moves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4138K1EG7kQ81FNba5Kp4
@github-actions github-actions Bot added the size/s label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/spec, touching 2 documentable anchor(s).

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

  • content/docs/ai/actions-as-tools.mdx (via IApprovalService (symbol, a top-level interface))
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 — 129 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 e944fdb246d6baa41133bb78c5a07af1e5279eedpackageMentionDocs.

Which tree this was computed on

This run read content/docs from a00d4b251bb599bb536a4fb2ec0c80990b57599b — the merge of head f50678c0e848fc874957381fd3fcf70f428ce102 into base e944fdb246d6baa41133bb78c5a07af1e5279eed, 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 a00d4b251bb599bb536a4fb2ec0c80990b57599b && git checkout a00d4b251bb599bb536a4fb2ec0c80990b57599b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e944fdb246d6baa41133bb78c5a07af1e5279eed f50678c0e848fc874957381fd3fcf70f428ce102 && git checkout -B drift-repro e944fdb246d6baa41133bb78c5a07af1e5279eed && git merge --no-ff f50678c0e848fc874957381fd3fcf70f428ce102

node scripts/docs-audit/affected-docs.mjs --json e944fdb246d6baa41133bb78c5a07af1e5279eed

⚠️ 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 e944fdb246d6baa41133bb78c5a07af1e5279eed → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentation Improvements or additions to documentation size/s tooling

Projects

None yet

2 participants