Skip to content

fix(managed-agent): stop a bound Turn under refused authorization - #13163

Open
yiliang114 wants to merge 123 commits into
mainfrom
fix/13162-cancel-refused-authorization
Open

yiliang114 wants to merge 123 commits into
mainfrom
fix/13162-cancel-refused-authorization

Conversation

@yiliang114

@yiliang114 yiliang114 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

The creator of a Workspace-bound Session can cancel an admitted running Turn after creation permission is revoked, the Workspace starts draining, or its registration changes, provided read access remains and the deployment enables Workspace files. Cancellation can recover the original Harness attachment after the Java cache is cleared. An already accepted cancellation continues retrying after subsequent read revocation; new API calls still require creator identity and read access.

New submit and rename requests require the Session’s bound generation and storage to remain current and refuse before recording a command when they do not. Recorded submit and completed rename outcomes remain readable to authorized same-key retries. WebShell keeps Cancel available when new-work capability becomes false.

Why it's needed

Cancellation inherited the authorization needed to start new work, leaving creators unable to stop an admitted Turn. Cold attachment recovery now validates the persisted cancellation and frozen Session identity, then reuses the original resident connection. Parked recovery adopts the original Runtime lease and reads status without preparing or executing work. An acquired identity is recorded immediately and retained through retryable cancellation failures until success or teardown; a concurrent teardown that strands an adoption is diagnosed. Ordinary attachment and new-work authorization retain their checks, and cancellation retains the active, undeleted Session requirement.

Reviewer Test Plan

How to verify

  • Start a creator’s bound Turn, revoke create while retaining read, drain the Workspace, and clear Java’s attachment cache. Cancel through WebShell or the public API: the original Turn should become cancelled, with one completed cancellation command and one terminal event.
  • Revoke read before a new cancel request: it should return 404 session_not_found and write no command. An already accepted cancellation should continue retrying. Non-creators, disabled deployments and closed Sessions retain their refusals.
  • Change generation and storage independently. Submit/rename should refuse before command creation and advertise workspaceTurns=false; creator cancellation should remain admitted.
  • Passive recovery must preserve the original identity, profile and client connection. Hold two overlapping recoveries, settle only the first, and verify teardown still refuses without releasing the Runtime lease. Once both finish or fail, teardown should release it once.
  • After a failed rename, complete a different rename, then retry the original key. A failing sibling must not prevent the newest successful request from committing. An older retired request must still refuse after a later rename completes.
  • After cancellation succeeds, recover the same Session: its old running Turn snapshot should be absent. A failed cancellation admission must retain recovery information for retry.
  • Answer an approval after its bound Workspace becomes permanently unavailable: the operation should fail promptly with workspace_unavailable. Retryable refusals should retry, and an already committed decision should remain settled after a lost reply.
  • In a normal ACTIVE Workspace, use real WebShell Send and Cancel: Running / Environment Ready should become Cancelled with the composer restored and no visible error.

Evidence (Before & After)

Current CI follow-up head: ce56f4395766f8c5bbd8e2aff070a3e225b6a31e. The CI/review follow-up fixes independent port allocation in the Hosted IT and adds startup diagnostics, with no production behavior change. Focused verification passes 66 distinct Java checks and two CLI overlap cases, with Checkstyle zero; the native cwd IT uses H2, current source Harness/worker and a controlled model. New-head MySQL/WebShell CI and human re-review remain pending. The previous production repair and verification report addresses three new Critical findings and one cancellation/DELETE correctness defect. Resident recovery preserves terminal refusal reasons and reattaches requested approvals; payable terminal projections settle, while unpayable work retains its refusal and owed lease. Approval delivery retries temporary mount I/O without changing shared acquire verdicts or new-work authority. A delayed cancellation load refuses a deleted or draining attachment after settlement.

Final focused CLI regression tests pass 12/12. Java compiled the production and test sources; 78 distinct focused checks passed across the consolidated run and a one-test fixture correction, with zero Checkstyle violations. CLI typecheck and focused ESLint pass. A controlled Java/H2/Flyway probe reproduces approval loss at f2864f6a and confirms retry/decision after a temporary mount failure with the delivered connector. These results are controlled route/native evidence, with no fresh external-model/browser/MySQL/MariaDB/process-death/physical-tool acceptance claim for this head. The report identifies the initial failures and subsequent focused verification rather than claiming a fresh full-suite run.

The round-7 real-stack report remains attributed to f2864f6a. Its inherited V48 rename ordering and surviving mutation gaps are recorded under #13269; optional R2-4/R2-6 suggestions remain follow-ups under the Critical-only rule after more than five rounds. The canonical scope ledger retains previous evidence and records this round's scope. CI and re-review for the delivered head are pending.

The real WebShell/tmux report with four screenshots belongs to b683a3d62a84: actual normal and revoked-create/DRAINING/cold-Java-cache Send/Cancel pairs returned 202, each cancellation settled once, and the UI stayed Environment Ready without an error through the recorded 179.154-second protected observation. The Linux real-stack feedback belongs to 25eb9ae and motivated the overlapping-recovery fix. Neither report is relabelled as fresh whole-runtime acceptance of the current head. Earlier native/UI and synchronization reports retain their tested SHAs and limitations.

Tested on

OS Evidence and attribution
macOS Current controlled Express/Broker and native Java21/Spring/H2 verification; real browser/tmux evidence is historical b683a3d
Linux Real-stack feedback on 25eb9ae; current-head CI including MySQL/MariaDB is pending
Windows Historical SDK/desktop CI; current-head CI pending, no fresh local UI claim

Risk & Scope

Linked Issues

Addresses #13162 items 1–3. Follow-ups: #13269, #13054, #13083, #13413 and #13162 items 4/5. Prerequisite #13112 has merged.

中文说明

最新 CI 跟进提交为 ce56f4395766,只修 Hosted Java IT 的独立端口分配与启动诊断,不改产品权限或恢复行为。Java 66 个不同用例、CLI 并发恢复两例及 Checkstyle 通过;原生 cwd IT 使用 H2、当前源码 Harness/worker 与受控模型。WebShell 首轮因 runner 连续五次未接单而未执行;新的 MySQL/WebShell CI 与人工复审仍待完成。本轮报告说明端口碰撞机制、旧编译缓存失败及修正后的验证。以下保留先前生产修复与历史证据的归属。

Workspace绑定会话的创建者,在仍有读取权限且部署启用Workspace文件能力时,创建权限被撤销、Workspace进入DRAINING或注册信息改变后,仍能取消已受理的运行中Turn。Java缓存丢失时恢复原始可用连接,已受理取消在随后撤销读取权限后继续重试,新请求仍检查创建者与读取权限。新的提交、改名要求绑定generation/storage有效;授权同key重放保留已有结果。冻结身份/profile及会话生命周期检查保留。

本轮 2748c36 正常合入主线与并发已发布的同步提交,并修复三项 Critical:取消受理成功后清除旧恢复快照,失败受理仍保留重试信息;重叠恢复用例先确认第一个请求抵达门闸,再启动第二个请求,直接检查409、不释放以及最终只释放一次;审批遇到明确不可重试的 Workspace 拒绝后及时记录失败,可重试拒绝和已经落库的决定保留原有行为。上一轮并发围栏、改名回执及内部可空迁移的证据仍归属原提交。

本轮验证报告记录相关 core/CLI 构建、CLI 类型与针对性 lint 通过;Java 原生用例106/106,Checkstyle/SpotBugs零问题;CLI 框架报告244/244。两项运行时缺陷都有仅去掉修复便准确失败的受控反证。旧 Shell 后台 inventory 断言仍被异常处理吞掉,不能计作有效工具清单验收;新并发断言直接 await,不受此限制。

首次推送后快照(2026-10-07 13:14 +08)无冲突:5项检查成功、3项跳过、17项待完成、无失败,有效批准0/2,GitHub仍为BLOCKED/CHANGES_REQUESTED。本轮三项Critical随后均已附证据解决;其余21条Suggestion已逐项链接记录为后续。超过五轮后只继续确定的Critical修复,最新CI与维护者审批仍待完成,范围台账保留历史归属。

真实浏览器/tmux四图报告仍归属b683,Linux真实栈反馈仍归属25eb,未重标为当前head完整验收。本轮只证明受控Broker的Express路径及实际Spring/JDBC/Flyway/H2,不代表最新浏览器、真实模型、MySQL/MariaDB或进程死亡验收。拒绝Broker create/ACTIVE时的parked认领仍由#13054跟踪,continuation/redrive与丢失worker为#13083,私有Harness写顺序为#13269,Session Store瞬断后写停为#13413;挂载准入与暂态IO分类仍为#13162第4/5项。历史refused-warm及非创建者控制没有在本轮重新验收。

代码修复、CI、真实运行栈验收与外部批准分别记录;本轮没有合并或自批PR。

yiliang114 and others added 21 commits September 30, 2026 22:29
…or its creator

G0 admits only the initial file-tool Turn created with a Workspace-bound
Session; every later public submit is refused with workspace_unavailable,
so a Hosted Session cannot hold a conversation.

Admit a later Turn when the deployment's Workspace-files opt-in is on
and the submitter created the Session. Execution already authorizes
each Turn against the creator's Workspace grants
(WorkspaceExecutionStore.authorize), so letting another actor submit
would run tools under the creator's authority; every other actor keeps
the existing refusal (404 without read access, 409 otherwise). The
store admits a bound Session's later Turn only under the same opt-in.
Cancel, rename, lifecycle and cwd operations stay gated.

- ManagedWorkspaceRegistry.createdSession looks up the creator in
  managed_workspace_create_command.
- HostedPublicWorkspaceIT: a reader who is not the creator is refused;
  the creator's second Turn runs write, edit and read again through the
  real Broker and worker and completes.
- ManagedWorkspaceAdmissionTest: the enabled store admits the later
  Turn and the creator lookup distinguishes actors and tenants.
- The Workspace-binding capability description (contract and generated
  WebShell types), the README and both G0 design documents describe the
  new boundary. The contract version is left for #13101, which takes
  1.25.

Not built or run locally.
The server now admits later Turns from a Workspace-bound Session's
creator, but the Managed panel still hid the composer for every bound
Session and the Java provider forced canSend to false.

- WebShellSessionCapabilities gains workspaceTurns, computed per Session
  and caller with the same rule the submit path enforces (opt-in on,
  caller can read, caller created the Session). The contract and the
  generated WebShell types describe it.
- The Java provider sends for a bound Session only when the service
  reports workspaceTurns, and reports it in the summary only when true.
- The Managed panel shows the composer for such a Session; cancel stays
  hidden for bound Sessions.
- HostedPublicWorkspaceIT checks the capability per caller through the
  WebShell route; the contract test's WebShell capability shape and
  provider/page tests cover the client.
Later Turns of a Workspace-bound Session could be submitted by the
creator but not cancelled: the service and store refused the cancel,
and the coordinator skipped bound Sessions entirely.

Apply the same rule to cancellation. The creator, with the Workspace-
files opt-in on, may cancel; the coordinator then cancels through the
Hosted Harness like any other Turn, which aborts the Turn and settles
its Runtime calls through their original identities. Other actors and
deployments without the opt-in keep the existing refusal, and the
coordinator still leaves a bound Session alone without the opt-in.

- HarnessCoordinatorTest: without the opt-in only the opt-in is read;
  with it, the bound Turn is cancelled through the Harness.
- HostedPublicWorkspaceIT: a later Turn is held in the model call; a
  reader's cancel is refused, the creator's cancel is accepted, and the
  Turn ends CANCELLED with no tool execution.
- WebShell reports canCancel for such a Session when workspaceTurns is
  set, so the Managed panel shows Cancel.
- Contract text, generated types, README and both G0 design documents
  now keep only lifecycle operations gated.
Rename of a Workspace-bound Session answered 409 until a role source
decided who may change it (D4 4.9). #12867 settled the first version:
the Session's creator is its only owner. Rename changes only the title
through the Hosted Harness and touches no Runtime, so apply the same
creator rule under the Workspace-files opt-in; the store admits only a
RENAME mutation of a bound Session, and only under the opt-in.

Close, archive, delete and unarchive stay gated: the embedded Broker's
drain only stops warming a closed Session and has no Harness-level
teardown yet, so closing a bound Session would leave its worker and any
held Workspace lease behind.

HostedPublicWorkspaceIT: a reader's rename is refused; the creator's
rename returns the new title.
…etadata

PublicSession carries its title under metadata.title; the rename
assertion read a top-level title and saw an empty string although the
rename returned 200.
Main's permission Actions work (#13101) and this branch's Workspace-bound
later Turns both extended WebShellSessionCapabilities with a second field,
so the record, its single construction site, the generated WebShell types
and the cross-tenant contract assertion each need the union rather than one
side.

- ApiModels: the record is now (tasks, actions, workspaceTurns).
- ManagedAgentService: pass both hasActions(session) and
  maySubmitWorkspaceTurn(session, actorId).
- managed-agent-api.ts: emit actions then workspaceTurns, matching the
  contract's property order now that neither field is planned; actions stays
  required and workspaceTurns optional per the schema's required list.
- ManagedAgentApiContractTest: the foreign-tenant session asserts both
  capabilities as false.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-conflict/jmuob3c85i1
Resolves the single content conflict in ManagedWorkspaceAdmissionTest.java.
Both sides added a different @test method at the same location, so both are
kept:

- PR side: enabledStoreAdmitsALaterTurnForTheBoundSessionCreator
- main side (from 51b80da): enabledCreationRefusesPolicyDriftAndAnotherTenantsMount

main's change to the 3-arg register() helper is absorbed unchanged; the
resolved file is a strict superset of origin/main for this path (0 removed
lines, +40 = the PR's test method plus its @Test/close-brace seam).

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-conflict/jmuojnzcnif
…rn admission

The G0 section and Prerequisites still published the pre-follow-up gate
("later submit/cancel remain gated"), contradicting the boundary
paragraph this PR adds; state the creator's submit/cancel/rename
admission and keep close/archive/delete/unarchive and cwd gated. Scope
the follow-up's refusal precisely: requireLegacyWorkspace answers
workspace_unavailable only when the actor can read the Workspace, and
session_not_found otherwise. Apply the same two corrections to the
bilingual design doc and note the follow-up's composer UI change.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuoob5v90j
The deferral resolved: #13101 merged and took 1.25.0, and #13117 took
1.26.0. Record this PR's contract change per the version-history
convention: creator later-Turn admission and the workspaceTurns
capability.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuoob5v90j
…an send later Turns

The workspace-binding banner always stated that message execution is
unavailable, contradicting the composer this PR enables for the creator
of a bound Session with workspaceTurns capability. Gate the third
banner paragraph on !capabilities.workspaceTurns so the page no longer
states both. Pin both branches in ManagedSessionsPage tests.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuoqgbnv0l
…trols

Three of the four clauses this PR adds to bound-Session admission could
be deleted with the suite staying green:

- ManagedWorkspaceRegistry.createdSession's session_id column: create a
  second bound Session by actor-b under a distinct idempotency key and
  assert createdSession(tenant, actor-a, otherSession) is false, so the
  lookup must match this Session, not any Session the actor created.
- maySubmitWorkspaceTurn's harness.isWorkspaceFilesAvailable() clause:
  assert the bound-session web-shell GET reports
  capabilities.workspaceTurns == false while the opt-in is off.
- boundRenameAllowed's kind conjunct: assert beginSessionMutation with
  UNARCHIVE on the enabled store throws workspace_unavailable, and add
  the positive insertCancelCommand control so the cancel path stays
  open for bound Sessions under the opt-in.

Each new assertion was verified to go red under the corresponding
mutation (session_id dropped, flag clause dropped, kind conjunct
dropped, cancel conjunct dropped) and the suite returns green with the
sources restored.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuoqgbnv0l
…pin canRead

HostedPublicWorkspaceIT changes:

- register() now grants the reader in both runs (can_create only in the
  files run), removing the access-table primary-key collision that
  forced the approvals run to skip the later-Turn block.
- The approvals run now drives a later Turn through answerActions and
  asserts it completes, so a later Turn admitted under
  approval-mode=default is covered; the cancel/rename probes keep their
  held model reply and stay in the files run to fit the method timeout.
- proof.txt is reset to a sentinel just before the later-Turn submit, so
  the post-Turn "after" assertion can fail if the later Turn's write and
  edit stop reaching the bound root (R1-13).
- After the rename check, revoking the creator's read grant asserts
  submit, cancel and PATCH all answer 404 session_not_found, pinning
  the canRead clause of maySubmitWorkspaceTurn (R1-12 clause 3).
- Model-request counts become 16 (approvals) / 18 (files) and the
  approvals run now answers 8 actions across the two Turns.

Compile-verified via mvn test-compile. Execution requires the bundled
dist/cli.js plus a MySQL/hosted-Harness stack, which this environment
cannot provide; behavior was traced against requireSubmitter /
requireLegacyWorkspace / boundRenameAllowed and the fixture request
model, and CI remains the executor.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmuoqgbnv0l
…ution authority

Round-2 review of #13112 found the admission predicate certifying Turns
that execution can never run. maySubmitWorkspaceTurn now also requires the
predicates the cited authority fixes at creation (Session ACTIVE, the
qwen-code agent, the frozen execution profile refs) and the creator's
can_create on an ACTIVE registry, read through the same grant row shape;
a can_read-revoked creator still falls through to the documented 404.
Cancellation attaches passively so an abort no longer depends on the
physical mount still verifying. renameSession answers a non-retryable
refusal with its own status and code instead of a transient 503.
requireSubmitter loads the Session once per call, and the session read
paths skip the canRead probe their entry already established. The OpenAPI
contract, README and both design docs now state the creator admission,
its grants qualifier and the still-gated lifecycle/cwd operations
consistently, and the generated WebShell types are regenerated from the
corrected contract. Tests pin the bound rename admission, the two
empty-creation profile escapes and the can_create-revoked arm.

Co-authored-by: Qwen-Coder <[email protected]>
…er-turns

Conflicts — the branch's workspaceTurns capability and main's artifacts
capability touched the same three shapes; merged as unions:

- WebShellSessionCapabilities record + constructor + contract-test JSON
  (sdk-java): fields now tasks, artifacts, actions, workspaceTurns.
- openapi.json: main's v1.27 (O3 artifact reads) kept, the branch's
  workspaceTurns entry renumbered to v1.28 and the document version
  bumped to match.
- web-shell provider + test: both capability flags on the wire shape;
  both new tests kept (the bound-Session admission pair and the
  download-cancellation one).

Verified here: the web-shell provider suite (17/17) and tsc pass. The
sdk-java build/tests run in CI (this box has no mvn).

Co-authored-by: Qwen-Coder <[email protected]>
26de98c made the live cancel attach passively. A passive attach reloads
the Session in the Hosted Harness instead of reusing the running Turn's
attachment, so the abort never reached the Turn and HostedPublicWorkspaceIT
timed out with the Turn still CANCELLING. Restore the ordinary attach for
the live path; cancellation recovery, which has no live attachment, keeps
attaching passively. Cancelling under a refused Workspace authorization
stays a follow-up.
Follow-ups to #13112 from its round-4 verification (#13162).

Cancelling aborts work that is already running, so it no longer needs the
grants that admit new work: the creator who can still read the Workspace
may cancel while can_create is revoked, the Workspace is draining or it was
re-registered. A live cancel reuses the running Turn's attachment and runs
no Workspace authorization; only without one does it fall back to the
existing attach. A cancel the Harness did not take is re-sent with backoff
while the Turn is still CANCELLING, since the running dispatcher checks
CANCELLING only once.

Admission of new work now also requires the Workspace generation and
storage the Session was bound to, so a re-registration refuses submit and
rename synchronously, before any command row is written. Tests pin the
opt-in clause, the creator cancel under revocation and re-registration, the
live-attachment path and the resend, and the IT cancels a running Turn after
revoking the grant and draining the Workspace.
yiliang114 and others added 3 commits October 1, 2026 22:32
The bound-Session cancel path attaches through the strict 3-arg createOrLoad.
When the Workspace authority, or the storage guard behind it, refused that
attach, the RuntimeException fell into the catch that promises recovery: no
harness.cancel was issued for a cancel the API had already answered 202 for,
no sweeper re-claims a Turn whose lease the live consumer keeps renewing, and
the Turn ran on and settled COMPLETED. Settle the Turn with the refusal's own
code instead of dropping the cancel silently.

Addresses review thread R2-1 (PRRT_kwDOPB-92c6n-tVW).

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmupkgkl21b
…g rename

The permanent-refusal branch in renameSession rethrew a 4xx after
beginSessionMutation had already written a PENDING RENAME_SESSION row. Nothing
retires that row, so every later rename with a fresh key died in
requireNoOpenOperation with session_operation_active for the Session's life.
abandonSessionMutation deletes the still-PENDING row before the refusal is
answered, so the Session accepts the next rename again.

Addresses review thread R3-1 (PRRT_kwDOPB-92c6n-tVl).

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmupkgkl21b
… published rule

maySubmitWorkspaceTurn's canRead operand, readGranted flag and 2-arg/3-arg
overload pair cannot change any result: findReadable already joins the same
access row with can_read = TRUE, and createdSession still runs first so an
actor id the registry key cannot encode keeps answering false instead of
throwing. The README's later-Turn rule now names the create grant and the
registry's ACTIVE state, as the OpenAPI capability text already does.

Addresses review threads R1-8 (PRRT_kwDOPB-92c6n-tVt) and R1-5
(PRRT_kwDOPB-92c6n-tV0, PRRT_kwDOPB-92c6n-tV-).

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-closeout/jmupkgkl21b
yiliang114 and others added 2 commits October 1, 2026 22:50
#13112 gained c809439, which fails a bound Turn when the cancel attach is
refused. Failing it in Java does not stop the Turn in the Hosted Harness, so
the Turn would keep writing while it reads as FAILED. Keep this branch's
behavior instead: cancel through the live attachment, which needs no
Workspace authorization, and re-send a cancel that was not delivered while
the Turn is still CANCELLING. Take #13112's single admission method, which
already carries the generation check, and its rename-command retirement.

@qwen-code-review-bot qwen-code-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

18 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-3 loose acquire wait in the stranding test — already reported (comment 4202227737)
  • R1-6 canCancel lost its server-published discriminator — already reported (comment 4202227739)
  • R1-7 recovery-identity rule duplicated three times — already reported (comment 4202227743)
  • R1-8 gate sequence re-reads rows it already fetched — already reported (comment 4202227746)
  • R1-34 bindingCurrent second SELECT on the same key — already reported (comment 4202227750)
  • R1-10 interleaved ternary SQL in authorizeAttachment — already reported (comment 4202227755)
  • R1-35 row-mapper ternary indentation — already reported (comment 4202227758)
  • R1-12 writer promise in the reattach test unpinned — already reported (comment 4202227764)
  • R1-14 silent bare catch on the primary reattach path — already reported (comment 4202227769)
  • R1-18 new refusal code asserted at store level only — already reported (comment 4202227781)
  • R1-20 same-key cancel replay ordering — already reported (comment 4202227787)
  • R1-21 cold test does not constrain the authorization spy — already reported (comment 4202227791)
  • R1-22 hooks Session on the passive resident load untested — already reported (comment 4202227797)
  • R1-23 duplicate authorize per new-work call — already reported (comment 4202227800)
  • R1-26 PENDING assertion blind to the submit leg — already reported (comment 4202227803)
  • R1-28 guard identity comparisons never true in tests — already reported (comment 4202227814)
  • R1-29 refusedAdoptions not drained on the close path — already reported (comment 4202227819)
  • R1-33 standalone Cancel render site unpinned — already reported (comment 4202227834)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": whether cancelAdmittedTurn 's per-tick retry is bounded anywhere above it (I read :859-934 and the renewal scheduler registration at :174-192 , but not the …; "agent reverse-audit (round 3)": whether harness.session-store.base-url can legitimately differ between API replicas or be rotated against a live daemon (I confirmed only that it is read from…; "agent reverse-audit (round 1)": did not compile or run ManagedWorkspaceAdmissionTest — every conclusion above rests on source reads of the test, ManagedAgentService , ManagedWorkspaceRegis…; "agent reverse-audit (round 1)": whether doRecoverManagedRuntime has an exit that returns without attachments.put — the only path on which the new cancellationAttachment could hand clien….

Not reviewed: reverse audit — stopped before round 4 by the review time budget.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • docs/design/2026-09-29-hosted-public-workspace-admission.md:43 — [review] The new design-doc invariant — a passive resident load…
  • packages/cli/src/serve/hosted-harness-session.test.ts:8960 — [review] The refusal-kind matrix drives two *different* exit arms…
  • packages/cli/src/serve/hosted-harness-session.test.ts:9191 — [review] refuses a teardown that lands after a resident passive…
  • packages/cli/src/serve/hosted-harness-session.test.ts:9272 — [review] Both new final-authorization fault-injection tests are…
  • packages/cli/src/serve/hosted-harness-session.ts:1957 — [review] The new resident drive-redrive arm has no up-front…
  • packages/cli/src/serve/hosted-harness-session.ts:1962 — [review] Splitting the resident re-answer into a drive arm and a…
  • packages/cli/src/serve/hosted-harness-session.ts:2021 — [review] The drive-redrive arm awaits recoverHostedRuntimeTurn …
  • packages/cli/src/serve/hosted-harness-session.ts:2060 — [review] The new resident passive arm re-derives the same fact from…
  • packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/store/WorkspaceExecutionStore.java:104 — [review] authorizeCancellation 's new turn-state disjunct…
  • packages/sdk-java/managed-agent-server/src/test/java/com/alibaba/qwen/code/managedagent/HostedPublicWorkspaceIT.java:670 — [review] The rebinding probe's restore snapshots storage_id into…
  • packages/sdk-java/managed-agent-server/src/test/java/com/alibaba/qwen/code/managedagent/harness/QwenHostedHarnessColdCancelRegressionTest.java:107 — [review] The mount-readiness half of the cancellation contract this…
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 18 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 3)":whether cancelAdmittedTurn 's per-tick retry is bounded anywhere above it (I read :859-934 and the renewal scheduler registration at :174-192 , but not the …;"agent reverse-audit (round 3)":whether harness.session-store.base-url can legitimately differ between API replicas or be rotated against a live daemon (I confirmed only that it is read from…;"agent reverse-audit (round 1)":did not compile or run ManagedWorkspaceAdmissionTest — every conclusion above rests on source reads of the test, ManagedAgentService , ManagedWorkspaceRegis…;"agent reverse-audit (round 1)":whether doRecoverManagedRuntime has an exit that returns without attachments.put — the only path on which the new cancellationAttachment could hand clien…。

未审查:反向审计——评审时间预算不足,未能开始第 4 轮。

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 11 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.25.0)

Comment thread packages/cli/src/serve/hosted-harness-session.ts
Comment thread packages/cli/src/serve/hosted-harness-session.ts Outdated
Comment thread packages/cli/src/serve/hosted-harness-session.test.ts
Comment thread packages/cli/src/serve/hosted-harness-session.ts
yiliang114 and others added 2 commits October 7, 2026 21:35
Preserve terminal decline reasons and reattach requested approvals. Settle payable terminal projections without retrying settled file history, and refuse a cancellation attachment removed during settlement.

Co-authored-by: Qwen-Coder <[email protected]>
Use delivery-time mount verification for action responses and recover cold approval attachments passively after rechecking new-work authority. Preserve shared acquire verdicts and permanent workspace refusals.

