Fix: document the service URI schemes, the DATABASE_URL fall-back and the memory:// defaults in adk --help - #385
Open
AmaadMartin wants to merge 3 commits into
Open
Conversation
added 2 commits
July 31, 2026 05:30
… in adk --help The --session_service_uri help advertised only memory:// and --artifact_service_uri only gs://, so the persistent backends the CLI already dispatches to were invisible unless a user read the registry source. getSessionServiceFromUri also routes postgres://, postgresql://, mysql://, mariadb://, mssql:// and sqlite:// to DatabaseSessionService, and getArtifactServiceFromUri also routes memory:// and file://. Both descriptions now name every scheme their registry accepts. This is a documentation fix: no dispatch logic changes. Because the two Option instances are shared, the corrected text renders for web, api_server, run, deploy cloud_run and deploy agent_engine. Two unit tests pin the "documented is a subset of accepted" invariant by reading the descriptions off the shared Options and feeding each named scheme back through the real registry factories, so the help text cannot silently drift from the registries again.
The tests assert "documented is a subset of accepted", but the names
("documents every session service URI scheme the registry accepts") and the
session table's doc comment claimed the reverse direction too. The registry
also routes vertexai://, which the table deliberately omits, so adding a
scheme to isDatabaseConnectionString without touching the help text would
still pass despite what the test name promised. Rename both tests to "names
only ... schemes the registry accepts" and record why vertexai:// is excluded
on the table itself.
Also assert not.toThrow() rather than toBeDefined(): both registry factories
return non-nullable types, so toBeDefined() could never fail. Throwing on an
unroutable URI is the actual failure mode being pinned.
…efaults `--session_service_uri` and `--artifact_service_uri` named their schemes but never said what happens when they are unset. `getSessionServiceFromOptions` falls back to the DATABASE_URL environment variable and then to `memory://`; `getArtifactServiceFromOptions` falls back to `memory://` only. Both help strings now state that. The session string also names `sqlite://:memory:`, which `getConnectionOptionsFromUri` special-cases into MikroORM's in-memory database. The fall-back belongs to the process that serves the agent. `web`, `api_server` and `run` apply it locally; the two `deploy` commands forward the raw flag and omit it when unset, so the deployed container applies the same chain against its own environment. No behaviour changed. Four new tests pin the fall-back chain, which nothing covered before, and two pin the new wording.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
No existing issue.
Problem:
adk --helpnamed one URI scheme per service flag, while the registries route seven session schemes and three artifact schemes. The help also never said what an unset flag does.getSessionServiceFromOptionsreads theDATABASE_URLenvironment variable and then falls back tomemory://, so a user who exportedDATABASE_URLfor another tool writes agent sessions into that database without knowing.DATABASE_URLappeared in exactly one line of the repository and in no help string.Solution: Both descriptions now name every scheme their registry accepts and state what an unset flag does. The session text also names
sqlite://:memory:, whichgetConnectionOptionsFromUrispecial-cases into MikroORM's in-memory database. The fall-back belongs to the process that serves the agent:web,api_serverandrunapply it locally, and the twodeploycommands omit the flag when it is unset, so the deployed container applies the same chain against its own environment. No dispatch logic changed — the source diff is two string literals.Both options are single shared
Optioninstances, so one edit correctsweb,api_server,run,deploy cloud_runanddeploy agent_engine.Deliberate omission:
vertexai://stays undocumented.getSessionServiceFromUrimatches it but discards the URI and buildsVertexAiSessionService({}), which throwsProject ID and Location are required., so the path cannot work from the CLI today. PR #331 makes the registry parse the URI; documenting the scheme belongs there.Collision check:
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, thengh pr diff --name-onlyon every adjacent PR. #433, #693, #587 and #358 also editdev/src/cli/cli.ts, but none touch these two descriptions. #348 scrubs an ambientDATABASE_URLin the sharedbeforeEachof the same test file. To avoid duplicating it, the new tests stubDATABASE_URLper test instead of changingbeforeEach; the only shared-hook edit here isvi.unstubAllEnvs()inafterEach.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
npx vitest run --project unit:dev dev/test/cli/cli_test.ts— 32 passed. No existing test was modified or deleted.Two tests read the descriptions off the shared
Optionand feed a sample URI for every named scheme back through the real registry factory. Six cases were added on top: two pin the new wording, and four pin the fall-back chain, which nothing covered before.Every test was mutation-proven. Each mutation failed exactly one test:
dev/src/cli/cli.ts|| process.env.DATABASE_URLmemory://->sqlite://:memory:DATABASE_URLbefore the flagDATABASE_URLsentence(or sqlite://:memory:)If unset, memory://.memory://->file:///tmp/adknpm run lint,npm run format:checkandtsc --noEmitreport nothing on either changed file.dev/test/cli/cli_create_test.tsfails on this machine with and without the change, because it reads an ambientGOOGLE_CLOUD_PROJECT; PR #569 fixes that.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
All five commands render both sentences:
A no-mocks run of the real server proved the documented chain, with no flag passed:
adk_sessions.dbappeared, and a restart against the sameDATABASE_URLreturned the session.DATABASE_URLunset returned404 Session not found: s1, which is thememory://default.DATABASE_URLset and--session_service_uri memory://also returnedSession not found, which is the documented precedence.An earlier run booted the same server with explicit
--session_service_uri sqlite://...and--artifact_service_uri file://...and round-tripped a session and an artifact.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.