Skip to content

Skip workspace diagnostic reports that fail to convert - #1914

Merged
Dirk Bäumer (dbaeumer) merged 2 commits into
microsoft:mainfrom
trivikr:client-diagnostic-convert-report-reject
Oct 26, 2026
Merged

Dirk Bäumer (dbaeumer) merged 2 commits into
microsoft:mainfrom
trivikr:client-diagnostic-convert-report-reject

Conversation

@trivikr

Copy link
Copy Markdown
Contributor

If one report in the final workspace/diagnostic response failed to convert, the whole pull was rejected, and after 6 such failures workspace pulls stopped for the session. This logs and skips the failing report instead, the same way the partial-result handler already does.

  • DiagnosticRequestor: the final-result loop in provideWorkspaceDiagnostics now catches a failed convertReport call, logs Converting workspace diagnostics failed., and keeps the other reports. Before, a single bad item (e.g. a diagnostic with an empty message, which VS Code rejects with message must be set) dropped every report and incremented workspaceErrorCounter.
  • The progress-handler leak test from fd6d380 used this rejection to reach the finally path. It now expects the pull to resolve, and the unused expectRejection helper parameter is gone.

New test: a valid report next to one with an empty message delivers the valid report and logs the failure once.

The final workspace/diagnostic response converted its reports without
per-item error handling, unlike the partial-result handler. A single
report that failed to convert (e.g. a diagnostic with an empty message,
which VS Code rejects with "message must be set") rejected the whole
pull, so the valid reports were dropped. Since the same payload fails
on every pull, workspaceErrorCounter passed its limit after 6 attempts
and workspace pulls stopped for the session.

Log and skip the failing report, as the partial-result path does. The
progress handler leak test now expects the pull to resolve, and a new
test covers keeping the valid reports.
Copilot AI balanced review requested due to automatic review settings October 22, 2026 20:52

Copilot AI 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.

🟡 Changes recommended

The new error call enables repeated user notifications during background polling instead of only logging conversion failures.

1 open issue
What changed in this PR

This PR isolates conversion failures in final workspace diagnostic responses so one malformed report no longer rejects the entire pull or eventually stops workspace polling.

Changes:

  • Catch and log conversion failures per report while preserving valid reports.
  • Update progress-handler cleanup tests to expect successful pulls.
  • Add regression coverage for mixed valid and malformed reports.
File Description
client/​src/​common/​diagnostic.ts Skip individual reports that fail conversion in final responses.
client-node-tests/​src/​integration.test.ts Update cleanup expectations and test malformed-report isolation.

Review effort: Balanced

Comment thread client/src/common/diagnostic.ts Outdated
Copilot AI balanced review requested due to automatic review settings October 22, 2026 21:06

Copilot AI 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.

🟢 Approval recommended

The focused change matches existing partial-result handling, preserves cleanup, and includes regression coverage with no unresolved findings.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@dbaeumer
Dirk Bäumer (dbaeumer) merged commit cc5c69a into microsoft:main Oct 26, 2026
5 checks passed
@trivikr
Trivikram Kamat (trivikr) deleted the client-diagnostic-convert-report-reject branch October 26, 2026 22:05
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.

5 participants