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

Edit: Fix missing context for instruction-based inputs #3309

Merged
merged 5 commits into from
Mar 5, 2024
Merged

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Mar 5, 2024

Description

@philipp-spiess noticed that Edit was not including context, which it should have been doing.

Recent work around commands + context improvements introduced a couple of bugs that weren't obvious:

  • contextMessages are defaulted to [] from the Edit input, we had a bad check for these
  • range is an optional value for ContextItem, but we rely on it to generate unique IDs for content within a file. This adds an extra ID mechanism to uniquely hash the content of the item alongside the file URI (if available)

Test plan

  1. Create an edit,
  2. Check preceding code and following code are included as context

@umpox umpox requested review from abeatrix, philipp-spiess and a team March 5, 2024 14:19
@@ -1,3 +1,4 @@
import crypto from 'crypto'
Copy link
Contributor

Choose a reason for hiding this comment

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

crypto will only work in node so this will likely not compile in the web config. Maybe use the crypto-js package we already bundle?

const gatewayTokenBytes = SHA256(SHA256(accessTokenBytes)).toString()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@umpox umpox enabled auto-merge (squash) March 5, 2024 14:32
@umpox umpox merged commit 3c10fc4 into main Mar 5, 2024
14 of 15 checks passed
@umpox umpox deleted the tr/edit-context branch March 5, 2024 14:39
@sqs
Copy link
Member

sqs commented Mar 5, 2024

It would be great to have a test for this. The code around this is pretty tricky. I have a PR #3307 that is cleaning up related unused code. I'll see if I can add a test.

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

3 participants