docs: add specification for the error catalogue in the SDK - #1266
Draft
ogenstad wants to merge 2 commits into
Draft
docs: add specification for the error catalogue in the SDK#1266ogenstad wants to merge 2 commits into
ogenstad wants to merge 2 commits into
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
57a7d34
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8276b218.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-error-catalogue-ifc-3034.infrahub-sdk-python.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-3034-error-catalogue/quickstart.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/quickstart.md:1">
P3: This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,196 @@ | |||
| # Quickstart: validating the error catalogue in the SDK | |||
Contributor
There was a problem hiding this comment.
P3: This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/quickstart.md, line 1:
<comment>This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.</comment>
<file context>
@@ -0,0 +1,196 @@
+# Quickstart: validating the error catalogue in the SDK
+
+Runnable checks that prove the feature works end to end. Each scenario names what it proves and the
</file context>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Specification, implementation plan, research decisions, data model, interface contracts, and a validation quickstart for making ordinary SDK operations raise the specific exception for the failure the server reported, on both the async and sync clients, without ever raising on a payload the SDK does not recognise. The design in brief: - A new `ApiError` base carries the parsed envelope — catalogue code, declared HTTP status, raw extensions, and the server's error list — with `GraphQLError` and `AuthenticationError` descending from it. One raise-time factory serves every existing raise site, so the code is readable against any server version even with no generated bindings present. - A catalogued error's payload is read as directly typed attributes on the exception (`exc.node_kind`, `exc.fields`), typed exactly as the catalogue declares them. The pydantic payload model validates the envelope and populates them; it is not the access path. Nothing is typed `Any` beyond raw decoded JSON, and no type-check suppression is anticipated. - `infrahub_sdk/exceptions.py` becomes a strictly layered package — hand-written base, generated catalogue, factory, façade — with imports pointing only downward and a test enforcing it. `infrahub_sdk.exceptions` remains the one supported import path, and a snapshot test pins that no name importable from it disappears. - Codes declaring 401 or 403 descend from both branches. Infrahub returns HTTP 200 for resolver-raised errors, so a permission failure arrives on the data path inside a response `except GraphQLError` catches today. - The raised class is a function of the response's first error code and the transport observed — never of payload validity, binding freshness, or a code's declared status. - Infrahub generates the bindings into the submodule from `backend.generate`, alongside the schema models and protocols it already generates there, and `backend.validate-generated` fails when they are stale. Three broadenings are accepted deliberately and recorded in the spec, together with the `NodeNotFoundError.identifier` widening that unification forces. Requirements FR-025 to FR-027 land in the Infrahub repository; everything else lands here.
ogenstad
force-pushed
the
pog-error-catalogue-IFC-3034
branch
from
September 2, 2026 16:04
a1e35dd to
a35e9c1
Compare
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Seven valid findings, three rejected. Most had been present since the original plan commit and survived six incremental reviews, which only ever read the delta. Valid: - The plan's Constraints section still said the raised class never depends on payload validity or on the transport observed, contradicting FR-004 and FR-012. Stale since the E3 reversal, and it would have led an implementer to route a payload-invalid 401/403 code to the authentication branch and lose the `except GraphQLError` coverage FR-018 requires. - Nothing documented set `code` on the three adopted classes, so the `exc.code is not None` test for "came from the server" could not work. The factory sets it per-instance there; a class attribute cannot, since the same class must report None on a client-side raise. - FR-002's "the base GraphQL error itself" was ambiguous once FR-001 introduced a shared base above both branches. It now names that base and excludes the per-code classes. - FR-003 and FR-012 described `code` as "absent" while two acceptance scenarios promise `None` — different observable contracts. Standardised on always-exists-and-may-be-None. - The Messages guarantee claimed every catalogued failure names the server's message, but three catalogued classes can be raised client-side with no server response. Qualified in FR-022 and the contract. - data-model.md said "nothing here is typed Any" two paragraphs above an `extensions: dict[str, Any]` row. - A local absolute developer path in plan.md. Rejected, but the gap behind two of them fixed: the catalogue counts and the `UNIQUENESS_VIOLATION` example are correct for `opsmill/infrahub@develop`, which this feature pairs with, and wrong only against the stable line the reviewer read. Nothing in the artefacts said which ref the numbers came from, so the survey now pins `develop` and records that US1 scenario 1 needs a catalogue containing that code. The third rejection — that the "Expired Signature" grep should expect two sites — misses that R10 puts both the code check and the legacy fallback in one shared helper; R10 and the quickstart now say so explicitly.
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-3034-error-catalogue/critiques/critique-20260824-161725.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/critiques/critique-20260824-161725.md:427">
P3: The trend paragraph says all seven defects "had survived since the original plan commit," but its own example — the Constraints contradiction — is dated "since the E3 reversal," and the ninth-pass section itself says only "most" survived that long. The E3 reversal landed in the Third pass, after the original plan commit. Qualify the claim with "most" so it matches the section it summarizes.</violation>
</file>
<file name="dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md:100">
P3: The new sentence says these client-side raises happen 'with no server response behind them,' but the same document's broadenings section and infrahub_sdk/file_handler.py:168 raise NodeNotFoundError from an actual REST 404 that does carry a server response and message (the `detail`). The grouping is correct for message purposes, but the justification is inaccurate. Say 'no catalogue code behind them' (the operative `exc.code is not None` test) instead of 'no server response behind them', so it does not contradict the REST 404 case listed just above.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| From the ninth: the saturation was an artefact of *incremental* review. Squashing the branch made the | ||
| reviewer read the whole diff again, and it immediately found seven real defects that had survived since | ||
| the original plan commit — including a Constraints section that had contradicted the spec since the E3 |
Contributor
There was a problem hiding this comment.
P3: The trend paragraph says all seven defects "had survived since the original plan commit," but its own example — the Constraints contradiction — is dated "since the E3 reversal," and the ninth-pass section itself says only "most" survived that long. The E3 reversal landed in the Third pass, after the original plan commit. Qualify the claim with "most" so it matches the section it summarizes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/critiques/critique-20260824-161725.md, line 427:
<comment>The trend paragraph says all seven defects "had survived since the original plan commit," but its own example — the Constraints contradiction — is dated "since the E3 reversal," and the ninth-pass section itself says only "most" survived that long. The E3 reversal landed in the Third pass, after the original plan commit. Qualify the claim with "most" so it matches the section it summarizes.</comment>
<file context>
@@ -389,6 +422,14 @@ about the text, not how much design risk remained. Declaring the documents finis
+From the ninth: the saturation was an artefact of *incremental* review. Squashing the branch made the
+reviewer read the whole diff again, and it immediately found seven real defects that had survived since
+the original plan commit — including a Constraints section that had contradicted the spec since the E3
+reversal. Rounds three through eight each reviewed only the delta, so nothing ever re-examined the parts
+that were not being edited. The lesson for the next long-lived design branch: a full-diff pass is worth
</file context>
| query is available as an attribute in both cases. | ||
|
|
||
| The qualifier matters because three catalogued classes can also be raised client-side, with no server | ||
| response behind them: `NodeNotFoundError`, `BranchNotFoundError`, and `SchemaNotFoundError`. Those |
Contributor
There was a problem hiding this comment.
P3: The new sentence says these client-side raises happen 'with no server response behind them,' but the same document's broadenings section and infrahub_sdk/file_handler.py:168 raise NodeNotFoundError from an actual REST 404 that does carry a server response and message (the detail). The grouping is correct for message purposes, but the justification is inaccurate. Say 'no catalogue code behind them' (the operative exc.code is not None test) instead of 'no server response behind them', so it does not contradict the REST 404 case listed just above.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md, line 100:
<comment>The new sentence says these client-side raises happen 'with no server response behind them,' but the same document's broadenings section and infrahub_sdk/file_handler.py:168 raise NodeNotFoundError from an actual REST 404 that does carry a server response and message (the `detail`). The grouping is correct for message purposes, but the justification is inaccurate. Say 'no catalogue code behind them' (the operative `exc.code is not None` test) instead of 'no server response behind them', so it does not contradict the REST 404 case listed just above.</comment>
<file context>
@@ -92,9 +92,14 @@ does, the generic class for the branch is raised.
+query is available as an attribute in both cases.
+
+The qualifier matters because three catalogued classes can also be raised client-side, with no server
+response behind them: `NodeNotFoundError`, `BranchNotFoundError`, and `SchemaNotFoundError`. Those
+raises keep the message they produce today, since there is no code and no server message to name. As
+everywhere else, `exc.code is not None` is the test for which case you are holding.
</file context>
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
1 participant
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.
Specification only — no behaviour change. Adds
dev/specs/ifc-3034-error-catalogue/covering how the SDK consumes Infrahub's GraphQL error catalogue, so that ordinary operations raise the specific error for the failure.GraphQLErrorremains the fallback for failures the catalogue does not cover and stays the common base class, so existingexcept GraphQLErrorcode keeps working.Ref: IFC-3034. Related: IFC-2279 (spike), INFP-468 (backend catalogue), GitHub #7498 (out of scope).
Decisions settled while drafting
ApiErrorbase above bothAuthenticationErrorandGraphQLError. Authentication failures reach consumers from the REST path as well as GraphQL, so they cannot simply be re-rooted underGraphQLError. Verified that a 401/403 on a GraphQL call is already handled as anhttpx.HTTPStatusErrorand raisesAuthenticationErrorbefore the body is parsed for GraphQL errors — soexcept GraphQLErrornever caught auth failures, and no dual inheritance is needed to preserve compatibility..codeis a catalogue string orNone. The/api/...envelope'sextensions.codeis an integer mirroring the HTTP status, a different thing with a different type; it is not surfaced through.code. The catalogue is GraphQL-only today.GraphQLError— rather than a hand-maintained per-code mapping.python_sdksubmodule, matching howprotocols.pyand the generated schema models already arrive. No copy of the catalogue schema is vendored here, so there is one freshness invariant instead of two, policed by extending Infrahub's existingvalidate-generatedcheck. No release-time gate is added on either side.NodeNotFoundError,BranchNotFoundErrorandSchemaNotFoundErrorare unified with their catalogue counterparts and re-rooted underGraphQLError, accepting thatexcept GraphQLErrornow also catches client-side lookup misses.Findings from the code survey worth a reviewer's eye
These are in the spec's Edge Cases section as specific hazards, not hypotheticals:
isinstanceladder gets shadowed.infrahub_sdk/ctl/utils.py:58-72testsGraphQLErrorat line 67 before(SchemaNotFoundError, NodeNotFoundError, ...)at line 70. Re-rooting those classes makes the later branch unreachable, silently changing CLI output for exactly the errors this feature makes specific. FR-018 requires the correction.GraphQLErrorbranch rendersexc.errors, a list of server error dicts. A unifiedNodeNotFoundErrorraised purely client-side has no server response behind it, so the list is empty.identifiercarries two types. The existing client-sideNodeNotFoundErrorhasidentifieras a mapping of filters; the catalogue payload has it as a single string. FR-016 mandates the unification; the reconciliation mechanism is left to the plan.NodeInvalidErrorsilently inheriting the re-rooting, a pre-existing call site passing a string whereGraphQLErrorexpects a list of error dicts,UNDEFINED_ERRORbeing a real code rather than the absence of one, and GraphQL data errors arriving as HTTP 200 while auth failures arrive as real 401/403 on a separate code path.Scope
Six prioritised user stories, 28 functional requirements. FR-025 to FR-027 land in the Infrahub repository (generation plus the extended drift check) and are tagged as such; everything else lands here.
Checks
rumdlclean across 131 files; Vale flags nothing in the new files. Requirements checklist atdev/specs/ifc-3034-error-catalogue/checklists/requirements.mdpasses 16/16 with no[NEEDS CLARIFICATION]markers remaining.Summary by cubic
Documents the IFC-3034 design for consuming Infrahub's GraphQL error catalogue in the Python SDK. This PR adds specifications and implementation guidance only; it does not change SDK behavior.
Proposed design
ApiErroraboveGraphQLErrorandAuthenticationError, with catalogue errors exposing typed payload fields directly..codeorNone.isinstancecompatibility requirements.python_sdksubmodule and assigns generation and drift validation to Infrahub under FR-025–FR-027.Review fixes
codeon the adopted classes, standardizedcodeas always present orNone, and qualified the Messages guarantee.Anycontradiction in data-model.md and a local developer path; pinned the catalogue survey toopsmill/infrahub@developsince counts and theUNIQUENESS_VIOLATIONexample differ on the stable line.Written for commit 57a7d34. Summary will update on new commits.