Context
PR #349 introduced GraphQL partial-success handling for automations.executionMetrics: a response can carry readable data alongside per-node errors (e.g. PERMISSION_DENIED) in one payload, and we want one denied automation not to drop the rest of the batch.
That PR modelled "partial" as a second capability on the execution seam:
- two execute verbs on the adapter:
execute_query (all-or-nothing, returns dict) and execute_query_allow_partial (returns PartialQueryResult)
- two protocols:
GraphQLExecutor and PartialGraphQLExecutor extends GraphQLExecutor
A follow-up already landed on that PR (Rung 1): the fatal-vs-partial decision was de-smeared. execute_query_allow_partial no longer classifies. It always returns a PartialQueryResult (a fully null response yields data={}), and the single "no readable data means failure" decision now lives in one pure function, _normalize_execution_metrics_result. This issue tracks the remaining structural cleanup.
Problem
Modelling partial-success as a separate transport capability is a false dichotomy. Every GraphQL response can carry data and errors together; all-or-nothing is just "get the result, then raise if any errors." Baking that split into the seam means:
- the seam has two verbs and two protocols where one of each would do
PartialGraphQLExecutor is a second Protocol with exactly one implementation and one test fake, i.e. speculative interface segregation- the next query that needs partial handling must widen its service to
PartialGraphQLExecutor and reach for the second verb, instead of the pattern being the default
Proposed change (Rung 2)
Collapse to one result type and one protocol.
- The primitive (e.g.
execute returning GraphQLResult{data, errors}) is the only thing the seam really exposes. It performs the effect and never decides what the errors mean. Genuine transport/network failures still raise.
execute_query stays as the raise-on-error convenience the other ~152 tools use, reimplemented as a thin wrapper: run the primitive, raise if errors, return data.- Delete
PartialGraphQLExecutor. Services depend on the one protocol; a service that wants partials reads errors off the result and hands it to its own pure classifier.
- Each domain that needs partial handling owns a pure classifier (as observability already does with
_normalize_execution_metrics_result).
The _on_graphql_error hook (InternalApiClient's [code=...][correlation_id=...] envelope) lands cleanly in this shape: the raising wrapper applies it; the primitive returns raw error dicts.
Why later, not now
This touches the shared executor contract and every caller of execute_query, so it deserves its own PR and review rather than riding on the execution-metrics feature. Rung 1 is a strict subset of this, so nothing here is blocked by having shipped it first.
Acceptance
- one execution protocol;
PartialGraphQLExecutor removed
- one primitive returning data + errors;
execute_query reimplemented over it with unchanged external behavior for existing callers
_on_graphql_error behavior preserved for InternalApiClient- observability partial-success path unchanged from a caller's perspective; full SDK + MCP suites green
References
Context
PR #349 introduced GraphQL partial-success handling for
automations.executionMetrics: a response can carry readabledataalongside per-nodeerrors(e.g.PERMISSION_DENIED) in one payload, and we want one denied automation not to drop the rest of the batch.That PR modelled "partial" as a second capability on the execution seam:
execute_query(all-or-nothing, returnsdict) andexecute_query_allow_partial(returnsPartialQueryResult)GraphQLExecutorandPartialGraphQLExecutor extends GraphQLExecutorA follow-up already landed on that PR (Rung 1): the fatal-vs-partial decision was de-smeared.
execute_query_allow_partialno longer classifies. It always returns aPartialQueryResult(a fully null response yieldsdata={}), and the single "no readable data means failure" decision now lives in one pure function,_normalize_execution_metrics_result. This issue tracks the remaining structural cleanup.Problem
Modelling partial-success as a separate transport capability is a false dichotomy. Every GraphQL response can carry
dataanderrorstogether; all-or-nothing is just "get the result, then raise if any errors." Baking that split into the seam means:PartialGraphQLExecutoris a second Protocol with exactly one implementation and one test fake, i.e. speculative interface segregationPartialGraphQLExecutorand reach for the second verb, instead of the pattern being the defaultProposed change (Rung 2)
Collapse to one result type and one protocol.
executereturningGraphQLResult{data, errors}) is the only thing the seam really exposes. It performs the effect and never decides what the errors mean. Genuine transport/network failures still raise.execute_querystays as the raise-on-error convenience the other ~152 tools use, reimplemented as a thin wrapper: run the primitive, raise iferrors, returndata.PartialGraphQLExecutor. Services depend on the one protocol; a service that wants partials readserrorsoff the result and hands it to its own pure classifier._normalize_execution_metrics_result).The
_on_graphql_errorhook (InternalApiClient's[code=...][correlation_id=...]envelope) lands cleanly in this shape: the raising wrapper applies it; the primitive returns raw error dicts.Why later, not now
This touches the shared executor contract and every caller of
execute_query, so it deserves its own PR and review rather than riding on the execution-metrics feature. Rung 1 is a strict subset of this, so nothing here is blocked by having shipped it first.Acceptance
PartialGraphQLExecutorremovedexecute_queryreimplemented over it with unchanged external behavior for existing callers_on_graphql_errorbehavior preserved for InternalApiClientReferences
execute_query_allow_partialno longer classifies;_normalize_execution_metrics_resultis the single decision point.