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

Revert "refactor: move commands away from recipe (#2542)" #2575

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Jan 5, 2024

The agent tests started failing after merging the PR #2542 when you re-record the HTTP requests. The tests were passing in replay mode for some reason, and we suspect it's due to a buggy merge in recording.har. When you rm -rf agent/recordings and record from scratch then the tests fail.

To prevent this from happening again, we're planning to provide scripts to cleanly update the recordings.

Test plan

Green CI.

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -234,9 +234,9 @@ describe('Agent', () => {
`
{
"contextFiles": [],
"displayText": " Hello! I don't have any selected code from /src/animal.ts to use. If you provide me with some selected code snippets from that file, I'd be happy to incorporate them into my responses.",
Copy link
Member Author

Choose a reason for hiding this comment

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

@abeatrix This was a regression from your PR. @pkukielka also has one rate-limiting regression that we discovered from your PR, he will contribute a test to reproduce that regressions. Happy to help you run these tests and troubleshoot the root cause.

@pkukielka
Copy link
Contributor

pkukielka commented Jan 5, 2024

FYI when running tests in recording mode it was failing with:

- Expected
+ Received

  {
    "contextFiles": [],
-   "displayText": " Hello!",
+   "displayText": " Hello! I don't have any selected code from /src/animal.ts to use. If you provide me with some selected code snippets from that file, I'd be happy to incorporate them into my responses.",
    "speaker": "assistant",
-   "text": " Hello!",
+   "text": " Hello! I don't have any selected code from /src/animal.ts to use. If you provide me with some selected code snippets from that file, I'd be happy to incorporate them into my responses.",
  }

It's also breaking on rate limit tests I just added on another PR (instead of RateLimitError it returns empty message).
I will try to merge my PR in the meantime so you would be able to see the issue just by rebasing on top of the main.

@abeatrix
Copy link
Contributor

abeatrix commented Jan 5, 2024

FYI when running tests in recording mode it was failing with:


- Expected

+ Received



  {

    "contextFiles": [],

-   "displayText": " Hello!",

+   "displayText": " Hello! I don't have any selected code from /src/animal.ts to use. If you provide me with some selected code snippets from that file, I'd be happy to incorporate them into my responses.",

    "speaker": "assistant",

-   "text": " Hello!",

+   "text": " Hello! I don't have any selected code from /src/animal.ts to use. If you provide me with some selected code snippets from that file, I'd be happy to incorporate them into my responses.",

  }

It's also breaking on rate limit tests I just added on another PR (instead of RateLimitError it returns empty message).

I will try to merge my PR in the meantime so you would be able to see the issue just by rebasing on top of the main.

@olafurpg @pkukielka what's the issue with the agent test?

i think the correct response should be " Hello! I don't have any selected code from /src/animal.ts to use. If you provide me with some selected code snippets from that file, I'd be happy to incorporate them into my responses." instead of " Hello!", shouldn't it? 🤔

Edit: " Hello!" Should be the correct response*

@olafurpg
Copy link
Member Author

olafurpg commented Jan 5, 2024

@abeatrix the current behavior of chat is that Cody replies back with "Hello!" when you post "Hello"

CleanShot 2024-01-05 at 15 05 13@2x

This PR introduced a behavioral change that made Cody complain about missing context, which has been a recurring complaint from customers about Cody complaining about missing context. Also, the title of the PR indicated it was a refactoring, which should mean that the behavior should be unchanged. If the behavioral change is expected then we should make this more clear in the PR title and description.

@pkukielka also discovered another behavioral change related to rate limits, he will add a test case to reproduce this change.

@olafurpg olafurpg merged commit e4b60d8 into main Jan 5, 2024
16 checks passed
@olafurpg olafurpg deleted the olafurpg/revert-regression branch January 5, 2024 14:08
abeatrix added a commit that referenced this pull request Jan 5, 2024
abeatrix added a commit that referenced this pull request Jan 5, 2024
abeatrix added a commit that referenced this pull request Jan 5, 2024
…th new fixes (#2583)

Close #2588

- Reverts #2575 with updated fixes from
#2580 that I've confirmed with
@Gedochao on call.

- Merged main and updated the recording to confirm there are no errors
caused by the changes in this branch in all covered tests:


![image](https://github.com/sourcegraph/cody/assets/68532117/e54d893a-0798-48af-a838-061056fbc889)

- Also confirmed this change also passed the new rate limit test
introduced in #2535

- Fixed upresponsive stop button when error is presented


https://github.com/sourcegraph/cody/assets/68532117/6d9263b0-3e9c-419e-939d-20a2ea011824

## Test Plan

- run `pnpm run test:unit` to confirm all the tests are passing
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.

None yet

4 participants