Repository navigation
Conversation
Both procedures apply at a known moment: before editing CI configuration or a script CI runs, and before calling a change done. AGENTS.md loaded them into every session. - ci-script-change: find every caller, enumerate the script's behavior branches from its history, prove a removal with a repo-wide grep, and check each calling job. Moved from "CI / Automation Script Changes", plus a pointer to the hermetic script test suites. - verify-profiler-change: which builds and tests each kind of change needs, by area. Moved from the first Development Guidelines bullet and extended with rows for JVM-specific, libc-sensitive, build-logic, script and doc changes, plus rules for flaky and blocked checks. Commands stay in doc/building/TestingGuide.md. AGENTS.md keeps "never claim success until all affected tests pass" and one-line pointers to both skills. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- ci-script-change and its AGENTS.md trigger cover deleting any tracked file, as the moved rule did, not only scripts. Callers are searched repo-wide, including Gradle files and hooks, and test suites are found with git ls-files instead of a partial list. - verify-profiler-change adds gtestTsan for signal-handler and atomic changes, says the containers have no Zing JDK (report blocked), covers .github and .gitlab scripts, states that the Pi cases need Node.js 22.6, and that running the check scripts directly is not qualityChecks. It drops the bug-fix test bullet that AGENTS.md already states. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e441e87204
ℹ️ 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".
| | --- | --- | | ||
| | C++ under `ddprof-lib/src/main/cpp/` | `:ddprof-lib:gtestDebug`; add `:ddprof-test:testDebug` when Java-visible behavior can change | | ||
| | Signal-handler paths, atomics, crash protection, thread lifecycle | The C++ row, plus `gtestAsan`, `gtestTsan` and `testAsan` where the sanitizers are available (Linux glibc), plus the aarch64 container (`./utils/run-containers-tests.sh --arch=aarch64 ...`) when there is no arm64 host: memory-ordering bugs show only there | | ||
| | JVM-implementation code (`hotspot/`, `j9/`, `zing/`) | `testDebug` against that JVM: `JAVA_TEST_HOME` set to it, or the container with the matching `--jdk` (for example `17-j9`). The containers have no Zing JDK: without a local Zing, report Zing as `blocked` and name the CI job that covers it | |
There was a problem hiding this comment.
When a change touches zing/ and the developer has no local Zing installation, this instruction cannot be followed: both active matrices in .github/workflows/test_workflow.yml omit all -zing entries (lines 54-56 and 439-443), and doc/architecture/JvmSupportLayer.md:253-254 explicitly says Zing CI is disabled. Requiring a named CI job therefore encourages fabricated coverage and leaves the required JVM check without a valid disposition; state that no CI coverage exists or name a real job after restoring it.
AGENTS.md reference: AGENTS.md:L366-L370
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 33374cfd5 and 34545bb5b: the JVM row says that neither the containers nor CI run Zing (citing doc/architecture/JvmSupportLayer.md), so without a local Zing the check is blocked with no CI coverage. The general blocked-check rule allows "no CI job covers it". The row also covers shared code that branches on VM::isZing() and the other VM predicates.
The skill told an agent without a local Zing to report the check as blocked and name the CI job that covers it. No such job exists: every Zing entry in CI is commented out because the Zing downloads are gone (.github/workflows/cache_java.yml). The skill now says so, and the general blocked-check rule allows "no CI job covers it". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The verify-profiler-change SKILL.md instructs contributors to name a CI job covering Zing-specific changes, but Zing is excluded from all active matrices in test_workflow.yml and its downloads are disabled in cache_java.yml, so no such job exists and the procedure cannot be followed. Both the JVM row and the blocked-check rule need to be updated to explicitly allow reporting that no CI coverage is available for Zing.
🤖 Bits Code Review · Commit e441e87
| | JVM-implementation code (`hotspot/`, `j9/`, `zing/`) | `testDebug` against that JVM: `JAVA_TEST_HOME` set to it, or the container with the matching `--jdk` (for example `17-j9`). The containers have no Zing JDK: without a local Zing, report Zing as `blocked` and name the CI job that covers it | | ||
| | libc-sensitive code (allocation hooks, `dlopen`, symbol lookup, TLS) | The matching row, plus the musl container (`--libc=musl`) | | ||
| | Java code or the Java API | `:ddprof-test:testDebug`, or the narrowest `-Ptests=` filter that covers the change, then the full suite before review | | ||
| | `build-logic/` or Gradle files | `assembleAll`, and the tasks the change affects | | ||
| | Scripts and hooks (`utils/`, `.github/scripts/`, `.gitlab/**`, `.claude/`, `.pi/`, `.githooks/`) | The test suites in the `tests/` directories next to them (`git ls-files '*/tests/*'`). The Pi adapter cases need Node.js 22.6 or newer (`--experimental-strip-types`); older versions skip them, which is not a pass. Scripts that CI, Gradle or hooks run also follow the `ci-script-change` skill. | | ||
| | Docs, skills, `AGENTS.md` | `:ddprof-lib:qualityChecks`. Running its `utils/check-*.py` scripts directly skips the signal-reachability check, which needs the debug build. | | ||
|
|
||
| - **Flaky failures:** re-run once. If the retry passes, record the first failure and the | ||
| retry. If it fails again, treat it as a real failure until shown otherwise. | ||
| - **When a check cannot run here** (no sanitizer runtime, no container runtime, no arm64 | ||
| host, no Zing), report it as `blocked` with the reason, and name the CI job that covers | ||
| it. Never report it as passed. |
There was a problem hiding this comment.
For a Zing-specific change without a local Zing JDK, the new instructions require naming a covering CI job. However, both architecture matrices in .github/workflows/test_workflow.yml exclude Zing, and its downloads are disabled in .github/workflows/cache_java.yml. No configured job supplies that verification, so the procedure cannot be followed as written. Both the JVM row and the final blocked-check rule should explicitly acknowledge unavailable CI coverage.
| | JVM-implementation code (`hotspot/`, `j9/`, `zing/`) | `testDebug` against that JVM: `JAVA_TEST_HOME` set to it, or the container with the matching `--jdk` (for example `17-j9`). The containers have no Zing JDK: without a local Zing, report Zing as `blocked` and name the CI job that covers it | | |
| | libc-sensitive code (allocation hooks, `dlopen`, symbol lookup, TLS) | The matching row, plus the musl container (`--libc=musl`) | | |
| | Java code or the Java API | `:ddprof-test:testDebug`, or the narrowest `-Ptests=` filter that covers the change, then the full suite before review | | |
| | `build-logic/` or Gradle files | `assembleAll`, and the tasks the change affects | | |
| | Scripts and hooks (`utils/`, `.github/scripts/`, `.gitlab/**`, `.claude/`, `.pi/`, `.githooks/`) | The test suites in the `tests/` directories next to them (`git ls-files '*/tests/*'`). The Pi adapter cases need Node.js 22.6 or newer (`--experimental-strip-types`); older versions skip them, which is not a pass. Scripts that CI, Gradle or hooks run also follow the `ci-script-change` skill. | | |
| | Docs, skills, `AGENTS.md` | `:ddprof-lib:qualityChecks`. Running its `utils/check-*.py` scripts directly skips the signal-reachability check, which needs the debug build. | | |
| - **Flaky failures:** re-run once. If the retry passes, record the first failure and the | |
| retry. If it fails again, treat it as a real failure until shown otherwise. | |
| - **When a check cannot run here** (no sanitizer runtime, no container runtime, no arm64 | |
| host, no Zing), report it as `blocked` with the reason, and name the CI job that covers | |
| it. Never report it as passed. | |
| | JVM-implementation code (`hotspot/`, `j9/`, `zing/`) | `testDebug` against that JVM: `JAVA_TEST_HOME` set to it, or the container with the matching `--jdk` (for example `17-j9`). The containers have no Zing JDK: without a local Zing, report Zing as `blocked`. Zing is also disabled in the CI matrices, so explicitly report that CI coverage is unavailable | | |
| | libc-sensitive code (allocation hooks, `dlopen`, symbol lookup, TLS) | The matching row, plus the musl container (`--libc=musl`) | | |
| | Java code or the Java API | `:ddprof-test:testDebug`, or the narrowest `-Ptests=` filter that covers the change, then the full suite before review | | |
| | `build-logic/` or Gradle files | `assembleAll`, and the tasks the change affects | | |
| | Scripts and hooks (`utils/`, `.github/scripts/`, `.gitlab/**`, `.claude/`, `.pi/`, `.githooks/`) | The test suites in the `tests/` directories next to them (`git ls-files '*/tests/*'`). The Pi adapter cases need Node.js 22.6 or newer (`--experimental-strip-types`); older versions skip them, which is not a pass. Scripts that CI, Gradle or hooks run also follow the `ci-script-change` skill. | | |
| | Docs, skills, `AGENTS.md` | `:ddprof-lib:qualityChecks`. Running its `utils/check-*.py` scripts directly skips the signal-reachability check, which needs the debug build. | | |
| - **Flaky failures:** re-run once. If the retry passes, record the first failure and the | |
| retry. If it fails again, treat it as a real failure until shown otherwise. | |
| - **When a check cannot run here** (no sanitizer runtime, no container runtime, no arm64 | |
| host, no Zing), report it as `blocked` with the reason. Name a covering CI job if one | |
| exists; otherwise, explicitly report that CI coverage is unavailable. Never report it | |
| as passed. |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in 33374cfd5 and 34545bb5b: the JVM row says that neither the containers nor CI run Zing (citing doc/architecture/JvmSupportLayer.md), so without a local Zing the check is blocked with no CI coverage. The general blocked-check rule allows "no CI job covers it". The row also covers shared code that branches on VM::isZing() and the other VM predicates.
…16542) Review follow-ups to the Zing fix: - The JVM row also covers shared code that branches on VM::isHotspot(), VM::isOpenJ9() or VM::isZing(), where most Zing behavior lives. - Instead of stating today's CI state, the row tells the agent to check the JDK matrix in .github/workflows/test_workflow.yml for a -zing entry before claiming coverage. The statement cannot go stale when Zing is re-enabled. - The blocked-check rule says where to look for a covering CI job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit told agents to search test_workflow.yml for a -zing entry. The file still contains -zing in commented-out matrices and in conditionals, so the search reports coverage that does not exist. The row now states the fact and cites doc/architecture/JvmSupportLayer.md, which records that Zing is disabled in the CI JDK matrix. The search for a covering job includes the .gitlab/ files that .gitlab-ci.yml includes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI Test ResultsRun: #42838093658 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-10 15:14:35 UTC |
There was a problem hiding this comment.
More details
The extracted procedures preserve caller tracing and behavior-preservation safeguards. The verification guidance explicitly marks unavailable Zing checks as blocked with no CI coverage, avoiding an unsupported claim of validation.
🤖 Bits Code Review · Commit 34545bb
|
What does this PR do?:
Moves two procedures from
AGENTS.mdinto agent skills:.agents/skills/ci-script-change/: before changing CI configuration or a script that CI, Gradle or a hook runs, and before deleting a tracked file or removing a default entry:.agents/skills/verify-profiler-change/: which builds and tests each kind of change needs before it is called done, and how to report flaky and blocked checks. Commands stay indoc/building/TestingGuide.md.AGENTS.mdkeeps "never claim success until all affected tests pass" and a one-line pointer to each skill. It shrinks from 544 to 534 lines.Stacked on #959, which adds the
check-skills.pygate that these skills pass.Motivation:
Both procedures apply at a known moment: editing CI, or claiming a change is done.
AGENTS.mdloaded them into every session. Step 2 of PROF-16466; design on PROF-16542 (decision D3 narrowsverify-profiler-changeto which tests to run).Additional Notes:
verify-profiler-changeadds rows for JVM-specific, libc-sensitive, build-logic, script and doc changes;gtestTsanfor signal-handler and atomic changes;blockedwith no CI coverage. Codex and Bits found that the first version asked for a covering CI job that does not exist. A later attempt to search the CI matrix gave false matches, so the skill states the fact and citesdoc/architecture/JvmSupportLayer.md. When Zing returns to CI, both need updating together;VM::isHotspot(),VM::isOpenJ9()orVM::isZing()./code-review(medium) on the first commit found 10 issues: narrowed triggers, an incomplete test-suite list, a missing TSan run, the Zing gap, a wrong Node.js version and one duplicated rule. The second commit fixes all 10./code-review(low) on the fix found nothing. No Sphinx run, because the PR changes only Markdown.How to test the change?:
python3 utils/check-skills.pybash utils/tests/test_quality_check_scripts.shprepare_reports.sh" loadsci-script-change; "what must pass after changing a signal-handler flag load" loadsverify-profiler-changemainvs. this branchgit log, search its callers and runbash -n; onmain, neither read the history. Both verify answers name ASan, TSan, the aarch64 container, the Gradle wrapper andblocked; onmain, Pi named neither TSan norblocked, and Claude Code named all fiveFor Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code