From f11bccc872d846a49ecb2645792d1c1b75b86331 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 16:29:07 +0000 Subject: [PATCH 1/6] refactor(adapter): typed FileOp.Action whose zero value is write (#230) `FileOp.Action` was a string with a documented `"" == "write"` convention: `render.Plan`, `render.Apply` and `render.PreviewApply` each rewrote "" to "write" at intake so their guards (which matched the literal "write") could not be dodged, and every downstream reader carried a comment saying whether its ops were plan-normalized or raw adapter output. It is now `adapter.Action` (`ActionWrite`, `ActionDelete`) with `ActionWrite` as the zero value, so a FileOp built without an action writes, there is no empty spelling, and the three normalizations plus their comment tax are deleted. `String()` keeps the dry-run label and `DispatchOps`' error text byte-identical for both valid values; an out-of-range value reads "action()". Every production producer now sets `Action: adapter.ActionWrite` / `adapter.ActionDelete` explicitly (a pure token swap at 41 adapter Render sites plus the five pipeline/CLI sites). The four `!= "" && != "write"` guards and reconcile's `!= "write"` collapse to `!= adapter.ActionWrite`, which is exactly behaviour-preserving for every plan `render.Plan` can build (all its ops were already spelled "write"). The `OwnedKeys` field comment is corrected while here: it is populated by `render.Plan` from state when Plan has one, not by Apply; a Render-set value is only a fallback. Tests: the two normalization tests become `TestPlan_ZeroActionIsWriteAtTheGuards` / `TestApply_ZeroActionForCallerBuiltPlans` (same containment assertions; the ops are deliberately built without an Action so the zero-value contract stays pinned), `TestDispatchOps` gains a zero-value case and an out-of-range case, and the new `internal/adapter/action_test.go` pins the zero value and `String()`. No golden changes: the #229 characterization harness differs only in its two fixture constructors. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- internal/adapter/action_test.go | 47 +++++++++++++ internal/adapter/adapter.go | 58 +++++++++++---- internal/adapter/claude/apply_test.go | 6 +- internal/adapter/claude/command.go | 2 +- internal/adapter/claude/hook.go | 2 +- internal/adapter/claude/largeint_test.go | 2 +- internal/adapter/claude/memory.go | 2 +- internal/adapter/claude/render.go | 2 +- internal/adapter/claude/skill.go | 4 +- internal/adapter/claude/subagent.go | 2 +- internal/adapter/cline/command.go | 2 +- internal/adapter/cline/mcp.go | 2 +- internal/adapter/cline/memory.go | 2 +- internal/adapter/codex/command.go | 2 +- internal/adapter/codex/hook.go | 2 +- internal/adapter/codex/mcp.go | 2 +- internal/adapter/codex/memory.go | 2 +- internal/adapter/codex/subagent.go | 2 +- internal/adapter/continuedev/command.go | 2 +- internal/adapter/continuedev/mcp.go | 2 +- internal/adapter/continuedev/memory.go | 2 +- internal/adapter/cursor/apply_test.go | 2 +- internal/adapter/cursor/command.go | 2 +- internal/adapter/cursor/hook.go | 2 +- internal/adapter/cursor/mcp.go | 2 +- internal/adapter/cursor/memory.go | 2 +- internal/adapter/cursor/subagent.go | 2 +- internal/adapter/dispatch.go | 14 ++-- internal/adapter/dispatch_test.go | 26 +++---- internal/adapter/gemini/command.go | 2 +- internal/adapter/gemini/hook.go | 2 +- internal/adapter/gemini/mcp.go | 2 +- internal/adapter/gemini/memory.go | 2 +- internal/adapter/gemini/subagent.go | 2 +- internal/adapter/generic/render.go | 4 +- internal/adapter/noop/noop_test.go | 4 +- internal/adapter/opencode/apply_test.go | 10 +-- internal/adapter/opencode/command.go | 2 +- internal/adapter/opencode/ingest_test.go | 2 +- internal/adapter/opencode/memory.go | 2 +- internal/adapter/opencode/render.go | 2 +- internal/adapter/opencode/subagent.go | 2 +- internal/adapter/roo/command.go | 2 +- internal/adapter/roo/mcp.go | 2 +- internal/adapter/roo/memory.go | 2 +- internal/adapter/windsurf/command.go | 2 +- internal/adapter/windsurf/mcp.go | 2 +- internal/adapter/windsurf/memory.go | 4 +- internal/cli/agent.go | 4 +- internal/cli/apply.go | 9 +-- internal/cli/apply_internal_test.go | 4 +- internal/cli/import.go | 4 +- internal/cli/iss162_internal_test.go | 2 +- internal/cli/iss227_internal_test.go | 2 +- internal/cli/planwalk.go | 6 +- .../cli/planwalk_characterization_test.go | 4 +- internal/cli/planwalk_internal_test.go | 18 ++--- internal/cli/reconcile.go | 4 +- internal/cli/traversal_guard_test.go | 2 +- internal/render/iss162_test.go | 4 +- internal/render/pipeline.go | 41 +++-------- internal/render/pipeline_test.go | 70 ++++++++++--------- internal/render/report_test.go | 50 ++++++------- internal/render/state_apply.go | 10 +-- internal/render/state_apply_test.go | 18 ++--- internal/render/writer_fifo_unix_test.go | 4 +- internal/render/writer_test.go | 52 +++++++------- 67 files changed, 309 insertions(+), 252 deletions(-) create mode 100644 internal/adapter/action_test.go diff --git a/internal/adapter/action_test.go b/internal/adapter/action_test.go new file mode 100644 index 00000000..d804cb83 --- /dev/null +++ b/internal/adapter/action_test.go @@ -0,0 +1,47 @@ +package adapter_test + +import ( + "fmt" + "testing" + + "github.com/spxrogers/agentsync/internal/adapter" +) + +// TestFileOpEnums_ZeroValues is the one explicit pin of the premise the typed +// FileOp fields rest on: a FileOp built without naming an action writes. The +// intake normalizations ("" → "write") were deleted on the strength of this, +// so the bare `var` declaration — never an explicit constant — is the point of +// each subtest. +func TestFileOpEnums_ZeroValues(t *testing.T) { + t.Run("zero Action is ActionWrite", func(t *testing.T) { + var a adapter.Action + if a != adapter.ActionWrite { + t.Fatalf("zero adapter.Action = %v, want ActionWrite", a) + } + }) +} + +// TestFileOpEnums_String pins the human surface the dry-run op label and +// DispatchOps' error text depend on: %s and %q both route through Stringer, so +// an out-of-range value reads "action()" rather than a bare integer. +func TestFileOpEnums_String(t *testing.T) { + tests := []struct { + name string + v fmt.Stringer + want string + }{ + {name: "ActionWrite", v: adapter.ActionWrite, want: "write"}, + {name: "ActionDelete", v: adapter.ActionDelete, want: "delete"}, + {name: "Action(9)", v: adapter.Action(9), want: "action(9)"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.v.String(); got != tt.want { + t.Errorf("String() = %q, want %q", got, tt.want) + } + if got, want := fmt.Sprintf("%q", tt.v), fmt.Sprintf("%q", tt.want); got != want { + t.Errorf("%%q = %s, want %s", got, want) + } + }) + } +} diff --git a/internal/adapter/adapter.go b/internal/adapter/adapter.go index 32f0731f..49e8751f 100644 --- a/internal/adapter/adapter.go +++ b/internal/adapter/adapter.go @@ -6,6 +6,7 @@ package adapter import ( "encoding/json" "errors" + "fmt" "io" "github.com/spxrogers/agentsync/internal/secrets" @@ -52,14 +53,41 @@ func (s Scope) String() string { } } -// FileOp describes one destination-side change. Action is "write" (the default; -// the empty string is treated as "write") or "delete" — see the Action field and -// DispatchOps, which both accept "" as write. render.Plan normalizes "" to -// "write" as it collects each adapter's ops, and render.Apply/PreviewApply -// re-normalize at intake before any pipeline guard runs (they are exported and -// accept a caller-built RenderPlan that never went through Plan), so the -// pipeline guards always see the literal "write"; only code reading raw -// adapter Render output (or state-derived ops) must still accept "" as write. +// Action says what a FileOp does to its destination. The zero value IS +// ActionWrite — a FileOp built without naming an action writes — which is why +// there is nothing to normalize at any intake: no empty spelling exists that +// an executor would write while a guard matching "write" let it through. +// Unlike SkipKind below, whose zero value is invalid by design so an unstamped +// Skip fails TestEverySkipLiteralSetsKind, the zero value of Action is +// deliberately valid: the common case must need no stamp. +type Action int + +const ( + // ActionWrite writes Content to Path — whole-file, or key-merged per + // MergeStrategy. It is the zero value. + ActionWrite Action = iota + // ActionDelete removes Path. No adapter renders one and none enters a + // plan's Ops: deletes are synthesized at apply time (orphan reclamation), + // by `agent disable --purge`, and as the drift walk's orphan item. + ActionDelete +) + +// String renders the action for the dry-run op label and DispatchOps' error +// text. %q on an Action quotes this, so an out-of-range value reads +// "action()" rather than a bare integer. +func (a Action) String() string { + switch a { + case ActionWrite: + return "write" + case ActionDelete: + return "delete" + default: + return fmt.Sprintf("action(%d)", int(a)) + } +} + +// FileOp describes one destination-side change. Action says what happens to +// the destination: write (the zero value) or delete. // Path is absolute (after AGENTSYNC_TARGET_ROOT redirection). // // CONTRACT — Content is ALWAYS JSON for a key-merge op, regardless of the @@ -80,13 +108,19 @@ func (s Scope) String() string { // format-specific merge). A new TOML/YAML-backed agent must keep Content JSON, // not emit the on-disk format here. type FileOp struct { - Action string // "" | "write" | "delete" ("" == "write"; render.Plan and render.Apply/PreviewApply rewrite "" → "write" at intake) + Action Action // write (zero value) | delete Path string Content []byte Mode uint32 - SourceID string // canonical source path that produced this op - MergeStrategy string // "replace" (default) | "merge-json-keys" | "merge-jsonc-keys" | "merge-toml-keys" - OwnedKeys []string // JSON pointers owned by agentsync; populated by Apply from state, not Render + SourceID string // canonical source path that produced this op + MergeStrategy string // "replace" (default) | "merge-json-keys" | "merge-jsonc-keys" | "merge-toml-keys" + // OwnedKeys lists the JSON pointers agentsync owns at this key-merge + // destination, so the merge can remove any the op no longer carries. + // render.Plan populates it from state — scoped to the top-level sections + // this op writes — whenever Plan is given a state (plugin explain/poll pass + // nil). A value an adapter's Render sets is only a fallback for Apply driven + // without the pipeline; Plan overwrites it whenever it has state. + OwnedKeys []string } // SkipKind classifies how much of a component was lost, so consumers never have diff --git a/internal/adapter/claude/apply_test.go b/internal/adapter/claude/apply_test.go index 35cbbc13..9cc9364c 100644 --- a/internal/adapter/claude/apply_test.go +++ b/internal/adapter/claude/apply_test.go @@ -16,7 +16,7 @@ func TestApply_NewSettings_WritesContent(t *testing.T) { a := claude.New(claude.Options{TargetRoot: tmp}) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(tmp, ".claude.json"), Content: []byte(`{"mcpServers":{"github":{"command":"npx"}}}`), Mode: 0o644, @@ -40,7 +40,7 @@ func TestApply_PreservesForeignKeys(t *testing.T) { _ = os.WriteFile(target, []byte(`{"foreign":{"x":1},"mcpServers":{"old":{}}}`), 0o644) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: target, Content: []byte(`{"mcpServers":{"new":{"command":"x"}}}`), Mode: 0o644, @@ -72,7 +72,7 @@ func TestApply_OrphanRemoval(t *testing.T) { _ = os.WriteFile(target, []byte(`{"mcpServers":{"github":{"command":"old"},"stale":{}}}`), 0o644) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: target, Content: []byte(`{"mcpServers":{"github":{"command":"new"}}}`), Mode: 0o644, diff --git a/internal/adapter/claude/command.go b/internal/adapter/claude/command.go index 1bc84def..c4ecfccd 100644 --- a/internal/adapter/claude/command.go +++ b/internal/adapter/claude/command.go @@ -16,7 +16,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, return nil, fmt.Errorf("encode command %s: %w", cmd.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.CommandsDir, cmd.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/claude/hook.go b/internal/adapter/claude/hook.go index 513dd8ce..b302b4f6 100644 --- a/internal/adapter/claude/hook.go +++ b/internal/adapter/claude/hook.go @@ -67,7 +67,7 @@ func (a *Adapter) renderHooks(c source.Canonical, p Paths) ([]adapter.FileOp, [] return nil, nil, fmt.Errorf("marshal hooks: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Settings, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/claude/largeint_test.go b/internal/adapter/claude/largeint_test.go index 0cb93daa..9e1309d7 100644 --- a/internal/adapter/claude/largeint_test.go +++ b/internal/adapter/claude/largeint_test.go @@ -22,7 +22,7 @@ func TestApply_PreservesForeignLargeInt(t *testing.T) { t.Fatal(err) } op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, MergeStrategy: "merge-json-keys", Content: []byte(`{"mcpServers":{"github":{"command":"npx"}}}`), diff --git a/internal/adapter/claude/memory.go b/internal/adapter/claude/memory.go index 1d49690e..2d499c76 100644 --- a/internal/adapter/claude/memory.go +++ b/internal/adapter/claude/memory.go @@ -13,7 +13,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, e } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, filepath.Base(p.Memory), c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Memory, Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/claude/render.go b/internal/adapter/claude/render.go index f0644a99..3b4aee38 100644 --- a/internal/adapter/claude/render.go +++ b/internal/adapter/claude/render.go @@ -161,7 +161,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths, scope adapter.Scope) ([ } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/claude/skill.go b/internal/adapter/claude/skill.go index 3433e495..eec0eb74 100644 --- a/internal/adapter/claude/skill.go +++ b/internal/adapter/claude/skill.go @@ -27,7 +27,7 @@ func SkillFileOps(skills []source.Skill, skillsDir string) ([]adapter.FileOp, er return nil, fmt.Errorf("encode skill %s: %w", s.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(skillsDir, s.Name, "SKILL.md"), Content: body, Mode: 0o644, @@ -40,7 +40,7 @@ func SkillFileOps(skills []source.Skill, skillsDir string) ([]adapter.FileOp, er mode = 0o644 } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(skillsDir, s.Name, filepath.FromSlash(f.Path)), Content: f.Content, Mode: mode, diff --git a/internal/adapter/claude/subagent.go b/internal/adapter/claude/subagent.go index eb7f86a5..7bffb7dd 100644 --- a/internal/adapter/claude/subagent.go +++ b/internal/adapter/claude/subagent.go @@ -16,7 +16,7 @@ func (a *Adapter) renderSubagents(c source.Canonical, p Paths) ([]adapter.FileOp return nil, fmt.Errorf("encode subagent %s: %w", s.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.AgentsDir, s.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/cline/command.go b/internal/adapter/cline/command.go index bb34634e..b1203c26 100644 --- a/internal/adapter/cline/command.go +++ b/internal/adapter/cline/command.go @@ -77,7 +77,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, }) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.WorkflowsDir, cmd.Name+".md"), Content: []byte(wrapManagedWorkflow(cmd.Body)), Mode: 0o644, diff --git a/internal/adapter/cline/mcp.go b/internal/adapter/cline/mcp.go index 9960c79b..ba8bcbc5 100644 --- a/internal/adapter/cline/mcp.go +++ b/internal/adapter/cline/mcp.go @@ -56,7 +56,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, []ad return nil, nil, fmt.Errorf("marshal cline mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.MCP, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/cline/memory.go b/internal/adapter/cline/memory.go index 70fa3788..22471c2f 100644 --- a/internal/adapter/cline/memory.go +++ b/internal/adapter/cline/memory.go @@ -27,7 +27,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, [ } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, memoryRuleFile, c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.RulesDir, memoryRuleFile), Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/codex/command.go b/internal/adapter/codex/command.go index cf3677f1..1f0b9dc8 100644 --- a/internal/adapter/codex/command.go +++ b/internal/adapter/codex/command.go @@ -35,7 +35,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths, scope adapter.Scop return nil, nil, fmt.Errorf("encode command %s: %w", cmd.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.PromptsDir, cmd.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/codex/hook.go b/internal/adapter/codex/hook.go index faada3cf..aee8d176 100644 --- a/internal/adapter/codex/hook.go +++ b/internal/adapter/codex/hook.go @@ -94,7 +94,7 @@ func (a *Adapter) renderHooks(c source.Canonical, p Paths) ([]adapter.FileOp, [] // hook orphan-cleanup path is exercised directly in settings_test.go // (TestMergeTOML_RemovesOrphanedHookKey). See adapter.FileOp.OwnedKeys. return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Config, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/codex/mcp.go b/internal/adapter/codex/mcp.go index 3205fd92..11b7230d 100644 --- a/internal/adapter/codex/mcp.go +++ b/internal/adapter/codex/mcp.go @@ -39,7 +39,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, []ad return nil, nil, fmt.Errorf("marshal codex mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Config, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/codex/memory.go b/internal/adapter/codex/memory.go index 80781440..f5429a52 100644 --- a/internal/adapter/codex/memory.go +++ b/internal/adapter/codex/memory.go @@ -14,7 +14,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, e } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, filepath.Base(p.Memory), c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Memory, Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/codex/subagent.go b/internal/adapter/codex/subagent.go index 37611690..80ca2f45 100644 --- a/internal/adapter/codex/subagent.go +++ b/internal/adapter/codex/subagent.go @@ -104,7 +104,7 @@ func (a *Adapter) renderSubagents(c source.Canonical, p Paths) ([]adapter.FileOp return nil, nil, fmt.Errorf("marshal subagent %s: %w", s.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.AgentsDir, s.Name+".toml"), Content: body, Mode: 0o644, diff --git a/internal/adapter/continuedev/command.go b/internal/adapter/continuedev/command.go index ed4cc818..3fe9393b 100644 --- a/internal/adapter/continuedev/command.go +++ b/internal/adapter/continuedev/command.go @@ -46,7 +46,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, return nil, nil, fmt.Errorf("encode command %s: %w", cmd.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.PromptsDir, cmd.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/continuedev/mcp.go b/internal/adapter/continuedev/mcp.go index 107bfd05..ad465120 100644 --- a/internal/adapter/continuedev/mcp.go +++ b/internal/adapter/continuedev/mcp.go @@ -105,7 +105,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, []ad return nil, nil, fmt.Errorf("marshal continue mcp %s: %w", m.ID, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.MCPDir, m.ID+".yaml"), Content: body, Mode: 0o644, diff --git a/internal/adapter/continuedev/memory.go b/internal/adapter/continuedev/memory.go index a62f070d..00d94bda 100644 --- a/internal/adapter/continuedev/memory.go +++ b/internal/adapter/continuedev/memory.go @@ -19,7 +19,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, e } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, memoryRuleFile, c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.RulesDir, memoryRuleFile), Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/cursor/apply_test.go b/internal/adapter/cursor/apply_test.go index efd96ce3..f2753e80 100644 --- a/internal/adapter/cursor/apply_test.go +++ b/internal/adapter/cursor/apply_test.go @@ -219,7 +219,7 @@ func TestApply_Hooks_OrphanCleanup_PreservesVersionAndForeign(t *testing.T) { // agentsync previously rendered (/hooks/preToolUse). merge-json-keys then // strips that owned-but-now-absent key while leaving foreign keys alone. op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: hooksPath, Content: []byte("{}\n"), Mode: 0o644, diff --git a/internal/adapter/cursor/command.go b/internal/adapter/cursor/command.go index 5c736d12..0ac7e99b 100644 --- a/internal/adapter/cursor/command.go +++ b/internal/adapter/cursor/command.go @@ -29,7 +29,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, }) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.CommandsDir, cmd.Name+".md"), Content: []byte(cmd.Body), Mode: 0o644, diff --git a/internal/adapter/cursor/hook.go b/internal/adapter/cursor/hook.go index 9a797263..30355f81 100644 --- a/internal/adapter/cursor/hook.go +++ b/internal/adapter/cursor/hook.go @@ -114,7 +114,7 @@ func (a *Adapter) renderHooks(c source.Canonical, p Paths) ([]adapter.FileOp, [] return nil, nil, fmt.Errorf("marshal hooks: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Hooks, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/cursor/mcp.go b/internal/adapter/cursor/mcp.go index 10503b8c..688ba07b 100644 --- a/internal/adapter/cursor/mcp.go +++ b/internal/adapter/cursor/mcp.go @@ -64,7 +64,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, erro return nil, fmt.Errorf("marshal cursor mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.MCP, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/cursor/memory.go b/internal/adapter/cursor/memory.go index 05a83618..1dd925b5 100644 --- a/internal/adapter/cursor/memory.go +++ b/internal/adapter/cursor/memory.go @@ -25,7 +25,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths, scope adapter.Scope) } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, filepath.Base(p.Memory), c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Memory, Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/cursor/subagent.go b/internal/adapter/cursor/subagent.go index 9c02ea84..2a61db64 100644 --- a/internal/adapter/cursor/subagent.go +++ b/internal/adapter/cursor/subagent.go @@ -52,7 +52,7 @@ func (a *Adapter) renderSubagents(c source.Canonical, p Paths) ([]adapter.FileOp return nil, nil, fmt.Errorf("encode subagent %s: %w", s.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.AgentsDir, s.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/dispatch.go b/internal/adapter/dispatch.go index a3585184..8b8a3187 100644 --- a/internal/adapter/dispatch.go +++ b/internal/adapter/dispatch.go @@ -3,11 +3,11 @@ package adapter import "fmt" // DispatchOps runs the standard write/delete dispatch over ops: it calls -// w.Delete for each "delete" action and writeOp for each "" / "write" action -// (the empty string is treated as "write"), and returns -// fmt.Errorf("unknown action %q", op.Action) for anything else. A delete error -// is wrapped as fmt.Errorf("delete %s: %w", op.Path, err); a writeOp error is -// returned as-is (writeOp owns its own wrapping). +// w.Delete for each ActionDelete op and writeOp for each ActionWrite op (the +// zero value), and returns fmt.Errorf("unknown action %q", op.Action) for any +// out-of-range value. A delete error is wrapped as +// fmt.Errorf("delete %s: %w", op.Path, err); a writeOp error is returned as-is +// (writeOp owns its own wrapping). // // Every adapter's Apply (except noop's trivial no-op) delegates here so this // dispatch — and its exact error strings — lives in exactly one place. The only @@ -19,11 +19,11 @@ import "fmt" func DispatchOps(ops []FileOp, w DestWriter, writeOp func(FileOp) error) error { for _, op := range ops { switch op.Action { - case "delete": + case ActionDelete: if err := w.Delete(op); err != nil { return fmt.Errorf("delete %s: %w", op.Path, err) } - case "", "write": + case ActionWrite: if err := writeOp(op); err != nil { return err } diff --git a/internal/adapter/dispatch_test.go b/internal/adapter/dispatch_test.go index 6cd86f69..9d388ec7 100644 --- a/internal/adapter/dispatch_test.go +++ b/internal/adapter/dispatch_test.go @@ -38,34 +38,36 @@ func TestDispatchOps(t *testing.T) { }{ { name: "write action calls the write closure", - ops: []adapter.FileOp{{Action: "write", Path: "a"}}, + ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: "a"}}, wantWrites: []string{"a"}, }, { - name: "empty action is treated as write", - ops: []adapter.FileOp{{Action: "", Path: "b"}}, + // Built without an Action on purpose: this pins that the zero value + // writes (do not "tidy" it to an explicit ActionWrite). + name: "zero-value action is treated as write", + ops: []adapter.FileOp{{Path: "b"}}, wantWrites: []string{"b"}, }, { name: "delete action calls w.Delete", - ops: []adapter.FileOp{{Action: "delete", Path: "c"}}, + ops: []adapter.FileOp{{Action: adapter.ActionDelete, Path: "c"}}, wantDelete: []string{"c"}, }, { - name: "unknown action errors", - ops: []adapter.FileOp{{Action: "frob", Path: "d"}}, - wantErr: `unknown action "frob"`, + name: "out-of-range action errors", + ops: []adapter.FileOp{{Action: adapter.Action(9), Path: "d"}}, + wantErr: `unknown action "action(9)"`, }, { name: "delete error is wrapped with path", - ops: []adapter.FileOp{{Action: "delete", Path: "e"}}, + ops: []adapter.FileOp{{Action: adapter.ActionDelete, Path: "e"}}, delErr: errors.New("boom"), wantDelete: []string{"e"}, wantErr: "delete e: boom", }, { name: "write closure error propagates verbatim", - ops: []adapter.FileOp{{Action: "write", Path: "f"}}, + ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: "f"}}, writeErr: errors.New("write-boom"), wantWrites: []string{"f"}, wantErr: "write-boom", @@ -73,9 +75,9 @@ func TestDispatchOps(t *testing.T) { { name: "mixed sequence in order", ops: []adapter.FileOp{ - {Action: "write", Path: "w1"}, - {Action: "delete", Path: "d1"}, - {Action: "", Path: "w2"}, + {Action: adapter.ActionWrite, Path: "w1"}, + {Action: adapter.ActionDelete, Path: "d1"}, + {Path: "w2"}, // zero-value Action on purpose: the zero value writes }, wantWrites: []string{"w1", "w2"}, wantDelete: []string{"d1"}, diff --git a/internal/adapter/gemini/command.go b/internal/adapter/gemini/command.go index 3cd01ff3..cdffb534 100644 --- a/internal/adapter/gemini/command.go +++ b/internal/adapter/gemini/command.go @@ -57,7 +57,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, return nil, nil, fmt.Errorf("marshal command %s: %w", cmd.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.CommandsDir, filepath.FromSlash(cmd.Name)+".toml"), Content: body, Mode: 0o644, diff --git a/internal/adapter/gemini/hook.go b/internal/adapter/gemini/hook.go index 16f0002c..0bb9739a 100644 --- a/internal/adapter/gemini/hook.go +++ b/internal/adapter/gemini/hook.go @@ -153,7 +153,7 @@ func (a *Adapter) renderHooks(c source.Canonical, p Paths) ([]adapter.FileOp, [] return nil, nil, fmt.Errorf("marshal hooks: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Settings, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/gemini/mcp.go b/internal/adapter/gemini/mcp.go index 3c8157a7..e6e7562d 100644 --- a/internal/adapter/gemini/mcp.go +++ b/internal/adapter/gemini/mcp.go @@ -52,7 +52,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, []ad return nil, nil, fmt.Errorf("marshal gemini mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Settings, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/gemini/memory.go b/internal/adapter/gemini/memory.go index e5d81582..57e9e4c9 100644 --- a/internal/adapter/gemini/memory.go +++ b/internal/adapter/gemini/memory.go @@ -17,7 +17,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, e } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, filepath.Base(p.Memory), c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Memory, Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/gemini/subagent.go b/internal/adapter/gemini/subagent.go index 2e92e27d..254d6258 100644 --- a/internal/adapter/gemini/subagent.go +++ b/internal/adapter/gemini/subagent.go @@ -88,7 +88,7 @@ func (a *Adapter) renderSubagents(c source.Canonical, p Paths) ([]adapter.FileOp return nil, nil, fmt.Errorf("encode subagent %s: %w", s.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.AgentsDir, s.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/generic/render.go b/internal/adapter/generic/render.go index 83c33118..8c23aa82 100644 --- a/internal/adapter/generic/render.go +++ b/internal/adapter/generic/render.go @@ -37,7 +37,7 @@ func (a *Adapter) Render(r secrets.Resolved, scope adapter.Scope, project string if memPath := a.memoryPath(scope, project); memPath != "" { body := source.RenderManagedMemory(renderC.Memory.Body, renderC.Memory.Fragments, filepath.Base(memPath), renderC.Config.MemoryBannerEnabled()) ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: memPath, Content: []byte(body), Mode: 0o644, @@ -141,7 +141,7 @@ func (a *Adapter) renderMCP(c source.Canonical, scope adapter.Scope, project str return nil, nil, fmt.Errorf("marshal %s mcp: %w", a.spec.Name, err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: mcpPath, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/noop/noop_test.go b/internal/adapter/noop/noop_test.go index bad3f707..52a26bee 100644 --- a/internal/adapter/noop/noop_test.go +++ b/internal/adapter/noop/noop_test.go @@ -101,8 +101,8 @@ func (s *spyWriter) Delete(adapter.FileOp) error { s.deletes++; return ni func TestNoop_ApplyIgnoresOps(t *testing.T) { spy := &spyWriter{} ops := []adapter.FileOp{ - {Action: "write", Path: "/tmp/should-not-write", Content: []byte("nope"), Mode: 0o644}, - {Action: "delete", Path: "/tmp/should-not-delete"}, + {Action: adapter.ActionWrite, Path: "/tmp/should-not-write", Content: []byte("nope"), Mode: 0o644}, + {Action: adapter.ActionDelete, Path: "/tmp/should-not-delete"}, } if err := noop.New("test").Apply(ops, spy); err != nil { t.Fatalf("Apply: %v", err) diff --git a/internal/adapter/opencode/apply_test.go b/internal/adapter/opencode/apply_test.go index 7d963ec7..a72be451 100644 --- a/internal/adapter/opencode/apply_test.go +++ b/internal/adapter/opencode/apply_test.go @@ -18,7 +18,7 @@ func TestApply_WritesNewFile(t *testing.T) { _ = os.MkdirAll(filepath.Dir(path), 0o755) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: path, Content: []byte(`{"mcp":{"github":{"command":"npx"}}}`), Mode: 0o644, @@ -47,7 +47,7 @@ func TestApply_JSONC_PreservesForeignKeysAndComments(t *testing.T) { _ = os.WriteFile(path, []byte(existing), 0o644) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: path, Content: []byte(`{"mcp":{"github":{"command":"npx"}}}`), Mode: 0o644, @@ -79,7 +79,7 @@ func TestApply_JSONC_OrphanRemoval(t *testing.T) { _ = os.WriteFile(path, []byte(`{"mcp":{"github":{},"stale":{}}}`), 0o644) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: path, Content: []byte(`{"mcp":{"github":{"command":"npx"}}}`), Mode: 0o644, @@ -107,7 +107,7 @@ func TestApply_Delete_RemovesFile(t *testing.T) { path := filepath.Join(tmp, "todelete.txt") _ = os.WriteFile(path, []byte("bye"), 0o644) - op := adapter.FileOp{Action: "delete", Path: path} + op := adapter.FileOp{Action: adapter.ActionDelete, Path: path} if err := a.Apply([]adapter.FileOp{op}, adapter.PassThroughWriter{}); err != nil { t.Fatal(err) } @@ -119,7 +119,7 @@ func TestApply_Delete_RemovesFile(t *testing.T) { func TestApply_Delete_MissingFileNoError(t *testing.T) { tmp := t.TempDir() a := opencode.New(opencode.Options{TargetRoot: tmp}) - op := adapter.FileOp{Action: "delete", Path: filepath.Join(tmp, "nonexistent.txt")} + op := adapter.FileOp{Action: adapter.ActionDelete, Path: filepath.Join(tmp, "nonexistent.txt")} if err := a.Apply([]adapter.FileOp{op}, adapter.PassThroughWriter{}); err != nil { t.Fatalf("delete missing file should not error: %v", err) } diff --git a/internal/adapter/opencode/command.go b/internal/adapter/opencode/command.go index 6d9e7afa..46fda6d4 100644 --- a/internal/adapter/opencode/command.go +++ b/internal/adapter/opencode/command.go @@ -68,7 +68,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, return nil, nil, fmt.Errorf("encode opencode command %s: %w", cmd.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.CommandsDir, cmd.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/opencode/ingest_test.go b/internal/adapter/opencode/ingest_test.go index 6c3a2433..fead2387 100644 --- a/internal/adapter/opencode/ingest_test.go +++ b/internal/adapter/opencode/ingest_test.go @@ -43,7 +43,7 @@ func ownFiles(t *testing.T, targetRoot string, scope adapter.Scope, project stri t.Helper() ops := make([]adapter.FileOp, 0, len(files)) for _, f := range files { - ops = append(ops, adapter.FileOp{Action: "write", Path: f, Mode: 0o644}) + ops = append(ops, adapter.FileOp{Action: adapter.ActionWrite, Path: f, Mode: 0o644}) } seedOwnedState(t, targetRoot, scope, project, ops) } diff --git a/internal/adapter/opencode/memory.go b/internal/adapter/opencode/memory.go index c20183f2..101910f3 100644 --- a/internal/adapter/opencode/memory.go +++ b/internal/adapter/opencode/memory.go @@ -13,7 +13,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, e } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, filepath.Base(p.Memory), c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Memory, Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/opencode/render.go b/internal/adapter/opencode/render.go index 3e3cdfe3..7897e68f 100644 --- a/internal/adapter/opencode/render.go +++ b/internal/adapter/opencode/render.go @@ -94,7 +94,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, erro return nil, fmt.Errorf("marshal opencode mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.Settings, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/opencode/subagent.go b/internal/adapter/opencode/subagent.go index 41af1a50..db29a806 100644 --- a/internal/adapter/opencode/subagent.go +++ b/internal/adapter/opencode/subagent.go @@ -113,7 +113,7 @@ func (a *Adapter) renderSubagents(c source.Canonical, p Paths) ([]adapter.FileOp return nil, nil, fmt.Errorf("encode opencode subagent %s: %w", s.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.AgentsDir, s.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/roo/command.go b/internal/adapter/roo/command.go index 07ae1709..d44ab10c 100644 --- a/internal/adapter/roo/command.go +++ b/internal/adapter/roo/command.go @@ -46,7 +46,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, return nil, nil, fmt.Errorf("encode command %s: %w", cmd.Name, err) } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.CommandsDir, cmd.Name+".md"), Content: body, Mode: 0o644, diff --git a/internal/adapter/roo/mcp.go b/internal/adapter/roo/mcp.go index 19b43209..d57140ef 100644 --- a/internal/adapter/roo/mcp.go +++ b/internal/adapter/roo/mcp.go @@ -53,7 +53,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, []ad return nil, nil, fmt.Errorf("marshal roo mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.MCP, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/roo/memory.go b/internal/adapter/roo/memory.go index aa31ff55..e6cc6f4a 100644 --- a/internal/adapter/roo/memory.go +++ b/internal/adapter/roo/memory.go @@ -17,7 +17,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, e } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, memoryRuleFile, c.Config.MemoryBannerEnabled()) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.RulesDir, memoryRuleFile), Content: []byte(body), Mode: 0o644, diff --git a/internal/adapter/windsurf/command.go b/internal/adapter/windsurf/command.go index 3865af36..96292513 100644 --- a/internal/adapter/windsurf/command.go +++ b/internal/adapter/windsurf/command.go @@ -46,7 +46,7 @@ func (a *Adapter) renderCommands(c source.Canonical, p Paths) ([]adapter.FileOp, // it neither truncates nor flags an oversized workflow, mirroring the // memory handling (documented in the capability matrix). ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.WorkflowsDir, cmd.Name+".md"), Content: []byte(cmd.Body), Mode: 0o644, diff --git a/internal/adapter/windsurf/mcp.go b/internal/adapter/windsurf/mcp.go index 747314f8..425fe0bf 100644 --- a/internal/adapter/windsurf/mcp.go +++ b/internal/adapter/windsurf/mcp.go @@ -53,7 +53,7 @@ func (a *Adapter) renderMCP(c source.Canonical, p Paths) ([]adapter.FileOp, []ad return nil, nil, fmt.Errorf("marshal windsurf mcp: %w", err) } return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.MCP, Content: append(body, '\n'), Mode: 0o644, diff --git a/internal/adapter/windsurf/memory.go b/internal/adapter/windsurf/memory.go index 83a6afc3..f8a731dd 100644 --- a/internal/adapter/windsurf/memory.go +++ b/internal/adapter/windsurf/memory.go @@ -48,7 +48,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, [ // truncates nor flags an oversized rule (documented in the capability matrix). body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, memoryRuleFile, banner) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: filepath.Join(p.RulesDir, memoryRuleFile), Content: []byte(memoryRuleFrontmatter + body), Mode: 0o644, @@ -66,7 +66,7 @@ func (a *Adapter) renderMemory(c source.Canonical, p Paths) ([]adapter.FileOp, [ } body := source.RenderManagedMemory(c.Memory.Body, c.Memory.Fragments, filepath.Base(p.GlobalRules), banner) return []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p.GlobalRules, Content: []byte(body), Mode: 0o644, diff --git a/internal/cli/agent.go b/internal/cli/agent.go index ee712766..ce178ea0 100644 --- a/internal/cli/agent.go +++ b/internal/cli/agent.go @@ -700,7 +700,7 @@ func purgeAgentDests(cmd *cobra.Command, name, home string, sc adapter.Scope, pr sharedKept++ continue } - ops = append(ops, adapter.FileOp{Action: "delete", Path: paths.FromHomeRelative(userHome, p)}) + ops = append(ops, adapter.FileOp{Action: adapter.ActionDelete, Path: paths.FromHomeRelative(userHome, p)}) deletedFiles++ } // Pointer prunes for key-owned dests: an empty merge op carrying only @@ -713,7 +713,7 @@ func purgeAgentDests(cmd *cobra.Command, name, home string, sc adapter.Scope, pr continue // already gone; nothing to prune } ops = append(ops, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: abs, Content: []byte("{}"), Mode: 0o644, diff --git a/internal/cli/apply.go b/internal/cli/apply.go index d7e3f6df..3803c94d 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -353,17 +353,14 @@ func printPlannedOp(w io.Writer, p *ui.Printer, op adapter.FileOp, wouldChange m fmt.Fprintf(w, " %s %s %s\n", p.Green(ui.GlyphOK), p.Green(ui.Pad("synced", 6)), dispPath) return } - // Plan ops never carry the "" Action spelling (Plan normalizes it to - // "write" at intake), so op.Action can be printed directly. - fmt.Fprintf(w, " %s %s %s\n", p.Cyan(ui.GlyphArrow), p.Cyan(ui.Pad(op.Action, 6)), dispPath) + fmt.Fprintf(w, " %s %s %s\n", p.Cyan(ui.GlyphArrow), p.Cyan(ui.Pad(op.Action.String(), 6)), dispPath) } // isSyncedOp reports whether a planned op is a write the destination already // satisfies — i.e. a real apply would skip it. Delete ops, and any write whose -// destination would be created or modified, are never "synced". Operates on -// plan ops, so Action is never "" (Plan normalizes it to "write" at intake). +// destination would be created or modified, are never "synced". func isSyncedOp(op adapter.FileOp, wouldChange map[string]bool) bool { - if op.Action != "write" { + if op.Action != adapter.ActionWrite { return false } return !wouldChange[op.Path] diff --git a/internal/cli/apply_internal_test.go b/internal/cli/apply_internal_test.go index dda98035..ffc75d6a 100644 --- a/internal/cli/apply_internal_test.go +++ b/internal/cli/apply_internal_test.go @@ -34,8 +34,8 @@ func TestSaveBestEffortState_OnlyRecordsWrittenPaths(t *testing.T) { plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: writtenPath, Content: []byte(`{"a":1}`)}, - {Action: "write", Path: foreignPath, Content: []byte(`{"a":2}`)}, + {Action: adapter.ActionWrite, Path: writtenPath, Content: []byte(`{"a":1}`)}, + {Action: adapter.ActionWrite, Path: foreignPath, Content: []byte(`{"a":2}`)}, }}, }} diff --git a/internal/cli/import.go b/internal/cli/import.go index 7220a97d..d66da3c4 100644 --- a/internal/cli/import.go +++ b/internal/cli/import.go @@ -704,9 +704,7 @@ func seedStateFromCurrentDest(agentsyncHome, srcHome, agentName string, reg *ada now := time.Now().UTC() for _, op := range ops { - // These ops are RAW adapter Render output (not plan-normalized), so "" - // must still be accepted as the documented "write" default. - if op.Action != "" && op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } switch { diff --git a/internal/cli/iss162_internal_test.go b/internal/cli/iss162_internal_test.go index 40ec495a..a9063830 100644 --- a/internal/cli/iss162_internal_test.go +++ b/internal/cli/iss162_internal_test.go @@ -102,7 +102,7 @@ func TestStatus_ModeDriftUsesOpModeNotRecordedMode(t *testing.T) { s := state.New() s.Files[stateFileKey(userHome, "claude", adapter.ScopeUser, "", p)] = state.FileEntry{SHA256: hashContent([]byte(content)), Mode: 0o644} plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{"claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: p, Content: []byte(content), Mode: 0o755, SourceID: "skills/x/run.sh"}, + {Action: adapter.ActionWrite, Path: p, Content: []byte(content), Mode: 0o755, SourceID: "skills/x/run.sh"}, }}}} model := buildStatusModel(plan, []string{"claude"}, s, userHome, adapter.ScopeUser, "") // Both halves, so a walk that emitted zero items cannot pass: exactly one diff --git a/internal/cli/iss227_internal_test.go b/internal/cli/iss227_internal_test.go index 0d27ff23..0a7e40c0 100644 --- a/internal/cli/iss227_internal_test.go +++ b/internal/cli/iss227_internal_test.go @@ -74,7 +74,7 @@ func TestStatus_OwnershipSurvivesLegacyStateMigration(t *testing.T) { plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: dest, Content: []byte(content), Mode: 0o644, SourceID: "memory/AGENTS.md"}, + {Action: adapter.ActionWrite, Path: dest, Content: []byte(content), Mode: 0o644, SourceID: "memory/AGENTS.md"}, }}, }} model := buildStatusModel(plan, []string{"claude"}, s, userHome, adapter.ScopeUser, "") diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index c3ae12b0..91846165 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -29,7 +29,7 @@ type planItem struct { agent string // op is the plan op that produced the item. For an ORPHAN it is SYNTHESIZED - // from state — adapter.FileOp{Action: "delete", Path, SourceID} — with Mode + // from state — adapter.FileOp{Action: adapter.ActionDelete, Path, SourceID} — with Mode // left 0 because the orphan removal path never reads it. op adapter.FileOp @@ -214,7 +214,7 @@ func walkPlanItems(w planWalk) []planItem { } seenPath := map[string]bool{} for _, op := range res.Ops { - if op.Action != "" && op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } if w.matchOp != nil && !w.matchOp(name, op) { @@ -280,7 +280,7 @@ func walkPlanItems(w planWalk) []planItem { // SourceID matters: the reclaimable-KIND check behind reconcile's // prompt wording is SourceID-keyed and silently degrades to // "unknown kind" without it. - op: adapter.FileOp{Action: "delete", Path: orphan, SourceID: entry.SourceID}, + op: adapter.FileOp{Action: adapter.ActionDelete, Path: orphan, SourceID: entry.SourceID}, happlied: entry.SHA256, hdest: hashFile(orphan), destPerm: perm, diff --git a/internal/cli/planwalk_characterization_test.go b/internal/cli/planwalk_characterization_test.go index 072bceca..350ce925 100644 --- a/internal/cli/planwalk_characterization_test.go +++ b/internal/cli/planwalk_characterization_test.go @@ -168,12 +168,12 @@ func ptrKey(userHome, agent, path, ptr string) state.Key { } func fileOp(path, content string) adapter.FileOp { - return adapter.FileOp{Action: "write", Path: path, Content: []byte(content), SourceID: "memory/AGENTS.md"} + return adapter.FileOp{Action: adapter.ActionWrite, Path: path, Content: []byte(content), SourceID: "memory/AGENTS.md"} } func keyOp(path, content string) adapter.FileOp { return adapter.FileOp{ - Action: "write", Path: path, Content: []byte(content), + Action: adapter.ActionWrite, Path: path, Content: []byte(content), MergeStrategy: "merge-json-keys", SourceID: "mcp/* (multiple)", } } diff --git a/internal/cli/planwalk_internal_test.go b/internal/cli/planwalk_internal_test.go index f71425ac..fc668087 100644 --- a/internal/cli/planwalk_internal_test.go +++ b/internal/cli/planwalk_internal_test.go @@ -145,7 +145,7 @@ func TestWalkPlanItems(t *testing.T) { t.Errorf("orphan placement: got %v want %v", got, want) } o := items[1] - if o.op.Action != "delete" || o.op.SourceID != "skills/x/SKILL.md" || o.op.Mode != 0 || + if o.op.Action != adapter.ActionDelete || o.op.SourceID != "skills/x/SKILL.md" || o.op.Mode != 0 || o.cls != drift.Orphan || o.hsrc != "" || o.happlied != hf("P") || o.hdest != hf("P") { t.Errorf("orphan item: %+v", o) } @@ -214,7 +214,7 @@ func TestWalkPlanItems(t *testing.T) { s := state.New() s.Files[fileKey(h, "claude", p)] = state.FileEntry{SHA256: hf("P")} del := fileOp(b, "B") - del.Action = "delete" + del.Action = adapter.ActionDelete plan := planFor(map[string][]adapter.FileOp{ "claude": {fileOp(a, "A"), fileOp(a, "A"), fileOp(b, "B"), del, keyOp(b, "{}")}, "opencode": {fileOp(b, "B")}, @@ -307,13 +307,15 @@ func TestWalkPlanItems(t *testing.T) { run: func(t *testing.T, h string) { a, b := dest(h, "a.md"), dest(h, "b.md") del := fileOp(b, "B") - del.Action = "delete" - empty := fileOp(a, "A") - empty.Action = "" - plan := planFor(map[string][]adapter.FileOp{"claude": {del, empty}}) + del.Action = adapter.ActionDelete + // Built without an Action on purpose: this pins that the zero + // value walks as a write (do not "tidy" it to ActionWrite). + zero := adapter.FileOp{Path: a, Content: []byte("A"), SourceID: "memory/AGENTS.md"} + plan := planFor(map[string][]adapter.FileOp{"claude": {del, zero}}) got := itemKeys(walkUser(h, plan, state.New(), []string{"claude"}, nil)) - // "" and "write" are the accepted spellings; anything else is - // skipped (matches explain and render.OrphanFiles). + // ActionWrite — including the zero value, which IS write — is + // walked; a delete is skipped (matches explain and + // render.OrphanFiles). if want := []string{"claude " + a}; !reflect.DeepEqual(got, want) { t.Errorf("got %v want %v", got, want) } diff --git a/internal/cli/reconcile.go b/internal/cli/reconcile.go index 4c260ee2..2c306ae4 100644 --- a/internal/cli/reconcile.go +++ b/internal/cli/reconcile.go @@ -630,9 +630,7 @@ func collectReconcileItems(plan render.RenderPlan, reg *adapter.Registry, s *sta continue } for _, op := range res.Ops { - // Plan ops never carry the "" Action spelling (Plan normalizes it - // to "write" at intake). - if op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } if render.IsKeyMerge(op.MergeStrategy) { diff --git a/internal/cli/traversal_guard_test.go b/internal/cli/traversal_guard_test.go index 4b7895ee..2fb7480f 100644 --- a/internal/cli/traversal_guard_test.go +++ b/internal/cli/traversal_guard_test.go @@ -93,7 +93,7 @@ func TestEveryAdapterRejectsTraversalComponentName(t *testing.T) { t.Fatalf("Plan(%s, legitimate, scope=%s) = %v; want nil (happy path unchanged)", name, sc.scope, err) } for _, op := range plan.PerAgent[name].Ops { - if op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } sawControlOp = true diff --git a/internal/render/iss162_test.go b/internal/render/iss162_test.go index d76e4f05..1bbcf80f 100644 --- a/internal/render/iss162_test.go +++ b/internal/render/iss162_test.go @@ -25,7 +25,7 @@ func TestWrite_ChmodReconverges(t *testing.T) { home := t.TempDir() dest := filepath.Join(t.TempDir(), "run.sh") content := []byte("#!/bin/sh\necho hi\n") - op := adapter.FileOp{Action: "write", Path: dest, Content: content, Mode: 0o755} + op := adapter.FileOp{Action: adapter.ActionWrite, Path: dest, Content: content, Mode: 0o755} st := state.New() // Initial write establishes content + 0755. @@ -106,7 +106,7 @@ func TestRecordOpsState_MergeTomlNumericNoFalseDrift(t *testing.T) { } st := state.New() op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: []byte(tc.ours), MergeStrategy: "merge-toml-keys", diff --git a/internal/render/pipeline.go b/internal/render/pipeline.go index 8b1ecc6e..8151c55e 100644 --- a/internal/render/pipeline.go +++ b/internal/render/pipeline.go @@ -124,18 +124,6 @@ func Plan(r secrets.Resolved, reg *adapter.Registry, agents []string, scope adap if err != nil { return out, fmt.Errorf("render %s: %w", name, err) } - // Normalize the documented `"" == "write"` Action default ONCE at the - // plan boundary, before any guard runs: executors (DispatchOps) treat - // "" as write, but the containment backstop below and the apply-time - // dedup/divergence check match the literal "write" — so an op emitted - // with the empty spelling would be written by every adapter while - // dodging both guards. After this loop (and Plan's own synthesized - // ops, which set Action explicitly), plan ops never carry "". - for i := range ops { - if ops[i].Action == "" { - ops[i].Action = "write" - } - } // Containment backstop (defense-in-depth): reject any write whose own cleaned // path still contains a ".." segment (an unrooted / relative-with-leading-".." // path). This is deliberately narrow — Plan does not know each adapter's dest @@ -147,7 +135,7 @@ func Plan(r secrets.Resolved, reg *adapter.Registry, agents []string, scope adap // derived from a filesystem walk via filepath.Rel with symlinks skipped, so a // ".." can never enter them — this remains the last conservative net. for _, op := range ops { - if op.Action == "write" && pathEscapes(op.Path) { + if op.Action == adapter.ActionWrite && pathEscapes(op.Path) { return out, fmt.Errorf("render %s: refusing FileOp path %q: escapes its destination directory via '..'", name, op.Path) } } @@ -340,7 +328,7 @@ func orphanCleanupOps(s *state.Targets, a adapter.Adapter, agent string, scope a continue } cleanup = append(cleanup, adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: abs, Content: []byte("{}"), Mode: 0o644, @@ -436,24 +424,13 @@ func applyPlan( if a == nil { return reports, written, unchanged, wouldChange, fmt.Errorf("adapter %q not registered at apply", name) } - // Intake normalization + containment backstop for caller-built plans. - // Apply and PreviewApply are exported and accept a RenderPlan that never - // went through Plan, so an op here can still carry the documented - // `"" == "write"` Action default — which every executor writes, while - // the traversal backstop and the dedup/divergence check below match the - // literal "write". Normalize at this shared entry (a Plan-built plan is - // already normalized, so this is a no-op for it) and re-run the - // backstop, so a hand-built plan cannot smuggle an unanchored ".." path - // past the guards Plan enforces. Scope honesty: "shared" covers the - // Apply/PreviewApply funnel only — cli/reconcile.go and cli/agent.go - // call an adapter's Apply directly with plan-derived or explicit-Action - // ops and stay outside this normalization; raw-adapter-output consumers - // keep their own `"" ==` guards. + // Containment backstop for caller-built plans. Apply and PreviewApply + // are exported and accept a RenderPlan that never went through Plan, so + // the traversal check runs again here: a hand-built plan cannot smuggle + // an unanchored ".." path past the guard Plan enforces. (A Plan-built + // plan already passed it, so this is a no-op for it.) for i := range res.Ops { - if res.Ops[i].Action == "" { - res.Ops[i].Action = "write" - } - if res.Ops[i].Action == "write" && pathEscapes(res.Ops[i].Path) { + if res.Ops[i].Action == adapter.ActionWrite && pathEscapes(res.Ops[i].Path) { return reports, written, unchanged, wouldChange, fmt.Errorf( "apply %s: refusing FileOp path %q: escapes its destination directory via '..'", name, res.Ops[i].Path, ) @@ -468,7 +445,7 @@ func applyPlan( // hooks, AND lspServers to settings.json), and each must run — // the adapter re-reads and merges per op. Deduping them by path // silently dropped every merge op after the first. - if op.Action == "write" && !IsKeyMerge(op.MergeStrategy) { + if op.Action == adapter.ActionWrite && !IsKeyMerge(op.MergeStrategy) { if prev, ok := seen[op.Path]; ok { // Identical content is the safe, expected dedup (claude and // opencode render byte-identical SKILL.md). Divergent content diff --git a/internal/render/pipeline_test.go b/internal/render/pipeline_test.go index 37444e34..6a64a3a6 100644 --- a/internal/render/pipeline_test.go +++ b/internal/render/pipeline_test.go @@ -64,7 +64,7 @@ func TestPipeline_UnknownAgentError(t *testing.T) { func TestPipeline_DedupesIdenticalWritesAcrossAdapters(t *testing.T) { sharedPath := "/tmp/fake-root/.claude/skills/my-skill/SKILL.md" sharedOp := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: sharedPath, Content: []byte("# My skill\n"), Mode: 0o644, @@ -223,7 +223,7 @@ func TestPlan_ContainmentBackstopRejectsEscapingFileOp(t *testing.T) { esc := &countingAdapter{ Adapter: noop.New("claude"), renderOps: []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: "../escape.md", // filepath.Clean keeps the leading ".." Content: []byte("x"), Mode: 0o644, @@ -242,20 +242,24 @@ func TestPlan_ContainmentBackstopRejectsEscapingFileOp(t *testing.T) { } } -// TestPlan_NormalizesEmptyActionBeforeGuards pins the "" == "write" boundary -// normalization: an adapter op emitted with the documented empty-Action default -// would be WRITTEN by every executor, yet the containment backstop and the -// apply-time dedup/divergence check match the literal "write" — so before Plan -// normalized "" at intake, an escaping op with Action "" slid past the backstop. -// The traversal path here must be refused exactly like the explicit-"write" -// case above; a second, benign "" op proves the plan output carries "write". -func TestPlan_NormalizesEmptyActionBeforeGuards(t *testing.T) { - t.Run("empty-Action traversal op is refused", func(t *testing.T) { +// TestPlan_ZeroActionIsWriteAtTheGuards pins that the containment backstop +// cannot be dodged by leaving Action unset. Before Action was typed, "" was a +// documented write default that every executor wrote while the backstop and +// the dedup/divergence check matched the literal "write" — so Plan rewrote "" +// at intake, and this test pinned the rewrite. The rewrite is gone because +// there is nothing left to rewrite: the zero value IS adapter.ActionWrite. The +// property that survives is the one that mattered — there is no spelling that +// writes while dodging a guard — so the ops here are built WITHOUT an Action on +// purpose (do not "tidy" them to an explicit ActionWrite: the point is that the +// zero value writes, and swapping the iota order so the zero value is +// ActionDelete must fail this test). +func TestPlan_ZeroActionIsWriteAtTheGuards(t *testing.T) { + t.Run("zero-Action traversal op is refused", func(t *testing.T) { reg := adapter.NewRegistry() esc := &countingAdapter{ Adapter: noop.New("claude"), renderOps: []adapter.FileOp{{ - Action: "", // documented write default — must not dodge the backstop + // No Action: the zero value writes — and must not dodge the backstop. Path: "../escape.md", Content: []byte("x"), Mode: 0o644, @@ -267,18 +271,18 @@ func TestPlan_NormalizesEmptyActionBeforeGuards(t *testing.T) { } _, err := render.Plan(secrets.ForRender(source.Canonical{}), reg, []string{"claude"}, adapter.ScopeUser, "", nil, "/tmp") if err == nil { - t.Fatal("Plan accepted an empty-Action FileOp whose path escapes via '..'; want rejection") + t.Fatal("Plan accepted a zero-Action FileOp whose path escapes via '..'; want rejection") } if !strings.Contains(err.Error(), "escapes its destination") { t.Errorf("error %q does not describe the containment failure", err.Error()) } }) - t.Run("empty Action is rewritten to write in the plan", func(t *testing.T) { + t.Run("zero Action reads back as ActionWrite in the plan", func(t *testing.T) { reg := adapter.NewRegistry() a := &countingAdapter{ Adapter: noop.New("claude"), renderOps: []adapter.FileOp{{ - Action: "", + // No Action on purpose: the zero value is the write. Path: "/tmp/dest/file.md", Content: []byte("x"), Mode: 0o644, @@ -293,29 +297,27 @@ func TestPlan_NormalizesEmptyActionBeforeGuards(t *testing.T) { t.Fatal(err) } ops := plan.PerAgent["claude"].Ops - if len(ops) != 1 || ops[0].Action != "write" { - t.Fatalf("Plan must rewrite Action \"\" to \"write\" at intake, got %+v", ops) + if len(ops) != 1 || ops[0].Action != adapter.ActionWrite { + t.Fatalf("a zero-Action op must be an ActionWrite in the plan, got %+v", ops) } }) } -// TestApply_NormalizesEmptyActionForCallerBuiltPlans pins the same "" == "write" -// intake normalization at the OTHER exported entries: Apply and PreviewApply -// accept a caller-built RenderPlan that never went through Plan, so before they -// re-normalized at intake, an op with the documented empty-Action default was -// written by every executor while dodging both the traversal containment -// backstop and the cross-agent divergence check (each matches the literal -// "write"). A hand-built plan carrying an escaping empty-Action op must be -// refused exactly like Plan refuses it, and a benign empty-Action op must reach -// the adapter already normalized to "write". -func TestApply_NormalizesEmptyActionForCallerBuiltPlans(t *testing.T) { +// TestApply_ZeroActionForCallerBuiltPlans pins the same property at the OTHER +// exported entries: Apply and PreviewApply accept a caller-built RenderPlan +// that never went through Plan and re-run the containment backstop there. The +// ops are built without an Action on purpose — the zero value writes — so a +// hand-built plan carrying an escaping zero-Action op must be refused exactly +// like Plan refuses it, and a benign zero-Action op must reach the adapter as +// an ActionWrite (there is no intake rewrite left to make it one). +func TestApply_ZeroActionForCallerBuiltPlans(t *testing.T) { planFor := func(op adapter.FileOp) render.RenderPlan { return render.RenderPlan{PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{op}}, }} } escaping := adapter.FileOp{ - Action: "", // documented write default — hand-built, never normalized by Plan + // No Action: the zero value writes — hand-built, never seen by Plan. Path: "../escape.md", Content: []byte("x"), Mode: 0o644, @@ -336,7 +338,7 @@ func TestApply_NormalizesEmptyActionForCallerBuiltPlans(t *testing.T) { }}, } for _, e := range entries { - t.Run(e.name+"/empty-Action traversal op is refused", func(t *testing.T) { + t.Run(e.name+"/zero-Action traversal op is refused", func(t *testing.T) { a := &countingAdapter{Adapter: noop.New("claude")} reg := adapter.NewRegistry() if err := reg.Register(a); err != nil { @@ -344,7 +346,7 @@ func TestApply_NormalizesEmptyActionForCallerBuiltPlans(t *testing.T) { } err := e.run(planFor(escaping), reg) if err == nil { - t.Fatalf("%s accepted an empty-Action FileOp whose path escapes via '..'; want refusal", e.name) + t.Fatalf("%s accepted a zero-Action FileOp whose path escapes via '..'; want refusal", e.name) } if !strings.Contains(err.Error(), "escapes its destination") { t.Errorf("error %q does not describe the containment failure", err.Error()) @@ -354,7 +356,7 @@ func TestApply_NormalizesEmptyActionForCallerBuiltPlans(t *testing.T) { } }) } - t.Run("benign empty Action reaches the adapter as write", func(t *testing.T) { + t.Run("benign zero Action reaches the adapter as ActionWrite", func(t *testing.T) { a := &countingAdapter{Adapter: noop.New("claude")} reg := adapter.NewRegistry() if err := reg.Register(a); err != nil { @@ -365,8 +367,8 @@ func TestApply_NormalizesEmptyActionForCallerBuiltPlans(t *testing.T) { if _, _, _, err := render.Apply(planFor(benign), reg, state.New(), t.TempDir(), t.TempDir(), adapter.ScopeUser, ""); err != nil { t.Fatal(err) } - if len(a.ops) != 1 || a.ops[0].Action != "write" { - t.Fatalf("Apply must rewrite Action \"\" to \"write\" at intake, got %+v", a.ops) + if len(a.ops) != 1 || a.ops[0].Action != adapter.ActionWrite { + t.Fatalf("a zero-Action op must reach the adapter as ActionWrite, got %+v", a.ops) } }) } @@ -377,7 +379,7 @@ func TestPipeline_OwnedKeysInjected(t *testing.T) { home := "/tmp/fake-root" destPath := "/tmp/fake-root/.claude.json" mergeOp := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: destPath, Content: []byte(`{"mcpServers":{"github":{}}}`), Mode: 0o644, diff --git a/internal/render/report_test.go b/internal/render/report_test.go index 1375c7f4..f71524f9 100644 --- a/internal/render/report_test.go +++ b/internal/render/report_test.go @@ -20,7 +20,7 @@ func TestBuildReport_NoPlugins(t *testing.T) { PerAgent: map[string]render.AgentResult{ "claude": { Ops: []adapter.FileOp{ - {Action: "write", Path: "/home/.claude.json", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, Path: "/home/.claude.json", MergeStrategy: "merge-json-keys"}, }, Skips: nil, }, @@ -55,13 +55,13 @@ func TestBuildReport_WithPlugin(t *testing.T) { PerAgent: map[string]render.AgentResult{ "claude": { Ops: []adapter.FileOp{ - {Action: "write", Path: "/home/.claude.json", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, Path: "/home/.claude.json", MergeStrategy: "merge-json-keys"}, }, Skips: nil, }, "opencode": { Ops: []adapter.FileOp{ - {Action: "write", Path: "/home/.config/opencode/opencode.json", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, Path: "/home/.config/opencode/opencode.json", MergeStrategy: "merge-json-keys"}, }, Skips: nil, }, @@ -116,7 +116,7 @@ func TestBuildReport_PartialCoverage(t *testing.T) { PerAgent: map[string]render.AgentResult{ "claude": { Ops: []adapter.FileOp{ - {Action: "write", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}, }, Skips: []adapter.Skip{ {Component: "hook", Name: "pre-run", Reason: "unsupported", Kind: adapter.SkipDropped}, @@ -152,7 +152,7 @@ func TestBuildReport_SkipDetails(t *testing.T) { plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ "codex": { - Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-toml-keys"}}, + Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-toml-keys"}}, Skips: skips, }, }, @@ -214,7 +214,7 @@ func TestBuildReport_SkipDetails_BaseBranch(t *testing.T) { plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ "codex": { - Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-toml-keys"}}, + Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-toml-keys"}}, Skips: []adapter.Skip{{Component: "lsp", Name: "gopls", Reason: "Codex has no LSP configuration concept", Kind: adapter.SkipDropped}}, }, }, @@ -245,7 +245,7 @@ func TestBuildReport_SkipDetails_OmittedWhenEmpty(t *testing.T) { } plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, // no skips + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, // no skips }, } report := render.BuildReport(c, plan, []string{"claude"}) @@ -274,12 +274,12 @@ func TestTranslationReport_PrintText(t *testing.T) { PerAgent: map[string]render.AgentResult{ "claude": { Ops: []adapter.FileOp{ - {Action: "write", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}, }, }, "opencode": { Ops: []adapter.FileOp{ - {Action: "write", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}, }, }, }, @@ -317,8 +317,8 @@ func TestTranslationReport_PrintTextStyled(t *testing.T) { } plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, - "opencode": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, + "opencode": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, }, } report := render.BuildReport(c, plan, []string{"claude", "opencode"}) @@ -365,7 +365,7 @@ func TestTranslationReport_SanitizesUntrustedPluginLabel(t *testing.T) { {ID: "evil", Plugin: source.PluginSpec{ID: evil, Version: "1.0.0", Disabled: disabled}}, }} plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, }} return render.BuildReport(c, plan, []string{"claude"}) } @@ -425,7 +425,7 @@ func TestTranslationReport_JSONKeepsUntrustedLabelRaw(t *testing.T) { {ID: "evil", Plugin: source.PluginSpec{ID: evil}}, }} plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, }} report := render.BuildReport(c, plan, []string{"claude"}) @@ -455,7 +455,7 @@ func TestTranslationReport_PrintJSON(t *testing.T) { plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ "claude": { - Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}, + Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}, }, }, } @@ -489,8 +489,8 @@ func TestBuildReport_CountsItemsNotOps(t *testing.T) { PerAgent: map[string]render.AgentResult{ "claude": { Ops: []adapter.FileOp{ - {Action: "write", Path: "/h/.claude.json", MergeStrategy: "merge-json-keys"}, - {Action: "write", Path: "/h/.claude/CLAUDE.md", MergeStrategy: "replace"}, + {Action: adapter.ActionWrite, Path: "/h/.claude.json", MergeStrategy: "merge-json-keys"}, + {Action: adapter.ActionWrite, Path: "/h/.claude/CLAUDE.md", MergeStrategy: "replace"}, }, }, }, @@ -521,7 +521,7 @@ func TestBuildReport_InventoryCountsAllKinds(t *testing.T) { } plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, }, } report := render.BuildReport(c, plan, []string{"claude"}) @@ -585,7 +585,7 @@ func TestBuildReport_CoveragePartialWhenSomethingRendered(t *testing.T) { plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ "codex": { - Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "replace"}}, // the skill rendered + Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "replace"}}, // the skill rendered Skips: []adapter.Skip{{Component: "hook", Name: "x", Reason: "unknown event", Kind: adapter.SkipDropped}}, }, }, @@ -608,7 +608,7 @@ func TestBuildReport_BaseCoverageFromRendered(t *testing.T) { c := source.Canonical{Skills: []source.Skill{{Name: "s"}}} // no Plugins → "(base)" rendered := render.RenderPlan{PerAgent: map[string]render.AgentResult{ "codex": { - Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "replace"}}, + Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "replace"}}, Skips: []adapter.Skip{{Component: "hook", Reason: "unknown event", Kind: adapter.SkipDropped}}, }, }} @@ -642,8 +642,8 @@ func TestBuildReport_CountsHonorTargeting(t *testing.T) { }, } plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-json-keys"}}}, - "codex": {Ops: []adapter.FileOp{{Action: "write", MergeStrategy: "merge-toml-keys"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-json-keys"}}}, + "codex": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, MergeStrategy: "merge-toml-keys"}}}, }} byAgent := map[string]render.PluginRow{} for _, r := range render.BuildReport(c, plan, []string{"claude", "codex"}).Rows { @@ -704,7 +704,7 @@ func TestBuildReport_NotTargetedRows(t *testing.T) { Plugins: []source.Plugin{{ID: "toolkit", Plugin: tc.spec}}, } plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ - tc.agent: {Ops: []adapter.FileOp{{Action: "write", Path: "/home/.claude.json"}}}, + tc.agent: {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: "/home/.claude.json"}}}, }} report := render.BuildReport(c, plan, []string{tc.agent}) if len(report.Rows) != 1 { @@ -746,11 +746,11 @@ func TestBuildReport_CoverageOutcomes(t *testing.T) { want string wantMark string }{ - {name: "no skips", ops: []adapter.FileOp{{Action: "write", Path: "/x"}}, want: "full", wantMark: "✓ full"}, + {name: "no skips", ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: "/x"}}, want: "full", wantMark: "✓ full"}, { name: "skipped something but still rendered", skips: []adapter.Skip{{Component: "lsp", Name: "l", Reason: "no concept"}}, - ops: []adapter.FileOp{{Action: "write", Path: "/x"}}, + ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: "/x"}}, want: "partial", wantMark: "◐ partial", }, { @@ -806,7 +806,7 @@ func TestBuildReport_CountsHonourPluginTargeting(t *testing.T) { }, } plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", Path: "/x"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: "/x"}}}, }} report := render.BuildReport(c, plan, []string{"claude"}) if len(report.Rows) != 1 { diff --git a/internal/render/state_apply.go b/internal/render/state_apply.go index 7c79ef90..9142550e 100644 --- a/internal/render/state_apply.go +++ b/internal/render/state_apply.go @@ -50,7 +50,7 @@ func PruneStaleState(s *state.Targets, userHome, agent string, scope adapter.Sco currentFiles := map[string]struct{}{} // portable path → present currentKeys := map[string]map[string]struct{}{} // portable path → set of pointers for _, op := range ops { - if op.Action != "" && op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } portable := paths.HomeRelative(userHome, op.Path) @@ -138,7 +138,7 @@ func OrphanFiles(s *state.Targets, userHome, agent string, scope adapter.Scope, portableProject := paths.HomeRelative(userHome, project) current := map[string]struct{}{} for _, op := range ops { - if op.Action != "" && op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } if IsKeyMerge(op.MergeStrategy) { @@ -255,7 +255,7 @@ func orphanDeletes(s *state.Targets, userHome, agent string, scope adapter.Scope portableProject := paths.HomeRelative(userHome, project) rendered := map[string]struct{}{} for _, op := range ops { - if op.Action != "" && op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } if IsKeyMerge(op.MergeStrategy) { @@ -275,7 +275,7 @@ func orphanDeletes(s *state.Targets, userHome, agent string, scope adapter.Scope continue } out = append(out, adapter.FileOp{ - Action: "delete", + Action: adapter.ActionDelete, Path: key.AbsPath(userHome), SourceID: entry.SourceID, Mode: entry.Mode, @@ -299,7 +299,7 @@ func RecordOpsState(s *state.Targets, userHome, agent string, scope adapter.Scop now := time.Now().UTC() scopeName := scope.String() for _, op := range ops { - if op.Action != "" && op.Action != "write" { + if op.Action != adapter.ActionWrite { continue } switch op.MergeStrategy { diff --git a/internal/render/state_apply_test.go b/internal/render/state_apply_test.go index 7fea84f0..e617685e 100644 --- a/internal/render/state_apply_test.go +++ b/internal/render/state_apply_test.go @@ -23,7 +23,7 @@ func TestRecordState_FilesAndKeys(t *testing.T) { s := state.New() // Use dir as home so the recorded state key uses HOME-relative form. err := render.RecordOpsState(s, dir, "claude", adapter.ScopeUser, "", []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p, MergeStrategy: "merge-json-keys", Content: []byte(`{"mcpServers":{"github":{"command":"npx"}}}`), @@ -49,7 +49,7 @@ func TestRecordState_FileReplace(t *testing.T) { s := state.New() err := render.RecordOpsState(s, dir, "claude", adapter.ScopeUser, "", []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: p, Content: content, Mode: 0o644, @@ -83,7 +83,7 @@ func TestPruneStaleState_DropsRemovedFiles(t *testing.T) { s.Files[otherAgent] = state.FileEntry{SHA256: "c"} render.PruneStaleState(s, home, "claude", adapter.ScopeUser, "", []adapter.FileOp{ - {Action: "write", Path: "/home/me/.claude/agents/keep.md"}, + {Action: adapter.ActionWrite, Path: "/home/me/.claude/agents/keep.md"}, }) if _, ok := s.Files[keep]; !ok { t.Fatal("kept entry was pruned") @@ -106,7 +106,7 @@ func TestPruneStaleState_DropsRemovedKeys(t *testing.T) { s.Keys[dropKey] = state.KeyEntry{SHA256: "b"} render.PruneStaleState(s, home, "claude", adapter.ScopeUser, "", []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: clauJSON, MergeStrategy: "merge-json-keys", Content: []byte(`{"mcpServers":{"keep":{"command":"x"}}}`), @@ -138,7 +138,7 @@ func TestState_PortableAcrossHomes(t *testing.T) { t.Fatal(err) } if err := render.RecordOpsState(s, macHome, "claude", adapter.ScopeUser, "", []adapter.FileOp{ - {Action: "write", Path: macPath}, + {Action: adapter.ActionWrite, Path: macPath}, }); err != nil { t.Fatal(err) } @@ -159,7 +159,7 @@ func TestState_PortableAcrossHomes(t *testing.T) { linuxHome := t.TempDir() // stand-in for /home/alice linuxPath := filepath.Join(linuxHome, ".claude.json") render.PruneStaleState(s, linuxHome, "claude", adapter.ScopeUser, "", []adapter.FileOp{ - {Action: "write", Path: linuxPath}, + {Action: adapter.ActionWrite, Path: linuxPath}, }) if _, ok := s.Files[gotKey]; !ok { t.Fatalf("portable key pruned on machine B; have %v", s.Files) @@ -174,8 +174,8 @@ func TestState_PortableAcrossHomes(t *testing.T) { // regression against anyone reintroducing string matching. func TestPruneStaleState_AmbiguousPathPrefixKeepsLiveKey(t *testing.T) { ops := []adapter.FileOp{ - {Action: "write", Path: "a", MergeStrategy: "merge-json-keys", Content: []byte(`{"x":1}`)}, - {Action: "write", Path: "a:b", MergeStrategy: "merge-json-keys", Content: []byte(`{"realptr":1}`)}, + {Action: adapter.ActionWrite, Path: "a", MergeStrategy: "merge-json-keys", Content: []byte(`{"x":1}`)}, + {Action: adapter.ActionWrite, Path: "a:b", MergeStrategy: "merge-json-keys", Content: []byte(`{"realptr":1}`)}, } liveKey := state.Key{Agent: "claude", Scope: "user", Path: "a:b", Pointer: "/realptr"} for i := 0; i < 64; i++ { @@ -193,7 +193,7 @@ func TestPruneStaleState_AmbiguousPathPrefixKeepsLiveKey(t *testing.T) { func TestRecordState_SkipsDeleteOps(t *testing.T) { s := state.New() err := render.RecordOpsState(s, "/tmp", "claude", adapter.ScopeUser, "", []adapter.FileOp{{ - Action: "delete", + Action: adapter.ActionDelete, Path: "/some/path", }}) if err != nil { diff --git a/internal/render/writer_fifo_unix_test.go b/internal/render/writer_fifo_unix_test.go index 3df762fa..c9d52605 100644 --- a/internal/render/writer_fifo_unix_test.go +++ b/internal/render/writer_fifo_unix_test.go @@ -51,7 +51,7 @@ func mkfifo(t *testing.T, path string) { func TestOrphanDeleteWillProceed_FIFO(t *testing.T) { fifo := filepath.Join(t.TempDir(), "pipe.md") mkfifo(t, fifo) - op := adapter.FileOp{Action: "delete", Path: fifo, SourceID: "subagents/pipe.md"} + op := adapter.FileOp{Action: adapter.ActionDelete, Path: fifo, SourceID: "subagents/pipe.md"} withinTimeout(t, "OrphanDeleteWillProceed", func() { if render.OrphanDeleteWillProceed(op) { t.Error("a FIFO cannot be read or preserved; it must not be reported as reclaimable") @@ -76,7 +76,7 @@ func TestWriterDelete_FIFODoesNotBlock(t *testing.T) { mkfifo(t, fifo) w := render.NewWriter(state.New(), home, tmp, adapter.ScopeUser, "", "claude") - op := adapter.FileOp{Action: "delete", Path: fifo, SourceID: "subagents/pipe.md"} + op := adapter.FileOp{Action: adapter.ActionDelete, Path: fifo, SourceID: "subagents/pipe.md"} withinTimeout(t, "Writer.Delete", func() { if err := w.Delete(op); err != nil { t.Errorf("a non-regular destination must be SKIPPED, not error the run: %v", err) diff --git a/internal/render/writer_test.go b/internal/render/writer_test.go index 3d319067..2a2dd130 100644 --- a/internal/render/writer_test.go +++ b/internal/render/writer_test.go @@ -41,11 +41,11 @@ func (f *fakeJSONApply) KeyMergeStrategy() string { return "merge-json-keys" } func (f *fakeJSONApply) Apply(ops []adapter.FileOp, w adapter.DestWriter) error { for _, op := range ops { switch op.Action { - case "delete": + case adapter.ActionDelete: if err := w.Delete(op); err != nil { return err } - case "", "write": + case adapter.ActionWrite: if op.MergeStrategy == "merge-json-keys" { existing := map[string]any{} if data, err := os.ReadFile(op.Path); err == nil { @@ -84,7 +84,7 @@ func TestWriter_SkipsUnchangedWrite(t *testing.T) { } st := state.New() w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") - op := adapter.FileOp{Action: "write", Path: dest, Content: content, Mode: 0o644, SourceID: "note.md"} + op := adapter.FileOp{Action: adapter.ActionWrite, Path: dest, Content: content, Mode: 0o644, SourceID: "note.md"} if err := w.Write(op, content); err != nil { t.Fatalf("Write: %v", err) } @@ -123,7 +123,7 @@ func TestWriter_FileLevelBackup(t *testing.T) { st := state.New() w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: []byte("---\nname: reviewer\n---\nNew shiny rendered prompt.\n"), Mode: 0o644, @@ -175,7 +175,7 @@ func TestWriter_KeyLevelBackup(t *testing.T) { w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") ours := []byte(`{"mcpServers":{"github":{"command":"npx","args":["-y","@m/server-github"]}}}`) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: ours, MergeStrategy: "merge-json-keys", @@ -237,7 +237,7 @@ func TestWriter_KeyLevelBackup_ForeignNonObjectAtOwnedKey(t *testing.T) { w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") ours := []byte(`{"mcpServers":{"github":{"command":"npx"}}}`) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: ours, MergeStrategy: "merge-json-keys", @@ -278,7 +278,7 @@ func TestWriter_KeyLevelBackup_ForeignNullAtOwnedPointer(t *testing.T) { w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") ours := []byte(`{"mcpServers":{"github":{"command":"npx"}}}`) op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: ours, MergeStrategy: "merge-json-keys", @@ -314,7 +314,7 @@ func TestWriter_NoCollisionWhenAlreadyOwned(t *testing.T) { st.Files[state.NewFileKey(tmp, "claude", "user", "", dest)] = state.FileEntry{SHA256: "anything"} w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") - op := adapter.FileOp{Action: "write", Path: dest, Content: []byte("ours-v2"), Mode: 0o644} + op := adapter.FileOp{Action: adapter.ActionWrite, Path: dest, Content: []byte("ours-v2"), Mode: 0o644} if err := w.Write(op, op.Content); err != nil { t.Fatal(err) } @@ -339,7 +339,7 @@ func TestWriter_NoCollisionWhenContentMatches(t *testing.T) { st := state.New() w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") - if err := w.Write(adapter.FileOp{Action: "write", Path: dest, Content: content}, content); err != nil { + if err := w.Write(adapter.FileOp{Action: adapter.ActionWrite, Path: dest, Content: content}, content); err != nil { t.Fatal(err) } if len(w.Reports()) != 0 { @@ -358,7 +358,7 @@ func TestWriter_DeleteSkipsBackup(t *testing.T) { st := state.New() w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "claude") - if err := w.Delete(adapter.FileOp{Action: "delete", Path: dest}); err != nil { + if err := w.Delete(adapter.FileOp{Action: adapter.ActionDelete, Path: dest}); err != nil { t.Fatal(err) } if _, err := os.Stat(dest); !os.IsNotExist(err) { @@ -389,9 +389,9 @@ func TestRenderApply_MultipleMergeOpsSamePathAllApplied(t *testing.T) { plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: dest, Content: []byte(`{"mcpServers":{"a":1}}`), MergeStrategy: "merge-json-keys", Mode: 0o644}, - {Action: "write", Path: dest, Content: []byte(`{"hooks":{"PreToolUse":"echo"}}`), MergeStrategy: "merge-json-keys", Mode: 0o644}, - {Action: "write", Path: dest, Content: []byte(`{"lspServers":{"go":{"command":"gopls"}}}`), MergeStrategy: "merge-json-keys", Mode: 0o644}, + {Action: adapter.ActionWrite, Path: dest, Content: []byte(`{"mcpServers":{"a":1}}`), MergeStrategy: "merge-json-keys", Mode: 0o644}, + {Action: adapter.ActionWrite, Path: dest, Content: []byte(`{"hooks":{"PreToolUse":"echo"}}`), MergeStrategy: "merge-json-keys", Mode: 0o644}, + {Action: adapter.ActionWrite, Path: dest, Content: []byte(`{"lspServers":{"go":{"command":"gopls"}}}`), MergeStrategy: "merge-json-keys", Mode: 0o644}, }}, }, } @@ -423,8 +423,8 @@ func TestRenderApply_MultipleMergeOpsSamePathAllApplied(t *testing.T) { func TestRenderApply_SharedWriteDivergence(t *testing.T) { newPlan := func(dest, claudeBody, opencodeBody string) render.RenderPlan { return render.RenderPlan{PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", Path: dest, Content: []byte(claudeBody), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, - "opencode": {Ops: []adapter.FileOp{{Action: "write", Path: dest, Content: []byte(opencodeBody), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: dest, Content: []byte(claudeBody), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, + "opencode": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: dest, Content: []byte(opencodeBody), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, }} } @@ -489,8 +489,8 @@ func TestRenderApply_IntraAgentDivergenceMessage(t *testing.T) { plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: dest, Content: []byte("from plugin A"), Mode: 0o644, SourceID: "subagents/code-reviewer.md"}, - {Action: "write", Path: dest, Content: []byte("from plugin B"), Mode: 0o644, SourceID: "subagents/code-reviewer.md"}, + {Action: adapter.ActionWrite, Path: dest, Content: []byte("from plugin A"), Mode: 0o644, SourceID: "subagents/code-reviewer.md"}, + {Action: adapter.ActionWrite, Path: dest, Content: []byte("from plugin B"), Mode: 0o644, SourceID: "subagents/code-reviewer.md"}, }}, }} _, _, _, err := render.Apply(plan, reg, state.New(), home, tmp, adapter.ScopeUser, "") @@ -518,8 +518,8 @@ func TestPreviewApply_SharedWriteDivergence(t *testing.T) { _ = reg.Register(&fakeJSONApply{name: "claude"}) _ = reg.Register(&fakeJSONApply{name: "opencode"}) plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ - "claude": {Ops: []adapter.FileOp{{Action: "write", Path: dest, Content: []byte("A"), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, - "opencode": {Ops: []adapter.FileOp{{Action: "write", Path: dest, Content: []byte("B"), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, + "claude": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: dest, Content: []byte("A"), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, + "opencode": {Ops: []adapter.FileOp{{Action: adapter.ActionWrite, Path: dest, Content: []byte("B"), Mode: 0o644, SourceID: "skills/x/SKILL.md"}}}, }} if _, _, _, err := render.PreviewApply(plan, reg, state.New(), home, tmp, adapter.ScopeUser, ""); err == nil { t.Fatal("expected dry-run preview to fail loud on divergent shared-path content") @@ -549,9 +549,9 @@ func TestPreviewApply_SyncedVsWouldChange(t *testing.T) { _ = reg.Register(&fakeJSONApply{name: "claude"}) plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: synced, Content: []byte("same\n"), Mode: 0o644, SourceID: "skills/a/SKILL.md"}, - {Action: "write", Path: changed, Content: []byte("new\n"), Mode: 0o644, SourceID: "skills/b/SKILL.md"}, - {Action: "write", Path: missing, Content: []byte("new\n"), Mode: 0o644, SourceID: "skills/c/SKILL.md"}, + {Action: adapter.ActionWrite, Path: synced, Content: []byte("same\n"), Mode: 0o644, SourceID: "skills/a/SKILL.md"}, + {Action: adapter.ActionWrite, Path: changed, Content: []byte("new\n"), Mode: 0o644, SourceID: "skills/b/SKILL.md"}, + {Action: adapter.ActionWrite, Path: missing, Content: []byte("new\n"), Mode: 0o644, SourceID: "skills/c/SKILL.md"}, }}, }} @@ -598,7 +598,7 @@ func TestRenderApply_FullPathBacksUpAcrossAgents(t *testing.T) { plan := render.RenderPlan{ PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{{ - Action: "write", + Action: adapter.ActionWrite, Path: claudeDest, Content: []byte("rendered-claude"), Mode: 0o644, @@ -668,7 +668,7 @@ func TestWriter_JSONCFallbackBacksUpWholeFile(t *testing.T) { st := state.New() // nothing owned → must back up w := render.NewWriter(st, home, tmp, adapter.ScopeUser, "", "opencode") op := adapter.FileOp{ - Action: "write", + Action: adapter.ActionWrite, Path: dest, Content: []byte(`{"mcp":{"github":{"command":"new"}}}`), MergeStrategy: "merge-json-keys", @@ -717,7 +717,7 @@ func TestApply_UnreadableOrphanIsSkippedNotDeleted(t *testing.T) { _ = reg.Register(&fakeJSONApply{name: "claude"}) plan := render.RenderPlan{PerAgent: map[string]render.AgentResult{ "claude": {Ops: []adapter.FileOp{ - {Action: "write", Path: live, Content: []byte("live"), Mode: 0o644, SourceID: "subagents/live.md"}, + {Action: adapter.ActionWrite, Path: live, Content: []byte("live"), Mode: 0o644, SourceID: "subagents/live.md"}, }}, }} @@ -766,7 +766,7 @@ func TestApply_UnreadableOrphanIsSkippedNotDeleted(t *testing.T) { func TestOrphanDeleteWillProceed(t *testing.T) { tmp := t.TempDir() op := func(name, sourceID string) adapter.FileOp { - return adapter.FileOp{Action: "delete", Path: filepath.Join(tmp, name), SourceID: sourceID} + return adapter.FileOp{Action: adapter.ActionDelete, Path: filepath.Join(tmp, name), SourceID: sourceID} } readable := filepath.Join(tmp, "readable.md") From 0207296a169eb4091db15e488da30f4f81c0edaf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 16:34:02 +0000 Subject: [PATCH 2/6] refactor(render): stamp an explicit OpCleanup kind on orphan-cleanup ops (#230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An orphan-cleanup op — the empty key-merge write `render.Plan` synthesizes when a source section empties, whose only work is pruning that section's owned keys through the merge path — was recognised by `apply` via three copies of a shape sniff (`IsKeyMerge && TrimSpace(Content) == "{}" && len(OwnedKeys) > 0`) plus a pinning test asserting no adapter could ever render that shape for a populated section. The op now carries its identity: `FileOp.Kind` (`adapter.OpKind`) is `OpRender` (the zero value) for an ordinary rendered op and `OpCleanup` for a cleanup, orthogonal to `Action` — a cleanup op stays an `ActionWrite`, since the merge performs the removal. The three sniffs in `printPlannedOp`, `planSyncCounts` and `removalCounts` become `op.Kind == adapter.OpCleanup`. Both synthesis sites — `render.orphanCleanupOps` and `agent disable --purge`'s pointer prune — build the op through the single constructor `adapter.CleanupOp(path, strategy, owned)`, the only producer of `OpCleanup`, so the kind cannot be missed at either site and no production code hand-rolls the `{}`+OwnedKeys literal any more. `TestAdapters_NeverRenderEmptyObjectForPopulatedSection` keeps its name and body but is now an adapter-fidelity guard only (an adapter rendering `{}` for a populated section has silently dropped it); nothing consumes the `{}`-uniqueness property any more. New: `TestOrphanCleanupOps_StampsOpCleanup` (render) pins the stamp through the pipeline path, and `TestCleanupOp` (adapter) pins every field the constructor sets, so dropping the stamp fails in three packages — those two plus `TestApplyDryRun_CleanupOpNotCountedToWrite` (cli). No user-visible behaviour changes; the #229 characterization goldens are untouched. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- internal/adapter/action_test.go | 55 ++++++++++++++-- internal/adapter/adapter.go | 54 ++++++++++++++- internal/cli/agent.go | 9 +-- internal/cli/apply.go | 41 ++++++------ internal/cli/keymergestrategy_test.go | 23 +++---- internal/render/pipeline.go | 9 +-- .../render/pipeline_cleanup_internal_test.go | 65 +++++++++++++++++++ 7 files changed, 198 insertions(+), 58 deletions(-) create mode 100644 internal/render/pipeline_cleanup_internal_test.go diff --git a/internal/adapter/action_test.go b/internal/adapter/action_test.go index d804cb83..9ccba93e 100644 --- a/internal/adapter/action_test.go +++ b/internal/adapter/action_test.go @@ -2,16 +2,18 @@ package adapter_test import ( "fmt" + "slices" "testing" "github.com/spxrogers/agentsync/internal/adapter" ) // TestFileOpEnums_ZeroValues is the one explicit pin of the premise the typed -// FileOp fields rest on: a FileOp built without naming an action writes. The -// intake normalizations ("" → "write") were deleted on the strength of this, -// so the bare `var` declaration — never an explicit constant — is the point of -// each subtest. +// FileOp fields rest on: a FileOp built without naming an action writes, and +// one built without naming a kind is an ordinary render. The intake +// normalizations ("" → "write") were deleted on the strength of this, so the +// bare `var` declaration — never an explicit constant — is the point of each +// subtest. func TestFileOpEnums_ZeroValues(t *testing.T) { t.Run("zero Action is ActionWrite", func(t *testing.T) { var a adapter.Action @@ -19,11 +21,18 @@ func TestFileOpEnums_ZeroValues(t *testing.T) { t.Fatalf("zero adapter.Action = %v, want ActionWrite", a) } }) + t.Run("zero OpKind is OpRender", func(t *testing.T) { + var k adapter.OpKind + if k != adapter.OpRender { + t.Fatalf("zero adapter.OpKind = %v, want OpRender", k) + } + }) } // TestFileOpEnums_String pins the human surface the dry-run op label and // DispatchOps' error text depend on: %s and %q both route through Stringer, so -// an out-of-range value reads "action()" rather than a bare integer. +// an out-of-range value reads "action()" / "opkind()" rather than a bare +// integer. func TestFileOpEnums_String(t *testing.T) { tests := []struct { name string @@ -33,6 +42,9 @@ func TestFileOpEnums_String(t *testing.T) { {name: "ActionWrite", v: adapter.ActionWrite, want: "write"}, {name: "ActionDelete", v: adapter.ActionDelete, want: "delete"}, {name: "Action(9)", v: adapter.Action(9), want: "action(9)"}, + {name: "OpRender", v: adapter.OpRender, want: "render"}, + {name: "OpCleanup", v: adapter.OpCleanup, want: "cleanup"}, + {name: "OpKind(9)", v: adapter.OpKind(9), want: "opkind(9)"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -45,3 +57,36 @@ func TestFileOpEnums_String(t *testing.T) { }) } } + +// TestCleanupOp pins every field the single cleanup-op constructor sets. It is +// the ONLY producer of OpCleanup — render.orphanCleanupOps and `agent disable +// --purge` both call it — so a field it drops is dropped at every synthesis +// site at once, and a missing Kind stamp here relabels every key removal as a +// write in `apply`. +func TestCleanupOp(t *testing.T) { + owned := []string{"/mcpServers/a", "/mcpServers/b"} + op := adapter.CleanupOp("/home/u/.claude.json", "merge-json-keys", owned) + tests := []struct { + name string + got, want any + }{ + {name: "Action is ActionWrite", got: op.Action, want: adapter.ActionWrite}, + {name: "Kind is OpCleanup", got: op.Kind, want: adapter.OpCleanup}, + {name: "Path", got: op.Path, want: "/home/u/.claude.json"}, + {name: "Content is the empty object", got: string(op.Content), want: "{}"}, + {name: "Mode", got: op.Mode, want: uint32(0o644)}, + {name: "MergeStrategy", got: op.MergeStrategy, want: "merge-json-keys"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.got != tt.want { + t.Errorf("got %v, want %v", tt.got, tt.want) + } + }) + } + t.Run("OwnedKeys are the input pointers", func(t *testing.T) { + if !slices.Equal(op.OwnedKeys, owned) { + t.Errorf("OwnedKeys = %v, want %v", op.OwnedKeys, owned) + } + }) +} diff --git a/internal/adapter/adapter.go b/internal/adapter/adapter.go index 49e8751f..e0bb897a 100644 --- a/internal/adapter/adapter.go +++ b/internal/adapter/adapter.go @@ -58,8 +58,9 @@ func (s Scope) String() string { // there is nothing to normalize at any intake: no empty spelling exists that // an executor would write while a guard matching "write" let it through. // Unlike SkipKind below, whose zero value is invalid by design so an unstamped -// Skip fails TestEverySkipLiteralSetsKind, the zero value of Action is -// deliberately valid: the common case must need no stamp. +// Skip fails TestEverySkipLiteralSetsKind, the zero values of Action and +// OpKind are deliberately valid (write, render): the common case must need no +// stamp. type Action int const ( @@ -86,8 +87,54 @@ func (a Action) String() string { } } +// OpKind says why a FileOp exists, orthogonally to what it does (Action). The +// zero value is OpRender — an ordinary op an adapter's Render emitted — so, as +// with Action, the common case needs no stamp. +type OpKind int + +const ( + // OpRender is an ordinary rendered op. It is the zero value. + OpRender OpKind = iota + // OpCleanup is an orphan-cleanup op, defined by its shape: an ActionWrite + // of "{}" to a key-merge destination whose only work is pruning OwnedKeys + // — the merge path performs the removal, so the op writes nothing new. It + // is produced by CleanupOp (called from render.orphanCleanupOps when a + // key-merge section empties in the source, and from `agent disable + // --purge`); consumers read this kind instead of sniffing the shape. + OpCleanup +) + +// String renders the kind for human notes: "render" | "cleanup" | "opkind()". +func (k OpKind) String() string { + switch k { + case OpRender: + return "render" + case OpCleanup: + return "cleanup" + default: + return fmt.Sprintf("opkind(%d)", int(k)) + } +} + +// CleanupOp builds the op that prunes owned keys from a key-merge destination: +// an ActionWrite of "{}" whose only work is the OwnedKeys removal the merge +// path performs. It is the ONLY producer of OpCleanup — every site that +// synthesizes a cleanup op must call it so the kind is never missed. +func CleanupOp(path, strategy string, owned []string) FileOp { + return FileOp{ + Action: ActionWrite, + Kind: OpCleanup, + Path: path, + Content: []byte("{}"), + Mode: 0o644, + MergeStrategy: strategy, + OwnedKeys: owned, + } +} + // FileOp describes one destination-side change. Action says what happens to -// the destination: write (the zero value) or delete. +// the destination (write — the zero value — or delete); Kind says why the op +// exists (an ordinary render, or a synthesized orphan cleanup). // Path is absolute (after AGENTSYNC_TARGET_ROOT redirection). // // CONTRACT — Content is ALWAYS JSON for a key-merge op, regardless of the @@ -109,6 +156,7 @@ func (a Action) String() string { // not emit the on-disk format here. type FileOp struct { Action Action // write (zero value) | delete + Kind OpKind // render (zero value) | cleanup Path string Content []byte Mode uint32 diff --git a/internal/cli/agent.go b/internal/cli/agent.go index ce178ea0..b6b75293 100644 --- a/internal/cli/agent.go +++ b/internal/cli/agent.go @@ -712,14 +712,7 @@ func purgeAgentDests(cmd *cobra.Command, name, home string, sc adapter.Scope, pr if _, err := os.Stat(abs); err != nil { continue // already gone; nothing to prune } - ops = append(ops, adapter.FileOp{ - Action: adapter.ActionWrite, - Path: abs, - Content: []byte("{}"), - Mode: 0o644, - MergeStrategy: strat, - OwnedKeys: ptrs, - }) + ops = append(ops, adapter.CleanupOp(abs, strat, ptrs)) prunedFiles++ } } diff --git a/internal/cli/apply.go b/internal/cli/apply.go index 3803c94d..7b9e0209 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -339,13 +339,14 @@ func printPlannedOp(w io.Writer, p *ui.Printer, op adapter.FileOp, wouldChange m // an ESC in a shared config's name can't inject escapes into the plan preview // (issue #93/#171). dispPath := ui.Sanitize(op.Path) - // A pure orphan-cleanup op (the "{}"+OwnedKeys signature) is a key REMOVAL: - // it is excluded from the "to write" headline count (planSyncCounts) and - // summarized under "Removals:", so labeling it "write" here would make the - // listing disagree with both. Checked BEFORE isSyncedOp — planSyncCounts - // classifies it as a removal first too, and an already-converged cleanup op - // printing "synced" would reopen the same listing/headline split. - if render.IsKeyMerge(op.MergeStrategy) && strings.TrimSpace(string(op.Content)) == "{}" && len(op.OwnedKeys) > 0 { + // An orphan-cleanup op (adapter.OpCleanup, stamped at synthesis) is a key + // REMOVAL: it is excluded from the "to write" headline count + // (planSyncCounts) and summarized under "Removals:", so labeling it "write" + // here would make the listing disagree with both. Checked BEFORE isSyncedOp + // — planSyncCounts classifies it as a removal first too, and an + // already-converged cleanup op printing "synced" would reopen the same + // listing/headline split. + if op.Kind == adapter.OpCleanup { fmt.Fprintf(w, " %s %s %s\n", p.Yellow(ui.GlyphArrow), p.Yellow(ui.Pad("remove", 6)), dispPath) return } @@ -372,13 +373,13 @@ func isSyncedOp(op adapter.FileOp, wouldChange map[string]bool) bool { func planSyncCounts(plan render.RenderPlan, wouldChange map[string]bool) (toWrite, synced, removals int) { for _, res := range plan.PerAgent { for _, op := range res.Ops { - // A pure orphan-cleanup op (the "{}"+OwnedKeys signature — same - // predicate as removalCounts) is previewed under "Removals:", and - // the real apply's headline subtracts it from "applied: X ops" — - // counting it in "to write" too made the dry-run and real headlines - // disagree by one per cleanup op. It is returned as its own count so - // the Plan line still sums to the total. - if render.IsKeyMerge(op.MergeStrategy) && strings.TrimSpace(string(op.Content)) == "{}" && len(op.OwnedKeys) > 0 { + // An orphan-cleanup op (adapter.OpCleanup — the same kind + // removalCounts reads) is previewed under "Removals:", and the real + // apply's headline subtracts it from "applied: X ops" — counting it + // in "to write" too made the dry-run and real headlines disagree by + // one per cleanup op. It is returned as its own count so the Plan + // line still sums to the total. + if op.Kind == adapter.OpCleanup { removals++ continue } @@ -408,7 +409,7 @@ func planSyncCounts(plan render.RenderPlan, wouldChange map[string]bool) (toWrit // // The two counts are returned separately so the headline can label them // distinctly (a deleted key is not an "op" in the same sense a deleted file is). -// appliedOps is plan.Total() with the pure "{}" removal ops subtracted, so a +// appliedOps is plan.Total() with the cleanup (OpCleanup) ops subtracted, so a // mixed run reports the removal under "removed:" and does not also count it as an // applied write. MUST be called BEFORE PruneStaleState, which drops the state // entries OrphanDeletes reads. (Dry-run never prunes, so it can call this @@ -428,12 +429,10 @@ func removalCounts(plan render.RenderPlan, s *state.Targets, userHome string, sc } } for _, op := range res.Ops { - // An orphan-cleanup op is a key-merge op whose rendered content is the - // empty object "{}" but which carries owned pointers to delete. Adapters - // never render "{}" for a populated section (pinned by - // TestAdapters_NeverRenderEmptyObjectForPopulatedSection), so this - // signature is unique to the cleanup synthesis. - if render.IsKeyMerge(op.MergeStrategy) && strings.TrimSpace(string(op.Content)) == "{}" && len(op.OwnedKeys) > 0 { + // An orphan-cleanup op (adapter.OpCleanup, stamped by + // adapter.CleanupOp) carries the owned pointers to delete and writes + // nothing else. + if op.Kind == adapter.OpCleanup { removedKeys += len(op.OwnedKeys) appliedOps-- // a removal, not an applied write } diff --git a/internal/cli/keymergestrategy_test.go b/internal/cli/keymergestrategy_test.go index 7e7a0fae..0032ea68 100644 --- a/internal/cli/keymergestrategy_test.go +++ b/internal/cli/keymergestrategy_test.go @@ -124,16 +124,14 @@ func TestKeyMergeStrategy_MatchesEmittedOps(t *testing.T) { } } -// TestAdapters_NeverRenderEmptyObjectForPopulatedSection pins the assumption -// removalCounts (internal/cli/apply.go) rests on: a key-merge op whose trimmed -// content is "{}" AND which carries OwnedKeys is uniquely the orphan-cleanup op -// render.Plan synthesizes for an emptied section — no adapter renders "{}" for -// a POPULATED section. Cleanup ops only exist post-Plan, so ANY trimmed-"{}" -// key-merge op straight out of an adapter's Render on this populated fixture -// would collide with that signature (once Plan attached owned pointers, apply -// would count the section's keys as "removed" and delete them). Renders the -// same MCP+hook fixture as TestKeyMergeStrategy_MatchesEmittedOps through every -// registered adapter at both scopes and asserts none emits it. +// TestAdapters_NeverRenderEmptyObjectForPopulatedSection is an adapter-fidelity +// guard: an adapter that renders a trimmed-"{}" key-merge op for a POPULATED +// canonical section has silently dropped that section (CLAUDE.md, "models must +// stay faithful to their on-disk artifacts"). It renders the same MCP+hook +// fixture as TestKeyMergeStrategy_MatchesEmittedOps through every registered +// adapter at both scopes — the only registry-wide check, so it also covers a +// thinly-tested breadth-tier agent — and asserts none emits it. It is a narrow +// guard: a `{"mcpServers":{}}` render for a populated section passes it. func TestAdapters_NeverRenderEmptyObjectForPopulatedSection(t *testing.T) { testenv.RequireContainer(t) fixture := keyMergeFixture() @@ -166,9 +164,8 @@ func TestAdapters_NeverRenderEmptyObjectForPopulatedSection(t *testing.T) { totalKeyMergeOps++ if strings.TrimSpace(string(op.Content)) == "{}" { t.Errorf("[%s/%s] rendered a trimmed-\"{}\" key-merge op for %q "+ - "(OwnedKeys=%v) from a populated fixture — this collides with the "+ - "synthesized-cleanup signature removalCounts keys off", - name, sc.name, op.Path, op.OwnedKeys) + "from a populated fixture — the adapter silently dropped the section", + name, sc.name, op.Path) } } } diff --git a/internal/render/pipeline.go b/internal/render/pipeline.go index 8151c55e..8de20045 100644 --- a/internal/render/pipeline.go +++ b/internal/render/pipeline.go @@ -327,14 +327,7 @@ func orphanCleanupOps(s *state.Targets, a adapter.Adapter, agent string, scope a if _, err := os.Stat(abs); err != nil { continue } - cleanup = append(cleanup, adapter.FileOp{ - Action: adapter.ActionWrite, - Path: abs, - Content: []byte("{}"), - Mode: 0o644, - MergeStrategy: strat, - OwnedKeys: ownedByPath[path], - }) + cleanup = append(cleanup, adapter.CleanupOp(abs, strat, ownedByPath[path])) } return cleanup } diff --git a/internal/render/pipeline_cleanup_internal_test.go b/internal/render/pipeline_cleanup_internal_test.go new file mode 100644 index 00000000..363a20f6 --- /dev/null +++ b/internal/render/pipeline_cleanup_internal_test.go @@ -0,0 +1,65 @@ +package render + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/adapter/noop" + "github.com/spxrogers/agentsync/internal/paths" + "github.com/spxrogers/agentsync/internal/state" + "github.com/spxrogers/agentsync/internal/testenv" +) + +// keyMergeNoop is a noop adapter that claims a key-merge strategy, so +// orphanCleanupOps has a destination format to synthesize against. +type keyMergeNoop struct{ *noop.Adapter } + +func (keyMergeNoop) KeyMergeStrategy() string { return "merge-json-keys" } + +// TestOrphanCleanupOps_StampsOpCleanup pins that the cleanup op the pipeline +// synthesizes for an emptied key-merge section is built by adapter.CleanupOp +// and so carries Kind == adapter.OpCleanup: `apply` labels and counts the op +// as a removal by reading that kind, not by sniffing the "{}"+OwnedKeys shape, +// so a missing stamp would silently relabel every key removal as a write. The +// purge path (`agent disable --purge`) calls the same constructor. +func TestOrphanCleanupOps_StampsOpCleanup(t *testing.T) { + testenv.RequireContainer(t) + userHome := t.TempDir() + dest := filepath.Join(userHome, ".claude.json") + // The dest must exist: orphanCleanupOps skips a dest that is already gone + // rather than creating an empty "{}" file. + if err := os.WriteFile(dest, []byte(`{"mcpServers":{"srv":{"command":"x"}}}`), 0o644); err != nil { + t.Fatal(err) + } + s := state.New() + owned := state.Key{Agent: "claude", Scope: "user", Path: paths.HomeRelative(userHome, dest), Pointer: "/mcpServers/srv"} + s.Keys[owned] = state.KeyEntry{SHA256: "deadbeef"} + + // The adapter rendered NO op for the mcpServers section this run — the + // source section emptied — so the owned pointer is orphaned. + got := orphanCleanupOps(s, keyMergeNoop{noop.New("claude")}, "claude", adapter.ScopeUser, "", userHome, nil) + if len(got) != 1 { + t.Fatalf("orphanCleanupOps = %+v, want exactly one cleanup op", got) + } + op := got[0] + if op.Kind != adapter.OpCleanup { + t.Errorf("Kind = %v, want OpCleanup — apply reads this kind to label and count the removal", op.Kind) + } + if op.Action != adapter.ActionWrite { + t.Errorf("Action = %v, want ActionWrite (the merge path performs the removal)", op.Action) + } + if op.Path != dest { + t.Errorf("Path = %q, want %q", op.Path, dest) + } + if string(op.Content) != "{}" { + t.Errorf("Content = %q, want \"{}\"", op.Content) + } + if op.MergeStrategy != "merge-json-keys" { + t.Errorf("MergeStrategy = %q, want the adapter's KeyMergeStrategy", op.MergeStrategy) + } + if len(op.OwnedKeys) != 1 || op.OwnedKeys[0] != "/mcpServers/srv" { + t.Errorf("OwnedKeys = %v, want [/mcpServers/srv]", op.OwnedKeys) + } +} From 21b7d1477cf46f2d33cdfd5b00a40bbc7f200207 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 16:35:57 +0000 Subject: [PATCH 3/6] docs(architecture): FileOp carries a typed Action and an explicit op kind (#230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the typed `adapter.FileOp` in the contract pages: §3 gains a "`FileOp` is typed too" paragraph next to the "Skips are typed" precedent — `Action` with `ActionWrite` as the zero value and no intake normalization; `Kind` saying why an op exists, orthogonal to `Action`; `adapter.CleanupOp` as the only producer of `OpCleanup`, called from `render.orphanCleanupOps` and `agent disable --purge`. `docs/components.md`'s `internal/adapter` key list and `internal/render` orphan-reclaim sentence say the same, and the CHANGELOG gets one internal-only bullet under [Unreleased] > Changed. The website's contract pages are generated from these files at build time (website/scripts/sync-docs.mjs) and its authored pages mention neither `FileOp` nor an action string, so no website edit is needed; the dry-run labels the user guide, README and CLI reference describe are unchanged. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 12 ++++++++++++ docs/architecture.md | 16 ++++++++++++++++ docs/components.md | 8 ++++++-- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b2a5f4e..cf4fd80a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -242,6 +242,18 @@ source layout, CLI surface, and state schema are stabilizing but may still chang same snapshot. The ways the four surfaces disagreed (mode-only drift, symlinked destinations) are resolved in the same release; see Fixed above. +- **Internal: `adapter.FileOp` carries a typed `Action` and an explicit op + `Kind`** ([#230](https://github.com/spxrogers/agentsync/issues/230)). + `Action` was a string with a `"" == "write"` convention normalized at three + pipeline intakes and re-asserted by a comment at every reader; it is now an + enum whose **zero value is write**, so the normalization and its comment tax + are gone. Orphan-cleanup ops — the empty key-merge write that prunes an + emptied section's owned keys — are stamped `OpCleanup` at synthesis (by the + single constructor `adapter.CleanupOp`) instead of being detected by three + copies of a `{}`+`OwnedKeys` shape sniff. No user-visible behaviour changes: + the `apply --dry-run` labels, the `removed: N key(s), M file(s)` headline and + every `--json` payload are byte-identical (`FileOp` is never serialized). + - **`.state/targets.json` is now `schema_version: 2`.** The upgrade is automatic and requires nothing: every command reads the old keys, and the first command that WRITES state (`apply`, `import`, `reconcile`, `migrate`, `agent disable diff --git a/docs/architecture.md b/docs/architecture.md index 21395291..2e314339 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -171,6 +171,22 @@ runtime (e.g. a scope-gap branch) cannot hide from it. `TestEveryAdapterClassifi adapter at both scopes, fails on any unset `Kind`, and pins that both kind values are exercised. +**`FileOp` is typed too.** `Action` (`adapter.Action`) is `ActionWrite` or +`ActionDelete`, and **`ActionWrite` is the zero value** — a `FileOp` built +without naming an action writes. There is no empty spelling and therefore no +intake normalization: `Plan`, `Apply` and `PreviewApply` used to rewrite +`"" → "write"` before their guards ran, and every downstream reader carried a +comment saying whether its ops were plan-normalized or raw adapter output; +both are gone. `Kind` (`adapter.OpKind`) says why the op exists — an ordinary +render (`OpRender`, the zero value) or a synthesized orphan cleanup +(`OpCleanup`) — and is orthogonal to `Action`: a cleanup op is an `ActionWrite` +of `{}` whose only work is pruning an emptied section's owned keys (the merge +path performs the removal via `OwnedKeys`), so the kind is what lets `apply` +label and count it as a removal rather than sniffing `{}`+`OwnedKeys`. Cleanup +ops are built by `adapter.CleanupOp` — the only producer of `OpCleanup`, called +from `render.orphanCleanupOps` and from `agent disable --purge` — so the kind +cannot be missed at a synthesis site. + **Key-merge strategies and on-disk format.** `KeyMergeStrategy` / `FileOp.MergeStrategy` name how an adapter co-owns keys inside a shared config file: `merge-json-keys` (Claude's `.claude.json`/`settings.json`, a project's diff --git a/docs/components.md b/docs/components.md index ef78b07a..30ca1f89 100644 --- a/docs/components.md +++ b/docs/components.md @@ -142,7 +142,9 @@ shared cross-agent dir it writes into, and MUST return nil at project scope (see [architecture § VersionedDirs](architecture.md#versioneddirs-optional)). - **Key:** `Adapter` (interface); `DestWriter` (interface); `VersionedDirs` (optional interface, `VersionRoots`); `NonEmptyDirs` (helper); - `Scope` (`ScopeUser`/`ScopeProject`); `FileOp`; `Skip` (with `SkipKind`); + `Scope` (`ScopeUser`/`ScopeProject`); `FileOp` (with typed `Action` — zero + value `ActionWrite` — and `OpKind`; `CleanupOp` builds the one `OpCleanup` + op); `Skip` (with `SkipKind`); `Registry` (`NewRegistry`, `Register`, `Lookup`, `Names`). Component support is expressed by what `Render` emits — an unsupported component yields a `Skip`, not an absent capability flag. @@ -379,7 +381,9 @@ noop-registered agent unless `AGENTSYNC_ALLOW_UNIMPLEMENTED=1`. Orchestrates apply: canonical + registry → per-agent `FileOp`s/`Skip`s, runs collision detection and backups, records state, and builds the translation report. It reclaims two kinds of orphan: emptied key-merge sections (synthesized -cleanup ops for orphaned owned keys) and **whole-file components** whose +cleanup ops for orphaned owned keys — built by `adapter.CleanupOp` and stamped +`adapter.OpCleanup`, so consumers identify them by kind rather than by shape) +and **whole-file components** whose `source_id` is under `skills/`, `subagents/`, `commands/`, or the retired `agents/` spelling — a destination whose source no longer renders it is deleted, backing up a hand-edit first, and SKIPPED (with the state entry kept, so the next From cfa9d2e2bc193aa3334dd49d8fc9cfe55ceaf175 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 21:34:32 +0000 Subject: [PATCH 4/6] =?UTF-8?q?refactor(adapter):=20close=20review-loop=20?= =?UTF-8?q?round=201=20on=20#249=20=E2=80=94=20NewCleanupOp,=20AST=20guard?= =?UTF-8?q?,=20anchored=20label=20pin=20(#230)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 (four lenses on 21b7d14) found two ISSUEs and a set of NITs: - The "only producer of OpCleanup" promise was asserted, not enforced. Add TestEveryCleanupLiteralUsesNewCleanupOp (sibling of TestEverySkipLiteralSetsKind): any production FileOp literal — direct or an elided slice element — whose Content is a static "{}" outside NewCleanupOp's body fails, with a matcher self-test alongside. - TestApplyDryRun_CleanupOpNotCountedToWrite's label check matched the "Removals: … will remove these" summary line, so printPlannedOp's Kind branch was unpinned. The assertion is anchored to the op's own line now (" remove ", never "write"/"synced"). - CleanupOp → NewCleanupOp (state.NewFileKey precedent; the old name read as a conversion and was one word-swap from OpCleanup). - Docs: OpCleanup is identified by kind, never "defined by its shape"; OpKind.String has no production consumer and says so; OwnedKeys is overwritten on every key-merge op; Kind is why the op exists, not which source produced its content; MergeStrategy stays a string, deferred to #250; the scope-honesty caveat on applyPlan's containment backstop is restored. - Tests: a dest-already-gone subtest for orphanCleanupOps; the skipkind AST fixture no longer spells Action: "write". Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 10 +- docs/architecture.md | 13 +- docs/components.md | 4 +- internal/adapter/action_test.go | 15 +- internal/adapter/adapter.go | 37 ++-- internal/adapter/cleanupop_guard_test.go | 194 ++++++++++++++++++ internal/adapter/skipkind_test.go | 2 +- internal/cli/agent.go | 2 +- internal/cli/apply.go | 4 +- internal/cli/iss155_test.go | 17 +- internal/cli/planwalk.go | 4 +- internal/render/pipeline.go | 7 +- .../render/pipeline_cleanup_internal_test.go | 90 ++++---- 13 files changed, 321 insertions(+), 78 deletions(-) create mode 100644 internal/adapter/cleanupop_guard_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cf4fd80a..d5443e9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -249,10 +249,14 @@ source layout, CLI surface, and state schema are stabilizing but may still chang enum whose **zero value is write**, so the normalization and its comment tax are gone. Orphan-cleanup ops — the empty key-merge write that prunes an emptied section's owned keys — are stamped `OpCleanup` at synthesis (by the - single constructor `adapter.CleanupOp`) instead of being detected by three - copies of a `{}`+`OwnedKeys` shape sniff. No user-visible behaviour changes: - the `apply --dry-run` labels, the `removed: N key(s), M file(s)` headline and + single constructor `adapter.NewCleanupOp`, guarded by an AST test against + hand-rolled `{}` literals) instead of being detected by three copies of a + `{}`+`OwnedKeys` shape sniff. No user-visible behaviour changes: the + `apply --dry-run` labels, the `removed: N key(s), M file(s)` headline and every `--json` payload are byte-identical (`FileOp` is never serialized). + `MergeStrategy` stays a plain string — typing it changes the published + `Adapter` interface and is deferred to + [#250](https://github.com/spxrogers/agentsync/issues/250). - **`.state/targets.json` is now `schema_version: 2`.** The upgrade is automatic and requires nothing: every command reads the old keys, and the first command diff --git a/docs/architecture.md b/docs/architecture.md index 2e314339..6774765f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -179,13 +179,18 @@ intake normalization: `Plan`, `Apply` and `PreviewApply` used to rewrite comment saying whether its ops were plan-normalized or raw adapter output; both are gone. `Kind` (`adapter.OpKind`) says why the op exists — an ordinary render (`OpRender`, the zero value) or a synthesized orphan cleanup -(`OpCleanup`) — and is orthogonal to `Action`: a cleanup op is an `ActionWrite` +(`OpCleanup`), not which source or plugin produced its content — and is +orthogonal to `Action`: a cleanup op is an `ActionWrite` of `{}` whose only work is pruning an emptied section's owned keys (the merge path performs the removal via `OwnedKeys`), so the kind is what lets `apply` label and count it as a removal rather than sniffing `{}`+`OwnedKeys`. Cleanup -ops are built by `adapter.CleanupOp` — the only producer of `OpCleanup`, called -from `render.orphanCleanupOps` and from `agent disable --purge` — so the kind -cannot be missed at a synthesis site. +ops are built by `adapter.NewCleanupOp` — the only producer of `OpCleanup`, +called from `render.orphanCleanupOps` and from `agent disable --purge` — and +`TestEveryCleanupLiteralUsesNewCleanupOp` fails any production `FileOp` literal +that hand-rolls the `{}` shape, so the kind cannot be missed at a synthesis +site. `MergeStrategy` stays a plain string: typing it would change the +published `Adapter` interface (`KeyMergeStrategy() string`) and is deferred to +[#250](https://github.com/spxrogers/agentsync/issues/250). **Key-merge strategies and on-disk format.** `KeyMergeStrategy` / `FileOp.MergeStrategy` name how an adapter co-owns keys inside a shared config diff --git a/docs/components.md b/docs/components.md index 30ca1f89..aa8f89db 100644 --- a/docs/components.md +++ b/docs/components.md @@ -143,7 +143,7 @@ shared cross-agent dir it writes into, and MUST return nil at project scope (see - **Key:** `Adapter` (interface); `DestWriter` (interface); `VersionedDirs` (optional interface, `VersionRoots`); `NonEmptyDirs` (helper); `Scope` (`ScopeUser`/`ScopeProject`); `FileOp` (with typed `Action` — zero - value `ActionWrite` — and `OpKind`; `CleanupOp` builds the one `OpCleanup` + value `ActionWrite` — and `OpKind`; `NewCleanupOp` builds the one `OpCleanup` op); `Skip` (with `SkipKind`); `Registry` (`NewRegistry`, `Register`, `Lookup`, `Names`). Component support is expressed by what `Render` emits — an unsupported component yields a `Skip`, @@ -381,7 +381,7 @@ noop-registered agent unless `AGENTSYNC_ALLOW_UNIMPLEMENTED=1`. Orchestrates apply: canonical + registry → per-agent `FileOp`s/`Skip`s, runs collision detection and backups, records state, and builds the translation report. It reclaims two kinds of orphan: emptied key-merge sections (synthesized -cleanup ops for orphaned owned keys — built by `adapter.CleanupOp` and stamped +cleanup ops for orphaned owned keys — built by `adapter.NewCleanupOp` and stamped `adapter.OpCleanup`, so consumers identify them by kind rather than by shape) and **whole-file components** whose `source_id` is under `skills/`, `subagents/`, `commands/`, or the retired diff --git a/internal/adapter/action_test.go b/internal/adapter/action_test.go index 9ccba93e..4d849595 100644 --- a/internal/adapter/action_test.go +++ b/internal/adapter/action_test.go @@ -58,14 +58,15 @@ func TestFileOpEnums_String(t *testing.T) { } } -// TestCleanupOp pins every field the single cleanup-op constructor sets. It is -// the ONLY producer of OpCleanup — render.orphanCleanupOps and `agent disable -// --purge` both call it — so a field it drops is dropped at every synthesis -// site at once, and a missing Kind stamp here relabels every key removal as a -// write in `apply`. -func TestCleanupOp(t *testing.T) { +// TestNewCleanupOp pins every field the single cleanup-op constructor sets. It +// is the only producer of OpCleanup — render.orphanCleanupOps and `agent +// disable --purge` both call it, and TestEveryCleanupLiteralUsesNewCleanupOp +// keeps it that way — so a field it drops is dropped at every synthesis site +// at once, and a missing Kind stamp here relabels every key removal as a write +// in `apply`. +func TestNewCleanupOp(t *testing.T) { owned := []string{"/mcpServers/a", "/mcpServers/b"} - op := adapter.CleanupOp("/home/u/.claude.json", "merge-json-keys", owned) + op := adapter.NewCleanupOp("/home/u/.claude.json", "merge-json-keys", owned) tests := []struct { name string got, want any diff --git a/internal/adapter/adapter.go b/internal/adapter/adapter.go index e0bb897a..71e6a6b6 100644 --- a/internal/adapter/adapter.go +++ b/internal/adapter/adapter.go @@ -87,24 +87,26 @@ func (a Action) String() string { } } -// OpKind says why a FileOp exists, orthogonally to what it does (Action). The -// zero value is OpRender — an ordinary op an adapter's Render emitted — so, as -// with Action, the common case needs no stamp. +// OpKind says why a FileOp exists — an ordinary render, or a synthesized +// orphan cleanup — orthogonally to what it does (Action). The zero value is +// OpRender, an ordinary op an adapter's Render emitted. type OpKind int const ( // OpRender is an ordinary rendered op. It is the zero value. OpRender OpKind = iota - // OpCleanup is an orphan-cleanup op, defined by its shape: an ActionWrite - // of "{}" to a key-merge destination whose only work is pruning OwnedKeys - // — the merge path performs the removal, so the op writes nothing new. It - // is produced by CleanupOp (called from render.orphanCleanupOps when a - // key-merge section empties in the source, and from `agent disable - // --purge`); consumers read this kind instead of sniffing the shape. + // OpCleanup marks an orphan-cleanup op: an ActionWrite of "{}" to a + // key-merge destination whose only work is pruning OwnedKeys — the merge + // path performs the removal, so the op writes nothing new. Consumers + // identify it by this kind, never by that shape. NewCleanupOp is its + // producer (called from render.orphanCleanupOps when a key-merge section + // empties in the source, and from `agent disable --purge`). OpCleanup ) -// String renders the kind for human notes: "render" | "cleanup" | "opkind()". +// String is the Stringer form — "render" | "cleanup" | "opkind()" — kept +// for symmetry with Action and for %v in test failures; nothing in production +// prints a kind. func (k OpKind) String() string { switch k { case OpRender: @@ -116,11 +118,13 @@ func (k OpKind) String() string { } } -// CleanupOp builds the op that prunes owned keys from a key-merge destination: -// an ActionWrite of "{}" whose only work is the OwnedKeys removal the merge -// path performs. It is the ONLY producer of OpCleanup — every site that -// synthesizes a cleanup op must call it so the kind is never missed. -func CleanupOp(path, strategy string, owned []string) FileOp { +// NewCleanupOp builds the op that prunes owned keys from a key-merge +// destination: an ActionWrite of "{}" whose only work is the OwnedKeys removal +// the merge path performs. It is the only producer of OpCleanup: every site +// that synthesizes a cleanup op must call it so the kind is never missed, and +// TestEveryCleanupLiteralUsesNewCleanupOp fails any production FileOp literal +// that hand-rolls the "{}" shape instead. +func NewCleanupOp(path, strategy string, owned []string) FileOp { return FileOp{ Action: ActionWrite, Kind: OpCleanup, @@ -167,7 +171,8 @@ type FileOp struct { // render.Plan populates it from state — scoped to the top-level sections // this op writes — whenever Plan is given a state (plugin explain/poll pass // nil). A value an adapter's Render sets is only a fallback for Apply driven - // without the pipeline; Plan overwrites it whenever it has state. + // without the pipeline; Plan overwrites it on every key-merge op whenever it + // has state. OwnedKeys []string } diff --git a/internal/adapter/cleanupop_guard_test.go b/internal/adapter/cleanupop_guard_test.go new file mode 100644 index 00000000..2178192f --- /dev/null +++ b/internal/adapter/cleanupop_guard_test.go @@ -0,0 +1,194 @@ +package adapter_test + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// TestEveryCleanupLiteralUsesNewCleanupOp is the reachability-independent guard +// behind NewCleanupOp's "only producer of OpCleanup" promise. apply identifies +// a cleanup op by Kind, never by shape, so a synthesis site that hand-rolls the +// shape — a FileOp literal whose Content is the empty object "{}" — and forgets +// the stamp would relabel every key removal as a write, and on the purge path +// (where nothing reads Kind) no test would notice. This test parses every +// production .go file under internal/ and fails on any FileOp composite literal +// whose Content is a []byte("{}") literal outside NewCleanupOp itself. Sibling +// of TestEverySkipLiteralSetsKind: same scan, same matcher self-test. +func TestEveryCleanupLiteralUsesNewCleanupOp(t *testing.T) { + root := moduleInternalDir(t) + fset := token.NewFileSet() + var ( + offenders []string + total int // FileOp literals matched, for the anti-vacuity floor + ) + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + // Production Go only: test fixtures build "{}" key-merge ops on purpose. + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + f, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + return fmt.Errorf("parse %s: %w", path, perr) + } + inPkgAdapter := f.Name.Name == "adapter" + // The one permitted site is the body of NewCleanupOp in package adapter. + var allowStart, allowEnd token.Pos + if inPkgAdapter { + for _, decl := range f.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Recv == nil && fn.Name.Name == "NewCleanupOp" { + allowStart, allowEnd = fn.Pos(), fn.End() + } + } + } + check := func(cl *ast.CompositeLit) { + total++ + inAllowed := allowStart.IsValid() && cl.Pos() >= allowStart && cl.Pos() < allowEnd + if hasEmptyObjectContent(cl) && !inAllowed { + offenders = append(offenders, posOf(fset, cl)) + } + } + ast.Inspect(f, func(n ast.Node) bool { + cl, ok := n.(*ast.CompositeLit) + if !ok { + return true + } + if isFileOpType(cl.Type, inPkgAdapter) { + check(cl) + return true + } + // `[]adapter.FileOp{ {...}, {...} }`: elided element types. + if at, ok := cl.Type.(*ast.ArrayType); ok && isFileOpType(at.Elt, inPkgAdapter) { + for _, el := range cl.Elts { + if ecl, ok := el.(*ast.CompositeLit); ok { + check(ecl) + } + } + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("scanning internal/ for adapter.FileOp literals: %v", err) + } + + // Anti-vacuity: the tree holds ~47 production FileOp literals (41 adapter + // Render sites plus the pipeline's and cli's synthesized ops). A healthy + // floor rather than an exact count, so adding or removing a site doesn't + // churn this test. + if total < 30 { + t.Fatalf("only matched %d adapter.FileOp literals — the matcher likely broke; expected ~47", total) + } + for _, o := range offenders { + t.Errorf("FileOp literal hand-rolls the \"{}\" cleanup shape without the OpCleanup stamp (call adapter.NewCleanupOp): %s", o) + } +} + +// TestCleanupOpStaticGuardMatchers is the standing self-test for the guard's +// matchers, so a refactor that silently stops them matching cannot turn the +// guard into a vacuous pass. +func TestCleanupOpStaticGuardMatchers(t *testing.T) { + lit := func(src string) *ast.CompositeLit { + t.Helper() + e, err := parser.ParseExpr(src) + if err != nil { + t.Fatalf("ParseExpr(%q): %v", src, err) + } + cl, ok := e.(*ast.CompositeLit) + if !ok { + t.Fatalf("ParseExpr(%q): not a composite literal", src) + } + return cl + } + tests := []struct { + name string + src string + inPkgAdapter bool + wantFileOp bool + wantEmptyObj bool + }{ + {name: "qualified, double-quoted {}", src: `adapter.FileOp{Content: []byte("{}")}`, wantFileOp: true, wantEmptyObj: true}, + {name: "qualified, raw-string {}", src: "adapter.FileOp{Content: []byte(`{}`)}", wantFileOp: true, wantEmptyObj: true}, + {name: "whitespace around {} is still the shape", src: `adapter.FileOp{Content: []byte(" {}\n")}`, wantFileOp: true, wantEmptyObj: true}, + {name: "populated content is not the shape", src: `adapter.FileOp{Content: []byte("{\"a\":1}")}`, wantFileOp: true, wantEmptyObj: false}, + {name: "non-literal content is not statically the shape", src: `adapter.FileOp{Content: body}`, wantFileOp: true, wantEmptyObj: false}, + {name: "no Content at all", src: `adapter.FileOp{Path: "x"}`, wantFileOp: true, wantEmptyObj: false}, + {name: "bare FileOp inside package adapter", src: `FileOp{Content: []byte("{}")}`, inPkgAdapter: true, wantFileOp: true, wantEmptyObj: true}, + {name: "bare FileOp outside package adapter is some other type", src: `FileOp{Content: []byte("{}")}`, wantFileOp: false}, + {name: "a Skip literal is not a FileOp", src: `adapter.Skip{Kind: adapter.SkipDropped}`, wantFileOp: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cl := lit(tt.src) + if got := isFileOpType(cl.Type, tt.inPkgAdapter); got != tt.wantFileOp { + t.Fatalf("isFileOpType = %v, want %v", got, tt.wantFileOp) + } + if !tt.wantFileOp { + return + } + if got := hasEmptyObjectContent(cl); got != tt.wantEmptyObj { + t.Errorf("hasEmptyObjectContent = %v, want %v", got, tt.wantEmptyObj) + } + }) + } +} + +// isFileOpType reports whether e is the type of an adapter.FileOp composite +// literal: the qualified `adapter.FileOp` everywhere, or the bare `FileOp` only +// within package adapter itself. +func isFileOpType(e ast.Expr, inPkgAdapter bool) bool { + switch t := e.(type) { + case *ast.SelectorExpr: + x, ok := t.X.(*ast.Ident) + return ok && x.Name == "adapter" && t.Sel.Name == "FileOp" + case *ast.Ident: + return inPkgAdapter && t.Name == "FileOp" + } + return false +} + +// hasEmptyObjectContent reports whether a FileOp literal sets Content to the +// static empty object — `[]byte("{}")` or `[]byte(`{}`)`, whitespace-trimmed +// like the writer sees it. Content built from a variable or a call is not a +// static shape and is not matched; the guard is deliberately literal-only, +// like its Skip sibling. +func hasEmptyObjectContent(cl *ast.CompositeLit) bool { + for _, el := range cl.Elts { + kv, ok := el.(*ast.KeyValueExpr) + if !ok { + continue + } + if id, ok := kv.Key.(*ast.Ident); !ok || id.Name != "Content" { + continue + } + call, ok := kv.Value.(*ast.CallExpr) + if !ok || len(call.Args) != 1 { + return false + } + at, ok := call.Fun.(*ast.ArrayType) + if !ok || at.Len != nil { + return false + } + if elt, ok := at.Elt.(*ast.Ident); !ok || elt.Name != "byte" { + return false + } + bl, ok := call.Args[0].(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return false + } + s, err := strconv.Unquote(bl.Value) + return err == nil && strings.TrimSpace(s) == "{}" + } + return false +} diff --git a/internal/adapter/skipkind_test.go b/internal/adapter/skipkind_test.go index 5a2fb0ca..9da0174b 100644 --- a/internal/adapter/skipkind_test.go +++ b/internal/adapter/skipkind_test.go @@ -174,7 +174,7 @@ func TestSkipKindStaticGuardMatchers(t *testing.T) { if !isSkipType(lit(`adapter.Skip{Kind: adapter.SkipDropped}`).Type, false) { t.Error("isSkipType(adapter.Skip) = false, want true") } - if isSkipType(lit(`adapter.FileOp{Action: "write"}`).Type, false) { + if isSkipType(lit(`adapter.FileOp{Path: "x"}`).Type, false) { t.Error("isSkipType(adapter.FileOp) = true, want false") } bare := lit(`Skip{Kind: SkipDropped}`).Type diff --git a/internal/cli/agent.go b/internal/cli/agent.go index b6b75293..ddd275a1 100644 --- a/internal/cli/agent.go +++ b/internal/cli/agent.go @@ -712,7 +712,7 @@ func purgeAgentDests(cmd *cobra.Command, name, home string, sc adapter.Scope, pr if _, err := os.Stat(abs); err != nil { continue // already gone; nothing to prune } - ops = append(ops, adapter.CleanupOp(abs, strat, ptrs)) + ops = append(ops, adapter.NewCleanupOp(abs, strat, ptrs)) prunedFiles++ } } diff --git a/internal/cli/apply.go b/internal/cli/apply.go index 7b9e0209..070d09e8 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -430,8 +430,8 @@ func removalCounts(plan render.RenderPlan, s *state.Targets, userHome string, sc } for _, op := range res.Ops { // An orphan-cleanup op (adapter.OpCleanup, stamped by - // adapter.CleanupOp) carries the owned pointers to delete and writes - // nothing else. + // adapter.NewCleanupOp) carries the owned pointers to delete and + // writes nothing else. if op.Kind == adapter.OpCleanup { removedKeys += len(op.OwnedKeys) appliedOps-- // a removal, not an applied write diff --git a/internal/cli/iss155_test.go b/internal/cli/iss155_test.go index ef34dde3..23f6027b 100644 --- a/internal/cli/iss155_test.go +++ b/internal/cli/iss155_test.go @@ -604,7 +604,20 @@ func TestApplyDryRun_CleanupOpNotCountedToWrite(t *testing.T) { if !strings.Contains(dry, "1 removal op(s)") { t.Fatalf("Plan headline should carry the removal-op partition; got:\n%s", dry) } - if !strings.Contains(dry, "remove") { - t.Fatalf("the cleanup op should be listed with the remove label; got:\n%s", dry) + // Anchor the label check to the op's OWN line: the "Removals:" summary + // above also says "remove", so a bare Contains passed with the label wrong. + dest := filepath.Join(tmp, ".claude.json") + var opLine string + for _, line := range strings.Split(dry, "\n") { + if strings.HasSuffix(line, dest) { + opLine = line + break + } + } + if opLine == "" { + t.Fatalf("dry-run should list the cleanup op for %s; got:\n%s", dest, dry) + } + if !strings.Contains(opLine, " remove ") || strings.Contains(opLine, "write") || strings.Contains(opLine, "synced") { + t.Fatalf("the cleanup op's own line must carry the remove label, never write/synced; got %q in:\n%s", opLine, dry) } } diff --git a/internal/cli/planwalk.go b/internal/cli/planwalk.go index 91846165..39ba076e 100644 --- a/internal/cli/planwalk.go +++ b/internal/cli/planwalk.go @@ -29,8 +29,8 @@ type planItem struct { agent string // op is the plan op that produced the item. For an ORPHAN it is SYNTHESIZED - // from state — adapter.FileOp{Action: adapter.ActionDelete, Path, SourceID} — with Mode - // left 0 because the orphan removal path never reads it. + // from state — adapter.FileOp{Action: adapter.ActionDelete, Path, SourceID} + // — with Mode left 0 because the orphan removal path never reads it. op adapter.FileOp // ptr is the RFC-6901 pointer for a key item; "" for a whole-file item. diff --git a/internal/render/pipeline.go b/internal/render/pipeline.go index 8de20045..e18efd6b 100644 --- a/internal/render/pipeline.go +++ b/internal/render/pipeline.go @@ -327,7 +327,7 @@ func orphanCleanupOps(s *state.Targets, a adapter.Adapter, agent string, scope a if _, err := os.Stat(abs); err != nil { continue } - cleanup = append(cleanup, adapter.CleanupOp(abs, strat, ownedByPath[path])) + cleanup = append(cleanup, adapter.NewCleanupOp(abs, strat, ownedByPath[path])) } return cleanup } @@ -421,7 +421,10 @@ func applyPlan( // are exported and accept a RenderPlan that never went through Plan, so // the traversal check runs again here: a hand-built plan cannot smuggle // an unanchored ".." path past the guard Plan enforces. (A Plan-built - // plan already passed it, so this is a no-op for it.) + // plan already passed it, so this is a no-op for it.) Scope honesty: + // this covers the Apply/PreviewApply funnel only — cli/reconcile.go and + // cli/agent.go call an adapter's Apply directly with plan-derived or + // state-derived ops and never pass through here. for i := range res.Ops { if res.Ops[i].Action == adapter.ActionWrite && pathEscapes(res.Ops[i].Path) { return reports, written, unchanged, wouldChange, fmt.Errorf( diff --git a/internal/render/pipeline_cleanup_internal_test.go b/internal/render/pipeline_cleanup_internal_test.go index 363a20f6..b2a98e29 100644 --- a/internal/render/pipeline_cleanup_internal_test.go +++ b/internal/render/pipeline_cleanup_internal_test.go @@ -19,47 +19,65 @@ type keyMergeNoop struct{ *noop.Adapter } func (keyMergeNoop) KeyMergeStrategy() string { return "merge-json-keys" } // TestOrphanCleanupOps_StampsOpCleanup pins that the cleanup op the pipeline -// synthesizes for an emptied key-merge section is built by adapter.CleanupOp +// synthesizes for an emptied key-merge section is built by adapter.NewCleanupOp // and so carries Kind == adapter.OpCleanup: `apply` labels and counts the op // as a removal by reading that kind, not by sniffing the "{}"+OwnedKeys shape, // so a missing stamp would silently relabel every key removal as a write. The -// purge path (`agent disable --purge`) calls the same constructor. +// purge path (`agent disable --purge`) calls the same constructor. The second +// case pins the dest-exists gate: an already-deleted dest gets no op (rather +// than a freshly created empty "{}" file) — PruneStaleState drops its entry. func TestOrphanCleanupOps_StampsOpCleanup(t *testing.T) { testenv.RequireContainer(t) - userHome := t.TempDir() - dest := filepath.Join(userHome, ".claude.json") - // The dest must exist: orphanCleanupOps skips a dest that is already gone - // rather than creating an empty "{}" file. - if err := os.WriteFile(dest, []byte(`{"mcpServers":{"srv":{"command":"x"}}}`), 0o644); err != nil { - t.Fatal(err) + // orphaned returns state that owns /mcpServers/srv at /.claude.json + // while the adapter rendered NO op for that section this run — the source + // section emptied, so the owned pointer is orphaned. + orphaned := func(t *testing.T) (userHome, dest string, s *state.Targets) { + t.Helper() + userHome = t.TempDir() + dest = filepath.Join(userHome, ".claude.json") + s = state.New() + owned := state.Key{Agent: "claude", Scope: "user", Path: paths.HomeRelative(userHome, dest), Pointer: "/mcpServers/srv"} + s.Keys[owned] = state.KeyEntry{SHA256: "deadbeef"} + return userHome, dest, s } - s := state.New() - owned := state.Key{Agent: "claude", Scope: "user", Path: paths.HomeRelative(userHome, dest), Pointer: "/mcpServers/srv"} - s.Keys[owned] = state.KeyEntry{SHA256: "deadbeef"} - // The adapter rendered NO op for the mcpServers section this run — the - // source section emptied — so the owned pointer is orphaned. - got := orphanCleanupOps(s, keyMergeNoop{noop.New("claude")}, "claude", adapter.ScopeUser, "", userHome, nil) - if len(got) != 1 { - t.Fatalf("orphanCleanupOps = %+v, want exactly one cleanup op", got) - } - op := got[0] - if op.Kind != adapter.OpCleanup { - t.Errorf("Kind = %v, want OpCleanup — apply reads this kind to label and count the removal", op.Kind) - } - if op.Action != adapter.ActionWrite { - t.Errorf("Action = %v, want ActionWrite (the merge path performs the removal)", op.Action) - } - if op.Path != dest { - t.Errorf("Path = %q, want %q", op.Path, dest) - } - if string(op.Content) != "{}" { - t.Errorf("Content = %q, want \"{}\"", op.Content) - } - if op.MergeStrategy != "merge-json-keys" { - t.Errorf("MergeStrategy = %q, want the adapter's KeyMergeStrategy", op.MergeStrategy) - } - if len(op.OwnedKeys) != 1 || op.OwnedKeys[0] != "/mcpServers/srv" { - t.Errorf("OwnedKeys = %v, want [/mcpServers/srv]", op.OwnedKeys) - } + t.Run("dest present: one op, built by NewCleanupOp", func(t *testing.T) { + userHome, dest, s := orphaned(t) + if err := os.WriteFile(dest, []byte(`{"mcpServers":{"srv":{"command":"x"}}}`), 0o644); err != nil { + t.Fatal(err) + } + got := orphanCleanupOps(s, keyMergeNoop{noop.New("claude")}, "claude", adapter.ScopeUser, "", userHome, nil) + if len(got) != 1 { + t.Fatalf("orphanCleanupOps = %+v, want exactly one cleanup op", got) + } + op := got[0] + if op.Kind != adapter.OpCleanup { + t.Errorf("Kind = %v, want OpCleanup — apply reads this kind to label and count the removal", op.Kind) + } + if op.Action != adapter.ActionWrite { + t.Errorf("Action = %v, want ActionWrite (the merge path performs the removal)", op.Action) + } + if op.Path != dest { + t.Errorf("Path = %q, want %q", op.Path, dest) + } + if string(op.Content) != "{}" { + t.Errorf("Content = %q, want \"{}\"", op.Content) + } + if op.MergeStrategy != "merge-json-keys" { + t.Errorf("MergeStrategy = %q, want the adapter's KeyMergeStrategy", op.MergeStrategy) + } + if len(op.OwnedKeys) != 1 || op.OwnedKeys[0] != "/mcpServers/srv" { + t.Errorf("OwnedKeys = %v, want [/mcpServers/srv]", op.OwnedKeys) + } + }) + + t.Run("dest already gone: nothing to prune", func(t *testing.T) { + userHome, _, s := orphaned(t) + // No file at dest. Synthesizing "{}" here would CREATE an empty file + // where the user deleted one; the pipeline skips it instead. + got := orphanCleanupOps(s, keyMergeNoop{noop.New("claude")}, "claude", adapter.ScopeUser, "", userHome, nil) + if len(got) != 0 { + t.Fatalf("orphanCleanupOps = %+v, want no op for an absent dest", got) + } + }) } From 9576712d48a3cb93f86c190de7233efc5d834c4f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 21:51:43 +0000 Subject: [PATCH 5/6] =?UTF-8?q?test(adapter):=20close=20review-loop=20roun?= =?UTF-8?q?d=202=20on=20#249=20=E2=80=94=20guard=20reads=20the=20cleanup?= =?UTF-8?q?=20shape=20in=20both=20directions,=20headline=20pin=20(#230)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 (four lenses on cfa9d2e) was NIT-only; all taken: - The AST guard now keys on the cleanup SHAPE — an empty-object Content headed for a key-merge destination (it names OwnedKeys, or a MergeStrategy other than "replace") — so a legitimate whole-file "{}" write is not misdirected to NewCleanupOp; it also flags a literal that stamps Kind: OpCleanup by hand (the other way around the constructor) and a positional literal it cannot read, in the safe direction like its Skip sibling; "{}" matches ignoring all whitespace. The per-file scan is extracted so TestCleanupOpStaticGuardScan can pin the NewCleanupOp allow-window on snippets (inside: exempt; another function or another package's NewCleanupOp: flagged; widening it to the whole file fails a row). The matched-literal count is logged; the prose said ~47, the scan matches 45. - TestApplyDryRun_CleanupOpNotCountedToWrite checks the label with the dest path trimmed off (t.TempDir embeds the test's own name, which contains "Write"), and now runs the real apply to pin the headline ("removed: 1 key(s)", no "applied:") that removalCounts' appliedOps-- exists to produce — the one behaviour whose deletion the suite missed. - docs/architecture.md no longer says the kind "cannot be missed" (the guard is literal-only): a hand-rolled cleanup literal cannot ship unstamped. The MergeStrategy deferral sentence moves to the end of the key-merge block it belongs to. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- docs/architecture.md | 9 +- internal/adapter/adapter.go | 2 +- internal/adapter/cleanupop_guard_test.go | 364 +++++++++++++++++------ internal/cli/iss155_test.go | 16 +- 4 files changed, 299 insertions(+), 92 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 6774765f..5d1932bf 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -187,10 +187,8 @@ label and count it as a removal rather than sniffing `{}`+`OwnedKeys`. Cleanup ops are built by `adapter.NewCleanupOp` — the only producer of `OpCleanup`, called from `render.orphanCleanupOps` and from `agent disable --purge` — and `TestEveryCleanupLiteralUsesNewCleanupOp` fails any production `FileOp` literal -that hand-rolls the `{}` shape, so the kind cannot be missed at a synthesis -site. `MergeStrategy` stays a plain string: typing it would change the -published `Adapter` interface (`KeyMergeStrategy() string`) and is deferred to -[#250](https://github.com/spxrogers/agentsync/issues/250). +that hand-rolls the cleanup shape or stamps the kind by hand, so a hand-rolled +cleanup literal cannot ship unstamped. **Key-merge strategies and on-disk format.** `KeyMergeStrategy` / `FileOp.MergeStrategy` name how an adapter co-owns keys inside a shared config @@ -213,6 +211,9 @@ widening the accessor to a per-path strategy first. A central guard MCP+hook fixture through every registered adapter and pins `KeyMergeStrategy()` against the `MergeStrategy` stamped on every key-merge `FileOp` it emits, so the accessor can never silently drift from what an adapter actually writes. +`MergeStrategy` itself stays a plain string: typing it would change this +published interface (`KeyMergeStrategy() string`) and is deferred to +[#250](https://github.com/spxrogers/agentsync/issues/250). **Deep vs breadth-tier adapters.** The nine hand-written packages above are *deep* adapters — agent-specific, multi-component, often bidirectional. Beyond diff --git a/internal/adapter/adapter.go b/internal/adapter/adapter.go index 71e6a6b6..e0b17dc0 100644 --- a/internal/adapter/adapter.go +++ b/internal/adapter/adapter.go @@ -123,7 +123,7 @@ func (k OpKind) String() string { // the merge path performs. It is the only producer of OpCleanup: every site // that synthesizes a cleanup op must call it so the kind is never missed, and // TestEveryCleanupLiteralUsesNewCleanupOp fails any production FileOp literal -// that hand-rolls the "{}" shape instead. +// that hand-rolls the cleanup shape or stamps OpCleanup by hand instead. func NewCleanupOp(path, strategy string, owned []string) FileOp { return FileOp{ Action: ActionWrite, diff --git a/internal/adapter/cleanupop_guard_test.go b/internal/adapter/cleanupop_guard_test.go index 2178192f..c79db73d 100644 --- a/internal/adapter/cleanupop_guard_test.go +++ b/internal/adapter/cleanupop_guard_test.go @@ -13,14 +13,19 @@ import ( ) // TestEveryCleanupLiteralUsesNewCleanupOp is the reachability-independent guard -// behind NewCleanupOp's "only producer of OpCleanup" promise. apply identifies -// a cleanup op by Kind, never by shape, so a synthesis site that hand-rolls the -// shape — a FileOp literal whose Content is the empty object "{}" — and forgets -// the stamp would relabel every key removal as a write, and on the purge path -// (where nothing reads Kind) no test would notice. This test parses every -// production .go file under internal/ and fails on any FileOp composite literal -// whose Content is a []byte("{}") literal outside NewCleanupOp itself. Sibling -// of TestEverySkipLiteralSetsKind: same scan, same matcher self-test. +// behind NewCleanupOp's "only producer of OpCleanup" promise, in both +// directions. apply identifies a cleanup op by Kind, never by shape, so a +// synthesis site that hand-rolls the shape — a FileOp literal whose Content is +// the static empty object "{}" going into a key-merge destination (it names a +// MergeStrategy or OwnedKeys) — and forgets the stamp would relabel every key +// removal as a write, and on the purge path (where nothing reads Kind) no test +// would notice; a literal that stamps Kind: OpCleanup by hand is the other way +// around the constructor. This test parses every production .go file under +// internal/ and fails on either, anywhere outside NewCleanupOp's own body. A +// whole-file write of "{}" (no strategy, no owned keys) is not the cleanup +// shape and passes. Deliberately literal-only, like TestEverySkipLiteralSetsKind: +// Content built from a variable or a call is not a static shape, and the +// runtime tier for the pipeline path is TestApplyDryRun_CleanupOpNotCountedToWrite. func TestEveryCleanupLiteralUsesNewCleanupOp(t *testing.T) { root := moduleInternalDir(t) fset := token.NewFileSet() @@ -41,57 +46,170 @@ func TestEveryCleanupLiteralUsesNewCleanupOp(t *testing.T) { if perr != nil { return fmt.Errorf("parse %s: %w", path, perr) } - inPkgAdapter := f.Name.Name == "adapter" - // The one permitted site is the body of NewCleanupOp in package adapter. - var allowStart, allowEnd token.Pos - if inPkgAdapter { - for _, decl := range f.Decls { - if fn, ok := decl.(*ast.FuncDecl); ok && fn.Recv == nil && fn.Name.Name == "NewCleanupOp" { - allowStart, allowEnd = fn.Pos(), fn.End() - } - } - } - check := func(cl *ast.CompositeLit) { - total++ - inAllowed := allowStart.IsValid() && cl.Pos() >= allowStart && cl.Pos() < allowEnd - if hasEmptyObjectContent(cl) && !inAllowed { - offenders = append(offenders, posOf(fset, cl)) - } - } - ast.Inspect(f, func(n ast.Node) bool { - cl, ok := n.(*ast.CompositeLit) - if !ok { - return true - } - if isFileOpType(cl.Type, inPkgAdapter) { - check(cl) - return true - } - // `[]adapter.FileOp{ {...}, {...} }`: elided element types. - if at, ok := cl.Type.(*ast.ArrayType); ok && isFileOpType(at.Elt, inPkgAdapter) { - for _, el := range cl.Elts { - if ecl, ok := el.(*ast.CompositeLit); ok { - check(ecl) - } - } - } - return true - }) + n, off := scanCleanupLiterals(fset, f) + total += n + offenders = append(offenders, off...) return nil }) if err != nil { t.Fatalf("scanning internal/ for adapter.FileOp literals: %v", err) } - // Anti-vacuity: the tree holds ~47 production FileOp literals (41 adapter - // Render sites plus the pipeline's and cli's synthesized ops). A healthy - // floor rather than an exact count, so adding or removing a site doesn't - // churn this test. + // Anti-vacuity: the tree holds ~45 production FileOp literals (41 adapter + // Render sites plus the synthesized delete/orphan ops and NewCleanupOp's + // own). A healthy floor rather than an exact count, so adding or removing + // a site doesn't churn this test; the count is logged so a drift is visible + // under -v. + t.Logf("matched %d production FileOp literals", total) if total < 30 { - t.Fatalf("only matched %d adapter.FileOp literals — the matcher likely broke; expected ~47", total) + t.Fatalf("only matched %d adapter.FileOp literals — the matcher likely broke; expected ~45", total) } for _, o := range offenders { - t.Errorf("FileOp literal hand-rolls the \"{}\" cleanup shape without the OpCleanup stamp (call adapter.NewCleanupOp): %s", o) + t.Error(o) + } +} + +// scanCleanupLiterals walks one parsed production file and reports every FileOp +// composite literal — direct, or an elided element of []adapter.FileOp{…} — +// that bypasses NewCleanupOp: one that hand-rolls the cleanup shape, one that +// stamps Kind: OpCleanup itself, or a positional literal the matchers cannot +// read (flagged in the safe direction, like the Skip guard). Literals inside +// NewCleanupOp's own body, in package adapter, are the one exempt site. It also +// returns how many FileOp literals it matched, for the caller's anti-vacuity +// floor. +func scanCleanupLiterals(fset *token.FileSet, f *ast.File) (total int, offenders []string) { + inPkgAdapter := f.Name.Name == "adapter" + var allowStart, allowEnd token.Pos + if inPkgAdapter { + for _, decl := range f.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Recv == nil && fn.Name.Name == "NewCleanupOp" { + allowStart, allowEnd = fn.Pos(), fn.End() + } + } + } + check := func(cl *ast.CompositeLit) { + total++ + if allowStart.IsValid() && cl.Pos() >= allowStart && cl.Pos() < allowEnd { + return + } + switch { + case isPositionalLiteral(cl): + offenders = append(offenders, posOf(fset, cl)+": positional FileOp literal — use keyed fields so the cleanup-shape guard can read it") + case stampsOpCleanup(cl): + offenders = append(offenders, posOf(fset, cl)+": FileOp literal stamps Kind: OpCleanup by hand — call adapter.NewCleanupOp, its only producer") + case hasCleanupShape(cl): + offenders = append(offenders, posOf(fset, cl)+": FileOp literal hand-rolls the cleanup shape (an empty \"{}\" object into a key-merge destination) without the OpCleanup stamp — call adapter.NewCleanupOp") + } + } + ast.Inspect(f, func(n ast.Node) bool { + cl, ok := n.(*ast.CompositeLit) + if !ok { + return true + } + if isFileOpType(cl.Type, inPkgAdapter) { + check(cl) + return true + } + // `[]adapter.FileOp{ {...}, {...} }`: the element literals have an + // elided (nil) Type, so they are reached only through the slice here. + if at, ok := cl.Type.(*ast.ArrayType); ok && isFileOpType(at.Elt, inPkgAdapter) { + for _, el := range cl.Elts { + if ecl, ok := el.(*ast.CompositeLit); ok { + check(ecl) + } + } + } + return true + }) + return total, offenders +} + +// TestCleanupOpStaticGuardScan pins the scan on parsed snippets — above all the +// NewCleanupOp allow-window, which the matcher rows below cannot see: a literal +// inside NewCleanupOp's body is exempt, the same literal in any other function +// (or another package's NewCleanupOp) is not, and widening the window to the +// whole file would let the second case through unreported. +func TestCleanupOpStaticGuardScan(t *testing.T) { + tests := []struct { + name string + src string + wantTotal int + wantMsgs []string // one substring per expected offender, in order + }{ + { + name: "inside NewCleanupOp is the one allowed site", + src: `package adapter +func NewCleanupOp(p, s string, o []string) FileOp { return FileOp{Kind: OpCleanup, Content: []byte("{}"), MergeStrategy: s, OwnedKeys: o} }`, + wantTotal: 1, + }, + { + name: "the same shape in another adapter function is flagged", + src: `package adapter +func NewCleanupOp(p, s string, o []string) FileOp { return FileOp{Kind: OpCleanup, Content: []byte("{}"), MergeStrategy: s, OwnedKeys: o} } +func other(s string) FileOp { return FileOp{Content: []byte("{}"), MergeStrategy: s} }`, + wantTotal: 2, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, + { + name: "a NewCleanupOp in another package grants no window", + src: `package other +func NewCleanupOp(s string) adapter.FileOp { return adapter.FileOp{Content: []byte("{}"), MergeStrategy: s} }`, + wantTotal: 1, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, + { + name: "hand-stamped Kind outside the constructor is flagged", + src: `package render +func f(body []byte) adapter.FileOp { return adapter.FileOp{Kind: adapter.OpCleanup, Content: body} }`, + wantTotal: 1, + wantMsgs: []string{"stamps Kind: OpCleanup by hand"}, + }, + { + name: "elided slice element is reached and flagged", + src: `package cli +var ops = []adapter.FileOp{{Action: adapter.ActionWrite, Content: []byte(` + "`{}`" + `), OwnedKeys: ptrs}}`, + wantTotal: 1, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, + { + name: "a whole-file {} write is not the cleanup shape", + src: `package x +var op = adapter.FileOp{Action: adapter.ActionWrite, Path: p, Content: []byte("{}"), Mode: 0o644}`, + wantTotal: 1, + }, + { + name: "positional literal is flagged in the safe direction", + src: `package adapter +var op = FileOp{ActionWrite, OpRender, "p", nil, 0, "", "", nil}`, + wantTotal: 1, + wantMsgs: []string{"positional FileOp literal"}, + }, + { + name: "a Skip literal is not counted", + src: `package x +var s = adapter.Skip{Kind: adapter.SkipDropped}`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "snippet.go", tt.src, 0) + if err != nil { + t.Fatalf("parse snippet: %v", err) + } + total, offenders := scanCleanupLiterals(fset, f) + if total != tt.wantTotal { + t.Errorf("total = %d, want %d", total, tt.wantTotal) + } + if len(offenders) != len(tt.wantMsgs) { + t.Fatalf("offenders = %q, want %d: %q", offenders, len(tt.wantMsgs), tt.wantMsgs) + } + for i, want := range tt.wantMsgs { + if !strings.Contains(offenders[i], want) { + t.Errorf("offender %d = %q, want it to mention %q", i, offenders[i], want) + } + } + }) } } @@ -112,19 +230,29 @@ func TestCleanupOpStaticGuardMatchers(t *testing.T) { return cl } tests := []struct { - name string - src string - inPkgAdapter bool - wantFileOp bool - wantEmptyObj bool + name string + src string + inPkgAdapter bool + wantFileOp bool + wantEmptyObj bool + wantShape bool + wantStamp bool + wantPositional bool }{ - {name: "qualified, double-quoted {}", src: `adapter.FileOp{Content: []byte("{}")}`, wantFileOp: true, wantEmptyObj: true}, - {name: "qualified, raw-string {}", src: "adapter.FileOp{Content: []byte(`{}`)}", wantFileOp: true, wantEmptyObj: true}, - {name: "whitespace around {} is still the shape", src: `adapter.FileOp{Content: []byte(" {}\n")}`, wantFileOp: true, wantEmptyObj: true}, - {name: "populated content is not the shape", src: `adapter.FileOp{Content: []byte("{\"a\":1}")}`, wantFileOp: true, wantEmptyObj: false}, - {name: "non-literal content is not statically the shape", src: `adapter.FileOp{Content: body}`, wantFileOp: true, wantEmptyObj: false}, - {name: "no Content at all", src: `adapter.FileOp{Path: "x"}`, wantFileOp: true, wantEmptyObj: false}, - {name: "bare FileOp inside package adapter", src: `FileOp{Content: []byte("{}")}`, inPkgAdapter: true, wantFileOp: true, wantEmptyObj: true}, + {name: "{} into a key-merge strategy is the shape", src: `adapter.FileOp{Content: []byte("{}"), MergeStrategy: strat}`, wantFileOp: true, wantEmptyObj: true, wantShape: true}, + {name: "{} with owned keys is the shape", src: `adapter.FileOp{Content: []byte("{}"), OwnedKeys: ptrs}`, wantFileOp: true, wantEmptyObj: true, wantShape: true}, + {name: "raw-string {} counts", src: "adapter.FileOp{Content: []byte(`{}`), OwnedKeys: ptrs}", wantFileOp: true, wantEmptyObj: true, wantShape: true}, + {name: "interior and edge whitespace do not hide {}", src: `adapter.FileOp{Content: []byte(" {\n }\n"), OwnedKeys: ptrs}`, wantFileOp: true, wantEmptyObj: true, wantShape: true}, + {name: "{} as a whole-file write is not the shape", src: `adapter.FileOp{Path: p, Content: []byte("{}"), Mode: 0o644}`, wantFileOp: true, wantEmptyObj: true}, + {name: "{} with an explicit replace strategy is not the shape", src: `adapter.FileOp{Content: []byte("{}"), MergeStrategy: "replace"}`, wantFileOp: true, wantEmptyObj: true}, + {name: "populated content is not the shape", src: `adapter.FileOp{Content: []byte("{\"a\":1}"), MergeStrategy: strat}`, wantFileOp: true}, + {name: "non-literal content is not statically the shape", src: `adapter.FileOp{Content: body, MergeStrategy: strat}`, wantFileOp: true}, + {name: "hand-stamped OpCleanup", src: `adapter.FileOp{Kind: adapter.OpCleanup, Content: body}`, wantFileOp: true, wantStamp: true}, + {name: "bare OpCleanup inside package adapter", src: `FileOp{Kind: OpCleanup}`, inPkgAdapter: true, wantFileOp: true, wantStamp: true}, + {name: "OpRender is not a hand stamp", src: `adapter.FileOp{Kind: adapter.OpRender}`, wantFileOp: true}, + {name: "positional literal", src: `adapter.FileOp{adapter.ActionWrite, adapter.OpRender, "p"}`, wantFileOp: true, wantPositional: true}, + {name: "empty literal is keyed enough", src: `adapter.FileOp{}`, wantFileOp: true}, + {name: "bare FileOp inside package adapter", src: `FileOp{Content: []byte("{}"), OwnedKeys: o}`, inPkgAdapter: true, wantFileOp: true, wantEmptyObj: true, wantShape: true}, {name: "bare FileOp outside package adapter is some other type", src: `FileOp{Content: []byte("{}")}`, wantFileOp: false}, {name: "a Skip literal is not a FileOp", src: `adapter.Skip{Kind: adapter.SkipDropped}`, wantFileOp: false}, } @@ -140,6 +268,15 @@ func TestCleanupOpStaticGuardMatchers(t *testing.T) { if got := hasEmptyObjectContent(cl); got != tt.wantEmptyObj { t.Errorf("hasEmptyObjectContent = %v, want %v", got, tt.wantEmptyObj) } + if got := hasCleanupShape(cl); got != tt.wantShape { + t.Errorf("hasCleanupShape = %v, want %v", got, tt.wantShape) + } + if got := stampsOpCleanup(cl); got != tt.wantStamp { + t.Errorf("stampsOpCleanup = %v, want %v", got, tt.wantStamp) + } + if got := isPositionalLiteral(cl); got != tt.wantPositional { + t.Errorf("isPositionalLiteral = %v, want %v", got, tt.wantPositional) + } }) } } @@ -158,37 +295,92 @@ func isFileOpType(e ast.Expr, inPkgAdapter bool) bool { return false } -// hasEmptyObjectContent reports whether a FileOp literal sets Content to the -// static empty object — `[]byte("{}")` or `[]byte(`{}`)`, whitespace-trimmed -// like the writer sees it. Content built from a variable or a call is not a -// static shape and is not matched; the guard is deliberately literal-only, -// like its Skip sibling. -func hasEmptyObjectContent(cl *ast.CompositeLit) bool { +// keyedField returns the value of the named keyed field in a composite literal, +// or nil when the literal does not set it. +func keyedField(cl *ast.CompositeLit, name string) ast.Expr { for _, el := range cl.Elts { kv, ok := el.(*ast.KeyValueExpr) if !ok { continue } - if id, ok := kv.Key.(*ast.Ident); !ok || id.Name != "Content" { - continue - } - call, ok := kv.Value.(*ast.CallExpr) - if !ok || len(call.Args) != 1 { - return false + if id, ok := kv.Key.(*ast.Ident); ok && id.Name == name { + return kv.Value } - at, ok := call.Fun.(*ast.ArrayType) - if !ok || at.Len != nil { - return false - } - if elt, ok := at.Elt.(*ast.Ident); !ok || elt.Name != "byte" { - return false - } - bl, ok := call.Args[0].(*ast.BasicLit) - if !ok || bl.Kind != token.STRING { - return false + } + return nil +} + +// isPositionalLiteral reports whether a non-empty composite literal has any +// unkeyed element. The matchers read keyed fields only, so such a literal is +// flagged rather than silently passed; go vet already rejects the positional +// form for the imported adapter.FileOp, so this only bites inside package +// adapter, where the codebase convention is keyed fields anyway. +func isPositionalLiteral(cl *ast.CompositeLit) bool { + for _, el := range cl.Elts { + if _, ok := el.(*ast.KeyValueExpr); !ok { + return true } - s, err := strconv.Unquote(bl.Value) - return err == nil && strings.TrimSpace(s) == "{}" } return false } + +// stampsOpCleanup reports whether a FileOp literal sets Kind to OpCleanup +// itself — `adapter.OpCleanup`, or bare `OpCleanup` — which only NewCleanupOp +// may do. +func stampsOpCleanup(cl *ast.CompositeLit) bool { + switch v := keyedField(cl, "Kind").(type) { + case *ast.SelectorExpr: + x, ok := v.X.(*ast.Ident) + return ok && x.Name == "adapter" && v.Sel.Name == "OpCleanup" + case *ast.Ident: + return v.Name == "OpCleanup" + } + return false +} + +// hasCleanupShape reports whether a FileOp literal is statically the cleanup +// shape: an empty-object Content headed for a key-merge destination, i.e. it +// also names a MergeStrategy other than the literal "replace", or OwnedKeys. A +// whole-file write of "{}" names neither and is not the shape. +func hasCleanupShape(cl *ast.CompositeLit) bool { + if !hasEmptyObjectContent(cl) { + return false + } + if keyedField(cl, "OwnedKeys") != nil { + return true + } + strat := keyedField(cl, "MergeStrategy") + if strat == nil { + return false + } + if bl, ok := strat.(*ast.BasicLit); ok && bl.Kind == token.STRING { + s, err := strconv.Unquote(bl.Value) + return err != nil || s != "replace" + } + return true +} + +// hasEmptyObjectContent reports whether a FileOp literal sets Content to the +// static empty object — `[]byte("{}")` or `[]byte(`{}`)`, ignoring all +// whitespace, like the merge path does. Content built from a variable or a call +// is not a static shape and is not matched; the guard is deliberately +// literal-only, like its Skip sibling. +func hasEmptyObjectContent(cl *ast.CompositeLit) bool { + call, ok := keyedField(cl, "Content").(*ast.CallExpr) + if !ok || len(call.Args) != 1 { + return false + } + at, ok := call.Fun.(*ast.ArrayType) + if !ok || at.Len != nil { + return false + } + if elt, ok := at.Elt.(*ast.Ident); !ok || elt.Name != "byte" { + return false + } + bl, ok := call.Args[0].(*ast.BasicLit) + if !ok || bl.Kind != token.STRING { + return false + } + s, err := strconv.Unquote(bl.Value) + return err == nil && strings.Join(strings.Fields(s), "") == "{}" +} diff --git a/internal/cli/iss155_test.go b/internal/cli/iss155_test.go index 23f6027b..ff3f42ac 100644 --- a/internal/cli/iss155_test.go +++ b/internal/cli/iss155_test.go @@ -617,7 +617,21 @@ func TestApplyDryRun_CleanupOpNotCountedToWrite(t *testing.T) { if opLine == "" { t.Fatalf("dry-run should list the cleanup op for %s; got:\n%s", dest, dry) } - if !strings.Contains(opLine, " remove ") || strings.Contains(opLine, "write") || strings.Contains(opLine, "synced") { + // Check the label part only: t.TempDir embeds this test's name in dest, + // and that name contains "Write". + label := strings.TrimSuffix(opLine, dest) + if !strings.Contains(label, " remove ") || strings.Contains(label, "write") || strings.Contains(label, "synced") { t.Fatalf("the cleanup op's own line must carry the remove label, never write/synced; got %q in:\n%s", opLine, dry) } + // The real apply reports the same op as a removal ONLY: "removed: 1 + // key(s)" with no "applied:" partition, which is what removalCounts' + // appliedOps-- exists to produce (without it the headline reads + // "applied: 1 ops, removed: 1 key(s)" for a run that wrote nothing). + out, err := runCLI(t, env, "apply") + if err != nil { + t.Fatalf("apply: %v\n%s", err, out) + } + if !strings.Contains(out, "removed: 1 key(s)") || strings.Contains(out, "applied:") { + t.Fatalf("real apply should report the cleanup op as a removal only; got:\n%s", out) + } } From a886a45a2fb2ce6f0adb3ae94c9e8882b16b380b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 22:16:12 +0000 Subject: [PATCH 6/6] =?UTF-8?q?test(adapter):=20close=20review-loop=20roun?= =?UTF-8?q?d=203=20on=20#249=20=E2=80=94=20container-generic=20elision=20w?= =?UTF-8?q?alk,=20headline-line=20pin=20(#230)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 (four lenses on 9576712, targeted at the round-2 changes) was NIT-only; all taken: - The guard's elided-element walk is container-generic: a type-elided FileOp element is reached through a slice, array or map — nested containers included — and an element that spells its own type is left to the plain walk, so nothing is counted twice. New scan rows pin the typed-element-counted-once case, a map value, a nested slice, and a method named NewCleanupOp (which grants no allow-window; that condition was the one unpinned by round 3's mutation sweep). - The shape message names the escape hatch: a whole-file write of "{}" names neither MergeStrategy nor OwnedKeys and is not flagged. The guard docs also say post-construction assignment is out of scope, and why an unquotable literal leans the way it does. - TestApplyDryRun_CleanupOpNotCountedToWrite's real-apply pin is anchored to the "removed: 1 key(s)" line itself — the same fix round 2 made to the label check — so a future zero-ops hint elsewhere in the output cannot fail it, and "applied: 0 ops, removed: 1 key(s)" still does. - Docs: a hand-rolled cleanup literal cannot ship "unstamped or hand-stamped"; the moved MergeStrategy sentence names "the published Adapter interface" again; the CHANGELOG says "hand-rolled cleanup literals". #250 carries a note to teach the guard the typed replace constant. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01M4VNyoCuGXx7pYxNfLVbFG --- CHANGELOG.md | 2 +- docs/architecture.md | 6 +- internal/adapter/cleanupop_guard_test.go | 99 +++++++++++++++++++----- internal/cli/iss155_test.go | 14 +++- 4 files changed, 96 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5443e9e..d5a84b0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -250,7 +250,7 @@ source layout, CLI surface, and state schema are stabilizing but may still chang are gone. Orphan-cleanup ops — the empty key-merge write that prunes an emptied section's owned keys — are stamped `OpCleanup` at synthesis (by the single constructor `adapter.NewCleanupOp`, guarded by an AST test against - hand-rolled `{}` literals) instead of being detected by three copies of a + hand-rolled cleanup literals) instead of being detected by three copies of a `{}`+`OwnedKeys` shape sniff. No user-visible behaviour changes: the `apply --dry-run` labels, the `removed: N key(s), M file(s)` headline and every `--json` payload are byte-identical (`FileOp` is never serialized). diff --git a/docs/architecture.md b/docs/architecture.md index 5d1932bf..e0fd8901 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -188,7 +188,7 @@ ops are built by `adapter.NewCleanupOp` — the only producer of `OpCleanup`, called from `render.orphanCleanupOps` and from `agent disable --purge` — and `TestEveryCleanupLiteralUsesNewCleanupOp` fails any production `FileOp` literal that hand-rolls the cleanup shape or stamps the kind by hand, so a hand-rolled -cleanup literal cannot ship unstamped. +cleanup literal cannot ship unstamped or hand-stamped. **Key-merge strategies and on-disk format.** `KeyMergeStrategy` / `FileOp.MergeStrategy` name how an adapter co-owns keys inside a shared config @@ -211,8 +211,8 @@ widening the accessor to a per-path strategy first. A central guard MCP+hook fixture through every registered adapter and pins `KeyMergeStrategy()` against the `MergeStrategy` stamped on every key-merge `FileOp` it emits, so the accessor can never silently drift from what an adapter actually writes. -`MergeStrategy` itself stays a plain string: typing it would change this -published interface (`KeyMergeStrategy() string`) and is deferred to +`MergeStrategy` itself stays a plain string: typing it would change the +published `Adapter` interface (`KeyMergeStrategy() string`) and is deferred to [#250](https://github.com/spxrogers/agentsync/issues/250). **Deep vs breadth-tier adapters.** The nine hand-written packages above are diff --git a/internal/adapter/cleanupop_guard_test.go b/internal/adapter/cleanupop_guard_test.go index c79db73d..5a707b4c 100644 --- a/internal/adapter/cleanupop_guard_test.go +++ b/internal/adapter/cleanupop_guard_test.go @@ -24,8 +24,9 @@ import ( // internal/ and fails on either, anywhere outside NewCleanupOp's own body. A // whole-file write of "{}" (no strategy, no owned keys) is not the cleanup // shape and passes. Deliberately literal-only, like TestEverySkipLiteralSetsKind: -// Content built from a variable or a call is not a static shape, and the -// runtime tier for the pipeline path is TestApplyDryRun_CleanupOpNotCountedToWrite. +// Content built from a variable or a call, or assigned after construction, is +// not a static shape, and the runtime tier for the pipeline path is +// TestApplyDryRun_CleanupOpNotCountedToWrite. func TestEveryCleanupLiteralUsesNewCleanupOp(t *testing.T) { root := moduleInternalDir(t) fset := token.NewFileSet() @@ -70,8 +71,8 @@ func TestEveryCleanupLiteralUsesNewCleanupOp(t *testing.T) { } // scanCleanupLiterals walks one parsed production file and reports every FileOp -// composite literal — direct, or an elided element of []adapter.FileOp{…} — -// that bypasses NewCleanupOp: one that hand-rolls the cleanup shape, one that +// composite literal — direct, or a type-elided element of a slice, array or map +// of FileOp (nested containers included) — that bypasses NewCleanupOp: one that hand-rolls the cleanup shape, one that // stamps Kind: OpCleanup itself, or a positional literal the matchers cannot // read (flagged in the safe direction, like the Skip guard). Literals inside // NewCleanupOp's own body, in package adapter, are the one exempt site. It also @@ -98,7 +99,29 @@ func scanCleanupLiterals(fset *token.FileSet, f *ast.File) (total int, offenders case stampsOpCleanup(cl): offenders = append(offenders, posOf(fset, cl)+": FileOp literal stamps Kind: OpCleanup by hand — call adapter.NewCleanupOp, its only producer") case hasCleanupShape(cl): - offenders = append(offenders, posOf(fset, cl)+": FileOp literal hand-rolls the cleanup shape (an empty \"{}\" object into a key-merge destination) without the OpCleanup stamp — call adapter.NewCleanupOp") + offenders = append(offenders, posOf(fset, cl)+": FileOp literal hand-rolls the cleanup shape (an empty \"{}\" object into a key-merge destination) without the OpCleanup stamp — call adapter.NewCleanupOp; a whole-file write of \"{}\" names neither MergeStrategy nor OwnedKeys and is not flagged") + } + } + // checkElided walks a container literal whose element literals elide + // their type — `[]adapter.FileOp{{…}}`, `map[string]adapter.FileOp{"k": {…}}`, + // or a nesting of those — and checks each FileOp element it reaches. An + // element that spells its own type is left to the plain Inspect branch + // below, so nothing is counted twice. + var checkElided func(cl *ast.CompositeLit, elt ast.Expr) + checkElided = func(cl *ast.CompositeLit, elt ast.Expr) { + for _, el := range cl.Elts { + if kv, ok := el.(*ast.KeyValueExpr); ok { + el = kv.Value + } + ecl, ok := el.(*ast.CompositeLit) + if !ok || ecl.Type != nil { + continue + } + if isFileOpType(elt, inPkgAdapter) { + check(ecl) + } else if inner := containerElem(elt); inner != nil { + checkElided(ecl, inner) + } } } ast.Inspect(f, func(n ast.Node) bool { @@ -108,22 +131,26 @@ func scanCleanupLiterals(fset *token.FileSet, f *ast.File) (total int, offenders } if isFileOpType(cl.Type, inPkgAdapter) { check(cl) - return true - } - // `[]adapter.FileOp{ {...}, {...} }`: the element literals have an - // elided (nil) Type, so they are reached only through the slice here. - if at, ok := cl.Type.(*ast.ArrayType); ok && isFileOpType(at.Elt, inPkgAdapter) { - for _, el := range cl.Elts { - if ecl, ok := el.(*ast.CompositeLit); ok { - check(ecl) - } - } + } else if elt := containerElem(cl.Type); elt != nil { + checkElided(cl, elt) } return true }) return total, offenders } +// containerElem returns the element type of a slice or array, or the value +// type of a map, or nil for any other type expression. +func containerElem(t ast.Expr) ast.Expr { + switch c := t.(type) { + case *ast.ArrayType: + return c.Elt + case *ast.MapType: + return c.Value + } + return nil +} + // TestCleanupOpStaticGuardScan pins the scan on parsed snippets — above all the // NewCleanupOp allow-window, which the matcher rows below cannot see: a literal // inside NewCleanupOp's body is exempt, the same literal in any other function @@ -171,6 +198,35 @@ var ops = []adapter.FileOp{{Action: adapter.ActionWrite, Content: []byte(` + "`{ wantTotal: 1, wantMsgs: []string{"hand-rolls the cleanup shape"}, }, + { + name: "an explicitly typed slice element is counted once", + src: `package cli +var ops = []adapter.FileOp{adapter.FileOp{Content: []byte("{}"), OwnedKeys: ptrs}}`, + wantTotal: 1, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, + { + name: "elided map value is reached", + src: `package cli +var byPath = map[string]adapter.FileOp{"a": {Content: []byte("{}"), OwnedKeys: ptrs}}`, + wantTotal: 1, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, + { + name: "elided element of a nested slice is reached", + src: `package cli +var batches = [][]adapter.FileOp{{{Content: []byte("{}"), MergeStrategy: s}}}`, + wantTotal: 1, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, + { + name: "a method named NewCleanupOp grants no window", + src: `package adapter +type T struct{} +func (T) NewCleanupOp(s string) FileOp { return FileOp{Content: []byte("{}"), MergeStrategy: s} }`, + wantTotal: 1, + wantMsgs: []string{"hand-rolls the cleanup shape"}, + }, { name: "a whole-file {} write is not the cleanup shape", src: `package x @@ -250,7 +306,7 @@ func TestCleanupOpStaticGuardMatchers(t *testing.T) { {name: "hand-stamped OpCleanup", src: `adapter.FileOp{Kind: adapter.OpCleanup, Content: body}`, wantFileOp: true, wantStamp: true}, {name: "bare OpCleanup inside package adapter", src: `FileOp{Kind: OpCleanup}`, inPkgAdapter: true, wantFileOp: true, wantStamp: true}, {name: "OpRender is not a hand stamp", src: `adapter.FileOp{Kind: adapter.OpRender}`, wantFileOp: true}, - {name: "positional literal", src: `adapter.FileOp{adapter.ActionWrite, adapter.OpRender, "p"}`, wantFileOp: true, wantPositional: true}, + {name: "positional literal", src: `adapter.FileOp{adapter.ActionWrite, adapter.OpRender, "p", nil, 0, "", "", nil}`, wantFileOp: true, wantPositional: true}, {name: "empty literal is keyed enough", src: `adapter.FileOp{}`, wantFileOp: true}, {name: "bare FileOp inside package adapter", src: `FileOp{Content: []byte("{}"), OwnedKeys: o}`, inPkgAdapter: true, wantFileOp: true, wantEmptyObj: true, wantShape: true}, {name: "bare FileOp outside package adapter is some other type", src: `FileOp{Content: []byte("{}")}`, wantFileOp: false}, @@ -354,6 +410,9 @@ func hasCleanupShape(cl *ast.CompositeLit) bool { return false } if bl, ok := strat.(*ast.BasicLit); ok && bl.Kind == token.STRING { + // A parser-produced string literal always unquotes; if one ever did + // not, it is treated like a non-literal strategy — not provably + // "replace", so still the shape. s, err := strconv.Unquote(bl.Value) return err != nil || s != "replace" } @@ -362,9 +421,11 @@ func hasCleanupShape(cl *ast.CompositeLit) bool { // hasEmptyObjectContent reports whether a FileOp literal sets Content to the // static empty object — `[]byte("{}")` or `[]byte(`{}`)`, ignoring all -// whitespace, like the merge path does. Content built from a variable or a call -// is not a static shape and is not matched; the guard is deliberately -// literal-only, like its Skip sibling. +// whitespace, like the merge path does. Content built from a variable or a +// call, or assigned after construction, is not a static shape and is not +// matched (nor is a string literal that fails to unquote, which the parser +// never produces); the guard is deliberately literal-only, like its Skip +// sibling. func hasEmptyObjectContent(cl *ast.CompositeLit) bool { call, ok := keyedField(cl, "Content").(*ast.CallExpr) if !ok || len(call.Args) != 1 { diff --git a/internal/cli/iss155_test.go b/internal/cli/iss155_test.go index ff3f42ac..400926c7 100644 --- a/internal/cli/iss155_test.go +++ b/internal/cli/iss155_test.go @@ -631,7 +631,17 @@ func TestApplyDryRun_CleanupOpNotCountedToWrite(t *testing.T) { if err != nil { t.Fatalf("apply: %v\n%s", err, out) } - if !strings.Contains(out, "removed: 1 key(s)") || strings.Contains(out, "applied:") { - t.Fatalf("real apply should report the cleanup op as a removal only; got:\n%s", out) + var headline string + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "removed: 1 key(s)") { + headline = line + break + } + } + if headline == "" { + t.Fatalf("real apply should report the cleanup op under \"removed: 1 key(s)\"; got:\n%s", out) + } + if strings.Contains(headline, "applied") { + t.Fatalf("the removal-only headline must carry no applied partition; got %q in:\n%s", headline, out) } }