checksum: run the verifier under an explicit divergence policy and mint the proofs (CO-2, CO-3)

[Kiran01bm]

Adds Verifier.Check: the chunk verifier run under an explicit divergence policy, with the repair primitive and the private constructors of VerifiedShadow and CleanWatermark, so a pass now says what a difference means and what it proves.

Why

#134 landed the comparison and a Report that proves nothing. The gate needs three things on top of it: a policy the caller states for every pass, never a default (CO-3 — in the steady state a divergence is a defect and aborts; only reconciliation after a lost slot repairs); a repair that recopies the differing chunks with exactly the statement the copy used, so the two cannot drift; and proofs that only a pass which found nothing and repaired nothing can mint (CO-2 — a repaired chunk was recopied, not verified; the proof comes from the next fresh pass).

What

• pkg/checksum:
  • DivergencePolicy (DivergenceAbort = abort, DivergenceRepair = repair); the zero value and any other string are refused with ErrNoDivergencePolicy before anything is read. ParseDivergencePolicy(string) gives an importer the same refusal at config load.
  • Check(ctx, pool, through, policy) (Outcome, error) runs Verify and acts on the report. Clean → Outcome carrying a CleanWatermark at through, plus a VerifiedShadow when through.Complete(); a partial clean pass mints only the watermark. Abort + differences → *DivergenceError{Report}, shadow untouched. Repair → every differing chunk is replaced in one guarded read-write transaction — all DELETE FROM shadow WHERE pk BETWEEN $1 AND $2, then all chunk inserts, one commit — and each chunk is then digested again in a fresh snapshot; a chunk that still differs is *RepairError{Repair, After}. One transaction because the shadow carries the source's unique indexes: a unique value that moved between two differing chunks would make a chunk-by-chunk recopy fail with unique_violation on every pass, and a half-repaired shadow is never visible. Every refusal Check returns is wrapped verify <schema>.<table>: ….
  • Outcome{Report, Repairs} comes back with the error too, so the repairs that committed before a RepairError or a lost lock are reported. Outcome.Clean() means "this pass minted the clean watermark"; CleanWatermark() (CleanWatermark, bool) and VerifiedShadow() (VerifiedShadow, bool) gate on their flags. The proof constructors are private; the zero values stay forgeable and consumers check the flag.
  • A repair pass is a reconciliation and says so on Check and in doc.go: it assumes no other shadow writer and that source rows in differing chunks hold still until the rereads — the pass after slot loss runs before change capture resumes, and the resumed capture carries the application's writes. A write that lands inside the pass makes the repaired chunk read different again and is refused as RepairError rather than repaired twice. The live transient-difference protocol (applier-LSN feed, retries) is its own plan item.
  • Guard refactor: begin takes pgx.TxOptions, so the repair transaction runs the same guard as the digest transactions (owner role, catalog-only search_path, ACCESS SHARE on both relations, lock confirmation, relation-OID check); a lost lock during a repair cancels it and is reported as LK-1.
