[matthewpeterkort]

Summary

Registers the post-window render surface in internal/dataframe/compiler/render/postgres/ so the PG arm can publish the #159 paired-benchmark AuthorWorkload output aggregate_sum_age_by_patient (previously 422 at render.go:245). The compiler scheduler places the generic SORT/LIMIT window (compiler/output.go) before the child-route LETs, so post-window TRAVERSAL / SET / AGGREGATE ops are legitimate plan shapes that the AQL arm already renders as LET collections and FLATTEN reducers.

1. Adjudication

Provenance memo + addendum accepted in orchestrator session loom-experiment-2 (offline repro: AQL arm renders clean, PG arm 422s; plan dump in the accepted memo). Three walls, all renderer-side:

1. Traversal guard (render.go:245): phase != 0 TRAVERSAL → ErrUnsupportedPostgresPlan, no dead-LET acceptance.
2. SET guard (render.go:260-263 area): post-window SET refused outright; renderSet further refused any set without a FIRST/ALL/DISTINCT reduction.
3. Zero aggregate dispatch: no PhysicalAggregateExpression case existed anywhere in the PG renderer (classifier or RETURN); the workload's SUM/COUNT/MIN/MAX RETURN projections were unreachable.

Layer verdict: renderer under-supports a legitimate plan. The compiler/scheduler output is canonical (AQL renders it); the scheduler, matcher (driver.go:296), harness, and AQL renderer are untouched — all 11 files are inside render/postgres/. Provenance check c6e16ddf: the PG arm's refusal is a never-silent-fallback typed error by design, so this is a capability gap to fill, not a defect to route upstream.

2. AQL-parity audit deltas (rendered semantics vs render/aql/collections.go renderAggregate)

• Item list deduplicates per reached document: the PG set-item subquery is SELECT DISTINCT over (logical_id, resource_type, payload). lower/child_sets.go:88 builds every child set with Unique: true, SortByKey: true, so the AQL set LET always wraps the item array in UNIQUE() (navigation_render.go:294-296) and collapses two edge occurrences reaching one document into one item; the PG arm matches, mirroring the sibling setReductionSublink convention (sets.go:274). A fhir_edge_occurrence duplicate (legal under fhir_edge_occurrence_identity_ux) therefore counts once in both arms — pinned live by TestPostWindowLiveReducersExecuteWithPredicates.
• ArangoDB type order for MIN/MAX: rank boolean < number < string < array < object (nulls pre-excluded), matching ArangoDB value-type ordering that AQL MIN/MAX consume; order keys cast ::numeric (jsonb is numeric-backed; out-of-double-range jsonb numbers order instead of erroring).
• Registered numeric band for SUM: per-operand callBandOperandClamp + groupDoubleBandCast (the groupBoundedDoubleSum shape), empty-set → NULL::jsonb, non-numeric guard raises the ASSERT: + NUMERIC_AGGREGATE_NON_NUMERIC guard-barrier idiom (calls.go:765 precedent, AQL ASSERT(IS_NUMBER(...))).
• COUNT arity split: Value-less COUNT is COUNT(*) over the item array (AQL LENGTH(items)); COUNT with a Value counts unnested non-null slots (AQL LENGTH(FOR v IN FLATTEN(values) FILTER v != null RETURN 1)), pinned by TestPostWindowCountWithValueCountsSlotsNotRows.
• ANY-quantified predicate renders through the registered quantifiedMatchFragment EXISTS form.

3. ENDPOINT_LOOKUP rim

ENDPOINT_LOOKUP is an ArangoDB access-path hint (graph traversal vs indexed edge scan); PG has no graph and edgeOccurrencePredicates renders the identical typed-identity join for both strategies, so it normalizes to NATIVE after payload checks — the registered precedent is stage_related_occurrence.go:373-379 / graph.go graph20NativeStrategyTraversal.

Admitted for aggregate-consumed sets ONLY (aggregateConsumesSetVariable whitelist). The reduction-consumed and related-existence surfaces keep the fail-closed refusal in validateTraversalContract until their owners adjudicate the shape. TestPostWindowEndpointLookupIsAdmittedOnlyForAggregateConsumers pins: byte-identical normalization for the admitted consumer, typed refusal for the reduction surface.

