diff --git a/.agents/rules/backend-component-design.md b/.agents/rules/backend-component-design.md index 92188404bcc..bfaa52f38d1 100644 --- a/.agents/rules/backend-component-design.md +++ b/.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. diff --git a/.agents/rules/testing-python.md b/.agents/rules/testing-python.md index 5db4db44269..f06cb5c31d3 100644 --- a/.agents/rules/testing-python.md +++ b/.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 diff --git a/AGENTS.md b/AGENTS.md index c5b4320d1c5..c8f12e6d524 100644 --- a/AGENTS.md +++ b/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//` 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//` directory for the old value or decision and update every file that repeats it in the same commit ### Ask First diff --git a/dev/guidelines/backend/python.md b/dev/guidelines/backend/python.md index ecb94fa67d0..5996db966b1 100644 --- a/dev/guidelines/backend/python.md +++ b/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 diff --git a/dev/guidelines/git-workflow.md b/dev/guidelines/git-workflow.md index ab7ca832c60..e12ea6bfa8b 100644 --- a/dev/guidelines/git-workflow.md +++ b/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 -- `); follow-up tickets often reference modules that are only on `develop` diff --git a/dev/guides/frontend/writing-e2e-tests.md b/dev/guides/frontend/writing-e2e-tests.md index 6b1f1ca6abf..2777878fe75 100644 --- a/dev/guides/frontend/writing-e2e-tests.md +++ b/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: ... ``` diff --git a/dev/knowledge/backend/database-schema.md b/dev/knowledge/backend/database-schema.md index 535b7122c7a..5a209da8b9a 100644 --- a/dev/knowledge/backend/database-schema.md +++ b/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`. diff --git a/dev/knowledge/frontend/react.md b/dev/knowledge/frontend/react.md index b653e8aa3d3..f0502e3a5a6 100644 --- a/dev/knowledge/frontend/react.md +++ b/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. diff --git a/docs/AGENTS.md b/docs/AGENTS.md index fff975ee63c..3a636535948 100644 --- a/docs/AGENTS.md +++ b/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):`), 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):`), 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 ; 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`) diff --git a/frontend/app/AGENTS.md b/frontend/app/AGENTS.md index 883b8897b94..9e81a87e5a6 100644 --- a/frontend/app/AGENTS.md +++ b/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