Skip to content

fix(server): make the runtime speak the 0.9.0 backend vocabulary it declares - #29

Merged
andrei-hasna merged 2 commits into
mainfrom
b89b27db-runtime
Aug 3, 2026
Merged

fix(server): make the runtime speak the 0.9.0 backend vocabulary it declares#29
andrei-hasna merged 2 commits into
mainfrom
b89b27db-runtime

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

The defect

PR #28 migrated the contract manifest to @hasna/contracts 0.9.0 (storage.modestorage.backend, kitVersion 0.9.0). That half is correct and mandatory. The runtime was not touched, so economy declared the new vocabulary and did the old one.

Measured on main @ bc6f7cc with the contract's own HealthResponseSchema — the exact instrument the health_shape gate runs:

backend: Required
<root>: Unrecognized key(s) in object: 'mode', 'service'
GET /health keys: [ "status", "version", "mode", "service" ]

Two 0.9.0 conformance gates were failing:

gate requirement
health_shape a sampled /health matches { status, version, backend }
server_backend_configuration DATABASE_URL presence selects postgresql; retired mode variables fail closed

What changed, per site

  • src/db/cloud.tsisCloudMode() branched on HASNA_ECONOMY_STORAGE_MODE, which is precisely the "normalized or silently mapped" behaviour CONTRACT.md §2 bans. Replaced with resolveEconomyServerBackend(): rejection delegated to the contract's assertNoLegacyStorageMode (so the migration hint matches the gate verbatim), backend selected from DSN presence alone.
  • src/server/serve.ts — the three probes shared one envelope, which is what leaked mode into all of them. Split into the §4 shapes: /health {status,version,backend}, /version {version}, /ready {ready,reason?}. HealthResponseSchema is strict, so the service key had to go too.
  • src/server/index.tsversion subcommand reports backend; fails closed with the hint instead of a stack trace.
  • openapi/economy.json (+ regenerated src/openapi.ts) — shared Foundation schema split into strict Health/Ready/Version. Edited at the JSON source and regenerated via scripts/sync-openapi.ts; the .ts is a generated mirror and was not hand-edited.
  • Dockerfile, Dockerfile.runtime, docker-compose.yml — removed the baked-in HASNA_ECONOMY_STORAGE_MODE=cloud. Required, not cosmetic: with the guard failing closed, a surviving variable refuses to boot the container. Every affected service already sets HASNA_ECONOMY_DATABASE_URL.

One passing assertion was deleted, deliberately

src/server/serve.test.ts asserted health.mode was defined, and it passed. Owner directive 2026-07-29 removed the deployment-mode axis (no mode enums, no mode branching, no mode vocabulary) and 0.9.0 retired the variables outright, so a test demanding a mode key pins the defect rather than the contract. It is replaced by schema conformance against the contract's own schemas, not merely dropped.

A design decision worth reviewing

The contract's own resolveServerDataBackend reads HASNA_ECONOMY_DATABASE_URL and ECONOMY_DATABASE_URLnot the bare DATABASE_URL, which economy has always honoured and which the migrate command documents. Deferring to it wholesale would silently downgrade such a deployment to sqlite: a wrong backend reported as healthy. So rejection is delegated to the package while DSN resolution stays local, and there is a test pinning the alias.

Evidence

Regression tests written first, both failing at the parent:

  • src/server/foundation-probe.contract.test.ts — asserts payloads against HealthResponseSchema / ReadyResponseSchema / VersionResponseSchema. It imports nothing this fix introduces, so it runs and fails on payload value at bc6f7cc rather than dying at import. (First draft did die at import; that is a weaker signal and was fixed.)
  • src/db/server-backend.contract.test.ts — pins backend selection, the bare DATABASE_URL alias, and fail-closed rejection of all four retired variables. Rejection assertions match /was removed/ rather than bare .toThrow(), which a missing export would also satisfy.
parent bc6f7cc:  0 pass,  5 fail   (on value, not import)
head:           14 pass,  0 fail
full suite:    376 pass,  0 fail   (44 files)
tsc --noEmit:  clean

Live server, not just unit tests:

GET /health   {"status":"ok","version":"0.3.8","backend":"sqlite"}
GET /version  {"version":"0.3.8"}
GET /ready    {"ready":true}
DATABASE_URL=postgres://…  ->  {"status":"ok","version":"0.3.8","backend":"postgresql"}
HASNA_ECONOMY_STORAGE_MODE=cloud  ->  rc=1
  "HASNA_ECONOMY_STORAGE_MODE was removed. Delete the storage-mode variable; set
   HASNA_ECONOMY_DATABASE_URL to select the postgresql server backend, or leave it
   unset for sqlite."

Scope — the client half is NOT in this PR

The client transport (src/lib/cloud-storage.ts, src/lib/contracts-client/, src/mcp/server.ts, docs/configuration.md) still reads the retired variables. 0.9.0 retires them on that axis too — its assertNoLegacyClientMode rejects identically — so this work is real and remains open under task b89b27db.

It is deliberately excluded because it changes CLI behaviour for every operator whose environment sets HASNA_ECONOMY_STORAGE_MODE, and would fail closed on their machines. src/lib/cloud-storage.test.ts carries 8 currently-passing assertions that a migration must delete. That is a different blast radius and deserves its own review rather than riding along here.

What was NOT checked

  • No PostgreSQL server was exercised; backend: "postgresql" was verified from DSN resolution only, not by connecting.
  • The contracts conformance CLI was not run end to end; gate satisfaction is demonstrated against the same schemas and resolver the gate imports.
  • No container was built or run; the Dockerfile change is reasoned from the fail-closed guard, not from a rebuilt image.

Refs: todos b89b27db


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…eclares

PR #28 migrated the contract MANIFEST to @hasna/contracts 0.9.0 (storage.mode ->
storage.backend) but left the runtime untouched, so economy DECLARED the new
vocabulary and DID the old one. Measured on main @ bc6f7cc with the contract's
own HealthResponseSchema:

  backend: Required
  <root>: Unrecognized key(s) in object: 'mode', 'service'
  GET /health keys: [ "status", "version", "mode", "service" ]

Both 0.9.0 conformance gates were failing:
  health_shape                   -- /health must match { status, version, backend }
  server_backend_configuration   -- DATABASE_URL selects postgresql; retired mode
                                    variables fail closed

Changes, per site:

- src/db/cloud.ts: isCloudMode() branched on HASNA_ECONOMY_STORAGE_MODE, which is
  exactly the "normalized or silently mapped" behaviour CONTRACT.md section 2
  bans. Replaced with resolveEconomyServerBackend(), which delegates rejection to
  the contract's assertNoLegacyStorageMode (so the migration hint matches the gate
  verbatim) and selects the backend from DSN presence alone. DSN resolution stays
  local because economy also honours the bare DATABASE_URL alias, which the
  contract's own resolver does not read -- deferring wholesale would silently
  downgrade such a deployment to sqlite.

- src/server/serve.ts: the three foundation probes shared one envelope, which is
  what leaked `mode` into all of them. Split into the CONTRACT.md section 4
  shapes: /health { status, version, backend }, /version { version },
  /ready { ready, reason? }. HealthResponseSchema is strict, so the `service` key
  had to go too.

- src/server/index.ts: `version` subcommand reported the retired mode; now reports
  backend, and fails closed with the migration hint rather than a stack trace.
  Help text updated.

- openapi/economy.json (+ regenerated src/openapi.ts): the shared Foundation
  schema became strict Health / Ready / Version schemas. Edited at the JSON
  source and regenerated via scripts/sync-openapi.ts, not hand-edited.

- Dockerfile, Dockerfile.runtime, docker-compose.yml: removed the baked-in
  HASNA_ECONOMY_STORAGE_MODE=cloud. This is REQUIRED, not cosmetic -- with the
  guard now failing closed, a surviving variable would refuse to boot the
  container. The backend is selected by HASNA_ECONOMY_DATABASE_URL, which every
  affected service already sets.

Deleted a PASSING assertion, deliberately: src/server/serve.test.ts asserted
`health.mode` was defined. Owner directive 2026-07-29 removed the deployment-mode
axis (no mode enums, no mode branching, no mode vocabulary), and @hasna/contracts
0.9.0 retired the variables outright, so a test demanding a `mode` key pins the
defect rather than the contract. It is replaced by schema conformance against the
contract's own schemas, not merely dropped.

