Skip to content

feat(viking): archive full ACP traces and add MCP tool namespacing - #382

Merged
Million-mo merged 90 commits into
wolf1069b:mainfrom
ChenBinfighting1:feature/chen.bin.niko/iroot-viking-mem-diag
Aug 26, 2026
Merged

feat(viking): archive full ACP traces and add MCP tool namespacing#382
Million-mo merged 90 commits into
wolf1069b:mainfrom
ChenBinfighting1:feature/chen.bin.niko/iroot-viking-mem-diag

Conversation

@ChenBinfighting1

@ChenBinfighting1 ChenBinfighting1 commented Aug 20, 2026

Copy link
Copy Markdown

No description provided.

wh and others added 30 commits August 11, 2026 13:59
- New wiki_build.py defines WikiBuildConfig/WikiBuildCapability/RoleFilter,
  lazily importing xeno_adp_agentic WikiBuildTools at runtime to keep the
  framework dependency-free
- Tools are exposed as FunctionToolset (or RoleFilteredToolset) with a
  RunContext-injected wrapper preserving the real tool signature
- Team task list (mine_only) shows owned subtasks under another member's
  parent; team mode prompt guides workers to list owned tasks by ID
- mypy: allow missing imports for xeno_adp_agentic/xeno_adp_harness
Extend wiki build tool registry with find_wiki and the OPS/OPL worker
tool set (create_ops/get_ops, create_opl/get_opls, op_flow_status).
Define dedicated wiki_opa_worker / wiki_ops_worker / wiki_opl_worker
role permissions and register them in WIKI_AGENT_ROLES.
…enhancements

- Add viking/wiki_index.py + wiki_build tools (batch entity, index inject, service entrypoint)
- MCP manager.py: PrefixedToolset monkey-patch for tool name conflict resolution
- mcp_server_cap: disabled_tools support
- team_comm_capability: team member role lookup
- compaction: message count compaction support
- DCP: fallback nested model config, team state directory propagation
- run.py: session controller run improvements
- Add tests for all new features
Add the generic layered `browse` tool and `browse_chapters` to
ALL_WIKI_TOOLS, _HELPER_TOOL_NAMES, and _READ_TOOLS so
conductor/extraction/relation/opa/ops workers can drill down over any
raw-root URI without the full tree listing. Previously browse_chapters
was gated as a helper but absent from _READ_TOOLS, so role filtering made
it unreachable.
…rden distillation JSON

- event_mapper: handle duplicated concatenated JSON + invalid JSON args
  (vLLM glm47/SGLang GLM streaming corruption); invalid args become {} so
  raw garbage never reaches model requests or tool execution
- turn.py: sanitize inbound restored history before first model request
- dcp/tools.py: cap distillation at 4000 chars to keep arguments valid JSON
- wiki_build: allow rebuild_all_backlinks for file_op/finalize roles
ExternalOPCapability exposes three tools (submit_external_opa,
submit_external_ops, apply_external_opl) letting an external expert
feed OPA→OPS→OPL feedback into the wiki OP flow. Supports revise-by-URI:
passing ops_uri/opl_uri updates the existing record in place so an
expert can iterate on one opinion until accepted. sync_after_apply
optionally pushes the patched page and OPL to remote Viking.
Extract tool functions to wiki_build_tools.py (matching tools.py pattern),
slim WikiBuildCapability to config+lifecycle+delegate, merge ExternalOPCapability
into wiki_external_expert role. Delete external_op.py.

- wiki_build_tools.py: ALL_WIKI_TOOLS, ROLE_TOOLS, RoleFilter, build_tools(cap),
  _build_method_wrappers, _build_external_op_fns, _sync_entity_to_remote,
  get_instructions(role)
- wiki_build.py: WikiBuildConfig (+sync_after_apply, +include_external_ops),
  WikiBuildCapability (config/tools properties, delegates get_toolset to build_tools)
- external_op.py: deleted (3 OP closures moved to wiki_build_tools.py)
- wiki_external_expert role: 3 OP tools + browse/read/search/get_opas/get_ops/get_opls
- docstrings: viking:// URI format + discovery guidance for agent perception
Add refine_opa_reason_code and ops_dispatch_plan to ALL_WIKI_TOOLS.
Expand wiki_ops_worker role with _OPA_REFINE_TOOLS, _OPA_RESOLVE_TOOLS,
and _OPS_RESOLVE_WRITE_TOOLS (patch_entity + rebuild_backlinks) so the
OPS resolver can close relation_missed gaps. Add _OPS_DISPATCH_TOOLS to
wiki_conductor so it can drive deterministic OPS dispatch. Entity
materialization tools (write_entity/merge_entity) remain excluded from
ops_worker — that stays with file_operator.
…l link_relations

