chore(skills): add code-review skill with improved structure

Improved from PR #7888: description is explicit about trigger phrases,
SKILL.md body is lean (no checklist duplication), checklist is in
references/ per skill anatomy conventions, and gh pr diff is the
preferred method for fetching PR content.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: ida613

.agents/skills/code-review/SKILL.md

@@ -1,46 +1,65 @@
 ---
 name: code-review
-description: >
-  Review a PR or code change against the dd-trace-js codebase standards. Use when asked to review
-  code, check a PR before submitting, or identify issues across architecture, correctness, style,
-  tests, and observability. Outputs categorised comments with severity labels and fix suggestions.
-disable-model-invocation: true
+description: |
+  Review a PR or code change against dd-trace-js codebase standards. Use this skill whenever
+  someone asks to "review" code or a PR, types "/review", asks "can you check this before I
+  submit", wants to identify issues in a change, asks for a code review, says "PTAL", or
+  mentions a PR number and wants quality feedback. Also use proactively when someone is about
+  to open a PR and wants a pre-submission check. The input can be a PR number, a file path,
+  a diff pasted inline, or a description of the change.
 allowed-tools: Read, Grep, Glob, Bash, WebFetch
 ---
 
-You are performing a thorough code review of a dd-trace-js change. Use the checklist in
-[review-checklist.md](review-checklist.md) to guide your analysis. Group your output by category,
-label each comment by severity, and include a concrete fix suggestion for every issue raised.
+# Code Review
 
-## Input
+You are performing a thorough code review of a dd-trace-js change. Work through
+[references/review-checklist.md](references/review-checklist.md) category by category, flag
+every issue you find, and always pair a finding with a concrete fix suggestion.
+
+## Getting the change
 
 `$ARGUMENTS` — a PR number, file path, diff, or description of the change to review.
 
-If a PR number is given, use the GitHub API or WebFetch to read the diff and description:
+**If a PR number is given**, prefer the GitHub CLI — it handles auth and gives the full diff:
+
+```bash
+gh pr view <PR_NUMBER> --repo DataDog/dd-trace-js      # title, description, labels
+gh pr diff <PR_NUMBER> --repo DataDog/dd-trace-js      # full unified diff
+```
+
+If `gh` is unavailable, fall back to WebFetch:
 - `https://github.com/DataDog/dd-trace-js/pull/<PR_NUMBER>/files`
-- `https://api.github.com/repos/DataDog/dd-trace-js/pulls/<PR_NUMBER>/files`
 
-If a file path is given, read the file and review it in context.
+**If a file path is given**, read the file and read the surrounding context (callers, tests,
+similar patterns in the codebase) before commenting.
 
-## Output Format
+## How to review
+
+1. Read [references/review-checklist.md](references/review-checklist.md) before starting.
+2. Work through each applicable category. For each finding, ask: what is the actual risk, and
+   is there an existing pattern in the codebase the author should follow?
+3. For reviewer-specific preferences (what rochdev, BridgeAR, simon-id, etc. tend to flag),
+   see `.cursor/skills/reviewer-profiles.md`.
+
+## Output format
+
+Group findings by category. Omit any category with no findings.
 
 ```
 ## Architecture & Design
 
 ### [BLOCKER | CONCERN | NIT] Short title
+
 **File:** path/to/file.js:line
-**Comment:** Specific feedback.
-**Suggested fix:** Concrete code or approach to address it.
 
----
+**Comment:** Specific feedback explaining why this matters.
 
-## Performance & Memory
+**Suggested fix:** Concrete code or approach.
 
-### [BLOCKER | CONCERN | NIT] Short title
-...
+---
 ```
 
-Repeat for each applicable category:
+Categories:
 - Architecture & Design
 - Performance & Memory
 - Configuration System
@@ -50,89 +69,12 @@ Repeat for each applicable category:
 - Observability & Logging
 - Documentation & PR Hygiene
 
-Omit any category that has no findings.
-
-Label each comment:
+Severity:
 - **BLOCKER** — must be fixed before merge
-- **CONCERN** — notable issue worth discussing; likely approved conditional on author's response
-- **NIT** — style/readability preference; non-blocking
-
-At the end, add a `## Summary` section with an overall verdict:
-- "LGTM" — no significant issues
-- "LGTM with caveats" — concerns worth tracking but not blocking
-- "CHANGES_REQUESTED" — one or more BLOCKERs present
-
----
+- **CONCERN** — worth discussing; approval may be conditional on author's response
+- **NIT** — non-blocking style preference
 
