docs(skill+pitfalls): three defect classes from the Library P1 review cycle#2079
Conversation
… cycle Pitfalls: tolerances do not test invariants (20), shell snippets in template literals must escape the interpolation sigil (21), conflict resolution reverts fixes when done mechanically (22). All three are drawn from real defects in this cycle, including a data-loss bug that a tolerance-based assertion kept green and 225 syntax errors from one unescaped construct. Skill: a verify-before-you-claim section covering tolerance assertions, mocks built from the caller's assumptions rather than the service contract, and compiling before opening a PR.
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? |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughContribution documentation adds verification and compilation checklists plus recurring pitfalls covering invariant assertions, external-service fixtures, template literals, and conflict resolution. ChangesContributor Guidance
Estimated code review effort: 1 (Trivial) | ~3 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 |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 286-288: Update the guidance around the “Typecheck or run the
thing” statement to reference the repository’s actual CI command, npm run build,
or use the neutral phrase “typecheck/build gate.” Remove the claim that the
executor directly runs tsc --noEmit, while preserving the instruction to
validate frontend changes before opening a PR.
🪄 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: 41dd6dc3-9176-4023-bc78-795a3f771365
📒 Files selected for processing (2)
.claude/skills/taos-development-skill/SKILL.mddocs/contributor-pitfalls.md
| - Typecheck or run the thing before opening the PR. A frontend change that does | ||
| not compile wastes a full review round, and the executor now gates on | ||
| `tsc --noEmit` for exactly that reason (pitfall 21). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the command claim with the actual CI gate.
The supplied repository contract runs npm run build, which executes tsc -b && vite build; tsconfig.json makes this non-emitting, but the executor is not shown running tsc --noEmit directly. Document the actual command or say “the typecheck/build gate” to avoid contributors validating a different check than CI.
Suggested wording
-- `tsc --noEmit` for exactly that reason (pitfall 21).
+- the frontend typecheck/build gate for exactly that reason (pitfall 21).📝 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.
| - Typecheck or run the thing before opening the PR. A frontend change that does | |
| not compile wastes a full review round, and the executor now gates on | |
| `tsc --noEmit` for exactly that reason (pitfall 21). | |
| - Typecheck or run the thing before opening the PR. A frontend change that does | |
| not compile wastes a full review round, and the executor now gates on | |
| the frontend typecheck/build gate for exactly that reason (pitfall 21). |
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 415: [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] 379: [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))
🤖 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 286 - 288,
Update the guidance around the “Typecheck or run the thing” statement to
reference the repository’s actual CI command, npm run build, or use the neutral
phrase “typecheck/build gate.” Remove the claim that the executor directly runs
tsc --noEmit, while preserving the instruction to validate frontend changes
before opening a PR.
Strengthens the mock guidance from a general principle into an enforceable rule. Scopes the danger to mocks of services we do not control, requires a captured real response over a hand-composed fixture, asks for one feature detecting integration test per contract, and rejects a follow-up issue as sufficient mitigation since it detaches the caveat from the code.
|
Strengthened the mock guidance after a challenge from Jay, who asked whether we should avoid mocks entirely or at least force a follow-up issue. I checked before answering: 372 of 535 Python test files use mocking, so a blanket ban is neither actionable nor right. Most of that is mocking our own internals or injecting errors nobody can produce on demand (a 500, a timeout, an ImportError), and none of it is what bit us. The dangerous class is narrow: mocks of external service contracts we do not control, whose fixtures were hand-composed from what the calling code expects. That fixture encodes a belief about someone else's API, and a wrong belief produces a green test that certifies the bug. All three failures in the #2062 cycle were that exact shape. Added as pitfall 23 with an enforceable hierarchy rather than a principle:
On the follow-up-issue suggestion specifically: I argued against it as the primary mitigation and said so in the text. With 260 open issues, filing one detaches the caveat from the code and functions as indefinite deferral. A comment in the file travels with the code and shows up in review; an issue does not. The reviewer-facing version is one question: where did this fixture come from? That is a question I could have asked on #2062 and did not. |
…achable Sibling services are not third parties. taOSmd's maintainer is on the A2A bus, so a contract question costs one message. Every mock failure in the 2062 cycle was a guess at something that was free to ask, and asking produced documented envelopes, a self-corrected stats key list, and a capabilities endpoint. Guessing is only justified when there is genuinely nobody to ask.
|
Revised again after Jay pointed out the obvious thing I had missed: for taOSmd there is no danger, because that is what the A2A bus is for. He is right, and it reframes the rule. taOSmd is not a third-party API, it is a sibling service whose maintainer is one message away. Every mock failure in the #2062 cycle was a guess at something that was free to obtain:
So pitfall 23 now splits by whether the contract has a reachable owner: Sibling services: ask, do not guess. Reverse-engineering a sibling's API from its source is a smell rather than diligence, because source tells you what it does today while the owner tells you what it guarantees. Contributors without bus access ask in the PR and the lead relays, which is what happened here. Genuine third parties (GitHub, OpenRouter, Reddit): capture, do not compose. There is nobody to ask, so record a real response instead of writing a fixture from what the caller expects. Both then keep one feature-detecting integration test, or mark the mock provisional in code. Worth stating plainly since it is my own miss: I spent a lot of this cycle verifying contracts against source when a bus message would have been faster and more authoritative. The rule now says so. |
Adds the routing rule that makes pitfall 23 actionable for contributors who are not on the A2A bus: a contract question goes to the commons repo (or taosmd directly, which is public), the lead relays to the owning agent, and the contributor escalates on the PR if it stalls. Marked explicitly as temporary scaffolding that retires when contributors can hold a taOS identity.
1 participant
Distils this cycle into the contributor docs. Every item is drawn from a real defect, not a hypothetical.
Pitfall 20: a tolerance is not a test of an invariant.
assert abs(after - before) <= 2ran green in #2062 while reprocess destroyed the user's original uploaded file (observedbefore=2, after=0). The same tolerance would have accepted a doubling. Assert equality and assert the resulting status.Pitfall 21: shell snippets inside template literals must escape
${. In #2077 a bash snippet in a JS template literal produced 225 TypeScript syntax errors from one cause, in one of four otherwise-clean files, which read like incoherent output rather than a single mechanical mistake. Generalises to any language sharing${...}with the shell, and is invisible without compiling.Pitfall 22: conflict resolution is a decision, not a mechanical act. Library P2/P3 both went CONFLICTING when P1 landed, and all four defects P1 fixed are reintroducible by a plausible-looking resolution.
Skill gains a short verify-before-you-claim section covering those three plus the mock-from-the-contract rule: a test whose fixture is built from the caller's assumption rather than the service's documented response certifies the bug. That happened three times in one PR cycle (invented
dbPathshape, un-nested poll response, tolerance assertion).Docs only.
Summary by CodeRabbit