Repository navigation
feat(cli): Declare the v2 execute/status/cancel Managed Runtime contract - #12630
Conversation
Local real-environment verification: head
|
| Claim | Result |
|---|---|
vitest run managed-runtime-attestation-contract.test.ts: 54 pass |
✅ 54/54 (plus the worker suite: 62/62 across both files) |
mvn test -Dtest=ManagedRuntimeAttestationConformanceTest: 6 pass |
✅ 6/6. The full runtime-broker module is 112/112 on JDK 21 |
| "no handler mounted, the tool paths still 404" | ✅ status is still 404 on the real bundled worker (Fig 1), but see F2 |
F1 (new, the one to fix): cancel/unknown-is-ok is rejected by the existing Broker
I fed the PR's own cancel fixtures, verbatim, as the transport response into the real RuntimeBrokerService.cancelExecution. I used both paths that actually reach transport.cancel: a live EXECUTING claim, and a lapsed claim fenced to UNKNOWN while the invocation is still running.
in-flight-cancel-requestedandprepared-settles-cancelledare absorbed correctly on both paths.unknown-is-okfails on both paths with503 runtime_execution_cancel_failed,retryable=true("Runtime cancellation returned an invalid status"). The cause isRuntimeBrokerService.RUNTIME_EXECUTION_STATES = {prepared, executing, cancel_requested, settled}(line 27). TheRuntimeTransportjavadoc lists the same four states.- This is exactly the case the contract is written for: the Broker calls
transport.cancelonUNKNOWNrecords, and a restarted Runtime will answerunknown. Today that becomes a retryable error, so the retry can never succeed. - Candidate fix, tested: add
"unknown"to that set, and update the javadoc to match. Both rows then succeed (EXECUTING → CANCEL_REQUESTED,UNKNOWNstaysUNKNOWN), andRuntimeBrokerServiceTeststays green (61/61 existing tests). - This is not a transport concern. The check lives in the Broker core, so no
HttpRuntimeTransportslice will fix it.
F2: gate admission without a handler contradicts the attestation design's own rule
I ran the real dist/cli.js managed-runtime-worker with a boot payload on stdin and sent raw fetch() requests. On the PR head, the three new paths now pass the raw gate, fall through to Express's finalhandler, and come back as text/html "Cannot POST …" pages, before authentication runs, including an unauthenticated 300 KiB POST. On the base build they got the gate's empty 404. The triage review already noted that the body changed. What it did not note is that docs/design/2026-09-22-managed-runtime-attestation-contract.md forbids this change (lines 47/83/101):
"Each operation is added when its real handler is extracted, in the same change that registers it. This prevents a manifest entry from claiming that a route exists when
mainhas no implementation."
The new doc says it follows "the manifest rule that a route joins with its real handler in the same change", while doing the opposite. The old doc still says the manifest "currently contains the one route". There is no security impact: no body is parsed, and status and classification are unchanged. But either update that rule, or keep the manifest entries out of the gate until the handler lands.
F3: the shared schema enforces fewer of the doc's rules than the prose claims (plus a candidate patch)
- The PR schema accepts 7 documents that break rules stated in §3.2/§3.3. Among them: a
statusrequest carryingtoolName+input(the doc says status is read-only),resulton a non-settledstate, andlastSequenceon execute or cancel. - Mutation matrix over the contract artifacts, 8 mutants × both suites: with the PR schema, 3/8 survive both suites (M4 settled→unknown, M6 execute gains
lastSequence, M8 ok case loses its body). - Candidate schema patch (+117/−2, Ajv-strict clean, fixtures untouched). It adds
if state=settled then result else no result, requires a body onokcases, and adds per-route request/response shapes viaif route=…. It rejects all 7 violations, kills 8/8 mutants, and both suites stay green (54/54 TS, 6/6 Java). Taking it in this PR is optional. The alternative is to state in §3.4 that these rules are enforced only by the Java field-set assertions.
Correction to the triage review
The review says HttpRuntimeTransport.execute() is "already reachable from RuntimeBrokerService.dispatch". It is not.
HttpRuntimeTransportdoes not implementRuntimeTransport, and no production class implements that interface.- The only production use is
LocalProcessRuntimeProvisionercallingtransport.attest(...). HttpRuntimeTransport.executehas zero callers, in production or in tests.
The envelope mismatch the review describes is real. I reproduced it against a Node server that enforces this PR's schema (Fig 2 B): 5 unevaluated fields, no toolName/input, and a schema-valid 20.6 KB result rejected as 413 by the 16 KiB cap. But the code involved is dead, untested preview code from #12552, so the follow-up is replacing a stub, not rewriting a live path. Deleting that method, or marking it @Deprecated with a pointer to this contract, would stop the next reviewer from rediscovering the mismatch. RuntimeTransport.execute(lease, session, reference) also has no toolName/input to send, and the Broker stores neither. The next slice has to widen that interface.
Nits
- Error codes and messages carry attestation names, and both existing
…_too_largeemitters hardcode "exceeds 16 KiB" (same as the triage review). - No oversized-body fixture exists for
status/cancel(16 KiB). preparedhas no fixture.erroris allowed on asuccessresult.
中文说明
本地真实环境验证：head d9f241db
结论：契约切片本身成立，作者声明的测试结果可复现。合入前（或作为下一个切片的第一行改动）需要修一个 triage 评审没发现的问题：新规则 cancel → 200 {state:"unknown"} 在现有 RuntimeBrokerService 里会变成可重试的 503。 除此之外没有发现阻塞合入的问题。triage 评审（stage 2/3）有一处需要更正，见下文。
环境：macOS arm64，Node 24.18.1，pnpm install --frozen-lockfile + npm run bundle，Zulu JDK 21。base 臂 = PR 树只把 managed-runtime-attestation-contract.ts 回退到 1d30ddc9fa 后重新打包。所有探针都是临时的，已回退，工作树在 d9f241db 上是干净的。
可复现的部分
- TS 契约测试 54/54；加上 worker 测试，两个文件共 62/62。
- Java conformance 6/6；
runtime-broker全模块在 JDK 21 上 112/112。 - 真实打包的 worker 上，三条新路径仍然返回 404（图 1），但见 F2。
F1（新发现，需要修）：cancel/unknown-is-ok 被现有 Broker 拒绝
把 PR 自己的 cancel fixtures 原样作为 transport 响应，喂给真实的 RuntimeBrokerService.cancelExecution。两条真正会调用 transport.cancel 的路径都测了：存活的 EXECUTING 声明，以及调用仍在运行、声明已过期并被围栏为 UNKNOWN 的情况。
in-flight-cancel-requested和prepared-settles-cancelled在两条路径上都被正确吸收。unknown-is-ok在两条路径上都抛出503 runtime_execution_cancel_failed，且retryable=true。 原因是RUNTIME_EXECUTION_STATES只有prepared/executing/cancel_requested/settled四个值（第 27 行），RuntimeTransport的 javadoc 也只列了这四个。- 这恰恰是契约要覆盖的场景：Broker 会对
UNKNOWN记录调用transport.cancel，而重启过的 Runtime 会回答unknown。现在这会变成可重试错误，重试永远不会成功。 - 候选修复（已实测）：在这个集合里加上
"unknown"，并同步修改 javadoc。两行都变为成功（EXECUTING → CANCEL_REQUESTED，UNKNOWN保持UNKNOWN），RuntimeBrokerServiceTest现有 61 个测试全绿。 - 这不是 transport 层的问题。校验在 Broker 核心里，后续的
HttpRuntimeTransport切片修不到它。
F2：没有处理器就放行，违反 attestation 设计文档自己的规则
用真实的 dist/cli.js managed-runtime-worker，从 stdin 传入 boot 数据，再发原始 fetch() 请求。在 PR head 上，三条新路径通过了原始门，落到 Express 的 finalhandler，返回 text/html 的 "Cannot POST …" 页面，而且发生在鉴权之前（未鉴权的 300 KiB POST 也一样）。base 构建返回的是门的空体 404。响应体的变化 triage 评审已经提到。它没提到的是：2026-09-22-managed-runtime-attestation-contract.md 第 47/83/101 行明确禁止这种做法——"每个操作在其真实处理器被提取时、于同一个变更中加入"。新文档声称遵守"同一变更"规则，实际做的却相反；旧文档仍写着 manifest "目前只有一条路由"。这没有安全影响（请求体不会被解析，状态码和分类都不变），但需要二选一：更新那条规则，或者在处理器落地之前不让这些条目进入门。
F3：共享 schema 实际强制的规则比文字描述少（附候选补丁）
- PR 的 schema 接受 7 个违反 §3.2/§3.3 规则的文档。例如：status 请求携带
toolName+input（文档说 status 只读），非settled状态携带result，execute/cancel 响应携带lastSequence。 - 对契约产物做了变异矩阵（8 个变异体 × 两套测试）：用 PR 的 schema，有 3/8 的变异体两套测试都杀不掉（M4、M6、M8）。
- 候选 schema 补丁（+117/−2，Ajv strict 下无报错，fixtures 不变）：拒绝全部 7 个违规文档，变异体 8/8 被杀，两套测试仍全绿（TS 54/54，Java 6/6）。可以在本 PR 里采纳，也可以在 §3.4 说明这些规则只由 Java 字段集断言保证。
对 triage 评审的更正
评审说 HttpRuntimeTransport.execute() "已经可以从 RuntimeBrokerService.dispatch 到达"。这不成立。
HttpRuntimeTransport没有实现RuntimeTransport，生产代码里也没有任何类实现这个接口。- 生产代码唯一的用法是
LocalProcessRuntimeProvisioner调用attest()。 execute没有任何调用方，生产代码和测试里都没有。
评审描述的信封不一致是真的，我已用一个强制执行本 PR schema 的 Node 服务端复现（图 2 B）：多出 5 个字段、缺少 toolName/input，一个 20.6 KB 的合法结果被 16 KiB 上限拒成 413。但涉及的代码是 #12552 留下的、没有测试的预览死代码，所以后续工作是替换一个桩，而不是重写一条在用的路径。建议删掉这个方法，或者标 @Deprecated 并指向本契约，免得下一位评审再重新发现一遍。另外，RuntimeTransport.execute(lease, session, reference) 没有 toolName/input 可以发送，Broker 也不存储这两个字段，下一个切片必须扩展这个接口。
小问题
- 错误码和消息沿用 attestation 的命名，现有两处
…_too_large发出方都把 "exceeds 16 KiB" 写死在消息里。 status/cancel没有超大请求体的 fixture（上限 16 KiB）。prepared状态没有 fixture。success结果允许携带error。
|
Addressed the real-environment verification findings:
Verification: |
Round 2 verification: head
|
Extend the owned managed-runtime route manifest with the three tool operations and add their shared schema and conformance fixtures. The wire shape fixes the evidence rules the recovery design demands: every call is keyed by the original reference (sessionId, promptId, callId, argsDigest) rather than any Broker-side identifier; status is read-only and answers unknown with 200 instead of 404 or 500, because a missing record is never evidence that a call did not run; a settled state is the only one that may carry a result; and status additionally returns the Runtime's own lastSequence cursor. Execute accepts up to 256 KiB of request input, and every operation answers at most 1 MiB. No handler is mounted yet: per the manifest rule the routes join in the same line of slices as the execute extraction that implements them. The TypeScript gate test proves the owned-route gate admits exactly these paths, and the Java consumer pins the route contract, every outcome classification, and the closed field sets, so the worker and transport slices that follow share one reviewed target. Related to QwenLM#12380.
Co-authored-by: Qwen-Coder <[email protected]>
1c836fd to
37fd360
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
The route manifest declares the v2 execute/status/cancel contract, but the raw gate treated every declaration as admitted, so three unimplemented operations reached Express ahead of any handler. The gate now admits only the attestation route it serves, and each handler expands admission in the change that mounts it. The shared schema also enforced less than the documented contract: one permissive request body covered all three routes, and nothing tied result to a settled state or lastSequence to status. Each route now binds its own closed request body, every ok case must carry a response body, result outside settled is rejected, and lastSequence is rejected outside status. Mutation tests break one rule at a time to prove each constraint is load-bearing, and the Java conformance test reads the renamed definitions. Both design doc pairs now separate contract declaration from gate admission. Co-authored-by: Qwen-Coder <[email protected]>
|
Addressed F2 and F3 from the real-environment verification; both are fixed in this PR rather than deferred.
Verification on the pushed head
|
|
Recorded deferral (non-blocking, from the round-2 verification): loading 记录推迟：第二轮验证中提到的可选建议——让 Broker 的两个取消测试从共享 fixtures 读取 |
|
@qwen-code /resolve |
…ntract Main gave the execution lookup its own state and field vocabulary while this branch taught cancellation to accept an unknown state. Both survive the merge: cancel validation keeps unknown, and the lookup keeps its separate sets, so neither contract narrows the other.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Conflict resolution — PR #12630 ← mainRoot cause. main's #12655 (3413e8c, UNKNOWN-execution reconciliation) added a Semantic, not just adjacent. Both sides redefined which private static final Set<String> RUNTIME_EXECUTION_STATES = Set.of(
"prepared", "executing", "cancel_requested", "settled",
"unknown"); // ← this PR: gates the cancel path
private static final Set<String> RUNTIME_STATUS_STATES = Set.of(
"prepared", "executing", "cancel_requested", "settled",
"unknown"); // ← main: gates the lookup path
private static final Set<String> RUNTIME_STATUS_FIELDS = Set.of(
"state", "result"); // ← mainLoad-bearing. The two state sets are now element-identical but must stay separate declarations: Could not verify. No build or tests ran. 中文说明冲突根因。 main 的 #12655（3413e8cc57，UNKNOWN 执行对账）为 属于语义冲突。 双方各自重新定义了 broker 接受哪些 关键约束。 两个状态集合元素已相同，但必须保持独立声明：前者只约束 未能验证。 未执行构建与测试。 |
Round 3 verification: head
|
| Round 1 | Now | How verified | |
|---|---|---|---|
| F2 gate admission | new paths → Express HTML 404, pre-auth | all three → the gate's empty 404, same as base | fresh pnpm install + bundle, real dist/cli.js managed-runtime-worker, same 8 raw requests |
| F3 schema vs prose | accepted 7/7 violating documents; 5/8 mutants killed | all 7 rejected; 8/8 killed | same Ajv-strict probe and the same 8 mutants. Fixtures are byte-identical to round 1, so the gain comes entirely from the schema |
F1 cancel unknown |
503 retryable | 6/6 cancel fixtures OK on the merged code | same probe against the real RuntimeBrokerService |
The design-doc rewrite ("declaration does not imply admission") is consistent in both languages and matches the gate code.
Build: the /resolve merge was never compiled, so I compiled it
runtime-brokermvn clean testpasses 156/156 on JDK 21 and on JDK 25.- The TS contract and worker suites pass 72/72, matching the author's number.
- PR CI is green (
review-prandweb-shell E2E Smokewere still running when I checked). - The two separate state sets the bot kept (
RUNTIME_EXECUTION_STATESfor cancel,RUNTIME_STATUS_STATES/RUNTIME_STATUS_FIELDSfor lookup) are both exercised: the cancel probe for the first, the status probe below for the second.
F4 (new, latent): the wire status response doesn't fit the Broker's closed lookup shape
I fed this PR's three status fixtures into main's real reconcileExecution through FakeTransport.status:
| fixture | verbatim, as the wire schema defines it | transport strips protocolVersion + lastSequence |
|---|---|---|
status/unknown-is-ok |
502 runtime_execution_status_invalid, non-retryable |
UNRESOLVED, record stays UNKNOWN ✅ |
status/executing |
502, non-retryable | UNRESOLVED ✅ |
status/settled-with-result |
502, non-retryable | RESOLVED → SETTLED/success ✅ |
- Cause. feat(java): Reconcile UNKNOWN tool executions from Runtime evidence #12655 closes the lookup answer to
RUNTIME_STATUS_FIELDS = {state, result}, and its javadoc says so. This PR's schema requiresprotocolVersionon every response and allowslastSequenceonstatus. - Impact once wired. A transport that passes the wire body through would turn every valid Runtime answer into a permanent 502. The execution then stays
UNKNOWN, which is exactly the stuck state feat(java): Reconcile UNKNOWN tool executions from Runtime evidence #12655 exists to clear. - Why it is latent. No production class implements
RuntimeTransport.status, andHttpRuntimeTransporthas nostatusmethod yet. So this does not block merge. - The inconsistency. The cancel path has no closed field check, so
protocolVersionpasses through there. Status rejects it. - What the probe shows. The two contracts compose correctly once the transport validates
protocolVersion == 2and strips it. - The open question is
lastSequence. The tool design (§3.3) justifies it as the cursor a reconciler uses "to advance without replaying", but the Broker's lookup has no field for it and never reads it. There are two options. Either add one sentence to §5 and thestatusjavadoc saying the HTTP transport validates and strips the envelope and thatlastSequenceis dropped until a consumer exists. Or widenRUNTIME_STATUS_FIELDSand consume it. A Java test that pipes the sharedstatusfixtures through that adapter would pin the decision.
Still open from earlier rounds (non-blocking, author-acknowledged)
HttpRuntimeTransport.executeis still the dead pre-contract stub.- The Broker cancellation tests still hand-build
Map.of("state", "unknown"); the author has deferred this.
中文说明
第三轮验证：head 53a3faac
这个 head 已 rebase 到 3413e8cc57（#12655），并包含 /resolve 机器人的合并。
结论：F1、F2、F3 都已修复，/resolve 的合并能编译、测试全过，没有阻塞合入的问题。 有一个新的潜在问题 F4，证实了 /resolve 机器人提出但没有验证的担忧。建议在本 PR 里定下来，或者在实现 status transport 的切片之前记录下来。
之前各轮的问题
- F2：用全新安装加打包的真实 worker 重放同样 8 个请求。三条工具路径都回到了门的空体 404，与 base 一致，不再返回 Express 的 HTML 页面。
- F3：同一套 Ajv strict 探针和 8 个变异体。7 个违规文档全部被拒（第一轮全部被接受）；8 个变异体全部被杀（第一轮 5/8）。fixtures 与第一轮逐字节相同，所以提升完全来自 schema。
- F1：在合并后的代码上重放 6 个 cancel fixtures，全部成功，没有回退。
- attestation 设计文档的改写（"声明不等于放行"）中英文一致，也与门的代码相符。
构建：/resolve 的合并没有编译过，由我补上
runtime-broker 的 mvn clean test 在 JDK 21 和 JDK 25 上都是 156/156。TS 契约和 worker 测试 72/72，与作者给出的数字一致。PR 的 CI 全绿（我查看时 review-pr 和 web-shell E2E Smoke 还在运行）。机器人保留的两套独立状态集合都被覆盖到了：cancel 探针覆盖前一套，下面的 status 探针覆盖后一套。
F4（新发现，潜在问题）：线上的 status 响应与 Broker 封闭的查询形态不匹配
把本 PR 的三个 status fixtures 原样经 FakeTransport.status 喂给 main 真实的 reconcileExecution，全部返回 502 runtime_execution_status_invalid，且不可重试。transport 剥掉 protocolVersion 和 lastSequence 之后，三者分别得到正确结果：UNRESOLVED、UNRESOLVED、RESOLVED→SETTLED/success。
- 原因：feat(java): Reconcile UNKNOWN tool executions from Runtime evidence #12655 把查询结果封闭为
{state, result}（javadoc 里也是这么写的），而本 PR 的 schema 要求每个响应都带protocolVersion，并允许 status 带lastSequence。 - 接上真实 transport 后的影响：如果 transport 把线上响应原样转交，每个合法的 Runtime 回答都会变成永久 502，执行一直停在
UNKNOWN，而这正是 feat(java): Reconcile UNKNOWN tool executions from Runtime evidence #12655 要解决的卡死状态。 - 为什么说是潜在问题：目前没有任何生产类实现
RuntimeTransport.status，HttpRuntimeTransport也还没有status方法，所以不阻塞合入。 - 不一致之处：cancel 路径不检查封闭字段集，
protocolVersion可以通过；status 路径则会拒绝它。 - 探针结果：只要 transport 校验
protocolVersion == 2后把它剥掉，两份契约就能正确衔接。 - 待决问题是
lastSequence：工具设计 §3.3 说它是对账方"免重放推进"的游标，但 Broker 的查询结果里没有这个字段，也从不读取它。二选一：在 §5 和status的 javadoc 里写明"HTTP transport 负责校验并剥掉外层字段，lastSequence在有消费方之前被丢弃"；或者放宽RUNTIME_STATUS_FIELDS并真正消费它。再加一个 Java 测试，把共享的 status fixtures 经这个适配器跑一遍，就能把这个决定固定下来。
之前各轮仍然存在的问题（不阻塞，作者已确认）
HttpRuntimeTransport.execute仍是契约之前的死代码桩。- Broker 的取消测试仍手写
Map.of("state","unknown")，作者已推迟处理。
|
Thanks for merging Two text points relative to #12655, which is now on main:
中文说明感谢合入 相对已经合入 main 的 #12655，有两处文字需要调整：
|
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued for an available runner; follow the workflow run for progress. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — the whole chunk (zh-CN doc + 318-line fixtures file) and both consumers were read in full; the JSON was also parsed for duplicate keys, which were absent….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：none — the whole chunk (zh-CN doc + 318-line fixtures file) and both consumers were read in full; the JSON was also parsed for duplicate keys, which were absent…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— DeepSeek/deepseek-v4.1-flash@2d473174 via Qwen Code /review (v0.24.5)
|
@qwen-code /triage |
|
Review follow-up pushed as 401d214. All ten inline suggestions have a concrete response. The general comments about the cancellation/lookup state comments and the already-shipped reconciler are also addressed; both design languages and the status interface document the required wire-envelope projection. Before committing, I completed three open-ended / presume-wrong audit passes. The first found a new test's readonly-cast typecheck/lint issue, which was corrected; the final two consecutive passes found no further issue. Reverse checks used isolated copies of the actual suites: all 12 schema mutants and all 4 Java fixture/limit mutants were caught by assertion failures, with green baselines. Validation on macOS arm64, Node 22.22.2 and JDK 21:
E2E report: the globally installed qwen did not expose a usable managed-runtime-worker startup for the baseline, so contract reproduction used a direct Ajv test script. The locally built R1-10 uses the review's explicit documentation option: responseParts element structure remains deferred until the worker/transport slice derives it from the actual SDK Part path and adds shared coverage. The fixture header closure applies to fixture objects, not arbitrary transport-added HTTP headers. The request caps remain 256 KiB for execute and 16 KiB for status/cancel, with 1 MiB responses. 中文说明已推送提交 401d214，逐条回应十条行内建议，同时处理状态注释和已落地对账器的说明。双语设计与 status 接口文档明确了线上信封向 Broker 形态的投影要求。 提交前进行了三轮无方向通读与反向核验。第一轮发现新增测试的只读类型转换与 lint 问题并修正；随后连续两轮未发现新问题。对实际测试文件的隔离副本撤掉约束或破坏语料，12 种 schema 退化和 4 种 Java fixture/上限退化全部触发断言失败，对照组保持通过。 macOS arm64、Node 22.22.2、JDK 21 上，完整 build、bundle、typecheck 通过，改动文件 ESLint/Prettier 和 diff 检查通过；TypeScript 测试 85/85，Java 测试 96/96。独立复现从原契约的 11 项失败转为 17/17 通过。 E2E 报告：全局 qwen 无法启动可用的 managed-runtime-worker，因此基线契约问题用直接 Ajv 脚本复现。本地 bundle 的 worker 启动成功，attestation 返回 200、精确预期响应和 no-store；execute/status/cancel 均返回 raw gate 的空体 404。未声称工具处理器已实现或已验证。 R1-10 按评审明确允许的文档方案处理：responseParts 元素结构推迟到 worker/transport 切片，从实际 SDK Part 路径确定后补齐共享覆盖。请求头封闭规则约束 fixture 对象，不禁止 HTTP 层正常添加的头。请求上限保持 execute 256 KiB、status/cancel 16 KiB，响应均为 1 MiB。 |
Round 4 verification: head
|
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Independent Critical-only review — head 401d2149
Declares the v2 execute/status/cancel Managed Runtime contract: a JSON schema (+475) and fixture set (+384), a conformance test (+142), four design docs, and 43 production lines across three files — RuntimeBrokerService.java (+5/-2), RuntimeTransport.java (+8/-4) and managed-runtime-attestation-contract.ts (+30/-3).
No historical blocker
No Critical has ever been filed against this PR. The only ledger, round 1 at 53a3faac, records floor:"o" with ten findings, all sev:"S", and that review has been dismissed; head has since moved to 401d2149, which carries a maintainer approval. All ten review threads are resolved with none outstanding, and there is no CHANGES_REQUESTED standing against this head.
The one behavioural change is a widening that can only fail safe
RUNTIME_EXECUTION_STATES gains "unknown", making it identical to RUNTIME_STATUS_STATES:
private static final Set<String> RUNTIME_EXECUTION_STATES = Set.of(
"prepared", "executing", "cancel_requested", "settled", "unknown");A set like this is only as safe as its use sites, so I enumerated them rather than reasoning from the name. There are exactly two, one per constant: RUNTIME_EXECUTION_STATES at :1076 and RUNTIME_STATUS_STATES at :1166. The first is the allowlist in absorbCancellationStatus:
Object state = status == null ? null : status.get("state");
if (!(state instanceof String) || !RUNTIME_EXECUTION_STATES.contains(state)) {
throw unavailable("runtime_execution_cancel_failed", "Runtime cancellation returned an invalid status");
}
if (!"settled".equals(state)) {
return;
}Accepting "unknown" therefore changes one thing: a cancel answer of unknown becomes a well-formed no-op instead of a 502. It falls straight into the !"settled" early return, so it absorbs nothing, settles nothing and leaves the record exactly as requestCancel left it. The widening cannot reach the only branch that writes. The second consumer already treated unknown this way, so both now agree — which is what the new comments and the interface javadoc state ("A lookup answers with the same states as a cancellation" / "Status results use the same states"), and it is the alignment an HTTP adapter needs to project one envelope onto both operations.
This is also semantically right rather than merely permissive: unknown means the Runtime holds no record of the reference, which is a legitimate answer to a cancel of something it never saw, and the sibling contract already establishes that it is never evidence the call did not run — so absorbing it without settling is the correct handling.
RuntimeTransport.java is javadoc-only: it restates the cancel states to include unknown and adds that an HTTP adapter must validate the wire envelope and project it to this shape, stripping protocolVersion and lastSequence because the Broker does not consume the Runtime's response cursor yet. No behaviour change, and it documents the adapter's obligation at the interface an adapter implements.
The new route declarations are bounded and non-cacheable
OWNED_MANAGED_RUNTIME_ROUTES gains three frozen entries under the existing /internal/managed-runtime/ prefix, all at protocolVersion: 2 and all cacheControl: 'no-store':
| route | method | request limit | response limit |
|---|---|---|---|
execute |
POST | 256 KiB (new MANAGED_RUNTIME_TOOL_REQUEST_BODY_LIMIT_BYTES) |
1 MiB (new MANAGED_RUNTIME_TOOL_RESULT_BODY_LIMIT_BYTES) |
status |
POST | 16 KiB (the existing attestation limit) | 1 MiB |
cancel |
POST | 16 KiB (the existing attestation limit) | 1 MiB |
The sizing is proportionate: execute carries a tool reference and arguments so it gets the larger request bound, while status and cancel carry only a reference and reuse the 16 KiB attestation bound. Every route has an explicit request and response bound, so no path reads an unbounded body, and no-store keeps execution results and cancellation answers out of any intermediary cache — the right posture for a control plane. The limits are exported named constants rather than inline literals, and the entries are frozen inside a frozen array, so the declaration cannot be mutated at runtime.
Scope disclosure
The schema and fixture files are declarative contract data, and the ten Suggestions from round 1 are all about how tightly they pin things — the envelope limits declared as any integer above 1024 rather than exact values, expected.code as a free non-empty string, request headers left open with additionalProperties: true, responseParts declared as an array with no item schema, a response allOf encoding only one direction, and the suites array pinned to exactly three items. Per Critical-only scope I did not gate on any of them; they are contract-tightness questions for the maintainer, and a permissive schema in a conformance fixture weakens what the test proves without mis-certifying runtime behaviour.
One production hunk I did not read: the ~2-line net change to isOwnedManagedRuntimeRoute at :311, the matcher over the route table. I ran out of budget before opening it. It is exercised by the +344-line attestation-contract test and the +142-line conformance test, and the lanes that completed are green, but I am naming it because it is the one piece of production code in this diff I cannot speak to.
CI
review-pr and web-shell E2E Smoke (ubuntu-latest, Node 22.x) were in progress at head and a route check shows cancelled from superseded runs; no failure is attributable to this diff and CI state is not a gate. Two of my API calls hit transient network timeouts during this review and were retried successfully, so nothing above rests on a failed read.
Verdict: APPROVE — No Critical found. The single behavioural change admits unknown as a cancel answer at its one use site, where it becomes a no-op that cannot reach the settling branch; the transport change is documentation; and the three new route declarations are bounded on both request and response and marked no-store. One small matcher hunk is disclosed above as unread.
…s-p0-p8 Brings in main through 2686cad. The conflicts are all in runtime-broker, where QwenLM#12627 upstreamed this branch's durable binding recovery onto main's asynchronous service. The branch keeps its own RuntimeBrokerService, as in earlier merges, and takes main's refinements of the shared data model: the provision seed binds its lease to the provisional Runtime instance, resource handles compare by their canonical JSON, the encryption contexts use a fixed-width digest of the binding id, provisioner_kind widens to 512 characters, and the binding repositories gain releaseOperation. placement_domain and runtime_template_digest stay in the request identity. Main's behaviors the branch service lacked are ported, with tests in the branch's API: - a release settles a Session that still pins a LOST generation, and a LOST binding that is still referenced no longer caches its failure, so the generation is reclaimed once the last reference settles (QwenLM#12627); - only identity evidence (managed_runtime_identity_conflict, managed_runtime_unauthorized) blocks the re-attestation of a restored or health-refreshed binding; a throttled or malformed attestation leaves it READY for the next attempt (QwenLM#12627); - a repository failure thrown while driving a binding no longer leaves it started with nothing driving it (QwenLM#12627); - a cancellation may answer unknown, and the recorded intent stands (QwenLM#12630). Main's DurableRuntimeRecoveryTest cases run against the branch service; the ones tied to main's separate provision step are expressed through the branch's ensure-then-reconcile path. Two auto-merge artifacts are fixed: a duplicated hasActiveByBinding and a releaseOperation override spliced into LocalProcessRuntimeProvisionerTest.
…s-p0-p8 Brings in main through 688e8af. The conflicts are in the Runtime Broker's HTTP transport, where QwenLM#12637 adds execute, status and cancel to main's attestation-only HttpRuntimeTransport against the QwenLM#12630 v2 tool contract. The branch's transport implements the whole RuntimeTransport against the branch's own worker, whose request and response shapes that contract does not accept yet, so the branch versions of the transport, its test and LocalProcessRuntimeProvisionerTest stand. Converging the worker and transport on the declared contract belongs with the split that upstreams them.
4 participants
What this PR does
Defines a shared v2 contract for the owned Managed Runtime's execute, status, and cancel operations. Calls use their original reference; status is read-only and returns unknown with HTTP 200 when the Runtime has no record. A settled response must contain a result, and other states must not. The declaration manifest includes the three routes, while the raw gate continues to admit only attestation until each real handler lands.
Why it's needed
The worker and Java transport need one reviewed target before tool execution is connected. Shared fixtures pin the closed envelopes, stable error vocabulary, route-specific limits, all five states and four execution outcomes, and optional non-negative status cursors. Cancellation also accepts the Runtime's unknown answer without converting it into a retryable error.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before these review fixes, 11 independent contract assertions failed. After the fixes, all 17 reproduction assertions pass. The focused TypeScript contract and worker suites pass 85 tests (77 contract + 8 worker); the Java conformance and Broker suites pass 96 tests (6 + 90). A real bundled worker returns attestation 200 and empty 404 responses for all three tool routes. Full build, bundle, typecheck, and changed-file lint/format checks pass. Reverse verification rejects all 12 schema regressions and all 4 Java fixture/limit regressions.
Tested on
Environment (optional)
macOS arm64, Node 22.22.2, JDK 21.
Risk & Scope
Linked Issues
Related to #12380 and the reconciler delivered in #12655.
Design: English / 简体中文.
中文说明
这个 PR 做什么
为 owned Managed Runtime 的 execute、status、cancel 操作定义共享 v2 契约。调用按原始 reference 识别；status 只读，Runtime 没有记录时以 HTTP 200 返回 unknown。settled 响应必须携带 result，其他状态禁止携带。声明清单包含这三条路由，但在真实处理器落地之前，raw gate 仍只放行 attestation。
为什么需要
worker 与 Java transport 在接通工具执行之前需要统一的评审目标。共享 fixtures 固定封闭信封、稳定错误码、逐路由上限、五种状态与四种执行结局，以及可选的非负 status 游标。取消路径也接受 Runtime 的 unknown 应答，不再把它转换为可重试错误。
评审测试计划
如何验证
前后证据
修复前，独立契约复现有 11 项断言失败；修复后 17 项全部通过。定向 TypeScript 契约与 worker 测试通过 85 项（77 + 8），Java 一致性与 Broker 测试通过 96 项（6 + 90）。真实 bundle worker 的 attestation 返回 200，三条工具路由均返回空体 404。完整 build、bundle、typecheck，以及改动文件 lint/format 检查通过。反向验证抓住全部 12 种 schema 退化和 4 种 Java fixture/上限退化。
测试平台
环境
macOS arm64、Node 22.22.2、JDK 21。
风险与范围
关联
关联 #12380 与已在 #12655 交付的对账器。
设计：English / 简体中文。