feat(cli): resolve database names in flags, mark default workspace, name catalog ids as catalog

[eddietejeda]

Summary

Two UX fixes that came out of a full end-to-end sweep of every CLI command against production (plus adversarial re-verification of each finding against the source), plus the catalog-id rename that replaces the withdrawn connection-id hiding:

1. -d <name> works everywhere -d <id> does

query, query status, queries, results, context, search show/remove, and databases use passed the flag value raw into X-Database-Id (or a scoped path), so names failed with a server "not found" while databases show <name> resolved fine. New resolve_database_flag resolves catalogs/names to ids; dbid… values pass through with zero extra round trips. Also extracts load's active-database disambiguation into a shared helper and applies it to search create --from <catalog>.…, which previously errored on an ambiguous catalog even when the active database's own catalog matched.

2. workspaces list -o json marks the default workspace

The JSON only carried active — a server-side state flag that is true for every usable workspace and a footgun for scripts reading it as "the current workspace" (all 11 workspaces in the test account report active: true). New default: bool (exactly one true), same source as the table's DEFAULT * marker, which now derives from it.

3. Name catalog ids as catalog in json/yaml output

Replaces the withdrawn connection-id hiding (reverted in 5c8b23d per @zfarrell: "connection ids will become catalog ids which should be exposed"). runtimedb has since removed connections as a concept — the engine is push-only, create_connection rejects every source type but managed, and v53 migrated external catalogs — so a connection id is a catalog id, and the CLI now exposes it under that name: databases show/create/fork -o json/yaml emit default_catalog_id (was default_connection_id) and attachments[].catalog_id (was connection_id). The table view already said catalog id:. Wire protocol and internal names are untouched (the server still sends default_connection_id; ids keep the conn prefix until runtimedb renames them).

Breaking: scripts reading default_connection_id from -o json must switch to default_catalog_id.

Testing

• 450 tests pass (8 new: preferring-active resolution via mockito, id passthrough, workspace default flag + credential-vs-cache resolution, catalog-id field names incl. the serialize-only rename round trip), clippy clean, fmt clean
• Live-verified against production: query/queries/results/context -d littlesis, databases use littlesis, search create --from default.… with active DB set (previously the exact ambiguity error), workspaces list -o json shows exactly one default: true, id paths unchanged, databases show -o json emits default_catalog_id and the table view still prints catalog id:

[zfarrell]

1. Hide internal connection ids from all output

@eddietejeda - don't do this. connection ids will become catalog ids which should be exposed. they just haven't been fully updated yet.

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 65 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/commands/workspace.rs	73.13%	18 Missing ⚠️
src/commands/databases.rs	90.68%	15 Missing ⚠️
src/main.rs	0.00%	9 Missing ⚠️
src/commands/queries.rs	0.00%	6 Missing ⚠️
src/commands/query.rs	0.00%	6 Missing ⚠️
src/commands/results.rs	0.00%	6 Missing ⚠️
src/commands/search.rs	0.00%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude]

nit: builds a second Api that every caller then builds again (not blocking).

Each call site is resolve_database_flag(workspace_id, database) followed by Api::new(Some(workspace_id)), so a name/catalog flag pays two Api::new runs — two config::loads and two ensure_access_token preflights. Taking &Api (and letting callers construct once, then .scoped_to_database_opt(...)) would avoid it and would also let the resolver reuse the caller's already-resolved ambient scope rather than re-deriving it. The Option<&str> → Option<String> shape can stay the same.

[claude]

Reviewed the full diff plus surrounding context in databases.rs, search.rs, workspace.rs, connections.rs, indexes.rs, and main.rs.

The three changes hold up:

• skip_serializing only affects the Serialize path, so the managed load/delete paths (managed_table_load_path), detach's alias→connection lookup, indexes::locate_by_name, and search create's conn_id all still read default_connection_id/connection_id unchanged.
• resolve_database_flag short-circuits on the dbid prefix, so the database-scoped-token path that set explicitly guards is not regressed at the other call sites either. search::run feeds query::execute a loc.database_id, which takes the passthrough branch. Keeping set on plain resolve_database rather than the preferring-active variant is the right call — preferring the active database while changing it would be circular.
• The tables_load extraction is behavior-preserving and slightly better: api.database_id() also honors HOTDATA_DATABASE, which the old inline load_current_database did not, and the active-match branch now reuses the detail response instead of re-fetching by id.
• workspaces list no longer loads config itself, but fetch_workspaces → Api::new(None) still exits on a config error before default_workspace_id runs, so that path is preserved.

Note that databases show/create/fork -o json dropping default_connection_id is a user-visible output change for any script reading it; the conventional-commit prefixes should surface it in the generated changelog.

Three non-blocking comments inline. CI was still queued when this review started, so I have not seen the test or fmt results.

[eddietejeda]

Addressed the three review nits in 1b383ff: unresolved attachments now serialize catalog: null (table view falls back to the raw id as the detach handle), resolve_database_flag takes the caller's &Api instead of building a second one, and the three stale --database help strings now read "id, catalog, or name". 395 tests passing; live-verified -d name/id paths and help text.

[claude]

nit: the comment says the opposite of what the code does (not blocking).

"The Api is built only when there's a non-id flag to resolve — the id fast-path never needs it" isn't what happens: Api::new(Some(workspace_id)) is an argument, so it's evaluated for every Some(d), id or not. resolve_database_flag then returns key.to_string() without touching it. So search show -d dbid… now pays a config::load + ensure_access_token preflight to build an Api that is immediately discarded — and locate_by_name builds its own right after. Before 1b383ff the id path built nothing here, so this is a small regression on the fast path, at the one call site that can't reuse a caller-held Api.

Either make the construction lazy (e.g. expose is_database_id as pub(crate) and guard on it, or have the resolver take impl FnOnce() -> Api), or just drop the claim from the comment.

[claude]

Review

All three nits from the last cycle are addressed in 1b383ff — unresolved attachments now serialize catalog: null with the table view falling back to the raw id, resolve_database_flag takes the caller's &Api, and the three --database help strings read "id, catalog, or name". Not re-raising any of them.

Blocking Issues

1. src/commands/search.rs:332-334 — formatting will fail CI / fmt. The new closure block is a single expression that rustfmt collapses to |d| expr; at 93 columns it fits, so rustfmt emits the break-after-= form instead of what's committed. There is no rustfmt.toml, so max_width is the default 100. CI / fmt had not reported when this review started, so this comes from the formatting rules, not from a log. Suggestion posted inline.

2. Unresolved maintainer objection to commit b14e54a. @zfarrell asked for the connection-id hiding to be dropped — "connection ids will become catalog ids which should be exposed. they just haven't been fully updated yet." That's a direction call on the first of the three changes here, not a code finding of mine, but it is unanswered and it governs roughly half the diff (skip_serializing on default_connection_id, the DatabaseAttachment.catalog field, resolve_attachment_catalogs, and the get display changes). The other two changes stand on their own and could ship without it.

Action Required

• Reformat locate_or_exit per the inline suggestion (or run cargo fmt).
• Settle the direction question on commit 1 with @zfarrell — keep, drop, or split it out.

One non-blocking nit inline on the locate_or_exit comment, which claims an id fast-path the code does not actually take.

I reviewed the full diff as provided; I did not run tests, linters, or the binary.

[eddietejeda]

@zfarrell dropped the connection-id hiding in 5c8b23d — databases show and -o json/yaml output are back to what main does (catalog id line, raw attachment ids, default_connection_id in JSON). PR is now just the -d <name> resolution and the workspaces-list default flag; title/description updated to match.

[claude]

Review

Blocking Issues

• src/commands/workspace.rs:55 — default_workspace_id uses config.workspaces.first(). Commands resolve their workspace through credentials::default_workspace_id (src/client/credentials.rs:75) instead. The two disagree for --api-key and HOTDATA_API_KEY credentials. A database token scoped to one workspace makes workspaces list -o json mark zero workspaces default: true, so the new field breaks the invariant it promises. See the inline comment for the full scenario and the fix.

Action Required

• Load the profile and call crate::client::credentials::default_workspace_id(&profile), keeping the HOTDATA_WORKSPACE check first. src/commands/auth.rs already calls that helper.
• Add a test covering an ApiKeySource::Env profile whose config cache front is not the workspace the credential authorizes.

Note (not blocking)

Commits b14e54a and 5c8b23d cancel each other. cliff.toml parses each commit and gives ^revert its own group (cliff.toml:83), so the release changelog gets a "Hide internal connection ids" line and a "Keep connection ids" line for a net no-op. Squash both away at merge. The PR title also still claims the connection ids are hidden.

Items 2 and 3 of the prior review are addressed. The revert of item 1 is clean: no references to resolve_attachment_catalogs or the attachment catalog field remain.

[eddietejeda]

Addressed the review blocker in 008643d: the default marker now resolves through credentials::default_workspace_id — the same helper resolve_workspace uses — instead of config.workspaces.first(), with HOTDATA_WORKSPACE still taking priority. Added the requested test: an ApiKeySource::Env profile whose config cache front isn't a workspace the credential authorizes marks the credential's own workspace. On the changelog note: this repo squash-merges, so b14e54a/5c8b23d collapse away at merge; title and description are already updated to the two-change scope.

[claude]

Prior blocking issue is resolved. The default marker now resolves through credentials::default_workspace_id, matching resolve_workspace in src/main.rs. The new test covers the env-credential case. No new blocking issues.

Note: CI / test had not reported when this review ran, so test results are unverified here.

[eddietejeda]

@zfarrell following up on your note: runtimedb's connections removal has landed (push-only engine, managed is the only creatable source type, v53 migrated external catalogs), so c92464b now does the rename you pointed at — the ids stay exposed, and -o json/yaml name them default_catalog_id / attachments[].catalog_id instead of *connection_id. Table view already said catalog id:. Wire protocol untouched (still default_connection_id from the server, conn-prefixed ids), so nothing breaks when runtimedb renames its own fields later. Flagged as breaking for scripts reading default_connection_id from -o json.

[claude]

Prior blocking issue on the workspace default marker is resolved in 008643d. The serde renames in c92464b are serialize-only: Database and DatabaseAttachment derive no Deserialize, and wire parsing goes through DatabaseDetailResponse. Table output and JSON output now use the same catalog-id name.

CI / test was still in progress at review time, so this approval cites no test result.

[claude]

Approved. The prior blocking finding is fixed: workspace.rs now resolves the default marker through crate::client::credentials::default_workspace_id, with HOTDATA_WORKSPACE checked first, and default_follows_the_credential_not_the_config_cache_front covers the credential-vs-cache case.

Checked the catalog-id rename for a deserialization hazard. Database and DatabaseAttachment derive Serialize only, so the plain #[serde(rename)] on those two cannot break wire parsing. CreateDatabaseResponse derives both and correctly uses rename(serialize = ...). No repo file reads default_connection_id back out of CLI JSON.

CI has not reported fmt or test yet at review time, so this approval does not assert those checks pass.

[eddietejeda]

Live e2e after the conflict-resolution merge

All against production (throwaway databases, cleaned up, config restored):

Case	Result
databases use <name>	✅ resolves to id, echoes it
query -d <name> (no active db)	✅ runs; -d <id> passes through
databases queries list / <run-id> -d <name>	✅
databases results list -d <name>	✅
databases context list -d <name>	✅
-d <shared catalog> with a fork active	✅ resolves to the active fork (was: ambiguous error); the run lands in its history
databases <id> -o json	✅ emits default_catalog_id, no default_connection_id
workspaces list -o json	✅ exactly one default: true among 12 workspaces, all of which report active: true
-d no-such-db	✅ clean error, exit 1

A finding that validates this PR's design choice: the default marker names workm4lz… — the credential-pinned workspace — while auth status displays the config-cache front (workbpclyz…/AgentRyan). Creates/uses really do land in workm4lz… (verified via databases list -w and the config's current_databases key), so the marker is telling the truth and auth status is the surface still showing the stale front. Follow-up candidate: point auth status's workspace line at the same credentials::default_workspace_id helper.

Also: 410 unit + integration tests pass on the merged head, clippy zero warnings, fmt clean.