Skip to content

docs(skill+pitfalls): defect classes from the #2043/#2042 review round #2069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jaylfc merged 1 commit into from
Jul 20, 2026
+63 −0
Merged

docs(skill+pitfalls): defect classes from the #2043/#2042 review round #2069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .claude/skills/taos-development-skill/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +243 to +245

@coderabbitai coderabbitai Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Close every finding, not only numbered items.

Both files narrow the response requirement to numbered items, allowing unnumbered maintainer comments or bot findings to remain formally open.

  • .claude/skills/taos-development-skill/SKILL.md#L243-L245: change “every numbered item” to “every finding/comment.”
  • docs/contributor-pitfalls.md#L145-L147: apply the same wording so both contributor guidance sections enforce the same closure contract.
🧰 Tools
🪛 SkillSpector (2.3.11)

[warning] 399: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.

Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.

(Rogue Agent (RA2))

[error] 363: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).

Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.

(Tool Misuse (TM1))

📍 Affects 2 files
  • .claude/skills/taos-development-skill/SKILL.md#L243-L245 (this comment)
  • docs/contributor-pitfalls.md#L145-L147
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/taos-development-skill/SKILL.md around lines 243 - 245,
Update the folding guidance in .claude/skills/taos-development-skill/SKILL.md
(lines 243-245) to require answering every finding/comment, not only numbered
items. Apply the same wording change in docs/contributor-pitfalls.md (lines
145-147) so both guidance sections enforce closure of all maintainer comments
and bot findings.


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
Expand All @@ -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
Expand Down
44 changes: 44 additions & 0 deletions docs/contributor-pitfalls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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).

Comment on lines +44 to +53

@coderabbitai coderabbitai Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not imply that squash merging cleans up a leaked key.

A squash merge keeps the secret out of the target branch, but it does not remove it from the PR/source history or hosting retention. Require immediate invalidation/rotation of the exposed key and explicitly distinguish target-branch prevention from repository-wide history cleanup.

Suggested wording 
-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).
+Any private key that reaches a pushed commit is burned: immediately
+invalidate/rotate it, remove it from reachable repository history where
+possible, and ensure it is not copied into the target branch. Squash merging
+prevents propagation to the target branch but does not erase the PR/source
+history.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
**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).
**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: immediately invalidate/rotate it, remove it from reachable
repository history where possible, and ensure it is not copied into the target
branch. Squash merging prevents propagation to the target branch but does not
erase the PR/source history. 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`).
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/contributor-pitfalls.md` around lines 44 - 53, Update the guidance in
“16. Never commit runtime state or key material.” to state that exposed keys
must be immediately invalidated or rotated. Clarify that squash merging only
prevents the secret from entering the target branch; it does not remove the leak
from PR/source history or hosting retention, so repository-wide history cleanup
is separately required.

## Correctness

**5. Never take element `[0]` of a collection that can hold more than one.**
Expand Down Expand Up @@ -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.**
Expand All @@ -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.**
Expand All @@ -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).
Loading