Add devctl restart, and restart a server when a config it reads at boot changes - #15
Merged
Conversation
Agents wrote `devctl stop X && devctl ensure X` by hand in a dozen sessions, and several guessed the verb already existed. That pair has two defects a single transition removes: another session's ensure can land between the two commands, and a refusal (a held resource, a paused server, a config that no longer parses) arrives only after the server is already down. Every refusal now happens before anything stops, which is what the headline test pins: moving the gate after the stop leaves the server down and the test fails on exactly that. The stop is non-retiring, so resume-on-boot survives what a deliberate stop would clear. The menu bar app drops its own stop-then-ensure pair, and the session context block names the verb, which is where the sessions that guessed at it were looking.
A supervised server outlives the session that started it, so it keeps running the config it booted with: a correct fix reads as inert and a harness keeps asserting stale behavior. A server can now list the files it reads at boot. Polling stat rather than holding descriptors, because nearly every editor and build tool saves by writing a temp file and renaming it over the target, which leaves a held descriptor pointing at an unlinked inode; the same replace is why the fingerprint carries the inode and not just mtime. The settle window folds a server's own boot-time write into the baseline, the quiet window makes one save one restart, and a burst suspends the watch with the paths named rather than looping. A hit under a live resource lock is deferred, not dropped, and fires when the hold releases. The debounce is a pure function over an injected clock, so its tests never sleep on a timer. The smoke gate asserts the restarted process printed the new config value, since a changed pid alone would not show the feature working.
Twice in roughly twenty full-suite runs, never in isolation. The assertion polls for five seconds now rather than sleeping a fixed slice, so a failure means the descendant really survived rather than that the test was impatient. Logged with what a repro would need rather than left as an unexplained flake.
There was a problem hiding this comment.
Pull request overview
Adds a first-class devctl restart daemon operation and a daemon-side watch mechanism that restarts a server when declared boot-read config files change, keeping behavior unchanged for servers that declare no watch paths.
Changes:
- Introduces
server.restartwire method + CLI (devctl restart) and app integration, implemented as one daemon-side transition that refuses before stopping. - Adds
watchto server specs/config, plus stat-based watch fingerprinting with settle/quiet/burst logic and a daemon sweep loop. - Extends tests, smoke coverage, and docs/contract text for restart and watch semantics.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/DevCtlKitTests/WatchPolicyTests.swift | Adds unit tests for watch debounce/burst/suspension and watch path resolution rules. |
| Tests/DevCtlKitTests/AgentContextTests.swift | Updates agent context “Useful” line to include devctl restart. |
| Tests/DevCtlDaemonCoreTests/WatchTests.swift | Adds daemon-core integration tests for watch-triggered restarts and lock/kill-switch behavior. |
| Tests/DevCtlDaemonCoreTests/RestartTests.swift | Adds daemon-core integration tests for restart semantics (lock refusal, broken config, unknown server). |
| Sources/fixture-server/main.swift | Extends fixture server to print a config file once and to self-touch a file for watch testing. |
| Sources/DevCtlKit/Protocol/Wire.swift | Adds server.restart wire method and RestartParams. |
| Sources/DevCtlKit/Model/Models.swift | Adds watch field to ServerSpec. |
| Sources/DevCtlKit/Config/WatchPolicy.swift | Introduces watch fingerprinting and a pure decision function for restart/suspend/wait/idle. |
| Sources/DevCtlKit/Config/WatchPaths.swift | Adds watch entry resolution/validation with warnings for ignored entries. |
| Sources/DevCtlKit/Config/ProjectConfig.swift | Plumbs watch through config loading, with warnings for invalid entries. |
| Sources/DevCtlKit/Agent/AgentContext.swift | Updates rendered agent context help text to mention restart. |
| Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift | Adds per-server watch state and evaluation, including suspension and restart recording. |
| Sources/DevCtlDaemonCore/Control/ControlServer.swift | Adds restart handler, implements restart path, and adds daemon watch sweep fan-out. |
| Sources/devctld/main.swift | Starts the periodic watch sweep after restore completes. |
| Sources/DevCtlApp/DaemonModel.swift | Switches app “restart” behavior to use server.restart instead of stop+ensure. |
| Sources/devctl/CLI.swift | Adds devctl restart command wiring and argument validation. |
| scripts/smoke.sh | Adds smoke assertions for restart behavior and watch-triggered restarts. |
| README.md | Documents restart and watch behavior at a high level. |
| docs/design.md | Documents watch semantics and how it differs from specStale. |
| docs/cli-contract.md | Extends CLI contract with restart and watch behavior/constraints. |
| BACKLOG.md | Updates backlog entries (including a new note about a flaky test). |
| AGENTS.md | Updates agent-facing codebase map and smoke guidance to include watch/restart. |
| .changeset/restart-and-watch.md | Adds changeset entry describing the new restart and watch features. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two findings from review. deferWatchRestart cleared the pending change while its own comment, the parameter it takes, and the contract text all said it keeps one. The edit was never actually lost: a refusal leaves the baseline untouched, so the next sweep sees the same difference and re-arms from scratch, which is why the test covering this path passed either way. It now keeps the stamp and only moves its timestamp forward, so the retry waits out one quiet window rather than re-deriving the same state a longer way round, and the unused parameter has a job. The watch sweep looped on `while true` with `try?` over its sleep, so a cancelled task would have spun hot, sweeping the process table as fast as the CPU allowed. Nothing cancels that task today, which is why it has never bitten, but the guard is one comparison per half second and the same shape the supervisor's own polling loops already use. No new test: both changes are equivalent to what shipped for every input, so a test written to cover them could not fail against the old code either.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
devctl restart <name>is a real command now. Agents were writingdevctl stop X && devctl ensure Xby hand, and several assumed the verb already existed. That pair has two problems: another session'sensurecan land between the two commands, and a refusal (a held resource, a paused server, a config that no longer parses) arrives only after the server is already down, leaving it down. A restart refuses before it stops anything, and it keeps the server's resume-on-boot intent, which a manual stop clears.A server can also list the config files it reads at boot but does not reload on its own:
{ "name": "web", "command": ["pnpm", "dev"], "watch": ["vite.config.ts"] }Change one and devctl restarts that server. Without it a long-lived supervised server keeps running the old config, so a correct fix looks like it did nothing and a test harness keeps checking stale behavior. A server whose framework already reloads its own config declares nothing and behaves exactly as before.
The failure modes an auto-restart invites are all closed. A config a server writes during its own startup will not bounce it. One save touching several files is a single restart, and a revert inside the window cancels the pending one. A restart refused by a held lock stays pending and fires on release rather than being dropped. A server that rewrites its own watched file has its watch suspended, with a log line naming the paths, rather than restarting forever; an explicit
devctl restartre-arms it.DEVCTL_NO_WATCH=1turns the sweep off entirely.watchandspecStalestay separate on purpose. Editingvite.config.tsrestarts the server; editingdevservers.jsonstill only reports a stale spec and bounces nothing, which is what keeps the declare-nothing default honest. Listdevservers.jsoninwatchto opt into the bounce.Notes for review
The watch mechanism polls
statrather than holding file descriptors. Nearly every editor and build tool saves by writing a temp file and renaming it over the target, after which a held descriptor names an unlinked inode and goes permanently deaf. The same rename is why the fingerprint carries the inode and not mtime alone.The debounce is a pure function over an injected clock, so its tests drive a synthetic timeline instead of sleeping. The sweep takes
nowand returns what it restarted, for the same reason.Verification
make testandscripts/smoke.shboth pass on this branch. The smoke gate gained four assertions: a restart produces a new process that comes back healthy; a restart under a live lock is refused and the server is still up afterward; a watched-file rewrite produces a new process whose log shows the new value; and a server declaring no watch is untouched by the same edit.