Conversation
PR #10018 (INBOX-20) got a reviewer ask for the whole PR to be just the 5-line repository.py annotation fix: no dev/specs/ spec-kit scaffolding, no changelog fragment for a change with no user-facing effect. Neither constraint was written down anywhere, so an agent following the normal speckit pipeline had no way to know to trim them. Document both: - changelog.md: skip the fragment when a change has no user-facing effect; housekeeping is not a catch-all for anything code-adjacent. - repository-organization.md: the spec-kit design record should be proportional to the change it documents, not a fixed-cost byproduct of running the workflow. - git-workflow.md: cross-reference both from the Pull Requests section so the rule is reachable from root AGENTS.md's Coding Standards link. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013MwudkACUQdCZTJ5aVBnr3
PR #10079 widened RelationshipManager.update()'s data parameter from an invariant list to a covariant Sequence to drop a call-site type: ignore. The reviewer's "definitely would have missed this one" was for the carve-out that makes it safe: str satisfies Sequence, so isinstance(data, str) must be checked before isinstance(data, Sequence), or a bare peer id gets iterated character-by-character. Nothing in the guidelines named this gotcha, so document it next to the neighboring isinstance-narrowing and list/Sequence-variance sections. The PR's other two review threads (skip-changelog for no-user-visible- effect changes, and proportional dev/specs/ ceremony) restate lessons already codified on this branch by c1b89c4 (INBOX-20) — that commit just hasn't reached develop yet, so PR #10079's branch never saw it. No doc gap, no edit needed there. The approving review's PeerWithRelationshipMetadata read/write-split suggestion is a real, confirmed observation (manager.py and node/create.py populate it asymmetrically) but is scoped to that one class, not a recurring authoring convention — discarded from this harvest as PR-local tech debt rather than encoded as a rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WgpvpBsruPYD3fYpyfzL7R
…NBOX-19] PR #10002 (INBOX-19) re-enabled ruff's BLE (blind-except) rule and resolved 78 violations: 8 narrowed to specific exception types, 70 kept `except Exception` with a justification comment + `# noqa: BLE001`. Reviewer @ogenstad's threads on that PR, plus an independent cubic-dev-ai finding, surfaced three durable gaps beyond "narrow when you can" (already documented in python.md's Exception Handling section): - No existing guidance asked whether a catch is needed *before* choosing its scope. @ogenstad on api/auth.py: the guarded event-emission goes to an internal queue that can't block the response, so the defensive catch is likely unnecessary altogether - a background task after the response is the better shape when isolation is genuinely required. Same pattern flagged on oauth2.py and oidc.py. - "Deliberate design choice" (the bar for `# noqa: BLE001`) was previously illustrated only by "top-level boundary" (task worker loop, request handler) - too narrow for the batch/collection and best-effort shapes this PR actually used 70 times over. Named all three explicitly with a worked example. - cubic-dev-ai flagged that the justification comment on m066_consolidate_duplicate_number_pools.py's except ("failures become MigrationResult errors") overstates safety: the `return` happens inside `async with db.start_transaction()`, so `InfrahubDatabase.__aexit__` (backend/infrahub/database/__init__.py:298-307) sees no exception and *commits* the partial work instead of rolling it back. Verified directly against both files. The new subsection requires a justification comment to state the actual commit/rollback consequence when nested inside a transaction, with a before/after example built on the real bug shape. Also documented (@ogenstad, twice, on validators/tasks.py and infrahub_load_tester.py): a lint-suppression PR that surfaces a pre-existing behavioral bug should ship the annotation only and file the bug separately, not fix it inline. New git-workflow.md bullet under Pull Requests, cross-linked from the noqa subsection above since the same transaction/commit bug is the concrete example of exactly this pattern. Extended dev/knowledge/backend/async-tasks.md's existing "post-commit follow-up is best-effort" contract with one sentence: it was scoped to per-item batch dispatch, but the same shape (confirm the call can fail, then isolate via a background task rather than an inline try/except) applies to a single inline side effect in a request handler too. Not encoded as a lesson: the three specific bugs flagged in review (m066 commit-not-rollback, tasks.py retry-defeating catch, infrahub_load_tester.py's undefined `all_branches` on failure) are PR-local defects, not conventions - the review thread itself already treats them as follow-up candidates pending a human filing decision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6uYDCYNXbWUdkFEJLW3Zx
…accrete rule
Internal docs had no concision rule and no size discipline, so harvested
lessons arrived padded and always additive: over the last four months
dev/guidelines and .agents/rules took +222/-16 lines, and this branch's own
harvests pushed backend/python.md to 480 lines against its 100-400 range.
The routing rules ("edit before create", "strengthen before duplicate") stop
duplicate rules, not growth — nothing in the flow ever removed a line.
Two additions:
- dev/guidelines/documentation.md gains "Writing Style / For Internal Docs",
covering dev/, the AGENTS.md files, and .agents/ — rule first, plain words,
no provenance, no padding, a budget, and pay-for-it: cut what the new rule
supersedes and check the file's size range before appending.
- The harvesting-review skill leads with "Refine, don't accrete", flagged as
outranking its other rules: measure the file first, compress or split one
that is at its limit, cut superseded prose, and report added/removed line
counts, since a harvest that deletes nothing accreted rather than refined.
Paid for the skill's new section by compressing its 1A load-trigger block,
which had grown into eight lines telling the reader to keep entries short.
Also drops the routing example that pointed at dev/guidelines/changelog.md,
which no longer exists now that changelog conventions live in the
creating-changelog-entries skill, and applies the new brevity rule to the
sections this branch added that read long.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
python.md had reached 480 lines against the 100-400 range that repository-organization.md sets for dev/guidelines, and this branch's harvests added 55 of those. Applying the rule the previous commit introduced: the file at its limit gets split, not extended. Exception Handling and the # noqa: BLE001 subsection move verbatim to dev/guidelines/backend/exceptions.md, which leaves python.md at 405 lines and gives the exception rules their own loadable home. Dropped the standalone "broad except is justified only at a top-level boundary" paragraph on the way, since the noqa list's first bullet already says it. Load-triggers updated so the new file is reachable: entries in backend/AGENTS.md, root AGENTS.md, and dev/README.md, plus a See Also link from python.md. The two inbound anchor links (git-workflow.md and knowledge/backend/async-tasks.md) now point at the new file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR #10146 (INBOX-30) removed a stale mypy override with no source change, and its review threads surfaced three durable gaps. Two were suggested candidates going in; investigation confirmed one and sharpened the other, plus turned up a third while mining the same threads. - cubic-dev-ai flagged a changelog fragment missing towncrier's `+` orphan prefix. The rule already exists in four places (the skill file, two backend/AGENTS.md refs, git-workflow.md, and the PR template checklist), so this is "covered but still flagged," not missing: the doc states the rule but never the failure mode, which is the concrete detail cubic had to supply. Added the consequence (bogus issue link / skipped fragment) to creating-changelog-entries/SKILL.md's Common Mistakes list. - Reviewer @ogenstad's "this needs to be rebased" corrected an escalation that reasoned from "0 commits behind, so a rebase is a no-op." That reasoning was actually correct at the moment it was made — the defect was trusting it unchanged after time passed (waiting on the review) and the upstream base got fixed in the interim (#10147). Neither rebase/SKILL.md (which documents a different mistake: counting against a stale local ref instead of origin/<base>) nor monitoring-pull- requests/SKILL.md addressed re-verifying a stale snapshot. Added a callout to the latter's Phase 2: re-fetch and recompute immediately before deciding, reproduce against the base's current tip, and treat a reviewer's rebase suggestion as a cue to re-verify rather than repeat an earlier count. - Self-disclosed in the same thread: `invoke lint`/`invoke backend.lint` run `ruff check --diff` (tasks/main.py:39, tasks/backend.py:101), which reports only auto-fixable rewrites and exits 0 on an unfixable violation. CI's python-lint job runs a plain `ruff check` (.github/workflows/ci.yml:325-326), so a locally-green lint run is not proof CI's will be. Verified both invocations directly before writing this down. Documented next to the commands themselves in root AGENTS.md's Linting & Formatting section. Not encoded: @ogenstad's unresolved "not sure we need a changelog entry for something like this" — no reply landed and the fragment shipped as-is, so there's no correction to derive a rule from. The existing housekeeping rule already tells the author to ask when unsure; whether this specific case should have triggered that is a human call, not something this thread settles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KWG4Cvk9ZzseV73X9o1QTj
Twelve merged PRs audited; the lessons that generalize are routed into the internal-doc layer, and the citation rot earlier runs left behind is fixed in the same pass. New rules: a benchmark must exercise the input shape a conditional optimization is gated on; reset shared Jotai atoms, mocks and URL state between component tests in one file; tasks/*.py imports stay function-local; a path-exclusion check must not use a bare startswith; a component's initialize() clears every derived cache, not just the obvious field; changelog fragment names use the GitHub issue number, never an internal tracker ID; don't add a return value to a mutating method just so a test can assert on it; a Map lookup seeded from the array you index it with is still a type lie when asserted with !; no file.py:line citations in internal docs. Strengthened where a rule existed but a reviewer still had to raise it: the module-singleton reset-hook case in the frontend unit-test guide, and a cross-link to the add_tags() pitfall where the async-task guide teaches the pattern that reproduces it. Pruned: stale PR/issue citations across five files, a defect-snapshot note that described a current bug rather than a durable convention, and a templates.md reference to a method that had since been renamed. Replayed from stable onto the internal-doc-style branch so both harvests land as one stack. Two reconciliations where the branches overlapped: - The skill's rot sweep now sits under the "Refine, don't accrete" principle the base branch added rather than beside it, and the apply step applies both. - The no-file.py:line rule joins the sibling work-item-citation rule in documentation.md's Don't list; the base branch's "No provenance" bullet points at both instead of restating them. Splitting ASGI Middleware out of python.md pays for the path-matching lesson this run adds. With it the file would have reached 431 lines against a 100-400 range, and the rule the base branch introduced says a file at its limit gets split, not extended. python.md ends at 393. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit facc5d3)
Fifteen merged PRs audited across develop and release-1.11. Three of the lessons were already written down and a reviewer still had to raise them, so those are strengthened in place rather than duplicated. Strengthened: - backend-component-design: the whole object graph is built at the entry point in one pass, not part-way through a run, and invalid wiring raises while the graph is built instead of degrading to a runtime fallback. One reviewer made this point four times in two PRs against a rule that is auto-injected on every turn; it only forbade construction inside __init__. - backend/testing: names the parametrize shape that actually got written — dict keys in the decorator, values looked up inside the test. - frontend/page-architecture: the no-mirroring rule was stated for forms and useState only, so URL state copied into a Jotai atom slipped past it. Added: - query-pattern: pass limit/offset to the base Query constructor, since execute() reads self.limit/self.offset to choose how to run and pages the query in chunks when both are None; and read a collect() column with get_as_list_of_type rather than unpacking rows. - code-doc-style: spec vocabulary is as unreadable as a spec ID in a test name or docstring. - documentation: measurements go in as relative comparisons, not absolute figures that depend on the environment they were taken in. - frontend/writing-component-tests: a test has to fail when the behavior breaks — assert the value and not the static text around it, and check the component reads the flag the setup varies. - python: the tasks/*.py function-local import exception covers a thin wrapper, not logic hosted in tasks/. Pruned: - typescript.md taught useMemo/useCallback dependency arrays while knowledge/frontend/react.md says not to use them at all under the React Compiler. A reviewer had to ask which was right. - schema-definitions.md pointed at an issue for planned improvements that closed on 2026-07-20. - query-pattern.md was 520 lines against a 200-400 range and this run adds to it, so its five near-identical result-dataclass walkthroughs collapse into one example plus an accessor table. The file ends at 459. Net -40 lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 4c4a19d)
A harvesting-review run over the week's review threads, restricted to the PRs the in-flight harvests (#10145, #10098) did not read: open and recently-merged PRs across develop, stable, and release-1.11. New rules and knowledge: - mutations.md: retry_db_transaction placement — wrap only rollback-able transaction scopes, skip under a caller-supplied transaction (#10121) - query-pattern.md: READ queries with insert_limit=False need their own LIMIT; auto-paginated reads need a total-order ORDER BY (#10132) - creating-migrations.md: fix data bugs at the migration layer, not in runtime save paths (#10105); per-item error collection (#10132); batch by the memory-bounding unit (#10132) - database-schema.md: generic kinds exist only as labels — n.kind never matches a generic; type concrete-only inputs as list[NodeSchema] (#9805) - events.md: changelog models mask secrets only at construction — post-hoc assignment leaks them into events (#10105) - async-tasks.md: InfrahubBatch is concurrent, not ordered (#10113) - testing.md + testing-python.md rules: wiring tests parse source with ast/inspect instead of instrumenting production code (#10121); poll-don't-sleep for async effects (#10133); branch-attributable removal assertions (#10132); pure-function extraction before skipping the cheap test tier (#10137) - checklist.md: new Settings fields must reach both compose entry points — one generated and CI-checked, one hand-maintained (#10122) - backend/AGENTS.md: new REST endpoints are ask-first — prefer existing GraphQL for SDK needs (#8594); route creating-migrations.md in Guides - pre-ci.md: invoke lint tasks run ruff check --diff, which exits 0 on unfixable violations — added the plain CI-parity ruff check (#10146) - development/grafana/AGENTS.md (new): only defined datasource variables, sweep drill-down links, regenerate the standalone compose (#10153) - docs/AGENTS.md: Excalidraw exports use an opaque white background (#9953) Strengthened in place: - creating-changelog-entries skill: removed the 'most maintenance still gets a fragment' push that contradicted the no-user-facing-effect boundary; added lint/typing config cleanups as a named example (#10146) Paid for by compressing testing.md's nested-dataclass example and query-pattern.md's duplicated accessor/return-properties sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYisRTn3sPygP55cxbVqzG (cherry picked from commit ca2190d)
Review follow-ups from #10030's threads, applied on top of the three cherry-picked harvest runs (#10098, #10145, #10157): - backend/AGENTS.md: keep the coding-standards list contiguous and let the exceptions.md pointer follow it, dropping the duplicated exception-handling bullet (cubic, polmichel) - speckit.opsmill.extract command + skill: error-handling conventions now route to dev/guidelines/backend/exceptions.md, and the target list is marked as routing examples to re-verify before writing (cubic, polmichel) - dev/guidelines/backend/python.md: replace the str-satisfies-Sequence survival section with a rule against one-or-many unions - prefer Sequence[T]/list[T] parameters and let callers wrap (polmichel) - AGENTS.md: compress the ruff --diff caveat and point at /pre-ci, which carries the CI-parity check (polmichel) - dev/guidelines/repository-organization.md: point the Git Workflow cross reference at its #pull-requests anchor (cubic) - harvesting-review skill 0.8.0: a split or move repoints every inbound reference including .agents/skills and .agents/commands routes; a lesson whose root cause is a fragile pattern becomes a rule steering away from the pattern, not a survival guide; lessons owned by an existing skill (creating-changelog-entries, pre-ci, pruning-residues) are routed into that skill instead of a parallel dev/ rule. Paid for by compressing the sweep's fix-every-hit prose and the report template comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- harvesting-review skill: the sweep greps now include .agents/rules, which the prose already claimed; pruning-residues is identified as an org skills-plugin skill not vendored in this repo - mutations.md: the retry decorator also replays on Neo.ClientError.Statement.EntityNotFound, not only TransientError - testing.md: wire the StaleEntryCleaner example (constructor + instantiation) so the snippet is reproducible - writing-component-tests.md: vi.resetAllMocks in the isolation example, clearAllMocks leaves mockReturnValue overrides in place - writing-unit-tests.md: promote Testing Module-Level Singletons out of Mocking to its own section, it is about module state, not mocks - typescript.md: relative link to react.md instead of a backticked absolute path - development/grafana/AGENTS.md: run convert_compose_standalone.py from development/, the script resolves default paths against the working directory Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Conflict: dev/guidelines/backend/python.md — this branch splits Exception Handling and ASGI Middleware out of the file; develop's copies had meanwhile gained the best-effort-side-effects subsection (#10103). Kept the split and ported that subsection into dev/guidelines/backend/exceptions.md, where the noqa BLE001 section now cross-references it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
docs/AGENTS.md's doc-type table and workflow pointed at guides/AGENTS.md and topics/AGENTS.md. Those were moved to docs/archive/ in #8958 and deleted in c5acd83; the guidance now lives in dev/guides/docs/writing-a-guide.md and writing-a-topic.md, already named further down the same file. Also dropped the two File Structure entries for the deleted files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 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>
cubic flagged these on #10270, the stable extraction; the files are byte-identical so they were live here too. The lint claim was factually wrong. Verified against tasks/main.py and tasks/backend.py: _lint_ruff runs 'uv run ruff check tasks models utilities python_testcontainers' and backend.ruff runs 'uv run ruff check backend', both plain. The only --diff is 'ruff format --check --diff', the formatter step. So the invoke tasks do exit nonzero on an unsuppressed BLE001 inside the paths they cover, and the real reason the whole-repo check is not redundant is coverage: CI runs 'ruff check . --exclude python_sdk', so a violation in development/, tests/, or a root-level script passes locally and fails in CI. Corrected in .agents/commands/pre-ci.md (both phases) and root AGENTS.md. - dev/guidelines/documentation.md: the citation example named core/regeneration/models.py::TargetSelection, a real module on develop but absent from stable. Replaced with a generic some/module.py::SomeClass, which is correct on both branches and cannot rot. - .agents/skills/harvesting-review/SKILL.md: the report template pointed at '(§6)' for house style, but §6 is Report; it now points at 'Refine, don't accrete'. The rot-sweep glob 'AGENTS.md */AGENTS.md' matched only one level deep, missing frontend/app/AGENTS.md and development/grafana/AGENTS.md; $(git ls-files '*AGENTS.md') covers all five and skips .venv, which a recursive glob would have pulled in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #10220 (INBOX-29) re-enabled ruff TRY400 across 36 backend logging sites. Its review was unusually dense - two cubic findings, a rebutted release-vehicle finding, and a long CI-triage thread - and investigation turned up five durable gaps. - cubic-dev-ai flagged graphql/app.py's ClientDisconnect site being converted to log.exception when it's a routine, expected condition (the traceback only shows the body-read path). The author's own research.md stated the criterion ("convert unless the traceback would be actively harmful or worthless") and misapplied it on this exact site - meaning the criterion existed only in a throwaway per-PR spec doc, never in the durable guideline. Added a log.exception vs log.error section to exceptions.md with both the worthless case (this one) and the harmful case (cross-linked to webhooks.md's TracebackSuppressionFilter, the mechanism the same PR's webhook site depends on). - cubic also flagged the same PR's tasks.md citing absolute line numbers that its own commits shifted mid-review. That rule already exists almost verbatim in documentation.md ("cite the module path and symbol only... a spec's own line-numbered citations routinely rot before the feature it describes even merges") - so this is "covered but still flagged," not missing. Neither speckit-plan nor speckit-tasks, the skills that actually generate research.md/plan.md/tasks.md, pointed at that rule while enumerating sites. Added a one-line pointer to each. - The same post-review fix (28/6 to 27/7 conversions) went stale in five sibling spec files (plan.md, research.md, tasks.md, alignment-check.md, the implementation report) before cubic finished flagging all five individually. Root AGENTS.md already tells authors to verify a claim against the diff before writing it down; extended that bullet to also require propagating a later correction across every file in dev/specs/<feature>/ that restates the same figure, not just the one a reviewer pointed at. - cubic asked whether this should target stable instead of develop ("repo tooling... cannot affect a running product"). The rebuttal was right (the diff converts 27 runtime logging call sites; the closest precedents targeted develop) but the underlying policy it invoked - "release-vehicle guidance" - doesn't exist anywhere in dev/ or AGENTS.md; grepped for it directly. Added the rule both sides were actually reasoning from into git-workflow.md's Branch Strategy: a tooling/lint/CI change with a runtime source diff targets develop like a product change; only a zero-source-diff change targets stable. - The PR's CI came back red repeatedly on self-hosted-runner exhaustion, and each reconcile pass proved out two techniques monitoring-pull- requests/SKILL.md didn't have: citing a specific run on the base branch as proof a failure predates the PR (rather than an inferred "probably pre-existing"), and re-running a failed job on the same SHA instead of pushing a fix-attempt commit first, since ci.yml sets concurrency.cancel-in-progress: true and a new push cancels the experiment. Added both to Phase 2's existing staleness callout. Not encoded: a Vale spelling-exceptions judgment call (reword a fragment vs. add a one-off word to the shared vocabulary) - no evidence this recurs, and the decision is a generic technical-writing tradeoff rather than an Infrahub-specific gotcha. The "rebase deliberately skipped" call and the runner-exhaustion diagnosis itself are the existing staleness rule working correctly and an infra incident, respectively - not new lessons. Swept the destination layer for citation rot per the skill's step 5; found none to prune. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QQtPSrTWwyKv5U4vpVTNZ
# Conflicts: # .agents/skills/monitoring-pull-requests/SKILL.md # backend/AGENTS.md # dev/guidelines/backend/testing.md # dev/guidelines/documentation.md # dev/guidelines/frontend/page-architecture.md # dev/guidelines/frontend/route-architecture.md # dev/knowledge/backend/async-tasks.md # dev/knowledge/backend/schema-definitions.md # dev/knowledge/backend/templates.md # dev/knowledge/frontend/design-system.md # dev/knowledge/frontend/shared-components.md
The conventions this branch carried reached develop through the stable sync, so every conflict was resolved by taking develop's newer wording. After the merge the branch adds nothing on top of develop in the internal-doc layer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X4MgERx1fmtgH46YTRqS49
Harvest of the review threads on PR #10600 (clear the mypy suppression on infrahub.lock). Covered but ineffective: the maintainer's `.agents/rules/python-typing.md` rule "No cast()" was in the author's context — added six days earlier — and the PR still shipped a cast() narrowing a lock connection by elimination, with a docstring justifying it. ogenstad restated the rule in review: "To me cast() is the same as having ignore statements in pyproject.toml or inline type ignores. I don't think we should have it as a solution." The correction landed as a positive isinstance that returns the value (632e082). The rule stated the principle but not the two rationalizations that defeated it — "my cast only records what the check above verified" and "the real check breaks mypy in 15 unrelated files" — so: - python-typing.md: the cast() bullet now names both, in one sentence. Same line count. - New dev/guidelines/backend/typing.md, split out of python.md (454 lines, over its 400-line range): carries the whole Type Hints section plus a new "Narrow with a check that returns the value, never with cast()" section with the ❌/✅ pair, and reconciles the grandfathering step with the rule — a scoped ignore covers only a pre-existing, unrelated violation while a module rule is being turned on; never a violation the change introduces, never cast(). The "suppression that needs a paragraph" signal covers cubic's docstring thread, whose rule already lives in code-doc-style.md. - fix-mypy-module command (the workflow that owns "clear an override"): dropped "cast value" and "widen the parameter type" as fix patterns, aligned the ignore guidance with the rule, pointed at typing.md. Missing: `from infrahub.core import registry` is ambiguous — infrahub.core.registry is both a submodule and the object core/__init__.py re-exports under the same name, so mypy binds whichever it resolves first; adding the runtime import in lock.py reordered the cycle and produced 15 errors in types.py and tasks/registry.py. Reproduced here: whole-backend mypy with the PR's lock.py and the old imports gives exactly those 15 errors; a two-file run does not, confirming the order dependence. 108 modules use the ambiguous form, 54 the direct one; the rule is scoped to new code and to modules the checker flags, not a sweep. The same collision exists for infrahub.api.storage and core.schema.definitions.internal. - python.md Imports: the rule with the ❌/✅ pair, pointing at the SchemaManager preference so it does not read as an endorsement of importing the registry. - knowledge/backend/package-init-files.md: a "Name collisions" bullet for the mechanism. - knowledge/backend/architecture.md: the registry entry pointed at core/__init__.py, the re-export, instead of core/registry.py. Routers repointed for the split: root AGENTS.md, backend/AGENTS.md (typing gets its own load trigger), dev/README.md, the harvesting-review skill and the rule's "full reference" line. Not lessons: cubic's NATS-without-service local fallback (pre-existing defect, design decision pending on the card, not documentation); the two suppressions put to ogenstad and not yet answered. Sweep: dropped a PR citation from knowledge/frontend/design-system.md; reframed the "known gap" note in knowledge/backend/async-tasks.md as the mechanism's limit (still unfixed in code: the purge is terminal-state scoped and the setup flows still tag by branch name); two pre-existing markdownlint list-spacing errors in the harvesting skill. Sizes: python.md 454 → 381 (in range); typing.md 135 new; tracked-file diff +49/−111 before the new file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X4MgERx1fmtgH46YTRqS49
Review feedback: the parenthetical "(no cast(), no new ignore)" on the backend/AGENTS.md typing entry was stricter than the guideline it points to. typing.md step 2 permits a scoped `# type: ignore[code] # reason` to grandfather a pre-existing violation while a module rule is being turned on, and clearing a pyproject.toml suppression is exactly that case, so an agent reading the router alone would be steered off the sanctioned path. Dropping the parenthetical rather than qualifying it: a router entry names the trigger and topic, never the rule's mechanics, so the strictness could not be restated there without duplicating — and drifting from — the guideline. The load trigger is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X4MgERx1fmtgH46YTRqS49
# Conflicts: # dev/guidelines/backend/typing.md
… subclasses are component-tested Review lesson from #10663: a unit test that hand-built QueryResult rows for DiffCountChanges was removed because the component suite already drove the method through the diff-tree resolver and asserted the counts it produces, against the real database. A grep for the class name had found nothing, so the method looked uncovered. - dev/guidelines/backend/testing.md: "What not to test" gains "skip a test that duplicates coverage the suite already has" - trace callers to the asserting test, a symbol grep is not a coverage check - with a bad/good example; the tier paragraph names a Query subclass as the standing component-layer case. - .agents/rules/testing-python.md: the tier rule gets the Query converse, plus a three-sentence "Check existing coverage before adding a test" section. - backend/AGENTS.md: the testing guideline was missing from the router; it now has a load trigger in the Testing section and in the Guidelines list. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqxMdEvBzARrBMKaoh4W48
github-actions
Bot
added
the
group/backend
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
saltas888
marked this pull request as ready for review
September 17, 2026 21:59
…uery Review feedback on #10670: the bad/good example used near-miss names (CountChangesByBranch, get_counts_by_branch, run_diff_tree_query) that exist nowhere in the repo, so a reader following the section's own advice - trace the symbol to the test that covers it - found nothing. Both snippets now use the real identifiers: DiffCountChanges with its required timestamps and get_num_changes_by_branch on the bad side, and the actual component test driving DIFF_TREE_QUERY through the graphql helper on the good side; the prose names the test file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqxMdEvBzARrBMKaoh4W48
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
…verage example Review feedback on #10670: the worked case claimed DiffCountChanges is reached only through the diff-tree resolver, but the diff coordinator reaches it too through the module's wrapper function. The sentence now names both callers and keeps the point that matters for the lesson: no test names the class, yet the resolver-path component test covers it against the real database. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqxMdEvBzARrBMKaoh4W48
Contributor
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. Docs-only change: adds a guideline about checking existing coverage before adding tests and registers the testing standards doc in AGENTS.md. No code, behavior, or operational changes; the edits are bounded and clearly beneficial.
Re-trigger cubic
5 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harvest of the review lessons from #10663 (INBOX-34) into the internal-doc layer. One human review thread, from @ajtmccarty, carried a lesson that generalizes; this PR writes it down where the next author will see it.
The thread: a new unit test file for
DiffCountChangeswas flagged as unnecessary because the component suite already covers the method indirectly. Verified against the code before writing anything:backend/tests/component/graphql/diff/test_diff_tree_query.pydrivesget_num_changes_by_branchthrough the diff-tree resolver and assertsnum_untracked_base_changes/num_untracked_diff_changes, including a non-zero case, against a real Neo4j. The deleted unit test hand-builtQueryResultrows, so it could never have caught the one thing that can break in that method: what the driver actually returns. Nothing inbackend/tests/unit/hand-buildsQueryResultrows;Querysubclasses are tested in 41 component files, directly or through their callers.Review-Lessons Report — PR #10663
Scope
1 review thread (resolved), 3 reviews (1 human approval, 1 cubic, 1 author reply), 2 conversation comments (codspeed, agent status). Bot output carried nothing to harvest.
Existing coverage to strengthen (covered but still flagged)
Lesson: a
Querysubclass is tested at the component layer against the real database, never by a unit test that fabricates its rows..agents/rules/testing-python.md§"Pick the cheapest test tier" anddev/guidelines/backend/testing.mdline 17 ("use the database only when behavior genuinely depends on it");dev/knowledge/backend/testing.mdline 29 ("do not mock infrastructure to force a unit test");dev/guidelines/backend/checklist.mdline 124 (component tests for queries).backend/AGENTS.mddid not list the testing guideline at all.Queryconverse;backend/AGENTS.mdgains a load trigger for the testing guideline in §Testing and in the Guidelines list.New rules to add (missing)
Lesson: before adding a test, find the existing coverage by tracing the code's callers to the test that asserts their output; a grep for the class or method name is not a coverage check.
8e9889cb6, which deleted the file with no replacement.361c4f756added four unit tests over hand-built rows and the PR body stated the method "had no coverage"; after, the file is gone and the component test's existing assertions are the coverage. Claim verified ingraphql/queries/diff/tree.py(_add_untracked_fieldscalls the query and writes the two fields) and in the component test's asserted values. Intent: a concrete request for this PR that rests on the standing convention that core query classes are exercised indirectly. Over-scoped readings ruled out: "never unit-test anything undercore/" and "delete unit tests that overlap with component tests".backend/tests/, which returns nothing for a class the suite reaches only through its callers. Writing down the method of checking coverage is what prevents the repeat, and the remainingcore.query.*suppression cards will face the same temptation.dev/guidelines/backend/testing.md§"What not to test" (full text with a bad/good example built on the realDiffCountChangesquery and its component test, so the shown symbols are greppable) and a three-sentence section in.agents/rules/testing-python.md.Not lessons
QueryResult.datais annotated for graph entities while rows carry scalars is a design observation, not a review comment; it is already recorded on the Jira card as a candidate for its own card.Branch note
Per the run instructions this PR is opened from
pha/conventionstodevelop.origin/developwas merged into the branch first, and the merged doc layer is byte-identical to develop, so the diff is the harvest commit alone. The merge surfaced one stale hunk: the branch re-added a paragraph develop already carries indev/guidelines/backend/python.md, which would have appeared twice; develop's copy was kept.dev/guidelines/git-workflow.mdroutes runtime-free diffs tostable; a reviewer who prefers that base can re-cut the single harvest commit onto astablebranch, as was done for #10614.Test plan
invoke docs.lint— markdownlint clean over 195 files (vale not installed locally)markdownlint-cli2run directly on the two edited markdown files underdev/and.agents/— 0 errors (the task only coversdocs/docs/)🤖 Generated with Claude Code
https://claude.ai/code/session_01LqxMdEvBzARrBMKaoh4W48