Better error handling: show GitVersion output when JSON parsing fails#2029
Better error handling: show GitVersion output when JSON parsing fails#2029
Conversation
There was a problem hiding this comment.
Pull request overview
Improves diagnostics when GitVersion returns non-JSON output despite exiting successfully, ensuring the underlying stdout is surfaced and failures are reported consistently.
Changes:
- Include GitVersion stdout in the “output is not valid JSON” error path (with an empty-stdout fallback message).
- Adjust GitVersion’s JSON-parse failure handling to return an
ExecResultwithstderr(instead of callingsetFaileddirectly) sosafeExecutereports the right message. - Fix
safeExecutefailure reporting to preferresult.stderr, then fall back toresult.error.messageto avoid##[error]undefined.
Reviewed changes
Copilot reviewed 2 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/gitversion/runner.ts | Surface non-JSON stdout in the error and return stderr via ExecResult for consistent failure reporting. |
| src/tools/common/runner.ts | Improve failure logging/setFailed behavior by handling missing stderr safely. |
| package-lock.json | Lockfile update (removes nested TypeScript entry). |
| dist/tools/libs/tools.mjs | Built output reflecting safeExecute behavior changes. |
| dist/tools/libs/tools.mjs.map | Source map update for the common runner changes. |
| dist/tools/libs/gitversion.mjs | Built output reflecting GitVersion runner error-message changes. |
| dist/tools/libs/gitversion.mjs.map | Source map update for GitVersion runner changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/tools/gitversion/runner.ts
Outdated
| const gitVersionStdout = result.stdout?.trim() | ||
| const message = gitVersionStdout | ||
| ? `GitVersion output is not valid JSON\n${gitVersionStdout}` | ||
| : 'GitVersion output is not valid JSON, see output details' | ||
| return this.handleOutputError(message) |
There was a problem hiding this comment.
gitVersionStdout is appended verbatim into the failure message. Since exec allows up to a 10MB buffer (see src/agents/common/build-agent.ts:257), this can create extremely large error messages (noisy logs and potential truncation/command-size limits in CI log commands). Consider truncating the embedded stdout (e.g., last N lines / first N chars) and indicating that it was truncated, while still keeping the full stdout available via normal log output when possible.
There was a problem hiding this comment.
Addressed in 27c05f4. processGitVersionOutput now logs the full stdout at info level (so it remains visible in the build log), then embeds only the last 20 lines of trimmed output in the error message. When the output is longer than 20 lines, a note like (showing last 20 of N lines) is appended to the header. Three new tests cover: short output (full snippet included), long output (truncated with line-count note), and empty output (fallback message).
| if (gitVersionOutput === null) { | ||
| return this.handleOutputError('GitVersion output is not valid JSON, see output details') | ||
| const gitVersionStdout = result.stdout?.trim() | ||
| const message = gitVersionStdout | ||
| ? `GitVersion output is not valid JSON\n${gitVersionStdout}` | ||
| : 'GitVersion output is not valid JSON, see output details' | ||
| return this.handleOutputError(message) | ||
| } |
There was a problem hiding this comment.
There are existing GitVersion runner tests (src/tests/tools/gitversion/runner.spec.ts), but none cover the updated behavior when JSON extraction returns null. Please add/adjust a test to assert that processGitVersionOutput returns a non-zero ExecResult whose stderr includes the original stdout (and uses the fallback message when stdout is empty), so the new error detail remains stable.
There was a problem hiding this comment.
These tests were already added in 27c05f4 as part of addressing the previous comment. The three new tests in testOnAgent cover:
- Short stdout:
processGitVersionOutputreturnscode: -1withstderrcontaining the stdout snippet - Long stdout (30 lines): truncated to last 20 lines with
showing last 20 of 30 linesnote - Empty stdout: fallback message
'GitVersion output is not valid JSON, see output details'
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/tools/gitversion/runner.ts
Outdated
| const { truncated, wasTruncated, totalLines } = this.truncateOutput(gitVersionStdout) | ||
| const truncationNote = wasTruncated ? ` (showing last 20 of ${totalLines} lines)` : '' |
There was a problem hiding this comment.
The truncation note hardcodes last 20 while truncateOutput accepts maxLines. To avoid the message drifting if maxLines is ever changed, consider defining a maxLines const, passing it into truncateOutput, and using it in the note instead of a literal.
| const { truncated, wasTruncated, totalLines } = this.truncateOutput(gitVersionStdout) | |
| const truncationNote = wasTruncated ? ` (showing last 20 of ${totalLines} lines)` : '' | |
| const maxLines = 20 | |
| const { truncated, wasTruncated, totalLines } = this.truncateOutput(gitVersionStdout, maxLines) | |
| const truncationNote = wasTruncated ? ` (showing last ${maxLines} of ${totalLines} lines)` : '' |
src/tools/gitversion/runner.ts
Outdated
| const lines = output.split('\n') | ||
| if (lines.length <= maxLines) { | ||
| return { truncated: output, wasTruncated: false, totalLines: lines.length } |
There was a problem hiding this comment.
truncateOutput splits only on \n. If the tool emits CRLF (\r\n), the truncated snippet will retain trailing \r characters, which can make the embedded error snippet look messy. Consider splitting with /\r?\n/ (and joining with \n) to normalize line endings.
| const lines = output.split('\n') | |
| if (lines.length <= maxLines) { | |
| return { truncated: output, wasTruncated: false, totalLines: lines.length } | |
| const lines = output.split(/\r?\n/) | |
| if (lines.length <= maxLines) { | |
| return { truncated: lines.join('\n'), wasTruncated: false, totalLines: lines.length } |
|
@copilot apply changes based on the comments in this thread |
Applied both suggestions in c9a6d00:
|
|
|
Thank you @Copilot for your contribution! |



When GitVersion exits with code 0 but produces non-JSON output (e.g., due to a configuration error), the task reported a generic
GitVersion output is not valid JSONerror with no actionable detail — the actual GitVersion output was silently discarded.Changes
runner.ts(gitversion) —processGitVersionOutput: whengitVersionOutput === null, the full stdout is logged atinfolevel first (keeping it visible in the build log), then the last 20 lines are embedded in the error message with a truncation note when the output is longer:Falls back to
'GitVersion output is not valid JSON, see output details'when stdout is empty.runner.ts(gitversion) —handleOutputError: removed the directsetFailedcall; instead setsstderron the returnedExecResultand delegates failure reporting tosafeExecute. Previously,safeExecutewould overwrite the meaningful error withsetFailed(undefined)afterhandleOutputErrorhad already set the right message.runner.ts(common) —safeExecute: fixed the non-zero exit branch to checkresult.stderrfirst, then fall back toresult.error.message, rather than unconditionally castingresult.stderr as string(which produced##[error]undefinedin the logs).runner.spec.ts(gitversion) — added tests forprocessGitVersionOutputcovering: short output (full snippet included in error), long output (truncated to last 20 lines with line-count note), and empty output (fallback message).