feat(runner): first-class current user turn and attachment delivery - #5615
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe runner adds attachment-aware protocol contracts, authenticated fetching and claiming, workspace restoration, capability-gated prompt delivery, attachment events, credential propagation, persistence, history reconstruction, and validation across cold, keep-alive, and approval-resume turns. ChangesAttachment-aware turn contracts
Workspace restoration
Prompt delivery
Run orchestration
Implementation record and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant SessionAPI
participant runTurn
participant Sandbox
Client->>Server: Submit user turn with attachment references
Server->>Server: Validate attachment count
Server->>SessionAPI: Persist and claim fresh-turn attachments
Server->>runTurn: Start turn with credential accessor
runTurn->>SessionAPI: Fetch attachment bytes
runTurn->>Sandbox: Materialize working copies
runTurn->>runTurn: Apply capability gate and build prompt blocks
runTurn->>Sandbox: Send attachment-aware prompt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
| } | ||
|
|
||
| /** Read only the tail message when it is a user turn. */ | ||
| export function currentUserTurn(request: AgentRunRequest): CurrentUserTurn { |
There was a problem hiding this comment.
The core of the package: the one authority on "what did the person just send". Four sites previously derived this from "the most recent non-empty user text" and each failed differently on an attachment-only turn (resend an earlier prompt, fail freshness, collide fingerprints, skip persistence). It reads the tail only; resolvePromptText survives as the documented approval-resume fallback.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/runner/src/protocol.ts (1)
13-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSync the attachment /run changes into the Python wire contract.
services/runner/src/protocol.tsnow includes"attachment"content blocks andAttachmentRef, plusmodelCapabilities. Add the matching fields tosdks/python/agenta/sdk/agents/wire_models.pyandsdks/python/agenta/sdk/agents/utils/wire.py, and update the shared/rungolden fixtures and contract tests so the runner and Python SDK agree on the wire contract.Source: Coding guidelines
🧹 Nitpick comments (7)
services/runner/src/engines/sandbox_agent/run-turn.ts (1)
306-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the forced image modality for legacy inline images.
Line 312 passes
modelCapabilities: { inputModalities: ["image"] }instead ofrequest.modelCapabilities. This bypasses the model-modality gate for legacy inline images and keeps the pre-existing behavior. The intent is not obvious at the call site, and a later reader can "fix" it intorequest.modelCapabilities, which would silently drop images for models with unknown modalities. Add a short comment that states the compatibility reason.♻️ Proposed comment
for (const image of legacyImages) { + // Legacy inline images predate modality resolution: assert "image" so an unknown or + // absent `request.modelCapabilities` keeps delivering them exactly as before. Only the + // transport, adapter, and provider inline caps still gate them. const gate = attachmentCapabilityGate({services/runner/tests/unit/sandbox-agent-orchestration.test.ts (1)
419-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the two degradation cases into separate test cases.
The loop runs both cases inside one
it. If the first case fails, Vitest reports one failure and never runs the second case. Useit.eachso each case reports independently and the failure names the input.♻️ Proposed refactor
- it("a legacy inline image degrades rather than throws", async () => { - for (const testCase of [ - { mimeType: "image/png", capabilities: { images: false } }, - { mimeType: "image/svg+xml", capabilities: { images: true } }, - ]) { - const { calls, deps, events } = fakeHarness({ - capabilities: testCase.capabilities, - }); + it.each([ + { mimeType: "image/png", capabilities: { images: false } }, + { mimeType: "image/svg+xml", capabilities: { images: true } }, + ])( + "a legacy inline image degrades rather than throws ($mimeType)", + async (testCase) => { + const { calls, deps, events } = fakeHarness({ + capabilities: testCase.capabilities, + }); const result = await runSandboxAgent( { harness: "pi_core", messages: [ { role: "user", content: [ { type: "image", uri: `data:${testCase.mimeType};base64,AQID`, }, { type: "text", text: "inspect this" }, ], }, ], }, undefined, undefined, deps, ); - assert.equal(result.ok, true, testCase.mimeType); + assert.equal(result.ok, true); assert.deepEqual(calls.promptBlocks, [ { type: "text", text: "inspect this" }, ]); assert.equal( events.some((event) => event.type === "attachment_delivery"), false, ); - } - }); + }, + );services/runner/tests/unit/attachment-materialize.test.ts (1)
200-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the pool reaches the configured concurrency.
assert.ok(maxActive <= 2)passes when the pool runs fully serially. The test name claims bounded concurrency, so assert both bounds. Four attachments need a fetch and each fetch sleeps 5 ms, so a pool of two reachesmaxActive === 2deterministically.♻️ Proposed change
- assert.ok(maxActive <= 2); + assert.equal(maxActive, 2);services/runner/tests/unit/attachment-capability-gate.test.ts (1)
98-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the oversized byte length from the base64 cap and pin the under-cap side.
Math.floor(7.5 * 1024 * 1024) + 1hides the reason 7.5 MiB is the trigger:base64Lengthexpands bytes by 4/3, so 7.5 MiB of bytes equals exactly the 10 MiB base64 cap inattachments.ts. The literal is repeated twice and encodes that ratio implicitly.Name the boundary, and assert the just-under case as well. Without it, a regression that removes the cap check entirely still fails only one direction.
♻️ Proposed change
+// `attachments.ts` caps inline base64 at 10 MiB, and base64 expands bytes by 4/3. +const INLINE_CAP_BYTES = (10 * 1024 * 1024 * 3) / 4; + it("keeps an oversized Claude image workspace-only", () => { assert.equal( attachmentCapabilityGate({ ...IMAGE_INPUT, - byteLength: Math.floor(7.5 * 1024 * 1024) + 1, + byteLength: INLINE_CAP_BYTES + 1, }).reasonCode, "provider_inline_cap", ); + assert.equal( + attachmentCapabilityGate({ + ...IMAGE_INPUT, + byteLength: INLINE_CAP_BYTES, + }).outcome, + "native", + ); }); it("keys the inline cap on provider, with ACP agent as the absent-provider fallback", () => { - const byteLength = Math.floor(7.5 * 1024 * 1024) + 1; + const byteLength = INLINE_CAP_BYTES + 1;services/runner/tests/unit/attachment-client.test.ts (1)
48-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a response that omits the verified headers.
fetchAttachmentinservices/runner/src/sessions/attachments.tsthrows"missing verified attachment headers"and returnsnullwhencontent-typeorcontent-dispositionis absent. That branch is what keeps unverified wire metadata out of working-path construction, and no test covers it.Add a 200 response with no
content-dispositionand assert the result isnull.💚 Proposed test
+ it("returns null when the verified headers are absent", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { "content-type": "image/png" }, + }), + ), + ); + assert.equal( + await fetchAttachment("session-1", ATTACHMENT_ID, () => "Bearer test"), + null, + ); + }); + it("parses quoted filename when filename* is absent", () => {services/runner/tests/unit/server.test.ts (1)
450-459: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sessionApiCallsrecords every non-/runURL, including the OTLP endpoint.The request body configures an OTLP exporter at
${s.url}/otlp/v1/traces. That URL is not${s.url}/run, so any telemetry export lands insessionApiCalls. The assertionassert.deepEqual(sessionApiCalls, [])then reports a session API call when the real cause is a trace export. Filter the telemetry endpoint so the failure names the right cause.♻️ Proposed change
.mockImplementation(async (input, init) => { const url = String(input); if (url === `${s.url}/run`) return realFetch(input, init); + if (url.startsWith(`${s.url}/otlp/`)) { + return new Response("{}", { status: 200 }); + } sessionApiCalls.push(url); return new Response("{}", { status: 200 }); });services/runner/src/engines/sandbox_agent/attachments.ts (1)
565-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the cold-frame label instead of hard-coding it.
translations.tsbuildsContinue the conversation. The user现在 says:\n${latest}, butattachments.tsandattachment-prompt-blocks.test.tsduplicate the label and trailing newline. Export the label from the transcript frame builder, import it inbuildPromptBlocks, and use it in the tests. If the producer wording changes, duplicates can drift without failing and attachment-only mentions will jump behind the prompt mentions instead of staying after the label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1080a48f-1b1c-419c-8cb3-ca658fc97ec1
📒 Files selected for processing (31)
docs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mdservices/runner/src/engines/sandbox_agent/attachments.tsservices/runner/src/engines/sandbox_agent/capabilities.tsservices/runner/src/engines/sandbox_agent/engine.tsservices/runner/src/engines/sandbox_agent/reconstruct-history.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/session-identity.tsservices/runner/src/engines/sandbox_agent/transcript.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/src/sessions/attachments.tsservices/runner/src/sessions/reconstruct.tsservices/runner/tests/unit/attachment-capability-gate.test.tsservices/runner/tests/unit/attachment-client.test.tsservices/runner/tests/unit/attachment-delivery-events.test.tsservices/runner/tests/unit/attachment-materialize.test.tsservices/runner/tests/unit/attachment-path-safety.test.tsservices/runner/tests/unit/attachment-prompt-blocks.test.tsservices/runner/tests/unit/current-user-turn.test.tsservices/runner/tests/unit/sandbox-agent-capabilities.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-persist.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/session-reconstruct-history.test.tsservices/runner/tests/unit/session-reconstruct.test.tsservices/runner/tests/unit/transcript.test.tsservices/runner/tests/unit/wire-contract.test.ts
ea80bd3 to
99f3088
Compare
dosubot
Bot
added
size:XXL
Railway Preview Environment
Updated at 2026-08-01T18:14:03.591Z |
cbd3ed8 to
c794326
Compare
7a96b5c to
ea5fd57
Compare
c794326 to
76cd766
Compare
ea5fd57 to
020b56d
Compare
|
🤖 The AI agent says: On the wire-contract sync request (the outside-diff comment): no change is needed, because the two sides are already pinned to each other. The Python side ships one lane higher, in #5617. Three related hardening changes also landed in 208fb31. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
services/runner/src/engines/sandbox_agent/attachments.ts (2)
372-388: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider re-checking the Daytona directory after
mkdirFs, and reuse the configured timeout.
rejectDaytonaSymlinksruns beforemkdirFs, sopath.directoryusually does not exist yet andtest -Lpasses without verifying anything. AftermkdirFscreates the directory, no second check runs beforewriteFsFile. The local path re-checks all three components aftermkdirSync(Line 296), so the two backends do not give the same guarantee.
rejectDaytonaSymlinksalso hardcodestimeoutMs: 15_000(Line 345) whileattachmentRestoreTimeoutMs()is configurable. Pass the bound in instead of duplicating the value.♻️ Proposed re-check after directory creation
await rejectDaytonaSymlinks(sandbox, [ path.root, path.directory, path.absolute, ]); await sandbox.mkdirFs({ path: path.directory }); + await rejectDaytonaSymlinks(sandbox, [path.directory, path.absolute]);
511-523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport an unknown modality with an "unknown" reason code in both branches.
modelModalityStatereturns only"supported"or"unknown". The comment at Line 463 states that absence is not a negative capability. WhenrequireNativeis set, this branch still reportsmodel_modality_unsupported, so the persisted delivery event claims the model declared no support for the modality. Operators cannot separate "the catalog said nothing" from "the catalog said no".Use
model_modality_unknownin both branches and keep the outcome difference.♻️ Proposed change
if (modelState === "unknown") { return input.requireNative ? { outcome: "failed", - reasonCode: "model_modality_unsupported", + reasonCode: "model_modality_unknown", kind, } : { outcome: "workspace_only", reasonCode: "model_modality_unknown", kind, }; }services/runner/src/engines/sandbox_agent/transcript.ts (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBreak the import cycle between
transcript.tsandattachments.ts.
transcript.tsimportsattachmentMentionfrom./attachments.ts, whileattachments.tsimportsCOLD_FRAME_USER_LABELfrom./transcript.ts. Both are call-time usages, so this does not currently create a TDZ failure, but the cycle makes future top-level changes fragile. MoveCOLD_FRAME_USER_LABELand the mention helpers into a shared leaf module such asattachment-text.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b0ed2c2-835c-46ad-a27c-af20a8e510fe
📒 Files selected for processing (31)
docs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mdservices/runner/src/engines/sandbox_agent/attachments.tsservices/runner/src/engines/sandbox_agent/capabilities.tsservices/runner/src/engines/sandbox_agent/engine.tsservices/runner/src/engines/sandbox_agent/reconstruct-history.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/session-identity.tsservices/runner/src/engines/sandbox_agent/transcript.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/src/sessions/attachments.tsservices/runner/src/sessions/reconstruct.tsservices/runner/tests/unit/attachment-capability-gate.test.tsservices/runner/tests/unit/attachment-client.test.tsservices/runner/tests/unit/attachment-delivery-events.test.tsservices/runner/tests/unit/attachment-materialize.test.tsservices/runner/tests/unit/attachment-path-safety.test.tsservices/runner/tests/unit/attachment-prompt-blocks.test.tsservices/runner/tests/unit/current-user-turn.test.tsservices/runner/tests/unit/sandbox-agent-capabilities.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-persist.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/session-reconstruct-history.test.tsservices/runner/tests/unit/session-reconstruct.test.tsservices/runner/tests/unit/transcript.test.tsservices/runner/tests/unit/wire-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- services/runner/tests/unit/wire-contract.test.ts
- services/runner/tests/unit/attachment-prompt-blocks.test.ts
- services/runner/src/engines/sandbox_agent/capabilities.ts
- services/runner/tests/unit/session-reconstruct.test.ts
- services/runner/tests/unit/session-reconstruct-history.test.ts
- services/runner/src/engines/sandbox_agent/runtime-contracts.ts
- services/runner/src/sessions/reconstruct.ts
- services/runner/tests/unit/attachment-path-safety.test.ts
- services/runner/src/engines/sandbox_agent/engine.ts
- services/runner/tests/unit/sandbox-agent-capabilities.test.ts
- services/runner/tests/unit/attachment-delivery-events.test.ts
- services/runner/tests/unit/attachment-capability-gate.test.ts
- services/runner/src/engines/sandbox_agent/reconstruct-history.ts
- services/runner/tests/unit/current-user-turn.test.ts
- services/runner/src/engines/sandbox_agent/run-plan.ts
- services/runner/tests/unit/session-persist.test.ts
- services/runner/tests/unit/sandbox-agent-run-plan.test.ts
- services/runner/tests/unit/session-pool.test.ts
- services/runner/src/engines/sandbox_agent/run-turn.ts
- services/runner/src/server.ts
| const provider = input.provider?.trim().toLowerCase(); | ||
| // Older callers omit provider, so retain the ACP-agent heuristic only as that fallback. | ||
| const usesAnthropicInlineLimit = provider | ||
| ? provider === "anthropic" | ||
| : input.acpAgent === "claude"; | ||
| if ( | ||
| kind === "image" && | ||
| usesAnthropicInlineLimit && | ||
| base64Length(input.byteLength) > CLAUDE_INLINE_BASE64_MAX_BYTES | ||
| ) { | ||
| return { | ||
| outcome: "workspace_only", | ||
| reasonCode: "provider_inline_cap", | ||
| kind, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate every attachmentCapabilityGate caller and check which pass requireNative.
set -euo pipefail
echo "== gate call sites =="
rg -nP --type=ts -C 10 '\battachmentCapabilityGate\s*\(' services/runner
echo "== requireNative usages =="
rg -nP --type=ts -C 6 '\brequireNative\b' services/runnerRepository: Agenta-AI/agenta
Length of output: 28285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== attachments.ts relevant section =="
sed -n '471,545p' services/runner/src/engines/sandbox_agent/attachments.ts
echo "== gate call sites relevant section =="
sed -n '300,872p' services/runner/src/engines/sandbox_agent/run-turn.ts | nl -ba | sed -n '1,220p'
echo "== attachments call construction =="
sed -n '830,876p' services/runner/src/engines/sandbox_agent/run-turn.ts
echo "== requireNative only in source =="
rg -n '\brequireNative\b' services/runner/src || true
echo "== tests cover provider_inline_cap + requireNative? =="
rg -n -C 3 'provider_inline_cap|requireNative' services/runner/tests/unit/attachment-capability-gate.test.ts || true
echo "== deterministic source-code invariant check =="
python3 - <<'PY'
from pathlib import Path
import re
src = Path('services/runner/src/engines/sandbox_agent/attachments.ts').read_text()
gate = re.search(r'export function attachmentCapabilityGate\(\{\d*[\s\S]*?\n\}\): AttachmentGate \{', src).group(0)
checks = {
'transport_returns_failed_when_requireNative': 'outcome: input.requireNative ? "failed" : "workspace_only"' in gate and 'transport_unsupported' in gate,
'adapter_returns_failed_when_requireNative': 'outcome: input.requireNative ? "failed" : "workspace_only"' in gate and 'adapter_unsupported' in gate,
'model_unknown_returns_failed_when_requireNative': 'input.requireNative' in gate and 'model_modality_unsupported' in gate,
'inline_cap_returns_fixed_workspace_only': re.search(r'provider_inline_cap', gate) and 'outcome: "workspace_only",' in gate[gate.find('provider_inline_cap'):gate.find('provider_inline_cap')+500],
}
print(checks)
print('attached native callers:', re.search(r'modelCapabilities,|requireNative', Path('services/runner/src/engines/sandbox_agent/run-turn.ts').read_text()))
PYRepository: Agenta-AI/agenta
Length of output: 2330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact provider_inline_cap branch =="
sed -n '525,542p' services/runner/src/engines/sandbox_agent/attachments.ts
echo "== requireNative only in source files =="
rg -n '\brequireNative\b' services/runner/src || true
echo "== attachment capability gate tests =="
sed -n '1,180p' services/runner/tests/unit/attachment-capability-gate.test.ts
echo "== all requireNative call sites in runner tests and source =="
rg -n '"requireNative":\s*true|requireNative:' services/runner/src services/runner/tests || true
echo "== deterministic source-code invariant check =="
python3 - <<'PY'
from pathlib import Path
import re
src = Path('services/runner/src/engines/sandbox_agent/attachments.ts').read_text()
branch_match = re.search(r'if \([\s\S]*?provider_inline_cap.*?\n\s*\}', src)
print('provider_inline_cap branch text:')
print(branch_match.group(0) if branch_match else 'NOT_FOUND')
print('contains requireNative:', 'requireNative' in (branch_match.group(0) if branch_match else ''))
print('contains outcome workspace_only:', 'outcome: "workspace_only"' in (branch_match.group(0) if branch_match else ''))
PYRepository: Agenta-AI/agenta
Length of output: 6264
Honor requireNative for the provider inline-size cap.
The inline-size branch returns workspace_only unconditionally, while the transport, adapter, and model-modality branches fail closed when input.requireNative is true. Return failed for provider_inline_cap when native delivery is required.
| for (const ref of refs) { | ||
| try { | ||
| assertCanonicalAttachmentId(ref.attachmentId); | ||
| } catch { | ||
| input.emit({ | ||
| type: "attachment_delivery", | ||
| attachmentId: ref.attachmentId, | ||
| outcome: "failed", | ||
| reasonCode: "contract_violation", | ||
| }); | ||
| failure ??= new Error("Attachment reference is invalid."); | ||
| continue; | ||
| } | ||
|
|
||
| const fetched = await fetchAttachment( | ||
| input.sessionId, | ||
| ref.attachmentId, | ||
| input.auth, | ||
| ); | ||
| if (!fetched) { | ||
| input.emit({ | ||
| type: "attachment_delivery", | ||
| attachmentId: ref.attachmentId, | ||
| outcome: "failed", | ||
| reasonCode: "fetch_failed", | ||
| }); | ||
| resolved.push({ | ||
| ref, | ||
| gate: { | ||
| outcome: "failed", | ||
| reasonCode: "fetch_failed", | ||
| kind: attachmentKind(ref.mediaType), | ||
| }, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| const authoritative = verifiedRef(ref, fetched); | ||
| let path: AttachmentPath; | ||
| try { | ||
| path = attachmentWorkingPath(input.plan.cwd, authoritative); | ||
| await materializeWorkingCopy( | ||
| input.sandbox, | ||
| input.plan, | ||
| authoritative, | ||
| fetched.bytes, | ||
| ); | ||
| } catch { | ||
| input.emit({ | ||
| type: "attachment_delivery", | ||
| attachmentId: ref.attachmentId, | ||
| outcome: "failed", | ||
| reasonCode: "materialize_failed", | ||
| }); | ||
| resolved.push({ | ||
| ref: authoritative, | ||
| gate: { | ||
| outcome: "failed", | ||
| reasonCode: "materialize_failed", | ||
| kind: attachmentKind(authoritative.mediaType), | ||
| }, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| const gate = attachmentCapabilityGate({ | ||
| acpAgent: input.plan.acpAgent, | ||
| provider: input.provider, | ||
| capabilities: input.capabilities, | ||
| modelCapabilities: input.modelCapabilities, | ||
| mediaType: authoritative.mediaType ?? "", | ||
| byteLength: fetched.bytes.byteLength, | ||
| }); | ||
| if (gate.outcome === "failed") { | ||
| input.emit({ | ||
| type: "attachment_delivery", | ||
| attachmentId: ref.attachmentId, | ||
| outcome: "failed", | ||
| reasonCode: gate.reasonCode, | ||
| workingPath: path.relative, | ||
| }); | ||
| failure ??= new Error( | ||
| attachmentDeliveryUnsupportedMessage( | ||
| input.plan.harness, | ||
| gate.kind, | ||
| gate.missing ?? "required capability", | ||
| ), | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| input.emit({ | ||
| type: "attachment_delivery", | ||
| attachmentId: ref.attachmentId, | ||
| outcome: gate.outcome, | ||
| reasonCode: gate.reasonCode, | ||
| workingPath: path.relative, | ||
| }); | ||
| resolved.push({ | ||
| ref: authoritative, | ||
| bytes: fetched.bytes, | ||
| path, | ||
| gate, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound the current-turn fetch loop the way the restore path is bounded.
This loop fetches and materializes every attachment serially on the turn path. attachmentCountError allows up to AGENTA_ATTACHMENTS_MAX_PER_TURN attachments, default 100, and fetchAttachment uses a 15 s timeout per request. A turn with many slow attachments therefore adds up to 100 sequential timeouts before the model is called.
restoreReferencedWorkingCopies already uses attachmentRestoreConcurrency() and withTimeout. Apply the same bounded concurrency and per-attachment deadline here. Keep the current ordering of emitted events so the record log stays deterministic.
| "44444444-4444-4444-8444-444444444444", | ||
| "55555555-5555-4555-8555-555555555555", | ||
| ]; | ||
| const existing = { attachmentId: ids[0], filename: `.png` }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the placeholder-free template literals with an explicit filename.
Lines 180, 201, and 236 use `.png` as the filename. The template literals contain no interpolation, which indicates a lost expression such as ${ids[0]}.png. The test still passes, because validatedFilename accepts .png. The fixture is confusing, and the "existing copy skips fetch" assertion depends on this filename matching the pre-materialized copy exactly.
Use a plain string literal with a clear name so the intent is explicit.
🧹 Proposed fixture cleanup
- const existing = { attachmentId: ids[0], filename: `.png` };
+ const EXISTING_FILENAME = "existing.png";
+ const existing = { attachmentId: ids[0], filename: EXISTING_FILENAME };- "content-disposition": `attachment; filename*=UTF-8''.png`,
+ "content-disposition": "attachment; filename*=UTF-8''untrusted.png", content: ids.map((attachmentId) => ({
type: "attachment",
attachmentId,
- filename: attachmentId === ids[0] ? ".png" : "untrusted.png",
+ filename: attachmentId === ids[0] ? EXISTING_FILENAME : "untrusted.png",
})), assert.equal(
Array.isArray(restored[0].content)
? restored[0].content[0].filename
: undefined,
- `.png`,
+ EXISTING_FILENAME,
);Also applies to: 201-201, 236-236
| it("degrades a legacy inline image when the caller's model declares no image input", async () => { | ||
| const { calls, deps, logs } = fakeHarness({ | ||
| capabilities: { images: true }, | ||
| }); | ||
|
|
||
| const result = await runSandboxAgent( | ||
| { | ||
| harness: "pi_core", | ||
| modelCapabilities: { inputModalities: ["text"] }, | ||
| messages: [ | ||
| { | ||
| role: "user", | ||
| content: [ | ||
| { type: "image", uri: "data:image/png;base64,AQID" }, | ||
| { type: "text", text: "inspect this" }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| undefined, | ||
| undefined, | ||
| deps, | ||
| ); | ||
|
|
||
| assert.equal(result.ok, true); | ||
| assert.deepEqual(calls.promptBlocks, [ | ||
| { type: "text", text: "inspect this" }, | ||
| ]); | ||
| assert.equal( | ||
| logs.find((message) => message.includes("legacy inline image")), | ||
| "[attachments] legacy inline image delivery=degraded reason=model_modality_unknown", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the modality reason-code mapping logic to confirm intended semantics.
rg -n "model_modality" services/runner/src/engines/sandbox_agentRepository: Agenta-AI/agenta
Length of output: 558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== attachment modality mapping =="
sed -n '30,60p;500,530p' services/runner/src/engines/sandbox_agent/attachments.ts
echo
echo "== relevant tests =="
sed -n '520,575p' services/runner/tests/unit/sandbox-agent-orchestration.test.tsRepository: Agenta-AI/agenta
Length of output: 3334
Fix the reason code for an explicit non-image modality list.
modelCapabilities: { inputModalities: ["text"] } is an explicit declaration that image input is not supported, but the test expects model_modality_unknown. Use model_modality_unsupported unless the mapping intentionally covers explicit exclusions under that same reason.
| const text = buildTurnText(request); | ||
|
|
||
| // The attachment is the new turn, so the settled call must not be replayed as a resume. | ||
| assert.match(text, /Continue the conversation\. The user now says:/); | ||
| assert.doesNotMatch(text, /responded to the request/); | ||
| assert.doesNotMatch(text, /responded to the pending approval/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the attachment becomes the current turn.
These assertions only check the normal closing text and the absence of two known resume phrases. They do not verify that currentUserTurn selected the attachment message or that the attachment reached latest. Add an assertion for the attachment-specific output after the closing frame, such as the verified mention or [attachment pending verification] for this fixture.
Part one gives the runner a first-class notion of the current user turn
(currentUserTurn in protocol.ts, tail-only). Four sites derived it from 'the
most recent non-empty user text' and each failed differently on an
attachment-only turn: the empty-turn rejection ('No user message to send'),
the freshness check, the history fingerprint (which now hashes ordered
attachment ids), and user-record persistence. An attachment-only turn is now
valid; resolvePromptText survives only as the documented approval-resume
fallback.
Part two is delivery: references resolve through the WP1 API routes (bounded
fetch, non-fatal claim), working copies materialize at
cwd/attachments/<id>/<filename> under strict path safety (UUID-validated ids,
RFC 5987 filename parsing, containment, symlink rejection, create-exclusive
writes with a wx fallback; the Daytona check-then-write race is documented and
accepted), the three-layer capability gate treats unknown model modalities as
workspace-only (never assumed vision; capabilities join the config
fingerprint), legacy inline data-URL images keep working via the dual read,
every current-turn attachment gets a mention line so the prompt is never
empty, cold starts restore referenced working copies with bounded concurrency,
and every outcome emits a persisted attachment_delivery event with a stable
reason code.
The two parts share hunks in seven files, so they land as one commit; the
conceptual split and the refactor's independent verification (155 tests before
any delivery work) are recorded in the stage protocol.
Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…me base run-plan, run-turn, and the run-plan tests build on the approval-resume changes in the same seams, which is why this lane now sits above that one in the stack. Completes the previous commit; the two form one package. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…review The rewritten empty-turn rejection was discarding human approvals: an in-band approval reply carries the full transcript, so the tail-only reading rejected it with 'No user message to send'. The rejection now falls back to the backward scan, and the three real shapes (in-band approval, tool-role tail, client-tool-result tail) are pinned as tests. Historical restore no longer throws: a single unrestorable attachment (for example swept after a failed claim) degrades to a 'no longer available' mention instead of permanently bricking every cold turn of its conversation. Legacy inline images degrade exactly as before this package when native delivery is not possible, instead of hard-failing turns in five reachable configurations. Cold restore checks existence before downloading; current-turn fetch and write failures degrade that attachment with honest reason codes (fetch_failed, materialize_failed); an over-cap turn is rejected before persisting its record; the Daytona symlink check runs before mkdir and is time-bounded; mentions moved into the latest-user-message position of the cold frame; the provider inline cap keys on the resolved provider. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
… turn Live QA on the deployed stack still reproduced the original bug: the current-turn reading counted only real attachment blocks, and today's front end sends inline data-URL blocks, so an image-only turn looked empty to the plan guard. A shared legacy-inline-image predicate now feeds the current-turn reading, the plan guard, freshness, and persistence; a native image-only prompt is the image block alone, a degraded one gets a non-empty fallback; and the legacy delivery path logs its decision, since it has no attachment id to key a delivery event. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…orted The model catalogs only ever enumerate text and image, so asserting 'model_modality_unsupported' for a document against such a list states a false negative the catalog cannot back. A modality absent from the list now yields unknown (workspace-only with model_modality_unknown); the unsupported code stays defined for a future catalog that enumerates negatives. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
The product owner raised the limit from the dark-shipped 5; provider count caps sit far higher, and the per-session quotas are the real bound. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…ification after review
76cd766 to
f9f3de9
Compare
020b56d to
2cde2f5
Compare
…ng the history tail (#5638)
1 participant
Sending an image without text fails the run with "Agent run failed: No user message to send (prompt/messages empty)", and an image sent with text is silently flattened before the model ever sees it. Both trace to the same root: the runner has no concept of "the current user turn", only "the most recent non-empty user text", and that assumption sits in four separate places. This PR is work package 2 of 4 of the Stage 1 plan (in-repo at
docs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.md), stacked on the approval-resume fix #5598, which it builds on in the same seams.Before: an image-only turn dies; an image-plus-text turn drops the image; an attachment reference has no consumer; a cold start rebuilds text only.
After:
currentUserTurn(request)is the one authority on what the person just sent, and all four former text-derived sites use it. An attachment-only turn is valid. Attachment references resolve through the WP1 API (bounded fetch, claim on acceptance), working copies materialize atcwd/attachments/<id>/<filename>under strict path safety, native blocks reach the harness when the three-layer gate allows, every current-turn attachment gets a mention line so the prompt is never empty, historical attachments restore at cold start (bounded, degradable), and every outcome emits a persistedattachment_deliveryevent with a stable reason code. Today's front end keeps working unchanged: the dual read matches the base64 data-URL shape it actually sends, so a pasted image now reaches the model natively where the harness supports it.The change was adversarially reviewed before this PR opened. The review probed real request shapes and found two release blockers plus one bad degradation, all inside the refactor's predicted blast radius, all fixed in the second and third commits:
The full findings-and-rulings trail is in
protocols/stage-1.md(committed on the WP1 branch below).Implicit decisions and their tradeoffs
modelCapabilitieswire field arrives in WP3; until then, only legacy inline images (which carry their own bytes) deliver natively.Forced routes to double-check
QA
1,375 runner tests pass; typecheck clean; the shared golden fixtures are byte-identical (the wire additions are all optional fields). Live QA ran on the dev stack against the deployed runner (full record in
protocols/stage-1.md):AGENTA 42, the model's reasoning describing the image. First time a composer-sent file reaches a model's perception.legacy inline image delivery=native reason=native_supported.https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx