Skip to content

fix(v2): resolve host permission rules per real tool name - #469

Open
ranxianglei wants to merge 3 commits into
2026-09-17_v2-basefrom
2026-09-29_v2-per-tool-permission
Open

ranxianglei wants to merge 3 commits into
2026-09-17_v2-basefrom
2026-09-29_v2-per-tool-permission

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Problem

Issue #459 (MAJOR): on the V2 line every ACP tool resolved its permission under the hard-coded literal "compress" — resolveEffectiveCompressPermission() called resolveV2Permission(v2Rules, "compress") regardless of which tool executed, and all five tools declared inert options.permission: "compress". A granular ruleset therefore produced the opposite of its intent: [{action:"compress",effect:"ask"}] refused four unrelated read-only tools with a message about the compress permission, while compress itself was never actually asked.

Fix

  • lib/host-permissions.ts — resolveEffectiveCompressPermission() gains an optional toolName parameter (default "compress") consulted only by the V2 ordered-rules branch. The legacy V1 branch stays keyed to the compress config entry (V1 host configs have no per-tool entries for ACP tools).
  • lib/v2/tools.ts — resolveToolPermission() receives the definition's real name; options.permission now carries each tool's own name (kept rather than deleted: measured inert today, but if the host ever honors it as the evaluation action, per-name values make granular host rules work end-to-end instead of mis-declaring compress); fail-closed ask/deny messages and result metadata now name the actual tool.
  • README.md — one sentence documenting per-tool rule resolution on V2.
  • Devlog: devlog/2026-09-29_v2-per-tool-permission/.