• pkg/internal/chunksql: Insert(schema, source, shadow, key, columns) is the one chunk-insert statement; copier.copySQL and the verifier's repair both build from it, so the statement cannot drift and nothing outside the module can call it. SAFETY.md and .golangci.yml (depguard) carry the package.
• pkg/copier: the copy transaction now pins search_path to pg_catalog, so the repair runs the shared statement under the same session as the copy and "the same guard as the copier" is exact in the docs. Watermark.Complete() names the top-of-key-space test the chunker already made.
• Docs in the same PR: invariants.md CO-1 / CO-2 / CO-3 "enforced today", copy-and-swap-design.md package map and D14 "where enforced", low-level-design.md reconciliation bullet (the pass runs before the new slot's stream is applied), SAFETY.md and architecture.md pkg/checksum rows.
• Tests on real PostgreSQL: a clean complete pass mints both proofs naming the compared relations; a clean partial pass mints only the watermark; abort on a missing, a changed, and an extra row returns all three chunks, leaves every difference in place, and the outcome is not clean; repair removes 999 / 1000 / 501 rows and inserts 1000 / 1000 / 500, the shadow then equals the source, no proof is minted, and the next pass mints both; a BEFORE INSERT trigger that zeroes qty makes the repair not take and is refused as RepairError, with the committed recopy left as the trigger wrote it; the same trigger on keys above 1000 only, with two differing chunks, reports both committed repairs and names the second; a unique value swapped between ids 100 and 1500 on the source converges; a shadow renamed away and replaced by an impostor as the repair transaction begins is refused as ST-6 with the impostor empty; a source write between the recopy and the reread is refused as RepairError; the zero and an unknown policy are refused with no pass run; losing the lock as the repair's DELETE starts (injected through a pool tracer, deterministic) is reported as LK-1 and the repair rolls back; the copier ignores a session search_path that shadows the bigint <= operator. Unit: policy validation and parsing, the zero Outcome is not clean and mints nothing, prove mints the shadow proof only at the complete watermark, the frozen repair and chunk-insert statements, Watermark.Complete at both sides of the boundary. Each new test fails under the mutation it exists to catch (chunk-by-chunk recopy, unguarded repair transaction, repairs dropped on error, Clean() reading the report, copier without the pin).

Before / after

Before (#134)                                 After
Verify ──▶ Report{Mismatches}                 Check(policy) ──▶ Verify ──▶ Report
             (a difference means nothing;        clean ───────────────▶ Outcome{CleanWatermark,
              nothing is minted)                                                  VerifiedShadow if complete}
                                                 differs, abort ──────▶ Outcome{Report} + DivergenceError   shadow untouched
                                                 differs, repair ─────▶ one guarded tx over every chunk:
                                                                          DELETE shadow [lo, hi]  (all chunks)
                                                                          chunksql.Insert        (all chunks)
                                                                        commit; reread each chunk fresh
                                                                          all equal ──▶ Outcome{Repairs}, no proof
                                                                          one differs ▶ Outcome{Repairs} + RepairError

Deferred (follow-up)

A checksum progress.WorkSource filler needs an OperationChecksum and counters for chunks compared, rows hashed, and chunks repaired in the progress contract (rows_copied is the wrong name); it lands with that contract change, before an orchestrator wires Check.

References

• docs/invariants.md CO-1, CO-2, CO-3; docs/copy-and-swap-design.md D14.
• Builds on checksum: chunk verifier comparing source and shadow in one snapshot (CO-1) #134 (merged).

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 1/2: adversarial correctness review of 4d39e3e. I read Check, the repair path and the guard refactor against the copier's statement and guard, and against CO-1, CO-2, CO-3, CO-4, CO-9, LK-1 and ST-6. Probes and mutations ran on real PostgreSQL (testcontainers), using the PR's own fixture.

0 blocking, 5 non-blocking.

I looked for any way the policy turns a real difference into a pass or a quiet repair, and found none. The zero and unknown policies are refused before anything is read. abort never writes. A pass that repaired anything mints neither proof. VerifiedShadow needs the complete watermark. Nothing retries a repair that did not take. No mismatch is clamped or dropped: the error reports len(Mismatches) of Chunks, and the chunks reach through exactly. Repairs touch only keys at or below the landed watermark, so they never overlap a chunk the copier still has in flight. Each repair recopies with the copier's own INSERT … ON CONFLICT (pk) DO NOTHING after a delete over the same closed bigint range. It runs under the verifier's guard: owner role, pg_catalog-only search_path, ACCESS SHARE on both relations, Confirm, and the OID check. Of 22 mutations, 19 are caught, one is equivalent, and two survive. The survivors are in the table below, and one of them is finding 3.

Non-blocking

1. Check returns an Outcome that answers Clean() == true with every refusal, DivergenceError included. check.go:32, check.go:72

Every error path returns Outcome{}, and Outcome.Clean() is Report.Clean(), which is len(Mismatches) == 0. So an aborted pass, a repair that did not take, a lost lock, and a refused policy all come back with an outcome that says it is clean. policy_test.go:34 pins this ("an empty report has no mismatches"). The proofs are safe, because both accessors gate on their flag. But Clean() is the accessor whose name answers the gate's question, so a caller that logs or branches on it before checking err reads a divergence as clean. The package already holds the opposite rule: ErrNothingLanded exists because "a pass that compared nothing must not read as clean". The fix is to make Clean() mean "this pass minted the clean watermark": return o.clean.Watermark().Valid(). Every existing Check test still passes with that change, and policy_test.go:34 flips to False.

Test that fails on 4d39e3e and passes with the fix

// A pass that aborted on a difference does not hand back an Outcome that
// answers Clean() == true.
func TestCheckAbortOutcomeIsNotClean(t *testing.T) {
	f := newVerifierFixture(t)
	target, lock, shadow := f.prepare(t)
	f.exec(t, "UPDATE "+f.shadowName(shadow)+" SET qty = 0 WHERE id = 1500")

	outcome, err := f.check(t, f.pool, target, shadow, lock, copier.NewWatermark(math.MaxInt64), checksum.DivergenceAbort)
	var divergence *checksum.DivergenceError
	require.ErrorAs(t, err, &divergence)
	assert.False(t, outcome.Clean(), "a pass that found a difference is not clean")
}

--- FAIL: TestCheckAbortOutcomeIsNotClean (1.15s) (Should be false) on 4d39e3e, three of three runs. It passes with the one-line fix, and the only other test that changes is TestOutcomeZeroValueMintsNothing.

2. When a later chunk's repair fails, the result drops the repairs that already committed. repair.go:42, repair.go:45, check.go:76

Each chunk commits its own repair. repairAll then returns nil on the first error, and Check returns Outcome{}. Take a pass that differs in chunks A and B. A's repair commits and rereads equal. B's repair does not take. The caller gets a RepairError that names only B, and has no record that A's rows were deleted and recopied. Nothing is proven either way, so this is not a false pass. But it is a write to the shadow that the result does not report, and an operator deciding whether to restart from scratch or reconcile again needs to know what was rewritten. The same loss happens on LK-1 partway through. Fix: return repairs alongside the error from repairAll, and Outcome{Report: report, Repairs: repairs} from Check. Both are three-line changes, and every existing test passes with them.

Test that fails on 4d39e3e and passes with the fix

// When a later chunk's repair does not take, the chunks already recopied
// and committed are still reported.
func TestCheckReportsCommittedRepairsWhenALaterRepairFails(t *testing.T) {
	f := newVerifierFixture(t)
	target, lock, shadow := f.prepare(t)
	f.exec(t, "DELETE FROM "+f.shadowName(shadow)+" WHERE id = 5")
	f.exec(t, "UPDATE "+f.shadowName(shadow)+" SET qty = 0 WHERE id = 1500")
	f.exec(t, `
		CREATE FUNCTION %s.zero_qty() RETURNS trigger LANGUAGE plpgsql AS $$
		BEGIN
			NEW.qty := 0;
			RETURN NEW;
		END
		$$`)
	f.exec(t, "CREATE TRIGGER zero_qty BEFORE INSERT ON "+f.shadowName(shadow)+" FOR EACH ROW WHEN (NEW.id > 1000) EXECUTE FUNCTION %s.zero_qty()")

	outcome, err := f.check(t, f.pool, target, shadow, lock, copier.NewWatermark(math.MaxInt64), checksum.DivergenceRepair)
	var failed *checksum.RepairError
	require.ErrorAs(t, err, &failed)
	assert.Equal(t, chunk(t, 1001, 2000), failed.Repair.Mismatch.Chunk)

	_, exists := f.shadowRow(t, shadow, 5)
	require.True(t, exists, "the first chunk's repair committed: row 5 is back")
	require.Len(t, outcome.Repairs, 1, "the committed repair of [MinInt64, 1000] is reported")
	assert.Equal(t, chunk(t, math.MinInt64, 1000), outcome.Repairs[0].Mismatch.Chunk)
}

--- FAIL: TestCheckReportsCommittedRepairsWhenALaterRepairFails (1.17s) ("[]" should have 1 item(s), but has 0) on 4d39e3e, three of three runs. Row 5 is back, so the first repair did commit. It passes with the fix.

3. No test pins that the repair transaction runs the guard. repair.go:64

Replacing v.begin(ctx, pool, pgx.TxOptions{AccessMode: pgx.ReadWrite}) with a bare pool.BeginTx survives the whole package. The lock-loss test still passes, because Bind cancels the context whether or not the transaction confirmed the lock. The guard is the only thing between the repair's DELETE and a shadow name that no longer belongs to the proven relation (ST-6). This is also the only write path in the package, so it is the one place the owner role and the OID check guard a write, not a read. The test below swaps the shadow for an impostor at the moment the repair transaction begins. It kills the mutation, and it passes today.

Test that passes on 4d39e3e and fails with the repair's guard removed

// A shadow replaced between the comparison pass and the repair is refused
// by the repair transaction's own guard before it writes: the impostor
// that now carries the shadow's name is left empty (ST-6, LK-1).
func TestCheckRepairRefusesAReplacedShadowBeforeWriting(t *testing.T) {
	f := newVerifierFixture(t)
	target, lock, shadow := f.prepare(t)
	f.exec(t, "UPDATE "+f.shadowName(shadow)+" SET qty = 0 WHERE id = 1500")
	name := f.shadowName(shadow)
	// The repair is the pass's only read-write transaction.
	pool := f.poolAfter(t, &afterHook{prefix: "begin read write", hook: func() {
		f.exec(t, "ALTER TABLE "+name+" RENAME TO displaced")
		f.exec(t, "CREATE TABLE "+name+" (LIKE %s.displaced INCLUDING ALL)")
	}})

	_, err := f.check(t, pool, target, shadow, lock, copier.NewWatermark(math.MaxInt64), checksum.DivergenceRepair)
	require.ErrorIs(t, err, checksum.ErrInvariantViolation)
	assert.Contains(t, err.Error(), "(ST-6)")
	var n int64
	require.NoError(t, f.pool.QueryRow(t.Context(), "SELECT count(*) FROM "+name).Scan(&n))
	assert.Zero(t, n, "the repair wrote nothing into the impostor")
}

// afterHook runs hook once, before the first statement starting with prefix
// that begins after `after` statements containing marker were sent.
type afterHook struct {
	marker, prefix string
	after          int
	hook           func()
	mu             sync.Mutex
	seen           int
	once           sync.Once
}

func (h *afterHook) TraceQueryStart(ctx context.Context, _ *pgx.Conn, data pgx.TraceQueryStartData) context.Context {
	h.mu.Lock()
	if strings.Contains(data.SQL, h.marker) {
		h.seen++
	}
	ready := h.seen >= h.after
	h.mu.Unlock()
	if ready && strings.HasPrefix(data.SQL, h.prefix) {
		h.once.Do(h.hook)
	}
	return ctx
}

func (h *afterHook) TraceQueryEnd(context.Context, *pgx.Conn, pgx.TraceQueryEndData) {}

func (f verifierFixture) poolAfter(t *testing.T, h *afterHook) *pgxpool.Pool {
	t.Helper()
	pc, err := pgxpool.ParseConfig(f.cfg.URL)
	require.NoError(t, err)
	pc.ConnConfig.Tracer = h
	pool, err := pgxpool.NewWithConfig(t.Context(), pc)
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	return pool
}

ok on 4d39e3e, three of three runs. With the guard removed, --- FAIL: TestCheckRepairRefusesAReplacedShadowBeforeWriting (1.91s) (Should be zero, but was 1000). The repair filled the impostor, and only the reread's guard noticed.

4. On a table taking writes, the reread treats a repair that worked as one that did not, so the reconciliation pass that repair exists for fails. repair.go:90-101, policy.go:54-58

This fails closed, so it is not a false pass. The concern is that CO-3's self-heal cannot be reached. The design runs the repair pass after slot loss, before CDC resumes from the new slot (low-level-design.md:719-721). The source keeps taking application writes during that pass, and the shadow does not follow them. So a single write to a repaired chunk, landing between its recopy and its reread, makes the chunk differ. That is a RepairError for the whole pass. The RepairError doc names this case ("or the source changed between the recopy and the read"), but it treats it the same as a trigger rewriting the shadow. On a write-heavy table a repair pass will hit that window, so reconciliation turns into a restart from scratch.

CO-2 already withholds the proof until the next fresh pass. So the reread does not need to be the final word on a transient difference. It does need to be the final word on a persistent one. One shape that keeps both: report a reread mismatch as repaired but unverified, and refuse with RepairError only when the same chunk still differs after N consecutive repair passes. The trigger test still ends in RepairError, and a single write no longer does. The other option is to keep the current behavior and state on Check that repair needs writes to the chunk quiesced, which the design does not currently provide.

Test that shows it on 4d39e3e (passes: one source write after the recopy becomes a RepairError)

// One application write to a repaired chunk between its recopy and its
// reread turns a repair that took into a RepairError for the whole pass.
func TestCheckRepairOneSourceWriteAfterTheRecopy(t *testing.T) {
	f := newVerifierFixture(t)
	target, lock, shadow := f.prepare(t)
	f.exec(t, "UPDATE "+f.shadowName(shadow)+" SET qty = 0 WHERE id = 1500")
	// After the repair's INSERT, the next transaction to begin is the reread.
	pool := f.poolAfter(t, &afterHook{marker: "INSERT INTO", prefix: "SET LOCAL lock_timeout", after: 1, hook: func() {
		f.exec(t, "UPDATE %s.orders SET qty = qty + 1 WHERE id = 1999")
	}})

	_, err := f.check(t, pool, target, shadow, lock, copier.NewWatermark(math.MaxInt64), checksum.DivergenceRepair)
	var failed *checksum.RepairError
	require.ErrorAs(t, err, &failed)
	qty, _ := f.shadowRow(t, shadow, 1500)
	assert.Equal(t, int64(1500), qty, "the recopy did take")
}

--- PASS on 4d39e3e, three of three runs. The error is chunk [1001, 2000] still differs after its repair: source 1000 rows, shadow 1000 rows, and row 1500 was repaired. Uses afterHook and poolAfter from finding 3.

5. copier.InsertChunk adds an exported writer into the shadow that runs no guard. copy_chunk.go:55

LK-1's enforcement says "Confirm is the in-transaction check every writer runs from its own connection before its first write" (copy-and-swap-design.md:383). Until this PR, the only public way to write the shadow was Copier, which runs Confirm, the owner role and the OID check. InsertChunk takes any pgx.Tx and any Shadow implementation and writes. The PR's own test calls it on a plain pool transaction, with a fake shadow whose OIDs are 1 and 2 (insert_chunk_integration_test.go:33-41). This is latent. It needs an importer to call it outside a guard, and the one caller here does run the guard. Since the only caller is pkg/checksum, moving the statement into an internal/ package shared by the two keeps "one statement, cannot drift" without adding it to the public surface.

Verified

• CO-1 upholds and extends: VerifiedShadow is minted only by prove on a clean report at Complete() (M2, M4, M16). The Enforced today text matches the code.
• CO-2 upholds: a pass with repairs mints neither proof (M3, M15). The reread is load-bearing (M6, M12).
• CO-3 upholds and extends: the zero and unknown policies are refused before the pass (M1, M21), and abort never writes (M5, M19).
• LK-1 upholds: the repair runs under Bind, and a loss mid-repair is LK-1 rather than the cancelled statement (M8, M18). The repair's own Confirm is unpinned (finding 3).
• ST-6 upholds: the guard's OID check refuses an impostor before the repair writes (finding 3's test).
• CO-9 upholds: the repair's DELETE and the recopy run under LocalSearchPath("pg_catalog"), so BETWEEN and ::bigint resolve to the catalog.
• CO-4: the DELETE then recopy overwrites landed rows, which the copier never does. That holds only while nothing else writes the shadow during the pass, and the design runs reconciliation before CDC resumes. A concurrent applier would make the reread fail closed (see 2/2).
• Watermark.Complete() is equivalent to the chunker's old Value() == MaxInt64 test, since a valid watermark only comes from NewWatermark.

Mutant	Caught by
M1 validate accepts ""	TestCheckRefusesAPassWithoutAPolicy, TestDivergencePolicyValidate
M2 prove even when the report differs	TestCheckAbortsOnDivergence…, TestCheckRepairsEveryDifferingChunk…, TestCheckRefusesARepairThatDidNotTake, TestCheckAbortsWhenTheLockIsLostMidRepair
M3 mint proofs after repairs	TestCheckRepairsEveryDifferingChunkAndMintsNothing
M4 VerifiedShadow on a partial pass	TestCheckMintsOnlyTheCleanWatermarkForAPartialPass, TestProveMints…OnlyWhenComplete
M5 abort repairs	TestCheckAbortsOnDivergenceAndLeavesTheShadowAlone
M6 skip the reread	TestCheckRefusesARepairThatDidNotTake
M7 drop repairAll's final lockLost	survives: low value, since no proof is minted either way
M8 repairAll returns the raw error, not lostOr	TestCheckAbortsWhenTheLockIsLostMidRepair
M9 skip the repair's Commit	TestCheckRefusesARepairThatDidNotTake, TestCheckRepairsEveryDifferingChunk…
M10 repair transaction without the guard	survives: killed by finding 3's test
M11 repair only the first mismatch	TestCheckRepairsEveryDifferingChunkAndMintsNothing
M12 reread compares row counts only	TestCheckRefusesARepairThatDidNotTake
M13 repair DELETE one key short	TestRepairSQLIsFrozen, TestCheckRepairsEveryDifferingChunk…, TestCheckRefusesARepairThatDidNotTake
M14 repair skips the DELETE	TestCheckRepairsEveryDifferingChunk…, TestCheckRefusesARepairThatDidNotTake, TestCheckAbortsWhenTheLockIsLostMidRepair
M15 CleanWatermark flag always true	7 tests, incl. TestOutcomeZeroValueMintsNothing
M16 VerifiedShadow flag always true	9 tests, incl. TestCheckMintsOnlyTheCleanWatermarkForAPartialPass
M17 Complete ignores validity	equivalent (no invalid watermark carries MaxInt64)
M18 Verify returns the raw error, not lostOr	TestVerifierAbortsWhenTheLockIsLostMidPass
M19 DivergenceError drops its report	TestCheckAbortsOnDivergenceAndLeavesTheShadowAlone
M20 insertChunk reports zero rows	TestInsertChunkRunsTheCopyStatement…, TestCheckRepairsEveryDifferingChunk…, 10 copier tests
M21 drop the policy check in Check	TestCheckRefusesAPassWithoutAPolicy
M22 repair in a read-only snapshot	TestCheckRepairsEveryDifferingChunk…, TestCheckRefusesARepairThatDidNotTake

go build ./..., go vet, and go test ./pkg/checksum/ ./pkg/copier/ pass on 4d39e3e.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 2/2: OSS adoption and integration ease, at 4d39e3e. These are lenses, not correctness findings. 0 blocking, 5 non-blocking.

The shape is easy to adopt. A string-typed DivergencePolicy maps one-to-one onto D14's --on-divergence=abort|repair and onto an importer's config value. DivergenceError and RepairError are errors.As-able and carry chunk ranges and row counts, which is enough to render without parsing a message. The (proof, bool) accessors make a forgeable zero value hard to use by accident.

1. Let importers validate a policy when they load config. validate is unexported (policy.go:30). So an orchestrator that reads the policy from configuration or a flag only finds out it is wrong at its first Check, which comes after the whole copy. An exported ParseDivergencePolicy(string) (DivergencePolicy, error) (or a Valid() method) returning the same ErrNoDivergencePolicy would let SchemaBot and others refuse the setting at startup.

2. State on Check who else may write the shadow during a repair pass, and make the two design texts agree. low-level-design.md:443-445 says the repair pass "re-copies divergent chunks while the new slot's stream is being applied". low-level-design.md:719-721 runs it "before resuming CDC from the new slot". The repair's DELETE then recopy overwrites landed rows. That is fine with no applier running. With one running, an applier UPDATE that blocked on a deleted row affects zero rows once the repair commits (measured on PostgreSQL: UPDATE 0, and the recopied row stands). The reread then fails the pass. A third-party orchestrator will wire this from the godoc, so the precondition belongs there. Finding 4 in 1/2 is the same question asked about source writes.

3. Keep copier.InsertChunk out of the public API (copy_chunk.go:55). At v0.x, each exported symbol is surface that importers can start to depend on. This one is an unguarded shadow writer whose only intended caller is pkg/checksum. An internal/ package shared by copier and checksum keeps the single statement and drops the export (1/2 finding 5).

4. Progress is still dark during Check. The PR body defers the checksum progress.WorkSource until there is an OperationChecksum. That is fine as sequencing. But SchemaBot renders only what the engine reports, so a multi-hour pass and its repairs will show no movement until that lands. Counters for chunks compared, rows hashed and chunks repaired would cover both phases, and should land before an orchestrator wires Check.

5. Doc accuracy: the verifier's guard is not "the same guard as the copier" on search_path. SAFETY.md:23 says it is, and so does the pkg/checksum row of the design's package map. But setCopySession sets no search_path (copy_chunk.go:87-98), while the verifier pins pg_catalog alone. The copy statement is fully qualified, so this changes nothing today. It does mean that "a repair puts back exactly what a copy would have" (repair.go:60-61) is true of the statement but not of the session. One example: a shadow default that resolves a name at run time, such as an SQL-language function, can behave differently under the two paths. Either "the copier's guard plus a catalog-only search_path" in the docs, or the same LocalSearchPath in the copier, would make the claim exact.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving 4d39e3e with 0 blocking findings. The 1/2 comment has five non-blocking findings. Three of them come with tests: a refused Check returns an outcome whose Clean() is true, repairs that already committed are dropped from a failed result, and nothing pins the repair transaction's guard. The 2/2 comment has five non-blocking integration notes.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/136, follow-up commit

Verdict: no blocking findings; all five correctness findings are fixed (two of them as the reviewer proposed, one by a repair-pass precondition rather than a retry); of the five integration-lens notes, four are taken (policy parser, godoc precondition, internal/ package, copier search_path pin) and the progress filler is tracked as its own plan row. The internal review's unique-index collision is also fixed and changed the repair's shape: every differing chunk is now recopied in one transaction.

#	Finding	Status	Explanation
C1-F1	Outcome.Clean() answers true on every refusal, DivergenceError included	✅ Fixed	Clean() is now o.clean.Watermark().Valid(): it means "this pass minted the clean watermark", so an aborted pass, a repair pass, a lost lock, and a refused policy all read not clean. TestOutcomeZeroValueMintsNothing flips to False, the abort test asserts !outcome.Clean() with the Report carried, and the refused-policy test asserts the same — that assertion is the one that fails if Clean() goes back to Report.Clean().
C1-F2	A later chunk's failed repair drops the repairs that already committed	✅ Fixed	repairAll returns the committed repairs alongside the error, and Check returns Outcome{Report, Repairs} with a RepairError or LK-1. TestCheckReportsCommittedRepairsWhenALaterRepairFails deletes id 5 and zeroes id 1500 in the shadow, with a BEFORE INSERT trigger that rewrites qty only when NEW.id > 1000, and asserts the outcome lists both repairs (999 removed / 1000 inserted for the first; the second is the one the RepairError names), row 5 is back, and no proof is minted. Dropping repairs on the error path fails it.
C1-F3	No test pins that the repair transaction runs the guard	✅ Fixed	TestCheckRepairRefusesAReplacedShadowBeforeWriting is the reviewer's probe: a pool hook fires on the repair's begin read write, renames the shadow to displaced, and puts an impostor LIKE … INCLUDING ALL in its place. The pass is refused as ST-6, the impostor holds zero rows, and displaced is still corrupt. A bare pool.BeginTx in place of v.begin fails it.
C1-F4	On a table taking writes the reread reads a repair that worked as one that did not	✅ Fixed (precondition) / ⏳ live protocol deferred	Not taken as a bounded retry: a reread that is allowed to retry can no longer tell a hot row from a trigger that rewrites the chunk, and the thing that makes a transient difference safe to wait out is the applier's LSN feed, which is plan row 6c. Instead the precondition is stated where an orchestrator will read it: Check's godoc and doc.go say a repair pass assumes no other shadow writer and that source rows in differing chunks hold still until the rereads — the pass after slot loss runs before change capture resumes, and the resumed capture carries the application's writes. TestCheckRepairRefusesASourceWriteInsideThePass pins the fail-closed half: a source UPDATE injected after the recopy's INSERT makes the pass a RepairError with the recopy committed. low-level-design.md L443 now agrees with L719 that reconciliation runs before the new slot's stream is applied (C2-F2).
C1-F5	copier.InsertChunk is an exported shadow writer that runs no guard	✅ Fixed	InsertChunk and its test are gone. New pkg/internal/chunksql holds the one Insert(schema, source, shadow, key, columns) statement, with a frozen-text unit test; copier.copySQL and Verifier.copySQL both build from it, so the statement cannot drift and nothing outside the module can call it. .golangci.yml depguard allows pkg/internal/** for core files, and SAFETY.md's package map gets a pkg/internal/chunksql row (core, SQL text only, no connection).
I-1 (internal review)	A unique value that moved between two differing chunks makes the chunk-by-chunk recopy a unique_violation on every pass	✅ Fixed	recopy now runs every chunk's DELETE and then every chunk's insert in one guarded read-write transaction, committed together; rereads then run per chunk in fresh snapshots. TestCheckRepairsAUniqueValueThatMovedBetweenChunks puts a unique index on qty, swaps the values of ids 100 and 1500 on the source only, and asserts the pass repairs both chunks and the shadow equals the source; the chunk-by-chunk order fails it with SQLSTATE 23505. The tradeoff is one transaction sized by the number of differing chunks, taken because the reconciliation case this policy exists for is exactly the one where values move, and because a half-repaired shadow is then never visible.
I-2 (internal review), C2-F5	The copier and the repair run the same statement under different search_paths; the docs say the guards are the same	✅ Fixed (copier pinned)	setCopySession now runs dbconn.LocalSearchPath("pg_catalog") in the copy transaction, so "a repair puts back exactly what a copy would have" is true of the session as well as the statement, and the SAFETY.md and design package-map rows are exact. TestCopierIgnoresTheSessionSearchPath runs the copier on the catalog-shadowing pool with a decoy bigint <= operator and fails without the pin. The consequence to know about: a shadow default or expression that resolved an unqualified name through the caller's search_path now fails at copy time rather than at repair time, which is the CO-9 behavior the verifier already had.
I-3 (internal review)	Committed repairs lost on a later failure	✅ Fixed	Same change as C1-F2.
I-4 (internal review)	DivergenceError and RepairError do not name the table	✅ Fixed	Check wraps every refusal it returns, including LK-1 and ST-6 from the repair path, as verify <schema>.<table>: …; the EqualError assertions now pin the prefix.
C2-F1	Importers cannot validate a policy at config load	✅ Fixed	ParseDivergencePolicy(string) (DivergencePolicy, error) returns the typed value or ErrNoDivergencePolicy; TestParseDivergencePolicy covers both names, the empty string, and an unknown string.
C2-F2	Say on Check who else may write the shadow during a repair pass; make the two design texts agree	✅ Fixed	See C1-F4: godoc precondition on Check and in doc.go; the L443 bullet is rewritten so both places say the pass runs before the new slot's stream is applied.
C2-F3	Keep copier.InsertChunk out of the public API	✅ Fixed	See C1-F5.
C2-F4	Progress is dark during Check	⏳ Deferred	Agreed it lands before an orchestrator wires Check. Tracked as plan row 6f (cs6-progress): OperationChecksum, counters for chunks compared, rows hashed, and chunks repaired, and the WorkSource filler, with the contract change it needs. Not taken here because the progress contract's counter names are shared with the copier and the change deserves its own review; the #134 response had placed the filler in this PR, and this moves it.
C2-F5	"Same guard as the copier" is not true on search_path	✅ Fixed	See I-2: the copier now pins it, so the claim is exact.

Tests added or changed in the follow-up commit: TestCheckReportsCommittedRepairsWhenALaterRepairFails, TestCheckRepairsAUniqueValueThatMovedBetweenChunks, TestCheckRepairRefusesAReplacedShadowBeforeWriting, TestCheckRepairRefusesASourceWriteInsideThePass (new, pkg/checksum/repair_integration_test.go); TestCopierIgnoresTheSessionSearchPath (new, pkg/copier); TestInsertIsFrozen (new, pkg/internal/chunksql); TestParseDivergencePolicy (new); TestOutcomeZeroValueMintsNothing, the abort and refused-policy tests, and the EqualError assertions updated. insert_chunk_integration_test.go is deleted with the symbol it tested. Each new test was run against the mutation it exists to catch and fails under it (chunk-by-chunk recopy order, bare pool.BeginTx for the repair, repairs dropped on the error path, Clean() back to Report.Clean(), copier without the search_path pin). go test -race ./pkg/checksum/ ./pkg/copier/ against PostgreSQL 16, SKIP_INTEGRATION=1 go test ./..., gofmt -l ., go vet ./..., and golangci-lint run ./... are green.

Source: block/pg-sprite#136, review comments 5924708298 and 5924709068 at head 4d39e3e; internal review scratch/code-reviews/pg-sprite-pr136-review.md items I-1 to I-4 at 16eaf228