From 75430e90a6e34877e881a3f6282ef290731f7603 Mon Sep 17 00:00:00 2001 From: Patrick Ogenstad Date: Wed, 26 Aug 2026 09:04:13 +0200 Subject: [PATCH 1/3] docs(agents): port SOLID and code-standard rules from Infrahub The Infrahub repository carries four agent rules under .agents/rules; this repository carried the testing and comment ones but had no equivalent for component design or module layout, and the two it shared were narrower than their Infrahub counterparts. Ports the missing structure, rewritten for the SDK rather than copied: - component-design.md: SOLID/DI rules grounded in the SDK's own worked examples (RateLimitRetryHandler, the transfer exporter/importer interfaces, DataProcessor, the ctl commands as composition roots), plus an SDK-specific section on keeping decision logic out of the async/sync split. - python-module-layout.md: constants.py holds constants only, imports at the top (PLC0415), async/sync variants stay in the same module. - code-comments.md: adds "no references to other code" and what good documentation looks like, carving out published docstrings, which are user-facing reference docs here. - python-testing.md: adds exact-expectation assertions, don't test the framework, cheapest test tier, no process-global leakage, test doubles in place of mocks, and full-message exception matching. Also fixes the tests/AGENTS.md async example, which called a method that does not exist and demonstrated the non-assertion the new rules forbid. --- .agents/rules/code-comments.md | 28 ++++++ .agents/rules/component-design.md | 123 ++++++++++++++++++++++++++ .agents/rules/python-module-layout.md | 26 ++++++ .agents/rules/python-testing-unit.md | 2 +- .agents/rules/python-testing.md | 34 ++++++- tests/AGENTS.md | 6 +- 6 files changed, 213 insertions(+), 6 deletions(-) create mode 100644 .agents/rules/component-design.md create mode 100644 .agents/rules/python-module-layout.md diff --git a/.agents/rules/code-comments.md b/.agents/rules/code-comments.md index f1740c405..a46dbd1fb 100644 --- a/.agents/rules/code-comments.md +++ b/.agents/rules/code-comments.md @@ -5,11 +5,39 @@ paths: # Comment and reference rules +Applies to docstrings, comments, and any inline documentation in source files. + ## Comment sparingly - Prefer clear code over explanation. Add a comment only when it conveys intent the code cannot — a non-obvious tradeoff, gotcha, or "why". - Keep comments as short as possible — one line where you can. - Do not restate what the code plainly does. +- Never narrate what a change is doing ("# fetch the user", "# loop over the results"). Reviewers repeatedly have to ask for these to be removed. + +## What good documentation looks like + +- Comment the *why*, never the *what*: a constraint, an invariant, a workaround, a deliberate deviation from the obvious approach. Never paraphrase the line below it or restate the type signature. +- If code needs a comment to explain *what* it does, rename or extract until it doesn't. A comment that restates the code is worse than none - noise that rots the moment the code changes. +- When a why-comment is warranted, one sentence. If the why needs a paragraph, it belongs in the function's docstring or a `dev/knowledge/` page, not inline. +- Public docstrings are different: `uv run invoke docs-generate` publishes them to the SDK reference docs, so they are user-facing API documentation. Document the contract - what it does, its arguments, what it returns, what it raises - in the google convention the `D`/`DOC` ruff rules enforce. Regenerate the docs after changing one. + +## No references to other code + +Do not name other classes, functions, methods, callers, or call sites in docstrings or comments. Examples of what to avoid: + +- "Used by `InfrahubClient` to ..." +- "Called from `execute_graphql` after authentication" +- "See also `NodeParser.transform`" +- "Mirrors the behavior of the sync branch manager" + +Why: code is renamed, moved, and deleted. These references rot silently and mislead readers. Well-named identifiers and grep make the relationships discoverable without the comment. + +Acceptable exceptions: + +- A published docstring may cross-reference another *public* SDK symbol, since that is part of the reference documentation a user reads. Name the public API, never an internal caller. +- Stable public contracts (a protocol or interface that other implementations must satisfy) - name the protocol, not its callers. +- A workaround that depends on a specific upstream library symbol - name the library function and version constraint. +- The async/sync counterpart of the symbol being documented, since the pair is a documented contract. ## Do not reference ephemeral artifacts diff --git a/.agents/rules/component-design.md b/.agents/rules/component-design.md new file mode 100644 index 000000000..6bb9f84a5 --- /dev/null +++ b/.agents/rules/component-design.md @@ -0,0 +1,123 @@ +--- +paths: + - "infrahub_sdk/**/*.py" +--- + +# Component design (SOLID / DI) + +Applies when creating a new component or making significant changes to an existing one. Does not apply to small bug fixes, single-function tweaks, or changes confined to existing code paths. When in doubt for anything that introduces a new class or reshapes responsibilities, follow this rule. + +## Use modular components with dependency injection + +New logic should live in components that receive their collaborators through constructor injection rather than instantiating them internally. This keeps components composable, swappable, and testable without patching. + +## Required dependencies, not optional + +Constructor dependencies for new code are required parameters - not `collaborator: Collaborator | None = None` with an internal default. Optional injection hides that the dependency exists and lets a caller silently skip wiring it. Make every collaborator an explicit, required constructor argument - explicit is better than implicit. + +The single exception is editing existing code where adding a required parameter would force a large change across many call sites. There, an optional parameter is a transitional compromise to keep the change small - not the target shape for new components. + +Late registration is the same anti-pattern in another shape. `set_collaborator(x)`, `register_handler(fn)`, or assigning `obj.on_change = fn` after construction hides the dependency at construction, lets a caller skip wiring it, lets a second caller silently clobber the first's, and forces a `None` check at every use site. Pass it to `__init__`. When the component feeds zero or more collaborators rather than exactly one, that argument is a required `list[...]`, and callers with nothing to wire pass `[]` explicitly. + +## Build components near the entry point + +Construct components as close to the entry point as possible. In this repository the entry points are the client constructors (`InfrahubClient` / `InfrahubClientSync`) and the `infrahubctl` command functions - those are the composition roots. `infrahub_sdk/ctl/exporter.py` is the worked example: the command resolves the client and the console, builds the exporter with them, and delegates - the exporter never reaches for a client of its own. + +Use a builder class or factory function when wiring is non-trivial, and inject each sub-component rather than constructing it inside a parent component's `__init__`. + +Anything that comes from outside the component's own domain - resolved configuration, a transport, a console, a logger - is resolved at that entry point, never inside the component: + +- **Configuration resolves at the entry point, not in the component.** A component takes plain values (`max_retries: int`, `backoff_base: float`), never a `Config` object and never a module-global read. `RateLimitRetryHandler` is the example to copy: the client reads `self.config.rate_limit_*` once and passes plain numbers down, so the handler is the only thing that has to be understood to test the retry decisions, and it is testable with hand-picked values. +- **A factory takes its out-of-domain collaborators as parameters too**, rather than choosing them. A factory that both reads config *and* picks the concrete implementations has only moved the coupling one level out; take them as arguments so the entry point names them and the factory stays reusable with different ones. +- **Configure at construction, never by assignment afterwards.** Reaching into a built object to finish setting it up leaves a window in which it is misconfigured, makes a fixed value look mutable, and scatters the wiring across two places. Pass it to `__init__`, and expose it through a read-only property if callers need to read it back. +- **Avoid mutable module-level registries.** A dict at module scope that other modules write into makes behaviour depend on which imports have run and leaks between tests in the same worker. Prefer passing the mapping into the factory, so the entry point names what is registered. + +## Single entry point, operating on arguments + +A component should generally expose a single public entry point method (occasionally more, when justified by cohesive responsibility). That method only accepts the entities being operated on as arguments - it should not require additional dependencies to be passed in alongside the work payload. `LineDelimitedJSONExporter.export(...)` is the shape: the client arrives in the constructor, the export directory, namespaces and branch arrive per call. + +## Constructor vs. method arguments + +- The client (or requester) is always injected to the constructor. +- `branch` is usually injected to the constructor, but not always - inject it when the component's lifetime is tied to a single branch; pass it per-call when the component is reused across branches. +- Entities being examined or updated (nodes, schemas, spec payloads, file contents, request parameters) are passed to the entry method, not stored on the instance. + +The boundary is: long-lived collaborators go in the constructor; transient work items go in the method. + +## Single Responsibility Principle + +Each component should have one reason to change. If a class is doing two unrelated things, split it. Prefer composition of small components over large multi-purpose ones. + +## Keep decision logic out of the async/sync split + +Every public feature ships in both an async and a sync variant, so any logic written inside the two variants is written twice and drifts. Put the decision logic in a plain component with no I/O, have both variants call it, and duplicate only the awaiting. + +`RateLimitRetryHandler` splits exactly this way: `parse_retry_after`, `compute_backoff` and `should_retry` are pure and shared, and only `send` / `asend` exist twice, because only they perform I/O. The pure half is then covered once by tests that need no transport at all. + +The corollary is a design test: if a rule can only be exercised through an awaited call, it is probably sitting on the wrong side of that line. + +## Interfaces for multiple implementations + +When more than one implementation of a component is required (different formats, different backends, a no-op variant), define a `Protocol` or abstract base class. The correct implementation is selected at the wiring layer and injected to the constructor - the consumer codes against the interface, not a concrete class. `ExporterInterface` / `ImporterInterface` (`infrahub_sdk/transfer/`) and `DataProcessor` (`infrahub_sdk/spec/processors/`) are the existing examples. + +A single implementation does not need an interface yet; introduce one when the second implementation arrives. Note that the second implementation can be either a no-op version or a testing version of a component. + +## Interfaces to keep an out-of-domain dependency out + +The other reason to declare a `Protocol` is to invert a dependency direction, and there **one implementation is enough**. The situation: a component's logic has no business knowing about some out-of-domain concern - logging, a recorder, a progress display, telemetry - but something has to feed that concern from inside the component's flow. Importing the concrete client directly is what you are avoiding: it makes the dependency viral, drags a third-party package into the import chain of pure logic, and means the component can no longer be constructed in a test without it. + +`Recorder` (`infrahub_sdk/recorder.py`) and `InfrahubLogger` (`infrahub_sdk/types.py`) are this pattern already: a `Protocol` the SDK owns, satisfied structurally by whatever the caller supplies. + +There are two acceptable shapes for the interface itself. Both keep the adapter and the logic from importing each other; pick one per interface and be consistent within it. + +1. **Implicit - a `Protocol` declared beside the consumer, which the adapter never imports.** Structural typing is what makes this work: the adapter satisfies the protocol by having matching signatures, so nothing in the adapter's module points back at the consumer's. This is the lower-friction option: one new class, no new module, and no coordination with the adapter. +2. **Explicit - an interface in a module of its own that both sides import.** Put the `Protocol` (or an ABC, if you want subclassing enforced) in a small, dependency-free interface module; the consumer imports it to type its constructor parameter, and the adapter imports it to declare that it implements it. Neither side imports the other, so the dependency still points inward at the interface, but the contract is now named at both ends: the adapter states what it implements, the type checker verifies it at the definition rather than only at the wiring call, and a reader of the adapter can find the interface without knowing which component motivated it. Explicit is better than implicit - prefer this one whenever the interface is worth naming as a contract, which is the case as soon as it has more than one implementer or more than one consumer. `infrahub_sdk/transfer/exporter/interface.py` is this shape. + +An ABC only works in shape 2 - a subclass must import whatever module the base lives in, so an ABC declared in the consumer's module drags the dependency backwards. Never do that; if you want an ABC, give it its own module. + +Whichever shape you pick, the remaining two parts do not change: + +- **Name the methods in the depending component's vocabulary**, not the adapter's, and pass the values as arguments rather than handing over `self`, so the adapter can never read back into the component. The component then depends on a shape it defined, has no idea what is on the other side, and stays free to change its internals. +- **Put the concrete adapter in a separate, purpose-named module** that is the only place importing the library, and **let only the wiring layer import both** (see "Build components near the entry point"). + +The acceptance test is an import-graph one: after this, the library is reachable from the entry point and from the adapter module, and from nowhere in the logic. Verify it by grepping for the package name - if it appears anywhere under the component's own package, the split is incomplete. + +This is the deliberate exception to "a single implementation does not need an interface yet" above. The interface earns its place by fixing which way the dependency points, not by abstracting over variants - and in practice the test doubles become the second and third implementations anyway. + +## Dispatching across implementations + +When a component must pick one of several implementations at runtime based on the input, do not branch with `isinstance` (or a `match` on the input's type) inside one class. Give each implementation a predicate on the shared interface (e.g. `supports(request) -> bool`) alongside its entry method, hold the implementations as an injected list in an aggregator component, and let the aggregator delegate to the first that supports the input: + +```python +class CheckerInterface(ABC): + @abstractmethod + def supports(self, request: Request) -> bool: ... + + @abstractmethod + def check(self, request: Request) -> Result: ... + + +class AggregatedChecker: + def __init__(self, checkers: list[CheckerInterface]) -> None: + self.checkers = checkers + + def run(self, request: Request) -> Result: + for checker in self.checkers: + if checker.supports(request): + return checker.check(request) + raise NoCheckerError(request) +``` + +The aggregator depends only on the interface; the concrete list is assembled by the factory at the wiring layer, so adding an implementation is one new class plus one line in the factory, with no edit to the dispatch logic. + +This is for an open, extensible set of implementations. When the set is closed and fixed (an enum, a sealed union), an exhaustive `match` with `typing.assert_never` is the right tool instead. + +## Why this design matters + +Stepping back from the individual rules above: constructor-injected long-lived dependencies plus method-passed transient entities is the boundary that lets components be reused across calls and substituted with real implementations instead of `unittest.mock`. The [testing rules](./python-testing.md) forbid `unittest.mock` - that prohibition is only practical when production code follows this design. + +Use this as a design driver, not just a constraint: the no-mock rule is the forcing function for this structure. When you make a component's decision logic testable without patching - collaborators injected through the constructor, a single entry point that is pure and operates only on its arguments - dependency inversion and single responsibility fall out as the path of least resistance rather than discipline you have to summon. The corollary is a useful smell test: if a component is hard to test without a mock, that is the signal it needs splitting or its dependencies injected, not that it needs a mock. + +## Existing code + +If existing nearby code violates this pattern, do not refactor it as part of an unrelated change. Raise it as a separate discussion - drive-by refactors balloon scope and make reviews harder. diff --git a/.agents/rules/python-module-layout.md b/.agents/rules/python-module-layout.md new file mode 100644 index 000000000..e013010cd --- /dev/null +++ b/.agents/rules/python-module-layout.md @@ -0,0 +1,26 @@ +--- +paths: + - "infrahub_sdk/**/*.py" +--- + +# Python module layout + +Applies when adding code to existing modules or deciding where new code lives. + +## constants.py holds constants only + +Do not put functions or classes in a file named `constants.py` - only module-level constant values (plain literals, enums, frozen containers). A value that must be computed, read from the environment, or resolved at runtime is not a constant; give it a home in a purpose-named module (for example `limits.py`, `config.py`) instead. + +Why: readers grep and import from `constants.py` expecting inert values with no behavior and no import-time or call-time side effects. A function hiding there muddies that contract and gets overlooked when reasoning about runtime behavior. + +If the value genuinely never changes at runtime, prefer an actual constant over a function returning one. + +## Imports at the top + +Keep imports at the top of the module. Do not import inside functions, methods, or classes. Ruff enforces this (`PLC0415`). + +A function-local import is acceptable only to break a genuine circular import or to defer an optional or heavy dependency that must not load on every import. Mark each such import with `# noqa: PLC0415` and a short reason. + +## Keep the async and sync variants side by side + +A feature that ships in both variants keeps them in the same module, named as a pair (`InfrahubClient` / `InfrahubClientSync`, `send` / `asend`). Do not split the sync variant into a separate module: the pair has to be read together to stay in step, and a reader looking for one always wants to see the other. diff --git a/.agents/rules/python-testing-unit.md b/.agents/rules/python-testing-unit.md index 6a27ca66a..e02904c2e 100644 --- a/.agents/rules/python-testing-unit.md +++ b/.agents/rules/python-testing-unit.md @@ -43,7 +43,7 @@ async def test_branch_list(clients: BothClients, client_type: str, mock_branch_l assert list(branches.keys()) == ["main", "branch01"] ``` -Assert the actual expected value. Assertions like `assert result is not None` or `assert result` do not verify behaviour — they only confirm something was returned. +Assert the actual expected value for each variant, per [Assert exact expectations](./python-testing.md). ## Test file layout diff --git a/.agents/rules/python-testing.md b/.agents/rules/python-testing.md index ba97f5475..b04fe9a3a 100644 --- a/.agents/rules/python-testing.md +++ b/.agents/rules/python-testing.md @@ -12,6 +12,10 @@ Do not use `unittest.mock`, `MagicMock`, or `patch`. The only sanctioned mocking - `httpx_mock` (pytest-httpx) — for intercepting HTTP calls at the transport layer - `monkeypatch` — for patching stdlib functions (for example: `ssl.create_default_context`) +Everything else is substituted by injecting a real implementation of the collaborator's interface, which [the component design rules](./component-design.md) exist to make possible. Two doubles are worth writing for an injected collaborator: a `Recording*` one that keeps the calls in order (assert the exact sequence and values, not "was called"), and - where the code claims to survive that collaborator failing - a `Failing*` one that raises, to prove the claim. + +Time is handled the same way: a component that needs the current time takes it as a parameter (`now: datetime | None = None`) so tests pass a fixed value, rather than patching the clock. + ## Async tests `asyncio_mode = "auto"` is configured globally. Do not add `@pytest.mark.asyncio`. Do not add loop scope markers manually — this is handled in `conftest.py`. @@ -53,10 +57,36 @@ async def test_branch_conflict(case: BranchCase) -> None: Always pass `match=` to `pytest.raises()`: ```python -with pytest.raises(NodeNotFoundError, match="Could not find node with id"): - await client.get(kind="NetworkDevice", id="missing") +with pytest.raises(Error, match=r"^Cannot use an unsaved node as the graph traversal source; save it first\.$"): + await client.traverse_paths(source=unsaved_node, destination=saved_node) ``` +Make `match` cover the whole stable message, anchored with `^...$` where practical, rather than a short fragment. A fragment keeps passing even when the rest of the wording regresses. Match only a substring when the message has a genuinely variable part (an id, a path, a count) that cannot be pinned down. + +## Assert exact expectations + +Assert the exact value, the exact collection (full list, set or dict equality, not `in` or `issubset`), and the exact message. Never assert mere existence or non-emptiness - `assert result is not None`, `assert result` and `len(result) > 0` all pass while the behaviour under test is broken. Where a count matters, assert the number, so a run that silently measures zero fails. + +Pin literal expected values. Never compute the expectation with the same serializer, query builder, or library call the implementation uses - that only asserts the code agrees with itself. + +A test for a rejected operation must also read the target back and assert nothing changed. + +## Don't test the framework + +Skip tests that only exercise library behaviour: plain `Enum` value or round-trip checks, pydantic field constraints (`ge`, `min_length`, ...), `SettingsConfigDict` env plumbing, or "the model has field X". Rule of thumb: if the test would still pass after deleting our implementation and reinstalling the library, it belongs to the library. + +Testing that *our* config maps a specific env var onto a specific field, or that a validator we wrote rejects a value, is ours and is worth a test. Testing that pydantic enforces `ge=0` is not. + +## Pick the cheapest test tier + +If the logic needs only in-memory inputs (a schema object, a dataclass, a pure function), write a unit test with no `httpx_mock` and no client at all - don't reach for a mocked client because a neighbouring test uses one. Reserve `tests/integration/` for behaviour that genuinely depends on a running Infrahub; starting a testcontainer to exercise a pure function costs minutes on every CI run. + +## Don't leak process-global state + +Tests share one interpreter, and a run may be distributed across xdist workers (`-n`, `--dist loadscope`), so anything left behind changes the outcome of whichever test runs next. Touch `logging` levels, handlers or filters, module-level registries and singletons, `sys.modules`, the working directory, or environment variables only through `monkeypatch`, which restores for you, or a save/restore fixture (change it, `yield`, restore it). `clean_env_vars` in `tests/conftest.py` is the shape to copy. + +Install only the piece under test, and remove it after the `yield`. Never call an application-wide initialisation routine from a test body: it owns the whole process, undoes nothing, and reconfigures every later test in that worker. + ## Fixtures and helpers - Shared fixtures live in the nearest `conftest.py` to the tests that use them. diff --git a/tests/AGENTS.md b/tests/AGENTS.md index cce67364c..334be2011 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -34,12 +34,12 @@ tests/ # Async test - NO decorator needed (auto mode) async def test_async_operation(httpx_mock: HTTPXMock): httpx_mock.add_response( - url="http://localhost:8000/api/graphql", + url="http://mock/graphql/main", json={"data": {"result": "success"}}, ) client = InfrahubClient() - result = await client.execute(query="...") - assert result is not None + result = await client.execute_graphql(query="...") + assert result == {"result": "success"} # Sync test def test_sync_operation(): From 3aea1840d7dc6a3651f575427977f5c62ab55184 Mon Sep 17 00:00:00 2001 From: Patrick Ogenstad Date: Wed, 26 Aug 2026 09:56:24 +0200 Subject: [PATCH 2/3] docs(agents): assert the narrowest exception class in the raises example --- .agents/rules/python-testing.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.agents/rules/python-testing.md b/.agents/rules/python-testing.md index b04fe9a3a..e72ecd20b 100644 --- a/.agents/rules/python-testing.md +++ b/.agents/rules/python-testing.md @@ -57,10 +57,12 @@ async def test_branch_conflict(case: BranchCase) -> None: Always pass `match=` to `pytest.raises()`: ```python -with pytest.raises(Error, match=r"^Cannot use an unsaved node as the graph traversal source; save it first\.$"): - await client.traverse_paths(source=unsaved_node, destination=saved_node) +with pytest.raises(BranchNotFoundError, match=r"^Unable to find the branch 'missing' in the Database\.$"): + await client.branch.get(branch_name="missing") ``` +Name the narrowest exception the call can raise, never the base `Error` (or `Exception`). A broad class passes when the code fails for an entirely unrelated reason, which is the failure mode the test exists to catch. Ruff's `PT011` is disabled under `tests/`, so nothing enforces this but review. + Make `match` cover the whole stable message, anchored with `^...$` where practical, rather than a short fragment. A fragment keeps passing even when the rest of the wording regresses. Match only a substring when the message has a genuinely variable part (an id, a path, a count) that cannot be pinned down. ## Assert exact expectations From 92d06873335c72b9e82d494ed7de1b2b73b851fd Mon Sep 17 00:00:00 2001 From: Patrick Ogenstad Date: Wed, 26 Aug 2026 12:13:58 +0200 Subject: [PATCH 3/3] docs(agents): correct four self-contradictions found in review - code-comments.md: two of the "avoid" examples were themselves listed as acceptable exceptions a few lines later (the async/sync counterpart and a public cross-reference). Reframed the rule as incidental vs. contractual references, and replaced the overlapping examples with a caller, a call site, a private helper and an incidental neighbour. - component-design.md: ExporterInterface, ImporterInterface and DataProcessor were cited as "interfaces for multiple implementations", but each has exactly one implementer, contradicting the following paragraph. Replaced with the two interfaces that genuinely carry a second implementation - Recorder (NoRecorder / JSONRecorder) and AsyncRequester / SyncRequester (httpx path / JSONPlayback) - and redirected the transfer interfaces to the dependency-inversion heading, which is what they actually demonstrate. - component-design.md: name PROCESSOR_PER_KIND as the module-global registry to avoid, so citing its package elsewhere cannot read as an endorsement of it. - tests/AGENTS.md: the mocked URL could never match, since a bare InfrahubClient() targets the default http://localhost:8000. Pass Config(address="http://mock"), as the repo's own fixtures do. --- .agents/rules/code-comments.md | 22 +++++++++++----------- .agents/rules/component-design.md | 13 ++++++++++--- tests/AGENTS.md | 3 ++- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/.agents/rules/code-comments.md b/.agents/rules/code-comments.md index a46dbd1fb..e58cbc17a 100644 --- a/.agents/rules/code-comments.md +++ b/.agents/rules/code-comments.md @@ -21,23 +21,23 @@ Applies to docstrings, comments, and any inline documentation in source files. - When a why-comment is warranted, one sentence. If the why needs a paragraph, it belongs in the function's docstring or a `dev/knowledge/` page, not inline. - Public docstrings are different: `uv run invoke docs-generate` publishes them to the SDK reference docs, so they are user-facing API documentation. Document the contract - what it does, its arguments, what it returns, what it raises - in the google convention the `D`/`DOC` ruff rules enforce. Regenerate the docs after changing one. -## No references to other code +## No incidental references to other code -Do not name other classes, functions, methods, callers, or call sites in docstrings or comments. Examples of what to avoid: +Do not point at code that merely happens to be related: who calls this, what runs before or after it, which internal helper it resembles. Examples of what to avoid: -- "Used by `InfrahubClient` to ..." -- "Called from `execute_graphql` after authentication" -- "See also `NodeParser.transform`" -- "Mirrors the behavior of the sync branch manager" +- "Used by `InfrahubClient` to ..." - a caller +- "Called from `execute_graphql` after authentication" - a call site +- "See also `_resolve_node_id`" - an internal helper +- "Kept in step with the object-spec loader" - an incidental neighbour Why: code is renamed, moved, and deleted. These references rot silently and mislead readers. Well-named identifiers and grep make the relationships discoverable without the comment. -Acceptable exceptions: +What stays is the opposite case: a reference that is part of a contract someone else depends on. None of these are incidental, so name them freely. -- A published docstring may cross-reference another *public* SDK symbol, since that is part of the reference documentation a user reads. Name the public API, never an internal caller. -- Stable public contracts (a protocol or interface that other implementations must satisfy) - name the protocol, not its callers. -- A workaround that depends on a specific upstream library symbol - name the library function and version constraint. -- The async/sync counterpart of the symbol being documented, since the pair is a documented contract. +- Another *public* SDK symbol, named in a published docstring. That docstring is reference documentation, and pointing a user at the related public API is its job. +- A protocol or interface that implementations must satisfy - name the protocol, not its implementers or callers. +- The async or sync counterpart of the symbol being documented, since the pair is itself a documented contract. +- An upstream library symbol a workaround depends on - name the library function and the version constraint that makes it necessary. ## Do not reference ephemeral artifacts diff --git a/.agents/rules/component-design.md b/.agents/rules/component-design.md index 6bb9f84a5..4f22ac335 100644 --- a/.agents/rules/component-design.md +++ b/.agents/rules/component-design.md @@ -30,7 +30,7 @@ Anything that comes from outside the component's own domain - resolved configura - **Configuration resolves at the entry point, not in the component.** A component takes plain values (`max_retries: int`, `backoff_base: float`), never a `Config` object and never a module-global read. `RateLimitRetryHandler` is the example to copy: the client reads `self.config.rate_limit_*` once and passes plain numbers down, so the handler is the only thing that has to be understood to test the retry decisions, and it is testable with hand-picked values. - **A factory takes its out-of-domain collaborators as parameters too**, rather than choosing them. A factory that both reads config *and* picks the concrete implementations has only moved the coupling one level out; take them as arguments so the entry point names them and the factory stays reusable with different ones. - **Configure at construction, never by assignment afterwards.** Reaching into a built object to finish setting it up leaves a window in which it is misconfigured, makes a fixed value look mutable, and scatters the wiring across two places. Pass it to `__init__`, and expose it through a read-only property if callers need to read it back. -- **Avoid mutable module-level registries.** A dict at module scope that other modules write into makes behaviour depend on which imports have run and leaks between tests in the same worker. Prefer passing the mapping into the factory, so the entry point names what is registered. +- **Avoid mutable module-level registries.** A dict at module scope that other modules write into makes behaviour depend on which imports have run and leaks between tests in the same worker. Prefer passing the mapping into the factory, so the entry point names what is registered. `PROCESSOR_PER_KIND` in `infrahub_sdk/spec/processors/factory.py` is the existing shape to avoid, not to copy. ## Single entry point, operating on arguments @@ -58,15 +58,22 @@ The corollary is a design test: if a rule can only be exercised through an await ## Interfaces for multiple implementations -When more than one implementation of a component is required (different formats, different backends, a no-op variant), define a `Protocol` or abstract base class. The correct implementation is selected at the wiring layer and injected to the constructor - the consumer codes against the interface, not a concrete class. `ExporterInterface` / `ImporterInterface` (`infrahub_sdk/transfer/`) and `DataProcessor` (`infrahub_sdk/spec/processors/`) are the existing examples. +When more than one implementation of a component is required (different formats, different backends, a no-op variant), define a `Protocol` or abstract base class. The correct implementation is selected at the wiring layer and injected to the constructor - the consumer codes against the interface, not a concrete class. + +Two examples in this codebase carry a genuine second implementation, both selected in `Config`: + +- `Recorder` (`infrahub_sdk/recorder.py`), with the no-op `NoRecorder` and the real `JSONRecorder`. +- `AsyncRequester` / `SyncRequester` (`infrahub_sdk/types.py`), with the client's own httpx path and `JSONPlayback` (`infrahub_sdk/playback.py`) replaying recorded responses. A single implementation does not need an interface yet; introduce one when the second implementation arrives. Note that the second implementation can be either a no-op version or a testing version of a component. +`ExporterInterface` / `ImporterInterface` (`infrahub_sdk/transfer/`) and `DataProcessor` (`infrahub_sdk/spec/processors/`) each have exactly one implementer today, so they are not examples of this reason to declare an interface. They earn their place under the next heading instead: they keep `ujson` and the file layout out of the `ctl` command that drives them. + ## Interfaces to keep an out-of-domain dependency out The other reason to declare a `Protocol` is to invert a dependency direction, and there **one implementation is enough**. The situation: a component's logic has no business knowing about some out-of-domain concern - logging, a recorder, a progress display, telemetry - but something has to feed that concern from inside the component's flow. Importing the concrete client directly is what you are avoiding: it makes the dependency viral, drags a third-party package into the import chain of pure logic, and means the component can no longer be constructed in a test without it. -`Recorder` (`infrahub_sdk/recorder.py`) and `InfrahubLogger` (`infrahub_sdk/types.py`) are this pattern already: a `Protocol` the SDK owns, satisfied structurally by whatever the caller supplies. +`InfrahubLogger` (`infrahub_sdk/types.py`) is this pattern already: a `Protocol` the SDK owns, satisfied structurally by whatever the caller supplies, so no logging library reaches the client's own logic. `Recorder` does both jobs at once - it inverts the file-writing dependency *and* has a second implementation - which is the common case once an interface has been in place for a while. There are two acceptable shapes for the interface itself. Both keep the adapter and the logic from importing each other; pick one per interface and be consistent within it. diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 334be2011..730257762 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -37,7 +37,8 @@ async def test_async_operation(httpx_mock: HTTPXMock): url="http://mock/graphql/main", json={"data": {"result": "success"}}, ) - client = InfrahubClient() + # The mocked URL must match the client's configured address + client = InfrahubClient(config=Config(address="http://mock")) result = await client.execute_graphql(query="...") assert result == {"result": "success"}