docs(skill+pitfalls): defect classes from the #2043/#2042 review round#2069
Conversation
…ew 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.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe PR expands contributor guidance with stricter PR review and scope rules, plus documented pitfalls covering authorization guards, runtime artifacts and secrets, desktop/mobile view registration, and guarded schema upgrades. ChangesContributor guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by hy3:free · Input: 64.1K · Output: 2.1K · Cached: 136.4K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In @.claude/skills/taos-development-skill/SKILL.md:
- Around line 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.
In `@docs/contributor-pitfalls.md`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 16e0b710-e004-43bf-b3b4-18259c957b35
📒 Files selected for processing (2)
.claude/skills/taos-development-skill/SKILL.mddocs/contributor-pitfalls.md
| 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. |
There was a problem hiding this comment.
🎯 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.
| **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). | ||
|
|
There was a problem hiding this comment.
🔒 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.
| **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.
1 participant
Distills this round of deep reviews into the contributor docs so the classes stop recurring:
data/-writing components gitignore their dir in the same PR; pushed keys are burned (fromdata/hub/identity.jsonappearing in feat(hub): friend-accept creates contact row and peer-link handshake #2043's history and again in feat(community): add collaborator community view (stats + leaderboard + kanban) #2042)._post_initALTER pattern plus an existing-DB upgrade test; a plain MIGRATIONS entry is a silent no-op on upgraded installs due to baseline-at-latest (reproduced in feat(hub): friend-accept creates contact row and peer-link handshake #2043).is_adminbypass + masked 404) copied verbatim; bare 403s leak existence.Docs only, no code changes.
Summary by CodeRabbit