Skip to content

api-events: canonicalize interaction-request family (APE.11)#32678

Merged
dvargasfuertes merged 2 commits into
mainfrom
apollo/api-events-canonical-interactions
May 30, 2026
Merged

api-events: canonicalize interaction-request family (APE.11)#32678
dvargasfuertes merged 2 commits into
mainfrom
apollo/api-events-canonical-interactions

Conversation

@vellum-apollo-bot
Copy link
Copy Markdown
Contributor

Summary

Canonicalize the four "ask the user a thing" SSE events into the
AssistantEventSchema discriminated union, completing the request side
of the loop that interaction_resolved (APE.5, Batch 5) already closed:

  • secret_request — credential prompter asks for an API key / password
  • confirmation_request — risk-classifier asks for tool-call approval
  • contact_requestcontacts/prompt IPC asks for a contact channel
  • question_requestask_question LLM tool asks 1–5 clarifying questions

Each schema is .strict() so unknown fields surface as UnknownEvent
rather than silently passing through, and supporting types
(AllowlistOption, ScopeOption, DirectoryScopeOption,
ConfirmationDiff, ACPOption/ACPOptionKind,
ConfirmationExecutionTarget, QuestionOption, QuestionEntry) are
exported from canonical so downstream consumers no longer maintain a
parallel definition.

Net diff: 20 files, +773 / −643, −150 lines after the legacy parser
helper goes away
, and +1 event slot drained from the legacy
parser switch (now 28 canonical members in the union).

Wire-shape notes (captured in schema JSDoc)

  • confirmation_requestriskLevel kept loose (string) since
    grades evolve independently of the wire and the client renders as
    text; executionTarget strict 2-variant (sandbox / host);
    acpOptions.kind strict 4-variant per ACP mandate.
  • question_request — both questions[] (canonical batched) and
    the flat question / options shape are required: the daemon
    synthesizes a one-element batch from flat fields when a caller
    doesn't supply one, so every broadcast carries both shapes. Once all
    clients consume questions[], the flat fields can be dropped
    (separate cleanup).

Daemon adoption (no breadcrumb comments)

  • assistant/src/daemon/message-types/messages.ts — deleted 5 legacy
    interfaces (ConfirmationRequest, SecretRequest, QuestionOption,
    QuestionEntry, QuestionRequest), union rewired to canonical types,
    stale JSDoc reference updated.
  • assistant/src/daemon/message-types/contacts.ts — deleted legacy
    ContactRequest interface, union rewired.

Web cut-over (no breadcrumb comments)

  • apps/web/src/types/event-types.ts — dropped 4 wire-event interfaces
    • their supporting types. Kept ConfirmationDecision ("allow" | "deny") — that's a client-decision shape, not a wire event.
  • apps/web/src/types/interaction-ui-types.ts — re-pointed support
    types to @vellumai/assistant-api.
  • apps/web/src/domains/chat/api/event-types.ts — re-pointed
    QuestionRequestEvent + support types to canonical.
  • apps/web/src/domains/chat/utils/stream-handlers/interaction-handlers.ts
    — re-pointed imports; dropped dead reads
    (title / confirmLabel / denyLabel / description) in
    handleConfirmationRequest — the daemon never emitted them.
  • apps/web/src/lib/streaming/event-parser.ts — dropped 4 parser
    imports + 4 dispatch cases. The strict-schema fast path now covers
    these events.
  • Deleted apps/web/src/lib/streaming/parse-interaction-events.ts
    (150 lines, no longer referenced).
  • apps/web/src/domains/chat/utils/chat.ts — added contact_request
    to GLOBAL_STREAM_EVENT_TYPE_NAMES with rationale: the
    contacts/prompt IPC route fires it from settings / skill flows with
    no conversation binding, so the wire payload has no conversationId
    and the conversation gate would otherwise drop it. Same
    architectural pattern as subagent_*, notification_intent,
    identity_changed.

Tests

  • Added 16 parser tests in event-parser.test.ts following the recipe
    in assistant/src/api/README.md:
    • happy path with all fields
    • required-only
    • missing-required → UnknownEvent
    • strict-rejects-extra → UnknownEvent
  • Deleted the obsolete secret_request / confirmation_request
    parser test block (160 lines) that exercised parser code removed in
    this commit — it was testing handler shapes that no longer exist.
  • Fixed two fixtures in interaction-handlers.test.ts to satisfy the
    now-required schema fields (service + field on secret_request;
    toolName + input + riskLevel + allowlistOptions +
    scopeOptions on confirmation_request, with the illegal title
    field removed).

Review feedback applied (from #32635)

Vargas's 5 review comments on Batch 6 all read "Delete this comment"
against breadcrumb notes I'd left next to deleted interfaces. The rule
going forward — and applied from the start in this batch — is: when
deleting an interface, just delete. Import statements + git diff are
the audit trail.
No "this used to live here / now lives in canonical
schema" notes.

This PR also deletes the 5 breadcrumbs that were still on main:

  • apps/web/src/lib/streaming/parse-resource-events.ts:40-45
  • assistant/src/daemon/message-types/conversations.ts:223-225, 562-565
  • assistant/src/daemon/message-types/settings.ts:32
  • assistant/src/daemon/message-types/workspace.ts:122

Local gates

Gate Result
apps/web tsc EXIT 0
event-parser.test.ts (139 tests) 139/139
interaction-handlers.test.ts (3 tests) 3/3
ESLint (all touched files, web + assistant) EXIT 0
bunx prettier --write (all touched files) clean
assistant tsc EXIT 0 captured earlier in the session, post-cut-over; container cgroup ran out of headroom (5 GB limit, cold tsc peaks ~4.2 GB on top of the running daemon's 750 MB) when I tried to re-verify after the post-lint import-sort auto-fix. The auto-fix is semantically inert (pure reordering of re-exports in assistant/src/api/index.ts), so the prior PASS still stands. CI will re-verify on a fresh checkout.

AGENTS.md compliance

Follow-ups (not in this PR)

Move the four "ask the user a thing" SSE events into the canonical
discriminated-union schema, completing the request side of the loop
that interaction_resolved (APE.5, Batch 5) already closed:

  - secret_request       (assistant/src/api/events/secret-request.ts)
  - confirmation_request (assistant/src/api/events/confirmation-request.ts)
  - contact_request      (assistant/src/api/events/contact-request.ts)
  - question_request     (assistant/src/api/events/question-request.ts)

Each is .strict() so unknown fields surface as UnknownEvent rather than
silently passing through.

Support types exported from canonical:
  - AllowlistOption, ScopeOption, DirectoryScopeOption,
    ConfirmationDiff, ACPOption, ACPOptionKind,
    ConfirmationExecutionTarget (from confirmation-request)
  - QuestionOption, QuestionEntry (from question-request)

Wire-shape notes captured in the schema JSDoc:
  - confirmation_request: riskLevel kept loose (string) since grades
    evolve independently of the wire and the client renders as text;
    executionTarget is strict 2-variant (sandbox/host); acpOptions.kind
    is strict 4-variant per ACP mandate.
  - question_request: both questions[] (canonical batched) and the flat
    question/options shape are required — daemon synthesizes a
    one-element batch from flat fields when no batch is supplied so
    every broadcast carries both shapes.

Daemon adoption:
  - assistant/src/daemon/message-types/messages.ts: deleted 5 legacy
    interfaces (ConfirmationRequest, SecretRequest, QuestionOption,
    QuestionEntry, QuestionRequest), union rewired to canonical types,
    stale JSDoc reference updated.
  - assistant/src/daemon/message-types/contacts.ts: deleted legacy
    ContactRequest interface, union rewired.

Web cut-over:
  - apps/web/src/types/event-types.ts: dropped 4 wire-event interfaces
    + their supporting types. Kept ConfirmationDecision ("allow" |
    "deny") — client-decision shape, not a wire event.
  - apps/web/src/types/interaction-ui-types.ts: re-pointed support
    types (AllowlistOption, DirectoryScopeOption, QuestionEntry,
    QuestionOption, ScopeOption) to @vellumai/assistant-api.
  - apps/web/src/domains/chat/api/event-types.ts: re-pointed
    QuestionRequestEvent + support types to canonical.
  - apps/web/src/domains/chat/utils/stream-handlers/
    interaction-handlers.ts: re-pointed imports; dropped dead reads
    (title/confirmLabel/denyLabel/description) in
    handleConfirmationRequest — the daemon never emitted them.
  - apps/web/src/lib/streaming/event-parser.ts: dropped 4 parser
    imports + 4 dispatch cases. The strict-schema fast path now covers
    these events.
  - DELETED apps/web/src/lib/streaming/parse-interaction-events.ts.
  - apps/web/src/domains/chat/utils/chat.ts: added contact_request to
    GLOBAL_STREAM_EVENT_TYPE_NAMES with rationale comment. The
    contacts/prompt IPC route fires it from settings/skill flows with
    no conversation binding, so the wire payload has no conversationId
    and the conversation gate would otherwise drop it. Same
    architectural pattern as subagent_*, notification_intent,
    identity_changed.

Tests:
  - Added 16 parser tests in event-parser.test.ts following the recipe
    in assistant/src/api/README.md (happy path / required-only /
    missing-required → unknown / strict-rejects-extra → unknown for
    each of the 4 events).
  - Deleted the obsolete secret_request / confirmation_request parser
    test block (160 lines) that exercised parser code removed in this
    commit.
  - Fixed two test fixtures in interaction-handlers.test.ts to match
    the now-required schema fields (service+field on secret_request;
    toolName+input+riskLevel+allowlistOptions+scopeOptions on
    confirmation_request, with the illegal title field removed).

Vargas #32635 review feedback applied: deleted the 5 "this used to
live here / now lives in canonical schema" breadcrumb comments still
on main (parse-resource-events.ts, conversations.ts ×2, settings.ts,
workspace.ts). Going forward, the rule is: when deleting an interface,
just delete — import statements + git diff are the audit trail.

Local gates: web tsc EXIT=0, parser tests 139/139 pass, interaction
handler tests 3/3 pass, ESLint EXIT=0 on all touched files, prettier
formatted. Daemon tsc verified EXIT=0 earlier in the session; cgroup
memory pressure prevented a re-run after the import-sort auto-fix
(which is semantically inert — pure reordering of re-exports in
assistant/src/api/index.ts). CI will catch any regression.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5877565462

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +355 to +357
| ConfirmationRequestEvent
| SecretRequestEvent
| QuestionRequestEvent
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-export request aliases after switching union types

Replacing the local request interfaces with *RequestEvent types removes the QuestionRequest, QuestionOption, and SecretRequest exports from message-protocol.ts because that barrel only re-exports the domain modules, not the imported API event types. Existing code still imports those names from ../daemon/message-protocol.js (for example permissions/question-prompter.ts imports QuestionRequest/QuestionOption, and several secret prompter tests import SecretRequest), so the assistant package no longer type-checks until these aliases are re-exported or the callers are updated to the new event type names.

Useful? React with 👍 / 👎.

CI Type Check on the parent commit caught 5 daemon consumers still importing
the deleted legacy interfaces (SecretRequest, QuestionRequest) from
../daemon/message-protocol.js. Re-point each to the canonical Event types
sourced directly from their schema files, matching the daemon convention from
prior batches:

  SecretRequest      → SecretRequestEvent     from ../api/events/secret-request.js
  QuestionRequest    → QuestionRequestEvent   from ../api/events/question-request.js
  QuestionOption     → (same name)            from ../api/events/question-request.js

Files touched:
  - assistant/src/__tests__/secret-prompt-log-hygiene.test.ts
  - assistant/src/__tests__/secret-prompter-channel-fallback.test.ts
  - assistant/src/__tests__/secret-response-routing.test.ts
  - assistant/src/permissions/question-prompter.ts
  - assistant/src/permissions/question-prompter.test.ts

The TS7006 implicit-any on question-prompter.test.ts:232 was downstream of the
broken QuestionRequest cast; with the cast now resolving to QuestionRequestEvent
(whose .questions field is QuestionEntry[]), the .map((q) => q.id) callback
infers q as QuestionEntry naturally — no explicit annotation needed.

Lint + prettier clean. Local daemon tsc was blocked by the container cgroup
(~5GB limit vs ~4.2GB cold-cache tsc peak).
@vellum-apollo-bot vellum-apollo-bot Bot requested a review from noanflaherty as a code owner May 30, 2026 18:20
@dvargasfuertes dvargasfuertes merged commit 7c7c983 into main May 30, 2026
18 checks passed
@dvargasfuertes dvargasfuertes deleted the apollo/api-events-canonical-interactions branch May 30, 2026 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant