Skip to content

Thorough review pass#99

Merged
austin-denoble merged 4 commits into
mainfrom
adenoble/thorough-review-pass
Jun 17, 2026
Merged

Thorough review pass#99
austin-denoble merged 4 commits into
mainfrom
adenoble/thorough-review-pass

Conversation

@austin-denoble

@austin-denoble austin-denoble commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

A pre-v1.0 finalization pass over the CLI. A review across code, help text, docs, and release plumbing surfaced a handful of release-quality bugs plus several inconsistencies in the soon-to-be-frozen public surface. This PR fixes the bugs and settles the surface ahead of tagging v1.0.0. Internal-quality follow-ups (test coverage, service-interface refactors) are intentionally deferred to post-tag.

Bug fixes

  • collection list: dimension/vector-count rendered via string(int32), emitting Unicode runes instead of numbers — now formatted with strconv.
  • index configure --json: the flag was read but never registered, so JSON output and FailJSON error paths were unreachable. Flag is now registered.
  • auth configure --project-id <unknown>: an unmatched project id fell through to a nil dereference (panic). Now fails cleanly with a clear message.

Help text & error-message corrections

  • auth local-keys prune: dry-run hint referenced a nonexistent --yes flag.
  • organization delete: stray doubled quote in an example.
  • project update / project describe / organization update / api-key list: "no target set" hints pointed users at the wrong subcommand.
  • index vector query: fixed a verbless required-input error string.

Public-surface finalization

  • Deletion-protection flag: dropped the inconsistent -p/-d shorthands on index configure and index restore (collided with project/dimension shorthands elsewhere); the flag is now long-form-only across create/configure/restore.
  • Default namespace: documented that namespace operations address the default namespace via __default__. Validated against the server contract (namespace ops require __default__ and reject ""; responses already render __default__ at the pinned API version), so no client-side rewriting was needed — help-text only. See pc index vector query: empty --namespace coerced to __default__ #85 for background.
  • Deletion confirmation prompts: added a shared confirm.Deletion helper and routed all delete commands through it — consolidating three duplicated confirmDelete copies (organization/project/api-key) and adding prompts to index, collection, backup, namespace, and vector delete. Each gains a --skip-confirmation flag, and --json also bypasses the prompt. vector delete confirms only for --all-vectors; targeted --ids/--filter deletes proceed silently.
  • --json docs: corrected the AGENTS.md claim — --json is explicit and is not inferred from TTY state (only color is auto-suppressed on a non-TTY); the login/target/auth interactive flows are the documented exception.

Deferred to post-tag

Test-coverage and refactor work that doesn't affect the public surface: vector-ops tests + service-interface refactor, api-key command tests, sdk credential-resolution tests, and whoami/logout de-duplication.

Testing

go build, go vet, go test ./..., and golangci-lint run all pass.


Note

Medium Risk
Adds confirmation gates and changes --json/TTY semantics across many delete commands, which can break non-interactive scripts that relied on silent deletes or implicit JSON; fixes are low-risk but behavior change is broad.

Overview
Pre-v1.0 pass that fixes several user-visible bugs, tightens help/errors, and standardizes destructive deletes and flag behavior.

Bug fixes: collection list now prints numeric dimension/vector counts via strconv instead of bogus Unicode from string(int32). index configure registers --json so JSON success/error paths work. auth configure --project-id with an unknown ID fails with a clear message instead of panicking.

Deletion UX: New shared confirm.Deletion replaces three duplicated confirm helpers and adds interactive prompts (plus --skip-confirmation; --json skips prompts) on index, collection, backup, namespace, and API key/org/project deletes. vector delete only prompts for --all-vectors.

Public surface / docs: --deletion-protection is long-form only on configure/restore (removed -p/-d shorthands). Namespace help documents __default__ for the default namespace. AGENTS.md clarifies --json is explicit (not TTY-inferred), except login/target/auth. Various hint strings and examples corrected; e2e teardowns pass --skip-confirmation on deletes.

Reviewed by Cursor Bugbot for commit cd109e5. Bugbot is set up for automated code reviews on this repo. Configure here.

Address three release-quality bugs plus a set of copy-paste mistakes in
user-facing strings surfaced during the pre-v1.0 review:

- collection list: render Dimension/VectorCount via strconv instead of
  string(int32), which emitted Unicode runes instead of numbers
- index configure: register the missing --json/-j flag so its existing
  JSON output and FailJSON error paths are reachable
- auth configure: nil-check the --project-id lookup so an unmatched ID
  fails cleanly instead of panicking on a nil deref

Help/error string fixes:
- local-keys prune: dry-run hint referenced a nonexistent --yes flag;
  corrected to re-run without --dry-run (+ optional --skip-confirmation)
- organization delete: remove doubled quote in example
- project update/describe, organization update, api-key list: "no target
  set" hints pointed at the wrong subcommand
- vector query: fix verbless required-input error string
  A2: remove the -p/-d shorthands from index configure and index
  restore deletion-protection flags so the flag is long-form-only and
  consistent across create/configure/restore (no shorthand collisions
  with project/dimension/description elsewhere).

  A3: document that namespace operations address the default namespace
  via __default__. Per the server contract (pc-commons namespace_util.rs,
  query-router service), namespace describe/delete require __default__
  and reject the empty string, while responses already render the default
  as __default__ at the pinned API version (2025-10). No CLI-side
  namespace rewriting is needed, so this is help-text only: add a
  __default__ example and flag hint to namespace describe and
  namespace delete.
A4: add interactive confirmation to destructive delete commands that
lacked it. Introduce a shared confirm.Deletion helper
(internal/pkg/utils/confirm) and route all delete commands through it,
replacing the three duplicated confirmDelete copies (organization,
project, api-key) and adding prompts to index delete, collection
delete, backup delete, namespace delete, and vector delete. Each gains
a --skip-confirmation flag, and --json also bypasses the prompt.
vector delete only confirms for --all-vectors; targeted --ids/--filter
deletes proceed without a prompt.

A1: correct the AGENTS.md --json description. The flag is explicit and
is not inferred from TTY state (only color is auto-suppressed on a
non-TTY); the login/target/auth interactive flows are the documented
exception.
@austin-denoble austin-denoble changed the title Adenoble/thorough review pass Thorough review pass Jun 17, 2026
@austin-denoble austin-denoble merged commit 35b3921 into main Jun 17, 2026
9 checks passed
@austin-denoble austin-denoble deleted the adenoble/thorough-review-pass branch June 17, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant