U4 — Tools (victor/tools/ + tool execution seam)

136 modules / 56,024 LOC / 1,538 churn commits (12m). Reviewer: parallel unit review, 2026-09-03.

Overview

victor/tools is a mature infrastructure layer whose per-tool ergonomics (the @tool decorator, schema-level tiers, folding) are genuinely good: schemas are built once at decoration time and metadata precedence is unified through resolve_contract. The problems are at the seams: the expensive work (schema generation, embedding, validation) is cached at the layer that nothing calls, while the real hot paths (per-turn selection, per-call execution) re-derive everything; and registration coherence rests on four-plus discovery mechanisms plus duplicated hand-maintained exclusion lists. Subprocess lifecycle handling (no process-group kill, buffer-then-truncate) is the main robustness gap. Churn leaders include two fully dead modules, and jira/slack/teams SaaS tools violate the "root stays generic" rule.

Findings

ID	Sev	Location	Finding	Why it matters	Fix sketch	Effort
F1	P1	victor/agent/services/tool_service.py:2784-2800 (call sites 2896,2946,2952,2960,3001,3009); victor/tools/registry.py:848-897	Per-turn selection regenerates full schemas: estimate_tool_tokens calls tool.to_schema(tier) + len(str(schema))//4 per tool, in ≥4 passes per turn; the registry's version-counter schema cache (get_tool_schemas, ~100 LOC) has zero production callers	Token estimate is a pure function of (tool, tier) but costs a dict build + repr each time, ×tools×passes per turn; amortization machinery built at the wrong layer	Cache (tool_name, tier)→(schema, token_est) in ToolService/selector, invalidated by registry version; wire get_tool_schemas in or delete it	M
F2	P1	victor/tools/bash.py:1899-1906; victor/tools/subprocess_executor.py:638-646	Timeout handling: process.kill() kills only the shell, not its children (no start_new_session+os.killpg); partial output discarded; identical duplicated logic in both runners	sh -c "make test" killed at timeout leaves orphan children running (writing to inherited pipes); two copies of the same lifecycle code drift	One shared runner: start_new_session=True, killpg on timeout, return partial stdout; both bash.py and run_command_async delegate to it	M
F3	P2	victor/tools/bash.py:1890-1924; subprocess_executor.py:445-481	Output caps applied AFTER full buffering: communicate() accumulates everything, then truncates (10k/2k lines, 1MB default); 500KB hard cap in tool_executor.py:976 also fires post-hoc	A runaway yes/cat big.bin OOMs the agent before truncation ever runs	Incremental reader that stops and kills at byte cap (extend _stream_subprocess_output, bash.py:1353)	M
F4	P2	victor/tools/semantic_selector.py:1135-1148, 1670-1736	_get_usage_boost re-awaits _get_embedding(query) per candidate tool plus up to 3 context embeds each (~4N embed computations/turn); sentence-transformers path is saved by EmbeddingService's text cache; the ollama/vllm/lmstudio path has NO cache → N×4 HTTP round-trips per turn	Per-turn selection latency scales with tools-used count on API embedding providers	Compute query embedding once per selection and pass it in; memoize _get_api_embedding; skip context-similarity below threshold	S
F5	P2	victor/tools/semantic_selector.py:1697-1701, 1732-1736	On embedding failure both paths silently fall back to np.random.randn(...) — "better than crashing"	Random vectors produce meaningless similarity scores; selection degrades to noise with only a warning, feeding bad scores into RL usage-stats learning	Fall back to keyword/mandatory-tool selection instead of random-vector scoring; ERROR with one-shot telemetry	S
F6	P2	victor/tools/base.py:607-611; registry.py:996; agent/tool_executor.py:602	Draft7Validator(schema) constructed on every validate_parameters_detailed call — per tool execution on the default LENIENT path	Schema is static per tool instance; validator construction is pure per-call CPU overhead around I/O-bound work	Cache the validator on the tool instance or build once in the decorator (decorators.py:836)	S
F7	P2	victor/agent/shared_tool_registry.py:404-428 vs victor/tools/registry.py:1155-1169; shared_tool_registry.py:493-496	Two duplicated diverging scan-exclusion lists (22 vs 13 entries; 9 missing from discover_lightweight, plus stale composition.py which is a directory); "shell" name collision between bash.py:1531 and unified/shell_tool.py:140 resolved only by os.listdir alphabetical scan order	Adding a helper module to victor/tools silently registers it as a tool or double-registers unified names if scan order changes; CLI listing ≠ runtime registry	Single exclusion manifest (or explicit __victor_tool_module__ marker) consumed by both scanners; register unified tools under distinct names	M
F8	P2	victor/tools/semantic_selector.py:300-329, 452-478	Embedding cache is all-or-nothing: any single tool description change invalidates the whole pickle and re-embeds all ~60 tools serially (one await per tool, no batching)	A one-line description tweak costs 60 embedding calls at next startup	Per-tool cache keyed by hash(tool_text); reuse unchanged vectors; batch-embed where supported	M
F9	P2	victor/tools/jira_tool.py:61, slack_tool.py:63, teams_tool.py:149	Jira/Slack/Teams SaaS integration tools in the generic root, auto-registering via the scan (availability-gated, but present)	Violates "root stays generic; domain behavior belongs in verticals/plugins"	Move to a vertical or plugin package; root keeps the adapter seam	M
F10	P2	victor/tools/browser_tool_legacy.py (913 LOC); code_search_tool.py (212 LOC stub)	browser_tool_legacy has no @tool, no BaseTool subclass, zero production importers — fully dead (superseded by AgentBrowser MCP, #964); code_search_tool is a self-declared STUB kept "for test compatibility" yet is the unit's #1 churn hotspot (churn=166)	1,125 LOC of dead weight; the stub's 166 commits are pure migration drag	Delete both; port the few tests to unified search_tool/agentbrowser (precedent: commit 613583914)	S
F11	P3	semantic_selector.py:470-478	Cache-diff logging bug: added/removed compare tool_names vs embeddings keys from the SAME cache_data — always empty	Debug output promised in the code never appears	Compute diff against the live tool list	S
F12	P3	victor/agent/tool_registrar.py:463-467; registry.py:527-535, 259	Main startup loop registers tools one-by-one without batch_update() (batch_registration.py:259 exists to provide it); each register() re-checks the feature-flag manager and re-imports the dedup module per tool	~60 tools × (flag lookup + dedup import + lock + version bump) of avoidable startup cost	Wrap the registrar loop in registry.batch_update(); hoist the flag check per batch	S
F13	P3	metadata_registry.py:547-575 vs semantic_selector.py:400-423	Two divergent "tools hash" implementations: metadata hash covers 16 fields; embedding hash covers only name/description/parameters via unstable str() repr	Keyword/metadata edits don't invalidate embeddings (stale vectors); repr-fragile across refactors	One shared snapshot+hash helper; canonical JSON (sort_keys) not str()	S
F14	P3	victor/ui/commands/verify.py:32; decorators.py:680; registry.py:679-700	UI imports victor.tools.verification directly (Client→Infrastructure); unused import hashlib in sanitize_schema_for_llm; register_dict defines a new DictTool class per call	Layering bypass unguarded; hygiene debt in high-fan-in modules	Route verify through a service/framework facade; delete import; hoist DictTool	S

Co-design opportunities

1. Schema/token snapshot at the consumption seam (with agent/services): (tool, tier)→schema/token-estimate cache keyed by registry version; retire or consume registry.get_tool_schemas (F1).
2. One serialization seam for tool results (with pipeline): agree a single orjson-based dict→LLM-text contract; today original_result uses str() repr (tool_executor.py:1002) and previews re-str() twice (tool_pipeline.py:2302, 3205).
3. Shared subprocess runner contract: process groups, incremental capped reads, partial-output-on-timeout — fixes F2/F3 for both owners at once.
4. Embedding-provider parity: lift caching into _get_embedding or expose batch-embed on the embeddings service (F4, F8).
5. Registration as one mechanism: entry points + explicit module registry; consolidate the 5 discovery mechanisms. Registry-shape changes are FEP-worthy (fan_in=48).
6. Validator/schema precompile in BaseTool or decorator (F6) — private cache needs no FEP; signature changes do.

Keep intact

1. mtime-validated FileContentCache + read-tracking (filesystem.py:279-344, 564-615), cleared after non-readonly shell commands (bash.py:1955-1965) — load-bearing locality.
2. Tool folding (folding.py, 24 folds): folded tools stay executable while hidden from advertised schemas — deliberate token economy; do not "fix" by unregistering.
3. resolve_contract weak-ref autogen cache (contract.py:39-91) — single metadata-precedence authority.
4. Decorator-time schema build (decorators.py:661-836) — the right amortization pattern; extend it rather than adding per-call caches.
5. ShellCommandCache per-command-prefix TTLs (shell_command_cache.py:55-93), readonly-only caching.

Quick metrics

• 64 @tool functions in root+unified; 122 test files in tests/unit/tools; 443 commits touched victor/tools in 12 months.
• Dead inventory: browser_tool_legacy.py 913 LOC + code_search_tool.py 212 LOC stub = 1,125 LOC.
• get_tool_schemas production callers = 0; estimate_tool_tokens regenerates schemas at 6+ call sites.
• Exclusion-list drift: 22 vs 13 entries, 9 missing, 1 stale.
• Metadata hash = 16 fields vs embedding hash = 3 fields.
• Registration mechanisms: 5 (entry points, in-tree scan, CLI scan, path plugins, flag-gated strategies); main registrar loop unbatched.