Repository navigation
test(managed-agent): stub the acquire provisioner overload the Broker calls - #14498
Conversation
… calls #14435 made placement resolution go through the three-argument RuntimeProvisioner.createRequest so a bound Session's Harness id travels on its own. Mockito never runs interface default methods, so this IT's two-argument stub stopped answering the call the Broker makes: the provisioner returned null, findOrCreate(null) threw "request is required", and the Broker mapped that internal failure to a 400 runtime_broker_invalid_request at tool-sessions:acquire. Stub the overload the Broker actually calls so the acceptance chain receives the request it constructed.
github-actions
Bot
added
the
review/self-reported
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The stub now targets the three-argument createRequest overload that RuntimeBrokerService#provisionRequest calls, and any() also matches the null isolation key path; the production default still delegates three-arg to two-arg, so the change stays test-only. Verified at 8c86b02 that the mock is only handed to RuntimeBrokerService, that service has a single createRequest call site, and the fixture request is therefore the one the binding repository receives.
yiliang114
added
category/core
|
Thanks for the PR — and for writing the cause up properly in #14494 instead of just shipping a green run. Template looks good ✓ — every required heading is present, and the Chinese section is a real translation rather than a stub. Problem: observed, not theoretical. #14494 is a P1 with a concrete failing run and job link on the Runtime Broker / Managed Agent MariaDB lane, and you reproduced it locally and recovered the cause the 400 had swallowed ( Direction: aligned. A red P1 CI lane, fixed in the test's mock rather than by bending product code to satisfy a stale test. There is no product-surface or mission question here, so the CHANGELOG direction signal doesn't really apply — this is internal Java test infrastructure. Size: not applicable. One file, +1/−1, entirely test code. No Stage 0 core path is touched — Approach: minimal, and I checked the diagnosis against the source rather than taking the description on faith:
I also checked whether the same stale mock exists elsewhere, since a partial fix here would just move the failure. The other five One thing worth raising before merge — not a blocker, and I agree with leaving it out of this diff: the reason this cost hours is that Risk: no elevated risk signals — the Stage 1e revert-correlated path patterns match nothing in this diff. Moving on to code review. 🔍 中文说明感谢贡献 —— 也感谢你在 #14494 里把根因写清楚，而不是只丢一个绿色的 CI 结果。 模板：完整 ✓ —— 所有必需小节都在，中文部分是真正的翻译而不是占位。 问题：已观测到的 bug，不是理论性加固。#14494 是 P1，附带了 Runtime Broker / Managed Agent MariaDB 通道上具体的失败 run 与 job 链接；你在本地复现并还原了被 400 吞掉的根因（ 方向：对齐。一条红色的 P1 CI 通道，修的是测试里的 mock，而不是为了让过期测试通过去改动产品代码。这里不涉及产品面或方向性取舍，所以 CHANGELOG 方向信号并不适用 —— 属于内部 Java 测试基础设施。 规模：不适用。1 个文件，+1/−1，全部是测试代码。未触及 Stage 0 的任何核心路径 —— 方案：足够精简，而且我对照源码核实了诊断，没有只采信 PR 描述：
我还确认了这个过期 mock 是否存在于别处，因为只修一半只会把失败挪个位置。另外五处 有一点想在合并前提出来 —— 不是阻塞项，我也同意不要塞进这个 diff：这件事之所以花了几个小时，是因为 风险：无升级风险信号 —— Stage 1e 中与回滚相关的高风险路径模式在本 diff 中没有任何命中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewBefore reading the diff I wrote down what I would have done from the title and the "Why it's needed" section alone: find the call site the Broker actually uses, stub that overload, leave production alone. That is what this PR does, and the one-line shape is the right one — the alternatives I considered are all worse here. Stubbing both overloads would be defensive code for a call shape nothing uses; I verified the three load-bearing claims against the source rather than accepting the description:
One detail worth confirming, because it decides whether the new stub fires at all: the Broker passes No correctness, security, or regression concerns, and no AGENTS.md violations. The managed-agent module asks for Nothing here blocks merge. The one non-blocking follow-up is the untracked observability gap I raised in Stage 1 — Test evidence — the PR's own CI, via the APIThis is an unattended CI run, so per the review rules I executed nothing from this PR: no Maven, no build, no test, no checkout. The evidence below is the PR's own check-runs on the reviewed commit, read through the API, plus static reading of the diff and the surrounding source. The headline is that this PR's CI never runs the test it fixes.
That is not a defect in this PR; it is how the tiering works, and the same is true of every other The good news is that the gap closes on its own. The full tier — the only lane that executes this test — is triggered by the first maintainer approval of this head ( That promotion has already started: At review time these were still in progress: CI results for
Sandboxed verification would settle this: Not verified: that the test passes on MariaDB. The lane is skipped on this PR's CI and I ran no PR-derived code. The before/after Maven output in the PR body — including the recovered 中文说明代码审查在看 diff 之前，我先根据标题和「为什么需要」写下了自己会怎么做：找到 Broker 实际使用的调用点，stub 那个重载，不动产品代码。这个 PR 正是如此，一行的形态也是对的 —— 我考虑过的其他方案在这里都更差。同时 stub 两个重载是为一个没人使用的调用形态写防御代码；对 mock 用 我对照源码核实了三个关键论断，而不是采信描述：
还有一个细节值得确认，因为它决定新 stub 是否会生效：当 scope 的 isolation class 不是 没有正确性、安全性或回归方面的问题，也没有违反 AGENTS.md。managed-agent 模块要求 SQL/事务改动跑 这里没有阻塞合并的问题。唯一的非阻塞后续项，是我在 Stage 1 提到的、尚未被跟踪的可观测性缺口 —— 测试证据 —— 通过 API 读取本 PR 自己的 CI这是一次无人值守的 CI 运行，因此按审查规则我没有执行本 PR 的任何代码：没有 Maven、没有构建、没有测试、没有 checkout。下面的证据是通过 API 读取的、该 commit 上本 PR 自己的 check-run 结果，外加对 diff 与周边源码的静态阅读。 关键结论是：本 PR 的 CI 从未运行它所修复的那个测试。
这不是本 PR 的缺陷，而是分级机制本身如此，下面其他标为 skipped 的 好消息是这个缺口会自行闭合。full tier —— 唯一会执行该测试的通道 —— 由对该 head 的首次维护者审批触发（ 这个提级已经开始： 审查时以下检查仍在进行中： 沙箱验证可以定论此事： 未验证：该测试在 MariaDB 上是否通过。该通道在本 PR 的 CI 上被跳过，我也没有运行任何 PR 派生代码。PR 正文中的 before/after Maven 输出 —— 包括还原出的 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the diagnosis holds up against the source and the change is one line of test mock that cannot reach product code; the reservation is that nothing in this PR's own fast-tier CI executes the test it fixes, plus one untracked follow-up. Stepping back: this is a well-run bugfix. The author hit a red P1 lane, refused to accept the masked 400 at face value, instrumented the swallowed exception to recover the real cause, correctly located the fault in the test rather than in the Broker, and then fixed exactly that and nothing more. The write-up explains why production is unaffected — the three-argument default delegating to the two-argument one — which is the part a reviewer would otherwise have to go and establish themselves. I checked it, and it is true. My independent proposal and the PR's approach are the same one-line change, so there is no simpler path I found that it missed. That is worth saying plainly, because it is the main thing this stage is for: I went looking for an over-engineered alternative and there isn't one. Every plausible "more robust" variant is worse — stubbing both overloads defends a call shape nothing uses, On the question this gate exists to ask — does the problem actually exist? — yes, and unusually well-evidenced: a linked P1 with a specific run and job, a recovered stack trace, and a before/after that isolates the stub as the only variable. This is not theoretical hardening. Six months from now this reads fine: a one-line fixture stub with an adjacent issue explaining the failure mode. The diff carries no unrelated edits, no drive-by refactor, and no formatting churn — the minimal-change bar is met exactly. My one real reservation is evidential rather than technical. The lane that owns this test is gated behind The non-blocking item, restated so it does not get lost: Verdict: approve. CI is still running (2 中文说明信心：4/5 —— 诊断在源码层面站得住，改动只有一行测试 mock，触达不到产品代码；保留意见在于本 PR 自己的 fast-tier CI 中没有任何环节执行它所修复的测试，另加一个尚未被跟踪的后续项。 退一步看整体：这是一次处理得很扎实的修复。作者遇到一条红色的 P1 通道，没有把被掩盖的 400 当作表面结论接受，而是给被吞掉的异常加了打印以还原真正的根因，正确地把问题定位在测试而非 Broker，然后只修这一处、不多做别的。PR 描述解释了为什么生产侧不受影响 —— 三参 default 委托给二参形式 —— 而这正是审查者本来需要自己去确认的部分。我核实过了，确实如此。 我独立想到的方案与 PR 的方案是同一个一行改动，所以我没有找到它遗漏的更简路径。这一点值得明说，因为这本阶段的主要目的：我去找了有没有过度设计的替代方案，结果没有。所有看似「更健壮」的变体都更差 —— 同时 stub 两个重载是在为一个无人使用的调用形态做防御； 关于本关卡存在的核心问题 —— 这个问题真的存在吗？ —— 存在，而且证据异常充分：一个带具体 run 与 job 的 P1 关联 issue、一段还原出的堆栈、以及一次把 stub 隔离为唯一变量的 before/after。这不是理论性加固。 六个月后再看这段代码没有问题：一行 fixture stub，旁边有一个解释失败模式的 issue。diff 中没有无关改动、没有顺手重构、没有格式化噪音 —— 精确满足最小改动的标准。 我唯一实质性的保留是证据层面的，而非技术层面的。拥有该测试的通道被 非阻塞项，重述一遍以免被忽略： 结论：approve（通过）。CI 仍在运行（审查时有 2 个 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
评审结论：未发现本次改动引入的缺陷。
评审提交：8c86b02bf097a7408e1e1848dd29b4e13e1d8cdd（相对 f110a6b7fae47f1a5fd4f30646610a159d876103，仅 1 行测试替换）。
核对了完整测试及其调用链：这个 mock 交给 RuntimeBrokerService，其 provisionRequest 实际调用三参 createRequest(scope, isolationKey, harnessSessionId)。新 stub 对应正确的重载，继续返回原先由 SQL fixture 构造的 request；any() 也覆盖 isolation key 为 null 的情况。原有 HTTP 状态、SQL grant/settlement、文件内容、保留历史和 worker 关闭断言均保持不变。
独立验证：使用 JDK 21、项目对应的 Mockito 5.17.0 / Byte Buddy 1.17.8，编译此提交的真实 RuntimeProvisioner 及其所需类型后运行最小脚本。旧二参 stub 在三参调用时返回 null；新三参 stub 返回指定 request，含第二个参数为 null 的情况。脚本通过；git diff --check 通过。
验证边界：本次未运行完整 WorkspaceCsiSqlWorkerMySqlIT 或 mvn verify；当前工作树缺少 CLI/Core 构建产物。PR 中的 MariaDB CI 任务为 SKIPPED，作者提供的完整 MariaDB Before/After 结果未在本次评审中独立重跑。以上动态验证仅确认重载和 mock 行为，不等同于完整 SQL/HTTP/worker 链路通过。
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
按 dws-review 频道的 Critical-only 流程复核 head 8c86b02bf097a7408e1e1848dd29b4e13e1d8cdd（1 文件，+1/-1，只改 WorkspaceCsiSqlWorkerMySqlIT.java 的一行 stub）。未发现可证明的 Critical。历史 Review 中没有阻塞问题：唯一一条是 APPROVED，本 PR 没有任何行内意见线程。
改动是把 when(provisioner.createRequest(any(), any())) 换成三参形式 when(provisioner.createRequest(any(), any(), any()))。这类"改 mock 重载"的一行改动，Critical 面只有一个：它是否让测试变得更空、或掩盖了生产调用契约。我把它依赖的前提在 head 上直接读了一遍，没有依赖他人结论：
- 生产确实调用三参重载。
RuntimeBrokerService.provisionRequest(RuntimeScope scope, String harnessSessionId)（第 5346 行起）的调用是provisioner.createRequest(scope, "session".equals(scope.getIsolationClass()) ? harnessSessionId : null, harnessSessionId)，即三参形式，且第二个参数在非session隔离类下为 null。 - 接口里两个重载都是 default，三参默认委派给二参。
RuntimeProvisioner第 26 行的二参 default 直接构造RuntimeProvisionRequest，第 37–39 行的三参 default 是return createRequest(scope, isolationKey);。但mock(RuntimeProvisioner.class)会替换包括 default 在内的全部方法，委派在 mock 上并不发生——所以旧的二参 stub 对生产的三参调用完全不生效，fixture 构造的request并不是代码实际拿到的那个。本 PR 修的正是这一点，方向是让 stub 对齐真实调用点，而不是放松断言。 any()覆盖 null 隔离键。 Mockito 的any()对引用类型匹配 null，因此生产在非session隔离类下传入的null也能命中该 stub，不会出现"stub 太窄导致仍返回 null"的残留问题。- 没有削弱任何断言。 diff 只有这一行，前后的
doAnswer(...).when(provisioner).ensureResource(any(), any(), any())、lease 的doAnswer、以及该用例原有的 HTTP 状态、SQL grant/settlement、文件内容、保留历史与 worker 关闭断言都未改动；也没有新增@Disabled或跳过标记。 - 改动是测试侧的，不影响生产契约。 接口中三参 default 委派给二参的行为、以及
WorkspaceRuntimeProvisioner/LocalProcessRuntimeProvisioner等真实实现对两个重载的处理都未被本 PR 触及，因此真实实现上的委派语义不变。
未覆盖 / 非阻塞
WorkspaceCsiSqlWorkerMySqlIT 属于需要真实 MySQL 的 IT，sdk-java.yml 没有 pull_request 触发器，因此本 PR 的 CI 不含这条用例的执行证据；作者报告在 JDK 21 与项目对应的 Mockito/Byte Buddy 版本下编译并跑过该用例，这属于其提供的证据，我未重跑 Java/MySQL lane。我的结论建立在对 head 上生产调用点与接口重载的直接阅读上——这两点足以判定 stub 对齐的是真实调用契约。
CI：无失败项，SDK Java (fast) / Real daemon E2E / Java 11、Flyway migration version uniqueness、Build and verify reference distribution、Classify PR、Managed Agent tier 选择、assign、label、authorize、route 等均 pass；Test (ubuntu-latest, Node 22.x)、Lint & Static、Integration Tests (no-AK, No Sandbox)、SDK Java (fast) / ubuntu-latest / Java 21、triage 与 review-pr 在我发布时仍为 pending，按频道规则不等待、不轮询。
|
Released in v0.25.3. |
What this PR does
Updates the MariaDB integration test's mocked provisioner so it answers the overload the Runtime Broker now calls when a Session acquires a Tool Runtime.
Why it's needed
The test failed on the Runtime Broker and Managed Agent MariaDB lane:
tool-sessions:acquireanswered400 runtime_broker_invalid_request. The cause was not the request the test sends — it was a stale mock.The Broker resolves placement through the three-argument form of the provisioner's request factory so a bound Session's Harness id can travel on its own. The test's mock only stubbed the two-argument overload, and Mockito never runs interface default methods, so the three-argument call returned
null. The Broker handed thatnullto the binding repository, which rejected it with "request is required"; thatIllegalArgumentExceptionsurfaced as the generic 400 at the acquire route.The production default still delegates the three-argument form to the two-argument one, so every real provisioner keeps its previous behaviour — only the mock stopped matching. Stubbing the overload the Broker calls restores the test's intent: the acceptance chain receives the request the test constructed.
Reviewer Test Plan
How to verify
Start a disposable MariaDB 10.11 (this lane uses MariaDB), build the JavaScript prerequisites (the test drives the real CLI worker), then run the single integration test from
packages/managed-agent-serverwith JDK 21:Expected: one test, zero failures, ~15 s. With the old two-argument stub the same command fails at the acquire step with
400 !== 200andruntime_broker_invalid_request.Evidence (Before & After)
Before (same machine, same MariaDB, only the stub reverted):
The swallowed cause, recovered by temporarily printing the mapped exception:
After:
mvn checkstyle:checkalso passes.Tested on
Risk & Scope
IllegalArgumentExceptionto a client-facing 400 without logging it, which is what hid this cause in CI — that observability gap is left for a follow-up rather than widened into this fix.Linked Issues
Fixes #14494
中文说明
本 PR 做了什么
修正 MariaDB 集成测试中被 mock 的 provisioner，使其响应 Runtime Broker 在 Session 获取 Tool Runtime 时实际调用的那个重载。
为什么需要
该测试在 Runtime Broker and Managed Agent MariaDB 车道上失败：
tool-sessions:acquire返回400 runtime_broker_invalid_request。根因不是测试发送的请求，而是过期的 mock。Broker 通过 provisioner 请求工厂的三参形式解析放置信息，使绑定 Session 的 Harness id 能单独传递。测试的 mock 只 stub 了二参重载，而 Mockito 不会执行接口默认方法，因此三参调用返回
null。Broker 把这个null交给绑定仓储，被以 "request is required" 拒绝；这个IllegalArgumentException在 acquire 路由上表现为通用的 400。生产侧的默认实现仍然把三参形式委托给二参形式，所以所有真实 provisioner 的行为不变——只有 mock 不再匹配。改为 stub Broker 实际调用的重载，即恢复测试本意：验收链路拿到测试构造的那个请求。
评审测试计划
如何验证
启动一次性 MariaDB 10.11（该车道使用 MariaDB），构建 JavaScript 前置产物（测试会驱动真实 CLI worker），然后在
packages/managed-agent-server下用 JDK 21 单独运行该集成测试：预期：1 个测试、0 失败、约 15 秒。若换回旧的二参 stub，同一命令会在 acquire 步骤失败，报
400 !== 200与runtime_broker_invalid_request。证据（修复前后）
修复前（同机、同 MariaDB，仅把 stub 还原）：
被吞掉的根因（临时打印被映射的异常后取得）：
修复后：
mvn checkstyle:check同样通过。测试环境
风险与范围
IllegalArgumentException映射成面向客户端的 400 且不打日志，这正是 CI 中掩盖根因的原因——该可观测性缺口留给后续处理，不并入本次修复。关联 Issue
Fixes #14494