Serve a "cortex" context in-process, eliminating the daemon for CLI use - #41
Merged
Conversation
A context pointed at a local `rossoctl cortex serve` only worked while that
daemon was running. Forget to start it and every command failed with connection
refused, which is a confusing way to learn that a background process is a
prerequisite for reading state that is sitting on the local disk.
The daemon turns out to be unnecessary for the CLI's own commands, and the
reason is a property of `serve` rather than a trick: it holds no in-memory
state. Every request re-reads the namespaces directory through instances.List
and instances.Get, so a running daemon is a pure function of the filesystem.
Anything that can read that directory can compute the same answers, and the
command already can. The daemon remains the right tool for its actual job,
which is letting something other than rossoctl — a web UI — reach a local
cortex over HTTP.
So a context named "cortex" is now answered by the same handlers, called
directly in the command's own process. internal/inprocess provides an
http.RoundTripper over an http.Handler, and rossoctlclient.NewClient installs
it on apiclient.Client.HTTPClient. Two seams made this cheap and both already
invited it: apiclient.Client exposes HTTPClient, and serve.Server.Handler is
documented as being "for tests and for callers that want to wrap or embed
them." There is exactly one apiclient.Client construction site in the repo, so
substituting the transport there reaches all thirteen command call sites.
This deliberately does not add a second implementation of the fifteen-method
Rossoctl interface. That was the file-backed cortex client already removed from
this repo, and re-adding it would undo that simplification and let the two
paths diverge in exactly the ways that are hardest to notice. One
implementation, one set of handlers, one place a bug can live.
Dispatch is on the context's name being exactly "cortex". No new flag, and
anyone who already has such a context gets the benefit with no config change.
The accepted cost is that a context named "cortex" pointed at a remote server
is answered locally instead. Since that is a dispatch decision rather than
advice, it is mitigated where a user would look: --verbose names the transport
on stderr, and the help for both `cortex` and `cortex serve` states the rule.
A test asserts the notice appears on stderr and not on stdout, so output stays
parseable.
There is no fallback in either direction, which is a decision and not an
omission. "Dial first, fall back in-process" would make results depend on
whether an unrelated daemon happened to be up, and cannot be done coherently at
the transport layer anyway: `agents get` issues two requests, and a daemon that
stopped between them would yield one command answered by two backends. An
explicit --server always dials, because cmd/root.go builds a synthetic context
with no name and so can never match. That was load-bearing by accident; a test
now pins it against a live server, asserting the request was really dialed.
A non-2xx response is not a transport error. RoundTrip returns an error only
for a misuse it cannot serve — a nil handler or a nil URL. A 500 UNIMPLEMENTED
is a response, and flattening it into a transport error would make the
in-process path report a different kind of failure than the daemon for the same
route. That equivalence is the whole claim being made, so a test drives a 500
through a real apiclient.Client and requires it to surface as *StatusError with
StatusCode 500. Pre-existing UNIMPLEMENTED routes stay UNIMPLEMENTED: `status`
still fails, and a test pins that rather than treating it as a bug to fix here.
A handler panic is not recovered, also deliberately. net/http recovers
per-connection so that one client cannot kill a server shared with others; here
the server and the client are the same process running one command, so a panic
should produce a stack trace rather than a synthesized 500 that hides the bug
under a plausible-looking response.
The RoundTripper contract has obligations that fail silently when broken, so
each is handled explicitly and tested. The request body is closed on every
path, including both error returns — the deferred close comes first, before any
validation, because the error paths are the ones that regress. The caller's
request is never modified: the handler gets a Clone, which matters because
ServeMux stores path values on the request it dispatches and serve's detail
handlers read them. Server-side fields a socket would have supplied are filled
in: RequestURI, RemoteAddr, and a nil Body normalized to http.NoBody.
httptest.NewRecorder captures the response. It is stdlib, so no new dependency,
but it is a new precedent — no production file here imported httptest before,
and the comment says why it earns the exception. It gets right several details
a hand-rolled ResponseWriter gets wrong, including defaulting the status to 200
when a handler writes a body without calling WriteHeader; the alternative is
about forty lines of subtle code whose only benefit is avoiding an import with
"test" in the name.
The mount path is derived the way apiclient.resolve derives its base: append a
trailing slash if absent, parse, take the path. Getting this wrong would 404
everything, so a table covers all four spellings of the server URI, plus the
root mount, the empty string, and a malformed URI. serve.SplitAddress is not
reused — it rejects anything containing "://" because it parses the --address
flag's host:port/path language, a different input.
`namespaces list` now reports the namespace directories that exist on disk
rather than the daemon's hardcoded team1,team2 default. This is a visible,
intended difference, and it makes `namespaces list` and `agents list`
consistent by construction instead of by coincidence.
That consistency is why `config create-context --name cortex` now also creates
the team1 and team2 namespace directories and names them on stdout. Without
it, a freshly created cortex context would report no namespaces at all, and its
own --namespace would name a directory that did not exist. The list comes from
`cortex serve`'s own flag default, so the two cannot drift. Seeding runs after
the config is saved, so a failure leaves a usable context rather than
discarding one already reported as created, and it is reported rather than
ignored, since the directories are what make the namespaces selectable. The new
instances.CreateNamespace is idempotent because callers use it to assert a
namespace exists rather than to claim it, and it uses the same 0700 as Create
for the same reason: what lands in there names ports serving an unauthenticated
session API.
Instance records move from ~/.config/rossocortex/namespaces to
~/.config/rossoctl/namespaces. The old location was justified by sitting beside
the rossoctl directory rather than inside it; that reasoning does not survive
the config file and the records being two halves of one tool's state, and a
user backing up or clearing that state should have one directory to handle. The
change is one line in instances.BaseDir, with the rest being documentation and
tests. XDG_CONFIG_HOME remains the only override.
GET /health and GET /ready are implemented, returning {"status":"healthy"} and
{"status":"ready"}. Both were placeholders answering 500 while `cortex serve
--help` already advertised them as served — the help was accurate about intent
and wrong about behavior. One Health type serves both responses because they
differ only in that word: liveness and readiness are distinct questions in
general, but this server answers them from the same state, having no
connections to warm and no caches to fill. Readiness is unconditional and
notably does not check that any instance records exist, since a cortex with
nothing running is serving an empty list rather than being unready.
Three documentation claims were wrong and are corrected: the serve package doc
said six routes were real while omitting the agent-card route, the count is now
nine, and the daemon's startup message said "other operations return 500
UNIMPLEMENTED" without mentioning the probes it had just started serving.
One existing test had to change meaning rather than merely be updated.
TestCortexNamespacesAreRealDirectories proved namespaces came from disk by
asserting team2 was absent, and seeding now creates team2 for real, so that
premise is gone. Deleting the check would have quietly dropped the coverage, so
it is replaced by TestCortexNamespacesIgnoreTheDaemonDefault, which writes the
context directly to bypass seeding and then requires that neither default name
appear. It was confirmed to be a real guard by injecting a hardcoded
team1,team2 into inprocess.New and observing the failure.
An unrelated pre-existing bug surfaced while running the suite.
TestSummaryWireShapeMatchesClientType stubbed getter while the list handlers
read lister, so it was reading the developer's own namespaces directory and
passed only on machines that happened to have a2a instances recorded. It now
stubs lister.
internal/serve and internal/apiclient are otherwise unchanged; both seams were
already public. cmd/ui.go is untouched, so `ui open` still opens the context's
server URL — which is why that field stays meaningful even in-process: its path
supplies the mount point and its scheme and host are the UI target.
Verification: go build, go vet, and gofmt are clean, the full suite passes, and
internal/inprocess and cmd pass under -race. Parity was measured rather than
assumed: with three instance records under an isolated XDG_CONFIG_HOME, seven
commands were run in-process with nothing listening and again via --server
against a live daemon, and the outputs diffed to exactly two lines, both
accounted for by the intended namespaces difference. The 404 and 500
UNIMPLEMENTED bodies were byte-identical. The probes were checked at the root
and the mount path, in-process and through the daemon; a root-mounted server
correctly 404s /api/v1/health. The daemon still starts and serves, a
differently-named context and an explicit --server still dial and still fail
against a dead port, and staticcheck was not run, as the installed build is Go
1.25 while this module requires 1.26.4.
Assisted by Claude.
Signed-off-by: Ed Snible <snible@us.ibm.com>
1 task
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
1 participant
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.
What
A context named
cortexis now answered byinternal/serve's handlers called directly in the command's own process, sorossoctl cortex serveis no longer a prerequisite for using the CLI against a local cortex. The daemon remains for its actual job: letting something other than rossoctl — a web UI — reach a local cortex over HTTP.Why this is possible
serveholds no in-memory state — every request re-reads the namespaces directory throughinstances.List/instances.Get. A running daemon is a pure function of the filesystem, so the command can compute the same answers itself.Two existing seams made it cheap, and both already invited it:
apiclient.ClientexposesHTTPClient, andserve.Server.Handleris documented as being "for tests and for callers that want to wrap or embed them." There is exactly oneapiclient.Clientconstruction site, so substituting the transport there reaches all thirteen command call sites.This deliberately does not add a second implementation of the fifteen-method
Rossoctlinterface — that was the file-backed cortex client already removed from this repo, and re-adding it would let the two paths diverge.Design decisions
Dispatch is on the context name being exactly
cortex. No new flag, and an existingcortexcontext gets the benefit with no config change. The accepted cost: acortexcontext pointed at a remote server is answered locally. Since that is a dispatch decision rather than advice, it is mitigated where a user would look —--verbosenames the transport on stderr, and the help for bothcortexandcortex servestates the rule.No fallback in either direction. "Dial first, fall back in-process" would make results depend on whether an unrelated daemon happened to be up, and cannot be done coherently at the transport layer anyway:
agents getissues two requests, and a daemon that stopped between them would yield one command answered by two backends. An explicit--serveralways dials, becausecmd/root.gobuilds a synthetic context with no name — that was load-bearing by accident and is now pinned by a test.A non-2xx is not a transport error.
RoundTriperrors only on a misuse it cannot serve (nil handler, nil URL). Flattening a 500 UNIMPLEMENTED into a transport error would make the in-process path report a different kind of failure than the daemon for the same route, and that equivalence is the whole claim. Pre-existing UNIMPLEMENTED routes stay that way —statusstill fails, pinned by a test rather than fixed here.A handler panic is not recovered.
net/httprecovers per-connection so one client cannot kill a shared server; here the server and client are the same process running one command, so a panic should produce a stack trace rather than a synthesized 500 hiding the bug.httptest.NewRecordercaptures the response. Stdlib, so no new dependency, but a new precedent — no production file here importedhttptestbefore, and a comment says why it earns the exception.Also in this PR
These interlock with the above rather than being drive-by changes.
namespaces listreports real directories instead of the daemon's hardcodedteam1,team2. A visible, intended difference; it makesnamespaces listandagents listconsistent by construction.create-context --name cortexseedsteam1andteam2. Follows from the above: otherwise a fresh cortex context reports no namespaces and its own--namespacenames a directory that does not exist. The list comes fromcortex serve's flag default so the two cannot drift; seeding runs after the config is saved so a failure leaves a usable context.~/.config/rossoctl/namespaces(fromrossocortex). One line ininstances.BaseDir; the config file and the records are two halves of one tool's state, so backing up or clearing it should mean one directory.XDG_CONFIG_HOMEremains the only override.GET /healthandGET /readyimplemented, returning{"status":"healthy"}/{"status":"ready"}. Both were placeholders answering 500 whilecortex serve --helpalready advertised them — the help was right about intent and wrong about behavior.servepackage doc said six routes were real while omitting agent-card (now nine), and the daemon's startup message omitted the probes it had just started serving.Reviewer notes
One test changed meaning, not just content.
TestCortexNamespacesAreRealDirectoriesproved namespaces came from disk by assertingteam2was absent — seeding now createsteam2, so the premise is gone. Rather than drop the coverage,TestCortexNamespacesIgnoreTheDaemonDefaultwrites the context directly to bypass seeding and requires neither default name to appear. Confirmed to be a real guard by injecting a hardcodedteam1,team2intoinprocess.Newand watching it fail.An unrelated pre-existing bug surfaced.
TestSummaryWireShapeMatchesClientTypestubbedgetterwhile the list handlers readlister, so it read the developer's own namespaces directory and passed only on machines that happened to have a2a instances recorded. Now stubslister.internal/serveandinternal/apiclientare otherwise unchanged — both seams were already public.cmd/ui.gois untouched, soui openstill opens the context's server URL; that is why the field stays meaningful in-process, supplying the mount path and the UI target.Verification
go build,go vet,gofmtclean; full suite passes;internal/inprocessandcmdpass under-race.Parity was measured, not assumed. With three instance records under an isolated
XDG_CONFIG_HOME, seven commands ran in-process with nothing listening and again via--serveragainst a live daemon. The outputs diffed to exactly two lines, both accounted for by the intended namespaces difference; the 404 and 500 UNIMPLEMENTED bodies were byte-identical.Probes checked at the root and the mount path, in-process and through the daemon; a root-mounted server correctly 404s
/api/v1/health. The daemon still starts and serves; a differently-named context and an explicit--serverstill dial and still fail against a dead port.staticcheck was not run — the installed build is Go 1.25 while this module requires 1.26.4.
Supersedes
Closes #40 (the connection-refused hint), which only explained the failure better instead of removing it.
Follow-ups (not this PR)
statusandtools getremain UNIMPLEMENTED inserve; now that the CLI can serve itself, real handlers for/auth/statusand/tools/{ns}/{name}would complete the local experience.--cortexincmd/cortex.gois parsed and never read. Wire it up or remove it.ROSSOCORTEX_CONFIG_DIRappears in a test comment and is read by nothing.🤖 Generated with Claude Code