Skip to content

wslc: add --follow-link to cp - #41501

Draft
ggarzia-MSFT wants to merge 5 commits into
masterfrom
user/ggarzia/wslc-cp-follow-link
Draft

wslc: add --follow-link to cp#41501
ggarzia-MSFT wants to merge 5 commits into
masterfrom
user/ggarzia/wslc-cp-follow-link

Conversation

@ggarzia-MSFT

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Adds --follow-link / -L to wslc container cp

Without -L the behavior is unchanged: the symlink itself is copied.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

CLI

  • ArgumentDefinitions.h gains ArgType::FollowLink (--follow-link, alias L, Kind::Flag).
  • ContainerCpCommand registers it alongside the existing --archive.

Container → local: statting a path over the Docker Engine API

Docker resolves the link with ContainerStatPath, i.e. HEAD /containers/{id}/archive?path=..., reading the X-Docker-Container-Path-Stat response header.

DockerHTTPClient cannot safely issue a HEAD today: SendRequest drives a Boost.Beast parser that would block waiting for a body that a HEAD response never sends. Rather than reshape the parser, StatArchivePath takes advantage of two existing properties of the client:

  1. GET /containers/{id}/archive returns the same X-Docker-Container-Path-Stat header as HEAD.
  2. SendRequest returns as soon as the response header has been parsed, handing the still-open socket back to the caller.

So StatArchivePath issues the GET, reads the header, and drops the socket without ever reading the tar body. Each request gets its own freshly-connected socket via SendRequestImpl, so abandoning it has no effect on subsequent calls.

The header is base64-encoded JSON, decoded with wslutil::Base64Decode into a new docker_schema::ContainerPathStat. The symlink test uses Go's os.ModeSymlink bit (1 << 27), since the mode field is a marshalled Go os.FileMode.

When the target is relative, it is joined against the parent directory of the source path — the same rebasing docker performs via archive.SplitPathDirEntry.

Interface changes

IWSLCContainer::ResolveArchiveSymlink is new. IWSLCContainer (wslc.idl) is internal and non-stable — it ships in lockstep with its only clients — so appending a method is safe. The SDK-facing IWSLCCompatContainer in WSLCCompat.idl is untouched.

Validation Steps Performed

  • Full cmake --build . — clean.

  • wslc container cp --help lists the new option:

    Options:
      -a  --archive      Archive mode (accepted for Docker CLI compatibility)
      -L  --follow-link  Always follow symlinks in SRC_PATH
      -?  --help         Shows help about the selected command
    
  • 13 new parser cases in CommandLineTestCases.h covering -L, --follow-link, =true/=false, combination with -a, the root-level wslc cp alias, and the negatives --followlink, -l (aliases are case-sensitive) and -L=invalid.

  • ContainerCpCommand_HasFollowLinkArgumentWithDockerAlias pins the name, alias, kind and optionality.

  • WSLCE2E_Container_Cp_HelpListsFollowLink asserts the flag is surfaced in help output.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 1, 2026 21:04

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.

🟡 Changes recommended

It adds new behavior without an e2e test validating -L actually follows symlinks, and it introduces at least one naming/doc inconsistency that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds --follow-link / -L support to wslc container cp to match docker cp -L semantics by resolving symlinks in the source path (both local→container and container→local) before copying.

Changes:

  • Adds a new CLI flag (ArgType::FollowLink, --follow-link, alias -L) and wires it into container cp.
  • Implements container-side symlink resolution by statting /containers/{id}/archive and decoding X-Docker-Container-Path-Stat, exposed via a new IWSLCContainer::ResolveArchiveSymlink method.
  • Adds parser/unit/e2e coverage for argument presence and help output surfacing.
File summaries
File Description
test/windows/wslc/WSLCCLICommandUnitTests.cpp Adds a unit test to pin --follow-link name/alias/kind.
test/windows/wslc/e2e/WSLCE2EContainerCpTests.cpp Adds an e2e test ensuring container cp --help lists --follow-link/-L.
test/windows/wslc/CommandLineTestCases.h Adds command-line parsing cases for -L/--follow-link (incl. boolean forms and negatives).
src/windows/wslcsession/WSLCContainer.h Adds ResolveArchiveSymlink to the container implementation and COM interface class.
src/windows/wslcsession/WSLCContainer.cpp Implements ResolveArchiveSymlink and exposes it via COM.
src/windows/wslcsession/DockerHTTPClient.h Declares StatArchivePath helper for reading X-Docker-Container-Path-Stat.
src/windows/wslcsession/DockerHTTPClient.cpp Implements StatArchivePath (GET archive, read stat header, close socket).
src/windows/wslc/tasks/ContainerTasks.cpp Adds followLink behavior for container cp in both copy directions.
src/windows/wslc/services/ContainerService.h Adds ResolveContainerSymlink service helper declaration.
src/windows/wslc/services/ContainerService.cpp Implements ResolveContainerSymlink by calling IWSLCContainer::ResolveArchiveSymlink.
src/windows/wslc/commands/ContainerCpCommand.cpp Registers ArgType::FollowLink for the container cp command.
src/windows/wslc/arguments/ArgumentDefinitions.h Defines the new argument type and localization hook.
src/windows/service/inc/wslc.idl Extends IWSLCContainer with ResolveArchiveSymlink.
src/windows/inc/docker_schema.h Adds ContainerPathStat schema for the archive stat header payload.
localization/strings/en-US/Resources.resw Adds localized help text for --follow-link.
Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/windows/inc/docker_schema.h Outdated
Comment thread src/windows/wslc/tasks/ContainerTasks.cpp Outdated
Comment thread src/windows/wslcsession/DockerHTTPClient.h Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 1, 2026 21:26

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.

🔵 Needs a closer look

DockerHTTPClient::StatArchivePath currently treats all non-200 responses (and even missing stat headers on 200) as “path not found,” which can silently mask real engine errors and cause --follow-link to behave incorrectly.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/windows/wslcsession/DockerHTTPClient.cpp:449

  • StatArchivePath() currently returns nullopt for any non-200 response, but its contract says nullopt means the path does not exist. This can silently mask real engine errors (e.g., 500/401) and cause --follow-link to fall back to copying the symlink instead of failing fast.

This issue also appears on line 451 of the same file.

src/windows/wslcsession/DockerHTTPClient.cpp:455

  • On a 200 response, a missing X-Docker-Container-Path-Stat header indicates an unexpected daemon/proxy behavior and should not be treated the same as "path does not exist"; otherwise --follow-link can silently do the wrong thing.
    const auto header = response["X-Docker-Container-Path-Stat"];
    if (header.empty())
    {
        return std::nullopt;
    }
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 2, 2026 01:28

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.

🟡 Changes recommended

DockerHTTPClient::StatArchivePath currently treats all non-200 responses (and missing stat headers) as “not found,” which can mask real engine/protocol failures and should be tightened to surface errors correctly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/windows/wslcsession/DockerHTTPClient.cpp:455

  • If the response is 200 but missing the X-Docker-Container-Path-Stat header, returning nullopt will make the caller treat it as "not a symlink" and proceed, which can mask a protocol/engine mismatch. It should be treated as an unexpected error so callers can fail fast with a clear diagnostic.
    const auto header = response["X-Docker-Container-Path-Stat"];
    if (header.empty())
    {
        return std::nullopt;
    }

src/windows/wslcsession/DockerHTTPClient.h:150

  • The comment for StatArchivePath says it returns nullopt when the path does not exist, but the implementation also treats other non-200 responses (and a missing stat header) as nullopt. Update the comment to reflect the actual contract (e.g., 404 => nullopt, other errors => exception) to avoid misleading future callers.
    // Reads the X-Docker-Container-Path-Stat header for a container path. Returns nullopt if the path does not exist.
    std::optional<common::docker_schema::ContainerPathStat> StatArchivePath(const std::string& ContainerID, const std::string& Path);
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/windows/wslcsession/DockerHTTPClient.cpp Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 2, 2026 18:54

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.

🔵 Needs a closer look

There is a confirmed path-handling bug in the container cp local→container flow where stripping a trailing separator can turn a drive-root parent directory (e.g. C:\) into C:, altering tar.exe -C behavior and potentially breaking copies from drive roots.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/windows/wslc/tasks/ContainerTasks.cpp:387

  • When SRC_PATH is at a drive root (e.g. "C:\file"), absPath.parent_path() becomes "C:\". The subsequent trailing-separator trimming in this block turns that into "C:", which changes tar.exe -C semantics ("C:" means the current directory on that drive) and can break copies from the drive root. Preserve root paths when stripping trailing separators.
    src/windows/wslc/services/ContainerService.h:68
  • This comment says ResolveContainerSymlink returns nullopt only when the path is not a symlink, but the underlying service method also returns nullopt when the path does not exist. Update the comment to match the actual behavior.
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 2, 2026 20:51

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.

🔵 Needs a closer look

The new container-side symlink resolution currently holds a shared lock across network I/O, and the new local→container follow-link behavior lacks corresponding automated coverage.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/windows/wslc/tasks/ContainerTasks.cpp:384

  • The new --follow-link behavior for local → container relies on std::filesystem::canonical() to resolve the source path before archiving. There is E2E coverage for container → local symlink following, but no test coverage for this local → container path-resolution behavior, so regressions here would be easy to miss.
    src/windows/wslcsession/WSLCContainer.cpp:2688
  • ResolveArchiveSymlink holds the container shared lock while issuing a Docker API request (StatArchivePath). This can block other container operations longer than necessary; other archive operations (e.g., DownloadArchive/UploadArchive) release the lock before doing potentially long I/O.
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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.

2 participants