Uh oh!
There was an error while loading. Please reload this page.
refactor(cli): split data.go into per-concern ingest files (mechanical) - #303
Merged
Conversation
Pure moves of whole top-level declarations out of the 1539-line data.go, same package cli, zero behavior change: - data_ingest_cmd.go — newDataIngestCmd (flag surface incl. hidden deprecated aliases) + runDataIngestArgs - data_ingest_local.go — sortedKeys / expandHome / statDatasetPath + printLocalSummary + runLocalPreflight - data_ingest_cluster.go — runIngestionRun + shouldReclaimStaging + printClusterSummary + test seams + destTableExists + existingTableAction - data_ingest_output.go — pushJSONResult/pushJSONSummary + writePushJSON/writePushErrorJSON + classifyPushOutcome (kept together with writePushJSON — mutation-hardened lockstep) data.go keeps the data group command, the deprecation-alias notice, and runDataIngest itself (extracted separately in cli#283). Only new text is each file's header comment + package/import clauses; every moved section was verified verbatim against the pre-split file. data_test.go is untouched and compiles unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka
commented
Jul 14, 2026
ContributorAuthor
👋 Heads-up — Code review queue is at 41 / 30 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.) |
8 tasks
LukasWodka
commented
Jul 14, 2026
ContributorAuthor
@BugBot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9f75c1d. Configure here.
8 tasks
saadqbal
approved these changes
Jul 14, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
👍 Verified it's a faithful mechanical split — line accounting reconciles exactly (data.go 1539→693, the 846 moved lines land in the 4 per-concern files), retained symbols keep identical signatures, and Test + all builds are green. Nice separation (cmd / local / cluster / output).
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…findings Pinned staticcheck@2025.1.1 joins the standalone lint set in both the Makefile lint target and build.yml's lint job (-checks all,-ST1005), mirroring the errcheck/ineffassign pin pattern. The golangci-lint OOM story that originally disabled it is stale for the standalone binary: a full run is ~12s wall locally. Findings fixed (the full non-ST1005 set): - ST1008: runSet (resources_set_test) returns (string, error), error last - ST1003: CheckMaskIdColumn -> CheckMaskIDColumn (+ call sites/comments) - ST1003: errors_as -> errorsAs, io_eof_or_similar -> ioEOFOrSimilar - ST1020: CoreFloorText doc comment starts with the function name ST1005 stays excluded: it flags ~58 customer-visible error strings that need a wording review, not a mechanical sweep — separate follow-up. Rebased onto develop: union-merged the lint job with develop's govulncheck job (#278), coverage-floor step (#301) and deadcode advisory gate; kept the audit comment #286 added above CheckMaskID*Column. The 'going back to the action' comment in build.yml is KEPT (not deleted) — #6 is still open on develop, so golangci-lint-action stays disabled and that rationale still holds. Detached the four data_ingest_*.go header comments from the package clause (#303 split) so the newly-enabled staticcheck gate is ST1000-clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…monthly) (#304) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…313) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: flip deadcode gate to blocking; delete the 3 dead #127 leftovers The deadcode CI step loses continue-on-error (and the Makefile target its '|| true'): both now run scripts/deadcode-check.sh, which fails on any function unreachable from ./cmd/tracebloc that isn't declared in scripts/deadcode-allowlist.txt. The tool itself always exits 0, so the old advisory step never could have blocked — the gate keys on output. Deleted (verified still dead with the pinned deadcode@v0.48.0): - config.clearAll + its three dedicated tests (TestClearAll, TestClearAll_HomeError, TestClear) — logout uses Save, nothing else ever called it - push.allCategoryIDs — moved verbatim into category_registry_test.go (the registry-pinning tests legitimately iterate every id; the shipped binary shouldn't carry the helper) - submit.isSubmitError — moved into client_test.go as the test-local assertion helper it always was (orphaned 'errors' import dropped) Allowlisted with reasons (the 4 legit findings): Status.String + JobOutcome.String (fmt-reflection Stringers) and ReadLabelValues + inferColumnType (di#349 test-only parity harnesses). Stale allowlist entries warn without failing; line numbers are stripped so edits that shift code don't red the gate. Coverage floors still clear after the test deletions (cli 82.9% >= 68, submit 80.3% >= 72). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…findings Pinned staticcheck@2025.1.1 joins the standalone lint set in both the Makefile lint target and build.yml's lint job (-checks all,-ST1005), mirroring the errcheck/ineffassign pin pattern. The golangci-lint OOM story that originally disabled it is stale for the standalone binary: a full run is ~12s wall locally. Findings fixed (the full non-ST1005 set): - ST1008: runSet (resources_set_test) returns (string, error), error last - ST1003: CheckMaskIdColumn -> CheckMaskIDColumn (+ call sites/comments) - ST1003: errors_as -> errorsAs, io_eof_or_similar -> ioEOFOrSimilar - ST1020: CoreFloorText doc comment starts with the function name ST1005 stays excluded: it flags ~58 customer-visible error strings that need a wording review, not a mechanical sweep — separate follow-up. Rebased onto develop: union-merged the lint job with develop's govulncheck job (#278), coverage-floor step (#301) and deadcode advisory gate; kept the audit comment #286 added above CheckMaskID*Column. The 'going back to the action' comment in build.yml is KEPT (not deleted) — #6 is still open on develop, so golangci-lint-action stays disabled and that rationale still holds. Detached the four data_ingest_*.go header comments (#303 split) plus exitcodes.go's (#284, which develop merged after this branch was cut) from the package clause so the newly-enabled staticcheck gate is ST1000-clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…monthly) (#304) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…313) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: flip deadcode gate to blocking; delete the 3 dead #127 leftovers The deadcode CI step loses continue-on-error (and the Makefile target its '|| true'): both now run scripts/deadcode-check.sh, which fails on any function unreachable from ./cmd/tracebloc that isn't declared in scripts/deadcode-allowlist.txt. The tool itself always exits 0, so the old advisory step never could have blocked — the gate keys on output. Deleted (verified still dead with the pinned deadcode@v0.48.0): - config.clearAll + its three dedicated tests (TestClearAll, TestClearAll_HomeError, TestClear) — logout uses Save, nothing else ever called it - push.allCategoryIDs — moved verbatim into category_registry_test.go (the registry-pinning tests legitimately iterate every id; the shipped binary shouldn't carry the helper) - submit.isSubmitError — moved into client_test.go as the test-local assertion helper it always was (orphaned 'errors' import dropped) Allowlisted with reasons (the 4 legit findings): Status.String + JobOutcome.String (fmt-reflection Stringers) and ReadLabelValues + inferColumnType (di#349 test-only parity harnesses). Stale allowlist entries warn without failing; line numbers are stripped so edits that shift code don't red the gate. Coverage floors still clear after the test deletions (cli 82.9% >= 68, submit 80.3% >= 72). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…findings Pinned staticcheck@2025.1.1 joins the standalone lint set in both the Makefile lint target and build.yml's lint job (-checks all,-ST1005), mirroring the errcheck/ineffassign pin pattern. The golangci-lint OOM story that originally disabled it is stale for the standalone binary: a full run is ~12s wall locally. Findings fixed (the full non-ST1005 set): - ST1008: runSet (resources_set_test) returns (string, error), error last - ST1003: CheckMaskIdColumn -> CheckMaskIDColumn (+ call sites/comments) - ST1003: errors_as -> errorsAs, io_eof_or_similar -> ioEOFOrSimilar - ST1020: CoreFloorText doc comment starts with the function name ST1005 stays excluded: it flags ~58 customer-visible error strings that need a wording review, not a mechanical sweep — separate follow-up. Rebased onto develop: union-merged the lint job with develop's govulncheck job (#278), coverage-floor step (#301) and deadcode advisory gate; kept the audit comment #286 added above CheckMaskID*Column. The 'going back to the action' comment in build.yml is KEPT (not deleted) — #6 is still open on develop, so golangci-lint-action stays disabled and that rationale still holds. Detached the four data_ingest_*.go header comments (#303 split) plus exitcodes.go's (#284, which develop merged after this branch was cut) from the package clause so the newly-enabled staticcheck gate is ST1000-clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…monthly) (#304) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…313) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: flip deadcode gate to blocking; delete the 3 dead #127 leftovers The deadcode CI step loses continue-on-error (and the Makefile target its '|| true'): both now run scripts/deadcode-check.sh, which fails on any function unreachable from ./cmd/tracebloc that isn't declared in scripts/deadcode-allowlist.txt. The tool itself always exits 0, so the old advisory step never could have blocked — the gate keys on output. Deleted (verified still dead with the pinned deadcode@v0.48.0): - config.clearAll + its three dedicated tests (TestClearAll, TestClearAll_HomeError, TestClear) — logout uses Save, nothing else ever called it - push.allCategoryIDs — moved verbatim into category_registry_test.go (the registry-pinning tests legitimately iterate every id; the shipped binary shouldn't carry the helper) - submit.isSubmitError — moved into client_test.go as the test-local assertion helper it always was (orphaned 'errors' import dropped) Allowlisted with reasons (the 4 legit findings): Status.String + JobOutcome.String (fmt-reflection Stringers) and ReadLabelValues + inferColumnType (di#349 test-only parity harnesses). Stale allowlist entries warn without failing; line numbers are stripped so edits that shift code don't red the gate. Coverage floors still clear after the test deletions (cli 82.9% >= 68, submit 80.3% >= 72). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Jul 14, 2026
…findings (#302) * ci: re-enable staticcheck (pinned standalone) + fix the 5 non-ST1005 findings Pinned staticcheck@2025.1.1 joins the standalone lint set in both the Makefile lint target and build.yml's lint job (-checks all,-ST1005), mirroring the errcheck/ineffassign pin pattern. The golangci-lint OOM story that originally disabled it is stale for the standalone binary: a full run is ~12s wall locally. Findings fixed (the full non-ST1005 set): - ST1008: runSet (resources_set_test) returns (string, error), error last - ST1003: CheckMaskIdColumn -> CheckMaskIDColumn (+ call sites/comments) - ST1003: errors_as -> errorsAs, io_eof_or_similar -> ioEOFOrSimilar - ST1020: CoreFloorText doc comment starts with the function name ST1005 stays excluded: it flags ~58 customer-visible error strings that need a wording review, not a mechanical sweep — separate follow-up. Rebased onto develop: union-merged the lint job with develop's govulncheck job (#278), coverage-floor step (#301) and deadcode advisory gate; kept the audit comment #286 added above CheckMaskID*Column. The 'going back to the action' comment in build.yml is KEPT (not deleted) — #6 is still open on develop, so golangci-lint-action stays disabled and that rationale still holds. Detached the four data_ingest_*.go header comments (#303 split) plus exitcodes.go's (#284, which develop merged after this branch was cut) from the package clause so the newly-enabled staticcheck gate is ST1000-clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) (#304) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci: flip deadcode gate to blocking; delete the 3 dead #127 leftovers (#313) * ci: goimports -local gate + Dependabot config (gomod weekly, actions monthly) goimports -local github.com/tracebloc/cli now blocks in both loops: build.yml's lint job (pinned goimports@v0.48.0, same x/tools version as the deadcode pin) and the Makefile's fmt-check target, with make fmt extended to auto-fix. .golangci.yml already declared this grouping via local-prefixes but nothing enforced it — 4 files had drifted (data.go, data_test.go, ingestion_run_test.go, resources_set_test.go), fixed here with import-grouping-only diffs. .github/dependabot.yml extends the org's backend-only Dependabot pilot: gomod weekly (k8s.io/* + sigs.k8s.io/* grouped, golang.org/x/* grouped), github-actions monthly. Unlike backend's security-only config, the CLI takes real version updates — customers install this binary, so staying current is security posture (see #276). Org-wide rollout decision flagged to Asad on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: canonicalize import grouping in data_test.go for the goimports gate Regroups imports into the layout goimports -local github.com/tracebloc/cli emits (stdlib / third-party / tracebloc-local). gofmt and goimports only sort within existing blank-line groups, so the manual groups passed fmt-check as-is but were not the canonical single-block output; Bugbot and our precheck both flagged the divergence. Verified idempotent under the pinned goimports v0.48.0 and green on make fmt-check + go build ./... Rebased onto ci/279 (post-#303 data.go split): data.go's import block is already canonical from the #303 split, so its canonicalization here is a no-op and dropped — this commit now regroups data_test.go only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: flip deadcode gate to blocking; delete the 3 dead #127 leftovers The deadcode CI step loses continue-on-error (and the Makefile target its '|| true'): both now run scripts/deadcode-check.sh, which fails on any function unreachable from ./cmd/tracebloc that isn't declared in scripts/deadcode-allowlist.txt. The tool itself always exits 0, so the old advisory step never could have blocked — the gate keys on output. Deleted (verified still dead with the pinned deadcode@v0.48.0): - config.clearAll + its three dedicated tests (TestClearAll, TestClearAll_HomeError, TestClear) — logout uses Save, nothing else ever called it - push.allCategoryIDs — moved verbatim into category_registry_test.go (the registry-pinning tests legitimately iterate every id; the shipped binary shouldn't carry the helper) - submit.isSubmitError — moved into client_test.go as the test-local assertion helper it always was (orphaned 'errors' import dropped) Allowlisted with reasons (the 4 legit findings): Status.String + JobOutcome.String (fmt-reflection Stringers) and ReadLabelValues + inferColumnType (di#349 test-only parity harnesses). Stale allowlist entries warn without failing; line numbers are stripped so edits that shift code don't red the gate. Coverage floors still clear after the test deletions (cli 82.9% >= 68, submit 80.3% >= 72). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
LukasWodka added a commit
that referenced
this pull request
Aug 17, 2026
…s a way back (cli#515) (#519) * fix(cluster): say which namespace, instead of offering --namespace blindly The §7.3 binding-miss error is RFC-0001's sentence with the remedy deleted: it names --namespace but never says WHICH namespace, and nothing else in the CLI will tell you. On a laptop with a healthy local install that left no supported way back. explain now diagnoses before advising. A binding-applied noParentReleaseError carries the clientset and server URL of the cluster that actually missed, and explain spends one naming-only cluster.FindClientNamespaces — the same read discoverRelease already spends purely to write a better message — then branches on isLocalServerURL: one client + local server URL name it, offer `client create` (a re-run on a cluster that already hosts a client adopts it, so the repoint mints nothing) client(s) on a remote cluster name the namespaces, offer ONLY --namespace; never `client create` there, because the client we found may be a colleague's (§7.5) none, scan clean today's text plus "No tracebloc client is running on this cluster either", which is when the installer is the right advice could not look today's text, byte for byte The last branch is the point of the three-valued clientSurvey: a nil probe or a failed scan is an absence of evidence, and printing it as "nothing is running here" would tell a user with a working client the opposite of the truth. allowScan() is untouched and still false for an applied binding: this changes what the CLI says, never what it targets. TestActiveClientBinding_AllowScan and TestDiscoverRelease_NoScanWhenExplicit pass unmodified, and TestExplain_BindingMiss_NamesTheLocalClientWithoutRetargeting pins both halves at once — the namespace appears in the message and nowhere else. The probe travels on the error rather than through explain's signature so a caller cannot hand it a clientset for a different cluster than the one that missed; six of the seven call sites never held one anyway (resolveClusterTarget builds it internally and returns nil on the error path). Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(doctor,home): a wrong pointer is not proof there is no environment #401 taught the home screen that an EMPTY active-client pointer says nothing about what runs on this machine. The wrong-pointer case was never covered, and it is the worse one: doctor binds the stale pointer, probes only that namespace, never scans, and prints "No secure environment on this machine yet" with the installer command underneath — over a perfectly healthy install. home has the same hole from the other side: its local-env fallback sat behind `if !binding.applied`, so a non-empty pointer skipped the #401 fix entirely. Both now route the miss through the same fallback: home the ErrNoParentRelease branch returns localEnvFallback(ctx) instead of a bare localNoRelease. Every failure inside the fallback degrades to localNoRelease, so this branch's old return value is still its floor. doctor on a ReachNoEnv result that a BINDING (not the user) aimed, re-probe the namespace the kubeconfig itself selects, via localEnvNamespace. The ownership gate is what makes this safe, and it is unchanged: both adopt only when isLocalServerURL says the kubeconfig's server is this machine — a cluster that is this machine by definition, so whatever runs there is this machine's environment. On a remote or shared cluster the honest no-environment answer stands, and a colleague's client is never greeted as yours (§7.5). No scan is spent either: the installer points the kubeconfig context at the client's namespace (client/scripts/lib/install-client-helm.sh runs `kubectl config set-context --current --namespace <ns>`), so reading the context is enough to find a healthy install that the pointer missed. doctor keeps the original results unless the re-probe actually finds an environment, so a genuinely bare machine still gets the installer advice and the --diagnose bundle still describes the namespace the user is configured for. An explicit --namespace is never second-guessed — no binding, no re-probe. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(client): list `create`, and stop calling a stale pointer "this machine" Two things the diagnose-before-advising fix needs in order to reach anyone. `client create` is visible. It was Hidden because a human running it standalone on a cluster with no client mints one the installer never deploys — an orphaned phantom (backend#970). That risk is real and unchanged, but hiding the command was never what prevented it. The two guards that do are untouched, and now have tests naming them: • on a TTY, the review + "Provision this client?" confirm. A re-run on a cluster that already hosts a client never reaches it — adoption happens first — so the repoint stays prompt-free and mints nothing. • off a TTY, a hard refusal without --yes/--credential-file, so a pipe or CI can never mint silently. What hiding did cost is #515: the one command that repoints a machine was unlisted, so the error telling a user to repoint pointed at nothing they could find. Its Short/Long now describe what it does for a user (adopt/repoint) rather than the installer's use of it. `client list` stays hidden. `client list` marks residency, not just selection. It labelled the active pointer "(active — this machine)" without ever checking where that client runs — so in exactly the state this ticket is about, the listing sat there confirming a client provably not on this machine. Selection (the local pointer) and residency (does it run on the cluster the kubeconfig reaches, keyed on the §7.2 cluster anchor) are now two separate facts, and a mismatch names the repoint. An unreadable anchor is a third state, not a "no": with no kubeconfig or an unreachable API server, no row claims to be here and none is denied — the marker degrades to bare "(active)". The installer's #303 pre-flight is unaffected; the markers sit in the row label and the greppable `namespace=<ns>` field is untouched (client_list_contract_test.go still passes). Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(cli): regenerate the copy catalog; `client create` is no longer hidden The golden diff is the whole user-visible change, reviewed line by line: 08-client.golden `create` now appears under Available Commands, with its new Short and the Long that explains adoption. zz-all-strings.golden the five §7.3 branches and the `client list` mismatch hint. Each branch of repointMessage is one format literal rather than a `+`-joined string, because the catalog's AST harvest only sees literal arguments — the message it replaced was invisible there for exactly that reason, and half a sentence in the completeness backstop is worse than none. cli-navigation.md carried two statements this change makes false: it drew `client create` as a hidden node, and its exit-4 remedy line said "run the installer (or --namespace)", which is now only one of three answers. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(release): bump VERSION to 0.10.9 v0.10.8 is already released and this PR changes published files under `internal/*`, so version-bump-gate (a required check) asks for the bump here rather than leaving it to fail the next prod hop on somebody else (backend#1561). The release train reads VERSION and cuts the tag from it. Patch, matching this repo's dominant pattern for user-facing copy and surface changes — say so on the PR if 0.11.0 is wanted for the `client create` unhide. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(cluster): say why explain replaces the error instead of wrapping it `%w` is the house convention (~325 sites), which makes a bare errors.New here read as an oversight. It is the same deliberate replacement the fmt.Errorf it replaced did: the §7.3 guidance is meant to BE the message, not to trail the raw "no release in namespace X". Wrapping would also make the result re-match errors.As(*noParentReleaseError) and so re-explainable. Recorded in place so a reviewer doesn't have to re-derive it. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(doctor,home,client): finding the environment is only half the story Both Bugbot findings on this PR were right, and both are the same mistake: a two-valued answer where the honest answer has three values. HIGH — "stale pointer still blocks after fallback". Re-probing found the healthy environment and then said nothing about the pointer that missed. So doctor printed "Everything looks good — you're ready to run training" and exited 0 while `data`/`resources`/`seal` all still bind the stale namespace and exit 4; before this PR that state at least exited 3. doctor now names the stale pointer and the repoint, and exits 2 — the code it already uses for every actionable finding. A problem WAS found; it just isn't in the cluster. The home half was worse, and was newly introduced here: local liveness came from the fallback's client while the heartbeat is still looked up by the STALE client's id, so a colleague's machine being online could render this one green. envProbe carries pointerStale, and resolveHomeModel refuses both directions off it — a stale heartbeat can no longer green the screen, nor harden into "backend reports not online" for a client it isn't about. It drops to "running, couldn't confirm", which is exactly true. MEDIUM — "empty cluster ID marked absent". `client list` compared anchors as a boolean, so a client whose OWN anchor is empty — legacy / not-yet-backfilled, which api.ProvisionedClient documents — was reported as "NOT on the cluster your kubeconfig reaches", with the repoint hint, possibly while running on this very machine. Exactly the collapse this PR's cluster-anchor handling was careful to avoid, missed one level down. Residency is now a three-valued residencyOf(): either anchor missing is resUnknown, and unknown claims nothing either way. realProbeEnv moved to home_local_fallback.go: home.go went 13 lines over its file budget, and the probe is now mostly a decision about WHICH fallback to take, so it reads better beside them than beside the renderer. Six mutations, each with its anchor asserted and each reddening an assertion rather than the compiler: collapse the empty client anchor; let doctor green a stale pointer; drop doctor's stale-pointer note; stop marking the fallback's result stale; let a stale pointer render Online; let another client's not-online harden into a verdict. TestDoctor_HealthyPointer_StillGreen is the control — without it, "never says Everything looks good" would pass vacuously. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(doctor): put the stale-pointer finding IN the readiness line Rendering the previous commit showed the fix was half done: the closing verdict was honest, but the line above it still read ✔ Connected to tracebloc ✔ Ready to run training ⚠ Your active client points at namespace "stale-ns" … — a green tick and, directly beneath it, a warning saying the opposite. That is the same unearned success the finding was about, moved up the screen. `Ready to run training` is false whenever the pointer is stale, however green the cluster checks are, because every data command binds the pointer. So the readiness healthLine is replaced rather than accompanied, and it carries the remedy, so the finding and the fix read as one thing: ✔ Connected to tracebloc ✖ Not ready — your active client points at namespace "stale-ns", which isn't on this cluster, so data commands will keep failing until you repoint. Point this machine at the environment above: tracebloc client create (this cluster already runs a client, so it adopts it — no new credential) Phrased "Not ready — …" to match the three readiness failures already in the catalog. `--diagnose` records the replaced line, which is what triage needs. Exit stays 2 via the pointerStale branch, which skips the "email support" nudge a doctorVerdict fail would add — we just gave a precise one-command fix. Three more mutations: disable the replacement (green tick returns) → red; drop the remedy → red; stop naming the stale namespace → red. The already-added TestDoctor_HealthyPointer_StillGreen now also asserts the green tick IS present when nothing is stale, so "no green tick" can't pass by the line disappearing entirely. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(client,home): don't name a target we haven't confirmed exists Bugbot round 2, two Mediums, and the same class a third time: a sentence asserting something the code never established. `client list` set the mismatch hint from the ACTIVE row alone, so with the pointer elsewhere and nothing confirmed on this cluster it still said "point this machine at the client that IS there: … client create". There may be no client there — and on a cluster with none, `client create` falls through to the MINT path and produces the orphaned phantom backend#970 exists to prevent. So this PR's own advice could manufacture the bug the command was hidden for. The repoint is now offered only when some row is resHere — that is what earns the phrase "the client that IS there". Otherwise the mismatch is still reported, without a target: "no client here is confirmed. Check your kubeconfig context, then run: … doctor". Deliberately covers BOTH remaining cases, because they are equally unnameable — no client here at all, and clients that might be here but carry no anchor to prove it (resUnknown). The home screen had the label version of the same thing: with a stale pointer, `env.name` was still overridden by the remembered handle, so the client the pointer names was printed as the environment running here — a client that is by construction NOT what the fallback found, and a name contradicting doctor's for the identical state. The override is now skipped when the pointer is stale, so the screen falls through to the probe's own name for the release that is actually running. Three mutations: let the repoint hint fire without anyHere → red; source anyHere from the active row instead of residency → red; restore the unconditional remembered-name override → red. Both fixes carry a control assertion in the same test (the repoint IS offered when a row is here; the remembered name IS still preferred when the pointer is fresh), so neither can pass by the behaviour disappearing altogether. N10's first attempt left `anyHere` unused and reddened the compiler; rewritten to keep it used and re-run. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(doctor): adopt the re-probe only on a CONFIRMED reachable cluster Bugbot round 3, High, and correct. The re-probe adopted on `reachStateOf(retry) != doctor.ReachNoEnv`, and ReachState has four members: ReachUnreachable and ReachError also satisfy that. Both mean "we could not tell". So a stale pointer plus RBAC on the context namespace, or a transient read failure, would have: • named an unconfirmed namespace as `Secure environment "…"`, • set pointerStale and printed "this cluster already runs a client, so it adopts it — no new credential", • and sent the user to `client create` on a cluster that may host nothing, where it does not adopt but MINTS — the backend#970 phantom. Which is the exact absence-as-presence collapse surveyCluster's `looked` and residencyOf's resUnknown exist to prevent, made twice more in the same PR. Adoption now requires a positive confirmation, via reachConfirmedOK(). It is deliberately NOT `reachStateOf(results) == ReachOK`: reachStateOf defaults to ReachOK when the check is ABSENT, which is the right lenient default on the main path and precisely the wrong one here, where the whole question is whether an unproven namespace may be believed. Absent, unreachable and errored all answer "could not tell", and none may authorize naming an environment or advising a repoint. The test derives its input domain from doctor.ReachState's declared surface — every non-OK member, plus the absent case — rather than picking the states that came to mind: mutation coverage cannot see a vocabulary gap, so a future member has to be added to the enum's own list to escape it. Three mutations: restore `!= ReachNoEnv` → three subtests red; make an absent check count as confirmed → red; let ReachError confirm → red. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(bugbot): make the recurring finding on this PR a rule Org standard: "A finding that recurs across PRs becomes a rule: add it to .cursor/BUGBOT.md". This one recurred three times inside a SINGLE PR — a failed scan read as "nothing is here", an empty legacy cluster_id read as "runs elsewhere", and `!= ReachNoEnv` read as "an environment is here" — each found by Bugbot only after the previous was fixed. Three instances of one root cause is past the threshold. BUGBOT.md already had the neighbouring rule, but scoped to the value a function RETURNS ("prefer a three-valued return"). Every instance here got the return type right and then collapsed it at the `if` that consumed it, so the existing bullet didn't catch any of them. The new bullet is about the branch, and names the two concrete shapes rather than restating the principle: • a negated comparison against ONE member of a multi-valued enum, which silently absorbs every member added later — with the corollary that the test's input domain must come from the enum's declared surface, since mutation coverage cannot see a vocabulary gap; • a lenient "not found" default reused where the question is "may I believe this?" — reachStateOf returning ReachOK for an ABSENT check is right for a verdict roll-up and wrong for authorising a claim, which is why reachConfirmedOK exists beside it. It closes with the customer-visible cost, per this file's own Tone section: each instance ended in advice to run `client create` on a cluster nothing was confirmed on, where it mints instead of adopting — the guidance manufacturing the orphaned phantom backend#970 is about. Refs cli#515 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Mechanical split of the 1539-line
internal/cli/data.gointo per-concern files — pure moves of whole top-level declarations, samepackage cli, zero behavior change:data_ingest_cmd.gonewDataIngestCmd(full flag surface incl. hidden deprecated aliases) +runDataIngestArgsdata_ingest_local.gosortedKeys,expandHome,statDatasetPath,printLocalSummary,runLocalPreflightdata_ingest_cluster.gorunIngestionRun,shouldReclaimStaging,printClusterSummary, the cluster test seams,destTableExists,existingTableActiondata_ingest_output.gopushJSONResult/pushJSONSummary,writePushJSON/writePushErrorJSON,classifyPushOutcome—writePushJSON+classifyPushOutcomedeliberately kept in one file (mutation-hardened lockstep)data.go(693 lines) keeps thedatagroup command, the deprecation-alias notice, andrunDataIngestitself — its extraction is the follow-up (#283).The only new text is each file's header comment + package/import clauses. Every moved section was verified byte-for-byte against the pre-split file with a line-range check (every non-blank line of the old file lands verbatim in exactly one new location).
data_test.gois untouched.Fixes#282
Part of tracebloc/backend#1106.
Type of change
Test plan
Ran locally:
go build ./...— cleango test ./...— full suite green (internal/cli 5.5s, all packages pass)go vet ./internal/cli/+gofmt -s -l .— cleanChecklist
data_test.gocompiles unchangedStacked PR 1/4 of the WS-B data.go chain — merge order: #this → cli#283 → cli#284 → cli#285. Each later branch is based on the previous one; merge this first, the rest rebase trivially.
🤖 Generated with Claude Code
Note
Low Risk
Pure file moves within
package cliwith tests and build reported green; no logic or API changes beyond organization.Overview
Mechanically splits the oversized
internal/cli/data.go(~1.5k lines) into four same-package files so ingest concerns are easier to navigate, with no intended behavior change.data_ingest_cmd.go—newDataIngestCmd, flags, andrunDataIngestArgsdata_ingest_local.go— local path helpers,printLocalSummary,runLocalPreflightdata_ingest_cluster.go—runIngestionRun, destination-table guards, cluster test seamsdata_ingest_output.go—--output-jsontypes/writers andclassifyPushOutcomedata.gonow holds thedatagroup command, deprecation-alias wiring, andrunDataIngestorchestration (~693 lines). Imports ondata.gowere trimmed to match what stayed.data_test.gois unchanged.Reviewed by Cursor Bugbot for commit 9f75c1d. Bugbot is set up for automated code reviews on this repo. Configure here.