fix: use remapped variables in cost calculation

[ysmolski]

I have no idea how it slipped but I implemented Cost Control
without any regard to remapped variables.

This PR fixes the situation. It adds an abstraction to deal with remapped
variables. Additionally, I have applied it in the resolver.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59c70a11-73b1-49c3-b0e7-71cac32c2aa0

📥 Commits

Reviewing files that changed from the base of the PR and between 6bdf5cc and a59d00e.

📒 Files selected for processing (2)

• execution/engine/execution_engine.go
• execution/engine/execution_engine_test.go

---

📝 Walkthrough

Walkthrough

PR refactors cost calculation to use a new request-scoped resolve.VariablesView instead of *astjson.Value, threads it through CostCalculator APIs and traversal, updates template variable lookup, computes variable remapping during normalization, and wires the view into request and execution entry points.

Changes

Cost Calculation Variables Abstraction Refactoring

Layer / File(s)	Summary
VariablesView abstraction foundation v2/pkg/engine/resolve/variables_view.go	New VariablesView type encapsulates JSON variables with optional remapping; Get() provides path-based lookup with key remapping and nested traversal, and IsEmpty() detects empty state.
Context helper and integration v2/pkg/engine/resolve/context.go	Add Context.VariablesView() method to construct the view from context variables and remap; docs updated to prefer read-only access.
InputTemplate variable lookup change v2/pkg/engine/resolve/inputtemplate.go	renderContextVariable uses ctx.VariablesView().Get() for lookup, treats only nil as undefined, and removes previous null-special-case branching.
Request and execution integration execution/graphql/request.go, execution/engine/execution_engine.go	Update Request.ComputeEstimatedCost() and ComputeActualCost() to accept resolve.VariablesView, remove astjson import from request, adjust OperationType consts, compute and assign remap during normalization, and obtain varsView in execution flow for validation and cost computation calls.
Execution tests update execution/engine/execution_engine_test.go	Expected upstream GraphQL request payloads in multiple tests changed to use generic variable placeholders ($a, $b) and corresponding variables keys.
CostCalculator API migration v2/pkg/engine/plan/cost.go	Update CostCalculator exported methods (EstimateCost, ActualCost, ValidateSliceArguments, DebugPrint) to accept resolve.VariablesView instead of *astjson.Value; add resolve import.
Cost tree traversal and multiplier implementation v2/pkg/engine/plan/cost.go	Thread VariablesView through FieldListSize methods (multiplier, resolveSlicingArg), extractSlicingArgValue, ArgumentInfo.inputFieldsCost, and CostTreeNode traversal (cost, costsAndMultiplier, maxMultiplierImplementingField) including implementing-type cases.
Slice argument validation and debug printing v2/pkg/engine/plan/cost.go	Update slice-argument validation to resolve arguments via VariablesView.Get(), thread vars through recursive validation, and refactor debug printing to use vars.IsEmpty() and vars.Get(...) when estimating.

Sequence Diagram(s)

sequenceDiagram
  participant ExecutionEngine
  participant Request
  participant CostCalculator
  participant VariablesView

  ExecutionEngine->>Request: build request/context
  Request->>VariablesView: Context.VariablesView()
  ExecutionEngine->>CostCalculator: ValidateSliceArguments(varsView)
  CostCalculator->>VariablesView: Get(path...)
  VariablesView-->>CostCalculator: value
  ExecutionEngine->>CostCalculator: EstimateCost(varsView)
  CostCalculator->>VariablesView: Get(path...)
  VariablesView-->>CostCalculator: value
  ExecutionEngine->>CostCalculator: ActualCost(varsView, ActualListSizes)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• wundergraph/graphql-go-tools#1461: Overlapping changes to v2/pkg/engine/plan/cost.go cost-calculator refactors and argument-cost logic.
• wundergraph/graphql-go-tools#1502: Related edits to cost computation path and list-multiplier behavior in v2/pkg/engine/plan/cost.go.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly addresses the main change: fixing cost calculation to use remapped variables, which is the core issue described in the PR objectives.
Description check	✅ Passed	The description relates to the changeset by explaining the problem (Cost Control ignoring remapped variables) and the solution (adding an abstraction for remapped variables and applying it in the resolver).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch yury/eng-9610-cost-calculation-ignores-remapvariables

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

v2/pkg/engine/resolve/context.go (1)

24-24: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix typo in documentation.

"Recommented" should be "Recommended".

📝 Proposed fix

