feat(agent): playground build kit (default agent config)#4926
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
@Agenta-AI please review tomorrow — DRAFT, do not merge. Specific feedback wanted:
Implemented by Codex (gpt-5.5, xhigh). Change 1 (collapsible drawer sections) intentionally excluded. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
services/oss/tests/pytest/unit/agent/test_default_agent_template.py (1)
59-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the sandbox-flag assertions for the builtin default.
The test name says every published default, but
execute_codeandwrite_filesare only checked oninspect_default. A regression on the SDK builtin path would still pass this suite.Proposed test tightening
assert builtin_default["tools"] == [] assert "permissions" not in builtin_default["sandbox"] + assert "execute_code" not in builtin_default["sandbox"] + assert "write_files" not in builtin_default["sandbox"] assert "skills" not in builtin_defaultweb/packages/agenta-entities/src/workflow/state/store.ts (1)
1106-1114: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the overlay with a schema at the inspect boundary.
This only proves the top level is an object, so drift inside
sandbox,tools, orskillswill flow straight into the merge/UI path asAgentTemplate. Please runagent_template_overlaythrough a local Zod schema andsafeParseWithLoggingbefore exposing it from this atom family. Based on coding guidelines, "Keep Zod validation at the API boundary even when using Fern-generated types, because local schemas still detect backend drift" and "UsesafeParseWithLoggingfrom@agenta/entities/sharedfor boundary validation so structured errors are logged without crashing."Source: Coding guidelines
web/packages/agenta-playground/tests/unit/agentRequest.test.ts (1)
285-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the real commit source stays bare.
Line 302 checks
workflowMolecule.selectors.configuration("e"), butprepareCommitParametersreadsentity.data?.parameters. This test can still pass if build-kit fields start leaking into the actual commit payload later. Seedover.datawithdata.parametersand assert that object remains unchanged instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 38316881-acc2-4f09-88d6-97cbd6271674
📒 Files selected for processing (20)
api/oss/src/apis/fastapi/applications/models.pyapi/oss/src/apis/fastapi/applications/overlay.pyapi/oss/src/apis/fastapi/applications/router.pyapi/oss/tests/pytest/unit/applications/test_build_kit_overlay.pydocs/design/agent-workflows/projects/default-agent-config/README.mddocs/design/agent-workflows/projects/default-agent-config/design.mddocs/design/agent-workflows/projects/default-agent-config/research.mddocs/design/agent-workflows/projects/default-agent-config/status.mdservices/oss/src/agent/schemas.pyservices/oss/tests/pytest/unit/agent/test_default_agent_template.pyweb/packages/agenta-entities/src/workflow/api/api.tsweb/packages/agenta-entities/src/workflow/index.tsweb/packages/agenta-entities/src/workflow/state/commit.tsweb/packages/agenta-entities/src/workflow/state/index.tsweb/packages/agenta-entities/src/workflow/state/store.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-playground/src/state/execution/agentRequest.tsweb/packages/agenta-playground/src/state/execution/index.tsweb/packages/agenta-playground/src/state/index.tsweb/packages/agenta-playground/tests/unit/agentRequest.test.ts
| agent_template_overlay: Optional[dict] = Field( | ||
| default=None, | ||
| description="Partial `parameters.agent` overlay applied by the playground only.", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Model agent_template_overlay explicitly instead of using dict.
This is now a backend/frontend contract, but Optional[dict] leaves the OpenAPI schema and runtime validation opaque. Please promote the overlay shape into concrete Pydantic models (or typed submodels for tools, skills, and sandbox) so downstream clients get a stable contract. As per coding guidelines, "Define explicit request and response models in models.py."
Source: Coding guidelines
| revision = catalog.retrieve_revision(slug=slug) | ||
| if revision and revision.flags and revision.flags.is_skill: | ||
| continue | ||
| slugs.append(slug) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tighten the reserved-workflow filter.
This currently appends reserved slugs even when retrieve_revision() returns None or revision.flags is missing. The expected overlay only includes confirmed non-skill static workflows, so this can leak invalid tool embeds into the playground.
Suggested fix
revision = catalog.retrieve_revision(slug=slug)
- if revision and revision.flags and revision.flags.is_skill:
+ if not revision or not revision.flags or revision.flags.is_skill:
continue
slugs.append(slug)📝 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.
| revision = catalog.retrieve_revision(slug=slug) | |
| if revision and revision.flags and revision.flags.is_skill: | |
| continue | |
| slugs.append(slug) | |
| revision = catalog.retrieve_revision(slug=slug) | |
| if not revision or not revision.flags or revision.flags.is_skill: | |
| continue | |
| slugs.append(slug) |
| additional_context=SimpleApplicationAdditionalContext( | ||
| playground_build_kit=PlaygroundBuildKitContext( | ||
| agent_template_overlay=build_agent_template_overlay(), | ||
| ), | ||
| ) | ||
| if simple_application | ||
| else None, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t let build-kit synthesis blank the whole fetch response.
fetch_simple_application() is wrapped in @suppress_exceptions(default=SimpleApplicationResponse()). If build_agent_template_overlay() raises, this path now returns an empty 200 response instead of the fetched application. Build the overlay behind a local try/except and fall back to additional_context=None so the inspect path still works. As per path instructions, use @suppress_exceptions(...) only for controlled defaults.
Source: Path instructions
| function overriddenPermissionKeys( | ||
| userPermissions: Record<string, unknown> | null | undefined, | ||
| overlayPermissions: Record<string, unknown> | null | undefined, | ||
| ): string[] { | ||
| if (!userPermissions || !overlayPermissions) return [] | ||
| return Object.entries(overlayPermissions) | ||
| .filter(([key, overlayValue]) => { | ||
| if (!(key in userPermissions)) return false | ||
| return stableString(userPermissions[key]) !== stableString(overlayValue) | ||
| }) | ||
| .map(([key]) => key) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include overlay-added permissions in the override list.
The key in userPermissions guard drops permissions that the build kit adds on top of the draft. In the common write_files/new-network-key case, the warning next to SandboxPermissionControl under-reports the effective playground permissions.
Suggested fix
function overriddenPermissionKeys(
userPermissions: Record<string, unknown> | null | undefined,
overlayPermissions: Record<string, unknown> | null | undefined,
): string[] {
- if (!userPermissions || !overlayPermissions) return []
+ if (!overlayPermissions) return []
return Object.entries(overlayPermissions)
- .filter(([key, overlayValue]) => {
- if (!(key in userPermissions)) return false
- return stableString(userPermissions[key]) !== stableString(overlayValue)
- })
+ .filter(
+ ([key, overlayValue]) =>
+ stableString(userPermissions?.[key]) !== stableString(overlayValue),
+ )
.map(([key]) => key)
}📝 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.
| function overriddenPermissionKeys( | |
| userPermissions: Record<string, unknown> | null | undefined, | |
| overlayPermissions: Record<string, unknown> | null | undefined, | |
| ): string[] { | |
| if (!userPermissions || !overlayPermissions) return [] | |
| return Object.entries(overlayPermissions) | |
| .filter(([key, overlayValue]) => { | |
| if (!(key in userPermissions)) return false | |
| return stableString(userPermissions[key]) !== stableString(overlayValue) | |
| }) | |
| .map(([key]) => key) | |
| function overriddenPermissionKeys( | |
| userPermissions: Record<string, unknown> | null | undefined, | |
| overlayPermissions: Record<string, unknown> | null | undefined, | |
| ): string[] { | |
| if (!overlayPermissions) return [] | |
| return Object.entries(overlayPermissions) | |
| .filter( | |
| ([key, overlayValue]) => | |
| stableString(userPermissions?.[key]) !== stableString(overlayValue), | |
| ) | |
| .map(([key]) => key) | |
| } |
| const agentTemplateOverlay = useAtomValue( | ||
| useMemo(() => workflowAgentTemplateOverlayAtomFamily(revisionId ?? ""), [revisionId]), | ||
| ) | ||
| const [buildKitEnabled, setBuildKitEnabled] = useAtom( | ||
| useMemo(() => workflowBuildKitEnabledAtomFamily(revisionId ?? ""), [revisionId]), | ||
| ) | ||
| const [buildKitExpanded, setBuildKitExpanded] = useState(true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cancel won't restore the build-kit toggle.
buildKitEnabled now lives outside the config snapshot, but cancelSection() only rolls back value. In drawer layouts, toggling this switch and pressing Cancel still changes later playground runs.
Suggested fix
+ const buildKitSnapshot = useRef<boolean | null>(null)
+
const openSectionDrawer = useCallback(
(key: "model-harness" | "advanced") => {
sectionSnapshot.current = value ?? {}
+ buildKitSnapshot.current = key === "advanced" ? buildKitEnabled : null
setOpenSection(key)
},
- [value],
+ [value, buildKitEnabled],
)
const cancelSection = useCallback(() => {
if (sectionSnapshot.current) onChange(sectionSnapshot.current)
+ if (openSection === "advanced" && buildKitSnapshot.current != null) {
+ setBuildKitEnabled(buildKitSnapshot.current)
+ }
setOpenSection(null)
- }, [onChange])
+ }, [onChange, openSection, setBuildKitEnabled])📝 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.
| const agentTemplateOverlay = useAtomValue( | |
| useMemo(() => workflowAgentTemplateOverlayAtomFamily(revisionId ?? ""), [revisionId]), | |
| ) | |
| const [buildKitEnabled, setBuildKitEnabled] = useAtom( | |
| useMemo(() => workflowBuildKitEnabledAtomFamily(revisionId ?? ""), [revisionId]), | |
| ) | |
| const [buildKitExpanded, setBuildKitExpanded] = useState(true) | |
| const agentTemplateOverlay = useAtomValue( | |
| useMemo(() => workflowAgentTemplateOverlayAtomFamily(revisionId ?? ""), [revisionId]), | |
| ) | |
| const [buildKitEnabled, setBuildKitEnabled] = useAtom( | |
| useMemo(() => workflowBuildKitEnabledAtomFamily(revisionId ?? ""), [revisionId]), | |
| ) | |
| const [buildKitExpanded, setBuildKitExpanded] = useState(true) | |
| const buildKitSnapshot = useRef<boolean | null>(null) | |
| const openSectionDrawer = useCallback( | |
| (key: "model-harness" | "advanced") => { | |
| sectionSnapshot.current = value ?? {} | |
| buildKitSnapshot.current = key === "advanced" ? buildKitEnabled : null | |
| setOpenSection(key) | |
| }, | |
| [value, buildKitEnabled], | |
| ) | |
| const cancelSection = useCallback(() => { | |
| if (sectionSnapshot.current) onChange(sectionSnapshot.current) | |
| if (openSection === "advanced" && buildKitSnapshot.current != null) { | |
| setBuildKitEnabled(buildKitSnapshot.current) | |
| } | |
| setOpenSection(null) | |
| }, [onChange, openSection, setBuildKitEnabled]) |
| {hasBuildKitOverlay ? ( | ||
| <div className="rounded border border-solid border-[var(--ag-c-EAEFF5,#eaeff5)] bg-[#fcfcfa]"> | ||
| <button | ||
| type="button" | ||
| onClick={() => setBuildKitExpanded((open) => !open)} | ||
| className="flex w-full cursor-pointer items-center gap-2 border-0 bg-transparent px-3 py-2.5 text-left" | ||
| > | ||
| <Wrench size={15} className="text-[var(--ag-c-586673,#586673)]" /> | ||
| <span className="text-[13px] font-medium">Playground build kit</span> | ||
| <span className="ml-auto inline-flex items-center gap-1.5 text-[11px] text-[var(--ag-c-586673,#586673)]"> | ||
| <span className="h-1.5 w-1.5 rounded-full bg-[#d97706]" /> | ||
| Removed on commit | ||
| </span> | ||
| <span | ||
| onClick={(e) => e.stopPropagation()} | ||
| className="inline-flex items-center" | ||
| > | ||
| <Switch | ||
| size="small" | ||
| checked={buildKitEnabled} | ||
| onChange={setBuildKitEnabled} | ||
| disabled={disabled} | ||
| /> | ||
| </span> | ||
| <CaretRight | ||
| size={14} | ||
| className={cn( | ||
| "text-[var(--ag-c-97A4B0,#97a4b0)] transition-transform", | ||
| buildKitExpanded && "rotate-90", | ||
| )} | ||
| /> | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsx'
# Show the relevant region with line numbers.
sed -n '1618,1692p' "$file"
# Map the component structure around the snippet.
grep -nE '<button|</button>|<Switch|onClick=|stopPropagation' "$file" | sed -n '1,120p'Repository: Agenta-AI/agenta
Length of output: 5513
Move the switch out of the header button
Switch is an interactive control, so nesting it inside the clickable <button> creates invalid interactive content and can break keyboard/focus handling. Keep the header toggle and the switch as separate controls.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🤖 The AI agent says: What to review on this PR:
|
|
🤖 The AI agent says: Superseded by #4929 — same content, rebuilt as a clean GitButler stack. Closing this one. |
1 participant
Context
When a user opens the playground to build a new agent, the assistant needs authoring scaffolding: the platform ops (find capabilities, commit a revision, etc.), the Agenta getting-started skill, and elevated sandbox permissions so it can write and execute files. That scaffolding is a build aid, not the user's app. The committed agent must carry only what the user authored.
Previously
build_agent_v0_default()baked these into the published default, so every deployed agent shipped with platform tools and sandbox elevation baked in. This PR fixes that by separating "what the playground injects for a build run" from "what gets committed."Design:
docs/design/agent-workflows/projects/default-agent-config/design.mdChanges
The backend now builds a read-only agent-template overlay and attaches it to the inspect response. The frontend merges it onto
parameters.agentonly for a playground run, and leaves it out when committing.Before:
build_agent_v0_default()returned a default that included platform tools, the authoring skill, and sandbox elevation. Every inspect response returned that enriched default. The commit path had to strip it out manually (or didn't, and it leaked in).After:
build_agent_v0_default()is bare.fetch_simple_applicationattachesadditional_context.playground_build_kit.agent_template_overlayto the response. The frontend'sapplyBuildKitOverlaydoes a deep-merge of the overlay into the run config. The commit reads only persisted entity parameters, so the overlay is excluded for free.The overlay shape:
{ "tools": [ { "type": "platform", "op": "<op_name>" }, { "@ag.embed": { "@ag.references": { "workflow": { "slug": "__ag__..." } } } } ], "skills": [{ "@ag.embed": { "@ag.references": { "workflow": { "slug": "__ag__getting_started_with_agenta" } } } }], "sandbox": { "permissions": { "write_files": "allow", "execute_code": "allow" } } }Key files:
api/oss/src/apis/fastapi/applications/overlay.py(new) —build_agent_template_overlay()assembles the overlay fromPLATFORM_OPS+ reserved static workflow slugs + the authoring skill slug.api/oss/src/apis/fastapi/applications/router.py—fetch_simple_applicationattaches the overlay; create/edit/commit paths do not.services/oss/src/agent/schemas.py— revertedbuild_agent_v0_default()to bare (no platform tools, no skills, no sandbox elevation).web/packages/agenta-playground/src/state/execution/agentRequest.ts—applyBuildKitOverlay(deep-merge objects, identity-merge lists), called inbuildAgentRequestonly when the kit is enabled.web/packages/agenta-entity-ui/.../AgentTemplateControl.tsx— read-only "Playground build kit" panel with an enable/disable toggle and "Removed on commit" tag.Out of scope: collapsible advanced-drawer sections (Change 1 from the design) — ships separately.
Scope / risk
The overlay is computed fresh on every inspect call from static sources (
PLATFORM_OPS,_STATIC_WORKFLOWS, the getting-started slug). It is never stored. A stale catalog entry would show in the overlay without affecting committed revisions.The merge in
applyBuildKitOverlayis shallow for top-level keys and identity-merge for lists. That means the overlay'stoolslist replaces (not appends to) a run'stoolsif the overlay is applied. Review whether deep-merge vs. replace is the right semantic fortoolsin edge cases where the user has also configured tools.The frontend toggle (
buildKitEnabled) defaults on. There is no server-side flag; disabling it is a client session state.Tests
api/oss/tests/pytest/unit/applications/test_build_kit_overlay.py(new): overlay builder shape; inspect response carriesadditional_context.playground_build_kit.services/oss/tests/pytest/unit/agent/test_default_agent_template.py: published default is bare across builtin, inspect schema, and catalog.web/packages/agenta-playground/tests/unit/agentRequest.test.ts: kit-on merges overlay, kit-off sends bare config, commit excludes the kit, applier never mutates the input.Codex-reported results: API overlay test 3/3, services default-agent test 5/5, playground vitest 148/148. Not independently re-run in CI on this branch yet.
How to QA
Prerequisites: local dev stack (
run.sh --oss --devor--ee --dev).Steps:
Expected result: Overlay present on run, absent on commit. Toggle removes it from runs.
Automated tests:
Edge cases: If the user has manually configured tools on their agent before the overlay merges, check that the commit writes only the user-configured tools and not the overlay tools.
https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc