feat(grpc): add update-grpc-proto skill and fix benchmark lint#518
feat(grpc): add update-grpc-proto skill and fix benchmark lint#518MartianGreed merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces documentation for Dojo integration testing and gRPC proto synchronization, along with new GitHub Actions workflows to automate Claude-assisted integration testing. A minor benchmark file update adds Bun timing utilities. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In @.github/workflows/integration-test.yaml:
- Around line 23-24: The workflow's permissions block only grants "contents:
read" which prevents the Claude action from posting results; update the
permissions block (the permissions key and its child keys) to include write
scopes such as "issues: write" and "pull-requests: write" (and/or "contents:
write" if the action needs to update repository contents) so the action can
create comments or update PR status; ensure the permissions block lists the
added keys: issues, pull-requests (and optionally contents) with "write".
🧹 Nitpick comments (5)
.claude/skills/dojo-integration-test/SKILL.md (1)
284-286: Consider usingkillwith signal handling instead ofkill -9.Using
kill -9(SIGKILL) doesn't allow processes to clean up gracefully. For troubleshooting docs, consider recommendingkill <PID>first (SIGTERM), which allows processes to handle shutdown gracefully, falling back tokill -9only if needed.📝 Suggested documentation update
# Kill by PID -kill -9 <PID> +kill <PID> # Try graceful shutdown first +kill -9 <PID> # Force kill if process doesn't respond.github/workflows/claude-assistant.yaml (2)
21-24: Label trigger condition may fire unexpectedly on non-PR events.The condition
github.event.label.name == 'test-integration'will match when any issue or PR is labeled, but the workflow also triggers onissues: [opened, assigned]which don't have label context. This could cause the job to be skipped silently or behave unexpectedly.Consider adding event type checks:
📝 Suggested fix
if: | (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) || - (github.event.label.name == 'test-integration') + ((github.event_name == 'issues' || github.event_name == 'pull_request') && github.event.action == 'labeled' && github.event.label.name == 'test-integration')
18-66: Add job timeout to prevent runaway costs.The Claude action with
--max-turns 30could potentially run for a long time. Adding atimeout-minuteshelps prevent excessive API usage and stuck workflows.📝 Suggested fix
jobs: claude-response: runs-on: ubuntu-latest + timeout-minutes: 30 if: |.github/workflows/integration-test.yaml (2)
40-42: Pin Bun version for reproducible CI builds.Same recommendation as the other workflow - pin to a specific version.
📝 Suggested fix
- uses: oven-sh/setup-bun@v1 with: - bun-version: latest + bun-version: "1.1.42"
26-65: Add job timeout to prevent runaway execution.With
--max-turns 30and user-provided prompts, the job could run for extended periods. Add a timeout for cost control and to prevent stuck workflows.📝 Suggested fix
jobs: integration-test: runs-on: ubuntu-latest + timeout-minutes: 45 steps:
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
Missing write permissions for Claude to post results.
The workflow only grants contents: read, but the Claude action may need to write comments or update PR status to report test results. Compare with the other workflow which grants issues: write and pull-requests: write.
📝 Suggested fix if Claude needs to post results
permissions:
contents: read
+ issues: write
+ pull-requests: write📝 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.
| permissions: | |
| contents: read | |
| permissions: | |
| contents: read | |
| issues: write | |
| pull-requests: write |
🤖 Prompt for AI Agents
In @.github/workflows/integration-test.yaml around lines 23 - 24, The workflow's
permissions block only grants "contents: read" which prevents the Claude action
from posting results; update the permissions block (the permissions key and its
child keys) to include write scopes such as "issues: write" and "pull-requests:
write" (and/or "contents: write" if the action needs to update repository
contents) so the action can create comments or update PR status; ensure the
permissions block lists the added keys: issues, pull-requests (and optionally
contents) with "write".
1 participant
Summary
/update-grpc-protoskill for updating gRPC proto files from Torii repositoryTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.