diff --git a/cmd/agentsync/link.go b/cmd/agentsync/link.go index b0cbc5b..715d24c 100644 --- a/cmd/agentsync/link.go +++ b/cmd/agentsync/link.go @@ -257,6 +257,11 @@ func runLink(args []string, stdout, stderr io.Writer) int { } if run.apply { run.done, run.errs = plan.Apply(repo, st) + // Save clears Dirty only on success, so a dirty document means the + // cleared markers are still on disk. + if st.Dirty() { + run.cleared, run.uncleared = nil, run.cleared + } } emitRepoChoice(reportStream(*asJSON, stdout, stderr), repo, repoFrom) @@ -298,9 +303,11 @@ type linkRun struct { statePath string // cleared and unmatched are the --undecline selectors that found a marker - // and the ones that did not. Both are reported. + // and the ones that did not. Both are reported. uncleared holds the + // cleared ones instead when the state document was not saved. cleared []state.Key unmatched []state.Key + uncleared []state.Key } // exitCode maps the run onto the CLI contract. @@ -588,6 +595,10 @@ func (r linkRun) emitSummary(w io.Writer) { n, plural(n, " would", "s would"), r.statePath) } } + if n := len(r.uncleared); n > 0 { + fmt.Fprintf(w, "%d declined marker%s NOT cleared (see stderr); %s still declined.\n", + n, plural(n, " was", "s were"), plural(n, "that agent is", "those agents are")) + } if n := len(r.unmatched); n > 0 { fmt.Fprintf(w, "%d --%s selector%s named no declined marker (%s); nothing was declined for %s, which is the state you asked for. If one of those is a typo, the agent it meant is still declined and is listed above.\n", n, linker.UndeclineFlag, plural(n, "", "s"), diff --git a/cmd/agentsync/link_test.go b/cmd/agentsync/link_test.go index 50d612a..32bb123 100644 --- a/cmd/agentsync/link_test.go +++ b/cmd/agentsync/link_test.go @@ -10,6 +10,7 @@ import ( "github.com/agentic-tooling/agentsync/internal/scan" "github.com/agentic-tooling/agentsync/internal/state" + "github.com/agentic-tooling/agentsync/internal/testfs" ) // --- fixtures --------------------------------------------------------------- @@ -624,6 +625,9 @@ func TestRunLink_UndeclineClearsTheMarkerAndLinks(t *testing.T) { t.Fatalf("exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", code, exitOK, stdout, stderr) } assertIsLink(t, projectAgentPath(base, "proj", "shared"), target) + if !strings.Contains(stdout, "1 declined marker was cleared in") || strings.Contains(stdout, "NOT cleared") { + t.Errorf("a saved undecline is not reported as cleared:\n%s", stdout) + } st, err := state.Load(mustRepo(t, repoRoot)) if err != nil { @@ -758,3 +762,122 @@ func declineInState(t *testing.T, repoRoot, project, name string) { t.Fatalf("Save: %v", err) } } + +// unsaveableFixture is linkFixture with the repo placed where its state file's +// path just fits the platform limit, so Load reads it as absent but Save's +// longer temporary name cannot be created. Project "p" and agent "a" keep every +// other repo path inside the limit. +func unsaveableFixture(t *testing.T) (base, repoRoot string) { + t.Helper() + + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("resolving the temp dir: %v", err) + } + repoRoot = testfs.DirWithRoom(t, base, len(string(filepath.Separator)+state.FileName)) + mustMkdirT(t, filepath.Join(repoRoot, "agents")) + mustMkdirT(t, filepath.Join(repoRoot, "project-agents")) + t.Setenv("HOME", base) + return base, repoRoot +} + +func TestRunLink_StateSaveFailureCountsTheLinkAsNotRecorded(t *testing.T) { + base, repoRoot := unsaveableFixture(t) + const body = "canonical shared agent\n" + target := globalAgent(t, repoRoot, "a", body) + path := projectAgentPath(base, "p", "a") + mustWriteT(t, path, body) + inProject(t, repoRoot, filepath.Join(base, "p")) + + code, stdout, stderr := runCLI(t, "link", "--apply") + if code != exitFailure { + t.Fatalf("exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", code, exitFailure, stdout, stderr) + } + if !strings.Contains(stdout, "1 link was created but NOT recorded") { + t.Errorf("the report does not count the link as unrecorded:\n%s", stdout) + } + if strings.Contains(stdout, " recorded in ") { + t.Errorf("the report claims a record the save never wrote:\n%s", stdout) + } + if !strings.Contains(stderr, ".agentsync-state-") { + t.Errorf("stderr does not report the failed save:\n%s", stderr) + } + assertIsLink(t, path, target) +} + +func TestRunLink_StateSaveFailureJSONRecordsNothing(t *testing.T) { + base, repoRoot := unsaveableFixture(t) + const body = "canonical shared agent\n" + globalAgent(t, repoRoot, "a", body) + mustWriteT(t, projectAgentPath(base, "p", "a"), body) + inProject(t, repoRoot, filepath.Join(base, "p")) + + code, stdout, stderr := runCLI(t, "link", "--apply", "--json") + if code != exitFailure { + t.Fatalf("exit = %d, want %d\nstderr:\n%s", code, exitFailure, stderr) + } + var rep struct { + Recorded []any `json:"recorded"` + Written []string `json:"written"` + } + if err := json.Unmarshal([]byte(stdout), &rep); err != nil { + t.Fatalf("the JSON report does not parse: %v\n%s", err, stdout) + } + if len(rep.Written) != 1 { + t.Errorf("written = %v, want the one link", rep.Written) + } + if rep.Recorded == nil || len(rep.Recorded) != 0 { + t.Errorf("recorded = %v, want [] after a failed save", rep.Recorded) + } +} + +func TestRunLink_StateSaveFailureLeavesTheMarkerDeclined(t *testing.T) { + base, repoRoot := unsaveableFixture(t) + globalAgent(t, repoRoot, "a", "canonical shared agent\n") + mustMkdirT(t, filepath.Dir(projectAgentPath(base, "p", "a"))) + // Written directly: the state file's own path fits, only Save's temporary + // name does not. + repo := mustRepo(t, repoRoot) + st := state.New() + key := state.Key{Project: "p", Name: "a"} + if err := st.Decline(key, "link gone, nothing in its place", time.Now()); err != nil { + t.Fatalf("Decline: %v", err) + } + b, err := st.Marshal() + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if err := os.WriteFile(state.Path(repo), b, 0o600); err != nil { + t.Fatalf("writing the state file: %v", err) + } + inProject(t, repoRoot, filepath.Join(base, "p")) + args := []string{"link", "--agent", "a", "--undecline=p/a", "--apply"} + + code, stdout, stderr := runCLI(t, args...) + if code != exitFailure { + t.Fatalf("exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", code, exitFailure, stdout, stderr) + } + if strings.Contains(stdout, "was cleared") || strings.Contains(stdout, "— cleared") { + t.Errorf("the report claims a cleared marker the save never wrote:\n%s", stdout) + } + if !strings.Contains(stdout, "1 declined marker was NOT cleared") { + t.Errorf("the report does not say the marker was not cleared:\n%s\nstderr:\n%s", stdout, stderr) + } + if got, err := state.Load(repo); err != nil || !got.IsDeclined(key) { + t.Errorf("the marker is gone from disk (load error %v)", err) + } + + code, stdout, _ = runCLI(t, append(args, "--json")...) + if code != exitFailure { + t.Fatalf("--json exit = %d, want %d", code, exitFailure) + } + var rep struct { + Undeclined []string `json:"undeclined"` + } + if err := json.Unmarshal([]byte(stdout), &rep); err != nil { + t.Fatalf("the JSON report does not parse: %v\n%s", err, stdout) + } + if rep.Undeclined == nil || len(rep.Undeclined) != 0 { + t.Errorf("undeclined = %v, want [] after a failed save", rep.Undeclined) + } +} diff --git a/cmd/agentsync/reconcile.go b/cmd/agentsync/reconcile.go index 3912390..45e72e6 100644 --- a/cmd/agentsync/reconcile.go +++ b/cmd/agentsync/reconcile.go @@ -325,13 +325,14 @@ func (r reconcileRun) incomplete() bool { // reconcileReport is the --json form. It carries the whole plan, not just the // writes: what was refused and why is the part a user cannot reconstruct. type reconcileReport struct { - Apply bool `json:"apply"` - Repo string `json:"repo"` - Root string `json:"root"` - Adopt bool `json:"adopt"` - Prune bool `json:"prune"` - Rekey []string `json:"rekey"` - DropKeys []string `json:"drop_capture"` + Apply bool `json:"apply"` + Repo string `json:"repo"` + Root string `json:"root"` + Adopt bool `json:"adopt"` + Prune bool `json:"prune"` + PruneAdopted bool `json:"prune_adopted"` + Rekey []string `json:"rekey"` + DropKeys []string `json:"drop_capture"` Items []reconcile.Item `json:"items"` Counts map[string]int `json:"counts"` @@ -363,13 +364,14 @@ type reconcileReport struct { func (r reconcileRun) emitJSON(w io.Writer) error { rep := reconcileReport{ - Apply: r.apply, - Repo: r.repoRoot, - Root: r.root, - Adopt: r.opts.Adopt, - Prune: r.opts.Prune, - Rekey: moveStrings(r.opts.Rekeys), - DropKeys: keyStrings(r.opts.Drops), + Apply: r.apply, + Repo: r.repoRoot, + Root: r.root, + Adopt: r.opts.Adopt, + Prune: r.opts.Prune, + PruneAdopted: r.opts.PruneAdopted, + Rekey: moveStrings(r.opts.Rekeys), + DropKeys: keyStrings(r.opts.Drops), Items: r.plan.Items, Counts: make(map[string]int), diff --git a/cmd/agentsync/reconcile_test.go b/cmd/agentsync/reconcile_test.go index 462f46c..9d9f7bb 100644 --- a/cmd/agentsync/reconcile_test.go +++ b/cmd/agentsync/reconcile_test.go @@ -741,3 +741,54 @@ func TestRunReconcile_AMovedProjectWritesNoMarkers(t *testing.T) { t.Errorf("records = %d, want both kept — dropping them makes the links at the new path unownable", n) } } + +func TestRunReconcile_StateSaveFailureReportsNoAdoption(t *testing.T) { + base, repoRoot := unsaveableFixture(t) + target := globalAgent(t, repoRoot, "a", "body\n") + path := projectAgentPath(base, "p", "a") + mustMkdirT(t, filepath.Dir(path)) + if err := os.Symlink(target, path); err != nil { + t.Fatalf("symlink: %v", err) + } + inProject(t, repoRoot, filepath.Join(base, "p")) + + code, stdout, stderr := runCLI(t, "reconcile", "--"+reconcile.AdoptFlag, "--apply") + if code != exitFailure { + t.Fatalf("exit = %d, want %d\nstdout:\n%s\nstderr:\n%s", code, exitFailure, stdout, stderr) + } + if !strings.Contains(stdout, "Carried out 0 of 1 change.") { + t.Errorf("the report counts an adoption the save never wrote:\n%s", stdout) + } + if strings.Contains(stdout, "recorded as adopted") { + t.Errorf("the report claims an adoption the save never wrote:\n%s", stdout) + } + if !strings.Contains(stderr, ".agentsync-state-") { + t.Errorf("stderr does not report the failed save:\n%s", stderr) + } +} + +func TestRunReconcile_JSONReportsPruneAdopted(t *testing.T) { + for _, tc := range []struct { + name string + flags []string + want bool + }{ + {"with the flag", []string{"--" + reconcile.PruneFlag, "--" + reconcile.PruneAdoptedFlag}, true}, + {"without it", []string{"--" + reconcile.PruneFlag}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + base, repoRoot := reconcileFixture(t) + code, stdout, stderr := runCLI(t, reconcileArgs(t, repoRoot, base, append(tc.flags, "--json")...)...) + if code != exitOK { + t.Fatalf("exit = %d, want %d\nstderr:\n%s", code, exitOK, stderr) + } + var rep map[string]any + if err := json.Unmarshal([]byte(stdout), &rep); err != nil { + t.Fatalf("the JSON report does not parse: %v\n%s", err, stdout) + } + if got := rep["prune_adopted"]; got != tc.want { + t.Errorf("prune_adopted = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/internal/linker/apply.go b/internal/linker/apply.go index fe2a3c8..e6d5a23 100644 --- a/internal/linker/apply.go +++ b/internal/linker/apply.go @@ -23,8 +23,8 @@ type Applied struct { Links []Item Excludes []Exclude - // Recorded is what went into the state document, one entry per link - // actually created — the document's own words, so a report cannot + // Recorded is what the saved state document holds, one entry per link + // actually created. It is empty when the save failed, so a report cannot // describe a record that was never written. Recorded []state.Record } @@ -98,6 +98,7 @@ func (p Plan) Apply(repo *scan.Repo, st *state.File) (Applied, []error) { if st.Dirty() { if err := st.Save(repo); err != nil { errs = append(errs, err) + done.Recorded = done.Recorded[:0] } } return done, errs diff --git a/internal/linker/apply_test.go b/internal/linker/apply_test.go index 04e9900..67b5568 100644 --- a/internal/linker/apply_test.go +++ b/internal/linker/apply_test.go @@ -535,6 +535,9 @@ func TestApply_StateSaveFailureIsReported(t *testing.T) { if len(done.Links) != 1 { t.Errorf("links = %v, want the adoption", done.Links) } + if len(done.Recorded) != 0 { + t.Errorf("recorded = %v, want none after a failed save", done.Recorded) + } assertLink(t, path, filepath.Join(f.repoRoot, "agents", "shared.md")) assertNoTemps(t, f.repoRoot, ".agentsync-state-") } diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index f47e60a..9c60783 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -18,7 +18,8 @@ import ( // The five kinds are reported separately because they are five different // promises: a removed link, a marker that changes what link will do in future, // a record that makes a link removable at all, a moved directory, and a deleted -// file. +// file. Declined and Adopted hold only what was saved, so they are empty after +// a failed save. type Applied struct { Pruned []Item Declined []state.Declined @@ -87,6 +88,10 @@ func (p Plan) Apply(repo *scan.Repo, st *state.File) (Applied, []error) { if st.Dirty() { if err := st.Save(repo); err != nil { errs = append(errs, err) + // Only these two exist solely in state; every other kind already + // changed the disk, so it stays reported. + done.Adopted = done.Adopted[:0] + done.Declined = done.Declined[:0] } } return done, errs diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index 0a02bdd..d0290ea 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -635,6 +635,42 @@ func TestApply_AdoptRefusesAChangedTarget(t *testing.T) { } } +// TestApply_StateSaveFailureReportsNoStateOnlyChanges: an adopt and a decline +// exist only in the state document, so a failed save means neither happened. +func TestApply_StateSaveFailureReportsNoStateOnlyChanges(t *testing.T) { + const project = "Web/proj" + base, repo := fixture(t) + + adoptTarget := globalAgent(t, repo, "adopt", "one\n") + mustLink(t, adoptTarget, agentPath(base, project, "adopt")) + + declineTarget := globalAgent(t, repo, "decline", "two\n") + declinePath := agentPath(base, project, "decline") + st := state.New() + recordCreated(t, st, project, "decline", declinePath, declineTarget, mustHash(t, declineTarget)) + + plan := planFor(t, base, repo, st, Options{Adopt: true}) + if got := itemFor(t, plan, project, "adopt").Op; got != OpAdopt { + t.Fatalf("adopt op = %s, want %s", got, OpAdopt) + } + if got := itemFor(t, plan, project, "decline").Op; got != OpDecline { + t.Fatalf("decline op = %s, want %s", got, OpDecline) + } + // A non-empty directory where the state file goes makes Save's rename fail. + mustWrite(t, filepath.Join(state.Path(repo), "kept"), "inside\n") + + done, errs := plan.Apply(repo, st) + if len(errs) != 1 || !strings.Contains(errs[0].Error(), state.FileName) { + t.Fatalf("want one error naming %s, got %v", state.FileName, errs) + } + if len(done.Adopted) != 0 { + t.Errorf("adopted = %v, want none after a failed save", done.Adopted) + } + if len(done.Declined) != 0 { + t.Errorf("declined = %v, want none after a failed save", done.Declined) + } +} + // --- dry run ---------------------------------------------------------------- // TestNewPlan_WritesNothing: planning is read-only, whatever the flags say. diff --git a/internal/testfs/testfs.go b/internal/testfs/testfs.go index 191e7fc..6dcaebd 100644 --- a/internal/testfs/testfs.go +++ b/internal/testfs/testfs.go @@ -336,16 +336,15 @@ func pathLimit(base string) int { // ENAMETOOLONG for its own reason, so a probe built that way would report the // per-component limit and the caller would never learn the total-path one. // -// The one length it cannot represent is len(base)+1: a component costs at least -// two bytes, the separator and one character. That n overshoots by exactly one, -// which is the smallest possible miss and harmless to a binary search over a -// monotone predicate. +// A component costs at least two bytes, so the loop never leaves exactly one +// byte over; the only length it cannot hit is len(base)+1, which overshoots by +// one. func probePath(base string, n int) string { b := base for len(b)+1 < n { - chunk := n - len(b) - 1 - if chunk > segmentLen { - chunk = segmentLen + chunk := min(n-len(b)-1, segmentLen) + if n-len(b)-1-chunk == 1 { + chunk-- } b += "/" + strings.Repeat("p", chunk) } @@ -418,3 +417,23 @@ func PlantAgent(t testing.TB, inside *os.Root, project, name, content string) { t.Fatalf("planting %s: %v", name, err) } } + +// DirWithRoom creates a directory under parent whose path leaves exactly room +// bytes before the platform's path limit, so a child name of room-1 bytes still +// opens and any longer one fails with ENAMETOOLONG, for root too. +func DirWithRoom(t testing.TB, parent string, room int) string { + t.Helper() + + dir := probePath(parent, pathLimit(parent)-room) + if err := os.MkdirAll(dir, 0o755); err != nil || !hasRoom(dir, room) { + t.Fatalf("could not build a %d-byte directory leaving exactly %d bytes of room (mkdir error: %v)", len(dir), room, err) + } + return dir +} + +// hasRoom reports whether a child name of room-1 bytes fits under dir and one +// of room bytes does not. +func hasRoom(dir string, room int) bool { + return !tooLong(filepath.Join(dir, strings.Repeat("f", room-1))) && + tooLong(filepath.Join(dir, strings.Repeat("f", room))) +} diff --git a/internal/testfs/testfs_test.go b/internal/testfs/testfs_test.go index 555b7cf..774835d 100644 --- a/internal/testfs/testfs_test.go +++ b/internal/testfs/testfs_test.go @@ -268,7 +268,7 @@ func TestPathLimit_IsDerivedAndExact(t *testing.T) { // probePath must hit its target length exactly, or the binary search // above is measuring something other than what it reports. The one // exception is len(base)+1, which no component can express. - for _, n := range []int{len(base) + 2, len(base) + 201, len(base) + 500} { + for _, n := range []int{len(base) + 2, len(base) + 201, len(base) + 202, len(base) + 500} { if got := len(probePath(base, n)); got != n { t.Errorf("probePath(base, %d) produced %d bytes; the length search would be off by %d", n, got, got-n) } @@ -284,3 +284,28 @@ func TestPathLimit_IsDerivedAndExact(t *testing.T) { } } } + +func TestDirWithRoom_LeavesExactlyThatRoom(t *testing.T) { + const room = 22 + dir := DirWithRoom(t, t.TempDir(), room) + + fits := filepath.Join(dir, strings.Repeat("f", room-1)) + if err := os.WriteFile(fits, nil, 0o644); err != nil { + t.Fatalf("a %d-byte name should fit: %v", room-1, err) + } + err := os.WriteFile(filepath.Join(dir, strings.Repeat("f", room)), nil, 0o644) + if !errors.Is(err, syscall.ENAMETOOLONG) { + t.Fatalf("a %d-byte name should fail with ENAMETOOLONG, got %v", room, err) + } + + // The self-check DirWithRoom relies on must reject a room off by one either + // way, and an ordinary directory. + for _, r := range []int{room - 1, room + 1} { + if hasRoom(dir, r) { + t.Errorf("hasRoom accepted %d bytes for a directory with %d", r, room) + } + } + if hasRoom(t.TempDir(), room) { + t.Error("hasRoom accepted a short directory") + } +}