fix(opencode): sync runtime safety hotfixes#504
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a secure application tmp path (Global.Path.tmp), implements byte-limited git patch APIs and cumulative patch budgeting, sanitizes lone UTF‑16 surrogates in outgoing messages, tightens media/compaction message logic, refactors tool/agent execution and effect-driven cancellation, switches external-directory permission checks to use resolved realpaths, and expands tests across these areas. ChangesInfrastructure and Global Configuration
Git and VCS Diff System
Provider Message Handling
Tool and Agent Execution
Session and Permission Management
Additional Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 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)
|
![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 introduces a global temporary directory, refactors VCS diffing to use native git patches with size limits, and enhances media handling in tool results, specifically addressing Bedrock's PDF limitations. It also implements surrogate sanitization for text content and refines subagent permission inheritance and cancellation logic. A critical issue was identified in the message compaction logic where the reordering of messages could lead to unintended data loss for messages preceding the tail index.
![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: 5
🧹 Nitpick comments (7)
packages/opencode/test/skill/skill.test.ts (1)
253-290: ⚡ Quick winAdd a complementary flag-off test for Claude skill exclusion.
This new default-path assertion is good, but please add a sibling case that sets
OPENCODE_DISABLE_CLAUDE_CODE_SKILLS=trueand verifies.claude/skillsis excluded while.agents/skillsstill loads. It will protect the new decoupled-flag behavior from regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/skill/skill.test.ts` around lines 253 - 290, Add a sibling test that creates the same temp layout (.agents/skills/agent-skill and .claude/skills/claude-skill) but sets OPENCODE_DISABLE_CLAUDE_CODE_SKILLS=true before invoking Skill.all(); assert that the agent-skill is present and claude-skill is not. Use the same tmpdir setup and Instance.provide pattern, set process.env.OPENCODE_DISABLE_CLAUDE_CODE_SKILLS = "true" (and restore/clear it after the test), call Skill.all(), then expect skills.find(s => s.name === "agent-skill") toBeDefined() and expect(skills.find(s => s.name === "claude-skill")).toBeUndefined().packages/ui/src/components/session-diff.ts (1)
30-54: ⚡ Quick winThe undefined-dereference path inside
trysilently swallows any future TypeError, not just parse failures.
const [patch] = parsePatch(diff.patch)can yieldundefined(whenparsePatchreturns[]), and the resultingTypeErroronpatch.hunksis the mechanism that triggers thecatch. This is an implicit error-detection pattern: any future accidentalTypeErrorintroduced inside thetryblock (e.g., a null-dereference in newly added code) would be swallowed identically and silently degrade the diff view to empty content.Consider guarding explicitly:
♻️ Proposed fix — explicit guard instead of implicit TypeError
try { - const [patch] = parsePatch(diff.patch) + const patches = parsePatch(diff.patch) + if (!patches.length) return { before: "", after: "", patch: diff.patch } + const [patch] = patches const beforeLines = [] const afterLines = []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/session-diff.ts` around lines 30 - 54, The try block currently relies on a TypeError from accessing patch.hunks to detect parse failures; instead explicitly guard the parse result: call parsePatch(diff.patch), check the returned array (e.g., const parsed = parsePatch(diff.patch); const patch = parsed[0];) and if !patch or !patch.hunks immediately return the fallback { before: "", after: "", patch: diff.patch } (or throw a clear parse error), then proceed to build beforeLines/afterLines; keep the try/catch narrowly around parsePatch if you still want to catch parser exceptions, and don't swallow unrelated TypeErrors from the rest of the logic.packages/opencode/src/git/index.ts (2)
475-490: 💤 Low value
statUntrackedparsing is fragile to whitespace — minor.
result.text()is not trimmed andparts[1]carries the trailing<TAB><file><LF>aftersplit("\t").parseInthappens to be tolerant of leading whitespace onparts[0], and theparts[1]numeric prefix is parsed correctly becauseparseIntstops at the first non-digit (the tab is consumed by the split, but the second-tab separator meansparts[1]is just digits). So today this works, but a future formatting change in git's--numstat(or e.g. a leading BOM under odd locales) would silently break it. Consider trimming and matching on a regex like^(\d+|-)\t(\d+|-)\tfor robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/git/index.ts` around lines 475 - 490, statUntracked's parsing of result.text() is fragile to extra whitespace or formatting changes; update the parser in the statUntracked Effect.fn to trim the output and extract additions/deletions via a strict regex (e.g. match /^(\d+|-)\t(\d+|-)\t/) against result.text() instead of relying on split("\t"), then parse the captured groups into numbers (treat "-" as 0) before returning the Stat-shaped object; continue to respect result.truncated and ensure Number.isFinite checks remain in place.
127-143: ⚖️ Poor tradeoffStream is fully drained even after
maxOutputBytesis hit.
Stream.runFolddoesn't short-circuit, and the spawn isn't interrupted, so a 100 MB diff under a 10 MB cap still pays the cost of reading the trailing 90 MB and waiting for git to terminate naturally. Memory is correctly bounded (chunksdoesn't grow past the cap), but wall-clock and IO are not.Reasonable to defer for this PR (caps are 10 MB and patches are usually small), but for adversarial inputs consider:
Stream.takeWhileover the byte counter and interrupting the child viahandle.kill?.(...)once truncation is observed, or- forking a watcher fiber that interrupts the spawn scope when the truncated flag flips.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/git/index.ts` around lines 127 - 143, The current collect implementation using Stream.runFold continues draining handle.stdout even after opts.maxOutputBytes is reached; change it to stop consuming and abort the child process when truncation is hit: replace the runFold usage with a stream pipeline that tracks bytes (e.g., Stream.mapChunks/Stream.takeWhile or Stream.scan + Stream.takeWhile) so the stream short-circuits once accumulated bytes >= opts.maxOutputBytes, and on truncation call handle.kill?.('SIGKILL' or appropriate) from the interrupt/finalizer (or spawn a watcher fiber that interrupts the spawn scope) so git is terminated early; keep the same output shape ({ buffer: Buffer.concat(...), truncated }) and reference collect, handle.stdout, opts.maxOutputBytes and handle.kill in your changes.packages/opencode/src/project/vcs.ts (2)
15-17: 💤 Low value
MAX_PATCH_BYTES === MAX_TOTAL_PATCH_BYTESlets a single file exhaust the batch budget.With both caps set to 10 MB, a single 10 MB patch can fill the entire batch budget and force every subsequent file to render as
emptyPatch(...). If that's the intended fail-safe behavior (front-loaded budget, explicit cap), consider adding a brief comment so the equality isn't read as a copy-paste mistake. Otherwise, set the per-file cap below the total (e.g. per-file = total / N or a fixed fraction) so one large file can't starve the rest of the diff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/project/vcs.ts` around lines 15 - 17, The per-file and total patch caps are both set equal (MAX_PATCH_BYTES and MAX_TOTAL_PATCH_BYTES), allowing a single file to consume the entire batch budget and force other files to become emptyPatch; change the per-file cap (MAX_PATCH_BYTES) to be strictly less than the total (e.g., set MAX_PATCH_BYTES = Math.floor(MAX_TOTAL_PATCH_BYTES / N) or a fixed fraction of MAX_TOTAL_PATCH_BYTES) or add an explicit comment next to PATCH_CONTEXT_LINES / MAX_* constants explaining the intentional equality; update the constants PATCH_CONTEXT_LINES, MAX_PATCH_BYTES and MAX_TOTAL_PATCH_BYTES (and any logic that assumes per-file vs total caps) accordingly so one large patch cannot starve the batch.
32-52: ⚡ Quick winDead
patchesMap field onPatchBatch.
PatchBatch.patchesis declared and constructed at every call site (lines 57, 89, 115) but never read or written byboundedPatchor any caller. It looks like a leftover from an earlier "collect into a map then merge" design.♻️ Drop the unused field
- type PatchBatch = { patches: Map<string, string>; total: number; capped: boolean } + type PatchBatch = { total: number; capped: boolean }- const batch: PatchBatch = { patches: new Map(), total: 0, capped: false } + const batch: PatchBatch = { total: 0, capped: false }(apply at all three construction sites)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/project/vcs.ts` around lines 32 - 52, PatchBatch currently declares an unused patches field; remove that dead field and its initializations so the type and its constructors only include total and capped (i.e., change type PatchBatch = { total: number; capped: boolean } and drop patches from every place where a PatchBatch is created), and ensure boundedPatch (and callers like the sites that build the batch) continue to use total and capped with existing symbols (boundedPatch, emptyPatch, MAX_TOTAL_PATCH_BYTES, Git.Item, Git.Patch) unchanged.packages/core/test/npm.test.ts (1)
78-78: ⚡ Quick winAssertion only checks
isSome, not the actual entrypoint value.The test verifies
Option.isSome(entry.entrypoint)but doesn't inspect what theSomecontains. If the fallback logic returnsSome("")orSome(anyWrongPath), this assertion still passes, giving no protection against regressions in the path-computation logic. Strengthening to check the expectednode_modules/fixture-providersuffix would make the test meaningful:✅ Proposed stronger assertion
- expect(Option.isSome(entry.entrypoint)).toBe(true) + expect(Option.isSome(entry.entrypoint)).toBe(true) + expect( + Option.getOrThrow(entry.entrypoint).includes(path.join("node_modules", "fixture-provider")) + ).toBe(true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/npm.test.ts` at line 78, The test currently only asserts Option.isSome(entry.entrypoint) which allows wrong or empty entrypoint values; update the assertion to unwrap and verify the actual value of entry.entrypoint (e.g., check that the Some string endsWith or equals the expected "node_modules/fixture-provider" entrypoint path) so the test fails on incorrect fallback paths; locate the assertion using Option.isSome and entry.entrypoint in packages/core/test/npm.test.ts and replace or add an assertion that inspects the Option's contained string for the expected suffix/value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/git/index.ts`:
- Around line 144-153: The current Result object mixes stdout and stderr
truncation (truncated: stdout.truncated || stderr.truncated), which causes
stderr noise to invalidate otherwise-complete stdout patches; change the result
to expose truncation separately (e.g., stdoutTruncated and stderrTruncated) and
set truncated (or the existing truncated field used by patch helpers) to reflect
only stdout truncation. Concretely, update the block that collects handle.stdout
and handle.stderr (used with collect and handle.exitCode) to keep stdout.buffer
and stderr.buffer but emit stdoutTruncated = stdout.truncated and
stderrTruncated = stderr.truncated, and set truncated: stdout.truncated (or
remove the combined OR) so downstream helpers like patchResult/boundedPatch only
react to stdout truncation; you can still apply a separate cap to stderr via
collect but ensure its flag is distinct.
In `@packages/opencode/src/session/message-v2.ts`:
- Around line 782-789: The current empty-text placeholder check
(hasAnthropicSignedReasoning) only tests part.metadata?.anthropic?.signature but
must also detect Bedrock-signed or redacted reasoning; update the predicate used
to set hasAnthropicSignedReasoning (or rename it to reflect both providers) to
also return true when part.metadata?.bedrock?.signature or
part.metadata?.bedrock?.redactedData is present, so the empty text branch in the
loop over msg.parts injects the synthetic space when Bedrock-signed/redacted
reasoning blocks are present; adjust the variable name if helpful and update any
related tests to cover Bedrock replay of empty reasoning blocks.
In `@packages/opencode/src/session/session.ts`:
- Around line 798-800: The compaction part update currently overwrites
p.tail_start_id with undefined when idMap.get(...) has no entry; instead, only
replace tail_start_id if a remap exists. In the block that checks p.type ===
"compaction" and p.tail_start_id, change the logic to test the presence of a
mapping (e.g., idMap.has(p.tail_start_id) or check that idMap.get(...) !==
undefined) and assign p.tail_start_id only when that mapping exists, leaving the
original p.tail_start_id intact otherwise; refer to the variables p,
p.tail_start_id and the idMap used in this function to locate and update the
code.
In `@packages/opencode/src/tool/agent.ts`:
- Around line 360-363: The cancel action ops.cancel(nextSession.id) is invoked
twice via cancelChild/runCancel.fork in the onParentAbort handler and again in
the Exit.hasInterrupts(exit) release branch; make the cancellation idempotent by
ensuring cancelChild is created once and guarded so it only executes a single
time across both paths. Concretely, construct a single cancel effect (the
existing cancelChild from ops.cancel(nextSession.id)) and share it between the
onParentAbort handler and the release branch, or wrap its invocation with a
one-time guard (e.g., an atomic/boolean "called" flag or a once/Promise-based
latch) so runCancel.fork(cancelChild) and the release-path call both delegate to
the same guarded cancel invocation; update the references around runCancel,
cancelChild, onParentAbort, and the Exit.hasInterrupts(exit) release branch
accordingly.
In `@packages/opencode/src/tool/read.ts`:
- Around line 159-160: Update the parameter documentation for params.offset in
the read function to reflect that offset 0 is accepted as an alias for the first
line/entry: change any wording that says strictly "1-indexed" to something like
"1-indexed (0 is accepted as an alias for the first entry)" and ensure the doc
block/comments immediately above the read function (and any exported API docs
that describe params.offset) explicitly mention this 0 alias while leaving the
existing validation logic for params.offset unchanged.
---
Nitpick comments:
In `@packages/core/test/npm.test.ts`:
- Line 78: The test currently only asserts Option.isSome(entry.entrypoint) which
allows wrong or empty entrypoint values; update the assertion to unwrap and
verify the actual value of entry.entrypoint (e.g., check that the Some string
endsWith or equals the expected "node_modules/fixture-provider" entrypoint path)
so the test fails on incorrect fallback paths; locate the assertion using
Option.isSome and entry.entrypoint in packages/core/test/npm.test.ts and replace
or add an assertion that inspects the Option's contained string for the expected
suffix/value.
In `@packages/opencode/src/git/index.ts`:
- Around line 475-490: statUntracked's parsing of result.text() is fragile to
extra whitespace or formatting changes; update the parser in the statUntracked
Effect.fn to trim the output and extract additions/deletions via a strict regex
(e.g. match /^(\d+|-)\t(\d+|-)\t/) against result.text() instead of relying on
split("\t"), then parse the captured groups into numbers (treat "-" as 0) before
returning the Stat-shaped object; continue to respect result.truncated and
ensure Number.isFinite checks remain in place.
- Around line 127-143: The current collect implementation using Stream.runFold
continues draining handle.stdout even after opts.maxOutputBytes is reached;
change it to stop consuming and abort the child process when truncation is hit:
replace the runFold usage with a stream pipeline that tracks bytes (e.g.,
Stream.mapChunks/Stream.takeWhile or Stream.scan + Stream.takeWhile) so the
stream short-circuits once accumulated bytes >= opts.maxOutputBytes, and on
truncation call handle.kill?.('SIGKILL' or appropriate) from the
interrupt/finalizer (or spawn a watcher fiber that interrupts the spawn scope)
so git is terminated early; keep the same output shape ({ buffer:
Buffer.concat(...), truncated }) and reference collect, handle.stdout,
opts.maxOutputBytes and handle.kill in your changes.
In `@packages/opencode/src/project/vcs.ts`:
- Around line 15-17: The per-file and total patch caps are both set equal
(MAX_PATCH_BYTES and MAX_TOTAL_PATCH_BYTES), allowing a single file to consume
the entire batch budget and force other files to become emptyPatch; change the
per-file cap (MAX_PATCH_BYTES) to be strictly less than the total (e.g., set
MAX_PATCH_BYTES = Math.floor(MAX_TOTAL_PATCH_BYTES / N) or a fixed fraction of
MAX_TOTAL_PATCH_BYTES) or add an explicit comment next to PATCH_CONTEXT_LINES /
MAX_* constants explaining the intentional equality; update the constants
PATCH_CONTEXT_LINES, MAX_PATCH_BYTES and MAX_TOTAL_PATCH_BYTES (and any logic
that assumes per-file vs total caps) accordingly so one large patch cannot
starve the batch.
- Around line 32-52: PatchBatch currently declares an unused patches field;
remove that dead field and its initializations so the type and its constructors
only include total and capped (i.e., change type PatchBatch = { total: number;
capped: boolean } and drop patches from every place where a PatchBatch is
created), and ensure boundedPatch (and callers like the sites that build the
batch) continue to use total and capped with existing symbols (boundedPatch,
emptyPatch, MAX_TOTAL_PATCH_BYTES, Git.Item, Git.Patch) unchanged.
In `@packages/opencode/test/skill/skill.test.ts`:
- Around line 253-290: Add a sibling test that creates the same temp layout
(.agents/skills/agent-skill and .claude/skills/claude-skill) but sets
OPENCODE_DISABLE_CLAUDE_CODE_SKILLS=true before invoking Skill.all(); assert
that the agent-skill is present and claude-skill is not. Use the same tmpdir
setup and Instance.provide pattern, set
process.env.OPENCODE_DISABLE_CLAUDE_CODE_SKILLS = "true" (and restore/clear it
after the test), call Skill.all(), then expect skills.find(s => s.name ===
"agent-skill") toBeDefined() and expect(skills.find(s => s.name ===
"claude-skill")).toBeUndefined().
In `@packages/ui/src/components/session-diff.ts`:
- Around line 30-54: The try block currently relies on a TypeError from
accessing patch.hunks to detect parse failures; instead explicitly guard the
parse result: call parsePatch(diff.patch), check the returned array (e.g., const
parsed = parsePatch(diff.patch); const patch = parsed[0];) and if !patch or
!patch.hunks immediately return the fallback { before: "", after: "", patch:
diff.patch } (or throw a clear parse error), then proceed to build
beforeLines/afterLines; keep the try/catch narrowly around parsePatch if you
still want to catch parser exceptions, and don't swallow unrelated TypeErrors
from the rest of the logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8aa60ab2-58d0-422b-8050-75f42ba96cd8
📒 Files selected for processing (39)
packages/core/src/flag/flag.tspackages/core/src/global.tspackages/core/src/npm.tspackages/core/test/fixture/effect-flock-worker.tspackages/core/test/npm.test.tspackages/core/test/util/effect-flock.test.tspackages/opencode/src/agent/agent.tspackages/opencode/src/format/index.tspackages/opencode/src/git/index.tspackages/opencode/src/mcp/index.tspackages/opencode/src/project/vcs.tspackages/opencode/src/provider/error.tspackages/opencode/src/provider/transform.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/session.tspackages/opencode/src/skill/index.tspackages/opencode/src/tool/agent.tspackages/opencode/src/tool/bash.tspackages/opencode/src/tool/bash.txtpackages/opencode/src/tool/read.tspackages/opencode/src/tool/registry.tspackages/opencode/src/util/timeout.tspackages/opencode/src/worktree/index.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/git/git.test.tspackages/opencode/test/mcp/headers.test.tspackages/opencode/test/project/vcs.test.tspackages/opencode/test/project/worktree.test.tspackages/opencode/test/provider/transform.test.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/session/messages-pagination.test.tspackages/opencode/test/session/retry.test.tspackages/opencode/test/skill/skill.test.tspackages/opencode/test/tool/agent.test.tspackages/opencode/test/tool/read.test.tspackages/opencode/test/util/timeout.test.tspackages/ui/src/components/session-diff.test.tspackages/ui/src/components/session-diff.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/tool/external-directory.ts`:
- Around line 17-18: resolveForPermission and assertExternalDirectoryEffect
currently normalize the incoming path before resolving filesystem symlinks,
which collapses `..` segments and can bypass symlink boundaries; change both
functions to call realpath (e.g., fs.realpathSync or your AppFileSystem realpath
equivalent) on the raw target first to resolve symlinks, then apply
normalization/AppFileSystem.normalizePath (with the base option for Windows) to
the realpath result so symlink traversal is preserved for the external-directory
permission check.
In `@packages/opencode/test/skill/skill.test.ts`:
- Around line 321-335: The test restores OPENCODE_DISABLE_CLAUDE_CODE_SKILLS
incorrectly by assigning the string "undefined" back when the variable was
originally absent; change the finally block so it checks the saved original
value and if original === undefined then delete
process.env.OPENCODE_DISABLE_CLAUDE_CODE_SKILLS, otherwise set
process.env.OPENCODE_DISABLE_CLAUDE_CODE_SKILLS = original; update the
restoration logic near the Instance.provide / Skill.all test to avoid leaking
env state between tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9634d76-06e3-4db0-8118-658015004a96
📒 Files selected for processing (15)
packages/core/src/global.tspackages/core/test/global.test.tspackages/core/test/npm.test.tspackages/opencode/src/git/index.tspackages/opencode/src/project/vcs.tspackages/opencode/src/session/session.tspackages/opencode/src/skill/index.tspackages/opencode/src/tool/agent.tspackages/opencode/src/tool/external-directory.tspackages/opencode/src/tool/read.tspackages/opencode/test/git/git.test.tspackages/opencode/test/session/messages-pagination.test.tspackages/opencode/test/skill/skill.test.tspackages/opencode/test/tool/external-directory.test.tspackages/ui/src/components/session-diff.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/ui/src/components/session-diff.ts
- packages/opencode/src/session/session.ts
- packages/core/test/npm.test.ts
- packages/opencode/test/session/messages-pagination.test.ts
- packages/opencode/src/project/vcs.ts
- packages/opencode/src/tool/read.ts
- packages/opencode/src/tool/agent.ts
- packages/opencode/src/git/index.ts
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/tool/external-directory.ts`:
- Around line 20-34: The Windows branch currently pre-normalizes the input via
AppFileSystem.normalizePath (assigned to normalized) which lexically collapses
'..' before calling realpathSync.native, allowing junction boundaries to be
escaped; change it to mirror the POSIX segment-walk: split the input path into
segments and iterate from the root, calling realpathSync.native on each
accumulated segment (handle ENOENT/ENOTDIR the same way you do now), process
'..' by popping the last resolved segment rather than letting win32.normalize
collapse them up-front, and only call AppFileSystem.normalizePath once the final
resolved realpath (the variable currently named resolved) is obtained (or when
returning normalized as fallback). Update logic around missing, current, and the
while-loop so resolution occurs per-segment and lexical normalization is
deferred until after symlink/junction resolution.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d85d2e9-f9b6-4855-9caa-862d071fdb2b
📒 Files selected for processing (5)
packages/opencode/src/tool/bash.tspackages/opencode/src/tool/external-directory.tspackages/opencode/test/skill/skill.test.tspackages/opencode/test/tool/bash.test.tspackages/opencode/test/tool/external-directory.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/skill/skill.test.ts
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/tool/external-directory.ts (1)
132-146: ⚡ Quick winMove bypass short-circuit before path resolution work.
resolveExternalPathForPermissionis invoked three times here (once forfull, once forscope.directory, optionally forscope.worktree), each performing per-segmentlstatSynccalls — including for stable, instance-level paths (ins.directory,ins.worktree) that don't depend ontarget. Whenoptions?.bypassistrue, all of that work is discarded. Hoisting the bypass check immediately afterfullavoids the sync fs traversals on the bypass hot path (e.g., callers that already trust the target).♻️ Proposed change
const ins = yield* InstanceState.context const full = process.platform === "win32" ? windowsPermissionPath(target, ins.directory) : path.isAbsolute(target) ? target : `${ins.directory.replace(/\/+$/, "")}/${target}` + if (options?.bypass) return full const resolved = resolveExternalPathForPermission(full, ins.directory) const scope = { ...ins, directory: resolveExternalPathForPermission(ins.directory, ins.directory), worktree: ins.worktree === "/" ? ins.worktree : resolveExternalPathForPermission(ins.worktree, ins.directory), } - if (options?.bypass) return full if (Instance.containsPath(resolved, scope)) return full🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/tool/external-directory.ts` around lines 132 - 146, The bypass short-circuit should be moved to run immediately after computing full to avoid unnecessary filesystem traversal; after computing full (using InstanceState.context and windowsPermissionPath/path.isAbsolute logic) check options?.bypass and return full if true, before calling resolveExternalPathForPermission or building scope, and therefore avoid calling resolveExternalPathForPermission for full, scope.directory, or scope.worktree and avoid invoking Instance.containsPath when bypass is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/tool/external-directory.ts`:
- Around line 132-146: The bypass short-circuit should be moved to run
immediately after computing full to avoid unnecessary filesystem traversal;
after computing full (using InstanceState.context and
windowsPermissionPath/path.isAbsolute logic) check options?.bypass and return
full if true, before calling resolveExternalPathForPermission or building scope,
and therefore avoid calling resolveExternalPathForPermission for full,
scope.directory, or scope.worktree and avoid invoking Instance.containsPath when
bypass is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b7215a1-6bab-448b-981a-86ffd025429f
📒 Files selected for processing (4)
packages/opencode/src/tool/bash.tspackages/opencode/src/tool/external-directory.tspackages/opencode/test/tool/bash.test.tspackages/opencode/test/tool/external-directory.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/tool/bash.ts
11 tasks
Astro-Han
added a commit
that referenced
this pull request
May 8, 2026
Remove the broken upstream `codesearch` tool from PawWork as the PR 3a follow-up for #477. Root cause: - Upstream opencode removed this tool in anomalyco/opencode#24992 because Exa MCP no longer exposes `get_code_context_exa`. - PawWork still had the stale tool registered after PR #504 intentionally split it out from the broader runtime safety slice. - Leaving it exposed meant the model could still call a tool that fails at runtime. Scope included: - Deleted `packages/opencode/src/tool/codesearch.ts` and `codesearch.txt`. - Removed registry initialization, builtin exposure, provider gating, default explore-agent permission, CLI rendering, and permission schema fields. - Removed stale SDK/OpenAPI permission schema entries. - Removed app settings labels and UI/i18n dedicated rendering for `codesearch`, so historical unknown parts fall back to generic rendering. - Removed the stale Effect migration checklist entry so future sync work does not try to migrate a deleted tool. - Added a registry regression test proving the built-in tool list no longer contains `codesearch`. Explicitly out of scope: - Web search behavior and Exa web-search quota/auth flows. - Desktop/app manual ports planned for the next #477 slice. - Effect foundation, effectCmd, HttpApi/listener migration, generated SDK regeneration beyond the removed field, dependencies, lockfile, CI/workflow, and packaging changes. Verification: - `bun install --frozen-lockfile` in the fresh worktree with no lockfile changes. - Red test confirmed the new registry assertion failed while production still exposed `codesearch`. - `bun --cwd packages/opencode test test/tool/registry.test.ts` -> 19 passed. - Residual scan showed only negative registry-test assertions mention `codesearch`. - `bun run --cwd packages/opencode typecheck` -> passed. - `bun run --cwd packages/ui typecheck` -> passed. - `bun run --cwd packages/app typecheck` -> passed. - `git diff --check` -> passed. - `packages/sdk/openapi.json` parsed successfully. - PR CI/status checks were green before merge, including ci, typecheck, unit-app, unit-desktop, unit-opencode, desktop-smoke, e2e-artifacts, CodeQL, dependency-review, commit-lint, PR title lint, action-semantic-pull-request, and CodeRabbit. - Review thread check returned no unresolved threads. Follow-up: - #477 remains open. The next sync work should move to the consolidated desktop/app manual-port slice unless a newer live upstream/PR scan shows an already-open PR covering it.
66 tasks
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
014dbd34c4f5612d9a037b3641a8244b213a8a30in five atomic commits.codesearchremoval split out because the local surface includes generated OpenAPI/SDK and broad app/UI references.Why
#477 is tracking upstream sync after opencode
17701628bd. PR #482 and #487 already landed the provider/model and auth/client slices. This PR implements the next consolidated runtime/session/tool slice while keeping desktop, HttpApi/listener, generated SDK/OpenAPI, dependency, and package-manifest churn out of scope.Included upstream anchors:
Split/defer decisions:
Related Issue
Refs #477
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
message-v2.tsandprovider/transform.ts, especially Anthropic signed reasoning, Bedrock empty text behavior, Bedrock PDF extraction, and surrogate sanitization.session-diff.ts.Risk Notes
Behavioral runtime change. Main risks are provider payload shape, permission scope, and VCS diff truncation. This PR intentionally avoids desktop, HttpApi/listener, generated SDK/OpenAPI, dependency, lockfile, package manifest, and release/workflow changes.
How To Verify
Screenshots or Recordings
Not applicable. No visible UI workflow change; the only UI-package change is defensive malformed persisted patch parsing covered by
session-diff.test.ts.Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Bug Fixes
Improvements