Skip to content

fix(mt#1444): Fix sessionsErrored always-zero in AgentTranscriptIngestService.ingestAll#910

Merged
edobry merged 1 commit into
mainfrom
task/mt-1444
Apr 30, 2026
Merged

fix(mt#1444): Fix sessionsErrored always-zero in AgentTranscriptIngestService.ingestAll#910
edobry merged 1 commit into
mainfrom
task/mt-1444

Conversation

@minsky-ai
Copy link
Copy Markdown
Contributor

@minsky-ai minsky-ai Bot commented Apr 30, 2026

Summary

Fixes mt#1444: IngestAllResult.sessionsErrored was structurally always 0 because ingestSession swallowed all three failure paths (HWM read, stream, upsert) and returned 0, so the outer try/catch in ingestAll never fired. Adopts spec option (A): change ingestSession to return a typed { ingested: number; error?: Error } so swallowed failures surface to the sweep.

Motivation and Context

Reviewer-bot flagged this on PR #883 (review). The existing "DB error isolation" test explicitly asserted sessionsErrored === 0 with a comment acknowledging the swallowing — meaning callers checking sessionsErrored > 0 to detect failures would always see zero even when every session failed silently. This is a correctness bug for any monitoring/alerting layer downstream of the ingest sweep.

Design

The spec offered two options. Option (A) preserves error visibility without breaking sweep semantics — ingestSession still recovers and continues; the swallowed error just surfaces via the return value. The error field is optional and is set only when a swallow path was hit; success returns { ingested: N } with no error.

HWM-read failure semantics: even when the recovery path proceeds (we read the HWM as null and may successfully upsert), the error is surfaced. Rationale documented in code: without an HWM, the next ingest would re-collect already-ingested lines (the upsert appends via JSONB-array-concat), so a recovered-from HWM error is a degraded state worth counting.

Key Changes

  • src/domain/transcripts/agent-transcript-ingest-service.ts:
    • New IngestSessionResult interface: { ingested: number; error?: Error }
    • ingestSession return type changed from Promise<number> to Promise<IngestSessionResult>
    • HWM-read, stream, and upsert paths now construct an Error (preserving non-Error throws) and return it via the result
    • ingestAll increments sessionsErrored on result.error !== undefined; the defensive outer catch remains for unexpected throws
  • src/adapters/shared/commands/transcripts.ts: single call site updated to read result.ingested and propagate sessionsErrored: result.error ? 1 : 0 to the MCP/CLI response

Spec criteria mapping

Criterion Evidence
ingestSession returns a typed result distinguishing success from caught failure IngestSessionResult interface; signature change
ingestAll increments sessionsErrored for every swallowed failure (HWM, stream, upsert) ingestAll checks result.error !== undefined
Existing "DB error isolation" test asserts sessionsErrored === 1 Updated test (line ~377)
No new dependencies, no API breakage outside the result shape Only the return shape changed; same exports
validate_typecheck and validate_lint clean Pre-commit hook reports 0 errors / 0 warnings
Acceptance test: upsert failure on 1-of-3 → sessionsErrored = 1, totalIngested = 2 sessions worth New test "upsert failure increments sessionsErrored for that session"
Acceptance test: HWM-read failure counts in sessionsErrored New test "HWM-read failure is surfaced via the typed result and counted"

Testing

11/11 tests pass on agent-transcript-ingest-service.test.ts (was 8 — added 3 new acceptance tests, kept all existing assertions strict).

Out of Scope

  • Real-DB concurrency tests (mt#1419's acceptance work).
  • Surfacing partial-success metadata in the MCP/CLI tool output beyond the existing sessionsErrored count.

🤖 Generated with Claude Code

…sult

Reviewer-bot flagged on PR #883 that IngestAllResult.sessionsErrored was
structurally always zero: ingestSession swallowed failures along three
paths (HWM read, stream, upsert) and returned 0, so the outer try/catch
in ingestAll never fired. Callers checking sessionsErrored greater than
zero would always see zero even when every session failed silently.

Adopts spec option (A): change the return shape to a typed
IngestSessionResult { ingested: number; error?: Error }. ingestSession
still recovers and continues — but the swallowed error surfaces in the
return value so ingestAll can count it honestly.

Tests:
- The existing DB error isolation test now asserts sessionsErrored === 1.
- New test: upsert failure on one of three sessions counts exactly one.
- New test: HWM-read failure surfaces and is counted, even when the
  recovery path proceeded (degraded state worth tracking — without HWM,
  next ingest would re-collect already-ingested lines).
- All ingestSession callers updated (3 in adapters, 6 in tests) to use
  the new typed shape.

11 of 11 tests pass; typecheck 0 errors, lint 0 warnings.
@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Apr 30, 2026
Copy link
Copy Markdown

@minsky-reviewer minsky-reviewer Bot left a comment

Choose a reason for hiding this comment

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

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown


The reviewer ran but produced no findings. This is not an approval — the model emitted no submit_finding, submit_inline_comment, or conclude_review calls.

@edobry edobry merged commit e8112ca into main Apr 30, 2026
2 checks passed
@edobry edobry deleted the task/mt-1444 branch April 30, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant