-
Notifications
You must be signed in to change notification settings - Fork 4
fix: make pane input descriptions harm-first #356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # Tool Description Safety Design | ||
|
|
||
| ## Context | ||
|
|
||
| Pane-input guards already reject oversized and dense inline payloads, but their | ||
| human-facing descriptions emphasize byte thresholds and an override. That | ||
| framing invites callers to split or override long prose even though long inline | ||
| delivery can wedge the receiving composer. The product rule must be stated in | ||
| the tool descriptions that govern pane writes. | ||
|
|
||
| `list_surfaces` is already condensed by default. A live 11-surface measurement | ||
| returned 3,779 structured bytes in condensed mode and 9,262 structured bytes in | ||
| verbose mode (2.45 times larger). The condensed fields beyond the original four | ||
| identify panes, columns, and real working directories; they were added for | ||
| deterministic placement and routing and should remain. | ||
|
|
||
| ## Approaches considered | ||
|
|
||
| 1. **Central safety wording in every pane-writing description (chosen).** Put | ||
| the same harm-first warning at the front of each applicable tool and payload | ||
| field description. Keep the numeric schema guards unchanged. This is the | ||
| smallest change and prevents wording drift. | ||
| 2. **Repeat custom warnings per tool.** This can tailor prose, but creates more | ||
| opportunities for one alias or nested prompt field to regress. | ||
| 3. **Replace the byte guard with a hard two-to-three-line guard.** This changes | ||
| runtime behavior beyond the ruling and would reject valid short commands or | ||
| deliberately guarded raw sends. | ||
|
|
||
| ## Design | ||
|
|
||
| - Replace the threshold-led shared guidance with harm-first wording: maximum | ||
| two to three short lines; longer payloads break the receiving pane; write the | ||
| payload to a file and send one `Read and follow <path>` line. | ||
| - Lead every pane-writing tool description with that guidance: `send_to`, | ||
| `send_input`, `send_command`, `broadcast`, `spawn_agent`, `new_split`, | ||
| `new_surface`, `new_worktree_split`, `spawn_in_workspace`, and the deprecated | ||
| `send_to_agent` alias. | ||
| - Lead text/command/prompt field descriptions with the same warning where an | ||
| inline payload exists. `new_split` has no inline `prompt` field on this base; | ||
| its tool description and file-backed `boot_prompt_path` description will | ||
| carry the rule without inventing a new unsafe input. | ||
| - Preserve `CMUXLAYER_MAX_INLINE_CHARS`, dense-line guards, and | ||
| `allow_long_inline` behavior as machine-enforced compatibility controls. | ||
| A review-time RED exposed that the deprecated `new_worktree_split` and | ||
| `spawn_in_workspace` prompt paths did not invoke those guards. Route all | ||
| three spawn APIs through one pre-mutation prompt validator; the deprecated | ||
| paths have no raw-inline override and must use file-backed delivery. | ||
| - Keep `list_surfaces` condensed fields unchanged. Update the tool and | ||
| `verbose` argument descriptions to state that verbose returns every raw cmux | ||
| field, materially increases token usage, and is rarely needed. | ||
|
|
||
| ## `submit_verified` recommendation | ||
|
|
||
| Do not blanket-fail `submit_verified:null`. On current `main`, normal | ||
| interactive `send_to` delivery attempts verification; an attempted but | ||
| unconfirmed submission becomes `false` and returns non-ok. A successful `null` | ||
| means verification was intentionally not attempted, such as `press_enter:false` | ||
| or `allow_busy:true`. PR #326 hardens the complementary spawn/boot settlement | ||
| path; it does not replace generic relay handling already present from #343. | ||
|
|
||
| ## Verification | ||
|
|
||
| - Regression-test all applicable tool and field descriptions. | ||
| - Regression-test that every spawn prompt path rejects over-cap, dense, and | ||
| multi-paragraph inline prompts before pane/workspace mutation. | ||
| - Verify the description tests fail before production changes and pass after. | ||
| - Run the targeted Vitest files, typecheck, build, pre-PR harness, and the full | ||
| Vitest suite under an isolated `TMPDIR`. | ||
| - Exercise the built MCP binary through a real stdio client, list the served | ||
| tools, and call `list_surfaces` in both modes. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # Tool Description Safety Implementation Plan | ||
|
|
||
| > **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. | ||
|
|
||
| **Goal:** Make every pane-writing MCP description warn that long inline payloads break receiving panes, and make the `list_surfaces` verbose cost explicit. | ||
|
|
||
| **Architecture:** Centralize the harm-first pane-input wording in `src/server.ts`, compose it into every applicable tool and payload field description, and pin the contract with description-level tests. Reuse one pre-mutation prompt validator across all spawn APIs so legacy prompt fields cannot bypass the existing runtime guards; keep response schemas unchanged. | ||
|
|
||
| **Tech Stack:** TypeScript, Zod MCP schemas, Vitest. | ||
|
|
||
| --- | ||
|
|
||
| ### Task 1: Pin pane-input description wording | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Use H2 headings for the tasks. Lines 13, 52, and 81 use Based on static analysis, this violates MD001. Also applies to: 52-52, 81-81 🧰 Tools🪛 markdownlint-cli2 (0.23.1)[warning] 13-13: Heading levels should only increment by one level at a time (MD001, heading-increment) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
|
|
||
| **Files:** | ||
| - Modify: `tests/pointer-discipline.test.ts` | ||
| - Test: `tests/pointer-discipline.test.ts` | ||
|
|
||
| **Step 1: Write the failing test** | ||
|
|
||
| Replace the old threshold-led description assertion with checks that every | ||
| pane-writing tool description starts with the shared harm-first warning and | ||
| that each inline text, command, or prompt field repeats the warning. Include | ||
| the compatibility aliases and nested `spawn_in_workspace.agents[].prompt`. | ||
|
|
||
| **Step 2: Run test to verify it fails** | ||
|
|
||
| Run: `npx vitest run tests/pointer-discipline.test.ts` | ||
|
|
||
| Expected: FAIL because current descriptions lead with thresholds and do not | ||
| say that long payloads break receiving panes. | ||
|
|
||
| **Step 3: Write minimal implementation** | ||
|
|
||
| Modify `src/server.ts` to replace `DENSE_INLINE_ROUTING_GUIDANCE` with the | ||
| harm-first wording and prepend it to every applicable tool and field | ||
| description. Do not change schemas or guards during this initial wording step. | ||
|
|
||
| **Step 4: Run test to verify it passes** | ||
|
|
||
| Run: `npx vitest run tests/pointer-discipline.test.ts` | ||
|
|
||
| Expected: PASS. | ||
|
|
||
| **Step 5: Close the review-discovered legacy guard gap** | ||
|
|
||
| Write RED cases proving `new_worktree_split.prompt` and | ||
| `spawn_in_workspace.agents[].prompt` bypass the over-cap, dense-line, and | ||
| multi-paragraph guards. Route all three spawn paths through one shared validator | ||
| before any pane/workspace mutation, then rerun the six cases to GREEN. | ||
|
|
||
| ### Task 2: Pin `list_surfaces` verbose-cost guidance | ||
|
|
||
| **Files:** | ||
| - Modify: `tests/server.test.ts` | ||
| - Test: `tests/server.test.ts` | ||
|
|
||
| **Step 1: Write the failing test** | ||
|
|
||
| Assert that the `list_surfaces` tool description calls the default condensed | ||
| and that the `verbose` field description says it returns raw fields, costs more | ||
| tokens, and is rarely needed. | ||
|
|
||
| **Step 2: Run test to verify it fails** | ||
|
|
||
| Run: `npx vitest run tests/server.test.ts -t "list_surfaces describes verbose cost"` | ||
|
|
||
| Expected: FAIL because the current description only says “full schema.” | ||
|
|
||
| **Step 3: Write minimal implementation** | ||
|
|
||
| Modify only the `list_surfaces` tool and `verbose` field descriptions in | ||
| `src/server.ts`. Keep the condensed and verbose response objects unchanged. | ||
|
|
||
| **Step 4: Run test to verify it passes** | ||
|
|
||
| Run: `npx vitest run tests/server.test.ts -t "list_surfaces describes verbose cost"` | ||
|
|
||
| Expected: PASS. | ||
|
|
||
| ### Task 3: Verify and publish the worker endpoint | ||
|
|
||
| **Files:** | ||
| - Verify: `src/server.ts` | ||
| - Verify: `tests/pointer-discipline.test.ts` | ||
| - Verify: `tests/server.test.ts` | ||
|
|
||
| **Step 1: Run focused and static verification** | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| npx vitest run tests/pointer-discipline.test.ts tests/server.test.ts | ||
| npm run typecheck | ||
| npm run build | ||
| npm run pre-pr:harness | ||
| ``` | ||
|
|
||
| Expected: all commands exit 0. | ||
|
|
||
| **Step 2: Run the full isolated suite** | ||
|
|
||
| Run: `env TMPDIR=<fresh-isolated-dir> npm test` | ||
|
|
||
| Expected: all Vitest files and tests pass. | ||
|
|
||
| **Step 3: Review, commit, push, and open a ready PR** | ||
|
|
||
| Run the bounded local CodeRabbit review, address valid findings, commit only the | ||
| planned files, push `fix/tool-description-safety`, and create a ready-for-review | ||
| PR. Do not merge; the worker brief assigns review routing to `@cmuxlayer`. | ||
|
|
||
| The first CodeRabbit pass found the legacy spawn guard gap fixed in Task 1. Its | ||
| suggestion to qualify the harm-first wording is waived because Etan supplied | ||
| the exact leading text. A second pass was attempted after the fix but the free | ||
| OSS service reported a 22-minute rate limit before analysis began. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new “every pane-writing tool” list omits the public
interacttool. Insrc/server.ts,interact(action="send")forwards arbitrarytextdirectly todeliverAgentInputwithout the inline, dense-line, or multiline guards, andtests/enter-reliability.test.tsstill asserts that a 2,000-character message succeeds. A caller using this API therefore receives none of the warning and can bypass the cap, reproducing the pane-wedging case this change is intended to prevent; include theinteractdescription/field and guard its send action.Useful? React with 👍 / 👎.