Warn when the Corgea webapp is older than this CLI requires - #151
Warn when the Corgea webapp is older than this CLI requires#151Ibrahimrahhal wants to merge 3 commits into
Conversation
Every authenticated command now reads GET /api/version before running and warns when the deployment is behind MIN_WEBAPP_VERSION (v1.71.3), so users learn why a command may misbehave instead of hitting an opaque failure. The check is best effort and never blocks: a 404 (webapp predating the endpoint), a null version, an unreachable endpoint, or a version with no numbers in it all leave the command to run silently. Deployment versions carry suffixes (v1.71.3-beta, v1.71.3-client-a), so only the leading major.minor.patch is compared -- a suffixed build counts as the release it was cut from rather than sorting below it the way semver pre-release ordering would. CORGEA_MIN_WEBAPP_VERSION overrides the floor and CORGEA_SKIP_WEBAPP_VERSION_CHECK skips the check entirely. Co-authored-by: ibrahim <ibrahim@corgea.com>
There was a problem hiding this comment.
Automated review risk: 4/5.
The advisory check can delay every authenticated command for 150 seconds and fails to warn the oldest unsupported deployments. Version parsing also accepts malformed values.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
The pre-flight inherited the shared client's 150s timeout, so a deployment that answered /api/v1/verify but stalled /api/version delayed the command the user actually asked for by two and a half minutes -- for a check that is advisory and whose failure is meant to cost nothing. Give it 5s of its own. Measured against a stub that accepts /api/version and never replies: 150.02s before, 5.02s after. Co-authored-by: ibrahim <ibrahim@corgea.com>
Matching a dotted number anywhere let a decorated value such as build-2026.08-v1.70.0 parse as 2026.8.0, which silently suppressed the warning an outdated webapp should have produced. Anchor the pattern and require the numbers to end the string or start a suffix, so a shape we do not recognize is unknown rather than optimistically new. Also record why a 404 stays silent: the endpoint ships in a release above MIN_WEBAPP_VERSION, so warning on it would flag every deployment sitting exactly at the floor. Co-authored-by: ibrahim <ibrahim@corgea.com>
| let response = client.get(&url).timeout(WEBAPP_VERSION_TIMEOUT).send()?; | ||
| let status = response.status(); | ||
|
|
||
| if status == StatusCode::NOT_FOUND { |
There was a problem hiding this comment.
[P1] The documented rollout makes the default check unable to warn any normally-versioned outdated deployment. This code says /api/version first ships in a release above v1.71.3: therefore every real release below the v1.71.3 floor takes this 404 → Ok(None) path and stays silent, while every release new enough to have the endpoint is already above the floor and also stays silent. The passing outdated test only works by giving a server with the new endpoint the artificial older value v1.70.0, a state the stated release sequence does not produce. As shipped, the users this task targets still receive no warning. Preserve 404 as a distinct result and emit an accurately worded “compatibility could not be verified; this CLI requires v1.71.3+” warning (the existing skip variable can suppress it), or ensure the endpoint is deployed/backported at or below the minimum before enabling this floor.
There was a problem hiding this comment.
I agree with this finding and think it should be addressed.
high: Default check cannot detect normally deployed outdated webapps
The endpoint first ships above v1.71.3, while v1.71.3 is the minimum. Thus every normal release below the minimum returns 404 and is treated as unknown, while releases exposing the endpoint are already above the minimum. The outdated test models an artificial v1.70.0 deployment that nevertheless exposes the newer endpoint. Delay enabling this floor until the endpoint release is at or below the minimum, or distinguish 404 and provide an accurate unverifiable-compatibility warning.
Proof or reproduction:
// Current behavior for every normally deployed version below v1.71.3:
if status == StatusCode::NOT_FOUND {
return Ok(None);
}
// warn_if_webapp_outdated then returns without warning on Ok(None).
There was a problem hiding this comment.
Automated review risk: 3/5.
The implementation is safe for commands but its default compatibility warning is effectively inert under the documented endpoint rollout.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
What this does
Every authenticated command now reads
GET /api/versionright after verifying the token and, when the deployment is behind the CLI's minimum, prints a warning before running:The floor lives in
src/version_check.rsasMIN_WEBAPP_VERSION, defaulting tov1.71.3. Raise it whenever the CLI starts depending on a webapp change.The endpoint is added by Corgea/doghouse#1860; this change works against webapps with and without it.
Comparing versions
Deployment versions are not plain semver. Releases ship as
v1.71.3, while pre-release and per-customer builds add a suffix:v1.71.3-beta,v1.71.3-client-a. So only the leadingmajor.minor.patchis compared, via an anchored^v?(\d+)\.(\d+)(?:\.(\d+))?(?:$|[-+]). Two things follow from that:1.71.3-betabelow1.71.3and warn about a webapp that is in fact current.build-2026.08-v1.70.0as2026.8.0and suppress a warning that should fire.A missing patch reads as
.0, sov1.71andv1.71.0compare equal.Never blocks a command
The check is best effort and only ever adds a warning. Every one of these leaves the command running exactly as before, with nothing on stderr beyond a
--debugline:404(it predates the endpoint)"version": null(it cannot determine its own version)Two details that make "never blocks" true rather than aspirational:
/api/v1/verifybut stalls this route cannot hold the user's command for two and a half minutes.check_for_warnings, so a pre-flight can never be what terminates the process. The command's own requests still surface deprecation signals.Why a 404 does not warn
Worth stating explicitly, because it looks like a missed case. The endpoint ships in a release above
MIN_WEBAPP_VERSION— doghouse's current tag isv1.71.3and #1860 is on top of it. So a 404 only narrows the webapp to "older than the release that added the route", a range that includes deployments running exactlyv1.71.3, which satisfy the floor. Warning on 404 would tell correctly-configured customers to upgrade, which is worse than staying quiet. The reasoning is recorded in the code so it is not "fixed" later.The consequence is that the warning is inert on old self-hosted deployments until they pick up the release carrying the endpoint, and it self-corrects from there. The clean way to close that gap later is a second constant recording the first webapp release that serves
/api/version; once the floor is raised above it, a 404 does prove non-compliance and can warn.Escape hatches
CORGEA_MIN_WEBAPP_VERSIONCORGEA_SKIP_WEBAPP_VERSION_CHECK=1The second exists for anyone deliberately pinned to an older self-hosted webapp who does not want the warning on every command.
Testing
./harness checkpasses: clippy (strict), format, and 653 tests.Unit tests in
src/version_check.rscover number extraction from plain and suffixed versions, a missing patch, decorated strings that must read as unknown (build-2026.08-v1.70.0,corgea 1.70.0,1.71.3.4), whitespace tolerance, the comparison boundary, and the warning's contents.tests/cli_webapp_version.rsdrives the real binary against a stubbed webapp for: an outdated webapp warning and still running the command, a current webapp staying silent, suffixed builds comparing on their numbers in both directions, a 404, a null version, a stalled endpoint, and the skip flag suppressing both the warning and the request.The stall test asserts elapsed time, not just success, so it fails if the per-request timeout is ever dropped. Verified by temporarily reverting the fix: 150.02s without it, 5.02s with it.
The cloud contract tests in
tests/cloud_commands_e2e/pin the exact request sequence of each command, so they gained an explicitwebapp_version_request()expectation afterverify_request(). It answers with a null version, which keeps those contracts independent of the CLI's minimum-version floor.Verified live against a real doghouse
The binary was pointed at a real doghouse dev server (Postgres, migrated, real token) running the Corgea/doghouse#1860 branch, restarting it for each case since the app caches its resolved version. Re-run after the anchoring change with identical results:
v1.70.0v1.71.3v1.71.3-client-av1.71.2-betav2.0.0v1.69.0(from theAPP_VERSIONfallback)null(webapp could not determine it)The 404 path was checked the same way, by removing the route from the running app so it answers like a webapp released before the endpoint existed. Even with a baked version of
v1.70.0— behind the floor — the CLI stayed silent, saying only underCORGEA_DEBUG=1: