Skip to content

fix(summary): align CLI success/failure with the ingestor — inserted-based rate + full has_failures - #193

Merged
saadqbal merged 5 commits into
developfrom
fix/summary-success-parity
Jul 9, 2026
Merged

fix(summary): align CLI success/failure with the ingestor — inserted-based rate + full has_failures#193
saadqbal merged 5 commits into
developfrom
fix/summary-success-parity

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

fix(summary): align CLI success/failure with the ingestor (silent-success + wrong staging-reclaim)

Found in the 2026-07-08 prod audit. The CLI's Summary methods diverged from the
ingestor's own success/failure determination, so a partial run could report
success AND reclaim (delete) the staged source:

  • HasFailures() was FailedRecords>0 || FileTransferFailures>0 — it IGNORED
    skipped rows, inserted<total, and api_sent<inserted. The ingestor's
    IngestionSummary.has_failures (data-ingestors ingestors/base.py) counts all
    five. So a run that silently SKIPPED rows or inserted fewer than total was
    classified "succeeded" → exit 0 AND the staging-reclaim gate deleted the
    user's source copy. Now mirrors the ingestor exactly. Every counter it reads
    is emitted unconditionally by the ingestor banner (reporting.py) and parsed,
    so the new inserted<total / api_sent<inserted clauses can't false-positive on
    a clean run (all counters equal).

  • SuccessRate() was ProcessedRecords/TotalRecords; the ingestor's banner uses
    inserted/total. processed (passed validation) is a superset of inserted (rows
    actually in MySQL), so the CLI OVERSTATED success (e.g. 100 processed / 70
    inserted showed 100%, not 70%) in the panel header + --output-json
    success_rate. Now inserted/total, matching the banner.

  • RenderSummary headline: regraded to hard-failures (DB/transfer errors) → red,
    softer partial (skips / short insert-or-sync) → yellow, clean → green — so
    the broader HasFailures drives exit-code + reclaim while the headline still
    distinguishes a hard failure from a partial.

Behavior change (intended): a run with skipped rows, inserted<total, or
api_sent<inserted now exits 9 (was 0) and its staged source is KEPT (not
reclaimed) — matching what the ingestor already reports in its own banner.

Tests: HasFailures + SuccessRate matrices rewritten to the ingestor-aligned
semantics (incl. the processed-overstates case + one case per new clause);
RenderSummary headline cases + one classifyPushOutcome fixture updated to
realistic full-counter summaries. Full cli suite green.

Standalone CLI correctness fix (files untouched by the in-flight CLI branches).
Relates to the data-ingest epic backend#1008 / test-hardening backend#1009.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com


Note

High Risk
Changes when ingestion is considered successful and when staged source is reclaimed—incorrect classification could still cause data loss or leave staging when users expect cleanup; behavior intentionally shifts exit 0 → 9 for several real-world partial outcomes.

Overview
Aligns ingestion exit codes, staging reclaim, and reported success with the ingestor’s own failure rules so partial runs no longer look successful and delete the user’s staged source.

HasFailures() now matches ingestor has_failures: treats skipped rows, inserted < total, and api_sent < inserted as failures—not only DB/file-transfer errors. Those cases exit 9 and keep staged data instead of reclaiming on exit 0.

SuccessRate() uses inserted / total (not processed / total), so the panel and --output-jsonsuccess_rate no longer overstate success when rows validated but did not land in MySQL.

RenderSummary separates hard failures (DB/transfer) from softer partials: skips vs “partially” when shortfall isn’t from skips, while still driving non-clean outcomes through the broader HasFailures().

Tests update fixtures to full counter sets (including APISentRecords on “clean” rows) and expand matrices for the new semantics.

Reviewed by Cursor Bugbot for commit afdbef4. Bugbot is set up for automated code reviews on this repo. Configure here.

…cess + wrong staging-reclaim)
Found in the 2026-07-08 prod audit. The CLI's Summary methods diverged from the
ingestor's own success/failure determination, so a partial run could report
success AND reclaim (delete) the staged source:
- HasFailures() was `FailedRecords>0 || FileTransferFailures>0` — it IGNORED
skipped rows, inserted<total, and api_sent<inserted. The ingestor's
IngestionSummary.has_failures (data-ingestors ingestors/base.py) counts all
five. So a run that silently SKIPPED rows or inserted fewer than total was
classified "succeeded" → exit 0 AND the staging-reclaim gate deleted the
user's source copy. Now mirrors the ingestor exactly. Every counter it reads
is emitted unconditionally by the ingestor banner (reporting.py) and parsed,
so the new inserted<total / api_sent<inserted clauses can't false-positive on
a clean run (all counters equal).
- SuccessRate() was ProcessedRecords/TotalRecords; the ingestor's banner uses
inserted/total. processed (passed validation) is a superset of inserted (rows
actually in MySQL), so the CLI OVERSTATED success (e.g. 100 processed / 70
inserted showed 100%, not 70%) in the panel header + --output-json
`success_rate`. Now inserted/total, matching the banner.
- RenderSummary headline: regraded to hard-failures (DB/transfer errors) → red,
softer partial (skips / short insert-or-sync) → yellow, clean → green — so
the broader HasFailures drives exit-code + reclaim while the headline still
distinguishes a hard failure from a partial.
Behavior change (intended): a run with skipped rows, inserted<total, or
api_sent<inserted now exits 9 (was 0) and its staged source is KEPT (not
reclaimed) — matching what the ingestor already reports in its own banner.
Tests: HasFailures + SuccessRate matrices rewritten to the ingestor-aligned
semantics (incl. the processed-overstates case + one case per new clause);
RenderSummary headline cases + one classifyPushOutcome fixture updated to
realistic full-counter summaries. Full cli suite green.
Standalone CLI correctness fix (files untouched by the in-flight CLI branches).
Relates to the data-ingest epic backend#1008 / test-hardening backend#1009.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0b15162. Configure here.

Comment threadinternal/submit/summary.go Outdated
@LukasWodka
LukasWodka requested a review from saadqbalJuly 9, 2026 06:11
saadqbaland others added 4 commits July 9, 2026 13:40
…skips"
RenderSummary routed every non-hard-failure through the yellow
"completed with skips" headline, including runs where SkippedRecords==0
and the only shortfall was inserted<total or api_sent<inserted. Exit
code and staging-reclaim were correct, but operators could misread an
insert/API shortfall as a validator skip. Word the yellow headline by
the actual shortfall: "skips" only when rows were skipped, else
"partially". Adds a RenderSummary headline case for the zero-skip
partial. (Bugbot #193.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merging develop brought TestRunIngestionRun_Matrix (#1009), whose
succeededResult() fixture set only TotalRecords+InsertedRecords. Under
this PR's ingestor-aligned HasFailures(), api_sent(0) < inserted(2)
classifies that as completed_with_failures → exit 9, no reclaim,
breaking the "succeeded" row. Set APISentRecords so every stage counter
is equal — a clean run — matching the coverage_test.go "clean" fixture.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal
saadqbal merged commit 518c85a into developJul 9, 2026
16 checks passed
@saadqbal
saadqbal deleted the fix/summary-success-parity branch July 9, 2026 11:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saadqbal