diff --git a/AGENTS.md b/AGENTS.md index 04d9bffd..c875f0be 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-monitor.md` (Multi-Repo). + ## 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 9cfeac2c..47a52367 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/future/code-improvements.md b/docs/spec/future/code-improvements.md index 1d128e52..737ee88f 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 00000000..e396762b --- /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. diff --git a/docs/spec/worktree-monitor.md b/docs/spec/worktree-monitor.md index d9c7af18..48c57da7 100644 --- a/docs/spec/worktree-monitor.md +++ b/docs/spec/worktree-monitor.md @@ -23,7 +23,9 @@ ### 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` → 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 @@ -256,6 +258,7 @@ 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 diff --git a/review/concerns/architecture.md b/review/concerns/architecture.md index 25ec9c8b..5d8eb810 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 9089c7fe..f5ae4ceb 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 diff --git a/src/Cli/Program.fs b/src/Cli/Program.fs index d920f0b6..bf1c56cf 100644 --- a/src/Cli/Program.fs +++ b/src/Cli/Program.fs @@ -215,6 +215,76 @@ 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 -> foldRootResults "Added" api.addRoot paths)) + + 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 -> foldRootResults "Removed" api.removeRoot paths)) + + 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 +294,7 @@ let main argv = addCommand launchCmd addCommand newCmd addCommand worktreesCmd + addCommand addCmd + addCommand removeCmd + addCommand rootsCmd } diff --git a/src/Server/Program.fs b/src/Server/Program.fs index 73f2c0ee..73162b0b 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,96 @@ 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}" + +/// 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`. +/// 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 + // 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 + else + match configRoots with + | Some roots -> roots, false + | None -> + let orphanRoots = readOrphanRoots () + orphanRoots, not (List.isEmpty orphanRoots) + + { 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 resolution.Roots} worktree root(s) to global config" + if resolution.ConsumeOrphan then deleteOrphanRoots () + | Error msg -> + Log.log "Startup" $"Failed to persist worktree roots: {msg}" + [] let main args = let config = parseArgs args @@ -143,7 +232,20 @@ 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). 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 + 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}" let appVersion = readAppVersion () @@ -155,7 +257,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 +281,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/Server/WorktreeApi.fs b/src/Server/WorktreeApi.fs index 78413ddb..f64c9b15 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 } + // 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) = let path = WorktreePath.value request.WorktreePath @@ -171,11 +175,21 @@ 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 () = + Environment.GetEnvironmentVariable("TREEMON_CONFIG_DIR") + |> Option.ofObj + |> Option.filter (fun d -> d <> "") + |> Option.defaultWith (fun () -> + Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), + ".treemon")) + 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 () @@ -209,6 +223,10 @@ 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 +/// `updateConfigAtPath`, 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 tryParseJsonObject (text: string) : System.Text.Json.Nodes.JsonObject option = @@ -262,6 +280,89 @@ let private writeCollapsedRepos (repos: RepoId list) = 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) updateGlobalConfig "collapsed repos" [ "collapsedRepos", repoArray :> System.Text.Json.Nodes.JsonNode ] +/// 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() + |> Seq.choose (fun el -> + 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, atomic single-writer path +/// (`updateConfigAtPath`), leaving every 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 = + 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) + updateConfigAtPath (globalConfigPath ()) [ "worktreeRoots", rootArray :> System.Text.Json.Nodes.JsonNode ] + +/// 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 @@ -759,4 +860,11 @@ 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 } + // 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/Shared/Types.fs b/src/Shared/Types.fs index 720fecf8..9b5872e9 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 } diff --git a/src/Tests/CliTests.fs b/src/Tests/CliTests.fs index 702749ee..da740bb3 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/src/Tests/ServerStartupResolutionTests.fs b/src/Tests/ServerStartupResolutionTests.fs new file mode 100644 index 00000000..6179ff06 --- /dev/null +++ b/src/Tests/ServerStartupResolutionTests.fs @@ -0,0 +1,179 @@ +module Tests.ServerStartupResolutionTests + +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 +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. +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 "treemon-startup-test" (fun _ -> + let cliRoots = [ @"C:\code\one"; @"C:\code\two" ] + 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))) + + [] + member _.``resolveWorktreeRoots prefers CLI args but does not overwrite a populated config``() = + withTempConfigDir "treemon-startup-test" (fun _ -> + let configured = [ @"C:\code\configured" ] + match Server.WorktreeApi.writeWorktreeRoots configured with + | Ok () -> () + | Error msg -> Assert.Fail $"setup write failed: {msg}" + + let resolution = resolveWorktreeRoots [ @"C:\code\arg" ] + // CLI args win for this run... + 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))) + + [] + member _.``resolveWorktreeRoots reads global config when no CLI args``() = + withTempConfigDir "treemon-startup-test" (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 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``() = + withTempConfigDir "treemon-startup-test" (fun tempDir -> + let orphanRoots = [ @"C:\code\orphan-a"; @"C:\code\orphan-b" ] + writeOrphan tempDir orphanRoots + + 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 + + // Migrated set persisted into the global config... + Assert.That(Server.WorktreeApi.readWorktreeRootsConfig (), Is.EqualTo(orphanRoots)) + // ...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 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)) + + // ----- 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 "treemon-startup-test" (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 resolution = resolveWorktreeRoots [] + + // The orphan must NOT resurrect the removed roots for this run... + 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 + | 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 "treemon-startup-test" (fun _ -> + match Server.WorktreeApi.writeWorktreeRoots [] with + | Ok () -> () + | Error msg -> Assert.Fail $"setup write failed: {msg}" + + let resolution = resolveWorktreeRoots [ @"C:\code\arg" ] + + // CLI args still win for THIS run (ephemeral override)... + 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 [] -> () + | other -> Assert.Fail $"expected the config to stay an explicit empty (Some []), got %A{other}") diff --git a/src/Tests/SmokeTests.fs b/src/Tests/SmokeTests.fs index 544b5302..a45e0a3c 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/TestUtils.fs b/src/Tests/TestUtils.fs index 7419232f..f07cb1d1 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/Tests.fsproj b/src/Tests/Tests.fsproj index bf0ff876..d2d66361 100644 --- a/src/Tests/Tests.fsproj +++ b/src/Tests/Tests.fsproj @@ -37,6 +37,8 @@ + + diff --git a/src/Tests/WorktreeRootsConfigTests.fs b/src/Tests/WorktreeRootsConfigTests.fs new file mode 100644 index 00000000..c0688c7a --- /dev/null +++ b/src/Tests/WorktreeRootsConfigTests.fs @@ -0,0 +1,190 @@ +module Tests.WorktreeRootsConfigTests + +open System +open System.IO +open System.Text.Json +open NUnit.Framework +open Server.WorktreeApi +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. +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) + +[] +[] +[] +// 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 "treemon-roots-test" (fun _ -> Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``writeWorktreeRoots then read round-trips roots in order``() = + withTempConfigDir "treemon-roots-test" (fun _ -> + let roots = [ @"C:\code\alpha"; @"C:\code\beta" ] + assertOk (writeWorktreeRoots roots) "writeWorktreeRoots should succeed" + Assert.That(readWorktreeRootsConfig (), Is.EqualTo(roots))) + + [] + member _.``writeWorktreeRoots preserves unrelated config keys (no clobber)``() = + 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 = + """{ + "collapsedRepos": [ "repo-one", "repo-two" ], + "editor": "vim", + "canvasPaneOpen": true +}""" + File.WriteAllText(configPath, seed) + + assertOk (writeWorktreeRoots [ @"C:\code\gamma" ]) "writeWorktreeRoots should succeed" + + // 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 _.GetString() + |> List.ofSeq + + Assert.That(collapsed, Is.EqualTo([ "repo-one"; "repo-two" ]))) + + [] + member _.``writeWorktreeRoots overwrites a previously written roots value``() = + 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 "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) ----- + // 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 "treemon-roots-test" (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + 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 "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) + | Ok () -> Assert.Fail "expected Error for a non-existent path" + Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``addRootToConfig rejects a blank path``() = + 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 "treemon-roots-test" (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + 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)) "re-adding the same root should succeed" + Assert.That(readWorktreeRootsConfig().Length, Is.EqualTo(1))) + + [] + member _.``addRootToConfig appends roots preserving insertion order``() = + 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 + 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) + Assert.That(sameRoot roots[1] beta, Is.True)) + + [] + member _.``addRootToConfig preserves unrelated config keys``() = + 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 + 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 "treemon-roots-test" (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + 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 "treemon-roots-test" (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 "treemon-roots-test" (fun tempDir -> + let root = Path.Combine(tempDir, "alpha") + Directory.CreateDirectory(root) |> ignore + assertOk (addRootToConfig root) "addRootToConfig should succeed" + Directory.Delete(root) + // Removal must not depend on the directory still existing on disk. + assertOk (removeRootFromConfig root) "removeRootFromConfig should succeed" + Assert.That(readWorktreeRootsConfig (), Is.Empty)) + + [] + member _.``addRootToConfig surfaces a persistence failure as Error``() = + 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. + 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") diff --git a/treemon.ps1 b/treemon.ps1 index ba0401fe..d42b5981 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,81 @@ 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 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 = @() } + } + + 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) } - } + } catch { + return [pscustomobject]@{ Parsed = $false; Roots = @() } } - return $null + + $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 @($legacy.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 +114,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 +178,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 +221,33 @@ function Start-ProductionServer([string[]]$Roots) { exit 1 } + # 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 - $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 +279,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 +366,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 +389,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" @@ -492,6 +485,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 @@ -501,53 +528,21 @@ 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 - $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 - } 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" { - $roots = Resolve-WorktreeRoots "start" - $roots | ForEach-Object { - if (-not (Test-Path $_)) { - Write-Host "Error: worktree root path does not exist: $_" -ForegroundColor Red - exit 1 - } - } - Start-ProductionServer $roots + Test-WorktreeRootPaths $WorktreeRoots + 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 +551,8 @@ 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 - } - } - Start-DevMode $roots + Test-WorktreeRootPaths $WorktreeRoots + Start-DevMode $WorktreeRoots } "demo" { Start-DemoMode @@ -577,13 +566,26 @@ 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 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) { Restart-ServerIfRunning } + exit $tmExit } "remove" { if (-not $WorktreeRoots -or $WorktreeRoots.Count -eq 0) { @@ -591,13 +593,16 @@ 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 - } - } - Remove-Roots $WorktreeRoots + + # 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 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) { Restart-ServerIfRunning } + exit $tmExit } "install-skill" { Install-Skill