feat(mesh): add persistent shared-thread agent collaboration - #11206
feat(mesh): add persistent shared-thread agent collaboration#11206yiliang114 wants to merge 170 commits into
Conversation
…tion Companion to the design's §5.2: what each of the ten steps lands, the gate that closes it, and the evidence to report. Records the one-PR delivery shape, the C2 correction from round 2, and the #9402 board relationship as a decision owed before storage work.
This is the single mesh PRDecision from the owner: the shared-thread subsystem ships as one PR, this one, rather than a stack. What that means for the branch:
Acceptance criteria per step are now on this branch: One decision owed before step 3 (storage protocol): the relationship between this thread store and the agent board in #9402. Both are filesystem-backed shared work items under the runtime dir with per-item locks. Board becomes the thread store, thread store supersedes the board, or they stay separate with a written reason. Two stores for one concept is the outcome to avoid. Still open product decisions the implementer must not make silently: §9.4, §9.5, §9.9, §9.10, §9.11 in the design. |
Records the runtime seams steps 2 and 3 reuse (executionAllowedTools, the AST read-only shell classifier, tsx for two-process tests), the storage protocol decisions, the named tests each step needs, the dependencies on #11200/#11202/#11204, and two design conflicts owed to the owner: MCP under the read-only ceiling, and batching the v1 schema.
Steps 2-3 brief committed (e08ddde)
Baseline observed on a build-less box with symlinked |
executionAllowedTools is name-level; a per-command read-only predicate needs an invocation-time hook that only exists once the launcher lands. Step 2 proves the predicate and the table; step 4/5 proves the wiring.
|
Thanks for the PR! Template is complete ✓ — and the Risk & Scope section is unusually honest about what has never run, which made this much easier to review. The head moved seven times while I was reading this ( Problem. This is a Direction. The area is clearly live: Two questions here, one of which you answered while I was writing it:
Size. Core paths are touched, so the breakdown at the reviewed commit: 1372 mesh production lines ( Approach. The delivery shape is now explicit in acceptance §4, which is better than what I inferred earlier: #11206 is the only PR that merges to What the answer surfaces instead, and this is my main remaining question: §4 also records that a PR based on the mesh branch runs no unit tests and no lint in this repo — I checked, Related, and smaller: the design's §0.2 is candid that "no agent has run this design end to end" and the dispatch rules are "reasoned from Multica's and Agent Team's failure modes, not from observed behaviour of this system"; acceptance puts the first live gate at step 7, and §5's watch list item 1 says to run it "as early as the stack allows". I'd second that instinct strongly — the rules landing here are validated by 38 tests against hand-built fixtures for six more steps before anything validates them against a running system, and watch item 1 is the one thing a model can break that no fixture will catch. Risk. No high-risk path match on the revert-correlation scan, even after the runtime merges — none of the ten touched runtime files are on the flagged list. But the scan is path-based and coarse: Two things I want to credit explicitly: keeping thread text under These are questions, not blocks. Moving on to code review — I found one concrete defect in the mesh storage layer, and a separate set of findings in the folded runtime changes, including one where the PR description no longer matches the diff. 🔍 中文说明感谢贡献！模板完整 ✓ —— 其中 Risk & Scope 部分对"哪些从未跑过"写得异常坦诚，这让评审轻松了很多。 我阅读期间 head 变了七次（ 问题。 这是 方向。 这个领域确实活跃： 这里有两个问题，其中一个你在我写的过程中已经回答了：
规模。 触及了核心路径，所以在被评审提交上的拆分：mesh 生产代码 1372 行（ 方案。 交付形态现在在验收文档 §4 中写明了，比我之前的推断更好：#11206 是唯一合入 而这个答案带出的问题，是我剩下的主要疑问： §4 同时记录了基于 mesh 分支的 PR 在本仓库不跑任何单元测试和 lint——我核对过， 相关的、更小的一点：设计文档 §0.2 很坦率——"no agent has run this design end to end"，且 dispatch 规则是"reasoned from Multica's and Agent Team's failure modes, not from observed behaviour of this system"；验收文档把第一个 live 门禁放在 step 7，而 §5 观察清单第 1 项说要在"stack 允许的最早时机"跑它。我强烈赞成这个直觉——此处落地的规则，在之后还有六步的时间里，其验证来源是 38 个针对手工构造 fixture 的测试，而不是一个真实运行的系统；而观察清单第 1 项正是那种模型能打破、任何 fixture 都抓不到的东西。 风险。 revert 相关性扫描没有命中高风险路径，即使在运行时合并之后也没有——被触及的十个运行时文件都不在被标记的清单上。但该扫描是基于路径且粗粒度的： 有两点我想明确肯定：把线程文本放在 以上是问题，不是阻断项。进入代码审查——我在 mesh 存储层发现了一个具体缺陷，在折入的运行时改动中发现了另一组问题，其中一条是 PR 描述已与 diff 不符。🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
…did not Per the plan's own rule — report observations, not percentages, and write "not run" where nothing ran. Each phase gets what landed, what is still absent, and the human decisions still blocking it. Refs #11206
… attached A2A JSON-RPC and the Host outbound pickup are both transport-only now: the semantics, storage and authorisation behind each are implemented and covered by assertions. The brief says which functions to call rather than what to build, so neither gets reimplemented alongside what already passes. The constraints carry their reasons, because several are load-bearing in ways that read as style otherwise: A2A's four authorisation failures must stay one undifferentiated refusal at the transport or the caller can enumerate agents; the public Agent Card is a different document from the per-caller one and must not list agents; streaming and pushNotifications stay false because the spec gates its optional operations on them; and every Host write goes through checkRunLease rather than a second reconciliation, since that check is the only thing stopping a returning worker overwriting its successor. Acceptance for each is an observation, not a build: A2A against the Python a2a-sdk chosen in P1 as the independent interop client, and the Host path against two Hosts where the first one's late write is refused as stale_lease. Refs #11206
Keep each ACP attempt active until its own terminal result, isolate managed Hosts from coordinator scheduling, and route peer results back to waiting agents. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… audit suite Two findings from reviewing the transport work. **The audit harness had stopped running.** `thread_create` now requires `assignee` — a defensible narrowing, since an unassigned sub-thread is work nobody owns — but the harness still called it without one, so it threw on startup and all 335 assertions silently stopped covering anything. Every one of the seven transport commits landed with the suite dead. Harness updated; it is green again, and the one assertion that then failed was its own fault, not the code's: pinning `autoTurnsUsed === 7` pinned the booking behaviour too, when what it is for is that a child inherits the parent's count rather than resetting. **`pickupRunForHost` handed out work the dispatcher refuses.** It is a second selection path beside `selectCandidates`, and it had no terminal-thread check — so a Host could be given a queued run on a thread whose caller had cancelled it or whose owner had marked it done, while the dispatcher correctly ignored the same run. This is exactly the failure `isThreadTerminal` was introduced to prevent when the seven existing sites were unified, and it is an eighth site added afterwards without it. Observed rather than argued: the harness gains a section that first asserts a Host *can* pick up live work — without that the negative case passes for the wrong reason, which it did on the first attempt — then marks the thread terminal and asserts both paths agree. 339 passed, 0 failed; mutation-checked by removing the guard again. The held-lease branch gets the same check. A thread that goes terminal while a Host holds it should not have that hold extended — the work is over. Refs #11206
`pickupRunForHost` decides what work leaves this daemon for another machine and `applyHostRunResult` is where a stale worker would do damage. Both landed with no assertions; neither needed a network to be checked. Pickup: a Host takes only work for an agent placed on it, never another Host's and never a locally-executed agent — and the local dispatcher leaves managed-host work alone, which is the other half of that separation, since without it both sides run the same task. A reconnecting Host resumes the lease it already holds rather than being handed a second copy of its own work under a new id. Concurrency is enforced across Hosts. Results: a lease id nobody holds, a different Host under the right lease id, a previous attempt, and an unknown run are each refused — and the refusals are asserted before the legitimate write, so none of them passes merely because the run was already closed. The holder's own result closes the run with the kind it reported rather than an implicit success, moves the thread to review, and resending it is recognised as a replay instead of closing twice. A failure reports as a failure with the Host's reason and no close kind invented for it. Mutation-checked, all four bite: dropping the hostId check on results (3 red), not recognising a replay (1), not distinguishing which Host is asking (6), and skipping the concurrency limit (1). 369 passed, 0 failed. One assertion was wrong on the first attempt and the API was right: testing the concurrency limit against a single Host asserted nothing, because a Host asking again resumes the assignment it already holds and the resume branch answers before the limit is ever consulted. The limit only decides anything when a second Host is placed on the same agent, which is what it now tests. Refs #11206
`routes/a2a.ts` mounts routes on an Express app, so the honest way to check it is to mount them and make requests — testing its internals would test a shape no client speaks. A fake workspace registry points at a temp project root; the store beneath is the real one, and there is no daemon and no model. It skips with exit 0 when the SDK is not installed rather than failing for the wrong reason. What it pins is the part a JSON-RPC layer can quietly get wrong. The public card lists no agents and names none, so anyone may fetch it. Six different authorisation failures — no credentials, wrong secret, ungranted agent, unknown agent, another caller's identity, unknown workspace — all answer with the same code and the same message, and a task that is not yours is byte-for-byte indistinguishable from one that does not exist. Streaming is refused, matching the capability the card advertises. Idempotency and caller scoping survive the transport: a resent message id yields the same task and the store holds one piece of work, a reused id with different content is a distinct error naming the existing task, and a second client gets its own task and cannot read, cancel or list the first's. Three assertions were wrong before they were right, each for a reason worth keeping: - Every request was missing `A2A-Version`, so the SDK assumed the legacy 0.3 wire version and refused all of them with one code — which turned "all refusals look alike" green without testing anything. The header is now sent, and the version negotiation itself is asserted. - The unauthenticated case sent empty params, so it failed parameter validation before reaching auth and reported a leak that was not there. Every refusal now sends a well-formed request, so the only difference between them is authorisation. - The public card's `skills` is absent rather than `[]`, because protobuf `toJSON` omits empty repeated fields. 34 passed, 0 failed. Mutation-checked: leaking why a refusal happened (2 red), listing agents on the public card (2), and answering differently for "not yours" than for "no such task" (1). A fourth mutation, swapping the not-found error type, correctly survives — it changes both cases alike and leaks nothing. Refs #11206
`thread_create` requires an assignee; `createThread` does not. That reads like an inconsistency and is not one: an agent splitting work must say who does it, or it has created work nothing will ever dispatch, while a person may raise a thread and decide later. Both halves are now asserted, along with the fact that the refusal names the missing field and leaves no orphan thread behind — it is rejected by the tool's own schema, so the model never reaches an execution it could misread as partial success. Also records the four audit scripts and what each covers in the plan, since "335 passed" in a commit message ages badly and the counts had already moved. 373 passed, 0 failed. Refs #11206
# Conflicts: # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/acp-integration/session/Session.ts # packages/core/src/agents/background-agent-resume.test.ts
…clear the CI gates Three findings from reviewing the branch after main was merged in. **A definition's external executor was silently dropped.** Subagent definitions gained an `executor` block (#11003): the turn runs on an external agent over ACP. Workspace Agents borrow a definition's prompt, model and tool config through `agentType`, but they never go through `createAgentHeadless`, which is the only thing that honours an executor — they are their own top-level session. So attaching such a definition took its instructions and ran the turn locally as Qwen: the operator asked for one runtime and got another wearing the first one's prompt. It now refuses, on exactly the reasoning the neighbouring rendered-prompt case already gives, and the message points at `execution.mode: "managed-host"`, which is how a workspace Agent actually says it runs elsewhere. Confirmed by execution before and after: the assertion failed with "resolved" first. **Seventeen files across the PR failed the Prettier gate**, mine and the transport work's alike. CI runs Prettier with `--experimental-cli`, which rejects files the classic CLI passes, so neither of us saw it. All formatted; the whole changed set is clean. **`routes/a2a.ts` had never been linted** and carried six errors. The failure mapping now has an exhaustiveness default, so adding an `A2AFailure` member is a compile error at the one place that decides what a caller is told rather than a silent fall-through; the unused registry parameter is marked; and the two streaming stubs are declared as methods returning `AsyncGenerator` instead of generators that never yield — which also means an unsupported operation fails when it is called rather than when a client first iterates a stream that will never arrive. The transport audit still reports streaming refused. Also checked and found sound, so recorded rather than changed: unknown tools still classify as `deny`, so an upstream tool landing without a classification fails closed; both lockfiles carry `@a2a-js/sdk` and its transitive `jose`; and the new upstream `SessionSourceService` is about attached content sources, not `sourceType: agent`, so it does not touch the server-binding check. 376 / 34 / 21 / 15 across the four audits, all green after the merge. Refs #11206
e0f7579 made a close @-mention peers whose runs ended `waiting`, so they resume. The risk in that is repeat wake-ups: if the wait were not acknowledged when answered, every later close on the thread would wake the same agent again and spend its turn budget on nothing new. Now asserted end to end: both agents mid-turn on one thread (a wait is only legal while something can still wake it), the waiter closes `waiting`, the closer's review wakes it with one new run and acknowledges the wait, the woken run is drained, and a second close wakes nothing. Every step is read from the store rather than a tool's return shape. The first version of this scenario had the waiter call `thread_wait` alone on the thread; the tool refused ("waiting would strand it"), the refusal sat in `llmContent` rather than `.error`, and the scenario went on asserting against a run that had closed `unclosed`. Draining the woken run before the second close is also deliberate — left queued, a second wake-up would coalesce into it and the count would stay flat for the wrong reason. Mutation-checked. Dropping the @-mention turns both wake assertions red. Dropping the acknowledgement in `closeRunInTransaction` turns the "acknowledged" assertion red but not the end-to-end one — because a person's post also acknowledges outstanding waits (`thread-actions.ts`), and the second closer turn here is triggered by one. So the property is defended at two sites, and the two assertions pin one each. Recorded rather than engineered around. 383 passed, 0 failed. Refs #11206
…h was failing on `thread_create` was narrowed to require `assignee`, and `thread-tools.test.ts` was not updated with it. That is a type error, `tsc --build` fails on it, and `npm run build --workspace=packages/core` fails with the build — so **all four failing CI jobs on this PR fail on the same two lines**: Lint & Static, Test, Integration Tests (no-AK) and web-shell E2E Smoke each build before they do anything else. The main CI has been red on this branch since 2026-09-08 and has never been green. Two call sites, both in a test about where a sub-thread lands rather than about assignment, so they take the same `bob` the neighbouring cases use. Also adds `scripts/audit/tsconfig.workspace-agents-tests.json`, because this should not have needed CI to find. The existing narrow configs exclude `*.test.ts` — they were written to check production sources — but CI compiles test files too, so a break confined to a test was invisible locally while being fatal remotely. The new config covers the workspace-agents tests and `thread-tools.test.ts`, and was verified by reverting the fix: it reports both TS2345s, then none. 383 passed, 0 failed on the behavioural audit; the four A2A/gate/capture suites are unaffected. Refs #11206
Restores CI. `pull_request` workflows run against the merge ref, so a conflicting PR gets no run created at all — not a failed one, none. This PR has been `mergeable: CONFLICTING` since main moved past the previous merge, which is why the main CI last ran at 06:01 and every push since produced only the `pull_request_target` bot workflows. It also meant the type-error fix in the previous commit could not be verified remotely. One conflict, in `acpAgent.ts`, and only in an import list: this branch adds `resolveAgentPersona`, `findAgentSessionBinding`, `resolveModelId` and `buildModelIdContext`; main adds `registerSession`. Both kept — the usages on each side are intact (`registerSession` at the session-registry call, the binding check at agent session creation). Audits on the merged tree: 383 / 34 / 21 / 15, all green. Narrow typechecks clean, including the new test-file config. Refs #11206
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review — head ea57d8b3, base main (draft)
SHA drift, stated up front: I began this review at ba5238e9 and the head has since moved to ea57d8b3bfc44713e9f413384c1cb56dc7b7e29e. Everything below has been re-anchored and re-verified at ea57d8b3 — I re-read each cited file at the new head rather than carrying forward the earlier line numbers. Note the tree also moved: what was under agents/mesh/ now lives under agents/workspace-agents/.
Scope. +31 707 / −329 across 136 files. Author is a repository admin, so the two-tier core-infrastructure gate does not block this; but the non-blocking size advisory in AGENTS.md triggers at 1 000 production logic lines and this is roughly thirty times that, with zero reviews so far. I read a minority of the diff — call it under 10% — and I want to be honest that this review is a spot check, not coverage. My concrete recommendation is to split this into reviewable increments the way the codex/mesh-step-* series does; as one PR it is not reviewable to the standard the rest of this subsystem is being held to, and a reviewer who approves it will be approving mostly unread code.
Critical 1 — A2A existence oracle: the not-found branch runs before authorization
In packages/core/src/agents/workspace-agents/a2a-server.ts, a2aGetTask looks the thread up first and returns not_found before it ever calls authorize:
const thread = await getExternalThreadForCaller(projectRoot, caller.callerId, taskId);
if (!thread || !thread.externalIntake)
return { ok: false, kind: 'not_found' };
const auth = await authorize(projectRoot, caller, thread.externalIntake.targetAgentId, 'analysis');
if (!auth.ok) return { ok: false, kind: 'refused' };The cancel path has the same shape. authorize is where the grant secret is actually checked, via checkA2AGrant.
To be precise about the severity, because it is narrower than it first looks: getExternalThreadForCaller is already scoped by callerId, so this is not a cross-tenant enumeration. What it does give you is that a caller who knows a callerId but not its secret can distinguish not_found from refused, and thereby probe which task ids exist under that caller. The file's own comment on a2aGetTask says not_found deliberately conflates "no such task" and "not yours" "because a caller able to tell them apart can enumerate another client's task ids" — the refused/not_found split reintroduces exactly the distinction that comment is defending against, just one authentication step earlier. authorize gets this right internally (it checks the secret before the agent-existence lookup, with a comment saying why); the outer call sites invert it.
Fix is ordering: authorize first, then look up, and return not_found for both the unauthorized and the absent case.
Compounding this — and I'd class this part as a Suggestion, not the Critical — buildUser in packages/cli/src/serve/routes/a2a.ts returns AuthenticatedA2AUser with isAuthenticated = true on header shape alone: /^Bearer ([A-Za-z0-9_-]{32,})$/ plus the presence of the workspace/caller/agent headers. The secret is never verified at that layer. The grant check does happen later, so this is not itself a bypass, but any transport-level logic that trusts isAuthenticated (logging, rate limiting, the authenticated() gate) is trusting a claim nothing has established. Renaming it, or deferring the flag until the grant verifies, would stop the next reader from relying on it.
Critical 2 — the retention bounds effectively never bind, and the outbox is never pruned
trimThread in store.ts (:1071-1109 at this head) pins runs from trimming:
const retainedRuns = thread.runs.filter(
(run, index) =>
index >= firstRetainedRun ||
run.usageByRound.length > 0 ||
run.status === 'queued' || run.status === 'running' ||
run.status === 'finishing' || run.status === 'cancelling',
);run.usageByRound.length > 0 is true for every run that consumed any tokens — i.e. every run that actually did work. So MAX_THREAD_RUNS = 200 never trims a real run. Messages inherit the same fate: they are retained if originEventId !== undefined (every externally-originated message) or if any retained run references them through triggerMessageIds / acceptedMessageIds / consumedMessageIds / finalMessageId. With runs never trimming, the referenced set never shrinks, so MAX_THREAD_MESSAGES = 500 does not bind either.
The declared bounds are therefore documentation rather than enforcement, and a long-lived thread's JSON grows without limit — read, validated, and atomically rewritten in full on every write, so the cost is quadratic in thread lifetime, not linear.
Separately, trimThread does not touch thread.outbox at all. It only ever grows (outbox: [...thread.outbox, stored]). Delivered and failed events are retained forever.
If the pinning is deliberate — and the token-usage condition reads like it is, presumably to preserve accounting — then the accounting needs to be summarised into a scalar so the runs themselves can be dropped. Either way the current combination cannot hold: a bound that is unreachable by construction is worse than no bound, because callers size their expectations against it.
Critical 3 — prompt-frame spoofing via agent-authored thread title and body
Agent-authored thread title and body are emitted into the run frame at the same 2-space indentation as authoritative frame fields such as Status: and Assignee:. A model that controls a title or body can therefore emit lines indistinguishable from fields the runtime asserts. This is the same defect class as the one in the sibling prompt-assembly PR in this series; it is worth fixing once, structurally, rather than per-field — a distinct prefix for content, or a fenced content region the frame parser and the reader can both rely on.
Suggestion — bilingual design docs are missing
Two new design docs land here: docs/design/2026-09-09-a2a-frozen-contract.md (English) and docs/design/2026-09-09-agent-service-collaboration.md (entirely Chinese, under a plain .md name). Neither has a .zh-CN.md sibling, and there are no reciprocal language links. AGENTS.md requires linked English and Chinese versions with matching structure, decisions, constraints, and acceptance criteria. Per the same rules a translation gap alone is a Suggestion, so I am not counting it as Critical — but the second file being Chinese-only under the English filename is the part most likely to confuse tooling later.
Verdict
C=3. The size is my main concern and it is not a formality: I found three Criticals in under a tenth of the diff, which is weak evidence about the rest but not encouraging. Limitations: no local test run, and I did not read the majority of the changed files.
…ype block
`Lint & Static` was still red after the build fix, on two warnings — and CI runs
`eslint . --max-warnings 0`, so a warning is a failure.
`Session.ts` already had one `import type { … }` block and one value block from
`@qwen-code/qwen-code-core`. This branch added `type AgentRunContext` inline in
the value block, which gives the module two type-import surfaces and trips
`import/no-duplicates` on both. Moved into the existing type block, which is the
arrangement main has and lints clean.
Confirmed it was ours rather than pre-existing by linting main's copy of the
file in place: clean there, two warnings here.
This is also a mistake in how I had been checking. I ran eslint with
`| grep -v "no-duplicates"` throughout this work and read the result as clean —
filtering out the one warning class that CI treats as fatal. `npm run lint:ci`,
the exact CI command, now passes repo-wide with no output; that is what I should
have been running all along.
383 passed on the behavioural audit; Prettier clean.
Refs #11206
# Conflicts: # packages/acp-bridge/src/bridge.test.ts # packages/acp-bridge/src/bridgeTypes.ts
Two more CI failures on 8d3e506, one mine and one from the session-naming work. **`Lint & Static`: "settings.schema.json is out of date."** I added `experimental.agentCollaboration` to `settingsSchema.ts` and never regenerated the JSON schema the gate diffs against. The generator cannot run in this sandbox — it resolves `@qwen-code/qwen-code-core/envVarResolver`, and the installed copy here predates that subpath export — so the entry is written by hand from its sibling `agentTeam`'s exact shape: same key order (`description`, `type`, `default`), same position between `agentTeam` and `artifact` as in the source. The description is asserted equal to the one in `settingsSchema.ts` verbatim, and the file is Prettier-clean. CI's regenerate- and-diff step is what confirms it; I could not. **`Test (ubuntu)`: `config.getWorkspaceAgentName is not a function`** — every failure in `acpAgent.test.ts` traces to one unguarded call in `createAndStoreSession`. The method exists on a real `Config`, but this layer is handed Config-shaped objects that are not always complete, which is why `getWarnings`, `getSessionId` and `getFailedMcpServerNames` in the same file are all called through a `typeof … === 'function'` guard. This one now is too, so a missing method costs the session its agent title instead of failing session creation outright. That test file cannot run here (it needs the workspace packages built, which OOMs in this sandbox), so the second fix is reviewed rather than executed — CI is the check. The alternative, teaching the test double the method, was rejected as the less conventional of the two: the guard follows three precedents in the same file. 383 passed on the behavioural audit; `npm run lint:ci` clean repo-wide. Refs #11206
**954 of the CLI shard's 557 test failures were one line of mine.** `Session.ts` called `this.config.isAgentCollaborationEnabled()` unguarded, and that layer is handed Config-shaped objects that are not always a full Config. This is the identical bug I diagnosed in `getWorkspaceAgentName` one commit ago and fixed there without looking at my own two call sites — the general lesson was in hand and I did not apply it. Both sites now go through one `#collaborationEnabled()` helper, so a third cannot forget the guard, and the third unguarded read in `acpAgent.ts`'s agent session branch is guarded too. Absent means off everywhere, which is the safe reading in both places: no run frame is established, and an agent session on a Config that cannot say whether the operator opted in is refused rather than granted. Swept the repo for other unguarded calls to either method; there are none. **NOTICES.txt** was the remaining `Lint & Static` failure (the settings-schema entry from the previous commit worked). The regenerated file is one line: `jose@6.1.3` → `jose@6.2.12`, the transitive dependency `@a2a-js/sdk` pulls in. Worth recording how that was produced, because the first attempt was wrong: run in this worktree as it stood, the generator saw 655 dependencies against an incomplete `node_modules` and rewrote the file with 589 insertions and 7481 deletions. CI's own diff was `2 +-`. A real `npm ci --ignore-scripts` first, then regenerate, reproduces CI's one-line diff exactly. A generated licence file is not something to hand-write or approximate. Audits: 383 / 34 / 21. `npm run lint:ci` clean repo-wide. The CLI test shard cannot run here (it needs the workspace packages built), so the guard is verified by the failure signature it removes rather than by a local green run. Refs #11206
CI's Test job is down to 24 failures across three shards; this clears all of them. Local runs became possible partway through — `tsc --build packages/core` with a 12GB heap succeeds in this sandbox, contrary to what I had recorded — so every fix below was run rather than reasoned about, except the three `server.test.ts` capability cases, which are blocked locally by an unrelated `web-templates` generator gap and were verified by comparing my expected order against `getRegisteredServeFeatures()` directly (index 47, between `workspace_agent_generate` and `workspace_env`, absent without the toggle). Registries that had to learn about what we added: `agent_collaboration_v1` is now documented in the serve protocol's conditional-features table, present in the registered-feature list in declaration order, has its predicate branch in the drift-insurance test, and `QWEN_CODE_ENABLE_AGENT_COLLABORATION` is declared in the process.env allowlist with its reason. The six `thread_*` tools gained Web Shell display names and zh translations. The four `--agent-host-*` serve flags are registered as fast-path fallbacks — a managed Host is a different startup mode, so handing off to the full CLI is right. Tests that encoded behaviour we deliberately changed: thread status reasons are prose for a person now, with the run id in `outstanding`; the GAP label dropped "earlier"; the persona states "not a subagent" rather than avoiding the word; `thread_create` reports work queued rather than woken; a close @-mentions waiters, so a review leaves the thread in progress rather than in review; a failed run posts a system message, so acknowledgement sequences shift; an `open` sub-thread with nobody on it cannot satisfy a wait; and the resumed workspace-Agent ceiling denies shell while always adding the thread tools. `ResidentAgentContinuationResult` is a string union, not a boolean. Two were worse than stale. `dispatcher.test.ts` asserted the delivery watermark moves at start, which `954c1ffa29` deliberately stopped doing — the local port reports `consumedOnStart: false` and the input is confirmed after the transcript flush. And `config.test.ts`'s collaboration-tools case asserted against `getAllToolNames()`, which this file stubs to `[]` at module scope: its positive half could never pass and its negative half could never fail. It now reads the recorded `registerFactory` calls, clearing the shared prototype mock between the two registries so the ordinary session does not inherit the agent's registrations. Local: 1171 core tests and 111 cli tests that were failing now pass; audits 383 / 34 / 21; `npm run lint:ci` clean repo-wide. Refs #11206
…r Goal-only CI is down to two failures from twenty-four; this clears both. **i18n.** Adding the six `thread_*` display names to zh alone was half the job: `check-i18n` requires every key to exist in `en.js` first and to be translated in both Chinese locales, so the run failed with "Extra key in zh.js (not in en.js)". English base keys and zh-TW translations added; `check-i18n` now passes all checks. The Web Shell keeps its own `toolName.*` table, which also needed the six — that is what its own drift test caught. **`terminateTurn` outside a Goal turn.** The test asserted an ordinary turn keeps its tool loop, and its comment explained why: "nothing but the Goal tool sets it today". That is no longer true — the workspace-Agent closing tools set it too, and `#endTurnAfterToolRun` (renamed from `#endGoalTurnAfterToolRun`) deliberately dropped the Goal-turn guard, because a hand-off makes later work in the same physical turn stale for the same reason a checkpoint does. The case now asserts the flag is honoured wherever it is set; tools that do not set it keep the ordinary loop, which the neighbouring cases already cover. `npm run lint:ci` clean, `check-i18n` green, and the two test files pass locally (`toolFormatting.test.ts` 51/51; the Session case is verified against CI's own received value, since that file needs a `web-templates` generator this sandbox cannot complete). Refs #11206
…exports map
The last two test failures cleared; `Lint & Static` went green for the first
time. What surfaced behind them was five failures in `scripts/tests/dev.test.js`,
all from one line this branch added.
The acp-bridge source remapping reads that package's `exports` and iterates it
directly. With no `exports` field — which is what the launcher sees when the
manifest read is stubbed, and what it would see against any manifest that omits
one — `Object.entries(undefined)` throws at module load, so the launcher dies
for every caller rather than skipping a remapping it has no input for. Absent
now means nothing to remap.
Verified by running the suite: 6/6 after, and removing the `?? {}` again
reproduces `TypeError: Cannot convert undefined or null to object` across the
file.
Refs #11206
Second time this branch has gone `CONFLICTING` and stopped getting CI: the previous push produced zero `pull_request` runs, because those execute against a merge ref GitHub cannot build for a conflicting PR. This repository's main moves fast enough (25 commits since the last merge) that the PR needs main merged on roughly the cadence it is pushed to, or CI silently stops — silently being the problem, since the run list simply shows nothing rather than a failure. One conflict, in `toolFormatting.ts`, and the same shape as last time: both sides appended to `TOOL_DISPLAY_NAMES` — the six `thread_*` names here, `exec` on main. Both kept, and the drift test that exists to catch exactly this passes (53/53 across both display-name suites). Gates on the merged tree: `check-i18n` green, audits 383 / 21, `scripts/tests/dev.test.js` 6/6. One local-only lint warning remains and is deliberately not acted on: `packages/qwen-live/src/memory/tokenize.ts` reports an unused `import/no-internal-modules` disable here because `@node-rs/jieba` is a platform-specific optional dependency this sandbox skipped, so the rule cannot resolve the import it guards. The file is byte-identical to main and CI's last lint run reported nothing for it. Removing the directive to quiet the local run would break the real gate. Refs #11206
Four jobs failed on the previous run and three of them shared one cause, in our file: `AGENT_TOOL_CLASSIFICATION` must cover every `CoreToolName`, and main added `exec`, so `tsc --build` failed on `capability.ts` and took the core build — and with it Test, Integration Tests and web-shell E2E — down with it. `exec` is classified `deny`, and that is the security-relevant answer rather than a formality. It is the code-mode entry point: ordinary tools are reached from inside an exec program as `tools.<name>(args)`. Allowing it would let an Agent call `tools.run_shell_command(...)` and every other denied tool through the wrapper, undoing the whole read-only ceiling in one line. Coverage is now exact — no unclassified core tool, nothing classified that is not one. The fourth failure was `check-lint-gate-freshness`: main changed `eslint.legacy-filenames.mjs` after the merge I had just pushed. main moved again during the run, so this merges it again (6 more commits, no conflicts). Local runs are trustworthy again: my `npm ci` predated main's code-mode and jieba dependencies, so `tsc` reported implicit-`any` errors in `code-mode/host.ts` that CI never saw, and eslint reported an unused `import/no-internal-modules` disable in `qwen-live`. Both were missing-package artefacts. Reinstalling clears them, which also confirms the earlier decision not to "fix" that disable directive — it is load-bearing wherever the package is actually present. Core builds clean; 840 agent/config/tool tests pass; audits 383 / 34 / 21 / 15; `npm run lint:ci` and `check-i18n` both green with nothing filtered. Refs #11206
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at 214b895b1b9760f51a35bc3f6cf61fd8edc86cad — 3 confirmed Criticals; Partial review — coverage gaps. COMMENT only; no approval implied.
Pinned base: 518f6795f9eda4b9911c2a1e8ea76ddc787551cb. This is the current 145-file scope, not the stale worktree or only the delta since my ea57d8b3 review. The selected head moved before acquisition; all findings below use the pinned head above. Author admin permission was verified, so the external-refactor gate does not apply. The large-feature advisory does: 72 non-test packages/*/src files alone contribute 13,494 changed source lines before removing comments/types, excluding the web client and an API-omitted patch. This is not an exact production-logic count or a size-based code defect.
Critical — task existence is disclosed before the caller secret is checked (previous Critical 1: still stands)
packages/core/src/agents/workspace-agents/a2a-server.ts:174-195,227-244 looks up the caller's task before authorize; absent tasks return not_found, while existing tasks with a bad/revoked secret return refused. external-intake.ts:245-255 does scope the lookup to callerId: this is an existence oracle for a known caller ID and candidate task ID, not a cross-tenant content-read bypass.
The actual transport preserves the distinction: packages/cli/src/serve/routes/a2a.ts:80-105 accepts credential header shape before the core grant check, and :118-128 emits different JSON-RPC errors. These routes are registered before the daemon bearer middleware (packages/cli/src/serve/server.ts:2159-2168). Thus a client that can reach the opt-in endpoint within its host/origin boundaries can probe task membership without the caller secret. Authenticate before producing task-dependent responses, or make unauthenticated absent/present outcomes indistinguishable without weakening the subsequent agent-scoped grant check.
Critical — retention does not bound charged runs or acknowledged outbox history (previous Critical 2: still stands, narrowed)
packages/core/src/agents/workspace-agents/store.ts:1071-1109 retains every run with nonempty usageByRound, regardless of the 200-run limit, and also retains messages referenced by those runs regardless of the 500-message limit. This is live data: dispatcher.ts:737-758 charges positive measured spend through thread-actions.ts:828-854, which leaves the ledger nonempty after completion. store.ts:1864-1929 appends outbox events and marks them acknowledged, but never compacts them; trimThread does not touch the outbox.
The result is retained history beyond the advertised bounds, repeatedly parsed, validated and rewritten as a whole (store.ts:1034-1050,1112-1125) under the workspace lock. Preserve usage totals and replay/idempotency guarantees in compacted accounting/history before dropping old records; simply removing the ledger predicate would break the actual admission/UI consumers that sum those ledgers.
Two corrections to my earlier wording: originEventId pins replayable parent-report messages, not every externally submitted message; and the current default 1,000,000-token admission budget limits paid admission, so I am not claiming an ordinary paid thread necessarily runs forever. It does not enforce the declared record-count limits or provide acknowledged-outbox retention.
Critical — the Host's renewal timer can claim work that it never executes (new)
packages/cli/src/serve/agent-host-client.ts:240-250 implements renewal by calling general pickup(..., 0) every 20 seconds and discarding both assignments and errors. The route at packages/cli/src/serve/routes/agent-hosts.ts:227-235 really invokes pickupRunForHost. That operation only renews an unexpired, running hold; otherwise it selects work and creates a new lease (packages/core/src/agents/workspace-agents/host-lease.ts:299-334,336-437; the lease lasts 60 seconds).
Concrete path: execution continues while renewal is unavailable for over a lease lifetime. The next timer call can reacquire the task with lease L2, but the executing client keeps L1. It submits L1 at agent-host-client.ts:318-325; the server correctly rejects it at host-lease.ts:234-248, and the client discards the completed result at agent-host-client.ts:348-354. Work can then execute again. Cancellation exposes the same ownership error without a network outage: the UI marks the original run cancelling (routes/workspace-agents.ts:1497-1537), so a timer pickup can claim another eligible queued run and keep renewing it while its assignment is ignored and the original execution continues. That reserves work which has not actually started.
Renew the exact thread/run/attempt/lease instead of performing admission from the renewal timer, and handle lease loss/cancellation in the execution owner. Do not accept stale results as a workaround.
Previous findings and remaining coverage
- Previous Critical 3 (prompt-frame spoofing): cannot fully clear or independently reconfirm its claimed security impact.
prompt.ts:176-196still renders model-authored title/body with the same prefix as Status/Assignee;thread_createremains a producer. Howeverpersona.ts:56-58explicitly labels title/body/posts untrusted, and actual mutation authority is the ambient run tuple checked against storage (run-lifecycle.ts:70-90), not prose parsing. The formatting witness persists; it is not counted here as a proven authority bypass. No model-behavior probe was run. - Older foundation triage: the per-thread
{}seed and unlocked deletion defects are addressed by the current workspace transaction and missing-file handling (store.ts:593-638,1034-1050,1820-1860). Queue counts and tree usage now come from stored threads inside admission (thread-actions.ts:176-210,251-263);resolveTargetsnow requires the explicit-mention flag (dispatch-policy.ts:181-194). These are narrow source confirmations, not acceptance of every historical claim. Earlier shared-runtime deliveryId/provenance, cumulative-round consumers, capacity/resume and lifecycle concerns are not exhaustively cleared. - Inspected current core admission/dispatcher, prompt/persona/capability, thread tools and ambient binding, A2A grants/intake/transport, Host enrollment/pickup/results, session dispatch and selected ACP delivery receipts. Route ownership: discovery is process-global; workspace-agent routes resolve a selected trusted runtime and persisted workspace; Host/A2A use the selected active workspace plus their own credentials; task execution uses its session/run owner. No primary-runtime fallback was found in these inspected paths. This does not certify every async workspace-replacement race.
- Meaningful gaps: complete storage validation/migrations/crash recovery; the remainder of run-lifecycle/thread-status/thread-actions; all folded background-agent/runtime/shared-consumer changes; full bridge metadata trust and lifecycle seams; indirect skill/hooks/tool-provider paths; web-shell UI; all tests/audit harnesses/plans and dependency integration. The whole 145-file PR has not received a full pass. Prior naming/documentation Suggestions remain deferred; no new Suggestions.
Validation: static review of pinned source and selected tests only. No PR code, tests, builds, live daemon/A2A/Host/provider requests, or network-failure experiments were executed; author demo results are not reviewer-run evidence. This posted partial review is not full review completion.
5 participants
What this PR does
Adds persistent workspace Agent identities that collaborate on shared threads. People can create and assign work, address one or more agents, interject while work is running, inspect attributed results and per-run history, cancel work, resolve blockers, and mark reviewed thread trees done. Agents can post to peers, create assigned child threads, wait for dependencies, block on a person, and submit work for review.
The runtime uses a hidden workspace-qualified host and durable FIFO bookings. It supports concurrent agents on one thread, resident continuation across threads, structured delivery ids, parent dependency reports, restart replay, bounded turns and tokens, source-first cancellation, stall recovery, and ambient workspace/thread/run provenance. Mesh agents get workspace-scoped read tools plus the six shared-thread tools; shell and MCP stay denied until they have enforceable isolation.
The Web Shell exposes the persistent roster and shared-thread ledger. It previews routing with the production admission rule, displays the resolver's actual status reason, keeps child work and run history beside the conversation, preserves deleted-agent attribution, and reports when a durable mutation succeeded but immediate background processing did not.
Why it's needed
Qwen Code can already launch short-lived subagents, but it has no durable colleague-like identities that share a visible work ledger and can be steered by a person while collaborating. This PR provides that single-machine v1 without claiming multi-user, remote-runtime, or write-capable isolation that does not yet exist.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: the branch provided only admission rules and storage sketches; no hosted mesh Agent had run end to end.
After: a real daemon, Web Shell, and real model completed the parent-child handoff and resident parent continuation to
in_review. A latest-source restart replayed a queued run and completed it ascompleted/review. A human interjection was present in both accepted and consumed delivery records and changed the same run's final result. In a separate shared thread, Alice and Bob enteredrunning138 ms apart, posted independently attributed reviews, and the aggregate thread reachedin_review. A daemonSIGKILLwhile running recovered the same run on attempt 2; a second crash with one accepted-but-unconsumed message replayed and consumed that message on attempt 2, and the final review contained both requested replay markers. After merging the latest 14 main-branch commits, a fresh assigned thread again auto-dispatched and completedthread_reviewtoin_review.A frozen-head correctness and trust-boundary review at
e8510f0f0da3rechecked the five earlier critical paths as fixed and found one remaining P1: read-only shell classification did not confine absolute paths to the workspace. The follow-up removes shell from mesh instead of pretending command classification is a filesystem sandbox.Earlier named implementation checks are recorded in the acceptance document. The latest demo-first passes did not run unit tests, lint, typecheck, or CI; only the ACP bridge package output was refreshed to replace a stale shared-worktree build.
Tested on
Risk & Scope
666,749 / 200,000because no new admission occurred between tool rounds.Linked Issues
Related to #9402, #10078, #10247, and #11140. Extracted from #11072.
中文说明
这个 PR 做了什么
加入持久的工作区 Agent 身份和共享线程协作。人可以创建和指派工作、同时唤醒多个 Agent、在运行中插话、查看署名结果和单次 run 历史、取消、解除 blocker，并把验收后的线程树标为 done。Agent 可以给同事发帖、创建并指派子线程、等待依赖、向人提问和提交验收。
运行时通过隐藏的工作区 host 和持久 FIFO booking 驱动，支持同线程并发 Agent、跨线程长期执行体续跑、结构化 delivery id、父子依赖回报、重启重放、turn/token 门禁、取消和卡死恢复，以及 ambient workspace/thread/run provenance。Mesh Agent 只拿工作区范围的读取工具和六个线程工具；shell 与 MCP 在具备可执行隔离前保持禁用。
Web Shell 提供持久 roster 和共享工作 ledger，使用真实准入规则预览路由，直接展示 resolver 的状态原因，并保留子线程、run 历史和删除 Agent 后的署名。
为什么需要
Qwen Code 已经能启动短生命周期 subagent，但没有能长期存在、共享可见工作记录并允许人随时纠偏的“同事型”Agent。这个 PR 交付单机 v1，不冒充尚未具备的多用户、远程 runtime 或写入隔离能力。
Reviewer 验证计划
如何验证
证据（前后对比）
Before：分支只有准入规则和存储草图，没有 hosted mesh Agent 的端到端运行证据。
After：真实 daemon、Web Shell 和真模型完成了父子交接、父 Agent 长期执行体续跑并进入
in_review。最新源码重启后重放 queued run 并完成为completed/review。人的运行中插话同时出现在 accepted/consumed 记录里，并改变了同一 run 的最终结果。另一个共享线程中 Alice 与 Bob 相差 138ms 进入running，分别提交署名 review，线程最终聚合为in_review。running 时SIGKILLdaemon 后，同一个 run 以 attempt 2 恢复；第二次崩溃前一条消息已 accepted 但未 consumed，重启后同一个 run 消费了它，最终 review 包含两条指定的重放标记。合入 main 最新 14 个提交后，又创建了一个全新指派线程，仍能自动派发并完成thread_review到in_review。早期命名检查记录在验收文档。最新 demo-first 验证没有运行单测、lint、typecheck 或 CI；只刷新 ACP bridge 包产物以替换共享 worktree 的旧 build。
风险与范围
666,749 / 200,000。