Skip to content

fix(vscode-ide-companion): preserve explicit null context limits#3098

Merged
tanzhenxin merged 1 commit into
QwenLM:fix/2847-vscode-new-session-minimalfrom
yiliang114:followup/2874-review-suggestions
Apr 11, 2026
Merged

fix(vscode-ide-companion): preserve explicit null context limits#3098
tanzhenxin merged 1 commit into
QwenLM:fix/2847-vscode-new-session-minimalfrom
yiliang114:followup/2874-review-suggestions

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

Summary

  • respect explicit contextLimit: null values from ACP instead of overriding them with a derived known-model limit
  • keep the derived-limit fallback only for payloads that omit contextLimit
  • remove the now-unused browser-side utils/tokenLimits.ts helper after the context-usage refactor in fix(vscode): force fresh ACP session on new-session action #2874

Testing

  • cd packages/vscode-ide-companion && npm run lint
  • cd packages/vscode-ide-companion && npm run check-types
  • cd packages/vscode-ide-companion && npx vitest run src/utils/acpModelInfo.test.ts src/utils/imageSupport.bundle.test.ts
  • cd packages/vscode-ide-companion && npm run build:dev

Context

Preserve server-provided null context limits instead of replacing them with derived values for known models.

Also remove the unused browser tokenLimits helper after the webview context-usage refactor.
@github-actions

Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR is a well-crafted follow-up to #2874 that fixes how explicit contextLimit: null values from ACP are handled. The changes ensure that explicit nulls from the server are preserved rather than being overridden by derived known-model limits, while maintaining the derived-limit fallback for payloads that omit contextLimit. The removal of the now-unused tokenLimits.ts helper is appropriate given the context-usage refactor.

🔍 General Feedback

  • Clean, focused changes: The PR makes minimal, surgical changes to address the specific issue without introducing unnecessary complexity.
  • Good test coverage: New test cases cover both extractSessionModelState and extractModelInfoFromNewSessionResult for the explicit null preservation behavior.
  • Clear priority reordering: The comment update accurately reflects the new priority order for context limit resolution.
  • Appropriate cleanup: Removing tokenLimits.ts after the refactor in fix(vscode): force fresh ACP session on new-session action #2874 prevents dead code accumulation.

🎯 Specific Feedback

🟢 Medium

  • File: packages/vscode-ide-companion/src/utils/acpModelInfo.ts:68-74 - The priority comment is helpful, but consider adding a brief example or reference to the ACP spec that documents what contextLimit: null semantically means ("limit intentionally unknown"). This would help future maintainers understand why explicit null takes precedence over derived limits.

🔵 Low

  • File: packages/vscode-ide-companion/src/utils/acpModelInfo.test.ts:255 - The renamed test "derives contextLimit for known models when metadata omits contextLimit" is clearer than the original, but consider also testing the edge case where _meta is explicitly null vs {} to ensure both paths are covered (the current test uses {}).

  • File: packages/vscode-ide-companion/src/utils/acpModelInfo.ts - After removing the knownTokenLimit import and getContextLimitFromMeta helper, consider whether any inline documentation should be added to explain the contextLimit resolution logic for readers unfamiliar with the ACP protocol semantics.

✅ Highlights

  • Excellent test additions: The two new test cases ('preserves explicit null contextLimit for known models') clearly demonstrate the intended behavior and protect against regressions.
  • Thoughtful prioritization: The reordering of the priority chain correctly respects server intent while maintaining sensible fallbacks.
  • Good cleanup discipline: Removing tokenLimits.ts (196 lines) demonstrates good hygiene in eliminating dead code after refactors.

@tanzhenxin tanzhenxin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review

Clean follow-up to #2874. Deletes the duplicated 196-line tokenLimits.ts, preserves explicit null context limits from ACP, and consolidates state-clearing into resetConversationState. Good test coverage.

Verdict

APPROVE

@tanzhenxin tanzhenxin merged commit 3e64b4d into QwenLM:fix/2847-vscode-new-session-minimal Apr 11, 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