executor: split native.go into cic_* files by concern

[Kiran01bm]

Why

pkg/executor/native.go held everything about concurrent index creation (CIC) in one 1232-line file: the error sentinels, the lock and statement budgets, the build itself, invalid-index inspection, and backend-state classification. Any CIC change, whatever its subject, touched this one file. That made concurrent work collide in merges, and made a reader (or an agent) load the whole file to reason about one mechanism.

What

The file is split into five files, one per concern, and the three native*_test.go files follow the same split. Nothing is renamed or reworded; the set of top-level declarations and the count of test functions (169) are identical before and after.

File	Holds
cic_errors.go	the Err* sentinels
cic_budget.go	ConcurrentBudget, min-connection rules, budgeted session acquisition, cancellation classification
cic_build.go	BuildIndexConcurrently[WithProgress], the build report and verdicts, target resolution
cic_invalid_index.go	InvalidIndexError, builder facts, invalid-index inspection and classification
cic_backend.go	waiting for and classifying a stopped backend

A second commit adds one bullet to AGENTS.md under Conventions: one file per concern, split past a few hundred lines or when a file serves more than one feature, move with rename-sized diffs so history follows.

How

git mv for the two test files that map whole to one bucket (native_test.go → cic_build_test.go, native_integration_test.go → cic_build_integration_test.go), so history follows them unchanged. The other files are carved out by cut and paste with only the package and import blocks added. Files outside the package that use these symbols (recover.go, sequence.go, code.go) are untouched.

Risk

Low. Pure file move inside a core package; the compiler, go vet, lint, and the full executor test package (unit and integration) all pass.

Testing

