fix(image_generate,resource_copy): add fallback for attachment key mi…#92
fix(image_generate,resource_copy): add fallback for attachment key mi…#92DivXPro wants to merge 1 commit into
Conversation
…ssing attachments/ prefix When the LLM constructs an attachment key without the leading attachments/ prefix (e.g. <accountID>/<msgID>/<file> instead of attachments/<accountID>/<msgID>/<file>), both image_generate and resource_copy fail because the actual filesystem path includes the prefix. - image_generate loadInputImages: after artifact and attachment store lookups fail, retry with attachments/ prefix if missing - resource_copy authorizeObjectKey: after Head() returns not-found, retry with attachments/ prefix if key lacks it - Also adds messageAttachmentStore field + WithMessageAttachmentStore to ToolExecutor (was missing on current main branch)
![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: 5203eab8f5
ℹ️ 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".
| func (e *ToolExecutor) WithMessageAttachmentStore(s objectstore.Store) *ToolExecutor { | ||
| e.messageAttachmentStore = s | ||
| return e | ||
| } |
There was a problem hiding this comment.
This setter is never used by the production registration path: registerStoredArtifactTools still constructs imagegeneratetool.NewToolExecutor(...) without chaining WithMessageAttachmentStore(attachmentStore). As a result, in normal worker/desktop composition messageAttachmentStore remains nil, so the fallback added below for unprefixed attachment keys is dead code and input_images that only exist in the message-attachment bucket still fail.
Useful? React with 👍 / 👎.
| if err != nil && objectstore.IsNotFound(err) && attachment && !strings.HasPrefix(key, "attachments/") { | ||
| info, err = store.Head(ctx, "attachments/"+key) | ||
| } |
There was a problem hiding this comment.
When an attachment is passed as attachment:<account_id>/... and the object exists only at attachments/<account_id>/..., this fallback can authorize the prefixed object, but fetch then calls getObject(ctx, e.AttachmentStore, key) with the original unprefixed key. In real stores where GetWithContentType returns not-found for the unprefixed key, the copy still fails after authorization succeeds; the resolved fallback key needs to be used for the subsequent read as well.
Useful? React with 👍 / 👎.
1 participant
…ssing attachments/ prefix
When the LLM constructs an attachment key without the leading attachments/ prefix (e.g. // instead of attachments///), both image_generate and resource_copy fail because the actual filesystem path includes the prefix.