docs: harvest review lessons from PRs reviewed 2026-08-14 to 2026-08-21 - #10360
Open
saltas888 wants to merge 2 commits into
Open
docs: harvest review lessons from PRs reviewed 2026-08-14 to 2026-08-21#10360saltas888 wants to merge 2 commits into
saltas888 wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XdeNcpTLZ15aUaz6H5uJVd
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
saltas888
marked this pull request as ready for review
August 21, 2026 09:19
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XdeNcpTLZ15aUaz6H5uJVd
Contributor
There was a problem hiding this comment.
0 issues found across 3 files (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. This PR codifies recent review feedback into the project's internal documentation and developer guidelines, covering theming, changelog discipline, and documentation standards.
Re-trigger cubic
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
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.
Why
Reviewers repeated the same findings across this week's PRs — most of all on the dark-theme token migration, where the same three findings (token swap ≠ no-op, WCAG demotions, missing changelog fragments) recurred across ten PRs. This is the weekly harvest run: it mines those threads for rules that generalize, verifies each against the code, and routes the durable ones into the internal-doc layer.
Non-goals: no production code or test changes; no duplication of edits pending in #10030 (
pha/conventions, ondevelop) — in particular the changelog-skill naming refinements already fixed there.PRs harvested from
Lessons in this PR trace to review threads on:
infrahubctlclaims against the pinnedpython_sdkcommit, never bump the pin to make docs resolve (two bot false positives rebutted withgit -C python_sdk show); GitHub's renderer does not follow repo symlinks in relative links (thespecs→dev/specscase)developwork; config/docs-only tooling may targetstableAGENTS.md"changelog fragment for any user-visible change / none for pure internal changes" lineAlso read, with nothing durable to harvest (no in-window substantive threads, lessons already codified, or PR-local design calls): #10312's transaction threads, #10353, #10358, #10354, #10339, #10334 (its maintainer corrections land in that open docs PR), #10341, #10348, #10347, #10331, #10335, #10290, #10250, #10238, #10298, #10299, #10302, #10316, #10317, #10287, #10288, #10291, #10281, #10256, #10231, #10266, #10270, #10030, #10178, #10232, #10135, #10258, #9451, #9965, #10164, and the bot merge/submodule/dependabot PRs in the window.
Notable non-edits, recorded deliberately:
to IS NULLon active-edge reads recurred on keep earliest create user ascreated_byduring merge #10281 hours after docs: harvest review lessons from PRs reviewed 2026-08-07 to 2026-08-14 #10259 codified exactly that rule indev/knowledge/backend/database-schema.md— the comment predates the doc's merge, so no further edit; worth watching whether it recurs now the rule is live.coreimportingRelationshipHop/ReachedPathfrominfrahub.graphql.analyzer) is unresolved with no landed fix, andinfrahub.corealready imports frominfrahub.graphqlin 8 places onstable— writing a "never" rule would contradict the tree, so this is flagged here instead of codified.deprecation_reasononis_isolated.What changed
Grouped by intent (documentation layer only, 10 files, +42/−7):
dev/guidelines/frontend/styling.md, new "Theme tokens" section): the five rules above, written as imperatives with the concrete failure shapes reviewers kept describing. The token system lives ondevelop; this lands onstableand reaches it through the routine stable→develop syncs, same as the rest of the harvest.AGENTS.md, Always Do): one line — towncrier fragment for any user-visible change, UI styling included; none for pure internal changes. The rule already exists in thecreating-changelog-entriesskill; this is the load-trigger that was missing (≥8 repeat comments this week).docs/AGENTS.md): the existing verify-claims bullet now names the pinned-submodule check for SDK features; new terminology line — "object", not "node", in user-facing text.dev/guidelines/markdown.md): relative links must not route through repo symlinks..agents/rules/code-doc-style.md): never describe what the code does not do.dev/guidelines/documentation.md): don't reference steps by number.dev/guidelines/git-workflow.md): the lint/tooling split, codifying the precedent argued on chore(backend): enforce ruff TRY400 so error logs carry tracebacks #10220.dev/knowledge/backend/schema-definitions.md,database-schema.md): deprecations name the removal version; instance-level self-loops unsupported.dev/knowledge/frontend/design-system.md): five stale "Migrated in #NNNN" citations removed —develop's copy already dropped them;stable's diverged copy still carried them.What stayed the same: no production code, no tests, no user-facing docs pages, no schema changes.
How to review
Every edit cites a real review thread (see the commit message for the per-file mapping). The judgement calls worth a second pair of eyes: the git-workflow branch-targeting bullet encodes @saltas888's rebuttal on #10220 that no maintainer overruled — flag it if the release-vehicle policy reads differently; and the self-loop line in
database-schema.mdencodes @ajtmccarty's statement on #10312.File sizes after the change: every touched file is inside its
repository-organization.mdrange (styling.md 106/100–400, database-schema.md 390/200–400 — closest to its cap).How to test
Ran locally: 0 errors (AGENTS.md files are excluded by the repo's markdownlint config; Vale not available in the sandbox; no
docs/docspages touched).Impact & rollout
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01XdeNcpTLZ15aUaz6H5uJVd
Generated by Claude Code