Regression tests first, both failing at the parent commit:
- src/server/foundation-probe.contract.test.ts asserts the probe payloads against
  HealthResponseSchema / ReadyResponseSchema / VersionResponseSchema -- the exact
  instrument the health_shape gate runs. It imports nothing this fix introduces,
  so it RUNS and fails on the payload value at bc6f7cc rather than dying at import.
- src/db/server-backend.contract.test.ts pins backend selection, the bare
  DATABASE_URL alias, and fail-closed rejection of all four retired variables. The
  rejection assertions match /was removed/ rather than bare .toThrow(), which a
  missing export would also satisfy.

Verified: 5/5 fail at parent on value, 14/14 pass at head, full suite 376 pass /
0 fail, tsc --noEmit clean. Live server confirms {"status":"ok","version":"0.3.8",
"backend":"sqlite"}, bare DATABASE_URL yields "backend":"postgresql", and a
retired variable exits 1 with the migration hint.

Scope: this closes the SERVER half. The CLIENT transport half (src/lib/
cloud-storage.ts, src/lib/contracts-client/, src/mcp/server.ts) still reads the
retired variables and is deliberately left to a separate PR -- it changes CLI
behaviour for every operator whose environment sets HASNA_ECONOMY_STORAGE_MODE,
which is a different blast radius that deserves its own review.

Refs: todos b89b27db

Agent: Silvanus
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #29 @ 12f4d84 — lens: boot-safety+client-boundary, reviewer economy-pr29-reviewer (1 of 1)

Reviewed in two fresh worktrees pinned to 12f4d84b5dfee42829394ae4e0ef4b0cc55a968e (head) and bc6f7cc1ef9cea2c61441acb67c1b4a830e4d0f1 (parent), both rev-parse verified. No claim below is taken from the PR body.

Verdict

GO. The change does exactly what it says, the fail-closed path is loud and self-serviceable, and the excluded client half is a defensible boundary — I tried to break it and could not. Four residuals, none blocking; two are one-line doc edits I recommend landing here before un-drafting.


1. Both test arms, re-run independently

Head arm — reproduces the claim exactly:

 14 pass
 0 fail
 17 expect() calls
Ran 14 tests across 2 files. [373.00ms]

Parent arm — does NOT match the claim. The two new files were copied into the parent tree and run there:

 0 pass
 6 fail
 1 error
 8 expect() calls
Ran 6 tests across 2 files. [329.00ms]

0 pass / 6 fail / 1 error, not 0 pass / 5 fail. The two files also fail for different reasons:

  • foundation-probe.contract.test.ts5 fail on VALUE, exactly as claimed. This is a genuinely strong regression test:
    - []
    + [
    +   "backend: Required",
    +   "<root>: Unrecognized key(s) in object: 'mode', 'service'",
    + ]
    
    Expected to not contain: "mode"
    Received: [ "status", "version", "mode", "service" ]
    
  • server-backend.contract.test.tsdies at import:
    SyntaxError: Export named 'resolveEconomyServerBackend' not found in module '.../src/db/cloud.ts'.
    

That is unavoidable for a test pinning a new export, and the author mitigated it correctly (/was removed/ matchers, so it cannot pass vacuously). But the summary block presents the aggregate as "(on value, not import)", which is true of only one of the two files. Evidence-reporting fix, not a code defect.

Full suite and typecheck reproduce:

 376 pass
 0 fail
 1600 expect() calls
Ran 376 tests across 44 files. [40.81s]

bunx tsc --noEmit → rc=0, stdout length 0, stderr length 0.


2. Boot-breakage enumeration — in-repo surface is complete and clean

Every in-repo site that could set a retired variable, all four names (HASNA_ECONOMY_STORAGE_MODE, HASNA_ECONOMY_MODE, ECONOMY_STORAGE_MODE, ECONOMY_MODE):

site state at head
Dockerfile, Dockerfile.runtime fixed — removed; only an explanatory comment remains
docker-compose.yml (both services) fixed
.github/workflows/ci.yml never set one — rc=1, positive control 23 matches on same file
.github/workflows/release-menubar.yml never set one — rc=1, positive control 19 matches on same file
.env.example / ECS / terraform / k8s / helm / systemd none tracked — 0 hits across 221 tracked files, positive control 121
test files (mcp, cli, gatherer, cloud-storage) set them, all green in the 376
docs/configuration.md instructs operators to set it — see F1

The migration hint is good enough to self-serve. Measured, not reasoned — economy serve with the variable set:

economy: HASNA_ECONOMY_STORAGE_MODE was removed. Delete the storage-mode variable; set HASNA_ECONOMY_DATABASE_URL to select the postgresql server backend, or leave it unset for sqlite.

rc=1, one line, no stack trace, names the variable, the removal, and both replacement paths.

Cleared, so nobody re-checks them: no container HEALTHCHECK parses the probes (the only healthcheck: in compose is pg_isready on the postgres service). The bundled dashboard consumes none of /health, /version, /ready, mode, backend — 2 hits across 51 tracked dashboard files under a deliberately loose pattern, both the string Bundler mode in tsconfig comments. isCloudMode has exactly one surviving reference repo-wide and it is a comment, so the rename leaves nothing dangling.


3. The client boundary — defensible, and I tested the specific way it could have been unsafe

The half-state is real and I reproduced it in one shell, same binary, same variable:

HASNA_ECONOMY_STORAGE_MODE=cloud  economy serve   -> rc=1  "...was removed..."
HASNA_ECONOMY_STORAGE_MODE=cloud  economy today   -> rc=0  renders normally

So one binary answers two contracts for one variable name. The question is whether that is unsafe, and the concrete hazard I went after is: an operator follows the hint, deletes the variable, and their CLIENT silently downgrades from cloud to local — a silent wrong store, which is precisely what CONTRACT.md §2 exists to prevent.

It does not happen. Probing resolveEconomyCloudStorage directly across four configurations:

A url+key+STORAGE_MODE=cloud  -> active=true
B url+key, no storage mode    -> active=true
C STORAGE_MODE=cloud only     -> THREW: HASNA_ECONOMY_STORAGE_MODE=cloud but no API key is set (HASNA_ECONOMY_API_KEY). Refusing to route to cloud; using local store. Set HASNA_ECONOMY_API_KEY to enable the cloud client.
D nothing set                 -> active=false

B is the finding: deleting the variable per the hint leaves the client on cloud, because URL+key is independently sufficient. C is the only configuration where the variable is load-bearing, and it already throws loudly and did so before this PR.

That, plus the fact that the client half is byte-unchanged from main, means the intermediate state introduces no new silent-wrong-store path. Every newly reachable failure is loud, hinted, and pre-serving. The exclusion rationale also checks out: src/lib/cloud-storage.test.ts is 11 pass, 0 fail and genuinely depends on the retired variable, so it is a real and separate blast radius.

Judgement: server-fixed / client-divergent is a coherent intermediate state, not an unsafe half-state. Its cost is coherence and documentation, not safety — and the documentation half is F1 below.


4. The deleted assertion — coverage is stronger, not thinner

expect(data['mode']).toBeDefined() was replaced in place by expect(data['backend']).toBeDefined(), and additionally by strict schema conformance. The old assertion was "some key exists"; the new file asserts the exact key set via strict HealthResponseSchema, plus backend === 'sqlite' with no DSN, plus 'mode' absent. Strictly stronger. The only other /health reference in serve.test.ts (line 225) asserts HTTP 200 inside an auth test and never touched the payload.

5. The generated file matches its generator — verified, with a control

Ran bun scripts/sync-openapi.ts on a clean tree: rc=0, git status --short empty → committed src/openapi.ts matches generator output.

That check is only meaningful if the generator actually writes, so I proved it can fail:

=== status after perturbation (expect M) ===
 M src/openapi.ts
=== status after regen (empty == generator restored it) ===
=== end ===

The generator rewrote my perturbation away. The empty result is a real match, not a no-op.

6. The resolver divergence is sound

getCloudDatabaseUrl reads HASNA_ECONOMY_DATABASE_URLECONOMY_DATABASE_URLDATABASE_URL; the contract's own resolver omits the third. Adopting it wholesale would resolve a bare-DATABASE_URL deployment to sqlite and report it healthy — a silent wrong backend, the exact class the fail-closed guard exists to prevent. Delegating only the rejection keeps the migration hint verbatim-identical to the gate's. Correct call, and docs/configuration.md confirms DATABASE_URL is a documented server alias.


