[Inok]

Closes #921 — AI-2737

What & why

Two pieces of the setup lane resolved themselves from process-global state. SetupCommand.FacadeOverride and ImportRunnerOverride were mutable statics existing only for tests, so a site that ignored its collaborator still passed; both now arrive injected, and ignoring one is a compile error (CS9113). ProfileOverridesGlobalSetup blanked KCAP_URL/KCAP_PROFILE for every test assembly to contain an exposure that is entirely at the spawn boundary — a child re-resolves them at its own root — so KcapProcess clears them for the child beside the two directories it already pins, and the guard retires.

Where to look

Retiring that guard un-skips nine *MemoryIndexLiveCertTests: it blanked the variable SkipUnlessLiveGateReady gates on, so they could never run. They stay gated on the live flag, and this PR does not exercise them.

Moving façade construction out leaves SetupCommand with no auth transports — four constructor parameters went unread and are gone. ChosenServerHttp is extracted so the command and the runner share one builder for the server chosen mid-run.

Verification

Every suite with a hostile KCAP_URL=https://bogus.invalid KCAP_PROFILE=bogus-profile exported — the case the retired guard existed for:

Suite	Result
Cli	4064/4064
Cli.Core	3293/3293
App	1894/1894
Integration	286/286
Cli.Daemon	3232/3233 (local vendor-pin drift, skipped in CI)

Mutations, each reverted: wrong pinned import flag → 2 failures; skip branch inverted → all 10 import tests; provisioner dropped at the factory call → 1; DI registrations removed → both Every_registered_command_resolves. Solution builds at 0 warnings; AOT publish emits no IL2026/IL3050.

SetupCommandTests goes from 14 exclusions to 8 — the 6 keyed on the import seam. The other 8 are driven by console capture and a working-directory move, which this does not touch.

[qodo-code-review]

PR Summary by Qodo

Inject setup collaborators and isolate spawned server selection

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Inject setup façade and import collaborators, replacing mutable process-global test overrides.
• Centralize HTTP construction around the server and profile selected during setup.
• Clear inherited server-selection variables only when spawning child CLI processes.

Diagram

graph TD
  DI["Command DI"] --> Setup["Setup Command"] --> FacadeFactory["Facade Factory"] --> Facade["Onboarding Facade"]
  Setup --> ImportRunner["Import Runner"] --> ChosenHttp["Chosen Server HTTP"]
  Spawn["Spawn Boundary"] -->|removes KCAP vars| Child["Child CLI Root"]
  DI --> FacadeFactory
  DI --> ImportRunner
  Setup --> ChosenHttp

Loading

High-Level Assessment

The injected collaborator approach is preferable to mutable static overrides because required dependencies become compile-time-enforced and tests can run without shared state. Clearing environment overrides at each child-process boundary is also narrower and safer than assembly-wide mutation, while the shared selected-server HTTP builder avoids duplicating sensitive profile-resolution logic.

Files changed (16) +373 / -313

Bug fix (2) +8 / -1

GuardsBootstrap.csStop installing the assembly-wide profile override guard +0/-1

Stop installing the assembly-wide profile override guard

• Removes initialization of the global KCAP_URL and KCAP_PROFILE clearing guard so in-process tests retain their intended environment.

test/Capacitor.Tests.Helpers/Guards/GuardsBootstrap.cs

KcapProcess.csClear server-selection variables at child spawn +8/-0

Clear server-selection variables at child spawn

• Removes inherited KCAP_URL and KCAP_PROFILE values when constructing child kcap processes, allowing each child composition root to resolve only explicitly supplied selection.

test/Capacitor.Tests.Helpers/KcapProcess.cs

Refactor (7) +133 / -82

ChosenServerHttp.csExtract selected-server HTTP provider construction +29/-0

Extract selected-server HTTP provider construction

• Adds a reusable builder that creates validated HTTP services using the server and profile selected during setup rather than startup-time resolution.

src/Capacitor.Cli/Commands/ChosenServerHttp.cs

IOnboardingFacadeFactory.csDefine the injected onboarding façade factory contract +12/-0

Define the injected onboarding façade factory contract

• Introduces an injectable factory for constructing onboarding façades with optional provisioning, picker, and requested-workspace collaborators.

src/Capacitor.Cli/Commands/IOnboardingFacadeFactory.cs

ISetupImportRunner.csDefine the injected setup import runner contract +9/-0

Define the injected setup import runner contract

• Introduces an abstraction for executing setup's nested import without relying on a mutable static test override.

src/Capacitor.Cli/Commands/ISetupImportRunner.cs

ImportInvocation.csPromote setup import arguments to a shared record +15/-0

Promote setup import arguments to a shared record

• Moves the import invocation model out of SetupCommand so production and fake runners can consume and inspect the same pinned arguments.

src/Capacitor.Cli/Commands/ImportInvocation.cs

SetupCommand.csInject setup façade and import collaborators +14/-82

Inject setup façade and import collaborators

• Replaces static façade and import overrides with constructor-injected interfaces. Removes unused authentication transports, delegates nested imports to the runner, and reuses selected-server HTTP construction.

src/Capacitor.Cli/Commands/SetupCommand.cs

SetupFacadeFactory.csProvide the production onboarding façade factory +20/-0

Provide the production onboarding façade factory

• Moves production OnboardingFacade construction and its authentication transports out of SetupCommand while preserving progress, picker, workspace guard, and key-watcher behavior.

src/Capacitor.Cli/Commands/SetupFacadeFactory.cs

SetupImportRunner.csProvide the production nested import runner +34/-0

Provide the production nested import runner

• Executes setup's nested import against HTTP services built for the selected server and forwards the pinned repository, visibility, exclusion, and profile settings.

src/Capacitor.Cli/Commands/SetupImportRunner.cs

Tests (6) +229 / -230

ServiceVerifyProcessTests.csSanitize server selection for shell-spawned test processes +6/-0

Sanitize server selection for shell-spawned test processes

• Removes KCAP_URL and KCAP_PROFILE from the one integration-test process launched through a shell instead of KcapProcess.

test/Capacitor.Cli.Tests.Integration/ServiceVerifyProcessTests.cs

FakeFacadeFactory.csAdd an injectable onboarding façade test factory +14/-0

Add an injectable onboarding façade test factory

• Provides tests with a locally scoped façade factory that supplies custom transports and progress sinks without process-global state.

test/Capacitor.Cli.Tests.Unit/Commands/FakeFacadeFactory.cs

FakeImportRunner.csAdd a recording setup import test runner +28/-0

Add a recording setup import test runner

• Adds configurable success, failure, and exception behavior while recording invocation arguments and call counts for setup tests.

test/Capacitor.Cli.Tests.Unit/Commands/FakeImportRunner.cs

SetupChosenServerTests.csSupply explicit setup collaborators in selected-server tests +4/-3

Supply explicit setup collaborators in selected-server tests

• Updates SetupCommand construction to provide fake façade and import collaborators after the constructor dependency change.

test/Capacitor.Cli.Tests.Unit/Commands/SetupChosenServerTests.cs

SetupCommandTests.csReplace static import seams with injected runners +135/-181

Replace static import seams with injected runners

• Refactors setup import and argument-validation tests to use FakeImportRunner and shared command construction. Removes serialization and cleanup required by the former process-global override.

test/Capacitor.Cli.Tests.Unit/Commands/SetupCommandTests.cs

SetupFacadeParityTests.csReplace static façade overrides with injected factories +42/-46

Replace static façade overrides with injected factories

• Updates discovery and login parity tests to inject per-test façade factories. Removes static override cleanup and narrows the remaining parallelism restriction to console capture.

test/Capacitor.Cli.Tests.Unit/Commands/SetupFacadeParityTests.cs

Other (1) +3 / -0

CommandServices.csRegister setup collaborators in dependency injection +3/-0

Register setup collaborators in dependency injection

• Registers the selected-server HTTP builder, onboarding façade factory, and setup import runner with the command service collection.

src/Capacitor.Cli/Commands/CommandServices.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Shell test bypasses environment scope ✗ Dismissed 📘 Rule violation ☼ Reliability

Description

ParentDeath_OrphanedChildStillReleasesTheServiceLock removes KCAP_URL and KCAP_PROFILE
directly from ProcessStartInfo.Environment instead of using EnvScope. When the shell is spawned,
those direct mutations determine the grandchild's server selection without the scope and explicit
non-parallel guard required for test environment changes.

