Skip to content

Add configurable custom CLI launcher support#579

Open
PureWeen wants to merge 3 commits intomainfrom
custom-cli-launcher-path
Open

Add configurable custom CLI launcher support#579
PureWeen wants to merge 3 commits intomainfrom
custom-cli-launcher-path

Conversation

@PureWeen
Copy link
Copy Markdown
Owner

@PureWeen PureWeen commented Apr 9, 2026

Summary

  • add a Custom CLI source with a user-specified executable path and optional launcher arguments
  • use the configured launcher for embedded sessions, persistent server startup, login commands, and model probing
  • add regression coverage for settings serialization, launch argument construction, and provider mapping

Validation

  • dotnet test PolyPilot.Tests/PolyPilot.Tests.csproj --filter "FullyQualifiedNameCliPathResolutionTests|FullyQualifiedNameConnectionSettingsTests|FullyQualifiedNameSettingsRegistryTests|FullyQualifiedNameServerManagerTests|FullyQualifiedNameProviderPluginTests|FullyQualifiedNameProtocolVersionMismatchTests|FullyQualifiedName~ServerRecoveryTests" --nologo
  • dotnet build PolyPilot.slnx -c Debug --nologo

Let users choose a custom CLI executable and optional launcher arguments for embedded and persistent modes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen
Copy link
Copy Markdown
Owner Author

PureWeen commented Apr 9, 2026

PR Review

CI status: ⚠️ No GitHub checks are currently reported on this branch. The PR description includes local dotnet test + dotnet build validation, but there isn't a branch CI signal to rely on yet.

Existing review feedback: No prior review comments were present, so nothing is duplicated here.

🟡 MODERATE

  1. Backslashes in custom launcher arguments are being stripped
    File: PolyPilot/Models/ConnectionSettings.cs (around SplitCommandLineArguments, lines 317-379)
    Consensus: All 3 reviewers agree after adversarial follow-up.

    SplitCommandLineArguments() currently treats every \ as an escape character outside single quotes:

    if (ch == '\\' && quote != '\'')
    {
        escaping = true;
        continue;
    }

    That silently corrupts path-bearing custom arguments such as:

    • --config C:\Users\me\.copilot\config.json
    • --config "C:\Program Files\Git\bin\bash.exe"

    by dropping the backslashes before the arguments ever reach ProcessStartInfo.ArgumentList. The result is a mangled path like C:Usersme.copilotconfig.json, which makes the new custom-launcher feature fail in exactly the scenario where users are most likely to need it.

    Why it matters: this PR introduces a user-facing “custom launcher” flow, and path arguments are a common use case. The failure is silent and will be hard for users to diagnose.

    Requested fix: preserve literal backslashes (or adopt platform-correct parsing rules) and add regression tests for unquoted and double-quoted Windows/UNC path cases.

🟢 MINOR

  1. Test coverage gap on the new parser path
    The new code paths are otherwise covered well, but the added tests only exercise quotes/spaces. They do not cover backslash-heavy inputs, which is why the bug above slipped through. Please extend the test matrix when fixing the parser.

No other actionable issues survived consensus.

Recommendation: ⚠️ Request changes — please fix the backslash/path parsing issue and add the missing regression tests before merge.

PureWeen and others added 2 commits April 9, 2026 16:45
Preserve literal backslashes in custom launcher arguments and add regression tests for quoted and unquoted path cases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen
Copy link
Copy Markdown
Owner Author

PureWeen commented Apr 9, 2026

PR Review (re-review)

CI status: ⚠️ No GitHub checks are currently reported on this branch. Local validation in the PR plus the follow-up fix includes targeted parser regression tests and a full solution build.

Previous findings status

  1. Backslashes in custom launcher arguments were being stripped
    Status: ⚠️ PARTIALLY FIXED
    The common path/UNC cases are now handled correctly, and the added regression tests cover the original issue well.

  2. Test coverage gap on the new parser path
    Status: ⚠️ PARTIALLY FIXED
    Coverage was improved substantially, but one important edge case is still missing.

🟡 MODERATE

  1. Double-quoted paths ending in a trailing backslash still swallow subsequent arguments
    File: PolyPilot/Models/ConnectionSettings.cs (SplitCommandLineArguments)
    Consensus: All 3 reviewers agree.

    The updated parser still misparses inputs like:

    --workdir "C:\MyProject\" --next-flag
    

    Because the backslash immediately before the closing " is treated as escaping the quote, the quote never closes and everything after it gets folded into the same token. In practice that means --next-flag is silently swallowed instead of being passed as its own argument.

    Why it matters: a trailing \ on a quoted directory path is a normal Windows form. This still leaves a real silent-corruption case in the new custom-launcher feature.

    Requested fix: handle the trailing-backslash-before-closing-quote case correctly and add a regression test for a quoted path ending in \.

Current assessment

The earlier bug is mostly fixed, but this edge case still needs to be addressed before merge.

Recommendation: ⚠️ Request changes — please fix the trailing-backslash quoted-path case and extend the tests for it.

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