Conversation
|
WalkthroughThis pull request expands CONTRIBUTING.md with a detailed PR workflow (draft-PR requirement, addressing CI comments, CI checks, ready-for-review steps), a cost/benefit analysis for risky changes, and restructured guidance for changesets and server changes. The GitHub Actions workflow renames the existing job from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (2)
.github/workflows/require-draft-pr.yml (2)
3-5: Potential race condition with vouch-check-pr.yml workflow.Both this workflow and
.github/workflows/vouch-check-pr.ymltrigger onpull_request_target: [opened]and both can close PRs. When an unvouched external contributor opens a non-draft PR, both workflows will run concurrently:
vouch-check-pr.ymlwill attempt to close it (unvouched user)require-draft-pr.ymlwill also attempt to close it (non-draft)This could result in redundant close attempts or confusing duplicate comments. Consider coordinating these workflows, for example by:
- Adding
reopenedto this workflow's triggers (so it catches PRs reopened after vouch approval)- Using
needs:to sequence workflows, or- Combining the checks into a single workflow
💡 Suggested change to also handle reopened PRs
on: pull_request_target: - types: [opened] + types: [opened, reopened]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/require-draft-pr.yml around lines 3 - 5, The require-draft-pr workflow can race with vouch-check-pr because both use pull_request_target: types: [opened] and may close the same PR; update the require-draft-pr workflow's trigger to include reopened (add "reopened" to the pull_request_target.types array) so it also handles PRs reopened after vouching, and add a concurrency key (e.g. concurrency: group: pr-close-${{ github.event.pull_request.number }}) to the require-draft-pr job to prevent it running simultaneously with vouch-check-pr.yml, or alternatively combine the draft-check and vouch-check logic into a single workflow to eliminate the race entirely.
12-15: Consider excluding COLLABORATOR from draft enforcement.The condition currently exempts
MEMBERandOWNER, butCOLLABORATOR(users with explicit repository access) are also trusted contributors who have been granted access by maintainers. Given the vouch-based trust system described in CONTRIBUTING.md, excludingCOLLABORATORwould make the draft-first policy more consistent across all trusted roles.💡 Optional: Exclude COLLABORATOR as well
if: > github.event.pull_request.draft == false && github.event.pull_request.author_association != 'MEMBER' && - github.event.pull_request.author_association != 'OWNER' + github.event.pull_request.author_association != 'OWNER' && + github.event.pull_request.author_association != 'COLLABORATOR'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/require-draft-pr.yml around lines 12 - 15, Update the workflow conditional that checks github.event.pull_request.author_association to also exempt 'COLLABORATOR' so trusted collaborators aren't forced to use draft PRs; locate the existing if block that currently checks for != 'MEMBER' and != 'OWNER' and add a third check != 'COLLABORATOR' (or refactor to explicit allowlist of MEMBER|OWNER|COLLABORATOR) to ensure the draft enforcement skips collaborators as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/require-draft-pr.yml:
- Around line 3-5: The require-draft-pr workflow can race with vouch-check-pr
because both use pull_request_target: types: [opened] and may close the same PR;
update the require-draft-pr workflow's trigger to include reopened (add
"reopened" to the pull_request_target.types array) so it also handles PRs
reopened after vouching, and add a concurrency key (e.g. concurrency: group:
pr-close-${{ github.event.pull_request.number }}) to the require-draft-pr job to
prevent it running simultaneously with vouch-check-pr.yml, or alternatively
combine the draft-check and vouch-check logic into a single workflow to
eliminate the race entirely.
- Around line 12-15: Update the workflow conditional that checks
github.event.pull_request.author_association to also exempt 'COLLABORATOR' so
trusted collaborators aren't forced to use draft PRs; locate the existing if
block that currently checks for != 'MEMBER' and != 'OWNER' and add a third check
!= 'COLLABORATOR' (or refactor to explicit allowlist of
MEMBER|OWNER|COLLABORATOR) to ensure the draft enforcement skips collaborators
as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 929534f5-166e-4e0e-973b-561127eeb48e
📒 Files selected for processing (2)
.github/workflows/require-draft-pr.ymlCONTRIBUTING.md
📜 Review details
⏰ 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). (27)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Bun Runtime
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
CONTRIBUTING.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3200
File: docs/config/config-file.mdx:353-368
Timestamp: 2026-03-10T12:44:19.869Z
Learning: In the triggerdotdev/trigger.dev repository, docs PRs are often written as companions to implementation PRs (e.g., PR `#3200` documents features being added in PR `#3196`). When reviewing docs PRs, the documented features may exist in a companion/companion PR branch rather than main. Always check companion PRs referenced in the PR description before flagging missing implementations.
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Do not commit directly to the `main` branch; all changes should be made in a separate branch and go through a pull request
Applied to files:
CONTRIBUTING.md
📚 Learning: 2026-03-13T13:37:49.544Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T13:37:49.544Z
Learning: Applies to {packages,integrations}/**/*.{ts,tsx,js,json} : When modifying public packages (packages/* or integrations/*), add a changeset using pnpm run changeset:add with default patch level for bug fixes and minor changes
Applied to files:
CONTRIBUTING.md
📚 Learning: 2026-03-02T12:43:48.124Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/trigger-sdk/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:48.124Z
Learning: Docs updates should typically be done in a separate PR from SDK feature implementation
Applied to files:
CONTRIBUTING.md
🪛 LanguageTool
CONTRIBUTING.md
[style] ~250-~250: Consider using a different verb for a more formal wording.
Context: ...bit. Go through each comment and either fix the issue or resolve it with a comment ...
(FIX_RESOLVE)
🔇 Additional comments (3)
CONTRIBUTING.md (2)
245-253: LGTM! Clear PR workflow guidance.The draft-first workflow is well-documented and aligns with the automated enforcement in the new GitHub Actions workflow. The step-by-step instructions are clear and actionable for external contributors.
254-261: Good addition of cost/benefit guidance and general guidelines.The cost/benefit analysis section sets appropriate expectations for risky changes, and the general guidelines maintain existing best practices.
.github/workflows/require-draft-pr.yml (1)
17-24: Workflow implementation looks good overall.The use of
pull_request_targetwithpull-requests: writepermission is appropriate since the workflow needs to close PRs but doesn't checkout untrusted code. The comment message is helpful and includes a direct link to the relevant CONTRIBUTING.md section.
ericallam
force-pushed
the
ea-branch-125
branch
from
d6ed70b to
f5a68cd
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…Rs open in "ready to review" status
ericallam
force-pushed
the
ea-branch-125
branch
from
f5a68cd to
722254d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
250-250: Optional wording tweak for clarity.“Resolve it with a comment” can read like a UI permission/action requirement. “Reply with a comment” is clearer for all contributors.
✏️ Suggested wording
-2. **Address all CodeRabbit code review comments.** Our CI runs an automated code review via CodeRabbit. Go through each comment and either fix the issue or resolve it with a comment explaining why no change is needed. +2. **Address all CodeRabbit code review comments.** Our CI runs an automated code review via CodeRabbit. Go through each comment and either fix the issue or reply with a comment explaining why no change is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 250, Update the phrasing in the CodeRabbit guidance under the "2. **Address all CodeRabbit code review comments.**" bullet: replace "Resolve it with a comment" with "Reply with a comment" to clarify the required action; ensure the sentence now reads that contributors should either fix the issue or reply with a comment explaining why no change is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 250: Update the phrasing in the CodeRabbit guidance under the "2.
**Address all CodeRabbit code review comments.**" bullet: replace "Resolve it
with a comment" with "Reply with a comment" to clarify the required action;
ensure the sentence now reads that contributors should either fix the issue or
reply with a comment explaining why no change is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 79dc4a8e-c942-4297-a763-bc53d7ba8a82
📒 Files selected for processing (2)
.github/workflows/vouch-check-pr.ymlCONTRIBUTING.md
📜 Review details
⏰ 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). (27)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
CONTRIBUTING.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3200
File: docs/config/config-file.mdx:353-368
Timestamp: 2026-03-10T12:44:19.869Z
Learning: In the triggerdotdev/trigger.dev repository, docs PRs are often written as companions to implementation PRs (e.g., PR `#3200` documents features being added in PR `#3196`). When reviewing docs PRs, the documented features may exist in a companion/companion PR branch rather than main. Always check companion PRs referenced in the PR description before flagging missing implementations.
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Do not commit directly to the `main` branch; all changes should be made in a separate branch and go through a pull request
Applied to files:
CONTRIBUTING.md
📚 Learning: 2026-03-13T13:37:49.544Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-13T13:37:49.544Z
Learning: Applies to {packages,integrations}/**/*.{ts,tsx,js,json} : When modifying public packages (packages/* or integrations/*), add a changeset using pnpm run changeset:add with default patch level for bug fixes and minor changes
Applied to files:
CONTRIBUTING.md
📚 Learning: 2026-03-02T12:43:48.124Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/trigger-sdk/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:48.124Z
Learning: Docs updates should typically be done in a separate PR from SDK feature implementation
Applied to files:
CONTRIBUTING.md
🪛 LanguageTool
CONTRIBUTING.md
[style] ~250-~250: Consider using a different verb for a more formal wording.
Context: ...bit. Go through each comment and either fix the issue or resolve it with a comment ...
(FIX_RESOLVE)
🔇 Additional comments (3)
.github/workflows/vouch-check-pr.yml (2)
33-44: Good idempotency guard before closing the PR.The
gh pr viewstate check correctly avoids failing whencheck-vouchalready closed the PR.
13-13: Verify that branch protection rules reference the new job context.Renaming the job from
check-prtocheck-vouchrequires updating any branch protection or ruleset rules that enforce the old check context. Please confirm that themainbranch protection rules have been updated to requirecheck-vouchinstead ofcheck-pr, or that no such rules exist.CONTRIBUTING.md (1)
247-253: PR workflow guidance aligns well with the new enforcement.The draft-first sequence here matches the
require-draftautomation and sets clear contributor expectations.
2 participants
No description provided.