Repository navigation
fix: 修复executor同时修改同一个文件目录，导致异常终止的问题 - #1423
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds multi-source turn file change tracking ( ChangesExecutor: Multi-source turn file change tracking
Wework: Tauri plugin-opener and external link handler
Wework: Per-file diff preview in FileChangesCard
Sequence DiagramssequenceDiagram
participant CodeXAgent
participant NativeTurnFileChangeTracker
participant CodeXEventMapper
participant TurnFileChangeArtifactStore
CodeXAgent->>NativeTurnFileChangeTracker: construct when device_id present
CodeXAgent->>CodeXEventMapper: construct with tracker
CodeXAgent->>TurnFileChangeArtifactStore: completion_fields_provider uses finalize
CodeXEventMapper->>NativeTurnFileChangeTracker: record_diff on turn/diff/updated
CodeXAgent->>NativeTurnFileChangeTracker: finalize at turn end
sequenceDiagram
participant User
participant FileChangesCard
participant onLoadDiff
participant FileChangesReviewDialog
User->>FileChangesCard: click FileChangeRow (file)
FileChangesCard->>FileChangesCard: set reviewFile
FileChangesCard->>FileChangesCard: open dialog
alt diff not cached
FileChangesCard->>onLoadDiff: load diff for subtask
onLoadDiff-->>FileChangesCard: diff bytes
end
FileChangesCard->>FileChangesReviewDialog: pass targetFile + diff
FileChangesReviewDialog-->>User: display scoped diff
flowchart LR
openExternalUrl --> isHttpCheck["isHttpUrl()"]
isHttpCheck -- false --> returnFalse["return false"]
isHttpCheck -- true --> tauriCheck{"__TAURI_INTERNALS__?"}
tauriCheck -- yes --> tauriOpener["await openUrl()"]
tauriCheck -- no --> windowOpen["window.open()"]
tauriOpener --> returnTrue["return true"]
windowOpen --> returnTrue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/components/layout/EnvironmentInfoPopover.tsx (1)
77-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid unhandled promise rejection when opening PR URL.
Line 81 calls an async opener with
voidand no rejection handling. If opener fails in Tauri runtime, this can surface as an unhandled rejection.Suggested fix
- function handleCreatePullRequest() { + async function handleCreatePullRequest() { if (!info.createPullRequestUrl) { return } - void openExternalUrl(info.createPullRequestUrl) + try { + await openExternalUrl(info.createPullRequestUrl) + } catch (error) { + console.error('Failed to open pull request URL:', error) + } }🤖 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 `@wework/src/components/layout/EnvironmentInfoPopover.tsx` around lines 77 - 82, The handleCreatePullRequest function calls the async openExternalUrl function with the void operator but provides no error handling for potential rejections. Add a catch handler to the openExternalUrl call to properly handle any errors that may occur when attempting to open the external URL, preventing unhandled promise rejections in the Tauri runtime. You can use a .catch() method chained to the function call or wrap it in a try-catch block if restructuring to async-await.
🧹 Nitpick comments (2)
wework/src/components/chat/FileChangesReviewDialog.tsx (1)
121-125: ⚡ Quick winAdd disclosure ARIA attributes to the section toggle button.
This toggle controls collapsible content but doesn’t expose expanded state to assistive tech. Add
aria-expandedandaria-controls(with a stable panel id) so screen readers can track section state.Suggested patch
- <button + <button type="button" onClick={() => toggleSection(index)} + aria-expanded={!collapsed.has(index)} + aria-controls={`file-diff-panel-${index}`} className="flex w-full items-center gap-1.5 border-b border-border bg-surface px-2.5 py-1.5 text-left font-mono text-xs font-medium text-text-primary hover:bg-muted" > @@ - <pre className="grid overflow-x-auto bg-background p-0 font-mono text-xs leading-5"> + <pre + id={`file-diff-panel-${index}`} + className="grid overflow-x-auto bg-background p-0 font-mono text-xs leading-5" + >🤖 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 `@wework/src/components/chat/FileChangesReviewDialog.tsx` around lines 121 - 125, The toggle button in FileChangesReviewDialog that calls toggleSection(index) is missing accessibility attributes required by screen readers to understand its expanded/collapsed state. Add the aria-expanded attribute set to the boolean value indicating whether the section at the given index is expanded or collapsed, and add the aria-controls attribute set to a stable panel identifier (typically constructed using the index, such as "section-panel-" plus the index number) that corresponds to the id of the collapsible content panel that this button controls. This ensures assistive technology can properly announce the section's state and its relationship to the controlled content.wework/src/main.tsx (1)
11-11: ⚡ Quick winDispose the global click handler on HMR updates.
At Line [11], the returned cleanup function is ignored; during hot reloads this can accumulate duplicate document listeners.
Suggested change
installDesktopExtensions() -installExternalLinkHandler() +const removeExternalLinkHandler = installExternalLinkHandler() +if (import.meta.hot) { + import.meta.hot.dispose(removeExternalLinkHandler) +} installPageZoomGuard()🤖 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 `@wework/src/main.tsx` at line 11, The call to installExternalLinkHandler() returns a cleanup function that is currently being ignored. During hot module replacement (HMR) updates, this causes duplicate document listeners to accumulate. Capture the returned cleanup function from installExternalLinkHandler() and ensure it is called during HMR updates by integrating it with your module's hot reload handler to properly dispose of the previous event listeners before new ones are installed.
🤖 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 `@executor/agents/claude_code/claude_code_agent.py`:
- Around line 1218-1231: The hook registration in the loop for "PreToolUse" and
"PostToolUse" events appends HookMatcher objects to existing hooks retrieved
from self.options, causing duplicate hooks when _create_and_connect_client() is
called multiple times. Instead of fetching and appending to existing hooks,
initialize a fresh hooks dictionary or explicitly reset the event_hooks for
these specific event names before appending the new HookMatcher. This ensures
that each call to _create_and_connect_client() registers the hooks exactly once
without accumulation.
In `@executor/services/turn_file_changes.py`:
- Around line 400-462: The newly added methods in the
TurnFileChangeArtifactStore class and related tracker/store methods lack
OpenTelemetry tracing decorators, creating observability gaps. First, ensure the
tracing decorators are imported from 'shared/telemetry/decorators.py' at the top
of the file. Then, apply `@trace_sync`() decorator to all synchronous methods and
`@trace_async`() decorator to all asynchronous methods across the affected
locations: the TurnFileChangeArtifactStore class and its methods including
completion_fields_from_patch at lines 400-462, the methods at lines 648-657, the
methods at lines 735-852, and the methods at lines 863-895. This will instrument
all new tracker/store lifecycle paths with proper tracing for observability.
In `@wework/src/components/chat/FileChangesCard.test.tsx`:
- Around line 108-110: The negative assertion at line 108 is checking for 'old
one' but the actual rendered text includes the dash prefix, so it should check
for '-old one' to match the rendered line format and properly verify that the
first file content is excluded. Update the queryByText call in the assertion to
include the dash prefix to be consistent with the subsequent assertion checking
for '-old two'.
In `@wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx`:
- Around line 161-163: The openExternalUrl function returns a boolean indicating
success or failure, but the code ignores this return value and always sets
shouldClosePanel to true. At both locations (lines 161-163 and 185-186 in
WorkspacePanelCards.tsx), wrap the shouldClosePanel assignment in a conditional
check: only set shouldClosePanel to true if openExternalUrl returns true. This
ensures the panel only closes when the URL was actually opened successfully, not
when validation fails due to non-HTTP(S) URLs.
In `@wework/src/lib/external-links.ts`:
- Around line 25-26: The browser fallback in the external-links.ts file
unconditionally returns true when calling window.open, even though window.open
can return null when popups are blocked. Capture the return value from the
window.open() call and check whether it was successful (non-null) before
returning the result. Return the actual success status instead of always
returning true, so that blocked popups are properly reported as failures.
---
Outside diff comments:
In `@wework/src/components/layout/EnvironmentInfoPopover.tsx`:
- Around line 77-82: The handleCreatePullRequest function calls the async
openExternalUrl function with the void operator but provides no error handling
for potential rejections. Add a catch handler to the openExternalUrl call to
properly handle any errors that may occur when attempting to open the external
URL, preventing unhandled promise rejections in the Tauri runtime. You can use a
.catch() method chained to the function call or wrap it in a try-catch block if
restructuring to async-await.
---
Nitpick comments:
In `@wework/src/components/chat/FileChangesReviewDialog.tsx`:
- Around line 121-125: The toggle button in FileChangesReviewDialog that calls
toggleSection(index) is missing accessibility attributes required by screen
readers to understand its expanded/collapsed state. Add the aria-expanded
attribute set to the boolean value indicating whether the section at the given
index is expanded or collapsed, and add the aria-controls attribute set to a
stable panel identifier (typically constructed using the index, such as
"section-panel-" plus the index number) that corresponds to the id of the
collapsible content panel that this button controls. This ensures assistive
technology can properly announce the section's state and its relationship to the
controlled content.
In `@wework/src/main.tsx`:
- Line 11: The call to installExternalLinkHandler() returns a cleanup function
that is currently being ignored. During hot module replacement (HMR) updates,
this causes duplicate document listeners to accumulate. Capture the returned
cleanup function from installExternalLinkHandler() and ensure it is called
during HMR updates by integrating it with your module's hot reload handler to
properly dispose of the previous event listeners before new ones are installed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf959ff0-c834-4951-8fbc-1678d29b5695
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlwework/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
docs/en/developer-guide/wework-startup-animation.mddocs/zh/developer-guide/wework-startup-animation.mdexecutor/agents/base.pyexecutor/agents/claude_code/claude_code_agent.pyexecutor/agents/codex/codex_agent.pyexecutor/agents/codex/event_mapper.pyexecutor/services/turn_file_changes.pyexecutor/tests/agents/test_claude_code_config_security.pyexecutor/tests/agents/test_codex_event_mapper.pyexecutor/tests/services/test_turn_file_changes.pywework/package.jsonwework/src-tauri/Cargo.tomlwework/src-tauri/capabilities/default.jsonwework/src-tauri/src/lib.rswework/src/components/chat/FileChangesCard.test.tsxwework/src/components/chat/FileChangesCard.tsxwework/src/components/chat/FileChangesReviewDialog.tsxwework/src/components/layout/EnvironmentInfoPopover.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/i18n/locales/en/chat.jsonwework/src/i18n/locales/zh-CN/chat.jsonwework/src/lib/external-links.test.tswework/src/lib/external-links.tswework/src/main.tsxwework/src/test/theme-token-guard.test.tswework/tailwind.config.js
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 `@wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx`:
- Around line 153-171: The Promise.all coupling of getLocalExecutorDeviceId and
localPathExists operations causes both probes to fail together, incorrectly
clearing executorDeviceId when only localPathExists fails. Decouple these two
probes by calling them separately instead of with Promise.all, and handle each
with its own error handling. This way, getLocalExecutorDeviceId can still
populate executorDeviceId even if localPathExists fails, and vice versa. Each
probe should independently update the corresponding field in the
setLocalTerminalCheck call (deviceId from getLocalExecutorDeviceId and
pathExists from localPathExists), with sensible defaults in their respective
error handlers.
In `@wework/src/components/settings/ConnectionsSettingsPage.tsx`:
- Around line 534-537: The window.open call in the startTerminal result handler
bypasses the centralized external-link guard and security validation. Replace
the window.open call with openExternalUrl(result.url) to ensure proper http(s)
validation, Tauri opener integration, and standardized security attributes
(noopener,noreferrer). Ensure openExternalUrl is imported from the appropriate
utility module if not already available in the file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 620a9237-24b2-4fde-b14d-bba9da650be7
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlwework/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
wework/package.jsonwework/src-tauri/Cargo.tomlwework/src-tauri/capabilities/default.jsonwework/src-tauri/src/lib.rswework/src-tauri/tauri.conf.jsonwework/src/components/chat/FileChangesCard.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.tsxwework/src/components/settings/ConnectionsSettingsPage.tsx
✅ Files skipped from review due to trivial changes (1)
- wework/src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- wework/src-tauri/capabilities/default.json
- wework/src-tauri/src/lib.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx`:
- Around line 153-171: The Promise.all coupling of getLocalExecutorDeviceId and
localPathExists operations causes both probes to fail together, incorrectly
clearing executorDeviceId when only localPathExists fails. Decouple these two
probes by calling them separately instead of with Promise.all, and handle each
with its own error handling. This way, getLocalExecutorDeviceId can still
populate executorDeviceId even if localPathExists fails, and vice versa. Each
probe should independently update the corresponding field in the
setLocalTerminalCheck call (deviceId from getLocalExecutorDeviceId and
pathExists from localPathExists), with sensible defaults in their respective
error handlers.
In `@wework/src/components/settings/ConnectionsSettingsPage.tsx`:
- Around line 534-537: The window.open call in the startTerminal result handler
bypasses the centralized external-link guard and security validation. Replace
the window.open call with openExternalUrl(result.url) to ensure proper http(s)
validation, Tauri opener integration, and standardized security attributes
(noopener,noreferrer). Ensure openExternalUrl is imported from the appropriate
utility module if not already available in the file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 620a9237-24b2-4fde-b14d-bba9da650be7
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlwework/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
wework/package.jsonwework/src-tauri/Cargo.tomlwework/src-tauri/capabilities/default.jsonwework/src-tauri/src/lib.rswework/src-tauri/tauri.conf.jsonwework/src/components/chat/FileChangesCard.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.test.tsxwework/src/components/layout/workspace-panels/WorkspacePanelCards.tsxwework/src/components/settings/ConnectionsSettingsPage.tsx
✅ Files skipped from review due to trivial changes (1)
- wework/src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- wework/src-tauri/capabilities/default.json
- wework/src-tauri/src/lib.rs
🛑 Comments failed to post (2)
wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx (1)
153-171:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecouple executor-id and local-path probes to avoid false negative local terminal blocking.
A failure in
localPathExists(projectLocalPath)currently clearsexecutorDeviceIdtoo, because both probes are tied byPromise.all. That can disable local terminal even when executor IDs already match.Suggested fix
- Promise.all([getLocalExecutorDeviceId(apiBaseUrl), localPathExists(projectLocalPath)]) - .then(([deviceId, pathExists]) => { - if (!cancelled) { - setLocalTerminalCheck({ - key: localTerminalCheckKey, - executorDeviceId: deviceId, - pathExists, - }) - } - }) - .catch(() => { - if (!cancelled) { - setLocalTerminalCheck({ - key: localTerminalCheckKey, - executorDeviceId: null, - pathExists: false, - }) - } - }) + Promise.allSettled([getLocalExecutorDeviceId(apiBaseUrl), localPathExists(projectLocalPath)]).then( + ([deviceIdResult, pathExistsResult]) => { + if (cancelled) return + setLocalTerminalCheck({ + key: localTerminalCheckKey, + executorDeviceId: deviceIdResult.status === 'fulfilled' ? deviceIdResult.value : null, + pathExists: pathExistsResult.status === 'fulfilled' ? pathExistsResult.value : false, + }) + } + )🤖 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 `@wework/src/components/layout/workspace-panels/WorkspacePanelCards.tsx` around lines 153 - 171, The Promise.all coupling of getLocalExecutorDeviceId and localPathExists operations causes both probes to fail together, incorrectly clearing executorDeviceId when only localPathExists fails. Decouple these two probes by calling them separately instead of with Promise.all, and handle each with its own error handling. This way, getLocalExecutorDeviceId can still populate executorDeviceId even if localPathExists fails, and vice versa. Each probe should independently update the corresponding field in the setLocalTerminalCheck call (deviceId from getLocalExecutorDeviceId and pathExists from localPathExists), with sensible defaults in their respective error handlers.wework/src/components/settings/ConnectionsSettingsPage.tsx (1)
534-537:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
openExternalUrlhere instead of rawwindow.open.This path bypasses the centralized external-link guard (
http(s)validation + Tauri opener) and drops the standardizednoopener,noreferrerbehavior.Suggested fix
const result = await createSettingsDeviceApi().startTerminal(device.device_id) if (result.url) { - window.open(result.url, '_blank', 'noopener') + const opened = await openExternalUrl(result.url) + if (!opened) { + throw new Error('Terminal session URL must be http(s)') + } }📝 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.const result = await createSettingsDeviceApi().startTerminal(device.device_id) if (result.url) { const opened = await openExternalUrl(result.url) if (!opened) { throw new Error('Terminal session URL must be http(s)') } }🤖 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 `@wework/src/components/settings/ConnectionsSettingsPage.tsx` around lines 534 - 537, The window.open call in the startTerminal result handler bypasses the centralized external-link guard and security validation. Replace the window.open call with openExternalUrl(result.url) to ensure proper http(s) validation, Tauri opener integration, and standardized security attributes (noopener,noreferrer). Ensure openExternalUrl is imported from the appropriate utility module if not already available in the file.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
executor/services/turn_file_changes.py (3)
499-506:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse
diff --githeaders without splitting on every space.
line.split(" ")misparses valid paths likemy file.txt, sofile_changes.filescan report the wrong path for artifacts containing spaces in filenames.Suggested parser adjustment
`@staticmethod` def _paths_from_diff_header(line: str) -> tuple[Optional[str], Optional[str]]: - parts = line.split(" ") - if len(parts) < 4: + prefix = "diff --git " + if not line.startswith(prefix): return None, None - old_path = TurnFileChangeArtifactStore._strip_diff_prefix(parts[2]) - new_path = TurnFileChangeArtifactStore._strip_diff_prefix(parts[3]) + payload = line.removeprefix(prefix) + if not payload.startswith("a/") or " b/" not in payload: + return None, None + old_raw, new_raw = payload.rsplit(" b/", 1) + old_path = TurnFileChangeArtifactStore._strip_diff_prefix(old_raw) + new_path = TurnFileChangeArtifactStore._strip_diff_prefix(f"b/{new_raw}") return new_path, old_path🤖 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 `@executor/services/turn_file_changes.py` around lines 499 - 506, The _paths_from_diff_header method uses line.split(" ") which incorrectly parses diff headers when file paths contain spaces. Instead of splitting on every space character, parse the diff header by locating the positions of " a/" and " b/" prefixes to properly extract the old_path and new_path components, since the standard diff --git format places these prefixes before the file paths and splitting indiscriminately corrupts paths with spaces in their names.
677-695:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the first baseline for a tool-use ID.
The same tracker is wired from Claude hooks and response-stream recording, so
pre_tool_use()can run twice for onetool_use_idbeforepost_tool_use(). The second call currently overwrites the earlier snapshot, which can make the captured patch empty or partial if it fires later.Suggested guard
if tool_use_id in self._completed_tool_use_ids: logger.info( "Claude edit baseline skipped because tool already completed: task_id=%s subtask_id=%s tool_use_id=%s", self.task_id, self.subtask_id, tool_use_id, ) return {} + if self._has_baseline(tool_use_id): + logger.info( + "Claude edit baseline skipped because baseline already exists: task_id=%s subtask_id=%s tool_use_id=%s", + self.task_id, + self.subtask_id, + tool_use_id, + ) + return {} try:🤖 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 `@executor/services/turn_file_changes.py` around lines 677 - 695, The pre_tool_use() method can be called twice for the same tool_use_id before post_tool_use() executes, causing the second call to overwrite the baseline snapshot. Add a guard condition before capturing the baseline in both branches (git snapshots and non-git file snapshots) to check if the baseline has already been recorded for this tool_use_id. Specifically, check if tool_use_id already exists in self._before_tree_by_tool_use_id (for the git snapshots branch) or self._before_file_by_tool_use_id (for the file snapshots branch), and skip the baseline capture if it has already been recorded.
811-818:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSummarize the net change, not the rollback-ordered patch.
finalize()reverses per-tool patches before summarizing, while_changed_paths_from_patch()and_numstat_by_path()collapse duplicate paths by overwrite. If the same file is edited multiple times, or modified then deleted, the user-facingchange_typeand additions/deletions can reflect the wrong diff section rather than the final workspace state.Persist the rollback-ordered patch if that is needed for revert, but compute
file_changesfrom a chronological/net patch or aggregate duplicate paths with final-state precedence.Also applies to: 409-430, 469-469, 523-526
🤖 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 `@executor/services/turn_file_changes.py` around lines 811 - 818, The finalize() method reverses patches before computing file changes via completion_fields_from_patch(), which causes issues when files are edited multiple times or modified then deleted—the summarized change_type and additions/deletions reflect intermediate states rather than the final workspace state. Keep the rollback-ordered patch if needed for revert functionality, but compute file_changes from chronological (non-reversed) patches or modify _changed_paths_from_patch() and _numstat_by_path() to aggregate duplicate paths using final-state precedence so that when the same file appears multiple times in the patch, the last occurrence determines the reported change_type and statistics. This fix applies across the finalize() method at lines 811-818 (anchor), and related patch processing code at lines 409-430, line 469, and lines 523-526 (siblings).
🤖 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 `@executor/agents/base.py`:
- Around line 154-155: The unguarded `await tracker.start()` call can raise
exceptions during Git baseline capture, causing the entire turn to fail even
though tracking is an optional feature. Wrap the `await tracker.start()` call in
a try-except block to catch any exceptions, log the error appropriately, and
ensure `started` is set to False when startup fails so the turn can continue
executing without tracking.
In `@executor/services/turn_file_changes.py`:
- Line 19: Add `ClassVar` to the imports from the typing module on the import
line. Then locate the `EDIT_TOOL_MATCHER` and `EDIT_TOOL_NAMES` class attributes
and annotate both with `ClassVar` type hints to indicate they are class
variables. Additionally, convert the `EDIT_TOOL_NAMES` set literal to a
`frozenset` to make it immutable and resolve the RUF012 warning about mutable
default values.
In `@executor/tests/services/test_turn_file_changes.py`:
- Line 245: Add type hints to the async test functions as required by the
repository's Python coding guidelines. In
executor/tests/services/test_turn_file_changes.py at line 245, add type hints to
the test_tracker_allows_concurrent_turns_in_same_workspace function by
annotating the tmp_path parameter with the appropriate pathlib type and adding a
return type hint of None. Apply the same changes to the other async test
function at line 352 in the same file, ensuring both functions have complete
type annotations for their parameters and return types to comply with repository
standards.
---
Outside diff comments:
In `@executor/services/turn_file_changes.py`:
- Around line 499-506: The _paths_from_diff_header method uses line.split(" ")
which incorrectly parses diff headers when file paths contain spaces. Instead of
splitting on every space character, parse the diff header by locating the
positions of " a/" and " b/" prefixes to properly extract the old_path and
new_path components, since the standard diff --git format places these prefixes
before the file paths and splitting indiscriminately corrupts paths with spaces
in their names.
- Around line 677-695: The pre_tool_use() method can be called twice for the
same tool_use_id before post_tool_use() executes, causing the second call to
overwrite the baseline snapshot. Add a guard condition before capturing the
baseline in both branches (git snapshots and non-git file snapshots) to check if
the baseline has already been recorded for this tool_use_id. Specifically, check
if tool_use_id already exists in self._before_tree_by_tool_use_id (for the git
snapshots branch) or self._before_file_by_tool_use_id (for the file snapshots
branch), and skip the baseline capture if it has already been recorded.
- Around line 811-818: The finalize() method reverses patches before computing
file changes via completion_fields_from_patch(), which causes issues when files
are edited multiple times or modified then deleted—the summarized change_type
and additions/deletions reflect intermediate states rather than the final
workspace state. Keep the rollback-ordered patch if needed for revert
functionality, but compute file_changes from chronological (non-reversed)
patches or modify _changed_paths_from_patch() and _numstat_by_path() to
aggregate duplicate paths using final-state precedence so that when the same
file appears multiple times in the patch, the last occurrence determines the
reported change_type and statistics. This fix applies across the finalize()
method at lines 811-818 (anchor), and related patch processing code at lines
409-430, line 469, and lines 523-526 (siblings).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 927fa2e6-ea58-4aca-b47f-3b539e07a43b
📒 Files selected for processing (8)
executor/agents/base.pyexecutor/agents/claude_code/claude_code_agent.pyexecutor/agents/claude_code/response_processor.pyexecutor/services/turn_file_changes.pyexecutor/tests/agents/test_claude_code_config_security.pyexecutor/tests/agents/test_claude_response_processor.pyexecutor/tests/agents/test_codex_event_mapper.pyexecutor/tests/services/test_turn_file_changes.py
💤 Files with no reviewable changes (1)
- executor/tests/agents/test_codex_event_mapper.py
🚧 Files skipped from review as they are similar to previous changes (1)
- executor/tests/agents/test_claude_code_config_security.py
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
2 participants
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 by CodeRabbit
Release Notes