From 82603676e7e4cbcdcd3779b1568d88efd12da4aa Mon Sep 17 00:00:00 2001 From: Evan Jacobs Date: Sun, 9 Aug 2026 15:25:54 -0400 Subject: [PATCH 1/3] feat(kit): validate inputs, screen ReDoS grep, and deadline the client ServerSpec.validationErrors is the per-spec check the register seam runs, so a directly-registered spec is screened like a committed one (bad port, empty command, a name carrying the reserved '::'). ProjectConfigLoader.validate now also rejects an empty lifecycle command, the one config field it skipped. grepRejection refuses a pattern that nests one unbounded quantifier inside another (the (a+)+ family), measured to run for seconds on a short line and never on a long one under Swift's backtracking Regex; the match runs on the log actor, so screening it up front is the only defense. DaemonClient gains a SO_RCVTIMEO response deadline, raised for a command that carries its own timeout, so a wedged daemon fails a request in bounded time instead of hanging the CLI and the app forever. --- Sources/DevCtlKit/Client/DaemonClient.swift | 38 ++++++++- Sources/DevCtlKit/Config/ProjectConfig.swift | 16 ++++ Sources/DevCtlKit/Logs/LogQuery.swift | 85 +++++++++++++++++-- Sources/DevCtlKit/Model/Models.swift | 28 ++++++ Tests/DevCtlKitTests/LogTests.swift | 24 ++++++ Tests/DevCtlKitTests/ProjectConfigTests.swift | 31 +++++++ 6 files changed, 216 insertions(+), 6 deletions(-) diff --git a/Sources/DevCtlKit/Client/DaemonClient.swift b/Sources/DevCtlKit/Client/DaemonClient.swift index 08b2e15..acd140c 100644 --- a/Sources/DevCtlKit/Client/DaemonClient.swift +++ b/Sources/DevCtlKit/Client/DaemonClient.swift @@ -16,6 +16,15 @@ public actor DaemonClient { private let socketPath: String + /** How long a single request waits for the daemon to answer before giving + up. The daemon sends nothing between the request and its one response, so + this is a whole-response deadline, not an idle gap: without it a wedged + daemon (a blocked actor, a deadlock) hangs `devctl` and the app forever, + with no output and no way out. `request` raises it for a command that + carries its own timeout so a long but healthy `ensure`, `wait`, or group + rollout is never cut off. */ + private static let defaultResponseTimeout: Double = 120 + public init(socketPath: String) { self.socketPath = socketPath } @@ -57,6 +66,7 @@ public actor DaemonClient { ) } fd = sock + setResponseTimeout(Self.defaultResponseTimeout) /** The socket is open but unproven from here, and `fd >= 0` is what the guard above reads as "already connected". So every failing exit has to put the client back to disconnected: leaving a live fd behind with @@ -99,12 +109,32 @@ public actor DaemonClient { pending = [] } + /** Sets the socket receive timeout (`SO_RCVTIMEO`); a blocking `read` then + fails with `EAGAIN` once no data arrives within the window. */ + private func setResponseTimeout(_ seconds: Double) { + guard fd >= 0 else { return } + let whole = seconds.rounded(.down) + var tv = timeval( + tv_sec: Int(whole), + tv_usec: Int32((seconds - whole) * 1_000_000)) + _ = setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, socklen_t(MemoryLayout.size)) + } + + /** `operationTimeoutSeconds` is the command's own health/wait budget, when it + has one. The response deadline is set well above it so a legitimately long + `ensure`/`wait`/group rollout (including a few dependency waves) is never + cut off, while a wedged daemon still fails in bounded time. */ public func request( _ method: WireMethod, params: P, - expecting: R.Type + expecting: R.Type, + operationTimeoutSeconds: Double? = nil ) throws -> R { try connect() + if let operationTimeoutSeconds { + setResponseTimeout(max(Self.defaultResponseTimeout, operationTimeoutSeconds * 2 + 60)) + } + defer { setResponseTimeout(Self.defaultResponseTimeout) } nextID += 1 let id = "c\(nextID)" let line = try NDJSON.encodeLine(WireRequest(id: id, method: method.rawValue, params: params)) @@ -136,6 +166,12 @@ public actor DaemonClient { } if n < 0 { if errno == EINTR { continue } + if errno == EAGAIN || errno == EWOULDBLOCK { + throw WireError( + code: .daemonUnreachable, + hint: "run: devctl daemon restart", + message: "devctld did not answer in time; it may be wedged") + } throw WireError(code: .daemonUnreachable, message: "read failed: \(String(cString: strerror(errno)))") } pending.append(contentsOf: buffer.feed(Data(scratch[0.. "))") } + /** `lifecycle` is the one config field the daemon never spawns; `devctl + switch` runs its argv locally. It was also the one field this + validator skipped, so a playbook with an empty command reached + `/usr/bin/env` with no executable. Checked here so `config check` + rejects it and `switch` refuses to run it. */ + for (playbook, commands) in (config.lifecycle ?? [:]).sorted(by: { $0.key < $1.key }) { + for (index, argv) in commands.enumerated() { + if argv.isEmpty { + view.errors.append( + "lifecycle '\(playbook)' command \(index + 1) is empty") + } else if argv[0].isEmpty { + view.errors.append( + "lifecycle '\(playbook)' command \(index + 1) has an empty executable") + } + } + } view.specs = specs view.warnings = warnings return view diff --git a/Sources/DevCtlKit/Logs/LogQuery.swift b/Sources/DevCtlKit/Logs/LogQuery.swift index 52ac6f1..446c8fa 100644 --- a/Sources/DevCtlKit/Logs/LogQuery.swift +++ b/Sources/DevCtlKit/Logs/LogQuery.swift @@ -41,17 +41,92 @@ public enum LogQuery { return files } - /** Why a caller-supplied grep pattern will not compile, or nil when it will. - Callers validate before querying: a pattern the engine cannot compile must - not silently degrade into "no filter", because returning every line reads - exactly like a query that matched everything. */ + /** Why a caller-supplied grep pattern must be refused, or nil when it is safe + to run. Callers validate before querying: a pattern the engine cannot + compile must not silently degrade into "no filter", because returning + every line reads exactly like a query that matched everything. A pattern + that compiles but nests an unbounded quantifier inside another is refused + too: Swift's `Regex` backtracks, so `^(a+)+$` against a handful of + characters runs for seconds and against a longer line never returns, + wedging the log actor while it churns. The match runs per line, so this + screen is the only place to stop it before it starts. */ public static func grepRejection(_ pattern: String) -> String? { do { _ = try Regex(pattern) - return nil } catch { return String(describing: error) } + if nestsUnboundedQuantifier(pattern) { + return + "'\(pattern)' repeats a group that itself repeats without bound (like (a+)+), which can make the log reader run for minutes on a single line; rewrite it without the nested repeat" + } + return nil + } + + /** True when an unbounded quantifier (`*`, `+`, `{n,}`) is applied to a group + whose body already contains an unbounded quantifier: the exponential + backtracking family. A lexical scan rather than a full parser, tuned to + reject that shape while leaving common safe patterns alone: a bounded + outer repeat (`(a+){2}`), disjoint alternation (`(foo|bar)+`), a class + (`[a-z]+`), and any top-level quantifier (`error.*failed`) are all fine + because none nests an unbounded repeat inside a repeated group. */ + static func nestsUnboundedQuantifier(_ pattern: String) -> Bool { + let chars = Array(pattern) + /** One flag per open group: does its body hold an unbounded quantifier. */ + var groupHasUnbounded: [Bool] = [] + var inClass = false + var index = 0 + func unboundedBraceLength(at start: Int) -> Int? { + /** `{n,}` is unbounded; `{n}` and `{n,m}` are not. Returns the token + length when unbounded so the caller can also treat it as applying + to whatever precedes it. */ + guard start < chars.count, chars[start] == "{" else { return nil } + var cursor = start + 1 + var digits = 0 + while cursor < chars.count, chars[cursor].isNumber { cursor += 1; digits += 1 } + guard digits > 0, cursor < chars.count, chars[cursor] == "," else { return nil } + cursor += 1 + guard cursor < chars.count, chars[cursor] == "}" else { return nil } + return cursor - start + 1 + } + while index < chars.count { + let char = chars[index] + if char == "\\" { index += 2; continue } + if inClass { + if char == "]" { inClass = false } + index += 1 + continue + } + switch char { + case "[": + inClass = true + case "(": + groupHasUnbounded.append(false) + case ")": + let innerUnbounded = groupHasUnbounded.popLast() ?? false + let next = index + 1 < chars.count ? chars[index + 1] : nil + let appliedUnbounded = + next == "*" || next == "+" || unboundedBraceLength(at: index + 1) != nil + if appliedUnbounded && innerUnbounded { return true } + /** A quantified group is itself an unbounded repeat inside its + parent, so propagate upward. */ + if appliedUnbounded, !groupHasUnbounded.isEmpty { + groupHasUnbounded[groupHasUnbounded.count - 1] = true + } + case "*", "+": + if !groupHasUnbounded.isEmpty { + groupHasUnbounded[groupHasUnbounded.count - 1] = true + } + case "{": + if unboundedBraceLength(at: index) != nil, !groupHasUnbounded.isEmpty { + groupHasUnbounded[groupHasUnbounded.count - 1] = true + } + default: + break + } + index += 1 + } + return false } /** Counts records on the given streams and brackets them in time, without diff --git a/Sources/DevCtlKit/Model/Models.swift b/Sources/DevCtlKit/Model/Models.swift index 4cfd0c6..fa9765e 100644 --- a/Sources/DevCtlKit/Model/Models.swift +++ b/Sources/DevCtlKit/Model/Models.swift @@ -247,6 +247,34 @@ public struct ServerSpec: Codable, Equatable, Sendable { self.waitFor = waitFor self.watch = watch } + + /** Per-spec config-check messages for a spec entering the daemon directly + through `server.register`, where the project-file validator never runs. + Mirrors the per-server checks `ProjectConfigLoader.validate` applies to a + committed entry, so one of the two ways a spec reaches the daemon can no + longer accept a spec the other would refuse. Cross-spec checks (unknown + dependency, cycles) are the file validator's job and are not decidable + from a single spec. */ + public func validationErrors() -> [String] { + var errors: [String] = [] + if name.isEmpty { + errors.append("server name is empty") + } + /** `::` is the separator devctl uses to build a persisted server key + from a project path and a server name, so a name carrying it splits + back into the wrong project and server. */ + if name.contains("::") { + errors.append("server '\(name)': name must not contain '::'") + } + if command.isEmpty { + errors.append("server '\(name)': command is empty") + } + for error in healthcheck?.validationErrors() ?? [] { + errors.append("server '\(name)': \(error)") + } + errors.append(contentsOf: PortClaim.configErrors(spec: self)) + return errors + } } /** Exit forensics for a server that ran and then died. */ diff --git a/Tests/DevCtlKitTests/LogTests.swift b/Tests/DevCtlKitTests/LogTests.swift index 71b4174..27b3f7d 100644 --- a/Tests/DevCtlKitTests/LogTests.swift +++ b/Tests/DevCtlKitTests/LogTests.swift @@ -105,6 +105,30 @@ import Testing #expect(LogQuery.run(current: current, options: LogQueryOptions(grep: "(unbalanced")).isEmpty) } + @Test func catastrophicBacktrackingPatternsAreRejected() throws { + /** Swift's Regex backtracks, so a group that repeats a group that itself + repeats runs for seconds on a short line and never returns on a long + one, wedging the log actor. These compile, so only the ReDoS screen + stops them. Each is refused before it ever runs. */ + for pattern in ["^(a+)+$", "(a*)*", "(.*)+", "(a+)*$", "(\\d+)+", "(ab+)+"] { + #expect(LogQuery.grepRejection(pattern) != nil, "expected \(pattern) rejected") + #expect(LogQuery.nestsUnboundedQuantifier(pattern), "expected \(pattern) flagged") + } + } + + @Test func safePatternsAreNotRejectedByTheReDoSScreen() throws { + /** Common log-grep shapes carry no nested unbounded repeat and must keep + working: a top-level quantifier, disjoint alternation, a character + class, a bounded outer repeat, and a plain literal. */ + for pattern in [ + "error.*failed", "(foo|bar)+", "[a-z]+", "(a+){2}", "(a+)?", "\\bwarn\\b", + "GET /api/\\d+", "timeout|refused", + ] { + #expect(!LogQuery.nestsUnboundedQuantifier(pattern), "expected \(pattern) allowed") + #expect(LogQuery.grepRejection(pattern) == nil, "expected \(pattern) accepted") + } + } + @Test func summarizeCountsAndBracketsErrorStream() throws { let lines = [ record(1, .out, "listening"), diff --git a/Tests/DevCtlKitTests/ProjectConfigTests.swift b/Tests/DevCtlKitTests/ProjectConfigTests.swift index f690a8a..be0eac7 100644 --- a/Tests/DevCtlKitTests/ProjectConfigTests.swift +++ b/Tests/DevCtlKitTests/ProjectConfigTests.swift @@ -62,6 +62,37 @@ import Testing #expect(view.warnings.contains { $0.contains("both declare port 3000") }) } + @Test func validationCatchesBadLifecycleCommands() { + /** `devctl switch` runs lifecycle argv locally, so a config the validator + would reject must not slip an empty command through to `/usr/bin/env`. */ + let config = ProjectFileConfig( + lifecycle: ["switch": [["pnpm", "install"], [], [""]]], + servers: ["a": ProjectFileServer(command: ["x"])]) + let view = ProjectConfigLoader.validate(config: config, project: "/p") + #expect(view.errors.contains { $0.contains("lifecycle 'switch' command 2 is empty") }) + #expect( + view.errors.contains { $0.contains("lifecycle 'switch' command 3 has an empty executable") }) + let ok = ProjectFileConfig( + lifecycle: ["switch": [["pnpm", "install"]]], + servers: ["a": ProjectFileServer(command: ["x"])]) + #expect(ProjectConfigLoader.validate(config: ok, project: "/p").errors.isEmpty) + } + + @Test func serverSpecValidationMirrorsFileChecks() { + /** The register path validates through ServerSpec.validationErrors; it must + catch the same per-spec problems the file validator does, plus a name + carrying the persisted-key separator. */ + let bad = ServerSpec( + command: [], + healthcheck: HealthCheckSpec(port: 70000, type: .tcp), + name: "a::b") + let errors = bad.validationErrors() + #expect(errors.contains { $0.contains("must not contain '::'") }) + #expect(errors.contains { $0.contains("command is empty") }) + #expect(errors.contains { $0.contains("healthcheck.port") }) + #expect(ServerSpec(command: ["x"], name: "web").validationErrors().isEmpty) + } + @Test func bareLoopbackHostsWarnButDoNotFail() { let config = ProjectFileConfig( host: "localhost", From 1e37b2439bd375d4df4dccb61fdeb726c1cf3c9d Mon Sep 17 00:00:00 2001 From: Evan Jacobs Date: Sun, 9 Aug 2026 15:26:05 -0400 Subject: [PATCH 2/3] feat(daemon): enforce the trust gate and validate every config entry point prepareSpawn is now the one home for the trust boundary. Its userInitiated flag records trust for an explicit command acting on a committed server, and refuses an autonomous boot restore or watch sweep of a project whose config was never approved, so a cloned repo's devservers.json cannot start itself after a reboot. The scattered recordTrustIfNeeded calls collapse into it. register validates the spec before storing it, closing the one entry point that skipped the file validator. writeConfig refuses to write for a project devctl does not already track, so a wire client cannot drop a devservers.json at an arbitrary path. The remaining project-scoped arms (writeConfig, logs query, logs mark, events, why) canonicalize the path at the decode seam. --- .../Control/ControlServer.swift | 131 ++++++++++------ .../RecoverAtStartupTests.swift | 36 +++++ .../TrustAndInputValidationTests.swift | 142 ++++++++++++++++++ 3 files changed, 267 insertions(+), 42 deletions(-) create mode 100644 Tests/DevCtlDaemonCoreTests/TrustAndInputValidationTests.swift diff --git a/Sources/DevCtlDaemonCore/Control/ControlServer.swift b/Sources/DevCtlDaemonCore/Control/ControlServer.swift index 7d45a82..14049f7 100644 --- a/Sources/DevCtlDaemonCore/Control/ControlServer.swift +++ b/Sources/DevCtlDaemonCore/Control/ControlServer.swift @@ -119,6 +119,17 @@ public actor Router { case .serverRegister: let request = try decoder.decode(WireRequest.self, from: line) let project = canonicalProjectPath(request.params.project) + /** register is the second way a spec enters the daemon, and until + now the only unchecked one: the committed-file path runs the + validator, so a spec `config check` would reject could still be + registered directly and then spawned. Refuse it at the seam. */ + let specErrors = request.params.spec.validationErrors() + guard specErrors.isEmpty else { + throw WireError( + code: .configInvalid, + hint: "run: devctl config check", + message: specErrors.joined(separator: "; ")) + } try await registry.register(project: project, spec: request.params.spec) let supervisor = await supervisor(project: project, spec: request.params.spec) await events.post( @@ -131,11 +142,12 @@ public actor Router { name: request.params.name, port: request.params.port, project: project) let merged = try await mergedSpecs(project: project) let supervisor = try await resolvedSupervisor(target) - await recordTrustIfNeeded(project: project, name: target.name, fileNames: merged.fileNames) if let spec = merged.specs.first(where: { $0.name == target.name }) { try await lockGate(project: project, spec: spec) } - try await prepareSpawn(target: target, supervisor: supervisor, portOverride: request.params.port) + try await prepareSpawn( + target: target, supervisor: supervisor, portOverride: request.params.port, + userInitiated: true) let result = await supervisor.ensure(timeoutSeconds: request.params.timeoutSeconds) DevCtlLog.daemon.info( "ensure \(target.name)@\(project) -> \(result.server.phase.rawValue)") @@ -147,12 +159,12 @@ public actor Router { name: request.params.name, port: request.params.port, project: project) let merged = try await mergedSpecs(project: project) let supervisor = try await resolvedSupervisor(target) - await recordTrustIfNeeded( - project: project, name: target.name, fileNames: merged.fileNames) if let spec = merged.specs.first(where: { $0.name == target.name }) { try await lockGate(project: project, spec: spec) } - try await prepareSpawn(target: target, supervisor: supervisor, portOverride: request.params.port) + try await prepareSpawn( + target: target, supervisor: supervisor, portOverride: request.params.port, + userInitiated: true) return try respond(id: head.id, result: ServerResult(server: await supervisor.start())) case .serverStatus: let request = try decoder.decode(WireRequest.self, from: line) @@ -205,7 +217,23 @@ public actor Router { return try respond(id: head.id, result: try await initConfig(request.params)) case .projectWriteConfig: let request = try decoder.decode(WireRequest.self, from: line) - let url = ProjectConfigLoader.configURL(project: request.params.project) + let project = canonicalProjectPath(request.params.project) + let url = ProjectConfigLoader.configURL(project: project) + /** writeConfig edits a project's committed config in place, so it + may only target a project devctl already tracks or one whose + devservers.json already exists. Without this a wire client + could hand any path and AtomicFile.write, which creates + intermediate directories, would drop a devservers.json + anywhere on disk. Creating a config for a brand-new project is + `config init`, not this method. */ + let known = await registry.project(project) != nil + let configExists = FileManager.default.fileExists(atPath: url.path) + guard known || configExists else { + throw WireError( + code: .notFound, + hint: "run: devctl config init in the project, or register a server there first", + message: "refusing to write devservers.json for a project devctl does not track: \(project)") + } let currentHash = (try? Data(contentsOf: url)).map { DevCtlPaths.hash8(String(decoding: $0, as: UTF8.self)) } ?? "" @@ -222,7 +250,7 @@ public actor Router { } catch { throw ProjectConfigLoader.configError(from: error, at: url) } - let view = ProjectConfigLoader.validate(config: parsed, project: request.params.project) + let view = ProjectConfigLoader.validate(config: parsed, project: project) guard view.errors.isEmpty else { throw WireError( code: .configInvalid, @@ -230,7 +258,7 @@ public actor Router { message: view.errors.joined(separator: "; ")) } try AtomicFile.write(Data(request.params.content.utf8), to: url) - configCache[request.params.project] = nil + configCache[project] = nil return try respond( id: head.id, result: CheckResult( @@ -258,7 +286,9 @@ public actor Router { let request = try decoder.decode(WireRequest.self, from: line) var params = request.params params.project = canonicalProjectPath(params.project) - return try respond(id: head.id, result: try await restartServers(params)) + return try respond( + id: head.id, + result: try await restartServers(params, userInitiated: true)) case .serverWait: let request = try decoder.decode(WireRequest.self, from: line) let target = ServerTargetParams( @@ -289,7 +319,9 @@ public actor Router { return try respond(id: head.id, result: result) case .logsQuery: let request = try decoder.decode(WireRequest.self, from: line) - let target = ServerTargetParams(name: request.params.name, project: request.params.project) + let target = ServerTargetParams( + name: request.params.name, + project: canonicalProjectPath(request.params.project)) let supervisor = try await resolvedSupervisor(target) var since = request.params.since if let markID = request.params.sinceMark { @@ -316,15 +348,16 @@ public actor Router { return try respond(id: head.id, result: LogsQueryResult(lines: lines)) case .logsMark: let request = try decoder.decode(WireRequest.self, from: line) + let project = canonicalProjectPath(request.params.project) let label = request.params.label ?? "cli" var marks: [PlacedMark] = [] if request.params.all == true { - for spec in await registry.specs(project: request.params.project) { - let supervisor = await supervisor(project: request.params.project, spec: spec) + for spec in await registry.specs(project: project) { + let supervisor = await supervisor(project: project, spec: spec) marks.append(await supervisor.placeMark(label: label, text: request.params.text)) } } else if let name = request.params.name { - let target = ServerTargetParams(name: name, project: request.params.project) + let target = ServerTargetParams(name: name, project: project) let supervisor = try await resolvedSupervisor(target) marks.append(await supervisor.placeMark(label: label, text: request.params.text)) } else { @@ -333,8 +366,11 @@ public actor Router { return try respond(id: head.id, result: MarkResult(marks: marks)) case .eventsQuery: let request = try decoder.decode(WireRequest.self, from: line) + /** Empty/nil project means machine-wide; only a real project path + is canonicalized, matching how EventStore.query keys the feed. */ + let project = request.params.project.map(canonicalProjectPath) var since = request.params.since - if let markID = request.params.sinceMark, let project = request.params.project { + if let markID = request.params.sinceMark, let project { for spec in await registry.specs(project: project) { let supervisor = await supervisor(project: project, spec: spec) if let markDate = await supervisor.resolveMark(markID) { @@ -349,20 +385,21 @@ public actor Router { } } let events = await events.query( - project: request.params.project, since: since, tail: request.params.tail) + project: project, since: since, tail: request.params.tail) return try respond(id: head.id, result: EventsQueryResult(events: events)) case .serverWhy: let request = try decoder.decode(WireRequest.self, from: line) - _ = try await resolvedSupervisor(request.params) - let merged = try await mergedSpecs(project: request.params.project) + let project = canonicalProjectPath(request.params.project) + let target = ServerTargetParams(name: request.params.name, project: project) + _ = try await resolvedSupervisor(target) + let merged = try await mergedSpecs(project: project) var statuses: [String: ServerStatus] = [:] var specsByName: [String: ServerSpec] = [:] for spec in merged.specs { statuses[spec.name] = await annotatedStatus( - project: request.params.project, spec: spec) + project: project, spec: spec) specsByName[spec.name] = spec } - let project = request.params.project let paths = self.paths let result = WhyEngine.diagnose( target: request.params.name, @@ -819,9 +856,17 @@ public actor Router { which is what lets `restart` raise every refusal before it stops anything. The port pre-check treats a listener the target itself owns as free, so a running server does not report its own port as held. */ + /** Every start-shaped path funnels through here, which is why the trust gate + lives here rather than at each call site. `userInitiated` is the security + boundary: an explicit command (ensure, start, up, restart) acting on a + server declared in the committed devservers.json IS the user's approval, + so it records trust and proceeds. An autonomous path (boot restore, the + watch sweep) must not act on a project's committed config until that + approval was given, so it refuses. A spec that came from `register` + rather than the file carries its own approval and is never gated. */ private func prepareSpawn( target: ServerTargetParams, supervisor: ServerSupervisor, portOverride: Int? = nil, - force: Bool = false + force: Bool = false, userInitiated: Bool = false ) async throws { if !force { let current = await supervisor.status() @@ -839,6 +884,18 @@ public actor Router { hint: "run: devctl status --json", message: "no server named '\(target.name)' in \(target.project)") } + if merged.fileNames.contains(target.name) { + let trusted = await registry.isTrusted(project: target.project) + if userInitiated { + if !trusted { try? await registry.setTrusted(project: target.project) } + } else if !trusted { + throw WireError( + code: .notTrusted, + hint: "run: devctl ensure \(target.name) --project \(target.project)", + message: + "refusing to start '\(target.name)' from \(target.project)/devservers.json: this project's committed config has not been approved. Start a server there once by hand to approve it.") + } + } let overlay = LocalOverlay.load(project: target.project) let overlayServer = overlay?.servers?[target.name] spec = LocalOverlay.apply(spec: spec, overlay: overlayServer, project: target.project) @@ -1135,14 +1192,6 @@ public actor Router { return view } - /** Acting on a committed config is what records trust: an explicit start or - ensure IS the approval. The hook advertises only already-trusted projects. */ - private func recordTrustIfNeeded(project: String, name: String, fileNames: Set) async { - if fileNames.contains(name), await !registry.isTrusted(project: project) { - try? await registry.setTrusted(project: project) - } - } - /** Acquire: refuse if another live holder owns it, pause active declarers without retiring boot intent, persist the hold, return who was paused. */ private func acquireLock(_ params: LockParams) async throws -> LockResult { @@ -1437,9 +1486,9 @@ public actor Router { a held resource or a broken config, leaving it down. The stop is non-retiring because the server is coming straight back, so resume-on-boot survives what `stop` would otherwise clear. */ - private func restartServers(_ params: RestartParams, rearm: Bool = true) async throws - -> GroupResult - { + private func restartServers( + _ params: RestartParams, rearm: Bool = true, userInitiated: Bool = false + ) async throws -> GroupResult { let merged = try await mergedSpecs(project: params.project) var wanted = merged.specs if let names = params.names { @@ -1453,8 +1502,6 @@ public actor Router { } var prepared: [(spec: ServerSpec, supervisor: ServerSupervisor)] = [] for spec in wanted { - await recordTrustIfNeeded( - project: params.project, name: spec.name, fileNames: merged.fileNames) try await lockGate(project: params.project, spec: spec) try await refuseIfPaused(project: params.project, spec: spec) prepared.append((spec: spec, supervisor: await supervisor(project: params.project, spec: spec))) @@ -1471,7 +1518,8 @@ public actor Router { try await prepareSpawn( target: ServerTargetParams( name: entry.spec.name, port: params.port, project: params.project), - supervisor: entry.supervisor, portOverride: params.port, force: true) + supervisor: entry.supervisor, portOverride: params.port, force: true, + userInitiated: userInitiated) } var results: [EnsureResult] = [] for entry in prepared { @@ -1498,9 +1546,11 @@ public actor Router { continue } guard let split = Self.splitServerID(id) else { continue } - /** The daemon never acts on a project's config before trust is - recorded. A running server implies trust was recorded, so this is - belt and braces rather than the only guard. */ + /** The daemon never acts on a project's committed config before trust + is recorded. `restartServers` runs autonomously here (userInitiated + defaults to false), so `prepareSpawn` enforces the same gate; this + skips the work early and cleanly for an untrusted project rather + than letting the restart raise and defer. */ guard await registry.isTrusted(project: split.project) else { continue } let relative = changed.map { $0.replacingOccurrences(of: split.project + "/", with: "") @@ -1576,10 +1626,6 @@ public actor Router { } wanted = wanted.filter { keep.contains($0.name) } } - for spec in wanted { - await recordTrustIfNeeded( - project: params.project, name: spec.name, fileNames: merged.fileNames) - } /** Port ownership is checked for the whole set before anything spawns, so a held port refuses the rollout instead of leaving half a project up next to a server that lost a race it never knew it entered. Servers @@ -1597,7 +1643,8 @@ public actor Router { name: spec.name, port: params.port, project: params.project) let supervisor = await supervisor(project: params.project, spec: spec) try await prepareSpawn( - target: target, supervisor: supervisor, portOverride: params.port) + target: target, supervisor: supervisor, portOverride: params.port, + userInitiated: true) prepared[spec.name] = supervisor } guard case .success(let waves) = DependencyGraph.waves(specs: wanted) else { diff --git a/Tests/DevCtlDaemonCoreTests/RecoverAtStartupTests.swift b/Tests/DevCtlDaemonCoreTests/RecoverAtStartupTests.swift index 4a0c286..7cff75c 100644 --- a/Tests/DevCtlDaemonCoreTests/RecoverAtStartupTests.swift +++ b/Tests/DevCtlDaemonCoreTests/RecoverAtStartupTests.swift @@ -84,6 +84,42 @@ private func stopServer(router: Router, project: String, name: String) async { await stopServer(router: router, project: env.projectPath, name: "web") } + /** The trust gate. A malicious repo's devservers.json must never be + auto-started: boot restore resolves the committed spec but `prepareSpawn` + refuses it while the project's config has never been approved (no trust + flag), so the server stays down. The mirror of + `restoresConfigDefinedServerWithResumeIntent`, which sets trust and does + spawn; the only difference here is the missing approval. */ + @Test func untrustedConfigDefinedServerIsNotRestored() async throws { + let env = try makeRecoverEnv() + try writeDevservers( + project: env.projectPath, + serversJSON: """ + { + "web": { + "command": ["/bin/sh", "-c", "sleep 30"] + } + } + """) + let registry = Registry(paths: env.paths) + /** Deliberately not trusted: no start-shaped command ever approved it. */ + let id = serverID(project: env.projectPath, name: "web") + try await registry.updateState(serverID: id) { entry in + entry.phase = .stopped + entry.resumeOnBoot = true + entry.pid = nil + } + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + await router.recoverAtStartup() + let web = try await statusList(router: router, project: env.projectPath) + .first { $0.server == "web" } + #expect(web?.pid == nil) + #expect(web?.phase != .running) + #expect(web?.phase != .starting) + /** Recover is autonomous, so it must not silently grant trust either. */ + #expect(await registry.isTrusted(project: env.projectPath) == false) + } + /** A rename leaves resume intent under the old name. Recover must drop the orphan row, not resurrect a ghost. */ @Test func dropsOrphanStateWhenSpecMissing() async throws { diff --git a/Tests/DevCtlDaemonCoreTests/TrustAndInputValidationTests.swift b/Tests/DevCtlDaemonCoreTests/TrustAndInputValidationTests.swift new file mode 100644 index 0000000..d4785df --- /dev/null +++ b/Tests/DevCtlDaemonCoreTests/TrustAndInputValidationTests.swift @@ -0,0 +1,142 @@ +import DevCtlKit +import Foundation +import Testing + +@testable import DevCtlDaemonCore + +/** The daemon's input-validation and trust boundary at the wire: a spec entering + through `register` is validated the same as one from the file, `writeConfig` + cannot drop a devservers.json at a path devctl does not track, and an explicit + start records the trust that boot restore later requires. */ +@Suite(.serialized) struct TrustAndInputValidationTests { + private struct Env { + let paths: DevCtlPaths + let project: String + } + + private func makeEnv() throws -> Env { + let base = FileManager.default.temporaryDirectory + .appending(path: "devctl-trust-\(UUID().uuidString)") + let project = base.appending(path: "proj") + try FileManager.default.createDirectory(at: project, withIntermediateDirectories: true) + return Env( + paths: DevCtlPaths( + dataDir: base.appending(path: "data"), logsDir: base.appending(path: "logs")), + project: project.path) + } + + /** Returns the decoded result, or the WireError when the daemon refused. */ + private func send( + _ router: Router, _ method: WireMethod, _ params: P, _ expecting: R.Type + ) async throws -> Result { + let line = try NDJSON.encodeLine( + WireRequest(id: "t", method: method.rawValue, params: params)) + let data = await router.handle(line: line) + let response = try JSONCoding.decoder().decode(WireResponse.self, from: data) + if response.ok, let result = response.result { return .success(result) } + return .failure(response.error ?? WireError(code: .internalError, message: "no result")) + } + + @Test func registerRefusesAnInvalidSpec() async throws { + let env = try makeEnv() + let registry = Registry(paths: env.paths) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + let outcome = try await send( + router, .serverRegister, + RegisterParams( + project: env.project, + spec: ServerSpec(command: [], name: "web", port: 70000)), + ServerResult.self) + guard case .failure(let error) = outcome else { + Issue.record("register accepted an invalid spec") + return + } + #expect(error.code == .configInvalid) + #expect(await registry.spec(project: env.project, name: "web") == nil) + } + + @Test func registerAcceptsAValidSpec() async throws { + let env = try makeEnv() + let registry = Registry(paths: env.paths) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + let outcome = try await send( + router, .serverRegister, + RegisterParams( + project: env.project, + spec: ServerSpec(command: ["bun", "dev"], name: "web", port: 3000)), + ServerResult.self) + #expect((try? outcome.get()) != nil) + #expect(await registry.spec(project: env.project, name: "web") != nil) + } + + @Test func writeConfigRefusesAnUntrackedProjectPath() async throws { + let env = try makeEnv() + let stranger = FileManager.default.temporaryDirectory + .appending(path: "devctl-stranger-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: stranger, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: stranger) } + let registry = Registry(paths: env.paths) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + let body = """ + {"servers":{"web":{"command":["bun","dev"]}},"version":1} + """ + let outcome = try await send( + router, .projectWriteConfig, + WriteConfigParams(baselineHash: "", content: body, project: stranger.path), + CheckResult.self) + guard case .failure(let error) = outcome else { + Issue.record("writeConfig created a config for an untracked path") + return + } + #expect(error.code == .notFound) + #expect( + !FileManager.default.fileExists( + atPath: stranger.appending(path: "devservers.json").path)) + } + + @Test func writeConfigCreatesForAKnownProject() async throws { + let env = try makeEnv() + let registry = Registry(paths: env.paths) + /** A registered server makes the project known, so the editor's + create-on-first-save flow is allowed. */ + try await registry.register( + project: env.project, spec: ServerSpec(command: ["bun", "dev"], name: "web")) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + let body = """ + {"servers":{"web":{"command":["bun","dev"]}},"version":1} + """ + let outcome = try await send( + router, .projectWriteConfig, + WriteConfigParams(baselineHash: "", content: body, project: env.project), + CheckResult.self) + #expect((try? outcome.get()) != nil) + #expect( + FileManager.default.fileExists( + atPath: URL(fileURLWithPath: env.project).appending(path: "devservers.json").path)) + } + + @Test func anExplicitStartRecordsTrust() async throws { + let env = try makeEnv() + let body = """ + {"servers":{"web":{"command":["/bin/sh","-c","sleep 30"]}},"version":1} + """ + try Data(body.utf8).write( + to: URL(fileURLWithPath: env.project).appending(path: "devservers.json")) + let registry = Registry(paths: env.paths) + #expect(await registry.isTrusted(project: env.project) == false) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + let outcome = try await send( + router, .serverStart, + ServerTargetParams(name: "web", project: env.project), + ServerResult.self) + #expect((try? outcome.get()) != nil) + /** The explicit start IS the approval: trust is now recorded, which is + what lets boot restore bring this server back next time. */ + #expect(await registry.isTrusted(project: env.project) == true) + let stop = try NDJSON.encodeLine( + WireRequest( + id: "s", method: WireMethod.serverStop.rawValue, + params: ServerTargetParams(name: "web", project: env.project))) + _ = await router.handle(line: stop) + } +} From 913495e0b05b2beacb42ec6bdb692f4e1a4d1dd8 Mon Sep 17 00:00:00 2001 From: Evan Jacobs Date: Sun, 9 Aug 2026 15:26:33 -0400 Subject: [PATCH 3/3] feat(cli): validate before switch lifecycle, size client waits, document devctl switch validates the branch's devservers.json after the checkout and before running its lifecycle argv, so a config the daemon would refuse is no longer fed to the shell. ensure, wait, up, down, restart, and switch pass their own timeout to the client so a long but healthy run is given room while a wedged daemon still fails in bounded time. Documents the register/logs/switch/restore behavior in the CLI contract, maps the trust gate and the client deadline in AGENTS, and adds the changeset. --- .../harden-trust-and-input-validation.md | 15 +++++ AGENTS.md | 7 ++- Sources/devctl/CLI.swift | 58 +++++++++++++++---- docs/cli-contract.md | 8 +-- 4 files changed, 70 insertions(+), 18 deletions(-) create mode 100644 .changeset/harden-trust-and-input-validation.md diff --git a/.changeset/harden-trust-and-input-validation.md b/.changeset/harden-trust-and-input-validation.md new file mode 100644 index 0000000..fb22391 --- /dev/null +++ b/.changeset/harden-trust-and-input-validation.md @@ -0,0 +1,15 @@ +--- +"devctl": patch +--- + +A cloned repo's devservers.json can no longer start itself. devctl's rule is that it never acts on a project's committed config until you approve it by running a server there once, but boot restore skipped that check: after a reboot it would bring back a committed server for a project that was never approved. It now honors the same gate every other path does, so an unapproved project's config stays inert until you start it by hand. An explicit `start`, `ensure`, or `up` still records that approval, exactly as before. + +`devctl register` now screens a server the same way the config file is screened. Registering a server directly was the one way into the daemon that skipped validation, so a spec `devctl config check` would reject (an out-of-range port, an empty command, a name containing the reserved `::`) could still be registered and then started. It is refused up front now. + +`devctl switch` validates the branch's devservers.json before running that branch's lifecycle commands. A config the daemon would refuse to load no longer has its commands handed to the shell anyway, and an empty lifecycle command is now caught by `config check`. + +A bad `--grep` pattern can no longer hang the daemon. A regular expression that nests one unbounded repeat inside another (the classic `(a+)+`) makes the engine run for minutes on a single log line; `devctl logs --grep` now rejects that shape before it runs, with a message that names the fix. + +devctl no longer hangs waiting on a stuck daemon. A wedged daemon used to leave `devctl` and the menu bar app blocked with no output and no way out; requests now fail in bounded time and point you at `devctl daemon restart`, while a legitimately long `ensure`, `wait`, or group rollout is given the room it needs. + +Editing a project's config through the menu bar can only write to a project devctl already tracks, closing a path where a crafted request could have dropped a devservers.json anywhere on disk. diff --git a/AGENTS.md b/AGENTS.md index f09b912..4c2c5f0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,8 +9,8 @@ Identity and stack - Three products, one daemon: devctld owns all server processes; devctl (CLI) and devctl.app (SwiftUI MenuBarExtra) are thin clients over a unix socket (default ~/Library/Application Support/devctl/daemon.sock; DEVCTL_SOCKET overrides; /tmp fallback near the sun_path limit), NDJSON protocol. devctl daemon install/uninstall/start/stop/restart manage the LaunchAgent (dev.quantizor.devctl); tests and the smoke gate run devctld --foreground. Codebase map -- Sources/DevCtlKit: shared core, the unit-test target of record. Models.swift (specs, phases, ServerStatus, whose displayPort is the one home for which of the three port fields a human is shown), Wire.swift (JSONCoding, typed request/response/event frames, NDJSON framing, stable error codes), Client/DaemonClient.swift (blocking-POSIX socket actor used unchanged by CLI and app), Paths/Paths.swift (path constants, canonical project path, atomic write + defensive load with per-call unique temp names so two writers in one process cannot rename each other's temp away, SHA-256 over CryptoKit with a chunked file-digest entry point so hashing a file costs a chunk of memory rather than the file; agent.path holds login-shell PATH for the daemon), Setup/ (SetupPlanner: first-run / upgrade decisions, harness offers, stage-and-rename binary install, and CLIOwner: whether devctl or Homebrew owns the CLI, decided by realpath-matching the running bundle against the Caskroom backlink rather than any `/Caskroom/` substring, which drives skipping the binary install and the PATH warning under brew; AppInstancePolicy decides which of two copies of one bundle quits at launch, scoped to the bundle path so the DMG-to-Applications handoff, the one case where two copies are correct, is left alone), Agent/ (AgentContext: the pure session-context renderer the hook injects, bad-state servers first with a devctl why recommendation and devctl's own stderr count, never raw child output; DiscoveryStanza), Net/LoopbackProbe.swift (dual-stack loopback listen probe shared by the daemon port pre-check and CLI doctor), Net/PortClaim.swift + PortMaterializer.swift (effectivePort claim: portSpan and named ports; env injection and URL rewrite, including resolving a root-relative head or healthcheck url against the server's own base), Config/ (ProjectConfig loader and validator; ConfigProjection projects merged specs back down to devservers.json, dropping everything the machine derived, for `config init`; EffectiveHost is the one home for the host a spawn will use, read by both prepareSpawn and config check; LocalOverlay; LockResource reads locks declarations and resolves a resource's state path; WatchPolicy is the pure settle/quiet/burst decision behind auto-restart and WatchPaths resolves the entries config check warns about), Resource/ResourceIdentity.swift (bounded fingerprint of a lock resource's state so `lock` can report a change made under a live holder), Launchd/ (LaunchdAdmin: dual install path; SMAppService via app deep link when /Applications/devctl.app exists, else legacy home LaunchAgent + Application Support bin/devctld; --legacy forces the home path; DaemonRecoveryPolicy decides whether an unreachable daemon is auto-restarted; AgentRebindPolicy + agent.rebind settle the ad-hoc CDHash window on DMG replace), DeepLink/ (parse/serialize + DeepLinkRunner + notification action map), Update/ (UpdateCheck: GitHub releases/latest against DevCtlVersion, one on-disk cache with an ETag shared by the app poll and `devctl doctor`, every failure silent, and never fed into AgentContext.render; DevCtlDistribution: the one home for the tap token, releases URL, and brew upgrade/uninstall commands), Log/DevCtlLog.swift (OSLog facade with a recording backend for tests). -- Sources/DevCtlDaemonCore: daemon logic as a library. Supervisor/ (ServerSupervisor actor per server: spawn, spool capture, health-gated phase machine, ensure/wait, group + descendant teardown with ProcessIdentity start-time revalidation; the ProcessLauncher seam; ProcessTree QA1123 sysctl sweep, plus narrowed/isAlive, the one home for turning a pid read off disk or the wire into one the kernel calls take, since a trapping conversion there is a crash loop under KeepAlive), Health/HealthProber.swift (EffectiveHealthcheck resolution, the HealthProber seam with ephemeral URLSession HTTP probes + BSD TCP, and PortGuard's lsof diagnostics, which live in that same file), Registry/ (owner of registry.json and state.json), Control/ (Router method dispatch + port pre-check + persisted resource locks with daemon-owned pause/resume and dead-holder auto-release + the boot-restore gate that answers daemon.info and refuses everything else with daemon-starting + NWListener ControlServer whose startAccepting awaits the listener's ready state and throws rather than suspending forever). +- Sources/DevCtlKit: shared core, the unit-test target of record. Models.swift (specs, phases, ServerStatus, whose displayPort is the one home for which of the three port fields a human is shown; ServerSpec.validationErrors is the per-spec check the `register` seam runs so a directly-registered spec is screened like a committed one), Wire.swift (JSONCoding, typed request/response/event frames, NDJSON framing, stable error codes), Client/DaemonClient.swift (blocking-POSIX socket actor used unchanged by CLI and app; a SO_RCVTIMEO response deadline, raised for a command carrying its own timeout, so a wedged daemon fails a request instead of hanging the client forever), Paths/Paths.swift (path constants, canonical project path, atomic write + defensive load with per-call unique temp names so two writers in one process cannot rename each other's temp away, SHA-256 over CryptoKit with a chunked file-digest entry point so hashing a file costs a chunk of memory rather than the file; agent.path holds login-shell PATH for the daemon), Setup/ (SetupPlanner: first-run / upgrade decisions, harness offers, stage-and-rename binary install, and CLIOwner: whether devctl or Homebrew owns the CLI, decided by realpath-matching the running bundle against the Caskroom backlink rather than any `/Caskroom/` substring, which drives skipping the binary install and the PATH warning under brew; AppInstancePolicy decides which of two copies of one bundle quits at launch, scoped to the bundle path so the DMG-to-Applications handoff, the one case where two copies are correct, is left alone), Agent/ (AgentContext: the pure session-context renderer the hook injects, bad-state servers first with a devctl why recommendation and devctl's own stderr count, never raw child output; DiscoveryStanza), Net/LoopbackProbe.swift (dual-stack loopback listen probe shared by the daemon port pre-check and CLI doctor), Net/PortClaim.swift + PortMaterializer.swift (effectivePort claim: portSpan and named ports; env injection and URL rewrite, including resolving a root-relative head or healthcheck url against the server's own base), Config/ (ProjectConfig loader and validator; ConfigProjection projects merged specs back down to devservers.json, dropping everything the machine derived, for `config init`; EffectiveHost is the one home for the host a spawn will use, read by both prepareSpawn and config check; LocalOverlay; LockResource reads locks declarations and resolves a resource's state path; WatchPolicy is the pure settle/quiet/burst decision behind auto-restart and WatchPaths resolves the entries config check warns about), Resource/ResourceIdentity.swift (bounded fingerprint of a lock resource's state so `lock` can report a change made under a live holder), Launchd/ (LaunchdAdmin: dual install path; SMAppService via app deep link when /Applications/devctl.app exists, else legacy home LaunchAgent + Application Support bin/devctld; --legacy forces the home path; DaemonRecoveryPolicy decides whether an unreachable daemon is auto-restarted; AgentRebindPolicy + agent.rebind settle the ad-hoc CDHash window on DMG replace), DeepLink/ (parse/serialize + DeepLinkRunner + notification action map), Update/ (UpdateCheck: GitHub releases/latest against DevCtlVersion, one on-disk cache with an ETag shared by the app poll and `devctl doctor`, every failure silent, and never fed into AgentContext.render; DevCtlDistribution: the one home for the tap token, releases URL, and brew upgrade/uninstall commands), Log/DevCtlLog.swift (OSLog facade with a recording backend for tests). +- Sources/DevCtlDaemonCore: daemon logic as a library. Supervisor/ (ServerSupervisor actor per server: spawn, spool capture, health-gated phase machine, ensure/wait, group + descendant teardown with ProcessIdentity start-time revalidation; the ProcessLauncher seam; ProcessTree QA1123 sysctl sweep, plus narrowed/isAlive, the one home for turning a pid read off disk or the wire into one the kernel calls take, since a trapping conversion there is a crash loop under KeepAlive), Health/HealthProber.swift (EffectiveHealthcheck resolution, the HealthProber seam with ephemeral URLSession HTTP probes + BSD TCP, and PortGuard's lsof diagnostics, which live in that same file), Registry/ (owner of registry.json and state.json), Control/ (Router method dispatch + port pre-check + persisted resource locks with daemon-owned pause/resume and dead-holder auto-release + the boot-restore gate that answers daemon.info and refuses everything else with daemon-starting + NWListener ControlServer whose startAccepting awaits the listener's ready state and throws rather than suspending forever). prepareSpawn is the one funnel every start-shaped path takes and the one home for the trust gate: its `userInitiated` flag records trust for an explicit command acting on a committed server and refuses an autonomous restore/sweep of an unapproved project. register validates the spec and writeConfig refuses a project the daemon does not track; every project-scoped arm canonicalizes the path at the decode seam. - Sources/devctld: thin main; identical behavior under launchd and --foreground (tests and the smoke gate use foreground). Applies agent.path into process env before spawn, accepts on the socket before boot restore and marks the router restoring across it so a client can tell a busy daemon from a dead one, and runs the watch sweep on its own timer once restore has finished, so a boot spawn is never read as a config change. - Tests: DevCtlKitTests is the unit-test target of record and holds the schema goldens; DevCtlDaemonCoreTests drives a real Router over temp paths; DevCtlCLITests covers CLI behavior with a contract and no other way to exercise it (argument parsing, the lock notices and identity verdict), importing the executable target with @testable. TestSupport.swift is the one home for the fixture-server lookup and reserves ports 45000 to 45500 for the unit suites; touching it reaps fixtures orphaned by an interrupted run, but only those whose parent is gone and whose port is in that block, so a concurrent test run and smoke.sh (which orphans a fixture on purpose, outside the block) are both left alone. - Sources/devctl: CLI (swift-argument-parser). Two files only: HookSupport.swift (HookContext, the thin socket fetch over DevCtlKit's AgentContext renderer, + HarnessAdapter registry, each adapter with install/uninstall/hookState over a settings file devctl does not own and never edits without being asked; adding a harness: CONTRIBUTING.md) and CLI.swift, which holds every command as a struct, including Switch (branch switching + lifecycle playbooks), Lock (run-under-resource-lock), Doctor (health report; owns the cross-project port-collision and squatter findings, plus report-only harness-hook and update findings), Uninstall (the one uninstall verb: agent, hooks, and CLI, with --agent-only for the cask and --purge for data; `daemon uninstall` is a deprecated alias warning on stderr), HookInstall/HookUninstall, and Link / x-url (deep links). CLI.swift is past the size where splitting is worth asking about. @@ -35,7 +35,8 @@ Hard rules - Wire methods are typed end to end: the daemon sniffs the {id, method} head, then re-decodes the full typed frame. A new method extends WireMethod plus Codable params/result types in Wire.swift; no untyped dictionaries on the wire. - Every CLI command supports --json with a stable schema generated from the shared Codable types; failures emit {ok:false, error:{code,message,hint}} on stdout, hint being the literal remediation command. Error codes grow append-only. Golden tests in Tests/DevCtlKitTests assert exact schema strings; a changed field is an API change: update docs/cli-contract.md in the same commit, then the golden. - Structured log files keep per-file monotonic timestamps (clamp on append); the since-query binary search depends on it. -- The daemon never acts on a project's committed config before trust is recorded; the SessionStart hook never emits raw log lines or command strings into agent context (child output is attacker-influenceable). +- The daemon never acts on a project's committed config before trust is recorded, enforced in prepareSpawn: an explicit command records trust, an autonomous restore or watch sweep refuses an unapproved project. The SessionStart hook never emits raw log lines or command strings into agent context (child output is attacker-influenceable). A spec reaching the daemon through `register` is validated like a committed one, and writeConfig only writes for a project the daemon already tracks. +- A user-supplied regex (`logs --grep`) is screened before it runs: a nested unbounded quantifier is refused, because Swift's backtracking engine turns `(a+)+` into minutes of CPU on a single line and the match runs on the log actor. - State files load defensively: parse failure quarantines to .corrupt- and continues; never fatal (a startup parse crash under launchd KeepAlive loops forever). Corollary: new fields on persisted types (registry, state) stay optional so existing files keep parsing. - Registry/state writes are temp + fsync + rename. - Binary upgrades stage and rename(2); never overwrite a running signed Mach-O. diff --git a/Sources/devctl/CLI.swift b/Sources/devctl/CLI.swift index 54736ea..d9ba370 100644 --- a/Sources/devctl/CLI.swift +++ b/Sources/devctl/CLI.swift @@ -223,7 +223,9 @@ struct Ensure: AsyncParsableCommand { let params = EnsureParams( name: name, port: port, project: global.resolvedProject(), timeoutSeconds: timeout) let result = await CLIRunner.run(json: global.json, bootstrap: !global.noBootstrap) { client in - try await client.request(.serverEnsure, params: params, expecting: EnsureResult.self) + try await client.request( + .serverEnsure, params: params, expecting: EnsureResult.self, + operationTimeoutSeconds: timeout) } CLIRunner.emit(result, json: global.json) { r in var text = CLIRunner.describe(r.server) @@ -259,7 +261,9 @@ struct Wait: AsyncParsableCommand { let params = WaitParams( condition: condition, name: name, project: global.resolvedProject(), timeoutSeconds: timeout) let result = await CLIRunner.run(json: global.json, bootstrap: !global.noBootstrap) { client in - try await client.request(.serverWait, params: params, expecting: EnsureResult.self) + try await client.request( + .serverWait, params: params, expecting: EnsureResult.self, + operationTimeoutSeconds: timeout) } CLIRunner.emit(result, json: global.json) { r in if let reason = r.reason { @@ -428,7 +432,9 @@ struct Restart: AsyncParsableCommand { names: name.map { [$0] }, port: port, project: global.resolvedProject(), timeoutSeconds: timeout) let result = await CLIRunner.run(json: global.json, bootstrap: !global.noBootstrap) { client in - try await client.request(.serverRestart, params: params, expecting: GroupResult.self) + try await client.request( + .serverRestart, params: params, expecting: GroupResult.self, + operationTimeoutSeconds: timeout) } CLIRunner.emit(result, json: global.json) { r in r.results.map { CLIRunner.describe($0.server) }.joined(separator: "\n") @@ -954,7 +960,9 @@ struct Up: AsyncParsableCommand { project: global.resolvedProject(), timeoutSeconds: timeout) let result = await CLIRunner.run(json: global.json, bootstrap: !global.noBootstrap) { client in - try await client.request(.groupUp, params: params, expecting: GroupResult.self) + try await client.request( + .groupUp, params: params, expecting: GroupResult.self, + operationTimeoutSeconds: timeout) } CLIRunner.emit(result, json: global.json) { r in r.results.map { entry in @@ -977,7 +985,11 @@ struct Down: AsyncParsableCommand { func run() async throws { let params = GroupParams(project: global.resolvedProject()) let result = await CLIRunner.run(json: global.json, bootstrap: !global.noBootstrap) { client in - try await client.request(.groupDown, params: params, expecting: GroupResult.self) + /** A deep dependency chain drains one wave at a time, each with its own + stop grace, so the client waits well past a single stop. */ + try await client.request( + .groupDown, params: params, expecting: GroupResult.self, + operationTimeoutSeconds: 120) } CLIRunner.emit(result, json: global.json) { r in r.results.isEmpty @@ -1750,7 +1762,8 @@ struct Switch: AsyncParsableCommand { } print("stopping servers…") _ = try? await CLIRunner.client().request( - .groupDown, params: GroupParams(project: project), expecting: GroupResult.self) + .groupDown, params: GroupParams(project: project), expecting: GroupResult.self, + operationTimeoutSeconds: 120) var switched = Self.git(["switch", branch], in: project) if switched.status != 0 { /** A remote-only branch needs a tracking checkout. */ @@ -1765,11 +1778,33 @@ struct Switch: AsyncParsableCommand { json: global.json) } print("on \(branch)") - let playbook = (try? ProjectConfigLoader.load(project: project)) - .flatMap { _ in try? JSONCoding.decoder().decode( + /** The lifecycle commands about to run come from the NEW branch's + devservers.json, so validate that file after the checkout, not before. + A config the daemon would refuse to load must not have its committed + argv executed: the previous shape discarded the validated view and + re-decoded the raw file, running lifecycle from a config `config + check` would reject. */ + let validated: ProjectConfigView? + do { + validated = try ProjectConfigLoader.load(project: project) + } catch let error as WireError { + CLIRunner.fail(error, json: global.json) + } + if let view = validated, !view.errors.isEmpty { + CLIRunner.fail( + WireError( + code: .configInvalid, + hint: "run: devctl config check", + message: "the branch's devservers.json is invalid, so its lifecycle was not run: \(view.errors.joined(separator: "; "))"), + json: global.json) + } + let playbook = + validated == nil + ? [] + : (try? JSONCoding.decoder().decode( ProjectFileConfig.self, - from: Data(contentsOf: ProjectConfigLoader.configURL(project: project))) }? - .lifecycle?["switch"] ?? [] + from: Data(contentsOf: ProjectConfigLoader.configURL(project: project))))? + .lifecycle?["switch"] ?? [] for argv in playbook { guard let executable = argv.first else { continue } print("lifecycle: \(argv.joined(separator: " "))") @@ -1801,7 +1836,8 @@ struct Switch: AsyncParsableCommand { try await client.request( .groupUp, params: GroupParams(project: project, timeoutSeconds: timeout), - expecting: GroupResult.self) + expecting: GroupResult.self, + operationTimeoutSeconds: timeout) } CLIRunner.emit(result, json: global.json) { r in r.results.isEmpty diff --git a/docs/cli-contract.md b/docs/cli-contract.md index 7216f04..a053ddb 100644 --- a/docs/cli-contract.md +++ b/docs/cli-contract.md @@ -48,7 +48,7 @@ Exit codes: 0 ok · 1 operation failed (crash, timeout, conflict) · 2 usage · Filled in per phase as each lands; golden tests reference the examples in this file. - `devctl status [name] [--all] --json` → `{servers: [ServerStatus], trusted?}`. Named lookup that matches nothing exits 4 (`not-found`); unnamed always exits 0. `--all` is machine-wide (every registered project) and auto-prunes checkouts whose path is gone; `trusted` is present only on a scoped project query. -- `devctl register --name N --cmd word --cmd word … [--port P] [--cwd D] [--write [--force]] --json` → `{server: ServerStatus}`. `--cmd` repeats per argv word and accepts dash-prefixed values. `--write` also appends the server to devservers.json, merging so every other entry survives; an existing entry of the same name needs `--force` and otherwise fails `already-exists`. +- `devctl register --name N --cmd word --cmd word … [--port P] [--cwd D] [--write [--force]] --json` → `{server: ServerStatus}`. `--cmd` repeats per argv word and accepts dash-prefixed values. The spec is validated the same way a committed one is (a name carrying `::`, an empty command, an out-of-range port or healthcheck value fail `config-invalid`), so a spec `config check` would reject can no longer be registered. `--write` also appends the server to devservers.json, merging so every other entry survives; an existing entry of the same name needs `--force` and otherwise fails `already-exists`. - `devctl start|stop --json` → `{server: ServerStatus}`. `start` returns at spawn with `phase: "starting"`; health promotion to `running` follows (use `wait`/`ensure` to block). `stop` on an already-stopped server exits 0. - `devctl ensure [--port P] [--timeout 60] --json` → `{reason?, server}`. Optional `--port` overrides the committed port for this run (flows through the same effectivePort pipeline as sibling rebind and `devctl.local.json`). State matrix: healthy → no-op; starting → joins the in-flight attempt (single-flight, two concurrent ensures cannot double-spawn); unhealthy → no-op reporting the phase; stopped/crashed/failed → start and block until healthy. Fails fast the moment the phase turns crashed/failed. `reason: "crashed"|"failed"|"stopped"|"timeout"`; reason present ⇒ exit 1, with `lastExit`/`spawnError`/`recentLogTail` forensics in `server`. Sibling port conflict auto-rebinds and still returns ok with `server.portConflict.state: "rebound"`. - `devctl start [--port P] --json` / `devctl up [--only a,b] [--port P] [--timeout 60] --json`: same optional `--port` override as ensure (applied to each started server under `up`). @@ -60,7 +60,7 @@ Filled in per phase as each lands; golden tests reference the examples in this f - `errorSummary` is devctl's own count of the last run's stderr lines with the first and last timestamps, captured when a server turns crashed, failed, or unhealthy. It is arithmetic over the log, never the lines themselves, so the session-context hook can surface that errors piled up without emitting attacker-influenceable child output; the agent reads the actual lines with `devctl why`. - `terminalEvidence` / `recentLogTail` on status: short out/err/sys lines captured at terminal transitions (and persisted across ensure truncate / daemon rehydrate) so `why` still sees a refusal after a retry. - Healthchecks: explicit `healthcheck` block wins; else a declared `port` implies a TCP probe; else healthy = alive past a 2s stabilization window. `ServerStatus.healthcheck` says which (`"http"|"tcp"|"none"`) so agents know when `running` is unverified. `unhealthyAfter` applies only after first-healthy: a slow boot stays `starting`. -- `devctl logs [--follow] [--tail N] [--since 5m|ISO] [--since-mark ] [--grep RE] [--stream out|err|sys|mark] --json` → one `{at, stream, text}` object per line on stdout. `--grep` is the Swift Regex dialect. `--follow` polls incrementally (restart-safe). Structured lines are sanitized: ANSI/OSC escapes stripped, NULs removed, CR spinner rewrites collapsed to the final frame; timestamps are per-file monotonic. +- `devctl logs [--follow] [--tail N] [--since 5m|ISO] [--since-mark ] [--grep RE] [--stream out|err|sys|mark] --json` → one `{at, stream, text}` object per line on stdout. `--grep` is the Swift Regex dialect. A pattern that will not compile, or one that nests an unbounded quantifier inside another (the `(a+)+` catastrophic-backtracking family, which Swift's regex engine runs for minutes on a single line), fails `usage` rather than running. `--follow` polls incrementally (restart-safe). Structured lines are sanitized: ANSI/OSC escapes stripped, NULs removed, CR spinner rewrites collapsed to the final frame; timestamps are per-file monotonic. - `devctl mark [--label L] --json` (or `--all `) → `{marks: [{at, id, server}]}`. The id feeds `--since-mark` on `logs` and `events`, so no clock agreement is needed. Marks flow through the same append path as process output; ordering is exact. - `devctl events [--all] [--since …] [--since-mark ] [--tail N] --json` → `{events: [{at, detail?, kind, project, server}]}` with kinds `started|stopped|crashed|failed|healthy|unhealthy|marked|registered|unregistered`. Scoped to the current project unless `--all`. This is the "what happened while I was compacted" answer. - `devctl why --json` → `{findings: [{server, phase, summary, evidence[]}], rootCause?}`. Diagnoses over the merged project view (committed `devservers.json` plus ad-hoc registry), the same set as `status` / `ensure`. Walks `dependsOn` to the deepest broken dependency; evidence prefers `recentLogTail` / persisted `terminalEvidence`, else a structured-log window (out+err+sys). Exit 0 after a short life notes that controlled refusals are common. Agent session context never embeds raw child lines; it points here. @@ -75,7 +75,7 @@ Filled in per phase as each lands; golden tests reference the examples in this f - `devctl config init [--dry-run] [--force] [--host H] [--name N --cmd word … [--port P]] --json` → `{check: CheckResult, content, notRecovered?, path, written}`. Writes devservers.json from the servers the daemon already knows, which is the way back from losing a gitignored file. Refuses an existing file with `already-exists` unless `--force`; `--dry-run` returns the content and writes nothing. The projection writes only what the file declares: an effective or rebound port, a worktree-derived host, a materialized url, an absolute icon path, and the port and host keys devctl injects into the environment are all dropped, so the file stays portable to another checkout on another machine. `lifecycle` has no runtime counterpart and cannot be recovered; it is named in `notRecovered` rather than silently lost. The written file is indented, unlike every wire frame. - `devctl config check --json` also reports `effectiveHost` and `effectiveHostReason` (`linked-worktree`, `local-overlay`, `server-override`) when a start from this directory would use a host other than the declared one, plus `serverHosts` for the servers that differ from the project. A linked worktree gets an ephemeral `worktree-