Skip to content

refactor: Replace custom copytree() with stdlib shutil.copytree#8934

Open
bnusunny wants to merge 1 commit intodevelopfrom
refactor/replace-custom-copytree-with-stdlib
Open

refactor: Replace custom copytree() with stdlib shutil.copytree#8934
bnusunny wants to merge 1 commit intodevelopfrom
refactor/replace-custom-copytree-with-stdlib

Conversation

@bnusunny
Copy link
Copy Markdown
Contributor

@bnusunny bnusunny commented Apr 23, 2026

Replace hand-rolled copytree implementations with
shutil.copytree(..., symlinks=True, dirs_exist_ok=True) in both osutils.py and copy_terraform_built_artifacts.py.

The custom implementations were originally needed for Python 3.7 compatibility (dirs_exist_ok was added in 3.8), but SAM CLI now requires Python 3.10+, making them unnecessary. Using the stdlib version with symlinks=True correctly preserves both file and directory symlinks without following them.

Changes:

  • osutils.copytree: replaced ~40-line manual implementation with single shutil.copytree call; removed unused errno import; replaced os.remove try/except with Path.unlink(missing_ok=True) in remove()
  • Terraform copy_terraform_built_artifacts.copytree: same replacement
  • Updated unit tests to match new implementation
  • Added integration-style tests verifying file and directory symlink preservation on disk

Mandatory Checklist

PRs will only be reviewed after checklist is complete

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bnusunny bnusunny requested a review from a team as a code owner April 23, 2026 21:23
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

This review has been superseded by a newer review.

Replace hand-rolled copytree implementations with
shutil.copytree(..., symlinks=True, dirs_exist_ok=True) in both
osutils.py and copy_terraform_built_artifacts.py.

The custom implementations were originally needed for Python 3.7
compatibility (dirs_exist_ok was added in 3.8), but SAM CLI now
requires Python 3.10+, making them unnecessary. Using the stdlib
version with symlinks=True correctly preserves both file and directory
symlinks without following them.

Changes:
- osutils.copytree: replaced ~40-line manual implementation with
  single shutil.copytree call; removed unused errno import; replaced
  os.remove try/except with Path.unlink(missing_ok=True) in remove()
- Terraform copy_terraform_built_artifacts.copytree: same replacement
- Updated unit tests to match new implementation
- Added integration-style tests verifying file and directory symlink
  preservation on disk
- Audited os.walk(followlinks=True) in package/utils.py — intentional
  and correct for zip packaging (zip files need dereferenced content)

This is a code quality / defense-in-depth improvement, not a security
fix. We do not believe this warrants a new CVE assignment.
@bnusunny bnusunny force-pushed the refactor/replace-custom-copytree-with-stdlib branch from 555d027 to 69b0424 Compare April 23, 2026 21:42
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Code Review Results

Reviewed: fdf769d..69b0424
Files: 3
Comments: 2

Comment thread samcli/lib/utils/osutils.py
Comment thread samcli/hook_packages/terraform/copy_terraform_built_artifacts.py
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.

2 participants