From 723a38609260ef599b4c69801093e75cbc99f95b Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 17:37:53 +0200 Subject: [PATCH 01/17] tm-config-audit-j09 Add addRoot/removeRoot/getRoots to IWorktreeApi Add addRoot/removeRoot/getRoots members to IWorktreeApi (src/Shared/Types.fs). Stub both record literals in src/Server/WorktreeApi.fs: readOnlyApi (permanent demo/fixture no-op stubs, roots stay []) and worktreeApi (temporary stubs pending tm-config-audit-9ow). Also commit the feature spec docs/spec/worktree-roots-config.md. --- docs/spec/worktree-roots-config.md | 167 +++++++++++++++++++++++++++++ src/Server/WorktreeApi.fs | 12 ++- src/Shared/Types.fs | 5 +- 3 files changed, 181 insertions(+), 3 deletions(-) create mode 100644 docs/spec/worktree-roots-config.md diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md new file mode 100644 index 0000000..85206d9 --- /dev/null +++ b/docs/spec/worktree-roots-config.md @@ -0,0 +1,167 @@ +# Worktree Roots in Global Config (CLI-managed, server-mediated) + +## Goals + +Make the set of watched worktree roots a **single machine-level setting** owned by the server, +managed live through the `tm` CLI — eliminating the PowerShell-managed `.treemon.config`, the +stale orphan `~/.treemon/roots.json`, and the restart-to-apply cycle. + +- One source of truth for roots: `~/.treemon/config.json` → `"worktreeRoots": [...]`. +- The **server is the only writer** of `config.json` (fixes the cross-process clobber hazard + where PowerShell's whole-file overwrite would wipe server-written keys like `collapsedRepos`). +- `tm add ` / `tm remove ` **persist immediately** (server-side, single writer); + the change is applied when the server (re)starts. The `treemon.ps1 add/remove` shims trigger + that restart automatically when the production server is running (matching today's UX). +- `treemon.ps1` keeps only lifecycle duties (`start/stop/restart/status/dev/deploy/log`) and no + longer needs a path for `start`/`dev` once roots are configured. +- One-time migration of existing `.treemon.config` and the orphan `~/.treemon/roots.json` into + the global config, then delete both. + +Non-goals (explicitly deferred): consolidating the *other* duplicated config readers +(`TreemonConfig` vs `WorktreeApi` global helpers) and introducing typed config records +(Solutions A/B from the investigation), and splitting `.treemon.json` shared-vs-local +(Solution C). This spec is Solution **D** only. + +## Expected Behavior + +### CLI +- `tm add [...]` — validates each path exists, normalizes it (absolute, + trailing-slash trimmed), calls the server, which persists it to global config. On success + prints `✓ Added (applies on next server restart)`. Adding an already-watched path is a + no-op success. Requires the server running (online-only); applied on the next (re)start. +- `tm remove [...]` — calls the server, which removes it from global config. On + success prints `✓ Removed (applies on next server restart)`. Removing an unknown path + reports a clear error. Removing the last remaining root is allowed (empty dashboard is valid). +- `tm roots` — lists currently watched roots (so users/scripts can inspect without the UI). +- All three are online-only: if the server isn't running they print the existing + "server is not running" message (consistent with `tm worktrees`/`new`/`launch`). + +### Server +- On startup, **effective roots** resolve by priority: + 1. roots passed as CLI args (used by `dev`/tests; preserves current arg behavior), + 2. else `worktreeRoots` from `~/.treemon/config.json`, + 3. else (migration) import the orphan `~/.treemon/roots.json` if present. +- After resolving, if `config.json` has no `worktreeRoots`, the server **persists** the resolved + roots into it (so a migrated/arg-provided set becomes the durable source of truth), and deletes + the orphan `roots.json` once imported. +- `--demo` and `--test-fixtures` modes are unchanged (roots stay `[]`, scheduler behavior as today). +- `addRoot`/`removeRoot` validate and persist to global config (single, locked writer); they + return `Result` so the CLI can surface failures (bad path, etc.). They do not + mutate the running watch set — roots are re-read at (re)start. + +### treemon.ps1 +- `start` / `dev` no longer require a path argument; with no path they let the server use the + global config. A path argument still works (passed as args → highest priority). +- `status` shows the watched roots by querying the server (via `tm roots`/endpoint), not by + reading a local file. +- `add` / `remove` become **thin shims** that call `tm add` / `tm remove` and then, if the + production server is running, restart it to apply (same effect as today). Usage text, AGENTS.md, + and README are updated accordingly. +- On any invocation, if a legacy `.treemon.config` exists in the script dir, its roots are passed + to the next `start` as args (so the server migrates+persists them) and the file is deleted. + +## Technical Approach + +### 1. Shared API surface (`src/Shared/Types.fs`, IWorktreeApi ~248-274) +Add three members (paths as plain `string`, consistent with existing simple endpoints): +```fsharp +addRoot: string -> Async> +removeRoot: string -> Async> +getRoots: unit -> Async +``` +No extra Fable.Remoting registration is needed beyond implementing them in the record +(`Remoting.fromValue api` in `Program.fs`). + +### 2. Global-config roots read/write + write lock (`src/Server/WorktreeApi.fs` ~174-234) +- Reuse the existing `withConfigDocument` (read) and `updateGlobalConfig` (RMW) helpers — add + `readWorktreeRootsConfig () : string list` and `writeWorktreeRoots (roots: string list)`. +- **Add a private lock object guarding `updateGlobalConfig`** (it currently has none). All + global-config writes (collapsedRepos, canvas state, lastViewedHashes, and the new roots) go + through the locked helper so concurrent API calls can't corrupt `config.json`. This is the + mechanism that makes "server is the single, serialized writer" true. +- **Test isolation:** the global-config directory must be overridable so endpoint tests don't + touch the real `~/.treemon`. Prefer test-side isolation (the fixture sets the server process's + `USERPROFILE`/`HOME` to a temp dir); only if that proves insufficient, honor a + `TREEMON_CONFIG_DIR` env override in `globalConfigPath`. + +### 3. Scheduler — no change (restart-to-apply) +Roots are read fresh from global config at each server (re)start (§5), so the scheduler needs no +runtime add/remove machinery. `rootPaths` continues to be built once in `start` (~671). *(If live +updates are wanted later, this is the spot: add `RootPaths` to `DashboardState`, `AddRoot`/ +`RemoveRoot` `StateMsg` cases, and have the refresh loop read roots from state.)* + +### 4. API endpoint implementation (`src/Server/WorktreeApi.fs` worktreeApi ~455-466) +- Implement `addRoot`/`removeRoot`: normalize+validate path → read-modify-write via + `writeWorktreeRoots` (§2). No scheduler message. `getRoots` returns `readWorktreeRootsConfig ()`. +- `getWorktrees`/`createWorktree`/path-validation keep using the `rootPaths` captured at startup + (line 466) — correct, since roots only change across restarts. + +### 5. Server startup resolution + migration (`src/Server/Program.fs` ~46-98, 141-186) +- **Allow empty roots in `parseArgs` (~46-98).** Today the final match arm (~95-98) prints usage and + `exit 1` whenever no positional root is given and `--demo` is absent. Change it so zero root args is + valid in normal mode (`WorktreeRoots = []`); leave `--demo`/`--test-fixtures` handling intact. This + is the change that lets `start`/`dev` run with no path. +- Add a root-resolution step before `RefreshScheduler.start`/`worktreeApi`: args → global config + → orphan import (per Expected Behavior). Persist resolved roots to global config if absent; + delete `~/.treemon/roots.json` after import. +- Pass the resolved roots to both `RefreshScheduler.start` and `worktreeApi` as today. Demo/ + fixture modes bypass this (unchanged). + +### 6. CLI subcommands (`src/Cli/Program.fs`) +Add `addCmd`, `removeCmd`, `rootsCmd` following the `newCmd` idiom (~164-187): `command "add" { +inputs (...) ; setAction handler }`, calling via `runApi`/`tryCallServer`. Register them in +`main` (~218-227). `add`/`remove` accept one-or-more paths; loop calling the endpoint per path. + +### 7. treemon.ps1 cleanup + migration (`treemon.ps1`) +- Delete `Save-Config`, `Get-SavedConfig`, `Resolve-WorktreeRoots`, `Add-Roots`, `Remove-Roots`, + the legacy `WorktreeRoot` (singular) fallback, and the `add`/`remove` switch arms. +- `start`/`dev`: make the path argument optional; pass through whatever paths are given. +- `restart`/`deploy`: stop relying on `Get-SavedConfig` — restart with no path args (server reads + global config). +- `status`: list roots via `tm roots`. +- Migration shim: if `$ConfigFile` (`.treemon.config`) exists, read its `WorktreeRoots`, pass them + to the next `Start-ProductionServer` as args (one time), then delete the file. + +### 8. Docs (`docs/spec/worktree-monitor.md`, `AGENTS.md`, `README.md`) +Update the config section + the `add`/`remove` command lines to describe `tm add`/`tm remove`, +the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. + +## Decisions + +- **Restart-to-apply (chosen for simpler code).** `addRoot`/`removeRoot` only persist to global + config; the new roots take effect when the server (re)starts, which the `treemon.ps1` + add/remove shims trigger automatically when the server is running (matching today's behavior). + This avoids the scheduler-state machinery a live model would need (§3). Live application remains + a clean future extension. +- **Server is the single writer of `config.json`**, with an added write lock (§2). The CLI never + writes config files (online-only, confirmed). This is the fix for the cross-process clobber + + the missing intra-process lock. +- **add/remove require the server running** (online-only CLI). Cold bootstrap: `treemon.ps1 start` + with no path launches an empty dashboard (empty roots is valid, as in demo mode), then `tm add` + populates it. +- **Roots become a per-machine singleton.** Dev and prod instances on the same machine now share + one global roots list, instead of independent per-script-dir `.treemon.config` files. Accepted + as the intended simplification. +- **Keep `treemon.ps1 add/remove` as thin shims** that call `tm` then restart the server if + running. `tm` is installed on PATH by `deploy`. Persistence is server-side (single writer); + process lifecycle stays in PowerShell — clean separation. +- **Orphan `~/.treemon/roots.json`** is migrated-then-deleted by the server; `.treemon.config` is + migrated-then-deleted by `treemon.ps1`. + +## Key Files + +| File | Role in this change | +|---|---| +| `src/Shared/Types.fs` | `IWorktreeApi` — add `addRoot`/`removeRoot`/`getRoots` | +| `src/Server/WorktreeApi.fs` | global-config roots read/write + write lock; endpoint impls; read live roots | +| `src/Server/RefreshScheduler.fs` | no change (roots re-read at restart); future home for live updates | +| `src/Server/Program.fs` | startup root resolution + orphan migration; pass resolved roots | +| `src/Cli/Program.fs` | `add`/`remove`/`roots` subcommands | +| `treemon.ps1` | strip roots logic; optional path for start/dev; status via `tm roots`; `.treemon.config` migration | +| `docs/spec/worktree-monitor.md`, `AGENTS.md`, `README.md` | docs | + +## Related Specs +- `docs/spec/worktree-monitor.md` — overall architecture and the existing config section this + amends. +- Investigation: `.agents/config-files-investigation.md` — full config inventory and the A/B/C/D + options; this spec implements D. diff --git a/src/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index b3f95bf..732a69f 100644 --- a/src/Server/WorktreeApi.fs +++ b/src/Server/WorktreeApi.fs @@ -67,7 +67,11 @@ let readOnlyApi archiveCanvasDoc = fun _ -> async { return Error $"Archive canvas doc is not available in {modeName}" } saveLastViewedHashes = fun _ -> async { return () } loadLastViewedHashes = fun () -> async { return Map.empty } - getBridgeLiveness = fun _ -> async { return Map.empty } } + getBridgeLiveness = fun _ -> async { return Map.empty } + // Roots are read-only/no-op in demo and fixture modes (roots stay []). + addRoot = fun _ -> async { return Ok() } + removeRoot = fun _ -> async { return Ok() } + getRoots = fun () -> async { return [] } } let private archiveCanvasDocImpl (request: ArchiveCanvasDocRequest) = let path = WorktreePath.value request.WorktreePath @@ -733,4 +737,8 @@ let worktreeApi archiveCanvasDocImpl req) saveLastViewedHashes = fun hashes -> async { writeLastViewedHashes hashes } loadLastViewedHashes = fun () -> async { return readLastViewedHashes () } - getBridgeLiveness = fun paths -> async { return CanvasBridge.getAllLiveness paths } } + getBridgeLiveness = fun paths -> async { return CanvasBridge.getAllLiveness paths } + // Stubs — real implementation lands in tm-config-audit-9ow. + addRoot = fun _ -> async { return Ok() } + removeRoot = fun _ -> async { return Ok() } + getRoots = fun () -> async { return [] } } diff --git a/src/Shared/Types.fs b/src/Shared/Types.fs index 720fecf..9b5872e 100644 --- a/src/Shared/Types.fs +++ b/src/Shared/Types.fs @@ -271,4 +271,7 @@ type IWorktreeApi = archiveCanvasDoc: ArchiveCanvasDocRequest -> Async> saveLastViewedHashes: Map> -> Async loadLastViewedHashes: unit -> Async>> - getBridgeLiveness: string list -> Async> } + getBridgeLiveness: string list -> Async> + addRoot: string -> Async> + removeRoot: string -> Async> + getRoots: unit -> Async } From c27c1646c0bf52f22ffb5427d782cb4a35346aa8 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 18:01:46 +0200 Subject: [PATCH 02/17] tm-config-audit-bjo Global-config roots read/write with write lock Add globalConfigLock serializing updateGlobalConfig (single serialized writer of config.json). Add internal readWorktreeRootsConfig/writeWorktreeRoots for the worktreeRoots key via withConfigDocument/updateGlobalConfig, and globalConfigDir() honoring TREEMON_CONFIG_DIR for in-process test isolation. Add WorktreeRootsConfigTests (round-trip, no-clobber, overwrite, empty-clear, absent-file) registered in Tests.fsproj. --- docs/spec/worktree-roots-config.md | 12 ++++ src/Server/WorktreeApi.fs | 78 ++++++++++++++++------ src/Tests/Tests.fsproj | 1 + src/Tests/WorktreeRootsConfigTests.fs | 93 +++++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 src/Tests/WorktreeRootsConfigTests.fs diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index 85206d9..302861a 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -147,6 +147,18 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. process lifecycle stays in PowerShell — clean separation. - **Orphan `~/.treemon/roots.json`** is migrated-then-deleted by the server; `.treemon.config` is migrated-then-deleted by `treemon.ps1`. +- **Config-dir test isolation uses `TREEMON_CONFIG_DIR`, not a fixture-set `USERPROFILE`/`HOME`.** + The spec preferred redirecting `USERPROFILE`/`HOME`, but on Windows .NET 9 + `Environment.GetFolderPath(SpecialFolder.UserProfile)` reads the user token, **not** the env + vars (empirically confirmed), so in-process unit tests can't redirect it that way. Per the + spec's allowed fallback, `WorktreeApi.globalConfigDir ()` honors a `TREEMON_CONFIG_DIR` override + (the directory that holds `config.json`, i.e. the `~/.treemon` equivalent); `globalConfigDir` is + `internal` so §5's `Program.fs` orphan-`roots.json` handling can resolve the same dir. Note this + override only covers `WorktreeApi`'s global reads/writes; the deferred-consolidation duplicate + reader in `TreemonConfig.fs` still targets the real `~/.treemon` — so a future endpoint/server + test that needs `TreemonConfig`-mediated global reads isolated must run in a separate process + whose `USERPROFILE`/`HOME` point at a temp dir (a server process *does* honor those for path + building), or that reader must also adopt the override. ## Key Files diff --git a/src/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index 732a69f..6708d11 100644 --- a/src/Server/WorktreeApi.fs +++ b/src/Server/WorktreeApi.fs @@ -175,11 +175,20 @@ let private resolveProvider (state: RefreshScheduler.DashboardState) (path: stri |> Map.tryFind path |> Option.bind (fun data -> data.Provider |> Option.orElse data.LastMessageProvider)) +/// Directory holding the machine-level Treemon config (`config.json`), normally `~/.treemon`. +/// The `TREEMON_CONFIG_DIR` override exists for test isolation: on Windows +/// `Environment.GetFolderPath(UserProfile)` ignores the USERPROFILE/HOME env vars, so an +/// in-process test can only redirect the config dir via this explicit override. +let internal globalConfigDir () = + match Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") with + | null | "" -> + Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), + ".treemon") + | dir -> dir + let private globalConfigPath () = - Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".treemon", - "config.json") + Path.Combine(globalConfigDir (), "config.json") let private withConfigDocument (defaultValue: 'a) (f: System.Text.Json.JsonElement -> 'a) : 'a = let path = globalConfigPath () @@ -213,30 +222,63 @@ let private readCollapsedRepos () : Set = |> Set.ofSeq | _ -> Set.empty) +/// Serializes every write to the machine-level `config.json`. All global-config writers +/// (collapsedRepos, canvas state, lastViewedHashes, worktreeRoots) funnel through +/// `updateGlobalConfig`, so this lock makes the server the single serialized writer and stops +/// concurrent read-modify-write cycles from clobbering each other's keys. +let private globalConfigLock = obj () + let private updateGlobalConfig (description: string) (update: System.Text.Json.Nodes.JsonObject -> unit) = - let configPath = globalConfigPath () - try - let dir = Path.GetDirectoryName(configPath) - if not (Directory.Exists(dir)) then Directory.CreateDirectory(dir) |> ignore + lock globalConfigLock (fun () -> + let configPath = globalConfigPath () + try + let dir = Path.GetDirectoryName(configPath) + if not (Directory.Exists(dir)) then Directory.CreateDirectory(dir) |> ignore - let root = - if File.Exists(configPath) then - try File.ReadAllText(configPath) |> System.Text.Json.Nodes.JsonNode.Parse :?> System.Text.Json.Nodes.JsonObject - with _ -> System.Text.Json.Nodes.JsonObject() - else System.Text.Json.Nodes.JsonObject() + let root = + if File.Exists(configPath) then + try File.ReadAllText(configPath) |> System.Text.Json.Nodes.JsonNode.Parse :?> System.Text.Json.Nodes.JsonObject + with _ -> System.Text.Json.Nodes.JsonObject() + else System.Text.Json.Nodes.JsonObject() - update root + update root - let options = System.Text.Json.JsonSerializerOptions(WriteIndented = true) - File.WriteAllText(configPath, root.ToJsonString(options)) - with ex -> - Log.log "Config" $"Failed to save {description}: {ex.Message}" + let options = System.Text.Json.JsonSerializerOptions(WriteIndented = true) + File.WriteAllText(configPath, root.ToJsonString(options)) + with ex -> + Log.log "Config" $"Failed to save {description}: {ex.Message}") let private writeCollapsedRepos (repos: RepoId list) = updateGlobalConfig "collapsed repos" (fun root -> let repoArray = System.Text.Json.Nodes.JsonArray(repos |> List.map (fun (RepoId s) -> System.Text.Json.Nodes.JsonValue.Create(s) :> System.Text.Json.Nodes.JsonNode) |> List.toArray) root["collapsedRepos"] <- repoArray) +/// Reads the machine-level set of watched worktree roots (`worktreeRoots` in `config.json`). +/// Returns `[]` when the key (or file) is absent. `internal` so the startup resolver +/// (`Program.fs`) and the `getRoots` endpoint can share the single reader. +let internal readWorktreeRootsConfig () : string list = + withConfigDocument [] (fun root -> + match root.TryGetProperty("worktreeRoots") with + | true, prop when prop.ValueKind = System.Text.Json.JsonValueKind.Array -> + prop.EnumerateArray() + |> Seq.choose (fun el -> + if el.ValueKind = System.Text.Json.JsonValueKind.String then Some(el.GetString()) + else None) + |> List.ofSeq + | _ -> []) + +/// Persists the watched worktree roots through the locked single-writer path, leaving every +/// other global-config key untouched. `internal` so the startup resolver and the +/// `addRoot`/`removeRoot` endpoints all write through this one helper. +let internal writeWorktreeRoots (roots: string list) = + updateGlobalConfig "worktree roots" (fun root -> + let rootArray = + System.Text.Json.Nodes.JsonArray( + roots + |> List.map (fun r -> System.Text.Json.Nodes.JsonValue.Create(r) :> System.Text.Json.Nodes.JsonNode) + |> List.toArray) + root["worktreeRoots"] <- rootArray) + let private readCanvasPaneOpen () : bool = withConfigDocument false (fun root -> match root.TryGetProperty("canvasPaneOpen") with diff --git a/src/Tests/Tests.fsproj b/src/Tests/Tests.fsproj index decc754..d044bd0 100644 --- a/src/Tests/Tests.fsproj +++ b/src/Tests/Tests.fsproj @@ -36,6 +36,7 @@ + diff --git a/src/Tests/WorktreeRootsConfigTests.fs b/src/Tests/WorktreeRootsConfigTests.fs new file mode 100644 index 0000000..8b9d00b --- /dev/null +++ b/src/Tests/WorktreeRootsConfigTests.fs @@ -0,0 +1,93 @@ +module Tests.WorktreeRootsConfigTests + +open System +open System.IO +open System.Text.Json +open NUnit.Framework +open Server.WorktreeApi + +/// Isolates the machine-level Treemon config dir to a throwaway temp dir via the +/// TREEMON_CONFIG_DIR override. This is required, not merely convenient: on Windows +/// Environment.GetFolderPath(UserProfile) ignores the USERPROFILE/HOME env vars, so the +/// override is the only way to keep these in-process tests off the real ~/.treemon. +let private withTempConfigDir (action: string -> unit) = + let tempDir = Path.Combine(Path.GetTempPath(), $"treemon-roots-test-{Guid.NewGuid()}") + Directory.CreateDirectory(tempDir) |> ignore + let original = Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") + Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", tempDir) + + try + action tempDir + finally + Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", original) + + try + Directory.Delete(tempDir, recursive = true) + with _ -> + () + +[] +[] +[] +// withTempConfigDir mutates the TREEMON_CONFIG_DIR env var (shared process state) so the +// global-config read/write helpers target a throwaway dir instead of the real ~/.treemon. +// Safe only because NUnit runs sequentially today; NonParallelizable guards against a future +// assembly-parallel switch racing the env var across fixtures. +[] +type WorktreeRootsConfigTests() = + + [] + member _.``readWorktreeRootsConfig returns empty when config file is absent``() = + withTempConfigDir (fun _ -> Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``writeWorktreeRoots then read round-trips roots in order``() = + withTempConfigDir (fun _ -> + let roots = [ @"C:\code\alpha"; @"C:\code\beta" ] + writeWorktreeRoots roots + Assert.That(readWorktreeRootsConfig (), Is.EqualTo(roots))) + + [] + member _.``writeWorktreeRoots preserves unrelated config keys (no clobber)``() = + withTempConfigDir (fun tempDir -> + let configPath = Path.Combine(tempDir, "config.json") + // Pre-seed config.json with keys owned by other features (collapsedRepos, canvas, editor). + let seed = + """{ + "collapsedRepos": [ "repo-one", "repo-two" ], + "editor": "vim", + "canvasPaneOpen": true +}""" + File.WriteAllText(configPath, seed) + + writeWorktreeRoots [ @"C:\code\gamma" ] + + // New key persisted. + Assert.That(readWorktreeRootsConfig (), Is.EqualTo([ @"C:\code\gamma" ])) + + // Pre-existing keys survive the read-modify-write. + use doc = JsonDocument.Parse(File.ReadAllText configPath) + let root = doc.RootElement + Assert.That(root.GetProperty("editor").GetString(), Is.EqualTo("vim")) + Assert.That(root.GetProperty("canvasPaneOpen").GetBoolean(), Is.True) + + let collapsed = + root.GetProperty("collapsedRepos").EnumerateArray() + |> Seq.map (fun e -> e.GetString()) + |> List.ofSeq + + Assert.That(collapsed, Is.EqualTo([ "repo-one"; "repo-two" ]))) + + [] + member _.``writeWorktreeRoots overwrites a previously written roots value``() = + withTempConfigDir (fun _ -> + writeWorktreeRoots [ @"C:\code\one" ] + writeWorktreeRoots [ @"C:\code\two"; @"C:\code\three" ] + Assert.That(readWorktreeRootsConfig (), Is.EqualTo([ @"C:\code\two"; @"C:\code\three" ]))) + + [] + member _.``writeWorktreeRoots with empty list clears roots (removing last root is valid)``() = + withTempConfigDir (fun _ -> + writeWorktreeRoots [ @"C:\code\one" ] + writeWorktreeRoots [] + Assert.That(readWorktreeRootsConfig (), Is.Empty)) From 3767bb3a75399ab2e2ee9c592fe4ac21cabd3c40 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 18:34:54 +0200 Subject: [PATCH 03/17] tm-config-audit-9ow Implement addRoot/removeRoot/getRoots endpoints (restart-to-apply) Wire worktreeApi endpoints to internal addRootToConfig/removeRootFromConfig helpers (normalize+validate path, read-modify-write via writeWorktreeRoots); getRoots returns readWorktreeRootsConfig(). updateGlobalConfig now returns Result so persistence failures surface to the CLI; best-effort UI-state writers |> ignore it. Tests +11. Decision recorded in spec. --- docs/spec/worktree-roots-config.md | 11 +++ src/Server/WorktreeApi.fs | 74 ++++++++++++-- src/Tests/WorktreeRootsConfigTests.fs | 136 ++++++++++++++++++++++++-- 3 files changed, 206 insertions(+), 15 deletions(-) diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index 302861a..b004e12 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -160,6 +160,17 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. whose `USERPROFILE`/`HOME` point at a temp dir (a server process *does* honor those for path building), or that reader must also adopt the override. +- **`addRoot`/`removeRoot` surface persistence failures.** `updateGlobalConfig` was changed to + return `Result` (it previously logged-and-swallowed write exceptions, which would + have made the endpoints report a false `Ok()` on a failed write); `writeWorktreeRoots` + propagates that outcome so the endpoints return `Error` when the config write fails. The four + best-effort UI-state writers (collapsedRepos, canvas open/position, lastViewedHashes) explicitly + `|> ignore` the result — their `save*` members are `Async` and a dropped UI-state write is + non-critical. The roots read-modify-write reads (`readWorktreeRootsConfig`) outside the lock and + writes (`writeWorktreeRoots`) inside it; the read-then-write window is accepted because add/remove + are driven by the serialized, online-only CLI (one path per call), so it is uncontended in + practice. A future live model would move the read inside the locked `updateGlobalConfig` callback. + ## Key Files | File | Role in this change | diff --git a/src/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index 6708d11..5bd1f2b 100644 --- a/src/Server/WorktreeApi.fs +++ b/src/Server/WorktreeApi.fs @@ -228,7 +228,7 @@ let private readCollapsedRepos () : Set = /// concurrent read-modify-write cycles from clobbering each other's keys. let private globalConfigLock = obj () -let private updateGlobalConfig (description: string) (update: System.Text.Json.Nodes.JsonObject -> unit) = +let private updateGlobalConfig (description: string) (update: System.Text.Json.Nodes.JsonObject -> unit) : Result = lock globalConfigLock (fun () -> let configPath = globalConfigPath () try @@ -245,13 +245,16 @@ let private updateGlobalConfig (description: string) (update: System.Text.Json.N let options = System.Text.Json.JsonSerializerOptions(WriteIndented = true) File.WriteAllText(configPath, root.ToJsonString(options)) + Ok() with ex -> - Log.log "Config" $"Failed to save {description}: {ex.Message}") + Log.log "Config" $"Failed to save {description}: {ex.Message}" + Error $"Failed to save {description}: {ex.Message}") let private writeCollapsedRepos (repos: RepoId list) = updateGlobalConfig "collapsed repos" (fun root -> let repoArray = System.Text.Json.Nodes.JsonArray(repos |> List.map (fun (RepoId s) -> System.Text.Json.Nodes.JsonValue.Create(s) :> System.Text.Json.Nodes.JsonNode) |> List.toArray) root["collapsedRepos"] <- repoArray) + |> ignore // best-effort UI state; failures are logged in updateGlobalConfig /// Reads the machine-level set of watched worktree roots (`worktreeRoots` in `config.json`). /// Returns `[]` when the key (or file) is absent. `internal` so the startup resolver @@ -268,9 +271,10 @@ let internal readWorktreeRootsConfig () : string list = | _ -> []) /// Persists the watched worktree roots through the locked single-writer path, leaving every -/// other global-config key untouched. `internal` so the startup resolver and the -/// `addRoot`/`removeRoot` endpoints all write through this one helper. -let internal writeWorktreeRoots (roots: string list) = +/// other global-config key untouched. Returns the write outcome so the `addRoot`/`removeRoot` +/// endpoints can surface a persistence failure to the CLI instead of reporting a false success. +/// `internal` so the startup resolver can also write through this one helper. +let internal writeWorktreeRoots (roots: string list) : Result = updateGlobalConfig "worktree roots" (fun root -> let rootArray = System.Text.Json.Nodes.JsonArray( @@ -279,6 +283,52 @@ let internal writeWorktreeRoots (roots: string list) = |> List.toArray) root["worktreeRoots"] <- rootArray) +/// Canonical comparison form for a worktree root: absolute path with trailing separators +/// trimmed. Total (never throws) so it is safe to fold over already-stored roots — a malformed +/// stored entry falls back to its raw value rather than aborting the whole add/remove. +let private canonicalRoot (path: string) = + try Path.GetFullPath(path).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + with _ -> path + +/// Normalizes a caller-supplied root path (absolute, trailing separators trimmed), surfacing a +/// readable error for blank or malformed input so the CLI can report it. +let private tryNormalizeRoot (path: string) : Result = + if String.IsNullOrWhiteSpace path then Error "Path is empty." + else + try Ok(Path.GetFullPath(path).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)) + with ex -> Error $"Invalid path '{path}': {ex.Message}" + +/// Adds a worktree root to the global config (restart-to-apply). Normalizes and verifies the +/// path is an existing directory, then read-modify-writes the roots list via the locked +/// single-writer helpers, surfacing a persistence failure rather than a false success. Adding an +/// already-watched path is a no-op success. add/remove are driven by the (serialized) +/// `tm add`/`tm remove` CLI, so the read-then-write is not contended in practice; the write +/// itself is serialized by `globalConfigLock`. +let internal addRootToConfig (path: string) : Result = + tryNormalizeRoot path + |> Result.bind (fun normalized -> + if not (Directory.Exists normalized) then + Error $"Path does not exist or is not a directory: {normalized}" + else + let existing = readWorktreeRootsConfig () + if existing |> List.exists (fun r -> pathEquals (canonicalRoot r) normalized) then + Ok() + else + writeWorktreeRoots (existing @ [ normalized ])) + +/// Removes a worktree root from the global config (restart-to-apply). Does not require the path +/// to still exist on disk (a deleted root is removable); reports an error when the path is not +/// currently watched, and surfaces a persistence failure instead of a false success. +let internal removeRootFromConfig (path: string) : Result = + tryNormalizeRoot path + |> Result.bind (fun normalized -> + let existing = readWorktreeRootsConfig () + let remaining = existing |> List.filter (fun r -> not (pathEquals (canonicalRoot r) normalized)) + if List.length remaining = List.length existing then + Error $"Not a watched root: {normalized}" + else + writeWorktreeRoots remaining) + let private readCanvasPaneOpen () : bool = withConfigDocument false (fun root -> match root.TryGetProperty("canvasPaneOpen") with @@ -289,6 +339,7 @@ let private readCanvasPaneOpen () : bool = let private writeCanvasPaneOpen (isOpen: bool) = updateGlobalConfig "canvas pane open state" (fun root -> root["canvasPaneOpen"] <- System.Text.Json.Nodes.JsonValue.Create(isOpen)) + |> ignore // best-effort UI state; failures are logged in updateGlobalConfig let private readCanvasPosition () : CanvasPosition = withConfigDocument CanvasPosition.Right (fun root -> @@ -311,6 +362,7 @@ let private writeCanvasPosition (position: CanvasPosition) = | CanvasPosition.Bottom -> "bottom" updateGlobalConfig "canvas position" (fun root -> root["canvasPosition"] <- System.Text.Json.Nodes.JsonValue.Create(value)) + |> ignore // best-effort UI state; failures are logged in updateGlobalConfig let private readLastViewedHashes () : Map> = withConfigDocument Map.empty (fun root -> @@ -340,6 +392,7 @@ let private writeLastViewedHashes (hashes: Map>) = innerObj[filename] <- System.Text.Json.Nodes.JsonValue.Create(hash)) outerObj[worktreePath] <- innerObj) root["lastViewedHashes"] <- outerObj) + |> ignore // best-effort UI state; failures are logged in updateGlobalConfig let private getEditorConfig () = let config = readGlobalConfig () @@ -780,7 +833,10 @@ let worktreeApi saveLastViewedHashes = fun hashes -> async { writeLastViewedHashes hashes } loadLastViewedHashes = fun () -> async { return readLastViewedHashes () } getBridgeLiveness = fun paths -> async { return CanvasBridge.getAllLiveness paths } - // Stubs — real implementation lands in tm-config-audit-9ow. - addRoot = fun _ -> async { return Ok() } - removeRoot = fun _ -> async { return Ok() } - getRoots = fun () -> async { return [] } } + // Roots are managed restart-to-apply: persist to global config only (no scheduler + // message, no live-roots read). getWorktrees/createWorktree/path-validation keep using + // the `rootPaths` captured at startup above — correct, since roots only change across + // (re)starts (the treemon.ps1 add/remove shims trigger the restart). + addRoot = fun path -> async { return addRootToConfig path } + removeRoot = fun path -> async { return removeRootFromConfig path } + getRoots = fun () -> async { return readWorktreeRootsConfig () } } diff --git a/src/Tests/WorktreeRootsConfigTests.fs b/src/Tests/WorktreeRootsConfigTests.fs index 8b9d00b..c239514 100644 --- a/src/Tests/WorktreeRootsConfigTests.fs +++ b/src/Tests/WorktreeRootsConfigTests.fs @@ -26,6 +26,21 @@ let private withTempConfigDir (action: string -> unit) = with _ -> () +/// Compares two roots the way the endpoints do: absolute, trailing separators trimmed, +/// case-insensitive. Lets assertions ignore the exact normalized form of the temp dir. +let private sameRoot (a: string) (b: string) = + let canon (p: string) = + Path.GetFullPath(p).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + String.Equals(canon a, canon b, StringComparison.OrdinalIgnoreCase) + +/// Asserts a `Result` is `Ok`, surfacing the error message on failure. Avoids the +/// `Is.EqualTo(Ok())` pitfall: the literal's error type infers as `obj`, so NUnit's structural +/// compare never matches the actual `Result` even when both are `Ok ()`. +let private assertOk (result: Result) = + match result with + | Ok () -> () + | Error msg -> Assert.Fail $"expected Ok but got Error: {msg}" + [] [] [] @@ -44,7 +59,7 @@ type WorktreeRootsConfigTests() = member _.``writeWorktreeRoots then read round-trips roots in order``() = withTempConfigDir (fun _ -> let roots = [ @"C:\code\alpha"; @"C:\code\beta" ] - writeWorktreeRoots roots + assertOk (writeWorktreeRoots roots) Assert.That(readWorktreeRootsConfig (), Is.EqualTo(roots))) [] @@ -60,7 +75,7 @@ type WorktreeRootsConfigTests() = }""" File.WriteAllText(configPath, seed) - writeWorktreeRoots [ @"C:\code\gamma" ] + assertOk (writeWorktreeRoots [ @"C:\code\gamma" ]) // New key persisted. Assert.That(readWorktreeRootsConfig (), Is.EqualTo([ @"C:\code\gamma" ])) @@ -81,13 +96,122 @@ type WorktreeRootsConfigTests() = [] member _.``writeWorktreeRoots overwrites a previously written roots value``() = withTempConfigDir (fun _ -> - writeWorktreeRoots [ @"C:\code\one" ] - writeWorktreeRoots [ @"C:\code\two"; @"C:\code\three" ] + assertOk (writeWorktreeRoots [ @"C:\code\one" ]) + assertOk (writeWorktreeRoots [ @"C:\code\two"; @"C:\code\three" ]) Assert.That(readWorktreeRootsConfig (), Is.EqualTo([ @"C:\code\two"; @"C:\code\three" ]))) [] member _.``writeWorktreeRoots with empty list clears roots (removing last root is valid)``() = withTempConfigDir (fun _ -> - writeWorktreeRoots [ @"C:\code\one" ] - writeWorktreeRoots [] + assertOk (writeWorktreeRoots [ @"C:\code\one" ]) + assertOk (writeWorktreeRoots []) + Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + // ----- addRoot/removeRoot endpoint logic (tm-config-audit-9ow) ----- + // Compares two roots the way the endpoints do (absolute, trailing-separator trimmed, + // case-insensitive) so assertions don't hinge on the exact normalized form of the temp dir. + + [] + member _.``addRootToConfig persists an existing directory``() = + withTempConfigDir (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + assertOk (addRootToConfig root) + let roots = readWorktreeRootsConfig () + Assert.That(roots.Length, Is.EqualTo(1)) + Assert.That(sameRoot roots.[0] root, Is.True)) + + [] + member _.``addRootToConfig rejects a path that does not exist``() = + withTempConfigDir (fun tempDir -> + let missing = Path.Combine(tempDir, "does-not-exist") + match addRootToConfig missing with + | Error msg -> Assert.That(msg, Is.Not.Empty) + | Ok () -> Assert.Fail "expected Error for a non-existent path" Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``addRootToConfig rejects a blank path``() = + withTempConfigDir (fun _ -> + match addRootToConfig " " with + | Error msg -> Assert.That(msg, Is.Not.Empty) + | Ok () -> Assert.Fail "expected Error for a blank path") + + [] + member _.``addRootToConfig is an idempotent no-op for an already-watched path``() = + withTempConfigDir (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + assertOk (addRootToConfig root) + // Re-adding the same path with a trailing separator normalizes to the same root and + // must not duplicate it. + assertOk (addRootToConfig (root + string Path.DirectorySeparatorChar)) + Assert.That(readWorktreeRootsConfig().Length, Is.EqualTo(1))) + + [] + member _.``addRootToConfig appends roots preserving insertion order``() = + withTempConfigDir (fun tempDir -> + let alpha = Path.Combine(tempDir, "alpha") + let beta = Path.Combine(tempDir, "beta") + Directory.CreateDirectory(alpha) |> ignore + Directory.CreateDirectory(beta) |> ignore + addRootToConfig alpha |> assertOk + addRootToConfig beta |> assertOk + let roots = readWorktreeRootsConfig () + Assert.That(roots.Length, Is.EqualTo(2)) + Assert.That(sameRoot roots.[0] alpha, Is.True) + Assert.That(sameRoot roots.[1] beta, Is.True)) + + [] + member _.``addRootToConfig preserves unrelated config keys``() = + withTempConfigDir (fun tempDir -> + let configPath = Path.Combine(tempDir, "config.json") + File.WriteAllText(configPath, """{ "editor": "vim" }""") + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + addRootToConfig root |> assertOk + use doc = JsonDocument.Parse(File.ReadAllText configPath) + Assert.That(doc.RootElement.GetProperty("editor").GetString(), Is.EqualTo("vim"))) + + [] + member _.``removeRootFromConfig removes a watched root``() = + withTempConfigDir (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + addRootToConfig root |> assertOk + assertOk (removeRootFromConfig root) + Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``removeRootFromConfig errors when the path is not watched``() = + withTempConfigDir (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + match removeRootFromConfig root with + | Error msg -> Assert.That(msg, Is.Not.Empty) + | Ok () -> Assert.Fail "expected Error for an unwatched path") + + [] + member _.``removeRootFromConfig removes a root whose directory no longer exists``() = + withTempConfigDir (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + addRootToConfig root |> assertOk + Directory.Delete(root) + // Removal must not depend on the directory still existing on disk. + assertOk (removeRootFromConfig root) + Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``addRootToConfig surfaces a persistence failure as Error``() = + withTempConfigDir (fun tempDir -> + // Make config.json an (unwritable) directory so the File.WriteAllText inside the + // locked writer throws. The endpoint must surface that as Error rather than a false + // Ok() — the whole point of the Result contract. + let configPath = Path.Combine(tempDir, "config.json") + Directory.CreateDirectory(configPath) |> ignore + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + match addRootToConfig root with + | Error msg -> Assert.That(msg, Is.Not.Empty) + | Ok () -> Assert.Fail "expected Error when persistence fails") From 5aa5cb39dbb5fb82d1f271f001b5d9ea99254c76 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 19:07:48 +0200 Subject: [PATCH 04/17] tm-config-audit-3ng Server startup root resolution + orphan migration parseArgs accepts zero positional roots in normal mode (dropped roots<>[] guard + unreachable usage arm). Added resolveWorktreeRoots (CLI args > global worktreeRoots > orphan roots.json; persists when config has none; orphan deleted only after successful persist). main computes effectiveRoots (demo/fixture bypass -> []) and passes them to RefreshScheduler.start + WorktreeApi.worktreeApi. Tests: ServerStartupResolutionTests.fs (10 tests); SmokeTests isolate config dir via TREEMON_CONFIG_DIR. --- docs/spec/worktree-roots-config.md | 24 ++++ src/Server/Program.fs | 90 +++++++++++++-- src/Tests/ServerStartupResolutionTests.fs | 135 ++++++++++++++++++++++ src/Tests/SmokeTests.fs | 37 +++++- src/Tests/Tests.fsproj | 1 + 5 files changed, 273 insertions(+), 14 deletions(-) create mode 100644 src/Tests/ServerStartupResolutionTests.fs diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index b004e12..ac1762f 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -171,6 +171,30 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. are driven by the serialized, online-only CLI (one path per call), so it is uncontended in practice. A future live model would move the read inside the locked `updateGlobalConfig` callback. +- **Startup resolution (§5 / `Program.fs`).** `parseArgs` now accepts zero positional roots in + normal mode (the previously-`exit 1` "usage" arm was removed; it had become unreachable once the + `roots <> []` guard was dropped, so the top-level match stays exhaustive with no redundant arm). + `resolveWorktreeRoots` resolves CLI args > global `worktreeRoots` > orphan `roots.json`, and + persists the resolved set **only when `config.json` has no `worktreeRoots` yet** — so a populated + config is never clobbered and CLI args act as an *ephemeral* override there (they win for the run + but don't rewrite the durable config). The orphan is read by a *pure* reader and deleted **only + after a successful persist** (not before): an eager read-then-delete would silently lose the + migrated set if the config write failed. Consequently the orphan is consumed only on the + priority-3 path (no args, empty config); with args present (e.g. `treemon.ps1` migrating + `.treemon.config`) the args win and a still-present orphan is left untouched. Orphan roots are + migrated verbatim (no `GetFullPath`/existence check) — downstream comparisons canonicalize at + compare time and the scheduler tolerates missing dirs. +- **Demo/fixture modes pass `[]` to `worktreeApi`/scheduler** (resolution is bypassed entirely). + This is behaviorally inert for fixture mode because `worktreeApi`'s fixture branch is built from + `readOnlyApi` and ignores `rootPaths`; passing `[]` matches the spec's "(roots stay [])". +- **Smoke tests isolate the config dir.** Both `SmokeTests` fixtures start the server in *normal* + mode with real roots, which now triggers the startup persist; without isolation that would write + the developer's real `~/.treemon`. Each fixture points the child server at a throwaway + `TREEMON_CONFIG_DIR` and deletes it in teardown. New in-process coverage lives in + `src/Tests/ServerStartupResolutionTests.fs` (parseArgs empty-roots + resolveWorktreeRoots + priority/persist/no-clobber/orphan-migration), isolated via the same `TREEMON_CONFIG_DIR` override + used by `WorktreeRootsConfigTests`. + ## Key Files | File | Role in this change | diff --git a/src/Server/Program.fs b/src/Server/Program.fs index 73f2c0e..a9327f3 100644 --- a/src/Server/Program.fs +++ b/src/Server/Program.fs @@ -81,7 +81,10 @@ let parseArgs (args: string array) = CanvasPort = None TestFixtures = None Demo = true } - | roots, port, canvasPort, testFixtures, _ when roots <> [] -> + | roots, port, canvasPort, testFixtures, _ -> + // Zero positional roots is valid in normal mode: `start`/`dev` no longer require a path. + // When no roots are passed the server resolves them from global config (or migrates a + // legacy/orphan set) at startup — see resolveWorktreeRoots in main. match canvasPort with | Some cp when cp = port -> eprintfn $"--canvas-port ({cp}) must differ from the main --port ({port})" @@ -92,10 +95,6 @@ let parseArgs (args: string array) = CanvasPort = canvasPort TestFixtures = testFixtures Demo = false } - | _ -> - eprintfn "Usage: Server [...] [--port ] [--canvas-port | --no-canvas] [--test-fixtures ]" - eprintfn " Server --demo [--port ]" - exit 1 let private populateAgentFromFixtures (agent: MailboxProcessor) (fixtures: FixtureData) = fixtures.Worktrees.Repos @@ -136,6 +135,72 @@ let private buildRemotingHandler (api: IWorktreeApi) = Propagate ex.Message) |> Remoting.buildHttpHandler +/// Path of the orphan `roots.json` under the (TREEMON_CONFIG_DIR-aware) global config dir. The +/// file is a stale migration artifact read by nothing per the config investigation. +let private orphanRootsPath () = + System.IO.Path.Combine(WorktreeApi.globalConfigDir (), "roots.json") + +/// Reads the orphan `roots.json` (schema `{ "WorktreeRoots": [...] }`), returning its roots or +/// `[]` when absent/unreadable. Pure read — the file is deleted only after its roots are durably +/// persisted (see resolveWorktreeRoots), so a failed config write can never lose the migrated set. +let private readOrphanRoots () : string list = + let orphanPath = orphanRootsPath () + if not (System.IO.File.Exists orphanPath) then [] + else + try + use doc = System.Text.Json.JsonDocument.Parse(System.IO.File.ReadAllText orphanPath) + match doc.RootElement.TryGetProperty("WorktreeRoots") with + | true, prop when prop.ValueKind = System.Text.Json.JsonValueKind.Array -> + prop.EnumerateArray() + |> Seq.choose (fun el -> + if el.ValueKind = System.Text.Json.JsonValueKind.String then Some(el.GetString()) + else None) + |> List.ofSeq + | _ -> [] + with ex -> + Log.log "Startup" $"Failed to read orphan roots.json: {ex.Message}" + [] + +/// Removes the orphan `roots.json` once its roots have been migrated into the global config. +/// Best-effort: a delete failure is logged, not fatal (the migrated roots are already persisted). +let private deleteOrphanRoots () = + let orphanPath = orphanRootsPath () + if System.IO.File.Exists orphanPath then + try + System.IO.File.Delete orphanPath + Log.log "Startup" "Deleted migrated orphan roots.json" + with ex -> + Log.log "Startup" $"Failed to delete orphan roots.json: {ex.Message}" + +/// Resolves the effective worktree roots at startup by priority: +/// 1. roots passed as CLI args (used by `dev`/tests; preserves current arg behavior), +/// 2. else `worktreeRoots` from the global `config.json`, +/// 3. else a one-time import of the orphan `roots.json`. +/// When `config.json` has no `worktreeRoots` yet, the resolved set is persisted so an arg-provided +/// or migrated set becomes the durable source of truth (a populated config is left untouched, so +/// CLI args act as an ephemeral override there). An orphan-sourced set is deleted only after that +/// persist succeeds — never before — so a failed write can't drop the migration. Demo/fixture +/// modes never call this; their roots stay `[]`. +let internal resolveWorktreeRoots (cliRoots: string list) : string list = + let configRoots = WorktreeApi.readWorktreeRootsConfig () + + let resolved, cameFromOrphan = + if not (List.isEmpty cliRoots) then cliRoots, false + elif not (List.isEmpty configRoots) then configRoots, false + else + let orphanRoots = readOrphanRoots () + orphanRoots, not (List.isEmpty orphanRoots) + + if List.isEmpty configRoots && not (List.isEmpty resolved) then + match WorktreeApi.writeWorktreeRoots resolved with + | Ok () -> + Log.log "Startup" $"Persisted {List.length resolved} worktree root(s) to global config" + if cameFromOrphan then deleteOrphanRoots () + | Error msg -> + Log.log "Startup" $"Failed to persist worktree roots: {msg}" + + resolved + [] let main args = let config = parseArgs args @@ -143,7 +208,14 @@ let main args = let serverUrl = $"http://localhost:{config.Port}" Log.init () - config.WorktreeRoots |> List.iter (fun root -> Log.log "Startup" $"Worktree root: {root}") + + // Effective roots: CLI args > global config > orphan import (persisted first-time). Demo and + // fixture modes bypass resolution entirely — they serve synthetic data, so roots stay []. + let worktreeRoots = + if config.Demo || config.TestFixtures.IsSome then [] + else resolveWorktreeRoots config.WorktreeRoots + + worktreeRoots |> List.iter (fun root -> Log.log "Startup" $"Worktree root: {root}") Log.log "Startup" $"Server URL: {serverUrl}" let appVersion = readAppVersion () @@ -155,7 +227,7 @@ let main args = config.TestFixtures |> Option.iter (fun path -> Log.log "Startup" $"Test fixtures: {path}") - config.WorktreeRoots |> List.iter (fun root -> printfn "Monitoring worktrees under: %s" root) + worktreeRoots |> List.iter (fun root -> printfn "Monitoring worktrees under: %s" root) let cts = new CancellationTokenSource() @@ -179,10 +251,10 @@ let main args = Log.log "Startup" $"ERROR: {msg}" System.Environment.Exit(1) | None -> - RefreshScheduler.start agent config.WorktreeRoots cts.Token + RefreshScheduler.start agent worktreeRoots cts.Token Log.log "Startup" "Scheduler background loop started" - WorktreeApi.worktreeApi agent syncAgent sessionAgent config.WorktreeRoots config.TestFixtures appVersion deployBranch + WorktreeApi.worktreeApi agent syncAgent sessionAgent worktreeRoots config.TestFixtures appVersion deployBranch |> buildRemotingHandler, Some agent System.AppDomain.CurrentDomain.ProcessExit.Add(fun _ -> diff --git a/src/Tests/ServerStartupResolutionTests.fs b/src/Tests/ServerStartupResolutionTests.fs new file mode 100644 index 0000000..f0a0858 --- /dev/null +++ b/src/Tests/ServerStartupResolutionTests.fs @@ -0,0 +1,135 @@ +module Tests.ServerStartupResolutionTests + +open System +open System.IO +open System.Text.Json.Nodes +open NUnit.Framework + +// `parseArgs`/`resolveWorktreeRoots` live in the server entry-point file's implicit `Program` +// module; `Server` exposes internals to `Tests` (InternalsVisibleTo), so `resolveWorktreeRoots` +// (internal) is reachable here. +open Program + +/// Isolates the machine-level config dir to a throwaway temp dir via the TREEMON_CONFIG_DIR +/// override (mirrors WorktreeRootsConfigTests). Required, not convenient: on Windows +/// Environment.GetFolderPath(UserProfile) ignores USERPROFILE/HOME, so this is the only way to +/// keep the in-process resolver off the real ~/.treemon. Both the global config read/write and the +/// orphan roots.json lookup resolve under this dir. +let private withTempConfigDir (action: string -> unit) = + let tempDir = Path.Combine(Path.GetTempPath(), $"treemon-startup-test-{Guid.NewGuid()}") + Directory.CreateDirectory(tempDir) |> ignore + let original = Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") + Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", tempDir) + + try + action tempDir + finally + Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", original) + + try + Directory.Delete(tempDir, recursive = true) + with _ -> + () + +/// Writes an orphan `roots.json` (`{ "WorktreeRoots": [...] }`) into the isolated config dir using +/// the JSON node API, so test paths never need manual backslash escaping. +let private writeOrphan (configDir: string) (roots: string list) = + let arr = JsonArray() + roots |> List.iter (fun r -> arr.Add(r)) + let root = JsonObject() + root["WorktreeRoots"] <- arr + File.WriteAllText(Path.Combine(configDir, "roots.json"), root.ToJsonString()) + +[] +[] +[] +// resolveWorktreeRoots tests mutate TREEMON_CONFIG_DIR (shared process state); NonParallelizable +// guards against a future assembly-parallel switch racing the env var across fixtures. +[] +type ServerStartupResolutionTests() = + + // ----- parseArgs: zero positional roots is valid in normal mode ----- + + [] + member _.``parseArgs with no args yields empty roots in normal mode``() = + let config = parseArgs [||] + Assert.That(config.WorktreeRoots, Is.Empty) + Assert.That(config.Demo, Is.False) + Assert.That(config.Port, Is.EqualTo(5000)) + + [] + member _.``parseArgs with only --port yields empty roots and the chosen port``() = + let config = parseArgs [| "--port"; "5050" |] + Assert.That(config.WorktreeRoots, Is.Empty) + Assert.That(config.Demo, Is.False) + Assert.That(config.Port, Is.EqualTo(5050)) + + [] + member _.``parseArgs with a single root keeps that root``() = + let config = parseArgs [| @"C:\code\alpha" |] + Assert.That(config.WorktreeRoots, Is.EqualTo([ @"C:\code\alpha" ])) + Assert.That(config.Demo, Is.False) + + [] + member _.``parseArgs --demo stays demo with empty roots``() = + let config = parseArgs [| "--demo" |] + Assert.That(config.Demo, Is.True) + Assert.That(config.WorktreeRoots, Is.Empty) + + // ----- resolveWorktreeRoots: priority + first-time persistence + orphan migration ----- + + [] + member _.``resolveWorktreeRoots persists CLI args when config has no roots``() = + withTempConfigDir (fun _ -> + let cliRoots = [ @"C:\code\one"; @"C:\code\two" ] + let resolved = resolveWorktreeRoots cliRoots + Assert.That(resolved, Is.EqualTo(cliRoots)) + // First-time persistence: the arg-provided set becomes the durable source of truth. + Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(cliRoots))) + + [] + member _.``resolveWorktreeRoots prefers CLI args but does not overwrite a populated config``() = + withTempConfigDir (fun _ -> + let configured = [ @"C:\code\configured" ] + match Server.WorktreeApi.writeWorktreeRoots configured with + | Ok () -> () + | Error msg -> Assert.Fail $"setup write failed: {msg}" + + let resolved = resolveWorktreeRoots [ @"C:\code\arg" ] + // CLI args win for this run... + Assert.That(resolved, Is.EqualTo([ @"C:\code\arg" ])) + // ...but a populated config is an ephemeral override, not clobbered. + Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(configured))) + + [] + member _.``resolveWorktreeRoots reads global config when no CLI args``() = + withTempConfigDir (fun _ -> + let configured = [ @"C:\code\x"; @"C:\code\y" ] + match Server.WorktreeApi.writeWorktreeRoots configured with + | Ok () -> () + | Error msg -> Assert.Fail $"setup write failed: {msg}" + + let resolved = resolveWorktreeRoots [] + Assert.That(resolved, Is.EqualTo(configured))) + + [] + member _.``resolveWorktreeRoots imports orphan roots.json then persists and deletes it``() = + withTempConfigDir (fun tempDir -> + let orphanRoots = [ @"C:\code\orphan-a"; @"C:\code\orphan-b" ] + writeOrphan tempDir orphanRoots + + let resolved = resolveWorktreeRoots [] + + Assert.That(resolved, Is.EqualTo(orphanRoots)) + // Migrated set persisted into the global config... + Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(orphanRoots)) + // ...and the orphan file is consumed (read-then-delete). + Assert.That(File.Exists(Path.Combine(tempDir, "roots.json")), Is.False)) + + [] + member _.``resolveWorktreeRoots returns empty when no args, no config, no orphan``() = + withTempConfigDir (fun _ -> + let resolved = resolveWorktreeRoots [] + Assert.That(resolved, Is.Empty) + // Nothing to persist, so the config stays absent/empty. + Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.Empty)) diff --git a/src/Tests/SmokeTests.fs b/src/Tests/SmokeTests.fs index 544b530..a45e0a3 100644 --- a/src/Tests/SmokeTests.fs +++ b/src/Tests/SmokeTests.fs @@ -20,7 +20,7 @@ let private serverProjectPath = let private worktreeRoot = @"Q:\code\AITestAgent" let private thisRepoName = Path.GetFileName(repoRoot) -let private startSmokeServerProc (args: string) = +let private startSmokeServerProc (configDir: string) (args: string) = let psi = ProcessStartInfo( FileName = "dotnet", @@ -32,10 +32,17 @@ let private startSmokeServerProc (args: string) = CreateNoWindow = true ) + // Isolate the smoke server's machine-level config dir. Startup root-resolution now persists + // the resolved worktreeRoots into config.json when it has none yet, so without this override a + // normal-mode smoke run would write the developer's real ~/.treemon. Point it at a caller-owned + // throwaway dir instead (WorktreeApi.globalConfigDir honors TREEMON_CONFIG_DIR); the fixture + // deletes it in teardown. + psi.EnvironmentVariables["TREEMON_CONFIG_DIR"] <- configDir + Process.Start(psi) -let private startSmokeServer (port: int) = - startSmokeServerProc $""""{worktreeRoot}" --port {port} --no-canvas""" +let private startSmokeServer (configDir: string) (port: int) = + startSmokeServerProc configDir $""""{worktreeRoot}" --port {port} --no-canvas""" let private startProcess fileName args workingDir envVars redirectOutput = TestUtils.startProcess fileName args workingDir envVars redirectOutput @@ -63,6 +70,10 @@ let private earlyExitMessage (proc: Process) (stdout: Task) (stderr: Tas type SmokeTests() = let smokePort = TestUtils.getFreeTcpPort () let smokeUrl = $"http://localhost:{smokePort}" + // Throwaway machine-config dir for the smoke server (see startSmokeServerProc); deleted in + // teardown so a normal-mode smoke run never persists into — or leaves cruft beside — the real + // ~/.treemon. + let smokeConfigDir = Path.Combine(Path.GetTempPath(), $"treemon-smoke-{Guid.NewGuid():N}") let mutable serverProc: Process option = None let killServer () = @@ -121,7 +132,7 @@ type SmokeTests() = [] member _.StartServer() = task { - let proc = startSmokeServer smokePort + let proc = startSmokeServer smokeConfigDir smokePort serverProc <- Some proc TestContext.Out.WriteLine($"Smoke server started (PID {proc.Id}) on port {smokePort}") @@ -139,6 +150,12 @@ type SmokeTests() = killServer () serverProc <- None + try + if Directory.Exists smokeConfigDir then + Directory.Delete(smokeConfigDir, recursive = true) + with _ -> + () + [] member _.``Server returns IsReady=true with real data``() = Assert.That(readyBody, Does.Contain("\"IsReady\":true"), "API should return IsReady=true after scheduler populates data") @@ -217,6 +234,10 @@ type MultiRepoSmokeTests() = let multiRepoUrl = $"http://localhost:{multiRepoPort}" let multiRepoViteUrl = $"http://localhost:{multiRepoVitePort}" + // Throwaway machine-config dir for this normal-mode server (see startSmokeServerProc); deleted + // in teardown so startup root-resolution never persists into the real ~/.treemon. + let multiRepoConfigDir = Path.Combine(Path.GetTempPath(), $"treemon-smoke-{Guid.NewGuid():N}") + let mutable serverProc: Process option = None let mutable viteProc: Process option = None @@ -270,7 +291,7 @@ type MultiRepoSmokeTests() = |> List.map (fun r -> $"\"{r}\"") |> String.concat " " - let proc = startSmokeServerProc $"""{rootArgs} --port {multiRepoPort} --no-canvas""" + let proc = startSmokeServerProc multiRepoConfigDir $"""{rootArgs} --port {multiRepoPort} --no-canvas""" serverProc <- Some proc TestContext.Out.WriteLine($"Multi-repo smoke server started (PID {proc.Id}) on port {multiRepoPort}") @@ -322,6 +343,12 @@ type MultiRepoSmokeTests() = serverProc <- None viteProc <- None + try + if Directory.Exists multiRepoConfigDir then + Directory.Delete(multiRepoConfigDir, recursive = true) + with _ -> + () + [] member this.NavigateToDashboard() = task { diff --git a/src/Tests/Tests.fsproj b/src/Tests/Tests.fsproj index d044bd0..8712be8 100644 --- a/src/Tests/Tests.fsproj +++ b/src/Tests/Tests.fsproj @@ -37,6 +37,7 @@ + From 56da3673f8fb48d3b234f36896dcf4ff03c58b5e Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 19:29:41 +0200 Subject: [PATCH 05/17] tm-config-audit-am6 CLI add/remove/roots subcommands Add addCmd/removeCmd/rootsCmd in src/Cli/Program.fs and register them in main. add/remove accept one-or-more paths and loop the server endpoint inside a single tryCallServer (single server-down message per batch); per-path errors via eprintfn, exit 1 if any path fails. roots lists configured roots (sanitized) or a no-roots message. Path validation/normalization delegated to server endpoints. --- src/Cli/Program.fs | 63 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/Cli/Program.fs b/src/Cli/Program.fs index d920f0b..aa53c0a 100644 --- a/src/Cli/Program.fs +++ b/src/Cli/Program.fs @@ -215,6 +215,66 @@ let worktreesCmd = setAction handler } +let addCmd = + let handler (paths: string[], port: int option) = + withPort port (fun port -> + // One tryCallServer for the whole batch so the "server is not running" message + // (and the connection check) happens once, not once per path. + tryCallServer port (fun api -> + (0, paths) + ||> Array.fold (fun exitCode path -> + match api.addRoot path |> Async.RunSynchronously with + | Ok() -> printfn $"✓ Added %s{path} (applies on next server restart)"; exitCode + | Error e -> eprintfn $"Error: %s{e}"; 1))) + + command "add" { + description "Add one or more worktree roots to watch (applies on next server restart)" + + inputs ( + argument "paths" |> arity Arity.OneOrMore |> desc "Worktree root path(s) to watch", + optionMaybe "--port" |> desc "Server port (default: 5000, env: TREEMON_PORT)" + ) + + setAction handler + } + +let removeCmd = + let handler (paths: string[], port: int option) = + withPort port (fun port -> + tryCallServer port (fun api -> + (0, paths) + ||> Array.fold (fun exitCode path -> + match api.removeRoot path |> Async.RunSynchronously with + | Ok() -> printfn $"✓ Removed %s{path} (applies on next server restart)"; exitCode + | Error e -> eprintfn $"Error: %s{e}"; 1))) + + command "remove" { + description "Remove one or more worktree roots (applies on next server restart)" + + inputs ( + argument "paths" |> arity Arity.OneOrMore |> desc "Worktree root path(s) to stop watching", + optionMaybe "--port" |> desc "Server port (default: 5000, env: TREEMON_PORT)" + ) + + setAction handler + } + +let rootsCmd = + let handler (port: int option) = + withPort port (fun port -> + tryCallServer port (fun api -> + match api.getRoots() |> Async.RunSynchronously with + | [] -> printfn "No worktree roots configured."; 0 + | roots -> + roots |> List.iter (fun root -> printfn $"%s{sanitizeForTerminal root}") + 0)) + + command "roots" { + description "List the configured worktree roots" + inputs (optionMaybe "--port" |> desc "Server port (default: 5000, env: TREEMON_PORT)") + setAction handler + } + [] let main argv = rootCommand argv { @@ -224,4 +284,7 @@ let main argv = addCommand launchCmd addCommand newCmd addCommand worktreesCmd + addCommand addCmd + addCommand removeCmd + addCommand rootsCmd } From dceafb10e2a5b1e3aae730f6b5d94b2233f85e75 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 20:11:35 +0200 Subject: [PATCH 06/17] tm-config-audit-krj Strip roots logic from treemon.ps1 + migrate .treemon.config Delete Save-Config/Get-SavedConfig/Resolve-WorktreeRoots/Add-Roots/Remove-Roots and the singular WorktreeRoot fallback. Add Read-LegacyRoots (one-time .treemon.config migration reader) and Invoke-Tm (tm CLI wrapper). start/dev/restart are path-optional; status lists roots via 'tm roots'; add/remove are thin shims to tm that restart prod only on CLI success (exit 0). .treemon.config migrated then deleted only after a confirmed server start. Spec Decisions updated with the 3 edge cases. --- docs/spec/worktree-roots-config.md | 14 ++ treemon.ps1 | 301 +++++++++++++---------------- 2 files changed, 149 insertions(+), 166 deletions(-) diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index ac1762f..3a84aac 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -147,6 +147,20 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. process lifecycle stays in PowerShell — clean separation. - **Orphan `~/.treemon/roots.json`** is migrated-then-deleted by the server; `.treemon.config` is migrated-then-deleted by `treemon.ps1`. +- **treemon.ps1 §7 implementation edge cases.** Three robustness details settled while implementing + the shims: (1) **Migration file deleted only after a confirmed start.** `Read-LegacyRoots` is a + pure reader; `Start-ProductionServer` removes `.treemon.config` *after* the post-launch + `HasExited` success check (by which point the server has persisted the roots into the global + config), so a `dotnet publish` failure or an immediate server crash can never lose the migrated + roots. (2) **Null/empty roots are filtered before building args.** `start`/`dev`/`restart` pass + `$WorktreeRoots` straight through, but with `[ValueFromRemainingArguments]` an omitted path binds + the parameter to `$null` (and `@($null)` is a *1-element* array), so `Start-ProductionServer`/ + `Start-DevMode` normalize via `@($Roots | Where-Object { $_ })` — otherwise the "no path" feature + threw on `$null.TrimEnd()` (and `restart` would kill prod then throw). (3) **add/remove restart + only on CLI success.** The shims gate the restart on `tm`'s exit code (`$tmExit -eq 0`) so a + rejected add/remove (bad path, server down) doesn't needlessly bounce prod; `Invoke-Tm` routes + the CLI's stdout to the host (`| Out-Host`) and returns only `[int]$LASTEXITCODE`, so the exit + code stays a scalar and the CLI's own messages aren't swallowed. - **Config-dir test isolation uses `TREEMON_CONFIG_DIR`, not a fixture-set `USERPROFILE`/`HOME`.** The spec preferred redirecting `USERPROFILE`/`HOME`, but on Windows .NET 9 `Environment.GetFolderPath(SpecialFolder.UserProfile)` reads the user token, **not** the env diff --git a/treemon.ps1 b/treemon.ps1 index ba0401f..c808dd8 100644 --- a/treemon.ps1 +++ b/treemon.ps1 @@ -13,6 +13,7 @@ $ErrorActionPreference = "Stop" $ScriptDir = $PSScriptRoot $PidFile = Join-Path $ScriptDir ".treemon.pid" $ConfigFile = Join-Path $ScriptDir ".treemon.config" +$TmScript = Join-Path $ScriptDir "tm.ps1" $LogDir = Join-Path $ScriptDir "logs" $LogFile = Join-Path $LogDir "treemon-prod.log" $PublishDir = Join-Path $ScriptDir ".publish" @@ -27,37 +28,61 @@ if (-not $Command) { Write-Host "Usage: .\treemon.ps1 [worktree-root] [additional-roots...]" -ForegroundColor Cyan Write-Host "" Write-Host "Commands:" -ForegroundColor White - Write-Host " start [...] Start production server (auto-builds if wwwroot/ is empty)" + Write-Host " start [...] Start production server (auto-builds if wwwroot/ is empty)" + Write-Host " No path uses the global config roots (~/.treemon/config.json)" Write-Host " stop Stop the production server" - Write-Host " restart Stop + start (reuses previously configured roots)" - Write-Host " status Show production server status" + Write-Host " restart Stop + start (uses the global config roots)" + Write-Host " status Show production server status (lists roots via 'tm roots')" Write-Host " log Tail the production server log" - Write-Host " dev [...] Start dev mode (server :5001 + Vite :5174), Ctrl+C to stop" + Write-Host " dev [...] Start dev mode (server :5001 + Vite :5174), Ctrl+C to stop" Write-Host " demo Start demo mode with fixture data (server :5001 + Vite :5174)" Write-Host " deploy Build frontend and deploy to wwwroot/ (restarts prod if running)" - Write-Host " add [...] Add watched root(s) to config" + Write-Host " add [...] Add watched root(s) via 'tm add' (restarts prod if running)" Write-Host " -Upstream Set the upstream remote for PR/diff (written to .treemon.json)" - Write-Host " remove Remove a watched root from config" + Write-Host " remove [...] Remove watched root(s) via 'tm remove' (restarts prod if running)" Write-Host " install-skill Install the tm CLI skill for AI coding agents" exit 0 } -function Get-SavedConfig { - if (Test-Path $ConfigFile) { +function Read-LegacyRoots { + # One-time migration of the legacy PowerShell-managed .treemon.config. + # Reads its WorktreeRoots and returns them (or @()). Does NOT delete the file — + # Start-ProductionServer removes it only after the server has actually started + # (and thus persisted the roots into ~/.treemon/config.json), so a publish/start + # failure can't lose the roots. + if (-not (Test-Path $ConfigFile)) { return @() } + + $roots = @() + try { $config = Get-Content $ConfigFile -Raw | ConvertFrom-Json if ($config.PSObject.Properties.Name -contains "WorktreeRoots") { - return @{ WorktreeRoots = @($config.WorktreeRoots) } - } - if ($config.PSObject.Properties.Name -contains "WorktreeRoot") { - return @{ WorktreeRoots = @($config.WorktreeRoot) } + $roots = @($config.WorktreeRoots | Where-Object { $_ }) } + } catch { + Write-Host "Warning: could not parse legacy .treemon.config; it will be removed after the server starts" -ForegroundColor Yellow } - return $null + + if ($roots.Count -gt 0) { + Write-Host "Migrating worktree roots from .treemon.config into global config" -ForegroundColor Gray + } + return @($roots) } -function Save-Config([string[]]$Roots) { - $trimmed = $Roots | ForEach-Object { $_.TrimEnd('\', '/') } - @{ WorktreeRoots = @($trimmed) } | ConvertTo-Json | Set-Content $ConfigFile +function Invoke-Tm([string[]]$TmArgs) { + # Thin wrapper around the local tm CLI (src/Cli via tm.ps1). Returns ONLY the + # CLI's integer exit code; the CLI's own stdout is routed to the host via Out-Host + # so it is not captured into the return value (which would both hide the CLI's + # messages and turn the returned exit code into an array). Wrapped in try/catch so + # a non-zero CLI exit can never abort treemon.ps1. + try { + & $TmScript @TmArgs | Out-Host + if ($null -eq $LASTEXITCODE) { return 0 } + return [int]$LASTEXITCODE + } catch { + Write-Host $_.Exception.Message -ForegroundColor Red + if ($LASTEXITCODE -and $LASTEXITCODE -ne 0) { return [int]$LASTEXITCODE } + return 1 + } } function Get-RunningPid { @@ -69,21 +94,6 @@ function Get-RunningPid { return $null } -function Resolve-WorktreeRoots([string]$Cmd) { - if ($WorktreeRoots -and $WorktreeRoots.Count -gt 0) { return $WorktreeRoots } - - $config = Get-SavedConfig - if ($config) { - $roots = $config.WorktreeRoots -join ", " - Write-Host "Using previously configured worktree roots: $roots" -ForegroundColor Gray - return $config.WorktreeRoots - } - - Write-Host "Error: worktree root path is required for first $Cmd" -ForegroundColor Red - Write-Host "Usage: .\treemon.ps1 $Cmd [...]" -ForegroundColor Gray - exit 1 -} - function Build-Frontend { Push-Location $ScriptDir try { @@ -148,18 +158,27 @@ function Start-ProductionServer([string[]]$Roots) { if (-not (Test-Path $LogDir)) { New-Item -ItemType Directory -Path $LogDir | Out-Null } "" | Set-Content $LogFile - Save-Config $Roots + # Resolve the roots to launch with. Explicit roots (from the command line) win. + # Filter out any $null/empty entries first: with ValueFromRemainingArguments an + # omitted path binds the parameter to $null, and @($null) is a 1-element array. + # With no roots, migrate a legacy .treemon.config (one-time) so the server persists + # it. Empty is valid: the server then resolves roots from its global config. + $effectiveRoots = @($Roots | Where-Object { $_ }) + if ($effectiveRoots.Count -eq 0) { + $effectiveRoots = Read-LegacyRoots + } Write-Host "Publishing server..." -ForegroundColor Cyan dotnet publish -c Release -o $PublishDir src/Server/Server.fsproj if ($LASTEXITCODE -ne 0) { throw "dotnet publish failed" } $serverExe = Join-Path $PublishDir "Treemon.exe" - $rootArgs = ($Roots | ForEach-Object { "`"$($_.TrimEnd('\', '/'))`"" }) -join " " + $rootArgs = ($effectiveRoots | ForEach-Object { "`"$($_.TrimEnd('\', '/'))`"" }) -join " " + $serverArgs = if ($rootArgs) { "$rootArgs --port $DefaultPort" } else { "--port $DefaultPort" } Write-Host "Starting production server on port $DefaultPort..." -ForegroundColor Cyan $process = Start-Process -FilePath $serverExe ` - -ArgumentList "$rootArgs --port $DefaultPort" ` + -ArgumentList $serverArgs ` -WorkingDirectory $ScriptDir ` -RedirectStandardOutput $LogFile ` -RedirectStandardError (Join-Path $LogDir "treemon-prod-stderr.log") ` @@ -182,8 +201,17 @@ function Start-ProductionServer([string[]]$Roots) { exit 1 } + # Server is up — it has resolved+persisted its roots into the global config, so the + # legacy .treemon.config (if any) is now safe to retire. Removing it only after a + # confirmed start means a publish/start failure never loses the migrated roots. + if (Test-Path $ConfigFile) { Remove-Item $ConfigFile -ErrorAction SilentlyContinue } + Write-Host "Production server started (PID: $($process.Id))" -ForegroundColor Green - $Roots | ForEach-Object { Write-Host "Monitoring: $_" -ForegroundColor Gray } + if ($effectiveRoots.Count -gt 0) { + $effectiveRoots | ForEach-Object { Write-Host "Monitoring: $_" -ForegroundColor Gray } + } else { + Write-Host "Monitoring roots from global config (~/.treemon/config.json)" -ForegroundColor Gray + } Write-Host "URL: http://localhost:$DefaultPort" -ForegroundColor Gray Write-Host "Log: $LogFile" -ForegroundColor Gray } @@ -215,15 +243,24 @@ function Show-Status { elseif ($uptime.Hours -gt 0) { "$($uptime.Hours)h $($uptime.Minutes)m" } else { "$($uptime.Minutes)m $($uptime.Seconds)s" } - $config = Get-SavedConfig - Write-Host "Production server is running" -ForegroundColor Green Write-Host " PID: $runningPid" Write-Host " Port: $DefaultPort" Write-Host " Uptime: $uptimeStr" Write-Host " URL: http://localhost:$DefaultPort" - if ($config) { - $config.WorktreeRoots | ForEach-Object { Write-Host " Monitor: $_" } + + # Watched roots come from the server (the single source of truth) via `tm roots`. + $rootLines = @() + try { + $rootsOutput = & $TmScript roots --port $DefaultPort 2>$null + if ($LASTEXITCODE -eq 0) { + $rootLines = @($rootsOutput | Where-Object { $_ -and $_.Trim() -and $_.Trim() -ne "No worktree roots configured." }) + } + } catch { } + if ($rootLines.Count -gt 0) { + $rootLines | ForEach-Object { Write-Host " Monitor: $_" } + } else { + Write-Host " Monitor: (none configured)" } Write-Host " Log: $LogFile" } @@ -293,8 +330,11 @@ function Start-DualProcess([string]$ServerArgs, [string]$ModeName, [string]$Serv } function Start-DevMode([string[]]$Roots) { - $rootArgs = ($Roots | ForEach-Object { "`"$($_.TrimEnd('\', '/'))`"" }) -join " " - Start-DualProcess -ServerArgs $rootArgs -ModeName "Dev" -ServerLabel "dotnet watch" -MonitorPaths $Roots + # Drop any $null/empty entries: an omitted path binds $Roots to $null, and + # @($null) is a 1-element array that would call .TrimEnd() on $null below. + $cleanRoots = @($Roots | Where-Object { $_ }) + $rootArgs = ($cleanRoots | ForEach-Object { "`"$($_.TrimEnd('\', '/'))`"" }) -join " " + Start-DualProcess -ServerArgs $rootArgs -ModeName "Dev" -ServerLabel "dotnet watch" -MonitorPaths $cleanRoots } function Start-DemoMode { @@ -313,89 +353,6 @@ function Set-UpstreamRemote([string]$RepoRoot, [string]$RemoteName) { Write-Host " Upstream remote set to '$RemoteName' for $RepoRoot" -ForegroundColor Green } -function Add-Roots([string[]]$NewRoots) { - $config = Get-SavedConfig - $existing = if ($config) { @($config.WorktreeRoots) } else { @() } - - $added = @() - foreach ($root in $NewRoots) { - $normalized = (Resolve-Path $root).Path.TrimEnd('\', '/') - if ($existing -contains $normalized) { - Write-Host "Already monitored: $normalized" -ForegroundColor Yellow - } else { - $existing += $normalized - $added += $normalized - } - - if ($Upstream) { - Set-UpstreamRemote $normalized $Upstream - } - } - - if ($added.Count -eq 0 -and -not $Upstream) { - Write-Host "No new roots to add" -ForegroundColor Yellow - return - } - - if ($added.Count -gt 0) { - Save-Config $existing - $added | ForEach-Object { Write-Host "Added: $_" -ForegroundColor Green } - } - - $runningPid = Get-RunningPid - if ($runningPid -and $added.Count -gt 0) { - Write-Host "Restarting server to pick up changes..." -ForegroundColor Cyan - Stop-ProductionServer - Start-Sleep -Seconds 1 - Start-ProductionServer $existing - } -} - -function Remove-Roots([string[]]$RootsToRemove) { - $config = Get-SavedConfig - if (-not $config) { - Write-Host "No roots configured" -ForegroundColor Yellow - return - } - - $existing = @($config.WorktreeRoots) - $removed = @() - $remaining = $existing - - foreach ($root in $RootsToRemove) { - $normalized = (Resolve-Path $root).Path.TrimEnd('\', '/') - $filtered = @($remaining | Where-Object { $_ -ne $normalized }) - if ($filtered.Count -eq $remaining.Count) { - Write-Host "Root not found: $normalized" -ForegroundColor Yellow - Write-Host "Current roots:" -ForegroundColor Gray - $existing | ForEach-Object { Write-Host " $_" -ForegroundColor Gray } - } else { - $remaining = $filtered - $removed += $normalized - } - } - - if ($removed.Count -eq 0) { - return - } - - if ($remaining.Count -eq 0) { - Write-Host "Error: cannot remove the last root" -ForegroundColor Red - return - } - - Save-Config $remaining - $removed | ForEach-Object { Write-Host "Removed: $_" -ForegroundColor Green } - - $runningPid = Get-RunningPid - if ($runningPid) { - Write-Host "Restarting server to pick up changes..." -ForegroundColor Cyan - Stop-ProductionServer - Start-Sleep -Seconds 1 - Start-ProductionServer $remaining - } -} - function Install-TmCommand { $shimDir = Join-Path $env:LOCALAPPDATA "tm-cli" $shimFile = Join-Path $shimDir "tm.cmd" @@ -504,17 +461,9 @@ function Deploy-Frontend { $runningPid = Get-RunningPid if ($runningPid) { Write-Host "Restarting production server..." -ForegroundColor Cyan - $config = Get-SavedConfig - $roots = if ($config) { $config.WorktreeRoots } else { $null } - - if (-not $roots) { - Write-Host "Warning: could not find saved worktree roots, skipping restart" -ForegroundColor Yellow - return - } - Stop-ProductionServer Start-Sleep -Seconds 1 - Start-ProductionServer $roots + Start-ProductionServer @() } else { Write-Host "Production server is not running, skipping restart" -ForegroundColor Gray } @@ -522,32 +471,23 @@ function Deploy-Frontend { switch ($Command) { "start" { - $roots = Resolve-WorktreeRoots "start" - $roots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red - exit 1 + if ($WorktreeRoots -and $WorktreeRoots.Count -gt 0) { + $WorktreeRoots | ForEach-Object { + if (-not (Test-Path $_)) { + Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red + exit 1 + } } } - Start-ProductionServer $roots + Start-ProductionServer $WorktreeRoots } "stop" { Stop-ProductionServer } "restart" { - $config = Get-SavedConfig - $roots = if ($WorktreeRoots -and $WorktreeRoots.Count -gt 0) { $WorktreeRoots } - elseif ($config) { $config.WorktreeRoots } - else { $null } - - if (-not $roots) { - Write-Host "Error: no worktree roots configured. Run 'start' first." -ForegroundColor Red - exit 1 - } - Stop-ProductionServer Start-Sleep -Seconds 1 - Start-ProductionServer $roots + Start-ProductionServer $WorktreeRoots } "status" { Show-Status @@ -556,14 +496,15 @@ switch ($Command) { Show-Log } "dev" { - $roots = Resolve-WorktreeRoots "dev" - $roots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red - exit 1 + if ($WorktreeRoots -and $WorktreeRoots.Count -gt 0) { + $WorktreeRoots | ForEach-Object { + if (-not (Test-Path $_)) { + Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red + exit 1 + } } } - Start-DevMode $roots + Start-DevMode $WorktreeRoots } "demo" { Start-DemoMode @@ -577,13 +518,31 @@ switch ($Command) { Write-Host "Usage: .\treemon.ps1 add [...]" -ForegroundColor Gray exit 1 } - $WorktreeRoots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: path does not exist: $_" -ForegroundColor Red - exit 1 + + # Thin shim: the server (single config writer) persists the roots; the change + # applies on the next (re)start, which we trigger below if prod is running. + $tmExit = Invoke-Tm (@("add") + $WorktreeRoots + @("--port", "$DefaultPort")) + + if ($Upstream) { + $WorktreeRoots | ForEach-Object { + if (Test-Path $_) { + Set-UpstreamRemote ((Resolve-Path $_).Path.TrimEnd('\', '/')) $Upstream + } } } - Add-Roots $WorktreeRoots + + # Restart only on a successful add so a failed/rejected CLI call (e.g. bad path, + # server down) doesn't needlessly bounce the production server. + if ($tmExit -eq 0) { + $runningPid = Get-RunningPid + if ($runningPid) { + Write-Host "Restarting server to apply changes..." -ForegroundColor Cyan + Stop-ProductionServer + Start-Sleep -Seconds 1 + Start-ProductionServer @() + } + } + exit $tmExit } "remove" { if (-not $WorktreeRoots -or $WorktreeRoots.Count -eq 0) { @@ -591,13 +550,23 @@ switch ($Command) { Write-Host "Usage: .\treemon.ps1 remove [...]" -ForegroundColor Gray exit 1 } - $WorktreeRoots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: path does not exist: $_" -ForegroundColor Red - exit 1 + + # Thin shim: the server removes the root from global config; applies on next + # (re)start, which we trigger below if prod is running. No existence check — + # a root whose directory was deleted must still be removable. + $tmExit = Invoke-Tm (@("remove") + $WorktreeRoots + @("--port", "$DefaultPort")) + + # Restart only on a successful remove (see 'add' above). + if ($tmExit -eq 0) { + $runningPid = Get-RunningPid + if ($runningPid) { + Write-Host "Restarting server to apply changes..." -ForegroundColor Cyan + Stop-ProductionServer + Start-Sleep -Seconds 1 + Start-ProductionServer @() } } - Remove-Roots $WorktreeRoots + exit $tmExit } "install-skill" { Install-Skill From e42e5e38ed96f3276604565de8a22dd888631803 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 20:17:44 +0200 Subject: [PATCH 07/17] tm-config-audit-l8z Update docs for global roots + tm add/remove Document global worktreeRoots config key (~/.treemon/config.json), tm add/remove/roots CLI, treemon.ps1 add/remove shims, and path-optional start/dev across worktree-monitor.md, AGENTS.md, and README.md. --- AGENTS.md | 8 +++++--- README.md | 5 +++++ docs/spec/worktree-monitor.md | 4 +++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 04d9bff..d05dc58 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,10 +9,10 @@ npm install dotnet test src/Tests/Tests.fsproj # all tests dotnet test src/Tests/Tests.fsproj --filter "Category=Fast" # fast suite (<60s) dotnet test src/Tests/Tests.fsproj --filter "Category=Unit" # unit tests only -.\treemon.ps1 dev "Q:\code\AITestAgent" # dev mode +.\treemon.ps1 dev "Q:\code\AITestAgent" # dev mode (path optional; omit to use global config roots) .\treemon.ps1 deploy # build + restart production -.\treemon.ps1 add "Q:\code\OtherProject" # add a root to config -.\treemon.ps1 remove "Q:\code\OtherProject" # remove a root from config +.\treemon.ps1 add "Q:\code\OtherProject" # add a watched root (shim for 'tm add'; restarts prod if running) +.\treemon.ps1 remove "Q:\code\OtherProject" # remove a watched root (shim for 'tm remove'; restarts prod if running) ``` ## F# Style Guide @@ -85,6 +85,8 @@ This project uses [focused-review](https://github.com/0101/focused-review) for a `treemon.ps1` manages the application lifecycle: `dev`, `deploy`, `start`, `stop`, `restart`, `status`, `log`, `add`, `remove`. +Watched worktree roots live in the global config (`~/.treemon/config.json` → `worktreeRoots`), written only by the server. `start`/`dev` no longer need a path — omit it to use the global roots (an empty list is valid). Manage roots live with the `tm` CLI — `tm add ...`, `tm remove ...`, `tm roots` — or the `treemon.ps1 add`/`remove` shims, which call `tm` and restart production when it is running. Changes persist immediately and apply on the next server (re)start. See `docs/spec/worktree-roots-config.md`. + ## Tech Stack - **Client**: F# with Fable (compiles to JS), Feliz for React bindings, Vite for bundling diff --git a/README.md b/README.md index 9cfeac2..47a5236 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,8 @@ npm install Open http://localhost:5000 — install as a PWA from the browser for a native app experience. +The roots you pass to `start` are saved to the global config (`~/.treemon/config.json` → `worktreeRoots`, written by the server), so afterwards `start`, `restart`, and `dev` no longer need a path — omit it to use the saved roots. Manage roots live with the `tm` CLI (`tm add`, `tm remove`, `tm roots`) or the `.\treemon.ps1 add`/`remove` shims; changes apply on the next server restart. + ### Managing the server ```powershell @@ -79,6 +81,9 @@ tm launch --path C:\code\my-project --fix-tests # fix failing tests tm launch --path C:\code\my-project --create-pr # create a pull request tm new --repo C:\code\my-project --branch feature/foo # create worktree tm worktrees # list all worktrees +tm add C:\code\my-project # watch a root (applies on next server restart) +tm remove C:\code\my-project # stop watching a root +tm roots # list watched roots ``` All commands accept `--port` (default: 5000, env: `TREEMON_PORT`). You can also run `.\tm.ps1` directly from the repo root without installing. diff --git a/docs/spec/worktree-monitor.md b/docs/spec/worktree-monitor.md index c7be211..e2c03c7 100644 --- a/docs/spec/worktree-monitor.md +++ b/docs/spec/worktree-monitor.md @@ -23,7 +23,8 @@ ### Multi-Repo -- Server accepts multiple root paths as positional CLI args +- Watched roots resolve at server startup by priority: CLI args → the global `worktreeRoots` key in `~/.treemon/config.json` → one-time import of the legacy `~/.treemon/roots.json`. With roots configured, `treemon.ps1 start`/`dev` no longer require a path; with no args the server uses the global config (an empty list is valid, as in demo mode). +- Roots are managed live through the `tm` CLI — `tm add ...`, `tm remove ...`, and `tm roots` (list). The server is the single writer of `config.json`; changes persist immediately and take effect on the next server (re)start. The `treemon.ps1 add`/`remove` shims call `tm` and then restart the production server when it is running. - Each root is an independent section — cards never mix across repos - Scheduler picks most-overdue task globally across all repos - Branch events scoped by `{repoId}/{branch}` to prevent cross-repo collisions @@ -254,6 +255,7 @@ After the burst, `lastRuns` is pre-populated and the normal sequential loop take ## Related Specs +- `docs/spec/worktree-roots-config.md` — global `worktreeRoots` config, CLI-managed roots (`tm add`/`remove`/`roots`), server as the single config writer - `docs/spec/user-idle-detection.md` — adaptive refresh cadence based on user activity level - `docs/spec/keyboard-navigation.md` — spatial arrow-key navigation and key bindings - `docs/spec/native-session-management.md` — Windows Terminal spawn/focus/kill via HWND tracking From 8462804fa8a392d2537343f628754a77231b0324 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 21:27:50 +0200 Subject: [PATCH 08/17] tm-config-audit-rf1 Fix empty-vs-absent worktreeRoots conflation that resurrects removed roots on restart Added WorktreeApi.tryReadWorktreeRootsConfig (None=key absent, Some []=explicit empty, Some roots=populated; malformed non-array -> None). readWorktreeRootsConfig is now a thin Option.defaultValue [] wrapper. resolveWorktreeRoots gates BOTH orphan-import fallthrough and first-time persist on key ABSENCE (Option.isSome), never List.isEmpty. 2 regression tests in ServerStartupResolutionTests. --- docs/spec/worktree-roots-config.md | 25 ++++++++++--- src/Server/Program.fs | 33 ++++++++++------- src/Server/WorktreeApi.fs | 22 ++++++++---- src/Tests/ServerStartupResolutionTests.fs | 43 +++++++++++++++++++++++ 4 files changed, 101 insertions(+), 22 deletions(-) diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index 3a84aac..1f39cf7 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -189,15 +189,32 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. normal mode (the previously-`exit 1` "usage" arm was removed; it had become unreachable once the `roots <> []` guard was dropped, so the top-level match stays exhaustive with no redundant arm). `resolveWorktreeRoots` resolves CLI args > global `worktreeRoots` > orphan `roots.json`, and - persists the resolved set **only when `config.json` has no `worktreeRoots` yet** — so a populated - config is never clobbered and CLI args act as an *ephemeral* override there (they win for the run - but don't rewrite the durable config). The orphan is read by a *pure* reader and deleted **only + persists the resolved set **only when `config.json` has no `worktreeRoots` *key* yet** (absence, + not mere emptiness — see the missing-vs-empty decision below) — so a present config is never + clobbered and CLI args act as an *ephemeral* override there (they win for the run but don't + rewrite the durable config). The orphan is read by a *pure* reader and deleted **only after a successful persist** (not before): an eager read-then-delete would silently lose the migrated set if the config write failed. Consequently the orphan is consumed only on the - priority-3 path (no args, empty config); with args present (e.g. `treemon.ps1` migrating + priority-3 path (no args, no `worktreeRoots` key); with args present (e.g. `treemon.ps1` migrating `.treemon.config`) the args win and a still-present orphan is left untouched. Orphan roots are migrated verbatim (no `GetFullPath`/existence check) — downstream comparisons canonicalize at compare time and the scheduler tolerates missing dirs. +- **Missing-vs-empty `worktreeRoots` (fix tm-config-audit-rf1).** Startup resolution must + distinguish a *missing* `worktreeRoots` key (fresh install / pre-migration) from a *present but + empty* one (the user curated every root away). The original `readWorktreeRootsConfig` collapsed + both to `[]`, so an explicit `worktreeRoots:[]` was treated like a fresh install and got + repopulated on restart — a stale orphan `roots.json` resurrected removed roots, and CLI args + overwrote the explicit empty. Fix: `WorktreeApi.tryReadWorktreeRootsConfig () : string list option` + is the presence-aware reader (`None` = key absent, `Some []` = explicit empty, `Some roots` = + populated; a malformed non-array value reports `None` to preserve the old lenient behavior). + `readWorktreeRootsConfig () : string list` stays as a thin `Option.defaultValue []` wrapper so the + `getRoots` endpoint and add/remove read-modify-write are unchanged. `resolveWorktreeRoots` gates + BOTH the orphan-import fallthrough AND the first-time persist on key **absence** + (`Option.isSome configRoots`), never on `List.isEmpty`. Result: a present `worktreeRoots:[]` is + the priority-2 source (resolves to `[]`, no orphan import) and is never persisted over, so removed + roots stay removed across restarts. Regression coverage: + `ServerStartupResolutionTests` — orphan-present and CLI-args-present each leave the explicit empty + intact. - **Demo/fixture modes pass `[]` to `worktreeApi`/scheduler** (resolution is bypassed entirely). This is behaviorally inert for fixture mode because `worktreeApi`'s fixture branch is built from `readOnlyApi` and ignores `rootPaths`; passing `[]` matches the spec's "(roots stay [])". diff --git a/src/Server/Program.fs b/src/Server/Program.fs index a9327f3..8166b4d 100644 --- a/src/Server/Program.fs +++ b/src/Server/Program.fs @@ -174,24 +174,33 @@ let private deleteOrphanRoots () = /// Resolves the effective worktree roots at startup by priority: /// 1. roots passed as CLI args (used by `dev`/tests; preserves current arg behavior), -/// 2. else `worktreeRoots` from the global `config.json`, -/// 3. else a one-time import of the orphan `roots.json`. -/// When `config.json` has no `worktreeRoots` yet, the resolved set is persisted so an arg-provided -/// or migrated set becomes the durable source of truth (a populated config is left untouched, so -/// CLI args act as an ephemeral override there). An orphan-sourced set is deleted only after that -/// persist succeeds — never before — so a failed write can't drop the migration. Demo/fixture -/// modes never call this; their roots stay `[]`. +/// 2. else `worktreeRoots` from the global `config.json` (a PRESENT key, even an explicit empty +/// list, wins here — the user may have curated every root away), +/// 3. else (the key is ABSENT) a one-time import of the orphan `roots.json`. +/// When `config.json` has no `worktreeRoots` KEY yet, the resolved set is persisted so an +/// arg-provided or migrated set becomes the durable source of truth. A present key (even +/// `worktreeRoots:[]`) is left untouched, so CLI args act as an ephemeral override and an explicit +/// empty is never repopulated on restart. An orphan-sourced set is deleted only after that persist +/// succeeds — never before — so a failed write can't drop the migration. Demo/fixture modes never +/// call this; their roots stay `[]`. let internal resolveWorktreeRoots (cliRoots: string list) : string list = - let configRoots = WorktreeApi.readWorktreeRootsConfig () + // `None` = the `worktreeRoots` key is absent (fresh install / pre-migration); `Some roots` = + // the key is present (possibly an explicit empty list). Gating migration on KEY ABSENCE — not + // `List.isEmpty` — is what stops an explicit `worktreeRoots:[]` from being resurrected by a + // stale orphan `roots.json` or overwritten by CLI args on restart. + let configRoots = WorktreeApi.tryReadWorktreeRootsConfig () + let configHasKey = Option.isSome configRoots let resolved, cameFromOrphan = if not (List.isEmpty cliRoots) then cliRoots, false - elif not (List.isEmpty configRoots) then configRoots, false else - let orphanRoots = readOrphanRoots () - orphanRoots, not (List.isEmpty orphanRoots) + match configRoots with + | Some roots -> roots, false + | None -> + let orphanRoots = readOrphanRoots () + orphanRoots, not (List.isEmpty orphanRoots) - if List.isEmpty configRoots && not (List.isEmpty resolved) then + if not configHasKey && not (List.isEmpty resolved) then match WorktreeApi.writeWorktreeRoots resolved with | Ok () -> Log.log "Startup" $"Persisted {List.length resolved} worktree root(s) to global config" diff --git a/src/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index 5bd1f2b..067c627 100644 --- a/src/Server/WorktreeApi.fs +++ b/src/Server/WorktreeApi.fs @@ -256,11 +256,14 @@ let private writeCollapsedRepos (repos: RepoId list) = root["collapsedRepos"] <- repoArray) |> ignore // best-effort UI state; failures are logged in updateGlobalConfig -/// Reads the machine-level set of watched worktree roots (`worktreeRoots` in `config.json`). -/// Returns `[]` when the key (or file) is absent. `internal` so the startup resolver -/// (`Program.fs`) and the `getRoots` endpoint can share the single reader. -let internal readWorktreeRootsConfig () : string list = - withConfigDocument [] (fun root -> +/// Reads the machine-level set of watched worktree roots (`worktreeRoots` in `config.json`), +/// distinguishing a MISSING key (`None`) from a present-but-empty list (`Some []`). The startup +/// resolver depends on that distinction: an explicit `worktreeRoots:[]` means the user curated +/// every root away, so it must NOT be treated like a fresh install and repopulated from CLI args +/// or a stale orphan `roots.json`. A malformed (non-array) value is reported as `None` — absent — +/// matching the original lenient behavior. `internal` so the resolver (`Program.fs`) shares it. +let internal tryReadWorktreeRootsConfig () : string list option = + withConfigDocument None (fun root -> match root.TryGetProperty("worktreeRoots") with | true, prop when prop.ValueKind = System.Text.Json.JsonValueKind.Array -> prop.EnumerateArray() @@ -268,7 +271,14 @@ let internal readWorktreeRootsConfig () : string list = if el.ValueKind = System.Text.Json.JsonValueKind.String then Some(el.GetString()) else None) |> List.ofSeq - | _ -> []) + |> Some + | _ -> None) + +/// Flattens `tryReadWorktreeRootsConfig` to a plain list (missing key -> `[]`) for callers that +/// don't need the missing-vs-empty distinction: the `getRoots` endpoint and the add/remove +/// read-modify-write. `internal` so the startup resolver and the endpoint can share the reader. +let internal readWorktreeRootsConfig () : string list = + tryReadWorktreeRootsConfig () |> Option.defaultValue [] /// Persists the watched worktree roots through the locked single-writer path, leaving every /// other global-config key untouched. Returns the write outcome so the `addRoot`/`removeRoot` diff --git a/src/Tests/ServerStartupResolutionTests.fs b/src/Tests/ServerStartupResolutionTests.fs index f0a0858..33c94e2 100644 --- a/src/Tests/ServerStartupResolutionTests.fs +++ b/src/Tests/ServerStartupResolutionTests.fs @@ -133,3 +133,46 @@ type ServerStartupResolutionTests() = Assert.That(resolved, Is.Empty) // Nothing to persist, so the config stays absent/empty. Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.Empty)) + + // ----- Regression (tm-config-audit-rf1): an explicit `worktreeRoots:[]` must stay empty ----- + // The bug: readWorktreeRootsConfig() returned [] for BOTH a missing key and a present-but-empty + // one, so resolveWorktreeRoots treated a curated-down-to-zero config like a fresh install and + // repopulated it from a stale orphan roots.json (or from CLI args). These pin the fix. + + [] + member _.``resolveWorktreeRoots leaves an explicit empty config empty despite an orphan roots.json``() = + withTempConfigDir (fun tempDir -> + // The user removed every root: the key is PRESENT but empty (not absent). + match Server.WorktreeApi.writeWorktreeRoots [] with + | Ok () -> () + | Error msg -> Assert.Fail $"setup write failed: {msg}" + // A stale orphan roots.json from a legacy upgrade still lingers on disk. + writeOrphan tempDir [ @"C:\code\removed-a"; @"C:\code\removed-b" ] + + let resolved = resolveWorktreeRoots [] + + // The orphan must NOT resurrect the removed roots for this run... + Assert.That(resolved, Is.Empty) + // ...the explicit empty config is preserved (present key, still empty — not None, not + // repopulated with the orphan's roots)... + match Server.WorktreeApi.tryReadWorktreeRootsConfig () with + | Some [] -> () + | other -> Assert.Fail $"expected the config to stay an explicit empty (Some []), got %A{other}" + // ...and the unconsumed orphan is left untouched (it is only migrated when the key is absent). + Assert.That(File.Exists(Path.Combine(tempDir, "roots.json")), Is.True)) + + [] + member _.``resolveWorktreeRoots does not persist CLI args over an explicit empty config``() = + withTempConfigDir (fun _ -> + match Server.WorktreeApi.writeWorktreeRoots [] with + | Ok () -> () + | Error msg -> Assert.Fail $"setup write failed: {msg}" + + let resolved = resolveWorktreeRoots [ @"C:\code\arg" ] + + // CLI args still win for THIS run (ephemeral override)... + Assert.That(resolved, Is.EqualTo([ @"C:\code\arg" ])) + // ...but the explicit empty config is not clobbered, so a restart with no args stays empty. + match Server.WorktreeApi.tryReadWorktreeRootsConfig () with + | Some [] -> () + | other -> Assert.Fail $"expected the config to stay an explicit empty (Some []), got %A{other}") From 28af7e677948998298e5005dcfdb231f31f346f6 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 21:38:58 +0200 Subject: [PATCH 09/17] tm-config-audit-rf2 Fix partial-batch tm add/remove suppressing the production server restart Add tri-state exit code (Cli.foldRootResults: 0=all ok, 1=all failed, 2=partial) and gate the treemon.ps1 add/remove restart on 0 OR 2, so a [valid; invalid] batch (valid persisted server-side, invalid rejected) still restarts prod to apply the persisted root. exit \ kept so failures still report non-zero. De-duplicates the per-path fold; adds FoldRootResultsTests (7 cases); documents the tri-state contract in spec Decision section 7 item (3). --- docs/spec/worktree-roots-config.md | 14 ++++++--- src/Cli/Program.fs | 34 +++++++++++++-------- src/Tests/CliTests.fs | 49 ++++++++++++++++++++++++++++++ treemon.ps1 | 14 ++++++--- 4 files changed, 90 insertions(+), 21 deletions(-) diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index 1f39cf7..53b77fa 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -157,10 +157,16 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. the parameter to `$null` (and `@($null)` is a *1-element* array), so `Start-ProductionServer`/ `Start-DevMode` normalize via `@($Roots | Where-Object { $_ })` — otherwise the "no path" feature threw on `$null.TrimEnd()` (and `restart` would kill prod then throw). (3) **add/remove restart - only on CLI success.** The shims gate the restart on `tm`'s exit code (`$tmExit -eq 0`) so a - rejected add/remove (bad path, server down) doesn't needlessly bounce prod; `Invoke-Tm` routes - the CLI's stdout to the host (`| Out-Host`) and returns only `[int]$LASTEXITCODE`, so the exit - code stays a scalar and the CLI's own messages aren't swallowed. + on any genuine change (tri-state exit code — fix tm-config-audit-rf2).** The CLI folds a path + batch into a tri-state exit code (`Cli.foldRootResults`): `0` = all succeeded, `1` = all failed, + `2` = partial. The shims gate the restart on `$tmExit -eq 0 -or $tmExit -eq 2` and keep + `exit $tmExit`, so prod restarts whenever at least one root was actually persisted (full or partial + success) while a fully-failed call (bad path, server down) still skips the restart and reports + non-zero. The original *binary* gate (`$tmExit -eq 0`) was the bug: `addRootToConfig` persists each + accepted path immediately, so a `[valid; invalid]` batch persisted `valid` but returned exit 1 — + the shim skipped the restart and the new root stayed dormant despite the printed success line. + `Invoke-Tm` routes the CLI's stdout to the host (`| Out-Host`) and returns only `[int]$LASTEXITCODE`, + so the exit code stays a scalar and the CLI's own messages aren't swallowed. - **Config-dir test isolation uses `TREEMON_CONFIG_DIR`, not a fixture-set `USERPROFILE`/`HOME`.** The spec preferred redirecting `USERPROFILE`/`HOME`, but on Windows .NET 9 `Environment.GetFolderPath(SpecialFolder.UserProfile)` reads the user token, **not** the env diff --git a/src/Cli/Program.fs b/src/Cli/Program.fs index aa53c0a..bf1c56c 100644 --- a/src/Cli/Program.fs +++ b/src/Cli/Program.fs @@ -215,17 +215,32 @@ let worktreesCmd = setAction handler } +/// Folds a per-path root operation (add/remove) into a tri-state exit code: +/// 0 = all paths succeeded, 1 = all paths failed, 2 = partial success. +/// A partial batch returns 2 (not 1) because the paths that succeeded WERE persisted +/// server-side immediately, before a later path failed validation. The treemon.ps1 shims +/// restart prod on 0 OR 2 so those persisted roots actually apply, while a full failure (1) +/// skips the restart (don't bounce prod when nothing changed). Any failure still yields a +/// non-zero exit, so callers/scripts can still detect rejected paths. +let foldRootResults (verb: string) (op: string -> Async>) (paths: string[]) : int = + let anySuccess, anyFailure = + ((false, false), paths) + ||> Array.fold (fun (anySuccess, anyFailure) path -> + match op path |> Async.RunSynchronously with + | Ok() -> printfn $"✓ %s{verb} %s{path} (applies on next server restart)"; (true, anyFailure) + | Error e -> eprintfn $"Error: %s{e}"; (anySuccess, true)) + + match anySuccess, anyFailure with + | true, true -> 2 + | false, true -> 1 + | _ -> 0 + let addCmd = let handler (paths: string[], port: int option) = withPort port (fun port -> // One tryCallServer for the whole batch so the "server is not running" message // (and the connection check) happens once, not once per path. - tryCallServer port (fun api -> - (0, paths) - ||> Array.fold (fun exitCode path -> - match api.addRoot path |> Async.RunSynchronously with - | Ok() -> printfn $"✓ Added %s{path} (applies on next server restart)"; exitCode - | Error e -> eprintfn $"Error: %s{e}"; 1))) + tryCallServer port (fun api -> foldRootResults "Added" api.addRoot paths)) command "add" { description "Add one or more worktree roots to watch (applies on next server restart)" @@ -241,12 +256,7 @@ let addCmd = let removeCmd = let handler (paths: string[], port: int option) = withPort port (fun port -> - tryCallServer port (fun api -> - (0, paths) - ||> Array.fold (fun exitCode path -> - match api.removeRoot path |> Async.RunSynchronously with - | Ok() -> printfn $"✓ Removed %s{path} (applies on next server restart)"; exitCode - | Error e -> eprintfn $"Error: %s{e}"; 1))) + tryCallServer port (fun api -> foldRootResults "Removed" api.removeRoot paths)) command "remove" { description "Remove one or more worktree roots (applies on next server restart)" diff --git a/src/Tests/CliTests.fs b/src/Tests/CliTests.fs index 702749e..da740bb 100644 --- a/src/Tests/CliTests.fs +++ b/src/Tests/CliTests.fs @@ -83,3 +83,52 @@ type FormatPrTests() = member _.``HasPr draft and conflicts shows both flags``() = let result = formatPr (makePrInfo 3 "Draft conflict" true false true) Assert.That(result, Is.EqualTo("PR #3 [draft, conflicts]: Draft conflict")) + +[] +[] +[] +type FoldRootResultsTests() = + + // Stub root op: fails for any path in failOn, succeeds otherwise. Lets us exercise the + // tri-state exit code (0 = all ok, 1 = all failed, 2 = partial) without a live server. + let stubOp (failOn: Set) (path: string) : Async> = + async { return (if failOn.Contains path then Error $"bad path: {path}" else Ok()) } + + [] + member _.``all paths succeed returns 0``() = + let result = foldRootResults "Added" (stubOp Set.empty) [| "a"; "b"; "c" |] + Assert.That(result, Is.EqualTo(0)) + + [] + member _.``single path success returns 0``() = + let result = foldRootResults "Added" (stubOp Set.empty) [| "a" |] + Assert.That(result, Is.EqualTo(0)) + + [] + member _.``all paths fail returns 1``() = + let result = foldRootResults "Added" (stubOp (Set.ofList [ "a"; "b" ])) [| "a"; "b" |] + Assert.That(result, Is.EqualTo(1)) + + [] + member _.``single path failure returns 1``() = + let result = foldRootResults "Removed" (stubOp (Set.ofList [ "a" ])) [| "a" |] + Assert.That(result, Is.EqualTo(1)) + + [] + member _.``partial success (valid then invalid) returns 2``() = + // The exact regression: a [valid; invalid] batch persists the valid root but + // must still signal "something changed" (2) so treemon.ps1 restarts to apply it. + let result = foldRootResults "Added" (stubOp (Set.ofList [ "invalid" ])) [| "valid"; "invalid" |] + Assert.That(result, Is.EqualTo(2)) + + [] + member _.``partial success (invalid then valid) returns 2``() = + // Order independence: failure first must not mask the later success. + let result = foldRootResults "Added" (stubOp (Set.ofList [ "invalid" ])) [| "invalid"; "valid" |] + Assert.That(result, Is.EqualTo(2)) + + [] + member _.``empty batch returns 0``() = + // Degenerate (CLI arity is OneOrMore, so unreachable in practice): no failures → 0. + let result = foldRootResults "Added" (stubOp Set.empty) [||] + Assert.That(result, Is.EqualTo(0)) diff --git a/treemon.ps1 b/treemon.ps1 index c808dd8..bd706e6 100644 --- a/treemon.ps1 +++ b/treemon.ps1 @@ -531,9 +531,12 @@ switch ($Command) { } } - # Restart only on a successful add so a failed/rejected CLI call (e.g. bad path, - # server down) doesn't needlessly bounce the production server. - if ($tmExit -eq 0) { + # Restart when at least one root actually changed. tm returns a tri-state exit + # code: 0 = all added, 2 = partial (some paths persisted, some rejected), 1 = all + # failed. Both 0 and 2 mean roots were persisted and need a restart to apply; exit 1 + # (e.g. bad path, server down — nothing persisted) skips the restart so we don't + # needlessly bounce the production server. + if ($tmExit -eq 0 -or $tmExit -eq 2) { $runningPid = Get-RunningPid if ($runningPid) { Write-Host "Restarting server to apply changes..." -ForegroundColor Cyan @@ -556,8 +559,9 @@ switch ($Command) { # a root whose directory was deleted must still be removable. $tmExit = Invoke-Tm (@("remove") + $WorktreeRoots + @("--port", "$DefaultPort")) - # Restart only on a successful remove (see 'add' above). - if ($tmExit -eq 0) { + # Restart on full (0) or partial (2) success — see 'add' above. Exit 1 (nothing + # removed) skips the restart. + if ($tmExit -eq 0 -or $tmExit -eq 2) { $runningPid = Get-RunningPid if ($runningPid) { Write-Host "Restarting server to apply changes..." -ForegroundColor Cyan From 091c5f09054e4bff42ed9a648dd404ec1c56ccf3 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 21:49:09 +0200 Subject: [PATCH 10/17] tm-config-audit-rf3 Read-only API should return Error (not Ok) for addRoot/removeRoot readOnlyApi addRoot/removeRoot now return Error "Root management is not available in {modeName}" instead of a silent Ok(); getRoots still returns []. Matches the existing read-only mutation pattern so the CLI surfaces a real failure instead of a false success line. Spec Decisions updated. --- docs/spec/worktree-roots-config.md | 6 ++++++ src/Server/WorktreeApi.fs | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md index 53b77fa..5a00737 100644 --- a/docs/spec/worktree-roots-config.md +++ b/docs/spec/worktree-roots-config.md @@ -224,6 +224,12 @@ the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. - **Demo/fixture modes pass `[]` to `worktreeApi`/scheduler** (resolution is bypassed entirely). This is behaviorally inert for fixture mode because `worktreeApi`'s fixture branch is built from `readOnlyApi` and ignores `rootPaths`; passing `[]` matches the spec's "(roots stay [])". +- **Read-only `addRoot`/`removeRoot` return `Error` (fix tm-config-audit-rf3).** In `--demo`/ + `--test-fixtures`, `readOnlyApi` wires these to `Error $"Root management is not available in + {modeName}"` (matching every other unsupported mutation), not a silent `Ok()`. The old `Ok()` + made the CLI print `✓ Added … (applies on next server restart)` against a read-only server even + though nothing is persisted (these modes force `worktreeRoots=[]`). `getRoots` still returns `[]` + (a read, correctly empty). - **Smoke tests isolate the config dir.** Both `SmokeTests` fixtures start the server in *normal* mode with real roots, which now triggers the startup persist; without isolation that would write the developer's real `~/.treemon`. Each fixture points the child server at a throwaway diff --git a/src/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index 067c627..78875c2 100644 --- a/src/Server/WorktreeApi.fs +++ b/src/Server/WorktreeApi.fs @@ -68,9 +68,9 @@ let readOnlyApi saveLastViewedHashes = fun _ -> async { return () } loadLastViewedHashes = fun () -> async { return Map.empty } getBridgeLiveness = fun _ -> async { return Map.empty } - // Roots are read-only/no-op in demo and fixture modes (roots stay []). - addRoot = fun _ -> async { return Ok() } - removeRoot = fun _ -> async { return Ok() } + // Root management is unavailable in demo/fixture modes (roots stay []); getRoots is just empty. + addRoot = fun _ -> async { return Error $"Root management is not available in {modeName}" } + removeRoot = fun _ -> async { return Error $"Root management is not available in {modeName}" } getRoots = fun () -> async { return [] } } let private archiveCanvasDocImpl (request: ArchiveCanvasDocRequest) = From 40bfe858bfd77906a0cbbb8527a947b29a97dfa5 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 21:57:06 +0200 Subject: [PATCH 11/17] tm-config-audit-rf4 Refactor treemon.ps1: extract Restart-ServerIfRunning and Test-WorktreeRootPaths helpers Extract Test-WorktreeRootPaths (path validation for start/dev) and Restart-ServerIfRunning (restart-if-running lifecycle for add/remove and Deploy-Frontend via optional Message/NotRunningMessage params). Removes 5 duplicated blocks (2 validation + 3 restart incl. Deploy-Frontend triplication). Resolves focused-review findings C-04..C-07. Parser verified clean; behavior-preserving. --- treemon.ps1 | 82 +++++++++++++++++++++++++---------------------------- 1 file changed, 39 insertions(+), 43 deletions(-) diff --git a/treemon.ps1 b/treemon.ps1 index bd706e6..80d3281 100644 --- a/treemon.ps1 +++ b/treemon.ps1 @@ -449,6 +449,40 @@ function Install-Extension { } } +function Test-WorktreeRootPaths([string[]]$Roots) { + # Validate that each provided worktree root exists before launching the server. + # exit inside a function still terminates the script, so callers need no extra guard. + if ($Roots -and $Roots.Count -gt 0) { + $Roots | ForEach-Object { + if (-not (Test-Path $_)) { + Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red + exit 1 + } + } + } +} + +function Restart-ServerIfRunning { + # Restart the production server only when it is currently running, so persisted + # config changes (added/removed roots, redeployed frontend) take effect. Roots are + # re-read from the global config at startup, so we restart with empty args (@()). + # Optional parameters let Deploy-Frontend keep its distinct log text while sharing + # the same lifecycle logic. + param( + [string]$Message = "Restarting server to apply changes...", + [string]$NotRunningMessage = "" + ) + $runningPid = Get-RunningPid + if ($runningPid) { + Write-Host $Message -ForegroundColor Cyan + Stop-ProductionServer + Start-Sleep -Seconds 1 + Start-ProductionServer @() + } elseif ($NotRunningMessage) { + Write-Host $NotRunningMessage -ForegroundColor Gray + } +} + function Deploy-Frontend { Write-Host "Building frontend..." -ForegroundColor Cyan Build-Frontend @@ -458,27 +492,12 @@ function Deploy-Frontend { try { Install-Skill } catch { Write-Host "Warning: skill install failed: $_" -ForegroundColor Yellow } try { Install-Extension } catch { Write-Host "Warning: extension install failed: $_" -ForegroundColor Yellow } - $runningPid = Get-RunningPid - if ($runningPid) { - Write-Host "Restarting production server..." -ForegroundColor Cyan - Stop-ProductionServer - Start-Sleep -Seconds 1 - Start-ProductionServer @() - } else { - Write-Host "Production server is not running, skipping restart" -ForegroundColor Gray - } + Restart-ServerIfRunning -Message "Restarting production server..." -NotRunningMessage "Production server is not running, skipping restart" } switch ($Command) { "start" { - if ($WorktreeRoots -and $WorktreeRoots.Count -gt 0) { - $WorktreeRoots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red - exit 1 - } - } - } + Test-WorktreeRootPaths $WorktreeRoots Start-ProductionServer $WorktreeRoots } "stop" { @@ -496,14 +515,7 @@ switch ($Command) { Show-Log } "dev" { - if ($WorktreeRoots -and $WorktreeRoots.Count -gt 0) { - $WorktreeRoots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red - exit 1 - } - } - } + Test-WorktreeRootPaths $WorktreeRoots Start-DevMode $WorktreeRoots } "demo" { @@ -536,15 +548,7 @@ switch ($Command) { # failed. Both 0 and 2 mean roots were persisted and need a restart to apply; exit 1 # (e.g. bad path, server down — nothing persisted) skips the restart so we don't # needlessly bounce the production server. - if ($tmExit -eq 0 -or $tmExit -eq 2) { - $runningPid = Get-RunningPid - if ($runningPid) { - Write-Host "Restarting server to apply changes..." -ForegroundColor Cyan - Stop-ProductionServer - Start-Sleep -Seconds 1 - Start-ProductionServer @() - } - } + if ($tmExit -eq 0 -or $tmExit -eq 2) { Restart-ServerIfRunning } exit $tmExit } "remove" { @@ -561,15 +565,7 @@ switch ($Command) { # Restart on full (0) or partial (2) success — see 'add' above. Exit 1 (nothing # removed) skips the restart. - if ($tmExit -eq 0 -or $tmExit -eq 2) { - $runningPid = Get-RunningPid - if ($runningPid) { - Write-Host "Restarting server to apply changes..." -ForegroundColor Cyan - Stop-ProductionServer - Start-Sleep -Seconds 1 - Start-ProductionServer @() - } - } + if ($tmExit -eq 0 -or $tmExit -eq 2) { Restart-ServerIfRunning } exit $tmExit } "install-skill" { From e2496b0cac51a731f327b5c44e2306e391365615 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 22:21:27 +0200 Subject: [PATCH 12/17] tm-config-audit-rf5 Hoist duplicated withTempConfigDir test helper into shared TestUtils.fs Move withTempConfigDir into TestUtils.fs (prefix-parameterized like withTempFile) and de-dupe assertOk onto TestUtils.assertOk. Removes both private copies + dead 'open System'. Resolves focused-review C-15/C-16. Build clean (0 warn/0 err); 26/26 affected tests pass. --- src/Tests/ServerStartupResolutionTests.fs | 37 ++------- src/Tests/TestUtils.fs | 21 ++++++ src/Tests/WorktreeRootsConfigTests.fs | 91 ++++++++--------------- 3 files changed, 61 insertions(+), 88 deletions(-) diff --git a/src/Tests/ServerStartupResolutionTests.fs b/src/Tests/ServerStartupResolutionTests.fs index 33c94e2..e83fded 100644 --- a/src/Tests/ServerStartupResolutionTests.fs +++ b/src/Tests/ServerStartupResolutionTests.fs @@ -1,6 +1,5 @@ module Tests.ServerStartupResolutionTests -open System open System.IO open System.Text.Json.Nodes open NUnit.Framework @@ -9,27 +8,7 @@ open NUnit.Framework // module; `Server` exposes internals to `Tests` (InternalsVisibleTo), so `resolveWorktreeRoots` // (internal) is reachable here. open Program - -/// Isolates the machine-level config dir to a throwaway temp dir via the TREEMON_CONFIG_DIR -/// override (mirrors WorktreeRootsConfigTests). Required, not convenient: on Windows -/// Environment.GetFolderPath(UserProfile) ignores USERPROFILE/HOME, so this is the only way to -/// keep the in-process resolver off the real ~/.treemon. Both the global config read/write and the -/// orphan roots.json lookup resolve under this dir. -let private withTempConfigDir (action: string -> unit) = - let tempDir = Path.Combine(Path.GetTempPath(), $"treemon-startup-test-{Guid.NewGuid()}") - Directory.CreateDirectory(tempDir) |> ignore - let original = Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") - Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", tempDir) - - try - action tempDir - finally - Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", original) - - try - Directory.Delete(tempDir, recursive = true) - with _ -> - () +open Tests.TestUtils /// Writes an orphan `roots.json` (`{ "WorktreeRoots": [...] }`) into the isolated config dir using /// the JSON node API, so test paths never need manual backslash escaping. @@ -80,7 +59,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots persists CLI args when config has no roots``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-startup-test" (fun _ -> let cliRoots = [ @"C:\code\one"; @"C:\code\two" ] let resolved = resolveWorktreeRoots cliRoots Assert.That(resolved, Is.EqualTo(cliRoots)) @@ -89,7 +68,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots prefers CLI args but does not overwrite a populated config``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-startup-test" (fun _ -> let configured = [ @"C:\code\configured" ] match Server.WorktreeApi.writeWorktreeRoots configured with | Ok () -> () @@ -103,7 +82,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots reads global config when no CLI args``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-startup-test" (fun _ -> let configured = [ @"C:\code\x"; @"C:\code\y" ] match Server.WorktreeApi.writeWorktreeRoots configured with | Ok () -> () @@ -114,7 +93,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots imports orphan roots.json then persists and deletes it``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-startup-test" (fun tempDir -> let orphanRoots = [ @"C:\code\orphan-a"; @"C:\code\orphan-b" ] writeOrphan tempDir orphanRoots @@ -128,7 +107,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots returns empty when no args, no config, no orphan``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-startup-test" (fun _ -> let resolved = resolveWorktreeRoots [] Assert.That(resolved, Is.Empty) // Nothing to persist, so the config stays absent/empty. @@ -141,7 +120,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots leaves an explicit empty config empty despite an orphan roots.json``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-startup-test" (fun tempDir -> // The user removed every root: the key is PRESENT but empty (not absent). match Server.WorktreeApi.writeWorktreeRoots [] with | Ok () -> () @@ -163,7 +142,7 @@ type ServerStartupResolutionTests() = [] member _.``resolveWorktreeRoots does not persist CLI args over an explicit empty config``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-startup-test" (fun _ -> match Server.WorktreeApi.writeWorktreeRoots [] with | Ok () -> () | Error msg -> Assert.Fail $"setup write failed: {msg}" diff --git a/src/Tests/TestUtils.fs b/src/Tests/TestUtils.fs index 7419232..f07cb1d 100644 --- a/src/Tests/TestUtils.fs +++ b/src/Tests/TestUtils.fs @@ -124,9 +124,30 @@ let withTempCwd (action: unit -> unit) = Environment.CurrentDirectory <- original try Directory.Delete(tempDir, recursive = true) with _ -> () +/// Run `action` with the machine-level Treemon config dir redirected to a throwaway temp dir via +/// the TREEMON_CONFIG_DIR override, then restore the previous value and delete the dir. Required, +/// not merely convenient: on Windows Environment.GetFolderPath(UserProfile) ignores USERPROFILE/HOME, +/// so the override is the only way to keep in-process config tests (the global read/write helpers and +/// the orphan roots.json lookup) off the real ~/.treemon. `prefix` names the temp dir for debugging, +/// mirroring withTempFile. TREEMON_CONFIG_DIR is process-global, so callers must stay non-parallel. +let withTempConfigDir (prefix: string) (action: string -> unit) = + let tempDir = Path.Combine(Path.GetTempPath(), $"{prefix}-{Guid.NewGuid()}") + Directory.CreateDirectory(tempDir) |> ignore + let original = Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") + Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", tempDir) + + try + action tempDir + finally + Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", original) + try Directory.Delete(tempDir, recursive = true) with _ -> () + let runAsync (a: Async<'T>) = Async.RunSynchronously(a, timeout = 30_000) +/// Asserts a `Result` is `Ok`, prefixing `message` to the surfaced error on failure. +/// Prefer this over `Is.EqualTo(Ok())`: the literal's error type infers as `obj`, so NUnit's +/// structural compare never matches the actual `Result` even when both are `Ok ()`. let assertOk (result: Result) (message: string) = match result with | Ok() -> () diff --git a/src/Tests/WorktreeRootsConfigTests.fs b/src/Tests/WorktreeRootsConfigTests.fs index c239514..33fd679 100644 --- a/src/Tests/WorktreeRootsConfigTests.fs +++ b/src/Tests/WorktreeRootsConfigTests.fs @@ -5,26 +5,7 @@ open System.IO open System.Text.Json open NUnit.Framework open Server.WorktreeApi - -/// Isolates the machine-level Treemon config dir to a throwaway temp dir via the -/// TREEMON_CONFIG_DIR override. This is required, not merely convenient: on Windows -/// Environment.GetFolderPath(UserProfile) ignores the USERPROFILE/HOME env vars, so the -/// override is the only way to keep these in-process tests off the real ~/.treemon. -let private withTempConfigDir (action: string -> unit) = - let tempDir = Path.Combine(Path.GetTempPath(), $"treemon-roots-test-{Guid.NewGuid()}") - Directory.CreateDirectory(tempDir) |> ignore - let original = Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") - Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", tempDir) - - try - action tempDir - finally - Environment.SetEnvironmentVariable("TREEMON_CONFIG_DIR", original) - - try - Directory.Delete(tempDir, recursive = true) - with _ -> - () +open Tests.TestUtils /// Compares two roots the way the endpoints do: absolute, trailing separators trimmed, /// case-insensitive. Lets assertions ignore the exact normalized form of the temp dir. @@ -33,14 +14,6 @@ let private sameRoot (a: string) (b: string) = Path.GetFullPath(p).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) String.Equals(canon a, canon b, StringComparison.OrdinalIgnoreCase) -/// Asserts a `Result` is `Ok`, surfacing the error message on failure. Avoids the -/// `Is.EqualTo(Ok())` pitfall: the literal's error type infers as `obj`, so NUnit's structural -/// compare never matches the actual `Result` even when both are `Ok ()`. -let private assertOk (result: Result) = - match result with - | Ok () -> () - | Error msg -> Assert.Fail $"expected Ok but got Error: {msg}" - [] [] [] @@ -53,18 +26,18 @@ type WorktreeRootsConfigTests() = [] member _.``readWorktreeRootsConfig returns empty when config file is absent``() = - withTempConfigDir (fun _ -> Assert.That(readWorktreeRootsConfig (), Is.Empty)) + withTempConfigDir "treemon-roots-test" (fun _ -> Assert.That(readWorktreeRootsConfig (), Is.Empty)) [] member _.``writeWorktreeRoots then read round-trips roots in order``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-roots-test" (fun _ -> let roots = [ @"C:\code\alpha"; @"C:\code\beta" ] - assertOk (writeWorktreeRoots roots) + assertOk (writeWorktreeRoots roots) "writeWorktreeRoots should succeed" Assert.That(readWorktreeRootsConfig (), Is.EqualTo(roots))) [] member _.``writeWorktreeRoots preserves unrelated config keys (no clobber)``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let configPath = Path.Combine(tempDir, "config.json") // Pre-seed config.json with keys owned by other features (collapsedRepos, canvas, editor). let seed = @@ -75,7 +48,7 @@ type WorktreeRootsConfigTests() = }""" File.WriteAllText(configPath, seed) - assertOk (writeWorktreeRoots [ @"C:\code\gamma" ]) + assertOk (writeWorktreeRoots [ @"C:\code\gamma" ]) "writeWorktreeRoots should succeed" // New key persisted. Assert.That(readWorktreeRootsConfig (), Is.EqualTo([ @"C:\code\gamma" ])) @@ -95,16 +68,16 @@ type WorktreeRootsConfigTests() = [] member _.``writeWorktreeRoots overwrites a previously written roots value``() = - withTempConfigDir (fun _ -> - assertOk (writeWorktreeRoots [ @"C:\code\one" ]) - assertOk (writeWorktreeRoots [ @"C:\code\two"; @"C:\code\three" ]) + withTempConfigDir "treemon-roots-test" (fun _ -> + assertOk (writeWorktreeRoots [ @"C:\code\one" ]) "writeWorktreeRoots should succeed" + assertOk (writeWorktreeRoots [ @"C:\code\two"; @"C:\code\three" ]) "writeWorktreeRoots should succeed" Assert.That(readWorktreeRootsConfig (), Is.EqualTo([ @"C:\code\two"; @"C:\code\three" ]))) [] member _.``writeWorktreeRoots with empty list clears roots (removing last root is valid)``() = - withTempConfigDir (fun _ -> - assertOk (writeWorktreeRoots [ @"C:\code\one" ]) - assertOk (writeWorktreeRoots []) + withTempConfigDir "treemon-roots-test" (fun _ -> + assertOk (writeWorktreeRoots [ @"C:\code\one" ]) "writeWorktreeRoots should succeed" + assertOk (writeWorktreeRoots []) "writeWorktreeRoots should succeed" Assert.That(readWorktreeRootsConfig (), Is.Empty)) // ----- addRoot/removeRoot endpoint logic (tm-config-audit-9ow) ----- @@ -113,17 +86,17 @@ type WorktreeRootsConfigTests() = [] member _.``addRootToConfig persists an existing directory``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let root = Path.Combine(tempDir, "alpha") Directory.CreateDirectory(root) |> ignore - assertOk (addRootToConfig root) + assertOk (addRootToConfig root) "addRootToConfig should succeed" let roots = readWorktreeRootsConfig () Assert.That(roots.Length, Is.EqualTo(1)) Assert.That(sameRoot roots.[0] root, Is.True)) [] member _.``addRootToConfig rejects a path that does not exist``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let missing = Path.Combine(tempDir, "does-not-exist") match addRootToConfig missing with | Error msg -> Assert.That(msg, Is.Not.Empty) @@ -132,31 +105,31 @@ type WorktreeRootsConfigTests() = [] member _.``addRootToConfig rejects a blank path``() = - withTempConfigDir (fun _ -> + withTempConfigDir "treemon-roots-test" (fun _ -> match addRootToConfig " " with | Error msg -> Assert.That(msg, Is.Not.Empty) | Ok () -> Assert.Fail "expected Error for a blank path") [] member _.``addRootToConfig is an idempotent no-op for an already-watched path``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let root = Path.Combine(tempDir, "alpha") Directory.CreateDirectory(root) |> ignore - assertOk (addRootToConfig root) + assertOk (addRootToConfig root) "addRootToConfig should succeed" // Re-adding the same path with a trailing separator normalizes to the same root and // must not duplicate it. - assertOk (addRootToConfig (root + string Path.DirectorySeparatorChar)) + assertOk (addRootToConfig (root + string Path.DirectorySeparatorChar)) "re-adding the same root should succeed" Assert.That(readWorktreeRootsConfig().Length, Is.EqualTo(1))) [] member _.``addRootToConfig appends roots preserving insertion order``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let alpha = Path.Combine(tempDir, "alpha") let beta = Path.Combine(tempDir, "beta") Directory.CreateDirectory(alpha) |> ignore Directory.CreateDirectory(beta) |> ignore - addRootToConfig alpha |> assertOk - addRootToConfig beta |> assertOk + assertOk (addRootToConfig alpha) "addRootToConfig should succeed" + assertOk (addRootToConfig beta) "addRootToConfig should succeed" let roots = readWorktreeRootsConfig () Assert.That(roots.Length, Is.EqualTo(2)) Assert.That(sameRoot roots.[0] alpha, Is.True) @@ -164,27 +137,27 @@ type WorktreeRootsConfigTests() = [] member _.``addRootToConfig preserves unrelated config keys``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let configPath = Path.Combine(tempDir, "config.json") File.WriteAllText(configPath, """{ "editor": "vim" }""") let root = Path.Combine(tempDir, "alpha") Directory.CreateDirectory(root) |> ignore - addRootToConfig root |> assertOk + assertOk (addRootToConfig root) "addRootToConfig should succeed" use doc = JsonDocument.Parse(File.ReadAllText configPath) Assert.That(doc.RootElement.GetProperty("editor").GetString(), Is.EqualTo("vim"))) [] member _.``removeRootFromConfig removes a watched root``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let root = Path.Combine(tempDir, "alpha") Directory.CreateDirectory(root) |> ignore - addRootToConfig root |> assertOk - assertOk (removeRootFromConfig root) + assertOk (addRootToConfig root) "addRootToConfig should succeed" + assertOk (removeRootFromConfig root) "removeRootFromConfig should succeed" Assert.That(readWorktreeRootsConfig (), Is.Empty)) [] member _.``removeRootFromConfig errors when the path is not watched``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let root = Path.Combine(tempDir, "alpha") Directory.CreateDirectory(root) |> ignore match removeRootFromConfig root with @@ -193,18 +166,18 @@ type WorktreeRootsConfigTests() = [] member _.``removeRootFromConfig removes a root whose directory no longer exists``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> let root = Path.Combine(tempDir, "alpha") Directory.CreateDirectory(root) |> ignore - addRootToConfig root |> assertOk + assertOk (addRootToConfig root) "addRootToConfig should succeed" Directory.Delete(root) // Removal must not depend on the directory still existing on disk. - assertOk (removeRootFromConfig root) + assertOk (removeRootFromConfig root) "removeRootFromConfig should succeed" Assert.That(readWorktreeRootsConfig (), Is.Empty)) [] member _.``addRootToConfig surfaces a persistence failure as Error``() = - withTempConfigDir (fun tempDir -> + withTempConfigDir "treemon-roots-test" (fun tempDir -> // Make config.json an (unwritable) directory so the File.WriteAllText inside the // locked writer throws. The endpoint must surface that as Error rather than a false // Ok() — the whole point of the Result contract. From 95b237e833d048f53d09cad352b0ca1e1bdbc418 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 19 Jun 2026 22:26:51 +0200 Subject: [PATCH 13/17] tm-config-audit-rf6 F# idiom nits in new roots code: _.GetString() shorthand and Option.ofObj boundary C-14: WorktreeRootsConfigTests.fs collapsedRepos read uses Seq.map _.GetString() (F# 9 shorthand). C-19: globalConfigDir() converts TREEMON_CONFIG_DIR at the boundary via Option.ofObj |> Option.filter |> Option.defaultWith, preserving the empty/null -> ~/.treemon fallback. --- src/Server/WorktreeApi.fs | 9 +++++---- src/Tests/WorktreeRootsConfigTests.fs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index 78875c2..f311e46 100644 --- a/src/Server/WorktreeApi.fs +++ b/src/Server/WorktreeApi.fs @@ -180,12 +180,13 @@ let private resolveProvider (state: RefreshScheduler.DashboardState) (path: stri /// `Environment.GetFolderPath(UserProfile)` ignores the USERPROFILE/HOME env vars, so an /// in-process test can only redirect the config dir via this explicit override. let internal globalConfigDir () = - match Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") with - | null | "" -> + Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") + |> Option.ofObj + |> Option.filter (fun d -> d <> "") + |> Option.defaultWith (fun () -> Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), - ".treemon") - | dir -> dir + ".treemon")) let private globalConfigPath () = Path.Combine(globalConfigDir (), "config.json") diff --git a/src/Tests/WorktreeRootsConfigTests.fs b/src/Tests/WorktreeRootsConfigTests.fs index 33fd679..a5990ad 100644 --- a/src/Tests/WorktreeRootsConfigTests.fs +++ b/src/Tests/WorktreeRootsConfigTests.fs @@ -61,7 +61,7 @@ type WorktreeRootsConfigTests() = let collapsed = root.GetProperty("collapsedRepos").EnumerateArray() - |> Seq.map (fun e -> e.GetString()) + |> Seq.map _.GetString() |> List.ofSeq Assert.That(collapsed, Is.EqualTo([ "repo-one"; "repo-two" ]))) From 610384c2c15ff45bda3ff5c6710b74dc0da82127 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Mon, 22 Jun 2026 16:42:51 +0200 Subject: [PATCH 14/17] Use modern indexer syntax in WorktreeRootsConfigTests Address Copilot review on PR #83: replace obsolete dot-bracket indexing (roots.[0]) with modern roots[0] per the project F# style guide. --- src/Tests/WorktreeRootsConfigTests.fs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Tests/WorktreeRootsConfigTests.fs b/src/Tests/WorktreeRootsConfigTests.fs index a5990ad..c0688c7 100644 --- a/src/Tests/WorktreeRootsConfigTests.fs +++ b/src/Tests/WorktreeRootsConfigTests.fs @@ -92,7 +92,7 @@ type WorktreeRootsConfigTests() = assertOk (addRootToConfig root) "addRootToConfig should succeed" let roots = readWorktreeRootsConfig () Assert.That(roots.Length, Is.EqualTo(1)) - Assert.That(sameRoot roots.[0] root, Is.True)) + Assert.That(sameRoot roots[0] root, Is.True)) [] member _.``addRootToConfig rejects a path that does not exist``() = @@ -132,8 +132,8 @@ type WorktreeRootsConfigTests() = assertOk (addRootToConfig beta) "addRootToConfig should succeed" let roots = readWorktreeRootsConfig () Assert.That(roots.Length, Is.EqualTo(2)) - Assert.That(sameRoot roots.[0] alpha, Is.True) - Assert.That(sameRoot roots.[1] beta, Is.True)) + Assert.That(sameRoot roots[0] alpha, Is.True) + Assert.That(sameRoot roots[1] beta, Is.True)) [] member _.``addRootToConfig preserves unrelated config keys``() = From f911a5c83b6e7ffb8ab46b5f204131587b8513eb Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Tue, 23 Jun 2026 15:22:11 +0200 Subject: [PATCH 15/17] Address review findings: pure roots resolver, lossless legacy migration, spec consolidation - resolveWorktreeRoots is now a pure resolver returning RootsResolution; the global-config write and orphan delete moved to persistResolvedRoots, applied at the startup boundary in main. Resolver tests assert the decision and apply the effect explicitly. - treemon.ps1 migration reads the legacy singular WorktreeRoot key (not just plural WorktreeRoots) and deletes .treemon.config only once every declared root was migrated; parse failures and unmigrated content are preserved with a warning instead of being silently dropped. - Fold the durable roots-config contract and decisions into worktree-monitor.md and delete the branch-specific worktree-roots-config.md plan/changelog; fix the AGENTS.md and Related Specs back-references. --- AGENTS.md | 2 +- docs/spec/worktree-monitor.md | 7 +- docs/spec/worktree-roots-config.md | 257 ---------------------- src/Server/Program.fs | 55 +++-- src/Tests/ServerStartupResolutionTests.fs | 56 +++-- treemon.ps1 | 72 ++++-- 6 files changed, 136 insertions(+), 313 deletions(-) delete mode 100644 docs/spec/worktree-roots-config.md diff --git a/AGENTS.md b/AGENTS.md index d05dc58..c875f0b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,7 +85,7 @@ This project uses [focused-review](https://github.com/0101/focused-review) for a `treemon.ps1` manages the application lifecycle: `dev`, `deploy`, `start`, `stop`, `restart`, `status`, `log`, `add`, `remove`. -Watched worktree roots live in the global config (`~/.treemon/config.json` → `worktreeRoots`), written only by the server. `start`/`dev` no longer need a path — omit it to use the global roots (an empty list is valid). Manage roots live with the `tm` CLI — `tm add ...`, `tm remove ...`, `tm roots` — or the `treemon.ps1 add`/`remove` shims, which call `tm` and restart production when it is running. Changes persist immediately and apply on the next server (re)start. See `docs/spec/worktree-roots-config.md`. +Watched worktree roots live in the global config (`~/.treemon/config.json` → `worktreeRoots`), written only by the server. `start`/`dev` no longer need a path — omit it to use the global roots (an empty list is valid). Manage roots live with the `tm` CLI — `tm add ...`, `tm remove ...`, `tm roots` — or the `treemon.ps1 add`/`remove` shims, which call `tm` and restart production when it is running. Changes persist immediately and apply on the next server (re)start. See `docs/spec/worktree-monitor.md` (Multi-Repo). ## Tech Stack diff --git a/docs/spec/worktree-monitor.md b/docs/spec/worktree-monitor.md index c41a278..48c57da 100644 --- a/docs/spec/worktree-monitor.md +++ b/docs/spec/worktree-monitor.md @@ -23,8 +23,9 @@ ### Multi-Repo -- Watched roots resolve at server startup by priority: CLI args → the global `worktreeRoots` key in `~/.treemon/config.json` → one-time import of the legacy `~/.treemon/roots.json`. With roots configured, `treemon.ps1 start`/`dev` no longer require a path; with no args the server uses the global config (an empty list is valid, as in demo mode). -- Roots are managed live through the `tm` CLI — `tm add ...`, `tm remove ...`, and `tm roots` (list). The server is the single writer of `config.json`; changes persist immediately and take effect on the next server (re)start. The `treemon.ps1 add`/`remove` shims call `tm` and then restart the production server when it is running. +- Watched roots resolve at server startup by priority: CLI args → the global `worktreeRoots` key in `~/.treemon/config.json` → a one-time import of the legacy orphan `~/.treemon/roots.json`. With roots configured, `treemon.ps1 start`/`dev` no longer require a path; with no args the server uses the global config (an empty list is valid, as in demo mode). A *present* `worktreeRoots` key — even an explicit empty list — is authoritative and never repopulated; the server persists a resolved set only when the key is *absent* (fresh install / migration), so curating every root away stays sticky across restarts. +- Roots are managed live through the `tm` CLI — `tm add ...` (validates the path exists, normalizes it, no-op if already watched), `tm remove ...` (errors on an unknown path; removing the last root is allowed), and `tm roots` (list). All three are online-only (require the running server). The server is the single, serialized writer of `config.json`; changes persist immediately and take effect on the next server (re)start. The `treemon.ps1 add`/`remove` shims call `tm` and then restart the production server when it is running. +- Roots are a per-machine singleton: dev and prod instances on the same machine share one global list. Legacy stores migrate then delete losslessly — `treemon.ps1` migrates a legacy `.treemon.config` (PowerShell-written, plural `WorktreeRoots` or the older singular `WorktreeRoot`) and the server migrates the orphan `roots.json`, each removed only after its roots are safely persisted (a parse failure or unmigrated content is preserved with a warning, never silently dropped). - Each root is an independent section — cards never mix across repos - Scheduler picks most-overdue task globally across all repos - Branch events scoped by `{repoId}/{branch}` to prevent cross-repo collisions @@ -257,10 +258,10 @@ After the burst, `lastRuns` is pre-populated and the normal sequential loop take - net9.0 (not net10.0): Fable 4.28.0 FCS hangs with .NET 10 preview SDK - Windows Terminal per-window tracking via HWND: tabs aren't reliably addressable, one window per worktree is simple and predictable - Upstream remote auto-detection over config-only: `upstream` remote name is the universal convention for fork workflows; config override available for non-standard setups +- Watched roots are server-owned and restart-to-apply (not live-updated): `tm add`/`remove` persist to the global config and take effect on the next server (re)start (the `treemon.ps1` shims trigger it when prod is running). Chosen for simpler code — no per-root scheduler-state machinery; live application remains a clean future extension. The server is the single writer of `config.json` (with an internal write lock); the online-only CLI never writes config files, which removes the cross-process clobber hazard. ## Related Specs -- `docs/spec/worktree-roots-config.md` — global `worktreeRoots` config, CLI-managed roots (`tm add`/`remove`/`roots`), server as the single config writer - `docs/spec/user-idle-detection.md` — adaptive refresh cadence based on user activity level - `docs/spec/keyboard-navigation.md` — spatial arrow-key navigation and key bindings - `docs/spec/native-session-management.md` — Windows Terminal spawn/focus/kill via HWND tracking diff --git a/docs/spec/worktree-roots-config.md b/docs/spec/worktree-roots-config.md deleted file mode 100644 index 5a00737..0000000 --- a/docs/spec/worktree-roots-config.md +++ /dev/null @@ -1,257 +0,0 @@ -# Worktree Roots in Global Config (CLI-managed, server-mediated) - -## Goals - -Make the set of watched worktree roots a **single machine-level setting** owned by the server, -managed live through the `tm` CLI — eliminating the PowerShell-managed `.treemon.config`, the -stale orphan `~/.treemon/roots.json`, and the restart-to-apply cycle. - -- One source of truth for roots: `~/.treemon/config.json` → `"worktreeRoots": [...]`. -- The **server is the only writer** of `config.json` (fixes the cross-process clobber hazard - where PowerShell's whole-file overwrite would wipe server-written keys like `collapsedRepos`). -- `tm add ` / `tm remove ` **persist immediately** (server-side, single writer); - the change is applied when the server (re)starts. The `treemon.ps1 add/remove` shims trigger - that restart automatically when the production server is running (matching today's UX). -- `treemon.ps1` keeps only lifecycle duties (`start/stop/restart/status/dev/deploy/log`) and no - longer needs a path for `start`/`dev` once roots are configured. -- One-time migration of existing `.treemon.config` and the orphan `~/.treemon/roots.json` into - the global config, then delete both. - -Non-goals (explicitly deferred): consolidating the *other* duplicated config readers -(`TreemonConfig` vs `WorktreeApi` global helpers) and introducing typed config records -(Solutions A/B from the investigation), and splitting `.treemon.json` shared-vs-local -(Solution C). This spec is Solution **D** only. - -## Expected Behavior - -### CLI -- `tm add [...]` — validates each path exists, normalizes it (absolute, - trailing-slash trimmed), calls the server, which persists it to global config. On success - prints `✓ Added (applies on next server restart)`. Adding an already-watched path is a - no-op success. Requires the server running (online-only); applied on the next (re)start. -- `tm remove [...]` — calls the server, which removes it from global config. On - success prints `✓ Removed (applies on next server restart)`. Removing an unknown path - reports a clear error. Removing the last remaining root is allowed (empty dashboard is valid). -- `tm roots` — lists currently watched roots (so users/scripts can inspect without the UI). -- All three are online-only: if the server isn't running they print the existing - "server is not running" message (consistent with `tm worktrees`/`new`/`launch`). - -### Server -- On startup, **effective roots** resolve by priority: - 1. roots passed as CLI args (used by `dev`/tests; preserves current arg behavior), - 2. else `worktreeRoots` from `~/.treemon/config.json`, - 3. else (migration) import the orphan `~/.treemon/roots.json` if present. -- After resolving, if `config.json` has no `worktreeRoots`, the server **persists** the resolved - roots into it (so a migrated/arg-provided set becomes the durable source of truth), and deletes - the orphan `roots.json` once imported. -- `--demo` and `--test-fixtures` modes are unchanged (roots stay `[]`, scheduler behavior as today). -- `addRoot`/`removeRoot` validate and persist to global config (single, locked writer); they - return `Result` so the CLI can surface failures (bad path, etc.). They do not - mutate the running watch set — roots are re-read at (re)start. - -### treemon.ps1 -- `start` / `dev` no longer require a path argument; with no path they let the server use the - global config. A path argument still works (passed as args → highest priority). -- `status` shows the watched roots by querying the server (via `tm roots`/endpoint), not by - reading a local file. -- `add` / `remove` become **thin shims** that call `tm add` / `tm remove` and then, if the - production server is running, restart it to apply (same effect as today). Usage text, AGENTS.md, - and README are updated accordingly. -- On any invocation, if a legacy `.treemon.config` exists in the script dir, its roots are passed - to the next `start` as args (so the server migrates+persists them) and the file is deleted. - -## Technical Approach - -### 1. Shared API surface (`src/Shared/Types.fs`, IWorktreeApi ~248-274) -Add three members (paths as plain `string`, consistent with existing simple endpoints): -```fsharp -addRoot: string -> Async> -removeRoot: string -> Async> -getRoots: unit -> Async -``` -No extra Fable.Remoting registration is needed beyond implementing them in the record -(`Remoting.fromValue api` in `Program.fs`). - -### 2. Global-config roots read/write + write lock (`src/Server/WorktreeApi.fs` ~174-234) -- Reuse the existing `withConfigDocument` (read) and `updateGlobalConfig` (RMW) helpers — add - `readWorktreeRootsConfig () : string list` and `writeWorktreeRoots (roots: string list)`. -- **Add a private lock object guarding `updateGlobalConfig`** (it currently has none). All - global-config writes (collapsedRepos, canvas state, lastViewedHashes, and the new roots) go - through the locked helper so concurrent API calls can't corrupt `config.json`. This is the - mechanism that makes "server is the single, serialized writer" true. -- **Test isolation:** the global-config directory must be overridable so endpoint tests don't - touch the real `~/.treemon`. Prefer test-side isolation (the fixture sets the server process's - `USERPROFILE`/`HOME` to a temp dir); only if that proves insufficient, honor a - `TREEMON_CONFIG_DIR` env override in `globalConfigPath`. - -### 3. Scheduler — no change (restart-to-apply) -Roots are read fresh from global config at each server (re)start (§5), so the scheduler needs no -runtime add/remove machinery. `rootPaths` continues to be built once in `start` (~671). *(If live -updates are wanted later, this is the spot: add `RootPaths` to `DashboardState`, `AddRoot`/ -`RemoveRoot` `StateMsg` cases, and have the refresh loop read roots from state.)* - -### 4. API endpoint implementation (`src/Server/WorktreeApi.fs` worktreeApi ~455-466) -- Implement `addRoot`/`removeRoot`: normalize+validate path → read-modify-write via - `writeWorktreeRoots` (§2). No scheduler message. `getRoots` returns `readWorktreeRootsConfig ()`. -- `getWorktrees`/`createWorktree`/path-validation keep using the `rootPaths` captured at startup - (line 466) — correct, since roots only change across restarts. - -### 5. Server startup resolution + migration (`src/Server/Program.fs` ~46-98, 141-186) -- **Allow empty roots in `parseArgs` (~46-98).** Today the final match arm (~95-98) prints usage and - `exit 1` whenever no positional root is given and `--demo` is absent. Change it so zero root args is - valid in normal mode (`WorktreeRoots = []`); leave `--demo`/`--test-fixtures` handling intact. This - is the change that lets `start`/`dev` run with no path. -- Add a root-resolution step before `RefreshScheduler.start`/`worktreeApi`: args → global config - → orphan import (per Expected Behavior). Persist resolved roots to global config if absent; - delete `~/.treemon/roots.json` after import. -- Pass the resolved roots to both `RefreshScheduler.start` and `worktreeApi` as today. Demo/ - fixture modes bypass this (unchanged). - -### 6. CLI subcommands (`src/Cli/Program.fs`) -Add `addCmd`, `removeCmd`, `rootsCmd` following the `newCmd` idiom (~164-187): `command "add" { -inputs (...) ; setAction handler }`, calling via `runApi`/`tryCallServer`. Register them in -`main` (~218-227). `add`/`remove` accept one-or-more paths; loop calling the endpoint per path. - -### 7. treemon.ps1 cleanup + migration (`treemon.ps1`) -- Delete `Save-Config`, `Get-SavedConfig`, `Resolve-WorktreeRoots`, `Add-Roots`, `Remove-Roots`, - the legacy `WorktreeRoot` (singular) fallback, and the `add`/`remove` switch arms. -- `start`/`dev`: make the path argument optional; pass through whatever paths are given. -- `restart`/`deploy`: stop relying on `Get-SavedConfig` — restart with no path args (server reads - global config). -- `status`: list roots via `tm roots`. -- Migration shim: if `$ConfigFile` (`.treemon.config`) exists, read its `WorktreeRoots`, pass them - to the next `Start-ProductionServer` as args (one time), then delete the file. - -### 8. Docs (`docs/spec/worktree-monitor.md`, `AGENTS.md`, `README.md`) -Update the config section + the `add`/`remove` command lines to describe `tm add`/`tm remove`, -the global `worktreeRoots` key, and that `start`/`dev` no longer need a path. - -## Decisions - -- **Restart-to-apply (chosen for simpler code).** `addRoot`/`removeRoot` only persist to global - config; the new roots take effect when the server (re)starts, which the `treemon.ps1` - add/remove shims trigger automatically when the server is running (matching today's behavior). - This avoids the scheduler-state machinery a live model would need (§3). Live application remains - a clean future extension. -- **Server is the single writer of `config.json`**, with an added write lock (§2). The CLI never - writes config files (online-only, confirmed). This is the fix for the cross-process clobber + - the missing intra-process lock. -- **add/remove require the server running** (online-only CLI). Cold bootstrap: `treemon.ps1 start` - with no path launches an empty dashboard (empty roots is valid, as in demo mode), then `tm add` - populates it. -- **Roots become a per-machine singleton.** Dev and prod instances on the same machine now share - one global roots list, instead of independent per-script-dir `.treemon.config` files. Accepted - as the intended simplification. -- **Keep `treemon.ps1 add/remove` as thin shims** that call `tm` then restart the server if - running. `tm` is installed on PATH by `deploy`. Persistence is server-side (single writer); - process lifecycle stays in PowerShell — clean separation. -- **Orphan `~/.treemon/roots.json`** is migrated-then-deleted by the server; `.treemon.config` is - migrated-then-deleted by `treemon.ps1`. -- **treemon.ps1 §7 implementation edge cases.** Three robustness details settled while implementing - the shims: (1) **Migration file deleted only after a confirmed start.** `Read-LegacyRoots` is a - pure reader; `Start-ProductionServer` removes `.treemon.config` *after* the post-launch - `HasExited` success check (by which point the server has persisted the roots into the global - config), so a `dotnet publish` failure or an immediate server crash can never lose the migrated - roots. (2) **Null/empty roots are filtered before building args.** `start`/`dev`/`restart` pass - `$WorktreeRoots` straight through, but with `[ValueFromRemainingArguments]` an omitted path binds - the parameter to `$null` (and `@($null)` is a *1-element* array), so `Start-ProductionServer`/ - `Start-DevMode` normalize via `@($Roots | Where-Object { $_ })` — otherwise the "no path" feature - threw on `$null.TrimEnd()` (and `restart` would kill prod then throw). (3) **add/remove restart - on any genuine change (tri-state exit code — fix tm-config-audit-rf2).** The CLI folds a path - batch into a tri-state exit code (`Cli.foldRootResults`): `0` = all succeeded, `1` = all failed, - `2` = partial. The shims gate the restart on `$tmExit -eq 0 -or $tmExit -eq 2` and keep - `exit $tmExit`, so prod restarts whenever at least one root was actually persisted (full or partial - success) while a fully-failed call (bad path, server down) still skips the restart and reports - non-zero. The original *binary* gate (`$tmExit -eq 0`) was the bug: `addRootToConfig` persists each - accepted path immediately, so a `[valid; invalid]` batch persisted `valid` but returned exit 1 — - the shim skipped the restart and the new root stayed dormant despite the printed success line. - `Invoke-Tm` routes the CLI's stdout to the host (`| Out-Host`) and returns only `[int]$LASTEXITCODE`, - so the exit code stays a scalar and the CLI's own messages aren't swallowed. -- **Config-dir test isolation uses `TREEMON_CONFIG_DIR`, not a fixture-set `USERPROFILE`/`HOME`.** - The spec preferred redirecting `USERPROFILE`/`HOME`, but on Windows .NET 9 - `Environment.GetFolderPath(SpecialFolder.UserProfile)` reads the user token, **not** the env - vars (empirically confirmed), so in-process unit tests can't redirect it that way. Per the - spec's allowed fallback, `WorktreeApi.globalConfigDir ()` honors a `TREEMON_CONFIG_DIR` override - (the directory that holds `config.json`, i.e. the `~/.treemon` equivalent); `globalConfigDir` is - `internal` so §5's `Program.fs` orphan-`roots.json` handling can resolve the same dir. Note this - override only covers `WorktreeApi`'s global reads/writes; the deferred-consolidation duplicate - reader in `TreemonConfig.fs` still targets the real `~/.treemon` — so a future endpoint/server - test that needs `TreemonConfig`-mediated global reads isolated must run in a separate process - whose `USERPROFILE`/`HOME` point at a temp dir (a server process *does* honor those for path - building), or that reader must also adopt the override. - -- **`addRoot`/`removeRoot` surface persistence failures.** `updateGlobalConfig` was changed to - return `Result` (it previously logged-and-swallowed write exceptions, which would - have made the endpoints report a false `Ok()` on a failed write); `writeWorktreeRoots` - propagates that outcome so the endpoints return `Error` when the config write fails. The four - best-effort UI-state writers (collapsedRepos, canvas open/position, lastViewedHashes) explicitly - `|> ignore` the result — their `save*` members are `Async` and a dropped UI-state write is - non-critical. The roots read-modify-write reads (`readWorktreeRootsConfig`) outside the lock and - writes (`writeWorktreeRoots`) inside it; the read-then-write window is accepted because add/remove - are driven by the serialized, online-only CLI (one path per call), so it is uncontended in - practice. A future live model would move the read inside the locked `updateGlobalConfig` callback. - -- **Startup resolution (§5 / `Program.fs`).** `parseArgs` now accepts zero positional roots in - normal mode (the previously-`exit 1` "usage" arm was removed; it had become unreachable once the - `roots <> []` guard was dropped, so the top-level match stays exhaustive with no redundant arm). - `resolveWorktreeRoots` resolves CLI args > global `worktreeRoots` > orphan `roots.json`, and - persists the resolved set **only when `config.json` has no `worktreeRoots` *key* yet** (absence, - not mere emptiness — see the missing-vs-empty decision below) — so a present config is never - clobbered and CLI args act as an *ephemeral* override there (they win for the run but don't - rewrite the durable config). The orphan is read by a *pure* reader and deleted **only - after a successful persist** (not before): an eager read-then-delete would silently lose the - migrated set if the config write failed. Consequently the orphan is consumed only on the - priority-3 path (no args, no `worktreeRoots` key); with args present (e.g. `treemon.ps1` migrating - `.treemon.config`) the args win and a still-present orphan is left untouched. Orphan roots are - migrated verbatim (no `GetFullPath`/existence check) — downstream comparisons canonicalize at - compare time and the scheduler tolerates missing dirs. -- **Missing-vs-empty `worktreeRoots` (fix tm-config-audit-rf1).** Startup resolution must - distinguish a *missing* `worktreeRoots` key (fresh install / pre-migration) from a *present but - empty* one (the user curated every root away). The original `readWorktreeRootsConfig` collapsed - both to `[]`, so an explicit `worktreeRoots:[]` was treated like a fresh install and got - repopulated on restart — a stale orphan `roots.json` resurrected removed roots, and CLI args - overwrote the explicit empty. Fix: `WorktreeApi.tryReadWorktreeRootsConfig () : string list option` - is the presence-aware reader (`None` = key absent, `Some []` = explicit empty, `Some roots` = - populated; a malformed non-array value reports `None` to preserve the old lenient behavior). - `readWorktreeRootsConfig () : string list` stays as a thin `Option.defaultValue []` wrapper so the - `getRoots` endpoint and add/remove read-modify-write are unchanged. `resolveWorktreeRoots` gates - BOTH the orphan-import fallthrough AND the first-time persist on key **absence** - (`Option.isSome configRoots`), never on `List.isEmpty`. Result: a present `worktreeRoots:[]` is - the priority-2 source (resolves to `[]`, no orphan import) and is never persisted over, so removed - roots stay removed across restarts. Regression coverage: - `ServerStartupResolutionTests` — orphan-present and CLI-args-present each leave the explicit empty - intact. -- **Demo/fixture modes pass `[]` to `worktreeApi`/scheduler** (resolution is bypassed entirely). - This is behaviorally inert for fixture mode because `worktreeApi`'s fixture branch is built from - `readOnlyApi` and ignores `rootPaths`; passing `[]` matches the spec's "(roots stay [])". -- **Read-only `addRoot`/`removeRoot` return `Error` (fix tm-config-audit-rf3).** In `--demo`/ - `--test-fixtures`, `readOnlyApi` wires these to `Error $"Root management is not available in - {modeName}"` (matching every other unsupported mutation), not a silent `Ok()`. The old `Ok()` - made the CLI print `✓ Added … (applies on next server restart)` against a read-only server even - though nothing is persisted (these modes force `worktreeRoots=[]`). `getRoots` still returns `[]` - (a read, correctly empty). -- **Smoke tests isolate the config dir.** Both `SmokeTests` fixtures start the server in *normal* - mode with real roots, which now triggers the startup persist; without isolation that would write - the developer's real `~/.treemon`. Each fixture points the child server at a throwaway - `TREEMON_CONFIG_DIR` and deletes it in teardown. New in-process coverage lives in - `src/Tests/ServerStartupResolutionTests.fs` (parseArgs empty-roots + resolveWorktreeRoots - priority/persist/no-clobber/orphan-migration), isolated via the same `TREEMON_CONFIG_DIR` override - used by `WorktreeRootsConfigTests`. - -## Key Files - -| File | Role in this change | -|---|---| -| `src/Shared/Types.fs` | `IWorktreeApi` — add `addRoot`/`removeRoot`/`getRoots` | -| `src/Server/WorktreeApi.fs` | global-config roots read/write + write lock; endpoint impls; read live roots | -| `src/Server/RefreshScheduler.fs` | no change (roots re-read at restart); future home for live updates | -| `src/Server/Program.fs` | startup root resolution + orphan migration; pass resolved roots | -| `src/Cli/Program.fs` | `add`/`remove`/`roots` subcommands | -| `treemon.ps1` | strip roots logic; optional path for start/dev; status via `tm roots`; `.treemon.config` migration | -| `docs/spec/worktree-monitor.md`, `AGENTS.md`, `README.md` | docs | - -## Related Specs -- `docs/spec/worktree-monitor.md` — overall architecture and the existing config section this - amends. -- Investigation: `.agents/config-files-investigation.md` — full config inventory and the A/B/C/D - options; this spec implements D. diff --git a/src/Server/Program.fs b/src/Server/Program.fs index 8166b4d..73162b0 100644 --- a/src/Server/Program.fs +++ b/src/Server/Program.fs @@ -172,18 +172,27 @@ let private deleteOrphanRoots () = with ex -> Log.log "Startup" $"Failed to delete orphan roots.json: {ex.Message}" +/// Outcome of resolving the effective worktree roots at startup: the resolved set plus whether the +/// boundary should persist it (first-time migration into the global config) and consume the orphan +/// `roots.json`. A pure decision — the caller (`persistResolvedRoots`) performs the write/delete. +type internal RootsResolution = + { Roots: string list + /// First-time persist: the `worktreeRoots` key is absent and the resolved set is non-empty. + PersistRoots: bool + /// Resolved set came from the orphan `roots.json` — delete it after a successful persist. + ConsumeOrphan: bool } + /// Resolves the effective worktree roots at startup by priority: /// 1. roots passed as CLI args (used by `dev`/tests; preserves current arg behavior), /// 2. else `worktreeRoots` from the global `config.json` (a PRESENT key, even an explicit empty /// list, wins here — the user may have curated every root away), /// 3. else (the key is ABSENT) a one-time import of the orphan `roots.json`. -/// When `config.json` has no `worktreeRoots` KEY yet, the resolved set is persisted so an -/// arg-provided or migrated set becomes the durable source of truth. A present key (even -/// `worktreeRoots:[]`) is left untouched, so CLI args act as an ephemeral override and an explicit -/// empty is never repopulated on restart. An orphan-sourced set is deleted only after that persist -/// succeeds — never before — so a failed write can't drop the migration. Demo/fixture modes never -/// call this; their roots stay `[]`. -let internal resolveWorktreeRoots (cliRoots: string list) : string list = +/// Pure/read-only: it only reads config + orphan state and decides the resolved set, whether a +/// first-time persist is needed, and whether the orphan should be consumed — it performs no writes +/// or deletes, so the resolution decision is unit-testable without mutating the filesystem. +/// `persistResolvedRoots` applies those effects at the boundary. Demo/fixture modes never call this; +/// their roots stay `[]`. +let internal resolveWorktreeRoots (cliRoots: string list) : RootsResolution = // `None` = the `worktreeRoots` key is absent (fresh install / pre-migration); `Some roots` = // the key is present (possibly an explicit empty list). Gating migration on KEY ABSENCE — not // `List.isEmpty` — is what stops an explicit `worktreeRoots:[]` from being resurrected by a @@ -200,16 +209,22 @@ let internal resolveWorktreeRoots (cliRoots: string list) : string list = let orphanRoots = readOrphanRoots () orphanRoots, not (List.isEmpty orphanRoots) - if not configHasKey && not (List.isEmpty resolved) then - match WorktreeApi.writeWorktreeRoots resolved with + { Roots = resolved + PersistRoots = not configHasKey && not (List.isEmpty resolved) + ConsumeOrphan = cameFromOrphan } + +/// Boundary effect for `resolveWorktreeRoots`: persists a first-time-resolved root set into the +/// global config and deletes the migrated orphan `roots.json` only after that write succeeds (so a +/// failed write can never drop the migration). A no-op when the resolution needs no persistence. +let internal persistResolvedRoots (resolution: RootsResolution) = + if resolution.PersistRoots then + match WorktreeApi.writeWorktreeRoots resolution.Roots with | Ok () -> - Log.log "Startup" $"Persisted {List.length resolved} worktree root(s) to global config" - if cameFromOrphan then deleteOrphanRoots () + Log.log "Startup" $"Persisted {List.length resolution.Roots} worktree root(s) to global config" + if resolution.ConsumeOrphan then deleteOrphanRoots () | Error msg -> Log.log "Startup" $"Failed to persist worktree roots: {msg}" - resolved - [] let main args = let config = parseArgs args @@ -218,11 +233,17 @@ let main args = Log.init () - // Effective roots: CLI args > global config > orphan import (persisted first-time). Demo and - // fixture modes bypass resolution entirely — they serve synthetic data, so roots stay []. + // Effective roots: CLI args > global config > orphan import (persisted first-time). Resolution + // is a pure decision; `persistResolvedRoots` applies the first-time persist + orphan cleanup at + // this startup boundary. Demo and fixture modes bypass resolution entirely — they serve + // synthetic data, so roots stay []. let worktreeRoots = - if config.Demo || config.TestFixtures.IsSome then [] - else resolveWorktreeRoots config.WorktreeRoots + if config.Demo || config.TestFixtures.IsSome then + [] + else + let resolution = resolveWorktreeRoots config.WorktreeRoots + persistResolvedRoots resolution + resolution.Roots worktreeRoots |> List.iter (fun root -> Log.log "Startup" $"Worktree root: {root}") Log.log "Startup" $"Server URL: {serverUrl}" diff --git a/src/Tests/ServerStartupResolutionTests.fs b/src/Tests/ServerStartupResolutionTests.fs index e83fded..6179ff0 100644 --- a/src/Tests/ServerStartupResolutionTests.fs +++ b/src/Tests/ServerStartupResolutionTests.fs @@ -61,9 +61,13 @@ type ServerStartupResolutionTests() = member _.``resolveWorktreeRoots persists CLI args when config has no roots``() = withTempConfigDir "treemon-startup-test" (fun _ -> let cliRoots = [ @"C:\code\one"; @"C:\code\two" ] - let resolved = resolveWorktreeRoots cliRoots - Assert.That(resolved, Is.EqualTo(cliRoots)) - // First-time persistence: the arg-provided set becomes the durable source of truth. + let resolution = resolveWorktreeRoots cliRoots + Assert.That(resolution.Roots, Is.EqualTo(cliRoots)) + // First-time persist decision: the arg-provided set should become the durable source. + Assert.That(resolution.PersistRoots, Is.True) + Assert.That(resolution.ConsumeOrphan, Is.False) + // Applying the resolution at the boundary writes it to the global config. + persistResolvedRoots resolution Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(cliRoots))) [] @@ -74,10 +78,13 @@ type ServerStartupResolutionTests() = | Ok () -> () | Error msg -> Assert.Fail $"setup write failed: {msg}" - let resolved = resolveWorktreeRoots [ @"C:\code\arg" ] + let resolution = resolveWorktreeRoots [ @"C:\code\arg" ] // CLI args win for this run... - Assert.That(resolved, Is.EqualTo([ @"C:\code\arg" ])) - // ...but a populated config is an ephemeral override, not clobbered. + Assert.That(resolution.Roots, Is.EqualTo([ @"C:\code\arg" ])) + // ...and because the config key is present, this is not a first-time persist. + Assert.That(resolution.PersistRoots, Is.False) + persistResolvedRoots resolution + // ...so a populated config is an ephemeral override, not clobbered. Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(configured))) [] @@ -88,8 +95,9 @@ type ServerStartupResolutionTests() = | Ok () -> () | Error msg -> Assert.Fail $"setup write failed: {msg}" - let resolved = resolveWorktreeRoots [] - Assert.That(resolved, Is.EqualTo(configured))) + let resolution = resolveWorktreeRoots [] + Assert.That(resolution.Roots, Is.EqualTo(configured)) + Assert.That(resolution.PersistRoots, Is.False)) [] member _.``resolveWorktreeRoots imports orphan roots.json then persists and deletes it``() = @@ -97,19 +105,28 @@ type ServerStartupResolutionTests() = let orphanRoots = [ @"C:\code\orphan-a"; @"C:\code\orphan-b" ] writeOrphan tempDir orphanRoots - let resolved = resolveWorktreeRoots [] + let resolution = resolveWorktreeRoots [] + + Assert.That(resolution.Roots, Is.EqualTo(orphanRoots)) + Assert.That(resolution.PersistRoots, Is.True) + Assert.That(resolution.ConsumeOrphan, Is.True) + // Resolution is pure: the orphan is still on disk until the boundary applies it. + Assert.That(File.Exists(Path.Combine(tempDir, "roots.json")), Is.True) + + persistResolvedRoots resolution - Assert.That(resolved, Is.EqualTo(orphanRoots)) // Migrated set persisted into the global config... Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(orphanRoots)) - // ...and the orphan file is consumed (read-then-delete). + // ...and the orphan file is consumed (deleted) only after a successful persist. Assert.That(File.Exists(Path.Combine(tempDir, "roots.json")), Is.False)) [] member _.``resolveWorktreeRoots returns empty when no args, no config, no orphan``() = withTempConfigDir "treemon-startup-test" (fun _ -> - let resolved = resolveWorktreeRoots [] - Assert.That(resolved, Is.Empty) + let resolution = resolveWorktreeRoots [] + Assert.That(resolution.Roots, Is.Empty) + Assert.That(resolution.PersistRoots, Is.False) + persistResolvedRoots resolution // Nothing to persist, so the config stays absent/empty. Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.Empty)) @@ -128,10 +145,13 @@ type ServerStartupResolutionTests() = // A stale orphan roots.json from a legacy upgrade still lingers on disk. writeOrphan tempDir [ @"C:\code\removed-a"; @"C:\code\removed-b" ] - let resolved = resolveWorktreeRoots [] + let resolution = resolveWorktreeRoots [] // The orphan must NOT resurrect the removed roots for this run... - Assert.That(resolved, Is.Empty) + Assert.That(resolution.Roots, Is.Empty) + // ...and a present (empty) key means no first-time persist... + Assert.That(resolution.PersistRoots, Is.False) + persistResolvedRoots resolution // ...the explicit empty config is preserved (present key, still empty — not None, not // repopulated with the orphan's roots)... match Server.WorktreeApi.tryReadWorktreeRootsConfig () with @@ -147,10 +167,12 @@ type ServerStartupResolutionTests() = | Ok () -> () | Error msg -> Assert.Fail $"setup write failed: {msg}" - let resolved = resolveWorktreeRoots [ @"C:\code\arg" ] + let resolution = resolveWorktreeRoots [ @"C:\code\arg" ] // CLI args still win for THIS run (ephemeral override)... - Assert.That(resolved, Is.EqualTo([ @"C:\code\arg" ])) + Assert.That(resolution.Roots, Is.EqualTo([ @"C:\code\arg" ])) + Assert.That(resolution.PersistRoots, Is.False) + persistResolvedRoots resolution // ...but the explicit empty config is not clobbered, so a restart with no args stays empty. match Server.WorktreeApi.tryReadWorktreeRootsConfig () with | Some [] -> () diff --git a/treemon.ps1 b/treemon.ps1 index 80d3281..d42b598 100644 --- a/treemon.ps1 +++ b/treemon.ps1 @@ -44,28 +44,48 @@ if (-not $Command) { exit 0 } -function Read-LegacyRoots { - # One-time migration of the legacy PowerShell-managed .treemon.config. - # Reads its WorktreeRoots and returns them (or @()). Does NOT delete the file — - # Start-ProductionServer removes it only after the server has actually started - # (and thus persisted the roots into ~/.treemon/config.json), so a publish/start - # failure can't lose the roots. - if (-not (Test-Path $ConfigFile)) { return @() } +function Get-LegacyConfig { + # Parses the legacy .treemon.config and extracts its watched roots, accepting BOTH the current + # plural `WorktreeRoots` array and the pre-multi-repo singular `WorktreeRoot` string (older + # versions wrote the singular key). Returns a result object { Parsed; Roots } so callers can tell + # a parse failure (Parsed=$false) apart from a successfully-parsed file that simply declares no + # roots (Parsed=$true, empty Roots) — a distinction PowerShell's empty-array collapse erases if + # you signal it through the return value. The object property keeps Roots a real array. + if (-not (Test-Path $ConfigFile)) { + return [pscustomobject]@{ Parsed = $true; Roots = @() } + } - $roots = @() try { $config = Get-Content $ConfigFile -Raw | ConvertFrom-Json - if ($config.PSObject.Properties.Name -contains "WorktreeRoots") { - $roots = @($config.WorktreeRoots | Where-Object { $_ }) - } } catch { - Write-Host "Warning: could not parse legacy .treemon.config; it will be removed after the server starts" -ForegroundColor Yellow + return [pscustomobject]@{ Parsed = $false; Roots = @() } } - if ($roots.Count -gt 0) { + $roots = @() + if ($config.PSObject.Properties.Name -contains "WorktreeRoots") { + $roots = @($config.WorktreeRoots | Where-Object { $_ }) + } elseif ($config.PSObject.Properties.Name -contains "WorktreeRoot") { + $roots = @($config.WorktreeRoot | Where-Object { $_ }) + } + return [pscustomobject]@{ Parsed = $true; Roots = @($roots) } +} + +function Read-LegacyRoots { + # One-time migration of the legacy PowerShell-managed .treemon.config. Returns its roots (or + # @()). Does NOT delete the file — Start-ProductionServer removes it only after the server has + # started (and thus persisted the roots into ~/.treemon/config.json) AND only when every root it + # declared was actually migrated, so a publish/start failure or an unrecognized config can't + # silently lose roots. + $legacy = Get-LegacyConfig + if (-not $legacy.Parsed) { + Write-Host "Warning: could not parse legacy .treemon.config; leaving it in place to avoid data loss" -ForegroundColor Yellow + return @() + } + + if ($legacy.Roots.Count -gt 0) { Write-Host "Migrating worktree roots from .treemon.config into global config" -ForegroundColor Gray } - return @($roots) + return @($legacy.Roots) } function Invoke-Tm([string[]]$TmArgs) { @@ -201,10 +221,26 @@ function Start-ProductionServer([string[]]$Roots) { exit 1 } - # Server is up — it has resolved+persisted its roots into the global config, so the - # legacy .treemon.config (if any) is now safe to retire. Removing it only after a - # confirmed start means a publish/start failure never loses the migrated roots. - if (Test-Path $ConfigFile) { Remove-Item $ConfigFile -ErrorAction SilentlyContinue } + # Server is up — it has resolved+persisted its effective roots into the global config. Retire + # the legacy .treemon.config only when it is SAFE: every root it declared was actually migrated + # (handed to the server). A file we couldn't parse, or one whose roots we didn't migrate (e.g. an + # explicit-path start that ignored it), is preserved with a warning so we never silently destroy + # unmigrated roots. Deleting only after a confirmed start also means a publish/start failure + # never loses the migrated roots. + if (Test-Path $ConfigFile) { + $legacy = Get-LegacyConfig + if (-not $legacy.Parsed) { + Write-Host "Warning: .treemon.config could not be parsed; leaving it in place to avoid data loss" -ForegroundColor Yellow + } else { + $migrated = @($effectiveRoots | ForEach-Object { $_.TrimEnd('\', '/').ToLowerInvariant() }) + $unmigrated = @($legacy.Roots | Where-Object { $_.TrimEnd('\', '/').ToLowerInvariant() -notin $migrated }) + if ($unmigrated.Count -eq 0) { + Remove-Item $ConfigFile -ErrorAction SilentlyContinue + } else { + Write-Host "Warning: .treemon.config has roots that were not migrated ($($unmigrated -join ', ')); leaving it in place to avoid data loss" -ForegroundColor Yellow + } + } + } Write-Host "Production server started (PID: $($process.Id))" -ForegroundColor Green if ($effectiveRoots.Count -gt 0) { From cd58f95e726dc0426e306c79789849c2276c982c Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Tue, 23 Jun 2026 15:23:04 +0200 Subject: [PATCH 16/17] Document remoting CSRF/Origin hardening as future work Capture the confirmed (Low) focused-review finding that the unauthenticated loopback Fable.Remoting surface accepts cross-origin simple-request CSRF writes (Fable.Remoting.Server does not enforce application/json, so a text/plain POST bypasses the CORS preflight). Recommends a pipeline-level Origin/Referer check that also covers the pre-existing process-launching endpoints; adds it to the improvements backlog. --- docs/spec/future/code-improvements.md | 1 + docs/spec/future/remoting-csrf-hardening.md | 138 ++++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 docs/spec/future/remoting-csrf-hardening.md diff --git a/docs/spec/future/code-improvements.md b/docs/spec/future/code-improvements.md index 1d128e5..737ee88 100644 --- a/docs/spec/future/code-improvements.md +++ b/docs/spec/future/code-improvements.md @@ -38,6 +38,7 @@ its own worktree. This file is the entry point; detailed designs live in their o | 3 | **Canvas roadmap items** — follow-on canvas-pane enhancements | `docs/spec/future/canvas-roadmap.md` | Idea | | 4 | **Process: guard against spec drift** — a lightweight check (or review rule) that flags `Key Files` references to moved/renamed modules so docs can't silently rot after refactors | — | Idea | | 5 | **Survey other large modules** — apply the same view/state/update extraction lens to the next-largest files (server-side `RefreshScheduler.fs`, `WorktreeApi.fs`, etc.) if they mix concerns | — | Idea (needs investigation) | +| 6 | **Remoting CSRF / Origin hardening** — pipeline-level Origin/Referer (and optional custom-header) check so a cross-origin browser page can't drive the unauthenticated loopback Fable.Remoting API (covers the dangerous pre-existing process-launching endpoints, not just watched-roots) | `docs/spec/future/remoting-csrf-hardening.md` | Idea (from focused-review) | > Add new candidates here as they surface (often from focused-review findings). Keep the list > honest: remove ones that turn out not to be worth it, and record why in the relevant spec. diff --git a/docs/spec/future/remoting-csrf-hardening.md b/docs/spec/future/remoting-csrf-hardening.md new file mode 100644 index 0000000..e396762 --- /dev/null +++ b/docs/spec/future/remoting-csrf-hardening.md @@ -0,0 +1,138 @@ +# Remoting CSRF / Origin Hardening + +Status: **Future / Deferred** — design only. Surfaced by focused-review (finding C-13 / +A-13, *Confirmed · Low*) on the `tm-config-audit` branch. Not implemented. + +The branch that surfaced this only **added** three endpoints (`addRoot`/`removeRoot`/ +`getRoots`) to an already-exposed surface; it did not create the gap. This spec is about +hardening the *whole* remoting surface, not those three endpoints. + +## Problem Statement + +The server exposes `IWorktreeApi` over Fable.Remoting with **no authentication and no +Origin/CSRF validation**, bound to loopback. A malicious web page open in the operator's +browser can therefore issue cross-origin `POST http://localhost:5000/IWorktreeApi/` +requests that the server executes as if they were same-origin — a classic localhost CSRF. + +The newly-added watched-roots endpoints are the *least* dangerous members of this surface. +The same handler already exposes state-changing, process-launching endpoints that share the +identical exposure — e.g. `launchSession`, `launchAction`, `resumeSession`, +`sendCanvasMessage` (builds and launches a coding-tool CLI process), `createWorktree`, +`deleteWorktree`, `openTerminal`, `openEditor`. If localhost CSRF is in scope at all, those +are the high-value targets; fixing it once at the pipeline protects all of them. + +### Why the usual mitigations don't close it (verified) + +- **No control exists.** `buildRemotingHandler` (`src/Server/Program.fs`) is + `createApi → fromValue → withErrorHandler → buildHttpHandler` and the Saturn pipeline is + `use_router / url / use_static / use_gzip` — no `use_cors`, no Origin/Referer middleware, + no CSRF token, no auth. A tree-wide grep for `cors|origin|access-control|referer| + x-requested-with` in `src/Server` finds only an unrelated git remote and same-origin + client JS. +- **Loopback bind is not a defense for this threat model.** The malicious page runs in a + browser *on the same machine*, which can reach `http://localhost:5000`. The default port + (`5000`) is well-known, so the attacker need not guess it. +- **The CORS-preflight "mitigation" does not apply.** A cross-origin `fetch` with + `Content-Type: application/json` would be a *non-simple* request and trigger a preflight + the no-CORS server fails — but Fable.Remoting.Server (verified against the resolved + `5.42.0`) does **not** enforce `application/json`. Its dispatcher requires `POST` for + argument-taking methods, then the only content-type branch is `multipart/form-data`; every + other type — including `text/plain` — falls through to + `StreamReader.ReadToEndAsync()` → `JsonConvert.DeserializeObject` and is dispatched. + A cross-origin `text/plain` POST is a CORS **simple request** (no preflight), so its + JSON-array body reaches the method. The attacker can't read the response (that *is* + CORS-blocked), which is irrelevant for a state-changing call. + +### Impact ceiling (why this is Low, not High) + +- The exposure is **pre-existing** — this is a latent design property of the remoting + handler, not a regression. +- For the roots endpoints specifically: `removeRoot` is a blind guess (the attacker must + supply the exact canonical watched path and can't enumerate it, since `getRoots`' response + is CORS-blocked from being read); `addRoot` only accepts a directory that already exists on + the victim's disk and its effect is deferred to the next server restart. Worst realistic + outcome there is polluting/clearing one config key or a wasteful filesystem walk on + restart — integrity/availability, never code execution. +- The real value of acting is the **process-launching** endpoints, where a forged call has + materially higher impact. + +## Goals + +- A cross-origin browser page **cannot** invoke any `IWorktreeApi` method. +- The fix is applied **once, at the remoting pipeline**, so every method (current and future) + is covered without per-endpoint changes. +- **No friction for legitimate callers**: the same-origin web client and the `tm` CLI keep + working unchanged (or with a single, central client change). +- Production keeps its well-known port and loud-on-conflict behavior (see + `docs/spec/future/port-management.md`); this is orthogonal to where it binds. + +## Technical Approach + +Add a small middleware *in front of* the Fable.Remoting handler in `buildRemotingHandler` +(or wrapping it in the Saturn pipeline) that rejects forged cross-origin requests before they +reach any method. + +**Primary control — Origin/Referer allowlist (no client change):** + +- If an `Origin` header is present and is **not** same-origin (`http://localhost:{port}` / + `http://127.0.0.1:{port}`), reject with `403`. +- If `Origin` is absent, fall back to a `Referer` same-origin check. +- Requests with **neither** header (the `tm` CLI via `HttpClient`, server-to-server) are + allowed — non-browser clients don't forge CSRF. + +Browsers attach `Origin` to cross-origin POSTs (and to same-origin POSTs), so the legitimate +web client passes and the malicious page is blocked, with **zero client changes**. This is +the lowest-friction option. + +**Defense in depth — required custom header (one central client change):** + +- Require a non-simple custom request header (e.g. `x-requested-by: treemon`) on every + remoting call. A CORS *simple request* cannot set a custom header without triggering a + preflight the no-CORS server will fail, so cross-origin pages are blocked at the browser. +- This needs the Fable.Remoting client to be configured to send the header on every call + (one place, via the client's header/route customization), and the `tm` CLI's `HttpClient` + to add the same header. + +Requiring `Content-Type: application/json` is **necessary but not sufficient** on its own +(confirmed the library doesn't enforce it and `text/plain` bypasses it), so it is not the +control — the Origin check and/or custom-header are. + +Recommended: ship the **Origin/Referer allowlist** first (smallest, no client change), and +optionally add the custom-header check as belt-and-suspenders. + +## Decisions + +- **Fix at the pipeline, not per-endpoint.** The vulnerability is a property of the shared + handler; a single middleware covers the dangerous pre-existing endpoints too. Per-endpoint + guards would be incomplete and easy to forget on the next method added. +- **Origin/Referer over CSRF tokens.** A stateless Origin allowlist needs no session, token + store, or client plumbing — appropriate for a single-user localhost tool. CSRF tokens would + be over-engineered here. +- **Allow header-less requests.** The non-browser `tm` CLI legitimately sends no `Origin`; + CSRF is exclusively a browser-driven attack, so absence-of-Origin is safe to allow and + avoids breaking the CLI. +- **`application/json` enforcement is not the control.** Kept only as optional extra + hardening; the verified `text/plain` fall-through means it can't stand alone. + +## Key Files + +- **Server**: `src/Server/Program.fs` — `buildRemotingHandler` (add the Origin/Referer + middleware here) and the Saturn app builder (`use_router/url/use_static/use_gzip`); the + loopback `serverUrl` + default port feed the same-origin allowlist value. +- **Shared contract (context)**: `src/Shared/Types.fs` — `IWorktreeApi` (all methods covered + by the pipeline fix; the roots methods that surfaced this are `addRoot`/`removeRoot`/ + `getRoots`). +- **Implementations (context)**: `src/Server/WorktreeApi.fs` — `addRootToConfig` / + `removeRootFromConfig` / `writeWorktreeRoots` and the pre-existing process-launching members. +- **Client (only if custom-header option is chosen)**: the Fable.Remoting client setup in + `src/Client/` (central header/route config) and the `tm` CLI `HttpClient` in + `src/Cli/Program.fs`. + +## Verification + +- A cross-origin `text/plain` POST to `removeRoot`/`addRoot` (and a process-launching method) + is rejected `403` once the middleware is in place. +- The same-origin web client and `tm add` / `tm remove` continue to work unchanged (Origin + option) or after the single client header change (custom-header option). +- Full suite stays green (Unit + Fast + E2E); E2E exercises the same-origin client path end + to end. From 5309ed3c3a0f636f73b7130bb16c537813b5abcb Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Tue, 23 Jun 2026 15:23:05 +0200 Subject: [PATCH 17/17] focused-review: inherit orchestrator model for architecture/bugs concerns Replace the pinned claude-opus-4.6 concern model with inherit so those concerns run on whatever model drives the review. --- review/concerns/architecture.md | 2 +- review/concerns/bugs.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/review/concerns/architecture.md b/review/concerns/architecture.md index 25ec9c8..5d8eb81 100644 --- a/review/concerns/architecture.md +++ b/review/concerns/architecture.md @@ -1,6 +1,6 @@ --- type: concern -models: [gemini-3-pro-preview, gpt-5.3-codex, claude-opus-4.6] +models: [gemini-3-pro-preview, gpt-5.3-codex, inherit] priority: standard --- # Architecture Reviewer diff --git a/review/concerns/bugs.md b/review/concerns/bugs.md index 9089c7f..f5ae4ce 100644 --- a/review/concerns/bugs.md +++ b/review/concerns/bugs.md @@ -1,6 +1,6 @@ --- type: concern -models: [gemini-3-pro-preview, gpt-5.3-codex, claude-opus-4.6] +models: [gemini-3-pro-preview, gpt-5.3-codex, inherit] priority: high --- # Bug Finder