Skip to content

docs(spec): APPROVER_ORG_SCOPED's team justification stops citing the removed engine gap - #11337

Merged
os-sam merged 1 commit into
mainfrom
claude/issue-10548-approver-team-docblock
Aug 23, 2026
Merged

docs(spec): APPROVER_ORG_SCOPED's team justification stops citing the removed engine gap#11337
os-sam merged 1 commit into
mainfrom
claude/issue-10548-approver-team-docblock

Conversation

@claude

@claudeclaudeBot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Fixes#10548

One-sentence justification rewrite in the APPROVER_ORG_SCOPED docblock. No flag value changes, and none should: the table answers ADR-0105 D9 retargetability, and that answer for team is unchanged. Only the justification had drifted, by resting on an engine behaviour that has since landed.

The drift

The docblock over APPROVER_ORG_SCOPED carried two clauses about team:

team is org-agnostic too — sys_team_member carries no organization column and the engine never scoped it (unlike position / membership-tier / department, which all do).

  • "sys_team_member carries no organization column" — still true, and still why a team's members are not individually placed.
  • "the engine never scoped it" — no longer describes the engine.

Premise re-measured on today's main, not recalled

Both halves of the card's premise were verified against the tree this branch cuts from (b9e9227e3), because the rewrite has to describe the engine as it is:

1. The stale sentence still reads as quotedpackages/spec/src/automation/approval.zod.ts, lines 258–260 before this diff.

2. expandTeamUsers really does screen on sys_team.organization_id — in packages/plugins/plugin-approvals/src/approval-service.ts:

  • expandTeamUsers (line 1294) opens with if (await this.teamIsProvablyOutsideOrg(teamId, organizationId)) return []; (line 1296) — the screen runs before the member fan-out, which is what keeps it at one row.
  • teamIsProvablyOutsideOrg (line 1337) reads sys_team for ['id', 'organization_id'] (lines 1350–1355) and drops the team when it names an organization other than the request's (lines 1358–1369). Absent tenancy facts fail open, deliberately.
  • Both call sites pass an organization: line 1053 (the static team branch) and line 1228 (expression with resolveAs: 'team').

A note on the wording chosen: the new sentence says the expansion is screened to the request's organization, which is precise for this table row. The two call sites pass different arguments — the static team branch passes organizationId and the expression branch passes directoryOrg — but on the team branch they are provably equal, since team is not org-scoped and an organization declaration on it is refused upstream. The comment at lines 1048–1052 says exactly this. The expression call site is a different approver type, one that is org-scoped.

The rewrite

 * ignoring it. `team` is org-agnostic for RETARGETING too — it consults no
* org-scoped directory, so an `organization` on it has nothing to redirect and
* is refused the same way (unlike position / membership-tier / department,
* which all do); its expansion is nonetheless screened to the REQUEST's
* organization on the team's own `sys_team.organization_id` (#10230), so this
* flag is about targeting, not tenancy.

The true contrast with position / membership-tier / department is kept; the false clause is replaced with the targeting-vs-tenancy distinction the card drafts.

No prose pin test exists

Searched the tree for the quoted phrases (never scoped it, org-agnostic too, carries no organization column) and for APPROVER_ORG_SCOPED itself. No test pins this prose — packages/spec/src/automation/approval.test.ts does not reference the constant at all. The one other hit for "never scoped it" is packages/verify/src/rls.ts, an unrelated sentence about RLS. Nothing to update in the same PR.

The generated surfaces (packages/spec/api-surface/automation.json, packages/spec/export-origins/automation.json) carry only the export name, not docblock prose, so a comment-only diff leaves them byte-identical.

Why a changeset rather than skip-changeset

Docblock-only spec PRs in this repo split both ways, so the precedent was checked rather than assumed. The deciding rule is the one PR #10669's changeset (.changeset/skill-tools-docblock-adr-0109.md, commit 5f2e54cc) states outright: it is "graded rather than skipped because the text ships to consumers: @objectstack/spec's files list publishes src/**/*.zod.ts, so this docblock travels in the npm tarball as source."

