Skip to content

fix(VIOL-0034): classify version-merge empties by cause, not file existence#771

Merged
Muizzkolapo merged 4 commits into
mainfrom
fix/viol-0034-classify-empties-by-cause
Jul 7, 2026
Merged

fix(VIOL-0034): classify version-merge empties by cause, not file existence#771
Muizzkolapo merged 4 commits into
mainfrom
fix/viol-0034-classify-empties-by-cause

Conversation

@Muizzkolapo

Copy link
Copy Markdown
Owner

Summary

Follow-up to the merged VIOL-0034 cascade-skip (#763). A review found the emptiness discriminator in resolve_correlated_input keys off list_target_files (file existence), which mis-scopes both directions:

  • A guard-filtered branch that wrote an empty output file ([]) has files but zero records — so it was read as "has output" and fell through to a ConfigurationError the executor does not catch, re-raising the exact crash fix(VIOL-0034): cascade-skip instead of crashing when all version branches filtered #763 set out to prevent.
  • Any all-empty state cascade-skipped and exited 0, masking sources that completed with zero output for non-filter reasons (missing data), which previously raised.
  • The discriminator's storage probes were unguarded — a backend error escaped as a raw sqlite3.Error/OSError.

Fix

Classify by cause, not file existence:

  • source has output records → correlation genuinely failed → ConfigurationError;
  • no records but guard-filtered (has_disposition(src, filtered)) → AllVersionsFilteredError → executor cascade-skips;
  • no records and not filtered → missing data → ConfigurationError naming the sources.

Record counts come from the already-guarded backend loader; the filtered-disposition probe is guarded, so a storage error surfaces a clean ConfigurationError rather than a raw crash.

Verification

  • Tests: empty-output-file skip (the crash variant), has-output failure, all-empty-not-filtered failure, guarded disposition-probe error. The correlator failure test now configures explicit output records instead of relying on incidental mock truthiness.
  • Non-tautological: reverting the record-count signal to file existence makes the empty-output-file test fail with the original crash.
  • 222 passed (managers + correlator + circuit-breaker); ruff + mypy clean.
  • Manifest documents the public AllVersionsFilteredError and its catch contract.

Follow-ups (lower severity)

Redundant per-source re-query; all_versions_filtered token vs sentence skip-reason taxonomy; shared _resolve_as_skipped(...) extraction.

…stence

resolve_correlated_input decided all-sources-empty from list_target_files
(file existence). Two failures followed:

- A guard-filtered branch that wrote an empty output file has files but zero
  records, so it was misread as 'has output' and fell through to a
  ConfigurationError the executor does not catch — the same crash this path
  exists to prevent.
- Any all-empty state cascade-skipped and exited 0, masking sources that
  completed with zero output for non-filter reasons (missing data), which
  previously raised.

Classify by cause instead: a source with output records means correlation
genuinely failed (raise); no records but guard-filtered means cascade-skip;
no records and not filtered means missing data (raise, naming the sources).
Record counts come from the already-guarded backend loader and the
filtered-disposition probe is guarded, so a storage error surfaces a clear
ConfigurationError rather than a raw backend crash.

Tests cover the empty-output-file skip, has-output failure, all-empty-not-
filtered failure, and the guarded disposition-probe error. The correlator
failure test now configures explicit output records instead of relying on
incidental mock truthiness. Manifest notes the public exception.
@Muizzkolapo

Copy link
Copy Markdown
Owner Author

Code review

Found 1 issue:

  1. _was_guard_filtered probes has_disposition(action_name, DISPOSITION_FILTERED) without a record_id, so it returns True if any single record of a source was guard-filtered — not whether the whole source was filtered. DISPOSITION_FILTERED is only ever written per-record (result_collector.py:683, processing.py:731); the node-level marker for "all records filtered, no output" is DISPOSITION_SKIPPED (result_collector.py:116-120). So a source that filtered some records but produced no output for the rest (e.g. crashed mid-run after writing per-record FILTERED dispositions) is classified as "guard-filtered" → AllVersionsFilteredError → cascade-skip, silently masking the missing data. That is the exact misclassification this PR sets out to eliminate ("masking sources that completed with zero output for non-filter reasons"). The added tests don't catch it because the _backend mock's has_disposition keys off whole-source membership (action_name in filtered_set), not per-record dispositions. The established pattern in this codebase checks node-level terminal dispositions with record_id=NODE_LEVEL_RECORD_ID (executor.py:313); this probe diverges from it.

def _was_guard_filtered(self, action_name: str) -> bool:
try:
return self.storage_backend.has_disposition(action_name, DISPOSITION_FILTERED)
except (OSError, sqlite3.Error) as e:
logger.warning(
"Failed to check filtered dispositions for %s: %s",
action_name,

Context — established node-level pattern:

for disp in (
DISPOSITION_FILTERED,
DISPOSITION_PASSTHROUGH,
DISPOSITION_SUCCESS,
DISPOSITION_UNPROCESSED,
):
if storage_backend.has_disposition(action_name, disp, record_id=NODE_LEVEL_RECORD_ID):
logger.info(

Context — what is actually written when all records are filtered (node-level SKIPPED, not FILTERED):

write_node_level_disposition(
storage_backend,
action_name,
DISPOSITION_SKIPPED,
"All records filtered — no output produced",
)

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

…ecord filter

The all-filtered probe checked has_disposition(src, filtered) with no
record_id, so it returned true if any single record was filtered. A source
that filtered some records then produced nothing for the rest (e.g. crashed
mid-run) was read as fully filtered and cascade-skipped — the same masking
this change exists to prevent.

The node-level marker for 'all records filtered, no output' is a SKIPPED
disposition at the node record id (written when only-guard-outcomes and no
output). Probe that instead, matching how the executor checks node-level
terminal dispositions. A source with per-record filters but no node marker
now surfaces as missing data.

Test backend now models node-level skip markers and per-record filters
distinctly; adds a regression for per-record-filtered-without-node-skip.
@Muizzkolapo

Copy link
Copy Markdown
Owner Author

Good catch — fixed in 68189417455d.

The probe was has_disposition(src, filtered) with no record_id, so it fired on any single per-record filter. Switched it to the node-level marker: has_disposition(src, DISPOSITION_SKIPPED, record_id=NODE_LEVEL_RECORD_ID) — the disposition result_collector writes only when only_guard_outcomes and not output (all records filtered, nothing produced), matching how the executor checks node-level terminal dispositions.

A source that filtered some records then produced nothing for the rest (crash mid-run, no node marker) now falls through to the missing-data ConfigurationError instead of cascade-skipping.

The test backend previously keyed has_disposition off whole-source membership regardless of disposition/record_id; it now models node-level skip markers and per-record filters distinctly, and there's a new regression — test_per_record_filtered_without_node_skip_raises_not_masks — which fails against the old per-record probe (verified: it masked with AllVersionsFilteredError) and passes with the node-level check.

When the node-level skip probe hit a storage error it returned False, so the
source fell into the missing-data branch and raised an error blaming the
user's agents for a storage-layer fault. Raise a storage-specific
ConfigurationError from the probe instead — still a clean, loud error rather
than a raw backend crash, but one that names the actual cause.
@Muizzkolapo

Copy link
Copy Markdown
Owner Author

Addressed the storage-error message finding in a1c50de9afbc. _was_skipped_without_output now raises a storage-specific ConfigurationError ("Could not read the skip state … from the storage backend: …") instead of returning False and falling into the missing-data branch that blamed the user's agents. Still a clean loud error, not a raw crash; the probe-error test now asserts the storage-specific message and that it isn't the missing-data one. The pre-existing _load_outputs_from_backend swallow is unchanged (out of scope).

…ecord count

The marker-based classifier reintroduced the crash it targeted. A fully
guard-filtered branch writes an empty output file (record_count=0), which
list_target_files returns, so the executor clears the node-level SKIPPED
marker as stale. By correlation time no marker survives, so the consumer
raised ConfigurationError instead of cascade-skipping — verified end to end
against a real SQLite backend.

Move the decision to where the truth already lives. prepare_correlated_input
loads the actual version-source records; when none exist it now raises
AllVersionsFilteredError (the executor cascade-skips), and a correlation or
storage fault raises a clean ConfigurationError instead of collapsing every
outcome to None. resolve_correlated_input delegates and propagates — the
fragile marker/record re-derivation and its helpers are deleted.

Classification is by record count (real data), independent of markers upstream
code clears and of the storage backend. A real-backend integration test covers
the empty-output-file cascade-skip, a records-present correlation, and a
storage fault; correlator/handoff tests move to the raise contract.
@Muizzkolapo

Copy link
Copy Markdown
Owner Author

The 51-agent review was right — the marker approach reintroduced the crash. Reworked in 600868fb770c.

Confirmed finding #1 empirically (real SQLite backend, not mocks): a fully guard-filtered branch writes an empty output file with record_count=0; list_target_files returns it; _resolve_completion_status (executor.py:718) then clears the node-level SKIPPED marker as "stale." By correlation time no marker survives, so the consumer raised ConfigurationError — the exact VIOL-0034 crash. My marker test was a false green because it mocked has_disposition and never exercised the clearing path.

Root cause (finding #10): prepare_correlated_input collapsed "all sources empty" and "correlation errored" into a bare None, forcing the caller to re-derive the reason from state upstream mutates.

Fix: move the decision to where the truth lives. prepare_correlated_input now raises AllVersionsFilteredError when not version_outputs (every source produced zero records — real data, no marker, backend-agnostic) and a clean ConfigurationError on correlation/storage faults. resolve_correlated_input delegates and propagates; the fragile marker/record re-derivation and its helpers are deleted.

This also resolves the cluster: #2 passthrough-only (zero correlatable records → cascade-skip via record count), #3 storage faults now surface loudly and cleanly, #5 no has_disposition dependency (backend-agnostic), #6 broad fault translation (a closed backend's RuntimeErrorConfigurationError, while AllVersionsFilteredError/framework errors pass through), #7 the boolean-predicate-that-raises helper is gone.

Verification: a real-backend integration test now covers the empty-output-file cascade-skip (reproduces the crash when reverted), a records-present correlation, and a storage fault. Correlator/handoff tests moved to the raise contract. Full suite: 7848 passed; ruff + mypy clean.

@Muizzkolapo Muizzkolapo merged commit 977d45b into main Jul 7, 2026
5 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant