Repository navigation
fix(managed-agent): Declare the tenant filter's 403 on the task read routes - #12966
Conversation
…routes The tenant filter answers 403 actor_scope_mismatch on every /v1/agents and WebShell route when an authenticated actor does not belong to the tenant. The Session, lifecycle, operation and Turn routes declare it, but the task list, detail and event routes did not, and the task contract note said that the read routes declare no 403. Declare it on the six task read routes of both surfaces, say in the shared Forbidden response that it covers this code, and move the contract to v1.21.0. The contract test probes the 403 on the four served task routes. The note also records the A9 decision: the Legacy paused and pausing states map to waiting, and TaskState gains no state. Part of #12847.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6b": did not run packages/web-shell/client/components/managed/managed-agent-api.test.ts — the generated-types freshness test that compares the checked-in file to ….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6b"：did not run packages/web-shell/client/components/managed/managed-agent-api.test.ts — the generated-types freshness test that compares the checked-in file to …。
— DeepSeek/deepseek-v4.1-flash@2d473174 via Qwen Code /review (v0.24.6)
…refix The contract test probes the 403 only on mapped routes, so nothing checked the declarations on the planned task event routes. A new check in PlannedTaskContractTest requires the 403 on the planned task event and cancel routes of both surfaces. The tenant filter matches the /v1/agents/ prefix with its trailing slash, so the collection path POST /v1/agents skips it. Say /v1/agents/ in the Forbidden description and the v1.21 note, as the design note does.
|
Addressed the review in 982da70:
中文说明已在 982da70 处理评审意见：
|
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only scan — APPROVE
Head reviewed: 982da70e142c687d18e76934b11fcbce74fce2b3
Scope
Contract, documentation and test changes only. No production code is touched:
openapi/managed-agent-public-api.openapi.json(+21/−3) — version 1.20.0 → 1.21.0, six added403declarations, one rewrittenForbiddendescriptionweb-shell/client/components/managed/generated/managed-agent-api.ts(+3/−1) — regenerated mirror- two design docs (+22/−8) and the server README (+5/−2)
ManagedAgentApiContractTest.java(+17),PlannedTaskContractTest.java(+12)
Historical blocking issues — none exist
There is no CHANGES_REQUESTED and no bot review on this PR. The only two inline findings were filed by the author against his own earlier head affe54ab and both are marked [Suggestion]; both were fixed at this head. The sandboxed verification comment reports "✅ passed — merge-ready", and there is no [Critical] or sev:C marker anywhere in this PR's history.
What I verified
Every declaration this PR adds is now pinned by a test, on both sides of the mapped/planned split. This was the substance of the author's own R1-1, and the fix is the right shape:
- The four mapped task routes —
listSessionTasks,getSessionTask,queryWebShellTasks,getWebShellTask— gain a realexchange(drift, …, 403, …)case each inManagedAgentApiContractTest.exchangeTasks. Each sends the correctX-Qwen-Tenant-Idwith an actor principal fromotherTenant, so the tenant filter's403 actor_scope_mismatchis actually produced by a request and checked against the spec through the drift map, not merely declared. - The four planned routes —
listSessionTaskEvents,queryWebShellTaskEvents,cancelSessionTask,cancelWebShellTask— cannot be exercised because no pass sends a request to aplannedoperation, so the newPlannedTaskContractTest.plannedTaskRoutesDeclareTheTenantFilterForbiddenasserts the declaration directly, with a failure message naming the operation and a comment recording exactly why this test has to exist separately. Deleting any one of those403blocks now reddens a named test.
Together these cover the whole set the v1.21 sentence claims, so the prose and the spec cannot diverge silently.
The contract no longer over-claims. The author's R1-2 was that "every /v1/agents route" is false for the collection path, because TenantContextFilter.shouldNotFilter matches path.startsWith("/v1/agents/") with the trailing slash and so skips POST /v1/agents entirely. At this head both the Forbidden description and the v1.21 sentence read /v1/agents/, which is accurate, and the collection-path gap is recorded as item B11 of #12847 rather than being papered over by a wrong sentence. Declaring a filter behaviour the filter does not have would have been the one real defect available in a contract-only PR, and it is not present.
The generated client is in step with the spec. managed-agent-api.ts carries the identical rewritten Forbidden description and adds 403: components["responses"]["Forbidden"] to the same operations, so the file reads as regenerated rather than hand-edited, and SDKs generated from it will surface the response.
The change is additive for callers. Each hunk inserts a 403 alongside the existing 400 and 404; no response, schema, field or status code is removed, and the v1.21 sentence records that a caller who cannot read a task still gets 404 — so the existing not-found behaviour is unchanged and only a previously-undocumented status becomes visible. The whole-spec count of Forbidden references at this head is 36, six more than before, matching the six declarations.
The Session and Turn reads already declared this, so the task routes were the inconsistency; this PR removes it rather than introducing a new response class.
CI
Green at this head: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, Capture web-shell visuals, the full Java matrix, Runtime Broker and Managed Agent MariaDB / Java 21, Hosted process fault gates / MySQL 8.4 / Java 21 and Real daemon E2E / Java 11 all pass. Only review-pr is pending, which is not a gating check. No failure is attributable to this PR.
The tenant filter answers 403 actor_scope_mismatch for an actor from another tenant and for an actor with an invalid ID; the Forbidden description named only the first. The task contract note now names both, lists the filter's 400 invalid_tenant beside it, and records the sixth PlannedTaskContractTest test in sections 5 and 6.
|
Round 2 of
Local checks on 032ea70: the three contract tests pass 14/14, and the WebShell types test passes 2/2 after regeneration. 中文说明第二轮
032ea70 的本地检查：三个契约测试 14/14 通过；重新生成后 WebShell 类型测试 2/2 通过。 |
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
批准
对照 032ea70 和当前 main（契约仍为 1.20.0）看过 diff、TenantContextFilter、两条契约测试和 #12847 的 A9 / A10。没有需要改的问题。
核对过的点
- A10。 六条任务路由的
403都指向共用的Forbidden。四条已映射的读取（listSessionTasks、getSessionTask、queryWebShellTasks、getWebShellTask）在exchangeTasks里用正确的租户头，加上另一租户的AuthenticatedTenantActor探测。exchange()会先要求该状态码已声明，再比对实际状态码，并按ErrorEnvelope校验响应体。两条planned事件路由发不出请求，由plannedTaskRoutesDeclareTheTenantFilterForbidden守住声明；两条取消路由本来就有403，放进同一检查是回归守卫。这比 #12847 点名的四条partial路由多两条事件查询，理由成立：过滤器的403是过滤器自己的行为，而planned的取消路由已经声明了403。 - 过滤器语义。
403的两个分支是!tenantId.equals(actor.tenantId())和!validActorId；400 invalid_tenant是头缺失，或不匹配^[A-Za-z0-9._:-]{1,128}$。中英设计说明、Forbidden描述和生成类型都写了这两种403。/v1/agents/的尾斜杠与shouldNotFilter一致，没有把会跳过过滤器的POST /v1/agents说进去。 - A9。 中英设计说明都写了 Legacy 的
paused/pausing映射为waiting，TaskState不增加取值，与 #12847 A9 一致。 - 生成物。
managed-agent-api.ts只给两条已提供的 WebShell 任务读取（queryWebShellTasks、getWebShellTask）加了403，Forbidden的@description与契约原文一致。公共路由不在这份 WebShell 镜像里；planned的事件与取消路由也不会进入生成类型。 - 行为不变。 只增加响应声明。读不到任务仍然是
404（另一租户的X-Qwen-Tenant-Id），与已认证 actor 跨租户时的403 actor_scope_mismatch是两条路径。 - 与当前
main可以干净合并。#12968改的是 transcript 描述，和这处不重叠。契约版本仍应从 1.20.0 升到 1.21.0。
先前两轮意见已经落到代码里：planned 路由的 403 有测试守住，描述使用带尾斜杠的 /v1/agents/，并写明了非法 actor ID 这个第二触发条件以及过滤器的 400 invalid_tenant。
合入时留意
#12946 如果仍占用 1.21.0，并往同一行 info.description 追加文字，后合入的一方改成 1.22.0 并手工解决这段文本。其余 19 个未声明 403 的操作留在 #12976，不在本 PR 范围。
CI
Java 矩阵、fault gate、Test (ubuntu-latest, Node 22.x)、Lint 均已通过。提交本评审时 web-shell E2E Smoke 仍在跑。本 PR 没有 UI 行为变化，生成类型是否与契约一致已由 Node 测试覆盖。
English
Approve. Checked 032ea70 against current main (contract still 1.20.0), TenantContextFilter, both contract tests, and #12847 A9/A10. Nothing to change.
The six task routes declare 403 via the shared Forbidden response. The four mapped reads are probed with the resource tenant plus an actor from another tenant, so exchange() pins the declaration, the status, and the ErrorEnvelope. The two planned event routes are pinned by plannedTaskRoutesDeclareTheTenantFilterForbidden; the two cancels already declared 403, and including them is a regression guard. The extra event routes beyond the four partial reads named in #12847 are justified: the filter's 403 is a property of the filter, and the planned cancels already declared it.
Both 403 branches (tenant mismatch and !validActorId) and the filter's 400 invalid_tenant are named in both design notes, the Forbidden description, and the generated types. The trailing slash on /v1/agents/ matches shouldNotFilter and does not claim POST /v1/agents. A9 records paused / pausing → waiting with no new TaskState value. The generated WebShell types add 403 only on queryWebShellTasks and getWebShellTask, and the description matches the contract byte for byte. Server behavior is unchanged: an unreadable task is still 404.
The branch merges cleanly with current main. If #12946 still takes 1.21.0 and appends to the same info.description line, whichever lands second should move to 1.22.0 and resolve that text. The other 19 undeclared 403s stay in #12976.
Java matrix, fault gates, Test (ubuntu-latest, Node 22.x), and Lint are green. web-shell E2E Smoke was still running when this review was submitted; this PR has no UI behavior change, and generated-type freshness is covered by the Node test.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only scan — APPROVE
Head reviewed: 032ea70ee1ba3cc0e8e7a36946eb91c503cb787d
My approval of the previous head 982da70e was dismissed when this commit landed, so this is a fresh review of the current head rather than a restatement.
What changed since the dismissed approval
One commit, docs(managed-agent): Name both 403 triggers and the filter's 400, touching four files and no logic:
- both design docs (+21/−15 between them)
openapi/managed-agent-public-api.openapi.json— oneForbiddendescription stringweb-shell/.../generated/managed-agent-api.ts— the mirrored string
The six 403 declarations, ManagedAgentApiContractTest (+17) and PlannedTaskContractTest (+12) are byte-identical to the head I already reviewed, so the verification below carries over unchanged and I re-confirmed it against this head's file list.
The new wording is accurate against the filter
The substantive claim this commit adds is that the tenant filter's 403 covers an invalid actor ID and not only a tenant mismatch. I read TenantContextFilter at this head rather than taking the description's word: the missing-or-malformed X-Qwen-Tenant-Id path sets SC_BAD_REQUEST with the invalid_tenant envelope, which is the new 400 row the design doc adds, and the SC_FORBIDDEN / actor_scope_mismatch branch is gated on validActorId(tenantId, actorId), which constructs a WorkspaceActor — so an actor ID that does not validate reaches the same 403 as a cross-tenant actor. Both triggers named in the rewritten description are real, and the doc's new invalid_tenant row matches the filter's actual status and code.
The design doc also now records that from 1.21.0 the contract test requires the tenant filter's 403 on the four planned task routes because ManagedAgentApiContractTest cannot probe an unmapped operation. That is an accurate description of PlannedTaskContractTest.plannedTaskRoutesDeclareTheTenantFilterForbidden, so the prose and the tests no longer disagree about which routes are covered how.
Spec and generated client are in step
The Forbidden description in the OpenAPI document and the @description on the generated Forbidden component are the same rewritten sentence, so the mirror was regenerated rather than hand-edited and SDKs generated from it will carry the corrected text.
Carried-forward verification
Unchanged from the previous head and re-confirmed present here:
- The four mapped task routes each gain a real
exchange(drift, …, 403, …)case sending the correct tenant header with an actor principal from another tenant, so the403is produced by a request and checked against the spec through the drift map. - The four planned routes are pinned by declaration, each with a failure message naming the operation, so removing any one
403block reddens a named test. - The change is additive for callers: each hunk inserts a
403beside the existing400and404, nothing is removed, and a caller who cannot read a task still gets404. - The contract does not over-claim the filter's reach — both the description and the doc say
/v1/agents/, correctly excluding the collection path the filter skips, which is recorded as item B11 of #12847.
Historical blocking issues — none
There is no CHANGES_REQUESTED in this PR's history and no inline finding other than the author's own two [Suggestion]s against an earlier head, both fixed before the head I first approved. doudouOUC approved this exact head.
CI
Green at this head: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, Capture web-shell visuals, the full Java matrix, Runtime Broker and Managed Agent MariaDB / Java 21, Hosted process fault gates / MySQL 8.4 / Java 21, Real daemon E2E / Java 11 and both Desktop Shell lanes all pass. Only review-pr is pending, which is not a gating check. No failure is attributable to this PR.
3 participants
What this PR does
This settles the two items of group A in #12847 that can land at any time, ahead of the slices that map task events and cancel.
403. The tenant filter answers403 actor_scope_mismatchon every/v1/agents/and WebShell route when an authenticated actor belongs to another tenant thanX-Qwen-Tenant-Idor has an invalid ID. Session get and list, the lifecycle routes, the operation query and the Turn reads already declare it. The task list and detail on both surfaces,partialsince H0c (feat(managed-agent): Commit Stage H records and serve the task list (H0c) #12855), did not, and neither did theplannedtask event routes. All six now declare it, and the sharedForbiddenresponse says that it covers this code as well as an actor that can read a resource but may not operate on it. feat(managed-agent): Track the task contract gaps deferred from the H0a review #12847 names the fourpartialroutes; the twoplannedevent routes are included because the filter's403is a fact about the filter rather than a decision H3 owes, and theplannedcancel routes already declare403. A caller that cannot read a task still gets404. The contract becomes v1.21.0.pausedandpausingstates map towaitingand thatTaskStategains no state. H0c already madeTaskStatepartialwith its eight values, so a new value would now be a breaking change.invalid_tenantandactor_scope_mismatchand no longer says that the read routes declare no403, in both languages.403on the four served task routes, as it already does on the Session and Turn reads. The contract test only probes mapped routes, so a separate check requires the403on the fourplannedtask routes (both event queries and both cancels).Why it's needed
The server already returns this
403on the task routes; the contract did not say so. Clients that follow the contract therefore do not expect it on a task read, and the contract test could not probe it, because it refuses a probe for a status the route does not declare. Declaring a response is additive, so #12847 notes that this can land at any time. A9 was decided in #12847 before H0c madeTaskStatepartial, but the note still said that the adapter slice would map the Legacy states "most likely towaiting, or the design adds a state".Reviewer Test Plan
How to verify
403withactor_scope_mismatch, which the contract now declares. Reading another tenant's Session still answers404.403response and the description of theForbiddenresponse changes; nothing else does.Evidence (Before & After)
N/A (no visible UI change).
Local results on Linux:
mvn testpasses (179 tests), and so does Checkstyle.403from the WebShell task detail route fails the contract test withgetWebShellTask declares 403, and removing it from theplannedWebShell task event query fails the planned-task check withqueryWebShellTaskEvents declares 403.mainonly by that403and description; the types test passes (2/2) and fails against the old types, andnpm run typecheckpasses.Tested on
Environment (optional)
JDK 25 running the Java 21 build with Maven; Node 22.
Risk & Scope
plannedtask events and cancel.403on the other 19 operations behind the tenant filter:implementedgetSessionEvents,webShellStreamEventsandwebShellTranscript;partiallistItems,listWorkspaces,getWorkspace,webShellQueryWorkspaces,webShellGetWorkspace,postSessionEvent,updateSession,unarchiveSession,webShellSubmitTurnandwebShellCancelTurn; andplannedlistArtifacts,getArtifactContent,changeSessionCwd,webShellChangeCwd,getAgentandupdateAgent. They are outside feat(managed-agent): Track the task contract gaps deferred from the H0a review #12847 and tracked in feat(managed-agent): Declare the tenant filter's 403 on the remaining filtered routes #12976.403declared on the MCP catalog, hook catalog, channel and automation routes, whose meaning H1, H2, H5 and H6 define (feat(managed-agent): Implement private Hosted MCP runtime (H1) #12946 for the MCP catalog).main's v1.20.0. feat(managed-agent): Implement private Hosted MCP runtime (H1) #12946 also takes v1.21.0 and appends to the sameinfo.description, so whichever merges second moves to v1.22.0 and resolves that textual conflict.Design note: English and 中文, both updated in sections 4.7 and 7.
Linked Issues
Related #12847 (A9 and A10). Part of #12827.
中文说明
这个 PR 做了什么
本 PR 处理 #12847 A 组中随时可以落地的两项，不必等映射任务事件与取消的切片。
403。 当已认证的 actor 属于X-Qwen-Tenant-Id以外的租户或其 ID 非法时，租户过滤器会在每条/v1/agents/与 WebShell 路由上返回403 actor_scope_mismatch。Session get 与 list、生命周期路由、operation 查询和 Turn 读取路由都已声明它。自 H0c（feat(managed-agent): Commit Stage H records and serve the task list (H0c) #12855）起为partial的两个表面上的任务列表与详情没有声明，planned的任务事件路由也没有。现在这六条路由都声明了它，共用的Forbidden响应也写明它既涵盖这个错误码，也涵盖能读取资源但无权操作的 actor。feat(managed-agent): Track the task contract gaps deferred from the H0a review #12847 列出的是四条partial路由；两条planned事件路由一并处理，因为过滤器的403是关于过滤器本身的事实，不是 H3 需要做的决定，而planned的取消路由已经声明了403。无权读取任务的调用方仍然得到404。契约版本变为 v1.21.0。paused与pausing状态映射为waiting，TaskState不增加状态。H0c 已把TaskState连同其八个值标为partial，此后再增加一个值就是破坏性变更。invalid_tenant与actor_scope_mismatch，不再写"只读路由不声明403"。403，与 Session 和 Turn 读取路由的做法相同。契约测试只探测已映射的路由，因此另有一项检查要求四条planned任务路由（两条事件查询与两条取消）声明403。为什么需要
服务端本来就会在任务路由上返回这个
403，只是契约没有写出来。因此按契约编写的客户端不会预期任务读取返回它，契约测试也无法探测它，因为测试拒绝探测路由未声明的状态码。声明一个响应是增量变更，所以 #12847 写明这项随时可以落地。A9 在 H0c 把TaskState变为partial之前就已在 #12847 中决定，但设计说明仍写着适配切片会把 Legacy 状态"最可能映射为waiting，或由设计增加一个状态"。评审测试计划
如何验证
403与actor_scope_mismatch，契约现在声明了它。读取另一个租户的 Session 仍返回404。403响应，Forbidden响应的说明随之更新，其余不变。证据（前后对比）
不适用（没有可见的 UI 变化）。
在 Linux 上的本地结果：
mvn test通过（179 个测试），Checkstyle 也通过。403，契约测试失败，报getWebShellTask declares 403；去掉planned的 WebShell 任务事件查询的403，planned 任务检查失败，报queryWebShellTaskEvents declares 403。main相比只差上述403与说明；类型测试通过（2/2），换回旧类型则失败；npm run typecheck通过。测试平台
环境（可选）
JDK 25 运行 Java 21 构建，使用 Maven；Node 22。
风险与范围
planned的任务事件与取消。403：implemented的getSessionEvents、webShellStreamEvents与webShellTranscript；partial的listItems、listWorkspaces、getWorkspace、webShellQueryWorkspaces、webShellGetWorkspace、postSessionEvent、updateSession、unarchiveSession、webShellSubmitTurn与webShellCancelTurn；以及planned的listArtifacts、getArtifactContent、changeSessionCwd、webShellChangeCwd、getAgent与updateAgent。它们不在 feat(managed-agent): Track the task contract gaps deferred from the H0a review #12847 范围内，由 feat(managed-agent): Declare the tenant filter's 403 on the remaining filtered routes #12976 跟踪。403，其含义由 H1、H2、H5 与 H6 定义（MCP catalog 见 feat(managed-agent): Implement private Hosted MCP runtime (H1) #12946）。main的 v1.20.0 之后。feat(managed-agent): Implement private Hosted MCP runtime (H1) #12946 也使用 v1.21.0，且同样在info.description末尾追加内容，后合入的一方改为 v1.22.0 并解决这处文本冲突。设计说明：English 与 中文，第 4.7 节与第 7 节均已更新。
关联 Issue
Related #12847（A9 与 A10）。属于 #12827。