Skip to content

Commit b08d7c5

Browse files
authored
fix(store,agent): lock WAL sidecars; report real turn ceiling on finalize (#254)
Two self-review findings from #250: - store/sqlite: os.Chmod only locked the main state.db to 0600; in WAL mode the -wal/-shm sidecars hold committed page data (the same diffs/transcripts) and inherited the umask default. Chmod them 0600 too, right after the schema write materializes them (defense-in-depth atop the 0700 dir). Test asserts sidecar perms when present. - engine/agent: the forced-finalization fall-through error still reported maxTurns after the loop bound was raised to maxTurns+maxEmptyRounds; report the real ceiling in all three backends.
1 parent 090dc15 commit b08d7c5

5 files changed

Lines changed: 21 additions & 7 deletions

File tree

internal/engine/agent/agent.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ func (a *anthropicAgent) Review(ctx stdctx.Context, rc Context) (engine.ReviewOu
269269
emptyRounds = 0
270270
params.Messages = append(params.Messages, anthropic.NewUserMessage(toolResults...))
271271
}
272-
return engine.ReviewOutput{}, fmt.Errorf("agent: forced finalization produced no parseable findings after %d turns", maxTurns)
272+
return engine.ReviewOutput{}, fmt.Errorf("agent: forced finalization produced no parseable findings after %d turns", maxTurns+maxEmptyRounds)
273273
}
274274

275275
// RepairPatch issues one tools-less, code-only completion (system =

internal/engine/agent/codex.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ func (a *codexAgent) Review(ctx stdctx.Context, rc Context) (engine.ReviewOutput
273273
emptyRounds = 0
274274
input = append(input, toolItems...)
275275
}
276-
return engine.ReviewOutput{}, fmt.Errorf("agent: forced finalization produced no parseable findings after %d turns", maxTurns)
276+
return engine.ReviewOutput{}, fmt.Errorf("agent: forced finalization produced no parseable findings after %d turns", maxTurns+maxEmptyRounds)
277277
}
278278

279279
// dispatch runs every function_call in resp, returning function_call_output

internal/engine/agent/openai.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,5 +250,5 @@ func (a *openaiAgent) Review(ctx stdctx.Context, rc Context) (engine.ReviewOutpu
250250
params.Messages = append(params.Messages, openai.ToolMessage(out, tc.ID))
251251
}
252252
}
253-
return engine.ReviewOutput{}, fmt.Errorf("agent: forced finalization produced no parseable findings after %d turns", maxTurns)
253+
return engine.ReviewOutput{}, fmt.Errorf("agent: forced finalization produced no parseable findings after %d turns", maxTurns+maxEmptyRounds)
254254
}

internal/store/sqlite/sqlite.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,14 @@ func Open(path string) (*Store, error) {
9191
_ = db.Close()
9292
return nil, fmt.Errorf("migrate schema: %w", err)
9393
}
94-
// SchemaSQL is the first write, so the DB file is now guaranteed to exist
95-
// (WAL mode may not materialize it on Ping alone). Lock it to owner-only —
96-
// defense-in-depth atop the 0700 dir — so the persisted diffs/transcripts
97-
// aren't world-readable.
94+
// SchemaSQL is the first write, so the DB file — and, in WAL mode, its -wal/-shm
95+
// sidecars (which also hold committed page data) — now exist. Lock them all to
96+
// owner-only, defense-in-depth atop the 0700 dir, so the persisted
97+
// diffs/transcripts aren't world-readable. Sidecars may be absent on some paths,
98+
// so ignore errors.
9899
_ = os.Chmod(path, 0o600)
100+
_ = os.Chmod(path+"-wal", 0o600)
101+
_ = os.Chmod(path+"-shm", 0o600)
99102
if err := migrateReviewColumns(db); err != nil {
100103
_ = db.Close()
101104
return nil, fmt.Errorf("migrate reviews columns: %w", err)

internal/store/sqlite/sqlite_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,17 @@ func TestOpenLocksDownPermissions(t *testing.T) {
6969
if got := fi.Mode().Perm(); got != 0o600 {
7070
t.Errorf("state.db mode = %o, want 600 (holds diffs/transcripts)", got)
7171
}
72+
// WAL -wal/-shm sidecars hold committed page data too; when present they must
73+
// be locked down like the main DB (the schema write above materializes them).
74+
for _, sidecar := range []string{path + "-wal", path + "-shm"} {
75+
si, err := os.Stat(sidecar)
76+
if err != nil {
77+
continue // may not exist depending on SQLite's WAL state
78+
}
79+
if got := si.Mode().Perm(); got != 0o600 {
80+
t.Errorf("%s mode = %o, want 600 (holds committed page data)", filepath.Base(sidecar), got)
81+
}
82+
}
7283
}
7384

7485
func TestSaveGetRoundTrip(t *testing.T) {

0 commit comments

Comments
 (0)