Skip to content

test: fix intermittent CI failures - #41

Open
eshanized wants to merge 2 commits into
masterfrom
fix-ci-tests-7927591488349224864
Open

test: fix intermittent CI failures#41
eshanized wants to merge 2 commits into
masterfrom
fix-ci-tests-7927591488349224864

Conversation

@eshanized

@eshanized eshanized commented Aug 9, 2026

Copy link
Copy Markdown
Owner

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 workDir symlinks in path checks and by increasing a subprocess test timeout to avoid race conditions.

  • Bug Fixes
    • Resolve symlinks on workDir in ContainedInWorkDir, preventing false “outside working directory” errors with macOS temp paths.
    • Increase the second call timeout in TestSubprocessManagerTimeout from 2s to 5s.

Written for commit 251608c. Summary will update on new commits.

Review in cubic

- 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>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

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>
@eshanized
eshanized marked this pull request as ready for review August 9, 2026 18:40

@cubic-dev-ai cubic-dev-ai Bot 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.

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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

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.

1 participant