refactor(adapter): typed FileOp.Action and an explicit OpCleanup kind (#230)

[spxrogers]

Summary

Closes #230. Third PR in the #226 code-quality series (after #240, #244, #247). No behaviour change.

1. adapter.FileOp.Action is a typed enum whose zero value is write (commit 1, f11bccc).
Action was a string with a documented "" == "write" convention that render.Plan, render.Apply and render.PreviewApply each rewrote at intake, and that every downstream reader re-asserted in a "plan-normalized vs raw adapter output" comment. It is now adapter.Action (ActionWrite = zero value, ActionDelete, String()), so there is no empty spelling to normalize: the three intake rewrites and the four reader comments are deleted. The containment backstops in Plan and applyPlan stay — they were always the load-bearing half, and their tests are rewritten to prove the property that survives: there is no spelling that writes while dodging a guard (the fixtures are built without an Action on purpose, and say so). %q/ui.Pad surfaces go through String(), so the dry-run label and DispatchOps' error text are unchanged for every value a producer can make. 41 adapter Action: "write" sites (38 files) are a pure token swap.

2. Orphan-cleanup ops carry an explicit Kind: adapter.OpCleanup, built by one constructor (commit 2, 0207296).
apply used to identify the synthesized empty key-merge write that prunes an emptied section's owned keys by sniffing IsKeyMerge && TrimSpace(Content) == "{}" && len(OwnedKeys) > 0 — three copies in apply.go, plus a registry-wide test whose stated job was to keep that signature unique. The op is now stamped at synthesis, by the single producer adapter.NewCleanupOp(path, strategy, owned) called from both render.orphanCleanupOps and agent disable --purge; the three sniffs read op.Kind. Kind is orthogonal to Action — a cleanup op is an ActionWrite (the merge path performs the removal via OwnedKeys), so it is not a third action and no == ActionWrite site had to grow an || cleanup. "Only producer" is enforced in both directions, not asserted: TestEveryCleanupLiteralUsesNewCleanupOp (sibling of TestEverySkipLiteralSetsKind) fails any production FileOp literal — direct, or a type-elided element of any slice/array/map nesting — that hand-rolls the cleanup shape (a static {} into a key-merge destination) or stamps Kind: OpCleanup by hand outside the constructor; a whole-file {} write is not the shape and passes. The pinning test keeps its name and body with its rationale rewritten to the contract it still enforces (adapter fidelity: rendering {} for a populated section is a silent drop).

3. The OwnedKeys field comment was wrong on both counts and is corrected (doc only): it is populated by render.Plan from state (scoped to the op's own top-level sections) on every key-merge op when Plan is given a state, not by Apply; and three adapters do set it in Render as a fallback for Apply-without-the-pipeline, which Plan overwrites.

Deliberately out of scope: typing MergeStrategy → #250. The cheap partial is only ≈7 prod + 2 test files, ~40 lines — but it leaves a half-typed API (a typed field fed by a string interface accessor through explicit conversions), and the full version changes the published Adapter interface. That belongs with its own issue; the CHANGELOG and architecture doc say so, and #250 carries the one guard-side follow-up (teach the shape exemption the typed replace constant).

Why this is behaviour-preserving, measured rather than argued.

• FileOp is never serialized (no json: tags, no --json payload carries one; planItem is pinned all-unexported), so there is no wire form to preserve.
• The Extract a shared plan→drift-classification walk for status/diff/reconcile/explain #229 characterization harness (27 fixtures × status/diff/reconcile/explain goldens) needed exactly two lines — the fileOp/keyOp fixture constructors — and zero golden edits.
• Every production FileOp literal already set Action explicitly (the "" default was documented but never used), and ActionDelete never enters a plan's Ops (orphan deletes are synthesized inside applyPlan; purge and the drift walk build their own slices), so the walk's != ActionWrite filter stays as belt-and-braces.
• The one place two spellings of "is this a write" disagreed — reconcile's rendered set rejected "" while the walk accepted it — collapses to one predicate. On the old code, respelling only the harness constructors to "" fails exactly one fixture (T-19, the reconcile rendered projection), so that set is a live oracle; applying the collapse to those same "" fixtures is fully green. The only thing that could move T-19 is the "" spelling, which no fixture uses and which the typed model makes unrepresentable.

Reading the diff. Commit 1 is 67 files, 41 of them the one-line adapter swap; commit 2 is 7 files and is where the thought is; commit 3 is docs; commits 4–6 close the three review-loop rounds. The plan and execution spec were reviewed by a fresh reviewer before execution.

Review loop (three rounds, four read-only lenses each — correctness, adversarial, API design, test rigor). Round 1 on 21b7d14: two ISSUEs — printPlannedOp's Kind branch was unpinned (the dry-run test's Contains(dry, "remove") matched the summary line) and "only producer of OpCleanup" was asserted, not enforced — plus prose nits; fixed in cfa9d2e. Round 2 on cfa9d2e: NIT-only, three lenses ship-it; all taken in 9576712 (guard narrowed to the cleanup shape and made two-directional, the allow-window pinned on parsed snippets, the real-apply headline pinned, doc sentences). Round 3 on 9576712 (targeted at the round-2 changes): NIT-only, three lenses ship-it, a 16-mutation sweep of the guard found one unpinned condition; all taken in a886a45 (container-generic elision walk, the method-receiver row, the headline pin anchored to its line, the escape-hatch clause, doc wording). Severity trace ISSUE → NIT → NIT.

Type of change

• Bug fix
• New feature / enhancement
• Refactor (no behavior change)
• Docs
• Tests / CI / tooling

Test plan

New tests:

• internal/adapter/action_test.go — TestFileOpEnums_ZeroValues (the one explicit pin that the zero values are write/render, via bare var), TestFileOpEnums_String (String() and %q for both enums incl. the out-of-range fallback), TestNewCleanupOp (every field the constructor sets).
• internal/adapter/cleanupop_guard_test.go — TestEveryCleanupLiteralUsesNewCleanupOp (AST walk of every production file under internal/; a FileOp literal, direct or a type-elided element of any slice/array/map nesting, is flagged if it is positional, stamps Kind: OpCleanup by hand, or hand-rolls the cleanup shape — all outside NewCleanupOp's own body; anti-vacuity floor, matched count logged), TestCleanupOpStaticGuardScan (the per-file scan on thirteen parsed snippets: the allow-window inside/outside/other-package/method-receiver, typed element counted once, elided slice/map/nested elements, the whole-file {} negative, the positional case) and TestCleanupOpStaticGuardMatchers (16-row matcher self-test).
• internal/render/pipeline_cleanup_internal_test.go — TestOrphanCleanupOps_StampsOpCleanup: the real orphanCleanupOps; with the dest on disk the op is ActionWrite/OpCleanup/{}/the owned pointer; with the dest already gone, no op.

Rewritten (not retired): TestPlan_ZeroActionIsWriteAtTheGuards, TestApply_ZeroActionForCallerBuiltPlans, two TestDispatchOps cases, TestWalkPlanItems/action-not-write-is-skipped — all built without an Action so the zero-value contract is pinned; TestAdapters_NeverRenderEmptyObjectForPopulatedSection (rationale only); TestApplyDryRun_CleanupOpNotCountedToWrite (label assertion anchored to the op's own line with the path trimmed; the real apply's removed: 1 key(s) headline line pinned to carry no applied partition).

Break-verifies run before commit:

• Swap the iota order so the zero value is ActionDelete → 7 test functions fail in 3 packages. Converting those fixtures to an explicit ActionWrite would drop that to 2, which is why they are zero-value on purpose.
• Delete the Kind: OpCleanup line inside NewCleanupOp → exactly TestNewCleanupOp (adapter), TestOrphanCleanupOps_StampsOpCleanup (render), TestApplyDryRun_CleanupOpNotCountedToWrite (cli) fail.
• Invert printPlannedOp's Kind check, or swap its label to "write" → the dry-run test fails on the op line. Drop removalCounts' appliedOps--, or print applied: 0 ops, removed: … → it fails on the headline line.
• Hand-roll the shape in orphanCleanupOps (direct, or with "{ }") or in purgeAgentDests (elided slice element) → the guard names render/pipeline.go:330 / cli/agent.go:715; hand-stamp Kind: adapter.OpCleanup there → flagged as a hand stamp; a positional bare literal in adapter.go → flagged; a whole-file {} write with no strategy → not flagged. Widen the allow-window to the whole file, drop the receiver condition, count typed elements twice, or drop map support → a snippet row fails each.
• Disable the dest-exists gate → the "dest already gone" subtest fails.

Gates, each commit independently and again on the head: go build, go vet, gofmt -l empty, gofumpt, AGENTSYNC_TEST_IN_CONTAINER=1 go test ./... (29 packages ok), go test -race ./internal/cli/..., go test -tags=e2e ./test/e2e/..., go test -tags=bdd ./test/bdd/..., GOTOOLCHAIN=go1.26.2 golangci-lint@v2.12.2 run ./... → 0 issues.

• just test-release is green (the release bar) — just is not installable in this session; every layer the recipe orchestrates (vet → build → race → e2e → bdd) was run directly as listed above, and CI's hermetic test-release job is green on the PR.
• just lint is clean — run directly with the pinned toolchain; go.mod/go.sum untouched.

Checklist

• Conventional commit messages with a scope (e.g. fix(secrets): …).
• Tests added/updated for the behavior changed.
• If this touches internal/secrets, internal/capture, or any
  source.Write* path, I've re-read the secret-handling invariants in
  CLAUDE.md / SECURITY.md and not weakened them. — Not touched: no file under internal/secrets, internal/capture or internal/source changes; walkSecretFields never visits FileOp.
• Docs updated if behavior, CLI surface, or capability coverage changed. — docs/architecture.md, docs/components.md, CHANGELOG.md; the website contract pages are generated from those, and no authored page mentions FileOp or an action string.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG