Skip to content

Commit 3aa1197

Browse files
josephgoksujosephgoksu
authored
fix: surface swallowed errors across storage and MCP layers (#18)
- task_store: log corrupt JSON instead of silently discarding (7 scan fields + 3 clarify session fields + 4 draft_state unmarshal sites) - sqlite: replace blind _, _ migration pattern with column-existence checks so real ALTER TABLE errors get logged - policy/audit: log corrupt input JSON in both scan methods - mcp/handlers: log GetProjectRoot failures in explain and simplify handlers instead of silently using empty base path
1 parent d0cc84f commit 3aa1197

4 files changed

Lines changed: 89 additions & 27 deletions

File tree

‎internal/mcp/handlers.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package mcp
44
import (
55
"context"
66
"fmt"
7+
"log/slog"
78
"os"
89
"path/filepath"
910
"strings"
@@ -153,7 +154,10 @@ func handleCodeExplain(ctx context.Context, repo *memory.Repository, params Code
153154
}
154155

155156
// Get base path for source code fetching
156-
basePath, _ := config.GetProjectRoot()
157+
basePath, err := config.GetProjectRoot()
158+
if err != nil {
159+
slog.Debug("GetProjectRoot failed in explain handler", "error", err)
160+
}
157161

158162
appCtx := app.NewContextForRole(repo, llm.RoleQuery)
159163
appCtx.BasePath = basePath
@@ -267,7 +271,10 @@ func handleCodeSimplify(ctx context.Context, repo *memory.Repository, params Cod
267271

268272
// If file_path provided, read the file content
269273
if filePath != "" && code == "" {
270-
projectRoot, _ := config.GetProjectRoot()
274+
projectRoot, err := config.GetProjectRoot()
275+
if err != nil {
276+
slog.Debug("GetProjectRoot failed in simplify handler", "error", err)
277+
}
271278

272279
// Validate path to prevent traversal attacks
273280
resolvedPath, err := validateAndResolvePath(filePath, projectRoot)

‎internal/memory/sqlite.go‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"database/sql"
55
"encoding/json"
66
"fmt"
7+
"log/slog"
78
"os"
89
"path/filepath"
910
"strings"
@@ -57,17 +58,35 @@ func NewSQLiteStore(basePath string) (*SQLiteStore, error) {
5758
return nil, fmt.Errorf("init schema: %w", err)
5859
}
5960

60-
// Migrations - ignore errors for columns that already exist
61-
_, _ = db.Exec(`ALTER TABLE tasks ADD COLUMN complexity TEXT DEFAULT 'medium'`)
62-
63-
// Interactive planning migrations (phases support)
64-
_, _ = db.Exec(`ALTER TABLE plans ADD COLUMN draft_state TEXT`)
65-
_, _ = db.Exec(`ALTER TABLE plans ADD COLUMN generation_mode TEXT DEFAULT 'batch'`)
66-
_, _ = db.Exec(`ALTER TABLE tasks ADD COLUMN phase_id TEXT REFERENCES phases(id) ON DELETE SET NULL`)
61+
// Migrations — add columns only if they don't already exist
62+
migrateAddColumn(db, "tasks", "complexity", `ALTER TABLE tasks ADD COLUMN complexity TEXT DEFAULT 'medium'`)
63+
migrateAddColumn(db, "plans", "draft_state", `ALTER TABLE plans ADD COLUMN draft_state TEXT`)
64+
migrateAddColumn(db, "plans", "generation_mode", `ALTER TABLE plans ADD COLUMN generation_mode TEXT DEFAULT 'batch'`)
65+
migrateAddColumn(db, "tasks", "phase_id", `ALTER TABLE tasks ADD COLUMN phase_id TEXT REFERENCES phases(id) ON DELETE SET NULL`)
6766

6867
return store, nil
6968
}
7069

70+
// migrateAddColumn adds a column if it doesn't already exist.
71+
// Logs real errors instead of silently swallowing them.
72+
func migrateAddColumn(db *sql.DB, table, column, ddl string) {
73+
var exists int
74+
err := db.QueryRow(
75+
`SELECT COUNT(*) FROM pragma_table_info(?) WHERE name = ?`,
76+
table, column,
77+
).Scan(&exists)
78+
if err != nil {
79+
slog.Warn("migration check failed", "table", table, "column", column, "error", err)
80+
return
81+
}
82+
if exists > 0 {
83+
return // already migrated
84+
}
85+
if _, err := db.Exec(ddl); err != nil {
86+
slog.Warn("migration failed", "table", table, "column", column, "error", err)
87+
}
88+
}
89+
7190
// initSchema creates the database tables if they don't exist.
7291
func (s *SQLiteStore) initSchema() error {
7392
schema := `

‎internal/memory/task_store.go‎

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"errors"
77
"fmt"
88
"log"
9+
"log/slog"
910
"strings"
1011
"time"
1112

@@ -156,9 +157,11 @@ func (s *SQLiteStore) CreatePlan(p *task.Plan) error {
156157
// Serialize draft state if present
157158
var draftStateJSON interface{}
158159
if p.DraftState != nil {
159-
if data, err := json.Marshal(p.DraftState); err == nil {
160-
draftStateJSON = string(data)
160+
data, err := json.Marshal(p.DraftState)
161+
if err != nil {
162+
return fmt.Errorf("marshal draft_state: %w", err)
161163
}
164+
draftStateJSON = string(data)
162165
}
163166

164167
if _, err = tx.Exec(`
@@ -207,7 +210,9 @@ func (s *SQLiteStore) GetPlan(id string) (*task.Plan, error) {
207210
}
208211
if draftStateJSON.Valid && draftStateJSON.String != "" {
209212
var draftState task.PlanDraftState
210-
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err == nil {
213+
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err != nil {
214+
slog.Warn("corrupt draft_state JSON", "plan", p.ID, "error", err)
215+
} else {
211216
p.DraftState = &draftState
212217
}
213218
}
@@ -257,7 +262,9 @@ func (s *SQLiteStore) ListPlans() ([]task.Plan, error) {
257262
}
258263
if draftStateJSON.Valid && draftStateJSON.String != "" {
259264
var draftState task.PlanDraftState
260-
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err == nil {
265+
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err != nil {
266+
slog.Warn("corrupt draft_state JSON", "plan", p.ID, "error", err)
267+
} else {
261268
p.DraftState = &draftState
262269
}
263270
}
@@ -459,7 +466,9 @@ func (s *SQLiteStore) GetClarifySession(id string) (*task.ClarifySession, error)
459466
session.CreatedAt, _ = time.Parse(time.RFC3339, createdAt)
460467
session.UpdatedAt, _ = time.Parse(time.RFC3339, updatedAt)
461468
if currentQuestionsJSON.Valid && currentQuestionsJSON.String != "" {
462-
_ = json.Unmarshal([]byte(currentQuestionsJSON.String), &session.CurrentQuestions)
469+
if err := json.Unmarshal([]byte(currentQuestionsJSON.String), &session.CurrentQuestions); err != nil {
470+
slog.Warn("corrupt current_questions JSON", "session", session.ID, "error", err)
471+
}
463472
}
464473

465474
return &session, nil
@@ -571,10 +580,14 @@ func (s *SQLiteStore) ListClarifyTurns(sessionID string) ([]task.ClarifyTurn, er
571580
turn.MaxRoundsReached = maxRoundsReachedInt == 1
572581
turn.CreatedAt, _ = time.Parse(time.RFC3339, createdAt)
573582
if questionsJSON.Valid && questionsJSON.String != "" {
574-
_ = json.Unmarshal([]byte(questionsJSON.String), &turn.Questions)
583+
if err := json.Unmarshal([]byte(questionsJSON.String), &turn.Questions); err != nil {
584+
slog.Warn("corrupt questions JSON", "turn", turn.ID, "error", err)
585+
}
575586
}
576587
if answersJSON.Valid && answersJSON.String != "" {
577-
_ = json.Unmarshal([]byte(answersJSON.String), &turn.Answers)
588+
if err := json.Unmarshal([]byte(answersJSON.String), &turn.Answers); err != nil {
589+
slog.Warn("corrupt answers JSON", "turn", turn.ID, "error", err)
590+
}
578591
}
579592
turns = append(turns, turn)
580593
}
@@ -648,25 +661,39 @@ func scanTaskRow(row taskRowScanner) (task.Task, error) {
648661
}
649662

650663
if acJSON.Valid && acJSON.String != "" {
651-
_ = json.Unmarshal([]byte(acJSON.String), &t.AcceptanceCriteria)
664+
if err := json.Unmarshal([]byte(acJSON.String), &t.AcceptanceCriteria); err != nil {
665+
slog.Warn("corrupt acceptance_criteria JSON", "task", t.ID, "error", err)
666+
}
652667
}
653668
if vsJSON.Valid && vsJSON.String != "" {
654-
_ = json.Unmarshal([]byte(vsJSON.String), &t.ValidationSteps)
669+
if err := json.Unmarshal([]byte(vsJSON.String), &t.ValidationSteps); err != nil {
670+
slog.Warn("corrupt validation_steps JSON", "task", t.ID, "error", err)
671+
}
655672
}
656673
if keywordsJSON.Valid && keywordsJSON.String != "" {
657-
_ = json.Unmarshal([]byte(keywordsJSON.String), &t.Keywords)
674+
if err := json.Unmarshal([]byte(keywordsJSON.String), &t.Keywords); err != nil {
675+
slog.Warn("corrupt keywords JSON", "task", t.ID, "error", err)
676+
}
658677
}
659678
if queriesJSON.Valid && queriesJSON.String != "" {
660-
_ = json.Unmarshal([]byte(queriesJSON.String), &t.SuggestedAskQueries)
679+
if err := json.Unmarshal([]byte(queriesJSON.String), &t.SuggestedAskQueries); err != nil {
680+
slog.Warn("corrupt suggested_ask_queries JSON", "task", t.ID, "error", err)
681+
}
661682
}
662683
if filesJSON.Valid && filesJSON.String != "" {
663-
_ = json.Unmarshal([]byte(filesJSON.String), &t.FilesModified)
684+
if err := json.Unmarshal([]byte(filesJSON.String), &t.FilesModified); err != nil {
685+
slog.Warn("corrupt files_modified JSON", "task", t.ID, "error", err)
686+
}
664687
}
665688
if expectedFilesJSON.Valid && expectedFilesJSON.String != "" {
666-
_ = json.Unmarshal([]byte(expectedFilesJSON.String), &t.ExpectedFiles)
689+
if err := json.Unmarshal([]byte(expectedFilesJSON.String), &t.ExpectedFiles); err != nil {
690+
slog.Warn("corrupt expected_files JSON", "task", t.ID, "error", err)
691+
}
667692
}
668693
if gitBaselineJSON.Valid && gitBaselineJSON.String != "" {
669-
_ = json.Unmarshal([]byte(gitBaselineJSON.String), &t.GitBaseline)
694+
if err := json.Unmarshal([]byte(gitBaselineJSON.String), &t.GitBaseline); err != nil {
695+
slog.Warn("corrupt git_baseline JSON", "task", t.ID, "error", err)
696+
}
670697
}
671698

672699
return t, nil
@@ -1144,7 +1171,9 @@ func (s *SQLiteStore) SearchPlans(query string, status task.PlanStatus) ([]task.
11441171
}
11451172
if draftStateJSON.Valid && draftStateJSON.String != "" {
11461173
var draftState task.PlanDraftState
1147-
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err == nil {
1174+
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err != nil {
1175+
slog.Warn("corrupt draft_state JSON", "plan", p.ID, "error", err)
1176+
} else {
11481177
p.DraftState = &draftState
11491178
}
11501179
}
@@ -1190,7 +1219,9 @@ func (s *SQLiteStore) GetActivePlan() (*task.Plan, error) {
11901219
}
11911220
if draftStateJSON.Valid && draftStateJSON.String != "" {
11921221
var draftState task.PlanDraftState
1193-
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err == nil {
1222+
if err := json.Unmarshal([]byte(draftStateJSON.String), &draftState); err != nil {
1223+
slog.Warn("corrupt draft_state JSON", "plan", p.ID, "error", err)
1224+
} else {
11941225
p.DraftState = &draftState
11951226
}
11961227
}

‎internal/policy/audit.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"database/sql"
55
"encoding/json"
66
"fmt"
7+
"log/slog"
78
"time"
89

910
"github.com/google/uuid"
@@ -202,7 +203,9 @@ func (s *AuditStore) scanDecision(row *sql.Row) (*PolicyDecision, error) {
202203
d.Violations = ParseViolations(violationsJSON)
203204
if inputJSON != "" && inputJSON != "{}" {
204205
var input any
205-
if err := json.Unmarshal([]byte(inputJSON), &input); err == nil {
206+
if err := json.Unmarshal([]byte(inputJSON), &input); err != nil {
207+
slog.Warn("corrupt policy decision input JSON", "id", d.DecisionID, "error", err)
208+
} else {
206209
d.Input = input
207210
}
208211
}
@@ -238,7 +241,9 @@ func (s *AuditStore) scanDecisionRows(rows *sql.Rows) (*PolicyDecision, error) {
238241
d.Violations = ParseViolations(violationsJSON)
239242
if inputJSON != "" && inputJSON != "{}" {
240243
var input any
241-
if err := json.Unmarshal([]byte(inputJSON), &input); err == nil {
244+
if err := json.Unmarshal([]byte(inputJSON), &input); err != nil {
245+
slog.Warn("corrupt policy decision input JSON", "id", d.DecisionID, "error", err)
246+
} else {
242247
d.Input = input
243248
}
244249
}

0 commit comments

Comments
 (0)