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).