refactor(tool): description cleanup + multiedit dead-code removal (PR1 of #129)#235
Conversation
Make trash explicitly the canonical deletion path; contrast with rm/del permanence; reference the existing permission-layer block on rm. Per #129.
The 'It is always better to speculatively perform multiple searches as a batch' sentence pushed weaker models toward parallel work without a supporting reason. Keep the parallel-call capability reminder; drop the push. Per #129.
The skill list is appended at the end of the skill tool description itself by registry.ts:283 (describeSkill), not in the system prompt. Both L1 and L5 had the wrong pointer. Per #129.
- Replace 'rm "path with spaces"' quoting example with 'mkdir "..."'; bash.txt's own L31 list forbids cat, and python would duplicate L20. - Add 'File deletion: Use the trash tool, NOT rm/del' sub-bullet to the L31 nested 'Avoid using Bash with' list. - Delete the two 'NEVER/DO NOT use the TodoWrite or Task tools' lines inside the git/PR workflow block; they contradict PawWork's project-level encouragement of TodoWrite for multi-step work. Per #129. Note: rm/rmdir/unlink are already denied at the permission layer (agent.ts:99-110); this commit reduces the wasted first-attempt and stops bash.txt from suggesting rm as the canonical example.
The 'demonstrate thoroughness to the user' framing on L1 and the 'When in doubt, use this tool ... demonstrates attentiveness' line on L166 push using the todo tool as performance, not organization. Remove both. The 8-example demonstration block remains deferred. Per #129.
- Delete fictional <example_agent_descriptions> block and both <example>
blocks referencing code-reviewer / greeting-responder.
- Delete Usage 1 ('Launch multiple agents concurrently') and Usage 4
('outputs should generally be trusted'); renumber remaining.
- Rewrite L12 dangling 'agent descriptions above' and Usage 6's 'agent
description' to schema-independent 'the available agents'.
Per #129.
Weaker models commonly delete a single line by passing the line content as oldString and an empty newString, but forget to include the trailing newline, leaving a stray blank line at the deleted position. Add an explicit hint so the tool is harder to misuse on this boundary case. Per #129.
multiedit was never registered in registry.ts builtin tool list. The
description claimed atomicity ('All edits must be valid for the
operation to succeed') but multiedit.ts:38-50 was a plain for loop
with each successful write landing immediately and no rollback; the
description lied about runtime behavior.
Remove .ts + .txt + three string-literal references in permission/config
+ one spec mention + three test references (rename two tests, delete
one multiedit-specific migration test). Atomic so no intermediate state
has dangling references.
Per user decision: no legacy users / no legacy config in the wild, so
the entire multiedit migration code path is removed (not preserved as
no-op). Models that need multiple edits in one file issue multiple edit
calls; functionally equivalent and removes the lying description.
Per #129.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoves the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/test/agent/agent.test.ts (1)
493-510:⚠️ Potential issue | 🟡 MinorTest name overstates coverage.
Line 493 says write/edit/patch are covered, but this case only validates
write -> edit. Please either add edit/patch cases or narrow the title.💡 Minimal title fix
-test("legacy tools config maps write/edit/patch to edit permission", async () => { +test("legacy tools config maps write to edit permission", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/agent/agent.test.ts` around lines 493 - 510, The test name "legacy tools config maps write/edit/patch to edit permission" is misleading because the body only checks write -> edit; update the test to either rename it to accurately reflect that it only validates write->edit (e.g., "legacy tools config maps write to edit permission") or extend the test to cover the other mappings by adding assertions that set config.agent.build.tools.edit and config.agent.build.tools.patch (using tmpdir, Instance.provide, Agent.get and evalPerm) and assert their expected permissions; locate the existing test declaration and modify the title or add the extra cases for edit and patch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/tool/task.txt`:
- Around line 16-17: Edit the two phrases that use redundant “back” wording:
replace “return a single message back to you” with “return a single message to
you” and replace “send a text message back to the user” (and any other
occurrences of “return back”/“send back”) with “send a text message to the user”
or “return to the user” so the instructions read concisely and without
duplicated prepositions.
---
Outside diff comments:
In `@packages/opencode/test/agent/agent.test.ts`:
- Around line 493-510: The test name "legacy tools config maps write/edit/patch
to edit permission" is misleading because the body only checks write -> edit;
update the test to either rename it to accurately reflect that it only validates
write->edit (e.g., "legacy tools config maps write to edit permission") or
extend the test to cover the other mappings by adding assertions that set
config.agent.build.tools.edit and config.agent.build.tools.patch (using tmpdir,
Instance.provide, Agent.get and evalPerm) and assert their expected permissions;
locate the existing test declaration and modify the title or add the extra cases
for edit and patch accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35612100-c150-44ad-86ec-0c4bab0242f7
📒 Files selected for processing (16)
packages/opencode/specs/effect-migration.mdpackages/opencode/src/config/agent.tspackages/opencode/src/config/config.tspackages/opencode/src/permission/index.tspackages/opencode/src/tool/bash.txtpackages/opencode/src/tool/edit.txtpackages/opencode/src/tool/glob.txtpackages/opencode/src/tool/multiedit.tspackages/opencode/src/tool/multiedit.txtpackages/opencode/src/tool/skill.txtpackages/opencode/src/tool/task.txtpackages/opencode/src/tool/todowrite.txtpackages/opencode/src/tool/trash.txtpackages/opencode/test/agent/agent.test.tspackages/opencode/test/config/config.test.tspackages/opencode/test/permission/next.test.ts
💤 Files with no reviewable changes (4)
- packages/opencode/specs/effect-migration.md
- packages/opencode/src/tool/multiedit.txt
- packages/opencode/test/config/config.test.ts
- packages/opencode/src/tool/multiedit.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-desktop
- GitHub Check: unit-opencode
- GitHub Check: typecheck
- GitHub Check: smoke-macos-arm64
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/src/config/config.tspackages/opencode/src/config/agent.tspackages/opencode/test/permission/next.test.tspackages/opencode/test/agent/agent.test.tspackages/opencode/src/permission/index.ts
packages/opencode/test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
packages/opencode/test/**/*.test.{ts,tsx}: Use thetmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup. Useawait usingsyntax to ensure automatic cleanup when the variable goes out of scope.
When using thetmpdirfunction with git repository support, pass thegit: trueoption to initialize a git repo with a root commit.
Use theconfigoption intmpdirto write anopencode.jsonconfig file during test setup by passing a partial Config.Info object.
Use theinitoption intmpdirto define custom setup functions that can return extra data accessible viatmp.extra, and use thedisposeoption for custom cleanup logic.
UsetestEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows.
Useit.effect(...)when the test should run withTestClockandTestConsole. Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Prefer Effect-aware helpers fromfixture/fixture.tsover building manual runtimes in tests: usetmpdirScoped()for scoped temp directories,provideInstance(dir)(effect)for low-level binding without directory creation,provideTmpdirInstance(...)for single temp instance binding, orprovideTmpdirServer(...)for tests that also need the test LLM server.
Defineconst it = testEffect(...)near the top of the test file and keep the test body insideEffect.gen(function* () { ... }). Yield services directly withyield* MyService.Serviceoryield* MyTool.
Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime.
When a test needs instance-local state, preferprovideTmpdirInstance(...)orprovideInstance(...)over manualInstance.provide(...)inside Promise-style tests.
Files:
packages/opencode/test/permission/next.test.tspackages/opencode/test/agent/agent.test.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/ui/src/theme/context.tsx:11-16
Timestamp: 2026-04-22T09:32:58.310Z
Learning: In Astro-Han/pawwork (`packages/ui/src/theme/context.tsx` and related files), the renaming of localStorage theme keys from `opencode-*` to `pawwork-*` (THEME_ID, COLOR_SCHEME, THEME_CSS_LIGHT, THEME_CSS_DARK) is intentional and should NOT include a migration path from the old keys. Migrating would re-couple PawWork and OpenCode browser storage namespaces, which the PR is explicitly designed to avoid. A reset to the PawWork default theme on upgrade is acceptable by design.
📚 Learning: 2026-04-21T16:57:25.580Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/agent.ts:108-119
Timestamp: 2026-04-21T16:57:25.580Z
Learning: In `packages/opencode/src/config/agent.ts` (Astro-Han/pawwork), `ConfigPermission.Info` only accepts permission objects or the three action strings `"ask"`, `"allow"`, `"deny"`, and transforms those action strings into `{ "*": action }` before `normalize()` runs. By the time `normalize()` is reached, `configuredPermission` is always either `undefined` or a `Record<string, Rule>` — never a raw arbitrary string. The `Object.assign(permission, configuredPermission)` pattern is therefore safe. Do not flag it as corrupting string permission references.
Applied to files:
packages/opencode/src/config/config.tspackages/opencode/src/config/agent.tspackages/opencode/test/permission/next.test.tspackages/opencode/test/agent/agent.test.tspackages/opencode/src/permission/index.ts
📚 Learning: 2026-04-21T16:57:20.257Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/command.ts:23-29
Timestamp: 2026-04-21T16:57:20.257Z
Learning: In packages/opencode/src/config/ config modules, treat both `Schema.Struct` and `Schema.Class` as valid schema patterns and do not require converting `Schema.Struct` to `Schema.Class`. If `Schema.Struct` is used in a config schema (e.g., in files like `command.ts`, `formatter.ts`, `provider.ts`, `permission.ts`, `lsp.ts`, `config.ts`, `keybinds.ts`), accept it as intentional; the selection should be based on upstream alignment and the local schema context, not on a rule that one form must replace the other.
Applied to files:
packages/opencode/src/config/config.tspackages/opencode/src/config/agent.ts
📚 Learning: 2026-04-22T08:49:47.800Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:47.800Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/opencode/test/permission/next.test.tspackages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-23T08:51:00.819Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:00.819Z
Learning: For pawwork tests under packages/opencode/test/**, auth.json teardown may intentionally combine `Filesystem.write` (from `packages/opencode/src/util/filesystem.ts`) with `node:fs/promises` `unlink` for cleanup. Do not flag this as inconsistent style; it is the established/intentional pattern because `Filesystem` does not provide a `remove`/`unlink` helper.
Applied to files:
packages/opencode/test/permission/next.test.tspackages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use `testEffect(...)` from `test/lib/effect.ts` for tests that exercise Effect services or Effect-based workflows.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/packages/app/src/testing/**/*.ts : Test-only hooks must be inert unless explicitly enabled and should not add normal-runtime listeners, reactive subscriptions, or per-update allocations
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `tmpdir` function from `fixture/fixture.ts` to create temporary directories for tests with automatic cleanup. Use `await using` syntax to ensure automatic cleanup when the variable goes out of scope.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:35:49.291Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/AGENTS.md:0-0
Timestamp: 2026-04-20T14:35:49.291Z
Learning: ALWAYS USE PARALLEL TOOLS WHEN APPLICABLE for tool calling
Applied to files:
packages/opencode/src/tool/bash.txt
🪛 LanguageTool
packages/opencode/src/tool/task.txt
[style] ~16-~16: Using “back” with the verb “return” may be redundant.
Context: ...s done, it will return a single message back to you. The result returned by the agen...
(RETURN_BACK)
[style] ~17-~17: Consider using just “return”.
Context: ...actly what information the agent should return back to you in its final and only message to...
(RETURN_BACK)
🔇 Additional comments (12)
packages/opencode/src/tool/skill.txt (1)
1-5: Good pointer cleanup in skill constraints.Line 1 and Line 5 now reference skills “listed below,” which is clearer and less brittle than referencing an external prompt location.
packages/opencode/src/tool/glob.txt (1)
6-6: Line 6 guidance is appropriately generalized.This keeps parallel/multi-tool capability explicit without prescribing speculative batching behavior.
packages/opencode/src/tool/edit.txt (1)
11-11: Helpful deletion-specific instruction added.Line 11 documents a real footgun for exact replacements and should reduce stray blank-line diffs.
packages/opencode/src/tool/todowrite.txt (1)
1-1: Line 1 wording is cleaner and more action-oriented.The revised sentence stays specific to task tracking value without over-framing.
packages/opencode/src/tool/trash.txt (1)
3-5: Strong safety clarification for deletion workflows.Line 3 and Line 5 make reversibility vs permanence explicit and reduce misuse of shell deletion commands.
packages/opencode/src/tool/bash.txt (2)
16-16: Line 16 example is a safer, clearer quoting exemplar.This avoids reinforcing destructive-command examples while still teaching correct path quoting.
38-38: Good hard guardrail on shell-based deletions.Line 38 is explicit and aligns the Bash tool contract with trash-first deletion behavior.
packages/opencode/src/tool/task.txt (1)
12-12: Line 12 agent-scoping language is clearer.This better anchors Task-tool exclusions to the actual available agent set.
packages/opencode/src/config/agent.ts (1)
111-115: Permission remapping update looks correct.This keeps legacy tools migration aligned with multiedit removal while preserving write/edit/patch →
permission.editbehavior.packages/opencode/src/config/config.ts (1)
733-737: Config-side legacy tool normalization is consistent.This mirrors the agent normalization change and cleanly removes multiedit aliasing from
editpermission derivation.packages/opencode/src/permission/index.ts (1)
297-303:EDIT_TOOLSupdate is aligned with tool deprecation.Removing
"multiedit"here keepsPermission.disabled()behavior consistent with the runtime tool set.packages/opencode/test/permission/next.test.ts (1)
390-402: Updated disabled-tool expectation is correct.This test now precisely reflects the supported
editaliases (edit,write,apply_patch) after multiedit removal.
There was a problem hiding this comment.
Code Review
This pull request removes the multiedit tool and its associated configurations, tests, and documentation. It also updates the instruction files for several other tools, including bash, edit, task, and trash, to refine operational guidance and safety protocols. Review feedback suggests using relative paths in documentation examples for better compatibility, clarifying the parameters for line deletion in the edit tool, and standardizing the use of the trash tool for all file deletions.
Astro-Han
left a comment
There was a problem hiding this comment.
Review pass: two concrete inline findings, graded by P-level.
Address @Astro-Han P2 review on PR #235: original L5 claimed PawWork's permission system blocks shell deletion 'by default', but agent.ts:106-109 only denies rm/rmdir/unlink/find-delete; Windows del/erase/Remove-Item are not yet blocked. Narrow the claim to rm-family and explicitly note the Windows gap. Also drop 'in user-facing flows' qualifier (gemini review): the trash tool is the canonical deletion path regardless of flow context.
Address @Astro-Han P3 review on PR #235: the test fixture only sets 'tools.write = false' and asserts the resulting edit permission, so the 'write/edit/patch' name overstates coverage. Narrow the name to 'tools.write' which matches what the test actually exercises. The agent.ts:111 union check still covers all three legacy aliases; this is a test-name accuracy fix only.
Address gemini-code-assist review on PR #235: the original hint named the trailing-newline requirement on oldString but left newString implicit. Make both sides of the line-deletion contract explicit so weaker models do not just check the (already-empty) newString and miss the oldString fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/tool/edit.txt`:
- Line 11: Split the long guidance sentence that begins "To delete a line
cleanly, include the trailing newline character in `oldString`..." into two
sentences for readability, and add a short concrete example immediately after
(e.g., show oldString containing "line-to-delete\n" replaced with "" results in
the line being removed with no blank line). Update the surrounding paragraph in
packages/opencode/src/tool/edit.txt to use the shorter sentences and include the
example to demonstrate the behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02df6eb9-9511-43cd-a88a-adf0de6e5ce3
📒 Files selected for processing (3)
packages/opencode/src/tool/edit.txtpackages/opencode/src/tool/trash.txtpackages/opencode/test/agent/agent.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: smoke-macos-arm64
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-app
- GitHub Check: unit-opencode
- GitHub Check: unit-desktop
- GitHub Check: typecheck
- GitHub Check: e2e-artifacts
- GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/test/agent/agent.test.ts
packages/opencode/test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
packages/opencode/test/**/*.test.{ts,tsx}: Use thetmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup. Useawait usingsyntax to ensure automatic cleanup when the variable goes out of scope.
When using thetmpdirfunction with git repository support, pass thegit: trueoption to initialize a git repo with a root commit.
Use theconfigoption intmpdirto write anopencode.jsonconfig file during test setup by passing a partial Config.Info object.
Use theinitoption intmpdirto define custom setup functions that can return extra data accessible viatmp.extra, and use thedisposeoption for custom cleanup logic.
UsetestEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows.
Useit.effect(...)when the test should run withTestClockandTestConsole. Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Prefer Effect-aware helpers fromfixture/fixture.tsover building manual runtimes in tests: usetmpdirScoped()for scoped temp directories,provideInstance(dir)(effect)for low-level binding without directory creation,provideTmpdirInstance(...)for single temp instance binding, orprovideTmpdirServer(...)for tests that also need the test LLM server.
Defineconst it = testEffect(...)near the top of the test file and keep the test body insideEffect.gen(function* () { ... }). Yield services directly withyield* MyService.Serviceoryield* MyTool.
Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime.
When a test needs instance-local state, preferprovideTmpdirInstance(...)orprovideInstance(...)over manualInstance.provide(...)inside Promise-style tests.
Files:
packages/opencode/test/agent/agent.test.ts
🧠 Learnings (10)
📚 Learning: 2026-04-21T16:57:25.580Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/agent.ts:108-119
Timestamp: 2026-04-21T16:57:25.580Z
Learning: In `packages/opencode/src/config/agent.ts` (Astro-Han/pawwork), `ConfigPermission.Info` only accepts permission objects or the three action strings `"ask"`, `"allow"`, `"deny"`, and transforms those action strings into `{ "*": action }` before `normalize()` runs. By the time `normalize()` is reached, `configuredPermission` is always either `undefined` or a `Record<string, Rule>` — never a raw arbitrary string. The `Object.assign(permission, configuredPermission)` pattern is therefore safe. Do not flag it as corrupting string permission references.
Applied to files:
packages/opencode/test/agent/agent.test.tspackages/opencode/src/tool/trash.txt
📚 Learning: 2026-04-23T08:51:00.819Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:00.819Z
Learning: For pawwork tests under packages/opencode/test/**, auth.json teardown may intentionally combine `Filesystem.write` (from `packages/opencode/src/util/filesystem.ts`) with `node:fs/promises` `unlink` for cleanup. Do not flag this as inconsistent style; it is the established/intentional pattern because `Filesystem` does not provide a `remove`/`unlink` helper.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use the `config` option in `tmpdir` to write an `opencode.json` config file during test setup by passing a partial Config.Info object.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use camelCase for variable names in tests
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-22T08:49:47.800Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/desktop-electron/src/main/index-sidecar-source.test.ts:3-11
Timestamp: 2026-04-22T08:49:47.800Z
Learning: In `packages/desktop-electron/src/main/index-sidecar-source.test.ts` (Astro-Han/pawwork), the test intentionally uses `expect(source).toContain` / `expect(source).not.toContain` string matching against the raw `index.ts` source text as a lightweight sidecar contract guard. The maintainer has explicitly chosen not to introduce an AST parser (e.g., `babel/parser` or acorn) for this purpose. Do not flag these string-based assertions as fragile or suggest converting them to AST-based matching.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Use `testEffect(...)` from `test/lib/effect.ts` for tests that exercise Effect services or Effect-based workflows.
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/opencode/test/agent/agent.test.ts
📚 Learning: 2026-04-23T08:51:04.230Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:04.230Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/util/filesystem.ts`), the `Filesystem` utility does NOT expose a `remove` or `unlink` helper. The established repository pattern for auth.json teardown in tests (e.g. `provider.test.ts`, `amazon-bedrock.test.ts`, `workspace-adaptor.test.ts`) is to combine `Filesystem.write` with `node:fs/promises unlink`. Do not flag this mixed usage as inconsistent — it is the correct and intentional pattern.
Applied to files:
packages/opencode/src/tool/trash.txt
🔇 Additional comments (2)
packages/opencode/src/tool/trash.txt (1)
3-6: Good safety clarification and accurate platform caveat.This wording is much clearer: it correctly promotes reversible deletion via Trash and avoids over-claiming by explicitly noting that Windows shell deletion commands are not yet blocked by default.
packages/opencode/test/agent/agent.test.ts (1)
493-493: LGTM — test name now accurately reflects coverage.The updated test name correctly describes what the test actually verifies: that the legacy
tools.writeconfig option maps to theeditpermission. This implements the suggestion from the past review to narrow the test name to match the actual fixture and assertion.
Address coderabbit nitpick on PR #235: the hint named the trailing newline requirement abstractly. Add an inline example showing the literal '\\n' byte and the oldString/newString shape, matching the existing inline-example convention at L5 ('(e.g., `1: `)'). Helps weaker models bridge the 'what does trailing newline look like' gap without expanding to a multi-line bullet.
Summary
PR1 of #129. Two strands combined: description cleanup across 7 tool files (including edit.txt line-deletion hint), and multiedit dead-code removal.
8 commits, 16 paths changed (+22 / -192).
trash.txtrm-vs-trash boundaryglob.txtskill.txtpointer to "listed below"bash.txtrm/deletion guardrails (rm→mkdir example, add File deletion sub-bullet, drop two TodoWrite/Task negations)todowrite.txttask.txtfictional examples and over-push (drop<example_agent_descriptions>block, Usage 1+4, fix dangling refs)edit.txtmultieditdead code (atomic 9-path: deletemultiedit.ts+multiedit.txt, strip string literals frompermission/index.ts+config/agent.ts+config/config.ts, drop spec mention, rename two tests, delete migration test)Why
Per #129. Tool descriptions in upstream opencode were tuned for a different audience and contained: fictional
<example_agent_descriptions>blocks (code-reviewer/greeting-respondernot in PawWork), over-push framing ("It is always better to speculatively...", "demonstrates attentiveness"), boundary leaks (rm "path with spaces/file.txt"example contradicting trash-as-canonical), wrong pointers (skill.txt"listed in the system prompt" — actually appended below byregistry.ts:283), and dangling references after registry edits.multieditwas never registered inregistry.ts(dead code), and its description claimed atomicity that the implementation never provided (multiedit.ts:38-50was a plain for-loop, no rollback).Per user decision: PawWork has no legacy users / no legacy config in the wild, so the entire
multieditmigration code path is removed (not preserved as no-op).apply_patch.txtwas originally going to get a*** Delete File:prose guardrail; dropped after design review showed the wording risked confusing models on legitimate refactor-time source deletions (apply_patch IS the right tool for tracked-source removal; trash is wrong for git-tracked files). Revisit if real sessions show models reaching for*** Delete File:on user-facing files.Related Issue
Closes #129 (PR1 only; PR2 will handle codesearch removal).
How To Verify
Local results:
multieditgrep: zero hitsManual smoke matrix (deferred to real PawWork sessions, recorded in PR comment):
ProviderID.opencoderuntaskdispatch; verify deletion goes throughtrash(notrm); verify multi-edit-style change works as repeatededitcalls; no model attemptsmultieditScreenshots or Recordings
N/A. No UI surface changed.
Checklist
rmdeny rule atagent.ts:99-110unchanged; bash.txt now points at trash explicitly)dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Removed Features
Behavior Changes
Documentation
Tests