Repository navigation
feat(vscode): editable .vscode/mcp.json configuration source (auto-detect, load, save) - #89
Conversation
Add HTML mockups exploring a configuration-source switcher that lets the VS Code extension auto-detect, load, edit and save .vscode/mcp.json instead of only the one-way new->export path, plus a structured task under docs/tasks/autodetect_mcp_source/ (PRD, analysis, plan, progress, verification).
Add src/configSource.ts: ConfigSource model, detectWorkspaceMcpJson / readWcli0Entry (JSONC-tolerant, never throws), and parseServerArgs / parseMcpEntry (the inverse of buildServerArgs/buildLaunchSpec) that map a .vscode/mcp.json servers.wcli0 entry back into the form's settings, preserving unmodeled flags in extraArgs. Add settings.defaultSettings() as the parse baseline. Phase 1 of docs/tasks/autodetect_mcp_source.
…, load/save Add a configuration-source switcher to the panel. On open it detects a workspace .vscode/mcp.json with a servers.wcli0 entry and offers a one-click load; loading reverse-parses the entry into the form, and Save writes it back to the file (preserving other servers) without touching wcli0.* settings. - commands.ts: extract writeMcpJsonFromSettings (returns success) from writeWorkspaceMcpJson so the file-source save reuses the safe merge. - webview.ts: source bar + switcher menu + detection banner; init carries source/detected; new sourceChange/sourceChangeRequest/saveToFile handlers; overlaySettings merges form edits onto the loaded-file baseline so unmodeled flags (extraArgs, blocked lists, env) survive. Home config is read-only and never a save target. Dirty source switch confirms first. Phases 2-3 of docs/tasks/autodetect_mcp_source.
Add an Extension Host integration test that merges the wcli0 entry into an existing .vscode/mcp.json and asserts other servers are preserved (the shared writeMcpJsonFromSettings path used by the file-source save). Document the configuration-source switcher, auto-detection and load->edit->save round trip in README. Mark task phases complete. Phase 4 of docs/tasks/autodetect_mcp_source.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 312a5def07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P1: refuse export actions while editing a file source (host guard +
disabled export buttons) so file-source edits never corrupt wcli0.* settings.
P2: reset the file source when the primary workspace folder changes, so a
later save cannot overwrite a different folder's .vscode/mcp.json.
P3: split custom launcher args at the first recognized wcli0 flag (not the
first dash) so dash-prefixed launcher options round-trip in order.
P4: re-baseline the file source from disk after a save so an omitted env is
not resurrected by a later edit.
P5: preserve a loaded http/sse URL verbatim when host/port are unchanged
instead of downgrading custom scheme/path/default-port URLs.
|
Addressed all 5 review comments in c5f90c4:
Added unit tests for each (configSource, commands, webview, webviewButtons); full suite passes (456 tests). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f90c47c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Preserve unmodeled .vscode/mcp.json fields and harden file-source saves: - P6: reject stale file-source saves after the primary workspace folder changes - P7/P12: merge generated fields onto the loaded raw entry, preserving headers/oauth/envFile/dev/sandboxEnabled - P8: keep a valid default port for default-port URLs instead of an invalid 0 - P9: round-trip non-string env values verbatim - P10: preserve socket/named-pipe URLs that cannot be decomposed - P11: clear stale file-source notes on clean reloads - P13: allow VS Code variable --config paths without blocking the save
|
Round 2 review feedback addressed in ca2b0f4 (8 comments, all valid):
Added unit tests for each; full suite passes (469/469) and |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca2b0f4fef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Harden the .vscode/mcp.json reverse parser and file-source save round-trip: - P14/P17: parse node/npx entries with launcher options before the script/package as custom so the launcher args round-trip verbatim - P15: split custom launcher args at the start of the longest pure wcli0 server-flag suffix so a wrapper option colliding with a wcli0 flag is preserved - P16: push a dedicated detection update after a workspace-folder change so the source switcher/banner appear without racing a save's scope realignment - P18: bypass local validation for VS Code launch variables in all preserved launch fields, not just --config - P19: remove the other transport mode's whole field set on a mode switch so headers/oauth/envFile/dev do not leak across - P20: merge the form-generated fields onto the current on-disk entry so external additions made after load are preserved - P21: skip URL userinfo when extracting host/port in parseHttpUrl
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Round 3 review feedback addressed in 5813de1 (8 comments, all valid):
Added 13 unit tests; full suite passes (482/482) and |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5813de1bac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Round 4 review feedback addressed (commit baf060b):
New tests cover P22-P25; full unit suite passes (490). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: facea1936d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed round-5 review feedback (commit 14ebba4):
Full unit suite green (500 tests). |
|
@codex review |
P96 (P1): preserve an explicit --shell all verbatim - the server loads only a shell module named all (none exists) while omitting the flag enables every default shell, so modeling it as the form sentinel let a no-op save open up an entry that had no usable shells. P93: treat a negative numeric token as an option value in the parser and the strippers, matching yargs. P94: recognize any attached boolean assignment as server-suffix evidence, matching the parser after P87. P95: refuse a file save when the entry changed between the handler read and the writer snapshot. P97: stop all three conflict strippers at the -- separator and copy the remainder verbatim.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a511197fd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P98 (P1): the duplicate-scalar pre-scan counts a scalar option on presence rather than only when a value token follows. yargs defines the key either way, so '--shell --debug --shell bash' is shell: ['', 'bash'] - no usable shell - while the parser modeled only bash and a no-op save rewrote the entry as a single --shell bash, enabling command execution through Bash.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16ad205a0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P99 (P1): model every value of a greedy array option, matching yargs' greedy-arrays default for the four options the server declares with array: true. Modeling only the first value left the rest in extraArgs, where the rebuild made them positionals and a no-op save silently dropped entries from a blocklist or the allowed-directory list. The wrapper-suffix detector consumes them too.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
wcli0/vscode-extension/src/webview.ts
Lines 1477 to 1479 in 4a494d6
Preserve whitespace in loaded string arguments
When editing a file source, this unconditional trim changes valid hand-authored argument values before the user edits anything. For example, --config " config.json" refers to a filename whose first character is a space because the server passes the raw CLI string to fs.existsSync; the form loads that raw value, but collect() turns it into config.json, marks the form dirty, and a Save retargets the launch to a different file if both names exist. Keep loaded file-source strings verbatim unless the user actually changes them, while retaining normalization for settings-sourced input if desired.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P100 (P1): preserve every shell name the form's select cannot hold, not just 'all' - an unofferable name left the select empty and let a save drop --shell, re-enabling every default shell. P101 (P1): re-read .vscode/mcp.json immediately before writing and refuse when it changed during the save-time modals; supersedes the write-anyway half of P69/P46, whose guarantees now hold more strictly. P102: restrict the negative-number predicate to the forms yargs really consumes, and stop counting a valueless NUMBER option as a duplicate occurrence (yargs drops it entirely).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1676fbc66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| i = consumeGreedyArrayValues(spec, i); | ||
| continue; | ||
| } | ||
| extraArgs.push(token); |
There was a problem hiding this comment.
For a plain Node entry such as node dist/index.js --shell --debug cmd, yargs parses shell as empty, enables debug, and leaves cmd positional. This branch preserves --shell and later preserves cmd, but the builder emits the modeled --debug before extraArgs, producing --debug --shell cmd; yargs now treats cmd as the shell value, changing the enabled-shell set on a no-op save. Preserve or reject the original valueless-option sequence when a modeled flag intervenes before the positional.
Useful? React with 👍 / 👎.
| (fileSource | ||
| ? preservedFileUrl(settings, urlBase) | ||
| : preservedTransportUrl(settings)) ?? | ||
| `http://${rebuiltHost}:${settings.transportPort}${ |
There was a problem hiding this comment.
When editing an HTTP/SSE file source, the host field is interpolated into the URL without validating that it is an authority host. For example, entering gateway.example/api reports a successful save but writes http://gateway.example/api:9444/mcp; on reload parseHttpUrl reads the host as only gateway.example with no explicit port, so the accepted edit is lost and the endpoint path is malformed. Reject host values containing path, query, fragment, or userinfo delimiters before constructing the URL.
Useful? React with 👍 / 👎.
P103 (P1): emit an explicitly empty --allowedDir verbatim on a file-source save. yargs yields [''] and applyCliShellAndAllowedDirs turns restrictWorkingDirectory ON with that empty allowlist, so dropping the blank removed the restriction and restored the config/default allowed paths. The settings and provider paths still drop blanks.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
wcli0/vscode-extension/src/webview.ts
Line 1483 in d5eb761
Preserve blank allowed-directory rows in the webview
Fresh evidence beyond the earlier builder-side report is that the client still normalizes a loaded allowedDirectories: [''] to [] here: the textarea renders the single empty value as blank, and filter(Boolean) removes it during collectChanged(). Consequently the form is immediately dirty, and saving any unrelated edit submits the empty array and removes --allowedDir ""; the server then falls back from an effectively empty allowlist to its configured/default allowed paths, widening command access. The file-source collector needs a lossless representation for authored empty entries.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const eq = token.indexOf('='); | ||
| if (eq > 0 && token.startsWith('-')) { | ||
| const flag = token.slice(0, eq); | ||
| if (isModeledValueOption(flag)) { | ||
| return true; | ||
| } | ||
| return BOOLEAN_FLAGS.has(flag); |
There was a problem hiding this comment.
For a custom wrapper ending in a token such as wrapper target --no-debug=false, this treats the token as a modeled boolean and splits it into the server suffix. The installed yargs-parser handles --name=value before boolean negation (node_modules/yargs-parser/build/lib/yargs-parser.js:191-210), so this defines the unrelated no-debug key rather than negating debug; parseServerArgs likewise preserves it as an extra argument. After the user edits a real field such as Shell, saving changes the order to target --shell cmd --no-debug=false, potentially changing the wrapper invocation. Attached --no-*= forms should not prove that a wrapper has a wcli0 server suffix.
Useful? React with 👍 / 👎.
…#89 P104 reported that the webview client normalizes a loaded allowedDirectories: [''] to [] and therefore submits an empty array on any unrelated save. The normalization is real but the dirty baseline is produced by the same collect() call, so the field is never dirty and no allowedDirectories key is submitted. Verified by running the shipped webview script in the existing harness: the posted payload is exactly {commandTimeout: 45}. Adds three regression tests (two client-side, one end-to-end) proving --allowedDir "" survives an unrelated save, and records the rejection rationale.
…#89 P104 reported that the webview client normalizes a loaded allowedDirectories: [''] to [] and therefore submits an empty array on any unrelated save. The normalization is real but the dirty baseline is produced by the same collect() call, so the field is never dirty and no allowedDirectories key is submitted. Verified by running the shipped webview script in the existing harness: the posted payload is exactly {commandTimeout: 45}. Adds three regression tests (two client-side, one end-to-end) proving --allowedDir "" survives an unrelated save, and records the rejection rationale.
0e0834b to
32a16f7
Compare
|
Re: the P1 in the 18:24Z review body — "Preserve blank allowed-directory rows in the webview" (webview.ts:1483). Not applying this one; the premise does not hold. The normalization is real: Verified against the shipped client rather than by reading it — the webview test harness runs the real browser-side script:
Adding a lossless representation for blank rows would trade this for a real hazard — a trailing newline or a cleared textarea would then submit an authored deny-all, imposing a restriction the user never asked for. Three regression tests added in 32a16f7 so this stays pinned: two client-side (untouched list not submitted; edited list still submitted) and one end-to-end that loads Note: this finding came in the review body rather than as a review thread, so there is no thread to resolve. |
There was a problem hiding this comment.
💡 Codex Review
wcli0/vscode-extension/src/argsBuilder.ts
Lines 384 to 386 in 32a16f7
Preserve whitespace in file-authored config paths
For a loaded file source, trimming here changes a valid path rather than round-tripping it. For example, --config "/srv/team-config " can intentionally reference a filename with a trailing space; parseServerArgs retains that value, but an unrelated Save rebuilds it as --config /srv/team-config. The server passes args.config directly to fs.existsSync and readFileSync in src/utils/config.ts:136-152, so the rewritten entry loads a different file or falls back to an implicit config, potentially changing its shell and security settings. Keep the original path bytes when preserveRelativePaths is enabled instead of applying trim().
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // stripConfigArgs). A value-option whose next token is a flag is preserved verbatim in | ||
| // extraArgs and the flag is parsed on the next iteration. | ||
| const spec = optionFor(token); | ||
| if (spec && isOptionValueToken(args[i + 1])) { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier valueless-scalar report is that this guard also separates valueless array flags from later positional tokens. For example, node dist/index.js --allowedDir --debug C:\work initially gives yargs an empty allowedDir array and leaves C:\work positional (allowedDir is declared as an array at src/index.ts:91-94), but a no-op save emits --debug --allowedDir C:\work; yargs then consumes the path as an allowed directory, and applyCliShellAndAllowedDirs enables working-directory restriction and disables injection protection. Preserve or reject this original sequence instead of moving the valueless array flag next to a positional.
Useful? React with 👍 / 👎.
| const settings = overlaySettings( | ||
| currentFileParsed?.settings ?? loadedFileSettings ?? defaultSettings(), | ||
| msg.values, | ||
| ); |
There was a problem hiding this comment.
When another editor changes the entry's launch method while the panel is open, this overlay can silently discard a method-specific edit from the form. For example, if the panel loaded an npx entry and the user edits only launch.packageSpec, but the file concurrently changes to a node entry, currentFileParsed.settings supplies launchMethod: 'node' while the submitted package value is overlaid onto it; the writer then emits the node launch unchanged, reports success, and the user's package edit disappears on reparse. The existing transport-mode conflict guard handles the analogous incompatibility, so the save should likewise reject a changed launch method when the submitted fields belong to the previously loaded method.
Useful? React with 👍 / 👎.
|
@codex review and return back with report for P0 and/or P1 remarks only. P2 and other should not be reported. Focus on source code. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32a16f7b90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (args[i].startsWith('-') && isPureServerFlagRun(args.slice(i), !allowIndexZero, stdio)) { | ||
| return i; |
There was a problem hiding this comment.
When a direct entry contains a positional between a boolean and its later negation, such as command: "wcli0", args: ["--allowAllDirs", "marker", "--no-allowAllDirs"], the index-zero run fails this purity check on marker, but the loop continues and selects the final negation as a separate server suffix. parseMcpEntry consequently stores the enabling flag in customArgs while modeling the suffix as false, and a no-op save omits the false value and writes only --allowAllDirs marker, changing the server from restricted to unrestricted directories. This is fresh evidence beyond the existing repeated-boolean parser case because the positional causes the split before parseServerArgs; for a recognized direct wcli0 command, parse or preserve the complete argument list rather than searching for a later suffix.
Useful? React with 👍 / 👎.
2 participants
Summary
Adds a configuration-source switcher to the wcli0 VS Code extension so the panel can
auto-detect, load, edit and save the workspace
.vscode/mcp.json(servers.wcli0entry) — aload → edit → saveround trip alongside the existing one-waynew → exportpath.On open, the panel detects a
.vscode/mcp.jsonthat already defines awcli0server and offers aone-click Load & edit. Loading reverse-maps the entry into the form; Save to file writes it
back, preserving other servers and never touching
wcli0.*settings. The implicit~/.win-cli-mcp/config.jsonis shown read-only and can never be a save target.Implements the mockup-02 (auto-detect on open) plus the minimal mockup-03 (edit/save the file) scope.
Side-by-side settings+file editing, arbitrary file browse, and in-place
config.jsonediting aredeferred (documented as non-requirements).
Documentation
vscode-extension/docs/mockups/exploring the source switcher (index + 5 pages).docs/tasks/autodetect_mcp_source/(PRD, analysis, implementation plan, progress,verification).
README.mdsection: "Editing an existing.vscode/mcp.json".Implementation (by commit)
src/configSource.ts:ConfigSourcemodel,detectWorkspaceMcpJson/readWcli0Entry(JSONC-tolerant, never throw), andparseServerArgs/parseMcpEntry(the inverseof
buildServerArgs/buildLaunchSpec); unmodeled flags preserved inextraArgs. Addssettings.defaultSettings().initcarriessource/detected; newsourceChange/sourceChangeRequest/saveToFilehost handlers;overlaySettingsmerges form edits onto the loaded-file baseline so non-form fields survive.Extracts
writeMcpJsonFromSettings(returns success) fromwriteWorkspaceMcpJsonto reuse the safemerge.
Testing
configSource.test.cjs, 6 webview, 3 commands).tsc --noEmitclean.Notes / review focus
message handlers; the integration test covers the shared file-write merge in-host.
buildServerArgs; unrecognized flags fall through toextraArgsso a save does not silently drop them.package.json/package-lock.jsonis intentionally left outof this branch.