Skip to content

picod: fix path test on macOS - #436

Merged
volcano-sh-bot merged 1 commit into
volcano-sh:mainfrom
acsoto:fix/picod-macos-path-test
Jul 14, 2026
Merged

picod: fix path test on macOS#436
volcano-sh-bot merged 1 commit into
volcano-sh:mainfrom
acsoto:fix/picod-macos-path-test

Conversation

@acsoto

@acsoto acsoto commented Jul 13, 2026

Copy link
Copy Markdown
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes TestSanitizePath on macOS, where /var resolves to /private/var. The test now canonicalizes the temporary workspace before checking that sanitized paths remain inside it.

This only corrects the test assertion; production path sanitization is unchanged.

Which issue(s) this PR fixes:

None.

Special notes for your reviewer:

This change was prepared with AI assistance and manually verified.

Environment:

macOS 26.5.2
Darwin 25.5.0 arm64
go1.26.4 darwin/arm64

Before this change:

$ go test ./pkg/picod -run '^TestSanitizePath$' -count=1
--- FAIL: TestSanitizePath
    --- FAIL: TestSanitizePath/valid_relative_path
        Expect "../../../../../../private/var/.../test.txt" to NOT match "^\\.\\."
    --- FAIL: TestSanitizePath/valid_nested_path
    --- FAIL: TestSanitizePath/path_traversal_in_middle
    --- FAIL: TestSanitizePath/absolute_path
    --- FAIL: TestSanitizePath/empty_path
    --- FAIL: TestSanitizePath/current_directory
    --- FAIL: TestSanitizePath/deep_nested_path
FAIL

After this change:

$ go test ./pkg/picod -run '^TestSanitizePath$' -count=10
ok github.com/volcano-sh/agentcube/pkg/picod 0.687s

$ go test ./pkg/picod -count=1
ok github.com/volcano-sh/agentcube/pkg/picod 4.872s

The production implementation is unchanged.

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Zhou Zihang <z@mcac.cc>
Copilot AI review requested due to automatic review settings July 13, 2026 06:27
@volcano-sh-bot volcano-sh-bot added the kind/bug Something isn't working label Jul 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes macOS-specific failures in TestSanitizePath by canonicalizing the temp workspace path (resolving /var/private/var) before asserting sanitized results remain within the workspace. This aligns the test’s containment check with Server.sanitizePath, which compares against a symlink-resolved workspace.

Changes:

  • Resolve the temporary workspace directory via filepath.EvalSymlinks.
  • Use the resolved workspace path as the base for filepath.Rel containment assertions.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the test suite in pkg/picod/files_test.go to resolve symlinks for the temporary directory using filepath.EvalSymlinks before performing containment assertions. This ensures that tests pass consistently on platforms like macOS, where /var is a symlink to /private/var. There are no review comments provided, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.47%. Comparing base (3b19390) to head (450e7ca).
⚠️ Report is 21 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
+ Coverage   58.41%   58.47%   +0.05%     
==========================================
  Files          36       36              
  Lines        3463     3463              
==========================================
+ Hits         2023     2025       +2     
+ Misses       1231     1230       -1     
+ Partials      209      208       -1     
Flag Coverage Δ
unittests 58.47% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RainbowMango RainbowMango left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks.

/remove-kind bug
/kind cleanup

@volcano-sh-bot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot
volcano-sh-bot merged commit 3de1272 into volcano-sh:main Jul 14, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants