test(clean): CLI e2e test covering all 9 migrations budnled into go test

[ran-codes]

Context

• notion-sync clean is the post-upgrade migration command — it absorbs one-time fixes (S3 URL stripping, notion-frozen-at removal, URL canonicalization, folderPath normalization, syncVersion bumps, AGENTS.md regen) so routine refreshes produce focused diffs.
• Until now, clean had unit coverage in internal/clean/clean_test.go and a hand-run smoke test, but no end-to-end coverage of the binary's CLI surface: arg parsing, exit codes, the stdout summary text users grep against, or interaction effects across migration types in one pass. PR fix(clean): surface URLs canonicalized in dry-run summary #71 was a recent example of that gap biting (the URLs canonicalized count was missing from dry-run output).
• The 3 existing system-test skills hit real Notion, so a network-free CLI test fits as a Go test rather than a skill — runs in go test ./... and CI for free, no manual invocation needed. Reframed accordingly in Add CLI-level e2e test for the clean command #66.

Changes

• New file cmd/notion-sync/clean_e2e_test.go (~300 lines).
• Synthetic temp-dir seed encoding all 9 documented migrations from internal/clean/clean.go's package doc:

  #	Migration	Fixture
  1	Strip S3 presigned query strings (.md)	entry.md PDF field
  2	Remove notion-frozen-at: line	entry.md
  3	Canonicalize notion-url: (.md)	entry.md
  4	Canonicalize "url" (JSON)	_database.json + _page.json
  5	Normalize folderPath separator	_database.json + _page.json
  6	Append trailing newline	pages/.../My Page.md
  7	Bump syncVersion in _database.json	My Database/_database.json
  8	Bump syncVersion in _page.json	pages/.../_page.json
  9	Regenerate stale AGENTS.md	workspace AGENTS.md (v0.0.1)

• Three phases asserted: dry-run is read-only (counts reported, files unchanged), real run applies every migration (count assertions + post-state byte checks), second run is idempotent.
• Top-of-file API-drift caveat: the seed encodes notion-sync's historical output shapes; if the importer's output contract changes meaningfully, the seed may need regeneration.
• Mutation pass at write-time: each of the 9 migrations was disabled one at a time and the test confirmed to fail on the matching assertion. Every migration is caught by ≥2 independent assertions (count + post-state, or two post-states).
• No production code changes — pure additive test file.

Related to #66.

---

🤖 Generated with Claude Code

[ran-codes]

Code Review (Pass 1)

Warning

Verdict: Needs Discussion
Test logic is sound and migration coverage is excellent — one false-pass risk in the negative test should be tightened before merge.

Action Items

• Negative test can pass for the wrong reason — cmd/notion-sync/clean_e2e_test.go:232 TestCLI_Clean_MissingFolderArg_ExitOne only checks err == nil from go run .. A build break, missing go, or any non-zero exit makes this test silently "pass" the negative path. Capture CombinedOutput() and assert stderr contains "missing folder path" (the actual message from runClean at main.go:580). While there, either tighten to assert exit code 1 via *exec.ExitError/ExitCode(), or rename to _NonZeroExit.

Suggestions

• Build once in TestMain, exec the binary — Four serial go run . invocations pay linker/cache-lookup overhead each time, and the cold-CI first run rebuilds from scratch. Standard pattern: go build -o ${tempdir}/notion-sync . once in TestMain, then mustRunClean execs that artifact. Bonus: you're now testing the actual binary the user installs, not a transient go run artifact.
• readFile shadows os.ReadFile — cmd/notion-sync/clean_e2e_test.go:294 Rename to mustReadFile to mirror the existing mustRunClean convention (both fail-the-test-on-error helpers).
• Split into subtests — Wrap dry-run / real-run / idempotency in t.Run("dry_run", ...) etc. Shared seed stays above. CI failure output then points at the broken phase, and -run lets you iterate on one phase. ~10-line refactor.
• Cross-reference the format string — main.go:604's fmt.Printf template is the contract this test asserts against. Add a one-line comment there pointing at this test so future format-tweakers see the coupling.

Notes

• Migration coverage table — Header table mapping the 9 migrations to fixture files is exactly the documentation a future maintainer will need.
• API-drift caveat — clean_e2e_test.go:25-29 correctly flags that the seed encodes historical output shapes. Right defensive note for a synthetic-fixture test.
• "Expected counts" derivation comment — clean_e2e_test.go:142-145 explaining why the numbers are 1/3/2/1/1 is the kind of narrative tests usually omit. Keep it.
• Layered assertions — Each migration is checked by ≥2 independent assertions (count + post-state, or two post-states). Combined with the mutation testing mentioned in the PR description, this is unusually rigorous coverage for a test PR.
• JSON backslash encoding — Fixture correctly uses \ between JSON quotes to represent a single backslash in folderPath, matching what a real Windows-side write produces. Easy to get wrong.

