Repository navigation
fix(translator): honour a BYO harness's configured PORT so the card and runtime agree - #2800
wangzhengzhuo05 wants to merge 2 commits into
Conversation
…untime agree The byo compiler advertised the agent card at http://127.0.0.1:80 while injecting no PORT, so a BYO image built with go/adk/pkg/app listened on its 8080 default, reported READY, and failed every invoke. Mirror the kagent compiler and inject PORT=80 so the advertised port and the runtime agree by construction. Fixes kagent-dev#2758 Signed-off-by: wangzhengzhuo05 <175673456+wangzhengzhuo05@users.noreply.github.com>
|
I approved this, but actually I realized that I'm not sure it makes sense. BYO is meant to be opaque harness, so I'm not sure we should auto inject that variable, maybe the user can do it via config? |
A BYO harness is opaque, so a port configured through spec.env must win over the compiler's default. The card now advertises the same port the environment resolves to, and PORT=80 is only injected when the harness configures nothing. Signed-off-by: wangzhengzhuo05 <175673456+wangzhengzhuo05@users.noreply.github.com>
|
Thanks for the review — agreed, and reworked. BYO is opaque, so the harness's own configuration now wins:
Covered by
|
|
Sorry for changing my mind here, but I actually think that this requires an API change. Part of any BYO harness integration is the port on which they serve A2A. Therefore I think that port should be a setting in the BYO section of Harness. What do you think? That would also allow us to avoid reading the |
| if environment[i].Name != "PORT" { | ||
| continue | ||
| } | ||
| port, err := strconv.Atoi(environment[i].Value) |
There was a problem hiding this comment.
credentialref port falls through to the 80 default here?
|
Thanks for the second look — and no worries at all about changing your mind, I think you're right. If the A2A port is part of what defines a BYO harness integration, then it belongs in the Closing it. For the record, what this branch contained (in case any of it is worth folding into the API-shaped version): it made the inference path and the rendered card agree on a single port — a Thanks for the careful review — the push-back was the most useful part of this thread. |
What
The
byoharness compiler advertised the agent card athttp://127.0.0.1:80while injecting noPORT, so a BYO image built withgo/adk/pkg/applistened on its8080default, reportedREADY, and then failed every invoke.The port is now taken from the harness's own configuration: whatever
PORTthe harness sets inspec.envis what both the runtime receives and the card advertises.PORT=80is only injected when the harness configures no usable port.Why
go/core/internal/translator/byo/compiler.gobuilt the card with a hard-codedhttp://127.0.0.1:80, but its environment was onlycompiled.Environmentplusadkconfig.HarnessEnvironment(harness).go/adk/pkg/appresolves the listen port ascfg.Port->PORTenv -> default8080, so the card and the runtime disagreed. Readiness is probed on:8081independently ofPORT, so nothing surfaced the mismatch and the instance stayedREADYwhile unreachable.BYO is an opaque harness, so the compiler must not force a port it cannot know (review feedback on this PR). At the same time the card and the runtime must not be able to disagree, so the advertised port is derived from the environment that is actually rendered.
How
adkconfig.DedupeEnv(compiled.Environment + HarnessEnvironment(harness)).configuredPortreads the lastPORTentry in that environment (the same last-wins semanticsDedupeEnvuses). A value in1..65535is the harness's port, and nothing is injected.80is appended, matching the built-inkagentcompiler for an unconfigured BYO harness.agentTemplateCardnow takes the resolved port and buildshttp://127.0.0.1:<port>.BuildProvenanceso the provenance hashes the same environment that ships.No API or CRD change.
Tests
TestCompilePortResolutioncovers: noPORT-> exactly onePORT=80and cardhttp://127.0.0.1:80;spec.env PORT=8080-> exactly onePORT=8080and cardhttp://127.0.0.1:8080;PORT=banana-> falls back to exactly onePORT=80and cardhttp://127.0.0.1:80.TestCompileOpaqueImageunchanged in intent.cd go && go test ./core/internal/translator/byo/...passes;cd go && go build ./...passes;gofmt -lclean.configuredPortto always report "not configured" makes thePORT=8080case fail, so the regression test guards the behaviour rather than the implementation.This change was implemented with AI assistance (OpenCode + muse-spark 1.3) and reviewed and verified locally; I understand it and take responsibility for it.
Fixes #2758