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

Make documentation consistent, and raise PermissionError for exec #308

Merged
merged 8 commits into from
Sep 3, 2024

Conversation

art-dsit
Copy link
Contributor

@art-dsit art-dsit commented Sep 2, 2024

This PR contains:

  • New features
  • Changes to dev-tools e.g. CI config / github tooling
  • Docs
  • Bug fixes
  • Code refactor

What is the current behavior? (You can also link to an open issue here)

The sandbox interface docs are inconsistent.
Sandbox behaviour is inconsistent when you try to exec something that's not executable.

What is the new behavior?

Docs are consistent.
The docker sandbox checks for "permission denied", for consistency with local. (Note: you could argue that the current behaviour of the docker sandbox makes more sense, and instead we should change the local sandbox. Happy to change this PR to do that instead)

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

It's possible that someone is coupled to the current behaviour, but the code here is an edge case already, so seems unlikely.

Other information:

@jjallaire
Copy link
Collaborator

Thank you for cleaning up that messiness in the docs + other inconsistencies! Some other related changes we should make here:

  1. Add the directory error to this type (maybe as "is_a_directory"):
    class ToolCallError:
    type: Literal[
    "parsing",
    "timeout",
    "unicode_decode",
    "permission",
    "file_not_found",
    "unknown",
    ]
  2. Catch IsADirectoryError here and construct a ToolCallError("is_a_directory", ...):
    try:
    with track_store_changes():
    result = await call_tool(tdefs, call)
    except TimeoutError:
    tool_error = ToolCallError(
    "timeout", "Command timed out before completing."
    )
    except UnicodeDecodeError as ex:
    tool_error = ToolCallError(
    "unicode_decode",
    f"Error decoding bytes to {ex.encoding}: {ex.reason}",
    )
    except PermissionError as ex:
    message = f"{ex.strerror}."
    if isinstance(ex.filename, str):
    message = f"{message} Filename '{ex.filename}'."
    tool_error = ToolCallError("permission", message)
    except FileNotFoundError as ex:
    tool_error = ToolCallError(
    "file_not_found",
    f"File '{ex.filename}' was not found.",
    )
    except ToolParsingError as ex:
    tool_error = ToolCallError("parsing", ex.message)
    except ToolError as ex:
    tool_error = ToolCallError("unknown", ex.message)

Note that we try to use known attributes of the exception to provide as detailed an error message as we can.

@art-dsit
Copy link
Contributor Author

art-dsit commented Sep 3, 2024

Thank you for cleaning up that messiness in the docs + other inconsistencies! Some other related changes we should make here:

  1. Add the directory error to this type (maybe as "is_a_directory"):
    class ToolCallError:
    type: Literal[
    "parsing",
    "timeout",
    "unicode_decode",
    "permission",
    "file_not_found",
    "unknown",
    ]
  2. Catch IsADirectoryError here and construct a ToolCallError("is_a_directory", ...):
    try:
    with track_store_changes():
    result = await call_tool(tdefs, call)
    except TimeoutError:
    tool_error = ToolCallError(
    "timeout", "Command timed out before completing."
    )
    except UnicodeDecodeError as ex:
    tool_error = ToolCallError(
    "unicode_decode",
    f"Error decoding bytes to {ex.encoding}: {ex.reason}",
    )
    except PermissionError as ex:
    message = f"{ex.strerror}."
    if isinstance(ex.filename, str):
    message = f"{message} Filename '{ex.filename}'."
    tool_error = ToolCallError("permission", message)
    except FileNotFoundError as ex:
    tool_error = ToolCallError(
    "file_not_found",
    f"File '{ex.filename}' was not found.",
    )
    except ToolParsingError as ex:
    tool_error = ToolCallError("parsing", ex.message)
    except ToolError as ex:
    tool_error = ToolCallError("unknown", ex.message)

Note that we try to use known attributes of the exception to provide as detailed an error message as we can.

I've done this; is there a test to which we should add this error case?

@jjallaire-aisi
Copy link
Collaborator

I've done this; is there a test to which we should add this error case?

Thanks! No, there aren't currently any tests for tool error dispatching.

@jjallaire-aisi jjallaire-aisi merged commit 9a2d08b into main Sep 3, 2024
9 checks passed
@jjallaire-aisi jjallaire-aisi deleted the sandbox_exec_permission_error branch September 3, 2024 10:34
max-kaufmann pushed a commit that referenced this pull request Sep 9, 2024
* Make documentation consistent, and raise PermissionError also from docker sandbox

* ruff

* mypy

* Changes from code review

---------

Co-authored-by: jjallaire <[email protected]>
Co-authored-by: jjallaire-aisi <[email protected]>
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