From 33e5df57ca41d42b78b671b83105b031445f4f11 Mon Sep 17 00:00:00 2001 From: Logan Lindquist Land Date: Mon, 19 Oct 2026 19:35:22 -0500 Subject: [PATCH] feat: agentsync optimize --apply writes suggested model and effort Writes each suggested change into the agent's real file (a linked agent's canonical file; the link stays a link), editing only the model: and effort: lines byte-for-byte. Refuses a file changed since it was read, foreign links, dereferenced copies, read-only files and frontmatter it cannot edit safely. Reports how many projects link each written file and reminds that the canonical repo is not committed. --- README.md | 2 +- cmd/agentsync/main.go | 17 +- cmd/agentsync/optimize.go | 190 ++++++++++++++++-- cmd/agentsync/optimize_test.go | 297 ++++++++++++++++++++++++++++- docs/exit-codes.md | 2 +- docs/optimize.md | 58 +++++- internal/optimize/optimize.go | 116 +++++++++++ internal/optimize/optimize_test.go | 70 +++++++ 8 files changed, 728 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 0bbf938..df577f9 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ Run without `--apply` first to see what a command would do. | `doctor` | report broken, foreign, or mismatched links and stale captures; never writes | | `reconcile` | act on that drift, gated on what agentsync itself recorded | | `inventory` | regenerate `inventory.json`, a record of every agent on the machine | -| `optimize` | ask an OpenRouter advisor which model and effort each agent should use; report only | +| `optimize` | ask an OpenRouter advisor which model and effort each agent should use; `--apply` writes the suggested changes | | `override` | set one checkout's model and effort for a Claude agent without forking it | `agentsync help` lists every flag. diff --git a/cmd/agentsync/main.go b/cmd/agentsync/main.go index 8eafd0f..e53ab0c 100644 --- a/cmd/agentsync/main.go +++ b/cmd/agentsync/main.go @@ -37,7 +37,8 @@ // confirmed, and refuses to replace an existing one without --force. // // optimize is the only command that uses the network: it asks an OpenRouter -// advisor about each agent's model and effort, and writes nothing. +// advisor about each agent's model and effort, and with --apply writes the +// suggested model: and effort: values into the agent files. // // override writes only the project's .claude/settings.local.json, with --apply. package main @@ -69,7 +70,7 @@ Usage: agentsync reconcile [flags] act on that drift, gated on what this tool recorded agentsync inventory [flags] regenerate inventory.json: every agent on this machine agentsync optimize [flags] ask an OpenRouter advisor which model and effort each - agent should use (report only; the one networked command) + agent should use (--apply writes it; the one networked command) agentsync override [flags] set this checkout's model and effort for a Claude agent agentsync version @@ -232,13 +233,16 @@ optimize flags: (default 0.50) --min-confidence P least probability a pick needs before it is suggested - (default 0.6); below it the row says keep (low confidence) + (default 0.6); a differing pick below it is not suggested --agent NAME ask only about this agent (repeatable) + --apply write each suggested change; without it optimize is a dry run optimize needs $OPENROUTER_API_KEY and sends each agent file (first 24 KB) to OpenRouter, one request per agent with no retries. It reads model: and effort: -from each agent's frontmatter and writes nothing. Exit 1 when a row is -unresolved or skipped. +from each agent's frontmatter. --apply edits only the suggested parts of those +lines, in the file a link points at: a canonical repo file changes for every +project linked to it, and agentsync never commits it. Exit 1 when a row is +unresolved or skipped, or a change was refused; exit 2 when a write failed. override flags: --agent NAME the agent to override, by its frontmatter name; it must be @@ -257,7 +261,8 @@ scan and doctor are read-only. import writes only into this repo, only with command that writes into a project's .claude/agents, only with --apply, and it refuses any project holding a file this repo has no copy of — import that first. override writes only the project's .claude/settings.local.json, also -only with --apply. +only with --apply. optimize --apply edits only the model: and effort: lines of +agent files it advised on. inventory also writes only into this repo, and takes no --apply: its sole output is inventory.json, which it owns, recomputes from scratch every run, and diff --git a/cmd/agentsync/optimize.go b/cmd/agentsync/optimize.go index 9e671f6..e56b73f 100644 --- a/cmd/agentsync/optimize.go +++ b/cmd/agentsync/optimize.go @@ -17,7 +17,9 @@ import ( "github.com/agentic-tooling/agentsync/internal/discover" "github.com/agentic-tooling/agentsync/internal/openrouter" "github.com/agentic-tooling/agentsync/internal/optimize" + "github.com/agentic-tooling/agentsync/internal/override" "github.com/agentic-tooling/agentsync/internal/scan" + "github.com/agentic-tooling/agentsync/internal/state" ) // openrouterURL is where optimize sends its requests; tests point it at an @@ -33,7 +35,7 @@ const ( var claudeOverrides = []string{"CLAUDE_CODE_SUBAGENT_MODEL", "CLAUDE_CODE_EFFORT_LEVEL"} // runOptimize asks an advisor model about each Claude agent under the root -// and reports its advice. It writes nothing. +// and reports its advice. Only --apply writes, and only the parts it suggests. func runOptimize(args []string, stdout, stderr io.Writer) int { fs := flag.NewFlagSet("optimize", flag.ContinueOnError) fs.SetOutput(stderr) @@ -45,6 +47,7 @@ func runOptimize(args []string, stdout, stderr io.Writer) int { advisor := fs.String("advisor", defaultAdvisor, "OpenRouter decision model to ask") maxCost := fs.Float64("max-cost", 0.50, "most this run may spend, in USD") minConf := fs.Float64("min-confidence", 0.6, "least probability a pick needs to be suggested") + apply := fs.Bool("apply", false, "write each suggested change into its agent's file") var names stringList fs.Var(&names, "agent", "ask only about this agent (repeatable)") @@ -125,7 +128,16 @@ func runOptimize(args []string, stdout, stderr io.Writer) int { rep := optimize.Run(ctx, client, agents, cfg) rep.Overrides = overrides + code := exitOK + if rep.Count(optimize.StatusUnresolved)+rep.Count(optimize.StatusSkipped) > 0 { + code = exitAtRisk + } + var applied strings.Builder + if *apply { + code = max(code, applyChanges(&applied, stderr, repo, res, ov, agents, rep.Rows)) + } if *asJSON { + fmt.Fprint(notes, applied.String()) enc := json.NewEncoder(stdout) enc.SetIndent("", " ") if err := enc.Encode(rep); err != nil { @@ -133,12 +145,155 @@ func runOptimize(args []string, stdout, stderr io.Writer) int { return exitFailure } } else { - emitOptimizeText(stdout, rep, cfg) + emitOptimizeText(stdout, rep, cfg, *apply, applied.String()) } - if rep.Count(optimize.StatusUnresolved)+rep.Count(optimize.StatusSkipped) > 0 { - return exitAtRisk + return code +} + +// refusal is a change left unwritten on purpose, as opposed to a failed write. +type refusal struct{ error } + +// applyChanges writes each change row into the file its agent resolves to and +// records the outcome on the row; rows[i] is the advice for agents[i]. It +// returns exitAtRisk for a refused row and exitFailure for a failed write. +func applyChanges(w, stderr io.Writer, repo *scan.Repo, res scan.Result, ov map[string]override.Value, agents []optimize.Agent, rows []optimize.Row) int { + code := exitOK + repoReal := resolved(repo.Root) + links, stateErr := state.Load(repo) + inRepo, warned := false, false + for i := range rows { + r := &rows[i] + if r.Status != optimize.StatusChange { + continue + } + real, err := writeChange(agents[i], r.SuggestedModel, r.SuggestedEffort, repoReal, links, stateErr) + if err != nil { + r.ApplyError = err.Error() + fmt.Fprintf(w, "%s: not written: %v\n", r.Agent, err) + if errors.As(err, new(refusal)) { + code = max(code, exitAtRisk) + } else { + code = exitFailure + } + continue + } + r.Applied = true + where := "" + if within(real, repoReal) { + inRepo = true + if stateErr != nil && !warned { + warned = true + fmt.Fprintf(stderr, "agentsync: warning: link counts not shown: %v\n", stateErr) + } + n := 0 + if links != nil { + for _, l := range links.Records() { + if resolved(l.Target) == real { + n++ + } + } + } + where = " (canonical file)" + if n > 0 { + where = fmt.Sprintf(" (canonical; linked into %d project%s)", n, plural(n, "", "s")) + } + } + fmt.Fprintf(w, "wrote %s%s\n", displayPath(repo.Home, real), where) + for _, e := range res.Entries { + v, ok := ov[e.Path] + masks := (v.Model != "" && r.SuggestedModel != "") || (v.Effort != "" && r.SuggestedEffort != "") + if ok && masks && resolved(e.Path) == real { + fmt.Fprintf(w, "Note: %s/%s's project override (%s) still wins over the written frontmatter there.\n", e.Project, e.Name, v) + } + } + } + if inRepo { + fmt.Fprintf(w, "Changed files in %s are not committed; review and commit them there.\n", displayPath(repo.Home, repo.Root)) + } + return code +} + +// writeChange edits the file a.Path resolves to, never the link itself, and +// returns that file's path. stateErr is why the state file could not be read, +// which leaves no way to tell a regular file from a dereferenced link. +func writeChange(a optimize.Agent, model, effort, repoReal string, links *state.File, stateErr error) (string, error) { + li, err := os.Lstat(a.Path) + var real string + if err == nil { + real, err = filepath.EvalSymlinks(a.Path) + } + var fi os.FileInfo + if err == nil { + fi, err = os.Stat(real) + } + var data []byte + if err == nil { + data, err = os.ReadFile(real) + } + if err != nil { + return "", err } - return exitOK + link := li.Mode()&os.ModeSymlink != 0 + recorded := false + if stateErr == nil { + _, recorded = links.LookupLink(a.Path) + } + switch { + case link && !within(real, repoReal): + return "", refusal{errors.New("it links outside the canonical repo, and agentsync leaves foreign links alone")} + case !link && stateErr != nil: + return "", fmt.Errorf("the state file could not be read to check that this was never an agentsync link: %w", stateErr) + case !link && recorded: + // Editing a dereferenced copy would turn reconcile's clean relink into a conflict. + return "", refusal{errors.New("agentsync recorded a link here that is now a copy; run agentsync link --apply first")} + case fi.Mode().Perm()&0o200 == 0: + return "", refusal{errors.New("the file is read-only")} + case string(data) != a.Content: + return "", refusal{errors.New("the file changed since it was read; run optimize again")} + } + out, err := optimize.Edit(a.Content, model, effort) + if err != nil { + return "", refusal{err} + } + return real, writeAtomic(real, []byte(out), fi.Mode().Perm()) +} + +// within reports whether p lies below dir. +func within(p, dir string) bool { + return strings.HasPrefix(p, dir+string(filepath.Separator)) +} + +// writeAtomic replaces path through a temp file beside it, so a reader sees +// the old file or the new one and never half of either. The temp name does not +// end in .md, so a scan running meanwhile never takes it for an agent. +func writeAtomic(path string, data []byte, mode os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), ".agentsync-*.tmp") + if err != nil { + return err + } + defer func() { _ = os.Remove(tmp.Name()) }() + _, err = tmp.Write(data) + if err == nil { + err = tmp.Chmod(mode) + } + if err == nil { + err = tmp.Sync() + } + if cerr := tmp.Close(); err == nil { + err = cerr + } + if err == nil { + err = os.Rename(tmp.Name(), path) + } + return err +} + +// resolved is p with every symlink followed, or p as given when that fails. +func resolved(p string) string { + if r, err := filepath.EvalSymlinks(p); err == nil { + return r + } + return p } func optimizeFlagsErr(fs *flag.FlagSet, advisor string, maxCost, minConf float64) error { @@ -189,10 +344,7 @@ func optimizeAgents(res scan.Result, names []string) ([]optimize.Agent, error) { continue } want[e.Name] = true - real := e.Path - if r, err := filepath.EvalSymlinks(e.Path); err == nil { - real = r - } + real := resolved(e.Path) if seen[real] { continue } @@ -211,7 +363,7 @@ func optimizeAgents(res scan.Result, names []string) ([]optimize.Agent, error) { return out, nil } -func emitOptimizeText(w io.Writer, rep optimize.Report, cfg optimize.Config) { +func emitOptimizeText(w io.Writer, rep optimize.Report, cfg optimize.Config, apply bool, applied string) { skipped := rep.Count(optimize.StatusSkipped) asked := len(rep.Rows) - skipped fmt.Fprintf(w, "Asked %s about %d agent%s", rep.Advisor, asked, plural(asked, "", "s")) @@ -236,5 +388,21 @@ func emitOptimizeText(w io.Writer, rep optimize.Report, cfg optimize.Config) { if skipped > 0 { fmt.Fprintf(w, "%d agent%s skipped (reasons above).\n", skipped, plural(skipped, " was", "s were")) } - fmt.Fprintf(w, "Spent $%.6f (reported by OpenRouter). Dry run: nothing written.\n", rep.Spent) + fmt.Fprint(w, applied) + fmt.Fprintf(w, "Spent $%.6f (reported by OpenRouter). ", rep.Spent) + n := rep.Count(optimize.StatusChange) + switch { + case apply: + written := 0 + for _, r := range rep.Rows { + if r.Applied { + written++ + } + } + fmt.Fprintf(w, "Wrote %d change%s.\n", written, plural(written, "", "s")) + case n > 0: + fmt.Fprintf(w, "Dry run: nothing written. Run with --apply to write %d change%s.\n", n, plural(n, "", "s")) + default: + fmt.Fprintln(w, "Dry run: nothing written.") + } } diff --git a/cmd/agentsync/optimize_test.go b/cmd/agentsync/optimize_test.go index ad74ae9..6cf60c0 100644 --- a/cmd/agentsync/optimize_test.go +++ b/cmd/agentsync/optimize_test.go @@ -34,11 +34,13 @@ func agentFile(name, model, effort string) string { // fakeOpenRouter serves models (status 200 unless modelsStatus says // otherwise) and the decisions in order, and points optimize at itself with -// testKey set. It counts both kinds of request. +// testKey set. It counts both kinds of request, and calls onDecide, if set, +// with each decision request's number before answering it. type fakeOpenRouter struct { models string modelsStatus int decisions []string + onDecide func(n int) mu sync.Mutex listed, sent int } @@ -66,6 +68,9 @@ func (f *fakeOpenRouter) start(t *testing.T) { return } f.sent++ + if f.onDecide != nil { + f.onDecide(f.sent) + } if f.sent > len(f.decisions) { w.WriteHeader(http.StatusTeapot) return @@ -163,6 +168,9 @@ func TestOptimize_JSON(t *testing.T) { if !reflect.DeepEqual(rep.Overrides, []string{"CLAUDE_CODE_SUBAGENT_MODEL"}) { t.Errorf("overrides = %v", rep.Overrides) } + if strings.Contains(stdout, `"appl`) { + t.Errorf("a dry run's JSON carries apply fields:\n%s", stdout) + } for _, w := range []string{"Repo: ", "Root: ", "Note: $CLAUDE_CODE_SUBAGENT_MODEL is set"} { if !strings.Contains(stderr, w) { t.Errorf("%q belongs on stderr with --json:\n%s", w, stderr) @@ -329,3 +337,290 @@ func mustRemoveAll(t *testing.T, p string) { t.Fatal(err) } } + +func TestOptimize_DryRunWritesNothing(t *testing.T) { + project, repoRoot := optimizeProject(t, map[string]string{"a.md": agentFile("a", "sonnet", "high")}) + (&fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{orFixture(t, "decision-clef.json")}}).start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--root", project, "--repo", repoRoot) + if code != exitOK || !strings.Contains(stdout, "Dry run: nothing written. Run with --apply to write 1 change.\n") { + t.Errorf("exit %d:\n%s%s", code, stdout, stderr) + } + if got := readFileT(t, filepath.Join(project, ".claude", "agents", "a.md")); got != agentFile("a", "sonnet", "high") { + t.Errorf("a dry run wrote the file:\n%s", got) + } +} + +// The canonical file is written through both links, which stay links, and +// doctor and reconcile report the same tree before and after. +func TestOptimize_ApplyWritesTheCanonicalTarget(t *testing.T) { + home := mustResolveDir(t, t.TempDir()) + t.Setenv("HOME", home) + web := filepath.Join(home, "Web") + repoRoot := filepath.Join(web, "agents-repo") + mustMkdirT(t, filepath.Join(repoRoot, "project-agents")) + target := filepath.Join(repoRoot, "agents", "shared.md") + const content = "---\nname: shared\ndescription: d\nmodel: sonnet # pinned\neffort: high\n---\nbody says model: sonnet\n" + mustWriteT(t, target, content) + for _, p := range []string{"one", "two"} { + mustMkdirT(t, filepath.Join(web, p, ".claude")) + if code, out, errOut := runCLI(t, "link", "--repo", repoRoot, "--root", web, "--project", "Web/"+p, "--agent", "shared", "--apply"); code != exitOK { + t.Fatalf("link %s: exit %d\n%s%s", p, code, out, errOut) + } + } + before := map[string]string{} + for _, cmd := range []string{"doctor", "reconcile"} { + code, out, errOut := runCLI(t, cmd, "--root", web, "--repo", repoRoot) + if code != exitOK { + t.Fatalf("%s before the apply: exit %d\n%s%s", cmd, code, out, errOut) + } + before[cmd] = out + } + f := &fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{orFixture(t, "decision-clef.json"), orFixture(t, "decision-jev.json"), orFixture(t, "decision-clef.json")}} + f.start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--apply", "--root", web, "--repo", repoRoot) + if code != exitOK { + t.Fatalf("exit = %d\n%s%s", code, stdout, stderr) + } + want := strings.Replace(content, "model: sonnet # pinned", "model: opus # pinned", 1) + if got := readFileT(t, target); got != want { + t.Errorf("canonical file =\n%s\nwant\n%s", got, want) + } + for _, p := range []string{"one", "two"} { + assertIsLink(t, filepath.Join(web, p, ".claude", "agents", "shared.md"), target) + } + for _, w := range []string{ + "wrote ~/Web/agents-repo/agents/shared.md (canonical; linked into 2 projects)\n", + "Changed files in ~/Web/agents-repo are not committed; review and commit them there.\n", + "Wrote 1 change.\n", + } { + if !strings.Contains(stdout, w) { + t.Errorf("report is missing %q:\n%s", w, stdout) + } + } + for cmd, was := range before { + if code, out, _ := runCLI(t, cmd, "--root", web, "--repo", repoRoot); code != exitOK || out != was { + t.Errorf("%s changed after the apply: exit %d\n%s\n->\n%s", cmd, code, was, out) + } + } + + // Without a state file there is no link count to give. + mustRemoveAll(t, filepath.Join(repoRoot, ".agentsync-state.json")) + code, stdout, stderr = runCLI(t, "optimize", "--apply", "--root", web, "--repo", repoRoot) + if code != exitOK || !strings.Contains(stdout, "wrote ~/Web/agents-repo/agents/shared.md (canonical file)\n") { + t.Errorf("exit %d:\n%s%s", code, stdout, stderr) + } + if got := readFileT(t, target); got != strings.Replace(want, "effort: high", "effort: xhigh", 1) { + t.Errorf("canonical file =\n%s", got) + } + + // An unreadable state file is warned about, not trusted for a count. + mustWriteT(t, filepath.Join(repoRoot, ".agentsync-state.json"), "{") + code, stdout, stderr = runCLI(t, "optimize", "--apply", "--min-confidence", "0.5", "--root", web, "--repo", repoRoot) + if code != exitOK || !strings.Contains(stdout, "(canonical file)\n") || !strings.Contains(stderr, "warning: link counts not shown") { + t.Errorf("exit %d:\n%s%s", code, stdout, stderr) + } +} + +// Only the confident differing part of a change row is written, in place; +// keep and unresolved rows are left alone. +func TestOptimize_ApplyEditsAProjectFileInPlace(t *testing.T) { + project, repoRoot := optimizeProject(t, map[string]string{ + "b.md": agentFile("b", "opus", "high"), + "c.md": agentFile("c", "opus", "low"), + }) + dir := filepath.Join(project, ".claude", "agents") + // Not the 0600 a temp file is created with, so a lost mode shows. + if err := os.WriteFile(filepath.Join(dir, "a.md"), []byte(agentFile("a", "sonnet", "low")), 0o750); err != nil { + t.Fatal(err) + } + before, err := os.Stat(filepath.Join(dir, "a.md")) + if err != nil || before.Mode().Perm() == 0o600 { + t.Fatalf("a.md mode %v, %v", before.Mode(), err) + } + clef := orFixture(t, "decision-clef.json") + (&fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{clef, clef}}).start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--apply", "--root", project, "--repo", repoRoot) + if code != exitAtRisk { + t.Errorf("exit = %d, want %d (c is unresolved)\n%s", code, exitAtRisk, stderr) + } + for name, want := range map[string]string{ + "a": agentFile("a", "opus", "low"), + "b": agentFile("b", "opus", "high"), + "c": agentFile("c", "opus", "low"), + } { + path := filepath.Join(dir, name+".md") + if got := readFileT(t, path); got != want { + t.Errorf("%s =\n%s\nwant\n%s", name, got, want) + } + assertIsRegular(t, path, "an in-place edit keeps a regular file") + } + if fi, err := os.Stat(filepath.Join(dir, "a.md")); err != nil || fi.Mode().Perm() != before.Mode().Perm() { + t.Errorf("a.md mode = %v, %v; want %v kept", fi.Mode(), err, before.Mode()) + } + if !strings.Contains(stdout, "wrote ~/Web/proj/.claude/agents/a.md\n") || !strings.Contains(stdout, "Wrote 1 change.\n") || strings.Contains(stdout, "not committed") { + t.Errorf("report:\n%s", stdout) + } +} + +// A file changed while the advisor answered, or one Edit refuses, is not +// written; the other rows still are. +func TestOptimize_ApplyRefusals(t *testing.T) { + block := "---\nname: b\ndescription: d\nmodel: |\n sonnet\n---\n" + project, repoRoot := optimizeProject(t, map[string]string{ + "a.md": agentFile("a", "sonnet", "high"), + "b.md": block, + "c.md": agentFile("c", "sonnet", "high"), + }) + dir := filepath.Join(project, ".claude", "agents") + edited := agentFile("a", "sonnet", "high") + "edited\n" + clef := orFixture(t, "decision-clef.json") + f := &fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{clef, clef, clef}} + f.onDecide = func(n int) { + if n == 1 { + mustWriteT(t, filepath.Join(dir, "a.md"), edited) + } + } + f.start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--apply", "--json", "--root", project, "--repo", repoRoot) + if code != exitAtRisk { + t.Errorf("exit = %d, want %d\n%s", code, exitAtRisk, stderr) + } + var rep optimize.Report + if err := json.Unmarshal([]byte(stdout), &rep); err != nil || len(rep.Rows) != 3 { + t.Fatalf("stdout: %v\n%s", err, stdout) + } + for i, w := range []string{"changed since it was read; run optimize again", "model: is not a simple one-line value", ""} { + r := rep.Rows[i] + if !strings.Contains(r.ApplyError, w) || (w == "") != r.Applied || (w == "") != (r.ApplyError == "") { + t.Errorf("row %s: applied %v, apply_error %q; want error %q", r.Agent, r.Applied, r.ApplyError, w) + } + } + for name, want := range map[string]string{"a": edited, "b": block, "c": agentFile("c", "opus", "high")} { + if got := readFileT(t, filepath.Join(dir, name+".md")); got != want { + t.Errorf("%s =\n%s\nwant\n%s", name, got, want) + } + } + if !strings.Contains(stderr, "a: not written: the file changed") || !strings.Contains(stderr, "wrote ~/Web/proj/.claude/agents/c.md\n") { + t.Errorf("stderr:\n%s", stderr) + } +} + +func TestOptimize_ApplyWriteFailureExitsTwo(t *testing.T) { + project, repoRoot := optimizeProject(t, map[string]string{ + "a.md": agentFile("a", "sonnet", "high"), + "b.md": agentFile("b", "sonnet", "high"), + }) + dir := filepath.Join(project, ".claude", "agents") + clef := orFixture(t, "decision-clef.json") + f := &fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{clef, clef}} + f.onDecide = func(n int) { + if n == 1 { + mustRemoveAll(t, filepath.Join(dir, "a.md")) + } + } + f.start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--apply", "--root", project, "--repo", repoRoot) + if code != exitFailure { + t.Errorf("exit = %d, want %d\n%s", code, exitFailure, stderr) + } + if got := readFileT(t, filepath.Join(dir, "b.md")); got != agentFile("b", "opus", "high") { + t.Errorf("b was not written after a failed:\n%s", got) + } + if !strings.Contains(stdout, "a: not written: ") || !strings.Contains(stdout, "Wrote 1 change.\n") { + t.Errorf("report:\n%s", stdout) + } +} + +func TestOptimize_ApplyNotesOverrideStillWins(t *testing.T) { + project, repoRoot, settings := overrideFixture(t) + mustWriteT(t, settings, oneOverride) + clef := orFixture(t, "decision-clef.json") + (&fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{clef, clef, clef, clef}}).start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--apply", "--agent", "a", "--root", project, "--repo", repoRoot) + if code != exitOK { + t.Fatalf("exit %d\n%s", code, stderr) + } + if !strings.Contains(stdout, "wrote ~/Web/proj/.claude/agents/a.md\nNote: Web/proj/a's project override (sonnet / -) still wins over the written frontmatter there.\n") { + t.Errorf("no override note after the write:\n%s", stdout) + } + if strings.Contains(stdout, "b's project override") { + t.Errorf("note for b, which was not written:\n%s", stdout) + } + + // b's model is written, but its override sets only effort, so nothing is masked. + code, stdout, stderr = runCLI(t, "optimize", "--apply", "--root", project, "--repo", repoRoot) + if code != exitOK || !strings.Contains(stdout, "wrote ~/Web/proj/.claude/agents/b.md\n") || strings.Contains(stdout, "still wins") { + t.Errorf("exit %d:\n%s%s", code, stdout, stderr) + } + + // a's effort is written, but its override sets only model. + code, stdout, stderr = runCLI(t, "optimize", "--apply", "--min-confidence", "0.5", "--agent", "a", "--root", project, "--repo", repoRoot) + if code != exitOK || !strings.Contains(stdout, "wrote ~/Web/proj/.claude/agents/a.md\n") || strings.Contains(stdout, "still wins") { + t.Errorf("exit %d:\n%s%s", code, stdout, stderr) + } +} + +// Foreign links, read-only files and dereferenced copies of agentsync links +// are refused. A project-local file is not written while the state file is +// unreadable, and that is exit 2: the question could not be answered. +func TestOptimize_ApplyRefusesWhatAgentsyncLeavesAlone(t *testing.T) { + project, repoRoot := optimizeProject(t, nil) + home := filepath.Dir(filepath.Dir(project)) + dir := filepath.Join(project, ".claude", "agents") + // Linked first: link refuses a project holding files the repo has no copy of. + mustWriteT(t, filepath.Join(repoRoot, "agents", "s.md"), agentFile("s", "sonnet", "high")) + // A sibling whose path merely starts with the repo's is still outside it. + sibling := filepath.Join(repoRoot+"2", "agents", "e.md") + mustWriteT(t, sibling, agentFile("e", "sonnet", "high")) + if code, out, errOut := runCLI(t, "link", "--repo", repoRoot, "--root", project, "--project", "Web/proj", "--agent", "s", "--apply"); code != exitOK { + t.Fatalf("link: exit %d\n%s%s", code, out, errOut) + } + foreign := filepath.Join(home, "dotfiles", "d.md") + mustWriteT(t, foreign, agentFile("d", "sonnet", "high")) + for name, target := range map[string]string{"d.md": foreign, "e.md": sibling} { + if err := os.Symlink(target, filepath.Join(dir, name)); err != nil { + t.Fatal(err) + } + } + // The write bit is what is checked, so this holds whatever user runs it. + if err := os.WriteFile(filepath.Join(dir, "r.md"), []byte(agentFile("r", "sonnet", "high")), 0o444); err != nil { + t.Fatal(err) + } + mustRemoveAll(t, filepath.Join(dir, "s.md")) + mustWriteT(t, filepath.Join(dir, "s.md"), agentFile("s", "sonnet", "high")) + clef := orFixture(t, "decision-clef.json") + (&fakeOpenRouter{models: orFixture(t, "models.json"), decisions: []string{clef, clef, clef, clef, clef, clef, clef, clef}}).start(t) + + code, stdout, stderr := runCLI(t, "optimize", "--apply", "--root", project, "--repo", repoRoot) + if code != exitAtRisk { + t.Errorf("exit = %d, want %d\n%s", code, exitAtRisk, stderr) + } + for _, w := range []string{ + "d: not written: it links outside the canonical repo", + "e: not written: it links outside the canonical repo", + "r: not written: the file is read-only", + "s: not written: agentsync recorded a link here that is now a copy; run agentsync link --apply first", + "Wrote 0 changes.", + } { + if !strings.Contains(stdout, w) { + t.Errorf("report is missing %q:\n%s", w, stdout) + } + } + for _, p := range []string{foreign, sibling, filepath.Join(dir, "r.md"), filepath.Join(dir, "s.md")} { + if got := readFileT(t, p); !strings.Contains(got, "model: sonnet") { + t.Errorf("%s was written:\n%s", p, got) + } + } + + mustWriteT(t, filepath.Join(repoRoot, ".agentsync-state.json"), "{") + code, stdout, stderr = runCLI(t, "optimize", "--apply", "--root", project, "--repo", repoRoot) + if code != exitFailure || !strings.Contains(stdout, "s: not written: the state file could not be read") || strings.Contains(stderr, "warning") { + t.Errorf("exit %d:\n%s%s", code, stdout, stderr) + } +} diff --git a/docs/exit-codes.md b/docs/exit-codes.md index a792d92..9c2e3ac 100644 --- a/docs/exit-codes.md +++ b/docs/exit-codes.md @@ -17,7 +17,7 @@ What counts as exit 1, by command: | `doctor` | a broken or mismatched link, a stale capture, an orphaned capture directory, an override that cannot run, or a project path the scan could not read (a capture or settings file it could not judge is exit 2) | | `reconcile` | a refused selector, a conflict, a diverged or dereferenced link, or a removable link left in place | | `inventory --check` | the file is stale or absent | -| `optimize` | an agent's advice is unresolved, or it was skipped by the cost cap or a stopped run | +| `optimize` | an agent's advice is unresolved, or it was skipped by the cost cap or a stopped run; with `--apply`, a change was refused (a foreign link, a dereferenced link, a read-only or changed file, or frontmatter that cannot be edited safely) | | `override` | never | One exception: the hidden `agentsync hook claude-agent`, which Claude Code runs diff --git a/docs/optimize.md b/docs/optimize.md index e59afd3..39d7429 100644 --- a/docs/optimize.md +++ b/docs/optimize.md @@ -2,7 +2,8 @@ `agentsync optimize` asks an OpenRouter decision model which Claude model and reasoning effort each of a project's Claude agents should run on, and reports -the advice. It writes nothing. It is the only agentsync command that uses the +the advice. With `--apply` it writes the suggested changes into the agent files +(see [Applying](#applying)). It is the only agentsync command that uses the network. ```bash @@ -11,6 +12,7 @@ agentsync optimize # the project you are in agentsync optimize --agent go-reviewer # one agent agentsync optimize --advisor cloudflare/clef --max-cost 0.05 agentsync optimize --json +agentsync optimize --apply # write the suggested changes ``` Use a dedicated OpenRouter key with a credit limit set on OpenRouter's side. @@ -52,6 +54,7 @@ Nothing else from the project is sent. | `--max-cost USD` | the most the run may spend; default `0.50` | | `--min-confidence P` | the probability a pick needs before it is suggested; default `0.6` | | `--agent NAME` | ask only about this agent; repeatable | +| `--apply` | write each suggested change; without it the run is a dry run | | `--json` | print the report as JSON on stdout (see below), with the Repo, Root and override notes on stderr | ## Reading the report @@ -88,6 +91,52 @@ something it was not offered. An unresolved row suggests nothing. | `model_low_confidence`, `effort_low_confidence` | `true` when that part's differing pick was dropped; omitted otherwise | | `reason` | why a row is unresolved or skipped | | `cost_usd` | the `usage.cost` OpenRouter reported for this row | +| `applied` | `true` when `--apply` wrote this row; omitted otherwise | +| `apply_error` | why `--apply` did not write this `change` row; omitted otherwise | + +With `--json`, the lines `--apply` prints go to stderr with the other notes. + +## Applying + +Without `--apply` nothing is written, and the last line says how many changes +`--apply` would write. With it, each `change` row is written; `keep`, +`unresolved` and `skipped` rows, and any part dropped for low confidence, are +not. There is no prompt: `--apply` is the write boundary, as for `link` and +`reconcile`. + +- Only the suggested parts change. An existing top-level `model:` or `effort:` + line keeps its spacing, its quotes and any trailing `# comment`; a missing + key is added just before the closing `---`, with the file's line ending. + Every other byte, including the body, is unchanged. +- The file written is the one the agent resolves to. For a linked agent that is + the canonical repo file, so every project linked to it gets the change; the + link stays a link. The report says `wrote (canonical; linked into N + projects)`, counting the link records in `.agentsync-state.json`, or + `(canonical file)` when there are none. A project-local agent file is edited + in place. +- agentsync never commits. When a written file is in the canonical repo, the + run ends by saying the changes there are not committed. +- A project [override](override.md) still wins over the written frontmatter in + its project, and the run notes each one that sets a part it wrote. +- Each file is written through a temp file in its own directory and renamed + over it, keeping its mode. + +A `change` row is refused, and the run exits 1, when: + +- the agent is a link to a file outside the canonical repo (a foreign link, + which agentsync never touches); +- it is a regular file where `.agentsync-state.json` records an agentsync link, + a dereferenced copy: run `agentsync link --apply` first; +- the file is read-only (its owner write bit is clear); +- the file changed since the advice was read: run optimize again; +- its frontmatter is not simple `key: value` lines: no closing `---`, `model:` + or `effort:` set twice or spelled another way (`model :`, `"model":`), or a + block, flow, anchor, alias, tag, multi-line or partly quoted value. + +A file that cannot be read or written is reported and the run exits 2. So is +every regular (unlinked) agent file while `.agentsync-state.json` cannot be +read, since a dereferenced copy cannot be told apart then. Either way the +other rows are still written. ## Cost @@ -113,11 +162,12 @@ spend is what OpenRouter reports. ## Exit codes -- `0`: every agent got advice. -- `1`: a row is unresolved or skipped. +- `0`: every agent got advice, and with `--apply` every change was written. +- `1`: a row is unresolved or skipped, or `--apply` refused a change. - `2`: no `$OPENROUTER_API_KEY`, bad flags, no repo or root configured, a scan that could not read everything, an agent file that could not be read, or a models list that is unreachable, has no fixed price for the advisor, or has - no Claude family. + no Claude family; or, with `--apply`, an agent file that could not be read or + written, or a regular agent file while the state file could not be read. See [exit codes](exit-codes.md). diff --git a/internal/optimize/optimize.go b/internal/optimize/optimize.go index eb7ca08..e35316f 100644 --- a/internal/optimize/optimize.go +++ b/internal/optimize/optimize.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "math" + "slices" "strconv" "strings" @@ -82,6 +83,9 @@ type Row struct { EffortLowConfidence bool `json:"effort_low_confidence,omitempty"` Reason string `json:"reason,omitempty"` Cost float64 `json:"cost_usd"` + // Applied and ApplyError are set only by a run that writes changes. + Applied bool `json:"applied,omitempty"` + ApplyError string `json:"apply_error,omitempty"` } // Report is one run's rows and the spend OpenRouter reported for them. @@ -353,3 +357,115 @@ func (r Row) Confidence() string { } return fmt.Sprintf("%s %.2f / %s %.2f", r.ModelPick.Value, r.ModelPick.Probability, r.EffortPick.Value, r.EffortPick.Probability) } + +// Edit returns content with its top-level model and effort set and every +// other byte unchanged; an empty value leaves that key alone. A missing key is +// added just before the closing ---. It refuses frontmatter it would have to +// guess at, and any result Current does not read back as asked. +func Edit(content, model, effort string) (string, error) { + lines := strings.SplitAfter(content, "\n") + eol := "\n" + if lines[0] == "---\r\n" { + eol = "\r\n" + } else if lines[0] != "---\n" { + return "", errors.New("the file has no frontmatter to edit") + } + end := 0 + for i := 1; i < len(lines) && end == 0; i++ { + if strings.HasPrefix(lines[i], "---") { + end = i + } + } + if end == 0 || strings.TrimRight(lines[end], "\r\n") != "---" { + return "", errors.New("the frontmatter has no closing --- line") + } + // YAML reads "model :" and "'model':" as the same key, which Frontmatter + // does not; editing beside such a spelling would leave a duplicate key. + at := map[string]int{} + for i := 1; i < end; i++ { + k, _, ok := strings.Cut(lines[i], ":") + if !ok || k == "" || strings.ContainsAny(k[:1], " \t-#") { + continue + } + name := strings.Trim(strings.TrimRight(k, " \t"), `"'`) + if name != "model" && name != "effort" { + continue + } + if k != name { + return "", fmt.Errorf("the frontmatter spells %s: as %q; respell it by hand first", name, k+":") + } + if _, dup := at[name]; dup { + return "", fmt.Errorf("the frontmatter sets %s: twice", name) + } + at[name] = i + } + for _, kv := range [][2]string{{"model", model}, {"effort", effort}} { + key, v := kv[0], kv[1] + if v == "" { + continue + } + i, found := at[key] + if !found { + lines = slices.Insert(lines, end, key+": "+v+eol) + end++ + continue + } + line, ok := setValue(lines[i], v) + if !ok || continued(lines[i+1:end]) { + return "", fmt.Errorf("the frontmatter's %s: is not a simple one-line value", key) + } + lines[i] = line + } + out := strings.Join(lines, "") + if m, e := Current(out); (model != "" && m != model) || (effort != "" && e != effort) { + return "", fmt.Errorf("the edited frontmatter reads back as %s / %s", m, e) + } + return out, nil +} + +// setValue swaps the value on a "key: value" line, keeping the spacing, any +// quotes and a trailing comment. It refuses a block, flow, anchor, alias or tag +// value, and a quoted one that is not a single complete quoted string. +func setValue(line, v string) (string, bool) { + text := strings.TrimRight(line, "\r\n") + key, rest, _ := strings.Cut(text, ":") + body := strings.TrimLeft(rest, " \t") + sp := rest[:len(rest)-len(body)] + if sp == "" { + sp = " " + } + // The comment is found as Field finds it, so the value read back is the one written. + s := " " + body + cut := len(s) + for _, sep := range []string{" #", "\t#"} { + if i := strings.Index(s, sep); i >= 0 && i < cut { + cut = i + } + } + val := s[1:max(cut, 1)] + core := strings.TrimRight(val, " \t") + if core != "" && strings.ContainsAny(core[:1], "|>[{&*!") { + return "", false + } + if core != "" && strings.ContainsAny(core[:1], `"'`) { + q := core[:1] + if len(core) < 2 || !strings.HasSuffix(core, q) || strings.Contains(core[1:len(core)-1], q) { + return "", false + } + v = q + v + q + } + return key + ":" + sp + v + val[len(core):] + s[cut:] + line[len(text):], true +} + +// continued reports whether the lines after a key carry more of its value: an +// indented or list line before the next top-level one. +func continued(next []string) bool { + for _, l := range next { + t := strings.TrimSpace(l) + if t == "" { + continue + } + return strings.ContainsAny(l[:1], " \t-") + } + return false +} diff --git a/internal/optimize/optimize_test.go b/internal/optimize/optimize_test.go index 01ed8e9..b758b3b 100644 --- a/internal/optimize/optimize_test.go +++ b/internal/optimize/optimize_test.go @@ -343,3 +343,73 @@ func TestRow_Display(t *testing.T) { t.Error("the zero row suggests something") } } + +func TestEdit(t *testing.T) { + const body = "body mentions\nmodel: opus\n" + for _, tc := range []struct { + name, in, model, effort, want, err string + }{ + {name: "replace both", in: "---\nname: a\nmodel: opus\neffort: high\n---\n" + body, model: "sonnet", effort: "low", + want: "---\nname: a\nmodel: sonnet\neffort: low\n---\n" + body}, + {name: "empty leaves a key alone", in: "---\nmodel: opus\neffort: high\n---\n", effort: "max", + want: "---\nmodel: opus\neffort: max\n---\n"}, + {name: "insert both", in: "---\nname: a\n---\n" + body, model: "haiku", effort: "medium", + want: "---\nname: a\nmodel: haiku\neffort: medium\n---\n" + body}, + {name: "insert one beside a replace", in: "---\neffort: high\n---\n", model: "opus", effort: "low", + want: "---\neffort: low\nmodel: opus\n---\n"}, + {name: "quotes and comment kept", in: "---\nmodel: \"opus\" # pinned\neffort: 'high'\t# why\n---\n", model: "sonnet", effort: "low", + want: "---\nmodel: \"sonnet\" # pinned\neffort: 'low'\t# why\n---\n"}, + {name: "comment-only and empty values", in: "---\nmodel: # todo\neffort:\n---\n", model: "opus", effort: "low", + want: "---\nmodel: opus # todo\neffort: low\n---\n"}, + {name: "CRLF", in: "---\r\nname: a\r\nmodel: opus\r\n---\r\nbody\r\n", model: "sonnet", effort: "high", + want: "---\r\nname: a\r\nmodel: sonnet\r\neffort: high\r\n---\r\nbody\r\n"}, + {name: "closing line at end of file", in: "---\nmodel: opus\n---", model: "haiku", want: "---\nmodel: haiku\n---"}, + {name: "blank line then next key", in: "---\nmodel: opus\n\nname: a\n---\n", model: "haiku", want: "---\nmodel: haiku\n\nname: a\n---\n"}, + {name: "nested model line is not the key", in: "---\nx:\n model: opus\n---\n", model: "haiku", want: "---\nx:\n model: opus\nmodel: haiku\n---\n"}, + {name: "no frontmatter", in: "body\nmodel: opus\n", model: "haiku", err: "no frontmatter"}, + {name: "empty file", in: "", model: "haiku", err: "no frontmatter"}, + {name: "no closing line", in: "---\nmodel: opus\n", model: "haiku", err: "no closing --- line"}, + {name: "closing line with more on it", in: "---\nmodel: opus\n--- x\n", model: "haiku", err: "no closing --- line"}, + {name: "duplicate key", in: "---\neffort: high\neffort: low\n---\n", effort: "max", err: "sets effort: twice"}, + {name: "block scalar", in: "---\nmodel: |\n opus\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "flow value", in: "---\nmodel: [opus]\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "indented continuation", in: "---\nmodel:\n\n opus\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "list continuation", in: "---\neffort:\n- high\n---\n", effort: "low", err: "effort: is not a simple one-line value"}, + {name: "key-only and nested lines skipped", in: "---\n: x\n model: opus\n- effort: low\n# model: x\n---\n", model: "haiku", + want: "---\n: x\n model: opus\n- effort: low\n# model: x\nmodel: haiku\n---\n"}, + {name: "other keys quoted or repeated", in: "---\n\"description\": x\ntools: a\ntools: b\nmodel: opus\n---\n", model: "haiku", + want: "---\n\"description\": x\ntools: a\ntools: b\nmodel: haiku\n---\n"}, + {name: "duplicate of the key not edited", in: "---\nmodel: opus\nmodel: haiku\n---\n", effort: "low", err: "sets model: twice"}, + {name: "space before colon", in: "---\nmodel : opus\n---\n", model: "haiku", err: `spells model: as "model :"`}, + {name: "tab before colon", in: "---\nmodel\t: opus\n---\n", model: "haiku", err: `spells model: as "model\t:"`}, + {name: "double-quoted key", in: "---\n\"model\": opus\n---\n", model: "haiku", err: `spells model: as "\"model\":"`}, + {name: "single-quoted key of the other part", in: "---\n'effort': high\n---\n", model: "haiku", err: `spells effort: as "'effort':"`}, + {name: "anchor", in: "---\nmodel: &m opus\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "alias", in: "---\nmodel: *m\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "tag", in: "---\nmodel: !!str opus\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "tab-indented continuation", in: "---\nmodel:\n\topus\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "unclosed quote", in: "---\nmodel: \"opus\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "lone quote", in: "---\nmodel: '\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "text after a quoted value", in: "---\nmodel: \"opus\" x\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "escaped quote inside", in: "---\nmodel: 'it''s'\n---\n", model: "haiku", err: "model: is not a simple one-line value"}, + {name: "quoted value with a comment", in: "---\nmodel: 'opus' # c\n---\n", model: "haiku", want: "---\nmodel: 'haiku' # c\n---\n"}, + {name: "value that does not read back", in: "---\nmodel: opus\n---\n", model: "a #b", err: "reads back as a / (not set)"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := Edit(tc.in, tc.model, tc.effort) + if tc.err != "" { + if err == nil || !strings.Contains(err.Error(), tc.err) || got != "" { + t.Fatalf("Edit = %q, %v; want error containing %q", got, err, tc.err) + } + return + } + if err != nil || got != tc.want { + t.Fatalf("Edit = %q, %v; want %q", got, err, tc.want) + } + m, e := Current(got) + if (tc.model != "" && m != tc.model) || (tc.effort != "" && e != tc.effort) { + t.Errorf("Current(edited) = %s / %s", m, e) + } + }) + } +}