From 41a6ade4fcd07186607ea184460591cadcc880d5 Mon Sep 17 00:00:00 2001 From: Alexey Kuznetsov Date: Wed, 22 Jul 2026 15:25:28 -0400 Subject: [PATCH 1/5] Add fix-latest-deps-pr skill Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/fix-latest-deps-pr/SKILL.md | 288 +++++++++++++++++++++ .claude/skills/fix-latest-deps-pr | 1 + 2 files changed, 289 insertions(+) create mode 100644 .agents/skills/fix-latest-deps-pr/SKILL.md create mode 120000 .claude/skills/fix-latest-deps-pr diff --git a/.agents/skills/fix-latest-deps-pr/SKILL.md b/.agents/skills/fix-latest-deps-pr/SKILL.md new file mode 100644 index 00000000000..698ea036bcf --- /dev/null +++ b/.agents/skills/fix-latest-deps-pr/SKILL.md @@ -0,0 +1,288 @@ +--- +name: fix-latest-deps-pr +description: >- + Triage and unblock the weekly "Update Gradle dependencies" PR when its GitLab + CI is red because updated latest dependencies broke `latestDepTest` builds. + Use when asked to "fix the latest deps PR", "unblock the gradle dependencies + PR", "fix update-gradle-dependencies", or when given a GitLab pipeline id + PR + number for a red dependency-update PR. The unblock step rolls back only the + conflicting module lockfiles (one commit per module, single push) to make CI green + again. An opt-in real-fix step then attempts a per-module code fix for the new + dependency version, tested locally, as a separate PR off master. +user-invocable: true +--- + +# Fix "Update Gradle dependencies" PR + +The weekly GitHub Action `.github/workflows/update-gradle-dependencies.yaml` bumps +all latest dependencies and opens up to two PRs (core + instrumentation). These PRs +frequently go red on GitLab CI because a newly-updated *latest* dependency is +incompatible with current `dd-trace-java` code — the failures surface as +`latestDepTest` task failures. + +This skill has two phases: + +1. **Unblock (always):** roll back only the `gradle.lockfile`s of modules whose + `latestDep*Test` failed, one commit per module, then push once. This restores + CI so the (still-valuable) lockfile updates for the other modules can merge. +2. **Real fix (opt-in, per module):** actually make the code compatible with the + new dependency version, verified locally, shipped as a separate PR off `master`. + +Only `latestDep*Test` failures are in scope. Ignore all other red jobs (flaky, +infra, unrelated test failures) — do not touch them. + +--- + +## Prerequisites + +Verify these before starting. If a required item is missing, stop and tell the user +what to set up rather than working around it. + +- **`ddci-mcp-prod` MCP server — required.** Must be installed and authorized in this + session. Phases 1 and 3 depend on its tools (`getCIStatus`, `getJobErrorSummary`, + `getJobLogs`). Confirm it is reachable early (e.g. a `getCIStatus` call succeeds); if + the tools are absent or unauthorized, stop and ask the user to install/authorize it. +- **GitHub CLI (`gh`) — required, authenticated.** Used to resolve, check out, and open + PRs (`gh pr view/checkout/create`). Run `gh auth status` if unsure. +- **Git remote `origin` with `master` — required.** The rollback baseline and the + Phase 3 branch base come from `origin/master`. +- **Push / PR permissions — required for the push steps.** You must be able to push to + the dependency PR branch (Phase 2) and create PRs off `master` (Phase 3). +- **GitLab API access — optional, Phase 3 only.** A token (e.g. via `ddtool`) to fetch + untruncated job logs from the `/trace` endpoint and download `reports.tar` artifacts + when the ddci summary/logs are insufficient. If unavailable, fall back to ddci + `getJobLogs` pagination. + +--- + +## Phase 0 — Preflight + +1. **Collect inputs.** Ask the user for: + - the **GitLab pipeline id** (used for cross-checking and for direct GitLab API + log/artifact fetching in Phase 3), and + - the **PR number** (source of truth for the branch and head commit). + +2. **Resolve the PR.** + ```bash + gh pr view --json number,headRefName,headRefOid,url,baseRefName,title + ``` + Capture `headRefName` (branch), `headRefOid` (head SHA). + +3. **Record the broken head SHA.** Remember the `headRefOid` value as session context — + refer to it below as `ORIG_PR_HEAD`. **Do not** rely on a shell variable to carry it: + each command may run in a separate shell, so substitute the literal 40-char SHA + directly into every command that needs it. Phase 3 needs this SHA to retrieve the + *broken* lockfiles after Phase 2 has rolled them back. + +4. **Ensure the branch is checked out.** + ```bash + git rev-parse --abbrev-ref HEAD + ``` + If it is **not** the PR branch, ask the user whether to check it out. Only if they + say yes: + ```bash + gh pr checkout + ``` + If they say no, stop — the skill needs the branch checked out to proceed. + +5. **Require a clean worktree.** This skill rewrites `gradle.lockfile`s in place, so any + uncommitted local work could be silently discarded. Check first: + ```bash + git status --porcelain + ``` + If the output is non-empty, **stop** and ask the user to commit, stash, or discard + their changes before continuing. Do not proceed with a dirty worktree. + +6. **Sync master reference** (needed for rollback + Phase 2 base): + ```bash + git fetch origin master + ``` + +--- + +## Phase 1 — Triage failed latestDep modules + +1. **Get CI status** using the PR head SHA (per the confirmed mapping: PR → head SHA + → ddci): + - `getCIStatus(commit_sha=, include_metadata=true)` + - This returns the DDCI `request_id` and the `tasks` map. Sanity-check the + returned pipeline/request against the user-provided pipeline id and note any + mismatch out loud before continuing. + +2. **Enumerate failed tasks** from the `tasks` map. For each failed task, capture its + full `task_id` (the map key, e.g. `gitlab--`) and its + `latest_task_execution.native_id` (the GitLab job id = `task_execution_id`). + +3. **Extract failing Gradle tasks** from each failed job: + - Start with `getJobErrorSummary(request_id, task_id, task_execution_id)`. + - If unclear, fall back to `getJobLogs(request_id, task_execution_id)`. + - Grep the output for lines of the form: + ``` + Execution failed for task ':dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest'. + ``` + - **Collect only** Gradle task paths whose final segment is a `latestDep` test task + — i.e. matches `:latestDepTest`, `:latestDepForkedTest`, or any + `:latestDep*Test` variant. **Ignore** everything else (`:test`, `:forkedTest`, + muzzle, infra, etc.). + - A single job can contain multiple failing latestDep tasks — collect them **all**, + across all failed jobs. Dedupe. + +4. **Map each Gradle task path → module dir → lockfile.** Strip the trailing + `:`, then convert `:` to `/`: + - `:dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest` + → module `dd-java-agent/instrumentation/openai-java/openai-java-3.0` + → lockfile `dd-java-agent/instrumentation/openai-java/openai-java-3.0/gradle.lockfile` + - Verify the lockfile exists. If the path mapping fails (rare mismatch between + Gradle project path and directory), locate it by the leaf module name: + `find . -path '*/gradle.lockfile'`. + +5. **Report the triage** to the user: the list of failed latestDep modules, each with + its Gradle path and lockfile, before making any change. + +--- + +## Phase 2 — Unblock (rollback lockfiles) + +For **each** failed module, create **one commit** rolling its lockfile back to the +pre-update state. Do **not** push between commits. + +1. Determine the pre-update baseline (the lockfile as it was before the update commit): + ```bash + BASE=$(git merge-base HEAD origin/master) + git checkout "$BASE" -- /gradle.lockfile + ``` + +2. Commit that single module's lockfile: + ```bash + git add /gradle.lockfile + git commit -m "temporary fix: rolled back conflicting dependencies to unblock PR merging + + Module: " + ``` + +3. Repeat for every failed module — one commit each. + +4. **Push once, after all commits exist.** Confirm with the user, then: + ```bash + git push + ``` + Pushing once (not per commit) triggers a single CI run. Report the pushed commits + and remind the user CI will re-run on the PR. + +--- + +## Phase 3 — Real fix (opt-in, per module) + +After unblocking, **ask** the user whether to create separate real-fix PRs (one per +failed module). If no, stop and hand them the module list. If yes, work the modules +**one at a time** — fully finish and verify a module before starting the next. + +For each module: + +1. **Fresh branch off master:** + ```bash + git fetch origin master + git checkout -b fix/latest-dep- origin/master + ``` + +2. **Reproduce the failure** by restoring the *broken* lockfile from the recorded PR + head (the version with the new, breaking dependency). Substitute the literal + `ORIG_PR_HEAD` SHA you noted in Phase 0 — do not use a shell variable: + ```bash + git checkout -- /gradle.lockfile + ``` + +3. **Research the breaking change** — gather what you need: + - Full CI logs via ddci `getJobLogs` (paginate with `offset`), or the GitLab API + `/trace` endpoint for the untruncated log; download `reports.tar` for + thread/heap dumps if the failure is a hang/crash. + - Read the failing module's source and tests. + - Diff the conflicting dependency's old vs new version: GitHub release notes, + changelog, tags/diffs, and decompile the new jar if needed to see the API/behavior + change. Use WebFetch/WebSearch for release notes and upstream docs. + +4. **Implement the fix** in the module's production/test source so it is compatible + with the new dependency version. Match surrounding code style; follow repo test + conventions. + +5. **Verify locally — the fix must make latestDep green WITHOUT breaking the base + test set.** `latestDepTest` is a separate source/test set from `test`, so run both, + plus every other test task the module defines: + ```bash + ./gradlew ::latestDepTest + ./gradlew ::test + ``` + Also run any other test tasks the module has (e.g. `forkedTest`, `latestDepForkedTest`). + List the module's tasks with `./gradlew ::tasks --group=verification` + and run all relevant ones. Every one must pass. Do not proceed while anything is red. + +6. **Only when everything is green**, commit, push the branch, and open a **draft** PR + off `master`. + + **Write a real, filled-in PR description — never a stub.** Author it yourself from + what you actually found and did in this run (the failing `latestDep` upgrade, the + API/behavior change you researched, the code you changed, the tests you ran). Keep it + **short** and use markdown to highlight coding stuff (backtick `identifiers` / + `ClassName#method`, code fences for snippets, links to release notes). The `<…>` + angle-bracket hints below are instructions for what to write — replace each one with + concrete content; do not leave placeholders, `...`, or the hints themselves in the + final body. Follow this template exactly: + + ```markdown + # What Does This Do + + <1–3 sentences: the code change and the dependency version it targets> + + # Motivation + + + + # Additional Notes + + + ``` + + Compose the finished body first, then pass it to `gh`. **Stage only the intended + files** — the restored `gradle.lockfile` plus the source/test files you changed — + never `git add -A`. Verify the staged diff before committing so nothing unrelated + sneaks in: + ```bash + git add /gradle.lockfile + git status # confirm nothing unintended is staged or left unstaged + git diff --cached # review exactly what will be committed + git commit -m " version>" + git push -u origin fix/latest-dep- + gh pr create --draft --base master \ + --title "" \ + --label "tag: ai generated" \ + --label "tag: dependencies" \ + --label "" \ + --label "" \ + --body "$(cat <<'EOF' + + EOF + )" + ``` + Per repo PR conventions: open as draft first; always include `tag: ai generated`, + at least one `comp:`/`inst:` label, and one `type:` label. + +7. Return to a clean state for the next module: + ```bash + git checkout + ``` + +Repeat for each remaining module. When done, summarize: unblock commits pushed, and +for each module either the fix PR URL or that it was skipped/deferred. + +--- + +## Guardrails + +- Touch only modules whose `latestDep*Test` failed. Never modify lockfiles or code for + unrelated red jobs. +- Phase 2 (unblock) changes **only** `gradle.lockfile`s — never source code. +- Never push per-commit in Phase 2; batch into a single push. +- Never open a Phase 3 PR until `latestDepTest`, `test`, and all other module test + tasks pass locally. +- Keep Gradle runs sequential. diff --git a/.claude/skills/fix-latest-deps-pr b/.claude/skills/fix-latest-deps-pr new file mode 120000 index 00000000000..798ea54f823 --- /dev/null +++ b/.claude/skills/fix-latest-deps-pr @@ -0,0 +1 @@ +../../.agents/skills/fix-latest-deps-pr \ No newline at end of file From ea7b846107b10bdc6f4087509b01cbc3d7ed19a8 Mon Sep 17 00:00:00 2001 From: Alexey Kuznetsov Date: Wed, 22 Jul 2026 15:40:17 -0400 Subject: [PATCH 2/5] Address review feedback on fix-latest-deps-pr skill Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/fix-latest-deps-pr/SKILL.md | 108 +++++++++++++++++---- 1 file changed, 87 insertions(+), 21 deletions(-) diff --git a/.agents/skills/fix-latest-deps-pr/SKILL.md b/.agents/skills/fix-latest-deps-pr/SKILL.md index 698ea036bcf..7303683300c 100644 --- a/.agents/skills/fix-latest-deps-pr/SKILL.md +++ b/.agents/skills/fix-latest-deps-pr/SKILL.md @@ -52,6 +52,14 @@ what to set up rather than working around it. untruncated job logs from the `/trace` endpoint and download `reports.tar` artifacts when the ddci summary/logs are insufficient. If unavailable, fall back to ddci `getJobLogs` pagination. +- **Module-specific credentials — conditional.** Some optional instrumentations are + excluded from the Gradle build unless a property is set, so their project path is + unresolvable in Phase 3 without it. Notably `akka-http-10.6` + (`:dd-java-agent:instrumentation:akka:akka-http:akka-http-10.6`) is omitted from + `settings.gradle.kts` when `akkaRepositoryToken` is blank — export + `ORG_GRADLE_PROJECT_akkaRepositoryToken` (as the weekly workflow does) before + reproducing or verifying a fix for that module. If a failed module is missing/omitted + from the build, check for a required token before assuming the mapping is wrong. --- @@ -74,7 +82,7 @@ what to set up rather than working around it. directly into every command that needs it. Phase 3 needs this SHA to retrieve the *broken* lockfiles after Phase 2 has rolled them back. -4. **Ensure the branch is checked out.** +4. **Ensure the branch is checked out at the PR head.** ```bash git rev-parse --abbrev-ref HEAD ``` @@ -85,6 +93,20 @@ what to set up rather than working around it. ``` If they say no, stop — the skill needs the branch checked out to proceed. + Branch name alone is not enough: a local branch with the PR's name can be stale or + behind `headRefOid`. After the branch is checked out, verify `HEAD` matches the + captured PR head SHA: + ```bash + git rev-parse HEAD # must equal ORIG_PR_HEAD + ``` + If it does not match, sync to the exact PR head before continuing (with the user's + agreement, since this moves their branch): + ```bash + git fetch origin && git reset --hard + ``` + Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 builds + rollback commits on the wrong tree and the push is rejected or reverts stale content. + 5. **Require a clean worktree.** This skill rewrites `gradle.lockfile`s in place, so any uncommitted local work could be silently discarded. Check first: ```bash @@ -120,21 +142,42 @@ what to set up rather than working around it. ``` Execution failed for task ':dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest'. ``` - - **Collect only** Gradle task paths whose final segment is a `latestDep` test task - — i.e. matches `:latestDepTest`, `:latestDepForkedTest`, or any - `:latestDep*Test` variant. **Ignore** everything else (`:test`, `:forkedTest`, - muzzle, infra, etc.). - - A single job can contain multiple failing latestDep tasks — collect them **all**, - across all failed jobs. Dedupe. + - **Collect only** Gradle task paths belonging to a module's **latest-dep** work. + A break can surface as either a test-execution failure or a *compile/resolution* + failure of the latest-dep source set (which fails before the test task runs), so + match any final segment that is: + - a latest-dep **test** task — `:latestDepTest`, `:latestDepForkedTest`, or any + `:latestDep*Test` variant; **or** + - a latest-dep **build** task — `:compileLatestDepTestJava`, + `:compileLatestDepTestGroovy`, or any `:*LatestDepTest*`/`:compileLatestDep*` + source-set task. + + **Ignore** everything else (`:test`, `:forkedTest`, `:compileTestJava`, muzzle, + infra, etc.). + - **Record the exact failing task name(s)** for each module — do not assume it is + `latestDepTest`. Phase 3 reuses the real task name(s) to verify the fix (some + modules define only `latestDepForkedTest`). + - A single job can contain multiple failing latest-dep tasks — collect them **all**, + across all failed jobs. Dedupe by module. 4. **Map each Gradle task path → module dir → lockfile.** Strip the trailing `:`, then convert `:` to `/`: - `:dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest` → module `dd-java-agent/instrumentation/openai-java/openai-java-3.0` → lockfile `dd-java-agent/instrumentation/openai-java/openai-java-3.0/gradle.lockfile` - - Verify the lockfile exists. If the path mapping fails (rare mismatch between - Gradle project path and directory), locate it by the leaf module name: - `find . -path '*/gradle.lockfile'`. + - Verify the derived lockfile path exists; if it does, use it directly — do not search. + - Only if the derived path does **not** exist (rare Gradle-path/directory mismatch), + resolve the directory authoritatively from the Gradle project path rather than + guessing by name: + ```bash + ./gradlew -q ::properties | grep '^projectDir:' + ``` + then use `/gradle.lockfile`. + - Do **not** fall back to a bare leaf-name search: leaf names are not unique (e.g. + `grpc-1.5` exists under both `dd-java-agent/instrumentation/` and `dd-smoke-tests/`), + so a search can roll back the wrong module's lockfile. If you must search, it has to + resolve to **exactly one** lockfile; on zero or multiple matches, stop and ask the + user for the fully-qualified module path. 5. **Report the triage** to the user: the list of failed latestDep modules, each with its Gradle path and lockfile, before making any change. @@ -146,15 +189,29 @@ what to set up rather than working around it. For **each** failed module, create **one commit** rolling its lockfile back to the pre-update state. Do **not** push between commits. -1. Determine the pre-update baseline (the lockfile as it was before the update commit): +1. Determine the pre-update baseline (the lockfile as it was before the update commit) + and restore that module's lockfile to it: ```bash BASE=$(git merge-base HEAD origin/master) - git checkout "$BASE" -- /gradle.lockfile + ``` + - **If the lockfile existed at `BASE`** (the common case — an update), restore it: + ```bash + git checkout "$BASE" -- /gradle.lockfile + ``` + - **If the lockfile did NOT exist at `BASE`** (the update workflow *created* a + brand-new lockfile for this module), there is nothing to check out — rolling back + means **deleting** it: + ```bash + git rm /gradle.lockfile + ``` + Detect which case applies with: + ```bash + git cat-file -e "$BASE:/gradle.lockfile" 2>/dev/null && echo existed || echo new ``` -2. Commit that single module's lockfile: +2. Commit that single module's change (a reverted or a removed lockfile): ```bash - git add /gradle.lockfile + git add /gradle.lockfile # 'git rm' already stages a deletion git commit -m "temporary fix: rolled back conflicting dependencies to unblock PR merging Module: " @@ -205,16 +262,25 @@ For each module: with the new dependency version. Match surrounding code style; follow repo test conventions. -5. **Verify locally — the fix must make latestDep green WITHOUT breaking the base - test set.** `latestDepTest` is a separate source/test set from `test`, so run both, - plus every other test task the module defines: +5. **Verify locally — the fix must make the latest-dep suite green WITHOUT breaking the + base test set.** The latest-dep suite is a separate source/test set from `test`. + First discover what the module actually defines — do not assume `latestDepTest` + exists (some modules define only `latestDepForkedTest`): + ```bash + ./gradlew ::tasks --group=verification + ``` + Then run, and require all to pass: + - the **exact failing task(s) you recorded in triage** for this module (e.g. + `::latestDepForkedTest`) — this is the suite that was red; + - the base `::test` (and its forked variant if defined), to prove the + fix does not regress the base set; + - any other verification tasks the module defines. + ```bash - ./gradlew ::latestDepTest + ./gradlew :: ./gradlew ::test ``` - Also run any other test tasks the module has (e.g. `forkedTest`, `latestDepForkedTest`). - List the module's tasks with `./gradlew ::tasks --group=verification` - and run all relevant ones. Every one must pass. Do not proceed while anything is red. + Do not proceed while anything is red. 6. **Only when everything is green**, commit, push the branch, and open a **draft** PR off `master`. From f7688c761b4b0c10979c5ed11491c30d9b9883d6 Mon Sep 17 00:00:00 2001 From: Alexey Kuznetsov Date: Thu, 23 Jul 2026 20:20:17 -0400 Subject: [PATCH 3/5] Applied review notes. Refactored to `glab` cli. Appled codex review. --- .agents/skills/fix-latest-deps-pr/SKILL.md | 439 +++++++++------------ 1 file changed, 187 insertions(+), 252 deletions(-) diff --git a/.agents/skills/fix-latest-deps-pr/SKILL.md b/.agents/skills/fix-latest-deps-pr/SKILL.md index 7303683300c..a57c35aef8b 100644 --- a/.agents/skills/fix-latest-deps-pr/SKILL.md +++ b/.agents/skills/fix-latest-deps-pr/SKILL.md @@ -1,324 +1,262 @@ --- name: fix-latest-deps-pr description: >- - Triage and unblock the weekly "Update Gradle dependencies" PR when its GitLab - CI is red because updated latest dependencies broke `latestDepTest` builds. - Use when asked to "fix the latest deps PR", "unblock the gradle dependencies - PR", "fix update-gradle-dependencies", or when given a GitLab pipeline id + PR - number for a red dependency-update PR. The unblock step rolls back only the - conflicting module lockfiles (one commit per module, single push) to make CI green - again. An opt-in real-fix step then attempts a per-module code fix for the new - dependency version, tested locally, as a separate PR off master. + Triage and unblock the weekly "Update Gradle dependencies" PR when GitLab CI is red from `latestDepTest` breakages. + Use when asked to "fix the latest deps PR", "unblock the gradle dependencies PR", "fix update-gradle-dependencies", or + when given a GitLab pipeline id + PR number. Rolls back the conflicting module lockfiles to make CI green, then + optionally ships per-module real fixes as separate PRs. user-invocable: true --- # Fix "Update Gradle dependencies" PR -The weekly GitHub Action `.github/workflows/update-gradle-dependencies.yaml` bumps -all latest dependencies and opens up to two PRs (core + instrumentation). These PRs -frequently go red on GitLab CI because a newly-updated *latest* dependency is -incompatible with current `dd-trace-java` code — the failures surface as -`latestDepTest` task failures. +The weekly GitHub Action `.github/workflows/update-gradle-dependencies.yaml` bumps all latest dependencies and opens up +to two PRs (core + instrumentation). These PRs can go red on GitLab CI because a newly-updated *latest* dependency is +incompatible with current `dd-trace-java` code — the failures surface as `latestDepTest` task failures. This skill has two phases: -1. **Unblock (always):** roll back only the `gradle.lockfile`s of modules whose - `latestDep*Test` failed, one commit per module, then push once. This restores - CI so the (still-valuable) lockfile updates for the other modules can merge. -2. **Real fix (opt-in, per module):** actually make the code compatible with the - new dependency version, verified locally, shipped as a separate PR off `master`. +1. **Unblock (always):** roll back only the `gradle.lockfile`s of modules whose `latestDep*Test` failed, one commit per + module, then push once. This restores CI so the (still-valuable) lockfile updates for the other modules can merge. +2. **Real fix (opt-in, per module):** actually make the code compatible with the new dependency version, verified + locally, shipped as a separate PR off `master`. -Only `latestDep*Test` failures are in scope. Ignore all other red jobs (flaky, -infra, unrelated test failures) — do not touch them. +Only failures of a module's **latest-dep source set** are in scope — its `latestDep*Test` suites *and* their +compile/resolution tasks (e.g. `compileLatestDep*`). Ignore all other red jobs (flaky, infra, unrelated test failures) +— do not touch them. --- ## Prerequisites -Verify these before starting. If a required item is missing, stop and tell the user -what to set up rather than working around it. - -- **`ddci-mcp-prod` MCP server — required.** Must be installed and authorized in this - session. Phases 1 and 3 depend on its tools (`getCIStatus`, `getJobErrorSummary`, - `getJobLogs`). Confirm it is reachable early (e.g. a `getCIStatus` call succeeds); if - the tools are absent or unauthorized, stop and ask the user to install/authorize it. -- **GitHub CLI (`gh`) — required, authenticated.** Used to resolve, check out, and open - PRs (`gh pr view/checkout/create`). Run `gh auth status` if unsure. -- **Git remote `origin` with `master` — required.** The rollback baseline and the - Phase 3 branch base come from `origin/master`. -- **Push / PR permissions — required for the push steps.** You must be able to push to - the dependency PR branch (Phase 2) and create PRs off `master` (Phase 3). -- **GitLab API access — optional, Phase 3 only.** A token (e.g. via `ddtool`) to fetch - untruncated job logs from the `/trace` endpoint and download `reports.tar` artifacts - when the ddci summary/logs are insufficient. If unavailable, fall back to ddci - `getJobLogs` pagination. -- **Module-specific credentials — conditional.** Some optional instrumentations are - excluded from the Gradle build unless a property is set, so their project path is - unresolvable in Phase 3 without it. Notably `akka-http-10.6` - (`:dd-java-agent:instrumentation:akka:akka-http:akka-http-10.6`) is omitted from - `settings.gradle.kts` when `akkaRepositoryToken` is blank — export - `ORG_GRADLE_PROJECT_akkaRepositoryToken` (as the weekly workflow does) before - reproducing or verifying a fix for that module. If a failed module is missing/omitted - from the build, check for a required token before assuming the mapping is wrong. +Require `glab`, `gh`, and `jq`. If any is missing or unauthenticated, report exactly what's missing and stop — don't +install it yourself or work around it; print the install command so the user can run it manually. + +- **`glab` (GitLab CLI) — required, authenticated.** `glab auth status` must be green for `gitlab.ddbuild.io`. If it's + missing, tell the user to install it manually (`brew install glab`, or the platform package manager). +- **GitHub CLI (`gh`) — required, authenticated.** Resolves, checks out, and opens PRs. Run `gh auth status` to verify. +- **`jq` — required.** Every GitLab API call is parsed with it; confirm it is on `PATH`. +- **Git remote `origin` with `master` — required.** The rollback baseline and the Phase 3 branch base come from the + latest `origin/master` — always `git fetch origin master` first so they aren't computed against a stale local ref. +- **Module-specific credentials — conditional.** Some optional instrumentations are excluded from the Gradle build + unless a property is set, so their project path is unresolvable — e.g. `akka-http-10.6` needs + `ORG_GRADLE_PROJECT_akkaRepositoryToken`. Skip such a module with a note to the user rather than assuming the mapping + is wrong. + +Every `glab api` call below uses two fixed values: `--hostname gitlab.ddbuild.io` (the remote is GitHub, so `glab` +can't infer the host) and the project path `DataDog%2Fapm-reliability%2Fdd-trace-java` (use exactly this — the shorter +`DataDog/dd-trace-java` only 301-redirects and breaks `glab`). --- ## Phase 0 — Preflight -1. **Collect inputs.** Ask the user for: - - the **GitLab pipeline id** (used for cross-checking and for direct GitLab API - log/artifact fetching in Phase 3), and - - the **PR number** (source of truth for the branch and head commit). +1. **Collect inputs.** Ask the user for the **GitLab pipeline id** and the **PR number**. 2. **Resolve the PR.** ```bash gh pr view --json number,headRefName,headRefOid,url,baseRefName,title ``` - Capture `headRefName` (branch), `headRefOid` (head SHA). + Capture `headRefName` (branch) and `headRefOid` (head SHA). Sanity-check that `baseRefName` is `master` and the + title/branch looks like the weekly dependency-update PR; if not, STOP and tell the user this isn't the PR the skill + handles (the whole skill assumes an `origin/master` baseline and lockfile-only diffs). -3. **Record the broken head SHA.** Remember the `headRefOid` value as session context — - refer to it below as `ORIG_PR_HEAD`. **Do not** rely on a shell variable to carry it: - each command may run in a separate shell, so substitute the literal 40-char SHA - directly into every command that needs it. Phase 3 needs this SHA to retrieve the - *broken* lockfiles after Phase 2 has rolled them back. +3. **Record the broken head SHA** as session context — call it `ORIG_PR_HEAD`. Substitute the literal 40-char SHA into + every command below rather than a shell variable (each command may run in a separate shell). Phase 3 needs it to + retrieve the *broken* lockfiles after Phase 2 rolls them back. -4. **Ensure the branch is checked out at the PR head.** - ```bash - git rev-parse --abbrev-ref HEAD - ``` - If it is **not** the PR branch, ask the user whether to check it out. Only if they - say yes: - ```bash - gh pr checkout - ``` - If they say no, stop — the skill needs the branch checked out to proceed. +4. **Check out the PR branch at its head.** - Branch name alone is not enough: a local branch with the PR's name can be stale or - behind `headRefOid`. After the branch is checked out, verify `HEAD` matches the - captured PR head SHA: +- If the worktree is **dirty**, STOP and ask the user to commit/stash/discard — this skill rewrites lockfiles in place + and a checkout could clobber uncommitted work. +- If clean and not already on the PR branch, run `gh pr checkout ` and tell the user you switched them. +- Verify `HEAD`, and if it doesn't match, sync to the PR head (with the user's OK, since `reset --hard` moves the branch + and drops any local commits above it): + ```bash + git rev-parse HEAD # must equal ORIG_PR_HEAD + git fetch origin && git reset --hard # only if HEAD != ORIG_PR_HEAD + ``` + +Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 builds commits on the wrong tree. + +5. **Sync the master reference** (rollback + Phase 2/3 base): ```bash - git rev-parse HEAD # must equal ORIG_PR_HEAD + git fetch origin master ``` - If it does not match, sync to the exact PR head before continuing (with the user's - agreement, since this moves their branch): + +--- + +## Phase 1 — Triage failed latestDep modules + +1. **Enumerate the failed jobs** for the given pipeline. First confirm the pipeline is actually the PR's — its SHA must + equal `ORIG_PR_HEAD`, or a wrong/stale id would roll back unrelated lockfiles; STOP if it doesn't match. Then list + the failed jobs (quote the path — zsh globs the `?`; `--paginate` to span all pages): ```bash - git fetch origin && git reset --hard + PROJ=DataDog%2Fapm-reliability%2Fdd-trace-java + PIPE= + glab api --hostname gitlab.ddbuild.io "projects/$PROJ/pipelines/$PIPE" | jq -er .sha # must equal ORIG_PR_HEAD + glab api --hostname gitlab.ddbuild.io --paginate \ + "projects/$PROJ/pipelines/$PIPE/jobs?scope=failed&per_page=100" \ + | jq -r '.[] | "\(.id)\t\(.name)\t\(.status)"' ``` - Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 builds - rollback commits on the wrong tree and the push is rejected or reverts stale content. -5. **Require a clean worktree.** This skill rewrites `gradle.lockfile`s in place, so any - uncommitted local work could be silently discarded. Check first: +2. **Extract failing Gradle tasks** from each failed job's full `/trace` (the `Execution failed for task '…'` line is + mid-log, not in the tail). Fetch into a scratch dir; abort if any trace can't be fetched, or a broken module goes + unidentified: ```bash - git status --porcelain + PROJ=DataDog%2Fapm-reliability%2Fdd-trace-java + LOGDIR=$(mktemp -d); trap 'rm -rf "$LOGDIR"' EXIT # don't leave internal CI logs lying around + JOBS=( ...) + for J in "${JOBS[@]}"; do + glab api --hostname gitlab.ddbuild.io "projects/$PROJ/jobs/$J/trace" > "$LOGDIR/$J.log" \ + || { echo "ERROR: trace fetch failed for job $J — abort" >&2; exit 1; } + done + grep -hoE "Execution failed for task '[^']*'" "$LOGDIR"/*.log | sort -u ``` - If the output is non-empty, **stop** and ask the user to commit, stash, or discard - their changes before continuing. Do not proceed with a dirty worktree. -6. **Sync master reference** (needed for rollback + Phase 2 base): +- **Collect only** latest-dep tasks — a break surfaces as either a test-execution failure or a *compile/resolution* + failure of the latest-dep source set (which fails before the test runs). Match any failing task whose name contains + `latestDep` — e.g. `latestDepTest`, `latestDepForkedTest`, `compileLatestDepJava`. **Ignore** everything else + (`:test`, `:forkedTest`, muzzle, infra, …). +- **Record the exact failing task name (s)** per module — don't assume `latestDepTest`; some modules define only + `latestDepForkedTest`, and Phase 3 reuses the real name to verify. +- **Record the JVM** the job ran on (the job name encodes it, e.g. a `j17`/`jdk17` segment). Phase 3 reproduces with + `-PtestJvm=`. If a module failed on more than one JVM, record each — verify them all. +- A single job can contain multiple failing latest-dep tasks — collect them **all**, across all jobs, deduped by module. + +3. **Map each Gradle task path → lockfile.** Strip the trailing `:`, convert the remaining `:` to `/`, and + append `/gradle.lockfile` — e.g. `::latestDepTest` → `/gradle.lockfile`. + Verify the derived path exists; if it doesn't (rare Gradle-path/dir mismatch), resolve it with + `./gradlew -q ::properties | grep '^projectDir:'` rather than guessing by leaf name. + Then confirm the PR actually changed this lockfile; if it didn't, the failure is flaky/unrelated (rolling back would + be a no-op) — exclude the module and flag it to the user: ```bash - git fetch origin master + BASE=$(git merge-base HEAD origin/master) # HEAD == ORIG_PR_HEAD (verified in Phase 0) + git diff --quiet "$BASE" HEAD -- /gradle.lockfile \ + && echo "UNCHANGED — exclude (flaky/unrelated)" || echo "changed — in scope" ``` ---- - -## Phase 1 — Triage failed latestDep modules - -1. **Get CI status** using the PR head SHA (per the confirmed mapping: PR → head SHA - → ddci): - - `getCIStatus(commit_sha=, include_metadata=true)` - - This returns the DDCI `request_id` and the `tasks` map. Sanity-check the - returned pipeline/request against the user-provided pipeline id and note any - mismatch out loud before continuing. - -2. **Enumerate failed tasks** from the `tasks` map. For each failed task, capture its - full `task_id` (the map key, e.g. `gitlab--`) and its - `latest_task_execution.native_id` (the GitLab job id = `task_execution_id`). - -3. **Extract failing Gradle tasks** from each failed job: - - Start with `getJobErrorSummary(request_id, task_id, task_execution_id)`. - - If unclear, fall back to `getJobLogs(request_id, task_execution_id)`. - - Grep the output for lines of the form: - ``` - Execution failed for task ':dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest'. - ``` - - **Collect only** Gradle task paths belonging to a module's **latest-dep** work. - A break can surface as either a test-execution failure or a *compile/resolution* - failure of the latest-dep source set (which fails before the test task runs), so - match any final segment that is: - - a latest-dep **test** task — `:latestDepTest`, `:latestDepForkedTest`, or any - `:latestDep*Test` variant; **or** - - a latest-dep **build** task — `:compileLatestDepTestJava`, - `:compileLatestDepTestGroovy`, or any `:*LatestDepTest*`/`:compileLatestDep*` - source-set task. - - **Ignore** everything else (`:test`, `:forkedTest`, `:compileTestJava`, muzzle, - infra, etc.). - - **Record the exact failing task name(s)** for each module — do not assume it is - `latestDepTest`. Phase 3 reuses the real task name(s) to verify the fix (some - modules define only `latestDepForkedTest`). - - A single job can contain multiple failing latest-dep tasks — collect them **all**, - across all failed jobs. Dedupe by module. - -4. **Map each Gradle task path → module dir → lockfile.** Strip the trailing - `:`, then convert `:` to `/`: - - `:dd-java-agent:instrumentation:openai-java:openai-java-3.0:latestDepTest` - → module `dd-java-agent/instrumentation/openai-java/openai-java-3.0` - → lockfile `dd-java-agent/instrumentation/openai-java/openai-java-3.0/gradle.lockfile` - - Verify the derived lockfile path exists; if it does, use it directly — do not search. - - Only if the derived path does **not** exist (rare Gradle-path/directory mismatch), - resolve the directory authoritatively from the Gradle project path rather than - guessing by name: - ```bash - ./gradlew -q ::properties | grep '^projectDir:' - ``` - then use `/gradle.lockfile`. - - Do **not** fall back to a bare leaf-name search: leaf names are not unique (e.g. - `grpc-1.5` exists under both `dd-java-agent/instrumentation/` and `dd-smoke-tests/`), - so a search can roll back the wrong module's lockfile. If you must search, it has to - resolve to **exactly one** lockfile; on zero or multiple matches, stop and ask the - user for the fully-qualified module path. - -5. **Report the triage** to the user: the list of failed latestDep modules, each with - its Gradle path and lockfile, before making any change. +4. **Report the triage** to the user before making any change: each failed latestDep module with its Gradle path, + lockfile, failing task (s), and JVM (s). --- ## Phase 2 — Unblock (rollback lockfiles) -For **each** failed module, create **one commit** rolling its lockfile back to the -pre-update state. Do **not** push between commits. +For **each** failed module, create **one commit** rolling its lockfile back to the pre-update state. Do **not** push +between commits. -1. Determine the pre-update baseline (the lockfile as it was before the update commit) - and restore that module's lockfile to it: +**Approval model:** the rollback commits are local and fully reversible, so create them without prompting. The one +action that needs the user's go-ahead is the **push** (step 4) — where changes leave the machine and re-trigger CI. + +1. Roll the module's lockfile back to the pre-update baseline. Run as **one block** — `BASE` must live in the same + shell: ```bash BASE=$(git merge-base HEAD origin/master) - ``` - - **If the lockfile existed at `BASE`** (the common case — an update), restore it: - ```bash - git checkout "$BASE" -- /gradle.lockfile - ``` - - **If the lockfile did NOT exist at `BASE`** (the update workflow *created* a - brand-new lockfile for this module), there is nothing to check out — rolling back - means **deleting** it: - ```bash - git rm /gradle.lockfile - ``` - Detect which case applies with: - ```bash - git cat-file -e "$BASE:/gradle.lockfile" 2>/dev/null && echo existed || echo new + M=/gradle.lockfile + if git cat-file -e "$BASE:$M" 2>/dev/null; then + git checkout "$BASE" -- "$M" # existed at BASE (common case — an update) → restore & stage + else + git rm "$M" # created by the update → rolling back means deleting it + fi ``` -2. Commit that single module's change (a reverted or a removed lockfile): +2. Commit that single module's already-staged change (restore or deletion — no `git add` needed): ```bash - git add /gradle.lockfile # 'git rm' already stages a deletion - git commit -m "temporary fix: rolled back conflicting dependencies to unblock PR merging - - Module: " + git commit -m "temporary fix: rolled back conflicting latest dependencies for module: to unblock PR merging" ``` 3. Repeat for every failed module — one commit each. -4. **Push once, after all commits exist.** Confirm with the user, then: +4. **Verify, then push once** (after all commits exist). One diff per module confirms the lockfile now matches the + baseline — this covers both cases (a restored file has no diff vs `BASE`; a correctly-deleted new file is absent in + both `BASE` and `HEAD`, so also no diff): + ```bash + BASE=$(git merge-base HEAD origin/master) + git diff --quiet "$BASE" HEAD -- /gradle.lockfile \ + && echo "matches baseline" || echo "DIFF — investigate before pushing" + ``` + Fix anything flagged. Then, with the user's go-ahead, push **explicitly** to the PR branch (`headRefName` from Phase + 0) so a missing/incorrect upstream can't send commits elsewhere: ```bash - git push + git push origin HEAD: ``` - Pushing once (not per commit) triggers a single CI run. Report the pushed commits - and remind the user CI will re-run on the PR. + +One push (not per commit) triggers a single CI run. Report the pushed commits and remind the user CI will re-run. --- ## Phase 3 — Real fix (opt-in, per module) -After unblocking, **ask** the user whether to create separate real-fix PRs (one per -failed module). If no, stop and hand them the module list. If yes, work the modules -**one at a time** — fully finish and verify a module before starting the next. +After unblocking, **ask** the user whether to create separate real-fix PRs (one per failed module). If no, stop and hand +them the module list. If yes, work modules **one at a time** — fully finish and verify one before starting the next. For each module: -1. **Fresh branch off master:** +1. **Fresh branch off master.** Name it from the **sanitized fully-qualified Gradle path** (strip the leading `:`, + replace every `:` with `-`), *not* the leaf — two modules can share a leaf (e.g. `grpc-1.5`) and collide: ```bash git fetch origin master - git checkout -b fix/latest-dep- origin/master + git checkout -b fix/latest-dep- origin/master ``` -2. **Reproduce the failure** by restoring the *broken* lockfile from the recorded PR - head (the version with the new, breaking dependency). Substitute the literal - `ORIG_PR_HEAD` SHA you noted in Phase 0 — do not use a shell variable: +2. **Reproduce the failure** by putting the module's lockfile into the *broken* PR-head state. Substitute the literal + `ORIG_PR_HEAD` SHA. The update regenerates every lockfile and can *delete* one, so mirror whichever state it has: ```bash - git checkout -- /gradle.lockfile + if git cat-file -e ":/gradle.lockfile" 2>/dev/null; then + git checkout -- /gradle.lockfile # present at PR head → restore + else + git rm /gradle.lockfile # absent at PR head → reproduce the deletion + fi ``` -3. **Research the breaking change** — gather what you need: - - Full CI logs via ddci `getJobLogs` (paginate with `offset`), or the GitLab API - `/trace` endpoint for the untruncated log; download `reports.tar` for - thread/heap dumps if the failure is a hang/crash. - - Read the failing module's source and tests. - - Diff the conflicting dependency's old vs new version: GitHub release notes, - changelog, tags/diffs, and decompile the new jar if needed to see the API/behavior - change. Use WebFetch/WebSearch for release notes and upstream docs. - -4. **Implement the fix** in the module's production/test source so it is compatible - with the new dependency version. Match surrounding code style; follow repo test - conventions. - -5. **Verify locally — the fix must make the latest-dep suite green WITHOUT breaking the - base test set.** The latest-dep suite is a separate source/test set from `test`. - First discover what the module actually defines — do not assume `latestDepTest` - exists (some modules define only `latestDepForkedTest`): +3. **Research the breaking change:** + +- Full CI logs via the `/trace` fetch in Phase 1 step 2; download `reports.tar` for thread/heap dumps if it's a + hang/crash. +- Read the failing module's source and tests. +- Diff the conflicting dependency old vs new: release notes, changelog, tags/diffs, decompile the new jar if needed. Use + web-fetch / web-search for release notes and upstream docs. + +4. **Implement the fix** so the module is compatible with the new version. Usually production/test source, but often + also module-local `build.gradle` changes (constraints, forced versions, source-set config) or test + fixtures/resources — keep those too. If the fix genuinely requires changes **outside** the failing module (a shared + helper, a related version module), that's allowed with clear justification, but you must extend verification (step 5) + to every module you touched. Match surrounding code style; follow repo test conventions. + +5. **Verify locally — the latest-dep suite must go green WITHOUT breaking the base test set** (a separate source set). + Discover what the module defines (don't assume `latestDepTest`): ```bash ./gradlew ::tasks --group=verification ``` - Then run, and require all to pass: - - the **exact failing task(s) you recorded in triage** for this module (e.g. - `::latestDepForkedTest`) — this is the suite that was red; - - the base `::test` (and its forked variant if defined), to prove the - fix does not regress the base set; - - any other verification tasks the module defines. - + Then, on the JVM (s) you recorded in triage (run each one that failed), require all to pass: ```bash - ./gradlew :: - ./gradlew ::test + ./gradlew :: -PtestJvm= + ./gradlew ::test -PtestJvm= ``` - Do not proceed while anything is red. - -6. **Only when everything is green**, commit, push the branch, and open a **draft** PR - off `master`. - - **Write a real, filled-in PR description — never a stub.** Author it yourself from - what you actually found and did in this run (the failing `latestDep` upgrade, the - API/behavior change you researched, the code you changed, the tests you ran). Keep it - **short** and use markdown to highlight coding stuff (backtick `identifiers` / - `ClassName#method`, code fences for snippets, links to release notes). The `<…>` - angle-bracket hints below are instructions for what to write — replace each one with - concrete content; do not leave placeholders, `...`, or the hints themselves in the - final body. Follow this template exactly: - - ```markdown - # What Does This Do - - <1–3 sentences: the code change and the dependency version it targets> - - # Motivation - - - - # Additional Notes - - - ``` - - Compose the finished body first, then pass it to `gh`. **Stage only the intended - files** — the restored `gradle.lockfile` plus the source/test files you changed — - never `git add -A`. Verify the staged diff before committing so nothing unrelated - sneaks in: + Also run any other verification task the module defines, and the `test` (and latest-dep) tasks of any **other** + module you changed. Skipping `-PtestJvm` runs your default JVM and can falsely pass a JVM-specific break. Do not + proceed while anything is red. + +6. **Only when everything is green**, commit, push the branch, and open a **draft** PR off `master`. + + **Write a real, filled-in PR description — never a stub.** Base it on `.github/pull_request_template.md` (read that + file, fill each section from what you found and did this run): **What Does This Do** (the change + dependency version + it targets), **Motivation** (which `latestDep` upgrade broke and the API/behavior change that caused it), + **Additional Notes** (tests run, links to release notes/upstream diff; omit if empty), and the **Contributor + Checklist**. + + **Stage only the intended files** — never `git add -A`. The lockfile's step-2 state (restore *or* deletion) is + already staged, so stage just the files you changed here and re-add the lockfile **only if it still exists** + (re-adding a deleted path errors). **Confirm with the user before committing** — show the staged diff and the PR + title/body, and get an explicit go-ahead (this pushes a branch and opens an external PR). Then: ```bash - git add /gradle.lockfile - git status # confirm nothing unintended is staged or left unstaged - git diff --cached # review exactly what will be committed + git add + [ -e /gradle.lockfile ] && git add -- /gradle.lockfile # deletion already staged from step 2 + git status && git diff --cached # review exactly what will be committed — show this to the user + # → get the user's go-ahead here, then: git commit -m " version>" - git push -u origin fix/latest-dep- + git push -u origin fix/latest-dep- gh pr create --draft --base master \ --title "" \ --label "tag: ai generated" \ @@ -326,29 +264,26 @@ For each module: --label "" \ --label "" \ --body "$(cat <<'EOF' - + EOF )" ``` - Per repo PR conventions: open as draft first; always include `tag: ai generated`, - at least one `comp:`/`inst:` label, and one `type:` label. + Per repo PR conventions: draft first; always include `tag: ai generated`, at least one `comp:`/`inst:` label, and one + `type:` label. 7. Return to a clean state for the next module: ```bash git checkout ``` -Repeat for each remaining module. When done, summarize: unblock commits pushed, and -for each module either the fix PR URL or that it was skipped/deferred. +When done, summarize: unblock commits pushed, and for each module either the fix PR URL or that it was skipped. --- ## Guardrails -- Touch only modules whose `latestDep*Test` failed. Never modify lockfiles or code for - unrelated red jobs. -- Phase 2 (unblock) changes **only** `gradle.lockfile`s — never source code. -- Never push per-commit in Phase 2; batch into a single push. -- Never open a Phase 3 PR until `latestDepTest`, `test`, and all other module test - tasks pass locally. +- **Scope:** only modules whose latest-dep source set failed. Phase 2 changes **only** those modules' `gradle.lockfile`s + — never source code, never another module. Phase 3 may extend beyond the failed module only with clear justification, + and verification must then cover every module you touched. +- **Approvals:** get the user's go-ahead before the Phase 2 push and before committing/opening any Phase 3 PR. - Keep Gradle runs sequential. From f9687692c59a5ae25433662848cae2cff8c78db3 Mon Sep 17 00:00:00 2001 From: Alexey Kuznetsov Date: Mon, 27 Jul 2026 12:47:34 -0400 Subject: [PATCH 4/5] Improved skill after real usage. --- .agents/skills/fix-latest-deps-pr/SKILL.md | 72 +++++++++++++++++----- 1 file changed, 56 insertions(+), 16 deletions(-) diff --git a/.agents/skills/fix-latest-deps-pr/SKILL.md b/.agents/skills/fix-latest-deps-pr/SKILL.md index a57c35aef8b..5a41383fcbe 100644 --- a/.agents/skills/fix-latest-deps-pr/SKILL.md +++ b/.agents/skills/fix-latest-deps-pr/SKILL.md @@ -29,11 +29,18 @@ compile/resolution tasks (e.g. `compileLatestDep*`). Ignore all other red jobs ( ## Prerequisites -Require `glab`, `gh`, and `jq`. If any is missing or unauthenticated, report exactly what's missing and stop — don't -install it yourself or work around it; print the install command so the user can run it manually. - -- **`glab` (GitLab CLI) — required, authenticated.** `glab auth status` must be green for `gitlab.ddbuild.io`. If it's - missing, tell the user to install it manually (`brew install glab`, or the platform package manager). +Require `glab`, `gh`, `jq`, and `ddtool`. If a **binary is missing**, report exactly which one and stop — don't install +it yourself; print the installation command so the user can run it manually. Authentication is different: an +expired/missing +`glab` token is expected and **recoverable** — refresh it with the recipe below rather than stopping. + +- **`glab` (GitLab CLI) — required.** If the binary is missing, tell the user to install it manually (`brew install + glab`, or the platform package manager). **Do not trust `glab auth status`** for `gitlab.ddbuild.io`: this repo + authenticates with a GitLab *project access token*, which cannot call the `/user` endpoint that `status` probes, so + `status` reports red 401 even when API access is fine. Instead, probe with a real project-scoped call (see readiness + probe below) and, if it 401s, run the auth-recovery recipe. +- **`ddtool` — required (for GitLab auth).** Datadog-internal CLI that mints the GitLab project access token. If the + binary is missing, tell the user to install it manually and stop. - **GitHub CLI (`gh`) — required, authenticated.** Resolves, checks out, and opens PRs. Run `gh auth status` to verify. - **`jq` — required.** Every GitLab API call is parsed with it; confirm it is on `PATH`. - **Git remote `origin` with `master` — required.** The rollback baseline and the Phase 3 branch base come from the @@ -47,13 +54,44 @@ Every `glab api` call below uses two fixed values: `--hostname gitlab.ddbuild.io can't infer the host) and the project path `DataDog%2Fapm-reliability%2Fdd-trace-java` (use exactly this — the shorter `DataDog/dd-trace-java` only 301-redirects and breaks `glab`). +### GitLab auth readiness probe & recovery + +Before Phase 1, confirm `glab` can actually reach the API with a project-scoped call (works with a project access token; +`glab auth status` does not): + +```bash +glab api --hostname gitlab.ddbuild.io "projects/DataDog%2Fapm-reliability%2Fdd-trace-java" | jq -er .path_with_namespace +``` + +If it prints `DataDog/apm-reliability/dd-trace-java`, auth is good — proceed. If it 401s (or `glab` reports no token), +refresh the token and retry the probe: + +```bash +TOKEN=$(ddtool auth gitlab project-token DataDog dd-trace-java | tail -1) +glab auth login --hostname gitlab.ddbuild.io --token "$TOKEN" +``` + +- If the `ddtool` command itself errors with an auth/login failure (not a GitLab 401), the developer's `ddtool` session + has expired — have them run `ddtool auth login` first, then re-run the recipe. +- Use **`project-token`** — *not* `ddtool auth gitlab token` (the oauth token 401s against this REST API; dead end). +- The `project-token` args are the **GitHub short name** `DataDog dd-trace-java`, even though every `glab api` path uses + the longer `DataDog%2Fapm-reliability%2Fdd-trace-java`. +- These tokens are short-lived. If **any** `glab api` call later in the run starts returning 401, just re-run this + recovery recipe and continue — don't abort. +- `glab auth login` may print a `gitlab.com` telemetry 401 warning; it's harmless — the probe above is the source of + truth. + --- ## Phase 0 — Preflight 1. **Collect inputs.** Ask the user for the **GitLab pipeline id** and the **PR number**. -2. **Resolve the PR.** +2. **Confirm GitLab auth** by running the readiness probe from + [GitLab auth readiness probe & recovery](#gitlab-auth-readiness-probe--recovery); if it 401s, run the recovery recipe + and re-probe before continuing. Do this before any `glab api` call. + +3. **Resolve the PR.** ```bash gh pr view --json number,headRefName,headRefOid,url,baseRefName,title ``` @@ -61,11 +99,11 @@ can't infer the host) and the project path `DataDog%2Fapm-reliability%2Fdd-trace title/branch looks like the weekly dependency-update PR; if not, STOP and tell the user this isn't the PR the skill handles (the whole skill assumes an `origin/master` baseline and lockfile-only diffs). -3. **Record the broken head SHA** as session context — call it `ORIG_PR_HEAD`. Substitute the literal 40-char SHA into +4. **Record the broken head SHA** as session context — call it `ORIG_PR_HEAD`. Substitute the literal 40-char SHA into every command below rather than a shell variable (each command may run in a separate shell). Phase 3 needs it to retrieve the *broken* lockfiles after Phase 2 rolls them back. -4. **Check out the PR branch at its head.** +5. **Check out the PR branch at its head.** - If the worktree is **dirty**, STOP and ask the user to commit/stash/discard — this skill rewrites lockfiles in place and a checkout could clobber uncommitted work. @@ -79,7 +117,7 @@ can't infer the host) and the project path `DataDog%2Fapm-reliability%2Fdd-trace Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 builds commits on the wrong tree. -5. **Sync the master reference** (rollback + Phase 2/3 base): +6. **Sync the master reference** (rollback + Phase 2/3 base): ```bash git fetch origin master ``` @@ -127,9 +165,9 @@ Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 bu 3. **Map each Gradle task path → lockfile.** Strip the trailing `:`, convert the remaining `:` to `/`, and append `/gradle.lockfile` — e.g. `::latestDepTest` → `/gradle.lockfile`. Verify the derived path exists; if it doesn't (rare Gradle-path/dir mismatch), resolve it with - `./gradlew -q ::properties | grep '^projectDir:'` rather than guessing by leaf name. - Then confirm the PR actually changed this lockfile; if it didn't, the failure is flaky/unrelated (rolling back would - be a no-op) — exclude the module and flag it to the user: + `./gradlew -q ::properties | grep '^projectDir:'` rather than guessing by leaf name. Then confirm the + PR actually changed this lockfile; if it didn't, the failure is flaky/unrelated (rolling back would be a no-op) — + exclude the module and flag it to the user: ```bash BASE=$(git merge-base HEAD origin/master) # HEAD == ORIG_PR_HEAD (verified in Phase 0) git diff --quiet "$BASE" HEAD -- /gradle.lockfile \ @@ -177,7 +215,9 @@ action that needs the user's go-ahead is the **push** (step 4) — where changes && echo "matches baseline" || echo "DIFF — investigate before pushing" ``` Fix anything flagged. Then, with the user's go-ahead, push **explicitly** to the PR branch (`headRefName` from Phase - 0) so a missing/incorrect upstream can't send commits elsewhere: + +0) so a missing/incorrect upstream can't send commits elsewhere: + ```bash git push origin HEAD: ``` @@ -282,8 +322,8 @@ When done, summarize: unblock commits pushed, and for each module either the fix ## Guardrails -- **Scope:** only modules whose latest-dep source set failed. Phase 2 changes **only** those modules' `gradle.lockfile`s - — never source code, never another module. Phase 3 may extend beyond the failed module only with clear justification, - and verification must then cover every module you touched. +- **Scope:** only modules whose latest-dep source set failed. Phase 2 changes **only** those modules' `gradle.lockfile` + s — never source code, never another module. Phase 3 may extend beyond the failed module only with clear + justification, and verification must then cover every module you touched. - **Approvals:** get the user's go-ahead before the Phase 2 push and before committing/opening any Phase 3 PR. - Keep Gradle runs sequential. From b90bd017b08654583ac73c7102cfecc45e46f73a Mon Sep 17 00:00:00 2001 From: Alexey Kuznetsov Date: Mon, 3 Aug 2026 14:58:50 -0400 Subject: [PATCH 5/5] Roll back Codex-flagged pre-release dependency bumps Co-Authored-By: Claude Opus 5 (1M context) --- .agents/skills/fix-latest-deps-pr/SKILL.md | 165 ++++++++++++++++----- 1 file changed, 130 insertions(+), 35 deletions(-) diff --git a/.agents/skills/fix-latest-deps-pr/SKILL.md b/.agents/skills/fix-latest-deps-pr/SKILL.md index 5a41383fcbe..84ae21c730e 100644 --- a/.agents/skills/fix-latest-deps-pr/SKILL.md +++ b/.agents/skills/fix-latest-deps-pr/SKILL.md @@ -3,8 +3,9 @@ name: fix-latest-deps-pr description: >- Triage and unblock the weekly "Update Gradle dependencies" PR when GitLab CI is red from `latestDepTest` breakages. Use when asked to "fix the latest deps PR", "unblock the gradle dependencies PR", "fix update-gradle-dependencies", or - when given a GitLab pipeline id + PR number. Rolls back the conflicting module lockfiles to make CI green, then - optionally ships per-module real fixes as separate PRs. + when given a GitLab pipeline id + PR number. Rolls back the conflicting module lockfiles to make CI green — including + modules a Codex review flagged as bumped to a pre-release (alpha/beta/RC) version — then optionally ships per-module + real fixes as separate PRs. user-invocable: true --- @@ -14,16 +15,26 @@ The weekly GitHub Action `.github/workflows/update-gradle-dependencies.yaml` bum to two PRs (core + instrumentation). These PRs can go red on GitLab CI because a newly-updated *latest* dependency is incompatible with current `dd-trace-java` code — the failures surface as `latestDepTest` task failures. -This skill has two phases: +The work has two goals, delivered by the phases below: -1. **Unblock (always):** roll back only the `gradle.lockfile`s of modules whose `latestDep*Test` failed, one commit per - module, then push once. This restores CI so the (still-valuable) lockfile updates for the other modules can merge. -2. **Real fix (opt-in, per module):** actually make the code compatible with the new dependency version, verified - locally, shipped as a separate PR off `master`. +- **Unblock (always, Phase 3):** roll back only the `gradle.lockfile`s of the in-scope modules, one commit per module, + then push once. This restores CI so the (still-valuable) lockfile updates for the other modules can merge. +- **Real fix (opt-in, per module, Phase 4):** actually make the code compatible with the new dependency version, verified + locally, shipped as a separate PR off `master`. Only **CI-failed** modules are eligible — pre-release rollbacks never + get a real-fix PR (see below). -Only failures of a module's **latest-dep source set** are in scope — its `latestDep*Test` suites *and* their -compile/resolution tasks (e.g. `compileLatestDep*`). Ignore all other red jobs (flaky, infra, unrelated test failures) -— do not touch them. +A module is in scope for rollback in exactly two cases: + +- **(A) CI-failed latest-dep source set** — its `latestDep*Test` suites *and* their compile/resolution tasks + (e.g. `compileLatestDep*`). Ignore all other red jobs (flaky, infra, unrelated test failures) — do not touch them. +- **(B) Codex flagged a pre-release bump** — an automated Codex review on the PR raises a risk about a module being moved + to an alpha/beta/RC/milestone/snapshot version, *even though CI is green*. Prefer safety: roll it back and wait for the + GA release. + +Case B is rolled back **exactly the same way** as case A, but it never gets a Phase 4 real-fix PR. Rationale: pre-release +artifacts aren't what users run, so a green latest-dep suite on a beta buys nothing and a subtly broken one costs real +triage time; and the API can still change before GA, so any fix written against a beta is likely throwaway work. Roll +back, wait for the final release — the next weekly run picks it up. --- @@ -41,9 +52,10 @@ expired/missing probe below) and, if it 401s, run the auth-recovery recipe. - **`ddtool` — required (for GitLab auth).** Datadog-internal CLI that mints the GitLab project access token. If the binary is missing, tell the user to install it manually and stop. -- **GitHub CLI (`gh`) — required, authenticated.** Resolves, checks out, and opens PRs. Run `gh auth status` to verify. +- **GitHub CLI (`gh`) — required, authenticated.** Resolves, checks out, and opens PRs, and reads the Codex review + comments in Phase 2. Run `gh auth status` to verify. - **`jq` — required.** Every GitLab API call is parsed with it; confirm it is on `PATH`. -- **Git remote `origin` with `master` — required.** The rollback baseline and the Phase 3 branch base come from the +- **Git remote `origin` with `master` — required.** The rollback baseline and the Phase 4 branch base come from the latest `origin/master` — always `git fetch origin master` first so they aren't computed against a stale local ref. - **Module-specific credentials — conditional.** Some optional instrumentations are excluded from the Gradle build unless a property is set, so their project path is unresolvable — e.g. `akka-http-10.6` needs @@ -85,7 +97,9 @@ glab auth login --hostname gitlab.ddbuild.io --token "$TOKEN" ## Phase 0 — Preflight -1. **Collect inputs.** Ask the user for the **GitLab pipeline id** and the **PR number**. +1. **Collect inputs.** Ask the user for the **GitLab pipeline id** and the **PR number**. The PR number is always + required. The pipeline id is only needed for Phase 1 — if CI is green and the user only wants the Codex pre-release + check, take the PR number, skip Phase 1, and go straight to Phase 2. 2. **Confirm GitLab auth** by running the readiness probe from [GitLab auth readiness probe & recovery](#gitlab-auth-readiness-probe--recovery); if it 401s, run the recovery recipe @@ -100,8 +114,8 @@ glab auth login --hostname gitlab.ddbuild.io --token "$TOKEN" handles (the whole skill assumes an `origin/master` baseline and lockfile-only diffs). 4. **Record the broken head SHA** as session context — call it `ORIG_PR_HEAD`. Substitute the literal 40-char SHA into - every command below rather than a shell variable (each command may run in a separate shell). Phase 3 needs it to - retrieve the *broken* lockfiles after Phase 2 rolls them back. + every command below rather than a shell variable (each command may run in a separate shell). Phase 4 needs it to + retrieve the *broken* lockfiles after Phase 3 rolls them back. 5. **Check out the PR branch at its head.** @@ -115,9 +129,9 @@ glab auth login --hostname gitlab.ddbuild.io --token "$TOKEN" git fetch origin && git reset --hard # only if HEAD != ORIG_PR_HEAD ``` -Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 builds commits on the wrong tree. +Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 3 builds commits on the wrong tree. -6. **Sync the master reference** (rollback + Phase 2/3 base): +6. **Sync the master reference** (rollback + Phase 3/4 base): ```bash git fetch origin master ``` @@ -157,8 +171,8 @@ Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 bu `latestDep` — e.g. `latestDepTest`, `latestDepForkedTest`, `compileLatestDepJava`. **Ignore** everything else (`:test`, `:forkedTest`, muzzle, infra, …). - **Record the exact failing task name (s)** per module — don't assume `latestDepTest`; some modules define only - `latestDepForkedTest`, and Phase 3 reuses the real name to verify. -- **Record the JVM** the job ran on (the job name encodes it, e.g. a `j17`/`jdk17` segment). Phase 3 reproduces with + `latestDepForkedTest`, and Phase 4 reuses the real name to verify. +- **Record the JVM** the job ran on (the job name encodes it, e.g. a `j17`/`jdk17` segment). Phase 4 reproduces with `-PtestJvm=`. If a module failed on more than one JVM, record each — verify them all. - A single job can contain multiple failing latest-dep tasks — collect them **all**, across all jobs, deduped by module. @@ -174,15 +188,77 @@ Do not triage or commit until `HEAD` equals the PR head — otherwise Phase 2 bu && echo "UNCHANGED — exclude (flaky/unrelated)" || echo "changed — in scope" ``` -4. **Report the triage** to the user before making any change: each failed latestDep module with its Gradle path, - lockfile, failing task (s), and JVM (s). +4. **Record the triage** for each failed latestDep module: Gradle path, lockfile, failing task (s), and JVM (s). Tag every + one of them **category A** (CI-failed → Phase 4 eligible). Don't report yet — Phase 2 adds category B, and the user + gets one combined table. + +--- + +## Phase 2 — Triage Codex-flagged pre-release bumps + +Codex reviews these PRs automatically and its findings land as **inline review comments on the lockfiles**, not as the +review body (the body is just a banner). A recurring finding is a module bumped to a pre-release version — e.g. PR 12131, +where `wildfly-9.0` moved `wildfly-embedded`/WildFly Core to `34.0.0.Beta3` while the same lockfile still unpacked the GA +`wildfly-dist:41.0.0.Final`, mixing two different core releases. CI was green; the combination is one no user runs. + +Run this **even when Phase 1 found nothing** — case B is independent of CI status. + +1. **Fetch the Codex inline comments** (the review body carries no findings, so read the comments endpoint): + ```bash + gh api repos/DataDog/dd-trace-java/pulls//comments --paginate \ + | jq -r '.[] | select(.user.login | test("codex"; "i")) | "=== \(.path):\(.line // .original_line)\n\(.body)\n"' + ``` + Also skim the top-level ones for anything Codex left outside a diff hunk: + ```bash + gh pr view --json comments,reviews \ + | jq -r '(.comments + .reviews)[] | select(.author.login | test("codex"; "i")) | .body' + ``` + +- Empty output means Codex found nothing (it reacts 👍 instead of commenting) — skip to Phase 3 with category A only. +- Codex prefixes findings with a `P1`/`P2`/`P3` badge. Severity does **not** gate the rollback: any pre-release finding is + rolled back regardless. +- Codex comments can be wrong or stale. Verify each one against the lockfile in the diff (step 2) before acting; never + roll back on the comment's word alone. + +2. **Keep only pre-release findings.** A finding qualifies for case B when the version Codex is complaining about is a + pre-release. Read the actual version out of the lockfile at the comment's path and check it, rather than trusting the + prose: + ```bash + BASE=$(git merge-base HEAD origin/master) + git diff "$BASE" HEAD -- /gradle.lockfile | grep '^+' \ + | grep -inE '[.-](alpha|beta|rc|cr|m[0-9]|milestone|snapshot|preview|dev|pre|ea)[0-9._-]*(=|$)' + ``` + Pre-release markers are separated by `.` or `-` (WildFly/JBoss use `.Beta3`, `.CR1`; most others use `-beta.1`, + `-RC1`, `-M2`, `-SNAPSHOT`). `.Final`, `.GA`, `.RELEASE`, `.SP1` and plain `1.2.3` are **GA — not in scope**. + +- If the added version is GA, the finding is *not* case B. Do **not** roll it back — surface it to the user verbatim in + the report as an unhandled Codex finding for them to judge. +- If the **baseline** version was already a pre-release (`git show "$BASE:/gradle.lockfile" | grep ...` matches + too), rolling back does not remove the pre-release. Flag that module to the user and let them decide instead of rolling + back silently. +- Findings unrelated to versions (style, logic, anything else) are out of scope for this skill entirely — list them in the + report and move on. + +3. **Map each qualifying comment → module.** The comment's `path` *is* the lockfile, so the module is that path minus + `/gradle.lockfile` and the Gradle path is the same with `/` → `:` and a leading `:`. Confirm the PR actually changed the + lockfile with the same `git diff --quiet` check as Phase 1 step 3 — if unchanged, the finding is stale (Codex reviewed + an older commit); exclude and flag it. + +4. **Dedupe against category A** — a module can be both CI-failed and Codex-flagged. It gets **one** rollback commit, and + category A wins for Phase 4 eligibility (a real CI failure is worth fixing even if it also happens to be a beta, + though the beta itself is usually the cause — say so in the report and let the user choose). + +5. **Report the combined triage** to the user before making any change: one table of every in-scope module with its + category (A / B), Gradle path, lockfile, and — for A — failing task (s) and JVM (s), or — for B — the pre-release + version and a one-line summary of the Codex finding. Add a separate list of Codex findings you did **not** act on and + why. --- -## Phase 2 — Unblock (rollback lockfiles) +## Phase 3 — Unblock (rollback lockfiles) -For **each** failed module, create **one commit** rolling its lockfile back to the pre-update state. Do **not** push -between commits. +For **each** in-scope module — category A *and* category B — create **one commit** rolling its lockfile back to the +pre-update state. Do **not** push between commits. **Approval model:** the rollback commits are local and fully reversible, so create them without prompting. The one action that needs the user's go-ahead is the **push** (step 4) — where changes leave the machine and re-trigger CI. @@ -199,12 +275,17 @@ action that needs the user's go-ahead is the **push** (step 4) — where changes fi ``` -2. Commit that single module's already-staged change (restore or deletion — no `git add` needed): +2. Commit that single module's already-staged change (restore or deletion — no `git add` needed), with the message for its + category so the history says *why* it was rolled back: ```bash - git commit -m "temporary fix: rolled back conflicting latest dependencies for module: to unblock PR merging" + # category A — CI-failed latest-dep source set + git commit -m "temporary fix: rolled back conflicting latest dependencies for module: to unblock PR merging" + + # category B — Codex-flagged pre-release bump (CI was green) + git commit -m "temporary fix: rolled back pre-release latest dependencies (:) for module: , waiting for the final release" ``` -3. Repeat for every failed module — one commit each. +3. Repeat for every in-scope module — one commit each. 4. **Verify, then push once** (after all commits exist). One diff per module confirms the lockfile now matches the baseline — this covers both cases (a restored file has no diff vs `BASE`; a correctly-deleted new file is absent in @@ -224,12 +305,21 @@ action that needs the user's go-ahead is the **push** (step 4) — where changes One push (not per commit) triggers a single CI run. Report the pushed commits and remind the user CI will re-run. +If a category-B rollback also drags an unrelated GA bump in the same lockfile back to baseline, that's acceptable — the +next weekly run re-applies it. Don't hand-edit a lockfile to keep part of the update; whole-file rollback is the only +supported operation. + --- -## Phase 3 — Real fix (opt-in, per module) +## Phase 4 — Real fix (opt-in, per module) + +**Category-B (Codex pre-release) modules are excluded from this phase — never open a real-fix PR for them.** Say so +explicitly when reporting; the resolution is "wait for the GA release", not a code change. If the user asks for one +anyway, tell them once why it's likely throwaway (the pre-release API can still change), then do it if they confirm. -After unblocking, **ask** the user whether to create separate real-fix PRs (one per failed module). If no, stop and hand -them the module list. If yes, work modules **one at a time** — fully finish and verify one before starting the next. +After unblocking, **ask** the user whether to create separate real-fix PRs (one per **category-A** module). If no, stop +and hand them the module list. If yes, work modules **one at a time** — fully finish and verify one before starting the +next. For each module: @@ -316,14 +406,19 @@ For each module: git checkout ``` -When done, summarize: unblock commits pushed, and for each module either the fix PR URL or that it was skipped. +When done, summarize: unblock commits pushed, and for each module either the fix PR URL, that it was skipped, or — for +category B — that it was rolled back and is intentionally waiting for the dependency's GA release. --- ## Guardrails -- **Scope:** only modules whose latest-dep source set failed. Phase 2 changes **only** those modules' `gradle.lockfile` - s — never source code, never another module. Phase 3 may extend beyond the failed module only with clear - justification, and verification must then cover every module you touched. -- **Approvals:** get the user's go-ahead before the Phase 2 push and before committing/opening any Phase 3 PR. +- **Scope:** only modules whose latest-dep source set failed (A) or that Codex flagged as bumped to a pre-release (B). + Phase 3 changes **only** those modules' `gradle.lockfile`s — never source code, never another module. Phase 4 may + extend beyond the failed module only with clear justification, and verification must then cover every module you + touched. +- **Pre-release modules get no fix PR.** Rollback only — the fix is the upstream GA release. +- **Never act on a Codex comment without verifying it** against the actual lockfile diff, and never silently discard the + ones you didn't act on — list them for the user. +- **Approvals:** get the user's go-ahead before the Phase 3 push and before committing/opening any Phase 4 PR. - Keep Gradle runs sequential.