Co-authored-by: Qwen-Coder <[email protected]>
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Delivered a9f7fec138ee1c132f9440bee2904cee1acabb47 as two additive commits (0227b2501474, a9f7fec138ee). This addresses the new Request changes at f2864f6a without changing the cancellation/new-work authority boundary.

Finding Disposition and observable behavior
R2-1 Fixed. A resident kernel decline preserves hosted_turn_recovery_declined and its reason; it no longer invites the deterministic refusal to retry as an ordinary attach conflict.
R2-2 Fixed. Requested approvals reattach on both resident load arms using the same client ID, with the obsolete recovery latch cleared. Payable terminal projections write the missing transcript terminal. The passive fence covers the authorization/projection decision; unpayable projections still refuse and retain the owed lease. Verification also exposed the shared projection predicate checking already-settled file history as though it were pending: it now applies that check only to pending Turn/Undo obligations, retaining the checkpoint/tool/transcript guards.
R2-3 Fixed at delivery. An action response rechecks the same new-work authority, uses the existing probe classification for mount reads, and restores a cold attachment passively. Temporary I/O re-arms delivery; structural refusals remain terminal. Shared acquire/tool-turn classification is unchanged. Revoked creation authority and DRAINING retain the R1-5 refusal; this does not resume work through a revoked grant.
R2-5 Fixed as a correctness defect despite its Suggestion label. A DELETE admitted during cancellation settlement still returns 204; the delayed load now returns 404 instead of handing back a deleted attachment. A draining resident returns the existing closing refusal.

Verification on the delivered sources:

  • Final focused CLI run: 12/12, including resident decline, both approval/projection arms, cancellation-vs-DELETE, adoption cleanup and unpayable controls. The unchanged recovery companion suite passed 44/44 in the initial consolidated run. The full initial two-file run found four failures (two affected authorization-count fixtures and two new projection cases); these were corrected and are covered by the final focused run. This is not a claim of a fresh full-suite run after the correction.
  • Java: 78 distinct focused tests passed across the consolidated run and the one-test fixture correction: connector 39, actions 25, existing storage 13, plus the new real-storage-guard/connector mount case. Production and test sources compiled; 0 Checkstyle violations.
  • CLI typecheck and two-file ESLint passed. The initial typecheck resolved minimatch 3 through this worktree's shared dependencies; reusing its cached declared-major version 9 locally fixed that environment mismatch, with no manifest/lockfile change.
  • Controlled native Java/H2/Flyway probe at f2864f6a: one temporary IOException produced 1 attempt, 0 retries, FAILED, leaving the action requested even after recovery. Identical candidate probe: 1 retry, then 2 attempts total, decided, one Harness send after clearing the blip. Healthy control decides once; shared acquire's terminal classification remains unchanged.

The route tests use controlled model/Broker behavior. The native mount probe uses the real storage guard and H2/Flyway with controlled authority/outbox/client boundaries. No fresh external-model, browser, MySQL/MariaDB, process-death or physical-tool interruption acceptance is claimed for this head. Round 7 remains evidence at its tested f2864f6a.

R2-4 and R2-6 remain optional follow-ups under the Critical-only rule after more than five rounds. Round 7's inherited V48 rename ordering and surviving mutation gaps are recorded in #13269; #13054/#13413 retain their separate ownership. These are not claimed fixed by this delivery. CI and re-review for the new head are pending; this report does not clear the GitHub review gate.

中文版

本轮以两个增量提交交付到 a9f7fec138ee,处理新增 Request changes 的三条 Critical,以及虽标为 Suggestion、但会返回已删除连接的 R2-5。恢复拒绝会保留终态错误码与原因;两种常驻恢复都能重新附着等待审批的会话,并补写可结算的终态记录;短暂挂载 I/O 故障会让审批投递重试,结构性拒绝与撤权语义保持原规则;取消结算期间已获准的 DELETE 仍返回 204,之后的 load 返回 404,不再返回失效连接。

最终 CLI 针对性用例 12/12 通过;未改动的恢复辅助测试在首次集中运行中 44/44 通过。首次两文件集中运行的四个失败已修复并被最终针对性运行覆盖,没有把它称为修复后的完整全量重跑。Java 共 78 个不同的针对性用例在集中运行与单用例修正后通过,编译与 Checkstyle 通过;CLI typecheck、两文件 ESLint 也通过。原 worktree 误用了 minimatch 3 的类型,复用缓存中的声明版本 9 后通过,未修改依赖清单。

原 head 的受控 Java/H2 挂载探针会在一次临时 I/O 故障后永久失败,恢复挂载也不会重新投递;候选版本会先重试,恢复后第二次投递成功,动作成为 decided,Harness 只收到一次答复。这里的权限/投递持久化/客户端边界为受控替身,不能推广为真实 EIO、完整 HTTP 或新 head 的真实模型/浏览器/MySQL/进程死亡验收。

R2-4、R2-6 按超过五轮后的仅修 Critical 规则留作后续;第七轮 V48 并发改名一致性和变异测试缺口记在 #13269,#13054/#13413 保持单独跟踪。新 head 的 CI 与复审仍待完成,不把本轮交付当成已批准或可合并。

yiliang114 and others added 2 commits October 7, 2026 22:38
main published V48__workspace_storage_migration.sql plus V49/V50 while this
branch was open, and the merge that pulled them in left two files claiming
version 48, so scripts/check-flyway-migrations.js fails the "Flyway migration
version uniqueness" gate. V51 is the next free version across both migration
locations (SQL and BaseJavaMigration, whose highest is V29). Nothing refers to
the file by name or version: the code and tests only use the
mutation_attempt_sequence column, and Flyway applies every migration in the
location, so the renumber only moves this ALTER after main's V48-V50.

Verified locally with the same command the gate runs:
  node scripts/check-flyway-migrations.js packages/sdk-java/managed-agent-server \
    packages/sdk-java/runtime-broker packages/sdk-java/qwencode
before: "2 migrations claim version 48" (rc=1); after: "51 migrations, all
versions unique" (rc=0).

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-conflict/jmuy7yk8zcm
@wenshao

wenshao commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Verification verdict: ✅ merge-ready — 543/543 scripted assertions passed (round 8, independent re-verification on a second macOS host)

Verified head: f2864f6a1d06f0cfa0b69891695fa27a70b814bb (matches the PR branch tip at report time). Controls: 2748c36dc44659d36be6130e4c567e6c1724f289 (the head before the f2864f6 fix, "x") and 9d4e1e1c5d698e9a57797b041046c83e6f26d1dc (main at the merge base, "base"), plus adb1050089c1be84ffd3cf6100df219a18cc4b0b ("m8", a trial merge of the head into current main d7b976545f, conflict-free). This is a fresh, independent rig on a different machine class than rounds 1–7 (macOS Catalina 10.15 x86_64, Node 22.23.3, JDK 21, MySQL 8.0.23): every arm runs its own server fat jar and its own bundled Harness, against a real MySQL and a scripted OpenAI-compatible model, with a recording tap in front of the Harness.

中文摘要(点击展开)

