diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index be52f40..5a3bca7 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,5 +1,5 @@ -{"id":"forgejo-mcp-broker-8ei","title":"Phase 1: internal/httpserver with /healthz and graceful shutdown","description":"Implement internal/httpserver: constructs a *http.Server bound to cfg.Listen, mounts GET /healthz (returns 200 with JSON build-info from the build-info package, including version, git revision, build date, and current store status), handles SIGTERM/SIGINT by initiating graceful shutdown with a configurable deadline (default 10s). Uses log/slog for structured JSON logs. Exposes a Run(ctx) error method that blocks until shutdown completes.","acceptance_criteria":"go test ./internal/httpserver passes; GET /healthz returns expected JSON; sending SIGTERM causes Run to return nil within 2 seconds after in-flight requests complete; slow in-flight request is allowed to finish within the deadline, then forcibly closed.","status":"in_progress","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:20Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:24:09Z","started_at":"2026-04-24T15:24:09Z","dependencies":[{"issue_id":"forgejo-mcp-broker-8ei","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:19Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} -{"id":"forgejo-mcp-broker-t37","title":"Phase 1: wire cmd/broker/main.go and integration test","description":"Final phase 1 task: wire config → log → store → httpserver in cmd/broker/main.go. Parse config, init slog, open store, start httpserver, wait for shutdown signal, close store, exit. Add an integration test under cmd/broker/ that builds the binary, runs it with a valid env + temp store path, curls /healthz, sends SIGTERM, verifies clean exit within 2s. This is the acceptance gate for phase 1.","acceptance_criteria":"make build; make test (incl. integration) pass; running the binary with missing config fails with a clear error; running with valid config serves /healthz; SIGTERM shuts down cleanly within 2s; /healthz JSON includes version, git revision, build date, and store OK status.","status":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:20Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:46:20Z","dependencies":[{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-8ei","type":"blocks","created_at":"2026-04-24T16:48:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-9jh","type":"blocks","created_at":"2026-04-24T16:48:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-9nq","type":"blocks","created_at":"2026-04-24T16:48:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:48:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":4,"dependent_count":0,"comment_count":0} +{"id":"forgejo-mcp-broker-8ei","title":"Phase 1: internal/httpserver with /healthz and graceful shutdown","description":"Implement internal/httpserver: constructs a *http.Server bound to cfg.Listen, mounts GET /healthz (returns 200 with JSON build-info from the build-info package, including version, git revision, build date, and current store status), handles SIGTERM/SIGINT by initiating graceful shutdown with a configurable deadline (default 10s). Uses log/slog for structured JSON logs. Exposes a Run(ctx) error method that blocks until shutdown completes.","acceptance_criteria":"go test ./internal/httpserver passes; GET /healthz returns expected JSON; sending SIGTERM causes Run to return nil within 2 seconds after in-flight requests complete; slow in-flight request is allowed to finish within the deadline, then forcibly closed.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:20Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:26:43Z","started_at":"2026-04-24T15:24:09Z","closed_at":"2026-04-24T15:26:43Z","close_reason":"httpserver shipped: /healthz with store probe, graceful shutdown with force-close fallback, ExtraHandler extension point. 97.9% coverage. internal/log also implemented in the same commit (100% coverage).","dependencies":[{"issue_id":"forgejo-mcp-broker-8ei","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:19Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} +{"id":"forgejo-mcp-broker-t37","title":"Phase 1: wire cmd/broker/main.go and integration test","description":"Final phase 1 task: wire config → log → store → httpserver in cmd/broker/main.go. Parse config, init slog, open store, start httpserver, wait for shutdown signal, close store, exit. Add an integration test under cmd/broker/ that builds the binary, runs it with a valid env + temp store path, curls /healthz, sends SIGTERM, verifies clean exit within 2s. This is the acceptance gate for phase 1.","acceptance_criteria":"make build; make test (incl. integration) pass; running the binary with missing config fails with a clear error; running with valid config serves /healthz; SIGTERM shuts down cleanly within 2s; /healthz JSON includes version, git revision, build date, and store OK status.","status":"in_progress","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:20Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:27:58Z","started_at":"2026-04-24T15:27:58Z","dependencies":[{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-8ei","type":"blocks","created_at":"2026-04-24T16:48:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-9jh","type":"blocks","created_at":"2026-04-24T16:48:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-9nq","type":"blocks","created_at":"2026-04-24T16:48:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:48:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":4,"dependent_count":0,"comment_count":0} {"id":"forgejo-mcp-broker-9jh","title":"Phase 1: internal/store with SQLite open and embedded schema migrations","description":"Implement internal/store: wraps a modernc.org/sqlite connection, applies embedded schema migrations in order via a schema_migrations table, exposes a *sql.DB and a Close method. Phase 1 schema is just the migrations table itself plus a health_check row — real tables (clients, auth_codes, access_tokens, refresh_tokens) ship in phase 2. Store_path from config; creates parent dirs if missing; fails fast on unwritable path. Migrations embedded via embed.FS under internal/store/migrations/.","acceptance_criteria":"go test ./internal/store passes; opening a fresh db file applies migrations; re-opening is idempotent (no re-application, no errors); corrupt/locked files yield a clear error; Close() leaves no file handles open.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:22:53Z","started_at":"2026-04-24T15:11:36Z","closed_at":"2026-04-24T15:22:53Z","close_reason":"Store package shipped: modernc.org/sqlite, embed.FS migrations, WAL + FK pragmas, idempotent reopen, 90.1% coverage including bad-SQL rollback and record-step PK conflict.","dependencies":[{"issue_id":"forgejo-mcp-broker-9jh","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:19Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-9nq","title":"Phase 1: internal/config package with flag + env parsing and validation","description":"Implement internal/config: a Config struct populated from CLI flags and environment variables (flags win), with validation at startup. Parse public-url, listen addr, forgejo-url, forgejo-oauth-client-id/secret/scopes, forgejo-mcp-binary, store-path, max-sessions, session-idle-timeout, debug. Validation: required fields present and non-empty; public-url parses as an https URL; store-path writable; idle-timeout positive; max-sessions positive. Unit tests cover happy path + every validation error branch.","acceptance_criteria":"go test ./internal/config passes with \u003e=90% coverage; missing required env produces a clear error message listing all missing fields; flag values override env; --help prints all config options.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:10:26Z","started_at":"2026-04-24T15:05:57Z","closed_at":"2026-04-24T15:10:26Z","close_reason":"Config package shipped with 94.1% coverage, handles all required-vs-optional fields, env/flag precedence, URL validation with loopback-http exception, and store-path writability. Full test suite green.","dependencies":[{"issue_id":"forgejo-mcp-broker-9nq","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:18Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-n84","title":"Phase 1: bootstrap Go project layout","description":"Set up the Go project skeleton so all subsequent phase 1 packages have somewhere to live. Initialize go.mod with module path kode.naiv.no/olemd/forgejo-mcp-broker, create the directory layout (cmd/broker, internal/config, internal/log, internal/store, internal/httpserver), add a Makefile with build/test/lint targets, and wire build-info injection (version, git revision, build date) via -ldflags.","acceptance_criteria":"go.mod present with correct module path; make build produces ./fjmcp-broker binary; make test and make lint targets exist and pass against an empty codebase; binary prints --version with injected build info.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:45:44Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:55:39Z","started_at":"2026-04-24T14:50:57Z","closed_at":"2026-04-24T14:55:39Z","close_reason":"Bootstrap complete: go.mod, Makefile, directory layout, ldflags-injected build info, --version flag all working. make build/test/lint pass.","dependency_count":0,"dependent_count":4,"comment_count":0} diff --git a/cmd/broker/main.go b/cmd/broker/main.go index 2bdb185..5f3a76e 100644 --- a/cmd/broker/main.go +++ b/cmd/broker/main.go @@ -1,31 +1,96 @@ // Command fjmcp-broker is an OAuth 2.1 authorization server and MCP session -// broker that fronts forgejo-mcp. See ../../README.md and ../../docs/ for the -// design. +// broker that fronts forgejo-mcp. See ../../README.md and ../../docs/ for +// the design. package main import ( + "context" + "errors" "flag" "fmt" + "io" "os" + "os/signal" + "syscall" "kode.naiv.no/olemd/forgejo-mcp-broker/internal/buildinfo" + "kode.naiv.no/olemd/forgejo-mcp-broker/internal/config" + "kode.naiv.no/olemd/forgejo-mcp-broker/internal/httpserver" + brokerlog "kode.naiv.no/olemd/forgejo-mcp-broker/internal/log" + "kode.naiv.no/olemd/forgejo-mcp-broker/internal/store" +) + +// Exit codes follow the usual convention: 0 success, 2 config/usage, 1 runtime. +const ( + exitSuccess = 0 + exitRuntime = 1 + exitConfig = 2 ) func main() { - var showVersion bool - flag.BoolVar(&showVersion, "version", false, "print build info and exit") - flag.Parse() + os.Exit(run(os.Args[1:], os.Stderr)) +} - if showVersion { - fmt.Printf("fjmcp-broker %s (rev %s, built %s)\n", - buildinfo.Version, buildinfo.GitRevision, buildinfo.BuildDate) - return +// run is the testable entry point. Parses config, wires dependencies, and +// blocks until the HTTP server exits or a shutdown signal arrives. Returns +// an OS exit code. +func run(args []string, out io.Writer) int { + // --version is handled before config.Load so operators can inspect a + // binary without providing the rest of the required config. + for _, a := range args { + if a == "--version" || a == "-version" { + fmt.Fprintf(out, "fjmcp-broker %s (rev %s, built %s)\n", + buildinfo.Version, buildinfo.GitRevision, buildinfo.BuildDate) + return exitSuccess + } } - // Full startup wiring (config → log → store → httpserver) lands in - // forgejo-mcp-broker-t37. Until then this binary only serves --version - // so the bootstrap acceptance criteria can be exercised. - fmt.Fprintln(os.Stderr, "fjmcp-broker: runtime wiring not yet implemented (phase 1 in progress)") - fmt.Fprintln(os.Stderr, "Use --version to print build info.") - os.Exit(2) + cfg, err := config.Load(args, out) + switch { + case errors.Is(err, flag.ErrHelp): + return exitSuccess + case err != nil: + fmt.Fprintln(out, "fjmcp-broker: config error:") + fmt.Fprintln(out, err.Error()) + return exitConfig + } + + logger := brokerlog.New(out, cfg.Debug) + logger.Info("starting broker", + "listen", cfg.Listen, + "public_url", cfg.PublicURL, + "forgejo_url", cfg.ForgejoURL, + "store_path", cfg.StorePath, + "max_sessions", cfg.MaxSessions, + "idle_timeout", cfg.SessionIdleTimeout.String(), + ) + + // Signal handling is owned here; the HTTP server just responds to ctx + // cancellation. This keeps internal/httpserver free of signal coupling + // and makes it testable without any OS-level wiring. + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + st, err := store.Open(ctx, cfg.StorePath) + if err != nil { + logger.Error("open store", "err", err.Error()) + return exitRuntime + } + defer func() { + if err := st.Close(); err != nil { + logger.Error("close store", "err", err.Error()) + } + }() + + srv := &httpserver.Server{ + Addr: cfg.Listen, + Log: logger, + Store: st, + } + if err := srv.Run(ctx); err != nil { + logger.Error("server exit", "err", err.Error()) + return exitRuntime + } + logger.Info("broker stopped") + return exitSuccess } diff --git a/cmd/broker/main_integration_test.go b/cmd/broker/main_integration_test.go new file mode 100644 index 0000000..8a52bb0 --- /dev/null +++ b/cmd/broker/main_integration_test.go @@ -0,0 +1,182 @@ +package main_test + +import ( + "encoding/json" + "fmt" + "io" + "net" + "net/http" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" +) + +// Binary path shared across tests — built once in TestMain to keep the +// integration suite fast. +var binPath string + +func TestMain(m *testing.M) { + dir, err := os.MkdirTemp("", "fjmcp-broker-bin-*") + if err != nil { + fmt.Fprintln(os.Stderr, "integration: mkdir:", err) + os.Exit(1) + } + defer os.RemoveAll(dir) + + binPath = filepath.Join(dir, "fjmcp-broker") + build := exec.Command("go", "build", "-o", binPath, ".") + build.Stderr = os.Stderr + if err := build.Run(); err != nil { + fmt.Fprintln(os.Stderr, "integration: build:", err) + os.Exit(1) + } + os.Exit(m.Run()) +} + +func TestBinary_Version(t *testing.T) { + out, err := exec.Command(binPath, "--version").CombinedOutput() + if err != nil { + t.Fatalf("--version should exit 0: %v (output: %s)", err, out) + } + if !strings.Contains(string(out), "fjmcp-broker") { + t.Errorf("version output missing binary name: %s", out) + } +} + +func TestBinary_MissingConfig_FailsWithClearError(t *testing.T) { + cmd := exec.Command(binPath) + // Reset env so the binary sees no ambient config. + cmd.Env = []string{"PATH=" + os.Getenv("PATH")} + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("binary should exit nonzero with no config") + } + // Every required field should be mentioned, not just the first. + for _, want := range []string{ + "public-url", "forgejo-url", + "forgejo-oauth-client-id", "forgejo-oauth-client-secret", + } { + if !strings.Contains(string(out), want) { + t.Errorf("config error should mention %q, got:\n%s", want, out) + } + } +} + +func TestBinary_Health_And_SigtermShutsDownCleanly(t *testing.T) { + addr := freePort(t) + storePath := filepath.Join(t.TempDir(), "broker.db") + + cmd := exec.Command(binPath, + "--public-url", "http://localhost:1234", + "--forgejo-url", "https://forgejo.example.com", + "--forgejo-oauth-client-id", "test-id", + "--forgejo-oauth-client-secret", "test-secret", + "--store-path", storePath, + "--listen", addr, + ) + cmd.Env = []string{"PATH=" + os.Getenv("PATH")} + // Capture stderr so test failures surface the broker's logs. + stderrR, stderrW := io.Pipe() + cmd.Stderr = stderrW + var capturedStderr strings.Builder + go func() { + _, _ = io.Copy(&capturedStderr, stderrR) + }() + + if err := cmd.Start(); err != nil { + t.Fatalf("start broker: %v", err) + } + // Ensure we always try to reap the process. + defer func() { + if cmd.ProcessState == nil && cmd.Process != nil { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + } + _ = stderrW.Close() + }() + + waitListening(t, addr, 5*time.Second) + + resp, err := http.Get("http://" + addr + "/healthz") + if err != nil { + t.Fatalf("GET /healthz: %v", err) + } + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("/healthz status = %d, want 200\nbody: %s", resp.StatusCode, body) + } + + var h map[string]string + if err := json.Unmarshal(body, &h); err != nil { + t.Fatalf("/healthz body not JSON: %v\nbody: %s", err, body) + } + for _, k := range []string{"status", "version", "git_revision", "build_date", "store"} { + if h[k] == "" { + t.Errorf("/healthz missing field %q: %v", k, h) + } + } + if h["status"] != "ok" { + t.Errorf("/healthz status = %q, want ok (body: %s)", h["status"], body) + } + if h["store"] != "ok" { + t.Errorf("/healthz store = %q, want ok (body: %s)", h["store"], body) + } + + // Send SIGTERM and verify clean exit. + start := time.Now() + if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatalf("signal SIGTERM: %v", err) + } + + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + + select { + case err := <-done: + if err != nil { + t.Errorf("broker exit with error: %v\nstderr:\n%s", err, capturedStderr.String()) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("shutdown took %s, want < 2s", elapsed) + } + case <-time.After(3 * time.Second): + _ = cmd.Process.Kill() + t.Fatal("broker did not exit within 3s of SIGTERM") + } +} + +// freePort returns a 127.0.0.1: the kernel assigned. The listener is +// closed immediately, so there's a tiny race window before the broker +// rebinds — acceptable on loopback. +func freePort(t *testing.T) string { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("pick free port: %v", err) + } + addr := l.Addr().String() + _ = l.Close() + return addr +} + +// waitListening polls the target until a TCP dial succeeds or the deadline +// expires. Serves as a sync barrier for "the binary is up". +func waitListening(t *testing.T, addr string, within time.Duration) { + t.Helper() + deadline := time.Now().Add(within) + for time.Now().Before(deadline) { + c, err := net.DialTimeout("tcp", addr, 100*time.Millisecond) + if err == nil { + _ = c.Close() + return + } + time.Sleep(25 * time.Millisecond) + } + t.Fatalf("broker did not start listening on %s within %s", addr, within) +}