2bdac87 · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

fix: mark files that are attched as read (#1777)

2bdac87Kujtim Hoxha9 months ago

internal/agent/tools/edit.go+11−10

compare with main

unifiedsplit

@@ -14,6 +14,7 @@ import (

14 "github.com/charmbracelet/crush/internal/csync"

15 "github.com/charmbracelet/crush/internal/diff"

16 "github.com/charmbracelet/crush/internal/filepathext"

17+ "github.com/charmbracelet/crush/internal/filetracker"

17 "github.com/charmbracelet/crush/internal/fsext"

18 "github.com/charmbracelet/crush/internal/fsext"

18 "github.com/charmbracelet/crush/internal/history"

19 "github.com/charmbracelet/crush/internal/history"

@@ -159,8 +160,8 @@ func createNewFile(edit editContext, filePath, content string, call fantasy.Tool

159 slog.Error("Error creating file history version", "error", err)

160 slog.Error("Error creating file history version", "error", err)

160 }

161 }

161

162

162− recordFileWrite(filePath)

163+ filetracker.RecordWrite(filePath)

163− recordFileRead(filePath)

164+ filetracker.RecordRead(filePath)

164

165

165 return fantasy.WithResponseMetadata(

166 return fantasy.WithResponseMetadata(

166 fantasy.NewTextResponse("File created: "+filePath),

167 fantasy.NewTextResponse("File created: "+filePath),

@@ -186,12 +187,12 @@ func deleteContent(edit editContext, filePath, oldString string, replaceAll bool

186 return fantasy.NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil

187 return fantasy.NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil

187 }

188 }

188

189

189− if getLastReadTime(filePath).IsZero() {

190+ if filetracker.LastReadTime(filePath).IsZero() {

190 return fantasy.NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil

191 return fantasy.NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil

191 }

192 }

192

193

193 modTime := fileInfo.ModTime()

194 modTime := fileInfo.ModTime()

194− lastRead := getLastReadTime(filePath)

195+ lastRead := filetracker.LastReadTime(filePath)

195 if modTime.After(lastRead) {

196 if modTime.After(lastRead) {

196 return fantasy.NewTextErrorResponse(

197 return fantasy.NewTextErrorResponse(

197 fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",

198 fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",

@@ -292,8 +293,8 @@ func deleteContent(edit editContext, filePath, oldString string, replaceAll bool

292 slog.Error("Error creating file history version", "error", err)

293 slog.Error("Error creating file history version", "error", err)

293 }

294 }

294

295

295− recordFileWrite(filePath)

296+ filetracker.RecordWrite(filePath)

296− recordFileRead(filePath)

297+ filetracker.RecordRead(filePath)

297

298

298 return fantasy.WithResponseMetadata(

299 return fantasy.WithResponseMetadata(

299 fantasy.NewTextResponse("Content deleted from file: "+filePath),

300 fantasy.NewTextResponse("Content deleted from file: "+filePath),

@@ -319,12 +320,12 @@ func replaceContent(edit editContext, filePath, oldString, newString string, rep

319 return fantasy.NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil

320 return fantasy.NewTextErrorResponse(fmt.Sprintf("path is a directory, not a file: %s", filePath)), nil

320 }

321 }

321

322

322− if getLastReadTime(filePath).IsZero() {

323+ if filetracker.LastReadTime(filePath).IsZero() {

323 return fantasy.NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil

324 return fantasy.NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil

324 }

325 }

325

326

326 modTime := fileInfo.ModTime()

327 modTime := fileInfo.ModTime()

327− lastRead := getLastReadTime(filePath)

328+ lastRead := filetracker.LastReadTime(filePath)

328 if modTime.After(lastRead) {

329 if modTime.After(lastRead) {

329 return fantasy.NewTextErrorResponse(

330 return fantasy.NewTextErrorResponse(

330 fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",

331 fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",

@@ -427,8 +428,8 @@ func replaceContent(edit editContext, filePath, oldString, newString string, rep

427 slog.Error("Error creating file history version", "error", err)

428 slog.Error("Error creating file history version", "error", err)

428 }

429 }

429

430

430− recordFileWrite(filePath)

431+ filetracker.RecordWrite(filePath)

431− recordFileRead(filePath)

432+ filetracker.RecordRead(filePath)

432

433

433 return fantasy.WithResponseMetadata(

434 return fantasy.WithResponseMetadata(

434 fantasy.NewTextResponse("Content replaced in file: "+filePath),

435 fantasy.NewTextResponse("Content replaced in file: "+filePath),

internal/agent/tools/file.godeleted−53

@@ -1,53 +0,0 @@

1−package tools

2−

3−import (

4− "sync"

5− "time"

6−)

7−

8−// File record to track when files were read/written

9−type fileRecord struct {

10− path string

11− readTime time.Time

12− writeTime time.Time

13−}

14−

15−var (

16− fileRecords = make(map[string]fileRecord)

17− fileRecordMutex sync.RWMutex

18−)

19−

20−func recordFileRead(path string) {

21− fileRecordMutex.Lock()

22− defer fileRecordMutex.Unlock()

23−

24− record, exists := fileRecords[path]

25− if !exists {

26− record = fileRecord{path: path}

27− }

28− record.readTime = time.Now()

29− fileRecords[path] = record

30−}

31−

32−func getLastReadTime(path string) time.Time {

33− fileRecordMutex.RLock()

34− defer fileRecordMutex.RUnlock()

35−

36− record, exists := fileRecords[path]

37− if !exists {

38− return time.Time{}

39− }

40− return record.readTime

41−}

42−

43−func recordFileWrite(path string) {

44− fileRecordMutex.Lock()

45− defer fileRecordMutex.Unlock()

46−

47− record, exists := fileRecords[path]

48− if !exists {

49− record = fileRecord{path: path}

50− }

51− record.writeTime = time.Now()

52− fileRecords[path] = record

53−}

internal/agent/tools/multiedit.go+7−6

@@ -14,6 +14,7 @@ import (

14 "github.com/charmbracelet/crush/internal/csync"

15 "github.com/charmbracelet/crush/internal/diff"

16 "github.com/charmbracelet/crush/internal/filepathext"

17+ "github.com/charmbracelet/crush/internal/filetracker"

17 "github.com/charmbracelet/crush/internal/fsext"

18 "github.com/charmbracelet/crush/internal/fsext"

18 "github.com/charmbracelet/crush/internal/history"

19 "github.com/charmbracelet/crush/internal/history"

19 "github.com/charmbracelet/crush/internal/lsp"

20 "github.com/charmbracelet/crush/internal/lsp"

@@ -206,8 +207,8 @@ func processMultiEditWithCreation(edit editContext, params MultiEditParams, call

206 slog.Error("Error creating file history version", "error", err)

207 slog.Error("Error creating file history version", "error", err)

207 }

208 }

208

209

209− recordFileWrite(params.FilePath)

210+ filetracker.RecordWrite(params.FilePath)

210− recordFileRead(params.FilePath)

211+ filetracker.RecordRead(params.FilePath)

211

212

212 var message string

213 var message string

213 if len(failedEdits) > 0 {

214 if len(failedEdits) > 0 {

@@ -244,13 +245,13 @@ func processMultiEditExistingFile(edit editContext, params MultiEditParams, call

244 }

245 }

245

246

246 // Check if file was read before editing

247 // Check if file was read before editing

247− if getLastReadTime(params.FilePath).IsZero() {

248+ if filetracker.LastReadTime(params.FilePath).IsZero() {

248 return fantasy.NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil

249 return fantasy.NewTextErrorResponse("you must read the file before editing it. Use the View tool first"), nil

249 }

250 }

250

251

251 // Check if file was modified since last read

252 // Check if file was modified since last read

252 modTime := fileInfo.ModTime()

253 modTime := fileInfo.ModTime()

253− lastRead := getLastReadTime(params.FilePath)

254+ lastRead := filetracker.LastReadTime(params.FilePath)

254 if modTime.After(lastRead) {

255 if modTime.After(lastRead) {

255 return fantasy.NewTextErrorResponse(

256 return fantasy.NewTextErrorResponse(

256 fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",

257 fmt.Sprintf("file %s has been modified since it was last read (mod time: %s, last read: %s)",

@@ -362,8 +363,8 @@ func processMultiEditExistingFile(edit editContext, params MultiEditParams, call

362 slog.Error("Error creating file history version", "error", err)

363 slog.Error("Error creating file history version", "error", err)

363 }

364 }

364

365

365− recordFileWrite(params.FilePath)

366+ filetracker.RecordWrite(params.FilePath)

366− recordFileRead(params.FilePath)

367+ filetracker.RecordRead(params.FilePath)

367

368

368 var message string

369 var message string

369 if len(failedEdits) > 0 {

370 if len(failedEdits) > 0 {

internal/agent/tools/multiedit_test.go+2−1

@@ -7,6 +7,7 @@ import (

7 "testing"

9 "github.com/charmbracelet/crush/internal/csync"

10+ "github.com/charmbracelet/crush/internal/filetracker"

10 "github.com/charmbracelet/crush/internal/history"

11 "github.com/charmbracelet/crush/internal/history"

11 "github.com/charmbracelet/crush/internal/lsp"

12 "github.com/charmbracelet/crush/internal/lsp"

12 "github.com/charmbracelet/crush/internal/permission"

13 "github.com/charmbracelet/crush/internal/permission"

@@ -119,7 +120,7 @@ func TestMultiEditSequentialApplication(t *testing.T) {

119 _ = NewMultiEditTool(lspClients, permissions, files, tmpDir)

120 _ = NewMultiEditTool(lspClients, permissions, files, tmpDir)

120

121

121 // Simulate reading the file first.

122 // Simulate reading the file first.

122− recordFileRead(testFile)

123+ filetracker.RecordRead(testFile)

123

124

124 // Manually test the sequential application logic.

125 // Manually test the sequential application logic.

125 currentContent := content

126 currentContent := content

internal/agent/tools/view.go+2−1

@@ -15,6 +15,7 @@ import (

15 "charm.land/fantasy"

16 "github.com/charmbracelet/crush/internal/csync"

17 "github.com/charmbracelet/crush/internal/filepathext"

18+ "github.com/charmbracelet/crush/internal/filetracker"

18 "github.com/charmbracelet/crush/internal/lsp"

19 "github.com/charmbracelet/crush/internal/lsp"

19 "github.com/charmbracelet/crush/internal/permission"

20 "github.com/charmbracelet/crush/internal/permission"

20)

21)

@@ -194,7 +195,7 @@ func NewViewTool(lspClients *csync.Map[string, *lsp.Client], permissions permiss

194 }

195 }

195 output += "\n</file>\n"

196 output += "\n</file>\n"

196 output += getDiagnostics(filePath, lspClients)

197 output += getDiagnostics(filePath, lspClients)

197− recordFileRead(filePath)

198+ filetracker.RecordRead(filePath)

198 return fantasy.WithResponseMetadata(

199 return fantasy.WithResponseMetadata(

199 fantasy.NewTextResponse(output),

200 fantasy.NewTextResponse(output),

200 ViewResponseMetadata{

201 ViewResponseMetadata{

internal/agent/tools/write.go+4−3

@@ -14,6 +14,7 @@ import (

14 "github.com/charmbracelet/crush/internal/csync"

15 "github.com/charmbracelet/crush/internal/diff"

16 "github.com/charmbracelet/crush/internal/filepathext"

17+ "github.com/charmbracelet/crush/internal/filetracker"

17 "github.com/charmbracelet/crush/internal/fsext"

18 "github.com/charmbracelet/crush/internal/fsext"

18 "github.com/charmbracelet/crush/internal/history"

19 "github.com/charmbracelet/crush/internal/history"

@@ -72,7 +73,7 @@ func NewWriteTool(lspClients *csync.Map[string, *lsp.Client], permissions permis

72 }

73 }

74 modTime := fileInfo.ModTime()

75 modTime := fileInfo.ModTime()

75− lastRead := getLastReadTime(filePath)

76+ lastRead := filetracker.LastReadTime(filePath)

76 if modTime.After(lastRead) {

77 if modTime.After(lastRead) {

77 return fantasy.NewTextErrorResponse(fmt.Sprintf("File %s has been modified since it was last read.\nLast modification: %s\nLast read: %s\n\nPlease read the file again before modifying it.",

78 return fantasy.NewTextErrorResponse(fmt.Sprintf("File %s has been modified since it was last read.\nLast modification: %s\nLast read: %s\n\nPlease read the file again before modifying it.",

78 filePath, modTime.Format(time.RFC3339), lastRead.Format(time.RFC3339))), nil

79 filePath, modTime.Format(time.RFC3339), lastRead.Format(time.RFC3339))), nil

@@ -156,8 +157,8 @@ func NewWriteTool(lspClients *csync.Map[string, *lsp.Client], permissions permis

156 slog.Error("Error creating file history version", "error", err)

157 slog.Error("Error creating file history version", "error", err)

157 }

158 }

158

159

159− recordFileWrite(filePath)

160+ filetracker.RecordWrite(filePath)

160− recordFileRead(filePath)

161+ filetracker.RecordRead(filePath)

161

162

162 notifyLSPs(ctx, lspClients, params.FilePath)

163 notifyLSPs(ctx, lspClients, params.FilePath)

163

164

internal/filetracker/filetracker.gonew file+70

@@ -0,0 +1,70 @@

1+// Package filetracker tracks file read/write times to prevent editing files

2+// that haven't been read, and to detect external modifications.

3+//

4+// TODO: Consider moving this to persistent storage (e.g., the database) to

5+// preserve file access history across sessions.

6+// We would need to make sure to handle the case where we reload a session and the underlying files did change.

7+package filetracker

9+import (

10+ "sync"

11+ "time"

12+)

13+

14+// record tracks when a file was read/written.

15+type record struct {

16+ path string

17+ readTime time.Time

18+ writeTime time.Time

19+}

20+

21+var (

22+ records = make(map[string]record)

23+ recordMutex sync.RWMutex

24+)

25+

26+// RecordRead records when a file was read.

27+func RecordRead(path string) {

28+ recordMutex.Lock()

29+ defer recordMutex.Unlock()

30+

31+ rec, exists := records[path]

32+ if !exists {

33+ rec = record{path: path}

34+ }

35+ rec.readTime = time.Now()

36+ records[path] = rec

37+}

38+

39+// LastReadTime returns when a file was last read. Returns zero time if never

40+// read.

41+func LastReadTime(path string) time.Time {

42+ recordMutex.RLock()

43+ defer recordMutex.RUnlock()

44+

45+ rec, exists := records[path]

46+ if !exists {

47+ return time.Time{}

48+ }

49+ return rec.readTime

50+}

51+

52+// RecordWrite records when a file was written.

53+func RecordWrite(path string) {

54+ recordMutex.Lock()

55+ defer recordMutex.Unlock()

56+

57+ rec, exists := records[path]

58+ if !exists {

59+ rec = record{path: path}

60+ }

61+ rec.writeTime = time.Now()

62+ records[path] = rec

63+}

64+

65+// Reset clears all file tracking records. Useful for testing.

66+func Reset() {

67+ recordMutex.Lock()

68+ defer recordMutex.Unlock()

69+ records = make(map[string]record)

70+}

internal/tui/components/chat/editor/editor.go+10

@@ -18,6 +18,7 @@ import (

18 tea "charm.land/bubbletea/v2"

19 "charm.land/lipgloss/v2"

20 "github.com/charmbracelet/crush/internal/app"

21+ "github.com/charmbracelet/crush/internal/filetracker"

21 "github.com/charmbracelet/crush/internal/fsext"

22 "github.com/charmbracelet/crush/internal/fsext"

22 "github.com/charmbracelet/crush/internal/message"

23 "github.com/charmbracelet/crush/internal/message"

23 "github.com/charmbracelet/crush/internal/session"

24 "github.com/charmbracelet/crush/internal/session"

@@ -202,11 +203,20 @@ func (m *editorCmp) Update(msg tea.Msg) (util.Model, tea.Cmd) {

202 m.currentQuery = ""

203 m.currentQuery = ""

203 m.completionsStartIndex = 0

204 m.completionsStartIndex = 0

204 }

205 }

206+ absPath, _ := filepath.Abs(item.Path)

207+ // Skip attachment if file was already read and hasn't been modified.

208+ lastRead := filetracker.LastReadTime(absPath)

209+ if !lastRead.IsZero() {

210+ if info, err := os.Stat(item.Path); err == nil && !info.ModTime().After(lastRead) {

211+ return m, nil

212+ }

213+ }

205 content, err := os.ReadFile(item.Path)

214 content, err := os.ReadFile(item.Path)

206 if err != nil {

215 if err != nil {

207 // if it fails, let the LLM handle it later.

216 // if it fails, let the LLM handle it later.

208 return m, nil

217 return m, nil

209 }

218 }

219+ filetracker.RecordRead(absPath)

210 m.attachments = append(m.attachments, message.Attachment{

220 m.attachments = append(m.attachments, message.Attachment{

211 FilePath: item.Path,

221 FilePath: item.Path,

212 FileName: filepath.Base(item.Path),

222 FileName: filepath.Base(item.Path),