diff --git a/devlog/2026-09-15_restore-command-abort-throw/REQ.md b/devlog/2026-09-15_restore-command-abort-throw/REQ.md new file mode 100644 index 00000000..02bc7957 --- /dev/null +++ b/devlog/2026-09-15_restore-command-abort-throw/REQ.md @@ -0,0 +1,79 @@ +# REQ - Restore command-abort throw for /acp subcommands (fix #398) + +- Task ID: `2026-09-15_restore-command-abort-throw` +- Home Repo: `opencode-acp` +- Created: 2026-09-15 +- Status: InProgress +- Priority: P1 +- Owner: ranxianglei (ework-daemon agent) +- References: https://github.com/ranxianglei/opencode-acp/issues/398, regression commit `5fd8f5ee9` (PR #297, fixing #296) + +## 1. Background & Problem Statement + +- **Context**: The `/acp` command is registered in the config hook with `template: ""` (no `$ARGUMENTS` + placeholder). opencode's `Plugin.trigger` aborts a command ONLY when the + `command.execute.before` hook throws; a normal return lets opencode proceed to render the + template and send it as a user message. Since the template is empty, the raw arguments are + appended verbatim into the message body (`packages/opencode/src/session/prompt.ts:1390-1393`). +- **Current behavior (symptom)**: On v1.17.0+ (master `70beea0`, v1.18.0), running `/acp status`, + `/acp stats`, or `/acp context` shows the `[ACP Status]` output AND leaks the subcommand word + (e.g. `status`) as a plain user message to the model, which then answers it (spurious model + call, token burn, session pollution). Bare `/acp` does not leak (empty args → empty prompt). + `/acp help` and `/acp export` do not leak (those branches still throw). +- **Expected behavior**: All `/acp` subcommands are fully handled inside the hook and the command + is aborted — no message is ever sent to the model (≤v1.16.0 semantics). +- **Impact**: Every `/acp status|stats|context` invocation triggers an unintended full model call + (~40K input tokens in the reporter's A/B test) and confuses the conversation. Regression since + v1.17.0. + +## 2. Reproduction (if applicable) + +- **Environment**: + - Node: 22 + - OS/Arch: linux-x64 +- **Minimal reproduction steps**: + 1) opencode ≥1.16 + opencode-acp ≥1.17.0, default config + 2) Type `/acp status` in a session + 3) Observe: `[ACP Status]` block appears, then a user message with text `status` triggers a model call +- **Relevant configuration**: none special; `config.commands.enabled: true` (default) + +## 3. Constraints & Non-Goals + +- **Constraints**: + - Backward compatibility: restoring the throw returns exactly ≤v1.16.0 semantics; no state + format, API, or tag changes. + - Known cost (accepted): on opencode ≥1.18.18 each `/acp` invocation logs one `level=ERROR` + line for the thrown `__DCP_CONTEXT_HANDLED__` error — this is the original complaint of + #296 and is the documented trade-off of the only available abort mechanism. Log noise ≪ + spurious model invocation. + - Performance requirements: none (command path only). +- **Non-Goals** (explicitly out of scope): + - Negotiating a silent-abort API with opencode upstream (hook returning a cancel signal / + control field in `output`). Track as follow-up; switch away from throw once such an API exists. + - User-side mitigation (pinning opencode-acp@1.16.0). + +## 4. Acceptance Criteria (must be testable) + +- **Correctness**: + - [ ] `lib/hooks.ts` stats/status/empty branch throws `__DCP_CONTEXT_HANDLED__` after `handleStatsCommand` + - [ ] `lib/hooks.ts` context fallback branch throws `__DCP_CONTEXT_HANDLED__` after `handleContextCommand` + - [ ] export/help branches unchanged (already correct) +- **Performance / Stability**: + - [ ] No change to non-command code paths +- **Regression**: + - [ ] New/modified test cases added to test suite and passing: every subcommand branch + (stats/status/""/context/help/export) asserts the handler REJECTS with + `__DCP_CONTEXT_HANDLED__` — this directly encodes "no subsequent prompt can be sent", + preventing a future revert of throw→return (the exact gap that allowed PR #297's + regression to survive 900+ tests) + - [ ] The PR #297 test that asserted normal return ("issue #296") is flipped to assert the throw + +## 5. Proposed Approach (optional) + +- **Affected modules & entry files**: + - `lib/hooks.ts` — `createCommandExecuteHandler` (2 lines: restore two throws) + - `tests/hooks-permission.test.ts` — flip the #296 test; add per-branch regression tests +- **Risks**: Low. Restores previously shipped, field-proven behavior (v1.10.0–v1.16.0 all threw). + The only behavioral delta vs v1.17.0/v1.18.0 is the ERROR log line on opencode ≥1.18.18 (#296), + which is the accepted trade-off. +- **Rollback strategy**: Revert the single fix commit. diff --git a/devlog/2026-09-15_restore-command-abort-throw/WORKLOG.md b/devlog/2026-09-15_restore-command-abort-throw/WORKLOG.md new file mode 100644 index 00000000..7267327a --- /dev/null +++ b/devlog/2026-09-15_restore-command-abort-throw/WORKLOG.md @@ -0,0 +1,79 @@ +# WORKLOG - Restore throw-based abort for /acp subcommands (#398) + +- Task ID: `2026-09-15_restore-command-abort-throw` +- Home Repo: `opencode-acp` +- Status: Done +- Updated: 2026-09-15 15:10 + +## 1. Summary + +- **What was done** (1–3 sentences): Restored `throw new Error("__DCP_CONTEXT_HANDLED__")` as the command-abort mechanism in the two `/acp` branches that PR #297 had downgraded to normal returns (`stats`/`status`/bare, and the `context` fallback). Added a regression-guard test suite asserting every handled `/acp` branch rejects with the sentinel AND that its notification prompt carries `noReply:true` + `ignored:true`. +- **Why** (1–3 sentences): opencode's `Plugin.trigger` aborts a command only when a hook throws; the command is registered with `template: ""` (no `$ARGUMENTS`), so a normal return lets opencode append the raw arguments to the empty template and send them to the model. Since v1.17.0 this leaked `status`/`stats`/`context` as user messages and triggered spurious ~40K-token model calls (issue #398). +- **Behavior / compatibility changes**: Yes — `/acp status|stats|context` now abort the command instead of leaking the argument word to the model. Side effect: one `level=ERROR` log line per `/acp` invocation on opencode ≥ 1.18.18 (the known cost of the only available abort mechanism; #296 noise, explicitly accepted per issue #398 discussion). No persisted-state, config-schema, or API changes. +- **Risk level**: Low + +## 2. Change Log + +### Commits + +| Commit | Description | +|--------|-------------| +| `2c59c07` | fix: restore throw-based abort for /acp subcommands (regression guard #398) | +| `85bc780` | docs: record final commit sha in WORKLOG | +| `2202bcc` | test: address review nits - type prompt recorder, tighten deny-permission abort assertion (#398) | + +### Key Files + +- `lib/hooks.ts` — `createCommandExecuteHandler`: stats/status/bare branch `return` → `throw`; context fallback gains trailing `throw`; added `[FIX #398]` comment documenting why throws are mandatory (this line was broken once before by PR #297). +- `tests/hooks-permission.test.ts` — replaced the flawed #296 test (asserted normal return) with `createCommandHarness()` + `expectAbortedAfterNotification()` helpers and two tests: regression guard looping all six handled branches, and non-acp pass-through. +- `devlog/2026-09-15_restore-command-abort-throw/REQ.md` — requirement record. + +## 3. Design & Implementation Notes + +- **Entry point / key function**: `createCommandExecuteHandler` in `lib/hooks.ts` (registered for `command.execute.before` in `index.ts`). +- **Key configuration items**: none changed. +- **Key logic explanation**: The hook has no cancel signal in its output type (`@opencode-ai/plugin` CommandOutput = `{ parts }` only), so throwing is the *only* way to stop opencode from executing the command template. All branches must therefore end in the sentinel throw *after* their handler runs (handlers send the `ignored:true, noReply:true` notification via `sendIgnoredMessage`). The `[FIX #398]` comment warns future maintainers not to convert these throws back into returns (PR #297 did exactly that while chasing #296's ERROR-log noise). + +## 4. Testing & Verification + +### Build & Test Commands + +```sh +npm run typecheck +npm test +node --import tsx --test tests/hooks-permission.test.ts +``` + +### Test Coverage + +- New/modified test files: `tests/hooks-permission.test.ts` +- Test count: 1264 total, 1264 pass, 0 fail (full suite); hooks-permission file: 10 pass / 0 fail +- Key scenarios verified: + - Each of `stats`, `status`, `""`, `context`, `help`, `export --stdout` rejects with `Error: __DCP_CONTEXT_HANDLED__` + - For each case exactly one `session.prompt` call is recorded with `body.noReply === true` and `parts[0].ignored === true` (notification path intact, no plain-message leak) + - Non-`acp` commands still resolve without throwing (pass-through preserved) + - **Mutation check**: temporarily reverting `lib/hooks.ts` (git stash) makes the regression guard fail (`not ok 4`), confirming the test catches the original bug; fix restored and re-verified green + +### Results + +- **PASS/FAIL**: PASS +- **Key logs/data** (optional): mutation run output — `not ok 4 - command execute aborts every handled /acp branch by throwing (regression guard #398)` on buggy code; full suite green after restore. + +## 5. Risk Assessment & Rollback + +- **Risk points**: opencode ≥ 1.18.18 logs one `level=ERROR` line per `/acp` invocation (accepted trade-off, see REQ.md). No other behavior change; export/help branches untouched. +- **Rollback method**: + - Revert commit(s): `2c59c07` + - Rollback impact: returns to the v1.17.0+ leak behavior (subcommand words sent to the model). +- **Compatibility notes** (data format, config schema): No changes. + +## 6. Lessons Learned (optional) + +- What went well: the A/B evidence in the issue plus reading `guard()` confirmed the throw propagates cleanly (not swallowed). +- What could be improved: PR #297's new test asserted the wrong property ("handler resolves") instead of the observable contract ("command aborted"); contract-level assertions should target side effects (prompt recording), not control flow. +- Reusable conclusions: in opencode, hook return values cannot cancel command execution — throw-or-leak until upstream adds a silent-abort API. + +## 7. Follow-ups (optional) + +- [ ] Negotiate a silent-abort API with opencode upstream (hook output control field / explicit cancel signal); once available, replace the sentinel throw and close #296 properly. +- [ ] Track upstream acceptance; if rejected, document the ERROR-log cost in README troubleshooting. diff --git a/lib/hooks.ts b/lib/hooks.ts index e508b791..63a977f6 100644 --- a/lib/hooks.ts +++ b/lib/hooks.ts @@ -509,10 +509,18 @@ export function createCommandExecuteHandler( workingDirectory, } + // [FIX #398] Every handled /acp branch MUST abort the command by throwing + // __DCP_CONTEXT_HANDLED__. A normal return does NOT stop execution: + // opencode's Plugin.trigger only aborts on hook errors, and the command is + // registered with template: "" (no $ARGUMENTS), so opencode appends the raw + // arguments to the empty template and sends them to the model as a user + // message ("/acp status" leaked "status" to the model in v1.17.0+). The + // resulting level=ERROR log line on opencode >= 1.18.18 is the known cost + // of this mechanism (#296) — do NOT replace these throws with returns. const sub = input.arguments?.trim().toLowerCase() if (sub === "stats" || sub === "status" || sub === "") { await handleStatsCommand(commandCtx) - return + throw new Error("__DCP_CONTEXT_HANDLED__") } if (sub === "export" || sub.startsWith("export ")) { @@ -527,6 +535,7 @@ export function createCommandExecuteHandler( } await handleContextCommand(commandCtx) + throw new Error("__DCP_CONTEXT_HANDLED__") } } } diff --git a/tests/hooks-permission.test.ts b/tests/hooks-permission.test.ts index 36fe6a0e..103e1fe1 100644 --- a/tests/hooks-permission.test.ts +++ b/tests/hooks-permission.test.ts @@ -161,38 +161,86 @@ test("command execute works even when effective permission resolves to deny (inf { global: undefined, agents: {} }, ) - // /acp (no args) now shows compression status — works regardless of compress permission - try { - await handler({ command: "dcp", sessionID: "session-1", arguments: "" }, output) - } catch (e: any) { - if (e?.message !== "__DCP_CONTEXT_HANDLED__") throw e - } + // /acp (no args) now shows compression status — works regardless of compress permission. + // Since #398 the abort is deterministic: only a throw stops opencode from sending + // the arguments to the model, so the hook MUST reject with the sentinel. + await expectAbortedAfterNotification(handler, "", "dcp") assert.equal(sessionMessagesCalls, 1) }) -test("command execute returns normally (no __DCP_CONTEXT_HANDLED__ throw) — issue #296", async () => { +// Minimal shape of the session.prompt call ACP sends for ignored notifications +// (see sendIgnoredMessage in lib/ui/notification.ts). +type IgnoredPromptCall = { + path: { id: string } + body: { noReply?: boolean; parts?: Array<{ type: string; text: string; ignored?: boolean }> } +} + +function createCommandHarness(permission: string = "allow") { let sessionMessagesCalls = 0 - const output = { parts: [] as any[] } - const handler = createCommandExecuteHandler( - { - session: { - messages: async () => { - sessionMessagesCalls += 1 - return { data: [] } - }, + const prompts: IgnoredPromptCall[] = [] + const client = { + session: { + messages: async () => { + sessionMessagesCalls += 1 + return { data: [] } }, - } as any, + prompt: async (args: IgnoredPromptCall) => { + prompts.push(args) + return {} + }, + }, + } + const handler = createCommandExecuteHandler( + client as any, createTestRegistry(createSessionState()), new Logger(false), - buildConfig("allow"), + buildConfig(permission), "/tmp", { global: undefined, agents: {} }, ) + return { handler, prompts, calls: () => sessionMessagesCalls } +} + +// Regression guard for #398 (reverts PR #297): opencode's Plugin.trigger only +// aborts a command when the hook THROWS — a normal return lets opencode append +// the raw arguments to the empty command template and send them to the model +// ("/acp status" leaked a user message "status", triggering a spurious ~40K-token +// model call). Every handled /acp branch MUST reject with +// __DCP_CONTEXT_HANDLED__ after delivering its ignored notification. +async function expectAbortedAfterNotification( + handler: ReturnType, + args: string, + command: string = "acp", +): Promise { + await assert.rejects( + () => handler({ command, sessionID: "session-1", arguments: args }, { parts: [] }), + (e: unknown) => e instanceof Error && e.message === "__DCP_CONTEXT_HANDLED__", + ) +} - await handler({ command: "acp", sessionID: "session-1", arguments: "context" }, output) +test("command execute aborts every handled /acp branch by throwing (regression guard #398)", async () => { + // Each form must reject with __DCP_CONTEXT_HANDLED__ AND have delivered its + // ignored notification first (proves the branch ran, then aborted — not swallowed). + for (const args of ["stats", "status", "", "context", "help", "export --stdout"]) { + const { handler, prompts, calls } = createCommandHarness() + await expectAbortedAfterNotification(handler, args) + assert.ok( + prompts.length >= 1, + `/acp ${args || "(bare)"} should send its ignored notification before aborting`, + ) + assert.equal(prompts[0].body.noReply, true) + assert.equal(prompts[0].body.parts[0].ignored, true) + assert.equal(prompts[0].path.id, "session-1") + assert.equal(calls(), 1, `/acp ${args || "(bare)"} should fetch session messages exactly once`) + } +}) - assert.equal(sessionMessagesCalls, 1) +test("command execute passes through non-acp commands without throwing", async () => { + const { handler } = createCommandHarness() + await assert.doesNotReject( + () => handler({ command: "other", sessionID: "session-1", arguments: "" }, { parts: [] }), + ) }) test("text complete strips hallucinated metadata tags", async () => {