fix(lint): flag an approval slate that is entirely manager rungs - #17034
Conversation
`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
📓 Docs Drift CheckThis PR changes 1 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # 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
|
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
Contract review (
|
…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
Part of #16748
approval-approvers-may-resolve-emptyreasoned aboutposition/team/departmentand said nothing about{ type: 'manager' }. That is the wrong way round: apositionrung resolves empty because the position is unstaffed, which an operator can fix; amanagerrung resolves empty becausesys_user.manager_idis unset, which they cannot — the managed-update whitelist forsys_useris 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, andmanageris 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's131851f5f2; both readings identical):Behavioural before leg, on the unmodified tree:
What this adds
A second arm under the same rule id and the same
infotier, in the same file. No new rule id, sopackages/lint/src/index.tsis untouched — the diff is 296 insertions, 0 deletions, across three files.{ type: 'manager' }rungs.lookupManagerreadssys_user.manager_idofrecord[value] ?? record.owner_idand 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 defaultlockRecordthe record stays locked.{ type: 'org_membership_level', value: 'owner' }.sys_user.manager_idon any seededsys_userrow. 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
manager_idis not made writable. No identity, auth or platform-object file is touched. That question stays where it was left.positionarm 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:manageris not inGROUP_ROUTED_TYPES, so that arm'severyis 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.info, 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/lintis gradedminor, notpatch.The dispatch direction was a deliberately conservative
yes. The crux it asked to be measured: does aninfofinding change any gate's verdict, or only the printed report? Measured, with a firing control and a nonsense control:The control matters: it proves the instrument can put a finding in
errors, so the target'serrors: 0is a reading and not an artefact. Both consumers agree, in their own source:os lint—packages/cli/src/commands/lint.ts:520mapsinfotosuggestion; the exit code is read fromfailing = errors.length + (strict ? warnings.length : 0)at:763, which excludes suggestions even under--strict. The exit code cannot move.splitBySeverityandevaluateRuntimeWriteboth classifyseverity !== 'error'as advisory;packages/metadata-protocol/src/runtime-authoring-gate.ts:727returns{ error: null }wheneverresult.errors.length === 0. The write proceeds; the finding rides the 2xx response.Conclusion: no verdict moves, anywhere.
Clause-②: yesstill holds, but for the other reason — the finding lands inSaveMetaItemResponseSchema.advisories, which is a published wire contract, and inos lint --json'sissues/suggestions. A consumer's output changes; its pass/fail does not.needs:contract-reviewis 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 packagevalidate-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.Typecheck —
pnpm --filter @objectstack/lint typecheck, exit 0. Coverage of the new test is measured, not assumed:tsc --listFiles -p tsconfig.test.jsonreachesvalidate-approval-approvers.test.ts(1) andvalidate-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, comparedgit hash-objectagainst theHEADblob before and after, and restored under atrapwith absolute paths.every(...)predicate forced falsemanager_idread forced falseNEGATIVE: a populated manager chain in the stack emits nothingLeg 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
504e748581d328ad2d5807591301bb7434cb95f0withgit diff HEADempty.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-authoringcaught a real defect on the first run and the fix is the second commit: the runtimehintprose 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-measureandcheck:published-readme-exports. Both require the wholepackages/*build closure on disk, which CI builds ahead of them; neither is evidence in either direction here.Changeset — measured, not assumed
@objectstack/lintpublishesfiles: ["dist", "README.md", "CHANGELOG.md"]and nosrc/. Built, then grepped the tarball's contents:npm pack --dry-run --json: 17 entries, 0 undersrc/. 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 isminor, the floor that act carries. Noskip-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 presentasserts 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 (thepositionarm 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 thepositionarm.🤖 Generated with Claude Code
https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
Generated by Claude Code