fix: stop --dry-run reporting an API error on commands that return 201 - #39
Closed
Bradenream wants to merge 2 commits into
Closed
Bradenream wants to merge 2 commits into
Bradenream wants to merge 2 commits into
Conversation
--dry-run swaps the HTTP client for one that prints the request and, instead of sending it, hands the SDK a stand-in 200. The SDK accepts only an operation's documented success code, and 28 of the 191 operations succeed with 201 alone: every create, plus environment clone and publish, evaluation run and test run create. Their dry runs printed the preview, then "API Error (HTTP 200): unknown status code returned", and exited 1. The stand-in response now carries an X-Vf-Dry-Run header. output.Error, which every generated command reports SDK errors through, does not report an error about that response. Errors raised before the preview, such as a body that fails to serialize, carry no response and are still reported. Commands that already worked are unchanged. Checked against all 191 API commands, run with placeholder flags. Of the ones the placeholders could build, master printed the preview and then failed 23; with this change none fail. The other 5 of the 28 were checked by hand: each failed on master and now exits 0.
4 of 5 tasks
4 of 5 tasks
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Error suppression must also verify that dry-run mode is active to prevent real server responses from being treated as success.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes --dry-run failures for API operations expecting HTTP 201.
Changes:
- Marks synthetic dry-run responses and suppresses their SDK status errors.
- Adds unit and behavior coverage for dry-run success and genuine failures.
| File | Description |
|---|---|
internal/client/diagnostics.go |
Marks synthetic responses. |
internal/client/diagnostics_test.go |
Tests response marking. |
internal/output/dryrun.go |
Detects marked responses. |
internal/output/dryrun_test.go |
Tests error suppression. |
internal/output/output.go |
Ignores synthetic-response errors. |
test/dry-run.test.ts |
Adds end-to-end coverage. |
Files not reviewed (2)
- internal/client/diagnostics.go: Generated file
- internal/output/output.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
output.Error skipped any error whose response carried X-Vf-Dry-Run. A header is something any server reached with --server-url can send, so a server could have turned its own error into a silent exit 0, without a dry run ever being asked for. Copilot raised this in review. The marker now counts only while --dry-run is on. During a dry run nothing is sent, so the stand-in is the only response there can be. A test server that answers a real request with the marker and an unexpected status shows the difference: before, vf exited 0; now it reports the error and exits 1.
effervescentia
approved these changes
Oct 2, 2026
Contributor
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--dry-runswaps the HTTP client for one that prints the request and, instead of sending it, hands the SDK a stand-in200. The SDK accepts only an operation's documented success code. 28 of the 191 operations succeed with201alone: everycreate, plusenvironment clone,environment publish,evaluation runandtest run create. Their dry runs printed the preview, thenAPI Error (HTTP 200): unknown status code returned, and exited 1.The stand-in response now carries an
X-Vf-Dry-Runheader. While--dry-runis on,output.Error, which every generated command reports SDK errors through, does not report an error about that response (internal/output/dryrun.go). The flag check matters: a header is something any server reached with--server-urlcould send, and it must not turn that server's error into a silent success. Errors raised before the preview, such as a body that fails to serialize, carry no response and are still reported. Commands that already worked are unchanged, including the{}that a200operation prints with--output-format json.Before and after
test/dry-run.test.ts(6 cases)Test plan
gofmt,go vet ./...andgo test ./...pass;go.modis unchanged.internal/output/dryrun_test.go: the stand-in is not reported, in human and agent mode; a real response and a pre-send error still are.internal/client/diagnostics_test.go: the dry-run client marks its stand-in. Removing either half of the fix fails its test.test/dry-run.test.ts:project create,environment publishandagent updatepreview and exit 0. Agent mode and JSON output report no error. A request that cannot be built still fails, and so does a real response that carries the marker without--dry-run.CLI behaviourshows master's 4 existing failures until fix: restore vf docs search, and make the test suite hermetic and green #37 merges.