# Scrum review — materializer_replay_substrate_fix — opus (opencode/claude-opus-4-7)

**Latency:** 34224ms · **Tokens:** 6 in / 1498 out · **Date:** 2026-05-02

---

SEVERITY: BLOCK
WHERE: cmd/vectord/main_test.go:TestSaveTask_Coalesces
WHAT: Test races on `s.mu` directly without going through `trigger`, but the `release` close happens before the catch-up `save()` runs and increments `saveCnt`; more importantly, the assertion `got != 2` is racy with the goroutine drain logic and the unused `sync` import in this file (or shadowing) is benign — actual concern: the loop in `trigger` runs the next `save` AFTER setting `pending=false` while still holding `inflight=true`, so the idle check works, but there is a window where 50 piled-up triggers could each see `inflight==true` (correct), yet the test asserts exactly 2 saves which is the right invariant.
WHY: On re-read this is actually correct — withdraw to INFO. (Self-correction.)

SEVERITY: WARN
WHERE: internal/vectord/index.go:Add
WHAT: After `safeGraphAdd` succeeds, the new node is in `i.g` but not yet in `i.vectors`; if a concurrent reader could observe `i.g` it would see an inconsistency — mitigated only by the write lock held across both, which is fine, but a panic between `safeGraphAdd` and the `i.vectors[id] = out` assignment would leave the graph holding an entry that the side-store doesn't know about and that the next rebuild would silently drop.
WHY: The diff's whole premise is that `i.vectors` is the source of truth; a non-panicking-but-aborted Add (e.g., future code added between those lines) would violate the invariant — worth a comment or deferred recovery.

SEVERITY: WARN
WHERE: internal/replay/replay.go:logReplayEvidence
WHAT: Truncating `ModelResponse` mutates `persist.ReplayResult.ModelResponse` but `persist` embeds `ReplayResult` by value, so the in-memory `result` returned to the caller is unaffected — confirm; however the comment says "persisted log" while the code path is correct, just verify the embedded-struct copy semantics are intentional.
WHY: `ReplayResult` is embedded by value in the anonymous struct, so mutation is local — behavior matches comment, but a future refactor to `*ReplayResult` would silently change caller-visible state.

SEVERITY: WARN
WHERE: internal/materializer/materializer.go:isoDatePartition
WHAT: On parse failure returns `"0000/00/00"` partition, but `MaterializeAll` already validates `RecordedAt` upfront via `validISOTimestamp`, so this fallback is unreachable — keep, but the comment claims it's reachable.
WHY: Dead defensive branch; the comment misleads future readers about possible output paths.

SEVERITY: WARN
WHERE: internal/vectord/index_test.go:TestAdd_SmallIndex_ConcurrentDistinctIDs
WHAT: This test function's doc comment is detached from its declaration — the comment block on lines preceding `TestAdd_SmallIndex_ConcurrentDistinctIDs` floats above a closing brace of the prior test, making it look like the comment belongs to nothing.
WHY: The diff shows `_ = safeGraphDelete(idx.g, "safe-test")` then `}` then a `// playbook_record pattern:` comment with no blank line and no leading `// TestAdd_...` — readability/godoc regression.

SEVERITY: INFO
WHERE: internal/replay/types.go:ReplayRequest
WHAT: `LocalOnly` is wired through CLI but never read in `Replay` except indirectly (the escalation gate uses `!opts.LocalOnly`); behavior matches docs but worth a unit test.
WHY: No test exercises `LocalOnly=true` path; coverage gap.

SEVERITY: INFO
WHERE: internal/materializer/materializer.go:Receipt
WHAT: `Errors` field is always empty in this code path (no producer ever appends to `errs`); only `warnings` are populated.
WHY: Dead variable / future placeholder — fine, but `emptyToNil` returns `[]string{}` not nil, so the JSON always emits `"errors": []`, which is the intended schema-compatible shape.

VERDICT: ship-with-fixes — fix index.go ordering comment + detached test docstring; rest is informational.
