docs: harvest review lessons from PRs reviewed 2026-08-21 to 2026-08-28

.agents/rules/backend-component-design.md

@@ -10,7 +10,7 @@ Applies when creating a new backend component or making significant changes to a
 
 ## Use modular components with dependency injection
 
-New logic lives in components that receive their collaborators through constructor injection rather than instantiating them internally, which keeps them composable, swappable, and testable without patching. Every collaborator is a **required** parameter — not `collaborator: Collaborator | None = None` with an internal default, which hides that the dependency exists and lets a caller silently skip wiring it.
+New logic lives in components that receive their collaborators through constructor injection rather than instantiating them internally, which keeps them composable, swappable, and testable without patching. A dataclass is data — inputs and outputs of functions; the moment it needs a collaborator to do work, it is a component: make it a plain class with the collaborator injected at construction. Every collaborator is a **required** parameter — not `collaborator: Collaborator | None = None` with an internal default, which hides that the dependency exists and lets a caller silently skip wiring it.
 
 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.
 

.agents/rules/testing-python.md

@@ -69,7 +69,11 @@ Never add a marker, attribute, or `type: ignore` to production code so a test ca
 
 ## Don't leak process-global state
 
-Every test in an xdist worker shares one interpreter. Change `logging` levels/handlers/filters, `structlog` config, module-level registries/singletons, `sys.path`/`sys.modules` or env vars only through a save/restore fixture (change it, `yield`, restore it), or `monkeypatch` where it applies. Never call an application startup routine such as `infrahub.log.configure_logging` from a test — it owns the whole process and undoes nothing, so it reconfigures every later test in the worker. Install only the piece under test and remove it after the `yield`. See `dev/guidelines/backend/testing.md` §"Leave process-global state as you found it".
+Every test in an xdist worker shares one interpreter. Change `logging` levels/handlers/filters, `structlog` config, module-level registries/singletons, class attributes (your own or a third-party library's), `sys.path`/`sys.modules` or env vars only through a save/restore fixture (change it, `yield`, restore it), or `monkeypatch` where it applies. Never call an application startup routine such as `infrahub.log.configure_logging` from a test — it owns the whole process and undoes nothing, so it reconfigures every later test in the worker. Install only the piece under test and remove it after the `yield`. See `dev/guidelines/backend/testing.md` §"Leave process-global state as you found it".
+
+## A regression guard must be shown to bite
+
+Before trusting a test that pins a fix or an optimization, run it against the code without the change (revert it, or reintroduce the old call) and watch it fail — a guard that passes on both sides asserts nothing, and several have. State the check in the PR ("fails with X when the fix is reverted"). A `strict=True` xfail swallows every assertion in its body, so it holds only the expected failure; invariants that must hold today go in a passing test.
 
 ## Test file placement
 

AGENTS.md

@@ -167,11 +167,11 @@ CI validates that all generated files are committed — the `validate-generated-
 - Before diagnosing _or_ modifying code in any domain, read the relevant docs in `dev/knowledge/` for that domain. The architectural intent (which layer owns a concern) is often the answer to the bug — don't reason from code alone
 - Run formatters before committing (`uv run invoke format`, `pnpm biome:fix`)
 - Write tests for new functionality
-- Add a towncrier changelog fragment for any user-visible change, UI styling included (use the `creating-changelog-entries` skill); internal maintenance still gets a `housekeeping` fragment, and only a refactor with no user-visible or maintenance impact needs none
+- Add a towncrier changelog fragment for any user-visible change, UI styling included (use the `creating-changelog-entries` skill). `housekeeping` is not a catch-all: internal maintenance gets a fragment only when a user could still notice the change (the skill draws the boundary on user visibility) — agent-doc, CI-config, and test-only tweaks are the typical cases a user never notices, so they get none
 - Use type hints for Python (backend) and TypeScript types (frontend)
 - In `tasks/*.py`, use the shared helpers for project-scoped Docker Compose operations rather than hard-coding `docker compose` or service names: build the command with `get_compose_cmd` (it selects the required `--profile`/`--ansi never` options) plus `get_env_vars`, run it through `execute_command` (which handles `sudo`), and reference named services via the shared constants (e.g. `SERVICE_WORKER_NAME`). Literal `docker compose` is acceptable only for genuinely global, project-agnostic discovery commands.
 - Before pushing, run `/pre-ci` (`.agents/commands/pre-ci.md`) — it runs the locally-executable CI checks, including generated-file and generated-doc validation (`docs.validate`); CI fails if any generated file is stale
-- Before writing a changelog fragment, PR description, or ADR that names a specific identifier, metric, or config default, grep the actual diff/code for it — state what landed, not what the plan intended. When a later fix changes a figure a spec-kit doc set already stated, grep the whole `dev/specs/<feature>/` directory for the old value and update every file that repeats it in the same commit
+- Before writing a changelog fragment, PR description, or ADR that names a specific identifier, metric, or config default, grep the actual diff/code for it — state what landed, not what the plan intended. When a later fix changes a figure — or reverses a decision — that a spec-kit doc set already stated, grep the whole `dev/specs/<feature>/` directory for the old value or decision and update every file that repeats it in the same commit
 
 ### Ask First
 

dev/guidelines/backend/python.md

@@ -336,6 +336,35 @@ origin: NodeMutationOrigin | None = None
 
 The annotation alone does not reject a bad value at runtime — a validation layer enforces it (a Pydantic model, or an explicit `NodeMutationOrigin(value)` conversion at the boundary for plain dataclasses/adapters). For a value exposed over GraphQL, reuse the existing Python-enum → GraphQL-enum conversion rather than re-declaring the values as strings in the GraphQL layer.
 
+The same applies on the read side: where an enum exists, branch on the member (`if rel.cardinality == RelationshipCardinality.MANY`), never on its string value — a comparison or query literal that hardcodes the value drifts silently when the enum changes.
+
+### A struct of mode flags is a union of dataclasses
+
+When a class carries several booleans of which most combinations are invalid (one flag excludes the others, two only make sense together), name the legal states instead: model each legal case as its own small dataclass and type the value as their union. Invalid combinations become unrepresentable, each case carries a name, and a `match` over the union replaces flag-order-sensitive `if` chains.
+
+```python
+# ❌ Bad - widen=True silently makes the other flags meaningless
+@dataclass
+class _Selection:
+    widen: bool = False
+    self_ids: bool = False
+    reader_lookup: bool = False
+
+# ✅ Good - each legal case is a type; invalid mixes cannot be built
+@dataclass(frozen=True)
+class Widen: ...
+
+@dataclass(frozen=True)
+class SelfTarget:
+    ids: list[str]
+
+@dataclass(frozen=True)
+class ReaderLookup:
+    reader_kind: str
+
+Selection = Widen | SelfTarget | ReaderLookup
+```
+
 ### Do not narrow a type in an override (Liskov / `ty`)
 
 An override may not make a parameter type *narrower* (or a return type *wider*) than the base declaration — `ty` rejects it as a Liskov violation. When an abstract method and its implementations must accept a union, declare the full shared type on the abstract **and** on every implementation; do not tighten one adapter.
@@ -355,6 +384,8 @@ async def set(self, key: str, value: str, expires: KVTTL | int | None = None) ->
 
 To branch on or read from a typed object, use `isinstance` so the type checker can narrow it; reaching for `getattr(obj, "attr", default)` defeats type analysis. When guarding a schema object, cover the whole family that carries the attribute — `isinstance(schema, (NodeSchema, ProfileSchema, TemplateSchema))` — since profiles and templates inherit node behavior and a `NodeSchema`-only check silently drops them.
 
+The same goes for named accessors: read a relationship manager with `node.get_relationship(name)`, not `getattr(node, name)` — the accessor is typed and greppable, and `getattr` hides the read from both.
+
 ### Don't write "one or many" unions — take the plural form and let callers wrap
 
 A parameter typed `T | Sequence[T]` forces runtime `isinstance` dispatch on every consumer, and when `T` includes `str` the dispatch is a trap: a bare string satisfies `Sequence[str]`, so it falls into the "many" branch and gets iterated character-by-character. Declare the plural form only — `list[str]` or `tuple[str]` — and have callers pass `[value]`.
@@ -398,31 +429,9 @@ if any(path == excluded or path.startswith(f"{excluded}/") for excluded in exclu
 
 The `python_testcontainers` package supports Python 3.10+, while the main backend requires Python 3.12+. When writing code that may be shared or used in `python_testcontainers`, be mindful of version-specific features.
 
-### datetime.UTC (Python 3.11+)
-
-The `datetime.UTC` constant was introduced in Python 3.11. For Python 3.10 compatibility, use `timezone.utc` instead:
-
-```python
-# ❌ Bad - Python 3.11+ only
-from datetime import UTC, datetime
-now = datetime.now(UTC)
-
-# ✅ Good - Works in Python 3.10+
-from datetime import datetime, timezone
-now = datetime.now(timezone.utc)
-```
-
-### Other Version-Specific Features
-
-When using newer Python features, verify they're available in the minimum supported version:
+### Version-specific features
 
-| Feature | Minimum Version |
-|---------|-----------------|
-| `datetime.UTC` | 3.11 |
-| `str \| None` union syntax | 3.10 |
-| `list[Type]` generic syntax | 3.9 |
-| `match` statements | 3.10 |
-| `Self` type hint | 3.11 (use `typing_extensions.Self` for 3.10) |
+The backend targets modern Python, but code shared with `python_testcontainers` must run on 3.10: there, avoid `datetime.UTC` (use `datetime.now(timezone.utc)`), and import `Self` from `typing_extensions`.
 
 ## Function Call Style
 

dev/guidelines/git-workflow.md

@@ -8,13 +8,19 @@ Git workflow and commit conventions for the project.
 
 - **Main branches:** `stable` (production), `develop` (development), `release-*` (releases)
 - **Feature branches:** Create from `develop`, merge back via PR
-- **Bug fixes:** target the oldest branch that needs the fix — `stable` when the bug is in released
-  code and the fix should ship in a patch release, `develop` when the code only exists there or the
-  fix can wait for the next minor
+- **Bug fixes, performance and test-reliability improvements:** target the oldest maintained branch
+  that should ship the change — `stable` when the affected code is released and the change can ship
+  in a patch release, `develop` when the code only exists there or the change can wait for the next
+  minor. A fix that changes observable behavior (which branch an event fires on, a value that is no
+  longer accepted) defaults to `develop` with a release-notes flag — shipping it in a patch release
+  is a deliberate call for an urgent fix, not the default
 - **Repo-tooling/lint/CI-config changes:** target `develop` if the diff also edits runtime source —
   converting call sites, changing behavior a new lint rule now gates — since what the code emits at
-  runtime changed regardless of how enabling it was triggered. Target `stable` only when the diff has
-  no source-code changes at all (pure config, docs, CI).
+  runtime changed regardless of how enabling it was triggered. Target `stable` when the diff has no
+  runtime source changes (pure config, docs, CI, test-only).
+- **Wide mechanical churn** (a reformat, a rename sweep): the category rules above yield to conflict
+  cost — land it on the branch where the touched files diverge least from the other main branch, and
+  say so in the PR description, or every forward merge pays for the churn again
 - **Verify the base before cutting:** check that the code the ticket references actually exists on
   the chosen base (`git ls-tree <base> -- <path>`); follow-up tickets often reference modules that
   are only on `develop`

dev/guides/frontend/writing-e2e-tests.md

@@ -318,7 +318,7 @@ Use `pytest.mark.skip` with a reason that names the cause (the equivalent of the
 `test.fixme`):
 
 ```python
-@pytest.mark.skip(reason="flaky upstream ordering, see #1234")
+@pytest.mark.skip(reason="flaky upstream ordering in the list view")
 async def test_broken(self, admin_page: Page) -> None: ...
 ```
 

dev/knowledge/backend/database-schema.md

@@ -374,15 +374,9 @@ These filters apply to *any* query whose contract mentions "active" or "current"
 
 Neo4j rejects some writes with errors that are safe to replay on a fresh transaction (a lock contention deadlock, or an entity that a concurrent transaction removed mid-statement). Infrahub retries these at the transaction layer with the `retry_db_transaction` decorator.
 
-`retry_db_transaction(name=...)` wraps an `async` method that owns its transaction. On a retriable error it re-runs the whole method after an exponential backoff with jitter, up to `retry_limit` attempts; a non-retriable error propagates immediately. The retriable set is defined by `is_retriable_db_error` in `database/__init__.py`:
+`retry_db_transaction(name=...)` wraps an `async` method that owns its transaction. On a retriable error — `is_retriable_db_error` accepts `TransientError` (deadlock, lock timeout) and `EntityNotFound`, nothing else — it re-runs the whole method after an exponential backoff with jitter, configured by the `INFRAHUB_DB_RETRY_*` settings; a non-retriable error propagates immediately.
 
-| Error | Retriable |
-|-------|-----------|
-| `neo4j.exceptions.TransientError` (deadlock, lock timeout) | Yes |
-| `ClientError` with code `Neo.ClientError.Statement.EntityNotFound` | Yes |
-| Any other exception | No |
-
-Backoff is configured under the `database` settings (`INFRAHUB_DB_RETRY_*` environment variables): `retry_limit`, `retry_base_delay`, `retry_max_delay`, `retry_jitter_max`.
+**A new session escapes the caller's transaction.** `start_transaction()` carries the current session forward, but `start_session()` builds a fresh session straight from the driver — writes made through it commit on their own, whatever transaction the caller holds, so a caller rollback keeps them. Code handed a `db` runs its queries on that `db`; a helper that opens per-task sessions for concurrency must fall back to running sequentially on the caller's `db` when `db.is_transaction`.
 
 **The retry must run at the transaction owner.** A method decorated with `retry_db_transaction` opens the transaction it retries. Code running *inside* that transaction (a query loop, a nested helper) must let a retriable error propagate to the owner rather than catching it and returning a failed result: a caught error still leaves the transaction poisoned, so the commit fails with a non-retryable `TransactionError` and the replay never happens. Only paths that run outside any transaction (they skip the transaction wrapper and have no owner to replay them) record the error as a failure instead. Gate that choice on `db.is_transaction`.
 

dev/knowledge/frontend/react.md

@@ -53,3 +53,7 @@ useEffect(() => setFiltered(items.filter(i => i.active)), [items]);
 Anything a user might bookmark, share, or refresh-and-resume (filters, current selection, mode toggle) lives in the URL — not in `useState`. Use `nuqs` for typed URL params, or `useFilters` for the standard filter pattern.
 
 The page component reads URL params and passes them down. Children should not read `searchParams` for state the page already owns. See `dev/guidelines/frontend/page-architecture.md` for the full state-ownership rules.
+
+## An effect-driven retry needs a dependency that changes on failure
+
+The REST client sets `retry: false` app-wide (`shared/api/rest/client.ts`), so a failed query stays failed until something re-triggers it. An effect that launches a must-eventually-succeed step re-runs only when a dependency changes; if every dependency is stable after a failure (same name, same boolean, a stable `refetch`), the step never retries and the screen wedges until reload. Give such an effect a fetch-identity dependency — TanStack Query's `dataUpdatedAt` — so each fresh response re-arms it.

docs/AGENTS.md

@@ -125,7 +125,7 @@ The `migrate-feature-page` skill documents the full workflow.
 - Include language tags on code blocks
 - Choose the appropriate documentation type (guide vs. topic)
 - Define technical terms on first use
-- Verify factual claims (attribute kinds, GraphQL fields, defaults) against the code on the branch the PR targets — docs PRs frequently target a release branch whose features differ from the development branch; this applies doubly before acting on a bot review claim that something "does not exist". For `infrahubctl`/SDK features, the reference is the commit the `python_sdk` submodule pins (`git -C python_sdk show $(git rev-parse HEAD:python_sdk):<path>`), not an SDK branch tip — and never bump the pin just to make docs resolve
+- Verify factual claims (attribute kinds, GraphQL fields, defaults, UI button and label text) against the code on the branch the PR targets — docs PRs frequently target a release branch whose features differ from the development branch; this applies doubly before acting on a bot review claim that something "does not exist". For `infrahubctl`/SDK features, the reference is the commit the `python_sdk` submodule pins (`git -C python_sdk show $(git rev-parse HEAD:python_sdk):<path>`), not an SDK branch tip — and never bump the pin just to make docs resolve
 - When documenting marketplace items, verify each item actually resolves in the live catalog at <https://marketplace.infrahub.app>; if an item is planned but unpublished, get an explicit decision on release timing before referencing it
 - Prefer plain Markdown/MDX over custom React components in doc pages; before adding anything to `docs/src/components/`, check the existing components for reuse, and give a genuinely new component typed props (the docs package typechecks with `tsc`)
 

frontend/app/AGENTS.md

@@ -46,7 +46,7 @@ cd frontend/app && pnpm test              # vitest (browser mode)
 
 ### Knowledge (How the system works)
 
-- `dev/knowledge/frontend/react.md` - React 19 and React Compiler patterns
+- `dev/knowledge/frontend/react.md` - React 19, React Compiler, and effect/retry patterns — load before writing a `useEffect` that drives a fetch or redirect
 - `dev/knowledge/frontend/architecture.md` - Project organization
 - `dev/knowledge/frontend/entities-structure.md` - Entity layer pattern (api/domain/ui), GraphQL fetching, backend authority
 - `dev/knowledge/frontend/shared-components.md` - **Reuse-first inventory** — look here before building anything generic