Uh oh!
There was an error while loading. Please reload this page.
refactor(cli): name every exit-code site via exitcodes.go + cross-command docs table - #315
Merged
Merged
Conversation
LukasWodka
commented
Jul 14, 2026
ContributorAuthor
👋 Heads-up — Code review queue is at 56 / 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.) |
LukasWodka
changed the base branch from
develop
to
refactor/283-rundataingest-extractJuly 14, 2026 12:49
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 a6de340. Configure here.
saadqbal
previously approved these changes
Jul 14, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
👍 Faithful mechanical refactor — the exitcodes.go constants match every code I'd verified across the commands (delete 4/5/7, submit 8/9, doctor 2, kubeconfig 3), the intentional shared numbers (2/5/7) are named per-context for readability, and the exit-code test matrix is green so nothing shifted. Naming these makes the exit contract much easier to audit. (Stacked on #309.)
This was referenced Jul 14, 2026
Merged
saadqbal
commented
Jul 14, 2026
Collaborator
saadqbalforce-pushed
the
refactor/283-rundataingest-extract
branch
from
July 14, 2026 14:43
a2464db to
e04bc04Comparesaadqbalforce-pushed
the
refactor/284-exitcodes-naming
branch
from
July 14, 2026 14:43
a6de340 to
8d93e43Compareinternal/cli/exitcodes.go names every exit code the CLI produces; every non-test &exitError construction site now uses a named constant instead of a bare number. exit.go keeps owning the extraction — this is a naming sweep, not a redesign; all numeric values are unchanged (and documented as frozen: they're the scripting contract customers branch on). Site census (the ticket's count of 138 grep hits includes one doc comment in exit.go; 137 are real construction sites): 61x1, 25x2, 23x3, 7x4, 4x5, 1x6, 5x7, 2x8, 4x9, 2x130, 2x kubeconfigExitCode(...) (the helper now returns named constants), 1x variable (runLocalPreflight's BadFlag fold, now built from constants). Zero numeric literals remain at non-test sites. Codes that grew more than one per-command meaning get one constant per MEANING sharing the value (2: exitBadInput / doctor's exitChecksFailed; 5: exitAuth / data delete's exitNoSuchDataset; 7: exitStagingFailed / exitTeardownFailed / exitQueryFailed), so each site reads honestly. docs/troubleshooting.md gains the cross-command exit-code table (code, meaning, producing commands, constant name), sourced from the per-command long-help blocks — data ingest's (data_ingest_cmd.go, pre-split data.go:236-252) is the fullest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
saadqbalforce-pushed
the
refactor/284-exitcodes-naming
branch
from
July 14, 2026 15:55
8d93e43 to
f12fa87Comparesaadqbal
approved these changes
Jul 14, 2026
saadqbal
previously approved these changes
Jul 14, 2026
…o make ci + build.yml (#316) * refactor(cli): extract resolveLocalInput + connectIngestTarget from runDataIngest runDataIngest was 586 lines; every pre-cluster step and the cluster pre-flight now live in two named stanza functions, moved verbatim: - resolveLocalInput (data_ingest_local.go) — steps 0–4 + the P3 content preflight: flag guard, banner, guided prompts, ~-expansion + existence-first check, table-name/category/misapplied-flag validation, layout walk, per-category spec resolution, spec synthesis + schema validation, local summary. Mutates a in place (via *runDataIngestArgs) so the --output-json defer and the cluster steps see the resolved spec exactly as before. - connectIngestTarget (data_ingest_cluster.go) — steps 5–8a: kubeconfig resolve, release + PVC discovery, verbose cluster summary, and the destination-table guard (incl. the folded interactive replace decision, which still flips a.Overwrite). The jsonEmitted/named-return defer dance stays INSIDE runDataIngest, next to the err it reads — both extracted functions feed that named return, so the error-JSON contract is unchanged. The dry-run stop (which sets jsonEmitted) also stays inside. Only in-stanza edits are what the new signatures force: multi-value returns on error paths, := to = where a named result already exists, and &a -> a where a is already a pointer. Step order, output order, and exit codes are untouched. data.go: 693 -> 240 lines; runDataIngest: 586 -> 138. Safety net: the #187 outcome-matrix table test + #264 mutation pins pass untouched; full suite green under -race; coverage floors hold (82.7%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(cli): name every exit-code site + cross-command docs table internal/cli/exitcodes.go names every exit code the CLI produces; every non-test &exitError construction site now uses a named constant instead of a bare number. exit.go keeps owning the extraction — this is a naming sweep, not a redesign; all numeric values are unchanged (and documented as frozen: they're the scripting contract customers branch on). Site census (the ticket's count of 138 grep hits includes one doc comment in exit.go; 137 are real construction sites): 61x1, 25x2, 23x3, 7x4, 4x5, 1x6, 5x7, 2x8, 4x9, 2x130, 2x kubeconfigExitCode(...) (the helper now returns named constants), 1x variable (runLocalPreflight's BadFlag fold, now built from constants). Zero numeric literals remain at non-test sites. Codes that grew more than one per-command meaning get one constant per MEANING sharing the value (2: exitBadInput / doctor's exitChecksFailed; 5: exitAuth / data delete's exitNoSuchDataset; 7: exitStagingFailed / exitTeardownFailed / exitQueryFailed), so each site reads honestly. docs/troubleshooting.md gains the cross-command exit-code table (code, meaning, producing commands, constant name), sourced from the per-command long-help blocks — data ingest's (data_ingest_cmd.go, pre-split data.go:236-252) is the fullest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: file-budget line-count ratchet (scripts/file-budget.sh) Clones the coverage-floor.sh pattern for file size: path:max_lines pairs, checked by wc -l, portable to bash 3.2, with the same malformed-entry guard — plus a missing-file guard so a moved/renamed budgeted file makes the stale list loud instead of silently un-budgeting it. Ceilings are a RATCHET: only ever lowered as files shrink; raising one is a deliberate, reviewed edit to the checked-in script. Seeds sit just above today's reality (develop + the WS-B split): internal/push/preflight.go:1650 (now 1635) internal/cli/data.go:500 (now 240 post cli#282/#283) internal/cli/client.go:1050 (now 1027) internal/cli/home.go:850 (now 841) Wired into `make ci` (new file-budget target, keeping the Makefile's "make ci green => CI green" invariant) and into build.yml's lint job as one surgical step. Exercised all three failure paths locally (over budget, missing file, malformed entry) — each exits 1 with a ::error:: annotation; shellcheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(file-budget): seed preflight.go at true size (Bugbot #316 HIGH) The ratchet seeded internal/push/preflight.go at 1650, but the file is 1655 lines (wc -l, same count the gate uses) and the check is strict- greater — so `make file-budget` would fail the moment this landed on develop. Raise the ceiling to 1700, the next round-50 ratchet value just above actual, matching how the other seeds sit above their files (client.go 1050>1027, home.go 850>849, data.go 500>240). Also correct the header comment to reality (preflight ~1655, home ~849); it wrongly read ~1635 / ~841. Proof: `bash scripts/file-budget.sh` now passes (preflight 1655 <= 1700). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
saadqbal
approved these changes
Jul 14, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
Re-approving — #316 merged into this PR's base branch, so #315 now carries both the exit-code-naming change AND #316's file-budget ratchet (both previously approved; build.yml keeps govulncheck + adds the File-budget step). Merging this lands both on develop. Approval was auto-dismissed when #316's commit was added.
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
internal/cli/exitcodes.gonames every exit code the CLI produces, and every non-test&exitError{code: N}site now uses a named constant.exit.gokeeps owning the extraction — this is a naming sweep, not a redesign: every numeric value is unchanged and documented as frozen (they're the scripting contract customers branch on).Site census — the ticket's grep count of 138 includes one doc comment in
exit.go; 137 are real construction sites, now all named:code: 1→exitFailurecode: 2→exitBadInput/exitChecksFailed(doctor)code: 3→exitLocalEnvcode: 4→exitNoWorkspacecode: 5→exitAuth/exitNoSuchDataset(data delete)code: 6→exitTableExistscode: 7→exitStagingFailed/exitTeardownFailed/exitQueryFailedcode: 8→exitSubmitFailedcode: 9→exitIngestFailedcode: 130→exitInterruptedkubeconfigExitCode(...)(helper now returns named constants)runLocalPreflight's BadFlag fold, now built from constants)Codes that grew more than one per-command meaning get one constant per meaning sharing the value, so each construction site reads honestly (e.g.
data delete's exit 5 is "no such dataset", not "auth").docs/troubleshooting.mdgains the cross-command exit-code table (code / meaning / producing commands / constant name), sourced from the per-command long-help blocks —data ingest's (nowdata_ingest_cmd.go; pre-splitdata.go:236-252) is the fullest.Deviations from the ticket, verified against the code: real distribution is 61×1 / 25×2 / 23×3 / 7×4 (ticket said 63/27/24/9); there is no
code: 0site (ticket said 1×0); the ticket missed the 2×130 Ctrl-C sites and the 2kubeconfigExitCodesites.Fixes#284
Part of tracebloc/backend#1106.
Type of change
Test plan
Ran locally:
go test ./...— full suite greengo build ./...,go vet,gofmt -s -l— cleancode:literals remain at non-test sitesChecklist
Stacked PR 3/4 of the WS-B data.go chain — merge order: cli#303 (#282) → cli#309 (#283) → this → cli#285. Branch is based on
refactor/283-rundataingest-extract; until #303/#309 merge, this diff shows their commits too. Review only the top commit (a6de340).🤖 Generated with Claude Code
Note
Low Risk
Refactor and docs only—exit numeric values are unchanged; the new file-budget gate only fails when tracked files exceed checked-in ceilings.
Overview
Introduces
internal/cli/exitcodes.gowith named constants for every CLI exit code (values unchanged and documented as frozen). All non-test&exitError{code: …}sites and helpers likeExitCodeFromError/kubeconfigExitCodenow use those names instead of raw integers, including shared numbers that mean different things per command (exitAuthvsexitNoSuchDataset, the exit-7 trio, etc.).docs/troubleshooting.mdgains a cross-command exit-code table tied to the constant names for grepping.Separately,
scripts/file-budget.shratchets line counts on a few large files;make ciand the build.yml lint job run it so local and CI stay aligned.Reviewed by Cursor Bugbot for commit ab1a923. Bugbot is set up for automated code reviews on this repo. Configure here.