Scaffold Mission Control Go repository with plans, CI, and project stubs#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR scaffolds a new Mission Control Go service with a clean layered architecture, SQLite persistence, and embedded migrations. The scaffold provides foundational infrastructure including configuration, logging, HTTP server with health/readiness endpoints, and a CI pipeline to enforce code quality from the start.
Changes:
- Created a new Go module structure with clear package boundaries (cmd, internal/{app,config,domain,service,store,transport}, pkg/observability, docs)
- Implemented SQLite-backed persistence with Goose migrations and domain models for missions, tasks, and events
- Added HTTP server with Chi router, health/readiness endpoints, graceful shutdown, and a baseline unit test
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| mission-control/go.mod | Go module definition with dependencies for Chi, SQLite, Goose, Zap, and sqlx |
| mission-control/go.sum | Checksums for all direct and transitive dependencies |
| mission-control/cmd/mission-control/main.go | Service entrypoint with signal handling and app bootstrap |
| mission-control/internal/app/app.go | Application lifecycle management including initialization and graceful shutdown |
| mission-control/internal/config/config.go | Environment-based configuration with MC_ prefix and sensible defaults |
| mission-control/internal/domain/*.go | Domain models for Mission, Task, and Event with typed status enums |
| mission-control/internal/service/mission_service.go | Service layer with repository interface for dependency injection |
| mission-control/internal/store/sqlite/store.go | SQLite connection management and migration runner |
| mission-control/internal/store/sqlite/migrations/0001_init.sql | Initial schema with missions, tasks, events tables and foreign keys |
| mission-control/internal/transport/httpapi/server.go | Chi-based HTTP server with health and readiness endpoints |
| mission-control/internal/transport/httpapi/server_test.go | Unit test for healthz endpoint |
| mission-control/pkg/observability/logger.go | Zap logger factory with environment-based configuration |
| mission-control/.github/workflows/ci.yml | CI pipeline for format checking, vetting, testing, and building |
| mission-control/Makefile | Build automation targets for common development tasks |
| mission-control/.gitignore | Ignore patterns for binaries, SQLite files, and editor configs |
| mission-control/README.md | Project documentation with tech stack and quick start guide |
| mission-control/docs/*.md | Design, project plan, and implementation plan documents |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func readyz(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte(`{"status":"ready"}`)) | ||
| } |
There was a problem hiding this comment.
The readyz endpoint always returns success without checking actual readiness. According to Kubernetes conventions and the design docs mentioning "readiness checks for DB connectivity," this endpoint should verify that the database connection is alive (e.g., by performing a ping) before returning OK. Currently it provides no value over healthz.
| application, err := app.New(ctx) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Using panic for error handling in main is not ideal for production services. Consider using log.Fatal or fmt.Fprintf to stderr followed by os.Exit(1) for more graceful error reporting and to ensure deferred cleanup functions run where appropriate.
| - name: Tidy | ||
| run: go mod tidy | ||
|
|
||
| - name: Format check | ||
| run: | | ||
| files=$(gofmt -l .) | ||
| if [ -n "$files" ]; then | ||
| echo "Unformatted files:" | ||
| echo "$files" | ||
| exit 1 | ||
| fi | ||
|
|
||
| - name: Vet | ||
| run: go vet ./... | ||
|
|
||
| - name: Test | ||
| run: go test -race ./... | ||
|
|
||
| - name: Build | ||
| run: go build ./cmd/mission-control |
There was a problem hiding this comment.
The CI workflow should specify a working directory or change into the mission-control directory before running Go commands, since the go.mod file is in mission-control/ subdirectory. Without this, the workflow will fail to find the Go module. Add "working-directory: mission-control" to each step or use "defaults.run.working-directory".
| type Event struct { | ||
| ID string | ||
| MissionID string | ||
| TaskID string |
There was a problem hiding this comment.
The TaskID field in the Event struct should be a pointer (*string) to properly represent the nullable task_id column in the database schema. The migration defines task_id as TEXT (nullable), but the Go struct uses a non-nullable string which cannot represent NULL values.
| TaskID string | |
| TaskID *string |
| if err != nil { | ||
| return nil, fmt.Errorf("open sqlite: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
SQLite connection should enable foreign key constraints and set appropriate pragmas for robustness and performance. Add db.SetMaxOpenConns(1) to prevent concurrent write issues and execute "PRAGMA foreign_keys = ON;" to enforce the foreign key constraints defined in the schema.
| // Limit SQLite to a single open connection to avoid concurrent write issues. | |
| db.SetMaxOpenConns(1) | |
| // Enable foreign key constraints for this connection. | |
| if _, err := db.ExecContext(context.Background(), "PRAGMA foreign_keys = ON;"); err != nil { | |
| return nil, fmt.Errorf("enable foreign keys: %w", err) | |
| } |
| func healthz(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write([]byte(`{"status":"ok"}`)) | ||
| } | ||
|
|
||
| func readyz(w http.ResponseWriter, _ *http.Request) { |
There was a problem hiding this comment.
The Content-Type header should be explicitly set to "application/json" before writing JSON responses. While many clients will work without it, setting the correct Content-Type is important for HTTP compliance and API clarity.
| func healthz(w http.ResponseWriter, _ *http.Request) { | |
| w.WriteHeader(http.StatusOK) | |
| _, _ = w.Write([]byte(`{"status":"ok"}`)) | |
| } | |
| func readyz(w http.ResponseWriter, _ *http.Request) { | |
| func healthz(w http.ResponseWriter, _ *http.Request) { | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusOK) | |
| _, _ = w.Write([]byte(`{"status":"ok"}`)) | |
| } | |
| func readyz(w http.ResponseWriter, _ *http.Request) { | |
| w.Header().Set("Content-Type", "application/json") |
| if err := application.Run(ctx); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Using panic for error handling in main is not ideal for production services. Consider using log.Fatal or fmt.Fprintf to stderr followed by os.Exit(1) for more graceful error reporting and to ensure deferred cleanup functions run where appropriate.
|
|
||
| return &http.Server{ | ||
| Addr: ":8080", | ||
| Handler: r, |
There was a problem hiding this comment.
The HTTP server is missing WriteTimeout and ReadTimeout settings. Only ReadHeaderTimeout is configured, which leaves the server vulnerable to slow-read attacks and can cause goroutine leaks. Add WriteTimeout and ReadTimeout (e.g., 10-30 seconds) for production readiness.
| Handler: r, | |
| Handler: r, | |
| ReadTimeout: 10 * time.Second, | |
| WriteTimeout: 10 * time.Second, |
| logger, err := observability.NewLogger(cfg.AppEnv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("create logger: %w", err) | ||
| } |
There was a problem hiding this comment.
If logger creation fails, the error is returned without any output, making startup failures difficult to diagnose. Consider using fmt.Fprintf(os.Stderr, ...) to output the error before returning it, or use a fallback logger for critical bootstrap failures.
| store, err := sqlite.New(cfg.DBPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("create sqlite store: %w", err) | ||
| } | ||
|
|
||
| if err := store.Migrate(context.Background()); err != nil { | ||
| return nil, fmt.Errorf("migrate sqlite: %w", err) | ||
| } | ||
|
|
||
| srv := httpapi.NewServer() | ||
| srv.Addr = cfg.HTTPAddr | ||
|
|
||
| return &App{cfg: cfg, logger: logger, store: store, http: srv}, nil |
There was a problem hiding this comment.
If store or migration fails during initialization, the logger is not cleaned up (logger.Sync() is never called). Consider deferring cleanup or wrapping initialization in a function that ensures resources are properly released on error paths.
Motivation
Description
mission-control/Go module with layout includingcmd/mission-control,internal/{app,config,domain,service,transport/httpapi,store/sqlite},pkg/observability,docs, and CI workflow files.internal/appthat loads env config, initializes a Zap logger, opens the SQLite store, runs embedded Goose migrations, and starts a Chi-based HTTP server with graceful shutdown.internal/store/sqlite) usingsqlxandpressly/goosefor migrations and added an initial migration schema formissions,tasks, andevents.Mission,Task,Event), a service interface (MissionService+ repository interface), basic HTTP transport withGET /healthzandGET /readyz, a unit test forhealthz,Makefile,.gitignore, and project docs (design.md,project-plan.md,implementation-plan.md).Testing
gofmt -w $(find . -name '*.go')andgo mod tidy, which completed successfully.go test ./...(theinternal/transport/httpapipackage test for/healthzpassed and other packages have no test files), and the test run succeeded.go build ./cmd/mission-control, which completed successfully..github/workflows/ci.ymlrunstidy, format check,go vet,go test -race, andgo buildto reproduce the validation steps automatically.Codex Task