Fix QRY037: CTE chains with captured inner and outer parameters (#305)

_sessions/305-fix-cte-outer-param-assignment/_issue-f5.md

@@ -0,0 +1,21 @@
+## Description
+The "resolve a CteDefinition site to its CteDef" lookup — `CteNameHelpers.ExtractShortName(site.Bound.Raw.CteEntityTypeName ?? site.EntityTypeName)` compared against `CteDef.Name`, first match wins — is now duplicated in four places. `CteDef.cs`'s doc contract ("both sides MUST use the same helper so the names compare equal under all input forms") is enforced only by convention; a shared lookup helper would give it a single enforcement point.
+
+## Location
+- `src/Quarry.Generator/IR/AssembledPlan.cs` — `GetCteInnerParamCount` (added in #306)
+- `src/Quarry.Generator/CodeGen/CarrierAnalyzer.cs` — `BuildLambdaInnerExtractionPlan` (~line 456)
+- `src/Quarry.Generator/CodeGen/TransitionBodyEmitter.cs` — `EmitCteDefinition`, twice (~lines 173–221)
+- Contract doc: `src/Quarry.Generator/IR/CteDef.cs` (`CteNameHelpers` remarks, ~lines 121–128)
+
+## Diagnostics
+None — this is a consolidation/maintainability cleanup, not a defect. Flagged as review finding F5 (severity L) during #306's review pass.
+
+## What Has Been Tried
+Nothing yet; the duplication was deliberately kept in #306 to mirror the established pattern rather than widen that PR's scope.
+
+## Gathered Information
+- All four sites use identical semantics: short-name match, first match wins.
+- Duplicate CTE names are rejected at compile time by QRY082, and failed-analysis CTEs produce no CteDef (chain already carries QRY080), so first-match/no-match semantics are uniform across call sites.
+
+## Suggested Approach
+Add a static helper next to `CteNameHelpers` (e.g. `CteNameHelpers.FindCteDef(TranslatedCallSite site, IReadOnlyList<CteDef> cteDefinitions)` or an overload taking the raw type name) and route all four call sites through it. Pure refactor — behavior must be byte-identical; the existing CTE test suite (CrossDialectCteTests, LambdaCteTests, MySqlBindOrderGenerationTests, CarrierGenerationTests CTE regions) is the regression gate.

_sessions/305-fix-cte-outer-param-assignment/_pr-body.md

@@ -0,0 +1,38 @@
+## Summary
+- Closes #305
+- Fixes the QRY037 build failure for CTE chains that combine a captured parameter inside the `With<T>(...)` lambda with a captured parameter in an outer clause (`Where` after `FromCte<T>()`).
+
+## Reason for Change
+The inner+outer parameterized CTE shape — valid SQL on all four dialects — was unbuildable: the generated carrier declared the outer parameter's field (`P1`) but no clause interceptor assigned it, tripping the generator's QRY037 self-check.
+
+**Root cause** (differs from the issue's suspicion): `ChainAnalyzer`'s parameter rebasing and `SqlAssembler`'s placeholder rebasing are both correct. The defect was in `AssembledPlan.BuildSiteParamsMap` / `BuildParamConditionalMap`: both walk clause entries accumulating the global parameter offset each emitter uses to route `__c.P{n}` assignments, but a `CteDefinition` entry (whose `Clause` is null — it is not clause-bearing) advanced the offset by 0 despite owning `InnerParameters.Count` slots in the chain's parameter list. Every param-bearing clause after a param-bearing `With()` therefore resolved its carrier P-fields short by the inner-param count: the outer `Where` wrote `P0` (the CTE's slot) and `P1` was never assigned. With zero inner params the skew is zero — exactly why inner-only, two-inner, and outer-only chains all worked.
+
+**Fix**: advance the offset by the matching `CteDef.InnerParameters.Count`, matched by CTE short name via `CteNameHelpers.ExtractShortName` (the same first-match rule as `TransitionBodyEmitter.EmitCteDefinition`; duplicate names are already QRY082 errors, failed-analysis CTEs already QRY080). All emitter paths funnel through this one map, so the single fix point covers every consumer.
+
+## Impact
+- Inner+outer parameterized CTE chains (single- and multi-CTE) now build and bind correctly on all four dialects.
+- No behavior change for any previously-buildable chain: the new branch fires only for `CteDefinition` entries, and every shape whose offsets would differ was a hard QRY037 build error before.
+- Also closes the #303 audit gap: the inner+outer combination was the one parameterized-CTE shape whose MySQL inner-before-outer positional bind order could not be execution-verified.
+
+## Plan items implemented as specified
+1. `AssembledPlan` offset-walk fix + generation guard (`CteInnerAndOuterCapturedParams_NoQRY037_AssignsBothPFields`) — verified to fail without the fix.
+2. Cross-dialect SQL + execution test (`Cte_FromCte_InnerAndOuterCapturedParams`) — exact WITH rendering and row verification on all four backends.
+3. MySQL bind-order execution pin (`ParameterizedCteInnerAndOuterParams_OnMySQL_BindsInnerBeforeOuter`), exactly as specified in #305, plus retirement of the QRY037 caveats in the sibling `ParameterizedCte*` pins.
+
+## Deviations from plan implemented
+- The planned "inline branch" in both walks was refined into a shared `GetCteInnerParamCount` helper called from an inline branch in each walk (review F1, dismissed as benign — behavior identical, duplication reduced).
+
+## Gaps in original plan implemented
+Review findings addressed post-implementation:
+- **F2 (M)**: `ConditionalOuterClause_AfterParameterizedCte_MySQL_NoQRY048` — the only shape that observes the `BuildParamConditionalMap` half of the fix (a conditional outer clause after a parameterized CTE on MySQL, newly reachable). Verified to fail when only that branch is removed.
+- **F3 (L)**: `Cte_TwoChainedWiths_CapturedParams_AndOuterCapturedParam` — cumulative offset accumulation across two param-bearing CTEs plus an outer captured param, on all four dialects.
+- **F4 (L)**: generation guard tightened to exact-count P0/P1 assertions (the pre-fix stomp signature was two P0 assignments and zero P1).
+
+Deferred (review F5, class C, pending confirmation): consolidating the CteDef-by-short-name first-match lookup (now present in `AssembledPlan`, `CarrierAnalyzer`, and twice in `TransitionBodyEmitter`) into a single shared helper — a cleanup, not a defect.
+
+## Performance Considerations
+The added lookup is a short linear scan over `Plan.CteDefinitions` (almost always 0–3 entries) inside two already-cached map builders — no measurable generator cost.
+
+## Breaking Changes
+- Consumer-facing: none (previously-failing shapes now build; no existing SQL or binding changes — manifest goldens show additions only).
+- Internal: none (private helpers only; no signature changes).

_sessions/305-fix-cte-outer-param-assignment/plan.md

@@ -0,0 +1,99 @@
+# Plan: 305-fix-cte-outer-param-assignment
+
+## Background
+
+A CTE chain combining a captured param inside `With<T>(...)` with a captured param in an
+outer clause fails the build with QRY037: the carrier declares the outer param's field
+(`P1`) but no interceptor body assigns it.
+
+The chain's parameter model is correct end-to-end: `ChainAnalyzer` places CTE inner params
+at global slots `[CteDef.ParameterOffset, ParameterOffset + InnerParameters.Count)` and
+remaps outer clause params to follow them; `SqlAssembler` rebases inner placeholder
+rendering with `paramBaseOffset: cte.ParameterOffset`; the carrier declares one `P{n}`
+field per global slot. The defect is confined to `AssembledPlan`'s cached site-parameter
+walk, which every emitter consults to decide *which* `P{n}` a clause interceptor assigns.
+
+## Key concept: the offset walk
+
+`AssembledPlan.BuildSiteParamsMap()` iterates `GetClauseEntries()` in chain order,
+maintaining `globalParamOffset` — the first global slot owned by the current site. Each
+entry advances the offset by the number of slots it owns (translated clause params,
+UpdateSetPoco columns, set-operation operand params, UpdateSetAction params, Select
+projection params). A `CteDefinition` entry owns `InnerParameters.Count` slots but has
+`Clause == null` (CteDefinition is not clause-bearing in `CallSiteTranslator`), so it
+falls through every case and advances the offset by 0. Every param-bearing clause after
+it then receives an offset short by the total inner-param count, and
+`CarrierEmitter.EmitCarrierClauseBody` emits `__c.P{offset+i} = ...` against the wrong
+slots: the outer Where writes `P0` (stomping the CTE copy) and `P1` stays unassigned →
+QRY037. Zero inner params ⇒ zero offset error ⇒ why inner-only / outer-only / two-inner
+shapes all work.
+
+`BuildParamConditionalMap()` walks the same way via `GetClauseParamCount` and has the
+identical gap. Its consumer (`PipelineOrchestrator.RewriteMySqlBindMarkers`) treats
+missing keys as unconditional/active, so CTE slots need no map entries — only the offset
+must advance so post-CTE clause params are keyed at their true slots.
+
+## Algorithm: the fix
+
+In both walks, add a `CteDefinition` case that advances the offset by the matching
+`CteDef.InnerParameters.Count`, matched by CTE short name — the same first-match-by-name
+rule `TransitionBodyEmitter.EmitCteDefinition` uses (duplicate names are already compile
+errors via QRY082; a failed-analysis CTE has no CteDef and the chain is already a QRY080
+error, so advancing 0 there is consistent):
+
+```csharp
+else if (clause.Site.Kind == Models.InterceptorKind.CteDefinition)
+{
+    var cteName = CteNameHelpers.ExtractShortName(
+        clause.Site.Bound.Raw.CteEntityTypeName ?? clause.Site.EntityTypeName);
+    foreach (var cte in Plan.CteDefinitions)
+    {
+        if (cte.Name == cteName)
+        {
+            globalParamOffset += cte.InnerParameters.Count;
+            break;
+        }
+    }
+}
+```
+
+Placement: before the generic `clause.Site.Clause != null` case is not required
+(CteDefinition sites always have null Clause) but the branch order must keep the existing
+cases untouched. `BuildParamConditionalMap` needs the same branch inline in its loop
+(alongside the existing set-operation `continue` case) because `GetClauseParamCount` is
+static and has no access to `Plan.CteDefinitions`.
+
+## Steps
+
+- [x] **Step 1 — Fix `AssembledPlan` offset walk + generation guard test.**
+  Modify `src/Quarry.Generator/IR/AssembledPlan.cs`: add the CteDefinition advance to
+  `BuildSiteParamsMap` and `BuildParamConditionalMap` (as an inline branch mirroring the
+  set-op handling). Add a generation test in
+  `src/Quarry.Tests/Generation/CarrierGenerationTests.cs` (QRY037 region): compile the
+  issue #305 chain shape (captured inner + captured outer param) via the real generator
+  pipeline; assert no QRY037 diagnostic and that the generated interceptor source assigns
+  both `__c.P0` (CTE copy in the With interceptor) and `__c.P1` (outer Where interceptor).
+  Run the full Quarry.Tests suite. Commit.
+  Tests: new generation test; existing suite green (esp. `CrossDialectCteTests`,
+  `MySqlBindOrderGenerationTests`, QRY037 self-check tests).
+
+- [x] **Step 2 — Cross-dialect SQL + execution test.**
+  Add to `src/Quarry.Tests/SqlOutput/CrossDialectCteTests.cs`: inner+outer captured
+  params (`With<Order>(orders => orders.Where(o => o.Total > threshold)).FromCte<Order>()
+  .Where(o => o.OrderId >= minId).Select(...)`) built on all four dialects with
+  `Prepare()`, `AssertDialects` on the rendered WITH statement (`@p0/@p1`, `$1/$2`, `?/?`,
+  `@p0/@p1`), then execute-and-verify rows on all four backends (expect only order 3:
+  `(3, 150.00m)`). Run suite. Commit.
+  Tests: the new cross-dialect test itself.
+
+- [x] **Step 3 — MySQL bind-order execution pin.**
+  Add `ParameterizedCteInnerAndOuterParams_OnMySQL_BindsInnerBeforeOuter` to
+  `src/Quarry.Tests/Integration/MySqlIntegrationTests.cs` exactly as specified in issue
+  #305 (companion to the three #304 `ParameterizedCte*` pins; same seed data). Also update
+  the now-stale comments in `ParameterizedCteFilter_OnMySQL_BindsInnerParamsBeforeOuter` /
+  `ParameterizedCteTwoInnerParams_OnMySQL_BindsBothInTextOrder` that describe QRY037 as
+  blocking this shape. Run suite. Commit.
+  Tests: the new MySQL integration test.
+
+Dependencies: Step 1 unblocks 2 and 3 (the shape doesn't build before the fix). Steps 2
+and 3 are independent of each other.

_sessions/305-fix-cte-outer-param-assignment/review.md

@@ -0,0 +1,49 @@
+# Review: 305-fix-cte-outer-param-assignment
+
+## Classifications
+
+| ID | Class | Rec | Sev | Section | Finding | Action Taken |
+|----|-------|-----|-----|---------|---------|--------------|
+| F1 | D | D | L | Plan Compliance | Plan said "inline branch"; implementation factored counting into shared `GetCteInnerParamCount` helper — benign, behavior identical | dismissed (benign refinement, noted for the record) |
+| F2 | B | B | M | Test Quality | Conditional-map half of fix has no alignment-observing test (conditional outer clause after parameterized CTE on MySQL is newly reachable, untested) | Added ConditionalOuterClause_AfterParameterizedCte_MySQL_NoQRY048 (MySqlBindOrderGenerationTests) — verified it fails when only the conditional-map branch is removed |
+| F3 | B | B | L | Test Quality | No multi-CTE (two param-bearing Withs) + outer captured param test — cumulative offset accumulation uncovered | Added Cte_TwoChainedWiths_CapturedParams_AndOuterCapturedParam (CrossDialectCteTests) — 2 param CTEs + outer param, SQL + execution on all 4 dialects |
+| F4 | B | B | L | Test Quality | Generation test regexes match anywhere in file; wrong-interceptor assignment would still pass | Tightened CteInnerAndOuterCapturedParams_NoQRY037_AssignsBothPFields to exact-count assertions (1x P0, 1x P1) |
+| F5 | C | C | L | Codebase Consistency | `GetCteInnerParamCount` is the 3rd/4th copy of the CteDef-by-short-name first-match loop — consolidation opportunity | issue creation pending user confirmation (outward-facing); to file: consolidate CteDef-by-name lookup into shared helper |
+
+## Plan Compliance
+
+| ID | Finding | Sev | Why It Matters |
+|----|---------|-----|----------------|
+| F1 | Plan specified the CteDefinition advance "inline" in both walks (BuildParamConditionalMap "needs the same branch inline in its loop... because GetClauseParamCount is static"); the implementation instead factors the name-match lookup into a new private instance helper `GetCteInnerParamCount` (AssembledPlan.cs:291) called from an inline branch in each walk. | L | Benign deviation — the branch is still inline per walk and only the counting is shared; behavior is identical to the plan's snippet and duplication is reduced. Noted for the record only. All three plan steps otherwise match exactly: fix + QRY037-region generation test (CarrierGenerationTests.cs:4489, inside the QRY037 region at 4349–4662), the four-dialect Prepare/AssertDialects/execute test with the specified placeholder shapes and `(3, 150.00m)` expectation, the MySQL pin named exactly as planned, and both stale QRY037 comments updated. No scope creep. |
+
+## Correctness
+
+No concerns.
+
+(Verified: CteDefinition maps to a ClauseRole so its entries do appear in `GetClauseEntries()` and the new branches are reachable; branch placement before `Clause != null` is safe because CteDefinition sites always have null Clause and cannot match the set-op or UpdateSetPoco cases; multi-CTE chains accumulate correctly because clause entries walk in chain order and ChainAnalyzer assigns each `CteDef.ParameterOffset` in the same order; zero-inner-param CTEs advance 0 (unchanged behavior); a failed-analysis CTE has no CteDef, `GetCteInnerParamCount` returns 0, and the chain already carries QRY080; duplicate names are first-match, consistent with TransitionBodyEmitter.EmitCteDefinition:199-203 and already rejected by QRY082; `ExtractShortName(null)` returns null which never equals a non-null `CteDef.Name`, so no NRE; `CteEntityTypeName` is always populated for With<T> sites (UsageSiteDiscovery.cs:3784-3787, With is always generic); `RewriteMySqlBindMarkers` (PipelineOrchestrator.cs:641) treats missing conditional-map keys as active, matching the skip-without-entries choice.)
+
+## Security
+
+No concerns.
+
+(The change emits no new code or SQL text; it only realigns integer offsets computed from generator-internal metadata. CTE names derive from C# type identifiers already constrained by the compiler and handled by the pre-existing quoting/emission paths.)
+
+## Test Quality
+
+| ID | Finding | Sev | Why It Matters |
+|----|---------|-----|----------------|
+| F2 | The `BuildParamConditionalMap` half of the fix is only structurally exercised (the branch runs for MySQL CTE chains via `RewriteMySqlBindMarkers`), but no test observes its key *alignment*: with only unconditional clauses, every param is treated as active regardless of map keys, so a misaligned offset is invisible. The shape that would observe it — a conditional outer clause (`if (x) q = q.Where(...)`) after a parameterized CTE on MySQL — was previously unreachable (QRY037) and is now newly buildable, yet remains untested. | M | A future regression in this walk would silently mislabel slots and surface only as a hard-to-diagnose QRY048/identity-bind failure (or wrong active-set validation) for conditional+CTE chains on MySQL. The plan's own working notes flagged this combination as "must be fixed consistently"; a pin for the newly reachable shape would close the loop. |
+| F3 | No test covers a multi-CTE chain (two `With<T>` with inner params each) combined with an outer captured param — the shape that exercises *cumulative* offset accumulation across multiple CteDefinition entries. Existing `LambdaCte_TwoChainedWiths_CapturedParams` (LambdaCteTests.cs:104) has no outer param; the new tests cover a single CTE only. | L | The per-entry advance makes cumulative correctness nearly self-evident, but multi-CTE was listed as a design boundary condition and this newly-enabled shape (three global slots: two inner + one outer) has zero coverage on any dialect. |
+| F4 | The generation test's regex assertions (`__c\.P0\s*=`, `__c\.P1\s*=`) match anywhere in the interceptor file, not scoped to the With/Where interceptor bodies respectively, and are largely implied by the QRY037-absence assertion (the self-check already guarantees every declared P-field is assigned somewhere). A regression that assigns P1 from the wrong interceptor would pass this test. | L | Low residual risk in practice — the cross-dialect and MySQL execution tests catch slot-value swaps at runtime — but the generation test's stated intent ("the outer Where interceptor must assign... not stomp P0") is stronger than what it asserts. |
+
+## Codebase Consistency
+
+| ID | Finding | Sev | Why It Matters |
+|----|---------|-----|----------------|
+| F5 | `GetCteInnerParamCount` is the third/fourth copy of the "resolve a CteDefinition site to its CteDef by `ExtractShortName(CteEntityTypeName ?? EntityTypeName)` first-match" loop — the same pattern exists in CarrierAnalyzer.BuildLambdaInnerExtractionPlan (CarrierAnalyzer.cs:456-462) and twice in TransitionBodyEmitter.EmitCteDefinition (TransitionBodyEmitter.cs:173-221). | L | The duplication faithfully follows the established repo pattern (and the fix's comments cross-reference it), but a shared `FindCteDef(site, cteDefinitions)` helper next to `CteNameHelpers` would give the "MUST use the same helper" doc contract in CteDef.cs:121-128 a single enforcement point. Purely a consolidation opportunity, not a defect. Otherwise the change is idiomatic: the new branch mirrors the adjacent `setOpIndex` handling shape, comment style/density matches the file, and the conditional-map branch uses the same `continue` pattern as the set-op case. |
+
+## Integration / Breaking Changes
+
+No concerns.
+
+(The change is purely additive in effect: the new branch fires only for CteDefinition entries, and for every previously-buildable chain shape the computed offsets are unchanged — chains with zero-inner-param CTEs advance by 0 as before, and all shapes where the offset would differ were hard build errors (QRY037) on master. Manifest golden churn is exactly the expected additions — one new `With(...).FromCte(...).Where(...).Select(...)` Prepare entry per dialect from CrossDialectCteTests plus one MySQL ExecuteFetchAllAsync entry from the integration pin, with counts advancing accordingly (+2 MySQL, +1 elsewhere) and no modifications to any existing rendered SQL. No public API surface changed.)