---

Review feedback assisted by the critical-code-reviewer skill.

[ran-codes]

Review pass 1 — applied changes (9e9c905)

Three findings from a self-review pass, all addressed in cmd/notion-sync/clean_e2e_test.go:

1. go run was shelling out 4× per test — replaced with build-once

mustRunClean was exec.Command("go", "run", ".", ...), paying the toolchain build cost on every invocation (3× in the main test + 1× in the missing-arg test). Added a package-level TestMain that builds the binary once into a temp dir and exposes testBinaryPath; both tests now exec that prebuilt binary. TestCLI_Clean_MissingFolderArg_ExitOne dropped from ~3.9s to 0.02s as a result.

func TestMain(m *testing.M) {
    dir, _ := os.MkdirTemp("", "notion-sync-e2e-")
    bin := "notion-sync-e2e"
    if runtime.GOOS == "windows" { bin += ".exe" }
    testBinaryPath = filepath.Join(dir, bin)
    exec.Command("go", "build", "-o", testBinaryPath, ".").CombinedOutput()
    code := m.Run()
    os.RemoveAll(dir)
    os.Exit(code)
}

2. Substring count assertions had false-positive risk

strings.Contains(out, "1 URLs stripped") also matches "11 URLs stripped", "21 ...", etc. Anchored every count with the literal punctuation that precedes it in the summary line emitted by runClean (cmd/notion-sync/main.go:604):

Modified: N files (N URLs stripped, N URLs canonicalized, N folderPaths normalized, ...)

So "1 URLs stripped" → "(1 URLs stripped" (after the (), and the rest become ", N <metric>" (after the comma-space). Same anchoring applied to the idempotent-run zero-counts block.

3. MissingFolderArg test only checked exit code

Previously asserted "non-zero exit," which would also pass if the binary failed for an unrelated reason (build regression, panic, future validation path change). Now captures CombinedOutput and asserts the actual error string "missing folder path" from runClean's validation:

out, err := exec.Command(testBinaryPath, "clean").CombinedOutput()
if err == nil { t.Fatalf("expected non-zero exit ...") }
if !strings.Contains(string(out), "missing folder path") {
    t.Errorf("expected 'missing folder path' in stderr, got:\n%s", out)
}

Verified

go test ./cmd/notion-sync/
ok  github.com/ran-codes/notion-sync/cmd/notion-sync  7.010s

All package tests still pass — TestMain is package-wide but the existing main_test.go tests that use go run . are unaffected (they just don't take advantage of the prebuilt binary; could migrate them later if CI cost becomes a concern).

[ran-codes]

Code Review (Pass 2)

Note

Verdict: Approve
Pass-1 required changes addressed in c8e0343; remaining items are optional polish.

Suggestions

• TestMain optimization scope — cmd/notion-sync/main_test.go:75-127 Four CLI tests still call exec.Command("go", "run", ".", ...). testBinaryPath is package-level and could be shared, shaving toolchain cost from those four tests too. PR description's "4 compiles → 1" framing currently only applies to the new e2e file.
• append(expectedCounts, ...) shared backing slice — cmd/notion-sync/clean_e2e_test.go:191,203 Works today because Go literal slices have cap == len so each append reallocates. Fragile if a future maintainer changes expectedCounts construction (e.g., to make([]string, 0, 16)) — the second call would silently overwrite the first. Cheap insurance: build one slice per call or use slices.Concat.
• Singular/plural grammar in summary line — cmd/notion-sync/main.go:604 "1 URLs stripped" / "1 trailing newlines added" reads wrong for count=1. Test asserts on the broken grammar to anchor counts, so any fix needs a paired test update.

Notes

• All 9 documented migrations exercised — Verified the seed in clean_e2e_test.go:97-160 hits every migration listed in internal/clean/clean.go's package doc. Counts match cmd/notion-sync/main.go:604-606 printf format.
• Tests pass clean against current code — go test ./cmd/notion-sync/ -v green for both the new e2e tests and the pre-existing main_test.go suite. No regression from the new TestMain.
• Pass-2 fix verified — expectMetadataMigrated now asserts databaseId/pageId, title, lastSyncedAt, and entryCount round-trip through Write{Database,Page}Metadata. AGENTS.md assertion anchors on the exact <!-- notion-sync-version: dev --> stamp.
• "Mutation pass" claim is unverifiable — PR description states each migration was disabled and the test confirmed to fail. No mutation harness committed; treat as author claim, not a repo guarantee. Assertion structure (count + post-state per migration) provides reasonable mutation-resistance regardless.

---

Review feedback assisted by the critical-code-reviewer skill.