feat(frontend): elicitation M1 follow-ups — enum "Other" option + emit-harness coverage - #5177
Conversation
Layer B for the interaction-kinds emit/resume path: drive buildClientToolRelay with the real ApprovalResponder built from run history (extract -> ConversationDecisions -> ApprovalResponder -> relay), exactly as a live /run wires it. Pins the whole emit/pause/resume decision end to end, not just the relay's mechanics. - cross-turn: a new identical call in a later turn pauses and emits a fresh form (verified red without the current-turn scoping fix; a FE-only mock could not have caught this). - in-turn: a genuine resume fulfills from its own output without re-emitting. Also admit request_input's elicitation render in the runner's TS-only RenderHint union, mirroring the existing connect member (render rides as an opaque dict on the wire, so this is type-only; no wire.py or golden change).
…ent-tool-relay-integration
An enum in an elicitation form rendered as a strict dropdown, so the user was locked into the agent's suggested options. But the consumer of the answer is the agent (an LLM), so those options are suggestions, not a hard constraint — the user should be able to go off-menu. Add an opt-in openEnums build option (@agenta/shared) that stamps allowCustomEnum onto enum descriptors; SchemaForm then renders enum fields with an "Other…" entry that reveals a free-text input. ElicitationWidget opts in. Gateway-tool execution forms do NOT (their enums are real API params) — guarded by a flag-off regression test. No wire/validator/golden change: the schema still declares enum, only the rendering changes, and the answer already rides as free-form content.
Layer A of the interaction-kinds emit-harness: a deterministic Playwright acceptance spec for the elicitation kind that mocks only the agent run (page.route on **/invoke*) with byte-accurate AI SDK v6 SSE, keeping auth, project, the seeded agent revision, and the playground shell real. - assets/elicitationStream.ts: canned SSE builders (paused elicitation turn + resume turn), pinned against the Python vercel adapter wire. - tests.ts: the transport mock, an is_agent app seed (data.uri = agenta:builtin:agent:v0), navigation, and the composer helper. - index.ts: round-trip, required-gate, settled-replay, reload-while-pending. First-run pending: the composer/field selectors and the reload rehydration source are flagged in-file to confirm on first green run against a live stack.
…ent-tool-relay-integration
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ 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:
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/oss/tests/playwright/acceptance/agent-chat/tests.ts (1)
36-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
page.unroutecleanup inmockElicitationInvoke.While Playwright's test isolation typically handles route cleanup between tests, explicitly unrouting in a fixture cleanup or after the
usecallback is more robust and prevents route leakage if the fixture is ever used outside the standard test lifecycle.web/oss/tests/playwright/acceptance/agent-chat/index.ts (1)
149-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
scenarios.given/andconsistently for setup steps in specs 3 & 4.Specs 1 and 2 wrap all setup in
scenarios.given/scenarios.and, but specs 3 and 4 (lines 167-171, 209-213) do rawawaitcalls for auth/seed/navigate/mock setup. This inconsistency makes the test report harder to read and diverges from the established pattern.♻️ Proposed fix for spec 3 (apply similarly to spec 4)
- await expectAuthenticatedSession(page) - const appId = await seedAgentChatApp() - await navigateToAgentPlayground(appId) - const mock = await mockElicitationInvoke() - mock.setResumeText("Recorded.") - - await sendChatMessage("hi") - await expect(page.getByText(ELICITATION_PAYLOAD.message)).toBeVisible({timeout: 30000}) - await page.getByLabel("First Name").fill("Ada") - await page.getByRole("button", {name: "Accept", exact: true}).click() - await expect(page.getByText("Provided the requested input.")).toBeVisible({ - timeout: 30000, - }) + let appId = "" + let mock!: Awaited<ReturnType<typeof mockElicitationInvoke>> + + await scenarios.given("the user is authenticated", async () => { + await expectAuthenticatedSession(page) + }) + await scenarios.and("a mocked elicitation run is open", async () => { + appId = await seedAgentChatApp() + await navigateToAgentPlayground(appId) + mock = await mockElicitationInvoke() + mock.setResumeText("Recorded.") + }) + await scenarios.and("the user fills and accepts the form", async () => { + await sendChatMessage("hi") + await expect(page.getByText(ELICITATION_PAYLOAD.message)).toBeVisible({timeout: 30000}) + await page.getByLabel("First Name").fill("Ada") + await page.getByRole("button", {name: "Accept", exact: true}).click() + await expect(page.getByText("Provided the requested input.")).toBeVisible({ + timeout: 30000, + }) + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d046fe70-6190-46c3-be16-6e95c56c6253
📒 Files selected for processing (12)
services/runner/src/protocol.tsservices/runner/tests/unit/client-tools.test.tsweb/oss/src/components/AgentChatSlice/components/clientTools/ElicitationWidget.tsxweb/oss/tests/playwright/acceptance/agent-chat/README.mdweb/oss/tests/playwright/acceptance/agent-chat/assets/elicitationStream.tsweb/oss/tests/playwright/acceptance/agent-chat/assets/types.tsweb/oss/tests/playwright/acceptance/agent-chat/elicitation.spec.tsweb/oss/tests/playwright/acceptance/agent-chat/index.tsweb/oss/tests/playwright/acceptance/agent-chat/tests.tsweb/packages/agenta-entity-ui/src/gatewayTool/components/SchemaForm.tsxweb/packages/agenta-shared/src/utils/gatewayToolSchema.tsweb/packages/agenta-shared/tests/unit/gatewayToolSchema.test.ts
| assert.equal(outcome, "pendingApproval", "a fresh identical call must pause for a new answer"); | ||
| assert.equal(s.events.length, 1, "the client_tool interaction is emitted for the new form"); | ||
| assert.equal((s.events[0] as { kind: string }).kind, "client_tool"); | ||
| assert.deepEqual((s.events[0] as { payload: { render: unknown } }).payload.render, { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove duplicate assertion line.
Line 389 is duplicated — the same assert.deepEqual call appears twice consecutively. This is a copy-paste artifact; the test still passes but the redundant line should be removed.
🧹 Proposed fix
assert.equal((s.events[0] as { kind: string }).kind, "client_tool");
assert.deepEqual((s.events[0] as { payload: { render: unknown } }).payload.render, {
- assert.deepEqual((s.events[0] as { payload: { render: unknown } }).payload.render, {
kind: "elicitation",
});📝 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.
| assert.deepEqual((s.events[0] as { payload: { render: unknown } }).payload.render, { | |
| assert.equal((s.events[0] as { kind: string }).kind, "client_tool"); | |
| assert.deepEqual((s.events[0] as { payload: { render: unknown } }).payload.render, { | |
| kind: "elicitation", | |
| }); |
…io steps - seedAgentChatApp now validates every step's response id (workflow, variant, revision) via a requireId helper, failing fast at the exact malformed step instead of silently returning a broken appId; drops the unsafe `as string` casts. - specs 3 & 4 wrap setup in scenarios.given/and to match specs 1 & 2 (readable BDD report; no behavior change). (CodeRabbit's "duplicate assertion" flag on client-tools.test.ts was a false positive — no duplicate exists; verified. The page.unroute nit is unnecessary — Playwright's per-test isolation discards routes.)
Closes #5190 (AGE-3935). A playbook could not prefill a proposed value — the contract typed `default` away (`default?: never`), so agent-templates worked around it with an enum-first "figure it out" option. The render path already existed: SchemaForm threads `default` into every field's initialValue for its other consumer (gateway forms). The actual changes: - contract: `default?: string | number | boolean`, with a validation rule rejecting non-primitive defaults (stable degradation reason). - request_input description now tells the model to propose defaults (one- click accept) and that enum options are suggestions, not exhaustive — the guidance change that retires the playbook workaround. - golden fixture gains a defaulted enum + boolean field; the pytest golden check pins primitive defaults and requires the fixture to exercise one. - EnumWithOther: an off-options value arriving after mount (a default via Form initialValue, or a replayed draft) opens Other-mode prefilled; the Other input only autofocuses when the user picked "Other…" themselves. - date/date-time fields ignore `default` instead of crashing: a wire default is an ISO string and antd DatePicker requires dayjs. Dates are outside the issue's scope (string/number/integer/boolean/enum).
The flat dialect had no way to express "pick several of these" — arrays
were banned wholesale, so a which-actions-to-enable question could only be
N booleans or a repeated single-select. Admit the ONE canonical JSON
Schema multi-select shape: {type: "array", items: {type: "string",
enum?: [...]}}. String leaves only, nothing deeper; content carries an
array of strings; defaults extend to arrays of strings on these fields.
- contract: array branch in the validator with stable degradation reasons
(items must be strings, no deeper nesting, array-of-strings default).
- renderer (openEnums-gated, elicitation only): a chip picker with the
same "Other…" escape hatch as single-select — picking Other… reveals a
text input that appends one custom chip, repeatable. Enum-less string
arrays degrade to free chip entry. Gateway forms keep their Form.List
arrays untouched (flag-off regression test).
- request_input description documents the multi-pick shape; the golden
fixture gains a defaulted multi-select field, pinned by the pytest
golden check (which now requires the fixture to exercise one).
A bare Select flattens options that need explaining ("merge to main" vs
"GitHub releases" vs "release branches" each deserve a sentence). Adopt
the standard JSON Schema idiom for labeled options — oneOf: [{const,
title, description}] — and render options as selectable cards when any
option carries a description; bare enums keep the Select.
- contract: oneOf admitted on single fields AND array items (multi-pick
cards designed in from the start); parse canonicalizes consts into
enum so downstream never branches on shape; malformed options reject
with a stable reason.
- renderer: ChoiceCards (radio semantics; checkbox when multiple) with
the same "Other…" escape hatch as the Selects — the last card reveals
a text input; custom multi values render as removable tags. Options
with titles but no descriptions upgrade Select labels instead.
- gateway forms unaffected: oneOf is ignored without openEnums (flag-off
regression test — no enumOptions key, no enum promotion).
- request_input description documents the shape; the golden fixture's
frequency field now uses oneOf with descriptions, pinned by the pytest
golden check (which now requires the fixture to exercise oneOf).
…tion-m1-followups
…tion-m1-followups
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/oss/tests/playwright/acceptance/agent-chat/index.ts (1)
254-257: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the spec header comment to align with the one-line guideline.
This 4-line comment explains test rationale rather than describing a surprising constraint, so it doesn't qualify for the brief-exception allowance. Consider collapsing to a single line.
As per coding guidelines: "Keep in-code comments to one short line maximum unless a genuinely surprising constraint requires a brief exception."
♻️ Suggested condensation
- // ── Spec 5: one-click accept — defaults + multi-select + choice cards (full dialect) ───────── - // The agent-templates headline (`#5190`): every field ships a proposed default, so Accept with - // ZERO edits must resume carrying exactly those values. Also pins the card upgrade: oneOf - // descriptions render as choice cards (option titles + descriptions visible), not a Select. + // ── Spec 5: one-click accept — defaults + multi-select + choice cards (full dialect) ───────── + // Every field has a proposed default (`#5190`); Accept with zero edits must resume unchanged. oneOf descriptions upgrade to choice cards, not a Select.Source: Coding guidelines
web/packages/agenta-entity-ui/src/gatewayTool/components/SchemaForm.tsx (1)
444-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChoiceCards: inner Radio/Checkbox creates duplicate focus stops; container lacks group role.
Each card div already carries
role="radio"/role="checkbox",aria-checked,tabIndex, and keyboard handling — but the inner antd<Radio>/<Checkbox>(lines 460, 488) still renders a natively focusable<input>.pointer-events-noneonly suppresses mouse events, not keyboard focus, so tab users hit a second, non-functional focus stop per card. Screen readers also encounter duplicate role information.Additionally, the container
<div className="flex flex-col gap-2">(line 444) has norole="radiogroup"orrole="group", so assistive technology doesn't announce the cards as a related set.♿ Proposed fix: remove inner control from tab order and add group semantics
return ( - <div className="flex flex-col gap-2"> + <div className="flex flex-col gap-2" role={multiple ? "group" : "radiogroup"} aria-label={undefined}> {options.map((o) => ( <div key={o.value} role={multiple ? "checkbox" : "radio"} aria-checked={isChecked(o.value)} tabIndex={disabled ? -1 : 0} onClick={() => pick(o.value)} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault() pick(o.value) } }} className={choiceCardCls(isChecked(o.value))} > - <Control checked={isChecked(o.value)} className="pointer-events-none" /> + <Control checked={isChecked(o.value)} tabIndex={-1} aria-hidden="true" className="pointer-events-none" /> <div className="flex min-w-0 flex-col"> <Typography.Text className="!text-xs font-medium"> {o.label ?? o.value} </Typography.Text> {o.description && ( <Typography.Text type="secondary" className="!text-[11px] leading-snug"> {o.description} </Typography.Text> )} </div> </div> ))} <div role={multiple ? "checkbox" : "radio"} aria-checked={otherActive} tabIndex={disabled ? -1 : 0} onClick={() => { if (!disabled && otherDraft === null) setOtherDraft("") }} onKeyDown={(e) => { if ((e.key === "Enter" || e.key === " ") && otherDraft === null) { e.preventDefault() if (!disabled) setOtherDraft("") } }} className={choiceCardCls(otherActive)} > - <Control checked={otherActive} className="pointer-events-none" /> + <Control checked={otherActive} tabIndex={-1} aria-hidden="true" className="pointer-events-none" /> <div className="flex min-w-0 flex-1 flex-col gap-1">Also applies to: 473-488
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 931cec65-bed9-452c-befe-fc765eecdfc8
📒 Files selected for processing (14)
api/oss/src/core/workflows/static_catalog.pyapi/oss/tests/pytest/unit/workflows/test_static_catalog.pydocs/design/agent-chat-interaction-kinds/decisions.mdweb/oss/tests/playwright/acceptance/agent-chat/assets/elicitationStream.tsweb/oss/tests/playwright/acceptance/agent-chat/index.tsweb/oss/tests/playwright/acceptance/agent-chat/tests.tsweb/packages/agenta-entity-ui/src/gatewayTool/components/SchemaForm.tsxweb/packages/agenta-entity-ui/src/gatewayTool/components/schemaFormOptions.tsweb/packages/agenta-entity-ui/tests/unit/schemaFormOptions.test.tsweb/packages/agenta-shared/src/utils/elicitation.tsweb/packages/agenta-shared/src/utils/gatewayToolSchema.tsweb/packages/agenta-shared/tests/fixtures/elicitation_request.jsonweb/packages/agenta-shared/tests/unit/elicitation.test.tsweb/packages/agenta-shared/tests/unit/gatewayToolSchema.test.ts
✅ Files skipped from review due to trivial changes (1)
- api/oss/src/core/workflows/static_catalog.py
🚧 Files skipped from review as they are similar to previous changes (2)
- web/oss/tests/playwright/acceptance/agent-chat/assets/elicitationStream.ts
- web/oss/tests/playwright/acceptance/agent-chat/tests.ts
…tch constraints, card a11y
Review round 2 on the dialect extensions, verified by execution before fixing:
- A top-level enum/oneOf on an array field (a natural LLM author slip) did
not just pass validation — canonicalization stamped a top-level enum and
the descriptor builder promoted the field to a single-select, silently
corrupting the multi-select. Fix folds misplaced options into items
(declared items win) and strips the top level, so the predictable slip
renders correctly instead of degrading; regression test pins parse→build
staying {type: array, multiple: true}.
- Scalar constraints now validate against the declared type: enum/oneOf
require type "string"; default must match the type (integer ⇒ whole
number). Stable per-type degradation reasons replace the loose
"must be a primitive" check.
- ChoiceCards a11y: the nested antd Radio/Checkbox kept a native focusable
input inside role-carrying cards (duplicate tab stop + duplicate role per
card; pointer-events-none does not remove keyboard focus), and the group
had no radiogroup/group role. The card is now the single interactive
element with an aria-hidden visual indicator; the container carries
radiogroup/group and accepts Form.Item's injected id.
- Condensed the spec-5 header comment to the one-line house rule.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…y defaults stripped
Live scenario-1 testing (all-optional prefilled form) surfaced two bugs:
- All-optional fields collapse behind "Optional (N)", and antd Collapse
does not render inactive panels — the Form.Items never registered, so a
one-click Accept silently returned EMPTY content, dropping every
proposed default. Fix: forceRender on the optional panel (defaults
register while collapsed; benefits gateway forms too), and when there
are no required fields render optional fields inline — the collapse
de-emphasizes extras below required fields, and with none there is
nothing to de-emphasize.
- A model emitting default: "" (its way of saying "no proposal") passed
the string type check and mounted enum fields in Other-mode with an
empty input. Parse now strips empty defaults ("" and []), and
isOffOptionsValue treats "" as unset (defense for replayed drafts).
… required error - Selected choice card dropped the full primary-bg fill (a heavy olive slab in the dark theme); selection reads from the primary border + the filled indicator. - Picking "Other…" emitted a no-op onChange(undefined), firing the required rule the instant the option was chosen — before the user could type. Both Selects now skip the no-op change; validation still fires on Accept.
mmabrouk
left a comment
There was a problem hiding this comment.
Thanks @ardaerzin very much needed
Typed field values lived only in antd Form state, so reloading while a form was pending lost everything the user had entered (the message parts persist; the draft never did). The widget now persists the draft to localStorage keyed by the toolCallId on every change, restores it on mount over the schema defaults, and clears it on any settle (accept/decline/dismiss). - SchemaForm gains an onValuesChange passthrough (raw values on purpose: cleanFormValues recurses into and destroys dayjs objects and JSON.parses typed strings — wrong for a draft snapshot). - Date fields round-trip via partitionElicitationDraft (@agenta/shared, tested): persisted ISO strings are revived to dayjs on restore, since DatePicker rejects strings.
…tion-m1-followups
The Agenta-AI#5190 follow-up: the elicitation form now has a real default field (shipped in Agenta-AI#5177), so the playbook layer retires its two workarounds — "put the recommended value in the field description" and "the FIRST enum option signals the default". - All 28 template playbooks migrated: proposed values become field defaults (one-click accept); researchable enums set "figure it out" AS the default and lean on the built-in Other… escape hatch; fields that already implied multiplicity use the multi-select array shape. Every body stays within its 1-2 KB budget. - write-template-playbooks skill: platform fact #1 flipped (defaults exist — use them; type-matched, empty stripped, dates ignore them), skeleton + checklist + worked example updated. - playbook-spec: "The request_input reality (no defaults)" replaced with the shipped dialect (defaults, multi-select, oneOf choice cards); open-questions #2 closed as shipped-and-adopted. - build-an-agent skill loop step 1 now tells the builder to propose defaults and use the richer shapes at the decisive prompt position.
Context
Follow-ups to the merged elicitation M1 (#5155). That PR shipped the feature verified but deferred automated end-to-end coverage, and dogfooding since surfaced one UX gap. This bundles both: the emit-harness test coverage M1 left open, and a small elicitation UX fix. All three changes touch the same seam.
What this adds
1. "Other…" custom value on elicitation enum fields (feat). An enum in an elicitation form rendered as a strict dropdown, so the user was locked into the agent's suggested options. But the consumer of the answer is the agent (an LLM), so those options are suggestions, not a hard constraint — the user should be able to go off-menu. Enum fields now render with an "Other…" entry that reveals a free-text input.
openEnumsbuild option in@agenta/sharedstampsallowCustomEnumonto enum descriptors;SchemaFormrenders the escape hatch;ElicitationWidgetopts in.enum, only the rendering changes, and the answer already rides as free-formcontent.2. Runner client-tool relay integration tests (layer B). History-driven tests that drive
buildClientToolRelaywith the realApprovalResponderbuilt from run history — the exact chain a live/runexecutes (extract → ConversationDecisions → ApprovalResponder → relay). The mock-responder tests already pin the relay's mechanics; these pin the whole emit/pause/resume decision end to end, where the cross-turn coalescing bug lived. Verified red without the current-turn scoping fix, so it's a real regression guard. Also admitsrequest_input'selicitationrender in the runner's TS-onlyRenderHintunion (mirrors the existingconnectmember; render rides as an opaque dict on the wire, so type-only).3. Elicitation E2E scaffold with an SSE transport mock (layer A). A deterministic Playwright acceptance spec for the elicitation kind that mocks only the agent run (
page.routeon**/invoke*) with byte-accurate AI SDK v6 SSE, keeping auth, project, the seeded agent revision, and the playground shell real. This closes the "no automated E2E" gap without a live LLM and without the mock-provider's client-tool-pause unknown.assets/elicitationStream.ts: canned SSE builders (paused elicitation turn + resume turn), pinned against the Python vercel adapter wire.tests.ts: the transport mock, anis_agentapp seed (data.uri = agenta:builtin:agent:v0), navigation, composer helper.index.ts: round-trip, required-field gate, settled replay, reload-while-pending.Tests
@agenta/shared: 258 pass — including the newopenEnumstests and the gateway-forms-stay-strict regression guard.services/runner: 719 pass — the 2 new integration tests coexist with the merged session-keepalive suites;tscclean.tsc --noEmitclean across@agenta/shared,@agenta/ui,@agenta/entities,@agenta/entity-ui.Not in this PR / first-run pending
The layer-A specs are authored and load in the harness (
--listgreen) but have not had a first green run against a live stack — the browser + dev server were flapping during authoring. The composer/field selectors and the reload rehydration source (a mocked run records no server-side transcript) are flagged in-file to confirm and adjust on that first run. Draft until then.What to QA
request_inputwith an enum field. The dropdown now shows an "Other…" entry; picking it reveals a text box, and the typed value comes back on Accept and flows to the agent.