Skip to content
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.

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

Fix missing current repo context in jetbrains #6649

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pkukielka
Copy link
Contributor

Fixes https://linear.app/sourcegraph/issue/CODY-3906/agent-allow-use-of-existing-fallback-that-looks-at-gitconfig-to-get
Fixes https://linear.app/sourcegraph/issue/CODY-4140/jetbrains-cody-current-repo-no-longer-available-as-context

I think that was a simple mistake introduced by https://linear.app/sourcegraph/issue/CODY-3906/agent-allow-use-of-existing-fallback-that-looks-at-gitconfig-to-get

If I remove code you can see in the diff it simply works.
All git based methods themselves check if they can be executed so this safeguard was not adding any value there from what I can see.

Test plan

  1. Open jetbrains with cody repo and login to sg02
  2. Current Repository should be visible in the chat context by default
  3. Remove Current Repository
  4. Type @ and then Start typing Current Repository - suggestion to add should appear and you should be able to accept it

Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

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

It'd be useful to know exactly why this works now, I'm assuming it didn't work at the time, unless this was just a mistake we missed.

Have we verified that the context is correctly included in the chat message too?

@pkukielka
Copy link
Contributor Author

It'd be useful to know exactly why this works now, I'm assuming it didn't work at the time, unless this was just a mistake we missed.

Have we verified that the context is correctly included in the chat message too?

I verified it works now.

I took some time to read sqs PR so I can tell you exactly what happened:

Look at vscode/src/chat/initialContext.ts:201
Previously if workspaceReposMonitor was not defined we were falling into else branch.
In the new code version remoteReposForAllWorkspaceFolders returns empty list, but we are still entering first if branch, and thus we have no chance to recover.

That said I think entering that first branch is right, we have remote repos.
We just should not return early in remoteReposForAllWorkspaceFolders. We obtain git repo name/url not obly using VSC git extension, we also use methods like this:

/**
 * Walks the tree from the current directory to find the `.git` folder and
 * extracts remote URLs.
 */
async function gitRemoteUrlsFromParentDirs

So giving up just because VSC git extension is not initialised is in fact incorrect.

Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

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

Thank you for looking into it! Makes sense from my perspective 👍

@pkukielka pkukielka force-pushed the pkukielka/fix-missing-context-repo branch from 5bdaa0d to 3a4f2a3 Compare January 15, 2025 15:12
@pkukielka
Copy link
Contributor Author

I added one small improvement:
3a4f2a3
This way if we will fail to find remote repo, we will at least add local one (which was previously always happening in JetBrains because we were, incorrectly, skipping looking for the remote)

@umpox
Copy link
Contributor

umpox commented Jan 15, 2025

@pkukielka LGTM!

@pkukielka pkukielka enabled auto-merge (squash) January 15, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants