fix(reads): align index inspection to Raft horizon (EN-1946)

[gfyrag]

Stack 4/7 for EN-1946. Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming.

[NumaryBot]

✅ Approve — automated review

The alignment, pinned version resolution, lifecycle handling, and horizon trimming are consistent. The previously reported removed-index classification issue is fixed.

No findings.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1891 (comment)

[codecov]

Codecov Report

❌ Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.40%. Comparing base (ee81a54) to head (6cef75d).

Files with missing lines	Patch %	Lines
internal/application/ctrl/controller_default.go	69.56%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@                           Coverage Diff                           @@
##           fix/en-1946-prepared-query-snapshot    #1891      +/-   ##
=======================================================================
+ Coverage                                77.38%   77.40%   +0.02%     
=======================================================================
  Files                                      458      458              
  Lines                                    48631    48643      +12     
=======================================================================
+ Hits                                     37632    37654      +22     
+ Misses                                    7839     7825      -14     
- Partials                                  3160     3164       +4     

Flag	Coverage Δ
e2e	77.40% <80.55%> (+0.02%)	⬆️
scenario	77.40% <80.55%> (+0.02%)	⬆️
unit	77.40% <80.55%> (+0.02%)	⬆️

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 aligns InspectIndex with the fixed-main-snapshot read path: the routed controller now carries the ReadIndex barrier, the controller reserves event history, opens one main snapshot, waits for the read-index certificate via query.AlignedIndexSnapshot, resolves the index version through PinnedVersionResolver at the main horizon, and passes that horizon into readstore.InspectIndex so all three inspect modes trim membership events above the pin. The store-level trimming logic itself is correct (verified byte offsets, group reset, and skip semantics), and the reservation/lease lifecycle is safe. However, the new readiness gate collapses the removed-vs-building distinction that the repository documents as a hard read-path contract, and the only new test bypasses the changed production path — the exact gap that lets the gate bug through. Recommendation: request changes (one P1 fix in InspectIndex's error classification, plus composed-path regression coverage).

Standards

1. [P1][blocking] Removed index misclassified as ErrIndexBuilding, violating the documented read-path contract — internal/application/ctrl/controller_default.go:1115-1124 (DefaultController.InspectIndex).

The new gate is:

version, primed, err := readstore.PinnedVersionResolver(snap, ledgerInfo.GetName(), mainSeq)(indexes.Canonical(indexID))
...
if !primed || version.Version == 0 {
    return nil, &domain.BusinessError{Err: &domain.ErrIndexBuilding{...}}
}

docs/technical/architecture/subsystems/read-path/read-snapshot-consistency.md:181-192 ("Index removal is a rejection, not a stale read") requires the opposite split when the main-store registry still declares the index at the read's pin (which InspectIndex already verified via indexes.Find at controller_default.go:1086-1094): an absent/tombstoned IndexVersionState after alignment means the index was removed after the pin and must return ErrIndexNotFound; only a record present at version 0 means ErrIndexBuilding. query.requireIndexReady (internal/query/compile.go:1588-1607) implements exactly this distinction, and PinnedVersionResolver deliberately surfaces primed for it (see internal/application/indexbuilder/removal_pin_probe_test.go). The new code computes primed and then discards it. Concrete impact: drop an index after the main snapshot is fixed and the projection folds the removal — the inspection returns "index building" for a build that will never occur, telling the client to wait forever. The pre-change code had the same collapsed behavior, but this PR rewrites this exact gate with the distinguishing signal in hand and the contract explicitly documented; fix by returning ErrIndexNotFound for !primed and ErrIndexBuilding only for version.Version == 0, or better, reuse a shared readiness classifier with requireIndexReady so inspection and compiled queries cannot diverge.

2. [P2] New regression test guards only the leaf trimming function, not the changed production path — internal/storage/readstore/iterator_event_resolve_test.go:303-397 (TestInspectIndex_ResolvesMembershipAtMainHorizon).

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." The test constructs InspectParams by hand — supplying the DB, version, and HorizonSequence directly to readstore.InspectIndex — so it proves the trimming arithmetic but would stay green if the routed barrier propagation, OpenReservedQueryHandle, AlignedIndexSnapshot, pin-aware version resolution, or the controller's error classification (finding 1) were broken. The existing e2e business test (tests/e2e/business/inspect_index_test.go, e2e build tag) only covers converged happy-path reads and exercises neither main/projection skew nor lifecycle errors. Add a focused controller/routed-level regression that establishes main/projection skew and proves barrier alignment, pinned trimming, and the removed-vs-building outcomes of finding 1.

3. [P3] Stale code comments contradict the new behavior — internal/application/ctrl/controller_default.go:1108-1110 and internal/storage/readstore/inspect.go:62-66.

Per AGENTS.md ("Interface/behavior change: update relevant code comments"): the edited readiness comment still frames the gate as reading "the local replica's IndexVersionState.CurrentVersion", but the code now reads PinnedVersionResolver(...)(...) at mainSeq, whose result can deliberately differ from CurrentVersion; and forEachLiveGroup/countLiveGroups still describe "whose latest event is an ADD" without the new "latest event at or below the horizon" semantics. Update both while fixing finding 1.

Spec

Spec basis: no linked issue or spec file is available (PR body references external tracker EN-1946); the reviewable contract is the PR's own stated behavior ("Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming") together with the read-path contract the PR's updated docs now reference.

1. [P1][blocking] Removed-after-pin requests return ErrIndexBuilding instead of ErrIndexNotFound — internal/application/ctrl/controller_default.go:1120-1124.

Same code as Standards finding 1, judged against the spec: the PR's stated purpose is to align InspectIndex with the pinned read path, and the pinned read path's documented semantics (read-snapshot-consistency.md:181-192) require that a drop folded after the fixed main snapshot produce an honest rejection (ErrIndexNotFound), not a "build in progress" that can never complete. Until the gate splits the two cases, the PR's alignment claim is not actually met for the removal lifecycle.

2. [P2] Missing regression coverage for the alignment behavior the PR claims — internal/storage/readstore/iterator_event_resolve_test.go:303-397.

The spec promises barrier alignment, pinned version resolution, and horizon trimming at the InspectIndex API level; the added test exercises only the leaf store function with a caller-supplied horizon, and no focused controller/routed test covers the composed path (stale vs default barrier behavior, reservation→lease handoff, removed/activation-after-pin outcomes). This coverage gap is why the spec finding 1 above is currently undetected. Add the composed-path regression described in Standards finding 2.

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 aligns InspectIndex with the cross-projection read contract: it reserves event history up front (OpenReservedQueryHandle), waits for a read-index Raft certificate over the fixed main snapshot (AlignedIndexSnapshot), resolves the servable index version through PinnedVersionResolver at the snapshot's native sequence, and forwards that sequence as HorizonSequence so all three inspect modes reconstruct membership at the main horizon instead of the projection head. The routing layer correctly propagates the local barrier horizon, the reservation-to-lease handoff matches the established pattern in the query path (double release is safe — Lease.Release uses once.Do), the horizon trimming in forEachLiveGroup is correct (big-endian sequence matches the key layout, and the added live = false reset prevents a fully-trimmed group from inheriting the previous group's liveness), and subsystem documentation is updated. However, the new readiness gate conflates a removed index with one still building, contradicting the documented removal semantics and the existing requireIndexReady precedent, and the only added test exercises the readstore helper directly rather than the changed production path.

Recommendation: request changes (one P2 contract violation; everything else is minor).

Standards

[P3][non-blocking] The new regression test does not guard the changed production path

internal/storage/readstore/iterator_event_resolve_test.go:303-399 — TestInspectIndex_ResolvesMembershipAtMainHorizon calls readstore.InspectIndex directly with HorizonSequence: 20, so it validates only the horizon-trimming mechanism. It stays green if any of the production wiring is removed: the barrier propagation in internal/bootstrap/controller_routed.go:450-454, the OpenReservedQueryHandle/AlignedIndexSnapshot handoff and the PinnedVersionResolver + HorizonSequence: mainSeq forwarding in internal/application/ctrl/controller_default.go:1044-1160. This conflicts with 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."

Resolution: add controller-level coverage that fails when the barrier, pin-aware version resolution, lease handoff, or HorizonSequence forwarding is bypassed — including a removed-after-pin case (see Spec below), which would have caught the gate conflation.

[P3][non-blocking] Read-path subsystem README not updated

docs/technical/architecture/subsystems/read-path/README.md:4-8 — the overview still says only "Filtered account/transaction queries and every log query additionally wait for the read index to align with that horizon before iterating it." InspectIndex is now another projection-aligned consumer, and read-snapshot-consistency.md was updated to say so, but AGENTS.md §Documentation maintenance requires updating "the matching docs/technical/architecture/ subsystem documentation and its README" for a non-obvious mechanism/invariant.

Resolution: mention index inspection's fixed-horizon alignment in the read-path README overview.

The following candidate findings were rejected after verification: the "stale version-state comment" in controller_default.go (the retained comment is still accurate — PinnedVersionResolver does read the version state through the snapshot); the duplicated InspectParams closures in the new test, the zero-HorizonSequence "speculative generality" note, and the bare-uint64 horizon (style judgement calls with no correctness or risk impact).

Spec

No issue/spec reference accompanies this change (single commit e7f8c2c1), so the axis is grounded in the repository's documented contracts.

[P2][blocking] Index removal after the pinned horizon is misreported as an in-progress build

internal/application/ctrl/controller_default.go:1120-1124 — the new gate is if !primed || version.Version == 0 { return ErrIndexBuilding }. readstore.PinnedVersionResolver deliberately returns primed == false both for an absent record and for a dropped (tombstoned) index (internal/storage/readstore/store.go:644-669). The registry lookup at controller_default.go:1086-1093 resolves from the pinned main handle, so when the main snapshot predates a DropIndex and the aligned read-index snapshot has already folded the tombstone and purged the rows, the registry check passes while primed is false — and the client gets ErrIndexBuilding for an index that will never finish building.

This directly violates docs/technical/architecture/subsystems/read-path/read-snapshot-consistency.md:181-192 ("Index removal is a rejection, not a stale read... requireIndexReady reads it that way and returns ErrIndexNotFound; a record present at version 0 still means a build in progress and returns ErrIndexBuilding. Telling a client to wait for readiness that will never arrive is the failure this prevents") and contradicts the existing precedent in internal/query/compile.go:1588-1602, where requireIndexReady maps !primed after a passing registry check to ErrIndexNotFound with exactly this rationale.

Resolution: split the gate — !primed → ErrIndexNotFound (removal, matching requireIndexReady), reserving ErrIndexBuilding for a primed record whose resolved version is 0 (initial build, or a version activating after the pin). Add the controller-level regression for the projection-ahead/drop-after-pin window described under Standards.

