[vuanhphung]

Description

Engineer Bot follow-ups cannot reuse Maven plugins downloaded through JFrog after switching to empty offline settings, as seen in #1742.

Normalize cached repository IDs from jfrog-central to central, and warm the Surefire provider plus the launcher matching the resolved JUnit Platform engine before scrubbing credentials. Reuse the author workflow's pinned Surefire fallback when the lookup fails; warn and skip launcher warming if its version is unavailable. The existing build already resolves declared dependencies and build plugins.

Run one fast unit test offline after credentials are cleared, then restore Spotless changes before the agent starts. Warm-up and smoke-test failures remain non-blocking. Bound those steps to seven minutes total and give the 30-minute agent setup headroom within a 45-minute job.

Testing

An isolated real-Maven fixture reproduced rejected cached jar/Spotless plugins, a missing Surefire provider, and a missing matching JUnit launcher. After the simplified warm-up and normalization, the offline JUnit test passed (1 test, 0 failures).

Validated YAML, Bash, credential/test/cleanup ordering and unchanged workflow gates. Shell checks cover successful and failed version lookups and an unavailable engine version. git diff --check passed. The real-Maven fixture also exercised the missing-help-plugin failure and Surefire fallback; the provider fetch warned about test transitives absent from the local file mirror, but the final offline test passed. The successful property lookup and full Java 21 protected-runner workflow still need CI validation.

Telemetry Errors

• Not applicable — this PR does not add or change a telemetry-visible error.

NO_CHANGELOG=true

---

This PR was created with GitHub MCP.

[peco-review-bot]

Verdict: 1 Medium

Approach looks right and matches the author workflow and warmMavenCache.yml: warm Surefire while credentials are live, normalize the repo IDs, then check offline before the agent runs. One medium concern: both test runs now hard-fail on the PR branch's own test state, so a branch whose test sources don't compile can no longer be fixed by a follow-up.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

Mostly sound: the warm-up, ID normalization and moving the spotless revert later all match engineer-bot.yml. There are three issues, the main one (medium) being that the new hard offline verify step can stop the follow-up with no reply on the PR and an error that hides the real cause.

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

The fix is mostly sound, with one medium concern: since the only JUnit test now runs after the offline switch, artifacts Surefire resolves at runtime (such as junit-platform-launcher) may never be cached. The step ordering, credential scrubbing and repo-ID normalization otherwise match engineer-bot.yml.

Other findings

• ⚪ Nit — The smoke test is pinned to DatabricksParameterMetaDataTest#testInitialization on the PR head branch. If a follow-up PR renames or removes that test, Surefire fails with "No tests matching pattern". That gets reported with the same generic "offline smoke test could not complete" warning, so a missing test looks like a cache miss. Consider adding -Dsurefire.failIfNoSpecifiedTests=false, or making the warning mention this possibility, so the agent isn't sent chasing a non-existent resolution problem.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks reasonable overall, with two low-severity concerns: the warm-up now relies on a hand-picked artifact list because the smoke test runs offline, and the warning messages claim the agent will be told about offline problems when nothing tells it. Nits: mvn -B dependency:get without -N/-pl runs once per reactor module, which is redundant but harmless. The header comment (L11-12, "Same spotless-churn revert + credential-scrub as the author") no longer describes this workflow, since the step order and the offline smoke test now differ from engineer-bot.yml.

Other findings

• ⚪ Nit — The warning messages say "the follow-up agent will report verification limits", but nothing in the workflow passes these results to the agent. They are only GitHub annotations: no file is written, no $GITHUB_ENV/step output is set, and the Run engineer follow-up step's env has nothing about them. The agent only finds out about a cache miss when its own mvn test fails. Either reword the warnings (e.g. "offline mvn test may fail"), or export a flag (e.g. echo "MAVEN_OFFLINE_DEGRADED=1" >> "$GITHUB_ENV") that the prompt or engine can read.

[peco-review-bot]

🔵 Low — The smoke test now runs after the scrub with -o, so it can no longer download anything. The author workflow runs this same test online because it is the step that picks up whatever Surefire resolves lazily at execution time. Here, that coverage depends on the hand-picked list (provider + junit-platform-launcher matched to the engine version). If Surefire or JUnit is bumped and needs another lazily-resolved artifact, this step only prints a warning, and the agent hits the same "absent" dead-end this PR is fixing.

The security gain is small: the Build driver step already runs PR-controlled build plugins with JFrog credentials live. Two ways to tighten this:

• Run the smoke test online (as engineer-bot.yml does), then repeat it offline as the check.
• Or keep the current order, but use ::error:: instead of ::warning:: when the offline smoke test fails because of a resolution error, so the regression can't go unnoticed.

[peco-review-bot]

Verdict: 1 Low

The fix looks right. Normalizing the repo ID and warming the Surefire provider and JUnit launcher before scrubbing credentials should solve the offline cache miss, and every pipefail fallback I traced handles failures correctly. One low-severity concern: the extra warm-up and offline-check Maven runs come out of the job's 30-minute limit, and the agent step has no timeout of its own.

[peco-review-bot]

🔵 Low — This PR adds about six more cold Maven runs before the agent starts: dependency:resolve/resolve-plugins, help:evaluate, two dependency:get, dependency:list, the offline re-check (up to three more runs) and an offline test run. The test run goes through spotless and compile again. All of this comes out of the job's fixed 30-minute limit. The follow-up step has no step-level timeout-minutes of its own, so if setup runs long the agent is cancelled at the job limit and doesn't fail cleanly. The author workflow had this exact problem and sized its budget explicitly to avoid it (see the budget comment in engineer-bot.yml).

Two possible fixes: (a) give Run engineer follow-up its own timeout-minutes and raise the job limit to leave the same headroom, or (b) cut the offline "Check" step down, since the smoke test already shows whether the offline cache works.

[peco-review-bot]

Verdict: 2 Low

Looks good, with two low-severity notes about the new offline warm-up and smoke-test steps. The step ordering, the jfrog-central→central rename, and the timeout budget (26 + 30 + 4 = 60 min) all add up. The Surefire version property exists in pom.xml, and the smoke-test target DatabricksParameterMetaDataTest#testInitialization exists in the test sources.

[peco-review-bot]

🔵 Low — The offline smoke test is capped at timeout-minutes: 1, but it runs a full mvn test lifecycle on a cold JVM: spotless:apply over the whole module (bound to compile per the old comment), incremental compile and test-compile, and a forked Surefire JVM. That can easily take close to a minute. When it overruns, Actions kills the step. You get the generic timeout annotation, not the custom ::warning:: explaining the cache miss, and the Surefire/launcher runtime classpath warm-up this step is meant to prove never gets validated. Because of continue-on-error, the job continues silently either way. The budget has room for more: the comment says 26 min of setup, and the steps add up to exactly 26. Consider giving this step 3–4 minutes, taken from the 6-minute build or warm-up caps, so the smoke test actually gives a signal.

[peco-review-bot]

🔵 Low — SUREFIRE_PROVIDER comes from steps.warm-maven.outputs.surefire-provider. If the warm-up step times out or dies before writing to $GITHUB_OUTPUT, which could happen during the first dependency:resolve call (this step has continue-on-error: true, so the job keeps going), the output is empty. The check step then runs mvn -o -N -B dependency:get -Dartifact= unconditionally. That fails with a confusing missing-parameter error, and the warning wrongly blames the Surefire provider. JUNIT_LAUNCHER already has an [[ -n ... ]] guard. Add the same guard to SUREFIRE_PROVIDER, or write the 3.1.2 fallback to the output before the first mvn call.

[peco-review-bot]

Verdict: 1 Medium · 2 Nit

The approach is sound, with one medium concern and two low ones. The new "Warm test runtime offline" step can only detect cache misses, not warm the cache. The rest are small robustness gaps in the offline check and the timeout budget.

Other findings

• ⚪ Nit — SUREFIRE_PROVIDER comes from steps.warm-maven.outputs.surefire-provider. That output is never written if the warm step times out or aborts before line ~133 (it has continue-on-error: true and a 6-minute cap). In that case this runs mvn -o -N dependency:get -Dartifact=, which fails on a malformed coordinate and prints the misleading warning "The Surefire provider ... unavailable offline". Guard it the same way JUNIT_LAUNCHER is guarded below (if [[ -n "$SUREFIRE_PROVIDER" ]]), or fall back to the 3.1.2 coordinate.
• ⚪ Nit — A 1-minute cap on mvn -o -pl jdbc-core test looks tight. Even for one test, the run goes through the bound spotless:apply over the whole module, a compile/test-compile up-to-date check, and Surefire fork startup on a cold JVM. If it times out, the step only logs a generic failure (continue-on-error), so the check gives no signal exactly when it matters. The setup budget currently sums to exactly 26 minutes, so giving this step about 3 minutes (taken from the 6-minute warm-up or build allowance) would make the check reliable without exceeding the job cap.

[peco-review-bot]

🟡 Medium — The step is called "Warm test runtime offline", but it runs after the scrub with MAVEN_ARGS=-o, so it can't fetch anything. It can only report whether the cache is already complete. The sibling author workflow runs this same test while credentials are live, because Surefire resolves some artifacts lazily at execution time. This PR handles the two lazy artifacts it already knows about (the provider and the launcher). Any other execution-time artifact still can't be fetched here; it only shows up as a warning, and the agent then hits the same offline dead-end this PR is fixing.

If the reason for running PR tests only after the scrub is to keep PR test code away from the JFrog token, note that the earlier Build driver step already runs PR-controlled build code (plugins, spotless) with that token live. Either:

• run the smoke test once before the scrub to actually warm the cache, as engineer-bot.yml does, or
• rename this step (e.g. "Verify test runtime offline") and state in the comment that it only detects missing artifacts and doesn't warm anything.

[peco-review-bot]

Verdict: 1 Low

Looks good. There is one low-severity robustness concern: if the Surefire version lookup fails, the warm-up step aborts and skips the launcher warm-up too. The order of credential scrub, smoke test, and Spotless revert is correct, and the repo-ID normalization matches what engineer-bot.yml already does.

[peco-review-bot]

🔵 Low — The step runs under set -euo pipefail. If help:evaluate fails (for example, the help plugin can't be resolved or a transient mirror error occurs), set -e aborts the step at this assignment. Both dependency:get calls are then skipped, including the JUnit launcher warm-up, which doesn't depend on the Surefire version. Because of continue-on-error, you only get a generic step-failure annotation instead of your ::warning:: lines. The cache then reaches the offline agent without the provider or launcher, and the agent hits the same "surefire-junit-platform (absent)" error this PR is meant to fix.

The author workflow avoids this with a fallback version. Suggest the same here: SUREFIRE_VERSION=$(mvn ... -q -DforceStdout 2>/dev/null | tail -n1) || true followed by [ -n "$SUREFIRE_VERSION" ] || SUREFIRE_VERSION=3.1.2. Likewise, when JUNIT_PLATFORM_VERSION comes back empty, emit an explicit warning rather than running dependency:get with a malformed junit-platform-launcher: coordinate.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity concern. The steps run in the right order (warm with credentials, normalize repo IDs, scrub credentials, run the offline smoke test, revert Spotless churn), the help:evaluate property exists in pom.xml:57, and the timeout budget fits.

[peco-review-bot]

🔵 Low — The offline smoke test is the only check that the warmed ~/.m2 actually works offline, and its result is thrown away. It has both continue-on-error: true and || echo "::warning::...", so it can never fail the step. Because it runs after the credential scrub, it also can't download anything that's still missing, unlike the author workflow, which runs the same test while credentials are live.

So if the warm-up misses an artifact Surefire loads lazily (your fixture already saw warnings about missing provider transitives), the job still starts the agent. The agent then spends up to the 30-minute step limit hitting the same offline cache-miss failure as #1742, and the only sign of trouble is a yellow annotation in the run log.

Two options: give the step an id and skip or short-circuit the follow-up step when its outcome is not success, or at least note in a comment that this is purely diagnostic. Keeping warm-up failures non-blocking is reasonable. A failed offline smoke test, though, is a reliable sign that the agent's mvn test will fail too.