Skip to content

fix: register /acp-cache command in the pi/omp thin plugin (#800) - #974

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-19_acp-cache-command
Sep 19, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-09-19_acp-cache-command

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What & why

Follow-up report on #800: /acp-cache worked for the model (the acp_cache tool) but not for humans in launcher mode (bili pi) or native mode (bare pi with the plugin installed) — the command was only ever built into billion-context-pi#449 (merged, unreleased), never into this repo's shared thin-plugin factory.

Root cause: createBiliPlugin() (src/agent/pi.ts) — which serves launcher mode AND native mode (src/agent/pi-native.ts loads the same factory) AND omp — registers the ACP tools from the proxy manifest (incl. acp_cache) but registered only the /acp command. No human entry point existed.

Changes

  • src/agent/pi.ts: register /acp-cache next to /acp. It forwards acp_cache over the existing /__bili/plugin/tool endpoint → identical report text on both sides (both paths hit handleAcpCache). Rendered as a persistent custom message (customType: bili-acp-cache, visible in TUI + pi-web) with ui.notify() fallback for hosts without sendMessage. The pi plugin: bili plugin install pi silently drops the npm:billion-context-pi entry; /acp "no proxy detected" warning misleads non-proxy users #788 no-proxy warning wording is extracted into a shared helper (byte-identical strings, existing tests untouched).
  • src/acp-panel.ts: wrapCacheReport() wraps the report in [acp-cache][/acp-cache] markers; isAcpPanelText recognizes them so the proxy strips the persisted report from model context like the /acp panel — at all three strip call sites (anthropic/openai messages + responses input). Whole-message-match discipline preserved: only the WRAPPED form is stripped; an unwrapped report (e.g. one the model produced via the acp_cache tool, or typed by a user) survives. Explicit markers were chosen because the report shape is kernel-owned and variable-length (LINE ITEMS table up to the 512-line sample cap) — anchoring on it would couple the strip predicate to acp-kernel renderer output.
  • CONFIGURATION.md / CONFIGURATION.zh-CN.md: pi/omp launcher lines mention the new command (en/zh kept in sync).
  • Tests: 10 new — strip signature (wrapped / unwrapped / suffix follow-up / whitespace / quoted markers; both message shapes) and handler paths (forwarding with conversationId, sendMessage shape, raw notify fallback, no-proxy warning, proxy-side error).

Out of scope (follow-ups, reported in #800)

  • dsh: its cordis plugin registers commands via a different mechanism and tool forwarding has no latest-session resolution (unlike status), so dsh /acp-cache needs a small proxy-side addition — separate change.
  • opencode: deliberately registers no commands (API instability; users call the acp_status/acp_cache tools).

Pre-flight (base = current master b6c2ef3)

  • npm run typecheck — clean
  • npm test — 1910 pass / 0 fail (2 skipped = e2e gate)
  • npm run build — success; verified dist/agent/pi.js contains the acp-cache registration + wrapper markers

Part of #800

@github-actions

github-actions Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-19_acp-cache-command (224e81e)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-974

Each push to this PR publishes a new version under the pr-974 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr974.tgz
npm install -g package

Install instructions are refreshed on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Reviewing PR #974 (2026-09-19_acp-cache-command) now: pulling the branch, checking CI, rebasing against current master (which moved b6c2ef32d1d502 since pre-flight), and re-running typecheck/test/build. Will report findings here shortly.

The cache-reconciliation feature shipped the model-side acp_cache tool
(manifest + wire) but never gave humans a command in the shared thin
plugin factory — so /acp-cache existed only in billion-context-pi
(unreleased), not in `bili pi` launcher mode or pi native mode.

- src/agent/pi.ts: registerCommand("acp-cache") next to /acp — forwards
  acp_cache to the proxy (identical report on both sides), renders it as a
  persistent custom message (customType bili-acp-cache), notify() fallback.
  One registration covers launcher mode + native mode (pi-native.ts loads
  the same factory) + omp. The #788 no-proxy warning is extracted into a
  shared helper, wording unchanged.
- src/acp-panel.ts: wrapCacheReport() markers ([acp-cache] ... [/acp-cache])
  + isAcpPanelText recognition, so the persistent report is stripped from
  model context like the /acp panel (whole-message match discipline; an
  unwrapped report — e.g. one the model produced via the acp_cache tool —
  is preserved).
- CONFIGURATION.md/.zh-CN.md: pi/omp launcher lines mention the new command.
- tests: strip signature cases (wrapped/unwrapped/suffix/whitespace/quote,
  messages + responses shapes) and handler paths (forwarding, sendMessage,
  raw notify fallback, no-proxy warning, proxy error).

Part of #800
@ranxianglei
ranxianglei force-pushed the 2026-09-19_acp-cache-command branch from 0480651 to 224e81e Compare September 19, 2026 01:49
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review complete — rebased onto current master, everything green, ready for a human merge.

What I did

  • Master had moved since your pre-flight base (feat: launcher max-output env channel (BILI_LAUNCHER_MODEL_MAX_OUTPUTS) #973 merged, b6c2ef32d1d502). Zero file overlap with this PR, so I rebased the branch cleanly and force-pushed the updated head: 0480651224e81e.
  • Re-ran the full pre-flight on the rebased head locally: typecheck clean, 1920 pass / 0 fail / 2 skipped (e2e gate), build success, and dist/agent/pi.js does contain the acp-cache registration + wrapper markers (grep-verified).
  • GitHub CI on 224e81e: all 8 checks success (ubuntu/windows × node 22/24, codex e2e, version-guard, build-artifact). PR mergeable_state is now clean.

Code review findings (all claims verified against the tree)

  • One registration covers all three surfaces: createBiliPlugin() (src/agent/pi.ts:265) serves launcher mode, native mode (src/agent/pi-native.ts:130) and omp (src/agent/omp.ts:3) — confirmed.
  • Proxy-side path confirmed end-to-end: POST /__bili/plugin/toolhandlePluginTool (src/plugin.ts:697) → allowlist gate (isProxyToolFor, acp_cache ∈ kernel ACP_TOOL_NAMES) → executeProxyTool (src/loop/core.ts:173) → handleAcpCache(session) (src/cache-ledger.ts:267). Same handler as the model-side tool, so the identical-report-text claim holds.
  • Strip coverage: all three call sites go through isAcpPanelTextsrc/server.ts:1979 + :2123 (anthropic/openai messages) and :2318 (responses input). Whole-message discipline holds: only the WRAPPED form strips; unwrapped reports (model tool output, user-typed) and wrapped-report-plus-follow-up messages survive (both asserted by the new tests). Anthropic tool_result blocks can't be caught because messageText() returns undefined for non-text parts.
  • forwardTool(proxyBase, conversationId, "acp_cache", {}) matches the signature at src/agent/shared.ts:155; the pi plugin: bili plugin install pi silently drops the npm:billion-context-pi entry; /acp "no proxy detected" warning misleads non-proxy users #788 warning extraction is byte-identical; console.error in pi.ts follows the file's existing convention (it runs host-side, not a server module).
  • Docs en/zh in sync; diff is tight — 6 files, 258 insertions, every line on-topic, no whitespace/lockfile churn.
  • Tests are deterministic (listen(0) ports, fake proxy server) and cover exactly the matrix you listed.

One minor note (non-blocking): the marker-based predicate means a hand-typed message whose entire trimmed content starts with [acp-cache] and ends with [/acp-cache] would be stripped. That's the inherent trade-off of plugin-owned markers vs anchoring on kernel renderer output (already documented in the acp-panel.ts header comment, consistent with the existing panel design), and the risk is negligible — the markers never enter model context, so there's no echo loop.

Per §7.4 this stays a human merge (wire/message-shape change — the strip predicate affects both modes' request pipeline). Nothing left for me to fix here.

中文摘要:已把分支 rebase 到最新 master(#973 之后)并重新跑完全部验证(typecheck/测试 1920 通过/构建/CI 8 项全绿),代码审查确认双端同走 handleAcpCache、三处剥离点全覆盖、整消息匹配纪律保持,无阻塞问题,可以合并。

@ranxianglei
ranxianglei merged commit c2b063e into master Sep 19, 2026
8 checks passed
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