Beyond CI: the declaration set was hashed before and after the move (rg '^(func|type|var|const)' pkg/executor/*.go, sorted) and matched, 254abb88…. The Docker-backed integration slice ran locally: go test ./pkg/executor/ -run 'BuildIndexConcurrently|InvalidIndex|Recover|Backend' -count=1 → ok … 55.1s.

Bigger picture

Other large files in the repo can follow the same pattern when they are next touched; this PR does not move anything else.

Generated with Amp (Claude)

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial review 1/2 — is the move actually pure? 9f0a16aa, 14 files, +1967/−1882.

A pure-move PR can't be reviewed by reading the diff — GitHub shows 1967 added lines and every one of them looks new. So I reviewed it by mechanically proving equivalence instead: strip package and import from both sides, and compare the remaining content as a line multiset. That is the check that catches a line silently lost in a cut-and-paste carve, which is the failure mode this PR's method is exposed to and which the compiler cannot see.

The four claims in the body all hold.

Claim	Verified
Declaration set identical	✅ ^(func|type|var|const) over pkg/executor/*.go, sorted, hashes equal before and after
169 test functions, unchanged	✅ 169 both sides, and the sorted set of test names hashes equal (a3907655e1f4), so none was renamed
Two files moved with git mv, history follows	✅ both show as 0-line renames under -M: native_test.go => cic_build_test.go, native_integration_test.go => cic_build_integration_test.go
recover.go / sequence.go / code.go untouched	✅ no other file in the package is in the diff

Also checked and clean: no init() anywhere in the package, so the rename cannot reorder package initialization; no //go:build tag on either side, so nothing changed which files compile; go build ./..., go vet ./pkg/executor/, and gofmt -l are all silent; go test -count=1 ./pkg/executor/ is ok 158.9s.

The line multiset is not quite equal, and the three differences are all real.

native.go -> cic_*.go                                 old=1159  new=1158
  ONLY IN OLD: -1x  // session and the verdict session reserved beside it.
native_internal_test.go -> cic_*_internal_test.go     old=578   new=582
  ONLY IN NEW: +1x  // White-box tests for the fail-closed decision helpers ... (4 lines)
native_test.go, native_integration_test.go            IDENTICAL

---

1. buildMinConns lost its doc comment, and half of it is stranded at EOF

The carve cut a two-line doc comment across the file boundary. In native.go it read:

// buildMinConns is the pool size a concurrent build needs: the build
// session and the verdict session reserved beside it.
const buildMinConns = 2

After the split, the first line is the last line of cic_errors.go — cic_errors.go:159, with nothing after it, a dangling half-sentence at end of file:

	ErrCancelledExternally = errors.New("the build was cancelled from outside the executor")
)

// buildMinConns is the pool size a concurrent build needs: the build
[EOF]

…while the constant itself landed in cic_budget.go:15 with no doc comment at all — sitting immediately above recoveryMinConns, which kept its two-line comment, so the asymmetry reads as deliberate. buildMinConns is the admission floor that cic_build.go:146 rejects an undersized pool against, and "why is this 2" is exactly the question the deleted line answered. It is also referenced by name from a comment in a third file, cic_errors.go:112.

Restore both lines above the constant in cic_budget.go and drop the orphan from cic_errors.go.

2. Two comments in the package still point at native.go, which this PR deletes

The body's "untouched" claim is true of symbols, but two files carry the deleted file's name in their doc comments, and a reader following either finds nothing:

• recover.go:2 — "the automatic counterpart of the refusal in native.go". That refusal is now classifyInvalidIndex / InvalidIndexError in cic_invalid_index.go.
• optimistic.go:8 — "the concurrent index build (see native.go)". Now cic_build.go.

Both are in files the PR deliberately did not open, which is why they were missed. recover.go's is the more costly one: it is the opening line of a file implementing LK-5 recovery, so it is the first thing a reader of the recovery path is pointed at.

Worth repointing at the symbol rather than the new file name — see 2/2.

3. The internal-test header is duplicated into two files and is now wrong in both

native_internal_test.go opened with a four-line header describing what its white-box tests cover. The split copied it verbatim into two of the four carved files — cic_build_internal_test.go:1 and cic_backend_internal_test.go:1 — and gave the other two none. That is the +4 above: one extra copy.

The text names two examples, "unknown backend states, a replaced target table", and after the split those live in different files: backend states in cic_backend_internal_test.go, the replaced target in cic_build_internal_test.go. So each file now carries a header that half describes its neighbour, while cic_budget_internal_test.go and cic_invalid_index_internal_test.go — carved from the same original and equally white-box — start at bare package executor.

Either give each of the four a one-line header naming its own subject, or keep one shared header and put it where the convention in 2/2 would expect it.

---

Not a finding, but worth recording: the file/test pairing the PR writes into AGENTS.md is honored by the split itself. Counting, per internal test file, how many symbols it references from each cic_*.go, every one references its own sibling most:

cic_backend_internal_test.go        own=2   other={}
cic_budget_internal_test.go         own=4   other={}
cic_build_internal_test.go          own=7   other={cic_invalid_index: 3, cic_budget: 1}
cic_invalid_index_internal_test.go  own=8   other={cic_build: 1}

The grouping is sound and the file names do not lie about their contents. Invariants in 2/2.

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

[aparajon]

🤖 Review 2/2 — two lenses: OSS adoption, and integration ease for importers.

Lens 1: OSS adoption

A 1232-line file holding five distinct mechanisms is a real barrier for anyone evaluating pg-sprite from the outside, and this split removes it. Someone landing on the repo to answer "how does this do CREATE INDEX CONCURRENTLY safely?" can now read cic_build.go for the build, cic_budget.go for the bounds, and cic_invalid_index.go for what happens when it fails, instead of paging through one file to find the boundaries themselves. The file names carry information the old name did not: native.go told you nothing about which native path it held.

One thing this PR should fix before merge, because it is public-facing rather than internal. The package executor doc comment lives at the top of optimistic.go, and it ends with "…the native executors for the classified safe idioms, starting with the concurrent index build (see native.go)." That is not just a code comment — it is the package synopsis, and it is what go doc and pkg.go.dev render. On this branch:

$ go doc ./pkg/executor
package executor // import "github.com/block/pg-sprite/pkg/executor"

Package executor runs schema changes against the database. ...
... and the native executors for the classified safe idioms, starting with
the concurrent index build (see native.go).

So the first paragraph an evaluator reads on pkg.go.dev will point at a file that does not exist in the tree. This is the same defect as finding 2 in 1/2, but its blast radius is the published documentation rather than an in-repo comment, which is why it is worth pulling out separately. Point it at the symbol — "(BuildIndexConcurrently)" — which both survives the next split and hyperlinks on pkg.go.dev; a file name does neither.

A smaller adoption note: cic is unexpanded anywhere a reader will meet it. The abbreviation is expanded once in this PR's description and nowhere in the tree — not in a file header, not in the package doc, not in AGENTS.md. An outside reader (or an agent) seeing cic_budget.go next to plainly-named optimistic.go, recover.go, sequence.go, create.go has to infer it. One clause in the package doc — naming concurrent index creation and saying its files carry the cic_ prefix — turns the prefix from a barrier into a signpost, and costs a line.

Lens 2: integration ease for schemabot and other orchestrators

The exported API surface is byte-identical. I rendered go doc -all ./pkg/executor before and after and hashed the declaration lines: e82306dc3744 both sides. Nothing moved package, nothing changed signature, nothing was exported or unexported. Every importer is unaffected — this is a no-op at the module boundary, and that is the strongest thing that can be said about a refactor of a core package in a dependency.

Worth making concrete, because it shows the split actually serves the consumer contract rather than merely not breaking it. An orchestrator consuming this package uses, from the files this PR touches, 17 symbols — and 12 of the 17 are Err* sentinels: ErrPoolTooSmall, ErrCancelledExternally, ErrInvalidIndexNotDroppable, ErrAbandonmentUnproven, and the rest, matched with errors.Is and mapped to the orchestrator's own outcome codes. The remaining five are BuildIndexConcurrentlyWithProgress, ConcurrentBudget, IndexBuildReport, InvalidIndexError, and ErrNotConcurrentIndexBuild.

That distribution is the argument for cic_errors.go existing at all. The sentinel set is the integration contract — it is the entire vocabulary an orchestrator branches on — and it now sits in one 159-line file that can be read end to end to enumerate every failure mode a caller must handle. Previously that required extracting one var block from the middle of 1232 lines. For anyone writing a second orchestrator against this engine, that file is the most useful artifact in the package.

The coupling this loosens, worth one sentence of vigilance. Proximity used to enforce something: a sentinel and the code returning it were in the same file, so adding one meant seeing its call site. Now a sentinel can be added to cic_errors.go without opening the file that returns it, and a sentinel that no caller can distinguish is worse than none — it looks like a contract and isn't. Not a change to request here; just the cost this file boundary buys, and the thing to watch on the next PR that adds an Err*.

Timing is clean and that is not permanent. No other open PR in the repo touches pkg/executor, so this lands with zero conflict cost today. That window is exactly what a 1967-line rename-shaped diff needs, and it is also the thing the PR's own motivation predicts will close — so this is a merge-soon PR rather than a sit-and-marinate one. Anyone carrying a fork or an in-flight branch against native.go should rebase before this lands rather than after.

Approving. The three findings in 1/2 plus the package-doc reference above are all comment-only, none blocks, and all four are two-line fixes worth a follow-up commit before merge — the package-doc one most of all, since it is the copy that ships to pkg.go.dev.

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

[aparajon]

🤖 Approved. The move is genuinely pure where it matters: the declaration set, the 169 test names, and the stripped line multiset all match before and after, both git mv files are 0-line renames, there is no init() to reorder, and build/vet/gofmt/tests are clean. Three findings in the review comments, all comments rather than code — the buildMinConns doc comment was cut in half across the file boundary and its constant now has none, two files still point readers at the deleted native.go, and the white-box header was duplicated into two files where it is now half wrong in each. Worth a follow-up commit before merge; none of them blocks. The invariants registry survives untouched because it cites symbols rather than file paths, which is the pattern those two broken comments should adopt.

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

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude) — block/pg-sprite pull/110, follow-up commit

All three findings are fixed across two follow-up commits; the move is now doc-comment-pure as well as code-pure, and no comment in the package refers to a file that no longer exists.

#	Finding	Status	Explanation
1	C1-F1 — buildMinConns lost the second half of its doc comment in the move	Fixed	The seven doc comments the split displaced (buildMinConns among them) are reattached to their declarations in full. An AST checker that pairs every top-level declaration in the pre-split file with its doc text and compares against the split files reports zero mismatches, so the check is by symbol rather than by eye.
2	C1-F2 — recover.go and optimistic.go headers reference the deleted native.go	Fixed	Both headers now name the symbol they depend on rather than the file that used to hold it: recover.go points at InvalidIndexError, optimistic.go at the executor it drives. A file name is a location that can move again; a symbol name survives the next split.
3	C1-F3 — the internal-test header was duplicated into two files and describes neither	Fixed	Each of the four cic_*_internal_test.go files carries its own header stating which unexported surface it reaches and why that needs package-internal access; the copied header is gone.
4	C2 — package doc says (see native.go), which renders on pkg.go.dev after the file is gone	Fixed	The package doc names BuildIndexConcurrently as the entry point and no longer references a file.
5	C2 — cic is never expanded for a first-time reader	Fixed	The package doc expands the cic_ prefix on first use as the concurrent-index-build (CREATE INDEX CONCURRENTLY) family, so the file names are self-explaining from the package page.
6	C1 — test/sibling file pairing is sound	No action	Verified correct by the review; unchanged.
7	C2 — exported API byte-identical; sentinel-file coupling to watch; merge-soon timing	No action	The API-surface and timing observations are accepted as stated. The sentinel-file coupling is a standing vigilance note rather than a change to make here; the per-file headers from #3 now say which unexported symbol each test reaches, which is the signal a future author needs before moving one.

Source: block/pg-sprite#110, review comments 5643746422 and 5643746666 and review 5185396043 at head 8cd78008