Skip to content

Add dd-apm-sdk-review skill and repo overrides, replace perf-review - #12364

Open
robertomonteromiguel wants to merge 7 commits into
masterfrom
robertomonteromiguel/dd-apm-sdk-review-core-overrides
Open

Add dd-apm-sdk-review skill and repo overrides, replace perf-review#12364
robertomonteromiguel wants to merge 7 commits into
masterfrom
robertomonteromiguel/dd-apm-sdk-review-core-overrides

Conversation

@robertomonteromiguel

@robertomonteromiguel robertomonteromiguel commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Adds the dd-apm-sdk-review skill to this repo: an automated, multi-lens code review that runs before a developer pushes, checking a diff for correctness, security, performance, design, maintainability, conventions, coherence, and cross-SDK consistency issues.

Concretely:

  • .agents/skills/dd-apm-sdk-review/ — the skill itself (SKILL.md + one rule file per lens). This is a verbatim copy from dd-apm-sdk-review-core, the shared source of truth for all tracer repos.
  • .agents/dd-apm-sdk-review-overrides/ — the dd-trace-java-specific layer: repo context and per-lens overrides that adapt the generic rules to this codebase.
  • .llm-validation/ — validation scaffolding to test the skill's review output against known cases.
  • Replaces the old perf-review skill, and updates CODEOWNERS, .gitignore, and AGENTS.md accordingly.

This lands as a single, squashed commit for ease of review. The branch history (with intermediate fixes from earlier automated review passes) is preserved at backup/phase1-agentic-review-rollout if needed.

