fix: auto-re-read stale files in agent loop instead of failing (#450)

[anandgupta42]

Summary

• When edit or write tools fail with "modified since it was last read", the agent loop now auto-re-reads the file and includes its current content in the error response
• The model sees the fresh file state and can adjust its next edit accordingly, preventing infinite retry loops
• The original safety check in FileTime.assert() is preserved — tools still throw on stale files
• Recovery happens at the agent level in processor.ts, not at the tool level

Why agent-level, not tool-level?

We considered two approaches:

1. Tool-level auto-refresh (what Gemini CLI does): silently refresh timestamp and proceed
2. Agent-level re-read (this PR): keep the safety check, but recover by feeding fresh content to the model

Approach 2 is better because the model context gets updated with current file content (e.g. after a SQL linter reformats select to SELECT), preventing secondary oldString match failures.

How it works

In processor.ts tool-error handler:

1. Detects stale file errors via regex
2. Re-reads the file from disk and updates FileTime
3. Appends fresh file content to error message
4. Model receives error + current content and can retry correctly

Context

984 stale-file errors in 7 days, bursts of up to 295 consecutive failures. Root cause: formatters/linters modify files between read and edit.

Closes #450

Summary by CodeRabbit

• Bug Fixes
  • Enhanced error detection for files modified during processing operations.
  • Implemented automatic recovery mechanism that intelligently attempts to refresh file content when changes are detected, with optimized handling for large files.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@anandgupta42 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3bcb8a0e-baa4-4d47-9517-349f2c5e044d

📥 Commits

Reviewing files that changed from the base of the PR and between b567494 and f2d5e8d.

📒 Files selected for processing (2)

• packages/opencode/src/session/processor.ts
• packages/opencode/test/file/stale-file-recovery.test.ts

📝 Walkthrough

Walkthrough

A new StaleFileError exception type is introduced alongside error handling logic that automatically attempts to re-read modified files when they have been changed since last access. The recovery mechanism conditionally reads and logs fresh file content or gracefully falls back to the original error.

Changes

Cohort / File(s)	Summary
Error Type Definition packages/opencode/src/file/time.ts	Introduced StaleFileError class extending Error with a filePath property. Updated FileTime.assert() to throw StaleFileError instead of generic Error when file mtime exceeds session's last-read timestamp by more than 50ms.
Stale File Recovery packages/opencode/src/session/processor.ts	Enhanced "tool-error" handling to detect StaleFileError instances and attempt automatic recovery. Constructs an error string with conditional messages: file missing note, size-limit warning (>50KB), or fresh file content retrieval and logging. Falls back to original error if recovery fails.

Sequence Diagram

sequenceDiagram
    participant Tool as Tool Execution
    participant Processor as Processor
    participant FileTime as FileTime
    participant Filesystem as Filesystem
    participant Logger as Logger

    Tool->>Processor: Tool result (StaleFileError)
    Processor->>Processor: Detect StaleFileError type
    
    alt File Missing
        Processor->>Processor: Append missing file note
    else File Size > 50KB
        Processor->>FileTime: Read to update metadata
        FileTime-->>Processor: Metadata updated
        Processor->>Processor: Append size warning
    else File Size ≤ 50KB
        Processor->>Filesystem: Read fresh file content
        Filesystem-->>Processor: File content retrieved
        Processor->>FileTime: Read to update metadata
        FileTime-->>Processor: Metadata updated
        Processor->>Logger: Log auto re-read event
        Logger-->>Processor: Event logged
        Processor->>Processor: Append content in fenced block
    else Recovery Fails
        Processor->>Logger: Log warning
        Logger-->>Processor: Warning logged
        Processor->>Processor: Use original error string
    end
    
    Processor->>Processor: Persist error with computed string

Loading

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 When files grow stale, we'd loop all day,
Now auto-recovery shows the way!
Fresh reads bloom where errors once fell,
No more retry loops from coding hell, ✨📖
And rabbits rejoice with "all is well!"

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The PR description covers all required sections: it explains what changed and why (Summary), but does not include a Test Plan or Checklist as specified in the template.	Add a Test Plan section describing how the changes were tested and a Checklist confirming tests were added/updated and documentation was reviewed.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: implementing automatic file re-reading for stale file errors at the agent loop level.
Linked Issues check	✅ Passed	The implementation successfully addresses the core objective from #450: detecting stale file errors, re-reading files, and providing fresh content to the model to enable graceful recovery and prevent infinite retry loops.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to resolving #450: introducing StaleFileError class for type-safe error detection and implementing agent-level auto-recovery in the processor; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/ai-450-edit-stale-file-retry

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/opencode/test/tool/edit.test.ts (1)

241-259: ⚠️ Potential issue | 🟠 Major

Strengthen this test to prove the stale branch is actually exercised.

Right now the test validates success after external modification, but it doesn’t assert that the file was truly stale before edit.execute. Add a strict FileTime.assert failure check first to lock in the intended regression coverage.

✅ Suggested assertion to make the precondition explicit

           // Simulate external modification
           await fs.writeFile(filepath, "modified externally", "utf-8")
+          await expect(FileTime.assert(ctx.sessionID, filepath)).rejects.toThrow(
+            "modified since it was last read",
+          )

           // Edit should succeed — auto-refreshes the stale read timestamp
           const edit = await EditTool.init()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/tool/edit.test.ts` around lines 241 - 259, The test
needs an explicit precondition asserting the file is stale before calling
EditTool.execute: after simulating the external modification (the fs.writeFile
call) and before creating EditTool via EditTool.init()/calling edit.execute,
invoke the stale-check helper (FileTime.assert or whatever function validates
read timestamps) and assert it throws/fails to prove the stale branch is
exercised; then proceed to create the EditTool and call edit.execute as now.
Ensure you reference the same filepath used in the test and use the testing
framework’s expect(...).toThrow/await expect(...).rejects.toThrow pattern so the
test fails if the stale assertion does not occur.

🧹 Nitpick comments (1)

packages/opencode/src/tool/edit.ts (1)

89-91: Drop unused stale binding.

stale is never read after Line 90, so this can be simplified to avoid dead local state.

♻️ Proposed cleanup

-      const { stale } = await FileTime.assertOrRefresh(ctx.sessionID, filePath)
+      await FileTime.assertOrRefresh(ctx.sessionID, filePath)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/src/tool/edit.ts` around lines 89 - 91, The local binding
"stale" returned from FileTime.assertOrRefresh is never used; replace the
destructuring assignment in the edit flow by calling
FileTime.assertOrRefresh(ctx.sessionID, filePath) without capturing the result
(i.e., remove "const { stale } ="). Update the call site inside the function
that references ctx.sessionID and filePath (the invocation of
FileTime.assertOrRefresh) so it awaits the call but does not create an unused
local variable.

🤖 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/cli/cmd/tui/component/tips.tsx`:
- Around line 53-55: The comment above the tipIndex assignment is misleading by
referencing React's useMemo; update the comment to reference SolidJS semantics
(or createMemo) or simply state that Solid component initialization runs once so
a random tip can be selected on mount; locate the tipIndex constant declaration
(tipIndex = Math.random()) in tips.tsx and replace the mention of "useMemo" with
either "createMemo" or a note that no memo hook is required in SolidJS to avoid
confusion for future maintainers.

---

Outside diff comments:
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 241-259: The test needs an explicit precondition asserting the
file is stale before calling EditTool.execute: after simulating the external
modification (the fs.writeFile call) and before creating EditTool via
EditTool.init()/calling edit.execute, invoke the stale-check helper
(FileTime.assert or whatever function validates read timestamps) and assert it
throws/fails to prove the stale branch is exercised; then proceed to create the
EditTool and call edit.execute as now. Ensure you reference the same filepath
used in the test and use the testing framework’s expect(...).toThrow/await
expect(...).rejects.toThrow pattern so the test fails if the stale assertion
does not occur.

---

Nitpick comments:
In `@packages/opencode/src/tool/edit.ts`:
- Around line 89-91: The local binding "stale" returned from
FileTime.assertOrRefresh is never used; replace the destructuring assignment in
the edit flow by calling FileTime.assertOrRefresh(ctx.sessionID, filePath)
without capturing the result (i.e., remove "const { stale } ="). Update the call
site inside the function that references ctx.sessionID and filePath (the
invocation of FileTime.assertOrRefresh) so it awaits the call but does not
create an unused local variable.

🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5d58c351-94f5-4dbe-b9a5-0e1da66f0664

📥 Commits

Reviewing files that changed from the base of the PR and between cfe1bad and 946ec9f.

📒 Files selected for processing (12)

• .opencode/skills/data-viz/references/component-guide.md
• docs/docs/reference/security-faq.md
• docs/docs/reference/telemetry.md
• packages/opencode/src/altimate/telemetry/index.ts
• packages/opencode/src/cli/cmd/tui/component/tips.tsx
• packages/opencode/src/cli/cmd/tui/routes/home.tsx
• packages/opencode/src/cli/welcome.ts
• packages/opencode/src/file/time.ts
• packages/opencode/src/tool/edit.ts
• packages/opencode/src/tool/write.ts
• packages/opencode/test/telemetry/telemetry.test.ts
• packages/opencode/test/tool/edit.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix misleading comment: "useMemo" is React terminology, not SolidJS.

The comment references "useMemo" which is a React hook. SolidJS uses createMemo. The approach works correctly (component functions run once in SolidJS), but the comment could confuse maintainers.

📝 Suggested fix

-  // Pick random tip index once on mount instead of recalculating randomly when props change
-  // Use useMemo without dependencies so it only evaluates once
+  // Pick random tip index once on mount instead of recalculating randomly when props change
+  // In SolidJS, component functions only run once, so this constant is stable across re-renders
   const tipIndex = Math.random()

📝 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

	// Pick random tip index once on mount instead of recalculating randomly when props change
	// Use useMemo without dependencies so it only evaluates once
	const tipIndex = Math.random()
	// Pick random tip index once on mount instead of recalculating randomly when props change
	// In SolidJS, component functions only run once, so this constant is stable across re-renders
	const tipIndex = Math.random()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/src/cli/cmd/tui/component/tips.tsx` around lines 53 - 55,
The comment above the tipIndex assignment is misleading by referencing React's
useMemo; update the comment to reference SolidJS semantics (or createMemo) or
simply state that Solid component initialization runs once so a random tip can
be selected on mount; locate the tipIndex constant declaration (tipIndex =
Math.random()) in tips.tsx and replace the mention of "useMemo" with either
"createMemo" or a note that no memo hook is required in SolidJS to avoid
confusion for future maintainers.