test(reads): bound cross-store alignment load (EN-1946)

[gfyrag]

Summary

• bound the EN-1748 cross-store alignment load at a 32-sequence native-index lag;
• stop pressure producers as soon as the bound is reached;
• keep the complement-read assertion while adding a deterministic controller regression that proves the read waits when the projection is behind.

Stack

EN-1946 stack 1/8. Base: release/v3.0 at ef83e61.

Merge/review order: #1897 → #1889 → #1890 → #1894 → #1891 → #1893 → #1892 → #1881.

Validation

Final base: ef83e61.
Final head: b9080b6.
Canonical PR validation: PASS.
Independent exact diff review: APPROVE (residual risk LOW).

Jira: EN-1946. Do not merge automatically.

[NumaryBot]

✅ Approve — automated review

The test-only changes bound E2E pressure and add a deterministic controller regression without introducing an actionable defect. Prior findings are resolved or superseded by the focused alignment test.

No findings.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.32%. Comparing base (ef83e61) to head (b9080b6).

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1897      +/-   ##
================================================
- Coverage         83.35%   83.32%   -0.04%     
================================================
  Files               459      459              
  Lines             42363    42363              
================================================
- Hits              35311    35297      -14     
- Misses             7047     7061      +14     
  Partials              5        5              

Flag	Coverage Δ
e2e	83.32% <ø> (-0.04%)	⬇️
scenario	83.32% <ø> (-0.04%)	⬇️
unit	83.32% <ø> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shipfox-ai]

This PR rewrites the EN-1748 cross-store alignment e2e regression to bound the load it generates (EN-1946): instead of an unbounded 25-second commit-probe loop plus an explicit backlog drain, it now drives the native index fold to a lag of ≥ 500, stops the pressure producers, and then retries the not(ts[_,_]) complement read, tolerating only the retryable not-caught-up rejection. The core regression assertion — a committed transaction must never surface as missing from a READY timestamp index — is preserved and still fails the test when violated. One standards breach remains around how the retryable error is identified; everything else is minor. Recommendation: approve with comments (test-only change; the findings affect failure diagnosis and coverage fidelity, not production correctness).

Standards

[P2] Retry predicate matches the shared codes.FailedPrecondition instead of the READ_INDEX_NOT_CAUGHT_UP reason

tests/e2e/business/cross_store_snapshot_alignment_test.go:143:

return status.Code(err) != codes.FailedPrecondition

AGENTS.md:78 requires that "Assertions must uniquely identify the intended branch", and docs/technical/contributing/testing.md:30-32 requires failure-path assertions to distinguish the intended branch rather than match a signal shared by other failures. The server explicitly marks the intended branch: internal/adapter/grpc/server.go:652-667 attaches an errdetails.ErrorInfo with Reason: "READ_INDEX_NOT_CAUGHT_UP" to the not-caught-up rejection. codes.FailedPrecondition is also produced by unrelated conditions: domain.KindConflict and domain.KindPrecondition (internal/adapter/grpc/errors.go:44-46), raft leadership/backup/stale-progress preconditions (internal/adapter/grpc/server.go:604-641), and external-service errors (internal/adapter/grpc/server.go:683+).

Impact: a transient unrelated precondition is retried as if it proved fold lag, and a subsequent successful response passes without demonstrating that only the permitted not-caught-up branch occurred; a persistent one consumes the full 3-minute budget and fails with the mislabeled message "read never reached an aligned snapshot" instead of the real error. (Conversely, any non-FailedPrecondition error ends the poll and fails via the bare Expect(queryErr).To(Succeed()) at line 153 without identifying the branch — same root cause.) Resolution: unwrap the gRPC status details and retry only READ_INDEX_NOT_CAUGHT_UP; treat every other error as terminal with an explicit assertion.

[P3] Errors in the pressure/lag phase are masked and can surface as a mislabeled "inconclusive" failure

At tests/e2e/business/cross_store_snapshot_alignment_test.go:103-105 a pressure worker silently returns on any Apply error (shrinking the generated load with no diagnostic), and at lines 122-125 the lag poll maps a GetIndexStatus error to return 0, indistinguishable from a caught-up fold. If load generation or status RPCs fail persistently, the only symptom is the generic "pressure never made the fold lag — inconclusive" after 25 seconds, misattributing an infrastructure fault. docs/technical/contributing/testing.md:143-147 (and the ✅ pattern at lines 588-592) documents the Eventually(func(g Gomega) ...) form with g.Expect(err).To(Succeed()), which fails fast with the real error. Minor, diagnosis-quality only.

No other standards findings: no time.Sleep is used; variable declarations are grouped; the precondition tolerance is explained in a comment.

Spec

[P3] The preserved complement assertion is no longer exercised against reads racing in-flight commits

The assertion itself is faithfully retained: the not(ts[_,_]) filter, the zero-rows requirement, and the identifying failure message at tests/e2e/business/cross_store_snapshot_alignment_test.go:155. However, per the PR's own stop-then-read design, stopPressure() (line 129) runs before any read, so the complement query is only ever served against the draining fold backlog — never while commits are still landing, which was the original EN-1748 trigger configuration. The essential regression window (lagging index + complement read) survives, since the read is only served once the index catches up; but the concurrent commit+read configuration no longer has an independent case, contrary to the preservation spirit of docs/technical/contributing/testing.md:26-29. If the bug requires a commit to land while a read is in flight, this window no longer catches it. Judgement call: bullets 1–2 of the PR body mandate exactly this stop-then-read ordering, so this follows the spec's stated design — but the "preserved" claim is narrower than it reads.

