fix: goose should track files it reads and not overwrite changes#46
fix: goose should track files it reads and not overwrite changes#46michaelneale merged 13 commits intomainfrom
Conversation
michaelneale
changed the title
|
ah yes, posix timestamp fun - well it works now. |
| import time | ||
|
|
||
| # Modify file externally to simulate change | ||
| time.sleep(1) |
There was a problem hiding this comment.
can use os.utime(test_file, (mtime, mtime)) to set the file time instead of sleep
lukealvoeiro
left a comment
lukealvoeiro
left a comment
There was a problem hiding this comment.
I would prefer if Goose was to inspect the modified time of each file before writing to it, and we kept a cache of the files we have read so far. That way, we could check if both Goose and any other program (or the user) modified the file, not just Goose.
@lukealvoeiro that is what it does here - it looks at the file time to know if it can write or not (ie I intented and verified it in the case of being modified outside of goose - when inside goose is all fine, this is specifically for the case of another program or user modifying it). |
Ah you're totally right - my bad, was reviewing quickly. One other thing we should do is add update the last read timestamp whenever goose itself modifies a file in |
baxen
left a comment
baxen
left a comment
There was a problem hiding this comment.
LGTM! Agree with @lukealvoeiro that we'd also want to update the timestamp after write_file since the model has that state. (But no changes in patch)
Will plan to test this more carefully after we get those changes in
|
|
||
| # Return a success message | ||
| return f"Succesfully wrote to {path}" | ||
| return f"Successfully wrote to {path}" |
src/goose/toolkit/developer.py
Outdated
| last_read_timestamp = last_read_timestamps[path] | ||
| current_timestamp = os.path.getmtime(path) | ||
| if current_timestamp > last_read_timestamp: | ||
| return f"File '{path}' has been modified since it was last read. Not writing to file." |
There was a problem hiding this comment.
this might be confusing for the model, because it'll show as a success in the tool use but the file will not change. I suspect it mostly works baised i'd raise a RuntimeError here so it is shown as an error
There was a problem hiding this comment.
yep - trying alternative - are you able to desk check it?
There was a problem hiding this comment.
(as want to make sure it actually reads the file, and not just barfs back to the user, which would be no better)
- Updated file timestamp cache after writing to a file - Changed global cache to class member - Raise an exception when a file is modified and not writeable - Fixed ruff linting issues
- Removed FileTimestampCache class - Simplified timestamp tracking to a member variable - Ensured all tests pass - Resolved ruff linting issues
Previous behavior would often ignore, leaving the changes at the end. This seems to fairly reliably incorporate them
baxen
left a comment
baxen
left a comment
There was a problem hiding this comment.
LGTM! Added some minor tweaks to the prompting that I think makes this incorporate any changes it finds more reliably
* origin/main: chore: release 0.9.0 (block#58) fix: goose should track files it reads and not overwrite changes (block#46) docs: Small dev notes for using exchange from source (block#50) fix: typo in exchange method `rewind` (block#54) fix: remove unsafe pop of messages (block#47) chore: Update LICENSE (block#53) chore(docs): update is_dangerous_command method description (block#48) refactor: improve safety rails speed and prompt (block#45) feat: make goosehints jinja templated (block#43) ci: enforce PR title follows conventional commit (block#14) feat: show available toolkits (block#37) adding in ability to provide per repo hints (block#32) Apply ruff and add to CI (block#40) added some regex based checks for dangerous commands (block#38) chore: Update publish github workflow to check package versions before publishing (block#19) # Conflicts: # src/goose/toolkit/developer.py # src/goose/utils/check_shell_command.py # tests/utils/test_check_shell_command.py
Co-authored-by: Bradley Axen <baxen@squareup.com>
Co-authored-by: Bradley Axen <baxen@squareup.com>
…ck#46) Co-authored-by: Bradley Axen <baxen@squareup.com>
5 participants
Fix for: #21
This one is slightly tricky to test, I did try it locally with main goose vs this and it would consistently re-read and not overwrite it.