|
| 1 | +### 16:23 Prompt |
| 2 | + |
| 3 | +Evaluate `java-sdk-tests.yml` and show me the differences with the file from which it was derived, `copilot-sdk-java-00/.github/workflows/build-test.yml`. |
| 4 | + |
| 5 | + |
| 6 | +### 16:25 Response |
| 7 | + |
| 8 | +Read [](file:///c%3A/Users/edburns/workareas/copilot-sdk-00/.github/workflows/java-sdk-tests.yml) |
| 9 | + |
| 10 | +Read [](file:///c%3A/Users/edburns/workareas/copilot-sdk-java-00/.github/workflows/build-test.yml#1-1), lines 1 to 150 |
| 11 | + |
| 12 | +Here's a comparison of the key differences: |
| 13 | + |
| 14 | +## Structural Differences |
| 15 | + |
| 16 | +| Aspect | build-test.yml (source) | java-sdk-tests.yml (new) | |
| 17 | +|--------|--------------------------|---------------------------| |
| 18 | +| **Name** | "Build & Test" | "Java SDK Tests" | |
| 19 | +| **Schedule trigger** | Weekly cron (Sundays 00:00 UTC) | Removed | |
| 20 | +| **Push paths** | `paths-ignore` (README, LICENSE, .github) | No path filter on push to main | |
| 21 | +| **PR paths** | `paths-ignore` pattern | `paths` include pattern (`java/**`, `test/**`, workflow file) with doc exclusions | |
| 22 | +| **OS matrix** | `ubuntu-latest` only | 3-OS matrix (`ubuntu`, `macos`, `windows`) | |
| 23 | +| **Working directory** | None (repo root = java root) | `working-directory: ./java` | |
| 24 | +| **Smoke test job** | Yes (calls `run-smoke-test.yml`, gates main job) | Removed (deferred to Phase 3) | |
| 25 | +| **Permissions** | `contents: write`, `checks: write`, `pull-requests: write` | `contents: read` only | |
| 26 | + |
| 27 | +## Removed Features (intentionally per plan) |
| 28 | + |
| 29 | +- **Smoke test job** — Phase 3 concern |
| 30 | +- **JaCoCo badge generation** + PR creation — not needed in monorepo CI |
| 31 | +- **`validate reference-impl-sync completeness` step** — Phase 4 concern |
| 32 | +- **Upload test results for site generation** — not needed in monorepo CI |
| 33 | +- **`COPILOT_GITHUB_TOKEN` / `COPILOT_CLI_PATH` env vars** — not used |
| 34 | +- **Clone-based test harness setup** (`cd target/copilot-sdk/nodejs && npm ci`) — replaced by local harness install |
| 35 | + |
| 36 | +## Added/Changed Features |
| 37 | + |
| 38 | +- **3-OS matrix** — matches other monorepo SDK test workflows |
| 39 | +- **Explicit Node.js setup** + `npm ci` on nodejs and harness — replaces the clone-based approach since harness is now local |
| 40 | +- **Path-scoped PR triggers** — only fires when java, test, or the workflow itself changes |
| 41 | +- **Spotless/Javadoc gated to Linux only** (`if: runner.os == 'Linux'`) — avoids redundant formatting checks on all 3 OS |
| 42 | +- **Action versions use tags** (`@v6.0.2`) instead of pinned commit SHAs — less secure but more readable; the source used full SHA pins |
| 43 | + |
| 44 | +## Potential Concerns |
| 45 | + |
| 46 | +1. **No SHA-pinned actions** — The source pinned actions to exact commit SHAs (e.g., `actions/checkout@de0fac2e...`). The new file uses version tags (`@v6.0.2`, `@v5`, `@v6`). This is less secure against supply-chain attacks but matches the style used in some monorepo workflows. |
| 47 | + |
| 48 | +2. **Missing `persist-credentials: false`** on checkout — the source had this explicitly set for security. |
| 49 | + |
| 50 | +3. **No `COPILOT_GITHUB_TOKEN`** — The source passed this to `mvn verify` for E2E tests that talk to a live Copilot CLI. If E2E tests in the monorepo still need a real token (vs. only the replay proxy), this would cause test failures. However, since the monorepo E2E tests use the replay proxy from harness, this is likely fine. |
| 51 | + |
| 52 | +4. **`distribution: "microsoft"`** for JDK — both files use this, which is consistent. |
| 53 | + |
| 54 | +5. **Push trigger has no path filter** — PRs are path-filtered, but pushes to `main` will run on every push regardless of which files changed. Other monorepo workflows (e.g., `dotnet-sdk-tests.yml`) may or may not have this same pattern — worth checking for consistency. |
| 55 | + |
| 56 | +### 16:25 Response |
| 57 | + |
| 58 | +Concerns |
| 59 | + |
| 60 | +1. I want a push path filter. Should only run on channges to `/java`. |
| 61 | + |
| 62 | +2. For the 3-OS matrix, make it only for `ubuntu-latest`. |
| 63 | + |
| 64 | +3. For permissions, make it have the same as `build-tests.yml`. |
| 65 | + |
| 66 | +4. Yes, I do want the Jacoco badge creation. Copy that entire mechanism from `copilot-sdk-java-00`. |
| 67 | + |
| 68 | +5. You stated |
| 69 | + |
| 70 | + `COPILOT_GITHUB_TOKEN / COPILOT_CLI_PATH env vars — not used` |
| 71 | + |
| 72 | + Why is this? Looking at `build-tests.yml` those things are clearly in there: |
| 73 | + |
| 74 | + ```yml |
| 75 | + - name: Run Java SDK tests |
| 76 | + env: |
| 77 | + CI: "true" |
| 78 | + COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }} |
| 79 | + COPILOT_CLI_PATH: ${{ steps.setup-copilot.outputs.path }} |
| 80 | + run: mvn verify |
| 81 | + ``` |
| 82 | + |
| 83 | + Surely we need them on the `copilot-sdk-00` side as well. There are Java tests that don't just use the relay proxy. |
| 84 | + |
| 85 | +6. Consider this review comment: |
| 86 | + |
| 87 | + > No SHA-pinned actions — The source pinned actions to exact commit SHAs (e.g., actions/checkout@de0fac2e...). The new file uses version tags (@v6.0.2, @v5, @v6). This is less secure against supply-chain attacks but matches the style used in some monorepo workflows. |
| 88 | + |
| 89 | + I want the pinned actions. |
| 90 | + |
| 91 | +7. Consider this review comment: |
| 92 | + |
| 93 | + > Missing persist-credentials: false on checkout — the source had this explicitly set for security. |
| 94 | + |
| 95 | + I want this set here as well. |
0 commit comments