Otherwise no missing requirements or scope creep: the pin-aware version resolution, membership trimming across all three inspect modes, lease-protected event history, and the documentation updates match the alignment contract the PR implements.

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 aligns InspectIndex with the pinned-horizon read machinery (Raft certificate via AlignedIndexSnapshot, PinnedVersionResolver, native-sequence membership trimming) and updates two subsystem docs. The core mechanism is sound: the horizon trim in forEachLiveGroup (including the new live = false group reset) is correct, cursor pagination stays deterministic at a fixed horizon, deferred cleanup order matches the established executor pattern, and the routed-barrier wiring mirrors the other aligned consumers. However, the new readiness gate misclassifies a removed index as "building", contradicting the pinned read-path contract this same PR documents — a one-line fix, but it must land before merge. Recommendation: request changes.

Standards

[P1] Removed indexes are classified as ErrIndexBuilding instead of ErrIndexNotFound

internal/application/ctrl/controller_default.go:1120 — the new gate if !primed || version.Version == 0 maps both cases to ErrIndexBuilding. readstore.PinnedVersionResolver (internal/storage/readstore/store.go:644-684) returns primed=false for both an absent record and a tombstoned (dropped) one; only a present record with ActivationSequence > pin yields primed=true, Version=0.

The flow makes the removed case reachable: the registry lookup (indexes.Find, controller_default.go:1085-1097) runs against the fixed main-store handle and passes while the aligned projection snapshot returned by AlignedIndexSnapshot has already folded a later drop. This is exactly the scenario docs/technical/architecture/subsystems/read-path/read-snapshot-consistency.md:181-190 ("Index removal is a rejection, not a stale read" — updated elsewhere in this diff's subsystem docs) and requireIndexReady (internal/query/compile.go:1588-1607) specify: an absent record after a successful registry lookup at the pin can only mean removal, and must return ErrIndexNotFound; ErrIndexBuilding is reserved for a present resolved version of zero. InspectIndex now sits on the same aligned/pinned machinery but classifies !primed as building, so clients are told to wait/retry for a build that can never finish. Fix: map !primed to &domain.ErrIndexNotFound{...} and keep ErrIndexBuilding for primed && version.Version == 0.

[P2] Regression test covers only the leaf, not the changed production path

internal/storage/readstore/iterator_event_resolve_test.go:303 (TestInspectIndex_ResolvesMembershipAtMainHorizon) supplies Reader, Version, and HorizonSequence directly to readstore.InspectIndex. It proves trimming in all three modes, but cannot fail if the reservation/lease handoff, barrier propagation, alignment, pin-aware version resolution, or the readiness classification above breaks. Existing InspectIndex e2e coverage (tests/e2e/business/inspect_index_test.go) exercises only converged state. This violates docs/technical/contributing/testing.md ("a regression test is a guard for a specific production path"), and the P1 finding above demonstrates the cost: the misclassification lives in exactly this unguarded composed path. Add controller/routed coverage with main/projection skew, including drop-after-pin (expecting ErrIndexNotFound) and activation-after-pin (expecting ErrIndexBuilding) outcomes.

[P3] Documentation not updated to match the new behavior

Per AGENTS.md:82-86, documentation is part of the change when behavior or interfaces change:

• docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152 still states "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — the diff changed all three claims (Raft barrier wait, pin-resolved version, horizon-trimmed membership), and this section now contradicts the implementation. The two other touched docs (indexes.md, read-snapshot-consistency.md) were updated correctly; this one was missed.
• docs/technical/architecture/subsystems/read-path/README.md:7-8 still lists only filtered entity/log queries as consumers that "wait for the read index to align with that horizon"; InspectIndex is now one too.
• internal/storage/readstore/inspect.go:66-69 and :131-132 — the forEachLiveGroup/countLiveGroups comments still describe "latest"/"current" membership rather than membership at or below the horizon, though both functions now take a horizon parameter.

Spec

Spec source: the PR's stated intent ("aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming") together with the pinned read-path contract in docs/technical/architecture/subsystems/read-path/read-snapshot-consistency.md.

[P1] Removed-index classification violates the pinned-horizon contract

Same location as the Standards P1: internal/application/ctrl/controller_default.go:1120. The spec text this PR operates under is explicit: "an absent record can only mean the index was removed after the pin. requireIndexReady reads it that way and returns ErrIndexNotFound; a record present at version 0 still means a build in progress and returns ErrIndexBuilding. Telling a client to wait for readiness that will never arrive is the failure this prevents." The new gate merges the two cases and returns ErrIndexBuilding for !primed, so a dropped index whose removal the projection has folded but the main snapshot has not is reported as building. Resolve by matching requireIndexReady: !primed → ErrIndexNotFound, present-but-zero version → ErrIndexBuilding.

[P2] The composed InspectIndex alignment path remains unguarded

Same finding as the Standards P2, framed against the PR's scope: the PR promises end-to-end alignment of InspectIndex (barrier → reservation/lease → alignment → pinned version → trimmed scan), but the added regression test (iterator_event_resolve_test.go:303) exercises only the leaf scan with hand-supplied parameters. A regression at any seam above the leaf — including the misclassified removal case — would ship green. Add coverage that drives the controller/routed path with projection-ahead skew.

---

Not retained: GLM's speculative-generality remark on the HorizonSequence == 0 fallback, its test-duplication suggestion, and its comment-wording nit — style-level judgement calls with no correctness, security, compatibility, 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]

This PR aligns InspectIndex with the fixed Raft/main-store horizon used by the other projection reads: it reserves event history up front, waits for the read projection's certificate over the main snapshot, pins the snapshot's native sequence, resolves the index version through PinnedVersionResolver, and trims membership-event scans to that horizon across all three inspect modes. The trimming implementation in readstore is correct (verified the event-key layout math, the group-reset semantics, and the reservation→lease handoff), and the doc updates match the code. However, the controller's error classification breaks the established pinned-read contract for removed indexes, and the new regression test guards only the leaf, leaving the composed alignment path unprotected. Recommendation: request changes.

Standards

[P2][blocking] Preserve the removed-index classification at the pinned horizon
internal/application/ctrl/controller_default.go:1115-1124. The new gate if !primed || version.Version == 0 maps both states to ErrIndexBuilding. PinnedVersionResolver returns primed=false for an absent or tombstoned version state (internal/storage/readstore/store.go:644-668), and requireIndexReady implements the documented split — !primed → ErrIndexNotFound, version 0 → ErrIndexBuilding (internal/query/compile.go:1588-1606). docs/technical/architecture/subsystems/read-path/read-snapshot-consistency.md ("Index removal is a rejection, not a stale read") states this contract explicitly and names "telling a client to wait for readiness that will never arrive" as the failure it prevents. The scenario is reachable: the registry lookup reads the main-store handle, which can still list the index at the read's pin while the aligned projection has already folded a later drop and tombstoned the version state. Impact: clients are told to wait for a build that will never finish, contradicting the documented pinned-read behavior. Resolution: return ErrIndexNotFound for !primed and reserve ErrIndexBuilding for a present state whose resolved version is zero, preferably via shared classification with requireIndexReady.

[P2][blocking] Test the changed production path, not only its leaf
internal/storage/readstore/iterator_event_resolve_test.go:303-397. The new test calls readstore.InspectIndex directly with a manually supplied Version and HorizonSequence. It remains green if routed barrier propagation, the reservation→lease handoff, AlignedIndexSnapshot acquisition, pinned version resolution, or the controller's forwarding of mainSeq breaks — and it passes despite the classification defect above. Existing InspectIndex e2e coverage (tests/e2e/business/inspect_index_test.go) exercises only converged happy paths and generic errors. This violates docs/technical/contributing/testing.md:23: "A regression test is a guard for a specific production path, not merely an example that reaches nearby code." Impact: the composed consistency regression can merge undetected. Resolution: add focused controller/routed-path coverage with main/projection skew, including pinned activation and removed-versus-building outcomes; retain the all-mode leaf test.

[P3][non-blocking] Update the owning subsystem README
docs/technical/architecture/subsystems/read-path/README.md:3-9. The overview still lists only filtered account/transaction queries and log queries as projection-aligned consumers and omits InspectIndex, although the subsystem doc (read-snapshot-consistency.md) was updated to name it as a projection consumer. AGENTS.md:86 requires a new non-obvious mechanism to update the matching subsystem documentation "and its README". Resolution: add index inspection's fixed-horizon alignment to the overview.

[P3][non-blocking] Make the stale behavior comments horizon-aware
internal/application/ctrl/controller_default.go:1108-1115 — the gate comment is a half-edited artifact: "We read the version\nstate through it" has a dangling antecedent (the direct snap := ctrl.readStore.NewSnapshot() was removed), it still attributes the gate to the live IndexVersionState.CurrentVersion (EN-1323), and it never describes the pin-aware resolution or the !primed arm. internal/storage/readstore/inspect.go:62-66,124 — "whose latest event is an ADD — i.e. current membership" and "counts current members" now describe the projection head, while the functions resolve the latest event at or below horizon. AGENTS.md:89 requires interface/behavior changes to update relevant code comments. Resolution: describe the pinned-horizon semantics and the zero-horizon latest-view exception.

Spec

Spec (PR body): "Stack 4/7 for EN-1946. Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming." All three named mechanisms are implemented: the fixed horizon and certificate wait (AlignedIndexSnapshot + routed withLocalBarrierHorizon wiring), pinned version resolution (PinnedVersionResolver at the pin, with ActivationSequence withholding), and native-sequence trimming (HorizonSequence applied to distinct values, facets, and both summary counts). The live = false reset is required for correct trimming of fully-trimmed groups, and the doc edits are the demanded documentation maintenance, not scope creep.

[P2][blocking] Removed-index classification breaks the alignment claim
internal/application/ctrl/controller_default.go:1115-1124. "Aligns InspectIndex with … pinned index version" implies faithful reuse of the established pinned-read semantics, but the controller collapses the removed-index state (primed=false: absent or tombstoned, internal/storage/readstore/store.go:644-668) into ErrIndexBuilding, diverging from the split requireIndexReady implements (internal/query/compile.go:1588-1606) and from the contract documented in read-snapshot-consistency.md. Impact: after a drop folded past the main snapshot, inspection rejects with a transient "building" error instead of the definitive ErrIndexNotFound. Resolution: mirror the requireIndexReady classification.

[P2][blocking] The composed alignment path lacks regression coverage
internal/storage/readstore/iterator_event_resolve_test.go:303-397. The spec's API-level guarantee (results reconstructed at the fixed main horizon) is only exercised by supplying Version/HorizonSequence to the leaf store function. No controller- or routed-level test drives main/projection skew through OpenReservedQueryHandle → AlignedIndexSnapshot → pinned resolution → trimming, and no test distinguishes removed from building at the pin. Impact: the spec requirement is not regression-protected; the classification defect above passes the added test. Resolution: add a skew test at the controller/routed layer; keep the leaf test for all-mode trimming.

No other missing requirements or scope creep confirmed. Independently inspected risk areas beyond both reports — event-key horizon byte offsets, group-reset semantics, releaseHold double-release idempotency, and the reservation→lease handoff ordering — and found no additional material issues.

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

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).

