Skip to content

fix: report unknown release versions instead of a server null reference - #696

Open
NickJosevski wants to merge 5 commits into
mainfrom
nj/issue-294
Open

fix: report unknown release versions instead of a server null reference#696
NickJosevski wants to merge 5 commits into
mainfrom
nj/issue-294

Conversation

@NickJosevski

@NickJosevski NickJosevski commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Refs #294

The bug

octopus release deploy --version latest (or any unknown version) passes the string straight to the executions API. In automation mode nothing looks the release up first, so the server tries to resolve a release named latest, fails, and NREs:

Octopus API error: Object reference not set to an instance of an object. []

This PR fixes the error message only. The latest feature is not implemented — see the open decision below.

What changed

New selectors.FindRelease (pkg/question/selectors/releases.go), alongside the existing FindProject / FindChannel / FindRunbook. It resolves the release before deploying and returns a typed ReleaseNotFoundError that names what was asked for:

cannot find a release with version 'latest' in project 'Fire Project'. 'latest' is not a
supported alias, specify an exact version. Run 'octopus release list --project "Fire Project"'
to see the available versions

Callers use errors.As to separate "no such release" (fatal) from every other failure — a permissions error or a 5xx is returned untouched and the deploy proceeds, so this new lookup cannot fail a deploy that previously succeeded.

Applied to release deploy (automation and interactive paths) and release progression allow/prevent, which now share one lookup and one message. Resolving up front also means options.ReleaseID is known before the POST, so the lookups that used to run after deploying just to print the "View this release" link are gone.

Net API calls for release deploy: −2 for table output, +1 for --output-format basic|json.

Verified against a live server

Octopus Server 2026.3.14820 (Cloud). GET /api/{space}/projects/{id}/releases/{missing} returns 404 with a JSON APIError body, and never sends Content-Length (HTTP/2 omits it, HTTP/1.1 is chunked). Go sees ContentLength = -1, so the == 0 short-circuit in the SDK's DoRawJsonRequest never fires and real lookups arrive as Confirmed: true.

ReleaseNotFoundError.Confirmed hedges the wording if a bodyless response ever does arrive (a reverse proxy emitting Content-Length: 0). That is a guard, not an expected path.

Tests

go build ./... clean. go test ./pkg/... — 60 packages ok. go vet ./pkg/... reports only the four pre-existing unreachable code findings in worker/shared, workerpool/shared, tenant/variables/list.

Known gap: the latest case in TestDeployCreate_AutomationMode mocks a bodyless 404, which the live server never sends, so it asserts the hedged wording rather than the message users actually get. Should be switched to a body-bearing 404 before merge.

Open decision — needs a maintainer call

benPearce1 called dropping latest intentional ("latest by time or latest by semver?"); DamienDaco wants the old octo behaviour back. That is a product call, so this PR stops at the error message.

  • (a) --version latest = newest by Assembled date. Matches old octo, no new flags. Re-introduces the ambiguity that got it removed, and collides with a release literally named latest.
  • (b) --latest-by time|semver, mutually exclusive with --version. Unambiguous and self-documenting. Costs new flag surface, and needs calls on whether it extends to runbook run and whether --channel scopes it.
  • (c) Ship only this error fix. The message points at octopus release list, so the failure is self-service.

Recommendation: (b), scoped to release deploy first — gives DamienDaco the capability without the ambiguity. If octo parity matters more, (a) is much smaller and I will swap. Either choice deletes the latest branch of ReleaseNotFoundError.

🤖 Generated with Claude Code

`release deploy` passed --version straight to the executions API, which
answers an unknown version with "Object reference not set to an instance
of an object". Resolve the release before deploying so a version that
doesn't exist is reported by name, and call out `latest` explicitly since
it is not a supported alias.

