Skip to content

Conversation

@sestinj
Copy link
Contributor

@sestinj sestinj commented Aug 28, 2025

Description

Correctly use on prem proxy settings in CLI


Summary by cubic

Routes CLI model requests through the configured on‑prem proxy. Also fixes login detection and trims noisy terminal output in tool results.

  • Bug Fixes
    • Use selectedModel.onPremProxyUrl for proxyUrl so requests go through the on‑prem proxy.
    • Check configuration.accessToken to detect not-logged-in state and unroll locally.
    • Truncate bash output to the first 4 lines and show a “+N lines” indicator.

@sestinj sestinj requested a review from a team as a code owner August 28, 2025 21:01
@sestinj sestinj requested review from tingwai and removed request for a team August 28, 2025 21:01
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 28, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

3 issues found across 3 files

React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.

apiKeyLocation: (selectedModel as any).apiKeyLocation,
orgScopeId: organizationId,
proxyUrl: undefined,
proxyUrl: (selectedModel as any).onPremProxyUrl ?? undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignores the useOnPremProxy flag; should only set proxyUrl when on-prem proxy usage is enabled.

(Based on your team's feedback about correctly honoring on-prem proxy settings in the CLI.)

Prompt for AI agents
Address the following comment on extensions/cli/src/services/ModelService.ts at line 105:

<comment>Ignores the useOnPremProxy flag; should only set proxyUrl when on-prem proxy usage is enabled.

(Based on your team&#39;s feedback about correctly honoring on-prem proxy settings in the CLI.)</comment>

<file context>
@@ -102,7 +102,7 @@ export class ModelService
                   apiKeyLocation: (selectedModel as any).apiKeyLocation,
                   orgScopeId: organizationId,
-                  proxyUrl: undefined,
+                  proxyUrl: (selectedModel as any).onPremProxyUrl ?? undefined,
                 },
               }
</file context>

apiKeyLocation: (selectedModel as any).apiKeyLocation,
orgScopeId: organizationId,
proxyUrl: undefined,
proxyUrl: (selectedModel as any).onPremProxyUrl ?? undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty string onPremProxyUrl will be treated as a base URL and can crash URL construction; coalesce falsy to undefined to avoid invalid base URLs.

(Based on your team's feedback about correctly using on-prem proxy settings in the CLI without introducing runtime errors.)

Prompt for AI agents
Address the following comment on extensions/cli/src/services/ModelService.ts at line 105:

<comment>Empty string onPremProxyUrl will be treated as a base URL and can crash URL construction; coalesce falsy to undefined to avoid invalid base URLs.

(Based on your team&#39;s feedback about correctly using on-prem proxy settings in the CLI without introducing runtime errors.)</comment>

<file context>
@@ -102,7 +102,7 @@ export class ModelService
                   apiKeyLocation: (selectedModel as any).apiKeyLocation,
                   orgScopeId: organizationId,
-                  proxyUrl: undefined,
+                  proxyUrl: (selectedModel as any).onPremProxyUrl ?? undefined,
                 },
               }
</file context>
Suggested change
proxyUrl: (selectedModel as any).onPremProxyUrl ?? undefined,
proxyUrl: (selectedModel as any).onPremProxyUrl || undefined,


// Unroll locally if not logged in
if (!(apiClient as any).configuration.apiKey) {
if (!(apiClient as any).configuration.accessToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly accessing configuration.accessToken can throw if configuration is undefined (e.g., with a mock client). Use optional chaining to avoid a runtime crash.

Prompt for AI agents
Address the following comment on extensions/cli/src/configLoader.ts at line 356:

<comment>Directly accessing configuration.accessToken can throw if configuration is undefined (e.g., with a mock client). Use optional chaining to avoid a runtime crash.</comment>

<file context>
@@ -353,7 +353,7 @@ async function loadAssistantSlug(
 
   // Unroll locally if not logged in
-  if (!(apiClient as any).configuration.apiKey) {
+  if (!(apiClient as any).configuration.accessToken) {
     return await unrollAssistantWithConfig(
       {
</file context>
Suggested change
if (!(apiClient as any).configuration.accessToken) {
if (!((apiClient as any).configuration?.accessToken)) {

@github-project-automation github-project-automation bot moved this from Todo to In Progress in Issues and PRs Aug 28, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 28, 2025
@sestinj sestinj merged commit 4f97b37 into main Aug 28, 2025
59 checks passed
@sestinj sestinj deleted the nate/fix-on-prem-proxy branch August 28, 2025 22:11
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Aug 28, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Aug 28, 2025
@github-actions github-actions bot added the tier 2 Important feature that adds new capabilities to the platform or improves critical user journeys label Aug 28, 2025
@sestinj
Copy link
Contributor Author

sestinj commented Sep 1, 2025

🎉 This PR is included in version 1.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sestinj
Copy link
Contributor Author

sestinj commented Sep 3, 2025

🎉 This PR is included in version 1.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm This PR has been approved by a maintainer released size:S This PR changes 10-29 lines, ignoring generated files. tier 2 Important feature that adds new capabilities to the platform or improves critical user journeys

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants