docs: harvest review lessons from PRs reviewed 2026-08-07 to 2026-08-14 - #10259
Conversation
Route durable review lessons from this week's PR review threads into the internal documentation layer: database edge/tombstone invariants, query cardinality and sargability patterns, schema registry cache semantics, a new git-sync knowledge page, testing assertion/fixture disciplines, spec artifact consistency, changelog fragment dedup, docs restructure and voice rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016dSumZaAMuhcjD6kujJ2gi
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
- docs/AGENTS.md: the marketplace-verification rule cited a nonexistent API route (`GET /api/v1/schemas/infrahub/<item>`); Infrahub's schema endpoints live under /api/schema and the catalog is the marketplace site, so point at https://marketplace.infrahub.app instead - dev/knowledge/backend/query-pattern.md: correct the cardinality claim. Binding an edge variable does not multiply rows in Cypher, one row per matching edge is inherent, and the graph holds one HAS_ATTRIBUTE edge per branch that touched the attribute. WITH DISTINCT is what fixes it; keeping the outer edge anonymous avoids the subquery shadowing error, not row duplication - .agents/skills/migrate-feature-page/SKILL.md: the Step 10 PR-template example still said "Standard spoke 'Related' closer" while the new rule mandates `## Next` - docs/AGENTS.md: repoint two dead links. guides/AGENTS.md and topics/AGENTS.md were archived in #8958 and deleted in c5acd83; the content lives in dev/guides/docs/writing-a-guide.md and writing-a-topic.md, already named further down this file Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
…e entries - Step 2 of the workflow gave the guide path in full but the topic path as a bare filename; both are now fully qualified, matching every other path reference in the file - the File Structure listing still advertised docs/guides/AGENTS.md and docs/topics/AGENTS.md, deleted in c5acd83 alongside the links fixed in 076ba66 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Documentation-only PR codifying review lessons into knowledge pages, guidelines, and agent rules; no production code, config, or behavior changes. All diffs are additive markdown edits, so no human-owned tradeoff remains.
Re-trigger cubic
- dev/knowledge/backend/testing.md: reframe the benchmark lesson around the input rather than the reporting tool. The rule is that a benchmark taking a different path measures its neighbour, so "no change" is an honest report of the wrong thing; dropped the wording that read as "don't trust CodSpeed" - .agents/skills/harvesting-review/SKILL.md: drop the str|Sequence example from the fragile-pattern guardrail. Naming this PR's own case inside a general rule is the over-customization the skill's own style rules forbid - .agents/skills/creating-changelog-entries/SKILL.md: merge the orphan-prefix and tracker-ID mistakes into one bullet stating the filename shape. Answers whether +ifc-2546-lorem-ipsum is valid: the + suppresses the issue link so it is, but the type segment is required and the ticket ID never renders - docs/AGENTS.md: fully qualify the topic-guide path, matching #10259 Net -1 line across the four files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| - Explains *why* the code exists when the why is non-obvious (a constraint, an invariant, a workaround for a specific upstream bug). | ||
| - Documents the contract of a public function (inputs, outputs, errors raised) when it crosses a module boundary. | ||
| - Stays silent by default. A comment that restates the code is worse than no comment — it adds noise and 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. |
| - **Same branch**: Set `to` property to deletion time | ||
| - **User branch deleting default branch data**: Add `status="deleted"` edge with `from` = deletion time | ||
|
|
||
| These two mechanisms are exclusive. A `status="deleted"` edge is terminal: it always keeps `to: NULL`, never receives a `to` timestamp, and is never re-opened. `to` is only ever set to close a `status="active"` edge. |
| | `add_subquery(str, alias)` | Wrap in `CALL (alias) { }` block | | ||
| | `update_return_labels(list)` | Add labels to RETURN clause | | ||
|
|
||
| Constructors take primitives (ids, names, ranges, kinds) — not domain objects like a `Node` subclass. A query that reads fields off a node instance creates a two-way dependency between the query layer and the node layer; compute the primitive values at the call site and pass them in. Some legacy queries (e.g. the number-pool family) still take node objects — follow this rule for new queries rather than the sibling precedent. |
|
|
||
| ### Query performance | ||
|
|
||
| `AttributeValueIndexed` values are stored natively typed (a number attribute's `av.value` is an integer). Compare `av.value` directly in `WHERE` predicates — wrapping it in a function (`toInteger(av.value) >= $x`) makes the predicate non-sargable and forces a scan of the kind instead of an index range seek. |
There was a problem hiding this comment.
no idea what non-sargable is. looks like it means "cannot use an index"
There was a problem hiding this comment.
me neither will adjust in plain english
…o stable (#10270) * docs(skills): bring the harvesting-review skill and its house style to stable Extracted from #10030 so the next harvest runs with the improved skill instead of waiting for the develop -> stable release train. Every file here is byte-identical to #10030's version, so the branches converge rather than drift. - .agents/skills/harvesting-review/SKILL.md: 'Refine, don't accrete' leads the skill and outranks its other rules (measure the target file against its size range, compress or split one at its limit, repoint every inbound reference on a move, cut what the new rule supersedes, report added/removed lines). Adds the per-run rot sweep that fixes stale citations and defect-snapshot notes in the same PR, and routes a lesson into the skill that owns its workflow. - dev/guidelines/documentation.md: the 'For Internal Docs' writing style the skill points at, plus the no-line-number and relative-measurement rules. The skill's house-style pointer would dangle without this. - .agents/commands/pre-ci.md: plain 'ruff check' CI-parity step, since the invoke tasks run 'ruff check --diff' and exit 0 on violations with no autofix. - .agents/skills/monitoring-pull-requests/SKILL.md: don't escalate from a stale 'commits behind' count. Deliberately not included, because they would not be correct on stable yet: the speckit-extract repoint (targets dev/guidelines/backend/exceptions.md, which only exists on develop), and the .agents/rules/* and creating-changelog-entries changes (their stable copies have diverged, and #10259 already edits them against stable). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: apply cubic's four findings Same fixes as on #10030 (757b34f), keeping every file byte-identical across the two branches. - the invoke-lint justification was factually wrong: main.lint and backend.ruff both run a plain 'ruff check', the only --diff is the formatter's. The real reason the whole-repo check matters is coverage, since CI lints everything outside the four dirs main.lint covers - the citation example named a develop-only module; now a generic placeholder - the report template's '(§6)' house-style pointer aimed at Report - the rot-sweep glob missed AGENTS.md files two levels deep Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…er harvesting-review v0.8.0 Address review feedback on the query-performance note (plain-English index explanation instead of 'non-sargable') and run the new skill version's rot sweep over the internal-doc layer: drop PR/issue-number citations that the documentation guidelines already forbid, remove drifted line-number references, and update a renamed template-flow method (Node.handle_object_template). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016dSumZaAMuhcjD6kujJ2gi
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Confidence score: 5/5
- In
dev/knowledge/backend/templates.md, the numbered list still has two stale hard-coded line references after removings.manage_object_template_schemas(), which could misdirect readers to the wrong sections and slow troubleshooting or onboarding — update those sibling line numbers (or switch to more stable references) to de-risk future edits.
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="dev/knowledge/backend/templates.md">
<violation number="1" location="dev/knowledge/backend/templates.md:32">
P3: The line-number sweep that removed `s.manage_object_template_schemas() (~line 2889)` left the two sibling entries in the same numbered list still carrying stale hard-coded line numbers. Both are now wrong by ~114 lines in the actual source: `add_relationships_to_template() (~line 2669)` is really at `schema_branch.py:2783`, and `manage_object_template_relationships() (~line 2535)` is really at `schema_branch.py:2649`. Since this PR's stated purpose is to sweep stale citations, either drop the `(~line …)` annotations from those two entries for consistency or correct them to the real locations.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| Template schemas are generated during schema processing in `process_pre_validation` (`schema_branch.py`), in this order: | ||
|
|
||
| 1. **`manage_object_template_schemas()` (~line 2889)** — for every `NodeSchema` that sets `generate_template: true`, call `generate_object_template_from_node()` to construct a `TemplateSchema` (or a generic template for shared abstractions) and register it under the `Template{kind}` name. Identifies dependent component peers via `identify_required_object_templates()` so they get subtemplates too. | ||
| 1. **`manage_object_template_schemas()`** — for every `NodeSchema` that sets `generate_template: true`, call `generate_object_template_from_node()` to construct a `TemplateSchema` (or a generic template for shared abstractions) and register it under the `Template{kind}` name. Identifies dependent component peers via `identify_required_object_templates()` so they get subtemplates too. |
There was a problem hiding this comment.
P3: The line-number sweep that removed s.manage_object_template_schemas() (~line 2889) left the two sibling entries in the same numbered list still carrying stale hard-coded line numbers. Both are now wrong by ~114 lines in the actual source: add_relationships_to_template() (~line 2669) is really at schema_branch.py:2783, and manage_object_template_relationships() (~line 2535) is really at schema_branch.py:2649. Since this PR's stated purpose is to sweep stale citations, either drop the (~line …) annotations from those two entries for consistency or correct them to the real locations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/knowledge/backend/templates.md, line 32:
<comment>The line-number sweep that removed `s.manage_object_template_schemas() (~line 2889)` left the two sibling entries in the same numbered list still carrying stale hard-coded line numbers. Both are now wrong by ~114 lines in the actual source: `add_relationships_to_template() (~line 2669)` is really at `schema_branch.py:2783`, and `manage_object_template_relationships() (~line 2535)` is really at `schema_branch.py:2649`. Since this PR's stated purpose is to sweep stale citations, either drop the `(~line …)` annotations from those two entries for consistency or correct them to the real locations.</comment>
<file context>
@@ -23,15 +23,15 @@ Every auto-generated template kind transitively inherits from one of two core ge
+Template schemas are generated during schema processing in `process_pre_validation` (`schema_branch.py`), in this order:
-1. **`manage_object_template_schemas()` (~line 2889)** — for every `NodeSchema` that sets `generate_template: true`, call `generate_object_template_from_node()` to construct a `TemplateSchema` (or a generic template for shared abstractions) and register it under the `Template{kind}` name. Identifies dependent component peers via `identify_required_object_templates()` so they get subtemplates too.
+1. **`manage_object_template_schemas()`** — for every `NodeSchema` that sets `generate_template: true`, call `generate_object_template_from_node()` to construct a `TemplateSchema` (or a generic template for shared abstractions) and register it under the `Template{kind}` name. Identifies dependent component peers via `identify_required_object_templates()` so they get subtemplates too.
-2. **`generate_object_template_from_node()` (~line 2759)** — builds the bare `TemplateSchema`: copies attributes that have `support_templates`, sets the `template_name__value` HFID, wires `inherit_from` to `CoreObjectTemplate` (or the auto-generated parent template if the source node inherits from one).
</file context>
Ports the remaining content of #10030 onto stable. That PR's branch has develop merged into it, so retargeting it would have turned a 35-file docs change into a 157-commit develop-to-stable merge; this carries the same net patch on a branch cut from stable instead. The harvesting-review skill, the internal-doc house style, the pre-ci CI-parity step, and the citation-rot pruning sweep already reached stable via #10270 and #10259. What was still missing is the conventions themselves: - dev/guidelines/backend/exceptions.md and asgi-middleware.md, split out of python.md so each loads only on its trigger - backend rules: Sequence[T] over T | Sequence[T], concrete-only n.kind, changelog secret masking, InfrahubBatch concurrency, retry_db_transaction scope, query limit/offset and auto-pagination - testing rules: verify wiring by parsing source, assert intermediate signals on converging paths, poll instead of sleep, parametrize with dataclasses - frontend rules: reset shared atoms and URL state between tests, assert the value not the text beside it, no owned state mirrored into a Jotai atom - process: PR ceremony scales with the change, migration-layer fixes over lazy repair in save(), Settings fields reach the hand-maintained compose anchor - development/grafana/AGENTS.md for dashboard conventions query-pattern.md's five near-identical walkthroughs collapse into one example plus an accessor table, paying for the additions. Docs only. Every symbol the new rules name resolves on stable, and all relative links and section anchors check out.
Why
Non-goals: no production code or test changes; no duplication of lessons already pending in #10030 (
pha/conventions, the consolidated harvest for 2026-07-16 → 2026-08-07).PRs harvested from
Lessons in this PR trace to review threads on:
config.SETTINGSin integration testshas_schema_changesgatingdev/knowledge/backend/git-sync.md)WITH DISTINCTbeforeCALLsubqueries, primitives-not-domain-objects in Query constructors, class-scoped schema fixtures, one-sentence why-comments## Nextrules, literal-words voice rule, GitHub links for source files, unused MDX importsAlso read, with nothing durable to harvest (no in-window substantive threads, or lessons already codified): #10251, #10233, #10222, #10221, #10217, #10195, #10183, #10171, #10163, #10151, #10141, #10132, #10121, #10145, #10088, #10016, #9967, #8398, and the dependabot bumps #10168/#10169/#10190/#10191/#10192.
What changed
Grouped by intent (all documentation-layer, 14 files, +93/−2):
dev/knowledge/backend/database-schema.md): stated explicitly thatstatus="deleted"edges are terminal (never receive atotimestamp) and that the two soft-delete mechanisms are exclusive; widened the active-filter guidance to cover detection/audit queries and test helpers, not just production reads.dev/knowledge/backend/query-pattern.md): new-query constructors take primitives, not domain node objects; anchor outer MATCHes with anonymous edges andWITH DISTINCTbeforeCALLsubqueries (branch-versioned edges multiply rows); new "Query performance" note —av.valueis stored natively typed, so don't wrap it intoInteger()and defeat the index.dev/knowledge/backend/schema-definitions.md): the in-workerregistry.schemacache is trusted as up-to-date (no defensiveload_schema_from_db), with the empty-SchemaBranchcaveat; migration-only schema loads belong inside thehas_schema_changesguard.dev/knowledge/backend/git-sync.md+ router entry inbackend/AGENTS.md): remote branch import/mapping (_get_mapped_target_branchvs thevalidate_remote_branchskip) and git error surfacing (git mergeconflicts go to stdout, soGitCommandError.stderris empty).dev/guidelines/backend/testing.md,dev/knowledge/backend/testing.md,.agents/rules/testing-python.md): pinconfig.SETTINGSfields with save/restore fixtures; assert persistence from the DB, not the cache the code wrote; pin literal expected values instead of deriving them with the implementation's own serializer; "does not raise" tests still need a side-effect assertion; documented the class-scoped schema fixture idiom and the deliberate within-moduletest_stepNNordering convention.dev/guidelines/repository-organization.md,.agents/skills/creating-changelog-entries/SKILL.md,.agents/rules/code-doc-style.md): mid-implementation redesigns must sweep every spec artifact in one pass; check for overlapping changelog fragments on multi-PR feature branches; why-comments capped at one sentence.docs/AGENTS.md,dev/guidelines/markdown.md,.agents/skills/migrate-feature-page/SKILL.md): verify factual claims against the PR's target branch and marketplace items against the live catalog; new "Restructuring existing docs" section (content inventory, redirects,## Nextspoke endings — surfacing rules previously reachable only inside the migrate-feature-page skill); literal-words voice rule; GitHub blob links for source-file references; remove unused MDX imports; the skill now mandates## Next(not "Next or Related").What stayed the same: no production code, no tests, no user-facing docs pages, no schema changes.
Notes for #10030 (not applied here because the affected text exists only on
pha/conventions): the parametrize wording at itsdev/guidelines/backend/testing.md§"When to Use This Pattern" still reads "the case data lives in theparametrizedecorator" (contradicts its own module-level-constants guideline), and its tasks-module import advice should state the single import stays function-local. Both trace to unresolved review threads on #10145.How to review
Every edit cites a real review thread; the highest-signal ones to sanity-check are the database-schema invariants and the schema-registry-cache section, both of which encode maintainer statements (ajtmccarty on #10224/#10207). The
migrate-feature-page## Nextmandate settles a heading-name question the reviewer left open on #10135 — flag if you'd rather keep both forms.Review fixes (
076ba6657)Four findings, each verified against the code before fixing:
GET /api/v1/schemas/infrahub/<item>, which does not exist — Infrahub's schema endpoints live under/api/schema(backend/infrahub/api/schema.py) and the catalog is the marketplace site. Now points at https://marketplace.infrahub.app, with no invented per-item path to rot.WITH DISTINCTlesson blamed row multiplication on binding an edge variable, but-[:R]->and-[r:R]->have identical cardinality in Cypher. Rewritten to state the real cause — one row per matching edge, and the graph keeps oneHAS_ATTRIBUTEedge per branch that touched the attribute — withWITH DISTINCTas the fix and the anonymous edge kept for a different reason (it avoids colliding with the subquery's own edge variable).## Nextfor spoke closers while Step 10's PR-description template still offered "Standard spoke 'Related' closer" as example text.docs/AGENTS.md's doc-type table pointed atguides/AGENTS.mdandtopics/AGENTS.md; those were archived in Documentation v2 #8958 and deleted inc5acd830a, and the guidance now lives indev/guides/docs/writing-a-guide.md/writing-a-topic.md— already named further down the same file. Repointed.Context budget
+97/−8 after the fixes. Two files sit outside their range, both pre-existing and already being reduced elsewhere — deliberately not compressed here, because compressing them differently on
stablethan #10030 does ondevelopwould guarantee a conflict at the release merge for no net gain:dev/knowledge/backend/query-pattern.md## Internalssplit is the flagged next changedev/knowledge/backend/testing.mddev/guidelines/backend/testing.mddev/knowledge/backend/database-schema.mddev/knowledge/backend/git-sync.md(new)This run was authored before #10030's "Refine, don't accrete" rule reached
stable, which is why its additions are not paid for with deletions — that rule lands with #10030 and applies from the next harvest.How to test
Ran locally: markdownlint 0 errors (Vale not available in the sandbox; no
docs/docspages touched).Impact & rollout
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_016dSumZaAMuhcjD6kujJ2gi
Generated by Claude Code