Skip to content

Comments

feat: reduce tool entrypoints in synopsis for text editor, bash, process manager#191

Merged
salman1993 merged 12 commits intomainfrom
sm/synopsis-tools-entrypoint-refactor
Oct 29, 2024
Merged

feat: reduce tool entrypoints in synopsis for text editor, bash, process manager#191
salman1993 merged 12 commits intomainfrom
sm/synopsis-tools-entrypoint-refactor

Conversation

@salman1993
Copy link
Contributor

@salman1993 salman1993 commented Oct 24, 2024

builds on top of the changes in #186

we collect similar tools into a collection:

  • bash (params for source_path, working_dir)
  • process manager (commands and optional params)
  • text editor (commands and optional params)

@salman1993 salman1993 changed the title feat: reduce tool entrypoints in synopsis for bash, process manager feat: reduce tool entrypoints in synopsis for text editor, bash, process manager Oct 24, 2024
self.notifier.log(Rule(RULEPREFIX + "processes", style=RULESTYLE, align="left"))
self.notifier.log(Markdown(f"```\nreading {process_id}\n```"))
self.notifier.log("")
output = system.view_process_output(process_id)
Copy link
Contributor

@lamchau lamchau Oct 24, 2024

Choose a reason for hiding this comment

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

out of scope for this PR but we should test failures. i believe in the event a process failed we don't catch stderr so that would be important to capture (which may aid in system recovery).

def view_process_output(self, process_id: int) -> str:
"""View the output of a running background process."""
if not (process := self._processes.get(process_id)):
raise ValueError(f"No process found with ID: {process_id}")
output = []
while line := process.stdout.readline():
output.append(line)
return "".join(output)

if command not in self.command_dispatch:
raise ValueError(f"Unknown command '{command}'.")

# Call the corresponding method from the command dispatch
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessary comment, delete?

Copy link
Contributor

@lamchau lamchau left a comment

Choose a reason for hiding this comment

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

lgtm! much cleaner, nice work 🥳 !

@lamchau
Copy link
Contributor

lamchau commented Oct 24, 2024

also out of scope we but in a follow up i think we should pick a different command for testing the process management 65% of the testing time is taken up by these

2.02s call     tests/synopsis/test_process_management.py::test_start_process
2.01s call     tests/synopsis/test_process_management.py::test_cancel_process
1.03s call     tests/toolkit/test_developer.py::test_write_file_prevent_write_if_changed

@michaelneale
Copy link
Collaborator

to save confusion should the other PR be closed and we just try this one out?

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

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

LGTM!

"cancel": self._cancel_process,
}

def logshell(self, command: str, title: str = "shell") -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we make a top level util out of this?

content = content[start_line - 1 : (end_line if end_line != -1 else len(content))]

system.remember_file(str(patho))
self._log_file_operation(str(patho), "".join(content), get_language(str(patho)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: imo, this is too verbose to show every read - it is consistent, but i'd consider only showing this on writes?

@salman1993 salman1993 merged commit 40da95b into main Oct 29, 2024
lifeizhou-ap added a commit that referenced this pull request Oct 30, 2024
* main:
  chore: housekeeping (#202)
  feat: reduce tool entrypoints in synopsis for text editor, bash, process manager (#191)
  feat: list moderators (#204)
  chore: Minor changes in providers envs logic (#161)
  chore: add `.vscode` workspace settings and suggested extensions (#200)
  feat: license checker (#201)
  feat: include new anthropic model in docs and recommended config (#198)
  feat: tiny change so you know what processing is doing (#196)
  docs: correct intellij link (#197)
  docs: remove small duplication (#194)
  chore: add tracing option to run and group traces under session name (#187)
  docs: fix mkdocs, update to new references (#193)
ahau-square pushed a commit that referenced this pull request Nov 6, 2024
ahau-square pushed a commit that referenced this pull request Nov 6, 2024
@yingjiehe-xyz yingjiehe-xyz deleted the sm/synopsis-tools-entrypoint-refactor branch February 5, 2025 21:12
ahau-square pushed a commit that referenced this pull request May 2, 2025
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025
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.

4 participants