Advance snapshot for budget-skipped container files#49
Conversation
Greptile SummaryThis PR fixes a repeated-read regression for container files that exceed the session byte budget. Previously, a budget-skipped file was never recorded in
Confidence Score: 4/5The production change is a single, focused The fix itself is correct and well-scoped. The test harness validates the primary regression scenario. Two test-quality gaps remain — a missing assertion for content-change re-detection and a command-agnostic src/services/container_session.rs — specifically the new test helpers Important Files Changed
Sequence DiagramsequenceDiagram
participant CC as capture_changes
participant S as snapshot (state)
participant VM as VM (read_file)
Note over CC,VM: First capture — file is new (no snapshot entry)
CC->>S: "snapshot.get(path) → None (changed=true)"
CC->>VM: read_file(path) → 4 096 B
CC->>CC: "projected > max_bytes_per_session → skip"
CC->>S: snapshot.insert(path, hash_v1) ← FIX
Note over CC,VM: Second capture — file unchanged
CC->>S: snapshot.get(path) → hash_v1
CC->>CC: "listing hash == hash_v1 → changed=false, skip"
Note over VM: read_file NOT called
Note over CC,VM: If file content changes later
CC->>S: snapshot.get(path) → hash_v1
CC->>CC: "listing hash == hash_v2 → changed=true"
CC->>VM: read_file(path)
CC->>CC: re-evaluate budget, re-warn if still over
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/services/container_session.rs:1257-1270
**Missing companion test: content-change re-detection after budget-skip**
The new test proves the happy-path fix (no re-read when content is unchanged), but there is no assertion that changing the file's content after a budget-skip causes the next capture to re-detect it and warn again. Without that test, a future regression that silently advances the snapshot on content changes (e.g. always using the old hash instead of the new one) would go unnoticed. A short extension of the existing test that mutates the `files` map and asserts `read_count == 2` on the third capture would cover this path.
### Issue 2 of 2
src/services/container_session.rs:1292-1307
**`FakeSession.exec` ignores the command string**
`FakeSession::exec` always returns the full file listing regardless of the command text. `capture_changes` currently issues exactly one `exec` (for `list_mnt_data`), so the test passes today. If `capture_changes` ever gains a second `exec` call (e.g. a cleanup command, a size check), the fake will return listing-shaped stdout for every call, silently corrupting the parse and making the test unreliable. Checking `_cmd.command.contains("sha256sum")` and returning an empty/error stream otherwise would make the stub more defensive at negligible cost.
Reviews (1): Last reviewed commit: "Advance snapshot for budget-skipped cont..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| container_id: new_container_id(), | ||
| runtime_label: "test", | ||
| created_at: Instant::now(), | ||
| idle_ttl: Duration::from_secs(60), | ||
| file_io: true, | ||
| session: Some(handle), | ||
| config, | ||
| state: Mutex::new(SessionState::default()), | ||
| persistence: None, | ||
| } | ||
| } | ||
|
|
||
| /// A file skipped by the session byte budget must advance the | ||
| /// snapshot so the next `capture_changes` doesn't re-detect it as |
There was a problem hiding this comment.
The new test proves the happy-path fix (no re-read when content is unchanged), but there is no assertion that changing the file's content after a budget-skip causes the next capture to re-detect it and warn again. Without that test, a future regression that silently advances the snapshot on content changes (e.g. always using the old hash instead of the new one) would go unnoticed. A short extension of the existing test that mutates the files map and asserts read_count == 2 on the third capture would cover this path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/container_session.rs
Line: 1257-1270
Comment:
**Missing companion test: content-change re-detection after budget-skip**
The new test proves the happy-path fix (no re-read when content is unchanged), but there is no assertion that changing the file's content after a budget-skip causes the next capture to re-detect it and warn again. Without that test, a future regression that silently advances the snapshot on content changes (e.g. always using the old hash instead of the new one) would go unnoticed. A short extension of the existing test that mutates the `files` map and asserts `read_count == 2` on the third capture would cover this path.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| assert!(refs.is_empty(), "over-budget file must not be captured"); | ||
| assert_eq!(fake.read_count(&path), 1, "expected exactly one read"); | ||
|
|
||
| // Second capture with the file unchanged: must NOT re-read it. | ||
| let refs = session.capture_changes().await.unwrap(); | ||
| assert!(refs.is_empty()); | ||
| assert_eq!( | ||
| fake.read_count(&path), | ||
| 1, | ||
| "snapshot was not advanced: over-budget file re-read on the next command" | ||
| ); | ||
| } | ||
|
|
||
| /// Thin wrapper so the test can keep an `Arc` to the fake for | ||
| /// assertions while still handing an owned `Box<dyn ShellSession>` to | ||
| /// `SessionHandle`. |
There was a problem hiding this comment.
FakeSession.exec ignores the command string
FakeSession::exec always returns the full file listing regardless of the command text. capture_changes currently issues exactly one exec (for list_mnt_data), so the test passes today. If capture_changes ever gains a second exec call (e.g. a cleanup command, a size check), the fake will return listing-shaped stdout for every call, silently corrupting the parse and making the test unreliable. Checking _cmd.command.contains("sha256sum") and returning an empty/error stream otherwise would make the stub more defensive at negligible cost.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/container_session.rs
Line: 1292-1307
Comment:
**`FakeSession.exec` ignores the command string**
`FakeSession::exec` always returns the full file listing regardless of the command text. `capture_changes` currently issues exactly one `exec` (for `list_mnt_data`), so the test passes today. If `capture_changes` ever gains a second `exec` call (e.g. a cleanup command, a size check), the fake will return listing-shaped stdout for every call, silently corrupting the parse and making the test unreliable. Checking `_cmd.command.contains("sha256sum")` and returning an empty/error stream otherwise would make the stub more defensive at negligible cost.
How can I resolve this? If you propose a fix, please make it concise.1 participant
No description provided.