diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index e05c3d8..b0b7ce4 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":"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-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-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":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:46:19Z","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":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:46:19Z","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-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":"in_progress","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:05:57Z","started_at":"2026-04-24T15:05:57Z","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/internal/config/config.go b/internal/config/config.go new file mode 100644 index 0000000..9aadb58 --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,257 @@ +// Package config loads broker configuration from environment variables and +// command-line flags, applies defaults, and validates the result. +// +// Precedence: flags win over environment variables. Empty environment values +// are treated as unset, so operators can neutralise an exported variable +// without needing to unset it. Required fields have no default and must be +// supplied one way or the other. Validation is strict — the process must not +// start with invalid config. +package config + +import ( + "errors" + "flag" + "fmt" + "io" + "net/url" + "os" + "path/filepath" + "strconv" + "time" +) + +// Default values for optional fields. Exposed so tests and callers can +// reference them without re-hardcoding. +const ( + DefaultListen = ":8080" + DefaultForgejoOAuthScopes = "read:user write:repository write:issue write:notification read:organization" + DefaultForgejoMCPBinary = "forgejo-mcp" + DefaultStorePath = "/data/broker.db" + DefaultMaxSessions = 100 + DefaultSessionIdleTimeout = 15 * time.Minute +) + +// Config holds fully resolved and validated broker configuration. +type Config struct { + PublicURL string + Listen string + ForgejoURL string + ForgejoOAuthClientID string + ForgejoOAuthClientSecret string + ForgejoOAuthScopes string + ForgejoMCPBinary string + StorePath string + MaxSessions int + SessionIdleTimeout time.Duration + Debug bool +} + +// Load resolves configuration from environment variables and the provided +// command-line arguments (typically os.Args[1:]), then validates the result. +// out receives flag-package output (usage text on -h / parse errors). +// +// Returns flag.ErrHelp when the user requested help — callers should treat +// that as a normal exit, not a configuration failure. +func Load(args []string, out io.Writer) (*Config, error) { + cfg, err := fromEnv() + if err != nil { + return nil, err + } + + fs := flag.NewFlagSet("fjmcp-broker", flag.ContinueOnError) + fs.SetOutput(out) + cfg.bindFlags(fs) + + if err := fs.Parse(args); err != nil { + return nil, err + } + if err := cfg.Validate(); err != nil { + return nil, err + } + return cfg, nil +} + +func fromEnv() (*Config, error) { + cfg := &Config{ + PublicURL: envOr("FJMCP_BROKER_PUBLIC_URL", ""), + Listen: envOr("FJMCP_BROKER_LISTEN", DefaultListen), + ForgejoURL: envOr("FORGEJO_URL", ""), + ForgejoOAuthClientID: envOr("FORGEJO_OAUTH_CLIENT_ID", ""), + ForgejoOAuthClientSecret: envOr("FORGEJO_OAUTH_CLIENT_SECRET", ""), + ForgejoOAuthScopes: envOr("FORGEJO_OAUTH_SCOPES", DefaultForgejoOAuthScopes), + ForgejoMCPBinary: envOr("FJMCP_BROKER_MCP_BINARY", DefaultForgejoMCPBinary), + StorePath: envOr("FJMCP_BROKER_STORE", DefaultStorePath), + MaxSessions: DefaultMaxSessions, + SessionIdleTimeout: DefaultSessionIdleTimeout, + } + + if v, ok := lookupNonEmpty("FJMCP_BROKER_MAX_SESSIONS"); ok { + n, err := strconv.Atoi(v) + if err != nil { + return nil, fmt.Errorf("FJMCP_BROKER_MAX_SESSIONS: %w", err) + } + cfg.MaxSessions = n + } + if v, ok := lookupNonEmpty("FJMCP_BROKER_IDLE_TIMEOUT"); ok { + d, err := time.ParseDuration(v) + if err != nil { + return nil, fmt.Errorf("FJMCP_BROKER_IDLE_TIMEOUT: %w", err) + } + cfg.SessionIdleTimeout = d + } + if v, ok := lookupNonEmpty("FJMCP_BROKER_DEBUG"); ok { + b, err := strconv.ParseBool(v) + if err != nil { + return nil, fmt.Errorf("FJMCP_BROKER_DEBUG: %w", err) + } + cfg.Debug = b + } + return cfg, nil +} + +func (c *Config) bindFlags(fs *flag.FlagSet) { + fs.StringVar(&c.PublicURL, "public-url", c.PublicURL, + "Public issuer URL, e.g. https://mcp.example.com (REQUIRED; env FJMCP_BROKER_PUBLIC_URL)") + fs.StringVar(&c.Listen, "listen", c.Listen, + "HTTP listen address (env FJMCP_BROKER_LISTEN)") + fs.StringVar(&c.ForgejoURL, "forgejo-url", c.ForgejoURL, + "Upstream Forgejo instance URL (REQUIRED; env FORGEJO_URL)") + fs.StringVar(&c.ForgejoOAuthClientID, "forgejo-oauth-client-id", c.ForgejoOAuthClientID, + "Forgejo OAuth client ID (REQUIRED; env FORGEJO_OAUTH_CLIENT_ID)") + fs.StringVar(&c.ForgejoOAuthClientSecret, "forgejo-oauth-client-secret", c.ForgejoOAuthClientSecret, + "Forgejo OAuth client secret (REQUIRED; env FORGEJO_OAUTH_CLIENT_SECRET)") + fs.StringVar(&c.ForgejoOAuthScopes, "forgejo-oauth-scopes", c.ForgejoOAuthScopes, + "Space-separated OAuth scopes requested from Forgejo (env FORGEJO_OAUTH_SCOPES)") + fs.StringVar(&c.ForgejoMCPBinary, "forgejo-mcp-binary", c.ForgejoMCPBinary, + "Path to the forgejo-mcp binary (env FJMCP_BROKER_MCP_BINARY)") + fs.StringVar(&c.StorePath, "store-path", c.StorePath, + "SQLite store file path (env FJMCP_BROKER_STORE)") + fs.IntVar(&c.MaxSessions, "max-sessions", c.MaxSessions, + "Maximum concurrent MCP sessions (env FJMCP_BROKER_MAX_SESSIONS)") + fs.DurationVar(&c.SessionIdleTimeout, "session-idle-timeout", c.SessionIdleTimeout, + "Idle timeout before a session subprocess is reaped (env FJMCP_BROKER_IDLE_TIMEOUT)") + fs.BoolVar(&c.Debug, "debug", c.Debug, + "Verbose logging (env FJMCP_BROKER_DEBUG)") +} + +// Validate checks that required fields are present and all values are +// internally consistent. Errors are aggregated so the operator sees every +// problem at once. +func (c *Config) Validate() error { + var errs []error + + if c.PublicURL == "" { + errs = append(errs, errors.New("public-url is required (--public-url or FJMCP_BROKER_PUBLIC_URL)")) + } + if c.ForgejoURL == "" { + errs = append(errs, errors.New("forgejo-url is required (--forgejo-url or FORGEJO_URL)")) + } + if c.ForgejoOAuthClientID == "" { + errs = append(errs, errors.New("forgejo-oauth-client-id is required (--forgejo-oauth-client-id or FORGEJO_OAUTH_CLIENT_ID)")) + } + if c.ForgejoOAuthClientSecret == "" { + errs = append(errs, errors.New("forgejo-oauth-client-secret is required (--forgejo-oauth-client-secret or FORGEJO_OAUTH_CLIENT_SECRET)")) + } + + if c.PublicURL != "" { + if err := validateIssuerURL("public-url", c.PublicURL); err != nil { + errs = append(errs, err) + } + } + if c.ForgejoURL != "" { + if err := validateIssuerURL("forgejo-url", c.ForgejoURL); err != nil { + errs = append(errs, err) + } + } + + if c.StorePath == "" { + errs = append(errs, errors.New("store-path must not be empty")) + } else if err := checkStorePathWritable(c.StorePath); err != nil { + errs = append(errs, fmt.Errorf("store-path: %w", err)) + } + + if c.Listen == "" { + errs = append(errs, errors.New("listen must not be empty")) + } + if c.MaxSessions <= 0 { + errs = append(errs, fmt.Errorf("max-sessions must be > 0, got %d", c.MaxSessions)) + } + if c.SessionIdleTimeout <= 0 { + errs = append(errs, fmt.Errorf("session-idle-timeout must be > 0, got %s", c.SessionIdleTimeout)) + } + + return errors.Join(errs...) +} + +// validateIssuerURL rejects malformed URLs, enforces http/https scheme, and +// disallows plain http for non-loopback hosts. Publishing an http issuer for +// a non-loopback host is a classic OAuth misconfiguration that undermines +// the whole point of TLS on the AS. +func validateIssuerURL(field, raw string) error { + u, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("%s: %w", field, err) + } + if u.Scheme != "http" && u.Scheme != "https" { + return fmt.Errorf("%s: scheme must be http or https, got %q", field, u.Scheme) + } + if u.Host == "" { + return fmt.Errorf("%s: missing host in %q", field, raw) + } + if u.Scheme == "http" && !isLoopback(u.Hostname()) { + return fmt.Errorf("%s: http scheme only allowed for loopback hosts, got %q", field, u.Hostname()) + } + return nil +} + +func isLoopback(host string) bool { + switch host { + case "localhost", "127.0.0.1", "::1": + return true + } + return false +} + +// checkStorePathWritable verifies the store's parent directory exists and +// the current process can create files in it. The store file itself is not +// created here — that is the storage layer's responsibility. +func checkStorePathWritable(path string) error { + abs, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("invalid path %q: %w", path, err) + } + dir := filepath.Dir(abs) + info, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("parent directory %q: %w", dir, err) + } + if !info.IsDir() { + return fmt.Errorf("parent %q is not a directory", dir) + } + f, err := os.CreateTemp(dir, ".fjmcp-broker-probe-*") + if err != nil { + return fmt.Errorf("parent directory %q not writable: %w", dir, err) + } + name := f.Name() + _ = f.Close() + _ = os.Remove(name) + return nil +} + +func envOr(key, fallback string) string { + if v, ok := lookupNonEmpty(key); ok { + return v + } + return fallback +} + +// lookupNonEmpty returns the env value and true only when the variable is +// set AND non-empty. An exported-but-empty variable is treated as unset, +// matching common shell usage. +func lookupNonEmpty(key string) (string, bool) { + v, ok := os.LookupEnv(key) + if !ok || v == "" { + return "", false + } + return v, true +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..aaccbd4 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,335 @@ +package config_test + +import ( + "bytes" + "errors" + "flag" + "io" + "path/filepath" + "strings" + "testing" + "time" + + "kode.naiv.no/olemd/forgejo-mcp-broker/internal/config" +) + +// envVars enumerates every environment variable read by the config package. +// Tests clear all of these to isolate from the ambient shell environment. +var envVars = []string{ + "FJMCP_BROKER_PUBLIC_URL", + "FJMCP_BROKER_LISTEN", + "FORGEJO_URL", + "FORGEJO_OAUTH_CLIENT_ID", + "FORGEJO_OAUTH_CLIENT_SECRET", + "FORGEJO_OAUTH_SCOPES", + "FJMCP_BROKER_MCP_BINARY", + "FJMCP_BROKER_STORE", + "FJMCP_BROKER_MAX_SESSIONS", + "FJMCP_BROKER_IDLE_TIMEOUT", + "FJMCP_BROKER_DEBUG", +} + +// clearEnv wipes every known env var for the test's lifetime. Setting to "" +// is sufficient because the config treats empty values as unset. +func clearEnv(t *testing.T) { + t.Helper() + for _, k := range envVars { + t.Setenv(k, "") + } +} + +// validBaseArgs returns the minimum required flag set, plus a writable +// store path pointing inside a per-test TempDir. +func validBaseArgs(t *testing.T) []string { + t.Helper() + storePath := filepath.Join(t.TempDir(), "broker.db") + return []string{ + "--public-url", "https://mcp.example.com", + "--forgejo-url", "https://forgejo.example.com", + "--forgejo-oauth-client-id", "client-id", + "--forgejo-oauth-client-secret", "client-secret", + "--store-path", storePath, + } +} + +func TestLoad_HappyPath_Defaults(t *testing.T) { + clearEnv(t) + cfg, err := config.Load(validBaseArgs(t), io.Discard) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Listen != config.DefaultListen { + t.Errorf("Listen = %q, want %q", cfg.Listen, config.DefaultListen) + } + if cfg.ForgejoOAuthScopes != config.DefaultForgejoOAuthScopes { + t.Errorf("ForgejoOAuthScopes = %q, want %q", cfg.ForgejoOAuthScopes, config.DefaultForgejoOAuthScopes) + } + if cfg.ForgejoMCPBinary != config.DefaultForgejoMCPBinary { + t.Errorf("ForgejoMCPBinary = %q, want %q", cfg.ForgejoMCPBinary, config.DefaultForgejoMCPBinary) + } + if cfg.MaxSessions != config.DefaultMaxSessions { + t.Errorf("MaxSessions = %d, want %d", cfg.MaxSessions, config.DefaultMaxSessions) + } + if cfg.SessionIdleTimeout != config.DefaultSessionIdleTimeout { + t.Errorf("SessionIdleTimeout = %s, want %s", cfg.SessionIdleTimeout, config.DefaultSessionIdleTimeout) + } + if cfg.Debug { + t.Error("Debug = true, want false by default") + } +} + +func TestLoad_Env_PopulatesAllFields(t *testing.T) { + clearEnv(t) + storePath := filepath.Join(t.TempDir(), "broker.db") + + t.Setenv("FJMCP_BROKER_PUBLIC_URL", "https://mcp.example.com") + t.Setenv("FJMCP_BROKER_LISTEN", ":9090") + t.Setenv("FORGEJO_URL", "https://forgejo.example.com") + t.Setenv("FORGEJO_OAUTH_CLIENT_ID", "envcid") + t.Setenv("FORGEJO_OAUTH_CLIENT_SECRET", "envcs") + t.Setenv("FORGEJO_OAUTH_SCOPES", "read:user") + t.Setenv("FJMCP_BROKER_MCP_BINARY", "/opt/forgejo-mcp") + t.Setenv("FJMCP_BROKER_STORE", storePath) + t.Setenv("FJMCP_BROKER_MAX_SESSIONS", "42") + t.Setenv("FJMCP_BROKER_IDLE_TIMEOUT", "5m") + t.Setenv("FJMCP_BROKER_DEBUG", "true") + + cfg, err := config.Load(nil, io.Discard) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := &config.Config{ + PublicURL: "https://mcp.example.com", + Listen: ":9090", + ForgejoURL: "https://forgejo.example.com", + ForgejoOAuthClientID: "envcid", + ForgejoOAuthClientSecret: "envcs", + ForgejoOAuthScopes: "read:user", + ForgejoMCPBinary: "/opt/forgejo-mcp", + StorePath: storePath, + MaxSessions: 42, + SessionIdleTimeout: 5 * time.Minute, + Debug: true, + } + if *cfg != *want { + t.Errorf("cfg mismatch\n got: %+v\nwant: %+v", *cfg, *want) + } +} + +func TestLoad_FlagBeatsEnv(t *testing.T) { + clearEnv(t) + storePath := filepath.Join(t.TempDir(), "broker.db") + + t.Setenv("FJMCP_BROKER_PUBLIC_URL", "https://env.example.com") + t.Setenv("FORGEJO_URL", "https://env-forgejo.example.com") + t.Setenv("FORGEJO_OAUTH_CLIENT_ID", "env-cid") + t.Setenv("FORGEJO_OAUTH_CLIENT_SECRET", "env-cs") + t.Setenv("FJMCP_BROKER_STORE", storePath) + t.Setenv("FJMCP_BROKER_MAX_SESSIONS", "10") + + cfg, err := config.Load([]string{ + "--public-url", "https://flag.example.com", + "--max-sessions", "77", + }, io.Discard) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.PublicURL != "https://flag.example.com" { + t.Errorf("PublicURL = %q, want flag value", cfg.PublicURL) + } + if cfg.MaxSessions != 77 { + t.Errorf("MaxSessions = %d, want 77", cfg.MaxSessions) + } + // Unoverridden fields should still come from env: + if cfg.ForgejoOAuthClientID != "env-cid" { + t.Errorf("ForgejoOAuthClientID = %q, want env-cid", cfg.ForgejoOAuthClientID) + } +} + +func TestLoad_EmptyEnvTreatedAsUnset(t *testing.T) { + // An exported-but-empty variable must not clobber the default. This matches + // the shell idiom where `export FOO=` is effectively a reset. + clearEnv(t) + t.Setenv("FJMCP_BROKER_LISTEN", "") // empty => use default + cfg, err := config.Load(validBaseArgs(t), io.Discard) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Listen != config.DefaultListen { + t.Errorf("Listen = %q, want default %q", cfg.Listen, config.DefaultListen) + } +} + +func TestLoad_MissingRequired_ListsAllFields(t *testing.T) { + clearEnv(t) + _, err := config.Load(nil, io.Discard) + if err == nil { + t.Fatal("expected error, got nil") + } + msg := err.Error() + for _, want := range []string{ + "public-url", + "forgejo-url", + "forgejo-oauth-client-id", + "forgejo-oauth-client-secret", + } { + if !strings.Contains(msg, want) { + t.Errorf("error message missing %q: %s", want, msg) + } + } +} + +func TestLoad_InvalidIssuerURL(t *testing.T) { + cases := []struct { + name string + url string + want string + }{ + {"bad_scheme", "ftp://example.com", "scheme must be http or https"}, + {"http_non_loopback", "http://example.com", "http scheme only allowed for loopback"}, + {"no_host", "http:///path", "missing host"}, + {"no_scheme", "example.com", "scheme must be http or https"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + clearEnv(t) + args := validBaseArgs(t) + // Replace the public-url value in args: + for i, a := range args { + if a == "--public-url" && i+1 < len(args) { + args[i+1] = tc.url + } + } + _, err := config.Load(args, io.Discard) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("want error containing %q, got %v", tc.want, err) + } + }) + } +} + +func TestLoad_IssuerURL_LoopbackHTTPAllowed(t *testing.T) { + // Local development should not require a TLS cert. Validate that the + // loopback escape hatch works for the three canonical forms. + for _, host := range []string{"http://localhost:8080", "http://127.0.0.1", "http://[::1]:9000"} { + t.Run(host, func(t *testing.T) { + clearEnv(t) + args := validBaseArgs(t) + for i, a := range args { + if a == "--public-url" && i+1 < len(args) { + args[i+1] = host + } + } + if _, err := config.Load(args, io.Discard); err != nil { + t.Errorf("loopback http should be allowed, got: %v", err) + } + }) + } +} + +func TestLoad_InvalidStorePath(t *testing.T) { + t.Run("empty", func(t *testing.T) { + clearEnv(t) + args := validBaseArgs(t) + for i, a := range args { + if a == "--store-path" && i+1 < len(args) { + args[i+1] = "" + } + } + _, err := config.Load(args, io.Discard) + if err == nil || !strings.Contains(err.Error(), "store-path") { + t.Errorf("want store-path error, got %v", err) + } + }) + t.Run("nonexistent_parent", func(t *testing.T) { + clearEnv(t) + args := validBaseArgs(t) + bogus := filepath.Join(t.TempDir(), "definitely", "not", "here", "broker.db") + for i, a := range args { + if a == "--store-path" && i+1 < len(args) { + args[i+1] = bogus + } + } + _, err := config.Load(args, io.Discard) + if err == nil || !strings.Contains(err.Error(), "store-path") { + t.Errorf("want store-path error, got %v", err) + } + }) +} + +func TestLoad_NumericBounds(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + {"max_sessions_zero", []string{"--max-sessions", "0"}, "max-sessions must be > 0"}, + {"max_sessions_negative", []string{"--max-sessions", "-5"}, "max-sessions must be > 0"}, + {"idle_timeout_zero", []string{"--session-idle-timeout", "0"}, "session-idle-timeout must be > 0"}, + {"idle_timeout_negative", []string{"--session-idle-timeout", "-10s"}, "session-idle-timeout must be > 0"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + clearEnv(t) + args := append(validBaseArgs(t), tc.args...) + _, err := config.Load(args, io.Discard) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("want %q, got %v", tc.want, err) + } + }) + } +} + +func TestLoad_BadEnvParse(t *testing.T) { + cases := []struct { + envKey string + value string + want string + }{ + {"FJMCP_BROKER_MAX_SESSIONS", "not-a-number", "FJMCP_BROKER_MAX_SESSIONS"}, + {"FJMCP_BROKER_IDLE_TIMEOUT", "forever", "FJMCP_BROKER_IDLE_TIMEOUT"}, + {"FJMCP_BROKER_DEBUG", "maybe", "FJMCP_BROKER_DEBUG"}, + } + for _, tc := range cases { + t.Run(tc.envKey, func(t *testing.T) { + clearEnv(t) + t.Setenv(tc.envKey, tc.value) + _, err := config.Load(validBaseArgs(t), io.Discard) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Errorf("want error containing %q, got %v", tc.want, err) + } + }) + } +} + +func TestLoad_Help_ListsAllFlagsAndEnvVars(t *testing.T) { + clearEnv(t) + var buf bytes.Buffer + _, err := config.Load([]string{"-help"}, &buf) + if !errors.Is(err, flag.ErrHelp) { + t.Fatalf("want flag.ErrHelp, got %v", err) + } + out := buf.String() + + wantFlags := []string{ + "-public-url", "-listen", "-forgejo-url", + "-forgejo-oauth-client-id", "-forgejo-oauth-client-secret", "-forgejo-oauth-scopes", + "-forgejo-mcp-binary", "-store-path", + "-max-sessions", "-session-idle-timeout", "-debug", + } + for _, f := range wantFlags { + if !strings.Contains(out, f) { + t.Errorf("help missing flag %q", f) + } + } + wantEnv := []string{ + "FJMCP_BROKER_PUBLIC_URL", "FORGEJO_URL", + "FORGEJO_OAUTH_CLIENT_ID", "FORGEJO_OAUTH_CLIENT_SECRET", + "FJMCP_BROKER_STORE", "FJMCP_BROKER_IDLE_TIMEOUT", + } + for _, e := range wantEnv { + if !strings.Contains(out, e) { + t.Errorf("help missing env var %q", e) + } + } +} diff --git a/internal/config/doc.go b/internal/config/doc.go deleted file mode 100644 index 90f26ef..0000000 --- a/internal/config/doc.go +++ /dev/null @@ -1,5 +0,0 @@ -// Package config loads broker configuration from flags and environment -// variables, applies defaults, and validates the result. -// -// Implementation lands in forgejo-mcp-broker-9nq. -package config