feat: scope all commands to active database; remove connections command#221
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if connection_id.is_none() | ||
| && crate::config::load_current_database("default", &workspace_id) | ||
| .is_some() | ||
| { | ||
| databases::tables_list( | ||
| &workspace_id, | ||
| None, | ||
| schema.as_deref(), | ||
| &output, | ||
| ) |
There was a problem hiding this comment.
When an active database is set (and no --connection-id), this branch routes to databases::tables_list(workspace_id, None, schema, output), which silently drops the --table, --limit, and --cursor flags the user may have passed. tables list --table users (or --limit 5, or --cursor <c>) will now return all tables in the active database with no error or warning — a misleading result, since the user explicitly asked to filter/paginate.
Two reasonable fixes:
- Fall through to
tables::listwhenever any oftable,limit, orcursoris provided, e.g. gate the new branch onconnection_id.is_none() && table.is_none() && limit.is_none() && cursor.is_none() && <active db set>; or - Emit a warning to stderr that these flags are ignored in active-database mode.
Silently ignoring explicit user input here is a correctness concern.
There was a problem hiding this comment.
Review
Blocking Issues
src/main.rs(lines 493-502): The new active-database branch callsdatabases::tables_list, which does not accept the--table,--limit, or--cursorflags. When an active database is set and none of these are consumed, the flags are silently ignored, sotables list --table usersreturns all tables instead of the filtered subset - a misleading result with no error or warning.
Action Required
- Either gate the new branch so it is only taken when
table,limit, andcursorare allNone(falling through totables::listotherwise), or warn on stderr that these flags are ignored in active-database mode.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| databases::tables_list( | ||
| &workspace_id, | ||
| None, | ||
| schema.as_deref(), | ||
| &output, | ||
| ) |
There was a problem hiding this comment.
Blocking (from cycle 1, still unaddressed): this active-database branch calls databases::tables_list(workspace_id, None, schema, output), whose signature only accepts (workspace_id, database, schema, format). The user's --table, --limit, and --cursor flags are structurally dropped, so tables list --table users (or --limit 5 / --cursor <c>) silently returns all tables in the active database with no error or warning — a misleading result when the user explicitly asked to filter/paginate.
The branch condition still only checks connection_id.is_none() && <active db set>; it does not account for these flags.
Suggested fix — gate the branch so it only fires when none of these flags are set, otherwise fall through to tables::list:
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) | |
| if connection_id.is_none() | |
| && table.is_none() | |
| && limit.is_none() | |
| && cursor.is_none() | |
| && crate::config::load_current_database("default", &workspace_id) | |
| .is_some() | |
| { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) |
(Alternatively, emit a stderr warning that these flags are ignored in active-database mode.)
There was a problem hiding this comment.
Review — Blocking
src/main.rs (lines 416-421): The active-database branch calls databases::tables_list, which only accepts (workspace_id, database, schema, format). The user's --table, --limit, and --cursor flags are silently dropped, so 'tables list --table users' returns all tables in the active database with no error or warning. This is the same blocking issue raised in the prior review; the code has not changed.
Action required: Gate the new branch on connection_id.is_none() together with table.is_none(), limit.is_none(), and cursor.is_none() (falling through to tables::list otherwise), or emit a stderr warning that these flags are ignored in active-database mode.
| if crate::config::load_current_database("default", &workspace_id).is_some() { | ||
| databases::tables_list( | ||
| &workspace_id, | ||
| None, | ||
| schema.as_deref(), | ||
| &output, | ||
| ) |
There was a problem hiding this comment.
Blocking (from cycles 1 and 2, still unaddressed): this active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), whose signature is only (workspace_id, database, schema, format). The destructured table, limit, and cursor flags are structurally dropped here — they're only used in the else branch. So with an active database set, tables list --table users (or --limit 5 / --cursor <c>) silently returns all tables in the active database, ignoring the filter/pagination the user explicitly requested.
The branch condition is still just load_current_database(...).is_some(); it doesn't account for these flags.
Suggested fix — only take this branch when none of those flags are set, otherwise fall through to tables::list:
| if crate::config::load_current_database("default", &workspace_id).is_some() { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) | |
| if table.is_none() | |
| && limit.is_none() | |
| && cursor.is_none() | |
| && crate::config::load_current_database("default", &workspace_id) | |
| .is_some() | |
| { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) |
(Alternatively, emit a stderr warning that these flags are ignored in active-database mode.)
There was a problem hiding this comment.
Review — Blocking
src/main.rs:411-417 — The active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), which accepts only (workspace_id, database, schema, format). The --table, --limit, and --cursor flags are silently dropped, so with an active database set tables list --table users (or --limit 5 / --cursor <c>) returns all tables in the active database, ignoring the filter/pagination the user requested. Flagged in cycles 1 and 2, still unaddressed.
Action required: Gate the active-database branch so it only fires when table, limit, and cursor are all unset, otherwise fall through to tables::list (or emit a stderr warning that these flags are ignored). See inline comment for a suggested diff.
| if crate::config::load_current_database("default", &workspace_id).is_some() { | ||
| databases::tables_list( | ||
| &workspace_id, | ||
| None, | ||
| schema.as_deref(), | ||
| &output, | ||
| ) |
There was a problem hiding this comment.
Blocking (from cycles 1, 2, and 3, still unaddressed): this active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), whose signature is only (workspace_id, database, schema, format). The destructured table, limit, and cursor flags are structurally dropped here — they are only used in the else branch. So with an active database set, tables list --table users (or --limit 5 / --cursor <c>) silently returns all tables in the active database, ignoring the filter/pagination the user explicitly requested.
The branch condition is still just load_current_database(...).is_some(); it does not account for these flags.
Only take this branch when none of those flags are set, otherwise fall through to tables::list:
| if crate::config::load_current_database("default", &workspace_id).is_some() { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) | |
| if table.is_none() | |
| && limit.is_none() | |
| && cursor.is_none() | |
| && crate::config::load_current_database("default", &workspace_id) | |
| .is_some() | |
| { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) |
(Alternatively, emit a stderr warning that these flags are ignored in active-database mode.)
There was a problem hiding this comment.
Review
Blocking Issues
src/main.rs:411-417— The active-database branch inTablesCommands::Listcallsdatabases::tables_list(&workspace_id, None, schema.as_deref(), &output), which structurally drops the destructuredtable,limit, andcursorflags. When an active database is set,tables list --table users(or--limit 5/--cursor <c>) silently returns all tables, ignoring the filter/pagination the user requested. Flagged in cycles 1-3, still unaddressed.
Action Required
Gate the active-database branch so it only fires when table, limit, and cursor are all None, falling through to tables::list otherwise (or emit a stderr warning that these flags are ignored in active-database mode). See inline comment for a suggested diff.
| if crate::config::load_current_database("default", &workspace_id).is_some() { | ||
| databases::tables_list( | ||
| &workspace_id, | ||
| None, | ||
| schema.as_deref(), | ||
| &output, | ||
| ) |
There was a problem hiding this comment.
Blocking (from cycles 1–4, still unaddressed): this active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), whose signature is only (workspace_id, database, schema, format). The destructured table, limit, and cursor flags are structurally dropped here — they are only used in the else branch. So with an active database set, tables list --table users (or --limit 5 / --cursor <c>) silently returns all tables in the active database, ignoring the filter/pagination the user explicitly requested.
The branch condition is still just load_current_database(...).is_some(); it does not account for these flags.
Only take this branch when none of those flags are set, otherwise fall through to tables::list:
| if crate::config::load_current_database("default", &workspace_id).is_some() { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) | |
| if table.is_none() | |
| && limit.is_none() | |
| && cursor.is_none() | |
| && crate::config::load_current_database("default", &workspace_id) | |
| .is_some() | |
| { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) |
(Alternatively, emit a stderr warning that these flags are ignored in active-database mode.)
There was a problem hiding this comment.
Review
Blocking Issues
src/main.rs:411-417—tables listactive-database branch drops the--table,--limit, and--cursorflags. When an active database is set,databases::tables_list(&workspace_id, None, schema.as_deref(), &output)is called, and that signature has no place for those flags. As a resulttables list --table users(or--limit 5/--cursor <c>) silently returns all tables in the active database, ignoring the filter/pagination the user explicitly requested. Flagged in cycles 1–4 and still unaddressed.
Action Required
Gate the active-database branch on table.is_none() && limit.is_none() && cursor.is_none() && load_current_database(...).is_some() so it falls through to tables::list when any of those flags are set, or emit a stderr warning that they are ignored in active-database mode.
| cursor.as_deref(), | ||
| &output, | ||
| ) | ||
| if crate::config::load_current_database("default", &workspace_id).is_some() { |
There was a problem hiding this comment.
Blocking (from cycles 1–5, still unaddressed): this active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), whose signature is only (workspace_id, database, schema, format). The destructured table, limit, and cursor flags are structurally dropped here — they are only used in the else branch. So with an active database set, tables list --table users (or --limit 5 / --cursor <c>) silently returns all tables in the active database, ignoring the filter/pagination the user explicitly requested.
The branch condition is still just load_current_database(...).is_some(); it does not account for these flags.
Only take this branch when none of those flags are set, otherwise fall through to tables::list:
| if crate::config::load_current_database("default", &workspace_id).is_some() { | |
| if table.is_none() | |
| && limit.is_none() | |
| && cursor.is_none() | |
| && crate::config::load_current_database("default", &workspace_id) | |
| .is_some() | |
| { | |
| databases::tables_list( | |
| &workspace_id, | |
| None, | |
| schema.as_deref(), | |
| &output, | |
| ) |
(Alternatively, emit a stderr warning that these flags are ignored in active-database mode.)
There was a problem hiding this comment.
Review
Blocking Issues
src/main.rs:411— The active-database branch oftables listgates only onload_current_database(...).is_some()and callsdatabases::tables_list(&workspace_id, None, schema.as_deref(), &output), structurally dropping the destructured--table,--limit, and--cursorflags. With an active database set,tables list --table users(or--limit 5/--cursor <c>) silently returns all tables, ignoring the filter/pagination the user explicitly requested. Flagged in cycles 1–5, still unaddressed.
Action Required
Gate the active-database branch on table.is_none() && limit.is_none() && cursor.is_none() so it only fires when none of those flags are set, otherwise fall through to tables::list (which honors them). Alternatively, emit a stderr warning that these flags are ignored in active-database mode.
| cursor.as_deref(), | ||
| &output, | ||
| ) | ||
| if crate::config::load_current_database("default", &workspace_id).is_some() { |
There was a problem hiding this comment.
Blocking (from cycles 1–6, still unaddressed): this active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), whose signature is only (workspace_id, database, schema, format) (src/commands/databases.rs:1336). The destructured table, limit, and cursor flags are structurally dropped here — they are only used in the else branch. So with an active database set, tables list --table users (or --limit 5 / --cursor <c>) silently returns all tables in the active database, ignoring the filter/pagination the user explicitly requested.
The branch condition is still just load_current_database(...).is_some(); it does not account for these flags.
Only take this branch when none of those flags are set, otherwise fall through to tables::list:
| if crate::config::load_current_database("default", &workspace_id).is_some() { | |
| if table.is_none() | |
| && limit.is_none() | |
| && cursor.is_none() | |
| && crate::config::load_current_database("default", &workspace_id).is_some() | |
| { |
(Alternatively, emit a stderr warning that these flags are ignored in active-database mode.)
There was a problem hiding this comment.
Review
Blocking Issues
- src/main.rs:411 — the tables list active-database branch calls databases::tables_list(&workspace_id, None, schema.as_deref(), &output), which has no parameters for table, limit, or cursor. These flags are structurally dropped, so with an active database set, "tables list --table users" (or --limit 5 / --cursor) silently returns all tables, ignoring the filter/pagination the user requested. Unaddressed since cycle 1.
Action Required
Gate the active-database branch on table.is_none() && limit.is_none() && cursor.is_none() && load_current_database(...).is_some() so these flags fall through to tables::list, or emit a stderr warning that they are ignored in active-database mode.
There was a problem hiding this comment.
Cycle 8: The prior blocking issue (tables list silently dropping --table/--limit/--cursor in active-database mode) is resolved. use_db_path now gates on limit.is_none() && cursor.is_none() (src/main.rs:413-414), and --table is forwarded to databases::tables_list (src/main.rs:422), whose signature was updated to filter accordingly (src/commands/databases.rs:1345,1356). No new blocking issues.
There was a problem hiding this comment.
Blocking issue from cycles 1–7 (tables list dropping --table/--limit/--cursor in active-database mode) is resolved: databases::tables_list now accepts and forwards those flags to collect_tables, and main.rs:418-426 passes them through. No new issues.
There was a problem hiding this comment.
The blocking issue from prior cycles is resolved: databases::tables_list now accepts table, limit, and cursor, and the active-database branch in src/main.rs passes all three through — so tables list --table/--limit/--cursor are no longer silently dropped. This is a cleaner fix than the fall-through I suggested. No remaining blocking issues.
eddietejeda
changed the title
There was a problem hiding this comment.
Cycle 8: The blocking issue tracked since cycle 1 — the active-database tables list branch dropping --table/--limit/--cursor — is resolved. databases::tables_list now accepts and forwards all three flags to collect_tables, and main.rs passes them through. Test coverage is present. No remaining blocking issues.
When a database is active, indexes list automatically narrows to that database connection ID. Falls back to workspace-wide when none is set.
…active DB --table now accepts schema.table (resolves catalog from active database) or the full catalog.schema.table form. Removes the old connection.table syntax. Also updates hotdata-search SKILL.md to match.
…ursor When an active database is set, --table now passes through to the API instead of being silently dropped. --limit and --cursor fall through to tables::list (which supports pagination) rather than the active-db path.
…imit/--cursor P1: Remove stale tests that pattern-matched IndexesCommands::Delete with the old connection_id field; update to match the new catalog-only shape. P2: tables list now always uses the active-database path when a DB is set, even with --limit/--cursor. collect_tables does a single paged fetch when limit/cursor are provided, so pagination works and the TABLE column stays catalog-prefixed.
…w args Fixes failing table-filter test: mockito matcher used 'table_name' but the SDK sends 'table' as the query parameter.
The merge-base changed after approval.
ce14185 to
5707cbc
Compare
There was a problem hiding this comment.
Prior blocking issue (cycles 1–7) is resolved: databases::tables_list now accepts and forwards table/limit/cursor (main.rs:417) to collect_tables, which honors them, with test coverage. No flags are dropped in active-database mode. No new blocking issues.
1 participant
Summary
connectionscommand —hotdata connectionsis gone entirely; the internal resolver is kept as a private helper used byindexesanddatabases attach/detachtables list— scopes to the active database when one is set (hotdata databases set <id>);--table,--schema,--limit, and--cursorall work within that scope; falls back to workspace-wide listing when no database is activetables show <schema.table>— new subcommand; acceptsschema.table(resolves catalog from active database) orcatalog.schema.table; displays columns with data typesresults show <id>— new subcommand; downloads and prints a stored result by IDindexes list— scopes to the active database's connection when one is set; workspace-wide otherwise;--connection-idflag removedindexes delete—--connection-idremoved;--catalogis now the only scope flag (resolves via the same catalog to connection resolver used byindexes create)search --table— changed fromconnection.table/connection.schema.tabletoschema.table(active database) orcatalog.schema.table; 2-part form resolves the catalog from the active database automaticallyBehavior
With an active database set (
hotdata databases set <id>):hotdata tables list— shows only that database's tables, catalog-prefixed (default.public.cpi)hotdata tables list --table foo --limit 10— filter and pagination work within the active databasehotdata tables show public.orders— resolves catalog from active DBhotdata search "query" --table public.orders— resolves catalog from active DBhotdata indexes list— scoped to active DB's connectionWithout an active database: all commands fall back to workspace-wide behavior (same as before).
Test plan
tables listwith no filter,--table,--schema,--limit,--cursor— all scoped to active DBtables show schema.tableandcatalog.schema.table— both resolve correctlyresults show <id>— downloads and prints stored resultindexes create --catalog/list/delete --catalog— full lifecycle, no--connection-idsearch --table schema.tableandcatalog.schema.table— resolves active DB catalog