fix(gfql): OPTIONAL MATCH + WITH pipeline row semantics (#1896) by lmeyerov · Pull Request #1897 · graphistry/pygraphistry

lmeyerov · 2026-08-14T10:33:08Z

Stacked on #1895. Fixes all four silent-wrong shapes from the #1896 re-probe umbrella:

CRITICAL guard bypass — terminal WITH after OM rode the single-alias row pipeline (pandas: nulled+collapsed; polars: fabricated seed ids into the optional column). New flatten_terminal_with_over_optional routes pure bare-alias carries onto the connected left-join lowering with the stage WHERE as a post-join binding-row filter — exact openCypher row semantics, which also makes the canonical WITH p, x WHERE x IS NULL anti-join answer correctly on both engines.
Zero-count groups — passthrough WITH-aggregates fold to direct RETURN aggregates (null-keeping group keys; count(b)=0 preserved).
3+4. Anonymous null fill — apply_optional_reentry_null_fill count-arithmetic replaced by identity-carrying anti-join of unmatched prefix rows (rename + LIMIT carries fixed); no-identity shapes decline typed, never anonymous rows.
ORDER BY decline messages now hint the working output-alias spelling.

21 new green pins (section F, hand-computed oracles, both engines) + 2 decline-shape negative controls. Gates: OM 81P, divergences 11P/3xf, hop 42P, flatten 25P, lowering 1420P (0 new failures), polars 587P, TCK 4145P/0F, ruff+guards green (surface baseline regen for intentional lowering growth 9454→9503).

🤖 Generated with Claude Code

https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

Four silent-wrong shapes from the round-004-precursor adversarial re-probe: 1 (CRITICAL): terminal WITH after OPTIONAL MATCH rode the single-alias row-column pipeline -- pandas nulled matched bindings and collapsed multiplicity, polars fabricated bid=seed-ids. flatten_terminal_with_over_ optional now rewrites pure bare-alias carries onto the connected left-join lowering, with the stage WHERE applied as a post-join binding-ROW filter (openCypher WITH..WHERE semantics; serves seed AND optional-alias predicates -- the canonical `WHERE x IS NULL` anti-join now answers). Terminal projection/aggregate stages passed through by RETURN fold into a direct RETURN (2: zero-count groups now kept -- count(b)=0 / count(*)=1 for unmatched seeds). Whole-row-next-to-aggregate stages keep their typed decline; any other unflattenable OM WITH stage now declines typed instead of riding the pipeline into silent-wrong. 3+4 (one root): the reentry null-fill replaced `prefix_rows - result_rows` anonymous null templates with an identity anti-join over projected carried-alias columns (bare props, renamed WITH scalars incl. their __cypher_reentry_ markers) -- rename (`WITH a AS p`) and LIMIT carries now null-extend with their seed identity; a multiplicity surplus no longer masks missing null rows. Shapes that project no identifying carried column decline typed (prefix_rows==1 count logic stays, it is sound). 5: the ORDER BY optional-property decline now hints the output-alias spelling that sorts with openCypher null placement. Pins: section F in test_optional_match_semantics.py -- pandas+polars hand-computed oracles for every finding, plus decline-shape negative controls; the anti-join message-audit pin flips to a served-answer pin. Surface-guard baseline regenerated: lowering.py 9454 -> 9503 (flatten hook + typed guard + post-join filter). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-14T10:40:29Z

GPU sweep receipt (dgx GB10, RAPIDS 26.02, --gpus-enforced, @ d502dea): full graphistry/tests/compute/gfql tree — 8820 passed, 53 skipped, 22 xfailed, 0 failed (4m40s). First zero-failure GPU sweep of the release campaign; prior tip was 1 failed / 8773 passed. Sanity probes (cudf sum + string to_pandas, polars GPUEngine raise_on_fail collect) green. Minor follow-up noted, not a failure: pandas FutureWarning on all-NA concat at reentry/execution.py:180 in the null-fill path.

Same coverage-floor playbook as flatten_pure_carry_optional: the new #1896 flatten shipped with only end-to-end coverage; 20 direct AST pins cover the admit paths and every decline branch (flatten.py floor 93.60% -> above 94.05%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-15T15:59:05Z

+    if fill_rows is None and carried_output_map:
+        # #1896: never synthesize ANONYMOUS null rows by count arithmetic --
+        # anti-join unmatched prefix rows so the null-extension keeps its
+        # carried seed identity, or decline typed below.


Violates comment rules too

lmeyerov · 2026-08-15T15:59:35Z

+    *,
+    prefix_df: DataFrameT,
+    result_df: Optional[DataFrameT],
+    null_row: Dict[str, Any],  # hygiene-ok: explicit-any -- heterogeneous Cypher fill values, mirrors _optional_reentry_carried_null_rows


Avoid Any when narrow available

lmeyerov · 2026-08-15T16:00:11Z

+
+    Disqualified shapes return None and stay on the (typed-decline) pipeline:
+    renames/DISTINCT/ORDER/SKIP/LIMIT on the stage, multiple stages, UNWIND/
+    CALL, references to non-carried aliases after a subset carry.


More comment issues

…ping (#1897 review) Owner inline review flagged three sites (reentry/execution.py:145 comment, reentry/execution.py:189 `Any` + hygiene-ok pin, reentry/flatten.py:311 docstring). The review skill's self-review gate was then run over the whole PR diff and every hit given a verdict. Names and structure instead of prose: reentry/flatten.py flatten_terminal_with_over_optional keeps a two-line docstring; its 20-line narration is now the helper names _is_terminal_with_over_optional_match, _stage_reshapes_rows, _match_clause_aliases, _query_without_pure_carry_stage, _query_with_terminal_stage_folded_into_return, _stage_has_aggregates. _pure_carry_aliases_ignoring_where was a copy of _pure_carry_aliases minus one guard; both now share _bare_carry_aliases + _stage_reshapes_rows. reentry/execution.py the carried_output_map tri-state (None declines / {} nothing to do / map fills) is a frozen CarriedOutputSources plus CARRIED_OUTPUTS_NOT_REPRODUCIBLE; the "never synthesize anonymous null rows" comment is the helper name _optional_reentry_unmatched_identity_null_rows. gfql_unified.py _carried_output_sources / _carried_output_source_column / _output_reads_carried_alias replace the inline branch cascade and its trailing comments. cypher/lowering.py both added comment blocks and the post_join_row_filter docstring paragraph deleted; the parameter name states the rule. Typing (aliases byte-identical to the #1894 review so the branches merge cleanly): cypher/ast.py CypherScalar = Union[None, bool, int, float, str]; CypherLiteral re-expressed over it agg_types.py CypherEmptyGroupValue / CypherEmptyGroupFills execution.py CypherFillValue / CypherFillRow Both `# hygiene-ok: explicit-any` pins this PR had introduced are removed, not suppressed. Ratchets locked down: explicit-any reentry/execution.py 9 -> 0, gfql_unified.py 21 -> 20 (4496 -> 4486 total); cypher surface lowering_py_max_lines 9503 -> 9493. Pins added where a deleted comment was the only record of a contract: test_flatten_terminal_with.py::test_admitted_query_never_retains_a_with_stage_so_recompiling_it_terminates test_optional_reentry_carried_outputs.py (all seven source-column branches, including the reentry-marker branch that had only a trailing comment) test_optional_match_semantics.py::test_optional_match_anti_join_with_where_x_is_null_keeps_only_unmatched_rows Parametrized decline cases carry pytest ids instead of `#` labels. No behavior change: graphistry/tests/compute/gfql failure set byte-identical to the PR base (93 failed, md5 d68f3d7cddf5bab5d11588e04727d362), test_compute_chain/test_compute_hops identical (2 pre-existing cudf-lane failures). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-15T16:41:07Z

Review addressed, with the self-review gate run over the whole PR diff rather than just the flagged lines.

The three sites

execution.py:145 — the 3-line # #1896: never synthesize ANONYMOUS null rows… was narration and why-a-fix-was-made, so a BLOCKER twice over. Deleted. The rule now lives in the helper name _optional_reentry_unmatched_identity_null_rows and in the type: the Optional[Mapping[str,str]] tri-state (None = decline / {} = nothing to do / map = fill) became a frozen CarriedOutputSources plus CARRIED_OUTPUTS_NOT_REPRODUCIBLE, which also retired a 4-line parameter docstring.
execution.py:189 — Dict[str, Any] with two hygiene-ok pins on brand-new code. Both pins removed, not suppressed; real aliases added (CypherScalar, CypherEmptyGroupValue, CypherFillValue, CypherFillRow), and five sibling helpers in the file narrowed with them. fix(gfql)!: OPTIONAL MATCH null-extension semantics (#1891) #1894's commit is not on this branch, so its aliases were copied byte-identically so the two merge cleanly.
flatten.py:311 — a 20-line docstring narrating the algorithm and re-listing guards the code already states. Now 2 lines plus six named helpers (_is_terminal_with_over_optional_match, _stage_reshapes_rows, _query_without_pure_carry_stage, _query_with_terminal_stage_folded_into_return, …). Also DRY'd a copy-paste: _pure_carry_aliases_ignoring_where was _pure_carry_aliases minus one guard; both now share _bare_carry_aliases.

Self-review gate, whole diff

18 sites given verdicts — including 12 # labels inside parametrize converted to pytest.param(id=…) and 3 per-test explanations folded into test names (e.g. test_declines_pure_carry_when_order_by_references_an_uncarried_alias). Afterwards grep '^\+\s*#' returns only the deliberate keeps, and grep '^\+.*(Any|type: ignore|hygiene-ok|cast\()' returns nothing.

Kept, with reasons: #: Sphinx doc-comments on the six type aliases (each states an openCypher spec fact no signature can express — the form accepted on #1894); the TYPE_CHECKING-only import-cycle note in agg_types.py, kept byte-identical to #1894's so the branches merge without conflict; and the test-file section banner matching the existing A–E convention.

Pins added where a comment was the only record

Deleting a comment without an equivalent pin is information loss, so three went in:

test_admitted_query_never_retains_a_with_stage_so_recompiling_it_terminates — 4 admitted shapes, asserts with_stages == () and that re-flattening declines (the recursion-termination claim the deleted comment made).
New test_optional_reentry_carried_outputs.py — 7 named params covering every branch of the carried-output mapping, including p.__cypher_reentry_av__ and the uncarried-marker fallthrough, which had no pin at all before.
test_optional_match_anti_join_with_where_x_is_null_keeps_only_unmatched_rows, with the hand-computed oracle in its docstring.

Gates

Failure list md5 d68f3d7cddf5bab5d11588e04727d362 — identical across base, PR head, and post-fix. Targeted suites 1538 passed; cypher dir 7 failed (all cudf on a GPU-less box, inside the identical 93); chain+hops byte-identical to base. ruff clean; mypy 328 files clean.

Hygiene improved and ratcheted down, not held flat: explicit-any in reentry/execution.py 9 → 0 (baseline entry deleted) and gfql_unified.py 21 → 20; total grandfathered 4496 → 4486. The cypher-surface baseline also ratcheted 9503 → 9493 as lowering.py shrank.

One judgement call flagged for you: that surface ratchet tightens within the bump this PR itself made (9454 → 9503 → 9493) rather than leaving 10 lines of slack.

lmeyerov · 2026-08-15T17:00:22Z

-        fill_rows = [dict(null_row) for _ in range(missing_count)]
-    elif not fill_rows:
+        if not carried_outputs.every_output_reproducible:
+            raise reentry_validation_error(


tck-gfql obligations likely, audit & handle as needed

lmeyerov · 2026-08-15T17:01:56Z

@@ -0,0 +1,150 @@
+"""Direct tests for terminal-WITH-over-OPTIONAL flattening (#1896).


shouldn't these be more like cypher/reentry/test_flatten.py instead of toplevel?

lmeyerov · 2026-08-15T17:02:25Z

+    return replace(query, with_stages=()), stage.where
+
+
+def _query_with_terminal_stage_folded_into_return(


seems significant positive/negative test obligations

lmeyerov · 2026-08-15T17:02:47Z


-CypherLiteral = Union[None, bool, int, float, str, ParameterRef]
+#: Every value openCypher can write literally: the four primitive types plus null.
+CypherScalar = Union[None, bool, int, float, str]


is this from lower on our pr stack?

lmeyerov · 2026-08-15T17:03:09Z

        )
        if compiled_connected_optional is not None:
            return _attach_graph_context(compiled_connected_optional)
+    if query.with_stages and any(m.optional for m in query.matches):


more significant pos/neg test amplifications obligations

lmeyerov · 2026-08-15T17:03:57Z


+    # TYPE_CHECKING only: `gfql.cypher.__init__` eagerly imports the whole compiler, and
+    # this module is imported from inside that import (row.pipeline), so a runtime import
+    # would be circular. The aliases below only need the name at type-check time.


avoid unnecessary and overly verbote comments... root cause why this happened, and both fix here, but all the root cause of why authoring & reviewing let this through

lmeyerov · 2026-08-15T17:04:51Z


-def _optional_reentry_aggregate_fill_values(compiled_query: CompiledCypherQuery) -> Dict[str, Any]:
+_IDENTIFIER_TOKEN = re.compile(r"[A-Za-z_][A-Za-z0-9_]*")
+_REENTRY_MARKER_COLUMN = re.compile(r"__cypher_reentry_(\w+)__")


strange location for these symbols, shouldn't they be externed for reuse by others?

lmeyerov · 2026-08-15T17:05:16Z

+    return None
+
+
+def _carried_output_sources(compiled_query: CompiledCypherQuery) -> CarriedOutputSources:


gfql_unified.py is already overly large..

lmeyerov · 2026-08-15T17:05:43Z

+    ops = list(compiled_query.chain.chain) if compiled_query.chain is not None else []
+    for op in ops:
+        function = getattr(op, "function", None)
+        op_params = getattr(op, "params", None) or {}


try to replace dynamic typing pattersn like getattr with more structured & static programming

lmeyerov · 2026-08-15T17:06:29Z

+    return None
+
+
+def _carried_output_sources(compiled_query: CompiledCypherQuery) -> CarriedOutputSources:


test amplification concerns?

lmeyerov · 2026-08-15T17:07:45Z

Answering the factual question first, then the rest is dispatched.

"is this from lower on our pr stack?" (ast.py:30) — Yes, and deliberately byte-identical. CypherScalar and the CypherLiteral re-expression are the same lines #1894 introduced (verified: git show on both branches returns identical text). #1894's commit is not an ancestor of this branch, so rather than invent a parallel alias the lane copied it verbatim so the two merge without conflict. Same for the TYPE_CHECKING note in agg_types.py. That is also why you are seeing the same agg_types.py:55 comment on both #1894 and here — one duplication, two PRs. It is being deleted in both, not just here.

"gfql_unified.py is already overly large" — confirmed and now tracked. It is 2,526 lines; lowering.py is 9,493 (#1921). Both are the same problem, and I have added gfql_unified.py to that refactor issue rather than opening a competing one. Note the immediate consequence for this PR: your point about the fill-value symbols being in a "strange location" and the file being oversized are the same observation — those symbols belong in an importable home (gfql/agg_types.py already exists for exactly this), not inline here.

Dispatched for fixing, grouped:

Encoding: agg_types.py:55 verbose comment (with the root-cause you asked for — see docs(review): encode the rejection categories the GFQL stack kept hitting #1924, which now carries seven causes and the corrected keep bar: "a justified keep is still a keep; the test is whether a name or a test can carry it").
Structure: extern the fill-value symbols for reuse; test file moved to mirror the source tree (cypher/reentry/test_flatten.py rather than top-level) — you are right that a test's location should follow the module it pins.
Typing: the remaining getattr probes replaced with static dispatch on the AST types.
Testing obligations (three separate comments — flatten.py:367, lowering.py:9155, gfql_unified.py:1508): a positive/negative matrix across both engines for the flatten admit/decline branches and the terminal-WITH folding, with hand-computed oracles and an anti-vacuity count at this PR's base.
tck-gfql obligations (execution.py:171): audited — this PR's branch has a companion tck-gfql branch carrying the promotion cascade, and I will report explicitly whether any scenario's outcome changes and whether the contract/manifest needs regenerating, rather than assuming it does not.

Also worth flagging from your earlier note, since it applies here: the surface-guard ratchet on this PR tightened within its own bump (9454 → 9503 → 9493), leaving no slack. Say the word if you would rather it left headroom.

lmeyerov · 2026-08-15T17:20:21Z

+    """Prefix-frame column behind each result output that reads the carried alias."""
+
+    columns: Mapping[str, str]
+    every_output_reproducible: bool


are any of these exernally needed types?

lmeyerov · 2026-08-15T17:20:56Z

+_IDENT_TOKEN = re.compile(r"[A-Za-z_][A-Za-z0-9_]*")
+_AGGREGATE_CALL = re.compile(
+    r"\b(count|sum|avg|min|max|collect|stdev|percentile\w*)\s*\(", re.IGNORECASE
+)


should these be externed and/or swapped to predicates?

lmeyerov · 2026-08-15T17:21:56Z

+#: What an aggregate answers for an EMPTY group, per the Cypher contract documented above:
+#: ``count``/``sum`` -> ``0``, ``count(*)`` over a synthesized null-extended row -> ``1``,
+#: ``collect`` -> ``[]``. Every other aggregate's empty-group answer is ``null`` and is
+#: therefore absent from a fill map rather than present as a value.


overly verbose..

Cascading base update after #1895 landed its review remediation. Resolutions: - graphistry/compute/gfql/agg_types.py: BOTH sides independently introduced an empty-group-aggregate contract (diff3 merge base empty). They are the SAME concept, so reconciled rather than unioned blindly: kept the base's shorter `#:` line for CypherEmptyGroupValue / CypherEmptyGroupFills and the base's CYPHER_ZERO_EMPTY_GROUP_AGGREGATIONS / CYPHER_EMPTY_LIST_EMPTY_GROUP_AGGREGATIONS frozensets, which the (cleanly merged) body of _optional_reentry_aggregate_fill_values already reads. Dropped this branch's 3-line TYPE_CHECKING circular-import narration; the import position already states it. - graphistry/compute/gfql_unified.py: import block took the base (superset: the two frozensets plus CypherParams, both used in the merged file). The _optional_reentry_aggregate_fill_values hunk is NOT a rival implementation -- the base only renamed its return type Dict[str, Any] -> Dict[str, CypherEmptyGroupValue], which this branch did too. Kept the base's signature line and this branch's genuinely-new carried-output helpers above it (_IDENTIFIER_TOKEN, _REENTRY_MARKER_COLUMN, _output_reads_carried_alias, _carried_output_source_column, _carried_output_sources), which are a different feature (#1896 carried-output reproducibility) that only shares the file. - graphistry/compute/gfql/cypher/reentry/execution.py: unioned the imports (this branch's CypherEmptyGroupValue/CypherScalar for CypherFillValue, the base's REENTRY_HIDDEN_COLUMN_PREFIX used at lines 572/576). Kept this branch's carried_outputs parameter on apply_optional_reentry_null_fill. Kept the base's deletion of the empty_result_row prose comment while keeping this branch's `null_row: CypherFillRow` annotation. - bin/ci_type_hygiene_baseline.json: took the base's numbers, then --update-baseline; reentry/execution.py now has zero findings and its entry is removed rather than left at 8. Gates: no conflict markers, ruff clean, type-hygiene guard clean, cypher surface guard pass, mypy shows only the 4 known polars-skew errors. graphistry/tests/compute/gfql = 8595 passed / 93 failed, and the same 93 fail on ghhttps/fix/gfql-1888-endpoint-closure alone (local cudf + polars version skew), so the merge adds no failures. Touched tests (test_flatten_terminal_with, test_optional_reentry_carried_outputs, test_optional_match_semantics, plus agg/flatten neighbours): 431 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

Cascading base update after #1897 took its review remediation. Resolutions: - graphistry/compute/gfql/cypher/reentry/flatten.py: the base refactored flatten_terminal_with_over_optional into named helpers and DELETED _pure_carry_aliases_ignoring_where (verified: absent from ghhttps/fix/gfql-1896-om-with-pipeline:flatten.py, its only caller replaced by _is_terminal_with_over_optional_match / _stage_reshapes_rows / _bare_carry_aliases). Took the base's deletion plus its new _stage_has_aggregates, which the cleanly-merged _query_with_terminal_stage_folded_into_return already calls. Kept this branch's genuinely-new flatten_pure_carry_terminal_with_nonoptional (#1899), which reads _pure_carry_aliases, not the deleted helper. `git grep _pure_carry_aliases_ignoring_where` is now empty. - bin/test-polars.sh: kept BOTH lane entries -- test_row_multiplicity_semantics.py (this branch) and test_hop_boundary_matrix.py (base) are different files, so picking a side would silently drop a polars lane. - bin/ci_cypher_surface_guard_baseline.json: took the base's 9493, then --write-baseline. lowering.py is 9635 lines after the merge, which is the union of both sides' additions and BELOW this branch's own previous cap of 9674; the ratchet is tightened, not loosened. Gates: no conflict markers, ruff clean, type-hygiene guard clean, cypher surface guard pass, mypy shows only the 4 known polars-skew errors. test_row_multiplicity_semantics + test_fast_path_engagement + tests/compute/gfql/cypher + test_optional_match_semantics = 3671 passed, 8 failed -- all 8 are [cudf] parametrizations that fail identically on the base branch alone (no GPU in this environment). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

#1897 went CONFLICTING when #1895's remediation landed, so GitHub could build no merge ref and stopped creating workflow runs entirely. Both conflicts were "each side appended a different new section", merge base empty on both: - test_optional_match_semantics.py: #1895 added section E (optional-reentry aggregate fill values, #1891, compile-level so the end-to-end sweep cannot discharge it vacuously via a decline); #1897 added section F (#1896 OM -> WITH pipeline row semantics). Different features, so BOTH kept. - ci_cypher_surface_guard_baseline.json: `lowering_py_max_lines` was 9493 ours / 9425 theirs. Neither is right for the merged tree -- taking either side would have left the ratchet loose or falsely tight. Set to the MEASURED count of the merged file, 9464. 90 passed in test_optional_match_semantics.py; cypher surface guard passes; ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

…ith-pipeline # Conflicts: # graphistry/compute/gfql/lazy/engine/polars/chain.py

… into fix/gfql-1896-om-with-pipeline

… mirror reentry tests Externs the optional-reentry carried-output symbols out of the oversized gfql_unified.py into graphistry/compute/gfql/cypher/reentry/carried_outputs.py, replaces the getattr AST probes with isinstance dispatch on ASTCall, and shares one identifier predicate across flatten/carry/carried_outputs via graphistry.compute.gfql.identifiers. Tests pinning cypher/reentry/* now mirror the source tree under graphistry/tests/compute/gfql/cypher/reentry/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

#1901 went CONFLICTING when #1897's merge landed, so GitHub could build no merge ref and created no workflow runs at all. One conflict, in ci_cypher_surface_guard_baseline.json: `lowering_py_max_lines` was 9635 ours / 9464 theirs / 9493 at the merge base. Neither side's number describes the merged tree, so it is set to the MEASURED count of the merged lowering.py, 9635. The cypher-surface guard passes at that value. gfql_fast_paths.py auto-merged. ruff clean; cypher suite 3517 passed with 7 failures, all `[cudf]` lanes already present in the recorded baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-15T21:55:48Z

Two things I promised in the dispatch comment above were not actually delivered at fcd46ace5, and one risk had never been checked. Both are closed now, and I re-audited all 16 threads against the current head rather than assuming the earlier write-up.

1. The test relocation was one level short

You asked for cypher/reentry/test_flatten.py rather than top-level. The files had only reached graphistry/tests/compute/gfql/cypher/ — there was no reentry/ directory at all. git mv'd, so history follows:

old	new	tests
`tests/compute/gfql/cypher/test_flatten_carried_endpoint_rebind.py`	`tests/compute/gfql/cypher/reentry/test_flatten_carried_endpoint_rebind.py`	23
`tests/compute/gfql/cypher/test_flatten_pure_carry_optional.py`	`tests/compute/gfql/cypher/reentry/test_flatten_pure_carry_optional.py`	26
`tests/compute/gfql/cypher/test_flatten_terminal_with.py`	`tests/compute/gfql/cypher/reentry/test_flatten_terminal_with.py`	24
`tests/compute/gfql/cypher/test_optional_reentry_carried_outputs.py`	`tests/compute/gfql/cypher/reentry/test_carried_outputs.py`	7

80 before, 80 after (23/26/24/7 each side, checked per file, not just the total).

The fourth one is the interesting case, and it is why I checked what each file pins instead of moving on filename: test_optional_reentry_carried_outputs.py did not pin a cypher/reentry/* module at all — it imported graphistry.compute.gfql_unified._carried_output_source_column. Moving it under reentry/ would have been the wrong location for the code as it stood. Your #1508/#1515 comments are the same observation from the other end, so I moved the code instead (below) and then the test followed it honestly, renamed to mirror its module.

No __init__.py — checked first: the sibling cypher/ and row/ test dirs have none (index/, layouts/, same_path/ do), so the new dir matches its immediate parent.

References swept (grep -rn over CI configs, bin/*.sh, docker/*.sh, pyproject.toml, DEVELOP.md, the guard baselines and test_polars_lane_completeness.py): none of the four files was registered anywhere. Specifically —

test_polars_lane_completeness.py keys on modules that mention polars; all four files contain zero occurrences of the string, so none was in the lane list or the NOT_POLARS_GATED dict.
.github/workflows/ci.yml path filters use the prefix regex ^graphistry/tests/compute/gfql/cypher/, which cypher/reentry/... still matches — no CI edit needed.
The polars lane still carries the #1896 execution tests via test_optional_match_semantics.py, which did not move.

2. The comment-density guard: it did fail, and the fix was not a cap

You were right that this was live, not hypothetical. Merging master (526976e91) — which brings #1927, #1924, #1926, #1894 — the guard failed on two files:

comment-block:    graphistry/compute/hop.py                8 findings, baseline 6
issue-rationale:  .../cypher/reentry/flatten.py            2 findings, baseline 1

hop.py was inherited, not this PR's: the base branch had already fixed it in a7c9d6fd5 ("merge: master into fix(gfql)!: endpoint-closure contract on every surface (#1888, absorbs #1808) #1895, and clear the comment guard it now trips"), which fcd46ace5 predates. Merging the current base head cleared it — no duplicate fix, no divergence with fix(gfql)!: endpoint-closure contract on every surface (#1888, absorbs #1808) #1895.
flatten.py was ours: flatten_terminal_with_over_optional's docstring cited (#1896) as its rationale. Deleted the citation; the rationale is carried by the pin names in reentry/test_flatten_terminal_with.py. The grandfathered 1 is the pre-existing #1341 module docstring.

No baseline cap was raised. The only baseline edit is a tightening: gfql_unified.py comment-block 32 → 21 and issue-rationale 15 → 11, locked because the externing below made the improvement real. I deliberately did not run a blanket --update-baseline: 26 other files are now below their caps, but those improvements belong to master/#1895 and locking them here would just manufacture a conflict on a shared file.

3. Merge resolution

One conflict, in lazy/engine/polars/chain.py. diff3 showed it was the trap you described: our side had trimmed a 5-line comment to 3, and theirs looked like it had rewritten the same comment — but the real content of their side was a new edge_src, edge_dst = _bound_edge_endpoints(self) line that the code below now depends on. Kept both: master's extraction plus our trimmed comment. _bound_edge_endpoints is master's new helper (chain.py:855) and is used at the return g0.nodes(...).edges(...) below.

bin/ci_cypher_surface_guard_baseline.json: lowering_py_max_lines resolved to 9464, which is the measured line count of the merged lowering.py (master was 9425 and untouched there; the delta is entirely this PR's). The ratchet you flagged still has zero slack by construction — I left it that way rather than quietly loosening it. Say the word and I'll add headroom.

4. Per-thread disposition (all 16)

#	Thread	Disposition	Evidence
1	`execution.py` "Violates comment rules too"	Addressed in `fcd46ace5`	`grep -n "never synthesize ANONYMOUS" reentry/execution.py` → no match. Rule now in the helper name `_optional_reentry_unmatched_identity_null_rows` (execution.py:197). Guard: file at 4 vs cap 5.
2	`execution.py` "Avoid Any when narrow available"	Addressed in `fcd46ace5`	`reentry/execution.py` is absent from `ci_type_hygiene_baseline.json`'s `explicit-any` map — absent means it must be zero. Aliases `CypherFillValue`/`CypherFillRow` at execution.py:45/48. Self-review gate over the whole product diff returns no added `Any`/`hygiene-ok`/`cast(`.
3	`flatten.py` "More comment issues"	Addressed in `fcd46ace5`, +1 residual fixed here	20-line docstring is now 2 lines + named helpers (`_is_terminal_with_over_optional_match`, `_stage_reshapes_rows`, `_query_without_pure_carry_stage`, `_query_with_terminal_stage_folded_into_return`). The residual `(#1896)` citation the new guard caught is deleted.
4	`execution.py:177` "tck-gfql obligations likely, audit & handle"	Addressed — and here is the explicit answer I owed you	Companion branch exists and CI will run it: `tck-gfql@fix/gfql-1896-om-with-pipeline` = `48ced5951`; `ci.yml:672-703` resolves the tck ref by `head_ref` match. Yes, scenario outcomes change and yes the manifest needed regenerating — `09008dd contract: 4 OPTIONAL MATCH scenarios now succeed with correct rows`, `674f3b6 contract: regen capability-debt manifest for the 4 promoted keys`, `48ced59 fully promote match-where6-2 + match7-29/30/31`.
5	test file location	Was open — fixed here	Section 1.
6	`flatten.py:354` "significant pos/neg test obligations"	Addressed in `fcd46ace5`	`reentry/test_flatten_terminal_with.py`, 24 tests, both admit and decline branches at AST level.
7	`ast.py:30` "is this from lower on our pr stack?"	Addressed — now reconciled, not just answered	`git diff ghhttps/master HEAD -- graphistry/compute/gfql/cypher/ast.py` is empty after the master merge (#1894 landed in `526976e91`). The byte-identical copy merged away exactly as intended. Same for `agg_types.py`.
8	`lowering.py:9152` "pos/neg test amplification obligations"	Addressed in `fcd46ace5`, verified anti-vacuous here	The `test_1896_` family in `test_optional_match_semantics.py`: 22 test instances over pandas+polars, including 3 decline pins. Measured anti-vacuity: replayed against the base `a7c9d6fd5`, 20 of 22 fail*, 2 pass (positive controls).
9	`agg_types.py` verbose comment + root cause	Addressed in `fcd46ace5`	`git diff ghhttps/master HEAD -- graphistry/compute/gfql/agg_types.py` empty; the TYPE_CHECKING note is gone on both. Root cause is #1924, and it is now a machine gate (`bin/ci_comment_density_guard.py`, #1926) rather than a promise — this branch runs it clean.
10	`gfql_unified.py:1515` "strange location, shouldn't they be externed?"	Was PARTIAL — fixed here	The empty-group fill-value symbols did land in `gfql/agg_types.py`. The carried-output resolution did not: two regexes and three functions were still inline. Now `graphistry/compute/gfql/cypher/reentry/carried_outputs.py` (167 lines), which is also where `CarriedOutputSources` / `CARRIED_OUTPUTS_NOT_REPRODUCIBLE` now live — with their producer instead of their consumer.
11	`gfql_unified.py` "already overly large"	Was open — fixed here	2541 → 2422 lines (−119). Ratcheted and locked: `comment-block` 32 → 21, `issue-rationale` 15 → 11.
12	"replace dynamic patterns like getattr with structured & static programming"	Was STILL OPEN at `fcd46ace5` — fixed here	`getattr(op, "function", None)` / `getattr(op, "params", None)` were still at gfql_unified.py:1553-1554. Now `isinstance(op, ASTCall)` in `_projection_items_and_grouping`, matching what the sibling `optional_reentry_aggregate_fill_values` already did in the same file.
13	"test amplification concerns?" on `_carried_output_sources`	Addressed in `fcd46ace5`	`reentry/test_carried_outputs.py`, 7 named params covering every branch (incl. `p.__cypher_reentry_av__` read-through and the uncarried-marker fallthrough), plus the 22-test execution family.
14	`execution.py:56` "are any of these externally needed types?"	Now answered, and acted on	Yes for `CarriedOutputSources` + `CARRIED_OUTPUTS_NOT_REPRODUCIBLE` — `gfql_unified` imports both — so they moved to `carried_outputs.py`. No for `CypherFillValue`/`CypherFillRow`: `grep -rn` shows every use is inside `reentry/execution.py`, so they stay private there.
15	`flatten.py:45` "should these be externed and/or swapped to predicates?"	Was open — fixed here, both halves	Externed and turned into predicates: `is_bare_identifier()` / `identifier_tokens()` in `graphistry/compute/gfql/identifiers.py`. That retired four duplicates of the same regex — `flatten._BARE_IDENT`, `flatten._IDENT_TOKEN`, `gfql_unified._IDENTIFIER_TOKEN`, and two inline copies in `reentry/carry.py`. `_REENTRY_MARKER_COLUMN` became `reentry_hidden_column_output_name()` in `naming.py`, built from the same prefix/suffix constants as `_reentry_hidden_column_name()` so the writer and the parser cannot drift. `_AGGREGATE_CALL` stays local — one call site, flatten-specific.
16	`agg_types.py` "overly verbose"	Addressed in `fcd46ace5`	Same as #9.

Also added the missing CHANGELOG.md entry for #1896 — it had none.

Gates

gate	result
`ruff check graphistry bin`	clean
`ci_comment_density_guard.py`	OK, no growth (was FAILING before the merge)
`ci_type_hygiene_guard.py`	OK, no growth
`ci_cypher_surface_guard.py`	pass; `lowering.py` 9464 = baseline 9464
`mypy graphistry`	4 errors, 329 files — byte-identical to the base's 4 errors / 328 files, zero new
`pytest graphistry/tests/compute`	102 failed / 10243 passed vs base 102 failed / 10190 passed — failure lists md5-identical (`6a75a38ebb30614a35083426514a76ef`), `diff` empty. The 102 are pre-existing cuDF-lane failures on this box, unchanged. +53 passed = the 53 tests this PR adds (22 `#1896` execution + 24 terminal-WITH + 7 carried-output).
self-review gate on the product diff	only `#:` Sphinx alias doc-comments (the form accepted on #1894); no added `Any` / `type: ignore` / `hygiene-ok` / `cast(`

Failure lists captured whole (… 2>&1 \| grep '^FAILED' \| sort > file, never through tail) and compared as sets against a baseline generated from the merged base a7c9d6fd5 in a separate worktree.

Pushed as e08e0dbad (fcd46ace5 -> merge master 526976e91 -> merge base a7c9d6fd5 -> the three commits above). mergeable: MERGEABLE, base unchanged, CI running. Nothing was resolved, force-pushed, rebased, or re-based.

`test-gfql-core (3.12)` was the only red lane on #1897, and no test failed in it -- 4756 passed. The coverage audit rejected the run: graphistry/compute/gfql/cypher/reentry/carried_outputs.py: actual=94.17% floor=0.00% reason=resolved target file is missing from the baseline That file is new: externing the carried-output resolution out of gfql_unified.py (review threads #10/#11) created it, and a newly added source file has to be registered or the audit fails closed. Fixing the omission, not the gate. Floors follow each lane's measured reality rather than one invented number: pandas gets the 94.17% CI measured, and both rapids lanes get 0.0, matching every sibling in `cypher/reentry/` (carry, compiletime, flatten, lowering_support, rewrite all sit at 0.0 there -- the GPU lane does not exercise them). No floor is asserted that was not measured on that lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

…ting The owner rejected the same classes across three consecutive PRs (graphistry#1894, graphistry#1895, graphistry#1897): mechanism narration where a name/test/structure should carry the meaning, perf and asymptotics claims that belong in pyg-bench, and Any/type-ignore/hygiene-pin instead of a narrow type. Their diagnosis was that this skill is incomplete or unused; both were true, and 'unused' was the larger half -- it is written for reviewing others' PRs and was never run against our own diffs across ~14 authored PRs. Adds three sections: an Encoding table (narration, why-a-fix-was-made, perf/asymptotics, engine-mechanics prose, restating the signature), a Typing table (Any with a known domain, new type: ignore, hygiene-ok on new code, cast() to satisfy the checker, and why 'no growth' is not sufficient), and a Self-review gate with the greps to run over <base>..HEAD before pushing -- plus the rule that fix-cycle briefs must carry these rules inline, since a brief that gates correctness and mentions style in passing produces code that satisfies what was measured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

The "Encoding: names, tests, and structure -- not prose" rules in agents/skills/review/SKILL.md were the only rule class on the 2026-08 GFQL stack with no automated enforcement, which is exactly why they were the only class that kept reaching the owner: rejected on graphistry#1894, graphistry#1895 and graphistry#1897 for the same reason each time. Everything else that mattered already had a gate -- bin/ci_type_hygiene_guard.py (AST, per-file baseline), bin/ci_cypher_surface_guard.py (line-count ratchet), the per-file coverage floors, test_polars_lane_completeness.py. Comment discipline was left to human review. This converts it into a gate. bin/ci_comment_density_guard.py is a stdlib-only tokenize + ast pass over graphistry/, modelled on the type-hygiene guard: same per-file count ratchet, same --report/--list/--update-baseline/--strict surface, same exit codes, and a `# guard-ok: <check> -- <reason>` escape hatch. It runs from bin/lint.sh, so it shares the python-lint-types matrix (py3.8-3.14) with no new workflow and no new job; that lane fires on any *.py change. Checks: comment-block 2+ adjacent full-line `#` comments (3+ for a Sphinx `#:` run, which was being used as a prose loophole) perf-claim complexity notation or performance vocabulary issue-rationale a standalone comment or docstring citing an issue number as the explanation comment-block is a form rule and reads `#` comments only; the two content rules also read docstrings, since a claim does not become admissible by moving into one. Tests are exempt from comment-block and issue-rationale -- a test may explain its oracle -- but not from perf-claim: measurement belongs in pyg-bench wherever it is written. Master's grandfathered debt: 948 comment-block, 216 perf-claim, 169 issue-rationale. Acceptance was measured, not asserted. Replayed against the three rejected branches at the commits the owner reviewed, the guard flags all 16 sites he flagged by hand, and each of the 9 touched files exceeds its master cap, so the ratchet would have failed all three PRs. Two false-positive classes were measured and tuned out rather than left as noise: `regress`/`A/B` also name correctness concepts, so they count only next to performance vocabulary; and a comment naming pyg-bench is a pointer to where measurement lives, not a claim. Together those were 128 of 344 raw perf-claim hits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

This was referenced Aug 14, 2026

CI lane-coverage audit: ~1,600 test ids with zero CI execution + neutral-skip gating (round-004 umbrella) #1898

Open

fix(gfql): row-pipeline multiplicity + aggregate identities (#1899) #1901

Open

lmeyerov commented Aug 15, 2026

View reviewed changes

lmeyerov mentioned this pull request Aug 15, 2026

docs(review): encode the rejection categories the GFQL stack kept hitting #1924

Merged

lmeyerov commented Aug 15, 2026

View reviewed changes

lmeyerov mentioned this pull request Aug 15, 2026

Refactor lowering.py: 9,260 lines / 217 top-level defs in one module #1921

Open

lmeyerov commented Aug 15, 2026

View reviewed changes

lmeyerov mentioned this pull request Aug 15, 2026

fix(gfql)!: OPTIONAL MATCH null-extension semantics (#1891) #1894

Merged

lmeyerov and others added 5 commits August 15, 2026 14:24

Merge remote-tracking branch 'ghhttps/master' into fix/gfql-1896-om-w…

69217d2

…ith-pipeline # Conflicts: # graphistry/compute/gfql/lazy/engine/polars/chain.py

Merge remote-tracking branch 'ghhttps/fix/gfql-1888-endpoint-closure'…

325d53f

… into fix/gfql-1896-om-with-pipeline

ci(gfql): lock the gfql_unified comment-density improvement

72892fc

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

docs: changelog entry for the #1896 OPTIONAL MATCH + WITH fix

e08e0db

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

		@@ -0,0 +1,150 @@
		"""Direct tests for terminal-WITH-over-OPTIONAL flattening (#1896).

		return replace(query, with_stages=()), stage.where


		def _query_with_terminal_stage_folded_into_return(

		return None


		def _carried_output_sources(compiled_query: CompiledCypherQuery) -> CarriedOutputSources:

Uh oh!

Conversation

lmeyerov commented Aug 14, 2026

Uh oh!

lmeyerov commented Aug 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmeyerov commented Aug 15, 2026

The three sites

Self-review gate, whole diff

Pins added where a comment was the only record

Gates

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmeyerov commented Aug 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmeyerov commented Aug 15, 2026

1. The test relocation was one level short

2. The comment-density guard: it did fail, and the fix was not a cap

3. Merge resolution

4. Per-thread disposition (all 16)

Gates

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant