Isolate AppHost launches in linked git worktrees. - #19384
Isolate AppHost launches in linked git worktrees.#19384Shaurya Srivastava (Shaurya2k06) wants to merge 63 commits into
Conversation
Linked worktrees used to start a second AppHost on the same ports and secrets, and aspire stop could target the nested checkout from the primary tree. Default to --isolated in linked worktrees and keep stop scoped to the current worktree. Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19384Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19384" |
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
I left four inline comments. The big two are that worktree inference is getting applied to deploy/publish/do, and the false opt-out is lost in both the extension and detached CLI path. I also reproduced the Git layout parser missing bare/separate-git-dir worktrees and treating a linked-worktree submodule as its own worktree.
Targeted CLI tests passed (66), as did the extension unit suite (1,910). I wasn't able to get a live AppHost running within the review budget, so randomized ports/user-secret isolation itself is still unverified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d # Conflicts: # extension/src/lm/appHostLifecycleTools.ts # extension/src/services/AppHostLaunchService.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5647184b-a0e8-42a7-b6a7-654c5b956b8d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
extension/src/utils/cliPath.ts:517
- A bare configured command name is still treated as an exact CLI pin, but it is not stable across the probe and launch working directories. For example, with a relative
PATHentry, probingaspirefrom the extension host can inspect one binary whileAspireDebugSessionlater spawnsaspirefrom the AppHost directory and runs another; on Windows, a bare name that resolves toaspire.cmdalso bypasses the.cmdwrapper because the returned value has no shim extension. Resolve bare names throughfindCliOnPathand retain the resulting concrete path before capability negotiation and launch.
extension/src/lm/appHostLifecycleToolContracts.ts:189 - This contradicts the PR's stated tool contract that “every start result includes the effective
isolatedvalue.” The implementation only adds it when the service knows a value (currently a newly started launch), while idempotent/other start outcomes omit it. Either make the result contract supply the value for every start-tool outcome or update the PR description to document the narrower contract.
if (tool === aspireAppHostStartToolName && isolated !== undefined) {
result.isolated = isolated;
}
- Files reviewed: 84/84 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 466da694-ac7b-4ade-b31d-87ab0b2f01d5
| Assert.Equal(CliExitCodes.Success, exitCode); | ||
| var expectedPath = Path.Combine("AppHost", "AppHost.csproj"); | ||
| Assert.Contains(statusMessages, message => message == string.Format(CultureInfo.CurrentCulture, StopCommandStrings.StoppingAppHost, expectedPath)); | ||
| Assert.DoesNotContain(GetDisplayedText(interactionService, statusMessages), message => message.Contains(".worktrees", StringComparison.Ordinal)); |
Description
Linked git worktrees used to start a second AppHost on the same ports and user secrets as the primary checkout, because isolation was opt-in and socket matching is path-based. Running
aspire stopwithout--apphostfrom the primary tree could also stop an AppHost in a nested linked worktree (for examplerepo/.worktrees/feature), because scope was based on path containment alone.This change makes AppHost lifecycle worktree-aware:
aspire start,aspire run, VS Code Run/Debug, andaspire_apphost_start): linked worktrees default to isolated mode (--isolated). Primary checkout behavior is unchanged. Explicit--isolatedstill forces isolation on; the LM tool accepts optionalisolated: true|falseto override inference.aspire stopwithout--apphost): only AppHosts in the current worktree are in scope. Nested linked worktrees are no longer stopped from the primary checkout.--apphostand--allare unchanged.aspire startoraspire rundelegates a launch to VS Code, explicitly supplied CLI options and AppHost arguments after--now reach the final AppHost process without flattening token boundaries. This preserves spaces, quotes, empty arguments, and option-shaped values while keeping forwarded values out of logs.Worktree detection is filesystem-only (no
gitspawn): a linked worktree has a.gitfile whosegitdir:line points at.git/worktrees/<name>. Submodule.gitfiles (.git/modules/) are not treated as linked worktrees.Fixes #19357
Why is this PR 83 files?
The original worktree-isolation change had to cover every AppHost launch path, including launches delegated from the CLI to VS Code. End-to-end testing of that path exposed a broader existing problem: arguments could be dropped or flattened as they crossed CLI parsing, the extension backchannel, debug configuration, command shims, and the final process spawn. Since
--isolatedis itself one of those arguments, fixing isolation reliably required fixing the general forwarding path rather than special-casing one flag.The diff therefore follows one launch pipeline across both the CLI and extension. It also adds the cross-platform process inspection, command-shim handling, process-tree cleanup, capability negotiation, reservation-race coverage, and unit/E2E tests needed to verify the actual final argv on Windows, macOS, and Linux.
Most of the size is coverage and test infrastructure, not 83 separate product changes: 57 extension files (including its unit and E2E suites), 12 CLI test files, 13 CLI source files, and one documentation file.
User-facing usage
For the VS Code language-model tool
aspire_apphost_start:isolatedto auto-isolate in a linked worktree.isolated: trueorisolated: falseto override.isolatedvalue. Idempotent or otherwise uncertain results omit it because the extension cannot verify the existing launch mode.Checklist