Uh oh!
There was an error while loading. Please reload this page.
test(data): seam the money path — table-test the ingest outcome matrix (#1009 P0) - #187
Merged
Merged
Conversation
…x (#1009 P0) runDataIngest's submit → port-forward → watch → classify → JSON → reclaim tail had no injection seam, so the path a customer actually lives in was covered by nothing (36.6%). A regression where the CLI previews success but the ingestor writes null/fewer, or reclaims staging on a partial failure, would ship silently. Extract the tail into runIngestionRun and route its four cluster-touching steps through package-level seams (mintIngestorTokenFn / portForwardJobsManagerFn / submitRunFn / cleanStagingFn), mirroring the existing listDatasetsFn seam. The extraction is behavior-identical — runDataIngest just calls it and threads jsonEmitted back for the --output-json error defer. Name the reclaim gate: shouldReclaimStaging(status) — the "reclaim ONLY on a clean success" invariant, so a detached / partial / failed / errored run keeps the staged source (for the still-reading Job, or for retry/inspection). This is the "must NOT reclaim on partial failure" gate #1009 calls out. Tests (no cluster needed): - TestRunIngestionRun_Matrix drives the whole tail through the seams and asserts, per outcome, the exit code (5 auth / 8 submit / 9 watch+ingest / 0 success+detached), whether the staging reclaim ran (only on succeeded), and the emitted --output-json status — all in lockstep. Covers the mint-fail (5) and port-forward-fail (8) pre-submit returns too. - TestShouldReclaimStaging pins the gate in isolation. - Extends the existing TestClassifyPushOutcome to cover the exit-5 (auth) and exit-8 (submit) buckets it was missing, plus unknown / nil-result. - TestSeamsWiredToRealFns guards that no seam is left nil. Also hardens submit.ForwardedConnection.Close to no-op on a zero-value connection (nil stopCh) — it panicked on close(nil) before, which blocked using a fake in the seam test; a never-started connection now closes safely. Coverage: internal/cli 70.1% → 72.2%; runIngestionRun 94.1%, shouldReclaimStaging 100%, classifyPushOutcome 92.9%. Part of backend#1009 (P0 seam the money path). Remaining #1009: CI drift wiring (pin the goldens/schema SHA + a per-package coverage floor so this can't rot), the cross-repo taxonomy contract test, and one content-compared ingest e2e. Part of the data-ingest epic backend#1008. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LukasWodka
commented
Jul 8, 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 0c39dc7. Configure here.
saadqbal added a commit
that referenced
this pull request
Jul 9, 2026
…0.3) (#188) * ci: pin the data-ingestors schema ref + add a coverage floor (#1009 P0.3) Two of the three CI drift tripwires from #1009. (The third — a scheduled `sync-validator-goldens.sh --check` against the pinned ingestor — waits for #340 to land on a stable ref, since the value-aware check regenerates against the ingestor read path and needs the #340 fix; the Go-side parity test already enforces the committed goldens on every PR.) (b) Pin the schema-drift check to a data-ingestors SHA, not floating master. sync-schema.sh now builds the fetch URL from scripts/.data-ingestors-ref (a pinned commit SHA), overridable via DATA_INGESTORS_REF. Before, any upstream commit touching the schema reddened every open CLI PR until someone synced; now adopting upstream is a deliberate SHA bump + re-sync in one PR. Pinned to 0de1f148 (current master; the embedded schema matches it — check stays green). (c) Add a per-package coverage floor. `go test -cover` printed numbers and asserted nothing, so the two load-bearing, historically thin-tested packages could silently rot. scripts/coverage-floor.sh fails the build if internal/cli or internal/submit drops below its floor; wired into the Test job after `go test`. Floors are a RATCHET (bump UP only, lowering is a reviewed edit), set just under current develop: internal/cli 68% (now 70.1%), internal/submit 72% (now 74.8%). Bump these up once #186/#187 land (they lift internal/cli to ~72%). The script is bash-3.2-portable (no associative arrays — macOS default). Verified locally: sync-schema.sh --check matches at the pinned SHA; coverage-floor.sh passes and bites (a 99% floor fails, a below-current floor passes); shellcheck clean; build.yml parses; full suite green. Part of backend#1009 (P0 CI drift tripwires). Remaining #1009: the goldens drift job (post-#340), the cross-repo taxonomy contract test, one content-compared ingest e2e. Part of the data-ingest epic backend#1008. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: fail loudly on a malformed coverage-floor entry (#1009 P0.3) A dropped ":floor" in FLOORS left min="$entry" (the whole token); the awk comparison then errored on that as bare source and exited non-zero, which `if awk` read as "not below floor" and printed a bogus "ok" — the ratchet became a silent no-op for that package. Validate each entry has a "package:INT" shape before the awk call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: validate the data-ingestors ref before it enters the schema URL (#1009 P0.3) The ref (pinned file value, or the DATA_INGESTORS_REF override) is interpolated into a raw.githubusercontent.com URL. An unvalidated ref could inject path traversal ("../..") or extra path segments — the same class scripts/install.sh already guards for its release tag. Restrict the ref to a SHA/branch/tag shape (alnum start; alnum . _ - / ; no "..") and fail with exit 2 otherwise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Asad Iqbal <asad.dsoft@gmail.com>
saadqbal
approved these changes
Jul 9, 2026
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 14, 2026
saadqbal pushed a commit
that referenced
this pull request
Jul 14, 2026
…unDataIngest 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>
saadqbal added a commit
that referenced
this pull request
Jul 14, 2026
…unDataIngest (#321) 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: Lukas Wuttke <lukas@tracebloc.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
saadqbal pushed a commit
that referenced
this pull request
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 pushed a commit
that referenced
this pull request
Jul 14, 2026
…mand docs table (#315) * 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) wired into 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> --------- Co-authored-by: Claude Fable 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
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
runDataIngest's tail — submit → port-forward → watch → classify → JSON → reclaim — is the path a customer actually lives in, and it had no injection seam, so it was covered by nothing (36.6%). A regression where the CLI previews success but the ingestor writes null/fewer, or where it reclaims the staging copy on a partial failure (deleting the source the user needs to retry), would ship silently.This is the P0 "seam the money path" item of #1009.
What changed
runIngestionRunfromrunDataIngestand route its four cluster-touching steps through package seams —mintIngestorTokenFn/portForwardJobsManagerFn/submitRunFn/cleanStagingFn— mirroring the existinglistDatasetsFnseam. The extraction is behavior-identical;runDataIngestjust calls it and threadsjsonEmittedback for the--output-jsonerror defer.shouldReclaimStaging(status)— the "reclaim ONLY on a clean success" invariant. A detached / partial / failed / errored run keeps the staged source (the Job may still be reading it, or the user wants to retry/inspect). This is the "must NOT reclaim on partial failure" gate #1009 calls out.submit.ForwardedConnection.Closeto no-op on a zero-value connection (nil stopCh) — it panicked onclose(nil)before, which is a latent bug and blocked using a fake in the seam test.Tests (no cluster needed)
TestRunIngestionRun_Matrixdrives the whole tail through the seams and asserts, per outcome, three things in lockstep: the exit code (5 auth / 8 submit / 9 watch+ingest / 0 success+detached), whether the staging reclaim ran (only onsucceeded), and the emitted--output-jsonstatus. Includes the mint-fail (5) and port-forward-fail (8) pre-submit returns.TestShouldReclaimStagingpins the gate in isolation.TestClassifyPushOutcometo cover the exit-5 (auth) and exit-8 (submit) buckets it was missing, plusunknown/nil-result.TestSeamsWiredToRealFnsguards that no seam is left nil.Coverage:
internal/cli70.1% → 72.2%;runIngestionRun94.1%,shouldReclaimStaging100%,classifyPushOutcome92.9%. Full suite green.Type
Test hardening + a latent-panic fix · cli · RFC-0002 foundation. Part ofbackend#1009 (P0 seam the money path) — independent of the value-level-parity PR (cli#186) and of data-ingestors #340.
Remaining #1009: CI drift wiring (pin the goldens/schema SHA + a per-package coverage floor so this can't rot), the cross-repo taxonomy contract test, one content-compared ingest e2e.
Note
Low Risk
Refactor is extraction plus test seams with stated behavior parity; the only production behavior change is the defensive port-forward Close fix.
Overview
Extracts the post-staging ingest tail (token mint → jobs-manager port-forward → submit/watch → classify →
--output-json→ staging reclaim) intorunIngestionRun, with behavior unchanged forrunDataIngestaside from threadingjsonEmittedback for the early-failure JSON defer.Cluster-touching steps go through package-level seams (
mintIngestorTokenFn,portForwardJobsManagerFn,submitRunFn,cleanStagingFn) so tests can drive the full exit code / JSON status / reclaim matrix without a cluster. The reclaim-only-on-clean-success rule is namedshouldReclaimStaging.submit.ForwardedConnection.Closenow no-ops whenstopChis nil (zero-value / test fakes), fixing a latentclose(nil)panic.Adds
ingestion_run_test.go(TestRunIngestionRun_Matrix,TestShouldReclaimStaging,TestSeamsWiredToRealFns) and extendsTestClassifyPushOutcomefor auth (5), submit (8),unknown, and nil-result cases.Reviewed by Cursor Bugbot for commit 0c39dc7. Bugbot is set up for automated code reviews on this repo. Configure here.