feat: defer additional fixes#1464
Closed
devsergiy wants to merge 19 commits into
Closed
Conversation
devsergiy
requested review from
SkArchon and
StarpTech
and removed request for
a team
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughImplements parallel and nested GraphQL Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as resolve.go
participant DB as DataBuffer
participant L as Loader
participant R as Resolvable
participant Writer as DeferWriter
Client->>Resolver: ResolveGraphQLDeferResponse(response)
Resolver->>R: NewResolvable + Init
Resolver->>DB: new DataBuffer(enableLock=false)
Resolver->>L: NewLoader(options, DB)
L->>L: LoadGraphQLResponseData (initial fetch tree)
Resolver->>R: inject data/errors from loader
R->>Writer: Resolve → {data:{...}, pending:[{id,path}...], hasNext:true}
Resolver->>DB: enableLock = true
Resolver->>Resolver: resolveDeferTree(DeferTreeNode)
par Parallel siblings via errgroup
Resolver->>L: groupLoader1.LoadGraphQLResponseData
L->>DB: Lock → mergeResult → Set
Resolver->>R: ResolveDefer(currentDefer1, hasNext)
R->>Writer: {incremental:[{id, data}], completed:[{id}], hasNext:true/false}
L->>DB: Unlock
and
Resolver->>L: groupLoader2.LoadGraphQLResponseData
L->>DB: Lock → mergeResult → Set
Resolver->>R: ResolveDefer(currentDefer2, hasNext)
R->>Writer: {incremental:[{id, data}], completed:[{id}], hasNext:false}
L->>DB: Unlock
end
Resolver->>Writer: Complete()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/defer-design.md`:
- Line 334: The markdown has heading level jumps: change the two headings
currently using "######" (the "Normal envelope (`deferItemDataNull=false`)"
heading and the similar heading in the incremental rendering subsection) to one
level higher "#####" so they sit under the existing "####" section and resolve
MD001; update both occurrences accordingly to maintain consistent heading
hierarchy.
🪄 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: 6482f06a-05fd-44f1-bef5-5c7bde984015
📒 Files selected for processing (1)
docs/defer-design.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/defer-design.md`:
- Around line 349-357: Update the "Null envelope (`deferItemDataNull=true`)"
description to match the wire-format: state that printDeferEnvelopeNullData
emits the null item ({"data": null, "path": [...], "errors": [...]}) inside the
standard {"incremental": [ ... ]} envelope rather than omitting the outer
wrapper; change the phrase "the outer `{\"incremental\": [` wrapper are never
written" to "the item is emitted inside the standard `{\"incremental\": [ ...
]}` chunk envelope and the `{\"data\": {` opener is skipped for that item," and
apply the same wording correction to the other occurrence referenced (lines
416-420).
🪄 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: efeed565-a6d7-4b62-aead-60c60b77a0f2
📒 Files selected for processing (1)
docs/defer-design.md
Comment on lines
+349
to
+357
| ##### Null envelope (`deferItemDataNull=true`) | ||
|
|
||
| `printDeferEnvelopeNullData` writes the entire item as | ||
| ```json | ||
| {"data": null, "path": [...], "errors": [...]} | ||
| ``` | ||
|
|
||
| in one shot — the normal `{"data": {` opener and the outer `{"incremental": [` wrapper are never written. The walker returns immediately without descending further. | ||
|
|
Contributor
There was a problem hiding this comment.
Null-envelope description contradicts the documented wire format.
The null-envelope section says the outer {"incremental":[ ... ]} wrapper is never written, but the wire-format section documents null responses with that wrapper. Please align the rendering description to avoid implementer confusion.
📝 Proposed doc fix
-`printDeferEnvelopeNullData` writes the entire item as
+`printDeferEnvelopeNullData` writes the null-data item as
```json
{"data": null, "path": [...], "errors": [...]}-in one shot — the normal {"data": { opener and the outer {"incremental": [ wrapper are never written. The walker returns immediately without descending further.
+in one shot — the normal {"data": { opener is skipped for that item. The item is then emitted inside the standard {"incremental": [ ... ]} chunk envelope. The walker returns immediately without descending further.
</details>
Also applies to: 416-420
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/defer-design.md around lines 349 - 357, Update the "Null envelope
(deferItemDataNull=true)" description to match the wire-format: state that
printDeferEnvelopeNullData emits the null item ({"data": null, "path": [...],
"errors": [...]}) inside the standard {"incremental": [ ... ]} envelope rather
than omitting the outer wrapper; change the phrase "the outer {\"incremental\": [ wrapper are never written" to "the item is emitted inside the standard
{\"incremental\": [ ... ]} chunk envelope and the {\"data\": { opener is
skipped for that item," and apply the same wording correction to the other
occurrence referenced (lines 416-420).
</details>
<!-- fingerprinting:phantom:triton:hawk:21fc2da2-0188-40e2-a9bb-a36a5dd0647a -->
<!-- This is an auto-generated comment by CodeRabbit -->
jensneuse
reviewed
Apr 8, 2026
|
|
||
| Resolves which datasource(s) handle each field. Also detects fields that require additional data to be fetched — `@key` fields for entity resolution and `@requires` fields for computed fields — and injects them directly into the operation AST in the correct defer scope. | ||
|
|
||
| **`@requires` fields** are stamped with the same `@__defer_internal` as the field that needs them. They must be present in the same deferred fetch so the field resolver has the data it depends on. |
Member
There was a problem hiding this comment.
what if multiple fields have overlapping requires?
Member
Author
There was a problem hiding this comment.
Currently, for such schemas
graphql-go-tools/execution/engine/execution_engine_defer_test.go
Lines 746 to 799 in f59a327
Such a query with 2 fields requires selecting a different nested field in settings, but this fields are in the same defer scope
graphql-go-tools/execution/engine/execution_engine_defer_test.go
Lines 1366 to 1376 in f59a327
Current logic always ads an alias to the required fields in defer scope, but reuses already existing alias
So operation with added required fields will look like this
query DeferAllFields {
user {
name @__defer_internal(id: "1")
billing @__defer_internal(id: "1") {
plan @__defer_internal(id: "1")
}
settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
}
account @__defer_internal(id: "1") {
type @__defer_internal(id: "1")
}
notifications @__defer_internal(id: "1")
___typename: __typename
__internal_billing: billing @__defer_internal(id: "1") {
plan @__defer_internal(id: "1")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
language @__defer_internal(id: "1")
}
__internal_name: name @__defer_internal(id: "1")
__typename
id
}
}In case of such query, when the fields with requirements in different defer scopes
graphql-go-tools/execution/engine/execution_engine_defer_test.go
Lines 1512 to 1520 in f59a327
The query will look like this
query DeferDerivedFieldsOnly {
user {
name
billing {
plan
}
settings {
region
language
}
account @__defer_internal(id: "1") {
type @__defer_internal(id: "1")
}
notifications @__defer_internal(id: "2")
__internal_billing: billing @__defer_internal(id: "1") {
plan @__defer_internal(id: "1")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
}
__internal_name: name @__defer_internal(id: "2")
__internal_2_settings: settings @__defer_internal(id: "2") {
language @__defer_internal(id: "2")
}
__typename
id
}
}We always alias requirements because in situation like this
query {
user {
settings {
a
}
... @defer {
fieldRequiresSettingsB
fieldRequiresSettingsC
}
}
}We can't reuse settings, because it is not in the defer scope, and it will mean that we will fetch required fields with primary response, not via deffer group
Member
Author
There was a problem hiding this comment.
hmm, after writing this comment I realized that there could be small improvement
settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
language @__defer_internal(id: "1")
}In this case the existing field falls into the same defer scope, so we could actually reuse it
But in the case of such an operation
settings @__defer_internal(id: "1") {
anyField @__defer_internal(id: "1")
region @__defer_internal(id: "2")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
language @__defer_internal(id: "1")
}when needed region field is in the other defer scope, we will need to add an alias to this nested field, which currently is not implemented, as an alias is added only at the root of the requires selection set
Using aliases is a tradeoff to be able to properly distribute dependencies between fields, to assign them to a proper planner
In the last example region will be returned to the client only with defer group 2
Potentially, I could mark a field that should be returned later, but fetched earlier via the other fetch
ysmolski
reviewed
Apr 8, 2026
…fer counter starting from index 1
ysmolski
reviewed
Jun 19, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
ysmolski
reviewed
Jun 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/superpowers/specs/2026-05-26-defer-parallel-errors-design.md (1)
607-626:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t derive
hasNextfrom fetch completion order.
remainingis decremented before the render lock, so “last” is defined by fetch completion, not emit order. Under concurrent siblings, a slower fetch can still flush after a faster one and wind up withhasNext:trueon the final payload. Tie the terminal flag to the render/flush critical section or tree walk instead.🤖 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 `@docs/superpowers/specs/2026-05-26-defer-parallel-errors-design.md` around lines 607 - 626, The current design decrements `remaining` before the render lock, causing `hasNext` to be determined by fetch completion order rather than emit order, which can result in slower fetches incorrectly marking `hasNext:true` on the final payload. To fix this, move the logic that determines the terminal flag (`hasNext`) so it is evaluated within the render/flush critical section or during a tree walk operation instead of being derived from when `remaining` is decremented. This ensures the final payload correctly reflects whether additional chunks will actually be emitted, not just whether all fetches have completed.
🧹 Nitpick comments (1)
execution/engine/execution_engine_test.go (1)
151-159: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake streaming expectation modes mutually exclusive.
At Line 153 and Line 157, both assertion paths can run if both fields are set, which makes failures ambiguous. Add a guard so tests choose exactly one mode.
Proposed diff
if opts.streamingResponse { streamingResponse := streamingBuf.String() + require.False(t, testCase.expectedResponse != "" && len(testCase.expectedResponses) > 0, + "set either expectedResponse or expectedResponses, not both") if testCase.expectedResponse != "" { assert.Equal(t, testCase.expectedResponse, streamingResponse) } if len(testCase.expectedResponses) > 0 { assert.Contains(t, testCase.expectedResponses, streamingResponse) } } else {🤖 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 `@execution/engine/execution_engine_test.go` around lines 151 - 159, The streaming response validation currently allows both testCase.expectedResponse and testCase.expectedResponses assertions to run simultaneously, creating ambiguous test failures when both fields are populated. Change the second condition checking len(testCase.expectedResponses) > 0 from an if statement to an else if statement, so that only one assertion path executes - either the assert.Equal for expectedResponse or the assert.Contains for expectedResponses, but never both.
🤖 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 `@docs/superpowers/plans/2026-04-17-defer-label-support.md`:
- Around line 5-8: This plan document (2026-04-17-defer-label-support.md) is now
superseded by later plans in the PR (central-map / pending-completed / subPath
docs) and keeping both active creates conflicting implementation contracts. Add
a clear supersession notice at the top of the document indicating that this plan
has been superseded by the newer approaches, so implementers don't follow
incompatible directions.
In `@docs/superpowers/plans/2026-04-29-defer-subpath.md`:
- Around line 120-140: The deferPath() function builds the descriptor.path using
schema field names, but it must use the response alias when one exists in the
operation (e.g., alias: user { ... `@defer` ... }). When iterating through
c.Walker.Path and collecting FieldName items, or when determining field names
from c.Walker.Ancestors NodeKindField entries, check whether each field has an
alias defined in the operation using c.operation and related helpers. If an
alias exists, use that alias name in the path instead of the actual field name
from the schema, since the response data structure will be keyed by the alias,
not the schema field name. This ensures both pending[].path and subPath work
correctly for aliased queries.
In `@docs/superpowers/plans/2026-05-26-defer-parallel-errors.md`:
- Around line 569-585: The clone-based context isolation in the defer-group
error handling prevents subgraph errors from being aggregated in
Context.SubgraphErrors(), which downstream callers depend on. Remove the
per-goroutine clone isolation by eliminating the `childCtx :=
ctx.clone(ctx.ctx)` line and pass the shared context directly to
r.resolveDeferTree instead. Then ensure all mutations to ctx.subgraphErrors
within the defer group goroutine are protected by db.Lock() (consistent with how
it's done in other parts of the code) so that errors are properly aggregated
into the request context without race conditions.
In `@execution/engine/execution_engine_defer_test.go`:
- Around line 699-700: The expectedResponse string in this test contains
malformed JSON with an extra closing brace before the `,"pending"` element. The
outer object is being closed prematurely with 4 closing braces after
"black@sabbat" when only 3 are needed. Remove one closing brace from the
sequence of closing braces that appears before `,"pending"` in the
expectedResponse string to properly structure the JSON so that the outer object
remains open to accept the pending, completed, and hasNext fields.
In `@execution/engine/execution_engine_test.go`:
- Around line 147-149: The comparison at line 148 using compactJSONForAssert
performs string-based JSON comparison, which fails when expectedJSONResponse and
actualResponse contain semantically identical JSON objects with keys in
different orders. Replace the string comparison assert.Equal call with a JSON
semantic comparison approach that parses both JSON strings into objects and
compares them structurally (either unmarshaling into maps or using a dedicated
JSON comparison library), rather than relying on compactJSONForAssert which does
not guarantee consistent key ordering.
---
Outside diff comments:
In `@docs/superpowers/specs/2026-05-26-defer-parallel-errors-design.md`:
- Around line 607-626: The current design decrements `remaining` before the
render lock, causing `hasNext` to be determined by fetch completion order rather
than emit order, which can result in slower fetches incorrectly marking
`hasNext:true` on the final payload. To fix this, move the logic that determines
the terminal flag (`hasNext`) so it is evaluated within the render/flush
critical section or during a tree walk operation instead of being derived from
when `remaining` is decremented. This ensures the final payload correctly
reflects whether additional chunks will actually be emitted, not just whether
all fetches have completed.
---
Nitpick comments:
In `@execution/engine/execution_engine_test.go`:
- Around line 151-159: The streaming response validation currently allows both
testCase.expectedResponse and testCase.expectedResponses assertions to run
simultaneously, creating ambiguous test failures when both fields are populated.
Change the second condition checking len(testCase.expectedResponses) > 0 from an
if statement to an else if statement, so that only one assertion path executes -
either the assert.Equal for expectedResponse or the assert.Contains for
expectedResponses, but never both.
🪄 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: 67884d71-388f-459b-911b-e8b76f7115e6
📒 Files selected for processing (48)
docs/superpowers/plans/2026-04-17-defer-id-string-to-int.mddocs/superpowers/plans/2026-04-17-defer-label-support.mddocs/superpowers/plans/2026-04-28-defer-label-refactor.mddocs/superpowers/plans/2026-04-28-defer-pending-completed.mddocs/superpowers/plans/2026-04-29-defer-subpath.mddocs/superpowers/plans/2026-05-18-defer-parallel-execution.mddocs/superpowers/plans/2026-05-20-defer-populate-parent-ids.mddocs/superpowers/plans/2026-05-26-defer-parallel-errors.mddocs/superpowers/plans/2026-06-09-per-fetch-subgraph-errors.mddocs/superpowers/specs/2026-05-15-defer-parallel-execution-design.mddocs/superpowers/specs/2026-05-26-defer-parallel-errors-design.mdexecution/engine/engine_config_test.goexecution/engine/execution_engine_defer_test.goexecution/engine/execution_engine_helpers_test.goexecution/engine/execution_engine_test.gov2/pkg/ast/ast_field.gov2/pkg/astnormalization/astnormalization.gov2/pkg/astnormalization/astnormalization_test.gov2/pkg/astnormalization/defer_expand_into_internal.gov2/pkg/astnormalization/defer_populate_parent_ids.gov2/pkg/astnormalization/defer_populate_parent_ids_test.gov2/pkg/asttransform/fixtures/complete.goldenv2/pkg/asttransform/fixtures/custom_query_name.goldenv2/pkg/asttransform/fixtures/mutation_only.goldenv2/pkg/asttransform/fixtures/schema_missing.goldenv2/pkg/asttransform/fixtures/simple.goldenv2/pkg/asttransform/fixtures/subscription_only.goldenv2/pkg/asttransform/fixtures/subscription_renamed.goldenv2/pkg/asttransform/fixtures/with_mutation_subscription.goldenv2/pkg/engine/datasource/graphql_datasource/graphql_datasource_defer_test.gov2/pkg/engine/plan/defer_info_collector.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/postprocess/build_defer_tree.gov2/pkg/engine/postprocess/build_defer_tree_test.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/resolve/const.gov2/pkg/engine/resolve/data_buffer.gov2/pkg/engine/resolve/data_buffer_test.gov2/pkg/engine/resolve/defer_tree.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/per_fetch_subgraph_errors_test.gov2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolvable_test.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/resolve_defer_parallel_test.gov2/pkg/engine/resolve/response.go
💤 Files with no reviewable changes (33)
- v2/pkg/engine/resolve/const.go
- v2/pkg/engine/resolve/defer_tree.go
- v2/pkg/engine/resolve/data_buffer.go
- v2/pkg/astnormalization/defer_populate_parent_ids_test.go
- v2/pkg/asttransform/fixtures/simple.golden
- v2/pkg/asttransform/fixtures/with_mutation_subscription.golden
- v2/pkg/engine/resolve/data_buffer_test.go
- v2/pkg/asttransform/fixtures/subscription_only.golden
- v2/pkg/asttransform/fixtures/subscription_renamed.golden
- v2/pkg/engine/postprocess/build_defer_tree.go
- v2/pkg/astnormalization/astnormalization_test.go
- v2/pkg/asttransform/fixtures/mutation_only.golden
- v2/pkg/astnormalization/defer_populate_parent_ids.go
- v2/pkg/asttransform/fixtures/complete.golden
- v2/pkg/asttransform/fixtures/schema_missing.golden
- v2/pkg/asttransform/fixtures/custom_query_name.golden
- v2/pkg/engine/postprocess/build_defer_tree_test.go
- v2/pkg/engine/resolve/response.go
- v2/pkg/engine/plan/defer_info_collector.go
- v2/pkg/astnormalization/astnormalization.go
- v2/pkg/astnormalization/defer_expand_into_internal.go
- v2/pkg/engine/resolve/per_fetch_subgraph_errors_test.go
- v2/pkg/engine/plan/visitor.go
- v2/pkg/engine/plan/planner.go
- v2/pkg/engine/postprocess/postprocess.go
- v2/pkg/engine/resolve/resolve_defer_parallel_test.go
- v2/pkg/engine/resolve/resolvable_test.go
- v2/pkg/ast/ast_field.go
- v2/pkg/engine/resolve/loader_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_defer_test.go
- v2/pkg/engine/resolve/resolve.go
- v2/pkg/engine/resolve/resolvable.go
- v2/pkg/engine/resolve/loader.go
✅ Files skipped from review due to trivial changes (3)
- execution/engine/engine_config_test.go
- docs/superpowers/plans/2026-04-17-defer-id-string-to-int.md
- docs/superpowers/plans/2026-06-09-per-fetch-subgraph-errors.md
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/superpowers/specs/2026-05-26-defer-parallel-errors-design.md (1)
607-626:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t derive
hasNextfrom fetch completion order.
remainingis decremented before the render lock, so “last” is defined by fetch completion, not emit order. Under concurrent siblings, a slower fetch can still flush after a faster one and wind up withhasNext:trueon the final payload. Tie the terminal flag to the render/flush critical section or tree walk instead.🤖 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 `@docs/superpowers/specs/2026-05-26-defer-parallel-errors-design.md` around lines 607 - 626, The current design decrements `remaining` before the render lock, causing `hasNext` to be determined by fetch completion order rather than emit order, which can result in slower fetches incorrectly marking `hasNext:true` on the final payload. To fix this, move the logic that determines the terminal flag (`hasNext`) so it is evaluated within the render/flush critical section or during a tree walk operation instead of being derived from when `remaining` is decremented. This ensures the final payload correctly reflects whether additional chunks will actually be emitted, not just whether all fetches have completed.
🧹 Nitpick comments (1)
execution/engine/execution_engine_test.go (1)
151-159: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake streaming expectation modes mutually exclusive.
At Line 153 and Line 157, both assertion paths can run if both fields are set, which makes failures ambiguous. Add a guard so tests choose exactly one mode.
Proposed diff
if opts.streamingResponse { streamingResponse := streamingBuf.String() + require.False(t, testCase.expectedResponse != "" && len(testCase.expectedResponses) > 0, + "set either expectedResponse or expectedResponses, not both") if testCase.expectedResponse != "" { assert.Equal(t, testCase.expectedResponse, streamingResponse) } if len(testCase.expectedResponses) > 0 { assert.Contains(t, testCase.expectedResponses, streamingResponse) } } else {🤖 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 `@execution/engine/execution_engine_test.go` around lines 151 - 159, The streaming response validation currently allows both testCase.expectedResponse and testCase.expectedResponses assertions to run simultaneously, creating ambiguous test failures when both fields are populated. Change the second condition checking len(testCase.expectedResponses) > 0 from an if statement to an else if statement, so that only one assertion path executes - either the assert.Equal for expectedResponse or the assert.Contains for expectedResponses, but never both.
🤖 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 `@docs/superpowers/plans/2026-04-17-defer-label-support.md`:
- Around line 5-8: This plan document (2026-04-17-defer-label-support.md) is now
superseded by later plans in the PR (central-map / pending-completed / subPath
docs) and keeping both active creates conflicting implementation contracts. Add
a clear supersession notice at the top of the document indicating that this plan
has been superseded by the newer approaches, so implementers don't follow
incompatible directions.
In `@docs/superpowers/plans/2026-04-29-defer-subpath.md`:
- Around line 120-140: The deferPath() function builds the descriptor.path using
schema field names, but it must use the response alias when one exists in the
operation (e.g., alias: user { ... `@defer` ... }). When iterating through
c.Walker.Path and collecting FieldName items, or when determining field names
from c.Walker.Ancestors NodeKindField entries, check whether each field has an
alias defined in the operation using c.operation and related helpers. If an
alias exists, use that alias name in the path instead of the actual field name
from the schema, since the response data structure will be keyed by the alias,
not the schema field name. This ensures both pending[].path and subPath work
correctly for aliased queries.
In `@docs/superpowers/plans/2026-05-26-defer-parallel-errors.md`:
- Around line 569-585: The clone-based context isolation in the defer-group
error handling prevents subgraph errors from being aggregated in
Context.SubgraphErrors(), which downstream callers depend on. Remove the
per-goroutine clone isolation by eliminating the `childCtx :=
ctx.clone(ctx.ctx)` line and pass the shared context directly to
r.resolveDeferTree instead. Then ensure all mutations to ctx.subgraphErrors
within the defer group goroutine are protected by db.Lock() (consistent with how
it's done in other parts of the code) so that errors are properly aggregated
into the request context without race conditions.
In `@execution/engine/execution_engine_defer_test.go`:
- Around line 699-700: The expectedResponse string in this test contains
malformed JSON with an extra closing brace before the `,"pending"` element. The
outer object is being closed prematurely with 4 closing braces after
"black@sabbat" when only 3 are needed. Remove one closing brace from the
sequence of closing braces that appears before `,"pending"` in the
expectedResponse string to properly structure the JSON so that the outer object
remains open to accept the pending, completed, and hasNext fields.
In `@execution/engine/execution_engine_test.go`:
- Around line 147-149: The comparison at line 148 using compactJSONForAssert
performs string-based JSON comparison, which fails when expectedJSONResponse and
actualResponse contain semantically identical JSON objects with keys in
different orders. Replace the string comparison assert.Equal call with a JSON
semantic comparison approach that parses both JSON strings into objects and
compares them structurally (either unmarshaling into maps or using a dedicated
JSON comparison library), rather than relying on compactJSONForAssert which does
not guarantee consistent key ordering.
---
Outside diff comments:
In `@docs/superpowers/specs/2026-05-26-defer-parallel-errors-design.md`:
- Around line 607-626: The current design decrements `remaining` before the
render lock, causing `hasNext` to be determined by fetch completion order rather
than emit order, which can result in slower fetches incorrectly marking
`hasNext:true` on the final payload. To fix this, move the logic that determines
the terminal flag (`hasNext`) so it is evaluated within the render/flush
critical section or during a tree walk operation instead of being derived from
when `remaining` is decremented. This ensures the final payload correctly
reflects whether additional chunks will actually be emitted, not just whether
all fetches have completed.
---
Nitpick comments:
In `@execution/engine/execution_engine_test.go`:
- Around line 151-159: The streaming response validation currently allows both
testCase.expectedResponse and testCase.expectedResponses assertions to run
simultaneously, creating ambiguous test failures when both fields are populated.
Change the second condition checking len(testCase.expectedResponses) > 0 from an
if statement to an else if statement, so that only one assertion path executes -
either the assert.Equal for expectedResponse or the assert.Contains for
expectedResponses, but never both.
🪄 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: 67884d71-388f-459b-911b-e8b76f7115e6
📒 Files selected for processing (48)
docs/superpowers/plans/2026-04-17-defer-id-string-to-int.mddocs/superpowers/plans/2026-04-17-defer-label-support.mddocs/superpowers/plans/2026-04-28-defer-label-refactor.mddocs/superpowers/plans/2026-04-28-defer-pending-completed.mddocs/superpowers/plans/2026-04-29-defer-subpath.mddocs/superpowers/plans/2026-05-18-defer-parallel-execution.mddocs/superpowers/plans/2026-05-20-defer-populate-parent-ids.mddocs/superpowers/plans/2026-05-26-defer-parallel-errors.mddocs/superpowers/plans/2026-06-09-per-fetch-subgraph-errors.mddocs/superpowers/specs/2026-05-15-defer-parallel-execution-design.mddocs/superpowers/specs/2026-05-26-defer-parallel-errors-design.mdexecution/engine/engine_config_test.goexecution/engine/execution_engine_defer_test.goexecution/engine/execution_engine_helpers_test.goexecution/engine/execution_engine_test.gov2/pkg/ast/ast_field.gov2/pkg/astnormalization/astnormalization.gov2/pkg/astnormalization/astnormalization_test.gov2/pkg/astnormalization/defer_expand_into_internal.gov2/pkg/astnormalization/defer_populate_parent_ids.gov2/pkg/astnormalization/defer_populate_parent_ids_test.gov2/pkg/asttransform/fixtures/complete.goldenv2/pkg/asttransform/fixtures/custom_query_name.goldenv2/pkg/asttransform/fixtures/mutation_only.goldenv2/pkg/asttransform/fixtures/schema_missing.goldenv2/pkg/asttransform/fixtures/simple.goldenv2/pkg/asttransform/fixtures/subscription_only.goldenv2/pkg/asttransform/fixtures/subscription_renamed.goldenv2/pkg/asttransform/fixtures/with_mutation_subscription.goldenv2/pkg/engine/datasource/graphql_datasource/graphql_datasource_defer_test.gov2/pkg/engine/plan/defer_info_collector.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/postprocess/build_defer_tree.gov2/pkg/engine/postprocess/build_defer_tree_test.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/resolve/const.gov2/pkg/engine/resolve/data_buffer.gov2/pkg/engine/resolve/data_buffer_test.gov2/pkg/engine/resolve/defer_tree.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_test.gov2/pkg/engine/resolve/per_fetch_subgraph_errors_test.gov2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolvable_test.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/resolve_defer_parallel_test.gov2/pkg/engine/resolve/response.go
💤 Files with no reviewable changes (33)
- v2/pkg/engine/resolve/const.go
- v2/pkg/engine/resolve/defer_tree.go
- v2/pkg/engine/resolve/data_buffer.go
- v2/pkg/astnormalization/defer_populate_parent_ids_test.go
- v2/pkg/asttransform/fixtures/simple.golden
- v2/pkg/asttransform/fixtures/with_mutation_subscription.golden
- v2/pkg/engine/resolve/data_buffer_test.go
- v2/pkg/asttransform/fixtures/subscription_only.golden
- v2/pkg/asttransform/fixtures/subscription_renamed.golden
- v2/pkg/engine/postprocess/build_defer_tree.go
- v2/pkg/astnormalization/astnormalization_test.go
- v2/pkg/asttransform/fixtures/mutation_only.golden
- v2/pkg/astnormalization/defer_populate_parent_ids.go
- v2/pkg/asttransform/fixtures/complete.golden
- v2/pkg/asttransform/fixtures/schema_missing.golden
- v2/pkg/asttransform/fixtures/custom_query_name.golden
- v2/pkg/engine/postprocess/build_defer_tree_test.go
- v2/pkg/engine/resolve/response.go
- v2/pkg/engine/plan/defer_info_collector.go
- v2/pkg/astnormalization/astnormalization.go
- v2/pkg/astnormalization/defer_expand_into_internal.go
- v2/pkg/engine/resolve/per_fetch_subgraph_errors_test.go
- v2/pkg/engine/plan/visitor.go
- v2/pkg/engine/plan/planner.go
- v2/pkg/engine/postprocess/postprocess.go
- v2/pkg/engine/resolve/resolve_defer_parallel_test.go
- v2/pkg/engine/resolve/resolvable_test.go
- v2/pkg/ast/ast_field.go
- v2/pkg/engine/resolve/loader_test.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_defer_test.go
- v2/pkg/engine/resolve/resolve.go
- v2/pkg/engine/resolve/resolvable.go
- v2/pkg/engine/resolve/loader.go
✅ Files skipped from review due to trivial changes (3)
- execution/engine/engine_config_test.go
- docs/superpowers/plans/2026-04-17-defer-id-string-to-int.md
- docs/superpowers/plans/2026-06-09-per-fetch-subgraph-errors.md
🛑 Comments failed to post (5)
docs/superpowers/plans/2026-04-17-defer-label-support.md (1)
5-8:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark this plan superseded.
This “purely additive” label-threading plan now conflicts with the later central-map / pending-completed / subPath docs in this PR. Keeping both active will send implementers down two incompatible contracts.
🤖 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 `@docs/superpowers/plans/2026-04-17-defer-label-support.md` around lines 5 - 8, This plan document (2026-04-17-defer-label-support.md) is now superseded by later plans in the PR (central-map / pending-completed / subPath docs) and keeping both active creates conflicting implementation contracts. Add a clear supersession notice at the top of the document indicating that this plan has been superseded by the newer approaches, so implementers don't follow incompatible directions.docs/superpowers/plans/2026-04-29-defer-subpath.md (1)
120-140:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the response alias when computing
descriptor.path.The algorithm here derives the path from schema field names, so an aliased mount like
alias: user { ...@defer... }will be anchored atuserinstead ofalias. That breaks bothpending[].pathandsubPathfor aliased queries.🤖 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 `@docs/superpowers/plans/2026-04-29-defer-subpath.md` around lines 120 - 140, The deferPath() function builds the descriptor.path using schema field names, but it must use the response alias when one exists in the operation (e.g., alias: user { ... `@defer` ... }). When iterating through c.Walker.Path and collecting FieldName items, or when determining field names from c.Walker.Ancestors NodeKindField entries, check whether each field has an alias defined in the operation using c.operation and related helpers. If an alias exists, use that alias name in the path instead of the actual field name from the schema, since the response data structure will be keyed by the alias, not the schema field name. This ensures both pending[].path and subPath work correctly for aliased queries.docs/superpowers/plans/2026-05-26-defer-parallel-errors.md (1)
569-585: 🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n '\bSubgraphErrors\(' v2 execution docsRepository: wundergraph/graphql-go-tools
Length of output: 4395
🏁 Script executed:
# Check the newer planning document to see if it addresses the defer-group subgraph errors issue sed -n '690,900p' docs/superpowers/plans/2026-06-09-per-fetch-subgraph-errors.md | head -100Repository: wundergraph/graphql-go-tools
Length of output: 3722
🏁 Script executed:
# Check the current implementation in resolve.go around the area mentioned in test comments sed -n '660,680p' v2/pkg/engine/resolve/resolve.goRepository: wundergraph/graphql-go-tools
Length of output: 929
🏁 Script executed:
# Check what the test expects around line 262-264 in resolve_defer_parallel_test.go sed -n '260,320p' v2/pkg/engine/resolve/resolve_defer_parallel_test.goRepository: wundergraph/graphql-go-tools
Length of output: 2089
🏁 Script executed:
# Check the actual current implementation in resolve.go around the defer parallel handling sed -n '650,690p' v2/pkg/engine/resolve/resolve.goRepository: wundergraph/graphql-go-tools
Length of output: 1578
🏁 Script executed:
# Check if there's a flushSubgraphErrors method that might have been added as part of the fix rg -n 'flushSubgraphErrors' v2/pkg/engine/resolve/Repository: wundergraph/graphql-go-tools
Length of output: 54
The clone-based isolation prevents defer-group subgraph errors from reaching
Context.SubgraphErrors(), but downstream callers do depend on it.Verification confirms that the router and other consumers rely on
Context.SubgraphErrors()to aggregate all subgraph errors including those from defer groups (see loader hooks tests, test expectations inresolve_defer_parallel_test.go, and router usage). The limitation is not tenable; a merge-back path under lock after the group completes is the required fix. The per-goroutine clone should be removed and the context shared directly, with all mutations toctx.subgraphErrorsprotected bydb.Lock()as done elsewhere.🤖 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 `@docs/superpowers/plans/2026-05-26-defer-parallel-errors.md` around lines 569 - 585, The clone-based context isolation in the defer-group error handling prevents subgraph errors from being aggregated in Context.SubgraphErrors(), which downstream callers depend on. Remove the per-goroutine clone isolation by eliminating the `childCtx := ctx.clone(ctx.ctx)` line and pass the shared context directly to r.resolveDeferTree instead. Then ensure all mutations to ctx.subgraphErrors within the defer group goroutine are protected by db.Lock() (consistent with how it's done in other parts of the code) so that errors are properly aggregated into the request context without race conditions.execution/engine/execution_engine_defer_test.go (1)
699-700:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMalformed JSON: extra closing brace before
,"pending".The expected response has 4 closing braces (
}}}}) after"black@sabbat"but should only have 3 (}}}). The current JSON closes the outer object before,"pending", making it invalid.🐛 Proposed fix
- expectedResponse: `{"data":{"user":{"name":"Black","info":{"email":"black@sabbat"}}}},"pending":[{"id":"1","path":["user"]}],"hasNext":true} + expectedResponse: `{"data":{"user":{"name":"Black","info":{"email":"black@sabbat"}}},"pending":[{"id":"1","path":["user"]}],"hasNext":true}🤖 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 `@execution/engine/execution_engine_defer_test.go` around lines 699 - 700, The expectedResponse string in this test contains malformed JSON with an extra closing brace before the `,"pending"` element. The outer object is being closed prematurely with 4 closing braces after "black@sabbat" when only 3 are needed. Remove one closing brace from the sequence of closing braces that appears before `,"pending"` in the expectedResponse string to properly structure the JSON so that the outer object remains open to accept the pending, completed, and hasNext fields.execution/engine/execution_engine_test.go (1)
147-149:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Locate compactJSONForAssert definition and usages" rg -n -C3 'func compactJSONForAssert|compactJSONForAssert\(' execution/engine echo echo "Show helper implementation file candidates" fd -a 'execution_engine_helpers_test.go|execution_engine_test.go' execution/engineRepository: wundergraph/graphql-go-tools
Length of output: 1230
🏁 Script executed:
cat -n execution/engine/json_assert_test.goRepository: wundergraph/graphql-go-tools
Length of output: 565
Confirm:
compactJSONForAssertdoes not normalize key order; use semantic JSON comparison instead.The function unmarshal then re-marshals JSON (lines 14–18 of
json_assert_test.go), but Go'sjson.Marshaldoes not canonicalize key ordering. IfexpectedJSONResponseandactualResponsecontain semantically identical objects with different key orders (e.g.,{"a":1,"b":2}vs{"b":2,"a":1}), the string comparison at line 148 will incorrectly fail. Replace with a JSON semantic comparison library or normalize key order before comparison.🤖 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 `@execution/engine/execution_engine_test.go` around lines 147 - 149, The comparison at line 148 using compactJSONForAssert performs string-based JSON comparison, which fails when expectedJSONResponse and actualResponse contain semantically identical JSON objects with keys in different orders. Replace the string comparison assert.Equal call with a JSON semantic comparison approach that parses both JSON strings into objects and compares them structurally (either unmarshaling into maps or using a dedicated JSON comparison library), rather than relying on compactJSONForAssert which does not guarantee consistent key ordering.
ysmolski
reviewed
Jun 22, 2026
ysmolski
approved these changes
Jun 30, 2026
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
3 participants
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.
@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.