How To Review This

  1. Start with dd-apm-sdk-review-core PR #1 — that's where the skill's design, rules, and rationale are reviewed in depth.
  2. Skip .agents/skills/dd-apm-sdk-review/reviewers/*.md here — those are an exact copy of the core repo's rules, not authored in this PR.
  3. Focus on .agents/dd-apm-sdk-review-overrides/ — this is the part specific to dd-trace-java and worth a real read.
  4. Skim .llm-validation/ and .promptfoo/ to confirm the test cases match what you'd expect the skill to catch (see Additional Notes below).

Motivation

Today, review feedback on things like resource leaks, TOCTOU security gaps, or serialization bugs only surfaces once a human reviewer catches it — after the PR is already open. This skill runs the same categories of checks locally, before push, so common and recurring issues get caught earlier and human review time is spent on judgment calls rather than patterns we already know how to detect.

Additional Notes

  • Some test cases were generated using Sphinx, an internal tool that mines merged PRs and fix commits from this repo's own history to surface recurring defect patterns. We used it to seed a first batch of promptfoo test cases from real, historical dd-trace-java bugs. This is a temporary home for those cases — once the testing tool is decided, they'll move to wherever that tool expects them.

Contributor Checklist

Jira ticket: [PROJ-IDENT]

Introduces .agents/skills/dd-apm-sdk-review as a multi-perspective PR
review skill (SKILL.md plus reviewer lenses: coherence, conventions,
correctness, cross-sdk, design, maintainability, performance, security,
and a shared report template), replacing the previous perf-review
skill.

Adds dd-trace-java-specific overrides under
.agents/skills/dd-apm-sdk-review-overrides (repo-context.md and
per-lens overrides for conventions, design, maintainability,
performance, security).

Wires up validation: .llm-validation/config.yaml and
suites/dd-apm-sdk-review.yaml, plus .promptfoo/dd-apm-sdk-review
config and test cases, to exercise the skill's review output.

Updates CODEOWNERS, .gitignore, AGENTS.md, and the .claude/skills
symlink accordingly.
@datadog-official

This comment has been minimized.

@dd-octo-sts

dd-octo-sts Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.79 s 14.64 s [+0.2%; +1.9%] (maybe worse)
startup:insecure-bank:tracing:Agent 13.69 s 13.63 s [-0.4%; +1.3%] (no difference)
startup:petclinic:appsec:Agent 17.54 s 17.38 s [+0.2%; +1.7%] (maybe worse)
startup:petclinic:iast:Agent 17.30 s 17.58 s [-2.4%; -0.8%] (maybe better)
startup:petclinic:profiling:Agent 17.51 s 17.37 s [-0.3%; +1.9%] (no difference)
startup:petclinic:sca:Agent 17.47 s 17.37 s [-0.2%; +1.4%] (no difference)
startup:petclinic:tracing:Agent 16.12 s 16.58 s [-7.0%; +1.5%] (no difference)

Commit: 1fccb971 · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

* ci: migrate dd-apm-sdk-review evals to llm-validation gate

Replace the promptfoo suite with .llm-validation cases, wire the reusable GitLab job, and document local Docker runs against the published llmval image.
@robertomonteromiguel
robertomonteromiguel marked this pull request as ready for review September 7, 2026 07:49
@robertomonteromiguel
robertomonteromiguel requested review from a team as code owners September 7, 2026 07:50
@robertomonteromiguel
robertomonteromiguel requested review from erikayasuda and removed request for a team September 7, 2026 07:50
@dd-octo-sts

dd-octo-sts Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Hi! 👋 Thanks for your pull request! 🎉

To help us review it, please make sure to:

  • Add at least one type, and one component or instrumentation label to the pull request

If you need help, please check our contributing guidelines.

@datadog-official datadog-official Bot left a comment

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.

Datadog Autotest: FAIL

The review skill prints committed diffs before it scans them for secrets. A committed secret can enter the transcript and saved logs.

Open Bits AI session

🤖 Datadog Autotest · Commit 02ec6ec · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest

Comment thread .agents/skills/dd-apm-sdk-review/SKILL.md Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02ec6ec22e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread .agents/skills/dd-apm-sdk-review/SKILL.md Outdated
Comment thread .agents/dd-apm-sdk-review-overrides/reviewers/performance.md Outdated
Comment thread .llm-validation/README.md Outdated
Comment thread .agents/dd-apm-sdk-review-overrides/reviewers/design.md Outdated
Comment thread .llm-validation/suites/dd-apm-sdk-review.yaml
Comment thread .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md Outdated
Comment thread .agents/dd-apm-sdk-review-overrides/repo-context.md
Comment thread .agents/dd-apm-sdk-review-overrides/reviewers/performance.md Outdated
Comment thread .agents/skills/dd-apm-sdk-review/reviewers/performance.md Outdated
@pr-commenter

pr-commenter Bot commented Sep 7, 2026

Copy link
Copy Markdown

LLM Validation

LLM Validation Gate — dd-apm-sdk-review

✅ PASS

  • Overall quality improved by 8.2 points with no blocking-case regressions.
  • Advisory: the candidate introduced a bad signal on 1 case(s) that the baseline did not — surfaced for review (see Cases); it does not block on its own.

Analysis

Changed instruction file(s): .agents/skills/dd-apm-sdk-review/SKILL.md, .agents/skills/dd-apm-sdk-review/reviewers/_common.md, .agents/skills/dd-apm-sdk-review/reviewers/correctness.md, .agents/skills/dd-apm-sdk-review/reviewers/performance.md, .agents/skills/dd-apm-sdk-review/reviewers/report-template.md, .agents/skills/dd-apm-sdk-review/reviewers/coherence.md, .agents/skills/dd-apm-sdk-review/reviewers/security.md, .agents/skills/dd-apm-sdk-review/reviewers/design.md, .agents/skills/dd-apm-sdk-review/reviewers/maintainability.md, .agents/skills/dd-apm-sdk-review/reviewers/conventions.md, .agents/skills/dd-apm-sdk-review/reviewers/cross-sdk.md, .agents/dd-apm-sdk-review-overrides/repo-context.md, .agents/dd-apm-sdk-review-overrides/reviewers/security.md, .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, .agents/dd-apm-sdk-review-overrides/reviewers/design.md, .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md, .agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md.

1 case(s) dipped but stayed within the pass bar.

  • java-correctness-mapper-state-leak: won 50% of blind comparisons, quality -17.1. ⚠️ Introduced a new bad signal (advisory).
    • Asked: Run dd-apm-sdk-review on the following change. There is no git repository to inspect — this diff is the entire change set; do not attempt any git commands, go
    • Tripped 1 bad signal(s):
      • Notes the flag exists but misses that nothing re-initializes it per chunk.
    • Baseline answer: I have what I need. Note first: I could not run the actual dd-apm-sdk-review skill — its SKILL.md and per-reviewer spec files have been deleted in this worktree (git status shows them D; only a README.md mirror stub survives) and the skill isn't registered with the Skill tool. So this is a manual review
    • Candidate answer: I have everything I need. The diff was pasted inline, so per the skill I ran in single-context sequential mode (expected for pasted input, not a capability gap) and grounded each lens against the real file and its call sites. Here is the consolidated report. --- # dd-apm-sdk-review: dd-trace-java Verdict:

Results

  • Pairwise win-rate: 77% [67%–86%] — candidate's share of blind comparisons (90% CI; spanning 50% = no clear difference)
  • Overall quality: 74.6 → 82.8 (/100, +8.2)
  • Bad signals introduced (advisory): 1
  • Candidate criteria coverage (advisory): 30/33 (91%) — expected_criteria the candidate met; does not affect the gate
  • Candidate bad signals (all, advisory): 1 case(s) — trips on the candidate, including ones the baseline also hit
  • Blocking-case regressions: 0

Cases

Case Mode Quality Δ Win-rate (90% CI) Safety
java-perf-lens-wrong-collection-001 block +1.7 50% [50%–50%] ok
java-perf-pipeline-full-review-002 block +37.9 100% [100%–100%] ok
java-security-crash-handler-before-trust block +8.8 75% [51%–99%] ok
java-correctness-capture-before-send block -0.4 50% [16%–84%] ok
java-correctness-sqs-queue-name-incomplete block +15.4 100% [100%–100%] ok
java-maintainability-resource-leak-streams block +8.8 88% [67%–100%] ok
java-correctness-span-events-list-only block +10.4 100% [100%–100%] ok
java-correctness-mapper-state-leak block -17.1 50% [3%–97%] ⚠️ bad signal

Per-dimension scores, token usage, latency, and estimated cost are in the CI job logs.

@robertomonteromiguel robertomonteromiguel added comp: tooling Build & Tooling tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: feature Enhancements and improvements labels Sep 8, 2026
…kill (#12428)

* Tell no-harness reviewers to load the skill lenses

GitHub Codex cannot invoke the skill; point it at the reviewer files and a temporary debug prefix so we can see which lens it used.

Co-authored-by: Cursor <cursoragent@cursor.com>

* Drop Codex debug prefix and bait class

Keep the no-harness lens-loading fallback; remove the experiment-only review prefix and unused shouldSample helper.

Co-authored-by: Cursor <cursoragent@cursor.com>

* Move no-harness review spec out of AGENTS.md

Keep the Codex fallback as a pointer; the lens list lives in the overrides file.

Co-authored-by: Cursor <cursoragent@cursor.com>

* Restore the inline no-harness review spec in AGENTS.md

Keep the Codex fallback that already worked; drop the extracted overrides file.

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: tooling Build & Tooling tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants