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(autofix): Handle more tool usage edge cases #283

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

jennmueng
Copy link
Member

@jennmueng jennmueng commented Mar 3, 2024

Fixes multiple edge cases seen with agent tool use in latest runs:

  1. Newlines in strings not being newline characters but \n. This was both in snippet matching and in the snippet replacement.
    -> Handled by replacing \n matches outside of quotes in the value of json strings
  2. ... matching didn't work if the ... was the only thing in a line as it didn't end with a ... as original code looked for.
  • Also adds support for our JSON parser to handle other non string values when they are valid. Invalid parsing will still just make everything a string but that is a loss we can take.

Should also have extensive test cases to test a lot of possible edge cases for this fragile parsing stuff!

@jennmueng jennmueng requested a review from a team March 3, 2024 07:58
def find_original_snippet(
snippet: str, file_contents: str, threshold=0.99, verbose=False
) -> str | None:
def find_original_snippet(snippet: str, file_contents: str, threshold=0.9) -> str | None:
Copy link
Member Author

Choose a reason for hiding this comment

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

default threshold value was simply adjusted to match what we were actually calling it with in the tool call.

@@ -94,15 +84,17 @@ def find_original_snippet(
if snippet_start is None:
return None

ellipsis_cases = ["... ", "// ...", "# ...", "/* ... */"]
ellipsis_comment_cases = ["// ...", "# ...", "/* ... */"]
Copy link
Member Author

Choose a reason for hiding this comment

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

the ... originally was to avoid matching javascript's rest ...someObject operator but in turn had the side effect of not working for ... in a line

Copy link
Contributor

@trillville trillville left a comment

Choose a reason for hiding this comment

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

lgtm, love the test coverage

@jennmueng jennmueng merged commit 9838b61 into main Mar 3, 2024
3 checks passed
@jennmueng jennmueng deleted the jenn/autofix/fix-snippet-matching branch March 3, 2024 19:21
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.

2 participants