fix(core): resolve conflicts and drift between review skills#709
Draft
superzadeh wants to merge 14 commits into
Draft
fix(core): resolve conflicts and drift between review skills#709superzadeh wants to merge 14 commits into
superzadeh wants to merge 14 commits into
Conversation
…view-skill # Conflicts: # .agents/skills/commit-push-pr/SKILL.md # plugins/core/skills/commit-push-pr/SKILL.md
Refresh /simple-review and /in-depth-review from latest local Claude commands. Apply markdownlint --fix and oxfmt to normalize formatting (blank lines around fences/lists/headings, emphasis style, table alignment) and add cursorrules, luxon, unvalidated, reviewee to the cspell dictionary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view-skill # Conflicts: # .agents/skills/babysit-pr/SKILL.md # .agents/skills/babysit-pr/scripts/_sentinel.sh # .agents/skills/commit-push-pr/SKILL.md # cspell.json # plugins/core/.claude-plugin/plugin.json # plugins/core/skills/babysit-pr/SKILL.md # plugins/core/skills/babysit-pr/scripts/_sentinel.sh # plugins/core/skills/commit-push-pr/SKILL.md
…eview Carry the simple-review noise guardrails and comment-body formatting rules into in-depth-review so both skills produce equally terse PR comments: - Add the "concrete, current, product-visible cost" litmus test to the severity rubric (binding for all agents). - Replace the comment-body template with the terse budget (<=60 words + code block), word caps on title/point/why-it-matters, and the drop-when-rephrasing rule. - Flip the Example/context fenced block to opt-out by default and add the no-bulleted-lists / no-prose-preamble / no-trailing-addenda rules. - Surface the terseness guardrail in the Rules summary. Regenerated the .agents/skills copy to match the plugins/core source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Align in-depth-review and simple-review with Anthropic's Agent Skills best practices: - Progressive disclosure: extract the conditional "Cross-repo evidence policy" and end-of-run "Posting an anchored PR review" sections into one-level-deep references/*.md, loaded on demand. SKILL.md bodies drop under the 500-line guideline (in-depth 611->491, simple 614->472), with prominent pointers left in place. - Descriptions: rewrite both frontmatter descriptions to state what the skill does plus explicit "Use when" triggers and a simple<->in-depth selection cue, so Claude can pick the right skill. Kept Claude-Code-first by design (multi-agent dispatch, AskUserQuestion gates). Regenerated the .agents/skills copies to match the plugins/core source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reconcile duplicated and conflicting guidance across the in-depth-review and simple-review skills (and their generated .agents copies). Conflicts resolved: - simple-review told the agent both to list and to never list convention sources in the Summary; drop the "list in the Summary" instruction so it matches the Summary section's "methodology is noise" stance (and in-depth-review's behavior). - Align the author-mode Execute step on `TodoWrite`; in-depth-review referenced a non-existent `TaskCreate` tool. - Clarify simple-review's "no /tmp persistence" note, which contradicted the posting reference that writes a transient payload file. Rubric drift reconciled (ported simple-review's extra checks up into in-depth-review so the shared checklists match): - Security: numeric min/max bounds and overflow/400-vs-500 handling. - Database: Mongoose Number precision, dual-write canonicalization, backfill. - Conventions: service test-structure specifics. - Engineering: route consumer-break suspicions through the cross-repo policy instead of asserting them.
Contributor
📝 WalkthroughWalkthroughThis PR introduces two new code review skills— ChangesCode Review Skills and Plugin Infrastructure
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
|
View your CI Pipeline Execution ↗ for commit 10f1556
☁️ Nx Cloud last updated this comment at |
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM
This is a documentation-only PR that reconciles known contradictions between two review skill files. The stated conflicts are properly resolved, reference files exist in all expected locations, and the .agents/ copies match their plugins/core/ sources. The architectural differences between the two skills (e.g., text-prompt vs structured picker for user gates) are intentional given their different complexity levels, not unresolved drift.
What this PR does
Reconciles conflicts and drift between the in-depth-review and simple-review skills by resolving three outright contradictions (convention-source listing, TaskCreate → TodoWrite, /tmp wording) and porting missing rubric checks from simple-review into in-depth-review. Also bumps version sentinels from core@3.4.1 to core@3.5.0 in babysit-pr and commit-push-pr skills, and regenerates .agents/skills copies.
Tag @mendral-app with feedback or questions. View session
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/core/skills/simple-review/references/posting-pr-review.md (1)
93-93: 💤 Low valueReplace "addenda" with a clearer English phrase.
Line 93 uses "No trailing addenda" — LanguageTool suggests this foreign term may be unclear to all readers. Consider "No trailing additions," "No trailing extra notes," or "No appendices."
🤖 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 `@plugins/core/skills/simple-review/references/posting-pr-review.md` at line 93, Replace the unclear phrase "No trailing addenda" with a clearer English alternative (e.g., "No trailing additions" or "No trailing extra notes") in the markdown entry that currently reads "No trailing addenda. \"If feature X isn't shipped yet…\", \"Note that this also affects…\", \"While you're here…\" — these belong in the in-chat synthesis, not on the posted comment."; update that exact sentence (search for the string "No trailing addenda") so the surrounding guidance remains identical but uses the clearer wording.plugins/core/skills/simple-review/SKILL.md (1)
72-72: 💤 Low valueUse a more specific number or phrase than "small number."
Line 72 says "more than a small number of commits" — this is vague. Replace with "more than a few" or specify a threshold (e.g., "more than 5 commits").
🤖 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 `@plugins/core/skills/simple-review/SKILL.md` at line 72, Replace the vague phrase "more than a small number of commits" in the sentence "5. `HEAD` is more than a small number of commits ahead of `origin/${base}` on Path A." with a clearer term or explicit threshold (e.g., "more than a few commits" or "more than 5 commits") so the condition in SKILL.md is unambiguous; update that exact bullet item text to use the chosen phrasing.
🤖 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.
Nitpick comments:
In `@plugins/core/skills/simple-review/references/posting-pr-review.md`:
- Line 93: Replace the unclear phrase "No trailing addenda" with a clearer
English alternative (e.g., "No trailing additions" or "No trailing extra notes")
in the markdown entry that currently reads "No trailing addenda. \"If feature X
isn't shipped yet…\", \"Note that this also affects…\", \"While you're here…\" —
these belong in the in-chat synthesis, not on the posted comment."; update that
exact sentence (search for the string "No trailing addenda") so the surrounding
guidance remains identical but uses the clearer wording.
In `@plugins/core/skills/simple-review/SKILL.md`:
- Line 72: Replace the vague phrase "more than a small number of commits" in the
sentence "5. `HEAD` is more than a small number of commits ahead of
`origin/${base}` on Path A." with a clearer term or explicit threshold (e.g.,
"more than a few commits" or "more than 5 commits") so the condition in SKILL.md
is unambiguous; update that exact bullet item text to use the chosen phrasing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a65ad8f7-657d-49c3-9a7b-18946812c9c8
⛔ Files ignored due to path filters (9)
.agents/skills/babysit-pr/SKILL.mdis excluded by!.agents/skills/**.agents/skills/babysit-pr/scripts/_sentinel.shis excluded by!.agents/skills/**.agents/skills/commit-push-pr/SKILL.mdis excluded by!.agents/skills/**.agents/skills/in-depth-review/SKILL.mdis excluded by!.agents/skills/**.agents/skills/in-depth-review/references/cross-repo-evidence.mdis excluded by!.agents/skills/**.agents/skills/in-depth-review/references/posting-pr-review.mdis excluded by!.agents/skills/**.agents/skills/simple-review/SKILL.mdis excluded by!.agents/skills/**.agents/skills/simple-review/references/cross-repo-evidence.mdis excluded by!.agents/skills/**.agents/skills/simple-review/references/posting-pr-review.mdis excluded by!.agents/skills/**
📒 Files selected for processing (12)
cspell.jsonplugins/core/.claude-plugin/plugin.jsonplugins/core/README.mdplugins/core/skills/babysit-pr/SKILL.mdplugins/core/skills/babysit-pr/scripts/_sentinel.shplugins/core/skills/commit-push-pr/SKILL.mdplugins/core/skills/in-depth-review/SKILL.mdplugins/core/skills/in-depth-review/references/cross-repo-evidence.mdplugins/core/skills/in-depth-review/references/posting-pr-review.mdplugins/core/skills/simple-review/SKILL.mdplugins/core/skills/simple-review/references/cross-repo-evidence.mdplugins/core/skills/simple-review/references/posting-pr-review.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ClipboardHealth/cbh-core(manual)
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Follow-up to #649. The two new review skills (
in-depth-reviewandsimple-review) share a large, intentionally-duplicated rubric. A review for duplicated/conflicting content surfaced three outright contradictions and several places where the duplicated checklists had already drifted apart. This PR reconciles them. It targets the #649 branch so the fixes can land with that PR.What changed
Conflicts resolved
simple-reviewcontradicted itself on convention sources. The Conventions lens said "In the Summary, list the convention sources you consulted" while the Summary section said "Do not … list the convention sources you consulted — that metadata is noise." Dropped the "list in the Summary" instruction so it matches the Summary section (andin-depth-review's "methodology is noise" stance).TaskCreate→TodoWrite.in-depth-review's author-mode Execute step referenced a non-existentTaskCreatetool;simple-reviewusedTodoWrite. Aligned onTodoWrite./tmpwording.simple-reviewsaid "no/tmp/*persistence needed" but its posting reference writes/tmp/simple-review-payload.json. Clarified that review state is in-context while the posting step writes one transient payload file.Rubric drift reconciled (ported
simple-review's extra checks up intoin-depth-review, the higher-stakes path, so the shared checklists match)min/maxbounds +Number.MAX_SAFE_INTEGER/BigInt/negative handling and 400-vs-500 behavior.Numberprecision note, dual-write canonicalization, backfill for new fields.createTestContext/tearDown,beforeAll/afterAllfor GET).Generated
.agents/skillscopies regenerated vianode --run sync-ai-rules.Not included (left for a follow-up decision)
posting-pr-review.mdandcross-repo-evidence.mdreference files are ~95% / partially duplicated across the skills and have also drifted. They could be consolidated into shared references, but that's a structural change deferred for now.Validation
oxfmt --check— pass (4 files)markdownlint-cli2— 0 errorscspell— pass (all ported vocabulary already existed insimple-review).agentscopies verified identical for both skillsverifyruns a repo-wideoxlintthat fails on pre-existing lint errors in unrelated TypeScript packages (mongo-jobs,notifications,util-ts, …). This is a markdown-only change;nx affectedCI does not lint those projects.Generated by Claude Code