Skip to content

feat(viking): archive full ACP traces, MCP tool namespacing, team-mode hardening - #402

Closed
Million-mo wants to merge 2 commits into
mainfrom
refactor/382-framework-only
Closed

feat(viking): archive full ACP traces, MCP tool namespacing, team-mode hardening#402
Million-mo wants to merge 2 commits into
mainfrom
refactor/382-framework-only

Conversation

@Million-mo

@Million-mo Million-mo commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Removed (business code — should go to separate repo/package)

  • src/wolfharness/capabilities/wiki/ — 68 files, 32K+ lines (wiki build system, ticket engine, OPA/OPS/OPL workflow, domain schema)
  • src/wolfharness/capabilities/viking/wiki_build.py, wiki_build_tools.py, wiki_index.py, ticket.py — wiki shims
  • templates/default_schema.yaml — excavator maintenance domain schema
  • Wiki-specific changelog entries, ruff/mypy exclusions

Kept (framework changes)

  • ACP trace archivingviking_archive.py, aligned to readable session transcripts
  • MCP tool prefix — per-server namespacing in config
  • Viking capability — index injection, trace reliability, tool filtering, URI prefix restrictions
  • Team-mode — atomic task binding, member subtasks, lease tokens, model config, state directory propagation
  • FallbackModelConfig — nested YAML discriminant models
  • DCP capability — prune/restore tools
  • Compaction — message count compaction, session pool config
  • Resource resolver — agent-scoped URI scheme owners

Stats

80 files, +7,758 / −2,000 (vs original 152 files, +40,654 / −2,001)

Test

ruff check src/ — all checks passed.

Note

#399 / #400 / #401 depend on this PR. Review and merge this first.

…e hardening (#382)

Framework-level changes from PR #382:
- ACP trace archiving aligned to readable session transcripts
- MCP tool prefix configuration (per-server namespacing)
- Viking capability: index injection, trace reliability, tool filtering
- Team-mode: atomic task binding, member subtasks, lease tokens
- FallbackModelConfig with nested YAML discriminant models
- DCP capability improvements
- Compaction, session pool, resource resolver enhancements

Business-domain wiki capability code (32K+ lines, 68 files) excluded —
belongs in downstream project, not framework core.
The except Exception block logged but swallowed startup errors (e.g.
OSError: Address already in use), causing silent exit 0. Add raise
after log.exception to propagate to the CLI.

Fixes CI failure in test_start_async_propagates_serve_oserror.
@github-actions

Copy link
Copy Markdown

Review — PR #402: framework-only re-submission

Reviewed against AGENTS.md, tests/AGENTS.md, and changelog/README.md conventions. Static analysis + targeted code reading (no test runner available in this environment, so test claims below are high-confidence inference — please run them before merge).


🔴 Blockers

1. test_acp_server_error_propagation.py will fail against the shipped code
tests/servers/acp_server/test_acp_server_error_propagation.py:19-42 asserts pytest.raises(OSError, match="Address already in use") from _start_async(), but src/wolfharness_server/acp_server/server.py:347-348 still does except Exception: self.log.exception("ACP server error") with no raise. structlog.exception() logs without re-raising, so _start_async returns normally and the test gets "DID NOT RAISE". The changelog changelog/unreleased/2026-08-27-serve-acp-silent-startup-failure.md claims "Startup errors now propagate to the CLI, which prints the error to stderr and exits non-zero" — that fix is not in this diff (serve_acp.py is untouched, and _start_async still swallows). Either the fix was dropped in the re-submission or the test/changelog are aspirational; both cannot be true. git log also shows this exact commit was merged once already — please run uv run pytest tests/servers/acp_server/test_acp_server_error_propagation.py to confirm.


🟠 Should fix (P1)

2. Business documents + binary logos shipped in a "framework-only" PR
WolfHarness情况说明_一页纸.md (51 lines), WolfHarness项目情况说明与价值评估报告.md (363 lines), and assets/logo/*.png (1 MB + 2.5 MB binaries, one a design draft) are SANY-internal business briefings with zero relation to framework code. This directly contradicts the PR's stated purpose of stripping business-domain content, violates the repo's English-only changelog/doc convention, and is a confidentiality risk in a public repo. Remove from this PR.

3. Dead/misleading code path: latest_user_prompt_text / deps=acp_session
handler.py:704-705 writes acp_session.latest_user_prompt_text / latest_user_prompt_message_id — nothing in src/ ever reads them (grep-confirmed). handler.py:724 passes deps=acp_session into send_message(), but RunHandle._inject_agent_context() (run.py:328) unconditionally replaces run_ctx.deps with an AgentContextDeps before the turn executes, so the session object can never reach capabilities. Either wire up the intended consumer (the viking profile path already reads the prompt from request_context.messages instead) or drop the dead state.

4. New production code violates the "no getattr/hasattr" red line

  • viking_archive.py:60-77 — 10× getattr(config, ...) against a typed ACPVikingArchiveConfig; also getattr(self._client, "close", None) (:158), hasattr(response, "json") (:179), hasattr(update, "model_dump") (:320), and a call to the SDK's private client._request("GET", "/health") (:178). 21 Any annotations in the module.
  • handler.py:60_content_items_to_text uses getattr(item, "text", None); isinstance(item, TextContentBlock) would be type-safe.
  • team_comm_capability.py:401-419_session_has_live_run() is built entirely on getattr/callable probes.
  • viking/ingest.py, viking/tools.py (_is_busy_error), resource_resolver.py:287,310 — additional getattr in new code.
    Some of these accommodate test doubles, but the ban is explicit in AGENTS.md; worth a follow-up to type them properly.

5. Dead config left over from the reverted PR

  • pyproject.toml:448-449 — mypy ignore_missing_imports for xeno_adp_agentic.* / xeno_adp_harness.*; nothing in src/ or tests/ imports them (grep-confirmed).
  • ruff.toml:156 — per-file-ignore for src/wolfharness/capabilities/viking/ticket.py, which does not exist in this branch.
  • .opencode/opencode.jsoninvalid JSON ({"$schema": "...",} trailing comma, no trailing newline), an empty file that breaks opencode config parsing. If not intended, delete it.

🟡 Nits / observations (P2)

6. Silent exception suppression on the critical event path. run.py:544,586-592 wraps every agent.event_handler() dispatch in contextlib.suppress(ValueError, TypeError, RuntimeError, KeyError, AttributeError) with no logging — a bug in any event handler becomes invisible. session_controller_runs.py:184-215 has the same pattern but logs at debug. Recommend logging the first occurrence or narrowing the exception set.

7. Missing changelog entries for: DCP auto_compact_on_critical + prune/restore + 4000-char distill guard, TruncateToolCallInputs compaction step, resource-resolver agent-scoped URI scheme owners, new env vars (WOLFHARNESS_DATABASE_URL / WOLFHARNESS_DATA_DIR / WOLFHARNESS_LOG_DIR), viking_upload_tree / viking_link_relations tools, write_allowed_uri_prefixes, and team lease tokens / member_can_create_subtasks / task idempotency keys. Per changelog/README.md these should be added or folded into existing entries.

8. Changelog format deviation. 2026-08-19-atomic-team-member-task-binding.md uses YAML frontmatter instead of the # Title heading format required by changelog/README.md.

9. New ACP trace-archiving subsystem is undocumented. viking_archive.py (384 lines) + the session_pool.acp_viking_archive config aren't in src/wolfharness_server/AGENTS.md's "Where To Look" table, and no docs/explanation/ page covers ACP trace archiving or the MCP tool_prefix feature.

10. event_mapper sanitizer behavior change. sanitize_tool_call_args_in_messages now replaces all invalid-JSON string args with {} (previously left untouched). Deliberate and documented (ModelRetry recovery), but it silently rewrites history on restored sessions — consider a per-tool opt-out.

11. viking_archive.py:136-145 — flush-failure recovery re-buffers the whole popped batch, so a mid-batch failure duplicates already-written messages on retry. Acceptable for best-effort archival, but a next_message_index resume would avoid dupes.

12. uv.lock churn (+1644/−1644) — verified benign: the only real change is openviking-sdk 0.1.7 → 0.1.8; the rest is bulk upload-time metadata and marker re-resolution.


✅ What's good

  • Team-mode hardening is genuinely well-tested — lease tokens, idempotency keys, initial-task atomic binding, and shutdown auto-release each have dedicated tests (~19 new in test_unit_capability.py), including the idempotency_key: "0" edge case.
  • get_live_run repair (session_controller_runs.py:62-83) is a clean consolidation; the annotation-only mixin stub (session_controller_agent.py:64-67) to avoid MRO shadowing is a nice touch.
  • ACP archive wiring goes through one chokepoint (_archive_and_send_update), applied consistently to subagent-completion, converted events, and user chunks; chunk-merging/ack-filtering has focused unit coverage.
  • 7 of the 8 new changelog entries are accurate and well-written; TruncateToolCallInputs, FallbackModelConfig, storage env vars, and the serve-acp startup test (once fixed) all have direct tests.

Bottom line: the underlying feature work is solid, but this PR ships with a likely-failing regression test, a changelog describing an unimplemented fix, dead config/code left over from the revert, and unrelated internal business documents that contradict the PR's own "framework-only" framing. Recommend: fix/ship the startup-error propagation (or drop the test + changelog), remove the business docs/logos, delete the dead config and dead latest_user_prompt_*/deps path, and add the missing changelog entries. Then this is good to merge ahead of #399/#400/#401.

@github-actions

Copy link
Copy Markdown

Review posted: #402 (comment)

Synthesized from two specialist reviews (code + docs) with my own verification of the P0/P1 claims. Key findings:

  • Blocker: test_acp_server_error_propagation.py asserts OSError propagates from _start_async, but server.py:347-348 still swallows exceptions — the changelog's "startup errors now propagate to the CLI" fix isn't in the diff, so the test will fail.
  • P1: Two Chinese business documents + 3.5 MB of logo binaries in a PR whose stated purpose is stripping business code; dead latest_user_prompt_*/deps=acp_session path; extensive getattr/hasattr in new code (violates AGENTS.md red line); dead config (invalid .opencode/opencode.json, ticket.py/xeno_adp ignores for non-existent files).
  • P2: missing changelog entries, silent contextlib.suppress on the event path, undocumented archive subsystem, format deviation in one changelog file.
  • Positives: team-mode hardening is well-tested, get_live_run repair is clean, archive wiring is a single chokepoint.

New%20session%20-%202026-08-28T08%3A06%3A46.023Z
opencode session  |  github run

@Million-mo Million-mo closed this Aug 28, 2026
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.

1 participant