Summary: #1891 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).

Summary: #1891 (comment)

[shipfox-ai]

This PR aligns InspectIndex with the cross-store read-consistency machinery already used by indexed queries: the routed controller now forwards the local read barrier, the controller reserves event history via OpenReservedQueryHandle, waits for a Raft certificate through AlignedIndexSnapshot, resolves the index version with PinnedVersionResolver at the main snapshot's native sequence, and trims membership events to that horizon in every inspect mode. The trimming logic itself is correct (verified against the append-only event key layout and group-boundary handling), and the subsystem docs that were touched are accurate. However, the alignment contract is incomplete at the index-lifecycle level, and the new regression test only guards the storage leaf, leaving the composed production path unguarded. Recommendation: request changes.

Standards

[P1] The new regression test guards only the storage leaf; the production alignment path is unguarded — internal/storage/readstore/iterator_event_resolve_test.go:303-397
TestInspectIndex_ResolvesMembershipAtMainHorizon hand-builds InspectParams (including HorizonSequence: 20) and calls readstore.InspectIndex directly. It stays green if the routed controller stops propagating the barrier context (internal/bootstrap/controller_routed.go:449-455), if the default controller stops using OpenReservedQueryHandle/AlignedIndexSnapshot/PinnedVersionResolver (internal/application/ctrl/controller_default.go:1044,1100,1115), or if HorizonSequence: mainSeq stops being forwarded (controller_default.go:1151-1161). The existing e2e coverage (tests/e2e/business/inspect_index_test.go) only exercises converged happy paths and error modes — never a read projection positioned relative to a fixed main snapshot. This violates the documented rule that a regression test guards a specific production path with assertions that uniquely identify the intended branch (docs/technical/contributing/testing.md; AGENTS.md §Engineering conventions). Add a deterministic controller/routing-level regression with a projection ahead of the fixed main snapshot, where mutating each new handoff (barrier forwarding, reservation→lease, pin resolution, horizon forwarding) makes it fail.

[P2] Subsystem docs updated, but the read-path README and typed-metadata.md were missed — docs/technical/architecture/subsystems/read-path/README.md:3-8, docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152
AGENTS.md requires updating the matching subsystem documentation and its README. The README still says only "Filtered account/transaction queries and every log query" wait for the read index to align with the horizon — InspectIndex is now an aligned projection consumer too. typed-metadata.md:150-152 still claims InspectIndex "pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — all three claims changed (Raft certificate wait, pin-resolved version, horizon-trimmed membership).

[P3] Stale code comments contradict the new pin/horizon semantics — internal/application/ctrl/controller_default.go:1106-1114, internal/storage/readstore/inspect.go:61-63,130
The retained comment in InspectIndex still says "the local replica's IndexVersionState.CurrentVersion decides whether queries can be served (EN-1323). We read the version state through it…" — but the code now resolves the version through PinnedVersionResolver(snap, …, mainSeq), which deliberately differs from the live CurrentVersion (versions activated above the pin report not-live), and the antecedent of "through it" (the removed snap := ctrl.readStore.NewSnapshot()) no longer exists. The !primed → ErrIndexNotFound arm is undocumented. Similarly, the forEachLiveGroup/countLiveGroups comments ("whose latest event is an ADD — i.e. current membership", "counts current members") now describe the projection head while both functions resolve membership at or below horizon; the zero-horizon latest-view exception is also undocumented. AGENTS.md: "Interface/behavior change: update relevant code comments."

Spec

[P1] A post-horizon index lifecycle mutation still leaks into the fixed-horizon inspection — internal/application/ctrl/controller_default.go:1115-1124, with internal/storage/readstore/store.go:644-679 and internal/application/indexbuilder/index_config.go:505-559
The spec is "Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming." That holds for membership events, but not for index lifecycle. The read projection can be ahead of the fixed main snapshot and may already have folded a DropIndex (or a retype version promotion) committed after mainSeq, while the main handle still shows the index (indexes.Find passes). In that state: a tombstoned version state makes PinnedVersionResolver return primed=false regardless of the pin (store.go:660-666), so InspectIndex returns ErrIndexNotFound; a drop+recreate or retype promotion above the pin resolves Version=0, so it returns ErrIndexBuilding. Either way, an inspection whose fixed horizon had the index live and servable reports a different lifecycle state. The new lease protects event-GC history only; handleDroppedIndexLog unconditionally tombstones and range-purges the field keyspace (including the append-only event history) in its fold batch, so the projection cannot reconstruct the index as of mainSeq after the fold. Resolve by making lifecycle/version history pin-aware (and retaining the relevant keyspace while an older pin is live), or otherwise guarantee the accepted snapshot can reconstruct the index at mainSeq — and scope the alignment claim in docs/technical/architecture/subsystems/indexer/indexes.md and read-snapshot-consistency.md accordingly. Add a controller-level regression where the main handle is fixed before a later drop folds and the inspection still returns the pre-drop result.

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

[shipfox-ai]

Final review — PR #1891 (fix(reads): align index inspection to Raft horizon, EN-1946)

This PR extends the existing read-alignment machinery (OpenReservedQueryHandle → AlignedIndexSnapshot → PinnedVersionResolver) to InspectIndex: the routed controller now propagates the local Raft read barrier, the default controller pins the main snapshot's native sequence, resolves the servable index version at that pin, and passes the horizon into every inspect mode so post-horizon membership events (including existence rows) are trimmed. I verified both candidate reports against the diff and the code: the trimming logic in forEachLiveGroup is correct (including the essential live = false reset on group start, without which an all-post-horizon group would inherit liveness from its predecessor), the hold/lease handoff is safe (Lease.Release is sync.Once-guarded, so the internal releaseHold() in AlignedIndexSnapshot plus the caller's defer is intentional and idempotent), and the controller_routed.go change follows the established withLocalBarrierHorizon pattern. No correctness, security, or compatibility defect was found. Recommendation: approve with comments — one moderate test-coverage gap and one minor stale comment.

Standards

1. [Moderate] No regression test guards the production alignment wiring.
   internal/application/ctrl/controller_default.go:1099–1157, internal/bootstrap/controller_routed.go:449–456; standard: docs/technical/contributing/testing.md:23 — "A regression test is a guard for a specific production path, not merely an example that reaches nearby code."
   The only added test (TestInspectIndex_ResolvesMembershipAtMainHorizon, internal/storage/readstore/iterator_event_resolve_test.go:303) calls readstore.InspectIndex directly with HorizonSequence set by hand. It stays green if the controller drops HorizonSequence: mainSeq from InspectParams (controller_default.go:1155), stops resolving the version at the pin (PinnedVersionResolver(snap, ledger, mainSeq), controller_default.go:1115), or if RoutedController.InspectIndex stops applying withLocalBarrierHorizon (controller_routed.go:455). No controller-level or routed-level test exercises InspectIndex (the existing e2e tests/e2e/business/inspect_index_test.go does not construct an ahead-of-main projection), so the alignment behavior that is the entire point of this PR is unguarded at the layers where a future refactor would break it. Existing resolver coverage (version_activation_test.go) does pin the resolver itself, but not this wiring. Add a deterministic controller-path test that fails when the barrier propagation, aligned snapshot, or pinned resolver is removed.

2. [Minor] Half-edited readiness comment no longer describes the code it annotates.
   internal/application/ctrl/controller_default.go:1107–1114; standard: AGENTS.md:89 — "Interface/behavior change: update relevant code comments."
   The edit deleted "We take the snapshot FIRST and read the version" but kept "We read the version state through it", leaving it with no antecedent in the comment (the snapshot is now produced seven lines earlier by AlignedIndexSnapshot). The comment also still describes only the old GC-race rationale and does not mention the new pin semantics — that the version is resolved at mainSeq, so a version activated after the horizon is withheld and surfaced as ErrIndexBuilding. Update the comment to describe the certificate/pin flow.

Candidate findings not retained (verified and rejected)

• "HorizonSequence: 0 silently disables trimming" (Codex, hard): rejected. Zero-as-sentinel is the documented, established convention in this codebase (PinnedVersionResolver's pin of 0 means "no pin", internal/storage/readstore/store.go:642); the doc comment on InspectParams.HorizonSequence states it explicitly; the sole production caller always passes mainSeq from AlignedIndexSnapshot, and mainSeq == 0 implies an empty main store with no membership events. No current correctness impact — a design-judgement nit at most.
• "indexes.md says a post-snapshot-activated replica returns 'not built locally' but the code returns ErrIndexBuilding" (GLM): rejected as a contradiction. "Not built locally" is the document's pre-existing informal phrasing for the ErrIndexBuilding refusal (the old sentence described exactly that error for CurrentVersion == 0), and the code path (store.go:665–666 → version.Version == 0 → ErrIndexBuilding, a transient retryable business error) matches the documented behavior of refusing rather than scanning.
• Deferred _ = x.Close() without justification comments (GLM): rejected. This is the pervasive, established idiom across the codebase (e.g., inspect.go, list_entities.go, aligned_snapshot.go); flagging new instances of it is noise.
• Primitive-obsession / data-clump / middle-man / test-IIFE smells (GLM): rejected as style judgement calls with no correctness or risk impact.

Spec

No confirmed material finding. The implementation matches the PR's stated scope — "Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming" — and nothing in the diff exceeds it. Verified specifically: the barrier is bound only for locally served reads via the existing withLocalBarrierHorizon helper; AlignedIndexSnapshot waits for the projection certificate ≥ the fixed main horizon and returns its native sequence; the pinned resolver withholds a version whose ActivationSequence exceeds the pin (surfaced as the transient ErrIndexBuilding, consistent with the resolver's documented contract and existing unit coverage in version_activation_test.go); tombstoned/absent state maps to ErrIndexNotFound per the resolver's "must read exactly like the removed index" contract; and the added test genuinely pins trimming for distinct values, facets, and summary counts (including existence rows). The coverage gap for the controller/routing path is recorded under Standards above and is not duplicated here.

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 aligns InspectIndex with a fixed Raft/main-store horizon: it reserves event history before opening the main snapshot (OpenReservedQueryHandle), waits for the read projection's certificate and pins the main snapshot's native sequence (AlignedIndexSnapshot), resolves the servable index version at that pin (PinnedVersionResolver), threads the sequence into InspectIndex as a trimming horizon, and propagates the routed read barrier. The production wiring itself is correct — I verified the !primed → ErrIndexNotFound / version 0 → ErrIndexBuilding gate matches requireIndexReady semantics, the live = false group reset in forEachLiveGroup correctly prevents liveness leakage when an entire group sits above the horizon, and the lease/release ordering matches existing call sites. However, the change ships with no regression test that guards the changed production path, and several authoritative docs and code comments still describe the pre-change behavior. Recommendation: request changes — add a composed-path regression and refresh the stale documentation.