- viking_upload_tree: single add_resource call (semantic+vectors, wait=True),
  temp strict-dir copy, 200-byte filename guard, skip index/source_packets
- viking_link_relations: aggregate relations by from_uri, link each node at
  most once per direction (multi-target single call) to dodge the server-side
  relations lock bug; resolve local X.md URIs to the server md wrapper
  X/X.md; best-effort with backoff, reports linked/failed/skipped
- viking_add_resource: optional processing_mode + wait params
- unit tests for URI resolution + aggregated link semantics; live E2E manual
  test verified against real server (upload embeddings 0 errors, linked=3)
Replace the external OP closures in wiki_build_tools.py with a ticket-based
capability (ticket.py): create_opa_ticket / create_ops_ticket /
create_opl_ticket / apply_opl_ticket / get_ticket_status /
submit_eval_payload. wiki_external_expert role now routes to the ticket
toolset; sync_after_apply pushes patched wiki pages to remote Viking.
…l-worker-tools

Integrate upstream #364/#356/#368/#372/#374/#375/#367 changes. Resolved:
- pyproject/uv.lock: openviking-sdk 0.1.8 (keep local; image download_bytes comment)
- mcp_server_cap.py: keep fork's self._tool_prefix logic
- viking/tools.py: combine imports (PurePosixPath + json/Path/shutil/tempfile)
- test_viking*.py: accept upload_tree (local wiki feature), 4→5 / 12→13
- wiki_build.py: FunctionTool used at runtime (FunctionTool.from_callable),
  import outside TYPE_CHECKING to avoid latent NameError (ruff TC004).
- test_live_e2e_manual.py: break combined assertion into per-edge checks
  (ruff PT018).
…orage config

Companion work that was pending on the branch:
- team_state/team_comm: lease-token task claim + automatic renewal, member
  subtask creation permission, lease TTL config
- model_configs: model capability additions + tests
- storage: config hardening + storage tests
- viking wiki_build_tools: extraction worker read-tool restriction (drop
  read_chapters_batch from extraction lane)
- serve_opencode: CLI wiring
Allow StringModelConfig to disable OpenAI strict tool definitions
(\"strict\": true) for backends that reject them (e.g. sglang/vLLM
speculative decoding with grammar constraints). When set to False,
pydantic-ai skips sending strict on tool schemas.
…wiki_read_resource

- WikiBuildCapability now implements ResourceAccess/ResourceTemplateAccess:
  OPA/OPS/OPL tickets surface as MCP resources (list_resources, @-completion,
  URI templates).
- Rename agent-facing tool read_resource -> wiki_read_resource to avoid a
  name clash with the generic ResourceCapability's read_resource when a
  native agent hosts both toolsets.
- The bound method stays read_resource (ResourceAccess protocol); the tool
  wrapper name is mapped in wiki_build_tools._build_method_wrappers.
Allow StringModelConfig to disable forced tool choice
(tool_choice='required') for backends that reject it when it conflicts
with their grammar constraints (e.g. sglang/vLLM speculative decoding
DFLASH). When set to False, pydantic-ai falls back to tool_choice='auto'.

Fixes structured outputs (output_type) on dspark which error with
'DFLASH speculative decoding does not support grammar-constrained
decoding yet' when tool_choice is forced.
…oice_required

The dspark (sglang DFLASH) backend keeps rejecting grammar-constrained
decoding regardless of these client-side switches; strict and forced
tool_choice flags did not help. Revert them to keep model config as
before.
…iking-ops-opl-worker-tools

# Conflicts:
#	src/wolfharness/capabilities/viking/tools.py
…fixes

- ticket: add update_ops_ticket (in-place OPS patch with status transitions)
- wiki_build_tools: register BOM enrichment + chapter planning tools
- file_team_state: count active (non-terminal) tasks for capacity, unify
  create lock between single and batch dispatch
- team_comm: task_list active_only filter
- mcp_server: drop redundant unconditional PrefixedToolset wrapping
  (mcp_server_cap conditional prefixing already covers it); fixes
  test_function_model_discovers_mcp_tools_through_acp_transport