Findings

F1 · P1 · docs/configuration.md:65 — non-blocking, but please fix in this PR (one line, inside your own blast radius).
The line sits under the heading ## Self-hosted server — server documentation, not the client half the PR scoped out:

The server switches to direct Postgres mode when HASNA_ECONOMY_STORAGE_MODE=cloud or when a DSN is present:

After this PR that is not merely stale, it instructs the operator to perform the action that now refuses to boot the server. The exclusion boundary was drawn at the file, but this line belongs to the half you changed. Not a blocker only because the resulting failure is loud and the hint is self-serviceable (measured above). Line 44 in the same file is client-scoped and correctly stays for task b89b27db.

F2 · P2 · docs/rest-api.md:19`GET` | `/version`, `/v1/version` | Version and deployment mode. /version now returns { version } only. Same one-line class as F1.

F3 · P2 · economy-serve migrate does not fail closed. Measured with the retired variable set:

migrate_rc=1
stderr: connect ECONNREFUSED 127.0.0.1:59999

It goes straight to connecting; no guard, no hint. Pre-existing, therefore non-blockingrunMigrate is byte-identical at head and parent and never consulted the mode variable, so this PR neither introduces nor worsens it. No silent-wrong-store risk either: migrate's target is DSN-derived and the retired variable never affected it. Flagged because the compose header comment this PR adds — "a surviving one now fails closed at startup" — overstates for the economy-migrate service defined in that same file, and docker compose run --rm economy-migrate is the documented first step. A one-line resolveEconomyServerBackend() at the top of runMigrate would close it.

F4 · P2 · PR body evidence. Parent arm is 0 pass / 6 fail / 1 error; the "(on value, not import)" qualifier holds for foundation-probe only. Worth correcting so the next reader is not surprised — the mitigation you applied is the right one and deserves to be stated accurately.

F5 · P3 · observation, pre-existing. serve.ts:259 — the "legacy health alias" if (path === '/health') return ok({ status: 'ok', ts }) is unreachable: line 227 returns first for rawPath === '/health', and path differs from rawPath only for /v1/ prefixes (line 216). Dead before this PR too; the comment claiming the SPA depends on it is misleading.


What I did NOT check

  • Out-of-repo deployment definitions. I bounded the in-repo surface completely (table above) — but ECS task definitions, AWS console env, systemd units, or any infra repo living outside hasna/economy were not reachable from here. This remains UNMEASURED, exactly as the dispatch brief warned. Anything out there still setting a retired variable will stop booting, loudly, with the hint.
  • No PostgreSQL server was connected; backend: "postgresql" verified from DSN resolution only.
  • No container was built or run; the Dockerfile changes are reasoned from the guard plus the enumeration, not from a rebuilt image.
  • contracts conformance was not run end to end.
  • --show-secrets-style reveal paths and the /v1/* authenticated surface were out of lens and untouched by this diff.

… mode var

The self-hosted server section still said the server "switches to direct
Postgres mode when HASNA_ECONOMY_STORAGE_MODE=cloud". After this branch's
runtime change that is inverted: resolveEconomyServerBackend selects the
backend from the DSN alone and calls assertNoLegacyStorageMode first, so the
variable now stops the server at startup instead of configuring it.

Say what the runtime does -- backend follows the database URL, retired mode
variables fail closed with a migration hint -- and name the three sibling
aliases that fail the same way.

The CLI/MCP client section is deliberately untouched: resolveEconomyCloudStorage
is economy's own resolver and still honours the mode variable, so the client
documentation at line 44 remains accurate. The new paragraph says the fail-closed
behaviour is server-only so the two sections cannot be read as contradicting.

Also drops "cloud" from "a non-loopback cloud server", which was a dangling
reference to the mode the preceding line no longer describes.

Agent: Silvanus
@andrei-hasna
andrei-hasna marked this pull request as ready for review August 3, 2026 04:18
@andrei-hasna
andrei-hasna merged commit 21e3796 into main Aug 3, 2026
3 checks passed
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