fix(587): align raw_records to observe-filtered records in FILE mode#794
fix(587): align raw_records to observe-filtered records in FILE mode#794Muizzkolapo wants to merge 4 commits into
Conversation
A FILE-mode tool/HITL action whose context_scope.observe drops records for a missing upstream namespace passes the full pre-observe data as raw_records while the observe-filtered survivors are fewer. prefilter_by_guard then asserts len(original_data) == len(data) and crashes with a length-mismatch RuntimeError, failing the action and cascade-SKIPping every downstream action.
FILE-mode tool/HITL processing passed the full pre-observe data as raw_records while the observe-filtered records were fewer (observe drops records missing an upstream namespace — the shape of a merge between a guard-filtered source and an unfiltered one). prefilter_by_guard indexes original_data positionally and asserts len(original_data) == len(data), so the mismatch raised a length-mismatch RuntimeError, failing the action and cascade-SKIPping all dependents. Drop the observe-skipped source_guids from raw_records so it stays 1:1 with the filtered records. Mirrors the existing cascade-quarantine lockstep filter in UnifiedProcessor. source_guid is unique per FILE-mode downstream record.
…servability Code review of PR #794 found the guid set-diff alignment is fragile: merged/version records can carry source_guid=None (loop.py warns and produces them — it does not raise), so a None guid is filtered out of the skip set, its record stays in raw_records while it is absent from filtered, and prefilter_by_guard raises the same length-mismatch RuntimeError the PR was written to prevent. Root-cause fix: align by input INDEX instead of source_guid. apply_context_scope_for_records now tags each skipped record with its position (a field on the dict it already returns — no return-arity change, one production call site), and the pipeline drops exactly those indices from raw_records. Immune to None and duplicate guids. Also from the review: - raise_if_terminal_failure now receives the observe-filtered survivors, so the terminal-failure count reflects records actually processed, not the inflated pre-observe total. - All-observe-skipped now writes a node-level SKIPPED explicitly (the survivors path no longer routes through raise_if_terminal_failure's branch-1, which cannot fire on an empty list) so downstream still cascade-skips. - Observe-skips without a source_guid are logged instead of silently dropped from the disposition batch. - ARCHITECTURE.md updated to show raw_records=aligned_raw. Tests: None-guid alignment; real (non-mocked) prefilter_by_guard invariant; accurate failure count; node-level skip preserved; unkeyed-skip warning; no-drop fast path; HITL branch coverage.
Addressed all 7 findings from
|
|
Closing in favor of the root fix (spec 588). #794 fixed the symptom — the Shipping #794 alone would have papered over the crack — the framework would keep resurrecting dead records, re-deriving a filter decision it already made upstream, and wasting a merge + a processing pass. 588 fixes the cause: a per-record The alignment invariant #794 hardened will be re-evaluated inside 588's blast-radius work and folded in properly if a legitimate observe-drop path can still reach the crash — not shipped as a standalone patch. Branch |
1 participant
Summary
dataasraw_recordswhile the observe-filtered records (filtered) were fewer — becausecontext_scope.observedrops records missing an observed upstream namespace.UnifiedProcessorforwardsraw_recordstoprefilter_by_guard, which indexesoriginal_data[idx]positionally and assertslen(original_data) == len(data). The mismatch raisedRuntimeError("prefilter_by_guard received N original_data for M input records — length mismatch"), failing the action and cascade-SKIPping every dependent.ql_code_centereddedup_by_conceptmergestag_code_concept(guard-filtered to 34) withdedup_code_blocks(52) → 18 observe-dropped → 34 vs 52 → crash →code_usage_scenario,generate_code_caveats, … all SKIP.agent_actions/workflow/pipeline.py): drop the observe-skippedsource_guids fromraw_recordsso it stays positionally 1:1 withfiltered. Mirrors the existing cascade-quarantine lockstep filter (UnifiedProcessor,unified.py:172-182, also guid-based).Spec deviation
Spec 587 recommended the "robust" fix (change
apply_context_scope_for_recordsto return the aligned raw subset). Deviated to the minimal guid-filter at the caller: the arity change breaks ~58 unpack sites across 9 files, andsource_guidis a hard upstream invariant here —write_sourceraisesDataValidationErroron any record lacking one (sqlite_backend.py:524-529) andUNIQUE(relative_path, source_guid)blocks duplicates — so thesource_guid=None/dup gap the robust fix would close is structurally unreachable. Minimal fix = far smaller blast radius, same correctness.Verification
tests/unit/workflow/test_file_mode_observe_drop_raw_records.pydrives the real FILE-mode_process_by_strategybranch with 5 records where 2 lack the observed namespace, and captures theraw_recordsthe pipeline passes. Pre-fix it is the full 5-recorddata(≠ the 3 survivors) → fails; post-fix it equals the 3 survivors.ruff format --check,ruff check,mypy agent_actionsclean.pipeline.pyis the only prod caller passingraw_records; RECORD mode doesn't pass it; no batch equivalent (FILE-mode tool/HITL always run online). All FILE-mode sibling suites stay green.