- dcp/tools: extract _DISTILLATION_MAX_CHARS constant (PLR2004)
- tests: add resource protocol test; ruff format + import fixes
test_live_upload_tree_then_link_relations requires a real OpenViking
configuration (~/.openviking/ovcli.conf). Mark it real_mcp so the core
CI job (which excludes real_mcp) does not collect it.
team_add_member now accepts initial_task or initial_task_id; the task is
persisted and assigned before the member receives its first message, so
dynamic workers never observe an empty mine_only task view and exit early.
Recovery can bind a released pending/blocked task to a replacement member
without duplicate task IDs. Includes related resource_capability and
viking ticket/build-tools updates plus tests.
…-worker-tools

# Conflicts:
#	src/wolfharness/capabilities/viking/__init__.py
#	src/wolfharness_config/capabilities.py
t-wanghai and others added 23 commits August 22, 2026 00:37
P2 concurrency:
- materialization.py: wave cap 8→12 to match max_parallel_members
- team_comm_capability.py: enforce max_parallel_members in team_add_member
  (count non-lead members with live runs, reject when cap hit)
- ops_dispatch_plan (opa.py + ticket_engine.py): add max_parallel_shards
  param, group items by target_class into _entity_batch_limit()-sized shards
  with shard_id/worker_role/opa_ids/task_description, wave-cap + remaining_count

P3 cleanup + OPA quality:
- _mark_merge_conflict (entities.py): entity name in title, readable opa_id
  slug, human-readable finding prefix
- discover_opa (opa.py + ticket_engine.py): prepend entity name to title;
  low-value filter skips relation_missed, frontmatter:* (unless
  content_missing/fact_conflict), Procedure/DTC/Part with
  relation_missed/extraction_missed; skipped_low_value counter in return
- wiki_conductor.j2: action cap ≤3→≤6; OPS dispatch via sharded
  ops_dispatch_plan(max_parallel_shards=N)
- wiki-team.yaml: removed stale write_text_atomic reference
- Deleted orphan prompts: wiki_opl_worker.j2, wiki_ticket_reviewer.j2
…tch optimization

Component template (template_materializer.py):
- Branch on concept==Component: emit 总成概览 + 工作机理 (from
  causal_chain.normal_function) + optional 组成零件 + 来源
- Drop 规格参数/关键事实/步骤/因果链 for Component pages
- Non-Component path unchanged

Finalize gate (finalize.py):
- Replace gap_category whitelist with _CORE_PATH_BLOCKER_CODES frozenset
- Only block on: Component.working_mechanism, Fault.failure_mechanism,
  Device.critical_components, DTC.related_faults
- Removed SymptomProfile.device_refs (schema-optional, no body fallback,
  closure defers it — stale as blocker in body-first design)
- All other errors log warning and proceed to finalize

Batch optimization (materialization.py):
- Raise template shard cap 12→50
- Conductor merges all template shard URIs into one
  materialize_template_batch call per wave
Conductor's audit_wiki cache was never invalidated because conductor
has no write tools — worker invalidations only affect their own
instances. Added force_refresh param for explicit bypass, plus
automatic entity-count staleness check (cheap list_entities call)
that catches adds/deletes without reading file contents.
…source refs

Template:
- Add Fault branch: emit 失效描述 + 失效机理 (from causal_chain) + 影响范围
- Remove ## 来源 section from all concept templates (raw refs only on Device)
- Remove 来源 from schema body_sections for all concepts

Audit:
- Strip confirmation_requirements to 4 core-path checks only:
  Component.working_mechanism, Fault.failure_mechanism,
  Device.critical_components, DTC.related_faults
- Unregister LightweightMaterializationHook, BodyEvidenceHook,
  SourceReferenceHook from ENTITY_VALIDATION_HOOKS

Also includes prior uncommitted fixes:
- storage.py: object_name byte-clip for long filenames
- opa.py/ticket_engine.py: repair_only disposition converts to gap OPA
  instead of silently skipping
When remote_sync fails, checkpoint is set to remote_sync_pending.  On
the next finalize_wiki call, instead of re-running audit (which would
activate strict hooks on already-promoted confirmed entities and
self-lock the gate), skip audit + promotion and retry only the remote
upload via _retry_remote_sync.
…ice fuzzy lookup

- finalize.py: entities param defaults to None; store auto-discovers
- team_workflow_guard.py: accept finalized_local as valid receipt status
- migration.py: fuzzy fallback for device_id → object_name mismatch
Truncated/stale source URIs in frontmatter sources are reference-quality
issues, not data-integrity blockers. Hard rejection blocked finalize
promotion writes when an entity had a bad source URI written via
write_entities_batch (which skips this check). Now logs a warning instead.
finalize_wiki calls _current_source_snapshot_id() which raised ValueError
on any unresolvable source URI. But audit_wiki drops unresolvable sources
from its hash instead of raising — so the two snapshots disagreed.
Now both sides skip unresolvable sources, matching hashes.
1. patch_entities_batch: add preloaded_contents param to skip double-read
   in build_relation_closure (content already read by ThreadPoolExecutor)
2. materialize_template_batch: skip overwriting confirmed/published
   entities (respect expert content during incremental ingestion)
3. plan_materialization_work: LLM shard task_description now includes
   existing_entity_check=mandatory instruction
- tickets/opa.py: feedback category never dedupes/merges — every submission
  mints a fresh record via uuid4 fragment on generated ids, so the same
  problem reported again (possibly by another expert) stays a separate OPA;
  explicit opa_id/opa_uri revisions still update in place
- tickets/ticket.py: drop the pending-OPA reuse shortcut that silently
  discarded new content when any pending OPA existed for the target
- carry in-progress wiki build batch (storage backends, quality, planning,
  hooks, templates) that the pipeline has been running against
…ing-mem-diag' into feature/wh/merge-to-chenbin
Same principle as the feedback-OPA fix: one external submission = one
record. Blind consume/merge surfaces removed:

- ingest_external_ops: auto id now carries a fresh random fragment, so
  same-parent/same-title submissions with different solution/analysis
  create distinct OPS records instead of being silently discarded as
  idempotent; explicit ops_uri revisions still rewrite in place
- create_ops: source_type=external_expert skips the pipeline worker
  consolidation (retrieval_query dedupe) and mints a fresh record;
  pipeline default unchanged (concurrent workers still consolidate)
- create_opl: source_type=external_expert skips the ops-set dedupe and
  mints a fresh record per snapshot; pipeline default unchanged

Regression tests (xeno workspace): distinct records for repeated
external OPS/OPL submissions, in-place revision via explicit URIs, and
pipeline consolidation preserved.
- Relax _validate_opa_uris to accept any viking://resources/ URI across
  namespaces, not just the wiki store namespace
- Update VikingCapability tools and instructions for cross-namespace
  ticket references
- Add ticket reference scope tests
- Align capabilities config for broader URI handling
- Run ruff format on all wiki/ and viking/ re-export files
- Run ruff check --fix --unsafe-fixes for auto-fixable lint issues
- Add per-directory ruff ignores for wiki/ (CJK unicode, complex methods,
  magic values, import ordering — inherited from xeno-adp-agentic codebase)
- Exclude wiki/ from mypy strict checking (841 type errors from copied code)
- Restore backward-compat re-exports in viking/wiki_build.py, viking/ticket.py,
  viking/wiki_index.py (ruff auto-fix had removed explicit re-exports of
  underscore-prefixed names that tests depend on)
- Fix test_resource_protocol.py wiki_cap fixture: directly inject
  FakeWikiBuildTools instead of mocking xeno_adp_agentic imports
  (_ensure_tools now imports from internal wolfharness.capabilities.wiki)
…quest

Import was 'wolfharness.capabilities.wiki.index' (non-existent) instead
of 'wolfharness.capabilities.wiki.tickets.index'. The ModuleNotFoundError
was silently caught by the except Exception block, causing index injection
to be skipped — WikiBuildIndex tests expected 2 messages but got 1.
- Fix _format_index_block import in build.py: was importing from
  non-existent wolfharness.capabilities.wiki.index, corrected to
  wolfharness.capabilities.wiki.tickets.index
- Fix mypy attr-defined error in viking/wiki_build.py re-export:
  place type: ignore comment before noqa for correct mypy parsing
- mcp_server_cap.py: take wolf1069b/main tool filtering (before imports)
- resource_capability.py: take wolf1069b/main MCP Resource refactor
  (3 tools: list_mcp_resources, list_mcp_resource_templates, read_mcp_resource)
- manager.py: take wolf1069b/main supports_resources() negotiation,
  remove lazy mode, restore tool_prefix fallback