Standards

[P2] New regression test guards only the leaf scan, not the changed production path — internal/storage/readstore/iterator_event_resolve_test.go:303 (TestInspectIndex_ResolvesMembershipAtMainHorizon).

docs/technical/contributing/testing.md ("Regression preservation and branch proof") states: "A regression test is a guard for a specific production path, not merely an example that reaches nearby code." The new test builds InspectParams by hand (Reader: s.DB(), Version: 1, HorizonSequence: 20) and calls readstore.InspectIndex directly. It proves the horizon-trimming arithmetic across all three modes, but it stays green if the production wiring regresses at any of the seams this PR touches: the reservation→lease handoff (controller_default.go:1044), AlignedIndexSnapshot acquisition (controller_default.go:1100), pin-aware version resolution (controller_default.go:1115), the controller's forwarding of HorizonSequence: mainSeq (controller_default.go:1151-1162), or the routed barrier propagation (internal/bootstrap/controller_routed.go:449-456). The existing E2E inspection tests (tests/e2e/business/inspect_index_test.go) explicitly wait for convergence before asserting, so they exercise only the converged state, not main/projection skew. Impact: the API-level consistency guarantee this PR exists to provide can silently regress at any newly wired seam while all focused tests pass. Resolution: retain the leaf all-mode test and add a controller/routed-level regression that creates main/read-projection skew and drives the request through the real path, covering barrier propagation, pinned version selection, horizon trimming, and the removed-versus-building classification at the pin.

[P2] Stale documentation and code comments contradict the changed behavior — AGENTS.md ("Documentation maintenance") requires behavior changes to update the owning subsystem documentation, its README, and relevant code comments. Four locations still describe pre-change semantics:

• internal/application/ctrl/controller_default.go:1108-1115: the half-edited readiness comment still says "the local replica's IndexVersionState.CurrentVersion decides whether queries can be served (EN-1323). We read the version state through it…" — the antecedent of "it" (the old snap := ctrl.readStore.NewSnapshot()) was removed, and the gate now reads PinnedVersionResolver(snap, …, mainSeq), whose resolved version can deliberately differ from CurrentVersion (activation after the pin) and whose !primed arm (→ ErrIndexNotFound) the comment never describes.
• internal/storage/readstore/inspect.go:61-66: forEachLiveGroup still documents "whose latest event is an ADD — i.e. current membership"; it now resolves the latest event at or below horizon. The horizon > 0 && … > horizon { continue } skip and the new live = false group reset are undocumented.
• internal/storage/readstore/inspect.go:127: countLiveGroups still says "counts current members" — same stale framing.
• docs/technical/architecture/subsystems/read-path/typed-metadata.md:149-153: the "Index inspection" section still states "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — all three claims changed in this PR, and the section now contradicts both the implementation and the updated read-snapshot-consistency.md.
• docs/technical/architecture/subsystems/read-path/README.md:3-9: still lists only filtered account/transaction queries and log queries as consumers that "wait for the read index to align with that horizon"; InspectIndex is now one, per this PR's own read-snapshot-consistency.md edit, but the owning README was not updated.

Impact: authoritative guidance materially misstates the new consistency contract for future contributors and agents. Resolution: update all five locations to describe fixed-main-horizon alignment, pin-resolved versions, and latest-at-or-below-horizon membership (including the zero-horizon exception).

Spec

No spec is available for this PR (it references external tracker EN-1946; there is no linked GitHub issue or in-repo spec), so this axis has no confirmed material finding. The end-to-end regression-coverage concern raised against the PR's stated goal is substantive and is captured above under Standards.

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

[shipfox-ai]

Final review — PR #1891 (InspectIndex horizon alignment)

This PR makes InspectIndex a projection consumer: the routed controller now applies the local read barrier, the default controller reserves event history via OpenReservedQueryHandle, waits for alignment via query.AlignedIndexSnapshot, resolves the servable version with PinnedVersionResolver at the main snapshot's native sequence, and forwards that sequence as InspectParams.HorizonSequence, which forEachLiveGroup uses to trim membership events above the horizon. I verified the wiring end to end and found the core implementation correct: the double release of the reservation hold is safe (Lease.Release is sync.Once-guarded, and the release-on-align/release-on-error split matches the established executor.go pattern); the stale path behaves exactly as the new indexes.md text claims (no quorum barrier, local alignment retained); the horizon offset in the event-key suffix (key[tpos+1:tpos+9]) matches the key layout; and readstore.InspectIndex has exactly one production caller. However, the PR leaves documentation that directly describes InspectIndex contradicting the new behavior, the changed production wiring has no regression that would fail if it were removed, and the new pinning contract has a drop/recreate edge case that should be either protected or explicitly documented. Recommendation: request changes — the fixes are small and localized, but the doc contradictions and the missing regression guard violate explicit repo standards (AGENTS.md §Documentation maintenance, docs/technical/contributing/testing.md).

Standards

1. [Must fix] No regression test guards the changed production wiring

TestInspectIndex_ResolvesMembershipAtMainHorizon (internal/storage/readstore/iterator_event_resolve_test.go:303) drives readstore.InspectIndex directly with a hand-set HorizonSequence: 20. The controller and routed wiring this PR actually changes — withLocalBarrierHorizon in internal/bootstrap/controller_routed.go:450-455, the OpenReservedQueryHandle → AlignedIndexSnapshot handoff and the HorizonSequence: mainSeq forwarding in internal/application/ctrl/controller_default.go:1041-1150 — are exercised by no test that would fail if they were removed or reverted. The e2e suite (tests/e2e/business/inspect_index_test.go) covers converged happy paths only, so a regression to an unbarriered or projection-head inspection would pass all added coverage. This violates 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 expectation at testing.md:40-42. Add a focused test that drives InspectIndex with the projection ahead of a fixed main snapshot and proves the endpoint returns the pinned result (retaining the lower-level all-mode test, which is good coverage of the trimming itself).

2. [Must fix] typed-metadata.md and the read-path README now contradict the implementation

• docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-151 still says "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version." All three claims are now false: the certificate wait goes through AlignedIndexSnapshot, the version is pin-resolved via PinnedVersionResolver (a version activating after the pin resolves as not-yet-live, not as the locally served one), and membership is trimmed at HorizonSequence.
• docs/technical/architecture/subsystems/read-path/README.md:5-6 still names only "Filtered account/transaction queries and every log query" as consumers that "wait for the read index to align with that horizon"; InspectIndex is now one too, per the PR's own read-snapshot-consistency.md edit.

AGENTS.md:86 requires updating the matching subsystem documentation "and its README" when behavior changes; the diff updated read-snapshot-consistency.md and indexer/indexes.md but missed the two documents that most directly describe this endpoint.

3. [Should fix] Behavior changed, code comments not

• internal/storage/readstore/inspect.go:67-69: forEachLiveGroup's doc still reads "whose latest event is an ADD — i.e. current membership", and inspect.go:129 still says countLiveGroups "counts current members" — both now resolve membership at or below the horizon parameter (inspect.go:110-112).
• internal/application/ctrl/controller_default.go:1107-1112: the gate comment still attributes readiness to "the local replica's IndexVersionState.CurrentVersion", but the gate now reads the pin-resolved version, which deliberately differs from the local current version when activation happens after the pin (that case resolves as not primed → ErrIndexNotFound).

AGENTS.md:89 ("Interface/behavior change: update relevant code comments").

Spec

1. [Should fix] A post-horizon drop/recreate in the projection is not pinned out of the inspection

The stated goal is that InspectIndex reflects a fixed main-store horizon with a pinned index version. The aligned projection snapshot is explicitly allowed to be ahead of the pin (controller_default.go:1098-1100), and if it has folded a DropIndex committed after the pin, handleDroppedIndexLog purges every version's rows and tombstones IndexVersionState in the same batch (internal/application/indexbuilder/index_config.go:510-524); PinnedVersionResolver treats the tombstone as absent regardless of the pin (internal/storage/readstore/store.go:684-692), so InspectIndex returns ErrIndexNotFound for an index that demonstrably exists with data at its fixed main horizon. If a post-pin recreation's backfill has also completed, completeBackfill leaves ActivationSequence at zero for backfill-built versions (internal/application/indexbuilder/backfill.go:1272-1277), so the resolver selects the new incarnation; its keyspace only contains events stamped with post-pin sequences, which the new trimming then removes — yielding plausible-but-empty statistics instead of the pinned incarnation's data. Native-sequence trimming cannot repair either case because the old keyspace was purged or a different version was selected.

Context that tempers but does not eliminate this: the prepared-query/filtered-read path has the identical exposure (internal/query/executor.go:173 resolves versions with the same PinnedVersionResolver over the same aligned snapshot), so InspectIndex is now consistent with its family rather than uniquely broken. Resolution options: preserve horizon-resolvable incarnation/version state and rows while pinned reads can still need them, reject the inspection when the resolved incarnation cannot be proven to exist at the pin, or — minimally — document this limitation explicitly in read-snapshot-consistency.md alongside the new InspectIndex paragraph, and add a regression covering main-snapshot-before-drop with the projection after drop, plus the drop/recreate case.

---

Rejected during synthesis: the "duplicated readiness classifier", "test closure duplication", and "HorizonSequence == 0 speculative generality" notes (style-level, no concrete impact; the zero-value fallback mirrors the documented SnapshotVersionResolver precedent); the traceability-chain finding (the diff already adds the mechanism explanation to the owning subsystem docs, which product-technical-traceability.md §5 accepts for decisions extending an existing, already-documented consistency mechanism); and GLM's claim that requireIndexReady lives at internal/query/compile.go:1578 without the same !primed split (it does — confirmed, the split matches, so no drift today).

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

[shipfox-ai]

Review of PR #1891 (a0583a2 — fix(reads): align index inspection to raft horizon)

The implementation itself is correct at the review target: routed barrier propagation (internal/bootstrap/controller_routed.go:449-456), the reservation-to-lease handoff (internal/application/ctrl/controller_default.go:1044, internal/query/aligned_snapshot.go:267-274), AlignedIndexSnapshot certificate wait, the correctly split readiness gate (!primed → ErrIndexNotFound, version.Version == 0 → ErrIndexBuilding at controller_default.go:1117-1130), and horizon trimming in all three inspect modes (internal/storage/readstore/inspect.go:73-133,178,243,313,351,359) were all verified against the code, including the group-reset/continue arithmetic in forEachLiveGroup and the idempotent double-release of the GC reservation. What falls short is validation and documentation: the PR's central behavior change — the composed InspectIndex alignment path — has no regression guard above the readstore leaf, and the repo's traceability and comment/doc-maintenance standards are not met. Recommendation: request changes (small, focused: add composed-path coverage, expand the PR description per the traceability standard, refresh the stale comments/docs).

