fix(gfql): nested let() scope isolation (#968)#969
Open
Conversation
…968) The dependency resolver walked into nested ASTLet bindings and surfaced their internal refs as dependencies of the outer DAG, causing validate_dependencies() to reject valid nested-let patterns with "references undefined nodes" errors. Now extract_dependencies() treats nested ASTLet as an opaque execution unit with zero external deps. - Fix: 2-line change in chain_let.py extract_dependencies() - Tests: 8 new tests (unit + integration) for nested let scope isolation - Docs: added nested let scope rules to wire protocol spec and LLM guide - Changelog: entry under [Development] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 10 more tests covering edge cases discovered during amplification:
- Three-level nesting (let in let in let)
- Sibling inner lets with duplicate binding names (no collision)
- Empty inner let (degenerate case)
- Diamond pattern on nested let result (multiple outer refs)
- Inner let reads outer binding via shared execution context
- Outer ref to inner binding correctly fails
- JSON round-trip (to_json / from_json) for nested let
- Dict dispatch (g.gfql({type: Let, ...})) for nested let
- extract_dependencies with ASTRef wrapping nested let
- extract_dependencies with Chain containing nested let
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The shared ExecutionContext caused three runtime bugs: 1. Inner binding names leaked into outer scope (sibling name collision) 2. Inner bindings shadowing outer names corrupted the outer value 3. Nested let received accumulated_result instead of original graph Fix: - Add _ChildExecutionContext with read-through (lexical closure) and write-local semantics via ExecutionContext.child_context() - execute_node() creates a child context for nested ASTLet execution - Nested let receives the original graph (not accumulated_result), matching Chain/Node behavior in the outer scope Tests: comprehensive 31-test semantic suite covering: A. Dependency graph (5 unit tests) B. Positive execution (7 integration tests) C. Lexical scoping / closure reads (2 tests) D. Scope isolation / no upward leakage (4 tests) E. Shadowing without corruption (3 tests) F. Sequencing / execution order (4 tests) G. Negative / must-fail patterns (2 tests) H. Serialization round-trip (2 tests) I. ExecutionContext child_context API (4 unit tests) Docs: updated wire protocol spec and LLM guide with full lexical scope rules (read-through, write-local, shadowing, sibling isolation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Audit findings addressed:
- Extracted _PERSONS/_ASSETS/_ALL_NODES class constants and
_assert_persons/_assert_assets helpers to eliminate ~20 repeated
Chain([n({type: X})]) expressions and all()/set() assertion patterns
- Fixed 10+ vacuous `all(empty_series)` assertions that silently passed
on empty DataFrames — all now use len() + set() checks
- Removed duplicate test (D.2 was structurally identical to D.1)
- Removed duplicate test (F.1 was identical to C.1)
- Added test_child_context_get_all_bindings_merges for merge semantics
- Simplified test_nested_let_validate_then_execute_agreement to avoid
fragile edge traversal that returned empty results
Net: 29 focused tests (was 31 with duplicates), all non-vacuous.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace mutable class-level Chain singletons (_PERSONS, _ASSETS, _ALL_NODES) with factory methods (_persons(), _assets(), _all_nodes()) to prevent cross-test mutation - Remove test_nested_let_validate_then_execute_agreement (identical to test_nested_let_executes_as_opaque_unit after simplification — ASTLet already calls validate() in __init__) - Remove test_inner_reads_outer_binding (subset of test_inner_reads_outer_leaf_binding in section F) Net: 27 focused tests, no mutable shared state, no duplicates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #968 — nested let() inside let() scope isolation.
Three layers of fix, stacked on this branch:
1. Dependency graph fix (commit 1)
extract_dependencies() now treats nested ASTLet as opaque (zero external deps). Previously it recursed into inner bindings, causing validate_dependencies() to reject valid nested-let patterns with "references undefined nodes".
2. Runtime context isolation (commit 3)
Added _ChildExecutionContext with read-through / write-local semantics:
3. Docs
Wire protocol spec and LLM guide updated with full lexical scope rules.
Test plan