fix(snapshot): harden track staging and tree writes#76
Open
riatzukiza wants to merge 4 commits intodevfrom
Open
fix(snapshot): harden track staging and tree writes#76riatzukiza wants to merge 4 commits intodevfrom
riatzukiza wants to merge 4 commits intodevfrom
Conversation
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughReworks snapshot git orchestration: adds retry/backoff command runner, conflict detection, output normalization, staged add/write split with validation, excludes management, and index.lock-aware behavior; updates track/patch/diff to short-circuit on staging failures and adds tests for lock contention scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Snapshot
participant FS as Filesystem
participant Git as GitCLI
participant Logger
Snapshot->>FS: check .git/index.lock
alt lock exists
FS-->>Snapshot: locked
Snapshot->>Logger: log lock, return undefined
else no lock
Snapshot->>Git: command(add) with retry/backoff
Git-->>Snapshot: exitCode/stdout/stderr
alt conflict/retryable
Snapshot->>Git: retry according to backoff
end
Snapshot->>Git: command(write-tree)
Git-->>Snapshot: treeHash
alt valid hash
Snapshot->>Logger: log tracked hash
Snapshot-->>Logger: update excludes
Snapshot-->>Snapshot: return snapshot info
else invalid / failed
Snapshot->>Logger: log failure details
Snapshot-->>Snapshot: return undefined
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/opencode/src/snapshot/index.ts (1)
405-432:let lastandfor...ofloop violate coding guidelines;await result.text()is a misleading no-op await.Three issues in
command():
let+ mutation in loop — guidelines requireconst; useletonly with ternaries/early returns. The loop body mutateslaston every iteration.for...ofinstead of functional array method — guidelines requireflatMap/filter/map/reduceover loops.await result.text()—textinGitResultis() => string(synchronous), soawait result.text()is anawaiton a plain string. It works, but it implies the call is async and is misleading.As per coding guidelines: "Prefer functional array methods (flatMap, filter, map) over for loops" and "Prefer
constoverlet. Use ternaries or early returns instead of reassignment".♻️ Proposed refactor
- let last = { - exitCode: 1, - stderr: "", - stdout: "", - text: "", - } - for (const ms of retry) { - if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) - const result = await runGit(input.args, { cwd: input.cwd }) - const stdout = (await result.text()).trim() - const stderr = output(result.stderr) - last = { - exitCode: result.exitCode, - stderr, - stdout, - text: stdout, - } - if (last.exitCode === 0) return last - if (!input.retryLock) return last - if (!conflict(`${stderr}\n${stdout}`)) return last - } - return last + const run = async (ms: number, prev?: { exitCode: number; stderr: string; stdout: string; text: string }): Promise<{ exitCode: number; stderr: string; stdout: string; text: string }> => { + if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) + const result = await runGit(input.args, { cwd: input.cwd }) + const stdout = result.text().trim() + const stderr = output(result.stderr) + const last = { exitCode: result.exitCode, stderr, stdout, text: stdout } + if (last.exitCode === 0) return last + if (!input.retryLock) return last + if (!conflict(`${stderr}\n${stdout}`)) return last + return last + } + // Sequential retries: try each delay, return as soon as one succeeds or a non-retryable error occurs + for (const ms of retry) { + const r = await run(ms) + if (r.exitCode === 0 || !input.retryLock || !conflict(`${r.stderr}\n${r.stdout}`)) return r + } + return await run(0)Alternatively, keep the
for...ofbut replacelet lastwith a pure innerconstand directreturn:async function command(input: { args: string[]; cwd: string; retryLock?: boolean }) { + const attempt = async () => { + const result = await runGit(input.args, { cwd: input.cwd }) + const stdout = result.text().trim() + const stderr = output(result.stderr) + return { exitCode: result.exitCode, stderr, stdout, text: stdout } + } + let last = { exitCode: 1, stderr: "", stdout: "", text: "" } for (const ms of retry) { if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) - const result = await runGit(input.args, { cwd: input.cwd }) - const stdout = (await result.text()).trim() - const stderr = output(result.stderr) - last = { exitCode: result.exitCode, stderr, stdout, text: stdout } + last = await attempt() if (last.exitCode === 0) return last if (!input.retryLock) return last if (!conflict(`${last.stderr}\n${last.stdout}`)) return last } return last }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/snapshot/index.ts` around lines 405 - 432, In command(), remove the mutable outer let last and the misleading await on result.text(): replace the for...of loop over retry with an async functional iteration (e.g., Promise-based async reduce over retry) that for each ms awaits the delay (if ms>0), calls runGit(args, { cwd }), reads const stdout = result.text() (no await) and const stderr = output(result.stderr), builds a const current = { exitCode: result.exitCode, stderr, stdout, text: stdout }, and returns current immediately when current.exitCode === 0 or when !input.retryLock or !conflict(`${stderr}\n${stdout}`); at the end return the last resolved current. This removes mutation of last, avoids await on a sync text() call, and replaces the for...of with a functional reducer while keeping existing helpers (runGit, conflict, output, retry).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 397-403: The conflict() helper is using the overly broad substring
"unable to create" which can match unrelated git errors; update conflict(input:
string) to either remove that check or tighten it to only match creation
failures for the index lock (e.g. use a regex like /unable to
create.*index\.lock/i or check for "index.lock" in combination with "unable to
create") so only index-lock contention triggers retries; modify the conflict
function (the condition inside conflict) accordingly.
In `@packages/opencode/test/snapshot/snapshot.test.ts`:
- Around line 704-727: The test "track retries snapshot index lock contention"
relies on a fragile 40ms setTimeout and risks lock leakage; update the removal
delay to a safer value (e.g., 60ms or greater) so the lock is removed well
inside the expected retry window, and make the lock cleanup exception-safe by
wrapping the test body that calls Snapshot.track() and the assertions in a
try/finally where the finally always calls fs.unlink(lock). Reference the test
function name, the lock variable, the setTimeout that creates the delayed
unlink, Snapshot.track() calls, and the final fs.unlink(lock).catch(() => {}) to
locate and change the code.
- Around line 684-702: The test "track returns undefined when snapshot index
lock blocks staging" currently writes an index.lock under
Global.Path.data/snapshot/<id> and removes it with fs.unlink only at the end,
which leaks the lock if an assertion throws; update the body passed to
Instance.provide (the async () => { ... }) to wrap the portion that creates the
lock and calls Snapshot.track() in a try/finally so the cleanup call to
fs.unlink(lock) always runs (use the existing git and lock variables and keep
Snapshot.track() checks inside the try, then unlink in the finally).
---
Nitpick comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 405-432: In command(), remove the mutable outer let last and the
misleading await on result.text(): replace the for...of loop over retry with an
async functional iteration (e.g., Promise-based async reduce over retry) that
for each ms awaits the delay (if ms>0), calls runGit(args, { cwd }), reads const
stdout = result.text() (no await) and const stderr = output(result.stderr),
builds a const current = { exitCode: result.exitCode, stderr, stdout, text:
stdout }, and returns current immediately when current.exitCode === 0 or when
!input.retryLock or !conflict(`${stderr}\n${stdout}`); at the end return the
last resolved current. This removes mutation of last, avoids await on a sync
text() call, and replaces the for...of with a functional reducer while keeping
existing helpers (runGit, conflict, output, retry).
📝 WalkthroughWalkthroughAdded Git operation retry logic with backoff intervals and conflict detection to the Snapshot class. Refactored staging and tree-write operations through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/opencode/test/snapshot/snapshot.test.ts (2)
692-700:⚠️ Potential issue | 🟡 MinorMake lock cleanup exception-safe in the blocking-lock test.
If an assertion throws before the final unlink,
index.lockcan leak and contaminate later tests.🔧 Proposed fix
const git = path.join(Global.Path.data, "snapshot", Instance.project.id) const lock = path.join(git, "index.lock") await Bun.write(lock, "") - const next = await Snapshot.track() - expect(next).toBeUndefined() - - await fs.unlink(lock) + try { + const next = await Snapshot.track() + expect(next).toBeUndefined() + } finally { + await fs.unlink(lock).catch(() => {}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/snapshot/snapshot.test.ts` around lines 692 - 700, The test creates an index.lock file and currently unlinks it only after assertions, risking leakage if an assertion throws; wrap the call to Snapshot.track() and the expect assertion in a try/finally block and move the cleanup into the finally to always remove the lock, using fs.unlink (or fs.remove) and optionally checking for existence before unlinking; reference the existing local variables git and lock and the Snapshot.track() call to locate where to add the try/finally.
716-725:⚠️ Potential issue | 🟡 MinorRetry test is timing-sensitive, and cleanup should be in
finally.The 40ms delayed unlock can be flaky on busy CI runners, and cleanup is still linear-flow. Use a safer window and
try/finally.🔧 Proposed fix
setTimeout(() => { void fs.unlink(lock).catch(() => {}) - }, 40) + }, 60) await Filesystem.write(`${tmp.path}/retry.txt`, "retry") - const next = await Snapshot.track() - expect(next).toBeTruthy() - - await fs.unlink(lock).catch(() => {}) + try { + const next = await Snapshot.track() + expect(next).toBeTruthy() + } finally { + await fs.unlink(lock).catch(() => {}) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/snapshot/snapshot.test.ts` around lines 716 - 725, The retry test uses a fragile 40ms delayed fs.unlink and linear cleanup; wrap the test body that schedules setTimeout(() => void fs.unlink(lock).catch(() => {}), 40) and the subsequent await Filesystem.write(...) / await Snapshot.track() calls in a try/finally so cleanup always runs, increase the delay to a safer window (e.g. 100ms) or use a Promise that resolves after the lock is removed, and perform both the immediate await fs.unlink(lock).catch(() => {}) and ensure the scheduled unlink is either awaited/cancelled in the finally block; reference the setTimeout callback, the lock variable, fs.unlink calls, Filesystem.write, and Snapshot.track when making the changes.packages/opencode/src/snapshot/index.ts (1)
397-402:⚠️ Potential issue | 🟡 MinorRetry trigger is still too broad for non-lock failures.
The
"unable to create"substring can match unrelated Git errors and cause unnecessary retries. Tighten this to lock-file-specific wording.🔧 Proposed fix
function conflict(input: string) { const text = input.toLowerCase() if (text.includes("index.lock")) return true if (text.includes("another git process seems to be running")) return true - if (text.includes("unable to create")) return true + if (text.includes("unable to create lock file")) return true return false }Are Git errors containing "unable to create" always lock-contention related, or do they also include unrelated failures (e.g., permission/path/file creation errors)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/snapshot/index.ts` around lines 397 - 402, The retry trigger in the conflict function is too broad because checking for "unable to create" matches unrelated Git errors; update conflict to only treat lock-related creation failures as retries by matching a more specific substring such as "unable to create index.lock" or "unable to create .git/index.lock" (or using a regex to detect "unable to create .*index.lock"), so only lock-file creation errors in the conflict() function trigger a retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Line 17: The treehash regex currently only allows 40-char SHA-1 IDs which
rejects SHA-256 object-format repos; update the validation to accept both 40-
and 64-character hex hashes by changing the treehash pattern to match either
length and ensure the write() function uses that updated treehash when
validating git write-tree output (reference the treehash constant and the
write() function in snapshot/index.ts).
---
Duplicate comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 397-402: The retry trigger in the conflict function is too broad
because checking for "unable to create" matches unrelated Git errors; update
conflict to only treat lock-related creation failures as retries by matching a
more specific substring such as "unable to create index.lock" or "unable to
create .git/index.lock" (or using a regex to detect "unable to create
.*index.lock"), so only lock-file creation errors in the conflict() function
trigger a retry.
In `@packages/opencode/test/snapshot/snapshot.test.ts`:
- Around line 692-700: The test creates an index.lock file and currently unlinks
it only after assertions, risking leakage if an assertion throws; wrap the call
to Snapshot.track() and the expect assertion in a try/finally block and move the
cleanup into the finally to always remove the lock, using fs.unlink (or
fs.remove) and optionally checking for existence before unlinking; reference the
existing local variables git and lock and the Snapshot.track() call to locate
where to add the try/finally.
- Around line 716-725: The retry test uses a fragile 40ms delayed fs.unlink and
linear cleanup; wrap the test body that schedules setTimeout(() => void
fs.unlink(lock).catch(() => {}), 40) and the subsequent await
Filesystem.write(...) / await Snapshot.track() calls in a try/finally so cleanup
always runs, increase the delay to a safer window (e.g. 100ms) or use a Promise
that resolves after the lock is removed, and perform both the immediate await
fs.unlink(lock).catch(() => {}) and ensure the scheduled unlink is either
awaited/cancelled in the finally block; reference the setTimeout callback, the
lock variable, fs.unlink calls, Filesystem.write, and Snapshot.track when making
the changes.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/opencode/src/snapshot/index.ts (1)
17-17:⚠️ Potential issue | 🟡 Minor
treehashstill rejects SHA-256 object-format repositories.
/^[0-9a-f]{40}$/only matches 40-char SHA-1 hashes.git write-treein a repo initialized with--object-format=sha256returns 64-char hashes, causingwrite()to returnundefinedand silently skip every snapshot write in those repos.🔧 Proposed fix
- const treehash = /^[0-9a-f]{40}$/ + const treehash = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/snapshot/index.ts` at line 17, The current treehash regex only matches 40-char SHA-1 hashes so repos using SHA-256 object-format produce 64-char hashes and get skipped; update the treehash pattern (the treehash constant in packages/opencode/src/snapshot/index.ts) to accept either 40- or 64-character lowercase hex (e.g. a regex matching ^(?:[0-9a-f]{40}|[0-9a-f]{64})$) and run or update any tests around write() that rely on treehash to ensure 64-char hashes are accepted.
🧹 Nitpick comments (1)
packages/opencode/src/snapshot/index.ts (1)
410-426:let lastand redundantawaiton synchronoustext().Two style/guideline issues in
command():
let lastis reassigned on every loop iteration, violating the "Preferconstoverlet" guideline. The accumulator pattern can be replaced with afor…ofthat returns directly, falling through to a final call:♻️ Proposed refactor
- let last = { - exitCode: 1, - stderr: "", - stdout: "", - text: "", - } - for (const ms of retry) { - if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) - const result = await runGit(input.args, { cwd: input.cwd }) - const stdout = (await result.text()).trim() - const stderr = output(result.stderr) - last = { - exitCode: result.exitCode, - stderr, - stdout, - text: stdout, - } - if (last.exitCode === 0) return last - if (!input.retryLock) return last - if (!conflict(`${stderr}\n${stdout}`)) return last - } - return last + const intervals = input.retryLock ? retry : [retry[0]] + for (const ms of intervals) { + if (ms > 0) await new Promise<void>((resolve) => setTimeout(resolve, ms)) + const result = await runGit(input.args, { cwd: input.cwd }) + const stdout = result.text().trim() + const stderr = output(result.stderr) + const last = { exitCode: result.exitCode, stderr, stdout, text: stdout } + if (last.exitCode === 0 || !input.retryLock || !conflict(`${stderr}\n${stdout}`)) return last + } + // exhausted retries — run one final time + const result = await runGit(input.args, { cwd: input.cwd }) + const stdout = result.text().trim() + const stderr = output(result.stderr) + return { exitCode: result.exitCode, stderr, stdout, text: stdout }
await result.text()at line 419 —textis typed as() => string(synchronous) inutil/git.ts; theawaitis harmless but misleading. Useresult.text()directly.As per coding guidelines: "Prefer
constoverlet. Use ternaries or early returns instead of reassignment."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/snapshot/index.ts` around lines 410 - 426, The loop should avoid reassigning let last and drop the redundant await on the synchronous result.text(); refactor the retry loop to use for...of with entries, call const result = await runGit(input.args, { cwd: input.cwd }), get stdout via const stdout = result.text() (no await), compute stderr via const stderr = output(result.stderr), build a const current = { exitCode: result.exitCode, stderr, stdout, text: stdout }, and return current immediately when result.exitCode === 0 or when i === retry.length - 1 so no external mutable last variable is needed; reference runGit, result.text(), output(), and retry in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 397-403: In function conflict(), the third conditional (checking
both "unable to create" && "index.lock") is unreachable because an earlier check
already returns true for any text containing "index.lock"; remove that redundant
branch and instead decide whether you want a separate check for "unable to
create" alone — either delete the third if entirely or change it to if
(text.includes("unable to create")) return true to capture messages that don’t
mention "index.lock" while keeping the existing index.lock check in conflict().
---
Duplicate comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Line 17: The current treehash regex only matches 40-char SHA-1 hashes so repos
using SHA-256 object-format produce 64-char hashes and get skipped; update the
treehash pattern (the treehash constant in
packages/opencode/src/snapshot/index.ts) to accept either 40- or 64-character
lowercase hex (e.g. a regex matching ^(?:[0-9a-f]{40}|[0-9a-f]{64})$) and run or
update any tests around write() that rely on treehash to ensure 64-char hashes
are accepted.
---
Nitpick comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 410-426: The loop should avoid reassigning let last and drop the
redundant await on the synchronous result.text(); refactor the retry loop to use
for...of with entries, call const result = await runGit(input.args, { cwd:
input.cwd }), get stdout via const stdout = result.text() (no await), compute
stderr via const stderr = output(result.stderr), build a const current = {
exitCode: result.exitCode, stderr, stdout, text: stdout }, and return current
immediately when result.exitCode === 0 or when i === retry.length - 1 so no
external mutable last variable is needed; reference runGit, result.text(),
output(), and retry in the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/opencode/src/snapshot/index.ts (1)
409-430:let+for…ofloop violatesconstand functional-methods guidelines;await result.text()treats a sync function as async.Three issues:
let lastwith reassignment — violates theconstguideline. Use early returns instead of accumulating state.for (const ms of retry)imperative loop — violates the functional array methods guideline. As per coding guidelines, preferflatMap/filter/map/reduceoverforloops.await result.text()—GitResult.textis typed() => string(synchronous in both branches ofutil/git.ts; the non-ACP branch wraps an already-resolvedShellOutputaccessor). Bun'sShellPromise.text()is typedPromise<string>on the unresolved promise, but once the shell process is awaited and the result object is in hand (as inutil/git.ts), accessors liketext()andarrayBuffer()are synchronous. Theawaitis a harmless no-op but is semantically incorrect and misleading to readers.Additionally,
stdoutandtexton the returned object are always the same value — thetextalias is redundant.♻️ Proposed refactor — eliminates let/for-of/await issues and deduplicates the field
- async function command(input: { - args: string[] - cwd: string - retryLock?: boolean - }) { - let last = { - exitCode: 1, - stderr: "", - stdout: "", - text: "", - } - for (const ms of retry) { - if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) - const result = await runGit(input.args, { cwd: input.cwd }) - const stdout = (await result.text()).trim() - const stderr = output(result.stderr) - last = { - exitCode: result.exitCode, - stderr, - stdout, - text: stdout, - } - if (last.exitCode === 0) return last - if (!input.retryLock) return last - if (!conflict(`${stderr}\n${stdout}`)) return last - } - return last - } + async function command(input: { args: string[]; cwd: string; retryLock?: boolean }) { + const run = async (ms: number) => { + if (ms > 0) await new Promise<void>((r) => setTimeout(r, ms)) + const result = await runGit(input.args, { cwd: input.cwd }) + const stdout = result.text().trim() + return { exitCode: result.exitCode, stderr: output(result.stderr), stdout } + } + return retry.reduce( + async (prev, ms) => { + const last = await prev + if (last.exitCode === 0) return last + if (!input.retryLock) return last + if (!conflict(`${last.stderr}\n${last.stdout}`)) return last + return run(ms) + }, + run(0), + ) + }Note: callers of
command()that useresult.text(inwrite()) will need to be updated toresult.stdoutafter removing the alias.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/snapshot/index.ts` around lines 409 - 430, The loop accumulates mutable state in `last`, uses an imperative `for (const ms of retry)` and wrongly awaits the synchronous `result.text()`, and returns a redundant `text` alias; refactor `command()` (the function containing `retry`, `runGit`, and `conflict`) to use functional iteration (e.g., `retry.some`/`find`/`reduce`) or early returns on success/error instead of reassigning `last`, call `result.text()` synchronously (remove `await`), eliminate the `text` field and return only `stdout`/`stderr`/`exitCode`, and update any callers (e.g., `write()`) that reference `result.text` to use `result.stdout`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 392-395: The output() helper currently accepts Buffer |
ReadableStream<Uint8Array> and returns "" for streams, which hides errors when
conflict() expects stderr as text; narrow the signature to accept only Buffer
(e.g., change function output(input: Buffer)) and remove the unreachable
ReadableStream branch, then update uses (notably conflict()) to pass the Buffer
directly so stderr is always converted via toString() and no stream path
silently drops content; reference the output() function and the conflict()
caller when making this change.
- Around line 392-395: The output function currently accepts Buffer |
ReadableStream<Uint8Array> but only handles Buffer and returns "" for streams;
change the signature of output to accept only Buffer (output(input: Buffer):
string), remove the ReadableStream branch and the unreachable empty-string
return, and simply return input.toString(); update any callers if necessary (the
existing caller passes result.stderr which is always a Buffer) so types align.
- Around line 404-431: The command function should be rewritten to avoid mutable
let and imperative loops, drop the unnecessary await on result.text(), and stop
returning duplicate stdout/text fields: use a functional array method (e.g.,
Array.prototype.find or reduce over retry) to try each delay value, call const
result = await runGit(...) and const stdout = result.text() (no await), compute
stderr = output(result.stderr), then immediately return a const object {
exitCode: result.exitCode, stderr, stdout } as soon as exitCode === 0 or when
conflict detection fails; ensure callers that expect result.text() (write()) use
result.stdout (or vice versa) by keeping a single text field name consistent
with add() which uses exitCode, stderr, stdout (so prefer returning stdout only
and remove the duplicate text property).
---
Nitpick comments:
In `@packages/opencode/src/snapshot/index.ts`:
- Around line 409-430: The loop accumulates mutable state in `last`, uses an
imperative `for (const ms of retry)` and wrongly awaits the synchronous
`result.text()`, and returns a redundant `text` alias; refactor `command()` (the
function containing `retry`, `runGit`, and `conflict`) to use functional
iteration (e.g., `retry.some`/`find`/`reduce`) or early returns on success/error
instead of reassigning `last`, call `result.text()` synchronously (remove
`await`), eliminate the `text` field and return only
`stdout`/`stderr`/`exitCode`, and update any callers (e.g., `write()`) that
reference `result.text` to use `result.stdout`.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/opencode/src/snapshot/index.tspackages/opencode/test/snapshot/snapshot.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/snapshot/snapshot.test.ts
Comment on lines
+392
to
+395
| function output(input: Buffer | ReadableStream<Uint8Array>) { | ||
| if (Buffer.isBuffer(input)) return input.toString() | ||
| return "" | ||
| } |
There was a problem hiding this comment.
output() silently discards ReadableStream stderr — narrow the type to match actual callers.
The ReadableStream<Uint8Array> branch is unreachable: util/git.ts returns Buffer for stderr in both the ACP and non-ACP paths. The overly broad union type creates a false safety guarantee — if stderr were ever actually a stream, conflict() would receive "" and suppress the lock-contention retry silently.
♻️ Proposed fix
- function output(input: Buffer | ReadableStream<Uint8Array>) {
- if (Buffer.isBuffer(input)) return input.toString()
- return ""
- }
+ function output(input: Buffer) {
+ return input.toString()
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/snapshot/index.ts` around lines 392 - 395, The output()
helper currently accepts Buffer | ReadableStream<Uint8Array> and returns "" for
streams, which hides errors when conflict() expects stderr as text; narrow the
signature to accept only Buffer (e.g., change function output(input: Buffer))
and remove the unreachable ReadableStream branch, then update uses (notably
conflict()) to pass the Buffer directly so stderr is always converted via
toString() and no stream path silently drops content; reference the output()
function and the conflict() caller when making this change.
🧩 Analysis chain
🏁 Script executed:
# Read the output() function to confirm its signature and implementation
sed -n '392,395p' packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode
Length of output: 200
🏁 Script executed:
# Find all calls to output() in the snapshot/index.ts file
rg -n "output\(" packages/opencode/src/snapshot/index.ts | head -20Repository: riatzukiza/opencode
Length of output: 177
🏁 Script executed:
# Check the util/git.ts file to understand what stderr type it returns
cat -n packages/opencode/src/util/git.ts | sed -n '1,70p'Repository: riatzukiza/opencode
Length of output: 2507
Narrow output() parameter type to Buffer — the ReadableStream branch is dead code.
The function accepts Buffer | ReadableStream<Uint8Array> but only handles the Buffer case, silently returning "" for streams. Both branches in util/git.ts always return Buffer for stderr: the ACP branch explicitly creates buffers via Buffer.from(), and the Bun shell branch buffers output by default without stream configuration. The single caller at line 419 passes result.stderr, which is always Buffer. The ReadableStream branch is unreachable and should be removed.
♻️ Proposed fix
- function output(input: Buffer | ReadableStream<Uint8Array>) {
- if (Buffer.isBuffer(input)) return input.toString()
- return ""
- }
+ function output(input: Buffer) {
+ return input.toString()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function output(input: Buffer | ReadableStream<Uint8Array>) { | |
| if (Buffer.isBuffer(input)) return input.toString() | |
| return "" | |
| } | |
| function output(input: Buffer) { | |
| return input.toString() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/snapshot/index.ts` around lines 392 - 395, The output
function currently accepts Buffer | ReadableStream<Uint8Array> but only handles
Buffer and returns "" for streams; change the signature of output to accept only
Buffer (output(input: Buffer): string), remove the ReadableStream branch and the
unreachable empty-string return, and simply return input.toString(); update any
callers if necessary (the existing caller passes result.stderr which is always a
Buffer) so types align.
Comment on lines
+404
to
431
| async function command(input: { | ||
| args: string[] | ||
| cwd: string | ||
| retryLock?: boolean | ||
| }) { | ||
| let last = { | ||
| exitCode: 1, | ||
| stderr: "", | ||
| stdout: "", | ||
| text: "", | ||
| } | ||
| for (const ms of retry) { | ||
| if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) | ||
| const result = await runGit(input.args, { cwd: input.cwd }) | ||
| const stdout = (await result.text()).trim() | ||
| const stderr = output(result.stderr) | ||
| last = { | ||
| exitCode: result.exitCode, | ||
| stderr, | ||
| stdout, | ||
| text: stdout, | ||
| } | ||
| if (last.exitCode === 0) return last | ||
| if (!input.retryLock) return last | ||
| if (!conflict(`${stderr}\n${stdout}`)) return last | ||
| } | ||
| return last | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
fd -t f "git\.ts" | head -20Repository: riatzukiza/opencode
Length of output: 96
🏁 Script executed:
find . -path "*/util/git.ts" -o -path "*/git.ts" | grep -E "(util|src)" | head -10Repository: riatzukiza/opencode
Length of output: 98
🏁 Script executed:
git ls-files | grep -E "git\.ts" | head -20Repository: riatzukiza/opencode
Length of output: 96
🏁 Script executed:
cat -n packages/opencode/src/util/git.tsRepository: riatzukiza/opencode
Length of output: 2507
🏁 Script executed:
cat -n packages/opencode/src/snapshot/index.ts | sed -n '404,431p'Repository: riatzukiza/opencode
Length of output: 993
🏁 Script executed:
# Check the full snapshot function context and callers
cat -n packages/opencode/src/snapshot/index.ts | sed -n '390,450p'Repository: riatzukiza/opencode
Length of output: 2133
🏁 Script executed:
# Find where the command() function is called to verify if both stdout and text fields are used
rg -A 3 -B 1 "command\(" packages/opencode/src/snapshot/index.ts | grep -E "(add|write|stdout|text)" | head -40Repository: riatzukiza/opencode
Length of output: 180
🏁 Script executed:
# Search for calls to the command function to see usage patterns
rg "await command\(" packages/opencode/src/snapshot/index.ts -A 5 -B 2Repository: riatzukiza/opencode
Length of output: 515
🏁 Script executed:
# Find the full add() and write() functions to see which result fields are used
rg -A 20 "async function add\(git: string\)" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode
Length of output: 533
🏁 Script executed:
# Find the full write() function
rg -A 15 "async function write\(git: string\)" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode
Length of output: 518
🏁 Script executed:
# Check if result.text is used anywhere after calling command()
rg "result\.text" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode
Length of output: 265
🏁 Script executed:
# Let's also verify the retry variable to understand the for loop better
rg -B 30 "async function command" packages/opencode/src/snapshot/index.ts | grep -E "retry|function snapshot"Repository: riatzukiza/opencode
Length of output: 45
🏁 Script executed:
# Find where retry is defined
rg "retry" packages/opencode/src/snapshot/index.ts -B 5 -A 2 | head -60Repository: riatzukiza/opencode
Length of output: 1316
🏁 Script executed:
# Check the full context around the command function to see retry definition and usage
cat -n packages/opencode/src/snapshot/index.ts | sed -n '350,410p'Repository: riatzukiza/opencode
Length of output: 1972
🏁 Script executed:
# Search for where result.text is actually used in the file
rg -n "result\.text" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode
Length of output: 289
let last with reassignment violates const and functional-methods guidelines; await result.text() is semantically incorrect.
Three issues in this function:
let lastwith reassignment — the guidelines requireconst; use early returns instead of accumulation.for (const ms of retry)imperative loop — the guidelines require functional array methods (findorreduce).await result.text()—GitResult.text()is a synchronous function in both the ACP and Bun shell branches ofutil/git.ts;awaiton a synchronous return value is semantically incorrect even though it is harmless.
Additionally, the returned object includes both stdout and text fields with identical values (line 424: text: stdout). The callers in add() use result.exitCode, result.stderr, and result.stdout, while write() needs only result.text; keeping both is unnecessary.
♻️ Proposed refactor addressing all three points
- async function command(input: {
- args: string[]
- cwd: string
- retryLock?: boolean
- }) {
- let last = {
- exitCode: 1,
- stderr: "",
- stdout: "",
- text: "",
- }
- for (const ms of retry) {
- if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms))
- const result = await runGit(input.args, { cwd: input.cwd })
- const stdout = (await result.text()).trim()
- const stderr = output(result.stderr)
- last = {
- exitCode: result.exitCode,
- stderr,
- stdout,
- text: stdout,
- }
- if (last.exitCode === 0) return last
- if (!input.retryLock) return last
- if (!conflict(`${stderr}\n${stdout}`)) return last
- }
- return last
- }
+ async function command(input: { args: string[]; cwd: string; retryLock?: boolean }) {
+ const run = async (ms: number) => {
+ if (ms > 0) await new Promise<void>((resolve) => setTimeout(resolve, ms))
+ const result = await runGit(input.args, { cwd: input.cwd })
+ const stdout = result.text().trim()
+ const stderr = output(result.stderr)
+ return { exitCode: result.exitCode, stderr, stdout }
+ }
+ return retry.reduce(
+ async (prev, ms) => {
+ const last = await prev
+ if (last.exitCode === 0) return last
+ if (!input.retryLock) return last
+ if (!conflict(`${last.stderr}\n${last.stdout}`)) return last
+ return run(ms)
+ },
+ run(0),
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/snapshot/index.ts` around lines 404 - 431, The command
function should be rewritten to avoid mutable let and imperative loops, drop the
unnecessary await on result.text(), and stop returning duplicate stdout/text
fields: use a functional array method (e.g., Array.prototype.find or reduce over
retry) to try each delay value, call const result = await runGit(...) and const
stdout = result.text() (no await), compute stderr = output(result.stderr), then
immediately return a const object { exitCode: result.exitCode, stderr, stdout }
as soon as exitCode === 0 or when conflict detection fails; ensure callers that
expect result.text() (write()) use result.stdout (or vice versa) by keeping a
single text field name consistent with add() which uses exitCode, stderr, stdout
(so prefer returning stdout only and remove the duplicate text property).
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.
Issue for this PR
Closes #74
Type of change
What does this PR do?
This phase hardens snapshot staging and tree writes when git index contention occurs. It retries lock-contention failures for
git addandwrite-tree, validates tree hashes, and fails closed sopatch/diffreturn safe empty results instead of propagating invalid snapshot state.How did you verify your code works?
bun test test/snapshot/snapshot.test.tsfrompackages/opencode.Screenshots / recordings
N/A (non-UI change).
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests