From ebf675d016a84ddd15569ce399a48b054faae638 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Mon, 14 Sep 2026 16:47:59 +0200 Subject: [PATCH 01/13] chore(agents): add starter dd-trace-php overrides and eval case One PSR-2 convention rule, a short AGENTS.md hook, and one llm-validation case. Co-authored-by: Cursor --- .../repo-context.md | 7 ++++ .../reviewers/conventions.md | 11 ++++++ .gitlab-ci.yml | 11 ++++++ .llm-validation/README.md | 15 ++++++++ .llm-validation/config.yaml | 33 ++++++++++++++++ .llm-validation/suites/dd-apm-sdk-review.yaml | 38 +++++++++++++++++++ AGENTS.md | 12 ++++++ 7 files changed, 127 insertions(+) create mode 100644 .agents/dd-apm-sdk-review-overrides/repo-context.md create mode 100644 .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md create mode 100644 .llm-validation/README.md create mode 100644 .llm-validation/config.yaml create mode 100644 .llm-validation/suites/dd-apm-sdk-review.yaml create mode 100644 AGENTS.md diff --git a/.agents/dd-apm-sdk-review-overrides/repo-context.md b/.agents/dd-apm-sdk-review-overrides/repo-context.md new file mode 100644 index 0000000000..a1849f0555 --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/repo-context.md @@ -0,0 +1,7 @@ +# Repo context — dd-trace-php + +Read only by the orchestrator (Step 0 of `SKILL.md`), not by individual reviewers. Repo-specific; not part of the shared core. + +## Related skills in this repo + +This repo has no other agent skills yet. Omit a "Related skills" list until one exists. diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md b/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md new file mode 100644 index 0000000000..41edcb45b0 --- /dev/null +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md @@ -0,0 +1,11 @@ +Override for `reviewers/conventions.md` (in the core skill folder) — read that file first, then this. + +# Conventions — dd-trace-php specifics + +This file starts with one confirmed pattern and should grow. Do not treat it as exhaustive. + +The source of truth is [`CONTRIBUTING.md`](../../../CONTRIBUTING.md) § "PHP linting". Apply that section as written. + +## PHP userland follows PSR-2 + +PHP under this repo must follow [PSR-2](https://www.php-fig.org/psr/psr-2/). Style is checked with `composer lint` and auto-fixed with `composer fix-lint`. A new or edited `.php` file that uses Allman braces, tabs for indent, or otherwise fails that check is a finding. Do not invent a different house style. diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a6ef68e5ce..5da33a4379 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -17,6 +17,17 @@ variables: description: "Run a specific datadog-reliability-env branch downstream" SYSTEM_TESTS_LIBRARY: php +include: + - project: "ddoghq/llm-validation-platform" + ref: &llmval_platform_sha "c331696647a78672101002ba48ccebbd41b0fae8" + file: "/ci/llm-validation.gitlab-ci.yml" + +"llm validation": + variables: + LLMVAL_PLATFORM_PROJECT: "ddoghq/llm-validation-platform" + LLMVAL_PLATFORM_REF: *llmval_platform_sha + LLMVAL_BASE_REF: "master" + generate-templates: stage: build image: registry.ddbuild.io/images/mirror/php:8.2-cli diff --git a/.llm-validation/README.md b/.llm-validation/README.md new file mode 100644 index 0000000000..3f3a82a402 --- /dev/null +++ b/.llm-validation/README.md @@ -0,0 +1,15 @@ +# LLM Validation — `dd-apm-sdk-review` + +This folder is how we test the review skill. It is **not** a PHPUnit run. +The cases live here; the runner lives in [`ddoghq/llm-validation-platform`](https://github.com/ddoghq/llm-validation-platform). + +Same gate as the other tracer repos. One starter case on purpose. + +## Add a rule + +1. Extend [`.agents/dd-apm-sdk-review-overrides/reviewers/`](../.agents/dd-apm-sdk-review-overrides/reviewers/). +2. Add the path to `instruction_files` in [`config.yaml`](./config.yaml). +3. Copy the starter case in [`suites/dd-apm-sdk-review.yaml`](./suites/dd-apm-sdk-review.yaml). +4. List the case id under `presets.gate.cases` if you want CI to run it. + +Never edit `.agents/skills/dd-apm-sdk-review/` in this repo. diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml new file mode 100644 index 0000000000..9206845427 --- /dev/null +++ b/.llm-validation/config.yaml @@ -0,0 +1,33 @@ +# LLM Validation Platform — dd-trace-php config +# +# Starter suite for the dd-apm-sdk-review skill. One case on purpose. +model: claude-sonnet-5 +runs: 2 + +instruction_files: + - AGENTS.md + - .agents/skills/dd-apm-sdk-review/SKILL.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/conventions.md + - .agents/skills/dd-apm-sdk-review/review-without-harness.md + - .agents/dd-apm-sdk-review-overrides/repo-context.md + - .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md + +default_level: gate +presets: + gate: + cases: + - php-conventions-psr2 + runs: 2 + minimum: + cases: + - php-conventions-psr2 + runs: 2 + full: + runs: 2 + +policy: + noise_threshold: 1.0 + pairwise_win_floor: 0.45 + blocking_fail_floor: 0.45 + blocking_fail_ci_upper: 0.55 diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml new file mode 100644 index 0000000000..04976f5098 --- /dev/null +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -0,0 +1,38 @@ +name: dd-apm-sdk-review +version: "0.1" + +# One file on purpose: the platform CLI errors if suites/ has more than one YAML. +# One starter case — a template. Copy this block to add the next one. +cases: + - id: php-conventions-psr2 + files: + - CONTRIBUTING.md + - .agents/skills/dd-apm-sdk-review/reviewers/conventions.md + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md + input: | + Apply this repo's conventions-review criteria (reviewers/conventions.md, its + .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md override, _common.md, + and CONTRIBUTING.md § "PHP linting") to the following change. No git checkout + available — this snippet is the entire change to review. + + ```php + Date: Mon, 14 Sep 2026 18:07:45 +0200 Subject: [PATCH 02/13] chore(agents): point Codex at the skill directory from AGENTS.md GitHub Codex follows markdown links in AGENTS.md. Link the skill folder (same hook as js/go/rb) and review-without-harness.md so the review spec lands in the GitHub review context. Co-authored-by: Cursor --- AGENTS.md | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e8422fd789..17f9ea4550 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,11 +2,16 @@ ## Review Guidelines -Before pushing or opening a PR, run the [dd-apm-sdk-review](./.agents/skills/dd-apm-sdk-review/SKILL.md) -skill on your changes. Fix blocking findings first, or get explicit authorization from the human you -work with and record them verbatim in the PR description. Never post security findings in a PR -description; route them through [SECURITY.md](SECURITY.md). +**Local agent with a skill harness:** Before pushing code or creating a pull request, you must run +the [dd-apm-sdk-review](./.agents/skills/dd-apm-sdk-review/) skill on your unpushed changes. If any +`P0` issues are reported, you must either fix them or get explicit authorization from the human you +are working with and record the unresolved finding verbatim in the PR description. `P1` and `P2` +findings should be fixed before pushing, but can be dismissed by the human. + +Exception: security findings are never pasted into a PR description — a PR is a public forum, so +posting one there is an improper disclosure. Route them privately +(see [SECURITY.md](SECURITY.md)). **Reviewer without a skill harness** (for example, GitHub Codex): read and follow -`.agents/skills/dd-apm-sdk-review/review-without-harness.md`. Do not load `SKILL.md` -or `reviewers/report-template.md`. +[`.agents/skills/dd-apm-sdk-review/review-without-harness.md`](./.agents/skills/dd-apm-sdk-review/review-without-harness.md). +Do not load `SKILL.md` or `reviewers/report-template.md`. From 27e6f2d3d249b4da89667876ea138d039da9ef1b Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Mon, 14 Sep 2026 18:09:32 +0200 Subject: [PATCH 03/13] chore(agents): add .llm-validation README matching the other tracers Document how to add a review-skill case and how to run the suite locally, same shape as dd-trace-go / dd-trace-rb. --- .llm-validation/README.md | 71 +++++++++++++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 7 deletions(-) diff --git a/.llm-validation/README.md b/.llm-validation/README.md index 3f3a82a402..7938b33cb2 100644 --- a/.llm-validation/README.md +++ b/.llm-validation/README.md @@ -3,13 +3,70 @@ This folder is how we test the review skill. It is **not** a PHPUnit run. The cases live here; the runner lives in [`ddoghq/llm-validation-platform`](https://github.com/ddoghq/llm-validation-platform). -Same gate as the other tracer repos. One starter case on purpose. +Same gate as [`dd-trace-js#10137`](https://github.com/DataDog/dd-trace-js/pull/10137) and +[`dd-trace-java#12409`](https://github.com/DataDog/dd-trace-java/pull/12409). -## Add a rule +It answers: *did an edit to a review rule make the agent better or worse?* -1. Extend [`.agents/dd-apm-sdk-review-overrides/reviewers/`](../.agents/dd-apm-sdk-review-overrides/reviewers/). -2. Add the path to `instruction_files` in [`config.yaml`](./config.yaml). -3. Copy the starter case in [`suites/dd-apm-sdk-review.yaml`](./suites/dd-apm-sdk-review.yaml). -4. List the case id under `presets.gate.cases` if you want CI to run it. +## Add a rule (this is the whole contribution) -Never edit `.agents/skills/dd-apm-sdk-review/` in this repo. +Overrides are owned by this repo. The shared core is not — never edit `.agents/skills/dd-apm-sdk-review/`. + +1. Create or extend a file under [`.agents/dd-apm-sdk-review-overrides/reviewers/`](../.agents/dd-apm-sdk-review-overrides/reviewers/). + Copy the shape of `conventions.md`: one pattern, why it matters, the fix. +2. Add the new override path to `instruction_files` in [`config.yaml`](./config.yaml) so CI + watches it. +3. Add a case in [`suites/dd-apm-sdk-review.yaml`](./suites/dd-apm-sdk-review.yaml). Copy the starter case. + A good case is a 10-line snippet plus 2–3 `expected_criteria` that would fail if the rule disappeared. +4. List the new case id under `presets.gate.cases` in [`config.yaml`](./config.yaml) if you want CI to run it. +5. Open a PR. That is it. + +The starter case in this folder is the example. Keep new ones that short. + +## Layout + +| Path | Role | +|---|---| +| [`config.yaml`](./config.yaml) | Monitored instruction files, model, `--level` presets | +| [`suites/dd-apm-sdk-review.yaml`](./suites/dd-apm-sdk-review.yaml) | Cases (one file only — the CLI errors if `suites/` has more than one YAML) | + +## Run locally (Docker) + +From the **`dd-trace-php` repo root**: + +```bash +export LLMVAL_IMAGE=registry.ddbuild.io/ci/llm-validation-platform/llmval:latest +docker pull "$LLMVAL_IMAGE" + +# Offline smoke — no gateway, no Claude (1 case) +docker run --rm -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level minimum --fake + +# Cheap real smoke — still 1 case +export LLMVAL_AUTH_HEADER="$(ddtool auth token rapid-ai-platform --datacenter us1.staging.dog --http-header)" +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level minimum --runs 1 + +# The gate preset (starter case; add more ids in config.yaml to grow it) +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --level gate --runs 1 + +# One named case +docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ + --repo /repo --base-sha master --case php-conventions-psr2 --runs 1 +``` + +`--level` picks **which cases** run (`minimum` / `gate` = the starter case until you add more, `full` = every case). +`--runs` only repeats those cases. Needs `ddtool` on the host for a real (non-`--fake`) run. + +CI includes the reusable `"llm validation"` job from the platform repo (see `.gitlab-ci.yml`). +This repo's default branch is `master`, so the job sets `LLMVAL_BASE_REF: "master"`. + +## What a pass means + +This is an A/B comparison, not an absolute score: + +- **Candidate** = the working tree. Uncommitted edits count. +- **Baseline** = `git show :`. A file that is not on `master` yet is treated as added. + +The gate fails only on a **confident regression**. Noisy changes WARN and do not block. From fb69c9011ccfbd0ac55d1ac8dd123bc3488dd180 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Mon, 14 Sep 2026 18:12:30 +0200 Subject: [PATCH 04/13] chore(agents): align llm-validation watch list and repo hook files Watch the full skill tree like go/rb, ignore local eval artifacts, and give PHP an explicit CODEOWNERS + CONTRIBUTING pointer for the gate. --- .github/CODEOWNERS | 5 +++++ .gitignore | 5 +++++ .llm-validation/config.yaml | 8 ++++++++ CONTRIBUTING.md | 4 ++++ 4 files changed, 22 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 56f470af44..55b1c86800 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,6 +1,11 @@ # Default owners * @DataDog/apm-php @DataDog/apm-php-core +# Agent review skill and eval suite (explicit so MergeGate shows an owner) +/.agents/ @DataDog/apm-php @DataDog/apm-php-core +/.llm-validation/ @DataDog/apm-php @DataDog/apm-php-core +/AGENTS.md @DataDog/apm-php @DataDog/apm-php-core + # Profiling team /profiling/ @DataDog/profiling-php @DataDog/apm-php-core diff --git a/.gitignore b/.gitignore index 6539a75e64..65d9f5fa34 100644 --- a/.gitignore +++ b/.gitignore @@ -72,3 +72,8 @@ tests/tested_versions .gitlab/*-gen.yml .gitlab-ci-local/ /.claude/settings.local.json + +# LLM Validation local/CI artifacts +.llm-validation/results.json +.llm-validation/report.md +.llm-validation/details.json diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml index 9206845427..4d255193ce 100644 --- a/.llm-validation/config.yaml +++ b/.llm-validation/config.yaml @@ -8,7 +8,15 @@ instruction_files: - AGENTS.md - .agents/skills/dd-apm-sdk-review/SKILL.md - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/coherence.md + - .agents/skills/dd-apm-sdk-review/reviewers/correctness.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/performance.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/skills/dd-apm-sdk-review/reviewers/report-template.md - .agents/skills/dd-apm-sdk-review/review-without-harness.md - .agents/dd-apm-sdk-review-overrides/repo-context.md - .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fec4f8f28a..c69f1ac5ab 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,6 +26,10 @@ The easiest way to get the development environment set up is to install [Docker] ## Developing and testing locally +### LLM Validation + +GitLab job `"llm validation"` from [`ddoghq/llm-validation-platform`](https://github.com/ddoghq/llm-validation-platform). It is an A/B eval of the `dd-apm-sdk-review` skill, not a PHPUnit run. Cases live in [`.llm-validation/`](./.llm-validation/); how to add one is in [`.llm-validation/README.md`](./.llm-validation/README.md). + ### PHP linting The PHP tracer conforms to the [PSR-2 coding style guide](https://www.php-fig.org/psr/psr-2/). The code style is checked with [PHP_CodeSniffer](https://github.com/squizlabs/PHP_CodeSniffer) which can be invoked with the following command: From d19d4b01e0477fde6ab0c2c0eb7646d38fea7685 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 08:28:34 +0200 Subject: [PATCH 05/13] chore(agents): run llm-validation on the shared BP-tools image Match the platform template and the other tracer PRs so this job does not inherit this repo's PHP CI image. Co-authored-by: Cursor --- .gitlab-ci.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5da33a4379..908158192a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -23,7 +23,10 @@ include: file: "/ci/llm-validation.gitlab-ci.yml" "llm validation": + # Same image as the platform template and the other tracer llm-validation jobs. + image: registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:newest variables: + LLMVAL_IMAGE: "registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:newest" LLMVAL_PLATFORM_PROJECT: "ddoghq/llm-validation-platform" LLMVAL_PLATFORM_REF: *llmval_platform_sha LLMVAL_BASE_REF: "master" From 8066e775ffe5c13530aa902758dc6385f2a1ed07 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 08:47:54 +0200 Subject: [PATCH 06/13] chore(agents): list sibling skills and score PHP style against phpcs Repo-context omitted check-ci, crash-analysis, and release-notes. The conventions override and starter case now follow phpcs.xml (PSR-12) instead of the superseded PSR-2 label. Co-authored-by: Cursor --- .agents/dd-apm-sdk-review-overrides/repo-context.md | 8 +++++++- .../reviewers/conventions.md | 8 +++++--- .llm-validation/suites/dd-apm-sdk-review.yaml | 13 +++++++------ 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/.agents/dd-apm-sdk-review-overrides/repo-context.md b/.agents/dd-apm-sdk-review-overrides/repo-context.md index a1849f0555..cda085ee8a 100644 --- a/.agents/dd-apm-sdk-review-overrides/repo-context.md +++ b/.agents/dd-apm-sdk-review-overrides/repo-context.md @@ -4,4 +4,10 @@ Read only by the orchestrator (Step 0 of `SKILL.md`), not by individual reviewer ## Related skills in this repo -This repo has no other agent skills yet. Omit a "Related skills" list until one exists. +Existing skills live under `.claude/skills/`. Cite them as authoritative for their area. Do not invoke them, and they must not invoke this skill: + +- `check-ci` — GitLab CI / GitHub Actions watch and failure investigation +- `crash-analysis` — wild crash reports (`event.json`) for this tracer +- `release-notes` — `CHANGELOG.md` for a minor/major release + +`.claude/skills/dd-apm-sdk-review` is a symlink to `.agents/skills/dd-apm-sdk-review`. No name clash. diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md b/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md index 41edcb45b0..b1070e3461 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/conventions.md @@ -4,8 +4,10 @@ Override for `reviewers/conventions.md` (in the core skill folder) — read that This file starts with one confirmed pattern and should grow. Do not treat it as exhaustive. -The source of truth is [`CONTRIBUTING.md`](../../../CONTRIBUTING.md) § "PHP linting". Apply that section as written. +The source of truth for *how* style is checked is [`phpcs.xml`](../../../phpcs.xml) via `composer lint` / `composer fix-lint`. [`CONTRIBUTING.md`](../../../CONTRIBUTING.md) § "PHP linting" still names PSR-2; the ruleset that command actually runs is [PSR-12](https://www.php-fig.org/psr/psr-12/). When those disagree, `phpcs.xml` wins. -## PHP userland follows PSR-2 +## PHP userland follows the phpcs ruleset (PSR-12) -PHP under this repo must follow [PSR-2](https://www.php-fig.org/psr/psr-2/). Style is checked with `composer lint` and auto-fixed with `composer fix-lint`. A new or edited `.php` file that uses Allman braces, tabs for indent, or otherwise fails that check is a finding. Do not invent a different house style. +A new or edited `.php` file that fails `composer lint` is a finding (Allman braces and tab indent fail that check). Do not invent a different house style than `phpcs.xml`. + +CI does **not** run `composer lint`. It runs `composer ci-lint`, a separate nearly-empty gate (same CONTRIBUTING section). That is not "this repo has no PHP style standard" — the standard is still `composer lint` / `phpcs.xml`. diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 04976f5098..fb2039985c 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -7,14 +7,15 @@ cases: - id: php-conventions-psr2 files: - CONTRIBUTING.md + - phpcs.xml - .agents/skills/dd-apm-sdk-review/reviewers/conventions.md - .agents/skills/dd-apm-sdk-review/reviewers/_common.md - .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md input: | Apply this repo's conventions-review criteria (reviewers/conventions.md, its .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md override, _common.md, - and CONTRIBUTING.md § "PHP linting") to the following change. No git checkout - available — this snippet is the entire change to review. + CONTRIBUTING.md § "PHP linting", and phpcs.xml) to the following change. No git + checkout available — this snippet is the entire change to review. ```php Date: Tue, 15 Sep 2026 10:14:12 +0200 Subject: [PATCH 07/13] chore(ci): skip submodules on the llm-validation job PHP defaults to recursive checkout; JS/Java do not. Match them so this job does not clone GitHub before run.sh hits NodeSource. Co-authored-by: Cursor --- .gitlab-ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 908158192a..f83ec67f25 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -23,13 +23,13 @@ include: file: "/ci/llm-validation.gitlab-ci.yml" "llm validation": - # Same image as the platform template and the other tracer llm-validation jobs. - image: registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:newest variables: - LLMVAL_IMAGE: "registry.ddbuild.io/images/benchmarking-platform-tools-ubuntu:newest" LLMVAL_PLATFORM_PROJECT: "ddoghq/llm-validation-platform" LLMVAL_PLATFORM_REF: *llmval_platform_sha LLMVAL_BASE_REF: "master" + # Repo-wide default is recursive (libdatadog, etc.). JS/Java skip submodules + # on this job; inherit that so the gate does not clone GitHub before run.sh. + GIT_SUBMODULE_STRATEGY: none generate-templates: stage: build From f4e0fdd5e18935d693411372f14f2e97125f1cce Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 10:41:54 +0200 Subject: [PATCH 08/13] chore(ci): bypass fabric-proxy for llm-validation node install The dd-trace-php service account 403s deb.nodesource.com. Send NodeSource, npm, and dot.net around the proxy for this job. Co-authored-by: Cursor --- .gitlab-ci.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f83ec67f25..d989ba33f1 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -30,6 +30,9 @@ include: # Repo-wide default is recursive (libdatadog, etc.). JS/Java skip submodules # on this job; inherit that so the gate does not clone GitHub before run.sh. GIT_SUBMODULE_STRATEGY: none + # dd-trace-php's fabric-proxy 403s NodeSource/npm/dot.net; JS/Java SAs allow + # them. Bypass the proxy for the hosts run.sh needs (CIEXE-714 annotation). + KUBERNETES_POD_ANNOTATIONS_NODESOURCE: "beta.fabric.datadoghq.com/no-proxy-additions=deb.nodesource.com,registry.npmjs.org,dot.net" generate-templates: stage: build From cdf48c4ece625c5a2341c718605547b3516e01d4 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 10:50:44 +0200 Subject: [PATCH 09/13] chore(ci): also bypass fabric for the .NET install redirect dot.net 301s to builds.dotnet.microsoft.com; that host was still proxied and returned 403 after NodeSource started working. Co-authored-by: Cursor --- .gitlab-ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d989ba33f1..6895c08aef 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -32,7 +32,8 @@ include: GIT_SUBMODULE_STRATEGY: none # dd-trace-php's fabric-proxy 403s NodeSource/npm/dot.net; JS/Java SAs allow # them. Bypass the proxy for the hosts run.sh needs (CIEXE-714 annotation). - KUBERNETES_POD_ANNOTATIONS_NODESOURCE: "beta.fabric.datadoghq.com/no-proxy-additions=deb.nodesource.com,registry.npmjs.org,dot.net" + # dot.net 301s to builds.dotnet.microsoft.com; the install script also uses aka.ms. + KUBERNETES_POD_ANNOTATIONS_NODESOURCE: "beta.fabric.datadoghq.com/no-proxy-additions=deb.nodesource.com,registry.npmjs.org,dot.net,builds.dotnet.microsoft.com,aka.ms" generate-templates: stage: build From 26fe899a5e44891029ea75a333865907ba6d6de9 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 17:43:47 +0200 Subject: [PATCH 10/13] chore(agents): drop the security lens and the pre-push hook Codex security covers that lane. Local skill runs on demand; GitHub Codex follows review-without-harness.md. --- .agents/skills/dd-apm-sdk-review/SKILL.md | 76 +++++++++++++------ .../review-without-harness.md | 14 +--- .../dd-apm-sdk-review/reviewers/README.md | 7 +- .../reviewers/correctness.md | 2 - .../reviewers/performance.md | 1 - .../reviewers/report-template.md | 18 +++-- .../dd-apm-sdk-review/reviewers/security.md | 38 ---------- .llm-validation/config.yaml | 1 - AGENTS.md | 9 +-- 9 files changed, 71 insertions(+), 95 deletions(-) delete mode 100644 .agents/skills/dd-apm-sdk-review/reviewers/security.md diff --git a/.agents/skills/dd-apm-sdk-review/SKILL.md b/.agents/skills/dd-apm-sdk-review/SKILL.md index b8726310bc..b757a77263 100644 --- a/.agents/skills/dd-apm-sdk-review/SKILL.md +++ b/.agents/skills/dd-apm-sdk-review/SKILL.md @@ -1,6 +1,6 @@ --- name: dd-apm-sdk-review -description: "ALWAYS USE BEFORE PUSHING CODE! Multi-perspective read-only review of changes in this tracer repo, consolidated into one report with an explicit go / no-go verdict." +description: "On-demand multi-perspective read-only review of changes in this tracer repo, consolidated into one report with an explicit go / no-go verdict. Invoke when asked. GitHub Codex does not use this file — it follows review-without-harness.md via AGENTS.md." model: opus effort: high allowed-tools: @@ -15,7 +15,7 @@ allowed-tools: You are the **orchestrator**. You do not review the code yourself. You determine what changed, delegate to the reviewers in the roster below, then consolidate. -If this skill is invoked twice in a row on the same set of changes **and the prior invocation actually completed with a verdict**, let the user know and no-op this skill. This is intentionally expensive as it is intended as a push gate. A prior run that was interrupted, timed out, or reported `NOT VERIFIED`/`review not performed` did not complete — always retry in that case rather than no-oping. +If this skill is invoked twice in a row on the same set of changes **and the prior invocation actually completed with a verdict**, let the user know and no-op this skill. This is intentionally expensive. A prior run that was interrupted, timed out, or reported `NOT VERIFIED`/`review not performed` did not complete — always retry in that case rather than no-oping. ## Step 0 — Load repo context @@ -45,19 +45,24 @@ protect you (the orchestrator) after you have ingested this output. ```bash # 1. Resolve the TARGET: the commit this work will merge INTO. Never @{u} - that # is this same branch on the remote, so once you have pushed, the merge base -# is HEAD and the diff comes back empty. Never assume the trunk either: on a -# stacked branch the parent is another feature branch. Never build "origin/" +# is HEAD and the diff comes back empty. Never build "origin/" # from baseRefName either: on a cross-repo PR, `origin` is the contributor's fork, # not the base repository, so that name can resolve to a stale fork branch or nothing. -# baseRefOid is the base repository's actual commit and has no such ambiguity. -# Pin --repo to a DataDog remote (upstream, then origin) so a fork checkout -# cannot resolve the wrong GitHub repository. Do not hardcode a tracer name. +# Prefer a PR's baseRefOid (the base repository's actual commit). If there is +# no PR yet — a local on-demand review before a PR exists — infer the default branch +# from the DataDog remote (upstream, then origin): remote/HEAD, then +# remote/master, then remote/main. Print that inferred target so a stacked +# branch can correct it. Only ask when none of those refs exist. +# Pin --repo to a DataDog remote so a fork checkout cannot resolve the +# wrong GitHub repository. Do not hardcode a tracer name. GH_REPO="" +DD_REMOTE="" for remote in upstream origin; do url=$(git remote get-url "$remote" 2>/dev/null) || continue case "$url" in *github.com[:/]DataDog/*) GH_REPO=$(printf '%s\n' "$url" | sed -E 's#.*github.com[:/](DataDog/[^/.]+).*#\1#') + DD_REMOTE=$remote break ;; esac @@ -70,11 +75,24 @@ fi TARGET=$(echo "$PR_JSON" | jq -r '.baseRefOid' 2>/dev/null) BASE_REF_NAME=$(echo "$PR_JSON" | jq -r '.baseRefName' 2>/dev/null) if [ -z "$TARGET" ] || [ "$TARGET" = "null" ]; then - # No PR yet, or gh failed to resolve one (e.g. a stacked branch with no PR open): - # do NOT silently fall back to origin/master. Stop and ask the human/agent to - # confirm the actual merge target before computing any diff or running reviewers. - echo "Could not resolve a PR base branch — what is the actual merge target for this branch (e.g. a parent feature branch on a stacked PR)?" - exit 1 + # No PR yet (or gh could not resolve one). Infer the repo default branch so + # a local-only, not-yet-pushed branch still reviews. Never @{u}. Print the + # inferred target — a stacked branch against a parent feature can correct + # it. Only ask when none of those refs exist locally. + infer_remote="${DD_REMOTE:-origin}" + TARGET="" + BASE_REF_NAME="" + for cand in "$infer_remote/HEAD" "$infer_remote/master" "$infer_remote/main"; do + oid=$(git rev-parse --verify "$cand" 2>/dev/null) || continue + TARGET=$oid + BASE_REF_NAME=$(git rev-parse --abbrev-ref "$cand" 2>/dev/null) + break + done + if [ -z "$TARGET" ]; then + echo "Could not infer a merge target (no PR and no ${infer_remote}/HEAD|master|main) — what is the actual merge target for this branch (e.g. a parent feature branch on a stacked PR)?" + exit 1 + fi + echo "inferred merge target (no PR): $BASE_REF_NAME ($TARGET) — say this in the report; ask if it looks wrong (stacked PR / non-default base)" fi # PR title and labels: on an existing PR, some reviewer overrides (e.g. release-note # policy, semver labels) audit these directly. Empty on a not-yet-opened PR - that's @@ -97,6 +115,9 @@ SECRET_GREP='-----BEGIN [A-Z ]*PRIVATE KEY-----|AKIA[0-9A-Z]{16}|ASIA[0-9A-Z]{16 # every change set as already-scanned. err_file=$(mktemp) || { echo "ERROR: mktemp failed, cannot safely scan diffs" >&2; exit 1; } trap 'rm -f "$err_file"' EXIT +# A match must survive the function return so Step 2 cannot APPROVE a redacted +# change set. 0 until a scan hits; the block after the untracked loop checks it. +SECRET_FOUND=0 # Capture a command's stdout to a temp file, grep it, and only then print. # A match (or a grep error) suppresses the body — fail closed, same as the @@ -117,14 +138,16 @@ emit_diff_or_redact() { local grc=$? if [ "$grc" -eq 0 ]; then echo "SUSPECT SECRET (not printed): $label - read it yourself, redact, then decide" + SECRET_FOUND=1 rm -f "$out" - return 0 + return 1 elif [ "$grc" -ge 2 ]; then echo "ERROR: could not scan $label for secrets - treating as suspect rather than skipping the scan" >&2 cat "$err_file" >&2 echo "SUSPECT SECRET (not printed): $label - read it yourself, redact, then decide" + SECRET_FOUND=1 rm -f "$out" - return 0 + return 1 fi cat "$out" rm -f "$out" @@ -137,10 +160,11 @@ echo "reviewing against: $BASE_REF_NAME ($TARGET)" # say this in the report # 2. Committed delta against the merge base with that target git rev-parse --is-shallow-repository # if true, merge-base may not resolve -BASE=$(git merge-base HEAD "$TARGET" 2>/dev/null) -if [ -n "$BASE" ]; then +if BASE=$(git merge-base HEAD "$TARGET" 2>/dev/null) && [ -n "$BASE" ]; then git diff --stat "$BASE"...HEAD emit_diff_or_redact "committed $BASE...HEAD" git diff "$BASE"...HEAD +else + echo "WARNING: could not resolve a merge base with $TARGET (shallow clone or missing history) — committed delta not captured; report as NOT VERIFIED (no merge base) unless more history is fetched" >&2 fi # 3. Uncommitted work: the file list AND the contents. `git status` alone gives @@ -171,17 +195,20 @@ while IFS= read -r -d '' f; do grc=$? if [ "$grc" -eq 0 ]; then echo "SUSPECT SECRET (diff not printed): $f - read it yourself, redact, then decide" + SECRET_FOUND=1 continue elif [ "$grc" -ge 2 ]; then echo "ERROR: could not scan $f for secrets - treating as suspect rather than skipping the scan" >&2 cat "$err_file" >&2 echo "SUSPECT SECRET (diff not printed): $f - read it yourself, redact, then decide" + SECRET_FOUND=1 continue fi # grep -I would have skipped this file. If it is binary, do not print a # "clean" diff — fail closed and ask for out-of-band inspection. if ! grep -Iq . -- "./$f" && grep -aq . -- "./$f"; then echo "SUSPECT SECRET (diff not printed): $f - binary file, inspect out of band" + SECRET_FOUND=1 continue fi # `--no-index` exits 1 when it finds a difference, which it always will here - @@ -201,6 +228,10 @@ while IFS= read -r -d '' f; do exit 1 fi done < <(git ls-files --others --exclude-standard -z) +if [ "${SECRET_FOUND:-0}" -ne 0 ]; then + echo "ERROR: suspect secret in the change set — do not proceed to Step 2; route privately; do not APPROVE" >&2 + exit 1 +fi ``` The grep above only catches known secret *shapes* (cloud keys, tokens with a recognizable prefix, PEM headers) — it is not a substitute for reading the output. Read each printed diff as it is produced (or read the file directly instead of shelling out) and check it for tokens, API keys, private keys, connection strings, `.env` values, and anything shaped like a long random secret that the pattern missed, before letting that output stand in your context. If a file looks like a credential — including one the grep already flagged as a suspect and skipped — redact the value at first sight — `[REDACTED — see location]`, keeping the `path:line` — and treat the printed diff as already-redacted from that point on; never diff a flagged file unredacted just to get around the flag. PR title, labels, recent commit subjects, and committed / staged / unstaged diffs are pre-scanned by `emit_diff_or_redact` before they are printed; still scan what *does* print as you read it. @@ -226,7 +257,6 @@ Also note, for the reviewers' benefit: |---|---|---| | Coherence | [reviewers/coherence.md](./reviewers/coherence.md) | — (fully language-agnostic) | | Correctness | [reviewers/correctness.md](./reviewers/correctness.md) | — (fully language-agnostic) | -| Security | [reviewers/security.md](./reviewers/security.md) | `.agents/dd-apm-sdk-review-overrides/reviewers/security.md` | | Design | [reviewers/design.md](./reviewers/design.md) | `.agents/dd-apm-sdk-review-overrides/reviewers/design.md` | | Performance | [reviewers/performance.md](./reviewers/performance.md) | `.agents/dd-apm-sdk-review-overrides/reviewers/performance.md` | | Maintainability | [reviewers/maintainability.md](./reviewers/maintainability.md) | `.agents/dd-apm-sdk-review-overrides/reviewers/maintainability.md` | @@ -247,7 +277,7 @@ As you resolve this roster (checking, for each lens, whether its override file e Two lenses are the exception: **Codebase conventions** needs to run a repo-defined check-only command (e.g. a formatter's check mode) to verify formatting, and **Cross-SDK consistency** needs `gh` or another read-only network lookup to compare against other SDKs. Neither can do its stated job on `Read`/`Grep`/`Glob` alone. Grant exactly those two reviewers a narrowly scoped, non-mutating `Bash` (or equivalent) restricted to the specific check-only commands their override names — never a general shell — or, if your harness can't scope `Bash` that tightly, have the orchestrator run those specific commands itself in Step 1 and pass the results into the reviewer's prompt instead of granting it a tool. Do not let either lens silently degrade to `NOT VERIFIED` just because the default restriction was applied uniformly: `NOT VERIFIED` never blocks the gate, so an unscoped blanket restriction here quietly removes formatting and cross-SDK verification from every review. If your harness has no per-subagent tool scoping at all, note that as a capability gap in the report rather than silently running reviewers unrestricted. -**Before you hand anything over, confirm the diff is free of secrets.** You should already have redacted anything credential-shaped as you read Step 1's output (see the note there — redacting only at delegation time is too late, since the value already sat in your own context first). Treat this as a second pass, not the first: re-check the change set you are about to hand to reviewers for tokens, API keys, private keys, connection strings, `.env` values, and anything shaped like a long random secret, and replace each with `[REDACTED — see location]` (keeping the `path:line`) before delegating. Report any leak by location, tell the human immediately, and route it through this repo's disclosure process: a committed credential needs rotating, not just deleting. Never paste the value into the report, a PR, or a reviewer prompt. +**Before you hand anything over, confirm the diff is free of secrets.** If Step 1 printed `ERROR: suspect secret` or exited non-zero for that reason, **stop**: do not run Step 2, do not emit `APPROVE` / `READY TO PUSH`, and route privately per this repo's disclosure process. You should already have redacted anything credential-shaped as you read Step 1's output (see the note there — redacting only at delegation time is too late, since the value already sat in your own context first). Treat this as a second pass, not the first: re-check the change set you are about to hand to reviewers for tokens, API keys, private keys, connection strings, `.env` values, and anything shaped like a long random secret, and replace each with `[REDACTED — see location]` (keeping the `path:line`) before delegating. Report any leak by location, tell the human immediately, and route it through this repo's disclosure process: a committed credential needs rotating, not just deleting. Never paste the value into the report, a PR, or a reviewer prompt. Give every reviewer: @@ -267,23 +297,21 @@ Collect their reports. Then: A reviewer that could not do its job reports `NOT VERIFIED ()` for its area. `NOT VERIFIED` never blocks. -Follow the report format in [reviewers/report-template.md](./reviewers/report-template.md), then state the gate line from that file's verdict table: `DO NOT PUSH` on `BLOCK`, `READY TO PUSH` on `APPROVE`, `WAITING ON HUMAN` on `APPROVE_WITH_COMMENTS`. On `APPROVE_WITH_COMMENTS`, show the P1 and P2 findings and ask whether to fix or dismiss them; do not emit `READY TO PUSH` or `DO NOT PUSH` until the human answers. Dismissal is the human's call, never a default. +Follow the report format in [reviewers/report-template.md](./reviewers/report-template.md): gate line first (`DO NOT PUSH` on `BLOCK`, `READY TO PUSH` on `APPROVE`, `WAITING ON HUMAN` on `APPROVE_WITH_COMMENTS`), and omit every empty section. On `APPROVE_WITH_COMMENTS`, show the P1 and P2 findings and the fix-or-dismiss question immediately under that gate line — not after bookkeeping; do not emit `READY TO PUSH` or `DO NOT PUSH` until the human answers. Dismissal is the human's call, never a default. ## Step 4 — Fix and re-review -Offer to fix the P0 and P1 findings. After fixes, re-run **every reviewer**, on the updated change set — not just the one that reported it. A security fix can add a hot-path allocation or new coupling, so a performance or design approval given against the pre-fix diff no longer applies. Repeat until the verdict is not `BLOCK`, or until the user decides to override. +Offer to fix the P0 and P1 findings. After fixes, re-run **every reviewer**, on the updated change set — not just the one that reported it. A fix can add a hot-path allocation or new coupling, so a performance or design approval given against the pre-fix diff no longer applies. Repeat until the verdict is not `BLOCK`, or until the user decides to override. If the user overrides an unresolved P0 finding, record it verbatim in the PR description. Do not silently drop it. -**Never do that for a finding from the security reviewer.** A PR description is a public or wide-audience forum, so writing an unfixed vulnerability there pre-discloses it. Route it through this repo's vulnerability disclosure process and note in the PR only that a security finding requires private routing. Never write that it *was* routed unless a handoff has actually happened: reporting the finding to the orchestrator is not disclosure. Either send it to the address this repo's disclosure policy names, or tell the human explicitly that the handoff is theirs to make, and say which of those you did. This applies to the report you print, too: state only that a security finding requires private routing — no location, no failure mode, no reproduction. - ## Scope and escape hatches -This review is required for code-bearing changes. "Code-bearing" means anything shipped to users, plus tests, benchmarks, developer tooling, CI configuration, and agent instructions under `.agents/` / `.claude/` (or wherever else a repo mirrors its skills for a specific editor/agent, e.g. `.cursor/`). Tests and tooling count because a weakened assertion, a newly flaky test, or a loosened lint rule is exactly what the maintainability and conventions lanes are for, and because CI config and agent instructions change how all future work gets done. It does **not** apply to prose documentation, non-executable release metadata (release note text, changelog copy edits), or a revert whose resulting diff is prose-only. Executable release tooling — a release script, a changelog generator, a publish workflow — stays in scope like any other developer tooling: it can break release generation or publication exactly like any other code-bearing change. A revert that removes or restores shipped code, tests, or tooling stays in scope too — it can reintroduce a defect exactly like any other code-bearing change. +When invoked, review code-bearing changes. "Code-bearing" means anything shipped to users, plus tests, benchmarks, developer tooling, CI configuration, and agent instructions under `.agents/` / `.claude/` (or wherever else a repo mirrors its skills for a specific editor/agent, e.g. `.cursor/`). Tests and tooling count because a weakened assertion, a newly flaky test, or a loosened lint rule is exactly what the maintainability and conventions lanes are for, and because CI config and agent instructions change how all future work gets done. It does **not** apply to prose documentation, non-executable release metadata (release note text, changelog copy edits), or a revert whose resulting diff is prose-only. Executable release tooling — a release script, a changelog generator, a publish workflow — stays in scope like any other developer tooling: it can break release generation or publication exactly like any other code-bearing change. A revert that removes or restores shipped code, tests, or tooling stays in scope too — it can reintroduce a defect exactly like any other code-bearing change. Degrade before you skip. No subagent capability is **not** a reason to skip the review: Step 2 mode 3 exists for exactly that case, so run the perspectives as sequential passes and label the report `DEGRADED MODE`. No network only stops cross-SDK verification — that lane reports `NOT VERIFIED` and every other lane still runs. -Only when even a degraded pass is impossible — context overflow, timeout, the skill's own files unreadable — say `review not performed: ` and **ask the human to explicitly authorize pushing unreviewed** before it proceeds; do not let the push continue on your own judgment. This mirrors the authorization the human must already give to override an unresolved P0 finding (Step 4) — an absent review is not a weaker case than an unresolved finding. **A missing tool is never a P0 finding**, but it is also not a licence to push unreviewed when a reduced review was available. Opening a *draft* PR to discuss a disputed finding is always allowed. +Only when even a degraded pass is impossible — context overflow, timeout, the skill's own files unreadable — say `review not performed: `. Do not invent a verdict. **A missing tool is never a P0 finding.** Opening a *draft* PR to discuss a disputed finding is always allowed. ## Related skills in this repo diff --git a/.agents/skills/dd-apm-sdk-review/review-without-harness.md b/.agents/skills/dd-apm-sdk-review/review-without-harness.md index 6c34968101..47d1b7e3da 100644 --- a/.agents/skills/dd-apm-sdk-review/review-without-harness.md +++ b/.agents/skills/dd-apm-sdk-review/review-without-harness.md @@ -13,7 +13,6 @@ review spec — the checks and the P0/P1/P2 bar only: - `.agents/skills/dd-apm-sdk-review/reviewers/_common.md` (always) - `.agents/skills/dd-apm-sdk-review/reviewers/coherence.md` - `.agents/skills/dd-apm-sdk-review/reviewers/correctness.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/performance.md` - `.agents/skills/dd-apm-sdk-review/reviewers/maintainability.md` @@ -27,17 +26,8 @@ review spec — the checks and the P0/P1/P2 bar only: Do not load `SKILL.md` or `reviewers/report-template.md`. Ignore harness-only rules in the files you do load: do not emit `READY TO PUSH` / `DO NOT PUSH` / `WAITING ON HUMAN`, and the `_common.md` rule "Never post -to GitHub" does not apply to you — post non-security findings as review -comments. Skip a lens that cannot apply to this diff rather than inventing -a finding. - -**Security is the exception.** There is no private orchestrator on this -path. `reviewers/security.md` still requires private handling: do not post -a file path, failure mode, reproduction, or secret value as a review -comment. Post only that a security finding requires private routing per -this repository's disclosure policy (typically `SECURITY.md`). Give the -details to a human out of band, or say the handoff is theirs. Never claim -the finding was routed unless you actually sent it. +to GitHub" does not apply to you — post findings as review comments. Skip +a lens that cannot apply to this diff rather than inventing a finding. If this change set is only agent-instruction files (`.agents/`, `.claude/`, `.cursor/`, `AGENTS.md`, `CLAUDE.md`), review that prose for broken paths diff --git a/.agents/skills/dd-apm-sdk-review/reviewers/README.md b/.agents/skills/dd-apm-sdk-review/reviewers/README.md index c50b3e62d9..1b84b2a607 100644 --- a/.agents/skills/dd-apm-sdk-review/reviewers/README.md +++ b/.agents/skills/dd-apm-sdk-review/reviewers/README.md @@ -1,9 +1,8 @@ -# ⚠️ This folder is a mirror — do not edit here +# Reviewer prompts -These files are copied verbatim from [`dd-apm-sdk-review-core`](https://github.com/DataDog/dd-apm-sdk-review-core). -Edits made in this tracer repo are overwritten and never propagate back. +In [`dd-apm-sdk-review-core`](https://github.com/DataDog/dd-apm-sdk-review-core) this directory is the **source** — edit the rules here. -To change a review rule, open a PR against the source repo: +When these files are copied into a tracer repo (`.agents/skills/dd-apm-sdk-review/reviewers/`), that copy is a **mirror**. Edits there are overwritten and never propagate back. To change a review rule, open a PR against the source repo: https://github.com/DataDog/dd-apm-sdk-review-core Before contributing, please read: diff --git a/.agents/skills/dd-apm-sdk-review/reviewers/correctness.md b/.agents/skills/dd-apm-sdk-review/reviewers/correctness.md index 2ae3478189..2826308643 100644 --- a/.agents/skills/dd-apm-sdk-review/reviewers/correctness.md +++ b/.agents/skills/dd-apm-sdk-review/reviewers/correctness.md @@ -38,7 +38,5 @@ already commented on the same lines for a different reason. - Do not comment on architecture, module placement, or abstraction fit — design owns that. - Do not comment on formatting, naming, or style — conventions owns that. - Do not comment on performance or allocation cost — performance owns that. -- Do not comment on security impact of a defect you find; name the defect and let the consolidator route it if it - also has a security angle. - Do not flag a defect you cannot demonstrate with a concrete input/state. "This might be wrong" without a reproducing case is not a finding. diff --git a/.agents/skills/dd-apm-sdk-review/reviewers/performance.md b/.agents/skills/dd-apm-sdk-review/reviewers/performance.md index 9a16bc2361..4899b07ff9 100644 --- a/.agents/skills/dd-apm-sdk-review/reviewers/performance.md +++ b/.agents/skills/dd-apm-sdk-review/reviewers/performance.md @@ -61,7 +61,6 @@ Each check below carries a stable slug in backticks. Cite checks by slug, never - Confidence: flag-with-confidence (unboundedness is structurally visible). - Severity: **SEV-1**. - Fix: bound by count *and* bytes, or don't cache/aggregate the high-cardinality input at all. Never flag the *absence* of a cache on open-cardinality input — not caching it is the correct choice. - - If the growth is attacker-triggerable via external input, also worth a security finding — that's the security lane's call, not yours to escalate. 4. `deferrable-critical-path-work` — **Expensive work on the critical path that could be deferred** — heavy compute / parse / normalize / serialize / I/O / lock on the synchronous request or span-finish path, that could be moved. - Confidence: flag-as-measure (deferability is contextual — verify the move would actually help). - Severity: SEV-1/2. diff --git a/.agents/skills/dd-apm-sdk-review/reviewers/report-template.md b/.agents/skills/dd-apm-sdk-review/reviewers/report-template.md index dfbf500ca9..e6604fec3f 100644 --- a/.agents/skills/dd-apm-sdk-review/reviewers/report-template.md +++ b/.agents/skills/dd-apm-sdk-review/reviewers/report-template.md @@ -14,8 +14,14 @@ A reviewer that could not do its job reports `NOT VERIFIED ()` for its a ## Report format +The **gate line is the first thing a human must see** — same line as the title, or the line immediately under it. Do not bury `DO NOT PUSH` / `WAITING ON HUMAN` / `READY TO PUSH` under empty headings. + +**Omit every section that has nothing to say.** An empty `## P0`, `## P1`, `## P2`, `## Not verified`, `## Checked and fine`, or `## Coverage gaps` heading is noise. If the only thing you would print is a placeholder, drop the section. + +Findings stay above the `---`. On `APPROVE_WITH_COMMENTS`, the P1/P2 list **and** the fix-or-dismiss question go **above** the `---`, right under the gate line — not after Rule files / Related skills. + ``` -# dd-apm-sdk-review: +# dd-apm-sdk-review: — DO NOT PUSH | WAITING ON HUMAN | READY TO PUSH Verdict: BLOCK | APPROVE_WITH_COMMENTS | APPROVE Target: ... Files: Mode: parallel | sequential | DEGRADED | pasted diff, no git @@ -24,8 +30,6 @@ Target: ... Files: Mode: parallel | sequential | DEGRADED - [design] path/to/file.ext:123 — Failure mode: Fix: -- [security] 1 finding, private routing required per this repo's disclosure process - (no location, no failure mode, no reproduction in this report: it is pasteable) ## P1 - [design] path/to/file.ext:45 — → @@ -36,12 +40,14 @@ Target: ... Files: Mode: parallel | sequential | DEGRADED ## Not verified - [cross-sdk] NOT VERIFIED (no spec source available) +On APPROVE_WITH_COMMENTS, ask here (before the ---): +Tell me whether to fix or dismiss each finding. If you fix, this skill re-runs every reviewer. + --- ## Rule files used - coherence: reviewers/coherence.md - correctness: reviewers/correctness.md -- security: reviewers/security.md<+ override path, or "(no override for this repo)"> - design: reviewers/design.md<+ override path, or "(no override for this repo)"> - performance: reviewers/performance.md<+ override path, or "(no override for this repo)"> - maintainability: reviewers/maintainability.md<+ override path, or "(no override for this repo)"> @@ -59,6 +65,6 @@ Target: ... Files: Mode: parallel | sequential | DEGRADED - ``` -The section below the `---` is bookkeeping for debugging the review itself — keep it after the findings, never before them. +The section below the `---` is bookkeeping for debugging the review itself — keep it after the findings, never before them. Still omit any of those bookkeeping sections that are empty. -Then state the gate line that matches the verdict table: `DO NOT PUSH` (`BLOCK`), `WAITING ON HUMAN` (`APPROVE_WITH_COMMENTS`), or `READY TO PUSH` (`APPROVE`). +Then state the gate line that matches the verdict table (already required in the title): `DO NOT PUSH` (`BLOCK`), `WAITING ON HUMAN` (`APPROVE_WITH_COMMENTS`), or `READY TO PUSH` (`APPROVE`). diff --git a/.agents/skills/dd-apm-sdk-review/reviewers/security.md b/.agents/skills/dd-apm-sdk-review/reviewers/security.md deleted file mode 100644 index 4877612e2e..0000000000 --- a/.agents/skills/dd-apm-sdk-review/reviewers/security.md +++ /dev/null @@ -1,38 +0,0 @@ -MUST READ FIRST: [_common.md](./_common.md) — do not review without it. - -# Reviewer: Security - -Your question: **does this change introduce a vulnerability or expose data it shouldn't?** - -This is a tracer. It runs inside every customer application, sees every request, and ships data to Datadog. A data-exposure bug here is a customer incident, not a bug report. - -This file is language-agnostic. This repo's language-specific security footguns — if any have been written yet — live in `.agents/dd-apm-sdk-review-overrides/reviewers/security.md`; read it too if it exists. - -## Tracer-specific checks (highest value — do these first) - -- **Data exposure into telemetry.** Does the change put request/response bodies, headers, query strings, cookies, auth tokens, connection strings, SQL bind values, user identifiers, or file paths into span tags, metrics, logs, or telemetry payloads? Anything reaching a span tag is customer-visible in the Datadog UI and leaves the customer's process. -- **Obfuscation and redaction.** If the change touches query/URL/SQL handling, is the existing obfuscation still applied on every path, including error and fallback paths? Adding a new code path that bypasses redaction is a P0 finding. -- **Logging.** Does new logging print user data, config values that may contain secrets (API keys, DSNs, passwords in URLs), or full exception payloads? -- **Config handling.** Is user-supplied config (env vars, config files, remote config) validated before use? Remote config is attacker-relevant: it arrives over the network, so it must never reach `eval`, a path concatenation, or a process spawn, and anything that decodes it must validate against an expected schema with bounded size. Decoding RC payloads is normal — the finding is unsafe or unvalidated deserialization, never deserialization itself. -- **Instrumentation safety.** Does instrumentation code execute application-controlled strings, deserialize untrusted input, or reflect on arbitrary names? Does it swallow exceptions from the *application* in a way that hides a security-relevant failure — or worse, propagate a tracer exception into the customer's request path? -- **Resource exhaustion.** Unbounded buffers, queues, caches, or retry loops driven by request volume. A tracer that OOMs the host application is a security problem. Flag it here specifically when it's attacker-triggerable (driven by external/request-volume input); general unbounded-growth findings with no attacker angle belong to the performance lane. -- **Third-party dependencies.** New or bumped dependencies: is the source trustworthy, is the version pinned, does it pull transitive native code? - -## Also check - -- Secrets committed in fixtures, tests, config, or CI files. -- Files that widen network exposure: new endpoints, ports, sockets, or permissive CORS/TLS settings. -- Weakened crypto or hashing, or hand-rolled crypto where a library exists. -- Path traversal in anything that resolves file paths from config or input. -- Command construction from non-constant strings. -- Permission or capability changes in CI, container, or build config. - -## Disclosure - -Your findings are the one category that must **not** be pasted into a wide-audience pull request description. Give the orchestrator enough to locate and fix the problem — file, line, failure mode — and state explicitly that the finding needs private handling per this repository's disclosure policy. Do not write a working exploit, and do not reproduce a leaked secret's value anywhere. - -## Do not - -- Do not report generic advice with no anchor in the diff. -- Do not report theoretical issues in code the change did not touch. -- Do not escalate a missing test to P0 — that belongs to the maintainability reviewer. diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml index 4d255193ce..c137c31a68 100644 --- a/.llm-validation/config.yaml +++ b/.llm-validation/config.yaml @@ -10,7 +10,6 @@ instruction_files: - .agents/skills/dd-apm-sdk-review/reviewers/_common.md - .agents/skills/dd-apm-sdk-review/reviewers/coherence.md - .agents/skills/dd-apm-sdk-review/reviewers/correctness.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/performance.md - .agents/skills/dd-apm-sdk-review/reviewers/maintainability.md diff --git a/AGENTS.md b/AGENTS.md index 17f9ea4550..491e5b319f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,15 +2,10 @@ ## Review Guidelines -**Local agent with a skill harness:** Before pushing code or creating a pull request, you must run -the [dd-apm-sdk-review](./.agents/skills/dd-apm-sdk-review/) skill on your unpushed changes. If any +**Local agent with a skill harness:** Run the [dd-apm-sdk-review](./.agents/skills/dd-apm-sdk-review/) skill on demand when asked. It is not required before every push. If any `P0` issues are reported, you must either fix them or get explicit authorization from the human you are working with and record the unresolved finding verbatim in the PR description. `P1` and `P2` -findings should be fixed before pushing, but can be dismissed by the human. - -Exception: security findings are never pasted into a PR description — a PR is a public forum, so -posting one there is an improper disclosure. Route them privately -(see [SECURITY.md](SECURITY.md)). +findings can be dismissed by the human. **Reviewer without a skill harness** (for example, GitHub Codex): read and follow [`.agents/skills/dd-apm-sdk-review/review-without-harness.md`](./.agents/skills/dd-apm-sdk-review/review-without-harness.md). From fbb7b1846393a706e0508b88da3fbcf0270baf12 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 18:11:52 +0200 Subject: [PATCH 11/13] chore(agents): sync the review skill and keep secrets out of PR text. --- .agents/skills/dd-apm-sdk-review/SKILL.md | 46 +++++++++++++------ .../review-without-harness.md | 7 ++- AGENTS.md | 2 +- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/.agents/skills/dd-apm-sdk-review/SKILL.md b/.agents/skills/dd-apm-sdk-review/SKILL.md index b757a77263..2ae417e5f1 100644 --- a/.agents/skills/dd-apm-sdk-review/SKILL.md +++ b/.agents/skills/dd-apm-sdk-review/SKILL.md @@ -49,10 +49,12 @@ protect you (the orchestrator) after you have ingested this output. # from baseRefName either: on a cross-repo PR, `origin` is the contributor's fork, # not the base repository, so that name can resolve to a stale fork branch or nothing. # Prefer a PR's baseRefOid (the base repository's actual commit). If there is -# no PR yet — a local on-demand review before a PR exists — infer the default branch -# from the DataDog remote (upstream, then origin): remote/HEAD, then -# remote/master, then remote/main. Print that inferred target so a stacked -# branch can correct it. Only ask when none of those refs exist. +# no PR yet — a local on-demand review before a PR exists — infer the default +# branch from the DataDog remote only (upstream, then origin, and only when +# that remote's URL is DataDog/*): remote/HEAD, then remote/master, then +# remote/main. Do not fall back to a contributor-fork `origin`. Print that +# inferred target so a stacked branch can correct it. Ask when there is no +# DataDog remote or none of those refs exist. # Pin --repo to a DataDog remote so a fork checkout cannot resolve the # wrong GitHub repository. Do not hardcode a tracer name. GH_REPO="" @@ -74,12 +76,19 @@ else fi TARGET=$(echo "$PR_JSON" | jq -r '.baseRefOid' 2>/dev/null) BASE_REF_NAME=$(echo "$PR_JSON" | jq -r '.baseRefName' 2>/dev/null) +INFERRED_NO_PR=0 if [ -z "$TARGET" ] || [ "$TARGET" = "null" ]; then # No PR yet (or gh could not resolve one). Infer the repo default branch so - # a local-only, not-yet-pushed branch still reviews. Never @{u}. Print the - # inferred target — a stacked branch against a parent feature can correct - # it. Only ask when none of those refs exist locally. - infer_remote="${DD_REMOTE:-origin}" + # a local-only, not-yet-pushed branch still reviews. Never @{u}. Never a + # non-DataDog origin (a fork's default is the wrong merge target). Print + # the inferred target after SECRET_GREP is defined — a stacked branch + # against a parent feature can correct it. Ask when there is no DataDog + # remote or none of those refs exist locally. + if [ -z "$DD_REMOTE" ]; then + echo "Could not infer a merge target (no PR and no DataDog remote) — what is the actual merge target for this branch (e.g. a parent feature branch on a stacked PR)?" + exit 1 + fi + infer_remote="$DD_REMOTE" TARGET="" BASE_REF_NAME="" for cand in "$infer_remote/HEAD" "$infer_remote/master" "$infer_remote/main"; do @@ -92,7 +101,7 @@ if [ -z "$TARGET" ] || [ "$TARGET" = "null" ]; then echo "Could not infer a merge target (no PR and no ${infer_remote}/HEAD|master|main) — what is the actual merge target for this branch (e.g. a parent feature branch on a stacked PR)?" exit 1 fi - echo "inferred merge target (no PR): $BASE_REF_NAME ($TARGET) — say this in the report; ask if it looks wrong (stacked PR / non-default base)" + INFERRED_NO_PR=1 fi # PR title and labels: on an existing PR, some reviewer overrides (e.g. release-note # policy, semver labels) audit these directly. Empty on a not-yet-opened PR - that's @@ -156,12 +165,16 @@ emit_diff_or_redact() { emit_diff_or_redact "PR title" printf '%s\n' "PR title: ${PR_TITLE:-}" emit_diff_or_redact "PR labels" printf '%s\n' "PR labels: ${PR_LABELS:-}" emit_diff_or_redact "recent commit subjects" git log --oneline -5 -echo "reviewing against: $BASE_REF_NAME ($TARGET)" # say this in the report; ask if it looks wrong +if [ "${INFERRED_NO_PR:-0}" -eq 1 ]; then + emit_diff_or_redact "inferred merge target" printf '%s\n' "inferred merge target (no PR): $BASE_REF_NAME ($TARGET) — say this in the report; ask if it looks wrong (stacked PR / non-default base)" +else + emit_diff_or_redact "review target" printf '%s\n' "reviewing against: $BASE_REF_NAME ($TARGET)" +fi # 2. Committed delta against the merge base with that target git rev-parse --is-shallow-repository # if true, merge-base may not resolve if BASE=$(git merge-base HEAD "$TARGET" 2>/dev/null) && [ -n "$BASE" ]; then - git diff --stat "$BASE"...HEAD + emit_diff_or_redact "committed stat $BASE...HEAD" git diff --stat "$BASE"...HEAD emit_diff_or_redact "committed $BASE...HEAD" git diff "$BASE"...HEAD else echo "WARNING: could not resolve a merge base with $TARGET (shallow clone or missing history) — committed delta not captured; report as NOT VERIFIED (no merge base) unless more history is fetched" >&2 @@ -169,7 +182,7 @@ fi # 3. Uncommitted work: the file list AND the contents. `git status` alone gives # filenames only, which would have reviewers approving edits they never saw. -git status --short +emit_diff_or_redact "git status" git status --short emit_diff_or_redact "staged" git diff --cached HEAD # Do NOT fold staged and unstaged together: if a worktree edit reverses a # staged one, `git diff HEAD` is empty while `--cached` still holds something @@ -191,6 +204,13 @@ emit_diff_or_redact "unstaged" git diff # kills the loop subshell, so a failed untracked-file diff would otherwise # truncate the scan and still exit 0. while IFS= read -r -d '' f; do + # A symlink to a directory is listed by ls-files as if it were a file. + # Grep follows it and can match secrets (or fail-closed) in the target + # tree — a false SECRET_FOUND. Skip the link; do not recurse. + if [ -L "./$f" ] && [ -d "./$f" ]; then + echo "skipping untracked symlink-to-directory: $f (not scanning the target)" + continue + fi grep -aqE -e "$SECRET_GREP" -- "./$f" 2>"$err_file" grc=$? if [ "$grc" -eq 0 ]; then @@ -303,7 +323,7 @@ Follow the report format in [reviewers/report-template.md](./reviewers/report-te Offer to fix the P0 and P1 findings. After fixes, re-run **every reviewer**, on the updated change set — not just the one that reported it. A fix can add a hot-path allocation or new coupling, so a performance or design approval given against the pre-fix diff no longer applies. Repeat until the verdict is not `BLOCK`, or until the user decides to override. -If the user overrides an unresolved P0 finding, record it verbatim in the PR description. Do not silently drop it. +If the user overrides an unresolved P0 finding, record it in the PR description. Do not silently drop it. Never paste secret values, tokens, credentials, or exploit/reproduction details into a PR description, a review comment, or the report — describe the location and class of issue only. ## Scope and escape hatches diff --git a/.agents/skills/dd-apm-sdk-review/review-without-harness.md b/.agents/skills/dd-apm-sdk-review/review-without-harness.md index 47d1b7e3da..70b10969b0 100644 --- a/.agents/skills/dd-apm-sdk-review/review-without-harness.md +++ b/.agents/skills/dd-apm-sdk-review/review-without-harness.md @@ -26,8 +26,11 @@ review spec — the checks and the P0/P1/P2 bar only: Do not load `SKILL.md` or `reviewers/report-template.md`. Ignore harness-only rules in the files you do load: do not emit `READY TO PUSH` / `DO NOT PUSH` / `WAITING ON HUMAN`, and the `_common.md` rule "Never post -to GitHub" does not apply to you — post findings as review comments. Skip -a lens that cannot apply to this diff rather than inventing a finding. +to GitHub" does not apply to you — post findings as review comments. Never +paste secret values, tokens, credentials, or exploit/reproduction details +into a review comment or PR description; describe the location and class +of issue only. Skip a lens that cannot apply to this diff rather than +inventing a finding. If this change set is only agent-instruction files (`.agents/`, `.claude/`, `.cursor/`, `AGENTS.md`, `CLAUDE.md`), review that prose for broken paths diff --git a/AGENTS.md b/AGENTS.md index 491e5b319f..5ff325e321 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ **Local agent with a skill harness:** Run the [dd-apm-sdk-review](./.agents/skills/dd-apm-sdk-review/) skill on demand when asked. It is not required before every push. If any `P0` issues are reported, you must either fix them or get explicit authorization from the human you -are working with and record the unresolved finding verbatim in the PR description. `P1` and `P2` +are working with and record the unresolved finding in the PR description (location and class of issue only — never paste secret values, tokens, credentials, or exploit details). `P1` and `P2` findings can be dismissed by the human. **Reviewer without a skill harness** (for example, GitHub Codex): read and follow From 48e47b4db5866caddd73983433f545ffef80bcb0 Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Tue, 15 Sep 2026 18:21:09 +0200 Subject: [PATCH 12/13] chore(agents): drop redundant CODEOWNERS entries. The default * rule already assigns apm-php and apm-php-core. --- .github/CODEOWNERS | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 55b1c86800..56f470af44 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,11 +1,6 @@ # Default owners * @DataDog/apm-php @DataDog/apm-php-core -# Agent review skill and eval suite (explicit so MergeGate shows an owner) -/.agents/ @DataDog/apm-php @DataDog/apm-php-core -/.llm-validation/ @DataDog/apm-php @DataDog/apm-php-core -/AGENTS.md @DataDog/apm-php @DataDog/apm-php-core - # Profiling team /profiling/ @DataDog/profiling-php @DataDog/apm-php-core From 2f53664b5f7d3e55d203928aecc00f5225a6744f Mon Sep 17 00:00:00 2001 From: Roberto Montero Date: Wed, 16 Sep 2026 10:52:01 +0200 Subject: [PATCH 13/13] chore(ci): bypass fabric for NuGet on llm-validation dotnet restore hits api.nuget.org; the PHP SA's proxy 403s it after Node and the SDK already install. Same temporary annotation as those hosts until dd-source#93032 lands. Co-authored-by: Cursor --- .gitlab-ci.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6895c08aef..a95f506c2b 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -30,10 +30,12 @@ include: # Repo-wide default is recursive (libdatadog, etc.). JS/Java skip submodules # on this job; inherit that so the gate does not clone GitHub before run.sh. GIT_SUBMODULE_STRATEGY: none - # dd-trace-php's fabric-proxy 403s NodeSource/npm/dot.net; JS/Java SAs allow + # dd-trace-php's fabric-proxy 403s NodeSource/npm/dot.net/NuGet; JS/Java SAs allow # them. Bypass the proxy for the hosts run.sh needs (CIEXE-714 annotation). + # Temporary: drop once ddoghq/dd-source#93032 (ACL) is deployed. # dot.net 301s to builds.dotnet.microsoft.com; the install script also uses aka.ms. - KUBERNETES_POD_ANNOTATIONS_NODESOURCE: "beta.fabric.datadoghq.com/no-proxy-additions=deb.nodesource.com,registry.npmjs.org,dot.net,builds.dotnet.microsoft.com,aka.ms" + # After the SDK lands, `dotnet build` restores from api.nuget.org. + KUBERNETES_POD_ANNOTATIONS_NODESOURCE: "beta.fabric.datadoghq.com/no-proxy-additions=deb.nodesource.com,registry.npmjs.org,dot.net,builds.dotnet.microsoft.com,aka.ms,api.nuget.org,nuget.org" generate-templates: stage: build