test: fix intermittent CI failures - #41
Conversation
- Resolves symlinks on workDir in ContainedInWorkDir to support macOS temp directories. - Increases timeout for second request in TestSubprocessManagerTimeout to fix race conditions. Co-authored-by: eshanized <148610067+eshanized@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
- Resolves symlinks on workDir in ContainedInWorkDir to support macOS temp directories. - Increases timeout for second request in TestSubprocessManagerTimeout to fix race conditions. Co-authored-by: eshanized <148610067+eshanized@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/tools/fileops/pathhelpers.go">
<violation number="1" location="internal/tools/fileops/pathhelpers.go:75">
P2: ContainedInWorkDir resolves `workDir` via EvalSymlinks but leaves the `resolved` argument un-normalized, so the function's equality/prefix check is now asymmetric: it compares a possibly-un-resolved `resolved` against a resolved `workDir`. All production callers happen to pre-resolve `resolved`, so the macOS symlink case works there — but the exported function's own contract ("checks that resolved is either equal to workDir or contained within it") doesn't require pre-resolved input, and the direct unit tests pass the raw `workDir` as `resolved`. On macOS, where the temp dir lives under `/var` (symlink to `/private/var`), `ContainedInWorkDir(workDir, workDir)` and `ContainedInWorkDir(subDir, workDir)` would now be rejected as 'outside working directory' — reintroducing the very false rejection this PR fixes for any caller that doesn't pre-resolve. Recommend normalizing `resolved` symmetrically inside the function (e.g. also EvalSymlinks `resolved` and fall back on failure so non-existent-path callers like ResolveAndContainPath still work), or explicitly update the direct tests to pass resolved paths and document the requirement.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // contained within it (with a trailing separator guard to prevent prefix attacks). | ||
| func ContainedInWorkDir(resolved, workDir string) error { | ||
| workDirPrefix := workDir | ||
| resolvedWorkDir, err := filepath.EvalSymlinks(workDir) |
There was a problem hiding this comment.
P2: ContainedInWorkDir resolves workDir via EvalSymlinks but leaves the resolved argument un-normalized, so the function's equality/prefix check is now asymmetric: it compares a possibly-un-resolved resolved against a resolved workDir. All production callers happen to pre-resolve resolved, so the macOS symlink case works there — but the exported function's own contract ("checks that resolved is either equal to workDir or contained within it") doesn't require pre-resolved input, and the direct unit tests pass the raw workDir as resolved. On macOS, where the temp dir lives under /var (symlink to /private/var), ContainedInWorkDir(workDir, workDir) and ContainedInWorkDir(subDir, workDir) would now be rejected as 'outside working directory' — reintroducing the very false rejection this PR fixes for any caller that doesn't pre-resolve. Recommend normalizing resolved symmetrically inside the function (e.g. also EvalSymlinks resolved and fall back on failure so non-existent-path callers like ResolveAndContainPath still work), or explicitly update the direct tests to pass resolved paths and document the requirement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tools/fileops/pathhelpers.go, line 75:
<comment>ContainedInWorkDir resolves `workDir` via EvalSymlinks but leaves the `resolved` argument un-normalized, so the function's equality/prefix check is now asymmetric: it compares a possibly-un-resolved `resolved` against a resolved `workDir`. All production callers happen to pre-resolve `resolved`, so the macOS symlink case works there — but the exported function's own contract ("checks that resolved is either equal to workDir or contained within it") doesn't require pre-resolved input, and the direct unit tests pass the raw `workDir` as `resolved`. On macOS, where the temp dir lives under `/var` (symlink to `/private/var`), `ContainedInWorkDir(workDir, workDir)` and `ContainedInWorkDir(subDir, workDir)` would now be rejected as 'outside working directory' — reintroducing the very false rejection this PR fixes for any caller that doesn't pre-resolve. Recommend normalizing `resolved` symmetrically inside the function (e.g. also EvalSymlinks `resolved` and fall back on failure so non-existent-path callers like ResolveAndContainPath still work), or explicitly update the direct tests to pass resolved paths and document the requirement.</comment>
<file context>
@@ -72,11 +72,21 @@ func ResolveAndContainPathExists(path, workDir string) (string, error) {
// contained within it (with a trailing separator guard to prevent prefix attacks).
func ContainedInWorkDir(resolved, workDir string) error {
- workDirPrefix := workDir
+ resolvedWorkDir, err := filepath.EvalSymlinks(workDir)
+ if err != nil {
+ if os.IsNotExist(err) {
</file context>
Fixes failing tests in CI by making paths correctly match symlinks in macOS temporary directories and fixing a race condition timeout in subprocess execution.
PR created automatically by Jules for task 7927591488349224864 started by @eshanized
Summary by cubic
Fixes flaky CI on macOS by resolving
workDirsymlinks in path checks and by increasing a subprocess test timeout to avoid race conditions.workDirinContainedInWorkDir, preventing false “outside working directory” errors with macOS temp paths.TestSubprocessManagerTimeoutfrom 2s to 5s.Written for commit 251608c. Summary will update on new commits.