diff --git a/dev/specs/ifc-3034-error-catalogue/checklists/requirements.md b/dev/specs/ifc-3034-error-catalogue/checklists/requirements.md new file mode 100644 index 000000000..d2178078c --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/checklists/requirements.md @@ -0,0 +1,60 @@ +# Specification Quality Checklist: Error Catalogue in the Python SDK + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-08-21 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +Two checklist items were resolved by scoping rather than by rewriting, and the reasoning is recorded +here so the plan phase does not relitigate it: + +- **"No implementation details" / "written for non-technical stakeholders"** — for a library, the + exception hierarchy *is* the user-facing product, so class names, catalogue codes, and the + transport split are domain vocabulary rather than implementation leakage. The spec names those and + deliberately withholds module layout, file names, generator implementation, and test mechanics. + Recorded as an explicit assumption in the spec rather than left implicit. +- **"Success criteria are technology-agnostic"** — SC-001 through SC-008 are stated as outcomes a + consumer or reviewer can verify (a failure is handleable without reading a message; no string + matching remains; a stale artefact fails validation) rather than as internal mechanics. They do + reference exceptions and catalogue codes, which is unavoidable and correct for this feature. + +Two items were originally deferred to the plan and have since been pulled back into the spec, both +prompted by automated review of the pull request: + +- **The `identifier` contract on the unified `NodeNotFoundError`.** Deferring the whole question was + wrong: *which* attributes a consumer can read is observable API surface and belongs here, even + though the mechanism does not. FR-016 now pins the contract — every construction shape in use today + keeps working, the server-reported kind and identifier are reachable, one documented accessor works + for both cases, and any type widening is called out in release notes. Surveying the code for this + also turned up that the attribute is *already* heterogeneous: the file handler passes a plain string + where the declared type is a mapping. +- **Multi-error precedence.** FR-013 originally required only that a rule exist, which is untestable + until the rule does. It now specifies that the first error in the response governs, with the + complete list retained, and records why first-*recognised* was rejected: it would make the raised + type depend on binding freshness rather than on the response. diff --git a/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md b/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md new file mode 100644 index 000000000..f4735f9df --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/contracts/exception-hierarchy.md @@ -0,0 +1,124 @@ +# Contract: The exception hierarchy + +The SDK's public interface here is the set of names a consumer can import from +`infrahub_sdk.exceptions`, catch, and read attributes off. This is what the change promises. + +`infrahub_sdk.exceptions` is the supported import path for every exception the SDK raises, generated or +hand-written. A consumer never needs to know which module inside it defines a given class, and the +modules beneath it are internal. Every name importable from `infrahub_sdk.exceptions` before this +change is still importable from it afterwards, pinned by a test against a committed snapshot rather +than asserted. + +## Catching + +| Intent | Clause | +|--------|--------| +| Anything the server rejected, on either transport | `except ApiError` | +| Any GraphQL-path failure, including catalogued permission and token failures | `except GraphQLError` | +| Any authentication or permission failure, either transport | `except AuthenticationError` | +| One specific catalogued failure | `except UniquenessViolationError` (and so on per code) | +| Anything the SDK raises | `except Error` | + +The catalogued 401/403 classes deliberately satisfy both `except GraphQLError` and +`except AuthenticationError`, because they reach the SDK on the GraphQL transport two different ways: + +- **Inside a 200 response's `errors` array**, when the failure was raised from within a resolver. + `except GraphQLError` catches such a response today; the dual base is what keeps that true while also + making `except AuthenticationError` catch it. +- **As a real 401 or 403**, when the failure escapes before query execution. + `except AuthenticationError` catches this today. `except GraphQLError` does not, because the SDK + raises before reading the body — under this change it will, since the authentication path now resolves + the catalogue code and raises the specific class. That is a broadening, listed below. + +Every clause that worked before the change still catches what it caught before (FR-018). Three +broadenings are deliberate: + +- `except GraphQLError` now also catches node, branch, and schema lookup misses that involved no + GraphQL request at all — both the client-side ones and the REST 404 the file handler turns into a + `NodeNotFoundError` — because those classes are re-rooted under it. +- `except GraphQLError` now also catches a real 401 or 403 **whenever the SDK raises a per-code class + for it**, where it previously raised a plain `AuthenticationError`. Only those classes carry both + parents, so the condition is exactly the condition for reaching one: the code is recognised by this + SDK's bindings *and* its payload validates. If either fails, the fallback raises the generic + `AuthenticationError` for the observed transport, which is not a `GraphQLError` — unchanged from + today. +- Code that catches the generic error to inspect its message will now sometimes receive a subclass + whose message names the code instead of embedding the query. + +## Reading a caught error + +Available on every `ApiError`: + +| Attribute | Contract | +|-----------|----------| +| `code` | The catalogue code string, or `None`. Never an integer. `None` means the SDK resolved no catalogue code — a pre-catalogue server, a REST failure, an error with no `extensions`, or an integer `code` on the wire. An unrecognised string code from a newer server is still readable here. | +| `http_status` | The code's catalogue-declared status, or `None`. This is metadata about the failure, not the HTTP status of the response — a catalogued data error arrives as HTTP 200. Where the error carried an `extensions` mapping, the status the server actually returned is available as `exc.extensions["http_status"]` — guard on `exc.extensions` first, since it is `None` when the error carried none. The two can legitimately differ: the server replaces a declared 500 with the real HTTP status when it has a more accurate one. | +| the payload's fields | Not on the base. Each catalogued class carries its payload's fields as directly typed attributes — `UniquenessViolationError.node_kind` is a `str`, `.fields` a `list[str]` — typed exactly as the catalogue declares them, so a required field is never optional and needs no guard. The raw payload dict remains in `extensions["data"]` for anything forwarding it verbatim. | +| `extensions` | The raw `extensions` mapping of the governing error, or `None`. | +| `errors` | The complete server error list, unreordered — empty for a client-side raise. | +| `query`, `variables` | The GraphQL query and variables where there was one, otherwise `None`. | + +`errors`, `query`, and `variables` are readable on every `ApiError`, not only on those built from a +server response. A purely client-side `NodeNotFoundError` has an empty `errors` and `None` for the rest, +so code that catches `GraphQLError` and inspects them never has to guard for a missing attribute. + +`UNDEFINED_ERROR` is a code like any other: it means the server explicitly reported a gap in its own +catalogue, and it is not the same as an error carrying no `extensions`. + +## Cross-version behaviour + +Any SDK version talks to any server version. Parsing never raises. + +| Situation | Behaviour | +|-----------|-----------| +| A code the SDK has never heard of | The generic class for the branch is raised — `GraphQLError` for data failures, `AuthenticationError` for 401/403 — with `code` set to the string the server sent. | +| A known code whose payload gained a field | The unknown field is ignored; behaviour is unchanged. | +| A server predating the catalogue, or an error with no `extensions` | Today's behaviour exactly; `code` is `None`. | +| An integer `code` on `/graphql` from a pre-catalogue server | Not surfaced as a catalogue code; `code` is `None`. | +| A payload that violates the catalogue's own contract | The generic class for the branch, with the code still readable. The specific class's attributes are typed as the catalogue declares them, so there is nothing to populate a required one with. | + +Every fallback above is logged at debug level with the code involved, so an SDK meeting a newer server +is diagnosable in the field rather than only in tests. + +Regenerating bindings buys typed handling of newly catalogued codes. It never changes which exception +a byte-identical response produces for a code the SDK already knows, because the first error in the +response governs unconditionally — not the first *recognised* one. + +## Multiple errors in one response + +The first error in the response determines the class raised. The complete list is retained on the +exception, unreordered, and nothing is discarded. If the first error carries no code and a later one +does, the generic class for the branch is raised. + +## 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. + +Where the catalogue provides them, the server's message names the failing action and resource kind, so +that detail now appears in logs and CLI output in place of the query text that used to be there. + +## Parity + +The async and sync clients raise the same type with the same attributes for the same failure, for +every catalogued code. + +## Stability + +`infrahub_sdk.exceptions` is treated as public and is the one import path a consumer needs. That is a +stronger promise than the constitution's tiering strictly requires — only `Config`, `InfrahubClient`, +and `InfrahubClientSync` are exported at top level — and it is made deliberately, because +`infrahubctl`, the Ansible collection, and external consumers already import from it directly. + +Concretely: + +- No name is removed or renamed, and no constructor loses a signature it has today. +- Every name importable from `infrahub_sdk.exceptions` before this change remains importable from it, + which a test pins against a committed snapshot. Restructuring the module into a package must not be + observable from the outside. +- Modules beneath `infrahub_sdk.exceptions` are internal. Importing `…exceptions.catalogue` or + `…exceptions.payloads` directly is not supported, and their layout may change. +- One annotation widens: `NodeNotFoundError.identifier` becomes `Mapping[str, list[str]] | str`. It is + called out in a changelog fragment because external consumers read these attributes even though + nothing in this repository does. diff --git a/dev/specs/ifc-3034-error-catalogue/contracts/generator-contract.md b/dev/specs/ifc-3034-error-catalogue/contracts/generator-contract.md new file mode 100644 index 000000000..b824f2223 --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/contracts/generator-contract.md @@ -0,0 +1,122 @@ +# Contract: Generating the SDK's error bindings + +One artefact crosses from the Infrahub repository into the SDK. This is the contract between the two +sides. The SDK holds no copy of the catalogue schema (FR-010). + +## The artefact + +- **Source**: `schema/error-catalogue.json` in the Infrahub repository. +- **Output**: `infrahub_sdk/exceptions/catalogue.py` in the SDK submodule. +- **Owner**: Infrahub. The SDK never regenerates it, exactly as it never regenerates `protocols.py` + or its schema models. +- **Committed**: yes. Generation happens in a pull request, not at install time, so catalogue drift is + visible in the diff. + +The file is generated **in full**. There is never a hand-edited region inside a generated file, nor a +generated region inside a hand-written one. + +## What the generated module contains + +It opens with a header marking it generated and not to be edited, naming the source artefact, recording +the catalogue's `infrahub_catalogue_version`, and giving the regeneration command (FR-009) — the same +marking style as the repository's other generated files. It declares `__all__`, which is what lets the +package façade re-export it without a hand-maintained list. + +The body holds three things: + +- One pydantic payload model per catalogue code, including codes with an empty payload and including + adopted codes. These validate the envelope and supply the promoted attributes' types. +- One exception class per catalogue code the SDK has not adopted, each promoting its payload's fields + to directly typed attributes and exposing a `from_payload` classmethod. +- `CODE_TO_EXCEPTION`, mapping every catalogue code to its class — generated classes for most, + imported adopted classes for the rest. + +It imports only `infrahub_sdk.exceptions.base`, which imports nothing from inside the package. That +keeps the package's import graph one-way with no cycle: `base` → `catalogue` → `factory` → the façade. + +Codes are emitted in sorted order so that reordering the catalogue's JSON does not churn the diff. + +## Derivation rules + +No hand-maintained per-code table exists on either side. Everything is derived from the catalogue +entry: + +| Output | Derived from | +|--------|--------------| +| Exception class name | The code's parts capitalised and joined, with `Error` appended only if it does not already end in `Error`. `UNDEFINED_ERROR` → `UndefinedError`. | +| Payload model name | `data_schema.title`, verbatim. | +| Base classes | `GraphQLError` always; `http_status in {401, 403}` additionally adds `AuthenticationError`, emitted as `(GraphQLError, AuthenticationError)`. | +| `http_status` class attribute | The catalogue's declared `http_status`. | +| Docstring | The catalogue's `description` and `stability`. | +| Promoted attribute names | The payload field names, verbatim. | +| Field and attribute types | The JSON Schema mapping in [data-model.md](../data-model.md); a required field is non-optional, a nullable one carries its declared default. | + +## Adoption + +Some catalogue codes are represented by a class the SDK already ships. Those classes declare the code +they represent with a `CODE` class attribute. The generator parses +`infrahub_sdk/exceptions/base.py` with `ast`, collects every class whose body assigns a `CODE` +string, and for those codes emits an import and a map entry instead of a class definition. The payload +model is still generated. + +An adopted class supplies its own `from_payload`, since its attribute names are its existing ones +rather than the catalogue's. + +Adopting a further code later is a one-line change in the SDK's hand-written module with no generator +edit. Discovery is by parsing rather than importing, so the generator stays a pure text transform and +generation never depends on the SDK checkout being importable. The same walk collects every class name +defined in `base.py`, which is what the collision check below needs. Parsing also sidesteps a trap an +attribute walk would hit: `NodeInvalidError` inherits `CODE` from `NodeNotFoundError`, so two classes +would appear to claim the same code. + +## Failing loudly + +Generation aborts, rather than emitting a guess, when: + +- a catalogue entry has no integer `http_status`; +- a catalogue entry has no non-empty `data_schema.title`; +- a `data_schema` uses a construct outside the supported vocabulary, in which case the offending + fragment appears in the error; +- `codes` is empty or the root is not an object; +- a derived class name collides with a class already defined in `base.py` that has not declared that + code as adopted. + +The first four are the assertions the frontend generator already makes, for the same reason. The last +is specific to Python's import semantics: the SDK's façade re-exports `base` and then `catalogue`, so an +undeclared collision would let the generated class silently take the name and change what an existing +`except` clause catches. The SDK already defines `ValidationError`, `RateLimitError`, +`InvalidResponseError`, `FileNotValidError`, and `ResourceNotDefinedError` — every one of them the name +a plausible future code would derive — so this is a live hazard rather than a theoretical one. Failing +generation forces the choice (adopt the code, or rename) into the pull request that adds the code. + +## Validation + +`uv run invoke frontend.regenerate-error-bindings` regenerates the catalogue JSON, the frontend +TypeScript bindings, the docs page, and now the SDK bindings, so all catalogue-derived artefacts move +together. + +`uv run invoke backend.validate-generated` verifies the SDK artefact with +`git -C python_sdk diff --exit-code infrahub_sdk/exceptions/catalogue.py`. The diff must run inside the +submodule, because from the superproject `git diff` only sees the submodule pointer — the same reason +the existing schema-model and protocol checks are written that way. + +In CI, `backend-validate-generated` hosts this because it is the job that runs +`backend.validate-generated`. Its trigger condition gains `error_catalogue == 'true'` so a +catalogue-only change cannot slip past, and `error_catalogue_files` in `.github/file-filters.yml` +gains the submodule artefact path and the template, so editing the committed bindings or the generator +also triggers the check. + +Submodule availability is not a factor in that placement. Infrahub declares +`infrahub-sdk = { path = "python_sdk", editable = true }`, so `uv sync` fails without the submodule and +every Python job there already requires `submodules: true`. Any job that needs the submodule declares +it; the check goes where it belongs and the checkout follows. + +A catalogue change that skips regeneration therefore fails the pull request that made it (FR-026). +There is no release-time gate on either side; pull-request-time validation is the mechanism, matching +how the existing generated artefacts are treated (FR-027). + +## What regeneration does and does not buy + +Regenerating adds typed handling for newly catalogued codes. It never changes which exception a +byte-identical response produces for a code the SDK already knows, and correctness never depends on +having regenerated — an SDK with stale bindings falls back rather than failing. diff --git a/dev/specs/ifc-3034-error-catalogue/critiques/critique-20260824-161725.md b/dev/specs/ifc-3034-error-catalogue/critiques/critique-20260824-161725.md new file mode 100644 index 000000000..2e699c353 --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/critiques/critique-20260824-161725.md @@ -0,0 +1,351 @@ +# Critique Report: Error Catalogue in the Python SDK + +**Date**: 2026-08-24 +**Feature**: [spec.md](../spec.md) +**Plan**: [plan.md](../plan.md) +**Verdict**: ⚠️ PROCEED WITH UPDATES + +--- + +## Executive Summary + +The spec is unusually strong for a library change: it names the user-facing contract (classes, codes, +attributes) while withholding mechanism, it enumerates hazards found by surveying real code rather than +hypotheticals, and FR-013's rationale for first-error precedence is the kind of reasoning that prevents +a whole class of later bugs. The plan is grounded — every decision cites the file it was derived from — +and the revised exceptions-package layering is a genuine solution to the cycle rather than a mitigation +of it. + +Three findings block task generation, all of them in the same family: the plan's treatment of what +happens when a class is re-rooted or resolved. **E1** is a latent `AttributeError` — the three adopted +classes become `GraphQLError` subclasses without ever setting `errors`, `query`, or `variables`, so any +consumer (or the CLI) reading `exc.errors` off a client-side lookup miss crashes; the plan's stated +mitigation assumes an empty list where the attribute is actually absent. **E2** is a verified conflict +between FR-008 and FR-018: Infrahub's `/graphql` app returns HTTP 200 for resolver-raised errors and its +formatter maps permission failures to `PERMISSION_DENIED`, so an auth-branch code really does arrive on +the data path — and deriving its parent solely from the declared status would make `except GraphQLError` +stop catching a response it catches today. **E3** is a contract inconsistency: the plan lets a malformed +payload downgrade the *raised type*, which contradicts FR-013's own reasoning that the type must be a +pure function of the response, and the server's payload builder has a reachable fallback that emits an +empty payload for codes whose schema declares required fields. + +None of the three requires rethinking the approach; each is a local correction to the plan, and two also +want a one-line clarification in the spec. + +--- + +## Product Lens Findings 🎯 + +### Problem Validation + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| P1 | 💡 | Problem validation is well-evidenced (a spike, the backend catalogue, the frontend's generated bindings, and the SDK's own `"Expired Signature"` string match). Nothing to challenge on need. The gap is *sequencing*: US5 "bindings that cannot silently drift" is labelled P2, but the per-code classes US1 (P1) delivers are produced by the generator US5 builds, so US5 is a hard prerequisite for the P1 story. | Make the cross-repo dependency explicit in `tasks.md`: the Infrahub generator and its first hand-verified run come before the SDK-side typed-raising work can be demonstrated. Priority labels describe value, not order — say so, so the task ordering is not derived from them. | + +### User Value Assessment + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| P2 | 💡 | The plan's new `topics/error_handling.mdx` is to describe "the catalogue codes it covers". Restating 15 codes by hand in the SDK repo creates a second source of truth that rots the first time Infrahub adds one, and nothing validates it — a direct Principle VII risk. Infrahub already generates `docs/docs/reference/error-catalogue.mdx` from the same artefact. | Scope the SDK page to what is genuinely SDK-specific: the hierarchy, how to catch by branch versus by code, the cross-version guarantees, and the two accepted broadenings. Link to Infrahub's generated catalogue reference for the code list instead of duplicating it. | +| P3 | 💡 | FR-016 requires the `identifier` widening to be "called out in the change's release notes", and the plan asserts this in the Constitution Check without naming a mechanism. The repository uses towncrier (`[tool.towncrier]`, `directory = "changelog"`, `orphan_prefix = "+"`), so a release note is a file, not a promise. | Add a changelog fragment task: one entry for the typed errors, one for the `identifier` widening, one for the `except GraphQLError` broadening. Naming the fragment in the plan makes FR-016 verifiable instead of aspirational. | + +### Alternative Approaches + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| P4 | 💡 | The plan rejects "generate all of `exceptions.py`" for good reasons, and the layered package is the right call. One alternative is not recorded: doing nothing on the generation side and hand-writing 15 classes once. Worth a sentence, because a reader will ask — 15 codes is small enough that the generator's value is drift protection (US5), not typing effort. | Add the alternative to research.md R2 with that framing, so the generator's justification reads as "drift protection" rather than "avoiding 15 classes of typing". | + +### Edge Cases & UX + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| P5 | 💡 | The plan adds no observability to the fallback paths. Every cross-version case — unknown code, absent envelope, invalid payload — is silent, which is exactly the signal a maintainer wants from the field when an SDK meets a newer server. The plan mentions a debug log for payload-validation failure only. | Log at debug on every fallback, including the code that failed to resolve. Cheap, and it makes SC-004's guarantees observable in production rather than only in tests. | +| P6 | 🤔 | E2's fix changes CLI output for a resolver-raised permission failure: today it renders through `print_graphql_errors`, and after the change the earlier `AuthenticationError` branch claims it and prints "Authentication failure: …". That is arguably better, but it is a user-visible change not currently anticipated by the spec. | Confirm the new rendering is wanted, then pin it with a test. If it is not wanted, the ladder needs a third position rather than the current two. | + +### Success Measurement + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| P7 | 🤔 | SC-008 promises "a developer can catch every server-reported error, on either transport, with one `except` clause", and that clause is `except ApiError`. But `ApiError` is reachable only by deep import, which the constitution's tiering places explicitly *outside* the guaranteed-stability tier. The spec's central promise is therefore backed by a surface the constitution says may change in a minor release. | Decide whether `ApiError` (and possibly `Error`) joins `infrahub_sdk/__init__.py`'s `__all__`. This permanently enlarges the guaranteed surface, so it is a maintainer call, not a plan call — but leaving it unanswered means SC-008 promises more stability than the constitution grants. | + +--- + +## Engineering Lens Findings 🔬 + +### Architecture Soundness + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E1 | 🎯 | **Re-rooting the three adopted classes under `GraphQLError` leaves `errors`, `query`, and `variables` unset.** `NodeNotFoundError.__init__` (and `BranchNotFoundError`, `SchemaNotFoundError`) never call `GraphQLError.__init__`, so on a client-side raise those attributes do not exist at all. Any consumer doing `except GraphQLError as exc: … exc.errors` gets `AttributeError`, and the CLI's `print_graphql_errors(errors=exc.errors)` raises *before* reaching the plan's "degrade when the list is empty" fix. The plan's mitigation is built on the wrong premise: the list is not empty, the attribute is absent. | Give `ApiError` safe class-level defaults for `errors` (empty tuple or `None`, not a mutable list), `query`, and `variables`, so every descendant has them regardless of which constructor ran. Keep the renderer's degradation as well, and add a test that reads `exc.errors`, `exc.query`, and `exc.variables` off a purely client-side `NodeNotFoundError`. | +| E2 | 🎯 | **Auth-branch codes really do arrive on the data path, so deriving the parent solely from the declared status breaks FR-018.** Verified in Infrahub: `backend/infrahub/graphql/app.py:298-300` returns `status_code=200` for every executed query, and `graphql/error_formatter.py` maps resolver-raised failures to `PERMISSION_DENIED` / `AUTHENTICATION_REQUIRED` / `TOKEN_EXPIRED` inside that 200 response's `errors` array. Today such a response raises `GraphQLError`. Under FR-008 as written it would raise a class descending only from `AuthenticationError`, so an existing `except GraphQLError` around `execute_graphql` silently stops catching it — an FR-018 and SC-003 violation. The spec's edge case ("auth failures come back as real 401/403 responses handled by a separate code path") holds only for failures that escape *before* execution, which `api/exception_handlers.py:52-55` states explicitly. | Give the 401/403 codes both parents: `class PermissionDeniedError(GraphQLError, AuthenticationError)`. The diamond closes cleanly on `ApiError`, the MRO gives `GraphQLError.__init__` (correct — these only ever arise on the GraphQL transport, since REST failures raise plain `AuthenticationError` per FR-015), and both `except AuthenticationError` (US3 AS1/AS2) and `except GraphQLError` (FR-018) are satisfied. Amend FR-008 from "descend from the authentication branch" to "*additionally* descend from the authentication branch". | +| E3 | 🎯 | **The payload-validation fallback downgrades the raised type, contradicting FR-013's own rationale.** The plan falls back to the generic branch class when a payload fails validation, to buy the guarantee that `exc.data` is never `None` on a specific class. But FR-013 argues the raised type must be a pure function of the response so it never depends on anything else — and this makes it depend on payload validity. It also defeats the P1 story in the case that matters: US1 AS2 asserts `.delete()` of a missing node raises `NodeNotFoundError`, which would become a bare `GraphQLError` if the payload were malformed. This is not theoretical: `error_formatter.py:59-60` defaults `payload = UndefinedErrorData()` and only overwrites it when an `isinstance` guard matches, so the server can emit `data: {}` under a code whose schema declares `node_kind` and `identifier` required. | Invert the choice: resolve the class from the code, always, and let `data` be `Model | None` with the generated annotation `Optional`. Log at debug when validation fails. The cost is a `None` check that in practice never fires; the benefit is that the type a consumer catches is exactly what FR-013 promises. Note in the contract that a `None` `data` means the server violated its own emission contract. | +| E4 | 💡 | FR-013 requires the exception to retain the complete error list, but `AuthenticationError.__init__(message)` has nowhere to put it, and FR-015 freezes that constructor. On the auth branch the list would currently be dropped. | Fold this into E1's fix: `errors` lives on `ApiError` with a safe default, and the auth factory populates it. One change satisfies both findings. | +| E5 | 💡 | The `ast`-based adoption discovery is the right call, but the documented import-based fallback has a trap: `NodeInvalidError` *inherits* `CODE = "NODE_NOT_FOUND"` from `NodeNotFoundError`, so an attribute walk would see two classes claiming the same code and let dict ordering pick the winner. | If the fallback is ever used, filter to classes whose own `__dict__` carries `CODE`. Worth one sentence in research.md R4 so the trap is documented where the fallback is offered. | +| E6 | 💡 | The layering test as described parses module-level and `TYPE_CHECKING` imports. A function-body import (`def f(): from .factory import …`) is the classic way a cycle gets reintroduced once the obvious route is closed. | Walk every `Import`/`ImportFrom` node in the module, not just top-level ones. Same effort, closes the remaining hole. | + +### Failure Mode Analysis + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E7 | 💡 | The factory sits on the failure path of every client method, which makes it the highest-blast-radius code in the change: an unexpected exception inside it replaces a legitimate server error with an SDK `TypeError`, and the original failure is lost. The plan guards payload validation but not the resolution logic around it. | Make the factory total: wrap resolution in a `try/except Exception` that falls back to constructing today's generic error, so a factory bug degrades to current behaviour instead of masking the server's error. Test it by feeding the factory a deliberately malformed envelope (`errors` as a string — which `analyzer.py:42` already produces, and `extensions` as a list). | +| E8 | 💡 | The wire `extensions.http_status` can legitimately differ from the code's declared status: `api/exception_handlers.py:26-27` overwrites a catalogue 500 with the actual FastAPI status, so an `UNDEFINED_ERROR` can arrive declaring 500 while carrying 422. The plan asserts `exc.http_status` is the catalogue value (matching US1 AS3) without noting the observable divergence. | Keep `exc.http_status` as the catalogue value, and state in the contract that the wire value remains available via `exc.extensions["http_status"]` and may differ for `UNDEFINED_ERROR`. One sentence prevents a confusing bug report. | + +### Security & Privacy + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E9 | 💡 | No new attack surface: the change parses a response the SDK already parses, adds no dependency, and reaches no new data. The one hygiene point is that a catalogued error's message now names the code and the server's message, and `PermissionDeniedData` carries `action` and `resource_kind` — which will show up in logs and CLI output where the previous message was a wall of query text. | Nothing to change. Worth one line in the docs page noting that error messages now surface the failing action and resource kind, so anyone shipping SDK logs to a third party knows what changed. | + +### Performance & Scalability + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E10 | 💡 | Cost is one dict lookup plus one pydantic validation per failed request, on a path that already decoded JSON. Not a concern. The only scaling question is generated-module size, and 15 codes is nothing. | None. The plan's "Performance Goals: None" is the correct answer, not an omission. | + +### Testing Strategy + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E11 | 💡 | SC-006 reads "async and sync clients raise the same type with the same attributes for the same failure, across all catalogued codes", but the plan's approach — exhaustive at the factory level, parametrized at the client level — does not say which layer covers "all codes". Read literally, SC-006 asks for 15 codes × 2 clients through the client layer. | State the split explicitly: factory-level tests cover all codes exhaustively; client-level parity tests cover a representative set that exercises both branches, both transports, and the file-upload variant. That satisfies SC-006's intent and keeps the suite fast, per the constitution's unit-test speed requirement. | +| E12 | 💡 | The plan does not name a test for the two accepted broadenings. They are deliberate behaviour changes, and the constitution requires each to be pinned by a test asserting the new behaviour. | Add explicit tests: `except GraphQLError` catches a client-side `NodeNotFoundError`; a catalogued failure's message differs from the generic one while the uncatalogued message is byte-identical. | + +### Operational Readiness + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E13 | 💡 | Rollback for a library is a revert, and the plan needs no deployment strategy — correct. But the cross-repo failure mode has no story: if Infrahub regenerates bindings into the submodule and the SDK's `sdk_ref` docs are generated from the `exceptions` package, the SDK's own `docs-validate` fails on the next SDK pull request, for a change made in another repository. `exceptions.py` is not documented in `sdk_ref` today, so adding it to `packages_to_document` creates this coupling from nothing. | Flip R1's consequence: put `"exceptions"` in `packages_to_ignore` and cover the hierarchy with the hand-written topic page, which is what FR-028 actually asks for. Preserves today's docs behaviour exactly and removes a cross-repo trap. | + +### Dependencies & Integration + +| ID | Severity | Finding | Suggestion | +|----|----------|---------|------------| +| E14 | 💡 | No new runtime dependency, and the generator reuses Infrahub's existing Jinja2 and ruff pipeline — sound. The unverified assumption is the type-checking one: the plan asserts that annotating `data` concretely on a subclass of an `Any`-typed base attribute is accepted by *both* mypy and `ty`. mypy accepts it; `ty` is newer and its variance handling on attribute overrides is not something the plan has evidence for. | Spike it before the generator template is finalised: one hand-written class, both checkers. If `ty` objects, the fallback is a covariant read-only property per generated class, which the template can emit just as easily. Cheap insurance against discovering it across 15 generated classes. | +| E15 | 🤔 | The bindings cross a submodule boundary, so the SDK commit that adds `catalogue.py` and the Infrahub commit that generates it must land together. The plan says the first generation is hand-verified once, but not how the two pull requests are paired thereafter, nor what happens when Infrahub's CI regenerates against an SDK branch that is not yet merged. | Confirm the intended workflow with the maintainer: does the Infrahub pull request carry a submodule pointer bump to an SDK branch, and is that branch merged first? This is existing practice for `protocols.py`, so the answer likely exists — it just is not written down here. | + +--- + +## Cross-Lens Insights 🔗 + +| ID | Finding | Product Impact | Engineering Impact | Suggestion | +|----|---------|---------------|-------------------|------------| +| X1 | Auth codes on HTTP 200 (E2) | A consumer's working `except GraphQLError` silently stops catching permission failures — the exact class of breakage the feature exists to prevent | FR-008's derivation rule conflicts with FR-018's no-clause-loses-coverage guarantee | Dual base `(GraphQLError, AuthenticationError)` for 401/403 codes, and amend FR-008's wording to "additionally descend" | +| X2 | Payload validity decides the raised type (E3) | US1 AS2's promise ("`.delete()` on a missing node raises `NodeNotFoundError`") becomes conditional on the server's payload being well-formed, and the server has a reachable path that emits an empty payload | Contradicts FR-013's stated rationale that the raised type is a pure function of the response | Resolve the class from the code unconditionally; make `data` `Model \| None` and document that `None` means the server broke its own contract | +| X3 | Documenting the `exceptions` package in `sdk_ref` (E13, P2) | A hand-maintained code list and a generated API page both rot, and readers cannot tell which is authoritative | Creates a cross-repo CI coupling where an Infrahub-side regeneration fails an SDK pull request | Ignore the package for API-doc generation; one hand-written topic page for hierarchy and guarantees, linking to Infrahub's generated catalogue reference for codes | + +--- + +## Findings Summary + +| Metric | Count | +|--------|-------| +| 🎯 Must-Address | 3 | +| 💡 Recommendations | 16 | +| 🤔 Questions | 3 | +| Product findings | 7 | +| Engineering findings | 15 | +| Cross-lens findings | 3 | + +--- + +## Consolidated Findings Table + +| ID | Lens | Severity | Category | Finding | Suggestion | +|----|------|----------|----------|---------|------------| +| E1 | Engineering | 🎯 | Architecture | Re-rooted adopted classes never set `errors`/`query`/`variables`; consumers and the CLI hit `AttributeError`, not an empty list | Safe class-level defaults on `ApiError`; test attribute access on a client-side raise | +| E2 | Engineering | 🎯 | Architecture | Auth codes arrive inside HTTP 200 GraphQL responses, so status-derived parents make `except GraphQLError` lose coverage | Dual base `(GraphQLError, AuthenticationError)`; amend FR-008 to "additionally descend" | +| E3 | Engineering | 🎯 | Architecture | A malformed payload downgrades the raised type, contradicting FR-013 and US1 | Resolve class from code unconditionally; `data: Model \| None` | +| E4 | Engineering | 💡 | Architecture | FR-013's error-list retention is unmet on the auth branch | Put `errors` on `ApiError`; auth factory populates it | +| E5 | Engineering | 💡 | Architecture | Import-based adoption fallback double-counts `NodeInvalidError`'s inherited `CODE` | Filter on the class's own `__dict__` | +| E6 | Engineering | 💡 | Architecture | Layering test misses function-body imports | Walk all import nodes, not just module-level | +| E7 | Engineering | 💡 | Failure modes | A factory bug replaces the server's error with an SDK `TypeError` | Make the factory total, falling back to today's construction | +| E8 | Engineering | 💡 | Failure modes | Wire `http_status` can differ from the declared one for `UNDEFINED_ERROR` | Document that `exc.extensions["http_status"]` holds the wire value | +| E9 | Engineering | 💡 | Security | Messages now surface action and resource kind into logs | One documentation line; no code change | +| E10 | Engineering | 💡 | Performance | No bottleneck; "Performance Goals: None" is correct | None | +| E11 | Engineering | 💡 | Testing | SC-006's "all codes" is not mapped to a test layer | State the factory-exhaustive / client-representative split | +| E12 | Engineering | 💡 | Testing | The two accepted broadenings have no named test | Add tests pinning both | +| E13 | Engineering | 💡 | Operations | Documenting the `exceptions` package couples SDK `docs-validate` to Infrahub regeneration | Use `packages_to_ignore`; rely on the topic page | +| E14 | Engineering | 💡 | Dependencies | `ty`'s acceptance of the narrowed `data` annotation is unverified | Spike one class against mypy and `ty` first | +| E15 | Engineering | 🤔 | Integration | Cross-repo pull-request pairing for the submodule artefact is unwritten | Confirm the `protocols.py` workflow and record it | +| P1 | Product | 💡 | Problem validation | US5 (P2) is a hard prerequisite for US1 (P1) | Make the dependency explicit in `tasks.md`; priorities are value, not order | +| P2 | Product | 💡 | User value | A hand-written code list in SDK docs becomes a second source of truth | Describe hierarchy and guarantees; link to Infrahub's generated reference | +| P3 | Product | 💡 | User value | FR-016's release-note requirement has no mechanism | Add towncrier fragments for the typed errors and both broadenings | +| P4 | Product | 💡 | Alternatives | "Hand-write 15 classes" is not recorded as a rejected alternative | Add it, framed as drift protection versus typing effort | +| P5 | Product | 💡 | Edge cases | Fallback paths are silent in the field | Debug-log every fallback with the unresolved code | +| P6 | Product | 🤔 | Edge cases | CLI output changes for resolver-raised permission failures | Confirm the new rendering, then pin it with a test | +| P7 | Product | 🤔 | Success measurement | SC-008's promise rests on a class outside the guaranteed-stability tier | Decide whether `ApiError` joins the top-level `__all__` | +| X1 | Cross-lens | 🎯 | Scope × Risk | See E2 | See E2 | +| X2 | Cross-lens | 🎯 | Scope × Risk | See E3 | See E3 | +| X3 | Cross-lens | 💡 | Docs × CI | See E13 and P2 | Ignore the package for API docs; one hand-written topic page | + +--- + +## Recommended Actions + +### 🎯 Must-Address (Before Proceeding) + +1. **E1**: In `data-model.md` and `plan.md`, give `ApiError` safe class-level defaults for `errors`, + `query`, and `variables`, and correct the CLI mitigation in research.md R11 — the hazard is a missing + attribute, not an empty list. Add the attribute-access test to the plan's test list. +2. **E2**: Amend spec FR-008 to "401 and 403 codes *additionally* descend from the authentication + branch", and update `data-model.md`, `research.md` R5, and + `contracts/exception-hierarchy.md` to the dual base. Record the verification (Infrahub + `graphql/app.py:298-300` returns 200; `graphql/error_formatter.py` maps resolver-raised auth + failures) so the reasoning is not lost. Also correct the spec's edge case, which currently claims + auth failures only ever arrive as real 401/403. +3. **E3**: Reverse the payload-validation decision in research.md R6, `data-model.md`, and + `contracts/exception-hierarchy.md`: the code resolves the class unconditionally, `data` is + `Model | None`, and a `None` means the server violated its own emission contract. + +### 💡 Recommendations (Strongly Suggested) + +1. **E4 + E13 + E7**: fold `errors` onto `ApiError`; flip the docs package to `packages_to_ignore`; + make the factory total. +2. **E5, E6, E8, E11, E12, E14, P4, P5**: one- to three-line corrections to the artefacts named in each + row above. +3. **P1, P2, P3**: sequencing note in `tasks.md`, docs page scoped to hierarchy and guarantees with a + link out for codes, and towncrier fragments named as tasks. + +### 🤔 Questions (Need Stakeholder Input) + +1. **P7**: Should `ApiError` (and `Error`) be exported from `infrahub_sdk/__init__.py` so SC-008's + promise sits in the guaranteed-stability tier? This permanently enlarges the guaranteed surface. +2. **P6**: Is the changed CLI rendering for a resolver-raised permission failure ("Authentication + failure: …" instead of the GraphQL error list) the wanted outcome? +3. **E15**: How are the paired Infrahub and SDK pull requests sequenced for a submodule artefact? The + `protocols.py` precedent presumably answers this. + +--- + +## Resolution + +Applied on 2026-08-25: all three must-address items and all sixteen recommendations. + +| Finding | Where it landed | +|---------|-----------------| +| E1 | `ApiError` gains class-level defaults for `errors`, `query`, `variables` — data-model.md, research.md R11 (which also corrects the wrong empty-versus-missing diagnosis), plan.md constraints | +| E2 | FR-008 amended to "additionally descend"; the HTTP 200 edge case corrected in spec.md; dual base recorded in research.md R5, data-model.md, both contracts | +| E3 | Reversed in research.md R6; FR-004 gained the rule; `data` is `Model \| None` throughout the contracts and data model | +| E4 | `errors` moved onto `ApiError` (same change as E1) | +| E5 | research.md R4 and the generator contract note the inherited-`CODE` trap | +| E6 | research.md R1 and quickstart scenario 3b: the layering test walks every import node | +| E7 | research.md R7: the factory is total; quickstart scenario 2 drives malformed envelopes | +| E8 | Wire-versus-declared `http_status` documented in spec.md, research.md R5, data-model.md, the hierarchy contract | +| E9 | FR-028 and research.md R13: messages now surface action and resource kind | +| E11, E12 | research.md R12 gained the per-layer coverage table including the broadening tests | +| E13 | research.md R1 flipped to `packages_to_ignore`, with the cross-repo reason; plan.md Principle VII row | +| E14 | New research.md R15: spike the narrowed annotation against mypy and `ty` first | +| E15, P6 | New research.md R17 and a plan.md "Open questions" section | +| P1 | New research.md R16 and a plan.md "Sequencing" section | +| P2 | research.md R13: the page links to the published catalogue instead of restating codes | +| P3 | New research.md R14: towncrier fragments in `changelog/` | +| P4 | research.md R2 gained the no-generator alternative | +| P5 | research.md R7: every fallback logs at debug | +| P7 | Answered by the maintainer, and the answer reframed the question — see below | + +### Second pass + +A follow-up review of the *applied* artefacts found three more blocker-class items, two of them created +by the fixes above, all now applied: + +| Finding | Fix | +|---------|-----| +| An undeclared name collision would silently shadow a hand-written class. `VALIDATION_ERROR`, `RATE_LIMIT`, `INVALID_RESPONSE`, `FILE_NOT_VALID`, and `RESOURCE_NOT_DEFINED` all derive names the SDK already defines, and the façade's ordered star-imports would let the generated class win | FR-006 gained the rule; generation aborts on an undeclared collision (research.md R3, generator contract) | +| The dual base (E2) gives auth classes `GraphQLError.__init__`, whose first positional parameter is `errors` — so passing the joined message positionally would assign it to `errors`, reproducing `analyzer.py`'s corruption by construction | Factories construct with keywords only, asserted by a test (research.md R7); the spec's constructor-misuse edge case now names the hazard | +| Every acceptance criterion was provable against fixtures authored alongside the parser, with no test against a real server — and the constitution puts server-dependent behaviour in the integration tier, which already exists here | Two real failures driven through testcontainers on both clients (research.md R12, plan Principle V, quickstart scenario 6b) | + +Smaller items from the same pass: the tuple default for `errors` (E1) would fall into +`print_graphql_errors`' non-list branch, which also lacks a `return`; the auth factory must use +`decode_json` so a non-JSON 401 body does not raise in place of the authentication error; there are two +pre-existing malformed `GraphQLError` construction sites, not one; and the accepted broadening is +slightly wider than stated, since the file handler's REST 404 also becomes a `GraphQLError`. + +### Third pass — the payload access design + +E3 and E14 both circled a payload *object* on the exception without questioning whether it should exist. +It should not, and removing it retired both findings along with several of the complications the earlier +passes introduced. + +US1 asks for the detail "as typed attributes", never for a payload object. So the payload's fields are +promoted to directly typed attributes on the exception (`exc.node_kind`, `exc.fields`), and the pydantic +model reverts to being the validation mechanism. Consequences: + +- `ApiError.data: Any` is gone, and with it the only invented `Any` in the design. There is nothing on + the base to narrow, so no variance problem, no property pair, no generic hierarchy, and no + suppression anywhere (E14's spike survives in reduced form as R15). +- The generated module drops from two files to one. The `payloads.py` / `catalogue.py` split existed + only so `base.py` could name a generated model type for the adopted classes; with promotion it names + none, `base.py` imports nothing from inside the package, and the package is four modules rather than + five. +- The access pattern is now uniform. The adopted classes already promoted their payload fields onto + `node_type` and `identifier`, so `.data` for the other twelve codes was an inconsistency the SDK's + users would have had to learn. +- E3 is reverted, with a better argument than the one that made it: a required catalogue field is a + non-optional attribute, so an invalid payload has nothing to populate it with and must fall back to + the generic class. The FR-013 reasoning used to reject that originally was overreaching — FR-013's + concern is that the raised type must not depend on *binding freshness*, and payload validity is a + property of the response, not of the SDK's bindings. + +### Fourth pass — automated PR review + +Eight findings on the committed artefacts. Six accepted as filed, one accepted with a corrected +diagnosis, one rejected. + +| Finding | Verdict | +|---------|---------| +| R6 and R7 disagreed on whether `code` stays readable when a recognised code's payload fails validation | **Valid and consequential.** R7 had folded that case into its `code is None` list, which would have made a payload-invalid catalogued error render as an uncatalogued one — R11's CLI branch and R9's server-reported test both key on `exc.code is not None`. R7 now separates the two questions: which class is raised, and what `code` reports. | +| FR-012's fallback could be read as routing by the code's declared status, which would send an unrecognised 401/403 code arriving in a 200 body to the authentication branch and out of `except GraphQLError` | **Valid as a wording defect**, though not as a reading of the intent — "the branch it is already on" meant the transport. But the wrong reading breaks FR-018 silently, so FR-012 now says transport explicitly and states why declared status cannot work: for an unrecognised code the SDK holds no binding and so does not know the declared status at all. | +| The hierarchy contract claimed the dual base's "both clauses catch it today" for the real-401/403 path, where `except GraphQLError` does not catch it today | **Valid**, and it surfaced an unrecorded consequence: the authentication path now resolves catalogue codes, so `except GraphQLError` will begin catching a real 401/403 whose code the bindings recognise. That is a third accepted broadening, now listed in both the contract and the spec. | +| `errors` documented as `list` on `GraphQLError` while an adopted class would expose the base's empty *tuple* | **Valid**, and the right fix is the one already preferred on design grounds: the adopted classes call `super().__init__(errors=[], …)` explicitly instead of leaning on a class-level default. The tuple/list divergence disappears rather than being documented. | +| `exc.extensions["http_status"]` recommended while `extensions` may be `None` | Valid. The contract now says to guard on `extensions` first. | +| Plan said "~12" `AuthenticationError` raise sites; the code has 11 (4 + 6 + 1), as research.md already stated | Valid. Corrected to 11. | +| The Resolution section read third pass before second pass | Valid. Reordered. | +| A pure-docs change should target a different release branch | **Rejected as actionable.** The stated remedy contradicts itself ("belong on stable; target develop instead"), the guideline it cites is not present in this repository, and `develop` and `infrahub-develop` are different branches here. Left as a question for the maintainer, who owns the branch model. | + +### Fifth pass — automated PR review of the fourth pass + +Three findings, all valid, two of them defects the fourth pass introduced. Applied. + +| Finding | Verdict | +|---------|---------| +| The third broadening was stated as "a real 401/403 carrying a catalogue code", which is overbroad — the dual base only applies to codes the bindings recognise, and an unrecognised code on a real 401/403 falls back to the generic `AuthenticationError`, which is not a `GraphQLError` | Valid. Both the contract and the spec now qualify the broadening to recognised codes and say explicitly what happens to the rest. | +| The data-model rationale claimed the adopted classes' explicit `super().__init__` call is what prevents an `AttributeError` — but the fourth pass *kept* `ApiError`'s class-level defaults, so the attributes exist regardless. The explicit call is about `errors` being a *list* | Valid, and self-inflicted: two fixes were applied and the old justification was left attached to the new mechanism. The two mechanisms are now described separately — defaults guarantee existence, the explicit call guarantees the type. | +| FR-012's new rationale was internally contradictory (it said the declared status is unknowable for an unrecognised code, then used an unrecognised code as the example of declared-status routing) and misattributed the coverage guarantee to FR-008's dual inheritance, which shapes per-code classes and not the generic class a fallback raises | Valid on both counts. The example is now a recognised 401/403 code whose payload fails to validate, and the coverage is attributed to FR-012's transport rule, with a note that the dual base does not help there. The same two errors were present in research.md R7 and are fixed there too. | + +### Sixth pass — automated PR review of the fifth pass + +One finding, valid: the qualified broadening ("a real 401/403 whose code the bindings recognise") is +still only true when the payload *also* validates, since a recognised code with an invalid payload falls +back to the generic `AuthenticationError` per FR-012. + +Rather than add a third conditional to a sentence that has now been narrowed twice, both the spec and +the contract restate the broadening as the mechanism that produces it: `except GraphQLError` catches a +real 401/403 exactly when the SDK raises a per-code class for it, because only those classes carry both +parents. That covers the recognised-code and valid-payload conditions without enumerating them, so it +cannot be narrowed again by a further condition on reaching a per-code class. + +### Reading of the trend + +Findings are shrinking — three in the fifth pass, one in the sixth, none above P3 since the fourth — +and are now about the accuracy of rationale prose rather than about the design, while most were +introduced by the previous round's edits. That is the signature of edit churn rather than of remaining +design risk: each round is mostly correcting the last. The design has been stable across three rounds; +what keeps moving is the explanation of it. + +The one durable lesson is in the sixth pass: a claim stated as an enumeration of conditions invites +narrowing forever, while the same claim stated as the mechanism that produces it cannot be narrowed. +Where a later reviewer finds another condition to add, restate rather than qualify. + +**P7 as answered**: no top-level export change. `infrahub_sdk.exceptions` is the supported import path +for every exception, generated or hand-written; a consumer never needs to know which module defines +what; and no name importable from it today may stop being importable from it. That is now FR-005's +second sentence, a rewritten stability assumption in the spec, the "Stability" section of the hierarchy +contract, and `tests/unit/sdk/test_exceptions_public_names.py` — a snapshot test, so the guarantee is +checked rather than asserted. Modules beneath the package are explicitly internal. + +--- + +**Severity Legend**: + +- 🎯 **Must-Address**: Blocks proceeding to implementation +- 💡 **Recommendation**: Strongly suggested improvement +- 🤔 **Question**: Needs stakeholder input to resolve diff --git a/dev/specs/ifc-3034-error-catalogue/data-model.md b/dev/specs/ifc-3034-error-catalogue/data-model.md new file mode 100644 index 000000000..0299ed7b9 --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/data-model.md @@ -0,0 +1,177 @@ +# Data Model: Error Catalogue in the Python SDK + +The entities here are exception classes and pydantic models. Field lists are the observable contract; +see [contracts/exception-hierarchy.md](./contracts/exception-hierarchy.md) for what a consumer may +rely on and [contracts/generator-contract.md](./contracts/generator-contract.md) for how the generated +half is produced. + +Which module holds what is [research.md](./research.md) R1. In short: the hand-written hierarchy sits +in `base.py`, which imports nothing from inside the package; the payload models and per-code exception +classes are generated into `catalogue.py`; `factory.py` sits above both. Imports only ever point +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. + +## Hierarchy + +```text +Error (existing root, unchanged) +├── ApiError NEW — "the server reported an error" +│ ├── GraphQLError re-rooted under ApiError +│ │ ├── NodeNotFoundError re-rooted, unified, adopts NODE_NOT_FOUND +│ │ │ └── NodeInvalidError inherits the re-rooting +│ │ ├── BranchNotFoundError re-rooted, unified, adopts BRANCH_NOT_FOUND +│ │ ├── SchemaNotFoundError re-rooted, unified, adopts SCHEMA_NOT_FOUND +│ │ └── +│ └── AuthenticationError re-rooted under ApiError, name and constructor unchanged +└── … every other existing exception, untouched + +# The 401/403 codes take both branches, since they arrive on the GraphQL transport +# either as a real 401/403 or inside a 200 response's errors array: +# +# AuthenticationRequiredError(GraphQLError, AuthenticationError) +# TokenExpiredError(GraphQLError, AuthenticationError) +# PermissionDeniedError(GraphQLError, AuthenticationError) +# +# MRO: → GraphQLError → AuthenticationError → ApiError → Error → Exception +``` + +## ApiError + +The base for "the server reported an error", carrying the parsed envelope (FR-001). + +| 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. | +| `errors` | `Sequence[dict[str, Any]]` (empty tuple by default) | The complete, unreordered server error list (FR-013). Lives here rather than only on `GraphQLError` because the authentication branch must retain it too and FR-015 freezes `AuthenticationError`'s constructor. The default is an immutable empty tuple and is only a floor for a directly constructed `AuthenticationError`; anything built from a response, and every adopted class, is constructed with a list. | +| `query` | `str \| None` | Class-level default `None`. | +| `variables` | `dict \| None` | Class-level default `None`. | + +`ApiError` adds no required constructor arguments. Its subclasses keep the constructors they have +today, and the factory sets these attributes after construction. + +Two mechanisms sit behind these attributes, and they answer different questions. The class-level +defaults guarantee the attributes *exist* on any `ApiError`, including an `AuthenticationError` +constructed directly through the constructor FR-015 freezes — so `except GraphQLError as exc: +exc.errors` can never raise `AttributeError`. The three adopted classes additionally call +`GraphQLError.__init__` explicitly so their envelope state is set by the constructor that owns it and +`errors` is a *list*, matching the type documented on `GraphQLError` rather than the base's tuple +default. + +## GraphQLError + +| Attribute | Type | Change | +|-----------|------|--------| +| `errors` | `list[dict[str, Any]]` | Unchanged. Complete and unreordered; the first element governs the raised class (FR-013). Every subclass reaches this constructor — the adopted three call it with `errors=[]` — so it is a list on every `GraphQLError`, never the base's tuple default. | +| `query` | `str \| None` | Unchanged, still populated for catalogued failures (FR-024). | +| `variables` | `dict \| None` | Unchanged. | +| `message` | `str` | Constructor gains an optional `message`. Omitted → today's string, byte-identical. Supplied by the factory for a catalogued failure → names the code and the server's message, with no query text (FR-022, FR-023). | + +## AuthenticationError + +Name, constructor, and default message unchanged (FR-015). It remains the class raised for REST +authentication failures, where `code` is `None`. It gains the three generated subclasses and the +inherited `ApiError` attributes. + +## Adopted classes + +Three hand-written classes declare the catalogue code they represent, which is how the generator +knows not to define them: + +| Class | `CODE` | Payload field promoted onto | Type change | +|-------|--------|----------------------------|-------------| +| `NodeNotFoundError` | `NODE_NOT_FOUND` | `node_kind` → `node_type`, `identifier` → `identifier` | `identifier` widens to `Mapping[str, list[str]] \| str` | +| `BranchNotFoundError` | `BRANCH_NOT_FOUND` | `branch_name` → `identifier` | none | +| `SchemaNotFoundError` | `SCHEMA_NOT_FOUND` | `kind` → `identifier` | none | + +Their `from_payload` is hand-written, because the target attribute names already exist and are not the +catalogue's. Every construction shape in use today keeps working: the filter mappings passed from +`infrahub_sdk/store.py` and `infrahub_sdk/client.py`, and the plain string passed from +`infrahub_sdk/file_handler.py` that the current annotation wrongly excludes. `exc.code is not None` +distinguishes a server-reported raise from a client-side one. + +## Generated exception classes + +Generated into `catalogue.py`, one per catalogue code that is not adopted. Each declares: + +| Member | Value | +|--------|-------| +| `CODE` | The catalogue code string. | +| `code` | Class attribute equal to `CODE`. | +| `http_status` | The catalogue-declared status. | +| `DATA_MODEL` | The payload model class, used to validate the envelope. | +| Promoted attributes | One per payload field, typed as the catalogue declares it — required fields non-optional, nullable fields carrying their declared default. Assigned in `__init__`, so they always exist. | +| `from_payload` | Classmethod taking a validated payload plus the envelope, returning the constructed exception. The factory's only construction path. | +| docstring | The catalogue's `description`, plus its stability level. | + +Base classes are derived from the status: every class descends from `GraphQLError`, and a 401/403 code +additionally descends from `AuthenticationError` (FR-008). Twelve single-parent classes and three +dual-parent classes today. + +## Generated payload models + +Generated into `catalogue.py` alongside the classes, one per catalogue code, including codes with an +empty payload and including the adopted codes. Named from the catalogue's `data_schema.title` verbatim +(FR-007), so SDK and frontend binding names agree. `model_config = ConfigDict(extra="ignore")` makes an +unknown field from a newer server a no-op (FR-004). Required fields stay required; nullable fields carry +the catalogue's declared default. + +These models are the validation mechanism and the source of the promoted attributes' types. They are +importable — useful for building a test fixture — but a consumer never reads one off an exception. + +A payload that fails validation falls back to the generic class for the branch, with the code still +readable. The server has a reachable path that emits an empty payload under a code whose schema declares +required fields, so this is a real state rather than a hypothetical one. + +Field types, mapped from the catalogue's JSON Schema vocabulary: + +| JSON Schema | Python | +|-------------|--------| +| `{"type": "string"}` | `str` | +| `{"type": "string", "format": "date-time"}` | `datetime` | +| `{"type": "integer"}` / `{"type": "number"}` | `int` / `float` | +| `{"type": "boolean"}` | `bool` | +| `{"type": "array", "items": T}` | `list[T]` | +| `{"anyOf": [T, {"type": "null"}]}` | `T \| None` | + +Anything outside that vocabulary fails generation loudly rather than emitting a guess. + +## Resolution map + +`CODE_TO_EXCEPTION: dict[str, type[ApiError]]` in `catalogue.py`, covering every catalogue +code: generated classes for most, imported adopted classes for the three. It is the only lookup the +factory performs, and a miss is the entire fallback story (FR-012). + +## The two envelopes + +Both shapes are read; only one is a catalogue envelope. + +**GraphQL** (`/graphql`) — the catalogue envelope. Data errors arrive as HTTP 200 with an `errors` +array; auth failures arrive as a real 401/403. + +```json +{ + "errors": [ + { + "message": "…", + "extensions": {"code": "UNIQUENESS_VIOLATION", "http_status": 422, + "data": {"node_kind": "TestPerson", "fields": ["name"]}} + } + ] +} +``` + +**REST** (`/api/…`) — the legacy envelope, where `extensions.code` is an *integer* mirroring the HTTP +status. No catalogue code, no `data`. Parsed for its messages only; `exc.code` stays `None`. + +```json +{"errors": [{"message": "…", "extensions": {"code": 401}}]} +``` + +## State transitions + +None. Exceptions are constructed, raised, and read. diff --git a/dev/specs/ifc-3034-error-catalogue/plan.md b/dev/specs/ifc-3034-error-catalogue/plan.md new file mode 100644 index 000000000..aa6d227ae --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/plan.md @@ -0,0 +1,212 @@ +# Implementation Plan: Error Catalogue in the Python SDK + +**Branch**: `pog-error-catalogue-IFC-3034` | **Date**: 2026-08-24 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `dev/specs/ifc-3034-error-catalogue/spec.md` + +## Summary + +Make 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 approach has four parts: + +1. **A parsed envelope on the base classes.** A new `ApiError` base carries `code`, `http_status`, the + raw `extensions`, and the server error list; `GraphQLError` and `AuthenticationError` both descend + from it. The envelope is read by one raise-time factory shared by every existing raise site, so the + code is readable against any server version even with no generated bindings at all. Because the + catalogue is GraphQL-only, every generated class descends from `GraphQLError`, and the 401/403 codes + descend from `AuthenticationError` as well. + + The payload is **not** an attribute. Each catalogued class promotes its payload's fields to directly + typed attributes — `exc.node_kind`, `exc.fields` — typed exactly as the catalogue declares them. + That is what US1 asks for, it matches how the three adopted classes already work, and it means no + class needs a loosely typed payload attribute for subclasses to narrow. Nothing in this design is + typed `Any` beyond the raw decoded JSON in `extensions` and `errors`. +2. **Generated per-code bindings.** Infrahub renders one exception class and one pydantic payload + model per catalogue code into a single module in the SDK submodule, next to the schema models and + protocols it already generates there, and its existing generated-artefact validation gains a check + for it. The module is generated in full and imports only the hand-written base, which is what keeps + the package's import graph one-way. +3. **Reconciling the three names that already exist.** `NodeNotFoundError`, + `BranchNotFoundError`, and `SchemaNotFoundError` are adopted by the generator rather than + duplicated: the hand-written classes declare the code they represent, the generator sees that + and imports them instead of defining them. +4. **Removing the string matching.** The silent-refresh decision reads the code, falling back to + the legacy message check only for servers that predate the catalogue. + +The catalogue holds 15 codes today (12 on the GraphQL branch, 3 on the authentication branch). + +## Technical Context + +**Language/Version**: Python 3.10-3.13 (SDK); the generator runs under the Infrahub repository's +Python environment + +**Primary Dependencies**: pydantic >= 2.0 (payload models), httpx (transport), typer + rich (CLI); +Jinja2 and invoke on the Infrahub side for generation. No new runtime dependency. + +**Storage**: N/A + +**Testing**: pytest with `asyncio_mode = "auto"`, `pytest-httpx` for transport-level mocking; +response-envelope fixtures under `tests/fixtures/`; both client variants exercised through the +`BothClients` fixture in `tests/unit/sdk/conftest.py` + +**Target Platform**: Library consumed by `infrahubctl`, the Infrahub Ansible collection, and +external Python applications + +**Project Type**: Library plus its CLI, spanning two repositories (bindings are generated in +Infrahub, consumed here) + +**Performance Goals**: None. The factory runs once per failed request; parsing is a dict lookup plus +one pydantic validation. + +**Constraints**: + +- Parsing MUST NOT raise for any envelope shape, including an unknown code, an unknown payload + field, an absent `extensions`, or a pre-catalogue integer `code`. +- Every existing exception name, constructor signature, and `except` clause keeps working. +- The SDK holds no copy of the catalogue schema; the generated bindings are the only artefact that + crosses the repository boundary. +- No circular imports. The exceptions package is strictly layered, and generated files are generated + in full — never a hand-edited region inside a generated file, and never a generated region inside a + hand-written one. +- `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. + +**Scale/Scope**: 15 catalogue codes; 11 `AuthenticationError` raise sites and 4 `GraphQLError` +raise sites collapse onto two factories; one generated module; one CLI ladder reordering; one new +documentation topic page. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +| Principle | Assessment | +|-----------|------------| +| I. Async/Sync Dual API Parity | **Pass.** No new public client method, so `test_method_count` and `test_validate_method_signature` are untouched. The behaviour change lands on the async and sync paths of `_execute_graphql`, `_execute_graphql_with_file`, and the relogin wrappers, which are separate implementations; both are covered by parametrized tests over `["standard", "sync"]`. | +| II. Backward Compatibility & Public API Stability | **Pass, with two documented broadenings.** Nothing is removed or renamed, so no deprecation path is required. `except GraphQLError` additionally catches client-side node/branch/schema lookup misses, and `NodeNotFoundError.identifier` widens to admit the plain string the file handler already passes. Both get towncrier fragments per FR-016. `infrahub_sdk.exceptions` is treated as public and is the one supported import path; a snapshot test pins that no name importable from it disappears, so restructuring it into a package is invisible from outside. | +| III. Layered Architecture | **Pass.** All envelope parsing, resolution, and message construction lives in `infrahub_sdk/`. The CLI change is confined to presentation: reordering its `isinstance` ladder and degrading the GraphQL renderer when there are no server errors to render. | +| IV. Type Safety & Typed Errors | **Pass, with no suppressions.** This principle is the feature. Generated payload models are pydantic v2; every failure mode gets a specific subclass under `Error`; each catalogued class carries its payload's fields as attributes typed exactly as the catalogue declares them. `Any` appears only where the value genuinely is unknown at the type level — the raw decoded JSON in `extensions` and `errors`, the latter already annotated that way today. No `# type: ignore` is anticipated anywhere; if the R15 spike shows one is needed, that is a signal the shape is wrong rather than a licence to add it. | +| V. Test-First Development | **Pass.** Tests ship in the same change: envelope fixtures per code, cross-version fallback cases, ladder assertions, relogin cases, and both-client parity. Deliberate behaviour changes (the message change, the re-rooting) are pinned by tests that assert the new behaviour rather than being worked around. Because every fixture is authored alongside the parser that reads it, the mocked suite alone could pass against an envelope shape the server never sends — so two real catalogued failures are also driven through testcontainers, which is where the constitution puts behaviour that depends on real server responses. | +| VI. Format & Lint Before Commit | **Pass, with one justified silencing.** `uv run invoke format lint-code` and `lint-docs`; the generated modules are `ruff format`ed by the generator, as the other generated artefacts are. The façade re-exports the generated classes with `from .catalogue import *`, which trips `F403` under `select = ["ALL"]`. A wildcard is the only re-export form that keeps the export surface automatic as codes are added *and* stays visible to mypy and `ty`; the alternative is a hand-maintained list edited every time the catalogue grows. Recorded as a commented `per-file-ignores` entry, mirroring the existing entry for `infrahub_sdk/schema/generated/*.py`. | +| VII. Documentation Accuracy | **Pass.** A new hand-written topic page describes the hierarchy and the cross-version guarantees, linking to Infrahub's published catalogue for the code list rather than restating it. Converting `exceptions.py` into a package requires categorising it in `tasks.py::get_modules_to_document`; it goes in `packages_to_ignore`, which preserves today's `sdk_ref` output exactly and avoids coupling the SDK's `docs-validate` to an Infrahub-side regeneration. | + +**Result**: no violations. Complexity Tracking is empty. + +## Project Structure + +### Documentation (this feature) + +```text +dev/specs/ifc-3034-error-catalogue/ +├── plan.md # This file +├── spec.md # Feature specification +├── research.md # Phase 0 output +├── data-model.md # Phase 1 output +├── quickstart.md # Phase 1 output +├── contracts/ +│ ├── exception-hierarchy.md # The consumer-facing contract +│ └── generator-contract.md # The Infrahub-to-SDK generation contract +├── checklists/ +│ └── requirements.md +└── tasks.md # Phase 2 output (/speckit-tasks, not created here) +``` + +### Source Code + +This repository (`infrahub-sdk-python`): + +```text +infrahub_sdk/ +├── exceptions/ # exceptions.py becomes a package, layered strictly one-way +│ ├── base.py # layer 0: today's exceptions.py + ApiError + adopted-code markers +│ ├── catalogue.py # GENERATED in full — layer 1: payload models, per-code classes, map +│ ├── factory.py # layer 2: raise-time resolution from a response envelope +│ └── __init__.py # layer 3: façade, re-exports base + catalogue, defines __all__ +├── client.py # Raise sites and the relogin wrappers call the factories +├── object_store.py # REST auth raise sites call the auth factory +├── file_handler.py # REST auth raise site calls the auth factory +├── analyzer.py # Pre-existing GraphQLError(str) misuse, corrected +└── ctl/ + └── utils.py # Ladder reordering and renderer degradation + +tests/ +├── fixtures/ +│ └── error_catalogue/ # Response-envelope fixtures per code and per cross-version case +└── unit/ + ├── sdk/ + │ ├── test_exceptions.py # Hierarchy, dual base, naming, adoption, messages + │ ├── test_exceptions_layering.py # Asserts the import graph stays one-way + │ ├── test_exceptions_public_names.py # No name importable from the package may disappear + │ ├── test_error_catalogue.py # Factory: resolution, precedence, fallbacks, totality + │ ├── test_relogin_headers.py # Extended with the typed refresh decision + │ └── test_client.py # Both-client raise-path assertions + └── ctl/ + └── test_utils.py # Ladder behaviour and no-server-errors rendering + +tests/integration/ +├── test_infrahub_client.py # Real catalogued failures, async +└── test_infrahub_client_sync.py # The same two failures, sync + +docs/docs/python-sdk/topics/ +└── error_handling.mdx # New topic page (sidebar globs this directory) + +changelog/ # towncrier fragments: typed errors, identifier widening, broadening +pyproject.toml # per-file-ignore for the façade's re-export star imports +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 +backend/templates/ +└── generate_sdk_errors.j2 # New template: payload models, exception classes, resolution map +tasks/ +├── backend.py # Renderer + the submodule diff check in validate_generated +└── frontend.py # regenerate_error_bindings also renders the SDK bindings +.github/ +├── file-filters.yml # error_catalogue_files gains the submodule artefact +└── workflows/ci.yml # backend-validate-generated also triggers on a catalogue change +``` + +**Structure Decision**: `infrahub_sdk/exceptions.py` becomes the `infrahub_sdk/exceptions/` package, +with a strictly one-way import graph and no cycle anywhere in it: + +```text +base.py (written) → imports nothing from inside the package +catalogue.py (generated) → imports base +factory.py (written) → imports base and catalogue +__init__.py (written) → imports all of the above +``` + +Each module may import only from a strictly lower layer, and no module ever imports the package +façade — internal code always names the concrete submodule. `base.py` sitting at the bottom with no +intra-package imports at all is what the payload decision buys: because a payload's fields are promoted +to attributes rather than exposed as an object, no hand-written class needs to name a generated model +type, so the generated module needs no separate models module below `base.py` and no type-only import +to keep the hand-written hierarchy independent of generated code. + +The rule is enforced by `tests/unit/sdk/test_exceptions_layering.py`, which parses each module and +fails on any upward import, so the property cannot decay silently. Every `from .exceptions import X` +in the codebase keeps working unchanged. See [research.md](./research.md) R1 and R6. + +## Sequencing + +Priority labels in the specification describe value, not order. The generator is US5 (P2) but produces +the per-code classes US1 (P1) delivers, so task ordering must follow the dependency: the Infrahub-side +generator and its first hand-verified run come first. FR-002 softens this — the envelope parses onto the +base classes with no bindings at all, so `code`, `http_status`, and the typed relogin decision (US4) are +independently landable — but the typed per-code classes are not. See [research.md](./research.md) R16. + +## Cross-repository landing order + +The SDK change lands first and Infrahub then bumps its submodule pointer to it — the pattern both +repositories already follow, and the only order under which Infrahub's content-level validation can +pass. See [research.md](./research.md) R17, which notes the one confirmation still worth getting. + +## Complexity Tracking + +No Constitution Check violations, so nothing to justify here. diff --git a/dev/specs/ifc-3034-error-catalogue/quickstart.md b/dev/specs/ifc-3034-error-catalogue/quickstart.md new file mode 100644 index 000000000..d775cb3ab --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/quickstart.md @@ -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 +success criterion it maps to. Details of the promised behaviour live in +[contracts/exception-hierarchy.md](./contracts/exception-hierarchy.md); the generation half lives in +[contracts/generator-contract.md](./contracts/generator-contract.md). + +## Prerequisites + +```bash +uv sync --all-groups --all-extras +``` + +Two checkouts are involved. Scenarios 1 to 6 run here; scenario 7 runs from an Infrahub checkout with +the SDK as its `python_sdk` submodule. + +## Scenario 1 — Typed errors and their payloads + +Proves that every catalogue code is reachable as its own type with its payload's fields as typed +attributes, and that a developer never has to read a message (SC-001). + +```bash +uv run pytest tests/unit/sdk/test_error_catalogue.py -v +``` + +Expected: one passing case per catalogue code. Each asserts the raised class and concrete attribute +values read directly off the exception — `exc.node_kind` and `exc.fields` for `UNIQUENESS_VIOLATION`, +`exc.node_type` and `exc.identifier` for `NODE_NOT_FOUND` — and asserts `exc.code` and +`exc.http_status` match the catalogue entry. No test reads a payload object, because there isn't one. + +## Scenario 2 — Cross-version tolerance + +Proves that nothing raises during parsing on any server version, old or new (SC-004). + +```bash +uv run pytest tests/unit/sdk/test_error_catalogue.py -k crossversion -v +``` + +Expected: passing cases for an unknown code (generic class for the branch, `code` readable as a +string), an unknown payload field (ignored), an absent `extensions` (`code is None`, today's +behaviour), a pre-catalogue integer `code` on `/graphql` (`code is None`), and a payload that +violates the catalogue contract (the generic class for the branch, with `exc.code` still readable). No +case raises during parsing. + +Then prove the factory cannot make things worse than they were, by feeding it malformed envelopes — +`errors` as a bare string, `extensions` as a list, `code` as a nested object: + +```bash +uv run pytest tests/unit/sdk/test_error_catalogue.py -k malformed -v +``` + +Expected: each degrades to today's generic exception. A bug in resolution can never replace the +server's error with an SDK `TypeError`. + +## Scenario 3 — The hierarchy and the existing clauses + +Proves that no `except` clause loses coverage, that the CLI ladder is not shadowed, and that one +clause catches every server-reported error on either transport (SC-003, SC-008). + +```bash +uv run pytest tests/unit/sdk/test_exceptions.py tests/unit/sdk/test_exceptions_public_names.py \ + tests/unit/ctl/test_utils.py -v +uv run pytest tests/unit/ -q +``` + +Expected: `TokenExpiredError` is caught by `except AuthenticationError` **and** by +`except GraphQLError`; `NodeInvalidError` is an instance of `GraphQLError`; `ApiError` catches both a +GraphQL and an authentication failure; every name importable from `infrahub_sdk.exceptions` before the +change is still importable from it; reading `exc.errors`, `exc.query`, and `exc.variables` off a purely +client-side `NodeNotFoundError` returns empty/`None` rather than raising `AttributeError`; driving +`handle_exception` with `NodeNotFoundError` produces the not-found rendering rather than the GraphQL +error rendering; a catalogued failure renders as its code plus the server's message rather than as +"Authentication failure" or a bare error list; rendering an exception with no server errors behind it +prints the message. The full unit suite is green, with any deliberately changed assertion updated rather +than skipped. + +## Scenario 3b — The exceptions package has no import cycle + +Proves the package's import graph points strictly downward, so the cycle the layout was designed to +avoid cannot creep back in. + +```bash +uv run pytest tests/unit/sdk/test_exceptions_layering.py -v +uv run python -c "import infrahub_sdk.exceptions.base" +``` + +Expected: the layering test passes, reporting any intra-package import that points at its own layer or +higher — including imports written inside a function body, which is how a cycle usually returns once the +obvious route is closed. The bare import succeeds on its own, which is the observable form of the rule +that `base.py` depends on nothing else in the package, generated or otherwise. + +## Scenario 4 — Async and sync parity + +Proves both clients raise the same type with the same attributes (SC-006). + +```bash +uv run pytest tests/unit/sdk/test_error_catalogue.py tests/unit/sdk/test_client.py -k "standard or sync" -v +``` + +Expected: every raise-path case passes for both `client_type` values, asserting the same class and the +same payload attributes. + +## Scenario 5 — No string matching left, and refresh still works + +Proves the silent-refresh decision is typed while a pre-catalogue server still refreshes (SC-002). + +```bash +uv run pytest tests/unit/sdk/test_relogin_headers.py -v +grep -rn "Expired Signature" infrahub_sdk/ +``` + +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. + +## Scenario 6 — Messages, lint, and docs + +Proves the message change and the repository gates (SC-007). + +```bash +uv run pytest tests/unit/sdk/test_exceptions.py -k message -v +uv run invoke format lint-code +uv run invoke docs-generate && uv run invoke docs-validate +uv run invoke lint-docs +ls changelog/ +``` + +Expected: 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. `docs-validate` passes with +no change to `sdk_ref`, since the `exceptions` package is categorised as ignored for API-doc +generation — if it is not categorised at all, `docs-generate` fails with +`Uncategorized packages under infrahub_sdk/`. `changelog/` carries fragments for the typed errors, the +`identifier` widening, and the `except GraphQLError` broadening. + +## Scenario 6b — The envelope shape is what the server actually sends + +Proves the contract against a live server rather than against fixtures written alongside the parser +that reads them. Requires Docker. + +```bash +uv run pytest tests/integration/test_infrahub_client.py tests/integration/test_infrahub_client_sync.py \ + -k "catalogue" -v +``` + +Expected: saving a node that collides on a unique attribute raises `UniquenessViolationError` with the +node kind and colliding fields populated from the real payload; deleting a missing node raises +`NodeNotFoundError` with its kind and identifier. Both pass on the async and sync clients. If these +fail while scenario 1 passes, the unit fixtures encode an envelope the server does not send. + +## Scenario 7 — Generated bindings cannot silently drift + +Proves that a catalogue change omitting regeneration fails validation, and that a regenerated artefact +is byte-identical to a fresh generation (SC-005). Run from the Infrahub checkout. + +```bash +uv run invoke frontend.regenerate-error-bindings +git -C python_sdk diff --exit-code infrahub_sdk/exceptions/catalogue.py # clean: byte-identical +uv run invoke backend.validate-generated # passes +``` + +Then prove the negative: add a code to the backend catalogue, regenerate the catalogue JSON alone, and +re-run the validator. + +```bash +uv run invoke backend.export-error-catalogue +uv run invoke backend.validate-generated # must fail, naming the stale submodule artefact +``` + +Expected: the second `validate-generated` exits non-zero with a hint pointing at +`uv run invoke frontend.regenerate-error-bindings`. Revert the catalogue change afterwards. + +The generator is a pure text transform over `schema/error-catalogue.json` plus an `ast` parse of the +SDK's `base.py`, so it needs no running Infrahub — only the submodule checked out, which every Python +job in that repository already requires. + +## Manual end-to-end check + +Against a running Infrahub, with the SDK installed: + +```python +from infrahub_sdk import InfrahubClient +from infrahub_sdk.exceptions import ApiError, UniquenessViolationError + +client = InfrahubClient() +node = await client.create(kind="TestPerson", name="Jane") # a name that already exists +try: + await node.save() +except UniquenessViolationError as exc: + print(exc.code, exc.http_status, exc.node_kind, exc.fields) +except ApiError as exc: + print("uncatalogued or unrecognised:", exc.code) +``` + +Expected: the specific class, its catalogue code and status, and the colliding field names read +directly off the exception with no guard and no intermediate object — and no query text in the message. diff --git a/dev/specs/ifc-3034-error-catalogue/research.md b/dev/specs/ifc-3034-error-catalogue/research.md new file mode 100644 index 000000000..d84e691d5 --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/research.md @@ -0,0 +1,643 @@ +# Research: Error Catalogue in the Python SDK + +Every decision below was reached against the two checkouts as they stand, not from the specification +alone. The specification carried no `NEEDS CLARIFICATION` markers; what follows resolves the +mechanism questions it deliberately left to the plan. + +## 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 +(`AUTHENTICATION_REQUIRED`, `UNDEFINED_ERROR`). One field carries `format: date-time` +(`TOKEN_EXPIRED.expired_at`); the rest are strings, nullable strings, and one string array +(`UNIQUENESS_VIOLATION.fields`). + +Three derived class names collide exactly with hand-written SDK classes: `NODE_NOT_FOUND` → +`NodeNotFoundError`, `BRANCH_NOT_FOUND` → `BranchNotFoundError`, `SCHEMA_NOT_FOUND` → +`SchemaNotFoundError`. + +Raise sites that need to change: `GraphQLError` is raised at `infrahub_sdk/client.py:1360` and +`:2348` (`_execute_graphql`, async and sync) and `:1429` and `:2417` (the file-upload variants). +`AuthenticationError` is raised from the same four GraphQL paths (`:1350`, `:1688`, `:2338`, `:3857`) +and from six REST paths in `infrahub_sdk/object_store.py` plus one in `infrahub_sdk/file_handler.py`, +all with the identical four-line "decode, collect messages, join with ` | `" shape. + +Infrahub already owns two generation paths that write into the submodule +(`tasks/backend.py::_generate_schemas` and `::_generate_protocols`) and validates both with +`git -C python_sdk diff --exit-code` inside `validate_generated`. Separately, +`tasks/frontend.py::regenerate_error_bindings` regenerates the three catalogue-derived artefacts +(the JSON, the frontend TypeScript bindings, the docs page) and `check_error_bindings` diffs them. +The frontend's hand-rolled generator (`frontend/app/scripts/generate-error-bindings.mjs`) is the +closest precedent for what the SDK generator must do. + +Infrahub declares the SDK as `infrahub-sdk = { path = "python_sdk", editable = true }`, so `uv sync` +fails outright without the submodule. Every Python job in that repository therefore already needs +`submodules: true`, and 20 of the 32 checkouts in `ci.yml` set it. + +## R1 — Where the generated bindings live in the SDK, with no circular imports + +**Decision**: convert `infrahub_sdk/exceptions.py` into a package of five modules in a strict, +enforced layer order. No module imports a module at its own level or above, and no module imports the +package façade. + +| Layer | Module | Written or generated | May import | +|-------|--------|----------------------|------------| +| 0 | `base.py` | hand-written | nothing from inside the package | +| 1 | `catalogue.py` | generated, in full | `base` | +| 2 | `factory.py` | hand-written | `base`, `catalogue` | +| 3 | `__init__.py` | hand-written | all of the above | + +**Rationale**: two constraints pull against each other. The generated classes must descend from the +base classes, and `infrahub_sdk.exceptions` must re-export the generated classes (FR-005). Keeping +both in one module makes that a cycle whose only resolution is a wildcard import at the bottom of the +file — fragile, and in exactly the module that must never fail to import. Splitting hand-written from +generated makes the dependency one-way and puts the façade above both. + +Nothing in `base.py` needs anything from the generated module. That falls out of the payload decision +in R6: because a payload's fields are promoted to directly typed attributes on the exception rather +than exposed as a payload object, no hand-written class needs to name a generated model type — not even +the three adopted classes. So the hand-written hierarchy imports and behaves identically whether or not +the generated module is present, with no type-only import required to achieve it. + +**Enforcement**: `tests/unit/sdk/test_exceptions_layering.py` parses each module in the package with +`ast` and asserts every intra-package import points to a strictly lower layer. It walks *every* +`Import` and `ImportFrom` node, not only module-level ones — a function-body import is the classic way +a cycle gets reintroduced once the obvious route is closed — and it counts imports inside +`TYPE_CHECKING` blocks. An upward import fails the pull request that adds it, so the property cannot +decay into the cycle it was designed out of. No new dependency. + +**Consequences to handle**: + +- `tasks.py::get_modules_to_document` auto-discovers packages under `infrahub_sdk/` and raises + `ValueError` for any package that is not explicitly categorised, so `"exceptions"` must be + categorised. It goes in `packages_to_ignore`. Documenting it would generate `sdk_ref` pages from the + *generated* classes, which couples the SDK's own `docs-validate` to an Infrahub-side regeneration: + Infrahub writes new bindings into the submodule, nothing re-runs the SDK's `docs-generate`, and the + next SDK pull request fails on stale docs for a change made in another repository. `exceptions.py` is + not documented in `sdk_ref` today, so ignoring the package preserves current behaviour exactly and + creates no coupling. FR-028 is satisfied by the hand-written topic page, which is the better artefact + for a hierarchy anyway. +- The façade re-exports with `from .base import *` and `from .catalogue import *`, each source module + declaring its own `__all__` (generated for `catalogue.py`). That keeps the export + surface automatic as codes are added and stays visible to mypy and `ty`, at the cost of an `F403` + `per-file-ignores` entry with a comment — the same treatment + `infrahub_sdk/schema/generated/*.py` already gets. `infrahub_sdk.exceptions` is the supported import + path for consumers; the submodules beneath it are internal, and the façade is what makes that true + rather than aspirational. +- First generation is a bootstrap: the SDK pull request lands `catalogue.py` produced by running the + Infrahub generator from the paired branch, verified by hand once, which is what US5 anticipates. + +**Alternatives considered**: + +- *A sibling `infrahub_sdk/error_catalogue.py`.* Fails FR-005 — `infrahub_sdk.exceptions` could not + re-export it without reintroducing the cycle. +- *Keep one `exceptions.py` and wildcard-import the generated module at the bottom of the file.* + Works, and is precisely the fragility this decision exists to avoid. +- *Generate the whole of `exceptions.py`.* Rejected. Roughly thirty hand-written exceptions + unrelated to the catalogue live there — rate limiting, fragment rendering, YAML validation, and the + three unified classes with custom constructor and `__str__` logic. Generating the file would put all + of that under a generator owned by another repository, and would require the generator to carry + hand-written class bodies as template data. What this design does take from that idea is the part + worth keeping: the generated file is generated *in full*, so there is never a hand-edited region + inside a generated file or a generated region inside a hand-written one. +- *Split the generated models into their own module below `base.py`.* Necessary only if a hand-written + class must name a generated model type, which the promotion decision in R6 removes. Without that + need the split buys nothing and costs a module, a second artefact to validate, and an ordering + constraint on generation. + +## R2 — Where the generator lives and how validation is wired + +**Decision**: one Jinja2 template `backend/templates/generate_sdk_errors.j2` rendered by a +`_generate_sdk_error_bindings` helper in `tasks/backend.py`, invoked from +`tasks/frontend.py::regenerate_error_bindings` alongside the other catalogue-derived artefacts, and +verified in `tasks/backend.py::validate_generated` with +`git -C python_sdk diff --exit-code infrahub_sdk/exceptions/catalogue.py`. + +**Rationale**: this splits the work along the two seams Infrahub already has, rather than inventing a +third mechanism. + +Generation belongs with the other artefacts Infrahub renders into the submodule from a Jinja2 +template: `_generate_protocols` is the direct precedent, including the `ruff format` and +`ruff check --fix` pass on the output. Validation belongs in `validate_generated` because that task's +whole purpose is "the generated artefacts are committed and current", and it already handles the one +non-obvious part — a `git diff` for a submodule artefact has to run *inside* the submodule, since from +the superproject `git diff` only sees the submodule pointer. + +**On the CI job**: `backend-validate-generated` is the right host because it runs +`backend.validate-generated`, not because of what it happens to check out. Submodule availability is +not a discriminator here at all: `infrahub-sdk = { path = "python_sdk", editable = true }` in +Infrahub's `pyproject.toml` means `uv sync` fails without the submodule, so every Python job in that +repository already requires `submodules: true` — 20 of the 32 checkouts in `ci.yml` set it today. Any +job that needs the submodule gets it declared; the check goes where it belongs and the checkout +follows. + +**Consequences**: + +- `backend-validate-generated`'s `if` currently fires on `backend == 'true' || documentation == + 'true'`. Add `error_catalogue == 'true'` so a catalogue-only change cannot slip past. +- `error_catalogue_files` in `.github/file-filters.yml` gains the submodule artefact path and the + template, so editing the committed bindings or the generator also triggers the check. +- The failure hint names `uv run invoke frontend.regenerate-error-bindings`, matching the hint the + frontend catalogue job already prints. + +**Alternatives considered**: + +- *A pytest in `backend/tests/unit/errors/test_export.py` that renders in memory and byte-compares + against the committed files*, mirroring `test_export_matches_committed_file` for the catalogue JSON. + Attractive — precise failure output, no git involved — but the renderer's output only becomes + canonical after `ruff format`, so the test would have to shell out to ruff or the template would + have to emit already-formatted output for all 15 codes. Rejected as one mechanism too many; + `validate_generated` already does render-then-format-then-diff for the two other submodule + artefacts. Worth revisiting if the diff output ever proves hard to act on. +- *Adding the SDK check to `frontend.check-error-bindings` and its CI job.* That job is Node-only and + named for the frontend; giving it a Python toolchain to validate a Python artefact makes the job + name lie. The invoke task still regenerates all four artefacts together, which is where the + "catalogue artefacts move as a set" property actually lives. +- *A standalone Python script mirroring the frontend's `.mjs` generator.* Would duplicate the + template-render-then-ruff pipeline `tasks/backend.py` already has. +- *No generator at all — hand-write the 15 classes once.* Worth stating plainly, because 15 classes is + not much typing. The generator's value is not saved keystrokes, it is US5: a catalogue change that + skips the SDK fails the pull request that made it, instead of surfacing months later as a code that + quietly falls back. Hand-written classes buy the typing and none of the drift protection. + +## R3 — Name derivation + +**Decision**: the exception class name is the code's underscore-separated parts capitalised and +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`. + +**Rationale**: FR-006 and FR-007. Taking the payload name from the declared title rather than +deriving it keeps SDK and frontend binding names identical, which the frontend generator already +relies on. The generator asserts a non-empty `data_schema.title`, as the frontend one does, so a +catalogue entry that omits it fails generation loudly. + +**Undeclared collisions must fail generation.** Deriving a name that already exists in `base.py` is +fine when the SDK declared that adoption (R4) and a latent disaster otherwise. The façade re-exports +`base` and then `catalogue`, so a generated class would win the name and silently shadow the +hand-written one — changing what an existing `except` clause catches, from a change made in the other +repository. This is not hypothetical: the SDK already defines `ValidationError`, `RateLimitError`, +`InvalidResponseError`, `FileNotValidError`, and `ResourceNotDefinedError`, and `VALIDATION_ERROR`, +`RATE_LIMIT`, `INVALID_RESPONSE`, `FILE_NOT_VALID`, and `RESOURCE_NOT_DEFINED` all derive exactly +those names. A 429 code in the catalogue is an entirely ordinary thing to add. + +The generator therefore collects every class name defined in `base.py`, not only those carrying a +`CODE`, and aborts on any derived name that matches one without a matching `CODE` declaration. The +remedy is then a deliberate SDK decision — adopt the code, or rename — taken in the pull request that +adds the code rather than discovered months later. + +## R4 — Codes whose class already exists + +**Decision**: adoption is discovered, not configured. The hand-written class declares the code it +represents (`CODE = "NODE_NOT_FOUND"` on `NodeNotFoundError`, and likewise for `BranchNotFoundError` +and `SchemaNotFoundError`); the generator parses `infrahub_sdk/exceptions/base.py` with `ast`, +collects every class whose body assigns a `CODE` string, and for those codes emits an import plus a +`CODE_TO_EXCEPTION` entry instead of a class definition. It still emits the payload model for an +adopted code. + +**Rationale**: the alternative is a hand-maintained code-to-class table in the generator, which is +the kind of thing FR-008 exists to avoid and which would live in the wrong repository — the decision +to unify a name is an SDK decision. Discovery puts the declaration next to the class it describes, so +adopting a fourth code later is a one-line SDK change with no generator edit. + +Reading the source with `ast` rather than importing the SDK keeps the generator a pure text transform, +as the frontend's generator is, and means generation never depends on the SDK checkout being in an +importable state. The same walk collects every class *name* defined in `base.py`, which is what the +collision check in R3 needs. + +**Fallback**: if the `ast` walk proves awkward, import `infrahub_sdk.exceptions.base` and read `CODE` +off the classes — `base.py` imports nothing from inside the package, so it is importable on its own. +Such a walk must filter to classes whose *own* `__dict__` carries `CODE`, because `NodeInvalidError` +inherits `CODE = "NODE_NOT_FOUND"` from `NodeNotFoundError`; it would otherwise see two classes +claiming the same code and let dict ordering pick the winner. The `ast` walk is immune, since it only +sees class bodies. + +**Alternative considered**: generating all 15 classes under distinct names and having the +hand-written unified classes subclass them. Rejected — the resolution map would then point at the +generated base, so the factory would raise the generated class and never the unified one that +consumers catch. + +## R5 — Parent class derivation + +**Decision**: every generated class descends from `GraphQLError`. Classes for codes declaring 401 or +403 *additionally* descend from `AuthenticationError`: `class PermissionDeniedError(GraphQLError, +AuthenticationError)`. Derived from the catalogue entry at generation time, with no per-code table +(FR-008). + +Today that yields `AuthenticationRequiredError`, `TokenExpiredError`, and `PermissionDeniedError` +with both parents, and the remaining twelve with `GraphQLError` alone. + +**Rationale**: the first design here made the two parents alternatives, which was wrong, and the +survey proves it rather than suspects it. `backend/infrahub/graphql/app.py:298-300` returns +`status_code=200` for every executed query, and `graphql/error_formatter.py` maps resolver-raised +failures onto catalogue codes inside that response's `errors` array — including `PERMISSION_DENIED`, +`AUTHENTICATION_REQUIRED`, and `TOKEN_EXPIRED`. Only failures that escape *before* execution get a +real 401/403, which `api/exception_handlers.py:52-55` states outright. So a permission failure can +arrive on the data path, in a response `except GraphQLError` catches today. A single authentication +parent would silently remove that coverage and violate FR-018. + +The diamond closes on `ApiError`, so the MRO is +`PermissionDeniedError → GraphQLError → AuthenticationError → ApiError → Error`, and `__init__` +resolves to `GraphQLError`'s — correct, because these classes only ever arise on the GraphQL +transport. A REST authentication failure raises plain `AuthenticationError` and never a generated +subclass, since REST carries no catalogue codes (FR-015). + +**Consequence for the CLI**: its ladder tests `AuthenticationError` before `GraphQLError`, so a +resolver-raised `PERMISSION_DENIED` will render as "Authentication failure: …" where today it renders +through `print_graphql_errors`. That is a deliberate, user-visible change; it is pinned by a test, and +flagged as an open question in the plan rather than assumed to be wanted. + +**Note on `http_status`**: the class attribute is the catalogue's declared value, which is what US1 +acceptance scenario 3 asserts. The wire value can differ — `api/exception_handlers.py:26-27` replaces +a declared 500 with the real HTTP status when it has a more accurate one — and stays available as +`exc.extensions["http_status"]`. Documented, so the divergence does not read as a bug. + +## R6 — How a consumer reads the payload + +**Decision**: the payload's fields are promoted to directly typed attributes on the exception class. +The payload model validates the envelope and populates them; it is not the access path. + +```python +# generated +class UniquenessViolationError(GraphQLError): + CODE = "UNIQUENESS_VIOLATION" + code = "UNIQUENESS_VIOLATION" + http_status = 422 + DATA_MODEL = UniquenessViolationData + + def __init__(self, node_kind: str, fields: list[str], **envelope: Any) -> None: + self.node_kind = node_kind + self.fields = fields + super().__init__(**envelope) +``` + +```python +# consumer +except UniquenessViolationError as exc: + print(exc.node_kind, exc.fields) # str, list[str] +``` + +Attribute types mirror the catalogue exactly: a required field is not optional, and a nullable field +carries its declared default. There is no payload attribute on `ApiError` and none on the generated +classes either. + +**Rationale**: this is what US1 actually asks for — "carrying the node kind and the colliding field +names as typed attributes". A payload *object* on the exception was never a requirement; it was a +design choice, and a costly one. Exposing `data` on the base forces the base to name a type for it, +and narrowing that type on each subclass is an unsound override of a mutable attribute, so it can only +be bought with `Any` on the base, a read-only property pair, or a generic hierarchy. Promotion makes +the whole question disappear: there is nothing on the base to narrow. + +It is also the access pattern the design already used for the three adopted classes, whose payload +fields land on the existing `node_type` and `identifier` attributes (R9). Promoting everywhere removes +an inconsistency where three codes were read one way and twelve another. + +Two further simplifications fall out. `base.py` no longer needs to name a generated model type, so the +package needs no separate generated models module and no type-only import to stay independent of +generated code (R1). And the `Optional` payload attribute — with its `if exc.data:` guard at every call +site — is gone. + +**Construction**: the factory never assembles attributes itself. Each class exposes a +`from_payload(cls, payload, **envelope)` classmethod — generated for generated classes, hand-written +for the three adopted ones, where it maps `node_kind` onto the existing `node_type` and so on. The +factory validates with `DATA_MODEL` and calls `cls.from_payload(...)`, so promotion stays with the +class that knows its own attribute names and the factory carries no per-code branching. + +**Payload model shape**: generated payload models are pydantic v2 `BaseModel`s with +`model_config = ConfigDict(extra="ignore")`, honouring the catalogue's `required` list so a required +field is a required model field. JSON Schema is mapped as: `string` → `str`, `integer` → `int`, +`number` → `float`, `boolean` → `bool`, `string` with `format: date-time` → `datetime`, +`array` → `list[T]`, `anyOf: [T, {"type": "null"}]` → `T | None` with the declared default. Any +construct outside that vocabulary fails generation with the offending fragment in the message, as the +frontend generator does. A code with an empty `properties` object still gets a real model class with +no fields, not a special case. + +**Decision on validation failure**: if the payload does not validate — a server violating its own +emission contract, or a `date-time` the SDK cannot parse — the operation falls back to the generic class +for the branch, with `code` readable, the raw `extensions` retained, and a debug-level log recording the +failure. A `ValidationError` never escapes a raise path. + +**Rationale**: FR-004 requires tolerating unknown *fields*, which `extra="ignore"` delivers, and User +Story 2 requires that nothing raises during parsing. Promotion then forces this: a required field is a +non-optional attribute, and there is nothing to populate it with when validation fails. The alternative +is making every promoted attribute optional on every class, which taxes every consumer of the feature's +central use for a narrow server-side glitch. + +An earlier form of this decision kept the specific class and left the payload `None`, on the grounds +that FR-013 requires the raised type to be "a pure function of the response" and so it must not depend +on payload validity. That reasoning was overreaching. A malformed payload *is* part of the response; +FR-013's concern is that the type must not depend on **binding freshness** — on which SDK version the +consumer happens to hold — and falling back on an invalid payload does not touch that property. + +The case is also narrower than it first appears. `graphql/error_formatter.py:59-60` initialises the +payload to `UndefinedErrorData()` and only overwrites it when an `isinstance` guard matches, so the +server can emit `data: {}` under a code whose schema declares required fields. But for the adopted +codes — the ones US1's acceptance scenarios exercise — the guard tests the very exception type the code +was resolved from, so it cannot realistically fail. + +## R7 — Raise-time resolution + +**Decision**: two hand-written factories in `infrahub_sdk/exceptions/factory.py`: + +- `graphql_error_from_response(errors, query, variables)` for the `errors`-array path. +- `authentication_error_from_response(response)` for the 401/403 path, which subsumes the four-line + shape repeated at eleven call sites and preserves today's ` | `-joined message. + +Resolution reads `extensions.code` from the **first** error in the response and looks it up in +`CODE_TO_EXCEPTION`. A hit validates `extensions.data` with the class's `DATA_MODEL` and raises via +`cls.from_payload(...)`. A miss — unrecognised code, absent `extensions`, a non-string `code`, or a +payload that fails validation — raises the generic class for the transport the SDK observed. + +**Which class is raised and what `code` reports are separate questions.** `exc.code` is set from the +wire whenever the wire carried a *string* code, whether or not a class matched it: an unrecognised code +from a newer server is readable there (US2 acceptance scenario 1), and so is a recognised code whose +payload failed to validate. `exc.code` is `None` only when there was no string code to read — an absent +`extensions`, or the REST envelope's integer `code` (FR-003). Conflating the two would break both places +that key on `exc.code is not None`: the CLI branch in R11 and the server-reported test in R9. + +The complete `errors` list is retained on the exception in every case, unreordered (FR-013) — which is +why `errors` belongs on `ApiError` rather than only on `GraphQLError`, since the authentication branch +must retain it too and FR-015 freezes `AuthenticationError`'s constructor. + +**The fallback follows the transport, never the code's declared status.** For an unrecognised code the +SDK has no binding and therefore cannot know the declared status at all; for a recognised one the +declared status describes the failure, not the transport. So the case that bites is a *recognised* +401/403 code whose payload fails to validate inside an HTTP 200 body: routing by declared status would +send it to the authentication branch, where an existing `except GraphQLError` would stop catching it. + +R5's dual base does not rescue that case, and it is worth being precise about why: the dual base shapes +the per-code classes, and the class a fallback raises is the *generic* one, which has a single parent. +Only the transport rule preserves the coverage here. + +**Construct with keyword arguments, always.** A generated class's `__init__` takes its promoted fields +first and forwards the envelope to `super().__init__`, and for the auth-branch classes the MRO resolves +that to `GraphQLError.__init__`, whose *first positional parameter* is `errors`. Anything constructing +one of these positionally — the shape the eleven existing `AuthenticationError` raise sites use today, +`TokenExpiredError(" | ".join(messages))` — assigns a message string into a field expecting a list of +error dicts, reproducing by construction the exact corruption `analyzer.py` already has. The factories +construct with keywords only, and a test asserts `exc.errors` is a sequence of dicts on the auth path. + +**The auth factory must tolerate a non-JSON body.** Two of the sites it replaces call +`exc.response.json()` directly rather than `decode_json`, so a 401 carrying an HTML error page from a +proxy currently raises a JSON decode error in place of the authentication error. The factory uses +`decode_json` and falls back to the plain status when the body is not JSON, which is the same tolerance +R10 requires of the relogin helper for the same reason. + +**The factory must be total.** It sits on the failure path of every client method, so an unexpected +exception inside it would replace a legitimate server error with an SDK `TypeError` and lose the +original failure entirely — the worst possible blast radius for a library. Resolution is therefore +wrapped so that *any* unexpected error degrades to constructing today's generic exception. Tested by +feeding the factory deliberately malformed envelopes: `errors` as a string (which `analyzer.py` +produces today), `extensions` as a list, `code` as a nested object. + +**Fallback logging**: every fallback — unresolved code, absent envelope, invalid payload — logs at +debug with the code involved. Cross-version fallbacks are precisely the signal a maintainer wants from +the field when an SDK meets a newer server, and silence makes SC-004's guarantees observable only in +tests. + +**Rationale**: the first error governs unconditionally, so the raised type is a pure function of the +response rather than of which generated bindings the SDK happens to hold — the reasoning FR-013 +records. Reading `extensions.code` only when it is a `str` is what keeps the REST envelope's integer +`code` from ever being surfaced as a catalogue code (FR-003). + +**Defensive detail**: two call sites already construct `GraphQLError` with something that is not a +list of dicts. `infrahub_sdk/analyzer.py:42` passes the bare string `"Schema is not provided"`, and +`infrahub_sdk/testing/schemas/animal.py:154` passes `[resp.errors]`, a list whose single element is not +a dict. The factory is on neither path, but the CLI renderer and anything iterating `errors` is. +Correct both call sites, and keep `print_graphql_errors`' non-list guard — which needs a `return` it +does not currently have, since today it prints the non-list value and then falls through and iterates +it anyway. + +## R8 — Message construction + +**Decision**: `GraphQLError.__init__` gains an optional `message` parameter. The factory passes a +message naming the code and the server's message for a catalogued failure and passes nothing for an +uncatalogued one, so today's `f"An error occurred while executing the GraphQL Query {query}, +{errors}"` string is reproduced byte-for-byte in the uncatalogued case (FR-023, SC-007). `query` and +`variables` remain attributes in both cases (FR-024). + +**Consequence**: `tests/unit/sdk/test_graph_traversal.py:383` asserts on `GraphQLError`'s message +text (`match="Source node not found"`). That path stays uncatalogued today, so the assertion holds; +the test is re-checked deliberately rather than assumed, per the specification's edge case. + +## R9 — Unifying `NodeNotFoundError`, `BranchNotFoundError`, `SchemaNotFoundError` + +**Decision**: these three promote their payload fields onto the attributes they already have, via a +hand-written `from_payload`, so a consumer reads the same attribute regardless of which path raised the +error: + +| Class | Existing attribute | Promoted from | +|-------|--------------------|---------------| +| `NodeNotFoundError` | `node_type`, `identifier` | `node_kind`, `identifier` | +| `BranchNotFoundError` | `identifier` | `branch_name` | +| `SchemaNotFoundError` | `identifier` | `kind` | + +This is the same promotion R6 applies to every other code; the only difference is that the target +attribute names already exist and are not the catalogue's, so the mapping is hand-written rather than +generated. + +`identifier`'s annotation widens to `Mapping[str, list[str]] | str`, which is what FR-016 calls +"documented as such": the plain string is not new behaviour, it is what +`infrahub_sdk/file_handler.py:168` already passes and what the current annotation wrongly excludes. The +three classes are re-rooted under `GraphQLError` (via `ApiError`), and `NodeInvalidError` inherits that +re-rooting — asserted by a test, not assumed. + +Because there is no payload attribute, "did this come from the server?" is answered by `exc.code is not +None`, which is true of every server-reported error rather than only of ones carrying a payload. + +**Rationale**: this satisfies "one documented way to obtain the identifying detail that works +regardless of which path raised the error" without inventing a new accessor that existing consumers +do not know about. Nothing in this repository reads these attributes except the classes' own `__str__` +rendering, so the blast radius is entirely external, which is why the widening goes in the release +notes. + +**Alternative considered**: a new normalised property (`identifier_display` or similar) alongside an +unchanged `identifier`. Rejected — it adds surface for a problem the widening already solves, and +leaves the file handler's existing string still outside the declared type. + +## R10 — The typed silent-refresh decision + +**Decision**: `handle_relogin` and `handle_relogin_sync` decide via a shared helper that reads +`errors[0].extensions.code == "TOKEN_EXPIRED"`, falling back to the existing +`"Expired Signature" in messages` check when no code is present (FR-019). The helper tolerates a +non-JSON or empty 401 body rather than letting `response.json()` raise, since the wrapper sees REST +responses too and only GraphQL carries the catalogue envelope. + +**Rationale**: the wrapper inspects the raw response before any exception exists, so it cannot reuse +the factory; a small shared reader keeps the async and sync copies from drifting. Keeping the legacy +check as a fallback is what makes a pre-catalogue server still refresh. + +**Out of scope, deliberately**: the GraphQL schema-validation probing used for server feature +detection matches on *uncatalogued* conditions and stays exactly as it is (FR-021). + +## R11 — The re-rooted classes' missing attributes, and the CLI ladder + +**The defect this fixes.** `NodeNotFoundError.__init__` does not call `GraphQLError.__init__`, and +neither do `BranchNotFoundError`'s or `SchemaNotFoundError`'s. Re-rooting them under `GraphQLError` +therefore produces instances on which `errors`, `query`, and `variables` do not exist *at all*. A +consumer writing `except GraphQLError as exc: … exc.errors` gets an `AttributeError` on any +client-side lookup miss, and `print_graphql_errors(errors=exc.errors)` raises while reading its +argument. Note that degrading the renderer on an *empty* `errors` does not address this: the attribute +is absent, not empty, so the renderer raises before it can check. + +**Decision**: + +- `handle_exception` gains a branch *above* the class-based ladder, keyed on `exc.code is not None` — + any server-reported error carrying a catalogue code — which renders the code and the server's + message, plus the GraphQL path where server errors exist. An error with no code falls through to + today's ladder unchanged. +- The three adopted classes call `super().__init__(errors=[], query=None, variables=None, message=...)` + explicitly, so their envelope attributes are set by the constructor that owns them. This is the fix + rather than relying on class-level defaults: a default standing in for constructor state also makes + `exc.errors` a *tuple* on a client-side raise and a *list* everywhere else, so the documented type + would be wrong for exactly the classes this feature unifies. +- `ApiError` still declares defaults for `errors`, `query`, and `variables` — an immutable empty tuple + for `errors` — but only as a can't-crash floor for a directly constructed `AuthenticationError`, whose + constructor FR-015 freezes. Anything built from a response goes through a constructor or factory that + sets a list. +- `print_graphql_errors` degrades to the exception's message when there is nothing to render. Its + `isinstance(errors, list)` guard also needs the `return` it currently lacks, since today it prints the + non-list value and then falls through and iterates it anyway. +- In `infrahub_sdk/ctl/utils.py::handle_exception`, move the + `(SchemaNotFoundError, NodeNotFoundError, ResourceNotDefinedError, GraphQLQueryError)` branch + *above* the `GraphQLError` branch, since re-rooting makes the later branch unreachable and would + silently change CLI output for exactly the errors this feature makes specific. + +**Tests**: read `exc.errors`, `exc.query`, and `exc.variables` off a purely client-side +`NodeNotFoundError`; drive `handle_exception` with each class to assert the ladder's behaviour rather +than reading the source; render an exception with no server errors behind it. + +**Why a keyed branch rather than a reordered class branch.** The requirement is that a user can see why +something failed. Neither existing branch delivers that for a catalogued failure: the +`AuthenticationError` branch would print "Authentication failure: …" for a `PERMISSION_DENIED`, which +mislabels it — the user is authenticated and simply not permitted — while the `GraphQLError` branch +prints the server error list without naming the condition. Rendering the code plus the server's message +names the actual failure in both cases. + +Keying the branch on `exc.code is not None` rather than on a class also removes the shadowing hazard for +catalogued errors permanently: it tests data rather than class identity, so no future re-rooting can +make it unreachable. And because it only claims errors carrying a code, every uncatalogued failure keeps +today's rendering byte-identical, which is the same guarantee FR-023 makes for messages. + +**Also checked**: `infrahub_sdk/ctl/cli_commands.py:237` and `infrahub_sdk/ctl/validate.py:88` each +catch `GraphQLError` with no competing branch, so re-rooting cannot shadow anything there. The class +ladder still needs its reordering, because a client-side `NodeNotFoundError` has no code and so reaches +it. + +## R12 — Testing strategy + +**Decision**: response-envelope fixtures under `tests/fixtures/error_catalogue/`, loaded via +`read_fixture()`, driven through `httpx_mock` at the transport boundary — no `unittest.mock`. +Parametrized cases use the dataclass-with-`name` pattern, and every `pytest.raises` carries `match=`. + +Coverage is split explicitly by layer, because SC-006 read literally ("across all catalogued codes") +would mean 15 codes on both clients through the client layer, which fights the constitution's +requirement that unit tests stay fast: + +| Layer | Scope | +|-------|-------| +| Factory | Exhaustive: every catalogue code, its raised class, and every promoted attribute's value. All cross-version cases — unknown code, unknown payload field, absent `extensions`, pre-catalogue integer `code`, invalid payload falling back to the generic class — plus the malformed-envelope totality cases. | +| Client | Representative parity set covering both branches, both transports, and the file-upload variant, parametrized over `["standard", "sync"]` via the `BothClients` fixture. | +| Hierarchy | The dual base for auth codes; `NodeInvalidError` inheriting the re-rooting; attribute access on a client-side raise; the ladder's behaviour. | +| Public surface | Every name importable from `infrahub_sdk.exceptions` before the change is still importable from it, pinned against a committed snapshot list. | +| Broadenings | Both accepted behaviour changes asserted directly: `except GraphQLError` catches a client-side `NodeNotFoundError`; a catalogued message differs from the generic one while an uncatalogued message stays byte-identical. | +| Integration | A small number of real catalogued failures driven against a live server via testcontainers, on both clients. | + +**Rationale**: the constitution requires both paths tested, concrete assertions, and deliberate +behaviour changes pinned by a test rather than worked around. The public-surface snapshot is what makes +"no name importable from `infrahub_sdk.exceptions` may disappear" a check rather than an intention — +necessary because the module is being restructured into a package. + +**Why unit tests alone are not sufficient here.** Every fixture in the layers above is written by the +same hand that writes the parser, so the whole suite can pass green against an envelope shape the +server never sends — and the shape is the entire contract this feature consumes. The constitution is +explicit that behaviour depending on real server responses belongs in integration tests, and the tier +already exists (`tests/integration/`, `infrahub-testcontainers`, with per-client files +`test_infrahub_client.py` and `test_infrahub_client_sync.py`). + +Scope it small and keep it there: drive two genuinely reachable failures — a uniqueness violation on +`.save()` and a missing node on `.delete()` — against a live server, and assert the raised class, the +code, and the promoted attributes. Two cases are enough to validate the envelope shape that every +unit fixture then reuses; the exhaustive per-code coverage stays at the fast, mocked layer where it +belongs. + +## R13 — Documentation + +**Decision**: a new hand-written `docs/docs/python-sdk/topics/error_handling.mdx` covering the +hierarchy, how to catch by branch versus by code, the cross-version guarantees, the two accepted +broadenings, and the note that `infrahub_sdk.exceptions` is the supported import path. It links to +Infrahub's published catalogue reference for the code list instead of restating it. The Python SDK +sidebar globs the `topics` directory, so no sidebar edit is needed. + +**Rationale**: restating 15 codes in this repository creates a second source of truth that rots the +first time a code is added upstream, and nothing here validates it — a direct Principle VII risk. +Infrahub already generates that list from the same artefact. What is genuinely SDK-specific is the +hierarchy and the guarantees, and that is what the page carries. + +One content note: a catalogued message now names the failing action and resource kind where the +catalogue provides them (`PermissionDeniedData` carries both), and that text reaches logs and CLI +output where a wall of query text used to be. Worth a line for anyone shipping SDK logs onward. + +## R14 — Release notes + +**Decision**: towncrier fragments in `changelog/`, one per user-visible change — the typed errors, the +`identifier` widening, and the `except GraphQLError` broadening. + +**Rationale**: FR-016 requires the widening to be called out in the change's release notes, and this +repository's release notes are files, not prose in a pull request: `[tool.towncrier]` in +`pyproject.toml` sets `directory = "changelog"` with `orphan_prefix = "+"` for entries without an issue +number. Naming the fragments as work makes FR-016 verifiable instead of aspirational. + +## R15 — Type-check the generated class shape before generating 15 of it + +**Decision**: hand-write one generated-shape class and run both `mypy` and `ty` over it before the +template is finalised. The shape to check: promoted attributes assigned in `__init__`, a `from_payload` +classmethod returning `Self`, `**envelope` forwarded to `super().__init__`, and the dual base from R5. + +**Rationale**: promotion removed the variance problem that made this a real risk, so what remains is +routine — but the constitution requires both checkers clean, `ty` is newer, and the dual base plus a +`Self`-returning classmethod is the least ordinary construct in the design. Finding a disagreement in +one hand-written class costs minutes; finding it across 15 generated ones costs a regeneration cycle in +another repository. + +Note that no suppression is anticipated anywhere in this design. If the spike shows one is needed, that +is a signal the shape is wrong rather than a licence to add it. + +## R16 — Sequencing across the two repositories + +**Decision**: the Infrahub-side generator and its first hand-verified run come before the SDK-side +typed-raising work can be demonstrated, regardless of user-story priority labels. + +**Rationale**: US1 is P1 and US5 is P2, but the per-code classes US1 delivers are produced by the +generator US5 builds. Priority labels describe value, not order. Task generation must take the order +from the dependency, not from the labels, or the P1 story will be picked up first and immediately +block. FR-002 does soften this — the envelope parses onto the base classes with no bindings at all, so +`code`, `http_status`, and the relogin fix (US4) are independently landable — but the typed per-code +classes are not. + +## R17 — Landing order across the two repositories + +**Decision**: the SDK change lands first, then Infrahub bumps its submodule pointer to it. + +**Rationale**: this is the pattern the two repositories already follow. Infrahub's pointer-moving +commits — `chore(sdk): bump python_sdk to head of infrahub-develop` and feature commits that move the +pointer inline — target SDK commits already present on the SDK's `infrahub-develop` branch, so the SDK +side is merged before the pointer advances. + +It is also the only order that works here. Infrahub's `validate_generated` diffs the submodule's +*content*, so the generated module must already exist in the SDK at the pointer Infrahub carries. The +first generation is therefore hand-run locally to produce the artefact for the SDK pull request, which +is what US5 anticipates, and every regeneration afterwards follows the same order. + +Inferred from the repositories' history rather than from a written practice, so worth one confirmation +from whoever owns the release flow before the paired pull requests go up. + +## R18 — What this plan does not touch + +The git integrator's repository-import failure handling, anything about what the server emits, and +any release-time gate on either side. Pull-request-time validation is the only enforcement mechanism, +matching how the existing generated artefacts are treated (FR-027). diff --git a/dev/specs/ifc-3034-error-catalogue/spec.md b/dev/specs/ifc-3034-error-catalogue/spec.md new file mode 100644 index 000000000..83b68066f --- /dev/null +++ b/dev/specs/ifc-3034-error-catalogue/spec.md @@ -0,0 +1,458 @@ +# Feature Specification: Error Catalogue in the Python SDK + +**Feature Branch**: `pog-error-catalogue-IFC-3034` + +**Created**: 2026-08-21 + +**Status**: Draft + +**Input**: IFC-3034 — Implement the error catalogue in the Python SDK. Related: IFC-2279 (spike), INFP-468 (backend catalogue), GitHub #7498. + +## Context + +Infrahub's GraphQL error catalogue gives every GraphQL error a stable string `extensions.code`, an +integer `extensions.http_status`, and a typed `extensions.data` payload, published as a +machine-readable schema at `schema/error-catalogue.json` in the Infrahub repository. The frontend +already consumes it through generated TypeScript bindings. + +The SDK consumes none of it. `execute_graphql` raises a generic `GraphQLError` whose message embeds +the entire query text, and consumers that need to branch on a failure still match on message +strings. This feature makes ordinary SDK operations raise the specific error for the failure. + +Two wire shapes matter, and they are not the same: + +- **`/graphql`** carries the catalogue envelope: string `code`, integer `http_status`, typed `data`. +- **`/api/...` (REST)** carries the legacy envelope, where `extensions.code` is an *integer* + mirroring the HTTP status. There is no catalogue code and no `data`. + +The catalogue is therefore GraphQL-only, and a REST `extensions.code` is a different thing with a +different type that must never be mistaken for a catalogue code. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Branch on a specific server failure (Priority: P1) + +A developer writing automation against Infrahub needs to react differently to different failures. A +`.save()` that collides on a uniqueness constraint should be distinguishable from a validation +failure, and the collision detail should be available as attributes rather than parsed out of prose. + +**Why this priority**: This is the feature. Everything else either protects it or maintains it. + +**Independent Test**: Drive each catalogued failure against a server (or a fixture of its response +envelope) and assert the raised type and the typed attributes carrying its detail, without reading any +message. + +**Acceptance Scenarios**: + +1. **Given** a node whose unique attribute already exists, **When** the developer calls `.save()`, + **Then** `UniquenessViolationError` is raised carrying the node kind and the colliding field names + as typed attributes. +2. **Given** a node that no longer exists, **When** the developer calls `.delete()`, **Then** + `NodeNotFoundError` is raised carrying the node kind and identifier. +3. **Given** any catalogued failure, **When** it is raised, **Then** `exc.code` equals the catalogue + code string and `exc.http_status` equals the catalogue status. +4. **Given** a developer who catches `GraphQLError` today, **When** a catalogued GraphQL failure + occurs, **Then** the specific subclass is caught by that existing clause. + +--- + +### User Story 2 - Keep working against any server version (Priority: P1) + +The SDK and the server are versioned and released independently, so any SDK version may talk to any +server version. Neither direction may break. + +**Why this priority**: The ticket states this is a hard requirement, not a nice-to-have. User Story 1 +is not shippable without it — typed errors that raise on an unrecognised payload would be a +regression, not a feature. + +**Independent Test**: Replay response fixtures representing a newer server, an older server, and a +pre-catalogue server against the parsing layer, asserting no parse failure and correct fallback in +each case. + +**Acceptance Scenarios**: + +1. **Given** a code the SDK has never heard of, emitted by a newer server, **When** the SDK raises, + **Then** it raises the generic fallback for that transport branch with `exc.code` readable as a + plain string, and does not raise on parse. +2. **Given** an existing code whose payload has gained a new attribute in a newer server, **When** + an older SDK parses it, **Then** the unknown attribute is ignored and behaviour is unchanged. +3. **Given** a server that predates the catalogue, or an error carrying no `extensions`, **When** the + SDK raises, **Then** behaviour matches today's and `exc.code` is `None`. +4. **Given** a pre-catalogue server emitting an *integer* `extensions.code` on `/graphql`, **When** + the SDK parses it, **Then** it is not surfaced as a catalogue code and `exc.code` is `None`. +5. **Given** any of the above, **When** the developer regenerates nothing, **Then** correctness is + unaffected — regeneration buys typed handling of newly catalogued errors, never correctness. + +--- + +### User Story 3 - Catch server-reported errors uniformly across transports (Priority: P2) + +Authentication failures reach the developer from both the REST and GraphQL paths. Today they collapse +into a single `AuthenticationError` that cannot distinguish "no credentials" from "token expired" +from "not permitted". The catalogue splits these into three codes, and the SDK needs a hierarchy +where that split is expressible without stranding the REST path. + +**Why this priority**: It restructures the hierarchy every other story hangs off, but User Story 1 +delivers value with the existing flat `AuthenticationError` still in place. + +**Independent Test**: Assert the class hierarchy directly, and assert that each existing `except` +clause in the SDK and CLI still catches what it caught before. + +**Acceptance Scenarios**: + +1. **Given** a GraphQL request with an expired token, **When** it fails, **Then** `TokenExpiredError` + is raised and is caught by an existing `except AuthenticationError` clause. +2. **Given** a GraphQL request the user is not permitted to make, **When** it fails, **Then** + `PermissionDeniedError` is raised, distinguishable from a missing-credentials failure. +3. **Given** a REST request that fails authentication, **When** it fails, **Then** + `AuthenticationError` is raised as it is today, with `exc.code` as `None`. +4. **Given** a developer who wants to catch anything the server rejected regardless of transport, + **When** they catch `ApiError`, **Then** both GraphQL and auth failures are caught. + +--- + +### User Story 4 - Stop the SDK string-matching its own server (Priority: P2) + +The SDK's silent token-refresh path decides whether to re-login by matching the literal string +`"Expired Signature"` in the response body. The catalogue makes that a typed decision. + +**Why this priority**: A correctness improvement to existing behaviour, valuable independently, but +it depends on the envelope parsing from User Story 1. + +**Independent Test**: Drive the relogin path with a catalogue `TOKEN_EXPIRED` envelope, with the +legacy string on a pre-catalogue server, and with an unrelated 401, asserting a refresh is attempted +in the first two cases and not the third. + +**Acceptance Scenarios**: + +1. **Given** a 401 carrying `TOKEN_EXPIRED`, **When** the SDK receives it, **Then** it refreshes the + token and retries, without inspecting any message text. +2. **Given** a 401 from a pre-catalogue server carrying the legacy `"Expired Signature"` message, + **When** the SDK receives it, **Then** it still refreshes and retries. +3. **Given** a 401 that is neither, **When** the SDK receives it, **Then** no refresh is attempted. + +--- + +### User Story 5 - Bindings that cannot silently drift (Priority: P2) + +A catalogue change that is not reflected in the SDK's bindings must surface as a failure, in the +change that caused it, rather than as silence that is noticed months later when a code falls back. + +**Why this priority**: Without it the typed errors decay. It is P2 rather than P1 only because the +first generation can be landed and verified by hand once. + +**Independent Test**: Modify the catalogue without regenerating, and confirm the validation step +fails; regenerate, and confirm it passes. + +**Acceptance Scenarios**: + +1. **Given** a change to the catalogue in the Infrahub repository, **When** the bindings in the SDK + submodule are not regenerated, **Then** Infrahub's generated-artefact validation fails the pull + request that changed the catalogue. +2. **Given** a regenerated set of bindings, **When** validation runs, **Then** it passes and the + generated file is byte-identical to a fresh generation. +3. **Given** the generated bindings file, **When** a developer opens it, **Then** it is marked as + generated and not to be edited, consistent with the repository's other generated artefacts. + +--- + +### User Story 6 - Messages that are about the failure (Priority: P3) + +`GraphQLError`'s message embeds the whole query text, so a one-line failure produces a wall of +output in logs and CLI sessions. + +**Why this priority**: Observable quality-of-life improvement, no functional dependency either way. + +**Independent Test**: Trigger a catalogued failure and an uncatalogued one, and compare their +messages. + +**Acceptance Scenarios**: + +1. **Given** a catalogued failure, **When** its message is rendered, **Then** it names the code and + the server's message and does not contain the query text. +2. **Given** an uncatalogued failure, **When** its message is rendered, **Then** it is unchanged from + today's, query text included. +3. **Given** any GraphQL failure, **When** a developer needs the query, **Then** it is still + available on the exception. + +### Edge Cases + +These are specific hazards found while surveying the current code, not hypotheticals. + +- **Ordered `isinstance` ladder is shadowed.** The CLI's error handler tests + `isinstance(exc, GraphQLError)` *before* it tests + `isinstance(exc, (SchemaNotFoundError, NodeNotFoundError, ...))`. Re-rooting those classes under + `GraphQLError` makes the later branch unreachable, silently changing CLI output for exactly the + errors this feature makes specific. The ladder must be reordered, and the same shadowing hazard + checked wherever else the SDK or CLI tests these classes in sequence. +- **A GraphQL error renderer with no server errors to render.** The CLI's `GraphQLError` branch + renders `exc.errors`, which is a list of server error dicts. A unified `NodeNotFoundError` raised + purely client-side has no server response behind it, so that list is empty. Rendering must degrade + to the message rather than printing nothing. +- **`identifier` already means two different things.** The client-side `NodeNotFoundError` declares + `identifier` as a mapping of filters, and the store and client lookup paths pass one. The file + handler, however, already passes a plain string, which the declared type does not admit — so the + attribute is heterogeneous today, before any unification. The catalogue payload adds a third + reading: a single server-side identifier string. Unification does not create this problem, it + forces a decision on it. FR-016 pins the resulting contract; the mechanism is left to the plan. + Nothing in this repository reads the attribute except the exception's own string rendering, so the + compatibility risk is entirely external. +- **A subclass inherits the re-rooting.** `NodeInvalidError` subclasses `NodeNotFoundError`, so it + silently becomes a `GraphQLError` too. Intended, but it must be asserted rather than assumed. +- **Pre-existing constructor misuse, in two places.** One call site constructs `GraphQLError` with a + plain string where the constructor expects a list of error dicts, so `errors` holds a string; another + passes a list whose single element is not a dict. Any code that now iterates `errors` to resolve a + code will meet both. Re-rooting makes this worse before it makes it better: once a class inherits a + constructor whose *first positional parameter* is `errors`, passing a message positionally silently + produces the same corruption. +- **A message-matching test inside our own suite.** At least one existing test asserts on + `GraphQLError`'s message text. Message changes must be reflected in the suite deliberately, not + worked around. +- **More than one error in one response.** A GraphQL response may carry several errors with different + codes. The rule for which code determines the raised class must be explicit, and no error may be + discarded from the exception. +- **`UNDEFINED_ERROR` is a code, not the absence of one.** A server that explicitly says + `UNDEFINED_ERROR` is reporting a catalogue gap on its side. That is distinct from an error carrying + no `extensions` at all, and the two must not collapse. +- **Codes with no payload.** Several codes declare an empty payload object. These must still produce + a usable class rather than a special case. +- **Silent-refresh runs on both transports.** The relogin wrapper inspects raw responses from REST + *and* GraphQL calls, but only GraphQL carries the catalogue envelope. It must read the code where + one exists and fall back to the legacy check where one does not. +- **GraphQL data errors arrive as HTTP 200, and so do some auth failures.** Catalogued data errors + come back with status 200 and an `errors` array. Only auth failures that escape *before* query + execution come back as real 401/403 responses on a separate code path; a permission or + authentication failure raised inside a resolver is formatted at the GraphQL layer and returned in + the 200 response's `errors` array like any other error. So the transport a code arrives on cannot be + inferred from the code, and a code's declared `http_status` is metadata about the failure rather than + the status the SDK saw. The declared status can also differ from the status on the wire: the server + replaces a declared 500 with the real HTTP status when it has a more accurate one. + +## Requirements *(mandatory)* + +### Functional Requirements + +#### Envelope parsing + +- **FR-001**: The SDK MUST expose a base class representing "the server reported an error", carrying + the catalogue code, the HTTP status, the raw error envelope, and the server's error list, from which + 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 + 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 + strict emission contract. Where a payload does not validate at all, the operation MUST fall back to + the generic class for the branch, with the code still readable, and MUST NOT raise from parsing. + This case is reachable rather than defensive — the server has a fallback path that emits an empty + payload under a code whose schema declares required fields. + + Rationale: the specific class exposes the payload's fields as attributes typed exactly as the + catalogue declares them, so a required field is not optional. There is nothing to populate those + attributes with when validation fails, and the alternative — making every payload attribute + optional on every class — would tax every consumer of the feature's central use for a narrow + server-side glitch. Note that this does not weaken FR-013: payload validity is a property of the + response, so the raised class remains a function of the response alone and never of which + generated bindings the SDK holds. + +#### Generated bindings + +- **FR-005**: Every catalogue code MUST have one exception class, rooted at the SDK's base `Error` + class, and one typed payload model. Both MUST be importable from `infrahub_sdk.exceptions`, but the + payload model is the parsing mechanism rather than the access path: each of its fields MUST be + reachable as a directly typed attribute on the exception itself, typed as the catalogue declares it. + Every name importable from `infrahub_sdk.exceptions` before this change MUST remain importable from + it afterwards, and that MUST be pinned by a test rather than asserted, since the module is being + restructured. +- **FR-006**: Exception class names MUST derive from the code deterministically, without producing a + doubled `Error` suffix for codes that already end in `_ERROR`. A derived name that collides with an + exception the SDK already defines MUST either be an intentional adoption, declared by the SDK, or + fail generation. It MUST NOT silently produce two classes with one name. + + Rationale: the SDK already defines `ValidationError`, `RateLimitError`, `InvalidResponseError`, + `FileNotValidError`, and `ResourceNotDefinedError`, every one of which is the name a plausible future + code would derive. A collision that is not caught would shadow the hand-written class of that name, + changing what an existing `except` clause catches — in this repository, from a change made in + another one. +- **FR-007**: Payload model names MUST come from the catalogue's declared payload title, so SDK and + frontend bindings agree on naming. +- **FR-008**: Parent classes MUST be derived from the code's declared HTTP status, with no + hand-maintained per-code mapping. Every catalogued code descends from the GraphQL branch, because + the catalogue is GraphQL-only and any code can reach the SDK inside a GraphQL response. Codes + declaring 401 or 403 MUST *additionally* descend from the authentication branch. + + Rationale: the two are not alternatives. A permission failure raised inside a resolver comes back + as an HTTP 200 GraphQL response carrying `PERMISSION_DENIED` in its `errors` array, which is a + response `except GraphQLError` catches today. Making the authentication branch the sole parent + would silently remove that coverage, violating FR-018. +- **FR-009**: The generated artefact MUST carry the same "generated, do not edit" marking as the + repository's other generated files, and MUST record the catalogue version it was generated from. +- **FR-010**: The SDK MUST NOT contain a copy of the catalogue schema. The generated bindings are the + only artefact that crosses the repository boundary. + +#### Raising the specific error + +- **FR-011**: Every operation that today raises the generic GraphQL error MUST raise the specific + class when the response carries a recognised code. +- **FR-012**: Where no class matches the code — unrecognised, absent, or an integer from a + pre-catalogue server — or where the matched class's payload does not validate, the operation MUST + raise the generic class for **the transport it observed**: the GraphQL error for anything read from an + `errors` array, and the authentication error only for a response the SDK saw as HTTP 401 or 403. The + fallback MUST NOT be selected from the code's declared HTTP status. + + Rationale: for an unrecognised code the SDK holds no binding and so cannot know the declared status + at all, and for a recognised one the declared status describes the failure rather than the transport. + Routing by declared status would therefore send a recognised 401/403 code whose payload fails to + validate, arriving inside an HTTP 200 body, to the authentication branch — where an existing + `except GraphQLError` would stop catching it, the coverage loss FR-018 forbids. Note that the dual + inheritance in FR-008 does not help here: it shapes the per-code classes, and the class the fallback + raises is the generic one, which has a single parent. Only the transport rule preserves the coverage. + + Where a string code was on the wire it MUST remain readable as `exc.code` even though the generic + class was raised; `exc.code` is absent only when no string code was present. +- **FR-013**: Where a response carries several errors, the **first** error in the response determines + the class raised. The exception MUST retain the complete list, and MUST NOT discard or reorder it. + The first error governs even when it carries no code and a later one does, in which case the generic + class for the branch is raised. + + Rationale: this makes the raised type a pure function of the response, independent of which version + of the generated bindings the SDK holds. Selecting the first *recognised* code instead would make + the type depend on binding freshness, so regenerating bindings could change which exception a + consumer receives for a byte-identical response — the opposite of the guarantee in FR-010 and User + Story 2. +- **FR-014**: Async and sync clients MUST behave identically, per the constitution's parity + principle, and both paths MUST be tested. + +#### Reconciling names that already exist + +- **FR-015**: `AuthenticationError` MUST keep its name and constructor and MUST remain the class + raised for REST authentication failures, while gaining the three catalogue subclasses beneath it. +- **FR-016**: `NodeNotFoundError` MUST be unified into a single class covering both the client-side + and the server-reported cases, re-rooted so that an existing `except GraphQLError` clause catches + it. The consequent broadening — that clause now also catches purely client-side lookup misses — is + accepted. + + The unified class MUST satisfy all of the following observable contract. The mechanism that achieves + it is left to the plan; the contract is not. + + - Every construction shape in use today MUST keep working unchanged. That includes the mapping of + filters passed on the store and client lookup paths **and** the plain string the file handler + already passes, which the current type annotation does not actually admit. + - The server-reported node kind and identifier MUST be reachable as typed attributes when the error + came from the server. + - There MUST be one documented way to obtain the identifying detail that works regardless of which + path raised the error, so a consumer never has to test which case it is holding. + - Any attribute whose type widens as a result MUST be documented as such, and the widening MUST be + called out in the change's release notes, since external consumers read these attributes even + though nothing in this repository does. +- **FR-017**: `BranchNotFoundError` and `SchemaNotFoundError` MUST be reconciled the same way as + `NodeNotFoundError`. +- **FR-018**: Every existing `except` clause and `isinstance` check in the SDK and CLI MUST still + catch what it caught before the change, with ordered ladders corrected where re-rooting shadows a + later branch. + +#### Removing string matching + +- **FR-019**: The silent token-refresh decision MUST be made from the catalogue code where one is + present, retaining the existing message check only as the fallback for servers that predate the + catalogue. +- **FR-020**: The SDK's remaining message-string checks for catalogued failures MUST be replaced with + typed handling. +- **FR-021**: Checks that detect *uncatalogued* conditions — notably GraphQL schema-validation probing + used for server feature detection — are explicitly out of scope and MUST be left in place. + +#### Messages + +- **FR-022**: A catalogued error's message MUST name the code and the server's message, and MUST NOT + embed the query text. +- **FR-023**: An uncatalogued error's message MUST remain exactly as it is today, query text included. +- **FR-024**: The query and variables MUST remain available as attributes on the exception in both + cases. + +#### Generation and validation (Infrahub repository) + +- **FR-025**: Infrahub MUST generate the SDK's error bindings into the SDK submodule as part of its + existing generation task, alongside the schema models and protocols it already generates there. +- **FR-026**: Infrahub's existing generated-artefact validation MUST be extended to fail when the + submodule's bindings do not match a fresh generation, so a catalogue change that skips regeneration + fails the pull request that made it. +- **FR-027**: No release-time gate is added on either side. Pull-request-time validation is the + mechanism, matching the treatment the existing generated artefacts receive. + +#### Documentation + +- **FR-028**: SDK documentation MUST describe the exception hierarchy, how to catch by branch and by + code, and the cross-version behaviour a consumer can rely on, updated in the same change as the + behaviour. It MUST reference the server's published catalogue for the code list rather than + restating it, so a code added upstream cannot leave the SDK's documentation quietly wrong. It MUST + also note that a catalogued message now names the failing action and resource kind where the + catalogue provides them, since that text reaches logs and CLI output. + +### Key Entities + +- **Catalogue code**: A stable string naming one failure mode, with a declared description, stability + level, HTTP status, and payload schema. Owned by Infrahub; the SDK is a consumer. +- **Error envelope**: What the server puts on the wire for one error. Two shapes exist — the catalogue + envelope on GraphQL, and the legacy integer-code envelope on REST. +- **Payload model**: The typed `data` for one code, tolerant of fields it does not recognise. Used to + validate the envelope and populate the exception's attributes; not the way a consumer reads them. +- **Exception hierarchy**: Rooted at the SDK's `Error`; below it a base for server-reported errors, + splitting into the authentication branch and the GraphQL branch, with one generated class per code. +- **Generated bindings module**: The single artefact crossing from Infrahub into the SDK, holding the + payload models, the per-code classes with their promoted attributes, and the code-to-class + resolution used at raise time. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Every code in the catalogue is reachable as its own exception type, with the catalogue's + payload fields readable as typed attributes on it; a developer can handle any catalogued failure + without reading a message. +- **SC-002**: No message-string matching remains in the SDK for any failure the catalogue covers. +- **SC-003**: The existing test suite passes with no `except` clause losing coverage it had before; + every deliberate behaviour change is pinned by a test that asserts the new behaviour. +- **SC-004**: Every cross-version case — unknown code, unknown payload field, absent envelope, + pre-catalogue integer code — is covered by a test and none of them raises during parsing. +- **SC-005**: A catalogue change that omits regeneration fails validation in the pull request that + introduced it, and a regenerated artefact is byte-identical to a fresh generation. +- **SC-006**: Async and sync clients raise the same type with the same attributes for the same + failure, across all catalogued codes. +- **SC-007**: A catalogued failure's message contains no query text, while an uncatalogued failure's + message is byte-identical to today's. +- **SC-008**: A developer can catch every server-reported error, on either transport, with one + `except` clause. + +## Assumptions + +- **The catalogue is GraphQL-only.** Confirmed against the server: REST responses keep the legacy + integer-code envelope. If REST later adopts the catalogue, the base class introduced here is where + it would attach, but no REST parsing is in scope. +- **Class and code names are the product, not implementation detail.** For an SDK the exception + hierarchy *is* the user-facing contract, so this spec names classes and codes. It deliberately does + not specify module layout, file names, generator implementation, or test framework mechanics. +- **Generation belongs to Infrahub.** The SDK cannot regenerate its own protocols or schema models + today either; those come from Infrahub's generation task writing into the submodule. Error bindings + follow that established pattern rather than introducing a second mechanism, which also removes any + need to keep a vendored catalogue copy in sync. +- **`infrahub_sdk.exceptions` is the supported import path, and it is treated as public.** A consumer + should never need to know which module inside it defines a given exception: every exception the SDK + raises — hand-written or generated — is importable from `infrahub_sdk.exceptions`, and no name + importable from it today may stop being importable from it. Modules beneath it are internal and are + not an import path for consumers. This is a stronger promise than the constitution's tiering + strictly requires, and it is made deliberately, because `infrahubctl`, the Ansible collection, and + external consumers already import from it directly. +- **Three broadenings are accepted deliberately**: `except GraphQLError` will additionally catch node, + branch, and schema lookup misses that never involved a GraphQL request at all — both the client-side + ones and the REST 404 the file handler turns into a `NodeNotFoundError`; it will also catch a real + 401 or 403 whenever the SDK raises a per-code class for it, since only those classes carry both + parents — that is, when the code is recognised and its payload validates, with anything else falling + back to `AuthenticationError` exactly as today; and code that catches the generic error to inspect its + message will now sometimes receive a subclass with a different message. All three follow from + answered decisions rather than oversight. +- **The repository-import failure handling in the git integrator (GitHub #7498) is out of scope**, as + is any change to what the server emits. +- **Both repositories are in scope for this document.** Requirements FR-025 to FR-027 land in the + Infrahub repository and must be executed from that checkout; everything else lands here.