Skip to content

docs: expand PR guidelines in CONTRIBUTING.md and AGENTS.md #14078

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.

Sign up for GitHub

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

Open
christian-byrne wants to merge 3 commits into
base: main
Choose a base branch
from
Open

docs: expand PR guidelines in CONTRIBUTING.md and AGENTS.md #14078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a84af84
fix: RAF-batch WebSocket progress events
christian-byrne May 4, 2026
7d70a42
docs: expand PR guidelines in CONTRIBUTING.md and AGENTS.md
christian-byrne Jul 24, 2026
b87e634
[automated] Apply ESLint and Oxfmt fixes
actions-user Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,18 @@ When referencing Comfy-Org repos:

Rules for agent-based coding tasks.

### PR Review Comment Resolution

**Never resolve review comments on PRs where you are the author.**

Per the team's [PR guidelines](CONTRIBUTING.md#comment-resolution), resolving comments is the reviewer's prerogative. As author, you may only resolve:

- Automated review comments (Coderabbit, Claude, etc.)
- Trivial single-interpretation comments (e.g. fixing a typo exactly as suggested)
- Comments addressed via GitHub's "Apply suggestion" feature used as-is

For all other comments: reply in the thread explaining what you changed (or why you disagree), then re-assign the PR to the reviewer. **Do not click Resolve.**

### Chrome DevTools MCP

When using `take_snapshot` to inspect dropdowns, listboxes, or other components with dynamic options:
Expand Down
62 changes: 61 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,13 +266,73 @@ The original litegraph repository (https://github.com/Comfy-Org/litegraph.js) is
- `refactor:` for code refactoring
- `test:` for test additions/changes
- `chore:` for maintenance tasks
5. **Draft PRs**: Mark as Draft if not ready for review
6. **Assign Reviewer**: When ready, set one person as `Assignee` — they'll receive a Slack notification. Use git blame and context to pick someone relevant.
7. **Re-review**: After addressing comments, re-assign to the reviewer (and optionally ping them in Slack)
8. **Merging**: Merge once all `Assignees` have approved and you feel all comments are addressed. Don't wait for non-assignee approvals unless they've added themselves as `Assignee`.
Comment on lines +270 to +272

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Wrap changed Markdown lines to 80 columns.

These changed prose lines exceed the repository’s 80-column Markdown rule. Reflow them across indented continuation lines without changing the guidance.

As per coding guidelines, Markdown files must use an 80-column width.

Also applies to: 306-306, 309-309, 334-335

🤖 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 `@CONTRIBUTING.md` around lines 270 - 272, Reflow the changed prose in the
Markdown guidance, including the lines around “Re-review,” “Merging,” and the
referenced sections, so each line is at most 80 columns. Use indented
continuation lines and preserve the exact guidance and Markdown structure.

Source: Coding guidelines

9. **Auto-merge**: Only enable after full approvals, not while waiting

### Comment Types

When leaving or receiving review comments:

- **`nit:`** — Optional suggestion; author chooses whether to implement
- **`Should we...?` / `What do you think?`** — Optional discussion point
- **No prefix** — Required change; must be addressed before merge

If a comment's blocking status is unclear, respond asking for clarification.

### Comment Resolution

- **Reviewers** resolve their own comments when satisfied
- **Authors** may only resolve in these specific cases:
- Automated reviews (Coderabbit, Claude, etc.)
- Trivial single-interpretation comments (e.g. fixing a typo)
- Applying a suggested fix exactly as written via GitHub's "Apply suggestion"

### Deferring Work

If you want to defer non-critical feedback to a follow-up PR, use:

```

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add language identifiers to the fenced blocks.

markdownlint reports MD040 for both fences. Use an explicit identifier such as text:

-```
+```text

Also applies to: 313-313

🧰 Tools
🪛 markdownlint-cli2 (0.23.0)

[warning] 297-297: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@CONTRIBUTING.md` at line 297, Update both fenced code blocks in
CONTRIBUTING.md, including the blocks around the referenced locations, to
specify an explicit language identifier such as text immediately after the
opening fence. Keep the existing block contents unchanged.

Source: Linters/SAST tools

@coderabbitai please make a tracking issue for this and assign me
```

Deferred work must have a tracking issue — implicit deferrals are not allowed.

### Review Process

1. All PRs require at least one review
2. Address review feedback promptly
2. Address review feedback promptly (reviewers should respond within 24 hours or reassign)
3. Keep PRs focused - one feature/fix per PR
4. Large features should be discussed in an issue first
5. For extended discussions or potential huddles, post the PR link in `#frontend-code-reviews`

### Reviewer Decision Guide

```
Starting PR Review
├── Changes needed → Request Changes
│ ├── All comments required → No prefix (blocking by default)
│ └── Some optional → Use "nit:" prefix
├── Looks good, confident → Approve
│ ├── With comments → All comments are nits
│ └── Clean approval → No comments needed
├── Looks good, not confident → Comment Review with "LGTM"
│ └── Indicates reviewed but not confident enough to approve alone
├── Need discussion/questions → Comment Review
│ └── Ask questions while indicating full PR review
└── Just adding annotations → Normal Comment
└── Add comments for future readers
```

### Resolving Disagreements

1. Use comment threads for clarification
2. Create a huddle or schedule a live review for complex discussions
3. Escalate to team leads if discussion goes in circles
4. Document recurring style conflicts in Coderabbit config or `CLAUDE.md`
5. Defer to the author for pure personal preference items (but don't mislabel technical decisions as preference)

## Questions?

Expand Down
Loading
Loading