Standards

1. [P2] Regression test guards the readstore leaf, not the changed production path. TestInspectIndex_ResolvesMembershipAtMainHorizon (internal/storage/readstore/iterator_event_resolve_test.go:303-399) hand-builds InspectParams{Version: 1, HorizonSequence: 20} and calls readstore.InspectIndex directly. It proves the trimming arithmetic but stays green if any new seam breaks: routed barrier propagation (internal/bootstrap/controller_routed.go:449-456), the OpenReservedQueryHandle reservation→lease handoff and AlignedIndexSnapshot acquisition (internal/application/ctrl/controller_default.go:1044,1100), PinnedVersionResolver (1115), or HorizonSequence: mainSeq forwarding (1157). No test invokes DefaultController.InspectIndex or RoutedController.InspectIndex under projection lag, and the e2e coverage (tests/e2e/business/inspect_index_test.go) waits for convergence via WaitForMetadataIndexReady (line 37) before inspecting — no main/projection skew is ever exercised. This violates 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 AGENTS.md's regression rule (assertions must uniquely identify the intended branch). Add routed/controller-level coverage whose observation fails when the wiring is bypassed.

2. [P2] Required traceability chain is missing from the PR description. docs/technical/contributing/product-technical-traceability.md requires significant consistency decisions to leave a repository-verifiable chain (need → limitation → requirement → decision → validation), including its minimum PR-description summary, and permits linking committed evidence instead of duplicating it. The complete PR description is the one-liner "Stack 4/7 for EN-1946. Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming." EN-1946 is not readable in-repo, and while the diff updates the subsystem docs with the chosen mechanism, neither the description nor a linked committed source establishes the observable failure being fixed, the alternatives considered, or the validation criterion. Add the required summary and link durable repository evidence.

3. [P3] Stale comments contradict the new gate semantics. AGENTS.md: "Interface/behavior change: update relevant code comments." internal/application/ctrl/controller_default.go:1107-1109 still reads "the local replica's IndexVersionState.CurrentVersion decides whether queries can be served (EN-1323). We read the version state through it…" — half-edited ("through it" lost its antecedent when the direct snapshot read was removed) and now wrong: line 1115 resolves the version through PinnedVersionResolver(snap, …, mainSeq), whose pin-resolved version can deliberately differ from live CurrentVersion. Similarly, internal/storage/readstore/inspect.go:67 ("latest event is an ADD — i.e. current membership") and :129 ("counts current members") still describe projection-head semantics, though both now resolve membership at or below horizon.

4. [P3] Subsystem documentation not updated for the changed inspection behavior. AGENTS.md makes documentation part of the change when behavior/interfaces change. docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152 still claims "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — all three claims are changed by this diff (aligned index snapshot at the main horizon, split not-found/building gate, pin-resolved version with horizon trimming), even though sibling docs updated in this PR (indexer/indexes.md, read-path/read-snapshot-consistency.md) describe the new behavior.

Spec

Spec basis: "Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming" (EN-1946), cross-checked against the repo's own contracts (read-snapshot-consistency.md "Index removal is a rejection, not a stale read"; requireIndexReady in internal/query/compile.go). All three mechanisms are implemented and verified correct at a0583a2, including the correctly split readiness gate (the earlier iteration that collapsed ErrIndexNotFound into ErrIndexBuilding is not present at this target).

1. [P2] The alignment claim is validated only below the controller/routing boundary. The spec's claim is about InspectIndex — the API — yet the only added test drives readstore.InspectIndex with synthetic parameters, and the existing AlignedIndexSnapshot/PinnedVersionResolver unit tests exercise those helpers independently. Removing the barrier propagation, the alignment call, or the pin-aware resolver from the InspectIndex path would not fail any test in this PR. The spec's "fixed Raft/main horizon and pinned-version" behavior therefore lacks composed-path regression proof; a routed/controller-level test under projection lag/fold-ahead (asserting the barrier is honored, the pin-aware readiness result is used, and post-horizon membership is excluded) would close the gap. Otherwise, no missing requirements and no scope creep were found; the doc edits are required documentation maintenance, not creep.

(One candidate finding was investigated and rejected: the HorizonSequence == 0 "no-trimming" fallback in inspect.go:114 is unreachable in production — native sequences start at 1 (internal/infra/state/fsmstate.go:74), so any ledger visible in the main snapshot yields mainSeq ≥ 1, and 0 is a documented sentinel. No correctness impact.)

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 makes InspectIndex a cross-store-aligned read: the routed controller attaches the local ReadIndex barrier horizon, the default controller reserves event history, waits for a projection Raft certificate covering the fixed main snapshot via query.AlignedIndexSnapshot, resolves the servable index version with PinnedVersionResolver at the snapshot's native sequence, and passes that sequence as HorizonSequence so every inspect mode reconstructs membership as of the main snapshot instead of the projection head. I verified the core mechanics against the code and they are sound: the horizon trim in forEachLiveGroup preserves "last op ≤ horizon wins" semantics (the added live = false reset is required once events can be skipped and is correctly placed), the !primed → ErrIndexNotFound / Version == 0 → ErrIndexBuilding mapping matches the documented PinnedVersionResolver contract, the double releaseHold() (inside AlignedIndexSnapshot on success plus the deferred call) is safe because Lease.Release is sync.Once-guarded and follows the existing executor/list-entities pattern, and withLocalBarrierHorizon correctly applies only to the local, linearizable path. However, the PR's central consistency guarantee is only validated at the leaf (readstore.InspectIndex with hand-injected Version/HorizonSequence), and several authoritative docs and code comments still describe the pre-change behavior.

Recommendation: request changes — the implementation is correct, but the composed production path is untested and the documentation contract (AGENTS.md "Documentation maintenance") is only partially met.

Standards

1. [P1] No regression test covers the composed InspectIndex alignment path — the PR's core guarantee can silently regress.
   Locations: internal/storage/readstore/iterator_event_resolve_test.go:303-397 (the only new test), wiring in internal/bootstrap/controller_routed.go:449-458 and internal/application/ctrl/controller_default.go:1041-1163.
   TestInspectIndex_ResolvesMembershipAtMainHorizon calls the leaf readstore.InspectIndex directly, supplying Version: 1 and HorizonSequence: 20 by hand. It stays green if the routed barrier propagation (withLocalBarrierHorizon), the OpenReservedQueryHandle reservation, the AlignedIndexSnapshot wait/lease handoff, pin-aware version resolution, the !primed/Version == 0 gate, or the HorizonSequence: mainSeq forwarding is removed or broken — none of these seams has an InspectIndex-specific test. I confirmed there is no controller-level InspectIndex test at all (internal/application/ctrl has no non-generated coverage), and tests/e2e/business/inspect_index_test.go explicitly waits for the index to converge before inspecting, so it never creates the main/projection skew this PR addresses. This violates docs/technical/contributing/testing.md:23 ("A regression test is a guard for a specific production path, not merely an example that reaches nearby code") and AGENTS.md's definition-of-done requirement to run tests appropriate to the touched subsystem.
   Resolution: keep the leaf all-mode test, and add a controller/routed-path regression that fixes a main snapshot, lets the read projection advance past it, invokes InspectIndex, and fails when the barrier → alignment → pinned-version → horizon-trim chain is bypassed (including the pin-activated "not built locally" branch).

2. [P2] Required documentation update is incomplete: the subsystem README and typed-metadata contract still describe the old behavior.
   Locations: docs/technical/architecture/subsystems/read-path/README.md:5-7; docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-153.
   The PR updated indexer/indexes.md and read-path/read-snapshot-consistency.md (the latter now correctly lists InspectIndex as a projection-aligned consumer), but AGENTS.md:86 requires updating "the matching subsystem documentation and its README". The README still says only "Filtered account/transaction queries and every log query additionally wait for the read index to align", contradicting the updated read-snapshot-consistency.md. typed-metadata.md's "Index inspection" section still says "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — it is now a pin-resolved version gated on activation sequence, with horizon-bounded membership.
   Resolution: update both so they describe fixed-main-horizon alignment, pin-resolved version selection (including the version-activated-after-pin → "not built locally" case), and horizon-bounded membership.

3. [P3] Stale code comments left behind by the behavior change.
   Locations: internal/application/ctrl/controller_default.go:1107-1114; internal/storage/readstore/inspect.go:63-67 and inspect.go:129.
   AGENTS.md:89 requires updating relevant code comments on a behavior change. In controller_default.go, deleting "We take the snapshot FIRST and read the version" left "We read the version state through it" without its antecedent, the sentence still claims "the local replica's IndexVersionState.CurrentVersion decides" although the decision is now the pin-resolved version, and the trailing "atomic version switch … between the gate and the iteration" rationale describes the removed gate-then-NewSnapshot() sequence rather than the AlignedIndexSnapshot + PinnedVersionResolver mechanism. In inspect.go, forEachLiveGroup still says "latest event is an ADD — i.e. current membership" and countLiveGroups "counts current members", although both now resolve membership at or below horizon (and treat zero as latest-view).
   Resolution: reword the controller comment around the aligned snapshot and pin-resolved version, and qualify the two inspect.go comments with the horizon semantics (noting zero = latest view).

Spec

No usable spec is available for this PR: the commit (a4dbdae8 fix(reads): align index inspection to raft horizon) carries no issue reference, no matching spec file exists in the review context, and "EN-1946" appears only as a comment in pre-existing e2e tests (tests/e2e/business/cross_store_snapshot_alignment_test.go:143, cross_store_value_skew_test.go:136) that predate this change. The Spec axis therefore has no confirmed material finding: the implementation matches the stated commit intent (fixed Raft/main-store horizon, pinned index version, native-sequence trimming) as far as it can be verified from the diff, and no implementation mismatch or scope creep was found.

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 aligns InspectIndex with the read-path consistency contract: the routed controller now carries the ReadIndex barrier into the local controller, the default controller reserves event history before opening the main snapshot, waits for a projection Raft certificate covering it via AlignedIndexSnapshot, resolves the servable index version through PinnedVersionResolver at the pinned native sequence, and passes that sequence as HorizonSequence so every inspect mode trims post-horizon membership events. I verified the composed wiring against the checkout: the routed-barrier forwarding (internal/bootstrap/controller_routed.go:450-455) mirrors the established pattern, the double release of the reservation is safe (Lease.Release uses once.Do, internal/storage/readstore/read_lease.go:94-104), and the split gate (!primed → ErrIndexNotFound, version.Version == 0 → ErrIndexBuilding) matches requireIndexReady (internal/query/compile.go:1578-1612). The implementation itself is sound, but the changed production path has no regression guard that would fail if that wiring were removed, and the owning-subsystem docs and two sets of code comments are stale. Recommendation: request changes (one blocking coverage gap; the rest are non-blocking comments-level fixes).

Standards

