Skip to content

Conversation

@cte
Copy link
Collaborator

@cte cte commented Jun 10, 2025

Description

The logic to rename .git has always been a bit dodgy; this seems like a safer approach until we re-visit checkpoints.

I tested multi-root workspaces as well and found that VSCode will set the cwd to be the path of the first root in the workspace, so if we want to handle that case then some additional logic will be required (which is probably worth doing since the checkpoints will only apply to the first root in the workspace which will be confusing for people).

Open question: Should we display something in the UI if checkpoints are disabled due to one of the conditions that makes the checkpoint initialization throw?


Important

Disable checkpoints in ShadowCheckpointService if nested git repositories are detected, and update related tests and logging.

  • Behavior:
    • Disable checkpoints if nested git repositories are detected in ShadowCheckpointService.
    • Remove logic to rename nested .git directories.
  • Logging:
    • Update log messages from [Cline#...] to [Task#...] in presentAssistantMessage.ts, index.ts, and getEnvironmentDetails.ts.
  • Tests:
    • Update ShadowCheckpointService.test.ts to test for detection of nested git repositories instead of renaming logic.
    • Remove tests related to renaming .git directories in excludes.test.ts.

This description was created by Ellipsis for 4093faa. You can customize this summary. It will automatically update as commits are pushed.

@cte cte requested review from jr and mrubens as code owners June 10, 2025 18:28
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 10, 2025

if (hasNestedGitRepos) {
throw new Error(
"Checkpoints are disabled because nested git repositories were detected in the workspace. " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is user-facing right? Should probably internationalize.

Copy link
Collaborator Author

@cte cte Jun 10, 2025

Choose a reason for hiding this comment

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

This is not currently user facing; it just shows up in the logs. I posed the question about surfacing this to users above; it sounds like you'd be supportive of that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do this as a follow-up.

Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

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

Looks good aside from the point on internationalization!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 10, 2025
@daniel-lxs
Copy link
Member

It would be interesting if you can take a quick look at #4494 since it seems related to the nested git repos

@cte
Copy link
Collaborator Author

cte commented Jun 10, 2025

It would be interesting if you can take a quick look at #4494 since it seems related to the nested git repos

Will take a look this week.

@cte cte merged commit 64dbcc8 into main Jun 10, 2025
19 checks passed
@cte cte deleted the cte/disable-checkpoints-for-nested-repos branch June 10, 2025 20:29
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Jun 10, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 10, 2025
lucasthahn pushed a commit to tne-ai/TNE-Code that referenced this pull request Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants