feat: add new visitor for inline arguments validation - #1577
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds inline-argument detection and response extension output, and refactors cost-control collection plus actual cost computation across resolve and execution. It also updates cost-related tests, changelog entries, and release version metadata. ChangesInline Arguments Rule
Cost Control and Actual Costing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
v2/pkg/astnormalization/inline_arguments_test.go (1)
46-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for validator reuse across multiple operations.
All current tests construct a fresh
InlineArgumentsValidatorper case, so the accumulation-across-runs behavior flagged ininline_arguments.go(findings/state not cleared automatically betweenNormalizeOperationcalls) isn't exercised here. A test reusing the same validator for two sequential operations would surface whetherFindingscorrectly resets or leaks.🤖 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/astnormalization/inline_arguments_test.go` around lines 46 - 155, Add a test in TestInlineArgumentsRule_Detection that reuses the same InlineArgumentsValidator across two sequential NormalizeOperation calls to verify Findings/state do not accumulate between runs. Use the existing runInlineArgumentsRule helper or InlineArgumentsValidator directly, and assert the second operation’s findings only reflect that operation. Reference InlineArgumentsValidator, NormalizeOperation, and Findings to locate the behavior in inline_arguments.go.
🤖 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.
Inline comments:
In `@v2/pkg/astnormalization/inline_arguments.go`:
- Around line 71-75: The inlineArgumentsVisitor struct has embedded-field
spacing that violates the linter rule. Update the inlineArgumentsVisitor
declaration by adding a blank line after the embedded *astvisitor.Walker field
and before the regular fields operation, definition, and validator so the struct
layout matches the expected grouping.
- Around line 38-50: The InlineArgumentsValidator is retaining stale Findings
across reused document walks, so NormalizeOperation can report results from
previous operations; update the validation flow around InlineArgumentsValidator
and OperationNormalizer to clear Findings at the start of each walk instead of
relying on callers to invoke ClearFindings manually. Ensure the reset happens in
the same path that triggers EnterArgument/HadInlineArguments so each document
gets isolated results, and avoid sharing a single validator instance across
concurrent walks unless access is synchronized.
---
Nitpick comments:
In `@v2/pkg/astnormalization/inline_arguments_test.go`:
- Around line 46-155: Add a test in TestInlineArgumentsRule_Detection that
reuses the same InlineArgumentsValidator across two sequential
NormalizeOperation calls to verify Findings/state do not accumulate between
runs. Use the existing runInlineArgumentsRule helper or InlineArgumentsValidator
directly, and assert the second operation’s findings only reflect that
operation. Reference InlineArgumentsValidator, NormalizeOperation, and Findings
to locate the behavior in inline_arguments.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bcdea2d-3f98-4624-9c99-09cf3a422029
📒 Files selected for processing (2)
v2/pkg/astnormalization/inline_arguments.gov2/pkg/astnormalization/inline_arguments_test.go
There was a problem hiding this comment.
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)
285-307: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
clone()doesn't deep-copyInlineArguments— shared backing array risk.
Files,RenameTypeNames,RemapVariables, andsubgraphErrorsare all deep-copied inclone(), but the newInlineArgumentsslice is left as a shallow copy fromcpy := *c. Cloned contexts (e.g., per-subgraph fetch contexts) will share the same backing array as the parent, so appends on one side can race with or unexpectedly mutate the other.🔧 Proposed fix
cpy.Files = append([]*httpclient.FileUpload(nil), c.Files...) cpy.Request.Header = c.Request.Header.Clone() cpy.RenameTypeNames = append([]RenameTypeName(nil), c.RenameTypeNames...) + cpy.InlineArguments = append([]string(nil), c.InlineArguments...)🤖 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` around lines 285 - 307, The clone() method in Context is still shallow-copying InlineArguments via cpy := *c, which can leave cloned contexts sharing the same backing array. Update clone() to explicitly deep-copy InlineArguments the same way Files and RenameTypeNames are copied, so per-clone mutations in Context do not affect the original or sibling clones.
♻️ Duplicate comments (1)
v2/pkg/astnormalization/inline_arguments.go (1)
38-51: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStale
Findingsacross reused validator — still unresolved.
OperationNormalizer/InlineArgumentsValidatoris intended for reuse acrossNormalizeOperationcalls in hot paths, butFindingsis only cleared via the manualClearFindings()method. IfInlineArgumentsRule(validator)is registered once and the normalizer reused, findings from prior operations leak into subsequent log-only reports unless every caller remembers to clear them explicitly. Concurrent use of the same validator instance also races on theappendinEnterArgument.This was flagged previously and does not appear to have been addressed — the struct and its
ClearFindings/HadInlineArgumentsAPI are unchanged from the earlier review round.Consider clearing
Findingsat the start of each walk (e.g., inEnterDocument) rather than relying on external callers, and document/enforce non-concurrent use of a shared validator.🤖 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/astnormalization/inline_arguments.go` around lines 38 - 51, The shared InlineArgumentsValidator still retains stale Findings between NormalizeOperation runs and is unsafe when reused concurrently. Update InlineArgumentsValidator so Findings is reset automatically at the start of each traversal, ideally in EnterDocument, rather than relying on external ClearFindings calls, and keep HadInlineArguments reporting only the current walk’s state. Also make the intended non-concurrent use of a reused validator explicit in the InlineArgumentsRule/OperationNormalizer usage or comments so callers do not share one instance across simultaneous walks.
🤖 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`:
- Around line 285-307: The clone() method in Context is still shallow-copying
InlineArguments via cpy := *c, which can leave cloned contexts sharing the same
backing array. Update clone() to explicitly deep-copy InlineArguments the same
way Files and RenameTypeNames are copied, so per-clone mutations in Context do
not affect the original or sibling clones.
---
Duplicate comments:
In `@v2/pkg/astnormalization/inline_arguments.go`:
- Around line 38-51: The shared InlineArgumentsValidator still retains stale
Findings between NormalizeOperation runs and is unsafe when reused concurrently.
Update InlineArgumentsValidator so Findings is reset automatically at the start
of each traversal, ideally in EnterDocument, rather than relying on external
ClearFindings calls, and keep HadInlineArguments reporting only the current
walk’s state. Also make the intended non-concurrent use of a reused validator
explicit in the InlineArgumentsRule/OperationNormalizer usage or comments so
callers do not share one instance across simultaneous walks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11a84895-49c5-407e-a97f-5c44fb489527
📒 Files selected for processing (5)
v2/pkg/astnormalization/inline_arguments.gov2/pkg/astnormalization/inline_arguments_test.gov2/pkg/engine/resolve/const.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/resolvable.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/resolve/const.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/astnormalization/inline_arguments_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/extensions_test.go (1)
154-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused no-op authorizer adds noise.
The
authorizerat Lines 156-160 always returnsnil, nil(never denies) and isn't reflected in the expected output's extensions. It doesn't appear necessary to exercise the rate-limit-deny + inline-arguments interaction.♻️ Simplify by dropping the no-op authorizer
t.Run("rate limit deny & inline arguments", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) { - authorizer := createTestAuthorizer(func(ctx *Context, dataSourceID string, input json.RawMessage, coordinate GraphCoordinate) (result *AuthorizationDeny, err error) { - return nil, nil - }, func(ctx *Context, dataSourceID string, object json.RawMessage, coordinate GraphCoordinate) (result *AuthorizationDeny, err error) { - return nil, nil - }) - limiter := &testRateLimiter{ policy: "policy", allowed: 0, allowFn: func(ctx *Context, info *FetchInfo, input json.RawMessage) (*RateLimitDeny, error) { return &RateLimitDeny{Reason: "rate limit exceeded"}, nil }, } res := generateTestFederationGraphQLResponse(t, ctrl) resolveCtx := NewContext(context.Background()) - resolveCtx.authorizer = authorizer resolveCtx.rateLimiter = limiterSeparately, consider adding coverage for InlineArguments combined with an authorization deny, and for the resolver context cleanup path (clearing
InlineArgumentsbetween requests) mentioned in the PR summary — not present in this file.🤖 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/extensions_test.go` around lines 154 - 180, The test case in testFnWithPostEvaluation is carrying an unused no-op authorizer that never denies and does not affect the expected response, so remove the authorizer setup and the resolveCtx.authorizer assignment from the “rate limit deny & inline arguments” scenario. Keep the focus on NewContext, rate limiter, RateLimitOptions, and InlineArguments so the test only covers the intended behavior, and consider adding separate coverage elsewhere for InlineArguments plus an authorization deny and for context cleanup between requests.
🤖 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/resolve/extensions_test.go`:
- Around line 154-180: The test case in testFnWithPostEvaluation is carrying an
unused no-op authorizer that never denies and does not affect the expected
response, so remove the authorizer setup and the resolveCtx.authorizer
assignment from the “rate limit deny & inline arguments” scenario. Keep the
focus on NewContext, rate limiter, RateLimitOptions, and InlineArguments so the
test only covers the intended behavior, and consider adding separate coverage
elsewhere for InlineArguments plus an authorization deny and for context cleanup
between requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 676a45ef-436f-4344-b53d-cb9d373dc13b
📒 Files selected for processing (1)
v2/pkg/engine/resolve/extensions_test.go
This fix includes basic and often used cases, but it does not include some combinations of abstract fields and fragments. For those I have included the test cases (disabled right now). I have simplified how parents are used in calculations. I have added children multiplier to distiungish them from the field cost which is always multiplied even when null was returned on that field. This solution can be sophisticated further if there is a need for it. Potentially, I could take different approach of walking the response from subgraphs and match them with Cost Tree. That would remove many heuristics and exceptions and make things much more simple. But that would happen in a separate PR. `typeNameStats` is populated only when CC is enabled.
🤖 I have created a release *beep* *boop* --- ## [2.9.1](v2.9.0...v2.9.1) (2026-07-07) ### Bug Fixes * do not charge children of null-parents ([#1574](#1574)) ([cf436ec](cf436ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [1.17.0](execution/v1.16.0...execution/v1.17.0) (2026-07-07) ### Features * add defer support part 4 ([#1547](#1547)) ([8891a0e](8891a0e)) ### Bug Fixes * do not charge children of null-parents ([#1574](#1574)) ([cf436ec](cf436ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/plan/cost.go (1)
794-847: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the ACTUAL-mode multiplier adjustment into a helper.
This block (list-size averaging, non-list child discounting, fragment exclusion, and abstract type-share narrowing) is dense and was flagged as high complexity. Splitting it into a small helper (e.g.,
node.adjustActualMultiplier(input, parentStats)) would make each branch easier to follow and test in isolation, without changing behavior.🤖 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 794 - 847, The ACTUAL-mode multiplier adjustment in the cost calculation is too dense and should be split into a helper for readability and testability. Extract the branching logic from the multiplier block in the cost adjustment flow into a focused helper such as a method on the node or a small function near cost.go that handles list-size averaging, non-list child discounting, fragment-field exclusion, and abstract-type share narrowing. Keep the existing behavior unchanged by preserving the current use of node.parent, node.returnsListType, node.returnsSimpleType, node.returnsAbstractType, node.isEnclosingTypeAbstract, and input.typeStats while moving the logic into the helper.
🤖 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 794-847: The ACTUAL-mode multiplier adjustment in the cost
calculation is too dense and should be split into a helper for readability and
testability. Extract the branching logic from the multiplier block in the cost
adjustment flow into a focused helper such as a method on the node or a small
function near cost.go that handles list-size averaging, non-list child
discounting, fragment-field exclusion, and abstract-type share narrowing. Keep
the existing behavior unchanged by preserving the current use of node.parent,
node.returnsListType, node.returnsSimpleType, node.returnsAbstractType,
node.isEnclosingTypeAbstract, and input.typeStats while moving the logic into
the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cadd52f9-eef7-4dc9-8e57-d10b7c44b572
📒 Files selected for processing (8)
execution/CHANGELOG.mdexecution/engine/execution_engine_cost_test.goexecution/engine/execution_engine_test.goexecution/graphql/request.gorelease-please-manifest.jsonv2/CHANGELOG.mdv2/pkg/engine/plan/cost.gov2/pkg/engine/resolve/resolvable.go
✅ Files skipped from review due to trivial changes (2)
- release-please-manifest.json
- v2/CHANGELOG.md
…uterengine-force-use-of-variables # Conflicts: # v2/pkg/engine/resolve/context.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
🤖 I have created a release *beep* *boop* --- ## [2.12.0](v2.11.0...v2.12.0) (2026-07-15) ### Features * add new visitor for inline arguments validation ([#1577](#1577)) ([fce54b2](fce54b2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
4 participants
This PR adds a visitor which can be registered by the router, and is used to validate inline arguments. And on top of that allow setting of inline argument values for printing with extensions (this is in case of non-enforced).
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.