fix(588): guard filter is authoritative across the DAG — no resurrection via unfiltered sibling#796
Merged
Merged
Conversation
…t fan-in A guard `on_false: filter` means a record is dead and cannot be carried forward (guards/ARCHITECTURE.md). But the FILE-mode storage fan-in (process_from_storage_backend) unions dependency outputs by source_guid, so a record one dependency filtered re-enters a downstream action through an unfiltered sibling dependency that still carries it. Real hit: tag_code_concept guard-filters 18 syllabus-rejected blocks, yet dedup_by_concept (which also depends on the unfiltered dedup_code_blocks) receives all 52 — the 18 dead records resurrect, get re-dropped by observe, and were the upstream cause of the #794 crash. Fix (read-side, no new persistence): a per-record FILTERED disposition is already written for every guard-filtered record (result_collector + unified.py). At fan-in, collect the FILTERED source_guids of each upstream dependency and subtract them from the assembled records before they reach the action. Applies to both the merge branch and the single-source branch; node-level (__node__) FILTERED markers are rerun signals, not record ids, and are ignored. A disposition marker is not a target/ output row, so filter's "no output row" contract still holds. Verification of the live ql_code_centered run is blocked upstream: the current run cascade-skips from flatten_code (see spec 586), so tag_code_concept never reaches its filter — no store holds a per-record FILTERED disposition yet. The write path is confirmed in code (unified.py:254-256, 315-317 → result_collector.py:678). Covered by unit tests against the real fan-in.
Blind review (YES_WITH_ISSUES) findings, resolved: - MED transitive resurrection: a guid filtered by a GRANDPARENT (not a direct dep) could re-enter via an unfiltered branch. Scope the subtraction to the action's transitive ancestors from the stored dependency_graph; fall back to DIRECT deps only (never the flat execution order, which would include parallel peers and risk dropping legitimately-live records). - LOW fail-open: the disposition read is a safety net, so a storage error now degrades to no-subtraction (warns) instead of aborting the action — matching the surrounding read handlers. - LOW note: documented that the filesystem fan-in (process_merged_files) omits the subtraction and is unreachable while dispositions exist. New tests: transitive/grandparent authority; record without source_guid is kept (no silent data loss); get_disposition error fails open.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Guard
on_false: filteris documented (guards/ARCHITECTURE.md) to mean a record is excluded entirely — cannot be carried forward (dead), as opposed toskipwhich carries the record forward. This PR makes that contract hold across the DAG.The bug: a guard-filtered record re-entered a downstream action through an unfiltered sibling dependency. The FILE-mode storage fan-in (
process_from_storage_backend) unions every dependency's output bysource_guid; a guid present only in the unfiltered sibling passed straight through. Filter leaves notarget/output row and is not a cascade-blocking state, so nothing suppressed it.Real hit (
ql_code_centered):tag_code_conceptguard-filters 18 syllabus-rejected blocks (filter_by_syllabus.is_syllabus_aligned == true, on_false: filter), butdedup_by_concept— which also depends on the unfiltereddedup_code_blocks— received all 52. The 18 dead records resurrected, got re-dropped byobserve, and were the upstream cause of theprefilter_by_guardlength-mismatch crash (previously patched in the now-closed #794).Fix
Read-side, no new persistence. A per-record
FILTEREDdisposition is already written for every guard-filtered record (unified.py:254-256/315-317→result_collector.py:678). At fan-in,process_from_storage_backendnow:FILTEREDsource_guids of each upstream dependency (get_disposition(dep, disposition="filtered")), once per action;__node__) FILTERED markers (rerun signals, not record ids).A disposition-table marker is not a
target/output row, so filter's "no output row" contract is preserved.Verification
tests/unit/workflow/test_filter_authoritative_fanin.py): drives the realprocess_from_storage_backend. Pre-fix, a guid filtered by one dependency resurrects via the unfiltered sibling (both merge and single-source branches) → fails. Post-fix → dropped. Plus regressions: no-filter union preserved; node-level marker ignored.ruff check+ruff format --check+mypy agent_actionsclean.Review
Blind fresh-context reviewer (LOW tier): YES_WITH_ISSUES — "core fix correct, minimal, right chokepoint; mutation-verified." Three findings, all resolved in
58b1eb14:dependency_graph(falling back to direct deps only — never the flat execution order, which would over-subtract parallel peers). Covered bytest_transitive_ancestor_filter_is_authoritative.test_get_disposition_error_fails_open.process_merged_filesomits the subtraction and is unreachable while dispositions exist.Smoke: skipped — LOW tier, no chokepoint/registry/provider surface (read-side fan-in subtraction only).
Deviations / follow-ups (named per harness)
dependency_graphis stored (legacy workflows), the collector falls back to direct dependencies only — deliberately not the flat execution order, which would treat parallel peers as ancestors and could drop legitimately-live records. So in legacy-metadata workflows the grandparent case degrades to direct-dep scope (conservative; no data loss).merge_json_files/process_merged_files) has no storage backend and thus no dispositions to consult; it is unreachable while dispositions exist (documented in-code). The incident is storage-backed.ql_code_centeredrun cascade-skips atflatten_code, so it never reachestag_code_concept's filter and no store yet holds a per-record FILTERED disposition. The write path is confirmed in code; unit tests exercise the real fan-in. Re-run once 586 lands to confirmdedup_by_conceptreceives 34, not 52.Supersedes the symptom fix in #794 (closed): #794 stopped the crash; this removes the cause.