From d936434fc315582344c635a704a1ec9e92fc5511 Mon Sep 17 00:00:00 2001 From: limit_yan Date: Mon, 13 Jul 2026 01:56:25 +0800 Subject: [PATCH] refactor(review): make strict delegation on demand --- docs/architecture/deep-review.md | 140 ++--- .../agent-workflow-staged-plan.md | 10 +- .../architecture/agent-workflow-design.md | 14 +- docs/sdlc-harness/design.md | 4 +- ...-requirements-agent-workflow-adjustment.md | 24 +- docs/sdlc-harness/traceability-matrix.md | 2 +- .../agents/definitions/hidden/deep_review.rs | 4 +- .../agents/prompts/deep_review_agent.md | 315 ++--------- .../task/launch_review_agent.rs | 8 +- .../agent-runtime/src/deep_review/budget.rs | 48 +- .../src/deep_review/execution_policy.rs | 106 +++- .../agent-runtime/src/deep_review/manifest.rs | 70 +-- .../DeepReviewConsentDialog.test.tsx | 10 +- .../components/DeepReviewConsentDialog.tsx | 6 +- src/web-ui/src/locales/en-US/flow-chat.json | 6 +- src/web-ui/src/locales/zh-CN/flow-chat.json | 6 +- src/web-ui/src/locales/zh-TW/flow-chat.json | 6 +- .../shared/services/review-team/defaults.ts | 19 +- .../src/shared/services/review-team/index.ts | 36 +- .../services/review-team/promptBlock.ts | 67 ++- .../services/review-team/tokenBudget.ts | 3 +- .../src/shared/services/review-team/types.ts | 2 + .../services/review-team/workPackets.ts | 289 +--------- .../shared/services/reviewTeamService.test.ts | 528 ++++-------------- 24 files changed, 472 insertions(+), 1251 deletions(-) diff --git a/docs/architecture/deep-review.md b/docs/architecture/deep-review.md index 95e8977bbe..2fdc227dc6 100644 --- a/docs/architecture/deep-review.md +++ b/docs/architecture/deep-review.md @@ -2,7 +2,7 @@ ## Scope -DeepReview is the compatibility runtime for `Review: Strict`, the highest-strength mode of the unified Review experience. It remains implemented as a child-session workflow that runs a configurable read-only reviewer set against a review target, but it should not be presented as a second ordinary product entry next to Review. +DeepReview is the compatibility runtime for `Review: Strict`, the highest-strength mode of the unified Review experience. It remains a read-only child session, but the child is now the primary strict reviewer rather than a dispatcher for a fixed reviewer committee. It should not be presented as a second ordinary product entry next to Review. Product-facing guardrails are summarized here: @@ -25,7 +25,7 @@ The backend does not resolve the review target or build the launch manifest. The ## Runtime Roles -`CodeReview` and the `DeepReview` orchestrator are read-only adversarial review identities. `CodeReview` handles ordinary Review as one isolated child and cannot silently expand into multiple reviewers. `DeepReview` is reserved for an explicit strict request, can launch only manifest-approved reviewers, inspect repository evidence, and submit the consolidated report; it has no edit, command, Git-mutation, or remediation tools. +`CodeReview` and `DeepReview` are read-only adversarial review identities. `CodeReview` handles ordinary Review as one isolated child and cannot silently expand into multiple reviewers. `DeepReview` is reserved for an explicit strict request, reviews the prepared target directly, may ask one manifest-approved specialist for a focused second perspective, and submits the report; it has no edit, command, Git-mutation, or remediation tools. `src/crates/assembly/core/src/agentic/agents/definitions/review/review_specialists.rs` defines read-only reviewer agents: @@ -36,7 +36,7 @@ The backend does not resolve the review target or build the launch manifest. The - `ReviewFrontend` - `ReviewJudge` -The reviewer agents use instruction-only context and read/search/diff tools. The existing generic Git exposure remains for legacy compatibility, but it is not authorized as prepared changed-code evidence. Prepared `GetFileDiff` is the source of truth for changed code; when the local binding is `matching_clean`, existing Read/Grep/Glob/LS tools may supplement it with repository context. `ReviewFrontend` is a conditional role. `ReviewJudge` validates reviewer evidence and consistency instead of performing a full independent review pass. +These agents form an optional specialist pool, not mandatory coverage lanes. A new strict run may launch at most one specialist for a concrete uncertainty. The existing generic Git exposure remains for legacy compatibility, but it is not authorized as prepared changed-code evidence. Prepared `GetFileDiff` is the source of truth for changed code; when the local binding is `matching_clean`, existing Read/Grep/Glob/LS tools may supplement it with repository context. `ReviewJudge` is a conditional quality check used only for a high-severity finding, conflicting evidence, or a materially low-confidence conclusion; it does not perform a full independent review pass. `ReviewFixer` is the separate writable remediation identity. DeepReview runtime policy rejects it during review execution. The frontend action surface invokes it only after user approval, and a new read-only Review run checks the fix when requested. @@ -79,123 +79,59 @@ Review target evidence is session-scoped and covers current workspace changes, a An explicit, complete Git range with a matching clean workspace or a provider PR with immutable base/head and complete per-file diff availability may report `complete`. Workspace evidence remains `limited` because it is mutable, even when its prepared diff coverage is complete. Limited or stale evidence does not rewrite the model's risk or recommendation; the report and UI display reliability separately. Invalid evidence fails closed, while historical manifests with no target evidence keep legacy behavior. -Prepared Review work packets use bounded `GetFileDiff` pages as changed-code evidence. Local ranges read exact Git revisions; PR targets read provider diffs on demand and revalidate base/head before each file. The parent Review has a 240,000-character aggregate allowance and admits at most 128 provider diff acquisitions before provider I/O; one acquisition normally performs one file-page request and one detail request. The fixed cap bounds repeated multi-reviewer acquisition without adding configuration or changing local ranges and ordinary Agent tool use. Repeating the same page for the same reviewer returns a compact already-served result instead of the diff again. Exhaustion and stale target bindings return structured limited evidence. Existing generic Git exposure remains for legacy compatibility but does not authorize ref guessing or scope widening; Read/Grep/Glob/LS are supplemental only for a matching clean Git-range binding, never for a provider-only PR target. +Prepared Review target evidence uses bounded `GetFileDiff` pages as changed-code evidence. Local ranges read exact Git revisions; PR targets read provider diffs on demand and revalidate base/head before each file. The parent Review has a 240,000-character aggregate allowance and admits at most 128 provider diff acquisitions before provider I/O; one acquisition normally performs one file-page request and one detail request. Repeating the same page for the same reviewer returns a compact already-served result instead of the diff again. Exhaustion and stale target bindings return structured limited evidence. Existing generic Git exposure remains for legacy compatibility but does not authorize ref guessing or scope widening; Read/Grep/Glob/LS are supplemental only for a matching clean Git-range binding, never for a provider-only PR target. Deleted, renamed, binary, oversized, conflicted, or unavailable files remain visible as coverage facts. The PR panel is the only built-in PR Review entry and associates progress/results by provider repository, PR id, and immutable revisions. Cached overview data is display-only until the selected PR is revalidated; revision or runtime-evidence changes make prior results stale, and failed or unavailable results remain distinct from limited coverage. The implementation does not add automatic checkout, reviewer command execution, speculative cache plans, automatic Review, inline comments, approval, merge, or automatic publishing. -## Strict Reviewer Configuration +## Strict Review Delegation Policy -The default strict reviewer configuration contract is mirrored in Rust and TypeScript. +The default strict-review contract is mirrored in Rust and TypeScript. New strict launches use the following fixed boundary: -Rust source: +- the `DeepReview` child performs the primary full review itself; +- applicable core and explicitly configured extra reviewers form an allowed specialist pool; +- at most one specialist may be launched for a concrete unresolved question; +- `ReviewJudge` is available only as a conditional quality check; +- automatic file splitting, same-role fan-out, and reviewer retry are disabled; +- the run uses one primary review-agent execution, with at most one specialist execution and one quality-inspector execution. -- `src/crates/assembly/core/src/agentic/deep_review/team_definition.rs` -- `src/crates/assembly/core/src/agentic/deep_review_policy.rs` -- `src/apps/desktop/src/api/agentic_api.rs` +The runtime enforces the one-specialist budget even if a weak model ignores the prompt. This is a resource ceiling, not a keyword or risk-score workflow rule. The model decides whether delegation is useful from the actual evidence and task, while the manifest limits which read-only agents it may call. -Frontend source: - -- `src/web-ui/src/shared/services/review-team/defaults.ts` -- `src/web-ui/src/shared/services/review-team/types.ts` -- `src/web-ui/src/shared/services/review-team/index.ts` - -The desktop command `get_default_review_team_definition` returns the backend default definition. The frontend normalizes that response and falls back to its TypeScript default if the command is unavailable. - -The persisted config path is `ai.review_teams.default`. The frontend config shape includes: - -- extra subagent ids -- review strategy level -- per-reviewer strategy overrides -- reviewer and judge timeouts -- reviewer file-split threshold -- max same-role instances -- max retries per role -- max parallel reviewers -- max queue wait seconds -- provider capacity queue enablement -- bounded auto-retry enablement and elapsed guard - -Extra reviewers must be enabled subagents with read-only review tooling. Core reviewers, `DeepReview`, and `ReviewFixer` are disallowed as extra reviewers. +Historical configuration fields for reviewer timeouts, file-split thresholds, same-role instances, retries, concurrency, and queue behavior remain readable so stored sessions can recover honestly. New strict manifests override split, same-role, retry, and specialist-call values to the bounded policy above. Extra reviewers must still be enabled subagents with read-only review tooling. `DeepReview` and `ReviewFixer` remain disallowed. ## Manifest Shape -`buildEffectiveReviewTeamManifest` in `src/web-ui/src/shared/services/review-team/index.ts` builds the launch manifest. The manifest has `reviewMode: 'deep'` and may include: - -- workspace path -- policy source -- target classification -- final strategy level -- scope profile -- frontend and backend strategy recommendations -- strategy decision -- execution policy -- concurrency policy -- change stats -- pre-review summary -- evidence pack -- token-budget plan -- active core reviewers -- quality-gate reviewer -- enabled extra reviewers -- skipped reviewers -- work packets - -The target classifier drives conditional reviewer selection. `ReviewFrontend` is included only when the target matches frontend-oriented files. - -The evidence pack is metadata-only. It lists changed file paths, aggregate diff stats, domain/risk tags, packet ids, hunk hints, contract hints, budget counts, and workspace/Git-range target facts. It explicitly excludes embedded source text, full diff text, model output, duplicated manifest JSON, provider raw bodies, synthetic diff references, speculative cache plans, and full file contents. - -## Strategies and Scope +`buildEffectiveReviewTeamManifest` in `src/web-ui/src/shared/services/review-team/index.ts` builds the launch manifest. The manifest keeps `reviewMode: 'deep'`, resolved target evidence, strategy/scope metadata, execution policy, specialist pool, optional quality-inspector identity, skipped members, and token/call budget facts. -Strategy profiles are internal DeepReview configuration. They do not select or upgrade ordinary Review. The frontend maps an explicit strict request to the deep profile in `src/web-ui/src/shared/services/review-team/strategy.ts` and `scopeProfile.ts`. +For new strict launches: -Supported strategy levels are `quick`, `normal`, and `deep`. +- `coreReviewers` and `enabledExtraReviewers` describe agents the primary reviewer may choose from; they are not scheduled calls; +- `qualityGateReviewer` identifies the available conditional inspector and does not require it to run; +- `workPackets` is empty; +- `executionPolicy.maxReviewerCalls` is `1`; +- file splitting and retries are disabled; +- the launch preview reports one planned primary review-agent execution and a maximum of three review-agent executions; it does not claim a bound on underlying model requests. -- `quick` uses high-risk-only scope, zero dependency hops, risk-matched optional reviewers, and no broad tool exploration. -- `normal` uses risk-expanded scope, one dependency hop, configured optional reviewers, and no broad tool exploration. -- `deep` uses full-depth scope, policy-limited dependency context, full optional reviewer policy, and broad tool exploration. +The evidence pack remains metadata-only. It lists changed file paths, aggregate diff stats, domain/risk tags, hunk hints, contract hints, budget counts, and workspace/Git-range target facts. It excludes embedded source text, full diff text, model output, provider raw bodies, speculative cache plans, and full file contents. -New strict launches carry only the minimal `qualityDecision: { level: 'l3' }` runtime marker. The portable runtime requires the deep strategy, every non-conditional core reviewer, `ReviewJudge` as the quality gate, and each conditional core reviewer to be active or explicitly `not_applicable`. Historical L2 manifests remain readable and continue to receive their legacy structural validation; manifests without `qualityDecision` retain older-session compatibility. The backend also parses the selected strategy from the manifest/config and uses it for runtime guardrails such as timeouts, policy classification, and retry limits. Frontend/backend recommendation fields remain report metadata and never upgrade an ordinary Review. +## Strategies and Scope -Launch consent shows the exact planned independent checks, maximum parallel calls, runtime tendency, and read-only boundary. It does not display a heuristic token estimate. +Ordinary Review remains one `CodeReview` child. A new explicit strict request always selects the deep profile, but “deep” now means deeper evidence inspection by the primary reviewer, not maximum fan-out. Security, performance, architecture, frontend, and test concerns are investigation dimensions for that model. -## Work Packets +`quick` and `normal` strategy values, legacy work packets, and older L2 manifests remain readable for stored-session recovery. They do not create new production Review launches. New L3 validation requires the deep strategy but no longer requires every core reviewer or a Judge call. If a quality-gate member is present, it must be `ReviewJudge`. -`src/web-ui/src/shared/services/review-team/workPackets.ts` creates pure launch-plan metadata. Work packets do not inspect file contents and do not make runtime retry or queue decisions. +Launch consent shows the exact target, one planned primary review-agent execution, the maximum bounded review-agent execution count, runtime tendency, and read-only boundary. It does not estimate underlying model requests or tokens. -Each work packet includes: +## Historical Work Packet Compatibility -- packet id -- phase (`reviewer` or `judge`) -- launch batch -- subagent id and labels -- assigned scope -- allowed tools -- timeout seconds -- required output fields -- strategy level and directive -- model slot +New strict reviews do not generate work packets or module-aware reviewer shards. Stored manifests may still contain reviewer/judge packets, launch batches, packet ids, assigned scopes, and retry metadata. Runtime parsing, report enrichment, recovery UI, and target-evidence validation continue to read those fields so historical sessions are not rewritten as complete or successful. -If the included file count exceeds the reviewer file-split threshold and same-role instances are allowed, reviewer scopes are split into module-aware groups. Reviewer packets are then assigned launch batches using the concurrency policy. The judge packet, when present, runs in the batch after the final reviewer batch. +Compatibility code must not turn historical packet support back into a new-launch requirement. Packet-specific queue and retry behavior applies only when an existing manifest actually contains those packets. ## Backend Policy and Admission -`DeepReviewExecutionPolicy` in `src/crates/assembly/core/src/agentic/deep_review/execution_policy.rs` parses runtime policy from config and classifies subagent launches. - -Allowed DeepReview runtime launches are: - -- core reviewer roles -- conditional reviewer roles when active in the manifest -- configured extra reviewer roles -- `ReviewJudge` - -Rejected launches include: - -- `ReviewFixer` during review execution -- nested `DeepReview` -- any subagent not configured for the review team -- subagents skipped or absent from the run manifest +`DeepReviewExecutionPolicy` in `src/crates/execution/agent-runtime/src/deep_review/execution_policy.rs` parses runtime policy and the new per-turn specialist-call ceiling. `DeepReviewRunManifestGate` admits only specialist-pool members and the optional `ReviewJudge`, rejects `ReviewFixer`, nested `DeepReview`, skipped members, and unconfigured agents, and preserves legacy manifest parsing and membership validation. -`DeepReviewRunManifestGate` in `manifest.rs` reads active subagent ids from `workPackets`, `coreReviewers`, `enabledExtraReviewers`, and `qualityGateReviewer`. It also records skipped reviewer reasons so policy failures can explain why a reviewer is inactive. +`DeepReviewBudgetTracker` separately permits at most one initial specialist and one Judge call for a new strict turn. This keeps the safety boundary deterministic without hard-coding which domain deserves delegation. ## Task Execution and Queue State @@ -208,11 +144,11 @@ The generic `Task` tool is adapted for DeepReview in: DeepReview task execution uses the manifest and tool context to: -- identify reviewer role and packet id +- identify an optional specialist or quality-inspector role and any historical packet id - read historical incremental-cache metadata when present, without creating cache plans for new runs -- enforce policy and retry coverage -- cap active reviewers -- preserve launch-batch ordering +- enforce the new specialist-call ceiling and historical retry coverage +- cap active optional reviewers +- preserve launch-batch ordering only for historical packet manifests - wait for transient capacity when allowed - emit queue state events - record runtime diagnostics and capacity skips @@ -316,8 +252,8 @@ The review action bar persists UI state separately through `ReviewActionBarPersi - Project integration adapters own raw workspace/Git target acquisition. The artifact/evidence layer owns the fixed session target manifest and its completeness. Mutable workspace targets may have complete prepared diff coverage, but their final evidence status remains `limited`. Reviewers may not mutate or silently widen that target. - The backend owns policy validation, runtime admission, queue/retry state, event emission, and report enrichment. - Reviewer subagents and review orchestrators stay read-only. Remediation runs under `ReviewFixer` after user approval, not during the reviewer pass. -- Work packets and evidence packs are planning metadata; they must not embed file contents or full diffs. -- Existing reviewer Git exposure remains unchanged for legacy compatibility, but prepared work packets do not authorize it as changed-code evidence and no dedicated multi-operation Git tool is added. Prepared `GetFileDiff` must be bounded and disable external diff/text conversion; live repository reads are supplemental and require a deterministic clean local binding. +- Historical work packets and current evidence packs are metadata only; they must not embed file contents or full diffs. +- Existing reviewer Git exposure remains unchanged for legacy compatibility, but prepared target evidence does not authorize it as changed-code evidence and no dedicated multi-operation Git tool is added. Prepared `GetFileDiff` must be bounded and disable external diff/text conversion; live repository reads are supplemental and require a deterministic clean local binding. ## Change Checklist diff --git a/docs/sdlc-harness/agent-workflow-staged-plan.md b/docs/sdlc-harness/agent-workflow-staged-plan.md index c4191bf138..7680b4d05e 100644 --- a/docs/sdlc-harness/agent-workflow-staged-plan.md +++ b/docs/sdlc-harness/agent-workflow-staged-plan.md @@ -61,17 +61,17 @@ BitFun 不需要把 dynamic workflow 做成一个新的主产品模式。用户 | 风险信号 | 安全、性能、架构、跨模块、关键 UI 流程或验证缺口只用于指导同一个 reviewer 的关注点，不自动扩展 reviewer 数量 | | 严格审查条件 | 当前由 `/review strict`、历史 `/DeepReview` alias 或内部显式 strict follow-up 启动；大型 PR、风险标签和团队策略本身不自动触发 | | GUI | 一个 Review 面板，按问题优先级合并输出 | -| 成本 | 显式严格审查前显示范围、计划 reviewer/调用数、并发或耗时倾向和只读边界；不展示无法证明的 Token 估算，也不声称提供尚未实现的范围调整 | +| 成本 | 显式严格审查前显示范围、一次计划主审、最多三次审查代理执行的硬上限、耗时倾向和只读边界；不估算底层模型请求或 Token，也不声称提供尚未实现的范围调整 | | 完成标准 | 必须修复、建议确认、已覆盖、未覆盖、下一步清楚 | | 禁止 | 把 PR 审查压进 P0 默认体验，或把 DeepReview 作为普通 review 默认入口 | 当前产品落点： - 文件变更菜单和命令面板只提供 `Review`，不让用户先选“普通/严格”。 -- `/review` 始终启动一个只读 reviewer，并由模型根据目标证据决定检查深度；`/review strict` 表达用户明确要求多 reviewer 严格覆盖，`/DeepReview` 仅保留历史兼容。 +- `/review` 始终启动一个只读 reviewer，并由模型根据目标证据决定检查深度；`/review strict` 表达用户明确要求严格主审，但不自动增加 reviewer。主审仅在具体不确定性确实需要独立视角时调用一个专家，`/DeepReview` 仅保留历史兼容。 - 目标证据先于 Review 决策：当前工作区使用一次有界 `HEAD -> worktree` 取证，但没有 immutable snapshot，因此最终 evidence status 始终为 `limited`；显式 Git range 由目标准备层固定 base/head，完整且无遗漏、workspace binding 为 matching_clean 时 evidence status 才可为 `complete`。Reviewer 不自行猜 ref，缺失、截断或预算耗尽必须进入覆盖说明，但不改写模型 recommendation。 -- PR1 保留只读 Reviewer 现有的 `Git` 暴露以避免旧 PR/历史诊断场景先发生能力回退，但 prepared work packet 不把它作为 changed-code 证据，也不新增多操作 Git 工具。prepared target 只通过有界 `GetFileDiff` 消费目标 diff；只有本地仓库与目标 head 匹配且整个工作区干净时，现有 Read/Grep/Glob/LS 才补充 live context。不做逐工具全仓重验、任意 shell、fetch、checkout 或仓库状态写入。 -- 显式严格审查扩大独立覆盖前展示单一 Review 方案确认，包括范围、计划 reviewer/调用数、耗时倾向和只读边界；不展示无法证明的 token 估算。 +- PR1 保留只读 Reviewer 现有的 `Git` 暴露以避免旧 PR/历史诊断场景先发生能力回退，但 prepared target evidence 不把它作为 changed-code 证据，也不新增多操作 Git 工具。prepared target 只通过有界 `GetFileDiff` 消费目标 diff；只有本地仓库与目标 head 匹配且整个工作区干净时，现有 Read/Grep/Glob/LS 才补充 live context。不做逐工具全仓重验、任意 shell、fetch、checkout 或仓库状态写入。 +- 显式严格审查前展示单一 Review 方案确认，包括范围、一次计划主审、最多三次审查代理执行的硬上限、耗时倾向和只读边界；不估算底层模型请求或 token。 - 修复不新开第二套产品界面：同一 Review 侧栏先把选中项交给 `ReviewFixer`；“审核修复”能精确归因时按原审核文件与 Fixer 直接改动文件的并集重新统计和决策，命令型修改无法可靠归因时明确提示并回退当前工作区 diff，在同一侧栏位置切换到新的隔离 Review 子会话。Fixer 基线和选中项在修复前持久化；follow-up 预留、relationship metadata 和后端 session id 复用同一 request id。启动确认不确定时保留稳定 turn 和已创建子会话、显示明确提示，不自动或在重启后重发启动消息；侧栏显示进行中和查看结果，只有明确失败的操作才提供重试。旧会话缺少范围信息时采用同样的显式工作区回退。 - PR 面板是唯一内置 PR Review 入口：adapter 固定 provider identity、base/head 和按需远程 diff，启动同一套单 reviewer Review，并按精确 PR revision 投影进行中、结果和过期状态。已移除 PR Review MiniApp 及其独立强度和 AI 草稿路径；平台 CI/审批/mergeability 事实仍与 AI Review 建议分层。本轮不自动触发 Review，不生成或发布 inline comment，也不增加 approve、merge、checkout、缓存或 Finding 生命周期。 @@ -187,7 +187,7 @@ P3/P4 再按需补充队列阻塞率、工作流回退率、后续返工率和 | [product-requirements.md](product-requirements.md) | 继承快速开发、上下文保障、团队治理、执行安全和合规/发布路径 | | [implementation-plan.md](implementation-plan.md) | 复用既有阶段路线，不新增 workflow 专属 P0-P4 | | [architecture/agent-workflow-design.md](architecture/agent-workflow-design.md) | 仅作为交互和边界补充，不定义独立核心架构 | -| [../architecture/deep-review.md](../architecture/deep-review.md) | 仅在 L3 严格审查中复用 DeepReview 的 reviewer 队列、只读 reviewer 和 action surface；通用任务生命周期、scheduler 和队列状态归 Agent Kernel，Harness 只通过 provider/plan/step 参与编排 | +| [../architecture/deep-review.md](../architecture/deep-review.md) | 仅在 L3 严格审查中复用 DeepReview 主审、按需专家、历史队列兼容和 action surface；通用任务生命周期、scheduler 和队列状态归 Agent Kernel，Harness 只通过 provider/plan/step 参与编排 | | [architecture/quality-data-plane.md](architecture/quality-data-plane.md) | 复用既有最小事件和指标口径，不新增 P0 默认事件 | ## 10. 参考资料 diff --git a/docs/sdlc-harness/architecture/agent-workflow-design.md b/docs/sdlc-harness/architecture/agent-workflow-design.md index 863a486550..0bed41cb87 100644 --- a/docs/sdlc-harness/architecture/agent-workflow-design.md +++ b/docs/sdlc-harness/architecture/agent-workflow-design.md @@ -1,7 +1,7 @@ # BitFun 智能体工作流交互与边界补充设计 > 范围：为 [../agent-workflow-staged-plan.md](../agent-workflow-staged-plan.md) 中的场景提供交互和边界补充。 -> 本文不定义新的 Agent Kernel、Harness、QDP 或 DeepReview 核心对象模型；实现时优先复用既有 session、task、Agent Kernel/Harness long-running queue、DeepReview manifest、work packets、runtime events 和质量数据面契约。 +> 本文不定义新的 Agent Kernel、Harness、QDP 或 DeepReview 核心对象模型；实现时优先复用既有 session、task、Agent Kernel/Harness long-running queue、DeepReview manifest、runtime events 和质量数据面契约。历史 work packets 仅用于旧会话兼容。 ## 1. 设计定位 @@ -16,7 +16,7 @@ - 不定义新的 workflow DSL。 - 不定义新的 P0/P1/P2/P3/P4。 - 不定义新的持久化实体作为 P0/P1 前置。 -- 不把多 reviewer 严格审查或任务控制台放入默认路径。 +- 不把固定多 reviewer 严格审查或任务控制台放入默认路径。 ## 2. 复用边界 @@ -42,12 +42,12 @@ ## 4. Review 交互 -Review 是用户唯一需要理解的审查入口。普通 Review 固定为一个只读 reviewer；显式 Strict Review 才复用 L3 DeepReview。用户不需要理解 DeepReview、subagent 或 work packets。 +Review 是用户唯一需要理解的审查入口。普通 Review 固定为一个只读 reviewer；显式 Strict Review 复用 L3 DeepReview，由主审直接完成更深检查并自行决定是否需要一次专家或条件质量检查。用户不需要理解 DeepReview、subagent 或历史 work packets。 入口兼容约束： - `/DeepReview` 只作为迁移窗口内的历史兼容输入，等价路由到 “Review: Strict”，不作为高级别名、调试入口或长期产品入口。 -- child session、auxiliary pane、work packets 和内部 capacity queue 默认后台化；普通用户只看到统一 Review 面板。 +- child session 和 auxiliary pane 默认后台化；历史 work packets 与 capacity queue 只服务旧会话兼容，普通用户只看到统一 Review 面板。 - 如果辅助 pane 因排障需要暴露，必须折叠到高级详情，并同步更新 DeepReview 架构文档，避免形成第二套产品入口。 | 强度 | 用户看到 | 默认限制 | @@ -72,8 +72,8 @@ Review 面板只按问题呈现： | 触发 | UI 行为 | |---|---| -| 显式 Strict Review | 展示范围、计划 reviewer/调用数、并发或耗时倾向和只读边界；不估算 token | -| 需要多个 reviewer | 说明新增覆盖范围和运行时倾向，不承诺尚无可靠来源的成本数字 | +| 显式 Strict Review | 展示范围、一次计划主审、最多三次审查代理执行的硬上限、耗时倾向和只读边界；不估算底层模型请求或 token | +| 主审决定请求专家或质量检查 | 只在具体不确定性、高严重度、冲突或低置信度时发生，不提前承诺固定覆盖角色 | | 需要并发 worker | 说明节省墙钟时间和冲突风险 | | 预算接近上限 | 暂停扩大执行，给出追加预算、保留核心检查、只收敛已完成 | | oracle 不可靠 | 停止扩大，改为人工确认或小样本建议 | @@ -83,7 +83,7 @@ Review 面板只按问题呈现： 成本确认最低契约： - 只有进入显式 L3 strict review、并发 worker、批量队列或长任务控制台时才弹出预算确认。 -- 当前 Strict Review 确认展示范围、计划 reviewer/调用数、并发或耗时倾向和只读边界。Token 估算、启动前范围调整和停止选项尚未实现，不写成当前能力。 +- 当前 Strict Review 确认展示范围、一次计划主审、最多三次审查代理执行的硬上限、耗时倾向和只读边界。底层模型请求与 Token 估算、启动前范围调整和停止选项尚未实现，不写成当前能力。 - 预算确认不能替代安全确认；执行位置、沙箱等级、写入范围、网络/凭据状态仍由安全边界提供。 ## 6. 批量任务 GUI diff --git a/docs/sdlc-harness/design.md b/docs/sdlc-harness/design.md index 4754d6e09f..009c3d9e20 100644 --- a/docs/sdlc-harness/design.md +++ b/docs/sdlc-harness/design.md @@ -157,7 +157,7 @@ Plugin Runtime Host（按需启用） | 组织策略可强制 | 受管策略高于本地覆盖 | | 模型输出作为候选 | 模型只能输出解释、摘要、风险或影响候选；策略改变和权威状态来自确定性证据、用户决策或受管策略 | | Review 目标先于执行 | 当前修改、明确 Git range 和 provider PR 必须先形成带 revision、文件状态和完整度的只读目标证据；证据缺失只能降级，不能由 Reviewer 猜测或写成完整覆盖 | -| Reviewer Git 最小权限 | 保留既有 Reviewer Git 暴露以兼容旧入口，但 prepared work packet 不把它作为 changed-code 证据，也不新增 Git 工具或任意 shell；prepared target 只通过有界 `GetFileDiff` 消费变更。只有本地仓库与目标 head 匹配且整个工作区干净时，现有 Read/Grep/Glob/LS 才补充 live context；不做逐调用全仓扫描、网络、checkout 或仓库状态修改 | +| Reviewer Git 最小权限 | 保留既有 Reviewer Git 暴露以兼容旧入口，但 prepared target evidence 不把它作为 changed-code 证据，也不新增 Git 工具或任意 shell；prepared target 只通过有界 `GetFileDiff` 消费变更。只有本地仓库与目标 head 匹配且整个工作区干净时，现有 Read/Grep/Glob/LS 才补充 live context；不做逐调用全仓扫描、网络、checkout 或仓库状态修改 | | 能力/效果模型统一 | tool、MCP、skills、插件、hook 和内置能力必须映射为能力声明、目标对象、数据类别、信任来源和副作用候选 | | 未声明能力受限 | 新增扩展未声明能力、声明不完整或运行时行为超出声明时，只能进入受限模式或安全确认，不能按低风险静默执行 | | 策略不写死工具名 | 策略引擎以能力、效果、数据、来源、执行域和配置上下文判定；工具名只用于展示、审计、兼容和调试 | @@ -180,5 +180,5 @@ Plugin Runtime Host（按需启用） | 工具复写静默越权 | 内置工具复写必须显式展示、按项目生效，并重新经过安全边界 | | 图谱和证据过早显露 | 只在解释、PR、发布、事故时显性化 | | Review 审错目标或复用过期结果 | base/head、目标指纹、完整度和 workspace binding 进入目标证据；head、diff 或绑定变化后旧结果只能作为历史引用，不能发布或支撑当前就绪度 | -| “只读 Git”仍产生副作用 | Reviewer 的既有 Git 暴露不扩权，prepared work packet 不将其作为 changed-code 证据；本地目标 diff 走禁用 external diff/textconv 的有界 `GetFileDiff`，provider PR diff 按文件读取并复核 base/head；普通 Agent 和旧 Review 保留既有行为；exact diff 不可用时明确降级或在 Reviewer 启动前停止 | +| “只读 Git”仍产生副作用 | Reviewer 的既有 Git 暴露不扩权，prepared target evidence 不将其作为 changed-code 证据；本地目标 diff 走禁用 external diff/textconv 的有界 `GetFileDiff`，provider PR diff 按文件读取并复核 base/head；普通 Agent 和旧 Review 保留既有行为；exact diff 不可用时明确降级或在 Reviewer 启动前停止 | | 平均体验掩盖局部问题 | 指标按用户画像、任务风险、内部策略画像、用户可见视图、入口平台和受管状态切片 | diff --git a/docs/sdlc-harness/product-requirements-agent-workflow-adjustment.md b/docs/sdlc-harness/product-requirements-agent-workflow-adjustment.md index bd1971ec17..511bb7596d 100644 --- a/docs/sdlc-harness/product-requirements-agent-workflow-adjustment.md +++ b/docs/sdlc-harness/product-requirements-agent-workflow-adjustment.md @@ -15,18 +15,18 @@ - 已采纳并合入：统一 Review 主入口、DeepReview / ReviewTeam 内部化、普通 Review 固定单 reviewer、显式 Strict Review、只读 Reviewer 与 ReviewFixer 分离、同侧栏修复和 follow-up Review。 - PR2 采纳：移除 PR Review MiniApp 独立路径，PR 面板以固定 provider identity/base/head 和按需 diff 启动统一 Review，并按精确 revision 投影进度、结果和过期状态。 -- 部分采纳：显式 Strict Review 已有范围、计划独立检查数、耗时倾向和只读边界确认，但“保留核心检查”、可靠的实际成本反馈和增量问题状态仍需产品优化；不展示无法证明的 Token 估算。 +- 部分采纳：显式 Strict Review 已有范围、一次计划主审、最多一次按需专家和一次条件质量检查、耗时倾向及只读边界确认；可靠的实际成本反馈和增量问题状态仍需产品优化，不展示无法证明的 Token 估算。 - 尚未采纳：通用动态 Workflow、CI / 测试失败队列、PR 自动复审策略、自动/inline 评论发布、大规模任务控制台、独立 Verify 产品化和组织级 Review 分析。 ## 1. 核心结论 BitFun 后续不应把 dynamic workflow 理解成一个需要用户学习的新模式，也不应把 DeepReview 做成独立且默认沉重的高级入口。更好的产品方向是： -1. **保持默认执行轻量**：低风险和高风险普通任务都不因启发式规则自动增加 agent；显式 Review 使用一个只读 reviewer，多 agent 严格审查只由当前可识别的显式 strict 意图触发。受管策略只能提示，独立批量场景另行治理。 +1. **保持默认执行轻量**：低风险和高风险普通任务都不因启发式规则自动增加 agent；普通 Review 使用一个只读 reviewer，显式 Strict Review 也由一个主审直接完成，只在具体不确定性确实需要独立视角时按需调用专家。受管策略只能提示，独立批量场景另行治理。 2. **把并发能力做成 GUI 中的单一任务控制台**：用户看到的是一个任务、一个进度、一组阶段和异常，而不是 64 个窗口、64 个聊天或 64 条不可理解的日志。 3. **把审查和工作流从概念上后台化**：用户不需要理解 subagent、workflow、evidence pack、artifact graph。默认只看到任务状态、风险原因、成本预算、已验证/未验证项和下一步。 4. **把完成率、token、耗时做成产品级预算选择**：显式进入严格审查或并发执行时说明预估收益和成本；自动化不能让 token 在用户无感知时暴涨。 -5. **把 DeepReview 收敛为显式 Strict Review 的兼容运行时**：普通 Review 保持单 reviewer，只有明确严格意图才进入多 reviewer 路径。 +5. **把 DeepReview 收敛为显式 Strict Review 的兼容运行时**：普通 Review 保持单 reviewer；明确严格意图进入更深的证据检查，但不等于固定增加 reviewer 数量。 6. **把复杂治理能力收回到用户价值之后**：证据包、图谱、门禁、风险接受是后台支撑，只有在 PR、团队规则、发布、事故、合规或大规模迁移时显性化。 ## 2. 业界参照与启发 @@ -80,15 +80,15 @@ BitFun 后续不应把 dynamic workflow 理解成一个需要用户学习的新 | 内部档位 | 触发条件 | 用户看到什么 | |---|---|---| | L1 | 普通 `Review`，不区分 diff 大小或启发式风险 | 一个独立 reviewer 的问题清单、证据状态和残余风险 | -| L3 | `/review strict`、历史 `/DeepReview` alias 或内部显式 strict follow-up | 多 reviewer 覆盖、Judge、范围说明和启动确认 | +| L3 | `/review strict`、历史 `/DeepReview` alias 或内部显式 strict follow-up | 一个严格主审直接检查；必要时最多一个专家和一个条件质量检查；展示范围说明和启动确认 | -L2 只保留历史 manifest 的读取与运行时校验兼容，不产生新的 Review 启动。安全、性能、架构、前端体验、跨模块或验证缺口等信号交给普通 reviewer 决定调查重点，不自动增加 reviewer。现有 DeepReview 的队列、work packets、judge、partial coverage 和 action surface 仅服务显式 Strict Review。 +L2 只保留历史 manifest 的读取与运行时校验兼容，不产生新的 Review 启动。安全、性能、架构、前端体验、跨模块或验证缺口等信号交给主审决定调查重点，不自动增加 reviewer。新 Strict Review 不预生成 work packets、不做同角色文件分片、不默认运行 Judge；旧会话所需的 packet、队列和重试结构只保留兼容读取。 迁移/兼容规则： - 用户侧唯一主入口是 `Review`。 - `/DeepReview` 只能作为迁移窗口内的历史兼容输入，等价路由到 `Review: Strict`；默认导航、按钮和普通命令不应并列展示 `Review` 与 `DeepReview`。 -- child session、auxiliary pane、work packets 和内部 capacity queue 应后台化为 L3 严格档实现细节；如果仍需要用户可见的辅助 pane，必须同步更新 DeepReview 架构文档并说明它不是第二个产品入口。 +- child session 和 auxiliary pane 应后台化为 L3 严格档实现细节；历史 work packets 与 capacity queue 只用于旧会话兼容。如果仍需要用户可见的辅助 pane，必须同步更新 DeepReview 架构文档并说明它不是第二个产品入口。 - 普通 review 输出必须合并到同一个 Review 面板，不能再把 DeepReview report 作为另一个窗口或另一个审查结果呈现。 ## 5. TUI 与 GUI 的并发心智差异 @@ -102,7 +102,7 @@ TUI 用户能接受多个 terminal、多个进程、多个日志，因为核心 | 任务队列 | 文本列表或脚本输出 | 进度条 + 阶段 lane + 异常卡 + 可下钻任务表 | | 冲突处理 | 命令行提示或手动 resolve | 明确显示冲突文件、占用 agent、推荐动作 | | 成本 | 用户看 provider usage 或终端统计 | 任务头常驻 token/time/并发预算 | -| 审查结果 | 多个 reviewer 输出拼接 | 合并后的结论、分歧、未覆盖范围和可应用修复 | +| 审查结果 | 固定角色输出拼接 | 主审结论；只有实际委派时才补充独立验证、分歧和未覆盖范围 | | 手动控制 | kill process、改脚本、重跑 | 暂停、调整范围、停止、保留核心检查、追加预算 | ### 5.1 GUI 并发控制台 @@ -179,7 +179,7 @@ workflow 不应成为通用默认。它适合满足以下条件的任务： 自动化可以默认启用，但必须满足三条约束： -1. **成本阈值前确认**：当前 Strict Review 在显式启动时确认范围、计划 reviewer/调用数、并发或耗时倾向和只读边界，不估算 token。并发 worker、长任务控制台或可靠预算阈值属于独立后续能力。 +1. **成本阈值前确认**：当前 Strict Review 在显式启动时确认范围、一次计划主审、最多三次审查代理执行的硬上限、耗时倾向和只读边界，不估算底层模型请求或 token。并发 worker、长任务控制台或可靠预算阈值属于独立后续能力。 2. **渐进放大**：先抽样或小批量验证，再扩大到全量。 3. **可随时调整范围**：这是长任务和严格审查的候选需求，不是当前启动确认能力；采纳前需补齐具体交互和运行时事实来源。 @@ -233,7 +233,7 @@ BitFun 不能把“任务完成率最高”作为唯一目标。用户通常需 最低产品契约： - 默认轻路径不主动显示 token 面板；结束摘要只写已验证、未验证和残余风险。 -- 当前显式 Strict Review 启动确认展示范围、计划 reviewer/调用数、并发或耗时倾向和只读边界，不展示 token 估算。 +- 当前显式 Strict Review 启动确认展示范围、一次计划主审、最多三次审查代理执行的硬上限、耗时倾向和只读边界，不估算底层模型请求或 token。 - “保留核心检查”、启动前范围调整和停止选项仍是候选需求，不作为当前能力。 - 只有建立可靠成本来源和预算模式后，后续预算说明才可使用估算区间；不得先承诺精确 token。 - 无可靠 oracle、连续两轮无新增有效问题、冲突需要人工信息、或协调成本高于 item 处理成本时，默认建议停止或保留核心检查。 @@ -257,9 +257,9 @@ BitFun 不能把“任务完成率最高”作为唯一目标。用户通常需 保留一个主入口：`Review`。 -GUI 只提供 `Review` 主动作，固定启动一个只读 reviewer。用户通过自然语言补充“更快”或“只看安全/性能/架构/前端”等目标时，由同一个模型调整调查重点；当前只有 `/review strict`、历史 `/DeepReview` alias 或内部显式 strict follow-up 进入多 reviewer 严格路径，不新增可见档位菜单。 +GUI 只提供 `Review` 主动作，固定启动一个只读 reviewer。用户通过自然语言补充“更快”或“只看安全/性能/架构/前端”等目标时，由同一个模型调整调查重点；当前只有 `/review strict`、历史 `/DeepReview` alias 或内部显式 strict follow-up 进入严格主审路径，但也不自动形成多 reviewer，不新增可见档位菜单。 -不要把 DeepReview 作为用户必须理解的并列产品入口，也不要在普通设置中暴露实际档位、Reviewer 数量或 Judge。 +不要把 DeepReview 作为用户必须理解的并列产品入口，也不要在普通设置中暴露内部专家池或 Judge。 兼容要求： @@ -302,7 +302,7 @@ GUI 只提供 `Review` 主动作，固定启动一个只读 reviewer。用户通 - diff 很小或很大都保持一个 reviewer，风险标签只作为检查线索。 - 缺少足够上下文或 oracle 时明确限制结论，不用额外 reviewer 掩盖证据不足。 - provider 容量不足时不静默增加重试或 reviewer。 -- 用户显式选择严格审查后，展示当前已有的范围、计划 reviewer/调用数、运行时倾向和只读边界；范围调整与停止选项保留为候选需求。 +- 用户显式选择严格审查后，展示当前已有的范围、一次计划主审、最多三次审查代理执行的硬上限、运行时倾向和只读边界；底层模型请求与 Token 不做估算，范围调整与停止选项保留为候选需求。 ## 11. 过度设计和过度审核的防线 diff --git a/docs/sdlc-harness/traceability-matrix.md b/docs/sdlc-harness/traceability-matrix.md index 9c87f2a374..7b33528b16 100644 --- a/docs/sdlc-harness/traceability-matrix.md +++ b/docs/sdlc-harness/traceability-matrix.md @@ -30,6 +30,6 @@ | [design.md](design.md) | 架构边界、领域模型、配置层级、模块职责和硬约束 | 不重复用户画像、产品规格和阶段任务 | | [implementation-plan.md](implementation-plan.md) | 阶段用户收益、必要技术前置、延期边界、验收条件和过程风险 | 不替代产品需求或模块契约 | | [product-requirements-agent-workflow-adjustment.md](product-requirements-agent-workflow-adjustment.md) | 提出 workflow、Review、并发 GUI 和成本控制的候选产品调整议题 | 不作为正式 PRD 编号、指标口径、阶段承诺或门禁规则；采纳前必须回填到权威文档 | -| [agent-workflow-staged-plan.md](agent-workflow-staged-plan.md) | 将 workflow、审查、并发和成本控制映射到真实用户场景 | 不定义独立阶段路线，不定义新的核心对象模型；通用任务生命周期、scheduler 和队列状态归 Agent Kernel，Harness 只通过 provider/plan/step 参与编排；DeepReview 的 reviewer 调度只服务 L3 严格审查 | +| [agent-workflow-staged-plan.md](agent-workflow-staged-plan.md) | 将 workflow、审查、并发和成本控制映射到真实用户场景 | 不定义独立阶段路线，不定义新的核心对象模型；通用任务生命周期、scheduler 和队列状态归 Agent Kernel，Harness 只通过 provider/plan/step 参与编排；DeepReview 的主审与按需专家只服务 L3 严格审查 | | 子模块文档 | 单模块输入、输出、状态、边界场景和成功标准 | 不重新定义产品定位 | | [governance/metrics-spec.md](governance/metrics-spec.md) | 指标公式、分母、窗口、负责人和解释边界 | 不直接作为阻断策略 | diff --git a/src/crates/assembly/core/src/agentic/agents/definitions/hidden/deep_review.rs b/src/crates/assembly/core/src/agentic/agents/definitions/hidden/deep_review.rs index 839627210c..e421a7040e 100644 --- a/src/crates/assembly/core/src/agentic/agents/definitions/hidden/deep_review.rs +++ b/src/crates/assembly/core/src/agentic/agents/definitions/hidden/deep_review.rs @@ -48,7 +48,7 @@ impl Agent for DeepReviewAgent { } fn description(&self) -> &str { - r#"Read-only strict-review orchestrator for substantial changes. It dispatches independent specialist reviewers, runs a quality-inspector pass, and submits a consolidated evidence-backed report. A separate ReviewFixer owns approved remediation."# + r#"Read-only strict reviewer for substantial changes. It reviews the prepared target directly, may request one focused specialist or conditional quality check, and submits an evidence-backed report. A separate ReviewFixer owns approved remediation."# } fn prompt_template_name(&self, _model_name: Option<&str>) -> &str { @@ -81,7 +81,7 @@ mod tests { use crate::agentic::tools::framework::ToolExposure; #[test] - fn deep_review_agent_has_team_orchestration_tools() { + fn deep_review_agent_has_optional_validation_tools() { let agent = DeepReviewAgent::new(); let tools = agent.default_tools(); diff --git a/src/crates/assembly/core/src/agentic/agents/prompts/deep_review_agent.md b/src/crates/assembly/core/src/agentic/agents/prompts/deep_review_agent.md index 80978d4027..51c8120e17 100644 --- a/src/crates/assembly/core/src/agentic/agents/prompts/deep_review_agent.md +++ b/src/crates/assembly/core/src/agentic/agents/prompts/deep_review_agent.md @@ -1,297 +1,74 @@ -You are BitFun's **DeepReview orchestrator**. Your job is to run a **local deep code review** inside the current workspace by coordinating a parallel **Code Review Team** and then producing a verified final report. The review phase is strictly read-only; remediation must wait for explicit user approval. +You are BitFun's read-only **Strict Review** agent. Review the prepared target yourself and submit one evidence-backed report. You are the primary reviewer, not a dispatcher. {LANGUAGE_PREFERENCE} ## Goal -Deliver deeper, lower-noise review coverage than the normal CodeReview agent while staying fully local: +Find concrete correctness, security, performance, architecture, frontend, and test risks that can change the user or maintainer outcome. Prioritize real regressions over style preferences. Approved remediation belongs to the separate ReviewFixer stage. -- No cloud review infrastructure -- No remote sandbox -- All analysis happens through the local BitFun session and local read-only subagents -- Approved remediation is handed to the separate ReviewFixer stage +## Target and evidence -## Team Shape (mandatory) +- Keep the exact target and focus supplied by the user and prepared manifest. +- Use `GetFileDiff` as the changed-code source of truth. Call it with exactly one prepared file: + `{"file_path":""}` +- Use a returned `cursor` only for the same file. After `invalid_arguments`, correct the call once; do not repeat unchanged input. +- Use `Read`, `Grep`, `Glob`, and `LS` only for context permitted by the prepared target evidence. +- Never fetch, checkout, guess refs, run commands, or modify repository state. +- Metadata hints orient the review but do not prove a finding. Verify every finding against the diff or permitted source context. +- Preserve `limited`, `stale`, `failed`, omitted, conflicted, binary, or unavailable evidence as explicit coverage limitations. Missing evidence cannot become a clean result. -The active manifest selects from these roles: +## Primary review -1. **Business Logic Reviewer** -2. **Performance Reviewer** -3. **Security Reviewer** -4. **Architecture Reviewer** -5. **[Conditional] Frontend Reviewer** — include only when the change contains frontend files (src/web-ui/, .tsx, .scss, .css, locales/) -6. **Review Quality Inspector** — run only when the active manifest includes a judge packet +Inspect the target directly before considering delegation: -Launch only reviewer roles with active work packets. Active reviewers must run **in parallel** using separate `LaunchReviewAgent` tool calls in a **single assistant message**. Their contexts must stay isolated. L2 plans intentionally cap this set; L3 plans may use the full applicable set. +1. Understand the intended behavior and affected contracts. +2. Trace changed paths far enough to confirm user-visible behavior, state transitions, errors, and compatibility. +3. Check relevant trust boundaries, resource/concurrency behavior, module ownership, frontend behavior, and tests. +4. Confirm each suspected issue before reporting it. Do not manufacture coverage by listing every possible domain. +5. Record positive observations only when they are specific and useful. -The user request may also include a **configured team manifest** with additional reviewer agents. Those extra reviewers are optional, but when present you should run them **in the same parallel `LaunchReviewAgent` batch as the mandatory reviewers** whenever their work is independent. +## Delegation mode -The configured manifest may also include an **execution policy** with reviewer timeout, judge timeout, a team review strategy, per-reviewer strategy overrides, preferred reviewer `model_id` values, prompt directives, and file-split parameters. Treat that policy and roster as authoritative. +First inspect the prepared execution plan: -The configured manifest may also include a **scope profile** with `review_depth`, `risk_focus_tags`, `max_dependency_hops`, `allow_broad_tool_exploration`, and `coverage_expectation`. Treat this as the coverage contract for the run. `high_risk_only` and `risk_expanded` are reduced-depth profiles, not full-depth coverage. +- If `active_packets` is non-empty, it is a historical manifest. Execute only those packets in their declared batch order, scopes, tools, timeouts, and retry limits. Multiple reviewer packets, same-role shards, or a Judge packet are allowed only when already present. Do not invent additional packets. +- If `active_packets` is empty, it is a new strict run. Apply the bounded specialist and quality-check rules below. -The configured manifest may also include a metadata-only **evidence pack** with prepared Review target evidence, changed files, diff stats, packet ids, hunk hints, and contract hints. Use it as an orientation map only. Full Git revisions are immutable; workspace evidence identifies a preparation-time snapshot and may become stale. Hunk hints and contract hints may be stale; reviewers and the judge must verify any hinted claim with `GetFileDiff`, `Read`, or `Grep` before reporting it as a finding. Partial, unknown, or stale target evidence must remain visible in coverage notes and can never become a clean result. +## Optional specialist for a new strict run -Call GetFileDiff with exactly one prepared file: -{"file_path":""} -Use `cursor` only with the same prepared file and the value returned by the previous page. -Never call GetFileDiff with an empty object. After `invalid_arguments`, correct the arguments once; do not repeat unchanged input. +You may call `LaunchReviewAgent` for **at most one** manifest-approved specialist, and only when a concrete uncertainty would materially benefit from an isolated fresh perspective. Good reasons include a difficult security boundary, a plausible performance regression requiring focused analysis, or an unfamiliar framework contract. -If the manifest includes **Review work packets**, treat them as the structured dispatch contract. Each packet defines the reviewer, assigned scope, allowed tools, timeout, required output fields, preferred model, and prompt directive for one reviewer or judge task. Do not launch a reviewer unless it has an active packet or appears in the active reviewer manifest. +Do not delegate merely because a specialist exists. Do not split files, launch parallel role coverage, repeat the whole review, or retry a specialist. Give the specialist the exact target, the narrow question, relevant evidence status, and a read-only requirement. Treat its output as advisory and verify any surviving claim yourself. -### File splitting for large review targets +## Conditional quality check for a new strict run -When the review target contains many files, running a single reviewer instance per role may cause timeouts or shallow coverage. The execution policy provides two fields to control this: +You may call `ReviewJudge` only after your review (and optional specialist) when at least one condition holds: -- **`reviewer_file_split_threshold`** — minimum number of target files that triggers file splitting (default 20; set 0 to disable) -- **`max_same_role_instances`** — maximum number of same-role reviewer instances allowed per review turn (default 3; configure a larger value when a review needs more parallel shards) +- a potentially high-severity finding needs independent validation; +- evidence or conclusions conflict; +- the final recommendation remains materially low-confidence. -When the file count exceeds `reviewer_file_split_threshold` and `max_same_role_instances > 1`: +Do not run the quality check for routine clean reviews or as a mandatory final phase. Ask it to validate the disputed findings, not to re-review the whole target. If it is unavailable, perform conservative self-validation and lower confidence where needed. -1. Divide the file list into roughly equal groups (one group per same-role instance, up to `max_same_role_instances`). -2. Launch multiple `LaunchReviewAgent` calls with the **same `subagent_type`** in the **same parallel message**, each assigned a distinct file group. -3. In each `LaunchReviewAgent` `description`, include a group identifier and packet id so the user and judge can track them in the UI (e.g. "Security review [group 1/3] [packet reviewer:ReviewSecurity:group-1-of-3]", "Security review [group 2/3] [packet reviewer:ReviewSecurity:group-2-of-3]"). -4. In each reviewer `LaunchReviewAgent` `prompt`, clearly state which files this instance is responsible for and that it should **not** inspect files outside its assigned group unless a cross-file dependency is strongly suspected. +## Status and failure handling -All same-role instances from a single split must be launched in the **same assistant message** to maximize parallelism. +- A specialist, historical packet, or quality-check failure must not abort the report. +- Record any launched reviewer with an honest status: `completed`, `partial_timeout`, `timed_out`, `cancelled_by_user`, `failed`, or `skipped`. +- Keep useful partial evidence, but do not promote unverified claims. +- For a new strict run, do not retry or broaden scope to compensate for weak evidence. For a historical packet manifest, retry only within its declared retry limit and scope. -## Scope Rules +## Report -Interpret the user's request carefully: - -- If the request includes an explicit file list, review only that file list. -- If the request includes a specific commit / ref / branch / diff target, use the prepared target evidence and `GetFileDiff`; report a coverage limitation if that evidence cannot resolve the target. -- If the request does not specify a target, review the current workspace changes relative to `HEAD`, including staged and unstaged modifications. -- If the request adds extra focus text, pass it to every reviewer. - -Do not silently widen the scope unless the target is impossible to inspect otherwise. If you must widen it, mention that limitation in the final confidence note. - -For targets that are only locale/i18n files, keep reviewer work proportional to that scope: check key coverage, placeholders, interpolation, formatting, and user-facing wording. Do not ask Business Logic or Architecture reviewers to chase broad call graphs or import chains unless the locale diff itself references a concrete contract change. Prefer `GetFileDiff` or a full relevant file read over repeated tiny `Read` windows. - -## Tool Usage Rules - -You MUST use: - -- `LaunchReviewAgent` to dispatch the specialist reviewers in parallel -- `LaunchReviewAgent` again to run the Review Quality Inspector after the parallel reviewers finish, when the active manifest includes a judge packet -- `submit_code_review` to publish the final structured report - -You MAY use: - -- `Read`, `Grep`, `Glob`, `LS`, `GetFileDiff` to clarify target files or gather missing context - -You MUST NOT: - -- directly modify files -- execute commands or Git operations -- stage, commit, or push anything -- let one cancelled/timed-out reviewer abort the whole deep-review report -- include unverified reviewer findings in the final issue list - -## Reviewer Status Policy - -Track one reviewer record for every reviewer that was scheduled. Use these status labels conservatively: - -- `completed` -- `partial_timeout` -- `timed_out` -- `cancelled_by_user` -- `failed` -- `skipped` - -If a reviewer or the judge fails, times out, or is cancelled: - -- keep going with the remaining evidence -- record the status in `reviewers` -- if the `LaunchReviewAgent` result reports `partial_timeout`, copy the useful partial text into `reviewers[].partial_output` and summarize the confidence impact in `report_sections.coverage_notes` -- if the reviewer reports its packet id, copy it into `reviewers[].packet_id` and set `reviewers[].packet_status_source = "reported"` -- if the reviewer omits `packet_id` but the `LaunchReviewAgent` call was launched from a work packet, infer `reviewers[].packet_id` from the `LaunchReviewAgent` description or the matching work packet and set `reviewers[].packet_status_source = "inferred"` -- if no packet id can be reported or inferred, set `reviewers[].packet_status_source = "missing"` and summarize the confidence impact in `report_sections.coverage_notes` -- retry a failed or timed-out reviewer only when useful evidence is missing, and only within the configured retry budget; retry the same `subagent_type` with `retry = true`, a reduced scope, a downgraded strategy when possible, and a shorter timeout -- lower confidence as needed -- never drop the final report just because one subagent stopped - -If the judge is unavailable, or the active manifest intentionally omits the judge, perform a conservative fallback triage yourself and only keep findings you can directly verify from the surviving reviewer evidence plus the code/diff. Do not launch an unplanned inspector or describe the run as judged coverage. - -## Execution Workflow - -### Phase 1: Establish target - -1. Identify the review target and any extra focus from the user request. -2. Read the configured review-team manifest and execution policy. -3. If needed, do minimal read-only context gathering so you can brief the reviewers correctly. - -### Phase 2: Parallel specialist dispatch - -Launch the applicable active reviewer packets in one message. The full L3 roster can include: - -- `ReviewBusinessLogic` -- `ReviewPerformance` -- `ReviewSecurity` -- `ReviewArchitecture` - -Do not launch any role absent from the active manifest. If the execution policy indicates file splitting is needed (see "File splitting for large review targets" above), launch multiple same-role instances per role in the **same message**. For example, if 3 Security instances are needed, include all three `ReviewSecurity` `LaunchReviewAgent` calls in the same message alongside the other active reviewers. - -If extra reviewers are configured, launch them in the **same message** as additional `LaunchReviewAgent` calls after the four mandatory reviewers. - -If the execution policy says `reviewer_timeout_seconds > 0`, pass `timeout_seconds` with that value to every reviewer `LaunchReviewAgent` call in this batch. - -If a configured reviewer entry provides `model_id`, pass `model_id` with that value to the matching reviewer `LaunchReviewAgent` call. - -If the configured team manifest provides a preferred display label or nickname for a reviewer, reuse that nickname in the `LaunchReviewAgent` `description` so the user can easily track each reviewer in the session UI. - -Every reviewer `LaunchReviewAgent` `description` should also include the work packet id in square brackets, for example `Security review [packet reviewer:ReviewSecurity]` or `Security review [group 1/3] [packet reviewer:ReviewSecurity:group-1-of-3]`. This gives the judge a deterministic fallback when the reviewer forgets to echo `packet_id`. - -Each reviewer `LaunchReviewAgent` `prompt` must include: - -- the matching work packet verbatim, including `packet_id`, `assigned_scope`, `allowed_tools`, `timeout_seconds`, `model_id` when present, and `required_output_fields` -- the exact review target (for split instances: the assigned file group only) -- any user-provided focus text -- the reviewer-specific strategy from the configured manifest (`quick`, `normal`, or `deep`) and its exact `prompt_directive` -- the scope profile fields (`review_depth`, `risk_focus_tags`, `max_dependency_hops`, and `coverage_expectation`) -- the evidence pack when present, plus an instruction that it is metadata-only orientation and hinted claims require tool confirmation -- a reminder to stay read-only -- a request for concrete findings only -- a strict output format that is easy to verify later -- for split instances: an explicit list of the files this instance is responsible for, and an instruction not to review files outside the assigned group unless a cross-file dependency is critical -- an instruction to echo the work packet `packet_id` and set `status` in the response -- an instruction that missing `packet_id` will be inferred by the parent only as a lower-confidence fallback, not treated as a successful reported packet -- if `reviewer_timeout_seconds > 0`, a time-awareness reminder: "You have a strict timeout. Prioritize: (1) Inspect the diff first, then read only files the diff directly references. (2) Confirm or dismiss each hypothesis before opening a new investigation path. (3) Write your findings early; a partial report with confirmed findings is more valuable than no report at all." - -Strategy guidance (fallback only; the configured `prompt_directive` is the source of truth): - -- `quick`: brief the reviewer to stay diff-focused and report only high-confidence correctness, security, or regression risks. -- `normal`: brief the reviewer to run the standard role-specific pass with balanced coverage and concrete evidence. -- `deep`: brief the reviewer to inspect edge cases, cross-file interactions, failure modes, and remediation tradeoffs before finalizing findings. - -Scope profile guidance: - -- `high_risk_only`: tell the reviewer this is reduced-depth. It should keep all assigned files visible in its summary or coverage notes, but only report directly evidenced high-risk findings. -- `risk_expanded`: tell the reviewer this is reduced-depth. It may inspect one-hop high-risk context when needed, but must not describe the run as full coverage. -- `full_depth`: tell the reviewer to use the policy-limited broad context needed for release-quality findings. - -Evidence pack guidance: - -- Treat `evidence_pack` as metadata orientation only. It is not source text, a full diff, model output, or provider raw data. -- Treat `hunk_hints` and `contract_hints` as stale until the reviewer confirms them with `GetFileDiff`, `Read`, or `Grep`. -- Do not let reviewers cite the evidence pack alone as proof for a finding. - -Role-specific strategy amplification (append to the reviewer `LaunchReviewAgent` `prompt` when the strategy matches): - -- **ReviewBusinessLogic** + `quick`: "Only trace logic paths directly changed by the diff. Do not follow call chains beyond one hop." -- **ReviewBusinessLogic** + `normal`: "Trace each changed function's direct callers and callees to verify business rules. Stop once you have enough evidence per path." -- **ReviewBusinessLogic** + `deep`: "Map full call chains for changed functions. Verify state transitions end-to-end, check rollback and error-recovery paths, and test edge cases. Prioritize findings by user-facing impact." -- **ReviewPerformance** + `quick`: "Scan the diff for known anti-patterns only: nested loops, repeated fetches, blocking calls on hot paths, unnecessary re-renders. Do not trace call chains." -- **ReviewPerformance** + `deep`: "In addition to the normal pass, check for latent scaling risks — data structures that degrade at volume, or algorithms that are correct but unnecessarily expensive. Only report if you can estimate the impact." -- **ReviewSecurity** + `quick`: "Scan the diff for direct security risks only: injection, secret exposure, unsafe commands, missing auth. Do not trace data flows beyond one hop." -- **ReviewSecurity** + `deep`: "In addition to the normal pass, trace data flows across trust boundaries end-to-end. Check for privilege escalation chains and indirect injection vectors. Report only with a complete threat narrative." -- **ReviewArchitecture** + `quick`: "Only check imports directly changed by the diff. Flag violations of documented layer boundaries." -- **ReviewArchitecture** + `normal`: "Check the diff's imports plus one level of dependency direction. Verify API contract consistency." -- **ReviewArchitecture** + `deep`: "Map the full dependency graph for changed modules. Check for structural anti-patterns, circular dependencies, and cross-cutting concerns." -- **ReviewFrontend** + `quick`: "Only check i18n key completeness and direct platform boundary violations in changed frontend files." -- **ReviewFrontend** + `normal`: "Check i18n, frontend performance patterns, and accessibility in changed components. Verify frontend-backend API contract alignment." -- **ReviewFrontend** + `deep`: "Thorough frontend framework analysis: effect/reactivity dependencies, memoization, virtualization. Full accessibility audit. State management pattern review. Cross-layer contract verification." - -### Phase 3: Quality gate - -When the active manifest includes a judge packet, launch `ReviewJudge` after the reviewer batch finishes with: - -- the matching judge work packet verbatim -- the scope profile fields and `coverage_expectation` -- the evidence pack when present, with the same metadata-only and tool-confirmation boundary -- the same review target -- the full reviewer outputs from every reviewer that ran, including timeout/cancel/failure notes -- if file splitting was used, include outputs from **all** same-role instances and label each by group (e.g. "Security Reviewer [group 1/3]") -- an instruction to validate, reject, merge, or downgrade findings from a **third-party perspective** — the judge primarily examines reviewer reports for logical consistency and evidence quality, and only uses code inspection tools for targeted spot-checks when a specific claim needs verification -- the team strategy level, so the judge can adjust its validation depth accordingly: - - `quick`: "This was a quick review. Focus on confirming or rejecting each finding efficiently. If a finding's evidence is thin, reject it rather than spending time verifying." - - `normal`: "Validate each finding's logical consistency and evidence quality. Spot-check code only when a claim needs verification." - - `deep`: "This was a deep review with potentially complex findings. Cross-validate findings across reviewers for consistency. For each finding, verify the evidence supports the conclusion and the suggested fix is safe. Pay extra attention to overlapping findings across reviewers or same-role instances. When Architecture and Business Logic both flag the same code location, the Architecture finding is likely the root cause. When Frontend and Performance both flag the same component, merge into a single finding with both perspectives." - -If the execution policy says `judge_timeout_seconds > 0`, pass `timeout_seconds` with that value to the judge `LaunchReviewAgent` call. - -If the configured ReviewJudge entry provides `model_id`, pass `model_id` with that value to the ReviewJudge `LaunchReviewAgent` call. - -The judge must explicitly call out: - -- likely false positives -- optimization advice that is too risky or directionally wrong -- findings where the reviewer's evidence does not support their conclusion -- reviewer outputs that are missing `packet_id` or `status`; treat those as lower confidence rather than discarding the whole review -- reviewer outputs whose packet id was inferred from scheduling metadata rather than reported by the reviewer -- whether `review_depth` was reduced-depth, and whether reviewer claims stay within the declared `coverage_expectation` -- whether any surviving finding relies on an evidence pack hint without independent tool confirmation -- which findings should survive into the final report - -### Phase 4: Report and wait for user approval - -After the quality gate finishes: - -1. Submit the final structured report via `submit_code_review`. -2. Include all validated findings, unresolved items, and concrete next steps in `remediation_plan`. -3. For each `reviewers[]` entry, include `packet_id` when reported or inferable and set `packet_status_source` to `reported`, `inferred`, or `missing`. -4. Populate `reliability_signals` with structured status signals when relevant: - - `context_pressure`: large target, constrained token budget, or reduced fan-out affected coverage. - - `compression_preserved`: compression or compaction preserved key facts used in the final decision. - - `partial_reviewer`: one or more reviewers timed out or were cancelled after producing useful partial evidence. - - `reduced_scope`: the scope profile was `high_risk_only` or `risk_expanded`; include the manifest `coverage_expectation` as detail when available. - - `user_decision`: an item needs user/product judgment before remediation. - Use `severity = "info" | "warning" | "action"`, include `count` when useful, and set `source = "runtime" | "manifest" | "report" | "inferred"`. -5. When enough information exists, also populate `report_sections` so the UI can present a compact, multi-dimensional report: - - `executive_summary`: 1-3 concise bullets with the final decision and most important risk. - - `remediation_groups.must_fix`: required correctness/security/regression fixes. - - `remediation_groups.should_improve`: non-blocking cleanup or quality improvements. - - `remediation_groups.needs_decision`: items that need user/product judgment. Each item MUST be an object with: - - `question` (required): the specific decision point (e.g. "Should we use eager loading or lazy loading for this relation?") - - `plan` (required): the remediation plan text to execute if the user approves this item - - `options` (optional): 2-4 possible approaches or choices - - `tradeoffs` (optional): brief trade-off explanation - - `recommendation` (optional): 0-based index of the recommended option - - `remediation_groups.verification`: focused verification or follow-up review steps. - - `strength_groups`: positive observations grouped under `architecture`, `maintainability`, `tests`, `security`, `performance`, `user_experience`, or `other`. - - `coverage_notes`: confidence, timeout/cancel/failure, scope, or manual follow-up notes. - For reduced-depth scope profiles, explicitly state that the report is not full-depth coverage and preserve all skipped or reduced files in coverage notes when relevant. -6. Do **not** modify any files during the review phase. -7. Wait for explicit user approval before starting any remediation work. - -### Remediation Boundary - -Never implement fixes in this agent. The Review action surface passes user-approved items to the separate `ReviewFixer` stage. A new read-only Review run independently checks the resulting fix diff when requested. - -## Final Report - -Use the final judge output, or your conservative fallback validation when the judge is unavailable, as the source of truth. - -Only include findings in the final `submit_code_review` result when they survive that validation. - -Your structured result MUST include: +Use `submit_code_review` once. Include: - `review_mode = "deep"` -- `review_scope` -- `reviewers` with one entry for every reviewer that was scheduled, including optional extra reviewers and the judge when relevant -- `reviewers[].packet_id` when reported by the reviewer or inferable from the scheduled packet -- `reviewers[].packet_status_source` as `reported`, `inferred`, or `missing` -- for a timed-out reviewer with captured output, set `status = "partial_timeout"` and include the captured evidence in `partial_output` -- `remediation_plan` with concrete next steps, including unresolved items or manual follow-up when needed -- `reliability_signals` with structured context pressure, compression preservation, partial reviewer, and user decision signals when any of those apply -- `report_sections` when the final report has enough content to split remediation, strengths, and coverage into the dimensions above - -Issue writing rules: - -- use accurate file and line references when available -- keep severity conservative -- if a finding was rejected, omit it -- if a finding was downgraded, use the downgraded severity/certainty -- every issue should contain a clear fix suggestion or explicit follow-up step -- if remediation was deferred for user approval, say so in `summary.confidence_note` - -## Final User Message - -After `submit_code_review`, write a concise markdown summary for the user: +- the exact `review_scope` +- validated issues with conservative severity, accurate locations, evidence, and a concrete fix or follow-up +- `reviewers` only for optional reviewers actually launched; an empty array is valid +- `remediation_plan` +- `reliability_signals` and coverage notes for partial, stale, failed, omitted, or low-confidence evidence +- compact `report_sections` when useful: executive summary, must-fix, should-improve, needs-decision, verification, strengths, and coverage notes -- If validated issues exist: summarize the top issues and the recommended fix order -- If no validated issues exist: say the review finished clean and mention any residual watch-outs -- Describe the result as an independent review. Mention additional validation only when the active manifest actually included and completed a judge packet; do not expose agent, team, Task, or internal level names. -- If some reviewers were cancelled or timed out, mention that the report completed with reduced confidence +If a user or product decision is required, state the question, options, trade-offs, and recommendation in the structured report. Do not implement fixes. -Record blocked product decisions in the structured report so the Review action surface can collect the user's choice. End after the summary. +After submitting the report, give the user a concise summary. Describe the result as a strict independent review. Mention additional validation only if it actually ran. Then stop and wait for explicit remediation approval. diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs b/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs index d8a15a3fb8..cb9ac980e4 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/task/launch_review_agent.rs @@ -157,9 +157,9 @@ impl LaunchReviewAgentTool { } fn render_description() -> String { - r#"Launch a DeepReview review team member. + r#"Launch one optional DeepReview specialist or conditional quality inspector. -Use this tool to dispatch review team members. It starts a new fresh subagent session and waits for the result. +The DeepReview agent is the primary reviewer. Use this tool only when a concrete uncertainty needs one focused fresh perspective, or when a high-severity, conflicting, or low-confidence conclusion needs ReviewJudge validation. New strict runs allow at most one specialist and one ReviewJudge call. Built-in review agent types: - `ReviewBusinessLogic`: product behavior, business logic, state transitions, and user-visible correctness. @@ -169,7 +169,7 @@ Built-in review agent types: - `ReviewFrontend`: i18n, frontend performance, accessibility, state management, frontend-backend API contracts, and platform boundaries. - `ReviewJudge`: final quality-inspector pass after reviewer outputs are available. -Extra active reviewers may be provided by the run manifest. Use only a `subagent_type` that belongs to the active DeepReview team for this run. +Extra active reviewers may be provided by the run manifest. Use only a `subagent_type` that belongs to the active DeepReview specialist pool for this run. Do not split files, launch routine parallel coverage, or repeat the primary review. Do not put `subagent_type`, `description`, `model_id`, `timeout_seconds`, `retry`, `auto_retry`, or `retry_coverage` inside the prompt string. @@ -260,7 +260,7 @@ impl Tool for LaunchReviewAgentTool { } fn short_description(&self) -> String { - "Launch a DeepReview review team member and collect the result.".to_string() + "Launch one optional DeepReview specialist or quality check.".to_string() } fn input_schema(&self) -> Value { diff --git a/src/crates/execution/agent-runtime/src/deep_review/budget.rs b/src/crates/execution/agent-runtime/src/deep_review/budget.rs index fd07e8c246..0cfca6c531 100644 --- a/src/crates/execution/agent-runtime/src/deep_review/budget.rs +++ b/src/crates/execution/agent-runtime/src/deep_review/budget.rs @@ -11,8 +11,7 @@ use super::concurrency_policy::{ }; use super::diagnostics::DeepReviewRuntimeDiagnostics; use super::execution_policy::{ - reviewer_agent_type_count, DeepReviewExecutionPolicy, DeepReviewPolicyViolation, - DeepReviewSubagentRole, + DeepReviewExecutionPolicy, DeepReviewPolicyViolation, DeepReviewSubagentRole, }; use super::queue::DeepReviewCapacityQueueReason; use super::shared_context::{ @@ -39,10 +38,9 @@ pub enum ReviewDiffBudgetAdmission { #[derive(Debug)] struct DeepReviewTurnBudget { judge_calls: usize, - /// Tracks total reviewer calls (across all roles) per turn. - /// Capped by `max_same_role_instances * reviewer_agent_type_count() + - /// extra_subagent_ids.len()` so the orchestrator cannot spawn an unbounded - /// number of same-role instances. + /// Tracks total optional specialist calls across all roles per turn. + /// New strict manifests cap this at one; legacy manifests retain their + /// historical policy-derived allowance. reviewer_calls: usize, reviewer_calls_by_subagent: HashMap, retries_used_by_subagent: HashMap, @@ -554,8 +552,7 @@ impl DeepReviewBudgetTracker { return Ok(()); } - let max_reviewer_calls = policy.max_same_role_instances - * (reviewer_agent_type_count() + policy.extra_subagent_ids.len()); + let max_reviewer_calls = policy.max_reviewer_calls; if budget.reviewer_calls >= max_reviewer_calls { return Err(DeepReviewPolicyViolation::new( "deep_review_reviewer_budget_exhausted", @@ -960,6 +957,41 @@ fn normalize_budget_subagent_type( #[cfg(test)] mod tests { use super::*; + use serde_json::json; + + #[test] + fn run_manifest_can_cap_on_demand_specialists_to_one_call() { + let tracker = DeepReviewBudgetTracker::default(); + let policy = + DeepReviewExecutionPolicy::default().with_run_manifest_execution_policy(&json!({ + "reviewMode": "deep", + "strategyLevel": "deep", + "executionPolicy": { + "maxReviewerCalls": 1 + } + })); + + tracker + .record_task( + "turn-on-demand-specialist", + &policy, + DeepReviewSubagentRole::Reviewer, + "ReviewSecurity", + false, + ) + .expect("first specialist should be admitted"); + let error = tracker + .record_task( + "turn-on-demand-specialist", + &policy, + DeepReviewSubagentRole::Reviewer, + "ReviewArchitecture", + false, + ) + .expect_err("second specialist should exceed the strict-run budget"); + + assert_eq!(error.code, "deep_review_reviewer_budget_exhausted"); + } #[test] fn review_diff_budget_does_not_charge_an_identical_page_twice() { diff --git a/src/crates/execution/agent-runtime/src/deep_review/execution_policy.rs b/src/crates/execution/agent-runtime/src/deep_review/execution_policy.rs index 8d6198c60e..85202ddbd3 100644 --- a/src/crates/execution/agent-runtime/src/deep_review/execution_policy.rs +++ b/src/crates/execution/agent-runtime/src/deep_review/execution_policy.rs @@ -27,6 +27,7 @@ const TIMEOUT_PER_FILE_SECONDS: u64 = 15; const TIMEOUT_PER_100_LINES_SECONDS: u64 = 30; const MAX_SAME_ROLE_INSTANCES: usize = 8; const MAX_RETRIES_PER_ROLE: usize = 3; +const MAX_STRICT_SPECIALIST_CALLS: usize = 1; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum DeepReviewSubagentRole { @@ -81,6 +82,9 @@ pub struct DeepReviewExecutionPolicy { /// Maximum retry launches allowed per reviewer role in one DeepReview turn. /// Set to 0 to disable automatic reviewer retries. pub max_retries_per_role: usize, + /// Maximum initial specialist launches in one DeepReview turn. New strict + /// manifests set this to one; legacy policy keeps its historical budget. + pub max_reviewer_calls: usize, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -117,6 +121,7 @@ impl Default for DeepReviewExecutionPolicy { reviewer_file_split_threshold: DEFAULT_REVIEWER_FILE_SPLIT_THRESHOLD, max_same_role_instances: DEFAULT_MAX_SAME_ROLE_INSTANCES, max_retries_per_role: DEFAULT_MAX_RETRIES_PER_ROLE, + max_reviewer_calls: DEFAULT_MAX_SAME_ROLE_INSTANCES * reviewer_agent_type_count(), } } } @@ -127,8 +132,18 @@ impl DeepReviewExecutionPolicy { return Self::default(); }; + let extra_subagent_ids = normalize_extra_subagent_ids(config.get("extra_subagent_ids")); + let max_same_role_instances = clamp_usize( + config.get("max_same_role_instances"), + 1, + usize::MAX, + DEFAULT_MAX_SAME_ROLE_INSTANCES, + ); + let legacy_max_reviewer_calls = max_same_role_instances + .saturating_mul(reviewer_agent_type_count().saturating_add(extra_subagent_ids.len())); + Self { - extra_subagent_ids: normalize_extra_subagent_ids(config.get("extra_subagent_ids")), + extra_subagent_ids, strategy_level: DeepReviewStrategyLevel::from_value(config.get("strategy_level")) .unwrap_or_default(), member_strategy_overrides: normalize_member_strategy_overrides( @@ -152,18 +167,19 @@ impl DeepReviewExecutionPolicy { usize::MAX, DEFAULT_REVIEWER_FILE_SPLIT_THRESHOLD, ), - max_same_role_instances: clamp_usize( - config.get("max_same_role_instances"), - 1, - usize::MAX, - DEFAULT_MAX_SAME_ROLE_INSTANCES, - ), + max_same_role_instances, max_retries_per_role: clamp_usize( config.get("max_retries_per_role"), 0, MAX_RETRIES_PER_ROLE, DEFAULT_MAX_RETRIES_PER_ROLE, ), + max_reviewer_calls: clamp_usize( + config.get("max_reviewer_calls"), + 1, + usize::MAX, + legacy_max_reviewer_calls, + ), } } @@ -264,6 +280,7 @@ impl DeepReviewExecutionPolicy { } let mut policy = self.clone(); + let mut has_explicit_specialist_ceiling = false; if let Some(strategy_level) = DeepReviewStrategyLevel::from_value(manifest.get("strategyLevel")) { @@ -301,10 +318,30 @@ impl DeepReviewExecutionPolicy { MAX_RETRIES_PER_ROLE, policy.max_retries_per_role, ); + policy.max_reviewer_calls = if execution_policy.contains_key("maxReviewerCalls") { + has_explicit_specialist_ceiling = true; + clamp_usize( + execution_policy.get("maxReviewerCalls"), + 1, + MAX_STRICT_SPECIALIST_CALLS, + MAX_STRICT_SPECIALIST_CALLS, + ) + } else { + policy.max_reviewer_calls + }; } policy.apply_strategy_runtime_budget(); + if !has_explicit_specialist_ceiling { + // Historical manifests predate the explicit specialist-call + // ceiling. Preserve their effective same-role/extra-member budget + // after all manifest and strategy bounds have been applied. + policy.max_reviewer_calls = policy.max_same_role_instances.saturating_mul( + reviewer_agent_type_count().saturating_add(policy.extra_subagent_ids.len()), + ); + } + policy } @@ -527,7 +564,10 @@ fn number_as_i64(value: &Value) -> Option { #[cfg(test)] mod tests { - use super::{ChangeRiskFactors, DeepReviewExecutionPolicy, DeepReviewStrategyLevel}; + use super::{ + reviewer_agent_type_count, ChangeRiskFactors, DeepReviewExecutionPolicy, + DeepReviewStrategyLevel, + }; use serde_json::json; #[test] @@ -548,6 +588,19 @@ mod tests { ); } + #[test] + fn legacy_config_derives_reviewer_budget_from_roles_and_extra_reviewers() { + let policy = DeepReviewExecutionPolicy::from_config_value(Some(&json!({ + "extra_subagent_ids": ["ReviewDatabase", "ReviewApi"], + "max_same_role_instances": 2 + }))); + + assert_eq!( + policy.max_reviewer_calls, + 2 * (reviewer_agent_type_count() + 2) + ); + } + #[test] fn run_manifest_strategy_applies_builtin_quick_budget_without_execution_policy() { let policy = DeepReviewExecutionPolicy::default(); @@ -602,6 +655,43 @@ mod tests { assert_eq!(effective.max_same_role_instances, 3); } + #[test] + fn strict_manifest_hard_caps_explicit_specialist_budget_to_one() { + let policy = DeepReviewExecutionPolicy::default(); + let manifest = json!({ + "reviewMode": "deep", + "strategyLevel": "deep", + "executionPolicy": { + "maxReviewerCalls": 999 + } + }); + + let effective = policy.with_run_manifest_execution_policy(&manifest); + + assert_eq!(effective.max_reviewer_calls, 1); + } + + #[test] + fn historical_manifest_rederives_budget_after_same_role_override() { + let policy = DeepReviewExecutionPolicy::from_config_value(Some(&json!({ + "extra_subagent_ids": ["ReviewDatabase", "ReviewApi"] + }))); + let manifest = json!({ + "reviewMode": "deep", + "strategyLevel": "deep", + "executionPolicy": { + "maxSameRoleInstances": 2 + } + }); + + let effective = policy.with_run_manifest_execution_policy(&manifest); + + assert_eq!( + effective.max_reviewer_calls, + 2 * (reviewer_agent_type_count() + 2) + ); + } + fn split_policy(threshold: usize) -> DeepReviewExecutionPolicy { DeepReviewExecutionPolicy { reviewer_file_split_threshold: threshold, diff --git a/src/crates/execution/agent-runtime/src/deep_review/manifest.rs b/src/crates/execution/agent-runtime/src/deep_review/manifest.rs index ffea322607..da6083703c 100644 --- a/src/crates/execution/agent-runtime/src/deep_review/manifest.rs +++ b/src/crates/execution/agent-runtime/src/deep_review/manifest.rs @@ -5,7 +5,6 @@ //! compatible with older manifest field spellings and should not silently hide //! reduced coverage, omitted files, or stale evidence hints. -use super::constants::{CONDITIONAL_REVIEWER_AGENT_TYPES, CORE_REVIEWER_AGENT_TYPES}; use super::execution_policy::DeepReviewPolicyViolation; use super::target_evidence::ReviewTargetEvidence; use serde_json::Value; @@ -640,8 +639,7 @@ impl DeepReviewRunManifestGate { Ok(None) => None, Err(error) => Some(error.to_string()), }; - let quality_decision_error = - validate_quality_decision(manifest, &active_subagent_ids, &skipped_subagent_reasons); + let quality_decision_error = validate_quality_decision(manifest, &active_subagent_ids); let (manifest_policy_error_code, manifest_policy_error) = if let Some(error) = target_evidence_error { ("deep_review_target_evidence_invalid", Some(error)) @@ -694,7 +692,6 @@ impl DeepReviewRunManifestGate { fn validate_quality_decision( manifest: &serde_json::Map, active_subagent_ids: &HashSet, - skipped_subagent_reasons: &HashMap, ) -> Option { let decision = manifest .get("qualityDecision") @@ -729,29 +726,8 @@ fn validate_quality_decision( if strategy_level != "deep" { return Some("L3 Review requires the deep strategy".to_string()); } - if quality_gate_id.as_deref() != Some("ReviewJudge") { - return Some("L3 Review requires ReviewJudge as its quality gate".to_string()); - } - let missing_core = CORE_REVIEWER_AGENT_TYPES - .iter() - .filter(|id| !active_subagent_ids.contains(**id)) - .copied() - .collect::>(); - if !missing_core.is_empty() { - return Some(format!( - "L3 Review is missing required core reviewers: {}", - missing_core.join(", ") - )); - } - for reviewer in CONDITIONAL_REVIEWER_AGENT_TYPES { - if !active_subagent_ids.contains(reviewer) - && skipped_subagent_reasons.get(reviewer).map(String::as_str) - != Some("not_applicable") - { - return Some(format!( - "L3 Review must activate {reviewer} or mark it not_applicable" - )); - } + if quality_gate_id.is_some() && quality_gate_id.as_deref() != Some("ReviewJudge") { + return Some("L3 Review quality gate must use ReviewJudge".to_string()); } } _ => return Some(format!("Unsupported Review quality level: {level}")), @@ -841,48 +817,26 @@ mod tests { } #[test] - fn l3_manifest_requires_deep_strategy_and_quality_gate() { + fn l3_manifest_requires_deep_strategy_but_allows_on_demand_delegation() { let manifest = quality_manifest("l3", "normal", &["ReviewBusinessLogic"]); let gate = DeepReviewRunManifestGate::from_value(&manifest).expect("gate should parse"); assert!(gate.ensure_active("ReviewBusinessLogic").is_err()); - let mut incomplete = quality_manifest("l3", "deep", &["ReviewBusinessLogic"]); - incomplete["qualityGateReviewer"] = json!({ "subagentId": "ReviewJudge" }); - let incomplete_gate = - DeepReviewRunManifestGate::from_value(&incomplete).expect("gate should parse"); - assert!(incomplete_gate.ensure_active("ReviewJudge").is_err()); + let single_specialist = quality_manifest("l3", "deep", &["ReviewSecurity"]); + let single_specialist_gate = + DeepReviewRunManifestGate::from_value(&single_specialist).expect("gate should parse"); + assert!(single_specialist_gate + .ensure_active("ReviewSecurity") + .is_ok()); - let mut wrong_judge = quality_manifest( - "l3", - "deep", - &[ - "ReviewBusinessLogic", - "ReviewPerformance", - "ReviewSecurity", - "ReviewArchitecture", - "ReviewFrontend", - ], - ); + let mut wrong_judge = quality_manifest("l3", "deep", &["ReviewSecurity"]); wrong_judge["qualityGateReviewer"] = json!({ "subagentId": "ReviewSecurity" }); let wrong_judge_gate = DeepReviewRunManifestGate::from_value(&wrong_judge).expect("gate should parse"); assert!(wrong_judge_gate.ensure_active("ReviewSecurity").is_err()); - let mut valid = quality_manifest( - "l3", - "deep", - &[ - "ReviewBusinessLogic", - "ReviewPerformance", - "ReviewSecurity", - "ReviewArchitecture", - ], - ); + let mut valid = quality_manifest("l3", "deep", &["ReviewSecurity"]); valid["qualityGateReviewer"] = json!({ "subagentId": "ReviewJudge" }); - valid["skippedReviewers"] = json!([{ - "subagentId": "ReviewFrontend", - "reason": "not_applicable" - }]); let valid_gate = DeepReviewRunManifestGate::from_value(&valid).expect("gate should parse"); assert!(valid_gate.ensure_active("ReviewJudge").is_ok()); } diff --git a/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.test.tsx b/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.test.tsx index b2de1bacbc..acb009e162 100644 --- a/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.test.tsx +++ b/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.test.tsx @@ -299,13 +299,13 @@ describeWithJsdom('DeepReviewConsentDialog', () => { expect(container.textContent).toContain('BitFun selected the most relevant checks for this target.'); expect(container.textContent).not.toContain('Estimated reviewer prompt input'); expect(container.textContent).not.toContain('Reviewer prompt input only'); - expect(container.textContent).toContain('Independent checks: 3 planned calls'); - expect(container.textContent).toContain('Up to 3 calls can run at the same time.'); + expect(container.textContent).toContain('Independent checks: 3 planned review agent run'); + expect(container.textContent).toContain('Up to 4 review agent runs may occur without another confirmation.'); expect(container.textContent).not.toContain('up to 4 initial calls'); expect(container.textContent).toContain('Run strategy: Standard'); expect(container.textContent).not.toContain('Do not show this again'); expect(container.textContent).not.toContain('Risk areas: Backend core'); - expect(container.textContent).toContain('Planned independent reviewer calls; token use is not estimated here.'); + expect(container.textContent).toContain('Planned review agent runs; model requests and token use are not estimated here.'); expect(container.textContent).not.toContain('1 extra specialist'); expect(container.textContent).not.toContain('Review depth: Risk-expanded'); expect(container.textContent).not.toContain('Frontend reviewer'); @@ -344,7 +344,7 @@ describeWithJsdom('DeepReviewConsentDialog', () => { expect(container.textContent).toContain('Provided context'); expect(container.textContent).not.toContain('0 files'); expect(container.textContent).not.toContain('Risk areas:'); - expect(container.textContent).toContain('Planned independent reviewer calls; token use is not estimated here.'); + expect(container.textContent).toContain('Planned review agent runs; model requests and token use are not estimated here.'); }); it('still opens when skip preference is set but reviewers are skipped', async () => { @@ -426,7 +426,7 @@ describeWithJsdom('DeepReviewConsentDialog', () => { expect(container.querySelectorAll('.deep-review-consent__strategy-heading')).toHaveLength(0); expect(container.textContent).not.toContain('Quick is narrower'); expect(container.textContent).not.toContain('Risk areas: Backend core'); - expect(container.textContent).toContain('Planned independent reviewer calls; token use is not estimated here.'); + expect(container.textContent).toContain('Planned review agent runs; model requests and token use are not estimated here.'); expect(container.textContent).not.toContain('1 extra specialist'); expect(container.textContent).not.toContain('Expected cost:'); expect(container.querySelectorAll('.deep-review-consent__strategy-selected-summary')).toHaveLength(0); diff --git a/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.tsx b/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.tsx index 45e7d5b911..3121e9486a 100644 --- a/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.tsx +++ b/src/web-ui/src/flow_chat/components/DeepReviewConsentDialog.tsx @@ -30,12 +30,12 @@ export interface DeepReviewConsentControls { function getInitialReviewCallFacts(preview: ReviewTeamRunManifest): { planned: number; - parallel: number; + maximum: number; } { const planned = Math.max(1, preview.tokenBudget.estimatedReviewerCalls || 1); return { planned, - parallel: Math.max(1, Math.min(planned, preview.concurrencyPolicy.maxParallelInstances)), + maximum: Math.max(planned, preview.tokenBudget.maxReviewerCalls || planned), }; } @@ -165,7 +165,7 @@ export function useDeepReviewConsent(): DeepReviewConsentControls { {t('deepReviewConsent.parallelCalls', { - count: callFacts.parallel, + count: callFacts.maximum, })} diff --git a/src/web-ui/src/locales/en-US/flow-chat.json b/src/web-ui/src/locales/en-US/flow-chat.json index 393fe7c598..6f2c04a9f1 100644 --- a/src/web-ui/src/locales/en-US/flow-chat.json +++ b/src/web-ui/src/locales/en-US/flow-chat.json @@ -973,13 +973,13 @@ "sessionConcurrencyTitle": "Active session is busy", "sessionConcurrencyBody": "The target session already has {{count}} review tasks running. Continuing will share the available review capacity.", "costLabel": "Coverage", - "cost": "Planned independent reviewer calls; token use is not estimated here.", + "cost": "Planned review agent runs; model requests and token use are not estimated here.", "timeLabel": "Time", "time": "Runs in background and may take longer.", "cancel": "Cancel", "confirm": "Start review", - "initialCalls": "Independent checks: {{planned}} planned calls", - "parallelCalls": "Up to {{count}} calls can run at the same time.", + "initialCalls": "Independent checks: {{planned}} planned review agent run", + "parallelCalls": "Up to {{count}} review agent runs may occur without another confirmation.", "runStrategy": "Run strategy: {{strategy}}", "strategyLabels": { "quick": "Focused", diff --git a/src/web-ui/src/locales/zh-CN/flow-chat.json b/src/web-ui/src/locales/zh-CN/flow-chat.json index 4649a5716d..e6e6441770 100644 --- a/src/web-ui/src/locales/zh-CN/flow-chat.json +++ b/src/web-ui/src/locales/zh-CN/flow-chat.json @@ -973,13 +973,13 @@ "sessionConcurrencyTitle": "当前会话较忙", "sessionConcurrencyBody": "目标会话已有 {{count}} 个审核工作在运行，继续后将共享当前可用的审核容量。", "costLabel": "覆盖", - "cost": "计划的独立审核调用；此处不提供无法证明的 Token 估算。", + "cost": "计划的审查代理执行；此处不估算底层模型请求或 Token。", "timeLabel": "耗时", "time": "后台运行，可能需要更久。", "cancel": "取消", "confirm": "开始审核", - "initialCalls": "独立检查：计划 {{planned}} 次调用", - "parallelCalls": "最多同时运行 {{count}} 次调用。", + "initialCalls": "独立检查：计划 {{planned}} 次审查代理执行", + "parallelCalls": "无需再次确认时，最多可能执行 {{count}} 次审查代理。", "runStrategy": "运行策略：{{strategy}}", "strategyLabels": { "quick": "聚焦", diff --git a/src/web-ui/src/locales/zh-TW/flow-chat.json b/src/web-ui/src/locales/zh-TW/flow-chat.json index a6b4137c6b..24aab2d2c8 100644 --- a/src/web-ui/src/locales/zh-TW/flow-chat.json +++ b/src/web-ui/src/locales/zh-TW/flow-chat.json @@ -973,13 +973,13 @@ "sessionConcurrencyTitle": "目前會話較忙", "sessionConcurrencyBody": "目標會話已有 {{count}} 個審核工作在執行，繼續後將共享目前可用的審核容量。", "costLabel": "覆蓋", - "cost": "計畫的獨立審核呼叫；此處不提供無法證明的 Token 估算。", + "cost": "計畫的審查代理執行；此處不估算底層模型請求或 Token。", "timeLabel": "耗時", "time": "背景執行，可能需要更久。", "cancel": "取消", "confirm": "開始審核", - "initialCalls": "獨立檢查：計畫 {{planned}} 次呼叫", - "parallelCalls": "最多同時執行 {{count}} 次呼叫。", + "initialCalls": "獨立檢查：計畫 {{planned}} 次審查代理執行", + "parallelCalls": "無需再次確認時，最多可能執行 {{count}} 次審查代理。", "runStrategy": "運行策略：{{strategy}}", "strategyLabels": { "quick": "聚焦", diff --git a/src/web-ui/src/shared/services/review-team/defaults.ts b/src/web-ui/src/shared/services/review-team/defaults.ts index c44ade6403..91d161c51e 100644 --- a/src/web-ui/src/shared/services/review-team/defaults.ts +++ b/src/web-ui/src/shared/services/review-team/defaults.ts @@ -110,23 +110,6 @@ export const REVIEW_WORK_PACKET_ALLOWED_TOOLS = [ 'LS', ] as const; -export const REVIEWER_WORK_PACKET_REQUIRED_OUTPUT_FIELDS = [ - 'packet_id', - 'status', - 'verdict', - 'findings', - 'reviewer_summary', -] as const; - -export const JUDGE_WORK_PACKET_REQUIRED_OUTPUT_FIELDS = [ - 'packet_id', - 'status', - 'decision_summary', - 'validated_findings', - 'rejected_or_downgraded_notes', - 'coverage_notes', -] as const; - export const DEFAULT_REVIEW_TEAM_CORE_ROLES: ReviewTeamCoreRoleDefinition[] = [ { key: 'businessLogic', @@ -229,7 +212,7 @@ export const FALLBACK_REVIEW_TEAM_DEFINITION: ReviewTeamDefinition = { id: DEFAULT_REVIEW_TEAM_ID, name: 'Strict Review Coverage', description: - 'A multi-reviewer coverage plan for strict code review with mandatory logic, performance, security, architecture, conditional frontend, and quality-gate roles.', + 'A strict code-review policy where the primary reviewer works directly and may request one focused specialist or a conditional quality check.', warning: 'Strict review may take longer and usually consumes more tokens than a standard review.', defaultModel: DEFAULT_REVIEW_TEAM_MODEL, diff --git a/src/web-ui/src/shared/services/review-team/index.ts b/src/web-ui/src/shared/services/review-team/index.ts index b2782f7027..418ab27b0b 100644 --- a/src/web-ui/src/shared/services/review-team/index.ts +++ b/src/web-ui/src/shared/services/review-team/index.ts @@ -54,7 +54,6 @@ import { buildTokenBudgetPlan, } from './tokenBudget'; import { - buildWorkPackets, resolveChangeStats, resolveMaxExtraReviewers, } from './workPackets'; @@ -79,6 +78,7 @@ import type { ReviewTeamRateLimitStatus, ReviewTeamRunManifest, ReviewTeamStoredConfig, + ReviewTeamWorkPacket, ReviewTokenBudgetMode, } from './types'; @@ -1332,20 +1332,21 @@ export function buildEffectiveReviewTeamManifest( const budgetLimitedExtraMembers = eligibleExtraMembers.slice(maxExtraReviewers); const enabledExtraReviewers = enabledExtraMembers .map((member) => toManifestMember(member)); - const executionPolicy = buildEffectiveExecutionPolicy({ - basePolicy: team.executionPolicy, - strategyLevel, - target, - changeStats, - }); - const workPackets = buildWorkPackets({ - reviewerMembers: [...coreReviewerMembers, ...enabledExtraMembers], - judgeMember: qualityGateReviewerMember, - target, - executionPolicy, - concurrencyPolicy, - targetEvidence: options.targetEvidence, - }); + const executionPolicy = { + ...buildEffectiveExecutionPolicy({ + basePolicy: team.executionPolicy, + strategyLevel, + target, + changeStats, + }), + // A strict run is reviewed by the DeepReview agent itself. Specialist + // agents are optional fresh perspectives, not a pre-scheduled team. + reviewerFileSplitThreshold: 0, + maxSameRoleInstances: 1, + maxRetriesPerRole: 0, + maxReviewerCalls: 1, + }; + const workPackets: ReviewTeamWorkPacket[] = []; const evidencePack = buildDeepReviewEvidencePack({ target, changeStats, @@ -1355,7 +1356,10 @@ export function buildEffectiveReviewTeamManifest( }); const tokenBudget = buildTokenBudgetPlan({ mode: tokenBudgetMode, - activeReviewerCalls: workPackets.length, + // One primary DeepReview agent execution is guaranteed. At most one + // specialist and one conditional quality-inspector execution may follow. + activeReviewerCalls: 1, + maxReviewerCalls: 3, eligibleExtraReviewerCount: eligibleExtraMembers.length, maxExtraReviewers, skippedReviewerIds: budgetLimitedExtraMembers.map((member) => member.subagentId), diff --git a/src/web-ui/src/shared/services/review-team/promptBlock.ts b/src/web-ui/src/shared/services/review-team/promptBlock.ts index e28933496c..14e1795f03 100644 --- a/src/web-ui/src/shared/services/review-team/promptBlock.ts +++ b/src/web-ui/src/shared/services/review-team/promptBlock.ts @@ -113,6 +113,15 @@ export function buildReviewTeamPromptBlockContent( manifest: ReviewTeamRunManifest, ): string { const executionPlan = compactExecutionPlan(manifest.workPackets); + const hasLegacyPackets = executionPlan.active_packets.length > 0; + const specialistPool = [ + ...manifest.coreReviewers, + ...manifest.enabledExtraReviewers, + ].map((member) => ({ + subagent_type: member.subagentId, + role: member.roleName, + model_id: member.model, + })); const compactManifest = { review_mode: manifest.reviewMode, selected_strategy: manifest.strategyLevel, @@ -121,6 +130,9 @@ export function buildReviewTeamPromptBlockContent( resolution: manifest.target.resolution, tags: manifest.target.tags, file_count: manifest.changeStats?.fileCount ?? manifest.target.files.length, + files: manifest.target.files + .filter((file) => !file.excluded) + .map((file) => file.normalizedPath), changed_line_count: manifest.changeStats?.totalLinesChanged ?? null, changed_line_count_source: manifest.changeStats?.lineCountSource ?? 'unknown', }, @@ -134,27 +146,60 @@ export function buildReviewTeamPromptBlockContent( } : null, execution: { - max_parallel_instances: manifest.concurrencyPolicy.maxParallelInstances, - max_retries_per_role: manifest.executionPolicy.maxRetriesPerRole, + ...(hasLegacyPackets + ? { + max_parallel_instances: manifest.concurrencyPolicy.maxParallelInstances, + max_retries_per_role: manifest.executionPolicy.maxRetriesPerRole, + } + : { + max_specialist_calls: manifest.executionPolicy.maxReviewerCalls ?? 1, + max_review_agent_executions: manifest.tokenBudget.maxReviewerCalls, + specialist_timeout_seconds: manifest.executionPolicy.reviewerTimeoutSeconds, + quality_inspector_timeout_seconds: manifest.executionPolicy.judgeTimeoutSeconds, + }), }, + specialist_pool: specialistPool, + quality_inspector: manifest.qualityGateReviewer + ? { + subagent_type: manifest.qualityGateReviewer.subagentId, + role: manifest.qualityGateReviewer.roleName, + model_id: manifest.qualityGateReviewer.model, + } + : null, ...executionPlan, }; - return [ + const rules = [ 'Prepared Review execution plan (target already resolved):', '```json', JSON.stringify(compactManifest, null, 2), '```', 'Execution rules:', '- Do not reinterpret, widen, or replace the prepared target.', - '- Launch only active_packets, in launch_batch order, and never exceed max_parallel_instances.', - '- Build each reviewer prompt from its active packet plus the referenced scope_group; do not repeat unrelated scopes or policies.', - '- Stay within allowed_tools and the referenced scope. Read one-hop context only when required to verify a concrete finding.', - '- Run a judge packet only after all reviewer packets finish.', - '- Every result must report packet_id and status. Infer missing packet_id only from the scheduled packet and mark it inferred.', - '- Retry a failed or timed-out role only when evidence is still missing and within max_retries_per_role.', '- Partial, unknown, stale, omitted, or exhausted evidence must remain an explicit coverage limitation.', '- Remain read-only. Do not launch ReviewFixer or start remediation without explicit user approval.', - '- Submit one structured final report after the active plan completes.', - ].join('\n'); + ]; + + if (hasLegacyPackets) { + rules.push( + 'Legacy packet compatibility:', + '- Launch only active_packets, in launch_batch order, and never exceed max_parallel_instances.', + '- Build each reviewer prompt from its packet and referenced scope_group; stay within allowed_tools and the assigned scope.', + '- Run a judge packet only after all reviewer packets finish.', + '- Retry only when evidence is still missing and within max_retries_per_role; do not invent additional packets.', + '- Every packet result must report packet_id and status; preserve missing or inferred packet state in coverage notes.', + '- Submit one structured final report after the historical packet plan completes.', + ); + } else { + rules.push( + '- Review the prepared target directly before considering delegation.', + '- Launch at most one specialist, and only for a concrete uncertainty where a fresh focused pass can materially improve the result.', + '- Do not use a specialist to repeat the primary review, divide files, or provide routine role coverage.', + '- Run the quality inspector only when a high-severity finding, conflicting evidence, or low-confidence conclusion needs independent validation.', + '- If no specialist or quality inspector is needed, complete the report directly.', + '- Submit one structured final report after review and any justified validation complete.', + ); + } + + return rules.join('\n'); } diff --git a/src/web-ui/src/shared/services/review-team/tokenBudget.ts b/src/web-ui/src/shared/services/review-team/tokenBudget.ts index 6bfeb50382..b073b30d34 100644 --- a/src/web-ui/src/shared/services/review-team/tokenBudget.ts +++ b/src/web-ui/src/shared/services/review-team/tokenBudget.ts @@ -72,6 +72,7 @@ export function buildEffectiveExecutionPolicy(params: { export function buildTokenBudgetPlan(params: { mode: ReviewTokenBudgetMode; activeReviewerCalls: number; + maxReviewerCalls?: number; eligibleExtraReviewerCount: number; maxExtraReviewers: number; skippedReviewerIds: string[]; @@ -106,7 +107,7 @@ export function buildTokenBudgetPlan(params: { return { mode: params.mode, estimatedReviewerCalls: params.activeReviewerCalls, - maxReviewerCalls: params.activeReviewerCalls, + maxReviewerCalls: params.maxReviewerCalls ?? params.activeReviewerCalls, maxExtraReviewers: params.maxExtraReviewers, ...(fileSplitGuardrailActive ? { maxFilesPerReviewer: params.executionPolicy.reviewerFileSplitThreshold } diff --git a/src/web-ui/src/shared/services/review-team/types.ts b/src/web-ui/src/shared/services/review-team/types.ts index 18bc602145..fa74a60456 100644 --- a/src/web-ui/src/shared/services/review-team/types.ts +++ b/src/web-ui/src/shared/services/review-team/types.ts @@ -205,6 +205,8 @@ export interface ReviewTeamExecutionPolicy { reviewerFileSplitThreshold: number; maxSameRoleInstances: number; maxRetriesPerRole: number; + /** Maximum optional specialist launches for a new strict-review turn. */ + maxReviewerCalls?: number; } export interface ReviewTeamConcurrencyPolicy { diff --git a/src/web-ui/src/shared/services/review-team/workPackets.ts b/src/web-ui/src/shared/services/review-team/workPackets.ts index 666fd55a16..0bd4b6a927 100644 --- a/src/web-ui/src/shared/services/review-team/workPackets.ts +++ b/src/web-ui/src/shared/services/review-team/workPackets.ts @@ -1,25 +1,12 @@ import type { ReviewTargetClassification } from '../reviewTargetClassifier'; -import { - DEFAULT_REVIEW_TEAM_MODEL, - JUDGE_WORK_PACKET_REQUIRED_OUTPUT_FIELDS, - REVIEWER_WORK_PACKET_REQUIRED_OUTPUT_FIELDS, -} from './defaults'; -import { toManifestMember } from './manifestMembers'; -import { groupFilesByWorkspaceArea } from './pathMetadata'; import type { ReviewTeamChangeStats, - ReviewTeamConcurrencyPolicy, - ReviewTeamExecutionPolicy, - ReviewTeamMember, - ReviewTeamWorkPacket, - ReviewTeamWorkPacketScope, ReviewTokenBudgetMode, - ReviewTargetEvidence, } from './types'; -import { allowsReviewLiveRepositoryContext } from './targetEvidence'; -// Work packets are pure launch-plan metadata. They must not inspect file -// contents or make runtime retry/queue decisions. +// Legacy manifests may still contain work packets, but new strict reviews do +// not pre-schedule reviewer calls. This module now retains only the small +// normalization helpers used while building a launch manifest. export function resolveMaxExtraReviewers( mode: ReviewTokenBudgetMode, eligibleExtraReviewerCount: number, @@ -57,273 +44,3 @@ export function resolveChangeStats( : 'unknown', }; } - -function buildWorkPacketScopeFromFiles( - target: ReviewTargetClassification, - files: string[], - group?: { index: number; count: number }, -): ReviewTeamWorkPacketScope { - return { - kind: 'review_target', - targetSource: target.source, - targetResolution: target.resolution, - targetTags: [...target.tags], - fileCount: files.length, - files, - excludedFileCount: - target.files.length - target.files.filter((file) => !file.excluded).length, - ...(group ? { groupIndex: group.index, groupCount: group.count } : {}), - }; -} - -function buildWorkPacket(params: { - member: ReviewTeamMember; - phase: ReviewTeamWorkPacket['phase']; - launchBatch: number; - scope: ReviewTeamWorkPacketScope; - timeoutSeconds: number; - targetEvidence?: ReviewTargetEvidence; -}): ReviewTeamWorkPacket { - const manifestMember = toManifestMember(params.member); - const liveRepositoryContextAllowed = !params.targetEvidence || - params.targetEvidence.source === 'workspace' || - allowsReviewLiveRepositoryContext(params.targetEvidence); - const liveRepositoryTools = new Set(['Read', 'Grep', 'Glob', 'LS']); - const packetGroupSuffix = - params.phase === 'reviewer' && - params.scope.groupIndex !== undefined && - params.scope.groupCount !== undefined - ? `:group-${params.scope.groupIndex}-of-${params.scope.groupCount}` - : ''; - - return { - packetId: `${params.phase}:${manifestMember.subagentId}${packetGroupSuffix}`, - phase: params.phase, - launchBatch: params.launchBatch, - subagentId: manifestMember.subagentId, - displayName: manifestMember.displayName, - roleName: manifestMember.roleName, - assignedScope: params.scope, - allowedTools: params.member.allowedTools.filter((tool) => - liveRepositoryContextAllowed || !liveRepositoryTools.has(tool)), - timeoutSeconds: params.timeoutSeconds, - requiredOutputFields: - params.phase === 'judge' - ? [...JUDGE_WORK_PACKET_REQUIRED_OUTPUT_FIELDS] - : [...REVIEWER_WORK_PACKET_REQUIRED_OUTPUT_FIELDS], - strategyLevel: manifestMember.strategyLevel, - strategyDirective: manifestMember.strategyDirective, - model: manifestMember.model || DEFAULT_REVIEW_TEAM_MODEL, - }; -} - -function splitFilesIntoGroups(files: string[], groupCount: number): string[][] { - if (groupCount <= 1) { - return [files]; - } - - const groups: string[][] = []; - let cursor = 0; - for (let index = 0; index < groupCount; index += 1) { - const remainingFiles = files.length - cursor; - const remainingGroups = groupCount - index; - const groupSize = Math.ceil(remainingFiles / remainingGroups); - groups.push(files.slice(cursor, cursor + groupSize)); - cursor += groupSize; - } - return groups; -} - -function splitFilesIntoModuleAwareGroups( - files: string[], - groupCount: number, -): string[][] { - if (groupCount <= 1) { - return [files]; - } - - const buckets = groupFilesByWorkspaceArea(files); - if (buckets.length <= 1) { - return splitFilesIntoGroups(files, groupCount); - } - - if (buckets.length >= groupCount) { - const groups = Array.from({ length: groupCount }, () => [] as string[]); - const sortedBuckets = [...buckets].sort( - (a, b) => b.files.length - a.files.length || a.index - b.index, - ); - - for (const bucket of sortedBuckets) { - let targetIndex = 0; - for (let index = 1; index < groups.length; index += 1) { - if (groups[index].length < groups[targetIndex].length) { - targetIndex = index; - } - } - groups[targetIndex].push(...bucket.files); - } - - return groups.filter((group) => group.length > 0); - } - - const chunkCounts = buckets.map(() => 1); - let remainingChunks = groupCount - buckets.length; - while (remainingChunks > 0) { - let targetBucketIndex = -1; - let largestAverageChunkSize = 0; - - for (let index = 0; index < buckets.length; index += 1) { - if (chunkCounts[index] >= buckets[index].files.length) { - continue; - } - const averageChunkSize = buckets[index].files.length / chunkCounts[index]; - if (averageChunkSize > largestAverageChunkSize) { - largestAverageChunkSize = averageChunkSize; - targetBucketIndex = index; - } - } - - if (targetBucketIndex === -1) { - break; - } - - chunkCounts[targetBucketIndex] += 1; - remainingChunks -= 1; - } - - return buckets.flatMap((bucket, index) => - splitFilesIntoGroups(bucket.files, chunkCounts[index]), - ); -} - -function effectiveMaxSameRoleInstances(params: { - executionPolicy: ReviewTeamExecutionPolicy; - concurrencyPolicy: ReviewTeamConcurrencyPolicy; - reviewerMemberCount: number; -}): number { - const reviewerMemberCount = Math.max(1, params.reviewerMemberCount); - const maxPerRole = Math.floor( - params.concurrencyPolicy.maxParallelInstances / reviewerMemberCount, - ); - - return Math.max( - 1, - Math.min(params.executionPolicy.maxSameRoleInstances, Math.max(1, maxPerRole)), - ); -} - -function resolveReviewerPacketScopes( - target: ReviewTargetClassification, - executionPolicy: ReviewTeamExecutionPolicy, - concurrencyPolicy: ReviewTeamConcurrencyPolicy, - reviewerMemberCount: number, -): ReviewTeamWorkPacketScope[] { - const includedFiles = target.files - .filter((file) => !file.excluded) - .map((file) => file.normalizedPath); - const shouldSplit = - executionPolicy.reviewerFileSplitThreshold > 0 && - executionPolicy.maxSameRoleInstances > 1 && - includedFiles.length > executionPolicy.reviewerFileSplitThreshold; - - if (!shouldSplit) { - return [buildWorkPacketScopeFromFiles(target, includedFiles)]; - } - - const maxSameRoleInstances = effectiveMaxSameRoleInstances({ - executionPolicy, - concurrencyPolicy, - reviewerMemberCount, - }); - const groupCount = Math.min( - maxSameRoleInstances, - Math.ceil(includedFiles.length / executionPolicy.reviewerFileSplitThreshold), - ); - if (groupCount <= 1) { - return [buildWorkPacketScopeFromFiles(target, includedFiles)]; - } - - const fileGroups = splitFilesIntoModuleAwareGroups(includedFiles, groupCount); - return fileGroups.map((files, index) => - buildWorkPacketScopeFromFiles(target, files, { - index: index + 1, - count: fileGroups.length, - }), - ); -} - -export function buildWorkPackets(params: { - reviewerMembers: ReviewTeamMember[]; - judgeMember?: ReviewTeamMember; - target: ReviewTargetClassification; - executionPolicy: ReviewTeamExecutionPolicy; - concurrencyPolicy: ReviewTeamConcurrencyPolicy; - targetEvidence?: ReviewTargetEvidence; -}): ReviewTeamWorkPacket[] { - const reviewerScopes = resolveReviewerPacketScopes( - params.target, - params.executionPolicy, - params.concurrencyPolicy, - params.reviewerMembers.length, - ); - const fullScope = buildWorkPacketScopeFromFiles( - params.target, - params.target.files - .filter((file) => !file.excluded) - .map((file) => file.normalizedPath), - ); - const reviewerSeeds = params.reviewerMembers.flatMap((member) => - reviewerScopes.map((scope) => ({ member, scope })), - ); - const buildReviewerPacketsForSeeds = ( - seeds: typeof reviewerSeeds, - firstLaunchBatch: number, - ): ReviewTeamWorkPacket[] => seeds.map((seed, index) => - buildWorkPacket({ - member: seed.member, - phase: 'reviewer', - launchBatch: - firstLaunchBatch + - Math.floor(index / params.concurrencyPolicy.maxParallelInstances), - scope: seed.scope, - timeoutSeconds: params.executionPolicy.reviewerTimeoutSeconds, - targetEvidence: params.targetEvidence, - }), - ); - const reviewerPackets = params.concurrencyPolicy.batchExtrasSeparately - ? (() => { - const coreReviewerPackets = buildReviewerPacketsForSeeds( - reviewerSeeds.filter((seed) => seed.member.source === 'core'), - 1, - ); - const extraFirstLaunchBatch = coreReviewerPackets.length > 0 - ? Math.max(...coreReviewerPackets.map((packet) => packet.launchBatch)) + 1 - : 1; - return [ - ...coreReviewerPackets, - ...buildReviewerPacketsForSeeds( - reviewerSeeds.filter((seed) => seed.member.source === 'extra'), - extraFirstLaunchBatch, - ), - ]; - })() - : buildReviewerPacketsForSeeds(reviewerSeeds, 1); - const finalReviewerBatch = reviewerPackets.reduce( - (maxBatch, packet) => Math.max(maxBatch, packet.launchBatch), - 0, - ); - const judgePacket = params.judgeMember - ? [ - buildWorkPacket({ - member: params.judgeMember, - phase: 'judge', - launchBatch: finalReviewerBatch + 1, - scope: fullScope, - timeoutSeconds: params.executionPolicy.judgeTimeoutSeconds, - targetEvidence: params.targetEvidence, - }), - ] - : []; - - return [...reviewerPackets, ...judgePacket]; -} diff --git a/src/web-ui/src/shared/services/reviewTeamService.test.ts b/src/web-ui/src/shared/services/reviewTeamService.test.ts index e092ce7f4a..fc742ad945 100644 --- a/src/web-ui/src/shared/services/reviewTeamService.test.ts +++ b/src/web-ui/src/shared/services/reviewTeamService.test.ts @@ -347,7 +347,7 @@ describe('reviewTeamService', () => { expect(promptBlock).toContain('"subagent_type": "ExtraEnabled"'); expect(promptBlock).not.toContain('"subagent_type": "ExtraDisabled"'); - expect(promptBlock).toContain('Launch only active_packets'); + expect(promptBlock).toContain('Launch at most one specialist'); }); it('can resolve the team from a backend-provided reviewer definition', () => { @@ -683,6 +683,85 @@ describe('reviewTeamService', () => { expect(manifest.strategyLevel).toBe('deep'); expect(manifest.qualityDecision).toEqual({ level: 'l3' }); expect(manifest.qualityGateReviewer?.subagentId).toBe('ReviewJudge'); + expect(manifest.workPackets).toEqual([]); + expect(manifest.executionPolicy).toMatchObject({ + reviewerFileSplitThreshold: 0, + maxSameRoleInstances: 1, + maxRetriesPerRole: 0, + maxReviewerCalls: 1, + }); + expect(manifest.tokenBudget).toMatchObject({ + estimatedReviewerCalls: 1, + maxReviewerCalls: 3, + }); + + const promptBlock = buildReviewTeamPromptBlock(team, manifest); + expect(promptBlock).toContain('Review the prepared target directly before considering delegation.'); + expect(promptBlock).toContain('Launch at most one specialist'); + expect(promptBlock).toContain('Run the quality inspector only'); + expect(promptBlock).toContain('"max_review_agent_executions": 3'); + expect(promptBlock).not.toContain('max_total_model_calls'); + expect(promptBlock).not.toContain('Launch only active_packets'); + }); + + it('keeps historical packet dispatch rules without applying the new strict ceiling', () => { + const team = resolveDefaultReviewTeam(coreSubagents(), storedConfigWithExtra()); + const manifest = buildEffectiveReviewTeamManifest(team, { + strategyOverride: 'deep', + qualityDecision: { level: 'l3' }, + }); + const assignedScope = { + kind: 'review_target' as const, + targetSource: 'workspace_diff' as const, + targetResolution: 'resolved' as const, + targetTags: [], + fileCount: 1, + files: ['src/lib.rs'], + excludedFileCount: 0, + }; + manifest.executionPolicy.maxRetriesPerRole = 1; + manifest.workPackets = [ + { + packetId: 'legacy-logic', + phase: 'reviewer', + launchBatch: 0, + subagentId: 'ReviewBusinessLogic', + displayName: 'Logic reviewer', + roleName: 'Business Logic Reviewer', + assignedScope, + allowedTools: ['GetFileDiff'], + timeoutSeconds: 1200, + requiredOutputFields: ['issues'], + strategyLevel: 'deep', + strategyDirective: 'Review behavior.', + model: 'fast', + }, + { + packetId: 'legacy-security', + phase: 'reviewer', + launchBatch: 0, + subagentId: 'ReviewSecurity', + displayName: 'Security reviewer', + roleName: 'Security Reviewer', + assignedScope, + allowedTools: ['GetFileDiff'], + timeoutSeconds: 1200, + requiredOutputFields: ['issues'], + strategyLevel: 'deep', + strategyDirective: 'Review trust boundaries.', + model: 'fast', + }, + ]; + + const promptBlock = buildReviewTeamPromptBlock(team, manifest); + + expect(promptBlock).toContain('Launch only active_packets'); + expect(promptBlock).toContain('"max_parallel_instances": 4'); + expect(promptBlock).toContain('"max_retries_per_role": 1'); + expect(promptBlock).toContain('"packet_id": "legacy-logic"'); + expect(promptBlock).toContain('"packet_id": "legacy-security"'); + expect(promptBlock).not.toContain('Launch at most one specialist'); + expect(promptBlock).not.toContain('Do not use a specialist to repeat'); }); it('keeps changed-file coverage metadata visible for focused-scope profiles', () => { @@ -702,16 +781,8 @@ describe('reviewTeamService', () => { expect(manifest.scopeProfile.reviewDepth).toBe('high_risk_only'); expect(manifest.target.files.map((file) => file.normalizedPath)).toEqual(files); - expect( - manifest.workPackets - ?.filter((packet) => packet.phase === 'reviewer') - .every((packet) => packet.assignedScope.files.every((file) => files.includes(file))), - ).toBe(true); - expect( - manifest.workPackets - ?.filter((packet) => packet.phase === 'reviewer') - .some((packet) => files.every((file) => packet.assignedScope.files.includes(file))), - ).toBe(true); + expect(manifest.evidencePack?.changedFiles).toEqual(files); + expect(manifest.workPackets).toEqual([]); }); it('includes focused-scope profile guidance in the prompt block', () => { @@ -730,75 +801,6 @@ describe('reviewTeamService', () => { expect(promptBlock).toContain('evidence must remain an explicit coverage limitation'); }); - it('generates structured work packets for active reviewers and the judge', () => { - const team = resolveDefaultReviewTeam( - [ - ...coreSubagents(), - subagent('ExtraEnabled', true, 'user', 'fast', true, true), - ], - storedConfigWithExtra(['ExtraEnabled']), - ); - const target = classifyReviewTargetFromFiles( - ['src/web-ui/src/components/ReviewPanel.tsx'], - 'session_files', - ); - - const manifest = buildEffectiveReviewTeamManifest(team, { - workspacePath: WORKSPACE_PATH, - target, - }); - - const logicPacket = manifest.workPackets?.find( - (packet) => packet.subagentId === 'ReviewBusinessLogic', - ); - const judgePacket = manifest.workPackets?.find( - (packet) => packet.subagentId === 'ReviewJudge', - ); - - expect(logicPacket).toMatchObject({ - packetId: 'reviewer:ReviewBusinessLogic', - phase: 'reviewer', - subagentId: 'ReviewBusinessLogic', - roleName: 'Business Logic Reviewer', - assignedScope: { - kind: 'review_target', - fileCount: 1, - files: ['src/web-ui/src/components/ReviewPanel.tsx'], - }, - allowedTools: ['GetFileDiff', 'Read', 'Grep', 'Glob', 'LS'], - timeoutSeconds: manifest.executionPolicy.reviewerTimeoutSeconds, - requiredOutputFields: expect.arrayContaining([ - 'packet_id', - 'status', - 'findings', - ]), - }); - expect(judgePacket).toMatchObject({ - packetId: 'judge:ReviewJudge', - phase: 'judge', - subagentId: 'ReviewJudge', - timeoutSeconds: manifest.executionPolicy.judgeTimeoutSeconds, - requiredOutputFields: expect.arrayContaining([ - 'packet_id', - 'status', - 'validated_findings', - ]), - }); - expect(manifest.workPackets?.map((packet) => packet.subagentId)).not.toContain( - 'ExtraDisabled', - ); - expect(manifest.executionPolicy.maxRetriesPerRole).toBe(1); - - const promptBlock = buildReviewTeamPromptBlock(team, manifest); - expect(promptBlock).toContain('"active_packets"'); - expect(promptBlock).toContain('"packet_id": "reviewer:ReviewBusinessLogic"'); - expect(promptBlock).toContain('"allowed_tools"'); - expect(promptBlock).toContain('"max_retries_per_role": 1'); - expect(promptBlock).toContain('Retry a failed or timed-out role only when evidence is still missing'); - expect(promptBlock).toContain('Build each reviewer prompt from its active packet plus the referenced scope_group'); - expect(promptBlock).toContain('Infer missing packet_id only from the scheduled packet and mark it inferred.'); - }); - it('intersects extra reviewer tools with the read-only review tool set', () => { const team = resolveDefaultReviewTeam( [ @@ -984,10 +986,7 @@ describe('reviewTeamService', () => { lineCountSource: 'diff_stat', }, riskFocusTags: manifest.scopeProfile?.riskFocusTags, - packetIds: expect.arrayContaining([ - 'reviewer:ReviewBusinessLogic', - 'judge:ReviewJudge', - ]), + packetIds: [], privacy: { content: 'metadata_only', }, @@ -1084,260 +1083,6 @@ describe('reviewTeamService', () => { expect(promptBlock).not.toContain('incremental_review_cache'); }); - it('splits reviewer work packets across file groups for large targets', () => { - const team = resolveDefaultReviewTeam( - coreSubagents(), - storedConfigWithExtra([], { - reviewer_file_split_threshold: 10, - max_same_role_instances: 3, - }), - ); - const target = classifyReviewTargetFromFiles( - Array.from( - { length: 25 }, - (_, index) => `src/web-ui/src/components/ReviewPanel${index}.tsx`, - ), - 'session_files', - ); - - const manifest = buildEffectiveReviewTeamManifest(team, { - target, - strategyOverride: 'deep', - concurrencyPolicy: { - maxParallelInstances: 16, - }, - }); - const logicPackets = manifest.workPackets?.filter( - (packet) => packet.subagentId === 'ReviewBusinessLogic', - ); - const judgePackets = manifest.workPackets?.filter( - (packet) => packet.subagentId === 'ReviewJudge', - ); - - expect(logicPackets).toHaveLength(3); - expect(logicPackets?.map((packet) => packet.packetId)).toEqual([ - 'reviewer:ReviewBusinessLogic:group-1-of-3', - 'reviewer:ReviewBusinessLogic:group-2-of-3', - 'reviewer:ReviewBusinessLogic:group-3-of-3', - ]); - expect(logicPackets?.map((packet) => packet.assignedScope.fileCount)).toEqual([ - 9, - 8, - 8, - ]); - expect(logicPackets?.[0].assignedScope).toMatchObject({ - groupIndex: 1, - groupCount: 3, - }); - expect(logicPackets?.[0].assignedScope.files.slice(0, 2)).toEqual([ - 'src/web-ui/src/components/ReviewPanel0.tsx', - 'src/web-ui/src/components/ReviewPanel1.tsx', - ]); - expect(logicPackets?.[0].assignedScope.files.at(-1)).toBe( - 'src/web-ui/src/components/ReviewPanel8.tsx', - ); - expect(judgePackets).toHaveLength(1); - expect(judgePackets?.[0].assignedScope).toMatchObject({ - fileCount: 25, - }); - expect(judgePackets?.[0].assignedScope.groupCount).toBeUndefined(); - expect(manifest.tokenBudget).toMatchObject({ - estimatedReviewerCalls: 16, - maxFilesPerReviewer: 10, - largeDiffSummaryFirst: false, - }); - - const promptBlock = buildReviewTeamPromptBlock(team, manifest); - expect(promptBlock).toContain('"packet_id": "reviewer:ReviewBusinessLogic:group-1-of-3"'); - expect(promptBlock).toContain('"group_index": 1'); - expect(promptBlock).toContain('"group_count": 3'); - }); - - it('keeps split reviewer work packets grouped by workspace area when possible', () => { - const team = resolveDefaultReviewTeam( - coreSubagents(), - storedConfigWithExtra([], { - reviewer_file_split_threshold: 4, - max_same_role_instances: 3, - }), - ); - const target = classifyReviewTargetFromFiles( - [ - 'src/web-ui/src/components/ReviewPanel.tsx', - 'src/crates/assembly/core/src/agentic/deep_review_policy.rs', - 'src/apps/desktop/src/api/review.rs', - 'src/web-ui/src/shared/services/reviewTeamService.ts', - 'src/crates/assembly/core/src/agentic/tools/implementations/task_tool.rs', - 'src/apps/desktop/src/api/agent.rs', - 'src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.tsx', - 'src/crates/assembly/core/src/agentic/agents/deep_review_agent.rs', - 'src/apps/desktop/src/api/config.rs', - 'src/web-ui/src/locales/en-US/scenes/agents.json', - 'src/crates/assembly/core/src/agentic/agents/prompts/deep_review_agent.md', - 'src/apps/desktop/src/api/subagent.rs', - ], - 'session_files', - ); - - const manifest = buildEffectiveReviewTeamManifest(team, { - target, - strategyOverride: 'deep', - concurrencyPolicy: { - maxParallelInstances: 16, - }, - }); - const logicPackets = manifest.workPackets?.filter( - (packet) => packet.subagentId === 'ReviewBusinessLogic', - ); - - expect(logicPackets).toHaveLength(3); - expect(logicPackets?.map((packet) => packet.assignedScope.files)).toEqual([ - [ - 'src/web-ui/src/components/ReviewPanel.tsx', - 'src/web-ui/src/shared/services/reviewTeamService.ts', - 'src/web-ui/src/flow_chat/deep-review/action-bar/CapacityQueueNotice.tsx', - 'src/web-ui/src/locales/en-US/scenes/agents.json', - ], - [ - 'src/crates/assembly/core/src/agentic/deep_review_policy.rs', - 'src/crates/assembly/core/src/agentic/tools/implementations/task_tool.rs', - 'src/crates/assembly/core/src/agentic/agents/deep_review_agent.rs', - 'src/crates/assembly/core/src/agentic/agents/prompts/deep_review_agent.md', - ], - [ - 'src/apps/desktop/src/api/review.rs', - 'src/apps/desktop/src/api/agent.rs', - 'src/apps/desktop/src/api/config.rs', - 'src/apps/desktop/src/api/subagent.rs', - ], - ]); - - const promptBlock = buildReviewTeamPromptBlock(team, manifest); - expect(promptBlock).toContain('"scope_groups"'); - expect(promptBlock).toContain('"scope_id": "scope-1"'); - expect(promptBlock).not.toContain('assigned_scope'); - }); - - it('caps file splitting and launch batches by concurrency policy', () => { - const team = resolveDefaultReviewTeam( - coreSubagents(), - storedConfigWithExtra([], { - reviewer_file_split_threshold: 10, - max_same_role_instances: 3, - }), - ); - const target = classifyReviewTargetFromFiles( - Array.from( - { length: 25 }, - (_, index) => `src/web-ui/src/components/ReviewPanel${index}.tsx`, - ), - 'session_files', - ); - - const manifest = buildEffectiveReviewTeamManifest(team, { target }); - const reviewerPackets = manifest.workPackets?.filter( - (packet) => packet.phase === 'reviewer', - ) ?? []; - const logicPackets = reviewerPackets.filter( - (packet) => packet.subagentId === 'ReviewBusinessLogic', - ); - - expect(manifest.concurrencyPolicy).toMatchObject({ - maxParallelInstances: 4, - staggerSeconds: 0, - maxQueueWaitSeconds: 1200, - batchExtrasSeparately: true, - }); - expect(logicPackets).toHaveLength(1); - expect(logicPackets[0].assignedScope.groupCount).toBeUndefined(); - expect(reviewerPackets).toHaveLength(5); - expect(reviewerPackets.slice(0, 4).map((packet) => packet.launchBatch)).toEqual([1, 1, 1, 1]); - expect(reviewerPackets[4].launchBatch).toBe(2); - expect(manifest.qualityGateReviewer && manifest.workPackets?.find( - (packet) => packet.subagentId === manifest.qualityGateReviewer?.subagentId, - )?.launchBatch).toBe(3); - - const promptBlock = buildReviewTeamPromptBlock(team, manifest); - expect(promptBlock).toContain('"max_parallel_instances": 4'); - expect(promptBlock).not.toContain('max_queue_wait_seconds'); - expect(promptBlock).toContain('in launch_batch order'); - expect(promptBlock).toContain('"launch_batch": 2'); - }); - - it('keeps extra reviewers in a separate launch batch when requested', () => { - const team = resolveDefaultReviewTeam( - [ - ...coreSubagents(), - subagent('ReviewProductExtra'), - ], - storedConfigWithExtra(['ReviewProductExtra'], { - max_parallel_reviewers: 6, - }), - ); - const target = classifyReviewTargetFromFiles([ - 'src/web-ui/src/components/ReviewPanel.tsx', - 'src/web-ui/src/components/ReviewPanel.css', - ], 'session_files'); - - const manifest = buildEffectiveReviewTeamManifest(team, { target }); - const reviewerPackets = manifest.workPackets?.filter( - (packet) => packet.phase === 'reviewer', - ) ?? []; - const corePackets = reviewerPackets.filter( - (packet) => packet.subagentId !== 'ReviewProductExtra', - ); - const extraPackets = reviewerPackets.filter( - (packet) => packet.subagentId === 'ReviewProductExtra', - ); - - expect(corePackets).toHaveLength(5); - expect(extraPackets).toHaveLength(1); - expect(corePackets.map((packet) => packet.launchBatch)).toEqual([1, 1, 1, 1, 1]); - expect(extraPackets.map((packet) => packet.launchBatch)).toEqual([2]); - expect(manifest.qualityGateReviewer && manifest.workPackets?.find( - (packet) => packet.subagentId === manifest.qualityGateReviewer?.subagentId, - )?.launchBatch).toBe(3); - }); - - it('reduces reviewer concurrency when rate limit remaining is tight', () => { - const team = resolveDefaultReviewTeam( - coreSubagents(), - storedConfigWithExtra([], { - reviewer_file_split_threshold: 10, - max_same_role_instances: 3, - }), - ); - const target = classifyReviewTargetFromFiles( - Array.from( - { length: 25 }, - (_, index) => `src/web-ui/src/components/ReviewPanel${index}.tsx`, - ), - 'session_files', - ); - - const manifest = buildEffectiveReviewTeamManifest(team, { - target, - rateLimitStatus: { remaining: 2 }, - }); - const reviewerPackets = manifest.workPackets?.filter( - (packet) => packet.phase === 'reviewer', - ) ?? []; - - expect(manifest.concurrencyPolicy).toMatchObject({ - maxParallelInstances: 2, - staggerSeconds: 10, - batchExtrasSeparately: true, - }); - expect(reviewerPackets.map((packet) => packet.launchBatch)).toEqual([1, 1, 2, 2, 3]); - expect(manifest.qualityGateReviewer && manifest.workPackets?.find( - (packet) => packet.subagentId === manifest.qualityGateReviewer?.subagentId, - )?.launchBatch).toBe(4); - - const promptBlock = buildReviewTeamPromptBlock(team, manifest); - expect(promptBlock).toContain('"max_parallel_instances": 2'); - expect(promptBlock).not.toContain('stagger_seconds'); - }); - it('skips the frontend reviewer when the resolved target has no frontend tags', () => { const team = resolveDefaultReviewTeam( coreSubagents(), @@ -1445,7 +1190,8 @@ describe('reviewTeamService', () => { expect(manifest.tokenBudget).toMatchObject({ mode: 'balanced', - estimatedReviewerCalls: 7, + estimatedReviewerCalls: 1, + maxReviewerCalls: 3, maxExtraReviewers: 1, skippedReviewerIds: [], }); @@ -1485,9 +1231,7 @@ describe('reviewTeamService', () => { maxExtraReviewers: 0, skippedReviewerIds: ['ExtraEnabled'], }); - expect(manifest.tokenBudget.maxReviewerCalls).toBe( - manifest.tokenBudget.estimatedReviewerCalls, - ); + expect(manifest.tokenBudget.maxReviewerCalls).toBe(3); expect(manifest.enabledExtraReviewers).toEqual([]); expect(manifest.executionPolicy).toMatchObject({ reviewerTimeoutSeconds: 1200, @@ -1557,16 +1301,10 @@ describe('reviewTeamService', () => { reviewerFileSplitThreshold: 0, maxSameRoleInstances: 1, }); - expect( - manifest.workPackets.filter( - (packet) => - packet.phase === 'reviewer' && - packet.subagentId === 'ReviewBusinessLogic', - ), - ).toHaveLength(1); + expect(manifest.workPackets).toEqual([]); }); - it('keeps deep strategy thorough with long budget and same-role splitting', () => { + it('keeps deep strategy thorough without automatic reviewer fan-out', () => { const team = resolveDefaultReviewTeam( coreSubagents(), storedConfigWithExtra(), @@ -1600,16 +1338,12 @@ describe('reviewTeamService', () => { expect(manifest.executionPolicy).toMatchObject({ reviewerTimeoutSeconds: 3600, judgeTimeoutSeconds: 2400, - reviewerFileSplitThreshold: 20, - maxSameRoleInstances: 3, + reviewerFileSplitThreshold: 0, + maxSameRoleInstances: 1, + maxRetriesPerRole: 0, + maxReviewerCalls: 1, }); - expect( - manifest.workPackets.filter( - (packet) => - packet.phase === 'reviewer' && - packet.subagentId === 'ReviewBusinessLogic', - ), - ).toHaveLength(2); + expect(manifest.workPackets).toEqual([]); }); it('does not invent prompt-byte pressure or hide assigned files', () => { @@ -1644,65 +1378,12 @@ describe('reviewTeamService', () => { expect.objectContaining({ kind: 'summary_first_full_scope' }), ]), ); - const reviewerPackets = manifest.workPackets.filter( - (packet) => packet.phase === 'reviewer', - ); - expect(reviewerPackets).not.toHaveLength(0); - for (const packet of reviewerPackets) { - expect(packet.assignedScope.files).toEqual(files); - } - - const promptBlock = buildReviewTeamPromptBlock(team, manifest); - expect(promptBlock).not.toContain('token_budget_decisions'); - expect(promptBlock).not.toContain('prompt_byte'); - }); - - it('keeps summary-first disabled when split guardrails fit the prompt-byte budget', () => { - const team = resolveDefaultReviewTeam( - coreSubagents(), - storedConfigWithExtra([], { - reviewer_file_split_threshold: 4, - max_same_role_instances: 2, - }), - ); - const files = Array.from( - { length: 5 }, - (_, index) => `src/crates/assembly/core/src/agentic/small_${index}.rs`, - ); - const target = classifyReviewTargetFromFiles(files, 'workspace_diff'); - - const manifest = buildEffectiveReviewTeamManifest(team, { - target, - strategyOverride: 'deep', - tokenBudgetMode: 'thorough', - concurrencyPolicy: { - maxParallelInstances: 8, - }, - changeStats: { - fileCount: files.length, - totalLinesChanged: 25, - lineCountSource: 'diff_stat', - }, - }); - - expect(manifest.tokenBudget).toMatchObject({ - maxFilesPerReviewer: 4, - largeDiffSummaryFirst: false, - }); - expect(manifest.tokenBudget.maxPromptBytesPerReviewer).toBeUndefined(); - expect(manifest.tokenBudget.promptByteLimitExceeded).toBeUndefined(); - expect(manifest.workPackets.filter((packet) => packet.phase === 'reviewer')) - .toEqual( - expect.arrayContaining([ - expect.objectContaining({ - assignedScope: expect.objectContaining({ - groupCount: 2, - }), - }), - ]), - ); + expect(manifest.workPackets).toEqual([]); const promptBlock = buildReviewTeamPromptBlock(team, manifest); + for (const file of files) { + expect(promptBlock).toContain(file); + } expect(promptBlock).not.toContain('token_budget_decisions'); expect(promptBlock).not.toContain('prompt_byte'); }); @@ -1734,8 +1415,8 @@ describe('reviewTeamService', () => { const promptBlock = buildReviewTeamPromptBlock(team, manifest); expect(promptBlock).toContain('"file_count": 25'); expect(promptBlock).toContain('"changed_line_count": null'); - expect(promptBlock).toContain('"timeout_seconds": 1800'); - expect(promptBlock).toContain('"timeout_seconds": 1200'); + expect(promptBlock).toContain('"specialist_timeout_seconds": 1800'); + expect(promptBlock).toContain('"quality_inspector_timeout_seconds": 1200'); }); it('keeps normal manifest timeouts bounded even with diff line stats', () => { @@ -1773,8 +1454,8 @@ describe('reviewTeamService', () => { const promptBlock = buildReviewTeamPromptBlock(team, manifest); expect(promptBlock).toContain('"changed_line_count": 800'); expect(promptBlock).toContain('"changed_line_count_source": "diff_stat"'); - expect(promptBlock).toContain('"timeout_seconds": 1800'); - expect(promptBlock).toContain('"timeout_seconds": 1200'); + expect(promptBlock).toContain('"specialist_timeout_seconds": 1800'); + expect(promptBlock).toContain('"quality_inspector_timeout_seconds": 1200'); }); it('keeps advisory risk recommendations in the manifest but out of the launch prompt', () => { @@ -2040,9 +1721,8 @@ describe('reviewTeamService', () => { expect(promptBlock).toContain('Prepared Review execution plan'); expect(promptBlock).toContain('Execution rules:'); expect(promptBlock).toContain('"subagent_type": "ReviewSecurity"'); - expect(promptBlock).toContain('"strategy": "deep"'); expect(promptBlock).toContain('"model_id": "primary"'); - expect(promptBlock).toContain(`"prompt_directive": ${JSON.stringify(REVIEW_STRATEGY_DEFINITIONS.deep.roleDirectives.ReviewSecurity)}`); + expect(promptBlock).not.toContain('prompt_directive'); expect(promptBlock).not.toContain('Token/time impact'); }); @@ -2092,7 +1772,7 @@ describe('reviewTeamService', () => { const promptBlock = buildReviewTeamPromptBlock(team, manifest); expect(promptBlock).toContain('"selected_strategy": "deep"'); expect(promptBlock).toContain('"subagent_type": "ReviewSecurity"'); - expect(promptBlock).toContain('"strategy": "quick"'); + expect(promptBlock).not.toContain('prompt_directive'); }); it('falls back removed concrete reviewer models to the strategy default model slot', () => { @@ -2152,7 +1832,7 @@ describe('reviewTeamService', () => { expect(promptBlock).not.toContain(WORKSPACE_PATH); expect(promptBlock).toContain('"subagent_type": "ExtraEnabled"'); expect(promptBlock).not.toContain('ExtraDisabled'); - expect(promptBlock).toContain('Launch only active_packets'); + expect(promptBlock).toContain('Launch at most one specialist'); expect(promptBlock).not.toContain('Configured code review team:'); expect(promptBlock).not.toContain('Team execution rules:'); expect(promptBlock).not.toContain('run it in parallel with the locked reviewers whenever the change contains frontend files');