You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Server design-review small findings that don't warrant individual issues (same convention as haverstack/core#69).
1. BASE_URL is loaded, documented, and used by nothing
src/config.ts:60 reads it, the README documents it ("auto-detected" — no detection exists either), and no code consumes config.baseUrl. Dead config advertised to operators. Either use it (natural home: a baseUrl field in /.well-known/stack so clients can learn their canonical origin behind proxies) or delete it from config + README.
2. POST /records/query: docs say auth Optional, code says required
docs/api.md lists it as Optional auth; the handler has requireAuth() (src/routes/records.ts:159). One is wrong. GET /records serves anonymous callers their public-record subset, and the spec describes /records/query as a superset of the same query surface — consistency says optional auth (anonymous → asEntity(null)), but deciding the other way just means fixing the doc. Pin the answer in a test either way.
3. POST /tokens fabricates createdAt and trusts entityId blind
The response's createdAt: new Date().toISOString() is invented at the route (src/routes/tokens.ts:28) rather than read back from the store — trivially divergent from what GET /tokens later reports. Return the stored row.
4. ENTITY_ID mismatch on an existing DB is silently ignored
Documented behavior ("ignored once the database exists"), but silently is the problem: an operator who changes ENTITY_ID expecting it to take effect gets no signal. Log a warning at startup when the env var is set and differs from the stack's stored ownerEntityId.
5. Discovery falls back to an empty-string entityId
entityId: ctx.stack.ownerEntityId ?? '' (src/routes/wellknown.ts:11) — core's ownerEntityId getter returns string, so the fallback is unreachable; if it ever weren't, "" is the worst possible answer (clients cache it as an identity per spec §Discovery). Drop the fallback; fail loudly if it can be absent.
6. Shutdown has no deadline
shutdown() waits for server.close() (src/index.ts:26-34), which waits for open keep-alive connections — potentially forever; under Docker that means SIGKILL at the grace-period boundary with no flush(). Add a timeout (e.g. 10 s) after which connections are destroyed and flush/close run anyway; also flush() on the fatal-error path.
Work items
BASE_URL: implement in discovery or remove
/records/query auth: decide, align code+docs, test
Server design-review small findings that don't warrant individual issues (same convention as haverstack/core#69).
1.
BASE_URLis loaded, documented, and used by nothingsrc/config.ts:60reads it, the README documents it ("auto-detected" — no detection exists either), and no code consumesconfig.baseUrl. Dead config advertised to operators. Either use it (natural home: abaseUrlfield in/.well-known/stackso clients can learn their canonical origin behind proxies) or delete it from config + README.2.
POST /records/query: docs say auth Optional, code says requireddocs/api.mdlists it asOptionalauth; the handler hasrequireAuth()(src/routes/records.ts:159). One is wrong.GET /recordsserves anonymous callers their public-record subset, and the spec describes/records/queryas a superset of the same query surface — consistency says optional auth (anonymous →asEntity(null)), but deciding the other way just means fixing the doc. Pin the answer in a test either way.3.
POST /tokensfabricatescreatedAtand trustsentityIdblindcreatedAt: new Date().toISOString()is invented at the route (src/routes/tokens.ts:28) rather than read back from the store — trivially divergent from whatGET /tokenslater reports. Return the stored row.body.entityIdis accepted as any string — no format check, no existence/_entitylookup, no warning. An owner typo mints a token for an entity that can never match a grant. At minimum validate non-empty string shape now; becomes a DID validation under Core sync: DID identity — challenge–response token issuance, DID entityIds in config and discovery (core #49) #42.4.
ENTITY_IDmismatch on an existing DB is silently ignoredDocumented behavior ("ignored once the database exists"), but silently is the problem: an operator who changes
ENTITY_IDexpecting it to take effect gets no signal. Log a warning at startup when the env var is set and differs from the stack's storedownerEntityId.5. Discovery falls back to an empty-string entityId
entityId: ctx.stack.ownerEntityId ?? ''(src/routes/wellknown.ts:11) — core'sownerEntityIdgetter returnsstring, so the fallback is unreachable; if it ever weren't,""is the worst possible answer (clients cache it as an identity per spec §Discovery). Drop the fallback; fail loudly if it can be absent.6. Shutdown has no deadline
shutdown()waits forserver.close()(src/index.ts:26-34), which waits for open keep-alive connections — potentially forever; under Docker that means SIGKILL at the grace-period boundary with noflush(). Add a timeout (e.g. 10 s) after which connections are destroyed and flush/close run anyway; alsoflush()on the fatal-error path.Work items
BASE_URL: implement in discovery or remove/records/queryauth: decide, align code+docs, testcreatedAt; entityId shape validationRefs #42 (DID validation supersedes 3b), #33 (any new error responses use the wire body).