chore(agents): add starter dd-trace-php overrides for dd-apm-sdk-review - #4191
Conversation
One PSR-2 convention rule, a short AGENTS.md hook, and one llm-validation case. Co-authored-by: Cursor <cursoragent@cursor.com>
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 16 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 2f53664 | Docs | View more details | Give us feedback! |
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 <cursoragent@cursor.com>
Document how to add a review-skill case and how to run the suite locally, same shape as dd-trace-go / dd-trace-rb.
Watch the full skill tree like go/rb, ignore local eval artifacts, and give PHP an explicit CODEOWNERS + CONTRIBUTING pointer for the gate.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb69c9011c
ℹ️ 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".
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
Benchmarks [ tracer ]Benchmark execution time: 2026-09-16 10:10:30 Comparing candidate commit 2f53664 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 1 unstable metrics.
|
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 <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
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 <cursoragent@cursor.com>
dot.net 301s to builds.dotnet.microsoft.com; that host was still proxied and returned 403 after NodeSource started working. Co-authored-by: Cursor <cursoragent@cursor.com>
Codex security covers that lane. Local skill runs on demand; GitHub Codex follows review-without-harness.md.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26fe899a5e
ℹ️ 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".
| # 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). | ||
| # 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" |
There was a problem hiding this comment.
Wrong place, these rules should be in https://github.com/ddoghq/dd-source/blob/main/domains/devex/ci/gitlab/apps/gitlab-runner-egress-acl/config/k8s/values/projects/dd-trace-php.yaml.
There was a problem hiding this comment.
Agreed — leaving the annotation here for now. Opened a draft against the project ACL: https://github.com/ddoghq/dd-source/pull/93032 (deb.nodesource.com, dot.net, builds.dotnet.microsoft.com, aka.ms; registry.npmjs.org was already allowed). Once that lands we can drop KUBERNETES_POD_ANNOTATIONS_NODESOURCE from this PR.
There was a problem hiding this comment.
Next 403 after Node/.NET: dotnet restore → api.nuget.org (NU1301, fabric-proxy 403). Added that host (and nuget.org) to the temporary annotation here, and to the ACL draft: https://github.com/ddoghq/dd-source/pull/93032. Still planning to drop KUBERNETES_POD_ANNOTATIONS_NODESOURCE once the ACL is deployed.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
The default * rule already assigns apm-php and apm-php-core.
Take the verbatim skill from the copy so the stacked PR no longer conflicts on SKILL.md / reviewers.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
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 <cursoragent@cursor.com>
LLM ValidationLLM Validation Gate — dd-apm-sdk-review✅ PASS
AnalysisChanged instruction file(s): No safety or blocking-case regressions across 1 case(s). Overall pairwise win-rate 100% [100%–100%], quality +17.1 — see the verdict above for whether that clears the noise band. Results
Per-dimension scores, token usage, latency, and estimated cost are in the CI job logs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f53664b5f
ℹ️ 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".
| 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.
When a contributor follows this “whole contribution” recipe for an override in a new reviewer file, adding it to instruction_files only makes CI watch the path; each case separately controls its evaluator inputs through files (suites/dd-apm-sdk-review.yaml:8-13). Copying the starter case without replacing or adding the new override there means the evaluator never sees the rule, so the gate can report misleading coverage. Update this step to require adding the override to the case's files list.
Useful? React with 👍 / 👎.
|
|
||
| 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. |
There was a problem hiding this comment.
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.
We dropped it now on master.
bwoebi
left a comment
bwoebi
left a comment
There was a problem hiding this comment.
I did a pass over the generic guidelines and how they apply to this repository, before approving on this.
Given that we're supposed to not comment in #4190, I'll comment here about missing overrides:
conventions.md
- **Commit and PR hygiene** as this repo requires — the exact title format, label rules, and template are in .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md if it exists, otherwise AGENTS.md.- no such thing.- **Config options.** Is a new option registered through this repo's own registration path, named per its conventions, documented, and given telemetry where the repo does that? This repo's exact registration steps are in .agents/dd-apm-sdk-review-overrides/reviewers/conventions.md if it exists — do not restate them from memory. Whether bypassing the registry rises to P0 is the design reviewer's call (it judges the architectural impact); report a bypass you find here as at least a P1 naming/registration gap.- Isn't configuration public API? Doesn't that rather point todesign.md?
correctness.md
- **Async and ordering.** A callback, promise, or event assumed to fire in an order the runtime does not guarantee; a race between two paths touching the same state.- the PHP threads are assumed to be isolated. Unless accessing global state, there must not be superfluous mutexes on only effectively thread locally accessible state. Conversely, check new global state, whether it's properly mutexed (obviously stuff running in minit or mshutdown is single-threaded and doesn't need a mutex), and if it is, check whether it risks being contended when hundreds of php threads run handling requests in the same process.
design.md
- Extension public API are the
*.stub.phpfiles, PHP public API is insrc/api. And probably configuration as defined inconfiguration.h. But note that some functions (internal functions are marked such) and configs can obviously be internal and broken at will. - **Direction of dependencies.** Does the change introduce a cycle, or make a lower layer depend on a higher one?- cross dependencies are normal, in particular not between the C and rust code. The only soft layer we have is ext/ -> product directories (have as little dependencies from ext/ on product dirs as possible). The hard layers (layers which are impossible to cross without a lot of effort) are between extension API and our PHP code in src/ and components -> zend_abstract_interface -> other extension code.- **Extension points.** If this is an integration/plugin/instrumentation, does it use the repo's standard extension mechanism rather than a bespoke hook?- I disagree on that. Use whatever fits best. We do have hooking functions, but in fact, we encourage using bespoke hooks if possible and non-invasive. (It just in most cases is not possible.)**Configuration surface.**-configuration.hfiles orotel_config.cfor otel stuff.- **Error strategy.** Does the change match the repo's convention for tracer failures (fail-soft, log-and-continue, never break the app)? A new hard throw on a customer path is a P0 finding.- A hard throw on direct public API calls is expected, in particular for bad input values. (But not for internal state depending behaviour).- Keep in mind that libdatadog is used as a submodule and may be taken into consideration.
- Hot path aggregation logic should be done in the extension / in a shared memory buffer. Aggregation across php threads and processes shall be done sidecar side. If operations can be batched, they should be and done on sidecar side
- The extension must never do network I/O itself, except through the sidecar.
- Avoid sending big buffers of data to the sidecar, that's expensive to read and write from (except as fallback).
maintainability.md
**Intent is recoverable.** Can a reader tell *why* this code exists, not just what it does? Non-obvious decisions, workarounds, and version-specific hacks need a comment naming the reason. A magic constant with no explanation is a P1.- magic constants are very common (sizing a buffer, comparing php versions, powers of two for bit operations), and "version specific" behaviour is also typical for the code base, like some stuff just being different between versions, not everything e.g. stuff in compatibility.h would need a comment.- **Function and file size.** Does a new function do one thing? Was an already long function made longer instead of split?- we don't mind about long functions, functions are mostly about code reuse and separation of concerns, not function size. The compiler can better reason within single functions. A function can be as small or big as needed to fulfil its task.- **Test quality.** Do new tests assert behavior or implementation details? Are they deterministic (no sleeps, no wall-clock dependence, no network, no ordering assumptions)? A flaky new test is a P1.- a lot of tests are dependent on local network and wall-clock to some degree. In particular tests asserting anything in framework tests or generally testing the end-to-end functionalities are going to send stuff to the request-replayer. This is within the boundaries of how testing works here. None of that is a problem. What should be asserted instead is that tests depending on the request-replayer are properly isolated. (phpt tests usingdatadog.trace.agent_test_session_tokenfor example).- **Error handling and observability.** When this fails in production, will the logs say what happened and where? Silent catch/swallow blocks that drop context are a P1; ones that swallow a real failure mode are P0.- wrong thing to check for in C code. We don't have errors propagating that way.- **Dead code and leftovers.** Commented-out code, unused parameters, debug prints,TODOwithout a ticket reference, stale docs left describing the old behavior.- we sometimes add TODO as a signal "this behaviour is this way, if this needs improvement, here's this TODO". We don't add TODO as "this should be fixed immediately / needs to be scheduled". TODO is perfectly fine.**Release notes / changelog.**- we have an AI skill compiling release notes at release time, there's no such thing.
performance.md
- **flag-with-confidence** — the cost is *mechanism-determined* and visible in the code: allocation, boxing, copying, unbounded growth, a native/FFI crossing. State it plainly.- Rust FFI crossing is cheap! Native (i.e. extension API) function calling is also generally cheaper than doing it in plain PHP code! copying is mostly fine, deep copying is not (like creating a rust string from a charslice can be expensive for big strings / if done often).| **SEV-2** | Startup latency | Eager loading, init, transformation |Startup latency is just as bad as response time for CLI processes. Same severity category.- Fix: defer to background/writer thread or task, lazy-compute, batch.- never spawn new threads in the extension code, only ever on the sidecar side.6. native-boundary-crossing- as said, just drop that completely. (why is this even in the universal guidelines, isn't that language specific how expensive it is?)
Please have a look on how this can be integrated into overrides, or the generic core instructions adjusted appropriately. I feel like some of those shouldn't be even needing to be overridden, but be dropped from the generic reviewer instructions alltogether and be added in language specific ways.
Thank you for taking the time to review this—I really appreciate your feedback. Since these comments relate to the shared core instructions, could you please move them to the [core PR](https://github.com/DataDog/dd-apm-sdk-review-core/pull/1)? That will make the discussion visible to all teams and ensure any agreed changes are applied consistently across repositories. |
2 participants
What does this PR do?
Adds the dd-trace-php layer on top of the shared skill copy.
.agents/dd-apm-sdk-review-overrides/— one starter PHP reviewer fact: userland follows PSR-2 /composer lint(fix withcomposer fix-lint), as written inCONTRIBUTING.md§ "PHP linting"AGENTS.md— short hook (this repo had none): local agents run the skill; Codex readsreview-without-harness.md.llm-validation/— first suite in this repo, one case (php-conventions-psr2), plus the GitLab"llm validation"includeStacked on the core-copy PR (#4190). Shared skill source: dd-apm-sdk-review-core#1.
Motivation
This is the half that PHP reviewers should actually read. The repo had no agent hook or eval gate yet; this lands the minimum so the skill is usable.
Additional Notes
How to review
.agents/dd-apm-sdk-review-overrides/AGENTS.mdReview Guidelines block.llm-validation/if you care about the gate.agents/skills/dd-apm-sdk-review/— that is the parent PR / core repoMerge with the parent skill-copy PR, not as a standalone.
Reviewer checklist
Made with Cursor