review-pr

Source excerpt starting at line 1.

1---2name: review-pr3description: Review a Deno runtime pull request for correctness, tests, security, and conventions. Use when asked to review a PR or when a PR number/URL is provided for review.4argument-hint: <pr-number-or-url>5allowed-tools: Bash(gh *) Bash(git *) Read Glob Grep Agent6---7 8# Deno PR Reviewer9 10Review PR `$ARGUMENTS` on the `denoland/deno` repository.11 12## Step 1: Gather PR context13 14Fetch the PR metadata, diff, and comments:15 16```!17gh pr view $ARGUMENTS --json number,title,body,author,labels,state,reviewDecision,commits,files,isDraft,createdAt,url18```19 20```!21gh pr diff $ARGUMENTS22```23 24```!25gh pr view $ARGUMENTS --comments --json comments26```27 28```!29gh pr checks $ARGUMENTS --json name,state,conclusion 2>/dev/null || echo "No checks found"30```31 32## Step 2: Gate checks33 34Before reviewing code, check these gates. If any fail, flag them prominently at35the top of your review and do not approve.36 371. **CI status** — All checks must pass. Point the author to specific failing38   checks. Known flaky tests (labeled `ci-test-flaky`) can be re-run.392. **PR title format** — Must follow `type(scope): description`. Types: `feat`,40   `fix`, `perf`, `refactor`, `chore`, `docs`, `test`, `revert`, `BREAKING`.41   Scope examples: `ext/node`, `ext/fetch`, `cli`, `lsp`, `runtime`.423. **No force pushes** — PRs are squash-merged. Authors should push new commits,43   not rewrite history.444. **Focused scope** — No drive-by cleanups or unrelated changes. Those belong45   in separate PRs.465. **AI disclosure** — If the PR looks AI-generated (boilerplate-heavy, generic47   comments, suspiciously broad) but has no disclosure, ask about it.486. **Linked issue (external contributors)** — If the PR author is not a49   `denoland` org member, the PR must link to an issue. If there is no linked50   issue, request changes and ask the author to open an issue and discuss the51   change first.52 53## Step 3: Code review54 55Read every changed file in the diff. Use the repo tools (`Read`, `Grep`, `Glob`)56to understand surrounding context when needed.57 58### Rust code59 60- **Correctness**: Edge cases handled? No `.unwrap()` on user-controlled data?61- **Error handling**: Proper error types, meaningful messages, no swallowed62  errors.63- **Performance**: No unnecessary allocations/copies, no blocking in async code.64- **Safety**: No `unsafe` without strong justification. No command injection,65  path traversal, or permission bypasses.66- **Permissions**: New capabilities must go through Deno's permission system.67  Watch `ext/node/` especially — Node.js APIs sometimes assume full access.68- **Dependencies**: New Cargo deps need strong justification. Prefer existing69  deps or stdlib.70 71### JavaScript/TypeScript code72 73- **Node.js compatibility** (`ext/node/`): Does the implementation match Node.js74  behavior? Check against Node.js docs and/or source code.75- **Primordials**: Internal JS should use primordials76  (`globalThis.__bootstrap.primordials`) to avoid prototype pollution. Built-in77  methods must not be called on user-controlled objects without primordial78  wrappers.79- **Web standards**: Web API implementations should follow the relevant spec.80  WPT coverage is preferred.81- **Lazy loading**: All code should use lazy-loaded imports where possible to82  reduce startup cost.83 84### Tests85 86- Every bug fix needs a test that would have caught the bug. Every feature needs87  happy-path + edge-case tests.88- Prefer unit tests over spec tests over integration tests. Only use spec tests89  when the behavior requires CLI-level validation.90- Spec tests live in `tests/specs/` using `__test__.jsonc`. Use `[WILDCARD]` for91  non-deterministic output, `[UNORDERED_START]`/`[UNORDERED_END]` for92  non-deterministic ordering.93- Tests must be deterministic — no race conditions, timing deps, or port94  conflicts.95 96### Security-sensitive areas97 98Pay extra attention to changes in:99 100- `runtime/permissions.rs` and permission checks throughout101- `ext/net/`, `ext/fs/` — network and filesystem access102- `ext/node/` — needs its own permission checks103- `cli/tools/compile.rs` — standalone binary compilation104- Any code that shells out or processes user-controlled paths/URLs105 106## Step 4: PR-type-specific checks107 108Apply additional checks based on the PR type:109 110- **Node.js compat** (`ext/node/`): Verify behavior against Node.js docs and/or111  source code, not just what "seems right". New polyfills must be registered in112  `ext/node/polyfills/01_require.js`.113- **Performance**: Must include before/after benchmarks or a clear argument for114  the improvement. Watch for correctness regressions.115- **Dependency updates**: Check changelog for breaking changes. Prioritize116  security updates.117- **WPT changes**: Verify passes are real, not just skipped assertions.118  Expectation file updates must match actual results. Suggest `ci-wpt-test`119  label if not present.120- **CI/release tooling**: Flag for `@bartlomieju` review — do not approve these121  yourself.122 123## Step 5: Write your review124 125Post a review using `gh pr review`. Structure:126 1271. **Summary** (1-2 sentences): What the PR does and your overall assessment.1282. **Gate issues** (if any): Blocking problems that must be fixed.1293. **Code comments**: Specific, actionable feedback referencing exact files and130   lines. Use `nit:` prefix for non-blocking suggestions. Suggest fixes when131   possible, not just "this is wrong."1324. **Verdict**: Approve, request changes, or comment.133 134### Tone135 136- Direct: "This needs a test" not "It would be wonderful if we could add a test137  here."138- Kind: Thank contributors, especially first-timers. Assume good intent.139- Helpful: If rejecting, show what a good version looks like.140- Brief: If the contributor clearly knows what they're doing, keep it tight.141 142### Posting the review143 144Prefer inline comments on specific lines where possible. Use a single review145with both a summary body and inline comments:146 147```148gh api repos/denoland/deno/pulls/{number}/reviews -f event=COMMENT -f body="summary" -f comments='[{"path":"file.rs","line":42,"body":"comment"}]'149```150 151Use `event=APPROVE` or `event=REQUEST_CHANGES` as appropriate instead of152`COMMENT`.153 154For simple reviews without inline comments, fall back to:155 156```157gh pr review $ARGUMENTS --comment --body "review text"158```159 160### Merge readiness161 162You do NOT have merge permissions. When a PR is ready:163 164- For first-time contributors: comment165  `@bartlomieju LGTM, needs maintainer signoff (first-time contributor)`166- For regular contributors: comment `@bartlomieju this is ready to merge`167 168## Rules169 170- Never approve a PR with failing CI.171- Never approve PRs that bypass the permission system.172- Never approve large architectural changes without flagging for maintainer173  discussion.174- Do not bikeshed style if it passes the linter.175- Do not request changes for things automated checks already enforce.176- Always confirm with the user before posting any review comments to GitHub.177