That rule applies here, and this case is strictly stronger on a second surface that PR explicitly measured itself as lacking:

  • packages/spec/package.json's files does contain src/**/*.zod.ts — verified, so this file ships as published source.
  • Unlike a property-level comment inside a z.object({ … }) literal (dropped from declaration emit — the reason docs(spec): align the skill.tools docblock with ADR-0109's authoring model #10669 noted its own text did not reach .d.ts), a docblock over a top-level export constsurvives. Measured in the built output: the stale sentence is present in packages/spec/dist/automation/index.d.ts (line 2348) and dist/automation/index.d.mts. That is the text a consumer's editor surfaces on hover.

So this PR carries .changeset/approver-org-scoped-team-justification.md (@objectstack/spec: patch) and does not take the skip-changeset label.

Verification

Gate families derived from the actual diff with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (the script reads its own change set from the merge base, covering the working tree and untracked files). All 22 matched families ran green, plus check:nul-bytes:

check:changeset-gate-self-tests · check:cross-package-test-inputs · check:doc-formula-expressions · check:empty-state · check:liveness · check:merge-driver · check:objectui-changeset · check:published-files · check:slot-lookup · check:spec-parsed-alias · check:strictness-ledger · check:test-source-alias · check:type-source-resolution · check:variant-docs · check-adr-0087-registration · check-changeset-no-major · check-ci-filter-parity · check-cross-package-test-inputs · check-dev-prereqs · check-empty-changeset · check-plugin-teardown-shape · check-affected-docs · check-nul-bytes

The union above was re-run after the final commit and is quoted from that run, at cae46c0c9 with a clean working tree — 23/23 green.

Two of those (check:doc-formula-expressions, check-dev-prereqs) were red on an earlier run for one environmental reason — the fresh worktree had only the spec dependency closure built, and both diagnose an unbuilt workspace (check-dev-prereqs says so in as many words: "The workspace is not built — 1 unmet precondition, not a list of problems"). A full pnpm build --concurrency=2 (71/71 tasks successful) cleared both, and both are green in the final run; neither red was attributable to this diff.

Also run:

  • pnpm --filter @objectstack/spec typecheck — green, including check:scripts-typecheck and check:test-typecheck ("OK — @objectstack/spec's test layer compiles under packages/spec/tsconfig.test.json").
  • pnpm --filter @objectstack/spec test418 test files / 11128 tests passed, 0 failed. (Invoked as test -- src/automation; the passthrough did not narrow vitest's selection, so this is the package's whole suite rather than the automation subset — broader than intended, and reported as what actually ran.)

The clause-② path limb fires mechanically on packages/spec/src/**, as the dispatch anticipated. The content limb is no: this diff is docblock prose only — no schema shape, no .describe() text, no accept/reject behaviour, and no public surface widening.


Generated by Claude Code

… removed engine gap (#10548)
The docblock justified `team: false` with "the engine never scoped it", which
the #10230 team organization screen made false. `expandTeamUsers` now opens
with `teamIsProvablyOutsideOrg`, which reads `sys_team.organization_id` and
drops the team when it names an organization other than the request's, at both
call sites.
The flag value is unchanged and deliberately so: the table answers ADR-0105 D9
retargetability, and `team` still consults no org-scoped directory. Only the
justification drifted. The replacement says the flag is about targeting, not
tenancy.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RadETjNRLALFLhFA3xehZP
@github-actions

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 1 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/spec/src/automation/approval.zod.ts) — pages documenting those are invisible to this run

Coarse fallback — 126 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 9ca161830d308e8ef2af2e0762f8d927413fd73fpackageMentionDocs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tooling labels Aug 23, 2026
@os-sam
os-sam marked this pull request as ready for review August 23, 2026 12:27
@os-sam
os-sam added this pull request to the merge queueAug 23, 2026
Merged via the queue into main with commit 8bdd955Aug 23, 2026
39 checks passed
@os-sam
os-sam deleted the claude/issue-10548-approver-team-docblock branch August 23, 2026 12:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/stooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] spec: APPROVER_ORG_SCOPED's docblock justifies team: false with "the engine never scoped it" — no longer true after the team org screen

2 participants

@os-sam@claude