fix: Coolify v4.2 compatibility without breaking pre-4.2 instances - #296
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
+ Coverage 91.61% 91.91% +0.30%
==========================================
Files 3 3
Lines 620 643 +23
Branches 163 169 +6
==========================================
+ Hits 568 591 +23
Misses 6 6
Partials 46 46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code ReviewOverviewThis PR fixes Coolify v4.2 compatibility by updating six state-changing API call sites from GET to POST, while preserving backward compatibility with pre-4.2 instances through a careful POST-first/GET-fallback strategy for the three endpoints that genuinely diverge between versions. It also correctly widens three response-type fields to optional to match v4.2's secret-hiding behaviour. The PR description is excellent — the upstream What's GoodCompatibility strategy is sound. The decision to split endpoints into two groups — unconditional POST (already
Per-endpoint caching is correct. Keying on a semantic name ( Test coverage is comprehensive. The 12 new tests cover every meaningful case: POST-first success, 405→GET fallback, cache hit on second call, cache isolation between endpoints, both 405 body variants, non-retry on 404 and 500, GET-retry failure propagation, and no caching on a failed GET retry. That last case (no caching on unproven fallback) is a subtle correctness property and it's good to see it tested. Type widening is conservative and well-scoped. Making Issues / Suggestions[Minor] 405
if (status === 405) {
return 'Coolify v4.2 moved state-changing endpoints from GET to POST. This client retries on 405, so if you see this error it means neither method was accepted — check the endpoint path or Coolify version.';
}The current wording confidently explains the retry behavior but may confuse users hitting a genuine 405 on a different endpoint. [Minor] Runtime upgrade silently sticks to the wrong method.
This is a reasonable trade-off (restoring the MCP server clears it), but it's worth a brief note either in the JSDoc or CLAUDE.md's gotchas section so it's easy to diagnose if it surfaces in production. [Minor] There's nothing type-level preventing a future call site from accidentally reusing an existing key (e.g. const LEGACY_ENDPOINT_KEYS = {
SERVERS_VALIDATE: 'servers.validate',
API_ENABLE: 'api.enable',
API_DISABLE: 'api.disable',
} as const;Not a blocker — just something to consider if this method grows more call sites. [Informational] The lock file diff is large (routine Babel/Hono/etc. bumps) but is stacked on Test Coverage CheckThe existing The new SummaryThis is a well-researched, well-implemented fix. The correctness properties are strong, the test suite covers the subtle edge cases thoroughly, and the type changes are minimal and precise. The notes above are minor — the 405 hint scope is the most user-visible one. Happy to approve once that's considered. |
Addresses review feedback on #296. The remembered-GET path was terminal: once an endpoint was cached as legacy-GET, a Coolify instance upgraded to v4.2 mid-session would 405 on every subsequent call until the MCP server restarted. The cache now self-heals in both directions — a 405 from a remembered GET drops the stale preference and re-probes POST. A non-405 failure from the remembered GET still propagates without re-probing, so a genuine error never triggers a second state-changing request. The fallback key is now a closed union (LEGACY_GET_ENDPOINTS) instead of a bare string, so a future call site cannot silently reuse another endpoint's cache entry. Documented why the key is an endpoint identifier rather than the request path: version compatibility is a property of the instance, not the resource, so keying on the path would re-probe per uuid. The 405 hint no longer asserts the v4.2 GET-to-POST story for every 405. It led with that explanation regardless of endpoint, which would mislead anyone hitting a genuine 405 elsewhere. Also recorded three Coolify API gotchas in CLAUDE.md: the non-uniform v4.2 method split, v4.2 secret-hiding plus Member-role read-only, and the preview-twin env var hook (see #257) with the warning never to dedupe env vars by key alone.
Code ReviewPR: fix: Coolify v4.2 compatibility without breaking pre-4.2 instances OverviewThis PR fixes a real-world breakage: Coolify v4.2 made six state-changing endpoints POST-only (returning 405 for GET), while older versions accepted GET on some of them. The approach correctly splits those endpoints into two groups:
What's GoodThe
The Test coverage is thorough: 12 new tests exercise the main path, the fallback, per-endpoint cache isolation, the self-healing upgrade scenario, non-retry on 500 (partial execution risk), failed-GET not caching, and the pre-v4.2 Laravel 405 body vs. the v4.2-style body. Type changes are correctly scoped: Issues and Suggestions1. Minor: 405 error hint doesn't match the test assertion The test at But the current hint text reads:
The phrase "GET to POST" is present, so the test passes — but the hint also says "this client already sends POST and retries with GET, so a 405 reaching you means neither was accepted." This is slightly misleading for a user who has already upgraded to v4.2 and is somehow getting a 405 on a non-covered endpoint. Consider making the v4.2 guidance more concise and separating it from the "if you see this, both methods failed" explanation. 2. Minor: The cache isolation test could be more explicit In 3. Edge case: Double-fallback on re-probe When a remembered GET returns 405 (instance upgraded), the code deletes the key and falls through to the POST block. If the POST block also gets a 405, it tries GET again and caches the key on success. This creates a scenario where:
This is correct behavior and would be practically impossible (both methods failing in different orders), but a brief comment in the fall-through branch noting this intentional delegation would help a reader who hasn't read the test suite. 4. Minor: Comment verbosity vs. CLAUDE.md guideline The project guideline says "default to writing no comments" — but 5. The PR description notes this is stacked on Correctness VerdictThe fallback mechanism is correct. The key invariants are all satisfied:
The type changes are accurate and appropriately scoped. SummaryThis is well-engineered. The approach of observed-behaviour keying over version probing is the right call, and the test coverage of the cache edge cases (especially the mid-session upgrade scenario) is solid. The issues above are minor — nothing blocking merge once Verdict: Approve after |
Review feedback on #296: the hint led with an explanation of the client's own retry internals, which reads as noise to anyone hitting a 405 on an endpoint the fallback does not cover. Says what changed and what to check, without narrating the implementation.
Code Review: Coolify v4.2 CompatibilityThis is a well-designed, well-tested fix. The problem analysis is thorough — verifying upstream What the PR does
Code qualityThe fallback logic is correct and handles the edge cases cleanly:
Minor observations
private async postWithLegacyGetFallback<T>(...): Promise<T> {
const isMethodNotAllowed = (error: unknown): boolean => // ← new closure every call
error instanceof CoolifyApiError && error.status === 405;It could be a module-level
Test label "defensive, direction-agnostic" on the v4.2-style 405 test could be clarified. In practice a v4.2 instance shouldn't 405 a POST to GET retry returning 405 isn't explicitly tested (the Test coverage12 new tests are high quality and cover the real edge cases:
The seven updated tests (GET→POST for services and deploy) are correct and necessary. Type changesMaking The masking downstream ( DocumentationCHANGELOG, README, and CLAUDE.md are all updated. The CLAUDE.md addition is particularly useful for future contributors — it explains the SummaryCorrect, well-tested, well-documented. The only actionable suggestion is extracting the |
Coolify v4.2 (coollabsio/coolify#10872) requires POST on state-changing endpoints and returns a hard 405 for GET. Six client call sites still sent GET: service start/stop/restart, deployByTagOrUuid, enableApi/disableApi, and validateServer (which passed no method, so it defaulted to GET). Checked upstream routes/api.php at v4.1.2, v4.0.0 and older betas to work out whether fixing this breaks anyone still on 4.1. It splits in two: - Service start/stop/restart and /deploy were already registered Route::match(['get','post']) well before v4.2, so they now send POST unconditionally. Application and database start/stop have been POSTing against 4.1 estates all along, which is the same route shape, so this is proven rather than assumed. - /enable, /disable and /servers/{uuid}/validate genuinely diverge: Route::get only up to v4.1.2, Route::post only from v4.2. Neither method works everywhere, so these send POST and retry once with GET on a 405, caching the resolved method per endpoint. The retry is safe because a 405 is raised by the router before the controller runs, so nothing executed and no state change can double-fire. Only 405 triggers the fallback; every other status propagates untouched, and a failed GET retry is not cached so it re-probes rather than trusting an unproven fallback. request() now throws CoolifyApiError carrying the HTTP status, since the fallback needs to distinguish 405 from everything else and the message text alone could not. The message is unchanged, so anything matching on error.message is unaffected. Also from #292: PrivateKey.private_key, EnvironmentVariable.value and EnvVarSummary.value are now optional, because v4.2 (coollabsio/coolify#9893) withholds secrets unless the token has sensitive-read scope, and a required type made that arrive as a silent undefined. Create*Request types are unchanged, as request payloads are unaffected. 405 now carries an error hint explaining the method move, and the 401/403 hint mentions v4.2 Member-role tokens being read-only. Reported by @StreamlinedStartup with the call sites already identified. Closes #292
Addresses review feedback on #296. The remembered-GET path was terminal: once an endpoint was cached as legacy-GET, a Coolify instance upgraded to v4.2 mid-session would 405 on every subsequent call until the MCP server restarted. The cache now self-heals in both directions — a 405 from a remembered GET drops the stale preference and re-probes POST. A non-405 failure from the remembered GET still propagates without re-probing, so a genuine error never triggers a second state-changing request. The fallback key is now a closed union (LEGACY_GET_ENDPOINTS) instead of a bare string, so a future call site cannot silently reuse another endpoint's cache entry. Documented why the key is an endpoint identifier rather than the request path: version compatibility is a property of the instance, not the resource, so keying on the path would re-probe per uuid. The 405 hint no longer asserts the v4.2 GET-to-POST story for every 405. It led with that explanation regardless of endpoint, which would mislead anyone hitting a genuine 405 elsewhere. Also recorded three Coolify API gotchas in CLAUDE.md: the non-uniform v4.2 method split, v4.2 secret-hiding plus Member-role read-only, and the preview-twin env var hook (see #257) with the warning never to dedupe env vars by key alone.
Review feedback on #296: the hint led with an explanation of the client's own retry internals, which reads as noise to anyone hitting a 405 on an endpoint the fallback does not cover. Says what changed and what to check, without narrating the implementation.
158fc5f to
203f050
Compare
Code ReviewOverall: excellent work. The problem is well-researched, the implementation is clean, and the test coverage is thorough. A few minor observations below, but nothing blocking. What this PR doesv4.2 changed six state-changing endpoints from GET to POST, returning hard 405s for the old method. The PR fixes them in two groups:
Also makes three response fields optional ( Strengths
Minor observations1. The 405 // coolify-client.ts ~398
if (status === 405) {
return 'Coolify v4.2 moved state-changing endpoints from GET to POST; older versions accept GET only. This client retries automatically, so a 405 reaching you means both methods were rejected …';
}The phrase "This client retries automatically" implies the fallback applies to all 405s, but it only applies to the three endpoints in 2. Concurrent callers to the same fallback endpoint can both probe (benign) If two concurrent calls hit 3. Nit: the "caches per endpoint, not globally" test could assert the final result // coolify-client.test.ts
it('caches per endpoint, not globally', async () => {
// …
await client.enableApi();
await client.disableApi(); // <-- resolves to { message: 'API disabled.' } but this isn't asserted
expect(mockFetch).toHaveBeenNthCalledWith(3, …, expect.objectContaining({ method: 'POST' }));
});The test correctly pins the method but doesn't assert that ConclusionThe research in the PR description (diffing Happy to approve once the stacked PR #295 merges. |
The PR review workflow was pinned to claude-sonnet-4-6 and the interactive @claude workflow to claude-opus-4-7. Both now use claude-opus-5. The review one is the change that matters: it runs on every PR and its findings have already caught real defects — a terminal cache in the v4.2 method fallback (#296) and an unverified is_preview assumption (#297) — so it is worth running on the strongest model available.
#312) * fix: fall back to GET on 404, not just 405 — the v4.2 path never fired A regression shipped in 2.15.0 that broke system enable_api / disable_api and validate_server on exactly the Coolify versions the fallback existed to support. postWithLegacyGetFallback retried with GET only on a 405. Coolify ends routes/api.php with a catch-all — Route::any('/{any}', ...) returning 404 "Not found." — which swallows an unmatched method+path before Laravel can raise a 405. So a pre-4.2 instance answers POST /enable with 404, the fallback never triggered, and the call failed outright where it had worked before #296. The route archaeology behind #296 was right about which methods each version registers and wrong about what a rejected method returns. Only running it against a real Coolify 4.1.2 showed the difference. 404 is now treated as a method rejection alongside 405. Safe for the same reason: the catch-all runs no controller, and a genuine "resource not found" 404 makes the GET retry return the same 404, so the correct error still surfaces at the cost of one extra request. A 500 still propagates untouched. Adds v42-compat.integration.test.ts, which asserts pre-4.2 behaviour on an older instance and v4.2 behaviour on a newer one, so it is meaningful either way. Every assertion is side-effect free: a rejected method executes nothing, and /deploy with a tag matching no resource proves the method was accepted without deploying anything. /disable is never called. Also gates the log integration tests on the instance version, so v4.2-only endpoints report as genuinely SKIPPED on an older box rather than failing. Resolved at module scope so jest skips the suites outright — an early return inside a test still shows a green tick, which is the false confidence these tests exist to prevent. dotenv now runs with override: true, because an empty COOLIFY_URL in the ambient environment otherwise wins over .env and silently skips everything. Verified against a live Coolify 4.1.2: 7 passed, 3 skipped as v4.2-only. * fix: apply review findings on #312 Four findings, one of which could have disabled the API on a live instance. 1. parseMajorMinor returned a float, so "4.10.0" became 4.1 and compared as OLDER than 4.2. That routed a 4.10 instance into the pre-4.2 branch, which fired a real POST /disable at a box that accepts it — turning off the API the client depends on, in a file whose docblock promises exactly the opposite. Now returns a [major, minor] tuple with an explicit compare, and /disable is dropped from the probes entirely: /enable and /servers/{uuid}/validate already prove the same routing property, and /disable carried the only irreversible downside in the suite. 2. The v4.2 branch asserted GET /enable returns 405. The catch-all is still the last route on 4.2, so 404 is equally possible — that block is skipped on a 4.1.2 box, so the assertion had never executed and would likely have failed the first time anyone ran it on 4.2. Now asserts the rejection set. The acceptance assertions used `not.toBe(405)`, which a 404 also satisfies while proving the opposite; they now assert status < 400. 3. Widening the fallback to any 404 discarded the informative error. On 4.2, validateServer('bad-uuid') would POST, get a genuine controller 404, be treated as a method rejection, retry GET, and surface the retry's error instead of "Server not found." Fixed at the root rather than approximately: CoolifyApiError now carries the parsed body, and the fallback keys off the catch-all's signature — it returns {message, docs} and no controller 404 carries a `docs` key. A controller's "not found" is a real answer and no longer triggers a retry at all. If the GET probe does fail, the original POST error is rethrown, since that is the one describing what the caller asked for. 4. Cache thrash on the remembered-GET path falls out of the same fix: a genuine 404 no longer invalidates the cached preference. Also: CLAUDE.md still taught "only ever trigger that fallback on a 405", the mental model this work disproved; stale 405 comments in the compat suite where the measured value is 404; a shared integration helpers module so the version-parse fix lives in one place rather than two; and COOLIFY_URL is trimmed of trailing slashes, which would otherwise produce //api/v1/... and make every "method rejected" assertion pass for the wrong reason. The /deploy probe needed the same routing-vs-controller distinction as the client: a tag matching nothing returns a controller 404, so asserting on status alone confused "nothing to deploy" with "POST rejected". It now checks the body signature, which is the point being proven anyway. Verified against live Coolify 4.1.2: 6 passed, 3 skipped as v4.2-only. * fix: apply re-review findings on #312 1. On a pre-4.2 instance the fix was throwing away the informative error. When the GET retry also failed, the POST error was rethrown — but the POST is a routing miss by construction (isMethodRejected already established nothing ran), so it says nothing about the request. The GET is the only one that reached a controller. validateServer('bad-uuid') therefore reported the catch-all's bare "Not found." — complete with the irrelevant "uuid may belong to a different resource type" hint — instead of the controller's "Server not found.". Now the GET error wins unless it was itself a method rejection, in which case neither routed and the POST error is the right one. 2. CHANGELOG still described the first commit's plain-404 behaviour rather than the body-shape matching that shipped. Synced. 3. The integration rejection assertions checked `[404, 405]` contains the status, which any 404 satisfies — including one from a wrong base URL or a proxy — so the suite could pass without proving anything. They now use rawProbe's `routed` predicate, which also exercises the catch-all's `docs` signature against a live instance. That signature is what the whole fix hinges on and nothing had asserted it end to end. 4. isRoutingCatchAll keyed solely off the `docs` key, so a Coolify version or proxy that dropped it would silently stop the fallback firing — the same invisible-failure class being fixed here. It now also accepts a 404 whose message is exactly "Not found.", the catch-all's wording, which a controller's "<Resource> not found." never matches. 5. Two uncovered paths: a 404 with a non-object body (an HTML 404 from a proxy) must not retry, and the cache self-heal via a catch-all 404 — which is the branch that actually runs in production, since a live pre-4.2 box never returns 405. 6. smoke and diagnostics still called bare dotenv config() with their own credential handling, so which config won depended on file ordering within a reused jest worker. Both migrated onto helpers.ts, which also gives them the loud skip warning and the trailing-slash strip. Verified against live Coolify 4.1.2: 9 passed, 6 skipped as v4.2-only.
1 participant
Closes #292. Reported by @StreamlinedStartup, who did the legwork and identified every affected call site.
Problem
Coolify v4.2 (coollabsio/coolify#10872) requires
POSTon state-changing endpoints and returns a hard405forGET. Six call sites incoolify-client.tsstill sentGET:startService/stopService/restartServicecontrol(services)deployByTagOrUuiddeploy, including deploy-and-waitenableApi/disableApisystem(enable_api,disable_api)validateServervalidate_servervalidateServerwas the sneaky one: it passed nomethodat all, so it defaulted toGETand was invisible to a grep formethod: 'GET'.Would fixing it break Coolify 4.1?
The obvious fix is "change them all to POST", which would break every pre-4.2 instance. So I pulled upstream's
routes/api.phpat v4.1.2, v4.0.0, beta.420 and beta.350 and diffed againstv4.xHEAD. It splits cleanly in two.Four sites need no compatibility work. These were already
Route::match(['get', 'post'], ...)well before v4.2, so they now send POST unconditionally:/services/{uuid}/start,/stop,/restart/deployThis is proven rather than assumed: application and database start/stop/restart have been POSTing to the identically-shaped routes against 4.1 instances since long before this PR.
Three sites genuinely diverge. GET-only up to v4.1.2, POST-only from v4.2, so no single method works everywhere:
/enable,/disableRoute::getRoute::post/servers/{uuid}/validateRoute::getRoute::postApproach for the diverged three
Send POST, and on a
405retry once with GET, caching the resolved method per endpoint so the extra round trip is paid at most once per endpoint rather than per call.Why the retry is safe: a
405is raised by the router before the controller runs, so nothing executed and there is no way to double-fire a state change. Only405triggers the fallback. Every other status, including500, propagates untouched rather than being retried, since a500may mean the action partially ran. A failed GET retry is deliberately not cached, so the next call re-probes instead of trusting an unproven fallback.I considered probing
/versionand branching on>= 4.2, but that costs a startup round trip and version strings get messy with nightlies and self-built instances. Keying off observed behaviour beats keying off a claimed version.request()now throwsCoolifyApiErrorcarrying the HTTP status, because the fallback has to distinguish405from everything else and the message text alone could not. The message is byte-identical to before, so anything matching onerror.messageis unaffected.On the
?latest=truequestion raised in the issue: it stays a query param. Upstream reads it via$request->boolean('latest'), which draws from the unified input bag, so it works on POST unchanged.Hidden secrets
v4.2 (coollabsio/coolify#9893) strips sensitive fields unless the token has sensitive-read scope.
PrivateKey.private_key,EnvironmentVariable.valueandEnvVarSummary.valuewere typed as required, so a withheld secret arrived asundefinedbehind a type promising astringand flowed downstream silently. All three are now optional.Create*Requesttypes are deliberately unchanged: v4.2 hides fields in responses, and those are outbound payloads the caller supplies. I checked the other secret-shaped required fields (CreateGitHubAppRequest.client_secret,CreateCloudTokenRequest.token,CreatePrivateKeyRequest.private_key) and they are all request types, so they correctly stay required.Also checked the nested server logdrain and sentinel fields the issue mentioned: our
ServerSettingstype only declares theis_*_enabledbooleans, which are toggles rather than secrets, so there is nothing to loosen there.Error hints
405previously surfaced as a bareHTTP 405: Method Not Allowedwith no explanation. It now points at the GET-to-POST move. The401/403hint gained a note that v4.2 Member-role tokens are read-only, which is the likely cause of a403appearing right after an upgrade.Testing
GETand were updated to assertPOST.post_requiredbody and a pre-4.2 Laravel 405 body, non-retry on 404 and 500, propagation when both methods fail, and no caching when the GET retry fails.npm run lint0 errors,npm run check:spec-driftOK (98 routes),npm run buildclean,prettier --checkclean.Spec-drift matching is HTTP-method-blind, so the method changes do not affect it.
Not included
The additive v4.2 surface area from the issue (tags,
/move, destinations, service application management, database and service log endpoints) is deliberately out of scope so the compatibility fix can ship on its own. Raising separate issues for those.