-
Notifications
You must be signed in to change notification settings - Fork 184
chore(agents): add starter dd-trace-php overrides for dd-apm-sdk-review #4191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: robertomonteromiguel/dd-apm-sdk-review-core-copy
Are you sure you want to change the base?
Changes from all commits
ebf675d
560d333
27e6f2d
fb69c90
d19d4b0
8066e77
3fb0c6d
f4e0fdd
cdf48c4
26fe899
fbb7b18
48e47b4
467db45
2f53664
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # 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 | ||
|
|
||
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| 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 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 the phpcs ruleset (PSR-12) | ||
|
|
||
| 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`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| # 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 [`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). | ||
|
|
||
| It answers: *did an edit to a review rule make the agent better or worse?* | ||
|
|
||
| ## Add a rule (this is the whole contribution) | ||
|
|
||
| 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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a contributor follows this “whole contribution” recipe for an override in a new reviewer file, adding it to Useful? React with 👍 / 👎. |
||
| 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 <base-sha>:<file>`. 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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # 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/coherence.md | ||
| - .agents/skills/dd-apm-sdk-review/reviewers/correctness.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 | ||
|
|
||
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| 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 | ||
| - 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, | ||
| CONTRIBUTING.md § "PHP linting", and phpcs.xml) to the following change. No git | ||
| checkout available — this snippet is the entire change to review. | ||
|
|
||
| ```php | ||
| <?php | ||
|
|
||
| namespace DDTrace; | ||
|
|
||
| class SpanBuffer | ||
| { | ||
| function flush() | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| ``` | ||
| expected_criteria: | ||
| - Flags that the snippet does not follow the phpcs ruleset (PSR-12 brace placement / indent). | ||
| - Names the repo check — `composer lint` (fix with `composer fix-lint`). | ||
| - Does not invent a different house style than `phpcs.xml` / PSR-12. | ||
| bad_signals: | ||
| - Approves the Allman-brace layout as this repo's style. | ||
| - Claims this repo has no documented PHP style standard. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # AGENTS.md - dd-trace-php | ||
|
|
||
| ## Review Guidelines | ||
|
|
||
| **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 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 | ||
| [`.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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to drop this phpcs.xml file. It's very outdated and usused and I've seen agents pick it up.
The only linting we use is
composer ci-lint.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dropped it now on master.