Skip to content

fix(formula): declare SCOPE_ROOTS as dyn in the strict env so a bare reference behind a root name is judged (#16412) - #17133

Draft
claude[bot] wants to merge 4 commits into
mainfrom
claude/issue-16412-strict-env-scope-roots-dyn
Draft

fix(formula): declare SCOPE_ROOTS as dyn in the strict env so a bare reference behind a root name is judged (#16412)#17133
claude[bot] wants to merge 4 commits into
mainfrom
claude/issue-16412-strict-env-scope-roots-dyn

Conversation

@claude

@claude claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #16412

Clause-②: yes

Option C of the card's ruling (5583983008), measure-first: the strict declaredness environment declares SCOPE_ROOTS as dyn instead of map, so the two environments this package builds agree about those names and the widest first-error masking class cannot arise.

What was wrong

firstUndeclaredReference asks cel-js's checker for the first undeclared identifier. That checker returns exactly ONE error and the helper acts only on Unknown variable: X, so when the first error is of another class every undeclared reference behind it in the same source goes unjudged and the answer is null — the same value that means "every reference is rooted". Four published call sites read that answer and none can tell the two readings apart.

The reachable way in was a disagreement inside this package: buildScopedEnv declared every SCOPE_ROOTS member map, while the permissive env celEngine.compile type-checks in leaves them dyn. map has no equality, ordering or arithmetic overload, so an ordinary comparison on a root — or on an object field or flow variable sharing a root's name — compiled clean and then faulted no such overload in the strict env only, took the single error slot, and silenced everything behind it.

An author reaches it by naming a field or a flow variable after a namespace root and reading it bare. On a metadata-editing form that is not a coincidence: ADR-0089 D3 binds the row under edit as data, which is a SCOPE_ROOTS member.

The measurement

Twelve rows, run through the real consumers on a built tree, before and after. 11 of 12 flipped; 17 of 17 negative controls stayed clean; the card's five-row regression table is unchanged.

# surface source before after
1 visibility-bare-identifier data == 'x' && status == 'active' 0 findings 1
2 visibility-bare-identifier record == 'x' && status == 'active' 0 1
3 visibility-bare-identifier config != null && status == 'active' 0 1
4 visibility-bare-identifier type == 'grid' && status == 'active' 0 0 — unchanged
5 flow shadowing config == 'x' && status == 'y' [] ['status']
6 flow shadowing data == 'x' && status == 'y' [] ['status']
7 flow shadowing result > 1 && status == 'y' [] ['status']
8 validate.ts record scope data == 'x' && status == 'qualified' ok=true, 0 errors ok=false, 1 error
9 validate.ts record scope config != null && status == 'qualified' ok=true ok=false
10 validate.ts record scope record == 'x' && status == 'qualified' ok=true ok=false
11 validate.ts flattened config == 'y' && amont == 'x' 0 warnings 1 did-you-mean
12 validate.ts flattened data == 'y' && amont == 'x' 0 warnings 1 did-you-mean

Why row 4 did not flip — measured here, not assumed

The dispatch predicted this row is out of reach because type is not a SCOPE_ROOTS member. That is true (asserted from the published list, not copied: expect(SCOPE_ROOTS).not.toContain('type')), but the load-bearing measurement is stronger and was taken directly off the checker. Building the strict env twice — once with roots map, once with roots dyn — and reading the raw first error:

roots as map:  "type == 'grid' && status == 'active'"  ->  no such overload: type == string
roots as dyn:  "type == 'grid' && status == 'active'"  ->  no such overload: type == string
roots as map:  "data == 'x' && status == 'active'"     ->  no such overload: map(dyn, dyn) == string
roots as dyn:  "data == 'x' && status == 'active'"     ->  Unknown variable: status

(the real cel-js message spells map's two type parameters in angle brackets; they are
written with round brackets here so the platform's body sanitiser cannot eat the line)

The offending operand's type in row 4 is type, CEL's own type-value type, not map. The message is byte-identical under both declarations, so the row is out of reach because CEL declares that identifier itself — no declaration this package makes touches it. The same holds for string, int and the rest of the CEL type names. ⇒ 11/12 is the ceiling of option C by construction, and this row is now pinned rather than left to be rediscovered.

Negative controls — the "cannot false-positive" property

17 sources in which every reference is rooted, or which are the legitimate CEL the narrowing exists to protect (type(record.x) == string, comprehension macros including one whose macro variable shadows a field name, both guard idioms, optional chaining, stdlib calls, two roots in one source, a root used as a namespace). Each asserted across four surfaces at once. 17/17 clean before, 17/17 clean after, and zero fields changed on any of them — the comparison is field-by-field on the whole probe record, not just "still zero findings".

⚠️ Corrected — contract review F1. The sentence that stood here was false. It read: "inferCelType shares buildScopedEnv, so its answers were pinned too: 8 expressions, all 8 unchanged." inferCelType does share the env, and that is exactly why the claim does not follow. The control was blind by construction: none of its eight expressions used a namespace root as a direct operand, which is the only class the changed declaration governs, so it could not have failed on the affected class no matter what the change did. Re-measured properly in the next section.

The published answer that DOES move — inferExpressionType (F1)

The change is one declaration: the CEL type of every SCOPE_ROOTS name inside buildScopedEnv. A declaration is observable exactly where the name's type is consulted, i.e. wherever a root sits as a direct operand — so the probe set was rebuilt from what the environment governs rather than from what looked representative: the cross-product of every syntactic position that consults a root's declared type and both directions the map -> dyn move can push an answer, over all 27 published roots, plus controls that structurally cannot observe it. 96 probes, read through the built dist, on both legs of the same ablation.

75 of 96 move, and every one of them moves unknown -> a concrete type: 37 to boolean, 34 to number, 4 to text. Nothing narrows to unknown, and no concrete answer becomes a different concrete answer. It is uniform across the published list — 54 of 54 root probes (27 roots x two operand shapes) move.

probe pre-fix at head
result + 1 unknown number
record ? 1 : 2 unknown number
record - 1, record * 2, record / 2, record % 2 unknown number
record + "x", record ? "a" : "b" unknown text
data == "x" ? "a" : "b", record > 1 ? "a" : "b" unknown text
record == "x", record != "x" unknown boolean
record < 1, record > 1, record <= 1, record >= 1 unknown boolean
record && true, `record false, !record`

Controls that cannot observe the declaration, and do not move: record.amount > 100 (boolean), record.amount + 1 (number), daysBetween(record.a, record.b) + 1 (number), undeclared_field + 1 (unknown), a bare record (unknown). Controls for the OTHER direction — the overloads map did carry, which a widening-only reading would never have checked — also do not move: size(record) (number), "a" in record (boolean), has(record.a) (boolean), record.all(k, k == "a") (boolean), and record == previous (boolean, because map == map already had an overload).

⇒ This is a widening on a published surface, and the round did not declare it. inferExpressionType is re-exported from packages/formula/src/index.ts and consumed by packages/mcp/src/mcp-http-tools.ts as validate_expression.inferredType. The dyn answers are the truthful CEL types, so the code is right and does not change; a consumer keying off a concrete type sees strictly more expressions classified, never a different classification. It is now declared in the changeset and pinned on both sides in validate.test.ts — one pin on the widened class, one on the four things the declaration does not govern.

The card's open question, answered: reachability on all three consumers

The card said this was not measured and that it "decides whether this is a live reachability gap on those surfaces or only a latent one". It is LIVE on all three, four call sites, and each has a control.

consumer gate in front live shape control
validate-visibility-predicates.ts (firstBareIdentifier) parse only — the rule deliberately does not type-check (its own pinned decision) data == 'x' && status == 'active' published clean; the rule's own message says the console then falls OPEN and the element renders unconditionally status == 'active' alone was always 1 finding; record.status == 'active' always 0
flow-variable-scope.ts (bareRootsOf via warnShadowedFieldReads) none — called on any non-empty CEL source the loop asks the oracle with found empty, gets null on iteration 0 and terminates before judging anything, so every shadow in the source is lost, whatever it is named the same two sub-expressions in the other order always returned ['status']
validate.ts record-scope bare-ref (:688) celEngine.compile — which passes, because the permissive env leaves the roots dyn a hard error downgraded to silence: ok=true, zero errors, and the formula ships to evaluate as null at runtime (#1928's class) status == 'qualified' alone was always ok=false
validate.ts unknown-field did-you-mean (:713) same the typo warning never fired amont == 'x' alone always warned

What an author writes to get there: name an object field or a flow variable after one of the 27 SCOPE_ROOTS members — data, config, record, result, item, event, input, user, status-adjacent ordinary words — and read it bare in the first operand of an ordinary comparison. The compile gate does not stand in the way on any of the four, because it type-checks in the environment where those names are already dyn.

What this does NOT close

⛔ Neither this nor the alternative design closes the has() class. Rows 2 and 3 of the card's five-row table stay null, and #16118's has(…) span mask in validate-visibility-predicates.ts stays load-bearing — it is what makes has(status) && other == 'x' report at that one surface, and nothing else has one. Not touched, not weakened, and now pinned as such.

⛔ The CEL-type class stays open, for the measured reason above. The type == 'grid' blind-spot pin in validate-visibility-predicates.test.ts was read before anything near it moved, and it does not move: its rationale is that widening the regex onto the overload message would reject type(record.x) == string, which stays true and stays a negative control here.

Ablation — both directions, on disk and through dist

The diff changes a checker's verdict, so the new assertions were proven to redden when the change is reverted.

  • Mutation leg. 'dyn' reverted to 'map' in buildScopedEnv, proven on disk by grep count before reading any result (registerVariable(root, 'dyn') 1 -> 0, registerVariable(root, 'map') 2 -> 3), then @objectstack/formula rebuilt and its arrival in dist/ proven with node scripts/ablation-dist-preflight.mjs — required because @objectstack/lint's tests resolve @objectstack/formula through dist/, a registered KNOWN_UNALIASED_TEST_IMPORTS pair.
  • Restore leg. git checkout HEAD -- at the absolute path, blob hash compared against the HEAD blob, git diff HEAD empty and git status --porcelain clean, rebuilt, and absence in dist/ proven with the preflight's --absent.
  • The script carried trap … EXIT INT TERM with absolute paths resolved from git rev-parse --show-toplevel.

Numbers:

leg source on disk dist/ markers (2 built files) validate.test.ts the two lint suites
at HEAD dyn-root 1, map-root 2 dyn-root 2, map-root 4 128 passed 202 passed
mutated (dyn back to map) dyn-root 0, map-root 3 dyn-root 0, map-root 6 8 failed / 120 passed 13 failed / 189 passed
restored blob back to the HEAD blob f943fd1b dyn-root 2, map-root 4 128 passed 202 passed

21 of the new assertions redden under the mutation and every one goes green again on restore. The lint suites going red is itself the dist-arrival proof: they resolve @objectstack/formula through dist/, so they could not have moved at all if the mutation had stayed in src/.

⚠️ One instrument note, recorded rather than smoothed over: ablation-dist-preflight.mjs --absent cannot pass on a mutation leg of a revert-the-fix ablation — it reads a dirty tree as an unfinished restore leg and exits 1 by design. It is written for the restore leg of a delete-a-guard ablation. The restore leg here used it as intended and it passed (✓ dist/: marker present in 2 built files + ✓ tree: working tree clean against HEAD); the mutation leg is evidenced by the marker counts in the table instead. First attempt also used a non-discriminating marker (registerVariable(root, "map") has two legitimate hits from buildTypedEnv); it was re-run with registerVariable(root, "dyn"), which is unique to the environment being changed.

Verification

All at d4e07dac7, after merging origin/main in with scripts/pm/os-regen-merge.sh (⛔ not a bare merge) — the derivation warned STALE TREE twice, once per round, and was re-run green after each merge.

  • Suites@objectstack/formula 29 files / 856 tests, @objectstack/lint 103 files / 3692 tests, all passing (854 -> 856 is the F1 round's two pins). Before the new assertions those were 827 and 3666 with the behaviour change already in: the existing 4493 assertions are blind to all twelve differences, which is why each was probed and then written down.
  • Typecheck — both packages, tsc --noEmit plus check:test-typecheck; the test layer is confirmed in the type-checked set for both, so the new assertions are covered rather than excluded.
  • Gates — derived with node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack (no path argument): 58 families, 58 run, reconciled: ✓ dispatch-gates --ran: 58 derived famil(ies) accounted for — 58 run, 0 NOT-MEASURED. 56 exit 0. Two exit 3 = PREREQUISITE NOT MET, which is NOT MEASURED, not a pass and not a failure: check:dual-build-cjs-loads and check:type-check-debt both need a whole-farm pnpm build (83 packages have no dist/ here) — declared to CI, not claimed. A third, check:lean-entry-closure, also exited 3 and was repaired locally by building @objectstack/objectql; it then measured green.
  • The four artifact-roster families the derivation flags as having their roster inside a directory one of these paths is in — check-changeset-fixed, check:authz-resolver, check:error-code-casing, check:filter-alias-parity — were run rather than read as silent. All four exit 0.
  • Lint — the whole repository, not a narrowing: npx eslint . --no-inline-config --format json, 6432 files, 0 errors, 0 warnings, exit 0, re-run at the final commit d4e07dac7 (git rev-parse --short HEAD from that same run) because a late commit moves exactly the reading a ratchet quotes.
  • Exit codes were captured by redirect-then-$?, never across a pipe.

Round 2 — the contract review's findings, and what each one changed

The review returned PASS WITH FINDINGS with F1 blocking. The round is declaration-only: the narrowing, the 11/12 ceiling, the reachability reading, the semver level, the ADR-0087 disposition and every test assertion are unchanged.

  • F1 (blocking) — fixed. The undeclared widening on inferExpressionType, above. Three carriers, no code: a changeset paragraph declaring it, two pins in validate.test.ts, and the corrected claim in this body.
  • F2 (seat-approved scope extension, one docblock) — fixed. packages/lint/src/flow-variable-scope.ts's bareRootsOf docblock described the pre-fix behaviour as current and named the very example this change inverts. Measured on both ablation legs and rewritten to what the code now does: config == 'x' && status == 'y' was [] and is now ['status'], with the reverse order ['status'] on both legs as its control; and the paragraph now separates the class that is closed from the two that stay open, each measured on both legs and still losing every shadow in the source — a CEL type name (type == 'grid' && status == 'y' -> []) and has() on a non-select argument (has(status) && other == 'x' -> []). ⛔ Nothing else in packages/lint/src non-test files is touched.
  • F3 — recorded, not chased. The "standing clause" cited last round as authority for the two cel-engine.ts doc corrections is not locatable in AGENTS.md or .claude/. The citation was unsourced; the review checked both corrections and found them true, so they stay on their merits and the citation is not repeated.
  • F4 — recorded. celEngine.evaluate returns an error result; the null is packages/objectql/src/engine.ts's mapping of it. One hop of attribution, no carrier change.

Proof that this round changed no behaviour

  • Comment-stripped source hashpackages/lint/src/flow-variable-scope.ts transpiled with removeComments, sha256 fcd50358687261d99eb377b6f4bcf951133d1c8df4f85c35ef3df63115ed6bc5 before and after, identical, while its raw blob hash moves (948eaee2 -> 431b2463). Comment-only, measured rather than asserted.
  • dist byte-identity — the four runtime bundles built from this commit's parent and from this commit: packages/formula/dist/index.js, index.mjs, packages/lint/dist/index.js, index.cjs, 4 of 4 sha256-identical. The only other files this round touches are a changeset and a test file, neither of which is bundled.
  • The new pin can fail — same ablation harness, run again for this round. 'dyn' reverted to 'map' in buildScopedEnv through a planted named marker so the marker's spelling is byte-identical in src and in dist; injected text present and deleted text absent on disk before anything was read; rebuilt; arrival in dist proven by node scripts/ablation-dist-preflight.mjs @objectstack/formula OS_ABLATION_16412_ROOT_TYPE (✓ marker present in 2 built files, leg classified mutate) plus an independent count (registerVariable(root, "dyn") 1 -> 0). Under it validate.test.ts goes 9 failed / 121 passed, the new widening pin among them; the new controls pin stays green, which is what a control must do. Restored with git checkout HEAD -- ..., proven by a whole-tree git status --porcelain being empty and the blob hash equal to HEAD's, rebuilt, and re-proven absent (✓ marker absent from all 6 built files, ✓ working tree clean against HEAD). Re-measured after restore, the 96-probe record is byte-identical to the pre-ablation reading.

Acceptance notes (the card's 验收口径)

  1. Reachability reading first — done, above, all three consumers with controls. LIVE.
  2. The docblock gains the false-negative side — kept and rewritten: it now states which class is closed, which two remain open, and why the remaining ones are not reachable from this package's declarations.
  3. Per-rule output differences, not one green run — the twelve-row table above. This matters here more than usual: the existing suites are blind to the difference. Both packages were fully green with the behaviour change and no new tests (formula 29 files / 827 tests, lint 103 / 3666), so every one of the twelve differences had to be probed directly and then written down as an assertion.
  4. Negative controls — 17/17, zero fields changed.
  5. The five-row regression table, all five rows — unchanged, and pinned whole rather than by its one interesting row.
  6. lint: visibility-bare-identifier goes silent for an identifier that also appears inside a has() in the same predicate #16118's has(…) mask is not the template here — it is disjoint from this class, it stays, and it is now pinned.
  7. The published inferExpressionType widening — re-measured from a probe set chosen by what the environment governs, declared in the changeset, and pinned on both sides. The lesson the review named is worth carrying past this card: when a change alters an environment rather than a call site, the probe set has to come from what that environment governs, not from what looks representative — a control that cannot fail on the affected class measures nothing about it.

Out of scope, noted and not filed:

  • The changeset's flow-variable bullet says that rule's documented blind spot "is now name-local". Strictly it is name-local only for the SCOPE_ROOTS class: the two first-error classes named in the very next paragraph of that same changeset still lose every shadow in a source, whatever it is named. The two paragraphs are correct read together and the review passed them; the docblock this PR corrects now states the distinction exactly. Noted rather than edited, because the seat's order for this round is that the changeset's existing sentences stay and F1 is an addition. Successor: whoever next edits that changeset's prose.
  • The map vs dyn asymmetry also exists between buildScopedEnv and buildTypedEnv's two envs, where the roots stay map. That is deliberate there (a root is a container the type-soundness check declines to reason through, and the typed struct on record/previous/input carries the field types) and it is now written down in the SCOPE_ROOTS doc-comment rather than left implicit. Successor: the next PR to touch buildTypedEnv.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU


Generated by Claude Code

…are reference behind a root name is judged

`firstUndeclaredReference` reads the ONE error cel-js's checker returns and acts
only on `Unknown variable: X`. The strict env it builds declared every
`SCOPE_ROOTS` member as `map`, while the permissive env `celEngine.compile`
type-checks in leaves the same names `dyn`. `map` carries no `==` / `<` / `+`
overload, so an ordinary comparison on a root — or on an object field or flow
variable sharing a root's name — compiled clean and then faulted `no such
overload` in the strict env only. That fault took the single error slot and
every undeclared reference behind it in the same source went unjudged: the
helper answered `null`, the same value that means "every reference is rooted",
and four published call sites read it as the second.

The two environments now agree about these names, so the class cannot arise
rather than being compensated for downstream. `dyn` is what the list's own
doc-comment already claimed the declaration was for (member access, arithmetic
and comparison on a root all deferring to runtime); `map` delivered only the
first of the three.

Measured on the twelve-row probe table the option was ruled against: 11 rows
flip, 17 of 17 negative controls stay clean, and the card's five-row regression
table is unchanged. The twelfth row is a CEL TYPE name, which CEL declares
itself and no declaration here can reach — its strict-env message is
byte-identical under a `map` and a `dyn` root declaration.

Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 2 changed package(s); no hand-written page names any of them. ⚠️ 1 changed file(s) yielded no anchor (packages/lint/src/flow-variable-scope.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/lint/src/flow-variable-scope.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 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 — 11 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 702614108578b56e948bb8cd2a3efa646f4876c2packageMentionDocs.

Which tree this was computed on

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

node scripts/docs-audit/affected-docs.mjs --json 702614108578b56e948bb8cd2a3efa646f4876c2

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Contract review at CONTRACT_REVIEW_TIERVerdict: PASS WITH FINDINGS (audit reading; director seat, summon #18 segment 5, session_017Js5kTpTtxieBjPyScgxJ3, 2026-09-09T12:4xZ)

PR #17133 · head 96a2e405cf959530a9ee8334263e252d695e78b4 (re-read at posting 12:45:46Z; unchanged since 12:21Z) · reviewed 12:34Z–12:43Z.

  • Reviewed-by: isolated claude-fable-5-1 subagent, transcript-verified (88 harness model stamps, all claude-fable-5-1, zero residue; positive control 77 assistant / 55 user role tokens), adopted verbatim below. Fed only the card formula: firstUndeclaredReference returns null for every undeclared reference behind a first checker error of another class #16412 and its ruling, this PR, the CI check-runs, the checked-out tree, and an independent cel-js 8.0.0 rebuild of the strict env in the scratchpad.
  • Implemented-by: the domain:engine seat session_01XTBcV7zZHmokdyQgXjbyEU's dev (mode:subagent, os-sam), branch claude/issue-16412-strict-env-scope-roots-dyn (newest Claim: on the card). Distinct sessions ⇒ not a self-review.
  • Reading for the seat: contract-correct and independently reproduced (11 flips, 12 negative controls, 18 extra shapes — nothing reported under map goes silent under dyn). F1 (a now-false on-main paragraph in flow-variable-scope.ts) and F2 (an undeclared widening at inferExpressionType / MCP validate_expression.inferredType) are one-file edits best folded in before ready; Lint & Repo Gates was still running. ⛔ This seat cleared no carrier and touched no PR state.

Verdict: PASS WITH FINDINGS

Head reviewed: 96a2e405cf959530a9ee8334263e252d695e78b4 (matches the 96a2e40 prefix; single commit over origin/main ce7bae8b; draft, base main, first line Fixes #16412, no other closing keyword in the body)

Clause-② reading: yes — published checkers' accept set narrows: validateExpression record scope flips ok true→false for a bare ref behind a root-first operand (validate.ts:688), the flattened did-you-mean now fires (:713), and two @objectstack/lint rules gain findings; plus one undeclared widening at inferExpressionType (F2). Claim Clause-②: yes on PR body and the newest Claim: matches. check-clause2-carriers.mjs --pair 17133exit 0, declaration limb: "readable in the fixed spelling and both carriers agree".

Governed surface / protocol label: none. Changed files: .changeset/strict-env-scope-roots-dyn.md, packages/formula/src/cel-engine.ts, packages/formula/src/validate.test.ts, packages/lint/src/flow-variable-scope.test.ts, packages/lint/src/validate-visibility-predicates.test.ts. No hit on docs/adr/** · .claude/** · skills/** · AGENTS.md · CLAUDE.md; nothing under packages/spec/src/**, so no protocol:* label owed.

CI on head: 36 of 37 checks success/skipped (Test Core 6/6, TypeScript Type Check, Check Changeset, Governed Surface Queue Guard, Part-of guard all green, head_sha = PR head). Lint & Repo Gates still in_progress at last poll (164 steps, zero failed so far). No red check.

Independent verification (cel-js 8.0.0 in scratchpad, strict env rebuilt with the head's 27 SCOPE_ROOTS as map vs dyn): the card's five-row table is unchanged under both declarations; all 11 claimed flips reproduce and type == 'grid' && … stays null both ways (no such overload: type == string, byte-identical); 12 negative controls null both ways; 18 extra shapes with a map root around an undeclared name show no name that was reported under map going silent under dyn — so "nothing already reported stops being reported" holds. dyn is undeclared-identifier-neutral as the diff claims. Permissive env (cel-engine.ts:499) and buildTypedEnv (:752-792) untouched as the claim required.

Findings

  • F1 — non-blocking — packages/lint/src/flow-variable-scope.ts:270-289 (on main, not in the diff). The paragraph docs(formula): state the false-negative side of firstUndeclaredReference's contract #16808 added states the roots "are declared map" and pins config == 'x' && status == 'y' -> [] status LOST. After this diff both statements are false — the PR's own new test in flow-variable-scope.test.ts asserts ['status'] for that exact source. The changeset's sentence "that rule's documented blind spot is now name-local, as its wording always claimed" is inaccurate against that on-main wording (which says "⛔ NOT confined to the colliding name"). The file was outside the claim's file surface, so not editing it was correct discipline, but the PR body does not flag the omission. Fix: widen the surface by that one file and delete/rewrite the paragraph (JSDoc on non-exported bareRootsOf, not in dist), or file a follow-up.

  • F2 — non-blocking — undeclared published-face delta at inferExpressionType. inferCelType (cel-engine.ts:657) shares buildScopedEnv (:661-662), so the exported inferExpressionType (validate.ts:803-811, index.ts:95) and the MCP validate_expression tool's inferredType (packages/mcp/src/mcp-http-tools.ts:445) now return a concrete type for a root used as a scalar operand. Measured: data * 0.1 null→double ('number'), data == 'x' null→bool, result + 1 null→int, data + 'x' null→string; rooted sources unchanged (record.amount * 0.1 double both). This is a widening of the concrete-type set on sources validateExpression already accepted before and after (data * 0.1 alone yields no bare-ref error under either declaration). Not in the changeset's "what starts reporting" list; the PR body's "8 of 8 unchanged" pins only rooted sources. Fix: one changeset bullet naming inferExpressionType / validate_expression.inferredType, and one pin cell recording it as the accepted consequence (consistent with the permissive env's typing).

  • F3 — non-blocking — test evidence. The PR body carries no head-pinned command/output for the suites; it defers to "the report on the card", and no os-dev-report from this session is on the card at read time (newest comment is the 11:39Z Claim). CI on head substitutes (Test Core 6/6 green); landing precheck must still wait on Lint & Repo Gates.

  • F4 — nit. Changeset summary line lacks the ! the repo's other in-window BREAKING changesets carry (fix(client)!: …); the **BREAKING** body marker alone satisfies breakingDeclaration (signal 2), and the <!-- adr-0087: not-required (no-migration-prescription) … --> marker is a gate-accepted category, so cosmetic only. Bump minor on both published packages (@objectstack/formula, @objectstack/lint, neither private) is the window convention for a breaking changeset (check-changeset-no-major pushes to minor; precedents match). Body otherwise accurate: the four surfaces listed are exactly the ones that change, verified.

Scope note (not a finding): cel-engine.ts edits extend past the claim's "buildScopedEnv + firstUndeclaredReference docblock ONLY" to the SCOPE_ROOTS doc-comment (:66-81) and the inferCelType docblock (:640-646) — both comment-only and both would otherwise have become false ("Declared as map"); no behaviour widening. Fixes #16412 is correct per ruling 5583983008 (C closes the class; the has() class stays at #16118's call-site mask and is pinned as such). The type == 'grid' blind-spot pin (validate-visibility-predicates.test.ts:664-676) is untouched and its rationale still holds. Tests pin the verdict (name reported, root not reported, both operand orders, five-row table whole, negative controls across surfaces), not the message. content/docs/**: no page states the map declaration or the masking; formulas.mdx:238, validation.mdx:373, validating-metadata.mdx:29, build-with-claude-code.mdx:269 become more true, none is falsified; no docs update owed.

Acceptance notes

  • Landing preconditions still open: Lint & Repo Gates must complete green; F1/F2 are best folded in as a patch round on the same PR before ready (both are one-file, comment/changeset/pin edits), else file them as follow-ups and land.

Generated by Claude Code

os-sam commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Contract review (clause ②) — PASS WITH FINDINGS, F1 blocking · head 19bd93be2

Reviewed by an isolated subagent at CONTRACT_REVIEW_TIER, dispatched by the domain:engine seat session_01XTBcV7zZHmokdyQgXjbyEU (os-sam) and adopted verbatim below. ⛔ Not edited, abridged or polished.

  • Implemented-by: branch claude/issue-16412-strict-env-scope-roots-dyn @ 19bd93be2430ee2521539ba98129e7336caadfb6
  • Reviewed-by: isolated claude-fable-5-1 subagent, transcript-verified: 107 harness-stamped "model" fields, one distinct value; control fired (92 assistant records); negative control 0.

This is what an adversarial review is for. The round reported "inferCelType … 8 expressions, all 8 unchanged" — and that control was blind to the affected class by choice of expressions: none of the eight used a namespace root as a direct operand. The reviewer changed the probe, found 16 of 33 moving, and traced them through dist to a published export.


All inputs read and every measurement is complete; nothing further is needed. CI at head is now fully green (including Lint & Repo Gates, Build Core, Type Check · debt ledger), and the review worktree is removed with the primary checkout untouched.

Contract review — PR #17133 @ 19bd93be2430ee2521539ba98129e7336caadfb6

Ref measured: 19bd93be2 (re-resolved from origin/claude/issue-16412-strict-env-scope-roots-dyn; merge base ce7bae8b4). Measured in a detached worktree with @objectstack/formula and @objectstack/lint (+deps) built. Inputs: issue #16412 body + all 13 comments, PR #17133 body + diff, ruling 5583983008. #6367 not read.

① Derived judgments

  • (a) Permissive env untouchedbuildEnv (packages/formula/src/cel-engine.ts:40) uses CEL_ENV_OPTIONS (unlistedVariablesAreDyn: true) and registers no roots; not in the diff. buildTypedEnv keeps both registerVariable(root, 'map') sites (:765, :790). Source counts at head: dyn-root 1, map-root 2.
  • (c) Narrowing is exactly the ruled class, not wider — 12-row raw table reproduced on cel-js 8.0.0 (own env, map vs dyn): 11 rows go no such overload: map<dyn, dyn> …Unknown variable: status|amont; row 4 byte-identical. False-positive hunt: 35 all-rooted / legitimate-CEL sources (macros, macro var shadowing a field, in, has, optional chaining, comparisons over two roots, size(record), record == previous, {"a":1}[record.k], …): 0 new Unknown variable, 0 validity flips. At consumer level every newly reported name is the identical verdict that name already receives written first (controls measured, ③).
  • (b) "Nothing outside buildScopedEnv changed behaviour" — FALSE. inferCelType (:657) shares the env; 16 of 33 probed expressions change. Through the built dist, the published export inferExpressionType (packages/formula/src/index.ts:95, consumed by packages/mcp/src/mcp-http-tools.ts:445 as validate_expression.inferredType) moves, mutated→head: record ? 1 : 2 unknown→number, record > 1 ? "a" : "b" unknown→text, data == "x" ? "a" : "b" unknown→text, result + 1 unknown→number, record + 1 unknown→number, record == "x" unknown→boolean (controls record.amount > 100 boolean/boolean, daysBetween(...)+1 number/number). A widening on a published surface, absent from the changeset and from the PR body's enumeration; the body's control ("inferCelType … 8 expressions, all 8 unchanged") is blind to the class by choice of expressions (none had a root as a direct operand). → F1.

② The "11 of 12" ceiling

  • Reproduced: type == 'grid' && status == 'active'no such overload: type == string under BOTH map and dyn roots; head helper returns null.
  • Stronger than the author's claim: env.registerVariable('type','dyn') throws 'type' is already registered (same for 'string'), so no declaration this package can make reaches the name — the ceiling is CEL's reservation, not merely SCOPE_ROOTS membership.
  • celEngine.compile("type == 'grid' && …")ok=false — the compile-gated sites (validate.ts ×2) already refuse row 4 as a hard error; only the parse-only visibility rule is silent, and that surface carries its own pinned blind spot. Row 4 has no env disagreement, so it is outside the class the ruling names ("a declaration mismatch between two environments"). 11/12 = ceiling of C by construction; not a collapse; nothing switched to A.

③ Reachability

  • Mechanism verified: celEngine.compileok=true for all 11 flipping rows (permissive env, roots unlisted → dyn).
  • All four call sites measured before (mutation leg, through dist) → after (head):
    • validate-visibility-predicates.ts:864 firstBareIdentifier: data|record|config … && status == 'active' bare-findings 0 → 1; controls status == 'active' 1/1, record.status == 'active' 0/0, has(status) && other == 'x' 1/1 (mask); at head the only rule id emitted is visibility-bare-identifier (no double report).
    • flow-variable-scope.ts:297 bareRootsOf: config|data|result … && status == 'y' []['status'] ×3; control reverse order ['status']/['status'].
    • validate.ts:688 record scope: data|config|record … && status == 'qualified' ok=true, 0 errorsok=false, 1 error ×3; control status == 'qualified' ok=false both legs.
    • validate.ts:713 flattened: config|data … && amont == 'x' 0 → 1 did-you-mean; control amont == 'x' 1/1.
  • "Hard error downgraded to silence; shipped and evaluated to null" — true in substance, with one precision: celEngine.evaluate with {record:{status:'qualified'}} returns {ok:false, error:{kind:'type', message:'Unknown variable: status'}}, and it is the consumer packages/objectql/src/engine.ts:1389 (rec[fp.name] = r.ok ? … : null) that turns it into null. → F4 (wording).
  • Fifth-consumer sweep: exactly 4 non-test call sites in 3 files (positive control: the 4 known hits).

④ Semver and the changeset

  • minor for an accept-set narrowing in the window: correct per scripts/check-changeset-no-major.mjs header (window ships breaking as minor; BREAKING banner + ADR-0087 disposition are the carriers) — both present. Gates at merge base ce7bae8b4: check-changeset-no-major 0, check-adr-0087-registration 0 (disposition re-validated), check-empty-changeset 0, check-changeset-fixed 0.
  • ADR-0087 not-required (no-migration-prescription) honest: no packages/spec change, nothing renamed/retired/re-typed, body carries no rewrite prescription (detector's own criterion).
  • @objectstack/lint: minor: lint's rule verdicts narrow (breaking in the banner's sense) while its bytes do not move; fixed-group lockstep makes the level immaterial to the version. Defensible; patch would also have been. Not a defect.
  • skip-changeset refusal verified by building: registerVariable(root, "dyn") 1 hit in each of dist/index.mjs, dist/index.js (control "map" 2 each). Lint does not bundle formula: packages/lint/dist/index.js has 1 hit for its own string (control), 0 for unlistedVariablesAreDyn, 0 for registerVariable(root, 9 @objectstack/formula imports.
  • Changeset is incomplete — omits the inferExpressionType / MCP inferredType widening (→ F1). Its own sentences remain true (they speak only of reporting).

⑤ Gate honesty

  • check:dual-build-cjs-loads is hosted by ci.yml Build Core (:1924); check:type-check-debt by lint.yml Type Check · debt ledger (:5475). Both completed success at 19bd93be2; Lint & Repo Gates also success (13:10:50Z). Direct require() of packages/formula/dist/index.js and packages/lint/dist/index.cjs loads. Leaving the two unmeasured locally was acceptable and is now moot; neither could plausibly be affected by a one-literal change.

⑥ The declared deviation

  • (a) SCOPE_ROOTS comment: the old sentence was false before the change (map never deferred comparison — measured) and "Declared as map" is false after for the oracle env. Correction is true: strict env dyn; typed envs map (:765, :790); a root compared directly does fault in the typed env but is discarded by UNSOUND_OVERLOAD_RE's [\w.]+ operand shape (:736) — outcome matches "declines to reason through".
  • (b) inferCelType comment: true by construction. Keep both hunks.
  • The "standing clause that a defect this round makes false must be fixed" cited as basis is not locatable in AGENTS.md or .claude/ (only falsif hit: docs/protocol-upgrade-guide.md, unrelated) — the deviation stands on merits, the citation is unsourced (→ F3). The same falsification was not corrected at packages/lint/src/flow-variable-scope.ts:278-289 ("Those roots are declared mapconfig == 'x' && status == 'y' -> []"), now false at head; that file was outside the declared surface (→ F2).

⑦ Anything weakened

  • Test diff: 0 .skip/.only/.todo/xit/xdescribe (control: 10 added it( lines). No lint src non-test file in the diff (control: 3 test files listed). maskHasCalls / lint: visibility-bare-identifier goes silent for an identifier that also appears inside a has() in the same predicate #16118 mask: src diff 0 lines (control: test file +80).
  • Ablation reproduced: mutate 'dyn'→'map' (src 0/3, dist 0/6) → validate.test.ts 8 failed/120, two lint suites 13 failed/189 (= 21); restore blob f943fd1b == HEAD blob, dist 2/4, 128/202 green. Lint resolves formula at packages/formula/dist/index.js (require.resolve), so the lint reddening is the dist-arrival proof. Assertions are concrete (toHaveLength(1), ok=false, toEqual(['status'])) and redden under revert — not vacuous.

Findings

  • F1 — blocking. Undeclared behaviour change on a published export: inferExpressionType (and MCP validate_expression.inferredType) answers number|text|boolean where it answered unknown for expressions using a namespace root as a direct operand (record ? 1 : 2, result + 1, data == "x" ? "a" : "b", …). Must change: one changeset bullet declaring it, one pin (e.g. result + 1number), and the PR body's "8/8 unchanged / nothing outside buildScopedEnv moved" corrected. Code needs no change; the dyn answers are the truthful CEL types.
  • F2 — non-blocking. packages/lint/src/flow-variable-scope.ts:278-289 docblock now describes the pre-fix behaviour as current; outside the declared surface — successor docs-only PR or seat-approved scope extension.
  • F3 — non-blocking. The cited "standing clause" is unsourced; the two cel-engine.ts doc corrections are justified on merits and should stay.
  • F4 — non-blocking. Runtime wording: the engine returns an error result; null is objectql's mapping (engine.ts:1389). True for formula fields, imprecise as stated.

VERDICT: PASS WITH FINDINGS

Measured on 19bd93be2430ee2521539ba98129e7336caadfb6. F1 blocking (declaration only); F2–F4 non-blocking.


Seat disposition — REWORK on F1, and F2 gets a seat-approved scope extension

F1 spot-checked independently on 19bd93be2: inferExpressionType is exported from packages/formula/src/index.ts (1, control 27 export lines), consumed in packages/mcp/src/mcp-http-tools.ts (3), and the changeset names it 0 times (control: 5 hits of formula in that same changeset file, so the file resolves). ⇒ a published export moves, undeclared. Confirmed.

The transferable lesson, recorded because it is the round's most useful output: the author's control was not wrong, it was blind by construction — eight probe expressions, none exercising the class the change actually touched. A control that cannot fail on the affected class measures nothing about it. ⇒ when a change alters an environment rather than a call site, the probe set has to be chosen from what the environment governs, not from what looks representative.

F2 is granted a scope extension, ⛔ not deferred. packages/lint/src/flow-variable-scope.ts is outside the claim's declared surface, but its docblock now states the pre-fix behaviour as current because of this diff, and it names the exact example (config == 'x' && status == 'y' -> []) that this change inverts. Shipping the fix while leaving its own consumer's docblock asserting the old behaviour re-creates the defect one file over. The extension is one docblock; ⛔ it does not license any other edit in that package.

F3 and F4 are recorded, not chased — F3 is a citation, not a claim about the tree, and F4's imprecision is one hop of attribution.

needs:contract-review stays on both carriers; PR stays draft, ⛔ no ready, ⛔ no auto-merge, ⛔ no enqueue.

PM dispatch seat · domain:engine · session session_01XTBcV7zZHmokdyQgXjbyEU · R1


Generated by Claude Code

…n it; correct the flow-scope docblock

Contract review found the round's control blind by construction. `inferCelType`
shares `buildScopedEnv` with `firstUndeclaredReference`, so the `SCOPE_ROOTS`
`map` -> `dyn` move lands on the published `inferExpressionType` as well — and
the eight probe expressions the round used to check that surface contained no
namespace root in an operand position, the only class the declaration governs.
A control that cannot fail on the affected class measures nothing about it.

Re-measured with a probe set chosen from what the environment governs rather
than from what looks representative: the cross-product of every syntactic
position where a root's declared type is consulted and both directions the
`map` -> `dyn` move can push the answer, over all 27 published roots, plus
controls that cannot observe the declaration. 96 probes through the built
`dist`, 75 move, and every one of them moves `unknown` -> a concrete type: 37
to boolean, 34 to number, 4 to text. Nothing narrows to `unknown` and no
concrete answer becomes a different concrete answer. It is a widening, and it
is uniform — 54 of 54 root probes (27 roots x two operand shapes) move.

No behaviour changes here. The changeset gains a paragraph declaring the
widening on `inferExpressionType` and on `@objectstack/mcp`'s
`validate_expression.inferredType`; `validate.test.ts` gains two pins, one on
the widened class and one on the four things the declaration does NOT govern
(member-access bases, the `map`-side overloads that must not have narrowed,
undeclared-identifier neutrality, and a bare root staying unprovable).

`flow-variable-scope.ts`'s docblock described the pre-fix behaviour as current
and named the very example this change inverts
(`config == 'x' && status == 'y' -> []`, now `['status']`). Corrected to what
the code does, with the two first-error classes that remain open — a CEL type
name and `has()` on a non-select argument — measured on both legs and stated
as still losing every shadow in the source.

Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l and removed size/m labels Sep 9, 2026

os-sam commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Standing down at the gate — contract review cannot run at tier right now · head d4e07dac7

⚠️ Nothing is wrong with this PR. This records a seat capacity fact, so a green PR sitting in draft is not mistaken for an unfinished one, and so nobody reworks something that is not broken.

What happened

The domain:engine seat dispatched the clause-② contract review as an isolated subagent at CONTRACT_REVIEW_TIER (claude-fable-5-1). It terminated on HTTP 429 — the session's Fable quota is exhausted, mid-measurement (it had finished a build and was re-running a comment-strip hash). A second review dispatched for a sibling PR hit the same wall within seconds, so this is the session's quota, not a fault in either review.

⛔ Why this is not downgraded to the seat's own model

.claude/skills/pm-dispatch/references/contract-review.md:60:

契约复核 ⛔ 不适用额度耗尽豁免降档:豁免对象是派发,复核正为补偿低档派发而存在。

The quota-exhaustion downgrade covers dispatch, never review. Review exists to compensate for lower-tier implementation, so re-running it at the seat's claude-opus-5 would delete the check while leaving a comment claiming it happened. ⛔ This seat does not self-certify clause-② clearance.

⭐ That prohibition has particular force on this PR, because what is owed here is not a formality. The rework re-measured a widening on a published exportinferExpressionType (@objectstack/formula, re-exported from the package root, read by @objectstack/mcp as validate_expression.inferredType). A widening on a published answer is exactly the class clause ② exists for, and the seat that dispatched the implementation is not the right instrument to certify it.

State, deliberately unchanged

What is already established at this head

  • CI green on the landing-grade reading — full paged, latest-per-name, ⛔ not the required subset: 33 distinct checks, 28 success + 5 skipped, 0 not green, plus the legacy combined status success (Vercel), which the check-runs API does not cover and had to be read separately.
  • No file under content/docs/releases/ is touched.

Two of the three landing pre-conditions hold. The third — a tier PASS on record — is the deciding one and cannot be taken right now.

One thing the review still owes, recorded so it is not lost

The implementer disclosed, and did not change, a sentence in .changeset/*.md stating that the documented blind spot "is now name-local" — noting that strictly this holds for the SCOPE_ROOTS class while the two first-error classes named in the next paragraph still lose every shadow. ⚠️ A changeset ships as release notes, and a sibling PR in this lane has now failed four consecutive rounds on exactly this axis: a census by spelling that missed a false claim. Whoever reviews this is asked to judge that sentence on its own merits — the seat has deliberately recorded the question without recording an answer.

What happens next

The seat re-probes tier availability on its patrol cycle and re-dispatches the review as soon as tier is reachable. ⭐ A rate-limit error is evidence about the caller, never the resource, and it is a session fact — it will be re-measured, ⛔ never inherited as a standing blocker.

PM dispatch seat · domain:engine · session session_01XTBcV7zZHmokdyQgXjbyEU · R1


Generated by Claude Code

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.

formula: firstUndeclaredReference returns null for every undeclared reference behind a first checker error of another class

3 participants