Repository navigation
feat: every command answers over a local socket, byte for byte - #272
Merged
Merged
Conversation
Task 1 of .procoder/plans/command-api.md. Every command's working directory and environment came from the process: doctor.Root() called os.Getwd, host.Detect() called os.Getenv, and run wrote through a package-level printLine straight to os.Stdout. That was correct while procoder was a short-lived process serving exactly one caller. A daemon serving several sessions in several checkouts cannot answer from any of them — its own directory and environment belong to whichever session started it, and every request after the first would be answered for the wrong one. run now takes a session: stdin, stdout, stderr, cwd, env. The eighteen helpers that needed a caller became methods on it, so the threading costs nothing at the call sites. processSession is the only place left in the command layer that reads a process global, and it is what the CLI is. Two fields carry a real handle rather than a stream, because two questions cannot be asked of a buffer: stdoutFile answers `procoder format f > f`, and stdinFile answers whether there is a person at the other end. Over a socket both are nil, which is the truthful answer to both. host.DetectIn reads the environment it is given and nothing else; host.Detect is now DetectIn(ProcessEnv()) and no caller changed. No command's behaviour moves. The audit guard that reads run()'s switch out of main.go follows the new signature. docs: none — internal refactor, no user-facing surface changed
Task 2 of .procoder/plans/command-api.md.
internal/api is the second door: one request shape and one response shape
that any of the 47 commands can be asked in. The first door does not move
— a command run from a terminal or from CI behaves exactly as it did, with
no daemon and no setup — which is why this package is a transport and not
a rewrite.
Three fields carry meaning by being nil rather than empty, and each one is
a bug if a reader flattens it:
- Confirm nil is "there was no person", which is the non-interactive
path a command takes today. It is not "no".
- Exit nil is "this job is still running". A caller that read it as zero
would call a running suite green.
- Result nil is "this command does not report findings", which an empty
findings list is not.
Result and Finding are declared here rather than with the collector that
fills them in Task 4, because Response does not compile without them; this
task names the shape, Task 4 fills the value. Finding is a copy of
gitx.Finding rather than the type itself, so declaring the envelope does
not import the domain layer and the wire shape does not change whenever
that struct does.
Envelopes are newline-delimited JSON: both ends are Go and both already
have encoding/json, and a framing nobody can read by eye is a debugging
cost paid on every future bug. The read is capped at 8 MiB on the scanner
rather than checked afterwards, because checking afterwards means having
already read it.
docs: none — internal package with no CLI surface yet; the commands and
API reference land with `procoder serve` in Task 7
Task 3 of .procoder/plans/command-api.md. api.Serve runs one request and builds its response — the whole of what a daemon does to a request, minus the transport. That is the point of doing it before the socket exists: parity between the two doors becomes testable without one, so the socket in Task 7 has nothing left to prove but its own plumbing. The import runs one way only. cmd/procoder hands internal/api a Runner; internal/api knows nothing of the 112 dispatch branches on the other side. apiRunner is where the two doors differ, and there are only three differences: the streams are buffers, the directory and environment come from the caller rather than the process, and there is no file handle at either end. The third is not a gap to close — over a socket there is genuinely no terminal and no redirect, and nil is what says so. Threading the environment the rest of the way found the case the parity test in #117 would have passed while missing: principles.RunHook and the commit hook's decide() both called host.Detect(), which reads the process. One daemon started by a Claude session and later serving a Qoder one would have shaped both answers for the wrong host, with the payload identical and nothing to see. Both now take the environment their caller was called with, and TestRequestEnvironmentPicksTheHost fails if either reads the process again. Stdout and stderr stay apart in the response because the domains already keep them apart — store.Notice, principles.Stderr and the stop hook's writer are separate channels, and merging them at the transport would splice lock notices into a hook's context. docs: none — internal transport; the commands reference lands with `procoder serve` in Task 7
docs: none — task ledger entry, no user-facing surface
Task 4 of .procoder/plans/command-api.md. A caller that wanted the gate's verdict had to parse the lines a person reads, which made every one of them a compatibility surface nobody declared. The response now carries the findings as data beside the prose, and the prose does not move: TestFindingsResultMatchesBytes runs the same gate at both doors and fails if collecting changes a single byte. Two things carry the property that makes this safe. The collector is nil for the CLI and every write to it goes through the session, so a command cannot tell whether anybody is collecting — a command that behaved differently when observed would make the parity test meaningless. And gate.RunCollecting's collector cannot change the verdict; a collector that could would be a second gate. Empty and absent are different answers, asserted by TestEmptyFindingsIsNotNull. A gate run that found nothing carries `"findings": []`; `procoder version`, which reports no findings at all, carries no result. A client that flattened the two would read a clean tree and a version number the same way. The fixture repository adopts procoder, because a repository without .procoder/ gets the gate's universal scope and no formatting check at all (ADR 0005) — the first version of this test asserted against a gate that had deliberately not looked. docs: none — internal transport; the reference lands with `procoder serve` in Task 7
Task 5 of .procoder/plans/command-api.md. Three commands, not the six the plan first named. config, todo list and version already compute the values they print — cfg.Settings carries key, value, source and the default it was relaxed from; todo.List returns []todo.Task; the version pair is two strings. Filling a typed result for those is reading a value that exists. status, spec check and the index queries are line-oriented all the way down: status.Report returns []string with its branch and dirty lines formatted inside six helpers, and spec.Check and codeindex.Find take an out func(string) and never hold a value. A typed result for those means restructuring their domains to build data and render from it — a different change from adding an envelope, and doing it here would mean either parsing our own output back or duplicating the git calls status already makes. The plan carries them as Task 16 rather than dropping them. Kinds() is checked against the source rather than exercised: the guard parses the package's own files and fails on a Kind constant that Kinds() does not list, or the reverse. A kind nobody calls is exactly the one that reaches a client as an object it cannot name, and a behavioural test proves only the paths it ran. version reports no latest. Nobody asked GitHub, and reporting the running version as the newest would be an answer the command did not compute. TestEmptyFindingsIsNotNull moved its example from version to principles. version acquired a kind in this task, which is exactly the drift that assertion watches for. docs: none — internal transport; the reference lands with `procoder serve` in Task 7
Task 6 of .procoder/plans/command-api.md. Six commands ask a person before acting, and over a socket copilot.CanAsk answers false because there is no character device — which is the right answer to the wrong question. What a caller needs to say is not "is there a terminal" but "is there an answer", and those are different: a supplied "no" is a person declining, which is not the same as nobody being there, and neither is the same as a tty. The request carries the answer. CanAskWith, PromptWith and ReadYesFrom take it; CanAsk, Prompt and ReadYes are the nil-supplied calls, so no existing caller changed and the definition of a yes stays in one place — anything but a bare y or yes. nil confirm is not a no. It is the non-interactive path a command takes today when nothing is attached to its stdin, and flattening the two would turn every unattended run into a refusal. PromptWith prints nothing when it has an answer: the question was already put, wherever the answer came from, and asking it again into a socket would be writing a prompt nobody will read. A confirmation sent to a command that never asks is ignored rather than refused. A client that sets it everywhere is clumsy, not wrong, and refusing would make the field a per-command lookup for every caller. docs: none — internal transport; the reference lands with `procoder serve` in Task 7
Task 7 of .procoder/plans/command-api.md. The daemon answers every command on a unix socket, and the CLI does not move: every command still runs in-process with no daemon and no setup, in CI and on a fresh clone. A second door is only useful if the first one is always open. The permission bits are the whole authentication. A unix socket is a filesystem object, so the filesystem has already answered who may talk to this daemon; a loopback port would not, being reachable by every process and every other user on the box and forwarded out of devcontainers by accident. The chmod is a separate call from the listen because net.Listen applies the process umask, which on a default umask leaves the socket world-connectable — the one thing this design must not be. The window between the two calls is closed by the run directory being 0700, which is why RunDir insists on that mode rather than assuming it. serve runs in the foreground and stops when its listener closes. Whatever started it owns it. It does not daemonise itself, for the same reason `procoder run` refuses to own a server's lifetime: a process nobody can see is worse than no process. A client speaking another protocol is refused with a reason on stderr, never served stale behaviour and never answered with silence — a response with no explanation reads to a client exactly like a command that printed nothing. Three guards caught this change and all three were right. docs/commands.md and spec.Commands both had to learn the command. The store's IO guard flagged internal/api/paths.go, which owns ~/.procoder/run — the user's home, not any repository. internal/store cannot own that directory because every one of its operations is scoped to a repo root, and a daemon serving ten checkouts has one run directory and no root to file it under. The skip is written down beside the reason rather than left as a silent exemption. Verified live: `serve --socket` answered a `check` request over the wire with exit 0, 18 findings as typed data, and the human bytes unchanged; the socket came back srw------- as intended.
Task 8 of .procoder/plans/command-api.md, with the two config keys Task 14 was going to add — the client cannot be wired without something to read. Where a machine set [service] mode = "local", a command goes to the socket first. Every failure on that path — no daemon, a daemon from another build, a socket that went away mid-request — falls through to the in-process path, so a degraded transport costs the caller the daemon's speed and never its answer. A transport whose failure could cost a verdict would be a worse gate, not a faster one. Off by default. No repository changes behaviour because it upgraded, and a typo in the value leaves the machine where it was rather than in a state its writer did not choose. The dial timeout is 250ms deliberately. The daemon is an optimisation and the in-process path is right there: a client that waited a second to save fifty milliseconds has made every caller slower to make some of them faster. api.Executes moved here from Task 11, because the client is the first thing that must not offer the four executing commands to the work socket. The socket's 0600 mode authenticates the USER, not the process — every process running as that user can open it, an agent session's own shell included — so #201's boundary cannot be left to the server alone. Two bugs found by running it rather than by testing it, both now pinned: - tryDaemon read stdin unconditionally, which blocks forever on a terminal. Every interactive command on a daemon-configured machine would have hung, and the daemon is meant to be invisible. It now reads only where there is something to read. - The collector setters guarded nil inside set() and assigned outside it. That compiled, passed every test that went through apiRunner, and segfaulted on `procoder version` at a terminal, because the CLI's collector IS nil. TestCommandsSurviveANilCollector runs five commands with no collector and fails on the panic. The config golden gained service.exec and service.mode: `procoder config` lists every setting that has a default, and these have one. docs: none — the daemon's user-facing surface is documented under `procoder serve` in docs/commands.md, which this task does not change
Task 9 of .procoder/plans/command-api.md. Not an optimisation. internal/store's lock is an O_EXCL lockfile with an mtime heartbeat and no in-process registry, so two goroutines of ONE daemon hitting the same repository do not queue behind each other: the second spins, finds a lock the first one's heartbeat keeps fresh so breakStale never fires, reaches lockTimeout, and returns "the write was NOT made". Today that is rare, because every hook is its own process arriving at its own moment. A daemon makes concurrent same-repository work ordinary, so without this the daemon converts a rare race into a routine five-second failure — worst on the read-modify-write ledgers, which are dispatch, claims and the ask queue. Per repository, not one global lock: two sessions in two checkouts share nothing, and serialising them together would make the daemon slower than the spawning it replaces. The queue is held across the whole command rather than around its writes, because the store's lock is per file and two requests interleaving between a read and a write of one ledger is exactly what it cannot see. The identity comes from store.IdentityFor rather than the path: a path is not a key, and two checkouts of one repository must agree. It is supplied to the server rather than computed there, so the transport does not import the identity ladder to answer a transport question. TestPerRootSerialisation fires fifty concurrent requests and fails if more than one is ever inside at once; TestDifferentRepositoriesDoNotWait fails if two repositories share a queue. Both pass under -race. docs: none — internal concurrency fix, no user-facing surface changed
Task 10 of .procoder/plans/command-api.md. Nine commands run a whole toolchain over a whole tree — test, audit, release, bench, deps, and security, docs, ci and index under the flag that makes each long. Held open on a connection, those are the commands a caller most wants back from, and the host hook timeouts (120s PreToolUse, 60s PostToolUse, 15s SessionStart, 10s Stop) are the ceiling they are measured against. They now answer with a job id in milliseconds and keep running behind it. A poll returns everything accumulated so far, so a caller can follow a suite without holding the connection that started it. Which commands are long is a list, not a timeout. It is a fact about procoder, known here; discovering it by waiting would make every caller pay the wait once to find out. The CLI still behaves like the CLI. followJob polls to completion and writes only the new bytes each tick, so `procoder test` over a daemon is `procoder test`. The job exists so the SOCKET does not have to hold a suite open, not so a person has to poll. Lost is not failed. A daemon that restarted does not know how the command got on, and answering "failed" would invent a verdict — so a lost job carries no exit code at all, and the client says the daemon went away rather than re-running a release or a suite that may already have run. Jobs start inside the repository's queue, so Task 9's serialisation still holds for a command that outlives its connection; a poll skips the queue, because reading a job's answer must not wait on the command it is asking about. The table is in memory only. A job that outlived the daemon would have to be re-attached to a process that no longer exists, and a caller whose daemon died needs to hear that rather than read a stale result. docs: none — the daemon's user-facing surface is documented under `procoder serve`, which this task does not change
Task 11 of .procoder/plans/command-api.md. run --exec, evidence record, init --yes and self-upgrade run what a repository — or a prior agent session — declared. procoder reads plenty that such a session could have written: the ask ledger, the handoff note, the backlog, the specs. Hooks run unattended on every write and every commit. #201 closed the gap between those two facts by making "look but don't run" a contract, and internal/hook/noexec_test.go holds it by asserting the hook package cannot even import runcmd. A socket does not change that and cannot be trusted to. Its 0600 mode authenticates the USER, not the process: every process running as that user can open it, an agent session's own shell included. So the four are refused at the work socket's door — exit 2, naming the boundary and saying where they are served — and reachable only on a second socket with its own opt-in, whose address the hooks are never told. Refused at the door and not offered by the client: the server checks because the client cannot be the only guard, and the client checks because a request that will be refused should not be sent. The read-only form of the same command is not executing. `run` prints the declared launch commands and `init` prints the install commands, and treating those as executing would put procoder's most useful read-only surfaces behind a door most machines will never open. TestClientTransportExecutesNothing reads internal/api's imports the way the hook package's guard reads its own: a behavioural test proves only the paths it exercised, and what is wanted here is absence from all of them. Verified live: self-upgrade over the work socket came back exit 2 with the refusal naming #201 and the exec socket's path. docs: none — the boundary and both sockets are documented under `procoder serve` in docs/commands.md, added in Task 7
Task 12 of .procoder/plans/command-api.md. The daemon holds one repository's warm state per repository — its code index and its parsed config — and lets each go on its own schedule. A morning's work in one checkout must not keep nine others' indexes resident, which is what a single global window would do. Thirty minutes by default, because the thing being kept warm is a code index and the gap between two pieces of work in one repository is a coffee, not a day. A window short enough to expire between two commits would throw away most of the reason to run a daemon at all. --idle overrides it. Using a repository restarts its window, so work in progress does not expire because it started thirty minutes ago. A daemon that has held nothing for a whole window closes its listener and stops. Staying resident to serve a request that may never come is how a convenience becomes a process somebody has to remember to kill — and exiting is safe precisely because starting is free: the next hook starts another, and a client that finds no daemon runs in-process. A daemon that has just started holds nothing too, so it gets a whole window to be given work before it decides nobody wants it. One sweep goroutine for the daemon rather than a timer per repository: it is cheaper to reason about, and being a minute late to release an index costs nothing. TestDaemonExitsHoldingNothing fails if Accept never returns; TestBusyDaemonStaysUp fails if a daemon exits out from under work in progress. Both pass under -race. docs: none — --idle is documented in the usage text and under `procoder serve` in docs/commands.md
Task 13 of .procoder/plans/command-api.md. The session-start hook starts the daemon when nothing is listening. That moment is chosen: it is when a machine has something for a daemon to do, and the only moment nobody is waiting on a command's answer. No launchd, no systemd, no install step — a daemon that needed installing would be a setup cost paid by every machine to benefit the ones that opted in. Started after the payload, never before it. The hook's stdout is parsed as JSON by three of the four hosts and the session is waiting on it; starting a daemon is worth up to two seconds and none of them belongs in front of the answer. Single-flight through an O_EXCL lock with a staleness rule — the shape internal/store uses, for the same reason: go.mod has no require block, so a portable flock is not there to spend a dependency on. A caller that loses the race waits for the winner's socket rather than starting a second daemon, and the socket is checked again under the lock because the previous holder may have finished in between. A half-written lock is one nobody is holding: the process died between creating the file and writing its pid, and treating that as live would block every session after it. The commit gate blocked this on exec.Command with a non-static argument, and it was right to. EnsureDaemon now takes no binary path at all: the daemon procoder starts is procoder, os.Executable is the only answer to which one, and a path from a caller would be a parameter that decides what this process executes — a thing to not have rather than a thing to validate. The remaining suppression names what the value provably is. Every failure is advisory and silent to stdout. A session whose daemon would not start runs in-process, which is every session today, and a hook that printed about it would put its noise inside the envelope three hosts parse. Verified live: five session-start hooks fired at once against one home directory left exactly one daemon and one srw------- socket. docs: none — auto-start is the behaviour documented under `procoder serve` in docs/commands.md; no new user-facing surface
Task 14 of .procoder/plans/command-api.md. The two config keys landed in Task 8, where the client first needed something to read; this is the question. A question rather than a default, because the daemon changes how this repository's commands run and nobody should discover it by noticing a socket. Asked from init because that is the one command a person runs deliberately, once, while setting a repository up — and asked after the formatters, which is what they ran it for. Nobody to ask writes nothing. Over the API, in CI, behind a pipe or a hook, there is nobody there, and nobody is not a no: it is nothing written at all. A repository must never acquire a daemon because a script ran init. A typo is the default too. "server" and "local" mean yes and everything else means no, because a machine that acquires a daemon from a mistyped answer is worse than one that has to answer the question twice. A repository that already chose is not asked again — that decision lives in a tracked file, which is where it can be changed properly — and an existing [service] section keeps its other keys. docs: none — the setting is documented under `procoder serve` in docs/commands.md, added in Task 7
Task 15 of .procoder/plans/command-api.md. Forty-eight commands, each run twice against one fixture repository — once through run() with buffers, once through api.Serve — and compared byte-for-byte on stdout, stderr and the exit code. Anything less than byte-identical would let the two implementations drift in exactly the place nobody looks. The table is READ out of the usage text rather than listed here, so a command added to procoder without a parity case fails this test instead of being quietly untested. Two commands are not compared, and both for a stated reason rather than because they were awkward. The four that execute what a repository declared are not served on the work socket at all, and comparing them would mean running them — a companion test asserts each is refused instead. serve blocks until its listener closes, so running it here would hang the suite. The environment is varied, not just the payload. #117's specified parity test compares one payload in and one out, and would have passed while missing the case this change actually found: host.Detect read the process environment, so one daemon serving a Claude session and a Qoder session answered both in one shape. Three requests differing ONLY in their environment now assert three hosts get their own envelope. docs: none — the behaviour is documented under `procoder serve` in docs/commands.md; this task adds only tests
Task 16 of .procoder/plans/command-api.md — the three commands Task 5
deferred because they were line-oriented all the way down.
Each is now one pass that both renders and collects, never two
computations and never a parse of our own output. status.ReportData fills
a Data from the same git calls the lines come from; spec.CheckVerdict runs
the controller once and returns both its lines and its verdicts;
codeindex.FindTags and SearchTags are the lookups Find and Search now
render, with Search's ranking no longer written twice.
The three tests that matter assert the value and the text agree, because
that is the property the restructure exists to buy — a second computation
is a second thing to keep in step, and one that reads its own output is
wrong the first time either side is reworded.
Three fields carry "not known" honestly rather than defaulting to
something readable:
- Dirty is -1 when git did not answer. Zero would say the tree is clean,
and a report that timed out has not seen a clean tree.
- Branch and Default are empty for a detached HEAD or a git that did not
answer, rather than a name that is not true.
- An index lookup that could not run carries no symbol list at all. An
empty one would say the index answered and found none.
SearchTags is uncapped where Search shows fifty: the cap is a thing done
to a person reading a terminal, not to a caller reading a list.
The parity table still passes for all 48 commands after the change, which
is what says the printed halves did not move.
docs: none — the typed results are the daemon's surface, documented under
`procoder serve` in docs/commands.md
All sixteen tasks of .procoder/plans/command-api.md are done. The ledger entry carries what proved each criterion rather than the claim that it was met. docs: none — task ledger entry, no user-facing surface
Files the decision this branch ends on, alongside the measurements already in the ledger under #117: a hook is spawned either way, so a service saves the WORK and not the spawn. This branch is a unix socket dialled from inside the binary rather than the curl the measurements used, so the extra spawn does not apply — but the warm-index case is still unmeasured, and `[learn] record` is off. docs: none — decision ledger entry, no user-facing surface
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and portability gaps in the new transport (notably Windows support for the “unix” socket transport, missing build/version handshake implementation, and response-size capping applied to responses) that can break advertised behavior and CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a local-socket “second door” for procoder: a daemon (procoder serve) that can execute commands via a newline-delimited JSON envelope and return byte-identical stdout/stderr/exit codes, optionally accompanied by typed results and job polling for long-running commands—while keeping the existing CLI in-process behavior as the primary/default path.
Changes:
- Adds
internal/apitransport (envelope, client/server, per-repo serialisation, warm state + idle eviction, background jobs, and an exec-socket boundary for executing commands). - Threads request environment/cwd/streams through sessions so daemon-served commands can behave like CLI runs (and fixes/extends typed result plumbing for select line-oriented domains).
- Extends configuration/docs/tests to cover
[service]settings, init prompting, parity tests across commands, and newservecommand discoverability.
File summaries
| File | Description |
|---|---|
| internal/store/testdata/golden/config.txt | Updates golden config output to include new [service] defaults. |
| internal/store/golden_test.go | Adjusts golden capture to pass explicit host env into principles hook. |
| internal/store/coverage_test.go | Excludes internal/api/paths.go from store coverage guard with rationale. |
| internal/status/status.go | Adds typed status.Data and ReportData alongside rendered lines. |
| internal/status/status_test.go | Extends tests for new typed status data and deadline behavior. |
| internal/spec/truth.go | Adds serve to the truth command list. |
| internal/spec/spec.go | Adds typed Verdict + CheckVerdict collection alongside printed output. |
| internal/principles/principles.go | Threads host.Env through hook rendering to avoid process-env dependence. |
| internal/principles/principles_test.go | Updates tests to pass host.ProcessEnv() into RunHook. |
| internal/initcmd/service.go | Adds init-time prompting/writing of [service] mode when user opts in. |
| internal/initcmd/service_test.go | Tests init server-mode prompting and config mutation behavior. |
| internal/host/host.go | Introduces host.Env, ProcessEnv, and DetectIn for request-scoped host detection. |
| internal/host/host_test.go | Adds a test ensuring DetectIn reads only its argument. |
| internal/hook/commit.go | Threads host.Env through commit hook output shaping. |
| internal/hook/commit_test.go | Updates commit hook tests to pass host.ProcessEnv(). |
| internal/gate/gate.go | Adds a Collector path to collect findings as data alongside printed output. |
| internal/docs/coverage.go | Adds serve to the docs coverage command list. |
| internal/copilot/prompt.go | Adds *With/*From variants to support supplied answers (socket/daemon). |
| internal/copilot/prompt_test.go | Tests “supplied answer” behavior for non-tty scenarios. |
| internal/config/config.go | Adds [service] mode/exec config fields and defaults. |
| internal/config/config_test.go | Tests service defaults and typo handling for mode. |
| internal/codeindex/query.go | Adds FindTags/SearchTags to return typed index results without parsing output. |
| internal/audit/trademark_test.go | Updates run() signature string search for discoverability test. |
| internal/api/warm.go | Adds per-repo warm state + per-repo eviction window logic. |
| internal/api/warm_test.go | Tests eviction, “keep warm on use”, and daemon idle-exit behavior. |
| internal/api/start.go | Adds single-flight daemon autostart with stale-lock breaking and bounded wait. |
| internal/api/start_test.go | Tests lock semantics, staleness handling, and dead-socket detection. |
| internal/api/server.go | Implements server listen/accept loop, exec-boundary enforcement, queueing, and job handling. |
| internal/api/server_test.go | Tests socket permissions, stale socket clearing, protocol skew refusal, and parity basics. |
| internal/api/serialise.go | Implements per-identity request serialisation (queues). |
| internal/api/serialise_test.go | Tests per-root serialisation and cross-root independence. |
| internal/api/runner.go | Adds a runner interface and Serve() helper for socketless parity testing. |
| internal/api/runner_test.go | Tests stream separation and nil-result semantics. |
| internal/api/result.go | Adds typed Result and cross-wire Finding shape. |
| internal/api/paths.go | Defines run dir + socket paths under user home with secure permissions. |
| internal/api/kinds.go | Declares typed result kinds and their schemas (+ Kinds() registry). |
| internal/api/kinds_test.go | Guards that kind constants and Kinds() do not drift. |
| internal/api/job.go | Adds long-running command classification and in-memory job table/polling. |
| internal/api/job_test.go | Tests long-running classification, job survival, and lost-job semantics. |
| internal/api/exec.go | Defines “executing” argv detection for the #201 boundary. |
| internal/api/exec_test.go | Tests executing-command detection, work-socket refusal, and transport noexec guard. |
| internal/api/envelope.go | Defines request/response envelope encoding + bounded read via scanner. |
| internal/api/envelope_test.go | Tests envelope round-trips and oversize request refusal. |
| internal/api/client.go | Adds daemon client with dial and protocol skew handling. |
| internal/api/client_test.go | Tests protocol skew fallback, dead socket behavior, and full response carry. |
| docs/commands.md | Documents procoder serve and the two-socket boundary. |
| cmd/procoder/serve.go | Adds serve command implementation and argument parsing. |
| cmd/procoder/parity_test.go | Adds full-command parity test comparing CLI vs envelope responses. |
| cmd/procoder/main_test.go | Adds session/process boundary tests and nil-collector safety tests. |
| cmd/procoder/flags.go | Adds known flags for serve (--socket, --exec, --idle). |
| cmd/procoder/flags_test.go | Updates flag refusal test to use new run() signature/session. |
| cmd/procoder/daemon.go | Adds daemon client fallback path, job following, and session-start autostart. |
| cmd/procoder/collect.go | Adds collector plumbing to build typed results alongside printed output. |
| cmd/procoder/api.go | Implements apiRunner that runs commands using a request-built session. |
| cmd/procoder/api_test.go | Adds extensive tests for typed results, confirmation threading, and parity. |
| .procoder/todo/20260903-command-api-tasks-2-16-the-second-door.md | Adds/records completed tasks and evidence for the “second door” work. |
| .procoder/todo/20260903-command-api-task-1-the-session-not-the-process-says-where.md | Adds/records task 1 rationale + evidence for session/process separation. |
| .procoder/specs/command-api.md | Adds the COMPLETE spec defining envelope/daemon/typed results/boundaries. |
| .procoder/ask/decisions.md | Adds decision log entries about committing/landing the branch. |
Review details
- Files reviewed: 61/61 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drops the issue-provenance framing from the command-api spec and the decision ledger. The spec says what it builds and why; it does not need a prior proposal's numbering to be read, and carrying it made every summary of this work an argument about that proposal instead. The design decisions themselves are unchanged and stay where they belong — in the sections that make them. docs: none — spec and ledger prose, no user-facing surface
Found by driving the hooks against a real daemon rather than by running the suite, which is what the suite could not see. tryDaemon reads stdin so it can put it in the request. When the daemon is not there — the ordinary case on a machine that configured one and has not started it yet — the command runs in-process instead, and it was handed the reader those bytes had already been taken out of. The effect was silent and total: `[service] mode = "local"` with nothing listening made every PostToolUse hook produce NOTHING. An empty payload reads as "there is no file to check", so the formatting gate stopped running and said nothing about it. Silent green is the one failure this tool exists to prevent, and it applied to all four hooks, `check --paths-from -` and `scrub -`. tryDaemon now returns the stdin the caller must use, and main uses it. TestFallbackKeepsTheCallersStdin drives the whole path — attempt, fall back, run — and fails if the envelope comes back empty. Also documents what the previous commits left undocumented: `[service] mode` and `[service] exec` in docs/configuration.md, `--idle` in the `procoder serve` section, and a CHANGELOG entry for the whole change. The docs obligation was cleared with `docs: none` on commits that added user-facing configuration, which it should not have been. docs: docs/configuration.md and docs/commands.md updated
…never get Two defects, both found by asking whether the switch to the daemon actually happens rather than whether the output looks right. The daemon never said what it served. Server.Notice was declared, wired and written to by nothing — so a served request and a silent in-process fallback produced identical output, and "is the daemon being used?" had no answer except by inference. It now prints a line per request: what it served, the exit code, how long it took, and separately what it refused and what it started as a job. That is what made the second defect visible. tryDaemon read stdin for every command. Reading it is only right for the commands that consume it, and wrong everywhere else: an open pipe with nothing coming — a CI runner, an agent's shell, a host holding a hook's pipe — never reaches EOF, and io.ReadAll on one waits forever. `procoder deps` against a running daemon hung until it was killed, with the daemon logging nothing because the request was never sent. api.ReadsStdin names the six shapes that consume it: the three hook payloads, `principles --hook`, `scrub -`, and `check --paths-from -`. A list for the same reason IsLongRunning is one — which commands read stdin is a fact about procoder, known here, and discovering it by reading and seeing what happens is the hang. Verified against a real daemon with nothing redirected: version, status, config, deps and the PostToolUse hook all served, each named in the daemon's log; with [service] mode unset the same daemon served zero. A warm daemon answered `version` in 173ms, then 39ms, then 20ms. docs: docs/commands.md documents the request log and why it exists
The in-process fallback is gone. Where [service] mode = "local" the daemon
is the path: a daemon that is not running, is from another build, or goes
away mid-request is an error naming the fix, and the command does not run.
The fallback was wrong for the reason this tool is built on. Two silent
paths to one verdict makes "where did this come from" unanswerable from
the outside, and a machine configured for the daemon could spend weeks
never reaching it with nothing saying so. An error is louder and shorter.
Three things follow, and each is now stated rather than implied:
- A wait that runs out is reported, not retried here. The command may
already be half-done inside the daemon, and running a release or a
suite twice is worse than saying it did not finish.
- A version mismatch refuses and runs nothing.
- The four commands that run what a repository declared are refused on a
server machine whose exec socket is closed, naming `exec = true` and
`mode = "off"` as the two ways forward. Not run here instead — there
is no second path anywhere in this design now.
The stdin hand-back added two commits ago goes with it: nothing runs after
a failed attempt, so there is nothing to hand back.
Verified: on a server machine with no daemon, config, status, version and
the PostToolUse hook all exit 1 with the reason and the fix, and produce
no output; `run --exec` exits 2 naming the setting. With mode unset all of
them exit 0 as the CLI.
NOTE for review: a hook on a server machine with no daemon now exits
non-zero, where every other procoder hook path is built to exit 0 rather
than disturb a session. The session-start hook starts the daemon, so the
window is small — but it is a real change to hook behaviour and it is
called out here rather than discovered.
docs: docs/configuration.md and docs/commands.md say there is no fallback
and why; CHANGELOG updated
Hooks fail when they cannot reach the server, as they should. Three of the four fail outright — exit 1, no output, the reason and the fix on stderr. The commit gate's hook is the exception, and not because it should be softer. It has to fail HARDER than an exit code can express. A deny is carried as a JSON decision on stdout; a non-zero exit reads to the host as the hook erroring, and the command it was gating goes ahead. So the previous commit's exit 1 meant that on a machine whose daemon had died, every `git commit` sailed through unchecked while the hook looked like it had failed loudly — worse than having no gate, because the failure looks handled. It now denies, saying nothing was checked and this commit is NOT verified, and naming both ways forward. Nothing ran, so nothing is passing — the same rule the rest of the tool holds to. hook.Deny is exported for this. The daemon client needs the host's own vocabulary for "do not run this", and reimplementing the envelope there would be a second place for the four host shapes to drift. Verified: with mode = "local" and no daemon, pre-tool-use emits the deny decision and exits 0; post-tool-use, stop and principles --hook all exit 1 with empty stdout. docs: docs/commands.md and the spec say what each hook does when the daemon is unreachable; CHANGELOG updated
Two defects, both found by running a session end to end rather than command by command. A server machine could never get its first daemon. The auto-start lived inside run()'s `principles --hook` branch, and viaDaemon takes that command before run() is reached — so the hook that starts the daemon failed for want of one, forever. It now ensures the daemon before dialling, and only for that hook: it is the moment a machine has something for a daemon to do and the only moment nobody is waiting on an answer. Every other command finding no daemon is a machine that has not started one, which is a thing to be told rather than papered over. An auto-started daemon logged to nothing. Its streams were nil, so the request log added two commits ago was invisible in exactly the case that produces most daemons — nobody starts one by hand. It appends to ~/.procoder/run/serve.log instead, which is where you look when a command is slow or a machine is not using the daemon it was configured for. Still never this process's streams: a daemon writing into a hook's stdout would put its startup line inside the JSON envelope the host parses. Verified end to end against one daemon, from its own log: session start (which started it), status, config, check, git, review, deps as a job, the PostToolUse hook and the commit gate — all served. `run --exec` is absent from the log because the client refuses it before sending. The gate denied an unformatted commit with the real finding and said nothing at all on a clean one, byte-identical to the CLI path. docs: docs/commands.md documents ~/.procoder/run/serve.log
…everything Answering "is this a bus or polling?" turned up three defects. It is polling — a job table in the daemon's memory, an id back in milliseconds, and the client asking. No broker: go.mod has no require block, and a message bus would be the first dependency, spent on a single-machine problem a map and a mutex already solve. A poll returned everything accumulated, every time. A two-minute suite polled every 100ms re-sends a growing buffer 1200 times — quadratic in the output, and worst on exactly the runs the job model exists to support. The request now carries how much the caller already has, per stream, and the daemon returns only what is new. An offset past the end reads empty rather than panicking: a client with more than the daemon has is a daemon that restarted, and the answer to that is a lost job, never a crash in the one process everything depends on. Eight sessions starting at once could start three daemons. TestStartLockUnderRace failed about one run in ten and the test was right: a loser read the winner's file before the winner had written to it, judged an empty lock abandoned, removed it and took its own. "A half-written lock is one nobody is holding" is true of a process that died mid-write and false of the one still writing, and only age tells them apart. It is now judged by age. TestHalfWrittenStartLockIsStale asserted the bug and has been rewritten to assert the rule. Naming a socket created a directory. The refusal for an executing command names the exec socket without any intention of opening it, and doing so ran MkdirAll on the developer's real home — a side effect outside anything the caller asked for, and interference between a test run and a daemon the same person was running. Path lookup is pure now; RunDir still creates, and is called only by the two things that need the directory to exist. Verified: 40 runs of the lock tests and 25 -race runs of the package, clean; three trials of eight simultaneous real session starts, one daemon each time. docs: none — internal transport fixes; the job model is already documented under `procoder serve`
The Windows CI job failed with "socket mode is 0666, want 0600", and it was right to. The socket has no port and no token: the permission bits ARE the answer to "who may drive this daemon". os.Chmod on Windows sets one thing, the read-only bit, so 0600 is unreachable there and a socket comes back 0666 — openable by every account on the machine. Serving anyway would leave the daemon least safe on the platform where the guarantee is weakest, and silently. So Listen refuses on Windows, saying why and saying what still works. Nothing is lost: every command runs in-process there, which is the whole of procoder on that platform. The spec claimed "a named pipe of the same name on Windows" in three places. No named pipe was ever written — every listener is net.Listen on a unix socket — so the spec was describing a thing that did not exist while the code shipped a weaker one. That is the more important half of this commit: the claim is now what the code does, and reaching real parity is named as what it is, a named pipe with an ACL, which needs golang.org/x/sys and therefore a decision about the zero-dependency rule rather than a quiet TODO. TestNamingASocketCreatesNothing set only HOME, so os.UserHomeDir on Windows kept resolving the real profile and the assertion was about a directory nothing had been asked to create. It sets USERPROFILE too. Verified: GOOS=windows builds and vets clean; the suite passes here. docs: docs/commands.md says the daemon is macOS and Linux only, and why
All from Copilot's review on #272, and all real. A second `procoder serve` stole the first one's socket. Listen removed any file at the path before binding, so a live daemon was orphaned — still running, holding a socket nothing could reach, while every client silently talked to the second. It now refuses when something is already listening. The old comment said "which is why the caller dials first"; no caller did, and dialling belongs here, where the removal happens. The version handshake compared only the protocol. Client.Version was unused and Server.Version never left the process, so two different builds speaking protocol 1 were treated as compatible — which is exactly the skew worth catching, because a daemon left running from an older build answers with that build's behaviour and says nothing. Both sides now state their build and a mismatch refuses. MaxRequestBytes capped responses too. A request is what an unknown caller sends and a response is this daemon's own output — `procoder audit` over a large tree legitimately produces megabytes — so an honest answer could be refused, with an error calling it a request. Responses have their own, much larger cap and their own wording. An unidentifiable request shared a queue and a warm cache with every other one, which is the opposite of what serialise.go promises. The working directory is now the fallback key: the weakest honest one, and the same rung store.IdentityFor falls back to. newJobID returned a constant when crypto/rand failed, so every job after that shared an id — each replacing the last in the table while its caller polled somebody else's command. A counter cannot collide within the one lifetime these ids have. `procoder init` asked again in a repository that had recorded `mode = "off"`, and a stray yes flipped it. "off" is both the default and a decision, and the effective value cannot tell them apart, so the written key is what is read now. Each fix carries the test that fails without it. docs: none — the two user-facing surfaces here already say what they now do; `--idle` was added to the synopsis when it was added to the command
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md # internal/gate/gate.go
The Windows job went red on eleven tests, all with the same message — "the daemon does not run on Windows" — which is the refusal added in the previous commit doing exactly its job. Every test that opens a socket was asserting behaviour of a thing that platform does not have. requireDaemon() skips those, and it sits inside testServer so its callers inherit it; the handful that call Listen directly say so themselves. What Windows does have is still asserted: TestWindowsRefusesToServe checks the refusal, and the thirty-odd tests in this package that need no socket — the envelope, the job table's offsets, IsLongRunning, Executes, ReadsStdin, the kinds guard, per-repository eviction, the start lock — keep running there. The run directory's mode is checked only off Windows for the same reason: os.Chmod there sets the read-only bit and nothing else, so the directory comes back 0777, and asserting 0700 is asserting a thing the platform cannot do. That limitation is the whole reason the daemon refuses. A script over the package now reports which tests open a socket without the guard, and it reports two: the permissions test, which carries its own skip, and the refusal test, which must run on Windows. docs: none — test-only change
This was referenced Sep 9, 2026
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.
Every procoder command can now be called over a local socket and answers
exactly what the binary answers. Spec
.procoder/specs/command-api.md,plan
.procoder/plans/command-api.md, both COMPLETE; sixteen tasks, eachcommitted gate-clean with the suite green.
What this is
A second door. The first one does not move: every command still runs
in-process, with no daemon, no socket and no setup, in CI and on a fresh
clone. A second door is only useful if the first is always open, and
[service] modeisoffby default — no repository changes behaviourbecause it upgraded.
A response carries the command's exact bytes on stdout and stderr, its
exit code, and — where the command has one — a typed result beside them.
Both, so that no caller has to be a parser and none has to be a renderer.
The claim, and the test that makes it
TestParityAcrossEveryCommandreads the command list out of the usagetext and runs 48 commands twice each, comparing stdout, stderr and the
exit code byte-for-byte. A command added to procoder without a parity case
fails the test rather than going quietly untested.
It varies the environment as well as the payload, which is what catches
the class of defect where the bytes match and the answer is still wrong
for the caller.
Three things worth a reviewer's attention
The executing boundary is structural, not a rule.
run --exec,evidence record,init --yesandself-upgraderun what a repository —or a prior agent session — declared. The socket's 0600 mode authenticates
the USER, not the process: every process running as that user can open it,
an agent session's own shell included. So those four are refused at the
work socket's door and reachable only on a second socket with its own
opt-in, whose address the hooks are never told.
TestClientTransportExecutesNothingreadsinternal/api's imports theway the hook package's guard reads its own.
One correctness fix that is not about the transport.
internal/store's lock is an O_EXCL lockfile with an mtime heartbeat andno in-process registry, so two goroutines of one daemon on the same
repository do not queue — the second waits out
lockTimeoutand returns"the write was NOT made". Rare today because every hook is its own
process; ordinary under a daemon.
TestPerRootSerialisationfires fiftyconcurrent requests and fails if more than one is ever inside.
A defect the environment threading found.
principles.RunHookand thecommit hook's
decide()both calledhost.Detect(), which reads theprocess environment. One daemon started by a Claude session and later
serving a Qoder one answered both in the wrong shape, with the payload
identical and nothing to see.
Deliberately not here
A structured output format for its own sake (the response carries today's
bytes, so the parity test stays writable); a team server; proactive work —
the daemon does nothing unasked.
Verified live, not only in tests
A
checkrequest over the socket returned exit 0 with 18 findings astyped data and the human bytes unchanged; the socket came back
srw-------;self-upgradeover the work socket returned exit 2 namingthe boundary and the exec socket's path; five session-start hooks fired at
once left exactly one daemon.
go test ./...— 57 packages, 0 failures.procoder check— 0 blocking.