Repository navigation
fix(executor): preserve Codex runtime task titles - #1636
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughTwo independent changes: ChangesThread naming on task creation
Atomic index file writes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
executor/src/agents/codex.rs (1)
355-370: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstrain the initial name RPC to newly started threads.
Line 355 only checks whether a name exists, so any future caller that passes both
resume_thread_idandinitial_thread_namewill rename an existing Codex thread. Guard this on the start path to preserve the “initial title only” contract at the callee boundary.Proposed fix
- if let Some(name) = initial_thread_name - .as_deref() - .map(str::trim) - .filter(|name| !name.is_empty()) - { - with_rpc_timeout( - "thread/name/set", - timeout_seconds, - rpc.request( - "thread/name/set", - json!({"threadId": thread_id.clone(), "name": name}), - &mut state, - ), - ) - .await?; + if thread_operation == "thread/start" { + if let Some(name) = initial_thread_name + .as_deref() + .map(str::trim) + .filter(|name| !name.is_empty()) + { + with_rpc_timeout( + "thread/name/set", + timeout_seconds, + rpc.request( + "thread/name/set", + json!({"threadId": thread_id.clone(), "name": name}), + &mut state, + ), + ) + .await?; + } }🤖 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/src/agents/codex.rs` around lines 355 - 370, Guard the thread naming RPC in codex.rs so it only runs for newly started threads, not when resuming an existing one. Update the initial_thread_name handling in the code path around with_rpc_timeout and rpc.request("thread/name/set", ...) to additionally check that resume_thread_id is absent (or equivalent start-path condition) before sending the rename request. Keep the “initial title only” behavior enforced at the callee boundary by tying the name-setting logic to the thread creation flow, not just the presence of initial_thread_name.
🤖 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/src/runtime_work/store.rs`:
- Around line 125-128: The write path in store.rs uses fs::rename to swap the
temporary file into index_path, which fails to overwrite an existing destination
on Windows. Update the write flow in the affected store method to use a
cross-platform replace/atomic overwrite approach when promoting
temporary_index_path(&self.index_path) to self.index_path, and keep the cleanup
of the temp file for failure cases.
---
Nitpick comments:
In `@executor/src/agents/codex.rs`:
- Around line 355-370: Guard the thread naming RPC in codex.rs so it only runs
for newly started threads, not when resuming an existing one. Update the
initial_thread_name handling in the code path around with_rpc_timeout and
rpc.request("thread/name/set", ...) to additionally check that resume_thread_id
is absent (or equivalent start-path condition) before sending the rename
request. Keep the “initial title only” behavior enforced at the callee boundary
by tying the name-setting logic to the thread creation flow, not just the
presence of initial_thread_name.
🪄 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: b56f46e0-7fd0-4272-af3f-58b9cceee304
📒 Files selected for processing (3)
executor/src/agents/codex.rsexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/store.rs
Comment on lines
+125
to
+128
| let temp_path = temporary_index_path(&self.index_path); | ||
| if fs::write(&temp_path, payload).is_ok() { | ||
| if fs::rename(&temp_path, &self.index_path).is_err() { | ||
| let _ = fs::remove_file(temp_path); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking whether the repo advertises Windows support in CI/docs/manifests..."
fd -HI 'README.md|Cargo.toml|*.yml|*.yaml' . \
| xargs -r rg -n -i 'windows|windows-latest|target_os\s*=\s*"windows"|cfg\(windows\)'
echo
echo "Looking for existing file-replacement helpers or atomic-write utilities..."
rg -n 'replace|rename\(|NamedTempFile|persist|atomic' executor .Repository: wecode-ai/Wegent
Length of output: 565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files mentioning Windows/support targets:"
git ls-files | rg -n '(^|/)(README\.md|Cargo\.toml|.*\.(ya?ml))$' || true
echo
echo "Search for Windows-target or Windows CI references:"
git ls-files | rg -n '(^|/)(README\.md|Cargo\.toml|.*\.(ya?ml))$' | while read -r f; do
rg -n -i 'windows|windows-latest|target_os\s*=\s*"windows"|cfg\(windows\)' "$f" || true
done
echo
echo "Relevant store.rs excerpt:"
sed -n '1,220p' executor/src/runtime_work/store.rs
echo
echo "Search for atomic-write / temp-file helpers:"
rg -n 'NamedTempFile|persist|atomic|rename\(' executor src . || trueRepository: wecode-ai/Wegent
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant manifest/docs files:"
git ls-files | rg '(^|/)(README\.md|Cargo\.toml|.*\.(ya?ml))$' || trueRepository: wecode-ai/Wegent
Length of output: 1949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant repo files:"
git ls-files | rg '(^|/)(README\.md|Cargo\.toml|.*\.(ya?ml))$' || true
echo
echo "Windows references in those files:"
while IFS= read -r f; do
rg -n -i 'windows|windows-latest|target_os\s*=\s*"windows"|cfg\(windows\)' "$f" || true
done < <(git ls-files | rg '(^|/)(README\.md|Cargo\.toml|.*\.(ya?ml))$')
echo
echo "store.rs excerpt:"
sed -n '110,170p' executor/src/runtime_work/store.rs
echo
echo "Any atomic-write or helper utilities elsewhere:"
rg -n 'NamedTempFile|persist|atomic|replace\(' . || trueRepository: wecode-ai/Wegent
Length of output: 2029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Manifest/docs files:"
git ls-files | rg '(^|/)(README\.md|Cargo\.toml|.*\.(ya?ml))$'Repository: wecode-ai/Wegent
Length of output: 1940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "executor/Cargo.toml:"
sed -n '1,220p' executor/Cargo.toml
echo
echo ".github/workflows/test-windows-executor.yml:"
sed -n '1,220p' .github/workflows/test-windows-executor.yml
echo
echo "Windows cfg / target references in executor:"
rg -n 'target_os\s*=\s*"windows"|cfg\(windows\)|windows-latest|windows' executor .github/workflows/test-windows-executor.yml README.md executor/Cargo.tomlRepository: wecode-ai/Wegent
Length of output: 10643
Use a cross-platform replace when writing index_path. fs::rename won’t overwrite an existing file on Windows, so once the index exists these updates can fail and the new payload gets dropped during cleanup.
🤖 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/src/runtime_work/store.rs` around lines 125 - 128, The write path in
store.rs uses fs::rename to swap the temporary file into index_path, which fails
to overwrite an existing destination on Windows. Update the write flow in the
affected store method to use a cross-platform replace/atomic overwrite approach
when promoting temporary_index_path(&self.index_path) to self.index_path, and
keep the cleanup of the temp file for failure cases.
This was referenced Jul 1, 2026
Merged
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
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
Tests
Summary by CodeRabbit
New Features
Bug Fixes