feat(db,measurement): db admin + measurement listing (PR3)#2
Merged
Conversation
PR3 of the phased arcctl build-out. Adds `arcctl db {list,show,
create,drop}` and `arcctl measurement list` subcommands plus the
matching internal/client/database.go.
Wire surface (verified against arc/internal/api/databases.go):
- GET /api/v1/databases (any-valid-token)
- POST /api/v1/databases (any-valid-token) *see #471
- GET /api/v1/databases/:name (any-valid-token)
- GET /api/v1/databases/:name/measurements (any-valid-token)
- DELETE /api/v1/databases/:name?confirm=true (admin + delete.enabled=true)
Highlights
- client/database.go: 5 typed wrappers with bounded response reads,
consistent error decoding (re-uses PR2's decodeWriteError), URL
escaping on :name path segments
- client.go: new setCrossDBHeaders() so cross-database endpoints
never accidentally inherit the client's default x-arc-database
header — the "absence is explicit" pattern
- db drop: client-side y/N prompt (per CLAUDE.md destructive-ops
rule), --yes / -y to skip. Surfaces the server's verbatim error
on 403 (e.g. "Set delete.enabled=true in arc.toml to enable")
- All list/show output formats (table / json / csv) sort by name
so the three formats agree on row order; server-reported Count
is passed through verbatim (not re-derived from len(slice))
- measurement list falls back to active connection's
default_database when --database omitted; client-side error
before any network call if neither set
- Three flag helpers (addCommonConnectionFlags, addTimeoutFlag,
confirmDestructive) so future PRs don't drift on flag wording
Verification
- 13 client tests (httptest): list/get/create/delete + cross-db
header pinning + URL escaping + delete-disabled-error +
not-admin-error + reserved-name-rejection
- 12 command tests: render dispatch (table/json/csv), empty-result
paths, sort stability, JSON round-trip, CSV column order,
confirmDestructive table-driven cases (y/yes/empty/n/NO/EOF/
whitespace)
- End-to-end smoke against `arc serve` (with delete.enabled=true
AND =false): every command + every error path verified. The
new drop prompt smoked through 4 cases (empty=abort, n=abort,
y=delete, --yes=no prompt)
Internal review (matrix + single deep reviewer) addressed:
- B1 Blocker: db drop missing y/N prompt + --yes flag → fixed
(matches CLAUDE.md destructive-ops rule, mirrors config delete)
- H1: db show JSON Count now mirrors server's list.Count (not
re-derived from len(measurements)) — safer against future
server-side pagination
- M2 + M3: JSON output now sorts before encoding, so table/json/
csv agree on row order across all list commands
- S2: new test pinning the setCommonHeaders default-fallback
semantic so accidental cross-db drift gets caught
- S3 + S4: README roadmap flipped, addTimeoutFlag factors out 5x
duplicated --timeout flag definition
- M1 deferred: db show partial-failure UX (fail-loud is defensible
per matrix; will revisit if real-world friction surfaces)
Related Arc issue
- #471 — POST /api/v1/databases accepts any-valid-token; should
require admin. Discovered while building this PR; filed separately.
Companion docs PR lands in docs.basekick.net (docs/cli/{db,
measurement}.md + index.md status table update).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Member
Author
|
@gemini-code-assist review please |
Merged
3 tasks
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements database and measurement administration features for arcctl (PR3), adding the db and measurement subcommands. It introduces client methods for listing, showing, creating, and deleting databases, as well as listing measurements, along with comprehensive unit tests. The feedback suggests a UX improvement in the db drop command to validate the client configuration before prompting the user for destructive confirmation, preventing unnecessary prompts if the configuration is invalid.
There was a problem hiding this comment.
Code Review
This pull request implements the db and measurement subcommands for the arcctl CLI, mapping to Arc's database administration and measurement listing endpoints. It introduces the necessary client methods, command-line interfaces, and comprehensive unit tests. The review feedback highlights a potential issue where empty or nil slices for databases and measurements could serialize to null instead of [] in JSON outputs, and suggests using make and copy to ensure consistent JSON serialization.
added 2 commits
May 31, 2026 20:55
Move the buildClient call ahead of the y/N confirmation in `db drop` so a misconfigured connection fails fast instead of making the user confirm a delete that can't be sent. No functional change beyond ordering — same code, two blocks swapped + a four-line "why" comment. Found by Gemini in #2 inline comment on internal/commands/db.go:240. The original matrix + deep-reviewer pass assumed a valid client config and never exercised the prompt-then-fail UX path. Tests + lint clean; existing confirmDestructive tests still pass since they exercise the prompt mechanics in isolation, not the wrapping order.
#2) Switch from `append([]T(nil), src...)` to `make([]T, len(src)); copy()` in the three render-paths that emit JSON. The append-to-nil form preserves nil when src is empty/nil; the make form always yields a non-nil slice. The difference is invisible for table/csv output but matters for `-o json` consumers (jq, frontends, JSON-schema validators with `minItems: 0`) that distinguish `"databases": null` from `"databases": []`. Fixed sites - internal/commands/db.go:323-329 renderDatabaseList - internal/commands/db.go:385-389 renderDatabaseShow - internal/commands/measurement.go:95-101 renderMeasurementList Regression tests - TestRenderDatabaseList_JSONEmpty_IsArrayNotNull - TestRenderDatabaseShow_JSONEmpty_IsArrayNotNull - TestRenderMeasurementList_JSONEmpty_IsArrayNotNull Tests verified via mutation: reverting one site to append-to-nil makes the corresponding test fail with the exact `"databases": null` output Gemini flagged; reapplying the fix passes. Found by Gemini in #2 on commit a77d514 (three inline comments at db.go:326, db.go:382, measurement.go:98). The earlier matrix exercised empty-result paths for table/csv output but didn't check the JSON shape — adding a "nil-vs-empty slice JSON encoding" axis to the destructive-ops matrix template for future PRs.
Member
Author
|
@gemini-code-assist review please |
There was a problem hiding this comment.
Code Review
This pull request implements the database and measurement administration subcommands (db and measurement) for the arcctl CLI tool, corresponding to PR3 of the development roadmap. It introduces new client methods to interact with the Arc database endpoints (list, show, create, drop, and list measurements) and wires them up to the CLI command tree with support for table, JSON, and CSV output formats. Comprehensive unit tests have been added to verify the client behavior, command execution, and output rendering. No review comments were provided, so there is no additional feedback to address.
xe-nvdk
pushed a commit
that referenced
this pull request
Jun 1, 2026
) Two findings from Gemini's review on #3, both Medium. #1 — `--skip-rows` accepts negative values silently A negative value passes through to `ImportCSV`, then gets dropped by the `opts.SkipRows > 0` gate in the client. User typed `--skip-rows -5`, nothing happens, no error. Now validated in the command's RunE before any client call. Regression test driven through cobra so the wired flag path is covered end-to-end; mutation-verified by disabling the guard (downstream "no database" check fires instead). #2 — `mw.Close()` on error paths writes trailing boundary into broken pipe In the multipart goroutine, the error paths used to call `mw.Close()` *before* `pw.CloseWithError(err)`. `multipart.Writer.Close()` writes the trailing boundary `--<boundary>--\r\n` into the pipe — but the preceding content is already known-bad (CreateFormFile failed, or io.Copy errored mid-file). Result: a multipart payload that's syntactically valid but semantically truncated, confusing for a reader attempting to make sense of partial data. Now: error paths skip `mw.Close()` entirely and signal the error on the pipe; happy path still closes the writer normally to emit the trailing boundary.
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
1 participant
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.
Summary
arcctl db {list,show,create,drop}andarcctl measurement list.internal/client/database.goagainst Arc's/api/v1/databasessurface.setCrossDBHeaderson the client so cross-database endpoints never accidentally inherit the per-clientx-arc-databasedefault — "absence is explicit."db dropfollows the project-wide destructive-ops rule: client-sidey/Nprompt by default,--yesto skip; on server refusal the verbatim error surfaces (e.g. the "Setdelete.enabled=trueinarc.toml" hint).table,json,csv) sort by name so the three formats agree on row order. Server-reportedcountis passed through verbatim rather than re-derived fromlen(slice).addCommonConnectionFlags,addTimeoutFlag,confirmDestructive) so future PRs don't drift on flag wording.Test plan
go test -race -count=1 ./...— 25 new tests acrossinternal/client/andinternal/commands/gofmt -l . && go vet ./...cleanarc serve(with bothdelete.enabled=trueand=false):db listtable / json / csv all render and sort correctlydb create+db show+measurement listhappy pathsdb dropinteractive: empty answer aborts;naborts;ydeletes;--yesskips promptdb dropwithdelete.enabled=false→ server's "Set delete.enabled=true in arc.toml to enable" surfaces verbatimmeasurement listfalls back todefault_database; clean client-side error when neither is set(no databases),(no measurements yet),(no measurements in database "X")Internal review
Matrix + single deep reviewer per
.claude/CLAUDE.md. Findings addressed pre-commit:db dropwas missing they/Nprompt +--yesflag. Fixed: mirrorsconfig delete's pattern, reads fromcmd.InOrStdin()(testable), table-driven tests covery/YES/n/NO/empty/EOF/whitespace cases.db showJSONCountnow mirrors the server'slist.Countrather thanlen(measurements). Safer against future server-side pagination.setCommonHeadersdefault-fallback semantic so accidental cross-DB drift gets caught.addTimeoutFlagfactors out 5× duplicated--timeoutflag definitions.db showpartial-failure UX (fail-loud is defensible per matrix; revisit if real-world friction surfaces).Related Arc issue
Discovered while building this PR: Basekick-Labs/arc#471 —
POST /api/v1/databasesaccepts any-valid-token; should require admin. Filed separately; arcctl works against current server behavior either way.Companion docs PR
Per the per-PR docs rule, docs land in docs.basekick.net at
docs/cli/db.md+docs/cli/measurement.md+ thecli/index.mdstatus table.🤖 Generated with Claude Code