feat!: GraphQL query service over relational read models#127
feat!: GraphQL query service over relational read models#127patrickleet wants to merge 16 commits into
Conversation
Implements specs/query-service-graphql end-to-end: Phase 1: TableKind + RelationshipDef.target_foreign_key (breaking), dep-free SDL renderer, dctl schema --format graphql. Phase 2: graphql feature with filter DSL, permissions, dialect-portable SQL compiler, SQLite/Postgres executors, dynamic role schemas, Service::with_graphql + POST /graphql. Phase 3: aggregates surface in SDL; per-role allow_aggregations. Phase 4: ReadModelChange broadcast on SqlxRepository, Postgres pg_notify hook, subscription root with one-shot streams. Phase 5: GraphqlCommands + Mutation root dispatch via CommandRequest; GraphqlInput/Output type metadata. Docs: docs/graphql.md + README Query API section. Tests: graphql_sdl, graphql_engine, graphql_sqlite (domain fixture exit), graphql_subscriptions_sqlite (commit notify), graphql_commands. Refs: tasks/graphql-qs-epic, tasks/graphql-qs-00-spike through tasks/graphql-qs-13-docs-skills
Implement ChangeHub-driven live query streams: initial yield, commit-path invalidation via repo broadcast forwarder, debounce, hash-gated push. Tests (shipped path): - subscription_pushes_exactly_once_per_commit (filtered list, SQLite) - hash_gate_no_push_when_result_unchanged - mutation_handler_projection_query_loop (mutation→write plan→query) - GraphqlInput/GraphqlOutput derives + mapping golden Fixes skeptic gaps on stub subscriptions and theater command tests. Refs: tasks/graphql-qs-10-subscriptions, tasks/graphql-qs-12-command-mutations, tasks/graphql-qs-11-gql-type-derives
- Embed distributed-graphql skill (registry/frontmatter/directory sync) - dctl scaffold --query-api emits src/query/ skeleton, graphql feature, build_with_graphql + DATABASE_URL pool wiring - query_api implies read-models and promotes in-memory store to sqlite Evidence: cargo test -p distributed_cli --lib (skills + query_api unit); scaffolded service under target/scaffold-query-api-demo cargo check green. Refs: tasks/graphql-qs-13-docs-skills
Wire queryApi.databaseUrl into generated values/env when --query-api is set. Add scripts/graphql-skill-dry-run.sh for AC2 agent-following-skill evidence. Implements [[tasks/graphql-qs-13-docs-skills]] [[tasks/graphql-qs-epic]]
Add cargo run --example graphiql with seeded SQLite orders, shared graphiql_page() with default identity headers, and scaffold GRAPHIQL env (on by default for local, set 0 to disable). Implements [[tasks/graphql-qs-epic]]
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughAdds a feature-gated GraphQL query service over relational read models, including SDL generation, role-based permissions, SQL compilation for SQLite/Postgres, HTTP and GraphiQL routing, live subscriptions, command mutations, repository notifications, CLI scaffolding, documentation, and integration tests. ChangesGraphQL Query Service
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphqlHttp
participant GraphqlEngine
participant SqlCompiler
participant SqlExecutor
Client->>GraphqlHttp: POST /graphql
GraphqlHttp->>GraphqlEngine: execute(session, request)
GraphqlEngine->>SqlCompiler: compile_root(selection, permissions)
SqlCompiler->>SqlExecutor: execute_sql(sql_plan)
SqlExecutor-->>GraphqlEngine: GraphQL value or error
GraphqlEngine-->>GraphqlHttp: GraphQL response
GraphqlHttp-->>Client: HTTP response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
distributed_cli/src/generate/service_crate.rs (1)
55-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--query-apiwith an in-memory store generates uncompilable code.
distributed_features()pushes"sqlite"only forStoreTarget::Sqlite(nothing forStoreTarget::InMemory), butservice_rs()'squery_apibranch (see below) maps every non-Postgres store — includingInMemory— todistributed::SqliteRepository. Scaffolding--query-apiwith an in-memory store therefore emits code referencing a type gated behind a Cargo feature that was never enabled, breaking the build. Since GraphQL fundamentally requires a SQL-backed pool, this combination should be rejected during scaffold generation (e.g. inScaffold::from_spec) rather than silently generating broken code.🛡️ Proposed fix: reject the invalid combination early
fn from_spec(spec: ServiceScaffoldSpec) -> Result<Self, ScaffoldError> { let names = ScaffoldNames::new(&spec.name)?; + if spec.query_api && spec.store == StoreTarget::InMemory { + return Err(ScaffoldError::new( + "--query-api requires a SQL-backed store (sqlite or postgres)", + )); + } let models = model_scaffolds(&spec.models)?;🤖 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 `@distributed_cli/src/generate/service_crate.rs` around lines 55 - 76, Reject the incompatible query_api and StoreTarget::InMemory combination during scaffold validation, preferably in Scaffold::from_spec, before code generation proceeds. Keep distributed_features unchanged unless needed for validation, and ensure the generator returns a clear error instead of emitting service code that references distributed::SqliteRepository without the required feature.
🧹 Nitpick comments (6)
tests/graphql_sqlite/main.rs (1)
117-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
permissions_filter_by_claimto reflect what it actually tests.The test only verifies anonymous deny-by-default (no grants → error or null
orders), not claim-based row-level filtering. The development comments at lines 124–129 explain the limitation but should be cleaned up. Consider renaming to something likeanonymous_role_denied_by_defaultand removing the exploratory notes.🤖 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 `@tests/graphql_sqlite/main.rs` around lines 117 - 146, Rename the test function permissions_filter_by_claim to reflect its actual anonymous deny-by-default behavior, such as anonymous_role_denied_by_default. Remove the exploratory comments describing unsupported claim-based filtering and builder internals, while preserving the existing setup and assertions.src/table/registry.rs (1)
269-325: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueExplicit
target_foreign_keyskips existence and collision validation.When
target_foreign_keyis explicitly set,resolve_m2m_target_foreign_keyreturns it without checking that the column exists on the join table or that it differs from the source-sideforeign_key. The caller at line 208 does validate existence, so the column check is covered. However, a collision withforeign_key(both pointing to the same column) would produce a semantically broken m2m join and is not caught anywhere. Consider adding a guard in the explicit path.🛡️ Optional guard against source/target FK collision
if let Some(explicit) = relationship.target_foreign_key.as_deref() { if explicit.is_empty() { return Err(TableStoreError::Metadata(format!( "model `{}` relationship `{}` target_foreign_key must not be empty", source.model_name, relationship.field_name ))); } + if explicit == relationship.foreign_key.as_deref().unwrap_or_default() { + return Err(TableStoreError::Metadata(format!( + "model `{}` relationship `{}` target_foreign_key must differ from foreign_key", + source.model_name, relationship.field_name + ))); + } return Ok(explicit.to_string()); }🤖 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 `@src/table/registry.rs` around lines 269 - 325, Add a collision guard in the explicit target_foreign_key branch of resolve_m2m_target_foreign_key: reject the configuration when the explicit target key matches relationship.foreign_key, returning a TableStoreError::Metadata describing the model and relationship. Preserve the existing empty-value validation and explicit-key return behavior for non-colliding values.src/graphql/engine.rs (1)
149-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead conditional.
The
ifblock computes a condition and only contains a comment; it has no effect and can be removed for clarity.🤖 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 `@src/graphql/engine.rs` around lines 149 - 154, Remove the no-op conditional in sdl_for_role that checks inner.roles and anonymous_role, leaving the schemas lookup and SDL mapping unchanged.src/graphql/subscribe.rs (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo-op
is_emptyguard inpublish.The
if change.is_empty()block only holds a comment and does nothing, yet the empty set is treated as the "all-dirty" lag signal by consumers (footprint_hits, Line 198). As written, any caller publishing an empty change silently triggers all-dirty re-execution. Either drop the misleading branch or explicitly reject/handle empty publishes so the reserved semantics aren't invoked accidentally.🤖 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 `@src/graphql/subscribe.rs` around lines 41 - 47, Update Subscription::publish to prevent ordinary callers from sending an empty ReadModelChange, since consumers interpret it as the all-dirty lag signal. Remove the no-op is_empty branch and explicitly reject or otherwise handle empty changes before self.tx.send, while preserving the forwarder’s intentional empty-change behavior through its existing explicit path.src/graphql/schema.rs (1)
631-651: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
resolve_subscription_livehelper. It isn’t referenced outside its own definition; the subscription field already builds the stream inline.🤖 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 `@src/graphql/schema.rs` around lines 631 - 651, Remove the unused resolve_subscription_live helper and its associated dead code, leaving the existing inline subscription stream construction unchanged.distributed_cli/src/generate/mod.rs (1)
144-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--query-apiwithout--read-modelssilently scaffolds an empty GraphQL API.
self.read_models(used to emitsrc/query/*.rsmodel modules) is only populated whenspec.read_modelsis true, independent ofself.query_api. Nothing here rejectsquery_api=true, read_models=false, so that combination generates a working-but-empty query API (no exposed models) with no warning to the user.Consider validating this combination in
from_spec(or emitting a generation warning) so users get clear feedback instead of a silently empty schema.🤖 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 `@distributed_cli/src/generate/mod.rs` around lines 144 - 154, Validate the configuration in from_spec so query_api cannot be enabled when read_models is disabled, or emit a clear generation warning for that combination. Ensure users are informed that --query-api without --read-models produces an empty schema, while preserving normal query generation when read models are enabled.
🤖 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 `@distributed_cli/src/cli.rs`:
- Around line 643-651: Add a warning in the store-selection block around
args.query_api when --query-api promotes StoreTarget::InMemory to
StoreTarget::Sqlite. Clearly state that in-memory storage is being replaced with
SQLite, while preserving the existing promotion behavior and avoiding warnings
for other store selections.
In `@distributed_cli/src/generate/gitops.rs`:
- Around line 139-153: Update query_api_env_yaml to conditionally emit
DATABASE_URL only when .Values.queryApi.databaseUrl is non-empty, while
retaining the existing query_api and queryApi.enabled checks. Follow the
truthiness-guard pattern used by tracing_env_yaml so an empty default value does
not create the environment variable and build_with_graphql can use its fallback
connection string.
In `@distributed_cli/src/generate/service_crate.rs`:
- Around line 283-341: Update the generated build_with_graphql function so its
routes receive the persistent repository connected from DATABASE_URL, rather
than constructing InMemoryRepository::new(). Reuse the store created by the
{repo_ty}::connect call when building Routes::new().with_dependencies(...),
ensuring command writes and GraphQL queries share the same backing store.
In `@src/graphql/commands.rs`:
- Around line 84-88: Update GraphqlCommands::command to detect an existing
command with the same name before appending to self.commands, and reject
duplicates with a clear API-level error consistent with the method’s existing
return contract. Preserve the current command_name assignment and insertion
behavior for unique names.
In `@src/graphql/compile.rs`:
- Around line 402-411: Update chunked_json_object’s key formatting to
defensively validate or escape each client-controlled response_key before
embedding it as a SQL string literal, including the single-chunk path shown here
and any equivalent chunked path. Preserve valid GraphQL aliases while preventing
quote-containing keys from being emitted unescaped.
- Around line 486-492: Update the BelongsTo arm in the join_pred match within
the relationship compilation logic to compare the target table’s primary-key
column against the source table’s foreign-key column. Keep the existing HasMany
predicate unchanged and use the available target PK and source FK symbols rather
than target-side fk lookup.
- Around line 88-146: Reorder bind collection in compile_root,
compile_relationship_subquery, and compile_m2m_subquery to follow each SQL
statement’s emitted placeholder order: collect projection binds before where
binds, then limit/offset binds. Update RootKind::Aggregate so the duplicated
where_sql interpolation has a corresponding second set of SQLite binds,
preserving correct positional binding.
In `@src/graphql/engine.rs`:
- Around line 201-211: The execute_stream path must handle roles absent from
self.inner.schemas without panicking. Replace the expect("role schema missing")
in execute_stream with the same graceful error behavior used by execute(),
returning an error stream or single error Response while preserving normal
execution for configured roles.
In `@src/graphql/execute.rs`:
- Around line 55-56: Update the SQLite result extraction in the surrounding
execution flow to propagate `row.try_get(0)` decode errors instead of converting
them to `None` with `.ok()` and falling back to `"null"`. Match the
error-surfacing behavior of the Postgres path while preserving the existing
successful text-result handling.
In `@src/graphql/filter.rs`:
- Around line 132-161: Add a direct From<f64> implementation for Operand
alongside the existing numeric conversions, converting the value through
LitValue::from so float literals can be passed to comparison methods without
wrapping them in lit(). Optionally add the equivalent From<f32> implementation
if LitValue supports that conversion.
In `@src/graphql/schema.rs`:
- Line 34: Update build_role_schema to use the disable_introspection parameter
when constructing the role schema, rather than discarding it in the let _
assignment. Ensure the generated anonymous-role schema disables introspection
when the flag is true, while preserving existing behavior when it is false.
In `@src/graphql/sdl.rs`:
- Around line 372-378: Update ensure_object_type to register the same nested
{rel}_aggregate field that emit_object_type emits when options.aggregates is
enabled, including its where argument and aggregate return type. Keep the
dynamic schema’s relationship and top-level aggregate registration unchanged,
and ensure the field is added only for the same has-many/many-to-many
relationships.
In `@tests/graphql_commands/main.rs`:
- Around line 188-191: Update the user-role assertion in the GraphQL command
test to check specifically for the `type Mutation` declaration, matching the
existing anonymous-role assertion, and remove the broad case-insensitive
`Mutation` checks.
In `@tests/graphql_sqlite/main.rs`:
- Around line 73-91: Remove the exploratory
`GraphqlEngine::builder(...).table_schema(...)` block and its comments from
`list_filter_and_by_pk`. Keep the `DistributedProjectManifest::new("orders")`
and `GraphqlEngine::from_manifest` setup, using the single `setup_pool()` result
and the original `schema` without cloning.
---
Outside diff comments:
In `@distributed_cli/src/generate/service_crate.rs`:
- Around line 55-76: Reject the incompatible query_api and StoreTarget::InMemory
combination during scaffold validation, preferably in Scaffold::from_spec,
before code generation proceeds. Keep distributed_features unchanged unless
needed for validation, and ensure the generator returns a clear error instead of
emitting service code that references distributed::SqliteRepository without the
required feature.
---
Nitpick comments:
In `@distributed_cli/src/generate/mod.rs`:
- Around line 144-154: Validate the configuration in from_spec so query_api
cannot be enabled when read_models is disabled, or emit a clear generation
warning for that combination. Ensure users are informed that --query-api without
--read-models produces an empty schema, while preserving normal query generation
when read models are enabled.
In `@src/graphql/engine.rs`:
- Around line 149-154: Remove the no-op conditional in sdl_for_role that checks
inner.roles and anonymous_role, leaving the schemas lookup and SDL mapping
unchanged.
In `@src/graphql/schema.rs`:
- Around line 631-651: Remove the unused resolve_subscription_live helper and
its associated dead code, leaving the existing inline subscription stream
construction unchanged.
In `@src/graphql/subscribe.rs`:
- Around line 41-47: Update Subscription::publish to prevent ordinary callers
from sending an empty ReadModelChange, since consumers interpret it as the
all-dirty lag signal. Remove the no-op is_empty branch and explicitly reject or
otherwise handle empty changes before self.tx.send, while preserving the
forwarder’s intentional empty-change behavior through its existing explicit
path.
In `@src/table/registry.rs`:
- Around line 269-325: Add a collision guard in the explicit target_foreign_key
branch of resolve_m2m_target_foreign_key: reject the configuration when the
explicit target key matches relationship.foreign_key, returning a
TableStoreError::Metadata describing the model and relationship. Preserve the
existing empty-value validation and explicit-key return behavior for
non-colliding values.
In `@tests/graphql_sqlite/main.rs`:
- Around line 117-146: Rename the test function permissions_filter_by_claim to
reflect its actual anonymous deny-by-default behavior, such as
anonymous_role_denied_by_default. Remove the exploratory comments describing
unsupported claim-based filtering and builder internals, while preserving the
existing setup and assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46112043-8920-4dad-93ad-83bf29e54e1e
📒 Files selected for processing (51)
Cargo.tomlREADME.mddistributed_cli/skills/distributed-graphql/SKILL.mddistributed_cli/src/cli.rsdistributed_cli/src/generate/gitops.rsdistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/src/lib.rsdistributed_cli/src/manifest_harness.rsdistributed_cli/src/skills.rsdistributed_macros/src/graphql_types.rsdistributed_macros/src/lib.rsdistributed_macros/src/read_model.rsdocs/graphql.mdexamples/graphiql.rsscripts/graphql-skill-dry-run.shsrc/graphql/commands.rssrc/graphql/compile.rssrc/graphql/engine.rssrc/graphql/execute.rssrc/graphql/filter.rssrc/graphql/http.rssrc/graphql/mod.rssrc/graphql/naming.rssrc/graphql/permissions.rssrc/graphql/schema.rssrc/graphql/sdl.rssrc/graphql/subscribe.rssrc/graphql/types.rssrc/lib.rssrc/manifest.rssrc/microsvc/http.rssrc/microsvc/mod.rssrc/microsvc/service.rssrc/outbox/table.rssrc/postgres_repo/mod.rssrc/read_model/change.rssrc/read_model/in_memory.rssrc/read_model/mod.rssrc/sqlx_repo/read_model.rssrc/sqlx_repo/repo.rssrc/table/metadata.rssrc/table/mod.rssrc/table/registry.rssrc/table/sql.rssrc/telemetry.rstests/graphql_commands/main.rstests/graphql_engine/main.rstests/graphql_sdl/main.rstests/graphql_sqlite/main.rstests/graphql_subscriptions_sqlite/main.rs
Export GitKB specs/query-layer/* and the query-service-graphql redirect into docs/ for agent and CI discoverability. Normative package includes Agent seams for metrics, AuthZ fixtures, WS, IR, and goldens. Refs: specs/query-layer/index
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
docs/query-layer/authorization.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an h2 for the first subsection.
After the level-1
# Authorizationheading, this heading should be##to satisfy markdownlint MD001 and maintain a valid hierarchy.🤖 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/query-layer/authorization.md` at line 13, Update the “Authorization: role-based select permissions” heading in the authorization documentation from level 3 to level 2, preserving the existing heading text and hierarchy beneath the level-1 “Authorization” heading.Source: Linters/SAST tools
docs/query-layer/security.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an h2 for the first subsection.
After the level-1
# Security, SQL execution, limits, and errorsheading, this heading should be##to satisfy markdownlint MD001.🤖 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/query-layer/security.md` at line 13, Change the “Execution: GraphQL → one SQL statement per root field” heading from level 3 to level 2 so it is the first subsection under the document’s level-1 heading and satisfies markdownlint MD001.Source: Linters/SAST tools
docs/query-layer/surface.md (2)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an h2 for the first subsection.
After the level-1
# Surface, schema derivation, layout, and dctlheading, this heading should be##to satisfy markdownlint MD001.🤖 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/query-layer/surface.md` at line 13, Update the first subsection heading, “Placement: graphql feature on the core crate,” from level 3 to level 2 so it directly follows the document’s level-1 heading and satisfies markdownlint MD001.Source: Linters/SAST tools
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify the fenced block language.
The directory-tree example should use a language such as
text(```text) so markdownlint MD040 passes and renderers handle it consistently.🤖 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/query-layer/surface.md` at line 231, Update the directory-tree fenced code block in the documentation to declare the text language, using the existing example content unchanged and ensuring the fence is labeled for markdownlint MD040 compliance.Source: Linters/SAST tools
docs/query-layer/quality.md (1)
15-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd explicit metrics and timeout tests to the quality bar.
The observability spec requires a GraphQL metrics assertion and a long-running SQLite statement that maps to
TIMEOUT, but the test-plan table has no entries for either. Add both tests so these contracts are part of the required release evidence.Also applies to: 30-40
🤖 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/query-layer/quality.md` around lines 15 - 25, Add explicit quality-bar entries for a GraphQL metrics assertion and a long-running SQLite statement that maps to TIMEOUT, placing them in the appropriate test-plan phases alongside the existing GraphQL engine/SQLite coverage. Ensure both observability contracts are listed as required release evidence.
🤖 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/query-layer/architecture.md`:
- Line 13: Update the Markdown structure in architecture.md: change the
`graphql` feature heading to the appropriate heading level relative to its
surrounding sections, and add a `text` language identifier to the module-layout
code fence. Preserve the documented content and layout.
- Around line 21-24: Resolve the conflicting GraphQL feature-gating requirements
across the architecture specification and implementation plan: choose either
compilation failure without a SQL store or successful compilation using an
uninhabited GraphqlPool, then update the relevant documented sections so they
consistently describe the same behavior.
In `@docs/query-layer/authorization.md`:
- Around line 62-72: Align the authorization specification with the runtime’s
current empty-role behavior: document that grant-less roles receive a schema
containing the `_empty` field, which returns the error “FORBIDDEN: role has no
GraphQL grants.” Update the surrounding claims about no schema, no schema
access, and the fixed error message while preserving the documented behavior for
roles with granted models and anonymous filters.
In `@docs/query-layer/http.md`:
- Line 13: Fix the Markdown lint violations in the service integration
documentation: adjust the heading containing Service::with_graphql to the
appropriate heading level, and add a language identifier such as text to the
directory-tree fenced code block. Keep the documented content unchanged.
- Around line 71-77: Reconcile the HTTP and subscription contracts throughout
the document by clearly labeling shipped versus planned behavior: align GraphiQL
defaults and non-GraphiQL GET responses, consistently state whether WebSocket
upgrades are shipped, and remove conflicting delivery disclaimers. Update the
debounce/coalescing description to preserve exactly-once delivery for every
distinct commit, explicitly explaining how coalescing cannot drop committed
events.
In `@docs/query-layer/implementation.md`:
- Around line 101-114: Update the fenced code block containing the src/graphql
module layout to specify the text language identifier immediately after the
opening fence, while leaving the directory tree content unchanged.
In `@docs/query-layer/observability.md`:
- Line 13: Update the Observability heading in the documentation so it uses the
appropriate heading level beneath the document’s top-level “Observability”
heading, correcting the reported hierarchy violation while preserving the
surrounding content.
- Around line 43-45: Align the SQLite timeout result in the observability
documentation with the stable GraphQL error contract: either add TIMEOUT to the
documented closed set of GraphQL error codes or rename the SQLite result to an
already-supported code. Ensure both specifications expose the same timeout
value.
In `@docs/query-layer/quality.md`:
- Line 13: Update the “Test plan (files, per phase)” heading in the
documentation to the appropriate heading level beneath the existing “# Quality
bar” heading, replacing the current ### marker while preserving the heading
text.
In `@docs/query-layer/relationships.md`:
- Line 13: Update the “Many-to-many traversal (normative)” heading to the
appropriate heading level beneath the document’s “# Relationships” heading,
replacing the current ### marker while preserving the heading text.
- Around line 59-77: Reconcile m2m through-table discovery with from_manifest’s
TableKind::ReadModel filtering so manifest-defined operational through tables
are not unexpectedly omitted. Either enforce that every m2m through table is a
ReadModel, or register non-ReadModel through tables as shadow catalog entries
that remain non-exposed while supporting traversal; update the documented
catalog requirement and related discovery behavior consistently.
In `@docs/query-layer/security.md`:
- Line 320: Update the “Slow query” row in the security documentation to avoid
claiming timeout support for both dialects: document Postgres-only timeout
enforcement with SQLite best-effort guards, unless an actual validated SQLite
deadline mechanism is implemented first.
In `@docs/query-layer/surface.md`:
- Around line 129-130: Update the Timestamp row in the query-layer wire-format
table to state that v1 exposes the stored Postgres text form as-is, matching the
policy in the security documentation; remove the unresolved normalization
language and leave the Bytes policy unchanged.
---
Nitpick comments:
In `@docs/query-layer/authorization.md`:
- Line 13: Update the “Authorization: role-based select permissions” heading in
the authorization documentation from level 3 to level 2, preserving the existing
heading text and hierarchy beneath the level-1 “Authorization” heading.
In `@docs/query-layer/quality.md`:
- Around line 15-25: Add explicit quality-bar entries for a GraphQL metrics
assertion and a long-running SQLite statement that maps to TIMEOUT, placing them
in the appropriate test-plan phases alongside the existing GraphQL engine/SQLite
coverage. Ensure both observability contracts are listed as required release
evidence.
In `@docs/query-layer/security.md`:
- Line 13: Change the “Execution: GraphQL → one SQL statement per root field”
heading from level 3 to level 2 so it is the first subsection under the
document’s level-1 heading and satisfies markdownlint MD001.
In `@docs/query-layer/surface.md`:
- Line 13: Update the first subsection heading, “Placement: graphql feature on
the core crate,” from level 3 to level 2 so it directly follows the document’s
level-1 heading and satisfies markdownlint MD001.
- Line 231: Update the directory-tree fenced code block in the documentation to
declare the text language, using the existing example content unchanged and
ensuring the fence is labeled for markdownlint MD040 compliance.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ef9d9c7-77ce-4236-97ac-23f51f272666
📒 Files selected for processing (13)
docs/graphql.mddocs/query-layer/architecture.mddocs/query-layer/authorization.mddocs/query-layer/decisions.mddocs/query-layer/http.mddocs/query-layer/implementation.mddocs/query-layer/index.mddocs/query-layer/observability.mddocs/query-layer/quality.mddocs/query-layer/relationships.mddocs/query-layer/security.mddocs/query-layer/surface.mddocs/query-service-graphql.md
✅ Files skipped from review due to trivial changes (3)
- docs/query-service-graphql.md
- docs/query-layer/decisions.md
- docs/graphql.md
|
|
||
| # Architecture | ||
|
|
||
| ### Placement: `graphql` feature on the core crate |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported Markdown lint violations.
Change the ### heading at Line [13] to the appropriate level and add a language identifier such as text to the module-layout fence beginning at Line [115].
Also applies to: 115-128
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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/query-layer/architecture.md` at line 13, Update the Markdown structure
in architecture.md: change the `graphql` feature heading to the appropriate
heading level relative to its surrounding sections, and add a `text` language
identifier to the module-layout code fence. Preserve the documented content and
layout.
Source: Linters/SAST tools
| Dialect executors compile when the corresponding store feature is also | ||
| enabled (`cfg(all(feature = "graphql", feature = "postgres"))` — the same | ||
| composition pattern as the `sqlx_repo` module itself). Enabling `graphql` | ||
| without any SQL store feature is a compile error with a clear message. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve the contradictory feature-gating contract.
Line [21]-[24] requires graphql without a SQL store to fail compilation, but Line [130]-[140] explicitly requires the opposite: no compile_error!, with an uninhabited GraphqlPool so each-feature CI succeeds. Choose one behavior and make the architecture and implementation specs agree before implementation.
🤖 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/query-layer/architecture.md` around lines 21 - 24, Resolve the
conflicting GraphQL feature-gating requirements across the architecture
specification and implementation plan: choose either compilation failure without
a SQL store or successful compilation using an uninhabited GraphqlPool, then
update the relevant documented sections so they consistently describe the same
behavior.
| anywhere, unauthenticated requests can query nothing. **Empty-role | ||
| handling** (GraphQL forbids object types with zero fields, so a | ||
| grant-less role cannot have a schema at all): a role with zero granted | ||
| models gets **no schema**; any request resolving to it receives a fixed | ||
| error response — `extensions.code: FORBIDDEN`, message "role has no | ||
| query surface" — without touching a schema. This is the one (and only) | ||
| producer of the `FORBIDDEN` error code. Roles with ≥1 grant get normal | ||
| schemas, and ungranted models within them surface as standard | ||
| unknown-field validation errors. One extra guard: `build()` **rejects `claim()` | ||
| references in anonymous filters** — anonymous requests carry no verified | ||
| claims, so such a filter could only ever 400; fail at startup instead. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Align empty-role behavior with the runtime.
The implementation still builds a schema for a grant-less role and adds an _empty field whose error is "FORBIDDEN: role has no GraphQL grants" (src/graphql/engine.rs:540-610, src/graphql/schema.rs:180-210). This section instead promises no schema, no schema access, and a different message. Update the specification to match the implemented contract, or change the runtime and tests together.
🤖 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/query-layer/authorization.md` around lines 62 - 72, Align the
authorization specification with the runtime’s current empty-role behavior:
document that grant-less roles receive a schema containing the `_empty` field,
which returns the error “FORBIDDEN: role has no GraphQL grants.” Update the
surrounding claims about no schema, no schema access, and the fixed error
message while preserving the documented behavior for roles with granted models
and anonymous filters.
|
|
||
| # HTTP, realtime, and command mutations | ||
|
|
||
| ### Service integration: `Service::with_graphql` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported Markdown lint violations.
Change the ### heading at Line [13] to the appropriate level and add a language identifier such as text to the directory-tree fence beginning at Line [96].
Also applies to: 96-105
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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/query-layer/http.md` at line 13, Fix the Markdown lint violations in the
service integration documentation: adjust the heading containing
Service::with_graphql to the appropriate heading level, and add a language
identifier such as text to the directory-tree fenced code block. Keep the
documented content unchanged.
Source: Linters/SAST tools
| schemas. Optional GraphiQL on `GET /graphql` sits behind a builder flag | ||
| (`.graphiql(true)`, dev-only, default off). The standalone router applies | ||
| its own `DefaultBodyLimit::max(MAX_HTTP_BODY_BYTES)` layer (the | ||
| `cloud_events_router` does the same — layers don't inherit across router | ||
| composition). `GET /graphql` multiplexes by request shape: an `Upgrade: | ||
| websocket` header routes to the graphql-ws handler (phase 4); otherwise | ||
| GraphiQL HTML when enabled, else 404. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the HTTP and subscription contracts consistent.
The document currently gives conflicting normative behavior:
- Lines [71]-[77] say GraphiQL defaults off and a non-GraphiQL
GET /graphqlreturns 404, while Lines [443]-[456] say the fallback is 405 and local/unset defaults on. - Lines [75]-[76] describe WS upgrade routing as available, while Line [513] says WS is not shipped.
- Lines [192]-[196] use debounce/latest-wins coalescing, while Lines [227]-[231] disclaim delivery guarantees; this conflicts with the PR objective of exactly-once-per-commit delivery.
Choose and clearly label the shipped versus planned behavior. If exactly-once-per-commit is required, define how it survives debounce/coalescing across distinct commits.
Also applies to: 192-196, 227-231, 443-456
🤖 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/query-layer/http.md` around lines 71 - 77, Reconcile the HTTP and
subscription contracts throughout the document by clearly labeling shipped
versus planned behavior: align GraphiQL defaults and non-GraphiQL GET responses,
consistently state whether WebSocket upgrades are shipped, and remove
conflicting delivery disclaimers. Update the debounce/coalescing description to
preserve exactly-once delivery for every distinct commit, explicitly explaining
how coalescing cannot drop committed events.
|
|
||
| # Quality bar | ||
|
|
||
| ### Test plan (files, per phase) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported heading-level violation.
Change the ### heading at Line [13] to the appropriate level under the document's # Quality bar heading.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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/query-layer/quality.md` at line 13, Update the “Test plan (files, per
phase)” heading in the documentation to the appropriate heading level beneath
the existing “# Quality bar” heading, replacing the current ### marker while
preserving the heading text.
Source: Linters/SAST tools
|
|
||
| # Relationships | ||
|
|
||
| ### Many-to-many traversal (normative) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported heading-level violation.
Change the ### heading at Line [13] to the appropriate level under the document's # Relationships heading.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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/query-layer/relationships.md` at line 13, Update the “Many-to-many
traversal (normative)” heading to the appropriate heading level beneath the
document’s “# Relationships” heading, replacing the current ### marker while
preserving the heading text.
Source: Linters/SAST tools
| **Catalog requirement**: m2m traversal needs the `through` table's | ||
| `TableSchema` in the catalog. Manifest-sourced catalogs **always have | ||
| it**: any manifest that renders DDL already registers the join table, | ||
| because `sql_statements`/`sql_migration_artifacts` run | ||
| `TableSchemaRegistry::validate()` (src/manifest.rs:101-108), which errors | ||
| on an unregistered `through` — so through-absent omission semantics only | ||
| arise on the typed builder path. There, `through` is a table-name string, | ||
| not a type, so the builder gains | ||
| `pub fn table_schema(self, schema: TableSchema) -> Self` — a value-based | ||
| catalog entry with **shadow semantics** (one-hop-traversable material, | ||
| invisible in every role schema, upgrades to exposed if later registered | ||
| via `.model::<M>()`, dedup rules identical to shadow entries). Because | ||
| `through` is a *table* name while the catalog is keyed by `model_name`, | ||
| the engine also maintains a **table_name index**, and a duplicate | ||
| `table_name` across catalog entries is a `build()` error (the registry | ||
| forbids this; the catalog must too). An m2m field whose target model or | ||
| through table is absent from the catalog is omitted from the schema | ||
| (untracked semantics); a `rel()` **permission filter** through such an | ||
| m2m is a `build()` error (permission filters never silently weaken). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reconcile m2m through-table discovery with TableKind filtering.
Lines [59]-[65] claim manifest-sourced catalogs always contain the through table, but the architecture and implementation contracts say from_manifest registers only TableKind::ReadModel entries. An operational through table can therefore exist in the manifest yet be absent from the GraphQL catalog, causing the relationship to be omitted unexpectedly. Either require m2m through tables to be ReadModel or register them as shadow catalog entries without exposing them.
🤖 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/query-layer/relationships.md` around lines 59 - 77, Reconcile m2m
through-table discovery with from_manifest’s TableKind::ReadModel filtering so
manifest-defined operational through tables are not unexpectedly omitted. Either
enforce that every m2m through table is a ReadModel, or register non-ReadModel
through tables as shadow catalog entries that remain non-exposed while
supporting traversal; update the documented catalog requirement and related
discovery behavior consistently.
| | Hostile aliases | Name allowlist | | ||
| | Deep where / selection | max_depth / complexity | | ||
| | Huge `_in` / limit | max_in_list / max_limit | | ||
| | Slow query | timeout both dialects | |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not claim timeout support for both dialects.
Lines 95-99 and 204-205 explicitly state that SQLite has no statement-timeout mechanism, so the “timeout both dialects” row is incorrect. Change it to Postgres-only with SQLite best-effort guards, or implement and validate an actual SQLite deadline before documenting this guarantee.
🤖 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/query-layer/security.md` at line 320, Update the “Slow query” row in the
security documentation to avoid claiming timeout support for both dialects:
document Postgres-only timeout enforcement with SQLite best-effort guards,
unless an actual validated SQLite deadline mechanism is implemented first.
| | Timestamp | `Timestamptz` (custom) | Postgres text form (what the store round-trips today); normalizing to RFC 3339 is an executor concern, decided at implementation | | ||
| | Bytes | `Bytea` (custom) | base64 string — **net-new policy**; nothing in the crate base64s today, and `RowValue::into_json` renders byte arrays as number arrays, which is wrong for an API | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve the timestamp wire format here.
This section still says normalization is undecided, but docs/query-layer/security.md:177-179 specifies that v1 exposes the stored text form as-is. State that decision here as well so generated-client and compatibility expectations are unambiguous.
🤖 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/query-layer/surface.md` around lines 129 - 130, Update the Timestamp row
in the query-layer wire-format table to state that v1 exposes the stored
Postgres text form as-is, matching the policy in the security documentation;
remove the unresolved normalization language and leave the Bytes policy
unchanged.
Mirror specs/query-layer/state for progress tracking against the package. Refs: specs/query-layer/state
- Validate GraphQL response keys; where/filter max_depth; sanitize client errors - Preserve JSON-looking string columns; SQLite statement timeout; no write txn on SELECT - Wire distributed_graphql_request_* metrics; honor introspection_for_anonymous - Scaffold GraphiQL off in production envs; claim AuthZ + relationship e2e suites - Fill graphql_compile/http/postgres tests; fix skills init count for graphql skill Implements [[tasks/graphql-qs-harden-1]] P0 + P1 (P2 deferred)
Increment where/filter boolean nesting depth; prove restricted role cannot select denied columns and nested _and exceeds max_depth with stable errors. Implements [[tasks/graphql-qs-harden-4]] [[tasks/graphql-qs-harden-7]]
Reflects column allowlist e2e, where max_depth, last-reviewed, and rollup posture for harden epic. Implements [[tasks/graphql-qs-harden-1]]
Specs live in GitKB (specs/query-layer/*) only. Drop docs/query-layer/ and docs/query-service-graphql.md; point docs/graphql.md at the KB. Implements [[tasks/graphql-qs-harden-1]]
- harden-12: anonymous __schema denied when introspection_for_anonymous(false) - harden-13: statement_timeout ZERO yields client TIMEOUT via execute path - harden-11: graphiql_enabled_from_env_vars(production) + HTTP GET 405; scaffold calls shared graphiql_enabled_from_env Implements [[tasks/graphql-qs-harden-1]]
Extract apply_statement_timeout + client_error_for_execute_err with paused-clock / pure unit tests. Integration holds BEGIN EXCLUSIVE so the engine SELECT cannot race past the budget (no Duration::ZERO flake). Implements [[tasks/graphql-qs-harden-1]]
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/metrics.rs (1)
591-600: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
reset()does not clear GraphQL metric maps.The
#[cfg(test)] reset()method clears all other metric maps but omitsgraphql_request_totalandgraphql_request_duration. This inconsistency could cause stale GraphQL metrics to persist across tests that callreset_for_tests(), leading to test interference.Proposed fix
fn reset(&self) { self.lock(&self.service_info).clear(); self.lock(&self.dispatch_total).clear(); self.lock(&self.dispatch_duration).clear(); self.lock(&self.transport_messages_total).clear(); self.lock(&self.transport_failures_total).clear(); self.lock(&self.outbox_messages_total).clear(); self.lock(&self.outbox_pending_messages).clear(); self.lock(&self.outbox_oldest_pending_age_seconds).clear(); + self.lock(&self.graphql_request_total).clear(); + self.lock(&self.graphql_request_duration).clear(); }🤖 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 `@src/metrics.rs` around lines 591 - 600, Update the test-only reset method to also clear the graphql_request_total and graphql_request_duration metric maps, alongside the existing metrics in reset, so reset_for_tests removes all metric state between tests.
🧹 Nitpick comments (1)
src/telemetry.rs (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
metric_labels::ROOT_FIELDconstant for consistency.All other entries in
ALLOWED_METRIC_LABELSreferencesuper::metric_labels::*constants, but"root_field"is a raw string literal. The same raw string appears inGraphqlCounterKey::labels()andGraphqlHistogramKey::labels()insrc/metrics.rs. A named constant would prevent drift if the label name ever changes.🤖 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 `@src/telemetry.rs` around lines 116 - 117, Define a metric_labels::ROOT_FIELD constant for the "root_field" label, then replace the raw string in ALLOWED_METRIC_LABELS and in GraphqlCounterKey::labels() and GraphqlHistogramKey::labels() with that constant.
🤖 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 `@src/graphql/execute.rs`:
- Around line 70-71: Update the SQLite result-column handling around row.try_get
in the execute flow to propagate decode or driver errors instead of converting
them to None and falling back to "null". Return the error in the same manner as
the Postgres path, while preserving the existing "null" fallback only for an
actual SQL NULL value.
In `@tests/graphql_compile/main.rs`:
- Around line 32-38: Update the SQLite setup in the test around the sqlx::query
call to avoid semicolon-separated CREATE TABLE and INSERT statements; execute
them separately or use sqlx::raw_sql(), ensuring the INSERT runs before the
existing row-count assertion.
In `@tests/graphql_postgres/main.rs`:
- Around line 40-46: Split the database seeding in the test setup into separate
sqlx::query calls: execute the CREATE TABLE statement first, then execute the
INSERT statement in a second call, preserving the existing pool and
expect("seed") error handling.
---
Outside diff comments:
In `@src/metrics.rs`:
- Around line 591-600: Update the test-only reset method to also clear the
graphql_request_total and graphql_request_duration metric maps, alongside the
existing metrics in reset, so reset_for_tests removes all metric state between
tests.
---
Nitpick comments:
In `@src/telemetry.rs`:
- Around line 116-117: Define a metric_labels::ROOT_FIELD constant for the
"root_field" label, then replace the raw string in ALLOWED_METRIC_LABELS and in
GraphqlCounterKey::labels() and GraphqlHistogramKey::labels() with that
constant.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62cca900-2a90-49ca-b4fa-fd29179ab508
📒 Files selected for processing (17)
Cargo.tomlREADME.mddistributed_cli/src/generate/mod.rsdistributed_cli/src/generate/service_crate.rsdistributed_cli/tests/cli_skills_init.rsdocs/graphql.mdsrc/graphql/compile.rssrc/graphql/engine.rssrc/graphql/execute.rssrc/graphql/mod.rssrc/graphql/schema.rssrc/metrics.rssrc/telemetry.rstests/graphql_compile/main.rstests/graphql_harden/main.rstests/graphql_http/main.rstests/graphql_postgres/main.rs
✅ Files skipped from review due to trivial changes (3)
- distributed_cli/tests/cli_skills_init.rs
- docs/graphql.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/graphql/mod.rs
- src/graphql/engine.rs
- distributed_cli/src/generate/mod.rs
- distributed_cli/src/generate/service_crate.rs
- src/graphql/schema.rs
- src/graphql/compile.rs
|
🤖 Agent dispatch: Branch: # Watch live:
atc watch --id tasks--graphql-qs-epic@review-fix@1783828313788-e4e0
# View logs:
atc logs tasks--graphql-qs-epic@review-fix@1783828313788-e4e0
# Attach to tmux:
tmux attach -t tasks--graphql-qs-epic@review-fix@1783828313788-e4e0 |
Implements [[tasks--graphql-qs-epic]]
Review-fix summaryPushed review-fix commit
Additional review flywheel fixes in the same commit: corrected has-many joins when parent PK and child FK names differ, prevented bidirectional relationship schema recursion, applied many-to-many client relationship filters instead of dropping them, expanded subscription table-footprint tracking, and cleaned up clippy issues across the touched GraphQL/test files. Build status:
Confidence: 100%. Human review needed: none flagged. |
|
I'll re-review the updated diff to verify each fix lands as described, particularly the bind-order reordering, BelongsTo join direction fix, and nested relationship aggregate execution, since those are the highest-risk changes. ᕮᕭ ᐡ•ᴥ•ᐡ nice work getting through the whole flywheel list ✅ Action performedReview finished.
|
1 participant
Summary
Implements the GraphQL query-service epic (
tasks/graphql-qs-epic/specs/query-service-graphql): Hasura-style read-only GraphQL over relational read models, with deny-by-default RBAC, dialect SQL compilation (SQLite + Postgres), commit-path live subscriptions, command mutations, CLI skill/scaffold, and a local GraphiQL playground.What’s included
by_pk/ aggregates / where / order_by / relationships;dctl schema --format graphqlSDL artifactCommandRequestdispatch → projection → query (phase-5 e2e)distributed-graphqlskill,dctl scaffold --query-api, gitopsDATABASE_URL/queryApi.databaseUrlcargo run --example graphiql --features "graphql,sqlite"→ http://127.0.0.1:4000/graphql; scaffold GraphiQL on by default (GRAPHIQL=0to disable)Breaking
feat!metadata batch:TableSchema/ relationships /TableKindfor operational vs read-model tables (operational tables never exposed on GraphQL)Test plan
cargo test --workspace --all-features(in worktree)cargo hack check --each-featuregraphql_sdl,graphql_engine,graphql_sqlite,graphql_subscriptions_sqlite,graphql_commandsquery_api_emits_query_modules_and_graphql_feature(incl. DATABASE_URL gitops)GET /graphql200,POSTquery returns seeded orders)DATABASE_URLavailable (env-gated; not required for merge bar)Notes
x-role/x-user-id); GraphiQL pre-fills defaults for local use only.tests/graphql_compile/tests/graphql_postgresplaceholders; observabilityrecord_metricsstill a stub.Implements [[tasks/graphql-qs-epic]]
Summary by CodeRabbit
New Features
/graphql, including configurable local access and identity headers.dctl schema --format graphql.dctl scaffold --query-apito generate GraphQL-ready services and deployment configuration.Documentation