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: fixed behavior for keyboard interrupts and user commands #384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErikBjare
Copy link
Owner

@ErikBjare ErikBjare commented Jan 5, 2025

Has some similarities to #371


Important

Refactor keyboard interrupt and user command handling in chat.py and commands.py for consistent behavior.

  • Behavior:
    • Refactor keyboard interrupt handling in chat.py and commands.py to use a consistent interrupt_msg.
    • Modify execute_cmd in commands.py to check both user role and command prefix.
  • Functions:
    • Update step() in chat.py to yield interrupt_msg on KeyboardInterrupt.
    • Adjust handle_cmd() in commands.py to handle /help command separately.
  • Misc:
    • Remove redundant assertions in execute_cmd() in commands.py.
    • Minor refactoring in chat.py to streamline message handling logic.

This description was created by Ellipsis for e3d04d2. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 450a0e5 in 57 seconds

More details
  • Looked at 148 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. gptme/chat.py:141
  • Draft comment:
    interrupt_msg is used before it is defined, which can lead to a NameError. Define interrupt_msg before its first use.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. gptme/commands.py:63
  • Draft comment:
    The check msg.role == "user" and msg.content.startswith("/") is repeated in multiple places. Consider refactoring this into a helper function for clarity and efficiency.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The execute_cmd function in gptme/commands.py checks if a message is a command by verifying if it starts with a "/" and if the role is "user". This check is repeated in multiple places, which can be refactored for clarity and efficiency.
3. gptme/commands.py:159
  • Draft comment:
    The help() function is called but not defined in the provided code, which can lead to a NameError. Ensure that the help() function is defined or imported correctly.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_CZu6xC7lCVe7l8DA


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2025

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
160 5 155 3
View the top 3 failed tests by shortest run time
tests.test_cli::test_block[ipython]
Stack Traces | 0.125s run time
args = ['--name', 'test-62770-test_block[ipython]', '/impersonate ```ipython\ndef test():\n    print("start")  # start\n\n    print("after empty line")\n```']
lang = 'ipython', runner = <click.testing.CliRunner object at 0x7fd62571a1a0>

    @pytest.mark.slow
    @pytest.mark.parametrize("lang", blocks.keys())
    def test_block(args: list[str], lang: str, runner: CliRunner):
        # tests that shell codeblocks are formatted correctly such that whitespace and newlines are preserved
        code = blocks[lang]
        code = f"""```{lang}
    {code.strip()}
    ```"""
        assert "'" not in code
    
        args.append(f"/impersonate {code}")
        print(f"running: gptme {' '.join(args)}")
        result = runner.invoke(gptme.cli.main, args)
        output = result.output
        print(f"output: {output}\nEND")
        # check everything after the second '# start'
        # (get not the user impersonation command, but the assistant message and everything after)
        output = output.split("# start", 2)[-1]
        printcmd = "print" if lang == "ipython" else "echo"
>       assert f"\n\n    {printcmd}" in output
E       AssertionError: assert '\n\n    print' in '[12:58:40] Using logdir .../gptme/logs/test-62770-test_block       \n'

.../gptme/tests/test_cli.py:243: AssertionError
tests.test_cli::test_command_tokens
Stack Traces | 0.177s run time
args = ['--name', 'test-68912-test_command_tokens', '/tokens']
runner = <click.testing.CliRunner object at 0x7f9a3df39f30>

    def test_command_tokens(args: list[str], runner: CliRunner):
        args.append("/tokens")
        result = runner.invoke(gptme.cli.main, args)
>       assert "/tokens" in result.output
E       AssertionError: assert '/tokens' in '[12:58:32] Using logdir                                                         \n           .../gptme/logs/test-68912-test_command_tokens           \n'
E        +  where '[12:58:32] Using logdir                                                         \n           .../gptme/logs/test-68912-test_command_tokens           \n' = <Result FileExistsError(17, 'File exists')>.output

.../gptme/tests/test_cli.py:80: AssertionError
tests.test_cli::test_command_tools
Stack Traces | 3.82s run time
args = ['--name', 'test-93637-test_command_tools', '/tools']
runner = <click.testing.CliRunner object at 0x7f4c22c4b160>

    def test_command_tools(args: list[str], runner: CliRunner):
        args.append("/tools")
        result = runner.invoke(gptme.cli.main, args)
>       assert "/tools" in result.output
E       AssertionError: assert '/tools' in '[12:58:35] Using logdir                                                         \n           .../gptme/logs/test-93637-test_command_tools            \n'
E        +  where '[12:58:35] Using logdir                                                         \n           .../gptme/logs/test-93637-test_command_tools            \n' = <Result FileExistsError(17, 'File exists')>.output

.../gptme/tests/test_cli.py:95: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e3d04d2 in 55 seconds

More details
  • Looked at 143 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. gptme/chat.py:222
  • Draft comment:
    The check msg.content.startswith("/") and msg.role == "user" is redundant here since execute_cmd already performs this check. Consider removing it for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The check is actually not redundant. execute_cmd returns a boolean indicating if it handled a command, but this code needs to prevent further processing of commands by returning early. The check and return on line 222-223 serves a different purpose than execute_cmd - it prevents command messages from being added to the log and processed further. Without this check, commands would be processed twice.
    I could be wrong about the command processing flow. Maybe there's another mechanism preventing double processing that I'm missing.
    Looking at the full context, I'm confident this check is needed. The code structure shows commands need special handling to prevent them from being treated as regular messages.
    The comment is incorrect - this check is not redundant and serves an important purpose in preventing command messages from being processed as regular messages.
2. gptme/commands.py:64
  • Draft comment:
    The check msg.role == "user" and msg.content.startswith("/") is redundant since execute_cmd is only called when these conditions are met. Consider simplifying the logic.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_disyq8FgF6gQ0cVt


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ErikBjare ErikBjare force-pushed the dev/fix-interrupt-and-user-commands branch from e3d04d2 to c7301e5 Compare January 7, 2025 12:57
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