Skip to content

feat(checkpoints): create checkpoint when user sends a message #7713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 5, 2025
Merged

feat(checkpoints): create checkpoint when user sends a message #7713

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1954d45
feat(checkpoints): create checkpoint on user message send
roomote Sep 5, 2025
3fd4fa2
fix(checkpoints): suppress implicit user-message checkpoint row; keep…
hannesrudolph Sep 5, 2025
317e775
Fix checkpoint suppression for user messages
daniel-lxs Sep 5, 2025
7b58980
fix: only create checkpoint on user message when files have changed
daniel-lxs Sep 5, 2025
cfaa471
test: update checkpoint test to include suppressMessage parameter
daniel-lxs Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/core/task/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,6 +890,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
this.askResponseText = text
this.askResponseImages = images

// Create a checkpoint whenever the user sends a message.
// Use allowEmpty=true to ensure a checkpoint is recorded even if there are no file changes.
if (askResponse === "messageResponse") {
void this.checkpointSave(true)
Copy link
Author

@roomote roomote bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for this new checkpoint behavior. The existing Task.spec.ts doesn't test handleWebviewAskResponse with messageResponse triggering checkpoints. Could this lead to regressions? Perhaps I should add tests to verify:

  • Checkpoint is created when askResponse === 'messageResponse'
  • Checkpoint uses allowEmpty=true
  • Other response types don't trigger checkpoints

Copy link
Author

@roomote roomote bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the fire-and-forget approach intentional here? If users send multiple messages quickly, could there be race conditions with concurrent checkpoint saves? The RepoPerTaskCheckpointService might handle this internally, but worth considering if saves should be queued or if the current behavior is acceptable.

Copy link
Author

@roomote roomote bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice we're using to ensure checkpoints even without file changes. This differs from other checkpoint triggers (write_to_file, apply_diff) that only save when files change. Is creating potentially many empty commits in the shadow git repository the intended behavior? It could be useful for conversation history, but might also create noise.

Copy link
Author

@roomote roomote bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling or at least logging if checkpoint saving fails. While the fire-and-forget approach prevents UI blocking, silent failures might make debugging difficult:

}

// Mark the last follow-up question as answered
if (askResponse === "messageResponse" || askResponse === "yesButtonClicked") {
// Find the last unanswered follow-up message using findLastIndex
Expand Down
Loading