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 one source-model ownership regression. The core build script grows from 274 to 281 lines, without crossing the size threshold.
Source-only review of f5410c45493f74d60b93fd1423ec51bbe8318420. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only validation; no builds or tests executed.
- Focused independent validation, not a repetition of the completed full reviews.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed deterministic compilation grouping, shared fixture visibility, compiled outputs/test discovery, and GraalVM task exclusions. Inspected all 1,892 partitioned generated tests for cross-file test dependencies: only service tests use the separately compiled TestServerExtension; ContractCases bridges remain with handwritten tests. No actionable findings.
Hosted CI at review: no failed checks observed. 4 checks pending.
Static review with an independent reviewer; 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 5b64e40. Actual Kotlin source sets now own exact partitioned sources, directory traversal admitted without widening file membership; compiler inputs derive from sourcesets; independent actual membership verifier covers originals exactly once and is required by testClasses/all Test tasks. Shared fixture/friend paths/aggregate classes intact. Static review only; no candidate code or local tests executed. Hosted CI follow-up retained.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
One dependency-model ownership issue remains. The source-membership repair does not address it. Core build script grows from 274 to 299 lines without crossing 1,000 lines.
Source-only review of 5b64e406d4fac16eeb2bb7bcc7c2c8e7370351da. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only focused validation; no builds, tests, or IDE imports executed.
- Inspected representative consumers, not every unchanged generated test.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Revising my earlier approval after verifying the new dependency-model finding: #1362 (comment) . Shared fixture output is added only to KotlinCompile.libraries; the service and ordinary test SourceSet.compileClasspath still omit it. Source-set consumers therefore cannot resolve the fixture dependency used by tests such as ModelServiceTest, despite compiler tasks having it. Retain the fixture SourceSet and add its output to the consuming source-set compile classpaths, avoiding a self-dependency for fixture/model groups. Extend verification to assert these classpaths, while retaining friend paths and aggregate outputs. Referencing the existing comment rather than duplicating it inline.
Coordinator and independent static review; no candidate code or local tests executed. The previous membership fix remains correct.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved the source-set dependency fix at 8610fd7. Service and original test source sets now include shared fixture outputs in their actual compile classpaths, with friend paths retained for internal visibility. Capturing the original classpath before consumer reassignment keeps fixture/model source sets independent. The verifier checks complete fixture class-directory membership on consumers and absence elsewhere. This addresses #1362 (comment) without changing aggregate discovery. Hosted CI pending. Coordinator and independent 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 actionable findings. Prior source-set ownership and fixture-classpath issues are addressed. Reviewed partitioning, shared fixture dependencies, aggregate discovery, verification and GraalVM exclusions. No 1,000-line crossing: core build script grows 274→306 lines; existing policy test grows 1,509→1,516.
Source-only review of 8610fd7a52799ecd2cb95a7dab37e2211d83ca0c. Two independent source reviews; no candidate code, tests, builds or generators executed.
Validation limits:
- Source-only review; no builds or tests executed.
- Inspected representative unchanged test consumers, not every generated test.
- Source-only review; no builds, tests, or IDE imports executed.
Compile generated core model tests in eight byte-balanced groups and generated service tests in two groups. Compile the server fixture once, preserving its shared lock, and retain handwritten tests and generated contract-case bridges in the original compilation. Sources stay in place and the normal/Jackson test tasks discover the aggregate outputs. A Gradle verification task checks that all 2,099 original Kotlin test sources belong to exactly one source set; compiler inputs are derived from those source sets. Service and ordinary test source sets explicitly depend on the shared fixture output; verification also checks their classpaths and keeps model/fixture compilations independent.
Update GraalVM tracing to skip these compiler tasks after the normal build, while retaining support for older release checkouts. Production compilation and published artifacts are unchanged.
A same-machine paired experiment reduced forced test compilation from approximately 95s to 29s, with average busy cores increasing from 2.4 to 10.1. A separate forced main-plus-test Kotlin compilation comparison improved from approximately 178s to 128–130s. These measurements predate subsequent production-compilation changes; they are not additive or a current overall CI speedup claim.
Validation: full local
:buildSrc:testpassed, including source-assignment invariants and GraalVM task-selection coverage; full Gradle configuration andgit diff --checkpassed. Public CI validates compiled tests, compatibility and consumers. Independently based on main; no telemetry or other experiment settings are included.