fix(backend/copilot): reference attached files via read_workspace_file, not a host path - #13470
Conversation
…e, not a host path Bot-attached files uploaded fine (AV-scanned, session-scoped in the workspace since #13464) but AutoPilot still couldn't read them: _prepare_file_attachments wrote each non-image file to the executor host's sdk_cwd and told the model 'saved to /tmp/copilot-<session>/<name>. Use the Read tool.' In E2B mode (the prod/dev default) the model's file tools operate on the remote sandbox filesystem — that host path doesn't exist there — and the built-in Read tool is disallowed in every mode. The model answered 'the path shows up but the file doesn't exist', which is exactly this split. Point the model at read_workspace_file with the file_id instead. That tool resolves the file from the workspace (GCS/DB) independent of any local disk, works in both E2B and bubblewrap modes, and is the same mechanism observed working for web-uploaded files. The sdk_cwd copy is still written for non-sandboxed tooling, but the model is no longer pointed at that path. Completes the bot file-upload chain: #13427 (upload pipeline) -> infra#348 (GCS perms) -> #13464 (session-scoped storage) -> this (readable reference).
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 0 conflict(s), 0 medium risk, 2 low risk (out of 2 PRs with file overlap) Auto-generated on push. Ignores: |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (12)
WalkthroughNon-image attachments now surface as ChangesAttachment Hint Change
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
autogpt_platform/backend/backend/copilot/sdk/service.py (1)
2846-2854: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocstring still describes the old (now-incorrect) Read-tool behavior.
The docstring says non-image files are saved to
sdk_cwd"so the CLI's built-in Read tool can access them," but the whole point of this PR is that the model must not use the Read tool / host path — it should useread_workspace_filewithfile_idinstead. This stale docstring will mislead future readers/maintainers.📝 Proposed docstring fix
- Non-image files (PDFs, text, etc.) are saved to *sdk_cwd* so the CLI's - built-in Read tool can access them. + Non-image files (PDFs, text, etc.) are described to the model via their + ``file_id`` so it can retrieve them with the ``read_workspace_file`` tool, + which works regardless of execution mode. A copy is still written to + *sdk_cwd* for non-E2B tooling, but the model is never pointed at that + host-side path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogpt_platform/backend/backend/copilot/sdk/service.py` around lines 2846 - 2854, Update the docstring on the attachment preparation logic in the service method that builds PreparedAttachments so it no longer mentions saving non-image files for the CLI built-in Read tool. Replace that stale explanation with the current behavior: non-image files are handled through read_workspace_file using file_id, while images remain embedded as vision content blocks; keep the wording aligned with the existing attachment handling symbols in this area.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@autogpt_platform/backend/backend/copilot/sdk/service.py`:
- Around line 2846-2854: Update the docstring on the attachment preparation
logic in the service method that builds PreparedAttachments so it no longer
mentions saving non-image files for the CLI built-in Read tool. Replace that
stale explanation with the current behavior: non-image files are handled through
read_workspace_file using file_id, while images remain embedded as vision
content blocks; keep the wording aligned with the existing attachment handling
symbols in this area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f8c2433-ded5-41fc-9d5b-6bd9ebfc0139
📒 Files selected for processing (2)
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: end-to-end tests
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: Check PR Status
🧰 Additional context used
📓 Path-based instructions (3)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
autogpt_platform/backend/**/*.py: Usepoetry run ...command for executing Python package dependencies
Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies likeopenpyxl
Use absolute imports withfrom backend.module import ...for cross-package imports; single-dot relative imports are acceptable for sibling modules within the same package; avoid double-dot relative imports
Do not use duck typing — avoidhasattr/getattr/isinstancefor type dispatch; use typed interfaces/unions/protocols instead
Use Pydantic models over dataclass/namedtuple/dict for structured data
Do not use linter suppressors — no# type: ignore,# noqa,# pyright: ignore; fix the type/code instead
Prefer list comprehensions over manual loop-and-append patterns
Use early return with guard clauses first to avoid deep nesting
Use%sfor deferred interpolation indebuglog statements for efficiency; use f-strings elsewhere for readability (e.g.,logger.debug("Processing %s items", count)vslogger.info(f"Processing {count} items"))
Sanitize error paths by usingos.path.basename()in error messages to avoid leaking directory structure
Be aware of TOCTOU (Time-Of-Check-Time-Of-Use) issues — avoid check-then-act patterns for file access and credit charging
Usetransaction=Truefor Redis pipelines to ensure atomicity on multi-step operations
Usemax(0, value)guards for computed values that should never be negative
Keep files under ~300 lines; if a file grows beyond this, split by responsibility (extract helpers, models, or a sub-module into a new file)
Keep functions under ~40 lines; extract named helpers when a function grows longer
...
Files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/*_test.py: Use pytest with snapshot testing for API responses
Colocate test files with source files using*_test.pynaming convention
Mock at boundaries — mock where the symbol is used, not where it's defined; after refactoring, update mock targets to match new module paths
UseAsyncMockfromunittest.mockfor async functions in tests
When writing tests, use Test-Driven Development (TDD): write failing tests marked with@pytest.mark.xfailbefore implementation, then remove the marker once the implementation is complete
When creating snapshots in tests, usepoetry run pytest path/to/test.py --snapshot-update; always review snapshot changes withgit diffbefore committing
Files:
autogpt_platform/backend/backend/copilot/sdk/service_test.py
🧠 Learnings (14)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-04-01T04:17:41.600Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12632
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-01T04:17:41.600Z
Learning: When reviewing AutoGPT Copilot tool implementations, accept that `readOnlyHint=True` (provided via `ToolAnnotations`) may be applied unconditionally to *all* tools—even tools that have side effects (e.g., `bash_exec`, `write_workspace_file`, or other write/save operations). Do **not** flag these tools for having `readOnlyHint=True`; this is intentional to enable fully-parallel dispatch by the Anthropic SDK/CLI and has been E2E validated. Only flag `readOnlyHint` issues if they conflict with the established `ToolAnnotations` behavior (e.g., missing/incorrect propagation relative to the intended annotation mechanism).
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-06-06T12:22:37.648Z
Learnt from: anvyle
Repo: Significant-Gravitas/AutoGPT PR: 13302
File: autogpt_platform/backend/backend/copilot/tools/agent_search.py:579-583
Timestamp: 2026-06-06T12:22:37.648Z
Learning: When writing LLM-facing instruction strings that trigger tool calls in this AutoGPT codebase, use the exact registered tool name `view_agent_output` (as defined in `backend/copilot/tools/agent_output.py` via its `name` property and exported via `TOOL_REGISTRY`). Do not reference the bare name `agent_output`, since it is not a valid tool name and will cause tool invocation to fail.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-03-16T16:35:40.236Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12440
File: autogpt_platform/backend/backend/api/features/workflow_import.py:54-63
Timestamp: 2026-03-16T16:35:40.236Z
Learning: Avoid using the word 'competitor' in public-facing identifiers and text. Use neutral naming for API paths, model names, function names, and UI text. Examples: rename 'CompetitorFormat' to 'SourcePlatform', 'convert_competitor_workflow' to 'convert_workflow', '/competitor-workflow' to '/workflow'. Apply this guideline to files under autogpt_platform/backend and autogpt_platform/frontend.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-03-31T15:37:38.626Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12623
File: autogpt_platform/backend/backend/copilot/tools/agent_generator/fixer.py:37-47
Timestamp: 2026-03-31T15:37:38.626Z
Learning: When validating/constructing Anthropic API model IDs in Significant-Gravitas/AutoGPT, allow the hyphen-separated Claude Opus 4.6 model ID `claude-opus-4-6` (it corresponds to `LlmModel.CLAUDE_4_6_OPUS` in `autogpt_platform/backend/backend/blocks/llm.py`). Do NOT require the dot-separated form in Anthropic contexts. Only OpenRouter routing variants should use the dot separator (e.g., `anthropic/claude-opus-4.6`); `claude-opus-4-6` should be treated as correct when passed to Anthropic, and flagged only if it’s used in the OpenRouter path where the dot form is expected.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-04-15T02:43:36.890Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12780
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-15T02:43:36.890Z
Learning: When reviewing Python exception handlers, do not flag `isinstance(e, X)` checks as dead/unreachable if the caught exception `X` is a subclass of the exception type being handled. For example, if `X` (e.g., `VirusScanError`) inherits from `ValueError` (directly or via an intermediate class) and it can be raised within an `except ValueError:` block, then `isinstance(e, X)` inside that handler is reachable and should not be treated as dead code.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-05-23T05:29:43.085Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13200
File: autogpt_platform/backend/backend/executor/scheduler.py:590-593
Timestamp: 2026-05-23T05:29:43.085Z
Learning: When reviewing Python code that uses Pydantic discriminated/tagged unions (e.g., `Annotated[Union[...], Field(discriminator="kind")]`), recognize that using `isinstance(x, SomeVariantInfo)` to narrow the union is an intentional and correct runtime guard and should also enable static type narrowing in tools like Pyright. Do not recommend replacing such `isinstance`-based narrowing with `cast(...)` when the check already proves the variant at runtime.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-04-22T11:46:04.431Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/config.py:0-0
Timestamp: 2026-04-22T11:46:04.431Z
Learning: Do not flag the Claude Sonnet 4.6 model ID as incorrect when it uses the project’s established hyphenated convention: `anthropic/claude-sonnet-4-6`. This hyphen form is the intentional, production convention and should be treated as valid (including in files like llm.py, blocks tests, reasoning.py, `_is_anthropic_model` tests, and config defaults). Note that OpenRouter also accepts the dot variant `anthropic/claude-sonnet-4.6`, so either form may be tolerated, but `anthropic/claude-sonnet-4-6` should be considered the standard to match project usage.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-04-22T11:46:12.892Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/baseline/service.py:322-332
Timestamp: 2026-04-22T11:46:12.892Z
Learning: In this codebase (Significant-Gravitas/AutoGPT), OpenRouter-routed Anthropic model IDs should use the hyphen-separated convention (e.g., `anthropic/claude-sonnet-4-6`, `anthropic/claude-opus-4-6`). Although OpenRouter may accept both hyphen and dot variants, treat the hyphen-separated form as the intended, correct codebase-wide convention and do not flag it as an error. Only flag the dot-separated variant (e.g., `anthropic/claude-sonnet-4.6`) as incorrect when reviewing/validating model ID strings for OpenRouter-routed Anthropic models.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-05-07T18:48:14.242Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13040
File: autogpt_platform/backend/backend/blocks/llm.py:0-0
Timestamp: 2026-05-07T18:48:14.242Z
Learning: In this repository, isort may split imports from the same module into separate blocks when some imports are aliased (e.g., `from module import X as Y`) and others are not. Preserve the two-block layout when it results from isort (such as keeping `from openai.types.chat import ChatCompletion as OpenAIChatCompletion` separate from non-aliased imports from `openai.types.chat`). Do not treat that split as a style issue during review; merging them into a single block can fail CI with `Imports are incorrectly sorted and/or formatted`.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-05-26T14:24:34.866Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 13217
File: autogpt_platform/backend/backend/api/features/search/service.py:137-137
Timestamp: 2026-05-26T14:24:34.866Z
Learning: In the Significant-Gravitas/AutoGPT backend, treat `user_id` (an opaque UUID used only for correlation/tracing) as non-PII. Do not flag direct logging of `user_id` in `logger.warning`/`logger.info` statements as a PII exposure issue, as the established convention is to log `user_id` for tracing while reserving PII for fields like email or display name.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
📚 Learning: 2026-06-11T19:39:10.493Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 13337
File: autogpt_platform/backend/backend/copilot/graphiti/reranker.py:0-0
Timestamp: 2026-06-11T19:39:10.493Z
Learning: In the Significant-Gravitas/AutoGPT Python backend, when calling the OpenAI Python client `chat.completions.create`, construct the `messages` payload using the concrete typed-dict variants from `openai.types.chat` (e.g., `ChatCompletionSystemMessageParam`, `ChatCompletionUserMessageParam`, etc.) rather than trying to instantiate `ChatCompletionMessageParam` directly. `ChatCompletionMessageParam` is a `Union` alias and is not constructible, so `ChatCompletionMessageParam(role=..., content=...)` should fail type checking. Build each message element with the appropriate concrete typed dict and then annotate the resulting list as `list[ChatCompletionMessageParam]` (e.g., `messages: list[ChatCompletionMessageParam] = [ChatCompletionSystemMessageParam(...), ...]`).
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_test.py
🔇 Additional comments (5)
autogpt_platform/backend/backend/copilot/sdk/service.py (2)
2897-2906: LGTM!
2916-2921: LGTM!autogpt_platform/backend/backend/copilot/sdk/service_test.py (3)
87-107: LGTM!
110-141: LGTM!
169-180: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #13470 +/- ##
==========================================
- Coverage 74.90% 74.90% -0.01%
==========================================
Files 2588 2588
Lines 194225 194239 +14
Branches 19081 19081
==========================================
- Hits 145492 145491 -1
- Misses 44578 44590 +12
- Partials 4155 4158 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…xtracts attachments Address review on #13470: _FILE_ID_RE (data/sharing/workspace_refs.py) parses 'file_id=<uuid>' out of message content to allowlist files on shared chats — the 'file_id: <uuid>' shape wouldn't match, so shared chats would lose access to bot-attached files. Match the established convention (pending_messages, list_workspace_files) and add a round-trip test through extract_workspace_file_ids so the shapes can't drift apart again. Also refresh the stale docstring that still described the old save-to-sdk_cwd Read-tool behaviour.
ntindle
left a comment
ntindle
left a comment
There was a problem hiding this comment.
Test this doesn't clog our hosts still on the remote and fill their disk because it seems like it may
github-project-automation
Bot
moved this from 🆕 Needs initial review
to 👍🏼 Mergeable
in AutoGPT development kanban
I will test in dev to check |
github-project-automation
Bot
moved this from 👍🏼 Mergeable
to ✅ Done
in AutoGPT development kanban
2 participants
Why
Discord-bot file attachments upload correctly — AV-scanned (ClamAV via
WorkspaceManager.write_file), stored session-scoped at/sessions/<session_id>/<name>(#13427 + #13464 + infra GCS perms) — but AutoPilot still can't read them. From the model's own trace in dev:Root cause (
sdk/service.py::_prepare_file_attachments): non-image attachments are written to the executor host'ssdk_cwdand the model is told "saved to<host path>. Use the Read tool." Two problems:use_e2b_sandbox=True) the model's file tools operate on the remote E2B sandbox filesystem (e2b_file_tools.pydeliberately routes non-tool-result paths tosandbox.files.read). The host-sidesdk_cwdfile doesn't exist there.Readtool the hint names is disallowed in every mode (SDK_DISALLOWED_TOOLS).So the model is pointed at a path it cannot see, with a tool it cannot use. Web-uploaded files work because the model reads them via
read_workspace_file(observed in production:Read test.txt from workspace:/sessions/<id>/test.txt) — which resolves byfile_idfrom the workspace, independent of any local disk.What
Point the model at
read_workspace_filewith thefile_idfor non-image attachments, instead of a host path + the disallowedReadtool.How
In
_prepare_file_attachments:file_id: <id>instead of the host path.read_workspace_file(withsave_to_pathmentioned for copying into the working directory) instead of "Use the Read tool".sdk_cwdcopy is still written, preserving the working-directory affordance for non-E2B tooling — the model just isn't pointed at it.Blast radius: this function only runs for turns with attached
file_ids. Web attachment turns get the same instruction that already matches their observed working behaviour; everything else is untouched.Completes the bot file-upload chain: #13427 (upload pipeline) → infra#348 (GCS write perms) → #13464 (session-scoped storage) → this (readable reference).
Testing
sdk/service_test.py::TestPrepareFileAttachmentspinning the new hint (file_id present, host path absent,read_workspace_filenamed, no "Use the Read tool") — failed before the fix, 7/7 pass after.service_test.pyrun compared against cleandev— identical pre-existing failure set (env-dependent tests under--noconftest), zero new failures.ruff/black/isortclean.