-	// Recommented read-only use via variables.Set returned by VariableSet().
+	// Recommended read-only use via variables.Set returned by VariableSet().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@v2/pkg/engine/resolve/context.go` at line 24, Update the comment containing
"Recommented read-only use via variables.Set returned by VariableSet()." to
correct the typo: change "Recommented" to "Recommended" so the comment reads
"Recommended read-only use via variables.Set returned by VariableSet()." Locate
this comment near the VariableSet() reference in resolve/context.go and update
the wording accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@v2/pkg/engine/resolve/context.go`:
- Line 24: Update the comment containing "Recommented read-only use via
variables.Set returned by VariableSet()." to correct the typo: change
"Recommented" to "Recommended" so the comment reads "Recommended read-only use
via variables.Set returned by VariableSet()." Locate this comment near the
VariableSet() reference in resolve/context.go and update the wording
accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aa72970e-3b85-47a3-a42a-93ea5fcecd43

📥 Commits

Reviewing files that changed from the base of the PR and between 35c2ca9 and ca40d86.

📒 Files selected for processing (1)

• v2/pkg/engine/resolve/context.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

v2/pkg/engine/plan/cost.go (2)

148-160: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Keep nested variable access inside VariablesView.

Both paths only use vars.Get() for the top-level variable and then descend through raw astjson.Values. That bypasses the nested/path remapping that VariablesView was introduced to provide, so dotted slicing arguments and weighted input-object fields can still be read from pre-remap keys and fall back to defaults/zero cost. This leaves a remapped-variable hole in cost control.

Also applies to: 283-304

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@v2/pkg/engine/plan/cost.go` around lines 148 - 160, The current
extractSlicingArgValue implementation extracts the top-level variable with
vars.Get(arg.varName) then traverses raw astjson.Value fields, bypassing
VariablesView remapping; update extractSlicingArgValue to resolve nested paths
via VariablesView (not by descending astjson.Value) by repeatedly calling
vars.Get on the progressively concatenated path (e.g., arg.varName,
arg.varName+"."+segment, ...) or using the VariablesView API for nested access
so remapping/defaults apply; apply the same change to the similar logic
referenced around the other block (the code at the other occurrence) so both use
VariablesView for nested/dotted access instead of manipulating astjson.Value
directly.

---

826-834: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use one source of truth for debug mode.

The header switches on actualListSizes != nil, but argument rendering switches on vars.IsEmpty(). Those diverge for estimated calls without variables and for actual-cost calls that still carry variables, so the same dump can label itself one way and format args the other way.

Also applies to: 899-907

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 148-160: The current extractSlicingArgValue implementation
extracts the top-level variable with vars.Get(arg.varName) then traverses raw
astjson.Value fields, bypassing VariablesView remapping; update
extractSlicingArgValue to resolve nested paths via VariablesView (not by
descending astjson.Value) by repeatedly calling vars.Get on the progressively
concatenated path (e.g., arg.varName, arg.varName+"."+segment, ...) or using the
VariablesView API for nested access so remapping/defaults apply; apply the same
change to the similar logic referenced around the other block (the code at the
other occurrence) so both use VariablesView for nested/dotted access instead of
manipulating astjson.Value directly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 98ec6368-77fd-413c-b3e6-034589439880

📥 Commits

Reviewing files that changed from the base of the PR and between ca40d86 and 4425b46.

📒 Files selected for processing (6)

• execution/engine/execution_engine.go
• execution/graphql/request.go
• v2/pkg/engine/plan/cost.go
• v2/pkg/engine/resolve/context.go
• v2/pkg/engine/resolve/inputtemplate.go
• v2/pkg/engine/resolve/variables_view.go

[coderabbitai]

🧹 Nitpick comments (1)

v2/pkg/engine/plan/cost.go (1)

904-911: 💤 Low value

Misleading comments: condition doesn't distinguish actual vs estimated mode.

The comments say "actual cost" and "estimated cost", but vars.IsEmpty() doesn't determine the cost mode—that's controlled by actualListSizes != nil (line 830). The condition here determines whether variable values are available for display, not the cost calculation mode.

📝 Suggested comment clarification

 			if arg.hasVariable {
 				if vars.IsEmpty() {
-					// actual cost
+					// variables unavailable, show variable name only
 					argStrs = append(argStrs, fmt.Sprintf("%s=$%s", name, arg.varName))
 				} else {
-					// estimated cost
+					// variables available, show resolved value
 					v := vars.Get(arg.varName)
 					argStrs = append(argStrs, fmt.Sprintf("%s=%s($%s)", name, v, arg.varName))
 				}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@v2/pkg/engine/plan/cost.go` around lines 904 - 911, The comments are
misleading: vars.IsEmpty() checks whether variable values are available for
display, not whether we're in "actual" vs "estimated" cost mode (that is
governed by actualListSizes != nil); update the comments around the argStrs
append in the block using vars.IsEmpty() to state that the branch prints the
variable name with either the raw variable value (when vars has no mapping) or
the variable's resolved value wrapper (when vars provides a value). Refer to
vars.IsEmpty(), vars.Get(arg.varName), arg.varName, actualListSizes and argStrs
to locate and clarify the comment.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@v2/pkg/engine/plan/cost.go`:
- Around line 904-911: The comments are misleading: vars.IsEmpty() checks
whether variable values are available for display, not whether we're in "actual"
vs "estimated" cost mode (that is governed by actualListSizes != nil); update
the comments around the argStrs append in the block using vars.IsEmpty() to
state that the branch prints the variable name with either the raw variable
value (when vars has no mapping) or the variable's resolved value wrapper (when
vars provides a value). Refer to vars.IsEmpty(), vars.Get(arg.varName),
arg.varName, actualListSizes and argStrs to locate and clarify the comment.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 63e2ce83-c74c-4445-bc7a-5d279dd85248

📥 Commits

Reviewing files that changed from the base of the PR and between 4425b46 and 93102fe.

📒 Files selected for processing (1)

• v2/pkg/engine/plan/cost.go