chore(agents): add dd-trace-java overrides for dd-apm-sdk-review - #12460
Conversation
Repo-specific reviewer facts, AGENTS.md hook, llm-validation suite, and replace the retired perf-review skill. Stacked on the verbatim skill copy.
The previous commit copied .gitlab-ci.yml from the old combined branch and dropped unrelated master changes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
|
🎯 Code Coverage (details) 🔗 Commit SHA: 8cbdbb0 | Docs | View more details | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Keep the stacked overrides branch on the latest verbatim skill copy.
Use the current platform SHA for the GitLab include and the job clone.
|
@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: f8f15a0d73
ℹ️ 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".
Keep the stacked overrides branch on the latest verbatim skill copy.
Evaluate crash-handler ordering as a security-lens report, not the orchestrator's redacted output. Require firstSpanWritten to reset on the payload-flush hook, not at every map().
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a96d17e1f
ℹ️ 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".
Don't flag feature-flagging modules against an aspirational -lib split; point J1 at EscapeShapeBenchmark; make the HashMap fixture lookup-only. Also merge the skill-copy secret-scan sync.
…robertomonteromiguel/dd-apm-sdk-review-java-overrides
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84bb97ad43
ℹ️ 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".
LLM ValidationLLM Validation Gate — dd-apm-sdk-review🔴 FAIL
AnalysisChanged instruction file(s): Blocking — 1 case(s) regressed; the gate failed on the worst below.
Results
Cases
Per-dimension scores, token usage, latency, and estimated cost are in the CI job logs. |
Auto-instrumentations still require InstrumenterModule; product transformers do not. The full-review fixture now states the J10/J12 preconditions the performance override already requires.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cbdbb070e
ℹ️ 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".
| - **Technical debt**: run `/techdebt` over branch changes before marking a PR ready to catch code duplication, unnecessary complexity, and dead code (refactor-only, never changes behavior) — see [.agents/skills/techdebt/SKILL.md](.agents/skills/techdebt/SKILL.md). | ||
| - **Performance**: run `/perf-review` over branch changes before marking a PR ready (advisory, not a merge gate) — see [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md). | ||
| - **Local agent with a skill harness (pilot):** before pushing, run [dd-apm-sdk-review](.agents/skills/dd-apm-sdk-review/SKILL.md) on your unpushed changes unless the user says not to. If any `P0` finding is reported, fix it or get explicit authorization and record the unresolved finding verbatim in the PR description. `P1`/`P2` findings should be fixed before pushing but can be dismissed by the human. Security findings are never pasted into the PR description — route them privately. (Performance review, previously the standalone `/perf-review` skill, is now one of this skill's perspectives — see `.agents/dd-apm-sdk-review-overrides/reviewers/performance.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`. |
There was a problem hiding this comment.
Unlike the corrected validation fixture, the newly enabled no-harness path has no private orchestrator: review-without-harness.md loads the security lens and directs the bot to post its findings as review comments, while reviewers/security.md requires those findings to be handled privately. When GitHub Codex discovers a vulnerability, it will therefore publish the location and failure mode instead of routing them privately; add an explicit security exception and private-reporting behavior to the no-harness contract.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The gate does not test the review contract without a skill harness. The security case also accepts P1 although the new rule requires P0.
🤖 Datadog Autotest · Commit 8cbdbb0 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| - .agents/skills/dd-apm-sdk-review/reviewers/correctness.md | ||
| - .agents/skills/dd-apm-sdk-review/reviewers/performance.md | ||
| - .agents/skills/dd-apm-sdk-review/reviewers/report-template.md | ||
| - .agents/skills/dd-apm-sdk-review/review-without-harness.md |
There was a problem hiding this comment.
A change can break review without a skill harness, and the gate can still pass.
Assertion details
- Input: Change only
.agents/skills/dd-apm-sdk-review/review-without-harness.md. - Expected:
A selected case must load this file and check its review rules. - Actual:
The job monitors this file, but no validation case loads it. The baseline and candidate prompts stay the same.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
| ``` | ||
| expected_criteria: | ||
| - "Flags that the JVM handler flag (`OnError`) is set to the target script/path BEFORE `CrashUploaderScriptInitializer.initialize(...)` — the ownership/trust validation — has run and returned a result. It is not enough to note that a validation step exists; the report must call out the ordering problem — if `initialize(...)` later rejects the path, the flag has already been committed to the untrusted path and the JVM crash handler will still execute it on a crash." | ||
| - "Classifies this as a security finding (P0 or P1, not merely a style/maintainability nit) — a security control that can be silently bypassed." |
There was a problem hiding this comment.
The gate can accept a review that gives the security bypass the wrong priority.
Assertion details
- Input: The reviewer reports the untrusted script-path bypass as P1.
- Expected:
The criterion must require P0, as the security rule states. - Actual:
The criterion accepts P0 or P1.
| - "Classifies this as a security finding (P0 or P1, not merely a style/maintainability nit) — a security control that can be silently bypassed." | |
| - "Classifies this as a security finding (P0, not merely a style/maintainability nit) — a security control that can be silently bypassed." |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
|
The Also no labels yet — per this repo's conventions, please add:
|
2 participants
What does this PR do?
Adds the dd-trace-java layer on top of the shared skill copy.
.agents/dd-apm-sdk-review-overrides/— Java reviewer facts (performance, conventions, design, security, maintainability, repo context)AGENTS.md— short hook: local agents run the skill; Codex readsreview-without-harness.md.llm-validation/— cases that exercise those rules, plus the GitLab"llm validation"include/perf-reviewskill (deleted here; its Java-specific rubric lives in the performance override)Stacked on the core-copy PR. Shared skill source: dd-apm-sdk-review-core#1.
Motivation
This is the half of #12364 that Java reviewers should actually read.
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.
Made with Cursor