Verbatim (unmodified, still-green) witness tests that depend on the non-aggregate rim:

• TestCompileCellTracePostgreSQLRefusesEndpointLookupTyped (compiler/cell_trace_test.go)
• TestSelectedSourceCompilerAndRowsContract/CellTracePostgreSQLRefusesEndpointLookupTraversalByNameWithZeroPortCalls (execution/selected_source_contract_test.go)
• TestSQL16RelatedOccurrenceEndpointLookupEquivalence, traversal-endpoint-lookup, endpoint-lookup-join-field-missing (stage_related_occurrence_test.go)
• EndpointLookupStrategyNormalizesToRegisteredJoin, EndpointLookupTruncatedPayloadRefuses (graph_test.go)
• route step may not use endpoint lookup (population_test.go)

4. Preview path (limit > 0)

Pinned with a separate test, per GO: TestPostWindowReducersUnderPreviewLimit inserts the bind-backed LIMIT between SORT and SET (the preview form of CompileRecipeOutput; the #159 benchmark runs limit=0) and asserts the window LIMIT clause, the bigint 25 parameter, and every reducer sublink. Soundness: reducers are scalar sublinks in the SELECT list, so LIMIT-over-root-rows composes unchanged.

5. Receipts

Pre-fix red / post-fix green via the real emission path. DecodeWorkspace → CompileWorkspace → BuildRecipePlan → ResolveRecipePlan → lower.CompileResolvedRecipePlan → optimize → compiler.CompileRecipeOutput for both targets: pre-fix, the AQL target renders clean while the PG target 422s with the render.go:245 guard text (evidence captured in the accepted memo). Post-fix, the PG target renders the full statement (witness: pg-aggregate_sum_age_by_patient.witness.sql, shared in AO session loom-experiment-247). Additionally, the whole committed post_window_test.go run at parent 492d3f01 (scratch worktree) is red: 27 failures, every one a wall refusal (sets/traversals must precede the execution window) or aggregate-gate drift; identical file green at head.

Per-clause fixtures (post_window_test.go):

• Accepted: TestPostWindowAggregateReducersRenderAsCorrelatedSublinks, TestPostWindowDeadTraversalRendersNothingAndLocatedConsumersRefuse (dead hop emits nothing — statement byte-identical to baseline).
• Rejected: 8 refusal classes in TestPostWindowAggregateSurfaceRefusals (scalar-cardinality, empty-on-null, unregistered reducer, ordering/window, non-set source, undeclared slot, slot-type mismatch, DISTINCT multiplicity) + TestPostWindowSetWithoutRegisteredConsumerKeepsTheWindowGuard.
• Deadness near-misses (TestPostWindowDeadTraversalNearMisses): consumer-in-a-later-subplan-block, reducer-referenced, subplan-sort-key-referenced — each refuses with the typed not dead voice naming the read variable.
• Unrimmed aggregate ops (TestPostWindowUnrimmedAggregateOperationsRefuse): MEAN, EXISTS, COUNT_DISTINCT, DISTINCT_VALUES, FIRST, COLLECT, REQUIRE_ONE, CONTAINS_ALL, FIRST_ORDERED — validator-legal payloads, refusal isolated to this unit's gate behind the canonical AGGREGATE ... no registered PostgreSQL rendering wrapper.
• Row-count preservation (TestPostWindowReducersPreserveRowCounts): outer FROM ... AS r0 ... ORDER BY tail byte-identical with and without the accepted SET + reducers; single root scan; no LEFT JOIN — the window stays final and root rows are neither multiplied nor filtered.

Review round 1 (threads PRRT_kwDOS-yV5s63K-w9/-xC/-xM/-xO, all resolved): (1) aggregate predicate renders over corr.targetAlias."payload" — the pre-fix itemAlias."payload" back-reference failed plan analysis on live PG (SQLSTATE 42P01); (2) items subselect carries SELECT DISTINCT — AQL parity with the always-UNIQUE set LET (child_sets.go:88 always sets Unique); (3) MIN/MAX text keys carry COLLATE "C" (category_scan.go:287 shape); (4) ENDPOINT_LOOKUP rewrite requires the direction-derived field pair, failing closed per graph.go:355-357. Live coverage added: TestPostWindowLiveReducersExecuteWithPredicates (post_window_live_test.go, sets_live_test.go pattern) executes the four with-predicate reducers on real PostgreSQL 18 — red pre-fix (42P01), green post-fix, pinning duplicate-occurrence collapse, predicate exclusion, and the zero-child patient. Fix commits 8ac909e8, 9bcdefce.

Verification: go test ./internal/... -count=1 green, gofmt/go vet clean. Full postgres render package green against a local PostgreSQL 18.1 (LOOM_TEST_POSTGRES_URL, zonky portable), including the live predicate/dedup test and the pwin-01 delta audit in TestMain. The live leg now also executes in CI: the PostgreSQL 18 opt-in fixture runner job gained two steps — "Run the post-window live reducer leg on PostgreSQL 18" (go test -timeout=240s -count=1 -v -run 'TestPostWindowLive' ./internal/dataframe/compiler/render/postgres against the job's existing postgres:18 service) and "Assert the post-window live leg executed and never skipped" (fail-closed --- PASS token required, SKIP/absence fails with an ::error annotation, mirroring the repository-publish spine's assertion) — commit 0430af8e, workflow-only. The four legacy live files in the package remain local-opt-in pending a follow-up tooling ticket. Live paired-benchmark execution is #159's CI gate.

Review round 2 / verifier finding (loom-experiment-248, batched wave): item-list dedup was the confirmed contract violation — §2 above now states the corrected parity semantics; the items subselect SELECT DISTINCTs the three projected columns (mirror of setReductionSublink), the duplicate-occurrence collapse is pinned live (p-1: SUM 40, COUNT 2, MIN 10, MAX 30 with both edge rows proven stored). TestZZVerify248EmissionBothTargets passes at the merge ref: both targets render, set.Unique=true, PG item list carries DISTINCT. Base also moved: 8f65f3c5 (#189) merged the repository-wide no-code-comments gate; this branch's 173 handwritten comments were stripped comment-only (d42f09e1), and scripts/check_no_code_comments.py --all-tracked reports violations=0 at the merge ref. Pre-fix red re-verified at 492d3f01 with the final committed test files: 28 leaf failures (27 wall refusals + gate-text drift, plus the one new endpoint-direction refusal test failing at the same wall), zero at head. Fix wave: 8ac909e8, 9bcdefce, d42f09e1.

Out of scope

render.go guard bodies (fail-closed default preserved — acceptors insert, fall-through kept), driver.go:296 matcher, harness, compiler/scheduler, AQL renderer, stage_sequence.go, #189-owned files, and the #159 benchmark branch.

[matthewpeterkort]

Reviewed d8cff54 against base 492d3f0 (11 files, +1224/-22). The registration structure is solid: the dead-traversal acceptor with its conservative consumer scan, the scalar-sublink-only post-window set rule, the fail-closed window guard plus new negative tests, the lazy bind-parameter registration (verified safe against the graph.go:384 labelIndex read), and the ENDPOINT_LOOKUP gating for aggregate-only consumers all check out against the AQL arm and the lowering contracts.

Verdict: changes requested (posted as COMMENT because GitHub rejects REQUEST_CHANGES from the PR author account). Four inline findings:

1. The with-predicate reducers render SQL that references the items subselect alias inside its own definition (post_window.go:384). PostgreSQL rejects it, and no live coverage exercises the predicate path, so unit tests pass on a statement that cannot execute.
2. The aggregate items array counts edge occurrences while the AQL arm deduplicates per reached document: the child-set lowering always sets Unique (lower/child_sets.go:88), so AQL items are always UNIQUE-wrapped projection objects keyed by _id/_key (render/aql/navigation_render.go:294-296,338-343). COUNT/SUM diverge on multi-occurrence edges, and post_window_test.go:138 pins the divergence by forbidding DISTINCT.
3. MIN/MAX string ordering follows the database collation instead of ArangoDB byte order, breaking AQL parity and the package COLLATE "C" convention (cf. category_scan.go:287, group.go:943-954).
4. ENDPOINT_LOOKUP normalization discards EndpointField/EndpointJoinField without validating them against Direction; the AQL arm renders on those fields (navigation_render.go:44-51), so a plan whose fields disagree with direction would render a different join in each arm, silently. The house style fails closed on such planner-metadata divergences (graph.go:355-357).

Optional notes (not required changes): the renderSetAggregateExpression doc comment (post_window.go:328-331) states the AQL type order as number < boolean while the code correctly ranks booleans before numbers (ArangoDB orders null < booleans < numbers < strings < arrays < objects) - the comment is wrong, not the ranking; aggregateSlotValues never compares the extract own Selector/ExecutionMode with the declared projection slot, where the fail-closed style would; the pre-window TRAVERSAL/SORT/LIMIT branches in renderOperations still dereference payload pointers without the nil checks the new post-window branches added (pre-existing); the ASSERT-message textParameter uses an empty cache key like renderCallAssert, so each SUM projection appends a duplicate bind - harmless.

[matthewpeterkort]

Review of head 7423a51 (re-review of d8cff54).

Verdict: ready - approved. No required changes.

All four required changes from the previous pass are implemented and pinned at unit and live level:

1. Aggregate predicate renders over the reached target row: aggregatePredicateFragment receives corr.targetAlias, so the items subselect predicate reads x1."payload" instead of its own a<seq> alias (which is not visible inside the subselect it defines). Pinned by the anti-self-reference and x1."payload" census in post_window_test.go and executed by post_window_live_test.go, where a self-reference would fail plan analysis.
2. Items are deduplicated per reached document (SELECT DISTINCT "logical_id", "resource_type", "payload"), matching the UNIQUE()-wrapped child sets from lower/child_sets.go. The live test seeds two occurrences reaching obs-a and asserts SUM 40 / COUNT 2, with a witness query confirming both edge rows are stored, so the collapse is real and not a fixture artifact.
3. MIN/MAX carry COLLATE "C" on both text ordering keys (boolean and string), after the type-rank and numeric keys; token-pinned at exactly 4 occurrences in post_window_test.go.
4. ENDPOINT_LOOKUP on the correlated child-set surface is validated against the direction-derived contract in sets.go: empty endpoint names, a direction other than OUTBOUND/INBOUND, and any (EndpointField, EndpointJoinField) divergence from the derived pair all refuse before the strategy normalizes to the native join. Tests cover the opposite-direction pair and the swapped join field.

Also checked since the previous pass: the comment-strip commit is behavior-neutral (the one insertion is a code line with its trailing comment removed); the lazy textParameter allocation in edgeOccurrencePredicates and the quantifiedMatchFragment extraction preserve prior semantics (the Correlation refusal pre-existed in renderSelectorMatchFragment and the aggregate path now inherits it); the index-based loop in render.go skips exactly the operations that matchCanonicalScopeBlock validated; post-window set admission stays fail-closed for a set with no registered consumer, and dead-traversal acceptance refuses on any later read of the LET variables.

Optional, no change required: _ = r at the top of acceptPostWindowTraversal is now dead weight; the dead-traversal path checks the endpoint fields are named but not that they agree with the direction (harmless, that path renders no SQL); live coverage of MIN/MAX is numeric-only, so the two C-collated text keys are token-pinned but not executed.

Reviewed by inspection of base 492d3f0..7423a51. Reviewer-side test execution was not run in this environment; the worker reported the render/postgres package green on live PostgreSQL 18.1.

[matthewpeterkort]

Review of head 3241520 (third pass; previously reviewed d8cff54 and 7423a51).

Verdict: ready - approved. No required changes.

The only delta since the approved head 7423a51 is merge commit 3241520, which brings the branch onto the moved base 8f65f3c (lane-05, repository-wide no-code-comments gate wave). The merge is faithful:

• Merge parents are exactly 7423a51 and 8f65f3c; the six previously pushed commits are unmodified (append-only hop, no rebase, no force).
• The effective PR diff (8f65f3c..3241520) is the same 13 files at +1323/-34 that I approved at 7423a51, and every worker file is byte-identical between the two heads.
• The only render/postgres change introduced by the merge (stage_sequence_test.go, 3+/3-) is byte-identical to the change the base itself makes to that file, so there are no conflict-resolution edits hiding in the merge.
• The base wave touches no compiler code besides that one test file, so it cannot interact with the post-window logic.

Gate posture at head: none of the six new-or-touched PR files contain comment tokens; the only comment line in the package is the base-mandated D-16a attribution in stage_sequence.go (base content, pinned by a base test).

The four required findings from the first pass remain fixed because the reviewed content is unchanged: the aggregate predicate reads x1."payload" on the reached target row, items are SELECT DISTINCT over (logical_id, resource_type, payload) for AQL UNIQUE parity, MIN and MAX carry COLLATE "C" on both text ordering keys, and ENDPOINT_LOOKUP refuses any (EndpointField, EndpointJoinField) divergence from the direction-derived contract before normalizing to the native join.

Optional, unchanged from the previous pass: dead _ = r in acceptPostWindowTraversal; no direction cross-check on the dead-traversal path (that path renders no SQL); MIN and MAX live coverage is numeric-only, so the two C-collated text keys are token-pinned but not executed.

Note the PR is currently draft; the merge hop is coordinator-ordered per the commit message. Reviewer-side test execution was not available in this environment; verification here was diff inspection against the exact head.

[matthewpeterkort]

Review of head 0430af8 (fourth pass; previously reviewed d8cff54, 7423a51, and 3241520).

Verdict: ready - approved. No required changes.

The only delta since the approved head 3241520 is commit 0430af8, which adds two steps to the pg-fixture job in .github/workflows/tests.yaml: a scoped go test -run TestPostWindowLive leg tee-ing to a runner-temp log, then a never-skipped assertion requiring the PASS token and refusing SKIP with an ::error annotation. This directly closes the gap flagged in my previous passes: TestPostWindowLiveReducersExecuteWithPredicates previously executed only on the worker machine, so the live predicate, dedup, and reducer semantics had no CI execution.

Verified against the job definition at this head:

• The pg-fixture job already carries the postgres:18 service, a job-level LOOM_TEST_POSTGRES_URL (job env is inherited by every step), and setup-go@v5 with go-version 1.26.5, so the new step has both the toolchain and the live URL the claim requires.
• Every failure mode is fail-closed: go build or test failure fails step 1 via set -euo pipefail with the tee pipe; -count=1 defeats result caching; if -run matched nothing the PASS token would be absent and step 2 exits 1; a SKIP prints the ::error and exits 1; the leading-dash grep patterns are correctly guarded with --.
• The assertion mirrors the existing pg-repository-publish spine step structure exactly, matching repository convention.
• The loom_test service user is the container superuser, so the owned-database CREATE/DROP in newOwnedService is permitted; the pwin-01 delta audit stays wrapped in TestMain, so a leaked pwin01_it_* resource fails the leg.
• -run TestPostWindowLive matches only the live family; the 13 previously approved files are byte-identical to head 3241520, so the four first-pass fixes (target-row predicate, DISTINCT per-document items, COLLATE C on both MIN/MAX text keys, endpoint-direction fail-closed check) stand unchanged.

Optional, no change required:

• The PASS-absence case fails step 2 via bare grep with no ::error annotation (only SKIP gets one); this mirrors the existing spine step verbatim, so consistency argues for leaving it.
• Step timeout ceilings now sum to 960s against the job 10-minute ceiling; the pre-existing steps already summed to 720s and the live leg measured ~0.07s plus cached build, so this is theoretical.
• The new log is not added to the Preserve fixture evidence artifact upload; the console output of step 1 plus the gating step 2 cover the evidence need.

Reviewer-side test execution was not available in this environment; verification was workflow-definition and diff inspection against the exact head, plus the worker reported receipts for the local run and assertion-logic checks (exit 0 on PASS log, exit 1 on SKIP, exit 1 on absent token).