Skip to content

A-1270 part 1: move zstash into agent repo#3953

Merged
zhming0 merged 3 commits into
mainfrom
ming/a-1270-part-1
May 26, 2026
Merged

A-1270 part 1: move zstash into agent repo#3953
zhming0 merged 3 commits into
mainfrom
ming/a-1270-part-1

Conversation

@zhming0
Copy link
Copy Markdown
Contributor

@zhming0 zhming0 commented May 26, 2026

Description

Mechanically copy zstash main head into buildkite-agent repo.

It's largely an exact copy, but I added two extra commits:

  1. all require/assert are removed to comply with repo's policy.
  2. fixes various test cases and code so it passes CI on Windows.

Context

part of A-1270.
(I plan to do more renaming and refactor dance afterwards)

Disclosures / Credits

My LLM minion

@zhming0 zhming0 requested a review from wolfeidau May 26, 2026 03:46
@zhming0 zhming0 requested review from a team as code owners May 26, 2026 03:46
@zhming0 zhming0 added the internal Non-user facing, internal change. label May 26, 2026
@zhming0 zhming0 force-pushed the ming/a-1270-part-1 branch from ee7db31 to f5d81a8 Compare May 26, 2026 03:57
@zhming0 zhming0 force-pushed the ming/a-1270-part-1 branch 3 times, most recently from e0ea7ee to 1da81ee Compare May 26, 2026 04:32
@zhming0 zhming0 force-pushed the ming/a-1270-part-1 branch from 1da81ee to f2b6859 Compare May 26, 2026 04:42
@zhming0
Copy link
Copy Markdown
Contributor Author

zhming0 commented May 26, 2026

@buildsworth-bk review please this is just largely a move file from one place to another change. We don't have production usage for this command, so the risk level should be low.

Copy link
Copy Markdown

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Choose a reason for hiding this comment

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

Read this as what it says on the tin: a mechanical copy of buildkite/zstash into internal/zstash, plus the testify-removal and Windows-CI commits. Spot-checked the non-mechanical bits and they look reasonable:

  • internal/cache/cache.go and internal/cache/cache_test.go are the only agent-side import rewrites needed; grepped for github.com/buildkite/zstash and there are no other references.
  • go.mod correctly promotes wolfeidau/quickzip from indirect to direct (it's imported by internal/zstash/archive/{create,extract}.go) and drops the buildkite/zstash dep.
  • The Windows fixes in commit f2b68597 are well-scoped: filepath.ToSlash on the zip-prefix comparison in archive/extract.go, separating chmod-walk and RemoveAll in restore.go so the os.Root handle is closed before the remove (needed on Windows), the file:///C:/... path correction in store/file.go, and allowing \ through validateFilePath on Windows. The latter is fine because runCommand uses exec.Command directly without a shell, so the validator is defense-in-depth rather than the actual injection boundary.
  • Not flagging anything in the moved zstash code itself: per the PR description that's an exact upstream copy and out of scope here. The follow-up renaming/refactor PRs will be the place for that.

No risk: label so I'm not approving, but I have no blocking concerns with the move.

Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 235, then answer my questions about the findings.

@zhming0
Copy link
Copy Markdown
Contributor Author

zhming0 commented May 26, 2026

@buildsworth-bk I gave it a risk:l1 label.

Copy link
Copy Markdown
Contributor

@buildkate buildkate left a comment

Choose a reason for hiding this comment

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

lgtm! 🚢

@zhming0 zhming0 merged commit 0e059a8 into main May 26, 2026
5 checks passed
@zhming0 zhming0 deleted the ming/a-1270-part-1 branch May 26, 2026 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Non-user facing, internal change. risk:l1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants