From d9475e7efb4c2a5a66d2572f5666b33f13670795 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Mon, 20 Jul 2026 17:38:02 +0100 Subject: [PATCH] docs(skill+pitfalls): four new defect classes from the 2043/2042 review round Pitfalls: runtime state and key material never enter commits (16), new views wire desktop AND mobile registries (17), retrofit columns on shipped stores need guarded ALTERs because the migration runner baselines pre-existing DBs without executing (18), scope deltas vs the issue are stated explicitly (19). Extended pitfall 1 with the house project auth-guard pattern and pitfall 15 with the answer-in-thread requirement. Skill: fold-first now requires in-thread replies per numbered item and bot review freshness checks; new scope-honesty subsection covers deferral notes, follow-up issues, and the git status runtime-artifact check. --- .../skills/taos-development-skill/SKILL.md | 19 ++++++++ docs/contributor-pitfalls.md | 44 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/.claude/skills/taos-development-skill/SKILL.md b/.claude/skills/taos-development-skill/SKILL.md index d2515b222..f4a96feef 100644 --- a/.claude/skills/taos-development-skill/SKILL.md +++ b/.claude/skills/taos-development-skill/SKILL.md @@ -240,6 +240,15 @@ A finding folded within hours merges the same day; a finding left while you open new PRs stalls the whole train behind it (the maintainer will not merge past an open finding, ever). +Folding means code AND a reply: answer every numbered item in the PR thread +with the commit that addresses it or a concrete rebuttal. Code pushed without +in-thread replies leaves the fold formally open and the verdict at HOLD. + +Bot review freshness is part of folding: check WHICH commit a bot actually +reviewed. A rate-limited or stale "SUCCESS" on an older head is not a pass - +after pushing fixes, re-trigger the review (for CodeRabbit: comment +`@coderabbitai review`). + ### Rebase cadence and the stale-base rule - dev moves fast. When your PR shows CONFLICTING, rebase onto current dev @@ -265,6 +274,16 @@ A close without a successor link reads as lost work and forces the maintainer into git forensics (this happened with #1927/#1924 - both were legitimate "landed via" closures that looked like data loss for hours). +### Scope honesty per slice + +- The PR body states exactly what it ships versus what its issue scopes. Any + deferred part gets an explicit deferral note AND a follow-up issue filed in + the same push; the parent issue never auto-closes on a partial slice. +- Before committing, `git status` must show only intended source changes - + runtime artifacts (anything under `data/`) never enter a commit, and a new + component that writes under `data/` gitignores its directory in the same PR + (pitfall 16). + ### One PR per slice - A fix and the test that proves it belong in ONE PR (pitfall 13 in diff --git a/docs/contributor-pitfalls.md b/docs/contributor-pitfalls.md index 54591933d..56bc1be74 100644 --- a/docs/contributor-pitfalls.md +++ b/docs/contributor-pitfalls.md @@ -12,6 +12,10 @@ session + CSRF, or the agent-token allowlist in `auth_middleware.py`) and a test asserting an unauthenticated or cross-principal caller gets 401/403. Example: PR #2036 added `GET /api/secrets/agent/{name}/github` with no guard, leaking installation IDs and repo names to any caller. +For project-scoped routes, copy the house guard verbatim: +`if not user.is_admin and user.user_id != p["user_id"]` followed by a masked +404 (see routes/projects.py). A bare 403 without the admin bypass both locks +out admins and leaks resource existence to other users (#2042). **2. Bind both directions on authenticated channels.** When a message or envelope arrives over an authenticated channel, verify BOTH @@ -37,6 +41,16 @@ and strips it. Tokens we only VERIFY are stored hashed; tokens we must PRESENT outbound are the only ones stored recoverable. Example: PR #2009 moved the GitHub App RSA key out of plaintext config.yaml. +**16. Never commit runtime state or key material.** +Anything a store or subsystem writes under `data/` at runtime is state, not +source. A PR that introduces a component writing under `data/` must add that +directory to `.gitignore` in the SAME PR, and `git status` must be checked for +runtime artifacts before every commit. Any private key that reaches a pushed +commit is burned: regenerate it, and keep it out of the target branch's history +(squash merge, or rewrite the branch). Example: `data/hub/identity.json` with +live signing/encryption keys entered one branch's history and was then +re-committed by a second PR (#2043 history, #2042). + ## Correctness **5. Never take element `[0]` of a collection that can hold more than one.** @@ -74,6 +88,12 @@ share flow), state it in the PR body and file a follow-up issue. Example: PR permissions. Wire the real value or do not add the parameter yet. Example: PR #2036 `handleSaveGrants`. +**17. A new view must be wired into every surface it has: desktop AND mobile.** +The desktop tab list and the mobile tab order are separate registries; updating +one and not the other ships a view that is unreachable on phones (#2042: +`TABS` updated, `mobileTabOrder` missed). Grep for every registry the sibling +views appear in and update all of them. + ## Store and schema **11. SCHEMA is the frozen v1; new columns and their indexes go in MIGRATIONS.** @@ -87,6 +107,18 @@ Running a data migration twice must be a no-op (existence checks, INSERT OR IGNORE, migrated-from markers) and there must be a test proving the second run changes nothing. Example done right: PR #2028 `test_migrate_idempotent`. +**18. Retrofitting a column onto an already-shipped store needs a guarded +ALTER, not just a MIGRATIONS entry.** +The migration runner baselines pre-existing databases at the latest version +WITHOUT executing the migrations (FOOTGUN #2 in `db_migrations.py`'s own +docstring), so a plain `MIGRATIONS = [(1, "ALTER TABLE ...")]` on a store that +already shipped is a silent no-op on every upgraded install: fresh DBs work, +upgraded DBs lose the feature at runtime. Use the guarded `_post_init` pattern +(PRAGMA table_info, ALTER only when the column is absent - see +`knowledge_store._migration_v1_add_user_id`) and add an upgrade test that +builds the PRE-change schema first. Fresh-DB tests are structurally blind to +this class (#2043: `peer_fingerprint`). + ## Process **13. A fix and its test belong in one PR.** @@ -110,3 +142,15 @@ Findings are gated on severity of content, not review state. Address each one (fix it or rebut it concretely in the thread); never merge past an open finding, and never let a "pass" verdict from a stale commit stand in for the current head. +A finding is closed only when it is ANSWERED IN-THREAD: reply to each numbered +item with the commit that addresses it or a concrete rebuttal. Pushing code +without replies leaves the finding open - the reviewer re-verifies blind and +the verdict stays HOLD (#2043 round two). + +**19. State scope deltas against the issue explicitly.** +If a PR ships less than its issue scopes - a subfeature dropped, owner-only +routes where the issue says peer-serving, a "live" view that fetches once - +say so in the PR body and file the follow-up issue in the same push. Never let +a partial slice close the parent issue. Silent shortfalls read as done, get +caught in review anyway, and cost a full extra round (#2042: community chat +and peer access absent with no deferral note).