* refactor(backend): LLD maintainability fixes in controllers/service layers
Addresses the high + medium severity findings from the LLD review of
backend/internal/httpd and backend/internal/service (#95):
1. Controllers no longer import internal/session_manager. Session sentinel
errors are now *domain.ServiceError values carrying their own HTTP mapping,
so the controller translates them with one generic errors.As — no
cross-package sentinel imports.
2. One error pattern across services: project.Error is now an alias of the
shared domain.ServiceError, and session_manager sentinels use it too. A
single writeServiceError replaces the per-resource error switches.
3. Clean-orchestrator business logic moved out of the controller into
session.Service.SpawnOrchestrator(ctx, projectID, clean).
4. isGitRepo no longer treats case-different paths as equal on case-sensitive
filesystems; case-insensitive compare is gated to darwin/windows via samePath.
5. Project repo check sits behind an injectable GitChecker, so the service is
testable without a real git binary.
6. httpd exports only the production constructors (NewWithDeps,
NewRouterWithControl); the 3 test-only wrappers are removed and the
"router with empty deps" convenience moved to an unexported test helper.
Closes#95
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(backend): standardize service errors on internal/httpd/errors
Replace the domain.ServiceError approach with a REST-API-scoped error package
and a single envelope renderer, per review feedback:
- Add internal/httpd/errors (package errors, aliased apierr): one structured
Error type with semantic Kinds (Internal/Invalid/NotFound/Conflict) and
constructors. Imports nothing, so any layer can depend on it.
- envelope.WriteError is now the single path from a service error to the wire
APIError, and the only place a Kind becomes an HTTP status/word. The
per-resource writeProjectError/writeSessionError translators are gone.
- Delete domain/errors.go (keeps domain pure of HTTP-flavored kinds) and
service/project/errors.go (no per-service error files); services build
errors inline via apierr constructors.
- session_manager sentinels are apierr.Error values (pointer identity still
works with errors.Is).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* revert(backend): drop GitChecker seam and isGitRepo case-sensitivity change
Defer findings #4 (isGitRepo case-sensitivity) and #5 (GitChecker seam) out
of this PR. Restores the original exec-based isGitRepo and the New(store)
constructor; removes git.go, git_test.go, and the test-only export shims. The
error-standardization and other findings are unaffected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(session): translate engine errors to API errors at the facade
The session_manager is the internal command engine and must not depend on the
REST API error vocabulary. Revert its sentinels to plain errors.New values and
move the engine→API translation into the service/session facade (toAPIError),
which is the correct boundary. Controllers still see apierr.Error and never
import the engine; the engine no longer imports internal/httpd/errors.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(session): tighten error comments to state what the code does
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* style(envelope): make KindInternal an explicit case in httpStatus
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(apierr): rename package, test SpawnOrchestrator, parity fixes
Address review feedback on PR #96:
- Rename internal/httpd/errors → internal/httpd/apierr (package apierr) so
importers no longer alias around the stdlib errors package.
- Add a commander seam to session.Service and unit-test the relocated
clean-orchestrator rule: clean=true kills all active orchestrators before
spawning; clean=false spawns without kills.
- project.Add: wrap the UpsertProject store error in apierr.Internal for parity
with its sibling paths (was a raw 500).
- Document that KindInternal is iota's zero value, so a zero-value Error
defaults to 500.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Introduces backend/.golangci.yml (27 linters across correctness, dead-code/
boilerplate, style, and security), wires it into CI as a blocking job, and
fixes every finding so the tree starts at zero.
Config:
- 27 linters: errcheck, govet, staticcheck, errorlint, bodyclose,
sqlclosecheck, rowserrcheck, nilerr, makezero, unused, unparam, unconvert,
wastedassign, copyloopvar, prealloc, dupl, revive (incl. exported-symbol doc
comments), gocritic, misspell, usestdlibvars, predeclared, nakedret, gosec, …
- Tuned for signal over noise: govet/shadow and gocritic hugeParam/rangeValCopy/
unnamedResult disabled (idiomatic-Go false positives); sqlc-generated code and
tests get scoped exclusions; gosec G304 excluded (paths are config/run-file/
worktree-derived, not user input); nilerr excluded in cli/status.go (probe
failures are the reported status, not a command error).
CI:
- New blocking lint job (golangci-lint-action, latest binary for Go-version
compatibility).
- go-version now read from go.mod (was pinned 1.22 while go.mod declares 1.25).
Cleanup to reach zero (no behavior change):
- errcheck: wrap deferred/inline Close()/Remove()/Rollback() with `_ =`.
- gosec: tighten dir/file perms (0755->0750, 0644->0600).
- unparam: drop always-nil error return from startLifecycle; drop unused
shellPath param (zellij PowerShell) and always-500 fallbackStatus param
(writeProjectError).
- gocritic: regexp \d, s != "", switch->if, combined appends.
- revive: doc comments on all exported symbols; rename project.ProjectRow ->
project.Row (stutter); rename `max` locals shadowing the builtin.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the /mux route: httpd performs the WebSocket upgrade (coder/websocket)
and adapts the connection to terminal.wsConn via wsjson, then hands it to
terminal.Manager.Serve. httpd owns only the upgrade and transport
adaptation; all stream logic stays in internal/terminal.
The route is mounted outside the per-request Timeout middleware (the
connection is long-lived) and is omitted entirely when no manager is wired,
so the daemon degrades to no terminal surface rather than failing. New/
NewRouter take the manager; main.go passes nil until commit 3 wires it.
mux_test.go drives the real upgrade + wsjson + Serve + creack/pty path with
a throwaway shell command, so it needs no tmux.
* feat(backend): HTTP daemon skeleton — config, health, runfile, graceful shutdown (#10)
Phase 1a of the Go HTTP daemon lane (#10). Stands up the loopback-only
sidecar skeleton the later REST/SSE/WS/static surfaces build on:
- config: env-driven (AO_HOST/PORT/ENV/timeouts/run-file) with zero-config
defaults; binds 127.0.0.1:3001; validates and fails fast on bad input.
- httpd: chi router with the recoverer → request-id → logger → real-ip
middleware stack and /healthz + /readyz probes. Per-request timeout is
carried in config but intentionally not global — it scopes to /api/v1 in
Phase 1b so it never throttles SSE/WS/health.
- runfile: atomic PID + port handshake (running.json) for the Electron
supervisor, with a dead-PID stale check so a crashed predecessor doesn't
block startup while a live one fails fast.
- server: bind-before-publish (port conflict fails fast), graceful shutdown
on SIGINT/SIGTERM via signal.NotifyContext with a 10s hard timeout, and
run-file cleanup on exit.
Why: the daemon must be safely supervisable as a child process — the
supervisor needs a discoverable PID/port and the daemon must not leave a
half-started process or stale handshake behind. Locking the lifecycle down
now keeps the future port split a small change rather than a rewrite.
Tests cover config defaults/overrides/validation, run-file round-trip and
live/dead PID detection, health probes, full Run lifecycle, and port-conflict
fail-fast.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* refactor(backend): drop Env config field — not needed yet (#10)
Per review on #14: AO_ENV / Config.Env / IsProduction() weren't load-bearing
for Phase 1a — they only switched the slog handler. Removing them now keeps
the surface minimal; the env knob can come back later when a real consumer
needs it.
- config: remove Env field, AO_ENV parsing, and IsProduction helper.
- main: collapse newLogger to a single text-handler path.
- httpd: drop the env field from the listening log line.
- tests: drop the env assertions and AO_ENV fixture.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs: add backend run + config quick-start to README (#10)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(backend): address Phase 1a review comments (#10)
- config: drop AO_HOST entirely — the daemon is loopback-only by design,
so making the bind host env-configurable was a security footgun
- config: use net.JoinHostPort in Addr() so IPv6 literals stay valid
- config: reject zero/negative AO_REQUEST_TIMEOUT and AO_SHUTDOWN_TIMEOUT
(time.ParseDuration accepts both; either would silently break the
daemon — instant request expiry / no graceful drain)
- runfile: split processAlive into unix/windows build-tagged files so
liveness detection is reliable on both platforms (Windows uses
OpenProcess; POSIX keeps signal 0)
- runfile: document os.Rename overwrite semantics (atomic on POSIX,
REPLACE_EXISTING on Windows) so the temp-then-rename pattern's
cross-platform behaviour is explicit
- httpd tests: give probe/waitForHealth clients an explicit per-request
timeout so a stalled connect can't hang the test on the outer deadline
* fix(backend): strip trailing blank line from runfile.go (#10)
gofmt CI was failing because removing the orphan processAlive doc
comment left an extra newline at EOF.
* fix(backend): cross-platform run-file replace + AO_HOST rationale (#10)
- runfile: introduce build-tagged atomicReplace — POSIX rename(2) on
Unix, MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows. The Go
runtime happens to do the Windows call internally already, but
invoking it directly makes the cross-platform contract explicit
instead of a runtime implementation detail
- runfile: tighten process_unix.go build tag from `!windows` to `unix`
so plan9/js/wasm fail to build rather than silently using a broken
signal-0 probe
- runfile: add TestWriteOverwritesExisting covering the stale run-file
replace path that none of the previous tests exercised
- config: anchor the loopback-only decision in the LoopbackHost doc so
the next contributor doesn't reintroduce AO_HOST without the security
rationale
* fix(backend): route chi access logs through slog/stderr (#10)
chi's middleware.Logger writes via stdlib log to stdout, but the
daemon's slog logger writes to stderr — so REST traffic and daemon
logs landed on different streams in different formats. Replace it
with a small slog-backed requestLogger that:
- Wraps the response writer via middleware.NewWrapResponseWriter so
status/bytes are accurate even when handlers return without an
explicit WriteHeader.
- Reads the request id off the context set by middleware.RequestID
(kept mounted just before this middleware so the id is available).
- Emits one structured Info line per request with method, path,
status, bytes, duration, and remote — same key=value shape as the
rest of the daemon, one stream for the Electron supervisor to
capture.
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
neversettle
prateek
Dhruv Sharma
Pritom14