Repository navigation
Expose the working directory in target snapshots - #882
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Transfer output still reports a hard-coded root that can conflict with the new snapshot value.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Exposes the target’s transfer working directory in snapshots without changing transfer behavior.
Changes:
- Adds JSON
workRootand textWork rootoutput. - Omits the field when unavailable.
- Adds regression tests and updates Sandbox guidance.
File summaries
| File | Description |
|---|---|
| src/winapp-CLI/WinApp.Cli/Commands/TargetSnapshotCommand.cs | Resolves and displays the work root. |
| src/winapp-CLI/WinApp.Cli.Tests/TargetCaptureCommandTests.cs | Tests output, non-default roots, and omission behavior. |
| docs/sandbox-execution.md | Documents snapshot and transfer-path semantics. |
Review details
Suppressed comments (1)
docs/sandbox-execution.md:292
- What is wrong: The new target-specific contract is not applied consistently across the shipped instructions. This guide's
--cwd C:\WinApp\work\Setupexample,docs/usage.md:1595, andplugins/winapp/skills/winapp-sandbox/SKILL.md:118,126still assume the default root.
Show me: If a snapshot reports workRoot = D:\Managed\work, those examples direct users or agents to C:\WinApp\work instead.
Why it matters: Follow-up commands can look in the wrong directory after a successful transfer.
Smallest fix: Update the hand-authored examples and references to derive --cwd from the snapshot's workRoot, retaining C:\WinApp\work only as the normal default.
For `push` and `pull`, **target paths are relative to the `workRoot` reported by
[`target snapshot`](#inspecting-the-sandbox)**. Absolute,
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The scoped implementation reuses authoritative path resolution and has focused regression coverage.
Review details
Suppressed comments (1)
docs/sandbox-execution.md:292
- What is wrong: The new guidance treats the snapshot's
workRootas authoritative, but the example immediately above still uses--cwd C:\WinApp\work\Setup. The command reference and shipped Sandbox skill also still describeC:\WinApp\workas fixed (docs/usage.md:1595andplugins/winapp/skills/winapp-sandbox/SKILL.md:118,126).
Show me: If the guest reports workRoot = D:\Managed\work, following these instructions runs the copied script from C:\WinApp\work\Setup instead of D:\Managed\work\Setup.
Why it matters: Users and agents can select a nonexistent working directory despite the snapshot exposing the correct base.
Smallest fix: Update the example and linked hand-authored guidance to derive --cwd from the reported workRoot; keep C:\WinApp\work only as the default example.
For `push` and `pull`, **target paths are relative to the `workRoot` reported by
[`target snapshot`](#inspecting-the-sandbox)**. Absolute,
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation uses authoritative guest path resolution with focused regression coverage and updated documentation.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Build Metrics ReportBinary Sizes
Test Results✅ 7611 passed, 37 skipped out of 7648 tests in 1311.1s (+13 tests, +118.9s vs. baseline) Test Coverage✅ 85.8% line coverage, 79.9% branch coverage · ✅ no change vs. baseline CLI Startup Time62ms median (x64, Try This BuildInstalls the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing. & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 882Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 882Run Updated 2026-09-19 22:37:10 UTC · commit |
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
Review fully clean. Approving.
Summary
Closes #879.
workRoot(text:Work root) separately fromcapabilities.managedRoot, usingGuestPaths.Resolveand the existing transferWorkScope. Missing guest/root metadata stays omitted; snapshot remains read-only.C:\WinApp\work. Resolve before copying, so missing-root metadata fails before writes. Copy destinations, accepted relative paths, and target lifecycle are unchanged.--cwd.Validation
.and default-root output are covered too.28306ce829ff950c1b692bcb096f44989fdb75f9: snapshot/capture-command fakes, shared guest paths, parser/path acceptance, path safety, source/link handling, and guest file services.scripts\build-cli.ps1 -SkipTestspassed both NativeAOT architectures, npm format/lint/compile/package, four NuGet packages, both MSIX packages and generated docs.scripts\validate-llm-docs.ps1passed including plugin conformance. Generated schema/manifests unchanged. Earlier iterations also passednpm run buildand-OnlyDocs.-SkipTeststo avoid uncoordinated live UI. Repository CI subsequently passed: CLI 6,632 passed / 0 failed / 35 skipped, UIAutomation 875 passed / 0 failed / 2 skipped, analyzer 65 passed, plus the separate UI E2E/coordination job and all 14 sample jobs.298df24bad11308be8e0523c6f58e69c69ec0394passed a coordinated live default-root snapshot/path check:workRoot=C:\WinApp\workmatched an installed-transport unique upload's destination/content; exact guest/host probes cleaned, epoch/readiness unchanged. No UI/lifecycle/guest-CLI change. This does not claim live coverage of the later formatter correction.WINAPP_TARGET_STATE_ROOTselector for the installed package's physical ownership store. The unconfigured developer build correctly reported its obsolete unpackaged record as stale. No state/credentials copied or modified; installed transfer environment unchanged. Installed CLI version was not explicitly recorded.301423ADFB580FF189307B6E31FCE50BA099F0AB9788BA350A2C6DAAF0CBC271. Current built ARM64 SHA256:F975D2D433A6B39943029BEE86CBF0FAC98E00C40E846DB43770829D152DDE38.CI retry disclosure
build-and-packageattempt 1 failed in the existingQueryOptions_RealApp_DelayedRootAndReadCommandsreplacement-provider wait (exit 1; 17.482s). That test and its UI implementation are unchanged; it passed on the exact base commit in 11.674s. The failure artifacts omit child stdout, so a timeout/provider-timing explanation is consistent with the evidence but not proven.One approved failed-job rerun on the unchanged head succeeded: Build and Package attempt 2. The same test passed in 11.635s. First-failure, baseline and rerun TRX evidence was preserved. No assertions were weakened, no unrelated code changed, and no further retries were needed.
Ready for human review
Verified head:
28306ce829ff950c1b692bcb096f44989fdb75f9; based on current main09a401bc9877625a1e7b2b915fea09833bafbb05.Required build/package and all relevant build, sample, UI E2E, docs and language-analysis checks are successful. The aggregate CodeQL check is neutral; underlying analyses succeeded. Copilot's latest-head review recommends approval, reviewed 10/10 files and generated no new comments. All 11 review threads are resolved: actionable findings fixed, fixed-relative-literal test reports explained. Human approval is still required. No merge requested.