-## Review Checklist (read before reviewing)
-
-See [review-checklist.md](review-checklist.md) for the full reference.
-
-### Architecture & Design (most likely BLOCKER)
-- Does the code bypass an existing designed mechanism (Diagnostics Channel, store bindings,
-  `runStores`/`bindStore`, propagator interfaces)?
-- Does it call internal APIs directly when a public/designed interface exists?
-- Does it mix cross-cutting concerns (e.g., tying baggage to span context when the systems are
-  intentionally separate)?
-- Are WeakRefs used for cleanup that should go through proper store lifecycle?
-- Is a helper function that only wraps another function with no transformation?
-- Are symbols re-exported without modification?
-- Is a function called in only one place that could be inlined?
-
-### Performance & Memory (BLOCKER or CONCERN)
-- Is there a risk of unbounded growth (streams, maps, listeners)?
-- Are there unnecessary allocations in a hot path?
-- If the PR claims a performance improvement, is there a benchmark?
-- Is a Map keyed by objects (e.g., request objects, contexts) without guaranteed cleanup? Prefer WeakMap.
-- Is timer-based cleanup used where it could cause sawtooth memory growth?
-- Is an expensive computation created eagerly when it is only needed conditionally?
-
-### Configuration System (BLOCKER)
-- Is a new environment variable defined outside of `packages/dd-trace/src/config/index.js`?
-- Is it missing from `supported-configurations.json`?
-- Does it lack telemetry registration?
-- Is the config file modified in a way that looks corrupted (e.g., rebase artifact)?
-- Is a new boolean toggle being added that tightly mirrors an existing option?
-
-### Async & Correctness (BLOCKER or CONCERN)
-- Are two `await` calls in sequence where rejection of the first would leave the second unhandled?
-  Use `Promise.all([...])` instead.
-- Does code rely on NaN being falsy? Be explicit.
-- Is there an unnecessary fallback that silently changes existing behaviour (e.g., `undefined` → `null`)?
-- Does a Node.js built-in API exist that avoids a custom workaround?
-
-### Test Quality (BLOCKER or CONCERN)
-- Are plugin tests mocking tracer internals instead of reconfiguring via public API?
-- Is `afterEach` cleanup missing? (Reset after an assertion = unstable test)
-- Does the test spy on implementation details that don't need to be tested?
-- Does a bug fix lack a test to verify it?
-- Does one `it` block depend on side effects from a previous `it` block?
-- Is shared mutable state missing `beforeEach`/`afterEach` setup/teardown?
-- Is there a redundant existence check immediately before a value assertion? Assert the value directly.
-- Are there copy-paste mistakes (duplicate assertions on the same property)?
-- Could multiple `assert.strictEqual` calls be combined into one `assert.deepStrictEqual` or
-  `assertObjectContains`?
-
-### Code Style & Readability (NIT)
-- Are `_underscore` prefixed properties used on a class where `#private` fields would work?
-- Are properties conditionally added to an object after construction, causing V8 shape changes?
-- Are static getters used instead of static class members? Use static properties.
-- Do non-class symbols have a leading capital letter? They shouldn't.
-- Are `require()` calls inside functions or try/catch blocks when they could be at the top of the file?
-- Is a try/catch wrapping more code than what can actually throw? Prefer narrow try/catch.
-- Are bitwise or type-coercion tricks used that are hard to follow at a glance?
-- Is there commented-out code that should be removed?
-
-### Observability & Logging (CONCERN or NIT)
-- Is any existing log/debug output being removed without a replacement?
-- Is there a code path where a feature could silently fail without any log?
-- Should a `catch` block log the error?
-
-### Documentation & PR Hygiene (NIT or CONCERN)
-- Are there unrelated changes that should be in a separate PR?
-- Is a new CI workflow justified, or could it go in an existing one?
-- Is retry logic too broad (retrying non-flaky commands)?
-- Does the PR add new public API surface area that needs a semver label update?
-- Does AGENTS.md use fenced code blocks where inline code would save tokens?
-- Does AGENTS.md contain inaccurate, vague, or contradictory claims?
-- Does moving a file to a new directory change a user-facing `require()` path?
+End with a `## Summary` section giving an overall verdict:
+- `LGTM` — no significant issues
+- `LGTM with caveats` — concerns worth tracking, not blocking
+- `CHANGES_REQUESTED` — one or more BLOCKERs present

.agents/skills/code-review/references/review-checklist.md