1. [P2][blocking] The new regression test guards only the leaf; the changed production wiring is unguarded — internal/storage/readstore/iterator_event_resolve_test.go:303-399. TestInspectIndex_ResolvesMembershipAtMainHorizon calls readstore.InspectIndex directly with hand-supplied Version and HorizonSequence: 20. It exercises horizon trimming in isolation (and AlignedIndexSnapshot and PinnedVersionResolver each have their own standalone unit tests), but nothing composes the changed path: RoutedController.InspectIndex → withLocalBarrierHorizon → DefaultController.InspectIndex → OpenReservedQueryHandle → AlignedIndexSnapshot → PinnedVersionResolver(snap, …, mainSeq) → InspectParams{HorizonSequence: mainSeq} (internal/application/ctrl/controller_default.go:1050-1161, internal/bootstrap/controller_routed.go:450-455). If the HorizonSequence: mainSeq forwarding, the AlignedIndexSnapshot call, or the reservation handoff were removed or broken, this test — and the existing e2e inspect coverage (tests/e2e/business/inspect_index_test.go), which waits for full convergence before inspecting and therefore cannot distinguish aligned inspection from projection-head inspection — would all stay green, while the endpoint silently returned projection-head statistics instead of main-horizon ones. That violates docs/technical/contributing/testing.md: "A regression test is a guard for a specific production path, not merely an example that reaches nearby code." Resolution: add a controller/routed-level regression with a projection ahead of a fixed main snapshot (covering activation-after-pin → building/not-found outcomes as well as correct horizon-trimmed stats), and mutation-check that breaking either wiring point fails it.

2. [P3] Owning-subsystem docs not updated for the new InspectIndex contract — AGENTS.md:84 ("Documentation is part of the change…"). (a) docs/technical/architecture/subsystems/read-path/README.md:4-7 still names only filtered account/transaction queries and every log query as consumers that "wait for the read index to align", omitting InspectIndex, which this diff's own change to read-snapshot-consistency.md:152-159 now declares a projection consumer. (b) docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152 still says InspectIndex "pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — it no longer mentions the certificate wait, the pin-resolved version, or that membership events (including existence events behind the summary counts) are trimmed at the horizon. Both contradict the updated indexes.md and read-snapshot-consistency.md in the same PR.

3. [P3] Stale code comments describe the pre-change behavior — AGENTS.md:89 ("Interface/behavior change: update relevant code comments"). (a) internal/application/ctrl/controller_default.go:1107-1114 still frames the gate as the local replica's live IndexVersionState.CurrentVersion (EN-1323), carries a dangling "read the version state through it" antecedent (the snapshot previously taken there is gone), and never describes the pin-aware PinnedVersionResolver resolution or the new !primed → not-found arm. (b) internal/storage/readstore/inspect.go:58-61 (forEachLiveGroup) still says a group is live when its "latest event is an ADD — i.e. current membership", and inspect.go:127-128 (countLiveGroups) "counts current members" — both now resolve the latest event at or below horizon, and the new skip arm at inspect.go:115-117 is undocumented.

Spec

No spec is available for this change (single commit, a4dbdae8, no linked issue in the fixed point), so no spec findings were confirmed. On the observable behavior both reviewers converge with the code: the barrier is carried into the local controller, the main snapshot is fixed and certified before the scan, the version is resolved at the pin, and all three modes plus both summary existence counts trim at the horizon; error paths retain the reservation until return and successful alignment hands protection to the lease before releasing it. No missing or incorrect requirement was found.

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

[shipfox-ai]

Review summary — PR #1891

The change correctly implements what the PR body promises: RoutedController.InspectIndex now propagates the local ReadIndex barrier (internal/bootstrap/controller_routed.go:450-455, consumed at internal/query/aligned_snapshot.go:41-45), DefaultController.InspectIndex opens a reserved main handle and obtains a Raft-certified snapshot with AlignedIndexSnapshot, resolves the index version through PinnedVersionResolver at the main snapshot's native sequence, and forwards HorizonSequence: mainSeq so every distinct-value, facet, and summary scan trims membership events above the horizon. I independently verified the trimming byte math (seq big-endian at key[tpos+1:tpos+9], op at tpos+9), the group-reset semantics for fully-trimmed groups, the !primed→ErrIndexNotFound / version-0→ErrIndexBuilding split against PinnedVersionResolver's contract (store.go:664-706) and the requireIndexReady precedent (compile.go:1588-1603), and that the double-release of releaseHold (called inside AlignedIndexSnapshot and deferred by the caller) is safe because Lease.Release is sync.Once-guarded (read_lease.go:100-102). No correctness bug was found.

What falls short is validation and documentation synchronization: the PR's core cross-store guarantee is regression-guarded only at the leaf store function, and several authoritative comments/docs still describe the pre-change behavior. Recommendation: request changes — the code is sound, but per the repo's own standards (AGENTS.md:78,86,89, docs/technical/contributing/testing.md:21-24) the missing composed-path regression test and the stale authoritative docs must be addressed before merge.

Standards

P1 — Regression test guards the leaf, not the changed production path

internal/storage/readstore/iterator_event_resolve_test.go:303-397 (production path: internal/bootstrap/controller_routed.go:450-455, internal/application/ctrl/controller_default.go:1043-1159).

TestInspectIndex_ResolvesMembershipAtMainHorizon hand-builds InspectParams (Version: 1, HorizonSequence: 20) and calls readstore.InspectIndex directly. It proves the trimming arithmetic, but stays green if the routed barrier propagation (withLocalBarrierHorizon), the OpenReservedQueryHandle/AlignedIndexSnapshot handoff, pin-aware version resolution, or the HorizonSequence: mainSeq forwarding is dropped or broken. AGENTS.md:78 and testing.md:21-24 require regression tests to be guards for the specific production path and additive across production triggers. Existing e2e inspect coverage (tests/e2e/business/inspect_index_test.go) exercises only converged paths, so no existing test would catch a silent loss of the alignment guarantee either. Add a controller- or routed-level regression that fixes the main snapshot, advances the projection past it (including post-horizon events and a post-horizon version activation), invokes the production InspectIndex path, and asserts barrier propagation, pinned-version behavior, and horizon-trimmed results.

P2 — Stale readiness-gate comment misattributes the pinned resolution to live CurrentVersion

internal/application/ctrl/controller_default.go:1107-1114.

The gate now resolves through readstore.PinnedVersionResolver(snap, ledgerInfo.GetName(), mainSeq)(...) (line 1115), but the retained comment still reads "the local replica's IndexVersionState.CurrentVersion decides whether queries can be served (EN-1323) … so the gate and the subsequent Inspect scan observe the same point-in-time view — without this the atomic version switch could promote CurrentVersion between the gate and the iteration". "It" has a dangling antecedent (the direct snap := ctrl.readStore.NewSnapshot() line was deleted), the live-CurrentVersion attribution is wrong (resolution is pinned at mainSeq and deliberately may not equal CurrentVersion), and neither the pin nor the !primed/version-0 split is described. Violates AGENTS.md:89 ("Interface/behavior change: update relevant code comments"). The gate logic itself is correct; only the comment is stale.

P2 — Authoritative InspectIndex documentation not synchronized

docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152 and docs/technical/architecture/subsystems/read-path/README.md:3-6.

typed-metadata.md still states "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — all three claims changed: it now waits for a Raft certificate covering the fixed main snapshot, resolves the version at the pin (a nonzero current version whose activation is after the pin is rejected as building), and trims membership at the horizon. The README's consumer list ("Filtered account/transaction queries and every log query additionally wait for the read index to align") omits InspectIndex, which is now a projection consumer too — and the PR itself updates read-snapshot-consistency.md to say exactly that, making the two documents within the same subsystem contradict each other. Violates AGENTS.md:86 (subsystem documentation and its README).

P3 — Stale trimming comments on forEachLiveGroup/countLiveGroups

internal/storage/readstore/inspect.go:64-67,129.

Both helpers now take horizon and resolve the latest event at or below it, skipping later events (including the HorizonSequence == 0 latest-view exception), but the doc comments still say "whose latest event is an ADD — i.e. current membership" and "counts current members under an event prefix". Same AGENTS.md:89 rule; the InspectParams comment was updated while these two were not.

P3 — Traceability chain incomplete for a consistency decision

docs/technical/contributing/product-technical-traceability.md:21-25,78 explicitly lists consistency/distributed-system mechanisms as requiring the need → limitation → requirement → decision → validation chain, with durable in-repo evidence. The PR body is a single sentence (the external EN-1946 reference is not fetchable), and the changed docs describe the mechanism but not the operational need (what stale/incorrect inspect results occurred), the considered alternatives, or the observable validation. Record the chain in the owning subsystem documentation per product-technical-traceability.md:78.

Rejected as not material: the unused HorizonSequence == 0 fallback (documented API default, no production caller, no impact) and the repeated withMode closure shape in the new test (style).

Spec

