docs: harvest review lessons from PRs reviewed 2026-08-21 to 2026-08-28 - #10441
docs: harvest review lessons from PRs reviewed 2026-08-21 to 2026-08-28#10441saltas888 wants to merge 3 commits into
Conversation
… to 2026-08-28 Weekly harvest run: mine the week's review threads for rules that generalize, verify each against the code, and route the durable ones into the internal-doc layer. +49/-44 across 10 files. Per-file provenance (review threads that produced each edit): - dev/guidelines/git-workflow.md — branch targeting reworked: perf/test-reliability changes join bug fixes on the oldest branch (fatih-acar asked "can we target stable?" on #10362 and #10364); behavior-changing fixes wait for the minor with a release note (polmichel on #10422); wide mechanical churn lands where files diverge least (saltas888's develop-base rationale on #10399, argued against ~18 bot notes citing a policy the repo never had). - AGENTS.md — changelog line aligned with the creating-changelog-entries skill's "housekeeping is not a catch-all" boundary, which maintainers applied three times this week (ogenstad on #10385, fatih-acar waiving on #10403 and removing a fragment as slop on #10432); spec-drift line broadened from "changes a figure" to "or reverses a decision" after ~12 stale storage-decision threads on #10431/#10429. - dev/guidelines/backend/python.md — branch on the enum member, not its string value (ajtmccarty on #10413, enum blocked by circular import on #10384); union-of-dataclasses over a struct of mode flags (polmichel drew both shapes on #10416); node.get_relationship() over getattr (ajtmccarty on #10404, twice). Paid for by compressing the version-features tail (min supported is 3.10; the 3.9/3.10 rows were dead weight). - .agents/rules/testing-python.md — "a regression guard must be shown to bite": the week's most repeated practice (fatih-acar proving a test "asserted nothing" on #10413, gmazoyer running a test against unfixed code on #10426, pa-lem reverting to count failing tests on #10424, xfail swallowing assertions on #10423); third-party class attributes added to the save/restore enumeration (#10363's unrestored _port_range monkey-patch). - .agents/rules/backend-component-design.md — dataclasses hold data; a collaborator makes it a component (ajtmccarty on #10262, twice). - dev/knowledge/backend/database-schema.md — start_session() escapes the caller's transaction (verified in database/__init__.py; found the hard way on #10364 where permission writes survived a rollback); retry table compressed to pay for it. - docs/AGENTS.md — UI button/label text added to the verify-claims enumeration (#10437's "Regenerate" button that does not exist). - dev/knowledge/frontend/react.md + frontend/app/AGENTS.md — effect retry trap under the app-wide retry:false client (pa-lem's wedge-the-app finding on #10394), with the router entry as the load-trigger. - dev/guides/frontend/writing-e2e-tests.md — rot sweep: skip-reason example carried an issue ID that code-doc-style forbids in source. Deliberately not codified: changelog fragment content (open PR #10400 already adds .agents/rules/changelog-entries.md covering it); the temporal from <= $at edge-closure guard (contested: added on #10435, rebutted as impossible-at-now on #10438); the TRY400 traceback-vs-task-log debate on #10220 (unresolved).
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
…oundary, and the union example Three review fixes: the behavior-changing-fix bullet now defaults to develop instead of mandating it, so the patch-release hotfix path survives; the AGENTS.md housekeeping line states the skill's user-visibility criterion as the rule and demotes the categories to typical cases; the union-of-dataclasses example defines its case types so it is self-contained and no longer reuses the bad block's name.
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
…ile's own dataclass rule
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 harvest PR: all 10 diffs update developer guidelines, AGENTS.md files, and knowledge docs to codify review lessons. No production code, tests, schema, config, or behavior change -- only descriptive/documentation text.
Re-trigger cubic
Why
Reviewers repeated the same findings across this week's PRs — branch-targeting questions came up on five PRs (including ~18 bot notes on one PR citing a "release-vehicle policy" the repo's own guideline contradicts), "prove the regression test bites" was the resolution on four, and maintainers rejected
housekeepingchangelog fragments three times against what the rootAGENTS.mdline said. This is the weekly harvest run: it mines the week's review 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 the changelog-content rule pending in #10400 (
.agents/rules/changelog-entries.md) — the week's four human comments on changelog wording are deliberately left to that PR.PRs I learned from
Lessons in this PR trace to review threads on:
developwith a release-note flag; saltas888 defended adevelopbase for wide reformatting on divergence-cost grounds against repeated bot notes → the git-workflow Branch Strategy section now states all three casesAGENTS.mdline said "internal maintenance still gets ahousekeepingfragment" while maintainers waived or deleted exactly those fragments; the line now defers to thecreating-changelog-entriesskill's "not a catch-all / only when a user could notice" boundaryAGENTS.mdspec-drift rule covered changed figures but not reversed decisions, so it now names both"DELETING"; the existing enum section covered typing but not the comparison sidepython.mdrulenode.get_relationship(name)overgetattr(node, name), asked twice and then swept across three modules; added to the existinggetattrsectiontesting-python.mdrule, plus the xfail-swallows-assertions trapstart_session()builds a session straight from the driver, so writes escape any caller transaction (verified indatabase/__init__.py; the PR found permission writes surviving a rollback); recorded next to the transaction-retry knowledgeretry: falseREST client, an effect whose dependencies are all stable after a failure never retries; newreact.mdsection with a router-entry load-triggerAlso read, with nothing durable to harvest (no in-window substantive threads, lessons already codified, PR-local design calls, or lessons owned by an open PR): #10439, #10440, #10438, #10396, #10434, #10416's remaining threads, #10435, #9807, #10405's pre-ci threads (fixed in that PR), #10424, #10427, #10431/#10429's workflow-security findings (PR-local, PoC drafts), #10231, #10246, #10262's remaining threads, #10421 (its findings were fixed in-PR), #10412, #10030, #10220, #10411, #10365, #10374, #10408, #10398, #10377, #10315, #10397, #10406, #10409, #10407, #10403, #10372, #10360, #10284, #10376, #10380, #10400, #10393, #10387, #10379, #10382, #10339, #10381, #10358, #10383, #10368, #10354, #10375, #10361, #10371, #10370, #10334 (docs accuracy pass owned by that open PR), #10359, #10367, #10348, #10362, #10422, #10423, #10426, and the bot merge/dependabot PRs in the window.
Notable non-edits, recorded deliberately:
.agents/rules/changelog-entries.mdcovering exactly this — no duplicate here.from <= $atedge-closure guard is contested: gmazoyer added it on fix: let an SSO user sign in after their account is deleted (closes #10304) #10435, ajtmccarty rebutted it as impossible for a migration running at the current timestamp on database migration for agnostic fields on deleted aware objects #10438. Not codified; worth watching.What changed
Documentation layer only, 10 files, +49/−44 — the deletions paid for the additions:
dev/guidelines/git-workflow.md(145→150, range 100–400): Branch Strategy reworked — perf/test-reliability changes join bug fixes on the oldest maintained branch; behavior-changing fixes wait for the minor with a release-note flag; wide mechanical churn lands where files diverge least; the tooling bullet's boundary corrected from "no source-code changes" to "no runtime source changes (…, test-only)"AGENTS.md: the two Always-Do line edits above (housekeeping boundary; reversed decisions)dev/guidelines/backend/python.md(464→462, over its 400 cap but net −2): three rules added, paid for by compressing the version-features tail (minimum supported is 3.10, so the 3.9/3.10 table rows were dead weight).agents/rules/testing-python.md(82→86): regression-guard-must-bite section; class attributes in the save/restore list.agents/rules/backend-component-design.md(129, +1 sentence): the dataclass boundarydev/knowledge/backend/database-schema.md(397→391, back inside its 200–400 range): session-escapes-transaction paragraph; retry table compressed to a sentencedocs/AGENTS.md,dev/knowledge/frontend/react.md+frontend/app/AGENTS.mdrouter entry,dev/guides/frontend/writing-e2e-tests.md(rot sweep: issue ID in a skip-reason example, forbidden by code-doc-style)What stayed the same: no production code, no tests, no user-facing docs pages, no schema changes.
How to review
Every edit cites its review threads in the commit message. The judgement calls worth a second pair of eyes: the git-workflow bullets encode fatih-acar's two "target stable?" asks and saltas888's divergence argument on #10399 — flag it if the release-vehicle policy reads differently; and the
AGENTS.mdhousekeeping line now defers to the skill's narrower boundary, encoding this week's three maintainer rejections over the older, broader wording.How to test
The lint's globs cover
docs/docs/**only — this diff touches no files there (AGENTS.mdfiles are excluded by the repo's markdownlint config), so it passes trivially; ran markdownlint-cli2 locally, 0 errors.Impact & rollout
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_019BZYhKMNyKnTxvwJZpXbEC
Generated by Claude Code