Skip to content

Feat/vision mcp - #36

Merged
KiwiGaze merged 13 commits into
KiwiGaze:mainfrom
elijahqi:feat/vision-mcp
Jul 30, 2026
Merged

Feat/vision mcp#36
KiwiGaze merged 13 commits into
KiwiGaze:mainfrom
elijahqi:feat/vision-mcp

Conversation

@elijahqi

@elijahqi elijahqi commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Related issue

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Refactor / internal change

Checklist

  • pnpm exec tsc -p ./ compiles with no errors.
  • pnpm exec vsce package --no-dependencies -o dist/ packages cleanly.
  • I tested the change in an Extension Development Host (F5).
  • User-facing strings are localized in both English and Simplified Chinese (if applicable).
  • I followed the contributing guidelines and the code style (explicit
    types, no logic comments, no new runtime dependencies).

Summary by CodeRabbit

  • New Features
    • Added GLM Vision image-input support in Copilot Chat, including text-only model compatibility.
    • Introduced a one-click local Vision MCP server install/uninstall flow with consent, pinned/integrity-locked setup, prompt configuration, and enable/disable controls.
    • Enabled image analysis via the Vision tool with validation, progress behavior, session caching, and graceful text-only fallback when analysis fails; added tool-approval gating.
  • Bug Fixes
    • Improved usage error reporting for failed business envelopes delivered with HTTP 200, preserving logged details.
  • Documentation
    • Updated README, changelog, and localized help/commands for Vision setup and usage.
  • Tests
    • Added comprehensive coverage for Vision MCP resolution, caching, and token estimation.

elijahqi added 7 commits July 27, 2026 12:26
…dings

Replace the describer-proxy design (house glm-4.6v chat call, visionModel
setting) with the GLM Vision MCP flow: explicit install/uninstall commands,
health tracking over vscode.lm.tools, a visionEnabled toggle, analyze_image
calls over content-addressed temp files, and new walkthrough steps.

Fix the confirmed findings from the 2026-07-17 branch review:

- Gate image analysis on the vision toggle. With visionEnabled off,
  uncached images from history or tool results are no longer written to
  disk or sent to the vision tool; they degrade to the unavailable marker
  with a "vision is turned off" notice. Cached descriptions still resolve
  so old conversations stay coherent.
- Require the GLM Vision label slug in analyze_image tool names so another
  server's look-alike tool can neither mark the server healthy nor receive
  the user's image files.
- Enforce the 256-file temp-image bound on every analysis call instead of
  once per session.
- Reset visionEnabled on uninstall (update to undefined clears the Global
  override) so a reinstall asks for consent again instead of silently
  resuming analysis.
- Check the Node.js major version during the install preflight (node
  --version, minimum 18), matching the documented behavior; an unknown
  version stays advisory and does not block.
- Correct key-source wording in README, CHANGELOG, and llms.txt: the
  injected key comes from SecretStorage or the documented settings
  fallback, and the extension itself never writes it to settings.json.

The P3 timeout-hang finding is intentionally not addressed: it is
plausible but unconfirmed and requires an MCP server that ignores
cancellation.
Treat authManager.getApiKey() rejections as a missing-key preflight failure so image analysis falls back to the unavailable marker instead of aborting the request.
@elijahqi
elijahqi requested a review from KiwiGaze as a code owner July 27, 2026 05:17
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on this repository. To trigger a review, include @coderabbit in the PR description. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c4824be4-b9fc-4df3-aa4d-0ef25033a750

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an optional local GLM Vision MCP server with locked installation and lifecycle commands, enables image analysis for text-only GLM models, caches descriptions, validates image inputs, handles failures and cancellation, and integrates vision state into model metadata, chat responses, and token estimation.

Changes

GLM Vision integration

Layer / File(s) Summary
Vision contracts and user-facing configuration
package.json, package.nls*.json, src/consts.ts, src/config.ts, src/types.ts, README.md, resources/walkthrough/*
Adds Vision settings, commands, walkthroughs, localized messages, MCP package metadata, model capability changes, and public configuration/auth contracts.
MCP package installation and server lifecycle
src/runtime/*, src/vision-tool.ts, resources/vision-mcp/*
Installs and verifies the pinned MCP package, probes Node/npm, registers the local server, injects API keys at startup, tracks health, and manages install, uninstall, and enablement flows.
Image analysis, caching, and validation
src/provider/vision/*, src/vision-tool.ts
Transforms image parts into MCP-generated text, supports cache hits, temporary file management, validation, progress reporting, cancellation, timeout handling, and localized fallback notices.
Chat provider and token integration
src/provider/*, src/runtime/provider.ts, src/test-helpers.ts, tsconfig.json
Connects Vision state to model capabilities, chat request resolution, cached-image token estimation, thinking progress, and provider registration. Tests cover runtime, resolution, caching, and token behavior.

Estimated code review effort: 5 (Critical) | ~90 minutes

Possibly related issues

Possibly related PRs

  • KiwiGaze/glm-for-copilot#10: Both changes modify model capability metadata and chat-info construction in src/consts.ts and src/provider/models.ts.

Suggested reviewers: kiwigaze

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and clearly points to the main change: adding Vision MCP support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@KiwiGaze

Copy link
Copy Markdown
Owner

@CodeRabbit @codex review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

@KiwiGaze: I’ll review the changes in this PR.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 026da7dfbe

ℹ️ 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".

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
src/provider/vision/resolve.ts (1)

353-373: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Prune can delete another request's in-flight run directory.

pruneTempImages runs on every analysis and rms the oldest entries recursively, including run directories created by concurrent chat requests. Once the directory count exceeds VISION_TEMP_MAX_FILES - 1, a still-running sibling request can lose its temp dir mid-analysis and fail with ENOENT. Consider skipping entries whose mtime is newer than process start of the current run, or tracking active run dirs in a module-level set.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/provider/vision/resolve.ts` around lines 353 - 373, Update
pruneTempImages to avoid deleting directories belonging to active analysis
requests during concurrent runs. Track currently active run directories in a
module-level set and skip those entries when selecting stale items for removal,
ensuring only inactive entries are pruned while preserving the existing
retention limit and warning behavior.
src/runtime/mcp-package.ts (2)

36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Derive the package.json path from the shared constants instead of re-hardcoding the scope/name.

entryPoint comes from VISION_MCP_ENTRYPOINT_PARTS while the manifest path is spelled out literally; if the pinned package or entry layout ever changes, these two can drift silently and isInstalled() would report a false negative/positive.

♻️ Proposed refactor
-	const packageJsonPath = join(installDir, 'node_modules', '`@z_ai`', 'mcp-server', 'package.json');
-	const entryPoint = join(installDir, ...VISION_MCP_ENTRYPOINT_PARTS);
+	const entryPoint = join(installDir, ...VISION_MCP_ENTRYPOINT_PARTS);
+	const packageJsonPath = join(installDir, 'node_modules', VISION_MCP_PACKAGE_NAME, 'package.json');
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/mcp-package.ts` around lines 36 - 41, Update
hasInstalledVisionMcp to derive packageJsonPath from the shared
VISION_MCP_ENTRYPOINT_PARTS constants rather than hard-coding the
`@z_ai/mcp-server` scope and package name, while preserving the existing existence
checks and boolean behavior.

63-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

maxBuffer: 1024 * 1024 may abort an otherwise successful npm ci.

npm writes progress/notice output to stderr; exceeding 1 MB kills the child with ENOBUFS and surfaces as a generic install failure. Consider raising it (e.g. 8 MB) or adding --no-progress/npm_config_progress: 'false'.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/mcp-package.ts` around lines 63 - 75, Update the npm ci spawn
options in the MCP package installation flow to prevent large stderr output from
aborting successful installs: either increase maxBuffer substantially (for
example, 8 MB) or disable npm progress via the existing environment
configuration. Preserve the current install timeout and other npm settings.
src/runtime/mcp-package.test.ts (1)

136-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert against VISION_MCP_PACKAGE_VERSION rather than a third copy of 0.1.4.

The pinned version now lives in src/consts.ts, in seedInstalledPackage (line 21), and here. Importing the constant keeps a version bump from silently passing/failing in only some places (the integrity hash still needs a manual update, which is the point of the test).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/mcp-package.test.ts` around lines 136 - 151, Update the test case
around the pinned MCP package assertion to import and compare against
VISION_MCP_PACKAGE_VERSION from src/consts.ts instead of hardcoding 0.1.4. Keep
the explicit integrity hash assertion unchanged so integrity updates remain
manual.
src/runtime/mcp.ts (1)

408-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Registration is pushed to context.subscriptions on every (re)install.

uninstall() disposes this.registration but nothing removes it from context.subscriptions, so an install → uninstall → install cycle accumulates dead disposables for the extension's lifetime. Since dispose() already tears the registration down via this.registration, the push is redundant.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/mcp.ts` around lines 408 - 410, Remove the
this.context.subscriptions.push(this.registration) call from the installation
flow around register and retain registration disposal through this.registration
in uninstall(). Ensure repeated install/uninstall cycles do not accumulate
disposed registrations while preserving the existing register and return
behavior.
package.json (1)

264-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Dropping vision from customModels while additionalProperties: false breaks existing user settings.

Users who previously set "vision": true on a custom model will now get a settings validation error (and getCustomModels silently ignores it). Consider keeping the property with a deprecation message instead of removing it outright.

♻️ Suggested deprecation shim
 								"thinking": {
 									"type": "boolean",
 									"description": "Whether the model supports thinking. Defaults to true."
+								},
+								"vision": {
+									"type": "boolean",
+									"deprecationMessage": "No longer used. Image input is provided by the GLM Vision MCP server via glm-copilot.visionEnabled.",
+									"description": "Deprecated."
 								}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package.json` around lines 264 - 290, Preserve backward compatibility in the
custom model schema by adding the legacy vision boolean property back alongside
id, name, and capability fields. Keep additionalProperties false and mark vision
as deprecated with a clear description indicating that it is retained for
existing settings and no longer used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/mcp.ts`:
- Around line 81-97: Update the child-process completion handling around the
stdout collector to resolve on the child’s close event instead of exit, ensuring
all stdout data has been flushed before returning { code, stdout }. Keep the
timeout and error handling behavior unchanged, and preserve the existing
probeNodeMajor result flow.

---

Nitpick comments:
In `@package.json`:
- Around line 264-290: Preserve backward compatibility in the custom model
schema by adding the legacy vision boolean property back alongside id, name, and
capability fields. Keep additionalProperties false and mark vision as deprecated
with a clear description indicating that it is retained for existing settings
and no longer used.

In `@src/provider/vision/resolve.ts`:
- Around line 353-373: Update pruneTempImages to avoid deleting directories
belonging to active analysis requests during concurrent runs. Track currently
active run directories in a module-level set and skip those entries when
selecting stale items for removal, ensuring only inactive entries are pruned
while preserving the existing retention limit and warning behavior.

In `@src/runtime/mcp-package.test.ts`:
- Around line 136-151: Update the test case around the pinned MCP package
assertion to import and compare against VISION_MCP_PACKAGE_VERSION from
src/consts.ts instead of hardcoding 0.1.4. Keep the explicit integrity hash
assertion unchanged so integrity updates remain manual.

In `@src/runtime/mcp-package.ts`:
- Around line 36-41: Update hasInstalledVisionMcp to derive packageJsonPath from
the shared VISION_MCP_ENTRYPOINT_PARTS constants rather than hard-coding the
`@z_ai/mcp-server` scope and package name, while preserving the existing existence
checks and boolean behavior.
- Around line 63-75: Update the npm ci spawn options in the MCP package
installation flow to prevent large stderr output from aborting successful
installs: either increase maxBuffer substantially (for example, 8 MB) or disable
npm progress via the existing environment configuration. Preserve the current
install timeout and other npm settings.

In `@src/runtime/mcp.ts`:
- Around line 408-410: Remove the
this.context.subscriptions.push(this.registration) call from the installation
flow around register and retain registration disposal through this.registration
in uninstall(). Ensure repeated install/uninstall cycles do not accumulate
disposed registrations while preserving the existing register and return
behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 255b762d-a945-475a-ad6b-31f59611d946

📥 Commits

Reviewing files that changed from the base of the PR and between f1e3966 and 026da7d.

⛔ Files ignored due to path filters (1)
  • resources/vision-mcp/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (34)
  • CHANGELOG.md
  • README.md
  • llms.txt
  • package.json
  • package.nls.json
  • package.nls.zh-cn.json
  • resources/vision-mcp/package.json
  • resources/walkthrough/enable-vision.md
  • resources/walkthrough/install-vision-mcp.md
  • src/auth.ts
  • src/config.ts
  • src/consts.ts
  • src/i18n.ts
  • src/provider/index.ts
  • src/provider/models.ts
  • src/provider/stream.ts
  • src/provider/thinking.ts
  • src/provider/tokens.test.ts
  • src/provider/tokens.ts
  • src/provider/vision/cache.test.ts
  • src/provider/vision/cache.ts
  • src/provider/vision/consts.ts
  • src/provider/vision/resolve.test.ts
  • src/provider/vision/resolve.ts
  • src/runtime/mcp-package.test.ts
  • src/runtime/mcp-package.ts
  • src/runtime/mcp.test.ts
  • src/runtime/mcp.ts
  • src/runtime/provider.ts
  • src/test-helpers.ts
  • src/types.ts
  • src/vision-tool.test.ts
  • src/vision-tool.ts
  • tsconfig.json

Comment thread src/runtime/mcp.ts

@KiwiGaze KiwiGaze left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three findings from reviewing this branch against the pinned @z_ai/mcp-server@0.1.4 package contents. Each was verified by executing or reading the pinned package rather than inferred from its docs.

Not duplicated with the existing CodeRabbit review. A fourth item I looked at — a race in runProbe where exit can fire before stdout is drained, silently skipping the Node version gate — is already covered by CodeRabbit's actionable comment on src/runtime/mcp.ts:97, so it is omitted here.

Comment thread src/runtime/mcp.ts
Comment thread src/provider/tokens.ts
Comment thread src/provider/vision/resolve.ts

@KiwiGaze KiwiGaze left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additional verified findings from the supplied review list. Items 1 and 2 are already present in the current review threads, so they are omitted here to avoid duplicates.

Comment thread src/runtime/mcp.ts Outdated
Comment thread src/provider/vision/consts.ts Outdated
@KiwiGaze

Copy link
Copy Markdown
Owner

@codex @CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

@KiwiGaze: I’ll review the changes in this PR.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/mcp-package.ts (1)

114-134: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make package lifecycle transitions rollback-safe.

Both install and uninstall can leave Vision unavailable while state no longer reflects the filesystem.

  • src/runtime/mcp-package.ts#L114-L134: preserve the existing installation until the staged directory has been successfully promoted, or use a backup-and-rollback sequence if promotion fails.
  • src/runtime/mcp.ts#L307-L357: clear persisted state and dispose registration only after uninstall succeeds; otherwise retain or mark the installation as pending cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/mcp-package.ts` around lines 114 - 134, The package lifecycle
transitions are not rollback-safe. In src/runtime/mcp-package.ts:114-134, update
install() to preserve the existing installation until staging is successfully
promoted, or restore it if promotion fails. In src/runtime/mcp.ts:307-357,
update uninstall() so persisted state is cleared and registration disposed only
after uninstall succeeds; otherwise retain the state or mark the installation
pending cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/runtime/mcp-package.ts`:
- Around line 114-134: The package lifecycle transitions are not rollback-safe.
In src/runtime/mcp-package.ts:114-134, update install() to preserve the existing
installation until staging is successfully promoted, or restore it if promotion
fails. In src/runtime/mcp.ts:307-357, update uninstall() so persisted state is
cleared and registration disposed only after uninstall succeeds; otherwise
retain the state or mark the installation pending cleanup.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cf32d7e1-db33-4ade-8e5e-535010e45d9a

📥 Commits

Reviewing files that changed from the base of the PR and between 026da7d and 88e6788.

📒 Files selected for processing (11)
  • src/consts.ts
  • src/provider/tokens.test.ts
  • src/provider/tokens.ts
  • src/provider/vision/consts.ts
  • src/provider/vision/parts.ts
  • src/provider/vision/resolve.test.ts
  • src/provider/vision/resolve.ts
  • src/runtime/mcp-package.test.ts
  • src/runtime/mcp-package.ts
  • src/runtime/mcp.test.ts
  • src/runtime/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/provider/vision/consts.ts
  • src/runtime/mcp-package.test.ts
  • src/provider/vision/resolve.test.ts
  • src/provider/tokens.test.ts
  • src/provider/vision/resolve.ts
  • src/runtime/mcp.test.ts
  • src/consts.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88e6788a4b

ℹ️ 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".

Comment thread src/runtime/mcp.ts
Comment thread src/provider/vision/resolve.ts
@KiwiGaze

Copy link
Copy Markdown
Owner

Addressed the outside-diff package lifecycle finding in 5d71178. Installation now preserves and restores the previous package if promotion fails, successful promotion remains valid if old-backup cleanup fails, and uninstall retains registration and persisted state when package removal fails. Regression coverage exercises all three failure paths. Verification: 204 tests, typecheck, lint, and git diff check passed under Node 22.22.

@KiwiGaze

Copy link
Copy Markdown
Owner

@codex @CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

@KiwiGaze: I’ll review the latest changes in this PR.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d71178db7

ℹ️ 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".

Comment thread src/runtime/mcp.ts
Comment thread src/runtime/mcp.ts Outdated
@KiwiGaze

Copy link
Copy Markdown
Owner

@codex @CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

@KiwiGaze: I’ll review the latest changes in this PR.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b18a78747

ℹ️ 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".

Comment thread src/provider/tokens.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/runtime/mcp.test.ts (1)

665-667: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Negative toHaveBeenCalledWith is arg-exact, so this can pass vacuously.

not.toHaveBeenCalledWith('visionMcp.uninstall.done') only fails on a call with exactly that one argument. If the success path ever passes an action button alongside the message, these tests would keep passing. Asserting on the message argument alone is more robust.

🧪 Suggested stricter assertion
-		expect(mocks.showInformationMessage).not.toHaveBeenCalledWith(
-			'visionMcp.uninstall.done',
-		);
+		expect(
+			mocks.showInformationMessage.mock.calls.map((call) => call[0]),
+		).not.toContain('visionMcp.uninstall.done');

Also applies to: 685-687

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/mcp.test.ts` around lines 665 - 667, Update the negative
assertions in the uninstall tests around the existing showInformationMessage
expectations to verify that no call contains the message argument, regardless of
additional action-button arguments. Apply the same message-only matching
approach to both assertion locations.
src/auth.test.ts (1)

48-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Consider pinning the settings-key fail-closed case too.

This asserts no fallback from the chat secret. The other fallback path that matters for credential isolation is getSettingsApiKey(): with no secrets stored at all, getApiKey() returns settings.secret while getVisionApiKey() must stay undefined.

🧪 Suggested additional case
+	it('does not fall back to the settings credential for the Vision key', async () => {
+		const authManager = new AuthManager(fakeContext(new Map()));
+
+		expect(await authManager.getApiKey()).toBe('settings.secret');
+		expect(await authManager.getVisionApiKey()).toBeUndefined();
+	});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/auth.test.ts` around lines 48 - 54, Add a test covering the settings-key
credential isolation case: construct AuthManager with no stored secrets, assert
getApiKey() returns the settings credential, and assert getVisionApiKey()
remains undefined. Keep the existing chat-secret fallback test unchanged.
pnpm-workspace.yaml (1)

1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin pnpm to 10.26+ before using allowBuilds.

allowBuilds was added in pnpm v10.26; before that, pnpm does not recognize it and falls back to existing build allowlist settings, so these entries may be ignored. Add a packageManager/runtime pin or switch back to onlyBuiltDependencies if older pnpm versions are supported.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pnpm-workspace.yaml` around lines 1 - 3, Ensure the workspace uses pnpm 10.26
or newer before relying on the allowBuilds configuration in pnpm-workspace.yaml,
by adding or updating the repository’s packageManager/runtime version pin. If
older pnpm versions must remain supported, replace allowBuilds with
onlyBuiltDependencies while preserving the '`@vscode/vsce-sign`' and keytar
entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/mcp-package.ts`:
- Around line 136-157: Update the promotion failure handling around renamePath
and the promotionError/restoreError catches so backupDir is also cleaned up on
any failed promotion, including when restoring the previous installation fails.
Perform this as best-effort cleanup without masking the original promotion or
aggregate restore error, while preserving the existing successful-install backup
removal.

In `@src/runtime/mcp.ts`:
- Around line 323-415: Update install() and uninstall() to use the existing
installInProgress flag as a shared operation lock. Have uninstall() return or
otherwise avoid starting while installation is active, and ensure install() sets
and clears the flag across its entire operation, including failure paths, so
uninstall cannot interleave with registration, state persistence, or package
cleanup.
- Around line 719-722: Update deleteTempImages to catch and ignore
vscode.FileSystemError.FileNotFound when deleting the directory returned by
getVisionTempDir, while preserving propagation of other deletion errors so
cleanupFailures still records genuine failures.

---

Nitpick comments:
In `@pnpm-workspace.yaml`:
- Around line 1-3: Ensure the workspace uses pnpm 10.26 or newer before relying
on the allowBuilds configuration in pnpm-workspace.yaml, by adding or updating
the repository’s packageManager/runtime version pin. If older pnpm versions must
remain supported, replace allowBuilds with onlyBuiltDependencies while
preserving the '`@vscode/vsce-sign`' and keytar entries.

In `@src/auth.test.ts`:
- Around line 48-54: Add a test covering the settings-key credential isolation
case: construct AuthManager with no stored secrets, assert getApiKey() returns
the settings credential, and assert getVisionApiKey() remains undefined. Keep
the existing chat-secret fallback test unchanged.

In `@src/runtime/mcp.test.ts`:
- Around line 665-667: Update the negative assertions in the uninstall tests
around the existing showInformationMessage expectations to verify that no call
contains the message argument, regardless of additional action-button arguments.
Apply the same message-only matching approach to both assertion locations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7f15d6de-f612-4d96-adc9-320ba0f27c48

📥 Commits

Reviewing files that changed from the base of the PR and between 88e6788 and 4b18a78.

📒 Files selected for processing (32)
  • .vscodeignore
  • CHANGELOG.md
  • README.md
  • llms.txt
  • package.json
  • package.nls.json
  • package.nls.zh-cn.json
  • pnpm-workspace.yaml
  • resources/walkthrough/enable-vision.md
  • src/auth.test.ts
  • src/auth.ts
  • src/config.ts
  • src/consts.ts
  • src/i18n.ts
  • src/provider/convert.ts
  • src/provider/index.ts
  • src/provider/models.test.ts
  • src/provider/models.ts
  • src/provider/thinking.ts
  • src/provider/tokens.ts
  • src/provider/vision/cache.test.ts
  • src/provider/vision/cache.ts
  • src/provider/vision/resolve.test.ts
  • src/provider/vision/resolve.ts
  • src/runtime/mcp-package.test.ts
  • src/runtime/mcp-package.ts
  • src/runtime/mcp.test.ts
  • src/runtime/mcp.ts
  • src/runtime/provider.ts
  • src/types.ts
  • src/vision-tool.test.ts
  • src/vision-tool.ts
🚧 Files skipped from review as they are similar to previous changes (19)
  • resources/walkthrough/enable-vision.md
  • src/provider/vision/cache.test.ts
  • src/provider/models.ts
  • src/provider/vision/cache.ts
  • src/vision-tool.test.ts
  • src/vision-tool.ts
  • src/runtime/provider.ts
  • src/config.ts
  • src/runtime/mcp-package.test.ts
  • llms.txt
  • src/provider/index.ts
  • package.nls.json
  • package.nls.zh-cn.json
  • src/types.ts
  • src/provider/tokens.ts
  • package.json
  • README.md
  • src/consts.ts
  • src/provider/vision/resolve.test.ts

Comment thread src/runtime/mcp-package.ts
Comment thread src/runtime/mcp.ts
Comment thread src/runtime/mcp.ts
@KiwiGaze KiwiGaze self-assigned this Jul 30, 2026
@KiwiGaze
KiwiGaze merged commit d83eb24 into KiwiGaze:main Jul 30, 2026
2 checks passed
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