feat: replace JSON file storage with SQLite#78
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates CLI persistence from filesystem/JSON to SQLite: adds a db package, schema, migration tooling, threads *db.DB/*db.Conn through CLI commands and subsystems, and rewrites persistence, APIs, and tests to use transactional DB operations. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Main
participant Detect as jsonFilesExist
participant FS as Filesystem
participant DB as db.Open / *db.DB
participant Migrate as MigrateJSON
CLI->>Detect: check for legacy JSON files
Detect->>FS: scan .fellowship & worktrees
FS-->>Detect: return file list
Detect-->>CLI: found/none
CLI->>DB: Open(fromDir) -> *db.DB
alt legacy JSON found
CLI->>Migrate: runMigrate() -> MigrateJSON(d, mainRepo)
Migrate->>FS: backup discovered files
Migrate->>DB: WithTx(parse & insert)
DB->>DB: INSERT into tables (fellowship, quests, quest_state, errands, herald, bulletin, autopsies...)
Migrate->>FS: remove original JSON files
Migrate-->>CLI: migration complete
end
sequenceDiagram
participant CLI as Command Handler
participant DB as *db.DB
participant TX as DB Transaction
participant Schema as applySchema
CLI->>DB: Open/Reuse DB
DB->>Schema: applySchema()
DB-->>CLI: *db.DB
CLI->>DB: WithTx(ctx, fn)
DB->>TX: Begin IMMEDIATE
TX->>DB: execute SQL (select/insert/update)
alt fn returns error
TX->>DB: Rollback
else
TX->>DB: Commit
end
DB-->>CLI: result/error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace file-based Load/Save/WithLock/FindStateFile with DB-backed Load/Upsert/Delete/FindQuest. Remove Version field, ErrNoSave, datadir and filelock imports. State struct and phase helpers unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implements MigrateJSON() which reads all 7 legacy JSON/JSONL file types from main repo and worktree .fellowship/ directories, backs them up, inserts into SQLite in a single transaction, and deletes originals. Wires runMigrate into main.go and removes deprecated OpenNoMutex flag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The filelock package provided advisory file locking for JSON state files. With the migration to SQLite, WAL mode handles all concurrency needs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justinjdev
force-pushed
the
feat/sqlite-storage
branch
from
aa4a5fc to
022661e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
cli/internal/db/db_test.go-57-74 (1)
57-74:⚠️ Potential issue | 🟡 MinorThis rollback test can pass without proving rollback.
The insert error, the
WithTxerror, and the finalWithConnerror are all ignored. If the insert never succeeds, orWithTxfails before the transaction body is actually exercised,countstill stays0and the test passes. Assert the rollback error and propagate the SQL errors so this actually validates transaction rollback.🧪 One way to tighten the test
- _ = d.WithTx(context.Background(), func(conn *Conn) error { - sqlitex.Execute(conn, `INSERT INTO quest_state (quest_name, phase, created_at, updated_at) VALUES ('test', 'Onboard', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')`, nil) - return fmt.Errorf("rollback") - }) + rollbackErr := fmt.Errorf("rollback") + err = d.WithTx(context.Background(), func(conn *Conn) error { + if err := sqlitex.Execute(conn, `INSERT INTO quest_state (quest_name, phase, created_at, updated_at) VALUES ('test', 'Onboard', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')`, nil); err != nil { + return err + } + return rollbackErr + }) + if err == nil || err.Error() != rollbackErr.Error() { + t.Fatalf("WithTx() error = %v, want %v", err, rollbackErr) + } @@ - d.WithConn(context.Background(), func(conn *Conn) error { + err = d.WithConn(context.Background(), func(conn *Conn) error { return sqlitex.Execute(conn, "SELECT count(*) FROM quest_state", &sqlitex.ExecOptions{ ResultFunc: func(stmt *sqlite.Stmt) error { count = stmt.ColumnInt(0) return nil }, }) }) + if err != nil { + t.Fatal(err) + }cli/internal/errand/errand_test.go-12-17 (1)
12-17:⚠️ Potential issue | 🟡 MinorSame
WithTxerror ignored pattern as other test files.The
seedQuesthelper discards transaction errors. Consider checking the error to catch seeding failures early.cli/internal/tome/tome_test.go-24-36 (1)
24-36:⚠️ Potential issue | 🟡 Minor
WithTxerror ignored in test body.Same issue as the helper—transaction errors are discarded. This pattern repeats across most tests in this file (Lines 43, 62, 78, 104, 132, 169, 176).
cli/internal/tome/tome_test.go-13-18 (1)
13-18:⚠️ Potential issue | 🟡 MinorIgnored error from
WithTxinseedQuesthelper.If
state.Upsertfails, the error is silently discarded, which could cause downstream test failures with misleading errors.🔧 Suggested fix
func seedQuest(t *testing.T, d *db.DB, name string) { t.Helper() - d.WithTx(context.Background(), func(conn *db.Conn) error { + if err := d.WithTx(context.Background(), func(conn *db.Conn) error { return state.Upsert(conn, &state.State{QuestName: name, Phase: "Research"}) - }) + }); err != nil { + t.Fatalf("seedQuest %s: %v", name, err) + } }cli/internal/dashboard/server_test.go-17-42 (1)
17-42:⚠️ Potential issue | 🟡 Minor
setupTestDBignoresWithTxerror.If any of
InitFellowship,AddQuest, orstate.Upsertfails, the test proceeds with incomplete setup. This could cause confusing test failures.🔧 Suggested fix
- d.WithTx(context.Background(), func(conn *db.Conn) error { + if err := d.WithTx(context.Background(), func(conn *db.Conn) error { InitFellowship(conn, "test-fellowship", "/tmp/repo", "main") - AddQuest(conn, QuestEntry{ + if err := AddQuest(conn, QuestEntry{ Name: "quest-login", Worktree: worktreeDir, TaskID: "t1", - }) + }); err != nil { + return err + } // ... rest return nil - }) + }); err != nil { + t.Fatalf("setupTestDB: %v", err) + }cli/internal/db/migrate_test.go-196-421 (1)
196-421:⚠️ Potential issue | 🟡 MinorIgnored error from
WithConncould mask test failures.The return value from
d.WithConnis discarded. If the callback returns an error (e.g., from a failed SQL query), the test will silently pass. Capture and check the error to ensure test reliability.🔧 Suggested fix
- d.WithConn(t.Context(), func(conn *Conn) error { + if err := d.WithConn(t.Context(), func(conn *Conn) error { // ... all assertions ... return nil - }) + }); err != nil { + t.Fatalf("WithConn: %v", err) + }cli/internal/herald/problems_test.go-14-29 (1)
14-29:⚠️ Potential issue | 🟡 MinorFail the test when fixture writes fail.
insertQuestStateand theseAnnouncecalls drop their errors, so several negative cases can still pass if the DB setup silently stops working. Bubble setup errors out of the transaction callback and fail fast instead of asserting against whatever state happens to be left in the test database.Also applies to: 81-85, 111-115, 134-135, 161-161, 180-185
cli/internal/herald/herald_test.go-152-154 (1)
152-154:⚠️ Potential issue | 🟡 MinorUnchecked error from test setup SQL.
The
sqlitex.Executereturn value is not checked. If the INSERT fails (e.g., schema mismatch), the test would continue with invalid preconditions, potentially causing confusing test failures.🧪 Proposed fix
- sqlitex.Execute(conn, + if err := sqlitex.Execute(conn, `INSERT INTO quest_state (quest_name, phase, gate_pending, created_at, updated_at) - VALUES ('q1', 'Research', 0, '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')`, nil) + VALUES ('q1', 'Research', 0, '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')`, nil); err != nil { + t.Fatalf("setup quest_state: %v", err) + }cli/internal/herald/herald_test.go-179-181 (1)
179-181:⚠️ Potential issue | 🟡 MinorSame issue: unchecked error from test setup SQL.
🧪 Proposed fix
- sqlitex.Execute(conn, + if err := sqlitex.Execute(conn, `INSERT INTO quest_state (quest_name, phase, gate_pending, created_at, updated_at) - VALUES ('q1', 'Complete', 0, '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')`, nil) + VALUES ('q1', 'Complete', 0, '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z')`, nil); err != nil { + t.Fatalf("setup quest_state: %v", err) + }cli/internal/status/status.go-90-105 (1)
90-105:⚠️ Potential issue | 🟡 MinorError from fellowship query is silently ignored on Line 100.
If the
SELECTquery fails (e.g., schema mismatch, DB corruption),erris non-nil but ignored. The code only checkserr == nil && hasFellowship, meaning a real query error would be treated the same as "no fellowship found."🛡️ Proposed fix to propagate query errors
if err == nil && hasFellowship { result.Fellowship = &FellowshipInfo{ Name: fellowshipName, CreatedAt: fellowshipCreatedAt, } + } else if err != nil { + return nil, fmt.Errorf("status: load fellowship: %w", err) }cli/internal/dashboard/fellowship.go-477-486 (1)
477-486:⚠️ Potential issue | 🟡 MinorAll
LoadFellowshiperrors are treated as "fellowship not initialized".If
LoadFellowshipfails due to a DB error (corruption, schema mismatch), this returns an empty status instead of propagating the error. Consider distinguishing "not found" from other errors.🛡️ Proposed fix to distinguish error types
func DiscoverQuests(conn *sqlite.Conn) (*DashboardStatus, error) { fs, err := LoadFellowship(conn) if err != nil { - // No fellowship row — return empty status - return &DashboardStatus{ - Quests: []QuestStatus{}, - Scouts: []ScoutEntry{}, - Companies: []CompanyEntry{}, - }, nil + // Check if it's a "not initialized" error vs a real DB error + if strings.Contains(err.Error(), "not initialized") { + return &DashboardStatus{ + Quests: []QuestStatus{}, + Scouts: []ScoutEntry{}, + Companies: []CompanyEntry{}, + }, nil + } + return nil, err }Alternatively,
LoadFellowshipcould return a sentinel error for "not initialized" that callers can check witherrors.Is.cli/internal/state/state.go-72-74 (1)
72-74:⚠️ Potential issue | 🟡 Minor
json.Unmarshalerror is silently ignored.If the
auto_approvecolumn contains malformed JSON, the error is discarded ands.AutoApproveGatesremains nil. Consider logging or returning an error for data integrity.🛡️ Proposed fix to handle unmarshal error
if aa := stmt.ColumnText(10); aa != "" { - json.Unmarshal([]byte(aa), &s.AutoApproveGates) + if err := json.Unmarshal([]byte(aa), &s.AutoApproveGates); err != nil { + return fmt.Errorf("unmarshal auto_approve: %w", err) + } }cli/internal/autopsy/autopsy_test.go-452-465 (1)
452-465:⚠️ Potential issue | 🟡 MinorUnchecked error from
sqlitex.Executecould mask test failures.The error returned by
sqlitex.Executeon Line 454 is not captured. If the query fails,triggerremains an empty string and the test would fail with a misleading "trigger = '', want abandonment" instead of the actual DB error.🧪 Proposed fix to check the error
- sqlitex.Execute(conn, + err = sqlitex.Execute(conn, `SELECT trigger_type FROM autopsies WHERE id = ?`, &sqlitex.ExecOptions{ Args: []any{id}, ResultFunc: func(stmt *sqlite.Stmt) error { trigger = stmt.ColumnText(0) return nil }, }) + if err != nil { + t.Fatalf("query trigger_type: %v", err) + }
🧹 Nitpick comments (14)
cli/internal/dashboard/fellowship_test.go (1)
13-32: Fail on the outerWithTx/WithConnerror.These tests consistently drop the return from
d.WithTx(...)andd.WithConn(...). If begin/commit/connection acquisition fails, the callback path never makes the test fail and you can get a false green. Prefer returning setup errors from the callback and asserting the outer helper result at the call site.Also applies to: 37-53, 58-75, 80-100, 105-118, 123-132, 137-194, 199-205, 226-235, 240-273, 278-300, 305-320
cli/internal/db/migrate_test.go (2)
199-208: Uncheckedsqlitex.Executeerrors within test assertions.Throughout this test,
sqlitex.Executecalls ignore the returned error. If a query fails (e.g., wrong table name, schema mismatch), theResultFuncwon't execute and variables will retain their zero values, potentially causing misleading assertion failures.Consider checking errors from these calls to get clearer diagnostics when tests fail.
34-36: Ignoredjson.Marshalerrors in test fixture helpers.The
json.Marshalreturn value is discarded. While unlikely to fail for these simple structs, explicitly handling errors improves test robustness and serves as documentation.- data, _ := json.Marshal(fs) + data, err := json.Marshal(fs) + if err != nil { + t.Fatalf("marshal fellowship state: %v", err) + }This pattern repeats in
writeQuestState,writeQuestTome,writeQuestErrands,writeHerald,writeBulletin, andwriteAutopsy.cli/internal/tome/tome_test.go (1)
43-55: Ignored errors fromtome.RecordGateandtome.LoadGates.Lines 44-45 discard errors from
RecordGate, and Line 47 discards the error fromLoadGates. If these operations fail, the test assertions will be meaningless.🔧 Suggested fix pattern
- tome.RecordGate(conn, "q1", "Research", "submitted", "") - tome.RecordGate(conn, "q1", "Research", "approved", "") + if err := tome.RecordGate(conn, "q1", "Research", "submitted", ""); err != nil { + t.Fatalf("RecordGate submitted: %v", err) + } + if err := tome.RecordGate(conn, "q1", "Research", "approved", ""); err != nil { + t.Fatalf("RecordGate approved: %v", err) + } - gates, _ := tome.LoadGates(conn, "q1") + gates, err := tome.LoadGates(conn, "q1") + if err != nil { + t.Fatalf("LoadGates: %v", err) + }cli/internal/errand/errand_test.go (1)
47-73: Multiple ignored errors inTestAddSequentialIDs.Lines 48-50 discard errors from
errand.Add, and Line 62 discards the error fromerrand.List. If ID generation or listing fails, assertions will be misleading.cli/internal/eagles/eagles_test.go (1)
62-66:WithConnerror ignored in test body.The error from
d.WithConnis discarded. IfSweepfails, the test continues with a nilreport, which would panic on Line 71. Consider checking the error explicitly.🔧 Suggested fix
- err := d.WithConn(context.Background(), func(conn *db.Conn) error { + if err := d.WithConn(context.Background(), func(conn *db.Conn) error { var err error report, err = Sweep(conn, opts) return err - }) - if err != nil { + }); err != nil { t.Fatalf("Sweep: %v", err) }This pattern repeats across most test functions in this file.
cli/internal/dashboard/server_test.go (1)
190-194:WithConnerror from herald read is ignored.While the herald read failing won't affect the gate operation (per learnings), it would cause
tidingsto be nil, leading to a misleading test failure on Line 196. Consider checking the error for clearer test diagnostics.cli/internal/hooks/files.go (1)
21-30: Redundant deduplication check.Lines 22-30 manually check if the file already exists before calling
RecordFiles. However, per the relevant code snippet fromcli/internal/tome/tome.go:61-71,RecordFilesusesINSERT OR IGNORE, which handles duplicates at the database level.This pre-check adds a round-trip query and O(n) iteration that could be removed:
♻️ Simplified implementation
- // Check if file already recorded. - existing, err := tome.LoadFiles(conn, questName) - if err != nil { - return false - } - for _, f := range existing { - if f == filePath { - return false - } - } - if err := tome.RecordFiles(conn, questName, []string{filePath}); err != nil { return false } return trueHowever, this changes the return value semantics—currently it returns
falseif the file was already recorded (no modification), while the simplified version would returntrue. If the caller relies on knowing whether the tome was actually modified, consider usingconn.Changes()after the insert to detect if a row was added.cli/internal/herald/herald.go (1)
64-76: Consider extracting duplicatedResultFunclogic.The
ResultFuncthat maps columns to aTidingstruct is identical in bothRead(Lines 66-74) andReadAll(Lines 106-114). Extracting this to a helper could reduce duplication:func scanTiding(stmt *sqlite.Stmt) Tiding { return Tiding{ Timestamp: stmt.ColumnText(0), Quest: stmt.ColumnText(1), Type: TidingType(stmt.ColumnText(2)), Phase: stmt.ColumnText(3), Detail: stmt.ColumnText(4), } }cli/internal/bulletin/bulletin.go (1)
84-86: Inconsistent nil vs empty slice return.
Loadreturnsnil, nilwhen there are no rows (Lines 84-86), whileherald.Readandherald.ReadAllreturn an empty slice[]Tiding{}. This inconsistency could cause nil-related issues in callers and produces different JSON output (nullvs[]).♻️ Suggested fix for consistency
if len(rows) == 0 { - return nil, nil + return []Entry{}, nil }cli/internal/autopsy/autopsy_test.go (1)
443-465: Consider adding a comment or tag to make the abandoned quest scannable.The test assigns
matchesbut immediately discards it with_ = matches. While the direct DB query verification is valid, adding a tag duringInferor in the test setup would make the assertion more robust.cli/internal/herald/herald_test.go (1)
100-100: Error fromReadAllis discarded.Consider checking the error to ensure test failures are properly diagnosed if the query fails unexpectedly.
🧪 Proposed fix
- tidings, _ := ReadAll(conn, 3) + tidings, err := ReadAll(conn, 3) + if err != nil { + t.Fatal(err) + }cli/internal/state/state.go (1)
133-146:FindQuestreturns empty string for both "not found" and potential empty quest name.If no matching worktree exists,
nameis empty string witherr == nil. This is fine if empty quest names are invalid, but consider returning a sentinel error for "not found" to make the API clearer.cli/internal/db/db.go (1)
144-150: Redundant if-else: both branches return the same value.Both the
if filepath.Base(gitCommon) == ".git"branch and the else branch returnfilepath.Dir(gitCommon). This could be simplified.♻️ Proposed simplification
- // The main repo root is the parent of the .git directory. - if filepath.Base(gitCommon) == ".git" { - return filepath.Dir(gitCommon), nil - } - // For bare repos or unusual layouts, go up one level. - return filepath.Dir(gitCommon), nil + // The main repo root is the parent of the .git directory (or git common dir). + return filepath.Dir(gitCommon), nil
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3b80972-015a-478b-a9a8-3876a17ddaf8
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
cli/cmd/fellowship/main.gocli/go.modcli/internal/autopsy/autopsy.gocli/internal/autopsy/autopsy_test.gocli/internal/bulletin/bulletin.gocli/internal/bulletin/bulletin_test.gocli/internal/company/company.gocli/internal/company/company_test.gocli/internal/dashboard/fellowship.gocli/internal/dashboard/fellowship_test.gocli/internal/dashboard/server.gocli/internal/dashboard/server_test.gocli/internal/db/db.gocli/internal/db/db_test.gocli/internal/db/migrate.gocli/internal/db/migrate_test.gocli/internal/db/schema.gocli/internal/db/testutil.gocli/internal/eagles/eagles.gocli/internal/eagles/eagles_test.gocli/internal/errand/errand.gocli/internal/errand/errand_test.gocli/internal/filelock/filelock_unix.gocli/internal/filelock/filelock_windows.gocli/internal/herald/herald.gocli/internal/herald/herald_test.gocli/internal/herald/problems.gocli/internal/herald/problems_test.gocli/internal/hooks/completion.gocli/internal/hooks/files.gocli/internal/hooks/files_test.gocli/internal/hooks/submit.gocli/internal/state/state.gocli/internal/state/state_test.gocli/internal/status/status.gocli/internal/status/status_test.gocli/internal/tome/tome.gocli/internal/tome/tome_test.go
💤 Files with no reviewable changes (2)
- cli/internal/filelock/filelock_windows.go
- cli/internal/filelock/filelock_unix.go
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (22)
cli/internal/db/schema.go (3)
31-54:⚠️ Potential issue | 🟠 MajorDon't hang permanent tome history off
quest_state.
quest_phases,quest_gates, andquest_filesall point at the transient state row. If state is pruned after completion, you'll either block deletion or be forced to keep stalequest_staterows just to preserve history. Anchor these tables to a durable quest identity instead.
68-74:⚠️ Potential issue | 🟠 MajorAdd the missing FK for
depends_on.The current table validates only the dependent errand. A row can still reference a prerequisite that doesn't exist, leaving dangling edges in the dependency graph.
Minimal schema fix
`CREATE TABLE IF NOT EXISTS errand_deps ( quest_name TEXT NOT NULL, errand_id TEXT NOT NULL, depends_on TEXT NOT NULL, PRIMARY KEY (quest_name, errand_id, depends_on), - FOREIGN KEY (quest_name, errand_id) REFERENCES errands(quest_name, id) + FOREIGN KEY (quest_name, errand_id) REFERENCES errands(quest_name, id), + FOREIGN KEY (quest_name, depends_on) REFERENCES errands(quest_name, id) )`,
187-202:⚠️ Potential issue | 🟠 MajorCapture the mutable quest fields in the changelog.
The triggers still omit
held_reason,gate_id,task_id,team_name, andauto_approve, and the insert trigger also skipslembas_completed/metadata_updated. That leaves the audit trail unable to explain holds, reassignments, or gate changes.cli/internal/eagles/eagles.go (1)
109-110:⚠️ Potential issue | 🟠 MajorDon't make zombie/respawn decisions depend on herald rows.
lastActivitytreats any herald row as authoritative even whenquest_state.updated_atis newer, andhasCheckpointignores the already-loadeds.LembasCompletedflag. Because herald writes are best-effort, missing or delayed log rows can flip an active quest tozombieor downgrade arespawnto anudge. Use state-backed fields as the source of truth and keep herald as supplemental history only.Based on learnings, herald.Announce calls should be non-fatal and best-effort: do not surface errors to clients or rely on logging for control flow.
Also applies to: 177-190, 202-235
cli/internal/hooks/completion.go (1)
25-26:⚠️ Potential issue | 🟠 MajorPropagate the completion-status write error.
Line 26 ignores
tome.SetStatus's result, so a failed DB write leavesfellowship_quests.statusstale with no signal to the caller.Minimal fix
// MarkTomeCompleted marks the quest tome status as "completed". -func MarkTomeCompleted(conn *sqlite.Conn, questName string) { - tome.SetStatus(conn, questName, "completed") +func MarkTomeCompleted(conn *sqlite.Conn, questName string) error { + return tome.SetStatus(conn, questName, "completed") }cli/internal/herald/problems.go (1)
31-33: 🛠️ Refactor suggestion | 🟠 MajorConsider returning an error from
DetectProblems.The current signature
DetectProblems(conn *db.Conn) []Problemcannot signal DB failures. Callers cannot distinguish between "no problems found" and "query failed."♻️ Suggested signature change
-func DetectProblems(conn *db.Conn) []Problem { +func DetectProblems(conn *db.Conn) ([]Problem, error) { var problems []Problem // Query all active quests (not Complete). type questInfo struct { questName string phase string gatePending bool gateID string } var quests []questInfo - _ = sqlitex.Execute(conn, + err := sqlitex.Execute(conn, `SELECT quest_name, phase, gate_pending, gate_id FROM quest_state WHERE phase != 'Complete'`, ... ) + if err != nil { + return nil, fmt.Errorf("detect problems: load quests: %w", err) + }cli/internal/dashboard/server.go (8)
412-424:⚠️ Potential issue | 🟠 MajorHerald read errors silently return empty array.
If
herald.ReadAllfails, the error is returned from the callback but then ignored, and an empty array is returned to the client.
439-451:⚠️ Potential issue | 🟠 MajorBulletin load errors silently return empty array.
Same pattern as herald — DB errors are swallowed and an empty array is returned.
65-81:⚠️ Potential issue | 🟠 MajorDB errors collapsed into "invalid worktree directory" response.
The
validWorktreeDirhelper returnsfalseon anyDiscoverQuestserror, causing gate endpoints to return400 Bad Requestfor server-side DB outages.Consider returning
(bool, error)and mapping DB errors to500 Internal Server Error.
128-140:⚠️ Potential issue | 🟠 MajorHerald errors should not roll back gate operations.
Returning
herald.Announceerrors here causes the transaction to roll back even though the state mutation succeeded. This violates the fire-and-forget pattern. Based on learnings: herald.Announce calls should be non-fatal and best-effort; state mutations should be committed before the herald log write.🐛 Proposed fix
if err := state.Upsert(conn, st); err != nil { return err } now := time.Now().UTC().Format(time.RFC3339) - if err := herald.Announce(conn, herald.Tiding{ + // Fire-and-forget: don't let herald errors affect the gate operation + herald.Announce(conn, herald.Tiding{ Timestamp: now, Quest: st.QuestName, Type: herald.GateApproved, Phase: prevPhase, Detail: fmt.Sprintf("Gate approved for %s", prevPhase), - }); err != nil { - return err - } - if err := herald.Announce(conn, herald.Tiding{ + }) + herald.Announce(conn, herald.Tiding{ Timestamp: now, Quest: st.QuestName, Type: herald.PhaseTransition, Phase: st.Phase, Detail: fmt.Sprintf("Phase advanced from %s to %s", prevPhase, st.Phase), - }); err != nil { - return err - } + })
142-152:⚠️ Potential issue | 🟠 MajorResponse sent before transaction commits.
The HTTP response is encoded inside the
WithTxcallback. If the transaction commit fails after the response is written, the client observes a successful operation that was actually rolled back.Move the response encoding outside the transaction callback, after
WithTxreturns successfully.
197-203:⚠️ Potential issue | 🟠 MajorHerald errors should not roll back gate rejection.
Same issue as the approve handler —
herald.Announceerrors roll back the gate state change. Based on learnings: herald.Announce calls should be non-fatal.
205-215:⚠️ Potential issue | 🟠 MajorResponse sent before transaction commits.
Same issue as the approve handler — response is encoded inside the transaction callback.
378-395:⚠️ Potential issue | 🟠 MajorDB errors silently return "not found" instead of server error.
If
state.FindQuestorerrand.Listfails due to a DB issue, the handler returns404instead of500. This masks server-side failures.🐛 Proposed fix
var errands []errand.Errand -s.db.WithConn(context.Background(), func(conn *db.Conn) error { +var dbErr error +s.db.WithConn(context.Background(), func(conn *db.Conn) error { questName, findErr := state.FindQuest(conn, dir) if findErr != nil || questName == "" { - return nil + dbErr = findErr + return nil } - errands, _ = errand.List(conn, questName) + var listErr error + errands, listErr = errand.List(conn, questName) + if listErr != nil { + dbErr = listErr + } return nil }) +if dbErr != nil { + http.Error(w, dbErr.Error(), http.StatusInternalServerError) + return +} if errands == nil {cli/internal/db/migrate.go (3)
54-67:⚠️ Potential issue | 🟠 Major
auto_approve_gatesmigration drops array data.The
questStateJSONstruct definesAutoApproveas*string, but the actual JSON schema usesauto_approve_gatesas a string slice. This will silently drop auto-approve configurations during migration.🐛 Proposed fix to handle the array field
type questStateJSON struct { Version int `json:"version"` QuestName string `json:"quest_name"` TaskID string `json:"task_id"` TeamName string `json:"team_name"` Phase string `json:"phase"` GatePending bool `json:"gate_pending"` GateID *string `json:"gate_id"` LembasCompleted bool `json:"lembas_completed"` MetadataUpdated bool `json:"metadata_updated"` Held bool `json:"held"` HeldReason *string `json:"held_reason"` - AutoApprove *string `json:"auto_approve"` + AutoApproveGates []string `json:"auto_approve_gates"` }Then update
migrateQuestState(around line 444-447):- var autoApprove any - if qs.AutoApprove != nil { - autoApprove = *qs.AutoApprove + var autoApprove any + if len(qs.AutoApproveGates) > 0 { + b, _ := json.Marshal(qs.AutoApproveGates) + autoApprove = string(b) }
149-151:⚠️ Potential issue | 🟠 MajorHard-coded
.fellowshipignores custom data-dir configuration.The migration hard-codes
.fellowshipfor the data directory path. Users who configured a custom data-dir via project or user config will be told there's nothing to migrate, while their legacy state files remain untouched.Consider using
datadir.Name()fromcli/internal/datadir/datadir.goto respect the configured data directory.
719-737:⚠️ Potential issue | 🟠 MajorWorktree discovery failure causes silent partial migration.
If
git worktree listfails, the code silently falls back to[mainRepo]and migration proceeds. This can result in main-repo files being migrated to SQLite while worktree JSON files remain behind, creating an inconsistent state.Consider either:
- Failing the migration with a clear error when worktree discovery fails
- Warning the user explicitly and requiring confirmation to proceed with partial migration
cli/internal/errand/errand.go (1)
57-96:⚠️ Potential issue | 🟠 MajorID generation race condition under concurrent writes.
The ID generation queries
MAX(id)then inserts with the computed value in separate statements. Concurrent transactions could race and generate duplicate IDs.Consider using SQLite's auto-increment
rowidwith a computed ID format on read, or adding a UNIQUE constraint with retry logic.cli/cmd/fellowship/main.go (2)
254-270:⚠️ Potential issue | 🔴 CriticalFail closed if quest lookup fails in hook mode.
If
WithConnorstate.FindQuesterrors here,questNamestays empty and the hook drops into the "lead session" path, returning0as if it were outside a quest. That bypasses gate/completion safeguards during a DB outage.
743-760:⚠️ Potential issue | 🟠 MajorOnly fall back to create on a true "not found".
state.Loadalso returns real DB/query failures. Treating every non-nil error as absence can create or reset the wrong quest row after a transient read failure.cli/internal/tome/tome.go (1)
157-173:⚠️ Potential issue | 🟠 MajorReturn
fellowship_questslookup failures fromLoad.This query's error is discarded, so a DB failure here produces a synthetic
"active"tome with emptyTaskand zeroRespawnsas if the load succeeded. Only defaultStatuswhen no row is found; propagate real query errors.cli/internal/autopsy/autopsy.go (1)
75-77:⚠️ Potential issue | 🟠 MajorConfigured autopsy retention is still a no-op.
Createalways stampsExpiresAtwithDefaultExpiryDays, andScanstill ignores itsexpiryDaysparameter. The CLI readsdatadir.AutopsyExpiryDays(...), but that setting never affects created or inferred autopsies.Also applies to: 134-135
🧹 Nitpick comments (14)
cli/internal/hooks/files.go (1)
21-33: Let the unique key handle deduplication.This read-before-write adds an extra query on every hook and still races under concurrent writers. Moving the dedupe into
tome.RecordFilesand letting the existing quest/file key arbitrate duplicates would make the write atomic and avoid loading the full file list first.cli/internal/bulletin/bulletin_test.go (2)
12-28: Unchecked errors in test code may hide failures.Several calls ignore errors that could indicate test setup issues:
- Line 13:
Post(conn, ...)error not checked- Line 17: Only
Loaderror is checkedWhile test code is less critical, silently ignoring errors can mask bugs in the functions being tested.
♻️ Suggested improvement
d.WithTx(context.Background(), func(conn *db.Conn) error { - Post(conn, Entry{ + if err := Post(conn, Entry{ Timestamp: "2026-01-01T00:00:00Z", Quest: "q1", Topic: "auth", Files: []string{"auth.go"}, Discovery: "needs refactor", - }) + }); err != nil { + t.Fatal(err) + } entries, err := Load(conn)
50-62: UncheckedClearcall.
Clear(conn)at line 56 may fail silently if the delete operations encounter an error.♻️ Suggested fix
- Clear(conn) + if err := Clear(conn); err != nil { + t.Fatal(err) + }cli/internal/status/status.go (1)
78-79: Type inconsistency: using*sqlite.Conndirectly instead of*db.Conn.The function signature uses
*sqlite.Conndirectly, but other modules in this PR (e.g.,bulletin.go,company.go) use the*db.Conntype alias defined incli/internal/db/db.go. Using the alias consistently across the codebase improves maintainability if the underlying type changes.♻️ Suggested fix
+import "github.com/justinjdev/fellowship/cli/internal/db" + // Scan discovers fellowship quest state from the DB and git worktrees for crash recovery. -func Scan(conn *sqlite.Conn, gitRoot string) (*StatusResult, error) { +func Scan(conn *db.Conn, gitRoot string) (*StatusResult, error) {cli/internal/company/company_test.go (1)
323-334: Consider usingWithTxfor consistency.
TestList_NoCompaniesusesWithConnwhile similar tests useWithTx. WhileWithConnworks for read-only operations, usingWithTxconsistently across tests ensures predictable isolation and rollback behavior.cli/internal/bulletin/bulletin.go (1)
118-144: Scan loads all entries before filtering — consider SQL-level filtering for large datasets.
ScancallsLoadto fetch all bulletin entries, then filters in Go. For large bulletin tables, this could be inefficient. However, given the current in-memory matching logic for bidirectional path containment, pushing this to SQL would be complex.This is acceptable for now if the bulletin table remains reasonably sized.
cli/internal/hooks/enrich.go (1)
14-14: Type inconsistency: using*sqlite.Conndirectly.Similar to
status.go, this file uses*sqlite.Conndirectly instead of the*db.Connalias. For consistency with other modules in this PR (bulletin, company, herald), consider using the alias.♻️ Suggested fix
+import "github.com/justinjdev/fellowship/cli/internal/db" + // GatherEnrichment collects quest metrics from the DB and worktree directory // and returns a formatted enrichment block to append to gate messages. // Returns empty string if no data sources are available. -func GatherEnrichment(conn *sqlite.Conn, questName string, dir string) string { +func GatherEnrichment(conn *db.Conn, questName string, dir string) string {And similarly for the helper functions at lines 30, 38, and 58.
Also applies to: 20-20
cli/internal/state/state_test.go (1)
46-65: Upsert errors ignored in update test.Lines 50 and 54 discard errors from
state.Upsert. If the initial insert or update fails, the test may pass or fail for wrong reasons.♻️ Suggested fix
d.WithTx(context.Background(), func(conn *db.Conn) error { s := &state.State{QuestName: "q1", Phase: "Onboard"} - state.Upsert(conn, s) + if err := state.Upsert(conn, s); err != nil { + t.Fatal(err) + } s.Phase = "Research" s.GatePending = true - state.Upsert(conn, s) + if err := state.Upsert(conn, s); err != nil { + t.Fatal(err) + }cli/internal/dashboard/fellowship_test.go (1)
38-52: Consider checking errors from CRUD operations in tests.Multiple test functions ignore errors returned by
AddQuest,AddScout,AddCompany,UpdateQuest,RemoveQuest, andListQuests. While this simplifies the test code, it can mask failures and make debugging harder if the underlying operations fail unexpectedly.For example, at Line 42
quests, _ := ListQuests(conn)silently ignores potential DB errors.💡 Suggested improvement for explicit error handling
- AddQuest(conn, QuestEntry{ + if err := AddQuest(conn, QuestEntry{ Name: "q1", TaskDescription: "build auth", Worktree: "/tmp/wt/q1", Branch: "feat/q1", - }) - quests, _ := ListQuests(conn) + }); err != nil { + t.Fatal(err) + } + quests, err := ListQuests(conn) + if err != nil { + t.Fatal(err) + }cli/internal/autopsy/autopsy_test.go (1)
443-465: Clean up unusedmatchesvariable.The
matchesvariable is assigned but immediately discarded with_ = matches. The comment explains the workaround, but this leaves dead code. Either remove theScancall entirely or use it for an assertion.♻️ Suggested cleanup
- matches, err := Scan(conn, ScanOptions{Modules: []string{"quest-abandoned"}}, 90) - if err != nil { - t.Fatal(err) - } - // No files means no modules, so search by the quest name directly - // Actually, let's just verify via a tag/module-less scan won't work; - // instead query directly - _ = matches - // Verify the autopsy was created by looking at the DB directly var trigger stringcli/internal/herald/herald_test.go (1)
90-106: Check error fromReadAllfor test reliability.Line 100 ignores the error from
ReadAll(conn, 3). If the function fails, the test would pass with incorrect assertions.💡 Suggested fix
- tidings, _ := ReadAll(conn, 3) + tidings, err := ReadAll(conn, 3) + if err != nil { + t.Fatal(err) + }cli/internal/dashboard/fellowship.go (1)
224-255: Delete-all-then-reinsert pattern is inefficient but acceptable.
SaveFellowshipclears all quests, scouts, and companies before re-inserting. While this works correctly, it's less efficient than diffing and applying targeted changes. For the expected scale of fellowship state, this is acceptable.cli/internal/errand/errand.go (2)
148-155: N+1 query pattern when loading dependencies.The
Listfunction loads dependencies for each errand in a loop, resulting in N+1 queries. For small errand counts this is acceptable, but could be optimized with a single JOIN query if performance becomes a concern.💡 Optimized approach using a single query
// Load all dependencies in one query depsMap := make(map[string][]string) err = sqlitex.Execute(conn, `SELECT errand_id, depends_on FROM errand_deps WHERE quest_name = :quest`, &sqlitex.ExecOptions{ Named: map[string]any{":quest": quest}, ResultFunc: func(stmt *sqlite.Stmt) error { id := stmt.ColumnText(0) dep := stmt.ColumnText(1) depsMap[id] = append(depsMap[id], dep) return nil }, }) // Then assign to items for i := range items { items[i].DependsOn = depsMap[items[i].ID] }
47-55: Init is now a no-op — consider removing or documenting the purpose.The
Initfunction does nothing since errands are stored per-row with a quest_name FK. Consider either removing it to avoid confusion, or adding a doc comment explaining why it exists (e.g., for API compatibility).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 067362f8-3fd7-4acc-b323-9c739f2d0eae
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
cli/cmd/fellowship/main.gocli/go.modcli/internal/autopsy/autopsy.gocli/internal/autopsy/autopsy_test.gocli/internal/bulletin/bulletin.gocli/internal/bulletin/bulletin_test.gocli/internal/company/company.gocli/internal/company/company_test.gocli/internal/dashboard/fellowship.gocli/internal/dashboard/fellowship_test.gocli/internal/dashboard/server.gocli/internal/dashboard/server_test.gocli/internal/db/db.gocli/internal/db/db_test.gocli/internal/db/migrate.gocli/internal/db/migrate_test.gocli/internal/db/schema.gocli/internal/db/testutil.gocli/internal/eagles/eagles.gocli/internal/eagles/eagles_test.gocli/internal/errand/errand.gocli/internal/errand/errand_test.gocli/internal/filelock/filelock_unix.gocli/internal/filelock/filelock_windows.gocli/internal/herald/herald.gocli/internal/herald/herald_test.gocli/internal/herald/problems.gocli/internal/herald/problems_test.gocli/internal/hooks/completion.gocli/internal/hooks/enrich.gocli/internal/hooks/enrich_test.gocli/internal/hooks/files.gocli/internal/hooks/files_test.gocli/internal/hooks/submit.gocli/internal/state/state.gocli/internal/state/state_test.gocli/internal/status/status.gocli/internal/status/status_test.gocli/internal/tome/tome.gocli/internal/tome/tome_test.go
💤 Files with no reviewable changes (2)
- cli/internal/filelock/filelock_unix.go
- cli/internal/filelock/filelock_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/internal/db/migrate_test.go
…ixes Production code: - Fail closed on quest lookup DB errors in runHook (critical) - Add ErrNotFound sentinel to state.Load; distinguish in runInit - server.go: return (bool, error) from validWorktreeDir; move herald calls to best-effort after tx commit; encode responses outside tx; surface DB errors as 500 in read handlers - migrate.go: fix auto_approve_gates field name; fail on worktree discovery errors instead of silent fallback - schema.go: add FK for errand_deps.depends_on; expand changelog triggers to include held_reason, gate_id, task_id, team_name, auto_approve - Return errors from MarkTomeCompleted, RecordGateSubmitted, DetectProblems; propagate in callers - Remove redundant dedup in FileTrack (INSERT OR IGNORE suffices) - Return empty slice instead of nil from bulletin.Load - Return json.Unmarshal error in state.Load for auto_approve - Simplify redundant if/else in db.resolveMainRepo Tests: - Check WithTx/WithConn/sqlitex.Execute errors across all test files - Check json.Marshal errors in migrate test fixtures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create .fellowship/ directory before opening DB (critical — new repos) - Wrap expires_at in datetime() for correct expiry comparison in autopsy - Return fellowship query error in status.Scan instead of ignoring - Fix remaining unchecked test errors (db_test, server_test) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/cmd/fellowship/main.go (2)
717-720:⚠️ Potential issue | 🟡 Minor
runInitrejects a valid phase.
state.phaseOrderincludesAdversarial, but this allowlist does not, sofellowship init --phase Adversarialfails even though the rest of the state machine accepts it.Suggested fix
validPhases := map[string]bool{ "Onboard": true, "Research": true, "Plan": true, - "Implement": true, "Review": true, "Complete": true, + "Implement": true, "Adversarial": true, "Review": true, "Complete": true, }
1388-1391:⚠️ Potential issue | 🟡 MinorThe invalid-status hint is out of sync with
ValidStatus.
errand.ValidStatusacceptsin_progressandskipped, but this message still advertisesactiveand omits both supported values. That steers users toward an invalid status value.Suggested fix
- fmt.Fprintf(os.Stderr, "fellowship: invalid status %q (use: pending, active, done, blocked)\n", statusStr) + fmt.Fprintf(os.Stderr, "fellowship: invalid status %q (use: pending, in_progress, done, blocked, skipped)\n", statusStr)
♻️ Duplicate comments (4)
cli/internal/db/db.go (1)
35-42:⚠️ Potential issue | 🔴 CriticalCreate the DB directory before opening it.
Lines 35-42 still assume
filepath.Dir(dbPath)already exists. On a brand-new repo,OpenPathwill fail beforeinitgets a chance to create that directory.🛠 Proposed fix
import ( "context" "fmt" + "os" "os/exec" "path/filepath" "strings" @@ func OpenPath(dbPath string) (*DB, error) { + if err := os.MkdirAll(filepath.Dir(dbPath), 0o755); err != nil { + return nil, fmt.Errorf("db: mkdir %s: %w", filepath.Dir(dbPath), err) + } + pool, err := sqlitex.NewPool(dbPath, sqlitex.PoolOptions{ PoolSize: 1, Flags: sqlite.OpenReadWrite | sqlite.OpenCreate | sqlite.OpenWAL, })cli/internal/db/migrate.go (1)
150-151:⚠️ Potential issue | 🟠 MajorRespect the configured data-dir during migration discovery and backup.
These paths are still pinned to
.fellowship. Repos/worktrees using a configured data-dir name will be told there is nothing to migrate, and the backup/remove phase will target the wrong directory tree.🛠 Proposed fix
import ( "bufio" "context" "encoding/json" "fmt" @@ "os/exec" "path/filepath" "strings" "time" + "github.com/justinjdev/fellowship/cli/internal/datadir" "zombiezen.com/go/sqlite/sqlitex" ) @@ - dataDir := filepath.Join(mainRepo, ".fellowship") + dataDir := filepath.Join(mainRepo, datadir.Name()) backupDir := filepath.Join(dataDir, "backup") @@ - mainDataDir := filepath.Join(mainRepo, ".fellowship") + mainDataDir := filepath.Join(mainRepo, datadir.Name()) @@ - wtDataDir := filepath.Join(wt, ".fellowship") + wtDataDir := filepath.Join(wt, datadir.Name())Also applies to: 219-233
cli/cmd/fellowship/main.go (1)
474-481:⚠️ Potential issue | 🟠 MajorSurface lookup failures instead of treating them as “no quest”.
These paths discard the
WithConn/state.FindQuesterror and then continue with an empty quest name. During a DB/query failure, gate/tome/errand commands will report "no quest found" instead of surfacing the real outage. Return the lookup error and only take the empty-result path when the query actually succeeded with no match.Also applies to: 1172-1183, 1938-1947
cli/internal/dashboard/server_test.go (1)
161-169:⚠️ Potential issue | 🟡 MinorCheck the override transaction result here too.
If this
WithTxcall fails, the test keeps the original pending-gate row and exercises the wrong scenario. This is the same swallowed-transaction pattern already fixed insetupTestDB.Suggested fix
- d.WithTx(context.Background(), func(conn *db.Conn) error { - return state.Upsert(conn, &state.State{ - QuestName: "quest-login", - TaskID: "t1", - TeamName: "team", - Phase: "Plan", - GatePending: false, - }) - }) + if err := d.WithTx(context.Background(), func(conn *db.Conn) error { + return state.Upsert(conn, &state.State{ + QuestName: "quest-login", + TaskID: "t1", + TeamName: "team", + Phase: "Plan", + GatePending: false, + }) + }); err != nil { + t.Fatalf("override quest state: %v", err) + }
🧹 Nitpick comments (3)
cli/internal/company/company_test.go (1)
257-313: Add the best-effort herald failure case.Lines 257-313 only prove the happy path. A regression that makes
BatchApprovefail or report partial approval when herald logging fails would still pass here, even though that behavior is supposed to stay fire-and-forget.Based on learnings,
herald.Announceerrors should not affect gate operation results and logging should remain best-effort.cli/internal/dashboard/server.go (2)
302-357: Herald calls inside transaction may block commit on logging failure.
batchApproveCompanycallsherald.Announce(lines 343-350) inside the transaction passed fromhandleCompanyApprove. Ifherald.Announcefails, the transaction still commits (error is ignored), but this differs from the pattern inhandleGateApprove/handleGateRejectwhere herald calls happen after the tx commits.For consistency with the learned pattern (herald calls are best-effort and should occur after state mutations commit), consider collecting the approved quests and making herald announcements after the transaction returns successfully.
Suggested approach
func batchApproveCompany(conn *db.Conn, c CompanyEntry, fs *FellowshipState) (approved []string, errs []error) { + type announcement struct { + quest, prevPhase, nextPhase string + } + var toAnnounce []announcement + for _, qName := range c.Quests { // ... state load and mutation ... if err := state.Upsert(conn, st); err != nil { errs = append(errs, fmt.Errorf("saving state for %s: %w", qName, err)) continue } - now := time.Now().UTC().Format(time.RFC3339) - herald.Announce(conn, herald.Tiding{...}) - herald.Announce(conn, herald.Tiding{...}) + toAnnounce = append(toAnnounce, announcement{qName, prevPhase, nextPhase}) approved = append(approved, qName) } - return approved, errs + return approved, errs, toAnnounce }Then make herald calls in
handleCompanyApproveafter the transaction commits.Based on learnings: herald.Announce calls should be non-fatal and best-effort; state mutations should be committed before the herald log write.
352-352: Dead assignment:_ = wtis unused.The
wtvariable is assigned (lines 305-311) but only used in a blank assignment. If the worktree isn't needed for DB operations, remove the lookup to avoid confusion.Suggested cleanup
func batchApproveCompany(conn *db.Conn, c CompanyEntry, fs *FellowshipState) (approved []string, errs []error) { for _, qName := range c.Quests { - // Find worktree from fellowship quests - var wt string - for _, q := range fs.Quests { - if q.Name == qName { - wt = q.Worktree - break - } - } - st, err := state.Load(conn, qName) // ... - _ = wt // worktree used for context but not needed for DB operations approved = append(approved, qName) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3171dae-72ef-426c-bdda-4ebdd378e064
📒 Files selected for processing (25)
cli/cmd/fellowship/main.gocli/internal/autopsy/autopsy_test.gocli/internal/bulletin/bulletin.gocli/internal/bulletin/bulletin_test.gocli/internal/company/company_test.gocli/internal/dashboard/fellowship_test.gocli/internal/dashboard/server.gocli/internal/dashboard/server_test.gocli/internal/db/db.gocli/internal/db/db_test.gocli/internal/db/migrate.gocli/internal/db/migrate_test.gocli/internal/db/schema.gocli/internal/errand/errand_test.gocli/internal/herald/herald_test.gocli/internal/herald/problems.gocli/internal/herald/problems_test.gocli/internal/hooks/completion.gocli/internal/hooks/files.gocli/internal/hooks/files_test.gocli/internal/hooks/submit.gocli/internal/state/state.gocli/internal/status/status_test.gocli/internal/tome/tome.gocli/internal/tome/tome_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/internal/db/schema.go
- cli/internal/hooks/files.go
- cli/internal/db/db_test.go
| if err := d.WithTx(ctx, func(conn *db.Conn) error { | ||
| s, err := state.Load(conn, questName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| changed := hooks.GatePrereq(s, input) | ||
| if changed { | ||
| if err := state.Upsert(conn, s); err != nil { | ||
| return err | ||
| } | ||
| herald.Announce(conn, herald.Tiding{ | ||
| Timestamp: time.Now().UTC().Format(time.RFC3339), | ||
| Quest: questName, | ||
| Type: herald.LembasCompleted, | ||
| Phase: s.Phase, | ||
| Detail: "Lembas skill completed", | ||
| }) |
There was a problem hiding this comment.
Emit herald tidings after the write transaction commits.
All of these herald.Announce calls still run inside the same transaction as the state/tome mutation. That keeps the core operation coupled to best-effort logging and can still let a herald write interfere with commit latency or rollback behavior. Build the tiding payload in WithTx, commit the state change, then write herald in a separate best-effort call that ignores announce errors. Based on learnings: herald.Announce calls should be non-fatal and best-effort, and state mutations should be committed before the herald log write.
Also applies to: 387-410, 438-454, 510-544, 555-576, 614-641, 677-697
There was a problem hiding this comment.
Keeping herald inside the transaction is intentional. Herald is an append-only log in the same SQLite DB — it can't fail independently from the transaction. Moving it outside would create orphaned herald entries on rollback (state rolled back but log entry persists). The current design ensures atomicity: state + log are consistent.
There was a problem hiding this comment.
@justinjdev, that's a compelling rationale — with herald now stored in the same SQLite database, keeping Announce inside the transaction is the correct approach. Atomicity between the state mutation and the log entry is exactly the right guarantee here: a rollback (e.g., on a failed state.Upsert) will cleanly unwind both, preventing orphaned herald entries. The previous best-effort/fire-and-forget guidance was specific to the file-based herald implementation where writes went to a separate JSONL file on disk. I'll update the long-term memory accordingly.
The old learnings about file-based herald being non-fatal are now stale — would you like me to remove them?
✏️ Learnings added
Learnt from: justinjdev
Repo: justinjdev/fellowship PR: 78
File: cli/cmd/fellowship/main.go:317-333
Timestamp: 2026-03-13T16:53:50.790Z
Learning: In the fellowship codebase (post-PR `#78` SQLite migration), herald.Announce calls are intentionally kept inside the same SQLite transaction as the state mutation (e.g., state.Upsert, tome.RecordGate). Herald is an append-only log in the same SQLite DB, so it participates in the same transaction for atomicity. Moving herald outside the transaction would create orphaned log entries if the state write rolls back. This is a deliberate design choice — do not flag herald.Announce inside WithTx as a smell or suggest moving it outside.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: justinjdev
Repo: justinjdev/fellowship PR: 41
File: cli/internal/dashboard/server.go:119-127
Timestamp: 2026-03-08T18:47:26.151Z
Learning: In Go code, herald.Announce calls should be non-fatal and best-effort: do not surface errors to clients or rely on logging for control flow. Ensure that state mutations (e.g., quest phase advancement, gate approvals) are committed before the herald log write, and avoid returning errors from Announce that could lead to retries or double-apply. Apply this non-fatal logging convention to all herald.Announce usages across the codebase.
Learnt from: justinjdev
Repo: justinjdev/fellowship PR: 41
File: cli/internal/company/company.go:111-119
Timestamp: 2026-03-08T18:47:19.908Z
Learning: In Go code across the repository, do not let herald.Announce() errors affect gate operation results. These calls are fire-and-forget by design; the gate path (approve, reject, hold, unhold, and related hooks) should not fail due to logging. Ensure this pattern is consistent with the fellowship codebase (e.g., main.go and cli/internal/company/company.go) and document expected behavior wherever herald logging is performed.
🧠 Learnings used
Learnt from: justinjdev
Repo: justinjdev/fellowship PR: 41
File: cli/internal/dashboard/server.go:119-127
Timestamp: 2026-03-08T18:47:26.151Z
Learning: In Go code, herald.Announce calls should be non-fatal and best-effort: do not surface errors to clients or rely on logging for control flow. Ensure that state mutations (e.g., quest phase advancement, gate approvals) are committed before the herald log write, and avoid returning errors from Announce that could lead to retries or double-apply. Apply this non-fatal logging convention to all herald.Announce usages across the codebase.
Learnt from: justinjdev
Repo: justinjdev/fellowship PR: 41
File: cli/internal/company/company.go:111-119
Timestamp: 2026-03-08T18:47:19.908Z
Learning: In Go code across the repository, do not let herald.Announce() errors affect gate operation results. These calls are fire-and-forget by design; the gate path (approve, reject, hold, unhold, and related hooks) should not fail due to logging. Ensure this pattern is consistent with the fellowship codebase (e.g., main.go and cli/internal/company/company.go) and document expected behavior wherever herald logging is performed.
| // Find quest for the given worktree dir. | ||
| var questName string | ||
| d.WithConn(ctx, func(conn *db.Conn) error { | ||
| questName, _ = state.FindQuest(conn, *dir) | ||
| return nil | ||
| }) | ||
| if questName == "" { | ||
| questName = filepath.Base(*dir) | ||
| } |
There was a problem hiding this comment.
Don't guess the quest from filepath.Base(dir) here.
If the worktree lookup misses, hold/unhold fall back to the directory basename and may mutate an unrelated quest with the same name. In the DB-backed model, --dir should resolve through fellowship_quests.worktree; otherwise these commands should fail with a clear lookup error.
Also applies to: 668-675
| if err := d.WithTx(ctx, func(conn *db.Conn) error { | ||
| // Query all quest_state rows that have stale flags. | ||
| type staleQuest struct { | ||
| name string | ||
| gatePending bool | ||
| held bool | ||
| } | ||
| statePath := filepath.Join(worktreesDir, entry.Name(), datadir.Name(), "quest-state.json") | ||
| if _, err := os.Stat(statePath); err != nil { | ||
| if !os.IsNotExist(err) { | ||
| fmt.Fprintf(os.Stderr, "fellowship: warning: could not access %s: %v\n", statePath, err) | ||
| } | ||
| continue | ||
| var stale []staleQuest | ||
| if err := sqliteExecRows(conn, `SELECT quest_name, gate_pending, held FROM quest_state WHERE gate_pending = 1 OR held = 1`, | ||
| func(name string, gp, h bool) { | ||
| stale = append(stale, staleQuest{name, gp, h}) | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| var prevPending, prevHeld bool | ||
| err := state.WithLock(statePath, func(s *state.State) error { | ||
| if !s.GatePending && !s.Held { | ||
| return state.ErrNoSave | ||
|
|
||
| for _, sq := range stale { | ||
| s, err := state.Load(conn, sq.name) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| prevPending, prevHeld = s.GatePending, s.Held | ||
| s.GatePending = false | ||
| s.GateID = nil | ||
| s.Held = false | ||
| s.HeldReason = nil | ||
| return nil | ||
| }) | ||
| if err == state.ErrNoSave { | ||
| continue | ||
| } | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "fellowship: warning: could not save %s: %v\n", statePath, err) | ||
| continue | ||
| if err := state.Upsert(conn, s); err != nil { | ||
| fmt.Fprintf(os.Stderr, "fellowship: warning: could not clean %s: %v\n", sq.name, err) | ||
| continue | ||
| } | ||
| cleaned = append(cleaned, cleanResult{sq.name, sq.gatePending, sq.held}) | ||
| } |
There was a problem hiding this comment.
clean-worktrees is doing a blanket reset, not a stale-only cleanup.
The query pulls every row with gate_pending = 1 OR held = 1, and the loop clears those flags unconditionally. There is no worktree or git-health staleness check here, so running this command will unblock active quests too. Add a real stale predicate before clearing state, or rename the command to reflect the destructive behavior.
| for _, name := range []string{"fellowship-state.json", "quest-state.json"} { | ||
| if _, err := os.Stat(filepath.Join(dataDir, name)); err == nil { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
The migration hint only checks two legacy files.
This migration replaces seven JSON/JSONL stores, but jsonFilesExist only looks for fellowship-state.json and quest-state.json. Repos that still only have the other legacy files won't be told to run fellowship migrate when DB open fails.
| func TestScan_ExcludesExpired(t *testing.T) { | ||
| d := db.OpenTest(t) | ||
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | ||
| // Insert an autopsy with an already-expired expires_at | ||
| if err := sqlitex.Execute(conn, | ||
| `INSERT INTO autopsies (timestamp, quest, trigger_type, what_failed, expires_at) | ||
| VALUES (datetime('now', '-100 days'), 'old-quest', 'recovery', 'old failure', datetime('now', '-10 days'))`, | ||
| nil); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| oldID := conn.LastInsertRowID() | ||
|
|
||
| // Add a module so we can search for it | ||
| if err := sqlitex.Execute(conn, | ||
| `INSERT INTO autopsy_modules (autopsy_id, module) VALUES (?, 'auth')`, | ||
| &sqlitex.ExecOptions{Args: []any{oldID}}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| matches, err := Scan(conn, ScanOptions{Modules: []string{"auth"}}, 90) | ||
| if err != nil { | ||
| t.Fatalf("Scan failed: %v", err) | ||
| } | ||
| if len(matches) != 0 { | ||
| t.Errorf("expired autopsy should be excluded, got %d matches", len(matches)) | ||
| } |
There was a problem hiding this comment.
Seed the expiry test with the production timestamp format.
Create stores expires_at as RFC3339 text, but this test inserts SQLite datetime(...) text instead. Since Scan compares a.expires_at > datetime('now') lexically, this setup can miss the same-day expiry edge case that only shows up with real rows. Use an RFC3339 value here, or create the row first and then update expires_at, so the test exercises the production representation.
| func TestPendingErrands(t *testing.T) { | ||
| d := db.OpenTest(t) | ||
| seedQuest(t, d, "q1") | ||
|
|
||
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | ||
| if _, err := errand.Add(conn, "q1", "A", ""); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if _, err := errand.Add(conn, "q1", "B", ""); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := errand.UpdateStatus(conn, "q1", "w-001", errand.Done); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| pending, err := errand.PendingErrands(conn, "q1") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if len(pending) != 1 { | ||
| t.Fatalf("expected 1 pending, got %d", len(pending)) | ||
| } | ||
| if pending[0].ID != "w-002" { | ||
| t.Errorf("expected w-002, got %s", pending[0].ID) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This doesn't exercise dependency resolution yet.
PendingErrands is interesting because of errand_deps, but this setup never creates a dependency or a blocked errand. An implementation that simply returns every pending item would still pass. Add at least one dependent item so the test proves the dependency gating logic works.
| if err := sqlitex.Execute(conn, | ||
| `SELECT timestamp FROM herald WHERE quest = ? ORDER BY id DESC LIMIT 1`, | ||
| &sqlitex.ExecOptions{ | ||
| Args: []any{qs.questName}, | ||
| ResultFunc: func(stmt *sqlite.Stmt) error { | ||
| lastTimestamp = stmt.ColumnText(0) | ||
| return nil | ||
| }, | ||
| }, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf("detect problems: query herald for %s: %w", qs.questName, err) | ||
| } |
There was a problem hiding this comment.
Use event time for zombie detection, not insert order.
Lines 79-90 pick the last inserted herald row with ORDER BY id DESC, but both Announce and the migration write caller-supplied timestamps. A backfilled or out-of-order event can therefore become the "latest" row here and incorrectly hide or create a zombie alert.
🛠 Proposed fix
- `SELECT timestamp FROM herald WHERE quest = ? ORDER BY id DESC LIMIT 1`,
+ `SELECT timestamp FROM herald WHERE quest = ? ORDER BY timestamp DESC, id DESC LIMIT 1`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := sqlitex.Execute(conn, | |
| `SELECT timestamp FROM herald WHERE quest = ? ORDER BY id DESC LIMIT 1`, | |
| &sqlitex.ExecOptions{ | |
| Args: []any{qs.questName}, | |
| ResultFunc: func(stmt *sqlite.Stmt) error { | |
| lastTimestamp = stmt.ColumnText(0) | |
| return nil | |
| }, | |
| }, | |
| ); err != nil { | |
| return nil, fmt.Errorf("detect problems: query herald for %s: %w", qs.questName, err) | |
| } | |
| if err := sqlitex.Execute(conn, | |
| `SELECT timestamp FROM herald WHERE quest = ? ORDER BY timestamp DESC, id DESC LIMIT 1`, | |
| &sqlitex.ExecOptions{ | |
| Args: []any{qs.questName}, | |
| ResultFunc: func(stmt *sqlite.Stmt) error { | |
| lastTimestamp = stmt.ColumnText(0) | |
| return nil | |
| }, | |
| }, | |
| ); err != nil { | |
| return nil, fmt.Errorf("detect problems: query herald for %s: %w", qs.questName, err) | |
| } |
| if err := d.WithConn(context.Background(), func(conn *db.Conn) error { | ||
| // Insert fellowship. | ||
| err := sqlitex.Execute(conn, | ||
| `INSERT INTO fellowship (id, version, name, main_repo, base_branch, created_at) | ||
| VALUES (1, '1', 'test', '/tmp/repo', 'main', '2025-01-01T00:00:00Z')`, | ||
| nil) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Insert a quest into fellowship_quests. | ||
| err = sqlitex.Execute(conn, | ||
| `INSERT INTO fellowship_quests (name, task_description, worktree, branch, task_id, status) | ||
| VALUES ('quest-1', 'Fix the bug', '/tmp/wt/quest-1', 'fellowship/quest-1', 'task-abc', 'active')`, | ||
| nil) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Insert matching quest_state. | ||
| err = sqlitex.Execute(conn, | ||
| `INSERT INTO quest_state (quest_name, task_id, team_name, phase, gate_pending, created_at, updated_at) | ||
| VALUES ('quest-1', 'task-abc', 'team-a', 'Implement', 1, '2025-01-01T00:00:00Z', '2025-01-01T00:00:00Z')`, | ||
| nil) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| result, err := Scan(conn, "/tmp/nonexistent-repo") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if len(result.Quests) != 1 { | ||
| t.Fatalf("expected 1 quest, got %d", len(result.Quests)) | ||
| } | ||
|
|
||
| q := result.Quests[0] | ||
| if q.Name != "quest-1" { | ||
| t.Errorf("Name = %q, want %q", q.Name, "quest-1") | ||
| } | ||
| if q.TaskDescription != "Fix the bug" { | ||
| t.Errorf("TaskDescription = %q, want %q", q.TaskDescription, "Fix the bug") | ||
| } | ||
| if q.Worktree != "/tmp/wt/quest-1" { | ||
| t.Errorf("Worktree = %q, want %q", q.Worktree, "/tmp/wt/quest-1") | ||
| } | ||
| if q.Branch != "fellowship/quest-1" { | ||
| t.Errorf("Branch = %q, want %q", q.Branch, "fellowship/quest-1") | ||
| } | ||
| if q.Phase != "Implement" { | ||
| t.Errorf("Phase = %q, want %q", q.Phase, "Implement") | ||
| } | ||
| if !q.GatePending { | ||
| t.Error("expected GatePending to be true") | ||
| } | ||
| // Classification should be "stale" (no checkpoint file, not merged) | ||
| if q.Classification != "stale" { | ||
| t.Errorf("Classification = %q, want %q", q.Classification, "stale") | ||
| } |
There was a problem hiding this comment.
Use temp dirs instead of hard-coded /tmp paths in this test.
Scan shells out to Git on gitRoot and checks checkpoint/git status under row.worktree, so /tmp/... makes the "stale" assertion depend on whatever already exists on the runner. Use t.TempDir() for both values to keep this test deterministic.
| func TestSetStatus(t *testing.T) { | ||
| d := db.OpenTest(t) | ||
| seedQuest(t, d, "q1") | ||
|
|
||
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | ||
| // Insert a fellowship_quests row for SetStatus to update. | ||
| if err := tome.SetStatus(conn, "q1", "completed"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| func TestLoadOrCreate_ExistingTome(t *testing.T) { | ||
| dir := t.TempDir() | ||
| path := filepath.Join(dir, "quest-tome.json") | ||
| original := &QuestTome{Version: 1, QuestName: "existing", Status: "active", PhasesCompleted: []PhaseRecord{}, GateHistory: []GateEvent{}, FilesTouched: []string{}} | ||
| Save(path, original) | ||
| // Insert fellowship_quests row and test SetStatus. | ||
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | ||
| // Manually insert a fellowship_quests row. | ||
| if err := sqlitex.Execute(conn, `INSERT INTO fellowship_quests (name, status) VALUES ('q1', 'active')`, nil); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := tome.SetStatus(conn, "q1", "completed"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| c := LoadOrCreate(path) | ||
| if c.QuestName != "existing" { | ||
| t.Errorf("QuestName = %q, want existing", c.QuestName) | ||
| qt, err := tome.Load(conn, "q1") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if qt.Status != "completed" { | ||
| t.Errorf("expected completed, got %s", qt.Status) | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test logic flaw: first SetStatus call updates nothing, but error isn't expected.
Lines 209-217 call SetStatus on "q1" before inserting a row into fellowship_quests. Since SetStatus uses UPDATE ... WHERE name = :quest, this silently updates zero rows. The test then opens a new transaction (line 220) and manually inserts a row before calling SetStatus again — but the first call's "success" is misleading.
Either:
- Remove the first
SetStatuscall (lines 209-217) since it does nothing useful. - Or assert that
conn.Changes() == 0if testing the "no-op on missing row" behavior intentionally.
Suggested fix: remove the redundant first SetStatus block
func TestSetStatus(t *testing.T) {
d := db.OpenTest(t)
seedQuest(t, d, "q1")
- if err := d.WithTx(context.Background(), func(conn *db.Conn) error {
- // Insert a fellowship_quests row for SetStatus to update.
- if err := tome.SetStatus(conn, "q1", "completed"); err != nil {
- t.Fatal(err)
- }
- return nil
- }); err != nil {
- t.Fatal(err)
- }
-
// Insert fellowship_quests row and test SetStatus.
if err := d.WithTx(context.Background(), func(conn *db.Conn) error {
// Manually insert a fellowship_quests row.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSetStatus(t *testing.T) { | |
| d := db.OpenTest(t) | |
| seedQuest(t, d, "q1") | |
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | |
| // Insert a fellowship_quests row for SetStatus to update. | |
| if err := tome.SetStatus(conn, "q1", "completed"); err != nil { | |
| t.Fatal(err) | |
| } | |
| return nil | |
| }); err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| func TestLoadOrCreate_ExistingTome(t *testing.T) { | |
| dir := t.TempDir() | |
| path := filepath.Join(dir, "quest-tome.json") | |
| original := &QuestTome{Version: 1, QuestName: "existing", Status: "active", PhasesCompleted: []PhaseRecord{}, GateHistory: []GateEvent{}, FilesTouched: []string{}} | |
| Save(path, original) | |
| // Insert fellowship_quests row and test SetStatus. | |
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | |
| // Manually insert a fellowship_quests row. | |
| if err := sqlitex.Execute(conn, `INSERT INTO fellowship_quests (name, status) VALUES ('q1', 'active')`, nil); err != nil { | |
| t.Fatal(err) | |
| } | |
| if err := tome.SetStatus(conn, "q1", "completed"); err != nil { | |
| t.Fatal(err) | |
| } | |
| c := LoadOrCreate(path) | |
| if c.QuestName != "existing" { | |
| t.Errorf("QuestName = %q, want existing", c.QuestName) | |
| qt, err := tome.Load(conn, "q1") | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if qt.Status != "completed" { | |
| t.Errorf("expected completed, got %s", qt.Status) | |
| } | |
| return nil | |
| }); err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| func TestSetStatus(t *testing.T) { | |
| d := db.OpenTest(t) | |
| seedQuest(t, d, "q1") | |
| // Insert fellowship_quests row and test SetStatus. | |
| if err := d.WithTx(context.Background(), func(conn *db.Conn) error { | |
| // Manually insert a fellowship_quests row. | |
| if err := sqlitex.Execute(conn, `INSERT INTO fellowship_quests (name, status) VALUES ('q1', 'active')`, nil); err != nil { | |
| t.Fatal(err) | |
| } | |
| if err := tome.SetStatus(conn, "q1", "completed"); err != nil { | |
| t.Fatal(err) | |
| } | |
| qt, err := tome.Load(conn, "q1") | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if qt.Status != "completed" { | |
| t.Errorf("expected completed, got %s", qt.Status) | |
| } | |
| return nil | |
| }); err != nil { | |
| t.Fatal(err) | |
| } | |
| } |
| // SetStatus updates the quest status in fellowship_quests. | ||
| func SetStatus(conn *sqlite.Conn, questName, status string) error { | ||
| return sqlitex.Execute(conn, | ||
| `UPDATE fellowship_quests SET status = :status WHERE name = :quest`, | ||
| &sqlitex.ExecOptions{Named: map[string]any{":quest": questName, ":status": status}}) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all call sites of tome.SetStatus to understand usage patterns
rg -n 'tome\.SetStatus' --type=go -C3Repository: justinjdev/fellowship
Length of output: 1392
🏁 Script executed:
# View the full test context around line 211
sed -n '200,235p' cli/internal/tome/tome_test.goRepository: justinjdev/fellowship
Length of output: 1013
🏁 Script executed:
# Also check how quests are typically created/inserted in the codebase
rg -n 'INSERT.*fellowship_quests' --type=go -B2 -A2Repository: justinjdev/fellowship
Length of output: 4627
🏁 Script executed:
# Check the broader context of how SetStatus is used in completion.go
sed -n '1,50p' cli/internal/hooks/completion.goRepository: justinjdev/fellowship
Length of output: 871
🏁 Script executed:
# Find seedQuest definition
rg -n 'func seedQuest' --type=go -A5Repository: justinjdev/fellowship
Length of output: 1935
🏁 Script executed:
# Find all call sites of MarkTomeCompleted
rg -n 'MarkTomeCompleted' --type=go -B2 -A2Repository: justinjdev/fellowship
Length of output: 827
🏁 Script executed:
# Find state.Upsert implementation
rg -n 'func.*Upsert' cli/internal/state/ --type=go -A15 | head -40Repository: justinjdev/fellowship
Length of output: 2450
🏁 Script executed:
# Also check if there's quest creation logic when a quest is initialized
rg -n 'fellowship_quests' cli/internal/state/ --type=go -B2 -A2Repository: justinjdev/fellowship
Length of output: 850
🏁 Script executed:
# Get the complete Upsert function
sed -n '94,130p' cli/internal/state/state.goRepository: justinjdev/fellowship
Length of output: 1461
🏁 Script executed:
# Also check if fellowship_quests is involved in Upsert
rg -n 'INSERT.*fellowship_quests' cli/internal/state/ --type=go -B2 -A2Repository: justinjdev/fellowship
Length of output: 508
🏁 Script executed:
# Look for where fellowship_quests rows are created outside of migrations
rg -n 'INSERT.*fellowship_quests' --type=go | grep -v test | grep -v migrateRepository: justinjdev/fellowship
Length of output: 198
🏁 Script executed:
# Check the test more carefully - does it verify the status was actually updated?
sed -n '200,240p' cli/internal/tome/tome_test.goRepository: justinjdev/fellowship
Length of output: 1064
SetStatus silently succeeds when no row matches and lacks proper error handling.
The UPDATE statement returns no error when WHERE name = :quest matches zero rows. If the quest doesn't exist in fellowship_quests, the caller won't know the status wasn't actually set. The test suite itself demonstrates this issue—the first test case (line 211) calls SetStatus after seedQuest without verifying the update succeeded, while the second test case (line 225) explicitly inserts a row and validates the result. Add a check for conn.Changes() == 0 to return an error when the quest is not found, or ensure fellowship_quests rows are created alongside quest state initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/internal/status/status.go (1)
131-133: Consider wrapping the error with context for consistency.The fellowship query wraps errors with
fmt.Errorf("status: load fellowship: %w", err), but this query returns the raw error. Adding context would improve debuggability.♻️ Suggested improvement
if err != nil { - return nil, err + return nil, fmt.Errorf("status: load quests: %w", err) }cli/internal/autopsy/autopsy.go (2)
144-188: Confusing build-then-remove pattern and missing LIKE wildcard escaping.Two issues in the file matching logic:
Confusing flow: Lines 144-160 build conditions/args, then lines 163-164 immediately discard them. This "build then undo" pattern is error-prone and hard to maintain. Remove the dead code (lines 144-164) entirely.
LIKE wildcard escaping: Directory names containing
%or_will cause incorrect matches (e.g.,foo_bar/%matchesfooxbar/file). Escape these characters or use a different matching strategy.♻️ Proposed fix: Remove dead code and add LIKE escaping
if len(opts.Files) > 0 { - filePlaceholders := make([]string, len(opts.Files)) - for i, f := range opts.Files { - filePlaceholders[i] = "?" - args = append(args, f) - } - // Match by exact file path or same directory - conditions = append(conditions, - fmt.Sprintf(`a.id IN ( - SELECT af.autopsy_id FROM autopsy_files af - WHERE af.file_path IN (%s) - OR EXISTS ( - SELECT 1 FROM autopsy_files af2 - WHERE af2.autopsy_id = af.autopsy_id - AND af2.file_path != af.file_path - ) - )`, strings.Join(filePlaceholders, ","))) - // Actually, we need directory-level matching. Let's use a simpler approach: - // For each query file, match autopsies that have a file in the same directory. - conditions = conditions[:len(conditions)-1] // remove the above - args = args[:len(args)-len(opts.Files)] // remove args - - // Use a subquery that checks directory matching var fileCondParts []string for _, f := range opts.Files { // Exact match args = append(args, f) fileCondParts = append(fileCondParts, "af.file_path = ?") // Same directory match (for files with directories) dir := filepath.Dir(filepath.ToSlash(f)) if dir != "." { - args = append(args, dir+"/%") + escaped := strings.ReplaceAll(strings.ReplaceAll(dir, "%", "\\%"), "_", "\\_") + args = append(args, escaped+"/%") - fileCondParts = append(fileCondParts, "af.file_path LIKE ?") + fileCondParts = append(fileCondParts, "af.file_path LIKE ? ESCAPE '\\'") }
246-250: N+1 query pattern in relation loading.Each autopsy requires 3 additional queries (files, modules, tags), resulting in 1 + 3N total queries. For large result sets, consider batching: collect all IDs, then query each junction table once with
WHERE autopsy_id IN (...), and map results back.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b2e7996-cb17-48d2-8d25-8f666fa54fd0
📒 Files selected for processing (5)
cli/internal/autopsy/autopsy.gocli/internal/dashboard/server_test.gocli/internal/db/db.gocli/internal/db/db_test.gocli/internal/status/status.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/internal/db/db.go
- cli/internal/db/db_test.go
| func Create(conn *sqlite.Conn, input *CreateInput) (int64, error) { | ||
| if input == nil { | ||
| return "", fmt.Errorf("input is required") | ||
| return 0, fmt.Errorf("input is required") | ||
| } | ||
| if input.Quest == "" { | ||
| return "", fmt.Errorf("quest is required") | ||
| return 0, fmt.Errorf("quest is required") | ||
| } | ||
| if input.WhatFailed == "" { | ||
| return "", fmt.Errorf("what_failed is required") | ||
| return 0, fmt.Errorf("what_failed is required") | ||
| } | ||
| if !validTriggers[input.Trigger] { | ||
| return "", fmt.Errorf("invalid trigger %q (must be recovery, rejection, or abandonment)", input.Trigger) | ||
| return 0, fmt.Errorf("invalid trigger %q (must be recovery, rejection, or abandonment)", input.Trigger) | ||
| } | ||
|
|
||
| now := time.Now().UTC() | ||
| a := &Autopsy{ | ||
| Version: 1, | ||
| Timestamp: now.Format(time.RFC3339), | ||
| Quest: input.Quest, | ||
| Task: input.Task, | ||
| Phase: input.Phase, | ||
| Trigger: input.Trigger, | ||
| Files: input.Files, | ||
| Modules: input.Modules, | ||
| WhatFailed: input.WhatFailed, | ||
| Resolution: input.Resolution, | ||
| Tags: input.Tags, | ||
| } | ||
| if a.Files == nil { | ||
| a.Files = []string{} | ||
| } | ||
| if a.Modules == nil { | ||
| a.Modules = []string{} | ||
| } | ||
| if a.Tags == nil { | ||
| a.Tags = []string{} | ||
| timestamp := now.Format(time.RFC3339) | ||
| expiresAt := now.AddDate(0, 0, DefaultExpiryDays).Format(time.RFC3339) | ||
|
|
||
| err := sqlitex.Execute(conn, | ||
| `INSERT INTO autopsies (timestamp, quest, task, phase, trigger_type, what_failed, resolution, expires_at) | ||
| VALUES (:ts, :quest, :task, :phase, :trigger, :what_failed, :resolution, :expires_at)`, | ||
| &sqlitex.ExecOptions{ | ||
| Named: map[string]any{ | ||
| ":ts": timestamp, | ||
| ":quest": input.Quest, | ||
| ":task": input.Task, | ||
| ":phase": input.Phase, | ||
| ":trigger": input.Trigger, | ||
| ":what_failed": input.WhatFailed, | ||
| ":resolution": input.Resolution, | ||
| ":expires_at": expiresAt, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("autopsy: insert: %w", err) | ||
| } | ||
|
|
||
| dir := filepath.Join(repoRoot, datadir.Name(), autopsyDir) | ||
| if err := os.MkdirAll(dir, 0755); err != nil { | ||
| return "", fmt.Errorf("creating autopsies directory: %w", err) | ||
| } | ||
| id := conn.LastInsertRowID() | ||
|
|
||
| randBytes := make([]byte, 4) | ||
| if _, err := rand.Read(randBytes); err != nil { | ||
| return "", fmt.Errorf("generating autopsy filename suffix: %w", err) | ||
| for _, f := range input.Files { | ||
| if err := sqlitex.Execute(conn, | ||
| `INSERT INTO autopsy_files (autopsy_id, file_path) VALUES (?, ?)`, | ||
| &sqlitex.ExecOptions{ | ||
| Args: []any{id, f}, | ||
| }); err != nil { | ||
| return 0, fmt.Errorf("autopsy: insert file: %w", err) | ||
| } | ||
| } | ||
| filename := fmt.Sprintf("%s-%s-%x.json", now.Format("20060102T150405"), sanitize(input.Quest), randBytes) | ||
| path := filepath.Join(dir, filename) | ||
|
|
||
| data, err := json.MarshalIndent(a, "", " ") | ||
| if err != nil { | ||
| return "", fmt.Errorf("marshaling autopsy: %w", err) | ||
| for _, m := range input.Modules { | ||
| if err := sqlitex.Execute(conn, | ||
| `INSERT INTO autopsy_modules (autopsy_id, module) VALUES (?, ?)`, | ||
| &sqlitex.ExecOptions{ | ||
| Args: []any{id, m}, | ||
| }); err != nil { | ||
| return 0, fmt.Errorf("autopsy: insert module: %w", err) | ||
| } | ||
| } | ||
| data = append(data, '\n') | ||
|
|
||
| if err := os.WriteFile(path, data, 0644); err != nil { | ||
| return "", fmt.Errorf("writing autopsy: %w", err) | ||
| for _, tag := range input.Tags { | ||
| if err := sqlitex.Execute(conn, | ||
| `INSERT INTO autopsy_tags (autopsy_id, tag) VALUES (?, ?)`, | ||
| &sqlitex.ExecOptions{ | ||
| Args: []any{id, tag}, | ||
| }); err != nil { | ||
| return 0, fmt.Errorf("autopsy: insert tag: %w", err) | ||
| } | ||
| } | ||
| return path, nil | ||
| } | ||
|
|
||
| // ScanOptions configures which autopsies to match. | ||
| type ScanOptions struct { | ||
| Files []string | ||
| Modules []string | ||
| Tags []string | ||
| return id, nil |
There was a problem hiding this comment.
Multiple inserts without transaction wrapper may leave partial data on failure.
If inserting into autopsy_files, autopsy_modules, or autopsy_tags fails after the main autopsies row is inserted, the database will contain an incomplete autopsy record. Wrap all inserts in an explicit transaction using sqlitex.Save or sqlitex.Transaction, or document that the caller must provide a transaction-wrapped connection.
🔧 Proposed fix using sqlitex.Save
func Create(conn *sqlite.Conn, input *CreateInput) (int64, error) {
+ var id int64
+ defer sqlitex.Save(conn)(&err)
+
if input == nil {
return 0, fmt.Errorf("input is required")
}Alternatively, wrap with explicit transaction:
func Create(conn *sqlite.Conn, input *CreateInput) (id int64, err error) {
defer sqlitex.Transaction(conn)(&err)
// ... rest of function
}…context - Remove build-then-remove pattern in autopsy file matching (dead code) - Add LIKE wildcard escaping for directory paths containing % or _ - Wrap status quest-loading error with context for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/internal/autopsy/autopsy.go (1)
79-130:⚠️ Potential issue | 🟠 MajorEnsure
Createis atomic across parent/child inserts.If any insert into
autopsy_files,autopsy_modules, orautopsy_tagsfails after Line 80 inserts intoautopsies, you can persist a partial autopsy record. Wrap the full write set in a transaction (or explicitly require/enforce transactional caller context).
🧹 Nitpick comments (2)
cli/internal/status/status.go (1)
113-117: Make quest output ordering deterministic.The quest query has no explicit ordering, so output order can drift between runs. Sorting at query-time keeps API/UI output stable.
♻️ Suggested change
err = sqlitex.Execute(conn, `SELECT fq.name, fq.task_description, fq.worktree, fq.branch, COALESCE(qs.phase, ''), COALESCE(qs.gate_pending, 0) FROM fellowship_quests fq - LEFT JOIN quest_state qs ON fq.name = qs.quest_name`, + LEFT JOIN quest_state qs ON fq.name = qs.quest_name + ORDER BY fq.name`, &sqlitex.ExecOptions{Also applies to: 146-167
cli/internal/autopsy/autopsy.go (1)
14-15:expiryDaysis currently a dead API input.
DefaultExpiryDaysis hardcoded at creation time, whileScan(..., expiryDays int)never usesexpiryDays. Either wire configured TTL into creation/expiry logic or remove this parameter to avoid a misleading contract.Also applies to: 77-78, 134-134
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f242c636-e1b3-4b76-827c-419de15f8996
📒 Files selected for processing (2)
cli/internal/autopsy/autopsy.gocli/internal/status/status.go
| for _, f := range opts.Files { | ||
| // Exact match | ||
| args = append(args, f) | ||
| fileCondParts = append(fileCondParts, "af.file_path = ?") | ||
|
|
||
| // Same directory match (for files with directories) | ||
| dir := filepath.Dir(filepath.ToSlash(f)) | ||
| if dir != "." { | ||
| escaped := strings.ReplaceAll(strings.ReplaceAll(dir, "%", "\\%"), "_", "\\_") | ||
| args = append(args, escaped+"/%") | ||
| fileCondParts = append(fileCondParts, "af.file_path LIKE ? ESCAPE '\\'") | ||
| } | ||
|
|
||
| for _, entry := range entries { | ||
| if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { | ||
| continue | ||
| // Query file is under a directory prefix in the autopsy | ||
| if strings.HasSuffix(f, "/") { | ||
| escaped := strings.ReplaceAll(strings.ReplaceAll(f, "%", "\\%"), "_", "\\_") | ||
| args = append(args, escaped+"%") | ||
| fileCondParts = append(fileCondParts, "af.file_path LIKE ? ESCAPE '\\'") |
There was a problem hiding this comment.
Normalize file filters before all comparisons.
Line 152 normalizes for directory matching, but exact match (af.file_path = ?) and prefix logic still use raw f. On Windows-style inputs, this can miss matches unexpectedly. Normalize once (e.g., f = filepath.ToSlash(f)) before exact, dir, and prefix conditions.
🔧 Suggested patch
- for _, f := range opts.Files {
+ for _, raw := range opts.Files {
+ f := filepath.ToSlash(raw)
// Exact match
args = append(args, f)
fileCondParts = append(fileCondParts, "af.file_path = ?")
// Same directory match (for files with directories)
- dir := filepath.Dir(filepath.ToSlash(f))
+ dir := filepath.Dir(f)
if dir != "." {
escaped := strings.ReplaceAll(strings.ReplaceAll(dir, "%", "\\%"), "_", "\\_")
args = append(args, escaped+"/%")
fileCondParts = append(fileCondParts, "af.file_path LIKE ? ESCAPE '\\'")
}Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
fellowship.db(SQLite with WAL mode)dbpackage with schema, connection pool, migrations, and provenance triggers*db.DB/*sqlite.Connfellowship migratecommand for one-shot JSON-to-SQLite migration with backupfilelockpackage (replaced by SQLite WAL concurrency)Motivation
The file-based storage had two core weaknesses:
SQLite provides crash safety, proper concurrency via WAL, queryable cross-quest history, and provenance tracking via triggers on
state_changelog.Test plan
cd cli && go test ./...— all 15 packages passcd cli && go build ./...— binary builds clean (pure Go, no CGO)fellowship migratewith existing JSON fixtures🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Bug Fixes