diff --git a/.agents/rules/backend-component-design.md b/.agents/rules/backend-component-design.md index 708c9d6b651..5c3c4f8462c 100644 --- a/.agents/rules/backend-component-design.md +++ b/.agents/rules/backend-component-design.md @@ -10,11 +10,7 @@ Applies when creating a new backend component or making significant changes to a ## 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. +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. 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. @@ -22,6 +18,10 @@ The single exception is editing existing code where adding a required parameter Construct components as close to the application entry point as possible. 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__`. +**The whole graph is built there, in one pass, before the work starts.** A component that builds a collaborator part-way through a run — when the flow reaches the step that needs it — has the same problem as one that builds it in `__init__`, and is harder to follow because the wiring is spread across the execution. Pass the collaborator in, and pass per-run values to the entry method instead of holding them as constructor state. + +**Invalid wiring fails while you build the graph.** Validate the combination at construction and raise; never let a missing or mismatched collaborator surface later as a silently skipped step or a degraded fallback. Where a generic ties two sides together, parameterize both so the mismatch is a type error rather than a runtime discovery — a `Handler[Any]` paired with `Output[Any]` type-checks against anything and defers the failure to the run. + Prefect `@flow` functions are application entry points: resolve singleton getters (`get_database()`, `get_workflow()`, …) at the top of the flow only — never inside helpers or component internals — then build the component and delegate to it. The flow body stays a thin composition root; the business logic lives in the component. ## Single entry point, operating on arguments diff --git a/.agents/rules/code-doc-style.md b/.agents/rules/code-doc-style.md index 8a8573c3657..8cfe733d477 100644 --- a/.agents/rules/code-doc-style.md +++ b/.agents/rules/code-doc-style.md @@ -39,6 +39,8 @@ Do not reference Jira tickets, GitHub issues, or spec-kit identifiers in docstri Why: these belong in the commit message and PR description. In source, they become noise once the ticket is closed and the codebase has moved on. +Spec **vocabulary** goes the same way as spec IDs. A phrase coined in a spec ("the unsound package-directory floor") means nothing to a reader who never read that spec, and test names and docstrings are read by people who never will. Say what the code does in plain terms instead. + Where IDs *do* belong: - Commit messages, PR titles/descriptions diff --git a/dev/guidelines/backend/python.md b/dev/guidelines/backend/python.md index ef7b8baa023..c410f2f886a 100644 --- a/dev/guidelines/backend/python.md +++ b/dev/guidelines/backend/python.md @@ -81,6 +81,10 @@ deliberate, documented exception: `pyproject.toml`'s `"tasks/**.py"` per-file-ig imports (stdlib, `invoke`, sibling `.shared`/`.utils` modules) at the top; defer the rest into the function that needs them. +The exception covers a thin task wrapper, not logic that happens to live in `tasks/`. A task body +needing a dozen deferred imports is telling you the logic belongs in a module of its own, which the +task then imports once — put it there and the deferred imports mostly disappear with it. + ## Data Structures Use the appropriate data structure based on context. Do not use Pydantic everywhere. diff --git a/dev/guidelines/backend/testing.md b/dev/guidelines/backend/testing.md index 4b5958bae5a..a9d9740f1dd 100644 --- a/dev/guidelines/backend/testing.md +++ b/dev/guidelines/backend/testing.md @@ -240,12 +240,10 @@ SCHEMA_VALIDATION_TEST_CASES: list[SchemaValidationTestCase] = [ ### When to Use This Pattern -Use the dataclass test case pattern when: +Reach for the dataclass test case pattern when several scenarios share one structure and only the data +varies — it keeps the pytest ids readable as the list grows. -- Testing a function with multiple input/output scenarios -- Test cases share a common structure -- You want readable test IDs in pytest output -- The test logic is the same but data varies +Whichever you pick, the case data lives in the `parametrize` decorator. Don't parametrize over the keys of a module-level dict and look the values up inside the test — the reader has to hold two places in their head to see what a case actually asserts, and the pytest id no longer tells them. For simpler cases with only 2-3 scenarios, standard `@pytest.mark.parametrize` with tuples may be sufficient: diff --git a/dev/guidelines/documentation.md b/dev/guidelines/documentation.md index be225c6612c..93c6aff5a5b 100644 --- a/dev/guidelines/documentation.md +++ b/dev/guidelines/documentation.md @@ -57,6 +57,9 @@ agents with a job to finish. - **Say it once**: if the rule is already written elsewhere, link to it instead of restating it - **Budget**: a new rule is a few lines plus one example. If it needs more than that, it's an explanation for `dev/knowledge/`, not a guideline +- **Relative numbers, not absolute ones**: a measurement is only meaningful as a comparison, since the + absolute figure depends on the environment it was taken in. Write "cuts merge wall time by roughly + 4x", not "runs in 10s" - **Pay for it**: cut the prose the new rule supersedes, and check the file against its size range in [Repository Organization](repository-organization.md) before appending. A file over its range gets split or compressed, not extended diff --git a/dev/guidelines/frontend/page-architecture.md b/dev/guidelines/frontend/page-architecture.md index 0732cdc0f42..f0db8bab310 100644 --- a/dev/guidelines/frontend/page-architecture.md +++ b/dev/guidelines/frontend/page-architecture.md @@ -21,6 +21,7 @@ Every piece of state has exactly one owner. When in doubt, push it up; never dup - **Page `useState` shadowed by selector `useState` for the same field.** Lift to a single owner. If the selector is a form, expose `onSubmit(values)` and let the page commit the values to the URL. - **`useEffect` to copy props into local state.** Derive during render or use `defaultValues` on the form. - **Reading `searchParams` in two places.** One hook call per param, at the page level. Pass values down. +- **Mirroring state that already has an owner into a second store.** URL state copied into a Jotai atom, or form values copied into a `useState`, gives you two values that can disagree and no way to tell which is current. Pick the owner from the table above and read from it; if you find yourself syncing the copy back, the owner is wrong, not missing. ## Pages own URL sync diff --git a/dev/guidelines/frontend/typescript.md b/dev/guidelines/frontend/typescript.md index 40ee6b1475a..63fb64a9477 100644 --- a/dev/guidelines/frontend/typescript.md +++ b/dev/guidelines/frontend/typescript.md @@ -27,7 +27,8 @@ type LinkProps = - Prefix: `use*` - Let TypeScript infer return types (annotate only when complex) -- Include all deps in useEffect/useCallback/useMemo arrays +- Include all deps in the `useEffect` array. Don't reach for `useMemo`/`useCallback`/`memo` at all — the + React Compiler memoizes for you (see `dev/knowledge/frontend/react.md`) ## Type Safety diff --git a/dev/guides/frontend/writing-component-tests.md b/dev/guides/frontend/writing-component-tests.md index 0f66594202b..8638727ae1e 100644 --- a/dev/guides/frontend/writing-component-tests.md +++ b/dev/guides/frontend/writing-component-tests.md @@ -67,6 +67,22 @@ Follow BDD structure consistently: - A test that needs a second WHEN/THEN is exercising a multi-phase flow - split it into separate, single-phase tests instead of chaining WHEN → THEN → WHEN → THEN in one test. +## The test has to fail when the behavior breaks + +Before you call a test done, ask what would make it fail. Two shapes pass no matter what the component +does: + +- **An assertion on the static part of the output.** `getByText("Now working on")` passes when the + branch name beside it is wrong, missing, or blank. Assert the whole user-visible string, including the + value the change is about: `getByText("Now working on platform-upgrade")`. +- **A setup flag the component never reads.** Setting `isFetching: true` on a mocked query hook proves + nothing if the component only destructures `isPending` — the test is indistinguishable from the one + above it. Check the component actually reads what you varied. + +A behavior change needs a test that exercises the new behavior, not the old default: replacing a +hardcoded `"main"` with a `is_default` lookup is only covered by a case whose default branch *isn't* +named `main`. + ## Querying Elements Prefer accessibility-based queries in this order: diff --git a/dev/knowledge/backend/query-pattern.md b/dev/knowledge/backend/query-pattern.md index 210fbc34b56..fe3199af113 100644 --- a/dev/knowledge/backend/query-pattern.md +++ b/dev/knowledge/backend/query-pattern.md @@ -125,7 +125,9 @@ class MyQuery(Query): ### Pagination -Pagination (`LIMIT`/`OFFSET`) is automatically appended based on constructor parameters. To write pagination directly in your query, set `insert_limit = False`: +Pagination (`LIMIT`/`OFFSET`) is automatically appended based on constructor parameters. Pass `limit` and `offset` through to `super().__init__()` — a subclass that keeps its own copies leaves the base `self.limit`/`self.offset` at `None`, and `execute()` reads those to decide how to run: with both unset it takes the `query_with_size_limit()` path, which re-runs the query in `query_size_limit` chunks with offsets of its own. The subclass then pages twice, against itself. + +To write pagination directly in your query, set `insert_limit = False`: > **List reads default to a page limit (e.g. `Branch.get_list` defaults to `limit=1000`).** Any check that must reason over *all* matching rows — "is any branch merging?", "are there duplicates?" — must not rely on an unbounded read of a default page. Narrow the query with a filter (a status/kind predicate) or paginate explicitly; otherwise the check silently ignores everything past the first page once the table grows. > @@ -248,99 +250,36 @@ class MyQuery(Query): | Result dataclass | `{QueryName}Result` | `NodeGetListQueryResult` | | Query method | `get_data()` | Yields typed results as a Generator | -### Pattern Examples - -The following examples show common patterns for building dataclasses within the `get_data()` method. - -**Scalar values** (`RETURN n.uuid AS uuid, n.kind AS kind`): - -```python -@dataclass(frozen=True) -class ScalarResult: - uuid: str - kind: str - -# In get_data(): -def get_data(self) -> Generator[ScalarResult, None, None]: - for result in self.get_results(): - yield ScalarResult( - uuid=result.get_as_type("uuid", str), - kind=result.get_as_type("kind", str), - ) -``` - -**Node properties** (`RETURN n.uuid AS node_uuid, n.kind AS node_kind, elementId(n) AS db_id`): - -```python -@dataclass(frozen=True) -class NodePropertiesResult: - uuid: str - kind: str - db_id: str - -# In get_data(): -def get_data(self) -> Generator[NodePropertiesResult, None, None]: - for result in self.get_results(): - yield NodePropertiesResult( - uuid=result.get_as_str("node_uuid"), - kind=result.get_as_str("node_kind"), - db_id=result.get_as_str("db_id"), - ) -``` - -**Node + relationship properties** (`RETURN n.uuid AS node_uuid, r.branch AS rel_branch, r.status AS rel_status`): - -```python -@dataclass(frozen=True) -class NodeWithRelResult: - node_uuid: str - rel_branch: str - rel_status: str +### Reading values out of a result -# In get_data(): -def get_data(self) -> Generator[NodeWithRelResult, None, None]: - for result in self.get_results(): - yield NodeWithRelResult( - node_uuid=result.get_as_str("node_uuid"), - rel_branch=result.get_as_str("rel_branch"), - rel_status=result.get_as_str("rel_status"), - ) -``` +Every shape follows the same `get_data()` loop; only the accessor changes. Pick it by the column's +shape and let it do the typing, rather than indexing the raw record and casting: -**Collection of properties** (`RETURN n.uuid AS primary_uuid, collect(peer.uuid) AS related_uuids`): +| Column shape | Accessor | +|---|---| +| A scalar | `get_as_type("uuid", str)` | +| A scalar that may be absent | `get_as_optional_type("description", str)` | +| A string (shorthand, returns `str | None`) | `get_as_str("node_uuid")` | +| A `collect(...)` list | `get_as_list_of_type("related_uuids", str)` | +| A whole node or relationship | `get("n")` | ```python @dataclass(frozen=True) -class CollectionResult: - primary_uuid: str - related_uuids: tuple[str, ...] # tuple for frozen dataclass - -# In get_data(): -def get_data(self) -> Generator[CollectionResult, None, None]: - for result in self.get_results(): - yield CollectionResult( - primary_uuid=result.get_as_str("primary_uuid"), - related_uuids=tuple(r_uuid for r_uuid in result.get("related_uuids")), - ) -``` - -**Optional values**: - -```python -@dataclass(frozen=True) -class OptionalResult: +class NodeWithPeers: uuid: str description: str | None + peer_uuids: tuple[str, ...] # tuple, so the dataclass can stay frozen -# In get_data(): -def get_data(self) -> Generator[OptionalResult, None, None]: +def get_data(self) -> Generator[NodeWithPeers, None, None]: for result in self.get_results(): - yield OptionalResult( + yield NodeWithPeers( uuid=result.get_as_type("uuid", str), description=result.get_as_optional_type("description", str), + peer_uuids=tuple(result.get_as_list_of_type("peer_uuids", str)), ) ``` + ### Query Method Patterns ```python diff --git a/dev/knowledge/backend/schema-definitions.md b/dev/knowledge/backend/schema-definitions.md index 68a7efe126d..4590ddc4bc0 100644 --- a/dev/knowledge/backend/schema-definitions.md +++ b/dev/knowledge/backend/schema-definitions.md @@ -89,7 +89,7 @@ All `AttributeSchema` entries must include a `description` field. This is enforc ## Constraint Count Test -`backend/tests/component/message_bus/operations/requests/test_proposed_change.py::test_get_proposed_change_schema_integrity_constraints` contains hardcoded constraint counts. These counts change whenever schemas are added or removed because `ConstraintValidatorDeterminer` iterates all schemas in the registry and generates one `SchemaUpdateConstraintInfo` per validatable property. After schema changes, run the test to get actual counts and update the assertions. See `#2592` for planned improvements. +`backend/tests/component/message_bus/operations/requests/test_proposed_change.py::test_get_proposed_change_schema_integrity_constraints` contains hardcoded constraint counts. These counts change whenever schemas are added or removed because `ConstraintValidatorDeterminer` iterates all schemas in the registry and generates one `SchemaUpdateConstraintInfo` per validatable property. After schema changes, run the test to get actual counts and update the assertions. ## Field Visibility and the Write / Read / Internal Models