This is a draft PR, so I'm judging whether it's ready to graduate to human review. It is — the implementation is solid and I'd be comfortable with it being merged after a final human sign-off.
What the change does: it threads image EXPOSE metadata through the build pipeline — from BuildResult.ExposedPorts (populated in both the stackbuild and Dockerfile paths) — and uses it to auto-populate svc.Port on the synthesized web service when exactly one TCP port is declared and nothing in app.toml already sets a port.
What I like about the implementation: The guard in defaultServicePortFromImage is conservative and correct: it skips the service if it isn't named "web", if it carries a custom Image (right call — EXPOSE metadata belongs to the app image, not a sidecar), if a scalar Port is already set, or if a Ports array is present. All four conditions are exercised by the test cases. singleExposedTCPPort is correctly strict: it rejects multiple ports (ambiguous), UDP, and out-of-range values, and it treats a bare port number (no / suffix) as TCP, matching Docker's own semantics.
The refactor in buildkit.go is clean: extracting applyImageConfig and exposedPortNames from the inline anonymous struct lets both the stackbuild path and the Dockerfile path share the same parsing, and the test TestApplyImageConfig now exercises the parsing directly. The sort in exposedPortNames makes port order deterministic, which matters because the test asserts a specific slice order (["4000/tcp", "8080/tcp"]).
One small thing worth noting: the TestBuildVersionConfigDefaultsWebPortFromImage table test uses buildVersionConfig (the full pipeline) rather than testing defaultServicePortFromImage in isolation. That's fine here — it means the test validates the real call site — but it also means each case goes through appconfig.ResolveDefaults and the full service-building path. Nothing is broken by this; I just want to flag it in case a future maintainer wonders why an isolated unit test isn't available. The coverage of the interesting cases (UDP, multiple ports, existing port wins, custom image, ports array) is thorough regardless.
No correctness issues I can find. The res != nil guard around defaultServicePortFromImage is consistent with the existing guard for WorkingDir. The draft is ready for human review.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1099,
"verdict": "ready",
"event": "comment",
"summary": "This is a draft PR, so I'm judging whether it's ready to graduate to human review. It is — the implementation is solid and I'd be comfortable with it being merged after a final human sign-off.\n\n**What the change does:** it threads image `EXPOSE` metadata through the build pipeline — from `BuildResult.ExposedPorts` (populated in both the stackbuild and Dockerfile paths) — and uses it to auto-populate `svc.Port` on the synthesized `web` service when exactly one TCP port is declared and nothing in app.toml already sets a port.\n\n**What I like about the implementation:** The guard in `defaultServicePortFromImage` is conservative and correct: it skips the service if it isn't named `\"web\"`, if it carries a custom `Image` (right call — EXPOSE metadata belongs to the *app* image, not a sidecar), if a scalar `Port` is already set, or if a `Ports` array is present. All four conditions are exercised by the test cases. `singleExposedTCPPort` is correctly strict: it rejects multiple ports (ambiguous), UDP, and out-of-range values, and it treats a bare port number (no `/` suffix) as TCP, matching Docker's own semantics.\n\n**The refactor in `buildkit.go` is clean:** extracting `applyImageConfig` and `exposedPortNames` from the inline anonymous struct lets both the stackbuild path and the Dockerfile path share the same parsing, and the test `TestApplyImageConfig` now exercises the parsing directly. The sort in `exposedPortNames` makes port order deterministic, which matters because the test asserts a specific slice order (`[\"4000/tcp\", \"8080/tcp\"]`).\n\n**One small thing worth noting:** the `TestBuildVersionConfigDefaultsWebPortFromImage` table test uses `buildVersionConfig` (the full pipeline) rather than testing `defaultServicePortFromImage` in isolation. That's fine here — it means the test validates the real call site — but it also means each case goes through `appconfig.ResolveDefaults` and the full service-building path. Nothing is broken by this; I just want to flag it in case a future maintainer wonders why an isolated unit test isn't available. The coverage of the interesting cases (UDP, multiple ports, existing port wins, custom image, ports array) is thorough regardless.\n\n**No correctness issues** I can find. The `res != nil` guard around `defaultServicePortFromImage` is consistent with the existing guard for `WorkingDir`. The draft is ready for human review.",
"posted_to_pr": true,
"draft": true
}