* 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>
Addresses review findings on PR #53 (on top of the rebase onto main).
- doctor: stop opening/migrating SQLite. The daemon is the sole store
writer/migrator (architecture.md §7); the CLI must not run migrations or
open a second writer against a DB a live daemon owns. doctor now reports
database-file presence and gains --json.
- stop: only remove running.json when it still belongs to the PID we
stopped, so a concurrent `ao start` that wrote a new run-file is not
clobbered into looking stopped.
- httpd: gate POST /shutdown to loopback callers with no Origin header,
closing the CSRF / DNS-rebinding vector against an unauthenticated,
state-changing endpoint.
- start: detach the spawned daemon into its own session/process group so a
Ctrl-C while `ao start` waits for readiness doesn't also kill it.
- cli: exit 2 for usage errors (bad flag / arg count) vs 1 for runtime
failures.
- daemon: unexport newLogger (only used in-package).
- tests: /shutdown guard (cross-origin + rebinding) and stop run-file
ownership guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* 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.
* feat(api): projects route shell (7 routes, REST-corrected) — #20
Mounts the /api/v1 surface on the skeleton router (#10·1a) and registers
the 7 canonical project routes as 501 stubs that emit a structured
PlannedRoute body documenting the future contract. Shared scaffolding
landed here (api.go, errors.go, stubs/, controllers/) so #21/#22 plug in
without re-touching the wiring.
WHY: opens the route-shell PRs in the Go HTTP daemon lane. Doing it
interface-first lets the dashboard team build against the contract
before any handler logic exists; the locked APIError envelope and
PlannedRoute shape become #19's OpenAPI source-of-truth.
REST audit corrections vs the legacy TS surface:
R3 PUT /projects/:id alias of PATCH: PUT not registered → 405.
R4 POST /projects/:id repair overload: canonical /repair; legacy 405.
R5 degraded GET returns 200 with error field: discriminator status.
R6 ok/success flag flips: drop on 2xx; return affected resource.
R9 bare {error: msg}: locked {error,code,message,requestId,details?}.
Legacy paths are deliberately NOT registered; each canonical handler
carries PlannedRoute.Legacy so consumers can discover the migration.
Zod schemas (TrackerConfig, SCMConfig, AgentConfig, ReactionConfig,
LocalProjectConfig, RoleAgentConfig) ported to typed Go structs with an
Extra map reserved for .passthrough() round-tripping in later PRs.
Closes part of #18; targets feat/issue-10 until #14 merges.
* refactor(api): collapse ProjectService → ProjectManager — #20
Controllers now depend on ONE inbound interface per resource — ports.ProjectManager —
mirroring the existing ports.SessionManager + LifecycleManager pattern.
Whether the manager impl reaches into the registry, the LCM, an outbound
port, or all three is its own concern; the HTTP layer no longer has to
know any of that.
WHY: the original split named the boundary type "ProjectService" and put
it in a sibling services.go. That implied a second category of port
distinct from inbound.go's *Manager interfaces, even though they play
the same role (things HTTP/CLI call into the core). Per review feedback,
collapse them onto one Manager-per-resource pattern.
Mechanical changes:
- ports/inbound.go gains ProjectManager next to SessionManager.
- ports/services.go renamed to projects.go; keeps only the DTOs the
ProjectManager methods take/return.
- ProjectsController.Svc renamed to Mgr; APIDeps.Projects type bumped
to ports.ProjectManager.
All tests pass unchanged; no behavioural change.
* refactor(api): replace stubs/ with OpenAPI-as-source-of-truth — #20
The first cut of the route shell duplicated each route's contract twice:
once as a Go literal (stubs.PlannedRoute{...}) in the controller, and
implicitly in the PR description. The Go literal was ~230 LoC of pure
throwaway that would be deleted in handler-impl PRs.
This commit eliminates the duplication:
- backend/internal/httpd/apispec/openapi.yaml: full OpenAPI 3.1 doc
covering the 7 project routes + shared schemas (Project, APIError,
config types). x-replaces records the legacy → canonical mapping
REST-audit corrections produced.
- apispec/apispec.go: //go:embed the YAML, expose Operation(method,
path) → the spec slice as a map, NotImplemented(w, r, method, path)
→ 501 with that slice embedded as `spec`.
- controllers/projects.go: each of 7 handlers is now a one-liner:
apispec.NotImplemented(w, r, "GET", "/api/v1/projects").
- /api/v1/openapi.yaml serves the embedded document so tooling
(SDK gen, the validator slated for #19, dashboard dev tools) can
fetch the whole spec from the same origin as the routes.
- stubs/ package deleted.
When a real handler lands, only the apispec.NotImplemented line goes
away — nothing else does. The spec stays as documentation; consumers
never had to know it was throwaway. #19 (OpenAPI follow-up) is now
half-folded into this PR; the validation middleware remains its own
follow-up.
Tests reshaped: assert envelope + spec.operationId + spec.x-replaces
(replaces the old planned.legacy assertion); add TestOpenAPIYAMLServed
to cover the static spec serve; add apispec_test.go for embed/lookup
behaviour.
* refactor(api): move projects contract to internal/project package — #20
Pilots the feature-package layout the backend is migrating toward: a
resource's inbound interface and its DTOs live with the resource, not in
a central ports/ catch-all.
WHY: review flagged ports/ as vague. It conflates three jobs — the
outbound capability seam (legit), single-impl inbound interfaces (Go
idiom wants these consumer-side), and DTOs that aren't ports at all.
This moves the projects contract out as the reference shape #21/#22
follow; the merged session/lifecycle/outbound contracts are left
untouched and migrated separately.
Scope: INTERFACE ONLY. No implementation — handlers still answer via
apispec.NotImplemented and the injected project.Manager stays nil. The
impl lands in a later handler-impl PR.
Changes:
- new internal/project: project.go (Manager interface, 7 endpoints) +
dto.go (AddInput/GetResult/UpdateConfigInput/RemoveResult/ReloadResult,
moved verbatim from ports/projects.go, Project-prefix dropped).
- ports/projects.go deleted; ProjectManager removed from ports/inbound.go.
outbound.go and facts.go untouched.
- controllers/projects.go and httpd/api.go depend on project.Manager.
Domain entities (Project, ProjectSummary, DegradedProject, config types)
stay in domain/ as shared vocabulary.
go build/vet/test/gofmt all clean; no behavioural change.
* refactor(api): consolidate project types into internal/project — #20
Addresses PR review: (1) "why are config_types required at the moment?"
and (2) "project objects already defined in project/ — how do we
differentiate?"
Both had the same root cause: project types were split across domain/
and project/. Fix — keep ALL project types in the project package; only
domain.ProjectID (shared with sessions/lifecycle/workspace) stays in
domain.
- domain/project.go → project/types.go: Project, Summary, Degraded
(renamed from ProjectSummary/DegradedProject; the package name carries
the "Project" prefix now).
- domain/config_types.go deleted. Kept only the 4 shapes the projects
API actually exposes — TrackerConfig, SCMConfig, SCMWebhookConfig,
ReactionConfig — moved into project/types.go. Dropped AgentConfig,
AgentPermission, RoleAgentConfig, LocalProjectConfig (zero references)
and the speculative `Extra map[string]any` passthrough fields (no
marshaller existed, so they silently dropped data — premature).
- project/dto.go + project/project.go reference the local types; ids
stay domain.ProjectID.
Net: one home for project types, no dead code. go build/vet/test/gofmt
clean; no behavioural change (handlers still 501 via apispec).
* feat(api): implement project routes with mock manager/store
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* merge: resolve conflicts with origin/main
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* refactor(httpd): share JSON/API error envelope helpers
* fix(api): align project mock store with sqlite schema
* fix(api): address project API review semantics
* canonicalize both paths with filepath.EvalSymlinks before comparing
* style(project): gofmt git repo validation
---------
Co-authored-by: Aditi Chauhan <aditi1178@gmail.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Vaibhaav <user@example.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.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>
yyovil
neversettle
prateek
itrytoohard
Dhruv Sharma
Pritom14
Vaibhaav Tiwari