refactor(sdk): move GraphQL error policy out of the transport seam [#354 Rung 3]#394
Draft
Danielmoraisg wants to merge 3 commits into
Draft
refactor(sdk): move GraphQL error policy out of the transport seam [#354 Rung 3]#394Danielmoraisg wants to merge 3 commits into
Danielmoraisg wants to merge 3 commits into
Conversation
…esult type Collapse partial-success handling from a second protocol and a partial-specific verb into the default shape of one primitive (issue #354, Rung 2). - Replace PartialQueryResult with GraphQLResult and delete PartialGraphQLExecutor. - GraphQLExecutor now exposes execute (the primitive that returns data and errors together, deciding nothing about what the errors mean) and execute_query (the raise-on-error convenience reimplemented over it). Transport and network failures still raise. - Preserve _on_graphql_error: execute_query applies the internal-API envelope as a ValueError, while formatter-less endpoints re-raise a TransportQueryError carrying the structured errors the MCP tool layer reads. - ObservabilityService depends on the one protocol and calls execute, handing errors to its own classifier. Refs #354
Rung 3 of #354. The seam now returns owned types and raises one owned exception; the [code=...] string envelope round-trip is gone. - Add PipefyGraphQLError (owned, carries the structured errors list) and the pure data_or_raise. execute_query is now data_or_raise over the execute primitive and raises PipefyGraphQLError for every endpoint (no gql type leaks to callers). - Drop the per-endpoint on_graphql_error formatter and delete internal_api_errors. - Delete the [code=...] [correlation_id=...] envelope and its three parsers in the MCP tool layer; codes and correlation ids come off the structured errors for both endpoints (extract_graphql_correlation_id reads errors[].extensions). - Migrate the except TransportQueryError sites (SDK services and CLI) to PipefyGraphQLError; unify the two portal permission mappers into one. - Export PipefyGraphQLError from pipefy_sdk. The protocol keeps execute_query as the raise-on-error default so the ~152 callers are unchanged and cannot silently drop errors; execute stays for partial-success callers. Refs #354
- Guard _graphql_error_message against a non-dict error item, matching the defensive isinstance checks in the MCP tool-layer extractors; add a regression test. - Convert a stale AI-automation test to raise a structured PipefyGraphQLError instead of the deleted [code=...] envelope string (it had passed vacuously). - Drop the removed marker-stripping line from enrich_behavior_error's docstring; rename four tests whose names referenced the deleted envelope and parsers. Refs #354
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
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.
What
Rung 3 of #354: move GraphQL error policy out of the transport seam.
Rung 2 collapsed the seam to one protocol and one result type. This rung takes the two decisions the seam still baked in (raise-on-error and how to format the error) out of the transport layer and into the pure core, and deletes the
[code=...] [correlation_id=...]string envelope round-trip.PipefyGraphQLError(owned, carries the raw structurederrorslist) and the puredata_or_raise(GraphQLResult) -> dict.execute_queryis nowdata_or_raiseover theexecuteprimitive and raisesPipefyGraphQLErrorfor every endpoint. No gql transport type leaks to services, CLI, or MCP.on_graphql_errorformatter and deleteinternal_api_errors.py.[code=...] [correlation_id=...]envelope and its three parsers in the MCP tool layer. Codes and correlation ids now come off the structurederrorsfor both endpoints (extract_graphql_correlation_idreadserrors[].extensions, mirroringextract_graphql_error_codes). This removes a serialize-then-reparse round-trip: the envelope only existed to encode codes into a string that the tool layer regexed back out.except TransportQueryErrorsites (SDK services + CLI) toexcept PipefyGraphQLError, and unify portal_service's two permission mappers into one structured mapper.PipefyGraphQLErrorfrompipefy_sdk.Why the seam keeps two methods
The protocol still exposes
execute(the primitive) andexecute_query(the raise-on-error convenience). Collapsing to a singleexecutewas considered and rejected: with no static type checker in this repo,(await ex.execute(...)).datawould silently drop GraphQL errors, and ~140 of ~142 call sites want raise-on-error. Keepingexecute_queryas the ergonomic default means those callers cannot forget to raise, and it is not a second transport capability (it does no I/O; it isdata_or_raiseoverexecute).Behavior
execute_querycallers are unchanged at the call site. On GraphQL errors they now catchPipefyGraphQLErrorinstead of a gqlTransportQueryError/ aValueErrorenvelope.debug=True, now read from the structurederrors.TransportErrorsubclasses, unchanged.message (code)format) instead of 2. Exit 2 is the usage-error code, so a server-side GraphQL failure exiting 2 was wrong; this aligns internal-API CLI errors with public-API ones. Permission errors are unchanged. No CLI test pinned the old exit 2 (every exit-2 assertion is a client-side validation path that never reaches the API).Testing
uv run pytest packages -m "not integration"-> 3068 passed, 5 skipped. Ruff check + format clean.data_or_raise/PipefyGraphQLErrorunit tests; converted the error-simulation tests across SDK, MCP, and CLI to raisePipefyGraphQLErrorwith structured errors.Stacking
Stacked on #382 (Rung 2); this PR targets that branch so its diff is only the Rung 3 delta. It retargets to
devautomatically once #382 merges. Tracked under #354.