All three named requirements from "Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming" are implemented as specified, with no scope creep and no wrong-looking implementations (the two documentation hunks beyond the literal body are required by the repo's documentation-maintenance policy, and the !primed→ErrIndexNotFound classification is required by the pinned-version contract the body invokes).

P2 — the end-to-end guarantee is unvalidated on the composed path. The spec line describes an end-to-end behavior of the InspectIndex API, but the only new regression test drives the leaf store function with hand-supplied Version/HorizonSequence (iterator_event_resolve_test.go:303-397). It would stay green if the barrier propagation, reservation→lease handoff, AlignedIndexSnapshot wait, pin-aware resolution, or HorizonSequence forwarding were broken or removed — i.e. the observable property the PR exists to provide is not validated by any test that exercises the composition. This is the spec-validation counterpart of the P1 Standards finding; same resolution: one controller/routed-level regression with main/projection skew.

Explicitly verified clean

Trimming byte-offset math and group-reset semantics; !primed/version-0 gate split against PinnedVersionResolver and the requireIndexReady precedent; routed barrier wiring through withLocalBarrierHorizon → ReadBarrierHorizon → mainAppliedHorizon; idempotent double-release of releaseHold (sync.Once); error wrapping, parallelism, and defer ordering in the new test. No material issue was found in these areas beyond the findings above.

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

[shipfox-ai]

Final review — PR #1891 (Align InspectIndex with the fixed main-store horizon)

This PR makes InspectIndex a projection-aligned consumer: it now reserves event history, waits for a Raft certificate covering its fixed main-store snapshot, resolves the index version through PinnedVersionResolver at the pinned native sequence, and trims membership events at that horizon across all three inspect modes. I verified the production wiring itself and found it sound: the double releaseHold() on the success path is safe (Lease.Release is sync.Once-guarded, read_lease.go:94-104), the split gate (!primed → ErrIndexNotFound, version 0 → ErrIndexBuilding) matches the documented removal-vs-building contract in read-snapshot-consistency.md, and the horizon-skip logic in forEachLiveGroup is correct given seq-ascending events within a group. What is not sound is the change's guard and documentation: the only new regression test exercises the storage leaf directly, so every layer this PR actually changed (routed barrier propagation, reservation-to-lease handoff, pinned version resolution, the controller gate) is unprotected, and several code comments and subsystem docs still describe the pre-change behavior. One candidate spec bug (index drop/recreate resolving the wrong incarnation) was investigated and rejected on code evidence — see the Spec section. Recommendation: request changes — add controller/routed-path regression coverage with projection skew, and complete the comment and documentation updates before merge.

Standards

1. [P2] The new regression test guards only the storage leaf, not the changed production path

docs/technical/contributing/testing.md:21-24: "A regression test is a guard for a specific production path, not merely an example that reaches nearby code."

TestInspectIndex_ResolvesMembershipAtMainHorizon (internal/storage/readstore/iterator_event_resolve_test.go:303-400) supplies Reader, Version, and HorizonSequence directly to readstore.InspectIndex. It stays green if any of the layers this PR changed regresses:

• the routed barrier propagation (internal/bootstrap/controller_routed.go:449-456 reverting to plain ctx),
• the OpenReservedQueryHandle → AlignedIndexSnapshot reservation/lease handoff (internal/application/ctrl/controller_default.go:1046-1104),
• the pinned version resolution and split gate (controller_default.go:1115-1131).

The existing e2e coverage cannot compensate: tests/e2e/business/inspect_index_test.go waits for the index to catch up before inspecting, so it never produces main/read-projection skew and passes regardless of the new trimming. Add a controller- or routed-level test with main/projection skew that fails when any new wiring limb is removed.

2. [P3] Stale code comments contradict the new behavior (AGENTS.md:89 — "Interface/behavior change: update relevant code comments")

• internal/application/ctrl/controller_default.go:1105-1114: the gate comment still says "the local replica's IndexVersionState.CurrentVersion decides whether queries can be served", but the code now calls readstore.PinnedVersionResolver(snap, ledgerInfo.GetName(), mainSeq) (line 1115), whose result deliberately differs from CurrentVersion — it withholds versions whose ActivationSequence exceeds the pin and reports absent/tombstoned state as not primed. The diff also half-edited the sentence ("We take the snapshot FIRST and" was deleted, leaving "We read the version state through it" without its antecedent).
• internal/storage/readstore/inspect.go:67-74: forEachLiveGroup still documents "whose latest event is an ADD — i.e. current membership", and inspect.go:130 still says countLiveGroups "counts current members" — both now resolve the latest event at or below the horizon.

3. [P3] Owning subsystem README and subsystem doc not updated (AGENTS.md:86 — subsystem doc and its README)

• docs/technical/architecture/subsystems/read-path/README.md:4-8 still names only "Filtered account/transaction queries and every log query" as consumers that "wait for the read index to align with that horizon"; InspectIndex is now one too — the diff's own read-snapshot-consistency.md update says so.
• docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152 still claims "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — all three claims changed in this PR (certificate wait + aligned snapshot, pin-resolved version with the !primed/version-0 split, horizon-trimmed membership).

4. [P3] PR description lacks the required traceability summary

docs/technical/contributing/product-technical-traceability.md:80-86 requires a minimum PR-description summary (need, current limitation, observable requirement, evidence, decision/proportionality, alternatives, validation) for significant consistency/distributed-system changes. The body states only the mechanism ("Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming."). Add the required fields, or link the committed contracts this PR updates (read-snapshot-consistency.md, indexes.md, readstore-event-keys.md) plus the regression evidence, per the doc's linking provision (line 76).

Spec

No confirmed material finding. No linked repository spec or GitHub issue exists for this PR (it references external tracker EN-1946), so the PR body was treated as the spec proxy.

For the record, one candidate spec finding was verified and rejected: the claim that a drop/recreate sequence (inspect opens its main snapshot while incarnation A exists → index dropped and recreated as B → B's backfill completes → the aligned snapshot resolves B and horizon-trims it) yields an empty or wrong result is contradicted by the code. A recreated index's backfill starts from cursor: 0 (internal/application/indexbuilder/backfill.go:128-133) and its events carry the original historical log sequences they were folded from (docs/technical/architecture/subsystems/read-path/readstore-event-keys.md:118-120; completeBackfill in backfill.go:1256+ intentionally writes no ActivationSequence for backfill-built versions). Trimming B's keyspace at the pin therefore reconstructs exactly incarnation A's membership at that horizon — the correct historical answer — rather than an empty result. The ActivationSequence guard remains meaningful for the case it was designed for (schema-rewrite promotions stamped at a single sequence).

---

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

[shipfox-ai]

Final review — PR #1891 (fix(reads): align index inspection to Raft horizon, EN-1946)

This PR routes InspectIndex through the cross-store alignment machinery: RoutedController.InspectIndex now propagates the ReadIndex barrier (internal/bootstrap/controller_routed.go:449-456), the controller reserves event history before opening the main snapshot (OpenReservedQueryHandle, internal/application/ctrl/controller_default.go:1041-1045), waits for a projection certificate and pins the main snapshot's native sequence (AlignedIndexSnapshot, controller_default.go:1103-1107), resolves the servable version at that pin (PinnedVersionResolver), and forwards HorizonSequence: mainSeq into the scan so all three inspect modes resolve membership at or below the fixed main horizon (controller_default.go:1148-1160, internal/storage/readstore/inspect.go).

I independently verified the riskiest seams and found them correct: the readiness gate (!primed → ErrIndexNotFound, version.Version == 0 → ErrIndexBuilding at controller_default.go:1119-1131) exactly matches the requireIndexReady contract and PinnedVersionResolver's pin-below-activation semantics (internal/storage/readstore/store.go:664-704, internal/query/compile.go:1578-1612); the horizon trim in forEachLiveGroup is sound, including the necessary live = false reset on group start and the big-endian sequence layout (inspect.go:113-118, key layout at internal/storage/readstore/event_keys.go:42-49); the reservation double-release is safe (Lease.Release uses sync.Once, internal/storage/readstore/read_lease.go:94-104); and the barrier propagation mirrors the established pattern in every other routed read.

Recommendation: request changes — the implementation is correct, but the PR's core guarantee (inspection reflects the fixed main-store horizon, not projection head) has no regression coverage at the composed path, contrary to this repo's own testing standard, and the changed behavior left stale code comments and subsystem documentation behind.

Standards

[P2] New regression test guards only the leaf scan, not the changed production path
internal/storage/readstore/iterator_event_resolve_test.go:303-397 (TestInspectIndex_ResolvesMembershipAtMainHorizon).

docs/technical/contributing/testing.md:23-24 states: "A regression test is a guard for a specific production path, not merely an example that reaches nearby code." The added test drives readstore.InspectIndex directly with hand-supplied Version and HorizonSequence values. It stays green if any of the seams this diff actually changed were broken or removed: barrier propagation in RoutedController.InspectIndex (controller_routed.go:449-456), the reservation-to-lease handoff (controller_default.go:1041-1045, 1103-1107), pin-aware version resolution (PinnedVersionResolver(snap, …, mainSeq)), or the HorizonSequence: mainSeq forwarding (controller_default.go:1148-1160). The existing e2e coverage (tests/e2e/business/inspect_index_test.go) waits for full convergence before inspecting, so it cannot detect main/read-projection skew. AlignedIndexSnapshot itself is well unit-tested (internal/query/aligned_snapshot_test.go), but nothing composes it with InspectIndex.

Impact: a refactor or miswire could revert the endpoint to projection-head statistics (or a version activated after the fixed main snapshot) while all added and existing tests pass — silently regressing the exact guarantee this PR ships.

Resolution: keep the leaf all-mode test, and add one composed regression (controller-level or e2e) that creates projection skew ahead of a fixed main snapshot and fails if barrier propagation, pin resolution, or horizon forwarding is bypassed.

[P3] Behavior change left stale code comments

• internal/storage/readstore/inspect.go:66-68 still says forEachLiveGroup emits groups "whose latest event is an ADD — i.e. current membership", and inspect.go:129 still says countLiveGroups "counts current members" — both now resolve membership at or below the horizon and deliberately ignore later events. The HorizonSequence doc (inspect.go:40-42) does describe the new semantics, making the older comments internally contradictory.
• internal/application/ctrl/controller_default.go:1107-1116 still attributes the gate to "the local replica's IndexVersionState.CurrentVersion", says "We read the version state through it" (dangling antecedent — the locally taken snapshot was removed in favor of the pinned snapshot from AlignedIndexSnapshot), and justifies the design with a "take the snapshot FIRST" GC argument that no longer matches the code.

AGENTS.md:89 ("Interface/behavior change: update relevant code comments") requires these be updated.

[P3] Subsystem documentation not updated for the new alignment mechanism

• docs/technical/architecture/subsystems/read-path/README.md:3-9 still lists only filtered account/transaction queries and every log query as projection-aligned consumers, omitting InspectIndex even though this diff makes it one.
• docs/technical/architecture/subsystems/read-path/typed-metadata.md:150-152 still says "InspectIndex pins a read-store snapshot, rejects CurrentVersion == 0" — this now contradicts the implementation, which waits for a Raft certificate, resolves the version at the main snapshot's pin (version == 0 at the pin surfaces ErrIndexBuilding; a removed index surfaces ErrIndexNotFound), and trims membership to HorizonSequence. (read-snapshot-consistency.md and indexer/indexes.md were updated correctly.)

AGENTS.md:86 requires a new mechanism to update the matching subsystem documentation and its README.

Spec

No separate spec document was available; the commit message states the intent ("Aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming"), which I used as the spec baseline.

[P2] The stated end-to-end alignment guarantee has no composed-path regression coverage
Same evidence as the Standards finding above: the only new test (iterator_event_resolve_test.go:303-397) bypasses every changed API-level seam, and the existing e2e suite (tests/e2e/business/inspect_index_test.go) only inspects converged data. Removing or miswiring the barrier propagation, the reservation handoff, or the pin/horizon forwarding would leave the endpoint returning projection-head membership — a violation of the PR's stated guarantee — with no failing test.

Resolution: as above — one composed regression that drives the real InspectIndex path with the read projection ahead of the fixed main snapshot and asserts post-horizon membership events are excluded and the pinned version is served.

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

[shipfox-ai]

Review summary

This PR aligns InspectIndex with the read path's existing alignment machinery: the routed controller carries the local ReadIndex barrier horizon into the call (internal/bootstrap/controller_routed.go:450-452), the default controller reserves event history up front, waits for a certified read-index snapshot at the fixed main horizon via AlignedIndexSnapshot, resolves the servable version through PinnedVersionResolver at that horizon's native sequence, and forwards the same sequence into the scan so metadata/existence membership events beyond the horizon are ignored. I verified the horizon decoding against the documented key layout (internal/storage/readstore/event_keys.go — big-endian sequence at tpos+1..tpos+9, op at tpos+9), the live = false reset on group start (required exactly because post-horizon events now continue past the live assignment, and exercised by the new test's (silver, a:1) / (null, a:3) groups), the idempotent double-release of the reservation hold (sync.Once in read_lease.go:94, matching the existing pattern at controller_default.go:999), and the !primed → ErrIndexNotFound mapping against PinnedVersionResolver's contract. No correctness defect was found in the changed logic. What is missing is evidence at the level the repository demands: the changed production wiring has no regression coverage, and the documentation updates describe the mechanism without completing the required traceability chain and without updating every affected surface.

Recommendation: request changes — the implementation looks correct, but the production-path regression coverage gap and the incomplete documentation/traceability updates should land in this PR.

Standards

1. [Major] Regression test guards the leaf, not the changed production path

internal/storage/readstore/iterator_event_resolve_test.go:303 (TestInspectIndex_ResolvesMembershipAtMainHorizon); untested wiring at internal/application/ctrl/controller_default.go:1124-1148 (reservation, AlignedIndexSnapshot, PinnedVersionResolver), :1181 (HorizonSequence: mainSeq forwarding), and internal/bootstrap/controller_routed.go:450-452 (barrier propagation).

Per docs/technical/contributing/testing.md ("A regression test is a guard for a specific production path, not merely an example that reaches nearby code") and the regression rules in AGENTS.md. The added test calls readstore.InspectIndex with an already-selected Version and HorizonSequence; it proves event trimming inside the scanner, but if the changed wiring regressed — HorizonSequence no longer forwarded, resolution reverting to the live CurrentVersion/NewSnapshot() flow, or the barrier stop being carried through the routed controller — the test stays green. Grep confirms no controller-level or routed-level test exercises InspectIndex at all (only generated mocks and HTTP-adapter stubs), while controller_default_*_test.go files show controller coverage is feasible in this repo.

Impact: the endpoint could regress to reporting projection-head membership or a version activated after the served snapshot while focused validation passes.

Resolution: add controller/routed coverage that drives InspectIndex with divergent main/read-projection horizons and a post-pin version activation, asserting barrier alignment, pin-aware version selection, and post-horizon trimming end to end.

2. [Moderate] Traceability chain incomplete in the changed authoritative docs

docs/technical/architecture/subsystems/indexer/indexes.md:129-140 and docs/technical/architecture/subsystems/read-path/read-snapshot-consistency.md:151-161.

docs/technical/contributing/product-technical-traceability.md requires consistency/distributed-system decisions to record the product/operational need, the concrete prior limitation, the observable requirement, why the decision is proportionate (with material alternatives), and validation, as durable in-repo evidence. Both updated documents describe the chosen mechanism (Raft certificate, pinned version, lease handoff, horizon trimming) but neither records the concrete failure mode being fixed (inspect statistics describing the projection head rather than the served main snapshot), why the added blocking wait and lease are proportionate for an introspection endpoint, or how the requirement is validated. The PR body is an implementation summary and is not durable repository evidence.

Impact: reviewers cannot establish the intended observable contract for the new blocking/wait semantics or judge whether the added latency on InspectIndex is justified against a documented requirement.

Resolution: add the missing need → limitation → requirement → decision → validation chain to the owning subsystem documents and summarize/link it in the PR description, including validation that observes the requirement.

3. [Minor] Code comments not updated for the new pin/horizon semantics

• internal/application/ctrl/controller_default.go:1131-1139: the readiness comment still opens with "the local replica's IndexVersionState.CurrentVersion decides whether queries can be served" and reasons about the removed NewSnapshot()-first flow. The code now resolves through PinnedVersionResolver at mainSeq, whose result deliberately differs from the live CurrentVersion (activation sequence above the pin resolves version 0 → ErrIndexBuilding), and adds the !primed → ErrIndexNotFound arm the comment does not mention.
• internal/storage/readstore/inspect.go:63-69 and :129: forEachLiveGroup still documents groups "whose latest event is an ADD — i.e. current membership" and countLiveGroups "counts current members"; both now resolve membership at-or-below the horizon parameter, skipping later events.

AGENTS.md requires "Interface/behavior change: update relevant code comments."

4. [Minor] Subsystem documentation not fully aligned with the change

• docs/technical/architecture/subsystems/read-path/README.md:3-8: the overview still lists only filtered account/transaction queries and log queries as readers that wait for the read index to align with the barrier horizon; the diff itself declares InspectIndex a projection consumer too (read-snapshot-consistency.md:154). AGENTS.md requires updating "the matching docs/technical/architecture/ subsystem documentation and its README."
• docs/technical/architecture/subsystems/indexer/indexes.md:185-187 (new text): states that a replica "whose current version activates after the main snapshot returns 'not built locally'", but the code returns ErrIndexBuilding ("index is still building") in that case; only the not-primed/tombstoned arms return ErrIndexNotFound. The sentence should distinguish the two outcomes.

Spec

No spec is available for this PR. I verified the diff against the PR's own stated scope ("aligns InspectIndex with a fixed Raft/main-store horizon, pinned index version, and native-sequence trimming") and found no scope creep and no unrelated externally observable behavior: the four code changes (routed barrier propagation, controller reservation/alignment/pin wiring, horizon-aware scanning, and the leaf regression test) and the documentation changes all serve that single goal. No confirmed material findings on this axis.

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

[shipfox-ai]

Review summary — PR #1891 (fix(reads): align index inspection to raft horizon)

This PR makes InspectIndex a projection consumer: routing attaches a ReadIndex barrier horizon, the controller reserves event history via OpenReservedQueryHandle, waits via AlignedIndexSnapshot at the main snapshot's native sequence, resolves the servable version with PinnedVersionResolver at that pin, and readstore.InspectIndex now trims membership (distinct values, facets, summaries, existence counts) to HorizonSequence. I verified the implementation against the code and found it correct: the big-endian sequence decode at key[tpos+1:tpos+9] matches KeyBuilder.PutUint64, the added live = false group reset is a necessary and correct fix (without it, a group whose only events exceed the horizon would inherit live from the previous group), no other callers of the changed helpers exist, and the removed-vs-building error classification matches the documented removal contract in requireIndexReady. What keeps this from merging as-is is one coverage gap on the production path plus stale comments/docs left behind by the behavior change.

Recommendation: request changes (one blocking coverage gap per the repo's own severity rules; the code itself is verified correct).

Standards

1. [P2][blocking] Added regression test guards the leaf, not the changed production path

Location: internal/storage/readstore/iterator_event_resolve_test.go:303 (TestInspectIndex_ResolvesMembershipAtMainHorizon); production wiring at internal/bootstrap/controller_routed.go:449-456 and internal/application/ctrl/controller_default.go:1057-1207.

Standard: docs/technical/contributing/testing.md:23 — "A regression test is a guard for a specific production path, not merely an example that reaches nearby code"; docs/technical/contributing/ai-review.md:31 — a material regression left untested is blocking.

Evidence: the only added test builds InspectParams by hand (HorizonSequence: 20) and calls readstore.InspectIndex directly. It proves the trimming arithmetic, but stays green if RoutedController.InspectIndex stops attaching the barrier horizon via withLocalBarrierHorizon, if DefaultController.InspectIndex stops composing OpenReservedQueryHandle/AlignedIndexSnapshot/PinnedVersionResolver, or if HorizonSequence: mainSeq stops being forwarded into InspectParams (controller_default.go:1196). No unit test drives InspectIndex through DefaultController or RoutedController, and the existing e2e spec (tests/e2e/business/inspect_index_test.go) was not extended to assert horizon behavior. Focused helper tests exist, but nothing proves this new caller composes them.

Impact: default inspection could again serve an index projection that is stale or ahead of its fixed main-store state, or lose pin-aware version gating and event-history protection, while all unit tests and existing e2e specs pass.

Resolution: add a controller/routing-level regression test that drives InspectIndex end-to-end and asserts the barrier context is propagated, alignment is awaited, a version activated after the main snapshot is rejected as "building", and post-horizon membership is excluded.

2. [P3] Stale readiness-gate comment in InspectIndex

Location: internal/application/ctrl/controller_default.go:1131-1134.

Standard: AGENTS.md:89 — "Interface/behavior change: update relevant code comments."

Evidence: the half-edited comment still frames the gate as reading "the local replica's IndexVersionState.CurrentVersion" and has a dangling antecedent ("We read the version state through it" — the direct readstore.NewSnapshot it referred to was removed). The code now resolves a pin-aware version at mainSeq via PinnedVersionResolver (line 1139) with a primed (removed vs. building) distinction the comment never mentions.

Impact: the comment now contradicts the gate's actual contract (pin-aware resolution, removal-vs-building split), which is exactly the subtlety a future reader must not get wrong here.

3. [P3] Stale horizon comments in readstore inspection helpers

Location: internal/storage/readstore/inspect.go:65-67 and inspect.go:129.

Standard: AGENTS.md:89 — "Interface/behavior change: update relevant code comments."

Evidence: forEachLiveGroup still documents groups "whose latest event is an ADD — i.e. current membership" and countLiveGroups "counts current members", but both now resolve the latest event at or below horizon (the new horizon > 0 → continue skip at inspect.go:111-112). InspectParams.HorizonSequence got horizon-aware comments; these two did not.

Impact: "current membership" is now wrong for any pinned read; a future caller could reasonably assume head-of-store semantics from the doc.

4. [P3] Subsystem documentation not updated for the new mechanism

Location: docs/technical/architecture/subsystems/read-path/README.md:5-8 and docs/technical/architecture/subsystems/read-path/typed-metadata.md:148-151.

Standard: AGENTS.md §Documentation maintenance (via docs/technical/architecture/README.md:49): new technical mechanisms require updating the matching subsystem documentation and its README.

Evidence: indexes.md and read-snapshot-consistency.md were updated, but the read-path README still lists only "filtered account/transaction queries and every log query" as alignment waiters (InspectIndex now waits too), and typed-metadata.md still says InspectIndex "pins a read-store snapshot, rejects CurrentVersion == 0, and scans only the locally served version" — all three claims changed by this diff (pin-aware resolution at the main horizon, primed/removed rejection, event-history trimming).

Impact: authoritative architecture guidance is now materially false on the inspected endpoint's consistency guarantees.

Spec

No confirmed material finding. No usable spec exists for this change: the commit message carries no issue reference, the PR body references only external tracker key EN-1946 (not a GitHub issue), and no spec file exists under docs/, specs/, or .scratch/. Against the requirement available in the commit/docs — align InspectIndex with a fixed Raft/main-store horizon, pin the index version at that horizon, and trim native-sequence membership — the implementation path (routed barrier handoff → reserved handle → aligned snapshot → PinnedVersionResolver at mainSeq → HorizonSequence filtering across all three modes) is complete and correct. No missing requirement, incorrect implementation path, or unrelated behavior change was found.

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