diff --git a/cli/internal/converge/converge.go b/cli/internal/converge/converge.go index 69ec28cb8..6ba6a8dd3 100644 --- a/cli/internal/converge/converge.go +++ b/cli/internal/converge/converge.go @@ -52,6 +52,15 @@ type Reconciler interface { Probe(env Env) error } +// Revisiter is a reconciler whose result depends on what a later one changes: +// the run applies it again right after that reconciler's apply changed the +// machine. records-harness revisits after tools, because it deploys the +// instruction files of the agents on PATH and tools installs agents (#2013 +// D11). A plan never revisits: nothing it reports has been made yet. +type Revisiter interface { + RevisitAfter() string // the reconciler whose change re-runs this one +} + // Status is the outcome of one reconciler in a run. type Status int @@ -117,6 +126,7 @@ func Select(reconcilers []Reconciler, names []string) ([]Reconciler, error) { func Run(reconcilers []Reconciler, env Env, dryRun bool) (Report, error) { rep := Report{DryRun: dryRun, GOOS: env.GOOS} var failed error + var ran []Reconciler gate := "" for _, r := range reconcilers { switch { @@ -135,10 +145,32 @@ func Run(reconcilers []Reconciler, env Env, dryRun bool) (Report, error) { if dryRun && g != "" { gate = g } + if err == nil && !dryRun && entry.Status == StatusChange { + failed = revisit(&rep, ran, r.Name(), env) + } + ran = append(ran, r) } return rep, failed } +// revisit applies again each reconciler that already ran and revisits after +// the one named, in registry order, and appends their entries. The first +// failure stops it and is returned, as Run's own loop does. +func revisit(rep *Report, ran []Reconciler, after string, env Env) error { + for _, q := range ran { + if rv, ok := q.(Revisiter); !ok || rv.RevisitAfter() != after || !platform.Supports(q.Platforms(), env.GOOS) { + continue + } + entry, _, err := runOne(q, env, false) + entry.Detail = "after " + after + ": " + entry.Detail + rep.Entries = append(rep.Entries, entry) + if err != nil { + return fmt.Errorf("converge: %s: %w", q.Name(), err) + } + } + return nil +} + func runOne(r Reconciler, env Env, dryRun bool) (Entry, string, error) { e := Entry{Name: r.Name()} if !platform.Supports(r.Platforms(), env.GOOS) { diff --git a/cli/internal/converge/converge_test.go b/cli/internal/converge/converge_test.go index 79a4c4c6f..d10ae62c8 100644 --- a/cli/internal/converge/converge_test.go +++ b/cli/internal/converge/converge_test.go @@ -20,6 +20,7 @@ type fake struct { skip string applied, planned, probed bool + applies int } func (f *fake) Name() string { return f.name } @@ -30,6 +31,7 @@ func (f *fake) Reconcile(_ Env, dryRun bool) (Result, error) { f.planned = true } else { f.applied = true + f.applies++ } if f.skip != "" { return Result{Skip: f.skip}, f.err @@ -177,3 +179,116 @@ func TestWriteReport_RecordsTheRunAndItsOutcome(t *testing.T) { } } } + +// revisiting is a fake that declares the step whose change re-runs it. +type revisiting struct { + *fake + after string +} + +func (r revisiting) RevisitAfter() string { return r.after } + +// An agent the tools step installs needs the instruction files a step before +// it skipped, so a step whose apply changed the machine re-runs the earlier +// reconcilers that declared it (#2013 D11): measured on macos-latest, the +// second converge deployed .copilot/copilot-instructions.md and was not a no-op. +func TestRun_AChangeRerunsTheStepsThatRevisitIt(t *testing.T) { + harness := revisiting{&fake{name: "harness"}, "tools"} + tools, later := &fake{name: "tools", changes: 2}, &fake{name: "later"} + + rep, err := Run([]Reconciler{harness, tools, later}, Env{GOOS: "linux"}, false) + if err != nil { + t.Fatal(err) + } + if harness.applies != 2 { + t.Errorf("harness applied %d times, want 2 (its own turn, then after tools)", harness.applies) + } + var names []string + for _, e := range rep.Entries { + names = append(names, e.Name) + } + if got := strings.Join(names, " "); got != "harness tools harness later" { + t.Errorf("entries %q, want the revisit right after tools", got) + } + if d := rep.Entries[2].Detail; !strings.HasPrefix(d, "after tools: ") { + t.Errorf("revisit detail %q does not say what re-ran it", d) + } +} + +// A converged machine pays nothing: no change, no revisit. A plan never +// revisits, since nothing it reports has been installed yet. +func TestRun_NoChangeOrAPlanRevisitsNothing(t *testing.T) { + harness := revisiting{&fake{name: "harness"}, "tools"} + if _, err := Run([]Reconciler{harness, &fake{name: "tools"}}, Env{GOOS: "linux"}, false); err != nil { + t.Fatal(err) + } + if harness.applies != 1 { + t.Errorf("a converged tools step re-ran harness: %d applies", harness.applies) + } + + elsewhere := revisiting{&fake{name: "harness", platforms: []string{"darwin"}}, "tools"} + rep, err := Run([]Reconciler{elsewhere, &fake{name: "tools", changes: 1}}, Env{GOOS: "windows"}, false) + if err != nil || len(rep.Entries) != 2 { + t.Errorf("a step this OS does not run was revisited: %d entries, err %v", len(rep.Entries), err) + } + + planned := revisiting{&fake{name: "harness"}, "tools"} + rep, err = Run([]Reconciler{planned, &fake{name: "tools", changes: 1}}, Env{GOOS: "linux"}, true) + if err != nil { + t.Fatal(err) + } + if len(rep.Entries) != 2 || planned.applied { + t.Errorf("a plan revisited: %d entries, applied %v", len(rep.Entries), planned.applied) + } +} + +// failsOnRevisit passes its own turn and fails its probe when re-run. +type failsOnRevisit struct{ revisiting } + +func (f failsOnRevisit) Probe(Env) error { + if f.applies > 1 { + return errors.New("instruction file missing") + } + return nil +} + +// A failing revisit fails the run like any step, naming it, and nothing after +// it runs. +func TestRun_AFailedRevisitStopsTheRun(t *testing.T) { + harness := failsOnRevisit{revisiting{&fake{name: "harness"}, "tools"}} + tools, later := &fake{name: "tools", changes: 1}, &fake{name: "later"} + + rep, err := Run([]Reconciler{harness, tools, later}, Env{GOOS: "linux"}, false) + if err == nil || !strings.Contains(err.Error(), "converge: harness:") { + t.Fatalf("err = %v, want the run failed naming harness", err) + } + if later.applied { + t.Error("a step after the failed revisit ran") + } + if got := statuses(rep); got != "harness=ok tools=change harness=failed later=skipped" { + t.Errorf("statuses: %s", got) + } +} + +// The registry wires D11: records-harness revisits after tools, and tools runs +// after it, so the revisit is reachable. +func TestRegistry_RecordsHarnessRevisitsAfterTools(t *testing.T) { + reg := Registry(Options{}) + pos := map[string]int{} + for i, r := range reg { + pos[r.Name()] = i + } + var h Reconciler + for _, r := range reg { + if r.Name() == "records-harness" { + h = r + } + } + rv, ok := h.(Revisiter) + if !ok || rv.RevisitAfter() != "tools" { + t.Fatalf("records-harness does not revisit after tools") + } + if pos["tools"] <= pos["records-harness"] { + t.Errorf("tools (%d) must run after records-harness (%d)", pos["tools"], pos["records-harness"]) + } +} diff --git a/cli/internal/converge/harness.go b/cli/internal/converge/harness.go index 0bb74c525..46f0888d9 100644 --- a/cli/internal/converge/harness.go +++ b/cli/internal/converge/harness.go @@ -28,8 +28,12 @@ type recordsHarness struct { has func(string) bool // is a command on PATH } -func (recordsHarness) Name() string { return "records-harness" } -func (recordsHarness) Platforms() []string { return []string{"linux", "darwin"} } +func (recordsHarness) Name() string { return "records-harness" } + +// RevisitAfter re-runs the deploy after tools changed the machine: an agent +// tools installs had no instruction files when this step first ran (#2013 D11). +func (recordsHarness) RevisitAfter() string { return "tools" } +func (recordsHarness) Platforms() []string { return []string{"linux", "darwin"} } func (r recordsHarness) Reconcile(env Env, dryRun bool) (Result, error) { stale, err := r.staleInstructions(env) diff --git a/specs/PLAT-001b-one-entrypoint/verification.md b/specs/PLAT-001b-one-entrypoint/verification.md index a6722fb47..e4f35c001 100644 --- a/specs/PLAT-001b-one-entrypoint/verification.md +++ b/specs/PLAT-001b-one-entrypoint/verification.md @@ -72,6 +72,8 @@ Brief log of non-obvious trade-offs or course corrections taken during the work. - Not the contract's `required_path_entries`, applied at the entrypoint: on a fresh machine the contract only exists after the checkout step, and it does not name the mise shims. Not `Environment=PATH=` in the unit: that fixes one caller on one OS, and the Windows task and a future launchd job would each need a copy. - A manager still out of reach after that (mise or uv placed where no listed dir points) fails the probe, naming the manager, rather than accept a wait. +- #2013 D11 (owner, 2026-10-10): an agent installed during the run gets its instruction files during that run. records-harness stays before tools (#2016, instructions-first) and declares `RevisitAfter() = "tools"`; the runner applies it again right after an apply in which tools changed the machine, reported as its own entry (`after tools: …`). Found by X1 on macos-latest: tools installed copilot after records-harness had skipped it, so the second converge deployed `.copilot/copilot-instructions.md`. A converged machine and a plan revisit nothing; a revisit's failure stops the run like any step. Alternatives declined: moving records-harness after tools (opens a window with an agent and no instructions) and gating on the catalog (compile-harness.sh gates on PATH too, so both sides would change). + ## Promotion candidates Answer each line `yes: `, naming the file you promoted, or `no: `. `dotf spec archive` refuses a line left unanswered, a `no` without a reason, and a `yes` whose file does not exist; a `00_meta/` path is looked up in the vault.