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
|
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
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
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}" |
| 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
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 (aaif-goose#58) fix: goose should track files it reads and not overwrite changes (aaif-goose#46) docs: Small dev notes for using exchange from source (aaif-goose#50) fix: typo in exchange method `rewind` (aaif-goose#54) fix: remove unsafe pop of messages (aaif-goose#47) chore: Update LICENSE (aaif-goose#53) chore(docs): update is_dangerous_command method description (aaif-goose#48) refactor: improve safety rails speed and prompt (aaif-goose#45) feat: make goosehints jinja templated (aaif-goose#43) ci: enforce PR title follows conventional commit (aaif-goose#14) feat: show available toolkits (aaif-goose#37) adding in ability to provide per repo hints (aaif-goose#32) Apply ruff and add to CI (aaif-goose#40) added some regex based checks for dangerous commands (aaif-goose#38) chore: Update publish github workflow to check package versions before publishing (aaif-goose#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>
…f-goose#46) Co-authored-by: Bradley Axen <baxen@squareup.com>
* feat: use ~/.agents/skills as standard skill storage path Migrate skill storage from ~/.goose/skills/ to ~/.agents/skills/ to align with the cross-agent standard (agentskills.io) convention where global skills live at ~/.agents/skills/ and local skills at <project>/.agents/skills/. Changes: - Update skills_dir() in Rust backend to use .agents/skills - Add AGENTS_STANDARD_DIR and SKILLS_DIR constants to storage.ts - Existing .goose/ constants preserved for other goose-specific data * chore: remove dead storage.ts constants file Delete src/shared/constants/storage.ts and the now-empty constants/ directory. None of the constants (GOOSE_DIR, RECIPES_DIR, AGENTS_DIR, SESSIONS_DIR, EXTENSIONS_DIR, AGENTS_STANDARD_DIR, SKILLS_DIR) were imported anywhere in the frontend codebase. Storage path logic lives entirely in the Rust backend (e.g. skills_dir() in src-tauri/src/commands/skills.rs), and the frontend communicates through Tauri invoke() commands. Keeping unused path constants in the frontend was misleading dead code.
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.