Skip to content

fix(start): resolve the launch cwd through the single transport-aware owner - #151

Merged
YoungSx merged 3 commits into
mainfrom
fix/chatlaunch-workflow-no-project-session
Sep 1, 2026
Merged

fix(start): resolve the launch cwd through the single transport-aware owner#151
YoungSx merged 3 commits into
mainfrom
fix/chatlaunch-workflow-no-project-session

Conversation

@YoungSx

@YoungSx YoungSx commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem

Starting a session with the Unclassified project always failed. Production telemetry (prod / salmonegg, 7 days, 4 events from 1 install on 1.4.2.0 Windows):

Start workflow stopped: no working directory resolved for the launch.
library.name: SalmonEgg.Presentation.Core.Services.Chat.ChatLaunchWorkflow

Two owners resolved one value, and they disagreed:

  • Launch used ProjectSelectionCwdResolver, which returns null for __unclassified__ because it is not a real project. ChatLaunchWorkflow then rejected the launch.
  • New-session draft used AcpSessionNewCwdResolver, which owns the transport-aware fallback: stdio defaults to the user profile directory, remote transports require an explicitly selected directory.

So the draft succeeded via that fallback while the launch failed on the same selection. The project selector also treats Unclassified as a legal choice, so the submit button stayed enabled — and the failure surfaced as the generic "Failed to start the session. Please try again," which no retry could ever fix.

Change

  • Route the launch cwd through AcpSessionNewCwdResolver, reusing ResolvePreviewCwd() as the same request source the draft already uses. One resolver, one request source.
  • Surface the resolver's own reason (for example "select a remote directory") instead of the generic retry message when a launch has no resolvable root. Added NewSessionDraft_MissingRemoteCwd across all four .resx.
  • Deleted SessionCwdResolver: its only remaining call site passed a constant null second argument, leaving it a Trim wrapper masquerading as an architecture layer.
  • Kept ChatLaunchWorkflow's own guard as defense in depth (the interface is public and ChatLaunchRequest.Cwd is nullable) and corrected its now-stale comment.

Tests

Added 4 behavior tests plus 1 fail-closed architecture gate:

Test Contract
StartSession_WhenUnclassifiedOnStdioProfile_LaunchesWithTransportFallbackCwd the reported bug: Unclassified + stdio now launches with the fallback root
StartSession_WhenLocalProjectSelected_LaunchesWithConfiguredProjectRoot a configured project still wins over the fallback
StartSession_WhenRemoteProfileHasNoSelectedDirectory_ReportsMissingCwdWithoutInvokingWorkflow the selector门禁 already blocks submit; the launch never reaches the workflow
StartSession_WhenRemoteDirectoryPathIsRelative_ReportsResolverReasonWithoutInvokingWorkflow reaches the new guard and asserts the resolver's own reason
NavigationCoreTests.StartLaunch_ResolvesCwdThroughTheTransportAwareResolverOnly fail-closed: the deleted resolver must not return

StartSessionAndSendAsync_LocalProfile_DoesNotLeakPersistedRemoteDirectoryCwd asserted Assert.Null(cwd) — it had encoded the defect as the contract. Its real intent (a local launch must not leak the persisted remote path) is preserved and strengthened to assert the stdio fallback root instead.

Verification

  • Presentation.Core suite: 3276 passed, 0 failed, 0 warnings, 0 errors.
  • Reverse verified the fix: reintroducing the null-cwd path turns 2 new tests red.
  • Reverse verified the gate: reverting StartViewModel alone turns the gate red.
  • Also audited every pre-existing test that reaches the launch path. Seven had no selected profile, so they returned at the new guard and hung forever on await workflowStarted.Task — the suite timed out with no failure output at all while the new tests alone passed in 0.6s. Fixed via a shared SelectStdioProfile helper; that failure mode is now written into AGENTS.md.

Merge

Branch is based exactly on origin/develop (0 behind, 2 ahead), so rebase merge applies cleanly with no conflicts.

🤖 Generated with Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@vercel

vercel Bot commented Sep 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
salmonegg Ready Ready Preview Sep 1, 2026 10:34am UTC

@YoungSx

YoungSx commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

All 14 gates now pass (1 skipping), mergeStateStatus: CLEAN.

For the record: Build and Test went red twice before going green, on TelemetryManagerTests.Reconfigure_UsesConfiguredNormalSamplingRate — a telemetry sampling test. That is a pre-existing Windows-only flake, not this PR:

  • Same commit 38be7818, same gate: fail, fail, pass.
  • b88b7c69 (this PR, one commit earlier, carrying all the logic changes) passed; 38be7818 adds only a 4-character whitespace fix. No causal path from whitespace to telemetry sampling.
  • This PR touches only SalmonEgg.Presentation.Core and AGENTS.md — zero Infrastructure/observability files.
  • Locally on Linux: that class 13 passed, the full Infrastructure assembly 709 passed, and the suspected ordering (RuntimeInstrumentationTestsTelemetryManagerTestsErrorBiasedSamplingTests) 21 passed.

Tracked separately in #152 with the evidence and the ruled-out hypotheses, rather than fixed here — it is unrelated to this change and would widen the diff.

YoungSx and others added 3 commits September 1, 2026 10:18
… owner

Starting a session with the Unclassified project always failed: StartViewModel
resolved the launch cwd via ProjectSelectionCwdResolver, which returns null for
Unclassified because it is not a real project, and ChatLaunchWorkflow then
rejected the launch with "no working directory resolved for the launch". The
generic "please try again" message was misleading — no retry could ever produce
a root.

Meanwhile the new-session draft resolved its cwd through
AcpSessionNewCwdResolver, which owns the transport-aware fallback (stdio
defaults to the user profile directory, remote transports require an explicitly
selected directory). So the draft succeeded via that fallback while the launch
failed on the same selection: two owners for one value.

Route the launch through AcpSessionNewCwdResolver as well, reusing
ResolvePreviewCwd as the request source the draft already uses, and surface the
resolver's own reason instead of the generic retry message when a launch has no
resolvable root. Delete SessionCwdResolver, whose only remaining call site
passed a constant null second argument, leaving it a Trim wrapper.

Verified: Presentation.Core suite 3276 passed, 0 failed, 0 warnings. Reverse
verified by reintroducing the null-cwd path (two new tests fail) and by reverting
StartViewModel against the new architecture gate (gate fails).

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…on rule

Documentation-only change; no tests run (docs-only per AGENTS section 5.4).

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Inserting the gate left the following [Fact] indented by 8 spaces and added a
stray blank line, which failed the Code Quality formatting gate.

Verified with the gate's own command (dotnet format --verify-no-changes over the
six touched files) and by rerunning NavigationCoreTests + StartViewModelTests:
153 passed, 0 failed.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@YoungSx
YoungSx force-pushed the fix/chatlaunch-workflow-no-project-session branch from 38be781 to e61a1fe Compare September 1, 2026 10:27
@YoungSx
YoungSx changed the base branch from develop to main September 1, 2026 10:27
@YoungSx

YoungSx commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Retargeted from develop to main and rebased onto main.

Why: develop is currently 8 commits behind main and 0 aheadmain already contains everything on develop, and the recent PRs (#150, #141, #140) merged straight into main. Targeting develop would have parked this fix on a branch that is strictly behind the release line.

Rebase is clean and content-preserving:

  • Zero conflicts onto main.
  • The resulting diff is byte-identical to the develop-based one except for four resx blob hashes — main added 9 acp-setup keys to each CoreStrings*.resx, and the new NewSessionDraft_MissingRemoteCwd key lands alongside them with no name collision (verified: exactly one occurrence per file).
  • 0 3 against origin/main, so rebase merge stays linear.

Re-verified on the main baseline (not just carried over from the develop run):

  • Build: 0 errors.
  • dotnet format --verify-no-changes over the six touched files: passes.
  • Presentation.Core suite: 3277 passed, 0 failed (one more than on the develop baseline, from the 8 commits main carries).

CI is re-running against the new base.

@YoungSx
YoungSx merged commit c33da7d into main Sep 1, 2026
15 checks passed
@YoungSx
YoungSx deleted the fix/chatlaunch-workflow-no-project-session branch September 1, 2026 10:49
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.

1 participant