docs: add specification for the error catalogue in the SDK - #1266
Draft
ogenstad wants to merge 1 commit into
Draft
docs: add specification for the error catalogue in the SDK#1266ogenstad wants to merge 1 commit into
ogenstad wants to merge 1 commit into
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
a35e9c1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9b3c3773.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.
10 issues found across 9 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:113">
P3: Scenario 5's expected result says the grep of `"Expired Signature"` returns "exactly one site", but the string lives in two places today — the async `handle_relogin` and the sync `handle_relogin_sync` in infrahub_sdk/client.py (lines 115 and 129). Since FR-014 requires async/sync parity and nothing in the spec or contracts consolidates these two wrappers into a shared fallback helper, the legacy string will still be matched in both relogin paths, so the grep will return two sites. Either expect two sites or specify that the fallback is extracted into a single shared helper used by both transports.</violation>
</file>
<file name="dev/specs/ifc-3034-error-catalogue/research.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/research.md:9">
P2: The survey states the catalogue 'holds 15 codes' with 'Twelve declare a non-auth status' and 'three declare 401/403', but opsmill/infrahub/schema/error-catalogue.json currently has 14 codes: 11 non-auth and 3 auth (statuses 400/404/422/423/500 vs 401/403). The same off-by-one appears in R5 ('the remaining twelve with GraphQLError alone') and R2 ('all 15 codes'). Correct the count to 14/11 so the spec does not misstate the artefact it is built around.</violation>
<violation number="2" location="dev/specs/ifc-3034-error-catalogue/research.md:203">
P2: UNIQUENESS_VIOLATION, used as the primary worked example throughout (R3 name derivation, R6 generated class, R12 integration case) and as US1 acceptance scenario 1 in spec.md, does not exist in opsmill/infrahub/schema/error-catalogue.json. The catalogue's constraint-style code is ATTRIBUTE_CONSTRAINT_VIOLATION (422). Since R3's design asserts derived names and this code drives the feature's headline example, reconcile the exemplar with an actual catalogue code (e.g. ATTRIBUTE_CONSTRAINT_VIOLATION) or confirm the code is being added, before the spec drives the generator and the US1 acceptance scenario.</violation>
</file>
<file name="dev/specs/ifc-3034-error-catalogue/plan.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/plan.md:76">
P2: The Constraints section says the raised class is a function of the response's first error code alone, "never of payload validity... or which transport observed it". That contradicts the spec this plan implements: FR-004 falls back to the generic class when the matched payload does not validate, and FR-012 selects that fallback by the transport observed (GraphQL vs HTTP 401/403), explicitly not by the code's declared status. An implementer following this constraint would route a recognised 401/403 code whose payload fails validation to the authentication branch, losing the `except GraphQLError` coverage FR-018 requires. Reword the constraint to match FR-004/FR-012.</violation>
<violation number="2" location="dev/specs/ifc-3034-error-catalogue/plan.md:162">
P3: `/Users/patrick/Code/opsmill/infrahub` is a local developer path that will not resolve for other contributors. Reference the repository by its slug (opsmill/infrahub) as the surrounding text already does.</violation>
</file>
<file name="dev/specs/ifc-3034-error-catalogue/spec.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/spec.md:242">
P3: FR-002 says to parse the envelope onto "the base GraphQL error itself", but FR-001 just introduced a shared base class (referred to as `ApiError` in User Story 3) above both the GraphQL and authentication branches, and it is that base which carries the code. "Base GraphQL error" could mean either class. Since FR-028 tells consumers they can rely on the code being readable for any server version, the spec should name the class explicitly so the implementation parses onto the right one.</violation>
<violation number="2" location="dev/specs/ifc-3034-error-catalogue/spec.md:244">
P3: The spec is internally inconsistent about the no-code contract: FR-003 and FR-012 say `code` is "absent" when no string code was on the wire, while User Story 2 scenario 3 and User Story 3 scenario 3 promise `exc.code is None`. "Absent" (AttributeError on access) and `None` are different observable contracts, and FR-028 documents this cross-version behavior for external consumers. Pick one — the scenarios imply the attribute always exists and is `None` — and state it consistently in FR-003, FR-012, and the scenarios.</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:95">
P3: The Messages section claims every catalogued failure's message names "the code and the server's message", but a client-side `NodeNotFoundError` is now a catalogued class re-rooted under `GraphQLError` with an empty `errors` list and no server response, so there is no server message to name. Qualify the guarantee so implementers do not assume a server message exists for client-side raises.</violation>
</file>
<file name="dev/specs/ifc-3034-error-catalogue/data-model.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/data-model.md:14">
P3: The intro claim "nothing here is typed Any" is contradicted two paragraphs later by the ApiError table, where `extensions` is typed `dict[str, Any]`. Qualify the sentence so the spec is self-consistent, e.g. "no payload attribute is typed Any".</violation>
<violation number="2" location="dev/specs/ifc-3034-error-catalogue/data-model.md:47">
P2: The documented `exc.code is not None` distinction does not work for the three adopted classes. They have no class-level `code` (only `CODE` for the generator), and the ApiError table documents per-instance `code` only for unrecognised codes — but a server-reported adopted error uses a recognised code, so nothing documented sets `code` on it. State that the factory sets `code` per-instance on adopted classes too, or `exc.code` will be None for server-raised NodeNotFoundError/BranchNotFoundError/SchemaNotFoundError.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| joined, with `Error` appended only when the result does not already end in `Error`. The payload model | ||
| name is `data_schema.title` verbatim. | ||
|
|
||
| Worked examples: `UNIQUENESS_VIOLATION` → `UniquenessViolationError` / `UniquenessViolationData`; |
Contributor
There was a problem hiding this comment.
P2: UNIQUENESS_VIOLATION, used as the primary worked example throughout (R3 name derivation, R6 generated class, R12 integration case) and as US1 acceptance scenario 1 in spec.md, does not exist in opsmill/infrahub/schema/error-catalogue.json. The catalogue's constraint-style code is ATTRIBUTE_CONSTRAINT_VIOLATION (422). Since R3's design asserts derived names and this code drives the feature's headline example, reconcile the exemplar with an actual catalogue code (e.g. ATTRIBUTE_CONSTRAINT_VIOLATION) or confirm the code is being added, before the spec drives the generator and the US1 acceptance scenario.
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/research.md, line 203:
<comment>UNIQUENESS_VIOLATION, used as the primary worked example throughout (R3 name derivation, R6 generated class, R12 integration case) and as US1 acceptance scenario 1 in spec.md, does not exist in opsmill/infrahub/schema/error-catalogue.json. The catalogue's constraint-style code is ATTRIBUTE_CONSTRAINT_VIOLATION (422). Since R3's design asserts derived names and this code drives the feature's headline example, reconcile the exemplar with an actual catalogue code (e.g. ATTRIBUTE_CONSTRAINT_VIOLATION) or confirm the code is being added, before the spec drives the generator and the US1 acceptance scenario.</comment>
<file context>
@@ -0,0 +1,676 @@
+joined, with `Error` appended only when the result does not already end in `Error`. The payload model
+name is `data_schema.title` verbatim.
+
+Worked examples: `UNIQUENESS_VIOLATION` → `UniquenessViolationError` / `UniquenessViolationData`;
+`UNDEFINED_ERROR` → `UndefinedError` (not `UndefinedErrorError`) / `UndefinedErrorData`;
+`MERGE_IN_PROGRESS` → `MergeInProgressError` / `MergeInProgressData`.
</file context>
|
|
||
| ## Survey findings the decisions rest on | ||
|
|
||
| The catalogue (`schema/error-catalogue.json`, `infrahub_catalogue_version: "1"`) holds 15 codes. |
Contributor
There was a problem hiding this comment.
P2: The survey states the catalogue 'holds 15 codes' with 'Twelve declare a non-auth status' and 'three declare 401/403', but opsmill/infrahub/schema/error-catalogue.json currently has 14 codes: 11 non-auth and 3 auth (statuses 400/404/422/423/500 vs 401/403). The same off-by-one appears in R5 ('the remaining twelve with GraphQLError alone') and R2 ('all 15 codes'). Correct the count to 14/11 so the spec does not misstate the artefact it is built around.
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/research.md, line 9:
<comment>The survey states the catalogue 'holds 15 codes' with 'Twelve declare a non-auth status' and 'three declare 401/403', but opsmill/infrahub/schema/error-catalogue.json currently has 14 codes: 11 non-auth and 3 auth (statuses 400/404/422/423/500 vs 401/403). The same off-by-one appears in R5 ('the remaining twelve with GraphQLError alone') and R2 ('all 15 codes'). Correct the count to 14/11 so the spec does not misstate the artefact it is built around.</comment>
<file context>
@@ -0,0 +1,676 @@
+
+## Survey findings the decisions rest on
+
+The catalogue (`schema/error-catalogue.json`, `infrahub_catalogue_version: "1"`) holds 15 codes.
+Twelve declare a non-auth status (400, 404, 422, 423, 500) and three declare 401/403
+(`AUTHENTICATION_REQUIRED`, `TOKEN_EXPIRED`, `PERMISSION_DENIED`). Two codes declare an empty payload
</file context>
Suggested change
| The catalogue (`schema/error-catalogue.json`, `infrahub_catalogue_version: "1"`) holds 15 codes. | |
| The catalogue (`schema/error-catalogue.json`, `infrahub_catalogue_version: "1"`) holds 14 codes. Eleven declare a non-auth status (400, 404, 422, 423, 500) and three declare 401/403 |
| - `infrahub_sdk.exceptions` is the one supported import path for every exception, and no name | ||
| importable from it today may stop being importable from it. The restructuring must be invisible from | ||
| outside the package. | ||
| - The raised class is a function of the response's first error code alone — never of payload validity, |
Contributor
There was a problem hiding this comment.
P2: The Constraints section says the raised class is a function of the response's first error code alone, "never of payload validity... or which transport observed it". That contradicts the spec this plan implements: FR-004 falls back to the generic class when the matched payload does not validate, and FR-012 selects that fallback by the transport observed (GraphQL vs HTTP 401/403), explicitly not by the code's declared status. An implementer following this constraint would route a recognised 401/403 code whose payload fails validation to the authentication branch, losing the except GraphQLError coverage FR-018 requires. Reword the constraint to match FR-004/FR-012.
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/plan.md, line 76:
<comment>The Constraints section says the raised class is a function of the response's first error code alone, "never of payload validity... or which transport observed it". That contradicts the spec this plan implements: FR-004 falls back to the generic class when the matched payload does not validate, and FR-012 selects that fallback by the transport observed (GraphQL vs HTTP 401/403), explicitly not by the code's declared status. An implementer following this constraint would route a recognised 401/403 code whose payload fails validation to the authentication branch, losing the `except GraphQLError` coverage FR-018 requires. Reword the constraint to match FR-004/FR-012.</comment>
<file context>
@@ -0,0 +1,214 @@
+- `infrahub_sdk.exceptions` is the one supported import path for every exception, and no name
+ importable from it today may stop being importable from it. The restructuring must be invisible from
+ outside the package.
+- The raised class is a function of the response's first error code alone — never of payload validity,
+ binding freshness, or which transport observed it.
+
</file context>
Suggested change
| - The raised class is a function of the response's first error code alone — never of payload validity, | |
| - The raised class is selected by the response's first error code, but the generic fallback is a function of the transport observed: a missing, unrecognised, or pre-catalogue integer code, or a matched payload that does not validate, falls back to the generic class for the transport that carried the response — never to one chosen by the code's declared HTTP status. |
|
|
||
| | Attribute | Type | Notes | | ||
| |-----------|------|-------| | ||
| | `code` | `str \| None` | A catalogue code string, or `None`. Never an integer, so the REST envelope's integer `code` cannot be mistaken for a catalogue code (FR-003). Set as a class attribute on generated classes; set per-instance by the factory when a code is present but unrecognised. | |
Contributor
There was a problem hiding this comment.
P2: The documented exc.code is not None distinction does not work for the three adopted classes. They have no class-level code (only CODE for the generator), and the ApiError table documents per-instance code only for unrecognised codes — but a server-reported adopted error uses a recognised code, so nothing documented sets code on it. State that the factory sets code per-instance on adopted classes too, or exc.code will be None for server-raised NodeNotFoundError/BranchNotFoundError/SchemaNotFoundError.
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/data-model.md, line 47:
<comment>The documented `exc.code is not None` distinction does not work for the three adopted classes. They have no class-level `code` (only `CODE` for the generator), and the ApiError table documents per-instance `code` only for unrecognised codes — but a server-reported adopted error uses a recognised code, so nothing documented sets `code` on it. State that the factory sets `code` per-instance on adopted classes too, or `exc.code` will be None for server-raised NodeNotFoundError/BranchNotFoundError/SchemaNotFoundError.</comment>
<file context>
@@ -0,0 +1,177 @@
+
+| Attribute | Type | Notes |
+|-----------|------|-------|
+| `code` | `str \| None` | A catalogue code string, or `None`. Never an integer, so the REST envelope's integer `code` cannot be mistaken for a catalogue code (FR-003). Set as a class attribute on generated classes; set per-instance by the factory when a code is present but unrecognised. |
+| `http_status` | `int \| None` | The code's catalogue-declared status, not the status observed on the wire. `None` when no code resolved. The wire value stays available in `extensions` and can legitimately differ — the server replaces a declared 500 with the real HTTP status when it has a more accurate one. |
+| `extensions` | `dict[str, Any] \| None` | The raw `extensions` mapping of the governing error, so nothing the SDK does not model is lost. `Any` here is the honest type of decoded JSON, not an escape hatch: the mapping's value types genuinely are not known at the type level. |
</file context>
Suggested change
| | `code` | `str \| None` | A catalogue code string, or `None`. Never an integer, so the REST envelope's integer `code` cannot be mistaken for a catalogue code (FR-003). Set as a class attribute on generated classes; set per-instance by the factory when a code is present but unrecognised. | | |
| | `code` | `str \| None` | A catalogue code string, or `None`. Never an integer, so the REST envelope's integer `code` cannot be mistaken for a catalogue code (FR-003). Set as a class attribute on generated classes; set per-instance by the factory when a code is present — recognised codes on the adopted classes and unrecognised codes alike. | |
| ``` | ||
|
|
||
| Expected: a refresh is attempted for a 401 carrying `TOKEN_EXPIRED` and for a 401 carrying the legacy | ||
| `"Expired Signature"` message, and not for an unrelated 401. The `grep` returns exactly one site — the |
Contributor
There was a problem hiding this comment.
P3: Scenario 5's expected result says the grep of "Expired Signature" returns "exactly one site", but the string lives in two places today — the async handle_relogin and the sync handle_relogin_sync in infrahub_sdk/client.py (lines 115 and 129). Since FR-014 requires async/sync parity and nothing in the spec or contracts consolidates these two wrappers into a shared fallback helper, the legacy string will still be matched in both relogin paths, so the grep will return two sites. Either expect two sites or specify that the fallback is extracted into a single shared helper used by both transports.
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 113:
<comment>Scenario 5's expected result says the grep of `"Expired Signature"` returns "exactly one site", but the string lives in two places today — the async `handle_relogin` and the sync `handle_relogin_sync` in infrahub_sdk/client.py (lines 115 and 129). Since FR-014 requires async/sync parity and nothing in the spec or contracts consolidates these two wrappers into a shared fallback helper, the legacy string will still be matched in both relogin paths, so the grep will return two sites. Either expect two sites or specify that the fallback is extracted into a single shared helper used by both transports.</comment>
<file context>
@@ -0,0 +1,196 @@
+```
+
+Expected: a refresh is attempted for a 401 carrying `TOKEN_EXPIRED` and for a 401 carrying the legacy
+`"Expired Signature"` message, and not for an unrelated 401. The `grep` returns exactly one site — the
+documented pre-catalogue fallback — and no other message match for a catalogued failure. The GraphQL
+schema-validation probing used for server feature detection is out of scope and still present.
</file context>
| tasks.py # Add `exceptions` to packages_to_ignore for API-doc generation | ||
| ``` | ||
|
|
||
| Infrahub repository (`/Users/patrick/Code/opsmill/infrahub`, requirements FR-025 to FR-027): |
Contributor
There was a problem hiding this comment.
P3: /Users/patrick/Code/opsmill/infrahub is a local developer path that will not resolve for other contributors. Reference the repository by its slug (opsmill/infrahub) as the surrounding text already does.
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/plan.md, line 162:
<comment>`/Users/patrick/Code/opsmill/infrahub` is a local developer path that will not resolve for other contributors. Reference the repository by its slug (opsmill/infrahub) as the surrounding text already does.</comment>
<file context>
@@ -0,0 +1,214 @@
+tasks.py # Add `exceptions` to packages_to_ignore for API-doc generation
+```
+
+Infrahub repository (`/Users/patrick/Code/opsmill/infrahub`, requirements FR-025 to FR-027):
+
+```text
</file context>
Suggested change
| Infrahub repository (`/Users/patrick/Code/opsmill/infrahub`, requirements FR-025 to FR-027): | |
| Infrahub repository (opsmill/infrahub, requirements FR-025 to FR-027): |
| both the GraphQL and the authentication branches descend. The base MUST NOT declare an attribute for | ||
| the typed payload: a payload's fields belong to the specific class that has a type for them, and a | ||
| base-level payload attribute could only be typed loosely enough to be useless. | ||
| - **FR-002**: The SDK MUST parse the error envelope onto the base GraphQL error itself, so the code is |
Contributor
There was a problem hiding this comment.
P3: FR-002 says to parse the envelope onto "the base GraphQL error itself", but FR-001 just introduced a shared base class (referred to as ApiError in User Story 3) above both the GraphQL and authentication branches, and it is that base which carries the code. "Base GraphQL error" could mean either class. Since FR-028 tells consumers they can rely on the code being readable for any server version, the spec should name the class explicitly so the implementation parses onto the right one.
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/spec.md, line 242:
<comment>FR-002 says to parse the envelope onto "the base GraphQL error itself", but FR-001 just introduced a shared base class (referred to as `ApiError` in User Story 3) above both the GraphQL and authentication branches, and it is that base which carries the code. "Base GraphQL error" could mean either class. Since FR-028 tells consumers they can rely on the code being readable for any server version, the spec should name the class explicitly so the implementation parses onto the right one.</comment>
<file context>
@@ -0,0 +1,458 @@
+ both the GraphQL and the authentication branches descend. The base MUST NOT declare an attribute for
+ the typed payload: a payload's fields belong to the specific class that has a type for them, and a
+ base-level payload attribute could only be typed loosely enough to be useless.
+- **FR-002**: The SDK MUST parse the error envelope onto the base GraphQL error itself, so the code is
+ readable against any server version without regenerated bindings.
+- **FR-003**: The code attribute MUST be either a catalogue code string or absent. The REST envelope's
</file context>
| base-level payload attribute could only be typed loosely enough to be useless. | ||
| - **FR-002**: The SDK MUST parse the error envelope onto the base GraphQL error itself, so the code is | ||
| readable against any server version without regenerated bindings. | ||
| - **FR-003**: The code attribute MUST be either a catalogue code string or absent. The REST envelope's |
Contributor
There was a problem hiding this comment.
P3: The spec is internally inconsistent about the no-code contract: FR-003 and FR-012 say code is "absent" when no string code was on the wire, while User Story 2 scenario 3 and User Story 3 scenario 3 promise exc.code is None. "Absent" (AttributeError on access) and None are different observable contracts, and FR-028 documents this cross-version behavior for external consumers. Pick one — the scenarios imply the attribute always exists and is None — and state it consistently in FR-003, FR-012, and the scenarios.
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/spec.md, line 244:
<comment>The spec is internally inconsistent about the no-code contract: FR-003 and FR-012 say `code` is "absent" when no string code was on the wire, while User Story 2 scenario 3 and User Story 3 scenario 3 promise `exc.code is None`. "Absent" (AttributeError on access) and `None` are different observable contracts, and FR-028 documents this cross-version behavior for external consumers. Pick one — the scenarios imply the attribute always exists and is `None` — and state it consistently in FR-003, FR-012, and the scenarios.</comment>
<file context>
@@ -0,0 +1,458 @@
+ base-level payload attribute could only be typed loosely enough to be useless.
+- **FR-002**: The SDK MUST parse the error envelope onto the base GraphQL error itself, so the code is
+ readable against any server version without regenerated bindings.
+- **FR-003**: The code attribute MUST be either a catalogue code string or absent. The REST envelope's
+ integer `code` MUST NOT be surfaced through it; the HTTP status is already available separately.
+- **FR-004**: Payload parsing MUST tolerate unknown fields, which is the inverse of the server's
</file context>
Comment on lines
+95
to
+96
| A catalogued failure's message names the code and the server's message and contains no query text. | ||
| An uncatalogued failure's message is byte-identical to today's, query text included. The query is |
Contributor
There was a problem hiding this comment.
P3: The Messages section claims every catalogued failure's message names "the code and the server's message", but a client-side NodeNotFoundError is now a catalogued class re-rooted under GraphQLError with an empty errors list and no server response, so there is no server message to name. Qualify the guarantee so implementers do not assume a server message exists for client-side raises.
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 95:
<comment>The Messages section claims every catalogued failure's message names "the code and the server's message", but a client-side `NodeNotFoundError` is now a catalogued class re-rooted under `GraphQLError` with an empty `errors` list and no server response, so there is no server message to name. Qualify the guarantee so implementers do not assume a server message exists for client-side raises.</comment>
<file context>
@@ -0,0 +1,124 @@
+
+## Messages
+
+A catalogued failure's message names the code and the server's message and contains no query text.
+An uncatalogued failure's message is byte-identical to today's, query text included. The query is
+available as an attribute in both cases.
</file context>
Suggested change
| A catalogued failure's message names the code and the server's message and contains no query text. | |
| An uncatalogued failure's message is byte-identical to today's, query text included. The query is | |
| A server-reported catalogued failure's message names the code and the server's message and contains no query text; a client-side `NodeNotFoundError` names the code and the identifier, with no server message. An uncatalogued failure's message is byte-identical to today's, query text included. The query is available as an attribute in both cases. |
Comment on lines
+14
to
+15
| object. No class in this design exposes a `data` attribute, and nothing here is typed `Any` — see | ||
| [research.md](./research.md) R6. |
Contributor
There was a problem hiding this comment.
P3: The intro claim "nothing here is typed Any" is contradicted two paragraphs later by the ApiError table, where extensions is typed dict[str, Any]. Qualify the sentence so the spec is self-consistent, e.g. "no payload attribute is typed Any".
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/data-model.md, line 14:
<comment>The intro claim "nothing here is typed Any" is contradicted two paragraphs later by the ApiError table, where `extensions` is typed `dict[str, Any]`. Qualify the sentence so the spec is self-consistent, e.g. "no payload attribute is typed Any".</comment>
<file context>
@@ -0,0 +1,177 @@
+downward.
+
+The payload of a catalogued error is read as **typed attributes on the exception**, not as a payload
+object. No class in this design exposes a `data` attribute, and nothing here is typed `Any` — see
+[research.md](./research.md) R6.
+
</file context>
Suggested change
| object. No class in this design exposes a `data` attribute, and nothing here is typed `Any` — see | |
| [research.md](./research.md) R6. | |
| The payload of a catalogued error is read as **typed attributes on the exception**, not as a payload | |
| object. No class in this design exposes a `data` attribute, and no payload attribute is typed `Any` — see |
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.Written for commit a35e9c1. Summary will update on new commits.