feat(runtime): establish durable form interactions - #4379
Conversation
be4056c to
ed745fb
Compare
032d77f to
d8bae9f
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
PR #4379 — d8bae9f — blind review sealed (stack base)
Summary: Durable form interaction baseline (Runtime/Core/Storage/Runtime Host). Exact head d8bae9fffaa501b5fa0de383371ece11421bc3a2 frozen, 0 checks/0 reviews/0 threads on stacked base; current-main da84f88de merge has only the explicitly ignored protocol epoch 83→84 vs 83→85 conflict (per @me2seeks). Source is approvable with comments; no simplify finding. Full build passed; focused SQLite/auth tests 208/208 green.
Findings (reproducible, decision-changing only):
- P2 — unsafe projection
tool formtext —formmessage/requester/field/optiontext is only byte-bounded, not reused Interaction safe projection. Probe withU+202E + sk-live-…persists and projects verbatim in the form path, whilequestionpath escapes bidi and redacts — untrusted provider can spoof source or leak secret. Fix: reuse existing safe projection/redaction on form rendering. - P2 —
answer/outcomecap mismatch — both 8 KiB caps, butoutcomeadds 4 required strings +min=max=2025wrapping. A form admitting exactly 8,172-byte answer is legal, wrapped as canonical outcome 8,195 bytes is rejected by store codec → pending Tool can neveraccept, onlydecline/cancel. Fix: reserve outcome overhead from answer cap or make admission account for wrapping. - P3 —
date-timevalidation —date-timeuses regex +Date.parse;2023-02-30T00:00:00Zis accepted and normalized to Mar 02 by canonical Host validator. Fix: strict calendar validation.
Gating: exact-head hosted checks green (2/2), no approval/review threads. Stack inheriting risk noted for #4384/#4392/#4397.
Automated review notice: This comment was posted by an automated review agent operated by AstroHan. It is not an independent human review and does not replace one.
简体中文
本条结论来自 @捣蛋鬼 在 exact head d8bae9f 的独立盲审,已按 @me2seeks 指示排除 compatibility epoch 冲突的计分。我作为编排仅核对 head 未漂移与 exact-head CI 状态,未替代独立审查。
d8bae9f to
1fba905
Compare
|
Addressed all three findings in 1fba905: form-facing text now goes through the shared safe projection/redaction boundary (including projected-label collision checks), accepted answers are bounded against the actual canonical outcome envelope, and date/date-time validation rejects normalized invalid calendar values. Added focused regressions for each case. I also rebased the stack onto current main (920d714). |
1fba905 to
4609622
Compare
|
Rebased onto current main (afbcabd). Main's catalog protocol change already occupies epoch 87, so the form interaction contract now advances to epoch 88; both compatibility notes remain in the ledger. Re-ran the 108 interaction/protocol checks on the rebased head with an isolated writable test root. |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed a12633d2. I spent most of the time trying to break the claim that this reuses the existing Interaction authority instead of growing a parallel one, and on the Host side I couldn't: #answerQuestion → #answerStoredInteraction, #requireLiveQuestion → #requireLiveStored, #commitAnswer widening to LiveStoredEntry, #requireLiveClientCapability folding into the shared path, InteractionStore picking up 12 lines inside the existing branch. That's real generalization. Concurrency convergence and continuation exactness also hold — I traced and measured both.
The findings are all one thing, so I'll describe it once.
The root: "which interaction kinds exist" is stored 18 times, as allow-lists with silent fallthrough
18 production sites dispatch on user_question_request. Six learned about form; twelve didn't. What separates them isn't care — it's whether the compiler could speak.
The one site that had to be updated, session-event-runtime-mapper.ts, is the only one of the six ending in:
const _exhaustive: never = event;Adding FormRequestEvent to SessionEvent made that a compile error. Everything that was missed looks like the opposite — a silent default: (statusFromEvent, reduceInteractionQueues, both stream-graph-*) or a hand-maintained positive list (runtime-kernel.ts:1591 and :3335). Add a kind and each is quietly wrong, and nothing says so.
Desktop and CLI surfaces (7 sites) are reasonably deferred to #4384/#4392. These five are this PR's own layer:
| site | consequence for form |
|---|---|
runtime-kernel.ts:1591 assertInteractionPublication |
the publication linearization point never runs; tracked.published stays false, so close() blocks on Promise.all(publicationBarriers) (interaction-authority.ts:447) |
runtime-kernel.ts:3335 interactionResumeAllowed |
never runs — which makes the form_answer_ack branch this PR added to settlementMatchesAck dead code |
session-projection-helpers.ts:118 statusFromEvent |
session reads running the whole time it is blocked on the user |
stream-graph-{projection,read-model}.ts |
a supervisor can't see a child blocked on a form |
ui/interaction-queue.ts:68 |
no form case, but reconcileInteractions takes whatever listActiveInteractions returns — which session-projector.ts now includes forms in |
The first row is the one that bothers me: the mechanism this PR exists to establish is switched off for the kind it introduces, and runtime-kernel.ts isn't in the diff at all. The last row plus chat-composer-region.tsx is concrete: a form becomes activeInteraction, hidden={… || Boolean(activeInteraction)} hides the composer, none of the three narrowing checks match, so nothing renders in its place.
The same shape recurses inside functions — details inline: projectInteractionFormRequest projects a list of fields rather than every displayable string (field.name, field.default, option.value keep raw ESC bytes); assertFormHasAcceptedAnswer witnesses required fields at their lower bound, so it proves "some answer fits" where admission needs "every legal answer fits"; and tool-runtime.ts adds 212 lines with 2 new kind discriminants where interaction-projection.ts adds 36 with 9 — copying doesn't need branches.
The repair that matches the cause
Not twelve new form branches. That's the same manual sweep again, and the sixth kind pays it a third time — a simplification pass alongside this review put the cost of adding one at ~45 edit sites. Instead, let the compiler do this sweep and every future one, using the idiom already in a file this PR touches:
- Replace the silent
default:instatusFromEvent,reduceInteractionQueuesand the twostream-graph-*projections withconst _exhaustive: never = event. Sites that genuinely care about a few event types should narrow their parameter instead — that narrowing is the work thedefaulthas been hiding. - Derive
assertInteractionPublicationandinteractionResumeAllowedfrom one table of which kinds are hosted interactions, rather than two lists that have to agree by hand. - Project every displayable string by construction, and witness the upper bound.
What disappears is the obligation: the sixth kind becomes a table entry plus whatever the compiler then points at. That's also the honest answer to what this change made redundant — right now it adds 1365 production lines and removes none, and the best evidence the pattern has passed its useful point is that this PR itself missed twelve sites.
Grading
None of this is reachable today — requestUserForm has no production caller (only two test files, against askUserQuestion's two production tools and a response path implemented down to desktop IPC). Every finding is a seam gap that goes live when a producer lands, which is why they're P2 and not higher, and why I'd fix them here rather than in a child PR that would otherwise have to switch on the root's own invariant. Specifically on the publication one: with a producer, immediate stop mode still reaches the right final state because ai-sdk-backend.stop() aborts the scope and finalize() seals publications — inverted ordering resting on an unrelated subsystem, not breakage. The hang needs after_step, which no client sends, so I'm not grading on it.
Two P3s: the local respondToUserForm path can't be reached in either configuration (backend-types.ts has no form twin, and the only production new SessionManager( always passes interactionAuthority, which makes the embedded responders throw), so ~95 production lines and the 200-line tool-runtime-form-interaction.test.ts cover a path that can't ship. And the nine INTERACTION_FORM_* constants have no consumer outside interaction.ts, four duplicating existing constants at the same value and meaning (FIELD_LABEL ≡ OPTION_LABEL, FIELD_DESCRIPTION ≡ OPTION_DESCRIPTION, FORM_VALUE ≡ ANSWER, FORM_REQUESTER_NAME ≡ INTERACTION_TOOL_NAME).
Coordination note: #4184 also sets RUNTIME_HOST_COMPATIBILITY_EPOCH = 88. And AgentGraphSupervisorAttentionReason is a closed union, so adding form_request to the supervisor signals later costs another bump — doing it now spends this one once instead of twice.
Evidence boundary: read against afbcabdc74 (#4433), the true base — a stale local main makes session-transcript-pager.ts appear in the diff and it is not part of this PR. Decode, projection, witness and convergence results are from running this branch's built @maka/core; the publication-barrier behaviour was reproduced by driving the real RuntimeInteractionRunBinding with the kernel's predicate copied verbatim, not through a live kernel and Host — there's no producer to drive one with. The 18-site count is a grep for user_question_request, so a site dispatching by some other spelling wouldn't appear. All four touched suites pass on this head (135 tests, 0 failures). I didn't review #4384/#4392/#4397.
AI-assisted review: drafted with Maka; I verified the exhaustiveness asymmetry, the 18-site count, the publication predicate, the projection gaps and the witness measurements against the branch source myself.
| }; | ||
| } | ||
|
|
||
| private async requestUserForm( |
There was a problem hiding this comment.
P3 — the third copy of one mechanism, and the discriminant count shows it.
Against main:
| file | added | kind discriminants |
|---|---|---|
interaction-projection.ts |
+36 | 20 → 29 |
interaction-coordinator.ts |
+122 | 40 → 46 |
interaction-authority.ts |
+112 | 10 → 15 |
tool-runtime.ts |
+212 | 54 → 56 |
Generalizing adds branches to absorb a kind; copying doesn't need any. requestUserForm (95 lines) and askUserQuestion (99) are the same control flow line for line — throwIfAborted, interactionRun(), park, the onAbort closure, if (hostedRun) void parked.catch, createXSettlement, admitXRequest, racePromiseWithAbort, ack, finally — differing only in payload construction and type names. settleUserFormAnswer, closeUserForm, finishDeferredFormTurnClosure and createFormSettlement are each their question twin too, so there are now three parallel registries and three deferred-closure flags whose shapes differ only in payload type.
P3 because it isn't a defect — the Host side really did generalize, and this is the one place that didn't. But it's the concrete reason twelve missed sites were possible: when each kind carries its own copy, nothing forces a new kind through a single seam where the gaps would show.
Related: requestUserForm, respondToUserForm, pendingUserFormCount and the new decodeInteractionFormResponse export have no production callers, and the non-hosted half can't be reached in either configuration. Extracting one kind-generic parking mechanism, or dropping the unreachable local path (~95 production lines plus the 200-line test that only exercises it), each leaves less here than there is now. If this layer is meant to land deliberately ahead of its producer, saying so in the description would help — a reader can't currently tell an omission from a plan.
Define a bounded provider-neutral primitive form contract and carry its request and acknowledgement facts through the Runtime Event Log. Broker pending forms through the existing InteractionStore authority so schema-invalid answers remain pending, concurrent equivalent answers converge on one canonical outcome, and Turn closure or Host restart closes the exact continuation. Part of #4364. Generated-by: OpenAI Codex
Expose one closed decoder for renderer-to-runtime form responses so surface adapters do not copy protocol validation. Queue the same canonical continuity refresh for form requests that user questions already receive. Refs #4364. Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
a12633d to
066718d
Compare
|
Addressed the form-interaction P2 follow-up in 066718d. Form requests and answer acknowledgements now share the hosted-interaction predicates used by Kernel publication and resume. I also added their session/stream-graph projections, projected string defaults through the existing review-text boundary, and made admission reserve the full legal answer envelope—including optional fields—so a valid submission cannot fail later at persistence. The rebase retains mains catalog epoch 88 and moves the form contract to epoch 89. Focused Core and Runtime suites pass 57/57. |
Generated-by: OpenAI Codex
|
Followed up on the remaining Desktop queue gap in 562b832. The parent PR now keeps the composer queue explicitly limited to request kinds this surface can render and settle; a form remains authoritative in Runtime Host, but can no longer hide the composer with no prompt in its place. Rehydration applies the same boundary. The actual form renderer and responder stay in sibling #4384, which widens this surface boundary together with the prompt. That avoids making #4379 claim a Desktop capability it does not yet provide. UI queue, Desktop typecheck, and the focused Core/Runtime suites pass (64 tests total across those suites). |
|
Read
The build is red on this head, and it's the closed union I mentioned.
One epoch note while you're there: main is on 88 as of #4460, and #4184 is also sitting on 89, so whichever of you merges second will need to move again. The one piece I'd still mention, not as a blocker: Happy to re-approve once the build is green. |
Summary
Establishes the provider-neutral Runtime Interaction foundation for structured form requests without introducing an MCP-specific authority.
ToolRuntimeand brokers hosted requests through the existing Runtime Host Interaction owner.InteractionStoreremains the sole pending/outcome authority: invalid answers leave the request pending, equivalent concurrent answers converge on one canonical result, and only that result resumes the captured continuation.Refs #4364 (rollout PR 1).
Rollout
This PR intentionally has no MCP request handler or user-facing renderer. Follow-up PRs will add Desktop/TUI form surfaces and then adapt MCP
elicitation/createonto this authority; neither follow-up may introduce a second pending-request or winner state.Verification
npm run buildnpx biome checkon all 28 changed filesgit show --check HEADReview focus
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex analyzed the existing Interaction authority, implemented the form contract and lifecycle, added tests, and performed separate correctness/lifecycle and architecture/ownership review passes. The commit contains the required
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?