U6 — Workflows, teams, coordination & protocols

134 modules / 73,211 LOC / 1,220 churn commits (12m). Reviewer: parallel unit review, 2026-09-03.

Overview

workflows is the bulk (99 files, 56k LOC) carrying the compiler+engine plus accreted peripheral features (scheduler, deployment, credentials, generation, metrics); teams+coordination implement the formations law well (coordinator-as-node is real); protocols is a domain grab-bag. The central defect is engine duality: the hand-rolled BFS WorkflowExecutor and the StateGraph path are both live in production, with divergent parallel-failure, caching, and checkpoint semantics. Second theme: everything is recompiled/revalidated/re-deep-copied per run — the "caches" cache the cheap artifact (parsed YAML), never the expensive one (CompiledGraph, state). ADR-023 durability is well built but concentrated 59 commits of churn into one 3,404-line god-file.

Findings

ID	Sev	Location	Finding	Why it matters	Fix sketch	Effort
F1	P1	victor/workflows/unified_executor.py:456-1095; victor/integrations/api/routes/workflow_routes.py:181; victor/workflows/mode_workflows.py:164; victor/workflows/services/workflow_service_provider.py:479	Two live workflow engines: WorkflowExecutor (= CompiledWorkflowExecutor) is a hand-rolled BFS DAG walker (own traversal, own ParallelNode gather, own checkpoint shape, own node cache) and is the package's canonical export; StateGraphExecutor compiles to CompiledGraph. API/mode-workflows/service-provider use the BFS one; chat UI + runtime.py use StateGraph	Violates "StateGraph is always the execution engine" (docs/architecture.md:457); two divergent semantics (parallel failure, caching, checkpointing) drift independently; HITL/ADR-023 features only exist on one path	Make CompiledWorkflowExecutor a thin facade compiling via compiler/boundary and delegating to CompiledGraph.invoke (or migrate the 4 call sites); delete the BFS loop	M
F2	P1	victor/workflows/unified_compiler.py:690-712, 803-842, 536-565	Compiled graphs are never cached. CachedCompiledGraph is a misnomer: on a definition-cache hit the code still runs WorkflowDefinitionValidator + full NativeWorkflowGraphCompiler.compile per execution; compile_definition/compile_graph check no cache; LegacyWorkflowGraphCompiler constructs a fresh backend compiler per call (boundary.py:378)	Every YAML run pays validate + graph construction per execution; the definition cache only skips YAML parsing — the cheap part. Binding constraint for repeated invocation is CPU, not LLM I/O	Cache CompiledGraph keyed by (definition-identity hash, executor-factory version); skip validation on hit; include $ref mtimes	M
F3	P2	victor/framework/graph_runtime.py:251; graph_execution.py:50-54	state_history.append((node, copy.deepcopy(state))) after EVERY node — unbounded full-state deep copies; no internal consumer reads state_history; plus per-node checkpoint save and per-branch deepcopies (graph.py:492, boundary.py:322)	O(nodes × state_size) CPU+memory around LLM waits; long/cyclic workflows grow unboundedly (25 iterations default)	Make state_history opt-in via GraphConfig; store reference or cap to last-N; one shared snapshot for checkpoint+history	S
F4	P2	graph_execution.py:38-47; graph_runtime.py:197-204, 250, 261-270; unified_executor.py:329-336	Interrupt/resume broken at the seam: GraphExecutionResult has no interrupted field (both interrupt paths return success=True); ExecutorResult.interrupted exists but never set; per-node checkpoint stores the completed node's id so resume re-executes the last completed LLM node; invoke() on a thread with a checkpoint silently ignores new input_state	HITL callers can't distinguish "paused" from "done"; crash-resume duplicates an expensive agent node; silent input discard is a trap	Add interrupted/next_node to result and checkpoints; separate "completed-at" from "resume-at"; error on resume with non-null input_state. FEP required (graph semantics)	M
F5	P2	victor/framework/graph.py:503-510 vs compiler/boundary.py:316-349	Parallel-branch failure semantics inconsistent: Send fan-out raises ParallelBranchExecutionError on ANY branch failure and discards successful branch states; ParallelNode path records per-child failure and continues	One flaky LLM branch destroys sibling work; the two mechanisms give authors opposite answers for the same YAML shape	Merge successful branch states, attach per-branch errors, let join policy decide; converge both mechanisms	M
F6	P2	graph.py:470-516; boundary.py:300-351; unified_executor.py:832-885	Three parallel-execution dataflow implementations (Send+_state_merger, ParallelNode gather into _parallel_results, legacy semaphore gather), all merging by whole-dict merge — no channel/reducer semantics on the state schema	Concurrency correctness depends on which path a node compiles into; reducers (append vs overwrite) are implicit and differ per path	One Send-based fan-out + declarative per-key reducers on WorkflowStateModel (FEP)	L
F7	P2	victor/teams/unified_coordinator.py:773-1122, 1270-2840	God-file (3,404 LOC, churn 59/12mo): _execute_formation ≈350 lines mixing formation dispatch, worktree planning/materialization, merge orchestration, approval gating, 8+ contract builders; ~1,700 lines of delegate/reentry/merge/approval contract construction in the coordinator	Highest churn×size risk in the unit; every ADR-023 PR (#733–#752) funneled here	Extract WorktreeMergeService and a DelegateContractBuilder (pure functions over MemberResults)	L
F8	P2	unified_coordinator.py:2976, 1472, 1337, 1378, 1994, 2002	Redundant copy.deepcopy(dict(context)) per team run: _execute_with deep-copies the whole context, _normalize_delegate_execution_request deep-copies again, each _build_delegate_* builder deep-copies step requests — 3-6 full copies of the same multi-KB payload per delegate step	Data movement around LLM calls compounding in delegate/reentry flows (exactly what ADR-023 added); none protect anything the previous didn't	One deep snapshot at the boundary; builders take read-only views; copy-count assert in tests	S
F9	P2	graph_execution.py:241, 252-271	Resume does checkpoint.state.copy() (shallow) while the loop mutates nested dicts in place (_bind_graph_checkpoint_id rewrites state["context"]) — stored checkpoint mutated through shared references	A second resume from the same checkpoint sees silently mutated history; breaks replay_from's purpose (graph.py:632-675)	Deep-copy at resume or immutable snapshot at save	S
F10	P3	victor/workflows/adapters.py (486 LOC); metrics.py (869 LOC); unified_executor.py:937-976	Dead-in-production modules with active churn: zero non-test importers, 14 churn commits each; _execute_chain_handler calls get_chain_registry() which unconditionally raises NotImplementedError — a dead path kept alive only by tests that patch it	Maintenance tax; tests pin dead behavior, blocking removal	Delete or move behind a legacy extra; migrate tests to canonical paths	S
F11	P3	victor/workflows/protocols.py:347 vs victor/framework/coordinators/protocols.py:55, 82; victor/protocols/team.py:357	Duplicate contract surfaces: IWorkflowExecutor defined twice, IStreamingExecutor vs IStreamingWorkflowExecutor, TeamCoordinatorProtocol aliasing ITeamCoordinator	Vertical/SDK consumers can't tell which protocol is the contract; both are structural Protocols so drift is silent	One canonical home (victor_contracts for cross-boundary, framework for engine-internal); re-export with shims	S
F12	P3	victor/protocols/provider_adapter.py (1,270 LOC), query_enhancement.py, search.py, lsp_types.py, grounding.py, quality.py, tool_selector.py	victor/protocols is a domain grab-bag: only team.py/coordination.py belong; provider_adapter is LLM-provider semantics; the rest is codegraph/search domain	Package-name artifact, not real coupling; past import cycles fixed by lazy imports (commit 09df774ed) stem from this mixing	Split: provider_adapter → victor/providers; search/LSP/grounding → search/codegraph; team protocols stay or move to contracts	M
F13	P3	victor/workflows/services/credentials.py (2,887 LOC)	Generic credential manager (env/keyring/encrypted file/IAM/cloud + the victor keys CLI) inside the workflow-compilation package; ditto definition.py (1,279 LOC of ServiceNode lifecycle)	Package cohesion: workflow compilation has no business owning keyring/IAM logic	Move credentials to victor/core (or config); document ServiceNode boundary	S
F14	P3	victor/workflows/hitl_api.py:1363-2704	~1,341 of 2,859 lines (47%) are inline HTML/CSS/JS string literals (get_hitl_ui_html 1,159 lines + request page 182)	Unlintable, untestable UI inside the workflows package	Move to template/static assets via importlib.resources	S

Co-design opportunities

1. Compile-once/execute-many artifact: one CompiledGraph cache keyed by definition hash + factory version, shared by WorkflowEngine, API routes, and the TUI (all three currently construct their own compiler+executor per request).
2. Typed team-result channel: a team channel with per-key reducers on WorkflowStateModel lets the graph engine share instead of copy (vs today's full-result dict into team_output at unified_coordinator.py:3386-3395).
3. One parallel dataflow: single channel/reducer core makes formations trivially expressible as node functions over the same semantics (FEP).
4. Unified checkpoint record: three checkpoint shapes today — graph WorkflowCheckpoint, ADR-023 team member checkpoints, HITL StoredRequest (hitl_api.py:106). One envelope (thread, node/member, resume-at, payload) fixes F4's confusion.
5. Prefix-cache-aware member hand-offs: sequential formation passes previous_output as one blob (formations/sequential.py:84-104); co-design with provider KV-prefix caching (frozen system prompt, delta-only user turns) so member B's prompt extends A's prefix.
6. Protocols consolidation into victor_contracts, enabling F1 engine unification without breaking external implementers.

Keep intact

1. UnifiedTeamCoordinator.__call__ + StateGraphNodeConfig key remapping (unified_coordinator.py:3204-3404) — the law-compliant node interface; formation_strategy injects a hint and never mutates self._formation (concurrency-safe).
2. ContextVar-scoped _CoordinatorExecutionState (:127-134, 283-286) — per-call state without locks; the concurrency model ADR-023 correctly built on.
3. ADR-023 opt-in hooks on TeamContext (formations/base.py:92-119): default-None so non-durable runs are byte-identical — preserve the null-default discipline in any extraction.
4. Definition cache keying by path+name+config_hash+mtime (cache.py:798-812) — clean invalidation primitive; extend it to the CompiledGraph cache.
5. CopyOnWriteState owner thread/task guard and COW-by-default (graph_state.py:51-90) — load-bearing for state isolation.

Quick metrics

• unified_coordinator.py = 3,404 LOC, 59 churn commits/12mo — 33% of teams+coordination LOC in one file.
• Executor sprawl: 19 modules define *Executor classes; 3 self-declared shims; victor/workflows/__init__.py exports 228 names.
• Engine split (live call sites): legacy BFS executor → API routes, mode_workflows, workflow_service_provider, agent/specs/converter (4); StateGraph → ui/commands/chat, workflows/runtime, deployment (3).
• Deep-copy density: 1 per node (state_history, zero internal readers), 1 per parallel branch ×2 mechanisms, 2-6 per team delegate run.
• Dead-in-prod: adapters.py (486) + metrics.py (869), 14 churn commits each; chain handler raises NotImplementedError, only tests exercise it.
• hitl_api.py: 47% inline HTML; duplicate IWorkflowExecutor protocols at two paths.
• Framework→workflows inversion: victor/framework/workflow_engine.py and framework/coordinators/* import victor.workflows.* (workflow_engine.py:95-100) — the "stable framework layer" engine delegates to 56k LOC of internals.