diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md new file mode 100644 index 00000000000..58aa5f63bd5 --- /dev/null +++ b/.agents/skills/code-review/SKILL.md @@ -0,0 +1,80 @@ +--- +name: code-review +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 +--- + +# Code Review + +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**, prefer the GitHub CLI — it handles auth and gives the full diff: + +```bash +gh pr view --repo DataDog/dd-trace-js # title, description, labels +gh pr diff --repo DataDog/dd-trace-js # full unified diff +``` + +If `gh` is unavailable, fall back to WebFetch: +- `https://github.com/DataDog/dd-trace-js/pull//files` + +**If a file path is given**, read the file and read the surrounding context (callers, tests, +similar patterns in the codebase) before commenting. + +## 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 explaining why this matters. + +**Suggested fix:** Concrete code or approach. + +--- +``` + +Categories: +- Architecture & Design +- Performance & Memory +- Configuration System +- Async & Correctness +- Test Quality +- Code Style & Readability +- Observability & Logging +- Documentation & PR Hygiene + +Severity: +- **BLOCKER** — must be fixed before merge +- **CONCERN** — worth discussing; approval may be conditional on author's response +- **NIT** — non-blocking style preference + +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 diff --git a/.agents/skills/code-review/references/review-checklist.md b/.agents/skills/code-review/references/review-checklist.md new file mode 100644 index 00000000000..37c8cb8e7b3 --- /dev/null +++ b/.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. diff --git a/.agents/skills/code-review/review-checklist.md b/.agents/skills/code-review/review-checklist.md new file mode 100644 index 00000000000..6ceb88f6bb5 --- /dev/null +++ b/.agents/skills/code-review/review-checklist.md @@ -0,0 +1,108 @@ +# 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.