Skip to content

refactor(runtime-host): use typed request as sole operation API - #3784

Merged
likun666661 merged 2 commits into
mainfrom
refactor/runtime-host-typed-request-api
Aug 25, 2026
Merged

refactor(runtime-host): use typed request as sole operation API#3784
likun666661 merged 2 commits into
mainfrom
refactor/runtime-host-typed-request-api

Conversation

@likun666661

Copy link
Copy Markdown
Member

Summary

  • Make typed request<K>() the sole direct Runtime Host operation surface on direct and reconnecting connections.
  • Remove 17 forwarding aliases and migrate Runtime Host, CLI, and Desktop direct callers to protocol operation keys.
  • Preserve validated status(), subscriptions, capabilities, listeners, lifecycle, and close() behavior.

Implements the scoped concept-reduction candidate from Discussion #3618.

Verification

  • npm run build
  • npm run lint
  • npm run format:check
  • npm --workspace @maka/runtime-host test (1,144 passed)
  • npm --workspace maka-agent run test:dist (453 passed)

Breaking change

Direct RuntimeHostConnection consumers must call the typed operation API, for example connection.request('turn.start', input), instead of operation-specific forwarding aliases. Dedicated lifecycle and streaming APIs remain unchanged.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex implemented the API reduction, migrated callers, and ran verification. The commit includes a Generated-by trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this head. No Spec blocking issues, but a hard governance breach blocks approval.

[P1] Breaking API change implemented before governance decision settled

The PR removes 17 exported RuntimeHostConnection methods (documented breaking change) citing Discussion #3618, which is still "proposed/not approved" until 2026-08-26. CONTRIBUTING.md:28 requires dev@maka.apache.org discussion before implementation. No dev-list decision record is linked.

Fix: hold/return to draft, settle on dev list, then implement the accepted decision.

Checks on 5f1d2b7291 are test: queued / package: failure (installer harness flake) — not green.

简体中文存在治理流程硬性阻断,需先经邮件列表决策。

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

I reviewed this head and found no other correctness or design issues. One important boundary remains:

[Important] Make validated status() the only client status surface

DirectRequestOperationKey still includes host.status, so callers can use connection.request('host.status', {}) and bypass the Host identity correlation performed by connection.status(). The reconnecting implementation also routes status() through #request()connection.request(), bypassing that validation.

A structurally valid response with a mismatched hostEpoch, compositionId, or compositionRevision can therefore be accepted. This also leaves two status contracts despite the PR’s goal of preserving validated status() as a dedicated boundary.

Please make status() the only supported client status surface:

  • exclude host.status from DirectRequestOperationKey;
  • reject host.status at the public request() boundary at runtime;
  • have direct status() use the private connection-scoped request pipeline and the canonical Host identity validator;
  • have reconnecting status() preserve query retry by dispatching through the underlying connection.status().
简体中文

我审查了当前提交,没有发现其他 correctness 或 design 问题。目前只剩一个重要边界需要处理:

[Important] 让经过校验的 status() 成为唯一的客户端状态入口

DirectRequestOperationKey 仍然包含 host.status,因此调用方可以使用 connection.request('host.status', {}),绕过 connection.status() 所执行的 Host 身份相关性校验。重连实现中的 status() 也通过 #request()connection.request() 派发,同样绕过了该校验。

因此,只要响应结构合法,即使其中的 hostEpochcompositionIdcompositionRevision 与当前连接不一致,也可能被接受。这也使系统继续维护两套状态调用契约,与本 PR 将经过校验的 status() 保留为专用边界的目标不符。

建议让 status() 成为唯一受支持的客户端状态入口:

  • DirectRequestOperationKey 中排除 host.status
  • 在公开 request() 的运行时边界拒绝 host.status
  • 让直连 status() 使用私有、connection-scoped 的请求管线和统一的 Host 身份校验;
  • 让重连 status() 通过底层 connection.status() 派发,在保留 query retry 的同时保留身份校验。

@likun666661

Copy link
Copy Markdown
MemberAuthor

Addressed the validated status boundary review in a4336ea86:

  • excluded host.status from DirectRequestOperationKey;
  • reject runtime attempts to bypass status() through request();
  • route direct status() through the private connection-scoped request pipeline and one Host identity validator shared with liveness probes;
  • route reconnecting status() through each underlying connection's validated status() while retaining query retry.

Added coverage for type/runtime bypass rejection, mismatched Host identity failure, and reconnect retry without falling back to request().

Verification:

  • root build, lint, and format check pass;
  • affected Runtime Host suites: 45/45 pass;
  • full Runtime Host run passed 1,144 tests and hit only the existing generation-mismatch cache-cleanup flake (ENOTEMPTY) in one test; that test passed immediately in isolated rerun.

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Follow-up review on a4336ea86: the validated status() boundary finding is fully addressed, and I found no other code issues. Approved; merge remains subject to required checks and the Discussion #3618 feedback window.\n\n

简体中文复审确认此前的 status() 校验边界问题已完整修复,未发现其他代码问题。批准;合并仍需等待 required checks 与 Discussion #3618 反馈期结束。