@@ -0,0 +1,133 @@
+# dd-trace-js Code Review Checklist
+
+Aggregated from analysis of review patterns across the primary maintainers of this repository.
+
+---
+
+## Architecture & Design
+
+**BLOCKER when a shortcut bypasses the intended system design.**
+
+- Route through designed interfaces — don't call internal APIs directly when a public interface
+  exists (Diagnostics Channel, `runStores`/`bindStore`, propagator interfaces).
+- Don't mix cross-cutting concerns. Systems that are intentionally separate (e.g., baggage and
+  span context) must stay separate.
+- Avoid `WeakRef` for cleanup — it has caused memory leaks in this codebase. Use proper store
+  lifecycle (`runStores`/`bindStore`) instead.
+- Don't add helpers that only call another function with no transformation. Inline them.
+- Don't re-export symbols without modification — it adds confusion.
+- Functions called in only one place should be inlined for clarity.
+- Plugin hook setup that is repetitive should be made implicit via `addHooks`.
+
+---
+
+## Performance & Memory
+
+**BLOCKER or CONCERN in hot paths; CONCERN elsewhere.**
+
+- dd-trace runs in application hot paths — every allocation and lookup counts.
+- Flag unbounded collections: streams, maps, or listeners that can grow without limit.
+- Flag unnecessary allocations or object creation in hot paths.
+- A Map keyed by user-operation objects (requests, contexts) must have a guaranteed cleanup
+  path, or use `WeakMap` instead. Timer-based cleanup is risky — it can cause sawtooth memory
+  growth.
+- Prefer lazy initialisation: don't create expensive objects or run expensive computations
+  until they are needed.
+- If a PR claims a performance improvement, a benchmark is required to demonstrate it.
+
+---
+
+## Configuration System
+
+**BLOCKER when registration is missing.**
+
+- New environment variables must be defined in `packages/dd-trace/src/config/index.js` inside
+  `#applyEnvironment()`.
+- Every new env var must have an entry in `supported-configurations.json` with a
+  `configurationNames` field.
+- Every new env var must be registered for telemetry.
+- `supported-configurations.json` must not be corrupted (watch for rebase artifacts).
+- Don't add a new boolean toggle that mirrors an existing option. If a feature is enabled by a
+  parent config, it should not need its own sub-toggle.
+
+---
+
+## Async & Correctness
+
+**BLOCKER for unhandled rejections; CONCERN for logic issues.**
+
+- Sequential `await` calls where rejection of the first would leave the second as an unhandled
+  rejection must be rewritten as `Promise.all([...])`.
+- Don't rely on NaN being falsy — be explicit. TypeScript rejects it and it obscures intent.
+- Don't introduce a fallback that silently changes existing behaviour (e.g., `undefined` → `null`).
+- Check whether a Node.js built-in API already solves the problem before writing a custom
+  workaround.
+
+---
+
+## Test Quality
+
+**BLOCKER for missing bug-fix tests or broken test isolation; CONCERN for flakiness risks.**
+
+- Every bug fix must have a test that would have caught the bug.
+- Plugin tests are integration tests — don't mock tracer internals. Reconfigure via the public
+  API instead.
+- Cleanup/reset logic must be in `afterEach`, not after an assertion. If the assertion throws,
+  the reset never runs.
+- Don't spy on implementation details — test behaviour only.
+- Each `it` block must be independently runnable with `it.only`. If it depends on side effects
+  from a previous `it`, move the shared setup into `beforeEach` and teardown into `afterEach`.
+- Don't add an existence check (`assert(x.foo)`) immediately before asserting the value
+  (`assert.strictEqual(x.foo, 'bar')`). Assert the value directly.
+- Combine multiple `assert.strictEqual` calls on the same object into a single
+  `assert.deepStrictEqual` or `assertObjectContains` call.
+- Check for copy-paste mistakes: duplicate assertions on the same property.
+
+---
+
+## Code Style & Readability
+
+**NIT unless it affects correctness.**
+
+- Use `#private` class fields instead of `_underscore` prefixes for class-internal state.
+- Pre-construct objects with all possible fields (some `undefined`) to keep V8 object shapes
+  stable. Don't conditionally add properties after construction.
+- Use static class properties, not static getters (static getters are a holdover from before
+  static properties were supported).
+- Non-class symbols must not have a leading capital letter.
+- `require()` calls must be at the top of the file. Only the instantiation (not the require)
+  should be inside try/catch if the require itself cannot fail.
+- Keep try/catch blocks narrow — wrap only the code that can actually throw.
+- Avoid bitwise or type-coercion tricks that require knowing operator precedence or implicit
+  coercion rules. Write explicit, readable code.
+- Remove commented-out code before merging.
+
+---
+
+## Observability & Logging
+
+**CONCERN when silent failures are possible; NIT for improvements.**
+
+- Don't remove existing log or debug output without a replacement. Diagnostic information helps
+  production debugging.
+- Any code path where a feature could silently fail should have a log statement.
+- `catch` blocks that swallow errors should log them.
+- Log at the point of failure, not only at the call site.
+
+---
+
+## Documentation & PR Hygiene
+
+**CONCERN or NIT.**
+
+- Unrelated changes belong in a separate PR.
+- A new CI workflow must be justified — prefer adding to an existing workflow.
+- Retry logic must be scoped: only commands that can be flaky (e.g., registry installs) should
+  be retried.
+- New public API surface area requires a semver-minor label and may require a corresponding PR
+  in dd-trace-api-js.
+- AGENTS.md: prefer inline code over fenced code blocks (token efficiency). Avoid vague
+  language; use concrete examples.
+- AGENTS.md must not contain inaccurate or contradictory claims, and should not include
+  contributing-guide content that belongs in CONTRIBUTING.md.
+- Moving a file to a new directory must not change a user-facing `require()` path.