Skip to content

fix(cli): send download id as ?id= query param to match HTTP API#467

Open
StressTestor wants to merge 2 commits into
SurgeDM:mainfrom
StressTestor:fix/cli-action-id-query-param
Open

fix(cli): send download id as ?id= query param to match HTTP API#467
StressTestor wants to merge 2 commits into
SurgeDM:mainfrom
StressTestor:fix/cli-action-id-query-param

Conversation

@StressTestor
Copy link
Copy Markdown

@StressTestor StressTestor commented May 31, 2026

What

The CLI's ExecuteAPIAction (cmd/utils.go) built the request path as <endpoint>/<id> (e.g. POST /pause/<id>), but the HTTP API registers exact routes (/pause, /resume, /delete, /open-file, /open-folder, …) and reads the download id from the id query parameter via withRequiredID (cmd/http_api.go). A path segment doesn't match the registered routes, so pause, resume, rm, and open returned 404 Not Found against a remote Surge daemon.

Fix

Send the id as ?id=<url-escaped>, matching the server's contract — and what the browser extension (/pause?id=...) and internal/core/remote_service.go (/pause?id=" + url.QueryEscape(id)) already use. One-line change in ExecuteAPIAction; the CLI was the only client using the path form.

Testing

  • Added TestExecuteAPIAction_SendsIDAsQueryParam (cmd/http_api_test.go): it registers the real routes via registerHTTPRoutes + httptest.NewServer, points the CLI at it via SURGE_HOST, and asserts the server receives the id.
  • Reproduced + fixed: with the old <endpoint>/<id> path the test fails with server error: 404 Not Found; with ?id= it passes.
  • go test ./..., gofmt, go vet ./..., and golangci-lint run (v2) all clean.

Fixes #456

Greptile Summary

This PR fixes a 404 regression in the CLI's ExecuteAPIAction function: it was building request paths as <endpoint>/<id> (path segment form), but the HTTP server only registers exact routes and reads the download ID from the ?id= query parameter via withRequiredID. The fix aligns the CLI with the browser extension and internal/core/remote_service.go, which already used the correct query-parameter form.

  • cmd/utils.go: One-line change replacing fmt.Sprintf(\"%s/%s\", endpoint, id) with fmt.Sprintf(\"%s?id=%s\", endpoint, url.QueryEscape(id)), with an explanatory comment.
  • cmd/http_api_test.go: Adds TestExecuteAPIAction_SendsIDAsQueryParam, a regression test that registers real HTTP routes via registerHTTPRoutes, starts a test server, and asserts all three ExecuteAPIAction callers (pause, resume, delete) correctly deliver the ID as a query parameter.

Confidence Score: 5/5

Safe to merge — the change is a targeted one-line fix to a well-understood API contract mismatch, backed by a regression test against real routes.

The fix is minimal and precisely targeted: it corrects the query-parameter format in a single function that all affected CLI commands share, and the new test exercises all three callers against a real HTTP mux to prevent future regressions.

No files require special attention.

Important Files Changed

Filename Overview
cmd/utils.go One-line fix: sends download ID as ?id= query parameter instead of a path segment; correctly uses url.QueryEscape and adds an explanatory comment.
cmd/http_api_test.go Adds a regression test covering all three ExecuteAPIAction callers (pause/resume/delete) against a real registered HTTP mux; test structure and assertions are correct.

Reviews (3): Last reviewed commit: "test(cli): cover resume and delete in Ex..." | Re-trigger Greptile

Comment thread cmd/http_api_test.go
StressTestor pushed a commit to StressTestor/Surge that referenced this pull request May 31, 2026
Broaden the SurgeDM#456 regression test to exercise every ExecuteAPIAction caller
(pause/resume/delete) rather than just pause, so a future action-specific
routing regression is caught. Addresses review feedback on SurgeDM#467.
Copy link
Copy Markdown
Member

@SuperCoolPencil SuperCoolPencil left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for contributing 🚀

StressTestor added a commit to StressTestor/Surge that referenced this pull request Jun 1, 2026
Broaden the SurgeDM#456 regression test to exercise every ExecuteAPIAction caller
(pause/resume/delete) rather than just pause, so a future action-specific
routing regression is caught. Addresses review feedback on SurgeDM#467.
ExecuteAPIAction built the request path as "<endpoint>/<id>" (e.g.
POST /pause/<id>), but the HTTP API registers exact routes and reads the id
from the "id" query parameter (withRequiredID in cmd/http_api.go), so
pause/resume/delete/open returned 404 against a remote daemon. Send the id as
?id= (url-escaped), matching the browser extension and
internal/core/remote_service.go.

Fixes SurgeDM#456
Broaden the SurgeDM#456 regression test to exercise every ExecuteAPIAction caller
(pause/resume/delete) rather than just pause, so a future action-specific
routing regression is caught. Addresses review feedback on SurgeDM#467.
@StressTestor StressTestor force-pushed the fix/cli-action-id-query-param branch from 4ed0a4f to 9ce9dc6 Compare June 1, 2026 08:31
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.

[Bug] CLI: pause, resume, delete, and open actions are broken when targeting a remote server

2 participants