Skip to content

Import against the server this run resolved, and relinquish on any exit - #765

Merged
George-Payne merged 3 commits into
mainfrom
georgepayne/ai-2482-first-run-abort-path
Sep 4, 2026
Merged

George-Payne merged 3 commits into
mainfrom
georgepayne/ai-2482-first-run-abort-path

Conversation

@George-Payne

Copy link
Copy Markdown
Member

AI-2482, AI-2483 — no GitHub issue; both were found in-house and filed in Linear only.

What & why

kcap setup's browser leg imports nothing on a first run, and then ends the process without telling the browser, which holds its step until the flow's twelve-hour lifetime expires.

The lane is handed profiles, the context resolved before Step 1, and ImportCommand reads its base URL from whatever context it gets. On a first run that names no server, and setup is one of the commands exempt from the entry gate that refuses an unconfigured command — so nothing between the two catches it. It now gets a context carrying the URL this run resolved, which is what the flow client one call earlier already used.

The empty URL then reaches EnsureAbsolute, which under FailFast writes a hint and calls Environment.Exit(2). That runs no finally and disposes nothing, so the relinquish notice the leg armed is never sent. Environment.Exit does raise AppDomain.ProcessExit, and this file already leans on that to flush telemetry, so the notice now goes out from there too — covering every Environment.Exit in the CLI, not just this one.

Where to look

The notice is claimed once, so a leg that settled normally sends nothing on the way out and a process with none armed is a no-op. It goes ahead of the telemetry flush because both spend the runtime's single exit budget and only one of them is holding a page open.

Making the exit itself catchable is the wider change, tracked separately as AI-2486; it needs an audit of every broad catch on a request path and is deliberately not here.

Verification

Reproduced the exit in the devcontainer with a schemeless KCAP_URL, matching the reported run on all three counts:

server_url is missing a scheme. Run: kcap config set server_url https://<host>
exit=2
crash log: none

Both guards were confirmed to fail without their fix:

Guard Without the fix
SetupImportLaneConstructionGuardTests SetupCommand.cs:1502 hands the lane a context that need not name a server
FirstRunExitNoticeGuardTests Expected to contain "FirstRunInterruptRelinquish.RunBeforeExit"

Capacitor.Cli.Tests.Unit 3984 and Capacitor.Cli.Core.Tests.Unit 2953, 0 failed. dotnet publish -c Release clean of IL2026/IL3050.

The context handed to the browser import lane is the snapshot taken before
Step 1, and setup is exempt from the entry gate that refuses an unconfigured
command — so on a first run nothing between the two catches an empty URL.
Environment.Exit runs no finally and disposes nothing, so a leg that ends
that way never sends the notice it armed and the browser waits out the
flow's whole lifetime. ProcessExit is the one hook Environment.Exit raises.
@George-Payne George-Payne self-assigned this Sep 4, 2026
@linear-code

linear-code Bot commented Sep 4, 2026

Copy link
Copy Markdown

AI-2482

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix first-run import routing and exit relinquish

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Routes first-run imports through the server URL resolved during current setup.
• Sends pending browser-flow relinquish notices before any process exit.
• Adds unit and source guards for context propagation and exit cleanup.
Diagram

graph TD
  Setup["Setup command"] -->|resolves URL| Context["Import context"] -->|constructs| Lane["Import lane"] -->|imports| Server["First-run server"]
  Setup -->|starts| Browser["Browser flow"] -->|arms| Notice["Pending notice"] -->|relinquishes| Server
  Exit["Process exit"] -->|claims once| Notice
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Persist the server before browser setup
  • ➕ Reuses normal profile resolution without creating a derived context.
  • ➕ Makes subsequent setup operations observe the selected server automatically.
  • ➖ Persists partial configuration before the browser leg completes.
  • ➖ Changes setup transaction semantics and may leave state after failure or cancellation.
2. Pass the server URL directly to imports
  • ➕ Makes the import endpoint an explicit dependency.
  • ➕ Avoids cloning the profile resolution solely to replace one field.
  • ➖ Requires broader changes to SetupImportLane and ImportCommand contracts.
  • ➖ Risks introducing a second URL-resolution path for normal imports.
3. Replace hard exits with structured cancellation
  • ➕ Allows finally blocks and disposals to perform cleanup naturally.
  • ➕ Improves testability of failure and interruption paths.
  • ➖ Requires auditing broad catch blocks and all Environment.Exit call sites.
  • ➖ Substantially expands scope and regression risk beyond these two defects.

Recommendation: Keep the PR's narrow approach. Deriving an import-only context preserves delayed profile persistence, while the existing ProcessExit hook is the smallest global safeguard for Environment.Exit paths. Structured cancellation is preferable long term but belongs in the separately tracked exit-path audit.

Files changed (6) +131 / -2

Bug fix (3) +18 / -2
SetupCommand.csRoute setup imports through the current server +10/-1

Route setup imports through the current server

• Adds an import-specific ProfileContext that preserves the selected profile and snapshot while replacing the server URL. The browser import lane now receives the server resolved during the current setup run rather than the pre-setup snapshot.

src/Capacitor.Cli/Commands/SetupCommand.cs

InteractiveLifetime.csExpose the bounded exit-notice budget +1/-1

Expose the bounded exit-notice budget

• Makes the one-second exit notice budget internally accessible so the process-wide exit handler and interrupt paths use the same bound.

src/Capacitor.Cli/InteractiveLifetime.cs

Program.csRelinquish pending browser flows during process exit +7/-0

Relinquish pending browser flows during process exit

• Invokes FirstRunInterruptRelinquish before telemetry flushing in the ProcessExit handler. This covers Environment.Exit paths that bypass finally blocks and disposal while retaining claim-once behavior.

src/Capacitor.Cli/Program.cs

Tests (3) +113 / -0
SetupImportLaneConstructionGuardTests.csGuard import-lane context construction +44/-0

Guard import-lane context construction

• Adds a source-level guard that locates every SetupImportLane construction and requires it to use ImportContext. The non-empty assertion prevents renames from making the scan pass accidentally.

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

SetupImportLaneTests.csVerify import context server replacement +30/-0

Verify import context server replacement

• Tests that ImportContext applies the current run's server URL while retaining the profile name, profile instance, and configuration snapshot.

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

FirstRunExitNoticeGuardTests.csGuard process-exit relinquish handling +39/-0

Guard process-exit relinquish handling

• Adds a source-level test that locates the ProcessExit handler and verifies it invokes FirstRunInterruptRelinquish.RunBeforeExit, preventing silent browser waits after hard exits.

test/Capacitor.Cli.Tests.Unit/FirstRunExitNoticeGuardTests.cs

@qodo-code-review

qodo-code-review Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Comments depend on Step 1 ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
New comments identify the relevant state using the flow coordinate Step 1, whose meaning can drift
if the flow is reordered. Describe the concrete server-resolution point instead of relying on
numbered-step metadata.
Code

src/Capacitor.Cli/Commands/SetupCommand.cs[1447]

+    /// context it is handed, and <c>profiles</c> is the snapshot resolved before Step 1 — on a first run
Relevance

●●● Strong

Recent SetupCommand precedents accept removing drift-prone process-coordinate and narrative comments
under comment-minimality rules.

PR-#640
PR-#675

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2897915 prohibits task, phase, scenario, and similar process coordinates in
comments. The cited new comments identify the snapshot boundary as occurring before Step 1 rather
than naming the stable behavior or operation directly.

Rule 2897915: Avoid time-sensitive or process-reference metadata in code comments
src/Capacitor.Cli/Commands/SetupCommand.cs[1445-1450]
test/Capacitor.Cli.Tests.Unit/Commands/SetupImportLaneConstructionGuardTests.cs[6-11]
test/Capacitor.Cli.Tests.Unit/Commands/SetupImportLaneTests.cs[418-423]

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 comments depend on the numbered flow coordinate `Step 1`, which is process-reference metadata that can become inaccurate when the flow changes.

## Issue Context
Preserve the behavioral rationale, but identify the concrete operation or state boundary—for example, that the profile snapshot predates server selection—without referencing a numbered step.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/SetupCommand.cs[1445-1450]
- test/Capacitor.Cli.Tests.Unit/Commands/SetupImportLaneConstructionGuardTests.cs[6-11]
- test/Capacitor.Cli.Tests.Unit/Commands/SetupImportLaneTests.cs[418-423]

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


Grey Divider

Context sources
✅ Compliance rules (platform): 56 rules
Review mode: ⚖️ Balanced: This changes first-run setup context resolution and global process-exit cleanup in runtime CLI paths, with meaningful lifecycle and remote-flow impact, but the localized logic does not warrant redundant extended passes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli/Commands/SetupCommand.cs Outdated
A step number drifts if the flow is reordered; what the context is missing
is a server, and the point it is missing one at is process start.
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