Skip to content

fix(agent): make deleted files readable through file_read_diff - #1698

Merged
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
Trainingcqy:fix/deleted-file-diff-lookup
Oct 12, 2026
Merged

lizhengfeng101 merged 2 commits into
alibaba:mainfrom
Trainingcqy:fix/deleted-file-diff-lookup

Conversation

@Trainingcqy

Copy link
Copy Markdown
Contributor

Description

#1680 makes the change list show deleted files under their real paths, but calling file_read_diff with those paths fails. file_read_diff reads diffs by path from a map built by injectDiffMap, which skips entries whose NewPath is the sentinel /dev/null and keys the rest by NewPath. A deleted file has /dev/null as its NewPath and its real path only in OldPath, so the map has no entries for deleted files.

Fix: the map is now keyed by effectivePath(d). The change list in #1680 prints the same value, so every file name in the list is a key in the map. For files that were not deleted, effectivePath(d) returns NewPath, and their keys stay the same.

The map is built before file selection, so deleted files that selection excludes are readable too, including those matching a secret path built into OCR. Files that were not deleted already behave this way on main; for example, a modified .env that selection excludes can be read through file_read_diff. #1262 also states that agent runtime tools were intentionally left unchanged when it introduced the secret path rules.

TestInjectDiffMap previously asserted that looking up deleted.go returns not found, and now asserts that the lookup returns the file's diff. The deletion fixture now carries the IsDeleted: true that the parser actually produces; otherwise an implementation that skips deleted files by IsDeleted would also pass the test. The test also adds a renamed file and asserts that it is still keyed by NewPath.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • With the test changes kept and the production change reverted, TestInjectDiffMap fails:

    coverage_test.go:151: DiffMap did not contain deleted.go diff, got: "Error: diff not found for the requested paths"
    
  • End to end: I built binaries from main and from this branch and ran ocr review --commit HEAD on a commit that deletes helper.go and modifies main.go. The LLM endpoint is a local stub, compatible with the OpenAI API, that records requests and answers the main task with a file_read_diff call for helper.go. On main the tool result is Error: diff not found for the requested paths; on this branch it is:

    ==== FILE: helper.go ====
    diff --git a/helper.go b/helper.go
    deleted file mode 100644
    --- a/helper.go
    +++ /dev/null
    @@ -1,3 +0,0 @@
    -package main
    -
    -func Helper() int { return 1 }
    

    With fix(agent): list deleted files under their own path in the change list #1680 merged into this branch, the same scenario shows DELETED helper.go (+0/-3) in the change list, and the tool result is the same as above.

  • make test passes locally

  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA
  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI/LLM disclosure, Claude Code (Claude Opus 5.5) was used to investigate the failure, prepare the changes, and draft this PR.

Related Issues

This PR follows up on #1680.

@github-actions

Copy link
Copy Markdown
Contributor

✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

@NanaseInori NanaseInori left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using effectivePath makes deleted-file diffs available under their original paths while retaining destination paths for renames. This matches the path convention used elsewhere in the agent. The updated test covers deletion and rename lookups. No blocking issue found in the code review.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 64654f8 into alibaba:main Oct 12, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants