Skip to content

fix(lint): flag an approval slate that is entirely manager rungs - #17034

Merged
baozhoutao merged 4 commits into
mainfrom
claude/issue-16748-approvers-manager-rung
Sep 9, 2026
Merged

fix(lint): flag an approval slate that is entirely manager rungs#17034
baozhoutao merged 4 commits into
mainfrom
claude/issue-16748-approvers-manager-rung

Conversation

@claude

@claude claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Part of #16748

approval-approvers-may-resolve-empty reasoned about position / team / department and said nothing about { type: 'manager' }. That is the wrong way round: a position rung resolves empty because the position is unstaffed, which an operator can fix; a manager rung resolves empty because sys_user.manager_id is unset, which they cannot — the managed-update whitelist for sys_user is exactly {name, image, locale} (ADR-0092), the auth admin endpoints do not accept the column, and the Console renders no field for it. The rule warned about the rung an author can rescue and stayed silent on the one they cannot, and manager is the canonical first rung of a tiered ladder, so the silent case was also the common one.

The before leg — run first, and the card stands

Anchors re-taken on this branch's merge base d127f9bab (newer than the brief's 131851f5f2; both readings identical):

target   git grep -c "'manager'"      validate-approval-approvers.ts  ->  0
control  git grep -c "'position'"     validate-approval-approvers.ts  ->  4   (firing control)
nonsense git grep -c "'zzz_no_such'"  validate-approval-approvers.ts  ->  0

Behavioural before leg, on the unmodified tree:

TARGET   manager-only node          -> []                                        (silent)
CONTROL  position-only node         -> 1  approval-approvers-may-resolve-empty   (instrument fires)
NONSENSE user-only node             -> 0
TARGET   manager-only + seeded chain-> []

What this adds

A second arm under the same rule id and the same info tier, in the same file. No new rule id, so packages/lint/src/index.ts is untouched — the diff is 296 insertions, 0 deletions, across three files.

  • Fires when a node's approver slate consists entirely of { type: 'manager' } rungs. lookupManager reads sys_user.manager_id of record[value] ?? record.owner_id and returns null when the column is unset, so the approver contributes nobody; with nothing else on the node the request waits forever, and under the default lockRecord the record stays locked.
  • Claims nothing it did not read. The message says in as many words that this is a static check which cannot read the column and that it does not assert the slate IS empty — it reports that nothing else on the node can approve if it is.
  • Names the real remedy: SCIM provisioning, a seed / bulk import, or directory sync — and explicitly not editing the user in the Console, which cannot write it. It also offers the escape that does not depend on the write-surface question at all: a fallback approver such as { type: 'org_membership_level', value: 'owner' }.
  • Stays quiet when the stack's own seed data wires sys_user.manager_id on any seeded sys_user row. Seed rows are the only manager-chain evidence a stack can carry, so that is the whole of what this check reads on the question — and the code says so.

The three boundaries, honoured

  1. manager_id is not made writable. No identity, auth or platform-object file is touched. That question stays where it was left.
  2. The position arm is unchanged — 4 occurrences of 'position' before and after, its message text byte-identical, and the new arm scoped so the two are disjoint by construction: manager is not in GROUP_ROUTED_TYPES, so that arm's every is false exactly when this one's is true. No node can draw both findings. In particular the [position, manager] slate this package pins as silent stays silent.
  3. The tier is unchangedinfo, the same as its sibling. Measured below.

Clause-② declaration

Clause-②: yes

Declared yes, but on a narrower ground than "it changes a gate" — and the level follows the declaration: this is a purely additive widening of a published surface, so @objectstack/lint is graded minor, not patch.

The dispatch direction was a deliberately conservative yes. The crux it asked to be measured: does an info finding change any gate's verdict, or only the printed report? Measured, with a firing control and a nonsense control:

TARGET   manager-only slate  -> {errors:0, advisories:1}  info:approval-approvers-may-resolve-empty
CONTROL  reserved decision   -> {errors:1, advisories:0}  (an error-severity finding DOES reach `errors`)
NONSENSE user-only slate     -> {errors:0, advisories:0}
SIBLING  position-only slate -> {errors:0, advisories:1}  info:approval-approvers-may-resolve-empty

The control matters: it proves the instrument can put a finding in errors, so the target's errors: 0 is a reading and not an artefact. Both consumers agree, in their own source:

  • os lintpackages/cli/src/commands/lint.ts:520 maps info to suggestion; the exit code is read from failing = errors.length + (strict ? warnings.length : 0) at :763, which excludes suggestions even under --strict. The exit code cannot move.
  • Runtime publish gatesplitBySeverity and evaluateRuntimeWrite both classify severity !== 'error' as advisory; packages/metadata-protocol/src/runtime-authoring-gate.ts:727 returns { error: null } whenever result.errors.length === 0. The write proceeds; the finding rides the 2xx response.

Conclusion: no verdict moves, anywhere. Clause-②: yes still holds, but for the other reason — the finding lands in SaveMetaItemResponseSchema.advisories, which is a published wire contract, and in os lint --json's issues / suggestions. A consumer's output changes; its pass/fail does not. needs:contract-review is applied on that basis, and a reviewer should read it as "new advisory rows on existing surfaces", not "a new way to fail".

Verification

Run against final commit a173e673e2.

Tests — packages/lint, full package

Test Files  102 passed (102)
Tests       3607 passed (3607)

validate-approval-approvers.test.ts: 31 → 42. All 31 pre-existing tests are green and untouched — the diff adds 158 lines to that file and removes none, including the pin that [position, manager] emits nothing.

Typecheckpnpm --filter @objectstack/lint typecheck, exit 0. Coverage of the new test is measured, not assumed: tsc --listFiles -p tsconfig.test.json reaches validate-approval-approvers.test.ts (1) and validate-approval-approvers.ts (1), with a nonsense control at 0.

Ablation — both legs mutated on disk, proven on disk, and restored against HEAD. Each leg printed its target-text and injected-marker grep counts, compared git hash-object against the HEAD blob before and after, and restored under a trap with absolute paths.

leg mutation predicted observed
A the manager arm's every(...) predicate forced false red 6 of the 11 new tests fail; all 31 pre-existing stay green
B the seed suppressor's manager_id read forced false red exactly 1 test failsNEGATIVE: a populated manager chain in the stack emits nothing

Leg B is the one that matters for the mandatory controls: it proves that negative control is discriminating, so the implementation is demonstrably not an always-fires. Both legs restored to blob 504e748581d328ad2d5807591301bb7434cb95f0 with git diff HEAD empty.

Repo-wide lint — actually run, not narrowed. eslint . --no-inline-config --format json: 6394 files (the population read from eslint's own config), 0 errors, 0 warnings, exit 0. No narrowing argument is needed.

Gates run locally, all exit 0: check:nul-bytes · check:cross-package-test-inputs · check:test-source-alias · check:published-files · check:changeset-no-major · check:empty-changeset · check:changeset-gate-self-tests · check:objectui-changeset · check:adr-0087-registration · check:comment-mask-adoption · check:comment-mask-corpus · check:keyed-text-bounds · check:closing-keyword-parity · check:reference-carrier-shape · check:doc-authoring · check:type-check-coverage · check:slot-lookup · check:query-options-erasure · check:watch-hint-literal · check:undeclared-dep-imports · check:select-gate-families · check:pm-governed-prose · check:partof-closing-keyword · check:registry-log-declared.

check:doc-authoring caught a real defect on the first run and the fix is the second commit: the runtime hint prose carried bare tracker ids, which Rule 3 forbids because that string reaches authors, operators and generated surfaces, none of whom can resolve them. The ids moved to the adjacent comments.

NOT MEASURED (exit 3 — the gates' own "prerequisite not met", explicitly neither a pass nor a finding): check:type-check-debt --re-measure and check:published-readme-exports. Both require the whole packages/* build closure on disk, which CI builds ahead of them; neither is evidence in either direction here.

Changeset — measured, not assumed

@objectstack/lint publishes files: ["dist", "README.md", "CHANGELOG.md"] and no src/. Built, then grepped the tarball's contents:

positive control  "routes to a group (position/team/department)"    -> 4 dist files
TARGET            "sys_user.manager_id of the record's owner"       -> dist/index.js, dist/index.cjs,
                                                                       dist/runtime.js, dist/runtime.cjs
nonsense control  "zzz_no_such_string"                              -> 0

npm pack --dry-run --json: 17 entries, 0 under src/. Something published moves, so a changeset is required — and because a rule that begins covering a case it was silent on is a purely additive widening of a published surface, the level is minor, the floor that act carries. No skip-changeset.

验收备注

One observation, noted and not filed, because it sits behind a boundary this round's dispatch set explicitly and filing a card against that boundary is the maintainer's call, not mine.

The existing pin does NOT flag when a guaranteed-staffed or individual fallback is present asserts that [{ type: 'position' }, { type: 'manager' }] emits nothing, and its comment calls the manager rung an individual fallback. This card's whole argument is that a manager rung is not a guaranteed fallback — so that slate can still resolve entirely empty, and it stays silent. Honouring boundary 2 (the position arm and its tests are the control that nothing broke) means leaving it exactly as it is, which is what this PR does; the residual gap is real and deliberate. Successor: whoever reviews this PR, and the seat that owns the position arm.

🤖 Generated with Claude Code

https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU


Generated by Claude Code

`approval-approvers-may-resolve-empty` reasoned only about group-routed
rungs (position/team/department) and was silent on `{ type: 'manager' }`,
which has the same empty-slate failure shape and a worse cause: an
unstaffed position is an operator's to fix in-product, an unset
`sys_user.manager_id` is not — the managed-update whitelist is
{name, image, locale}, the auth admin endpoints refuse the column and the
Console renders no field for it.

Adds a second arm under the same rule id and the same `info` tier. It is
scoped to slates that are ENTIRELY manager rungs, which keeps it disjoint
from the existing arm by construction and leaves every `position` verdict
— mixed slates included — byte-identical. The message says plainly that
the check is static and does not assert the slate is empty; the hint
prescribes SCIM / import / directory sync rather than a Console edit that
is not possible. A stack whose own seeds wire `sys_user.manager_id`
silences it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
`check:doc-authoring` Rule 3: a runtime string reaches authors, operators
and generated surfaces, none of whom can resolve a bare id. The anchors
move to the adjacent comments, where the reader who can resolve them is
already reading the source. Also adds the changeset — the new advisory
prose reaches four published bundles (dist/index.{js,cjs},
dist/runtime.{js,cjs}), all inside the package's `files`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/approvals.mdx (via org_membership_level (literal, a string literal in validateApprovalApprovers))
  • content/docs/plugins/packages.mdx (via org_membership_level (literal, a string literal in validateApprovalApprovers))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v14.mdx (via org_membership_level (literal, a string literal in validateApprovalApprovers))
  • content/docs/releases/v16.mdx (via org_membership_level (literal, a string literal in validateApprovalApprovers))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: sys_user (literal, 31 pages)
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 5 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 c0e4bc749d162a7cae17f54f94e6bab7c9c46729packageMentionDocs.

Which tree this was computed on

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

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

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

A rule that begins covering a case it was silent on is a purely additive
widening of a published package's public surface, and the standing
maintainer ruling puts a floor of `minor` on that act regardless of the
commit type. The clause-② declaration this PR carries is correct and
stays; it was the level under it that was wrong.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU

os-bill commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Contract review (CONTRACT_REVIEW_TIER, director seat audit) — PR #17034 @ 05e4ac1410

Verdict: PASS on the contract question. This is an audit reading by the director seat (summon #20); the domain:devx seat adopts it verbatim or discards it. ⛔ This seat takes no release action on this carrier (no ready flip, no auto-merge, no enqueue).

  • Tier: get_session at 2026-09-09T03:27Z → external_metadata.last_served_model = claude-fable-5-1 = CONTRACT_REVIEW_TIER.
  • Implemented-by: branch claude/issue-16748-approvers-manager-rung (mode:subagent dev at opus, per claim 5595021276, session session_012GKcPZbMoGq7WPzKLfRBTU).
  • Reviewed-by: session_01Tep4AYXZvyBA7jsvne5KZV (GitHub os-bill). Different session from the implementer ⇒ independent, not SELF-REVIEW.

① Derived judgments — what the diff does to the published surface

  1. @objectstack/lint's published rule approval-approvers-may-resolve-empty gains a second arm: it fires at info when every routable approver on a node canonicalises to manager and the stack's own seed rows (stack.data[], SeedSchema: object === 'sys_user', records[].manager_id non-blank) show no populated manager chain. A consumer's lint report gains a finding on input it was silent on; no refusal is added (info is advisory on every consumer, os lint exit code unchanged). Clause-②: yes is the correct conservative reading — public surface widens by one finding, no accept set moves.
  2. No new export, no new rule id (the id is reused), no schema file touched. GROUP_ROUTED_TYPES is unchanged, so the position/team/department arm is behaviourally byte-identical. The two arms are disjoint by construction: managerGROUP_ROUTED_TYPES, so the two every predicates cannot both hold on one node — the [position, manager] silence pin therefore holds for the right reason, not by test luck.
  3. The suppressor's shape matches the contract it reads: packages/spec/src/stack.zod.ts:718 declares data: z.array(SeedSchema), and seed.zod.ts carries object + records: z.array(z.record(...)). Blank strings, non-string/non-number values, other objects and malformed data entries do not suppress — each has a firing control in the tests.
  4. Rule 3 of check:doc-authoring (no tracker ids in runtime strings): the message and hint carry none; the ids stay in comments. ✓
  5. Acceptance item 2 (a lint rule must not claim a runtime fact it did not read): the message states it is a static check and explicitly does not assert the slate is empty. ✓ Item 4 (dependency on { type: 'manager' } resolves a column no product surface can write: sys_user.manager_id is refused by the data API and absent from the auth admin endpoints #16678): recorded on MANAGER_ONLY_REMEDY. ✓

② semver

.changeset/approval-approvers-manager-rung-may-resolve-empty.md grades @objectstack/lint: minor and names the act (additive widening of a published package's surface). Consistent with the yes declaration and with the level axis. ✓

③ Boundary flags

None raised on the thread beyond the report. Non-blocking notes for the lane seat, not owed before landing: (a) the negative pin not.toMatch(/[Ee]dit .{0,40}in the Console\b(?!.*NOT)/) only matches edit followed by a space, so it would not catch editing … in the Console; the positive toContain('NOT by editing the user in the Console') pin carries the intent. (b) The PR body reads Part of #16748 while all four acceptance items on the card read as delivered by this diff — the lane seat decides at ACCEPT whether the card closes on merge or names the residual.

Landing pre-check readings taken by this seat (for the lane seat's use)

  • node scripts/pm/check-clause2-carriers.mjs --pair 17034 → exit 0 at 03:35Z (declaration readable, both carriers agree).
  • Check runs on 05e4ac1410 at 03:47Z, latest-per-name: 19 success · 3 skipped · 8 in progress · 0 failed. Landing waits for all-green; that wait and the release action are the lane seat's.

Generated by Claude Code

…ed suppressor applies

The hint listed SCIM, bulk import and directory sync as equally available
routes for populating `sys_user.manager_id`. Measured against this tree,
two of the three have no writer here: `admin-import-users.ts` matches
`manager_id` 0 times against a control of `phone_number` 8 and its update
set is {name, image, locale} + phone_number + role, and the SCIM
Enterprise `manager` attribute is declared without any non-test file
projecting it onto the column. A seed — or any other system-context write
— does work, because both write guards gate on `isUserContextWrite`
(`userId && !isSystem`).

An exact diagnosis whose prescription cannot be carried out is worse than
no prescription, so the routes are now graded rather than listed. SCIM and
directory sync stay named: a deployment running a real one may populate
the column through it, and the defect was presenting them as something
this platform provides.

Also records the surface asymmetry of the seed suppressor. The runtime
publish gate's context carries objects, permissions, books, datasets and
pages, and no `data`, so the suppression is CLI-side only and a Studio
publish draws the advisory however the tenant's users are wired. Prose
only — the arm, the tier and the position arm are untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
@baozhoutao
baozhoutao marked this pull request as ready for review September 9, 2026 04:44
@baozhoutao
baozhoutao enabled auto-merge September 9, 2026 04:44
@baozhoutao
baozhoutao added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit 86f4246 Sep 9, 2026
35 checks passed
@baozhoutao
baozhoutao deleted the claude/issue-16748-approvers-manager-rung branch September 9, 2026 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants