Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if err := process.ResumeSuspended(command.Process); err != nil { | ||
| killErr := command.Process.Kill() | ||
| waitErr := command.Wait() | ||
| var closeErr error | ||
| if job != nil { | ||
| closeErr = job.Close() | ||
| } | ||
| failure := newError(protocol.CodeUVExecFailed, options.Stage, "uv 进程隔离失败", | ||
| map[string]any{"failureKind": "process_isolation_failed"}, errors.Join(err, killErr, waitErr, closeErr)) | ||
| failure.diagnostics = isolationDiagnostics(UVResult{ExitCode: exitCode(waitErr), Duration: clock().Sub(started)}, resolved, "resume_process", err) |
There was a problem hiding this comment.
Bug: A cancellation occurring during process.ResumeSuspended is misclassified as CodeUVExecFailed instead of returning a cancellation error.
Severity: LOW
Suggested Fix
In the process.ResumeSuspended failure branch, check ctx.Err(). If ctx.Err() != nil, return runnerCancellationError instead of returning CodeUVExecFailed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: internal/uv/runner.go#L308-L317
Potential issue: When `ctx` is canceled while `process.ResumeSuspended` is executing,
the process watcher may kill the process mid-resume. Failure during thread snapshotting
or resuming routes execution into the resume failure branch. Because this branch
directly constructs and returns a `CodeUVExecFailed` error with failure kind
`process_isolation_failed` without checking `ctx.Err()`, cancellation during this
specific timing window is misclassified as an isolation failure instead of returning
`runnerCancellationError`.
Did we get this right? 👍 / 👎 to inform future reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e166c55e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| failure := newError(protocol.CodeUVExecFailed, options.Stage, "uv 进程隔离失败", | ||
| map[string]any{"failureKind": "process_isolation_failed"}, errors.Join(err, killErr, waitErr, closeErr)) |
There was a problem hiding this comment.
On Windows, if cancellation occurs after the preceding ctx.Err() check but before or during ResumeSuspended, the exec.CommandContext watcher can kill the suspended child before its initial thread is found or resumed. This branch then reports UV_EXEC_FAILED: command.Wait() returns an *exec.ExitError for the killed child, so the joined errors need not contain the context cancellation. Callers such as environment check propagate that error to classifyFailure, which consequently reports an isolation failure with exit code 50 instead of OPERATION_CANCELLED with exit code 130. Recheck ctx.Err() after cleanup and use runnerCancellationError, preserving the cleanup errors, when cancellation won this race; the existing cancellation test only covers cancellation before the preceding check.
Useful? React with 👍 / 👎.
已由 PR #37 替代。新 PR 和提交说明已改成直白的说法，文件内容逐个核对一致。