diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 046a59e..850dd44 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -99,7 +99,7 @@ func TestClassifyPushOutcome(t *testing.T) { wantStat string wantCode int }{ - {"clean", &submit.Result{Submit: resp, Watch: &submit.WatchResult{Outcome: submit.JobOutcomeSucceeded, Summary: &submit.Summary{TotalRecords: 10, InsertedRecords: 10}}}, nil, "succeeded", 0}, + {"clean", &submit.Result{Submit: resp, Watch: &submit.WatchResult{Outcome: submit.JobOutcomeSucceeded, Summary: &submit.Summary{TotalRecords: 10, InsertedRecords: 10, APISentRecords: 10}}}, nil, "succeeded", 0}, {"partial", &submit.Result{Submit: resp, Watch: &submit.WatchResult{Outcome: submit.JobOutcomeSucceeded, Summary: &submit.Summary{TotalRecords: 10, InsertedRecords: 7, FailedRecords: 3}}}, nil, "completed_with_failures", 9}, {"failed", &submit.Result{Submit: resp, Watch: &submit.WatchResult{Outcome: submit.JobOutcomeFailed}}, nil, "failed", 9}, {"unknown", &submit.Result{Submit: resp, Watch: &submit.WatchResult{Outcome: submit.JobOutcomeUnknown}}, nil, "unknown", 9}, diff --git a/internal/cli/ingestion_run_test.go b/internal/cli/ingestion_run_test.go index 94b7bfa..c7eb7af 100644 --- a/internal/cli/ingestion_run_test.go +++ b/internal/cli/ingestion_run_test.go @@ -25,7 +25,11 @@ import ( func succeededResult() *submit.Result { return &submit.Result{ Submit: &submit.SubmitResponse{Namespace: "tracebloc", JobName: "ingestor-x"}, - Watch: &submit.WatchResult{Outcome: submit.JobOutcomeSucceeded, Summary: &submit.Summary{TotalRecords: 2, InsertedRecords: 2}}, + // A genuinely clean run: every stage counter equal (total == inserted + // == api_sent). APISentRecords must be set too — the ingestor-aligned + // HasFailures() treats api_sent < inserted as a partial, so omitting it + // (defaulting to 0) would misclassify this "succeeded" row as a failure. + Watch: &submit.WatchResult{Outcome: submit.JobOutcomeSucceeded, Summary: &submit.Summary{TotalRecords: 2, InsertedRecords: 2, APISentRecords: 2}}, } } diff --git a/internal/submit/summary.go b/internal/submit/summary.go index eb0a40b..0f19042 100644 --- a/internal/submit/summary.go +++ b/internal/submit/summary.go @@ -69,26 +69,41 @@ type Summary struct { FailedRecords int64 } -// HasFailures returns true if any failure-class counter is non-zero. -// Used by the orchestrator to decide which exit code to return -// (success: 0, ingest-with-failures: non-zero) and how to color -// the rendered panel. +// HasFailures returns true if any non-trivial failure occurred. It MIRRORS +// the ingestor's IngestionSummary.has_failures EXACTLY (data-ingestors +// ingestors/base.py) — DB insert short of total, API short of inserted, a +// file-transfer or processing drop (skipped), or a hard failure — so the CLI's +// exit code + staging-reclaim gate agree with the ingestor's own "completed +// successfully" banner. The narrower prior version (only FailedRecords / +// FileTransferFailures) reported success and reclaimed the staged source on a +// run that silently SKIPPED rows or inserted fewer than total — silent data +// loss that then deletes the user's only copy. Every counter this reads is +// emitted unconditionally by the ingestor banner + parsed above, so the +// inserted 0 || s.FailedRecords > 0 + return s.FailedRecords > 0 || + s.FileTransferFailures > 0 || + s.SkippedRecords > 0 || + s.InsertedRecords < s.TotalRecords || + s.APISentRecords < s.InsertedRecords } -// SuccessRate returns a 0-100 percentage for the panel header. -// Defined as ProcessedRecords / TotalRecords; returns 0 when -// TotalRecords is 0 to avoid divide-by-zero in early-failure -// banners. +// SuccessRate returns a 0-100 percentage for the panel header. Defined as +// InsertedRecords / TotalRecords — matching the ingestor's own banner +// (reporting.py: inserted_records / total_records), since InsertedRecords (rows +// that actually landed in MySQL) is the metric that matters for training, and +// ProcessedRecords (passed validation) is a superset that OVERSTATED success +// when rows validated but failed to insert. Returns 0 when TotalRecords is 0 to +// avoid divide-by-zero in early-failure banners. func (s *Summary) SuccessRate() float64 { if s == nil || s.TotalRecords == 0 { return 0 } - return float64(s.ProcessedRecords) / float64(s.TotalRecords) * 100 + return float64(s.InsertedRecords) / float64(s.TotalRecords) * 100 } // ansiCodeRE matches the ANSI SGR (Select Graphic Rendition) @@ -337,10 +352,21 @@ func RenderSummary(p *ui.Printer, s *Summary) { headline := fmt.Sprintf("ingested %s of %s records (%.1f%%)", commaSep(s.InsertedRecords), commaSep(s.TotalRecords), s.SuccessRate()) switch { - case s.HasFailures(): + case s.FailedRecords > 0 || s.FileTransferFailures > 0: + // Hard failures: rows errored at DB insert or file transfer. p.Errorf("Ingestion completed with failures — %s", headline) - case s.SkippedRecords > 0: - p.Warnf("Ingestion completed with skips — %s", headline) + case s.HasFailures(): + // No hard failure, but not clean: rows skipped, or fewer inserted/ + // synced than the ingestor saw. Exit-coded as not-clean (HasFailures), + // but colored distinctly from a hard failure. Word it by which soft + // shortfall actually occurred — "skips" only when rows were skipped; + // an insert/API shortfall with zero skips is a partial result, not a + // skip, and mislabeling it reads as a validator drop. + if s.SkippedRecords > 0 { + p.Warnf("Ingestion completed with skips — %s", headline) + } else { + p.Warnf("Ingestion completed partially — %s", headline) + } default: p.Successf("Ingestion complete — %s", headline) } diff --git a/internal/submit/summary_test.go b/internal/submit/summary_test.go index 6df9546..58ab6b7 100644 --- a/internal/submit/summary_test.go +++ b/internal/submit/summary_test.go @@ -71,20 +71,24 @@ func TestSummaryParser_RealBannerEndToEnd(t *testing.T) { // that the orchestrator uses to choose between success exit code // (0) and ingest-failure exit code (9). func TestSummaryParser_HasFailures(t *testing.T) { + // Mirrors the ingestor's IngestionSummary.has_failures exactly. cases := []struct { name string s *Summary want bool }{ {"nil", nil, false}, - {"all zero", &Summary{TotalRecords: 100, ProcessedRecords: 100}, false}, - {"file transfer failures", &Summary{FileTransferFailures: 1}, true}, - {"failed records", &Summary{FailedRecords: 1}, true}, - {"both", &Summary{FileTransferFailures: 1, FailedRecords: 1}, true}, - // Skipped records are NOT failures — they're rows that - // validators rejected. The customer wants to see the - // count but it doesn't change the exit code. - {"skipped is not failure", &Summary{SkippedRecords: 100}, false}, + // A genuinely clean run: every counter equal, nothing skipped/failed. + {"clean", &Summary{TotalRecords: 100, ProcessedRecords: 100, InsertedRecords: 100, APISentRecords: 100}, false}, + {"file transfer failures", &Summary{TotalRecords: 1, InsertedRecords: 1, APISentRecords: 1, FileTransferFailures: 1}, true}, + {"failed records", &Summary{TotalRecords: 1, InsertedRecords: 1, APISentRecords: 1, FailedRecords: 1}, true}, + // Skipped rows ARE a failure — a dropped row is silent data loss + // (#234); the ingestor counts it, so the CLI must too (was the bug). + {"skipped is a failure", &Summary{TotalRecords: 100, InsertedRecords: 100, APISentRecords: 100, SkippedRecords: 5}, true}, + // Fewer rows in MySQL than the ingestor saw → partial run. + {"inserted < total", &Summary{TotalRecords: 100, ProcessedRecords: 100, InsertedRecords: 99, APISentRecords: 99}, true}, + // Rows in MySQL but the central catalog got fewer. + {"api_sent < inserted", &Summary{TotalRecords: 100, InsertedRecords: 100, APISentRecords: 99}, true}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -99,6 +103,7 @@ func TestSummaryParser_HasFailures(t *testing.T) { // rendered panel's "Success rate: XX%" line. Divide-by-zero on // empty banner is the critical edge case. func TestSummaryParser_SuccessRate(t *testing.T) { + // Rate is INSERTED/total (matches the ingestor banner), not processed/total. cases := []struct { name string s *Summary @@ -106,8 +111,11 @@ func TestSummaryParser_SuccessRate(t *testing.T) { }{ {"nil", nil, 0}, {"empty banner", &Summary{}, 0}, - {"100%", &Summary{TotalRecords: 100, ProcessedRecords: 100}, 100}, - {"50%", &Summary{TotalRecords: 100, ProcessedRecords: 50}, 50}, + {"100%", &Summary{TotalRecords: 100, ProcessedRecords: 100, InsertedRecords: 100}, 100}, + {"50%", &Summary{TotalRecords: 100, InsertedRecords: 50}, 50}, + // The overstatement the fix closes: all rows validated (processed=100) + // but only 70 landed in MySQL → 70%, not the old 100%. + {"processed overstates: inserted