server/migrations/sqlite/000001_create_initial_schema.up.sql (2)

13-22: Consider adding a UNIQUE constraint on server name or (name, repo_url).

If server names should be unique within the system (or unique per repository), adding a constraint would prevent duplicate entries at the database level. This is optional depending on your business requirements.

---

25-35: Consider adding an index on (server_id, agent_timestamp DESC) as a composite index.

You have separate indexes on server_id and agent_timestamp, but queries filtering by server_id and ordering by agent_timestamp (e.g., getting the latest status for a server) would benefit from a composite index.

♻️ Suggested composite index

 CREATE INDEX IF NOT EXISTS idx_agent_status_server_id ON agent_status(server_id);
-CREATE INDEX IF NOT EXISTS idx_agent_status_timestamp ON agent_status(agent_timestamp DESC);
+CREATE INDEX IF NOT EXISTS idx_agent_status_server_timestamp ON agent_status(server_id, agent_timestamp DESC);

server/migrations/sqlite/015_add_agent_tokens.up.sql (1)

14-15: Redundant index on token_hash.

The UNIQUE constraint on token_hash (line 4) already creates an implicit unique index in SQLite. The explicit index idx_agent_tokens_token_hash is redundant and adds unnecessary overhead during writes.

♻️ Suggested fix

 -- Create index for efficient token lookups
-CREATE INDEX idx_agent_tokens_token_hash ON agent_tokens(token_hash);
 CREATE INDEX idx_agent_tokens_server_id ON agent_tokens(server_id);
 CREATE INDEX idx_agent_tokens_active ON agent_tokens(is_active) WHERE is_active = 1;

If you remove this index from the up migration, also remove DROP INDEX IF EXISTS idx_agent_tokens_token_hash; from the down migration.

CONTRIBUTING.md (3)

31-33: LGTM! Clear PR guidelines with helpful cross-references.

The updated guidance properly references the new Testing section and clarifies documentation expectations.

Optional: Consider wrapping the bare URL on Line 32 in angle brackets per Markdown best practices: <https://github.com/pullbase/docs>

---

58-77: Excellent addition! Comprehensive testing guidance.

The new Testing section clearly distinguishes between sanity, full, and integration tests, helping contributors choose the appropriate test level.

Optional: Line 66 has a minor grammar quirk. Consider rewording for clarity:

📝 Suggested grammar improvement

-Full tests (requires built UI assets for server without stubs):
+Full tests (requires UI assets to be built for server without stubs):

Or more concisely:

-Full tests (requires built UI assets for server without stubs):
+Full tests (need built UI assets, no stubs):

---

79-85: Great addition! Clear guidance for documentation and API changes.

The new section provides essential instructions for keeping documentation and Swagger artifacts up-to-date.

Optional: Line 81 has a bare URL. Consider wrapping it in angle brackets per Markdown conventions: <https://github.com/pullbase/docs>

server/cmd/pullbasectl/status.go (1)

125-138: Good signal handling pattern.

The context-based cancellation with signal notification is well implemented. The buffered channel prevents the signal from being lost if not immediately received.

Minor note: signal.Stop(sigCh) could be called in the deferred cleanup for completeness, though it's not critical for a CLI tool that exits after this function returns.

♻️ Optional: Add signal cleanup

 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
 	sigCh := make(chan os.Signal, 1)
 	signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
+	defer signal.Stop(sigCh)
 	go func() {
 		<-sigCh
 		cancel()
 	}()

server/migrations/sqlite/014_add_auto_reconcile_to_environments.up.sql (1)

5-5: Consider adding IF NOT EXISTS for idempotent index creation.

Other migrations in this PR (e.g., 010_create_events.up.sql) use IF NOT EXISTS for table creation. For consistency and to allow safe re-runs, consider adding the same guard to the index creation.

Additionally, an index on a low-cardinality boolean column (0/1) typically provides minimal query optimization benefit unless you're frequently querying for rare values.

Suggested improvement

--- Create index for auto_reconcile column for performance
-CREATE INDEX idx_environments_auto_reconcile ON environments(auto_reconcile);
+-- Create index for auto_reconcile column for performance
+CREATE INDEX IF NOT EXISTS idx_environments_auto_reconcile ON environments(auto_reconcile);

server/migrations/sqlite/010_create_events.up.sql (1)

12-16: Index creation missing IF NOT EXISTS guard.

The table creation uses IF NOT EXISTS for idempotency, but the index creation statements do not. If this migration is partially applied or re-run, the indexes could fail to create or cause errors.