Behavior changes (old → new)

  • Every non-compress V2 tool (decompress, search_context, acp_status, acp_context_recap): resolved under action "compress" → resolved under its own name. Rules targeting those names now take effect; rules that previously leaked onto them no longer do. This is the fix.
  • Refusal metadata permission field / message text for non-compress tools: hard-coded "compress" → the real tool name (previously misleading).
  • Unchanged on purpose: behavior for compress itself is byte-identical (resolution input, message text, metadata); OpenCode ordered last-match semantics preserved (the issue's literal example [{compress:ask},{*:deny}] resolves to deny-for-all under last-match because the later * wins for every action — express intent by ordering the specific rule last); ACP's own compress.permission config gate remains a global gate over all ACP tools; fail-closed ask preserved (OpenCode 2.0.3 exposes no native permission-request creation API to server plugins — DESIGN §8.1); V1 path and nudge gating (state.compressPermission) untouched.

Tests

  • New regression suite V2 resolves host rules per real tool name so one tool's rule never leaks (issue #459): deny/ask isolation across all five tool names, metadata accuracy, reverse granularity, including the issue's failure scenarios. Verified to FAIL against pre-fix code (5 of 12 failing pre-fix, 12/12 post-fix).
  • Updated 3 existing tests whose rules targeted "compress" while the fake tool was named "test" (they now target the tool's own name — which is exactly what the fix makes meaningful), plus the registration options assertion per tool name.
  • Full suite: 1411 pass / 1 fail — the failure is tests/soft-block.test.ts, pre-existing in this sandbox only (it mkdirs into /tmp, which is read-only here); unrelated to this diff. Typecheck + build clean.
  • Independent review: APPROVE (no missed callers; byte-identity of compress confirmed).

Closes #459

Issue #459: the V2 resolver hard-coded the literal "compress" so all five ACP tools shared one permission decision, and options.permission declared an inert "compress" for every tool. Resolve each tool under its own name (OpenCode ordered last-match semantics preserved), declare options.permission per tool name, and make fail-closed ask/deny messages plus result metadata name the actual tool. Behavior for compress itself is byte-identical; V1 path and nudge gating untouched.
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review started: pulling PR #469 branch, reading original issue #459, running the full review pass (issue direction → regression → code → tests → diff cleanliness). Will report findings shortly.

Review follow-up on #469: the PR documented per-tool rule resolution in
README.md only; mirror the same sentence in README.zh-CN.md so both
readings stay aligned.
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review complete: APPROVE on code quality — but flagging a routing conflict you already decided. Details below; verdict and a question for you at the end.

① Original issue (#459) — direction valid, root cause correct

Reproduced the failure mode against the base commit (2026-09-17_v2-base @ 1fe36e09): with the PR's new test suite applied to pre-fix code, 5 of 12 tests fail (registration options, deny/ask isolation across tool names, ordered-rules override, agent-allow override, the #459 regression suite itself). The root cause is exactly as reported — resolveEffectiveCompressPermission() resolved every V2 tool under the hard-coded literal "compress", so rules targeting other tool names never applied while compress-keyed rules leaked onto four unrelated read-only tools. The fix hits the root cause (resolution key), not the symptom. The last-match nuance disclosed in floor 3 is correct and I verified it in code: resolveV2Permission() iterates rules backward, so OpenCode's ordered last-match semantics are preserved.

② Regression review — existing behaviors on touched paths

Existing behavior Verdict
ACP config gate compress.permission === "deny" blocks all five tools Preserved — short-circuits before rule lookup, untouched
compress tool behavior under any ruleset Byte-identical — resolution input still "compress" (default param), message text and metadata verified char-for-char against old constants
Nudge gating state.compressPermission (lib/compress-permission.ts:20) Unchanged — uses default "compress", intended per REQ §3
V1 legacy branch (compressDisabledByOpencode) Untouched, still keyed to the compress config entry
Fail-closed ask/deny on V2 (no interactive prompt) Preserved — only message wording/metadata changed for non-compress tools
Ordered last-match rule semantics Preserved (iteration order unchanged)
Registration options.codemode: false; subagent/lifecycle/bili-proxy/invalid-input paths; exception deny-all fallback Untouched
options.permission value: "compress" → own name Intentional change, disclosed in PR body (measured inert today, future-proof if host ever honors it)

No regressions found. All intentional changes are disclosed in the PR body with old→new and rationale.

③ Code review

Clean. Optional toolName param with "compress" default keeps the signature backward-compatible; only two call sites exist and both pass explicitly (grep-verified no missed callers — lib/compress/pipeline.ts/decompress.ts ask({permission:"compress"}) calls are the V1 shared path, no-op on V2 via the stub in v2/tools.ts). No casts/assertions added; npm run typecheck clean on the branch.

④ Test effectiveness

Real tests that bite: I ran the new suite against pre-fix code and watched it fail (5/12), satisfying the "must fail when the bug is present" bar. Assertions are meaningful — content equality (executed:<name> vs /disabled/i) plus metadata.permission deep-equal per tool name, covering deny/ask isolation in both directions, metadata accuracy, and reverse granularity with unambiguous orderings. The 3 updated existing tests now target the fake tool's own name ("test") — exactly what makes them meaningful post-fix. Minor gap (acceptable): no direct test of cross-tool-name last-match ordering, but that's platform semantics pinned indirectly by the explicit-ordering cases.

⑤ Diff cleanliness

6 files, all on-topic (2 source, 1 test, 1 README, 2 devlog). No version bump (correct for a non-release branch), no unrelated churn. Devlog REQ+WORKLOG present; no DESIGN.md needed (no architecture/data-flow change).

Verification & caveats

  • Local: typecheck clean; full suite 1411 pass / 1 fail — the fail is tests/soft-block.test.ts hitting read-only /tmp in this sandbox (pre-existing, unrelated, matches your claim). Build runs clean via npm ci prepare.
  • CI note: GitHub Actions shows zero check runs for this head commit because all workflows trigger only on pull_request → master, and this PR targets v2-base. Local verification above is the only gate here.
  • I pushed one small fix to the PR branch: eb24c6a0 docs: sync per-tool permission resolution note into Chinese README — the PR documented per-tool resolution in README.md only; I mirrored the same sentence into README.zh-CN.md so the two readings stay aligned. Labeled commit, Prettier-checked, nothing else touched.

⚠️ Routing conflict — needs your call

Floor 5 on #459 (your note, after this PR opened) says the in-repo V2 port is retired per #442 and OpenCode 2.x findings go to billion-context. This PR fixes retired code in this repo. Per your stated policy my recommendation is: close #469 without merging, and port this exact (now review-verified) fix to billion-context where the shipped 2.x code lives. If you'd rather keep v2-base internally consistent before archiving it, merging is technically clean too — everything above holds either way. I won't touch billion-context without your explicit approval (cross-repo policy); say the word and I'll file the issue there with the 来源 marker and the diff ready to apply.


中文摘要:修复了 V2 下所有 ACP 工具都按硬编码 "compress" 解析 host 权限规则的缺陷(按真实工具名解析,compress 本身行为逐字节不变);代码、回归、测试均审过且本地验证通过(修前测试确实失败 5/12),另补了一个中文 README 同步的小 commit(eb24c6a0);但按你在 #459 的退役决定,V2 线已弃用——建议关闭本 PR 并把此修复移植到 billion-context(等你批准),或者你希望保留 v2-base 一致性也可直接合并。

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