Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Warn when a bare flow-condition identifier is shadowed by a declared flow variable - #14263

Merged
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier
Sep 2, 2026
Merged

Warn when a bare flow-condition identifier is shadowed by a declared flow variable#14263
os-support-ai merged 3 commits into
mainfrom
claude/issue-14089-flattened-scope-bare-identifier

Conversation

@claude

@claudeclaudeBot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Closes#14089

Implements the maintainer's ruling of 2026-09-01 (director batch #23): option C — warn only on the shadowing sub-case. Options A, B and D were excluded, and the PM's earlier three-way ruling was struck.

What changes

A flow node/edge condition is evaluated in a flattened scope, so a bare status normally resolves to the trigger record's field. That form is correct: @objectstack/formula's published contract says so (ExprSchemaHint.scope, cel-engine.ts), AutomationEngine.seedRunVariables flattens the record's fields for exactly that purpose, and two shipped example apps read fields bare. This PR does not judge a bare identifier for being bare, and adds no error.

It names one sub-case that is both genuinely ambiguous and genuinely broken: a bare name that is BOTH a declared flow variable AND a field on the bound object.

constvariables=this.seedDeclaredVariables(flow,context);// (1) variables firstif(context?.record){variables.set('record',context.record);for(const[k,v]ofObject.entries(context.record)){if(!variables.has(k))variables.set(k,v);// (2) guarded flatten}}

The variable wins, the field is unreachable under its own name, and nothing anywhere reports the collision — on the surface where a wrong predicate is least visible, because a flow condition that never fires produces no record, no error and no log line. Severity is warning; the diagnostic names the mechanism and both repairs.

New module packages/lint/src/flow-variable-scope.ts holds the collection surface and the oracle; validate-expressions.ts gains the two emission call sites (flow-node condition, flow-edge condition) and one collection pass.

The collection surface, measured against the executors

Variables are flow-scoped, not graph-scopedseedRunVariables builds one map per run — so the set is one flat union across every ADR-0031 region collectFlowGraphs yields.

rowdeclaration pointruntime binder
1flow.variables[].nameseedDeclaredVariables
2-5config.iteratorVariable / indexVariable / errorVariable / outputVariableloop, map, try_catch, crud/screen/subflow nodes
6assignment targets, wrapper OBJECT shapelogic-nodes.ts
7assignment targets, wrapper ARRAY shape (variable / name / key)same
8assignment targets, no wrapper at all — the config's own keys ARE the namessame, its else branch
9node ids — a bare CEL root at runtimeevaluateCondition expands a nodeId.outputKey variable into a nested object AT the node id, overwriting whatever scalar was flattened there

Row 8 is gated on the node type, deliberately: it reads every top-level config key, which is correct for an assignment node and catastrophic over-collection for any other. There is a test for that gate.

The rejected itemVariable alias is not read — tolerating a spelling control-flow.zod.ts refuses by name would be consumer-side alias tolerance (Prime Directive 12), and it cannot arrive on the parsed path this rule runs on.

The oracle

firstUndeclaredReference from @objectstack/formula, notcollectCelRootIdentifiers, as the maintainer pinned. The former acts only on cel-js's own unknown-variable fault, so comprehension-macro variables and function names cannot false-positive; the latter reports macro variables as roots, and a macro variable sharing a field's name would then be flagged for a collision that cannot exist. Both are covered by tests.

Its known, deliberate blind spot is pinned rather than left to be discovered: SCOPE_ROOTS members are declared in the strict environment, so a flow variable named result / data / item goes unwarned. That is an under-report, the safe direction for a new warning, and closing it means consulting the AST — which re-opens the macro-variable false positive.

packages/formula is untouched. No new dependency edge: packages/lint already declares @objectstack/formula, and a sibling file in the same package already imports this helper.

Verification — all at d142c49b unless stated

  • pnpm --filter @objectstack/lint test89 files, 2505 tests passed, exit 0 (captured by redirect before any pipe).
  • pnpm --filter @objectstack/lint typecheck — exit 0.
  • Both pinned tests pass unchanged.validate-expressions.test.ts "does NOT flag bare references in a flow condition (flattened scope)" (bare amount / bare stage at zero issues) and formula/src/validate.test.ts's flattened-scope pin are untouched by this diff — neither file's pinned assertions were edited, deleted or re-baselined.
  • Ablation. With both emission call sites deleted, exactly the 6 positive tests go red and every negative control stays green. Mutation confirmed on disk before the run — HEAD blob hash equal to the pre-mutation hash, call-site count 2 to 0, post-mutation hash different — and the restore leg proved by an empty git diff HEAD. No rebuild leg was needed and none is claimed: the test imports ./validate-expressions.js relatively, so vitest reads this package's source, and nothing resolves it through packages/lint/dist.
  • The shipped example apps stay clean, and that is a reading rather than silence.objectstack validate exits 0 on app-todo, app-showcase and app-crm with no shadowing warning. Positive control through the same channel: injecting a status flow variable into app-todo's real task_completion flow (the one whose start condition reads bare status) makes the diagnostic appear, at warning severity with validate still exiting 0. Both legs restored, verified by an empty git diff HEAD.
  • Gate families: 33 derived from the actual diff via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — not a hand-written list. 30 green. 3 NOT MEASURED, none of them a red: check-test-completeness and check:dual-build-cjs-loads first returned exit 3 (PREREQUISITE NOT MET); the latter was then run on the built closure and passed, while the former has no local turbo test log to grade. scripts/pm/check-half-states.mjs is a network-bound GitHub patrol that does not terminate in this container.
  • Ratchets re-run on the final head after the last commit: check:type-check-coverage --re-measure — 27 ledger entries, 1217 raw tsc errors, none above its recorded number, surplus none. Also green at that head: comment-mask-adoption, engine-double-contract, where-matcher, query-options-erasure, type-check-coverage, dual-build-cjs-loads.

Two findings from those gates were repaired rather than routed around, and both are visible in the commit history: the new test file originally carried a private comment-stripper (check:comment-mask-adoption reds on a new one) and now asserts on the module's exported constants instead; and ASSIGNMENT_NODE_TYPE was made module-private after rule-id-barrel-exports correctly read a slug-shaped export const as a rule id owing a barrel line.

Clause 2: no. The accept set does not move in either direction. Nothing that linted clean before this diff lints red after it, and nothing previously rejected is now accepted — the only behavioural change is one additional warning, and a warning never fails the build. The four surfaces the ruling protects are untouched: no contract change, no pinned test deleted or re-baselined, no example app edited, and packages/formula unmodified.

Meta-guard note

validate-expressions.test.ts's #5017 receiver scan gained one PLUMBING entry, scope. It is not a receiver: it is the tail of the './flow-variable-scope.js' import specifier, which the scan cannot tell from a property read — the same artefact as the fields and guards entries already there. No metadata receiver's expected list changed, so the declared-key guard loses no coverage.

Generated by Claude Code


Generated by Claude Code

…clared variable
WIP — implementation + tests + changeset, before the first verified build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
…urce scan
Replaces the private comment-stripper the new test carried (check:comment-mask-adoption
reds on a new one) with assertions over the module's exported constants, which pin the
values the collection walk indexes with rather than the spelling someone typed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
rule-id-barrel-exports reads every slug-shaped `export const` in src/ as a rule id
that a published barrel must carry, and 'assignment' is slug-shaped. It is a node
type, not a rule id; its gate is pinned through behaviour instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5
@github-actionsgithub-actionsBot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 1, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

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

  • content/docs/automation/flows.mdx(via errorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), indexVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), iteratorVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS), outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))
  • content/docs/kernel/runtime-services/examples.mdx(via outputVariable (literal, a string literal in VARIABLE_NAME_CONFIG_KEYS))

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

  • content/docs/releases/v16.mdx(via validateStackExpressions (symbol, a top-level function))

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

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5c539e964928c53fc9b717425818364053cba918 — the merge of head d142c49b9df746f9f39cf500fc6b707cb4e23208 into base e7645078d8680027e1d2b9760dfc686c2cefb8e8, 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 5c539e964928c53fc9b717425818364053cba918 && git checkout 5c539e964928c53fc9b717425818364053cba918
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e7645078d8680027e1d2b9760dfc686c2cefb8e8 d142c49b9df746f9f39cf500fc6b707cb4e23208 && git checkout -B drift-repro e7645078d8680027e1d2b9760dfc686c2cefb8e8 && git merge --no-ff d142c49b9df746f9f39cf500fc6b707cb4e23208
node scripts/docs-audit/affected-docs.mjs --json e7645078d8680027e1d2b9760dfc686c2cefb8e8

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e7645078d8680027e1d2b9760dfc686c2cefb8e8 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-support-ai
os-support-ai marked this pull request as ready for review September 2, 2026 00:59
@os-support-ai
os-support-ai added this pull request to the merge queueSep 2, 2026
Merged via the queue into main with commit 06ee8bfSep 2, 2026
34 checks passed
@os-support-ai
os-support-ai deleted the claude/issue-14089-flattened-scope-bare-identifier branch September 2, 2026 01:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate-expressions has no flow leg for bare identifiers — a bare field reference in a flow condition passes objectstack validate clean

2 participants

@os-support-ai@claude