Refs #294

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
return nil, err
}
// a 404 with an empty body doesn't reach the error path above; it decodes as an empty release
if release == nil || release.GetID() == "" {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The empty-ID fallback is status-blind: the SDK's DoRawJsonRequest returns resp, nil for any status code when ContentLength == 0 (not just 404), and DoRequest then hands back a zero-valued Release with a nil error. So a 502/503 from a proxy, or a 403 with an empty body, lands here and gets reported as cannot find a release with version '1.0' in project 'X' — a misleading not-found during an outage or permission failure, for release deploy and release progression allow/prevent alike. Worth hedging the message (e.g. "could not resolve a release...") or noting the limitation, since the real status code is unrecoverable at this layer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actioned in 2541136. The fallback now returns a typed selectors.ReleaseNotFoundError with a Confirmed flag: a 404 carrying an APIError body sets Confirmed: true and keeps the definite "cannot find a release with version ... " wording, while the empty-body path leaves it false and hedges — "could not resolve a release with version 'X' in project 'Y'; the server returned an empty response, which usually means there is no such release, but can also mean the lookup itself failed". The DoRawJsonRequest short-circuit is written up in the doc comment on the type so the reason for hedging survives.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up after checking a live instance (md.octopus.app, Octopus Server 2026.3.14820) — full HTTP evidence in the pre-flight thread.

The hedged wording is sound as a guard, but the real server never takes that branch: the 404 carries an APIError body, and the endpoint never sends Content-Length (HTTP/2 omits it, HTTP/1.1 is chunked), so resp.ContentLength is -1 and the == 0 short-circuit never fires. Real lookups come back Confirmed: true.

That surfaces a test problem worth fixing before merge. The 'latest' case at deploy_test.go:1636 mocks a bodyless 404:

api.ExpectRequest(t, "GET", ".../releases/latest").RespondWithStatus(404, "NotFound", nil)

and therefore asserts the hedged message. But latest is the headline scenario from #294, and against a real server it produces the confirmed message instead:

cannot find a release with version 'latest' in project 'Cycle'. 'latest' is not a supported alias,
specify an exact version. Run 'octopus release list --project "Cycle"' to see the available versions

(verified end-to-end through selectors.FindRelease against the live instance). So the one test covering the bug this PR exists to fix is asserting a message that users will not see. Suggest switching that mock to a 404 with an APIError body — matching the sibling test at deploy_test.go:1617 — and asserting the confirmed wording. If you want to keep coverage of the bodyless shape, it is better as a separate case named for what it is (a proxy/edge response), not as the latest case.

if options.ReleaseVersion != "" {
// resolve the release up front; the executions API reports an unknown version as an
// unhelpful null reference error, and having the ID saves looking it up again later
release, err := selectors.FindRelease(octopus, f.GetCurrentSpace().ID, project, options.ReleaseVersion)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Behavioral note: this pre-flight makes every automation-mode deploy depend on being able to GET the release (ReleaseView), which the old flow never required — the executions API only ever saw the version string. A CI service account scoped to deploy but not to read releases (or a transient 5xx on this GET) now aborts a deploy that previously succeeded, since non-404 errors are returned untouched. Probably an acceptable trade, but worth a conscious decision — an alternative is to treat only a definitive 404 as fatal and fall through to the POST on any other lookup failure.

@NickJosevski NickJosevski Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actioned in 14ff2ee. The pre-flight now only aborts on selectors.ReleaseNotFoundError (via errors.As); any other error leaves options.ReleaseID empty and falls through to the POST, letting the server stay the authority. New automation-mode test covers a 403 from the release GET still reaching the deploy.

One residual worth naming, because it is narrower than it looks. The fatal/non-fatal split is not really "was it a 404" — it is "did the response carry a body". DoRawJsonRequest short-circuits on resp.ContentLength == 0 before it ever reads the status, so the status code is only available for responses that had a decodable JSON body:

  • 404 with an APIError body → Confirmed: true → fatal (correct)
  • 404 with an empty body → Confirmed: false → fatal (correct, and the reason this case cannot be made non-fatal — it would put the NRE back)
  • 403 / 502 with a body → returned untouched → non-fatal, deploy proceeds (what the new test covers)
  • 403 / 502 with Content-Length: 0 → indistinguishable from the row above it → still fatal, reported as "could not resolve …"

So the mitigation covers failures that come back with a body, and the bodyless non-404 is the leftover. Note a genuine proxy 502 usually has an HTML body, which fails to decode as APIError and returns a decode error — also non-fatal. That leaves the gap at explicitly bodyless non-404 responses, which is a small set.

The question that would close it properly: does Octopus GET /api/{space}/projects/{id}/releases/{version} return a 404 with an APIError body for a version that does not exist? If it always does, the empty-body branch is only ever reached by non-404s and could safely be non-fatal, collapsing the residual entirely. This PR handles both shapes defensively but does not establish which one the server actually sends — worth confirming against a real instance before deciding.

@NickJosevski NickJosevski Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checked against a live instance (md.octopus.app, Octopus Server 2026.3.14820, Octopus Cloud). The residual is not reachable there — Confirmed: true is the real path.

Raw HTTP for GET /api/Spaces-1/projects/{id}/releases/{missing-version}:

HTTP/2 404
content-type: application/json; charset=UTF-8

{ "ErrorMessage": "Release '9.9.9-does-not-exist' for project 'Cycle' was not found." }

The 404 carries an APIError body, so it decodes on the error path and arrives as Confirmed: true.

The framing detail matters more than the body, though. This endpoint never sends Content-Length — HTTP/2 omits it, and forcing --http1.1 gives transfer-encoding: chunked. Go sets resp.ContentLength = -1 in both cases, so resp.ContentLength == 0 in DoRawJsonRequest is false and the short-circuit is never taken at all, regardless of status code. Verified through the real SDK rather than inferred:

RAW: proto=HTTP/2.0 status=404 ContentLength=-1  -> short-circuit (==0)? false

--- FindRelease("9.9.9-nope") ---
  ReleaseNotFoundError, Confirmed=true
--- FindRelease("latest") ---
  ReleaseNotFoundError, Confirmed=true
--- FindRelease("0.0.23") ---
  OK, release ID=Releases-1094

So on this server the Confirmed: false branch is unreachable, which also means the bodyless-403/502 case I worried about cannot arise from the server itself. It could still arise from something in front of it (a customer reverse proxy emitting Content-Length: 0), so the branch is worth keeping as a guard — but as a guard, not as an expected path. I would leave 14ff2ee as-is; it costs nothing and the errors.As split is the right shape either way.

Comment thread pkg/cmd/release/deploy/deploy.go Outdated
if err != nil {
return err
}
options.ReleaseID = release.ID

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With this in place, the fallback web-URL lookup below (lines ~362-374, if releaseID == "") is now dead code: the interactive path always sets options.ReleaseID (AskQuestions line ~450), and in automation mode the executor rejects the deploy unless both ProjectName and ReleaseVersion are non-empty — exactly the condition under which this block sets ReleaseID. The FindProject + GetReleaseInProject fallback and its "we may already have the release ID from AskQuestions" comment could be deleted (or the comment updated if you want to keep it as a defensive belt-and-braces).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actioned in cf2dc4c. The FindProject + GetReleaseInProject fallback is gone; the block is now just if releaseID := options.ReleaseID; releaseID != "", with the comment rewritten to say why it can be trusted (both paths resolve the release before reaching here, and the only way it is empty is a lookup we deliberately ignored, which would fail again if repeated).

}

return existingRelease, nil
return selectors.FindRelease(octopus, octopus.GetSpaceID(), project, version)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Minor: shared.FindRelease is now a pure one-line passthrough — GetReleaseID (and any other caller) could call selectors.FindRelease directly and this wrapper could go. Related pre-existing wart this touch makes more visible: GetReleaseID takes a spaceID parameter that is never used — the lookup always uses octopus.GetSpaceID() — so passing the selector call the spaceID argument here would also let you drop the ignored parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actioned in 4e9ec94. The passthrough wrapper is deleted and GetReleaseID calls selectors.FindRelease directly. Rather than dropping the ignored spaceID parameter it now gets used — both callers (allow.go:95, prevent.go:100) already pass opts.Client.GetSpaceID(), so this is identical to the old hardcoded octopus.GetSpaceID() behaviour, just no longer a lie in the signature.

NickJosevski and others added 4 commits August 31, 2026 12:08
The SDK's DoRawJsonRequest short-circuits on `resp.ContentLength == 0` and
returns `(resp, nil)` for any status code, so DoRequest hands back a
zero-valued Release with a nil error. A 404 with no body lands there, but so
does a 403 with an empty body or a 502 from a proxy, and the status code is
not recoverable at this layer.

Reporting all of those as "cannot find a release with version X" is
misleading during an outage or a permissions failure. Introduce
selectors.ReleaseNotFoundError, which records whether the server confirmed
the answer with a 404 carrying an APIError body, and hedge the wording when
it did not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the release resolved before the deploy, `options.ReleaseID` is always
set on both paths that reach the link: AskQuestions in interactive mode, the
pre-flight lookup in automation mode (the executor rejects the deploy unless
both ProjectName and ReleaseVersion are set, which is exactly when the
pre-flight runs). The FindProject + GetReleaseInProject fallback can only run
when the pre-flight lookup already failed, where repeating it would fail too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pre-flight lookup is new to `release deploy`; before it, the automation
path never read the release and the executions API only ever saw the version
string. Failing the whole deploy on any lookup error would break a CI service
account scoped to deploy but not to ReleaseView, and would turn a transient
5xx on that GET into an aborted deployment that previously succeeded.

Fail only on a ReleaseNotFoundError, which is the case issue #294 is about.
For anything else, carry on without the release ID and let the server remain
the authority on permissions and availability.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shared.FindRelease was left as a one-line passthrough, so remove it. Doing so
also puts GetReleaseID's spaceID parameter to use — it was accepted and then
ignored in favour of octopus.GetSpaceID(). Both callers already pass
opts.Client.GetSpaceID(), so the resolved space is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@NickJosevski
NickJosevski marked this pull request as ready for review September 4, 2026 07:19
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.

1 participant