No other Spec findings. Verified non-issues: the explicit backlog drain removed from the old test is implicitly performed by the aligned-read retry loop (the query is only served once the index is caught up), so its removal has no teardown-budget impact; the realized backlog exceeding 500 by the in-flight batches at pressure.Wait() is consistent with the stated "bounded" goal; calling GetIndexStatus without the ledger filter is correct — the aggregate counters are documented as global (misc/proto/bucket.proto:1132-1137) and the spec runs on a dedicated single-node server; and the closure-captured queryErr/resultLen/firstID reflect the final (successful) poll invocation, so the post-loop assertions examine the right response.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1897 (comment)

[shipfox-ai]

This PR (11b7c98) reworks only the EN-1748 regression E2E test cross_store_snapshot_alignment_test.go, replacing the fixed 25-second load window with a bounded lag condition (poll GetIndexStatus().Lag until ≥500), stopping the load producers before the filtered read, and keeping the not(ts[_,_]) phantom-row assertion intact. The core regression assertion is sound and the change is a genuine stabilization improvement. However, the new retry/terminal error handling has two confirmed documented-standard breaches that can mask or mislabel real failures, plus one independently verified inaccuracy in the retry loop's premise. Recommend: approve with comments (test-only change; the regression assertion itself is correct).

Standards

1. [Hard] The retry predicate is branch-blind and the terminal timeout hides the underlying error.
   tests/e2e/business/cross_store_snapshot_alignment_test.go:139-152. return status.Code(err) != codes.FailedPrecondition (line 143) retries any FailedPrecondition, but the server produces that code from many unrelated paths: the KindConflict/KindPrecondition mapping (internal/adapter/grpc/errors.go:44-46), leadership-transfer, stale-raft-progress, removed-node and backup sentinels (internal/adapter/grpc/server.go:601-640), and S3/infrastructure errors (server.go:677-697). This breaches AGENTS.md ("Assertions must uniquely identify the intended branch") and docs/technical/contributing/testing.md:30-32 ("Failure-path assertions must distinguish the intended branch"). Worse, when a persistent FailedPrecondition exhausts the 3-minute budget, Gomega aborts at Should(BeTrue(), "read never reached an aligned snapshot"), so the Expect(queryErr).To(Succeed()) on line 153 never executes and the actual error is never surfaced — the failure is reported only as the generic timeout message. Fix: unwrap the status details and retry only when the errdetails.ErrorInfo reason is READ_INDEX_NOT_CAUGHT_UP (attached at internal/adapter/grpc/server.go:649-660), treat every other error as terminal, and fold the final error into the terminal assertion (e.g. a func(g Gomega) callback or reporting the last error on timeout).

2. [Hard] The retryable rejection the loop waits for is not producible by this request shape — the comment and the retry branch are misleading.
   Line 131-133 claims "The pre-EN-1946 path can reject with a retryable not-caught-up precondition," but with the request the test sends, it cannot: actions.ListTransactionsFiltered (pkg/actions/read.go:563-577) never sets Read.MinLogSequence; ErrReadIndexNotCaughtUp is returned only when req.GetMinLogSequence() > 0 (internal/query/executor.go:154-159); the server-side waitMinLogSequence is a no-op at 0 (internal/adapter/grpc/server_bucket.go:482-485); and for filtered reads, alignment is a server-side blocking wait bounded by the caller's context (internal/query/aligned_snapshot.go:64-75, internal/application/ctrl/list_entities.go:102), not a client-retryable rejection. Consequently the codes.FailedPrecondition retry branch is effectively dead code, the comment documents behavior the server does not have for this call, and the intended fail-then-success retry sequence is never actually exercised — the exact thing testing.md:33-35 requires retry tests to verify. Fix together with finding 1: either set Read.MinLogSequence on the request so the not-caught-up rejection is genuinely reachable and retry only that machine-readable reason, or drop the retry branch and correct the comment.

3. [Hard] Index-status errors are swallowed instead of handled explicitly.
   Lines 121-128: st, err := actions.GetIndexStatus(ctx, client); if err != nil { return 0 } converts a persistent RPC/server failure into "lag = 0", so the 25-second Eventually exhausts and fails with "pressure never made the fold lag — inconclusive" while the real error is erased. This breaches AGENTS.md ("Do not ignore errors. Handle them explicitly…") and docs/technical/contributing/conventions.md:50-52 ("Always handle errors explicitly"), and diverges from the documented Gomega pattern in docs/technical/contributing/testing.md:578-592. Impact: an infrastructure failure is misdiagnosed as an inconclusive test-setup problem. Fix: use Eventually(func(g Gomega) uint64 { st, err := …; g.Expect(err).To(Succeed()); return st.GetLag() }, …), which still tolerates transient errors but surfaces the last error on timeout.

Spec

No confirmed material finding. No spec document is available in the review context, so spec conformance could not be formally assessed. As far as the change's own stated intent goes, the diff matches it: the load window is bounded by an observed lag condition rather than a fixed sleep, producers are stopped before the read is issued, the not(ts[_,_]) probe and the phantom-row assertion (count + first transaction ID) are preserved, and no scope beyond this test file was touched.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1897 (comment)

[shipfox-ai]

This PR rewrites tests/e2e/business/cross_store_snapshot_alignment_test.go so the EN-1748 regression test establishes a bounded fold lag (≥500 within 25s) before issuing the index-complement read, instead of probing in a loop under sustained pressure. The bounded-load mechanics are sound and the error-handling rework is a genuine improvement, but both reviewers converged — and code inspection confirms — on one material problem: the lag proof and the read's snapshot capture are not synchronized, so the rewritten test can pass without exercising the torn-snapshot state it exists to guard. Recommendation: request changes — a single, well-scoped fix (synchronize the lag proof with the read's horizon capture, or provide mutation/sensitivity evidence) unblocks it.

Standards

[Blocking] The lag proof is decoupled from the read's snapshot capture, so the test may no longer guard the production path it claims to guard
tests/e2e/business/cross_store_snapshot_alignment_test.go:121-134

The sequence is: Eventually(... GetLag() >= 500) (lines 121–126) → stopPressure() (line 127, which closes the producer channel and waits for as many as four in-flight 20-transaction Apply calls to finish) → ListTransactionsFiltered (line 131). Two verified facts make this a race:

1. GetIndexStatus computes lag from a read-store snapshot opened before its main-store handle (internal/application/ctrl/controller_default.go:1218-1229), so the observed value is a point-in-time sample that can already overstate the lag at the moment the poll succeeds.
2. The indexer keeps folding throughout the stopPressure() wait and RPC setup. The read's main-store horizon (mainSeq) is captured inside the RPC, after OpenQueryHandle — nothing re-verifies at that instant that the index cursor is still behind.

Consequently, if the fold drains the 500+ sequence backlog during the gap, the query captures an aligned pair regardless of whether AlignedIndexSnapshot (internal/query/aligned_snapshot.go) is in the path. Reverting the EN-1748 fix would then yield an empty complement and a green test — the regression test fails when the guarded production call is bypassed, violating docs/technical/contributing/testing.md:23-24 ("a regression test is a guard for a specific production path, not merely an example that reaches nearby code") and the mutation-check guidance at testing.md:40-42. Note that merely sampling lag again immediately before the RPC does not fix this: the horizon is captured inside the RPC after the handle opens, so the same race remains.

Resolution: either couple the lag evidence to acquisition of the read's main-store horizon (e.g., guarantee the fold cannot catch up between threshold and capture, or prove lag > 0 at a point ordered before the read's snapshot), or provide mutation/sensitivity evidence that bypassing AlignedIndexSnapshot reliably fails this focused test.

No other material Standards finding. The new Eventually(func(g Gomega) ...) form matches the documented pattern in testing.md, no time.Sleep was introduced, and the Fail messages distinguish their branches.

Spec

[Missing/partial] S2/S4 — the bounded lag is established but not preserved through the read's snapshot capture
tests/e2e/business/cross_store_snapshot_alignment_test.go:121-134

The PR claims to "make the existing EN-1748 cross-store alignment regression establish a bounded native-index lag before reading" while preserving "the assertion that an index-complement read never surfaces committed rows missing from a READY index." The first half is implemented as specified (threshold ≥500, 25s cap, producers stopped at the threshold, labelled inconclusive on timeout). The preservation, however, holds only in form: the complement assertion is exercised if and only if the fold is still behind the main store when the read begins, and as detailed under Standards, nothing guarantees that — stopPressure()'s wait for in-flight applies plus RPC setup gives the indexer an unobserved window to catch up. The original test read while commits were landing under pressure, which guaranteed a torn window; the rewrite removes that trigger without a replacement proof, so the test can pass without ever reaching the EN-1748 regression state. The in-code comment ("The first response it serves must therefore already be aligned; observing any rows from the complement is the regression") describes correct aligned behavior, not detection sensitivity under the regression, so it does not close the gap.

Resolution: as under Standards — synchronize the lag proof with the read's horizon capture, or attach mutation evidence demonstrating the focused test fails when AlignedIndexSnapshot is bypassed. Verified non-issues, for the record: the removed explicit drain step is superseded by the alignment wait; the error-handling changes to the pressure workers (Fail on Apply error outside the stop window) serve the bounded-load purpose and cannot misfire on the retryable READ_INDEX_NOT_CAUGHT_UP precondition, which is query-side only (internal/query/executor.go:26-50,151-159); GetIndexStatus without a ledger filter is correct on the dedicated single-node server.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1897 (comment)

[shipfox-ai]

This PR rewrites the EN-1748 cross-store alignment E2E regression to (1) establish a bounded native-index lag via GetIndexStatus, (2) stop the load producers once the lag threshold is reached, and (3) keep the not(ts[_,_]) complement read and its zero-rows assertion. The rewrite is a clear improvement in error handling and uses the documented Eventually(func(g Gomega) ...) pattern, but both retained findings reduce to one structural problem: the lag evidence is gathered and consumed at a different instant than the read whose guard it is supposed to establish, so the test no longer reliably reaches the torn-snapshot state it exists to detect. Verified against internal/query/aligned_snapshot.go, internal/application/ctrl/controller_default.go, AGENTS.md, and docs/technical/contributing/testing.md / ai-review.md.

