diff --git a/src/agentkit/bitable/repository.py b/src/agentkit/bitable/repository.py index a4fd4ff..426780b 100644 --- a/src/agentkit/bitable/repository.py +++ b/src/agentkit/bitable/repository.py @@ -39,6 +39,26 @@ from agentkit.bitable.models import ( logger = logging.getLogger(__name__) +# U3/DR-1: field_id 在 jsonb_set path 与 where 子句中走字符串插值(jsonb path 不支持绑定参数)。 +# 防御性校验:field_id 必须匹配 UUID 格式,拒绝任意字符串插值到 SQL 结构。 +# ponytail: ceiling — 仅覆盖 field_id;其余 SQL 结构(op / direction)已由 service 层白名单枚举校验。 +# 升级路径:迁移到 ORM JSON 函数(sqlalchemy.dialects.postgresql.jsonb_*)彻底消除字符串插值。 +_FIELD_ID_RE = re.compile( + r"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$" +) + + +def _validate_field_id(field_id: str, *, context: str = "") -> str: + """校验 field_id 是 UUID 格式(DR-1 防御性校验)。 + + field_id 在 jsonb_set path 与 SQL where 子句中走字符串插值(jsonb path 不支持绑定参数), + 必须确保非系统 UUID 不进入 SQL 结构。校验失败抛 ValueError。 + """ + if not isinstance(field_id, str) or not _FIELD_ID_RE.match(field_id): + ctx = f" ({context})" if context else "" + raise ValueError(f"Invalid field_id format{ctx}: {field_id!r} — expected UUID") + return field_id + class BitableRepository: """Async repository for bitable CRUD operations. @@ -375,7 +395,9 @@ class BitableRepository: return [Record.model_validate(e) for e in entities], next_cursor - async def update_record_values(self, record_id: str, values: dict[str, object]) -> Record | None: + async def update_record_values( + self, record_id: str, values: dict[str, object] + ) -> Record | None: """Update a record's values (full replace).""" async with self._session_factory() as session: stmt = ( @@ -471,6 +493,35 @@ class BitableRepository: await session.commit() return result.rowcount > 0 + async def delete_view_if_not_last(self, view_id: str, table_id: str) -> bool: + """Atomically delete a view only if it's not the last one in its table (DR-4 TOCTOU fix). + + Uses ``SELECT ... FOR UPDATE`` in a single transaction to lock sibling + rows for the duration of the count + delete, preventing concurrent + ``delete_view`` calls from racing past the last-view check and leaving + the table with zero views. + + Returns: + True if the view was deleted. + False if NOT deleted because it would be the last view (or the + view doesn't belong to the given table_id). + """ + async with self._session_factory() as session: + # Lock all sibling views — concurrent delete_view calls on the + # same table block here until this transaction commits. + lock_sql = text( + "SELECT id FROM bitable.bitable_views WHERE table_id = :table_id FOR UPDATE" + ) + result = await session.execute(lock_sql, {"table_id": table_id}) + sibling_ids = {str(row[0]) for row in result.fetchall()} + if len(sibling_ids) <= 1: + return False # Last view — refuse to delete + if view_id not in sibling_ids: + return False # View doesn't belong to this table_id + result = await session.execute(delete(ViewModel).where(ViewModel.id == view_id)) + await session.commit() + return result.rowcount > 0 + # ── Recalc Queue ──────────────────────────────────────── async def enqueue_recalc( @@ -646,9 +697,13 @@ class BitableRepository: return import json + # U3/DR-1: 校验所有 field_id 是 UUID 格式后再插值到 jsonb_set path(防御性,避免 SQL 结构注入) + for fid in agent_field_values: + _validate_field_id(fid, context="upsert_record_agent_fields") + async with self._session_factory() as session: # Build nested jsonb_set: jsonb_set(jsonb_set(values, '{f1}', CAST(:v0 AS jsonb), true), ...) - # ponytail: field_ids are system UUIDs, safe to interpolate into path literals. + # ponytail: field_ids validated above as UUID format, safe to interpolate into path literals. # Use CAST(:param AS jsonb) instead of :param::jsonb — asyncpg dialect # misparses the `::` as part of the param name. inner = "values" @@ -684,9 +739,17 @@ class BitableRepository: import base64 import json as _json + # U3/DR-1: 校验 filters/sorts 中的 field_id 是 UUID 格式后再插值到 SQL 结构 + if filters: + for f in filters: + _validate_field_id(f["field_id"], context="list_records_filtered.filter") + if sorts: + for s in sorts: + _validate_field_id(s["field_id"], context="list_records_filtered.sort") + async with self._session_factory() as session: # Build raw SQL with JSONB filter/sort translation. - # ponytail: field_ids in filters/sorts are system UUIDs (validated by service layer). + # ponytail: field_ids validated above as UUID format, safe to interpolate into SQL structure. where_clauses = ["table_id = :table_id"] params: dict[str, object] = {"table_id": table_id} diff --git a/src/agentkit/bitable/service.py b/src/agentkit/bitable/service.py index 7245247..ad81b31 100644 --- a/src/agentkit/bitable/service.py +++ b/src/agentkit/bitable/service.py @@ -545,23 +545,28 @@ class BitableService: return await self._repo.get_view(view_id) async def delete_view(self, view_id: str) -> bool: - """Delete a view with last-view protection (U6). + """Delete a view with last-view protection (U6, DR-4 TOCTOU fix). Raises :class:`LastViewDeletionError` if the view is the last one in its table — preventing users from making a table inaccessible. The route layer is responsible for the 404 + ownership checks before calling this (matching the existing ``delete_field`` pattern). Returns True if a row was deleted. + + DR-4: delegates to ``repo.delete_view_if_not_last`` which atomically + checks sibling count + deletes in a single ``SELECT FOR UPDATE`` + transaction, eliminating the TOCTOU race between ``list_views`` count + and ``delete_view`` execution. """ view = await self._repo.get_view(view_id) if view is None: return False - siblings = await self._repo.list_views(view.table_id) - if len(siblings) <= 1: - raise LastViewDeletionError( - "Cannot delete the last view of a table" - ) - return await self._repo.delete_view(view_id) + deleted = await self._repo.delete_view_if_not_last(view_id, view.table_id) + if not deleted: + # Either it was the last view, or a concurrent call raced ahead. + # Either way, the invariant "table has at least 1 view" is preserved. + raise LastViewDeletionError("Cannot delete the last view of a table") + return True # ── Recalc (U3: formula recalc pipeline) ──────────────── diff --git a/src/agentkit/server/frontend/src/stores/chatStream.ts b/src/agentkit/server/frontend/src/stores/chatStream.ts index 39f35c9..020d95f 100644 --- a/src/agentkit/server/frontend/src/stores/chatStream.ts +++ b/src/agentkit/server/frontend/src/stores/chatStream.ts @@ -869,7 +869,8 @@ export function dispatchWsEvent( case "team_synthesis_chunk": { // U4: 团队综合流式 chunk — 首次创建 streaming 占位,后续累加。 - // P2 fix: 用 synthesis_id 去重,避免 chunk 附身到上一次孤儿 streaming milestone。 + // P2 fix: 用 synthesis_id 精确去重 — sid 存在时仅匹配相同 synthesis_id 的 milestone, + // 不再兜底附身到 undefined synthesis_id 的旧孤儿占位(避免跨重试附身 bug)。 const conversationId = state.resolveIncomingConvId(); if (!conversationId) break; const conv = state.conversations.value.find( @@ -882,13 +883,11 @@ export function dispatchWsEvent( (m) => m.message_type === "milestone" && m.status === "streaming" && - (sid ? m.synthesis_id === sid || m.synthesis_id === undefined : true), + (sid ? m.synthesis_id === sid : true), ); if (existing) { state.updateMessage(conversationId, existing.id, { content: (existing.content || "") + (event.data.chunk || ""), - // 首次附身到无 synthesis_id 的 milestone 时,标记上 synthesis_id - ...(sid && !existing.synthesis_id ? { synthesis_id: sid } : {}), }); } else { const synthMsg: IChatMessage = { @@ -926,7 +925,7 @@ export function dispatchWsEvent( (m) => m.message_type === "milestone" && m.status === "streaming" && - (sid ? m.synthesis_id === sid || m.synthesis_id === undefined : true), + (sid ? m.synthesis_id === sid : true), ); if (existing) { state.updateMessage(conversationId, existing.id, { diff --git a/src/agentkit/server/frontend/tests/unit/bitable-tokens.test.ts b/src/agentkit/server/frontend/tests/unit/bitable-tokens.test.ts new file mode 100644 index 0000000..e028777 --- /dev/null +++ b/src/agentkit/server/frontend/tests/unit/bitable-tokens.test.ts @@ -0,0 +1,97 @@ +/** + * DR-3: Bitable design tokens — static structural assertion. + * + * Parses `bitable-tokens.css` and asserts all required `--bitable-*` tokens + * are defined inside `:root`. Prevents accidental token removal/renaming + * that would silently break var() references in bitable components. + * + * ponytail: pure regex parse — no CSS parser dependency. Ceiling: doesn't + * validate token values (only presence). Upgrade path: postcss + stylelint + * custom rules for value-level validation. + */ +import { describe, expect, it } from 'vitest' +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' + +const CSS_PATH = resolve( + __dirname, + '../../src/styles/bitable-tokens.css', +) + +const cssContent: string = readFileSync(CSS_PATH, 'utf-8') + +/**Extract the first `:root { ... }` block content (without the selector).*/ +function extractRootBlock(css: string): string { + const match = /:root\s*\{([^}]*)\}/.exec(css) + if (!match || match[1] === undefined) { + throw new Error('No :root block found in bitable-tokens.css') + } + return match[1] +} + +const rootBlock = extractRootBlock(cssContent) + +/**Required token names grouped by category (mirrors the CSS file structure).*/ +const REQUIRED_TOKENS = { + color: [ + '--bitable-color-primary', + '--bitable-color-bg', + '--bitable-color-bg-secondary', + '--bitable-color-bg-tertiary', + '--bitable-color-text', + '--bitable-color-text-secondary', + '--bitable-color-text-tertiary', + '--bitable-color-text-placeholder', + '--bitable-color-border', + '--bitable-color-border-hover', + '--bitable-color-border-split', + ], + cf: [ + '--bitable-cf-red-bg', + '--bitable-cf-red-fg', + '--bitable-cf-orange-bg', + '--bitable-cf-orange-fg', + '--bitable-cf-yellow-bg', + '--bitable-cf-yellow-fg', + '--bitable-cf-green-bg', + '--bitable-cf-green-fg', + '--bitable-cf-blue-bg', + '--bitable-cf-blue-fg', + '--bitable-cf-purple-bg', + '--bitable-cf-purple-fg', + '--bitable-cf-gray-bg', + '--bitable-cf-gray-fg', + '--bitable-cf-neutral-bg', + '--bitable-cf-neutral-fg', + ], + drawer: ['--bitable-drawer-width', '--bitable-drawer-width-wide'], +} as const + +describe('bitable-tokens.css — DR-3 design token contract', () => { + it('defines a :root block', () => { + expect(rootBlock.length).toBeGreaterThan(0) + }) + + it.each(REQUIRED_TOKENS.color)('defines color token %s in :root', (token) => { + // Match `--token-name:` (declaration), not just substring presence. + const declPattern = new RegExp(`${token.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*:`) + expect(declPattern.test(rootBlock)).toBe(true) + }) + + it.each(REQUIRED_TOKENS.cf)('defines conditional-format token %s in :root', (token) => { + const declPattern = new RegExp(`${token.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*:`) + expect(declPattern.test(rootBlock)).toBe(true) + }) + + it.each(REQUIRED_TOKENS.drawer)('defines drawer token %s in :root', (token) => { + const declPattern = new RegExp(`${token.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*:`) + expect(declPattern.test(rootBlock)).toBe(true) + }) + + it('does not hardcode tokens outside :root (no component-level overrides)', () => { + // ponytail: bitable tokens are exclusively :root-scoped; component-level + // overrides would break the single-source-of-truth invariant. + const outsideRoot = cssContent.replace(/:root\s*\{[^}]*\}/, '') + expect(outsideRoot).not.toMatch(/--bitable-[a-z-]+\s*:/) + }) +}) diff --git a/src/agentkit/tools/bitable_tool.py b/src/agentkit/tools/bitable_tool.py index 6365155..b6d760e 100644 --- a/src/agentkit/tools/bitable_tool.py +++ b/src/agentkit/tools/bitable_tool.py @@ -558,15 +558,23 @@ class BitableTool(Tool): field_id = kwargs.get("field_id") if not field_id: return {"success": False, "error": "Missing required field: field_id"} + # DR-5: type 变更不被支持 — UpdateFieldRequest 当前只接受 name + config, + # 传入 type 会被 Pydantic extra=ignore 静默丢弃(用户以为改了类型,实际无效果)。 + # 显式拒绝并返回错误,避免静默失败的 UX 陷阱。 + # 升级路径:如需支持类型变更,扩展 UpdateFieldRequest 接受 field_type + + # 实现数据迁移逻辑(类型转换 + 校验)。 + if kwargs.get("type") is not None: + return { + "success": False, + "error": ( + "Unsupported field type change — field type is immutable after " + "creation. To change a field's type, delete and recreate it." + ), + "code": "UNSUPPORTED_FIELD_TYPE_CHANGE", + } payload: dict[str, object] = {} if kwargs.get("name") is not None: payload["name"] = kwargs["name"] - # ponytail: PATCH /fields/{id} is backed by UpdateFieldRequest which - # currently accepts only name + config. `type` is forwarded as - # `field_type` so the tool is forward-compatible once the request - # model adds it; today it is silently ignored by Pydantic (extra=ignore). - if kwargs.get("type") is not None: - payload["field_type"] = kwargs["type"] if kwargs.get("config") is not None: payload["config"] = kwargs["config"] client = await self._get_client() diff --git a/tests/unit/bitable/test_bitable_tool.py b/tests/unit/bitable/test_bitable_tool.py index 1f885fd..8b6b75b 100644 --- a/tests/unit/bitable/test_bitable_tool.py +++ b/tests/unit/bitable/test_bitable_tool.py @@ -552,9 +552,9 @@ async def test_update_view_action(tool: BitableTool) -> None: """update_view action PATCHes /views/{id} with name + config.""" result = await tool.execute(action="create_table", table_name="VU") table_id = result["table"]["id"] - view_id = ( - await tool.execute(action="create_view", table_id=table_id, name="Old") - )["view"]["id"] + view_id = (await tool.execute(action="create_view", table_id=table_id, name="Old"))["view"][ + "id" + ] resp = await tool.execute( action="update_view", @@ -682,9 +682,7 @@ async def test_new_actions_internal_token_passthrough( table_id = result["table"]["id"] # create_view via the new action. - v = await tool_real_auth.execute( - action="create_view", table_id=table_id, name="tv1" - ) + v = await tool_real_auth.execute(action="create_view", table_id=table_id, name="tv1") assert v["success"] is True view_id = v["view"]["id"] @@ -716,3 +714,60 @@ async def test_delete_view_internal_token_404_when_missing( resp = await tool_real_auth.execute(action="delete_view", view_id="no-such-view") assert resp["success"] is False assert "404" in resp["error"] + + +# --------------------------------------------------------------------------- +# DR-5: _update_field rejects type change (no silent success) +# --------------------------------------------------------------------------- + + +async def test_update_field_rejects_type_change(tool: BitableTool) -> None: + """DR-5: _update_field with `type` arg returns error, does NOT call HTTP. + + Without this guard, the field_type would be silently dropped by + UpdateFieldRequest's Pydantic extra=ignore, and the agent would + believe the type changed when it didn't. + """ + result = await tool.execute(action="create_table", table_name="DR5") + table_id = result["table"]["id"] + client = await tool._get_client() + field_id = ( + await client.post( + f"/tables/{table_id}/fields", + json={"name": "col", "field_type": "text", "owner": "user"}, + ) + ).json()["field"]["id"] + + resp = await tool.execute( + action="update_field", + field_id=field_id, + type="number", + ) + assert resp["success"] is False + assert resp.get("code") == "UNSUPPORTED_FIELD_TYPE_CHANGE" + assert "immutable" in resp["error"] + + +async def test_update_field_still_works_without_type(tool: BitableTool) -> None: + """DR-5: _update_field without `type` still PATCHes name/config normally. + + Regression guard: the DR-5 guard must not block legitimate updates. + """ + result = await tool.execute(action="create_table", table_name="DR5b") + table_id = result["table"]["id"] + client = await tool._get_client() + field_id = ( + await client.post( + f"/tables/{table_id}/fields", + json={"name": "col", "field_type": "text", "owner": "user"}, + ) + ).json()["field"]["id"] + + resp = await tool.execute( + action="update_field", + field_id=field_id, + name="renamed", + config={"description": "ok"}, + ) + assert resp["success"] is True + assert resp["field"]["name"] == "renamed" diff --git a/tests/unit/bitable/test_repository.py b/tests/unit/bitable/test_repository.py new file mode 100644 index 0000000..eafe031 --- /dev/null +++ b/tests/unit/bitable/test_repository.py @@ -0,0 +1,113 @@ +"""Tests for BitableRepository — U3/DR-1 + DR-4 fixes. + +DR-1: field_id UUID validation before SQL interpolation (pure unit test). +DR-4: atomic delete_view_if_not_last (requires PostgreSQL). +""" + +from __future__ import annotations + +import pytest + +from agentkit.bitable.repository import _validate_field_id + + +# ── DR-1: field_id UUID validation ────────────────────────── + + +class TestValidateFieldId: + """DR-1: field_id 校验 — 拒绝非 UUID 格式进入 SQL 结构插值。""" + + def test_valid_uuid_passes(self): + valid = "550e8400-e29b-41d4-a716-446655440000" + assert _validate_field_id(valid) == valid + + def test_valid_uppercase_uuid_passes(self): + valid = "550E8400-E29B-41D4-A716-446655440000" + assert _validate_field_id(valid) == valid + + @pytest.mark.parametrize( + "bad_field_id,label", + [ + ("not-a-uuid", "arbitrary string"), + ("", "empty string"), + ("550e8400-e29b-41d4-a716", "too short"), + ("550e8400-e29b-41d4-a716-446655440000-extra", "too long"), + ("550e8400-e29b-41d4-a716-44665544000g", "non-hex char"), + ("'; DROP TABLE bitable.bitable_views; --", "SQL injection attempt"), + ("values->>'field' = :fv0", "SQL fragment"), + ], + ) + def test_invalid_field_id_raises(self, bad_field_id: str, label: str): + with pytest.raises(ValueError, match="Invalid field_id format"): + _validate_field_id(bad_field_id, context=label) + + def test_non_string_raises(self): + with pytest.raises(ValueError, match="Invalid field_id format"): + _validate_field_id(None) # type: ignore[arg-type] + with pytest.raises(ValueError, match="Invalid field_id format"): + _validate_field_id(12345) # type: ignore[arg-type] + + +# ── DR-4: atomic delete_view_if_not_last ──────────────────── +# These tests require PostgreSQL (SELECT FOR UPDATE behavior) + + +pytestmark_postgres = pytest.mark.postgres + + +@pytest.mark.postgres +class TestDeleteViewIfNotLastAtomic: + """DR-4: 原子条件 delete_view — SELECT FOR UPDATE 事务防止 TOCTOU。 + + 并发场景不变量:"table 至少 1 view" 在并发删除最后两个 view 时保持。 + """ + + async def test_delete_view_if_not_last_succeeds_when_multiple_siblings( + self, bitable_service, make_table + ): + """多 view 时删除成功。""" + from agentkit.bitable.repository import BitableRepository + + table = await make_table(name="T1") + v1 = await bitable_service.create_view(table.id, name="v1") + await bitable_service.create_view(table.id, name="v2") + + repo = BitableRepository(bitable_service._repo._db) + deleted = await repo.delete_view_if_not_last(v1.id, table.id) + assert deleted is True + # v1 gone, v2 still present + assert await bitable_service.get_view(v1.id) is None + assert (await bitable_service.list_views(table.id)) is not None + + async def test_delete_view_if_not_last_refuses_last_view(self, bitable_service, make_table): + """最后一个 view 时拒绝删除(返回 False,不抛异常 — service 层负责抛 LastViewDeletionError)。""" + from agentkit.bitable.repository import BitableRepository + + table = await make_table(name="T2") + only_view = await bitable_service.create_view(table.id, name="only") + + repo = BitableRepository(bitable_service._repo._db) + deleted = await repo.delete_view_if_not_last(only_view.id, table.id) + assert deleted is False + # View still exists + assert await bitable_service.get_view(only_view.id) is not None + + async def test_delete_view_if_not_last_wrong_table_returns_false( + self, bitable_service, make_table + ): + """view_id 不属于给定 table_id 时返回 False(不误删其他 table 的 view)。""" + from agentkit.bitable.repository import BitableRepository + + table_a = await make_table(name="TA") + table_b = await make_table(name="TB") + va = await bitable_service.create_view(table_a.id, name="va") + # table_b 有自己的 view + await bitable_service.create_view(table_b.id, name="vb1") + await bitable_service.create_view(table_b.id, name="vb2") + + repo = BitableRepository(bitable_service._repo._db) + # 用 table_b 的 id 但 view_id 是 table_a 的 — 不应删除 + deleted = await repo.delete_view_if_not_last(va.id, table_b.id) + assert deleted is False + # va 仍然存在 + assert await bitable_service.get_view(va.id) is not None diff --git a/tests/unit/bitable/test_service.py b/tests/unit/bitable/test_service.py index 67605a5..4bfffb4 100644 --- a/tests/unit/bitable/test_service.py +++ b/tests/unit/bitable/test_service.py @@ -8,7 +8,7 @@ from __future__ import annotations import pytest from agentkit.bitable.models import FieldOwner, FieldType -from agentkit.bitable.service import FieldDependencyError +from agentkit.bitable.service import FieldDependencyError, LastViewDeletionError pytestmark = pytest.mark.postgres @@ -294,3 +294,44 @@ async def test_list_records_filtered_cursor_pagination(bitable_service) -> None: # All records unique all_ids = {r.id for r in [records, records2, records3] for r in r} assert len(all_ids) == 5 + + +# --------------------------------------------------------------------------- +# DR-4: delete_view last-view protection (service layer) +# --------------------------------------------------------------------------- + + +async def test_delete_view_raises_last_view_deletion_error(bitable_service, make_table) -> None: + """DR-4: service.delete_view raises LastViewDeletionError on the last view. + + Guards the invariant "every table has at least 1 view" at the service + layer so the route can map it to HTTP 409 without touching the repo. + """ + table = await make_table(name="LastView") + only = await bitable_service.create_view(table.id, name="only") + + with pytest.raises(LastViewDeletionError): + await bitable_service.delete_view(only.id) + # View is still there + assert await bitable_service.get_view(only.id) is not None + + +async def test_delete_view_succeeds_when_multiple_views(bitable_service, make_table) -> None: + """DR-4: service.delete_view succeeds when ≥2 views remain (happy path).""" + table = await make_table(name="MultiView") + v1 = await bitable_service.create_view(table.id, name="v1") + await bitable_service.create_view(table.id, name="v2") + + deleted = await bitable_service.delete_view(v1.id) + assert deleted is True + assert await bitable_service.get_view(v1.id) is None + + +async def test_delete_view_returns_false_when_missing(bitable_service, make_table) -> None: + """DR-4: service.delete_view on a non-existent view returns False (no raise).""" + table = await make_table(name="MissingView") + # ensure ≥1 view exists so the table is valid + await bitable_service.create_view(table.id, name="v1") + + deleted = await bitable_service.delete_view("nonexistent-view-id") + assert deleted is False diff --git a/tests/unit/experts/test_team_orchestrator.py b/tests/unit/experts/test_team_orchestrator.py index a21c2c2..6de5b60 100644 --- a/tests/unit/experts/test_team_orchestrator.py +++ b/tests/unit/experts/test_team_orchestrator.py @@ -81,15 +81,17 @@ def _make_mock_expert( } # Mock agent.execute() to return a successful TaskResult mock_agent = MagicMock() - mock_agent.execute = AsyncMock(return_value=TaskResult( - task_id="test", - agent_name=name, - status=TaskStatus.COMPLETED.value, - output_data={"content": f"Result from {name}"}, - error_message=None, - started_at=None, - completed_at=None, - )) + mock_agent.execute = AsyncMock( + return_value=TaskResult( + task_id="test", + agent_name=name, + status=TaskStatus.COMPLETED.value, + output_data={"content": f"Result from {name}"}, + error_message=None, + started_at=None, + completed_at=None, + ) + ) # U3: orchestrator._run_agent_steps() calls agent.execute_stream() (async gen), # not agent.execute(). Mock it to yield one final_answer event. mock_agent.execute_stream = make_execute_stream_mock(f"Result from {name}") @@ -162,20 +164,24 @@ def _make_mock_llm_gateway( def _make_mock_pool() -> MagicMock: """创建 mock AgentPool,模拟上下文隔离的 agent 创建""" pool = MagicMock() + def _create_agent(config): m = MagicMock() - m.execute = AsyncMock(return_value=TaskResult( - task_id="test", - agent_name=config.name, - status=TaskStatus.COMPLETED.value, - output_data={"content": f"Isolated result from {config.name}"}, - error_message=None, - started_at=None, - completed_at=None, - )) + m.execute = AsyncMock( + return_value=TaskResult( + task_id="test", + agent_name=config.name, + status=TaskStatus.COMPLETED.value, + output_data={"content": f"Isolated result from {config.name}"}, + error_message=None, + started_at=None, + completed_at=None, + ) + ) # U3: orchestrator calls agent.execute_stream() (async gen) m.execute_stream = make_execute_stream_mock(f"Isolated result from {config.name}") return m + pool.create_agent = AsyncMock(side_effect=_create_agent) pool.remove_agent = AsyncMock() return pool @@ -220,9 +226,24 @@ class TestPipelineExecution: # LLM 分解为 3 个串行阶段 gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "阶段B", "depends_on": ["A"]}, - {"name": "C", "assigned_expert": "member2", "task_description": "阶段C", "depends_on": ["B"]}, + { + "name": "A", + "assigned_expert": "lead", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "阶段B", + "depends_on": ["A"], + }, + { + "name": "C", + "assigned_expert": "member2", + "task_description": "阶段C", + "depends_on": ["B"], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -231,10 +252,12 @@ class TestPipelineExecution: execution_order: list[str] = [] for name in ["lead", "member1", "member2"]: original_stream = team._experts[name].agent.execute_stream + async def tracking_stream(task_msg, _orig=original_stream, _name=name): execution_order.append(_name) async for ev in _orig(task_msg): yield ev + team._experts[name].agent.execute_stream = tracking_stream result = await orchestrator.execute("串行任务") @@ -256,9 +279,24 @@ class TestPipelineExecution: gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "阶段B", "depends_on": []}, - {"name": "C", "assigned_expert": "member2", "task_description": "阶段C", "depends_on": ["A", "B"]}, + { + "name": "A", + "assigned_expert": "lead", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "阶段B", + "depends_on": [], + }, + { + "name": "C", + "assigned_expert": "member2", + "task_description": "阶段C", + "depends_on": ["A", "B"], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -310,6 +348,59 @@ class TestPipelineExecution: event_types = [c[0][1]["type"] for c in calls] assert "team_synthesis" in event_types + @pytest.mark.asyncio + async def test_synthesis_failure_emits_terminal_team_synthesis_error(self): + """U3/R4-F2: 流式综合异常时发 team_synthesis 终结事件(status=error),避免前端孤儿 milestone 永久转圈。 + + 行为契约: + - 内层 except 捕获 synthesis 异常 → 广播 team_synthesis(status=error, synthesis_id) → re-raise + - 外层 except 捕获 → 调用 _fallback_to_single_agent(不 re-raise) + - 因此 execute() 返回 status="fallback",且最后一个 team_synthesis 事件 status=error。 + """ + team = _make_team_with_experts() + orchestrator = TeamOrchestrator(team) + + # 让 _synthesize_results 抛异常 — 触发 error 终结事件路径 + async def _fail_synthesize(*args, **kwargs): + raise RuntimeError("synthesis boom") + + orchestrator._synthesize_results = _fail_synthesize + + # 不应抛出 — 外层 except 捕获后 fallback + result = await orchestrator.execute("测试任务") + assert result["status"] == "fallback" + + calls = team._handoff_transport.send.call_args_list + synth_events = [c for c in calls if c[0][1].get("type") == "team_synthesis"] + # 必须有终结事件,且 status=error + assert len(synth_events) >= 1 + terminal = synth_events[-1] + assert terminal[0][1].get("status") == "error" + assert terminal[0][1].get("synthesis_id") is not None + assert "synthesis boom" in terminal[0][1].get("error", "") + + @pytest.mark.asyncio + async def test_synthesis_cancelled_emits_terminal_team_synthesis_cancelled(self): + """U3/R4-F2: 流式综合被 CancelledError 中断时发 team_synthesis 终结事件(status=cancelled)。""" + team = _make_team_with_experts() + orchestrator = TeamOrchestrator(team) + + # 让 _synthesize_results 抛 CancelledError — 触发 cancelled 终结事件路径 + async def _cancel_synthesize(*args, **kwargs): + raise asyncio.CancelledError() + + orchestrator._synthesize_results = _cancel_synthesize + + with pytest.raises(asyncio.CancelledError): + await orchestrator.execute("测试任务") + + calls = team._handoff_transport.send.call_args_list + synth_events = [c for c in calls if c[0][1].get("type") == "team_synthesis"] + assert len(synth_events) >= 1 + terminal = synth_events[-1] + assert terminal[0][1].get("status") == "cancelled" + assert terminal[0][1].get("synthesis_id") is not None + # ── 阶段事件广播测试 ────────────────────────────────────── @@ -351,8 +442,18 @@ class TestPhaseEvents: gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "阶段B", "depends_on": ["A"]}, + { + "name": "A", + "assigned_expert": "lead", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "阶段B", + "depends_on": ["A"], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -403,9 +504,24 @@ class TestTaskDecomposition: gateway = _make_mock_llm_gateway( phases=[ - {"name": "规划", "assigned_expert": "lead", "task_description": "设计架构", "depends_on": []}, - {"name": "前端", "assigned_expert": "member1", "task_description": "实现UI", "depends_on": ["规划"]}, - {"name": "后端", "assigned_expert": "member2", "task_description": "实现API", "depends_on": ["规划"]}, + { + "name": "规划", + "assigned_expert": "lead", + "task_description": "设计架构", + "depends_on": [], + }, + { + "name": "前端", + "assigned_expert": "member1", + "task_description": "实现UI", + "depends_on": ["规划"], + }, + { + "name": "后端", + "assigned_expert": "member2", + "task_description": "实现API", + "depends_on": ["规划"], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -456,10 +572,22 @@ class TestTaskDecomposition: @pytest.mark.asyncio async def test_parse_phases_valid_json(self): """_parse_phases 正确解析 JSON 数组""" - content = json.dumps([ - {"name": "A", "assigned_expert": "member1", "task_description": "任务A", "depends_on": []}, - {"name": "B", "assigned_expert": "member2", "task_description": "任务B", "depends_on": ["A"]}, - ]) + content = json.dumps( + [ + { + "name": "A", + "assigned_expert": "member1", + "task_description": "任务A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member2", + "task_description": "任务B", + "depends_on": ["A"], + }, + ] + ) phases = TeamOrchestrator._parse_phases(content, ["member1", "member2"], "lead") assert len(phases) == 2 assert phases[0].name == "A" @@ -473,9 +601,16 @@ class TestTaskDecomposition: @pytest.mark.asyncio async def test_parse_phases_invalid_expert_falls_back_to_lead(self): """_parse_phases 对无效专家名回退到 lead""" - content = json.dumps([ - {"name": "A", "assigned_expert": "nonexistent", "task_description": "任务A", "depends_on": []}, - ]) + content = json.dumps( + [ + { + "name": "A", + "assigned_expert": "nonexistent", + "task_description": "任务A", + "depends_on": [], + }, + ] + ) phases = TeamOrchestrator._parse_phases(content, ["member1"], "lead") assert len(phases) == 1 assert phases[0].assigned_expert == "lead" @@ -489,10 +624,22 @@ class TestTaskDecomposition: @pytest.mark.asyncio async def test_parse_phases_empty_name_skipped(self): """_parse_phases 跳过空名称的阶段""" - content = json.dumps([ - {"name": "", "assigned_expert": "member1", "task_description": "任务A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "任务B", "depends_on": []}, - ]) + content = json.dumps( + [ + { + "name": "", + "assigned_expert": "member1", + "task_description": "任务A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "任务B", + "depends_on": [], + }, + ] + ) phases = TeamOrchestrator._parse_phases(content, ["member1"], "lead") assert len(phases) == 1 assert phases[0].name == "B" @@ -500,10 +647,22 @@ class TestTaskDecomposition: @pytest.mark.asyncio async def test_parse_phases_resolves_depends_on_by_name(self): """_parse_phases 通过名称解析 depends_on 为 ID""" - content = json.dumps([ - {"name": "规划", "assigned_expert": "lead", "task_description": "设计", "depends_on": []}, - {"name": "实现", "assigned_expert": "member1", "task_description": "编码", "depends_on": ["规划"]}, - ]) + content = json.dumps( + [ + { + "name": "规划", + "assigned_expert": "lead", + "task_description": "设计", + "depends_on": [], + }, + { + "name": "实现", + "assigned_expert": "member1", + "task_description": "编码", + "depends_on": ["规划"], + }, + ] + ) phases = TeamOrchestrator._parse_phases(content, ["lead", "member1"], "lead") assert len(phases) == 2 # 实现 depends on 规划 @@ -512,9 +671,16 @@ class TestTaskDecomposition: @pytest.mark.asyncio async def test_parse_phases_unknown_dependency_ignored(self): """_parse_phases 对未知依赖名称忽略""" - content = json.dumps([ - {"name": "A", "assigned_expert": "lead", "task_description": "任务A", "depends_on": ["unknown_phase"]}, - ]) + content = json.dumps( + [ + { + "name": "A", + "assigned_expert": "lead", + "task_description": "任务A", + "depends_on": ["unknown_phase"], + }, + ] + ) phases = TeamOrchestrator._parse_phases(content, ["lead"], "lead") assert len(phases) == 1 assert len(phases[0].depends_on) == 0 # unknown dependency ignored @@ -558,7 +724,12 @@ class TestPhaseExecution: gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "nonexistent", "task_description": "任务A", "depends_on": []}, + { + "name": "A", + "assigned_expert": "nonexistent", + "task_description": "任务A", + "depends_on": [], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -578,8 +749,18 @@ class TestPhaseExecution: gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "阶段B", "depends_on": ["A"]}, + { + "name": "A", + "assigned_expert": "lead", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "阶段B", + "depends_on": ["A"], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -639,9 +820,24 @@ class TestPhaseFailure: gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "阶段B", "depends_on": ["A"]}, - {"name": "C", "assigned_expert": "member2", "task_description": "阶段C", "depends_on": ["B"]}, + { + "name": "A", + "assigned_expert": "lead", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "阶段B", + "depends_on": ["A"], + }, + { + "name": "C", + "assigned_expert": "member2", + "task_description": "阶段C", + "depends_on": ["B"], + }, ] ) team._experts["lead"].agent._llm_gateway = gateway @@ -787,8 +983,18 @@ class TestResultSynthesis: gateway = _make_mock_llm_gateway( phases=[ - {"name": "A", "assigned_expert": "member1", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member2", "task_description": "阶段B", "depends_on": []}, + { + "name": "A", + "assigned_expert": "member1", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member2", + "task_description": "阶段B", + "depends_on": [], + }, ], synthesis_content="综合结果", ) @@ -810,10 +1016,22 @@ class TestResultSynthesis: gateway = AsyncMock() decomp_response = MagicMock() - decomp_response.content = json.dumps([ - {"name": "A", "assigned_expert": "member1", "task_description": "阶段A", "depends_on": []}, - {"name": "B", "assigned_expert": "member2", "task_description": "阶段B", "depends_on": []}, - ]) + decomp_response.content = json.dumps( + [ + { + "name": "A", + "assigned_expert": "member1", + "task_description": "阶段A", + "depends_on": [], + }, + { + "name": "B", + "assigned_expert": "member2", + "task_description": "阶段B", + "depends_on": [], + }, + ] + ) # ponytail: 函数式 side_effect — 首次返回 decomposition,后续一律抛 RuntimeError # (列表式 side_effect 耗尽会抛 StopIteration,被 U3 收窄后的 except 漏捕获; # 函数式让"LLM 不可用"语义明确,覆盖验收+综合所有后续调用) @@ -826,12 +1044,14 @@ class TestResultSynthesis: raise RuntimeError("LLM unavailable") gateway.chat = AsyncMock(side_effect=chat_side_effect) + # U3: synthesizer tries chat_stream first (broadcast_callback is set). # Make it raise so synthesizer catches and falls back to concatenation, # matching the test intent ("without LLM"). async def _stream_raise(*a, **kw): raise RuntimeError("LLM unavailable") yield # async gen marker + gateway.chat_stream = MagicMock(side_effect=_stream_raise) team._experts["lead"].agent._llm_gateway = gateway @@ -1049,11 +1269,13 @@ class TestConcurrencyLimit: team = _make_team_with_experts(expert_names=["lead", "m1", "m2"]) orchestrator = TeamOrchestrator(team) - gateway = _make_mock_llm_gateway(phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, - {"name": "B", "assigned_expert": "m1", "task_description": "B", "depends_on": []}, - {"name": "C", "assigned_expert": "m2", "task_description": "C", "depends_on": []}, - ]) + gateway = _make_mock_llm_gateway( + phases=[ + {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, + {"name": "B", "assigned_expert": "m1", "task_description": "B", "depends_on": []}, + {"name": "C", "assigned_expert": "m2", "task_description": "C", "depends_on": []}, + ] + ) team._experts["lead"].agent._llm_gateway = gateway tracker = _ConcurrencyTracker() @@ -1082,10 +1304,17 @@ class TestConcurrencyLimit: team = _make_team_with_experts(expert_names=names) orchestrator = TeamOrchestrator(team) - gateway = _make_mock_llm_gateway(phases=[ - {"name": f"P{i}", "assigned_expert": name, "task_description": f"P{i}", "depends_on": []} - for i, name in enumerate(names) - ]) + gateway = _make_mock_llm_gateway( + phases=[ + { + "name": f"P{i}", + "assigned_expert": name, + "task_description": f"P{i}", + "depends_on": [], + } + for i, name in enumerate(names) + ] + ) team._experts["lead"].agent._llm_gateway = gateway tracker = _ConcurrencyTracker() @@ -1113,11 +1342,13 @@ class TestConcurrencyLimit: team = _make_team_with_experts(expert_names=["lead", "m1", "m2"]) orchestrator = TeamOrchestrator(team, max_concurrent_phases=1) - gateway = _make_mock_llm_gateway(phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, - {"name": "B", "assigned_expert": "m1", "task_description": "B", "depends_on": []}, - {"name": "C", "assigned_expert": "m2", "task_description": "C", "depends_on": []}, - ]) + gateway = _make_mock_llm_gateway( + phases=[ + {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, + {"name": "B", "assigned_expert": "m1", "task_description": "B", "depends_on": []}, + {"name": "C", "assigned_expert": "m2", "task_description": "C", "depends_on": []}, + ] + ) team._experts["lead"].agent._llm_gateway = gateway tracker = _ConcurrencyTracker() @@ -1144,11 +1375,13 @@ class TestConcurrencyLimit: team = _make_team_with_experts(expert_names=["lead", "m1", "m2"]) orchestrator = TeamOrchestrator(team) - gateway = _make_mock_llm_gateway(phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, - {"name": "B", "assigned_expert": "m1", "task_description": "B", "depends_on": []}, - {"name": "C", "assigned_expert": "m2", "task_description": "C", "depends_on": []}, - ]) + gateway = _make_mock_llm_gateway( + phases=[ + {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, + {"name": "B", "assigned_expert": "m1", "task_description": "B", "depends_on": []}, + {"name": "C", "assigned_expert": "m2", "task_description": "C", "depends_on": []}, + ] + ) team._experts["lead"].agent._llm_gateway = gateway tracker = _ConcurrencyTracker() @@ -1210,11 +1443,13 @@ class TestConcurrencyLimit: orchestrator._generate_debate_opening = AsyncMock(return_value="Opening statement") orchestrator._generate_debate_argument = tracking_debate_argument orchestrator._generate_debate_summary = AsyncMock(return_value="Round summary") - orchestrator._generate_debate_verdict = AsyncMock(return_value={ - "conclusion": "Final conclusion", - "decision": "adopt", - "rationale": "Because", - }) + orchestrator._generate_debate_verdict = AsyncMock( + return_value={ + "conclusion": "Final conclusion", + "decision": "adopt", + "rationale": "Because", + } + ) await orchestrator._execute_debate_phase(phase, plan) @@ -1345,10 +1580,17 @@ class TestSharedWorkspaceRedis: team = _make_team_with_experts() orchestrator = TeamOrchestrator(team) - gateway = _make_mock_llm_gateway(phases=[ - {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, - {"name": "B", "assigned_expert": "member1", "task_description": "B", "depends_on": ["A"]}, - ]) + gateway = _make_mock_llm_gateway( + phases=[ + {"name": "A", "assigned_expert": "lead", "task_description": "A", "depends_on": []}, + { + "name": "B", + "assigned_expert": "member1", + "task_description": "B", + "depends_on": ["A"], + }, + ] + ) team._experts["lead"].agent._llm_gateway = gateway # Mock _execute_phase to return large content + verify offloading