@Astro-Han

Astro-Han commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

The direction is right and the alias removal is thorough. Neither note below blocks the merge — one thing just works against the PR's own claim.

[P3] Making host.status an exception costs more than the alias it removed

Three additions exist only to carve host.status out of the typed surface:

  • DirectRequestOperationKey now excludes it
  • request() rejects it at runtime via isHostStatusOperation
  • because #request can no longer carry it, reconnecting-connection.ts:161-173 hand-rolls a reconnect loop

That third one is the real cost. It is character-for-character the query branch of #request (:258-266) — same requestDeadline, same #waitForConnection, same isRetryableQueryInterruption, same previous handoff — and openSessionSubscription (:181-190) is the same shape again with a different predicate. The loop was already duplicated before this PR; now there are three copies, plus 62 lines of new test to cover the one just added.

Two ways out, and I'd take the first:

  1. Let request('host.status', …) route to the validated path inside the direct connection instead of rejecting it. The exclusion, the runtime guard, the third loop, and its test all disappear, and status() on the reconnecting side goes back to one line.
  2. If the exception stays, fold all three into #withReconnect(operation, isRetryable, dispatch). Net deletion either way, but it leaves the exception in place.

It also matters for the stated goal. "Typed request<K>() as the sole operation API" is weaker with an operation the sole API refuses to serve. The capability mutations are a different case — they carry a stateful provider and are not request/response at all. host.status is an ordinary request with a post-check. It is also the only one of the five exclusions that is new.

[P3] The liveness probe was tightened without saying so

connection.ts:816 used to check only hostEpoch; it now calls #validateHostStatusIdentity, which also checks compositionId and compositionRevision. I traced this one out: the probe's .catch already routed to #fail, and #fail is idempotent, so the only real change is that a composition swap during a live connection now fails the probe. That matches what reconnecting-connection.ts:78-82 already treats as fatal, so it's a consistency fix rather than a regression — but the body says status validation is preserved, and this quietly changes when a probe fails. Worth a line in the description.

What this gets right

All 17 aliases are gone from both connections with no residue, callers moved to operation keys consistently, and runtime-host-session-driver.ts narrows its structural Pick from four members to three — the dependency shrank, not just the call syntax. Real net deletion.

简体中文

方向是对的,别名删得彻底。下面两条都不阻塞合并,只是有一处和这个 PR 自己的主张相抵。

[P3] 把 host.status 做成例外,代价超过它删掉的那个别名

有三处新增只是为了把 host.status 从 typed 接口里挖出去:DirectRequestOperationKey 排除它、request()isHostStatusOperation 在运行时拒绝它,以及——因为 #request 不能再承载它——reconnecting-connection.ts:161-173 手写了一个重连循环。

第三处才是真代价。它与 #requestquery 分支(:258-266)逐字相同:一样的 requestDeadline、一样的 #waitForConnection、一样的 isRetryableQueryInterruption、一样的 previous 交接;而 openSessionSubscription:181-190)又是同一个形状换了个判据。这个循环在本 PR 之前就已重复,现在变成三份,外加 62 行新测试专门覆盖刚加的那一份。

两条出路,我选第一条:

  1. request('host.status', …) 在 direct connection 内部转到校验路径,而不是拒绝。排除项、运行时守卫、第三份循环和它的测试全部消失,reconnecting 那边的 status() 回到一行。
  2. 若保留例外,就把三处收成 #withReconnect(operation, isRetryable, dispatch)。两条都是净删除,但第二条留下了例外本身。

这对目标本身也有影响。「typed request<K>() 是唯一的操作 API」,却有一个操作被这个唯一的 API 拒绝服务,主张就弱了。capability mutations 是另一回事——它们携带有状态的 provider,根本不是请求-响应形状;host.status 只是一个带后置校验的普通请求,而且它是五个排除项里唯一新增的那个。

[P3] liveness probe 被收紧了,但没写出来

connection.ts:816 原本只校验 hostEpoch,现在调 #validateHostStatusIdentity,同时校验 compositionIdcompositionRevision。这条我追到底了:probe 的 .catch 本来就会走 #fail,而 #fail 是幂等的,所以唯一的实质变化是——连接存活期间发生 composition 切换,现在 probe 会失败。这与 reconnecting-connection.ts:78-82 已经视为致命的情况一致,所以是补齐一致性而非回归。但正文说 status 校验是「preserved」,而这悄悄改变了 probe 何时失败,值得在描述里补一句。

做得好的地方

17 个别名在两个连接上都清干净了,没有残留;调用方一致地迁到了 operation key;runtime-host-session-driver.ts 的结构化 Pick 从四个成员收窄到三个——缩小的是依赖,不只是调用语法。是真正的净删除。

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. Both notes above are non-blocking: the host.status exception is worth revisiting so the reconnect loop does not stay duplicated three ways, and the liveness-probe tightening just needs a line in the description.

Sign up for freeto 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.

3 participants

@likun666661@Astro-Han@M4n5ter