fix(sdk): ship Pi's built-in tools in the default agent template (#5590) - #5597
Conversation
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR gives newly created Pi agents the ChangesDefault Pi built-in tool flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 |
![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/oss/src/agent/config.py (1)
111-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve default tools when
agent.jsonomits the key.
toolsis initialized withDEFAULT_TOOLS, but an existingagent.jsonwithout atoolsfield overwrites that value with[]. This leaves partial templates with no Pi tools, while explicit"tools": []should still retain the documented grant-nothing semantics. Add coverage for this case as well.Proposed fix
meta = json.loads(meta_path.read_text(encoding="utf-8")) model = meta.get("model") or DEFAULT_MODEL - tools = meta.get("tools", []) or [] + if "tools" in meta: + tools = meta["tools"] or []
🧹 Nitpick comments (1)
web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
anyfrom the new workspace-package tests.These casts can hide a broken agent-tools payload shape—the primary contract these tests are meant to protect.
web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts#L58-L60: use a minimalunknown-based workflow/config shape and narrow before readingparameters.agent.web/packages/agenta-playground/tests/unit/agentRequest.test.ts#L362-L372: type the request parameters/tools shape and use a narrow type guard for tool names.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 729092dc-5e31-4cba-af34-95f5b67f75c9
📒 Files selected for processing (36)
.agents/skills/agent-release-gate/resources/qa_probe.py.agents/skills/agent-release-gate/resources/qa_product.pyapi/oss/tests/pytest/unit/resources/test_workflow_catalog.pydocs/design/agent-workflows/documentation/agent-configuration.mddocs/design/agent-workflows/documentation/tools.mddocs/design/agent-workflows/interfaces/README.mddocs/design/agent-workflows/interfaces/in-service/harness-adapters.mddocs/design/agent-workflows/interfaces/public-edge/agent-config-schema.mddocs/design/agent-workflows/projects/default-agent-builtins/README.mddocs/design/agent-workflows/projects/default-agent-builtins/context.mddocs/design/agent-workflows/projects/default-agent-builtins/design.mddocs/design/agent-workflows/projects/default-agent-builtins/open-questions.mddocs/design/agent-workflows/projects/default-agent-builtins/plan.mddocs/design/agent-workflows/projects/default-agent-builtins/research.mddocs/design/agent-workflows/projects/default-agent-builtins/status.mddocs/design/agent-workflows/projects/default-agent-builtins/testing.mdsdks/python/agenta/sdk/agents/adapters/agenta_builtins.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/pi_builtins.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.jsonsdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_config_template_fallback.pyservices/oss/tests/pytest/unit/agent/test_default_agent_template.pyservices/runner/config/agent.jsonservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/tests/unit/pi-default-builtins-parity.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsweb/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
6144bf9 to
4c6fd34
Compare
Railway Preview Environment
Updated at 2026-08-01T18:14:30.335Z |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
web/packages/agenta-playground/tests/unit/agentRequest.test.ts (1)
362-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid introducing
anyin this workspace-package test.Use the existing request type or narrow
unknownvalues before accessingparameters.agent.tools; the newas anyand(tool: any)suppress schema regressions that this test should catch.As per coding guidelines, workspace packages should avoid
any.Source: Coding guidelines
web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test helper type-safe.
Record<string, any>andas anysuppress TypeScript checks, while optional chaining can still returnundefined. Define the small agent-config shape needed by these assertions and fail explicitly when the entity is missing.Proposed fix
-function readAgentConfig(localId: string): Record<string, any> { - const entity = getDefaultStore().get(workflowLocalServerDataAtomFamily(localId)) as any - return entity?.data?.parameters?.agent +type AgentConfig = { + harness?: {kind?: string} + llm?: {model?: string; provider?: string} + tools?: Array<{type: "builtin"; name: string}> +} + +function readAgentConfig(localId: string): AgentConfig { + const entity = getDefaultStore().get(workflowLocalServerDataAtomFamily(localId)) as + | {data?: {parameters?: {agent?: AgentConfig}}} + | undefined + const agent = entity?.data?.parameters?.agent + if (!agent) throw new Error(`Agent workflow ${localId} was not created`) + return agent }As per coding guidelines, workspace package TypeScript code should avoid
any.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a01f2878-bfdb-495e-a417-7f955d1cf5e8
📒 Files selected for processing (36)
.agents/skills/agent-release-gate/resources/qa_probe.py.agents/skills/agent-release-gate/resources/qa_product.pyapi/oss/tests/pytest/unit/resources/test_workflow_catalog.pydocs/design/agent-workflows/documentation/agent-configuration.mddocs/design/agent-workflows/documentation/tools.mddocs/design/agent-workflows/interfaces/README.mddocs/design/agent-workflows/interfaces/in-service/harness-adapters.mddocs/design/agent-workflows/interfaces/public-edge/agent-config-schema.mddocs/design/agent-workflows/projects/default-agent-builtins/README.mddocs/design/agent-workflows/projects/default-agent-builtins/context.mddocs/design/agent-workflows/projects/default-agent-builtins/design.mddocs/design/agent-workflows/projects/default-agent-builtins/open-questions.mddocs/design/agent-workflows/projects/default-agent-builtins/plan.mddocs/design/agent-workflows/projects/default-agent-builtins/research.mddocs/design/agent-workflows/projects/default-agent-builtins/status.mddocs/design/agent-workflows/projects/default-agent-builtins/testing.mdsdks/python/agenta/sdk/agents/adapters/agenta_builtins.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/pi_builtins.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.jsonsdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_config_template_fallback.pyservices/oss/tests/pytest/unit/agent/test_default_agent_template.pyservices/runner/config/agent.jsonservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/tests/unit/pi-default-builtins-parity.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsweb/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/design/agent-workflows/interfaces/README.md
- docs/design/agent-workflows/projects/default-agent-builtins/context.md
- docs/design/agent-workflows/documentation/agent-configuration.md
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx
- docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md
- sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py
4c6fd34 to
bb7728c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
docs/design/agent-workflows/projects/default-agent-builtins/research.md (1)
62-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit
allow_readsto granted tools.The default grant contains
read, notgrep,find, orls. State that the granted read-only built-in runs without asking. The current wording implies that all four tools are active.docs/design/agent-workflows/projects/default-agent-builtins/testing.md (1)
70-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the shipped constant name in the test example.
PI_DEFAULT_BUILTIN_NAMESdoes not exist. UsePI_DEFAULT_ACTIVE_BUILTINSso the example matches the implementation.
🟡 Minor comments (13)
services/runner/src/engines/sandbox_agent/run-plan.ts-321-337 (1)
321-337: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a terminal approval reply before accepting an empty request.
carriesApprovalReplyOnly(request)returnstruewhen any message contains an approvedtool_result. It does not confirm that the current continuation is that approval reply.For example, a historical approval followed by a later unresolved assistant
tool_callpasses this condition. The request then bypasses the empty-prompt rejection. Restrict the helper to the terminal approval-resume envelope, and add a regression test for this sequence.docs/design/agent-workflows/projects/default-agent-builtins/design.md-297-315 (1)
297-315: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the conflicting constant-location guidance.
Lines 297-301 place
PI_DEFAULT_ACTIVE_BUILTINSinagenta_builtins.py. Lines 308-315 correctly reject that location. Document only the implemented location,sdks/python/agenta/sdk/agents/pi_builtins.py.docs/design/agent-workflows/projects/default-agent-builtins/research.md-107-117 (1)
107-117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRefresh the design workspace to match the implemented state.
Several sections still describe the pre-change empty-tools implementation or pending work. This can mislead future implementers and make release evidence appear incomplete.
docs/design/agent-workflows/projects/default-agent-builtins/research.md#L107-L117: document the four-tool default and current consumers.docs/design/agent-workflows/projects/default-agent-builtins/design.md#L199-L204: remove the claim that the service fallback still usestools: [].docs/design/agent-workflows/projects/default-agent-builtins/design.md#L206-L210: update the build-an-agent skill example status.docs/design/agent-workflows/projects/default-agent-builtins/design.md#L332-L342: state that existing fixtures are unchanged and that the new parity fixture was added.docs/design/agent-workflows/projects/default-agent-builtins/plan.md#L12-L24: replace the source-reading test plan with the shared golden-fixture approach.docs/design/agent-workflows/projects/default-agent-builtins/research.md#L139-L149: update the independent default copies.docs/design/agent-workflows/projects/default-agent-builtins/research.md#L158-L171: update the Claude warning description.docs/design/agent-workflows/projects/default-agent-builtins/research.md#L211-L251: update the test inventory and crossing-test coverage.docs/design/agent-workflows/projects/default-agent-builtins/status.md#L3-L4: replace the uncommitted/no-PR state with the current PR state.docs/design/agent-workflows/projects/default-agent-builtins/status.md#L78-L79: reconcile the manual-verification status withtesting.md.docs/design/agent-workflows/projects/default-agent-builtins/testing.md#L136-L155: mark catalog and frontend-factory coverage as implemented.docs/design/agent-workflows/projects/default-agent-builtins/testing.md#L179-L185: mark the release-gate seed update as complete.docs/design/agent-workflows/projects/agent-multi-modality/plan.md-48-71 (1)
48-71: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMark Stage 0 as complete.
This section still says paste and drag-to-attach are ungated and lists their gating as optional work.
protocols/stage-0.mdandplans/stage-1-implementation.mdsay the guard landed in commitf2aa193cb2and was verified.Leaving this text unchanged can trigger duplicate implementation or an incorrect rollout decision. Update the Stage 0 section to describe the completed work and retain only any remaining documentation cleanup.
api/oss/src/core/sessions/attachments/service.py-200-202 (1)
200-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the actually missing attachment id.
reference_readyreturns None when any requested id is not ready. The service then reportsordered_ids[0], which is normally a valid attachment. Clients and logs receive a wrong id for multi-attachment references. Return the resolved ids from the DAO, or re-query to compute the missing set before raising.api/pyproject.toml-48-48 (1)
48-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid unintentionally permitting Python-incompatible
puremagicmajors.
puremagic>=1.30allowspuremagic>=2, but the2.xline requires Python >=3.12 while this project supports Python >=3.11. Add a cap that keeps the dependency constrained to compatible majors, such aspuremagic>=1.30,<2.api/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.py-29-35 (1)
29-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose an existing engine before resetting the module global.
The fixture sets
engine_module._transactions_engine = Noneat setup. If another module already created an engine, that engine loses its last reference withoutclose(), and its pooled connections stay open for the run. Close it if present.🛡️ Proposed change
async def _fresh_engine_per_test(): + if engine_module._transactions_engine is not None: + await engine_module._transactions_engine.close() engine_module._transactions_engine = None yieldapi/oss/src/apis/fastapi/sessions/router.py-1019-1039 (1)
1019-1039: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRun the permission check before request-size validation.
The route validates
Content-Lengthand raisesAttachmentLengthRequiredorAttachmentTooLargeat Lines 1019-1036, but it checksPermission.EDIT_SESSIONSonly at Line 1039. A caller without the session permission receives 411 or 413 and can inferlimits.max_raw_bytes. Move_validate_session_id_httpand_checkabove the size validation. The size validation still runs beforerequest.form(), so the bounded-read guarantee stays intact.🔒 Proposed reordering
) -> SessionAttachmentResponse: + _validate_session_id_http(session_id) + await self._check(request, Permission.EDIT_SESSIONS) + content_length = request.headers.get("content-length") if content_length is None: raise AttachmentLengthRequired() @@ if request_size > max_request_bytes: raise AttachmentTooLarge(size=request_size, limit=max_request_bytes) - _validate_session_id_http(session_id) - await self._check(request, Permission.EDIT_SESSIONS) - form = await request.form()api/oss/tests/pytest/unit/mounts/test_protected_mount_policy.py-541-567 (1)
541-567: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe test pins behavior that contradicts a service comment.
Lines 550 and 557 assert that
archive_session_mountsandunarchive_session_mountsexclude the protected mount. That is the actual behavior, because both methods read throughmounts_dao.query_mounts, which now applies the SQL protected-mount exclusion.The comment at
api/oss/src/core/mounts/service.pylines 748-749 states that session lifecycle bypasses the filter "to retain protected mounts for teardown and archival". Only teardown bypasses it.delete_session_mountsusesdelete_by_session_idand is unfiltered, which line 563 confirms. Archival is filtered. Correct the comment to say teardown only, so the next reader does not assume archived sessions also archive their attachment mount.api/oss/src/core/mounts/service.py-596-647 (1)
596-647: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the
mounts_storeNone guard to the new attachment operations.
self.mounts_storeis optional (line 409)._bucket()only validatesself.bucket. If a deployment configures a bucket without a store, these three methods raiseAttributeErroronself.mounts_store.put_object.handle_mount_exceptionsdoes not mapAttributeError, so the caller receives 500 instead of the 503 thatMountStorageUnavailableproduces.sign_mount_credentialsalready performs this check at line 860.🛡️ Proposed guard
) -> None: validate_file_path(path) + if self.mounts_store is None: + raise MountStorageUnavailable() mount = await self._resolve_mount( project_id=project_id, mount_id=mount_id, allow_protected=True, )services/runner/src/engines/sandbox_agent/attachments.ts-412-449 (1)
412-449: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign model modality tokens with the catalog field and gate missing capability flags.
The catalog schema and
ModelCatalogEntry.modalitiesuse"image"and"text", whilemodelModalityStatealso treats"audio","document", and"documents"as supported. Add explicit validation/mapping for those catalog tokens, and treat malformedmodelCapabilities.modelCapabilities.inputModalitiesas a clear mismatch instead of letting the gate degrade attachments toworkspace_only.Source: Linters/SAST tools
services/runner/src/sessions/attachments.ts-109-115 (1)
109-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrip Content-Type parameters from
mediaType.
content-typecan carry parameters, for exampleimage/png; charset=binary. The raw header value becomes the authoritativemediaTypefor the fetched attachment. Any downstream comparison against a MIME allowlist (native image delivery, capability gating) then fails for a value that is in fact supported. Parse the essence before the first;and lowercase it.🛠️ Proposed fix
- const mediaType = response.headers.get("content-type")?.trim(); + const mediaType = response.headers + .get("content-type") + ?.split(";")[0] + .trim() + .toLowerCase();services/runner/src/engines/sandbox_agent/run-turn.ts-305-322 (1)
305-322: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRespect resolved model image support for legacy inline images.
attachmentCapabilityGatetreats missing or mismatchedinputModalitiesasworkspace_only, but this loop always gates legacy inline images with{ inputModalities: ["image"] }. If the run model only advertises text, inline images still arrive as native image blocks while an identical image sent with an attachment fails gating; userequest.modelCapabilitieshere if resolved image support must apply to legacy inline images.
🧹 Nitpick comments (14)
api/oss/src/dbs/postgres/sessions/attachments/dao.py (1)
241-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeduplicate ids inside
reference_ready.The readiness check compares row count with
len(attachment_ids). A duplicate id in the input makes the counts differ and returns None, which callers translate into a not-found error.SessionAttachmentsService.reference_attachmentscurrently deduplicates first, so behavior is correct today. Deduplicate here as well so the DAO contract does not depend on caller preprocessing.♻️ Proposed hardening
) -> Optional[List[Attachment]]: if not attachment_ids: return [] + unique_ids = list(dict.fromkeys(attachment_ids)) async with self.engine.session() as session: result = await session.execute( select(SessionAttachmentDBE) .where( SessionAttachmentDBE.project_id == project_id, SessionAttachmentDBE.session_id == session_id, - SessionAttachmentDBE.id.in_(attachment_ids), + SessionAttachmentDBE.id.in_(unique_ids), SessionAttachmentDBE.state == AttachmentState.READY.value, ) .with_for_update() ) attachment_dbes = list(result.scalars().all()) - if len(attachment_dbes) != len(attachment_ids): + if len(attachment_dbes) != len(unique_ids): return Noneapi/oss/src/core/sessions/attachments/interfaces.py (1)
119-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving original deletion out of the DAO contract.
reap_stale_pendingandsweep_unreferenced_readyaccept adelete_originalcallback. The DAO therefore performs object-store side effects while it deletes rows. An alternative is to return the stale rows from the DAO and let the service or sweep task delete the originals. That keeps the DAO restricted to persistence and makes the deletion order explicit and testable.If you keep the callback, document the ordering guarantee (object delete before or after row delete) in the interface docstring, because it determines whether a failure leaves an orphaned object or an orphaned row.
api/oss/src/tasks/asyncio/sessions/attachment_sweep.py (1)
135-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFloor the sleep interval to avoid a hot loop on misconfiguration.
sweep_interval_secondscomes from configuration. If an operator sets it to0, this loop repeatedly acquires the Redis lock and scans the database with no pause.lease_ttl_secondsalready applies a floor at line 70. Apply an equivalent floor to the sleep.♻️ Proposed floor for the sweep interval
) -> None: + interval_seconds = max(sweep_interval_seconds, 1) while True: try: await run_attachment_sweep( @@ ) except asyncio.CancelledError: raise except Exception as error: log.error( "attachment_sweep: error during sweep pass: %s", error, exc_info=True, ) - await asyncio.sleep(sweep_interval_seconds) + await asyncio.sleep(interval_seconds)api/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.py (1)
208-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the
reference_readyawait.
reference_readyruns while the sweep task waits onallow_object_delete. If a future change keeps the tombstone transaction open acrossdelete_original, this call blocks on the row lock and the test hangs, because the event is set only after the call returns. Wrap the call inasyncio.wait_for, as the quota test at Lines 248-263 already does.♻️ Proposed change
- claimed = await dao.reference_ready( - project_id=attachment_scope["project_id"], - session_id=session_id, - attachment_ids=[ready.id], - referenced_at=datetime.now(timezone.utc), - ) + claimed = await asyncio.wait_for( + dao.reference_ready( + project_id=attachment_scope["project_id"], + session_id=session_id, + attachment_ids=[ready.id], + referenced_at=datetime.now(timezone.utc), + ), + timeout=30, + )api/oss/tests/pytest/unit/sessions/test_attachment_state_machine.py (1)
549-562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the bounded read with an understated
Content-Length.The current tests cover the header-based rejection only.
_read_boundedis the defense when a client sends a smallContent-Lengthand a larger body. Add a case that sendscontent_lengthbelow the real body size and asserts a 413 response, so the byte-level bound stays covered.api/oss/tests/pytest/unit/sessions/test_attachment_sweep.py (1)
141-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the existing pytest-asyncio test style for new session unit tests.
api/pytest.inisetsasyncio_mode = auto, andpytest-asynciois the configured dependency;pytest-anyiois not listed. Preferasync deftests without@pytest.mark.anyio/anyio_backendunless anyio-specific backend support is required.api/oss/src/apis/fastapi/sessions/models.py (1)
193-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute
countinstead of relying on manual assignment at each call site.
countdefaults to0in bothSessionAttachmentResponseandSessionAttachmentsResponse. Nothing in these models enforces thatcountmatches the actual payload. If a future call site forgets to setcountexplicitly, the response silently reports0whileattachment/attachmentsstill carries data.♻️ Proposed fix using computed values
+from pydantic import model_validator + class SessionAttachmentResponse(BaseModel): - count: int = 0 + count: Literal[1] = 1 attachment: SessionAttachment class SessionAttachmentsResponse(BaseModel): - count: int = 0 attachments: List[SessionAttachment] = Field(default_factory=list) + + `@property` + def count(self) -> int: + return len(self.attachments)hosting/docker-compose/oss/nginx/nginx.conf (1)
35-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider scoping
client_max_body_sizeto the/api/location.This directive is set at server scope, so it also raises the body-size ceiling for
/services/completion/and/services/chat/, which have no relationship to attachments. Move the32mlimit into thelocation /api/block to keep the increase scoped to the feature that needs it.♻️ Proposed fix to scope the limit
- # Client max body size - client_max_body_size 32m; - # Proxy to FastAPI backend location /api/ { limit_req zone=api burst=20 nodelay; + client_max_body_size 32m; proxy_pass http://api:8000/;api/oss/src/core/mounts/service.py (1)
482-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider constraining
purposeto the known constant.
purposeis an openOptional[str]here. Any value disables the reserved-name guard on line 495. Today onlyget_or_create_attachment_mountpasses it, so this is not reachable from clients. ALiteral[ATTACHMENTS_MOUNT_PURPOSE]type (or an enum) would make the invariant explicit and prevent a future caller from minting an unclassifiedattachmentsmount.api/oss/src/dbs/postgres/mounts/mappings.py (1)
42-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueOptional:
purposeis not backfilled on re-bind.This mapping only supplies
purposein theinsert(...).values()part. Theon_conflict_do_updateset_inapi/oss/src/dbs/postgres/mounts/dao.py(lines 85-93) does not includepurpose. A pre-migration attachments row therefore keepspurpose = NULLacross every re-bind, so it depends on the transitional slug fallback permanently.Classification is still correct, because
is_protected_mountand the DAOLIKEpredicate both cover the legacy slug. If you want the durable column to converge, add"purpose"to the conflictset_, or backfill it in the migration.api/oss/tests/pytest/unit/mounts/test_protected_mount_policy.py (1)
84-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe double duplicates the SQL filter, so the service filter is untested.
Line 110 applies
is_protected_mountinside the DAO double. This mirrors the real SQL predicate, which is good fortest_fetch_and_query_hide_protected_mounts. It also meanstest_fetch_and_query_hide_protected_mountswould still pass if the Python filter atapi/oss/src/core/mounts/service.pyline 749 were deleted.The comment in the service calls that filter the defense against DAO drift, so consider one extra test that returns a protected mount from the double unfiltered and asserts
query_mountsstill hides it.services/runner/src/engines/sandbox_agent/attachments.ts (2)
455-519: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
requireNativedoes not gate the adapter and model branches.
requireNativeis read only inside the transport branch. If the transport advertises the kind but the adapter or the model modality blocks native delivery, the gate returnsoutcome: "workspace_only"even when the caller demanded native delivery. A future caller that setsrequireNative: truewould therefore degrade silently, which is the exact outcome the flag exists to prevent.Related:
"model_modality_unsupported"is declared at Line 37 but no branch returns it. Either produce it or remove it.No production caller passes
requireNativetoday, so this is a contract gap rather than a live defect. Consider adding gate tests for the adapter and model branches withrequireNative: trueinservices/runner/tests/unit/attachment-capability-gate.test.ts.♻️ Proposed fix to honor `requireNative` on every branch
export function attachmentCapabilityGate(input: { acpAgent: string; provider?: string; capabilities: HarnessCapabilities; modelCapabilities?: AgentRunRequest["modelCapabilities"]; mediaType: string; byteLength: number; requireNative?: boolean; }): AttachmentGate { const kind = attachmentKind(input.mediaType); + const degrade = ( + reasonCode: AttachmentDeliveryReasonCode, + missing: string, + ): AttachmentGate => + input.requireNative + ? { outcome: "failed", reasonCode: "contract_violation", kind, missing } + : { outcome: "workspace_only", reasonCode, kind }; + if (!transportSupports(kind, input.capabilities)) { - if (input.requireNative) { - return { - outcome: "failed", - reasonCode: "contract_violation", - kind, - missing: "transport capability", - }; - } - return { - outcome: "workspace_only", - reasonCode: "transport_unsupported", - kind, - }; + return degrade("transport_unsupported", "transport capability"); } if (!adapterSupports(input.acpAgent, kind)) { - return { - outcome: "workspace_only", - reasonCode: "adapter_unsupported", - kind, - }; + return degrade("adapter_unsupported", "adapter capability"); } const modelState = modelModalityState( input.modelCapabilities?.inputModalities, kind, ); if (modelState === "unknown") { - return { - outcome: "workspace_only", - reasonCode: "model_modality_unknown", - kind, - }; + return degrade("model_modality_unknown", "declared model modality"); }
501-516: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a provider-independent inline ceiling.
The base64 ceiling applies only when the provider is Anthropic. For every other provider an image of any size is inlined into the prompt, so the runner base64-encodes the whole buffer and holds both copies in memory. A large upload then fails at the provider instead of degrading to
workspace_only. Add an absolute maximum that applies to all providers, and keep the Anthropic value as the stricter case.services/runner/tests/unit/session-pool.test.ts (1)
331-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert order sensitivity too.
The test name states "ordered user attachment ids", but the assertion only compares two different ids. Add a case with the same two ids in swapped order across two user messages. That case proves
userAttachmentIdskeeps positional identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 423b23a0-d9c4-43c0-b015-1e3ba85c5e27
⛔ Files ignored due to path filters (1)
api/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (128)
.agents/skills/agent-release-gate/resources/qa_probe.py.agents/skills/agent-release-gate/resources/qa_product.pyapi/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000020_add_session_attachments.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/sessions/attachments/__init__.pyapi/oss/src/core/sessions/attachments/dtos.pyapi/oss/src/core/sessions/attachments/interfaces.pyapi/oss/src/core/sessions/attachments/media.pyapi/oss/src/core/sessions/attachments/service.pyapi/oss/src/core/sessions/attachments/types.pyapi/oss/src/core/sessions/interactions/dtos.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/src/dbs/postgres/mounts/dbas.pyapi/oss/src/dbs/postgres/mounts/mappings.pyapi/oss/src/dbs/postgres/sessions/attachments/__init__.pyapi/oss/src/dbs/postgres/sessions/attachments/dao.pyapi/oss/src/dbs/postgres/sessions/attachments/dbas.pyapi/oss/src/dbs/postgres/sessions/attachments/dbes.pyapi/oss/src/dbs/postgres/sessions/attachments/mappings.pyapi/oss/src/tasks/asyncio/sessions/attachment_sweep.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/mounts/test_attachments_mount_hidden.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachment_teardown.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachments.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/resources/test_workflow_catalog.pyapi/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.pyapi/oss/tests/pytest/unit/sessions/test_attachment_filename.pyapi/oss/tests/pytest/unit/sessions/test_attachment_media.pyapi/oss/tests/pytest/unit/sessions/test_attachment_quotas.pyapi/oss/tests/pytest/unit/sessions/test_attachment_state_machine.pyapi/oss/tests/pytest/unit/sessions/test_attachment_sweep.pyapi/oss/tests/pytest/unit/sessions/test_interaction_request_tool_call_id.pyapi/oss/tests/pytest/unit/sessions/test_records_truncation.pyapi/pyproject.tomldocs/design/agent-workflows/documentation/agent-configuration.mddocs/design/agent-workflows/documentation/tools.mddocs/design/agent-workflows/interfaces/README.mddocs/design/agent-workflows/interfaces/in-service/harness-adapters.mddocs/design/agent-workflows/interfaces/public-edge/agent-config-schema.mddocs/design/agent-workflows/projects/agent-multi-modality/README.mddocs/design/agent-workflows/projects/agent-multi-modality/context.mddocs/design/agent-workflows/projects/agent-multi-modality/decisions.mddocs/design/agent-workflows/projects/agent-multi-modality/design.mddocs/design/agent-workflows/projects/agent-multi-modality/plan.mddocs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.mddocs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-0.mddocs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mddocs/design/agent-workflows/projects/agent-multi-modality/research.mddocs/design/agent-workflows/projects/agent-multi-modality/scope.mddocs/design/agent-workflows/projects/agent-multi-modality/status.mddocs/design/agent-workflows/projects/default-agent-builtins/README.mddocs/design/agent-workflows/projects/default-agent-builtins/context.mddocs/design/agent-workflows/projects/default-agent-builtins/design.mddocs/design/agent-workflows/projects/default-agent-builtins/open-questions.mddocs/design/agent-workflows/projects/default-agent-builtins/plan.mddocs/design/agent-workflows/projects/default-agent-builtins/research.mddocs/design/agent-workflows/projects/default-agent-builtins/status.mddocs/design/agent-workflows/projects/default-agent-builtins/testing.mdhosting/docker-compose/oss/nginx/nginx.confsdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/adapters/agenta_builtins.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/errors.pysdks/python/agenta/sdk/agents/handler.pysdks/python/agenta/sdk/agents/pi_builtins.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.jsonsdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/oss/src/agent/app.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_config_template_fallback.pyservices/oss/tests/pytest/unit/agent/test_default_agent_template.pyservices/oss/tests/pytest/unit/agent/test_select_backend.pyservices/runner/config/agent.jsonservices/runner/src/engines/sandbox_agent/acp-interactions.tsservices/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/interactions.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/pi-default-builtins-parity.test.tsservices/runner/tests/unit/reconstruct-resume-nonfatal.test.tsservices/runner/tests/unit/sandbox-agent-acp-interactions.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-keepalive-approval.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.tsweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- docs/design/agent-workflows/interfaces/README.md
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx
- services/runner/tests/unit/pi-default-builtins-parity.test.ts
- .agents/skills/agent-release-gate/resources/qa_product.py
- sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx
- sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py
- services/oss/tests/pytest/unit/agent/test_config_template_fallback.py
- services/runner/config/agent.json
- .agents/skills/agent-release-gate/resources/qa_probe.py
- web/packages/agenta-playground/tests/unit/agentRequest.test.ts
- sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py
- api/oss/tests/pytest/unit/resources/test_workflow_catalog.py
- docs/design/agent-workflows/projects/default-agent-builtins/context.md
- web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts
- sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py
- sdks/python/agenta/sdk/agents/pi_builtins.py
- docs/design/agent-workflows/documentation/agent-configuration.md
- sdks/python/agenta/sdk/utils/types.py
- sdks/python/agenta/sdk/agents/adapters/harnesses.py
- services/oss/src/agent/config.py
- web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts
- docs/design/agent-workflows/projects/default-agent-builtins/README.md
- sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py
- services/oss/tests/pytest/unit/agent/test_default_agent_template.py
- docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
docs/design/agent-workflows/projects/default-agent-builtins/research.md (1)
62-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit
allow_readsto granted tools.The default grant contains
read, notgrep,find, orls. State that the granted read-only built-in runs without asking. The current wording implies that all four tools are active.docs/design/agent-workflows/projects/default-agent-builtins/testing.md (1)
70-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the shipped constant name in the test example.
PI_DEFAULT_BUILTIN_NAMESdoes not exist. UsePI_DEFAULT_ACTIVE_BUILTINSso the example matches the implementation.
🟡 Minor comments (13)
services/runner/src/engines/sandbox_agent/run-plan.ts-321-337 (1)
321-337: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a terminal approval reply before accepting an empty request.
carriesApprovalReplyOnly(request)returnstruewhen any message contains an approvedtool_result. It does not confirm that the current continuation is that approval reply.For example, a historical approval followed by a later unresolved assistant
tool_callpasses this condition. The request then bypasses the empty-prompt rejection. Restrict the helper to the terminal approval-resume envelope, and add a regression test for this sequence.docs/design/agent-workflows/projects/default-agent-builtins/design.md-297-315 (1)
297-315: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the conflicting constant-location guidance.
Lines 297-301 place
PI_DEFAULT_ACTIVE_BUILTINSinagenta_builtins.py. Lines 308-315 correctly reject that location. Document only the implemented location,sdks/python/agenta/sdk/agents/pi_builtins.py.docs/design/agent-workflows/projects/default-agent-builtins/research.md-107-117 (1)
107-117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRefresh the design workspace to match the implemented state.
Several sections still describe the pre-change empty-tools implementation or pending work. This can mislead future implementers and make release evidence appear incomplete.
docs/design/agent-workflows/projects/default-agent-builtins/research.md#L107-L117: document the four-tool default and current consumers.docs/design/agent-workflows/projects/default-agent-builtins/design.md#L199-L204: remove the claim that the service fallback still usestools: [].docs/design/agent-workflows/projects/default-agent-builtins/design.md#L206-L210: update the build-an-agent skill example status.docs/design/agent-workflows/projects/default-agent-builtins/design.md#L332-L342: state that existing fixtures are unchanged and that the new parity fixture was added.docs/design/agent-workflows/projects/default-agent-builtins/plan.md#L12-L24: replace the source-reading test plan with the shared golden-fixture approach.docs/design/agent-workflows/projects/default-agent-builtins/research.md#L139-L149: update the independent default copies.docs/design/agent-workflows/projects/default-agent-builtins/research.md#L158-L171: update the Claude warning description.docs/design/agent-workflows/projects/default-agent-builtins/research.md#L211-L251: update the test inventory and crossing-test coverage.docs/design/agent-workflows/projects/default-agent-builtins/status.md#L3-L4: replace the uncommitted/no-PR state with the current PR state.docs/design/agent-workflows/projects/default-agent-builtins/status.md#L78-L79: reconcile the manual-verification status withtesting.md.docs/design/agent-workflows/projects/default-agent-builtins/testing.md#L136-L155: mark catalog and frontend-factory coverage as implemented.docs/design/agent-workflows/projects/default-agent-builtins/testing.md#L179-L185: mark the release-gate seed update as complete.docs/design/agent-workflows/projects/agent-multi-modality/plan.md-48-71 (1)
48-71: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMark Stage 0 as complete.
This section still says paste and drag-to-attach are ungated and lists their gating as optional work.
protocols/stage-0.mdandplans/stage-1-implementation.mdsay the guard landed in commitf2aa193cb2and was verified.Leaving this text unchanged can trigger duplicate implementation or an incorrect rollout decision. Update the Stage 0 section to describe the completed work and retain only any remaining documentation cleanup.
api/oss/src/core/sessions/attachments/service.py-200-202 (1)
200-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport the actually missing attachment id.
reference_readyreturns None when any requested id is not ready. The service then reportsordered_ids[0], which is normally a valid attachment. Clients and logs receive a wrong id for multi-attachment references. Return the resolved ids from the DAO, or re-query to compute the missing set before raising.api/pyproject.toml-48-48 (1)
48-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid unintentionally permitting Python-incompatible
puremagicmajors.
puremagic>=1.30allowspuremagic>=2, but the2.xline requires Python >=3.12 while this project supports Python >=3.11. Add a cap that keeps the dependency constrained to compatible majors, such aspuremagic>=1.30,<2.api/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.py-29-35 (1)
29-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose an existing engine before resetting the module global.
The fixture sets
engine_module._transactions_engine = Noneat setup. If another module already created an engine, that engine loses its last reference withoutclose(), and its pooled connections stay open for the run. Close it if present.🛡️ Proposed change
async def _fresh_engine_per_test(): + if engine_module._transactions_engine is not None: + await engine_module._transactions_engine.close() engine_module._transactions_engine = None yieldapi/oss/src/apis/fastapi/sessions/router.py-1019-1039 (1)
1019-1039: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRun the permission check before request-size validation.
The route validates
Content-Lengthand raisesAttachmentLengthRequiredorAttachmentTooLargeat Lines 1019-1036, but it checksPermission.EDIT_SESSIONSonly at Line 1039. A caller without the session permission receives 411 or 413 and can inferlimits.max_raw_bytes. Move_validate_session_id_httpand_checkabove the size validation. The size validation still runs beforerequest.form(), so the bounded-read guarantee stays intact.🔒 Proposed reordering
) -> SessionAttachmentResponse: + _validate_session_id_http(session_id) + await self._check(request, Permission.EDIT_SESSIONS) + content_length = request.headers.get("content-length") if content_length is None: raise AttachmentLengthRequired() @@ if request_size > max_request_bytes: raise AttachmentTooLarge(size=request_size, limit=max_request_bytes) - _validate_session_id_http(session_id) - await self._check(request, Permission.EDIT_SESSIONS) - form = await request.form()api/oss/tests/pytest/unit/mounts/test_protected_mount_policy.py-541-567 (1)
541-567: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe test pins behavior that contradicts a service comment.
Lines 550 and 557 assert that
archive_session_mountsandunarchive_session_mountsexclude the protected mount. That is the actual behavior, because both methods read throughmounts_dao.query_mounts, which now applies the SQL protected-mount exclusion.The comment at
api/oss/src/core/mounts/service.pylines 748-749 states that session lifecycle bypasses the filter "to retain protected mounts for teardown and archival". Only teardown bypasses it.delete_session_mountsusesdelete_by_session_idand is unfiltered, which line 563 confirms. Archival is filtered. Correct the comment to say teardown only, so the next reader does not assume archived sessions also archive their attachment mount.api/oss/src/core/mounts/service.py-596-647 (1)
596-647: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the
mounts_storeNone guard to the new attachment operations.
self.mounts_storeis optional (line 409)._bucket()only validatesself.bucket. If a deployment configures a bucket without a store, these three methods raiseAttributeErroronself.mounts_store.put_object.handle_mount_exceptionsdoes not mapAttributeError, so the caller receives 500 instead of the 503 thatMountStorageUnavailableproduces.sign_mount_credentialsalready performs this check at line 860.🛡️ Proposed guard
) -> None: validate_file_path(path) + if self.mounts_store is None: + raise MountStorageUnavailable() mount = await self._resolve_mount( project_id=project_id, mount_id=mount_id, allow_protected=True, )services/runner/src/engines/sandbox_agent/attachments.ts-412-449 (1)
412-449: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign model modality tokens with the catalog field and gate missing capability flags.
The catalog schema and
ModelCatalogEntry.modalitiesuse"image"and"text", whilemodelModalityStatealso treats"audio","document", and"documents"as supported. Add explicit validation/mapping for those catalog tokens, and treat malformedmodelCapabilities.modelCapabilities.inputModalitiesas a clear mismatch instead of letting the gate degrade attachments toworkspace_only.Source: Linters/SAST tools
services/runner/src/sessions/attachments.ts-109-115 (1)
109-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrip Content-Type parameters from
mediaType.
content-typecan carry parameters, for exampleimage/png; charset=binary. The raw header value becomes the authoritativemediaTypefor the fetched attachment. Any downstream comparison against a MIME allowlist (native image delivery, capability gating) then fails for a value that is in fact supported. Parse the essence before the first;and lowercase it.🛠️ Proposed fix
- const mediaType = response.headers.get("content-type")?.trim(); + const mediaType = response.headers + .get("content-type") + ?.split(";")[0] + .trim() + .toLowerCase();services/runner/src/engines/sandbox_agent/run-turn.ts-305-322 (1)
305-322: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRespect resolved model image support for legacy inline images.
attachmentCapabilityGatetreats missing or mismatchedinputModalitiesasworkspace_only, but this loop always gates legacy inline images with{ inputModalities: ["image"] }. If the run model only advertises text, inline images still arrive as native image blocks while an identical image sent with an attachment fails gating; userequest.modelCapabilitieshere if resolved image support must apply to legacy inline images.
🧹 Nitpick comments (14)
api/oss/src/dbs/postgres/sessions/attachments/dao.py (1)
241-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeduplicate ids inside
reference_ready.The readiness check compares row count with
len(attachment_ids). A duplicate id in the input makes the counts differ and returns None, which callers translate into a not-found error.SessionAttachmentsService.reference_attachmentscurrently deduplicates first, so behavior is correct today. Deduplicate here as well so the DAO contract does not depend on caller preprocessing.♻️ Proposed hardening
) -> Optional[List[Attachment]]: if not attachment_ids: return [] + unique_ids = list(dict.fromkeys(attachment_ids)) async with self.engine.session() as session: result = await session.execute( select(SessionAttachmentDBE) .where( SessionAttachmentDBE.project_id == project_id, SessionAttachmentDBE.session_id == session_id, - SessionAttachmentDBE.id.in_(attachment_ids), + SessionAttachmentDBE.id.in_(unique_ids), SessionAttachmentDBE.state == AttachmentState.READY.value, ) .with_for_update() ) attachment_dbes = list(result.scalars().all()) - if len(attachment_dbes) != len(attachment_ids): + if len(attachment_dbes) != len(unique_ids): return Noneapi/oss/src/core/sessions/attachments/interfaces.py (1)
119-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving original deletion out of the DAO contract.
reap_stale_pendingandsweep_unreferenced_readyaccept adelete_originalcallback. The DAO therefore performs object-store side effects while it deletes rows. An alternative is to return the stale rows from the DAO and let the service or sweep task delete the originals. That keeps the DAO restricted to persistence and makes the deletion order explicit and testable.If you keep the callback, document the ordering guarantee (object delete before or after row delete) in the interface docstring, because it determines whether a failure leaves an orphaned object or an orphaned row.
api/oss/src/tasks/asyncio/sessions/attachment_sweep.py (1)
135-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFloor the sleep interval to avoid a hot loop on misconfiguration.
sweep_interval_secondscomes from configuration. If an operator sets it to0, this loop repeatedly acquires the Redis lock and scans the database with no pause.lease_ttl_secondsalready applies a floor at line 70. Apply an equivalent floor to the sleep.♻️ Proposed floor for the sweep interval
) -> None: + interval_seconds = max(sweep_interval_seconds, 1) while True: try: await run_attachment_sweep( @@ ) except asyncio.CancelledError: raise except Exception as error: log.error( "attachment_sweep: error during sweep pass: %s", error, exc_info=True, ) - await asyncio.sleep(sweep_interval_seconds) + await asyncio.sleep(interval_seconds)api/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.py (1)
208-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the
reference_readyawait.
reference_readyruns while the sweep task waits onallow_object_delete. If a future change keeps the tombstone transaction open acrossdelete_original, this call blocks on the row lock and the test hangs, because the event is set only after the call returns. Wrap the call inasyncio.wait_for, as the quota test at Lines 248-263 already does.♻️ Proposed change
- claimed = await dao.reference_ready( - project_id=attachment_scope["project_id"], - session_id=session_id, - attachment_ids=[ready.id], - referenced_at=datetime.now(timezone.utc), - ) + claimed = await asyncio.wait_for( + dao.reference_ready( + project_id=attachment_scope["project_id"], + session_id=session_id, + attachment_ids=[ready.id], + referenced_at=datetime.now(timezone.utc), + ), + timeout=30, + )api/oss/tests/pytest/unit/sessions/test_attachment_state_machine.py (1)
549-562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the bounded read with an understated
Content-Length.The current tests cover the header-based rejection only.
_read_boundedis the defense when a client sends a smallContent-Lengthand a larger body. Add a case that sendscontent_lengthbelow the real body size and asserts a 413 response, so the byte-level bound stays covered.api/oss/tests/pytest/unit/sessions/test_attachment_sweep.py (1)
141-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the existing pytest-asyncio test style for new session unit tests.
api/pytest.inisetsasyncio_mode = auto, andpytest-asynciois the configured dependency;pytest-anyiois not listed. Preferasync deftests without@pytest.mark.anyio/anyio_backendunless anyio-specific backend support is required.api/oss/src/apis/fastapi/sessions/models.py (1)
193-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute
countinstead of relying on manual assignment at each call site.
countdefaults to0in bothSessionAttachmentResponseandSessionAttachmentsResponse. Nothing in these models enforces thatcountmatches the actual payload. If a future call site forgets to setcountexplicitly, the response silently reports0whileattachment/attachmentsstill carries data.♻️ Proposed fix using computed values
+from pydantic import model_validator + class SessionAttachmentResponse(BaseModel): - count: int = 0 + count: Literal[1] = 1 attachment: SessionAttachment class SessionAttachmentsResponse(BaseModel): - count: int = 0 attachments: List[SessionAttachment] = Field(default_factory=list) + + `@property` + def count(self) -> int: + return len(self.attachments)hosting/docker-compose/oss/nginx/nginx.conf (1)
35-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider scoping
client_max_body_sizeto the/api/location.This directive is set at server scope, so it also raises the body-size ceiling for
/services/completion/and/services/chat/, which have no relationship to attachments. Move the32mlimit into thelocation /api/block to keep the increase scoped to the feature that needs it.♻️ Proposed fix to scope the limit
- # Client max body size - client_max_body_size 32m; - # Proxy to FastAPI backend location /api/ { limit_req zone=api burst=20 nodelay; + client_max_body_size 32m; proxy_pass http://api:8000/;api/oss/src/core/mounts/service.py (1)
482-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider constraining
purposeto the known constant.
purposeis an openOptional[str]here. Any value disables the reserved-name guard on line 495. Today onlyget_or_create_attachment_mountpasses it, so this is not reachable from clients. ALiteral[ATTACHMENTS_MOUNT_PURPOSE]type (or an enum) would make the invariant explicit and prevent a future caller from minting an unclassifiedattachmentsmount.api/oss/src/dbs/postgres/mounts/mappings.py (1)
42-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueOptional:
purposeis not backfilled on re-bind.This mapping only supplies
purposein theinsert(...).values()part. Theon_conflict_do_updateset_inapi/oss/src/dbs/postgres/mounts/dao.py(lines 85-93) does not includepurpose. A pre-migration attachments row therefore keepspurpose = NULLacross every re-bind, so it depends on the transitional slug fallback permanently.Classification is still correct, because
is_protected_mountand the DAOLIKEpredicate both cover the legacy slug. If you want the durable column to converge, add"purpose"to the conflictset_, or backfill it in the migration.api/oss/tests/pytest/unit/mounts/test_protected_mount_policy.py (1)
84-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe double duplicates the SQL filter, so the service filter is untested.
Line 110 applies
is_protected_mountinside the DAO double. This mirrors the real SQL predicate, which is good fortest_fetch_and_query_hide_protected_mounts. It also meanstest_fetch_and_query_hide_protected_mountswould still pass if the Python filter atapi/oss/src/core/mounts/service.pyline 749 were deleted.The comment in the service calls that filter the defense against DAO drift, so consider one extra test that returns a protected mount from the double unfiltered and asserts
query_mountsstill hides it.services/runner/src/engines/sandbox_agent/attachments.ts (2)
455-519: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
requireNativedoes not gate the adapter and model branches.
requireNativeis read only inside the transport branch. If the transport advertises the kind but the adapter or the model modality blocks native delivery, the gate returnsoutcome: "workspace_only"even when the caller demanded native delivery. A future caller that setsrequireNative: truewould therefore degrade silently, which is the exact outcome the flag exists to prevent.Related:
"model_modality_unsupported"is declared at Line 37 but no branch returns it. Either produce it or remove it.No production caller passes
requireNativetoday, so this is a contract gap rather than a live defect. Consider adding gate tests for the adapter and model branches withrequireNative: trueinservices/runner/tests/unit/attachment-capability-gate.test.ts.♻️ Proposed fix to honor `requireNative` on every branch
export function attachmentCapabilityGate(input: { acpAgent: string; provider?: string; capabilities: HarnessCapabilities; modelCapabilities?: AgentRunRequest["modelCapabilities"]; mediaType: string; byteLength: number; requireNative?: boolean; }): AttachmentGate { const kind = attachmentKind(input.mediaType); + const degrade = ( + reasonCode: AttachmentDeliveryReasonCode, + missing: string, + ): AttachmentGate => + input.requireNative + ? { outcome: "failed", reasonCode: "contract_violation", kind, missing } + : { outcome: "workspace_only", reasonCode, kind }; + if (!transportSupports(kind, input.capabilities)) { - if (input.requireNative) { - return { - outcome: "failed", - reasonCode: "contract_violation", - kind, - missing: "transport capability", - }; - } - return { - outcome: "workspace_only", - reasonCode: "transport_unsupported", - kind, - }; + return degrade("transport_unsupported", "transport capability"); } if (!adapterSupports(input.acpAgent, kind)) { - return { - outcome: "workspace_only", - reasonCode: "adapter_unsupported", - kind, - }; + return degrade("adapter_unsupported", "adapter capability"); } const modelState = modelModalityState( input.modelCapabilities?.inputModalities, kind, ); if (modelState === "unknown") { - return { - outcome: "workspace_only", - reasonCode: "model_modality_unknown", - kind, - }; + return degrade("model_modality_unknown", "declared model modality"); }
501-516: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a provider-independent inline ceiling.
The base64 ceiling applies only when the provider is Anthropic. For every other provider an image of any size is inlined into the prompt, so the runner base64-encodes the whole buffer and holds both copies in memory. A large upload then fails at the provider instead of degrading to
workspace_only. Add an absolute maximum that applies to all providers, and keep the Anthropic value as the stricter case.services/runner/tests/unit/session-pool.test.ts (1)
331-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert order sensitivity too.
The test name states "ordered user attachment ids", but the assertion only compares two different ids. Add a case with the same two ids in swapped order across two user messages. That case proves
userAttachmentIdskeeps positional identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 423b23a0-d9c4-43c0-b015-1e3ba85c5e27
⛔ Files ignored due to path filters (1)
api/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (128)
.agents/skills/agent-release-gate/resources/qa_probe.py.agents/skills/agent-release-gate/resources/qa_product.pyapi/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000020_add_session_attachments.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/sessions/attachments/__init__.pyapi/oss/src/core/sessions/attachments/dtos.pyapi/oss/src/core/sessions/attachments/interfaces.pyapi/oss/src/core/sessions/attachments/media.pyapi/oss/src/core/sessions/attachments/service.pyapi/oss/src/core/sessions/attachments/types.pyapi/oss/src/core/sessions/interactions/dtos.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/src/dbs/postgres/mounts/dbas.pyapi/oss/src/dbs/postgres/mounts/mappings.pyapi/oss/src/dbs/postgres/sessions/attachments/__init__.pyapi/oss/src/dbs/postgres/sessions/attachments/dao.pyapi/oss/src/dbs/postgres/sessions/attachments/dbas.pyapi/oss/src/dbs/postgres/sessions/attachments/dbes.pyapi/oss/src/dbs/postgres/sessions/attachments/mappings.pyapi/oss/src/tasks/asyncio/sessions/attachment_sweep.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/mounts/test_attachments_mount_hidden.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachment_teardown.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachments.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/resources/test_workflow_catalog.pyapi/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.pyapi/oss/tests/pytest/unit/sessions/test_attachment_filename.pyapi/oss/tests/pytest/unit/sessions/test_attachment_media.pyapi/oss/tests/pytest/unit/sessions/test_attachment_quotas.pyapi/oss/tests/pytest/unit/sessions/test_attachment_state_machine.pyapi/oss/tests/pytest/unit/sessions/test_attachment_sweep.pyapi/oss/tests/pytest/unit/sessions/test_interaction_request_tool_call_id.pyapi/oss/tests/pytest/unit/sessions/test_records_truncation.pyapi/pyproject.tomldocs/design/agent-workflows/documentation/agent-configuration.mddocs/design/agent-workflows/documentation/tools.mddocs/design/agent-workflows/interfaces/README.mddocs/design/agent-workflows/interfaces/in-service/harness-adapters.mddocs/design/agent-workflows/interfaces/public-edge/agent-config-schema.mddocs/design/agent-workflows/projects/agent-multi-modality/README.mddocs/design/agent-workflows/projects/agent-multi-modality/context.mddocs/design/agent-workflows/projects/agent-multi-modality/decisions.mddocs/design/agent-workflows/projects/agent-multi-modality/design.mddocs/design/agent-workflows/projects/agent-multi-modality/plan.mddocs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.mddocs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-0.mddocs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mddocs/design/agent-workflows/projects/agent-multi-modality/research.mddocs/design/agent-workflows/projects/agent-multi-modality/scope.mddocs/design/agent-workflows/projects/agent-multi-modality/status.mddocs/design/agent-workflows/projects/default-agent-builtins/README.mddocs/design/agent-workflows/projects/default-agent-builtins/context.mddocs/design/agent-workflows/projects/default-agent-builtins/design.mddocs/design/agent-workflows/projects/default-agent-builtins/open-questions.mddocs/design/agent-workflows/projects/default-agent-builtins/plan.mddocs/design/agent-workflows/projects/default-agent-builtins/research.mddocs/design/agent-workflows/projects/default-agent-builtins/status.mddocs/design/agent-workflows/projects/default-agent-builtins/testing.mdhosting/docker-compose/oss/nginx/nginx.confsdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/adapters/agenta_builtins.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/errors.pysdks/python/agenta/sdk/agents/handler.pysdks/python/agenta/sdk/agents/pi_builtins.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.jsonsdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/oss/src/agent/app.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_config_template_fallback.pyservices/oss/tests/pytest/unit/agent/test_default_agent_template.pyservices/oss/tests/pytest/unit/agent/test_select_backend.pyservices/runner/config/agent.jsonservices/runner/src/engines/sandbox_agent/acp-interactions.tsservices/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/interactions.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/pi-default-builtins-parity.test.tsservices/runner/tests/unit/reconstruct-resume-nonfatal.test.tsservices/runner/tests/unit/sandbox-agent-acp-interactions.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-keepalive-approval.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.tsweb/oss/src/components/AgentChatSlice/AgentConversation.tsxweb/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- docs/design/agent-workflows/interfaces/README.md
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx
- services/runner/tests/unit/pi-default-builtins-parity.test.ts
- .agents/skills/agent-release-gate/resources/qa_product.py
- sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx
- sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py
- services/oss/tests/pytest/unit/agent/test_config_template_fallback.py
- services/runner/config/agent.json
- .agents/skills/agent-release-gate/resources/qa_probe.py
- web/packages/agenta-playground/tests/unit/agentRequest.test.ts
- sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py
- api/oss/tests/pytest/unit/resources/test_workflow_catalog.py
- docs/design/agent-workflows/projects/default-agent-builtins/context.md
- web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts
- sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py
- sdks/python/agenta/sdk/agents/pi_builtins.py
- docs/design/agent-workflows/documentation/agent-configuration.md
- sdks/python/agenta/sdk/utils/types.py
- sdks/python/agenta/sdk/agents/adapters/harnesses.py
- services/oss/src/agent/config.py
- web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts
- docs/design/agent-workflows/projects/default-agent-builtins/README.md
- sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py
- services/oss/tests/pytest/unit/agent/test_default_agent_template.py
- docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md
🛑 Comments failed to post (8)
api/oss/src/core/sessions/attachments/media.py (1)
112-141: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect the session attachment download route response headers. set -euo pipefail fd -t f 'router.py' api/oss/src/apis/fastapi/sessions | while IFS= read -r file; do rg -n -C 12 'attachment' "$file" done # Look for content-type / disposition handling anywhere in the attachment paths. rg -n -i 'content-disposition|content_type|media_type=' --glob 'api/oss/src/apis/fastapi/**/*.py'Repository: Agenta-AI/agenta
Length of output: 14120
🏁 Script executed:
#!/bin/bash # Inspect the response content-type and sanitize any active-media active-content handling. set -euo pipefail sed -n '950,1092p' api/oss/src/apis/fastapi/sessions/router.py printf '\n--- classification constants and functions ---\n' sed -n '1,190p' api/oss/src/core/sessions/attachments/media.py printf '\n--- content model / fetch path ---\n' rg -n -C 8 'AttachmentContent|fetch_attachment_content|media_type=content|media_type =' api/oss/src/core/sessions/attachments api/oss/src/apis/fastapi/sessionsRepository: Agenta-AI/agenta
Length of output: 20703
Sanitize active-media content types before download.
download_session_attachment_contentreturns uploads as the storedmedia_type.classifycan exposetext/htmlorimage/svg+xml, and fallback can retain caller-declaredtext/*media types. For active-media types, either strip the upload from safe media types before classification or returnapplication/octet-streamfrom this endpoint.docs/design/agent-workflows/projects/agent-multi-modality/design.md (1)
388-393: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Define the text attachment path separately from native documents.
text/*andapplication/jsonare listed underdocument, but the same design says small text can be inlined as text. The Stage 2 plan treats documents as workspace-only until adapter support. If the runner deriveskind = documentfrom the stored type, text attachments can be incorrectly blocked or sent throughembeddedContext.Define a separate text path, or document an explicit exception for text and JSON. Apply the same rule in
plan.mdandplans/stage-1-implementation.md.Also applies to: 729-742
docs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.md (1)
120-123: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the sweep contract with the implemented phased deletion.
These sections say the sweep holds row locks while deleting objects and re-checks state inside the deleting transaction.
protocols/stage-1.mdrecords a different implementation: tombstone rows, commit, delete objects outside the transaction, then remove rows.These algorithms have different lock duration and failure semantics. Use the phased tombstone algorithm as the single contract, or update the protocol and tests to the lock-held algorithm.
Also applies to: 515-531
docs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.md (1)
220-239: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the future-dated QA claim.
The review date is July 31, 2026, but this section reports live QA on August 1, 2026 and says the results were re-verified before the PR left draft.
Change the date to the actual execution date or mark the QA as pending. Release evidence must not use a future date.
docs/design/agent-workflows/projects/agent-multi-modality/research.md (1)
648-653: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the replay claim.
D12 states that cold replay is mention-first. It restores the working copy and textual mention; it does not resend inline ACP content. Limit this statement to the running turn.
Proposed wording
- and survives a replay. + and is deterministic for the running turn; cold replay restores a working copy and textual mention.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.The conclusion the design draws: use both, for different purposes. Inline delivery (Mechanism A) is the only path that guarantees perception: it happens deterministically at the prompt-build seam the runner owns, works the same on every harness that advertises the capability, and is deterministic for the running turn; cold replay restores a working copy and textual mention. The disk copy (Mechanism B) serves tool use, and on some harnesses it adds a second, agent-driven route to perception, which is a useful fallback rather than a substitute. This is not redundant. The two mechanisms serve the two separate outcomes from [context.md](context.md): perception and tooldocs/design/agent-workflows/projects/agent-multi-modality/status.md (1)
41-43: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the stage tracker with the first-release contract.
scope.mdsays the first release includes browser dictation and that attachment attempts must not silently do nothing. This table places dictation in Stage 2 and makes Stage 0 optional, although Stage 0 contains the paste and drag gate. Move these requirements into required Stage 1, or revise the scope before implementation starts.services/runner/src/server.ts (1)
1136-1148: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle a failed attachment claim.
claimAttachmentsreturnsfalsewhen the reference call fails. The result is discarded here, so the turn continues with attachments that the API still treats as unreferenced. The attachment sweep can then delete those rows and blobs while the persisted user record still points at them, which breaks both this turn's materialization and later history replay.The claim also runs after the user record is persisted. If the claim fails, the durable record already asserts a reference that the API never recorded.
Claim first, then persist, and act on the failure (fail the turn or record the degraded state explicitly).
🛠️ Proposed ordering and failure handling
if (tailIsFreshUserMessage(request)) { + if (turn.attachments.length > 0) { + const claimed = await claimAttachments( + sessionId, + turn.attachments.map((attachment) => attachment.attachmentId), + watchdog.credential, + ); + if (!claimed) { + const message = "Could not reference this turn's attachments."; + persist({ type: "error", message }, "agent"); + await flush(); + await watchdog.release().catch(() => {}); + writeRecord({ + kind: "result", + result: { ok: false, error: message, events: [] }, + }); + res.end(); + return; + } + } persist( { type: "message", text: turn.text, attachments: turn.attachments }, "user", ); - if (turn.attachments.length > 0) { - await claimAttachments( - sessionId, - turn.attachments.map((attachment) => attachment.attachmentId), - watchdog.credential, - ); - } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (tailIsFreshUserMessage(request)) { if (turn.attachments.length > 0) { const claimed = await claimAttachments( sessionId, turn.attachments.map((attachment) => attachment.attachmentId), watchdog.credential, ); if (!claimed) { const message = "Could not reference this turn's attachments."; persist({ type: "error", message }, "agent"); await flush(); await watchdog.release().catch(() => {}); writeRecord({ kind: "result", result: { ok: false, error: message, events: [] }, }); res.end(); return; } } persist( { type: "message", text: turn.text, attachments: turn.attachments }, "user", ); }web/oss/src/components/AgentChatSlice/AgentConversation.tsx (1)
1778-1779: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce
uploadsEnabledat the shared attachment boundaries.
attachmentsBlocked()protects paste and drag-and-drop, butaddFiles()andhandleSubmit()remain ungated.ComposerAttachmentsreceivesonAdd={addFiles}unconditionally, andhandleSubmit()still converts staged files intofilePartswhenuploadsEnabledis false.Apply the flag at the shared mutation and submission boundaries. Keep voice audio as an explicit separate path if
NEXT_PUBLIC_AGENT_VOICE_INPUTis intentionally independent. Otherwise, the “every attach path” guarantee is not enforced.Also applies to: 1815-1815
9b0edae to
8039bcc
Compare
bb7728c to
f4a014e
Compare
8039bcc to
35d24d6
Compare
f4a014e to
200c360
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/design/agent-workflows/projects/default-agent-builtins/testing.md (1)
66-69: 🎯 Functional Correctness | 🟡 MinorState that this test verifies grants, not execution permission.
The test proves that the default carries
read,bash,edit, andwritegrants. Underallow_reads,bash,edit, andwritestill require approval. Replace “must be able to” with wording such as “must carry grants for” to avoid implying unattended execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c628177-556c-49bb-bc3d-089c99368c44
📒 Files selected for processing (36)
.agents/skills/agent-release-gate/resources/qa_probe.py.agents/skills/agent-release-gate/resources/qa_product.pyapi/oss/tests/pytest/unit/resources/test_workflow_catalog.pydocs/design/agent-workflows/documentation/agent-configuration.mddocs/design/agent-workflows/documentation/tools.mddocs/design/agent-workflows/interfaces/README.mddocs/design/agent-workflows/interfaces/in-service/harness-adapters.mddocs/design/agent-workflows/interfaces/public-edge/agent-config-schema.mddocs/design/agent-workflows/projects/default-agent-builtins/README.mddocs/design/agent-workflows/projects/default-agent-builtins/context.mddocs/design/agent-workflows/projects/default-agent-builtins/design.mddocs/design/agent-workflows/projects/default-agent-builtins/open-questions.mddocs/design/agent-workflows/projects/default-agent-builtins/plan.mddocs/design/agent-workflows/projects/default-agent-builtins/research.mddocs/design/agent-workflows/projects/default-agent-builtins/status.mddocs/design/agent-workflows/projects/default-agent-builtins/testing.mdsdks/python/agenta/sdk/agents/adapters/agenta_builtins.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/pi_builtins.pysdks/python/agenta/sdk/utils/types.pysdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.jsonsdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/oss/src/agent/config.pyservices/oss/tests/pytest/unit/agent/test_config_template_fallback.pyservices/oss/tests/pytest/unit/agent/test_default_agent_template.pyservices/runner/config/agent.jsonservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/tests/unit/pi-default-builtins-parity.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsweb/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- services/runner/tests/unit/pi-default-builtins-parity.test.ts
- docs/design/agent-workflows/documentation/agent-configuration.md
- services/oss/tests/pytest/unit/agent/test_default_agent_template.py
- api/oss/tests/pytest/unit/resources/test_workflow_catalog.py
- .agents/skills/agent-release-gate/resources/qa_product.py
- services/runner/config/agent.json
- sdks/python/oss/tests/pytest/unit/agents/test_wire_contract.py
- sdks/python/oss/tests/pytest/unit/agents/test_pi_builtins_parity.py
- web/packages/agenta-entity-ui/tests/unit/itemDescriptors.test.ts
- sdks/python/agenta/sdk/agents/pi_builtins.py
- docs/design/agent-workflows/interfaces/README.md
- services/oss/src/agent/config.py
- sdks/python/agenta/sdk/utils/types.py
- sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsx
- docs/design/agent-workflows/interfaces/public-edge/agent-config-schema.md
- sdks/python/agenta/sdk/agents/adapters/harnesses.py
- docs/design/agent-workflows/projects/default-agent-builtins/context.md
- sdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.py
- web/packages/agenta-playground/tests/unit/agentRequest.test.ts
- sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py
- web/packages/agenta-entities/tests/unit/create-ephemeral-app-from-template.test.ts
- web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/PiSettingsControl.tsx
- docs/design/agent-workflows/projects/default-agent-builtins/README.md
| Two copies of the default do not go through the builder and are worth naming so a reader does not | ||
| assume they moved. | ||
|
|
||
| `services/oss/src/agent/config.py:106` supplies `tools: []` as the request-time fallback for a | ||
| request that carries no agent template at all (threaded through | ||
| `services/oss/src/agent/app.py:58` into `AgentTemplate.from_params`). This path is reached only by | ||
| a caller that posts an agent invocation with no `parameters.agent`, which the platform does not | ||
| do. Leave it alone in this change. Aligning it is a separate cleanup with its own risk, and it is | ||
| listed in [open-questions.md](open-questions.md). | ||
|
|
||
| `sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py:126` holds a documentation copy of the | ||
| config shape inside the build-an-agent skill, and its example shows `"tools": []`. That example | ||
| teaches the builder agent what a config looks like, so it should show the new default. It is a | ||
| text change with a drift test already in place | ||
| (`sdks/python/oss/tests/pytest/unit/agents/test_agenta_builtins_reference_files.py`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the consumer inventory after the implementation.
This section still says that services/oss/src/agent/config.py and sdks/python/agenta/sdk/agents/adapters/agenta_builtins.py use tools: []. The current change updates the service fallback and the build-agent fixture to the four Pi built-ins. Mark these entries as implemented and describe their current values.
| ## No pinned wire contract moves | ||
|
|
||
| The `/run` request field stays `tools?: string[]` | ||
| (`services/runner/src/protocol.ts:469`). No field is added, removed, renamed, or retyped. | ||
|
|
||
| The shared golden fixtures under `sdks/python/oss/tests/pytest/unit/agents/golden/` do not change. | ||
| `test_wire_contract.py` builds its Pi payload from a hand-written | ||
| `PiAgentTemplate(builtin_tools=["read", "write"])` at `:125` and never touches | ||
| `build_agent_v0_default` or `AgentTemplate.from_params`, so the golden is unaffected. | ||
| `services/runner/tests/unit/wire-contract.test.ts` reads the same files and asserts on the parsed | ||
| request, not on a run plan built from it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the golden-fixture statement.
The PR adds sdks/python/oss/tests/pytest/unit/agents/golden/pi_default_active_builtins.json. Replace “The shared golden fixtures ... do not change” with wording that says the existing run-request fixtures remain unchanged and the new parity fixture is added.
| Every agent saved since the empty default shipped carries `tools: []` and keeps failing outside the | ||
| playground until its author edits it. Repairing them means reinterpreting a stored value, and there | ||
| is no signal that separates "the default put an empty list here" from "I deselected everything", | ||
| because they are the same value. The full reasoning is in | ||
| [design.md](design.md#repair-agents-already-saved). | ||
|
|
||
| Revisit if the count of affected agents turns out to be large. The narrow version, backfilling only | ||
| Pi agents whose tools list is empty and whose revision predates the fix, is implementable as its | ||
| own change. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit the migration statement to default-derived Pi agents.
“Every agent saved since the empty default shipped” also includes agents with explicit tool lists or other harnesses. The objective states that existing configurations retain their values. Change this to “agents created from the empty Pi default” so the repair scope remains precise.
| ## Blocking nothing, waiting on nothing | ||
|
|
||
| The open questions in [open-questions.md](open-questions.md) did not block the work; the first of | ||
| them determines whether follow-up work is needed for the scheduled-run half of | ||
| [#5562](https://github.com/Agenta-AI/agenta/issues/5562). | ||
|
|
||
| When the issue is closed, say that this fixes newly created agents only. It does not repair agents | ||
| already saved, and it does not make an unattended write-capable run complete on its own. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Rename this section because pre-ship conditions remain open.
Line 79-80 and Line 122-125 state that manual verification and the local-sandbox deployment decision remain open. The heading “Blocking nothing, waiting on nothing” contradicts those statements and can make the change appear ready for deployment before the host-file access risk is addressed. Rename the heading to distinguish completed implementation work from remaining release checks.
Suggested wording
-## Blocking nothing, waiting on nothing
+## No implementation blockers remain📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Blocking nothing, waiting on nothing | |
| The open questions in [open-questions.md](open-questions.md) did not block the work; the first of | |
| them determines whether follow-up work is needed for the scheduled-run half of | |
| [#5562](https://github.com/Agenta-AI/agenta/issues/5562). | |
| When the issue is closed, say that this fixes newly created agents only. It does not repair agents | |
| already saved, and it does not make an unattended write-capable run complete on its own. | |
| ## No implementation blockers remain | |
| The open questions in [open-questions.md](open-questions.md) did not block the work; the first of | |
| them determines whether follow-up work is needed for the scheduled-run half of | |
| [`#5562`](https://github.com/Agenta-AI/agenta/issues/5562). | |
| When the issue is closed, say that this fixes newly created agents only. It does not repair agents | |
| already saved, and it does not make an unattended write-capable run complete on its own. |
200c360 to
4708ca5
Compare
dosubot
Bot
added
size:M
dosubot
Bot
added
size:XXL
A saved agent shipped `tools: []`, which the runner reads as "grant Pi no built-in tools", so the agent had no read, bash, edit or write anywhere except the playground. The playground hid it: its build kit overlay adds read and bash back on every run, and nothing else does. The default template now ships Pi's own four defaults as typed builtin entries. The runner's grant semantics are untouched, and the permission mode still gates bash, edit and write behind an approval. Also: the Claude harness only warns about built-ins that differ from that exact set, the built-in picker no longer claims an empty selection leaves Pi's defaults on, and a built-in row shows its own name. Closes #5590
…ate QA seeds to Pi
4708ca5 to
356aa03
Compare
1 participant
Context
A saved agent could not run a shell command or write a file anywhere except the playground. Asked to do either, it answered that it had no shell or filesystem tool available.
Pi keeps seven tools inside the harness, and the run request names which of them to switch on. The runner treats a missing
toolsfield as "use Pi's defaults" and an empty array as "grant nothing", and the Pi extension then deletes every non-granted tool from the model's list. The default agent template shipped an empty array, and the Python side always sends the field, so every saved agent said "grant nothing".The playground hid this. Its build kit overlay adds
readandbashback on every run, and nothing else does. So an agent worked while you built it and failed the moment a schedule ran it.Changes
build_agent_v0_default()now ships Pi's own four defaults.Before:
"tools": []After:
The runner's rules are untouched. An empty list still means grant nothing, and the permission mode still gates
bash,editandwritebehind an approval. Having a tool is not the same as being allowed to use it.Three supporting fixes ship with it:
The Claude harness warns only about built-ins outside that exact set. Built-ins are a Pi idea and Claude drops them, so without this every Claude run would log a warning about the four tools the template now always carries.
The built-in tool picker no longer tells you that clearing your selection leaves Pi's defaults on. It does the opposite.
The release gate seeds no longer hand-write the empty list, so the gate stops reporting green on the exact shape this change replaces.
Scope is deliberately new agents only. Agents saved before this keep their empty list and stay broken outside the playground. On one development deployment that is 66 of 73 Pi agents, so it is most of them. Repairing them is its own decision, filed as #5595 with the counting query.
Before merging
Do not deploy this to a shared deployment that still enables the
localsandbox. The default sandbox islocal, which runs on the runner host with no jail, andreadnever asks under the default permission mode. So a new agent could read host files unattended.This adds no capability an author did not already have. Both the tool list and the sandbox are author-writable today, so anyone could already save the same agent and schedule it. What changes is what an unwitting author gets by default. Confirm
AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERSexcludeslocalbefore deploying, and remember that leaving it unset meanslocalis enabled.Tests
A crossing test now runs the real chain from the shipped default through the harness adapter to the
/runbody and asserts the four names arrive. Neither existing suite crossed that boundary, which is why both stayed green while every agent shipped with no tools.The four names are pinned by a shared golden fixture that the Python and TypeScript suites both read, so the two languages cannot drift apart silently.
Runner 1308 passing, SDK 2330, API 1495, and 1427 across three web packages. Every error in the Python suites is an acceptance test asking for a live stack.
Verified on a running deployment: a fresh account's
/inspectreturns the four built-in entries as the default a new agent pre-fills with.Reviewed by Codex at high effort before implementation, and by two independent reviewers after. Their findings and what was accepted or rejected are recorded in
docs/design/agent-workflows/projects/default-agent-builtins/status.md.What to QA
Closes #5590