Skip to content

Comments

feat: Add synopisis core loop#166

Merged
baxen merged 4 commits intomainfrom
baxen/chatless
Oct 18, 2024
Merged

feat: Add synopisis core loop#166
baxen merged 4 commits intomainfrom
baxen/chatless

Conversation

@baxen
Copy link
Collaborator

@baxen baxen commented Oct 17, 2024

This uses a combination of a moderator and a new toolkit to implement a modified approach to managing the context goose uses to make changes to a local environment.

@michaelneale
Copy link
Collaborator

@baxen does this do .goosehints now - I wasn't sure so hard to know how to compare

@michaelneale
Copy link
Collaborator

michaelneale commented Oct 18, 2024

Doesn't seem to do well with large files:

  File "/Users/micn/Documents/code/goose/packages/exchange/src/exchange/providers/utils.py", line 35, in raise_for_status
    raise httpx.HTTPStatusError(f"{e}\n{response.text}", request=e.request, response=e.response)
httpx.HTTPStatusError: Client error '400 Bad Request' for url 'https://api.openai.com/v1/chat/completions'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
{
  "error": {
    "message": "Invalid 'messages[1].content': string too long. Expected a string with maximum length 1048576, but got a string with length 36908118 instead.",
    "type": "invalid_request_error",
    "param": "messages[1].content",
    "code": "string_above_max_length"
  }
}

vs current released goose:

image

@baxen baxen force-pushed the baxen/chatless branch 3 times, most recently from 5babf3b to 51fbf45 Compare October 18, 2024 04:28
@baxen
Copy link
Collaborator Author

baxen commented Oct 18, 2024

@baxen does this do .goosehints now - I wasn't sure so hard to know how to compare

Thanks for the reminder!! Added

@baxen
Copy link
Collaborator Author

baxen commented Oct 18, 2024

Doesn't seem to do well with large files:

  File "/Users/micn/Documents/code/goose/packages/exchange/src/exchange/providers/utils.py", line 35, in raise_for_status
    raise httpx.HTTPStatusError(f"{e}\n{response.text}", request=e.request, response=e.response)
httpx.HTTPStatusError: Client error '400 Bad Request' for url 'https://api.openai.com/v1/chat/completions'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
{
  "error": {
    "message": "Invalid 'messages[1].content': string too long. Expected a string with maximum length 1048576, but got a string with length 36908118 instead.",
    "type": "invalid_request_error",
    "param": "messages[1].content",
    "code": "string_above_max_length"
  }
}

vs current released goose:

image

Yup this does make sense. I added a quick patch - it will work like before and for files beyond the context size limit it will hopefully recover.

image

This uses a combination of a moderator and a new toolkit
to implement a modified approach to managing the context
goose uses to make changes to a local environment.
@michaelneale
Copy link
Collaborator

@baxen any idea why it isn't running the tests on this branch? (also I keep getting conflicts - I guess the force push? not usually had this)

@michaelneale
Copy link
Collaborator

@baxen thanks seems better now (sorry made a few merge/tweaks to it)

import traceback
from pathlib import Path
from typing import Optional
from typing import List, Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

:nit: we can just use list

content = f.read()

max_output_chars = 2**20
max_output_tokens = 16000
Copy link
Contributor

Choose a reason for hiding this comment

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

over time this might be variable depending on the model used no? do we want to expose it as a configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup agreed this is variable. I think this will be one of a few things we surface for configuration over time but i'm thinking in another wider scale refactor

OperatingSystem class that can manage background processes created using subprocess.Popen.
"""

cwd: str = os.getcwd()
Copy link
Contributor

Choose a reason for hiding this comment

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

:nit: did we forget field for these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure we need it? i dont think this requires using a factory instead of just a default value, or were you thinking of something else?

"""Terminate all processes in the process queue."""
for process_id in list(self._processes.keys()):
self.cancel_process(process_id)

Copy link
Contributor

Choose a reason for hiding this comment

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

we can save this for later, but i feel adding async context management might be useful for better handling of process lifecycles.

@asynccontextmanager
async def managed_process(*args, timeout=None, **kwargs):
    process = await asyncio.create_subprocess_exec(*args, **kwargs)
    try:
        try:
            await asyncio.wait_for(
                asyncio.create_task(process.wait()),
                timeout=timeout,
            )
        except Exception:
            print(
                f"timeout limit {timeout} second(s) exceeded, "
                f"terminating process (pid={process.pid}, args={args})"
            )
            process.terminate()
            await process.wait()
        yield process
    finally:
        if process.returncode is None:
            print(f"cleaning up process: {process.pid}")
            process.terminate()
            await process.wait()
        else if process.returncode != 0:
            # clean up
...

async def main():
    async with managed_process("sleep", "600", timeout=5) as process:
        pass

the error also becomes consistent for us to handle as it's normalized across all commands

timeout limit 5 second(s) exceeded, terminating process (pid=93675, args=('sleep', '600'))

it's a bit extra but now we can

  1. forcibly exit long running commands (force another eval loop)
  2. additionally, this would allow creating some "chunking" workflows
  • i often will pipe a series of commands to whittle down a surface area, creating intermediate files to make limit the scope of downstream toolchains

def cancel_process(self, process_id: int) -> bool:
"""Cancel the background process with the specified ID."""
process = self._processes.pop(process_id, None)
if process:
Copy link
Contributor

Choose a reason for hiding this comment

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

:nit: we can walrus this too

dict(
os=self.platform,
cwd=str(self.to_patho(self.cwd)),
shell=os.environ.get("SHELL", "unknown"),
Copy link
Contributor

@lamchau lamchau Oct 18, 2024

Choose a reason for hiding this comment

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

:nit: probably not the right place but this might return the incorrect shell. when i drop into a subshell i'll only get my login shell rather than the current shell execution environment.

fish-3.7.1$ echo "echo \$SHELL" | uv run goose run
Warning: Input is not a terminal (fd=0).
starting session | name:y3n9  profile:default
saving to /Users/lam/.config/goose/sessions/y3n9.jsonl

─── shell ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

 echo $SHELL

It seems your current shell is Fish (/opt/homebrew/bin/fish). How can I assist you further?
ended run | name:y3n9  profile:default


fish-3.7.1$ bash
bash-5.2$
bash-5.2$ echo "echo \$SHELL" | uv run goose run
Warning: Input is not a terminal (fd=0).
starting session | name:r6w3  profile:default
saving to /Users/lam/.config/goose/sessions/r6w3.jsonl

─── shell ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

 echo $SHELL

Your current shell is Fish, located at /opt/homebrew/bin/fish.
ended run | name:r6w3  profile:default

we should eventually try to consider this as a util

def get_current_shell():
    cmd = ["ps", "-p", str(subprocess.os.getppid()), "-o", "comm="]
    result = subprocess.run(cmd, capture_output=True, text=True)
    return result.stdout.lstrip('-') 

edit: ignore last comment - quirks ended up something else

@lamchau
Copy link
Contributor

lamchau commented Oct 18, 2024

lgtm! added a couple of comments but works great 👍

edit: just noticed a small ux thing that we should do our help is truncated. probably easier to make a shorter message like run/exit from a text file or something to that effect (otherwise we'll need to tinker with the formatters)

Commands:
  providers          Manage providers
  run                Run a single-pass session with a message from a...

self.logshell(command)
return shell(command, self.notifier, self.exchange_view, cwd=system.cwd, env=system.env)

@tool
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a tool for forget_file? to provide an escape hatch in case the list of active files gets too long

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried this! unfortunately it confused the meaning of rm and forget very often. I'd like to revisit this though as we explore


{{synopsis.hints}}

# Relevant Files
Copy link
Contributor

Choose a reason for hiding this comment

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

this is really neat! i am guessing the context mgmt work can be injected here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I think thats a great home for it!

Copy link
Contributor

@salman1993 salman1993 left a comment

Choose a reason for hiding this comment

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

lgtm! i tested it with tracing

ran goose session start --profile synopsis --tracing

Screenshot 2024-10-18 at 10 58 52 AM

@baxen baxen merged commit 6cf416c into main Oct 18, 2024

self._active_files.add(path)

def forget_file(self, path: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this used anywhere, is this something you image might be called by a tool in case the user asks Goose explicitly to forget/stop paying attention to certain files?

You can use the shell tool to run any command that would work on the relevant operating system.

You are an expert with ripgrep - `rg`. When you need to locate content in the code base, use
`rg` exclusively. It will respect ignored files for efficiency.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - why not suggest using rg -i ? I've found the case sensitivity to be a problem sometimes and have had to explicitly ask it to do a case insensitive search

@lamchau lamchau deleted the baxen/chatless branch October 24, 2024 11:44
lamchau added a commit to lamchau/goose that referenced this pull request Oct 24, 2024
ahau-square pushed a commit that referenced this pull request May 2, 2025
Co-authored-by: Michael Neale <michael.neale@gmail.com>
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025
Co-authored-by: Michael Neale <michael.neale@gmail.com>
Ghenghis referenced this pull request in Ghenghis/Super-Goose Feb 5, 2026
- Update tar to 7.5.7 to fix path traversal vulnerabilities (alerts #164-166)
- @modelcontextprotocol/sdk already at 1.26.0 (fixes alert #167)
- Add @types/node 25.2.0 for type safety
- All npm audit checks passing with 0 vulnerabilities

Fixes:
- Alert #167: MCP SDK cross-client data leak
- Alert #166: tar hardlink path traversal
- Alert #165: tar race condition via Unicode ligatures
- Alert #164: tar symlink poisoning

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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.

5 participants