From 10cf015b5a589507c17345b82b36360404d43daa Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 07:35:44 +0000 Subject: [PATCH 1/2] docs: harvest review lessons from PRs reviewed 2026-08-14 to 2026-08-21 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Weekly harvest of review-thread lessons into the internal-doc layer: - frontend/styling.md: new Theme tokens section — token swaps are visual changes, palette-family consistency, WCAG AA tiers, fixed-scheme surfaces, sibling consistency, no dead tokens (from the dark-theme PR series 10267-10284, 10295) - AGENTS.md: changelog fragment for any user-visible change in Always Do (the single most-repeated review comment this week) - docs/AGENTS.md: verify SDK/infrahubctl claims against the pinned python_sdk commit (10293); say "object" not "node" in user-facing text (10228) - markdown.md: never route relative links through repo symlinks — GitHub does not follow them (10293) - code-doc-style.md: never document what the code does not do (10322, 10228) - documentation.md: don't reference steps by number (10311) - git-workflow.md: branch targeting for lint/tooling PRs (10220) - schema-definitions.md: deprecation messages name the removal version (10306) - database-schema.md: instance-level relationship self-loops are unsupported (10312) - design-system.md: pruned five stale PR-number citations (rot sweep) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01XdeNcpTLZ15aUaz6H5uJVd --- .agents/rules/code-doc-style.md | 1 + AGENTS.md | 1 + dev/guidelines/documentation.md | 1 + dev/guidelines/frontend/styling.md | 23 +++++++++++++++++++++ dev/guidelines/git-workflow.md | 3 +++ dev/guidelines/markdown.md | 1 + dev/knowledge/backend/database-schema.md | 3 +++ dev/knowledge/backend/schema-definitions.md | 2 +- dev/knowledge/frontend/design-system.md | 10 ++++----- docs/AGENTS.md | 4 +++- 10 files changed, 42 insertions(+), 7 deletions(-) diff --git a/.agents/rules/code-doc-style.md b/.agents/rules/code-doc-style.md index 08cd8218241..cbbe3d384e5 100644 --- a/.agents/rules/code-doc-style.md +++ b/.agents/rules/code-doc-style.md @@ -51,3 +51,4 @@ Where IDs *do* belong: - Documents the contract of a public function (inputs, outputs, errors raised) when it crosses a module boundary. - Stays silent by default. 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. Reviewers repeatedly ask for multi-line inline comments to be condensed. +- Never describe what the code does *not* do. Docstring paragraphs explaining the approach not taken or the calls deliberately avoided belong in the PR description; keep the one line stating what the code does. Reviewers repeatedly ask for these paragraphs to be deleted. diff --git a/AGENTS.md b/AGENTS.md index 578a9683647..e3bfe553685 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -163,6 +163,7 @@ 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); pure internal changes need 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 diff --git a/dev/guidelines/documentation.md b/dev/guidelines/documentation.md index 14445783394..57121267a26 100644 --- a/dev/guidelines/documentation.md +++ b/dev/guidelines/documentation.md @@ -115,6 +115,7 @@ agents with a job to finish. - Focus on "how to" instead of "how it works" (in topics) - Reference Jira tickets, GitHub issues, PR numbers, or spec files as the reason for a rule — describe the underlying behavior or constraint instead. These rot once the item closes and the spec is forgotten, and a reader can't verify a closed reference the way a reviewer could at review time. Work-item IDs belong in commit messages, PR descriptions, and changelog fragments; track a significant architectural decision in `dev/adr/` (see `dev/adr/README.md`) instead, written as a self-contained Context/Decision/Consequences record independent of the spec that prompted it - Cite a `file.py:123` or `file.py:100-140` line location — reference the module path and symbol only (`some/module.py::SomeClass`). A symbol reference survives the code moving within a file or being renamed at the call site; a line number does not, and a spec's own line-numbered citations routinely rot before the feature it describes even merges +- Reference another step by its number ("see step 4") — numbering shifts when a step is added or removed; name the action instead ("after restarting the workers") ## Documentation Workflow diff --git a/dev/guidelines/frontend/styling.md b/dev/guidelines/frontend/styling.md index ef3f28f6e0e..74f1e7ae5d3 100644 --- a/dev/guidelines/frontend/styling.md +++ b/dev/guidelines/frontend/styling.md @@ -72,6 +72,29 @@ export const Button = ({ variant, size, className, ref, ...props }: ButtonProps) ); ``` +## Theme tokens + +Style with the semantic theme tokens (`bg-surface`, `text-foreground-muted`, ...) instead of raw +palette classes, so surfaces follow the active light/dark theme. When migrating a hard-coded color +to a token: + +- **A token swap is a visual change unless the rendered value is identical.** Check the token's + computed value in both themes against the class it replaces before claiming "light theme + unchanged" in a PR, and list any deliberate visual change in the description. A solid fill + replaced by a translucent overlay, or `neutral-100` replaced by a `stone-600/10` wash, is a + user-visible change even though the diff looks mechanical. +- **Stay in the theme's palette family.** Don't map a surface to a `gray-*`-backed token when the + surrounding theme uses `neutral`/`stone` — pick the token whose family and shade match what the + surface rendered before. +- **Keep readable text at WCAG AA (4.5:1).** Secondary text (labels, badges, nav items, hints) takes + the muted-foreground tier; the faintest tier is only for decorative or placeholder content that + may fall below AA. Demoting readable text to the faintest tier is the most-repeated review finding. +- **Fixed-scheme surfaces don't take theme tokens.** A component hardcoded to one scheme (an + always-dark code viewer) needs values readable on that surface; a token that flips with the theme + is unreadable in one mode. +- **Identical sibling controls take identical tokens**, and a token added to `theme.css` needs a + consumer in the same PR. + ## Forbidden | Don't | Do | diff --git a/dev/guidelines/git-workflow.md b/dev/guidelines/git-workflow.md index 6297aaeda9e..ea9f7d1e29a 100644 --- a/dev/guidelines/git-workflow.md +++ b/dev/guidelines/git-workflow.md @@ -14,6 +14,9 @@ Git workflow and commit conventions for the project. - **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` +- **Lint/tooling changes follow the same split:** enabling a lint rule that rewrites runtime call + sites is development work — target `develop`; a tooling change that touches only config or docs + may target `stable` - **Branch naming:** `-` (e.g., `jd-add-breadcrumbs`) ## Versioning diff --git a/dev/guidelines/markdown.md b/dev/guidelines/markdown.md index 49d09808e0a..22de955322e 100644 --- a/dev/guidelines/markdown.md +++ b/dev/guidelines/markdown.md @@ -87,6 +87,7 @@ from infrahub_sdk import InfrahubClient - Use relative paths for internal documentation links - All documentation URLs should be relative (not absolute) - When referencing Infrahub source files (models, sample scripts), link the file on GitHub (`https://github.com/opsmill/infrahub/blob/stable/`); never cite a bare repo path — docs readers have no checkout +- Never route a relative link through a repo symlink (the root `specs` is a symlink to `dev/specs`) — GitHub's renderer does not follow symlinks, so the link 404s on the web UI even though it resolves in a checkout; link the real path ```markdown diff --git a/dev/knowledge/backend/database-schema.md b/dev/knowledge/backend/database-schema.md index eff10ed0e23..78d453f2e7f 100644 --- a/dev/knowledge/backend/database-schema.md +++ b/dev/knowledge/backend/database-schema.md @@ -235,6 +235,9 @@ Implementation: `Branch.get_query_filter_path()` in `backend/infrahub/core/branc Outbound on `n1`, inbound on `n2`. +A node is never its own relationship peer: instance-level self-loops (`n1` = `n2`) are unsupported. +Same-kind relationships between two distinct nodes are the supported case (the unidirectional form). + ### Node Existence ```cypher diff --git a/dev/knowledge/backend/schema-definitions.md b/dev/knowledge/backend/schema-definitions.md index 6c7eabca4ff..5877e47b69c 100644 --- a/dev/knowledge/backend/schema-definitions.md +++ b/dev/knowledge/backend/schema-definitions.md @@ -54,7 +54,7 @@ core_standard_webhook = NodeSchema( | `on_delete` | `RelationshipDeleteBehavior \| None` | `None` | `None` (no-action) or `cascade` | | `allow_override` | `AllowOverrideType` | `ANY` | Whether inheriting nodes can override this relationship | | `read_only` | `bool` | `False` | Prevents user modification | -| `deprecation` | `str \| None` | `None` | Deprecation message shown to users | +| `deprecation` | `str \| None` | `None` | Deprecation message shown to users; name the version after which the field is removed (applies to GraphQL `deprecation_reason` too) | | `common_parent` | `str \| None` | `None` | Constrains peer's parent to match this object's parent | | `common_relatives` | `list[str] \| None` | `None` | Peer relationships that must share the same set of peers | diff --git a/dev/knowledge/frontend/design-system.md b/dev/knowledge/frontend/design-system.md index 36d611213db..81c8671d116 100644 --- a/dev/knowledge/frontend/design-system.md +++ b/dev/knowledge/frontend/design-system.md @@ -14,9 +14,9 @@ Most components wrap `react-aria-components` primitives with Tailwind styling. C | Component family | Purpose | |---|---| -| `Button` / `LinkButton` | Any clickable styled button (+ `buttonVariants`). Migrated in #9065. | -| `Card` (`CardHeader`, `CardContent`) | Bordered + rounded + shadowed content surface. Migrated in #9048. | -| `Modal` (`ModalOverlay`) | Dialog/overlay with focus trap and escape handling. Migrated in #9088. | +| `Button` / `LinkButton` | Any clickable styled button (+ `buttonVariants`). | +| `Card` (`CardHeader`, `CardContent`) | Bordered + rounded + shadowed content surface. | +| `Modal` (`ModalOverlay`) | Dialog/overlay with focus trap and escape handling. | | `Sheet` | Side-panel overlay; integrates the dismiss guard (see hooks below). | | `Popover` (`PopoverDialog`, `PopoverTrigger`) | React-aria popover. See the app-popover duality note below. | | `Tooltip` | Hover/focus tooltip with arrow; supports non-interactive triggers. | @@ -29,8 +29,8 @@ Most components wrap `react-aria-components` primitives with Tailwind styling. C | `Tree` (`TreeItem`, `TreeItemContent`, `TreeItemLoader`) | Expandable tree with lazy loading. | | `SortableList` / `SortableItem` | Drag-and-drop reorderable list (react-aria `useDragAndDrop`). | | `ResizablePanelGroup` / `ResizablePanel` / `ResizableHandle` | Split panes built on `react-resizable-panels`. | -| `ScrollArea` | Styled scroll container. Migrated in #9101. | -| `Meter` | Progress/utilization bar. Migrated in #9100. | +| `ScrollArea` | Styled scroll container. | +| `Meter` | Progress/utilization bar. | | `Spinner` | Loading indicator. | | `DismissGuardContext` / `useDismissGuard` | Hook + context to block overlay dismissal (used by `Sheet`; consumers such as dirty forms mark themselves undismissable). | diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 3da71bb3f8e..f417ca6b501 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -91,6 +91,8 @@ Capitalize these Infrahub-specific terms when referring to the feature: **Never use "transform" or "transforms" as a noun.** Always use "Transformation" or "Transformations". +**Say "object", not "node", in user-facing text** (docs, error messages, UI copy) — "node" is internal graph vocabulary. + ## Documentation Workflow 1. **Choose documentation type** using the table above (if not specified) @@ -121,7 +123,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" +- 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 - 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`) From 33018d126ad370dd31d0a530e30d57ac2bd6c96b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 13:41:45 +0000 Subject: [PATCH 2/2] docs: narrow three harvested rules after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - code-doc-style: the "what the code does not do" rule banned durable negative contracts too ("this never raises" in api/admission/priority.py, graphql/api/dependencies.py). Scoped to narrating the rejected approach, with the contract case kept explicitly — it is an auto-injected rule, so an over-broad never would have agents deleting real warnings. - docs/AGENTS.md: "node" is genuine schema vocabulary in user-facing text (schema/nodes-and-attributes.mdx, "Node kind" in webhooks and reference docs, NodeKind: "Node" in the schema provider). Rule now covers populated instances and leaves the schema-kind sense alone. - AGENTS.md: the changelog exemption contradicted the creating-changelog-entries skill, which keeps a housekeeping fragment for internal maintenance. Matched to the skill. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XdeNcpTLZ15aUaz6H5uJVd --- .agents/rules/code-doc-style.md | 2 +- AGENTS.md | 2 +- docs/AGENTS.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.agents/rules/code-doc-style.md b/.agents/rules/code-doc-style.md index cbbe3d384e5..c8c9e646387 100644 --- a/.agents/rules/code-doc-style.md +++ b/.agents/rules/code-doc-style.md @@ -51,4 +51,4 @@ Where IDs *do* belong: - Documents the contract of a public function (inputs, outputs, errors raised) when it crosses a module boundary. - Stays silent by default. 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. Reviewers repeatedly ask for multi-line inline comments to be condensed. -- Never describe what the code does *not* do. Docstring paragraphs explaining the approach not taken or the calls deliberately avoided belong in the PR description; keep the one line stating what the code does. Reviewers repeatedly ask for these paragraphs to be deleted. +- Don't narrate the approach *not* taken. A paragraph on the alternative rejected, or the call deliberately avoided, belongs in the PR description; keep the line stating what the code does. Reviewers repeatedly ask for these paragraphs to be deleted. A negative statement that is part of the contract stays — "this never raises", "does not commit the transaction", "not thread-safe". diff --git a/AGENTS.md b/AGENTS.md index e3bfe553685..57cff96428b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -163,7 +163,7 @@ 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); pure internal changes need none +- 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 - 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 diff --git a/docs/AGENTS.md b/docs/AGENTS.md index f417ca6b501..72d5dd97789 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -91,7 +91,7 @@ Capitalize these Infrahub-specific terms when referring to the feature: **Never use "transform" or "transforms" as a noun.** Always use "Transformation" or "Transformations". -**Say "object", not "node", in user-facing text** (docs, error messages, UI copy) — "node" is internal graph vocabulary. +**Call a populated instance an "object", not a "node"**, in user-facing text (docs, error messages, UI copy). "Node" stays where it names a schema kind — the counterpart of "Generic" — which is the term the schema docs and the UI already use. ## Documentation Workflow