Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Retained the Windows classification regression and cache-policy coverage gap, with a corrected cache-test remedy. No changed file crosses 1,000 lines.
Source-only review of d18360c8195ce29c4c4e23789d62926cfe6f0be7. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only validation; no commands, builds, or tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Confirmed the existing Windows partitioning finding: #1364 (comment) . Native File.path separators do not match the slash-only model/service classifiers on Windows. All sources become shared, leaving shards 1–7 empty; the new unrestricted TestKit regression requires SUCCESS for those NO-SOURCE tasks. Normalize separators for both classifications while preserving service grouping and shared fixtures. Referencing the existing finding without a duplicate inline comment.
No failed hosted checks observed. 4 checks pending.
Static review with independent review; no candidate code or local tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed exact commit 070584b. The latest increment explicitly resolves Kotlin stdlib 2.2.21 before the offline TestKit fixture and aligns repository metadata with the parent dependency cache, addressing the observed prior-head CI failure. The preceding separator and cache-policy assertions remain intact. Approve source changes; hosted rerun is pending, not verified green. Static review only; no candidate code or local tests executed.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
No demonstrated actionable findings. Prior separator and cache-policy findings are addressed. Reviewed partitioning, policy isolation, compiler configuration, consumers, and regression coverage. File sizes remain below 1,000 lines: 69→80, 0→157, and 503→594.
Source-only review of 070584b7a6c8818cf70e004b1871f5a20a64f96e. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only review; no code, builds, or tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved the isolated fixture dependency configuration. compatibilityTestDependencies resolves Kotlin stdlib 2.2.21 independently of the embedded Kotlin/test runtime, and declaring it as a test input forces resolution before offline TestKit starts. Repository identities and shared read-only cache remain aligned; the offline, group-coverage, fresh-compilation and expected-failure assertions remain intact. This addresses the observed missing-stdlib failure on the previous head; fresh hosted CI is pending, so qualification remains open. Coordinator and independent static review; no candidate code or local tests executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Request changes at b0768d3 based on the new hosted failure: the offline TestKit fixture still lacks its Kotlin build-tools dependency. The stdlib fix alone does not make the fixture reproducible on clean CI runners. Separately, OkTest’s test job succeeded but its reporter rejected mismatched merge parents; that is distinct from this source/test setup defect. Coordinator and independent static follow-up review; no candidate code or local tests executed.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Dismissed the parallel-scheduling candidate because configuration caching already enables concurrent execution. Retained the independently assessed offline dependency-preparation gap raised in the updated discussion.
Source-only review of b0768d3e422aff0759f55633687c6373ec0dddad. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only validation; no commands, builds, tests, or CI queries performed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved d9f6ca0. This addresses my offline TestKit finding: the build-tools implementation and scripting compiler are now resolved transitively before the fixture runs, with an explicit isolated TestKit directory and the read-only parent dependency cache. Fresh compilation, cache-disabled assertions, and the deliberate proposed incompatibility check remain intact. Coordinator and independent static re-review found no further actionable issues. Hosted CI is still pending; no candidate code or local tests executed.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
No demonstrated actionable findings at d9f6ca0. Reviewed partition coverage, policy isolation, compiler settings, cache enforcement, dependency preparation, and regression tests. Prior findings are addressed. File sizes remain below 1,000 lines: 69→88, 0→158, and 503→594.
Source-only review of d9f6ca084899678d05aa31016d015b8055ca2e55. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only review; no code, builds, or tests executed. CI results were not used.
- Source-only review; no code, builds, or tests executed. CI results were not evaluated.
Compile API compatibility inputs in eight independent Gradle projects per selected policy. Balance model tests by source bytes, keep mutually dependent service tests together, and share supporting fixtures. Preserve the exact source union and inherit the original compiler, JVM target, classpath and options. Every compatibility compilation remains mandatory and bypasses the build cache.
Keep the existing policy behavior: identical baseline/proposed inputs compile once; different inputs compile both policies independently; both public API manifests are still checked. This also works without configuration-cache reuse. The temporary compiler projects exist only during the detector invocation and do not change published artifacts.
A preceding same-machine experiment reduced the compatibility compilation phase from approximately 128s to 22–23s. That is a historical phase measurement, not an overall CI speedup claim for this PR.
Validation: full local build-logic suite passed. The Gradle TestKit regression exercises single and dual policies, all eight groups, mandatory recompilation, shared fixtures, mutually dependent services, and failure of an incompatible proposed source. The fixture uses a Java 21 toolchain targeting Java 8, matching the SDK. It enables an isolated build cache, verifies that normal SDK compilation stores a cache entry, and checks that every compatibility compiler explicitly disables caching and stores no entry. The fixture compiler dependencies are resolved in a separate configuration, independently of Gradle's embedded Kotlin version. TestKit uses a fresh directory and reads those dependencies offline. The full build-logic suite passed with an empty Gradle dependency cache and fresh TestKit home, including the cache-enabled regression. Shell syntax and
git diff --checkpassed. Full public CI validates the real SDK source set.Independently based on main; no telemetry or other experiment settings are included.