-
-
Notifications
You must be signed in to change notification settings - Fork 638
Reorganize PR testing docs into .claude/agents/ #2112
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.
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
Conversation
WalkthroughUpdate: two Markdown docs under .claude/agents had internal relative links changed to point to new locations under ../docs/ (testing-build-scripts.md and master-health-monitoring.md). No functional code changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 - PR #2112SummaryThis PR reorganizes PR testing documentation from .claude/docs/ to a new .claude/agents/ directory. The reorganization follows a logical structure and improves discoverability. ✅ What Looks Good
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.claude/agents/pr-testing-agent.md (2)
155-161: Add language specification to fenced code block. Line 155 begins a fenced code block without specifying the language, which reduces syntax highlighting clarity.Apply this diff to add the language identifier:
-``` +```bash ❌ CANNOT test build scripts in isolated workspace ⚠️ MANUAL TESTING REQUIRED before merge:
354-364: Add language specification to fenced code block. Line 354 has a code fence without language specification. This block contains status message template text.Apply this diff to add the language identifier:
-``` +```markdown ⚠️ UNTESTED - Requires environment not available in Conductor workspace.claude/agents/pr-testing-guide.md (2)
56-56: Add language specifications to fenced code blocks. Multiple code blocks throughout the guide lack language identifiers, which reduces syntax highlighting and readability. Most should bebash(for shell examples),markdown(for template examples), or left unspecified if deliberately generic.Representative examples of fixes needed:
-``` +"```bash "I changed lib/react_on_rails/helper.rb. According to the PR Testing Agent,and
-``` +"```markdown "Based on PR Testing Agent Section 1, you need:Also applies to: 70-70, 82-82, 92-92, 101-101, 110-110, 119-119, 130-130, 160-160, 191-191, 204-204, 229-229, 264-264, 288-288, 295-295, 302-302, 309-309
555-555: Add language specifications to remaining fenced code blocks. Lines 555, 599, 628, and 667 in the "Communicating Test Status" and "Real-World Scenarios" sections have code blocks without language specifications. These should bebashfor shell commands ormarkdownfor message templates.Examples:
-``` +"```markdown Fix webpack CSS modules configuration for Shakapacker 9and
-``` +"```bash $ bundle exec rspec spec/react_on_rails/helper_spec.rbAlso applies to: 599-599, 628-628, 667-667
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.claude/agents/pr-testing-agent.md(1 hunks).claude/agents/pr-testing-guide.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.claude/agents/pr-testing-guide.md
[style] ~32-~32: Try using a synonym here to strengthen your wording.
Context: ...h pr create`) - Responding to PR review comments about testing - CI failures occur and n...
(COMMENT_REMARK)
[grammar] ~494-~494: Ensure spelling is correct
Context: ...tion tests: rake run_rspec:dummy ### Environment Limitations **Mark any testing blocked...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
.claude/agents/pr-testing-guide.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
110-110: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
130-130: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
204-204: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
229-229: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
264-264: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
288-288: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
295-295: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
309-309: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
555-555: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
599-599: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
628-628: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
667-667: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.claude/agents/pr-testing-agent.md
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (13)
.claude/agents/pr-testing-agent.md (5)
1-17: Clear, well-organized PR testing agent definition. The structure effectively separates role/principles from detailed requirements, with comprehensive cross-references to supporting documentation. The Quick Reference section correctly uses relative paths from the new.claude/agents/location (e.g.,../docs/for supporting docs,../../for repo-root files).
18-46: Excellent articulation of skeptical stance and communication standards. The Agent Behavior section clearly defines the default mindset and provides concrete DO/DON'T examples that guide implementation. This will help Claude Code apply the agent correctly and communicates expectations clearly to developers.
106-162: Build Configuration Changes section provides critical guidance on silent failures. The MANDATORY manual testing checklist (steps 1-6) and explicit reference to real examples intesting-build-scripts.mdeffectively communicate why build testing is non-negotiable. The Conductor workspace limitation call-out (lines 153-161) is particularly valuable for developers working in isolated environments.
331-385: Environment limitations section is pragmatic and helpful. The distinction between what CAN and CANNOT be tested in Conductor workspace (lines 335-351) sets realistic expectations, and the Ruby 3.4 + OpenSSL workaround (lines 366-384) shows awareness of real pain points developers face. The UNTESTED label template is actionable.
386-470: Seven success criteria effectively operationalize the agent's core principles. Criterion #4 (Testing Documentation) with the markdown template (lines 423-442) is particularly valuable—it makes the skeptical, transparent approach concrete. Criterion #5 (Clear Communication) reinforces UNTESTED labeling patterns defined earlier, creating a consistent vocabulary for testing status throughout the PR lifecycle..claude/agents/pr-testing-guide.md (8)
1-24: Excellent companion guide with clear practical orientation. The structure effectively supports the PR Testing Agent by showing when and how to use it with Claude Code. Cross-references to the agent definition and supporting docs are correct and help users navigate the documentation ecosystem.
45-62: Excellent invocation examples for Claude Code integration. The prompts at lines 54-62 show the specific pattern: reference the agent document directly (.claude/agents/pr-testing-agent.md) and request specific sections. This pattern will help Claude Code apply the agent consistently across different prompts.
64-124: Comprehensive workflow examples with realistic Claude Code interactions. The progression from Quick Start → Common Workflows → Specific Section Referencing establishes a clear learning path. Each workflow includes both the prompt pattern and relevant section reference, making it easy for developers to replicate successful invocations.
145-282: Complete lifecycle example effectively demonstrates agent usage in context. The walk-through from code change → testing guidance → execution → validation → documentation → CI investigation shows how developers move through the PR lifecycle while consulting the agent. The realistic Claude Code responses (e.g., lines 165-189, 209-221) help set expectations for what guidance developers can expect.
421-508: Pre-Merge Testing Checklist effectively operationalizes agent requirements. The organization by file type directly parallels the PR Testing Agent's section structure, making it easy for developers to find relevant items. The MANDATORY emphasis on build config testing (line 455-458) and environment limitations call-out (lines 494-508) reinforce the agent's core principles about testing comprehensiveness and transparency.
510-594: Communication templates provide clear structure for testing status updates. The PR comment template (lines 514-549) uses sections (Verified Locally, Requires Manual Verification, Cannot Test, CI Status) and visual indicators (✅,⚠️ , ❌) to make testing status scannable. The commit message template (lines 551-570) shows how to document testing in git history. Both effectively support the agent's emphasis on explicit, honest communication about what was/wasn't tested.
572-668: Real-world scenarios effectively teach the skeptical approach through contrast. The three scenarios (RSpec failures, dependency updates, CI reproduction) show the progression from insufficient → required documentation. Scenario 2's emphasis on mandatory build testing (clean install, build, prepack, yalc publish) reinforces the Build Configuration Changes guidance from the agent. Scenario 3's acknowledgment of environment limitations and proposed testing steps models how to handle "can't reproduce" situations transparently.
344-419: Integration guidance clearly positions the agent within the broader development workflow. The explanation of how it complements (but doesn't replace) the code-reviewer agent and CI checks prevents role confusion. The integration points (pre-commit, pre-push, during CI, pre-merge) create a natural progression, and the workflow example shows how to consult the agent across the PR lifecycle.
Creates a clearer structure following the /commands pattern: .claude/ ├── agents/ # Agent definitions (NEW) │ ├── pr-testing-agent.md │ └── pr-testing-guide.md ├── commands/ # Slash commands └── docs/ # Supporting documentation Benefits: - Clear separation: agents vs commands vs supporting docs - Consistent with /commands structure - Easy to find agent definitions - Future-proof for more agents Changes: - Created .claude/agents/ directory - Moved pr-testing-agent.md from docs/ to agents/ - Moved pr-testing-guide.md from docs/ to agents/ - Updated all cross-references to use ../docs/ paths - Updated prompt examples to use .claude/agents/ path No duplicate files remain - clean move from docs/ to agents/ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
22d9a0e to
9480326
Compare
Code Review - PR #2112SummaryThis PR reorganizes the PR testing documentation by creating a dedicated ✅ Strengths1. Clear organizational structure
2. Comprehensive cross-reference updates
3. Low-risk change
4. Future-proof design
✅ VerifiedCross-reference accuracy:
Updated paths:
📋 Review Checklist
🎯 RecommendationLGTM - Ready to merge This is a clean, well-executed documentation reorganization that:
No changes needed. 📝 Minor Note (optional enhancement)Consider adding a brief README.md in
This is not required for this PR but could help future contributors. Could be addressed in a follow-up if desired. Nice work on maintaining clean documentation structure! 🚀 |
The script/convert tool is used by CI 'minimum' dependency tests to downgrade to minimum supported versions (Ruby 3.2, Node 20, Shakapacker 8.2.0, React 18). After the monorepo restructure (PR #2112), the script was failing because it was looking for spec/dummy/package.json at the old root location, but these files are now under react_on_rails/spec/dummy/ and react_on_rails_pro/spec/dummy/. Updated all file path references: - spec/dummy/package.json → react_on_rails/spec/dummy/package.json - spec/dummy/config/shakapacker.yml → react_on_rails/spec/dummy/config/shakapacker.yml Fixes CI failure: https://github.com/shakacode/react_on_rails/actions/runs/19624872033/job/56191736323 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
### Summary
Reorganizes the PR testing documentation into a clearer directory
structure, following the established pattern of `.claude/commands/`.
### Changes
**New structure:**
```
.claude/
├── agents/ # Agent definitions (NEW)
│ ├── pr-testing-agent.md
│ └── pr-testing-guide.md
├── commands/ # Slash commands
└── docs/ # Supporting documentation
├── analysis/
├── testing-build-scripts.md
├── master-health-monitoring.md
└── ...
```
**What changed:**
- Created `.claude/agents/` directory
- Moved `pr-testing-agent.md` and `pr-testing-guide.md` from `docs/` to
`agents/`
- Updated all cross-references in both files (now point to `../docs/`
for supporting docs)
- Updated prompt examples to use new path
(`.claude/agents/pr-testing-agent.md`)
### Benefits
- **Clear separation**: Agents vs commands vs supporting documentation
- **Consistent structure**: Follows the `.claude/commands/` pattern
- **Easy to find**: Agent definitions have their own directory
- **Future-proof**: Ready for additional agents
- **No functionality changes**: Only file moves and reference updates
### Pull Request checklist
- [x] Add/update test to cover these changes - N/A, documentation
reorganization
- [x] Update documentation - Updated all cross-references
- [x] Update CHANGELOG file - N/A, documentation reorganization
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Documentation**
* Updated PR Testing Agent and PR Testing Guide: fixed several relative
links and references to point to the new documentation locations and
corrected prompt/example references. No functional changes.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Claude <noreply@anthropic.com>
### Summary
Reorganizes the PR testing documentation into a clearer directory
structure, following the established pattern of `.claude/commands/`.
### Changes
**New structure:**
```
.claude/
├── agents/ # Agent definitions (NEW)
│ ├── pr-testing-agent.md
│ └── pr-testing-guide.md
├── commands/ # Slash commands
└── docs/ # Supporting documentation
├── analysis/
├── testing-build-scripts.md
├── master-health-monitoring.md
└── ...
```
**What changed:**
- Created `.claude/agents/` directory
- Moved `pr-testing-agent.md` and `pr-testing-guide.md` from `docs/` to
`agents/`
- Updated all cross-references in both files (now point to `../docs/`
for supporting docs)
- Updated prompt examples to use new path
(`.claude/agents/pr-testing-agent.md`)
### Benefits
- **Clear separation**: Agents vs commands vs supporting documentation
- **Consistent structure**: Follows the `.claude/commands/` pattern
- **Easy to find**: Agent definitions have their own directory
- **Future-proof**: Ready for additional agents
- **No functionality changes**: Only file moves and reference updates
### Pull Request checklist
- [x] Add/update test to cover these changes - N/A, documentation
reorganization
- [x] Update documentation - Updated all cross-references
- [x] Update CHANGELOG file - N/A, documentation reorganization
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Documentation**
* Updated PR Testing Agent and PR Testing Guide: fixed several relative
links and references to point to the new documentation locations and
corrected prompt/example references. No functional changes.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Claude <noreply@anthropic.com>
### Summary
Reorganizes the PR testing documentation into a clearer directory
structure, following the established pattern of `.claude/commands/`.
### Changes
**New structure:**
```
.claude/
├── agents/ # Agent definitions (NEW)
│ ├── pr-testing-agent.md
│ └── pr-testing-guide.md
├── commands/ # Slash commands
└── docs/ # Supporting documentation
├── analysis/
├── testing-build-scripts.md
├── master-health-monitoring.md
└── ...
```
**What changed:**
- Created `.claude/agents/` directory
- Moved `pr-testing-agent.md` and `pr-testing-guide.md` from `docs/` to
`agents/`
- Updated all cross-references in both files (now point to `../docs/`
for supporting docs)
- Updated prompt examples to use new path
(`.claude/agents/pr-testing-agent.md`)
### Benefits
- **Clear separation**: Agents vs commands vs supporting documentation
- **Consistent structure**: Follows the `.claude/commands/` pattern
- **Easy to find**: Agent definitions have their own directory
- **Future-proof**: Ready for additional agents
- **No functionality changes**: Only file moves and reference updates
### Pull Request checklist
- [x] Add/update test to cover these changes - N/A, documentation
reorganization
- [x] Update documentation - Updated all cross-references
- [x] Update CHANGELOG file - N/A, documentation reorganization
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Documentation**
* Updated PR Testing Agent and PR Testing Guide: fixed several relative
links and references to point to the new documentation locations and
corrected prompt/example references. No functional changes.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Claude <noreply@anthropic.com>
Summary
Reorganizes the PR testing documentation into a clearer directory structure, following the established pattern of
.claude/commands/.Changes
New structure:
What changed:
.claude/agents/directorypr-testing-agent.mdandpr-testing-guide.mdfromdocs/toagents/../docs/for supporting docs).claude/agents/pr-testing-agent.md)Benefits
.claude/commands/patternPull Request checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.