docs(agents): port SOLID and code-standard rules from Infrahub - #1280
docs(agents): port SOLID and code-standard rules from Infrahub#1280ogenstad wants to merge 3 commits into
Conversation
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.
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Deploying infrahub-sdk-python with
|
| Latest commit: |
92d0687
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://084717f1.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-agent-rules-solid-standa.infrahub-sdk-python.pages.dev |
- 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.
There was a problem hiding this comment.
1 issue found across 3 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=".agents/rules/component-design.md">
<violation number="1" location=".agents/rules/component-design.md:70">
P3: The claim that DataProcessor 'keeps ujson and the file layout out of the ctl command that drives them' is inaccurate for DataProcessor: it is driven by infrahub_sdk/spec/object.py via DataProcessorFactory.process_data, not by ctl, and involves neither ujson nor file layout. Scope that justification to ExporterInterface/ImporterInterface, or give DataProcessor its own reason (its processors are injected/selected in DataProcessorFactory rather than hard-coded into spec/object.py).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| 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. |
There was a problem hiding this comment.
P3: The claim that DataProcessor 'keeps ujson and the file layout out of the ctl command that drives them' is inaccurate for DataProcessor: it is driven by infrahub_sdk/spec/object.py via DataProcessorFactory.process_data, not by ctl, and involves neither ujson nor file layout. Scope that justification to ExporterInterface/ImporterInterface, or give DataProcessor its own reason (its processors are injected/selected in DataProcessorFactory rather than hard-coded into spec/object.py).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/rules/component-design.md, line 70:
<comment>The claim that DataProcessor 'keeps ujson and the file layout out of the ctl command that drives them' is inaccurate for DataProcessor: it is driven by infrahub_sdk/spec/object.py via DataProcessorFactory.process_data, not by ctl, and involves neither ujson nor file layout. Scope that justification to ExporterInterface/ImporterInterface, or give DataProcessor its own reason (its processors are injected/selected in DataProcessorFactory rather than hard-coded into spec/object.py).</comment>
<file context>
@@ -58,15 +58,22 @@ The corollary is a design test: if a rule can only be exercised through an await
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
</file context>
| `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. | |
| `ExporterInterface` / `ImporterInterface` (`infrahub_sdk/transfer/`) 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 by keeping `ujson` and the file layout out of the `ctl` command that drives them, and the transfer modules stay the only place that touches the filesystem. |
Why
The Infrahub repository carries four agent rules under
.agents/rules. This repository carried only two of the four, and both were narrower than their Infrahub counterparts, so agents working in the SDK get materially less guidance on component design and code standards than agents working in the backend.Goal: the same overall structure on both sides, written for the SDK rather than copied from Infrahub.
Non-goals: no code changes, no refactors to make existing code conform. Where existing code diverges from a new rule (an optional
consoleon the JSON exporter, the module-levelPROCESSOR_PER_KINDregistry), it is left alone - the rules say explicitly not to drive-by refactor.What changed
Two new rules:
component-design.md- the SOLID/DI counterpart to Infrahub'sbackend-component-design.md, scoped toinfrahub_sdk/**/*.py. Constructor injection, required rather than optional dependencies, building near the entry point, single entry point operating on arguments, SRP, interfaces for multiple implementations, interfaces to invert an out-of-domain dependency, runtime dispatch. Every example is drawn from this codebase:RateLimitRetryHandlerfor configuration resolving at the entry point,ExporterInterface/ImporterInterfaceandDataProcessorfor interfaces,RecorderandInfrahubLoggerfor dependency inversion,ctl/exporter.pyas the composition root.python-module-layout.md-constants.pyholds constants only, imports stay at the top (PLC0415, which ruff already enforces everywhere buttasks.py), and the async/sync pair stays in one module.Two extended rules:
code-comments.md- adds "no references to other code" and a "what good documentation looks like" section. The SDK-specific carve-out:docs-generatepublishes public docstrings to the reference docs, so those legitimately cross-reference other public API and get the full google-convention contract treatment, while inline comments stay minimal.python-testing.md- adds exact-expectation assertions, don't-test-the-framework, cheapest-test-tier, no-process-global-leakage,Recording*/Failing*doubles in place of mocks, and full-message anchoredmatch=onpytest.raises. The duplicated assertion sentence inpython-testing-unit.mdnow points at the shared rule.Deliberately not ported
Infrahub content with no SDK analogue:
db/branchconstructor conventions tied to Neo4j, the Repository/Query persistence pattern, Prefect flows as entry points,StandardNodelegacy notes,freezegun(not a dependency here - the SDK'snow=parameter convention is documented instead), and GraphQL-response error assertions.Also
tests/AGENTS.md's async example calledclient.execute(...), which does not exist, and assertedresult is not None, which is the exact non-assertion the new testing rule forbids. Corrected toexecute_graphqlwith a real expected value, so the example and the rules agree.How to review
Read
component-design.mdfirst - it is the bulk of the diff and the one place where a claim about the codebase could be wrong. Each named example was checked against the source; the async/sync section is the one part with no Infrahub counterpart.How to test
uv run rumdl check .Passes on all 131 files. Documentation only, so there is nothing else to exercise.
Impact & rollout
Checklist
uv run towncrier create ...)