Code

test/Capacitor.Cli.Tests.Integration/ServiceVerifyProcessTests.cs[R119-120]

+        psi.Environment.Remove(ProfileOverrides.UrlVar);
+        psi.Environment.Remove(ProfileOverrides.ProfileVar);

Relevance

●●● Strong

A close precedent accepted direct ProcessStartInfo environment cleanup for spawned integration-test
processes.

PR-#400

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2821221 requires test environment mutations, including child-process environment changes, to
use EnvScope; the cited test directly removes both variables before starting the shell.

Rule 2821221: Use EnvScope for all environment variable mutations in tests
test/Capacitor.Cli.Tests.Integration/ServiceVerifyProcessTests.cs[117-122]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The integration test directly removes two environment variables from the child process environment instead of expressing the changes through `EnvScope` with the required test isolation.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Integration/ServiceVerifyProcessTests.cs[117-120]

## Recommended Fix
Mark the test with bare `[NotInParallel]`, use disposable `EnvScope.Exclusive` instances to clear `KCAP_URL` and `KCAP_PROFILE` while constructing and starting the shell process, and remove the direct `ProcessStartInfo.Environment.Remove` calls.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. A new comment narrates old behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

ChosenServerHttp says token lookup targets the profile it always did, embedding change-history
narration instead of stating the current profile invariant. If profile resolution changes later,
readers must infer which past behavior always refers to and can preserve a stale assumption.

Code

src/Capacitor.Cli/Commands/ChosenServerHttp.cs[15]

+/// stay the process's, so the token lookup targets the profile it always did.</para>

Relevance

●●● Strong

Recent precedents accept shortening historical or rationale-heavy comments in production
documentation.

PR-#360
PR-#638
PR-#507

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2897915 prohibits historical narration in changed comments, and the added documentation uses
always did to characterize prior token lookup behavior.

Rule 2897915: Avoid time-sensitive or process-reference metadata in code comments
src/Capacitor.Cli/Commands/ChosenServerHttp.cs[12-15]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new XML documentation uses `always did` to narrate historical behavior, making its meaning dependent on prior implementations rather than the current invariant.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/ChosenServerHttp.cs[12-15]

## Recommended Fix
Rewrite the final sentence to state directly that the existing profile name and configuration root determine token lookup, without referring to how the code behaved previously.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Spawn helper skips environment scopes ✗ Dismissed 📘 Rule violation ☼ Reliability

Description

KcapProcess.StartInfo removes KCAP_URL and KCAP_PROFILE directly from every returned
ProcessStartInfo.Environment instead of using the test environment scope abstraction. Every test
process created through this helper receives those mutations without EnvScope lifecycle
enforcement or validation of the calling test's parallelism constraint.

Code

test/Capacitor.Tests.Helpers/KcapProcess.cs[R38-39]

+        psi.Environment.Remove(ProfileOverrides.UrlVar);
+        psi.Environment.Remove(ProfileOverrides.ProfileVar);

Relevance

●●● Strong

Spawned-process environment isolation is an accepted reliability fix; helper-level cleanup matches
that precedent.

PR-#400

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 2821221 explicitly covers environment mutations in test helpers, while the new helper lines
directly remove two variables from each spawned process's environment.

Rule 2821221: Use EnvScope for all environment variable mutations in tests
test/Capacitor.Tests.Helpers/KcapProcess.cs[35-41]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The shared process helper directly removes two environment variables from child launch state, bypassing the repository's required `EnvScope` abstraction and its parallelism checks.

## Fix Focus Areas
- test/Capacitor.Tests.Helpers/KcapProcess.cs[38-39]

## Recommended Fix
Remove the direct environment removals from `KcapProcess.StartInfo`; have tests that require cleared server selection create disposable `EnvScope.Exclusive` scopes for both keys before constructing and starting the process, and mark those tests with bare `[NotInParallel]`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

  Explored: repo: kurrent-io/kcap-server (sha: ec890215)

Review mode: 🧠 Deep: This is a broad, behavior-changing DI and process-boundary refactor spanning production setup flows, server selection, environment propagation, and numerous test paths, with many independent logic sites where subtle regressions could be missed in one pass.

Tip of the day

💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[realtonyyoung]

NO FINDINGS