- test_resource_capability.py: take wolf1069b/main test assertions
- test_pool_skills.py: update _FakeClient with connected + supports_resources
  for compatibility with wolf1069b/main setup_server
_ticket_evidence() blindly merged cited_references[].uri with every
evidence entry. evidence is a free-form user expression that may carry
provider URIs or plain audit text (e.g. 'QuotedText: ...'), which the
TicketEngine's _validate_opa_uris rejects as invalid evidence URIs and
fails the whole OPA/OPS ticket submission.

Extract the engine's OPA/OPS URI format rule into is_valid_op_uri
(shared by _validate_opa_uris) on OPAMixin and TicketEngine. Ticket
closures capture that predicate and pass it to _ticket_evidence, which
keeps cited_references[].uri verbatim and only admits evidence entries
that pass. Engines without the predicate keep the historical blind merge.

Regression tests cover text rejection, URI extraction, and back-compat.
…ools

The tool_prefix config was not applied in the get_capabilities() path —
MCPToolset was never wrapped in PrefixedToolset and allowed_tools were
not prefixed. This caused test_tool_prefix_namespaces_mcp_tools to fail.

Fix: wrap MCPToolset in PrefixedToolset when tool_prefix is set (matching
the McpServerCap provider path, RFC-0058) and prefix enabled_tools for
the allowed_tools advertisement.
@Million-mo

Copy link
Copy Markdown
Collaborator

Code Review — PR #382 (APC/Viking archive + Wiki knowledge-base subsystem)

Scope: 152 files, +40,654 / −2,001. Core framework changes + a new capabilities/wiki/ subsystem (67 Python files, ≈31K lines).

Verdict: Needs work — do not merge as-is. The framework-side design is solid in places, but three of the headline reliability guarantees are actually broken in practice, and the new wiki subsystem contains two already-diverged ~3K-line ticket engines with essentially no test coverage. Per the project convention that all changes be tested, this PR is not in a mergeable state.


🔴 Critical (must fix before merge)

1. DCP non-native fallback discards the compaction result (capabilities/dcp/capability.py:914-924)

pipeline.apply(request_messages) output is only used in a log line — before_model_request returns the DCP-pruned messages, never the pipeline output. For any agent without native_agent deps (HAgents / non-native nodes), auto-compaction is a silent no-op that still runs a potentially LLM-based Summarize step and then sets critical_compacted=True, blocking all later attempts in the episode.
Fix: return dataclasses.replace(request_context, messages=compacted_messages) in the else branch, or skip the pipeline and log clearly.

2. ACP archive retry-on-failure defeated by an over-narrow exception tuple (src/wolfharness_server/acp_server/viking_archive.py:18,128-145)

_ERROR_TYPES = (RuntimeError, OSError, TimeoutError, ValueError, TypeError, ImportError). The OpenViking SDK's own error classes and httpx.TimeoutException/ConnectError are not subclasses of any member — so a network timeout (the most common failure) escapes: the popped batch is silently lost and the background task exception is never retrieved. This defeats the PR's core selling point (retain pending records on flush failure).
Fix: add httpx.HTTPError + the SDK error base to the tuple, and register a task done_callback that logs non-tuple exceptions.

3. Auto-ingest retry reuses the same session_id → duplicates or permanent failure (capabilities/viking/__init__.py:2163-2187 + ingest.py:306)

The failed batch retains the original session_id; retry calls create_session again. If the first attempt failed after create_session succeeded (partial add_message), the retry either raises "session already exists" (batch stuck forever in _failed_ingest_batches) or re-adds the already-committed prefix → duplicate transcript entries. No cursor/acking.
Fix: mint a fresh session_id per retry (f"ingest-{uuid.uuid4().hex}"), mirroring _flush_tail.

4. TruncateToolCallInputs truncates string-encoded tool args mid-JSON (messaging/compaction.py:421-428)

Model tool-call args are a JSON string; cutting at a byte offset yields invalid JSON. Downstream sanitize_tool_call_args_in_messages then replaces unparseable args with {} — the tool call is neutered to a no-arg call and re-executed with unintended semantics (or hard-rejected by vLLM-style backends).
Fix: drop the str branch; truncate only dict/list — cut string values at JSON-safe boundaries (parse + re-dump).

5. Two ~3K-line ticket engines have already diverged semantically (wiki/tickets/opa.py 2,979 lines vs ticket_engine.py 3,221 lines)

