fix: address Bugbot review on #49 (output-json early-failure JSON + rm partial-teardown msg)#51
Merged
Merged
Conversation
- dataset push --output-json: emit a JSON error object on early-failure paths (validation, discovery, staging, token, port-forward), not just on dry-run/post-submit. runDatasetPush now uses a named return + a jsonEmitted flag + a defer, so '… --output-json | jq' always gets JSON instead of empty stdout on a failure. Adds Error/ExitCode to the result shape + writePushErrorJSON; covered by TestRunDatasetPush_OutputJSONEarlyFailureEmitsJSON. - dataset rm: on a partial teardown (DROP TABLE succeeded but the PVC file removal failed), the error now says so and points the user to re-run — both teardown ops are idempotent (DROP TABLE IF EXISTS / rm -rf), so a re-run completes cleanup rather than reporting a flat 'teardown failed'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
👋 Heads-up — Code review queue is at 16 / 8 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
saadqbal
approved these changes
Jun 4, 2026
aptracebloc
added a commit
that referenced
this pull request
Jun 4, 2026
* feat(#26): installer-style terminal UX package (internal/ui) (#32) * feat(#26): add internal/ui installer-style terminal UX package Printer renders colored step/✔/⚠/· output matching the tracebloc/client installer (scripts/lib/common.sh). TTY+NO_COLOR auto-detect with a WithColor override (functional-options); fatih/color forced per-instance so it's testable on a buffer. Helpers: Banner, Step, Successf, Warnf, Infof, Errorf, Hintf, PromptHeader. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#26): render dataset push pre-flight via internal/ui + add --plain Adds a persistent --plain root flag and a printerFor(cmd) helper, plus ui.Section/ui.Field. dataset push pre-flight now renders through the ui.Printer (colored Section/Field rows, ⚠ for the RWO warning) instead of ad-hoc fmt.Fprintf; behavior unchanged (the pinned RendersKeyFacts test still passes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#26): render cluster info via internal/ui cluster info now prints through the ui.Printer (Section/Field + a green ✔ ready line) instead of raw fmt.Fprintf, matching the dataset push pre-flight. Test updated for the *ui.Printer signature (still asserts exit-3 on a bad kubeconfig). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#27): installer-style ingestion summary via internal/ui (#33) Replaces the box-drawing RenderPanel with RenderSummary(p *ui.Printer, s): an outcome-colored headline (green clean / yellow skips / red failures) + Section/Field counts + a 'what's next' block. Dropping the Unicode box resolves the v0.2 plain-ASCII TODO. submit.Options gains Printer (nil → ui.New(Out)); dataset push threads its --plain-aware printer through. Parser untouched. RenderPanel tests → RenderSummary (basic shape, nil, table-driven outcome). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#28): interactive guided dataset push — engine + core prompts (PR-a) (#34) On a TTY (unless --no-input), dataset push now prompts for any missing core inputs (path, category, table, intent, label) before validation; flags already passed win. Introduces a prompter interface (survey-backed in prod, fake in tests) so the prompt-mapping is unit-testable without a pseudo-terminal. Args ExactArgs(1)->MaximumNArgs(1) so a bare command can prompt for the path; explicit path-required error otherwise. MLM skips the label prompt; the table prompt reuses push.ValidateTableName. PR-b adds category-specific prompts + a confirm screen. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#28): category-specific prompts + confirm screen (PR-b) (#35) Completes the guided dataset push. After the core prompts, asks only what each category needs: image resolution (blank=auto-detect), required keypoint count, tabular schema (blank=infer) + regression label-policy + time-column. Then a Review screen + 'Proceed?' confirm — shown only when something was actually prompted, so full-flag pushes aren't nagged. Declining or Ctrl-C (survey terminal.InterruptErr, translated to errInteractiveCancelled at the prompter boundary) exits cleanly (0). Prompter interface gains Confirm; fake-prompter tests cover keypoint, tabular-regression, and cancel. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#29): branded home screen + cluster info banner (PR-a) (#36) Bare 'tracebloc' (no subcommand) now renders a branded home screen — ui.Banner + a 'get started' command list — via root.RunE, instead of cobra's raw usage dump; subcommands and --help are unaffected. cluster info gains a Banner header. PR-a of #29; --output-json follows in PR-b (it needs runDatasetPush to return a result struct + JSON tags on Summary, so it's its own change). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#29): --output-json for dataset push (PR-b) (#38) * feat(#29): --output-json for dataset push (PR-b) --output-json emits a machine-readable JSON result on stdout and routes all human output (preflight, logs, summary) to stderr, so 'push ... --output-json | jq' is clean. Implies non-interactive. JSON is emitted at the dry-run stop (status=dry-run) and after submit (status=succeeded/failed/detached/unknown + summary counts). The wire shape (pushJSONResult/pushJSONSummary) lives in the CLI layer so submit.Summary stays json-tag-free. Adds printerForWriter for the stderr routing; unit-tested via TestWritePushJSON. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#29): align --output-json status with exit code + emit JSON on errors classifyPushOutcome maps submit.Run's result to a (status, exitError) pair in lockstep: JobOutcomeSucceeded + summary.HasFailures() now yields status "completed_with_failures" + exit 9 (was "succeeded" while exiting 9). --output-json now emits exactly one result object on EVERY path — including auth/submit/watch errors (with job namespace+name when available) — instead of nothing on error. Adds TestClassifyPushOutcome. Addresses Bugbot on #38. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#37): expand ~ in dataset path (interactive prompt + literal arg) (#41) A leading ~ / ~/… in the dataset path now resolves under $HOME. The shell only expands an *unquoted* ~ on the command line; a path typed at the interactive prompt (#28) or a quoted/literal ~ positional arg reaches the CLI literally, and filepath.Abs would just prepend the CWD (.../cwd/~/...). Adds expandHome (mirrors cluster.expandPath) and applies it in runDatasetPush right after a.LocalPath is resolved — before any push.Discover* call — so both entry points are covered in one place. Relative/absolute/empty paths pass through untouched. Deferred the optional in-prompt existence validator: it would couple the prompt-mapping unit tests to real directories on disk. Filed nothing — noted in the PR. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#30): dataset rm — in-cluster teardown (table + PVC) (#40) Adds 'tracebloc dataset rm <table>': the in-cluster teardown of a pushed dataset, packaging the manual kubectl-exec cleanup. Discovers the cluster (parent release + shared PVC), shows the teardown plan, confirms (reuses the #28 prompter; --yes to skip, --dry-run to preview), then DROPs the MySQL table (exec into the mysql pod, using its own $MYSQL_ROOT_PASSWORD so no credential transits the CLI) and rm -rf's the PVC dirs (exec into the jobs-manager pod). Exit codes mirror push (2 bad name, 3 kubeconfig/refused, 4 no release/PVC, 7 teardown failed). Does NOT remove the central backend catalog entry — the CLI has no direct line to that backend; tracked as the cross-repo follow-up #39. The exec-into-pods mechanism (CLI-direct) vs a server-side jobs-manager delete endpoint is the open architecture question — see the DESIGN NOTE on push.Teardown. Hence: draft, pending team-lead review. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(release): multi-arch v0.1.0 — add linux/386, linux/arm, windows/arm64 (#43) Widen the release build matrix from 5 to 8 targets for the first stable v0.1.0: add linux/386, linux/arm (GOARM=6, covers armv6 + armv7), and windows/arm64. macOS amd64/arm64 and linux/windows amd64/arm64 unchanged. - install.sh detect_arch: learn i386|i686 -> 386 and armv6l|armv7l|armv8l|armhf -> arm (else auto-install breaks on them). - install.ps1 Get-Arch: return arm64 (was a hard fail) and prefer PROCESSOR_ARCHITEW6432 so x64-emulated PowerShell on a Windows-ARM host still picks the native arm64 build. - README + RELEASE_CHECKLIST: reflect the 8-platform stable release; the releases/latest/download/install.sh one-liner now resolves (a plain v0.1.0 tag is non-prerelease per release.yml's gate, so it is `latest`). Verified: all 8 targets cross-compile (CGO_ENABLED=0) to the right arch; release.yml YAML + install.ps1 parse clean. No Go source changed. Refs #42 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * feat(#31): friendlier dataset push — prose intro, guided steps, per-prompt examples (#44) * feat(#31): friendlier dataset push — prose intro + guided 4-step flow Replaces the terse bullet header with a plain-English explainer (grounded in docs.tracebloc.io: 'your workspace', 'the cluster your workspace was installed on', 'your data stays on that cluster'), via a new ui.Printer.Para for prose. Restructures the push into a clearly-numbered, narrated 4-step flow — Check your dataset / Connect to your cluster / Stage your files / Run the ingestion — each with a one-line how/why for medium-technical users (steps 1-2 visible in --dry-run, 3-4 live). Splits printPushPreflight into printLocalSummary + printClusterSummary so each sits under its step; the dry-run stop now names the live-only steps it skipped. dataset rm: drop the backend-catalog clause from the runtime warning (now just 'Destructive and cannot be undone.'); --help NOTE + #39 tracking unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(#31): per-prompt guidance + examples in interactive push Each interactive input prompt now shows a visible hint with an example above it — previously this guidance was hidden behind survey's '?' key. Covers the core prompts (path, table, intent, label column) and the category-specific ones (keypoint count, image resolution, tabular schema, regression label-policy, time column). Rendered via a new ui.PromptHint: a leading blank line for separation + cyan text, so per-field guidance stands out and reads distinctly from the dim generic hints (e.g. the 'Press Enter' meta-line). Verified by TestRunInteractive_ShowsExampleHints, which drives runInteractive with a buffer-backed Printer — no TTY needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(build): compile-check all 8 release targets in build.yml (#46) release.yml builds 8 targets but build.yml (PR CI) only compiled 5, so a break in linux/386, linux/arm, or windows/arm64 wouldn't surface until a release tag. Bring build.yml's matrix in lock-step: add linux/386, linux/arm (GOARM=6), windows/arm64. Compile-only (the native linux/amd64 smoke step is unchanged). Closes #45 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * feat(#31): add dataset rm to home screen + in-run guidance (#47) The bare-'tracebloc' home screen now lists 'dataset rm' alongside push/cluster-info/ingest-validate. The rm command itself gains the same guidance treatment as push: a plain-English Para intro after the banner (what it removes — table + files — and that it can't be undone), and a cyan PromptHint above the confirmation. Home-screen test asserts 'dataset rm' appears. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: prep README for v0.2.0 (#50) - Status → v0.2.0 (latest stable; builds on v0.1.0 with the friendlier guided dataset push + dataset rm on the home screen). - Reconcile the Roadmap: "Next (v0.2)" framed v0.2 as the cloud-sources milestone, but v0.2.0 ships UX polish — so cloud sources / segmentation / `list` move to a label-free "Next", and `rm` drops from the future verbs (it shipped in v0.1.0). - Phase-5 table link pinned to the v0.1.0 tag (text said v0.1.0 but linked to /latest, which now resolves to v0.2.0). Refs #48 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix: address Bugbot review on #49 (#51) - dataset push --output-json: emit a JSON error object on early-failure paths (validation, discovery, staging, token, port-forward), not just on dry-run/post-submit. runDatasetPush now uses a named return + a jsonEmitted flag + a defer, so '… --output-json | jq' always gets JSON instead of empty stdout on a failure. Adds Error/ExitCode to the result shape + writePushErrorJSON; covered by TestRunDatasetPush_OutputJSONEarlyFailureEmitsJSON. - dataset rm: on a partial teardown (DROP TABLE succeeded but the PVC file removal failed), the error now says so and points the user to re-run — both teardown ops are idempotent (DROP TABLE IF EXISTS / rm -rf), so a re-run completes cleanup rather than reporting a flat 'teardown failed'. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Asad Iqbal (Saadi) <asad.dsoft@gmail.com>
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.
Addresses the two Bugbot findings on the develop→main sync (#49). Per our sync-PR convention, the fixes land on
develophere; #49 (head = develop) picks them up automatically.Fixes
dataset.go, feat(#29): --output-json for dataset push (PR-b) #38 code) —--output-jsononly emitted JSON at the dry-run + post-submit points; the ~18 earlierexitErrorreturns left stdout empty, breaking… --output-json | jq.runDatasetPushnow uses a named return +jsonEmittedflag + adeferthat emits a{"status":"error",…,"exit_code":N}object on any early-failure path. AddsError/ExitCodeto the result shape +writePushErrorJSON; unit-tested.teardown.go, feat(#30): dataset rm — in-cluster teardown (table + PVC) [DRAFT] #40 code) — on a partial teardown (table dropped, file removal failed), the error now names the partial state and tells the user to re-run. Both ops are idempotent (DROP TABLE IF EXISTS/rm -rf), so a re-run completes the cleanup.Verification
make cigreen; newTestRunDatasetPush_OutputJSONEarlyFailureEmitsJSON.dataset push … --output-jsonwith a bad table now prints a JSON error on stdout (was empty) and exits 2.🤖 Generated with Claude Code
Note
Low Risk
CLI-only output and error-message changes; no auth or data-path logic changes beyond surfacing partial teardown state.
Overview
dataset push --output-jsonnow always writes a JSON object to stdout, including failures before dry-run or post-submit (validation, discovery, staging, auth, etc.).runDatasetPushuses a named error return, ajsonEmittedflag, and a defer that callswritePushErrorJSONwithstatus: "error",error, andexit_code. Success paths setjsonEmittedafter existing emits.pushJSONResultgains optionalerrorandexit_codefields and documents statuserror.dataset rmtreats partial teardown explicitly: ifTeardowndropped the MySQL table but PVC file removal failed, the CLI returns exit 7 with a message that the table is gone and the user should re-runtracebloc dataset rmto finish file cleanup.Adds
TestRunDatasetPush_OutputJSONEarlyFailureEmitsJSON(invalid table → exit 2 + JSON on stdout).Reviewed by Cursor Bugbot for commit b873bd0. Bugbot is set up for automated code reviews on this repo. Configure here.