Repository navigation
docs(research): plan attachment fixes without a new primitive - #4625
AndrewBarba wants to merge 3 commits into
Conversation
![roboeve[bot]](https://avatars.githubusercontent.com/b/10660?s=60&v=4){kind=small}
| When `fetchFile` returns `null`, or the channel has none, eve fetches public | ||
| `https:` links itself. The fetch rejects private and reserved addresses, stops |
There was a problem hiding this comment.
The fetcher needs a tighter spec than "rejects private and reserved addresses". If eve only checks the hostname or the first resolved IP, a public https: URL can still 30x to 169.254.169.254 or rebind DNS between the check and the connect. Today's resolvers call plain fetch with default redirect handling (packages/eve/src/public/channels/slack/attachments.ts:183-193), so there's nothing to copy from.
Suggest saying: resolve and pin the IP, reject private/reserved v4 and v6 (including mapped forms), follow redirects manually with a hop limit and recheck each target, and stream with a byte cap and timeout. Then add the redirect-to-private and rebinding tests to Verification.
| - Channels don't filter attachments by media type. `uploadPolicy` decides what | ||
| eve accepts, and the inline decision decides inline or label. |
There was a problem hiding this comment.
uploadPolicy runs before staging, against the declared media type, and it skips the size check when the length isn't known. A URL's length never is (packages/eve/src/public/channels/upload-policy.ts:148-153, packages/eve/src/internal/attachments/data.ts:73-80). Staging then writes whatever the resolver returned and never checks the policy again (packages/eve/src/harness/attachment-staging.ts:485-497).
Under this plan, a part declared as image/png gets through an image/* allowlist, gets fetched, and gets restaged as application/octet-stream (L102-104). A custom maxBytes below 25 MiB also never applies to fetched links, because L67 caps at the default. The plan should rerun the effective policy after resolution and sniffing, using the real byte count and the verified type, and cap the download at the channel's maxBytes. Small one: the default is 25 * 1024 * 1024 (upload-policy.ts:35-38), so write 25 MiB to match the other limits.
|
|
||
| | Surface | Change | | ||
| | -------------------------- | -------------------------------------------------------------------------------------------------------- | | ||
| | `FilePart.data` | Bytes, base64, and `data:` URLs are bytes. Any other string or `URL` with a scheme is a link to resolve. | |
There was a problem hiding this comment.
"Any other string or URL with a scheme is a link to resolve" also matches eve-sandbox:, eve-url:, and eve-attachment:. Caller-supplied internal refs are a known privileged-read vector: the HTTP route rejects them for that reason (packages/eve/src/eve-channel/request.ts:481-488, see also packages/eve/src/internal/attachments/url-refs.ts:29-45). Please say explicitly that internal schemes are rejected at every untrusted boundary (channel input and send() payloads), and that only refs minted by staging are trusted. That matters most in step 2, which removes eve-attachment: and touches this same classification.
| | Surface | Change | | ||
| | -------------------------- | -------------------------------------------------------------------------------------------------------- | | ||
| | `FilePart.data` | Bytes, base64, and `data:` URLs are bytes. Any other string or `URL` with a scheme is a link to resolve. | | ||
| | `fetchFile(url, ctx)` | Also on `eveChannel()`. Returning `null` now means "not mine", not "let the provider fetch it". | |
There was a problem hiding this comment.
This breaks a documented public contract. Today null means "pass the URL through to the model provider unchanged" (packages/eve/src/shared/channel-definition.ts:97-103, packages/eve/src/channel/adapter.ts:160-166), and staging returns the part as-is (packages/eve/src/harness/attachment-staging.ts:471-472). A custom fetchFile that relies on provider fetching for non-https: or non-public URLs will now get a note. Please add a migration note covering who's affected, what to return instead, and whether it needs a changeset or major bump.
| 7. `attachmentError` and `FetchFileContext.session` (#4304), and a `deliver` | ||
| that returns nothing falls back to the default input. |
There was a problem hiding this comment.
This flips existing void semantics. deliver is typed StepInput | void (packages/eve/src/shared/channel-definition.ts:67), and right now a custom deliver that returns undefined suppresses that payload. The default projection only runs when there's no deliver at all (packages/eve/src/execution/session/turn-step.ts:320-326). A handler that returns nothing on purpose to consume or drop a delivery would start sending the raw payload to the model. The plan needs an explicit drop result, or it should keep void as "drop" and add a separate opt-in. Either way, give it a migration note and a test for current void-returning handlers. This also doesn't fit under the attachments heading, so explain why it ships in this step.
| - History holds only `eve-sandbox:` refs or text notes for attachments. It | ||
| never holds a raw URL, an `eve-url:` marker, or inline bytes. This covers user | ||
| messages, cancelled turns, and tool results. |
There was a problem hiding this comment.
The rollout only enforces this for new input. Staging covers the current delivery (packages/eve/src/harness/step/intake.ts:88-99), and hydrateSandboxAttachments returns history unchanged when it has no sandbox ref (packages/eve/src/harness/attachment-staging.ts:174-180). So sessions that already have raw URLs or inline bytes in history will keep sending them, and keep failing, after the upgrade. Those are exactly the stuck #3419/#855 sessions. Old refs also keep their stored, unverified media type for the inline decision (packages/eve/src/internal/attachments/sandbox-refs.ts:124-127). Please add a model-call normalization step that turns pre-existing raw parts into notes and renders unverified old refs label-only, and test it with pre-upgrade history, not just old-ref hydration.
| holds. It inlines only formats that eve verified from the bytes. | ||
| - Every attachment the model sees has a label with its sandbox path. | ||
| - Channels don't filter attachments by media type. `uploadPolicy` decides what | ||
| eve accepts, and the inline decision decides inline or label. |
There was a problem hiding this comment.
Slack isn't the only channel that filters by media type. Telegram only builds attachments from photo and document, and drops audio, voice, and video (packages/eve/src/public/channels/telegram/inbound.ts:221-230). The rollout only fixes Slack (step 8). Either narrow the invariant or add a step for the other built-in channels.
| import { eveChannel } from "eve/channels/eve"; | ||
| import { attachmentError } from "eve/channels"; | ||
|
|
||
| export default eveChannel({ |
There was a problem hiding this comment.
This example won't type-check: auth is required on EveChannelInput (packages/eve/src/eve-channel/types.ts:103), and TOKEN isn't defined. Add an auth entry and read the token from process.env so authors can copy it as-is.
Signed-off-by: Andrew Barba <barba@hey.com>
…ction Signed-off-by: Andrew Barba <barba@hey.com>
1905484 to
d5cc259
Compare
…d deliver change Signed-off-by: Andrew Barba <barba@hey.com>
|
Collapsed into #4635. |
This branch was successfully deployed
1 participant
Summary
Attachment bugs keep breaking whole sessions. When eve can't resolve a file, or a provider rejects it, the file stays in history and every later model call fails. #4223 proposed a pluggable attachment store, which is more than these bugs need. This plan keeps today's storage and fixes the pipeline around one rule: every attachment becomes a staged file or a note before it enters history. It adds three small authoring surfaces,
fetchFileoneveChannel(),attachmentError, andFetchFileContext.session, and no new definition type. It replaces #4223, and the PRs above this one in the stack implement it.Related to #3419, #855, #705, #497, #4194, #4304, and #543.
Validation
pnpm docs:checkpasses.oxfmtformatted the plan.Checklist
evepackagegit commit --signoff)