Skip to content

fix: show different dialogs for authorization errors and other errors#305

Merged
liukatkat merged 5 commits intodevelopfrom
katrina/transient-login-error-nudge
Apr 7, 2026
Merged

fix: show different dialogs for authorization errors and other errors#305
liukatkat merged 5 commits intodevelopfrom
katrina/transient-login-error-nudge

Conversation

@liukatkat
Copy link
Copy Markdown
Contributor

@liukatkat liukatkat commented Mar 26, 2026 •
edited
Loading

This PR adds a new command loginFailedNudge which is different from the loginNudge. This command is intended to be the pop up dialog for any non-authorization errors occurred during login.

Reasons as to why we need to do this is documented here: https://github.com/semgrep/semgrep-proprietary/pull/5938

This change should only be released after the change in the semgrep-proprietary repo is released and bumped in this repo.

Test plan:

Experimental LS

No token:
image

Network error:
image
and token was not cleared out.

Bad token (authorization error):
image
and token was not cleared out.

Also checked that rules were successfully refreshed for the valid token case

Legacy:

No token:
image

Network error:
image
and token wasn't cleared out.

Bad token (authorization error):
image
and token wasn't cleared out.

Also checked that rules were successfully refreshed for the valid token case

PR checklist:

  • Purpose of the code is evident to future readers
  • Tests included or PR comment includes a reproducible test plan
  • Documentation is up-to-date
  • A changelog entry was for any user-facing change
  • Change has no security implications (otherwise, ping security team)

If you're unsure about any of this, please see:

@liukatkat Graphite App
Copy link
Copy Markdown
Contributor Author

liukatkat commented Mar 26, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

liukatkat
liukatkat commented Mar 26, 2026
View reviewed changes
Comment thread src/views/policy.ts
export class SemgrepPolicyViewProvider
implements vscode.TreeDataProvider<PolicyItem>
{
export class SemgrepPolicyViewProvider implements vscode.TreeDataProvider<PolicyItem> {
Copy link
Copy Markdown
Contributor Author

@liukatkat liukatkat Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is an unrelated change, but I had to do this to pass precommit.

@liukatkat liukatkat changed the title handle login error from LSP fix: show different dialogs for authorization errors and other errors Mar 26, 2026
@liukatkat liukatkat marked this pull request as ready for review March 26, 2026 21:26
semgrep-ci Bot pushed a commit to semgrep/semgrep that referenced this pull request Apr 1, 2026
@liukatkat
fix(lsp): distinguish unauthorized vs other errors in login verificat…
66340cd 
…ion (semgrep/semgrep-proprietary#5938)

### Why:

As reported in ENGINE-2638 (IDE login issues propagating to MCP), users who have both MCP/Plugin and the IDE enabled are frequently required to rerun `semgrep login`.

During investigation, I found that we clear the stored Semgrep token and surface an “invalid token” error (via a popup dialog) even when token validation fails for reasons other than a 401, which is bad.

Additionally, I want to ensure that this IDE behavior does not negatively impact MCP. Currently, the IDE clears tokens whenever validation fails, but the underlying issue may be with how validation is performed rather than the token itself. Since MCP depends on these tokens, this can lead to unnecessary login churn.

### How:

To address the first issue, in this PR (and [a corresponding PR ](semgrep/semgrep-vscode#305 the `semgrep-vscode` repo), I’ve updated error handling so that different failure modes produce distinct logs and popup dialogs. This will help us better understand what’s actually happening when users encounter this issue again.

For the second point, this PR removes all instances where the IDE proactively clears stored tokens. Instead, if the IDE determines that a token is invalid, we return an error and allow the user to explicitly re-authenticate, overwriting the existing token if needed.

One remaining issue is that we currently rely on `is_logged_in_weak` during `LoginStart` requests to determine login state. If we stop clearing invalid tokens, this check may incorrectly treat a bad token as valid. As a result, we should update `LoginStart` to perform a proper token validation check rather than relying solely on `is_logged_in_weak`.

### Test plan:

See the test plan in semgrep/semgrep-vscode#305.

synced from Pro 8dc8daf9a14a8c3e9526601dd00877acf6dfe6b9
semgrep-ci Bot pushed a commit to semgrep/semgrep that referenced this pull request Apr 2, 2026
@liukatkat
fix(lsp): distinguish unauthorized vs other errors in login verificat…
7541b74 
…ion (semgrep/semgrep-proprietary#5938)

### Why:

As reported in ENGINE-2638 (IDE login issues propagating to MCP), users who have both MCP/Plugin and the IDE enabled are frequently required to rerun `semgrep login`.

During investigation, I found that we clear the stored Semgrep token and surface an “invalid token” error (via a popup dialog) even when token validation fails for reasons other than a 401, which is bad.

Additionally, I want to ensure that this IDE behavior does not negatively impact MCP. Currently, the IDE clears tokens whenever validation fails, but the underlying issue may be with how validation is performed rather than the token itself. Since MCP depends on these tokens, this can lead to unnecessary login churn.

### How:

To address the first issue, in this PR (and [a corresponding PR ](semgrep/semgrep-vscode#305 the `semgrep-vscode` repo), I’ve updated error handling so that different failure modes produce distinct logs and popup dialogs. This will help us better understand what’s actually happening when users encounter this issue again.

For the second point, this PR removes all instances where the IDE proactively clears stored tokens. Instead, if the IDE determines that a token is invalid, we return an error and allow the user to explicitly re-authenticate, overwriting the existing token if needed.

One remaining issue is that we currently rely on `is_logged_in_weak` during `LoginStart` requests to determine login state. If we stop clearing invalid tokens, this check may incorrectly treat a bad token as valid. As a result, we should update `LoginStart` to perform a proper token validation check rather than relying solely on `is_logged_in_weak`.

### Test plan:

See the test plan in semgrep/semgrep-vscode#305.

synced from Pro 8dc8daf9a14a8c3e9526601dd00877acf6dfe6b9
semgrep-ci Bot pushed a commit to semgrep/semgrep that referenced this pull request Apr 3, 2026
@liukatkat
fix(lsp): distinguish unauthorized vs other errors in login verificat…
54906fe 
…ion (semgrep/semgrep-proprietary#5938)

### Why:

As reported in ENGINE-2638 (IDE login issues propagating to MCP), users who have both MCP/Plugin and the IDE enabled are frequently required to rerun `semgrep login`.

During investigation, I found that we clear the stored Semgrep token and surface an “invalid token” error (via a popup dialog) even when token validation fails for reasons other than a 401, which is bad.

Additionally, I want to ensure that this IDE behavior does not negatively impact MCP. Currently, the IDE clears tokens whenever validation fails, but the underlying issue may be with how validation is performed rather than the token itself. Since MCP depends on these tokens, this can lead to unnecessary login churn.

### How:

To address the first issue, in this PR (and [a corresponding PR ](semgrep/semgrep-vscode#305 the `semgrep-vscode` repo), I’ve updated error handling so that different failure modes produce distinct logs and popup dialogs. This will help us better understand what’s actually happening when users encounter this issue again.

For the second point, this PR removes all instances where the IDE proactively clears stored tokens. Instead, if the IDE determines that a token is invalid, we return an error and allow the user to explicitly re-authenticate, overwriting the existing token if needed.

One remaining issue is that we currently rely on `is_logged_in_weak` during `LoginStart` requests to determine login state. If we stop clearing invalid tokens, this check may incorrectly treat a bad token as valid. As a result, we should update `LoginStart` to perform a proper token validation check rather than relying solely on `is_logged_in_weak`.

### Test plan:

See the test plan in semgrep/semgrep-vscode#305.

synced from Pro 8dc8daf9a14a8c3e9526601dd00877acf6dfe6b9
semgrep-ci Bot pushed a commit to semgrep/semgrep that referenced this pull request Apr 3, 2026
@liukatkat
fix(lsp): distinguish unauthorized vs other errors in login verificat…
bbf5158 
…ion (semgrep/semgrep-proprietary#5938)

### Why:

As reported in ENGINE-2638 (IDE login issues propagating to MCP), users who have both MCP/Plugin and the IDE enabled are frequently required to rerun `semgrep login`.

During investigation, I found that we clear the stored Semgrep token and surface an “invalid token” error (via a popup dialog) even when token validation fails for reasons other than a 401, which is bad.

Additionally, I want to ensure that this IDE behavior does not negatively impact MCP. Currently, the IDE clears tokens whenever validation fails, but the underlying issue may be with how validation is performed rather than the token itself. Since MCP depends on these tokens, this can lead to unnecessary login churn.

### How:

To address the first issue, in this PR (and [a corresponding PR ](semgrep/semgrep-vscode#305 the `semgrep-vscode` repo), I’ve updated error handling so that different failure modes produce distinct logs and popup dialogs. This will help us better understand what’s actually happening when users encounter this issue again.

For the second point, this PR removes all instances where the IDE proactively clears stored tokens. Instead, if the IDE determines that a token is invalid, we return an error and allow the user to explicitly re-authenticate, overwriting the existing token if needed.

One remaining issue is that we currently rely on `is_logged_in_weak` during `LoginStart` requests to determine login state. If we stop clearing invalid tokens, this check may incorrectly treat a bad token as valid. As a result, we should update `LoginStart` to perform a proper token validation check rather than relying solely on `is_logged_in_weak`.

### Test plan:

See the test plan in semgrep/semgrep-vscode#305.

synced from Pro 8dc8daf9a14a8c3e9526601dd00877acf6dfe6b9
semgrep-ci Bot pushed a commit to semgrep/semgrep that referenced this pull request Apr 6, 2026
@liukatkat
fix(lsp): distinguish unauthorized vs other errors in login verificat…
c5c17fb 
…ion (semgrep/semgrep-proprietary#5938)

### Why:

As reported in ENGINE-2638 (IDE login issues propagating to MCP), users who have both MCP/Plugin and the IDE enabled are frequently required to rerun `semgrep login`.

During investigation, I found that we clear the stored Semgrep token and surface an “invalid token” error (via a popup dialog) even when token validation fails for reasons other than a 401, which is bad.

Additionally, I want to ensure that this IDE behavior does not negatively impact MCP. Currently, the IDE clears tokens whenever validation fails, but the underlying issue may be with how validation is performed rather than the token itself. Since MCP depends on these tokens, this can lead to unnecessary login churn.

### How:

To address the first issue, in this PR (and [a corresponding PR ](semgrep/semgrep-vscode#305 the `semgrep-vscode` repo), I’ve updated error handling so that different failure modes produce distinct logs and popup dialogs. This will help us better understand what’s actually happening when users encounter this issue again.

For the second point, this PR removes all instances where the IDE proactively clears stored tokens. Instead, if the IDE determines that a token is invalid, we return an error and allow the user to explicitly re-authenticate, overwriting the existing token if needed.

One remaining issue is that we currently rely on `is_logged_in_weak` during `LoginStart` requests to determine login state. If we stop clearing invalid tokens, this check may incorrectly treat a bad token as valid. As a result, we should update `LoginStart` to perform a proper token validation check rather than relying solely on `is_logged_in_weak`.

### Test plan:

See the test plan in semgrep/semgrep-vscode#305.

synced from Pro 8dc8daf9a14a8c3e9526601dd00877acf6dfe6b9
yosefAlsuhaibani pushed a commit to semgrep/semgrep that referenced this pull request Apr 6, 2026
@liukatkat @yosefAlsuhaibani
fix(lsp): distinguish unauthorized vs other errors in login verificat…
5762a85 
…ion (semgrep/semgrep-proprietary#5938)

### Why:

As reported in ENGINE-2638 (IDE login issues propagating to MCP), users who have both MCP/Plugin and the IDE enabled are frequently required to rerun `semgrep login`.

During investigation, I found that we clear the stored Semgrep token and surface an “invalid token” error (via a popup dialog) even when token validation fails for reasons other than a 401, which is bad.

Additionally, I want to ensure that this IDE behavior does not negatively impact MCP. Currently, the IDE clears tokens whenever validation fails, but the underlying issue may be with how validation is performed rather than the token itself. Since MCP depends on these tokens, this can lead to unnecessary login churn.

### How:

To address the first issue, in this PR (and [a corresponding PR ](semgrep/semgrep-vscode#305 the `semgrep-vscode` repo), I’ve updated error handling so that different failure modes produce distinct logs and popup dialogs. This will help us better understand what’s actually happening when users encounter this issue again.

For the second point, this PR removes all instances where the IDE proactively clears stored tokens. Instead, if the IDE determines that a token is invalid, we return an error and allow the user to explicitly re-authenticate, overwriting the existing token if needed.

One remaining issue is that we currently rely on `is_logged_in_weak` during `LoginStart` requests to determine login state. If we stop clearing invalid tokens, this check may incorrectly treat a bad token as valid. As a result, we should update `LoginStart` to perform a proper token validation check rather than relying solely on `is_logged_in_weak`.

### Test plan:

See the test plan in semgrep/semgrep-vscode#305.

synced from Pro 8dc8daf9a14a8c3e9526601dd00877acf6dfe6b9
@liukatkat Graphite App
Copy link
Copy Markdown
Contributor Author

liukatkat commented Apr 7, 2026 •
edited
Loading

Merge activity

  • Apr 7, 2:35 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 7, 2:35 PM UTC: @liukatkat merged this pull request with Graphite.

@liukatkat liukatkat merged commit 18eaee5 into develop Apr 7, 2026
11 checks passed
@liukatkat liukatkat deleted the katrina/transient-login-error-nudge branch April 7, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajbt200128 ajbt200128 ajbt200128 approved these changes

@brandonspark brandonspark Awaiting requested review from brandonspark

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@liukatkat @ajbt200128
⚡