Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
2af53e7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://604faf7c.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-error-catalogue-ifc-3034.infrahub-sdk-python.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-3034-error-catalogue/quickstart.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/quickstart.md:1">
P3: This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,196 @@ | |||
| # Quickstart: validating the error catalogue in the SDK | |||
Contributor
There was a problem hiding this comment.
P3: This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/quickstart.md, line 1:
<comment>This is a pure documentation/specification change (dev/specs/...), so it can't affect a running product and should ship on the stable release vehicle rather than the develop train. Per the release-vehicle guideline, pure docs changes belong on stable; target develop instead.</comment>
<file context>
@@ -0,0 +1,196 @@
+# Quickstart: validating the error catalogue in the SDK
+
+Runnable checks that prove the feature works end to end. Each scenario names what it proves and the
</file context>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Specification, implementation plan, research decisions, data model, interface contracts, and a validation quickstart for making ordinary SDK operations raise the specific exception for the failure the server reported, on both the async and sync clients, without ever raising on a payload the SDK does not recognise. The design in brief: - A new `ApiError` base carries the parsed envelope — catalogue code, declared HTTP status, raw extensions, and the server's error list — with `GraphQLError` and `AuthenticationError` descending from it. One raise-time factory serves every existing raise site, so the code is readable against any server version even with no generated bindings present. - A catalogued error's payload is read as directly typed attributes on the exception (`exc.node_kind`, `exc.fields`), typed exactly as the catalogue declares them. The pydantic payload model validates the envelope and populates them; it is not the access path. Nothing is typed `Any` beyond raw decoded JSON, and no type-check suppression is anticipated. - `infrahub_sdk/exceptions.py` becomes a strictly layered package — hand-written base, generated catalogue, factory, façade — with imports pointing only downward and a test enforcing it. `infrahub_sdk.exceptions` remains the one supported import path, and a snapshot test pins that no name importable from it disappears. - Codes declaring 401 or 403 descend from both branches. Infrahub returns HTTP 200 for resolver-raised errors, so a permission failure arrives on the data path inside a response `except GraphQLError` catches today. - The raised class is a function of the response's first error code and the transport observed — never of payload validity, binding freshness, or a code's declared status. - Infrahub generates the bindings into the submodule from `backend.generate`, alongside the schema models and protocols it already generates there, and `backend.validate-generated` fails when they are stale. Three broadenings are accepted deliberately and recorded in the spec, together with the `NodeNotFoundError.identifier` widening that unification forces. Requirements FR-025 to FR-027 land in the Infrahub repository; everything else lands here.
ogenstad
force-pushed
the
pog-error-catalogue-IFC-3034
branch
from
September 2, 2026 16:04
a1e35dd to
a35e9c1
Compare
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Seven valid findings, three rejected. Most had been present since the original plan commit and survived six incremental reviews, which only ever read the delta. Valid: - The plan's Constraints section still said the raised class never depends on payload validity or on the transport observed, contradicting FR-004 and FR-012. Stale since the E3 reversal, and it would have led an implementer to route a payload-invalid 401/403 code to the authentication branch and lose the `except GraphQLError` coverage FR-018 requires. - Nothing documented set `code` on the three adopted classes, so the `exc.code is not None` test for "came from the server" could not work. The factory sets it per-instance there; a class attribute cannot, since the same class must report None on a client-side raise. - FR-002's "the base GraphQL error itself" was ambiguous once FR-001 introduced a shared base above both branches. It now names that base and excludes the per-code classes. - FR-003 and FR-012 described `code` as "absent" while two acceptance scenarios promise `None` — different observable contracts. Standardised on always-exists-and-may-be-None. - The Messages guarantee claimed every catalogued failure names the server's message, but three catalogued classes can be raised client-side with no server response. Qualified in FR-022 and the contract. - data-model.md said "nothing here is typed Any" two paragraphs above an `extensions: dict[str, Any]` row. - A local absolute developer path in plan.md. Rejected, but the gap behind two of them fixed: the catalogue counts and the `UNIQUENESS_VIOLATION` example are correct for `opsmill/infrahub@develop`, which this feature pairs with, and wrong only against the stable line the reviewer read. Nothing in the artefacts said which ref the numbers came from, so the survey now pins `develop` and records that US1 scenario 1 needs a catalogue containing that code. The third rejection — that the "Expired Signature" grep should expect two sites — misses that R10 puts both the code check and the legacy fallback in one shared helper; R10 and the quickstart now say so explicitly.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The Messages qualifier said the three unified classes are raised "with no server response behind them", but the file handler raises NodeNotFoundError from a REST 404 that does carry a response and a message — a case the broadenings list two paragraphs above. Restated around the operative property instead: no catalogue code behind them, which covers both the client-side miss and the REST 404, whose response carries the legacy envelope. FR-022 corrected the same way. The critique's trend paragraph also claimed all seven ninth-pass defects had survived since the original plan commit, while the section it summarises says "most" — and its own example only became a contradiction at the E3 reversal.
The three 401/403 codes no longer get their own exception class. They are the only codes that reach the SDK on two different transports, and each transport already has a class an existing `except` clause depends on: a real 401/403 raises AuthenticationError, and a resolver-raised failure inside an HTTP 200 raises GraphQLError. The transport rule already in FR-012 picks the right one, and `exc.code` carries the identity. One class per code could satisfy both only by inheriting from both branches. That diamond had already produced three defects, all found in review rather than by design: the positional-argument corruption, since method resolution handed those classes GraphQLError.__init__ whose first parameter is `errors`; the cooperative super().__init__ reaching AuthenticationError.__init__ on the GraphQL path, where its default message escaped substitution only by accident; and two generated class shapes instead of one. It also misread as GraphQLError descending from AuthenticationError on first contact. All to distinguish three codes whose payloads are empty or entirely nullable and usually unset. The hierarchy is now a tree with no class having more than one parent. FR-008 changes from "which parents" to "which codes get classes" — one rule, and the factory needs no special case, since the lookup simply misses for those codes. Amended: US3's acceptance scenarios, which specified distinct types before the HTTP 200 behaviour was verified, plus FR-005, FR-008, SC-001, and the Key Entities description. The third broadening added in the previous round disappears, because a real 401/403 once again produces AuthenticationError and nothing else. Accepted cost: those three codes have no typed payload attributes, reachable only through `exc.extensions["data"]` if the catalogue later gives them substantive fields.
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/ifc-3034-error-catalogue/data-model.md">
<violation number="1" location="dev/specs/ifc-3034-error-catalogue/data-model.md:76">
P2: The new AuthenticationError text says "It gains no subclasses", but spec.md FR-015 still promises the opposite: "MUST ... remain the class raised for REST authentication failures, while gaining the three catalogue subclasses beneath it" (dev/specs/ifc-3034-error-catalogue/spec.md:352). At the current PR head the two spec documents contradict each other on whether AuthenticationError has subclasses. Update FR-015 to drop "while gaining the three catalogue subclasses beneath it", and audit FR-012's rationale, which still refers to "the dual inheritance in FR-008" after FR-008's dual-inheritance wording was removed.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| ## AuthenticationError | ||
|
|
||
| Name, constructor, and default message unchanged (FR-015). It gains no subclasses and inherits the |
Contributor
There was a problem hiding this comment.
P2: The new AuthenticationError text says "It gains no subclasses", but spec.md FR-015 still promises the opposite: "MUST ... remain the class raised for REST authentication failures, while gaining the three catalogue subclasses beneath it" (dev/specs/ifc-3034-error-catalogue/spec.md:352). At the current PR head the two spec documents contradict each other on whether AuthenticationError has subclasses. Update FR-015 to drop "while gaining the three catalogue subclasses beneath it", and audit FR-012's rationale, which still refers to "the dual inheritance in FR-008" after FR-008's dual-inheritance wording was removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/ifc-3034-error-catalogue/data-model.md, line 76:
<comment>The new AuthenticationError text says "It gains no subclasses", but spec.md FR-015 still promises the opposite: "MUST ... remain the class raised for REST authentication failures, while gaining the three catalogue subclasses beneath it" (dev/specs/ifc-3034-error-catalogue/spec.md:352). At the current PR head the two spec documents contradict each other on whether AuthenticationError has subclasses. Update FR-015 to drop "while gaining the three catalogue subclasses beneath it", and audit FR-012's rationale, which still refers to "the dual inheritance in FR-008" after FR-008's dual-inheritance wording was removed.</comment>
<file context>
@@ -74,9 +73,15 @@ default.
-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.
+Name, constructor, and default message unchanged (FR-015). It gains no subclasses and inherits the
+`ApiError` attributes. It remains the class raised for every failure the SDK observed as HTTP 401 or
+403, with `code` set per-instance by the factory: `None` on the REST path, which carries no catalogue
</file context>
Neither provenance can populate a unified class's full attribute set, and they fail to in both directions: the catalogue supplies no `branch_name` at all, while four of the seven client-side raise sites supply no node kind and fall back to "unknown". So a promoted attribute on `NodeNotFoundError`, `BranchNotFoundError`, or `SchemaNotFoundError` must be optional even where the catalogue declares the underlying field required — a required attribute is a promise a class raised from two provenances cannot keep. Whether the SDK could fill a future catalogue field client-side depends entirely on the field, which is why the policy has to be optional-by-default rather than decided per field. Recorded with a tripwire for revisiting: an adopted code gaining a field that is both required and semantically server-only. Also corrects this plan's own framing of R9. Earlier rounds treated the unification as the design's weak point. A split is in fact more expensive than the optional attribute, because the derived name for NODE_NOT_FOUND is the existing class name — so splitting means breaking FR-005, FR-006, or every `except NodeNotFoundError` around a store lookup. Records the direction of travel the maintainer set: separate classes for the SDK's own failures, which would retire `identifier`'s dual meaning and make `branch_name` coherent, reached through the constitution's deprecation path as a later change rather than inside this one.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Five findings from the review of the two design commits, all valid at least in part. Two were real contradictions the MI removal left behind: - FR-015 still promised "gaining the three catalogue subclasses beneath it" while the data model said AuthenticationError gains none, and FR-012's rationale still cited "the dual inheritance in FR-008" after that wording was removed. - The catching table still promised `except AuthenticationError` for "any authentication or permission failure, either transport", which the tree design does not deliver: a resolver-raised permission failure inside an HTTP 200 raises GraphQLError. The table and the example described the diamond's capability. `except ApiError` is now named as the clause that spans both arrival paths. Rejected the finding's FR-018 claim, which measured coverage against the unshipped diamond rather than against today's behaviour, where `except AuthenticationError` does not catch such a response either. Three were accuracy fixes: - R5 said "twelve generated classes" where nine are generated and three adopted. - The optional-attribute row justified itself as "raised without a server response", but the REST 404 carries a response and merely no catalogue code. Same error already fixed in the Messages section, reintroduced verbatim. - The NodeNotFoundError construction-site counts were wrong. Re-derived from source: nine sites, eight `raise` plus one deferred construction at store.py:184; node_type supplied by five, omitted by the four store raises; identifier a mapping at eight and a plain string at one.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The worked examples commented that TOKEN_EXPIRED and AUTHENTICATION_REQUIRED arrive as a real 401/403 while PERMISSION_DENIED is the resolver-raised one. R5 records the opposite, having verified it: the formatter maps any error escaping a resolver onto a catalogue code, AuthorizationError included, so all three can arrive inside an HTTP 200. The example would have sent a consumer to `except AuthenticationError` and had them miss every 200-path arrival of the same code. The examples now reference the arrival table instead of restating it, and lead with `except ApiError` as the form to reach for. US3 scenario 5 generalised from "a permission failure" to "any of the three authentication codes". This is the fifth restated-mechanism defect, so the mitigation is recorded as a rule rather than another fix: state each load-bearing mechanism in one place and point at it from everywhere else.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The review finding: the two catch forms were in one code block, and since AuthenticationError descends from ApiError, copying the block as a handler sequence leaves the final clause unreachable. Split into alternatives with the shadowing stated. The ast discovery walk (R3, R4) was executed against the real sources. It finds all 32 classes in exceptions.py and classifies every catalogue code as the plan claims: nine to generate, three with no class, three colliding. It confirms the 9/3/3 split independently, fires the collision check on VALIDATION_ERROR, RATE_LIMIT, INVALID_RESPONSE and TIMESTAMP_FORMAT, and is correctly blind to an inherited CODE. It also surfaced an ordering constraint the plan had not stated: generation aborts until base.py declares the three CODE attributes, so adoption is a prerequisite for the generator running at all, not just for its output. Recorded in R4 and as a prerequisite section in the plan, since the instinct is to build the generator first. The generated class shape (R15) is clean under both mypy 1.11.2 and ty 0.0.14 with zero suppressions. The types a consumer sees were asserted by typed assignment rather than inspected, so a wrong one would have been an error: node_kind is str rather than optional or Any, fields is list[str], code is str | None, and from_payload returns the concrete class through Self.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The review found R4's spike paragraph enumerating a different collision set from R3's in the same document, four codes against five. Both lists were factually correct, which is why neither had been caught: the defect is that two lists each read as *the* set. Reconciled to one list of six in R3, verified against the current module, with R4 referring to it rather than repeating it. A sweep of the ten load-bearing mechanisms against their authoritative statements found two more, both stale text describing a superseded design: - R1's decision said "a package of five modules" above a four-row layer table, left over from when the payload models had their own module. - The plan's summary said "12 on the GraphQL branch, 3 on the authentication branch", which describes the diamond. There are no classes on the authentication branch now. And one in the same family: the quickstart's message expectation lacked the "server-reported" qualifier that FR-022 and the contract carry, so it read as promising a catalogued message for a client-side raise. The other seven mechanisms are consistent: transport-to-class (including a check for the inverted mapping, which appears nowhere), the `exc.code` contract, the payload-validation fallback, generation placement, the broadening count, promoted attribute optionality, and the raise-site counts.
Phase order follows dependency rather than the priority labels, which the plan's Sequencing section requires but which reads as wrong at first glance: US1 is P1 and lands last among the behaviour phases. Two constraints force it. The three CODE declarations are a prerequisite for generation succeeding at all, not merely for its output, so the US3 hierarchy work precedes the generator; and the generator produces the per-code classes US1 delivers. US2 and US4 need no bindings, so they land first. Also records the mapping from tasks to pull requests, since 83 tasks reads as far more delivery than this needs. Four issues: the envelope with no bindings, the re-rooting, the typed per-code classes, and the Infrahub generator. The boundaries come from the repository split, the SDK-first landing order, and keeping the compatibility-sensitive re-rooting under its own review rather than buried in a restructure. The one inversion worth stating: the generator issue is developed before the typed-classes issue but merges after it, because that pull request has to carry the generated artefact before Infrahub's content-level validation can pass against the pointer it bumps to.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
ogenstad
changed the title
Review finding: the section heading claimed phase order was dependency order, while the document's own dependency list two screens down showed US2, US3, US4 and US6 depending on Foundational alone, and the parallel-opportunities bullet said three of them run concurrently. The heading was the load-bearing statement a reader would act on, so it would have serialized four independent phases. Only two adjacencies are actually forced: US3 before US5, because the CODE declarations are a prerequisite for generation succeeding at all, and US5 before US1, because the generator produces the classes US1 delivers. Both still cut against the priority labels, which was the point the section existed to make, so the correction narrows the claim rather than removing it. The parallel-opportunities bullet also understated the set and overstated the independence: it named phases 3, 5 and 6 but not 4, and said the three share no files beyond the factory. US3 and US6 both add cases to test_exceptions.py. Both overlaps are now named, since they are what actually needs coordinating.
…#1348) * feat: expose the server error envelope on SDK exceptions Turn infrahub_sdk/exceptions.py into a package and funnel every authentication and GraphQL raise site through two raise-time factories, so envelope parsing lives in one place instead of being repeated at fifteen call sites. AuthenticationError and GraphQLError now share an ApiError base carrying the parsed envelope (code, http_status, extensions, errors), letting a caller branch on the server's catalogue code rather than matching on message text. Exceptions raised from a status code alone, such as URLNotFoundError and RateLimitError, deliberately stay under Error. Also fixes an AttributeError escaping the client when a 401 carried a body that was valid JSON but not an object, which a proxy or gateway can produce. The factories are total: any shape the parser cannot read degrades to the generic exception rather than to an SDK error of our own. * docs: record where the implementation diverged from the task list Six tasks landed differently from how they were written. Annotate each with what shipped and why, in the style T020 and T021 already use, so the plan stays usable by whoever picks up the later issues. The one that would have bitten: T009's optional `message` parameter is deferred to its first caller rather than added unused, so T035 and US6 now say to add it there instead of assuming it already exists. * fix: raise AuthenticationError when a file upload is rejected The multipart upload path called raise_for_status() bare, so a 401 or 403 on a file upload surfaced as a raw httpx.HTTPStatusError while every other request raised AuthenticationError. FR-015 asks for the same class on every failure the SDK observes as 401 or 403, and an upload is no exception. The silent token refresh already covered this path, since _post_multipart carries the relogin decorator; only the error type was inconsistent. Also narrows the DOC501 suppression from the global ignore list to the three modules that raise from the factories, so the rule keeps working across the rest of the SDK, and corrects the fixture README: an integer extensions.code is a pre-catalogue shape on either transport, not a REST marker, which the graphql_integer_code fixture already demonstrated. * refactor: suppress the DOC501 false positives at the six sites, not by file Replaces the per-file DOC501 ignores with a noqa on each docstring that actually triggers one, leaving no config-level suppression at all. Ruff anchors the diagnostic on the docstring rather than the raise, so the directive goes after the closing quotes; nothing on the def or raise line has any effect. Narrowing it immediately paid for itself: the per-file ignore had been hiding a real omission, in that both multipart methods re-raise httpx.HTTPStatusError for a non-401/403 status and neither docstring said so. Both now document it. RUF100 is enabled repo-wide, so these directives report themselves as unused if ruff ever resolves a factory call to its return type, which a per-file ignore would never have done. * fix: keep the reason and status on an unreadable authentication response The message join overwrote the HTTP-status fallback unconditionally, so a 401 whose body is the REST API's bare {"detail": ...} lost both the server's stated reason and the status, surfacing only the generic default message. Each fallback now applies only where the one before it yielded nothing: the server's messages, then detail, then the plain status. The body is also read through response.json() rather than utils.decode_json, whose one addition is to raise JsonDecodeError -- which this factory caught and discarded, having been built to absorb exactly that case. It cost a deferred import inside the function body, since utils imports this package, so dropping it leaves the exceptions package depending on nothing else in the SDK. token_expired_in exposes the refresh decision the client had been reaching in for through three private helpers. * fix: report a GraphQL failure infrahubctl cannot read as an envelope Filtering exc.errors to dicts left every renderer with a payload it drops entirely: print_graphql_errors emitted nothing, and infrahubctl run and validate graphql reported "0 error(s)" with no diagnostics, each still exiting non-zero. The isinstance(error, str) branch that used to carry the --branch hint became unreachable at the same time. print_graphql_errors takes an optional fallback so the existing signature keeps working, and the two command renderers now share print_graphql_query_errors, which keys the hint on the server's message and degrades to the exception's message rather than printing a count of nothing. * fix: tolerate any body on a file 404 The 404 branch sat directly below the hardened 401/403 one and still called response.json() bare, so an HTML error page from an intermediary escaped as json.JSONDecodeError and a JSON array as an AttributeError on .get() -- the same crash class the branch above it exists to prevent. Narrowing the identifier to a str made the type checker report a violation that had been there all along, so NodeNotFoundError.identifier is widened to Mapping[str, list[str]] | str. The SDK already raised it with a plain string to name a missing file; nothing changes at runtime. * refactor: route the refresh decision through the factory, not its privates client.py imported _catalogue_code, _extensions_of and _server_messages across a module boundary, which undercut the factory's claim that envelope parsing lives in one place. It now calls the public token_expired_in instead. The retry that decision drives is also skipped for a client authenticating with an API token: login(refresh=True) returns without touching the auth header unless there is a username and password behind it, so the retry replayed the token the server had just rejected and earned a second 401 for nothing. The layering test asserted only that imports inside the exceptions package point downward, which was half the property -- every other module is free to raise, so a dependency in that direction is a cycle waiting to be found, and the deferred import that works around one is invisible to an intra-package check. It now also fails on any import reaching elsewhere in the SDK, in either spelling. * docs: reconcile the task list and contracts with what the fixes landed T040 and T041 were scheduled for the next pull request, but both cover regressions this one introduces, so they are pulled forward with T036 and T078 and the pull-request split is renumbered. T012 prescribed decode_json and T015 prescribed only the intra-package check; both now record why the implementation went further. The http_status contract claimed the wire status stays available in extensions. It does not: exc.http_status is a copy of extensions["http_status"], so the status the transport observed is not on the exception at all. Both statements of that rule are corrected, and the distinction they were reaching for is attributed to the generated classes that will introduce it. * fix: catch a dependency on the SDK root in the layering check The predicate matched only names beginning with "infrahub_sdk.", so "from infrahub_sdk import utils" and "import infrahub_sdk" were not flagged -- both pull in the facade, which imports the client, so the check permitted exactly the dependency it exists to forbid. Both spellings are now cases in the detection test. * docs: stop three changelog notes claiming more than the fixes cover The validate subcommand is graphql-query, not graphql. The 404 fix is download-only: an upload goes through the GraphQL multipart path, never handle_error_response, so a 404 there still raises httpx.HTTPStatusError. And _get_streaming carries no relogin decorator, so streaming downloads never replayed an expired-token 401 and cost nothing to begin with. * docs: pin the http_status rule to the condition the server applies Both statements of the rule hand-waved at the server replacing a declared 500 "when it has a more accurate one". The substitution has an exact trigger -- the catalogue resolving nothing more specific than 500 -- so the envelope carries the catalogue's status in every other case, and that is what a reader needs to know before branching on it. The decode_json rationale also described the exception type as its only addition, understating it: JsonDecodeError carries the response URL and body, and this factory discards both deliberately, since the status and the server's reason are what the caller needs on that path. The issue 2 task count now names its range rather than leaving the reader to guess which tasks it counts. * refactor: declare the exceptions package's public surface explicitly The package had no __all__, so "import *" fell back to every public name in the namespace -- which includes the base and factory submodules, since importing them makes them attributes of the parent. Callers were handed two names that are an artefact of the layout and that shadow those names in their own scope. The facade now declares __all__, taken from base so there is one list rather than two, and the wildcard is exactly the exception classes: what an end user catches. token_expired_in also comes out of the factory module's __all__. Nothing star-imports that module, so the list is documentary, and it had mirrored the names the facade re-exports; advertising a third that does not leave misdescribed the surface. The name stays unprefixed and the client keeps importing it -- public to the SDK, not published API. The two raise-time factories are no longer in the wildcard either, and a test asserts they are still importable by name. Both are new in this pull request, so nothing released can be wildcard-importing them. * refactor: write out the exceptions the package exports instead of star-importing A wildcard hides what the package exports, which is the one thing this file exists to state, so the class list is written out and reading __init__.py now answers the question. The cost is two hand-written lists, held in step by three tests: the facade's __all__ against base.__all__, every declared name against what the import block actually binds, and base.__all__ against the classes base defines. That last one closes a gap nothing else covered: a class omitted from base.__all__ is absent everywhere downstream, so the snapshot still matched and no test noticed. Each guard was checked against a deliberately drifted copy of the lists to confirm it fails rather than merely passing today. * docs: name the shape that actually raised a TypeError The fragment blamed a body carrying no "errors" key. That case joined an empty list of messages, which AuthenticationError turns into its generic default -- an authentication error that silently dropped the status, not a TypeError. The TypeError came from an errors array whose entries have no message key, where joining a list containing None raises. Both are now stated as they happened.
* feat: unify and re-root the not-found classes under GraphQLError NodeNotFoundError, BranchNotFoundError and SchemaNotFoundError now descend from GraphQLError and declare the catalogue code they represent, so one class covers a lookup miss however it arose and their envelope state is set by the constructor that owns it. NodeInvalidError inherits the re-rooting. A failure the server reported with a catalogue code now carries a message naming that code and the governing error's message in place of the query text. An uncatalogued failure keeps today's message byte for byte, and the complete error list, the query and the variables stay readable on the exception. infrahubctl gains a branch above its class ladder keyed on the code, so a catalogued failure is reported by its code rather than mislabelled as an authentication failure or rendered as a bare error list. The lookup-miss branch moves above GraphQLError, which the re-rooting would otherwise shadow. Two broadenings are deliberate and carry changelog fragments: except GraphQLError additionally catches client-side lookup misses, and a catalogued message changes shape for anyone matching on its text. * fix: keep the failed operation's path in catalogued CLI output The catalogued branch rendered the server's error list only when there was more than one error, so a single-error failure lost the path naming the operation that failed -- which infrahubctl printed before this branch existed. The coded line has nowhere to put a path, so where the server sent errors they now render the detail and the code line just names the failure, which keeps the path without printing the governing message twice. print_graphql_errors falls back to an error's message instead of dumping the whole entry as a dict when it carries no path, so an authentication envelope reaching it reads as the server's words rather than as decoded JSON. * fix: keep uncatalogued CLI rendering byte-identical Falling back to an error's message when it carries no path was applied to the shared renderer, so it also changed uncatalogued failures -- and a GraphQL validation error is exactly that shape, carrying `locations` and no `path`, which meant the coordinates pointing at the offending part of the query were dropped from the output. The message-only fallback belongs to the catalogued branch alone, where the code has already named the failure, so it moves to its own renderer and the shared one goes back to printing the raw entry. * fix: distinguish a described failure from a merely coded one The server codes every error it reports, falling back to UNDEFINED_ERROR where its own catalogue has no entry, so `exc.code is not None` was never the test for "the server described this". The short message form therefore applied universally, dropping the query text and every error after the first from what reaches a log or a traceback, and GraphQL validation errors took the coded CLI branch where their line and column were discarded under a headline that named nothing. code_names_the_failure() is that test now, and both factories and the CLI ladder key on it. UNDEFINED_ERROR stays readable on exc.code; it simply no longer shapes the message. The authentication factory also follows the rule the GraphQL path already documented: only the governing error's message may be named beside a code, with the rest on exc.errors. A failure the server reports under an adopted code raises the class that adopted it, built from the payload through that class's own from_payload, so NODE_NOT_FOUND raises NodeNotFoundError rather than a generic GraphQLError. query_groups drops the substring match that stood in for it, which only matched while the whole error list was embedded in the message, and its sync counterpart gains the guard it never had. This covers the three adopted codes only; the generated bindings still own the rest. Smaller corrections in the same area: - every message infrahubctl prints is escaped, so a branch name in brackets is no longer eaten as console markup - GraphQLError keeps a deliberately empty message instead of replacing it with the query placeholder - NodeInvalidError clears CODE rather than inheriting NODE_NOT_FOUND, since a wrong-kind result is not a lookup miss - the two CLI error renderers become one function with a flag, keyed on a path that is present and not null - the JSON importer reads an error's message with .get, so an entry carrying none cannot raise out of the continue-on-error path - the ruff per-file F403 ignore for the exceptions facade goes, since the facade writes its imports out rather than star-importing CODE is declared ClassVar[str | None] so a subclass can clear it, which makes the adopted declarations annotated assignments, so the generator contract now names both AST forms for its adoption walk. * refactor: re-export only the names that have a caller UNDEFINED_ERROR_CODE and the three payload protocols were added to the exceptions facade so that a downstream annotation would not have to reach into a private submodule, but nothing in the repository reads any of them. The facade is a documented stability promise, so exporting a name with no caller commits us to supporting it in exchange for nothing. The protocols stay importable from `.base` for anything that needs them, and their shapes are already kept honest by the factory's `from_payload` call sites type-checking against them. The three that remain keep the `X as X` spelling, which is not redundancy: it is what marks a name as re-exported rather than merely imported, and is the form ruff's F401 accepts for an import nothing in this file uses. The module docstring now says so, along with the rule that avoids the next round of this: a name in the facade earns its place by having a caller rather than by being plausibly useful one day. * fix: name a code only when the governing error said something Review follow-up on the catalogue envelope handling. _named_by_code returned a bare code when the governing error carried no message, which is a worse headline than what either call site would otherwise have produced and drops the reasons living in the later errors. On the authentication path the `or message` fallback meant to catch that could never fire, since the bare code is truthy, so the joined reasons were lost. Both factories now name the code only where the governing error actually said something, which also leaves an adopted class its own sentence rather than replacing it with a code. delete_unused keeps a fallback for servers older than the catalogue. Those report a cascade-deleted member as a generic failure with the reason only in the message, so keying purely on NodeNotFoundError turned a graceful skip into an aborted group update against every server before 1.10.10. The legacy check is guarded on there being no code, so a coded failure reaching it is a different failure and is never swallowed on the strength of its wording. The JSON importer's execute_batches bound the (node, result) pair its batch yields to a single name, so every check of the result looked at a tuple, which no task can ever produce. A failed task was counted as a success: nothing printed, --continue-on-error had nothing to report, and the exception object was returned among the imported results. Unpacking the pair makes the error branch reachable for the first time, which is also what lets it be tested. Also: the ctl fallback branch escapes its traceback, so bracketed text in an unknown exception is not eaten as console markup; the broadenings in the spec and the hierarchy contract are no longer counted, since adopting a further code adds one; and the changelog no longer implies dispatch applies to payloads the SDK deliberately leaves generic. * docs: name the command the importer fix actually affects The changelog fragment for the execute_batches fix named `infrahubctl transfer import`, which is not a command. `load` is registered at the top level in cli_commands.py, so the command a reader has to run is `infrahubctl load`, and the fragment is renamed to match. Also restores a dropped article in the adopted-codes fragment. * revert: take the importer batch-unpacking fix out of this branch execute_batches never unpacks the (node, result) pair its batch yields, so its error branch is unreachable and a failed import task is counted as a success. Fixing that here made the branch reachable for the first time, which turned six test_export_import integration tests red: update_optional_relationships gets an HTTP 500 from the server, and those tests had been passing only because the importer swallowed it. That is a real fault on both sides and neither belongs to the error catalogue, so both go to their own issue rather than holding this branch red behind a server bug. The importer is back to its previous behaviour, along with the tests and changelog fragment that described the fix. The defensive `.get` on an error entry's message stays. It guards code that cannot currently run, so it is untested by design until the unpacking is fixed, and it costs nothing to leave correct in the meantime. * docs: cut the comments back to what the code cannot say Review feedback on one four-line comment, applied to the three others in this branch that had the same problem. Each kept restating the line beneath it and describing behaviour that belongs to the caller, leaving the one genuinely non-obvious fact buried at the end. What survives is that fact alone: why GraphQLError tests `is not None` rather than the `or` every sibling class uses, and why an adopted class needs its envelope attached after the fact. The rest was either evident from the code or already documented where it belongs.
The transient-retry suite pinned the legacy `An error occurred while executing the GraphQL Query` text on envelopes whose governing error carries a catalogue code, which now name the failure by that code instead. The expected message moves onto the parametrized case so the uncatalogued one keeps asserting the legacy form. `ty` 0.0.74 no longer reads the mypy ignore comments on two of the new test files, so their violations move to a scoped override.
ogenstad
force-pushed
the
pog-error-catalogue-IFC-3034
branch
from
September 17, 2026 07:48
e047546 to
2af53e7
Compare
github-actions
Bot
added
the
type/documentation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## infrahub-develop #1266 +/- ##
====================================================
+ Coverage 86.28% 86.49% +0.21%
====================================================
Files 149 151 +2
Lines 14537 14497 -40
Branches 1995 1987 -8
====================================================
- Hits 12543 12539 -4
+ Misses 1431 1388 -43
- Partials 563 570 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Specification, plan, and task breakdown for IFC-3034. No behaviour change: this adds
dev/specs/ifc-3034-error-catalogue/covering how the SDK consumes Infrahub's GraphQL error catalogue, so that ordinary operations raise the specific error for the failure.GraphQLErrorremains the fallback for failures the catalogue does not cover and stays the common base class, so existingexcept GraphQLErrorcode keeps working.Ref: IFC-3034. Related: IFC-2279 (spike), INFP-468 (backend catalogue), GitHub #7498 (out of scope).
Previously approved work
Decisions settled while drafting
ApiErrorbase above bothAuthenticationErrorandGraphQLError. Authentication failures reach consumers from the REST path as well as GraphQL, so they cannot simply be re-rooted underGraphQLError. Verified that a 401/403 on a GraphQL call is already handled as anhttpx.HTTPStatusErrorand raisesAuthenticationErrorbefore the body is parsed for GraphQL errors — soexcept GraphQLErrornever caught auth failures, and no dual inheritance is needed to preserve compatibility..codeis a catalogue string orNone. The/api/...envelope'sextensions.codeis an integer mirroring the HTTP status, a different thing with a different type; it is not surfaced through.code. The catalogue is GraphQL-only today.GraphQLError— rather than a hand-maintained per-code mapping.python_sdksubmodule, matching howprotocols.pyand the generated schema models already arrive. No copy of the catalogue schema is vendored here, so there is one freshness invariant instead of two, policed by extending Infrahub's existingvalidate-generatedcheck. No release-time gate is added on either side.NodeNotFoundError,BranchNotFoundErrorandSchemaNotFoundErrorare unified with their catalogue counterparts and re-rooted underGraphQLError, accepting thatexcept GraphQLErrornow also catches client-side lookup misses.Findings from the code survey worth a reviewer's eye
These are in the spec's Edge Cases section as specific hazards, not hypotheticals:
isinstanceladder gets shadowed.infrahub_sdk/ctl/utils.py:58-72testsGraphQLErrorat line 67 before(SchemaNotFoundError, NodeNotFoundError, ...)at line 70. Re-rooting those classes makes the later branch unreachable, silently changing CLI output for exactly the errors this feature makes specific. FR-018 requires the correction.GraphQLErrorbranch rendersexc.errors, a list of server error dicts. A unifiedNodeNotFoundErrorraised purely client-side has no server response behind it, so the list is empty.identifiercarries two types. The existing client-sideNodeNotFoundErrorhasidentifieras a mapping of filters; the catalogue payload has it as a single string. FR-016 mandates the unification; the reconciliation mechanism is left to the plan.NodeInvalidErrorsilently inheriting the re-rooting, a pre-existing call site passing a string whereGraphQLErrorexpects a list of error dicts,UNDEFINED_ERRORbeing a real code rather than the absence of one, and GraphQL data errors arriving as HTTP 200 while auth failures arrive as real 401/403 on a separate code path.Scope
Six prioritised user stories, 28 functional requirements. FR-025 to FR-027 land in the Infrahub repository (generation plus the extended drift check) and are tagged as such; everything else lands here.
tasks.mdbreaks the work into 83 tasks and maps them onto four pull requests: three here (the envelope with no bindings, the re-rooting, the typed per-code classes) and one in Infrahub (the generator). Phase numbering is a nominal sequence; only US3-before-US5 and US5-before-US1 are dependency-forced.Checks
rumdlclean across 131 files; Vale flags nothing in the new files. Requirements checklist atdev/specs/ifc-3034-error-catalogue/checklists/requirements.mdpasses 16/16 with no[NEEDS CLARIFICATION]markers remaining.Summary by cubic
Makes the SDK consume Infrahub's GraphQL error catalogue: authentication and GraphQL failures surface the server's error envelope, and the three lookup-miss codes now raise their own classes under
GraphQLError. Also lands the IFC-3034 specification, plan, contracts, and 83-task breakdown indev/specs/ifc-3034-error-catalogue/.What changed
AuthenticationErrorandGraphQLErrorshare a newApiErrorbase carryingcode,http_status,extensions, anderrors, so callers can branch on the server's catalogue code instead of matching message text.NODE_NOT_FOUND,BRANCH_NOT_FOUND, andSCHEMA_NOT_FOUNDraiseNodeNotFoundError,BranchNotFoundError, andSchemaNotFoundError; all three descend fromGraphQLError, andNodeInvalidErrorinherits the re-rooting withCODEcleared.UNIQUENESS_VIOLATION: Node of kind TestPerson already has name 'John') instead of embedding the query text; uncatalogued failures keep the old message verbatim, and the full error list stays onexc.errors.infrahub_sdk/exceptions.pybecame a strictly layered package (base,factory, façade) with raise-time factories for every auth and GraphQL raise site;infrahub_sdk.exceptionsstays the one supported import path, pinned by a snapshot test.AuthenticationErrorwith the best reason available (server messages, then RESTdetail, then the plain status) instead ofJsonDecodeError,TypeError, or the generic default message; the same parsing fixes anAttributeErroron a valid non-object body, and a file 404 now always raisesNodeNotFoundErrorwhatever the body contains.infrahubctlprints the exception's message instead of "0 error(s)" for an unreadable GraphQL failure, and escapes bracketed text so rich no longer eats branch names.NodeNotFoundError.identifieris widened toMapping[str, list[str]] | str, documenting a shape the SDK already raised.Design notes
code_names_the_failure()separates a described failure from a merely coded one:UNDEFINED_ERRORstays readable onexc.codebut does not shape the message.GraphQLErrorremains the fallback for anything the catalogue does not cover.Written for commit 2af53e7. Summary will update on new commits.