结论:可以合并(merge-ready),543/543 条脚本断言全部通过。 这是在另一台 macOS 机器(Catalina x86_64)上对同一 head f2864f6a 的独立复验,真实栈:managed-agent-server fat jar(内嵌 Runtime Broker)+ 打包的 Hosted Harness + 真实 MySQL 8.0.23 + 脚本化模型 + Harness 前的录制代理。

  • 核心 A/B(冷缓存取消,创建者已被撤销 create 权限):base(main 合并基)在准入层直接拒绝取消(409 workspace_unavailable,没有任何 POST /cancel 到达 Harness,Turn 保持 RUNNING);head 受理(202)→ 冷重附着 load 200 → cancel 204 送达 Harness → 被扣住的模型请求在 60.7s 被中止。见 r8-05 图。
  • R1-5(审批答复在撤销后到达):base 上撤权后的重试仍把答复送达(COMPLETED,Action 变 decided);head 上操作在撤权后 1.4s 以 FAILED workspace_unavailable 终结、Action 保持 requested,随后创建者取消在 0.5s 内以 CANCELLED 结束。两臂对照清晰。
  • 探针矩阵 74/74:取消矩阵 12/12 cell(revoke/draining/regen/storage/unread/control×各角色)、c5/c7/c8/c3、c16 能力翻转、f4b 生命周期、c19/c21 改名序列,head/base/m8 全绿。
  • 合并新鲜度:head 与当前 main(d7b97654,今天 13:24 UTC)试合并零冲突;合并树(m8)上取消准入与 R1-5 行为与 head 一致。main 在合并基之后动过 hosted-harness-session.ts(feat(managed-agent): give Hosted turns the Workspace's project context #13168、feat(managed-agent): H4a child agent and child acceptance record contract #13505),合并后行为保持不变已实测。
  • 非 vacuous 证明:在 head 源码上删掉 f2864f6 的 resident.active === undefined 谓词后,新增冷重附着测试在预期断言上失败(reattach 返回 409 而非 200);原样 244/244 通过。
  • 套件:Harness 244/244、WebShell 相关 81/81、Java 聚焦 117/117(H2, JDK 21)。V48 迁移在真实 MySQL 8.0.23 上随各臂首次启动应用(base 47 个迁移,x/head/m8 48 个)。
  • 遗留观察(均非本 PR 引入):V48 边界分歧在本机复现(与 r6/r7 一致,DB=Alpha vs Harness=Bravo,base 与 head 表现相同——修复或记入 fix(managed-agent): #13163 follow-ups: cold-cache cancel and deferred review suggestions #13269 由维护者决定);bug(hosted): a transient Managed Session Store outage permanently stops Session log writes, wedging the running Turn #13413 的 Harness 写入器锁死在本机 4/4 冷取消运行中复现(r7 那台较快机器为 1/4),锁死根因是 writer 租约续期撞上 ~15s 的 Spring 重启窗口——慢速主机上几乎必现,这一频率数据对 bug(hosted): a transient Managed Session Store outage permanently stops Session log writes, wedging the running Turn #13413 的优先级有参考意义;x 臂在本机未能复现 r7 的 load-409 判别(两臂都送达了取消),f2864f6a 的载荷性在本机由单测变异和机制链证明。
  • 流程项:reviewDecision 仍为 CHANGES_REQUESTED(来自一条已不可见的评审,r7 已追溯到被封的机器人账号),需要维护者手动清除;CI 在本报告撰写时仍在运行。

Previous findings — status at f2864f6a, re-measured on this host

# Finding (round) Status here Evidence
1 2748c36d cold-cancel regression; f2864f6 fixes it (r7) Confirmed fixed on head (load 200 → cancel 204 → model aborted at +60.7 s). The x arm's r7 discrimination (every load 409, cancel never delivered) did not reproduce on this host — both arms delivered; see Observations. The fix is load-bearing here at the unit level: removing the predicate turns the new cold-reattach test red on its intended assertion (409 vs 200). r8-05, r8-04
2 R6-2 (concurrent rename leg) fixed (r6/r7) Holds. c19 bound+unbound: the surviving retry completes 200, database and Harness agree on "title A". r8-01
3 V48 boundary divergence (r6, open; maintainer decision: fix or defer to #13269) Still reproduces, bound and unbound, identically on base and head: held K1 attempt #1 completes 200 "Alpha" after "Bravo"; database says Alpha, the Harness's last title is Bravo; the next K1 retry is a replay (Harness call count stays 4 → 4). Not a regression against main (main diverges identically); the PR description leaves it to #13269. r8-01
4 #13413 session-log writer latch (inherited) Reproduces 4/4 cold-cancel runs on this host (r7's faster Mac: 1/4). Root line in the Harness log: session log writes stopped after an earlier failure: Managed Session Store POST /writers:renew failed after 3 attempts: fetch failed — the writer lease renewal lands inside the ~15 s Spring restart window and latches. New data point for #13413's severity: on slower hosts the latch is near-deterministic for any Spring restart mid-Turn, not a tail event. r8-05
5 #13054 parked re-adoption under refused Broker create Not re-tested this round (needs the Broker-refusal rig). —
6 N35/N24 mutation survivors (r7) Not re-run; the f2864f6 predicate mutation is killed (new in this round). r8-04
7 reviewDecision=CHANGES_REQUESTED from an invisible review (r7 traced it to the suspended bot) Still set at report time; mergeStateStatus UNKNOWN/BLOCKED. Needs a maintainer's manual action; nothing in this PR's code blocks. PR page

Central claim and A/B proof

Claim: the creator of a Workspace-bound Session can cancel an admitted running Turn after create permission is revoked / the Workspace is draining / its registration moved (read access retained), including after a cold start that empties the server's attachment cache; new submit/rename admission still requires the bound generation/storage to be current, and recorded outcomes survive same-key retries.

The A/B scenario (probe c14-cold-cache): register a Workspace, run a Turn to completion, start a later Turn whose model request is held (240 s G_SLOW), revoke can_create, restart Spring (cold attachment cache), then the creator cancels.

Cell base 9d4e1e1c head f2864f6a
creator cancel after the restart 409 workspace_unavailable (refused at admission) 202 accepted
cold reattach POST /session/:id/load n/a (never attempted a cancel delivery) 200
POST /cancel reaching the Harness none ([]) 204 at +28.5 s
held model request never aborted aborted at +60.7 s
Turn 90 s after the cancel RUNNING (retry_count 7, no cancel ever happened) CANCELLING — settlement then blocked by the #13413 latch (below)

The same scenario with grants intact (c14-none): base admits but the Turn stays CANCELLING (R3-3, main's pre-existing cold-reattach gap); head delivers the cancel (204) and aborts the model.

Wire witness:

r8-05-cold-cancel-wire

Full 26-cell adjudication including x and m8 arms:

r8-01-ab-adjudication

R1-5: approval answer racing a revocation

The creator approves; the first delivery of the answer gets a retryable 503 (tap); can_create is revoked during the backoff.

base 9d4e1e1c head f2864f6a m8 (trial merge)
answer operation COMPLETED at +1.3 s — the retried delivery lands after the revocation; Action becomes decided FAILED workspace_unavailable at +1.4 s; Action stays requested FAILED workspace_unavailable at +1.9 s (head behavior preserved)
creator cancel afterwards 409 → Turn keeps RUNNING 202 → CANCELLED at +0.5 s 202 → CANCELLED

Regression matrix, suites, migration, vacuity

  • Probe matrix: 74/74 scripted assertions pass across head/base/m8 (r8-02-probe-matrix.png): the cancel matrix 12/12 cells (revoke / draining / regen / storage / unread / control, as creator alice, second creator carol, reader bob, stranger mallory, early- and late-restore variants — every cell ends CANCELLED for creators and refused for non-creators, no command row written on refusal), re-registration admission (c5 ×2), replay ordering (c7), refused renames (c8 ×6), lost deliveries (c3 ×2), the WebShell capability flip after re-registration (c16: workspaceTurns=false for the moved Workspace's Sessions on list and get, control Workspace unaffected, submit refused 409, restored after re-binding), Session lifecycle states (f4b), rename races (c19/c21 ×2 each on head and base).

r8-02-probe-matrix

  • Suites at head: Harness hosted-harness-session.test.ts 244/244; WebShell PR-touched suites (ManagedSessionsPage + java-managed-agent-provider) 81/81; Java focused classes (ManagedWorkspaceAdmissionTest, QwenHostedHarnessColdCancelRegressionTest, ManagedSessionLifecycleTest, ManagedActionsTest, QwenHostedHarnessConnectorTest) 117/117 on H2 / JDK 21 (r8-03-suites.png).

r8-03-suites

  • V48 migration on real MySQL 8.0.23: first boot per arm applies 47 migrations on base vs 48 (incl. V48) on x/head/m8; later boots validate cleanly. V48 is still free on current main (d7b97654), but five other open PRs also claim V48 — whoever lands second renumbers.
  • Vacuity / load-bearing proof of the f2864f6 test: deleting the resident.active === undefined predicate from head source turns exactly the new "aborted live turn … Java-style cold reattach" test red, failing on the intended assertion (expected 409 to be 200 on the cold reattach); the unmutated file passes 244/244 (r8-04-migration-vacuity.png).

r8-04-migration-vacuity

  • Jar integrity control: the four server jars were built sequentially against one Maven repo; verified each embeds its own arm's code (V48 migration present in x/head/m8, absent in base).

Observations (not new defects in this PR)

  1. bug(hosted): a transient Managed Session Store outage permanently stops Session log writes, wedging the running Turn #13413 is near-deterministic on a slower host. Every cold-cancel run here (4/4, incl. one with the pre-restart settle raised to 15 s) latched the Harness writer when the lease renewal met the ~15 s Spring outage. The cancelled Turn then stays CANCELLING even though the cancel was delivered and the model aborted. r7 measured 1/4 on a faster Mac. Worth weighting in bug(hosted): a transient Managed Session Store outage permanently stops Session log writes, wedging the running Turn #13413's priority: any Spring restart mid-Turn on a slow host triggers it, and this PR's cold-cancel path is exactly the flow that restarts exercise.
  2. The x-arm discrimination of f2864f6 is timing-sensitive at the rig level. r7 (faster host) saw every x-arm load refused 409; on this host both arms' first cold load answered 200 and delivered the cancel. Mechanism: the pre-fix arm only refuses when the resident Turn is still active at load time; on this host the store outage during the restart clears/latches that state before Java's first load (~27 s after admission) arrives. The predicate's effect is nonetheless pinned deterministically by the unit mutation above, and head's mechanism (reattach 200 → cancel 204 → model abort) executed on every run.
  3. Rig deviations from r7, all disclosed: the unpublished adapter.jar is replaced by the in-tree TrustedActorHeaderFilter (--qwen.managed-agent.trusted-actor-header=X-Rig-Actor, present on base and head alike); MySQL 8.0.23 instead of 8.4.7; the c5/c7/c8/c3 probes were missing from r7's published probe directory and were taken from the r5-linux evidence (same lib API).

Not covered

  • Real-model runs (the rig uses a scripted deterministic model); WebShell real-browser UI (r7's four screenshots at b683a3d/head remain the UI evidence); Linux on this head (r6 covered it at 13df2a65); MariaDB (CI lanes); the overlapping-recovery fence probe (r7's, needs the second Broker tap); N24/N35 Java mutation survivors; R4-1 end to end; physical interruption of an executing tool. The current head's CI was still running at report time.

Methodology

Host: macOS Catalina 10.15.7 x86_64, Node 22.23.3, JDK 21 (Temurin), MySQL 8.0.23 (Catalina build), Maven 3.9.9. Four arms (head f2864f6a, x 2748c36d, base 9d4e1e1c, m8 trial merge into d7b97654), each = server fat jar + bundled Harness (dist/cli.js serve --profile hosted-harness) + embedded Runtime Broker + scripted model + recording tap, isolated MySQL schemas per arm. All probes and the adjudicator are mock-free with respect to the code under test; every number above comes from a scripted check that ran (probe JSONs, tap/model ledgers, suite outputs) under pr13163/r8/ in the evidence branch. assertions.json counts: 74 probe checks + 26 A/B adjudications + 442 suite tests + 1 mutation kill = 543.

Evidence: wenshao/qwen-code@assets-pr13163/pr13163/r8.

@qwen-code-review-bot qwen-code-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Round-2 Criticals verified fixed at this head; my CHANGES_REQUESTED now stands on one red required lane, and the fix is one line.

R2-1, R2-2 and R2-3 are all closed in source at 89aa9d43, checked line by line rather than from the replies:

  • R2-1: the resident drive arm answers a typed kernel declined with recoveryDeclined(res, outcome.reason) again (hosted-harness-session.ts:2059-2062), restoring the only refusal the connector maps to a terminal outcome.
  • R2-2: inapplicable on the resident arms now routes through answerResidentInapplicable, and the shared helper at :1799-1819 refuses with hosted_turn_recovery_required only when !approvalPending && settle === null — i.e. genuinely unpayable — so the plain attach for a requested approval and the settle projection are paid on the resident path as the cold path pays them.
  • R2-3: doResolveAction no longer runs the shared acquire; it uses authorizePassiveAttachment plus the new verifyMountForProbe, whose guard entry maps IOException to unavailableTransient (retryable = true) while structural refusals keep terminal unavailable() (WorkspaceStorageGuard.java:407-421, WorkspaceExecutionStore.java:311-325). The coordinator's terminal discard requires !isRetryable(), so a momentary mount blip now re-arms through the delivery machine instead of permanently discarding a committed user decision. The retryability split is exactly the one the finding asked for.

What blocks now is CI, not a finding: Runtime Broker and Managed Agent MariaDB / Java 21 failed at this head (job 112865259176, completed 15:41:40Z) on WorkspaceMigrationMySqlIT.upgradesCurrentMainWithoutChangingAppliedMigrations. The failure is the V48→V51 renumber in 89aa9d43 without bumping that test's main-tail pin: it seeds at target("47"), re-migrates with the tree, and asserts the newly applied versions are exactly ["48","49","50"] (WorkspaceMigrationMySqlIT.java:65-67); the tree now applies ["48","49","50","51"]. The renumber itself is correct — V51 is free on main, whose tail is 48-50 — and the migration's substance is untouched; only the pin is stale. The one-line fix is containsExactly("48", "49", "50", "51"). Hosted process fault gates was still in flight when I wrote this; Test (ubuntu) and Lint & Static are green.

Once that pin lands and both Java gates complete green at the fix commit, my CR clears with an approval — R2-1/2/3 need nothing further from you. The 18 Suggestion-level items and the round-2 deferred list remain as the record and do not block.

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

COMMENT at head 89aa9d43b87617f513bbe0bf8f868d2774cdea30. Not approving. The three Criticals from the standing CHANGES_REQUESTED are fixed as far as I can trace them — what blocks is a red Java lane in this PR's own area that I could not attribute inside this channel's budget.

The round-2 Criticals are closed — traced in source at this head

The CHANGES_REQUESTED at f2864f6a filed R2-1, R2-2 and R2-3. Two commits answer them and I read the result rather than taking the commit messages on faith.

R2-1 — terminal decline reason restored. 0227b250 changes the resident drive-redrive arm from error(res, 409, 'hosted_session_already_attached') to recoveryDeclined(res, outcome.reason). That is exactly the code the finding said was lost, and it is the refusal QwenHostedHarnessConnector maps to a terminal outcome, so an unrecoverable decline again terminates instead of being retried.

R2-2 — both dropped payments restored. The inapplicable arm no longer answers an unconditional 409; it delegates to the new answerResidentInapplicable, which performs the two payments the finding named: when harnessRunAuthorization() reports status === 'runnable' with checkpoint.approval?.state === 'requested' it sets approvalPending and falls through to sendAttachment, i.e. the plain attach for a Turn parked on a live approval; otherwise it calls settleProjectablePromptId and runs runSettleProjection for the inline terminal projection. The 409 hosted_turn_recovery_required now fires only when neither is payable (!approvalPending && settle === null). It also gained the two liveness guards the cold path has — sessions.get(sessionId) !== resident → 404 and resident.mcpClosing → 409 — before attaching.

R2-3 — the transient/permanent classification is split at the point the finding identified. a9f7fec1 routes the action-response path off the shared acquire: doResolveAction now calls requireReadyForNewWork(tenantId, sessionId, true), and that branch runs authorizePassiveAttachment(session) plus verifyMountForProbe(session.workspace()) instead of workspaceExecution.authorize(session). The decisive part is WorkspaceStorageGuard.verifyProbe (:407-416): identities.read(root) failing with IOException now throws WorkspaceExecutionStore.unavailableTransient(error), while only RuntimeException maps to the terminal unavailable(). The finding's chain was that every failure on the shared path became the terminal unavailable() = RuntimeBrokerException(409, …, false), so a momentary mount blip hit ActionResponseCoordinator's !isRetryable() exit and permanently discarded a committed action response. With the mount I/O failure now retryable, that exit is no longer reachable from a blip, and permanent refusals (row not READY, marker mismatch, cwd escaping the root) still throw terminal. WorkspaceStorageGuardTest gained 67 lines alongside.

I also re-traced R1-1 from round 1, since it was a snapshot-leak claim: doCancel (QwenHostedHarnessConnector.java:378-382) now ends with pendingRecovery.remove(new AttachmentKey(tenantId, sessionId)), matching the cancelManagedRuntime sibling it was compared against, so a cold cancel no longer leaves its recovery snapshot to be handed to the next Turn.

What blocks approval

1. Runtime Broker and Managed Agent MariaDB / Java 21 fails at this head, and I cannot attribute it. The annotation carries only Process completed with exit code 1 with no test name, and the run's logs are not retrievable while another job in the same run is still pending (Hosted process fault gates / MySQL 8.4 / Java 21), so the failing test could not be read. This is the lane that executes exactly the suites this PR modifies — QwenHostedHarnessConnectorTest, WorkspaceStorageGuardTest and the managed-agent ITs — and both fix commits touch them.

It is not a globally broken lane: the same lane passes at 3a7f6238 on #13467, a head that also contains a current merge of main. So the failure tracks this branch's code rather than the runner or main. That is a strong indication but not the direct evidence this channel requires to file it as a Critical, and I am not filing it as one — I am recording that the gate is unconfirmed. Every other lane is green, including Test (ubuntu-latest, Node 22.x), Lint & Static, Serve A/B, Integration Tests (no-AK, No Sandbox) and Flyway migration version uniqueness, the last confirming the V51 renumbering in 89aa9d43 does not collide.

2. Two round-1 Criticals I did not re-trace. R1-2 (the keeps teardown fenced until overlapping passive loads finish (%s) gates binding by arrival order at broker.acquire() rather than request order, making firstGate/secondGate a race) and R1-5 (removing the verifiedRecoveryEnabled() short-circuit making the creation-authority recheck unconditional for submit, continueManagedRuntime and resolveAction, silently withholding an accepted approval) are not among the round-2 findings and are not in its 18-item already-reported list, which is consistent with both having closed between rounds. Round 2 did run a Critical scan and filed three new ones, so its silence is meaningful — but I have not read either mechanism at this head, and 0227b250 rewrote the resident arms R1-2's test exercises.

3. The CHANGES_REQUESTED at f2864f6a is still the standing review decision. Nothing has superseded or dismissed it at 89aa9d43.

Next step

Re-run the Java lane once its current run finishes and read the failing test name — if it is one of the suites 0227b250 or a9f7fec1 touched, that is the remaining blocker to clear; if it is a flake, a green re-run plus a note here is enough. Confirming R1-2's gate ordering and R1-5's recheck scope at this head would let the next review close on all five Criticals at once.

… upgrade IT

WorkspaceMigrationMySqlIT.upgradesCurrentMainWithoutChangingAppliedMigrations
pins every migration applied after the V47 baseline, so renumbering
V48__managed_mutation_attempt_sequence.sql to V51 left that expectation one
version short and the MariaDB failsafe job failed 1 of 58 tests.

Co-authored-by: Qwen-Coder <[email protected]>
Patrol-Run: qwen-pr-conflict/jmuya3q1ncp
chiga0
chiga0 previously approved these changes Oct 7, 2026

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: fix(managed-agent): stop a bound Turn under refused authorization

Tier: Deep — authorization surface, persisted-format change (DB migration V51), lifecycle/concurrency (passive-recovery counter, lease adoption).

Scope: Primary analysis on head 89aa9d43 (31 files, all patches fetched). Head moved to 3c42bc68 mid-review; incremental diff is one test-fixture line (adds "51" to expected Flyway version list). Source-code analysis unchanged.


No blocking findings.

Approval blockers: none.


What was checked

Authorization split (Java)

requireCanceller — narrower rule: active session shape + canRead + createdSession. Does not check can_create, bindingCurrent, or registry ACTIVE state. Short-circuit evaluation in the condition prevents NPE when workspace is null (shape check returns false first). Non-workspace sessions pass through requireLegacyWorkspace silently — same as the pre-PR requireSubmitter path.

requireBoundCreator — guards replay paths in submitTurn and renameSession for workspace-bound Sessions. Checked before the replay lookup; non-creators cannot reach the replay table.

maySubmitWorkspaceTurn (singular + batch) — adds bindingCurrent (generation + storageId) and the batch twin adds the same comparison against the ReadableGrant fields. New submissions are refused before any command row is written after re-registration.

Submit replay ordering — requireBoundCreator before replay; requireHarness inside the replay block; fresh admission path calls requireSubmitter + requireHarness. Consistent.

Rename COMPLETED replay — findCommand checked after requireBoundCreator, before requireSubmitter. PENDING row falls through to beginSessionMutation.

DB migration and mutation sequencing

V51 adds mutation_attempt_sequence BIGINT nullable. Existing rows retain NULL, falling back to COALESCE(…, e.sequence_id) in supersededByLaterMutation. INSERT path in beginSessionMutation does two separate SQL statements (insert then UPDATE sequence); atomicity inferred from the surrounding FOR UPDATE context (method designed for transactional callers), and the NULL fallback makes this safe in the gap case.

supersededByLaterMutation — guards completeSessionMutation against a FAILED command completing after a later completed one. Empty attempts list guard prevents getFirst() throw. COUNT(*) > 0 null-check is defensive.

TypeScript session lifecycle

mcpRecovering counter (boolean → number) — allows concurrent passive recovery loads to each hold the fence. wakeBusy now checks > 0 (correct). MCP route guards check truthiness (0 is falsy, non-zero is truthy — correct).

Resident session validation — new checks for storeBaseUrl and definition fields before any recovery path; covered by new tests.

onPassiveRuntimeAcquired callback — fires immediately after broker.acquire(), before any fallible reads. Session retains the lease even when later authorization fails; teardown releases it. Covered by releases a resident passive adoption after $failure tests.

settleProjectablePromptId guard — new (fileHistory.pendingTurn || fileHistory.pendingUndo) && guard skips canSettleHostedFileHistory when no pending work exists. Prevents spurious settlement refusal on a complete file history. Intentional and correct.

UI

canCancel decoupled from workspaceTurns — cancel button shown whenever there is an active turn, regardless of new-work availability. New standalone render site when the composer is hidden. Parameterised test covers both workspaceTurns=true/false.

ActionResponseCoordinator

Terminal workspace_unavailable — new early-exit fires only when !isRetryable() && code=="workspace_unavailable". Retryable variant and other non-retryable codes fall through to the retry scheduler. workspaceAdmissionFailureFollowsRetryability test covers permanent/retryable/other-code cases.


Cross-check against existing reviews

Finding Status at 89aa9d43
R1-1 (cancel certifies falsely) Previously addressed rounds; not visible as a defect in current diff
R1-5 (verifiedRecoveryEnabled removed) Not visible as a defect — cancellation now uses authorizeCancellation SQL guard instead
R2-1 (drive-redrive declined without owed lease) No lease acquired before declined; no adoption record needed
R2-2 (resident inapplicable skips refusedAdoptions.delete) answerResidentInapplicable calls refusedAdoptions.delete in its success path; failure paths call noteOwedAdoption
R2-3 (terminal workspace exit keys on isRetryable) Condition is !isRetryable() && code=="workspace_unavailable" — correctly narrows to that case only
bot CHANGES_REQUESTED Per qqqys comment at current head: three Criticals fixed; outstanding bot block is a CI lane, outside this review's verdict
P2 late-rename supersession (yiliang114) Known bounded race: external title can temporarily lead the DB title; DB is authoritative and self-correcting. Not a merge blocker.

Unreviewed dimensions

  • No local build or test run (runtime tier unavailable; execution-level test validity not independently exercised).
  • Transaction boundary of beginSessionMutation INSERT path not directly confirmed (file too large to fetch within timeout; NULL fallback in COALESCE makes the gap safe).
  • Windows/Linux platform behavior not independently verified.
  • Test patches reviewed for shape and coverage only, not executed locally.

Reviewed with AI assistance.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Delivered ce56f4395766f8c5bbd8e2aff070a3e225b6a31e as one additive commit (+15/-9 in one existing Java IT; no production changes).

Follow-up to qqqys’s review.

The MariaDB lane passes at the starting head 3c42bc684b5a. The remaining Java failure is HostedPublicWorkspaceIT.workspaceCwdChangeSettlesThroughBothSurfaces: its Harness readiness request returns 404 for 30 seconds before any cwd assertion runs. The fixture released each of three ephemeral ports before allocating the model server. The repair binds the model first and reserves all three remaining ports together, so the fixture cannot allocate the same port to two of its servers. Readiness failures now retain the HTTP status/body and Harness stdout.

A controlled real-Harness collision reproduces the symptom: the requested port returns 404, while the still-live Harness logs the collision and serves capabilities on the next port. That probe used the historical a9f7fec1 bundle; the relevant listen implementation is unchanged at the current head. The original CI report did not retain Harness stdout, so the exact process owning its port is unknown. This repair removes the observed fixture allocation risk; the next MySQL CI result remains the acceptance gate.

R1-2 is ordered in the current source: the test waits for the first acquire before starting the second load, then asserts teardown refusal and no release until both loads settle. Both resolved/rejected cases pass, 2/2, at the starting head.

R1-5 uses the previously accepted terminal-operation alternative. Submit/continuation still require new-work authority; action responses check the creator’s current grant, bound generation/storage and mount probe. Cancellation uses its separately admitted authority. A permanent workspace_unavailable fails the recorded response operation visibly instead of retrying forever; temporary mount I/O retries, and an already committed decision remains completed after reply loss. This does not reopen new work after revocation.

WebShell’s first attempt never ran: its check annotation says the runner failed to acquire the job in five attempts. Attempt 2 was already running when investigated; no WebShell source change is needed for that failure.

Verification on the delivered sources: 66 distinct Java tests pass across the consolidated run and its cache correction (connector 39/39, actions 26/26, the previously failing cwd IT 1/1); Checkstyle has zero violations. R1-2 passes 2/2 focused CLI tests. Eight current gate-defining commits are ancestors of this head and their files match the fetched base; the standard helper refused the repository’s shallow status, so ancestry and file content were verified directly. The native cwd IT uses H2, real Spring/Harness/Runtime worker, the current TypeScript sources via TSX and a controlled model. No Node build was needed. The initial local run found one obsolete generated V48 migration left in target/classes; removing that artifact unblocked the actions/IT run, without changing source. The 39 connector tests already passed in the initial run and were not repeated. This is focused validation, not a full suite or MySQL acceptance claim.

中文说明

本轮只修复一个 Java IT 夹具的端口分配与启动失败诊断,不改产品权限或恢复行为。模型先绑定、其余三个端口同时占用后取号,避免同一测试内的端口重复或模型抢占。受控真实 Harness 复现了端口占用后自动换号、原地址持续 404 的现象;原 CI 没保存 Harness stdout,不能认定当时具体由哪个进程占用。

qqqys 提到的 MariaDB lane 在起始 head 已通过。R1-2 的两种并发门闸结局在当前源码 2/2 通过;R1-5 保留撤权后的新工作拒绝,明确不可重试的审批投递记录为失败,暂时错误与已落库决定分别保留重试和成功语义。WebShell 首轮根本没有执行,原因为 runner 连续五次未接单,已有重跑。

Java 66 个不同用例通过(connector 39、actions 26、原失败 cwd IT 1),Checkstyle 零问题;CLI 并发恢复 2/2 通过。原生 IT 使用 H2、真实 Spring/Harness/worker 与当前 TSX 源码、受控模型,没有复用旧 bundle,也无需 Node build。首次本地验证的旧 V48 编译产物已精确清掉,39 个已绿 connector 用例没有重跑;其他受阻用例随后通过。这里不声称全量或 MySQL 验收。当前 head 的 MySQL/WebShell CI 与维护者批准分别记录,不把历史证据当作本轮完整验收。

@qwen-code-review-bot qwen-code-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

12 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • canCancel lost its server-published discriminator — already reported as R1-6 (comment 4202227739) at packages/web-shell/client/components/managed/java-managed-agent-provider.ts:301; re-derived this round by three independent agents
  • primary cancellation-reattach branch's silent cause-less catch — already reported as R1-14 (comment 4202227769) at packages/cli/src/serve/hosted-harness-session.ts:2219; re-derived this round by two agents
  • loose acquire wait in the stranding test — already reported as R1-3 (comment 4202227737) at packages/cli/src/serve/hosted-harness-session.test.ts:9359; this round's territory finder graded the same defect Critical while the standing thread …
  • recovery-identity rule stated three times in one file — already reported as R1-7 (comment 4202227743); re-derived this round at packages/cli/src/serve/hosted-harness-session.ts:2178-2187
  • resident-reattach drift table asserts only the HTTP status, leaving hosted_session_store_mismatch unpinned — already reported as R2-4 (comment 4206295709) at packages/cli/src/serve/hosted-harness-session.test.ts:7824
  • decorative @valuesource parameter in QwenHostedHarnessConnectorTest — already reported as R1-27 (comment 4202227808) at packages/sdk-java/managed-agent-server/src/test/java/com/alibaba/qwen/code/managedagent/harness/QwenHostedHarnessConnect…
  • gate stack re-reads the same session row three times per request — already reported as R1-8 (comment 4202227746) at packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/service/ManagedAgentService.java:227…
  • cancelTurn lacks the replay-before-gate ordering the same diff gives submitTurn — already reported as R1-20 (comment 4202227787) at packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/service/ManagedAgent…
  • OpenAPI required-list tightened and workspaceTurns narrowed without a version bump — already reported as R1-17 (comment 4202227775) at packages/sdk-java/managed-agent-server/src/main/resources/openapi/managed-agent-public-api.openapi.json:5…
  • bindingCurrent is a second SELECT against the same registry primary key and restates the binding rule in a divergent form — already reported as R1-34 (comment 4202227750) at packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/q…
  • interleaved ternary SQL in authorizeAttachment and its row-mapper indentation — already reported as R1-10 (comment 4202227755) and R1-35 (comment 4202227758) at packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/mana…
  • late-rename supersession leaves the Harness title diverged from SQL after a session_mutation_superseded 409 — already discussed at packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/store/ManagedAgentSto…

Not reviewed: test-efficacy probe — harnessValidated: null and all 13 probed files inconclusive (no-output: the probe's reverted tree lacks the gitignored packages/cli/src/generated/git-commit.ts, so every probe died in vitest global setup); mutants probed 0 of 18 and hunks 0 of 66, so no revert/mutant/hunk-necessity coverage was measured for this diff.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": whether the turn-admission path ( ManagedAgentStore.claimTurn / the submit admission in ManagedAgentService ) can leave two managed_agent_turn rows in CANC…; "agent reverse-audit (round 1)": the Java/JVM-performance lens my brief assigns for ApiModels.java , ManagedAgentStore.java , ManagedWorkspaceRegistry.java , HostedPublicWorkspaceIT.java a…; "agent reverse-audit (round 1)": I did not open WorkspaceExecutionStore.unavailable() / unavailableTransient(...) to confirm the code and isRetryable values the new ManagedActionsTest a…; "agent reverse-audit (round 1)": I did not establish *why* the ManagedAgentServerIntegrationTest rename fixture was swapped to a typed retryable exception — I checked ManagedAgentService.ren…; "agent reverse-audit (round 1)": the brief names packages/sdk-java/managed-agent-server/README.md under the consumer-facing contract-documentation rule, but chunk 9 contains no README hunk; I…, and 10 more.

Not reviewed: reverse audit — stopped before round 4 by the review time budget.

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/serve/hosted-harness-session.ts:2104 — [probe] three of the four resident-reattach frozen-definition clauses (mcpServers, hookCatalog, captureBytes) are exercised by no test, and the first of the two new resident 409 assert…
  • packages/cli/src/serve/hosted-harness-session.test.ts:9244 — [probe] the rejected arm of the overlapping-passive-loads test writes a false owed-adoption record that installs no stderr spy, and the surviving entry silences the next genuine s…
  • packages/sdk-java/managed-agent-server/README.md:765 — [probe] the diff documents recovered cancellation as shipped while README.md:342-345 still lists it as pending and says not to advertise it, so one document states both
  • packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/store/ManagedWorkspaceRegistry.java:229 — [probe] readableGrant's new ACTIVE folding is unpinned on the batch capability surface; mutating it alone keep…
  • packages/sdk-java/managed-agent-server/src/test/java/com/alibaba/qwen/code/managedagent/harness/QwenHostedHarnessColdCancelRegressionTest.java:107 — [probe] the only cold-cancel witness builds the guard-nulling two-arg WorkspaceExecutionSto…
  • packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/store/WorkspaceExecutionStore.java:103 — [probe] neither arm of the new persisted-intent disjunction is individually pinned, because the only real-store…
  • packages/web-shell/client/components/managed/ManagedSessionsPage.tsx:410 — [probe] the server-409 compensating control the new comment names is unwitnessed: no test makes client.cancel reject, so the catch arm and role=alert render in zero …
  • packages/cli/src/serve/hosted-harness-session.ts:2034 — [probe] the drive-redrive arm's early 200 skips the mcpClosing and unsettled-input pre-answer guards both sibling resident paths apply, and drains an owed-adoption record for a lease i…

Convergence: round 3 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 6 (6 new). Findings keep coming back to the same files: packages/cli/src/serve/hosted-harness-session.test.ts (findings in round 2; 1 more now); packages/cli/src/serve/hosted-harness-session.ts (findings in round 2; 1 more now); packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/service/ActionResponseCoordinator.java (findings in round 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 12 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):test-efficacy probe — harnessValidated: null and all 13 probed files inconclusive (no-output: the probe's reverted tree lacks the gitignored packages/cli/src/generated/git-commit.ts, so every probe died in vitest global setup); mutants probed 0 of 18 and hunks 0 of 66, so no revert/mutant/hunk-necessity coverage was measured for this diff.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)":whether the turn-admission path ( ManagedAgentStore.claimTurn / the submit admission in ManagedAgentService ) can leave two managed_agent_turn rows in CANC…;"agent reverse-audit (round 1)":the Java/JVM-performance lens my brief assigns for ApiModels.java , ManagedAgentStore.java , ManagedWorkspaceRegistry.java , HostedPublicWorkspaceIT.java a…;"agent reverse-audit (round 1)":I did not open WorkspaceExecutionStore.unavailable() / unavailableTransient(...) to confirm the code and isRetryable values the new ManagedActionsTest a…;"agent reverse-audit (round 1)":I did not establish *why* the ManagedAgentServerIntegrationTest rename fixture was swapped to a typed retryable exception — I checked ManagedAgentService.ren…;"agent reverse-audit (round 1)":the brief names packages/sdk-java/managed-agent-server/README.md under the consumer-facing contract-documentation rule, but chunk 9 contains no README hunk; I…,另有 10 条。

未审查:反向审计——评审时间预算不足,未能开始第 4 轮。

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 3 轮发布了 6 条行内评论,其中 6 条是首次提出;上一轮发布了 6 条(其中 6 条首次提出)。发现反复回到同一批文件:packages/cli/src/serve/hosted-harness-session.test.ts(第 2 轮已出过发现,本轮又有 1 条);packages/cli/src/serve/hosted-harness-session.ts(第 2 轮已出过发现,本轮又有 1 条);packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/service/ActionResponseCoordinator.java(第 2 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.25.0)

Comment thread packages/cli/src/serve/hosted-harness-session.ts
}
const refused = await load;
expect(refused.status).toBe(404);
expect(refused.body.code).toBe('hosted_session_not_found');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R3-2: This test pins the cancellation-takeover arm's post-await re-validation, but nothing pins the identical pair inside the new answerResidentInapplicable (hosted-harness-session.ts:1821-1828), so the R2-2 fix ships two unwitnessed guards.

Failure scenario. Deleting the if (sessions.get(sessionId) !== resident) and if (resident.mcpClosing) blocks keeps all 251 tests in this file green — the four new tests are the decline-reason wire test, the requested-approval reattach, the payable-settle redrive and the cancellation-takeover deletion (which exercises the other arm's copy), and the pre-existing takeover_inapplicable_unpayable tests only reach the unpayable 409 at :1813-1819. A later refactor of the resident inapplicable path can therefore drop either guard silently, re-opening exactly the failure this test was written for: a Session deleted during the settlement awaits is answered 200 with a clientId no route can use, and a draining Session is handed a settle projection that writes to a closing journal.

Witness — mutation run in an isolated mirror, whole file both sides:

intact                          Tests 251 passed (251)
both guard blocks deleted       Tests 251 passed (251)   <- nothing reddens

Suggested fix. Add one test beside the cancellation-takeover one: park a resident turn, mock the authorization to turn_settled, hold answerResidentInapplicable's reads, DELETE /session/:id (expect 204), release, and assert the redriven load answers 404 hosted_session_not_found; a second case can set mcpClosing and assert 409 hosted_session_closing.

Note that noteOwedAdoption records nothing unless the Session holds a lease (:3666-3674) and a drive-arm inapplicable never acquires one (hosted-runtime-recovery.ts:505 returns before originalRuntimeBroker), so the new test can only assert the wire answer, not an owed-adoption side effect.

The new test is itself the acceptance criterion: it must go red when :1821-1824 is removed, which today it does not — please confirm that mutation reds it.

中文说明

这个测试固定了 cancellation-takeover 分支在 await 之后的重新校验,但新的 answerResidentInapplicable(hosted-harness-session.ts:1821-1828)里那对完全相同的校验却没有任何测试覆盖,于是 R2-2 的修复带着两处无人见证的守卫上线。

触发场景。 删掉 if (sessions.get(sessionId) !== resident) 和 if (resident.mcpClosing) 两个代码块,本文件 251 个测试全部仍然通过——四个新测试分别是 decline 原因的报文测试、requested 审批重连、可结算 projection 的 redrive,以及 cancellation-takeover 删除(它走的是另一个分支的副本),而既有的 takeover_inapplicable_unpayable 测试只到达 :1813-1819 的不可结算 409。因此后续对 resident inapplicable 路径的重构可以悄无声息地移除任一守卫,重新打开这个测试本该防住的故障:在结算 await 期间被删除的 Session 会得到 200 和一个任何路由都无法使用的 clientId,正在 draining 的 Session 会拿到一个向正在关闭的日志写入的结算投影。

建议修复。 在 cancellation-takeover 测试旁边补一个用例:停驻一个 resident turn,把授权 mock 成 turn_settled,挂起 answerResidentInapplicable 的读取,执行 DELETE /session/:id(期望 204),释放,然后断言被 redrive 的 load 返回 404 hosted_session_not_found;第二个用例可设置 mcpClosing 并断言 409 hosted_session_closing。

注意 noteOwedAdoption 只有在 Session 持有租约时才会记录(:3666-3674),而 drive 分支的 inapplicable 从不去获取租约(hosted-runtime-recovery.ts:505 在 originalRuntimeBroker 之前就返回),所以新测试只能断言报文应答,不能断言 owed-adoption 副作用。

新测试本身就是验收标准:移除 :1821-1824 时它必须变红,而目前不会——请确认该变异确实使它变红。

— qwen3.8-max via Qwen Code /review (v0.25.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed at head ce56f4395766, read-only.

  • hosted-harness-session.test.ts:9895 is expect(refused.body.code).toBe('hosted_session_not_found'); — the tail assertion of the new 'refuses a cancellation takeover attachment deleted during settlement' test, i.e. the cancellation-takeover arm, as described.
  • This file's diff against merge-base 1162c96ab98b adds exactly 7 it(...) blocks. None of them drives answerResidentInapplicable with the Session deleted (or mcpClosing set) across its awaits. The only pre-existing takeover_inapplicable_unpayable references are stderr assertions at :10021 and :10052, which sit on the unpayable-409 arm (hosted-harness-session.ts:1812-1818).
  • The un-witnessed guards are :1820 (sessions.get(sessionId) !== resident) and :1825 (resident.mcpClosing). The finding cites :1821-1828; the first if opens on :1820.

One correction: the finding says "the four new tests". The diff adds seven. That does not change the conclusion — none of the seven pins those two guards.

I did not run the mutation (both guard blocks deleted → file still green). The scope fuse forbids code changes this round and the cli suite needs its bootstrap, so that mutation result on record is the bot's, not mine.

Why no fix this round, and a dependency. Scope fuse: +2193/-341 over the 1500-addition limit ⇒ replies-only. This item is also dependent on the R2-2 ruling rather than independent: the requested test pins the guard pair inside the very helper that R2-2 says is missing a third guard. If the owner narrows or reverts that extraction, the guards this test would witness move. Take it in the same session as R2-2, not separately. Not human-gated on its own.

Left unresolved.

store.abandonSessionMutation(tenant, "RENAME_SESSION", "k1", sessionId);
if (legacyReceipt) {
jdbc.update("UPDATE managed_agent_command SET mutation_attempt_sequence"
+ " = NULL WHERE tenant_id = ? AND operation = ?"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R3-3: Both new @ParameterizedTests create the state that distinguishes their two arms with an unasserted jdbc.update whose returned row count is discarded — legacyReceipt's UPDATE ... SET mutation_attempt_sequence = NULL here and recreatedReceipt's DELETE FROM managed_agent_command at :578-582, both keyed on (tenant_id, operation, idempotency_key). The moment that triple stops identifying exactly one row, the setup matches 0 rows, both boolean arms run identical code, and every assertion still passes.

Failure scenario. What is lost with nothing turning red is the only coverage of the COALESCE(c.mutation_attempt_sequence, e.sequence_id) legacy fallback in supersededByLaterMutation (ManagedAgentStore.java:2843) — the path every pre-V51 command row takes, since the migration adds a bare nullable BIGINT with no default. With that coverage gone, dropping the COALESCE's second argument ships clean: a pre-V51 retired receipt resolves to a NULL boundary, the attempts.getFirst() == null early return makes supersededByLaterMutation return false, the guard at :634 does not fire, and the retired rename's surviving sibling completes and reverts the newer committed title.

Witness — three runs of this class in an isolated repo-layout Maven copy:

intact                                   green
setup key "k1" -> "k1-absent"            both parameterized arms still pass
                                         (setup silently matched 0 rows)

Suggested fix. Assert the setup mutation's row count instead of discarding it, in both tests:

Suggested change
+ " = NULL WHERE tenant_id = ? AND operation = ?"
assertThat(jdbc.update("UPDATE managed_agent_command SET mutation_attempt_sequence"
+ " = NULL WHERE tenant_id = ? AND operation = ?"
+ " AND idempotency_key = ?",
tenant, "RENAME_SESSION", "k1")).isEqualTo(1);

and likewise for the recreatedReceipt DELETE.

The expected count must be exactly 1, not >= 1 — (tenant_id, operation, idempotency_key) is the receipt identity the store itself updates by (ManagedAgentStore.java:570), and this test already reads that row with a single-row queryForObject on the same triple, which would throw on a second match.

The acceptance criterion is the mutation above: with the assertion in place, changing the setup key to "k1-absent" must red retiredRenameCannotCompleteOverALaterCompletedRename[legacyReceipt=true]; without it, the test passes while having stopped exercising the legacy NULL-sequence path entirely.

中文说明

两个新的 @ParameterizedTest 都用一次未加断言的 jdbc.update 来构造区分两个分支的状态,且丢弃了返回的影响行数——这里是 legacyReceipt 的 UPDATE ... SET mutation_attempt_sequence = NULL,:578-582 是 recreatedReceipt 的 DELETE FROM managed_agent_command,两者都以 (tenant_id, operation, idempotency_key) 为条件。一旦这个三元组不再唯一标识一行,前置构造就会匹配 0 行,两个布尔分支执行完全相同的代码,而所有断言依然通过。

触发场景。 在没有任何测试变红的情况下丢失的,是 supersededByLaterMutation(ManagedAgentStore.java:2843)中 COALESCE(c.mutation_attempt_sequence, e.sequence_id) 遗留回退分支的唯一覆盖——而这正是所有 V51 之前的命令行走的路径,因为该迁移只加了一个无默认值的可空 BIGINT。失去该覆盖后,删掉 COALESCE 的第二个参数也能干净上线:V51 之前的已退役回执会解析出 NULL 边界,attempts.getFirst() == null 的提前返回使 supersededByLaterMutation 返回 false,:634 的守卫不触发,退役 rename 的存活兄弟请求便会完成并把更新的已提交标题回滚掉。

建议修复。 在两个测试中都断言前置构造的影响行数,而不是丢弃它(见上方 suggestion 块),recreatedReceipt 的 DELETE 同理。

期望值必须恰好是 1 而不是 >= 1——(tenant_id, operation, idempotency_key) 正是 store 自身更新时使用的回执身份(ManagedAgentStore.java:570),而本测试已经用单行 queryForObject 按同一三元组读取该行,匹配到第二行就会抛异常。

验收标准就是上面那个变异:加上断言后,把前置构造的键改成 "k1-absent" 必须使 retiredRenameCannotCompleteOverALaterCompletedRename[legacyReceipt=true] 变红;不加断言时,测试会通过,但已完全不再执行遗留的 NULL sequence 路径。

— qwen3.8-max via Qwen Code /review (v0.25.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed at head ce56f4395766, read-only.

  • ManagedSessionLifecycleTest.java:520-525 (legacyReceipt arm): jdbc.update("UPDATE managed_agent_command SET mutation_attempt_sequence = NULL WHERE tenant_id = ? AND operation = ? AND idempotency_key = ?", tenant, "RENAME_SESSION", "k1") — returned row count discarded, no assertion.
  • :577-582 (recreatedReceipt arm of latestRenameAttemptCompletesAfterItsSiblingRetires): jdbc.update("DELETE FROM managed_agent_command WHERE tenant_id = ? AND operation = ? AND idempotency_key = ?", tenant, "RENAME_SESSION", "k1") — same shape, same discard.
  • The behaviour the two arms exist to distinguish is ManagedAgentStore.supersededByLaterMutation (:2843), whose SELECT COALESCE(c.mutation_attempt_sequence, e.sequence_id) at :2846-2847 is the legacy fallback. V51__managed_mutation_attempt_sequence.sql is the migration that adds the bare nullable column, and it is the only configOrMigration risk signal in this PR's scope snapshot.
  • The exactly-one-row identity claim checks out independently: the same test reads that row with jdbc.queryForObject("SELECT command_status FROM managed_agent_command WHERE tenant_id = ? AND operation = 'RENAME_SESSION' AND idempotency_key = 'k1'", String.class, tenant) at :540-543, which would throw on a second match.

So a 0-row setup match collapses both boolean arms onto identical code with every assertion still passing. Real, and cheap to close — two assertThat(...).isEqualTo(1) wrappers, ~0 net additions.

Not locally verifiable by execution. packages/sdk-java/managed-agent-server cannot be built or tested on this box: no mvn, java is 1.8 (module targets 21), no ~/.m2, and the module is absent from the local main checkout. I read the source from the PR worktree at the live head; I did not run the "k1" -> "k1-absent" mutation.

Why no fix this round. Scope fuse: +2193/-341 over the sweep's 1500-addition limit ⇒ replies-only, no code, no push. Not human-gated — mechanical test hardening. Needs an owner session with Java 21 + Maven.

Left unresolved.


// The sibling still inside the Harness finishes K1 last: it must
// not revert the newer committed title.
assertThatThrownBy(() -> store.completeSessionMutation(tenant,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R3-4: The two new tests pin both arms of the new superseded arbitration (ManagedAgentStore.java:634, session_mutation_superseded) — retired-and-superseded → 409, retired-and-re-driven → completes — but neither pins the guard's third, load-bearing condition: that it is skipped for a still-PENDING receipt. "FAILED".equals(command.status()) && can be deleted and the module stays green.

Failure scenario. That precondition is what keeps a never-retired, currently-admitted rename from being refused. Reachable interleaving, every step legal today: beginSessionMutation(kA) → abandonSessionMutation(kA) (kA FAILED, boundary S_A) → beginSessionMutation(kB) (admitted, because hasOpenOperation counts only PENDING rows and kA is FAILED; S_B > S_A) → kA's sibling, still inside the Harness, calls completeSessionMutation(kA), which finds no completed control event after S_A, commits title "A" and appends session.updated after S_B → kB's own request then calls completeSessionMutation(kB) while kB is PENDING. With the precondition the guard is skipped and kB commits "B"; without it, supersededByLaterMutation(kB) counts kA's just-committed event and throws 409 session_mutation_superseded — "a later change completed after this request was retired" — for a request that was never retired, and the user's rename silently never applies.

Witness — mutation run in an isolated repo-layout Maven copy:

intact                                              Tests run: N, Failures: 0
"FAILED".equals(command.status()) && deleted        module still green

Suggested fix. Extend retiredRenameCannotCompleteOverALaterCompletedRename (or add a sibling case) with the mirror interleaving: begin kA, abandon kA, begin kB and leave it PENDING, completeSessionMutation(kA, ..., "A", bootId) — which must succeed, since nothing had committed after S_A — then assert completeSessionMutation(kB, ..., "B", bootId) does not throw and the title is "B".

The setup must abandonSessionMutation(kA) before beginSessionMutation(kB): hasOpenOperation (ManagedAgentStore.java:1121-1135) counts only command_status = 'PENDING' rows, so beginning kB while kA is still PENDING is refused with session_operation_active (pinned at ManagedWorkspaceAdmissionTest.java:800-804).

That new case is the acceptance criterion: deleting "FAILED".equals(command.status()) && from ManagedAgentStore.java:634 must make the final completeSessionMutation(kB) throw and the title assertion read "A" — please confirm the mutation reds it, since today nothing does.

中文说明

两个新测试固定了新的 superseded 仲裁(ManagedAgentStore.java:634,session_mutation_superseded)的两个分支——已退役且被后来者取代 → 409,已退役但被重新驱动 → 完成——但都没有固定该守卫第三个、真正承重的条件:对仍处于 PENDING 的回执要跳过仲裁。"FAILED".equals(command.status()) && 可以被删掉而整个模块依然通过。

触发场景。 该前置条件正是防止一个从未退役、当前已被准入的 rename 被拒绝的关键。今天完全合法的一种交错:beginSessionMutation(kA) → abandonSessionMutation(kA)(kA 变为 FAILED,边界 S_A)→ beginSessionMutation(kB)(被准入,因为 hasOpenOperation 只统计 PENDING 行,而 kA 已 FAILED;S_B > S_A)→ kA 那个仍在 Harness 内的兄弟请求调用 completeSessionMutation(kA),它发现 S_A 之后没有已完成的控制事件,于是提交标题 "A" 并在 S_B 之后追加 session.updated → 接着 kB 自己的请求在 kB 仍为 PENDING 时调用 completeSessionMutation(kB)。有该前置条件时守卫被跳过,kB 提交 "B";没有它时,supersededByLaterMutation(kB) 会统计到 kA 刚提交的事件并抛出 409 session_mutation_superseded——对一个从未退役的请求宣称"更晚的变更在本请求退役之后完成",用户的 rename 就此静默失效。

建议修复。 扩展 retiredRenameCannotCompleteOverALaterCompletedRename(或新增一个兄弟用例),补上镜像交错:begin kA、abandon kA、begin kB 并保持 PENDING、completeSessionMutation(kA, ..., "A", bootId)(此时必须成功,因为 S_A 之后没有任何提交),然后断言 completeSessionMutation(kB, ..., "B", bootId) 不抛异常且标题为 "B"。

前置构造必须先 abandonSessionMutation(kA) 再 beginSessionMutation(kB):hasOpenOperation(ManagedAgentStore.java:1121-1135)只统计 command_status = 'PENDING' 的行,因此在 kA 仍为 PENDING 时开始 kB 会被以 session_operation_active 拒绝(该行为已由 ManagedWorkspaceAdmissionTest.java:800-804 固定)。

这个新用例就是验收标准:从 ManagedAgentStore.java:634 删除 "FAILED".equals(command.status()) && 后,最后的 completeSessionMutation(kB) 必须抛异常且标题断言读到 "A"——请确认该变异使其变红,因为目前没有任何测试会。

— qwen3.8-max via Qwen Code /review (v0.25.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed at head ce56f4395766, read-only — with an independent static witness for the coverage half.

  • ManagedAgentStore.java:634 is if ("FAILED".equals(command.status()) && supersededByLaterMutation( — the third, load-bearing condition exists exactly as described, with its rationale in the comment at :630-633.
  • grep -rn "session_mutation_superseded" src/test --include=*.java across the whole Java test tree returns exactly one hit: ManagedSessionLifecycleTest.java:538. So the 409 is asserted in one place only, and that place (retiredRenameCannotCompleteOverALaterCompletedRename) calls store.abandonSessionMutation(tenant, "RENAME_SESSION", "k1", sessionId) at :519 first — k1 is already FAILED when it completes. The second new test likewise abandons k1 before completing it (:585-587). Neither arm ever reaches :634 with a PENDING receipt.
  • The named interleaving is consistent with hasOpenOperation counting only PENDING rows, which is why abandonSessionMutation(kA) must precede beginSessionMutation(kB).

Scope of my evidence. The grep proves the error code is asserted once; it does not by itself prove that no other test would redden if the "FAILED".equals(...) precondition were deleted (a test could assert only a successful completeSessionMutation). That half rests on the module-wide mutation run in the finding, which I could not re-run here — no mvn, java 1.8, no ~/.m2.

Why no fix this round. Scope fuse: +2193/-341 over the sweep's 1500-addition limit ⇒ replies-only, no code, no push. This is also the one Java item that would materially grow the diff (a new sibling case, ~15 lines) on a PR already over the fuse, so it touches human-gated condition ④ (enlarging the diff beyond the PR's promised scope) as well.

Left unresolved. Needs an owner session with Java 21 + Maven, and a maintainer call on whether that case belongs in this PR or a follow-up.

assertThatThrownBy(() -> guard.verify(binding))
.isInstanceOfSatisfying(RuntimeBrokerException.class,
error -> assertThat(error.isRetryable()).isFalse());
unreadable.set(0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R3-5: actionResponseRetriesAMomentaryMountReadWithoutChangingAcquireVerdicts promises in its name that acquire verdicts are unchanged, but it observes guard.verify only inside the failure window and never asserts it succeeds once the mount is readable again — the negative claim has no positive control. After unreadable.set(0) the test only calls connector.resolveAction(...), which exercises verifyProbe (the new retryable variant), never verify/verifyLocked — the acquire variants used by WorkspaceExecutionStore.claim (:187) and assertHeld (:201).

Failure scenario. The test therefore cannot distinguish "the shared acquire path is unchanged" from "the shared acquire path is now permanently poisoned by one momentary blip". A future change that records the failure inside WorkspaceStorageGuard.identity()'s catch (:637-642) — fencing the lease, or resetting mount_state to UNVERIFIED — would make every later Turn admission on that Workspace refuse terminally forever, and this test, the only one whose name claims to guard the acquire verdict, would stay green. That is the same "momentary blip becomes permanent" shape R2-3 was filed for on the sibling path.

Witness — mutation run in an isolated repo-layout Maven copy:

identity()'s IOException catch made to latch durably
  -> module suite green; this test reddens only once the positive control below is added

(The mutation this finding originally named is in fact detected; the surviving shape is the one above — the gap is real either way.)

Suggested fix. After unreadable.set(0); and before the retry, add the positive control:

Suggested change
unreadable.set(0);
unreadable.set(0);
guard.verify(binding);

matching the pattern the rest of this class already uses for a successful post-register verify.

The added assertion must not require re-classifying verify — the guard's own doctrine forbids it: "the shared acquire path above is deliberately untouched: re-classifying inside verify() would flip tool-turn verdicts" (WorkspaceStorageGuard.java:393-396), and identity() still maps IOException | RuntimeException to the terminal, cause-less unavailable() (:637-642). The control is that verify recovers, not that it becomes retryable.

With the added line, making identity()'s IOException catch latch the lease must turn this test red; without it the whole module stays green under that same mutation — please confirm both sides.

中文说明

actionResponseRetriesAMomentaryMountReadWithoutChangingAcquireVerdicts 在名字里承诺 acquire 判定不受影响,但它只在失败窗口内部观察 guard.verify,从未断言挂载恢复可读后 verify 能成功——这个否定式结论没有正向对照。unreadable.set(0) 之后测试只调用 connector.resolveAction(...),走的是 verifyProbe(新的可重试变体),从不走 verify/verifyLocked——也就是 WorkspaceExecutionStore.claim(:187)和 assertHeld(:201)使用的 acquire 变体。

触发场景。 因此该测试无法区分"共享 acquire 路径未变"和"共享 acquire 路径已被一次瞬时抖动永久污染"。将来若有人在 WorkspaceStorageGuard.identity() 的 catch(:637-642)里记录该失败——例如封锁租约,或把 mount_state 重置为 UNVERIFIED——该 Workspace 之后所有 Turn 准入都会永久终止性拒绝,而这个唯一在名字里声称守护 acquire 判定的测试依然通过。这与 R2-3 在兄弟路径上被提出的"瞬时抖动变成永久故障"是同一形态。

建议修复。 在 unreadable.set(0); 之后、重试之前加上正向对照 guard.verify(binding);(不得抛异常),与本类中既有的 register 后成功 verify 的写法一致。

新增断言不得要求重新分类 verify——guard 自身的约定禁止这样做:"上述共享 acquire 路径是刻意不动的:在 verify() 内重新分类会翻转 tool-turn 判定"(WorkspaceStorageGuard.java:393-396),且 identity() 仍然把 IOException | RuntimeException 映射为终止性、无 cause 的 unavailable()(:637-642)。这个对照要证明的是 verify 能恢复,而不是它变成可重试。

加上该行后,让 identity() 的 IOException catch 持久封锁租约,本测试必须变红;不加该行时,同一变异下整个模块仍然通过——请两侧都确认。

— qwen3.8-max via Qwen Code /review (v0.25.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed at head ce56f4395766, read-only.

  • WorkspaceStorageGuardTest.java:297 is unreadable.set(0);, followed only by connector.resolveAction("tenant", sessionId, "action", response) at :298 and verify(client).resolveAction(attached, ...) at :299. There is no guard.verify(binding) after recovery. The only guard.verify in this test is at :294, inside the failure window, asserting isRetryable()==false. So the name's "WithoutChangingAcquireVerdicts" claim has no positive control, exactly as described.
  • connector.resolveAction really does reach the probe variant and not the acquire variant: QwenHostedHarnessConnector.doResolveAction (:353-358) → requireReadyForNewWork(..., actionResponse=true) (:449-457) → WorkspaceExecutionStore.verifyMountForProbe (:58-62) → storageGuard.verifyProbe.
  • The acquire variants are separate, and they are precisely what the doctrine protects: WorkspaceStorageGuard.java:394-396 — "the shared acquire path above is deliberately untouched: re-classifying inside verify() would flip tool-turn verdicts". Consumers: WorkspaceExecutionStore.claim → storageGuard.verifyLocked(binding) at :187, and assertHeld → storageGuard.verify(binding) at :201.
  • The mutation shape named here is live code: WorkspaceStorageGuard.identity() maps IOException | RuntimeException to the terminal, cause-less WorkspaceExecutionStore.unavailable() at :636-642.

The gap is real and the suggested one-line control (guard.verify(binding); after unreadable.set(0);) matches the pattern this class already uses for a successful post-register verify. Smallest of the four Java items, and it does not require re-classifying verify — the control is that verify recovers.

Not locally executable. No mvn, java 1.8, no ~/.m2 on this box, and the module is absent from the local main checkout. I read source at the live head; I ran neither side of the latch mutation, so I cannot confirm the "module green / test reds only with the control added" pair myself.

Why no fix this round. Scope fuse: +2193/-341 over the sweep's 1500-addition limit ⇒ replies-only, no code, no push. Not human-gated — one added assertion, ~0 net additions. Needs an owner session with Java 21 + Maven.

Left unresolved.

chiga0
chiga0 previously approved these changes Oct 8, 2026

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Round 4 — incremental from 3c42bc6 → ce56f43

Incremental (1 file, test-only)

  • HostedPublicWorkspaceIT.java (+15/-9) — Replaces the freePort() helper (allocate → close → hope port is still free, a TOCTOU race) with simultaneous ServerSocket(0) binding in a single try-with-resources block: all three ports are bound together, their numbers recorded, then the sockets are closed before Spring and Harness start. This eliminates the port-collision window under parallel CI. Adds a startup diagnostic message-body assertion to the Harness health-check. ✓

No production code changed since round 3.

Cross-check against bot's latest CHANGES_REQUESTED (ce56f43)

12 carry-over Suggestion-level items from prior rounds; no new Critical findings at this HEAD. All prior Criticals confirmed fixed in rounds 2–3: requireBoundCreator/requireCanceller split, supersededByLaterMutation guard, mcpRecovering counter semantics, V51 migration pin in WorkspaceMigrationMySqlIT.

No blockers at HEAD ce56f439. Approving.

…response path

A revoked creation grant or a registry that left ACTIVE is
operator-mutable state, not the structural verdict the terminal exit
promises: stamping the committed approval decision FAILED over it
certifies a confirmation the Harness never saw, and restoring the grant
cannot recover it. The passive attachment authority now splits its
verdict into structural, grant-only, and authorized; only the
action-response delivery maps the grant-only refusal to a retryable
workspace_unavailable so the delivery machine re-arms while recovery
waits for the operator. Every other caller — creation, passive load,
cancellation — keeps the previous terminal refusal.
…is active

The helper re-validated registration and closing after its awaits but
never that the Session was still idle, and the latch clear removed the
only incidentally closing guard: a files/rewind admitted inside the
window had its prompt slot overwritten by the settle projection and its
AbortController dropped while /status reported the Session idle. Add
the third post-await re-validation the continue and managed-runtime
cancel routes already perform, answering the session-scoped refusal
before the projection can run.

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

COMMENT — not approving, but both standing Criticals are fixed at this head. Recorded so the fix is not re-litigated: I verified each in source at 2740c230d24460334026f894623a0d38dde73c7b, and neither thread's unresolved flag reflects the code any more.

Historical blocking issues: both closed at 2740c230

The standing block is round 3 CHANGES_REQUESTED at ce56f439, which carried exactly two Criticals. The commits after it (ce56f439...2740c230, six files, three of them production) answer both.

R3-1 — a durably committed approval decision was discarded over operator-reversible state. Fixed. The row mapper in WorkspaceExecutionStore.authorizeAttachment now returns a tri-state verdict per matched row instead of a boolean: 0 structural mismatch, 1 refused only by operator-mutable grant or registry state, 2 authorized. The state/can_read/can_create conjuncts moved out of the structural term into the 2 : 1 decision, and dispatch is:

if (grants.size() != 1) { throw unavailable(); }
if (grants.getFirst() == 1) {
    throw actionResponse ? unavailablePendingGrant() : unavailable();
}
if (grants.getFirst() != 2) { throw unavailable(); }

unavailablePendingGrant() is new RuntimeBrokerException(409, "workspace_unavailable", …, true) against unavailable()'s …, false), and the fourth constructor argument is retryable (RuntimeBrokerException.java:12-32, exposed as isRetryable() at :48). I followed the consumer: ActionResponseCoordinator:119-124 takes the terminal actions.complete(...) exit only when error instanceof RuntimeBrokerException failure && !failure.isRetryable() && "workspace_unavailable".equals(failure.getCode()), so the retryable variant now falls through to sessions.retryOperation(...) at :130 and a restored grant still reaches the Harness. The route into it is QwenHostedHarnessConnector.requireReadyForNewWork:456, which calls authorizeActionResponse(session) on the action-response arm only; authorizePassiveAttachment and authorizeCancellation keep their previous non-retryable behaviour, and the cancellation arm still returns 2 on structural match so retries stay independent of mutable creation grants.

R2-2 — the reattach helper re-validated registration and draining after its awaits but never idleness, and resident.blocked = false removed the latch that incidentally closed the window. Fixed. hosted-harness-session.ts:1830-1834 now guards before the latch is cleared:

if (resident.active !== undefined) {
  noteOwedAdoption(resident, sessionId);
  error(res, 409, 'hosted_turn_active');
  return;
}
refusedAdoptions.delete(sessionId);
resident.blocked = false;

A turn that became active during the await is now refused as hosted_turn_active and recorded as owed, rather than being raced by runSettleProjection's unconditional session.active assignment. The same commit adds 140 lines to hosted-harness-session.test.ts and 91 to WorkspaceStorageGuardTest.java, which also addresses the R3-2 and R3-5 test-coverage Suggestions.

The four remaining unresolved threads (R3-2, R3-3, R3-4, R3-5) are all graded Suggestions and do not affect this verdict.

Gate I could not complete in budget

This review is a COMMENT rather than an approval for one reason: the Critical-only scan covered the incremental diff since the last blocking review, not the PR's own base diff. Roughly 2,200 added lines across 32 files — the ManagedAgentService submit/rename gate sequence, ManagedAgentStore superseded arbitration, the V51__managed_mutation_attempt_sequence migration, the WorkspaceExecutionStore authorization SQL, and the WebShell capability plumbing — were not read at this head within the review budget, and the earlier rounds' Criticals (R1-1, R1-2, R1-4, R1-5, R2-1, R2-3) were not re-verified in source; round 3 carried only R2-2 forward, which is evidence about what that reviewer re-assessed, not an independent check. For a change whose subject is authorization under revocation, I am not willing to certify that surface unread.

CI

CI is mid-flight on this head: 13 checks pass, 12 are still pending — including Test (ubuntu-latest, Node 22.x), Lint & Static, Hosted process fault gates / MySQL 8.4 / Java 21, Runtime Broker and Managed Agent MariaDB / Java 21 and Real daemon E2E / Java 11. No failure is attributable to this PR. Pending CI is not treated as a blocker here and is not the reason for the COMMENT; it does mean the Java fault gates that cover this PR's own changed integration tests have not yet reported on the commits that fixed R3-1.

Next step: the two Criticals need no further work. Once CI finishes green on 2740c230, a re-review that reads the base diff — or a fresh automatic round at this head — is what turns this into an approval.


中文说明

结论:COMMENT,暂不批准;但两个待处理的 Critical 在当前 head 上均已修复。 记录在此以免重复讨论:我在 2740c230d24460334026f894623a0d38dde73c7b 的源码上逐条核实,两个线程的「未解决」标记已不再反映代码实际状态。

历史阻塞问题:在 2740c230 上均已关闭。 当前的阻塞是 ce56f439 上的第三轮 CHANGES_REQUESTED,其中只有两个 Critical。其后的提交(ce56f439...2740c230,共 6 个文件,其中 3 个是生产代码)分别回应了这两个问题。

R3-1 — 已持久化提交的审批决定因运维可逆状态被丢弃。已修复。 WorkspaceExecutionStore.authorizeAttachment 的行映射器现在对每条匹配行返回三态判定,而不再是布尔值:0 结构性不匹配,1 仅因运维可变的授权或注册状态被拒,2 已授权。state/can_read/can_create 三个条件从结构性判定中移出,改为决定 2 : 1;分派逻辑为:grants.size() != 1 抛 unavailable();判定为 1 时按 actionResponse ? unavailablePendingGrant() : unavailable();判定不为 2 时抛 unavailable()。unavailablePendingGrant() 是 new RuntimeBrokerException(409, "workspace_unavailable", …, true),而 unavailable() 传 false,第四个构造参数即 retryable(RuntimeBrokerException.java:12-32,通过 :48 的 isRetryable() 暴露)。我核实了消费端:ActionResponseCoordinator:119-124 仅在 !failure.isRetryable() 且 code 为 workspace_unavailable 时走终态 actions.complete(...),因此可重试的变体现在会落到 :130 的 sessions.retryOperation(...),授权恢复后仍能送达 Harness。入口是 QwenHostedHarnessConnector.requireReadyForNewWork:456,只在 action-response 分支调用 authorizeActionResponse(session);authorizePassiveAttachment 与 authorizeCancellation 保持原有不可重试行为,且 cancellation 分支在结构匹配时仍返回 2,因此重试不依赖可变的创建授权。

R2-2 — 重挂载辅助函数在 await 之后重新校验了注册与 draining,却从未校验会话是否仍空闲,而 resident.blocked = false 又移除了原本偶然关闭该窗口的闩。已修复。 hosted-harness-session.ts:1830-1834 现在在清除该闩之前加入保护:resident.active !== undefined 时调用 noteOwedAdoption,返回 409 hosted_turn_active 并直接返回。await 期间变为活跃的 Turn 现在会被拒绝并记为欠付,而不是被 runSettleProjection 对 session.active 的无条件赋值抢跑。同一提交为 hosted-harness-session.test.ts 增加 140 行、为 WorkspaceStorageGuardTest.java 增加 91 行,同时回应了 R3-2 与 R3-5 的测试覆盖建议。

其余四个未解决线程(R3-2、R3-3、R3-4、R3-5)均被评定为 Suggestion,不影响本结论。

预算内未能完成的门禁: 本次给出 COMMENT 而非批准只有一个原因——Critical-only 扫描覆盖的是自上次阻塞评审以来的增量 diff,不是本 PR 自身的基础 diff。约 2200 行新增、跨 32 个文件的内容未在当前 head 上读完,包括 ManagedAgentService 的 submit/rename 门禁顺序、ManagedAgentStore 的 superseded 仲裁、V51__managed_mutation_attempt_sequence 迁移、WorkspaceExecutionStore 的授权 SQL 以及 WebShell 能力位接线;早几轮的 Critical(R1-1、R1-2、R1-4、R1-5、R2-1、R2-3)也未在源码上复核。第三轮只把 R2-2 继续追踪,这只能说明那位评审者复核了什么,不构成独立核实。对一个主题正是「授权被撤销时如何处置」的改动,我不愿在未读过的范围上给出认证。

CI: 当前 head 上 CI 仍在进行:13 项通过,12 项 pending,其中包括 Test (ubuntu-latest, Node 22.x)、Lint & Static、Hosted process fault gates / MySQL 8.4 / Java 21、Runtime Broker and Managed Agent MariaDB / Java 21 与 Real daemon E2E / Java 11。没有可归因于本 PR 的失败。Pending 的 CI 在此不作为卡点,也不是给出 COMMENT 的原因;但它确实意味着覆盖本 PR 所改集成测试的 Java fault gate 尚未就修复 R3-1 的这些提交给出结果。

下一步: 两个 Critical 无需再改。等 2740c230 上 CI 全绿后,由一次读完基础 diff 的复审(或该 head 上新一轮自动评审)即可转为批准。

yiliang114 and others added 2 commits October 8, 2026 11:49
# Conflicts:
#	packages/cli/src/serve/hosted-harness-session.ts
#	packages/sdk-java/managed-agent-server/src/main/java/com/alibaba/qwen/code/managedagent/store/WorkspaceExecutionStore.java
#	packages/sdk-java/managed-agent-server/src/main/resources/openapi/managed-agent-public-api.openapi.json
#	packages/web-shell/client/components/managed/generated/managed-agent-api.ts
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Verified the published head 576a39a90bd08cce1dd9ca3cf928e719a33b7dde after the concurrent main merge. It is conflict-free and retains main’s L3 lifecycle authority together with the two delivered Critical fixes (234037, 2740). The mutation-attempt SQL is unchanged at V52; the MySQL upgrade assertion now includes V52.

  • Current-head CLI build, typecheck, formatting and lint pass. The full Hosted Harness suite passes 295/295, including lifecycle adoption/detach, passive resident recovery, cancellation and the post-await rewind guard. HEAD and checkout cleanliness were checked before and after execution.
  • WebShell contract/provider/page suites pass 85/85 on local merge candidate fc53eadd; their source and the public OpenAPI contract are byte-identical at this published head. The original run keeps its SHA attribution.
  • MySQL/MariaDB, changed Java cancellation renewal behavior, full runtime acceptance and external approvals remain pending. Earlier native/browser reports retain their tested SHAs. Bounded source review found no new confirmed Critical in the inspected merge paths; it is not an exhaustive approval of the whole PR.

The local merge candidate is preserved unpublished after the ordinary push rejected a concurrent branch update. I adopted the already-published synchronization for verification. Existing Suggestions remain deferred under the Critical-only rule; the scope ledger preserves the original baseline and reconstructs the two delivered batches since round19.

@qqqys: once current-head CI passes, this still needs the full-base re-review identified in your last review. I have not marked it merge-ready.

@yiliang114
yiliang114 requested a review from qqqys October 8, 2026 04:03
@yiliang114

Copy link
Copy Markdown
Collaborator Author

One remaining action-response path needs verification at 576a39a90bd08cce1dd9ca3cf928e719a33b7dde: a cold connector cache performs two authority checks. doResolveAction first calls the new action-response check, then attachment(..., false) calls passive createOrLoad, whose second check still uses ordinary passive-attachment classification.

If the operator revokes can_create or changes ACTIVE to DRAINING between those real SQL reads, the first check can pass and the second can emit non-retryable workspace_unavailable. The action-response coordinator then takes its terminal failure exit although no approval answer reached the Harness. The public producer is an already admitted ACTION_RESPONSE dispatched with an empty connector cache.

The existing grant test changes the row before the first check, while the cold-cache connector test mocks the authority store; neither exercises this interleaving. This is source-supported reachability, not an executed Java witness yet. I am preparing a real JDBC check and have retained the R3-1 thread until that path is verified. The 295/295 CLI suite does not cover this Java path.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Fixed the remaining cold-attachment form of R3-1 in 00be8ed0ff12b370a0bbac6bae29cde163f50e55. The second authorization check during approval reattachment now uses the same action-response classification: a revoked creation grant or changed registry state remains retryable, while mismatched workspace identity remains terminal.

Verification: the extended real-JDBC test fails on the preceding production head at the retryability assertion, then passes with this repair; it also verifies restored delivery to the original approval and terminal generation drift. 90 focused Java tests and Checkstyle pass. The first combined run had one unrelated child-process timeout; an unchanged rerun passes all90. Incremental correctness/security reviews found no actionable defect in the two-file repair. The earlier 295 Harness tests ran at parent 576a39a; those sources are unchanged. Full-PR review and new-head CI are still required; this report is not an approval.

The previously fixed R2-2 idle-slot guard is retained and covered by that 295-case Harness run. Existing Suggestions stay deferred under Critical-only scope.

@qqqys please review the complete current diff once required CI passes.

This branch has not been deployed

No deployments
Sign up for free to 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.

5 participants