fix(tool): remove built-in trash tool - #314
Merged
Merged
Conversation
Astro-Han
added
bug
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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)
📝 WalkthroughWalkthroughThe built-in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Review rate limit: 0/3 reviews remaining, refill in 4 minutes and 48 seconds. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the TrashTool and its associated trash dependency, replacing the specialized tool with updated instructions in bash.txt that guide the agent to use system-native trash commands like gio trash or trash-put. Correspondingly, the tool registry, tests, and lockfile have been updated to reflect the removal of this functionality and its dependencies. Feedback was provided regarding the platform-specific nature of the command -v instruction in the updated prompt, suggesting a more shell-agnostic approach to support Windows environments.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/test/tool/registry.test.ts`:
- Around line 50-57: The root-level bun.lock assertions in registry.test.ts (the
lockfile variable and its subsequent expect checks) are too broad; instead parse
the bun.lock contents and scope assertions to the "packages/opencode" workspace
section only (or remove the lockfile checks entirely if you prefer relying on
the packageJson assertion). Locate the lockfile variable in the test, extract
the "packages/opencode" subsection from its text, and run the
expect(...).not.toContain checks against that subsection (ensuring the keys
checked—e.g., '"trash": "10"' and '"trash": ["trash@'—are applied to the scoped
string) so sibling workspaces cannot make the test fail.
🪄 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: cd08ae55-66e8-4bff-98ed-2dd3037ee000
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/opencode/package.jsonpackages/opencode/src/tool/bash.txtpackages/opencode/src/tool/registry.tspackages/opencode/src/tool/trash.tspackages/opencode/src/tool/trash.txtpackages/opencode/test/tool/registry.test.tspackages/opencode/test/tool/trash.test.ts
💤 Files with no reviewable changes (5)
- packages/opencode/src/tool/trash.txt
- packages/opencode/package.json
- packages/opencode/src/tool/trash.ts
- packages/opencode/src/tool/registry.ts
- packages/opencode/test/tool/trash.test.ts
📜 Review details
🧰 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/tool/registry.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/tool/registry.test.ts
🧠 Learnings (15)
📓 Common learnings
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.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:172-175
Timestamp: 2026-04-28T04:56:18.533Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the `files dies on nonexistent directory` test hardcodes `/tmp/nonexistent-dir-12345` as the missing-directory path. This is upstream-inherited behaviour adopted wholesale via the graft strategy (`project_upstream_strategy.md`). The fix — replacing the hardcoded path with a platform-neutral guaranteed-nonexistent child path derived from the `tmpdir` fixture — is intentionally deferred to a follow-up PR or upstream report, to avoid mixing bugfix + refactor intents and drifting the diff from the upstream baseline future syncs need. Do NOT re-flag the hardcoded `/tmp/nonexistent-dir-12345` path in this file as a blocking or actionable issue until the follow-up PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/question.ts:6-16
Timestamp: 2026-04-28T06:47:20.342Z
Learning: In Astro-Han/pawwork, `packages/opencode/src/tool/question.ts` is a permanent PawWork carve-out: `Question.Prompt` (defined in `packages/opencode/src/question/index.ts`) remains a Zod schema, NOT an Effect Schema. The `ts-expect-error` boundary in `tool/registry.ts` marks the Zod ↔ Effect Schema seam. The dropped `packages/opencode/test/tool/question.test.ts` covered the old Zod-mixed surface. Schema-bound regression tests for this tool are intentionally deferred until `question/index.ts` migrates to Effect Schema in its own dedicated PR. Do NOT request restoring question tool tests in upstream-sync PRs; flag it only in the PR that migrates `question/index.ts` to Effect Schema.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/grep.test.ts:16-22
Timestamp: 2026-04-28T05:36:25.456Z
Learning: In `packages/opencode/test/tool/grep.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a manual `ManagedRuntime.make(Layer.mergeAll(...))` setup and raw `bun:test` test cases rather than the `testEffect(...)` harness. The migration to `testEffect` + `it.live(...)` + `provideTmpdirInstance(...)` is a whole-file rewrite deferred to a dedicated sweep PR that will migrate all tool tests still on the manual ManagedRuntime pattern together. Do NOT re-flag the ManagedRuntime setup in this file as needing harness migration until that sweep PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:311-328
Timestamp: 2026-04-28T05:36:18.200Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the ripgrep binary download flow (`HttpClientRequest.get` → `fs.writeWithDirs` → `extract`) does NOT verify a pinned SHA-256 checksum. Adding per-platform hash verification (15.1.0 × 6 platforms) requires a dedicated manifest + `crypto.subtle.digest` step and is intentionally deferred to a follow-up PR. Existing mitigations: (1) `VERSION` is pinned to `"15.1.0"`, (2) downloads are over HTTPS from GitHub Releases, (3) PawWork production builds ship a bundled `rg` binary so this download path is a last-resort fallback (system PATH → cached → download). Do NOT re-flag the missing checksum verification in this file until the dedicated follow-up PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:0-0
Timestamp: 2026-04-28T05:36:24.561Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), after the streaming-Service refactor of `ripgrep.ts`, there is only one code path that strips `RIPGREP_CONFIG_PATH` from the spawned environment (`delete env.RIPGREP_CONFIG_PATH` at `src/file/ripgrep.ts` line ~155). There is no longer a separate "worker" vs "direct" execution mode, so a single test titled "ignores RIPGREP_CONFIG_PATH from spawned env" (with an explanatory comment) is the correct and complete coverage. Do NOT flag the absence of a separate worker-mode RIPGREP_CONFIG_PATH test as a gap.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/edit.ts:35-45
Timestamp: 2026-04-28T04:38:11.727Z
Learning: In `packages/opencode/src/tool/edit.ts` (Astro-Han/pawwork, PR `#270`), the module-scoped `Map<string, Semaphore.Semaphore>` (`locks`) and `lock(filePath)` helper are intentionally left as a global, never-cleaned registry. PR `#270` is a pure upstream-sync graft and mixing in a bugfix would drift the diff from the upstream baseline. The fix (moving per-file semaphores into `InstanceState`/`ScopedCache` for per-instance lifecycle management) is tracked as a follow-up to land in its own PR or be reported upstream. Do NOT re-flag the absence of InstanceState wiring for the lock map in this file until the follow-up PR is opened.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:469-470
Timestamp: 2026-04-28T08:29:02.858Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the `tree()` function filters files with `if (file.includes(".opencode")) continue`, which is a raw substring match that would also drop unrelated paths whose names merely contain `.opencode` (e.g., `.opencode-backup`). The correct fix is `if (file.split(path.sep).includes(".opencode")) continue` to match only the exact path segment. However, upstream `dev:packages/opencode/src/file/ripgrep.ts` carries the identical false-positive filter. PR `#270` is an upstream-sync graft; fixing it here would diverge from the baseline. The fix is deferred to a PawWork ripgrep filter cleanup PR or an upstream-first fix, to land alongside the other ripgrep deferrals (Schema.Class, bytes-union, abort short-circuit, mixed-separator). Do NOT re-flag the `.opencode` substring filter in upstream-sync PRs.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:9-10
Timestamp: 2026-04-28T04:38:21.935Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the tests intentionally use a local `run` helper (`effect.pipe(Effect.provide(Ripgrep.defaultLayer), Effect.runPromise)`) rather than the `testEffect(...)` harness. This file was adopted from upstream wholesale as part of an upstream-sync graft; migrating to `testEffect` + `it.live(...)` was deferred to a separate follow-up PR to avoid mixing refactor and harness-migration intents within the sync. Do NOT re-flag the local `run` wrapper as needing harness migration until that follow-up lands.
📚 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
📚 Learning: 2026-04-28T05:36:25.456Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/grep.test.ts:16-22
Timestamp: 2026-04-28T05:36:25.456Z
Learning: In `packages/opencode/test/tool/grep.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a manual `ManagedRuntime.make(Layer.mergeAll(...))` setup and raw `bun:test` test cases rather than the `testEffect(...)` harness. The migration to `testEffect` + `it.live(...)` + `provideTmpdirInstance(...)` is a whole-file rewrite deferred to a dedicated sweep PR that will migrate all tool tests still on the manual ManagedRuntime pattern together. Do NOT re-flag the ManagedRuntime setup in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T04:56:21.338Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T05:36:24.561Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:0-0
Timestamp: 2026-04-28T05:36:24.561Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), after the streaming-Service refactor of `ripgrep.ts`, there is only one code path that strips `RIPGREP_CONFIG_PATH` from the spawned environment (`delete env.RIPGREP_CONFIG_PATH` at `src/file/ripgrep.ts` line ~155). There is no longer a separate "worker" vs "direct" execution mode, so a single test titled "ignores RIPGREP_CONFIG_PATH from spawned env" (with an explanatory comment) is the correct and complete coverage. Do NOT flag the absence of a separate worker-mode RIPGREP_CONFIG_PATH test as a gap.
Applied to files:
packages/opencode/test/tool/registry.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/tool/registry.test.ts
📚 Learning: 2026-04-28T04:56:18.533Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:172-175
Timestamp: 2026-04-28T04:56:18.533Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the `files dies on nonexistent directory` test hardcodes `/tmp/nonexistent-dir-12345` as the missing-directory path. This is upstream-inherited behaviour adopted wholesale via the graft strategy (`project_upstream_strategy.md`). The fix — replacing the hardcoded path with a platform-neutral guaranteed-nonexistent child path derived from the `tmpdir` fixture — is intentionally deferred to a follow-up PR or upstream report, to avoid mixing bugfix + refactor intents and drifting the diff from the upstream baseline future syncs need. Do NOT re-flag the hardcoded `/tmp/nonexistent-dir-12345` path in this file as a blocking or actionable issue until the follow-up PR lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T06:47:20.342Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/question.ts:6-16
Timestamp: 2026-04-28T06:47:20.342Z
Learning: In Astro-Han/pawwork, `packages/opencode/src/tool/question.ts` is a permanent PawWork carve-out: `Question.Prompt` (defined in `packages/opencode/src/question/index.ts`) remains a Zod schema, NOT an Effect Schema. The `ts-expect-error` boundary in `tool/registry.ts` marks the Zod ↔ Effect Schema seam. The dropped `packages/opencode/test/tool/question.test.ts` covered the old Zod-mixed surface. Schema-bound regression tests for this tool are intentionally deferred until `question/index.ts` migrates to Effect Schema in its own dedicated PR. Do NOT request restoring question tool tests in upstream-sync PRs; flag it only in the PR that migrates `question/index.ts` to Effect Schema.
Applied to files:
packages/opencode/test/tool/registry.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/tool/registry.test.ts
📚 Learning: 2026-04-27T11:18:47.596Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/agent/agent.test.ts:440-447
Timestamp: 2026-04-27T11:18:47.596Z
Learning: In `packages/opencode/test/agent/agent.test.ts` (Astro-Han/pawwork), all agent-permission tests intentionally use the manual `tmpdir()` + `Instance.provide(...)` pattern. Do not flag individual tests in this file for conversion to `provideTmpdirInstance(...)` or `provideInstance(...)`; a full harness migration would be a separate PR if the pattern ever needs to change.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T04:38:21.935Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:9-10
Timestamp: 2026-04-28T04:38:21.935Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the tests intentionally use a local `run` helper (`effect.pipe(Effect.provide(Ripgrep.defaultLayer), Effect.runPromise)`) rather than the `testEffect(...)` harness. This file was adopted from upstream wholesale as part of an upstream-sync graft; migrating to `testEffect` + `it.live(...)` was deferred to a separate follow-up PR to avoid mixing refactor and harness-migration intents within the sync. Do NOT re-flag the local `run` wrapper as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-27T08:27:43.672Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:255-279
Timestamp: 2026-04-27T08:27:43.672Z
Learning: In `packages/opencode/src/session/diagnostics.ts` (PR `#264`, issue `#229`), target-level loop detection is intentionally tool-scoped. The `real` filter includes `r.tool === input.tool` and target signature keys are formatted as `target:${tool}:${targetHash}`. Cross-tool retries on the same URL/path/query are considered legitimate exploration (e.g., webfetch failed → switch to a different tool), NOT a stuck loop. Only same-tool + same-target accumulation counts toward block/stop. Do NOT suggest removing the tool scope from target signature keys or the `real` filter.
Applied to files:
packages/opencode/test/tool/registry.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 `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/opencode/test/tool/registry.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/tool/registry.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} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/opencode/test/tool/registry.test.ts
🔇 Additional comments (3)
packages/opencode/src/tool/bash.txt (1)
38-38: Good reversible-delete fallback guidance.This keeps the permanent-delete denylist intact while steering the model toward reversible OS commands and explicit confirmation when none is available.
packages/opencode/test/tool/registry.test.ts (2)
29-37: Well-targeted built-in exposure regression.Checking
ToolRegistry.ids()from a clean instance is the right contract for proving the built-intrashtool is no longer exposed.
43-49: Nice prompt-surface contract check.Asserting the bash prompt text directly should catch accidental reintroduction of the old built-in-trash instructions without coupling this test to registry internals.
Contributor
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/test/tool/registry.test.ts`:
- Around line 61-66: The test assumes another workspace follows
packages/opencode by using afterOpencodeWorkspaceHeader.search(...) and slicing
using that offset; when packages/opencode is the last workspace search() returns
-1 and the slice length is wrong causing a false failure. Fix by checking the
result of nextWorkspaceOffset (from afterOpencodeWorkspaceHeader.search(...))
and if it is -1 treat the end boundary as the rest of the string (use the
remainder length) when computing opencodeWorkspaceLockfileSection; update the
logic that uses opencodeWorkspaceStart, opencodeWorkspaceHeader.length and
nextWorkspaceOffset to compute the slice end so it works for both "another
workspace follows" and "last workspace" cases.
🪄 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: f8238f76-0116-4310-a41b-670504ff0504
📒 Files selected for processing (2)
packages/opencode/src/tool/bash.txtpackages/opencode/test/tool/registry.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: unit-app
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-desktop
- GitHub Check: typecheck
- GitHub Check: unit-opencode
- 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/test/tool/registry.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/tool/registry.test.ts
🧠 Learnings (23)
📓 Common learnings
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.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:172-175
Timestamp: 2026-04-28T04:56:18.533Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the `files dies on nonexistent directory` test hardcodes `/tmp/nonexistent-dir-12345` as the missing-directory path. This is upstream-inherited behaviour adopted wholesale via the graft strategy (`project_upstream_strategy.md`). The fix — replacing the hardcoded path with a platform-neutral guaranteed-nonexistent child path derived from the `tmpdir` fixture — is intentionally deferred to a follow-up PR or upstream report, to avoid mixing bugfix + refactor intents and drifting the diff from the upstream baseline future syncs need. Do NOT re-flag the hardcoded `/tmp/nonexistent-dir-12345` path in this file as a blocking or actionable issue until the follow-up PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/question.ts:6-16
Timestamp: 2026-04-28T06:47:20.342Z
Learning: In Astro-Han/pawwork, `packages/opencode/src/tool/question.ts` is a permanent PawWork carve-out: `Question.Prompt` (defined in `packages/opencode/src/question/index.ts`) remains a Zod schema, NOT an Effect Schema. The `ts-expect-error` boundary in `tool/registry.ts` marks the Zod ↔ Effect Schema seam. The dropped `packages/opencode/test/tool/question.test.ts` covered the old Zod-mixed surface. Schema-bound regression tests for this tool are intentionally deferred until `question/index.ts` migrates to Effect Schema in its own dedicated PR. Do NOT request restoring question tool tests in upstream-sync PRs; flag it only in the PR that migrates `question/index.ts` to Effect Schema.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/grep.test.ts:16-22
Timestamp: 2026-04-28T05:36:25.456Z
Learning: In `packages/opencode/test/tool/grep.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a manual `ManagedRuntime.make(Layer.mergeAll(...))` setup and raw `bun:test` test cases rather than the `testEffect(...)` harness. The migration to `testEffect` + `it.live(...)` + `provideTmpdirInstance(...)` is a whole-file rewrite deferred to a dedicated sweep PR that will migrate all tool tests still on the manual ManagedRuntime pattern together. Do NOT re-flag the ManagedRuntime setup in this file as needing harness migration until that sweep PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:0-0
Timestamp: 2026-04-28T05:36:24.561Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), after the streaming-Service refactor of `ripgrep.ts`, there is only one code path that strips `RIPGREP_CONFIG_PATH` from the spawned environment (`delete env.RIPGREP_CONFIG_PATH` at `src/file/ripgrep.ts` line ~155). There is no longer a separate "worker" vs "direct" execution mode, so a single test titled "ignores RIPGREP_CONFIG_PATH from spawned env" (with an explanatory comment) is the correct and complete coverage. Do NOT flag the absence of a separate worker-mode RIPGREP_CONFIG_PATH test as a gap.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:311-328
Timestamp: 2026-04-28T05:36:18.200Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the ripgrep binary download flow (`HttpClientRequest.get` → `fs.writeWithDirs` → `extract`) does NOT verify a pinned SHA-256 checksum. Adding per-platform hash verification (15.1.0 × 6 platforms) requires a dedicated manifest + `crypto.subtle.digest` step and is intentionally deferred to a follow-up PR. Existing mitigations: (1) `VERSION` is pinned to `"15.1.0"`, (2) downloads are over HTTPS from GitHub Releases, (3) PawWork production builds ship a bundled `rg` binary so this download path is a last-resort fallback (system PATH → cached → download). Do NOT re-flag the missing checksum verification in this file until the dedicated follow-up PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/edit.ts:35-45
Timestamp: 2026-04-28T04:38:11.727Z
Learning: In `packages/opencode/src/tool/edit.ts` (Astro-Han/pawwork, PR `#270`), the module-scoped `Map<string, Semaphore.Semaphore>` (`locks`) and `lock(filePath)` helper are intentionally left as a global, never-cleaned registry. PR `#270` is a pure upstream-sync graft and mixing in a bugfix would drift the diff from the upstream baseline. The fix (moving per-file semaphores into `InstanceState`/`ScopedCache` for per-instance lifecycle management) is tracked as a follow-up to land in its own PR or be reported upstream. Do NOT re-flag the absence of InstanceState wiring for the lock map in this file until the follow-up PR is opened.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:469-470
Timestamp: 2026-04-28T08:29:02.858Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the `tree()` function filters files with `if (file.includes(".opencode")) continue`, which is a raw substring match that would also drop unrelated paths whose names merely contain `.opencode` (e.g., `.opencode-backup`). The correct fix is `if (file.split(path.sep).includes(".opencode")) continue` to match only the exact path segment. However, upstream `dev:packages/opencode/src/file/ripgrep.ts` carries the identical false-positive filter. PR `#270` is an upstream-sync graft; fixing it here would diverge from the baseline. The fix is deferred to a PawWork ripgrep filter cleanup PR or an upstream-first fix, to land alongside the other ripgrep deferrals (Schema.Class, bytes-union, abort short-circuit, mixed-separator). Do NOT re-flag the `.opencode` substring filter in upstream-sync PRs.
📚 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
📚 Learning: 2026-04-28T05:36:25.456Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/grep.test.ts:16-22
Timestamp: 2026-04-28T05:36:25.456Z
Learning: In `packages/opencode/test/tool/grep.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a manual `ManagedRuntime.make(Layer.mergeAll(...))` setup and raw `bun:test` test cases rather than the `testEffect(...)` harness. The migration to `testEffect` + `it.live(...)` + `provideTmpdirInstance(...)` is a whole-file rewrite deferred to a dedicated sweep PR that will migrate all tool tests still on the manual ManagedRuntime pattern together. Do NOT re-flag the ManagedRuntime setup in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T04:56:21.338Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T06:47:20.342Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/question.ts:6-16
Timestamp: 2026-04-28T06:47:20.342Z
Learning: In Astro-Han/pawwork, `packages/opencode/src/tool/question.ts` is a permanent PawWork carve-out: `Question.Prompt` (defined in `packages/opencode/src/question/index.ts`) remains a Zod schema, NOT an Effect Schema. The `ts-expect-error` boundary in `tool/registry.ts` marks the Zod ↔ Effect Schema seam. The dropped `packages/opencode/test/tool/question.test.ts` covered the old Zod-mixed surface. Schema-bound regression tests for this tool are intentionally deferred until `question/index.ts` migrates to Effect Schema in its own dedicated PR. Do NOT request restoring question tool tests in upstream-sync PRs; flag it only in the PR that migrates `question/index.ts` to Effect Schema.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T05:36:24.561Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:0-0
Timestamp: 2026-04-28T05:36:24.561Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), after the streaming-Service refactor of `ripgrep.ts`, there is only one code path that strips `RIPGREP_CONFIG_PATH` from the spawned environment (`delete env.RIPGREP_CONFIG_PATH` at `src/file/ripgrep.ts` line ~155). There is no longer a separate "worker" vs "direct" execution mode, so a single test titled "ignores RIPGREP_CONFIG_PATH from spawned env" (with an explanatory comment) is the correct and complete coverage. Do NOT flag the absence of a separate worker-mode RIPGREP_CONFIG_PATH test as a gap.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T04:56:18.533Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:172-175
Timestamp: 2026-04-28T04:56:18.533Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the `files dies on nonexistent directory` test hardcodes `/tmp/nonexistent-dir-12345` as the missing-directory path. This is upstream-inherited behaviour adopted wholesale via the graft strategy (`project_upstream_strategy.md`). The fix — replacing the hardcoded path with a platform-neutral guaranteed-nonexistent child path derived from the `tmpdir` fixture — is intentionally deferred to a follow-up PR or upstream report, to avoid mixing bugfix + refactor intents and drifting the diff from the upstream baseline future syncs need. Do NOT re-flag the hardcoded `/tmp/nonexistent-dir-12345` path in this file as a blocking or actionable issue until the follow-up PR lands.
Applied to files:
packages/opencode/test/tool/registry.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/tool/registry.test.ts
📚 Learning: 2026-04-28T04:38:21.935Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/file/ripgrep.test.ts:9-10
Timestamp: 2026-04-28T04:38:21.935Z
Learning: In `packages/opencode/test/file/ripgrep.test.ts` (Astro-Han/pawwork, PR `#270`), the tests intentionally use a local `run` helper (`effect.pipe(Effect.provide(Ripgrep.defaultLayer), Effect.runPromise)`) rather than the `testEffect(...)` harness. This file was adopted from upstream wholesale as part of an upstream-sync graft; migrating to `testEffect` + `it.live(...)` was deferred to a separate follow-up PR to avoid mixing refactor and harness-migration intents within the sync. Do NOT re-flag the local `run` wrapper as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-27T11:19:24.963Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/websearch-auth.test.ts:0-0
Timestamp: 2026-04-27T11:19:24.963Z
Learning: In `packages/opencode/test/tool/websearch-auth.test.ts` (Astro-Han/pawwork), the tests intentionally use a small local `runWith` runner with raw `bun:test` and `Effect.runPromise` rather than the `testEffect` harness. Each test case injects a custom in-memory `Auth.Service` layer; switching to `testEffect` would be style-only churn without changing risk coverage. Do not flag these tests as needing harness migration.
Applied to files:
packages/opencode/test/tool/registry.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} : Prefer Effect-aware helpers from `fixture/fixture.ts` over building manual runtimes in tests: use `tmpdirScoped()` for scoped temp directories, `provideInstance(dir)(effect)` for low-level binding without directory creation, `provideTmpdirInstance(...)` for single temp instance binding, or `provideTmpdirServer(...)` for tests that also need the test LLM server.
Applied to files:
packages/opencode/test/tool/registry.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/tool/registry.test.ts
📚 Learning: 2026-04-28T04:38:11.727Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/edit.ts:35-45
Timestamp: 2026-04-28T04:38:11.727Z
Learning: In `packages/opencode/src/tool/edit.ts` (Astro-Han/pawwork, PR `#270`), the module-scoped `Map<string, Semaphore.Semaphore>` (`locks`) and `lock(filePath)` helper are intentionally left as a global, never-cleaned registry. PR `#270` is a pure upstream-sync graft and mixing in a bugfix would drift the diff from the upstream baseline. The fix (moving per-file semaphores into `InstanceState`/`ScopedCache` for per-instance lifecycle management) is tracked as a follow-up to land in its own PR or be reported upstream. Do NOT re-flag the absence of InstanceState wiring for the lock map in this file until the follow-up PR is opened.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T08:29:02.858Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:469-470
Timestamp: 2026-04-28T08:29:02.858Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the `tree()` function filters files with `if (file.includes(".opencode")) continue`, which is a raw substring match that would also drop unrelated paths whose names merely contain `.opencode` (e.g., `.opencode-backup`). The correct fix is `if (file.split(path.sep).includes(".opencode")) continue` to match only the exact path segment. However, upstream `dev:packages/opencode/src/file/ripgrep.ts` carries the identical false-positive filter. PR `#270` is an upstream-sync graft; fixing it here would diverge from the baseline. The fix is deferred to a PawWork ripgrep filter cleanup PR or an upstream-first fix, to land alongside the other ripgrep deferrals (Schema.Class, bytes-union, abort short-circuit, mixed-separator). Do NOT re-flag the `.opencode` substring filter in upstream-sync PRs.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T07:27:49.810Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:45-72
Timestamp: 2026-04-28T07:27:49.810Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork), `PathText`, the `lines` field inside `SearchMatch`, and `submatches[].match` accept only `{text: string}` and do NOT handle ripgrep's `{bytes: base64}` JSON variant (emitted for non-UTF-8 paths/match text). This matches the upstream `dev:packages/opencode/src/file/ripgrep.ts` baseline exactly. The fix — adding `Schema.Union([Schema.Struct({text: Schema.String}), Schema.Struct({bytes: Schema.String})])` for each location plus base64-decode in downstream consumers — is intentionally deferred to either a dedicated PawWork PR or an upstream-first fix inherited via the next sync. Do NOT re-flag the missing `{bytes: ...}` union variants in upstream-sync PRs; flag it only in a PawWork-authored PR that directly touches these schema definitions or their downstream consumers.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-27T10:33:12.228Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-27T08:27:43.672Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/diagnostics.ts:255-279
Timestamp: 2026-04-27T08:27:43.672Z
Learning: In `packages/opencode/src/session/diagnostics.ts` (PR `#264`, issue `#229`), target-level loop detection is intentionally tool-scoped. The `real` filter includes `r.tool === input.tool` and target signature keys are formatted as `target:${tool}:${targetHash}`. Cross-tool retries on the same URL/path/query are considered legitimate exploration (e.g., webfetch failed → switch to a different tool), NOT a stuck loop. Only same-tool + same-target accumulation counts toward block/stop. Do NOT suggest removing the tool scope from target signature keys or the `real` filter.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-24T00:02:53.315Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 203
File: packages/app/e2e/sidebar/sidebar-session-links.spec.ts:34-55
Timestamp: 2026-04-24T00:02:53.315Z
Learning: In Astro-Han/pawwork E2E tests (`packages/app/e2e/**/*.spec.ts`), `project.trackDirectory()` and `project.trackSession()` cannot be called before `project.open()` — the `project` fixture throws until `open()` initializes its internal state. The correct pattern is: call `project.trackSession(sessionID)` from inside the `beforeGoto` callback (where state already exists), call `project.trackDirectory(directory)` and cross-workspace `project.trackSession(id, directory)` immediately after `project.open()` returns, and rely on explicit `finally` cleanup (e.g. `cleanupSession` / `cleanupTestProject`) for any resources created before `open()` that cannot yet be tracked via the fixture.
Applied to files:
packages/opencode/test/tool/registry.test.ts
📚 Learning: 2026-04-28T08:14:31.436Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/edit.ts:48-48
Timestamp: 2026-04-28T08:14:31.436Z
Learning: In `packages/opencode/src/tool/edit.ts` (Astro-Han/pawwork), the `filePath` schema description (`"The absolute path to the file to modify"`) is upstream-inherited from `dev:packages/opencode/src/tool/edit.ts:48`. The runtime actually accepts relative paths (resolved via `Instance.directory` at lines 79-81), but the description fix is intentionally deferred to a single PawWork-authored description-cleanup PR that will also cover the identical mismatch in `packages/opencode/src/tool/write.ts:24`. Do NOT re-flag the too-narrow `filePath` description in upstream-sync PRs; flag it only in the dedicated description-cleanup PR.
Applied to files:
packages/opencode/test/tool/registry.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 `init` option in `tmpdir` to define custom setup functions that can return extra data accessible via `tmp.extra`, and use the `dispose` option for custom cleanup logic.
Applied to files:
packages/opencode/test/tool/registry.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/tool/registry.test.ts
📚 Learning: 2026-04-27T11:18:47.596Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/agent/agent.test.ts:440-447
Timestamp: 2026-04-27T11:18:47.596Z
Learning: In `packages/opencode/test/agent/agent.test.ts` (Astro-Han/pawwork), all agent-permission tests intentionally use the manual `tmpdir()` + `Instance.provide(...)` pattern. Do not flag individual tests in this file for conversion to `provideTmpdirInstance(...)` or `provideInstance(...)`; a full harness migration would be a separate PR if the pattern ever needs to change.
Applied to files:
packages/opencode/test/tool/registry.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} : When a test needs instance-local state, prefer `provideTmpdirInstance(...)` or `provideInstance(...)` over manual `Instance.provide(...)` inside Promise-style tests.
Applied to files:
packages/opencode/test/tool/registry.test.ts
🔇 Additional comments (2)
packages/opencode/src/tool/bash.txt (1)
38-38: Good safety-focused replacement for built-in trash behavior.This keeps reversible-delete preference explicit, adds shell-specific availability checks, and preserves user confirmation before irreversible deletion.
packages/opencode/test/tool/registry.test.ts (1)
29-37: Registry contract assertion is correct and targeted.The new test cleanly enforces that the removed built-in
trashtool is no longer exposed viaToolRegistry.ids().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
trashpackage dependency.Why
Issue #310 showed the packaged macOS app can fail when the
trashnpm package tries to execute its bundled native helper from insideapp.asar. The design direction is to remove this built-in tool instead of fixing the package path, keep permanent-delete denylist guardrails, and let the model use shell-native reversible trash commands when available.Design discussion: #310 (comment)
Related Issue
Closes #310
How To Verify
Cold-read review also passed with no Critical, Important, or Minor findings.
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Features Removed
Documentation
Tests