Suggested fix for idempotent index creation

 -- Create indexes for efficient querying
-CREATE INDEX idx_events_environment_id ON events(environment_id);
-CREATE INDEX idx_events_server_id ON events(server_id);
-CREATE INDEX idx_events_event_type ON events(event_type);
-CREATE INDEX idx_events_timestamp ON events(timestamp DESC);
+CREATE INDEX IF NOT EXISTS idx_events_environment_id ON events(environment_id);
+CREATE INDEX IF NOT EXISTS idx_events_server_id ON events(server_id);
+CREATE INDEX IF NOT EXISTS idx_events_event_type ON events(event_type);
+CREATE INDEX IF NOT EXISTS idx_events_timestamp ON events(timestamp DESC);

server/migrations/sqlite/008_create_environments.up.sql (1)

18-18: Low-value index on provider column.

Since the CHECK constraint enforces provider = 'github' (single allowed value), this index will have extremely low cardinality and provide no query benefit. Consider removing it unless you plan to support multiple providers in the future.

♻️ Suggested removal

 CREATE INDEX idx_environments_repo_url ON environments(repo_url);
-CREATE INDEX idx_environments_provider ON environments(provider);
 CREATE INDEX idx_environments_status ON environments(status);

server/migrations/sqlite/021_add_notification_webhook.up.sql (1)

1-1: LGTM!

Adding a nullable TEXT column for webhook URLs is safe and appropriate.

Minor note: Other migrations in this PR (e.g., 011_add_deployed_commit.up.sql) include a descriptive comment header. Consider adding one for consistency, though this is optional.

server/migrations/sqlite/009_add_rollback_events.up.sql (1)

17-20: Consider adding IF NOT EXISTS to index creation for idempotency.

The table uses CREATE TABLE IF NOT EXISTS for safe re-runs, but the indexes use plain CREATE INDEX. If the migration partially succeeds (table created, some indexes created) and is retried, it will fail on the already-created indexes.

♻️ Suggested fix

 -- Create indexes for efficient querying
-CREATE INDEX idx_rollback_events_environment_id ON rollback_events(environment_id);
-CREATE INDEX idx_rollback_events_status ON rollback_events(status);
-CREATE INDEX idx_rollback_events_created_at ON rollback_events(created_at DESC);
+CREATE INDEX IF NOT EXISTS idx_rollback_events_environment_id ON rollback_events(environment_id);
+CREATE INDEX IF NOT EXISTS idx_rollback_events_status ON rollback_events(status);
+CREATE INDEX IF NOT EXISTS idx_rollback_events_created_at ON rollback_events(created_at DESC);

server/migrations/sqlite/012_remove_polling_support.down.sql (1)

1-3: Clarify that data changes are also not reverted.

The comment only mentions the CHECK constraint limitation, but doesn't clarify that the status = 'fallback' → 'error' data change from the up migration is also not reversed. This could mislead developers.

📝 Suggested improvement

 -- Migration: Restore polling support and fallback status
 -- Note: SQLite cannot modify CHECK constraints after table creation.
--- This is a no-op for SQLite.
+-- This is a no-op for SQLite. The data change (fallback -> error) from the
+-- up migration is not reverted as we cannot reliably identify which 'error'
+-- statuses were originally 'fallback'.

server/pkg/testutil/testutil.go (1)

156-166: Consider documenting the path assumption.

The function traverses up 3 directory levels from testutil.go to reach server/migrations/. This works correctly for the current structure but could break if the test utility file is moved.

A brief comment documenting the expected path relationship would aid future maintainers:

📝 Suggested documentation

 func defaultMigrationPath() string {
+	// Navigate from server/pkg/testutil/testutil.go up to server/migrations/
 	_, currentFile, _, ok := runtime.Caller(0)
 	if !ok {
 		return "file://migrations"
 	}
 
 	serverDir := filepath.Dir(filepath.Dir(filepath.Dir(currentFile)))
 	migrationsDir := filepath.Join(serverDir, "migrations")
 
 	return "file://" + migrationsDir
 }

server/pkg/apierrors/errors.go (1)

300-301: Consider handling JSON encoding errors.

The json.NewEncoder(w).Encode(resp) error is silently ignored. While this is common practice for response writing (since headers are already sent), consider logging encoding failures for debugging purposes.

♻️ Optional: Log encoding errors

-	json.NewEncoder(w).Encode(resp)
+	if err := json.NewEncoder(w).Encode(resp); err != nil {
+		// Log but don't return - headers already sent
+		// Consider adding: log.Warn("failed to encode error response", "error", err)
+	}

server/pkg/rollback/service_test.go (1)

201-201: Consider using io.Discard instead of os.Stdout for quieter test output.

The logger is initialized with os.Stdout, which will produce output during test runs. Other test files in this PR (e.g., webhook_handlers_integration_test.go) use io.Discard to suppress log output. Consider aligning for consistency and cleaner test output.

Suggested change

+import "io"
+
-logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
+logger := logging.NewLogger(logging.Options{Format: "text", Output: io.Discard})

Also applies to: 337-337, 364-364, 400-400

server/pkg/notifications/webhook_test.go (1)

127-148: Test correctly validates context timeout during retry backoff.

The test flow is accurate: first request returns 500 → retry backoff begins (50ms) → context times out (25ms) → ctx.Err() returned via the select statement. The implementation properly checks ctx.Done() during the backoff wait, so cancellation is correctly handled.

The timing margins are intentionally tight (25ms timeout vs 50ms backoff) to ensure the context expires during the wait period rather than before. This is a valid test scenario. However, in loaded CI environments where the initial HTTP request itself may take measurable time, the timing could be tighter than intended, potentially causing the test to be sensitive to execution speed.

Consider either increasing the context timeout slightly (e.g., 40ms) while keeping it below the 50ms backoff, or reducing the backoff delay to create a clearer separation and more predictable behavior across environments.

server/pkg/server/handlers_agent.go (2)

241-247: Move defer r.Body.Close() after reading the body.

The defer r.Body.Close() is placed before json.NewDecoder(r.Body).Decode(). While this will technically work (defer executes at function return), it's unconventional and could cause issues if the code is refactored to return early. The standard pattern is to close the body after you're done reading it.

♻️ Suggested improvement

 func (api *API) UpdateAgentStatusHandler(w http.ResponseWriter, r *http.Request) {

 	claims, ok := GetUserClaims(r.Context())
 	if !ok || claims == nil {
 		writeAPIError(w, apierrors.Unauthorized("Authorization header or session cookie required"))
 		return
 	}

 	serverID := chi.URLParam(r, "serverID")
 	var payload AgentStatusPayload
-	defer r.Body.Close()

 	if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
+		r.Body.Close()
 		api.log().Error("bad request for status update", "server_id", serverID, "error", err)
 		writeAPIError(w, apierrors.BadRequestf("Invalid status payload: %s", err.Error()))
 		return
 	}
+	r.Body.Close()

Alternatively, keep defer but place it immediately after acquiring the body or at function start:

 func (api *API) UpdateAgentStatusHandler(w http.ResponseWriter, r *http.Request) {
+	defer r.Body.Close()

 	claims, ok := GetUserClaims(r.Context())

---

527-533: Same issue: move defer r.Body.Close() to function start.

Same pattern issue as UpdateAgentStatusHandler. Consider moving the defer to the beginning of the function for clarity.

♻️ Suggested improvement

 func (api *API) AgentUpdateStatusHandler(w http.ResponseWriter, r *http.Request) {
+	defer r.Body.Close()

 	agentToken, ok := GetAgentToken(r.Context())
 	if !ok || agentToken == nil {
 		writeAPIError(w, apierrors.Unauthorized("Agent token required"))
 		return
 	}

 	serverID := agentToken.ServerID
 	var payload AgentStatusPayload
-	defer r.Body.Close()

server/cmd/server/main.go (1)

280-284: Consider consolidating duplicate logger instances.

Two separate logger instances are created with identical options (Format: "text", Output: os.Stdout). Consider creating a single logger instance and reusing it.

♻️ Suggested consolidation

+	logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
+
 	var environmentMonitor *gitmonitor.EnvironmentMonitor
 	var rollbackGitMonitor rollback.GitMonitor
 	var installationTokenProvider gitmonitor.InstallationTokenProvider
 	if cfg.Git.Enabled {
 		// ... encryption key and GitHub client setup ...

-		logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
 		webhookRouter := gitmonitor.NewWebhookRouter(logger, nil)
 		webhookManager := gitmonitor.NewWebhookManager(webhookRouter, logger, envRepo)
 		// ...
 	}

-	logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
 	rollbackService := rollback.NewService(repo, rollbackGitMonitor, logger)

This would require moving the logger declaration before the if cfg.Git.Enabled block and ensuring it's in scope for both use cases.

Also applies to: 302-303

server/docs/swagger.yaml (1)

481-482: Stripped API metadata may impact documentation quality.

The info section has been reduced to just contact: {}. While this compiles, consider retaining essential metadata like title, version, and description for better API documentation and client SDK generation.

server/docs/swagger.json (1)

3-5: Stripped API metadata.

Similar to swagger.yaml, the info block has minimal content. If this is auto-generated, the source annotations should include proper API metadata.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

CONTRIBUTING.md (1)

32-32: Consider wrapping bare URLs in angle brackets.

Static analysis flagged bare URLs at lines 32 and 83. For markdown best practices, consider wrapping them in angle brackets.

📝 Proposed formatting improvements

Line 32:

-5. Update documentation if the change alters behavior or adds features (docs live at https://github.com/pullbase/docs)
+5. Update documentation if the change alters behavior or adds features (docs live at <https://github.com/pullbase/docs>)

Line 83:

-- Update relevant docs in https://github.com/pullbase/docs when features or behavior change.
+- Update relevant docs in <https://github.com/pullbase/docs> when features or behavior change.

Also applies to: 83-83

server/pkg/server/handlers.go (1)

268-278: Consider using instance logger for consistency.

The requireRole function uses package-level logging.Error and logging.Warn, which write to the default logger. However, other methods in this file (e.g., RecordAuditLog at lines 297, 312) use api.log() to respect a custom logger if configured.

Since requireRole doesn't have access to the API instance, this is acceptable, but worth noting that log output from role checks will always go to the default logger regardless of API.Logger configuration.

server/cmd/server/main.go (1)

279-283: Consider reusing the logger instance.

A logger is created here at line 279, and another identical one is created at line 301. Both use the same options (Format: "text", Output: os.Stdout). Consider creating the logger once earlier and reusing it.

♻️ Suggested refactor

Create the logger once before the if cfg.Git.Enabled block:

+	logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
+
 	var environmentMonitor *gitmonitor.EnvironmentMonitor
 	var rollbackGitMonitor rollback.GitMonitor
 	var installationTokenProvider gitmonitor.InstallationTokenProvider
 	if cfg.Git.Enabled {
 		// ... existing code ...
-		logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
 		webhookRouter := gitmonitor.NewWebhookRouter(logger, nil)

And remove the duplicate creation at line 301.

server/pkg/testutil/testutil.go (2)

155-165: Path resolution may be fragile in some test execution contexts.

defaultMigrationPath() uses runtime.Caller(0) to locate the source file and navigates up three directories to find migrations. This works when running tests from the standard Go toolchain but may fail if:

• Tests are run from a different working directory
• The binary is compiled and run separately from source

The fallback to "file://migrations" at line 158-159 provides a safety net. Consider documenting this behavior or adding a test to verify the path resolution.

---

334-340: Hardcoded migration version may require manual updates.

The fallback schema inserts migration version 22 as the current version. This needs to be updated whenever new migrations are added, which is easy to forget.

Consider extracting this as a constant or documenting the maintenance requirement.

♻️ Suggested refactor

+const fallbackSchemaVersion = 22 // Update when adding new migrations

 // In setupFallbackSchema():
-		INSERT INTO schema_migrations (version, dirty) VALUES (22, false) ON CONFLICT DO NOTHING;
+		INSERT INTO schema_migrations (version, dirty) VALUES (%d, false) ON CONFLICT DO NOTHING;

Or add a comment near the INSERT statement:

-- NOTE: Update this version when adding new migrations
INSERT INTO schema_migrations (version, dirty) VALUES (22, false) ON CONFLICT DO NOTHING;

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: hacktivist123
Repo: pullbase/pullbase PR: 1
File: server/pkg/config/config_test.go:89-92
Timestamp: 2026-01-15T20:03:09.042Z
Learning: Go 1.22+ fixes: range loop variables are captured per iteration, so closures (including t.Cleanup) generally don’t require shadowing or explicit capture. Since server/go.mod specifies Go 1.24.0, reviewers can assume this behavior across Go files in the repo and avoid shadowing for range variables in tests and implementation. If there’s code intended for older Go versions, consider explicit capture to preserve pre-1.22 semantics.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

CONTRIBUTING.md (1)

68-68: Minor grammar issue: "requires built" is nonstandard.

Consider rewording to "requires UI assets to be built" or simply "requires built UI assets".

CONTRIBUTING.md (1)

81-88: LGTM - swag paths are now correct.

The swag init command now correctly references server/cmd/server/main.go and outputs to server/docs, matching the actual project structure.

Static analysis flags bare URLs on lines 32 and 83. Consider using markdown link syntax for consistency:

• [pullbase/docs](https://github.com/pullbase/docs)

server/pkg/testutil/testutil.go (3)

29-32: Consider documenting which migration version corresponds to FallbackSchemaVersion.

The comment says "Update this when adding new migrations" but doesn't indicate what schema state version 22 represents. Consider adding a reference to the migration file name or date for easier maintenance.

---

144-157: Silently falling back to manual schema may hide migration issues.

When InitSchema fails, the code logs and falls back to setupFallbackSchema(). This could mask real migration bugs in CI. Consider making the fallback opt-in or failing loudly in CI environments.

💡 Suggested approach

 func (tdb *TestDB) MustMigrate(migrationPath string) {
 	tdb.t.Helper()

 	if migrationPath == "" {
 		migrationPath = defaultMigrationPath()
 	}
 	tdb.t.Logf("using migration path: %s", migrationPath)

 	ctx := context.Background()
 	if err := database.InitSchema(ctx, tdb.DB, tdb.Dialect, migrationPath); err != nil {
-		tdb.t.Logf("Migration failed: %v - falling back to manual schema", err)
-		tdb.setupFallbackSchema()
+		if os.Getenv("CI") != "" {
+			tdb.t.Fatalf("Migration failed in CI: %v", err)
+		}
+		tdb.t.Logf("Migration failed: %v - falling back to manual schema (disable with CI=true)", err)
+		tdb.setupFallbackSchema()
 	}
 }

---

159-169: Fragile path resolution using runtime.Caller with hardcoded directory depth.

The triple filepath.Dir call assumes the file is always at server/pkg/testutil/testutil.go. If the file moves or the directory structure changes, this silently returns an incorrect path.

💡 Suggested improvement

Add validation that the computed path exists:

 func defaultMigrationPath() string {
 	_, currentFile, _, ok := runtime.Caller(0)
 	if !ok {
 		return "file://migrations"
 	}

 	serverDir := filepath.Dir(filepath.Dir(filepath.Dir(currentFile)))
 	migrationsDir := filepath.Join(serverDir, "migrations")

+	if _, err := os.Stat(migrationsDir); os.IsNotExist(err) {
+		// Fall back if computed path doesn't exist
+		return "file://migrations"
+	}
+
 	return "file://" + migrationsDir
 }

server/cmd/server/main.go (2)

249-250: Consider explicitly setting the log level.

The logging.Options struct accepts a Level field (per the relevant snippets), but it's not specified here. While there's likely a sensible default, consider making this configurable via cfg to allow operators to control verbosity.

💡 Suggested improvement

-	logger := logging.NewLogger(logging.Options{Format: "text", Output: os.Stdout})
+	logger := logging.NewLogger(logging.Options{
+		Level:  cfg.Logging.Level, // or a sensible default like "info"
+		Format: "text",
+		Output: os.Stdout,
+	})

---

410-416: Consider channel-based error handling for server startup failures.

Calling os.Exit(1) inside goroutines (lines 414, 459) bypasses deferred cleanup like db.Close(). While startup failures typically warrant immediate exit, a channel-based approach would allow graceful cleanup.

This is a minor concern since startup failures are fatal anyway, but for completeness:

errChan := make(chan error, 1)
go func() {
    if err := httpServer.ListenAndServe(); err != nil && err != http.ErrServerClosed {
        errChan <- err
    }
}()
// Then select on errChan and stopChan in main

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: hacktivist123
Repo: pullbase/pullbase PR: 1
File: server/pkg/config/config_test.go:89-92
Timestamp: 2026-01-15T20:03:09.042Z
Learning: Go 1.22+ fixes: range loop variables are captured per iteration, so closures (including t.Cleanup) generally don’t require shadowing or explicit capture. Since server/go.mod specifies Go 1.24.0, reviewers can assume this behavior across Go files in the repo and avoid shadowing for range variables in tests and implementation. If there’s code intended for older Go versions, consider explicit capture to preserve pre-1.22 semantics.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: hacktivist123
Repo: pullbase/pullbase PR: 1
File: server/pkg/config/config_test.go:89-92
Timestamp: 2026-01-15T20:03:09.042Z
Learning: Go 1.22+ fixes: range loop variables are captured per iteration, so closures (including t.Cleanup) generally don’t require shadowing or explicit capture. Since server/go.mod specifies Go 1.24.0, reviewers can assume this behavior across Go files in the repo and avoid shadowing for range variables in tests and implementation. If there’s code intended for older Go versions, consider explicit capture to preserve pre-1.22 semantics.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}