mirror of
https://github.com/google/adk-go.git
synced 2026-10-28 16:44:38 +08:00
Sashaandwolo
9171bd6b97
* fix(launcher): pin the webui-before-api order, and clarify the adk-web pin Two loose ends from review of #1459 and #1460, neither changing behaviour. The API sublauncher mounts a catch-all route when its path prefix is empty, which matches every path including /ui/. gorilla/mux serves the first route that matches, so the web UI survives only because NewLauncher passes webui before api. Nothing enforced that: swapping the two arguments made the entire UI 404 with no build or test failure. Add a test that starts the server with an empty API prefix and asserts /ui/ still serves the UI document. The Dockerfile buried the important caveat at the end of a fifteen-line comment, where a reader asked what it was for. ADK_WEB_REF does not describe the bundle committed in cmd/launcher/web/webui/distr/, which was built on 2026-06-30, before the pin existed, and records no provenance. Building at the pin moves the UI forward rather than reproducing what is in the tree. Lead with what the argument is, state the gap plainly, and move the reasoning to README.md, which gains a section on it. Testing Plan: go build, go vet, go test -race -shuffle=on ./cmd/... and golangci-lint clean. The ordering test is mutation-proved: swapping the arguments to web.NewLauncher makes GET /ui/ return 404. * fix(launcher/full): drive the real launcher in the ordering test Review found the test did not test what it claimed. It built its own launcher from webui and api rather than calling NewLauncher, so the composition in full.go was unprotected. Swapping the production order there left the test green, which is the exact regression its comment describes. Drive NewLauncher through Execute instead. The composition is the thing being pinned, and a test that assembles an equivalent launcher only asserts that its own argument order works. Also from the same review: - Surface the launcher error rather than discarding it. A bind failure previously surfaced as the /ui/ 404, reporting a regression that had not happened. - Bound each request, not only the retry loop. A server that accepts and never answers used to hang until the package timeout, which CI leaves at the ten minute default. - Do not dereference the response on a path where it can be nil. - Cut the duplicated documentation. The new README section repeated what the provenance section already said, and the rewritten Dockerfile comment had kept the old opening line alongside its replacement. Testing Plan: go build, go vet, go test -race -shuffle=on ./cmd/... and golangci-lint clean. Mutation-proved against the real composition: swapping webui and api in full.go now fails with "GET /ui/ status = 404, want 200". * fix(launcher/full): cover every route the api catch-all can shadow The ordering test covered webui only. Three other sublaunchers are registered before api and are exposed to the same shadowing: a2a, pubsub and eventarc. pubsub and eventarc matter more than the case already covered. They default to --path_prefix /api, which is api's own default, so they collide with it in the shipped configuration rather than only when api is given an empty prefix. Registering api first makes POST /api/apps/{app}/trigger/pubsub return the REST 404 instead of reaching the trigger handler. Make the test table-driven over all four. The rows that need an empty api prefix and the rows that need the defaults want different servers, so it starts two. pubsub and eventarc are POST-only, and gorilla/mux falls through to the catch-all when the path matches but the method does not, so a GET row there would pass whatever the order is. Report a lost bind as a lost bind. freePort hands out a port number rather than a held listener, so another process can take it before the launcher binds. The readiness probe cannot tell the two apart, so a row could pass against a foreign server, or fail as a shadowing regression that had not happened. Documentation, from the same review: - Drop the SHA, tag and date restated above ARG ADK_WEB_REF. They were held in three hand-maintained places and update-adk-web.sh writes none of them, so a bump left two stale. - Remove the section describing the gap between the pin and the committed bundle. The provenance section already covers it. - Bump step 1 asked for the tag in the comment above ADK_WEB_REF, which puts back what this removes. Name it in the commit message. Testing Plan: go build, go vet, gofmt and go test -race -count=1 -shuffle=on ./cmd/... all clean. Mutation-proved: moving api.NewLauncher() to the front of full.go fails all four rows with "api is shadowing the route". Under 24 concurrent processes at -test.count=25, every red names the bind failure and none reports shadowing. --------- Co-authored-by: wolo <wolo@google.com>