Recommendation: request changes. The gap is fixable with a modest change (synchronize the lag proof with the read's horizon capture, and/or restore a read-under-live-lag case, and/or provide mutation evidence).

Standards

1. [Major] The original regression trigger — reading the complement while commits land under live lag — was replaced, not preserved

tests/e2e/business/cross_store_snapshot_alignment_test.go:129-143 (removed code at old lines ~116-155 in the diff).

AGENTS.md:78: "Regression tests are additive across production triggers: adding coverage for a new caller or failure path must not replace the existing regression trigger," and docs/technical/contributing/testing.md:26-29 requires keeping an independent case for the original trigger rather than replacing it. The pre-PR spec read the not(ts[_,_]) complement repeatedly while apply pressure was active and the fold was lagging — the concurrent commit+read configuration that actually exercises the torn pair. The rewrite does Eventually(lag >= 32) → stopPressure() → a single read, so the read now happens only after producers stop and in-flight batches drain; the read-under-live-lag configuration no longer exists anywhere in this spec. Relatedly, testing.md:36-38 asks for mutation evidence ("temporarily removing or bypassing the guarded production call and confirming that the focused test fails") when practical; none is provided, and per the Spec finding below the mutation would plausibly not fail this test. Resolution: keep the lag-gated stop-then-read as a sibling case alongside a read issued while the fold is verifiably behind, or otherwise prove sensitivity (e.g., re-read lag immediately around the RPC, or mutation-check bypassing query.AlignedIndexSnapshot).

2. [Minor] In-flight Apply failures are silently discarded once producers stop

tests/e2e/business/cross_store_snapshot_alignment_test.go:106-113.

if _, err := client.Apply(ctx, ...); err != nil {
    select {
    case <-stop:
        return
    default:
        Fail(fmt.Sprintf("apply pressure failed: %v", err))
    }
}

Closing stop does not cancel ctx or the in-flight RPC — it only prevents the next batch — so a genuine failure of a batch already in flight after stopPressure() (line 135) is silently swallowed. This deviates from AGENTS.md:68 ("Do not ignore errors. Handle them explicitly...") and can mask a server failing under load behind the shutdown path, misdiagnosing an E2E failure. Surface every Apply error; use stop only for the pre-batch check at line 86. (This is still an improvement over the old unconditional return, hence minor.)

Spec

1. [Major] The lag proof is not synchronized with the read's horizon capture, so the preserved assertion may never be exercised against a torn pair

tests/e2e/business/cross_store_snapshot_alignment_test.go:129-143.

PR bullets: "establish a bounded native-index lag before reading" and "preserve the assertion that an index-complement read never surfaces committed rows missing from a READY index." The test observes GetIndexStatus().Lag >= 32 at one poll instant (GetIndexStatus itself samples the read-index snapshot before opening its main-store handle — internal/application/ctrl/controller_default.go:1218-1232 — so even that observation is not a synchronized pair), then stopPressure() waits for up to 4 in-flight 20-tx batches, and only then does ListTransactionsFiltered capture its main-store horizon inside the RPC (OpenQueryHandle → AlignedIndexSnapshot, internal/query/aligned_snapshot.go:76, via internal/ctrl/list_entities.go:102). Nothing re-establishes that the fold is still behind the captured horizon; after pressure stops, the indexer is free to drain the ~32-sequence backlog during exactly that window. In that execution the index snapshot and the main handle are aligned by the time the read runs, so the complement is empty even if query.AlignedIndexSnapshot were bypassed entirely — the literal zero-row assertion passes while the EN-1748 regression goes undetected. This is precisely the failure mode docs/technical/contributing/ai-review.md:87 warns about: "verify that a rewritten test still reaches the original regression state rather than only a nearby proxy state." The "preserve" bullet is therefore only partially satisfied: the assertion text is preserved, but its regression sensitivity is not. Resolution: order the lag evidence against the read's horizon capture (e.g., verify the fold is still behind the sequence the read pins, or hold the fold behind until after the handle is captured), or provide mutation evidence that bypassing alignment reliably fails this spec. Note the removed explicit backlog drain is not a problem — on the success path the aligned read implies a drained fold — and the lag threshold being a minimum rather than a bound matches the PR's stated intent of avoiding an unbounded backlog.

Both sections otherwise have no further confirmed material findings; GLM's duplicate-select and sync.Once observations were rejected as style nitpicks with no correctness or test-risk impact.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

Standards

[P2] Rewritten concurrency test no longer proves it reaches the regression state — the single read can execute against an already-aligned fold

tests/e2e/business/cross_store_snapshot_alignment_test.go:132-151

The test observes Lag >= 32 (lines 132-137), calls stopPressure() (line 138), re-checks Lag > 0 via a second, independent GetIndexStatus (lines 139-141), then performs exactly one ListTransactionsFiltered read (line 146). None of these observations is bound to the snapshot the read uses:

• GetIndexStatus pins its own point-in-time views (internal/application/ctrl/controller_default.go:1215), so its Lag says nothing about later calls.
• The read path (ListTransactionsFrom → query.OpenQueryHandle → listEntities → query.AlignedIndexSnapshot, internal/query/aligned_snapshot.go:75-121) takes a fresh read-index snapshot and only enters the alignment-wait path if lastIndexed < mainSeq at that instant. After stopPressure() the indexer keeps folding concurrently (producers are not even joined until the deferred cleanup), so the remaining lag can drain between the line-141 check and the read's snapshot. In that execution AlignedIndexSnapshot returns immediately with lastIndexed >= mainSeq, the complement is empty, and the test passes — including a build where the alignment wait is removed entirely. The former repeated probing under live pressure gave many opportunities to hit the lagging state; the single unbound read materially weakens branch proof, contrary to docs/technical/contributing/ai-review.md ("verify that a rewritten test still reaches the original regression state rather than only a nearby proxy state") and docs/technical/contributing/testing.md ("Regression preservation and branch proof").

The race also cuts the other way: if the fold fully drains between the Eventually success and the line-141 check, the test fails spuriously with "fold caught up while stopping pressure — inconclusive".

Resolution: synchronize on evidence that the exercised read captured a main-store horizon ahead of the fold (e.g., tie the read's mainSeq/observed alignment wait to the lag previously observed, or assert an observable alignment wait/state transition on that same read), or supply mutation/sensitivity evidence that disabling the alignment path makes this focused test fail.

[Minor] Intentional error discard lacks the required justification comment

tests/e2e/business/cross_store_snapshot_alignment_test.go:107-114

if _, err := client.Apply(ctx, servicepb.UnsignedApplyRequest("", reqs...)); err != nil {
    select {
    case <-stop:
        return
    default:
        Fail(fmt.Sprintf("apply pressure failed: %v", err))
    }
}

Dropping a genuine apply failure because shutdown raced it is a reasonable intent, but AGENTS.md ("Do not ignore errors. Handle them explicitly, or use _ = ... with a justification comment when intentional") requires the intentional-discard path to carry a justification comment. Add one (e.g. "error raced shutdown — load validity is enforced by the lag gate") or surface the error.

Spec

No spec artifact was available to this verification pass; the requirement below is quoted from the candidate report. Its technical substance was independently verified against the code.

[P2] The lag observation is not bound to the snapshot exercised by the read

tests/e2e/business/cross_store_snapshot_alignment_test.go:132-146

Spec: "make the existing EN-1748 cross-store alignment regression establish a bounded 32-sequence native-index lag before reading."

The test does establish Lag >= 32 before reading, but the lag is observed through independent GetIndexStatus snapshots while ListTransactionsFiltered later opens a new main-store handle in OpenQueryHandle and takes a fresh read-index snapshot in AlignedIndexSnapshot. The asynchronous indexer can drain the remaining lag after either status snapshot but before that handle's snapshot is taken; in that execution AlignedIndexSnapshot finds lastIndexed >= mainSeq immediately and the empty complement proves only an already-aligned read. The second Lag > 0 check narrows but does not close this window, so the stabilized test can pass without exercising EN-1748, and the PR's central requirement is only probabilistically covered. Resolution is the same as the Standards finding above: bind the tested read to the observed lag, or assert an observable alignment wait tied to that same read.

---

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR bounds the cross-store alignment E2E's apply pressure (correct goal, and the new controller-level unit test is a genuinely good, deterministic addition), but the rewrite replaces the EN-1748 complement probe's regression trigger — reading under live fold lag — with a single read issued after pressure stops, gated only by an unordered point-in-time lag sample. As a result the E2E can pass even if the alignment wait were bypassed entirely, and the "preserve the assertion" spec bullet is only met in surface form. Recommendation: request changes. The fix is contained: keep at least one complement read racing the live fold (as the sibling cross_store_value_skew_test.go still does for its trigger), or otherwise bind the lag observation to the read's horizon capture, plus two one-line comment fixes.

Standards

[Hard] The rewritten E2E no longer proves it reaches the regression state: the lag sample is never bound to the read it guards

tests/e2e/business/cross_store_snapshot_alignment_test.go:136-152. The flow is: Eventually(GetLag() >= 32) → stopPressure() → one more GetIndexStatus asserting Lag > 0 (line 145) → a single ListTransactionsFiltered (line 150). Verified against the code:

• GetIndexStatus composes Lag from two independently pinned views — it opens the read-index snapshot first and the main-store handle afterward (internal/application/ctrl/controller_default.go:1218-1234) — so it is a point sample, unordered with anything that follows.
• The read opens a fresh main-store handle and captures its own mainSeq inside the RPC; AlignedIndexSnapshot (internal/query/aligned_snapshot.go:75-118, reached via internal/query/executor.go:128) waits only if lastIndexed < mainSeq at that instant.

After stopPressure() nothing holds the fold back, so the indexer keeps draining the ≥32-sequence backlog through the Lag > 0 re-check and into the read's in-RPC horizon capture. If it catches up in that gap, the read runs against an already-aligned pair, the complement is legitimately empty, and the test passes with AlignedIndexSnapshot's wait removed entirely — failing the mutation-sensitivity requirement (docs/technical/contributing/testing.md:40-42) and "a regression test is a guard for a specific production path, not merely an example that reaches nearby code" (testing.md:21-24). The same race cuts the other way: if the fold drains between the Eventually success and the second sample, line 145 fails spuriously with "fold caught up while stopping pressure — inconclusive". The old spec (removed hunk) issued the not(ts[_,_]) complement repeatedly while commits were landing, which made reaching the torn state overwhelmingly likely; that configuration no longer exists for this trigger. AGENTS.md:78 ("Regression tests are additive across production triggers ... must not replace the existing regression trigger") is breached. Note for precision: the read-under-live-lag pattern survives in tests/e2e/business/cross_store_value_skew_test.go:133-158, but that spec guards a different trigger (accounts metadata index-vs-enrichment skew), not the transaction complement — so the replacement is real. The new unit test (internal/application/ctrl/list_transactions_alignment_test.go) does deterministically exercise the torn-wait path at the controller level and partially offsets the loss, but it does not restore the end-to-end read-under-live-lag trigger this spec existed to preserve.

[Minor] Intentional error discards lack the required justification comments

• internal/application/ctrl/list_transactions_alignment_test.go:58: t.Cleanup(func() { _ = rs.Close() }) — no reason given. AGENTS.md:68 and docs/technical/contributing/conventions.md:61-62 require intentional _ = ... discards to carry a justification (e.g. // Best effort cleanup).
• tests/e2e/business/cross_store_snapshot_alignment_test.go:107-113: in the worker's stop branch, when stop is closed and the context has expired, an in-flight Apply error is silently swallowed via early return, with no comment marking the discard as intentional. Same rule applies.

Rejected as below the materiality bar: the duplicated not(ts[_,_]) construction across the two tests and the long generated-proto accessor chain (*notTimestamp.GetNot()...) — test-fixture idiom with no correctness or test-risk impact.

Spec

The spec is the PR description (four bullets; GLM's "no spec available" was incorrect).

[Major] Bullet 4 ("preserve the assertion that an index-complement read never surfaces committed rows missing from a READY index") is preserved only in surface form, and bullet 1 ("establish a bounded 32-sequence native-index lag before reading") is satisfied without the lag being bound to the read

The assertion text survives — the not(ts[_,_]) filter, the empty-complement check, and the Fail on any returned row (cross_store_snapshot_alignment_test.go:150-153). But the trigger configuration that makes the assertion meaningful was replaced: no commit lands once the read begins, and the Lag > 0 re-check (line 145) does not close the gap because GetIndexStatus's lag is a point sample (controller_default.go:1218-1234) while the read captures its horizon independently (aligned_snapshot.go:75-118). The test can therefore pass without ever reaching the torn-snapshot state the assertion exists to detect — i.e. it verifies the invariant given alignment works, rather than detecting its absence. This is the same defect as the Standards hard finding, assessed against the PR's own contract; bullets 2 (correctness-probe framing of minimumObservedLag = 32, not a benchmark) and 3 (producers stop at threshold) are implemented as described and are not findings.

Verified clean, for the record: the new unit test is fully deterministic and branch-specific (first call: mainSeq=1 vs lastIndexed=0 → blocks in WaitForSequence → context.DeadlineExceeded, correctly %w-wrapped; second call: fold row + WriteProgress(1) in one atomic batch → aligned → empty complement); it is additive and is the only deterministic exercise of the torn-wait path; ListTransactionsFiltered does send the filter, so AlignmentOwed is true and the read routes through AlignedIndexSnapshot/MainHorizonKeep; no time.Sleep, t.Parallel() present, the Eventually(func(g Gomega) ...) form matches testing.md, and the apply-error stop/handler rework is an improvement.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

Both reports were verified line-by-line against diff.patch and the production code (internal/query/aligned_snapshot.go, internal/application/ctrl/controller_default.go, internal/application/ctrl/list_entities.go) before retention. The two changed files are test-only; no production behavior changes. The material issue both reviewers independently surfaced survives verification: the rewritten EN-1748 E2E regression no longer binds its lag observation to the horizon the guarded read actually uses, so the "preserved" assertion can pass without ever reaching the torn-snapshot state the spec exists to exercise. The two error-handling findings are small but confirmed breaches of the documented standard. The long accessor chain flagged by both reviewers was rejected as a style nitpick with no concrete impact, and the new controller-level unit test — while not named in the PR body — was verified correct and deterministic, and is treated as benign additive scope. Recommendation: request changes — restore a complement read issued while the fold is genuinely still lagging (or otherwise bind the read's horizon to the observed lag), or provide the mutation/sensitivity evidence docs/technical/contributing/testing.md asks for.

Standards

[Major] Rewritten E2E no longer guarantees it reaches the guarded torn-snapshot state

tests/e2e/business/cross_store_snapshot_alignment_test.go:130-151. Verified end-to-end: GetIndexStatus computes Lag from two independently opened point-in-time snapshots (controller_default.go:1142-1165); stopPressure() (line 142) only closes the stop channel — workers are not joined (pressure.Wait() runs only in the deferred waitForPressure), so in-flight 20-tx batches land after the sample; and query.AlignedIndexSnapshot (internal/query/aligned_snapshot.go:122) waits only if lastIndexed < mainSeq at the instant the read captures its horizon. Nothing re-binds the ≥32 lag sample to that horizon. If the indexer drains the backlog during the stop→recheck→read gap, the pair is already aligned when the read runs, the complement is legitimately empty, and the test passes even if the alignment wait were bypassed — the EN-1748 guard is unexercised. The race also cuts the other way: the Lag > 0 recheck at line 145 can fail spuriously ("fold caught up while stopping pressure — inconclusive"). This replaces the old trigger, which issued the not(ts[_,_]) complement read repeatedly while commits landed under live fold lag, breaching AGENTS.md:78 ("adding coverage … must not replace the existing regression trigger"), docs/technical/contributing/testing.md ("a regression test is a guard for a specific production path, not merely an example that reaches nearby code"; mutation-check when practical), and docs/technical/contributing/ai-review.md:87 ("verify that a rewritten test still reaches the original regression state rather than only a nearby proxy state"). Partially mitigated by the new deterministic unit test TestListTransactions_AlignsComplementWithPrimaryHorizon, which drives the controller torn-wait path, but that does not restore the E2E read-under-live-lag configuration. No mutation/sensitivity evidence accompanies the rewrite.

[Minor] Discarded error lacks the required justification comment

internal/application/ctrl/list_transactions_alignment_test.go:59: t.Cleanup(func() { _ = rs.Close() }). AGENTS.md:68 ("use _ = ... with a justification comment when intentional") and docs/technical/contributing/conventions.md:61-62 (explicit discard with reason, e.g. _ = file.Close() // Best effort cleanup) require a stated reason; none is present.

[Minor] In-flight apply error silently dropped without justification

tests/e2e/business/cross_store_snapshot_alignment_test.go:108-116. In the worker's case <-stop branch, when ctx.Err() != nil the function returns with the client.Apply error discarded and no comment marking the discard intentional, breaching AGENTS.md:68. A real server fault coinciding with context cancellation becomes invisible to the spec. (Impact is bounded — a canceled context already fails the spec — hence Minor, but the discard should carry a justification comment.)

Spec

Verdict against the four PR-body bullets: bullet 2 (correctness probe, portable minimumObservedLag = 32 with explanatory comment) and bullet 3 (stopPressure() runs immediately after the threshold) are implemented as stated. Bullets 1 and 4 are met in letter only; see the finding below. The new internal/application/ctrl/list_transactions_alignment_test.go is a test-only, purpose-aligned addition not named in the bullets — benign scope, verified correct (seed fixture, torn-wait context.DeadlineExceeded, then aligned empty complement), and treated as offsetting coverage, not a finding.

[Major] Bullet 1 (bounded 32-sequence lag "before reading") and bullet 4 ("preserve the assertion") hold in form, not as a binding guarantee

tests/e2e/business/cross_store_snapshot_alignment_test.go:141-151. The Eventually(... GetLag() >= 32) gate and the preserved assertion text (notTs filter, zero-rows check, and the Fail("not(ts[_,_]) returned %d row(s)...") message) are all present. But the lag is a point-in-time sample from an unordered GetIndexStatus call, and after stopPressure() the read opens a fresh main handle inside the RPC — capturing a mainSeq that includes any in-flight batch that landed after the sample — while the fold keeps draining through the Lag > 0 recheck into the read. Nothing ties the observed ≥32 lag to the horizon the read actually uses, so if the fold catches up in the gap, AlignedIndexSnapshot returns immediately, the complement is legitimately empty, and the "preserved" guard passes without exercising the torn state it exists to detect. The old read-under-live-lag configuration that guaranteed a torn window was replaced rather than retained alongside the new gate. The new controller-level unit test deterministically covers the torn-wait path and partially offsets this, but it is not the E2E regression bullet 4 says is preserved. Suggested direction: issue at least one complement read while the fold is verifiably still behind the horizon the read will capture (e.g., under residual pressure or with the fold's progress held below the read's captured sequence), or supply the mutation evidence testing.md requests showing the focused E2E still fails if the guarded alignment path is bypassed.

---

Clean, for the record: no time.Sleep introduced; Eventually(func(g Gomega) ...) form matches testing.md; t.Parallel() present; grouped const declarations conform to conventions.md; the unit test's error paths assert branch-specifically with require.ErrorIs(t, err, context.DeadlineExceeded), and Fail messages uniquely identify their branches; seedCreatedTransaction extracts shared fixture logic without duplication.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR (EN-1946) adds a deterministic controller-level regression for the EN-1748 cross-store alignment wait and rewrites the e2e spec so the complement read is gated on a sampled fold lag of ≥32 before producers stop. The new unit test is genuinely valuable — it deterministically drives the torn-snapshot state (main store at seq 1, index progress at 0) and proves the read blocks to context.DeadlineExceeded before folding — and the e2e apply-error handling is now correctly branch-distinguishing. However, the rewrite deletes the e2e's read-under-live-pressure loop and replaces it with a single read after producers stop, so the e2e's phantom-rows assertion can now pass vacuously when the fold drains during the sample→read gap. Since AGENTS.md explicitly requires that regression triggers not be replaced, and both models converged on this as the material gap, the recommendation is: request changes — restore a read-under-live-lag probe (or bind the lag evidence to the horizon the read actually captures), plus two one-line justification comments.

Standards

1. [Major] The existing e2e regression trigger is replaced, not preserved. tests/e2e/business/cross_store_snapshot_alignment_test.go:137–157: the removed loop issued the not(ts[_,_]) complement read repeatedly while apply pressure kept the fold lagging (~25s of torn-snapshot reads, with an Expect(served).To(BeNumerically(">", 0)) inconclusiveness guard). The rewrite instead does Eventually(GetLag() >= 32) → stopPressure() → one ListTransactionsFiltered after producers stop. This breaches AGENTS.md ("Regression tests are additive across production triggers … must not replace the existing regression trigger") and docs/technical/contributing/testing.md §"Regression preservation and branch proof" ("preserve an independent case for the original trigger"). The new controller unit test does not restore this: it covers the alignment wait at the controller layer, not the e2e read-under-live-pressure trigger.

2. [Minor] Intentionally discarded errors lack justification comments. internal/application/ctrl/list_transactions_alignment_test.go:69: t.Cleanup(func() { _ = rs.Close() }). tests/e2e/business/cross_store_snapshot_alignment_test.go:111–118: in case <-stop, when ctx.Err() != nil the worker returns, dropping the client.Apply error without comment. AGENTS.md: "Do not ignore errors. Handle them explicitly, or use _ = ... with a justification comment when intentional." Both behaviors are defensible (best-effort teardown; cancellation-induced apply failure), but each needs a one-line reason per the documented convention.

No other confirmed material standards findings. The long protobuf accessor chain (*notTimestamp.GetNot()...), the 42 literal, and the seedCreatedTransaction parameter shape are protobuf-idiom/style nitpicks without correctness or risky-test impact and are not retained.

Spec

Spec source: the PR body (S1 bounded 32-sequence lag before reading; S3 stop producers once the lag is reached; S4 preserve the complement-reads-no-phantoms assertion). Jira is unreachable from this review.

1. [Major] S1/S4 are met in letter, but the regression state the e2e exists to detect is no longer guaranteed to be reached. The lag gate (GetIndexStatus().GetLag() >= 32) and the assertion text are both present, but the lag sample and the read's horizon are taken at different instants. GetIndexStatus builds lag from two unordered point-in-time views (internal/application/ctrl/controller_default.go:1147–1167: read-index snapshot first, then the primary handle), and after stopPressure() (line 146) nothing sustains the fold's lag — waitForPressure/pressure.Wait() run only in deferred teardown (lines 130–134) — so up to four in-flight 20-tx batches can drain the backlog during the sample→read gap. The read captures its horizon fresh inside the RPC (internal/query/aligned_snapshot.go: ReadLastSequence, then waits only while lastIndexed < mainSeq), so if the fold catches up first, the snapshot is already aligned, the complement is legitimately empty, and the spec passes even if the alignment wait were bypassed entirely. The diff's own comment concedes this and delegates sensitivity to the new unit test. Concretely: keep one complement read issued while pressure is still running (retryable not-caught-up rejections tolerated, as before), or have the read's response echo the fold cursor it aligned against so the assertion is bound to a captured lagging horizon.

2. [Minor, informational] Unrequested but benign additions. The controller unit test and the apply-worker error rewrite are not among the PR's stated bullets. Both are purpose-aligned with EN-1748 and the unit test partially offsets finding 1 by deterministically driving the torn-wait path (blocked read → DeadlineExceeded, then aligned empty complement). They should simply be named in the PR description; no change required on their account.

Beyond these, S2 (minimumObservedLag = 32 with an explicit not-a-benchmark comment) and S3 (stopPressure() immediately after the threshold) are implemented as specified, and the phantom-rows Fail message is preserved verbatim with its branch-identifying text.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[gfyrag]

Final ownership pass on 2b57fc6: the bound cross-store regression now asserts lag immediately before the read and retains the direct deterministic controller branch proof. All inline findings are resolved. Canonical validation PASS; exact final review APPROVE (LOW).

[shipfox-ai]

This PR (EN-1946) reworks the EN-1748 cross-store alignment e2e so apply pressure stops once the read-store fold observes a 32-sequence lag (minimumObservedLag, BeNumerically(">=", 32)), replaces the old in-pressure probe loop with a single post-pressure complement read, and adds a deterministic controller-level regression (internal/application/ctrl/list_transactions_alignment_test.go) that proves a complement read blocks (context.DeadlineExceeded) while the projection cursor is behind, then returns an aligned empty result once the timestamp row and progress fold atomically. I verified the retained complement-read assertion, the alignment wait against internal/query/aligned_snapshot.go (AlignedIndexSnapshot really does block until the fold cursor covers the main handle), the producer stop/error triage, and the fixture writes (WriteTransactionTimestampIndex, WriteProgress, NotifyProgress) against the actual code. All three PR-summary bullets are implemented; both changed files are test-only. Recommendation: approve with comments — the findings below are minor.

Standards

1. [Minor — documented standard] Intentional error discard missing its justification comment. internal/application/ctrl/list_transactions_alignment_test.go:59: t.Cleanup(func() { _ = rs.Close() }). AGENTS.md ("Do not ignore errors… use _ = ... with a justification comment when intentional") and the docs/technical/contributing/conventions.md error-handling example (_ = file.Close() // Best effort cleanup) both require the reason. The sibling helper the new test itself uses follows the pattern exactly (controller_default_ledger_not_found_test.go:98–99: // Best-effort test cleanup: nothing to assert on close.). One-line fix.

2. [Minor — judgement call] The e2e regression's in-pressure read trigger was replaced rather than preserved. The deleted loop issued the not(ts[_,_]) probe on every pass while sustained apply pressure raced the lagging fold; the new code stops producers (cross_store_snapshot_alignment_test.go:143) and then makes a single read (line 150). docs/technical/contributing/testing.md:26–28 ("Regression preservation and branch proof") calls for keeping an independent case for the original trigger rather than replacing it with the new one. The complement-read assertion survives, the read still lands while the fold is ≥32 sequences behind in most schedules (so the alignment-wait path is still exercised), and the new deterministic controller test pins the lagging horizon directly — but the sustained read-vs-apply interleaving the e2e uniquely covered is gone, and detection there is now schedule-dependent (see Spec 2). Mitigated, not eliminated.

Otherwise clean: the reworked producer error path (select on stop, distinguishing "in-flight apply failed after pressure stopped" from "apply pressure failed") correctly satisfies testing.md's branch-proof rule; the Eventually(func(g Gomega)…) drain/lag wait replaces both the silent return ^uint64(0) error swallow and the 25s sleep-free deadline per convention; t.Parallel(), t.Cleanup, and require.NoError usage conform. Baseline smells (five-link proto accessor walk at list_transactions_alignment_test.go:76, seven-parameter seedCreatedTransaction with a single call site) are style-only and not retained.

Spec

1. [Low] "Bound the load at a 32-sequence native-index lag" is a floor, not a cap. tests/e2e/business/cross_store_snapshot_alignment_test.go:73,142 asserts GetLag() >= 32 (verified: Lag = LastLogSequence − LastIndexedSequence, i.e. native fold lag), then stops producers. Workers observe stop only between 20-transaction batches and lag is polled every 10 ms, so the lag at stop overshoots 32 by up to roughly one batch per worker. Compared to the old "hundreds of sequences behind under sustained pressure", the load is genuinely bounded and the old 3-minute drain wait is no longer needed — but a reviewer should read the "32" figure as a stop threshold, not an exact bound.

2. [Low] The retained e2e complement read is a schedule-dependent probe. After stopPressure() the fold may legitimately drain before the read at cross_store_snapshot_alignment_test.go:150 executes (the code's own comment concedes this), in which case the zero-row assertion passes without exercising the alignment wait at all. This is consistent with the PR's stated pairing — the deterministic sensitivity is carried by TestListTransactions_AlignsComplementWithPrimaryHorizon, which pins the lagging horizon exactly (projection cursor 0, timestamp row absent → context.DeadlineExceeded at list_transactions_alignment_test.go:99–104) — but the e2e assertion alone can no longer be relied on to catch a regression of the wait mechanism; it is effectively a smoke check whose bite depends on the fold still lagging when the read lands.

No missing requirements beyond the caveats above, and no scope creep: the producer error triage and waitForPressure teardown are direct consequences of "stop pressure producers", and nothing outside the two test files changed.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.