refactor(goose2): remove attachment preamble - #9052
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d16b9e79c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const acpPrompt = | ||
| promptWithPaths || (images?.length ? " " : promptWithPaths); |
There was a problem hiding this comment.
If a user sends only non-image attachments that have no path (the pathless browser-file case covered by tests) and no message text, appendAttachmentPaths(text.trim(), attachments) returns "" and acpPrompt stays empty. The earlier guard still allows this send because hasAttachments is true, so acpSendMessage receives an empty text block, which is the same class of payload this function already special-cases for image-only sends via " ". In practice this makes attachment-only submits fail for pathless files/directories instead of reaching the model.
Useful? React with 👍 / 👎.
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
nice improvement. the tests we are touching here are showing they are not very useful seeing that we need to change them for something not that related. I think we could drop them
bb226d5 to
eec6c0c
Compare
💡 Codex Reviewgoose/ui/goose2/src/features/chat/hooks/useChat.ts Lines 233 to 234 in eec6c0c Keep prompt non-empty for pathless attachment-only sends
When attachments are present but none have a usable  Delimit appended attachment paths with an unambiguous format
Joining attachment paths with a single space makes path boundaries ambiguous whenever a path itself contains spaces (common on desktop OSes), so the model can receive a merged token stream that no longer cleanly identifies each file/directory path. This regression is introduced by replacing structured per-item formatting with ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…-refactor * origin/main: refactor: switch to official new rust-sdk for ACP (#9062) refactor(goose2): remove attachment preamble (#9052) Align CODEOWNERS with pull request review rules in GOVERNANCE.md (#9056) bring MAINTAINERS.md up to date (#9053) feat(acp): expose built-in skills through sources list acp calls (#9045) add provider-first onboarding (#9039) feat: ACP streamable http spec compliance (#9034) Skip automatic fix which crashes (#9036) fix(openai): accept null tool_call arguments in streaming chunks (#9035) Signed-off-by: Lifei Zhou <lifei@squareup.com> # Conflicts: # ui/goose2/src/features/chat/hooks/useChat.ts
2 participants
Summary
Remove the preamble for images, and make it so for files/dirs it just does the same thing goose1 desktop used to, which is to suffix an absolute path to the files and let the agent read it. This version has one improvement where it doesn't include it as plaintext and renders a nice bubble instead.
Testing
Existing tests + manual test in goose2 desktop
Related Issues
N/A
Screenshots/Demos (for UX changes)