Uh oh!
There was an error while loading. Please reload this page.
fix(lint): extend the flattened-scope shadowing warning to the descriptor-declared predicate slots - #14576
Conversation
…ptor-declared predicate slots (#14288) The #14089 shadowing warning ran on the node `condition` and the edge `condition` only. The #4027 descriptor-declared predicate slots were checked for dialect by the same traversal but never passed through the shadowing pass, so a bare name that is BOTH a declared flow variable AND a field on the bound object stayed silent there for exactly the reason it was silent on `condition` before #14089. Measured on the engine before extending the lint, because "same scope" is the whole premise: `seedRunVariables` builds ONE map per run and it threads unchanged into every node executor. `decision.conditions[].expression` evaluates against that same Map object; `screen.fields[].visibleWhen` evaluates against the persisted snapshot of it with the submitted bag overlaid (a superset, and the overlay can never restore the displaced field). One more `warnShadowedFieldReads` call site reusing the `declaredVariables` set already collected once per flow. Warning-only: no new rule id, no severity above `warning`, no accept set moved, no bare identifier judged for being bare. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLJQhde67SeTccsmnBVarV
…scriptor-slot-shadow-warn
📓 Docs Drift CheckThis PR changes 1 package(s): ⛔ 1 release-owned page(s) name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 84c605e5de514dcae6217d69c81ef4f8776e4371 && git checkout 84c605e5de514dcae6217d69c81ef4f8776e4371
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7286dd58e806ed321cbdfc23a1c455db8f80b1ad b3fbd92bc3db882601a32037d05bdb3b2695fc6e && git checkout -B drift-repro 7286dd58e806ed321cbdfc23a1c455db8f80b1ad && git merge --no-ff b3fbd92bc3db882601a32037d05bdb3b2695fc6e
node scripts/docs-audit/affected-docs.mjs --json 7286dd58e806ed321cbdfc23a1c455db8f80b1ad
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#14288
This card was measure-first: the ruling made "narrower per-node scope ⇒ close as not-a-gap, no PR" a complete deliverable, so the scope question was answered on the engine before any lint file was touched. The measurement says shared map, for both predicate slot kinds — so the fix applies, and it is one call site.
1. The measurement (engine, read-only)
All citations against
packages/services/service-automation/src/engine.tsat basefed4fa409.The shadowing mechanism is unchanged
seedRunVariables(engine.ts:7673-7692) seeds the flow's declared variables first and flattens the record's fields only where nothing is bound yet::7679—const variables = this.seedDeclaredVariables(flow, context);:7684—for (const [k, v] of Object.entries(context.record)) { if (!variables.has(k)) variables.set(k, v); }So a name bound as BOTH a declared variable and a field on the bound object resolves to the variable, silently. That is the #14089 diagnostic's whole subject.
The single map threads to every node executor — no clone, no narrowing
:3835—const variables = this.seedRunVariables(flow, flowName, context, runId);:3937—await this.executeNode(startNode, flow, variables, runContext, steps);:6578-6583—executeNode(node, flow, variables: Map, context, steps):6647/:6652—executor.execute(node, variables, context)The two positions PR #14263 already covers ride that same map: node conditions at
:3874and edge conditions at:6955. A grep fornew Map(variables/new Map(...variables/scopeFor/narrowScopeoverengine.tsreturns zero hits — the onlyconst scope = new Mapin the file is:5097, and it is a superset (below).Slot kind 1 —
decision.conditions[].expression⇒ SHARED MAPpackages/services/service-automation/src/builtin/logic-nodes.ts:73variablesis the executor's own second parameter (async execute(node, variables, _context),logic-nodes.ts:46) — literally the sameMapobject handed in atengine.ts:6647/:6652, and therefore the same object a nodeconditionis judged against at:3874. Same verdict ascondition.Slot kind 2 —
screen.fields[].visibleWhen⇒ SHARED MAP (superset)refuseInvalidScreenInput(engine.ts:5084, called:4848)::5097—const scope = new Map(Object.entries(run.variables));:5098—for (const [k, v] of Object.entries(bag)) scope.set(k, v);:5102—return this.evaluateCondition(String(field.visibleWhen), scope);run.variablesis the persistedseedRunVariablesmap: the suspend path snapshots it at:4007(Object.fromEntries(variables)) intopersistSuspendedRun:4014; the re-suspend path does the same at:4971/:4976, andexecuteWithoutRetryat:7866/:7873. Resume rebuilds the traversal map from that identical field at:4855.The submitted bag is overlaid on top, making the scope a superset — so the shadow still reaches it. The overlay carries the screen's own collected values (user input), never the bound record's field, so it can never hand back a field the variable displaced. A superset makes the warning more correct here, not less.
Verdict
Both
predicaterows on the ledger evaluate against the run's one flattened variable map. Outcome A.loop.collectionandmap.collectionareflow-template, notpredicate, and the slot loop already skips them before this pass — so they are correctly untouched.2. The change
packages/lint/src/validate-expressions.ts— the descriptor-slot loop (:1137-1143on base). The location label is hoisted into oneslotWhereconst (it was already built inline forcheckDeclaredPredicate) and onewarnShadowedFieldReads(slotWhere, found.value)call is added inside the existingrole === 'predicate'branch, reusing thedeclaredVariablesset collected once per flow at:1093. The comment records the measurement above so the next reader does not have to re-derive it.Within option C's letter, as the ruling requires: warning-only, same severity, no new rule id, no accept set moved, and no bare identifier judged for being bare. Nothing that linted clean before can newly fail a build.
3. Tests
packages/lint/src/validate-expressions.test.ts— one positive and one negative per predicate slot kind, plus a severity assertion, nested inside the existingflattened-scope shadowing (#14089)block and reusing itsshadowFields/record_changefixtures:visibleWhenreading a shadowed bare name ⇒ exactly one warning namingscreen field visibleWhenatconfig.fields[0].visibleWhenvisibleWhenwhose bare name is a field only ⇒ zero issues (the canon-taught form)decision branch expressionatconfig.conditions[0].expressionwarning(atoHaveLength(1)would still pass if the issue were an error, so severity is asserted separately)Reverse verification. Predicted direction: RED. The subject under test is imported relatively (
from './validate-expressions.js'), so it resolves to package source, notdist— no rebuild leg applies. From the committed state, the single call-site line was deleted and the mutation confirmed on disk before measuring (marker count 1 before, 0 after;git diff --statshowed1 deletion). Result: 3 failed, 2 passed — both positives (expected [] to have a length of 1) and the severity test (expected [] to have a length of 2). The two negative controls stayed green, which is correct: they assert zero warnings and so cannot detect the call site — they are controls, not detectors. The restore leg was then proven by byte identity, not by an exit code: worktree blob47a90b260358d4c0ffdf8d4ca8adfe15e686844bequals theHEADblob,git diff HEADempty, marker count back to 1.4. Verification
Union run on the final merged commit
b3fbd92bc(aftergit merge origin/main).pnpm --filter @objectstack/lint test— 93 files, 2822 passedpnpm --filter @objectstack/lint typecheck— clean.tsconfig.jsonexcludes*.test.ts, so thecheck:test-typecheckhalf is what covers the new tests;tsc -p tsconfig.test.json --listFilesconfirms both edited files are in that program (the "green over source nothing read" trap), and the 6 residual errors are the ledgeredTS6059in two other files, none of them mine.pnpm lint— whole repo,eslint . --no-inline-config, exit 0. Not narrowed, so no narrowing argument is owed.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack(no paths passed — the script derives its own change set from the merge base): 34 families, identical before and after the merge. Harvested with--commands, not from the prose. Result: 33 exit 0, 1 NOT MEASURED —check-test-completeness.mjs, which grades a CI-producedturbo run testlog and printsPREREQUISITE NOT METwith exit 3 locally, its own documented NOT MEASURED branch.node scripts/pm/dispatch-gates.mjs --ranreconciles:34 derived, 34 run, 0 UNRUN.turbo run build, 70/70):check:type-check-debt(--re-measure: OK, none above its recorded number) andcheck:dual-build-cjs-loadsboth exit 0, as did the build-free ratchets.Exit codes were captured by redirecting first and reading
$?before any pipe, and each verdict above is the gate's own printed judgement line rather than a bare$?.5. Changeset
.changeset/lint-shadow-warning-descriptor-predicate-slots.md—@objectstack/lint: patch.Draft, per the dispatch order: not marked ready, auto-merge not enabled.
Generated by Claude Code
Generated by Claude Code