The files are ~87% character-identical, but the divergence is semantic, not cosmetic:

  • "One feedback = one OPA record" guarantee lost: opa.py:690-735 skips dedupe and appends a uuid4 suffix per submission; ticket_engine.py dropped uuid4 entirely (0 occurrences) — repeated/multi-person expert feedback merges into a single record in the standalone engine.
  • External-expert OPS/OPL uniqueness lost: opa.py:1059-1101, 1727-1798 mint unique ids; ticket_engine.py:1144-1178, 1790-1855 dedupe unconditionally → concurrent expert submissions collapse.
  • _record_from_content parses different sections in each → the same files produce different records via get_ops/apply_opl.
    Fix: extract one canonical internal service; WikiBuildTools composes it and TicketEngine becomes a thin store binding. Never maintain two 3K-line copies.

🟠 Major (recommend fixing before merge)

Framework / Viking side:

  • 6. _schedule_ephemeral_cleanup (team_comm_capability.py:2957-2962) was missed in the migration to _get_team_base_dir — when team_base_dir is carried in session metadata (the documented preferred path), cleanup resolves to tempfile.gettempdir() and never finds the task files, leaving ephemeral-worker tasks orphaned on the board.
  • 7. _extract_full_trace (viking/ingest.py:106-192) has zero production call sites (test-only). Also: outcome="denied" is reported as "completed", and _serialize_tool_output can emit multi-MB base64 for BinaryContent.
  • 8. MCP tool_prefix dedup runs in the provider path but is bypassed in the lazy session-config path (mcp_server/manager.py:419-425 vs 798-803) — two session-scoped servers with the same tool_prefix produce identical prefixed names: the exact collision the dedup was built to prevent.
  • 9. DCPState.critical_compacted is not round-tripped by from_dict (capabilities/dcp/state.py:164-225) — a restored session re-fires compaction on the next CRITICAL, defeating the once-per-episode guarantee.
  • 10. api_key is a plain dataclass field (viking_archive.py:35-36, only _client is repr=False) → leaks via repr/logging; and os.path.expandvars is applied to api_key (config/capabilities.py:252-262) — a key containing a literal $ gets mangled.
  • 11. Renewing an expired lease with claim=True increments attempt/task_retry_count every TTL (file_team_state.py:798-804) → normal lease renewal inflates retry metrics and the audit attempt field.
  • 12. Claim token is not enforced when the caller provides none (file_team_state.py:791-797); currently unreachable from team_comm member tools, but the wiki harness calls update_task through the same API — a missing token should be rejected whenever a lease is active.
  • 13. Restored member sessions are locked out of active leases until TTL expiry (team_comm_capability.py:1511-1515,1583) — session_tokens live in session.metadata and are lost on restore. Recovery hint: when role/owner/current_member already match, restore the token from existing_task.lease_token.
  • 14. wiki/** is excluded from mypy and heavily exempted in ruff (pyproject.toml:385-389, ruff.toml:190-223 — E501, BLE001, PLR2004/0911/0915, E402, TRY301, RUF012…) → ~20K of 40K inserted lines ship unchecked. The exemptions should be time-boxed; at minimum keep the shim layer (viking/wiki_build.py, viking/ticket.py, viking/wiki_index.py) lint-checked.
  • 15. task_list empty-result format changed (team_comm_capability.py:1246-1258) from <task_list>(empty)</task_list> to …</task_list>\n0 tasks — verify no agent prompt/test asserted the old token.

Wiki subsystem:

  • 16. Fallback engine apply_opl is a stub that returns content unchanged (ticket_engine.py:199-201) yet reports apply_status: applied (:2257-2266) — an OPL carrying only candidate_operations is "applied" without any change. Return needs_review or raise instead of falsely reporting success.
  • 17. create_raw_reader (storage/__init__.py:219-222): the return LocalFS(...) line precedes the raw-viking branch, so the default WIKI_STORAGE_BACKEND=viking reads raw chapters from local disk and the remote-raw path is dead code. Drop the early return.
  • 18. DualFS.commit_many (dual_fs.py:99-130): local commit is atomic, but the remote mirror is sequential write_many inside except Exception — a mid-set remote failure leaves the remote permanently partial with no drift detection, retry, or backfill tool, violating the atomic-commit contract.
  • 19. register_redirect lost-update race (storage/storage.py:1218-1233): read-modify-write without lock/merge — two concurrent move_entity calls silently drop one redirect. Also get_backlinks/resolve_redirect call json.loads unguarded → JSONDecodeError at the agent on a truncated index.
  • 20. Regex-heuristic task validation (harness/team_workflow_guard.py: ~20 strict regexes) — any LLM wording drift triggers a hard ModelRetry (conductor rejection loops); _PHASE_DONE_RE is copy-pasted in team_wake.py:55 and team_workflow_guard.py:97. Validate against structured args; keep prose checks as soft warnings.
  • 21. WikiStore discovery cache is keyed per-process (storage/storage.py:140-154,864-906, 1-60s window) — two team workers can disagree on "does entity X exist" within the window.
  • 22. entities/finalize.py:871,940 reaches into self.store._fs (private), swallows all upload errors via broad except Exception, and _retry_remote_sync blocks on ThreadPoolExecutor.result(timeout=3600) (:951) — a finalize gate that can hang for an hour.
  • 23. Finalize receipts live in session metadata (harness/team_workflow_guard.py:174-200) and are popped on any non-finalize tool call — an interrupted conductor re-spawn is locked out of phase=done permanently, while a stale receipt from an obsolete build still authorizes done. Persist in the durable checkpoint and compare against it.

🧪 Test coverage

Area Coverage Notes
ACP archive encoding ✅ Good 10 tests, but client is 100% AsyncMock — real SDK round-trip never exercised; no VCR cassettes (repo has pytest-recording + vcr/ convention)
MCP tool_prefix / env expansion ✅ Good Strong asserts
team_comm / file_team_state ✅ Good 19 tests on real FileTeamState via tmp_path
DCP auto-compact ✅ Good 4 tests, real messages
Wiki core engine (≈32K lines) None planning/, entities/, io/, storage/, harness/, tickets/ all untested
Wiki protocol surface ⚠️ Partial FakeWikiBuildTools + sys.modules injection; the real 810-line WikiBuildTools is never imported
Wiki ticket internals ⚠️ 5 tests test_ticket_reference_scopes.py bypasses __init__ via TicketEngine.__new__ and pokes private methods

Other test-quality notes: test_acp_prompt_metadata.py drains background tasks with asyncio.sleep(0) polling (timing-sensitive); wiki tests violate the "test through public APIs" convention; the manual live E2E reads ~/.openviking/ovcli.conf.

Most concerning gaps: entity planning/relation-closure, finalize/audit, team_wake/guards, and the ticket lifecycle (create → evaluate → close through __init__) have zero tests — the ~8 fix(wiki:) commits in this PR are verified only by source review.


🏗️ Architecture assessment

The storage layer is genuinely well-engineered: clean FSBackend abstraction, genuinely atomic local writes, bounded retry/backoff on the remote path, hash-preconditioned batch writes, read-your-write durability polling, optimistic-lock OPL application, and a thorough finalize gate (snapshot-consistency audit, checkpoint idempotency). The layering is mostly respected.

The dominant architectural debt is duplication at the worst possible scale: two 90%-identical ticket engines that have already drifted semantically, plus the god-mixin bundle (WikiBuildTools = WikiBuildDeps + OPAMixin + FinalizeMixin + RelationMixin + AuditMixin) — the "decoupled" standalone engine is really a second, subtly different implementation, not a thin binding. A second theme: process-local state pretending to be durable (session-metadata receipts, per-process discovery caches, swallowed remote-mirror failures) — all fine in a single-process demo, silently broken in the multi-agent team deployment this PR is explicitly built for.

Positives: the lease handshake + session-cached tokens, the shared .create.lock capacity guard, and the QUEUE-mode notification fix are correctly reasoned, and the failed-batch retention design for auto-ingest/archive is the right idea.


✅ Recommendations

  1. Fix Criticals 1–5 before mergefeat: implement Unified Model Selection Configuration #5 (engine dedup) is the largest item and the most urgent: the code has already drifted, and the cost only grows.
  2. Gate the wiki subsystem as experimental/default-off with time-boxed mypy/ruff exemption removal.
  3. Add a minimum wiki test suite before merge: ticket lifecycle, entity finalize, and storage round-trips.
  4. Fold Majors 6–23 into follow-up issues, prioritized by reliability/security impact.

Note: review performed statically via git diff main..pr-382; test suite not executed during review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants